Re: [PATCH] #2850 Retrieve arbitrary revprops from log (long)

Eric privately sent me a new version of his patch; here's my review:

> Index: subversion/libsvn_ra/wrapper_template.h
> ===================================================================
> --- subversion/libsvn_ra/wrapper_template.h (revision 26570)
> +++ subversion/libsvn_ra/wrapper_template.h (working copy)
> @@ -352,7 +352,7 @@
> void *receiver_baton,
> apr_pool_t *pool)
> {
> - svn_log_message_receiver2_t receiver2;
> + svn_log_entry_receiver_t receiver2;
> void *receiver2_baton;
>
> svn_compat_wrap_log_receiver(&receiver2, &receiver2_baton,
> @@ -362,7 +362,7 @@
> return VTBL.get_log(session_baton, paths, start, end, 0, /* limit */
> discover_changed_paths, strict_node_history,
> FALSE, /* include_merged_revisions */
> - FALSE, /* omit_log_text */
> + svn_compat_log_revprops_in(pool), /* revprops */
> receiver2, receiver2_baton, pool);
> }
>
> Index: subversion/libsvn_ra/ra_loader.c
> ===================================================================
> --- subversion/libsvn_ra/ra_loader.c (revision 26570)
> +++ subversion/libsvn_ra/ra_loader.c (working copy)
> @@ -844,14 +844,14 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool)
> {
> return session->vtable->get_log(session, paths, start, end, limit,
> discover_changed_paths, strict_node_history,
> - include_merged_revisions, omit_log_text,
> + include_merged_revisions, revprops,
> receiver, receiver_baton, pool);
> }
>
> @@ -866,7 +866,7 @@
> void *receiver_baton,
> apr_pool_t *pool)
> {
> - svn_log_message_receiver2_t receiver2;
> + svn_log_entry_receiver_t receiver2;
> void *receiver2_baton;
>
> svn_compat_wrap_log_receiver(&receiver2, &receiver2_baton,
> @@ -875,7 +875,8 @@
>
> return svn_ra_get_log2(session, paths, start, end, limit,
> discover_changed_paths, strict_node_history,
> - FALSE, FALSE, receiver2, receiver2_baton, pool);
> + FALSE, svn_compat_log_revprops_in(pool),
> + receiver2, receiver2_baton, pool);
> }
>
> svn_error_t *svn_ra_check_path(svn_ra_session_t *session,
> Index: subversion/libsvn_ra/ra_loader.h
> ===================================================================
> --- subversion/libsvn_ra/ra_loader.h (revision 26570)
> +++ subversion/libsvn_ra/ra_loader.h (working copy)
> @@ -159,8 +159,8 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool);
> svn_error_t *(*check_path)(svn_ra_session_t *session,
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 26570)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -1137,8 +1137,8 @@
> * If @a strict_node_history is set, copy history (if any exists) will
> * not be traversed while harvesting revision logs for each path.
> *
> - * If @a omit_log_text is set, the text of the log message will not be
> - * returned.
> + * If @a revprops is NULL, retrieve all revprops; else, retrieve only the
> + * revprops named in the array (i.e. retrieve none if the array is empty).
> *
> * If any invocation of @a receiver returns error, return that error
> * immediately and without wrapping it.
> @@ -1154,7 +1154,7 @@
> * readable and unreadable changed-paths, then silently omit the
> * unreadable changed-paths when pushing the revision.
> *
> - * See also the documentation for @c svn_log_message_receiver2_t.
> + * See also the documentation for @c svn_log_entry_receiver_t.
> *
> * Use @a pool for temporary allocations.
> *
> @@ -1169,17 +1169,18 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> + apr_array_header_t *revprops,
> svn_repos_authz_func_t authz_read_func,
> void *authz_read_baton,
> - svn_log_message_receiver2_t receiver,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool);
>
> /**
> - * Same as svn_repos_get_logs4(), but with @a receiver being a
> - * @c svn_log_message_receiver_t instead of @c svn_log_message_receiver2_t.
> - * Also, @a omit_log_text is set to @c FALSE.
> + * Same as svn_repos_get_logs4(), but with @a receiver being a @c
> + * svn_log_message_receiver_t instead of @c svn_log_entry_receiver_t.

Inconsistent grammar (*a* svn_log_message_receiver_t vs
svn_log_entry_receiver_t) carried over from original.

> + * Also, @a include_merged_revisions is set to @c FALSE and @a revprops is
> + * svn:author, svn:date, and svn:log.
> *
> * @since New in 1.2.
> * @deprecated Provided for backward compatibility with the 1.4 API.
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 26570)
> +++ subversion/include/svn_types.h (working copy)
> @@ -586,15 +586,9 @@
> /** The revision of the commit. */
> svn_revnum_t revision;
>
> - /** The author of the commit. */
> - const char *author;
> + /** The hash of revision properties, or NULL if none. */

Perhaps insert "requested" before "revision"?

> + apr_hash_t *revprops;
>
> - /** The date of the commit. */
> - const char *date;
> -
> - /** The log message of the commit. */
> - const char *message;
> -
> /**
> * Whether or not this message has children.
> *
> @@ -660,13 +654,13 @@
> * @since New in 1.5.
> */
>
> -typedef svn_error_t *(*svn_log_message_receiver2_t)
> +typedef svn_error_t *(*svn_log_entry_receiver_t)
> (void *baton,
> svn_log_entry_t *log_entry,
> apr_pool_t *pool);
>
> /**
> - * Similar to svn_log_message_receiver2_t, except this uses separate
> + * Similar to svn_log_entry_receiver_t, except this uses separate
> * parameters for each part of the log entry.
> *
> * @deprecated Provided for backward compatibility with the 1.4 API.
> @@ -706,6 +700,8 @@
> void *baton);
>
> 
> +/* XXX Hmm, new svn_compat.h header for these? */
> +

Yeah, that might be a good idea.

> /** Return, in @a *callback2 and @a *callback2_baton a function/baton that
> * will call @a callback/@a callback_baton, allocating the @a *callback2_baton
> * in @a pool.
> @@ -721,6 +717,34 @@
> void *callback_baton,
> apr_pool_t *pool);
>
> +/** Clear svn:author, svn:date, and svn:log from @a revprops if not NULL.
> + * Use this if you must handle these three properties separately for
> + * compatibility reasons.
> + *
> + * @since New in 1.5.
> + */
> +void
> +svn_compat_log_revprops_clear(apr_hash_t **revprops);
> +
> +/** Return a list to pass to post-1.5 log-retrieval functions in order to
> + * retrieve the pre-1.5 set of revprops: svn:author, svn:date, and svn:log.
> + *
> + * @since New in 1.5.
> + */
> +apr_array_header_t *
> +svn_compat_log_revprops_in(apr_pool_t *pool);
> +
> +/** Return, in @a **author, @a **date, and @a **message, the values of the
> + * svn:author, svn:date, and svn:log revprops from @a revprops. If @a
> + * revprops is NULL, all return values are NULL. Any return value may be
> + * NULL if the corresponding property is not set in @a revprops.
> + *
> + * @since New in 1.5.
> + */
> +void
> +svn_compat_log_revprops_out(const char **author, const char **date,
> + const char **message, apr_hash_t *revprops);
> +
> /** Return, in @a *receiver2 and @a *receiver2_baton a function/baton that
> * will call @a receiver/@a receiver_baton, allocating the @a *receiver2_baton
> * in @a pool.
> @@ -730,7 +754,7 @@
> *
> * @since New in 1.5.
> */
> -void svn_compat_wrap_log_receiver(svn_log_message_receiver2_t *receiver2,
> +void svn_compat_wrap_log_receiver(svn_log_entry_receiver_t *receiver2,
> void **receiver2_baton,
> svn_log_message_receiver_t receiver,
> void *receiver_baton,
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 26570)
> +++ subversion/include/svn_client.h (working copy)
> @@ -1768,8 +1768,8 @@
> * If @a include_merged_revisions is set, log information for revisions
> * which have been merged to @a targets will also be returned.
> *
> - * If @a omit_log_text is set, the contents of the log message will not
> - * be returned.
> + * If @a revprops is NULL, retrieve all revprops; else, retrieve only the
> + * revprops named in the array (i.e. retrieve none if the array is empty).
> *
> * If @a start->kind or @a end->kind is @c svn_opt_revision_unspecified,
> * return the error @c SVN_ERR_CLIENT_BAD_REVISION.
> @@ -1798,16 +1798,17 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + 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_log4(), but using @c svn_log_message_receiver_t
> - * instead of @c svn_log_message_receiver2_t. Also, @a
> - * include_merged_revisions and @a omit_log_text are set to @c FALSE
> + * instead of @c svn_log_entry_receiver_t. Also, @a
> + * include_merged_revisions is set to @c FALSE and @a revprops is

Perhaps "contains" instead of "is" at the end here?

> + * svn:author, svn:date, and svn:log.
> *
> * @deprecated Provided for compatibility with the 1.4 API.
> * @since New in 1.4.
> Index: subversion/include/svn_ra.h
> ===================================================================
> --- subversion/include/svn_ra.h (revision 26570)
> +++ subversion/include/svn_ra.h (working copy)
> @@ -1158,8 +1158,8 @@
> * If @a include_merged_revisions is set, log information for revisions
> * which have been merged to @a targets will also be returned.
> *
> - * If @a omit_log_text is set, the contents of the log message will not
> - * be returned.
> + * If @a revprops is NULL, retrieve all revprops; else, retrieve only the
> + * revprops named in the array (i.e. retrieve none if the array is empty).
> *
> * If any invocation of @a receiver returns error, return that error
> * immediately and without wrapping it.
> @@ -1174,6 +1174,10 @@
> *
> * Use @a pool for memory allocation.
> *
> + * @note Pre-1.5 servers do not support custom revprop retrieval; if @a
> + * revprops lists any but svn:author, svn:date, and svn:log, an @c
> + * SVN_ERR_RA_NOT_IMPLEMENTED error is returned.

How about "if @a revprops is NULL or contains a revprop other than svn:author,
svn:date, or svn:log, an ..."? (This makes it clear that (a) NULL is
bad and (b) listing a subset of the three standard ones is OK.)

> + *
> * @since New in 1.5.
> */
>
> @@ -1185,15 +1189,16 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool);
>
> /**
> * Similar to svn_ra_get_log2(), but uses @c svn_log_message_receiver_t
> - * instead of @c svn_log_message_recevier2_t. Also @a omit_log_text is
> - * always set to @c FALSE.
> + * instead of @c svn_log_entry_receiver_t. Also, @a
> + * include_merged_revisions is set to @c FALSE and @a revprops is

As above, s/is/contains/?

> + * svn:author, svn:date, and svn:log.
> *
> * @since New in 1.2.
> * @deprecated Provided for backward compatibility with the 1.4 API.
> Index: subversion/libsvn_subr/compat.c
> ===================================================================
> --- subversion/libsvn_subr/compat.c (revision 26570)
> +++ subversion/libsvn_subr/compat.c (working copy)
> @@ -63,34 +63,88 @@
> }
>
>
> +void
> +svn_compat_log_revprops_clear(apr_hash_t **revprops)
> +{
> + if (*revprops)
> + {
> + apr_hash_set(*revprops, SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING, NULL);
> + apr_hash_set(*revprops, SVN_PROP_REVISION_DATE,
> + APR_HASH_KEY_STRING, NULL);
> + apr_hash_set(*revprops, SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING, NULL);
> + /* XXX bother with this? */
> +/* if (apr_hash_count(*revprops) == 0) */
> +/* *revprops = NULL; */

I think leaving this out is good, since the semantics of NULL vs empty
are rather different in most places (though admittedly not in the
couple of places where this function is used).

