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

Re: Questions about code in svn_client_log5()'s helper func resolve_log_targets()

From: Paul Burba <ptburba_at_gmail.com>
Date: Thu, 9 May 2013 13:00:07 -0400

On Fri, May 3, 2013 at 4:37 PM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> Hi Paul.
> A bit more review.
>> + If TARGETS contains a single URL and one or more relative paths, then
>> + set *RA_TARGET to a copy of that URL and *CONDENSED_PATHS to a copy of
>> + each relative path after the URL.
>> [...]
>> +resolve_log_targets()

Done (see r1480723)

>> +find_youngest_and_oldest_revs(...)
>> +{
>> [...]
>> + return;
>> +}
> Redundant "return".


>> + if (memcmp(&(range->start), &(range->end),
>> + sizeof(svn_opt_revision_t)) == 0)
>> + start_same_as_end = TRUE;
> I don't think 'memcmp' is a safe way of comparing svn_opt_revision_t structures, since they may have holes for padding.

Fixed that with an element-by-element compare.

> From the context, I can see you are comparing them only in order to eliminate duplicate look-ups of the same value, so a false negative result would still produce a correct final result. Nevertheless, I don't like it, but the next observation may make this point moot.
> These functions:
> resolve_wc_opt_revs()
> resolve_wc_head_revs()
> resolve_wc_date_revs()
> together with the code that calls them, seem to be implementing the basic "convert an svn_opt_revision_t to a revision number" functionality that we already have in other places. Is that right? If so, could we avoid re-writing that functionality here?
> The only thing it appears to be doing that a simple call to, say, svn_client__get_revision_number() doesn't do, is avoid opening a session if one is not needed here. Instead, couldn't we change svn_client__get_revision_number() to be able to open a session iff one is needed? But wait -- in fact we're going to need a session anyway -- the caller opens one if this function doesn't. So can't we simply open one before calling this function, and let this function make simple calls to svn_client__get_revision_number()?
> - Julian
> C. Michael Pilato wrote:
>> Was reviewing your svn_client_log5() changes. There are a couple of places
>> in your reworked svn_client_log5() code (resolve_log_targets(),
>> specifically) that read like so or similar:
>> if (peg_revision->kind == svn_opt_revision_unspecified)
>> (*peg_revision).kind = svn_opt_revision_head;
>> Is there any reason for that not to be simply:
>> if (peg_revision->kind == svn_opt_revision_unspecified)
>> peg_revision->kind = svn_opt_revision_head;
>> ?
>> Also, I would suggest that, instead of initializing the *relative_targets
>> array with a single slot, we go ahead and use the number of slots we know
>> we'll need (so we can avoid resizing the array later):
>> /* Initialize the output array. At a minimum, we need room for one
>> (possibly empty) relpath. Otherwise, we have to hold a relpath
>> for every item in TARGETS except the first. */
>> *relative_targets = apr_array_make(result_pool,
>> MAX(1, targets->nelts - 1),
>> sizeof(const char *));
>> Finally, do we need to be strdup()ing the stuff we put into the
>> relative_targets array? Looks to me (via code inspection) like perhaps not.
>> It's entirely possible that you didn't introduce any of this stuff with
>> your
>> recent code reorg here -- I'm not claiming otherwise -- but I wanted to make
>> sure you agreed with me before I started messing around in that relatively
>> complex bit of code.

Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Received on 2013-05-09 19:00:38 CEST

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.