Patchwork extdiff: add --mode option

login
register
mail settings
Submitter Ludovic Chabant
Date Jan. 3, 2019, 9:03 p.m.
Message ID <cc964d5d3e3faca1bbbc.1546549429@BUR1-D1044227.eac.ad.ea.com>
Download mbox | patch
Permalink /patch/37443/
State New
Headers show

Comments

Ludovic Chabant - Jan. 3, 2019, 9:03 p.m.
# HG changeset patch
# User Ludovic Chabant <ludovic@chabant.com>
# Date 1546539395 28800
#      Thu Jan 03 10:16:35 2019 -0800
# Node ID cc964d5d3e3faca1bbbc680732c38cf785754fc2
# Parent  5a2a6ab7bc37af9294b58edfd4d224706940ff19
extdiff: add --mode option

The new option lets the user control how the external program is run.
By default, Mercurial passes the 2 snapshot directories as usual, but
it can also run the program repeatedly on each file's snapshot pair,
and optionally prompt the user each time.

The option can be passed via the CLI or be set in the 'extdiff' tool
config.
Yuya Nishihara - Jan. 10, 2019, 12:17 p.m.
On Thu, 03 Jan 2019 13:03:49 -0800, Ludovic Chabant wrote:
> # HG changeset patch
> # User Ludovic Chabant <ludovic@chabant.com>
> # Date 1546539395 28800
> #      Thu Jan 03 10:16:35 2019 -0800
> # Node ID cc964d5d3e3faca1bbbc680732c38cf785754fc2
> # Parent  5a2a6ab7bc37af9294b58edfd4d224706940ff19
> extdiff: add --mode option

Sorry for late review. Almost all devs were inactive while holidays.

First, can you break this into a series of a few patches?

https://www.mercurial-scm.org/wiki/ContributingChanges#Organizing_patches

I expect the first patch will probably reorder or extract the current behavior
to function such that new modes can be added.

> +    The -m/--mode option controls how the external program is executed:
> +
> +    :d (or nothing):    Runs the external program once and passes the
> +                        names of two directories to compare. Each
> +                        directory contains snapshots of the files before
> +                        and after the given revisions. This is the
> +                        default mode.
> +    :f:                 Runs the external program once for each file,
> +                        passing the path to the snapshots.
> +    :p:                 Modifies the 'f' mode by prompting the user
> +                        before running the external program for each
> +                        file.

I don't think "--mode <char>" is good ui. I don't have nice idea, but maybe
it can be split into two flags (e.g. --per-file and --confirm/--interactive)?

And the config option can specify if the tool support directory diff, for
example. If the tool doesn't support directory diff, and if it's gui, spawn
processes per file by default.

> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -45,6 +45,9 @@
>      'inotify',
>      'hgcia'
>  }
> +_aliases = {
> +    'email': 'patchbomb'
> +}

Unrelated changes?
Ludovic Chabant - Jan. 11, 2019, 12:03 a.m.
> Sorry for late review. Almost all devs were inactive while holidays.

No worries! Between the holidays and the 4.8.2 release, I figured unsolicited new features were a low priority.


> First, can you break this into a series of a few patches?
> 
> https://www.mercurial-scm.org/wiki/ContributingChanges#Organizing_patches
> 
> I expect the first patch will probably reorder or extract the current behavior
> to function such that new modes can be added.

Sure! I wasn't sure exactly what qualifies as multi vs single patch changes.


> I don't think "--mode <char>" is good ui. I don't have nice idea, but maybe
> it can be split into two flags (e.g. --per-file and --confirm/--interactive)?
> 
> And the config option can specify if the tool support directory diff, for
> example. If the tool doesn't support directory diff, and if it's gui, spawn
> processes per file by default.

I was actually debating with myself between the 2 ways of specifying the diff mode, and eventually I settled on the --mode option. Here's the rationale (although I can be easily convinced to use the alternative):

