Re: Questions about code in svn_client_log5()'s helper func resolve_log_targets()
From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Fri, 3 May 2013 21:37:00 +0100 (BST)
A bit more review.
> + If TARGETS contains a single URL and one or more relative paths, then
> + if (memcmp(&(range->start), &(range->end),
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.
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()?
C. Michael Pilato wrote:
> Was reviewing your svn_client_log5() changes. There are a couple of places
This is an archived mail posted to the Subversion Dev mailing list.