On Saturday 14 January 2006 15:36, Maxime Petazzoni wrote:
> [[[
> Make mod_dav_svn retrieve a repository name even when configured to
> operate with SVNParentPath.
>
> * subversion/mod_dav_svn/mod_dav_svn.c
> (dav_svn_get_repo_name2): New function, revved from
> dav_svn_get_repo_name. Takes new root_path and repo_name arguments
> and returns a dav_error for error propagation. Retrieves the
> repository name in a SVNParentPath setup using dav_svn_split_uri.
> (dav_svn_get_repo_name): Deprecate.
The revving of mod_dav_svn internal functions was quickly brought up on irc,
and the final consensus was "When in doubt, rev it. Even so, I can't help
wondering if this is not overkill in this case. To me, the function feels
clearly internal, and is called in a single spot of mod_dav_svn and nowhere
else. Is it really required to add the noise of API revving in this case?
Especially given that the v1 function will be lacking in functionality, as
the repository name cannot be extracted without being passed the extra
root_path argument.
Well, that's the colour of my bikeshed at least. Do we have known 3rd party
modules relying directly on that function? In the contrary case, my feeling
is that we could spare the revving.
> Index: subversion/mod_dav_svn/mod_dav_svn.c
> ===================================================================
> --- subversion/mod_dav_svn/mod_dav_svn.c (revision 18075)
> +++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
> @@ -326,6 +326,9 @@
> return NULL;
> }
>
> +/* Get the name of the repository targetted by the request. Note that
> + if the configuration uses SVNParentPath, NULL is always returned
> + (see dav_svn_get_repo_name2). */
> const char *dav_svn_get_repo_name(request_rec *r)
> {
> dav_svn_dir_conf *conf;
> @@ -334,6 +337,47 @@
> return conf->repo_name;
> }
>
> +dav_error *dav_svn_get_repo_name2(request_rec *r, const char *root_path,
> + char **repo_name)
> +{
> + const char *fs_parent_path;
> + const char *repos_name;
> + const char *ignored_path_in_repos;
> + const char *ignored_cleaned_uri;
> + const char *ignored_relative;
> + int ignored_had_slash;
> + dav_error *derr;
> + dav_svn_dir_conf *conf;
> +
> + /* Handle the SVNParentPath case. When this directive is used, we
> + can't retrieve the repository name directly from the module's
> + configuration structure : we have to fetch it from the URI. */
> + fs_parent_path = dav_svn_get_fs_parent_path(r);
> +
> + if (fs_parent_path != NULL)
> + {
> + /* Split the svn URI to get the name of the repository below
> + the parent path. */
> + derr = dav_svn_split_uri(r, r->uri, root_path,
> + &ignored_cleaned_uri,&ignored_had_slash,
> + &repos_name,
> + &ignored_relative,&ignored_path_in_repos);
> +
> + if (derr)
> + return derr;
> +
> + *repo_name = repos_name;
> + return NULL;
> + }
> +
> + /* Otherwise, return the repository name from the SVNPath
> + configuration directive. */
> + conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
> + *repo_name = conf->repo_name;
> +
> + return NULL;
> +}
> +
> const char *dav_svn_get_xslt_uri(request_rec *r)
> {
> dav_svn_dir_conf *conf;
> Index: subversion/mod_dav_svn/dav_svn.h
> ===================================================================
> --- subversion/mod_dav_svn/dav_svn.h (revision 18075)
> +++ subversion/mod_dav_svn/dav_svn.h (working copy)
> @@ -313,6 +313,8 @@
>
> /* Return a descriptive name for the repository */
> const char *dav_svn_get_repo_name(request_rec *r);
> +dav_error *dav_svn_get_repo_name2(request_rec *r, const char *root_path,
> + char **repo_name);
>
> /* Return the URI of an XSL transform stylesheet */
> const char *dav_svn_get_xslt_uri(request_rec *r);
> Index: subversion/mod_dav_svn/repos.c
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 18075)
> +++ subversion/mod_dav_svn/repos.c (working copy)
> @@ -1267,7 +1267,10 @@
> dav_locktoken_list *ltl;
> struct cleanup_fs_access_baton *cleanup_baton;
>
> - repo_name = dav_svn_get_repo_name(r);
> + err = dav_svn_get_repo_name2(r, root_path, &repo_name);
> + if (err)
> + return err;
> +
> xslt_uri = dav_svn_get_xslt_uri(r);
> fs_parent_path = dav_svn_get_fs_parent_path(r);
Other than the revving issue, +1 on the patch. If there are no further
thoughts on the revving in the next few hours I'll commit this.
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jan 14 16:26:34 2006