Patchwork [v2] rebase: add potential divergent commit hashes to error message (issue5086)

login
register
mail settings
Submitter Kostia Balytskyi
Date Feb. 17, 2016, 8:32 p.m.
Message ID <c91240e672a88d78abdc.1455741142@ikostia-mbp>
Download mbox | patch
Permalink /patch/13250/
State Accepted
Headers show

Comments

Kostia Balytskyi - Feb. 17, 2016, 8:32 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1455741094 0
#      Wed Feb 17 20:31:34 2016 +0000
# Node ID c91240e672a88d78abdcd571cfdcf2cba36504e6
# Parent  95bf01b8754016200a99fd3538e78030b2028c60
rebase: add potential divergent commit hashes to error message (issue5086)
Sean Farley - Feb. 17, 2016, 10:25 p.m.
Kostia Balytskyi <ikostia@fb.com> writes:

> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1455741094 0
> #      Wed Feb 17 20:31:34 2016 +0000
> # Node ID c91240e672a88d78abdcd571cfdcf2cba36504e6
> # Parent  95bf01b8754016200a99fd3538e78030b2028c60
> rebase: add potential divergent commit hashes to error message (issue5086)

Looks good to me. Pierre-Yves?
Augie Fackler - Feb. 22, 2016, 11:02 p.m.
On Wed, Feb 17, 2016 at 08:32:22PM +0000, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1455741094 0
> #      Wed Feb 17 20:31:34 2016 +0000
> # Node ID c91240e672a88d78abdcd571cfdcf2cba36504e6
> # Parent  95bf01b8754016200a99fd3538e78030b2028c60
> rebase: add potential divergent commit hashes to error message (issue5086)
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -305,10 +305,13 @@
>                  divergencebasecandidates = rebaseobsrevs - rebaseobsskipped
>
>                  if divergencebasecandidates and not divergenceok:
> -                    msg = _("this rebase will cause divergence")
> +                    divhashes = (repo.unfiltered()[r].hex()
> +                                 for r in divergencebasecandidates)
> +                    msg = _("this rebase will cause "
> +                            "divergences with base(s): %s")
>                      h = _("to force the rebase please set "
>                            "rebase.allowdivergence=True")
> -                    raise error.Abort(msg, hint=h)
> +                    raise error.Abort(msg % (",".join(divhashes),), hint=h)
>
>                  # - plain prune (no successor) changesets are rebased
>                  # - split changesets are not rebased if at least one of the
> diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
> --- a/tests/test-rebase-obsolete.t
> +++ b/tests/test-rebase-obsolete.t
> @@ -771,7 +771,7 @@
>    phases: 8 draft
>    unstable: 1 changesets
>    $ hg rebase -s 10 -d 12
> -  abort: this rebase will cause divergence
> +  abort: this rebase will cause divergences with base(s): 121d9e3bc4c60bd1c9c007e7de31d6796b882a45
>    (to force the rebase please set rebase.allowdivergence=True)

So, as a user, I'm actually not sure what I should /do/ in response to
this "will cause divergence" message other than force the rebase with
allowdivergence. Can we try and wordsmith this so that the /actual
problem/ is more clear? As it is we're just telling people a long hash
with no context.

