Patchwork [2,of,2,V2] extdiff: add --mode option

login
register
mail settings
Submitter Ludovic Chabant
Date Jan. 11, 2019, 5:37 a.m.
Message ID <dfda7867497e5609a037.1547185041@newslytherin.local>
Download mbox | patch
Permalink /patch/37656/
State Accepted
Headers show

Comments

Ludovic Chabant - Jan. 11, 2019, 5:37 a.m.
# HG changeset patch
# User Ludovic Chabant <ludovic@chabant.com>
# Date 1547180577 28800
#      Thu Jan 10 20:22:57 2019 -0800
# Node ID dfda7867497e5609a03727508dc0da2cab3218a3
# Parent  ef0e2f7224358c32b0f62b13e83e89ba2399c8cf
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. 12, 2019, 6:47 a.m.
On Thu, 10 Jan 2019 21:37:21 -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 dfda7867497e5609a03727508dc0da2cab3218a3
> # Parent  ef0e2f7224358c32b0f62b13e83e89ba2399c8cf
> extdiff: add --mode option

> +            proc = _systemdetached(curcmdline, cwd=tmproot)
> +            waitprocs.append(proc)
> +
> +    if waitprocs:
> +        with ui.timeblockedsection('extdiff'):
> +            for proc in waitprocs:
> +                proc.wait()

Maybe need to check the .gui flag of the tool. Otherwise the console
would be messed up if the tool wasn't a GUI program.

> +    :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.

(from the V1 thread)

On Thu, 10 Jan 2019 16:03:00 -0800, Ludovic Chabant wrote:
> > 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.

Well, I think it will produce 8 modes,
dir-or-file x revpair-or-revrage x prompt-or-not.

And we can even get rid of the dir-or-file mode from command flags by
registering two tools, one for dir-diff and the other for per-file-diff.
I don't know if that is a better idea, but we'll never use the per-file
mode if the tool had a decent support for directory diff. So it's a tool-
specific property at some level.

> - The --confirm option doesn't do anything if you don't also pass --per-file...

A prompt could be shown for the directory diff. I don't think it's useful
right now, but if we had a per-revision-diff mode, it would make some sense
to show prompt to skip some revisions.

> 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).
Ludovic Chabant - Jan. 12, 2019, 5:26 p.m.
Thanks for the comments!

> Maybe need to check the .gui flag of the tool. Otherwise the console
> would be messed up if the tool wasn't a GUI program.

Good point, although the existing extdiff code doesn't seem to be checking that, no? So should I do that in a separate patch?
 

> Well, I think it will produce 8 modes,
> dir-or-file x revpair-or-revrage x prompt-or-not.
> 
> And we can even get rid of the dir-or-file mode from command flags by
> registering two tools, one for dir-diff and the other for per-file-diff.
> I don't know if that is a better idea, but we'll never use the per-file
> mode if the tool had a decent support for directory diff. So it's a tool-
> specific property at some level.

It's not super important, but I disagree about the second-to-last-point here, since I actually wrote this patch with the intent of using per-file diffs with tools like BeyondCompare and vim+DirDiff, which both handle dir diffs pretty well. It's just that 95% of the time I want to see the diff of _all_ the files in a revision, and going through a dir-diff UI just adds unnecessary actions/clicks to get all those diffs to open.
The only times I want a dir-diff where I'm going to only look at _some_ of the files is when diffing a big merge.

As a result, I was indeed planning on configuring per-file-diffing by default in my .hgrc for those tools, while occasionally passing a dir-diff argument when appropriate. Sure, I could make 2 different tools in my .hgrc if we didn't want to expose new options to the CLI, but for some reason it feels wrong to force users to edit their .hgrc for that.

I see your point about modes vs flags. Like I said, I don't have very strong opinions about it so I guess I'll change the second patch and use some flags instead. Are we going for --per-file and --confirm then?

Note that the tool config in .hgrc would look a bit less elegant as a result, like this:

cmd.blah=/path/to/blah
opts.blah=--some-option -z
per-file.blah=true
confirm.blah=true

So instead I'm proposing a new config that specifies options for the hg extdiff command itself, like this:

cmd.blah=/path/to/blah
opts.blah=--some-option -z
hgopts.blah=--per-file --confirm

...but I don't know if that opens up potential abuse or something.  What do you think?

> A prompt could be shown for the directory diff. I don't think it's useful
> right now, but if we had a per-revision-diff mode, it would make some sense
> to show prompt to skip some revisions.

I would personally just ignore the prompt when showing only one revision in dir-diff mode, but yes, for a revision range dir-diff, it would definitely show a prompt between each revision.
Yuya Nishihara - Jan. 13, 2019, 2:27 a.m.
On Sat, 12 Jan 2019 09:26:25 -0800, Ludovic Chabant wrote:
> Thanks for the comments!
> 
> > Maybe need to check the .gui flag of the tool. Otherwise the console
> > would be messed up if the tool wasn't a GUI program.
> 
> Good point, although the existing extdiff code doesn't seem to be checking that, no? So should I do that in a separate patch?

