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 <luke1410_at_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
Received on 2016-10-18 11:55:12 CEST