On Mon, 8 Nov 2004 kfogel@collab.net wrote:
> 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...
>
Don't think about how my head spinned when I wrote it:-) There is a lot
left, but I did want to do this in chunks. But I had to do this in a chunk
to not break the build.
> > --- 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 :-).
>
Well, originalprops provides the properties on the deleted file. If you
provide a diff for an add, you provide the properties (not exactly
everywhere, since that's buggy, but anyway). Then it's inconsistent to not
provide properties on a file being deleted, isn't it? I don't, however,
provide a diff in the form of a array of svn_prop_t,
since, because all properties are going away, it would just be a list of
all property names with null values. It is different with add, since
sometimes we provide diffs against the copy source.
> > --- 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?
>
I choose callbacks_wrapper_baton and clarified the documentation.
Actually, it is used while wrapping svn_wc_diff_callbacks_t by an
svn_wc_diff_callbacks2_t. Tha might explain your confusion.
> 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.
>
Maybe my explanation above clears it up. Wrapping the other way around is
no problem,, and that's what we need in this file. The new routines in the
file call a callbacks2_t, the wrapper dispatches to, for example,
file_added plus props_changed (or just one of them) in the callbacks_t
struct.
> > +static struct svn_wc_diff_callbacks2_t callbacks2_wrapper = {
> > + file_changed,
> > + file_added,
> > + file_deleted,
> > + dir_added,
> > + dir_deleted,
> > + dir_props_changed
> > +};
>
> Documentation?
>
And name change for consistency.
As usual, thanks. And if I didn't comment, I just fixed. Will commit
shortly.
regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 8 21:24:45 2004