Paul Burba wrote: > On Fri, Mar 22, 2013 at 2:39 PM, Julian Foad wrote: >> Paul Burba wrote: >>> On Fri, Mar 8, 2013 at 4:17 PM, Paul Burba <ptbu...@gmail.com> wrote: >>>> 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.
Fantastic. Thanks, Paul. For the record, this resolves the two issues <http://subversion.tigris.org/issues/show_bug.cgi?id=4258> "Automatic merge after subtree merge in opposite direction" and <http://subversion.tigris.org/issues/show_bug.cgi?id=4329> "automatic merge uses reintegrate type merge if source is fully synced". (You've already updated them, I just want to mention them here.) > 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. One candidate for optimisation is that I added a check whether the source and target branches are related, which is called from 'svn' before the merge API is called. This check involves retrieving the location-segment histories of the two branch root dirs. We then throw away these location histories and retrieve them again later in the merge. We should store them and re-use them. (I'm pretty sure they were already being fetched more than once even before I added this relatedness check.) - Julian
