Re: The purpose of prepare_merge_props_changed()

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.

-- 
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba