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 <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
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_option_merged_text,
+ svn_client_conflict_option_working_text,
_("accept binary file as it appears in the working copy"),
resolve_text_conflict);
}
Index: subversion/svn/conflict-callbacks.c
===================================================================
--- subversion/svn/conflict-callbacks.c (revision 1764448)
+++ subversion/svn/conflict-callbacks.c (working copy)
@@ -1845,7 +1845,7 @@
*option_id = svn_client_conflict_option_base_text;
break;
case svn_cl__accept_working:
- *option_id = svn_client_conflict_option_merged_text;
+ *option_id = svn_client_conflict_option_working_text;
break;
case svn_cl__accept_mine_conflict:
*option_id = svn_client_conflict_option_working_text_where_conflicted;
Index: subversion/svn/resolve-cmd.c
===================================================================
--- subversion/svn/resolve-cmd.c (revision 1764448)
+++ subversion/svn/resolve-cmd.c (working copy)
@@ -380,7 +380,7 @@
switch (opt_state->accept_which)
{
case svn_cl__accept_working:
- option_id = svn_client_conflict_option_merged_text;
+ option_id = svn_client_conflict_option_working_text;
break;
case svn_cl__accept_base:
option_id = svn_client_conflict_option_base_text;
Index: subversion/tests/cmdline/resolve_tests.py
===================================================================
--- subversion/tests/cmdline/resolve_tests.py (revision 1764448)
+++ subversion/tests/cmdline/resolve_tests.py (working copy)
@@ -602,7 +602,6 @@
# Test for issue #4647 'auto resolution mine-full fails on binary file'
@Issue(4647)
-@XFail()
def automatic_binary_conflict_resolution(sbox):
"resolve -R --accept [base | mf | tf] binary file"
smime.p7s
Description: S/MIME Cryptographic Signature
