On Mon, Jun 16, 2014 at 03:21:05PM +0100, Gabriela Gibson wrote: > Hi, Hi Gabriela,
I finally found some time to take a look at this. Comments below, based on a discussion of your patch between me and Ivan Zhakov. Specifically, we were looking for ways to simplify your patch, i.e. make it smaller without losing desired functionality. > I'm not sure how to update my branch because the underlying base has > changed(and doesn't merge all that well) and thus, I restarted with > just a patch and a copy of the current trunk. I'd suggest that you remove your branch, create a new one, and keep working on a branch in the repository. > So, just to keep things simple, I just post it as a patch for now. > > Gabriela > > Ps.: I'll take a look at the --invoke-diff3-cmd part this week sometime. > > ====================================================================== > Introduction to --invoke-diff-cmd > ====================================================================== > > --invoke-diff-cmd allows command line selection of an external diff > program and will be extended to cover the existing diff3 option with a > similar --invoke-diff3-cmd option. > > Currently this capability is provided by user written shell scripts > which are passed as the diff program via the svn config file. > > --invoke-diff-cmd is currently implemented for 'diff', 'log', > 'svnlook' and the config file. > > See: http://subversion.tigris.org/issues/show_bug.cgi?id=2044 for the > original motivation for this project. Would it be sufficient to provide the configuration file option only, and drop the command line option --invoke-diff-cmd from this patch? The --invoke-diff-cmd command line option doesn't seem very useful because we already have a --config-option which could be used if anyone had a good reason to override the configuration file option from the command line (e.g. for scripts, and the regression test). Also, the documentation for this option is long. A paragraph of comments in the config file seems to be a better place for the documentation than the 'svn help diff' output. > What --invoke-diff-cmd provides > =============================== > > Users can type 'free-style' command lines for their selected > diff/merge program, and optionally select a diff command file that > applies stored commands to selected files. If the file diffed is not > in the list, the given command will be used instead. > > from 'svn help diff': > > --invoke-diff-cmd ARG: > > use ARG as format string for external diff command > invocation. > > Substitutions: %svn_new new file > %svn_old old file > %svn_label_new label of the new file > %svn_label_old label of the old file > Examples: > --invoke-diff-cmd='diff -y %svn_new %svn_old' > --invoke-diff-cmd="kdiff3 -auto -o /home/u/log \ > %svn_new %svn_old --L1 %svn_new_label \ > --L2 "Custom Label" ' > Substitution variables may be embedded in strings: > +%svn_new, %svn_new- and file=%svn_label_new+ > > > Structure of the feature: > ========================= > > API components > -------------- > > ./subversion/libsvn_subr/io.c __create_custom_diff_cmd() > > transforms the user input 'invoke-diff-cmd' into a command line > call by substitution the labels and file names(where defined), > whilst leaving everything else untouched. > > > ./subversion/libsvn_subr/io.c svn_io_run_external_diff() > > calls __create_custom_diff_cmd() and does all the error checking > required, before routing the result to the actual call to the APR > routine that makes it. I'm not sure we need to add APIs because I don't see why a external program or library would need to make use of this. No libsvn_subr changes should be necessary to support this feature. Adding that code as static helper functions within libsvn_client/diff.c seems to be sufficient. > > UI components > ------------- > > --invoke-diff-cmd and its user interface components for the command > line have been installed everywhere where --diff-cmd is available, > in svnlook.c, svn.c, svnlog.c. > > > Tests > ===== > > The test for the 'invoke-diff-cmd' feature is > > /subversion/tests/cmdline/diff_tests.py diff_invoke_external_diffcmd > > > [[[ > > * subversion/include/private/svn_io_private.h > > (svn_io__create_custom_diff_cmd): New function declaration. > > > * subversion/include/svn_config.h > > (SVN_CONFIG_OPTION_INVOKE_DIFF_CMD): New definition. > > > * subversion/include/svn_error_codes.h > > (SVN_CLIENT_DIFF_CMD): New macro. This is called SVN_ERR_CLIENT_DIFF_CMD, not SVN_CLIENT_DIFF_CMD. Currently this error code is used when both diff-cmd and invoke-diff-cmd options are used. This might have been suggested and discussed before and I may have missed it, but why do define a new option instead of extending the functionality of the existing diff-cmd option? Would the following work? - The --diff-cmd option remains as-is (no substition support) and overrides config file diff-cmd option. - The diff-cmd option in the config file (and, by extension, the --config-option config:helpers:diff-cmd option) support substitution and return SVN_ERR_CLIENT_DIFF_CMD if an unknown substitution token is encountered (to prevent running of unexpected commands in case of accidental misconfiguration). > * subversion/include/svn_io.h > > (svn_io_run_external_diff): New function. > > * subversion/libsvn_client/diff.c > > (diff_writer_info_t): New member: 'invoke_diff_cmd'. > > (create_diff_writer_info): Add routine to read invoke_diff_cmd > either from the commandline (preferential) or from the config > file, after it is established that the diff-cmd is not defined on > the commandline or in the .subversion/config file. Readjust the > flow of the function to ensure that the internal diff routine is > called if neither diff_cmd or invoke_diff_cmd are called. > > (diff_content_changed): Raise an error if both diff_cmd and > invoke-diff-cmd are set. Add invoke-diff-cmd to if condition. > Add comment explaining the presence of non-canonical path in > function call. Call svn_io_run_external_diff if --invoke-diff-cmd > option was specified, otherwise retain previous behaviour. > > > * subversion/libsvn_subr/config_file.c > > (svn_config_ensure,"invoke-diff-cmd"): New entry: invoke-diff-cmd. > Add help info. > > > * subversion/libsvn_subr/io.c > > (svn_io__create_custom_diff_cmd): New function. > > (svn_io_run_external_diff): New function. > > > * subversion/svn/cl.h > > (struct svn_cl__opt_state_t.diff): New member: 'invoke_diff_cmd'. > > > * subversion/svn/log-cmd.c > > (log_receiver_baton): New struct member invoke_diff_cmd. > > (svn_cl__log): Ensure mutual exclusions between invoke_diff_cmd and > diff-cmd. Require 'diff' option to be set when requesting > invoke_diff option. Populate log_receiver_baton member > invoke_diff_cmd. > > > * subversion/svnlook/svnlook.c > > (enum): New variable svnlook__invoke_diff_cmd. I think we should put svnlook alone for now. It's always possible to use file:// URLs with 'svn diff', except while generating diffs from transactions in pre-commit hooks and I don't think this is a very important use case. I guess you're changing svnlook for consistency but I believe it's better to focus on just 'svn' first. 'svn diff' and 'svnlook diff' serve different use cases and have always been somewhat inconsistent. > > (options_table[]): New entry 'invoke-diff-cmd'. > > (cmd_tablcmd[]): Add svnlook__invoke_diff_cmd to diff cmd table > entry. > > (svnlook_opt_state): New member variable "invoke_diff_cmd". Adjust > comment alignment on diff-cmd. > > (svnlook_ctxt_t): New member variable "invoke_diff_cmd". > > (print_diff_tree): Modify 'if condition' to include new > invoke_diff_cmd. Add conditional call to > /include/svn_io.c:svn_io_run_external_diff(). > > (get_ctxt_baton): Assign invoke_diff_cmd data. > > (main): Add case svnlook__invoke_diff_cmd. Assign opt_arg to > opt_state.invoke_diff_cmd. Add exclusiveness test for > invoke_diff_cmd and diff_cmd. > > > * subversion/svn/svn.c > > (svn_cl__longopt_t): New enum opt_invoke_diff_cmd. > > (svn_cl__options "invoke-diff-cmd"): Add help information. Add new > variable 'opt_invoke_diff_cmd'. > > (svn_cl__cmd_table[]): New option: 'invoke-diff-cmd', help info. Add > opt_invoke_diff_cmd to svn_cl__log entry. Add opt_invoke_diff_cmd > to the list of valid subcommands. > > (sub_main): Add case opt_invoke_diff_cmd. Prohibit simultaneous > usage of --invoke-diff-cmd and --internal-diff. Prohibit > simultaneous usage of --diff-cmd and --invoke-diff-cmd. Add call > to svn_config_set. > > > * subversion/tests/cmdline/diff_tests.py > > (diff_invoke_external_diffcmd): New function. > > (test_list): Add new entry 'diff_invoke_external_diffcmd'. > > > * subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout > > (--invoke-diff-cmd): Add new entry to 'help' output data. > > ]]] > Index: subversion/include/private/svn_io_private.h > =================================================================== > --- subversion/include/private/svn_io_private.h (revision 1602604) > +++ subversion/include/private/svn_io_private.h (working copy) > @@ -161,6 +161,51 @@ > apr_pool_t *result_pool); > #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 */ > Index: subversion/include/svn_client.h > =================================================================== > --- subversion/include/svn_client.h (revision 1602604) > +++ subversion/include/svn_client.h (working copy) > @@ -3055,6 +3055,11 @@ > * The above two options are mutually exclusive. It is an error to set > * both to TRUE. > * > + * @a invoke_diff_cmd is used to call an external diff program but may > + * not be @c NULL. The command line invocation will override the > + * invoke-diff-cmd invocation entry(if any) in the Subversion > + * configuration file. > + * > * Generated headers are encoded using @a header_encoding. > * > * Diff output will not be generated for binary files, unless @a > Index: subversion/include/svn_config.h > =================================================================== > --- subversion/include/svn_config.h (revision 1602604) > +++ subversion/include/svn_config.h (working copy) > @@ -123,6 +123,8 @@ > #define SVN_CONFIG_OPTION_DIFF_EXTENSIONS "diff-extensions" > #define SVN_CONFIG_OPTION_DIFF3_CMD "diff3-cmd" > #define SVN_CONFIG_OPTION_DIFF3_HAS_PROGRAM_ARG "diff3-has-program-arg" > +/** @since New in 1.9. */ > +#define SVN_CONFIG_OPTION_INVOKE_DIFF_CMD "invoke-diff-cmd" > #define SVN_CONFIG_OPTION_MERGE_TOOL_CMD "merge-tool-cmd" > #define SVN_CONFIG_SECTION_MISCELLANY "miscellany" > #define SVN_CONFIG_OPTION_GLOBAL_IGNORES "global-ignores" > Index: subversion/include/svn_error_codes.h > =================================================================== > --- subversion/include/svn_error_codes.h (revision 1602604) > +++ subversion/include/svn_error_codes.h (working copy) > @@ -1219,6 +1219,11 @@ > SVN_ERR_CLIENT_CATEGORY_START + 23, > "The operation is forbidden by the server") > > + /** @since New in 1.9 */ > + SVN_ERRDEF(SVN_ERR_CLIENT_DIFF_CMD, > + SVN_ERR_CLIENT_CATEGORY_START + 24, > + "More than one diff command defined") > + > /* misc errors */ > > SVN_ERRDEF(SVN_ERR_BASE, > Index: subversion/include/svn_io.h > =================================================================== > --- subversion/include/svn_io.h (revision 1602604) > +++ subversion/include/svn_io.h (working copy) > @@ -2462,6 +2462,23 @@ > > /** @} */ > > +/** Run the external diff command defined by the invoke-diff-cmd > + * option. > + * > + * @since New in 1.9. > + */ > +svn_error_t * > +svn_io_run_external_diff(const char *dir, > + const char *label1, > + const char *label2, > + const char *tmpfile1, > + const char *tmpfile2, > + int *pexitcode, > + apr_file_t *outfile, > + apr_file_t *errfile, > + const char *external_diff_cmd, > + apr_pool_t *scratch_pool); > + > #ifdef __cplusplus > } > #endif /* __cplusplus */ > Index: subversion/libsvn_client/deprecated.c > =================================================================== > --- subversion/libsvn_client/diff.c (revision 1602604) > +++ subversion/libsvn_client/diff.c (working copy) > @@ -541,6 +541,9 @@ > /* If non-null, the external diff command to invoke. */ > const char *diff_cmd; > > + /* external custom diff command */ > + const char *invoke_diff_cmd; > + > /* This is allocated in this struct's pool or a higher-up pool. */ > union { > /* If 'diff_cmd' is null, then this is the parsed options to > @@ -777,8 +780,12 @@ > return SVN_NO_ERROR; > } > > + if (dwi->diff_cmd && dwi->invoke_diff_cmd) > + return svn_error_create(SVN_ERR_CLIENT_DIFF_CMD, NULL, > + _("diff-cmd and invoke-diff-cmd are " > + "mutually exclusive.")); > > - if (dwi->diff_cmd) > + if (dwi->diff_cmd || dwi->invoke_diff_cmd) > { > svn_stream_t *errstream = dwi->errstream; > apr_file_t *outfile; > @@ -810,7 +817,6 @@ > SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL, > svn_io_file_del_on_pool_cleanup, > scratch_pool, scratch_pool)); > - > errfile = svn_stream__aprfile(errstream); > if (errfile) > errfilename = NULL; > @@ -819,13 +825,24 @@ > svn_io_file_del_on_pool_cleanup, > scratch_pool, scratch_pool)); > > - SVN_ERR(svn_io_run_diff2(".", > - dwi->options.for_external.argv, > - dwi->options.for_external.argc, > - label1, label2, > - tmpfile1, tmpfile2, > - &exitcode, outfile, errfile, > - dwi->diff_cmd, scratch_pool)); > + /* "." is a non-canonical path for the diff process's working > directory. */ > + if (dwi->diff_cmd) > + SVN_ERR(svn_io_run_diff2(".", > + dwi->options.for_external.argv, > + dwi->options.for_external.argc, > + label1, label2, > + tmpfile1, tmpfile2, > + &exitcode, outfile, errfile, > + dwi->diff_cmd, scratch_pool)); > + else > + { > + SVN_ERR(svn_io_run_external_diff(".", So, here, I'd suggest that you'd invoke a static helper function which wraps svn_io_run_cmd(). Then you could get rid off the new svn_io_* API which I don't think we'll really need as an API. > + label1, label2, > + tmpfile1, tmpfile2, > + &exitcode, outfile, errfile, > + dwi->invoke_diff_cmd, > + scratch_pool)); > + } > > /* Now, open and copy our files to our output streams. */ > if (outfilename) > @@ -2228,7 +2245,8 @@ > { > const char *diff_cmd = NULL; > > - /* See if there is a diff command and/or diff arguments. */ > + /* See if there is a diff-cmd command and/or diff arguments first on > + the command line and then in .subversion/config */ > if (config) > { > svn_config_t *cfg = svn_hash_gets(config, SVN_CONFIG_CATEGORY_CONFIG); > @@ -2249,37 +2267,53 @@ > options = apr_array_make(result_pool, 0, sizeof(const char *)); > > if (diff_cmd) > - SVN_ERR(svn_path_cstring_to_utf8(&dwi->diff_cmd, diff_cmd, > - result_pool)); > - else > - dwi->diff_cmd = NULL; > - > - /* If there was a command, arrange options to pass to it. */ > - if (dwi->diff_cmd) > { > - const char **argv = NULL; > - int argc = options->nelts; > - if (argc) > - { > - int i; > - argv = apr_palloc(result_pool, argc * sizeof(char *)); > - for (i = 0; i < argc; i++) > - SVN_ERR(svn_utf_cstring_to_utf8(&argv[i], > - APR_ARRAY_IDX(options, i, const char *), result_pool)); > - } > - dwi->options.for_external.argv = argv; > - dwi->options.for_external.argc = argc; > + SVN_ERR(svn_path_cstring_to_utf8(&dwi->diff_cmd, diff_cmd, > + result_pool)); > + /* If there was a command, arrange options to pass to it. */ > + { > + const char **argv = NULL; > + int argc = options->nelts; > + if (argc) > + { > + int i; > + argv = apr_palloc(result_pool, argc * sizeof(char *)); > + for (i = 0; i < argc; i++) > + SVN_ERR(svn_utf_cstring_to_utf8(&argv[i], > + APR_ARRAY_IDX(options, i, > + const char *), > + result_pool)); > + } > + dwi->options.for_external.argv = argv; > + dwi->options.for_external.argc = argc; The above code uses tabs for indentation but it should be using spaces. > + } > } > - else /* No command, so arrange options for internal invocation instead. */ > + else > + /* See if there is a diff-cmd command and/or diff arguments first on > + the command line and then in .subversion/config */ > { > - dwi->options.for_internal = svn_diff_file_options_create(result_pool); > - SVN_ERR(svn_diff_file_options_parse(dwi->options.for_internal, > - options, result_pool)); > + svn_config_t *cfg = svn_hash_gets(config, SVN_CONFIG_CATEGORY_CONFIG); > + svn_config_get(cfg, &diff_cmd, SVN_CONFIG_SECTION_HELPERS, > + SVN_CONFIG_OPTION_INVOKE_DIFF_CMD, NULL); > + > + if (diff_cmd) > + { > + SVN_ERR(svn_path_cstring_to_utf8(&dwi->invoke_diff_cmd, > + diff_cmd, > + result_pool)); > + } > + else > + { > + /* No command, so arrange options for internal invocation instead. */ > + dwi->invoke_diff_cmd = NULL; > + dwi->diff_cmd = NULL; > + dwi->options.for_internal = svn_diff_file_options_create(result_pool); > + SVN_ERR(svn_diff_file_options_parse(dwi->options.for_internal, > + options, result_pool)); > + } More tabs above. > } > - > return SVN_NO_ERROR; > } > - > /*----------------------------------------------------------------------- */ > > /*** Public Interfaces. ***/ > Index: subversion/libsvn_subr/config_file.c > =================================================================== > --- subversion/libsvn_subr/config_file.c (revision 1602604) > +++ subversion/libsvn_subr/config_file.c (working copy) > @@ -1218,6 +1218,11 @@ > "### Set diff3-has-program-arg to 'yes' if your 'diff3' program" > NL > "### accepts the '--diff-program' option." > NL > "# diff3-has-program-arg = [yes | no]" > NL > + "### Set invoke-diff-cmd to the absolute path of your 'diff'" > NL > + "### program." > NL > + "### This will override the compile-time default, which is to use" > NL > + "### Subversion's internal diff implementation." > NL > + "# invoke-diff-cmd = (see svn help diff for examples)" > NL > "### Set merge-tool-cmd to the command used to invoke your external" > NL > "### merging tool of choice. Subversion will pass 5 arguments to" > NL > "### the specified command: base theirs mine merged wcfile" > NL > Index: subversion/libsvn_subr/io.c > =================================================================== > --- subversion/libsvn_subr/io.c (revision 1602604) > +++ subversion/libsvn_subr/io.c (working copy) > @@ -3004,8 +3004,166 @@ > 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_invoke_external_diffcmd > + */ > > + apr_array_header_t *words; > + const char ** result; > + size_t argv, item, i, delimiters = 6; > + 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); This code is splitting by whitespace and as a result has to deal with quoted substrings which were broken up inappropriately. I guess it would be easier to parse the input one byte at a time, copying bytes to the output string unless a substitution is found (signalled by '%' not followed by '%'). Something like this: char *c = cmd; while (*c) { if (c[0] == '%' && c[1] == '%') { /* output literal percent */ c++; } else if (c[0] == '%') { /* perform substitution and output result, return error if invalid substitution label is found; try to forward c beyond substitution label and be careful not to read beyond '\0' */ } else { /* output byte literally */ } c++; } That's all for now. > + > + result = apr_palloc(pool, > + (words->nelts+1) * words->elt_size * sizeof(char *) ); > + > + for (item = 0, argv = 0; item < words->nelts; argv++, item++) > + { > + svn_stringbuf_t *word; > + > + word = svn_stringbuf_create_empty(scratch_pool); > + svn_stringbuf_appendcstr(word, APR_ARRAY_IDX(words, item, char *)); > + > + if ( (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++; > + } > + } > + } > + i = 0; > + 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_external_diff(const char *dir, > + const char *label1, > + const char *label2, > + const char *tmpfile1, > + const char *tmpfile2, > + int *pexitcode, > + apr_file_t *outfile, > + apr_file_t *errfile, > + const char *external_diff_cmd, > + apr_pool_t *pool) > +{ > + int exitcode; > + const char ** cmd; > + > + apr_pool_t *scratch_pool = svn_pool_create(pool); > + > + if (0 == strlen(external_diff_cmd)) > + return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL, NULL); > + > + cmd = svn_io__create_custom_diff_cmd(label1, label2, NULL, > + tmpfile1, tmpfile2, NULL, > + external_diff_cmd, scratch_pool); > + if (pexitcode == NULL) > + pexitcode = &exitcode; > + > + 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." > + > + 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 > + error for anything other than 0 or 1 as the output is likely to be > + corrupt. > + */ > + if (*pexitcode != 0 && *pexitcode != 1) > + { > + int i; > + const char *failed_command = ""; > + > + for (i = 0; cmd[i]; ++i) > + failed_command = apr_pstrcat(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"), > + external_diff_cmd, > + failed_command, > + *pexitcode); > + } > + else > + svn_pool_destroy(scratch_pool); > + return SVN_NO_ERROR; > +} > + > +svn_error_t * > svn_io_run_diff2(const char *dir, > const char *const *user_args, > int num_user_args, > Index: subversion/svn/cl-log.h > =================================================================== > --- subversion/svn/cl-log.h (revision 1602604) > +++ subversion/svn/cl-log.h (working copy) > @@ -60,6 +60,9 @@ > /* Depth applied to diff output. */ > svn_depth_t depth; > > + /* invoke-diff-cmd arguments received from command line. */ > + const char *invoke_diff_cmd; > + > /* Diff arguments received from command line. */ > const char *diff_extensions; > > Index: subversion/svn/cl.h > =================================================================== > --- subversion/svn/cl.h (revision 1602604) > +++ subversion/svn/cl.h (working copy) > @@ -184,6 +184,8 @@ > { > const char *diff_cmd; /* the external diff command to use > (not converted to UTF-8) */ > + const char *invoke_diff_cmd; /* the format string to specify args > */ > + /* for the external diff cmd > */ > svn_boolean_t internal_diff; /* override diff_cmd in config file */ > svn_boolean_t no_diff_added; /* do not show diffs for deleted files > */ > svn_boolean_t no_diff_deleted; /* do not show diffs for deleted files > */ > Index: subversion/svn/log-cmd.c > =================================================================== > --- subversion/svn/log-cmd.c (revision 1602604) > +++ subversion/svn/log-cmd.c (working copy) > @@ -704,6 +707,10 @@ > "XML mode")); > } > > + if (opt_state->diff.diff_cmd && opt_state->diff.diff_cmd) > + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, > + _("'diff-cmd' and 'invoke-diff-cmd' options are " > + "mutually exclusive")); > if (opt_state->quiet && opt_state->show_diff) > return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, > _("'quiet' and 'diff' options are " > @@ -712,6 +719,10 @@ > return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, > _("'diff-cmd' option requires 'diff' " > "option")); > + if (opt_state->diff.invoke_diff_cmd && (! opt_state->show_diff)) > + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, > + _("'invoke-diff-cmd' option requires 'diff' " > + "option")); > if (opt_state->diff.internal_diff && (! opt_state->show_diff)) > return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, > _("'internal-diff' option requires " > Index: subversion/svn/svn.c > =================================================================== > --- subversion/svn/svn.c (revision 1602604) > +++ subversion/svn/svn.c (working copy) > @@ -85,6 +85,7 @@ > opt_ignore_properties, > opt_properties_only, > opt_patch_compatible, > + opt_invoke_diff_cmd, > /* end of diff options */ > opt_dry_run, > opt_editor_cmd, > @@ -347,6 +348,34 @@ > {"diff", opt_diff, 0, N_("produce diff output")}, /* maps to show_diff */ > /* diff options */ > {"diff-cmd", opt_diff_cmd, 1, N_("use ARG as diff command")}, > + {"invoke-diff-cmd", opt_invoke_diff_cmd, 1, > + N_("use ARG as format string for external diff command\n" > + " " > + "invocation.\n" > + " " > + "The following reserved keywords are replaced:\n" > + " " > + " %svn_new -- new file\n" > + " " > + " %svn_old -- old file\n" > + " " > + " %svn_label_new -- label of the new file\n" > + " " > + " %svn_label_old -- label of the old file\n" > + " " > + "Examples:\n" > + " " > + "--invoke-diff-cmd=\'diff -y %svn_new %svn_old\'\n" > + " " > + "--invoke-diff-cmd=\"kdiff3 -auto -o /home/u/log \\\n" > + " " > + " %svn_new %svn_old --L1 %svn_label_new \\\n" > + " " > + " --L2 \"Custom Label\" \'\n" > + " " > + "Reserved keywords may be embedded in strings:\n" > + " " > + "+%svn_new %svn_new- and file=%svn_label_new+")}, > {"internal-diff", opt_internal_diff, 0, > N_("override diff-cmd specified in config file")}, > {"no-diff-added", opt_no_diff_added, 0, > @@ -640,7 +669,8 @@ > opt_internal_diff, 'x', opt_no_diff_added, opt_no_diff_deleted, > opt_ignore_properties, opt_properties_only, > opt_show_copies_as_adds, opt_notice_ancestry, opt_summarize, > opt_changelist, > - opt_force, opt_xml, opt_use_git_diff_format, opt_patch_compatible} }, > + opt_force, opt_xml, opt_use_git_diff_format, opt_patch_compatible, > + opt_invoke_diff_cmd} }, > { "export", svn_cl__export, {0}, N_ > ("Create an unversioned copy of a tree.\n" > "usage: 1. export [-r REV] URL[@PEGREV] [PATH]\n" > @@ -805,7 +835,7 @@ > {'r', 'q', 'v', 'g', 'c', opt_targets, opt_stop_on_copy, opt_incremental, > opt_xml, 'l', opt_with_all_revprops, opt_with_no_revprops, > opt_with_revprop, opt_auto_moves, opt_depth, opt_diff, opt_diff_cmd, > - opt_internal_diff, 'x', opt_search, opt_search_and }, > + opt_internal_diff, 'x', opt_invoke_diff_cmd, opt_search, opt_search_and > }, > {{opt_with_revprop, N_("retrieve revision property ARG")}, > {'c', N_("the change made in revision ARG")}} }, > > @@ -2155,6 +2185,9 @@ > case opt_diff_cmd: > opt_state.diff.diff_cmd = apr_pstrdup(pool, opt_arg); > break; > + case opt_invoke_diff_cmd: > + opt_state.diff.invoke_diff_cmd = apr_pstrdup(pool, opt_arg); > + break; > case opt_merge_cmd: > opt_state.merge_cmd = apr_pstrdup(pool, opt_arg); > break; > @@ -2559,7 +2592,7 @@ > "--non-interactive")); > } > > - /* Disallow simultaneous use of both --diff-cmd and > + /* Disallow simultaneous use of --diff-cmd, --invoke-diff-cmd and > --internal-diff. */ > if (opt_state.diff.diff_cmd && opt_state.diff.internal_diff) > { > @@ -2568,6 +2601,20 @@ > "are mutually exclusive")); > } > > + if (opt_state.diff.invoke_diff_cmd && opt_state.diff.internal_diff) > + { > + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, > + _("--invoke-diff-cmd and --internal-diff " > + "are mutually exclusive")); > + } > + > + if ((opt_state.diff.diff_cmd) && (opt_state.diff.invoke_diff_cmd)) > + { > + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, > + _("--invoke-diff-cmd and --diff-cmd " > + "are mutually exclusive")); > + } > + > /* Ensure that 'revision_ranges' has at least one item, and make > 'start_revision' and 'end_revision' match that item. */ > if (opt_state.revision_ranges->nelts == 0) > @@ -2778,6 +2825,9 @@ > if (opt_state.diff.diff_cmd) > svn_config_set(cfg_config, SVN_CONFIG_SECTION_HELPERS, > SVN_CONFIG_OPTION_DIFF_CMD, opt_state.diff.diff_cmd); > + if (opt_state.diff.invoke_diff_cmd) > + svn_config_set(cfg_config, SVN_CONFIG_SECTION_HELPERS, > + SVN_CONFIG_OPTION_INVOKE_DIFF_CMD, > opt_state.diff.invoke_diff_cmd); > if (opt_state.merge_cmd) > svn_config_set(cfg_config, SVN_CONFIG_SECTION_HELPERS, > SVN_CONFIG_OPTION_DIFF3_CMD, opt_state.merge_cmd); > Index: subversion/svnlook/svnlook.c > =================================================================== > --- subversion/svnlook/svnlook.c (revision 1602604) > +++ subversion/svnlook/svnlook.c (working copy) > @@ -102,6 +102,7 @@ > svnlook__ignore_properties, > svnlook__properties_only, > svnlook__diff_cmd, > + svnlook__invoke_diff_cmd, > svnlook__show_inherited_props, > svnlook__no_newline > }; > @@ -138,6 +139,9 @@ > {"diff-cmd", svnlook__diff_cmd, 1, > N_("use ARG as diff command")}, > > + {"invoke-diff-cmd", svnlook__invoke_diff_cmd, 1, > + N_("Customizable diff command (see svn help diff)")}, > + > {"ignore-properties", svnlook__ignore_properties, 0, > N_("ignore properties during the operation")}, > > @@ -230,7 +234,8 @@ > "Print GNU-style diffs of changed files and properties.\n"), > {'r', 't', svnlook__no_diff_deleted, svnlook__no_diff_added, > svnlook__diff_copy_from, svnlook__diff_cmd, 'x', > - svnlook__ignore_properties, svnlook__properties_only} }, > + svnlook__ignore_properties, svnlook__properties_only, > + svnlook__invoke_diff_cmd} }, > > {"dirs-changed", subcommand_dirschanged, {0}, > N_("usage: svnlook dirs-changed REPOS_PATH\n\n" > @@ -335,7 +340,8 @@ > svn_boolean_t quiet; /* --quiet */ > svn_boolean_t ignore_properties; /* --ignore_properties */ > svn_boolean_t properties_only; /* --properties-only */ > - const char *diff_cmd; /* --diff-cmd */ > + const char *diff_cmd; /* --diff-cmd */ > + const char *invoke_diff_cmd; /* --invoke-diff-cmd */ > svn_boolean_t show_inherited_props; /* --show-inherited-props */ > svn_boolean_t no_newline; /* --no-newline */ > }; > @@ -360,6 +366,7 @@ > svn_boolean_t ignore_properties; > svn_boolean_t properties_only; > const char *diff_cmd; > + const char *invoke_diff_cmd; > > } svnlook_ctxt_t; > > @@ -951,7 +958,7 @@ > } > else > { > - if (c->diff_cmd) > + if (c->diff_cmd || c->invoke_diff_cmd) > { > apr_file_t *outfile; > apr_file_t *errfile; > @@ -1006,14 +1013,31 @@ > SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, > NULL, > svn_io_file_del_on_pool_cleanup, pool, pool)); > > - SVN_ERR(svn_io_run_diff2(".", > - diff_cmd_argv, > - diff_cmd_argc, > - orig_label, new_label, > - orig_path, new_path, > - &exitcode, outfile, errfile, > - c->diff_cmd, pool)); > + if (c->diff_cmd) > + SVN_ERR(svn_io_run_diff2(".", > + diff_cmd_argv, > + diff_cmd_argc, > + orig_label, new_label, > + orig_path, new_path, > + &exitcode, outfile, errfile, > + c->diff_cmd, pool)); > > + else if (c->invoke_diff_cmd) > + SVN_ERR(svn_io_run_external_diff(".", > + orig_label, > + new_label, > + orig_path, > + new_path, > + &exitcode, > + outfile, > + errfile, > + c->invoke_diff_cmd, > + pool)); > + > + SVN_ERR(svn_io_file_close(outfile, pool)); > + SVN_ERR(svn_io_file_close(errfile, pool)); > + > + > /* Now, open and copy our files to our output streams. */ > if (outfilename) > { > @@ -2100,6 +2124,7 @@ > " \t\n\r", TRUE, pool); > baton->ignore_properties = opt_state->ignore_properties; > baton->properties_only = opt_state->properties_only; > + baton->invoke_diff_cmd = opt_state->invoke_diff_cmd; > baton->diff_cmd = opt_state->diff_cmd; > > if (baton->txn_name) > @@ -2514,7 +2539,6 @@ > _("Invalid revision number supplied")); > } > break; > - > case 't': > opt_state.txn = opt_arg; > break; > @@ -2605,6 +2629,10 @@ > opt_state.diff_cmd = opt_arg; > break; > > + case svnlook__invoke_diff_cmd: > + opt_state.invoke_diff_cmd = opt_arg; > + break; > + > case svnlook__show_inherited_props: > opt_state.show_inherited_props = TRUE; > break; > @@ -2635,6 +2663,13 @@ > _("Cannot use the '--show-inherited-props' option with the " > "'--revprop' option")); > > + /* The --diff-cmd and --invoke-diff-cmd options may not co-exist. */ > + if (opt_state.diff_cmd && opt_state.invoke_diff_cmd) > + SVN_INT_ERR(svn_error_create > + (SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL, > + _("Cannot use the '--diff-cmd' option with the " > + "'--invoke-diff-cmd' option"))); > + > /* If the user asked for help, then the rest of the arguments are > the names of subcommands to get help on (if any), or else they're > just typos/mistakes. Whatever the case, the subcommand to > Index: subversion/tests/cmdline/diff_tests.py > =================================================================== > --- subversion/tests/cmdline/diff_tests.py (revision 1602604) > +++ subversion/tests/cmdline/diff_tests.py (working copy) > @@ -3065,6 +3065,7 @@ > 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 > def diff_external_diffcmd(sbox): > "svn diff --diff-cmd provides the correct arguments" > @@ -3102,7 +3103,54 @@ > 'diff', '--diff-cmd', diff_script_path, > iota_path) > > +# Check the correct parsing of arguments for an external diff tool > +def diff_invoke_external_diffcmd(sbox): > + "svn diff --invoke-diff-cmd passes correct args" > > + diff_script_path = os.path.abspath(".")+"/diff" Are you sure a forward slash is correct here? This test needs to be able to run on windows. os.path.join() is usually the right function to use when constructing paths. > + > + 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 > + > + sbox.build(read_only = True) > + os.chdir(sbox.wc_dir) > + > + iota_path = 'iota' > + svntest.main.file_append(iota_path, "new text in iota") > + > + expected_output = svntest.verify.ExpectedOutput([ > + "Index: iota\n", > + > "===================================================================\n", > + # correct label %svn_label_old -> label 1 > + "iota (revision 1)\n", > + > + # correct file %svn_old -> old > + os.path.abspath(svntest.wc.text_base_path("iota")) + "\n", > + > + # correct label %svn_label_new -> label 2 > + "iota (working copy)\n", > + > + # correct file %svn_new -> new > + os.path.abspath("iota") + "\n", > + > + # preservation of quoted string "X Y Z"-> "X Y Z" > + "\"X Y Z\"\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', > + '--invoke-diff-cmd='+diff_script_path+ > + ' %svn_label_old %svn_old %svn_label_new %svn_new \"X Y Z\" +%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 +4778,7 @@ > diff_file_depth_empty, > diff_wrong_extension_type, > diff_external_diffcmd, > + diff_invoke_external_diffcmd, > diff_url_against_local_mods, > diff_preexisting_rev_against_local_add, > diff_git_format_wc_wc, > Index: subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout > =================================================================== > --- subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout > (revision 1602604) > +++ subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout > (working copy) > @@ -111,6 +111,20 @@ > -w, --ignore-all-space: Ignore all white space > --ignore-eol-style: Ignore changes in EOL > style > -p, --show-c-function: Show C function name > + --invoke-diff-cmd ARG : use ARG as format string for external diff > command > + invocation. > + The following reserved keywords are replaced: > + %svn_new -- new file > + %svn_old -- old file > + %svn_label_new -- label of the new file > + %svn_label_old -- label of the old file > + Examples: > + --invoke-diff-cmd='diff -y %svn_new %svn_old' > + --invoke-diff-cmd="kdiff3 -auto -o /home/u/log \ > + %svn_new %svn_old --L1 %svn_label_new \ > + --L2 "Custom Label" ' > + Reserved keywords may be embedded in strings: > + +%svn_new %svn_new- and file=%svn_label_new+ > --search ARG : use ARG as search pattern (glob syntax) > --search-and ARG : combine ARG with the previous search pattern >
