Patchwork D6655: continue: added support for graft

login
register
mail settings
Submitter phabricator
Date July 18, 2019, 5:46 p.m.
Message ID <differential-rev-PHID-DREV-ergrsmczumvhuh57ly6n-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40969/
State Superseded
Headers show

Comments

phabricator - July 18, 2019, 5:46 p.m.
taapas1128 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This adds support of graft to hg continue plan.
  
  The patch creates a seperate function `cmdutil.continuegraft`
  so that continue logic for graft can be called independently.
  This logic is registered to the statedetection API as `continuefunc`.
  
  Results are shown as tests.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/commands.py
  tests/test-graft.t

CHANGE DETAILS




To: taapas1128, #hg-reviewers
Cc: mercurial-devel
phabricator - July 18, 2019, 5:58 p.m.
durin42 added a comment.


  I'm landing this, but in the future I suspect we should make sure we just have a mix of `hg graft --continue` and `hg graft` rather than run the whole test twice, as it'll severely bloat the already-slow testsuite to do that for many tests.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - July 18, 2019, 6:20 p.m.
martinvonz added a comment.


  There is a lot of duplication here. Can you try to extract common parts?

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: martinvonz, durin42, mercurial-devel
phabricator - July 18, 2019, 7:12 p.m.
taapas1128 added a comment.


  @martinvonz I will send a patch doing that.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: martinvonz, durin42, mercurial-devel
phabricator - July 19, 2019, 4:19 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> test-graft.t:1
> -#testcases abortcommand abortflag
> +#testcases abortcommand abortflag continueflag continuecommand
>  

This has the same problem D6579 <https://phab.mercurial-scm.org/D6579> had where there are 4 separate test cases, instead of 2 with sub cases.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, martinvonz, durin42, mercurial-devel
phabricator - July 19, 2019, 10:03 p.m.
martinvonz added a comment.


  In D6655#97447 <https://phab.mercurial-scm.org/D6655#97447>, @taapas1128 wrote:
  
  > @martinvonz I will send a patch doing that.

INLINE COMMENTS

> mharbison72 wrote in test-graft.t:1
> This has the same problem D6579 <https://phab.mercurial-scm.org/D6579> had where there are 4 separate test cases, instead of 2 with sub cases.

I tried to fix that but the test fails if I do that.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, martinvonz, durin42, mercurial-devel
phabricator - July 19, 2019, 10:21 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> cmdutil.py:3482-3498
> +            if not cont:
> +                # perform the graft merge with p1(rev) as 'ancestor'
> +                overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
> +                base = ctx.p1()
> +                with ui.configoverride(overrides, 'graft'):
> +                    stats = mergemod.graft(repo, ctx, base, ['local', 'graft'])
> +                # report any conflicts

It looks like pretty much this entire function is copies `commands.graft()` and then all this code is left dead here (since `cont = True` above). I'd prefer to de-queue this commit and let you clean this up and then submit a new patch, but maybe other reviewers are less picky or stubborn than I am, so I'll leave it queued until I hear from someone else.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, martinvonz, durin42, mercurial-devel
phabricator - July 20, 2019, 3:48 a.m.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in cmdutil.py:3482-3498
> It looks like pretty much this entire function is copies `commands.graft()` and then all this code is left dead here (since `cont = True` above). I'd prefer to de-queue this commit and let you clean this up and then submit a new patch, but maybe other reviewers are less picky or stubborn than I am, so I'll leave it queued until I hear from someone else.

@martinvonz That code is not dead but it takes care not to merge the first commit when continuing graft. `cont=True` is just the first iteration of the for loop at line `3455` after that it becomes `False`. Although it may seem like all the code has been copied from `commands.graft` but only that part is copied but only that part is imported that works when `--continue` flag is called. Regarding cleaning I do agree spliting code from `commands.graft()` would be nice. So have a look at D6659 <https://phab.mercurial-scm.org/D6659>.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, martinvonz, durin42, mercurial-devel
Yuya Nishihara - July 20, 2019, 7:35 a.m.
> It looks like pretty much this entire function is copies `commands.graft()`
> and then all this code is left dead here (since `cont = True` above).
> I'd prefer to de-queue this commit and let you clean this up and then
> submit a new patch

