On Wed, Jan 18, 2012 at 8:45 AM, Julian Foad <julianf...@btopenworld.com> wrote: > Hi Paul. > > Paul Burba wrote: > >> Julian Foad wrote: >>> I (Julian Foad) wrote: >> I think I understand now. Your patch works fine when the only cyclic >> merges are done via reintegrate (i.e. r24). > > [...] > >> I thought you were going to tackle more complex reflective merge >> cases, such as this example: > > > Yes, exactly! > >> A@1---r4---r5---r6---r7----------------r11-----------> >> |\ ^ | >> | \ | | >> | \ reintegrate | >> | V | | >> | A_COPY_2-----------------r9---r10--- | >> | ^ sync merge >> | / | >> | cherry-pick merge of r8 | >> V / V >> A_COPY-------------------r8-------------------------> > > > Thanks for this good example of a more complex use case. I've included it in > <http://wiki.apache.org/subversion/KeepingReintegratedBranchAlive> and > written up a discussion of the options there. > > What do we want in this case? The options are basically: > > (1) try to merge (as we do now), despite knowing it will conflict; > (2) skip r11 (not usually good in this kind of case); > (3) figure out what really "should" be merged and merge it; > > (4) stop and tell the user what's up. > > I'm not aiming for (3) because I think that's much more complex than it > sounds.
Actually it *sounds* pretty complex ;-) Seriously, if you solve #3 then I think you will also have solved issue #2897. > With any other option, I believe it is essential to tell the user why > Subversion can't simply include or exclude r11, with details. > > > Any of these options could be best for the user. It is not at all the case > that (1) is best. It depends how big the changes in r8/r9 are compared with > those in r10, and how much they conflict, and how the user prefers to resolve > the situation. > > I'm aiming for: > > * Detect whether we have the simple case (the rev is effectively already > merged) or the complex case (partially merged). > * Simple case: Do The Right Thing which is skip it but record it as merged. > > * Complex case: let user choose (1) or (2) or (4), in advance or > interactively. +1 > * Complex case: always issue a diagnostic message that clearly explains > what's up. > >> As your patch stands, the above use-case causes some serious problems. > > Sure, my last patch only detects a Go/No-go and chooses No-go because that's > sufficient for the simple use case. But now we can get to the interesting > bit: how to detect the "Partially merged" situation. Sorry, I was initially unclear on how far you intended to take this. I'm with you now. > The principle that we are grasping as humans but haven't yet codified > formally is that we want the merge to know whether all the "original changes" > have been made already (directly or via any series of merges) in the target > branch. > > I describe the idea of "logical changes" in > <http://wiki.apache.org/subversion/MergeTrackingIdeas>. Most of the ideas there make sense, though I'm a bit foggy as to how the mergeinfo format will change and how we'll migrate existing repositories. > Briefly: When we make a new change in A@10 and merge that into B as B@12, we > get two physical changes in the repository. B@12 is a physical manifestation > of the same logical change as A@10, but it looks a bit different because it > is adjusted (automatically and/or manually) to fit the physical form and > logical requirements of branch B. I say that there is one "logical change" > there, which we refer to by the co-ordinates where it originated, A@10. > > Whenever we ask Subversion to track merges, although the current > implementation just tracks physical changes from the immediate source branch, > I believe that what we (as users) really expect is for it to track logical > changes. The "partial merge" use case above is an example of where we have > to make a distinction and explicitly ask whether all the required logical > changes are present. > > ALGORITHM > > Purpose: To determine whether all logical changes comprising a given physical > change (e.g. A@11) are already present in the target. > > Step 1: build a tree of merged revisions (i.e. mergeinfo changes), recursing > until we reach "logical changes". I know you are still early in the proof-of-concept phase, but have you given much thought to performance? In particular I'm thinking of how mergeinfo_graph_populate recursively scans the history of reflective merges to build the mergeinfo graph. In doing so it ultimately calls svn_client__get_repos_mergeinfo twice for each revision (operative or not) described in the reflected mergeinfo. That could get slow in a hurry. > Step 2: traverse the tree of merged revisions, seeing whether all of the > logical changes that comprise A@11 are recorded in the mergeinfo of the > target (A_COPY). > > While doing this we need to ignore no-op revisions because we don't care > whether they are recorded in the target or not. > > I've written code for step 1 (the attached 'merge-avoid-reflective-5.patch' > is my current state) and am starting on step 2. I look forward to see what you come up with for step 2. Paul >> (The attached patch is an update of yours with a new > >> merge_reintegrate.py test #20, which demonstrates the above use case) > > Thanks; that's very useful. > > > - Julian
