You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gabriela Gibson <ga...@gmail.com> on 2014/06/16 16:21:05 UTC

[PATCH] --invoke-diff-cmd

Hi,

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.

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.


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.


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.


* 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.

   (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.

]]]

Re: [PATCH] --invoke-diff-cmd

Posted by Stefan Sperling <st...@elego.de>.
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
>