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 <ptburba_at_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
Received on 2013-04-04 17:08:50 CEST