Not filtering self-ref mergeinfo in reverse merges [was: The purpose of prepare_merge_props_changed()]

Paul Burba wrote on 2013-01-07:

> Julian Foad wrote:
>> 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.
>
> [...] there are still the two advantages mentioned in the thread to
> skipping self-referential filtering during reverse merges:

I'd just like to set down my own thoughts on this matter.

> 1) Performance -- filter_self_referential_mergeinfo(), potentially run
> for every subtree with mergeinfo changes, calls
> svn_client__repos_location().

If it was obvious that leaving self-ref mergeinfo in place is a correct behaviour, and that making the forward-merge filtering reasonably efficient is too difficult to contemplate at the moment, and that this speed improvement during reverse merges is a significant overall improvement to the product, then the "performance" argument would be a no-brainer.

Is it obviously correct?  It wasn't obvious to me.  At first I thought the intent was not to allow self-ref mergeinfo and the code was meaning to omit the filtering if and only if it knew that there couldn't be any incoming self-ref mergeinfo in this case, and I couldn't verify that was the case.  Later I divined that on the contrary our rule is only "self-ref mergeinfo is allowed, but mostly we prefer to filter it out", and the code is simply preferring to omit the filtering in this case.  Then, yes, skipping the filtering is obviously correct.

I would like to suggest that even if that is our rule today, it would be advantageous to strengthen the rule to at least "self-ref mergeinfo shall never be added to an svn:mergeinfo prop" or better still "when an svn:mergeinfo prop is written it shall not contain any self-ref mergeinfo" -- see below.

Is it too difficult to make the filtering efficient at the moment?  I'll accept that it is too difficult for now.

Is the speed improvement significant?  Of course it is locally significant to someone performing a reverse merge that includes subtree mergeinfo changes.  Overall, though?  I don't know; honestly I suspect not.

> 2) Reverse merges "revert" to the same prior mergeinfo representation.

So, the thinking is that's a Good Thing, because it means you can verify the result by diffing against the original and finding no difference, for example.  Sure.

However, we seem to have decided that self-ref mergeinfo is a Bad Thing in general, to be avoided when possible.  We know that consistency is a Good Thing; therefore not introducing self-ref mergeinfo at all would be a Good Thing.  Bear in mind that the mergeinfo change in a reverse merge comes from arbitrarily long ago, perhaps before we had this rule about not storing self-ref mergeinfo, or perhaps it was already breaking this rule in the first place.

Reverting to a state that once existed, but that is now considered a Bad Thing, is not so obviously desirable.

If we were to apply a no-self-ref rule universally, then we could use broad techniques such as verifying in the test suite that there is no self-ref mergeinfo at any time (unless injected on purpose by the test).  If we don't, we can't.

I happen to think that the benefits of consistency should outweigh the desire for speed in this case.

If and when I am re-working the merge code architecture, that's when I'd want to remove this condition, and I would hope to have a more efficient filtering in place before then.  I am not pressing to remove the condition right now.

- Julian

> 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.
Received on 2013-01-10 17:46:11 CET