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
>
Received on 2014-06-18 02:33:38 CEST