subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gabriela Gibson <>
Subject Re: Review of invoke-diff-cmd-feature branch
Date Tue, 04 Jun 2013 08:56:57 GMT

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


+ /* ### 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().

View raw message