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