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 <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. > 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
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1764640)
+++ subversion/include/svn_client.h (working copy)
@@ -4653,6 +4653,7 @@
svn_client_conflict_text_get_resolution_options(apr_array_header_t **options,
svn_client_conflict_t
*conflict,
svn_client_ctx_t *ctx,
+ svn_boolean_t ui_resolutions,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
Index: subversion/libsvn_client/conflicts.c
===================================================================
--- subversion/libsvn_client/conflicts.c (revision 1764640)
+++ subversion/libsvn_client/conflicts.c (working copy)
@@ -7084,6 +7084,7 @@
svn_client_conflict_text_get_resolution_options(apr_array_header_t **options,
svn_client_conflict_t
*conflict,
svn_client_ctx_t *ctx,
+ svn_boolean_t ui_resolutions,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
@@ -7114,9 +7115,22 @@
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);
+
+ // for binary conflicts we exclude the merged_text option in case the
+ // resolution list is requested for the ui presentation, since this
+ // option basically equals the behavior of the working_text option an
+ // we do not want to have two options with the same semantics being
+ // reported on the other side we need to report both options being valid
+ // to resolve text conflicts for binary files (example: fixes
+ // --accept=working not being accepted for binary files)
+ if (!ui_resolutions)
+ add_resolution_option(*options, conflict,
+ svn_client_conflict_option_merged_text,
+ _("accept binary file as it appears in the working copy"),
+ resolve_text_conflict);
}
else
{
@@ -8460,7 +8474,7 @@
svn_client_conflict_option_t *option;
SVN_ERR(svn_client_conflict_text_get_resolution_options(
- &resolution_options, conflict, ctx,
+ &resolution_options, conflict, ctx, FALSE,
scratch_pool, scratch_pool));
option = svn_client_conflict_option_find_by_id(resolution_options,
option_id);
Index: subversion/svn/conflict-callbacks.c
===================================================================
--- subversion/svn/conflict-callbacks.c (revision 1764640)
+++ subversion/svn/conflict-callbacks.c (working copy)
@@ -708,7 +708,7 @@
apr_pool_t *iterpool;
SVN_ERR(svn_client_conflict_text_get_resolution_options(&builtin_options,
- conflict, ctx,
+ conflict, ctx, TRUE,
scratch_pool,
scratch_pool));
nopt = builtin_options->nelts + ARRAY_LEN(extra_resolver_options);
@@ -913,10 +913,9 @@
if (knows_something || is_binary)
*next_option++ = "r";
- /* The 'mine-full' option selects the ".mine" file so only offer
- * it if that file exists. It does not exist for binary files,
- * for example (questionable historical behaviour since 1.0). */
- if (my_abspath)
+ /* The 'mine-full' option selects the ".mine" file for texts or
+ * the current working directory file for binary files. */
+ if (my_abspath || is_binary)
*next_option++ = "mf";
*next_option++ = "tf";
Index: subversion/tests/cmdline/resolve_tests.py
===================================================================
--- subversion/tests/cmdline/resolve_tests.py (revision 1764640)
+++ 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"
Index: subversion/tests/libsvn_client/conflicts-test.c
===================================================================
--- subversion/tests/libsvn_client/conflicts-test.c (revision 1764640)
+++ subversion/tests/libsvn_client/conflicts-test.c (working copy)
@@ -147,7 +147,8 @@
apr_array_header_t *actual;
SVN_ERR(svn_client_conflict_text_get_resolution_options(&actual, conflict,
- ctx, pool, pool));
+ ctx, FALSE, pool,
+ pool));
SVN_ERR(assert_conflict_options(actual, expected, pool));
return SVN_NO_ERROR;
smime.p7s
Description: S/MIME Cryptographic Signature
