Patchwork D6735: update: added support for --abort flag(issue4404)

login
register
mail settings
Submitter phabricator
Date Aug. 17, 2019, 4:14 p.m.
Message ID <differential-rev-PHID-DREV-t354japty4n47u3xiwiz-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41336/
State New
Headers show

Comments

phabricator - Aug. 17, 2019, 4:14 p.m.
taapas1128 created this revision.
Herald added subscribers: mercurial-devel, mjpieters.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch adds functionality to abort a conflicted
  update. A new function `hg.abortupdate()` is added for
  the purpose which has a helper function
  `hg._unmarkandresolvelocal()` which deals with the
  conflicts occured.
  
  Results are shown in tests.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/transplant.py
  mercurial/commands.py
  mercurial/hg.py
  tests/test-merge-tools.t

CHANGE DETAILS




To: taapas1128, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - Aug. 17, 2019, 7:29 p.m.
pulkit added inline comments.

INLINE COMMENTS

> test-merge-tools.t:2092
> +
> +Test for 'hg update --abort'
> +

I suggest taking `update --abort` tests in a new file as we will need to extensively test that.

> test-merge-tools.t:2129
> +  merging file
> +  warning: conflicts while merging file! (edit, then use 'hg resolve --mark')
> +  update aborted

we need to hide this, maybe use more low level APIs. I haven't look at code in details, will try to look soon.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mjpieters, mercurial-devel
phabricator - Aug. 18, 2019, 1:08 p.m.
taapas1128 added inline comments.
taapas1128 marked an inline comment as done.

INLINE COMMENTS

> pulkit wrote in test-merge-tools.t:2092
> I suggest taking `update --abort` tests in a new file as we will need to extensively test that.

I have created a separate file will add more tests.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mjpieters, mercurial-devel
phabricator - Aug. 19, 2019, 3:27 p.m.
pulkit added a comment.


  Nice start! If I remember correctly, the mergestate stores the local version of the file. We can use that directly instead.

INLINE COMMENTS

> hg.py:992
> +    for f in ms:
> +        ms.mark(f, mergemod.MERGE_RECORD_UNRESOLVED_PATH)
> +        ms.mark(f, mergemod.MERGE_RECORD_UNRESOLVED)

this line is not required.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mjpieters, mercurial-devel
phabricator - Aug. 19, 2019, 4:32 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> taapas1128 wrote in test-merge-tools.t:2092
> I have created a separate file will add more tests.

When you add more tests, please include some with 1 dirty subrepo, and then another where the first subrepo is already merged/resolved, and the second subrepo is dirty and pending a merge/resolve.  If it simplifies the initial implementation to detect a dirty subrepo and then abort the `--abort` before it starts, that seems fine.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, pulkit, mjpieters, mercurial-devel
phabricator - Aug. 22, 2019, 2:21 p.m.
taapas1128 added inline comments.
taapas1128 marked an inline comment as done.

INLINE COMMENTS

> mharbison72 wrote in test-merge-tools.t:2092
> When you add more tests, please include some with 1 dirty subrepo, and then another where the first subrepo is already merged/resolved, and the second subrepo is dirty and pending a merge/resolve.  If it simplifies the initial implementation to detect a dirty subrepo and then abort the `--abort` before it starts, that seems fine.

@mharbison72 Have a look. I have updated the diff.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, pulkit, mjpieters, mercurial-devel
phabricator - Sept. 11, 2019, 3:24 p.m.
durin42 added a comment.
durin42 accepted this revision as: durin42.


  This looks good to me. Does anyone else have comments? @mharbison72 are you happy with the subrepo test coverage?

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42
Cc: durin42, mharbison72, pulkit, mjpieters, mercurial-devel
phabricator - Sept. 11, 2019, 4:27 p.m.
taapas1128 added a comment.
taapas1128 marked an inline comment as done.


  @durin42 I don't think the subrepos are properly initialized in the tests. I would be updating them soon.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42
Cc: durin42, mharbison72, pulkit, mjpieters, mercurial-devel
phabricator - Sept. 12, 2019, 5:33 a.m.
mharbison72 added a comment.


  In D6735#100392 <https://phab.mercurial-scm.org/D6735#100392>, @durin42 wrote:
  
  > This looks good to me. Does anyone else have comments? @mharbison72 are you happy with the subrepo test coverage?
  
  I forgot about this, thanks for the reminder.  @taapas1128 is correct- the subrepos aren't being initialized as subrepos (i.e. there's no .hgsub file).  I'll try to play with this more Friday or over the weekend.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42
Cc: durin42, mharbison72, pulkit, mjpieters, mercurial-devel
phabricator - Sept. 12, 2019, 5:34 a.m.
This revision now requires changes to proceed.
mharbison72 added a comment.
mharbison72 requested changes to this revision.


  Oops, meant to flag changes needed too.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42, mharbison72
