Patchwork D6957: rebase: unconditionally clear merge state when `--stop`ing a rebase

login
register
mail settings
Submitter phabricator
Date Oct. 4, 2019, 1:40 a.m.
Message ID <differential-rev-PHID-DREV-cyjg76r4ov5tatmyitkv-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41943/
State New
Headers show

Comments

phabricator - Oct. 4, 2019, 1:40 a.m.
durin42 created this revision.
Herald added a reviewer: martinvonz.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/rebase.py

CHANGE DETAILS




To: durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 4, 2019, 9:52 a.m.
martinvonz added a comment.


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

REPOSITORY
  rHG Mercurial

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

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

To: durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 4, 2019, 1:11 p.m.
marmoute added a comment.


  In D6957#101977 <https://phab.mercurial-scm.org/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)

REPOSITORY
  rHG Mercurial

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

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

To: durin42, martinvonz, #hg-reviewers
Cc: marmoute, mercurial-devel
phabricator - Oct. 4, 2019, 1:18 p.m.
martinvonz added a comment.


  In D6957#101984 <https://phab.mercurial-scm.org/D6957#101984>, @marmoute wrote:
  
  > In D6957#101977 <https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

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

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

To: durin42, martinvonz, #hg-reviewers
Cc: marmoute, mercurial-devel
phabricator - Oct. 4, 2019, 1:32 p.m.
durin42 added a comment.


  In D6957#101985 <https://phab.mercurial-scm.org/D6957#101985>, @martinvonz wrote:
  
  > In D6957#101984 <https://phab.mercurial-scm.org/D6957#101984>, @marmoute wrote:
  >
  >> In D6957#101977 <https://phab.mercurial-scm.org/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`;)

REPOSITORY
  rHG Mercurial

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

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

To: durin42, martinvonz, #hg-reviewers
Cc: marmoute, mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -909,10 +909,9 @@ 
                     hint=_('either enable obsmarkers to allow unstable '
                            'revisions or use --keep to keep original '
                            'changesets'))
-            if needupdate(repo, rbsrt.state):
-                # update to the current working revision
-                # to clear interrupted merge
-                hg.updaterepo(repo, rbsrt.originalwd, overwrite=True)
+            # update to the current working revision
+            # to clear interrupted merge
+            hg.updaterepo(repo, rbsrt.originalwd, overwrite=True)
             rbsrt._finishrebase()
             return 0
     elif inmemory: