kfogel_at_tigris.org writes:
> Follow up to r27377 with documentation and allocation fixes.
>
> * subversion/libsvn_wc/update_editor.c
> (choose_base_paths): Rewrite doc string, clarifying some details.
> (copy_regular_props): Clarify lifetime of returned data.
> (add_file_with_history): Use a subpool for temporary allocations,
> and remove comment about this.
>
> * subversion/libsvn_wc/merge.c
> (svn_wc__merge_internal): Document new copyfrom_text parameter,
> taking the description from the r27377 log message.
David, et al: this commit follows up to:
http://svn.haxx.se/dev/archive-2007-10/1221.shtml
http://svn.haxx.se/dev/archive-2007-10/1239.shtml
(I should have mentioned them in the log message; I'll edit it.)
We're near being able to close issue #2986, I think. Responding to
your points from the first mail above:
> Am I being careful to use revert-base instead of text-base everywhere
> necessary? I don't think I'm regressing anywhere but there might be
> some pre-existing problems. For example, should svn_wc__merge_props
> be considering using revert-props?
This is the one thing left that bothers me. I think we probably
should be using revert-props as you suggest, but the Right Way to do
that is, I guess, to extend choose_base_paths() to pick the
appropriate prop base as well, setting it in the file baton as it does
with the text base paths.
However, choose_base_paths() is currently called in apply_textdelta().
Since there's no telling whether apply_textdelta() will even be called
(props could change without text changing), we'd have to move the
choose_base_paths() call out to somewhere else, like open_file().
I think that means that some of the information it would return would
have to be stored in even more new fields in file_baton, since we
can't change the signature of apply_textdelta() of course (and anyway
we don't call it directly, it's called for us).
Does this summary of the situation make sense to you?
> Some stuff in add_file_with_history should be moved into a subpool
> and cleared.
Done (though not ported to 1.5.x).
> There's still the issue in locate_copyfrom where we are making a brand
> new adm_access instead of using the set we already have, and not
> taking out a write lock.
I don't think that matters much. We don't need a write lock for these
purposes; the only issue here may be an inefficiency (we already have
an associated set, but we neglect to use it). But the increase in
code complexity to get rid of this inefficiency is not, IMHO, worth
it.
Normally, I'd move issue #2986 out of the 1.5 milestone -- after all,
these bugs have been present forever, r27377 just alleviated them.
But, even just with the props, there may be potential for data loss,
so we should probably deal with this now.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-04 00:23:19 CET