This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8454

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

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

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHGc1c922391314: phabricator: ensure that `phabsend` is given a contiguous, linear commit range
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mharbison72 created this revision.Apr 16 2020, 8:01 PM
marmoute added a subscriber: marmoute.Apr 17 2020, 2:26 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.

mharbison72 added a comment.May 1 2020, 9:59 AM

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.

marmoute added a comment.May 1 2020, 10:08 AM
In D8454#126857, @mharbison72 wrote:

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 ?

mharbison72 added a comment.May 1 2020, 10:24 AM
In D8454#126862, @marmoute wrote:
In D8454#126857, @mharbison72 wrote:

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

mharbison72 added a commit: rHGc1c922391314: phabricator: ensure that `phabsend` is given a contiguous, linear commit range.May 1 2020, 12:02 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGc1c922391314: phabricator: ensure that `phabsend` is given a contiguous, linear commit range (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.
marmoute added a comment.May 2 2020, 12:36 PM
In D8454#126863, @mharbison72 wrote:
In D8454#126862, @marmoute wrote:
In D8454#126857, @mharbison72 wrote:

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"

mharbison72 added a comment.May 2 2020, 4:34 PM
In D8454#126891, @marmoute wrote:

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?

marmoute added a comment.May 2 2020, 5:37 PM
In D8454#126892, @mharbison72 wrote:
In D8454#126891, @marmoute wrote:

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 ?

Revision Contents
Changeset List

PathPackages
Mhgext/phabricator.py (27 lines)
Mtests/test-phabricator.t (10 lines)

Diff 21265

hgext/phabricator.py

if opts.get(b'amend'): if opts.get(b'amend'):
cmdutil.checkunfinished(repo) cmdutil.checkunfinished(repo)
ctxs = [repo[rev] for rev in revs] ctxs = [repo[rev] for rev in revs]
if any(c for c in ctxs if c.obsolete()): if any(c for c in ctxs if c.obsolete()):
raise error.Abort(_(b"obsolete commits cannot be posted for review")) raise error.Abort(_(b"obsolete commits cannot be posted for review"))
# Ensure the local commits are an unbroken range. The semantics of the
# --fold option implies this, and the auto restacking of orphans requires
# it. Otherwise A+C in A->B->C will cause B to be orphaned, and C' to
# get A' as a parent.
def _fail_nonlinear_revs(revs, skiprev, revtype):
marmouteUnsubmitted
Not Done

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.)

marmoute: Not sure about the use of `first` and `last` here. Are the `revs` already sorted ? Did we…
mharbison72AuthorUnsubmitted
Done

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?

mharbison72: > Are the revs already sorted ? Yes, see line 1298 above. (Which has test coverage IIRC) >…
marmouteUnsubmitted
Not Done
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.

marmoute: > > Are the revs already sorted ? > Yes, see line 1298 above. (Which has test coverage IIRC)…
mharbison72AuthorUnsubmitted
Done

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.

mharbison72: > As an interresting side effect, a non linear set will have more than one head and more than…
marmouteUnsubmitted
Not Done

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

marmoute: I think you are looking for `mercurial.scmutil.nodesummaries`.
mharbison72AuthorUnsubmitted
Done

Yes, that was it. Thanks.

mharbison72: Yes, that was it. Thanks.
badnodes = [repo[r].node() for r in revs if r != skiprev]
raise error.Abort(
_(b"cannot phabsend multiple %s revisions: %s")
% (revtype, scmutil.nodesummaries(repo, badnodes)),
marmouteUnsubmitted
Not Done

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.

marmoute: This error message does not specify the revision it it complaining about it. It would be better…
hint=_(b"the revisions must form a linear chain"),
)
heads = repo.revs(b'heads(%ld)', revs)
if len(heads) > 1:
_fail_nonlinear_revs(heads, heads.max(), b"head")
marmouteUnsubmitted
Not Done

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

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

marmoute: I would be a bit more explicit. Maybe something like: `cannot phasend revisions group with…
mharbison72AuthorUnsubmitted
Done

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?

mharbison72: > I would be a bit more explicit. Maybe something like: > > `cannot phasend revisions group…
marmouteUnsubmitted
Not Done

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.

marmoute: Well, ther would be one message for head and one message for roots. It is a bit anoying to have…
mharbison72AuthorUnsubmitted
Done

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.