A separate patch seems fine. To make clear, there's no need to check if the
tool is console-based or not unless multiple diff processes are spawned
simultaneously. Currently the .gui flag is tested only by filemerge.py.

> > Well, I think it will produce 8 modes,
> > dir-or-file x revpair-or-revrage x prompt-or-not.
> > 
> > And we can even get rid of the dir-or-file mode from command flags by
> > registering two tools, one for dir-diff and the other for per-file-diff.
> > I don't know if that is a better idea, but we'll never use the per-file
> > mode if the tool had a decent support for directory diff. So it's a tool-
> > specific property at some level.
> 
> It's not super important, but I disagree about the second-to-last-point here, since I actually wrote this patch with the intent of using per-file diffs with tools like BeyondCompare and vim+DirDiff, which both handle dir diffs pretty well. It's just that 95% of the time I want to see the diff of _all_ the files in a revision, and going through a dir-diff UI just adds unnecessary actions/clicks to get all those diffs to open.
> The only times I want a dir-diff where I'm going to only look at _some_ of the files is when diffing a big merge.
> 
> As a result, I was indeed planning on configuring per-file-diffing by default in my .hgrc for those tools, while occasionally passing a dir-diff argument when appropriate. Sure, I could make 2 different tools in my .hgrc if we didn't want to expose new options to the CLI, but for some reason it feels wrong to force users to edit their .hgrc for that.
> 
> I see your point about modes vs flags. Like I said, I don't have very strong opinions about it so I guess I'll change the second patch and use some flags instead. Are we going for --per-file and --confirm then?

To all, any thoughts?

I feel uncomfortable these days as there's little response to this kind of
UI patches. I'm not a good UI designer.

> Note that the tool config in .hgrc would look a bit less elegant as a result, like this:
> 
> cmd.blah=/path/to/blah
> opts.blah=--some-option -z
> per-file.blah=true
> confirm.blah=true
> 
> So instead I'm proposing a new config that specifies options for the hg extdiff command itself, like this:
> 
> cmd.blah=/path/to/blah
> opts.blah=--some-option -z
> hgopts.blah=--per-file --confirm
> 
> ...but I don't know if that opens up potential abuse or something.  What do you think?

The latter looks worse for me. Instead, you can use [alias] to pass in
arguments to hg commands.
Ludovic Chabant - Jan. 13, 2019, 11:31 p.m.
> A separate patch seems fine. To make clear, there's no need to check if the
> tool is console-based or not unless multiple diff processes are spawned
> simultaneously. Currently the .gui flag is tested only by filemerge.py.

So you mean I don't need to modify the previous code path (dir-diff), nor do I need to check the gui flag if the "confirm" flag is given (i.e. one external process at a time)?
How come? Wouldn't even one gui process be a problem if you're in a shell-only instance of hg?


> The latter looks worse for me. Instead, you can use [alias] to pass in
> arguments to hg commands.

That's true, but then the entire extdiff configuration section would be deprecated if that was the case, no?
Like, AFAIK there's not much difference between:

[extdiff]
cmd.bcomp = /path/to/bcomp
opts.bcomp = --whatever

and:

[alias]
bcomp = extdiff -p /path/to/bcomp -o "--whatever"

I assume the point of extdiff is to be a slighly better version of an alias for the purpose of a diffing stuff, but maybe someone with a better knowledge of the history of both features can correct me.

More importantly, I actually just realized (maybe) why a --mode option might be better. Remember how I intended to have, say, BeyondCompare setup to do per-file-diffs by default, and I would pass a dir-diff option for the 5% cases where I want to only diff a few files in a revision?
Well, if we have multiple flags, the extdiff/alias/whatever section of my .hgrc would specify --per-file (so I get per-file diffs by default), but if I then pass --dir (to revert back to a dir-diff), it wouldn't override the default... the extdiff command would instead get both --per-file and --dir, and would most likely throw an error because it doesn't make sense to pass those options togethers.
However, with a --mode option, the one I pass on the command line would correctly override the one specified in the .hgrc, and everything works as you would expect... does that make sense?
Yuya Nishihara - Jan. 14, 2019, 3:54 a.m.
On Sun, 13 Jan 2019 15:31:07 -0800, Ludovic Chabant wrote:
> > A separate patch seems fine. To make clear, there's no need to check if the
> > tool is console-based or not unless multiple diff processes are spawned
> > simultaneously. Currently the .gui flag is tested only by filemerge.py.
> 
> So you mean I don't need to modify the previous code path (dir-diff), nor do I need to check the gui flag if the "confirm" flag is given (i.e. one external process at a time)?
> How come? Wouldn't even one gui process be a problem if you're in a shell-only instance of hg?

I was just speaking about the current implementation (without your patch.)
Maybe I should say .gui flag isn't checked because the current extdiff never
spawns more than one processes at a time.

> > The latter looks worse for me. Instead, you can use [alias] to pass in
> > arguments to hg commands.
> 
> That's true, but then the entire extdiff configuration section would be deprecated if that was the case, no?
> Like, AFAIK there's not much difference between:
> 
> [extdiff]
> cmd.bcomp = /path/to/bcomp
> opts.bcomp = --whatever
> 
> and:
> 
> [alias]
> bcomp = extdiff -p /path/to/bcomp -o "--whatever"
> 
> I assume the point of extdiff is to be a slighly better version of an alias for the purpose of a diffing stuff, but maybe someone with a better knowledge of the history of both features can correct me.

