You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2007/02/27 15:47:55 UTC
[PATCH]Use svn_path_is_ancestor instead of custom incomplete code
Hi All,
Peter Lundblad and Daniel Rall have suggested to use
'svn_path_is_ancestor' in deciding whether to call (no-diff)set_path on
a subtree.
Attached patch has the testcase and the fix.
With regards
Kamesh Jayachandran
Re: [PATCH]Use svn_path_is_ancestor instead of custom incomplete
code
Posted by Kamesh Jayachandran <ka...@collab.net>.
Thanks Dan. Committed at r23953.
With regards
Kamesh Jayachandran
Daniel Rall wrote:
> +1 to commit.
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]Use svn_path_is_ancestor instead of custom incomplete code
Posted by Daniel Rall <dl...@collab.net>.
+1 to commit.
On Thu, 08 Mar 2007, Kamesh Jayachandran wrote:
> 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
Re: [PATCH]Use svn_path_is_ancestor instead of custom incomplete
code
Posted by Kamesh Jayachandran <ka...@collab.net>.
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
Re: [PATCH]Use svn_path_is_ancestor instead of custom incomplete code
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 27 Feb 2007, Peter Lundblad wrote:
...
> An alternative to adding a new test might be to introduce this into
> existing tests that deal with excluding of subtrees. I don't know
> if this is feasable, so this is just a suggestion. Maybe that's
> better than having lots of tests caused by short-lived bugs during
> the initial implementation.
I too prefer this approach, and have been doing my best to avoid
introducing new tests when they can be reasonably avoided.
Re:
[PATCH]Use svn_path_is_ancestor instead of custom incomplete code
Posted by Peter Lundblad <pl...@google.com>.
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.
>
...
> Patch by: kameshj
> Found by: plundblad
^ I'm actually "lundblad", but I understand why you're confused;)
> 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.
An alternative to adding a new test might be to introduce this into
existing tests that deal with excluding of subtrees. I don't know
if this is feasable, so this is just a suggestion.
Maybe that's better than having lots of tests caused by short-lived
bugs during the initial implementation.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org