On Wed, 2008-09-10 at 11:40 -0400, Paul Burba wrote:
> On Tue, Sep 9, 2008 at 4:32 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> > Now for the specifics: What's going wrong here is that when we perform
> > the reverse merge of -c-32625,
> > libsvn_subr/merge.c:filter_self_referential_mergeinfo() is filtering
> > out /trunk:1-31375' portion of svnpatch-diff's mergeinfo. As
> > mentioned above it should only filter out '/trunk:1-28961', so that is
> > one bug. But I can't see any reason to ever do filtering of this type
> > during a *reverse* merge. Currently the svn_wc_diff_callbacks3_t
> > callback merge.c:merge_props_changed() calls
> > filter_self_referential_mergeinfo() without consideration of the
> > merge's direction.
> >
> > So we need to do the following:
> >
> > 1) Stop filtering self-referential mergeinfo during reverse merges
> > (easier said than done as we don't have enough context in the callback
> > to determine merge direction right now). If you or anything else can
> > see a reason this filtering needs to be done in reverse merges please
> > let me know!
This looks OK, but I struggled to fully understand both the motivation
and the correctness.
It seems to me this is a fairly ambivalent decision, rather than a
"need".
Note that the "filtering", although currently implemented as a single
function call, is performing two different logical functions:
1. Avoid adding any self-referential mergeinfo in the current merge.
2. Clean up any self-referential mergeinfo that may have existed.
Step (1). I think it is true that a reverse merge can never add
self-referential mergeinfo ("mergeinfo" meaning "merge history"),
because it never adds any mergeinfo, only takes some away. I wonder if a
reverse merge can add an svn:mergeinfo _property_ containing self-ref
mergeinfo onto a sub-tree, where previously that sub-tree inherited its
mergeinfo. If that is possible, then avoiding adding self-ref mergeinfo
in that scenario would make some sense. If not, then step (1) is
inoperative.
Step (2). I can't see a particular reason to avoid "cleaning up" during
a reverse merge. If we choose to clean up self-referential mergeinfo
during a forward merge, then from one point of view it is logical to do
it also during a reverse merge.
The nicety of having a reverse merge revert to exactly the previous
_representation_ is not to be ignored, but having it revert to exactly
the previous mergeinfo _state_ (with a possibly different
representation) is not exactly wrong.
Another reason noted in the log message is efficiency. Since reverse
merges are (presumably) infrequent, and since the efficiency of this
filtering wants to be improved for the sake of forward merges anyway,
this isn't a particularly compelling reason.
On the other hand, I can only see one reason against making this change,
and that is the slight increase in code complexity. However, this is
miniscule compared with the rest of the merging code.
> The attached patch takes care of this. I'll commit later today if
> there are no objections.
[[[
> Don't try to filter natural history from mergeinfo during reverse
> merges.
>
> If a repository somehow gets self-referential mergeinfo in it, it's
> nice to clean this up, but a reverse merge is not the place to do it.
> A reverse merge should simply return mergeinfo to a previous state.
> See
> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142777.
Hmm. Does "a reverse merge" mean "a reversal of a merge recorded in this
thing's merge history" or "a merge of URL1_at_REV1:URL2_at_REV2 in which
REV2<REV1"? It looks like these two definitions are supposed to come to
the same thing.
> This change also improves reverse merge performance as it eliminates some
> unnecessary communication with the repos.
>
> * subversion/libsvn_client/merge.c
> (merge_cmd_baton_t): Add 'is_rollback' member.
> (merge_props_changed): Don't try to filter natural history for a reverse
> merge.
> (do_merge): Set merge_cmd_baton_t's new 'is_rollback' member to reflect
> the current merge source being merged.
]]]
[[[
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 33012)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -264,6 +264,13 @@
> svn_ra_session_t *ra_session1;
> svn_ra_session_t *ra_session2;
>
> + /* Whether the merge being performed is a reverse or forward merge.
> + Note: Since combinations of forward and reverse merges are supported
> + by svn_client_merge_peg3() the value of this field may change and
> + applies only to the merge_source_t *currently* being merged by
> + do_merge(). */
> + svn_boolean_t is_rollback;
This field serves the immediate purpose. I did notice before, when
looking at storing info about conflicts, that it would be useful to have
the merge source (URL1_at_REV1, URL2_at_REV2) stored in the baton, where
currently we have only URL2 (called "url") and now the information (REV2
< REV1).
See the attached patch to replace "url" with a "merge_source" structure.
Dou you think that would be better?
> /* Pool which has a lifetime limited to one iteration over a given
> merge source, i.e. it is cleared on every call to do_directory_merge()
> or do_file_merge() in do_merge(). */
> @@ -564,9 +571,11 @@
> TRUE, -1, ctx->cancel_func,
> ctx->cancel_baton, subpool));
>
> - /* Don't add mergeinfo from PATH's own history. */
> - SVN_ERR(filter_self_referential_mergeinfo(&props, path, merge_b,
> - adm_access, subpool));
> + /* If this is a forward merge then don't add mergeinfo from
> + PATH's own history. */
The wording can be read as implying that we might want to add such
mergeinfo during a reverse merge. Can we make the intent more explicit:
If this is a forward merge then don't add mergeinfo from
PATH's own history, and take this opportunity to remove any
that might already be present.
If this is a reverse merge then we can't be adding any, and
we don't want to filter out any that might already be
present because we want the result in simple cases to be
exactly as if the forward merge had never been performed.
> + if (!merge_b->is_rollback)
> + SVN_ERR(filter_self_referential_mergeinfo(&props, path, merge_b,
> + adm_access, subpool));
>
> err = svn_wc_merge_props2(state, path, adm_access, original_props, props,
> FALSE, merge_b->dry_run, ctx->conflict_func,
> @@ -5867,6 +5876,7 @@
> merge_cmd_baton.paths_with_new_mergeinfo = NULL;
> merge_cmd_baton.ra_session1 = ra_session1;
> merge_cmd_baton.ra_session2 = ra_session2;
> + merge_cmd_baton.is_rollback = (rev1 > rev2);
>
> /* Populate the portions of the merge context baton that require
> an RA session to set, but shouldn't be reset for each iteration. */
]]]
In summary, I've no problem with you committing this if you could give
some sort of tweak to the comment I commented on, and all the rest of my
thoughts are just for your consideration!
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-11 14:52:01 CEST