On Fri, Mar 22, 2013 at 2:39 PM, Julian Foad <julianf...@btopenworld.com> wrote: > Paul Burba wrote: > >> On Fri, Mar 8, 2013 at 4:17 PM, Paul Burba <ptbu...@gmail.com> wrote: >>> On Tue, Mar 5, 2013 at 11:37 AM, Julian Foad wrote: >>>> 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? >> >> fyi http://subversion.tigris.org/issues/show_bug.cgi?id=4329#desc7 >> explains my proposed patch in terms of how it changes the symmetric >> merge algorithm. > > Hi Paul. That did help, yes. > > It took me a while to get my head back into this stuff, but yes it looks like > this hangs together theoretically as well as fixing the immediate use case. > > As for the implementation, in find_last_merged_location() the task is to find > the last location on SOURCE_BRANCH such that all changes on SOURCE_BRANCH > after YCA up to and including *BASE_P have already been merged into the > target branch. The function finds the first eligible revision, considering > mergeinfo only on the branch root, and then you add a call to > svn_client_mergeinfo_log2() to find the first eligible revision after that, > considering subtree mergeinfo and considering only operative changes. Two > questions: > > - Why do you only call svn_client_mergeinfo_log2() if there has been a > merge from SOURCE to TARGET recorded at the root of the branch? That would > seem to miss the cases where there have only been subtree merges.
You are quite right. This can be done entirely with svn_client_mergeinfo_log2() and catch the case where only subtree merges have been performed, but are "complete" (do we have an official term to describe the situation where a given subtree merge of rX is effectively the same as a merge of rX to the branch root?). > - Why not just use svn_client_mergeinfo_log2(), if it answers the question, > and delete all the prior code that first looks at just the branch-root > mergeinfo? If the initial look at branch-root mergeinfo is a performance > optimization, then it should be inside svn_client_mergeinfo_log2(), I would > assume. I reworked the patch to do just this and committed it in r1464102. > You might consider committing the part that adds a 'limit' argument to a > mergeinfo API as a separate change, as it looks like a fairly large part of > the patch. But if we can use svn_client_mergeinfo_log2() to answer the whole > question in one go, then it probably won't need the limit argument anyway. r1464102 simply raises a SVN_ERR_CEASE_INVOCATION error in the svn_log_entry_receiver_t callback. This, combined with my earlier commit (r1463237) to allow to ask for revisions in youngest:oldest or oldest:younger order, means that there is no need for the limit code; we can call svn_client_mergeinfo_log2() in such a way as to find the youngest merged revision or the oldest elibigle revision in the first callback. What I've struggled with is here is the (not unexpected) catch: Using svn_client_mergeinfo_log2(), rather than simply looking at the mergeinfo on the branch roots, may be more correct, but obviously takes more time. During my ad hoc testing with some of our own branches, it adds anywhere from 5 to 25 seconds of real time. I'm still looking if there are any places we can claw back some of that performance loss before 1.8...but after a long history of speeding up merge since 1.5, we might have to take a performance hit in the name of correctness. -- Paul T. Burba CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development Skype: ptburba
