Hi, I hope I've resolved most issues, here are ones I need to ask about:
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. Hmm, what I was trying to communicate was that if a NULL(or "") is passed, this is an error that will be caught. I'm not quite sure what to write here now. The deprecated function is guaranteed to not have an execution path to invoke-diff-cmd but the log-cmd.c has been fixed. ---------- **** ------------ --- 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 */ (done) + /* ### 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 + /* ### Document how the setting may contain argv too, + not only argv[0] */ Not sure what you mean here? ---------- **** ------------ + /* 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); I didn't find one, but perhaps I missed it? ---------- **** ------------ + How does this parse "%%%f1%"? Is "%%f1%%" an error? %%%f1% becomes %%f1% and %%f1%% becomes %f1%%, neither is an error. However, %f1%% is parsed out as sub%, also, +%f1% ends up as +sub. What you cannot do is: %%f1% to get %sub, it will render as %f1%. If this is a show stopper, we could go back to the triple dash model and not deal with escaping %'s, or choose another delimiter. + (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? */ I'm not sure, what do others think? ---------- **** ------------ @@ -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)); I check for this condition before it reaches there. Neither "" nor NULL value for invoke_diff_cmd is accepted, but an error is raised before it reaches svn_io_create_custom_diff_cmd().