S.Ramaswamy wrote:
>>>Index: subversion/include/svn_client.h
>>>===================================================================
>>>--- subversion/include/svn_client.h (revision 15298)
>>>+++ subversion/include/svn_client.h (working copy)
>>>@@ -915,7 +915,8 @@
>>> /**
>>> * Invoke @a receiver with @a receiver_baton on each log message from @a
>>> * start to @a end in turn, inclusive (but never invoke @a receiver on a
>>>- * given log message more than once).
>>>+ * given log message more than once). @a peg_revision indicates in which
>>>+ * revision @a targets are valid.
>>
>>This needs to specify what svn_opt_revision_unspecified means as a peg revision ...
>>
>>> * @a targets contains either a URL followed by zero or more relative
>>> * paths, or a list of working copy paths, as <tt>const char *</tt>,
>>
>>[...]
>>
>>> /**
>>>+ * Similar to svn_client_log3(), but with the @a peg_revision
>>>+ * parameter always set to @c svn_opt_revision_unspecified.
>>
>>... if svn_client_log2() is documented in terms of it. I wasn't easily able to
>>determine the meaning for myself.
>
> I guess you are saying that, if an explanation of
> svn_opt_revision_unspecified in the svn_client_log3() doc string is
> included, then the svn_client_log2() doc string becomes easier to
> understand. Will do.
I meant that, but more strongly: I meant the pair of doc strings are
incomplete. If log2 depends on "unspecified" having a particular meaning in
log3, then either that meaning must be documented publically for log3 or it is
an implementation detail that shouldn't be mentioned publically for log2. In
the former case, it would be fine for log2 to be described in terms of what
value it passes to log3; in the latter case, log2 would have to be described in
terms of its effect rather than that value which would be an implementation detail.
Now that I write and think about this clearly enough to present those two
options, I wonder whether log3 needs to accept "unspecified" as part of its
publically documented interface. It would be better if it didn't, unless there
is some good public reason to do so. We have too many unnecessary defaults
already in APIs like this; we shouldn't add more unless there is a clear
benefit. It could still accept "unspecified" privately to facilitate the
implementation of log2.
(When I said, "I wasn't easily able to determine the meaning for myself," I
meant that I wasn't able to determine the meaning of "unspecified", even by
looking at the implementation. It certainly wasn't possible from the doc strings.)
>>>+ if (start->kind == svn_opt_revision_number &&
>>>+ end->kind == svn_opt_revision_number)
>>>+ revision = (start->value.number > end->value.number) ? *start : *end;
>>>+ else
>>>+ revision.kind = svn_opt_revision_unspecified;
>>>
>>>+ SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
>>>+ &url_p, path,
>>>+ peg_revision, &revision, ctx,
>>>+ pool));
>>>+
>>
>>Please could you explain this change. I don't understand why something special
>>is happening if both the start and end revisions are specified as numbers
>>rather than, for example, dates. And I don't really understand _what_ special
>>behaviour is happening in that case.
I'd still like you to explain this part of the patch if you would, please.
>>I see from the implementation of svn_client__ra_session_from_path() that an
>>unspecified "revision" means "revision = peg_revision", so wouldn't it be
>>clearer to make your "else" clause say "revision = *peg_revision" (or an
>>equivalent pointer assignment) ?
>
> I overlooked the fact that dates/keywords can also be specified as
> revisions. I can change that to,
>
> if (start->kind == svn_opt_revision_number) &&
> (end->kind == svn_opt_revision_number)
> revision = (start->value.number > end->value.number) ? *start : *end;
> else if (start->kind == svn_opt_revision_date) &&
> (end->kind == svn_opt_revision_date)
> revision = (start->value.date > end->value.date) ? *start : *end;
>
> Revision numbers can also be specified as a combination of dates and
> revision numbers, as well as a combination of keywords (PREV, HEAD, etc),
> numbers etc. In those cases the revision gets set to
> svn_opt_revision_unspecified in the current patch(v2).
Yes, but _why_ do you want it set to "unspecified" in such cases? :-) (Maybe
it's really obvious, but not to me. Maybe I just haven't studied it hard enough.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 5 18:27:12 2005