Peter Lundblad wrote:
> 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.
>
It should have been "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.
>>
>>
> Keep lines < 80 chars.
>
Done.
>
>> * 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.)
>
Yes I am aware of this.
>
>
>> 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.
>
Done.
>
>> 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.
>
Done.
>
>> + 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.
>
Yes because of big variable name. Now fixed it.
>
>> + 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;)
>
No. Here we intend to do 'svn_path_is_child'. I don't prefer
'svn_path_is_child' because it does extra things which I am not
interested in (Giving a tail portions since match.). Given that I know
about my input here('child_wcpath', 'target_wcpath'), I don't want it to
undergo the rigorous ':' checks for windows.
>> {
>> @@ -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.
>
Done.
>
>> {
>> - 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...
>
Spaces around parens?? Taken care of spurious initialisation.
>
>>
>> @@ -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.
>
>
Taken care.
>> + 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?
>
Should we?
>> 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
>
>
Thanks for the review. Find the new patch in response to Dan's comments.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 13 10:19:04 2007