Summary: Also change the internal API so it no longer accepts the "heads" argument.
Reviewed By: ryanmce
Differential Revision: D6745865
fbshipit-source-id: 368742be49b192f7630421003552d0a10eb0b76d
Invocation of "diff tool.py" in test-extdiff.t tests whether
shellquote() is applied on specified command as expected.
But direct invocation of "*.py" file might cause unexpected result on
Windows according to suffix binding.
For example, starting IDE, showing dialog to choose program to be
used, and so on. In such case, running test-extdiff.t is easily timed
out.
This patch uses intermediate *.bat file on Windows, to avoid such
unexpected result. Naming that intermediate file as "diff tool.bat" is
enough to test applying shellquote().
Some tools like BeyondCompare allow the file mode to be changed. The change
was previously applied if the content of the file changed (either according to
size or mtime), but was not being copied back for a mode-only change. That
would seem to indicate handling this in an 'elif' branch, but I opted not to in
order to avoid copying back the mode without the content changes when mtime and
size are unchanged. (Yes, that's a rare corner case, but all the more reason
not to have a subtle difference in behavior.)
The only way I can think to handle this undetected change is to set each file in
the non-wdir() snapshot to readonly, and check for that attribute (as well as
mtime) when deciding to copy back. That would avoid the overhead of copying the
whole file when only the mode changed. But a chmod in a diff tool is likely
rare. See also 40f0dd92cf6b.
In theory, it should be enough to pay attention only to the modification time
when detecting if a snapshotted working directory file changed. In practice,
BeyondCompare preserves all file attributes when syncing files at the directory
level. (If you open the file and sync individual hunks, then mtime does change,
and everything was being copied back as desired.) I'm not sure how many other
synchronization tools would trigger this issue, but it's annoyingly inconsistent
(if a single file is diffed, it isn't snapshotted, so the same BeyondCompare
file sync operation _is_ visible, because wdir() is updated in place.
I filed a bug with them, and they stated it is on their wish list, but won't be
fixed in the near term. This isn't a complete fix (there is still the case of
the size not changing), but this seems like a trivial enough change to fix most
of the problem. I suppose we could fool around with making files in the other
snapshot readonly, and copy back if we see the readonly bit copied. That seems
pretty hacky though, and only works if the external tool copies all attributes.
Now that output can be conditionalized, the few `chmod +x` specific outputs can
be conditionalized, and the rest of the tests run as normal. Disable one test
that is failing on Windows for now.
The ability to negate any boolean flags itself is great, but I think we are not
ready to expose the help side of it yet.
First, while there exist a handful of such flags whose default value can be
changed (eg: git diff, patchwork confirmation), there is only a few of them. The
users who benefit the most from this change are alias users and large
installation that can deploy extension to change behavior (eg: facebook
tweakdefault). So the majority of user who will be affected by a large change
to command help that is not yet relevant to them. (I expect this to become
relevant when ui.progressive start to exists).
Below is an example of the impact of the new help on 'hg help diff':
-r --rev REV [+] revision
-c --change REV change made by revision
-a --[no-]text treat all files as text
-g --[no-]git use git extended diff format
--[no-]nodates omit dates from diff headers
--[no-]noprefix omit a/ and b/ prefixes from filenames
-p --[no-]show-function show which function each change is in
--[no-]reverse produce a diff that undoes the changes
-w --[no-]ignore-all-space ignore white space when comparing lines
-b --[no-]ignore-space-change ignore changes in the amount of white space
-B --[no-]ignore-blank-lines ignore changes whose lines are all blank
-U --unified NUM number of lines of context to show
--[no-]stat output diffstat-style summary of changes
--root DIR produce diffs relative to subdirectory
-I --include PATTERN [+] include names matching the given patterns
-X --exclude PATTERN [+] exclude names matching the given patterns
-S --[no-]subrepos recurse into subrepositories
Another issue with the current state of help, the default value for the
flag is not conveyed to the user. For example in the 'backout' help, there is
no real distinction between "--[no-]backup" (default to True) and "--[no-]keep"
(default) to False:
--[no-]backup no backups
--[no-]keep do not modify working directory during strip
In addition, I've discussed with Augie Fackler and the last batch of the work on
this have burned him out quite some. Therefore he is not intending to perform
any more work on this topic. Quoting him, he would rather see the help part
backed out than spending more time on it.
I do not think we are ready to expose this to users in 4.0 (freeze in a week),
especially because we cannot expect quick improvement on these aspect as this
topic no longer have an owner. We should be able to reintroduce that change in
the future when someone get back on it and the main issues are solves:
* Introduction of ui.progressive makes it relevant for a majority of user,
* Current default value are efficiently conveyed to the user.
(In addition, the excerpt from diff help show that we still have some issue with
some negative option like '--nodates' so further improvement are probably
welcome there.)
That is, help gets tweaked thus:
global options ([+] can be repeated):
-v --[no-]verbose enable additional output
Other proposals have included:
global options ([+] can be repeated, options marked [?] are boolean flags):
-v --verbose[?] enable additional output
and
global options ([+] can be repeated, options marked [^] are boolean flags):
-v --verbose[^] enable additional output
which avoid the unfortunate visual noise in this patch. In this
version's favor, it's consistent with what I'm used to seeing in man
pages and similar documentation venues.
The existing code (a) assumed path would be specified in
encoding.encoding and (b) assumed unicode() objects wouldn't cause
other parts of Mercurial to blow up. Both are dangerous assumptions.
Since we don't know the encoding of path and can't pass non-ASCII
through docstrings, just escape the path and drop the early _(). Will
have to suffice until we can teach docstrings to handle UTF-8b
escaping.
This has the side-effect that the line containing the path is now
variable by the time it reaches _() and thus can't be translated.
This rule detects "hg extdiff" invocation without -p/--program and
-o/--option.
This patch specifies "-p diff" explicitly in test-extdiff.t to avoid
false positive matching.
Previously, when extdiff was called on two changesets where
a subrepository had been removed, an unexpected KeyError would
be raised.
Now, the missing subrepository will be ignored. This behavior
mirrors the behavior in diffordiffstat from cmdutil.py line
~1138-1153. The KeyError is caught and the revision is
set to None.
try/catch of LookupError around matchmod.narrowmatcher and
sub.status is removed, as LookupError is not raised anywhere
within those methods or deeper calls.
When using 'extdiff --patch' to check the changes in a rebase, 'precursors(x)'
evaluated to an empty set because I forgot the --hidden flag, so the other
revision was used as the replacement for the empty set. The result was the
patch for the other revision was diffed against itself, and the tool saying
there were no differences. That's misleading since the expected diff args were
silently changed, so it's better to bail out.
The other uses of scmutil.revpair() are commands.diff and commands.status, and
it doesn't make sense to allow an empty revision there either. The code here
was suggested by Yuya Nishihara.
One of the things I missed the most when transitioning from versioned MQ to
evolve was the loss of being able to check that rebase conflicts were properly
resolved by:
$ hg ci --mq -m "before"
$ hg rebase -s qbase -d tip
$ hg bcompare --mq
The old csets stay in the tree with evolve, but a straight diff includes all of
the other changes that were pulled in, obscuring the code that was rebased.
Diffing deltas can be confusing, but unless radical changes were made during the
resolve, it is very clear when individual hunks are added, dropped or modified.
Unlike the MQ technique, this can only compare a single pair of csets/patches at
a time. Like the MQ method, this also highlights changes in the commit comment
and other metadata.
I originally tried monkey patching from the evolve extension, but that is too
complicated given that it depends on the order the two different extensions are
loaded. This functionality is also useful when comparing grafts however, so
implementing it in the core is more than just convenience.
The --change argument doesn't make much sense for this, but it isn't harmful so
I didn't bother blocking it. The -I/-X options are ignored because of a
limitation of cmdutil.export(). We'll fix that next.
The '~' in the bug report is being expanded to a path with Windows style slashes
before being passed to shellquote() via util.shellquote(). But shlex.split()
strips '\' out of the string, leaving an invalid path in dispatch.aliasargs().
This regressed in 72640182118e.
For now, the tests need to be conditionalized for Windows (because those paths
are quoted). In the future, a more complex regex could probably skip the quotes
if all component separators are double '\'. I opted to glob away the quotes in
test-rename-merge2.t and test-up-local-change.t (which only exist on Windows),
because they are in very large blocks of output and there are way too many diffs
to conditionalize with #if directives. Maybe the entire path should be globbed
away like the following paths in each changed line. Or, letting #if directives
sit in the middle of the output as was mentioned a few months back would work
too.
Unfortunately, I couldn't figure out how to test the specific bug. All of the
'hg serve' tests have a #require serve declaration, causing them to be skipped
on Windows. Adding an alias for 'expandtest = outgoing ~/bogusrepo' prints the
repo as '$TESTTMP/bogusrepo', so the test runner must be changing the
environment somehow.
My Mac test machine has 'echo' in '/opt/local/libexec/gnubin/echo' since, well,
GNU is not BSD.
Also, I feel it need to be said about using regexes:
Some people, when confronted with a problem, think "I know, I'll use regular
expressions." Now they have two problems.
b8552020f458 broke things ... and the following cleanups didn't fix all issues.
It didn't work with the diffargs shipped in mergetools.rc with explicit
quoting. Parameters would end up with being quoted twice - especially if they
really needed quoting.
To work around that, look for explicit quotes around the variables that will be
substituted with proper quoting. Also accept an additional prefix so we can
handle both
--foo='$parent'
and
'--foo=$parent'
It will however still fail if the user intentionally place the variable inside
a quoted string, as in
'parent $parent is on the left'
There is currently no good way to handle that, short of knowing exactly which
quoting mechanism will be used.
This patch makes "posix.shellquote" examine the specified string and
quote it only when it may have to be quoted for safety, like as the
previous patch for "windows.shellquote".
In fact, on POSIX environment, quoting itself doesn't cause issues
like issue4463. But (almost) equivalent quoting policy can avoid
examining test result differently on POSIX and Windows (even though
showing command line with "%r" causes such examination in
"test-extdiff.t").
The last hunk for "test-extdiff.t" in this patch isn't needed for the
previous patch for "windows.shellquote", because the code path of it
is executed only "#if execbit" (= avoided on Windows).
Before this patch, "windows.shellquote" (as used as "util.shellquote")
always quotes specified strings with double quotation marks, for
external process invocation.
But some problematic applications can't work correctly, when command
line arguments are quoted: see issue4463 for detail.
On the other hand, quoting itself is needed to specify arguments
containing whitespaces and/or some special characters exactly.
This patch makes "windows.shellquote" examine the specified string and
quote it only when it may have to be quoted for safety.
Before this patch, all command line arguments for external tools are
quoted by the combination of "shlex.split" and "util.shellquote". But
this causes some problems.
- some problematic commands can't work correctly with quoted arguments
For example, 'WinMerge /r ....' is OK, but 'WinMerge "/r" ....' is
NG. See also below for detail about this problem.
https://bitbucket.org/tortoisehg/thg/issue/3978/
- quoting itself may change semantics of arguments
For example, when the environment variable CONCAT="foo bar baz':
- mydiff $CONCAT => mydiff foo bar baz (taking 3 arguments)
- mydiff "$CONCAT" => mydiff "foo bar baz" (taking only 1 argument)
For another example, single quoting (= "util.shellquote") on POSIX
environment prevents shells from expanding environment variables,
tilde, and so on:
- mydiff "$HOME" => mydiff /home/foobar
- mydiff '$HOME' => mydiff $HOME
- "shlex.split" can't handle some special characters correctly
It just splits specified command line by whitespaces.
For example, "echo foo;echo bar" is split into ["echo",
"foo;echo", "bar"].
On the other hand, if quoting itself is omitted, users can't specify
options including space characters with "--option" at runtime.
The root cause of this issue is that "shlex.split + util.shellquote"
combination loses whether users really want to quote each command line
elements or not, even though these can be quoted arbitrarily in
configurations.
To resolve this problem, this patch does:
- prevent configurations from being processed by "shlex.split" and
"util.shellquote"
only (possibly) "findexe"-ed or "findexternaltool"-ed command path
is "util.shellquote", because it may contain whitespaces.
- quote options specified by "--option" via command line at runtime
This patch also makes "dodiff()" take only one "args" argument instead
of "diffcmd" and "diffopts. It also omits applying "util.shellquote"
on "args", because "args" should be already stringified in "extdiff()"
and "mydiff()".
The last hunk for "test-extdiff.t" replaces two whitespaces by single
whitespace, because change of "' '.join()" logic causes omitting
redundant whitespaces.
There are three ways to configure an extdiff tool:
1) cmd.tool = (/path/to/exe optional)
2) tool = (path/to/exe optional)
3) tool = sometool someargs
Previously, if no executable is specified in the first two forms, the named tool
must be in $PATH, or the invocation fails. Since the [merge-tools] section
already has the path to the diff executable, and/or the registry keys to find
the executable on Windows, reuse that configuration for forms 1 and 2 instead of
failing. We already fallback to [diff-tools] and then [merge-tools] for program
arguments if they aren't specified in the [extdiff] section.
Since this additional lookup only occurs if an executable is not on the $PATH
for the named tool, this is backwards compatible. For now, we assume the user
knows what he is doing if a path is provided.
This change allows a configuration file like this (assuming beyondcompare3 is
configured in merge-tools), instead of hardcoding system specific a path:
[extdiff]
beyondcompare3 =
We used to have two slightly different message which people wouldn't read...
and then complain that they couldn't find the global options or examples.
So we unify them into one message that's upfront that STUFF IS
INTENTIONALLY HIDDEN and that looks more like our normal hint style.
$ hg extdiff -p cmd -o "name <user@example.com>"
resulted in a shell redirection error (due to the less-than sign),
rather than passing the single option to cmd. This was due to options
not being quoted for passing to the shell, via util.system(). Apply
util.shellquote() to each of the user-specified options (-o) to the
comparison program before they are concatenated and passed to
util.system(). The requested external diff command (-p) and the
files/directories being compared are already quoted correctly.
The discussion at the time of changeset 6654fcb57d92 correctly noted
that this course of action breaks whitespace-separated options specified
for external diff commands in the configuration. The lower part of the
patch corrects this by lexing options read from the configuration file
into separate options rather than reading them all into the first
option.
Update test to cover these conditions.
Related changesets (reverse-chronological):
- 6654fcb57d92 (fix reverted to make configuration file options work)
- c64ec6e8ffa2 (issue fixed but without fix for configuration file)
Before this patch, there is no information about whether help document
is fully displayed or not.
So, some users seem to misunderstand "-v" for "hg help" just as "the
option to show list of global options": experience on "hg help -v" for
some commands not containing verbose containers may strengthen this
misunderstanding.
Such users have less opportunity for noticing omitted help document,
and this may cause insufficient understanding about Mercurial.
This patch indicates help omitting, if help document is not fully
displayed.
For command help, the message below is displayed at the end of help
output, if help document is not fully displayed:
use "hg -v help xxxx" to show more complete help and the global
options
and otherwise:
use "hg -v help xxxx" to show the global options
For topics and extensions help, the message below is displayed, only
if help document is not fully displayed:
use "hg help -v xxxx" to show more complete help
This allows users to know whether there is any omitted information or
not exactly, and can trigger "hg help -v" invocation.
This patch causes formatting help document twice, to switch messages
one for omitted help, and another for not omitted. This decreases
performance of help document formatting, but it is not mainly focused
at help command invocation, so this wouldn't become problem.
