Patchwork [V3] extdiff: add --per-file and --confirm options

login
register
mail settings
Submitter Ludovic Chabant
Date Jan. 19, 2019, 1:12 p.m.
Message ID <5686d56d5a2cebaa5043.1547903556@newslytherin.local>
Download mbox | patch
Permalink /patch/37886/
State Superseded
Headers show

Comments

Ludovic Chabant - Jan. 19, 2019, 1:12 p.m.
# HG changeset patch
# User Ludovic Chabant <ludovic@chabant.com>
# Date 1547180577 28800
#      Thu Jan 10 20:22:57 2019 -0800
# Node ID 5686d56d5a2cebaa504383b270e359156352090f
# Parent  ef0e2f7224358c32b0f62b13e83e89ba2399c8cf
extdiff: add --per-file and --confirm options

The new options 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.
Yuya Nishihara - Jan. 26, 2019, 7:53 a.m.
On Sat, 19 Jan 2019 05:12:36 -0800, Ludovic Chabant wrote:
> # HG changeset patch
> # User Ludovic Chabant <ludovic@chabant.com>
> # Date 1547180577 28800
> #      Thu Jan 10 20:22:57 2019 -0800
> # Node ID 5686d56d5a2cebaa504383b270e359156352090f
> # Parent  ef0e2f7224358c32b0f62b13e83e89ba2399c8cf
> extdiff: add --per-file and --confirm options

> +def _checktool(ui, tool):
> +    if not procutil.gui():
> +        needsgui = (ui.config('diff-tools', tool+'.gui') or
> +                    ui.config('merge-tools', tool+'.gui'))
> +        if needsgui:
> +            ui.warn(_("tool %s requires a GUI\n") % tool)
> +            return False
> +    return True

Can you split this to a separate patch? It'll need a bit more churn, and
we wouldn't want to make it a blocker of the --per-file/--confirm feature.

What I pointed out last time was that we shouldn't spawn non-GUI tools
asynchronously. You can see the problem by the following command on Unix.

  $ hg --config extdiff.vimdiff= vimdiff -c. --per-file

>      def __call__(self, ui, repo, *pats, **opts):
> +        if not _checktool(ui, self._cmd):
> +            return 2

Nit: just raise error.Abort().

And perhaps, it's too late to compare ui.config value based on self._cmd
here. If self._cmd is the key of [diff-tools] for example, we shouldn't look
for [merge-tools].
Ludovic Chabant - Jan. 30, 2019, 7:52 a.m.
> Can you split this to a separate patch? It'll need a bit more churn,
> and we wouldn't want to make it a blocker of the --per-file/--confirm
> feature.

I just sent PATCH V4 which should be just that feature alone, where, if
you don't specify --confirm, it launches the external tool one by one.

The second patch should be ready soon.

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,
@@ -175,6 +177,100 @@ 
         cmdline += ' $parent1 $child'
     return re.sub(regex, quote, cmdline)
 
