You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan <lu...@posteo.de> on 2016/10/10 15:53:11 UTC
[PATCH] Resolve issue #4647 on trunk
On 8/28/2016 11:32 PM, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>> Sent: zondag 28 augustus 2016 20:23
>> To: Stefan <lu...@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
]]]
Regards,
Stefan
Re: [PATCH] Resolve issue #4647 on trunk (v4)
Posted by Stefan <lu...@posteo.de>.
On 10/14/2016 4:05 PM, Stefan Sperling wrote:
> On Fri, Oct 14, 2016 at 03:35:48PM +0200, Stefan wrote:
>> 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)
> +1, please commit.
>
> Thank you!
Thanks, committed in 1764902.
Regards,
Stefan
Re: [PATCH] Resolve issue #4647 on trunk (v4)
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Oct 14, 2016 at 03:35:48PM +0200, Stefan wrote:
> 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)
+1, please commit.
Thank you!
> [[[
> 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"
>
[PATCH] Resolve issue #4647 on trunk (v4)
Posted by Stefan <lu...@posteo.de>.
On 10/14/2016 12:19 PM, Ivan Zhakov wrote:
> On 14 October 2016 at 00:29, Stefan <lu...@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 <lu...@posteo.de> wrote:
>>>>>>> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>>>>>>>> Sent: zondag 28 augustus 2016 20:23
>>>>>>>>> To: Stefan <lu...@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
Re: [PATCH] Resolve issue #4647 on trunk (v3)
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 14 October 2016 at 00:29, Stefan <lu...@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 <lu...@posteo.de> wrote:
>>>>>> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>>>>>>> Sent: zondag 28 augustus 2016 20:23
>>>>>>>> To: Stefan <lu...@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
[PATCH] Resolve issue #4647 on trunk (v3)
Posted by Stefan <lu...@posteo.de>.
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 <lu...@posteo.de> wrote:
>>>>> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>>>>>> Sent: zondag 28 augustus 2016 20:23
>>>>>>> To: Stefan <lu...@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
Re: [PATCH] Resolve issue #4647 on trunk (v2)
Posted by Stefan <lu...@posteo.de>.
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 <lu...@posteo.de> wrote:
>>>> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>>>>> -----Original Message-----
>>>>>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>>>>> Sent: zondag 28 augustus 2016 20:23
>>>>>> To: Stefan <lu...@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.
Regards,
Stefan
[PATCH] Resolve issue #4647 on trunk (v2)
Posted by Stefan <lu...@posteo.de>.
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 <lu...@posteo.de> wrote:
>>> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>>>> -----Original Message-----
>>>>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>>>> Sent: zondag 28 augustus 2016 20:23
>>>>> To: Stefan <lu...@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
Re: [PATCH] Resolve issue #4647 on trunk
Posted by Stefan <lu...@posteo.de>.
On 10/10/2016 6:12 PM, Ivan Zhakov wrote:
> On 10 October 2016 at 17:53, Stefan <lu...@posteo.de> wrote:
>> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>>> -----Original Message-----
>>>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>>> Sent: zondag 28 augustus 2016 20:23
>>>> To: Stefan <lu...@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
Re: [PATCH] Resolve issue #4647 on trunk
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 10 October 2016 at 17:53, Stefan <lu...@posteo.de> wrote:
> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>
>>> -----Original Message-----
>>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>> Sent: zondag 28 augustus 2016 20:23
>>> To: Stefan <lu...@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.
--
Ivan Zhakov