Re: Problems Reintegrating the fsfs-format7 branch

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.

  - 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.

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.

- Julian
Received on 2013-03-22 19:39:47 CET