On 10/14/2016 12:19 PM, Ivan Zhakov wrote: > On 14 October 2016 at 00:29, Stefan <[email protected]> wrote: >> 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 <[email protected]> wrote: >>>>>>> On 8/28/2016 11:32 PM, Bert Huijben wrote: >>>>>>>>> -----Original Message----- >>>>>>>>> From: Daniel Shahaf [mailto:[email protected]] >>>>>>>>> Sent: zondag 28 augustus 2016 20:23 >>>>>>>>> To: Stefan <[email protected]> >>>>>>>>> Cc: [email protected] >>>>>>>>> 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. >> >> 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); >> > What is UI_RESOLUTIONS option for? It should be documented in docstring > anyway. > > To my mind, this doesn't look like a good idea to have such flag in > the public svn_client_conflict_text_get_resolution_options() API. It > it useful and easy to understand for the third-party API users?
Thanks for taking the time to review the patch, Ivan (and also stsp). After talking to stsp, here's a different approach to solve the issue without the need for adding a new parameter to the public API. (all tests pass on Windows and the conflict resolution UI was tested manually to ensure no duplicate options are displayed for the conflict resolution) [[[ Fix svn resolve --accept=working not working on binary conflicts by retrying to find a resolution option for the semantically corresponding svn_client_conflict_option_working_text option. Additionally, enable the mf option for binary conflicts, so to have the option displayed in the client. * subversion/libsvn_client/conflicts.c (svn_client_conflict_text_get_resolution_options): return the more suitable svn_client_conflict_option_working_text to resolve binary conflicts. (svn_client_conflict_text_resolve_by_id): retry determining resolution option with svn_client_conflict_option_working_text. * subversion/svn/conflict-callbacks.c (handle_text_conflict): add "mf" to the list of allowed resolutions for binary conflicts. * subversion/tests/cmdline/resolve_tests.py (automatic_binary_conflict_resolution): remove XFail marker ]]] Regards, Stefan
Index: subversion/libsvn_client/conflicts.c
===================================================================
--- subversion/libsvn_client/conflicts.c (revision 1764824)
+++ subversion/libsvn_client/conflicts.c (working copy)
@@ -7193,7 +7193,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);
}
@@ -8537,6 +8537,7 @@
{
apr_array_header_t *resolution_options;
svn_client_conflict_option_t *option;
+ const char *mime_type;
SVN_ERR(svn_client_conflict_text_get_resolution_options(
&resolution_options, conflict, ctx,
@@ -8543,13 +8544,29 @@
scratch_pool, scratch_pool));
option = svn_client_conflict_option_find_by_id(resolution_options,
option_id);
+
+ /* Support svn_client_conflict_option_merged_text for binary conflicts by
+ * mapping this onto the semantically equal
+ * svn_client_conflict_option_working_text.
+ */
if (option == NULL)
- return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
- NULL,
- _("Inapplicable conflict resolution option "
- "given for conflicted path '%s'"),
- svn_dirent_local_style(conflict->local_abspath,
- scratch_pool));
+ {
+ if (option_id == svn_client_conflict_option_merged_text) {
+ mime_type = svn_client_conflict_text_get_mime_type(conflict);
+ if (mime_type && svn_mime_type_is_binary(mime_type))
+ option = svn_client_conflict_option_find_by_id(resolution_options,
+ svn_client_conflict_option_working_text);
+ }
+ }
+
+ if (option == NULL)
+ return svn_error_createf(SVN_ERR_CLIENT_CONFLICT_OPTION_NOT_APPLICABLE,
+ NULL,
+ _("Inapplicable conflict resolution option "
+ "given for conflicted path '%s'"),
+ svn_dirent_local_style(conflict->local_abspath,
+ scratch_pool));
+
SVN_ERR(svn_client_conflict_text_resolve(conflict, option, ctx,
scratch_pool));
return SVN_NO_ERROR;
Index: subversion/svn/conflict-callbacks.c
===================================================================
--- subversion/svn/conflict-callbacks.c (revision 1764824)
+++ subversion/svn/conflict-callbacks.c (working copy)
@@ -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 1764824)
+++ 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
