Review in diff form:
Index: subversion/include/svn_client.h =================================================================== --- subversion/include/svn_client.h (revision 1484305) +++ subversion/include/svn_client.h (working copy) @@ -2988,8 +2988,8 @@ svn_client_blame(const char *path_or_url, * * @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. + * invoke-diff-cmd invocation entry(if any) in @c ctx->config. + * ### "May not be NULL" !? log-cmd.c and deprecated.c pass NULL for it. * * Generated headers are encoded using @a header_encoding. * @@ -3047,7 +3047,8 @@ svn_client_diff7(const apr_array_header_t *options svn_client_ctx_t *ctx, apr_pool_t *pool); -/** Similar to svn_client_diff7(), but with @a invoke_diff_cmd. +/** Similar to svn_client_diff7(), but with @a invoke_diff_cmd + * set to @c NULL. * * @deprecated Provided for backward compatibility with the 1.8 API. * @since New in 1.8. @@ -3245,6 +3246,7 @@ svn_client_diff_peg7(const apr_array_header_t *dif /** Similar to svn_client_peg5(), but with @a no_diff_added set to * FALSE, @a ignore_properties set to FALSE and @a properties_only * set to false. + * ### copy-pasto? ^^ * * @deprecated Provided for backward compatibility with the 1.7 API. * @since New in 1.9. Index: subversion/include/svn_io.h =================================================================== --- subversion/include/svn_io.h (revision 1484305) +++ subversion/include/svn_io.h (working copy) @@ -2281,6 +2281,8 @@ svn_io_file_readline(apr_file_t *file, /** Parse a user defined command to contain dynamically created labels * and filenames. * + * @todo Document the parameters and return value. + * * @since New in 1.9. */ const char ** @@ -2295,6 +2297,8 @@ svn_io_create_custom_diff_cmd(const char *label1, /** Run the external diff command defined by the invoke-diff-cmd * option. + * + * @todo Document the parameters and return value. * * @since New in 1.9. */ Index: subversion/libsvn_client/diff.c =================================================================== --- subversion/libsvn_client/diff.c (revision 1484305) +++ subversion/libsvn_client/diff.c (working copy) @@ -790,6 +790,7 @@ diff_content_changed(svn_boolean_t *wrote_header, } + /* ### Should this code error if both are set? */ if (diff_cmd_baton->diff_cmd || diff_cmd_baton->invoke_diff_cmd) { apr_file_t *outfile; @@ -821,6 +822,10 @@ diff_content_changed(svn_boolean_t *wrote_header, scratch_pool, scratch_pool)); if (diff_cmd_baton->diff_cmd) + /* ### "." is not canonical, and the docstring doesn't bless passing + a non-canonical dirent. (See svn_dirent_canonicalize()) + Should this pass "" (canonicalized "."), or should svn_io.h + bless passing non-canonical dirents (at least in this case)? */ SVN_ERR(svn_io_run_diff2(".", diff_cmd_baton->options.for_external.argv, diff_cmd_baton->options.for_external.argc, @@ -2522,6 +2527,7 @@ set_up_diff_cmd_and_options(struct diff_cmd_baton argv = apr_palloc(pool, argc * sizeof(char *)); for (i = 0; i < argc; i++) SVN_ERR(svn_utf_cstring_to_utf8(&argv[i], + /* next line shouldn't have been un-indented. */ APR_ARRAY_IDX(options, i, const char *), pool)); } diff_cmd_baton->options.for_external.argv = argv; Index: subversion/libsvn_subr/config_file.c =================================================================== --- subversion/libsvn_subr/config_file.c (revision 1484305) +++ subversion/libsvn_subr/config_file.c (working copy) @@ -1086,6 +1086,9 @@ svn_config_ensure(const char *config_dir, apr_pool "# diff3-has-program-arg = [yes | no]" NL "### Set invoke-diff-cmd to the absolute path of your 'diff'" NL "### program." NL + /* ### Document the replaceables */ + /* ### Document how the setting may contain argv too, + not only argv[0] */ "### This will override the compile-time default, which is to use" NL "### Subversion's internal diff implementation." NL "# invoke-diff-cmd = \"diff -y --label %l1% %f1% --label %l2% %f2%\""NL Index: subversion/libsvn_subr/io.c =================================================================== --- subversion/libsvn_subr/io.c (revision 1484305) +++ subversion/libsvn_subr/io.c (working copy) @@ -2936,16 +2936,22 @@ svn_io_create_custom_diff_cmd(const char *label1, const char ** ret; int argv; + /* Just take two pool arguments instead? (result+scratch) */ subpool = svn_pool_create(pool); + /* ### space after comma */ + /* This reimplements "split a string into argv". Is there an existing + svn/apr function that does that can be reused here? (We might gain + an "escape spaces" functionality this way.) */ tmp = svn_cstring_split(cmd," ",TRUE, subpool); + /* ### s/calloc/alloc/; you initialize everything explicitly. */ ret = apr_pcalloc(pool, tmp->nelts * tmp->elt_size*sizeof(char *)); for (argv = 0; argv < tmp->nelts ; argv++) - { + { /* needs to be indented two spaces, not one */ svn_stringbuf_t *com; int i; @@ -2969,6 +2975,14 @@ svn_io_create_custom_diff_cmd(const char *label1, { len = strlen(found); + /* ### Looks dubious. Won't this access com->data[-1] when + !strcmp(com->data, token_list[0]) ? + How does this parse "%%%f1%"? Is "%%f1%%" an error? + (The answers to both of these questions should have been + decided prior to coding, and I recall a list thread but I + don't recall that thread reached consensus on a specific + escaping UI.) Has consensus on the UI implemented here + been reached? */ /* if we find a % in front of this, consume it */ if (com->data[com->len - strlen(found)-1] == '%') svn_stringbuf_remove(com, strlen(found)-1, 1); @@ -2993,8 +3007,8 @@ svn_io_create_custom_diff_cmd(const char *label1, } } ret[argv] = com->data; - } - ret[argv] = NULL; + } + ret[argv] = NULL; /* segfault. need (nelts + 1) in the allocation */ svn_pool_destroy(subpool); return ret; @@ -3018,6 +3032,7 @@ svn_io_run_external_diff(const char *dir, if (0 == strlen(external_diff_cmd)) return svn_error_createf(SVN_ERR_CL_INSUFFICIENT_ARGS, NULL, _("The --invoke-diff-cmd string was empty.\n")); + /* ### No newline in error messages */ cmd = svn_io_create_custom_diff_cmd(label1, label2, NULL, tmpfile1, tmpfile2, NULL, @@ -3025,6 +3040,7 @@ svn_io_run_external_diff(const char *dir, if (pexitcode == NULL) pexitcode = &exitcode; + /* if invoke_diff_cmd is "", cmd[0] would be NULL? */ SVN_ERR(svn_io_run_cmd(dir, cmd[0], cmd, pexitcode, NULL, TRUE, NULL, outfile, errfile, pool)); @@ -3036,10 +3052,10 @@ svn_io_run_external_diff(const char *dir, for (i = 0, size = 0; cmd[i]; i++) size += strlen(cmd[i]) + 1; - failed_command = apr_pcalloc(pool, size * sizeof(char *)); + failed_command = apr_palloc(pool, size * sizeof(char)); for (i = 0; cmd[i]; i++) - { + { /* wrong indent */ strcat(failed_command, cmd[i]); strcat(failed_command, " "); } @@ -3048,6 +3064,9 @@ svn_io_run_external_diff(const char *dir, _("'%s' was expanded to '%s' and returned %d\n"), external_diff_cmd, svn_dirent_local_style(failed_command, pool), + /* ### Huh? failed_comand is dirent, it's + a string those first " "-separated word + is a dirent. */ *pexitcode); } return SVN_NO_ERROR; Index: subversion/svn/log-cmd.c =================================================================== --- subversion/svn/log-cmd.c (revision 1484305) +++ subversion/svn/log-cmd.c (working copy) @@ -140,7 +140,7 @@ display_diff(const svn_log_entry_t *log_entry, outstream, errstream, NULL, - NULL, + NULL, /* ### why not pass invoke_diff_cmd? */ - ctx, pool)); + ctx, pool)); /* s/tabs/spaces/ */ SVN_ERR(svn_stream_puts(outstream, _("\n"))); return SVN_NO_ERROR; Index: subversion/svn/svn.c =================================================================== --- subversion/svn/svn.c (revision 1484305) +++ subversion/svn/svn.c (working copy) @@ -2751,8 +2751,12 @@ sub_main(int argc, const char *argv[], apr_pool_t svn_config_set(cfg_config, SVN_CONFIG_SECTION_HELPERS, SVN_CONFIG_OPTION_DIFF_CMD, opt_state.diff.diff_cmd); } - else + else { + /* ### Why not set this unconditionally and let the library figure out + which option takes precedence? Or, alternatively, error out + if --diff-cmd and --invoke-diff-cmd both specified (to avoid + silently ignoring the latter)? */ 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); Index: subversion/tests/cmdline/diff_tests.py =================================================================== --- subversion/tests/cmdline/diff_tests.py (revision 1484305) +++ subversion/tests/cmdline/diff_tests.py (working copy) @@ -3266,6 +3266,8 @@ def diff_invoke_external_diffcmd(sbox): diff_script_path = os.path.abspath(".")+"/diff" + # wrap to 80 columns, align second argument, + # consider printing repr() in the hook for easier parsing 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':