This is an archive of the discontinued Mercurial Phabricator instance.

Differential D803

extdiff: allow pager to be used
AbandonedPublic

Authored by quark on Sep 22 2017, 7:13 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Summary

Now the pager extension is deprecated, there is no easy way to enable pager
for extdiff. This patch makes it possible to enable pager for individual
extdiff commands by setting pager.attend-x. We don't enable pager by
default because it could have unwanted side-effects if the external pager is
a GUI or ncurses program.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Sep 22 2017, 7:13 PM
indygreg added a subscriber: indygreg.Sep 24 2017, 10:45 AM

Before I accept this, I think we should have a brief conversation on the possibility that an external diff tool invokes its own pager.

Do we need a per-diff tool config option to control whether hg invokes a pager? I think hg using a pager by default makes sense. But I also suspect there will be an external diff tool that doesn't like this behavior.

yuja requested changes to this revision.Sep 25 2017, 8:47 AM
yuja added a subscriber: yuja.

Pager should be disabled at least for GUI tools. Otherwise hg gui-diff & would
be suspended by signal. Perhaps curses-based tools would have a similar problem.

This revision now requires changes to proceed.Sep 25 2017, 8:47 AM
quark edited the summary of this revision. (Show Details)Sep 25 2017, 6:34 PM
quark retitled this revision from extdiff: use pager to extdiff: allow pager to be used.
quark updated this revision to Diff 2070.
quark updated this revision to Diff 2071.Sep 25 2017, 6:37 PM
yuja requested changes to this revision.Sep 29 2017, 2:42 AM

It seems confusing to support the global pager.attend-* option only in extdiff context.
Instead, maybe we can use the .gui flag of merge-tools (plus another boolean for
vimdiff-like tools.)

Alternatively, we could resurrect the global pager.attend-* support, but perhaps there
would be a reason not to?

This revision now requires changes to proceed.Sep 29 2017, 2:42 AM
quark added a comment.EditedSep 29 2017, 5:46 PM
In D803#14125, @yuja wrote:

It seems confusing to support the global pager.attend-* option only in extdiff context.
Instead, maybe we can use the .gui flag of merge-tools (plus another boolean for
vimdiff-like tools.)

The option is still there.

The difference between core commands and extdiff commands here is core commands have
pager.attend-* set to True by default. But extdiff commands have attend-* set to
False to not cause surprises.

To be consistent, we can enable pager by default and allow it to be disabled via
attend-* configs, just like core hg commands. Since we have already BC on "log"
(default changed from not using pager to use pager), I think it's okay to BC here.
That looks cleaner than adding more config flags?

Alternatively, we could resurrect the global pager.attend-* support, but perhaps there
would be a reason not to?

One reason for not doing command-level pager is a "command" is conceptually bigger than
"page content". ex. users may want pager for shelve -l / bookmark, but not shelve /
bookmark foo.

yuja added a comment.Sep 29 2017, 6:29 PM

The difference between core commands and extdiff commands here is core commands have
pager.attend-* set to True by default. But extdiff commands have attend-* set to
False to not cause surprises.

That's only true for page-able commands. You can't get pager for e.g. hg version by
setting pager.attend-version=True unless you have the pager extension enabled.
So the pager.attend-* option seems tricky as it is right now. That's probably why it
isn't documented, and we suggest using pager.ignore instead.

To be consistent, we can enable pager by default and allow it to be disabled via
attend-* configs, just like core hg commands. Since we have already BC on "log"
(default changed from not using pager to use pager), I think it's okay to BC here.
That looks cleaner than adding more config flags?

Given GUI diff is one of the main use case of extdiff, it's probably unacceptable to
introduce BC of breaking it.

quark abandoned this revision.EditedOct 9 2017, 9:21 PM

I'm not sure if it's worth adding more config options. Maybe just make it the user's responsibility since they can always pipe the output to a pager, or use --pager=on.

Given the fact that there are easy alternative solutions. I'm abandoning this patch.

Revision Contents
Changeset List

PathPackages
Mhgext/extdiff.py (25 lines)
Mtests/test-extdiff.t (24 lines)

Diff 2071

hgext/extdiff.py

