Patchwork [2,of,4,V2] resolve: move implementation into cmdutil

login
register
mail settings
Submitter Siddharth Agarwal
Date March 21, 2016, 3:18 a.m.
Message ID <65d7aff92105e37a40d8.1458530320@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/14010/
State Changes Requested
Delegated to: Martin von Zweigbergk
Headers show

Comments

Siddharth Agarwal - March 21, 2016, 3:18 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1458530235 25200
#      Sun Mar 20 20:17:15 2016 -0700
# Node ID 65d7aff92105e37a40d845d7dd95267cfb920299
# Parent  b190b4bd5de25c137286cbb21572e9e1160b415b
resolve: move implementation into cmdutil

This function's getting way too big, and having this will allow us to wrap more
of resolve inside a ui.tempconfig context.

For 'showhint' we return a tuple so that we can return multiple pieces of
information. That'll come in handy in a subsequent patch.

The passing in of 'flaglist' is temporary -- in a subsequent patch we'll stop
passing it in.
via Mercurial-devel - March 22, 2016, 5:16 p.m.
On Sun, Mar 20, 2016 at 8:18 PM, Siddharth Agarwal <sid0@fb.com> wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1458530235 25200
> #      Sun Mar 20 20:17:15 2016 -0700
> # Node ID 65d7aff92105e37a40d845d7dd95267cfb920299
> # Parent  b190b4bd5de25c137286cbb21572e9e1160b415b
> resolve: move implementation into cmdutil
>
> This function's getting way too big, and having this will allow us to wrap more
> of resolve inside a ui.tempconfig context.
>
> For 'showhint' we return a tuple so that we can return multiple pieces of
> information. That'll come in handy in a subsequent patch.
>
> The passing in of 'flaglist' is temporary -- in a subsequent patch we'll stop
> passing it in.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2883,6 +2883,142 @@ def commitstatus(repo, node, branch, bhe
>  def postcommitstatus(repo, pats, opts):
>      return repo.status(match=scmutil.match(repo[None], pats, opts))
>
> +def resolve(ui, repo, ms, pats, opts, flaglist):
> +    from . import merge as mergemod

Why is it imported here? I've seen this kind of thing done in a few
other places, but can you remind me when it's needed/useful? Is it
some import cycle? If it is, then which direction is correct, for
cmdutil to depend on merge or for merge to depend on cmdutil?

I think I saw someone send patches for resolving a cycle involving
mergestate. Do you know if that's related?
Siddharth Agarwal - March 22, 2016, 5:18 p.m.
On 3/22/16 10:16, Martin von Zweigbergk via Mercurial-devel wrote:
> Why is it imported here? I've seen this kind of thing done in a few
> other places, but can you remind me when it's needed/useful? Is it
> some import cycle? If it is, then which direction is correct, for
> cmdutil to depend on merge or for merge to depend on cmdutil?

Hmm, I think there's a cycle involving three or four different modules. 
Will need to dig in to find out more.

>
> I think I saw someone send patches for resolving a cycle involving
> mergestate. Do you know if that's related?

Yeah, that's the real fix here.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2883,6 +2883,142 @@  def commitstatus(repo, node, branch, bhe
 def postcommitstatus(repo, pats, opts):
     return repo.status(match=scmutil.match(repo[None], pats, opts))
 
+def resolve(ui, repo, ms, pats, opts, flaglist):
+    from . import merge as mergemod
+    mark = opts.get('mark')
+    unmark = opts.get('unmark')
+
+    if not (ms.active() or repo.dirstate.p2() != nullid):
+        raise error.Abort(
+            _('resolve command not applicable when not merging'))
+
+    wctx = repo[None]
+
+    if ms.mergedriver and ms.mdstate() == 'u':
+        proceed = mergemod.driverpreprocess(repo, ms, wctx)
+        ms.commit()
+        # allow mark and unmark to go through
+        if not mark and not unmark and not proceed:
+            return 1, None
+
+    m = scmutil.match(wctx, pats, opts)
+    ret = 0
+    didwork = False
+    runconclude = False
+
+    tocomplete = []
+    for f in ms:
+        if not m(f):
+            continue
+
+        didwork = True
+
+        # don't let driver-resolved files be marked, and run the conclude
+        # step if asked to resolve
+        if ms[f] == "d":
+            exact = m.exact(f)
+            if mark:
+                if exact:
+                    ui.warn(_('not marking %s as it is driver-resolved\n')
+                            % f)
+            elif unmark:
+                if exact:
+                    ui.warn(_('not unmarking %s as it is driver-resolved\n')
+                            % f)
+            else:
+                runconclude = True
+            continue
+
+        if mark:
+            ms.mark(f, "r")
+        elif unmark:
+            ms.mark(f, "u")
+        else:
+            # backup pre-resolve (merge uses .orig for its own purposes)
+            a = repo.wjoin(f)
+            try:
+                util.copyfile(a, a + ".resolve")
+            except (IOError, OSError) as inst:
+                if inst.errno != errno.ENOENT:
+                    raise
+
+            try:
+                # preresolve file
+                ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
+                             'resolve')
+                complete, r = ms.preresolve(f, wctx)
+                if not complete:
+                    tocomplete.append(f)
+                elif r:
+                    ret = 1
+            finally:
+                ui.setconfig('ui', 'forcemerge', '', 'resolve')
+                ms.commit()
+
+            # replace filemerge's .orig file with our resolve file, but only
+            # for merges that are complete
+            if complete:
+                try:
+                    util.rename(a + ".resolve",
+                                scmutil.origpath(ui, repo, a))
+                except OSError as inst:
+                    if inst.errno != errno.ENOENT:
+                        raise
+
+    for f in tocomplete:
+        try:
+            # resolve file
+            ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
+                         'resolve')
+            r = ms.resolve(f, wctx)
+            if r:
+                ret = 1
+        finally:
+            ui.setconfig('ui', 'forcemerge', '', 'resolve')
+            ms.commit()
+
+        # replace filemerge's .orig file with our resolve file
+        a = repo.wjoin(f)
+        try:
+            util.rename(a + ".resolve", scmutil.origpath(ui, repo, a))
+        except OSError as inst:
+            if inst.errno != errno.ENOENT:
+                raise
+
+    ms.commit()
+    ms.recordactions()
+
+    if not didwork and pats:
+        hint = None
+        if not any([p for p in pats if p.find(':') >= 0]):
+            pats = ['path:%s' % p for p in pats]
+            m = scmutil.match(wctx, pats, opts)
+            for f in ms:
+                if not m(f):
+                    continue
+                flags = ''.join(['-%s ' % o[0] for o in flaglist
+                                               if opts.get(o)])
+                hint = _("(try: hg resolve %s%s)\n") % (
+                         flags,
+                         ' '.join(pats))
+                break
+        ui.warn(_("arguments do not match paths that need resolving\n"))
+        if hint:
+            ui.warn(hint)
+    elif ms.mergedriver and ms.mdstate() != 's':
+        # run conclude step when either a driver-resolved file is requested
+        # or there are no driver-resolved files
+        # we can't use 'ret' to determine whether any files are unresolved
+        # because we might not have tried to resolve some
+        if ((runconclude or not list(ms.driverresolved()))
+            and not list(ms.unresolved())):
+            proceed = mergemod.driverconclude(repo, ms, wctx)
+            ms.commit()
+            if not proceed:
+                return 1, None
+
+    return ret, ('final',)
+
 def revert(ui, repo, ctx, parents, *pats, **opts):
     parent, p2 = parents
     node = ctx.node()
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -6025,145 +6025,20 @@  def resolve(ui, repo, *pats, **opts):
 
     with repo.wlock():
         ms = mergemod.mergestate.read(repo)