>    [255]
>    $ hg log -G
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Feb. 22, 2016, 11:41 p.m.
On 02/23/2016 12:02 AM, Augie Fackler wrote:
> On Wed, Feb 17, 2016 at 08:32:22PM +0000, Kostia Balytskyi wrote:
>> # HG changeset patch
>> # User Kostia Balytskyi <ikostia@fb.com>
>> # Date 1455741094 0
>> #      Wed Feb 17 20:31:34 2016 +0000
>> # Node ID c91240e672a88d78abdcd571cfdcf2cba36504e6
>> # Parent  95bf01b8754016200a99fd3538e78030b2028c60
>> rebase: add potential divergent commit hashes to error message (issue5086)
>>
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -305,10 +305,13 @@
>>                   divergencebasecandidates = rebaseobsrevs - rebaseobsskipped
>>
>>                   if divergencebasecandidates and not divergenceok:
>> -                    msg = _("this rebase will cause divergence")
>> +                    divhashes = (repo.unfiltered()[r].hex()
>> +                                 for r in divergencebasecandidates)
>> +                    msg = _("this rebase will cause "
>> +                            "divergences with base(s): %s")
>>                       h = _("to force the rebase please set "
>>                             "rebase.allowdivergence=True")
>> -                    raise error.Abort(msg, hint=h)
>> +                    raise error.Abort(msg % (",".join(divhashes),), hint=h)
>>
>>                   # - plain prune (no successor) changesets are rebased
>>                   # - split changesets are not rebased if at least one of the
>> diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
>> --- a/tests/test-rebase-obsolete.t
>> +++ b/tests/test-rebase-obsolete.t
>> @@ -771,7 +771,7 @@
>>     phases: 8 draft
>>     unstable: 1 changesets
>>     $ hg rebase -s 10 -d 12
>> -  abort: this rebase will cause divergence
>> +  abort: this rebase will cause divergences with base(s): 121d9e3bc4c60bd1c9c007e7de31d6796b882a45
>>     (to force the rebase please set rebase.allowdivergence=True)
>
> So, as a user, I'm actually not sure what I should /do/ in response to
> this "will cause divergence" message other than force the rebase with
> allowdivergence. Can we try and wordsmith this so that the /actual
> problem/ is more clear? As it is we're just telling people a long hash
> with no context.

Yes, long hash is un-necessary/unusual here. We should stick to a short 
version,

Rebase complain because you are about to rewrite (here rebase), an 
absolete changeset that already have a sucessors. Creating a second 
successor will create divergence.

There is a couple thing we could improve here:

a) finish Laurent work on skipping changeset already in destination, 
that should remove most of it.

b) suggest/tell/explain-how the user could exclude that changesets from 
its rebase.

c) points at that already existing successors,

d) improvement d.

that said, this message is already some improvement compared to the new 
one. I've pushed to the clowncopter a version using short hash.

Cheers,
Augie Fackler - Feb. 23, 2016, 12:28 a.m.
> On Feb 22, 2016, at 6:41 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> Yes, long hash is un-necessary/unusual here. We should stick to a short version,
> 
> Rebase complain because you are about to rewrite (here rebase), an absolete changeset that already have a sucessors. Creating a second successor will create divergence.

I know that, but the error output is still useless to me, as it offers no suggestions on how I might avoid the divergence or why it’s bad.

(Note: **I** know why divergence is bad, but I don’t actually know what I’m doing wrong that would result in divergence. The above sentence is partially me pretending to be a novice user of rebase.)

> 
> There is a couple thing we could improve here:
> 
> a) finish Laurent work on skipping changeset already in destination, that should remove most of it.
> 
> b) suggest/tell/explain-how the user could exclude that changesets from its rebase.
> 
> c) points at that already existing successors,
> 
> d) improvement d.
> 
> that said, this message is already some improvement compared to the new one. I've pushed to the clowncopter a version using short hash.

I disagree that this is a measurable improvement over the status quo. Option B would be a significant improvement, and C is probably a small improvement.

Option A is somewhat orthogonal - it’s important, but it doesn’t move the needle when users are actually about to burn themselves.
Sean Farley - Feb. 23, 2016, 1:19 a.m.
Augie Fackler <raf@durin42.com> writes:

>> On Feb 22, 2016, at 6:41 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>> 
>> Yes, long hash is un-necessary/unusual here. We should stick to a short version,
>> 
>> Rebase complain because you are about to rewrite (here rebase), an absolete changeset that already have a sucessors. Creating a second successor will create divergence.
>
> I know that, but the error output is still useless to me, as it offers no suggestions on how I might avoid the divergence or why it’s bad.

I definitely think it's an improvement over the current error output:
"abort: this rebase will cause divergence".

> (Note: **I** know why divergence is bad, but I don’t actually know what I’m doing wrong that would result in divergence. The above sentence is partially me pretending to be a novice user of rebase.)

For the user that knows about divergence, then they can investigate the
precursor / other divergent head with the given hash (bikeshedding on
short hash).

For the user that doesn't, then it's even more confusing.

>> There is a couple thing we could improve here:
>> 
>> a) finish Laurent work on skipping changeset already in destination, that should remove most of it.
>> 
>> b) suggest/tell/explain-how the user could exclude that changesets from its rebase.
>> 
>> c) points at that already existing successors,
>> 
>> d) improvement d.
>> 
>> that said, this message is already some improvement compared to the new one. I've pushed to the clowncopter a version using short hash.
>
> I disagree that this is a measurable improvement over the status quo. Option B would be a significant improvement, and C is probably a small improvement.

