lundblad@tigris.org writes:
> Log:
> First step towards resolving issue #2064: redesign the svn_wc_diff_callbacks_t
> API, so that it provides contents and properties in the same callback call.
> This commit changes the public API functions. Users of these will be
> updated in later commits.
Nice job, Peter! This is really thorough. I found a only few minor
nits & questions. Apologies if I missed anything major. It was a big
commit; I did review the whole thing, but my eye started to wander a
bit after the first few hundred lines...
> --- trunk/subversion/include/svn_wc.h (original)
> +++ trunk/subversion/include/svn_wc.h Sun Nov 7 15:50:26 2004
> @@ -626,10 +644,16 @@
> svn_revnum_t rev2,
> const char *mimetype1,
> const char *mimetype2,
> + const apr_array_header_t *propchanges,
> + apr_hash_t *originalprops,
> void *diff_baton);
>
> /** A file @a path was deleted. The [loss of] contents can be seen by
> - * comparing @a tmpfile1 and @a tmpfile2.
> + * comparing @a tmpfile1 and @a tmpfile2. Similarly, the [loss of]
> + * properties is provided in @a originalprops. (The list of property changes
> + * is not provided, since it would just be a list of all properties in
> + * @a originalprops with @c NULL values. ### This is inconsistent, what
> + * ### about dropping the file that will always be empty?)
> *
> * If known, the @c svn:mime-type value of each file is passed into
> * @a mimetype1 and @a mimetype2; either or both of the values can
I don't really understand the language about properties here. What
exactly is provided in 'originalprops'? And what is the parenthetical
aside trying to say? Help :-).
> @@ -1879,6 +1974,25 @@
> * If @a cancel_func is non-null, it will be used along with @a cancel_baton
> * to periodically check if the client has canceled the operation.
> */
> +svn_error_t *svn_wc_get_diff_editor3 (svn_wc_adm_access_t *anchor,
> + const char *target,
> + const svn_wc_diff_callbacks2_t *callbacks,
> + void *callback_baton,
> + svn_boolean_t recurse,
> + svn_boolean_t ignore_ancestry,
> + svn_boolean_t use_text_base,
> + svn_boolean_t reverse_order,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + const svn_delta_editor_t **editor,
> + void **edit_baton,
> + apr_pool_t *pool);
> +
> +
> +/** @deprecated Provided for backwards compatibility with the 1.1.0 API.
> + *
> + * Similar to @c svn_wc_get_diff_editor3(), but with an
> + * @c svn_wc_diff_callbacks_t instead of the @c svn_wc_diff_callbacks2_t. */
> svn_error_t *svn_wc_get_diff_editor2 (svn_wc_adm_access_t *anchor,
> const char *target,
> const svn_wc_diff_callbacks_t *callbacks,
s/instead of the/instead of/
> --- trunk/subversion/libsvn_wc/diff.c (original)
> +++ trunk/subversion/libsvn_wc/diff.c Sun Nov 7 15:50:26 2004
>
> @@ -221,6 +220,11 @@
> apr_pool_t *pool;
> };
>
> +/* Used to wrap svn_diff_wc_callbacks2_t. */
> +struct callbacks2_baton {
> + const svn_wc_diff_callbacks_t *callbacks;
> + void *baton;
> +};
Typo: you wrote "svn_diff_wc_" instead of "svn_wc_diff_".
Also, calling this 'callbacks2_baton' invites confusion with the baton
(of a totally different type) that lives inside it. I'm not sure what
the best solution here is. Maybe call it a "wrapper" or a
"wrap_baton" instead, or something?
Or maybe if it is documented more thoroughly, then it won't be
confusing (at least to me) anymore. As written, I'm a bit unsure of
how this struct is used. It says it wraps 'svn_diff_wc_callbacks2_t',
but inside it I see just a 'svn_diff_wc_callbacks_t', and a baton that
presumably goes with the latter not the former.
I'm worried that a slight incompatibility might have been introduced
here. In the old svn_wc_diff_callbacks_t, the props_changed callback
is documented to be called on both files and dirs. In the new
svn_wc_diff_callbacks2_t, of course, it's 'dir_props_changed' --
called only for dirs, even though the callback's signature is exactly
the same. So I'm not quite sure how a wrapping strategy works here;
it would seem to me that to get truly backwards-compatible behavior,
drivers would have to behave a bit differently depending on which
callback/baton combination they're driving -- in other words, one
can't just substitute in any set of callbacks/batons and drive it the
same way as another.
Like I said, it could be that I'm simply being dense, and not
understanding the compatibility strategy here. If you could clarify
it, that would be great...
> @@ -1333,14 +1344,141 @@
> return SVN_NO_ERROR;
> }
>
> +/* A diff_callbacks2 function. */
> +static svn_error_t *
> +file_changed (svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *contentstate,
> + svn_wc_notify_state_t *propstate,
> + const char *path,
> + const char *tmpfile1,
> + const char *tmpfile2,
> + svn_revnum_t rev1,
> + svn_revnum_t rev2,
> + const char *mimetype1,
> + const char *mimetype2,
> + const apr_array_header_t *propchanges,
> + apr_hash_t *originalprops,
> + void *diff_baton)
Write out the entire type name: "svn_wc_diff_callbacks2".
> +/* An diff_callbacks2 function. */
> +static svn_error_t *
> +file_added (svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *contentstate,
> + svn_wc_notify_state_t *propstate,
> + const char *path,
> + const char *tmpfile1,
> + const char *tmpfile2,
> + svn_revnum_t rev1,
> + svn_revnum_t rev2,
> + const char *mimetype1,
> + const char *mimetype2,
> + const apr_array_header_t *propchanges,
> + apr_hash_t *originalprops,
> + void *diff_baton)
Same here, and with the other callbacks (I won't list the rest).
Note that here you used "An" instead of "A". I guess when you have
the "svn_" prefix on the type, you'll want to use "An" consistently.
> +static struct svn_wc_diff_callbacks2_t callbacks2_wrapper = {
> + file_changed,
> + file_added,
> + file_deleted,
> + dir_added,
> + dir_deleted,
> + dir_props_changed
> +};
Documentation?
The name of this struct is confusingly similar to the earlier claim
about 'struct callbacks2_baton' ("Used to wrap
svn_diff_wc_callbacks2_t"). I see how/where this static struct is
used, but I have to admit that at this point my head is spinning from
the layers of compatibility glue going on here. Somewhere, a comment
explaining the overall strategy is needed, IMHO.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 8 18:55:34 2004