subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Hett <ste...@egosoft.com>
Subject Re: svn commit: r1764902 - in /subversion/trunk/subversion: libsvn_client/conflicts.c svn/conflict-callbacks.c tests/cmdline/resolve_tests.py
Date Tue, 18 Oct 2016 09:54:59 GMT
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@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


Mime
View raw message