On 10/10/2016 6:12 PM, Ivan Zhakov wrote: > On 10 October 2016 at 17:53, Stefan <luke1...@posteo.de> wrote: >> On 8/28/2016 11:32 PM, Bert Huijben wrote: >>>> -----Original Message----- >>>> From: Daniel Shahaf [mailto:d...@daniel.shahaf.name] >>>> Sent: zondag 28 augustus 2016 20:23 >>>> To: Stefan <luke1...@posteo.de> >>>> Cc: dev@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@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
smime.p7s
Description: S/MIME Cryptographic Signature
