Marcus Rueckert <darix@web.de> writes:
> Add a function to get the path of the repository on disk from other
> apache modules.
>
> * subversion/mod_dav_svn/mod_dav_svn.c
> (dav_svn_get_fs_dir): returns the path to the repository on disk
>
> * subversion/include/mod_dav_svn.h
> (dav_svn_get_fs_dir): declare the function and make it public to the
> for other apache modules.
Seems an odd name, why not 'dav_svn_repos_path' or something like that?
> Index: subversion/include/mod_dav_svn.h
> ===================================================================
> --- subversion/include/mod_dav_svn.h (revision 13799)
> +++ subversion/include/mod_dav_svn.h (working copy)
> @@ -74,6 +74,14 @@
> const char **repos_path);
>
>
> +/* Given an apache request R and a ROOT_PATH to the svn location
> + block this function returns the path to the repository on disk.
> +
> + The function returns NULL in case of an error in dav_svn_split_uri.
> + */
> +AP_MODULE_DECLARE(const char *) dav_svn_get_fs_dir(request_rec *r,
> + const char *root_path);
> +
The doc string here indicates that it returns "path to the repository"
too.
This doc string describes the NULL return in terms of an
implementation detail. A reader may not know or care why
dav_svn_split_uri() would be called anyway. Can you reword it to
describe something meaningful about the circumstances under which NULL
could be returned?
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/mod_dav_svn/mod_dav_svn.c
> ===================================================================
> --- subversion/mod_dav_svn/mod_dav_svn.c (revision 13799)
> +++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
> @@ -32,6 +32,7 @@
>
> #include "dav_svn.h"
>
> +#include <mod_dav_svn.h>
Why did you have to add this #include line? I'm surprised it wasn't
required before. Also, you don't mention it in your log message.
> /* 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(const char) *dav_svn_get_fs_dir(request_rec *r,
> + const char *root_path)
> +{
Indentation problem in the declaration.
> + const char *fs_path;
> + const char *repo_name;
> + const char *cleaned_uri;
> + const char *repos_name;
> + const char *relative;
> + const char *repos_path;
> + int had_slash;
You have one variable "repo_name", and another "repos_name", and a
third "repos_path". Can you see why a reader might get confused? :-)
Some of these are used but ignored, for example, 'had_slash'. You
should named the variable 'ignored', or 'ignored_had_slash', or put a
comment next to it indicating that it is unused, or *something* to
give the reader some guidance.
> + dav_error *err;
> + dav_svn_dir_conf *conf;
> +
> + conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
> +
> + repo_name = dav_svn_get_repo_name(r);
> +
> + /* This does all the work of interpreting/splitting the request uri. */
> + err = dav_svn_split_uri(r, r->uri, root_path,
> + &cleaned_uri, &had_slash,
> + &repos_name, &relative, &repos_path);
> +
> + /* we encountered an error on splitting. silently ignore it and return nothing. */
> + if (err)
> + return NULL;
> +
> + /* The path that we will eventually try to open as an svn
> + repository. Normally defined by the SVNPath directive.
> +
> + At this point we can be sure that fs_path or fs_parent_path
> + is non-NULL. otherwise dav_svn_split_uri had thrown an error.
> + */
> + fs_path = conf->fs_path;
>
> + /* If the SVNParentPath directive was used instead... */
> + if (conf->fs_parent_path != NULL)
> + {
> + /* ...and we need to specify exactly what repository to open. */
> + fs_path = svn_path_join(conf->fs_parent_path, repos_name, r->pool);
> + }
> +
> + return fs_path;
> +}
I'm a confused by this code.
You *call* dav_svn_split_uri(), but if conf->fs_parent_path is NULL,
you don't use any of the results, all you use is conf->fs_path, which
was obtained from the very first function call. I don't understand
how the normal SVNPath case is handled. Why is it sufficient to set
fs_path to conf->fs_path and just return it, in this case? Don't we
need to do something more? (And if we don't, then why do we do so
much other work before the return?)
You set 'repo_name' to the result of dav_svn_get_repo_name(), but you
never use the value anywhere.
Also, you have a comment line exceeding 80 characters; please keep
lines under 78 chars or so.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 31 20:23:22 2005