[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: calculate_remaining_ranges(child->path == NULL)

From: Paul Burba <ptburba_at_gmail.com>
Date: Wed, 24 Sep 2008 14:03:44 -0400

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."?

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)));

       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?

Paul

---------------------------------------------------------------------
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 20:03:56 CEST

This is an archived mail posted to the Subversion Dev mailing list.