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

Re: svn commit: r1569697 - create a diff processor in one place...

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Wed, 19 Feb 2014 12:58:00 +0000 (GMT)

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

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.