| 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 <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?
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
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,
> 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.
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.
> 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. :/
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