This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6957

rebase: unconditionally clear merge state when `--stop`ing a rebase
Needs RevisionPublic

Authored by durin42 on Oct 3 2019, 9:39 PM.

Details

Reviewers
martinvonz
baymax
Group Reviewers
hg-reviewers
Summary

I am NOT sure of the correctness of this change, but I've hit a point
on a work repository where the needupdate function returns false and
so the merge state never gets correctly cleared. As far as I can tell
when we're *always* in a merge state at this point, so we should
*always* clear the merge state.

I'm not sure how to structure the test at the moment, but if others
agree that this looks plausible I'll try and figure out what state my
work repository is in to craft a test case that can be used to prevent
any backsliding on this fix.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Oct 3 2019, 9:39 PM
martinvonz added a comment.Oct 4 2019, 5:52 AM

How was it failing for you?

I don't think I've used hg rebase --stop, but I had been wondering what it would do with the working directory. It's unclear to me what it should do, and now after looking at the code, it's also unclear what the author meant it to do. Maybe it would be best to just leave the working directory untouched? Oh, we should also compare to what hg evolve --stop does (I haven't checked).

marmoute added a subscriber: marmoute.Oct 4 2019, 9:11 AM
In D6957#101977, @martinvonz wrote:

How was it failing for you?
I don't think I've used hg rebase --stop, but I had been wondering what it would do with the working directory. It's unclear to me what it should do, and now after looking at the code, it's also unclear what the author meant it to do. Maybe it would be best to just leave the working directory untouched? Oh, we should also compare to what hg evolve --stop does (I haven't checked).

The evolve semantic, is to:

  • get out of the current conflict (so, reverting the working copy)
  • preserve all successful evolution so far.

Since you cannot continue after the --stop, we would not be able to do anything good with the conflict if the user resolved it anyway.

I think rebase should have the same logic and revert the current conflict (preserving all successfully rebased revision)

martinvonz added a comment.Oct 4 2019, 9:18 AM
In D6957#101984, @marmoute wrote:
In D6957#101977, @martinvonz wrote:

How was it failing for you?
I don't think I've used hg rebase --stop, but I had been wondering what it would do with the working directory. It's unclear to me what it should do, and now after looking at the code, it's also unclear what the author meant it to do. Maybe it would be best to just leave the working directory untouched? Oh, we should also compare to what hg evolve --stop does (I haven't checked).

The evolve semantic, is to:

  • get out of the current conflict (so, reverting the working copy)
  • preserve all successful evolution so far.

Since you cannot continue after the --stop, we would not be able to do anything good with the conflict if the user resolved it anyway.
I think rebase should have the same logic and revert the current conflict (preserving all successfully rebased revision)

True, you'd get to resolve the conflicts on the next hg rebase/evolve anyway, so I agree there's no need to preserve the working copy.

@durin42, any idea how you ended up with just one working copy parent?

durin42 added a comment.Oct 4 2019, 9:32 AM
In D6957#101985, @martinvonz wrote:
In D6957#101984, @marmoute wrote:
In D6957#101977, @martinvonz wrote:

How was it failing for you?
I don't think I've used hg rebase --stop, but I had been wondering what it would do with the working directory. It's unclear to me what it should do, and now after looking at the code, it's also unclear what the author meant it to do. Maybe it would be best to just leave the working directory untouched? Oh, we should also compare to what hg evolve --stop does (I haven't checked).

The evolve semantic, is to:

  • get out of the current conflict (so, reverting the working copy)
  • preserve all successful evolution so far.

Since you cannot continue after the --stop, we would not be able to do anything good with the conflict if the user resolved it anyway.
I think rebase should have the same logic and revert the current conflict (preserving all successfully rebased revision)

True, you'd get to resolve the conflicts on the next hg rebase/evolve anyway, so I agree there's no need to preserve the working copy.
@durin42, any idea how you ended up with just one working copy parent?

I don't have just one parent. If you want to inspect the state, look at augie/a 59473;)

baymax requested changes to this revision.Jan 24 2020, 12:31 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:31 PM

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (7 lines)

Diff 16789

hgext/rebase.py

raise error.Abort(_("cannot stop in --collapse session")) raise error.Abort(_("cannot stop in --collapse session"))
allowunstable = obsolete.isenabled(repo, obsolete.allowunstableopt) allowunstable = obsolete.isenabled(repo, obsolete.allowunstableopt)
if not (rbsrt.keepf or allowunstable): if not (rbsrt.keepf or allowunstable):
raise error.Abort(_("cannot remove original changesets with" raise error.Abort(_("cannot remove original changesets with"
" unrebased descendants"), " unrebased descendants"),
hint=_('either enable obsmarkers to allow unstable ' hint=_('either enable obsmarkers to allow unstable '
'revisions or use --keep to keep original ' 'revisions or use --keep to keep original '
'changesets')) 'changesets'))
if needupdate(repo, rbsrt.state):
# update to the current working revision # update to the current working revision
# to clear interrupted merge # to clear interrupted merge
hg.updaterepo(repo, rbsrt.originalwd, overwrite=True) hg.updaterepo(repo, rbsrt.originalwd, overwrite=True)
rbsrt._finishrebase() rbsrt._finishrebase()
return 0 return 0
elif inmemory: elif inmemory:
try: try:
# in-memory merge doesn't support conflicts, so if we hit any, abort # in-memory merge doesn't support conflicts, so if we hit any, abort
# and re-run as an on-disk merge. # and re-run as an on-disk merge.
overrides = {('rebase', 'singletransaction'): True} overrides = {('rebase', 'singletransaction'): True}
with ui.configoverride(overrides, 'rebase'): with ui.configoverride(overrides, 'rebase'):