[PATCH] svn log taking multiple revision arguments

I got frustrated that svn log can't take multiple revisions on the
command line (which contributes in part to svnmerge.py being
unbearably slow)...and, well, this patch works for me. =) Against,
my svn server (located in a far far away nether land *grin*), this
reduces the time from:

svn log -rX -rY -rZ
takes 7.354 seconds
cf.
for i in X Y Z; do svn log -r$i; done
takes 18.162 seconds

(The time to do add'l revs with this is essentially O(1)+6secs
compared to ~6secs per revision previously. Yay.)

Before I proceed any further with this patch (ie unit test), I'd like
to get some feedback to make sure I'm on the right path. If anyone
wants to help out, lemme know. =)

Outstanding issues/questions:
 - Can we support multiple -c syntax?
 - How can we support limit - as currently implemented, limit is
handled by the RA layer, so...uh...
 - Moving some of the parameter checks from svn/log-cmd.c into
svn_client_log5 seems like the right thing to do, but...

Thanks. -- justin

(Attached and inline as I have no idea what CN's MX is gonna do.)

Have 'svn log' take multiple revs options to optimize session usage.

* subversion/include/svn_client.h
  (svn_client_log5): New log variant that takes a set of ranges.
  (svn_client_log4): Deprecate.
* subversion/libsvn_client/deprecated.c
  (svn_client_log4): Wrap log5.
* subversion/libsvn_client/log.c
  (svn_client_log4): Rename to...
  (svn_client_log5): this; take array of revisions and loop logs; moves some
  of the checking that the client did into here; optimize session-opening.
* subversion/svn/log-cmd.c
  (svn_cl__log): Prevent -c used with multiple revs; use svn_client_log5;
  move manipulation of unspecified ranges into svn_client_log5.
* subversion/svn/main.c
  (main): Permit log to have multiple revs.

Index: subversion/svn/log-cmd.c
===================================================================
--- subversion/svn/log-cmd.c (revision 35065)
+++ subversion/svn/log-cmd.c (working copy)
@@ -471,53 +471,26 @@ svn_cl__log(apr_getopt_t *os,
   /* Determine if they really want a two-revision range. */
   if (opt_state->used_change_arg)
     {
- if (opt_state->start_revision.value.number <
- opt_state->end_revision.value.number)
- opt_state->start_revision = opt_state->end_revision;
+ if (opt_state->revision_ranges->nelts > 1)
+ {
+ return svn_error_create
+ (SVN_ERR_CLIENT_BAD_REVISION, NULL,
+ _("Can not mix change revision with multiple revisions"));
+ }
+ svn_opt_revision_range_t *range;
+ range = APR_ARRAY_IDX(opt_state->revision_ranges, 0,
+ svn_opt_revision_range_t *);
+
+ if (range->start.value.number < range->end.value.number)
+ range->start = range->end;
       else
- opt_state->end_revision = opt_state->start_revision;
+ range->end = range->start;
     }

   /* Strip peg revision if targets contains an URI. */
   SVN_ERR(svn_opt_parse_path(&peg_revision, &true_path, target, pool));
   APR_ARRAY_IDX(targets, 0, const char *) = true_path;

- if ((opt_state->start_revision.kind != svn_opt_revision_unspecified)
- && (opt_state->end_revision.kind == svn_opt_revision_unspecified))
- {
- /* If the user specified exactly one revision, then start rev is
- set but end is not. We show the log message for just that
- revision by making end equal to start.
-
- Note that if the user requested a single dated revision, then
- this will cause the same date to be resolved twice. The
- extra code complexity to get around this slight inefficiency
- doesn't seem worth it, however. */
-
- opt_state->end_revision = opt_state->start_revision;
- }
- else if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
- {
- /* Default to any specified peg revision. Otherwise, if the
- first target is an URL, then we default to HEAD:0. Lastly,
- the default is BASE:0 since WC_at_HEAD may not exist. */
- if (peg_revision.kind == svn_opt_revision_unspecified)
- {
- if (svn_path_is_url(target))
- opt_state->start_revision.kind = svn_opt_revision_head;
- else
- opt_state->start_revision.kind = svn_opt_revision_base;
- }
- else
- opt_state->start_revision = peg_revision;
-
- if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
- {
- opt_state->end_revision.kind = svn_opt_revision_number;
- opt_state->end_revision.value.number = 0;
- }
- }
-
   if (svn_path_is_url(target))
     {
       for (i = 1; i < targets->nelts; i++)
@@ -609,10 +582,9 @@ svn_cl__log(apr_getopt_t *os,
       APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_DATE;
       if (!opt_state->quiet)
         APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_LOG;
- SVN_ERR(svn_client_log4(targets,
+ SVN_ERR(svn_client_log5(targets,
                               &peg_revision,
- &(opt_state->start_revision),
- &(opt_state->end_revision),
+ opt_state->revision_ranges,
                               opt_state->limit,
                               opt_state->verbose,
                               opt_state->stop_on_copy,
Index: subversion/svn/main.c
===================================================================
--- subversion/svn/main.c (revision 35065)
+++ subversion/svn/main.c (working copy)
@@ -1689,7 +1689,8 @@ main(int argc, const char *argv[])
     }

   /* Only merge supports multiple revisions/revision ranges. */
- if (subcommand->cmd_func != svn_cl__merge)
+ if (subcommand->cmd_func != svn_cl__merge
+ && subcommand->cmd_func != svn_cl__log)
     {
       if (opt_state.revision_ranges->nelts > 1)
         {
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 35065)
+++ subversion/include/svn_client.h (working copy)
@@ -1911,9 +1911,10 @@ svn_client_status(svn_revnum_t *result_rev,
  */

 /**
- * 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).
+ * Invoke @a receiver with @a receiver_baton on each log message from
+ * each start/end revision pair in the @a revision_ranges in turn,
+ * inclusive (but never invoke @a receiver on a given log message more
+ * than once).
  *
  * @a targets contains either a URL followed by zero or more relative
  * paths, or 1 working copy path, as <tt>const char *</tt>, for which log
@@ -1953,9 +1954,29 @@ svn_client_status(svn_revnum_t *result_rev,
  * If @a ctx->notify_func2 is non-NULL, then call @a ctx->notify_func2/baton2
  * with a 'skip' signal on any unversioned targets.
  *
+ * @since New in 1.6.
+ */
+svn_error_t *
+svn_client_log5(const apr_array_header_t *targets,
+ const svn_opt_revision_t *peg_revision,
+ const apr_array_header_t *revision_ranges,
+ int limit,
+ svn_boolean_t discover_changed_paths,
+ svn_boolean_t strict_node_history,
+ svn_boolean_t include_merged_revisions,
+ const apr_array_header_t *revprops,
+ svn_log_entry_receiver_t receiver,
+ void *receiver_baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
+
+/**
+ * Similar to svn_client_log5(), but takes explicit start and end parameters
+ * instead of an array of revision ranges.
+ *
+ * @deprecated Provided for compatibility with the 1.5 API.
  * @since New in 1.5.
  */
-
 svn_error_t *
 svn_client_log4(const apr_array_header_t *targets,
                 const svn_opt_revision_t *peg_revision,
Index: subversion/libsvn_client/deprecated.c
===================================================================
--- subversion/libsvn_client/deprecated.c (revision 35065)
+++ subversion/libsvn_client/deprecated.c (working copy)
@@ -885,6 +885,40 @@ svn_client_ls(apr_hash_t **dirents,

 /*** From log.c ***/
 svn_error_t *
+svn_client_log4(const apr_array_header_t *targets,
+ const svn_opt_revision_t *peg_revision,
+ 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_boolean_t include_merged_revisions,
+ const apr_array_header_t *revprops,
+ svn_log_entry_receiver_t receiver,
+ void *receiver_baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
+{
+ apr_array_header_t *revision_ranges;
+ svn_opt_revision_range_t *range;
+
+ range = apr_palloc(pool, sizeof(svn_opt_revision_range_t));
+ range->start = *start;
+ range->end = *end;
+
+ revision_ranges = apr_array_make(pool, 0,
+ sizeof(svn_opt_revision_range_t *));
+
+ APR_ARRAY_PUSH(revision_ranges, svn_opt_revision_range_t *) = range;
+
+ return svn_client_log5(targets, peg_revision, revision_ranges, limit,
+ discover_changed_paths, strict_node_history,
+ include_merged_revisions, revprops, receiver,
+ receiver_baton, ctx, pool);
+}
+
+
+svn_error_t *
 svn_client_log3(const apr_array_header_t *targets,
                 const svn_opt_revision_t *peg_revision,
                 const svn_opt_revision_t *start,
Index: subversion/libsvn_client/log.c
===================================================================
--- subversion/libsvn_client/log.c (revision 35065)
+++ subversion/libsvn_client/log.c (working copy)
@@ -291,10 +291,9 @@ pre_15_receiver(void *baton, svn_log_entry_t *log_

 svn_error_t *
-svn_client_log4(const apr_array_header_t *targets,
+svn_client_log5(const apr_array_header_t *targets,
                 const svn_opt_revision_t *peg_revision,
- const svn_opt_revision_t *start,
- const svn_opt_revision_t *end,
+ const apr_array_header_t *revision_ranges,
                 int limit,
                 svn_boolean_t discover_changed_paths,
                 svn_boolean_t strict_node_history,
@@ -307,33 +306,117 @@ svn_error_t *
 {
   svn_ra_session_t *ra_session;
   const char *url_or_path;
+ svn_boolean_t is_url;
+ svn_boolean_t has_log_revprops;
   const char *actual_url;
   apr_array_header_t *condensed_targets;
   svn_revnum_t ignored_revnum;
   svn_opt_revision_t session_opt_rev;
   const char *ra_target;
+ pre_15_receiver_baton_t rb;
+ int i;

- if ((start->kind == svn_opt_revision_unspecified)
- || (end->kind == svn_opt_revision_unspecified))
+ if (revision_ranges->nelts == 0)
     {
       return svn_error_create
         (SVN_ERR_CLIENT_BAD_REVISION, NULL,
          _("Missing required revision specification"));
     }

+ /* Use the passed URL, if there is one. */
   url_or_path = APR_ARRAY_IDX(targets, 0, const char *);
+ is_url = svn_path_is_url(url_or_path);

- /* Use the passed URL, if there is one. */
- if (svn_path_is_url(url_or_path))
+ if (is_url && SVN_CLIENT__REVKIND_NEEDS_WC(peg_revision->kind))
     {
- if (SVN_CLIENT__REVKIND_NEEDS_WC(peg_revision->kind) ||
- SVN_CLIENT__REVKIND_NEEDS_WC(start->kind) ||
- SVN_CLIENT__REVKIND_NEEDS_WC(end->kind))
+ return svn_error_create
+ (SVN_ERR_CLIENT_BAD_REVISION, NULL,
+ _("Revision type requires a working copy path, not a URL"));
+ }

- return svn_error_create
- (SVN_ERR_CLIENT_BAD_REVISION, NULL,
- _("Revision type requires a working copy path, not a URL"));
+ session_opt_rev.kind = svn_opt_revision_unspecified;

+ for (i = 0; i < revision_ranges->nelts; i++)
+ {
+ svn_opt_revision_range_t *range;
+
+ range = APR_ARRAY_IDX(revision_ranges, i, svn_opt_revision_range_t *);
+
+ if ((range->start.kind != svn_opt_revision_unspecified)
+ && (range->end.kind == svn_opt_revision_unspecified))
+ {
+ /* If the user specified exactly one revision, then start rev is
+ * set but end is not. We show the log message for just that
+ * revision by making end equal to start.
+ *
+ * Note that if the user requested a single dated revision, then
+ * this will cause the same date to be resolved twice. The
+ * extra code complexity to get around this slight inefficiency
+ * doesn't seem worth it, however. */
+ range->end = range->start;
+ }
+ else if (range->start.kind == svn_opt_revision_unspecified)
+ {
+ /* Default to any specified peg revision. Otherwise, if the
+ * first target is an URL, then we default to HEAD:0. Lastly,
+ * the default is BASE:0 since WC_at_HEAD may not exist. */
+ if (peg_revision->kind == svn_opt_revision_unspecified)
+ {
+ if (svn_path_is_url(url_or_path))
+ range->start.kind = svn_opt_revision_head;
+ else
+ range->start.kind = svn_opt_revision_base;
+ }
+ else
+ range->start = *peg_revision;
+
+ if (range->end.kind == svn_opt_revision_unspecified)
+ {
+ range->end.kind = svn_opt_revision_number;
+ range->end.value.number = 0;
+ }
+ }
+
+ if ((range->start.kind == svn_opt_revision_unspecified)
+ || (range->end.kind == svn_opt_revision_unspecified))
+ {
+ return svn_error_create
+ (SVN_ERR_CLIENT_BAD_REVISION, NULL,
+ _("Missing required revision specification"));
+ }
+
+ if (is_url
+ && (SVN_CLIENT__REVKIND_NEEDS_WC(range->start.kind)
+ || SVN_CLIENT__REVKIND_NEEDS_WC(range->end.kind)))
+ {
+ return svn_error_create
+ (SVN_ERR_CLIENT_BAD_REVISION, NULL,
+ _("Revision type requires a working copy path, not a URL"));
+ }
+
+ /* Determine the revision to open the RA session to. */
+ if (session_opt_rev.kind == svn_opt_revision_unspecified)
+ {
+ if (range->start.kind == svn_opt_revision_number &&
+ range->end.kind == svn_opt_revision_number)
+ {
+ session_opt_rev =
+ (range->start.value.number > range->end.value.number ?
+ range->start : range->end);
+ }
+ else if (range->start.kind == svn_opt_revision_date &&
+ range->end.kind == svn_opt_revision_date)
+ {
+ session_opt_rev =
+ (range->start.value.date > range->end.value.date ?
+ range->start : range->end);
+ }
+ }
+ }
+
+ /* Use the passed URL, if there is one. */
+ if (is_url)
+ {
       /* Initialize this array, since we'll be building it below */
       condensed_targets = apr_array_make(pool, 1, sizeof(const char *));

@@ -418,16 +501,6 @@ svn_error_t *
       targets = real_targets;
     }

- /* 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;

   {
     /* If this is a revision type that requires access to the working copy,
@@ -442,6 +515,19 @@ svn_error_t *
                                              &actual_url, ra_target, NULL,
                                              peg_revision, &session_opt_rev,
                                              ctx, pool));
+
+ SVN_ERR(svn_ra_has_capability(ra_session, &has_log_revprops,
+ SVN_RA_CAPABILITY_LOG_REVPROPS, pool));
+
+ if (!has_log_revprops) {
+ /* See above pre-1.5 notes. */
+ rb.ctx = ctx;
+ SVN_ERR(svn_client_open_ra_session(&rb.ra_session, actual_url,
+ ctx, pool));
+ rb.revprops = revprops;
+ rb.receiver = real_receiver;
+ rb.baton = real_receiver_baton;
+ }
   }

   /* It's a bit complex to correctly handle the special revision words
@@ -491,59 +577,47 @@ svn_error_t *
    * epg wonders if the repository could send a unified stream of log
    * entries if the paths and revisions were passed down.
    */
- {
- svn_revnum_t start_revnum, end_revnum, youngest_rev = SVN_INVALID_REVNUM;
- const char *path = APR_ARRAY_IDX(targets, 0, const char *);
- svn_boolean_t has_log_revprops;
+ for (i = 0; i < revision_ranges->nelts; i++)
+ {
+ svn_revnum_t start_revnum, end_revnum, youngest_rev = SVN_INVALID_REVNUM;
+ const char *path = APR_ARRAY_IDX(targets, 0, const char *);
+ svn_opt_revision_range_t *range;

- SVN_ERR(svn_client__get_revision_number
- (&start_revnum, &youngest_rev, ra_session, start, path, pool));
- SVN_ERR(svn_client__get_revision_number
- (&end_revnum, &youngest_rev, ra_session, end, path, pool));
+ range = APR_ARRAY_IDX(revision_ranges, i, svn_opt_revision_range_t *);

- SVN_ERR(svn_ra_has_capability(ra_session, &has_log_revprops,
- SVN_RA_CAPABILITY_LOG_REVPROPS, pool));
- if (has_log_revprops)
- return svn_ra_get_log2(ra_session,
- condensed_targets,
- start_revnum,
- end_revnum,
- limit,
- discover_changed_paths,
- strict_node_history,
- include_merged_revisions,
- revprops,
- real_receiver,
- real_receiver_baton,
- pool);
- else
- {
- /* See above pre-1.5 notes. */
- pre_15_receiver_baton_t rb;
- rb.ctx = ctx;
- SVN_ERR(svn_client_open_ra_session(&rb.ra_session, actual_url,
- ctx, pool));
- rb.revprops = revprops;
- rb.receiver = real_receiver;
- rb.baton = real_receiver_baton;
- SVN_ERR(svn_client__ra_session_from_path(&ra_session,
- &ignored_revnum,
- &actual_url, ra_target, NULL,
- peg_revision,
- &session_opt_rev,
- ctx, pool));
- return svn_ra_get_log2(ra_session,
- condensed_targets,
- start_revnum,
- end_revnum,
- limit,
- discover_changed_paths,
- strict_node_history,
- include_merged_revisions,
- svn_compat_log_revprops_in(pool),
- pre_15_receiver,
- &rb,
- pool);
- }
- }
+ SVN_ERR(svn_client__get_revision_number
+ (&start_revnum, &youngest_rev, ra_session, &range->start, path,
+ pool));
+ SVN_ERR(svn_client__get_revision_number
+ (&end_revnum, &youngest_rev, ra_session, &range->end, path,
+ pool));
+
+ if (has_log_revprops)
+ SVN_ERR(svn_ra_get_log2(ra_session,
+ condensed_targets,
+ start_revnum,
+ end_revnum,
+ limit,
+ discover_changed_paths,
+ strict_node_history,
+ include_merged_revisions,
+ revprops,
+ real_receiver,
+ real_receiver_baton,
+ pool));
+ else
+ SVN_ERR(svn_ra_get_log2(ra_session,
+ condensed_targets,
+ start_revnum,
+ end_revnum,
+ limit,
+ discover_changed_paths,
+ strict_node_history,
+ include_merged_revisions,
+ svn_compat_log_revprops_in(pool),
+ pre_15_receiver,
+ &rb,
+ pool));
+ }
+ return SVN_NO_ERROR;
 }