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

Discussing the behaviour of the backward-compatibility APIs:

Bert wrote:
> I don't think the strict ordening will be a problem for old clients at the
> client level. We don't promis ordering in the ra layers anyway and we have
> changed the ordering many times before. As long as we call the callbacks I
> think clients would be fine. And it will solve the existing problem of
> broken operations when the network layer times out on a long callback
> invocation.

I am not concerned specifically about the *order* of the notifications, but rather the *kind* of notifications.  The notification callbacks are now going to have their status field set to (the API representation of) 'conflicted', like this (showing the output from 'svn' just as a way to represent what happens in the API):

  $ svn update --accept=mf
  Updating...
   C   wc/A2/B
   C   wc/A2/mu
  Resolved conflicted state of 'wc/A2/B'
  Resolved conflicted state of 'wc/A2/mu'

An old client expecting notification status 'updated' or 'merged', like this:

  $ svn update --accept=mf
  Updating...
   U   wc/A2/B
   U   wc/A2/mu

... may be confused if it actually looks at the notification status rather than merely displaying it to the user.

Bert clarified on IRC [1] that he feels typical clients won't care about this sort of change, and that we've already made comparable changes for 1.8 and in earlier minor-version releases, and that it would be unreasonably difficult to avoid any changes to notifications.  The implication is it's not really worth making the effort to keep the notifications exactly the same in the backward-compatibility APIs.  Bert, please correct me if I misrepresented your views.

Mark Phippard said that for Subclipse, he would probably need to make some changes to support 1.8, but that the backward compatibility issue doesn't matter because each version of Subclipse is tied to exactly one minor version of the Subversion libraries.

Advantages of putting compatibility code in to call the resolver function at the old times and thus give the old notifications:

  - Back-compat.

Advantages of not putting the extra compatibility in:

  - Simplicity.

Anyone else have a view?

For now I'll commit what I have, and if we want the extra compatibility I'll add it later.

- Julian

[1] #svn-dev IRC channel, today, logged at <http://colabti.org/irclogger/irclogger_log/svn-dev?date=2013-03-27#l247>.

> Julian Foad wrote:
> Stefan Sperling wrote:
>
>> On Tue, Mar 26, 2013 at 04:26:33PM +0000, Julian Foad wrote:
>>>  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.
>>
>> I agree that this makes more a whole lot more sense, and would
>> like to see the 1.8 API behave this way, if GUI clients can deal
>> with it.
>>
>> What about third-party callers that call the 1.7 and earlier APIs?
>> Their callbacks will be called at a different time when they run
>> against 1.8 libs, won't they? Is this a problem?
>
> Good point.  I think we should preserve the behaviour of the old APIs exactly.  I'll make that happen.
>
>>>  This changes the notifications a bit, as mentioned in the log message
>>> (which is in the patch file).
>>
>> This might also be a backwards-compatibility concern.
>>
>> If possible, we should try to keep the old APIs working as-is.
>> I think this was part of the rationale for doing this within 'svn'.
>
> As above.  The changes in notification are very closely tied to the changes in when the CB is called.  I'll make both happen the old way when calling the old API.
>
> This will extend the patch a bit, and result in bumping the svn_client_update/switch API versions.
>
> - Julian
Received on 2013-03-27 17:23:04 CET