[PATCH] Resolve issue #4647 on trunk (v4)

On 10/14/2016 12:19 PM, Ivan Zhakov wrote:
> On 14 October 2016 at 00:29, Stefan <luke1410_at_posteo.de> wrote:
>> On 10/13/2016 11:38 AM, Stefan wrote:
>>> On 10/13/2016 11:08 AM, Stefan wrote:
>>>> On 10/10/2016 11:39 PM, Stefan wrote:
>>>>> On 10/10/2016 6:12 PM, Ivan Zhakov wrote:
>>>>>> On 10 October 2016 at 17:53, Stefan <luke1410_at_posteo.de> wrote:
>>>>>>> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
>>>>>>>>> Sent: zondag 28 augustus 2016 20:23
>>>>>>>>> To: Stefan <luke1410_at_posteo.de>
>>>>>>>>> Cc: dev_at_subversion.apache.org
>>>>>>>>> Subject: Re: [PATCH] Fix a conflict resolution issue related to binary files (patch
>>>>>>>>> v4)
>>>>>>>>>
>>>>>>>>> Stefan wrote on Sun, Aug 28, 2016 at 13:31:39 +0200:
>>>>>>>>>> The regression test was tested against 1.9.4, 1.9.x and trunk r1743999.
>>>>>>>>>>
>>>>>>>>>> I also tried to run the test against 1.8.16 but there it fails (didn't
>>>>>>>>>> investigate in detail).
>>>>>>>>>> Trunk r1758069 caused some build issues on my machine. Therefore I
>>>>>>>>>> couldn't validate/check the patch against the latest trunk (maybe it's
>>>>>>>>>> just some local issue with my build machine rather than some actual
>>>>>>>>>> problem on trunk - didn't look into that yet).
>>>>>>>>> For future reference, you might have tried building trunk_at_HEAD after
>>>>>>>>> locally reverting r1758069; i.e.:
>>>>>>>>>
>>>>>>>>> svn up
>>>>>>>>> svn merge -c -r1758069
>>>>>>>>> <apply patch>
>>>>>>>>> make check
>>>>>>>>>
>>>>>>>>> Stefan wrote on Sun, Aug 28, 2016 at 18:33:55 +0200:
>>>>>>>>>> Got approved by Bert.
>>>>>>>>>>
>>>>>>>>> (Thanks for stating so on the thread.)
>>>>>>>>>
>>>>>>>>>> Separated the repro test from the actual fix in order to have the
>>>>>>>>>> possibility to selectively only backport the regression test to the 1.8
>>>>>>>>>> branch.
>>>>>>>>> Good call, but the fix and the "remove XFail markers" (r1758129 and
>>>>>>>>> r1758130) should have been done in a single revision: they _are_
>>>>>>>>> a single logical change. That would also avoid breaking 'make check'
>>>>>>>>> (at r1758129 'make check' exits non-zero because of the XPASS).
>>>>>>>> I do this the same way sometimes, when I want to use the separate revision for backporting... But usually I commit things close enough that nobody notices the bot results ;-)
>>>>>>>> (While the initial XFail addition is still running, you can commit the two follow ups, and the buildbots collapses all the changes to a single build)
>>>>>>>>
>>>>>>>> I just committed the followup patch posted in another thread to unbreak the bots for the night...
>>>>>>>>
>>>>>>>> Bert
>>>>>>> Attached is a patch which should resolve the test case you added, Bert.
>>>>>>> Anybody feels like approving it? Or is there something I should
>>>>>>> improve/change?
>>>>>>>
>>>>>>> [[[
>>>>>>>
>>>>>>> Add support for the svn_client_conflict_option_working_text resolution for
>>>>>>> binary file conflicts.
>>>>>>>
>>>>>>> * subversion/libsvn_client/conflicts.c
>>>>>>> (): Add svn_client_conflict_option_working_text to binary_conflict_options
>>>>>>>
>>>>>>> * subversion/tests/cmdline/resolve_tests.py
>>>>>>> (automatic_binary_conflict_resolution): Remove XFail marker
>>>>>>>
>>>>>>> ]]]
>>>>>>>
>>>>>> It seems this patch breaks interactive conflict resolve:
>>>>>> With trunk I get the following to 'svn resolve' on binary file:
>>>>>> [[[
>>>>>> Merge conflict discovered in binary file 'A_COPY\theta'.
>>>>>> Select: (p) postpone,
>>>>>> (r) accept binary file as it appears in the working copy,
>>>>>> (tf) accept incoming version of binary file: h
>>>>>>
>>>>>> (p) - skip this conflict and leave it unresolved [postpone]
>>>>>> (tf) - accept incoming version of binary file [theirs-full]
>>>>>> (r) - accept binary file as it appears in the working copy [working]
>>>>>> (q) - postpone all remaining conflicts
>>>>>> ]]]
>>>>>>
>>>>>> But with patch I get the following:
>>>>>> [[[
>>>>>> Merge conflict discovered in binary file 'A_COPY\theta'.
>>>>>> Select: (p) postpone,
>>>>>> (r) accept binary file as it appears in the working copy,
>>>>>> (tf) accept incoming version of binary file: h
>>>>>>
>>>>>> (p) - skip this conflict and leave it unresolved [postpone]
>>>>>> (tf) - accept incoming version of binary file [theirs-full]
>>>>>> (mf) - accept binary file as it appears in the working copy [mine-full]
>>>>>> (r) - accept binary file as it appears in the working copy [working]
>>>>>> (q) - postpone all remaining conflicts
>>>>>> ]]]
>>>>>>
>>>>>> I think it's confusing and we should not offer the same option twice.
>>>>>>
>>>>> Completely agreed. The display of the option in the UI shouldn't be like
>>>>> that. Certainly an oversight on my side. Will revise the patch and come
>>>>> up with a different/better approach tomorrow.
>>>>>
>>>>> Regards,
>>>>> Stefan
>>>> Trying to put together a revised patch without the issue. The attached
>>>> patch fixes the 4647 test but breaks two other tests:
>>>>
>>>> basic#41
>>>> update#38
>>>>
>>>> Still looking into what I'm doing wrong, but any pointers would be much
>>>> appreciated.
>>> Looks like update#38 is actually fixed. Leaving basic#41 broken:
>>> FAIL: basic_tests.py 41: automatic conflict resolution
>>>
>>> Attached is the full test output.
>> I realized the problems with the previous patches and think the best
>> solution is to go with the initially discussed idea with stsp. Attached
>> is the proposed patch. Please let me know if I'd change anything there
>> or whether it's ok to apply as is.
>>
>> Index: subversion/include/svn_client.h
>> ===================================================================
>> --- subversion/include/svn_client.h (revision 1764640)
>> +++ subversion/include/svn_client.h (working copy)
>> @@ -4653,6 +4653,7 @@
>> svn_client_conflict_text_get_resolution_options(apr_array_header_t **options,
>> svn_client_conflict_t *conflict,
>> svn_client_ctx_t *ctx,
>> + svn_boolean_t ui_resolutions,
>> apr_pool_t *result_pool,
>> apr_pool_t *scratch_pool);
>>
> What is UI_RESOLUTIONS option for? It should be documented in docstring anyway.
>
> To my mind, this doesn't look like a good idea to have such flag in
> the public svn_client_conflict_text_get_resolution_options() API. It
> it useful and easy to understand for the third-party API users?

Thanks for taking the time to review the patch, Ivan (and also stsp).
After talking to stsp, here's a different approach to solve the issue
without the need for adding a new parameter to the public API.

(all tests pass on Windows and the conflict resolution UI was tested
manually to ensure no duplicate options are displayed for the conflict
resolution)

[[[
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
]]]

Regards,
Stefan