[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH][merge-tracking] Reporter api calls should be made in depth first order (Fix and TestCase)

From: Kamesh Jayachandran <kamesh_at_collab.net>
Date: 2007-02-14 16:04:44 CET

> 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

This is an archived mail posted to the Subversion Dev mailing list.