You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2013/03/26 17:26:33 UTC
[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.
- Julian
[1] At the end of the dev@ message "Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts" by Julian Foad on 2013-03-20, <http://svn.haxx.se/dev/archive-2013-03/0340.shtml>.
--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: [PATCH] Update and switch APIs call conflict resolver at end of operation
Posted by Julian Foad <ju...@btopenworld.com>.
--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: Mark Phippard <ma...@gmail.com>
> To: Julian Foad <ju...@btopenworld.com>
> Cc: "bert@qqmail.nl" <be...@qqmail.nl>; Stefan Sperling <st...@elego.de>; Subversion Development <de...@subversion.apache.org>; Stefan Küng <to...@gmail.com>
> Sent: Wednesday, 27 March 2013, 12:31
> Subject: Re: [PATCH] Update and switch APIs call conflict resolver at end of operation
>
> On Wed, Mar 27, 2013 at 12:22 PM, Julian Foad
> <ju...@btopenworld.com> wrote:
>> 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.
>
> Just setting aside API users for a minute. Is the first example above
> what the command line will show?
Yes.
> If we are happy with that output for command line users, then as an API
> user I am sure I can make it work for me.
>
> As a command line user, I am not sure what I think. I imagine some
> users will be confused by this and ask questions about the behavior.
> But it also makes sense to me, so I do not personally think it is
> something I would be hung up on.
I want to make some improvements to the command-line output.
First, I don't like the arbitrary ordering of resolver callbacks. For a pre-specified resolution, the order in which the updates arrived (so the order of the 'C' notifications) would be better. For interactive resolution, either that or preferably lexicographical order of the pathnames would be better. It doesn't matter in a trivial case like I showed here where there are just two files, but for interactive resolving of say 20 conflicts I would like my brain to be able to context-switch into one subdirectory at a time rather than jumping from one path to another at random.
Since the API doesn't know whether the callback is for interactive resolution or not, I propose to sort them into lex. order of paths. Even if there are thousands of conflicted paths, this step should take insignificant time compared with the total update/switch/merge operation time.
Second, we can improve the output shown above, for pre-specified resolution, by making the 'Resolved' lines say what they actually did and why, perhaps like this:
> $ svn update --accept=mf
> Updating...
> C wc/A2/B
> C wc/A2/mu
> Resolving all conflicts as 'mine-full'...
> Resolved conflict on 'wc/A2/B'
> Resolved conflict on 'wc/A2/mu'
> If we are happy with this behavior for the command line output, then I
> am OK with it at the API layer. Thanks for the example though. I do
> now have a better understanding of the question.
OK, glad to hear it.
- Julian
Re: [PATCH] Update and switch APIs call conflict resolver at end of operation
Posted by Mark Phippard <ma...@gmail.com>.
On Wed, Mar 27, 2013 at 12:22 PM, Julian Foad
<ju...@btopenworld.com> wrote:
> 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.
Just setting aside API users for a minute. Is the first example above
what the command line will show? If we are happy with that output for
command line users, then as an API user I am sure I can make it work
for me.
As a command line user, I am not sure what I think. I imagine some
users will be confused by this and ask questions about the behavior.
But it also makes sense to me, so I do not personally think it is
something I would be hung up on.
If we are happy with this behavior for the command line output, then I
am OK with it at the API layer. Thanks for the example though. I do
now have a better understanding of the question.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: [PATCH] Update and switch APIs call conflict resolver at end of operation
Posted by Julian Foad <ju...@btopenworld.com>.
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
Re: [PATCH] Update and switch APIs call conflict resolver at end of operation
Posted by be...@qqmail.nl.
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.
Bert
Sent from Windows Mail
From: Julian Foad
Sent: Tuesday, March 26, 2013 9:18 PM
To: Stefan Sperling
Cc: Subversion Development; Bert Huijben; Stefan Küng
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
Re: [PATCH] Update and switch APIs call conflict resolver at end of operation
Posted by Julian Foad <ju...@btopenworld.com>.
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
Re: [PATCH] Update and switch APIs call conflict resolver at end of
operation
Posted by Stefan Sperling <st...@elego.de>.
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?
> 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'.
Re: [PATCH] Update and switch APIs call conflict resolver at end of operation
Posted by Julian Foad <ju...@btopenworld.com>.
--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: Julian Foad <ju...@btopenworld.com>
> To: Subversion Development <de...@subversion.apache.org>
> Cc: StefanSperling <st...@elego.de>; Bert Huijben <be...@qqmail.nl>; Stefan Küng <to...@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