> + }
> +}
> +
> +apr_array_header_t *
> +svn_compat_log_revprops_in(apr_pool_t *pool)
> +{
> + apr_array_header_t *revprops = apr_array_make(pool, 3, sizeof(char *));
> +
> + APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_AUTHOR;
> + APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_DATE;
> + APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_LOG;
> +
> + return revprops;
> +}
> +
> +void
> +svn_compat_log_revprops_out(const char **author, const char **date,
> + const char **message, apr_hash_t *revprops)
> +{
> + svn_string_t *author_s, *date_s, *message_s;
> +
> + *author = *date = *message = NULL;
> + if (revprops)
> + {
> + if ((author_s = apr_hash_get(revprops, SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING)))
> + *author = author_s->data;
> + if ((date_s = apr_hash_get(revprops, SVN_PROP_REVISION_DATE,
> + APR_HASH_KEY_STRING)))
> + *date = date_s->data;
> + if ((message_s = apr_hash_get(revprops, SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING)))
> + *message = message_s->data;
> + }
> +}
> +
> /* Baton for use with svn_compat_wrap_log_receiver */
> struct log_wrapper_baton {
> void *baton;
> svn_log_message_receiver_t receiver;
> };
>
> -/* This implements svn_log_message_receiver2_t. */
> +/* This implements svn_log_entry_receiver_t. */
> static svn_error_t *
> log_wrapper_callback(void *baton,
> svn_log_entry_t *log_entry,
> apr_pool_t *pool)
> {
> struct log_wrapper_baton *lwb = baton;
> + const char *author, *date, *message;
>
> + svn_compat_log_revprops_out(&author, &date, &message, log_entry->revprops);

Move the declaration of the variables into the conditional, and remove
this duplicate call to svn_compat_log_revprops_out.

> if (lwb->receiver && SVN_IS_VALID_REVNUM(log_entry->revision))
> - return lwb->receiver(lwb->baton,
> - log_entry->changed_paths,
> - log_entry->revision,
> - log_entry->author,
> - log_entry->date,
> - log_entry->message,
> - pool);
> + {
> + svn_compat_log_revprops_out(&author, &date, &message,
> + log_entry->revprops);
> + return lwb->receiver(lwb->baton,
> + log_entry->changed_paths,
> + log_entry->revision,
> + author, date, message,
> + pool);
> + }
>
> return SVN_NO_ERROR;
> }
>
> void
> -svn_compat_wrap_log_receiver(svn_log_message_receiver2_t *receiver2,
> +svn_compat_wrap_log_receiver(svn_log_entry_receiver_t *receiver2,
> void **receiver2_baton,
> svn_log_message_receiver_t receiver,
> void *receiver_baton,
> Index: subversion/libsvn_ra_local/ra_plugin.c
> ===================================================================
> --- subversion/libsvn_ra_local/ra_plugin.c (revision 26570)
> +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> @@ -799,7 +799,7 @@
> struct log_baton
> {
> svn_ra_local__session_baton_t *session;
> - svn_log_message_receiver2_t real_cb;
> + svn_log_entry_receiver_t real_cb;
> void *real_baton;
> };
>
> @@ -826,8 +826,8 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool)
> {
> @@ -871,7 +871,7 @@
> discover_changed_paths,
> strict_node_history,
> include_merged_revisions,
> - omit_log_text,
> + revprops,
> NULL, NULL,
> receiver,
> receiver_baton,
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 26570)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -87,15 +87,15 @@
> svn_client__revision_is_local(const svn_opt_revision_t *revision);
>
>
> -/* Given the CHANGED_PATHS and REVISION from an instance of a
> - svn_log_message_receiver_t function, determine at which location
> - PATH may be expected in the next log message, and set *PREV_PATH_P
> - to that value. KIND is the node kind of PATH. Set *ACTION_P to a
> - character describing the change that caused this revision (as
> - listed in svn_log_changed_path_t) and set *COPYFROM_REV_P to the
> - revision PATH was copied from, or SVN_INVALID_REVNUM if it was not
> - copied. ACTION_P and COPYFROM_REV_P may be NULL, in which case
> - they are not used. Perform all allocations in POOL.
> +/* Given the CHANGED_PATHS and REVISION from an svn_log_entry_t,
> + determine at which location PATH may be expected in the next log
> + message, and set *PREV_PATH_P to that value. KIND is the node kind
> + of PATH. Set *ACTION_P to a character describing the change that
> + caused this revision (as listed in svn_log_changed_path_t) and set
> + *COPYFROM_REV_P to the revision PATH was copied from, or
> + SVN_INVALID_REVNUM if it was not copied. ACTION_P and
> + COPYFROM_REV_P may be NULL, in which case they are not used.
> + Perform all allocations in POOL.
>
> This is useful for tracking the various changes in location a
> particular resource has undergone when performing an RA->get_logs()
> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c (revision 26570)
> +++ subversion/libsvn_client/log.c (working copy)
> @@ -42,7 +42,7 @@
> 
> /*** Getting misc. information ***/
>
> -/* A log callback conforming to the svn_log_message_receiver2_t
> +/* A log callback conforming to the svn_log_entry_receiver_t
> interface for obtaining the last revision of a node at a path and
> storing it in *BATON (an svn_revnum_t). */
> static svn_error_t *
> @@ -64,13 +64,14 @@
> apr_pool_t *pool)
> {
> apr_array_header_t *rel_paths = apr_array_make(pool, 1, sizeof(rel_path));
> + apr_array_header_t *revprops = apr_array_make(pool, 0, sizeof(char *));
> *oldest_rev = SVN_INVALID_REVNUM;
> APR_ARRAY_PUSH(rel_paths, const char *) = rel_path;
>
> /* Trace back in history to find the revision at which this node
> was created (copied or added). */
> return svn_ra_get_log2(ra_session, rel_paths, 1, rev, 1, FALSE, TRUE,
> - FALSE, TRUE, revnum_receiver, oldest_rev, pool);
> + FALSE, revprops, revnum_receiver, oldest_rev, pool);
> }
>
> /* The baton for use with copyfrom_info_receiver(). */
> @@ -271,7 +272,85 @@
> return SVN_NO_ERROR;
> }
>
> +/* compatibility with pre-1.5 servers, which send only author/date/log
> + *revprops in log entries */
> +typedef struct
> +{
> + svn_client_ctx_t *ctx;
> + /* ra session for retrieving revprops from old servers */
> + svn_ra_session_t *ra_session;
> + /* caller's list of requested revprops, receiver, and baton */
> + apr_array_header_t *revprops;
> + svn_log_entry_receiver_t receiver;
> + void *baton;
> +} pre_15_receiver_baton_t;
>
> +static svn_error_t *
> +pre_15_receiver(void *baton, svn_log_entry_t *log_entry, apr_pool_t *pool)
> +{
> + pre_15_receiver_baton_t *rb = baton;
> +
> + if (log_entry->revision == SVN_INVALID_REVNUM)
> + return rb->receiver(rb->baton, log_entry, pool);
> +
> + if (rb->revprops)
> + {
> + int i;
> + svn_boolean_t want_author, want_date, want_log;
> + want_author = want_date = want_log = FALSE;
> + for (i = 0; i < rb->revprops->nelts; i++)
> + {
> + const char *name = APR_ARRAY_IDX(rb->revprops, i, const char *);
> + svn_string_t *value;
> +
> + /* If a standard revprop is requested, we know it is already in
> + * log_entry->revprops if available. */
> + if (strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0)
> + {
> + want_author = TRUE;
> + continue;
> + }
> + if (strcmp(name, SVN_PROP_REVISION_DATE) == 0)
> + {
> + want_date = TRUE;
> + continue;
> + }
> + if (strcmp(name, SVN_PROP_REVISION_LOG) == 0)
> + {
> + want_log = TRUE;
> + continue;
> + }
> + SVN_ERR(svn_ra_rev_prop(rb->ra_session, log_entry->revision,
> + name, &value, pool));
> + if (log_entry->revprops == NULL)
> + log_entry->revprops = apr_hash_make(pool);
> + apr_hash_set(log_entry->revprops, (const void *)name,
> + APR_HASH_KEY_STRING, (const void *)value);
> + }
> + if (log_entry->revprops)
> + {
> + /* Pre-1.5 servers send the standard revprops unconditionally;
> + * clear those the caller doesn't want. */
> + if (!want_author)
> + apr_hash_set(log_entry->revprops, SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING, NULL);
> + if (!want_date)
> + apr_hash_set(log_entry->revprops, SVN_PROP_REVISION_DATE,
> + APR_HASH_KEY_STRING, NULL);
> + if (!want_log)
> + apr_hash_set(log_entry->revprops, SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING, NULL);
> + }
> + }
> + else
> + {
> + SVN_ERR(svn_ra_rev_proplist(rb->ra_session, log_entry->revision,
> + &log_entry->revprops, pool));
> + }
> +
> + return rb->receiver(rb->baton, log_entry, pool);
> +}
> +

It might be better to make the fallback implementation use
svn_ra_rev_proplist for any request which requires non-standard
revprops instead of potentially making multiple svn_ra_rev_prop
requests. The tradeoff is minimizing the number of round-trip
requests vs. minimizing the amount of unrequested data sent; I'm not
sure what the best alternative in practice would be.

> 
> /*** Public Interface. ***/
>
> @@ -285,19 +364,20 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> - void *receiver_baton,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t real_receiver,
> + void *real_receiver_baton,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> svn_ra_session_t *ra_session;
> const char *url_or_path;
> - const char *ignored_url;
> + const char *actual_url;
> const char *base_name = NULL;
> apr_array_header_t *condensed_targets;
> svn_revnum_t ignored_revnum;
> svn_opt_revision_t session_opt_rev;
> + const char *ra_target;
>
> if ((start->kind == svn_opt_revision_unspecified)
> || (end->kind == svn_opt_revision_unspecified))
> @@ -403,8 +483,8 @@
> else
> session_opt_rev.kind = svn_opt_revision_unspecified;
>
> +
> {
> - const char *target;
>
> /* If this is a revision type that requires access to the working copy,
> * we use our initial target path to figure out where to root the RA
> @@ -413,12 +493,12 @@
> || peg_revision->kind == svn_opt_revision_committed
> || peg_revision->kind == svn_opt_revision_previous
> || peg_revision->kind == svn_opt_revision_working)
> - SVN_ERR(svn_path_condense_targets(&target, NULL, targets, TRUE, pool));
> + SVN_ERR(svn_path_condense_targets(&ra_target, NULL, targets, TRUE, pool));
> else
> - target = url_or_path;
> + ra_target = url_or_path;
>
> SVN_ERR(svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> - &ignored_url, target,
> + &actual_url, ra_target,
> peg_revision, &session_opt_rev,
> ctx, pool));
> }
> @@ -451,6 +531,7 @@
>
> svn_boolean_t start_is_local = svn_client__revision_is_local(start);
> svn_boolean_t end_is_local = svn_client__revision_is_local(end);
> + svn_boolean_t pre_15_server = FALSE;
>
> if (! start_is_local)
> SVN_ERR(svn_client__get_revision_number
> @@ -499,6 +580,95 @@
> SVN_ERR(svn_client__get_revision_number
> (&end_revnum, ra_session, end, target, pool));
>
> + if (!pre_15_server
> + && ((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))
> + && err->apr_err == SVN_ERR_RA_NOT_IMPLEMENTED))
> + {
> + /* Caller requested custom revprops from apre-1.5 server;

Missing space between "a" and "pre".

> + * start over, using pre_15_receiver to retrieve them
> + * on a second ra session. */
> + 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;
> +
> + /* The first time through this loop, we have to dump our
> + * existing session, whose server is probably still sending an
> + * unknown number of log entries. If we had svn_ra_close,
> + * we'd want to call that here. */
> + if (!pre_15_server)

This conditional is always true (since we're in a "if (!pre_15_server
&& ...)" block). Actually, it looks like the logic for this section
is broken.

(I pause to note that wow, the behavior of this codepath (for
something like "svn log -rCOMMITTED:42 file1 file2") is actually
really bizarre.)

It looks like in your version, when doing compatability mode,
essentially nothing gets returned after the first iteration through
the loop; I think you're missing a block that says "if pre_15_server
call svn_ra_get_log2 with pre_15_receiver".

Also, I think that the pre_15_server boolean declaration can be moved
into the "if (start_is_local || end_is_local)" block.

> + SVN_ERR(svn_client__ra_session_from_path(&ra_session,
> + &ignored_revnum,
> + &actual_url,
> + ra_target,
> + peg_revision,
> + &session_opt_rev,
> + ctx, pool));
> + pre_15_server = TRUE;
> +
> + svn_error_clear(err);
> + 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);
> + }
> + if (err)
> + break;
> + }
> + }
> + else /* both revisions are static, so no loop needed */
> + {
> + if ((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))
> + && err->apr_err == SVN_ERR_RA_NOT_IMPLEMENTED)
> + {
> + /* 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,
> + peg_revision,
> + &session_opt_rev,
> + ctx, pool));
> + svn_error_clear(err);
> err = svn_ra_get_log2(ra_session,
> condensed_targets,
> start_revnum,
> @@ -507,29 +677,12 @@
> discover_changed_paths,
> strict_node_history,
> include_merged_revisions,
> - omit_log_text,
> - receiver,
> - receiver_baton,
> + svn_compat_log_revprops_in(pool),
> + pre_15_receiver,
> + &rb,
> pool);
> - if (err)
> - break;
> }
> }
> - else /* both revisions are static, so no loop needed */
> - {
> - err = svn_ra_get_log2(ra_session,
> - condensed_targets,
> - start_revnum,
> - end_revnum,
> - limit,
> - discover_changed_paths,
> - strict_node_history,
> - include_merged_revisions,
> - omit_log_text,
> - receiver,
> - receiver_baton,
> - pool);
> - }
>
> return err;
> }
> @@ -548,7 +701,7 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> - svn_log_message_receiver2_t receiver2;
> + svn_log_entry_receiver_t receiver2;
> void *receiver2_baton;
>
> svn_compat_wrap_log_receiver(&receiver2, &receiver2_baton,
> @@ -557,7 +710,8 @@
>
> return svn_client_log4(targets, peg_revision, start, end, limit,
> discover_changed_paths, strict_node_history, FALSE,
> - FALSE, receiver2, receiver2_baton, ctx, pool);
> + svn_compat_log_revprops_in(pool),
> + receiver2, receiver2_baton, ctx, pool);
> }
>
> svn_error_t *
> Index: subversion/bindings/swig/python/tests/ra.py
> ===================================================================
> --- subversion/bindings/swig/python/tests/ra.py (revision 26570)
> +++ subversion/bindings/swig/python/tests/ra.py (working copy)
> @@ -23,10 +23,9 @@
> self.repos = repos.open(REPOS_PATH)
> self.fs = repos.fs(self.repos)
>
> - callbacks = ra.Callbacks()
> + self.callbacks = ra.Callbacks()
> + self.ra_ctx = ra.open2(REPOS_URL, self.callbacks, None, None)
>
> - self.ra_ctx = ra.open2(REPOS_URL, callbacks, None, None)
> -
> def test_get_file(self):
> # Test getting the properties of a file
> fs_revnum = fs.youngest_rev(self.fs)
> @@ -264,6 +263,61 @@
> self.assertRaises(core.SubversionException,
> lambda: ra.lock(self.ra_ctx, {"/": 0}, "sleutel", False, callback))
>
> + def test_get_log2(self):
> + # Get an interesting commmit.
> + self.test_commit3()
> + rev = fs.youngest_rev(self.fs)
> + revprops = ra.rev_proplist(self.ra_ctx, rev)
> + self.assert_("svn:log" in revprops)
> + self.assert_("testprop" in revprops)
> +
> + def receiver(log_entry, pool):
> + called[0] = True
> + self.assertEqual(log_entry.revision, rev)
> + if discover_changed_paths:
> + self.assertEqual(log_entry.changed_paths.keys(), ['/bla3'])
> + changed_path = log_entry.changed_paths['/bla3']
> + self.assert_(changed_path.action in ['A', 'D', 'R', 'M'])
> + self.assertEqual(changed_path.copyfrom_path, None)
> + self.assertEqual(changed_path.copyfrom_rev, -1)
> + else:
> + self.assertEqual(log_entry.changed_paths, None)
> + if log_revprops is None:
> + self.assertEqual(log_entry.revprops, revprops)
> + elif len(log_revprops) == 0:
> + self.assertEqual(log_entry.revprops, None)
> + else:
> + revprop_names = log_entry.revprops.keys()
> + revprop_names.sort()
> + log_revprops.sort()
> + self.assertEqual(revprop_names, log_revprops)
> + for i in log_revprops:
> + self.assertEqual(log_entry.revprops[i], revprops[i],
> + msg="%s != %s on %s"
> + % (log_entry.revprops[i], revprops[i],
> + (log_revprops, discover_changed_paths)))
> +
> + for log_revprops in (
> + # Retrieve the standard three.
> + ["svn:author", "svn:date", "svn:log"],
> + # Retrieve just testprop.
> + ["testprop"],
> + # Retrieve all.
> + None,
> + # Retrieve none.
> + [],
> + ):
> + for discover_changed_paths in [True, False]:
> + called = [False]
> + ra.get_log2(self.ra_ctx, [""],
> + rev, rev, # start, end
> + 1, # limit
> + discover_changed_paths,
> + True, # strict_node_history
> + False, # include_merged_revisions
> + log_revprops, receiver)
> + self.assert_(called[0])
> +
> def test_update(self):
> class TestEditor(delta.Editor):
> pass
> Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c

For the record I'm not reviewing any of the swig changes.