mharbison72: It's not clear to me that the individual messages will nudge the user in the right direction…
marmouteUnsubmitted
Not Done

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.

marmoute: I don't ahve a strong opinion on the actual message as long as it is simple to understand and…
roots = repo.revs(b'roots(%ld)', revs)
if len(roots) > 1:
_fail_nonlinear_revs(roots, roots.min(), b"root")
fold = opts.get(b'fold') fold = opts.get(b'fold')
if fold: if fold:
if len(revs) == 1: if len(revs) == 1:
# TODO: just switch to --no-fold instead? # TODO: just switch to --no-fold instead?
raise error.Abort(_(b"cannot fold a single revision")) raise error.Abort(_(b"cannot fold a single revision"))
# There's no clear way to manage multiple commits with a Dxxx tag, so # There's no clear way to manage multiple commits with a Dxxx tag, so
# require the amend option. (We could append "_nnn", but then it # require the amend option. (We could append "_nnn", but then it
# becomes jumbled if earlier commits are added to an update.) It should # becomes jumbled if earlier commits are added to an update.) It should
# lock the repo and ensure that the range is editable, but that would # lock the repo and ensure that the range is editable, but that would
# make the code pretty convoluted. The default behavior of `arc` is to # make the code pretty convoluted. The default behavior of `arc` is to
# create a new review anyway. # create a new review anyway.
if not opts.get(b"amend"): if not opts.get(b"amend"):
raise error.Abort(_(b"cannot fold with --no-amend")) raise error.Abort(_(b"cannot fold with --no-amend"))
# Ensure the local commits are an unbroken range
revrange = repo.revs(b'(first(%ld)::last(%ld))', revs, revs)
if any(r for r in revs if r not in revrange) or any(
r for r in revrange if r not in revs
):
raise error.Abort(_(b"cannot fold non-linear revisions"))
# It might be possible to bucketize the revisions by the DREV value, and # It might be possible to bucketize the revisions by the DREV value, and
# iterate over those groups when posting, and then again when amending. # iterate over those groups when posting, and then again when amending.
# But for simplicity, require all selected revisions to be for the same # But for simplicity, require all selected revisions to be for the same
# DREV (if present). Adding local revisions to an existing DREV is # DREV (if present). Adding local revisions to an existing DREV is
# acceptable. # acceptable.
drevmatchers = [ drevmatchers = [
_differentialrevisiondescre.search(ctx.description()) _differentialrevisiondescre.search(ctx.description())
for ctx in ctxs for ctx in ctxs

tests/test-phabricator.t

Phabimport accepts multiple DREVSPECs Phabimport accepts multiple DREVSPECs
$ hg rollback --config ui.rollback=True $ hg rollback --config ui.rollback=True
repository tip rolled back to revision 1 (undo phabimport) repository tip rolled back to revision 1 (undo phabimport)
$ hg phabimport --no-stack D7917 D7918 --test-vcr "$VCR/phabimport-multi-drev.json" $ hg phabimport --no-stack D7917 D7918 --test-vcr "$VCR/phabimport-multi-drev.json"
applying patch from D7917 applying patch from D7917
applying patch from D7918 applying patch from D7918
Phabsend requires a linear range of commits
$ hg phabsend -r 0+2+3
abort: cannot phabsend multiple head revisions: c44b38f24a45
(the revisions must form a linear chain)
[255]
Validate arguments with --fold Validate arguments with --fold
$ hg phabsend --fold -r 1 $ hg phabsend --fold -r 1
abort: cannot fold a single revision abort: cannot fold a single revision
[255] [255]
$ hg phabsend --fold --no-amend -r 1:: $ hg phabsend --fold --no-amend -r 1::
abort: cannot fold with --no-amend abort: cannot fold with --no-amend
[255] [255]
$ hg phabsend --fold -r 0+3
abort: cannot fold non-linear revisions
[255]
$ hg phabsend --fold -r 1:: $ hg phabsend --fold -r 1::
abort: cannot fold revisions with different DREV values abort: cannot fold revisions with different DREV values
[255] [255]
Setup a series of commits to be folded, and include the Test Plan field multiple Setup a series of commits to be folded, and include the Test Plan field multiple
times to test the concatenation logic. No Test Plan field in the last one to times to test the concatenation logic. No Test Plan field in the last one to
ensure missing fields are skipped. ensure missing fields are skipped.