Partial review, including only the public API changes, and only the diff
hunks (without their context not in the patch):
Gabriela Gibson wrote on Thu, Apr 11, 2013 at 22:39:24 +0100:
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1466727)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2978,6 +2978,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 extermal diff command
> + * with the string it contains.
> + *
Typo "extermal".
> * Generated headers are encoded using @a header_encoding.
> *
> * Diff output will not be generated for binary files, unless @a
> @@ -3007,9 +3010,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.
> + *
> + * @since New in 1.8.
> + */
> +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);
> +
> +/** Similar to svn_client_diff7(), but with @a invoke_diff_cmd.
"... set to NULL", presumably.
> *
> + * @deprecated Provided for backward compatibility with the 1.8 API.
> * @since New in 1.8.
> */
> +SVN_DEPRECATED
> svn_error_t *
> svn_client_diff6(const apr_array_header_t *diff_options,
Since this API is new in 1.8, you should just extend it without
deprecating it.
(We only deprecate released APIs. svn_client_diff5 is released and set
in stone (save for certain const-ness and pointed-to structs changes,
maybe); svn_client_diff6 has not been released and can be modified
arbitrarily. (but those changes should be added to the "SVN 1.8 APIs
review" wiki page))
> const char *path_or_url1,
> Index: subversion/include/svn_config.h
> ===================================================================
> --- subversion/include/svn_config.h (revision 1466727)
> +++ 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"
> +#define SVN_CONFIG_OPTION_INVOKE_DIFF_CMD "invoke-diff-cmd"
Don't you need a /* @since */ here?
> #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 1466727)
> +++ subversion/include/svn_io.h (working copy)
> @@ -2260,8 +2260,40 @@ 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.
> + *
> + ** @since New in 1.8. (?)
Yes. Also, please document the return value (NULL-terminated argv array?).
> + */
> /** @} */
Oops! This is a doxygen block end marker. (Look for the matching
'@{'.) You should have the docstring and declaration on the same side of
the block-end-marker, and shouldn't replicate the block-end-marker in
the svn_io_run_external_diff() declaration.
> +const char **
> +svn_io_create_custom_diff_cmd(const char *label1,
> + const char *label2,
> + const char *label3,
> + const char *tmpfile1,
> + const char *tmpfile2,
> + const char *base,
> + const char *cmd,
> + apr_pool_t *pool);
Convention in new code is scratch_pool/result_pool. Shouldn't you be
following that convention?
(Maybe it's for consistency with surrounding declarations? Haven't
checked, sorry.)
>
> +/* Run the external diff command defined by the invoke-diff-cmd
> + * option.
> + *
Again, need to document the parameters (or point to another function
having a similar signature where they are documented).
> + ** @since New in 1.8.
> + */
> +/** @} */
(see above)
> +svn_error_t *
> +svn_io_run_external_diff(const char *dir,
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1466727)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -2832,7 +2832,6 @@ svn_io_run_cmd(const char *path,
> return svn_io_wait_for_cmd(&cmd_proc, cmd, exitcode, exitwhy, pool);
> }
>
> -
> svn_error_t *
> svn_io_run_diff2(const char *dir,
> const char *const *user_args,
Superfluous whitespace change
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 1466727)
> +++ 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
> */
Having a struct-member and (nested struct)-member having the same name
is a bad idea (confusion is likely). Why not rename (or remove) one of them?
> 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 */