Cc: durin42, mharbison72, pulkit, mjpieters, mercurial-devel
phabricator - Sept. 16, 2019, 3:14 a.m.
mharbison72 added a comment.


  Here's a case I stumbled upon that is a problem.  It looks like it thinks it isn't in the middle of an update, but .hgsubstate isn't put back to the pre-update state.
  
    diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
    --- a/tests/test-subrepo.t
    +++ b/tests/test-subrepo.t
    @@ -1027,10 +1027,38 @@ filesystem (see also issue4583))
       > [extensions]
       > fakedirstatewritetime = $TESTDIR/fakedirstatewritetime.py
       > EOF
    +  $ hg -R repo st -S
    +  ? s/b
    +  $ hg -R repo diff -S
    +  $ hg -R repo log -r .
    +  changeset:   0:c4018e9aea1b
    +  user:        test
    +  date:        Thu Jan 01 00:00:00 1970 +0000
    +  summary:     1
    +
    +  $ cat repo/.hgsubstate
    +  f7b1eb17ad24730a1651fccd46c43826d1bbc2ac s
       $ hg -R repo update
       b: untracked file differs
       abort: untracked files in working directory differ from files in requested revision (in subrepository "s")
       [255]
    +  $ hg -R repo update --abort
    +  abort: no update in progress
    +  [255]
    +  $ hg -R repo diff -S
    +  diff -r c4018e9aea1b .hgsubstate
    +  --- a/.hgsubstate Thu Jan 01 00:00:00 1970 +0000
    +  +++ b/.hgsubstate Thu Jan 01 00:00:00 1970 +0000
    +  @@ -1,1 +1,1 @@
    +  -f7b1eb17ad24730a1651fccd46c43826d1bbc2ac s
    +  +cc505f09a8b2644adffa368adb4abc6f70d07bc0 s
    +  $ hg -R repo log -r .
    +  changeset:   0:c4018e9aea1b
    +  user:        test
    +  date:        Thu Jan 01 00:00:00 1970 +0000
    +  summary:     1
    +
    +
       $ cat >> repo/.hg/hgrc <<EOF
       > [extensions]
       > fakedirstatewritetime = !
  
  Another good way to induce this .hgsubstate issue is to `hg pull -u` on a repo, where the remote subrepo isn't available.  You don't need http for this- you can just rename the remote subrepo.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42, mharbison72
Cc: durin42, mharbison72, pulkit, mjpieters, mercurial-devel
phabricator - March 16, 2020, 9:47 p.m.
khanchi97 added a comment.


  In D6735#100642 <https://phab.mercurial-scm.org/D6735#100642>, @mharbison72 wrote:
  
  > Here's a case I stumbled upon that is a problem.  It looks like it thinks it isn't in the middle of an update, but .hgsubstate isn't put back to the pre-update state.
  >
  >   diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
  >   --- a/tests/test-subrepo.t
  >   +++ b/tests/test-subrepo.t
  >   @@ -1027,10 +1027,38 @@ filesystem (see also issue4583))
  >      > [extensions]
  >      > fakedirstatewritetime = $TESTDIR/fakedirstatewritetime.py
  >      > EOF
  >   +  $ hg -R repo st -S
  >   +  ? s/b
  >   +  $ hg -R repo diff -S
  >   +  $ hg -R repo log -r .
  >   +  changeset:   0:c4018e9aea1b
  >   +  user:        test
  >   +  date:        Thu Jan 01 00:00:00 1970 +0000
  >   +  summary:     1
  >   +
  >   +  $ cat repo/.hgsubstate
  >   +  f7b1eb17ad24730a1651fccd46c43826d1bbc2ac s
  >      $ hg -R repo update
  >      b: untracked file differs
  >      abort: untracked files in working directory differ from files in requested revision (in subrepository "s")
  
  @mharbison72 I am bit confused here.
  
  1. Since `hg -R repo update` resulted with an abort then why we have dirty working directory now? Isn't transaction rollback worked correctly?
  2. Also if you run `hg status` it says "To continue: run `hg update .`" but I don't think it's really a "continue", since we are still on the same changeset where we ran the first update command and running `hg update .` won't take us to the changeset we wanted to go.
  3. I found that for interrupted update (only for some particular kind of interrupted update) we store target node value in `.hg/updatestate` but I couldn't find where we exactly use that value. I found some code which check if that file exists but not one where we use the value stored in that file.
  
  >   [255]
  >
  > +  $ hg -R repo update --abort
  > +  abort: no update in progress
  > +  [255]
  > +  $ hg -R repo diff -S
  > +  diff -r c4018e9aea1b .hgsubstate
  > +  --- a/.hgsubstate Thu Jan 01 00:00:00 1970 +0000
  > +  +++ b/.hgsubstate Thu Jan 01 00:00:00 1970 +0000
  > +  @@ -1,1 +1,1 @@
  > +  -f7b1eb17ad24730a1651fccd46c43826d1bbc2ac s
  > +  +cc505f09a8b2644adffa368adb4abc6f70d07bc0 s
  > +  $ hg -R repo log -r .
  > +  changeset:   0:c4018e9aea1b
  > +  user:        test
  > +  date:        Thu Jan 01 00:00:00 1970 +0000
  > +  summary:     1
  > +
  > +
  >
  >   $ cat >> repo/.hg/hgrc <<EOF
  >   > [extensions]
  >   > fakedirstatewritetime = !
  >
  >   Another good way to induce this .hgsubstate issue is to `hg pull -u` on a repo, where the remote subrepo isn't available.  You don't need http for this- you can just rename the remote subrepo.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42, mharbison72