- A --mode option is easier to later extend to more ways to diff things, like for example a mode where, when you pass a revision range, it will diff each revision one by one (invoking the external program N times if there are N revisions in the range). Different flags that may or may not be compatible between each other are more confusing IMHO.

- The --confirm option doesn't do anything if you don't also pass --per-file... I don't know if there are other aspects of the hg cli that have this? Now I realize that I also fell short of that downside since "--mode p" doesn't do anything either, but the point is that if we want something to represent "per file" and "per file with prompt", 2 modes do that better than 2 flags (and I could change my patch to have "p" be a mode of its own instead of modifying the "f" mode).

But like I was saying, I'm open to changing it. I figured this would probably be the main debate with this patch.


> Unrelated changes?

Woops, yes. Sorry, I didn't correctly check my cmdline arguments when sending the patch.
Ludovic Chabant - Jan. 11, 2019, 5:45 p.m.
> First, can you break this into a series of a few patches?

As you might have seen, I sent the change split in 2 patches to the mailing list. I didn't change anything else yet, so we can keep discussing the CLI on the new patch. Thanks!

Patch

diff --git a/hgext/extdiff.py b/hgext/extdiff.py
--- a/hgext/extdiff.py
+++ b/hgext/extdiff.py
@@ -71,6 +71,7 @@ 
 import re
 import shutil
 import stat
