Re: [PATCH] Resolve issue #4647 on trunk (v2)
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 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 >> 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 >> >> 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. Regards, Stefan same: C:\Projects\MaxSVN\dependencies\openssl\out3
[PATCH] Resolve issue #4647 on trunk (v2)
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 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 > 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 > > 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. Regards, Stefan Index: subversion/libsvn_client/conflicts.c === --- subversion/libsvn_client/conflicts.c(revision 1764448) +++ subversion/libsvn_client/conflicts.c(working copy) @@ -7121,7 +7121,7 @@ resolve_text_conflict); add_resolution_option(*options, conflict, -svn_client_conflict_