That's my feeling. Perhaps the core `for` loop can be extracted to a helper
function. Or alternatively, `continuegraft()` can be implemented on top of
commands.graft()?
phabricator - July 20, 2019, 7:36 a.m.
yuja added a comment.


  > It looks like pretty much this entire function is copies `commands.graft()`
  > and then all this code is left dead here (since `cont = True` above).
  > I'd prefer to de-queue this commit and let you clean this up and then
  > submit a new patch
  
  That's my feeling. Perhaps the core `for` loop can be extracted to a helper
  function. Or alternatively, `continuegraft()` can be implemented on top of
  commands.graft()?

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: yuja, mharbison72, martinvonz, durin42, mercurial-devel
phabricator - July 20, 2019, 1:46 p.m.
taapas1128 added a comment.


  @yuja have a look at D6659 <https://phab.mercurial-scm.org/D6659> and suggest changes there. Extracting the core `for` loop as a helper function sounds like a nice idea. I will update the patch for the same.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: yuja, mharbison72, martinvonz, durin42, mercurial-devel
phabricator - July 20, 2019, 1:46 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-graft.t:1
> I tried to fix that but the test fails if I do that.

I will have a look.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: yuja, mharbison72, martinvonz, durin42, mercurial-devel
phabricator - July 20, 2019, 2:06 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> mharbison72 wrote in test-graft.t:1
> This has the same problem D6579 <https://phab.mercurial-scm.org/D6579> had where there are 4 separate test cases, instead of 2 with sub cases.

@mharbison72 I have generated a patch for that in D6659 <https://phab.mercurial-scm.org/D6659>. I minimized it to 3 cases as @durin42 stated about bloating of the test sets.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: yuja, mharbison72, martinvonz, durin42, mercurial-devel
phabricator - July 21, 2019, 2:10 p.m.
martinvonz added a comment.


  Sorry, but since both Yuya and I are uncomfortable with this patch, I'll de-queue it so it doesn't block other patches from getting queued. I think it would be good if you can fold D6659 <https://phab.mercurial-scm.org/D6659> into this one and then split it up so have only the refactoring in a first patch and the new support for continue in a second patch. Thanks.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: yuja, mharbison72, martinvonz, durin42, mercurial-devel
phabricator - July 21, 2019, 2:12 p.m.
taapas1128 added a comment.


  @martinvonz No problem, I will update patch D6659 <https://phab.mercurial-scm.org/D6659>.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: yuja, mharbison72, martinvonz, durin42, mercurial-devel

Patch

diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -1,4 +1,4 @@ 
-#testcases abortcommand abortflag
+#testcases abortcommand abortflag continueflag continuecommand
 
   $ cat >> $HGRCPATH <<EOF
   > [extdiff]
@@ -13,6 +13,13 @@ 
   > EOF
 #endif
 
+#if continueflag
+  $ cat >> $HGRCPATH <<EOF
+  > [alias]
+  > continue = graft --continue
+  > EOF
+#endif
+
 Create a repo with some stuff in it:
 
   $ hg init a
@@ -92,9 +99,11 @@ 
 
   $ hg -q up -cr tip
   $ hg rm -q e
-  $ hg graft --continue
-  abort: no graft in progress
-  [255]
+  $ hg continue
+  abort: no graft in progress (continueflag !)
+  abort: no operation in progress (no-continueflag !)
+  [255]
+
   $ hg revert -r . -q e
 
 Need to specify a rev:
@@ -1697,7 +1706,13 @@ 
   $ hg resolve -m
   (no more unresolved files)
   continue: hg graft --continue
-  $ hg graft --continue
+
+#if continuecommand
+  $ hg continue --dry-run
+  graft in progress, will be resumed
+#endif
+
+  $ hg continue
   grafting 1:80e6d2c47cfe "added b"
   grafting 2:8be98ac1a569 "added c"
 
@@ -1754,7 +1769,7 @@ 
   (no more unresolved files)
   continue: hg graft --continue
 
-  $ hg graft --continue
+  $ hg continue
   grafting 1:80e6d2c47cfe "added b"
   grafting 2:8be98ac1a569 "added c"
 
@@ -1803,7 +1818,7 @@ 
   $ hg resolve -m
   (no more unresolved files)
   continue: hg graft --continue
-  $ hg graft --continue
+  $ hg continue
   grafting 1:80e6d2c47cfe "added b"
   grafting 2:8be98ac1a569 "added c"
 
@@ -1843,7 +1858,7 @@ 
   (no more unresolved files)
   continue: hg graft --continue
 
-  $ hg graft --continue
+  $ hg continue
   grafting 1:80e6d2c47cfe "added b"
   grafting 2:8be98ac1a569 "added c"
 
@@ -1997,7 +2012,7 @@ 
 
   $ hg abort
   abort: no interrupted graft to abort (abortflag !)
-  abort: no operation in progress (abortcommand !)
+  abort: no operation in progress (no-abortflag !)
   [255]
 
 when stripping is required
