[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH]log and blame command should log the revision range in operational log

From: Daniel Rall <dlr_at_collab.net>
Date: 2007-02-20 23:05:31 CET

I'm conceptually in favor of this patch, but think the implementation
could be improved.

On Tue, 20 Feb 2007, Kamesh Jayachandran wrote:
...
> [[[
> 'log' and 'blame' commands need to log the revision range.

Better written as:

  Log the revision range for 'log' and 'blame' operations.

> * subversion/mod_dav_svn/reports/log.c
> (dav_svn__log_report): Log revision range.
> * subversion/mod_dav_svn/reports/file-revs.c
> (dav_svn__file_revs_report): Log revision range for 'blame' command.
>
> Patch by: kameshj
> Suggested by: Honey George <george@collab.net>
> ]]]

> Index: subversion/mod_dav_svn/reports/log.c
> ===================================================================
> --- subversion/mod_dav_svn/reports/log.c (revision 23436)
> +++ subversion/mod_dav_svn/reports/log.c (working copy)
> @@ -320,15 +320,20 @@
> if (paths->nelts == 0)
> action = "log";

Why don't we log the revision range for this case?

> else if (paths->nelts == 1)
> - action = apr_psprintf(resource->pool, "log-all '%s'",
> + action = apr_psprintf(resource->pool, "log-all '%s' r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> svn_path_uri_encode(APR_ARRAY_IDX
> (paths, 0, const char *),
> - resource->pool));
> + resource->pool),
> + start, end);
> else
> - action = apr_psprintf(resource->pool, "log-partial '%s'",
> + action = apr_psprintf(resource->pool, "log-partial '%s' " \
> + "r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> svn_path_uri_encode(APR_ARRAY_IDX
> (paths, 0, const char *),
> - resource->pool));
> + resource->pool),
> + start, end);

The only difference between these blocks now are the "log-all"
vs. "log-partial" identifiers. These two blocks could be collapsed
into one which contains an inline if/else statement which selects
which fragment of action text to use.

>
> apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION", action);
>
> Index: subversion/mod_dav_svn/reports/file-revs.c
> ===================================================================
> --- subversion/mod_dav_svn/reports/file-revs.c (revision 23436)
> +++ subversion/mod_dav_svn/reports/file-revs.c (working copy)
> @@ -311,8 +311,10 @@
>
> /* We've detected a 'high level' svn action to log. */
> apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION",
> - apr_psprintf(resource->pool, "blame '%s'",
> - svn_path_uri_encode(path, resource->pool)));
> + apr_psprintf(resource->pool, "blame '%s' r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> + svn_path_uri_encode(path, resource->pool),
> + start, end));
>
> /* Flush the contents of the brigade (returning an error only if we
> don't already have one). */

Looks good.

  • application/pgp-signature attachment: stored
Received on Tue Feb 20 23:05:44 2007

This is an archived mail posted to the Subversion Dev mailing list.