> ===================================================================
> --- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (revision 26570)
> +++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (working copy)
> @@ -446,9 +446,12 @@
> void *ctx, PyObject *py_pool)
> {
> apr_hash_index_t *hi;
> - PyObject *dict = PyDict_New();
> + PyObject *dict;
>
> - if (dict == NULL)
> + if (hash == NULL)
> + Py_RETURN_NONE;
> +
> + if ((dict = PyDict_New()) == NULL)
> return NULL;
>
> for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
> @@ -786,6 +789,42 @@
> return NULL;
> }
>
> +PyObject *svn_swig_py_changed_path_hash_to_dict(apr_hash_t *hash)
> +{
> + apr_hash_index_t *hi;
> + PyObject *dict;
> +
> + if (hash == NULL)
> + Py_RETURN_NONE;
> +
> + if ((dict = PyDict_New()) == NULL)
> + return NULL;
> +
> + for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
> + {
> + const void *key;
> + void *val;
> + PyObject *value;
> +
> + apr_hash_this(hi, &key, NULL, &val);
> + value = make_ob_log_changed_path(val);
> + if (value == NULL)
> + {
> + Py_DECREF(dict);
> + return NULL;
> + }
> + if (PyDict_SetItemString(dict, (char *)key, value) == -1)
> + {
> + Py_DECREF(value);
> + Py_DECREF(dict);
> + return NULL;
> + }
> + Py_DECREF(value);
> + }
> +
> + return dict;
> +}
> +
> apr_array_header_t *svn_swig_py_rangelist_to_array(PyObject *list,
> apr_pool_t *pool)
> {
> @@ -1032,12 +1071,62 @@
> return hash;
> }
>
> +apr_hash_t *svn_swig_py_changed_path_hash_from_dict(PyObject *dict,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *hash;
> + PyObject *keys;
> + int i, num_keys;
> +
> + if (dict == Py_None)
> + return NULL;
> +
> + if (!PyDict_Check(dict))
> + {
> + PyErr_SetString(PyExc_TypeError, "not a dictionary");
> + return NULL;
> + }
> +
> + hash = apr_hash_make(pool);
> + keys = PyDict_Keys(dict);
> + num_keys = PyList_Size(keys);
> + for (i = 0; i < num_keys; i++)
> + {
> + PyObject *key = PyList_GetItem(keys, i);
> + PyObject *py_changed_path = PyDict_GetItem(dict, key);
> + const char *path = make_string_from_ob(key, pool);
> + svn_log_changed_path_t *changed_path;
> + if (!path)
> + {
> + PyErr_SetString(PyExc_TypeError,
> + "dictionary keys aren't strings");
> + Py_DECREF(keys);
> + return NULL;
> + }
> + svn_swig_ConvertPtrString(py_changed_path, (void *)&changed_path,
> + "svn_log_changed_path_t *");
> + if (!changed_path)
> + {
> + PyErr_SetString(PyExc_TypeError,
> + "dictionary values aren't svn_log_changed_path_t");
> + Py_DECREF(keys);
> + return NULL;
> + }
> + apr_hash_set(hash, path, APR_HASH_KEY_STRING, changed_path);
> + }
> + Py_DECREF(keys);
> + return hash;
> +}
> +
> const apr_array_header_t *svn_swig_py_strings_to_array(PyObject *source,
> apr_pool_t *pool)
> {
> int targlen;
> apr_array_header_t *temp;
>
> + if (source == Py_None)
> + return NULL;
> +
> if (!PySequence_Check(source))
> {
> PyErr_SetString(PyExc_TypeError, "not a sequence");
> @@ -2323,6 +2412,49 @@
> return err;
> }
>
> +svn_error_t *svn_swig_py_log_entry_receiver(void *baton,
> + svn_log_entry_t *log_entry,
> + apr_pool_t *pool)
> +{
> + PyObject *receiver = baton;
> + PyObject *result, *py_pool;
> + svn_error_t *err = SVN_NO_ERROR;
> + PyObject *py_log_entry;
> +
> + if ((receiver == NULL) || (receiver == Py_None))
> + return SVN_NO_ERROR;
> +
> + svn_swig_py_acquire_py_lock();
> +
> + py_pool = make_ob_pool(pool);
> + if (py_pool == NULL)
> + {
> + err = callback_exception_error();
> + goto finished;
> + }
> +
> + py_log_entry = svn_swig_NewPointerObjString(log_entry, "svn_log_entry_t *",
> + py_pool);
> + if ((result = PyObject_CallFunction(receiver,
> + (char *)"OO", py_log_entry,
> + py_pool)) == NULL)
> + {
> + err = callback_exception_error();
> + }
> + else
> + {
> + if (result != Py_None)
> + err = callback_bad_return_error("Not None");
> + Py_DECREF(result);
> + }
> +
> + Py_DECREF(py_log_entry);
> + Py_DECREF(py_pool);
> +finished:
> + svn_swig_py_release_py_lock();
> + return err;
> +}
> +
> svn_error_t *svn_swig_py_info_receiver_func(void *baton,
> const char *path,
> const svn_info_t *info,
> Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
> ===================================================================
> --- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h (revision 26570)
> +++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h (working copy)
> @@ -177,7 +177,12 @@
> SVN_SWIG_SWIGUTIL_EXPORT
> PyObject *svn_swig_py_array_to_list(const apr_array_header_t *strings);
>
> +/* helper function to convert a hash mapping char * to svn_string_t * to a
> + * Python dict mapping str to str. */
> SVN_SWIG_SWIGUTIL_EXPORT
> +PyObject *svn_swig_py_changed_path_hash_to_dict(apr_hash_t *hash);
> +
> +SVN_SWIG_SWIGUTIL_EXPORT
> apr_array_header_t *svn_swig_py_rangelist_to_array(PyObject *list,
> apr_pool_t *pool);
>
> @@ -227,6 +232,13 @@
> apr_hash_t *svn_swig_py_path_revs_hash_from_dict(PyObject *dict,
> apr_pool_t *pool);
>
> +/* helper function to convert a Python dictionary mapping strings to
> + strings into an apr_hash_t mapping const char *'s to svn_string_t *'s,
> + allocated in POOL. */
> +SVN_SWIG_SWIGUTIL_EXPORT
> +apr_hash_t *svn_swig_py_changed_path_hash_from_dict(PyObject *dict,
> + apr_pool_t *pool);
> +
> /* helper function to convert a Python sequence of strings into an
> 'apr_array_header_t *' of 'const char *' objects. Note that the
> objects must remain alive -- the values are not copied. This is
> @@ -341,6 +353,12 @@
> const char *msg,
> apr_pool_t *pool);
>
> +/* thunked log receiver2 function */
> +SVN_SWIG_SWIGUTIL_EXPORT
> +svn_error_t *svn_swig_py_log_entry_receiver(void *baton,
> + svn_log_entry_t *log_entry,
> + apr_pool_t *pool);
> +
> /* thunked info receiver function */
> SVN_SWIG_SWIGUTIL_EXPORT
> svn_error_t *svn_swig_py_info_receiver_func(void *py_receiver,
> Index: subversion/bindings/swig/include/svn_types.swg
> ===================================================================
> --- subversion/bindings/swig/include/svn_types.swg (revision 26570)
> +++ subversion/bindings/swig/include/svn_types.swg (working copy)
> @@ -716,6 +716,21 @@
> #endif
>
> /* -----------------------------------------------------------------------
> + Callback: svn_log_entry_receiver_t
> + svn_client_log4()
> + svn_ra_get_log2()
> + svn_repos_get_logs4()
> +*/
> +
> +#ifdef SWIGPYTHON
> +%typemap(in) (svn_log_entry_receiver_t receiver,
> + void *receiver_baton) {
> + $1 = svn_swig_py_log_entry_receiver;
> + $2 = (void *)$input;
> +}
> +#endif
> +
> +/* -----------------------------------------------------------------------
> Callback: svn_commit_callback_t
> svn_ra get_commit_editor()
> svn_repos_get_commit_editor()
> Index: subversion/bindings/swig/include/svn_containers.swg
> ===================================================================
> --- subversion/bindings/swig/include/svn_containers.swg (revision 26570)
> +++ subversion/bindings/swig/include/svn_containers.swg (working copy)
> @@ -170,8 +170,34 @@
> }
> }
>
> +%typemap(out) apr_hash_t *PROPHASH
> +{
> + %append_output(svn_swig_py_prophash_to_dict($1));
> +}
> #endif
>
> +#ifdef SWIGPYTHON
> +%typemap(in) apr_hash_t *changed_paths
> + (apr_pool_t *_global_pool = NULL, PyObject *_global_py_pool = NULL)
> +{
> + if (_global_pool == NULL)
> + {
> + if (svn_swig_py_get_parent_pool(args, $descriptor(apr_pool_t *),
> + &_global_py_pool, &_global_pool))
> + SWIG_fail;
> + }
> +
> + $1 = svn_swig_py_changed_path_hash_from_dict($input, _global_pool);
> + if (PyErr_Occurred()) {
> + SWIG_fail;
> + }
> +}
> +%typemap(out) apr_hash_t *changed_paths
> +{
> + %append_output(svn_swig_py_changed_path_hash_to_dict($1));
> +}
> +#endif
> +
> #ifdef SWIGRUBY
> %typemap(in) apr_hash_t *PROPHASH
> {
> @@ -199,7 +225,8 @@
> apr_hash_t *source_props,
> apr_hash_t *hash,
> apr_hash_t *props,
> - apr_hash_t *revprop_table
> + apr_hash_t *revprop_table,
> + apr_hash_t *revprops
> };
> #endif
>
> @@ -398,8 +425,8 @@
> %typemap(in) const apr_array_header_t *STRINGLIST {
> $1 = (apr_array_header_t *) svn_swig_py_strings_to_array($input,
> _global_pool);
> - if ($1 == NULL)
> - SWIG_fail;
> + if (PyErr_Occurred())
> + SWIG_fail;
> }
> #endif
> #ifdef SWIGPERL
> @@ -426,6 +453,7 @@
> const apr_array_header_t *args,
> const apr_array_header_t *diff_options,
> apr_array_header_t *paths,
> + apr_array_header_t *revprops,
> const apr_array_header_t *targets,
> apr_array_header_t *preserved_exts
> };
> Index: subversion/libsvn_ra_serf/log.c
> ===================================================================
> --- subversion/libsvn_ra_serf/log.c (revision 26570)
> +++ subversion/libsvn_ra_serf/log.c (working copy)
> @@ -51,6 +51,8 @@
> CREATOR,
> DATE,
> COMMENT,
> + REVPROP_NAME,
> + REVPROP_VALUE,
> HAS_CHILDREN,
> ADDED_PATH,
> REPLACED_PATH,
> @@ -70,6 +72,9 @@
>
> /* Log information */
> svn_log_entry_t *log_entry;
> +
> + /* Place to hold revprop name until we get the revprop-value element. */
> + char *revprop_name;
> } log_info_t;
>
> typedef struct {
> @@ -85,8 +90,15 @@
> int status_code;
>
> /* log receiver function and baton */
> - svn_log_message_receiver2_t receiver;
> + svn_log_entry_receiver_t receiver;
> void *receiver_baton;
> +
> + /* pre-1.5 compatibility */
> + svn_boolean_t seen_revprop_element;
> + svn_boolean_t want_author;
> + svn_boolean_t want_custom_revprops;

Reverse the order of the above two fields, since want_author,
want_date, and want_message are three of a kind.

> + svn_boolean_t want_date;
> + svn_boolean_t want_message;
> } log_context_t;
>
> 
> @@ -124,6 +136,17 @@
> info->tmp_path->copyfrom_rev = SVN_INVALID_REVNUM;
> }
>
> + if (state == CREATOR || state == DATE || state == COMMENT
> + || state == REVPROP_NAME || state == REVPROP_VALUE)
> + {
> + log_info_t *info = parser->state->private;
> +
> + if (!info->log_entry->revprops)
> + {
> + info->log_entry->revprops = apr_hash_make(info->pool);
> + }
> + }
> +
> return parser->state->private;
> }
>
> @@ -174,6 +197,19 @@
> {
> push_state(parser, log_ctx, COMMENT);
> }
> + else if (strcmp(name.name, "no-custom-revprops") == 0)
> + {
> + log_ctx->seen_revprop_element = TRUE;
> + }
> + else if (strcmp(name.name, "revprop-name") == 0)
> + {
> + log_ctx->seen_revprop_element = TRUE;
> + push_state(parser, log_ctx, REVPROP_NAME);
> + }
> + else if (strcmp(name.name, "revprop-value") == 0)
> + {
> + push_state(parser, log_ctx, REVPROP_VALUE);
> + }
> else if (strcmp(name.name, "has-children") == 0)
> {
> push_state(parser, log_ctx, HAS_CHILDREN);
> @@ -263,6 +299,12 @@
> else if (state == ITEM &&
> strcmp(name.name, "log-item") == 0)
> {
> + if (log_ctx->want_custom_revprops && !log_ctx->seen_revprop_element)
> + /* Caller asked for custom revprops, but server is too old. */
> + return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, NULL,
> + _("Server does not support custom revprops"
> + " via log"));
> +
> /* Give the info to the reporter */
> SVN_ERR(log_ctx->receiver(log_ctx->receiver_baton,
> info->log_entry,
> @@ -280,27 +322,60 @@
> else if (state == CREATOR &&
> strcmp(name.name, "creator-displayname") == 0)
> {
> - info->log_entry->author = apr_pstrmemdup(info->pool, info->tmp,
> - info->tmp_len);
> - info->tmp_len = 0;
> + if (log_ctx->want_author)
> + {
> + apr_hash_set(info->log_entry->revprops, SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING,
> + svn_string_ncreate(info->tmp, info->tmp_len,
> + info->pool));
> + info->tmp_len = 0;
> + }
> svn_ra_serf__xml_pop_state(parser);
> }
> else if (state == DATE &&
> strcmp(name.name, "date") == 0)
> {
> - info->log_entry->date = apr_pstrmemdup(info->pool, info->tmp,
> - info->tmp_len);
> - info->tmp_len = 0;
> + if (log_ctx->want_date)
> + {
> + apr_hash_set(info->log_entry->revprops, SVN_PROP_REVISION_DATE,
> + APR_HASH_KEY_STRING,
> + svn_string_ncreate(info->tmp, info->tmp_len,
> + info->pool));
> + info->tmp_len = 0;
> + }
> svn_ra_serf__xml_pop_state(parser);
> }
> else if (state == COMMENT &&
> strcmp(name.name, "comment") == 0)
> {
> - info->log_entry->message = apr_pstrmemdup(info->pool, info->tmp,
> - info->tmp_len);
> + if (log_ctx->want_message)
> + {
> + apr_hash_set(info->log_entry->revprops, SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING,
> + svn_string_ncreate(info->tmp, info->tmp_len,
> + info->pool));
> + info->tmp_len = 0;
> + }
> + svn_ra_serf__xml_pop_state(parser);
> + }
> + else if (state == REVPROP_NAME &&
> + strcmp(name.name, "revprop-name") == 0)
> + {
> + info->revprop_name = apr_pstrmemdup(info->pool, info->tmp,
> + info->tmp_len);
> info->tmp_len = 0;
> svn_ra_serf__xml_pop_state(parser);
> }
> + else if (state == REVPROP_VALUE &&
> + strcmp(name.name, "revprop-value") == 0)
> + {

Possibly sanity-check here that info->revprop_name is non-NULL?

(Also, I noticed that in the output of "svn log --xml" revprop names
are attributes on an element, whereas here they are elements
alternating with the values; any reason not to make them attributes in
the DAV request too?)

> + apr_hash_set(info->log_entry->revprops, info->revprop_name,
> + APR_HASH_KEY_STRING,
> + svn_string_ncreate(info->tmp, info->tmp_len, info->pool));
> + info->tmp_len = 0;
> + info->revprop_name = NULL;
> + svn_ra_serf__xml_pop_state(parser);
> + }
> else if (state == HAS_CHILDREN &&
> strcmp(name.name, "has-children") == 0)
> {
> @@ -350,6 +425,8 @@
> case CREATOR:
> case DATE:
> case COMMENT:
> + case REVPROP_NAME:
> + case REVPROP_VALUE:
> case ADDED_PATH:
> case REPLACED_PATH:
> case DELETED_PATH:
> @@ -373,8 +450,8 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool)
> {
> @@ -448,11 +525,32 @@
> session->bkt_alloc);
> }
>
> - if (omit_log_text)
> + if (revprops)
> {
> + int i;
> + for (i = 0; i < revprops->nelts; i++)
> + {
> + char *name = APR_ARRAY_IDX(revprops, i, char *);
> + svn_ra_serf__add_tag_buckets(buckets,
> + "S:revprop", name,
> + session->bkt_alloc);
> + if (strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0)
> + log_ctx->want_author = TRUE;
> + else if (strcmp(name, SVN_PROP_REVISION_DATE) == 0)
> + log_ctx->want_date = TRUE;
> + else if (strcmp(name, SVN_PROP_REVISION_LOG) == 0)
> + log_ctx->want_message = TRUE;
> + else
> + log_ctx->want_custom_revprops = TRUE;
> + }
> + }
> + else
> + {
> svn_ra_serf__add_tag_buckets(buckets,
> - "S:omit-log-text", NULL,
> + "S:all-revprops", NULL,
> session->bkt_alloc);
> + log_ctx->want_author = log_ctx->want_date = log_ctx->want_message = TRUE;
> + log_ctx->want_custom_revprops = TRUE;
> }
>
> if (paths)
> Index: subversion/libsvn_ra_serf/ra_serf.h
> ===================================================================
> --- subversion/libsvn_ra_serf/ra_serf.h (revision 26570)
> +++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
> @@ -946,8 +946,8 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool);
>
> Index: subversion/libsvn_ra_neon/log.c
> ===================================================================
> --- subversion/libsvn_ra_neon/log.c (revision 26570)
> +++ subversion/libsvn_ra_neon/log.c (working copy)
> @@ -56,13 +56,21 @@
>
> /* Information about each log item in turn. */
> svn_log_entry_t *log_entry;
> + /* Place to hold revprop name until we get the revprop-value element. */
> + char *revprop_name;
> + /* pre-1.5 compatibility */
> + svn_boolean_t seen_revprop_element;
> + svn_boolean_t want_author;
> + svn_boolean_t want_custom_revprops;

Reverse order here too?

> + svn_boolean_t want_date;
> + svn_boolean_t want_message;
>
> /* The current changed path item. */
> svn_log_changed_path_t *this_path_item;
>
> /* Client's callback, invoked on the above fields when the end of an
> item is seen. */
> - svn_log_message_receiver2_t receiver;
> + svn_log_entry_receiver_t receiver;
> void *receiver_baton;
>
> int limit;
> @@ -86,9 +94,7 @@
> reset_log_item(struct log_baton *lb)
> {
> lb->log_entry->revision = SVN_INVALID_REVNUM;
> - lb->log_entry->author = NULL;
> - lb->log_entry->date = NULL;
> - lb->log_entry->message = NULL;
> + lb->log_entry->revprops = NULL;
> lb->log_entry->changed_paths = NULL;
> lb->log_entry->has_children = FALSE;
>
> @@ -119,6 +125,12 @@
> SVN_RA_NEON__XML_CDATA },
> { SVN_XML_NAMESPACE, "replaced-path", ELEM_replaced_path,
> SVN_RA_NEON__XML_CDATA },
> + { SVN_XML_NAMESPACE, "no-custom-revprops", ELEM_no_custom_revprops,
> + SVN_RA_NEON__XML_CDATA },
> + { SVN_XML_NAMESPACE, "revprop-name", ELEM_revprop_name,
> + SVN_RA_NEON__XML_CDATA },
> + { SVN_XML_NAMESPACE, "revprop-value", ELEM_revprop_value,
> + SVN_RA_NEON__XML_CDATA },
> { "DAV:", SVN_DAV__VERSION_NAME, ELEM_version_name,
> SVN_RA_NEON__XML_CDATA },
> { "DAV:", "creator-displayname", ELEM_creator_displayname,
> @@ -144,10 +156,15 @@
> case ELEM_replaced_path:
> case ELEM_deleted_path:
> case ELEM_modified_path:
> + case ELEM_revprop_name:
> + case ELEM_revprop_value:
> case ELEM_comment:
> lb->want_cdata = lb->cdata;
> svn_stringbuf_setempty(lb->cdata);
> break;
> + case ELEM_no_custom_revprops:
> + lb->seen_revprop_element = TRUE;
> + break;
> case ELEM_has_children:
> lb->log_entry->has_children = TRUE;
> break;
> @@ -168,7 +185,7 @@
> lb->this_path_item->copyfrom_rev = SVN_INVALID_REVNUM;
>
> /* See documentation for `svn_repos_node_t' in svn_repos.h,
> - and `svn_log_message_receiver_t' in svn_types.h, for more
> + and `svn_log_changed_path_t' in svn_types.h, for more
> about these action codes. */
> if ((elm->id == ELEM_added_path) || (elm->id == ELEM_replaced_path))
> {
> @@ -218,10 +235,24 @@
> lb->log_entry->revision = SVN_STR_TO_REV(lb->cdata->data);
> break;
> case ELEM_creator_displayname:
> - lb->log_entry->author = apr_pstrdup(lb->subpool, lb->cdata->data);
> + if (lb->want_author)
> + {
> + if (! lb->log_entry->revprops)
> + lb->log_entry->revprops = apr_hash_make(lb->subpool);
> + apr_hash_set(lb->log_entry->revprops, SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING,
> + svn_string_create_from_buf(lb->cdata, lb->subpool));
> + }
> break;
> case ELEM_log_date:
> - lb->log_entry->date = apr_pstrdup(lb->subpool, lb->cdata->data);
> + if (lb->want_date)
> + {
> + if (! lb->log_entry->revprops)
> + lb->log_entry->revprops = apr_hash_make(lb->subpool);
> + apr_hash_set(lb->log_entry->revprops, SVN_PROP_REVISION_DATE,
> + APR_HASH_KEY_STRING,
> + svn_string_create_from_buf(lb->cdata, lb->subpool));
> + }
> break;
> case ELEM_added_path:
> case ELEM_replaced_path:
> @@ -235,11 +266,36 @@
> lb->this_path_item);
> break;
> }
> + case ELEM_revprop_name:
> + lb->seen_revprop_element = TRUE;
> + lb->revprop_name = apr_pstrdup(lb->subpool, lb->cdata->data);
> + break;
> + case ELEM_revprop_value:
> + if (! lb->log_entry->revprops)
> + lb->log_entry->revprops = apr_hash_make(lb->subpool);

Check lb->revprop_name != NULL?

> + apr_hash_set(lb->log_entry->revprops, lb->revprop_name,
> + APR_HASH_KEY_STRING,
> + svn_string_create_from_buf(lb->cdata, lb->subpool));
> + lb->revprop_name = NULL;
> + break;
> case ELEM_comment:
> - lb->log_entry->message = apr_pstrdup(lb->subpool, lb->cdata->data);
> + if (lb->want_message)
> + {
> + if (! lb->log_entry->revprops)
> + lb->log_entry->revprops = apr_hash_make(lb->subpool);
> + apr_hash_set(lb->log_entry->revprops, SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING,
> + svn_string_create_from_buf(lb->cdata, lb->subpool));
> + }
> break;
> case ELEM_log_item:
> {
> + if (lb->want_custom_revprops && !lb->seen_revprop_element)
> + /* Caller asked for custom revprops, but server is too old. */
> + return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, NULL,
> + _("Server does not support custom revprops"
> + " via log"));
> +
> /* Compatability cruft so that we can provide limit functionality
> even if the server doesn't support it.
>
> @@ -283,19 +339,19 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool)
> {
> /* The Plan: Send a request to the server for a log report.
> * Somewhere in mod_dav_svn, there will be an implementation, R, of
> - * the `svn_log_message_receiver2_t' function type. Some other
> + * the `svn_log_entry_receiver_t' function type. Some other
> * function in mod_dav_svn will use svn_repos_get_logs() to loop R
> * over the log messages, and the successive invocations of R will
> * collectively transmit the report back here, where we parse the
> * report and invoke RECEIVER (which is an entirely separate
> - * instance of `svn_log_message_receiver2_t') on each individual
> + * instance of `svn_log_entry_receiver_t') on each individual
> * message in that report.
> */
>
> @@ -357,11 +413,33 @@
> "<S:include-merged-revisions/>"));
> }
>
> - if (omit_log_text)
> + if (revprops)
> {
> + lb.want_author = lb.want_date = lb.want_message = FALSE;
> + lb.want_custom_revprops = FALSE;
> + for (i = 0; i < revprops->nelts; i++)
> + {
> + char *name = APR_ARRAY_IDX(revprops, i, char *);
> + svn_stringbuf_appendcstr(request_body, "<S:revprop>");
> + svn_stringbuf_appendcstr(request_body, name);

Reading this makes me wonder if we need to validate the property name
before putting it into XML output...

> + svn_stringbuf_appendcstr(request_body, "</S:revprop>");
> + if (strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0)
> + lb.want_author = TRUE;
> + else if (strcmp(name, SVN_PROP_REVISION_DATE) == 0)
> + lb.want_date = TRUE;
> + else if (strcmp(name, SVN_PROP_REVISION_LOG) == 0)
> + lb.want_message = TRUE;
> + else
> + lb.want_custom_revprops = TRUE;
> + }
> + }
> + else
> + {
> svn_stringbuf_appendcstr(request_body,
> apr_psprintf(pool,
> - "<S:omit-log-text/>"));
> + "<S:all-revprops/>"));
> + lb.want_author = lb.want_date = lb.want_message = TRUE;
> + lb.want_custom_revprops = TRUE;
> }
>
> if (paths)
> @@ -388,6 +466,7 @@
> lb.limit_compat_bailout = FALSE;
> lb.cdata = svn_stringbuf_create("", pool);
> lb.log_entry = svn_log_entry_create(pool);
> + lb.seen_revprop_element = FALSE;
> lb.want_cdata = NULL;
> reset_log_item(&lb);
>
> Index: subversion/libsvn_ra_neon/ra_neon.h
> ===================================================================
> --- subversion/libsvn_ra_neon/ra_neon.h (revision 26570)
> +++ subversion/libsvn_ra_neon/ra_neon.h (working copy)
> @@ -306,8 +306,8 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool);
>
> @@ -698,6 +698,9 @@
> ELEM_checked_in,
> ELEM_collection,
> ELEM_comment,
> + ELEM_no_custom_revprops,
> + ELEM_revprop_name,
> + ELEM_revprop_value,
> ELEM_creationdate,
> ELEM_creator_displayname,
> ELEM_ignored_set,
> Index: subversion/mod_dav_svn/reports/log.c
> ===================================================================
> --- subversion/mod_dav_svn/reports/log.c (revision 26570)
> +++ subversion/mod_dav_svn/reports/log.c (working copy)
> @@ -48,6 +48,9 @@
> /* How deep we are in the log message tree. We only need to surpress the
> SVN_INVALID_REVNUM message if the stack_depth is 0. */
> int stack_depth;
> +
> + /* whether the client requested any custom revprops */
> + svn_boolean_t requested_custom_revprops;
> };
>
>
> @@ -70,7 +73,7 @@
> }
>
>
> -/* This implements `svn_log_message_receiver2_t'.
> +/* This implements `svn_log_entry_receiver_t'.
> BATON is a `struct log_receiver_baton *'. */
> static svn_error_t *
> log_receiver(void *baton,
> @@ -78,6 +81,7 @@
> apr_pool_t *pool)
> {
> struct log_receiver_baton *lrb = baton;
> + svn_boolean_t no_custom_revprops = lrb->requested_custom_revprops;

OK, so this line is entirely correct, but at first glance it looks
like you've written

svn_boolean_t no_foo = lrb->foo;

Maybe throw in a comment that makes it clear that there's no missing !
?

>
> SVN_ERR(maybe_send_header(lrb));
>
> @@ -95,27 +99,56 @@
> "<S:log-item>" DEBUG_CR "<D:version-name>%ld"
> "</D:version-name>" DEBUG_CR, log_entry->revision));
>
> - if (log_entry->author)
> + if (log_entry->revprops)
> + {
> + apr_hash_index_t *hi;
> + for (hi = apr_hash_first(pool, log_entry->revprops);
> + hi != NULL;
> + hi = apr_hash_next(hi))
> + {
> + char *name;
> + svn_string_t *value;
> + apr_hash_this(hi, (void *)&name, NULL, (void *)&value);
> + if (strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0)
> + SVN_ERR(dav_svn__send_xml(lrb->bb, lrb->output,
> + "<D:creator-displayname>%s"
> + "</D:creator-displayname>" DEBUG_CR,
> + apr_xml_quote_string(pool,
> + value->data, 0)));
> + else if (strcmp(name, SVN_PROP_REVISION_DATE) == 0)
> + /* ### this should be DAV:creation-date, but we need to format
> + ### that date a bit differently */
> + SVN_ERR(dav_svn__send_xml(lrb->bb, lrb->output,
> + "<S:date>%s</S:date>" DEBUG_CR,
> + apr_xml_quote_string(pool,
> + value->data, 0)));
> + else if (strcmp(name, SVN_PROP_REVISION_LOG) == 0)
> + SVN_ERR(dav_svn__send_xml(lrb->bb, lrb->output,
> + "<D:comment>%s</D:comment>" DEBUG_CR,
> + apr_xml_quote_string
> + (pool, svn_xml_fuzzy_escape(value->data,
> + pool), 0)));
> + else
> + {
> + no_custom_revprops = FALSE;
> + SVN_ERR(dav_svn__send_xml(lrb->bb, lrb->output,
> + "<S:revprop-name>%s</S:revprop-name>"
> + "<S:revprop-value>%s</S:revprop-value>"
> + DEBUG_CR,
> + apr_xml_quote_string(pool, name, 0),
> + apr_xml_quote_string(pool,
> + value->data, 0)));
> + }
> + }
> + }
> + if (no_custom_revprops)
> + /* If the client requested any custom revprops and we didn't send
> + any, ack the request, so the client can distinguish the absence
> + of custom revprops from an older server that didn't send them.
> + */
> SVN_ERR(dav_svn__send_xml(lrb->bb, lrb->output,
> - "<D:creator-displayname>%s"
> - "</D:creator-displayname>" DEBUG_CR,
> - apr_xml_quote_string(pool, log_entry->author,
> - 0)));
> + "<S:no-custom-revprops/>" DEBUG_CR));
>
> - /* ### this should be DAV:creation-date, but we need to format
> - ### that date a bit differently */
> - if (log_entry->date)
> - SVN_ERR(dav_svn__send_xml(lrb->bb, lrb->output,
> - "<S:date>%s</S:date>" DEBUG_CR,
> - apr_xml_quote_string(pool, log_entry->date, 0)));
> -
> - if (log_entry->message)
> - SVN_ERR(dav_svn__send_xml(lrb->bb, lrb->output,
> - "<D:comment>%s</D:comment>" DEBUG_CR,
> - apr_xml_quote_string
> - (pool, svn_xml_fuzzy_escape(log_entry->message,
> - pool), 0)));
> -
> if (log_entry->has_children)
> {
> SVN_ERR(dav_svn__send_xml(lrb->bb, lrb->output,
> @@ -230,6 +263,7 @@
> const char *target = NULL;
> int limit = 0;
> int ns;
> + svn_boolean_t seen_revprop_element;
>
> /* These get determined from the request document. */
> svn_revnum_t start = SVN_INVALID_REVNUM; /* defaults to HEAD */
> @@ -237,7 +271,8 @@
> svn_boolean_t discover_changed_paths = FALSE; /* off by default */
> svn_boolean_t strict_node_history = FALSE; /* off by default */
> svn_boolean_t include_merged_revisions = FALSE; /* off by default */
> - svn_boolean_t omit_log_text = FALSE;
> + apr_array_header_t *revprops = apr_array_make(resource->pool, 3,
> + sizeof(const char *));
> apr_array_header_t *paths
> = apr_array_make(resource->pool, 1, sizeof(const char *));
> svn_stringbuf_t *comma_separated_paths =
> @@ -255,8 +290,15 @@
> SVN_DAV_ERROR_TAG);
> }
>
> + /* If this is still FALSE after the loop, we haven't seen either of
> + the revprop elements, meaning a pre-1.5 client; we'll return the
> + standard author/date/log revprops. */
> + seen_revprop_element = FALSE;

Why not initialize at the declaration?

> +
> + /* XXX can i remove this comment in a follow-up? */
> /* ### todo: okay, now go fill in svn_ra_dav__get_log() based on the
> syntax implied below... */
> + lrb.requested_custom_revprops = FALSE;
> for (child = doc->root->first_child; child != NULL; child = child->next)
> {
> /* if this element isn't one of ours, then skip it */
> @@ -275,8 +317,28 @@
> strict_node_history = TRUE; /* presence indicates positivity */
> else if (strcmp(child->name, "include-merged-revisions") == 0)
> include_merged_revisions = TRUE; /* presence indicates positivity */
> - else if (strcmp(child->name, "omit-log-text") == 0)
> - omit_log_text = TRUE; /* presence indicates positivity */
> + else if (strcmp(child->name, "all-revprops") == 0)
> + {
> + revprops = NULL; /* presence indicates fetch all revprops */
> + seen_revprop_element = lrb.requested_custom_revprops = TRUE;
> + }
> + else if (strcmp(child->name, "revprop") == 0)
> + {
> + /* XXX should we raise an error if all-revprops and revprop
> + elements are given? */

Sure!

> + if (revprops)
> + {
> + /* We're not fetching all revprops, append to fetch list. */
> + const char *name = dav_xml_get_cdata(child, resource->pool, 0);
> + APR_ARRAY_PUSH(revprops, const char *) = name;
> + if (!lrb.requested_custom_revprops
> + && strcmp(name, SVN_PROP_REVISION_AUTHOR) != 0
> + && strcmp(name, SVN_PROP_REVISION_DATE) != 0
> + && strcmp(name, SVN_PROP_REVISION_LOG) != 0)
> + lrb.requested_custom_revprops = TRUE;
> + }
> + seen_revprop_element = TRUE;
> + }
> else if (strcmp(child->name, "path") == 0)
> {
> const char *rel_path = dav_xml_get_cdata(child, resource->pool, 0);
> @@ -297,6 +359,14 @@
> /* else unknown element; skip it */
> }
>
> + if (!seen_revprop_element)
> + {
> + /* pre-1.5 client */
> + APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_AUTHOR;
> + APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_DATE;
> + APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_LOG;
> + }
> +
> /* Build authz read baton */
> arb.r = resource->info->r;
> arb.repos = resource->info->repos;
> @@ -307,8 +377,9 @@
> lrb.output = output;
> lrb.needs_header = TRUE;
> lrb.stack_depth = 0;
> + /* lrb.requested_custom_revprops set above */
>
> - /* Our svn_log_message_receiver_t sends the <S:log-report> header in
> + /* Our svn_log_entry_receiver_t sends the <S:log-report> header in
> a lazy fashion. Before writing the first log message, it assures
> that the header has already been sent (checking the needs_header
> flag in our log_receiver_baton structure). */
> @@ -322,7 +393,7 @@
> discover_changed_paths,
> strict_node_history,
> include_merged_revisions,
> - omit_log_text,
> + revprops,
> dav_svn__authz_read_func(&arb),
> &arb,
> log_receiver,
> Index: subversion/tests/cmdline/log_tests.py
> ===================================================================
> --- subversion/tests/cmdline/log_tests.py (revision 26570)
> +++ subversion/tests/cmdline/log_tests.py (working copy)
> @@ -18,6 +18,8 @@
>
> # General modules
> import re, os, sys
> +import difflib, pprint
> +import xml.parsers.expat
>
> # Our testing module
> import svntest
> @@ -413,7 +415,95 @@
> missing_revs, chain)
>
>
> +class LogEntry:
> + """Represent a log entry (ignoring changed_paths for now)."""
> + def __init__(self, revision):
> + self.revision = revision
> + self.revprops = {}
>
> + def assert_revprops(self, revprops):
> + """Assert that the dict revprops is the same as this entry's revprops.
> +
> + Raises svntest.Failure if not.
> + """
> + if self.revprops != revprops:
> + raise svntest.Failure('\n' + '\n'.join(difflib.ndiff(
> + pprint.pformat(revprops).splitlines(),
> + pprint.pformat(self.revprops).splitlines())))
> +
> +class LogParser:
> + def parse(self, data):
> + """Return a list of LogEntrys parsed from the sequence of strings data.
> +
> + This is the only method of interest to callers.
> + """
> + for i in data:
> + try:
> + self.parser.Parse(i)
> + except xml.parsers.expat.ExpatError:
> + sys.stderr.write('broken xml: %s\n' % (''.join(data),))
> + raise
> + self.parser.Parse('', True)
> + return self.entries
> +
> + def __init__(self):
> + # for expat
> + self.parser = xml.parsers.expat.ParserCreate()
> + self.parser.StartElementHandler = self.handle_start_element
> + self.parser.EndElementHandler = self.handle_end_element
> + self.parser.CharacterDataHandler = self.handle_character_data
> + # Ignore some things.
> + self.ignore_elements('log', 'paths', 'path', 'revprops')
> + self.ignore_tags('logentry_end', 'author_start', 'date_start', 'msg_start')
> + # internal state
> + self.cdata = []
> + self.property = None
> + # the result
> + self.entries = []
> +
> + def ignore(self, *args, **kwargs):
> + del self.cdata[:]

Maybe it's more efficient or something, but "self.cdata = []" is more
clear to me.

> + def ignore_tags(self, *args):
> + for tag in args:
> + setattr(self, tag, self.ignore)
> + def ignore_elements(self, *args):
> + for element in args:
> + self.ignore_tags(element + '_start', element + '_end')
> +
> + # expat handlers
> + def handle_start_element(self, name, attrs):
> + return getattr(self, name + '_start')(attrs)
> + def handle_end_element(self, name):
> + return getattr(self, name + '_end')()
> + def handle_character_data(self, data):
> + self.cdata.append(data)
> +
> + # element handler utilities
> + def use_cdata(self):
> + result = ''.join(self.cdata).strip()
> + del self.cdata[:]

Ditto.

> + return result
> + def svn_prop(self, name):
> + self.entries[-1].revprops['svn:' + name] = self.use_cdata()
> +
> + # element handlers
> + def logentry_start(self, attrs):
> + self.entries.append(LogEntry(int(attrs['revision'])))
> + def author_end(self):
> + return self.svn_prop('author')

svn_prop doesn't seem to return anything...

> + def msg_end(self):
> + return self.svn_prop('log')

ditto

> + def date_end(self):
> + # svn:date could be anything, so just note its presence.
> + self.cdata = ['']
> + return self.svn_prop('date')

ditto

> + def property_start(self, attrs):
> + self.property = attrs['name']
> + def property_end(self):
> + self.entries[-1].revprops[self.property] = self.use_cdata()
> +
> +
> +
> ######################################################################
> # Tests
> #
> @@ -1027,7 +1117,318 @@
> log_chain = parse_log_output(output)
> check_log_chain(log_chain, [2, 5, 7])
>
> +#----------------------------------------------------------------------
> +def retrieve_revprops(sbox):
> + "test revprop retrieval"
>
> + sbox.build()
> + svntest.actions.enable_revprop_changes(sbox.repo_dir)
> +
> + # svn_log_xml sets the command so we can display a nice, detailed message
> + # for failing tests.
> + command = [None]
> + def svn_log_xml(*args):
> + command[0] = ' '.join(args)
> + (stdout, stderr) = svntest.actions.run_and_verify_svn(
> + None, None, [], # message, expected_stdout, expected_stderr
> + 'log', '--xml', sbox.repo_url, *args)
> + return LogParser().parse(stdout)
> +
> + # test properties
> + author = 'jrandom'
> + msg1 = 'Log message for revision 1.'
> + msg2 = 'Log message for revision 2.'
> + custom_name = 'retrieve_revprops'
> + custom_value = 'foo bar'
> +
> + # Commit a change.
> + wc_dir = sbox.wc_dir
> + cwd = os.getcwd()
> + os.chdir(wc_dir)
> + svntest.main.file_append(os.path.join('A', 'D', 'H', 'omega'), "new otext")
> + os.chdir(cwd)
> + omega_path = os.path.join(wc_dir, 'A', 'D', 'H', 'omega')
> + expected_output = svntest.wc.State(wc_dir, {
> + 'A/D/H/omega' : Item(verb='Sending'),
> + })
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('A/D/H/omega', wc_rev=2, status=' ')
> + svntest.actions.run_and_verify_commit(wc_dir,
> + expected_output,
> + expected_status,
> + None,
> + None, None,
> + None, None,
> + '-m', msg2,
> + omega_path)
> +
> + # Set custom property on r1 and r2.
> + svntest.actions.run_and_verify_svn(
> + None, None, [], # message, expected_stdout, expected_stderr
> + 'ps', '--revprop', '-r1', custom_name, custom_value, sbox.repo_url)
> + svntest.actions.run_and_verify_svn(
> + None, None, [], # message, expected_stdout, expected_stderr
> + 'ps', '--revprop', '-r2', custom_name, custom_value, sbox.repo_url)
> +
> + try:
> + # basic test without revprop options
> + (entry,) = svn_log_xml('-r1')
> + revprops = {'svn:author': author, 'svn:date': '', 'svn:log': msg1}
> + entry.assert_revprops(revprops)
> + # again with -v, to make sure determining paths doesn't interfere
> + (entry,) = svn_log_xml('-v', '-r1')
> + entry.assert_revprops(revprops)
> +
> + # basic test without revprop options, with multiple revisions
> + (entry2, entry1) = svn_log_xml()
> + revprops1 = {'svn:author': author, 'svn:date': '', 'svn:log': msg1}
> + revprops2 = {'svn:author': author, 'svn:date': '', 'svn:log': msg2}
> + entry1.assert_revprops(revprops1)
> + entry2.assert_revprops(revprops2)
> + # again with -v
> + (entry2, entry1) = svn_log_xml('-v')
> + entry1.assert_revprops(revprops1)
> + entry2.assert_revprops(revprops2)
> +
> + # -q with no revprop options must suppress svn:log only.
> + (entry,) = svn_log_xml('-q', '-r1')
> + revprops = {'svn:author': author, 'svn:date': ''}
> + entry.assert_revprops(revprops)
> + # again with -v
> + (entry,) = svn_log_xml('-v', '-q', '-r1')
> + entry.assert_revprops(revprops)
> +
> + # Request svn:date, svn:log, and a non-existent property.
> + (entry,) = svn_log_xml('-r1', '--with-revprop=svn:date',
> + '--with-revprop', 'svn:log',
> + '--with-revprop', 'nosuchprop')
> + revprops = {'svn:date': '', 'svn:log': msg1}
> + entry.assert_revprops(revprops)
> + # again with -v
> + (entry,) = svn_log_xml('-v', '-r1', '--with-revprop=svn:date',
> + '--with-revprop', 'svn:log',
> + '--with-revprop', 'nosuchprop')
> + entry.assert_revprops(revprops)
> +
> + if True: #svntest.main.server_minor_version >= 5:
> + # Server is post-1.5 and will return custom revprops.
> +
> + # Get all revprops.
> + (entry,) = svn_log_xml('-r1', '--with-all-revprops')
> + revprops = {'svn:author': author, 'svn:date': '',
> + 'svn:log': msg1, custom_name: custom_value}
> + entry.assert_revprops(revprops)
> + # again with -v
> + (entry,) = svn_log_xml('-v', '-r1', '--with-all-revprops')
> + entry.assert_revprops(revprops)
> +
> + # Get all revprops, with multiple revisions.
> + (entry2, entry1) = svn_log_xml('--with-all-revprops')
> + revprops1 = {'svn:author': author, 'svn:date': '',
> + 'svn:log': msg1, custom_name: custom_value}
> + revprops2 = {'svn:author': author, 'svn:date': '',
> + 'svn:log': msg2, custom_name: custom_value}
> + entry1.assert_revprops(revprops1)
> + entry2.assert_revprops(revprops2)
> + # again with -v
> + (entry2, entry1) = svn_log_xml('-v', '--with-all-revprops')
> + revprops1 = {'svn:author': author, 'svn:date': '',
> + 'svn:log': msg1, custom_name: custom_value}
> + revprops2 = {'svn:author': author, 'svn:date': '',
> + 'svn:log': msg2, custom_name: custom_value}
> + entry1.assert_revprops(revprops1)
> + entry2.assert_revprops(revprops2)
> +
> + # Get only the custom property.
> + (entry,) = svn_log_xml('-r1', '--with-revprop', custom_name)
> + revprops = {custom_name: custom_value}
> + entry.assert_revprops(revprops)
> + # again with -v
> + (entry,) = svn_log_xml('-v', '-r1', '--with-revprop', custom_name)
> + entry.assert_revprops(revprops)
> +# else:
> +# # Server is pre-1.5; test client compatibility.
> +
> +# # Get all revprops.
> +# (entry,) = svn_log_xml('-r1', '--with-all-revprops')
> +# revprops = {'svn:author': author, 'svn:date': '', 'svn:log': msg1}
> +# entry.assert_revprops(revprops)
> +# # again with -v
> +# (entry,) = svn_log_xml('-v', '-r1', '--with-all-revprops')
> +# entry.assert_revprops(revprops)
> +
> +# # Get all revprops, with multiple revisions.
> +# (entry2, entry1) = svn_log_xml('--with-all-revprops')
> +# revprops1 = {'svn:author': author, 'svn:date': '', 'svn:log': msg1}
> +# revprops2 = {'svn:author': author, 'svn:date': '', 'svn:log': msg2}
> +# entry1.assert_revprops(revprops1)
> +# entry2.assert_revprops(revprops2)
> +# # again with -v
> +# (entry2, entry1) = svn_log_xml('-v', '--with-all-revprops')
> +# revprops1 = {'svn:author': author, 'svn:date': '', 'svn:log': msg1}
> +# revprops2 = {'svn:author': author, 'svn:date': '', 'svn:log': msg2}
> +# entry1.assert_revprops(revprops1)
> +# entry2.assert_revprops(revprops2)
> + except:
> + print 'Error running svn log --xml', command[0]
> + raise
> + return
> +
> +
> +# def retrieve_revprops(sbox):
> +# "test revprop retrieval"
> +
> +# sbox.build()
> +# svntest.actions.enable_revprop_changes(sbox.repo_dir)
> +
> +# def run_and_verify_log_xml(message=None, expected_paths=None,
> +# expected_revprops=None, expected_stdout=None,
> +# expected_stderr=None, args=[]):
> +# """Call svntest.actions.run_and_verify_svn with log --xml and args
> +# (optional) as command arguments, and pass along message,
> +# expected_stdout, and expected_stderr.
> +
> +# If message is None, pass the svn log command as message.
> +
> +# expected_paths is not yet implemented.
> +
> +# expected_revprops is an optional list of dicts, compared to each
> +# revision's revprops. The list must be in the same order the log
> +# entries come in.
> +
> +# expected_paths and expected_revprops are ignored if expected_stdout or
> +# expected_stderr is specified.
> +# """
> +# if message == None:
> +# message = ' '.join(args)
> +# if expected_stderr == None:
> +# expected_stderr = []
> +# (stdout, stderr) = svntest.actions.run_and_verify_svn(
> +# message, expected_stdout, expected_stderr,
> +# 'log', '--xml', sbox.repo_url, *args)
> +# if expected_stdout != None or expected_stderr != None:
> +# # Caller specified output, don't parse.
> +# return
> +# for (index, entry) in enumerate(LogParser().parse(stdout)):
> +# entry.assert_revprops(expected_revprops[index])
> +# def run_and_verify_log_xml_v(message=None, expected_paths=None,
> +# expected_revprops=None, expected_stdout=None,
> +# expected_stderr=None, args=[]):
> +# """Call run_and_verify_log_xml, adding in the -v option."""
> +# return run_and_verify_log_xml(message, expected_paths, expected_revprops,
> +# expected_stdout=None,
> +# expected_stderr=expected_stderr,
> +# args=['-v'] + args)
> +
> +# # test properties
> +# author = 'jrandom'
> +# msg1 = 'Log message for revision 1.'
> +# msg2 = 'Log message for revision 2.'
> +# custom_name = 'retrieve_revprops'
> +# custom_value = 'foo bar'
> +
> +# # Commit a change.
> +# wc_dir = sbox.wc_dir
> +# cwd = os.getcwd()
> +# os.chdir(wc_dir)
> +# svntest.main.file_append(os.path.join('A', 'D', 'H', 'omega'), "new otext")
> +# os.chdir(cwd)
> +# omega_path = os.path.join(wc_dir, 'A', 'D', 'H', 'omega')
> +# expected_output = svntest.wc.State(wc_dir, {
> +# 'A/D/H/omega' : Item(verb='Sending'),
> +# })
> +# expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> +# expected_status.tweak('A/D/H/omega', wc_rev=2, status=' ')
> +# svntest.actions.run_and_verify_commit(wc_dir,
> +# expected_output,
> +# expected_status,
> +# None,
> +# None, None,
> +# None, None,
> +# '-m', msg2,
> +# omega_path)
> +
> +# # Set custom property on r1 and r2.
> +# svntest.actions.run_and_verify_svn(
> +# None, None, [], # message, expected_stdout, expected_stderr
> +# 'ps', '--revprop', '-r1', custom_name, custom_value, sbox.repo_url)
> +# svntest.actions.run_and_verify_svn(
> +# None, None, [], # message, expected_stdout, expected_stderr
> +# 'ps', '--revprop', '-r2', custom_name, custom_value, sbox.repo_url)
> +
> +# try:
> +# # basic test without revprop options
> +# args = ['-r1']
> +# revprops = [{'svn:author': author, 'svn:date': '', 'svn:log': msg1}]
> +# run_and_verify_log_xml(args=args, expected_revprops=revprops)
> +# # again with -v, to make sure determining paths doesn't interfere
> +# run_and_verify_log_xml_v(args=args, expected_revprops=revprops)
> +
> +# # basic test without revprop options, with multiple revisions
> +# revprops = [{'svn:author': author, 'svn:date': '', 'svn:log': msg2},
> +# {'svn:author': author, 'svn:date': '', 'svn:log': msg1}]
> +# run_and_verify_log_xml(expected_revprops=revprops)
> +# run_and_verify_log_xml_v(expected_revprops=revprops)
> +
> +# # -q with no revprop options must suppress svn:log only.
> +# args = ['-q', '-r1']
> +# revprops = [{'svn:author': author, 'svn:date': ''}]
> +# run_and_verify_log_xml(expected_revprops=revprops, args=args)
> +# run_and_verify_log_xml_v(expected_revprops=revprops, args=args)
> +
> +# if svntest.main.server_minor_version >= 5:
> +# # Server is post-1.5 and will return custom revprops.
> +
> +# # Get all revprops.
> +# args = ['-r1', '--with-all-revprops']
> +# revprops = [{'svn:author': author, 'svn:date': '',
> +# 'svn:log': msg1, custom_name: custom_value}]
> +# run_and_verify_log_xml(expected_revprops=revprops, args=args)
> +# run_and_verify_log_xml_v(expected_revprops=revprops, args=args)
> +
> +# # Get all revprops, with multiple revisions.
> +# args = ['--with-all-revprops']
> +# revprops = [{'svn:author': author, 'svn:date': '',
> +# 'svn:log': msg2, custom_name: custom_value},
> +# {'svn:author': author, 'svn:date': '',
> +# 'svn:log': msg1, custom_name: custom_value}]
> +# run_and_verify_log_xml(expected_revprops=revprops, args=args)
> +# run_and_verify_log_xml_v(expected_revprops=revprops, args=args)
> +
> +# # Get only the custom property.
> +# args = ['-r1', '--with-revprop', custom_name]
> +# revprops = [{custom_name: custom_value}]
> +# run_and_verify_log_xml(expected_revprops=revprops, args=args)
> +# run_and_verify_log_xml_v(expected_revprops=revprops, args=args)
> +
> +# # Request svn:date, svn:log, and a non-existent property.
> +# args = ['-r1', '--with-revprop=svn:date',
> +# '--with-revprop', 'svn:log',
> +# '--with-revprop', 'nosuchprop']
> +# revprops = [{'svn:date': '', 'svn:log': msg1}]
> +# run_and_verify_log_xml(expected_revprops=revprops, args=args)
> +# run_and_verify_log_xml_v(expected_revprops=revprops, args=args)
> +# else:
> +# # Server is pre-1.5; test client compatibility.
> +# out = ['<?xml version="1.0"?>\n',
> +# '<log>\n']
> +# err = '.*Server does not support custom revprops via log'
> +# # Get all revprops.
> +# run_and_verify_log_xml(expected_stdout=out, expected_stderr=err,
> +# args=['-r1', '--with-all-revprops'])
> +# # Get all revprops, with multiple revisions.
> +# run_and_verify_log_xml(expected_stdout=out, expected_stderr=err,
> +# args=['--with-all-revprops'])
> +# # Request svn:date, svn:log, and a non-existent property.
> +# run_and_verify_log_xml(expected_stdout=out, expected_stderr=err,
> +# args=['-r1', '--with-revprop=svn:date',
> +# '--with-revprop', 'svn:log',
> +# '--with-revprop', 'nosuchprop'])
> +# except:
> +# raise
> +# return
> +
> +

I would refactor this test into three tests (with a common setup
function): one which should work against any server, one for 1.5
servers, and one for older server, and then use conditional Skips to
only run two of them.

> ########################################################################
> # Run the tests
>
> @@ -1056,6 +1457,7 @@
> XFail(log_single_change),
> XFail(log_changes_range),
> XFail(log_changes_list),
> + retrieve_revprops,
> ]
>
> if __name__ == '__main__':
> Index: subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout
> ===================================================================
> --- subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout (revision 26570)
> +++ subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout (working copy)
> @@ -48,6 +48,8 @@
> --config-dir ARG : read user configuration files from directory ARG
> -l [--limit] ARG : maximum number of log entries
> --changelist ARG : operate only on members of changelist ARG
> + --with-all-revprops : retrieve all revision properties
> + --with-revprop ARG : retrieve revision property ARG
>
> switch (sw): Update the working copy to a different URL.
> usage: 1. switch URL [PATH]
> Index: subversion/tests/cmdline/svntest/main.py
> ===================================================================
> --- subversion/tests/cmdline/svntest/main.py (revision 26584)
> +++ subversion/tests/cmdline/svntest/main.py (working copy)
> @@ -941,6 +941,7 @@
> print 'EXCEPTION: %s: %s' % (ex.__class__.__name__, ex_args)
> else:
> print 'EXCEPTION:', ex.__class__.__name__
> + traceback.print_exc(file=sys.stdout)