@@ -2273,7 +2288,7 @@ 
   (no more unresolved files)
   continue: hg graft --continue
 
-  $ hg graft --continue
+  $ hg continue
   grafting 3:09e253b87e17 "A in file a"
   $ hg log -GT "{rev}:{node|short} {desc}\n"
   @  4:2aa9ad1006ff B in file a
@@ -2350,7 +2365,7 @@ 
   $ hg resolve --mark
   (no more unresolved files)
   continue: hg graft --continue
-  $ hg graft --continue
+  $ hg continue
   grafting 3:09e253b87e17 "A in file a"
   $ hg diff
   diff -r 2aa9ad1006ff a
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2706,6 +2706,7 @@ 
 statemod.addunfinished(
     'graft', fname='graftstate', clearable=True, stopflag=True,
     continueflag=True, abortfunc=cmdutil.hgabortgraft,
+    continuefunc=cmdutil.continuegraft,
     cmdhint=_("use 'hg graft --continue' or 'hg graft --stop' to stop")
 )
 
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -3425,3 +3425,90 @@ 
     with repo.wlock():
         graftstate = statemod.cmdstate(repo, 'graftstate')
         return abortgraft(ui, repo, graftstate)
+
+def continuegraft(ui, repo):
+    """logic to resume interrupted graft using 'hg continue'"""
+    with repo.wlock():
+        graftstate = statemod.cmdstate(repo, 'graftstate')
+        statedata = readgraftstate(repo, graftstate)
+        opts = {}
+        cont = True
+        if statedata.get('date'):
+            opts['date'] = statedata['date']
+        if statedata.get('user'):
+            opts['user'] = statedata['user']
+        if statedata.get('log'):
+            opts['log'] = True
+        if statedata.get('no_commit'):
+            opts['no_commit'] = statedata.get('no_commit')
+        nodes = statedata['nodes']
+        revs = [repo[node].rev() for node in nodes]
+
+        skipped = set()
+        for rev in repo.revs('%ld and merge()', revs):
+            ui.warn(_('skipping ungraftable merge revision %d\n') % rev)
+            skipped.add(rev)
+        revs = [r for r in revs if r not in skipped]
+        if not revs:
+            return -1
+
+        for pos, ctx in enumerate(repo.set("%ld", revs)):
+            desc = '%d:%s "%s"' % (ctx.rev(), ctx,
+                               ctx.description().split('\n', 1)[0])
+            names = repo.nodetags(ctx.node()) + repo.nodebookmarks(ctx.node())
+            if names:
+                desc += ' (%s)' % ' '.join(names)
+            ui.status(_('grafting %s\n') % desc)
+
+            source = ctx.extra().get('source')
+            extra = {}
+            if source:
+                extra['source'] = source
+                extra['intermediate-source'] = ctx.hex()
+            else:
+                extra['source'] = ctx.hex()
+            user = ctx.user()
+            if opts.get('user'):
+                user = opts['user']
+                statedata['user'] = user
+            date = ctx.date()
+            if opts.get('date'):
+                date = opts['date']
+                statedata['date'] = date
+            message = ctx.description()
+            if opts.get('log'):
+                message += '\n(grafted from %s)' % ctx.hex()
+                statedata['log'] = True
+            if not cont:
+                # perform the graft merge with p1(rev) as 'ancestor'
+                overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
+                base = ctx.p1()
+                with ui.configoverride(overrides, 'graft'):
+                    stats = mergemod.graft(repo, ctx, base, ['local', 'graft'])
+                # report any conflicts
+                if stats.unresolvedcount > 0:
+                    # write out state for --continue
+                    nodes = [repo[rev].hex() for rev in revs[pos:]]
+                    statedata['nodes'] = nodes
+                    stateversion = 1
+                    graftstate.save(stateversion, statedata)
+                    hint = _("use 'hg resolve' and 'hg graft --continue'")
+                    raise error.Abort(
+                            _("unresolved conflicts, can't continue"),
+                            hint=hint)
+            else:
+                cont = False
+            editor = getcommiteditor(editform='graft',
+                                      **pycompat.strkwargs(opts))
+            if not opts.get('no_commit'):
+                node = repo.commit(text=message, user=user, date=date,
+                                   extra=extra, editor=editor)
+                if node is None:
+                    ui.warn(
+                    _('note: graft of %d:%s created no changes to commit\n') %
+                    (ctx.rev(), ctx))
+            # checking that newnodes exist because old state files won't have it
+                if statedata.get('newnodes') is not None:
+                    statedata['newnodes'].append(node)
+            graftstate.delete()
+        return 0