On Tue, 05 Dec 2006, Madan S. wrote:
> On Tue, 05 Dec 2006 00:42:39 +0530, Daniel Rall <dlr@collab.net> wrote:
>
> >On Mon, 04 Dec 2006, Madan S. wrote:
>
> [snip]
>
> >I have a question/concern about the use of svn_client_commit_item2_t's
> >wcprop_changes field (which references
> >http://subversion.tigris.org/issues/show_bug.cgi?id=806).
> >
> >The doc string on libsvn_client/ra.c:push_wc_prop() mentions that it
> >"implements the 'svn_ra_push_wc_prop_func_t' interface". This
> >function does make use of wcprop_changes, which contains property data
> >sent back from the repository for *post-commit* processing. That is,
> >it appears that any data left in wcprop_changes after a commit occurs
> >is intended to trigger some additional processing which sets its
> >property data on the WC. See 'svn di -r 3632:3635', which shows
> >wcprop_changes propogated down to
> >libsvn_wc/adm_ops.c:process_committed_leaf(), by way of what's now the
> >svn_wc_process_committed4() -- or _committed5, on the merge-tracking
> >branch -- API.
> >
> >While I didn't notice anything in particular which would prevent us
> >from using wcprop_changes as a pre-commit queue of additional property
> >changes to send from the client to the repository, won't we need to
> >clear out the wcprop_changes array after sending the data to the repos
> >as part of the commit to avoid doing unnecessary and incorrect
> >post-commit processing (which sets the WC's properties to what we sent
> >to the repos)? Something along the lines of a
> >"item->wcprop_changes->nelts = 0;" as the last statement at the end of
> >your "if (item->wcprop_changes)" block might suffice.
>
> You are correct. I didn't understand this logic earlier... but again, as
> you say, this should not prevent us from using the member prior to commit
> on the repos (maybe the name doesnt exactly match what we want to do ? -
> as it says 'wcprop' in a different sense?).
Yes, the name doesn't match exactly what we want to do, as we're not
actually changing any WC properties here. Too bad the field isn't
named prop_changes. Hmmmmmm.
All our options here appear to involving rev'ing the data structure
and its callers. :-\
a) Rename wcprop_changes to prop_changes, and use it for both extra
client -> repos and repos -> WC property edits, with an API contract
indicating that it must be cleared after the client -> repos prop
edits. This could be confusing to developers using the structure for
the first time, but might use less memory.
b) Add a repos_prop_changes field for client -> repos prop edits.
Either way, wcprop_changes could REALLY use some better documentation!
> But I guess like you said above, setting restting the wcprop_changes with
> a new array (not just setting nelts to 0) would not interfere with the
> current implementation.
Really? I didn't notice any place where it might interfere.
The doc string on apr_array_header_t's nelts field says "The number of
active elements in the array". It has a separate field, nalloc, which
represents "The number of elements allocated in the array". My
understanding of apr_array_header_t is that manipulating nelts is an
acceptable way to shrink or clear the list (though there's also the
apr_array_pop() API).
process_committed_leaf() makes proper use of the nelts field:
/* Do wcprops in the same log txn as revision, etc. */
if (wcprop_changes && (wcprop_changes->nelts > 0))
{
*shrug*
> >A change like this should be committed to trunk first, as it
> >introduces a general non-WC property setting mechanism which is
> >decoupled from Merge Tracking.
>
> Agree. Will submit a patch to trunk after testing on all ra_xxx's.
> Does ra_serf need to be tested too?
Yes, but if you get a patch working with the other three, I can give
it a whirl over ra_serf.
> Thanks for the review, Dan.
You're welcome, Madan!
- Dan
- application/pgp-signature attachment: stored
Received on Tue Dec 5 19:17:00 2006