You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2014/02/19 13:58:00 UTC
Re: svn commit: r1569697 - create a diff processor in one place...
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
RE: svn commit: r1569697 - create a diff processor in one place...
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: woensdag 19 februari 2014 13:58
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1569697 - create a diff processor in one
place...
>
> 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?
No time for a full reply, but this is necessary for proper calculation of
the full path in diffs.
Currently (at 1.8) we duplicate anchor calculations in many places... See
followup patches which use this to remove duplicated code.
Bert