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.
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.
- Julian
I (Julian Foad) wrote:
> I'm trying to understand prepare_merge_props_changed(). I don't get why
> the stripping
> of self-referential and foreign-repos mergeingo is *not* performed when
> doing a reverse merge, as I highlight below. Paul, can you shed any light on
> it?
>
> Index: subversion/libsvn_client/merge.c
> ===================================================================
>
> /* Prepare a set of property changes PROPCHANGES to be used for a merge
> - operation on LOCAL_ABSPATH. Store the result in *PROP_UPDATES.
> + operation on LOCAL_ABSPATH.
>
> + Remove all non-regular prop-changes (entry-props and WC-props).
> + Remove all non-mergeinfo prop-changes if it's a record-only merge.
> + Remove self-referential mergeinfo (### in some cases...)
> + Remove foreign-repository mergeinfo (### in some cases...)
> +
> + Store the filtered property changes in *PROP_UPDATES.
> Store information on where mergeinfo is updated in MERGE_B.
>
> Used for both file and directory property merges. */
> static svn_error_t *
> prepare_merge_props_changed(const apr_array_header_t **prop_updates,
> const char *local_abspath,
> const apr_array_header_t *propchanges,
> merge_cmd_baton_t *merge_b,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> apr_array_header_t *props;
>
> [...]
>
> if (props->nelts)
> {
> /* 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. */
> + /*
> + * ### Whereas, if this is a reverse merge, this implies it's OK
> + * to add new mergeinfo that is already part of PATH's own
> + * history (and OK to add foreign-repository mergeinfo) ???
> + *
> + * This doesn't sound right. Why are these two kinds of
> + * filtering conditional at all?
> + */
> 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)
> SVN_ERR(filter_self_referential_mergeinfo(&props,
> local_abspath,
> merge_b->ra_session2,
> merge_b->ctx,
> result_pool));
> }
> }
>
>
> It looks like if I were to try a foreign-repos reverse merge, where the change
> in the foreign repos included a mergeinfo change, that mergeinfo change would
> (wrongly) propagate into the target repos.
> The following snippet isn't a full repro recipe but shows that a foreign
> mergeinfo change is indeed being applied in this case:
>
> $ svn merge -c-5 file:///home/julianfoad/tmp/svn/frm/repo/branches/A foo/
> --- Reverse-merging (from foreign repository) r5 into 'foo':
> U foo/A
> C foo
> Summary of conflicts:
> Property conflicts: 1
> Conflict for property 'svn:mergeinfo' discovered on 'foo'.
> Trying to delete property 'svn:mergeinfo'
> but the local property value is different.
> <<<<<<< (local property value)
> /trunk:1-999=======
>>>>>>>> (incoming property value)
>
>
> The discussion in
> <http://subversion.tigris.org/issues/show_bug.cgi?id=3383> doesn't say
> anything about the filtering of foreign-repos mergeinfo being conditional.
>
> I haven't investigated/tested the self-ref filtering part of this.
>
>
> - Julian
>
Received on 2013-01-04 21:04:38 CET