On the 'diff-cmd-feature' branch: sync with trunk@r1936562. git-svn-id: https://svn.apache.org/repos/asf/subversion/branches/diff-cmd-feature@1936563 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/BRANCH-README b/BRANCH-README new file mode 100644 index 0000000..48b37a3 --- /dev/null +++ b/BRANCH-README
@@ -0,0 +1,63 @@ +This branch implements the 'invoke-diff-cmd' feature and is located at +https://svn.apache.org/repos/asf/subversion/branches/diff-cmd-feature/ + +This branch adds a 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 + +At a minimum, %svn_new and %svn_old are required amongst any arbitrary +character sequence. + +The code looks for the presence of %svn_old and if it does not detect +it, it translates the --diff-cmd input and --extensions if they have +been set to match the old --diff-cmds' translation before parsing the +result with svn_io__create_custom_diff_cmd(). + + +Usage: +====== + +Examples: + +svn diff --diff-cmd=diff %svn_new %svn_old +svn diff --diff-cmd=diff -L %svn_new_label %svn_new -L "Old File" %svn_old + + +TODO: +===== + +1) Finalise the current parsing capabilities +2) Write a fuzzing test for svn_io__create_custom_diff_cmd() +3) Rewrite the final svn_io__create_custom_diff_cmd() design +4) Solve the "diff\ script" problem +5) Solve the empty single back quotes problem +6) Discuss if adding this to the existing --diff-cmd (as per 2044) + is useful and desired. + + +Problems: +========= + +1) The "diff\ script" problem test problem: + + /subversion/tests/cmdline/diff_tests.py 52 + +fails, because whilst the output is as expected, the shell fails to +find the "diff script". + +2) The empty single back quotes problem: + +Emtpy single quotes are sent to the shell duplicated (twice it seems). + +The command: + +./subversion/svn/svn diff --diff-cmd="diff --changed-group-format='%<'\ + --unchanged-group-format='' %svn_new %svn_old" + +Causes the external diff command to display 4 backticks in the +unchanged-group-format, and where there should be no line at all, +"''''" (4 backticks) are displayed. +
diff --git a/subversion/include/private/svn_io_private.h b/subversion/include/private/svn_io_private.h index 1a876ab..b4ac3bd 100644 --- a/subversion/include/private/svn_io_private.h +++ b/subversion/include/private/svn_io_private.h
@@ -226,6 +226,51 @@ #endif /* WIN32 */ +/** Parse a user defined command to contain dynamically created labels + * and filenames. This function serves both diff and diff3 parsing + * requirements. + * + * When used in a diff context: (responding parse tokens in braces) + * + * @a label1 (%svn_label_old) refers to the label of @a tmpfile1 + * (%svn_old) which is the pristine copy. + * + * @a label2 (%svn_label_new) refers to the label of @a tmpfile2 + * (%svn_new) which is the altered copy. + * + * When used in a diff3 context: + * + * @a label1 refers to the label of @a tmpfile1 which is the 'mine' + * copy. + * + * @a label2 refers to the label of @a tmpfile2 which is the 'older' + * copy. + * + * @a label3 (%svn_label_base) refers to the label of @a base + * (%svn_base) which is the 'base' copy. + * + * In general: + * + * @a cmd is a user defined string containing 0 or more parse tokens + * which are expanded by the required labels and filenames. + * + * @a pool is used for temporary allocations. + * + * @return A NULL-terminated character array. + * + * @since New in 1.9. + */ +const char ** +svn_io__create_custom_diff_cmd(const char *label1, + const char *label2, + const char *label3, + const char *from, + const char *to, + const char *base, + const char *cmd, + apr_pool_t *pool); + + #ifdef __cplusplus } #endif /* __cplusplus */
diff --git a/subversion/libsvn_subr/io.c b/subversion/libsvn_subr/io.c index 4ec6adc..f6d1891 100644 --- a/subversion/libsvn_subr/io.c +++ b/subversion/libsvn_subr/io.c
@@ -3548,6 +3548,138 @@ return svn_io_wait_for_cmd(&cmd_proc, cmd, exitcode, exitwhy, pool); } +const char ** +svn_io__create_custom_diff_cmd(const char *label1, + const char *label2, + const char *label3, + const char *from, + const char *to, + const char *base, + const char *cmd, + apr_pool_t *pool) +{ + /* + This function can be tested with: + /subversion/tests/cmdline/diff_tests.py diff_external_diffcmd_1 + /subversion/tests/cmdline/diff_tests.py diff_external_diffcmd_2 + /subversion/tests/cmdline/diff_tests.py diff_external_diffcmd_3 + /subversion/tests/cmdline/diff_tests.py diff_external_diffcmd_4 + /subversion/tests/cmdline/diff_tests.py diff_external_diffcmd_5 + */ + + apr_array_header_t *words; + const char ** result; + size_t argv, item, i, delimiters = 6, spacer = 0; + apr_pool_t *scratch_pool = svn_pool_create(pool); + + struct replace_tokens_tab + { + const char *delimiter; + const char *replace; + } tokens_tab[] = { /* Diff terminology */ + { "%svn_label_old", label1 }, + { "%svn_label_new", label2 }, + { "%svn_label_base", label3 }, + { "%svn_old", from }, + { "%svn_new", to }, + { "%svn_base", base }, + { NULL, NULL } + }; + + if (label3) /* Merge terminology */ + { + tokens_tab[0].delimiter = "%svn_label_mine"; + tokens_tab[1].delimiter = "%svn_label_yours"; + tokens_tab[3].delimiter = "%svn_mine"; + tokens_tab[4].delimiter = "%svn_yours"; + } + + words = svn_cstring_split(cmd, " ", TRUE, scratch_pool); + + result = apr_palloc(pool, + (words->nelts+1) * words->elt_size * sizeof(char *) ); + + for (item = 0, argv = 0; item < (words->nelts + spacer); argv++, item++) + { + svn_stringbuf_t *word; + + word = svn_stringbuf_create_empty(scratch_pool); + svn_stringbuf_appendcstr(word, APR_ARRAY_IDX(words, item, char *)); + + /* Ensure program names with spaces are put as one entry into + the execv compatible array we are producing. */ + if ((0 == item) /* only checking the program name */ + && (word->len > 1) + && (word->data[word->len-1] == 92)) /* has a '\' */ + { + svn_stringbuf_t * complete = svn_stringbuf_create_empty(scratch_pool); + svn_stringbuf_appendcstr(complete, + APR_ARRAY_IDX(words, item++, char *)); + + svn_stringbuf_appendcstr(complete, " "); + svn_stringbuf_appendcstr(complete, + APR_ARRAY_IDX(words, item++, char *)); + + /* this accounts for two words becoming one in this case, + otherwise, the last word is not processed if an entry is + split with an escaped space. (note the second item++ + above) */ + spacer++; + + word->data = complete->data; + } + /* paste "delimited words that were separated back together */ + else if ((word->len > 1) + && (word->data[0] == '"') + && (word->data[word->len-1] != '"') ) + { + svn_stringbuf_t * complete = svn_stringbuf_create_empty(scratch_pool); + int done = 0; + + while( (!done) && item < words->nelts) + { + svn_stringbuf_appendcstr(complete, + APR_ARRAY_IDX(words, item, char *)); + + if ( (complete->data[complete->len-1] == '"') + || (item == words->nelts - 1) ) + { + word->data = complete->data; + done = 1; + } + else + { + svn_stringbuf_appendcstr(complete, " "); + item++; + } + } + } + /* find and substitute keywords */ + i = 0; + if ((item > 0) /* no need to check the program name */ + && (word->len > 7)) /* %svn_old is the smallest keyword */ + { + while (i < delimiters) + { + char *found = strstr(word->data, tokens_tab[i].delimiter); + + if (!found) + { + i++; + continue; + } + svn_stringbuf_replace(word, found - word->data, + strlen(tokens_tab[i].delimiter), + tokens_tab[i].replace, + strlen(tokens_tab[i].replace)); + } + } + result[argv] = apr_pstrdup(pool,word->data); + } + result[argv] = NULL; + svn_pool_destroy(scratch_pool); + return result; +} svn_error_t * svn_io_run_diff2(const char *dir, @@ -3561,65 +3693,72 @@ apr_file_t *outfile, apr_file_t *errfile, const char *diff_cmd, - apr_pool_t *pool) + apr_pool_t *result_pool) { - const char **args; - int i; int exitcode; - int nargs = 4; /* the diff command itself, two paths, plus a trailing NULL */ - apr_pool_t *subpool = svn_pool_create(pool); + const char ** cmd; + char *dc = NULL; + apr_pool_t *scratch_pool = svn_pool_create(result_pool); + + if (0 == strlen(diff_cmd)) + return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL, NULL); + + if (!strstr(diff_cmd, "%svn_old")) /* this is an old style command */ + { + /* + This section can be tested with: + /subversion/tests/cmdline/diff_tests.py diff_external_diffcmd + */ + svn_stringbuf_t *com; + com = svn_stringbuf_create_empty(scratch_pool); + svn_stringbuf_appendcstr(com, diff_cmd); + svn_stringbuf_appendcstr(com, " "); + + if (user_args != NULL) + { + int j; + for (j = 0; j < num_user_args; ++j) + { + svn_stringbuf_appendcstr(com, user_args[j]); + svn_stringbuf_appendcstr(com, " "); + } + } + else /* assume -u if the user didn't give us any args */ + svn_stringbuf_appendcstr(com, "-u "); + + if (label1 != NULL) + svn_stringbuf_appendcstr(com,"-L %svn_label_old "); + + if (label2 != NULL) + svn_stringbuf_appendcstr(com,"-L %svn_label_new "); + + svn_stringbuf_appendcstr(com,"%svn_old %svn_new "); + + dc = com->data; + } + else { + dc = apr_palloc(scratch_pool, strlen(diff_cmd) * sizeof(char *) ); + strcpy(dc, diff_cmd); + } + if (0 == strlen(dc)) + return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL, NULL); + + cmd = svn_io__create_custom_diff_cmd(label1, label2, NULL, + svn_dirent_local_style(from, scratch_pool), + svn_dirent_local_style(to, scratch_pool), + NULL, + dc, scratch_pool); if (pexitcode == NULL) - pexitcode = &exitcode; + pexitcode = &exitcode; - if (user_args != NULL) - nargs += num_user_args; - else - nargs += 1; /* -u */ - - if (label1 != NULL) - nargs += 2; /* the -L and the label itself */ - if (label2 != NULL) - nargs += 2; /* the -L and the label itself */ - - args = apr_palloc(subpool, nargs * sizeof(char *)); - - i = 0; - args[i++] = diff_cmd; - - if (user_args != NULL) - { - int j; - for (j = 0; j < num_user_args; ++j) - args[i++] = user_args[j]; - } - else - args[i++] = "-u"; /* assume -u if the user didn't give us any args */ - - if (label1 != NULL) - { - args[i++] = "-L"; - args[i++] = label1; - } - if (label2 != NULL) - { - args[i++] = "-L"; - args[i++] = label2; - } - - args[i++] = svn_dirent_local_style(from, subpool); - args[i++] = svn_dirent_local_style(to, subpool); - args[i++] = NULL; - - SVN_ERR_ASSERT(i == nargs); - - SVN_ERR(svn_io_run_cmd(dir, diff_cmd, args, pexitcode, NULL, TRUE, - NULL, outfile, errfile, subpool)); + SVN_ERR(svn_io_run_cmd(dir, cmd[0], cmd, pexitcode, NULL, TRUE, + NULL, outfile, errfile, scratch_pool)); /* The man page for (GNU) diff describes the return value as: - "An exit status of 0 means no differences were found, 1 means - some differences were found, and 2 means trouble." + "An exit status of 0 means no differences were found, 1 means + some differences were found, and 2 means trouble." A return value of 2 typically occurs when diff cannot read its input or write to its output, but in any case we probably ought to return an @@ -3627,17 +3766,25 @@ corrupt. */ if (*pexitcode != 0 && *pexitcode != 1) - return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL, - _("'%s' returned %d"), - svn_dirent_local_style(diff_cmd, pool), - *pexitcode); + { + int i; + const char *failed_command = ""; - svn_pool_destroy(subpool); - + for (i = 0; cmd[i]; ++i) + failed_command = apr_pstrcat(result_pool, failed_command, + cmd[i], " ", (char*) NULL); + svn_pool_destroy(scratch_pool); + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL, + _("'%s' was expanded to '%s' and returned %d"), + diff_cmd, + failed_command, + *pexitcode); + } + else + svn_pool_destroy(scratch_pool); return SVN_NO_ERROR; } - svn_error_t * svn_io_run_diff3_3(int *exitcode, const char *dir,
diff --git a/subversion/tests/cmdline/diff_tests.py b/subversion/tests/cmdline/diff_tests.py index 46fa735..d37fcd4 100755 --- a/subversion/tests/cmdline/diff_tests.py +++ b/subversion/tests/cmdline/diff_tests.py
@@ -3062,9 +3062,11 @@ svntest.actions.run_and_verify_svn([], 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) @@ -3074,7 +3076,7 @@ # 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' @@ -3099,7 +3101,193 @@ '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 @@ -5419,6 +5607,10 @@ 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,