On Tue, 24 Jan 2006, Julian Foad wrote:
> Daniel Rall wrote:
...
> >http://svn.collab.net/viewcvs/svn?rev=18179&view=rev
...
> 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.
Yeah. I spent quite a bit of time looking into them, but didn't come
up with entirely satisfactory answers for all of them.
> 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.
PATH is currently passed to svn_client__ra_session_from_path(), after
being set to the first target. At least for the command-line client,
this is correct for the URI case, where any URI required to be the
first target. Is this also correct when svn_client_log3() is used as
an API (not by the command-line client)?
This certainly does seem questionable for local paths, where the
path's ".svn/entries" file is eventually used to procure its URI (via
svn_client_url_from_path). If multiple local paths to different
directories are specified in the TARGETS argument, will the wrong base
URI be inferred when opening the RA session?
> >+ /* 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.
This change is from Ramaswamy's latest patch -- you asked him similar
questions in your previous reviews during the patch's evolution. This
is the most informative one:
http://svn.haxx.se/dev/archive-2005-09/0281.shtml
I too puzzled over this change, and given the help of the test suite
came to the conclusion that it must be right. See
log_tests.py:url_missing_in_head(), which uses r8 of "alpha" as a peg
revision. "alpha" is removed in r9, and IIRC, without that block, the
test fails (Ram mentions something similar in his response at the URL
above). I think this code needs an in-line comment...suggestions?
WRT using a later revision number or date goes, this was the intent of
my use of the svn_opt_revision_t END argument to
svn_client__ra_session_from_path() from the early incarnations of my
patch (at first foolishly assuming that END would always be younger),
following the example of svn_client_blame2(). 'svn blame' has a
different enough implementation that it was not clear whether it
needed any similar processing.
...
> "@peg_revision" -> "@a peg_revision"
r18212, thanks.
> 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.
Yes, and I checked. It was documented to do so (by you :) after this
came up in discussion of Ram's original patches:
http://svn.collab.net/viewcvs/svn?rev=16034&view=rev
> >o In the new call to [svn_client__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.
>
> (... 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 :-).
I experimented with using what's now named IGNORED_REVNUM in place of
START_REVNUM or END_REVNUM, but the values didn't necessarily match
up, and it was simpler to continue calling
svn_client__get_revision_number(). If someone was able to
successfully make use of IGNORED_REVNUM, changing this could be a
perfomance enhancement, but would also add to code complexity.
IGNORED_URL discussed above.
> >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.
I considered the same factors after Peter Lundblad's suggestion, and
decided to have svn_client_log() delegate to svn_client_log2() on the
basis that it is 1) simpler to maintain and 2) resulted in less code
change.
--
Daniel Rall
- application/pgp-signature attachment: stored
Received on Wed Jan 25 02:45:55 2006