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@PEG on branch URL@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
