Gabriela Gibson wrote on Mon, Apr 29, 2013 at 21:31:09 +0100:
> Take two.
>
> Thanks for looking!
>
> Gabriela
>
> [[[
> Add new diff option "--invoke-diff-cmd" which allows the user to
> define a custom command line or config file entry for an external
> diff program.
>
> * subversion/include/svn_client.h
> (svn_client_diff6): Deprecate. Add new Doxygen comment.
> (svn_client_diff7): Add new Doxygen comment.
> (svn_client_diff7): New function.
"New doxygen comment" is too low-level; I think what you mean is "Add
docstring" (or "Document" (as a verb)), but that's an implicit part of
adding a new function; it's not interesting enough for a log message.
Counter-suggestion:
* foo.h
(foo6): Deprecate.
(foo7): Declare the new API. Like foo6 but with invoke_diff_cmd parameter.
> (svn_client_diff_peg_6): Deprecate. Refresh Doxygen comment.
> (svn_client_diff_peg_7): New function.
>
> * subversion/include/svn_config.h
> (SVN_CONFIG_OPTION_INVOKE_DIFF_CMD): New definition.
>
The '(' should have two, not three, preceding spaces.
> * subversion/include/svn_io.h
> (svn_io_create_custom_diff_cmd): New function.
> (svn_io_run_external_diff): New function.
>
> * subversion/libsvn_client/deprecated.c
> (svn_client_diff6): Deprecate.
> (svn_client_diff_peg6): Deprecate.
>
> * subversion/libsvn_client/diff.c
> (struct diff_cmd_baton): New variable.
>
s/variable/member/, and it would be good to mention it's name.
> (diff_content_changed): Call svn_io_run_external_diff if
> --invoke-diff-cmd option was specied, otherwise retain previous
> behaviour.
>
"specified". Personally, I normally indent follow-up lines with one or
two spaces in addition to the two that precede the '(' (so 3-4 spaces),
too.
> (set_up_diff_cmd_and_options): Apply invoke-diff-cmd option
> preferentially. Old behavior unchanged.
Not "behaviour"?
> * subversion/svn/diff-cmd.c
> (svn_cl__diff): Modify call to deprecated function.
>
That sentence sounds like you reshuffle the parameters in a call to
svn_client_diff6(). What you actually do is call svn_client_diff7()
instead. You might want to check svn logs to see what phrasing we
normally use? (The most common one is "Update callers", but that
one is used primarily changing a function's signature without revving
the function.)
> * subversion/svn/svn.c
> (svn_cl__options[]): New variable and help info.
> (svn_cl__cmd_table[]): New variable.
> (sub_main): Add exclusiveness condition. Expand if condition.
Julian already pointed out the following to you previously:
"Expand if condition" describes the shape of the source code change, not
the end (difference of functionality) it achieves.
> ]]]
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1475745)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2986,6 +2986,9 @@ svn_client_blame(const char *path_or_url,
> * The above two options are mutually exclusive. It is an error to set
> * both to TRUE.
> *
> + * If @a invoke_diff_cmd is non-null, invoke external diff command
> + * with the string it contains.
> + *
I don't find this sentence clear. The text supports 4 interpretations:
c = svn_config_get(..., "external-diff-cmd")
subprocess.check_call(invoke_diff_cmd, shell=True)
subprocess.check_call(c + ' ' + invoke_diff_cmd, shell=True)
subprocess.check_call([c, invoke_diff_cmd])
subprocess.check_call(c, shell=True,
stdin=svn_stream_from_stringbuf(invoke_diff_cmd))
Can you please be clearer in the docstring, which one (if any!) it is.
> * Generated headers are encoded using @a header_encoding.
> *
> * Diff output will not be generated for binary files, unless @a
> @@ -3015,9 +3018,39 @@ svn_client_blame(const char *path_or_url,
> *
> * @note @a relative_to_dir doesn't affect the path index generated by
> * external diff programs.
> + *
Indentation
> + * @since New in 1.8.
s/8/9/
> + */
> +svn_error_t *
> +svn_client_diff7(const apr_array_header_t *options,
> + const char *path_or_url1,
> + const svn_opt_revision_t *revision1,
> + const char *path_or_url2,
> + const svn_opt_revision_t *revision2,
> + const char *relative_to_dir,
> + svn_depth_t depth,
> + svn_boolean_t ignore_ancestry,
> + svn_boolean_t no_diff_added,
> + svn_boolean_t no_diff_deleted,
> + svn_boolean_t show_copies_as_adds,
> + svn_boolean_t ignore_content_type,
> + svn_boolean_t ignore_properties,
> + svn_boolean_t properties_only,
> + svn_boolean_t use_git_diff_format,
> + const char *header_encoding,
> + svn_stream_t *outstream,
> + svn_stream_t *errstream,
> + const apr_array_header_t *changelists,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool,
> + const char *invoke_diff_cmd);
"ctx" and "pool" are always last, I think. So please preserve that
convention, add "invoke_diff_cmd" earlier.
> Index: subversion/include/svn_config.h
> ===================================================================
> --- subversion/include/svn_config.h (revision 1475745)
> +++ subversion/include/svn_config.h (working copy)
> @@ -112,6 +112,7 @@ typedef struct svn_config_t svn_config_t;
> #define SVN_CONFIG_OPTION_DIFF_EXTENSIONS "diff-extensions"
> #define SVN_CONFIG_OPTION_DIFF3_CMD "diff3-cmd"
> #define SVN_CONFIG_OPTION_DIFF3_HAS_PROGRAM_ARG "diff3-has-program-arg"
/** @since New in 1.9. */
> +#define SVN_CONFIG_OPTION_INVOKE_DIFF_CMD "invoke-diff-cmd"
> #define SVN_CONFIG_OPTION_MERGE_TOOL_CMD "merge-tool-cmd"
> #define SVN_CONFIG_SECTION_MISCELLANY "miscellany"
> #define SVN_CONFIG_OPTION_GLOBAL_IGNORES "global-ignores"
> Index: subversion/include/svn_io.h
> ===================================================================
> --- subversion/include/svn_io.h (revision 1475745)
> +++ subversion/include/svn_io.h (working copy)
> @@ -2273,8 +2273,39 @@ svn_io_file_readline(apr_file_t *file,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
>
> +/* Parse a user defined command to contain dynamically
> + * created labels and filenames.
> + *
You need "/**" for doxygen.
> + ** @since New in 1.8. (?)
> + */
> /** @} */
> +const char **
> +svn_io_create_custom_diff_cmd(const char *label1,
> + const char *label2,
You haven't addressed my comment about not adding the docstring and
declarations on opposite sides of doxygen section-end markers ("@}").
> Index: subversion/libsvn_client/deprecated.c
> Index: subversion/libsvn_client/diff.c
> Index: subversion/libsvn_subr/io.c
I didn't review these parts.
> Index: subversion/libsvn_subr/config_file.c
> ===================================================================
> --- subversion/libsvn_subr/config_file.c (revision 1475745)
> +++ subversion/libsvn_subr/config_file.c (working copy)
> @@ -1084,6 +1084,12 @@ svn_config_ensure(const char *config_dir, apr_pool
> "### Set diff3-has-program-arg to 'yes' if your 'diff3' program"
> NL
> "### accepts the '--diff-program' option."
> NL
> "# diff3-has-program-arg = [yes | no]"
> NL
> + "### Set invoke-diff-cmd to the absolute path of your diff'"
> NL
> + "### program."
> NL
> + "### This will override the compile-time default, which is to use"
> NL
> + "### Subversion's internal diff implementation."
> NL
> + "### invoke-diff-cmd = "
> NL
> + "### diff_program (diff, gdiff, etc.) <options> <substitutions>"
> NL
s/###/#/ for consistency
> "### Set merge-tool-cmd to the command used to invoke your external"
> NL
> "### merging tool of choice. Subversion will pass 5 arguments to"
> NL
> "### the specified command: base theirs mine merged wcfile"
> NL
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 1475745)
> +++ subversion/svn/cl.h (working copy)
> @@ -181,6 +181,8 @@ typedef struct svn_cl__opt_state_t
> struct
> {
> const char *diff_cmd; /* the external diff command to use */
> + const char *invoke_diff_cmd; /* the format string to specify args
> */
> + /* for the external diff cmd
> */
> svn_boolean_t internal_diff; /* override diff_cmd in config file */
> svn_boolean_t no_diff_added; /* do not show diffs for deleted files
> */
> svn_boolean_t no_diff_deleted; /* do not show diffs for deleted files
> */
> @@ -213,6 +215,7 @@ typedef struct svn_cl__opt_state_t
> const char *changelist; /* operate on this changelist
> THIS IS TEMPORARY (LAST OF CHANGELISTS)
> */
> svn_boolean_t keep_changelists;/* don't remove changelists after commit */
> + const char *invoke_diff_cmd; /* external customizable diff command
> */
You didn't address (or disagree with) an earlier review point about not
naming a struct member and the same struct's nested struct's member the
same.
> svn_boolean_t keep_local; /* delete path only from repository */
> svn_boolean_t all_revprops; /* retrieve all revprops */
> svn_boolean_t no_revprops; /* retrieve no revprops */
> Index: subversion/svn/svn.c
> ===================================================================
> --- subversion/svn/svn.c (revision 1475745)
> +++ subversion/svn/svn.c (working copy)
> @@ -84,6 +84,7 @@ typedef enum svn_cl__longopt_t {
> opt_ignore_properties,
> opt_properties_only,
> opt_patch_compatible,
> + opt_invoke_diff_cmd,
> /* end of diff options */
> opt_dry_run,
> opt_editor_cmd,
> @@ -336,6 +337,55 @@ const apr_getopt_option_t svn_cl__options[] =
> {"diff", opt_diff, 0, N_("produce diff output")}, /* maps to show_diff */
> /* diff options */
> {"diff-cmd", opt_diff_cmd, 1, N_("use ARG as diff command")},
> +#ifdef WIN32
> + {"invoke-diff-cmd", opt_invoke_diff_cmd, 1,
> + N_("use ARG as format string for external diff command\n"
> + " "
> + "invocation. \n
> \n"
> + " "
> + "Substitutions: ---f1 ---f2 files to compare
> \n"
> + " "
> + " ---l1 ---l2 user defined labels
> \n"
> + " "
> + "Examples: --invoke-diff-cmd=\"diff -y ---f1 ---f2
> \n"
> + " --invoke-diff-cmd=\"kdiff3 -auto -o /home/u/log \\
> \n"
> + " "
> + " ---f1 ---f2 --L1 ---l1 --L2 \"Custom Label\"\"
> \n"
> + " "
> + "The switch symbol '%' can be modified by defining an
> \n"
> + " "
> + "alternative symbol string, starting with '#', '$' or
> '-'\n"
> + " "
> + "Example:
> \n"
> + " "
> + " --invoke-diff-cmd=\"%% diff -y %%f1 %%f2\"
> \n"
> + )},
> +#else
> + {"invoke-diff-cmd", opt_invoke_diff_cmd, 1,
> + N_("use ARG as format string for external diff command\n"
> + " "
> + "invocation. \n
> \n"
> + " "
> + "Substitutions: %f1 %f2 files to compare
> \n"
> + " "
> + " %l1 %l2 user defined labels
> \n"
> + " "
> + "Examples: --invoke-diff-cmd=\"diff -y %f1 %f2
> \n"
> + " "
> + " --invoke-diff-cmd=\"kdiff3 -auto -o /home/u/log \\
> \n"
> + " "
> + " %f1 %f2 --L1 %l1 --L2 \"Custom Label\" \"
> \n"
> + " "
> + "The switch symbol '%' can be modified by defining an
> \n"
> + " "
> + "alternative symbol string, starting with '#','$' or
> '-'\n"
> + " "
> + "Example:
> \n"
> + " "
> + " --invoke-diff-cmd=\"--- diff -y ---f1 ---f2\"
> \n"
> + )},
> +#endif /* WIN32 */
> +
Having a different UI for WIN32 and !WIN32 is a bad idea; uniformity is
better. Let's try and agree on a config file syntax first.
Requirements for that syntax:
- easy to read
- easy to parse
- allows specifying the replaceable multiple times
- allows escaping, i.e., allows specifying the replaceable literally
- easy to enter (this is meant to rule out '%' and its double-escaping hell)
- (other requirements?)
(stopping without suggesting a concrete format, pending an ongoing IRC
discussion)
> {"internal-diff", opt_internal_diff, 0,
> N_("override diff-cmd specified in config file")},
> {"no-diff-added", opt_no_diff_added, 0,
> @@ -2497,6 +2551,14 @@ sub_main(int argc, const char *argv[], apr_pool_t
> return EXIT_ERROR(err);
> }
>
> + if (opt_state.diff.invoke_diff_cmd && opt_state.diff.internal_diff)
> + {
> + err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("--invoke_diff_cmd and --internal-diff "
s/_/-/
> + "are mutually exclusive"));
> + return EXIT_ERROR(err);
> + }