Eek. This is all under the umbrella of 'improve evolve UI'. I honestly
don't know how to do small (experimental) improvements there within the
constraints of BC.

> Option A is somewhat orthogonal - it’s important, but it doesn’t move the needle when users are actually about to burn themselves.

Yeah, I would say that too.
Augie Fackler - Feb. 23, 2016, 2:26 a.m.
> On Feb 22, 2016, at 8:19 PM, Sean Farley <sean@farley.io> wrote:
> 
>> I know that, but the error output is still useless to me, as it offers no suggestions on how I might avoid the divergence or why it’s bad.
> 
> I definitely think it's an improvement over the current error output:
> "abort: this rebase will cause divergence”.

So, it tells me the changeset I’m about to rebase that would be divergent, but it doesn’t empower me to figure out what it would be divergent against.

Not me feigning ignorance: I don’t know how I’d figure out what the other changeset that would be part of the potential future divergence would be.

> 
>> (Note: **I** know why divergence is bad, but I don’t actually know what I’m doing wrong that would result in divergence. The above sentence is partially me pretending to be a novice user of rebase.)
> 
> For the user that knows about divergence, then they can investigate the
> precursor / other divergent head with the given hash (bikeshedding on
> short hash).

See above - this change adds information which is not helpful, even at my level of familiarity with Mercurial. That’s why I said it’s not an improvement. :/
Sean Farley - Feb. 23, 2016, 5:36 a.m.
Augie Fackler <raf@durin42.com> writes:

>> On Feb 22, 2016, at 8:19 PM, Sean Farley <sean@farley.io> wrote:
>> 
>>> I know that, but the error output is still useless to me, as it offers no suggestions on how I might avoid the divergence or why it’s bad.
>> 
>> I definitely think it's an improvement over the current error output:
>> "abort: this rebase will cause divergence”.
>
> So, it tells me the changeset I’m about to rebase that would be divergent, but it doesn’t empower me to figure out what it would be divergent against.
>
> Not me feigning ignorance: I don’t know how I’d figure out what the other changeset that would be part of the potential future divergence would be.
>
>> 
>>> (Note: **I** know why divergence is bad, but I don’t actually know what I’m doing wrong that would result in divergence. The above sentence is partially me pretending to be a novice user of rebase.)
>> 
>> For the user that knows about divergence, then they can investigate the
>> precursor / other divergent head with the given hash (bikeshedding on
>> short hash).
>
> See above - this change adds information which is not helpful, even at my level of familiarity with Mercurial. That’s why I said it’s not an improvement. :/

Ok, I now understand what you mean. Yes, I agree that this a movement
sideways and not forward. For starters, we need to use the same
nomenclature throughout Mercurial:

abort: this rebase will cause divergences with precursor(s): XYZ

Following up with what Augie said, could we give a hint? Something like,

use the revset 'successors(XYZ)'. Random thought, with the new graph log
proposals, could we have an easy command for the ascii graph of:

hg obsgraph xyz...asdf

(yes, I realize this will be bikeshedding)

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -305,10 +305,13 @@ 
                 divergencebasecandidates = rebaseobsrevs - rebaseobsskipped
 
                 if divergencebasecandidates and not divergenceok:
-                    msg = _("this rebase will cause divergence")
+                    divhashes = (repo.unfiltered()[r].hex()
+                                 for r in divergencebasecandidates)
+                    msg = _("this rebase will cause "
+                            "divergences with base(s): %s")
                     h = _("to force the rebase please set "
                           "rebase.allowdivergence=True")
-                    raise error.Abort(msg, hint=h)
+                    raise error.Abort(msg % (",".join(divhashes),), hint=h)
 
                 # - plain prune (no successor) changesets are rebased
                 # - split changesets are not rebased if at least one of the
diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -771,7 +771,7 @@ 
   phases: 8 draft
   unstable: 1 changesets
   $ hg rebase -s 10 -d 12
-  abort: this rebase will cause divergence
+  abort: this rebase will cause divergences with base(s): 121d9e3bc4c60bd1c9c007e7de31d6796b882a45
   (to force the rebase please set rebase.allowdivergence=True)
   [255]
   $ hg log -G