You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by pb...@apache.org on 2013/01/25 21:19:15 UTC
svn commit: r1438683 - in /subversion/trunk/subversion:
libsvn_client/merge.c tests/cmdline/merge_tests.py
Author: pburba
Date: Fri Jan 25 20:19:15 2013
New Revision: 1438683
URL: http://svn.apache.org/viewvc?rev=1438683&view=rev
Log:
Fix issue #4306 'multiple editor drive file merges record wrong mergeinfo
during conflicts'.
* subversion/libsvn_client/merge.c
(do_file_merge): If we can only perform a partial merge because a conflict
broke the merge, then only record mergeinfo for that partial merge. That
was subsequent merges can finish the job.
* subversion/tests/cmdline/merge_tests.py
(conflict_aborted_mergeinfo_described_partial_merge): Remove XFail
decorator and tweak failure status comment.
Modified:
subversion/trunk/subversion/libsvn_client/merge.c
subversion/trunk/subversion/tests/cmdline/merge_tests.py
Modified: subversion/trunk/subversion/libsvn_client/merge.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1438683&r1=1438682&r2=1438683&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/merge.c (original)
+++ subversion/trunk/subversion/libsvn_client/merge.c Fri Jan 25 20:19:15 2013
@@ -6923,7 +6923,10 @@ do_file_merge(svn_mergeinfo_catalog_t re
&filtered_rangelist,
mergeinfo_path,
merge_target->implicit_mergeinfo,
- &range, iterpool));
+ /* Only record partial mergeinfo if only a partial merge was
+ performed before a conflict was encountered. */
+ conflicted_range ? conflicted_range : &range,
+ iterpool));
/* Only record mergeinfo if there is something other than
self-referential mergeinfo, but don't record mergeinfo if
Modified: subversion/trunk/subversion/tests/cmdline/merge_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_tests.py?rev=1438683&r1=1438682&r2=1438683&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/merge_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/merge_tests.py Fri Jan 25 20:19:15 2013
@@ -18176,8 +18176,7 @@ def merge_properties_on_adds(sbox):
@SkipUnless(server_has_mergeinfo)
@Issue(4306)
-@XFail()
-# Test for issue #4306 'multiple editor drive merges record wrong
+# Test for issue #4306 'multiple editor drive file merges record wrong
# mergeinfo during conflicts'
def conflict_aborted_mergeinfo_described_partial_merge(sbox):
"conflicted split merge can be repeated"
@@ -18218,8 +18217,8 @@ def conflict_aborted_mergeinfo_described
svntest.actions.run_and_verify_svn(None, None, '.*', 'merge', '^/iota',
iota_copy_path, '--accept', 'postpone')
- # Currently this test fails because the merge fails after merging
- # only r2 (as it should) but mergeinfo for r5-6 is recorded, preventing
+ # Previously this test failed because the merge failed after merging
+ # only r2 (as it should) but mergeinfo for r5-6 was recorded, preventing
# subsequent repeat merges from applying the operative r5.
svntest.actions.run_and_verify_svn(
"Incorrect mergeinfo set during conflict aborted merge",
Re: svn commit: r1438683 - in /subversion/trunk/subversion:
libsvn_client/merge.c tests/cmdline/merge_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Jan 25, 2013 at 08:19:15PM -0000, pburba@apache.org wrote:
> Author: pburba
> Date: Fri Jan 25 20:19:15 2013
> New Revision: 1438683
>
> URL: http://svn.apache.org/viewvc?rev=1438683&view=rev
> Log:
> Fix issue #4306 'multiple editor drive file merges record wrong mergeinfo
> during conflicts'.
Hey Paul,
nice fix. I've created a backport branch for this and nominated it for 1.7.x.
Re: r1438683 - issue #4306 'multiple editor drive file merges record wrong mergeinfo during conflicts'
Posted by Julian Foad <ju...@btopenworld.com>.
--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: Paul Burba <pt...@gmail.com>
> To: Julian Foad <ju...@btopenworld.com>
> Cc: "dev@subversion.apache.org" <de...@subversion.apache.org>
> Sent: Tuesday, 12 February 2013, 21:03
> Subject: Re: r1438683 - issue #4306 'multiple editor drive file merges record wrong mergeinfo during conflicts'
>
> On Mon, Feb 4, 2013 at 5:08 PM, Julian Foad <ju...@btopenworld.com>
> wrote:
>> Paul Burba wrote:
>>
>>> Julian Foad wrote:
>>>> I (Julian Foad) wrote on 2013-01-31:
>>>>
>>>>> Hi Paul. Not sure about this...
>>>>
>>>> 1441810 fixes this and extends the test.
>>>
>>> Thanks Julian.
>>>
>>> I added r1441810 to the issue #4306 group on 1.7.x, with the caveat
>>> that property conflicts are not handled properly in 1.7, rendering
>>> your latest version of the test problematic on that branch -- see
>>>
> http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?r1=1442368&r2=1442367&pathrev=1442368
>>
>> OK, but something's lost in the translation. You say you
> "reworked the earlier version of test to demonstrate the problem r1441810
> fixes", but it fixes two problems (quoting from the log msg):
>>
>> (1) It didn't abort if there were conflicts on the last sub-range of a
> non-last
>> requested range. (2) When aborting with conflicts it recorded mergeinfo
>> describing only the current sub-range, not the sub-ranges merged before the
>> conflict.
>>
>> Your test only specifies a single range ("all") to merge, so it
> can't test (1).
>>
>>
>> Something looks wrong in this hunk of your change, at least with the
> comment:
>>
>> # Previously this test failed because the merge failed after merging
>> - # only r2 (as it should) but mergeinfo for r5-6 was recorded, preventing
>> + # only r5 (as it should) but only mergeinfo for r5 was recorded, even
>> + # though preventing
>> # subsequent repeat merges from applying the operative r5.
>> svntest.actions.run_and_verify_svn(
>> "Incorrect mergeinfo set during conflict aborted merge",
>> - ['/iota:2-4\n'], [], 'pg', SVN_PROP_MERGEINFO,
> iota_copy_path)
>> + ['/iota:2-6\n'], [], 'pg', SVN_PROP_MERGEINFO,
> iota_copy_path)
>>
>> On trunk, there were two previous buggy behaviours: most recently,
> mergeinfo for only r5 would have been recorded here, but before your initial fix
> (and, I guess, in 1.7.x) mergeinfo for the whole requested range (including in
> this case r3, r5, r7) was recorded; the latter is therefore what we want to
> mention here.
>>
>> - Julian
>
> In r1445454 I did what I probably should have done from the start:
> Take your version test (rather than my earlier version) and use it as
> a template for a 1.7 test that simple replaces the prop edits on
> 'A/mu' with text edits. The test coverage should now be as identical
> as possible on the 1.7.x-issue4306 branch and trunk.
Cool, thanks Paul. (The annoying thing is I originally started writing the test with text edits, and then I deleted that and switched to prop edits in anticipation of extending it to test a directory merge as well (which I have since done). Of course a directory merge *can* be tested with text edits (in children) but it was easier to write the test code with prop edits.)
- Julian
Re: r1438683 - issue #4306 'multiple editor drive file merges record
wrong mergeinfo during conflicts'
Posted by Paul Burba <pt...@gmail.com>.
On Mon, Feb 4, 2013 at 5:08 PM, Julian Foad <ju...@btopenworld.com> wrote:
> Paul Burba wrote:
>
>> Julian Foad wrote:
>>> I (Julian Foad) wrote on 2013-01-31:
>>>
>>>> Hi Paul. Not sure about this...
>>>
>>> 1441810 fixes this and extends the test.
>>
>> Thanks Julian.
>>
>> I added r1441810 to the issue #4306 group on 1.7.x, with the caveat
>> that property conflicts are not handled properly in 1.7, rendering
>> your latest version of the test problematic on that branch -- see
>> http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?r1=1442368&r2=1442367&pathrev=1442368
>
> OK, but something's lost in the translation. You say you "reworked the earlier version of test to demonstrate the problem r1441810 fixes", but it fixes two problems (quoting from the log msg):
>
> (1) It didn't abort if there were conflicts on the last sub-range of a non-last
> requested range. (2) When aborting with conflicts it recorded mergeinfo
> describing only the current sub-range, not the sub-ranges merged before the
> conflict.
>
> Your test only specifies a single range ("all") to merge, so it can't test (1).
>
>
> Something looks wrong in this hunk of your change, at least with the comment:
>
> # Previously this test failed because the merge failed after merging
> - # only r2 (as it should) but mergeinfo for r5-6 was recorded, preventing
> + # only r5 (as it should) but only mergeinfo for r5 was recorded, even
> + # though preventing
> # subsequent repeat merges from applying the operative r5.
> svntest.actions.run_and_verify_svn(
> "Incorrect mergeinfo set during conflict aborted merge",
> - ['/iota:2-4\n'], [], 'pg', SVN_PROP_MERGEINFO, iota_copy_path)
> + ['/iota:2-6\n'], [], 'pg', SVN_PROP_MERGEINFO, iota_copy_path)
>
> On trunk, there were two previous buggy behaviours: most recently, mergeinfo for only r5 would have been recorded here, but before your initial fix (and, I guess, in 1.7.x) mergeinfo for the whole requested range (including in this case r3, r5, r7) was recorded; the latter is therefore what we want to mention here.
>
> - Julian
In r1445454 I did what I probably should have done from the start:
Take your version test (rather than my earlier version) and use it as
a template for a 1.7 test that simple replaces the prop edits on
'A/mu' with text edits. The test coverage should now be as identical
as possible on the 1.7.x-issue4306 branch and trunk.
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Re: r1438683 - issue #4306 'multiple editor drive file merges record wrong mergeinfo during conflicts'
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> Julian Foad wrote:
>> I (Julian Foad) wrote on 2013-01-31:
>>
>>> Hi Paul. Not sure about this...
>>
>> 1441810 fixes this and extends the test.
>
> Thanks Julian.
>
> I added r1441810 to the issue #4306 group on 1.7.x, with the caveat
> that property conflicts are not handled properly in 1.7, rendering
> your latest version of the test problematic on that branch -- see
> http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?r1=1442368&r2=1442367&pathrev=1442368
OK, but something's lost in the translation. You say you "reworked the earlier version of test to demonstrate the problem r1441810 fixes", but it fixes two problems (quoting from the log msg):
(1) It didn't abort if there were conflicts on the last sub-range of a non-last
requested range. (2) When aborting with conflicts it recorded mergeinfo
describing only the current sub-range, not the sub-ranges merged before the
conflict.
Your test only specifies a single range ("all") to merge, so it can't test (1).
Something looks wrong in this hunk of your change, at least with the comment:
# Previously this test failed because the merge failed after merging
- # only r2 (as it should) but mergeinfo for r5-6 was recorded, preventing
+ # only r5 (as it should) but only mergeinfo for r5 was recorded, even
+ # though preventing
# subsequent repeat merges from applying the operative r5.
svntest.actions.run_and_verify_svn(
"Incorrect mergeinfo set during conflict aborted merge",
- ['/iota:2-4\n'], [], 'pg', SVN_PROP_MERGEINFO, iota_copy_path)
+ ['/iota:2-6\n'], [], 'pg', SVN_PROP_MERGEINFO, iota_copy_path)
On trunk, there were two previous buggy behaviours: most recently, mergeinfo for only r5 would have been recorded here, but before your initial fix (and, I guess, in 1.7.x) mergeinfo for the whole requested range (including in this case r3, r5, r7) was recorded; the latter is therefore what we want to mention here.
- Julian
Re: r1438683 - issue #4306 'multiple editor drive file merges record
wrong mergeinfo during conflicts'
Posted by Paul Burba <pt...@gmail.com>.
On Sat, Feb 2, 2013 at 5:35 PM, Julian Foad <ju...@btopenworld.com> wrote:
> I (Julian Foad) wrote on 2013-01-31:
>
>> Hi Paul. Not sure about this...
>
> 1441810 fixes this and extends the test.
Thanks Julian.
I added r1441810 to the issue #4306 group on 1.7.x, with the caveat
that property conflicts are not handled properly in 1.7, rendering
your latest version of the test problematic on that branch -- see
http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?r1=1442368&r2=1442367&pathrev=1442368
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Re: r1438683 - issue #4306 'multiple editor drive file merges record wrong mergeinfo during conflicts'
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote on 2013-01-31:
> Hi Paul. Not sure about this...
1441810 fixes this and extends the test.
- Julian
>> URL: http://svn.apache.org/viewvc?rev=1438683&view=rev
>
>> Log:
>> Fix issue #4306 'multiple editor drive file merges record wrong
>> mergeinfo during conflicts'.
>> @@ -6923,7 +6923,10 @@ do_file_merge(svn_mergeinfo_catalog_t re
>> &filtered_rangelist,
>> mergeinfo_path,
>> merge_target->implicit_mergeinfo,
>> - &range, iterpool));
>> + /* Only record partial mergeinfo if only a partial merge was
>> + performed before a conflict was encountered. */
>> + conflicted_range ? conflicted_range : &range,
>> + iterpool));
>
> Shouldn't that be "the range starting at 'range.start' and
> ending at 'conflicted_range.end'?
>
> It looks to me like it will only record mergeinfo for the sub-range that raised
> the conflict, and not for the earlier sub-ranges that completed successfully.
>
> I'm currently writing an extended test that will cover when a conflict is
> raised in:
>
> { first sub-range of first specified range;
> last sub-range of first specified range;
> first sub-range of last specified range;
> last sub-range of last specified range }
> x
> { single-file merge;
> dir merge }
>
> That should be able to confirm my suspicion about the above code.
>
> I'm doing this because I'm working on refactoring so that the three main
> merge subroutines called from do_merge() (that is the cases: single-file,
> dir-with-mergeinfo, dir-no-mergeinfo), each return info about where they stopped
> if they stopped because of a conflict, instead of throwing an error containing
> that info in human-readable form.
>
> Then I will be able to put the merge in a "do { merge; resolve any
> conflicts } while (merge is not complete)" loop at the 'svn'
> level. And thereby fix interactive conflict resolution which currently aborts
> at this point even if you resolved the conflicts.
>
> - Julian
Re: r1438683 - issue #4306 'multiple editor drive file merges record wrong mergeinfo during conflicts'
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Paul. Not sure about this...
> URL: http://svn.apache.org/viewvc?rev=1438683&view=rev
> Log:
> Fix issue #4306 'multiple editor drive file merges record wrong mergeinfo
> during conflicts'.
>
> * subversion/libsvn_client/merge.c
> (do_file_merge): If we can only perform a partial merge because a conflict
> broke the merge, then only record mergeinfo for that partial merge. That
> was subsequent merges can finish the job.
>
> * subversion/tests/cmdline/merge_tests.py
> (conflict_aborted_mergeinfo_described_partial_merge): Remove XFail
> decorator and tweak failure status comment.
>
> Modified:
> subversion/trunk/subversion/libsvn_client/merge.c
> subversion/trunk/subversion/tests/cmdline/merge_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_client/merge.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1438683&r1=1438682&r2=1438683&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/merge.c (original)
> +++ subversion/trunk/subversion/libsvn_client/merge.c Fri Jan 25 20:19:15 2013
> @@ -6923,7 +6923,10 @@ do_file_merge(svn_mergeinfo_catalog_t re
> &filtered_rangelist,
> mergeinfo_path,
> merge_target->implicit_mergeinfo,
> - &range, iterpool));
> + /* Only record partial mergeinfo if only a partial merge was
> + performed before a conflict was encountered. */
> + conflicted_range ? conflicted_range : &range,
> + iterpool));
Shouldn't that be "the range starting at 'range.start' and ending at 'conflicted_range.end'?
It looks to me like it will only record mergeinfo for the sub-range that raised the conflict, and not for the earlier sub-ranges that completed successfully.
I'm currently writing an extended test that will cover when a conflict is raised in:
{ first sub-range of first specified range;
last sub-range of first specified range;
first sub-range of last specified range;
last sub-range of last specified range }
x
{ single-file merge;
dir merge }
That should be able to confirm my suspicion about the above code.
I'm doing this because I'm working on refactoring so that the three main merge subroutines called from do_merge() (that is the cases: single-file, dir-with-mergeinfo, dir-no-mergeinfo), each return info about where they stopped if they stopped because of a conflict, instead of throwing an error containing that info in human-readable form.
Then I will be able to put the merge in a "do { merge; resolve any conflicts } while (merge is not complete)" loop at the 'svn' level. And thereby fix interactive conflict resolution which currently aborts at this point even if you resolved the conflicts.
- Julian
>
> /* Only record mergeinfo if there is something other than
> self-referential mergeinfo, but don't record mergeinfo if
>
> Modified: subversion/trunk/subversion/tests/cmdline/merge_tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_tests.py?rev=1438683&r1=1438682&r2=1438683&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/merge_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/merge_tests.py Fri Jan 25 20:19:15
> 2013
> @@ -18176,8 +18176,7 @@ def merge_properties_on_adds(sbox):
>
> @SkipUnless(server_has_mergeinfo)
> @Issue(4306)
> -@XFail()
> -# Test for issue #4306 'multiple editor drive merges record wrong
> +# Test for issue #4306 'multiple editor drive file merges record wrong
> # mergeinfo during conflicts'
> def conflict_aborted_mergeinfo_described_partial_merge(sbox):
> "conflicted split merge can be repeated"
> @@ -18218,8 +18217,8 @@ def conflict_aborted_mergeinfo_described
> svntest.actions.run_and_verify_svn(None, None, '.*', 'merge',
> '^/iota',
> iota_copy_path, '--accept',
> 'postpone')
>
> - # Currently this test fails because the merge fails after merging
> - # only r2 (as it should) but mergeinfo for r5-6 is recorded, preventing
> + # Previously this test failed because the merge failed after merging
> + # only r2 (as it should) but mergeinfo for r5-6 was recorded, preventing
> # subsequent repeat merges from applying the operative r5.
> svntest.actions.run_and_verify_svn(
> "Incorrect mergeinfo set during conflict aborted merge",
>