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

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

s/CONDENSED_PATHS/RELATIVE_TARGETS/.

> +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.  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.
Received on 2013-05-03 22:38:33 CEST