Cc: khanchi97, durin42, mharbison72, pulkit, mjpieters, mercurial-devel
phabricator - March 16, 2020, 10:45 p.m.
mharbison72 added a comment.


  In D6735#123875 <https://phab.mercurial-scm.org/D6735#123875>, @khanchi97 wrote:
  
  > In D6735#100642 <https://phab.mercurial-scm.org/D6735#100642>, @mharbison72 wrote:
  >
  >> Here's a case I stumbled upon that is a problem.  It looks like it thinks it isn't in the middle of an update, but .hgsubstate isn't put back to the pre-update state.
  >>
  >>   diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
  >>   --- a/tests/test-subrepo.t
  >>   +++ b/tests/test-subrepo.t
  >>   @@ -1027,10 +1027,38 @@ filesystem (see also issue4583))
  >>      > [extensions]
  >>      > fakedirstatewritetime = $TESTDIR/fakedirstatewritetime.py
  >>      > EOF
  >>   +  $ hg -R repo st -S
  >>   +  ? s/b
  >>   +  $ hg -R repo diff -S
  >>   +  $ hg -R repo log -r .
  >>   +  changeset:   0:c4018e9aea1b
  >>   +  user:        test
  >>   +  date:        Thu Jan 01 00:00:00 1970 +0000
  >>   +  summary:     1
  >>   +
  >>   +  $ cat repo/.hgsubstate
  >>   +  f7b1eb17ad24730a1651fccd46c43826d1bbc2ac s
  >>      $ hg -R repo update
  >>      b: untracked file differs
  >>      abort: untracked files in working directory differ from files in requested revision (in subrepository "s")
  >
  > @mharbison72 I am bit confused here.
  >
  > 1. Since `hg -R repo update` resulted with an abort then why we have dirty working directory now? Isn't transaction rollback worked correctly?
  
  Two things to keep in mind.
  
  1. updates aren't transactions
  2. even if they were, a transaction is within *one* repo.
  
  When you update with subrepos, you check for dirty from the top, recurse to the bottom, and on the way up you update the subrepo.  So in the simple case of parent repo P and subrepo S, you check P for dirty (recursively), then update S, then update P.  If something goes wrong updating P, S is not rolled back.
  
  > 2. Also if you run `hg status` it says "To continue: run `hg update .`" but I don't think it's really a "continue", since we are still on the same changeset where we ran the first update command and running `hg update .` won't take us to the changeset we wanted to go.
  
  Maybe it needs to continue if it did a partial checkout?  IDK when the parent in dirstate is updated- before or after all files are checked out.
  
  > 3. I found that for interrupted update (only for some particular kind of interrupted update) we store target node value in `.hg/updatestate` but I couldn't find where we exactly use that value. I found some code which check if that file exists but not one where we use the value stored in that file.
  
  Sorry, I'm not sure about that either.  I'd imagine it's for the --continue case though.  Maybe that isn't fully implemented yet?

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42, mharbison72
Cc: khanchi97, durin42, mharbison72, pulkit, mjpieters, mercurial-devel

Patch

diff --git a/tests/test-merge-tools.t b/tests/test-merge-tools.t
--- a/tests/test-merge-tools.t
+++ b/tests/test-merge-tools.t
@@ -4,6 +4,8 @@ 
   $ cat >> $HGRCPATH << EOF
   > [ui]
   > merge=
+  > [extensions]
+  > graphlog=
   > EOF
   $ hg init repo
   $ cd repo
@@ -2086,3 +2088,63 @@ 
   f = false
 
   $ cd ..
