Hi,
IN general, I think this patch is good. Some comments below.
Kamesh Jayachandran writes:
> 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.
>
> (do_merge): Change the signature to accept the Depth first ordered list of
^ s/D/d/
> as child with mergeinfo instead of hash.
I don't understand this sentence.
> (svn_client_merge3, svn_client_merge_peg3): Adjust for the above 2 API changes.
>
Keep lines < 80 chars.
> * 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
Same and missing period.
> (test_list): include the new testcase.
s/i/I/
>
> Patch by: kameshj
This can go if you commit yourself. (Just a reminder.)
> 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"
> @@ -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,
Please keep the old shorter name. The constraint about the ordering
should be in the function documentation.
> 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;
Spurious initialization of j.
> + 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,
Indentation looks a bit off.
> + 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)
It is not exactly part of this patch, but wouldn't svn_path_is_ancestor
be possible to use in some way here? If so, it would be
clearear, at least to me;)
> {
> @@ -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
Please use a shorter name.
> {
> - 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;
Spaces around parens. OH, but this initializatioin is not needed...
>
> @@ -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);
Whatch out for long lines;) There seem to be more long lines, but I'm not
adding a remark for every instance.
> + 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);
Also not changed here, but that added path here won't be URL-encoded, right?
> if (child_entry->kind == svn_node_file)
> 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,
Hey, it has to be possible to find a name that's not 55 characters long!
In general, I think clear and reasonably verbose names improve readability,
but such long names does the oposite.
Also, it would be nice with some documentation for this new function.
Sorry for ending the review here, but I'm currently running out of time.
I will ocntinue with the review next week.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 9 18:30:39 2007