rooneg@tigris.org writes:
> Log:
> Implement a '--limit' switch for 'svn log', which allows you to limit
> the number of log entries returned.
Yay!
A few small comments below. (I should really have reviewed the patch
earlier, rather than the commit now, sorry for being late.)
> --- trunk/subversion/include/svn_client.h (original)
> +++ trunk/subversion/include/svn_client.h Mon Sep 27 20:22:42 2004
> @@ -729,6 +729,8 @@
> *
> * ### todo: the above paragraph is not fully implemented yet.
> *
> + * If @a limit is non-zero only the first @a limit logs are returned.
> + *
> * If @a discover_changed_paths is set, then the `@a changed_paths' argument
> * to @a receiver will be passed on each invocation.
> *
> @@ -757,6 +759,24 @@
> * If @a ctx->notify_func is non-null, then call @a ctx->notify_func/baton
> * with a 'skip' signal on any unversioned targets.
> *
> + */
> +svn_error_t *
> +svn_client_log2 (const apr_array_header_t *targets,
> + const svn_opt_revision_t *start,
> + const svn_opt_revision_t *end,
> + int limit,
> + svn_boolean_t discover_changed_paths,
> + svn_boolean_t strict_node_history,
> + svn_log_message_receiver_t receiver,
> + void *receiver_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
We need a "@since New in 1.2." line in this doc string now, right?
> --- trunk/subversion/include/svn_ra.h (original)
> +++ trunk/subversion/include/svn_ra.h Mon Sep 27 20:22:42 2004
> [...]
> + * [...]
> + *
> + * If @a limit is non-zero only return the first @a limit logs.
> + *
> + * [...]
> [...]
> + svn_error_t *(*get_log2) (void *session_baton,
> + const apr_array_header_t *paths,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + int limit,
> + svn_boolean_t discover_changed_paths,
> + svn_boolean_t strict_node_history,
> + svn_log_message_receiver_t receiver,
> + void *receiver_baton,
> + apr_pool_t *pool);
Likewise, "@since New in 1.2." for this one's doc string.
The phrasing "...only return the first @a limit logs" makes intuitive
sense, but "only invoke @a receiver on the first @a limit logs" would
be more formally correct. (The only thing actually returned is an
error, or null.) The same point may apply to other doc strings in
this commit...
> --- trunk/subversion/include/svn_repos.h (original)
> +++ trunk/subversion/include/svn_repos.h Mon Sep 27 20:22:42 2004
> @@ -691,6 +691,8 @@
> * "Single repository, multiple projects?" for more. We may simple
> * need to offer a few different semantics for @a paths.
> *
> + * If @a limit is non-zero then only return the first @a limit logs.
> + *
> * If @a discover_changed_paths, then each call to @a receiver passes a
> * <tt>const apr_hash_t *</tt> for the receiver's @a changed_paths
> * argument; the hash's keys are all the paths committed in that revision.
...like this one...
> @@ -716,6 +718,24 @@
> * See also the documentation for @c svn_log_message_receiver_t.
> *
> * Use @a pool for temporary allocations.
> + */
> +svn_error_t *
> +svn_repos_get_logs3 (svn_repos_t *repos,
> + const apr_array_header_t *paths,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + int limit,
> + svn_boolean_t discover_changed_paths,
> + svn_boolean_t strict_node_history,
> + svn_repos_authz_func_t authz_read_func,
> + void *authz_read_baton,
> + svn_log_message_receiver_t receiver,
> + void *receiver_baton,
> + apr_pool_t *pool);
... which also wants the "@since New in 1.2." tag.
> --- trunk/subversion/libsvn_ra_dav/log.c (original)
> +++ trunk/subversion/libsvn_ra_dav/log.c Mon Sep 27 20:22:42 2004
> @@ -79,6 +79,9 @@
> svn_log_message_receiver_t receiver;
> void *receiver_baton;
>
> + int limit;
> + int count;
> +
> /* If `receiver' returns error, it is stored here. */
> svn_error_t *err;
> };
> @@ -215,6 +218,14 @@
> lb->date,
> lb->msg,
> lb->subpool);
> +
> + /* Compatability cruft so that we can provide limit functionality
> + even if the server doesn't support it. */
> + if (lb->limit && (++lb->count == lb->limit))
> + {
> + lb->err = NULL;
> + return SVN_RA_DAV__XML_INVALID;
> + }
>
> reset_log_item (lb);
Exploratory question:
Why are we returning SVN_RA_DAV__XML_INVALID here? I realize it may
work out to have the desired effect, but to the reader, it looks like
we're returning an error for a non-error condition. (I'm assuming a
non-modern server is not considered an error! :-) )
The only purpose of SVN_RA_DAV__XML_INVALID is to tell the XML parser
to stop processing, so it won't take us to the 'ELEM_log_item' case
again, right? But, this strategy doesn't save the user much wall
clock time, because they still end up waiting for all the unuseable
logs to come down the wire. (Stop me if I'm wrong...)
At first, I thought a cleaner-looking solution might be to simply
protect the receiver invocation with a limit check, e.g.:
case ELEM_log_item:
{
### if (lb->limit && (++lb->count == lb->limit))
### {
svn_error_t *err = (*(lb->receiver))(lb->receiver_baton,
lb->changed_paths,
lb->revision,
lb->author,
lb->date,
lb->msg,
lb->subpool);
### }
(New lines marked with "###" because I'm too lazy to generate a real
patch.)
But then I realized, the difference between that solution and yours is
that your solution spares us the effort of parsing XML only to ignore
the results. In which case, maybe the best thing is to just add a
comment to your change, pointing out that we're not really generating
an error, we're just stopping parsing, and say why this is better.
If any of this analysis misses the point, please bop me. I just
wanted to know if you had considered the other kind of conditional,
and if so, whether you rejected it for the reasons I'm guessing you
did.
> --- trunk/subversion/libsvn_ra_dav/ra_dav.h (original)
> +++ trunk/subversion/libsvn_ra_dav/ra_dav.h Mon Sep 27 20:22:42 2004
> @@ -721,6 +721,18 @@
> apr_array_header_t *location_revisions,
> apr_pool_t *pool);
>
> +svn_error_t *
> +svn_ra_dav__get_log2 (void *session_baton,
> + const apr_array_header_t *paths,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + int limit,
> + svn_boolean_t discover_changed_paths,
> + svn_boolean_t strict_node_history,
> + svn_log_message_receiver_t receiver,
> + void *receiver_baton,
> + apr_pool_t *pool);
> +
I realize the original was probably not documented either, and they're
both internal functions, but it's still worth putting an "@since New
in 1.2." marker when replacing like this. That will help us find all
the old cruft, when we go to 2.0.
> --- trunk/subversion/libsvn_ra_local/ra_plugin.c (original)
> +++ trunk/subversion/libsvn_ra_local/ra_plugin.c Mon Sep 27 20:22:42 2004
> @@ -609,15 +609,16 @@
>
> +svn_ra_local__get_log2 (void *session_baton,
> + const apr_array_header_t *paths,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + int limit,
> + svn_boolean_t discover_changed_paths,
> + svn_boolean_t strict_node_history,
> + svn_log_message_receiver_t receiver,
> + void *receiver_baton,
> + apr_pool_t *pool)
Same comment applies here.
That's it,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 28 18:26:32 2004