You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by lu...@apache.org on 2016/10/14 14:07:58 UTC
svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c
tests/cmdline/resolve_tests.py
Author: luke1410
Date: Fri Oct 14 14:07:58 2016
New Revision: 1764902
URL: http://svn.apache.org/viewvc?rev=1764902&view=rev
Log:
Fix svn resolve --accept=working not working on binary conflicts by retrying
to find a resolution option for the semantically corresponding
svn_client_conflict_option_working_text option.
Additionally, enable the mf option for binary conflicts, so to have the
option displayed in the client.
* subversion/libsvn_client/conflicts.c
(svn_client_conflict_text_get_resolution_options): return the more
suitable
svn_client_conflict_option_working_text to resolve binary conflicts.
(svn_client_conflict_text_resolve_by_id): retry determining resolution
option with svn_client_conflict_option_working_text.
* subversion/svn/conflict-callbacks.c
(handle_text_conflict): add "mf" to the list of allowed resolutions for
binary conflicts.
* subversion/tests/cmdline/resolve_tests.py
(automatic_binary_conflict_resolution): remove XFail marker
Approved by: stsp
Modified:
subversion/trunk/subversion/libsvn_client/conflicts.c
subversion/trunk/subversion/svn/conflict-callbacks.c
subversion/trunk/subversion/tests/cmdline/resolve_tests.py
Modified: subversion/trunk/subversion/libsvn_client/conflicts.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/conflicts.c?rev=1764902&r1=1764901&r2=1764902&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/conflicts.c (original)
+++ subversion/trunk/subversion/libsvn_client/conflicts.c Fri Oct 14 14:07:58 2016
@@ -7359,7 +7359,7 @@ svn_client_conflict_text_get_resolution_
resolve_text_conflict);
add_resolution_option(*options, conflict,
- svn_client_conflict_option_merged_text,
+ svn_client_conflict_option_working_text,
_("Mark as resolved"),
_("accept binary file as it appears in the working copy"),
resolve_text_conflict);
@@ -8824,19 +8824,36 @@ svn_client_conflict_text_resolve_by_id(
{
apr_array_header_t *resolution_options;
svn_client_conflict_option_t *option;
+ const char *mime_type;
SVN_ERR(svn_client_conflict_text_get_resolution_options(
&resolution_options, conflict, ctx,
scratch_pool, scratch_pool));
option = svn_client_conflict_option_find_by_id(resolution_options,
option_id);
+
+ /* Support svn_client_conflict_option_merged_text for binary conflicts by
+ * mapping this onto the semantically equal
+ * svn_client_conflict_option_working_text.
+ */
if (option == NULL)
- return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
- NULL,
- _("Inapplicable conflict resolution option "
- "given for conflicted path '%s'"),
- svn_dirent_local_style(conflict->local_abspath,
- scratch_pool));
+ {
+ if (option_id == svn_client_conflict_option_merged_text) {
+ mime_type = svn_client_conflict_text_get_mime_type(conflict);
+ if (mime_type && svn_mime_type_is_binary(mime_type))
+ option = svn_client_conflict_option_find_by_id(resolution_options,
+ svn_client_conflict_option_working_text);
+ }
+ }
+
+ if (option == NULL)
+ return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
+ NULL,
+ _("Inapplicable conflict resolution option "
+ "given for conflicted path '%s'"),
+ svn_dirent_local_style(conflict->local_abspath,
+ scratch_pool));
+
SVN_ERR(svn_client_conflict_text_resolve(conflict, option, ctx, scratch_pool));
return SVN_NO_ERROR;
Modified: subversion/trunk/subversion/svn/conflict-callbacks.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/conflict-callbacks.c?rev=1764902&r1=1764901&r2=1764902&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/conflict-callbacks.c (original)
+++ subversion/trunk/subversion/svn/conflict-callbacks.c Fri Oct 14 14:07:58 2016
@@ -933,10 +933,9 @@ handle_text_conflict(svn_boolean_t *reso
if (knows_something || is_binary)
*next_option++ = "r";
- /* The 'mine-full' option selects the ".mine" file so only offer
- * it if that file exists. It does not exist for binary files,
- * for example (questionable historical behaviour since 1.0). */
- if (my_abspath)
+ /* The 'mine-full' option selects the ".mine" file for texts or
+ * the current working directory file for binary files. */
+ if (my_abspath || is_binary)
*next_option++ = "mf";
*next_option++ = "tf";
Modified: subversion/trunk/subversion/tests/cmdline/resolve_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/resolve_tests.py?rev=1764902&r1=1764901&r2=1764902&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/resolve_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/resolve_tests.py Fri Oct 14 14:07:58 2016
@@ -602,7 +602,6 @@ def multi_range_merge_with_accept(sbox):
# Test for issue #4647 'auto resolution mine-full fails on binary file'
@Issue(4647)
-@XFail()
def automatic_binary_conflict_resolution(sbox):
"resolve -R --accept [base | mf | tf] binary file"
Re: svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c
tests/cmdline/resolve_tests.py
Posted by Stefan Hett <st...@egosoft.com>.
Just a quick heads up since I didn't get to reply to this one yet:
Thanks for your review, Evgeny.
On 10/14/2016 6:24 PM, Evgeny Kotkov wrote:
> Stefan Hett <lu...@apache.org> writes:
>
>> if (option == NULL)
>> - return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
>> - NULL,
>> - _("Inapplicable conflict resolution option "
>> - "given for conflicted path '%s'"),
>> - svn_dirent_local_style(conflict->local_abspath,
>> - scratch_pool));
>> + {
>> + if (option_id == svn_client_conflict_option_merged_text) {
>> + mime_type = svn_client_conflict_text_get_mime_type(conflict);
>> + if (mime_type && svn_mime_type_is_binary(mime_type))
>> + option = svn_client_conflict_option_find_by_id(resolution_options,
>> + svn_client_conflict_option_working_text);
>> + }
>> + }
> Looks like the indentation and the brace formatting are broken here and
> in the next hunk.
Right, my bad. Already wrote a patch to correct this indentation issue
and the other one on Sunday on the train but didn't get to send it yet.
Meanwhile I see that stsp already corrected (and simplified) that code
section. Thanks for that, stsp.
>
>> +
>> + if (option == NULL)
>> + return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
>> + NULL,
>> + _("Inapplicable conflict resolution option "
>> + "given for conflicted path '%s'"),
> As for the change itself, I don't think that silently transforming one
> conflict option id to another in svn_client_conflict_text_resolve_by_id()
> API is a proper thing to do, because we don't expose this option id as a
> viable resolution option in svn_client_conflict_text_get_resolution_options().
>
> I think that a better way would be to handle this case in the command line
> by using svn_client_conflict_option_working_text for binary files, if
> we run with --accept=working.
I'll get back to that with some details and will double check the
current approach, as soon as I can free up some time here (not sure
whether this will be this week though). Only got half through the review
on Sunday unfortunately.
--
Regards,
Stefan Hett, Developer/Administrator
EGOSOFT GmbH, Heidestrasse 4, 52146 W�rselen, Germany
Tel: +49 2405 4239970, www.egosoft.com
Gesch�ftsf�hrer: Bernd Lehahn, Handelsregister Aachen HRB 13473
Re: svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c tests/cmdline/resolve_tests.py
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
>> Great work :)
>
> Thanks. I'll commit the patch sometime tomorrow.
Committed in r1769269.
Regards,
Evgeny Kotkov
Re: svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c tests/cmdline/resolve_tests.py
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Stefan Sperling <st...@elego.de> writes:
> So basically the 1.10 svn client is now doing the reverse-mapping of
> conflict_option_id_to_wc_conflict_choice() in libsvn_client/conflicts.c
> with some extra tweaks.
> Perhaps other clients than the command line client won't have such
> problems, because they can adapt their end-user interfaces without
> worrying so much about existing scripts?
We are talking about third-party clients exposing options similar to
--accept=base/working/mine-conflict/... that would like to start using
the new API, right?
I don't see a big problem here. Since switching to the new API requires
changing the code, in the worst case developers could just do the same
thing as in svn_cl__resolve_conflict().
What I find important is keeping these kinds of mappings localized,
instead of doing one part in the command line client, and then fixing
things within the libsvn_client API. And I think the API shouldn't
contradict itself by allowing to pass an option id that isn't a part
of the allowed resolution options, and that it shouldn't be trying to
outsmart the user in this case.
> Great work :)
Thanks. I'll commit the patch sometime tomorrow.
Regards,
Evgeny Kotkov
Re: svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c
tests/cmdline/resolve_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 10, 2016 at 07:34:09PM +0300, Evgeny Kotkov wrote:
> Here is the patch. Not sure that I fully understand the point about the
> number of iterations, though ;)
Ah, yes. I see what you're doing. Unravelling the accept arg during
the conflict walk instead of before the walk is very nice.
To explain where my scepticism came from:
Historically the conflict walker was inside libsvn_wc, and this code
evolved from there. It was difficult to do what you're doing now
before we created a new 1.10 conflict walker separate from the walker
in the libsvn_wc library. In the early stages of 1.10 conflict development
the walk inside libsvn_wc was supporting both 1.9 and 1.10 clients,
so the new conflict code had to deal with whatever values clients
were passing in via the legacy conflict callback.
Which is also why the initial set of svn_client_conflict_option_t
values were deliberately aligned with svn_wc_conflict_choice_t.
Since 1.9 clients will still be supported by the libsvn_wc library
and will keep getting the same semantics, I don't see any backwards
compat concern which my brain somehow thought was still there.
What is kind of annoying is that libsvn_client is still transforming
option_id inputs back to accept-style arguments for libsvn_wc, at
least for text and prop conflicts. But changing that is a huge effort.
So basically the 1.10 svn client is now doing the reverse-mapping of
conflict_option_id_to_wc_conflict_choice() in libsvn_client/conflicts.c
with some extra tweaks.
Perhaps other clients than the command line client won't have such
problems, because they can adapt their end-user interfaces without
worrying so much about existing scripts?
Great work :)
Re: svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c tests/cmdline/resolve_tests.py
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Stefan Sperling <st...@elego.de> writes:
> There's a similar hack for resolving tree-conflicts with --mine-conflict,
> which the client API does not expose as a valid option either.
>
> I'd say leave these as-is. We won't accumulate more such hacks. They
> are clearly marked as hacks in comments, and they exist only because
> we have to remain compatible with the old conflict system somehow.
I think that it is wrong to add a hack based on having a similar hack
somewhere, as well as expect this to be the "very last hack" that we
add, because things like this just happen to pile up.
The actual problem here is that we bend the API based on the immediate
needs of the command-line client, and that can cause problems in the
longer run. It's the cl client that should be remapping various --accept
options to svn_client_conflict_option_id_t. And currently we have the
API guessing that, and silently doing something other than what it was
told to do.
Furthermore, I don't see any problems with inverting this (and I think
that the code actually becomes simpler, with all the mapping performed
in one place).
> Then try out your idea and show us your diff ;-)
> Stefan has already gone through several iterations of this patch,
> so I'd rather not ask him to revisit this again.
Here is the patch. Not sure that I fully understand the point about the
number of iterations, though ;)
Log message:
[[[
Remove two hacks from the conflict resolver API that were added to
allow handling --accept=mine-conflict | working for tree conflicts
and --accept=working for binary text conflicts.
This patch makes the command-line client fully responsible for choosing
the appropriate conflict option id, and keeps the various new APIs
such as svn_client_conflict_text_resolve_by_id() simple and doing
just what they were told to do.
* subversion/libsvn_client/conflicts.c
(svn_client_conflict_text_resolve_by_id,
svn_client_conflict_tree_resolve_by_id): Remove hacks from these
functions.
* subversion/svn/conflict-callbacks.c
(resolve_conflict_by_accept_option): Inline parts of this function that
handle svn_cl__accept_edit and svn_cl__accept_launch ...
(svn_cl__resolve_conflict): ...here. Accept svn_cl__accept_t by value,
drop the option id argument. Pick the appropriate option id based on
the passed-in svn_cl__accept_t argument. Prompt the user if there is
no option or if the option did not apply.
* subversion/svn/resolve-cmd.c
(svn_cl__resolve): Handle unsupported --accept [--non-interactive] cases
in this function.
(svn_cl__walk_conflicts): Remove the is_resolve_cmd argument. Don't
map the --accept option to option id here, as we will do it in the
svn_cl__resolve_conflict() function. Adjust calls to walk_conflicts()
and svn_cl__resolve_conflict().
(walk_conflicts): Remove option_id argument, accept svn_cl__accept_t
by value.
(conflict_status_walker): Adjust call to svn_cl__resolve_conflict().
(conflict_status_walker_baton): Remove option_id field, store
accept_which field by value.
* subversion/svn/merge-cmd.c
(svn_cl__merge): Adjust call to svn_cl__walk_conflicts().
* subversion/svn/switch-cmd.c
(svn_cl__switch): Adjust call to svn_cl__walk_conflicts().
* subversion/svn/update-cmd.c
(svn_cl__update): Adjust call to svn_cl__walk_conflicts().
* subversion/svn/cl.h
(svn_cl__resolve_conflict, svn_cl__walk_conflicts): Update declarations.
]]]
Regards,
Evgeny Kotkov
Re: svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c
tests/cmdline/resolve_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Oct 14, 2016 at 06:24:05PM +0200, Evgeny Kotkov wrote:
> As for the change itself, I don't think that silently transforming one
> conflict option id to another in svn_client_conflict_text_resolve_by_id()
> API is a proper thing to do, because we don't expose this option id as a
> viable resolution option in svn_client_conflict_text_get_resolution_options().
There's a similar hack for resolving tree-conflicts with --mine-conflict,
which the client API does not expose as a valid option either.
I'd say leave these as-is. We won't accumulate more such hacks. They
are clearly marked as hacks in comments, and they exist only because
we have to remain compatible with the old conflict system somehow.
> I think that a better way would be to handle this case in the command line
> by using svn_client_conflict_option_working_text for binary files, if
> we run with --accept=working.
Then try out your idea and show us your diff ;-)
Stefan has already gone through several iterations of this patch,
so I'd rather not ask him to revisit this again.
I think a little text/binary conflict style problem isn't worth worrying
about while several important tree conflict resolution options remain
unimplemented.
Re: svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c
tests/cmdline/resolve_tests.py
Posted by Stefan <lu...@gmx.de>.
Hi Evgeny,
it took me a while to get back to this, since I find it still quite
challenging to get my head into this part of the SVN design and needed
some uninterrupted time to fully focus on this topic.
On 10/14/2016 18:24, Evgeny Kotkov wrote:
> Stefan Hett <lu...@apache.org> writes:
>
>> [...]
On the indentation issues (actually related to the code below), I sent a
separate patch-mail to the dev list already earlier: "[PATCH] Correct
indentation in svn_client_conflict_text_resolve_by_id".
>> +
>> + if (option == NULL)
>> + return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
>> + NULL,
>> + _("Inapplicable conflict resolution option "
>> + "given for conflicted path '%s'"),
> As for the change itself, I don't think that silently transforming one
> conflict option id to another in svn_client_conflict_text_resolve_by_id()
> API is a proper thing to do, because we don't expose this option id as a
> viable resolution option in svn_client_conflict_text_get_resolution_options().
>
> I think that a better way would be to handle this case in the command line
> by using svn_client_conflict_option_working_text for binary files, if
> we run with --accept=working.
With the current design it doesn't seem to be too simple to achieve what
you describe. The approach you describe was the first one I tried during
the hackathon. However, I soon realized that it won't work without
significantly changing the API. That led me to a second approach to
simply extend the API of
svn_client_conflict_text_get_resolution_options() to query the
appropriate option directly. Ivan pointed out that this is not good (and
I agree there with him). Therefore, stsp suggested to go with the third
approach (which is the approach presented in r1764902).
To explain the situation in more detail, as far as I understand it
(since I'm still learning the SVN source I might be wrong, so please
correct me, if you see any mistake here):
The entry point for the resolve-command is in resolve-cmd.c:
svn_cl__resolve().
This then passes on the call to svn_cl__walk_conflicts() to walk all
conflicts. The specified accept-option is kept in the opt_state.
svn_cl__walk_conflicts() translates the accept-option to a corresponding
option_id (svn_client_conflict_option_merged_text in this case).
Since up to that point there is no "binary" or "text" conflict state,
you cannot determine which resolution option is the "correct" one to go
with... The current design relies on one option being given for any type
of conflict which can be applied to either type.
The option_id is then being passed to walk_conflicts() where
svn_wc_walk_stats() transitions the call to the WC layer with the
callback function being set to conflict_status_walker(). In that
function svn_cl__resolve_conflict() is called to resolve an actual
conflict based on the already specified option_id.
Only at the point of the data flow you then can determine whether the
given (accept-)option is applied onto a "binary" or a "text" conflict
and that's where the current chosen solution will first determine an
appropriate conflict option based on the retrieved text-resolution
options or (if none exists) will attempt to retrieve a conflict option
for the semantically equal
svn_client_conflict_option_working_text-option (in case of a binary
conflict).
So the options one could think of would be to extend the API and allow
two conflict options to be passed to svn_cl__resolve_conflict() rather
than only a single one (aka: one for plain-text and one for binary-text
conflicts). But this is where I concluded that the complexity of such an
approach is not preferable over one of the other alternatives.
Or did I miss your point and you had something else in mind?
Regards,
Stefan
Re: svn commit: r1764902 - in /subversion/trunk/subversion:
libsvn_client/conflicts.c svn/conflict-callbacks.c tests/cmdline/resolve_tests.py
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Stefan Hett <lu...@apache.org> writes:
> if (option == NULL)
> - return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
> - NULL,
> - _("Inapplicable conflict resolution option "
> - "given for conflicted path '%s'"),
> - svn_dirent_local_style(conflict->local_abspath,
> - scratch_pool));
> + {
> + if (option_id == svn_client_conflict_option_merged_text) {
> + mime_type = svn_client_conflict_text_get_mime_type(conflict);
> + if (mime_type && svn_mime_type_is_binary(mime_type))
> + option = svn_client_conflict_option_find_by_id(resolution_options,
> + svn_client_conflict_option_working_text);
> + }
> + }
Looks like the indentation and the brace formatting are broken here and
in the next hunk.
> +
> + if (option == NULL)
> + return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
> + NULL,
> + _("Inapplicable conflict resolution option "
> + "given for conflicted path '%s'"),
As for the change itself, I don't think that silently transforming one
conflict option id to another in svn_client_conflict_text_resolve_by_id()
API is a proper thing to do, because we don't expose this option id as a
viable resolution option in svn_client_conflict_text_get_resolution_options().
I think that a better way would be to handle this case in the command line
by using svn_client_conflict_option_working_text for binary files, if
we run with --accept=working.
Regards,
Evgeny Kotkov