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?
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.
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 13 11:32:17 2007