On Wed, 2008-09-24 at 14:03 -0400, Paul Burba wrote:
> On Wed, Sep 24, 2008 at 12:39 PM, Julian Foad
> <julianfoad_at_btopenworld.com> wrote:
> > I noticed that calculate_remaining_ranges()'s doc string implies its
> > "child" argument's "path" field is always used:
> >
> >> /* Helper for do_file_merge and do_directory_merge (by way of
> >> populate_remaining_ranges() for the latter).
> >>
> >> Determine what portions of URL1_at_REVISION1 -> URL2_at_REVISION2 have already
> >> been merged to CHILD->PATH and populate CHILD->REMAINING_RANGES with the
> >> ranges that still need merging.
> >> [...]
> >
> > ... but do_file_merge() calls it with the "child" argument being a
> > structure freshly created with apr_pcalloc(), so its "path" field is
> > null.
> >
> > Is there some special case missing from the doc string?
>
> Hi Julian,
>
> calculate_remaining_ranges(), nor any of its helpers, ever actually
> uses CHILD->PATH, even when called by
> do_directory_merge()/populate_remaining_ranges().
>
> Perhaps the doc string should simply say, "Determine what portions of
> URL1_at_REVISION1 -> URL2_at_REVISION2 have already been merged to CHILD and
> populate CHILD->REMAINING_RANGES with the ranges that still need
> merging."?
That doesn't help the situation I pointed out, where the whole structure
passed as "child" is blank. The phrase "have already been merged to
CHILD" is meaningless with that input.
> Regardless, even though it is currently unnecessary I think
> do_file_merge() should allocate an empty string for CHILD->PATH when
> it allocates the svn_client__merge_path_t *:
>
> [[[
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 33269)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -5115,6 +5115,7 @@
> const char *source_root_url;
> svn_client__merge_path_t *merge_target =
> apr_pcalloc(pool, sizeof(*merge_target));
> + merge_target->path = apr_pcalloc(pool, sizeof(*(merge_target->path)));
Er... wouldn't
merge_target->path = "";
be nicer?
> SVN_ERR(svn_ra_get_repos_root2(merge_b->ra_session1,
> &source_root_url, pool));
>
> ]]]
>
> It strikes me as awkward and inconsistent to special case
> svn_client__merge_path_t for file merges. Does that sound reasonable?
Yes, that sentence sounds reasonable. I don't know about the earlier
part.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-24 23:45:05 CEST