On 10/14/2016 12:19 PM, Ivan Zhakov wrote: > On 14 October 2016 at 00:29, Stefan <luke1...@posteo.de> 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 <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. >> >> 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