Please commit this, but separately!

> except KeyboardInterrupt:
> print 'Interrupted'
> sys.exit(0)
> Index: subversion/libsvn_repos/log.c
> ===================================================================
> --- subversion/libsvn_repos/log.c (revision 26570)
> +++ subversion/libsvn_repos/log.c (working copy)
> @@ -38,9 +38,9 @@
> const char *path,
> svn_revnum_t rev,
> svn_boolean_t discover_changed_paths,
> - svn_boolean_t omit_log_text,
> + apr_array_header_t *revprops,
> svn_boolean_t descending_order,
> - svn_log_message_receiver2_t receiver,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> svn_repos_authz_func_t authz_read_func,
> void *authz_read_baton,
> @@ -830,22 +830,14 @@
> svn_revnum_t rev,
> svn_fs_t *fs,
> svn_boolean_t discover_changed_paths,
> - svn_boolean_t omit_log_text,
> + apr_array_header_t *revprops,
> svn_repos_authz_func_t authz_read_func,
> void *authz_read_baton,
> apr_pool_t *pool)
> {
> - svn_string_t *author, *date, *message;
> apr_hash_t *r_props, *changed_paths = NULL;
> + svn_boolean_t get_revprops = TRUE, censor_message = FALSE;

The censor_message-based logic here is wrong. The security policy is
that partially-readable revisions allow only svn:author and svn:date,
not that they disallow only svn:log. See
svn_repos_fs_revision_proplist, for example.

(Random notes while I'm at it: perhaps this function should be using
the new svn_repos_check_revision_access; and also, this security
policy should perhaps be documented with
svn_repos_check_revision_access or svn_repos_revision_access_level_t.)

>
> - SVN_ERR(svn_fs_revision_proplist(&r_props, fs, rev, pool));
> - author = apr_hash_get(r_props, SVN_PROP_REVISION_AUTHOR,
> - APR_HASH_KEY_STRING);
> - date = apr_hash_get(r_props, SVN_PROP_REVISION_DATE,
> - APR_HASH_KEY_STRING);
> - message = apr_hash_get(r_props, SVN_PROP_REVISION_LOG,
> - APR_HASH_KEY_STRING);
> -
> /* Discover changed paths if the user requested them
> or if we need to check that they are readable. */
> if ((rev > 0)
> @@ -866,9 +858,7 @@
> /* All changed-paths are unreadable, so clear all fields. */
> svn_error_clear(patherr);
> changed_paths = NULL;
> - author = NULL;
> - date = NULL;
> - message = NULL;
> + get_revprops = FALSE;
> }
> else if (patherr
> && patherr->apr_err == SVN_ERR_AUTHZ_PARTIALLY_READABLE)
> @@ -877,7 +867,7 @@
> log message. (The unreadable paths are already
> missing from the hash.) */
> svn_error_clear(patherr);
> - message = NULL;
> + censor_message = TRUE;
> }
> else if (patherr)
> return patherr;
> @@ -888,15 +878,42 @@
> changed_paths = NULL;
> }
>
> - /* Intentionally omit the log message if requested. */
> - if (omit_log_text)
> - message = NULL;
> + /* Get revprops if the user is allowed to see them. */
> + if (get_revprops)
> + {
> + SVN_ERR(svn_fs_revision_proplist(&r_props, fs, rev, pool));
> + if (revprops)
> + {
> + int i;
> + for (i = 0; i < revprops->nelts; i++)
> + {
> + char *name = APR_ARRAY_IDX(revprops, i, char *);
> + svn_string_t *value = apr_hash_get(r_props, name,
> + APR_HASH_KEY_STRING);
> + if (censor_message && strcmp(name, SVN_PROP_REVISION_LOG) == 0)
> + continue;
> + {
> + FILE *fp = fopen("/tmp/svnservelog", "a");
> + fprintf(fp, "send %s\n", name);
> + fclose(fp);
> + }

This doesn't have an XXX; make sure to remember to remove it.

> + if (log_entry->revprops == NULL)
> + log_entry->revprops = apr_hash_make(pool);
> + apr_hash_set(log_entry->revprops, name,
> + APR_HASH_KEY_STRING, value);
> + }
> + }
> + else
> + {
> + if (censor_message)
> + apr_hash_set(r_props, SVN_PROP_REVISION_LOG, APR_HASH_KEY_STRING,
> + NULL);
> + log_entry->revprops = r_props;
> + }
> + }
>
> log_entry->changed_paths = changed_paths;
> log_entry->revision = rev;
> - log_entry->author = author ? author->data : NULL;
> - log_entry->date = date ? date->data : NULL;
> - log_entry->message = message ? message->data : NULL;
>
> return SVN_NO_ERROR;
> }
> @@ -938,14 +955,15 @@
> /* Send log tree, beging with REV to RECEIVER with its RECEIVER_BATON.
> *
> * FS is used with REV to fetch the interesting history information,
> - * such as author, date, etc.
> + * such as changed paths, revprops, etc.
> *
> * The detect_changed function is used if either AUTHZ_READ_FUNC is
> * not NULL, or if DISCOVER_CHANGED_PATHS is TRUE. See it for details.
> *
> * If DESCENDING_ORDER is true, send child messages in descending order.
> *
> - * If OMIT_LOG_TEXT is true, don't send the log text to RECEIVER.
> + * If REVPROPS is NULL, retrieve all revprops; else, retrieve only the
> + * revprops named in the array (i.e. retrieve none if the array is empty).
> *
> * If INCLUDE_MERGED_REVISIONS is TRUE, send history information for any
> * revisions which were merged in as a result of REV immediately following
> @@ -958,9 +976,9 @@
> svn_fs_t *fs,
> svn_boolean_t discover_changed_paths,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> + apr_array_header_t *revprops,
> svn_boolean_t descending_order,
> - svn_log_message_receiver2_t receiver,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> svn_repos_authz_func_t authz_read_func,
> void *authz_read_baton,
> @@ -972,7 +990,7 @@
>
> log_entry = svn_log_entry_create(pool);
> SVN_ERR(fill_log_entry(log_entry, rev, fs, discover_changed_paths,
> - omit_log_text, authz_read_func, authz_read_baton,
> + revprops, authz_read_func, authz_read_baton,
> pool));
>
> /* Check to see if we need to include any extra merged revisions. */
> @@ -1027,7 +1045,7 @@
> continue;
>
> SVN_ERR(do_merged_log(fs, merge_source, revision,
> - discover_changed_paths, omit_log_text,
> + discover_changed_paths, revprops,
> descending_order, receiver, receiver_baton,
> authz_read_func, authz_read_baton, pool));
> }
> @@ -1130,9 +1148,9 @@
> const char *path,
> svn_revnum_t rev,
> svn_boolean_t discover_changed_paths,
> - svn_boolean_t omit_log_text,
> + apr_array_header_t *revprops,
> svn_boolean_t descending_order,
> - svn_log_message_receiver2_t receiver,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> svn_repos_authz_func_t authz_read_func,
> void *authz_read_baton,
> @@ -1166,7 +1184,7 @@
> if (changed)
> {
> SVN_ERR(send_logs(paths, rev, fs, discover_changed_paths, TRUE,
> - omit_log_text, descending_order,
> + revprops, descending_order,
> receiver, receiver_baton,
> authz_read_func, authz_read_baton, pool));
> }
> @@ -1185,9 +1203,9 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> + apr_array_header_t *revprops,
> svn_boolean_t descending_order,
> - svn_log_message_receiver2_t receiver,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> svn_repos_authz_func_t authz_read_func,
> void *authz_read_baton,
> @@ -1243,7 +1261,7 @@
> if (descending_order)
> {
> SVN_ERR(send_logs(paths, current, fs, discover_changed_paths,
> - include_merged_revisions, omit_log_text,
> + include_merged_revisions, revprops,
> descending_order, receiver, receiver_baton,
> authz_read_func, authz_read_baton, iterpool));
>
> @@ -1272,7 +1290,7 @@
> svn_revnum_t),
> fs, discover_changed_paths,
> include_merged_revisions,
> - omit_log_text, descending_order,
> + revprops, descending_order,
> receiver, receiver_baton,
> authz_read_func, authz_read_baton, iterpool));
>
> @@ -1295,10 +1313,10 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> + apr_array_header_t *revprops,
> svn_repos_authz_func_t authz_read_func,
> void *authz_read_baton,
> - svn_log_message_receiver2_t receiver,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton,
> apr_pool_t *pool)
> {
> @@ -1373,7 +1391,7 @@
> SVN_ERR(send_logs(paths, rev, fs,
> discover_changed_paths,
> include_merged_revisions,
> - omit_log_text, descending_order,
> + revprops, descending_order,
> receiver, receiver_baton,
> authz_read_func, authz_read_baton,
> iterpool));
> @@ -1385,7 +1403,7 @@
>
> SVN_ERR(do_logs(repos->fs, paths, hist_start, hist_end, limit,
> discover_changed_paths, strict_node_history,
> - include_merged_revisions, omit_log_text,
> + include_merged_revisions, revprops,
> descending_order, receiver, receiver_baton,
> authz_read_func, authz_read_baton, pool));
>
> @@ -1407,7 +1425,7 @@
> void *receiver_baton,
> apr_pool_t *pool)
> {
> - svn_log_message_receiver2_t receiver2;
> + svn_log_entry_receiver_t receiver2;
> void *receiver2_baton;
>
> svn_compat_wrap_log_receiver(&receiver2, &receiver2_baton,
> @@ -1416,7 +1434,8 @@
>
> return svn_repos_get_logs4(repos, paths, start, end, limit,
> discover_changed_paths, strict_node_history,
> - FALSE, FALSE, authz_read_func, authz_read_baton,
> + FALSE, svn_compat_log_revprops_in(pool),
> + authz_read_func, authz_read_baton,
> receiver2, receiver2_baton,
> pool);
> }
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 26570)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1178,17 +1178,19 @@
> svn_boolean_t discover_changed_paths,
> svn_boolean_t strict_node_history,
> svn_boolean_t include_merged_revisions,
> - svn_boolean_t omit_log_text,
> - svn_log_message_receiver2_t receiver,
> + apr_array_header_t *revprops,
> + svn_log_entry_receiver_t receiver,
> void *receiver_baton, apr_pool_t *pool)
> {
> svn_ra_svn__session_baton_t *sess_baton = session->priv;
> svn_ra_svn_conn_t *conn = sess_baton->conn;
> apr_pool_t *subpool;
> int i;
> - const char *path, *author, *date, *message, *cpath, *action, *copy_path;
> + const char *path, *cpath, *action, *copy_path;
> + svn_string_t *author, *date, *message;
> svn_ra_svn_item_t *item, *elt;
> - apr_array_header_t *cplist;
> + char *name;
> + apr_array_header_t *cplist, *rplist;
> apr_hash_t *cphash;
> svn_revnum_t rev, copy_rev;
> svn_log_changed_path_t *change;
> @@ -1196,6 +1198,8 @@
> apr_uint64_t has_children_param, invalid_revnum_param;
> svn_boolean_t has_children;
> svn_log_entry_t *log_entry;
> + svn_boolean_t want_custom_revprops;
> + apr_uint64_t revprop_count;
>
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w((!", "log"));
> if (paths)
> @@ -1206,11 +1210,32 @@
> SVN_ERR(svn_ra_svn_write_cstring(conn, pool, path));
> }
> }
> - SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)(?r)(?r)bbnbb)", start, end,
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)(?r)(?r)bbnb!", start, end,
> discover_changed_paths, strict_node_history,
> (apr_uint64_t) limit,
> - include_merged_revisions,
> - omit_log_text));
> + include_merged_revisions));
> + if (revprops)
> + {
> + want_custom_revprops = FALSE;
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!w(!", "revprops"));
> + for (i = 0; i < revprops->nelts; i++)
> + {
> + name = APR_ARRAY_IDX(revprops, i, char *);
> + SVN_ERR(svn_ra_svn_write_cstring(conn, pool, name));
> + if (!want_custom_revprops
> + && strcmp(name, SVN_PROP_REVISION_AUTHOR) != 0
> + && strcmp(name, SVN_PROP_REVISION_DATE) != 0
> + && strcmp(name, SVN_PROP_REVISION_LOG) != 0)
> + want_custom_revprops = TRUE;
> + }
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!))"));
> + }
> + else
> + {
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!w())", "all-revprops"));
> + want_custom_revprops = TRUE;
> + }
> +
> SVN_ERR(handle_auth_request(sess_baton, pool));
>
> /* Read the log messages. */
> @@ -1223,10 +1248,16 @@
> if (item->kind != SVN_RA_SVN_LIST)
> return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> _("Log entry not a list"));
> - SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, subpool, "lr(?c)(?c)(?c)?BB",
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, subpool, "lr(?s)(?s)(?s)?BBnl",
> &cplist, &rev, &author, &date,
> &message, &has_children_param,
> - &invalid_revnum_param));
> + &invalid_revnum_param,
> + &revprop_count, &rplist));
> + if (want_custom_revprops && rplist == NULL)
> + /* Caller asked for custom revprops, but server is too old. */
> + return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, NULL,
> + _("Server does not support custom revprops"
> + " via log"));
>
> if (has_children_param == SVN_RA_SVN_UNSPECIFIED_NUMBER)
> has_children = FALSE;
> @@ -1271,11 +1302,71 @@
>
> log_entry->changed_paths = cphash;
> log_entry->revision = rev;
> - log_entry->author = author;
> - log_entry->date = date;
> - log_entry->message = message;
> log_entry->has_children = has_children;
> -
> + if (rplist)
> + SVN_ERR(svn_ra_svn_parse_proplist(rplist, pool,
> + &log_entry->revprops));
> + if (revprops == NULL)
> + {
> + /* Caller requested all revprops; set author/date/log. */
> + if (author)
> + {
> + /* XXX I do this all over the place; either I should
> + write a utility function to apr_hash_make if needed
> + before apr_hash_set, or we should sometimes return an
> + empty hash instead of NULL, unlike changed_paths . */
> + if (log_entry->revprops == NULL)
> + log_entry->revprops = apr_hash_make(pool);
> + apr_hash_set(log_entry->revprops, SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING, author);
> + }
> + if (date)
> + {
> + if (log_entry->revprops == NULL)
> + log_entry->revprops = apr_hash_make(pool);
> + apr_hash_set(log_entry->revprops, SVN_PROP_REVISION_DATE,
> + APR_HASH_KEY_STRING, date);
> + }
> + if (message)
> + {
> + if (log_entry->revprops == NULL)
> + log_entry->revprops = apr_hash_make(pool);
> + apr_hash_set(log_entry->revprops, SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING, message);
> + }
> + }
> + else
> + {
> + /* Caller requested some; maybe set author/date/log. */
> + for (i = 0; i < revprops->nelts; i++)
> + {
> + name = APR_ARRAY_IDX(revprops, i, char *);
> + if (author && strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0)
> + {
> + if (log_entry->revprops == NULL)
> + log_entry->revprops = apr_hash_make(pool);
> + apr_hash_set(log_entry->revprops,
> + SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING, author);
> + }
> + if (date && strcmp(name, SVN_PROP_REVISION_DATE) == 0)
> + {
> + if (log_entry->revprops == NULL)
> + log_entry->revprops = apr_hash_make(pool);
> + apr_hash_set(log_entry->revprops,
> + SVN_PROP_REVISION_DATE,
> + APR_HASH_KEY_STRING, date);
> + }
> + if (message && strcmp(name, SVN_PROP_REVISION_LOG) == 0)
> + {
> + if (log_entry->revprops == NULL)
> + log_entry->revprops = apr_hash_make(pool);
> + apr_hash_set(log_entry->revprops,
> + SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING, message);
> + }
> + }
> + }
> SVN_ERR(receiver(receiver_baton, log_entry, subpool));
> }
>
> Index: subversion/libsvn_ra_svn/protocol
> ===================================================================
> --- subversion/libsvn_ra_svn/protocol (revision 26570)
> +++ subversion/libsvn_ra_svn/protocol (working copy)
> @@ -342,14 +342,17 @@
> log
> params: ( ( target-path:string ... ) [ start-rev:number ]
> [ end-rev:number ] changed-paths:bool strict-node:bool
> - ? limit:number
> - ? include-merged-revisions:bool omit-log-text:bool )
> + ? limit:number
> + ? include-merged-revisions:bool
> + all-revprops | revprops
> + ? ( revprop:string ... ) )
> Before sending response, server sends log entries, ending with "done".
> If a client does not want to specify a limit, it should send 0 as the
> limit parameter.
> log-entry: ( ( change:changed-path-entry ... ) rev:number
> - [ author:string ] [ date:string ] [ message:string ]
> - ? has-children:bool invalid-revnum:bool )
> + [ author:string ] [ date:string ] [ message:string ]
> + ? has-children:bool invalid-revnum:bool
> + revprop-count:number rev-props:proplist )

