Hi Bert. I like what you're doing here. Here's a quick review.
> URL: http://svn.apache.org/r1569697
> Log:
> Following up on r1569551, update the libsvn_client 'svn diff' code to
> create adiff processor in one place and directly pass that to all the
> different diff implementations.
>
> Move handling of all arguments that are only used for tree transformations
> to libsvn_client (and the deprecated apis).
>
> * subversion/include/private/svn_client_private.h
> (includes): Add private/svn_diff_tree.h.
>
> * subversion/include/private/svn_wc_private.h
> (svn_wc__get_diff_editor): Update argument type.
>
> * subversion/libsvn_client/diff.c
> (diff_wc_wc): Update caller. Use anchor output argument.
> (diff_repos_repos): Update argument type. Switch argument order.
> (diff_repos_wc): Update caller.
> (do_diff): Create diff processor. Update caller.
>
> (diff_summarize_repos_wc,
> diff_summarize_wc_wc,
> do_diff_summarize): Update caller.
>
> * subversion/libsvn_client/diff_local.c
> (svn_client__arbitrary_nodes_diff): Use diff processor argument. Pass anchor
> to interested callers.
>
> * subversion/libsvn_wc/deprecated.c
> (svn_wc_get_diff_editor6): Update caller.
>
> * subversion/libsvn_wc/diff_editor.c
> (make_edit_baton,
> svn_wc__get_diff_editor): Update argument type. Remove now unused arguments.
>
> * subversion/libsvn_wc/diff_local.c
> (diff_baton): Remove unused variable.
> (svn_wc_diff6): Rename to...
> (svn_wc__diff7): ... this. Update argument type. Remove now unused arguments.
> Allow providing anchor as output argument.
> (svn_wc_diff6): Reimplement as new function wrapping svn_wc__diff7.
> Modified: subversion/trunk/subversion/include/private/svn_client_private.h
> ==============================================================================
> /* Produce a diff with depth DEPTH between two files or two directories at
> - * LOCAL_ABSPATH1 and LOCAL_ABSPATH2, using the provided diff callbacks to
> + * LEFT_ABSPATH1 and RIGHT_ABSPATH, using the provided diff callbacks to
s/LEFT_ABSPATH1/LEFT_ABSPATH/
> * show changes in files. The files and directories involved may be part of
> * a working copy or they may be unversioned. For versioned files, show
> - * property changes, too. */
> + * property changes, too.
> + *
> + * If ANCHOR_ABSPATH is not null, set it to the anchor of the diff before
> + * the first processor call. (The anchor is LEFT_ABSPATH or an ancestor of it)
What is the meaning or purpose of this "anchor" path?
> + */
> svn_error_t *
> -svn_client__arbitrary_nodes_diff(const char *local_abspath1,
> - const char *local_abspath2,
> +svn_client__arbitrary_nodes_diff(const char **anchor_abspath,
> + const char *left_abspath,
> + const char *right_abspath,
> svn_depth_t depth,
> - const svn_wc_diff_callbacks4_t *callbacks,
> - void *callback_baton,
> + const svn_diff_tree_processor_t *diff_processor,
> svn_client_ctx_t *ctx,
> + apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
>
> Modified: subversion/trunk/subversion/include/private/svn_wc_private.h
> ==============================================================================
> @@ -1559,7 +1559,7 @@
> *
> - * @a callbacks/@a callback_baton is the callback table to use.
> + * @a diff_processor will retrieve the diff report.
> *
> @@ -1630,14 +1630,11 @@ svn_wc__get_diff_editor(const svn_delta_
> const char *target,
> svn_depth_t depth,
> svn_boolean_t ignore_ancestry,
> - svn_boolean_t show_copies_as_adds,
> - svn_boolean_t use_git_diff_format,
Remove references to these params from the doc string.
> svn_boolean_t use_text_base,
> svn_boolean_t reverse_order,
The 'reverse' feature is mostly handled by the diff processor now. Is the parameter still needed here for internal purposes, not yet completely isolated?
Maybe the parameter needs a new name now that it doesn't actually reverse the diff but only makes a small change to the operation. Looking at the implementation, it now goes into a baton field named 'local_before_remote', so maybe its name and description should be changed to that?
> svn_boolean_t server_performs_filtering,
> const apr_array_header_t *changelist_filter,
> - const svn_wc_diff_callbacks4_t *callbacks,
> - void *callback_baton,
> + const svn_diff_tree_processor_t *diff_processor,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> apr_pool_t *result_pool,
> @@ -1842,6 +1839,25 @@
> +
> +/* The implemementation of svn_wc_diff6(), but reporting to a diff processor
Too many 'em's :-)
> + *
> + * If ANCHOR_ABSPATH is not null, set it to the anchor of the diff before
> + * the first processor call. (The anchor is LOCAL_ABSPATH or an ancestor of it)
> + */
> +svn_error_t *
> +svn_wc__diff7(const char **anchor_abspath,
> + svn_wc_context_t *wc_ctx,
> + const char *local_abspath,
> + svn_depth_t depth,
> + svn_boolean_t ignore_ancestry,
> + const apr_array_header_t *changelist_filter,
> + const svn_diff_tree_processor_t *diff_processor,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
>
> Modified: subversion/trunk/subversion/libsvn_client/diff.c
> ==============================================================================
[...]
> @@ -2103,21 +2123,27 @@ do_diff(const svn_wc_diff_callbacks4_t *
> {
> svn_boolean_t is_repos1;
> svn_boolean_t is_repos2;
> + svn_diff_tree_processor_t *diff_processor;
>
> /* Check if paths/revisions are urls/local. */
> SVN_ERR(check_paths(&is_repos1, &is_repos2, path_or_url1,
> path_or_url2,
> revision1, revision2, peg_revision));
>
> + SVN_ERR(svn_wc__wrap_diff_callbacks(&diff_processor,
> + callbacks, callback_baton,
> + TRUE /* walk_deleted_dirs */,
> + pool, pool));
> +
> if (is_repos1)
> {
> if (is_repos2)
> {
> /* ### Ignores 'show_copies_as_adds'. */
That comment is obsolete now, since the copies-as-adds feature is handled by the passed-in processor and not by this code.
> - SVN_ERR(diff_repos_repos(callbacks, callback_baton, ctx,
> + SVN_ERR(diff_repos_repos(callback_baton,
> path_or_url1, path_or_url2,
> revision1, revision2,
> peg_revision, depth, ignore_ancestry,
> - pool));
> + diff_processor, ctx, pool));
> }
> else /* path_or_url2 is a working copy path */
> {
[...]
> @@ -2240,6 +2271,7 @@ diff_summarize_wc_wc(svn_client_diff_sum
> void *callback_baton;
> const char *abspath1, *target1;
> svn_node_kind_t kind;
> + const svn_diff_tree_processor_t *diff_processor;
>
> SVN_ERR_ASSERT(! svn_path_is_url(path1));
> SVN_ERR_ASSERT(! svn_path_is_url(path2));
> @@ -2265,14 +2297,17 @@ diff_summarize_wc_wc(svn_client_diff_sum
> &callbacks, &callback_baton, target1, FALSE,
> summarize_func, summarize_baton, pool));
>
> - SVN_ERR(svn_wc_diff6(ctx->wc_ctx,
> - abspath1,
> - callbacks, callback_baton,
> - depth,
> - ignore_ancestry, FALSE /* show_copies_as_adds */,
> - FALSE /* use_git_diff_format */, changelists,
> - ctx->cancel_func, ctx->cancel_baton,
> - pool));
> + SVN_ERR(svn_wc__wrap_diff_callbacks(&diff_processor,
> + callbacks, callback_baton, TRUE,
> + pool, pool));
Is the diff processor going to show_copies_as_adds by default, and so do you need to wrap it with a show-copies-as-changes processor here?
> +
> + SVN_ERR(svn_wc__diff7(NULL,
> + ctx->wc_ctx,
> + abspath1,
> + depth, ignore_ancestry, changelists,
> + diff_processor,
> + ctx->cancel_func, ctx->cancel_baton,
> + pool, pool));
> return SVN_NO_ERROR;
> }
[...]
- Julian
Received on 2014-02-19 13:58:40 CET