AFAIK, it duplicates alias functionality because there wasn't no [alias]
when the extdiff extension was introduced. And I think that's the major
reason why the extdiff is still an extension. It can't be trivially ported
to a core command.

If the extdiff had the option to look for a diff tool from stock templates
(i.e. [diff-tools], [merge-tools], and maybe [extdiff]), alias could be
expressed as follows:

  [alias]
  bcomp = extdiff --tool bcomp --per-file

This should be good enough.

> More importantly, I actually just realized (maybe) why a --mode option might be better. Remember how I intended to have, say, BeyondCompare setup to do per-file-diffs by default, and I would pass a dir-diff option for the 5% cases where I want to only diff a few files in a revision?
> Well, if we have multiple flags, the extdiff/alias/whatever section of my .hgrc would specify --per-file (so I get per-file diffs by default), but if I then pass --dir (to revert back to a dir-diff), it wouldn't override the default... the extdiff command would instead get both --per-file and --dir, and would most likely throw an error because it doesn't make sense to pass those options togethers.

You can turn off the --per-file by --no-per-file. A separate --dir option
isn't needed as long as --no-per-file == --dir. See hg help flags.
Ludovic Chabant - Jan. 14, 2019, 6:05 a.m.
> I was just speaking about the current implementation (without your patch.)
> Maybe I should say .gui flag isn't checked because the current extdiff never
> spawns more than one processes at a time.

Yeah, and I meant that I don't understand what's the difference between one vs. multiple processes?


> AFAIK, it duplicates alias functionality because there wasn't no [alias]
> when the extdiff extension was introduced. And I think that's the major
> reason why the extdiff is still an extension. It can't be trivially ported
> to a core command.
> 
> If the extdiff had the option to look for a diff tool from stock templates
> (i.e. [diff-tools], [merge-tools], and maybe [extdiff]), alias could be
> expressed as follows:
> 
>   [alias]
>   bcomp = extdiff --tool bcomp --per-file
> 
> This should be good enough.

Yeah makes sense, I figured extdiff predated alias. And I would indeed like if we could clean up the way extdiff integrates with the rest of mercurial... it's not far off since, right now, "hg extdiff -p blah" will just run "blah" instead of first checking if there's a "merge-tools.blah" config... but I guess implementing that would qualify as a breaking change and wouldn't be acceptable?


> You can turn off the --per-file by --no-per-file. A separate --dir option
> isn't needed as long as --no-per-file == --dir. See hg help flags.

Ah right I forgot about the "no" flags... it still feels a bit awkward to use IMHO (since you have to remember the implementation of the alias or extdiff in order to remember to cancel a specific flag) but I can live with that.
Yuya Nishihara - Jan. 15, 2019, 1:17 p.m.
On Sun, 13 Jan 2019 22:05:53 -0800, Ludovic Chabant wrote:
> > I was just speaking about the current implementation (without your patch.)
> > Maybe I should say .gui flag isn't checked because the current extdiff never
> > spawns more than one processes at a time.
> 
> Yeah, and I meant that I don't understand what's the difference between one vs. multiple processes?

vimdiff wouldn't be usable unless the process is waited one by one, for example.
It's okay to spawn multiple GUI diff instances and wait them, but console
programs generally don't work in that way.

> > If the extdiff had the option to look for a diff tool from stock templates
> > (i.e. [diff-tools], [merge-tools], and maybe [extdiff]), alias could be
> > expressed as follows:
> > 
> >   [alias]
> >   bcomp = extdiff --tool bcomp --per-file
> > 
> > This should be good enough.
> 
> Yeah makes sense, I figured extdiff predated alias. And I would indeed like if we could clean up the way extdiff integrates with the rest of mercurial... it's not far off since, right now, "hg extdiff -p blah" will just run "blah" instead of first checking if there's a "merge-tools.blah" config... but I guess implementing that would qualify as a breaking change and wouldn't be acceptable?

Probably. That's why I used -t/--tool in example.

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,
@@ -175,6 +182,91 @@ 
         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 _runperfilediff(cmdline, repo_root, ui, do3way, diffmode,
+                    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 for the "prompt" mode.
+    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 '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
+
+        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()
+
 def dodiff(ui, repo, cmdline, pats, opts):
     '''Do the actual diff:
 
@@ -201,6 +293,8 @@ 
         else:
             ctx1b = repo[nullid]
 
+    diffmode = opts.get('mode')
+
     node1a = ctx1a.node()
     node1b = ctx1b.node()
     node2 = ctx2.node()
@@ -217,6 +311,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:
@@ -304,15 +400,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 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
+            _runperfilediff(
+                cmdline, repo.root, ui, do3way=do3way, diffmode=diffmode,
+                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 +444,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
 
@@ -357,15 +462,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')
@@ -390,19 +510,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):
@@ -418,7 +547,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:
@@ -440,7 +569,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,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