[PATCH][Issue 2287] svn_client_log should take a peg revision

On Sat, 14 Jan 2006, Peter N. Lundblad wrote:

> I also find it easier (and more likely:-) to review patches sent to dev@
> and it is easier to reply with comments inline as well.
 
Here's what I attached to the issue (which pass log_tests.py), with a
change log. I'll pick it back up on Monday. Also, a few issues:

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?

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.

o Peter suggested leaving svn_client_log() calling svn_client_log2().

[[[
* subversion/libsvn_client/log.c
  (svn_client_log3): New function based on the previous incarnation of
   svn_client_log2() which accepts a peg revision argument.

  (svn_client_log2): Delegate to svn_client_log3(), passing "end" for
   the "peg_revision" parameter.

  (svn_client_log): Delegate to svn_client_log3() instead of
   svn_client_log2() (in the same fashion as that function).

* subversion/include/svn_client.h
  (svn_client_log3): New declaration based on the previous incarnation
   of svn_client_log2() which accepts a peg revision argument.

  (svn_client_log2): Deprecated, and adjusted doc string.
]]]

Index: subversion/libsvn_client/log.c
===================================================================
--- subversion/libsvn_client/log.c (revision 18103)
+++ subversion/libsvn_client/log.c (working copy)
@@ -45,7 +45,8 @@
 
 
 svn_error_t *
-svn_client_log2 (const apr_array_header_t *targets,
+svn_client_log3 (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,
@@ -58,7 +59,7 @@
 {
   svn_ra_session_t *ra_session;
   const char *path;
- const char *base_url;
+ const char *base_url, *session_url;
   const char *base_name = NULL;
   apr_array_header_t *condensed_targets;
   svn_revnum_t start_revnum, end_revnum;
@@ -159,11 +160,20 @@
 
   /* Open a repository session to the BASE_URL. */
   SVN_ERR (svn_path_condense_targets (&base_name, NULL, targets, TRUE, pool));
- SVN_ERR (svn_client__open_ra_session_internal (&ra_session, base_url,
- base_name, NULL, NULL,
- (NULL != base_name), TRUE,
- ctx, pool));
+ /* ### This function duplicates some, but not all, of what
+ ### svn_client__ra_session_from_path() does. Some of that
+ ### processing should be removed from this function to avoid
+ ### extra round-trips. Compare'em and see what needs to be
+ ### changed.
 
+ ### what are we going to do with session_url?
+
+ ### end_revnum shouldn't be set twice
+ */
+ SVN_ERR (svn_client__ra_session_from_path (&ra_session, &end_revnum,
+ &session_url, base_name,
+ peg_revision, end, ctx, pool));
+
   /* It's a bit complex to correctly handle the special revision words
    * such as "BASE", "COMMITTED", and "PREV". For example, if the
    * user runs
@@ -194,7 +204,7 @@
       SVN_ERR (svn_client__get_revision_number
                (&start_revnum, ra_session, start, base_name, pool));
 
- if (! end_is_local)
+ if (! end_is_local && end_revnum != SVN_INVALID_REVNUM)
       SVN_ERR (svn_client__get_revision_number
                (&end_revnum, ra_session, end, base_name, pool));
 
@@ -270,6 +280,23 @@
 }
 
 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)
+{
+ return svn_client_log3 (targets, end, start, end, limit,
+ discover_changed_paths, strict_node_history,
+ receiver, receiver_baton, ctx, pool);
+}
+
+svn_error_t *
 svn_client_log (const apr_array_header_t *targets,
                 const svn_opt_revision_t *start,
                 const svn_opt_revision_t *end,
@@ -282,7 +309,7 @@
 {
   svn_error_t *err = SVN_NO_ERROR;
 
- err = svn_client_log2 (targets, start, end, 0, discover_changed_paths,
+ err = svn_client_log3 (targets, end, start, end, 0, discover_changed_paths,
                          strict_node_history, receiver, receiver_baton, ctx,
                          pool);
     
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 18103)
+++ subversion/include/svn_client.h (working copy)
@@ -1177,7 +1177,8 @@
  * 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 @a limit is non-zero only invoke @a receiver on the first @a limit
  * logs.
@@ -1202,6 +1203,27 @@
  * 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.4.
+ */
+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,
+ 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);
+
+
+/**
+ * Similar to svn_client_log3(), but with the @a peg_revision
+ * parameter set to @c end.
+ *
+ * @deprecated Provided for backward compatibility with the 1.2 API.
  * @since New in 1.2.
  */
 svn_error_t *

...
> I reviewed and S.Ramaswamy posted another patch with some good stuff
> in it. Did you look at it? (I haven't looked at your latest patch,
> so I don't know.)

No, I didn't see it. Way back on Jun 29 -- okay, I'll take a look, thanks.

-- 
Daniel Rall