Patchwork D6567: abort: added functionality for graft

login
register
mail settings
Submitter phabricator
Date June 23, 2019, 5:43 p.m.
Message ID <differential-rev-PHID-DREV-vxzgbwfylqtqqn3lhtao-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40653/
State Superseded
Headers show

Comments

phabricator - June 23, 2019, 5:43 p.m.
taapas1128 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This modifies existing `_abortgraft()` and removes its dependency
  of `graftstate`. This is done because the operations listed in the
  previous commits have special objects w.r.t abort. This dependency must
  be removed.
  This also `graft` support to `hg abort`.
  
  Tests have been shown as `test-abort.t`

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/commands.py
  mercurial/state.py
  tests/test-abort.t

CHANGE DETAILS




To: taapas1128, #hg-reviewers
Cc: mercurial-devel
phabricator - June 30, 2019, 7:13 p.m.
pulkit added inline comments.

INLINE COMMENTS

> test-abort.t:2
> +
> +####TEST `hg abort` operation graft
> +

I think we can use cases functionality in existing tests to test `hg abort`.

I mean, in one case alias `abort` to `graft --abort` and in other case it will be the normal abort command. That will help us to prevent duplicating tests and making sure in future things work for both the cases.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - June 30, 2019, 7:15 p.m.
pulkit added inline comments.

INLINE COMMENTS

> commands.py:2738
> +    'graft', fname='graftstate', clearable=True, stopflag=True,
> +    continueflag=True, abortfunc=_abortgraft,
> +    cmdhint=_("use 'hg graft --continue' or 'hg graft --stop' to stop")

Directly calling `_abortgraft` is not safe. `graft --abort` takes wlock first and then calls it. We skipped taking the lock in case of `hg abort`.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - July 4, 2019, 2:59 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> pulkit wrote in commands.py:2738
> Directly calling `_abortgraft` is not safe. `graft --abort` takes wlock first and then calls it. We skipped taking the lock in case of `hg abort`.

@pulkit the wlock is activated later in `graft --abort` during cleanup. That is same in the case of `hg abort`

> pulkit wrote in test-abort.t:2
> I think we can use cases functionality in existing tests to test `hg abort`.
> 
> I mean, in one case alias `abort` to `graft --abort` and in other case it will be the normal abort command. That will help us to prevent duplicating tests and making sure in future things work for both the cases.

I will alias this later if you want once the code for this is finalized. this is only a portion of tests taken and tests for `no-backup` and `dry-run` are added.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - July 4, 2019, 3 p.m.
pulkit added inline comments.

INLINE COMMENTS

> taapas1128 wrote in commands.py:2738
> @pulkit the wlock is activated later in `graft --abort` during cleanup. That is same in the case of `hg abort`

`_dograft()` is called after taking the lock. `_abortgraft` before taking the lock does a lot of reading which should be covered with lock. Otherwise there can be a process which changes state of things between we read the data and we operate on it.

> taapas1128 wrote in test-abort.t:2
> I will alias this later if you want once the code for this is finalized. this is only a portion of tests taken and tests for `no-backup` and `dry-run` are added.

Let's start aliasing from now and have current set of patches good to go.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - July 5, 2019, 9:54 p.m.
pulkit added a comment.


  > Tests have been shown as test-abort.t
  
  There is no such test now. :)

INLINE COMMENTS

> commands.py:2733
>  
> +def hgabortgraft(ui, repo, **opts):
> +    if opts.get('no_backup'):

let's move this function to cmdutil.py. Should move _readgraftstate, _abortgraftstate to cmdutil.py too. Movement can be done as a separate patch before this patch.

> commands.py:2735
> +    if opts.get('no_backup'):
> +        raise error.Abort(_("graft does not support no-backup flag"))
> +    if opts.get('dry_run'):

`aborting graft ...`

> commands.py:2736
> +        raise error.Abort(_("graft does not support no-backup flag"))
> +    if opts.get('dry_run'):
> +        return 0

We can get rid of this if if we prevent calling abortfunc() in abort command.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - July 9, 2019, 10:29 a.m.
pulkit added a comment.


  > This logic is registered to the statesetection API as abortfunc.
  
  s/statesection/statedetection
  
  > graft currently supports --dry-run flag.
  
  This sounds ambiguous. Does this means `hg graft` supports `--dry-run` flag? IIUC, `hg abort` supports `--dry-run` flag which is mentioned in last patch and we can drop the mention here. Applies to next patches too which has similar mention in commit messages.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - July 9, 2019, 12:28 p.m.
taapas1128 added a comment.


  amended.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel

Patch

