Re: [PATCH] Update and switch APIs call conflict resolver at end of operation

--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: Julian Foad <julianfoad_at_btopenworld.com>
> To: Subversion Development <dev_at_subversion.apache.org>
> Cc: StefanSperling <stsp_at_elego.de>; Bert Huijben <bert_at_qqmail.nl>; Stefan Küng <tortoisesvn_at_gmail.com>
> Sent: Tuesday, 26 March 2013, 12:26
> Subject: [PATCH] Update and switch APIs call conflict resolver at end of operation
> 
> Following on from the fix for issue #4316 "Merge errors out after resolving 
> conflicts" r1459012, in which I made the merge APIs call the conflict 
> resolver callback for all conflicts before returning, I mentioned that we should 
> do the same for update and switch [1].  I'll explain a bit more.
> 
> Currently, subversion/svn/update-cmd.c:svn_cl__update() does this:
> 
>   * Call svn_client_update4(...)
> 
>     - with ctx->conflict_func2 set to svn_cl__conflict_func_postpone()
>       which records the conflicted paths but does not resolve them.
> 
>   * Call svn_cl__resolve_postponed_conflicts()
> 
>     - which calls svn_client_resolve()
> 
>     - with ctx->conflict_func2 set to svn_cl__conflict_func_interactive()
>       which does interactive or non-interactive (pre-specified) resolution.
> 
> There's something wrong with this usage pattern.  svn_client_update4() 
> claims to support a conflict resolver callback, and yet 'svn' -- a 
> really simple client application -- doesn't like the way it works, and 
> instead uses the callback just to collect a list of paths and then runs its own 
> conflict resolution loop instead.  Why?
> 
> AFAIK, the two main reasons are:
> 
>   - If the client is going to do interactive resolution, that could take a long 
> time, and so the client prefers to wait until all of the repository-contacting 
> phase of the update is complete, to avoid network timeouts.
> 
>   - When resolving a tree conflict, we can be sure that all the incoming changes 
> have been received, so that if we need to look at more than one path (such as 
> for a tree conflict involving a copy or a move) then we know that the changes 
> have been received for any path we need to look at.
> 
> Other reasons are consistency (by making update and switch work the same as 
> merge, we can get rid of some support code in 'svn') and making this 
> logic available to all clients (even if GUI clients, for example, may not use 
> this).
> 
> I now intend to make libsvn_client do the resolving (I mean call the resolver 
> callback) at the end of the update, for each conflicted path.  And 
> 'switch', of course.  I see this as a development of the idea that 
> resulted in 'svn' doing the two-stage think it is presently doing.
> 
> With this patch, subversion/svn/update-cmd.c:svn_cl__update() will do this:
> 
>   * Call svn_client_update4(...)
> 
>     - with ctx->conflict_func2 set to svn_cl__conflict_func_interactive()
>       which does interactive or non-interactive (pre-specified) resolution.
> 
>     - which calls the callback after completing the update, before returning.
> 
> This changes the notifications a bit, as mentioned in the log message (which is 
> in the patch file).
> 
> I will commit this soon if no objections.
I'll tweak the patch to avoid asking the WC to call the little record_conflict() helper if ctx->conflict_func2 is null, because in that case we don't need a list of conflicted paths.  That will save a bit of processing in the WC code.
- Julian