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 Hett <st...@egosoft.com> on 2016/07/26 15:58:54 UTC
Re: [PATCH] Fix a conflict resolution issue related to binary files
(patch v2)
On 7/26/2016 5:46 PM, Stefan Hett wrote:
> Hi,
>
> based on the talk on IRC with stsp and Bert I'd like to propose the
> following patch which would resolve an issue in TSVN when selecting to
> prefer the local version of a binary file in conflict (see: "code
> location related to investigate potential issue in resolve dialog"
> thread on the TSVN user's mailing list for further details).
>
> [[[
> Teach build_text_conflict_resolve_items() to skip installling files,
> if these
> are not required to resolve the conflict.
>
> * libsvn_wc/conflicts.c
> (build_text_conflict_resolve_items): introduce new allow_ski_install
> variable
> and set it in the two cases
> where we
> simply accept the current local
> file to
> resolve the conflict.
> ]]]
>
Attached is a revised patch replacing the tabs with whitespaces of the
original patch (spotted by stsp).
--
Regards,
Stefan Hett
Re: [PATCH] Fix a conflict resolution issue related to binary files
(patch v4)
Posted by Stefan <lu...@posteo.de>.
On 8/28/2016 20:23, Daniel Shahaf wrote:
> 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
The problem was a local build glitch (some project files were outdated).
Unfortunately I must have run into another glitch, since after resolving
this trunk fails the new regression test with r1743999.
Will start a new thread on this one.
>> 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).
Right, my bad. Will take that into account the next time.
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
[PATCH] Resolve issue #4647 on trunk
Posted by Stefan <lu...@posteo.de>.
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] Fix a conflict resolution issue related to binary files (patch v4)
Posted by Bert Huijben <be...@qqmail.nl>.
> -----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
>
> Thanks for seeing this through!
>
> Daniel
Re: [PATCH] Fix a conflict resolution issue related to binary files
(patch v4)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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).
Thanks for seeing this through!
Daniel
Re: [PATCH] Fix a conflict resolution issue related to binary files
(patch v4)
Posted by Stefan <lu...@posteo.de>.
On 8/28/2016 13:31, Stefan wrote:
> Hi,
>
> attached is a new version of the patch which adds a regression test (the
> actual patch/fix was left unchanged).
> 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).
>
> [[[
> Fix issue #4647 by resolving the error case for binary file conflicts
> selecting
> to use the full local version, by allowing using the local file, if no
> merged
> file is present.
>
> * subversion/libsvn_wc/conflicts.c
> (build_text_conflict_resolve_items): in case mine_abspath is null, take
> local_abspath instead in case of
> svn_wc_conflict_choose_mine_full
>
> * subversion/tests/cmdline/resolve_tests.py
> (automatic_binary_conflict_resolution): add new regression test
>
> Suggested by: stsp
> ]]]
>
> Regards,
> Stefan
Got approved by Bert.
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.
Committed to trunk in r1758128-30 and nominated for backport to 1.9 in
r1758131.
Regards,
Stefan
[PATCH] Fix a conflict resolution issue related to binary files
(patch v4)
Posted by Stefan <lu...@posteo.de>.
Hi,
attached is a new version of the patch which adds a regression test (the
actual patch/fix was left unchanged).
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).
[[[
Fix issue #4647 by resolving the error case for binary file conflicts
selecting
to use the full local version, by allowing using the local file, if no
merged
file is present.
* subversion/libsvn_wc/conflicts.c
(build_text_conflict_resolve_items): in case mine_abspath is null, take
local_abspath instead in case of
svn_wc_conflict_choose_mine_full
* subversion/tests/cmdline/resolve_tests.py
(automatic_binary_conflict_resolution): add new regression test
Suggested by: stsp
]]]
Regards,
Stefan
Re: [PATCH] Fix a conflict resolution issue related to binary files
(patch v3)
Posted by Stefan <lu...@posteo.de>.
On 8/16/2016 17:54, Stefan Hett wrote:
> Hi,
>
> attached is a different approach to resolve the conflict resolution
> issue based on stsp's suggestion.
> The patch was tested against 1.9.x as well as trunk without triggering
> any test failures.
> It was also tested manually against TSVN 1.9.4 to confirm it resolves
> the conflict resolution issue there.
>
> [[[
> Resolves the error case for binary file conflicts selecting to use the
> full
> local version, by allowing using the local file, if no merged file is
> present.
>
> * subversion/libsvn_wc/conflicts.c
> (build_text_conflict_resolve_items): in case mine_abspath is null, take
> local_abspath instead in case of
> svn_wc_conflict_choose_mine_full
>
> Suggested by: stsp
> ]]]
>
>
> For the record: I certainly think that investigating Bert's idea to
> not require setting an install path, if nothing needs to be installed,
> sounds like a better approach, but this also seems to require more
> work due to the test failures the previous patch triggered.
>
With the help from danielsh I was able to reproduce the issue with the
svn command line client as well (or better said: I believe that the
issue I triggered with the command line client has the same underlying
root cause the issue in TSVN has). See [1].
I've also attached a repro batch file to the JIRA issue, demonstrating
the problem.
[1] = https://issues.apache.org/jira/browse/SVN-4647
Regards,
Stefan
Re: [PATCH] Fix a conflict resolution issue related to binary files
(patch v3)
Posted by Stefan Hett <st...@egosoft.com>.
Hi,
attached is a different approach to resolve the conflict resolution
issue based on stsp's suggestion.
The patch was tested against 1.9.x as well as trunk without triggering
any test failures.
It was also tested manually against TSVN 1.9.4 to confirm it resolves
the conflict resolution issue there.
[[[
Resolves the error case for binary file conflicts selecting to use the full
local version, by allowing using the local file, if no merged file is
present.
* subversion/libsvn_wc/conflicts.c
(build_text_conflict_resolve_items): in case mine_abspath is null, take
local_abspath instead in case of
svn_wc_conflict_choose_mine_full
Suggested by: stsp
]]]
For the record: I certainly think that investigating Bert's idea to not
require setting an install path, if nothing needs to be installed,
sounds like a better approach, but this also seems to require more work
due to the test failures the previous patch triggered.
--
Regards,
Stefan Hett
Re: [PATCH] Fix a conflict resolution issue related to binary files
(patch v2)
Posted by Stefan <lu...@posteo.de>.
On 7/26/2016 17:58, Stefan Hett wrote:
> On 7/26/2016 5:46 PM, Stefan Hett wrote:
>> Hi,
>>
>> based on the talk on IRC with stsp and Bert I'd like to propose the
>> following patch which would resolve an issue in TSVN when selecting
>> to prefer the local version of a binary file in conflict (see: "code
>> location related to investigate potential issue in resolve dialog"
>> thread on the TSVN user's mailing list for further details).
>>
>> [[[
>> Teach build_text_conflict_resolve_items() to skip installling files,
>> if these
>> are not required to resolve the conflict.
>>
>> * libsvn_wc/conflicts.c
>> (build_text_conflict_resolve_items): introduce new
>> allow_ski_install variable
>> and set it in the two cases
>> where we
>> simply accept the current
>> local file to
>> resolve the conflict.
>> ]]]
>>
> Attached is a revised patch replacing the tabs with whitespaces of the
> original patch (spotted by stsp).
>
I just ran the regression tests on 1.9.4 against the patch and this
causes three test failures:
- basic#11
- update#38
- third one I didn't note down unfortunately
Therefore I pull back this patch nomination until I either resolve the
test failure or come up with a different approach not breaking the tests.
Running the basic#11 test this produces the following error:
W: =============================================================
Expected 'mu' and actual 'mu' in status tree are different!
=============================================================
EXPECTED NODE TO BE:
=============================================================
* Node name: mu
Path: svn-test-work\working_copies\basic_tests-11.backup\A\mu
Contents: None
Properties: {}
Attributes: {'status': 'M ', 'wc_rev': '2'}
Children: None (node is probably a file)
=============================================================
ACTUAL NODE FOUND:
=============================================================
* Node name: mu
Path: svn-test-work\working_copies\basic_tests-11.backup\A\mu
Contents: None
Properties: {}
Attributes: {'status': ' ', 'wc_rev': '2'}
Children: None (node is probably a file)
Am I reading this correct in that the modified status was lost?
Regards,
Stefan