Peter Lundblad wrote:
> Kamesh Jayachandran writes:
>
>> [[[
>>
>> Use time proven 'svn_path_is_ancestor' rather than custom incomplete code.
>> Testcase to showcase the defect.
>>
>
> It is normal for a bug fix to be acompanied by a test case, so this last
> line is unnecesary.
>
>
Fine. Fixed it.
> ...
>
>> Patch by: kameshj
>> Found by: plundblad
>>
>
> ^ I'm actually "lundblad", but I understand why you're confused;)
>
Done.
>> Index: subversion/tests/cmdline/merge_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/merge_tests.py (revision 23505)
>> +++ subversion/tests/cmdline/merge_tests.py (working copy)
>>
> ...
>
>> @@ -4563,6 +4639,161 @@
>> finally:
>> os.chdir(saved_cwd)
>>
>> +def call_set_path_only_on_subtree_while_doing_subtree_merges(sbox):
>> + "call set_path only on subtree path"
>>
>
> This is not a good name and description for a test case, since it talks about
> details of a specific implementation in the libs. The test
> is still valid if we change the implementation to something completely
> different. You should talk about what the test actually checks (that
> path name components that are prefixes of other path name components
> can cause the path with the longer component to be inappropriately
> excluded). Coming up with a good terse name might be a challenge, but
> letting implementation details leak out into the test suite might get very
> confusing in the future.
>
> Also, I'm not sure why you need this very deep tree structure for this,
> but I haven't tried to reproduce it.
>
Yes it is the minimum tree I could think about to reproduce.
> An alternative to adding a new test might be to introduce this into
> existing tests that deal with excluding of subtrees.
Ok. Changed the behaviour of
'avoid_repeated_merge_on_subtree_with_merge_info' to take care of this
case too. (Just overwritten it with the function I sent in the original
patch.)
Find the attached patch.
With regards
Kamesh Jayachandran
[[[
Use time proven 'svn_path_is_ancestor' rather than custom incomplete code.
* subversion/libsvn_client/diff.c
(do_merge):
Use time proven 'svn_path_is_ancestor' rather than custom incomplete code.
* subversion/tests/cmdline/merge_tests.py
(): Bump up copyright year.
(create_deep_trees): Changed the behaviour to create 2 deep trees (/A/B/F/E
and /A/B/F/E1) rather than one deep tree '/A/B/F/E'.
(avoid_repeated_merge_using_inherited_merge_info): Adjust for the above
change.
(avoid_repeated_merge_on_subtree_with_merge_info):
Enhance this testcase to handle subtrees(which has mergeinfo) with
similarly looking names(/A/B/F/E and /A/B/F/E1).
Patch by: kameshj
Found by: lundblad
dlr
]]]
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 23736)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -2397,18 +2397,14 @@
j,
svn_sort__item_t);
child_wcpath = item->key;
- if (item->klen < target_wcpath_len ||
- strcmp(child_wcpath, target_wcpath) == 0 ||
- strncmp(child_wcpath, target_wcpath, target_wcpath_len) != 0)
+ if (svn_path_is_ancestor(target_wcpath, child_wcpath) &&
+ strcmp(child_wcpath, target_wcpath) != 0)
{
- /* Path is not a child of the WC's merge target. */
- continue;
+ child_repos_path = child_wcpath + target_wcpath_len + 1;
+ SVN_ERR(reporter->set_path(report_baton, child_repos_path,
+ is_revert ? r->end - 1 : r->end,
+ FALSE, NULL, pool));
}
-
- child_repos_path = child_wcpath + target_wcpath_len + 1;
- SVN_ERR(reporter->set_path(report_baton, child_repos_path,
- is_revert ? r->end - 1 : r->end,
- FALSE, NULL, pool));
}
}
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 23736)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -6,7 +6,7 @@
# See http://subversion.tigris.org for more information.
#
# ====================================================================
-# Copyright (c) 2000-2006 CollabNet. All rights reserved.
+# Copyright (c) 2000-2007 CollabNet. All rights reserved.
#
# This software is licensed as described in the file COPYING, which
# you should have received as part of this distribution. The terms
@@ -4063,13 +4063,17 @@
expected_backup_skip)
def create_deep_trees(wc_dir):
- """Create A/B/F/E by moving A/B/E to A/B/F/E, copy A/B to
- A/copy-of-B, and return the expected status."""
+ """Create A/B/F/E by moving A/B/E to A/B/F/E.
+ Copy A/B/F/E to A/B/F/E1.
+ Copy A/B to A/copy-of-B, and return the expected status.
+ At the end of this function WC would be at r4"""
A_path = os.path.join(wc_dir, 'A')
A_B_path = os.path.join(A_path, 'B')
A_B_E_path = os.path.join(A_B_path, 'E')
A_B_F_path = os.path.join(A_B_path, 'F')
+ A_B_F_E_path = os.path.join(A_B_F_path, 'E')
+ A_B_F_E1_path = os.path.join(A_B_F_path, 'E1')
# Deepen the directory structure we're working with by moving E to
# underneath F and committing, creating revision 2.
@@ -4089,29 +4093,48 @@
expected_status, None,
None, None, None, None, wc_dir)
- # Bring the entire WC up to date with rev 2.
+ svntest.main.run_svn(None, 'cp', A_B_F_E_path, A_B_F_E1_path)
+ expected_output = wc.State(wc_dir, {
+ 'A/B/F/E1' : Item(verb='Adding')
+ })
+ expected_status.add({
+ 'A/B/F/E1' : Item(status=' ', wc_rev=3),
+ 'A/B/F/E1/alpha' : Item(status=' ', wc_rev=3),
+ 'A/B/F/E1/beta' : Item(status=' ', wc_rev=3),
+ })
+ svntest.actions.run_and_verify_commit(wc_dir, expected_output,
+ expected_status, None,
+ None, None, None, None, wc_dir)
+
+ # Bring the entire WC up to date with rev 3.
svntest.actions.run_and_verify_svn(None, None, [], 'update', wc_dir)
+ expected_status.tweak(wc_rev=3)
- # Copy B and commit, creating revision 3.
+ # Copy B and commit, creating revision 4.
copy_of_B_path = os.path.join(A_path, 'copy-of-B')
svntest.main.run_svn(None, "cp", A_B_path, copy_of_B_path)
expected_output = svntest.wc.State(wc_dir, {
'A/copy-of-B' : Item(verb='Adding'),
})
- expected_status.tweak(wc_rev=2)
expected_status.add({
- 'A/copy-of-B' : Item(status=' ', wc_rev=3),
- 'A/copy-of-B/F' : Item(status=' ', wc_rev=3),
- 'A/copy-of-B/F/E' : Item(status=' ', wc_rev=3),
- 'A/copy-of-B/F/E/alpha': Item(status=' ', wc_rev=3),
- 'A/copy-of-B/F/E/beta' : Item(status=' ', wc_rev=3),
- 'A/copy-of-B/lambda' : Item(status=' ', wc_rev=3),
+ 'A/copy-of-B' : Item(status=' ', wc_rev=4),
+ 'A/copy-of-B/F' : Item(status=' ', wc_rev=4),
+ 'A/copy-of-B/F/E' : Item(status=' ', wc_rev=4),
+ 'A/copy-of-B/F/E/alpha' : Item(status=' ', wc_rev=4),
+ 'A/copy-of-B/F/E/beta' : Item(status=' ', wc_rev=4),
+ 'A/copy-of-B/F/E1' : Item(status=' ', wc_rev=4),
+ 'A/copy-of-B/F/E1/alpha' : Item(status=' ', wc_rev=4),
+ 'A/copy-of-B/F/E1/beta' : Item(status=' ', wc_rev=4),
+ 'A/copy-of-B/lambda' : Item(status=' ', wc_rev=4),
})
svntest.actions.run_and_verify_commit(wc_dir, expected_output,
expected_status, None,
None, None, None, None, wc_dir)
+ # Bring the entire WC up to date with rev 4.
+ svntest.actions.run_and_verify_svn(None, None, [], 'update', wc_dir)
+ expected_status.tweak(wc_rev=4)
return expected_status
def avoid_repeated_merge_using_inherited_merge_info(sbox):
@@ -4129,7 +4152,7 @@
# Create a deeper directory structure.
expected_status = create_deep_trees(wc_dir)
- # Edit alpha and commit it, creating revision 4.
+ # Edit alpha and commit it, creating revision 5.
alpha_path = os.path.join(A_B_F_path, 'E', 'alpha')
new_content_for_alpha = 'new content to alpha\n'
svntest.main.file_write(alpha_path, new_content_for_alpha)
@@ -4137,7 +4160,7 @@
expected_output = svntest.wc.State(wc_dir, {
'A/B/F/E/alpha' : Item(verb='Sending'),
})
- expected_status.tweak('A/B/F/E/alpha', wc_rev=4)
+ expected_status.tweak('A/B/F/E/alpha', wc_rev=5)
svntest.actions.run_and_verify_commit(wc_dir, expected_output,
expected_status, None,
None, None, None, None, wc_dir)
@@ -4146,31 +4169,37 @@
# this file, to understand why we shorten and chdir() below.
short_copy_of_B_path = shorten_path_kludge(copy_of_B_path)
- # Merge changes from rev 4 of B (to alpha) into copy_of_B.
+ # Merge changes from rev 5 of B (to alpha) into copy_of_B.
expected_output = wc.State(short_copy_of_B_path, {
'F/E/alpha' : Item(status='U '),
})
expected_status = wc.State(short_copy_of_B_path, {
- '' : Item(status=' M', wc_rev=3),
- 'F/E' : Item(status=' ', wc_rev=3),
- 'F/E/alpha' : Item(status='M ', wc_rev=3),
- 'F/E/beta' : Item(status=' ', wc_rev=3),
- 'lambda' : Item(status=' ', wc_rev=3),
- 'F' : Item(status=' ', wc_rev=3),
+ '' : Item(status=' M', wc_rev=4),
+ 'F/E' : Item(status=' ', wc_rev=4),
+ 'F/E/alpha' : Item(status='M ', wc_rev=4),
+ 'F/E/beta' : Item(status=' ', wc_rev=4),
+ 'F/E1' : Item(status=' ', wc_rev=4),
+ 'F/E1/alpha' : Item(status=' ', wc_rev=4),
+ 'F/E1/beta' : Item(status=' ', wc_rev=4),
+ 'lambda' : Item(status=' ', wc_rev=4),
+ 'F' : Item(status=' ', wc_rev=4),
})
expected_disk = wc.State('', {
- '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B:4'}),
- 'F/E' : Item(),
- 'F/E/alpha' : Item(new_content_for_alpha),
- 'F/E/beta' : Item("This is the file 'beta'.\n"),
- 'F' : Item(),
- 'lambda' : Item("This is the file 'lambda'.\n")
+ '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B:5'}),
+ 'F/E' : Item(),
+ 'F/E/alpha' : Item(new_content_for_alpha),
+ 'F/E/beta' : Item("This is the file 'beta'.\n"),
+ 'F/E1' : Item(),
+ 'F/E1/alpha' : Item("This is the file 'alpha'.\n"),
+ 'F/E1/beta' : Item("This is the file 'beta'.\n"),
+ 'F' : Item(),
+ 'lambda' : Item("This is the file 'lambda'.\n")
})
expected_skip = wc.State(short_copy_of_B_path, { })
saved_cwd = os.getcwd()
try:
os.chdir(svntest.main.work_dir)
- svntest.actions.run_and_verify_merge(short_copy_of_B_path, '3', '4',
+ svntest.actions.run_and_verify_merge(short_copy_of_B_path, '4', '5',
sbox.repo_url + \
'/A/B',
expected_output,
@@ -4185,7 +4214,7 @@
finally:
os.chdir(saved_cwd)
- # Commit the result of the merge, creating revision 5.
+ # Commit the result of the merge, creating revision 6.
expected_output = svntest.wc.State(copy_of_B_path, {
'' : Item(verb='Sending'),
'F/E/alpha' : Item(verb='Sending'),
@@ -4194,10 +4223,10 @@
None, None,
None, None, None, None, wc_dir)
- # Update the WC to bring /A/copy_of_B/F from rev 3 to rev 5.
+ # Update the WC to bring /A/copy_of_B/F from rev 4 to rev 6.
# Without this update, a subsequent merge will not find any merge
# info for /A/copy_of_B/F -- nor its parent dir in the repos -- at
- # rev 3. Merge info wasn't introduced until rev 5.
+ # rev 4. Merge info wasn't introduced until rev 6.
copy_of_B_F_E_path = os.path.join(copy_of_B_path, 'F', 'E')
svntest.actions.run_and_verify_svn(None, None, [], 'update', wc_dir)
@@ -4210,9 +4239,9 @@
# target (/A/copy-of-B/F/E) to avoid a repeated merge.
expected_output = wc.State(copy_of_B_F_E_path, { })
expected_status = wc.State(short_copy_of_B_F_E_path, {
- '' : Item(status=' ', wc_rev=5),
- 'alpha' : Item(status=' ', wc_rev=5),
- 'beta' : Item(status=' ', wc_rev=5),
+ '' : Item(status=' ', wc_rev=6),
+ 'alpha' : Item(status=' ', wc_rev=6),
+ 'beta' : Item(status=' ', wc_rev=6),
})
expected_disk = wc.State('', {
'alpha' : Item(new_content_for_alpha),
@@ -4222,9 +4251,8 @@
saved_cwd = os.getcwd()
try:
os.chdir(svntest.main.work_dir)
- svntest.actions.run_and_verify_merge(short_copy_of_B_F_E_path, '3', '4',
- sbox.repo_url + \
- '/A/B/F/E',
+ svntest.actions.run_and_verify_merge(short_copy_of_B_F_E_path, '4', '5',
+ sbox.repo_url + '/A/B/F/E',
expected_output,
expected_disk,
expected_status,
@@ -4239,7 +4267,11 @@
def avoid_repeated_merge_on_subtree_with_merge_info(sbox):
"use subtree's merge info to avoid repeated merge"
-
+ # Create deep trees A/B/F/E and A/B/F/E1 and copy A/B to A/copy-of-B
+ # with the help of 'create_deep_trees'
+ # As /A/copy-of-B/F/E1 is not a child of /A/copy-of-B/F/E,
+ # set_path should not be called on /A/copy-of-B/F/E1 while
+ # doing a implicit subtree merge on /A/copy-of-B/F/E.
sbox.build()
wc_dir = sbox.wc_dir
@@ -4253,67 +4285,68 @@
# Create a deeper directory structure.
expected_status = create_deep_trees(wc_dir)
- # Edit alpha and commit it, creating revision 4.
+ # Edit alpha and commit it, creating revision 5.
alpha_path = os.path.join(A_B_F_E_path, 'alpha')
- new_content_for_alpha = 'new content to alpha\n'
- svntest.main.file_write(alpha_path, new_content_for_alpha)
+ new_content_for_alpha1 = 'new content to alpha\n'
+ svntest.main.file_write(alpha_path, new_content_for_alpha1)
expected_output = svntest.wc.State(wc_dir, {
'A/B/F/E/alpha' : Item(verb='Sending'),
})
- expected_status.tweak('A/B/F/E/alpha', wc_rev=4)
+ expected_status.tweak('A/B/F/E/alpha', wc_rev=5)
svntest.actions.run_and_verify_commit(wc_dir, expected_output,
expected_status, None,
None, None, None, None, wc_dir)
- copy_of_B_F_E_path = os.path.join(copy_of_B_path, 'F', 'E')
- # Search for the comment entitled "The Merge Kluge" elsewhere in
- # this file, to understand why we shorten and chdir() below.
- short_copy_of_B_F_E_path = shorten_path_kludge(copy_of_B_F_E_path)
+ for path in ('E', 'E1'):
+ path_name = os.path.join(copy_of_B_path, 'F', path)
+ # Search for the comment entitled "The Merge Kluge" elsewhere in
+ # this file, to understand why we shorten and chdir() below.
+ short_path_name = shorten_path_kludge(path_name)
- # Merge changes to alpha from rev 4.
- expected_output = wc.State(short_copy_of_B_F_E_path, {
- 'alpha' : Item(status='U '),
- })
- expected_status = wc.State(short_copy_of_B_F_E_path, {
- '' : Item(status=' M', wc_rev=3),
- 'alpha' : Item(status='M ', wc_rev=3),
- 'beta' : Item(status=' ', wc_rev=3),
- })
- expected_disk = wc.State('', {
- '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F/E:4'}),
- 'alpha' : Item(new_content_for_alpha),
- 'beta' : Item("This is the file 'beta'.\n"),
- })
- expected_skip = wc.State(short_copy_of_B_F_E_path, { })
- saved_cwd = os.getcwd()
- try:
- os.chdir(svntest.main.work_dir)
- svntest.actions.run_and_verify_merge(short_copy_of_B_F_E_path, '3', '4',
- sbox.repo_url + '/A/B/F/E',
- expected_output,
- expected_disk,
- expected_status,
- expected_skip,
- None,
- None,
- None,
- None,
- None, 1)
- finally:
- os.chdir(saved_cwd)
+ # Merge r5 to path_name.
+ expected_output = wc.State(short_path_name, {
+ 'alpha' : Item(status='U '),
+ })
+ expected_status = wc.State(short_path_name, {
+ '' : Item(status=' M', wc_rev=4),
+ 'alpha' : Item(status='M ', wc_rev=4),
+ 'beta' : Item(status=' ', wc_rev=4),
+ })
+ expected_disk = wc.State('', {
+ '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F/E:5'}),
+ 'alpha' : Item(new_content_for_alpha1),
+ 'beta' : Item("This is the file 'beta'.\n"),
+ })
+ expected_skip = wc.State(short_path_name, { })
+ saved_cwd = os.getcwd()
+ try:
+ os.chdir(svntest.main.work_dir)
+ svntest.actions.run_and_verify_merge(short_path_name, '4', '5',
+ sbox.repo_url + '/A/B/F/E',
+ expected_output,
+ expected_disk,
+ expected_status,
+ expected_skip,
+ None,
+ None,
+ None,
+ None,
+ None, 1)
+ finally:
+ os.chdir(saved_cwd)
- # Commit the result of the merge, creating revision 5.
- expected_output = svntest.wc.State(copy_of_B_F_E_path, {
- '' : Item(verb='Sending'),
- 'alpha' : Item(verb='Sending'),
- })
- svntest.actions.run_and_verify_commit(short_copy_of_B_F_E_path,
- expected_output, None, None,
- None, None, None, None, wc_dir)
+ # Commit the result of the merge, creating new revision.
+ expected_output = svntest.wc.State(path_name, {
+ '' : Item(verb='Sending'),
+ 'alpha' : Item(verb='Sending'),
+ })
+ svntest.actions.run_and_verify_commit(short_path_name,
+ expected_output, None, None,
+ None, None, None, None, wc_dir)
- # Edit A/B/F/E/alpha and commit it, creating revision 6.
+ # Edit A/B/F/E/alpha and commit it, creating revision 8.
new_content_for_alpha = 'new content to alpha\none more line\n'
svntest.main.file_write(alpha_path, new_content_for_alpha)
@@ -4321,9 +4354,9 @@
'alpha' : Item(verb='Sending'),
})
expected_status = wc.State(A_B_F_E_path, {
- '' : Item(status=' ', wc_rev=2),
- 'alpha' : Item(status=' ', wc_rev=6),
- 'beta' : Item(status=' ', wc_rev=2),
+ '' : Item(status=' ', wc_rev=4),
+ 'alpha' : Item(status=' ', wc_rev=8),
+ 'beta' : Item(status=' ', wc_rev=4),
})
svntest.actions.run_and_verify_commit(A_B_F_E_path, expected_output,
expected_status, None,
@@ -4333,44 +4366,50 @@
# this file, to understand why we shorten and chdir() below.
short_copy_of_B_path = shorten_path_kludge(copy_of_B_path)
- # Update the WC to bring /A/copy_of_B from rev 3 to rev 6.
- # Without this update expected_status tree would be cumbersome to
+ # Update the WC to bring /A/copy_of_B to rev 8.
+ # Without this update expected_status tree would be cumbersome to
# understand.
svntest.actions.run_and_verify_svn(None, None, [], 'update', wc_dir)
- # Merge changes from rev 3:6 of A/B into A/copy_of_B.
+ # Merge changes from rev 4:8 of A/B into A/copy_of_B.
expected_output = wc.State(short_copy_of_B_path, {
'F/E/alpha' : Item(status='U ')
})
expected_status = wc.State(short_copy_of_B_path, {
# When we merge multiple sub-targets, we record merge info on each
- # child.
- '' : Item(status=' M', wc_rev=6),
- 'F/E' : Item(status=' M', wc_rev=6),
- 'F/E/alpha' : Item(status='M ', wc_rev=6),
- 'F/E/beta' : Item(status=' ', wc_rev=6),
- 'lambda' : Item(status=' ', wc_rev=6),
- 'F' : Item(status=' ', wc_rev=6),
+ # child.
+ '' : Item(status=' M', wc_rev=8),
+ 'F/E' : Item(status=' M', wc_rev=8),
+ 'F/E/alpha' : Item(status='M ', wc_rev=8),
+ 'F/E/beta' : Item(status=' ', wc_rev=8),
+ 'F/E1' : Item(status=' M', wc_rev=8),
+ 'F/E1/alpha' : Item(status=' ', wc_rev=8),
+ 'F/E1/beta' : Item(status=' ', wc_rev=8),
+ 'lambda' : Item(status=' ', wc_rev=8),
+ 'F' : Item(status=' ', wc_rev=8)
})
expected_disk = wc.State('', {
- '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B:4-6'}),
- 'F/E' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F/E:4-6'}),
- 'F/E/alpha' : Item(new_content_for_alpha),
- 'F/E/beta' : Item("This is the file 'beta'.\n"),
- 'F' : Item(),
- 'lambda' : Item("This is the file 'lambda'.\n")
+ '' : Item(props={SVN_PROP_MERGE_INFO : '/A/B:5-8'}),
+ 'F/E' : Item(props={SVN_PROP_MERGE_INFO : '/A/B/F/E:5-8'}),
+ 'F/E/alpha' : Item(new_content_for_alpha),
+ 'F/E/beta' : Item("This is the file 'beta'.\n"),
+ 'F' : Item(),
+ 'F/E1' : Item(props={SVN_PROP_MERGE_INFO :
+ '/A/B/F/E:5\n/A/B/F/E1:5-8\n'}),
+ 'F/E1/alpha' : Item(new_content_for_alpha1),
+ 'F/E1/beta' : Item("This is the file 'beta'.\n"),
+ 'lambda' : Item("This is the file 'lambda'.\n")
})
expected_skip = wc.State(short_copy_of_B_path, { })
saved_cwd = os.getcwd()
try:
os.chdir(svntest.main.work_dir)
- svntest.actions.run_and_verify_merge(short_copy_of_B_path, '3', '6',
- sbox.repo_url + \
- '/A/B',
+ svntest.actions.run_and_verify_merge(short_copy_of_B_path, '4', '8',
+ sbox.repo_url + '/A/B',
expected_output,
expected_disk,
expected_status,
- expected_skip,
+ expected_skip,
None,
None,
None,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 8 09:43:05 2007