> Great! Once build_tree_from_wc() can handle files, will that be
> sufficient to allow run_and_verify_merge() to handle them also?
>
>
Not really sure. But hope so.
> ...
>
>> With the style you suggested above simplified the code a lot.
>>
>
> Cool! With that much condensing of the code, I'd say the added
> conceptual complexity is a fair trade-off.
>
Yes.
> ...
>
>> [[[
>> Reporter api calls should be made in a depth first order.
>>
>> * subversion/libsvn_client/diff.c
>> (global): Include "svn_sorts.h".
>>
>
> Drop "(global): ". Subversion's convention is no prefix, or "(): " (I
> personally hate the latter).
>
>
>> (discover_and_merge_children): Change the signature to accept
>> 'children_with_mergeinfo' of type 'apr_array_header_t **'
>> as an out param instead of 'apr_hash_t *'.
>> Sort 'children_with_mergeinfo_hash' a local hash by path and pass the
>> sorted list to subsequent calls to do_merge.
>> (do_merge): Change the signature to accept the '*depth first ordered list*
>>
> ^
> Odd formatting in here.
>
Corrected.
>
>> of children 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
>> (tweak_src_then_merge_to_dest,
>> obey_reporter_api_semantics_while_doing_subtree_merges):
>> New functions.
>> (test_list): Include the new testcase.
>>
>> Patch by: kameshj
>> Found by: plundblad
>> Reviewed by: plundblad
>> dlr
>> ]]]
>>
>
> Two comments inline below.
>
> ...
>
>> --- subversion/tests/cmdline/merge_tests.py (revision 23381)
>> +++ subversion/tests/cmdline/merge_tests.py (working copy)
>>
> ...
>
>> +def tweak_src_then_merge_to_dest(sbox, src_path, dst_path,
>> + canon_src_path, contents, cur_rev):
>> + """Edit src and commit it. This results in new_rev.
>> + Merge new_rev to dst_path. Return new_rev."""
>>
> ...
>
>> + return new_rev
>> +
>> +def obey_reporter_api_semantics_while_doing_subtree_merges(sbox):
>> + "drive reporter api in depth first 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 depth-first order.
>> + # Children considered
>> + # /A/copy-of-D/
>> + # G/pi
>> + # G/rho
>> + # G/tau
>> + # G
>> + # gamma
>> + # H/chi
>> + # H/omega
>> + # H/psi
>> + # H all are made to 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 rONE+1:TWO of /A/D on /A/copy-of-D it should merge smoothly.
>>
>
> I was hoping that the amount of vertical screen real estate could be
> compressed by putting all those paths on the same line here (I'm not
> convinced we need all those paths listed again at all).
>
>
Corrected them to be short and precise.
> ...
>
>> + new_rev = 2
>>
>
> Isn't this more like "cur_rev" in this context (or maybe just "rev"
> for brevity)?
>
>
I changed all instances of new_rev to cur_rev in this function as it
really is.
>> + for path in (["A", "D", "G", "pi"],
>> + ["A", "D", "G", "rho"],
>> + ["A", "D", "G", "tau"],
>> + ["A", "D", "H", "chi"],
>> + ["A", "D", "H", "omega"],
>> + ["A", "D", "H", "psi"],
>> + ["A", "D", "gamma"]):
>> + path_name = os.path.join(wc_dir, *path)
>> + canon_path_name = os.path.join(*path)
>> + path[1] = "copy-of-D"
>> + copy_of_path_name = os.path.join(wc_dir, *path)
>> + var_name = 'new_content_for_' + path[len(path) - 1]
>> + file_contents = "new content to " + path[len(path) - 1] + "\n"
>> + globals()[var_name] = file_contents
>> + new_rev = tweak_src_then_merge_to_dest(sbox, path_name,
>> + copy_of_path_name, canon_path_name,
>> + file_contents, 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
>>
> ...
>
> Looks great, Kamesh. With the additional fixes suggested by Peter,
> this is ready to commit and polish in the repository.
>
I addressed the comments by peter here. Except the 'uri_encode' and
'svn_path_is_ancestor' stuff. Both I would like to be a different commit.
Find the attached patch and log.
Thanks.
With regards
Kamesh Jayachandran
[[[
Reporter api calls should be made in a depth first order.
* subversion/libsvn_client/diff.c
Include "svn_sorts.h".
(discover_and_merge_children): Change the signature to accept
'children_with_mergeinfo' of type 'apr_array_header_t **'
as an out param instead of 'apr_hash_t *'.
Sort 'children_with_mergeinfo_hash' a local hash by path and pass the
sorted list to subsequent calls to do_merge.
(do_merge): Change the signature to accept the
'*depth first ordered list* of children 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
(tweak_src_then_merge_to_dest,
obey_reporter_api_semantics_while_doing_subtree_merges):
New functions.
(test_list): Include the new testcase.
Patch by: kameshj
Found by: plundblad
Reviewed by: plundblad
dlr
]]]
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 23394)
+++ 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>
@@ -2179,7 +2180,9 @@
CHILDREN_WITH_MERGEINFO may contain child paths with merge info
which differs from that of the merge target root (it may be empty,
- but not NULL). Because of this, we drive the diff editor in such a
+ but not NULL). CHILDREN_WITH_MERGEINFO list should have entries sorted in
+ depth first order as mandated by the reporter API.
+ Because of this, we drive the diff editor in such a
way that it avoids merging child paths when a merge is driven for
their parent path. */
static svn_error_t *
@@ -2196,7 +2199,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_with_mergeinfo,
apr_pool_t *pool)
{
apr_hash_t *target_mergeinfo;
@@ -2216,7 +2219,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 +2387,17 @@
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;
+ for (j = 0; j < children_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_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 +3458,8 @@
}
-/* Fill CHILDREN_WITH_MERGEINFO with child paths which might have
- intersecting merges (it should start be empty, but not NULL). For
+/* Fill *CHILDREN_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 +3468,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_with_mergeinfo,
const svn_wc_entry_t *parent_entry,
const char *parent_wc_url,
const char *initial_path1,
@@ -3482,10 +3482,11 @@
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_hash = apr_hash_make(pool);
+ int i;
- SVN_ERR(svn_client__get_prop_from_wc(children_with_mergeinfo,
+ SVN_ERR(svn_client__get_prop_from_wc(children_with_mergeinfo_hash,
SVN_PROP_MERGE_INFO,
merge_cmd_baton->target,
FALSE,
@@ -3494,17 +3495,18 @@
TRUE,
merge_cmd_baton->ctx,
pool));
- for (hi = apr_hash_first(pool, children_with_mergeinfo);
- hi;
- hi = apr_hash_next(hi))
+ *children_with_mergeinfo = svn_sort__hash(children_with_mergeinfo_hash,
+ svn_sort_compare_items_as_paths,
+ pool);
+ for (i = 0; i < (*children_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_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 +3515,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 +3542,7 @@
ignore_ancestry,
&merge_callbacks,
merge_cmd_baton,
- children_with_mergeinfo,
+ *children_with_mergeinfo,
pool));
}
}
@@ -3569,7 +3570,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_with_mergeinfo;
/* This is not a pegged merge. */
peg_revision.kind = svn_opt_revision_unspecified;
@@ -3659,7 +3660,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_with_mergeinfo,
entry,
URL1,
path1,
@@ -3754,7 +3755,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_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 +3828,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_with_mergeinfo,
entry,
URL,
path,
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 23394)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -4378,6 +4378,187 @@
finally:
os.chdir(saved_cwd)
+def tweak_src_then_merge_to_dest(sbox, src_path, dst_path,
+ canon_src_path, contents, cur_rev):
+ """Edit src and commit it. This results in new_rev.
+ Merge new_rev to dst_path. Return new_rev."""
+
+ wc_dir = sbox.wc_dir
+ new_rev = cur_rev + 1
+ svntest.main.file_write(src_path, contents)
+
+ expected_output = svntest.wc.State(src_path, {
+ '': Item(verb='Sending'),
+ })
+
+ expected_status = wc.State(src_path,
+ { '': Item(wc_rev=new_rev, status=' ')})
+
+ svntest.actions.run_and_verify_commit(src_path, expected_output,
+ expected_status, None,
+ None, None, None, None, 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 src_path to dst_path.
+
+ # Search for the comment entitled "The Merge Kluge" elsewhere in
+ # this file, to understand why we shorten and chdir() below.
+ short_dst_path = shorten_path_kludge(dst_path)
+ expected_status = wc.State(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_dst_path + '\n'],
+ [],
+ 'merge', '-c', str(new_rev),
+ sbox.repo_url + '/' + canon_src_path,
+ short_dst_path)
+ finally:
+ os.chdir(saved_cwd)
+
+ svntest.actions.run_and_verify_status(dst_path, expected_status)
+
+ return new_rev
+
+def obey_reporter_api_semantics_while_doing_subtree_merges(sbox):
+ "drive reporter api in depth first 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 depth-first order.
+ # On all 'file' descendants of /A/copy-of-D/ we run merges.
+ # We create /A/D/umlaut directly over URL it results in rev rTWO.
+ # When we merge rONE+1:TWO of /A/D on /A/copy-of-D 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(wc_dir, 'A', 'D')
+ copy_of_A_D_path = os.path.join(wc_dir, 'A', 'copy-of-D')
+
+ 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)
+
+
+ cur_rev = 2
+ for path in (["A", "D", "G", "pi"],
+ ["A", "D", "G", "rho"],
+ ["A", "D", "G", "tau"],
+ ["A", "D", "H", "chi"],
+ ["A", "D", "H", "omega"],
+ ["A", "D", "H", "psi"],
+ ["A", "D", "gamma"]):
+ path_name = os.path.join(wc_dir, *path)
+ canon_path_name = os.path.join(*path)
+ path[1] = "copy-of-D"
+ copy_of_path_name = os.path.join(wc_dir, *path)
+ var_name = 'new_content_for_' + path[len(path) - 1]
+ file_contents = "new content to " + path[len(path) - 1] + "\n"
+ globals()[var_name] = file_contents
+ cur_rev = tweak_src_then_merge_to_dest(sbox, path_name,
+ copy_of_path_name, canon_path_name,
+ file_contents, cur_rev)
+
+ copy_of_A_D_wc_rev = cur_rev
+ svntest.actions.run_and_verify_svn(None,
+ ['\n',
+ 'Committed revision ' + str(cur_rev+1) + '.\n'],
+ [],
+ 'mkdir', sbox.repo_url + '/A/D/umlaut',
+ '-m', "log msg")
+ rev_to_merge_to_copy_of_D = cur_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='-'),
+ })
+
+ merged_rangelist = "3-%d" % rev_to_merge_to_copy_of_D
+
+
+ expected_disk = wc.State('', {
+ '' : Item(props={SVN_PROP_MERGE_INFO : '/A/D:' + merged_rangelist}),
+ 'G' : Item(),
+ 'G/pi' : Item(new_content_for_pi,
+ props={SVN_PROP_MERGE_INFO : '/A/D/G/pi:' + merged_rangelist}),
+ 'G/rho' : Item(new_content_for_rho,
+ props={SVN_PROP_MERGE_INFO : '/A/D/G/rho:' + merged_rangelist}),
+ 'G/tau' : Item(new_content_for_tau,
+ props={SVN_PROP_MERGE_INFO : '/A/D/G/tau:' + merged_rangelist}),
+ 'H' : Item(),
+ 'H/chi' : Item(new_content_for_chi,
+ props={SVN_PROP_MERGE_INFO : '/A/D/H/chi:' + merged_rangelist}),
+ 'H/omega' : Item(new_content_for_omega,
+ props={SVN_PROP_MERGE_INFO : '/A/D/H/omega:' + merged_rangelist}),
+ 'H/psi' : Item(new_content_for_psi,
+ props={SVN_PROP_MERGE_INFO : '/A/D/H/psi:' + merged_rangelist}),
+ 'gamma' : Item(new_content_for_gamma,
+ props={SVN_PROP_MERGE_INFO : '/A/D/gamma:' + merged_rangelist}),
+ '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 +4606,7 @@
XFail(merge_eolstyle_handling),
avoid_repeated_merge_using_inherited_merge_info,
avoid_repeated_merge_on_subtree_with_merge_info,
+ obey_reporter_api_semantics_while_doing_subtree_merges,
]
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 Feb 14 16:04:55 2007