-
-        if not (ms.active() or repo.dirstate.p2() != nullid):
-            raise error.Abort(
-                _('resolve command not applicable when not merging'))
-
-        wctx = repo[None]
-
-        if ms.mergedriver and ms.mdstate() == 'u':
-            proceed = mergemod.driverpreprocess(repo, ms, wctx)
-            ms.commit()
-            # allow mark and unmark to go through
-            if not mark and not unmark and not proceed:
-                return 1
-
-        m = scmutil.match(wctx, pats, opts)
-        ret = 0
-        didwork = False
-        runconclude = False
-
-        tocomplete = []
-        for f in ms:
-            if not m(f):
-                continue
-
-            didwork = True
-
-            # don't let driver-resolved files be marked, and run the conclude
-            # step if asked to resolve
-            if ms[f] == "d":
-                exact = m.exact(f)
-                if mark:
-                    if exact:
-                        ui.warn(_('not marking %s as it is driver-resolved\n')
-                                % f)
-                elif unmark:
-                    if exact:
-                        ui.warn(_('not unmarking %s as it is driver-resolved\n')
-                                % f)
-                else:
-                    runconclude = True
-                continue
-
-            if mark:
-                ms.mark(f, "r")
-            elif unmark:
-                ms.mark(f, "u")
-            else:
-                # backup pre-resolve (merge uses .orig for its own purposes)
-                a = repo.wjoin(f)
-                try:
-                    util.copyfile(a, a + ".resolve")
-                except (IOError, OSError) as inst:
-                    if inst.errno != errno.ENOENT:
-                        raise
-
-                try:
-                    # preresolve file
-                    ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
-                                 'resolve')
-                    complete, r = ms.preresolve(f, wctx)
-                    if not complete:
-                        tocomplete.append(f)
-                    elif r:
-                        ret = 1
-                finally:
-                    ui.setconfig('ui', 'forcemerge', '', 'resolve')
-                    ms.commit()
-
-                # replace filemerge's .orig file with our resolve file, but only
-                # for merges that are complete
-                if complete:
-                    try:
-                        util.rename(a + ".resolve",
-                                    scmutil.origpath(ui, repo, a))
-                    except OSError as inst:
-                        if inst.errno != errno.ENOENT:
-                            raise
-
-        for f in tocomplete:
-            try:
-                # resolve file
-                ui.setconfig('ui', 'forcemerge', opts.get('tool', ''),
-                             'resolve')
-                r = ms.resolve(f, wctx)
-                if r:
-                    ret = 1
-            finally:
-                ui.setconfig('ui', 'forcemerge', '', 'resolve')
-                ms.commit()
-
-            # replace filemerge's .orig file with our resolve file
-            a = repo.wjoin(f)
-            try:
-                util.rename(a + ".resolve", scmutil.origpath(ui, repo, a))
-            except OSError as inst:
-                if inst.errno != errno.ENOENT:
-                    raise
-
-        ms.commit()
-        ms.recordactions()
-
-        if not didwork and pats:
-            hint = None
-            if not any([p for p in pats if p.find(':') >= 0]):
-                pats = ['path:%s' % p for p in pats]
-                m = scmutil.match(wctx, pats, opts)
-                for f in ms:
-                    if not m(f):
-                        continue
-                    flags = ''.join(['-%s ' % o[0] for o in flaglist
-                                                   if opts.get(o)])
-                    hint = _("(try: hg resolve %s%s)\n") % (
-                             flags,
-                             ' '.join(pats))
-                    break
-            ui.warn(_("arguments do not match paths that need resolving\n"))
-            if hint:
-                ui.warn(hint)
-        elif ms.mergedriver and ms.mdstate() != 's':
-            # run conclude step when either a driver-resolved file is requested
-            # or there are no driver-resolved files
-            # we can't use 'ret' to determine whether any files are unresolved
-            # because we might not have tried to resolve some
-            if ((runconclude or not list(ms.driverresolved()))
-                and not list(ms.unresolved())):
-                proceed = mergemod.driverconclude(repo, ms, wctx)
-                ms.commit()
-                if not proceed:
-                    return 1
-
-    # Nudge users into finishing an unfinished operation
-    unresolvedf = list(ms.unresolved())
-    driverresolvedf = list(ms.driverresolved())
-    if not unresolvedf and not driverresolvedf:
-        ui.status(_('(no more unresolved files)\n'))
-        cmdutil.checkafterresolved(repo)
-    elif not unresolvedf:
-        ui.status(_('(no more unresolved files -- '
-                    'run "hg resolve --all" to conclude)\n'))
+        ret, showhint = cmdutil.resolve(ui, repo, ms, pats, opts, flaglist)
+
+    if showhint is None:
+        pass
+    elif showhint[0] == 'final':
+        # Nudge users into finishing an unfinished operation
+        unresolvedf = list(ms.unresolved())
+        driverresolvedf = list(ms.driverresolved())
+        if not unresolvedf and not driverresolvedf:
+            ui.status(_('(no more unresolved files)\n'))
+            cmdutil.checkafterresolved(repo)
+        elif not unresolvedf:
+            ui.status(_('(no more unresolved files -- '
+                        'run "hg resolve --all" to conclude)\n'))
 
     return ret