You should probably document whether or not author/date/message show
up in the rev-props as well. (I think they don't?)

> | done
> changed-path-entry: ( path:string A|D|R|M [ copy-path:string ]
> [ copy-rev:number ] )
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 26570)
> +++ subversion/svn/cl.h (working copy)
> @@ -84,6 +84,7 @@
> svn_cl__xml_opt,
> svn_cl__keep_local_opt,
> svn_cl__with_revprop_opt,
> + svn_cl__with_all_revprops_opt,
> svn_cl__parents_opt,
> svn_cl__accept_opt
> } svn_cl__longopt_t;
> @@ -156,7 +157,8 @@
> const char *changelist; /* operate on this changelist */
> svn_boolean_t keep_changelist; /* don't remove changelist after commit */
> svn_boolean_t keep_local; /* delete path only from repository */
> - apr_hash_t *revprop_table; /* table with revision properties to set */
> + svn_boolean_t all_revprops; /* retrieve all props */
> + apr_hash_t *revprop_table; /* table of revision properties to get/set */
> svn_boolean_t parents; /* create intermediate directories */
> svn_boolean_t use_merge_history; /* use/display extra merge information */
> svn_accept_t accept_which; /* automatically resolve conflict */
> Index: subversion/svn/log-cmd.c
> ===================================================================
> --- subversion/svn/log-cmd.c (revision 26570)
> +++ subversion/svn/log-cmd.c (working copy)
> @@ -41,7 +41,7 @@
> 
> /*** Code. ***/
>
> -/* Baton for log_message_receiver() and log_message_receiver_xml(). */
> +/* Baton for log_entry_receiver() and log_entry_receiver_xml(). */
> struct log_receiver_baton
> {
> /* Check for cancellation on each invocation of a log receiver. */
> @@ -64,7 +64,7 @@
> "------------------------------------------------------------------------\n"
>
>
> -/* Implement `svn_log_message_receiver2_t', printing the logs in
> +/* Implement `svn_log_entry_receiver_t', printing the logs in
> * a human-readable and machine-parseable format.
> *
> * BATON is of type `struct log_receiver_baton'.
> @@ -142,14 +142,14 @@
> *
> */
> static svn_error_t *
> -log_message_receiver(void *baton,
> - svn_log_entry_t *log_entry,
> - apr_pool_t *pool)
> +log_entry_receiver(void *baton,
> + svn_log_entry_t *log_entry,
> + apr_pool_t *pool)
> {
> struct log_receiver_baton *lb = baton;
> - const char *author = log_entry->author;
> - const char *date = log_entry->date;
> - const char *msg = log_entry->message;
> + const char *author;
> + const char *date;
> + const char *message;
>
> /* Number of lines in the msg. */
> int lines;
> @@ -157,7 +157,9 @@
> if (lb->cancel_func)
> SVN_ERR(lb->cancel_func(lb->cancel_baton));
>
> - if (log_entry->revision == 0 && log_entry->message == NULL)
> + svn_compat_log_revprops_out(&author, &date, &message, log_entry->revprops);
> +
> + if (log_entry->revision == 0 && message == NULL)
> return SVN_NO_ERROR;
>
> if (! SVN_IS_VALID_REVNUM(log_entry->revision))
> @@ -169,10 +171,10 @@
> /* ### See http://subversion.tigris.org/issues/show_bug.cgi?id=807
> for more on the fallback fuzzy conversions below. */
>
> - if (log_entry->author == NULL)
> + if (author == NULL)
> author = _("(no author)");
>
> - if (log_entry->date && log_entry->date[0])
> + if (date && date[0])
> {
> /* Convert date to a format for humans. */
> apr_time_t time_temp;
> @@ -183,16 +185,16 @@
> else
> date = _("(no date)");
>
> - if (! lb->omit_log_message && log_entry->message == NULL)
> - msg = "";
> + if (! lb->omit_log_message && message == NULL)
> + message = "";
>
> SVN_ERR(svn_cmdline_printf(pool,
> SEP_STRING "r%ld | %s | %s",
> log_entry->revision, author, date));
>
> - if (! lb->omit_log_message)
> + if (message != NULL)
> {
> - lines = svn_cstring_count_newlines(msg) + 1;
> + lines = svn_cstring_count_newlines(message) + 1;
> SVN_ERR(svn_cmdline_printf(pool,
> (lines != 1)
> ? " | %d lines"
> @@ -252,10 +254,10 @@
> }
> }
>
> - if (! lb->omit_log_message)
> + if (message != NULL)
> {
> /* A blank line always precedes the log message. */
> - SVN_ERR(svn_cmdline_printf(pool, "\n%s\n", msg));
> + SVN_ERR(svn_cmdline_printf(pool, "\n%s\n", message));
> }
>
> SVN_ERR(svn_cmdline_fflush(stdout));
> @@ -267,7 +269,7 @@
> }
>
>
> -/* This implements `svn_log_message_receiver2_t', printing the logs in XML.
> +/* This implements `svn_log_entry_receiver_t', printing the logs in XML.
> *
> * BATON is of type `struct log_receiver_baton'.
> *
> @@ -304,26 +306,31 @@
> *
> */
> static svn_error_t *
> -log_message_receiver_xml(void *baton,
> - svn_log_entry_t *log_entry,
> - apr_pool_t *pool)
> +log_entry_receiver_xml(void *baton,
> + svn_log_entry_t *log_entry,
> + apr_pool_t *pool)
> {
> struct log_receiver_baton *lb = baton;
> /* Collate whole log message into sb before printing. */
> svn_stringbuf_t *sb = svn_stringbuf_create("", pool);
> char *revstr;
> - const char *date = log_entry->date;
> + const char *author;
> + const char *date;
> + const char *message;
>
> if (lb->cancel_func)
> SVN_ERR(lb->cancel_func(lb->cancel_baton));
>
> - if (log_entry->revision == 0 && log_entry->message == NULL)
> + svn_compat_log_revprops_out(&author, &date, &message, log_entry->revprops);
> +
> + if (log_entry->revision == 0 && message == NULL)
> return SVN_NO_ERROR;
>
> if (! SVN_IS_VALID_REVNUM(log_entry->revision))
> {
> - svn_xml_make_close_tag(&sb, pool, "logentry");
> - SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
> + /* XXX: Hyrum, double-check me here? */
> +/* svn_xml_make_close_tag(&sb, pool, "logentry"); */
> +/* SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout)); */
> apr_array_pop(lb->merge_stack);
>
> return SVN_NO_ERROR;
> @@ -335,13 +342,13 @@
> "revision", revstr, NULL);
>
> /* <author>xxx</author> */
> - svn_cl__xml_tagged_cdata(&sb, pool, "author", log_entry->author);
> + svn_cl__xml_tagged_cdata(&sb, pool, "author", author);
>
> /* Print the full, uncut, date. This is machine output. */
> - /* According to the docs for svn_log_message_receiver_t, either
> + /* According to the docs for svn_log_entry_receiver_t, either
> NULL or the empty string represents no date. Avoid outputting an
> empty date element. */
> - if (log_entry->date && log_entry->date[0] == '\0')
> + if (date && date[0] == '\0')
> date = NULL;
> /* <date>xxx</date> */
> svn_cl__xml_tagged_cdata(&sb, pool, "date", date);
> @@ -397,17 +404,22 @@
> svn_xml_make_close_tag(&sb, pool, "paths");
> }
>
> - if (! lb->omit_log_message)
> + if (message != NULL)
> {
> - const char *msg = log_entry->message;
> -
> - if (log_entry->message == NULL)
> - msg = "";
> -
> /* <msg>xxx</msg> */
> - svn_cl__xml_tagged_cdata(&sb, pool, "msg", msg);
> + svn_cl__xml_tagged_cdata(&sb, pool, "msg", message);
> }
>
> + svn_compat_log_revprops_clear(&log_entry->revprops);
> + if (log_entry->revprops && apr_hash_count(log_entry->revprops) > 0)
> + {
> + svn_xml_make_open_tag(&sb, pool, svn_xml_normal, "revprops", NULL);
> + SVN_ERR(svn_cl__print_xml_prop_hash(&sb, log_entry->revprops,
> + FALSE, /* name_only */
> + pool));
> + svn_xml_make_close_tag(&sb, pool, "revprops");
> + }
> +
> if (log_entry->has_children)
> APR_ARRAY_PUSH(lb->merge_stack, svn_revnum_t) = log_entry->revision;
> else
> @@ -434,7 +446,20 @@
> int i;
> svn_opt_revision_t peg_revision;
> const char *true_path;
> + apr_array_header_t *revprops;
>
> + if (!opt_state->xml)
> + {
> + if (opt_state->all_revprops)
> + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'with-all-revprops' option only valid in"
> + " XML mode"));
> + if (opt_state->revprop_table != NULL)
> + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'with-revprop' option only valid in"
> + " XML mode"));
> + }
> +

