Re: Fold the merge_automatic API into the existing merge_peg API

On 28.03.2013 21:38, Julian Foad wrote:
> Now with a patch for discussion.
>
> (More below...)
>
> Julian Foad wrote:
>>> Brane told me that while updating the JavaHL API he noticed that the new
>>> svn_client_merge_automatic() is Yet Another Merge API, and would prefer make
>>> the existing JavaHL merge API encompass that functionality rather than add
>>> yet another variant. So I said if let's see if we can apply that idea to the
>>> libsvn_client API.
>>>
>>> Idea: 'automatic' merge is (in a high level logical sense) most similar to
>>> a subset of the 'pegged' merge. So make 'merge_peg' do the automatic merge
>>> (like calling the automatic merge API) if the params are suitable. The
>>> intention is that this should be easier for Subversion client developers to
>>> understand and for them to DTRT.
>>>
>>> API differences between pegged and automatic merge:
>>>
>>> Pegged merge Automatic merge
>>>
>>> The 'find'API: Single merge_peg call does Separate'find' and 'do'..
>>> the whole process. Result of 'find' ... used
>>> used for 'svn mergeinfo' graph.
>>>
>>> Source: Revision ranges X:Y[,X:Y...] Revision range 0:Y
>>> on branch URL_at_PEG on branch URL_at_PEG
>>>
>>> Target: optional non-WC tgt for 'find'
>>>
>>> If mergeinfo: skip already-merged revs (same)
>>> record the merge
>>>
>>> No mergeinfo: don't skip revs error out
>>> don't record
>>>
>>> Options differ: allow_mixed_rev allow_mixed_rev
>>> allow_local_mods
>>> allow_switched_subtrees
>>> ignore_mergeinfo
>>>
>>> At that level, it looks close enough to be feasible to embed
>>> 'automatic' merge inside 'pegged'. I'm looking closer now.
>>>
>>> Any thoughts?
> Branko Čibej wrote:
>> This is pretty much the same conclusion I came to earlier today. I looks
>> like the easiest thing to do would be to make the _automatic_ APIs
>> private within libsvn_client, move the selection logic to
>> svn_client_merge_pegX, and simplify the client implementation.
>>
>> The only trouble with that is, as you say, that "svn mergeinfo" relies
>> on having the automatic_find API available. I think this could be solved
>> in several ways:
>>
>> 1. by splitting the merge-peg into two, as the current automatic-merge; or,
>> 2. by making only the "do" phase of the automatic merge private, and
>> wile leaving the "find" phase public -- but renaming it to something
>> more in line with merginfo discovery; or,
>> 3. somehow exposing the "find" phase through one of the existing (or
>> revised) svn_client_mergeinfo_* APIs.
>>
>> I'm kind of leaning towards #3, but don't have a sense of how
>> complicated that would be.
> Mark Phippard wrote:
>> There are obviously some benefits for having less API, especially when
>> it comes time to rev it for something. That said, as a caller of the
>> API, it seems nice that the separate "automatic merge" API can provide
>> a more focused and simpler interface. For example, the interface does
>> not need to expose things like a revision range, which should not
>> apply when doing this kind of merge, but obviously is needed when
>> doing a cherry-pick merge.
>>
>> Looking through a long list of API is hard sometime, but it is also
>> hard when you want to do something like merge everything in BranchX
>> and the interface requires passing a bunch of parameters that do not
>> directly apply.
> I like the focused API, but I also see how the automatic merge can be considered to fill in a bit of missing functionality that merge_peg ought to provide.
>
> Perhaps we can have both. Teach merge_peg to DTRT in this case, and still have the focused API available for when a client knows it wants an automatic merge. Is there sufficient merit in that to outweigh the overhead of having to test two similar but different entry points?
>
> The attached patch moves the decision to call the 'automatic merge' API from 'svn' into 'svn_merge_peg5()'. I have run some merge tests and tree conflict tests, but not the whole test suite yet. Here is the log msg.
>
> [[[
> Teach svn_client_merge_peg5() to do an automatic merge, and let 'svn merge'
> rely on that instead of calling the dedicated automatic merge APIs.
>
> TODO: Decide whether to keep or make private the 'automatic merge' APIs.
> TODO: This reduces the verbosity of 'svn merge --verbose'. Consider
> doing something about it, perhaps by adding some new notifications for the
> notifier callback?
>
> * subversion/include/svn_client.h,
> subversion/libsvn_client/merge.c
> (svn_client_merge_peg5): Do an automatic merge if no revision range
> specified.
>
> * subversion/svn/merge-cmd.c
> (automatic_merge): Delete.
> (run_merge): Don't take special action to handle an automatic merge, let
> the pegged merge code path handle it.
> ]]]
>
> Thoughts?

I like it. Apparently the encapsulation is even simpler than I expected.

For JavaHL, a simple overload of ISVNClient.merge can provide the
"focused" interface without inventing yet another type of merge API.
Even better, passing null for the revision ranges in the existing
merge-peg overload can be made to yield the same effect, without
affecting the API signature at all. (Currently IIUC passing a null
ranges array will cause an error.)

-- Brane

-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com