Hi All,
Subtree merge implementation violates the Reporter API by calling it in
a random order.
This patch fixes it, Testcase proves the fix.
Find the attached patch and log.
With regards
Kamesh Jayachandran
[[[
Reporter api calls should be made in a depth first order.
* subversion/libsvn_client/diff.c
(global): include svn_sorts.h
(discover_and_merge_children): Change the signature to accept
'depth_first_ordered_child_with_mergeinfo' of type 'apr_array_header_t **'
as an out param instead of 'apr_hash_t *'.
Sort 'children_with_mergeinfo' local hash by path and pass the sorted list
to cascaded calls to do_merge.
(do_merge): Change the signature to accept the Depth first ordered list of
as child with mergeinfo instead of hash.
(svn_client_merge3, svn_client_merge_peg3): Adjust for the above 2 API changes.
* subversion/tests/cmdline/merge_tests.py
(modify_and_commit_source_merge_the_resulting_rev_to_dst,
subtrees_with_merge_info_should_be_excluded_in_depth_first_order): New functions
(test_list): include the new testcase.
Patch by: kameshj
Found by: plundblad
]]]
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 23311)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -41,6 +41,7 @@
#include "svn_config.h"
#include "svn_props.h"
#include "svn_time.h"
+#include "svn_sorts.h"
#include "client.h"
#include <assert.h>
@@ -2196,7 +2197,7 @@
svn_boolean_t ignore_ancestry,
const svn_wc_diff_callbacks2_t *callbacks,
struct merge_cmd_baton *merge_b,
- apr_hash_t *children_with_mergeinfo,
+ apr_array_header_t *children_in_DF_order_with_mergeinfo,
apr_pool_t *pool)
{
apr_hash_t *target_mergeinfo;
@@ -2216,7 +2217,6 @@
svn_opt_revision_t *revision1, *revision2;
const svn_wc_entry_t *entry;
int i;
- apr_hash_index_t *hi;
ENSURE_VALID_REVISION_KINDS(initial_revision1->kind,
initial_revision2->kind);
@@ -2385,19 +2385,18 @@
operation such that no diff is retrieved for them from
the repository. */
apr_size_t target_wcpath_len = strlen(target_wcpath);
-
- for (hi = apr_hash_first(NULL, children_with_mergeinfo);
- hi;
- hi = apr_hash_next(hi))
+ int j=0;
+ for (j = 0; j < children_in_DF_order_with_mergeinfo->nelts; j++)
{
- const void *key;
- apr_ssize_t klen;
const char *child_repos_path;
const char *child_wcpath;
- apr_hash_this(hi, &key, &klen, NULL);
- child_wcpath = key;
- if (klen < target_wcpath_len ||
+ svn_sort__item_t *item =
+ &APR_ARRAY_IDX(children_in_DF_order_with_mergeinfo,
+ 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)
{
@@ -3458,8 +3457,8 @@
}
-/* Fill CHILDREN_WITH_MERGEINFO with child paths which might have
- intersecting merges (it should start be empty, but not NULL). For
+/* Fill *CHILDREN_IN_DF_ORDER_WITH_MERGEINFO with child paths which might have
+ intersecting merges. Here the paths are arranged in a Depth First Order. For
each such child, call do_merge() or do_single_file_merge() with the
appropriate arguments (based on the type of child). Use
PARENT_ENTRY and ADM_ACCESS to fill CHILDREN_WITH_MERGEINFO.
@@ -3468,7 +3467,7 @@
MERGE_CMD_BATON to do_merge() and do_single_file_merge(). All
allocation occurs in POOL. */
static svn_error_t *
-discover_and_merge_children(apr_hash_t *children_with_mergeinfo,
+discover_and_merge_children(apr_array_header_t **children_in_DF_order_with_mergeinfo,
const svn_wc_entry_t *parent_entry,
const char *parent_wc_url,
const char *initial_path1,
@@ -3482,8 +3481,9 @@
struct merge_cmd_baton *merge_cmd_baton,
apr_pool_t *pool)
{
- apr_hash_index_t *hi;
const svn_wc_entry_t *child_entry;
+ apr_hash_t *children_with_mergeinfo = apr_hash_make(pool);
+ int i=0;
SVN_ERR(svn_client__get_prop_from_wc(children_with_mergeinfo,
SVN_PROP_MERGE_INFO,
@@ -3494,17 +3494,18 @@
TRUE,
merge_cmd_baton->ctx,
pool));
- for (hi = apr_hash_first(pool, children_with_mergeinfo);
- hi;
- hi = apr_hash_next(hi))
+ *children_in_DF_order_with_mergeinfo = svn_sort__hash(children_with_mergeinfo,
+ svn_sort_compare_items_as_paths,
+ pool);
+ for (i = 0; i < (*children_in_DF_order_with_mergeinfo)->nelts; i++)
{
- const void *key;
const char *child_repos_path;
const char *child_wcpath;
const char *child_url;
-
- apr_hash_this(hi, &key, NULL, NULL);
- child_wcpath = key;
+ svn_sort__item_t *item = &APR_ARRAY_IDX(*children_in_DF_order_with_mergeinfo,
+ i,
+ svn_sort__item_t);
+ child_wcpath = item->key;
if (strcmp(child_wcpath, merge_cmd_baton->target) == 0)
continue;
SVN_ERR(svn_wc_entry(&child_entry, child_wcpath,
@@ -3513,7 +3514,6 @@
return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
_("'%s' is not under version control"),
svn_path_local_style(child_wcpath, pool));
-
child_repos_path = child_wcpath + strlen(merge_cmd_baton->target) + 1;
child_url = svn_path_join(parent_wc_url, child_repos_path, pool);
if (child_entry->kind == svn_node_file)
@@ -3541,7 +3541,7 @@
ignore_ancestry,
&merge_callbacks,
merge_cmd_baton,
- children_with_mergeinfo,
+ *children_in_DF_order_with_mergeinfo,
pool));
}
}
@@ -3569,7 +3569,7 @@
const char *URL1, *URL2;
const char *path1, *path2;
svn_opt_revision_t peg_revision;
- apr_hash_t *children_with_mergeinfo = apr_hash_make(pool);
+ apr_array_header_t *children_in_DF_order_with_mergeinfo;
/* This is not a pegged merge. */
peg_revision.kind = svn_opt_revision_unspecified;
@@ -3659,7 +3659,7 @@
if (strcmp(URL1, URL2) == 0)
{
/* Merge children with differing merge info. */
- SVN_ERR(discover_and_merge_children(children_with_mergeinfo,
+ SVN_ERR(discover_and_merge_children(&children_in_DF_order_with_mergeinfo,
entry,
URL1,
path1,
@@ -3688,7 +3688,7 @@
ignore_ancestry,
&merge_callbacks,
&merge_cmd_baton,
- children_with_mergeinfo,
+ children_in_DF_order_with_mergeinfo,
pool));
}
@@ -3754,7 +3754,7 @@
struct merge_cmd_baton merge_cmd_baton;
const char *URL;
const char *path;
- apr_hash_t *children_with_mergeinfo = apr_hash_make(pool);
+ apr_array_header_t *children_in_DF_order_with_mergeinfo;
/* if source1 or source2 are paths, we need to get the underlying url
* from the wc and save the initial path we were passed so we can use it as
@@ -3827,7 +3827,7 @@
else if (entry->kind == svn_node_dir)
{
/* Merge children with differing merge info. */
- SVN_ERR(discover_and_merge_children(children_with_mergeinfo,
+ SVN_ERR(discover_and_merge_children(&children_in_DF_order_with_mergeinfo,
entry,
URL,
path,
@@ -3855,7 +3855,7 @@
ignore_ancestry,
&merge_callbacks,
&merge_cmd_baton,
- children_with_mergeinfo,
+ children_in_DF_order_with_mergeinfo,
pool));
}
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 23311)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -4378,6 +4378,267 @@
finally:
os.chdir(saved_cwd)
+def modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
+ fs_src_path,
+ fs_dst_path,
+ canon_src_path,
+ contents,
+ cur_rev):
+ # Edit src and commit it, creating revision cur_rev+1.
+ wc_dir = sbox.wc_dir
+ new_rev = cur_rev + 1
+ svntest.main.file_write(fs_src_path, contents)
+
+ expected_output = svntest.wc.State(fs_src_path, {
+ '': Item(verb='Sending'),
+ })
+
+ expected_status = wc.State(fs_src_path,
+ { '': Item(wc_rev=new_rev, status=' ')})
+
+ svntest.actions.run_and_verify_commit(fs_src_path, expected_output,
+ expected_status, None,
+ None, None, None, None, fs_src_path)
+
+ # Update the WC to new_rev so that it would be easier to expect everyone
+ # to be at new_rev.
+ svntest.actions.run_and_verify_svn(None, None, [], 'update', wc_dir)
+
+ # Merge new_rev of fs_src_path to fs_dst_path
+
+ # Search for the comment entitled "The Merge Kluge" elsewhere in
+ # this file, to understand why we shorten and chdir() below.
+ short_fs_dst_path = shorten_path_kludge(fs_dst_path)
+ expected_status = wc.State(fs_dst_path,
+ { '': Item(wc_rev=new_rev, status='MM')})
+
+ saved_cwd = os.getcwd()
+ try:
+ os.chdir(svntest.main.work_dir)
+ svntest.actions.run_and_verify_svn(None,
+ ['U ' + short_fs_dst_path + '\n'],
+ [],
+ 'merge', '-c', str(new_rev),
+ sbox.repo_url + '/' + canon_src_path,
+ short_fs_dst_path)
+ finally:
+ os.chdir(saved_cwd)
+
+ svntest.actions.run_and_verify_status(fs_dst_path, expected_status)
+
+ return new_rev
+
+def subtrees_with_merge_info_should_be_excluded_in_depth_first_order(sbox):
+ "exclude subtrees with mergeinfo in DF order"
+
+ #copy /A/D to /A/copy-of-D it results in RONE
+ #create children at different hierarchies having some merge-info
+ #to test the set_path calls on a reporter in a non-depth-first order.
+ #Childrens considered
+ #/A/copy-of-D/G/pi
+ #/A/copy-of-D/G/rho
+ #/A/copy-of-D/G/tau
+ #/A/copy-of-D/G
+ #/A/copy-of-D/gamma
+ #/A/copy-of-D/H/chi
+ #/A/copy-of-D/H/omega
+ #/A/copy-of-D/H/psi
+ #/A/copy-of-D/H would have the mergeinfo.
+ #We run merges on individual files listed above.
+ #We create /A/D/umlaut directly over URL it results in rev RTWO
+ #When we merge /A/D with rRONE+1:RTWO it should merge smoothly
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ A_path = os.path.join(wc_dir, 'A')
+
+ A_D_path = os.path.join(A_path, 'D')
+ copy_of_A_D_path = os.path.join(A_path, 'copy-of-D')
+
+ A_D_gamma_path = os.path.join(A_D_path, 'gamma')
+ copy_of_A_D_gamma_path = os.path.join(copy_of_A_D_path, 'gamma')
+
+ A_D_G_path = os.path.join(A_D_path, 'G')
+ copy_of_A_D_G_path = os.path.join(copy_of_A_D_path, 'G')
+
+ A_D_G_pi_path = os.path.join(A_D_G_path, 'pi')
+ copy_of_A_D_G_pi_path = os.path.join(copy_of_A_D_G_path, 'pi')
+
+ A_D_G_rho_path = os.path.join(A_D_G_path, 'rho')
+ copy_of_A_D_G_rho_path = os.path.join(copy_of_A_D_G_path, 'rho')
+
+ A_D_G_tau_path = os.path.join(A_D_G_path, 'tau')
+ copy_of_A_D_G_tau_path = os.path.join(copy_of_A_D_G_path, 'tau')
+
+ A_D_H_path = os.path.join(A_D_path, 'H')
+ copy_of_A_D_H_path = os.path.join(copy_of_A_D_path, 'H')
+
+ A_D_H_chi_path = os.path.join(A_D_H_path, 'chi')
+ copy_of_A_D_H_chi_path = os.path.join(copy_of_A_D_H_path, 'chi')
+
+ A_D_H_omega_path = os.path.join(A_D_H_path, 'omega')
+ copy_of_A_D_H_omega_path = os.path.join(copy_of_A_D_H_path, 'omega')
+
+ A_D_H_psi_path = os.path.join(A_D_H_path, 'psi')
+ copy_of_A_D_H_psi_path = os.path.join(copy_of_A_D_H_path, 'psi')
+
+ svntest.main.run_svn(None, "cp", A_D_path, copy_of_A_D_path)
+
+ expected_output = svntest.wc.State(wc_dir, {
+ 'A/copy-of-D' : Item(verb='Adding'),
+ })
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.add({
+ 'A/copy-of-D' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/G' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/G/pi' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/G/rho' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/G/tau' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/H' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/H/chi' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/H/omega' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/H/psi' : Item(status=' ', wc_rev=2),
+ 'A/copy-of-D/gamma' : Item(status=' ', wc_rev=2),
+ })
+ svntest.actions.run_and_verify_commit(wc_dir, expected_output,
+ expected_status, None,
+ None, None, None, None, wc_dir)
+
+ # Edit A/D/G/pi and commit it, creating revision new_rev.
+ new_content_for_pi = 'new content to pi\n'
+ new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
+ A_D_G_pi_path,
+ copy_of_A_D_G_pi_path,
+ 'A/D/G/pi',
+ new_content_for_pi,
+ 2)
+
+ # Edit A/D/G/rho and commit it, creating revision new_rev.
+ new_content_for_rho = 'new content to rho\n'
+ new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
+ A_D_G_rho_path,
+ copy_of_A_D_G_rho_path,
+ 'A/D/G/rho',
+ new_content_for_rho,
+ new_rev)
+
+ # Edit A/D/G/tau and commit it, creating revision new_rev.
+ new_content_for_tau = 'new content to tau\n'
+ new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
+ A_D_G_tau_path,
+ copy_of_A_D_G_tau_path,
+ 'A/D/G/tau',
+ new_content_for_tau,
+ new_rev)
+
+ # Edit A/D/H/chi and commit it, creating revision new_rev.
+ new_content_for_chi = 'new content to chi\n'
+ new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
+ A_D_H_chi_path,
+ copy_of_A_D_H_chi_path,
+ 'A/D/H/chi',
+ new_content_for_chi,
+ new_rev)
+
+ # Edit A/D/H/omega and commit it, creating revision new_rev.
+ new_content_for_omega = 'new content to omega\n'
+ new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
+ A_D_H_omega_path,
+ copy_of_A_D_H_omega_path,
+ 'A/D/H/omega',
+ new_content_for_omega,
+ new_rev)
+
+ # Edit A/D/H/psi and commit it, creating revision new_rev.
+ new_content_for_psi = 'new content to psi\n'
+ new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
+ A_D_H_psi_path,
+ copy_of_A_D_H_psi_path,
+ 'A/D/H/psi',
+ new_content_for_psi,
+ new_rev)
+
+ # Edit A/D/gamma and commit it, creating revision new_rev.
+ new_content_for_gamma = 'new content to gamma\n'
+ new_rev = modify_and_commit_source_merge_the_resulting_rev_to_dst(sbox,
+ A_D_gamma_path,
+ copy_of_A_D_gamma_path,
+ 'A/D/gamma',
+ new_content_for_gamma,
+ new_rev)
+ copy_of_A_D_wc_rev = new_rev
+ svntest.actions.run_and_verify_svn(None,
+ ['\n',
+ 'Committed revision ' + str(new_rev+1) + '.\n'],
+ [],
+ 'mkdir', sbox.repo_url + '/A/D/umlaut',
+ '-m', "log msg")
+ rev_to_merge_to_copy_of_D = new_rev + 1
+
+ # Search for the comment entitled "The Merge Kluge" elsewhere in
+ # this file, to understand why we shorten and chdir() below.
+ short_copy_of_A_D_path = shorten_path_kludge(copy_of_A_D_path)
+
+ expected_output = wc.State(short_copy_of_A_D_path, {
+ 'umlaut' : Item(status='A '),
+ })
+
+ expected_status = wc.State(short_copy_of_A_D_path, {
+ '' : Item(status=' M', wc_rev=copy_of_A_D_wc_rev),
+ 'G' : Item(status=' ', wc_rev=copy_of_A_D_wc_rev),
+ 'G/pi' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
+ 'G/rho' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
+ 'G/tau' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
+ 'H' : Item(status=' ', wc_rev=copy_of_A_D_wc_rev),
+ 'H/chi' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
+ 'H/omega' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
+ 'H/psi' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
+ 'gamma' : Item(status='MM', wc_rev=copy_of_A_D_wc_rev),
+ 'umlaut' : Item(status='A ', copied='+', wc_rev='-'),
+ })
+
+ svn_merged_rev_val = "3-" + str(rev_to_merge_to_copy_of_D)
+ expected_disk = wc.State('', {
+ '' : Item(props={SVN_PROP_MERGE_INFO : '/A/D:' + svn_merged_rev_val}),
+ 'G' : Item(),
+ 'G/pi' : Item(new_content_for_pi,
+ props={SVN_PROP_MERGE_INFO : '/A/D/G/pi:' + svn_merged_rev_val}),
+ 'G/rho' : Item(new_content_for_rho,
+ props={SVN_PROP_MERGE_INFO : '/A/D/G/rho:' + svn_merged_rev_val}),
+ 'G/tau' : Item(new_content_for_tau,
+ props={SVN_PROP_MERGE_INFO : '/A/D/G/tau:' + svn_merged_rev_val}),
+ 'H' : Item(),
+ 'H/chi' : Item(new_content_for_chi,
+ props={SVN_PROP_MERGE_INFO : '/A/D/H/chi:' + svn_merged_rev_val}),
+ 'H/omega' : Item(new_content_for_omega,
+ props={SVN_PROP_MERGE_INFO : '/A/D/H/omega:' + svn_merged_rev_val}),
+ 'H/psi' : Item(new_content_for_psi,
+ props={SVN_PROP_MERGE_INFO : '/A/D/H/psi:' + svn_merged_rev_val}),
+ 'gamma' : Item(new_content_for_gamma,
+ props={SVN_PROP_MERGE_INFO : '/A/D/gamma:' + svn_merged_rev_val}),
+ 'umlaut' : Item(),
+ })
+ expected_skip = wc.State(short_copy_of_A_D_path, { })
+ saved_cwd = os.getcwd()
+ try:
+ os.chdir(svntest.main.work_dir)
+ svntest.actions.run_and_verify_merge(short_copy_of_A_D_path,
+ 2,
+ str(rev_to_merge_to_copy_of_D),
+ sbox.repo_url + '/A/D',
+ expected_output,
+ expected_disk,
+ expected_status,
+ expected_skip,
+ None,
+ None,
+ None,
+ None,
+ None, 1)
+ finally:
+ os.chdir(saved_cwd)
+
########################################################################
# Run the tests
@@ -4425,6 +4686,7 @@
XFail(merge_eolstyle_handling),
avoid_repeated_merge_using_inherited_merge_info,
avoid_repeated_merge_on_subtree_with_merge_info,
+ subtrees_with_merge_info_should_be_excluded_in_depth_first_order,
]
if __name__ == '__main__':
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jan 31 14:23:42 2007