Re: [PATCH] perform keywords substitution in mod_dav_svn

On 04/07/2013 01:38 PM, jinfroster wrote:
> Index: subversion/mod_dav_svn/dav_svn.h
> ===================================================================
> --- subversion/mod_dav_svn/dav_svn.h (revision 1465320)
> +++ subversion/mod_dav_svn/dav_svn.h (working copy)
> @@ -289,6 +289,9 @@
>
> /* Cache any revprop change error */
> svn_error_t *revprop_error;
> +
> + /* was keywords subsitution requested by client? (ie: /path/to/item?kw=1)*/
> + svn_boolean_t keyword_subst;
> };

Some typos there. "substitution".

> Index: subversion/mod_dav_svn/repos.c
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 1465320)
> +++ subversion/mod_dav_svn/repos.c (working copy)

[...]

> @@ -1924,6 +1926,10 @@
> 0, "redirecting to canonical location");
> }
>
> + keyword_subst = apr_table_get(pairs, "kw");
> + if (keyword_subst && *keyword_subst != '0')
> + comb->priv.keyword_subst = TRUE;
> +
> return NULL;
> }

I'm with Daniel -- this should check for "1" explicitly.

> @@ -3026,18 +3032,23 @@
>
>
> /* if we aren't sending a diff, then we know the length of the file,
> - so set up the Content-Length header */
> - serr = svn_fs_file_length(&length,
> - resource->info->root.root,
> - resource->info->repos_path,
> - resource->pool);
> - if (serr != NULL)
> + so set up the Content-Length header.
> + Except when keywords substitution was requested (length may increase
> + during deliver) */
> + if (resource->info->keyword_subst == FALSE)

Just use "if (!resource->info->keyword_subst)" -- no need to explicitly test
for the value "FALSE".

> @@ -3563,6 +3574,60 @@
> resource->pool);
> }
>
> + /* Perform keywords substitution if requested by client */
> + if (resource->info->keyword_subst == TRUE)

Same here, but the reverse.

> + {
> + const char *err_msg_props = "could not get revision properties"
> + " for keywords substitution";
> + svn_string_t *keywords;
> + serr = svn_fs_node_prop(&keywords,
> + resource->info->root.root,
> + resource->info->repos_path,
> + SVN_PROP_KEYWORDS,
> + resource->pool);
> + if (serr != NULL)
> + return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + err_msg_props, resource->pool);

Here, you return an error message about revision properties when you've just
tried to fetch *node* properties. In fact, I'm not thrilled about this
reused error message at all. Why not take the time to be give specific
errors for each specific problem we might hit?

> +
> + if (keywords)
> + {
> + apr_hash_t *kw;
> + svn_revnum_t cmt_rev;
> + char *str_cmt_rev, *str_uri;
> + const char *cmt_date, *cmt_author;
> + apr_time_t when = 0;
> +
> + serr = svn_repos_get_committed_info(&cmt_rev, &cmt_date, &cmt_author,
> + resource->info->root.root,
> + resource->info->repos_path,
> + resource->pool);
> + if (serr != NULL)
> + return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + err_msg_props, resource->pool);
> +
> + str_cmt_rev = apr_psprintf(resource->pool,
> + "%ld", cmt_rev);
> + str_uri = apr_psprintf(resource->pool,
> + "%s%s", resource->info->repos->base_url,
> + resource->info->r->uri);

This URL construction doesn't feel quite right. I *think* you want:

   svn_path_url_add_component2(resource->info->repos->base_url,
                               resource->info->repos_path,
                               resource->pool);

> + serr = svn_time_from_cstring(&when, cmt_date, resource->pool);
> + if (serr != NULL)
> + return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + err_msg_props, resource->pool);

Again, more sloppy error construction.

All that said, I think you've given us enough to work with here, so I'll try
to patch up your patch, test, and commit.

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development