[diff-tools] [diff-tools]
kdiff3.diffargs=--L1 '$plabel1' --L2 '$clabel' $parent $child kdiff3.diffargs=--L1 '$plabel1' --L2 '$clabel' $parent $child
You can use -I/-X and list of file or directory names like normal You can use -I/-X and list of file or directory names like normal
:hg:`diff` command. The extdiff extension makes snapshots of only :hg:`diff` command. The extdiff extension makes snapshots of only
needed files, so running the external diff program will actually be needed files, so running the external diff program will actually be
pretty fast (at least faster than having to compare the entire tree). pretty fast (at least faster than having to compare the entire tree).
Pager is disabled by default. To enable pager for an extdiff command, set::
[pager]
attend-cdiff = true
''' '''
from __future__ import absolute_import from __future__ import absolute_import
import os import os
import re import re
import shutil import shutil
import tempfile import tempfile
ui.note(' %s\n' % wfn) ui.note(' %s\n' % wfn)
if node is None: if node is None:
dest = os.path.join(base, wfn) dest = os.path.join(base, wfn)
fnsandstat.append((dest, repo.wjoin(fn), os.lstat(dest))) fnsandstat.append((dest, repo.wjoin(fn), os.lstat(dest)))
return dirname, fnsandstat return dirname, fnsandstat
def dodiff(ui, repo, cmdline, pats, opts): def dodiff(ui, repo, cmdname, cmdline, pats, opts):
'''Do the actual diff: '''Do the actual diff:
- copy to a temp structure if diffing 2 internal revisions - copy to a temp structure if diffing 2 internal revisions
- copy to a temp structure if diffing working revision with - copy to a temp structure if diffing working revision with
another one and more than 1 file is changed another one and more than 1 file is changed
- just invoke the diff for a single file in the working dir - just invoke the diff for a single file in the working dir
''' '''
# Match parent2 first, so 'parent1?' will match both parent1 and parent # Match parent2 first, so 'parent1?' will match both parent1 and parent
regex = (r'''(['"]?)([^\s'"$]*)''' regex = (r'''(['"]?)([^\s'"$]*)'''
r'\$(parent2|parent1?|child|plabel1|plabel2|clabel|root)\1') r'\$(parent2|parent1?|child|plabel1|plabel2|clabel|root)\1')
if not do3way and not re.search(regex, cmdline): if not do3way and not re.search(regex, cmdline):
cmdline += ' $parent1 $child' cmdline += ' $parent1 $child'
cmdline = re.sub(regex, quote, cmdline) cmdline = re.sub(regex, quote, cmdline)
ui.debug('running %r in %s\n' % (cmdline, tmproot)) ui.debug('running %r in %s\n' % (cmdline, tmproot))
# Unlike the default opt-out behavior, extdiff pager is opt-in. i.e.
# users need to explicitly enable pager for extdiff commands. This is
# because pager should not be used if the external program uses curses
# or GUI, which we cannot tell here.
if ui.configbool('pager', 'attend-%s' % cmdname):
ui.pager(cmdname)
ui.system(cmdline, cwd=tmproot, blockedtag='extdiff') ui.system(cmdline, cwd=tmproot, blockedtag='extdiff')
for copy_fn, working_fn, st in fnsandstat: for copy_fn, working_fn, st in fnsandstat:
cpstat = os.lstat(copy_fn) cpstat = os.lstat(copy_fn)
# Some tools copy the file and attributes, so mtime may not detect # Some tools copy the file and attributes, so mtime may not detect
# all changes. A size check will detect more cases, but not all. # all changes. A size check will detect more cases, but not all.
# The only certain way to detect every case is to diff all files, # The only certain way to detect every case is to diff all files,
# which could be expensive. # which could be expensive.
revisions are specified, the working directory files are compared revisions are specified, the working directory files are compared
to its parent.''' to its parent.'''
program = opts.get('program') program = opts.get('program')
option = opts.get('option') option = opts.get('option')
if not program: if not program:
program = 'diff' program = 'diff'
option = option or ['-Npru'] option = option or ['-Npru']
cmdline = ' '.join(map(util.shellquote, [program] + option)) cmdline = ' '.join(map(util.shellquote, [program] + option))
return dodiff(ui, repo, cmdline, pats, opts) return dodiff(ui, repo, 'extdiff', cmdline, pats, opts)
class savedcmd(object): class savedcmd(object):
"""use external program to diff repository (or selected files) """use external program to diff repository (or selected files)
Show differences between revisions for the specified files, using Show differences between revisions for the specified files, using
the following program:: the following program::
%(path)s %(path)s
When two revision arguments are given, then changes are shown When two revision arguments are given, then changes are shown
between those revisions. If only one revision is specified then between those revisions. If only one revision is specified then
that revision is compared to the working directory, and, when no that revision is compared to the working directory, and, when no
revisions are specified, the working directory files are compared revisions are specified, the working directory files are compared
to its parent. to its parent.
""" """
def __init__(self, path, cmdline): def __init__(self, path, cmdline, cmdname):
# We can't pass non-ASCII through docstrings (and path is # We can't pass non-ASCII through docstrings (and path is
# in an unknown encoding anyway) # in an unknown encoding anyway)
docpath = util.escapestr(path) docpath = util.escapestr(path)
self.__doc__ = self.__doc__ % {'path': util.uirepr(docpath)} self.__doc__ = self.__doc__ % {'path': util.uirepr(docpath)}
self._cmdline = cmdline self._cmdline = cmdline
self._cmdname = cmdname
def __call__(self, ui, repo, *pats, **opts): def __call__(self, ui, repo, *pats, **opts):
options = ' '.join(map(util.shellquote, opts['option'])) options = ' '.join(map(util.shellquote, opts['option']))
if options: if options:
options = ' ' + options options = ' ' + options
return dodiff(ui, repo, self._cmdline + options, pats, opts) return dodiff(ui, repo, self._cmdname, self._cmdline + options, pats,
opts)
def uisetup(ui): def uisetup(ui):
for cmd, path in ui.configitems('extdiff'): for cmd, path in ui.configitems('extdiff'):
path = util.expandpath(path) path = util.expandpath(path)
if cmd.startswith('cmd.'): if cmd.startswith('cmd.'):
cmd = cmd[4:] cmd = cmd[4:]
if not path: if not path:
path = util.findexe(cmd) path = util.findexe(cmd)
diffopts = False diffopts = False
# look for diff arguments in [diff-tools] then [merge-tools] # look for diff arguments in [diff-tools] then [merge-tools]
if not diffopts: if not diffopts:
args = ui.config('diff-tools', cmd+'.diffargs') or \ args = ui.config('diff-tools', cmd+'.diffargs') or \
ui.config('merge-tools', cmd+'.diffargs') ui.config('merge-tools', cmd+'.diffargs')
if args: if args:
cmdline += ' ' + args cmdline += ' ' + args
command(cmd, extdiffopts[:], _('hg %s [OPTION]... [FILE]...') % cmd, command(cmd, extdiffopts[:], _('hg %s [OPTION]... [FILE]...') % cmd,
inferrepo=True)(savedcmd(path, cmdline)) inferrepo=True)(savedcmd(path, cmdline, cmd))
# tell hggettext to extract docstrings from these functions: # tell hggettext to extract docstrings from these functions:
i18nfunctions = [savedcmd] i18nfunctions = [savedcmd]

