Re: [PATCH] Resolve issue #4647 on trunk

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.

-- 
Ivan Zhakov