[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.