Patchwork D6659: graft: split graft code to avoid duplication

login
register
mail settings
Submitter phabricator
Date July 18, 2019, 8:21 p.m.
Message ID <differential-rev-PHID-DREV-cpkunbgxd35z6d2l75by-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40977/
State New
Headers show

Comments

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

REVISION SUMMARY
  To avoid duplication of code due to `cmdutil.continuegraft()`; `graft()`
  is split into `cmdutil.finishgraft()` which deals with the execution of graft
  once `revs` are generated and `cmdutil.continuegraftstate()` which updates opts
  from `graftstate` file when graft is continued from an interrupted state.
  
  Further tests for issue1175 are updated to show the behaviour of `hg continue`.

REPOSITORY
  rHG Mercurial

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

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

CHANGE DETAILS




To: taapas1128, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 1, 2019, 5:42 p.m.
martinvonz added a comment.


  I just found a comment I wrote a while ago and forgot to submit. Sorry.

INLINE COMMENTS

> cmdutil.py:3495-3508
> +def continuegraftstate(repo, graftstate, opts):
> +    """updates opts based on the interrupted graftstate once
> +    '--continue' flag is called."""
> +    statedata = readgraftstate(repo, graftstate)
> +    if statedata.get('date'):
> +        opts['date'] = statedata['date']
> +    if statedata.get('user'):

I think we generally avoid having functions mutate inputs and instead make them return the new value. The usual reason for mutating instead is for speed, but that should not be an issue here. The side-effect is especially non-obvious (when looking at call site) since the function has a return value (if it had not had one, the reader would probably assume there must be some side-effect).

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Aug. 1, 2019, 6:36 p.m.
This revision now requires changes to proceed.
durin42 added a comment.
durin42 requested changes to this revision.


  I agree with Martin's request for changes here.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42
Cc: durin42, martinvonz, mercurial-devel
phabricator - Aug. 4, 2019, 12:16 p.m.
taapas1128 added a comment.


  @martinvonz @durin42 Have a look I ve updated the patch.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42
Cc: durin42, martinvonz, mercurial-devel
phabricator - Aug. 7, 2019, 10:09 p.m.
pulkit added inline comments.

INLINE COMMENTS

> cmdutil.py:3431
> +    """logic to execute graft once revs are generated"""
> +    graftstate = statemod.cmdstate(repo, 'graftstate')
> +    for pos, ctx in enumerate(repo.set("%ld", revs)):

let's initialize `graftstate` only where we need it.

> cmdutil.py:3495
> +
> +def continuegraftstate(repo, graftstate, opts):
> +    """updates opts based on the interrupted graftstate once

the function name is not correct with respect to what it does, maybe something like `getopts` or something. Also, we no longer update the `opts`, so we can drop that as argument and also need to update the function description.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers, durin42
Cc: pulkit, durin42, martinvonz, mercurial-devel
phabricator - Aug. 8, 2019, 4:22 p.m.
taapas1128 added inline comments.

INLINE COMMENTS

> pulkit wrote in cmdutil.py:3495
> the function name is not correct with respect to what it does, maybe something like `getopts` or something. Also, we no longer update the `opts`, so we can drop that as argument and also need to update the function description.

I havent removed `opts` from argument yet.

REPOSITORY
  rHG Mercurial

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

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

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

Patch

diff --git a/tests/test-issue1175.t b/tests/test-issue1175.t
--- a/tests/test-issue1175.t
+++ b/tests/test-issue1175.t
@@ -1,3 +1,11 @@ 
+#testcases continueflag continuecommand
+#if continueflag
+  $ cat >> $HGRCPATH <<EOF
+  > [alias]
+  > continue = graft --continue
+  > EOF
+#endif
+
 https://bz.mercurial-scm.org/1175
 
   $ hg init
@@ -80,7 +88,7 @@ 
   $ hg resolve --mark b
   (no more unresolved files)
   continue: hg graft --continue
-  $ hg graft --continue
+  $ hg continue
   grafting 1:5974126fad84 "b1"
   warning: can't find ancestor for 'b' copied from 'a'!
   $ hg log -f b -T 'changeset:   {rev}:{node|short}\nsummary:     {desc}\n\n'
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2474,9 +2474,6 @@ 
     if not opts.get('date') and opts.get('currentdate'):
         opts['date'] = "%d %d" % dateutil.makedate()
 
-    editor = cmdutil.getcommiteditor(editform='graft',
-                                     **pycompat.strkwargs(opts))
-
     cont = False
     if opts.get('no_commit'):
         if opts.get('edit'):
@@ -2522,17 +2519,7 @@ 
             raise error.Abort(_("can't specify --continue and revisions"))
         # read in unfinished revisions
         if graftstate.exists():
-            statedata = cmdutil.readgraftstate(repo, graftstate)
-            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]
+            revs = cmdutil.continuegraftstate(repo, graftstate, opts)
         else:
             cmdutil.wrongtooltocontinue(repo, _('graft'))
     else:
@@ -2623,69 +2610,8 @@ 
 
     if opts.get('no_commit'):
         statedata['no_commit'] = True
-    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)
-        if opts.get('dry_run'):
-            continue
-
-        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
-
-        # we don't merge the first commit when continuing
-        if not cont:
-            # perform the graft merge with p1(rev) as 'ancestor'
-            overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
-            base = ctx.p1() if basectx is None else basectx
-            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
-
-        # commit if --no-commit is false
-        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
-            elif statedata.get('newnodes') is not None:
-                statedata['newnodes'].append(node)
-
+
+    cmdutil.finishgraft(repo, ui, basectx, revs, statedata, cont, opts)
     # remove state when we complete successfully
     if not opts.get('dry_run'):
         graftstate.delete()
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -3430,85 +3430,101 @@ 
     """logic to resume interrupted graft using 'hg continue'"""
     with repo.wlock():
         graftstate = statemod.cmdstate(repo, 'graftstate')
-        statedata = readgraftstate(repo, graftstate)
         opts = {}
+        statedata = {}
         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]
-
+        basectx = None
+        revs = continuegraftstate(repo, graftstate, opts)
         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 basectx is None:
+        # check for merges
+            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,
+        finishgraft(repo, ui, basectx, revs, statedata, cont, opts)
+        graftstate.delete()
+        return 0
+
+def finishgraft(repo, ui, basectx, revs, statedata, cont, opts):
+    """logic to execute graft once revs are generated"""
+    graftstate = statemod.cmdstate(repo, 'graftstate')
+    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(
+        names = repo.nodetags(ctx.node()) + repo.nodebookmarks(ctx.node())
+        if names:
+            desc += ' (%s)' % ' '.join(names)
+        ui.status(_('grafting %s\n') % desc)
+        if opts.get('dry_run'):
+            continue
+
+        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
+
+        # we don't merge the first commit when continuing
+        if not cont:
+            # perform the graft merge with p1(rev) as 'ancestor'
+            overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
+            base = ctx.p1() if basectx is None else basectx
+            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))
+        # commit if --no-commit is false
+        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
+            elif statedata.get('newnodes') is not None:
+                statedata['newnodes'].append(node)
+
+def continuegraftstate(repo, graftstate, opts):
+    """updates opts based on the interrupted graftstate once
+    '--continue' flag is called."""
+    statedata = readgraftstate(repo, graftstate)
+    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']
+    return [repo[node].rev() for node in nodes]