tests/test-extdiff.t

$ echo "[extensions]" >> $HGRCPATH $ cat >> $TESTTMP/fakepager.py <<EOF
$ echo "extdiff=" >> $HGRCPATH > import sys
> for line in sys.stdin:
> sys.stdout.write('paged! %s' % line)
> sys.stdout.flush()
> EOF
$ cat >> $HGRCPATH <<EOF
> [pager]
> pager=$PYTHON $TESTTMP/fakepager.py
> [extensions]
> extdiff=
> EOF
$ hg init a $ hg init a
$ cd a $ cd a
$ echo a > a $ echo a > a
$ echo b > b $ echo b > b
$ hg add $ hg add
adding a adding a
adding b adding b
Check diff are made from the first parent: Check diff are made from the first parent:
$ hg falabala -c 3 || echo "diff-like tools yield a non-zero exit code" $ hg falabala -c 3 || echo "diff-like tools yield a non-zero exit code"
diffing "*\\extdiff.*\\a.2a13a4d2da36\\a" "a.46c0e4daeb72\\a" (glob) (windows !) diffing "*\\extdiff.*\\a.2a13a4d2da36\\a" "a.46c0e4daeb72\\a" (glob) (windows !)
diffing */extdiff.*/a.2a13a4d2da36/a a.46c0e4daeb72/a (glob) (no-windows !) diffing */extdiff.*/a.2a13a4d2da36/a a.46c0e4daeb72/a (glob) (no-windows !)
diff-like tools yield a non-zero exit code diff-like tools yield a non-zero exit code
Pager can be used if explicitly specified:
$ hg falabala -c 1 --config ui.formatted=yes --config pager.attend-falabala=1
paged! * (glob)
[1]
$ hg extdiff -p echo -r 0 --config ui.formatted=yes --config pager.attend-extdiff=1
paged! * (glob)
[1]
issue3153: ensure using extdiff with removed subrepos doesn't crash: issue3153: ensure using extdiff with removed subrepos doesn't crash:
$ hg init suba $ hg init suba
$ cd suba $ cd suba
$ echo suba > suba $ echo suba > suba
$ hg add $ hg add
adding suba adding suba
$ hg ci -m "adding suba file" $ hg ci -m "adding suba file"