+import subprocess
 
 from mercurial.i18n import _
 from mercurial.node import (
@@ -80,6 +81,7 @@ 
 from mercurial import (
     archival,
     cmdutil,
+    encoding,
     error,
     filemerge,
     formatter,
@@ -104,6 +106,11 @@ 
     generic=True,
 )
 
+configitem('extdiff', br'mode\..*',
+    default='',
+    generic=True,
+)
+
 configitem('diff-tools', br'.*\.diffargs$',
     default=None,
     generic=True,
@@ -152,6 +159,41 @@ 
                 fnsandstat.append((dest, repo.wjoin(fn), os.lstat(dest)))
     return dirname, fnsandstat
 
+def formatcmdline(cmdline, repo_root, do3way,
+                  parent1, plabel1, parent2, plabel2, child, clabel):
+    # Function to quote file/dir names in the argument string.
+    # When not operating in 3-way mode, an empty string is
+    # returned for parent2
+    replace = {'parent': parent1, 'parent1': parent1, 'parent2': parent2,
+               'plabel1': plabel1, 'plabel2': plabel2,
+               'child': child, 'clabel': clabel,
+               'root': repo_root}
+    def quote(match):
+        pre = match.group(2)
+        key = match.group(3)
+        if not do3way and key == 'parent2':
+            return pre
+        return pre + procutil.shellquote(replace[key])
+
+    # Match parent2 first, so 'parent1?' will match both parent1 and parent
+    regex = (br'''(['"]?)([^\s'"$]*)'''
+             br'\$(parent2|parent1?|child|plabel1|plabel2|clabel|root)\1')
+    if not do3way and not re.search(regex, cmdline):
+        cmdline += ' $parent1 $child'
+    return re.sub(regex, quote, cmdline)
+
+def _systemdetached(cmd, environ=None, cwd=None):
+    ''' like 'procutil.system', but returns the Popen object directly
+        so we don't have to wait on it.
+    '''
+    cmd = procutil.quotecommand(cmd)
+    env = procutil.shellenviron(environ)
+    proc = subprocess.Popen(procutil.tonativestr(cmd),
+                            shell=True, close_fds=procutil.closefds,
+                            env=procutil.tonativeenv(env),
+                            cwd=pycompat.rapply(procutil.tonativestr, cwd))
+    return proc
+
 def dodiff(ui, repo, cmdline, pats, opts):
     '''Do the actual diff:
 
@@ -178,6 +220,8 @@ 
         else:
             ctx1b = repo[nullid]
 
+    diffmode = opts.get('mode')
+
     node1a = ctx1a.node()
     node1b = ctx1b.node()
     node2 = ctx2.node()
@@ -194,6 +238,8 @@ 
     if opts.get('patch'):
         if subrepos:
             raise error.Abort(_('--patch cannot be used with --subrepos'))
+        if diffmode:
+            raise error.Abort(_('--patch cannot be used with --mode'))
         if node2 is None:
             raise error.Abort(_('--patch requires two revisions'))
     else:
@@ -281,29 +327,86 @@ 
             label1b = None
             fnsandstat = []
 
-        # Function to quote file/dir names in the argument string.
-        # When not operating in 3-way mode, an empty string is
-        # returned for parent2
-        replace = {'parent': dir1a, 'parent1': dir1a, 'parent2': dir1b,
-                   'plabel1': label1a, 'plabel2': label1b,
-                   'clabel': label2, 'child': dir2,
-                   'root': repo.root}
-        def quote(match):
-            pre = match.group(2)
-            key = match.group(3)
-            if not do3way and key == 'parent2':
-                return pre
-            return pre + procutil.shellquote(replace[key])
+        if not diffmode or 'd' in diffmode:
+            # Run the external tool on the 2 temp directories or the patches
+            cmdline = formatcmdline(
+                cmdline, repo.root, do3way=do3way,
+                parent1=dir1a, plabel1=label1a,
+                parent2=dir1b, plabel2=label1b,
+                child=dir2, clabel=label2)
+            ui.debug('running %r in %s\n' % (pycompat.bytestr(cmdline),
+                                             tmproot))
+            ui.system(cmdline, cwd=tmproot, blockedtag='extdiff')
+        elif 'f' in diffmode:
+            # Run the external tool once for each pair of files
+            # Note that we need to sort the list of files because it was
+            # built in an "unstable" way and it's annoying to get files in a
+            # random order, especially for the "prompt" mode.
+            waitprocs = []
+            totalfiles = len(common)
+            for idx, commonfile in enumerate(sorted(common)):
+                path1a = os.path.join(tmproot, dir1a, commonfile)
+                label1a = commonfile + rev1a
+                if not os.path.isfile(path1a):
+                    path1a = os.devnull
+
+                path1b = ''
+                label1b = ''
+                if do3way:
+                    path1b = os.path.join(tmproot, dir1b, commonfile)
+                    label1b = commonfile + rev1b
+                    if not os.path.isfile(path1b):
+                        path1b = os.devnull
+
+                path2 = os.path.join(dir2root, dir2, commonfile)
+                label2 = commonfile + rev2
 
-        # Match parent2 first, so 'parent1?' will match both parent1 and parent
-        regex = (br'''(['"]?)([^\s'"$]*)'''
-                 br'\$(parent2|parent1?|child|plabel1|plabel2|clabel|root)\1')
-        if not do3way and not re.search(regex, cmdline):
-            cmdline += ' $parent1 $child'
-        cmdline = re.sub(regex, quote, cmdline)
+                if 'p' in diffmode:
+                    # Prompt before showing this diff
+                    difffiles = _('diff %s (%d of %d)') % (commonfile, idx + 1,
+                                                           totalfiles)
+                    responses = _('[Yns?]'
+                                  '$$ &Yes, show diff'
+                                  '$$ &No, skip this diff'
+                                  '$$ &Skip remaining diffs'
+                                  '$$ &? (display help)')
+                    r = ui.promptchoice('%s %s' % (difffiles, responses))
+                    if r == 3: # ?
+                        while r == 3:
+                            for c, t in ui.extractchoices(responses)[1]:
+                                ui.write('%s - %s\n' % (c, encoding.lower(t)))
+                            r = ui.promptchoice('%s %s' %
+                                                (difffiles, responses))
+                    if r == 0: # yes
+                        pass
+                    elif r == 1: # no
+                        continue
+                    elif r == 2: # skip
+                        break
 
-        ui.debug('running %r in %s\n' % (pycompat.bytestr(cmdline), tmproot))
-        ui.system(cmdline, cwd=tmproot, blockedtag='extdiff')
+                curcmdline = formatcmdline(
+                    cmdline, repo.root, do3way=do3way,
+                    parent1=path1a, plabel1=label1a,
+                    parent2=path1b, plabel2=label1b,
+                    child=path2, clabel=label2)
+                ui.debug('running %r in %s\n' % (pycompat.bytestr(curcmdline),
+                                                 tmproot))
+
+                if 'p' in diffmode:
+                    # Run the comparison program and wait for it to exit
+                    # before we show the next file.
+                    ui.system(curcmdline, cwd=tmproot, blockedtag='extdiff')
+                else:
+                    # Run the comparison program but don't wait, as we're
+                    # going to rapid-fire each file diff and then wait on
+                    # the whole group.
+                    proc = _systemdetached(curcmdline, cwd=tmproot)
+                    waitprocs.append(proc)
+
+            if waitprocs:
+                with ui.timeblockedsection('extdiff'):
+                    for proc in waitprocs:
+                        proc.wait()
 
         for copy_fn, working_fn, st in fnsandstat:
             cpstat = os.lstat(copy_fn)
@@ -331,6 +434,7 @@ 
      _('pass option to comparison program'), _('OPT')),
     ('r', 'rev', [], _('revision'), _('REV')),
     ('c', 'change', '', _('change made by revision'), _('REV')),
+    ('m', 'mode', '', _('specifies how to invoke the comparison program')),
     ('', 'patch', None, _('compare patches for two revisions'))
     ] + cmdutil.walkopts + cmdutil.subrepoopts
 
@@ -348,15 +452,30 @@ 
     default options "-Npru".
 
     To select a different program, use the -p/--program option. The
-    program will be passed the names of two directories to compare. To
-    pass additional options to the program, use -o/--option. These
-    will be passed before the names of the directories to compare.
+    program will be passed the names of two directories to compare (but
+    see the '--mode' option's documentation below for other execution
+    modes). To pass additional options to the program, use -o/--option.
+    These will be passed before the names of the directories to compare.
 
     When two revision arguments are given, then changes are shown
     between those revisions. If only one revision is specified then
     that revision is compared to the working directory, and, when no
     revisions are specified, the working directory files are compared
-    to its parent.'''
+    to its parent.
+
+    The -m/--mode option controls how the external program is executed:
+
+    :d (or nothing):    Runs the external program once and passes the
+                        names of two directories to compare. Each
+                        directory contains snapshots of the files before
+                        and after the given revisions. This is the
+                        default mode.
+    :f:                 Runs the external program once for each file,
+                        passing the path to the snapshots.
+    :p:                 Modifies the 'f' mode by prompting the user
+                        before running the external program for each
+                        file.
+    '''
     opts = pycompat.byteskwargs(opts)
     program = opts.get('program')
     option = opts.get('option')
@@ -381,19 +500,28 @@ 
     to its parent.
     """
 
-    def __init__(self, path, cmdline):
+    def __init__(self, cmd, path, cmdline):
         # We can't pass non-ASCII through docstrings (and path is
         # in an unknown encoding anyway), but avoid double separators on
         # Windows
         docpath = stringutil.escapestr(path).replace(b'\\\\', b'\\')
         self.__doc__ %= {r'path': pycompat.sysstr(stringutil.uirepr(docpath))}
         self._cmdline = cmdline
+        self._cmd = cmd
 
     def __call__(self, ui, repo, *pats, **opts):
         opts = pycompat.byteskwargs(opts)
+        if not opts['mode']:
+            # The '--mode' option wasn't passed on the command-line so
+            # check if we have a default value in the config.
+            cfgmode = ui.config('extdiff', 'mode.' + self._cmd)
+            if cfgmode:
+                opts['mode'] = cfgmode
+
         options = ' '.join(map(procutil.shellquote, opts['option']))
         if options:
             options = ' ' + options
+
         return dodiff(ui, repo, self._cmdline + options, pats, opts)
 
 def uisetup(ui):
@@ -409,7 +537,7 @@ 
             cmdline = procutil.shellquote(path)
             if diffopts:
                 cmdline += ' ' + diffopts
-        elif cmd.startswith('opts.'):
+        elif cmd.startswith('opts.') or cmd.startswith('mode.'):
             continue
         else:
             if path:
@@ -431,7 +559,7 @@ 
                 cmdline += ' ' + args
         command(cmd, extdiffopts[:], _('hg %s [OPTION]... [FILE]...') % cmd,
                 helpcategory=command.CATEGORY_FILE_CONTENTS,
-                inferrepo=True)(savedcmd(path, cmdline))
+                inferrepo=True)(savedcmd(cmd, path, cmdline))
 
 # tell hggettext to extract docstrings from these functions:
 i18nfunctions = [savedcmd]
diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -45,6 +45,9 @@ 
     'inotify',
     'hgcia'
 }
+_aliases = {
+    'email': 'patchbomb'
+}
 
 def extensions(ui=None):
     if ui:
@@ -166,9 +169,11 @@ 
     _validatecmdtable(ui, getattr(mod, 'cmdtable', {}))
 
 def load(ui, name, path, loadingtime=None):
+    unaliased_name = name
     if name.startswith('hgext.') or name.startswith('hgext/'):
         shortname = name[6:]
     else:
+        name = _aliases.get(name,  name)
         shortname = name
     if shortname in _builtin:
         return None
@@ -196,6 +201,8 @@ 
     _validatetables(ui, mod)
 
     _extensions[shortname] = mod
+    if unaliased_name != name:
+        _extensions[unaliased_name] = mod
     _order.append(shortname)
     ui.log(b'extension', b'    - invoking registered callbacks: %s\n',
            shortname)
diff --git a/tests/test-extdiff.t b/tests/test-extdiff.t
--- a/tests/test-extdiff.t
+++ b/tests/test-extdiff.t
@@ -48,6 +48,7 @@ 
    -o --option OPT [+]      pass option to comparison program
    -r --rev REV [+]         revision
    -c --change REV          change made by revision
+   -m --mode VALUE          specifies how to invoke the comparison program
       --patch               compare patches for two revisions
    -I --include PATTERN [+] include names matching the given patterns
    -X --exclude PATTERN [+] exclude names matching the given patterns
@@ -128,6 +129,41 @@ 
   diffing a.398e36faf9c6 a.5ab95fb166c4
   [1]
 
+Test --mode option (files):
+
+  $ hg up -q -C 3
+  $ echo a2 > a
+  $ echo b2 > b
+  $ hg ci -d '3 0' -mtestmode1
+  created new head
+  $ hg falabala -c 6 -m f
+  diffing "*\\extdiff.*\\a.46c0e4daeb72\\a" "a.81906f2b98ac\\a" (glob) (windows !)
+  diffing */extdiff.*/a.46c0e4daeb72/a a.81906f2b98ac/a (glob) (no-windows !)
+  diffing "*\\extdiff.*\\a.46c0e4daeb72\\b" "a.81906f2b98ac\\b" (glob) (windows !)
+  diffing */extdiff.*/a.46c0e4daeb72/b a.81906f2b98ac/b (glob) (no-windows !)
+  [1]
+
+Test --mode option (files, prompt yes/no):
+
+  $ hg --config ui.interactive=True falabala -c 6 -m fp <<EOF
+  > n
+  > y
+  > EOF
+  diff a (1 of 2) [Yns?] n
+  diff b (2 of 2) [Yns?] y
+  diffing "*\\extdiff.*\\a.46c0e4daeb72\\b" "a.81906f2b98ac\\b" (glob) (windows !)
+  diffing */extdiff.*/a.46c0e4daeb72/b a.81906f2b98ac/b (glob) (no-windows !)
+  [1]
+
+Test --mode option in config (files, prompt skip):
+
+  $ hg --config ui.interactive=True --config extdiff.mode.falabala=fp falabala -c 6 <<EOF
+  > s
+  > EOF
+  diff a (1 of 2) [Yns?] s
+  [1]
+
+
 issue4463: usage of command line configuration without additional quoting
 
   $ cat <<EOF >> $HGRCPATH