This is an archive of the discontinued Mercurial Phabricator instance.

phabricator: ensure that `phabsend` is given a contiguous, linear commit range
ClosedPublic

Authored by mharbison72 on Apr 16 2020, 8:01 PM.

Details

Summary

Supplying a non-linear range was another orphan factory. While in theory there
could be a use case for skipping over garbage commits (like adding debugging)
and getting the valuable commits extracted out at the same time as posting a
review, it seems far more likely that specifying a non-linear range is a user
error. This is another case of issue6045, but predates both 0680b8a1992a and
601ce5392cb0.

Neither the --no-amend case nor resubmitting a previously submitted commit
would cause orphans. But for the sake of simplicity and to keep the parents
tracked on Phabricator in the proper state, ban missing commits unconditionally.

Diff Detail

Repository
rHG Mercurial
Branch
stable
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

mharbison72 created this revision.Apr 16 2020, 8:01 PM
marmoute added inline comments.
hgext/phabricator.py
1314

Not sure about the use of first and last here. Are the revs already sorted ? Did we filter set with multiple head/root already?

If so, it could be useful to mention the assertion in the inline comment. If not, we have a probleme.

(also, not that if revs is a smartset, you can use first() and last() on them IIRC.)

mharbison72 added inline comments.Apr 17 2020, 3:09 PM
hgext/phabricator.py
1314

Are the revs already sorted ?

Yes, see line 1298 above. (Which has test coverage IIRC)

Did we filter set with multiple head/root already?

We did not. I thought I saw something in evolve or another extension that required a linear set, and this is getting complicated enough that it probably deserves a utility function. But I think before doing this, we should get to the bottom of D8457 in case there are other ways to hit that.

(also, not that if revs is a smartset, you can use first() and last() on them IIRC.)

It's returned by scmutil.revrange() and has sort(), so I assume it is?

marmoute added inline comments.Apr 17 2020, 5:47 PM
hgext/phabricator.py
1314
Are the revs already sorted ?

Yes, see line 1298 above. (Which has test coverage IIRC)

Great

Did we filter set with multiple head/root already?

We did not.

Okay, then the current code will fail to detect non-linearity from other head or root.

As an interresting side effect, a non linear set will have more than one head and more than one root. So implementing root/head checking will reach your current goal for free.

You can do so using repo.revs('heads(%ld)') and repo.revs('heads(%ld)'). You could report all extra roots but the minimal one and/or all extra heads but the maximal ones.

(also, not that if revs is a smartset, you can use first() and last() on them IIRC.)

It's returned by scmutil.revrange() and has sort(), so I assume it is?

I doubled checked, it is.

1318

This error message does not specify the revision it it complaining about it. It would be better to do so. It make the error simpler to understand and resolve for the user.

mharbison72 added inline comments.Apr 17 2020, 11:25 PM
hgext/phabricator.py
1314

As an interresting side effect, a non linear set will have more than one head and more than one root.

Oh, clever!

This would still allow diamonds, which might be OK (aside from D8457).

You could report all extra roots but the minimal one and/or all extra heads but the maximal ones.

I thought I remembered pull or something listing the new heads, but putting a cap on the number of hashes reported. I can't find that code. But maybe we don't care here, because there shouldn't be *that* many revisions selected.

marmoute added inline comments.Apr 19 2020, 7:58 AM
hgext/phabricator.py
1314

I think you are looking for mercurial.scmutil.nodesummaries.

mharbison72 added inline comments.Apr 19 2020, 5:43 PM
hgext/phabricator.py
1314

Yes, that was it. Thanks.

marmoute requested changes to this revision.Apr 22 2020, 12:22 PM

The code looks good, but the message is a bit obscure, can you try finding something clearer for the end-users ?

hgext/phabricator.py
1324

I would be a bit more explicit. Maybe something like:

cannot phasend revisions group with multiple heads: %list-of-all-heads%

This revision now requires changes to proceed.Apr 22 2020, 12:22 PM
mharbison72 added inline comments.Apr 22 2020, 1:49 PM
hgext/phabricator.py
1324

I would be a bit more explicit. Maybe something like:
cannot phasend revisions group with multiple heads: %list-of-all-heads%

That message is less correct though, because it also doesn't want multiple roots either. I can't think of any other places where we talk about "linear" offhand, but doesn't that immediately paint a picture of the problem for even non-gurus?

marmoute added inline comments.Apr 24 2020, 11:30 AM
hgext/phabricator.py
1324

Well, ther would be one message for head and one message for roots. It is a bit anoying to have to solve two errors one after the other in the most complicated case. However the benefit of having a simpler message seems better.

mharbison72 added inline comments.Apr 24 2020, 4:38 PM
hgext/phabricator.py
1324

It's not clear to me that the individual messages will nudge the user in the right direction (I'm guessing with a 1+3 submission that complains about '1' being an extra head that some people will just drop '1' entirely). But I'm fine with it, with a mention of "linear" in the hint as a compromise.

marmoute added inline comments.Apr 24 2020, 6:10 PM
hgext/phabricator.py
1324

I don't ahve a strong opinion on the actual message as long as it is simple to understand and easily actionable. Your idea to mention linear(ity) in the hint seems good.

We can also start with something descent and improve it with suggestion later.

Gentle ping on this. I think I addressed the concerns raised, and didn't want it overlooked for the release if it's still in the changes requested state.

Gentle ping on this. I think I addressed the concerns raised, and didn't want it overlooked for the release if it's still in the changes requested state.

Did not we want to update the message ?

Gentle ping on this. I think I addressed the concerns raised, and didn't want it overlooked for the release if it's still in the changes requested state.

Did not we want to update the message ?

I did: https://phab.mercurial-scm.org/D8454?vs=21150&id=21216#toc

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

Gentle ping on this. I think I addressed the concerns raised, and didn't want it overlooked for the release if it's still in the changes requested state.

Did not we want to update the message ?

I did: https://phab.mercurial-scm.org/D8454?vs=21150&id=21216#toc

Now that I see it, I think we shoul dinclude all heads because the mssage might be confusing "multiple heads: one-hash"

Now that I see it, I think we shoul dinclude all heads because the mssage might be confusing "multiple heads: one-hash"

Heh, I didn't think of that. So are you saying that one of the previous diffs is OK, or don't filter out min/max head/root?

Now that I see it, I think we shoul dinclude all heads because the mssage might be confusing "multiple heads: one-hash"

Heh, I didn't think of that. So are you saying that one of the previous diffs is OK, or don't filter out min/max head/root?

I would says "Let's not filter min/max root/head". What do you think ?