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

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.

[[[
Fix svn resolve --accept=working not working on binary conflicts by making
svn_client_conflict_text_get_resolutions_options returning the list of
accepted resolution options, based on whether it's needed for UI/user
presented options or internal/API supported options.

Additionally, enable the mf option for binary conflicts, so to have the
option
displayed in the client.

* subversion/include/svn_client.h
  (svn_client_conflict_text_get_resolution_options): add new ui_resolutions
  parameter.

* subversion/libsvn_client/conflicts.c
  (svn_client_conflict_text_get_resolution_options): filter the returned
  resolution options for binary conflicts based on the added ui_resolutions
  parameter.
  (svn_client_conflict_text_resolve_by_id): update call to
  svn_client_conflict_text_get_resolution_options().

* subversion/svn/conflict-callbacks.c
  (build_text_conflict_options): update call to
  svn_client_conflict_text_get_resolution_options().

* subversion/tests/cmdline/resolve_tests.py
  (automatic_binary_conflict_resolution): remove XFail marker

* subversion/tests/libsvn_client/conflicts-test.c
  (assert_text_conflict_options): update call to
  svn_client_conflict_text_get_resolution_options().
]]]

Regards,
Stefan