[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Review of invoke-diff-cmd-feature branch

From: Daniel Shahaf <danielsh_at_elego.de>
Date: Sun, 19 May 2013 20:23:13 +0300

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

This is an archived mail posted to the Subversion Dev mailing list.