Re: [PATCH] Split up the reintegrate merge API: first find what to do, then do it - v2

Responses in-line, but first a reminder of why I'm doing this.

It's because I've been looking at making 'svn merge' give better diagnostics and confirmations.  Things like issuing an error or warning if the source and target are in fact the same 'branch', or if the user is trying to reintegrate in the wrong direction (the same direction as some sync merges have been done).  To do this, the client needs to ask questions like 'what branches and revisions would this merge affect?' before executing the merge.  Similar issues apply to all kinds of merges, but I happen to have taken the reintegrate first, since it's known to be a wrapper for a two-URL merge.

I expect kind of control this would be especially valuable for GUIs, and I would appreciate feedback on this matter.

I don't suggest that every client should be required to execute the steps separately.  I think we should keep the 'svn_client_merge_reintegrate' API as a shortcut, and not deprecate it.  Opinions welcome.

Daniel Shahaf wrote:

> OK.  I can't find reintegrate_merge_locked(),

Oops, I meant 'merge_reintegrate_locked'.

And:
> Julian Foad wrote:
>> >
>> >     svn_client_find_reintegrate_merge(&url1, &rev1, &url2, &rev2, ...);
>> >     svn_client_merge4(url1, rev1, url2, rev2, ...);
>>
>> One issue is that there is some duplication of work in this
>> formulation.  Both of these functions check that the target WC is
>> suitable for merging into.  Both open up RA sessions.  Both determine
>> the youngest common ancestor.
>
> I don't think "opening an RA session" is a problem for a function
> that's about to do a merge operation :)

Yes, even for a small merge the cost of that can't be of much importance.

> Checking the WC's sanity, presumably that's necessary since the WC may
> be modified after svn_client_find_reintegrate_merge() but before
> svn_client_merge4()?  Especially if the user is prompted to +1 the
> output of svn_client_find_reintegrate_merge() before the actual merge
> is done.  (and then goes to lunch, comes back, makes some commits, finds
> a loose TSVN dialog and OKs it)

Good thoughts.  The current situation in this patch (v2) is that step 1 (_find_reintegrate_merge) checks the WC is:

  * single-revision
  * unmodified
  * unswitched

and step 2 (_merge4) checks the WC is:

  * single-revision

because that's all that a normal (non-reintegrate) invocation of svn_client_merge4() needs.

If step 2 could repeat the same check as step 1, that would be useful for your scenario, but otherwise there's little point in step 2 doing a partial check.  So for now I'll take the simple solution which is to call svn_client_merge4(allow_mixed_rev=TRUE) to bypass the check.

If and when we want to perform the full check in step 2, then we'll want to alter svn_client_merge4() in some way to provide more control over those checks, or else expose the check function separately.  The best way might become clearer with further work on the APIs.

- Julian
Received on 2011-12-12 11:51:54 CET