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? -- Ivan Zhakov