On Fri, Mar 22, 2013 at 2:39 PM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> Paul Burba wrote:
>
>> On Fri, Mar 8, 2013 at 4:17 PM, Paul Burba <ptburba_at_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
Received on 2013-04-03 18:52:09 CEST