diff --git a/tests/test-abort.t b/tests/test-abort.t
new file mode 100644
--- /dev/null
+++ b/tests/test-abort.t
@@ -0,0 +1,123 @@ 
+
+####TEST `hg abort` operation graft
+
+Testing the hg abort for `hg graft` which aborts and rollback to state
+before the graft
+
+  $ hg init abortgraft
+  $ cd abortgraft
+  $ for ch in a b c d; do echo $ch > $ch; hg add $ch; hg ci -Aqm "added "$ch; done;
+
+  $ hg up '.^^'
+  0 files updated, 0 files merged, 2 files removed, 0 files unresolved
+
+  $ echo x > x
+  $ hg ci -Aqm "added x"
+  $ hg up '.^'
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ echo foo > c
+  $ hg ci -Aqm "added foo to c"
+
+  $ hg log -GT "{rev}:{node|short} {desc}"
+  @  5:36b793615f78 added foo to c
+  |
+  | o  4:863a25e1a9ea added x
+  |/
+  | o  3:9150fe93bec6 added d
+  | |
+  | o  2:155349b645be added c
+  |/
+  o  1:5f6d8a4bf34a added b
+  |
+  o  0:9092f1db7931 added a
+  
+  $ hg up 9150fe93bec6
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+  $ hg abort
+  abort: no operation in progress
+  [255]
+
+when stripping is required
+  $ hg graft -r 4 -r 5
+  grafting 4:863a25e1a9ea "added x"
+  grafting 5:36b793615f78 "added foo to c" (tip)
+  merging c
+  warning: conflicts while merging c! (edit, then use 'hg resolve --mark')
+  abort: unresolved conflicts, can't continue
+  (use 'hg resolve' and 'hg graft --continue')
+  [255]
+
+  $ hg abort
+  graft aborted
+  working directory is now at 9150fe93bec6
+  $ hg log -GT "{rev}:{node|short} {desc}"
+  o  5:36b793615f78 added foo to c
+  |
+  | o  4:863a25e1a9ea added x
+  |/
+  | @  3:9150fe93bec6 added d
+  | |
+  | o  2:155349b645be added c
+  |/
+  o  1:5f6d8a4bf34a added b
+  |
+  o  0:9092f1db7931 added a
+  
+when stripping is not required
+  $ hg graft -r 5
+  grafting 5:36b793615f78 "added foo to c" (tip)
+  merging c
+  warning: conflicts while merging c! (edit, then use 'hg resolve --mark')
+  abort: unresolved conflicts, can't continue
+  (use 'hg resolve' and 'hg graft --continue')
+  [255]
+
+  $ hg abort
+  graft aborted
+  working directory is now at 9150fe93bec6
+  $ hg log -GT "{rev}:{node|short} {desc}"
+  o  5:36b793615f78 added foo to c
+  |
+  | o  4:863a25e1a9ea added x
+  |/
+  | @  3:9150fe93bec6 added d
+  | |
+  | o  2:155349b645be added c
+  |/
+  o  1:5f6d8a4bf34a added b
+  |
+  o  0:9092f1db7931 added a
+  
+when some of the changesets became public
+
+  $ hg graft -r 4 -r 5
+  grafting 4:863a25e1a9ea "added x"
+  grafting 5:36b793615f78 "added foo to c" (tip)
+  merging c
+  warning: conflicts while merging c! (edit, then use 'hg resolve --mark')
+  abort: unresolved conflicts, can't continue
+  (use 'hg resolve' and 'hg graft --continue')
+  [255]
+
+  $ hg log -GT "{rev}:{node|short} {desc}"
+  @  6:6ec71c037d94 added x
+  |
+  | o  5:36b793615f78 added foo to c
+  | |
+  | | o  4:863a25e1a9ea added x
+  | |/
+  o |  3:9150fe93bec6 added d
+  | |
+  o |  2:155349b645be added c
+  |/
+  o  1:5f6d8a4bf34a added b
+  |
+  o  0:9092f1db7931 added a
+  
+  $ hg phase -r 6 --public
+
+  $ hg abort
+  cannot clean up public changesets 6ec71c037d94
+  graft aborted
+  working directory is now at 6ec71c037d94
diff --git a/mercurial/state.py b/mercurial/state.py
--- a/mercurial/state.py
+++ b/mercurial/state.py
@@ -185,7 +185,13 @@ 
 def addunfinished(opname, **kwargs):
     """this registers a new command or operation to unfinishedstates
     """
-    statecheckobj = _statecheck(opname, **kwargs)
+    from . import commands
+    if opname == 'graft':
+        statecheckobj = _statecheck(opname, abortfunc=commands._abortgraft,
+                                    **kwargs)
+    else:
+        statecheckobj = _statecheck(opname, **kwargs)
+
     if opname == 'merge':
         _unfinishedstates.append(statecheckobj)
     else:
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2481,7 +2481,7 @@ 
                 opts.get('currentuser'), opts.get('rev'))):
             raise error.Abort(_("cannot specify any other flag with '--abort'"))
 
-        return _abortgraft(ui, repo, graftstate)
+        return _abortgraft(ui, repo)
     elif opts.get('continue'):
         cont = True
         if revs:
@@ -2658,9 +2658,10 @@ 
 
     return 0
 
-def _abortgraft(ui, repo, graftstate):
+def _abortgraft(ui, repo):
     """abort the interrupted graft and rollbacks to the state before interrupted
     graft"""
+    graftstate = statemod.cmdstate(repo, 'graftstate')
     if not graftstate.exists():
         raise error.Abort(_("no interrupted graft to abort"))
     statedata = _readgraftstate(repo, graftstate)