+def _checktool(ui, tool):
+    if not procutil.gui():
+        needsgui = (ui.config('diff-tools', tool+'.gui') or
+                    ui.config('merge-tools', tool+'.gui'))
+        if needsgui:
+            ui.warn(_("tool %s requires a GUI\n") % tool)
+            return False
+    return True
+
+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 _runperfilediff(cmdline, repo_root, ui, do3way, confirm,
+                    commonfiles, tmproot, dir1a, dir1b,
+                    dir2root, dir2,
+                    rev1a, rev1b, rev2):
+    # 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 when "confirm" mode is enabled.
+    waitprocs = []
+    totalfiles = len(commonfiles)
+    for idx, commonfile in enumerate(sorted(commonfiles)):
+        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
+
+        if confirm:
+            # 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
+
+        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 confirm:
+            # 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()
+
 def dodiff(ui, repo, cmdline, pats, opts):
     '''Do the actual diff:
 
@@ -201,6 +297,9 @@ 
         else:
             ctx1b = repo[nullid]
 
+    perfile = opts.get('per_file')
+    confirm = opts.get('confirm')
+
     node1a = ctx1a.node()
     node1b = ctx1b.node()
     node2 = ctx2.node()
@@ -217,6 +316,8 @@ 
     if opts.get('patch'):
         if subrepos:
             raise error.Abort(_('--patch cannot be used with --subrepos'))
+        if perfile:
+            raise error.Abort(_('--patch cannot be used with --per-file'))
         if node2 is None:
             raise error.Abort(_('--patch requires two revisions'))
     else:
@@ -304,15 +405,23 @@ 
             label1b = None
             fnsandstat = []
 
-        # 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')
+        if not perfile:
+            # 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')
+        else:
+            # Run the external tool once for each pair of files
+            _runperfilediff(
+                cmdline, repo.root, ui, do3way=do3way, confirm=confirm,
+                commonfiles=common, tmproot=tmproot, dir1a=dir1a, dir1b=dir1b,
+                dir2root=dir2root, dir2=dir2,
+                rev1a=rev1a, rev1b=rev1b, rev2=rev2)
 
         for copy_fn, working_fn, st in fnsandstat:
             cpstat = os.lstat(copy_fn)
@@ -340,6 +449,10 @@ 
      _('pass option to comparison program'), _('OPT')),
     ('r', 'rev', [], _('revision'), _('REV')),
     ('c', 'change', '', _('change made by revision'), _('REV')),
+    ('', 'per-file', False,
+     _('compare each file instead of revision snapshots')),
+    ('', 'confirm', False,
+     _('prompt user before each external program invocation')),
     ('', 'patch', None, _('compare patches for two revisions'))
     ] + cmdutil.walkopts + cmdutil.subrepoopts
 
@@ -357,15 +470,23 @@ 
     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,
+    unless the --per-file option is specified (see below). To pass
+    additional options to the program, use -o/--option. These will be
+    passed before the names of the directories or files 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 --per-file option runs the external program repeatedly on each
+    file to diff, instead of once on two directories.
+
+    The --confirm option will prompt the user before each invocation of
+    the external program. It is ignored if --per-file isn't specified.
+    '''
     opts = pycompat.byteskwargs(opts)
     program = opts.get('program')
     option = opts.get('option')
@@ -390,15 +511,18 @@ 
     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):
+        if not _checktool(ui, self._cmd):
+            return 2
         opts = pycompat.byteskwargs(opts)
         options = ' '.join(map(procutil.shellquote, opts['option']))
         if options:
@@ -440,7 +564,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/tests/test-extdiff.t b/tests/test-extdiff.t
--- a/tests/test-extdiff.t
+++ b/tests/test-extdiff.t
@@ -48,6 +48,8 @@ 
    -o --option OPT [+]      pass option to comparison program
    -r --rev REV [+]         revision
    -c --change REV          change made by revision
+      --per-file            compare each file instead of revision snapshots
+      --confirm             prompt user before each external program invocation
       --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 +130,40 @@ 
   diffing a.398e36faf9c6 a.5ab95fb166c4
   [1]
 
+Test --per-file option:
+
+  $ hg up -q -C 3
+  $ echo a2 > a
+  $ echo b2 > b
+  $ hg ci -d '3 0' -mtestmode1
+  created new head
+  $ hg falabala -c 6 --per-file
+  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 --per-file and --confirm options:
+
+  $ hg --config ui.interactive=True falabala -c 6 --per-file --confirm <<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 --per-file and --confirm options with skipping:
+
+  $ hg --config ui.interactive=True falabala -c 6 --per-file --confirm <<EOF
+  > s
+  > EOF
+  diff a (1 of 2) [Yns?] s
+  [1]
+
 issue4463: usage of command line configuration without additional quoting
 
   $ cat <<EOF >> $HGRCPATH