On Mon, Jan 7, 2013 at 1:28 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Fri, Jan 4, 2013 at 3:04 PM, Julian Foad <julianfoad_at_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
Received on 2013-01-07 22:58:53 CET