Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Subject: Re: svn commit: r1764902 - in /subversion/trunk/subversion:
 libsvn_client/conflicts.c svn/conflict-callbacks.c
 tests/cmdline/resolve_tests.py
To: dev@subversion.apache.org
References: <20161014140759.823A63A039C@svn01-us-west.apache.org>
 <CAP_GPNgmjxNyQwhScuGJO=wjPhvPZprrJtUXbTVLEjDozfWhfw@mail.gmail.com>
From: Stefan Hett <stefan@egosoft.com>
Message-ID: <f5acf4f0-b21a-4d2b-1df1-d9547101aa66@egosoft.com>
Date: Tue, 18 Oct 2016 11:54:59 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <CAP_GPNgmjxNyQwhScuGJO=wjPhvPZprrJtUXbTVLEjDozfWhfw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
archived-at: Tue, 18 Oct 2016 09:55:11 -0000

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