On Tue, Mar 5, 2013 at 11:37 AM, Julian Foad <julianf...@btopenworld.com> wrote: > Paul Burba wrote: >> On Wed, Feb 27, 2013 at 5:04 PM, Julian Foad wrote: >>> Paul Burba wrote: >>>> I found the cause of the conflict filled reintegrate merge. The >>>> automatic merge code seems to be doing the right thing re Mark's >>>> automatic merge above, the problem arises earlier. >>> >>> Paul, thanks for investigating. While I'm glad to hear the automatic >>> merge was not the main root cause of the problem, it would make sense >>> for it to detect that merges in the opposite direction have been done >>> and at least warn the user. >> >> Hi Julian, >> >> What are we going to warn them to do exactly? > > What I was thinking of here is to detect the most general condition that > isn't handled properly. Let me explain this part, although I'm sure you're > familiar with it. > > Whenever the direction of merge is from A to B and the automatic merge code > doesn't detect that a reintegrate merge is required, and yet one or more > cherry-picked revision ranges and/or subtree merges have been merged from B > to A, Subversion currently doesn't notice those merges from B to A, and > proceeds to merge "everything" from A to B including those changes that were > the result of B-to-A merges. That is what the plain "merge" from A to B has > always done -- it has always ignored any mergeinfo describing merges from B > to A.
Yes, http://subversion.tigris.org/issues/show_bug.cgi?id=4255 for the curious. > In the example at hand, there has been a B-to-A merge of everything from B > (except some non-operative revisions), and so we get loads of conflicts by > ignoring this when we merge the other way. The intelligent user thinks, > "This is clearly a reintegrate scenario." Do they? I'm not very confident that most users will immediately think that, especially since we've deprecated the --reintegrate option. Even if they do think "I need a reintegrate style merge!" I suspect they are more likely to do just that, explicitly using the --reintegrate option, rather than re-syncing B->A and then repeating the automatic "reintegrate style" merge. > On the other hand, if there has been a B-to-A merge of not quite everything > from B (leaving an operative unmerged part at the beginning or somewhere in > the middle), then an old "reintegrate" merge from A to B would error out, > whereas an old non-reintegrate merge would just do the merge and wrongly > re-merge all the B-to-A changes back to B. The new automatic merge, if it > determines (based on the merging of the branch root node) to do a > non-reintegrate, currently does the latter. > > At some point, maybe not for 1.8, if we can detect any such condition, it > would be a good to warn the user: > > A clean merge from A to B is not possible because some but not all > changes have been merged from B to A. You're going to see conflicts > (unless all your potential conflicts are trivially auto-resolved). But a reintegrate style merge already does this, if the automatic merge could only determine up front that that is the correct type of merge. > We could add some suggestions to the warning: > > If you are expecting this to be a reintegrate-like merge, check that > all changes on B including non-operative revision ranges have been > merged to A and recorded in the mergeinfo; try an automatic sync merge > from B to A to fill in any gaps. > > If you are expecting a sync merge, note that there have been some > cherry-pick and/or subtree merges from B to A, and this merge will > attempt to merge these same changes back to B, which will likely > produce conflicts. > > We could add more specific details of what's been found, and more specific > suggestions related to the details, such as the case at hand where all > operative revs have been merged B-to-A, if we think it's worth the effort. > > A basic implementation would not be cheap if there is a significant amout of > subtree mergeinfo to check. And I have carefully avoided the issue of > whether we should error out or proceed with the merge, as there's no point > bike-shedding about that unless we really decide to do this. > > So let's take a closer look at what we could do for the specific case at hand. > > >> To explicitly use >> --reintegrate option? Because that would work in Stefan's case, so we >> should just do it rather than warn...unless you mean something else. > > So, you're thinking more about detecting the very specific kind of scenario > that Stefan encountered -- where everything except some leading non-operative > revisions had been merged B-to-A, and a reintegrate would give the "correct" > result. Why is "correct" in quotes? How is a reintegrate style merge ever the wrong choice in that scenario? Actually, don't answer that, because I'm thinking of a more general case: The missing non-operative revisions may be leading, as in Stefan's case, or they may be interspersed with operative revisions that have already been merged (including subtree merges where the change is only operative in the given subtree -- i.e. would be a no-op even if repeated at the branch root). Right now the automatic merge code (i.e. merge.c:find_last_merged_location()) obviously doesn't handle this, but we already have code that can answer this question: svn_client_mergeinfo_log2[1]. Here's an example from our test suite of what I mean: Run merge_reintegrate_tests.py 13 then revert the local changes the test leaves behind. We want to merge A_COPY to A, but there have been prior cherrypicks and subtree merges from A to A_COPY: >svn pl -vR A_COPY Properties on 'A_COPY': svn:mergeinfo /A:6 Properties on 'A_COPY\B': svn:mergeinfo /A/B:5 Properties on 'A_COPY\D\G\rho': svn:mergeinfo /A/D/G/rho:4 Properties on 'A_COPY\D\H': svn:mergeinfo /A/D/H:3,6 However, we can see that these prior cherrypicks and subtree merges actually constitute all operative revisions up to r8: >svn mergeinfo --show-revs eligible -R ^^/A A_COPY r9 So an explicit reintegrate merge works fine: >svn merge ^^/A_COPY A --reintegrate --- Merging differences between repository URLs into 'A': U A\mu --- Recording mergeinfo for merge between repository URLs into 'A': U A But an automatic merge doesn't notice this, wrongly chooses a sync style merge and results in spurious changes/conflicts: >svn revert -Rq . >svn merge ^^/A_COPY A --- Merging r2 through r9 into 'A': U A\mu C A\D\H\psi --- Recording mergeinfo for merge of r2 through r9 into 'A': U A Summary of conflicts: Text conflicts: 1 Conflict discovered in file 'A\D\H\psi'. Select: (p) postpone, (df) diff-full, (e) edit, (m) merge, (mc) mine-conflict, (tc) theirs-conflict, (s) show all options: p So my point is simply that if an explicit --reintegrate merge can DTRT, why can't an automatic merge? [1] calculate_left_hand_side also does effectively the same thing -- which makes me wonder if we can't use svn_client_mergeinfo_log2 there, but that's another issue. > In an immediate sense, certainly we could "just do it", special-case this > particular scenario (have you thought about what the exact condition should > be?) and automatically choose the reintegrate code path in that case. See above "I'm thinking of a more general case". > The trouble is, that doesn't resolve the situation completely. It gives the > correct result in terms of the changes merged, but it doesn't straighten out > the mergeinfo. You mean the mergeinfo on the source? > That means we have to make sure the rest of the merge subsystem consistently > handles this case too: the "sync"-style merge, merge graphing tools that > determine what kind of merge was done and what kinds of merge are eligible, > and so on. See below. I'm not sure how this is a problem. If we do a reintegrate style merge in the general situation I describe above, how does that break subsequent sync style merges? Can you point to an example? in terms of determining eligible revisions, 'svn mergeinfo' already handles these gaps. Which specific graphing tools are you speaking of? >> I filed an issue for this: >> http://subversion.tigris.org/issues/show_bug.cgi?id=4329 and added a >> test which demonstrates a very simple version of the problem. I set >> issue #4329 a 1.8 blocker. While I think what Stefan is doing is >> somewhat atypical[1], if we are deprecating --reintegrate, then >> automatic merges should handle cases like this. After all, if we >> explicitly use the --reintegrate option the merge works fine. What do >> others think? > > I agree it's atypical (or rather unintended) usage, and I also acknowledge > that if Stefan used merge like this, then sometimes other people will do too. > > However, I don't want us to put any more special cases in the code, > especially without putting them into the *design*. If the old > "--reintegrate" accepts a missing leading no-op revision range in considering > whether "everything" has been merged, that should now be considered a bug. Seriously? As long as we allow cherrypicks and subtree merges I think we want to support use cases like this...to call it a bug that we DTRT, I guess I just don't get it. > The new rule is that if every revision on B is *recorded* as merged to A, > then we can reintegrate from A to B, else we can't. I never came away with the understanding in all the discussion of automatic merges...but maybe I'm the only one. > The main alternative would be to adopt some principle such as "When > determining what's been merged and what needs to be merged, we only notice > operative revisions, aka non-null changes". I considered that during the > design of automatic merge, and was strongly drawn to the idea and started > prototyping around it, but that turned out to be a rat-hole involving > potentially exhaustive traversal of the whole mergeinfo graph (all branches) > to determine whether a given bit of mergeinfo represented an operative or an > inoperative change. And the benefits are near zero, because both sync and > reintegrate merges were already filling in any gaps in the mergeinfo in the > direction of the merge so the situation can only arise if you use manual > merges. > > The sensible choice in the end was to require that revision ranges are > explicitly recorded whether operative or not. > > Having found a case that we suspect may occur frequently enough to cause > annoyance, I believe the best thing to do is to admit that we don't handle it > automatically. Just because the old reintegrate handles this particular case > doesn't mean we're making a mistake if the new automatic merge doesn't. This > behaviour change is a small adjustment towards overall more consistent merge > behaviour, and consistency is key to large-scale usability, much more so than > special casing is. > > So, what to do exactly? Options seem to be: > > 0) leave it as it is > > 1) detect this specific case and warn or error out > > 2) detect this specific case and do a reintegrate The attached patch accomplishes, let's call it "2.5", because it handles the more general case I outlined above, but not the case represented by issue #4255. Would you mind taking a look? -- Paul T. Burba CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development Skype: ptburba > 3) detect the general case and warn or error out > > 4) detect the general case and merge properly (hah! that's for 1.9) > > I would say 0/1/2 are achievable and 3/4 are too difficult, and 2 is a bad > idea. > > Thoughts? > > - Julian
Fix issue #4329 'automatic merge uses reintegrate type merge if source is fully synced'. * subversion/libsvn_client/merge.c (operative_revs_receiver): New svn_log_entry_receiver_t. (find_last_merged_location): Add new svn_client__pathrev_t argument tracking the target. If the target's mergeinfo shows that a previous merge has been performed from the source to the target, then use svn_client_mergeinfo_log2() to determine if there are any inoperative revisions younger than the last merged location determined strictly by the target's mergeinfo. (find_base_on_source, find_base_on_target): Update calls to find_last_merged_location(). * subversion/include/svn_client.h (svn_client_mergeinfo_log2): Add new limit argument similar to that in svn_client_log5(). (svn_client_mergeinfo_log): Doc string tweak to reflect change to wrapped API. * subversion/libsvn_client/deprecated.c (svn_client_mergeinfo_log): Update call to svn_client_mergeinfo_log2. * subversion/libsvn_client/mergeinfo.c (filter_log_entry_baton_t): Add new members to implement new limit argument to svn_client_mergeinfo_log2(). (filter_log_entry_with_rangelist): Stop calling wrapped log receiver if limit has been reached. Note: This is fairly ugly because *this* callback is still called for all the remaining ranges! (logs_for_mergeinfo_rangelist): Add new argument tracking the LIMIT arg to svn_client_mergeinfo_log2(). (svn_client_mergeinfo_log2): Implement new limit argument. * subversion/svn/mergeinfo-cmd.c (svn_cl__mergeinfo): Update calls to svn_client_mergeinfo_log2(). * subversion/tests/cmdline/merge_automatic_tests.py (effective_sync_results_in_reintegrate): Remove XFail decorator and tweak comment re failure status. * subversion/tests/cmdline/merge_reintegrate_tests.py (reintegrate_with_subtree_merges): Expand test to cover issue #4329.
4329.12.diff
Description: Binary data
