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 <cmpil...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
signature.asc
Description: OpenPGP digital signature
