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

login
register
mail settings
Submitter phabricator
Date April 17, 2020, 12:02 a.m.
Message ID <differential-rev-PHID-DREV-ojhgqe4trd5se6m5to57-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/46179/
State Superseded
Headers show

Comments

phabricator - April 17, 2020, 12:02 a.m.
mharbison72 created this revision.
Herald added subscribers: mercurial-devel, Kwan.
Herald added a reviewer: hg-reviewers.

REVISION 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 <https://phab.mercurial-scm.org/rHG0680b8a1992a74360b4f1a20bebf46c6c6962be8> and
  601ce5392cb0 <https://phab.mercurial-scm.org/rHG601ce5392cb092df601bef8c83fc250726f99c3d>.
  
  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.

REPOSITORY
  rHG Mercurial

BRANCH
  stable

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

AFFECTED FILES
  hgext/phabricator.py
  tests/test-phabricator.t

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: Kwan, mercurial-devel
phabricator - April 17, 2020, 6:26 p.m.
marmoute added inline comments.

INLINE COMMENTS

> phabricator.py:1314
> +    # get A' as a parent.
> +    revrange = repo.revs(b'(first(%ld)::last(%ld))', revs, revs)
> +    if any(r for r in revs if r not in revrange) or any(

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers
Cc: marmoute, Kwan, mercurial-devel
phabricator - April 17, 2020, 7:09 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> marmoute wrote in 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.)

> 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 <https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers
Cc: marmoute, Kwan, mercurial-devel
phabricator - April 17, 2020, 9:47 p.m.
marmoute added inline comments.

INLINE COMMENTS

> mharbison72 wrote in 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 <https://phab.mercurial-scm.org/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?

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

> phabricator.py:1318
> +    ):
> +        raise error.Abort(_(b"cannot phabsend non-linear revisions"))
> +

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers
Cc: marmoute, Kwan, mercurial-devel
phabricator - April 18, 2020, 3:25 a.m.
mharbison72 added inline comments.

INLINE COMMENTS

> marmoute wrote in 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.

> 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 <https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers
Cc: marmoute, Kwan, mercurial-devel
phabricator - April 19, 2020, 11:58 a.m.
marmoute added inline comments.

INLINE COMMENTS

> mharbison72 wrote in 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 <https://phab.mercurial-scm.org/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.

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers
Cc: marmoute, Kwan, mercurial-devel
phabricator - April 19, 2020, 9:43 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> marmoute wrote in phabricator.py:1314
> I think you are looking for `mercurial.scmutil.nodesummaries`.

Yes, that was it.  Thanks.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers
Cc: marmoute, Kwan, mercurial-devel
phabricator - April 22, 2020, 4:23 p.m.
Herald added a subscriber: mercurial-patches.
This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.


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

INLINE COMMENTS

> phabricator.py:1324
> +        )
> +
>      fold = opts.get(b'fold')

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

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - April 22, 2020, 5:49 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> marmoute wrote in phabricator.py:1324
> I would be a bit more explicit. Maybe something like:
> 
> `cannot phasend revisions group with multiple heads: %list-of-all-heads%`

> 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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - April 24, 2020, 3:30 p.m.
marmoute added inline comments.

INLINE COMMENTS

> mharbison72 wrote in 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?

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - April 24, 2020, 8:38 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> marmoute wrote in 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.

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - April 24, 2020, 10:10 p.m.
marmoute added inline comments.

INLINE COMMENTS

> mharbison72 wrote in 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.

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - May 1, 2020, 1:59 p.m.
mharbison72 added a comment.


  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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - May 1, 2020, 2:08 p.m.
marmoute added a comment.


  In D8454#126857 <https://phab.mercurial-scm.org/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 ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - May 1, 2020, 2:24 p.m.
mharbison72 added a comment.


  In D8454#126862 <https://phab.mercurial-scm.org/D8454#126862>, @marmoute wrote:
  
  > In D8454#126857 <https://phab.mercurial-scm.org/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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - May 2, 2020, 4:37 p.m.
marmoute added a comment.


  In D8454#126863 <https://phab.mercurial-scm.org/D8454#126863>, @mharbison72 wrote:
  
  > In D8454#126862 <https://phab.mercurial-scm.org/D8454#126862>, @marmoute wrote:
  >
  >> In D8454#126857 <https://phab.mercurial-scm.org/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"

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - May 2, 2020, 8:34 p.m.
mharbison72 added a comment.


  In D8454#126891 <https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel
phabricator - May 2, 2020, 9:37 p.m.
marmoute added a comment.


  In D8454#126892 <https://phab.mercurial-scm.org/D8454#126892>, @mharbison72 wrote:
  
  > In D8454#126891 <https://phab.mercurial-scm.org/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 ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8454/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8454

To: mharbison72, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, Kwan, mercurial-devel

Patch

diff --git a/tests/test-phabricator.t b/tests/test-phabricator.t
--- a/tests/test-phabricator.t
+++ b/tests/test-phabricator.t
@@ -589,6 +589,12 @@ 
   applying patch from D7917
   applying patch from D7918
 
+Phabsend requires a linear range of commits
+
+  $ hg phabsend -r 0+3
+  abort: cannot phabsend non-linear revisions
+  [255]
+
 Validate arguments with --fold
 
   $ hg phabsend --fold -r 1
@@ -597,9 +603,6 @@ 
   $ hg phabsend --fold --no-amend -r 1::
   abort: cannot fold with --no-amend
   [255]
-  $ hg phabsend --fold -r 0+3
-  abort: cannot fold non-linear revisions
-  [255]
   $ hg phabsend --fold -r 1::
   abort: cannot fold revisions with different DREV values
   [255]
diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -1307,6 +1307,16 @@ 
     if any(c for c in ctxs if c.obsolete()):
         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.
+    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 phabsend non-linear revisions"))
+
     fold = opts.get(b'fold')
     if fold:
         if len(revs) == 1:
@@ -1322,13 +1332,6 @@ 
         if not opts.get(b"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
         # iterate over those groups when posting, and then again when amending.
         # But for simplicity, require all selected revisions to be for the same