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':
Received on 2013-05-19 19:24:28 CEST