Daniel Rall wrote:
> I've committed a patch adding support for peg revisions to 'svn log'
> (and of course the corresponding underlying APIs), based on work by
> myself and Ramaswamy (and all the reviews that work received):
>
> http://svn.collab.net/viewcvs/svn?rev=18179&view=rev
OK. It's certainly good to have this.
We have to pay a bit of attention to the details that seem to be unfinished. I
think you asked some questions and didn't get any answers.
Note: some confusion may result from the fact that, as far as I recall, this
log function has never worked properly for multiple local paths. We (I)
changed our command-line client to not support that usage, but it seems we
didn't fix the problem in the API. I don't know whether that affects any of
the issues below.
> /* Find the base URL and condensed targets relative to it. */
> - SVN_ERR (svn_path_condense_targets (&base_url, &condensed_targets,
> + SVN_ERR (svn_path_condense_targets (&ignored_url, &condensed_targets,
> target_urls, TRUE, pool));
I don't see how it can be right to ignore the base URL determined by that
function and yet use the condensed targets that are relative to that base.
> + /* Determine the revision to open the RA session to. */
> + if (start->kind == svn_opt_revision_number &&
> + end->kind == svn_opt_revision_number)
> + session_opt_rev = (start->value.number > end->value.number ?
> + *start : *end);
> + else if (start->kind == svn_opt_revision_date &&
> + end->kind == svn_opt_revision_date)
> + session_opt_rev = (start->value.date > end->value.date ? *start : *end);
> + else
> + session_opt_rev.kind = svn_opt_revision_unspecified;
I don't see how that can make sense. This seems to say: if the two revisions
were specified by the user in different ways then we don't need to specify what
revision to open the RA session to; yet if they are both numbers or both dates
then we should use the later. Please explain why. All I can think of, if this
is correct, is that specifying the later revision is an optimisation or "hint"
but is not actually necessary.
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 18178)
> +++ subversion/include/svn_client.h (revision 18179)
> @@ -1177,7 +1177,11 @@
> * for which log messages are desired. The repository info is
> * determined by taking the common prefix of the target entries' URLs.
> * @a receiver is invoked only on messages whose revisions involved a
> - * change to some path in @a targets.
> + * change to some path in @a targets. @a peg_revision indicates in
> + * which revision @a targets are valid. If @peg_revision is @c
"@peg_revision" -> "@a peg_revision"
You wrote, with an earlier draft of the patch:
> o The previous log function is passing its "end" parameter for
> "peg_revision" (as done by svn_client_blame), but the doc string for
> svn_client__open_ra_session_from_path makes me think that perhaps
> svn_opt_revision_unspecified might be more appropriate?
You're talking about your new (tiny, forwarding) version of svn_client_log2()
calling svn_client_log3(), passing "end" for "peg_revision" in that earlier
patch, or, in the version you checked in, passing
"svn_opt_revision_unspecified" for "peg_revision". I hope you have determined
that the latter preserves svn_client_log2()'s existing behaviour; I haven't
checked.
> o In the new call to svn_client__open_ra_session_from_path, the local
> variable session_url is never used after being set. What should be
> done with it (e.g. replace base_url)? Similarly, end_revnum is used,
> but perhaps no longer exactly right.
(The function is called svn_client__ra_session_from_path(). In the checked-in
code, you have called these two results "ignored_url" and "ignored_revnum"
respectively.)
Hmm. I don't know. I could believe that they might not be needed but we need
someone else to check it (or me to spend more time on it :-).
> o Peter suggested leaving svn_client_log() calling svn_client_log2().
... rather than changing it to call svn_client_log3() directly. The overhead
of an extra call is totally insignificant in this context, so I would choose
whichever is easiest to understand and maintain, which would probably be
calling svn_client_log2(), but in this case it doesn't make much difference so
what you checked in is fine.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jan 24 22:18:49 2006