On Mon, Jan 7, 2013 at 1:28 PM, Paul Burba <ptbu...@gmail.com> wrote: > On Fri, Jan 4, 2013 at 3:04 PM, Julian Foad <julianf...@btopenworld.com> > wrote: >> The condition was originally added in r873100, following the discussion >> (mainly between Paul and me) at >> <http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=72921>. >> At that time, the "foreign repos" filtering was inside the function >> "filter_self_referential_mergeinfo", so that may have caused confusion. >> >> It looks like r873100 was in error and the foreign-repos filtering should be >> unconditional. > > Hi Julian, > > Quite correct, +1 for this change. > >> Fix: >> [[[ >> Index: subversion/libsvn_client/merge.c >> =================================================================== >> --- subversion/libsvn_client/merge.c (revision 1429021) >> +++ subversion/libsvn_client/merge.c (working copy) >> @@ -1293,32 +1293,33 @@ prepare_merge_props_changed(const apr_ar >> } >> props = mergeinfo_props; >> } >> >> if (props->nelts) >> { >> + /* Issue #3383: We don't want mergeinfo from a foreign repos. >> + >> + If this is a merge from a foreign repository we must strip all >> + incoming mergeinfo (including mergeinfo deletions). Otherwise if >> + this property isn't mergeinfo or is NULL valued (i.e. prop removal) >> + or empty mergeinfo it does not require any special handling. There >> + is nothing to filter out of empty mergeinfo and the concept of >> + filtering doesn't apply if we are trying to remove mergeinfo >> + entirely. */ >> + if (! merge_b->same_repos) >> + SVN_ERR(omit_mergeinfo_changes(&props, props, result_pool)); >> + >> /* If this is a forward merge then don't add new mergeinfo to >> PATH that is already part of PATH's own history, see >> http://svn.haxx.se/dev/archive-2008-09/0006.shtml. If the >> merge sources are not ancestral then there is no concept of a >> 'forward' or 'reverse' merge and we filter unconditionally. */ >> if (merge_b->merge_source.loc1->rev < merge_b->merge_source.loc2->rev >> || !merge_b->merge_source.ancestral) >> { >> - /* Issue #3383: We don't want mergeinfo from a foreign repos. >> - >> - If this is a merge from a foreign repository we must strip all >> - incoming mergeinfo (including mergeinfo deletions). Otherwise >> if >> - this property isn't mergeinfo or is NULL valued (i.e. prop >> removal) >> - or empty mergeinfo it does not require any special handling. >> There >> - is nothing to filter out of empty mergeinfo and the concept of >> - filtering doesn't apply if we are trying to remove mergeinfo >> - entirely. */ >> - if (! merge_b->same_repos) >> - SVN_ERR(omit_mergeinfo_changes(&props, props, result_pool)); >> - else if (HONOR_MERGEINFO(merge_b) || merge_b->reintegrate_merge) >> + if (HONOR_MERGEINFO(merge_b) || merge_b->reintegrate_merge) >> SVN_ERR(filter_self_referential_mergeinfo(&props, >> local_abspath, >> merge_b->ra_session2, >> merge_b->ctx, >> result_pool)); >> } >> ]]] >> >> FWIW, I tried removing the "if (! reverse-merge)" condition entirely (from >> both the foreign-repos and the self-ref filtering), and the test suite >> passed. That is not too surprising, as it probably has very few test cases >> for >> foreign-repos reverse merge and/or self-ref mergeinfo. > > Hmmm. Reviewing the old thread > (http://svn.haxx.se/dev/archive-2008-09/0397.shtml) it seems my most > compelling argument in favor not filtering during reverse merges was > delivered to you via IRC -- in the days before it was logged :-\ I > wish I could recall what I said! Regardless, there are still the two > advantages mentioned in the thread to skipping self-referential > filtering during reverse merges: > > 1) Performance -- filter_self_referential_mergeinfo(), potentially run > for every subtree with mergeinfo changes, calls > svn_client__repos_location(). > > 2) Reverse merges "revert" to the same prior mergeinfo representation. > > I can come up with a contrived example where filtering > self-referential mergeinfo from reverse merges gives a better result > (i.e. self-referential mergeinfo is actually removed), but this > involves purposefully setting self-referential mergeinfo with 'svn > propset'. The word "tortured" comes to mind. > > Unless there is a realistic use-case I am missing (which demonstrates > the benefit of filtering self-referential mergeinfo during reverse > merges) then I'm reluctant to change this. Reluctant, but not totally > against, I can see the opposing argument in support of this > behavior...there's no "correct" answer here that I can see; each > approach has it's pros and cons.
Hi Julian, Ben was kind enough to provide a copy of the IRC logs from September 2008. Unfortunately there was nothing terribly enlightening there (see below). So the above comments still represent my thoughts on the issue of filtering during reverse merges (i.e. that it is only useful in highly contrived use cases). 07:06 <@!pburba> julianf: ping 07:07 < !julianf> pburba: hi! 07:07 <@!pburba> julianf: Re your latest on the 'Incorrect handling of svn:mergeinfo during merging on the svnpatch-diff branch' thread, you say 'Note that the "filtering", although currently implemented as a single function call, is performing two different logical functions:' 07:07 <@!pburba> julianf: Did you mean this comment and what follows only to apply to reverse merges? Or both forward and reverse merges? Or something else? 07:09 < !julianf> I meant that, altogether, it can perform two different logical functions. Then I go on to try to see which of the two are relevant for forward merges, and which are relevant for reverse merges. 07:09 < !julianf> In forward merges, both functions appear to be desired. 07:10 <@!pburba> So when saying '2. Clean up any self-referential mergeinfo that may have existed.' what do you mean by 'mergeinfo that may have existed'? Existed on the WC target? 07:11 * !pburba is not purposely trying to be dense here! 07:11 < !julianf> Hold on a sec while I think about that. 07:11 <@!pburba> np, respond at your leisure 07:17 < !julianf> pburba: Yes, by "clean up" I was always referring to self-ref mergeinfo that already existed on the target. (I hadn't thought about the case where the source change might include addition of self-ref mergeinfo w.r.t the source branch, which occurs to me now; is that a relevant issue here?) 07:21 <@!pburba> julianf: That's all this is about in fact, this function won't filter or otherwise alter the mergeinfo that exists on the wc target. It is only filtering the PROPCHANGES argument to merge_props_changed(), it doesn't touch the ORIGINAL_PROPS argument. 07:22 < !julianf> Ah... 07:22 < !julianf> I misunderstood that. 07:22 <@!pburba> A glance at merge_tests.py 86 'cyclic merges don't add mergeinfo from own history' may be enlightening here, it demonstrates the original problem this function was trying to solve as regards cyclic merges 07:24 < !julianf> (any particular bit of that test? :-) 07:25 <@!pburba> heh, well first see the comment at the start 'Merge r3 from 'A' to 'A_COPY', make a text mod to 'A_COPY/mu' and...'. This function kicks in when that final merge is attempted 07:26 <@!pburba> julianf: Sorry, the second merge is what I mean, that test got added on to 07:27 * !pburba now wonders if looking at this test is *really* going to help :-\ 07:28 <@!pburba> For the benefit of those playing at home: Merge r3 from 'A' to 'A_COPY', make a text mod to 'A_COPY/mu' and commit both as r7. This results in mergeinfo of '/A:3' on 'A_COPY'. Then merge r7 from 'A_COPY' to 'A'. This attempts to add the mergeinfo '/A:3' to 'A', but that is self-referrential and should be filtered out, leaving only the mergeinfo '/A_COPY:7' on 'A'. 07:28 < !julianf> OK. So this "filter" is just avoiding giving the target any new self-ref mergeinfo as part of the present merge. 07:28 <@!pburba> exactly 07:29 <@!pburba> merge.c:filter_natural_history_from_mergeinfo() is in the business of trying not to create *new* self-referential mergeinfo 07:29 < !julianf> So what was going wrong with it in a reverse merge? 07:31 < !julianf> Hold on a sec, while I familiarise myself with the difference between "filter_natural_history_...()" and "filter_self_ref...()". 07:33 < !julianf> You wrote: [what's wrong is] 07:34 < !julianf> "filter_self_ref...() is filtering out 1-31375... Should only filter out 1-28961, so this is one bug..." 07:35 < !julianf> So, maybe it is buggy for reverse merges, in which case this patch to avoid using it will avoid the bug. 07:36 <@!pburba> No there is a bug in filter_self_referential_mergeinfo() for forward merges too, not filtering self-ref MI for reverse merges avoids that bug in Arfrever's particular example, but it doesn't solve the underlying problem, which can occur during forward merges (ast the new merge_tests.py 110 'natural history filtering permits valid mergeinfo' demonstrates) 07:40 <@!pburba> So there are 3 issues here: 07:40 <@!pburba> 1) Does it make sense to never filter self referential mergeinfo from incoming propchanges during reverse merges. 07:40 <@!pburba> 2) Overfiltering incoming prop changes during forward merges (as demonstrated by merge test 110) needs to be fixed. 07:40 <@!pburba> 3) And, though we haven't really discussed it much, filtering performance can be improved in some common use-cases by using svn_client__get_history_as_mergeinfo(). If I'm in this code I'd like to tackle this too - see http://svn.haxx.se/dev/archive-2008-02/0063.shtml 07:45 * !pburba thinks those 3 items are in dramatically increasing order of importance 07:53 < !julianf> pburba: OK, thanks for the explanation. I'll revisit my on-list reply unless you're setting it straight for me. 07:55 <@!pburba> julianf: I was in the middle of replying when I started this conversation, I'll note this conersation there... 07:55 < !julianf> ok 09:17 < !julianf> :q 09:44 < !julianf> pburba: How about I commit my merge_baton patch (with your tweak to the doc string), and then you commit your patch that uses it? 09:45 <@!pburba> julianf: Sounds good to me -- Paul T. Burba CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development Skype: ptburba
