Patchwork [1,of,2] dispatch: move part of callcatch to scmutil

login
register
mail settings
Submitter Jun Wu
Date Nov. 24, 2016, 1:17 a.m.
Message ID <c76f0d4bdee6bfbd7bda.1479950269@x1c>
Download mbox | patch
Permalink /patch/17742/
State Accepted
Headers show

Comments

Jun Wu - Nov. 24, 2016, 1:17 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1479948520 0
#      Thu Nov 24 00:48:40 2016 +0000
# Node ID c76f0d4bdee6bfbd7bda771d5c05939d1d4cb132
# Parent  7ef2ebf5cdf91192a66b461ff56f653a65e65dd7
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r c76f0d4bdee6
dispatch: move part of callcatch to scmutil

Per discussion at 39149b6036e6 [1], we need "callcatch" in worker.py. Move
it to scmutil.py to avoid cycles.

Note that dispatch's callcatch handles some additional high-level exceptions
related to config parsing, and commands. Moving them to scmutil will make
scmutil depend on "commands" or require "_formatparse" and "_getsimilar"
(and "difflib") to be moved as well. In the worker use-case, it is forked
when config and commands are fully loaded. So it should not care about those
exceptions.

[1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-August/087116.html
timeless - Nov. 24, 2016, 2:36 a.m.
fwiw, it is possible to keep blame for this if you really wanted to.

The approach is:
7ef2ebf5cdf91192a66b461ff56f653a65e65dd7->x
x:
 copy scmutil.py -> scmutil_.py
7ef2ebf5cdf91192a66b461ff56f653a65e65dd7->y
y:
 copy dispatch.py-> scmutil_.py
 delete lines from scmutil_.py

x+y->z
z:
 resolve merge conflicts in scmutil_.py as in this patch
 delete scmutil.py
 rename scmutil_.py -> scmutil.py
 delete lines from dispatch.py
 adjust dispatch.py as in this patch

If you do it this way, commits `x` and `y` still build (they just have
temporary files scmutil_.py which isn't used), and you still have
blame for scmutil.py for both parts.
Jun Wu - Nov. 24, 2016, 2:49 a.m.
That's an interesting idea.

I happened to be interested in the diff algorithm recently and think they
are related. A fundamental problem of the diff algorithm is, it does not
track line moves. As long as our diff algorithm cannot do that, I won't do
the merge thing manually.

Will that kind of "smart" diff algorithm exist? Probably. And if we can have
the diff algorithm which detects moves (I'm in dreaming mode), and new
version of revlog (probably will contain both the new diff algorithm, and
some kind of packing to reduce inode), it will help making deltas smaller.

And I don't think the "move" information has to be hashed / committed. It
could be calculated afterwards and store separately. (this also means I
think the git data model is cleaner). 

Excerpts from timeless's message of 2016-11-23 21:36:02 -0500:
> fwiw, it is possible to keep blame for this if you really wanted to.
> 
> The approach is:
> 7ef2ebf5cdf91192a66b461ff56f653a65e65dd7->x
> x:
>  copy scmutil.py -> scmutil_.py
> 7ef2ebf5cdf91192a66b461ff56f653a65e65dd7->y
> y:
>  copy dispatch.py-> scmutil_.py
>  delete lines from scmutil_.py
> 
> x+y->z
> z:
>  resolve merge conflicts in scmutil_.py as in this patch
>  delete scmutil.py
>  rename scmutil_.py -> scmutil.py
>  delete lines from dispatch.py
>  adjust dispatch.py as in this patch
> 
> If you do it this way, commits `x` and `y` still build (they just have
> temporary files scmutil_.py which isn't used), and you still have
> blame for scmutil.py for both parts.
Yuya Nishihara - Nov. 24, 2016, 1:28 p.m.
On Thu, 24 Nov 2016 01:17:49 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1479948520 0
> #      Thu Nov 24 00:48:40 2016 +0000
> # Node ID c76f0d4bdee6bfbd7bda771d5c05939d1d4cb132
> # Parent  7ef2ebf5cdf91192a66b461ff56f653a65e65dd7
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r c76f0d4bdee6
> dispatch: move part of callcatch to scmutil

Queued these, thanks.

> -    except ImportError as inst:
> -        ui.warn(_("abort: %s!\n") % inst)
> -        m = str(inst).split()[-1]
> -        if m in "mpatch bdiff".split():
> -            ui.warn(_("(did you forget to compile extensions?)\n"))
> -        elif m in "zlib".split():
> -            ui.warn(_("(is your Python install correct?)\n"))

This seems better belonging to the high-level callcatch(), but anyway it looks
like a moot. If C extensions aren't compiled at all, ImportError would be
raised earlier.
Jun Wu - Nov. 24, 2016, 1:39 p.m.
Excerpts from Yuya Nishihara's message of 2016-11-24 22:28:28 +0900:
> This seems better belonging to the high-level callcatch(), but anyway it looks
> like a moot. If C extensions aren't compiled at all, ImportError would be
> raised earlier.

demandimport could be a source of (surprising) ImportErrors with 3rd-party
code. Like hgsubversion imports svn binding, while the binding does not
exist.
Yuya Nishihara - Nov. 24, 2016, 2:03 p.m.
On Thu, 24 Nov 2016 13:39:25 +0000, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-11-24 22:28:28 +0900:
> > This seems better belonging to the high-level callcatch(), but anyway it looks
> > like a moot. If C extensions aren't compiled at all, ImportError would be
> > raised earlier.
> 
> demandimport could be a source of (surprising) ImportErrors with 3rd-party
> code. Like hgsubversion imports svn binding, while the binding does not
> exist.

Yeah, but "hg" calls util.setbinary(), where ImportError would be raised if
osutil.so is missing.

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -16,5 +16,4 @@  import re
 import shlex
 import signal
-import socket
 import sys
 import time
@@ -39,4 +38,5 @@  from . import (
     pycompat,
     revset,
+    scmutil,
     templatefilters,
     templatekw,
@@ -219,28 +219,13 @@  def _runcatch(req):
 
 def callcatch(ui, func):
-    """call func() with global exception handling
-
-    return func() if no exception happens. otherwise do some error handling
-    and return an exit code accordingly.
+    """like scmutil.callcatch but handles more high-level exceptions about
+    config parsing and commands. besides, use handlecommandexception to handle
+    uncaught exceptions.
     """
     try:
-        return func()
-    # Global exception handling, alphabetically
-    # Mercurial-specific first, followed by built-in and library exceptions
+        return scmutil.callcatch(ui, func)
     except error.AmbiguousCommand as inst:
         ui.warn(_("hg: command '%s' is ambiguous:\n    %s\n") %
                 (inst.args[0], " ".join(inst.args[1])))
-    except error.ParseError as inst:
-        _formatparse(ui.warn, inst)
-        return -1
-    except error.LockHeld as inst:
-        if inst.errno == errno.ETIMEDOUT:
-            reason = _('timed out waiting for lock held by %s') % inst.locker
-        else:
-            reason = _('lock held by %s') % inst.locker
-        ui.warn(_("abort: %s: %s\n") % (inst.desc or inst.filename, reason))
-    except error.LockUnavailable as inst:
-        ui.warn(_("abort: could not lock %s: %s\n") %
-               (inst.desc or inst.filename, inst.strerror))
     except error.CommandError as inst:
         if inst.args[0]:
@@ -250,32 +235,7 @@  def callcatch(ui, func):
             ui.warn(_("hg: %s\n") % inst.args[1])
             commands.help_(ui, 'shortlist')
-    except error.OutOfBandError as inst:
-        if inst.args:
-            msg = _("abort: remote error:\n")
-        else:
-            msg = _("abort: remote error\n")
-        ui.warn(msg)
-        if inst.args:
-            ui.warn(''.join(inst.args))
-        if inst.hint:
-            ui.warn('(%s)\n' % inst.hint)
-    except error.RepoError as inst:
-        ui.warn(_("abort: %s!\n") % inst)
-        if inst.hint:
-            ui.warn(_("(%s)\n") % inst.hint)
-    except error.ResponseError as inst:
-        ui.warn(_("abort: %s") % inst.args[0])
-        if not isinstance(inst.args[1], basestring):
-            ui.warn(" %r\n" % (inst.args[1],))
-        elif not inst.args[1]:
-            ui.warn(_(" empty string\n"))
-        else:
-            ui.warn("\n%r\n" % util.ellipsis(inst.args[1]))
-    except error.CensoredNodeError as inst:
-        ui.warn(_("abort: file censored %s!\n") % inst)
-    except error.RevlogError as inst:
-        ui.warn(_("abort: %s!\n") % inst)
-    except error.SignalInterrupt:
-        ui.warn(_("killed!\n"))
+    except error.ParseError as inst:
+        _formatparse(ui.warn, inst)
+        return -1
     except error.UnknownCommand as inst:
         ui.warn(_("hg: unknown command '%s'\n") % inst.args[0])
@@ -293,59 +253,9 @@  def callcatch(ui, func):
             if not suggested:
                 commands.help_(ui, 'shortlist')
-    except error.InterventionRequired as inst:
-        ui.warn("%s\n" % inst)
-        if inst.hint:
-            ui.warn(_("(%s)\n") % inst.hint)
-        return 1
-    except error.Abort as inst:
-        ui.warn(_("abort: %s\n") % inst)
-        if inst.hint:
-            ui.warn(_("(%s)\n") % inst.hint)
-    except ImportError as inst:
-        ui.warn(_("abort: %s!\n") % inst)
-        m = str(inst).split()[-1]
-        if m in "mpatch bdiff".split():
-            ui.warn(_("(did you forget to compile extensions?)\n"))
-        elif m in "zlib".split():
-            ui.warn(_("(is your Python install correct?)\n"))
-    except IOError as inst:
-        if util.safehasattr(inst, "code"):
-            ui.warn(_("abort: %s\n") % inst)
-        elif util.safehasattr(inst, "reason"):
-            try: # usually it is in the form (errno, strerror)
-                reason = inst.reason.args[1]
-            except (AttributeError, IndexError):
-                # it might be anything, for example a string
-                reason = inst.reason
-            if isinstance(reason, unicode):
-                # SSLError of Python 2.7.9 contains a unicode
-                reason = reason.encode(encoding.encoding, 'replace')
-            ui.warn(_("abort: error: %s\n") % reason)
-        elif (util.safehasattr(inst, "args")
-              and inst.args and inst.args[0] == errno.EPIPE):
-            pass
-        elif getattr(inst, "strerror", None):
-            if getattr(inst, "filename", None):
-                ui.warn(_("abort: %s: %s\n") % (inst.strerror, inst.filename))
-            else:
-                ui.warn(_("abort: %s\n") % inst.strerror)
-        else:
-            raise
-    except OSError as inst:
-        if getattr(inst, "filename", None) is not None:
-            ui.warn(_("abort: %s: '%s'\n") % (inst.strerror, inst.filename))
-        else:
-            ui.warn(_("abort: %s\n") % inst.strerror)
+    except IOError:
+        raise
     except KeyboardInterrupt:
         raise
-    except MemoryError:
-        ui.warn(_("abort: out of memory\n"))
-    except SystemExit as inst:
-        # Commands shouldn't sys.exit directly, but give a return code.
-        # Just in case catch this and and pass exit code to caller.
-        return inst.code
-    except socket.error as inst:
-        ui.warn(_("abort: %s\n") % inst.args[-1])
-    except:  # perhaps re-raises
+    except:  # probably re-raises
         if not handlecommandexception(ui):
             raise
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -15,4 +15,5 @@  import os
 import re
 import shutil
+import socket
 import stat
 import tempfile
@@ -142,4 +143,106 @@  def nochangesfound(ui, repo, excluded=No
         ui.status(_("no changes found\n"))
 
+def callcatch(ui, func):
+    """call func() with global exception handling
+
+    return func() if no exception happens. otherwise do some error handling
+    and return an exit code accordingly. does not handle all exceptions.
+    """
+    try:
+        return func()
+    # Global exception handling, alphabetically
+    # Mercurial-specific first, followed by built-in and library exceptions
+    except error.LockHeld as inst:
+        if inst.errno == errno.ETIMEDOUT:
+            reason = _('timed out waiting for lock held by %s') % inst.locker
+        else:
+            reason = _('lock held by %s') % inst.locker
+        ui.warn(_("abort: %s: %s\n") % (inst.desc or inst.filename, reason))
+    except error.LockUnavailable as inst:
+        ui.warn(_("abort: could not lock %s: %s\n") %
+               (inst.desc or inst.filename, inst.strerror))
+    except error.OutOfBandError as inst:
+        if inst.args:
+            msg = _("abort: remote error:\n")
+        else:
+            msg = _("abort: remote error\n")
+        ui.warn(msg)
+        if inst.args:
+            ui.warn(''.join(inst.args))
+        if inst.hint:
+            ui.warn('(%s)\n' % inst.hint)
+    except error.RepoError as inst:
+        ui.warn(_("abort: %s!\n") % inst)
+        if inst.hint:
+            ui.warn(_("(%s)\n") % inst.hint)
+    except error.ResponseError as inst:
+        ui.warn(_("abort: %s") % inst.args[0])
+        if not isinstance(inst.args[1], basestring):
+            ui.warn(" %r\n" % (inst.args[1],))
+        elif not inst.args[1]:
+            ui.warn(_(" empty string\n"))
+        else:
+            ui.warn("\n%r\n" % util.ellipsis(inst.args[1]))
+    except error.CensoredNodeError as inst:
+        ui.warn(_("abort: file censored %s!\n") % inst)
+    except error.RevlogError as inst:
+        ui.warn(_("abort: %s!\n") % inst)
+    except error.SignalInterrupt:
+        ui.warn(_("killed!\n"))
+    except error.InterventionRequired as inst:
+        ui.warn("%s\n" % inst)
+        if inst.hint:
+            ui.warn(_("(%s)\n") % inst.hint)
+        return 1
+    except error.Abort as inst:
+        ui.warn(_("abort: %s\n") % inst)
+        if inst.hint:
+            ui.warn(_("(%s)\n") % inst.hint)
+    except ImportError as inst:
+        ui.warn(_("abort: %s!\n") % inst)
+        m = str(inst).split()[-1]
+        if m in "mpatch bdiff".split():
+            ui.warn(_("(did you forget to compile extensions?)\n"))
+        elif m in "zlib".split():
+            ui.warn(_("(is your Python install correct?)\n"))
+    except IOError as inst:
+        if util.safehasattr(inst, "code"):
+            ui.warn(_("abort: %s\n") % inst)
+        elif util.safehasattr(inst, "reason"):
+            try: # usually it is in the form (errno, strerror)
+                reason = inst.reason.args[1]
+            except (AttributeError, IndexError):
+                # it might be anything, for example a string
+                reason = inst.reason
+            if isinstance(reason, unicode):
+                # SSLError of Python 2.7.9 contains a unicode
+                reason = reason.encode(encoding.encoding, 'replace')
+            ui.warn(_("abort: error: %s\n") % reason)
+        elif (util.safehasattr(inst, "args")
+              and inst.args and inst.args[0] == errno.EPIPE):
+            pass
+        elif getattr(inst, "strerror", None):
+            if getattr(inst, "filename", None):
+                ui.warn(_("abort: %s: %s\n") % (inst.strerror, inst.filename))
+            else:
+                ui.warn(_("abort: %s\n") % inst.strerror)
+        else:
+            raise
+    except OSError as inst:
+        if getattr(inst, "filename", None) is not None:
+            ui.warn(_("abort: %s: '%s'\n") % (inst.strerror, inst.filename))
+        else:
+            ui.warn(_("abort: %s\n") % inst.strerror)
+    except MemoryError:
+        ui.warn(_("abort: out of memory\n"))
+    except SystemExit as inst:
+        # Commands shouldn't sys.exit directly, but give a return code.
+        # Just in case catch this and and pass exit code to caller.
+        return inst.code
+    except socket.error as inst:
+        ui.warn(_("abort: %s\n") % inst.args[-1])
+
+    return -1
+
 def checknewlabel(repo, lbl, kind):
     # Do not use the "kind" parameter in ui output.
diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t
+++ b/tests/test-devel-warnings.t
@@ -93,4 +93,5 @@ 
    */mercurial/dispatch.py:* in _runcatch (glob)
    */mercurial/dispatch.py:* in callcatch (glob)
+   */mercurial/scmutil.py* in callcatch (glob)
    */mercurial/dispatch.py:* in _runcatchfunc (glob)
    */mercurial/dispatch.py:* in _dispatch (glob)
@@ -128,4 +129,5 @@ 
    */mercurial/dispatch.py:* in _runcatch (glob)
    */mercurial/dispatch.py:* in callcatch (glob)
+   */mercurial/scmutil.py* in callcatch (glob)
    */mercurial/dispatch.py:* in _runcatchfunc (glob)
    */mercurial/dispatch.py:* in _dispatch (glob)
@@ -151,4 +153,5 @@ 
    */mercurial/dispatch.py:* in _runcatch (glob)
    */mercurial/dispatch.py:* in callcatch (glob)
+   */mercurial/scmutil.py* in callcatch (glob)
    */mercurial/dispatch.py:* in _runcatchfunc (glob)
    */mercurial/dispatch.py:* in _dispatch (glob)