+
+Test for 'hg update --abort'
+
+  $ hg init test
+  $ cd test
+  $ echo before > file
+  $ hg add file
+  $ hg commit -mbefore
+  $ echo after > file
+  $ hg commit -mafter
+  $ echo edited > file
+
+Pre-update log and file content
+  $ hg glog
+  @  changeset:   1:47d1da7c1f80
+  |  tag:         tip
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     after
+  |
+  o  changeset:   0:dd67ebc4de84
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     before
+  
+
+  $ cat file
+  edited
+  $ hg update 0
+  merging file
+  warning: conflicts while merging file! (edit, then use 'hg resolve --mark')
+  0 files updated, 0 files merged, 0 files removed, 1 files unresolved
+  use 'hg resolve' to retry unresolved file merges
+  [1]
+
+  $ hg update --abort
+  aborting the update, updating back to 47d1da7c1f80
+  merging file
+  warning: conflicts while merging file! (edit, then use 'hg resolve --mark')
+  update aborted
+  working directory is now at 47d1da7c1f80
+
+Post-update log and file content
+  $ hg glog
+  @  changeset:   1:47d1da7c1f80
+  |  tag:         tip
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     after
+  |
+  o  changeset:   0:dd67ebc4de84
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     before
+  
+
+  $ cat file
+  edited
+
+  $ cd ..
diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -984,6 +984,34 @@ 
     _showstats(repo, stats)
     return stats.unresolvedcount > 0
 
+def _unmarkandresolvelocal(ui, repo , ms):
+    """Helper for abortupdate(). This unmarks partially resolved files and then
+    resolves the update generated conflicts."""
+    wctx = repo[None]
+    for f in ms:
+        ms.mark(f, mergemod.MERGE_RECORD_UNRESOLVED_PATH)
+        ms.mark(f, mergemod.MERGE_RECORD_UNRESOLVED)
+        try:
+            overrides = {('ui', 'forcemerge'): ':local'}
+            with ui.configoverride(overrides, 'resolve'):
+                ms.preresolve(f, wctx)
+                ms.resolve(f, wctx)
+        finally:
+            ms.commit()
+
+def abortupdate(ui, repo):
+    """logic to abort an conflicted update"""
+    ms = mergemod.mergestate.read(repo)
+    node = ms.localctx.hex()
+    _unmarkandresolvelocal(ui, repo, ms)
+    repo.ui.status(_("aborting the update, updating back to"
+                     " %s\n") % node[:12])
+    mergemod.update(repo, node, branchmerge=False, force=False)
+    ms2 = mergemod.mergestate.read(repo)
+    _unmarkandresolvelocal(ui, repo, ms)(ui, repo, ms2)
+    ui.status(_("update aborted\n"))
+    ui.status(_("working directory is now at %s\n") % node[:12])
+
 def _incoming(displaychlist, subreporecurse, ui, repo, source,
         opts, buffered=False):
     """
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -6154,6 +6154,7 @@ 
     [('C', 'clean', None, _('discard uncommitted changes (no backup)')),
     ('c', 'check', None, _('require clean working directory')),
     ('m', 'merge', None, _('merge uncommitted changes')),
+    ('', 'abort', False, _('abort interrupted update')),
     ('d', 'date', '', _('tipmost revision matching date'), _('DATE')),
     ('r', 'rev', '', _('revision'), _('REV'))
      ] + mergetoolopts,
@@ -6220,6 +6221,7 @@ 
     clean = opts.get(r'clean')
     check = opts.get(r'check')
     merge = opts.get(r'merge')
+    abort = opts.get(r'abort')
     if rev and node:
         raise error.Abort(_("please specify just one revision"))
 
@@ -6245,6 +6247,8 @@ 
         updatecheck = 'none'
 
     with repo.wlock():
+        if abort:
+            return hg.abortupdate(ui, repo)
         cmdutil.clearunfinished(repo)
         if date:
             rev = cmdutil.finddate(ui, repo, date)
diff --git a/hgext/transplant.py b/hgext/transplant.py
--- a/hgext/transplant.py
+++ b/hgext/transplant.py
@@ -786,13 +786,27 @@ 
     return n and nodemod.hex(n) or ''
 
 def extsetup(ui):
+<<<<<<< destination
     statemod.addunfinished (
         'transplant', fname='transplant/journal', clearable=True,
         continuefunc=continuecmd,
         statushint=_('To continue:    hg transplant --continue\n'
                      'To stop:        hg transplant --stop'),
         cmdhint=_("use 'hg transplant --continue' or 'hg transplant --stop'")
+=======
+     statemod.addunfinished (
+         'transplant', fname='transplant/journal', clearable=True,
+<<<<<<< destination
+         continuefunc=continuecmd,
+         statushint=_('To continue:    hg transplant --continue\n'
+                      'To stop:        hg transplant --stop'),
+         cmdhint=_("use 'hg transplant --continue' or 'hg transplant --stop'")
+=======
+        continuefunc=continuecmd, abortfunc=aborttransplant,
+>>>>>>> evolving
+>>>>>>> evolving
     )
 
+
 # tell hggettext to extract docstrings from these functions:
 i18nfunctions = [revsettransplanted, kwtransplanted]