Perhaps check here to disallow use of both --with-all-revprops and
--with-revprop simultaneously?

> /* Before allowing svn_opt_args_to_target_array() to canonicalize
> all the targets, we need to build a list of targets made of both
> ones the user typed, as well as any specified by --changelist. */
> @@ -547,6 +572,31 @@
> if (! opt_state->incremental)
> SVN_ERR(svn_cl__xml_print_header("log", pool));
>
> + if (opt_state->all_revprops)
> + revprops = NULL;
> + else if (opt_state->revprop_table != NULL)
> + {
> + apr_hash_index_t *hi;
> + revprops = apr_array_make(pool,
> + apr_hash_count(opt_state->revprop_table),
> + sizeof(char *));
> + for (hi = apr_hash_first(pool, opt_state->revprop_table);
> + hi != NULL;
> + hi = apr_hash_next(hi))
> + {
> + char *property;
> + apr_hash_this(hi, (void *)&property, NULL, NULL);

Here (or actually, better in parse_revprop), use
svn_prop_name_is_valid to make sure the property name is legal.

Also, it appears that the user can type

  $ svn log --xml --with-revprop foo=bar

and it will be interpreted as '--with-revprop foo'. Perhaps check to
make sure that the *value* in the hash is "" and error otherwise?

(Also, huh, there's no "make an array from the keys of this hash"
function?)

> + APR_ARRAY_PUSH(revprops, char *) = property;
> + }
> + }
> + else
> + {
> + revprops = apr_array_make(pool, 3, sizeof(char *));
> + APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_AUTHOR;
> + 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,
> &peg_revision,
> &(opt_state->start_revision),
> @@ -555,8 +605,8 @@
> opt_state->verbose,
> opt_state->stop_on_copy,
> opt_state->use_merge_history,
> - opt_state->quiet,
> - log_message_receiver_xml,
> + revprops,
> + log_entry_receiver_xml,
> &lb,
> ctx,
> pool));
> @@ -566,6 +616,11 @@
> }
> else /* default output format */
> {
> + revprops = apr_array_make(pool, 3, sizeof(char *));
> + APR_ARRAY_PUSH(revprops, const char *) = SVN_PROP_REVISION_AUTHOR;
> + 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,
> &peg_revision,
> &(opt_state->start_revision),
> @@ -574,8 +629,8 @@
> opt_state->verbose,
> opt_state->stop_on_copy,
> opt_state->use_merge_history,
> - opt_state->quiet,
> - log_message_receiver,
> + revprops,
> + log_entry_receiver,
> &lb,
> ctx,
> pool));
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 26570)
> +++ subversion/svn/main.c (working copy)
> @@ -198,6 +198,8 @@
> N_("don't delete changelist after commit")},
> {"keep-local", svn_cl__keep_local_opt, 0,
> N_("keep path in working copy")},
> + {"with-all-revprops", svn_cl__with_all_revprops_opt, 0,
> + N_("retrieve all revision properties")},
> {"with-revprop", svn_cl__with_revprop_opt, 1,
> N_("set revision property ARG in new revision\n"
> " "
> @@ -521,7 +523,9 @@
> " svn log http://www.example.com/repo/project foo.c bar.c\n"),
> {'r', 'q', 'v', 'g', svn_cl__targets_opt, svn_cl__stop_on_copy_opt,
> svn_cl__incremental_opt, svn_cl__xml_opt, SVN_CL__AUTH_OPTIONS,
> - svn_cl__config_dir_opt, 'l', svn_cl__changelist_opt} },
> + svn_cl__config_dir_opt, 'l', svn_cl__changelist_opt,
> + svn_cl__with_all_revprops_opt, svn_cl__with_revprop_opt},
> + {{svn_cl__with_revprop_opt, N_("retrieve revision property ARG")}} },
>
> { "merge", svn_cl__merge, {0}, N_
> ("Apply the differences between two sources to a working copy path.\n"
> @@ -1356,6 +1360,9 @@
> case svn_cl__keep_local_opt:
> opt_state.keep_local = TRUE;
> break;
> + case svn_cl__with_all_revprops_opt:
> + opt_state.all_revprops = TRUE;
> + break;
> case svn_cl__with_revprop_opt:
> err = parse_revprop(&opt_state.revprop_table, opt_arg, pool);
> if (err != SVN_NO_ERROR)
> Index: subversion/svnserve/serve.c
> ===================================================================
> --- subversion/svnserve/serve.c (revision 26570)
> +++ subversion/svnserve/serve.c (working copy)
> @@ -1525,6 +1525,8 @@
> svn_log_changed_path_t *change;
> svn_boolean_t invalid_revnum = FALSE;
> char action[2];
> + const char *author, *date, *message;
> + apr_uint64_t revprop_count;
>
> if (log_entry->revision == SVN_INVALID_REVNUM)
> {
> @@ -1556,13 +1558,26 @@
> change->copyfrom_rev));
> }
> }
> - SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)r(?c)(?c)(?c)bb",
> + if (log_entry->revprops)
> + {
> + svn_compat_log_revprops_out(&author, &date, &message,
> + log_entry->revprops);
> + svn_compat_log_revprops_clear(&log_entry->revprops);
> + /* XXX change if we make the above NULL this out */
> + revprop_count = apr_hash_count(log_entry->revprops);
> + }
> + else
> + {
> + author = date = message = NULL;
> + revprop_count = 0;
> + }
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)r(?c)(?c)(?c)bbn(!",
> log_entry->revision,
> - log_entry->author,
> - log_entry->date,
> - log_entry->message,
> + author, date, message,
> log_entry->has_children,
> - invalid_revnum));
> + invalid_revnum, revprop_count));
> + SVN_ERR(svn_ra_svn_write_proplist(conn, pool, log_entry->revprops));
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)"));
>
> if (log_entry->has_children)
> b->stack_depth++;
> @@ -1578,28 +1593,49 @@
> svn_revnum_t start_rev, end_rev;
> const char *full_path;
> svn_boolean_t changed_paths, strict_node, include_merged_revisions;
> - svn_boolean_t omit_log_text;
> - apr_array_header_t *paths, *full_paths;
> + apr_array_header_t *paths, *full_paths, *revprop_items, *revprops;
> + char *revprop_word;
> svn_ra_svn_item_t *elt;
> int i;
> - apr_uint64_t limit, include_merged_revs_param, omit_log_text_param;
> + apr_uint64_t limit, include_merged_revs_param;
> log_baton_t lb;
>
> - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "l(?r)(?r)bb?n?BB", &paths,
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "l(?r)(?r)bb?n?Bwl", &paths,
> &start_rev, &end_rev, &changed_paths,
> &strict_node, &limit,
> &include_merged_revs_param,
> - &omit_log_text_param));
> + &revprop_word, &revprop_items));
>
> if (include_merged_revs_param == SVN_RA_SVN_UNSPECIFIED_NUMBER)
> include_merged_revisions = FALSE;
> else
> include_merged_revisions = include_merged_revs_param;
>
> - if (omit_log_text_param == SVN_RA_SVN_UNSPECIFIED_NUMBER)
> - omit_log_text = FALSE;
> + if (revprop_word == NULL)
> + /* pre-1.5 client */
> + revprops = svn_compat_log_revprops_in(pool);
> + else if (strcmp(revprop_word, "all-revprops") == 0)
> + revprops = NULL;
> + else if (strcmp(revprop_word, "revprops") == 0)
> + {
> + revprops = apr_array_make(pool, revprop_items->nelts,
> + sizeof(char *));
> + if (revprop_items)
> + {
> + for (i = 0; i < revprop_items->nelts; i++)
> + {
> + elt = &APR_ARRAY_IDX(revprop_items, i, svn_ra_svn_item_t);
> + if (elt->kind != SVN_RA_SVN_STRING)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Log revprop entry not astring"));

Missing space between "a" and "string".

> + APR_ARRAY_PUSH(revprops, const char *) = elt->u.string->data;
> + }
> + }
> + }
> else
> - omit_log_text = omit_log_text_param;
> + return svn_error_createf(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Unknown revprop word '%s' in log command"),
> + revprop_word);
>
> /* If we got an unspecified number then the user didn't send us anything,
> so we assume no limit. If it's larger than INT_MAX then someone is
> @@ -1614,6 +1650,7 @@
> elt = &APR_ARRAY_IDX(paths, i, svn_ra_svn_item_t);
> if (elt->kind != SVN_RA_SVN_STRING)
> return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + /* XXX wrap in _() in follow-up? */
> "Log path entry not a string");
> full_path = svn_path_join(b->fs_path->data,
> svn_path_canonicalize(elt->u.string->data,
> @@ -1629,7 +1666,7 @@
> lb.stack_depth = 0;
> err = svn_repos_get_logs4(b->repos, full_paths, start_rev, end_rev,
> (int) limit, changed_paths, strict_node,
> - include_merged_revisions, omit_log_text,
> + include_merged_revisions, revprops,
> authz_check_access_cb_func(b), b, log_receiver,
> &lb, pool);
>
>

In general I like the feature though :)

--dave

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Sep 16 01:54:36 2007