Marcus Rueckert wrote:
> Add a function to get the path of the repository on disk from other
> apache modules.
The direction of "from" is ambiguous - it can sounds like we are getting the
path from them. Maybe: "Add a function to mod_dav_svn to get the path of the
repository on disk. This can be useful to other Apache modules."
>
> * subversion/mod_dav_svn/mod_dav_svn.c
No indentation needed before "*".
> (dav_svn_get_repos_path): returns the path to the repository on disk
>
> Include mod_dav_svn.h so we get the prototype for
> dav_svn_get_repos_path and dav_svn_split_uri
Full stops would be nice.
> * subversion/include/mod_dav_svn.h
> (dav_svn_get_repos_path): declare the function and make it public to the
> for other apache modules.
Delete "for" (or "to the").
> ------------------------------------------------------------------------
>
> Index: subversion/include/mod_dav_svn.h
> ===================================================================
> --- subversion/include/mod_dav_svn.h (revision 13816)
> +++ subversion/include/mod_dav_svn.h (working copy)
> @@ -74,6 +74,13 @@
> const char **repos_path);
>
>
> +/* Given an apache request R and a ROOT_PATH to the svn location
> + block return the path to the repository on disk.
"block, set *REPOS_PATH to the path to the repository on disk."
(It returns an error code.)
> +*/
> +AP_MODULE_DECLARE(dav_error *) dav_svn_get_repos_path(request_rec *r,
> + const char *root_path,
> + const char **repos_path);
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/mod_dav_svn/mod_dav_svn.c
> ===================================================================
> --- subversion/mod_dav_svn/mod_dav_svn.c (revision 13816)
> +++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
> @@ -32,6 +32,7 @@
>
> #include "dav_svn.h"
>
> +#include <mod_dav_svn.h>
>
> /* This is the default "special uri" used for SVN's special resources
> (e.g. working resources, activities) */
> @@ -248,7 +249,52 @@
> return conf->fs_parent_path;
> }
>
> +AP_MODULE_DECLARE(dav_error *) dav_svn_get_repos_path(request_rec *r,
> + const char *root_path,
> + const char **repos_path)
> +{
>
> + const char *fs_path; /* the path to the repository on disk */
> + 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 *err;
> +
> + /* The path that we will eventually try to open as an svn
> + repository. Normally defined by the SVNPath directive. */
> + fs_path = dav_svn_get_fs_path(r);
> +
> + if (fs_path != NULL)
> + {
> + /* The config object should be long-lived, right?
> + * So can't we just directly assign a pointer? */
Please don't ask questions in the source comments. When possible, ask the
questions on the mailing list and put the answers into the comments as facts.
Otherwise each developer that looks at this will wonder, "I don't know ... is
this right or not? This code shouldn't have been committed unless the original
programmer was sure."
> + *repos_path = fs_path;
> + return NULL;
> + }
> +
> + /* If the SVNParentPath directive was used instead... */
> + fs_path = dav_svn_get_fs_parent_path(r);
> +
> + /* At this point we can be sure that fs_path is NULL.
We can? Then what was the purpose of the previous function call?
> + now let just compute the full path below svn parent path. */
"Now let's"
> + /* This does all the work of interpreting/splitting the request uri. */
> + err = dav_svn_split_uri(r, r->uri, root_path, &ignored_cleaned_uri,
> + &ignored_had_slash,
> + &repos_name,
> + &ignored_relative, &ignored_path_in_repos);
> +
> + /* push the error to the caller */
> + if (err)
> + return err;
> +
> + /* ...and we need to specify exactly what repository to open. */
> + *repos_path = svn_path_join(fs_path, repos_name, r->pool);
> + return NULL;
> +}
> +
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Apr 1 15:36:20 2005