Peter Lundblad wrote:
> Kamesh Jayachandran writes:
>
>> Peter Lundblad wrote:
>>
>>>
>>>
>>>> + 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.
>>
>
> That's a good point about svn_path_is_child...
>
> This code is a bit complex, so please tell me if I am misunderstanding.
> To me it apperas like we have a big hash table with mergeinfo for
> the target of the whole merge and descendant paths in that working copy
> subtree. Then, in this function, we are working with a subtree of the
> original merge, rooted at target_wcpath. What the test above wants to do is
> to fitler out parts of the whole merge tree that are not descendants of
> of target_wcpath. Is this correct?
>
Yes.
> Assuming that my interpration is right, what does your code do if there's
> a sibling of target_wcpath whose name contains target_wcpath as a prefix?
> The sibling is longer than target_wcpath, so the first test fails.
> The strings are not equal, so the second test fails.
> The third test will also fail, because the strings are equal up until and
> including target_wcpath_len. If I'm not mistaken (which might well be the
> case), this sibling will be treated as a child which is not exactly what we
> want.
>
>
Great catch!. Now I buy svn_path_is_ancestor. Will post a different
patch probably with a testcase to exhibit the failure you mentioned above.
> This is not the first such bug we've had in our codebase and that's why
> svn_path_is_ancestor was introduced. So, why not do something like this:
> if (svn_path_is_ancestor(target_wcpath, child_wcpath) &&
> strcmp(target_wcpath, child_wcpath) != 0)
> /* Report this path... */
>
> It is easier to read and it avoids suble bugs as the one described above.
>
>
>>>> {
>>>> - 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.
>>
>>
> Sorry, should have been spaces around operators, but nevermind.
>
>
>>>> + 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?
>>
>
> You are constructing a new URL by concatenating an URL and
> a working copy path fragment, right? What happens if that fragment
> contains characters that should be URI escaped?
>
> Thanks,
> //Peter
>
>
Will try to make it fail and fix it.
Thanks for the great review.
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 12:16:34 2007