Hi,
After looking at Stefan and Ivan's very helpful critique, I had one of
those 'may be good or may be terrible' ideas :)
--invoke-diff-cmd is now absorbed into --diff-cmd (whilst preserving the
old functionality and look), and most issues that Ivan and Stefan
flagged up have been resolved thus.
I like this better because there is only one --diff-cmd now, and it's
a much simpler structure (at least this is the theory).
I've adjusted the original diff_external_diffcmd_test and added 4 more,
and all bar one XFAIL pass.
Because I'm not a great parser coder, and I'm not quite sure that the
current behavior is what we actually want, I've left the library
function style in svn_io__create_custom_diff_cmd() in place for now,
until we have a precise idea of what is actually required, since I
know exactly what this does, which isn't a givens for C strings ;-)
As suggested by danielsh, I attempted to add support for program names
with a space in them and allowed the user to type 'my\ diff foo bar baz'
but unfortunately this fails at shell level, and I don't know enough
about this subject to find a good solution here.
The failed test diff_tests.py 52 shows up like this:
(I renamed the diff_script to 'diff script' at generation time, test
52 is a copy of test 51, bar the diff script name change.)
[[[
g@campion:~/test_trunk/subversion/tests/cmdline$ ./diff_tests.py 52
W: exec of './diff\ script' failed: No such file or
directorysubversion/svn/diff-cmd.c:427,
W: subversion/libsvn_client/diff.c:2403,
W: subversion/libsvn_client/diff.c:2211,
W: subversion/libsvn_client/diff.c:1676,
W: subversion/libsvn_wc/diff_local.c:500,
W: subversion/libsvn_wc/status.c:2717,
W: subversion/libsvn_wc/status.c:1460,
W: subversion/libsvn_wc/status.c:1115,
W: subversion/libsvn_wc/status.c:834,
W: subversion/libsvn_wc/diff_local.c:369,
W: subversion/libsvn_wc/diff_editor.c:555,
W: subversion/libsvn_client/diff.c:950,
W: subversion/libsvn_client/diff.c:828,
W: subversion/libsvn_subr/io.c:3235: (apr_err=SVN_ERR_EXTERNAL_PROGRAM)
W: svn: E200012: './diff\ script -L "X Y Z" -L "A B C D" %svn_old
+%svn_new+' was expanded to './diff\ script "X Y Z" -L "A B C D"
/home/g/test_trunk/subversion/tests/cmdline/svn-test-work/working_copies/diff_tests-52/.svn/pristine/2c/2c0aa9014a0cd07f01795a333d82485ef6d083e2.svn-base
+/home/g/test_trunk/subversion/tests/cmdline/svn-test-work/working_copies/diff_tests-52/iota+
' and returned 255
W: CWD:
/home/g/test_trunk/subversion/tests/cmdline/svn-test-work/working_copies/diff_tests-52
W: EXCEPTION: Failure: Command failed:
"/home/g/test_trunk/subversion/svn/svn diff --diff-cmd=./diff\ script -L
"X Y Z" -L "A B C D" %svn_old +%svn_new+ iota ..."; exit code 1
Traceback (most recent call last):
File "/home/g/test_trunk/subversion/tests/cmdline/svntest/main.py",
line 1621, in run
rc = self.pred.run(sandbox)
File
"/home/g/test_trunk/subversion/tests/cmdline/svntest/testcase.py", line
114, in run
return self._delegate.run(sandbox)
File
"/home/g/test_trunk/subversion/tests/cmdline/svntest/testcase.py", line
176, in run
return self.func(sandbox)
File "./diff_tests.py", line 3292, in diff_external_diffcmd_4
iota_path)
File
"/home/g/test_trunk/subversion/tests/cmdline/svntest/actions.py", line
284, in run_and_verify_svn
expected_exit, *varargs)
File
"/home/g/test_trunk/subversion/tests/cmdline/svntest/actions.py", line
322, in run_and_verify_svn2
exit_code, out, err = main.run_svn(want_err, *varargs)
File "/home/g/test_trunk/subversion/tests/cmdline/svntest/main.py",
line 702, in run_svn
*(_with_auth(_with_config_dir(varargs))))
File "/home/g/test_trunk/subversion/tests/cmdline/svntest/main.py",
line 368, in run_command
None, *varargs)
File "/home/g/test_trunk/subversion/tests/cmdline/svntest/main.py",
line 560, in run_command_stdin
'"; exit code ' + str(exit_code))
Failure: Command failed: "/home/g/test_trunk/subversion/svn/svn diff
--diff-cmd=./diff\ script -L "X Y Z" -L "A B C D" %svn_old +%svn_new+
iota ..."; exit code 1
XFAIL: diff_tests.py 52: svn diff --diff-cmd w. spaces in diff program name
]]]
thanks for looking,
Gabriela
[[[
Patch for discussion: Add new syntax facility to --diff-cmd and it's
config counterpart, whilst preserving the old behaviour.
The new syntax allows the use of 4 keywords:
%svn_new_label, %svn_old_label, %svn_old %svn_new
So constructs like:
$svn diff --diff-cmd=
"diff --ignore-file-name-case -L "One Two" -L %svn_new_label %svn_old
%svn_new"
are possible. --extensions is not used if the code does not detect an
old style diff-command command. Characters immediately adjacent to a
keyword are also supported (as requested by Julian Foad).
The tests for this patch may be run with ./diff_tests.py 48 through 52
* subversion/include/private/svn_io_private.h
(svn_io__create_custom_diff_cmd): Declare new function and document
it.
* subversion/libsvn_subr/io.c
(svn_io__create_custom_diff_cmd): New Function. This function
translates user input to diff input.
(svn_io_run_diff): Refactor function to preserve old behaviour of
--diff-cmd whilst facilitiating the new extended syntax.
* subversion/tests/cmdline/diff_tests.py
(diff_external_diffcmd): Change test description, rename the test
script 'diff' to the more descriptive 'diff_script.
(diff_external_diffcmd_1): New function. Test that the --extensions
option works as before when the --diff-cmd is used with the
classic syntax.
(diff_external_diffcmd_2): New function. Check that the new syntax
produces a correct, simple diff program call.
(diff_external_diffcmd_3): New function. Check that multi word
labels that may just have one letter are correctly handled. Check
that file names can be correctly parsed into +file_name+
form. This case handles also the cases of -filename or filename+
where + and - are placeholders for any chosen character. Check
that escaped spaces are correctly handled, in case the user has a
windows-style program name with a space in it. This currently
fails at shell level, so this may be the wrong approach
alltogether. Error message obtained is provided as a comment at
the end of the function.
(diff_external_diffcmd_4): New function with XFAIL result. Check
that escaped spaces are correctly handled, in case the user has a
program name with a space in it. This currently fails at shell
level, so this may be the wrong approach alltogether.
(test_list): Add diff_external_diffcmd_1, diff_external_diffcmd_2,
diff_external_diffcmd_3 and diff_external_diffcmd_4.
]]]
Index: diff_tests.py
===================================================================
--- diff_tests.py (revision 1604282)
+++ diff_tests.py (working copy)
@@ -3065,9 +3065,11 @@ def diff_wrong_extension_type(sbox):
svntest.actions.run_and_verify_svn(None, [], err.INVALID_DIFF_OPTION,
'diff', '-x', sbox.wc_dir, '-r', '1')
+#----------------------------------------------------------------------
# Check the order of the arguments for an external diff tool
+# This tests the old-style command
def diff_external_diffcmd(sbox):
- "svn diff --diff-cmd provides the correct arguments"
+ "org. svn diff --diff-cmd provides correct args"
sbox.build(read_only = True)
os.chdir(sbox.wc_dir)
@@ -3077,7 +3079,7 @@ def diff_external_diffcmd(sbox):
# Create a small diff mock object that prints its arguments to stdout.
# (This path needs an explicit directory component to avoid searching.)
- diff_script_path = os.path.join('.', 'diff')
+ diff_script_path = os.path.join('.', 'diff_script')
# TODO: make the create function return the actual script name, and rename
# it to something more generic.
svntest.main.create_python_hook_script(diff_script_path, 'import sys\n'
@@ -3102,7 +3104,193 @@ def diff_external_diffcmd(sbox):
'diff', '--diff-cmd', diff_script_path,
iota_path)
+#----------------------------------------------------------------------
+# Check the order of the arguments for an external diff tool
+# This tests the old-style command with extensions
+def diff_external_diffcmd_1(sbox):
+ "old svn diff --diff-cmd provides correct ext"
+ sbox.build(read_only = True)
+ os.chdir(sbox.wc_dir)
+
+ iota_path = 'iota'
+ svntest.main.file_append(iota_path, "new text in iota")
+
+ # Create a small diff mock object that prints its arguments to stdout.
+ # (This path needs an explicit directory component to avoid searching.)
+ diff_script_path = os.path.join(os.getcwd(), 'diff_script')
+ # TODO: make the create function return the actual script name, and rename
+ # it to something more generic.
+ svntest.main.create_python_hook_script(diff_script_path, 'import sys\n'
+ 'for arg in sys.argv[1:]:\n print(arg)\n')
+ if sys.platform == 'win32':
+ diff_script_path = "%s.bat" % diff_script_path
+
+ expected_output = svntest.verify.ExpectedOutput([
+ "Index: iota\n",
+ "===================================================================\n",
+ "-p\n",
+ "-u\n",
+ "-L\n",
+ "iota\t(revision 1)\n",
+ "-L\n",
+ "iota\t(working copy)\n",
+ os.path.abspath(svntest.wc.text_base_path("iota")) + "\n",
+ os.path.abspath("iota") + "\n"])
+
+ # Check that the output of diff corresponds with the expected arguments,
+ # in the correct order.
+ svntest.actions.run_and_verify_svn(None, expected_output, [],
+ 'diff',
+ '-x -p -u',
+ '--diff-cmd',
+ diff_script_path,
+ iota_path)
+
+# Check the correct parsing of arguments for an external diff tool
+# this tests the simple new style command
+def diff_external_diffcmd_2(sbox):
+ "new svn diff --diff-cmd provides correct args"
+
+ sbox.build(read_only = True)
+ os.chdir(sbox.wc_dir)
+
+ diff_script_path = os.path.join('.', 'diff_script')
+
+ svntest.main.create_python_hook_script(diff_script_path, 'import sys\n'
+ 'for arg in sys.argv[1:]:\n print(arg)\n')
+
+ if sys.platform == 'win32':
+ diff_script_path = "%s.bat" % diff_script_path
+
+ iota_path = 'iota'
+ svntest.main.file_append(iota_path, "new text in iota")
+
+ expected_output = svntest.verify.ExpectedOutput([
+ "Index: iota\n",
+ "===================================================================\n",
+ "-L\n",
+
+ # correct label %svn_label_old -> label 1
+ "iota (revision 1)\n",
+
+ "-L\n",
+ # correct label %svn_label_new -> label 2
+ "iota (working copy)\n",
+
+ # correct file %svn_old -> old
+ os.path.abspath(svntest.wc.text_base_path("iota")) + "\n",
+
+ # correct file %svn_new -> new
+ os.path.abspath("iota") + "\n",
+ ])
+
+ svntest.actions.run_and_verify_svn(None, expected_output, [],
+ 'diff',
+ '--diff-cmd='+diff_script_path+' -L %svn_label_old -L %svn_label_new %svn_old %svn_new ',
+ iota_path)
+
+
+# Check the correct parsing of arguments for an external diff tool
+# this tests the new style command with n-word labels,
+# +'s on either side of the expanded file name and whether
+# program names with spaces are correctly interpreted.
+def diff_external_diffcmd_3(sbox):
+ "svn diff --diff-cmd labels with ext. syntax"
+
+ sbox.build(read_only = True)
+ os.chdir(sbox.wc_dir)
+
+ #make a windows style script name, with a space in it.
+ diff_script_path = os.path.join('.', "diff_script")
+
+ svntest.main.create_python_hook_script(diff_script_path, 'import sys\n'
+ 'for arg in sys.argv[1:]:\n print(arg)\n')
+
+ if sys.platform == 'win32':
+ diff_script_path = "%s.bat" % diff_script_path
+
+ iota_path = 'iota'
+ svntest.main.file_append(iota_path, "new text in iota")
+
+ expected_output = svntest.verify.ExpectedOutput([
+ "Index: iota\n",
+ "===================================================================\n",
+ "-L\n",
+ # correct label %svn_label_old -> label 1
+ '"X"\n',
+
+ # correct label %svn_label_new -> label 2
+ "-L\n",
+
+ '"A B C D"\n',
+
+ # correct file %svn_old -> old
+ os.path.abspath(svntest.wc.text_base_path("iota")) + "\n",
+
+ # correct insertion of filename into string "+%svn_new+" -> "+"+new+"+"
+ "+" + os.path.abspath("iota") + "+\n",
+ ])
+
+ svntest.actions.run_and_verify_svn(None, expected_output, [],
+ 'diff',
+ '--diff-cmd='+
+ diff_script_path+
+ ' -L "X" -L "A B C D" %svn_old +%svn_new+',
+ iota_path)
+
+# Check that an escaped space using '\ 'is correctly interpreted.
+# This test is expected to fail currently, see sample output in
+# comment at the end.
+@XFail()
+def diff_external_diffcmd_4(sbox):
+ "svn diff --diff-cmd w. spaces in diff program name"
+
+ sbox.build(read_only = True)
+ os.chdir(sbox.wc_dir)
+
+ #make a windows style script name, with a space in it.
+ diff_script_path = os.path.join('.', "diff script")
+
+ svntest.main.create_python_hook_script(diff_script_path, 'import sys\n'
+ 'for arg in sys.argv[1:]:\n print(arg)\n')
+ # now rename it like a human would type it
+ diff_script_path = os.path.join('.', "diff\ script")
+ if sys.platform == 'win32':
+ diff_script_path = "%s.bat" % diff_script_path
+
+ iota_path = 'iota'
+ svntest.main.file_append(iota_path, "new text in iota")
+
+ expected_output = svntest.verify.ExpectedOutput([
+ "Index: iota\n",
+ "===================================================================\n",
+ "-L\n",
+ # correct label %svn_label_old -> label 1
+ '"X Y Z"\n',
+
+ # correct label %svn_label_new -> label 2
+ "-L\n",
+
+ '"A B C D"\n',
+
+ # correct file %svn_old -> old
+ os.path.abspath(svntest.wc.text_base_path("iota")) + "\n",
+
+ # correct insertion of filename into string "+%svn_new+" -> "+"+new+"+"
+ "+" + os.path.abspath("iota") + "+\n",
+ ])
+
+ # This currently fails, but shows the anticipated output. This may be a
+ # shell issue, and needs investigating. See failure message below.
+ svntest.actions.run_and_verify_svn(None, expected_output, [],
+ 'diff',
+ '--diff-cmd='+
+ diff_script_path+
+ ' -L "X Y Z" -L "A B C D" %svn_old +%svn_new+',
+ iota_path)
+
+
#----------------------------------------------------------------------
# Diffing an unrelated repository URL against working copy with
# local modifications (i.e. not committed). This is issue #3295 (diff
@@ -4730,6 +4918,10 @@ test_list = [ None,
diff_file_depth_empty,
diff_wrong_extension_type,
diff_external_diffcmd,
+ diff_external_diffcmd_1,
+ diff_external_diffcmd_2,
+ diff_external_diffcmd_3,
+ diff_external_diffcmd_4,
diff_url_against_local_mods,
diff_preexisting_rev_against_local_add,
diff_git_format_wc_wc,
