On Feb 7, 2008 4:23 PM, David Glasser <glasser_at_davidglasser.net> wrote:
> On Feb 4, 2008 9:41 AM, Paul Burba <ptburba_at_gmail.com> wrote:
> > On Feb 1, 2008 5:37 PM, David Glasser <glasser_at_davidglasser.net> wrote:
> >
> > Hi David, appreciate you taking a look at this, comments below.
> >
> >
> > > On Jan 30, 2008 11:34 AM, <pburba_at_tigris.org> wrote:
> > > > Author: pburba
> > > > Date: Wed Jan 30 11:34:41 2008
> > > > New Revision: 29085
>
> > > Whoa. We're doing a full RA roundtrip for every range in every source
> > > in every mergeinfo prop? That seems terribly suboptimal. We should
> > > go the other way round: use svn_client__get_history_as_mergeinfo to
> > > find the history of the path just once, and then use something like
> > > svn_mergeinfo_remove. You should be able to replace the whole
> > > function with that sort of thing.
> >
> > The problem is that svn_client__get_history_as_mergeinfo() can only
> > look backwards (I know, hence the name!). This isn't a problem if our
> > merge targets working revision is greater than any of the added
> > mergeinfo's revisions. Just peg the target to the working rev and do
> > as you suggest (I haven't tried this but it certainly seems like it
> > should work).
> >
> > But what if our WC merge target is at working revision *less* than any
> > of the added ranges? What can we use for our peg revision to
> > svn_client__get_history_as_mergeinfo()? Consider this simple example:
>
> [clear example snipped]
>
> Ugh. You're right. (and this is why letting people merge into
> non-simple wcs is hard...)
>
> You might be able to do something where you start with
> svn_client__get_history_as_mergeinfo, and then if you see newer ranges
> and find out from the server that it's the same line-of-history, then
> you can augment the history-as-mergeinfo. Or something. But you're
> right, it's definitely more complicated than I thought.
There are definitely some optimizations along those lines that could be done.
> Remind me, how important is this change? Is it largely cosmetic, or
> does it have serious effects?
It's cosmetic as best I can tell. I haven't found any problems that
arise from a path having explicit mergeinfo that is redundant with
that path's own history. I find it slightly confusing to have it and
since, IMHO, merge tracking is already a bit confusing, I was only
trying to reduce the "noise" for eventual users who haven't been
thinking about mergeinfo for the last year :-) But if I'm the only
one who thinks this is a problem I'm glad to revert it altogether!
> Have we tested its performance impact?
No. I'm entirely open to doing so of course, though I'm unsure what a
valid test should look like.
> Again, if I'm not misinterpreting how it works, it adds a huge number
> of roundtrips to every merge request.
Yes, there are potentially a lot of calls to
svn_client__repos_locations(). "A lot" being one for every discrete
revision range in every path's *added* mergeinfo.
So if anyone wants to vote I'd love to hear some opinions:
A) Bah, who cares, revert it all, there is no problem here.
B) Leave as is, the overhead of calling svn_client__repos_locations()
repeatedly isn't that bad and merge is already slow.
C) Optimize the situation when merging into an up to date WC
D) I <INSERT NAME HERE> have a brilliant solution that avoids the
overhead of numerous svn_client__repos_locations() calls.
I tend towards C, but would like to hear other's opinions.
> > > I must be missing something> > because I can't see how this can possibly work at all.
> >
> > ...and at first I thought you were right, this must be broken, but I
> > tested it out with multi-line mergeinfo and it works. Why, because
> > filter_self_referential_mergeinfo() returns an array of svn_prop_t's
> > to merge_props_changed() which calls svn_wc_merge_props2() to merge
> > the prop adds to the original props. Thing is, svn_wc_merge_props2()
> > works perfectly well even if multiple elements in the incoming prop
> > changes array refer to the same propname, e.g.
> >
> > If we have this single element propchanges array:
> >
> > Name Value
> > svn:mergeinfo '/A:6\n/A_COPY_2:9\n/A_COPY_3:10'
> >
> > Or this semantically equivalent 3 element array:
> >
> > Name Value
> > svn:mergeinfo '/A:6'
> > svn:mergeinfo '/A_COPY_2:9'
> > svn:mergeinfo '/A_COPY_3:10'
> >
> > Then svn_wc_merge_props2() happily treats these the same way.
>
> Ohhhh... because mergeinfo adds are very special-cased or something?
Exactly, see this code in the svn_wc_merge_props2() helper
libsvn_wc/props.c:apply_single_prop_add():
/* The WC difference doesn't match the new value.
We only merge mergeinfo; other props conflict */
if (strcmp(propname, SVN_PROP_MERGEINFO) == 0)
{
SVN_ERR(combine_mergeinfo_props(&new_val, working_val,
new_val, pool));
.
.
.
See also apply_single_prop_change() and
combine_forked_mergeinfo_props() where similar special casing is done
when the path we are adding mergeinfo to already has existing
mergeinfo.
> > > (Also,
> > > shouldn't there be a space after the ":" in the property value?)
> >
> > No, our created mergeinfo has no whitespace. Although it appears our
> > parser allows whitespace when using svn propset, but then it doesn't
> > get interpreted correctly. I'll look into this some more and start a
> > separate thread if necessary.
>
> Huh, wonder why I thought that.
Starting another thread on this.
Paul
---------------------------------------------------------------------
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-11 16:34:32 CET