You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2005/04/01 13:35:01 UTC
Re: [PATCH] dav_svn_get_repos_path
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
Re: [PATCH] dav_svn_get_repos_path
Posted by Max Bowsher <ma...@ukf.net>.
Marcus Rueckert wrote:
>> hi
>>
>> hopefully the last version.
>> i fixed the comments for max.
>>
>> i fixed the log message. (ok i just copied the text from cmpilato)
>>
>> and i ran make fsfscheck successfully..
>> and my the tests from my mod_authn_svn.
Now committed.
( I tweaked the comments yet again :-) ).
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] dav_svn_get_repos_path
Posted by Marcus Rueckert <da...@web.de>.
hi
hopefully the last version.
i fixed the comments for max.
i fixed the log message. (ok i just copied the text from cmpilato)
and i ran make fsfscheck successfully..
and my the tests from my mod_authn_svn.
[[[
<Location /svn/path>
DAV svn
SVNPath /opt/apache2.1/svn/repos1
SVNAutoversioning on
# auth
AuthType Basic
AuthName "svn path"
AuthBasicAuthoritative On
AuthBasicProvider svn
require valid-user
</Location>
<Location /svn/parentpath>
DAV svn
SVNParentPath /opt/apache2.1/svn
SVNAutoversioning on
# auth
AuthType Basic
AuthName "svn parent path"
AuthBasicAuthoritative On
AuthBasicProvider svn
require valid-user
</Location>
<Location /nopath>
AuthType Basic
AuthName "svn parent path"
AuthBasicAuthoritative On
AuthBasicProvider svn
AuthSvnFile "/opt/apache2.1/svn/repos1/conf/htpasswd"
require valid-user
</Location>
]]]
this code snippet is from a Apache 2.1 module:
[[[
/* Given a username and password, expected to return AUTH_GRANTED
* if we can validate this user/password combination.
*/
static authn_status check_password(request_rec *r, const char *user,
const char *password)
{
authn_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authn_svn_module);
const char *repos_path;
dav_error *err;
err = dav_svn_get_repos_path (r, conf->base_path, &repos_path );
if (err)
{
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
"darix is debugging: %s", err->desc);
return AUTH_DENIED;
}
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
"darix is debugging: %s", repos_path);
return AUTH_GRANTED;
}
]]]
if any one is interested in the full code i will send it.
the results for the various blocks
curl -u darix http://localhost:8080/nopath
[[[
[Fri Apr 01 21:22:27 2005] [error] [client 127.0.0.1] darix is debugging: The server is misconfigured: either an SVNPath or SVN
ParentPath directive is required to specify the location of this resource's repository.
[Fri Apr 01 21:22:27 2005] [error] [client 127.0.0.1] user darix: authentication failure for "/nopath/": Password Mismatch
]]]
curl -u darix http://localhost:8080/svn/path/
[[[
[Fri Apr 01 21:21:41 2005] [error] [client 127.0.0.1] darix is debugging: /opt/apache2.1/svn/repos1
]]]
curl -u darix http://localhost:8080/svn/parentpath/repos2/
[[[
[Sat Apr 02 00:19:29 2005] [error] [client 127.0.0.1] darix is debugging: /opt/apache2.1/svn/repos2
]]]
hoping the best
darix
--
irssi - the client of the smart and beautiful people
http://www.irssi.de/
Re: [PATCH] dav_svn_get_repos_path
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> Marcus Rueckert <da...@web.de> writes:
>>Add a function to mod_dav_svn to get the path of the repository on diski.
>>This can be usefull for other apache modules.
>
> s/diski/disk/
> s/usefull/useful/
> s/apache/Apache/
[...]
> Noisy. You just need:
>
> Add a function to mod_dav_svn to get the path of the repository on disk.
> This can be useful for other Apache modules.
>
> * subversion/include/mod_dav_svn.h
> (dav_svn_get_repos_path): New.
>
> * subversion/mod_dav_svn/mod_dav_svn.c
> Include mod_dav_svn.h.
> (dav_svn_get_repos_path): New.
[...]
> Docstring is fine, I guess. Were it my patch, I might do the
> following instead:
[...]
Right. Given some or all of C. Michael Pilato's textual clean-ups, I'm happy
with this patch. However, it's not my area of expertise at all, and I can't
successfully run "make check" at the moment, so I'll have to leave this for
some other committer to commit.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] dav_svn_get_repos_path
Posted by "C. Michael Pilato" <cm...@collab.net>.
Marcus Rueckert <da...@web.de> writes:
> Add a function to mod_dav_svn to get the path of the repository on diski.
> This can be usefull for other apache modules.
s/diski/disk/
s/usefull/useful/
s/apache/Apache/
> * subversion/mod_dav_svn/mod_dav_svn.c
> (dav_svn_get_repos_path): Set *repos_path to 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.
>
> * subversion/include/mod_dav_svn.h
> (dav_svn_get_repos_path): Declare the function and make it public for other
> apache modules.
Noisy. You just need:
Add a function to mod_dav_svn to get the path of the repository on disk.
This can be useful for other Apache modules.
* subversion/include/mod_dav_svn.h
(dav_svn_get_repos_path): New.
* subversion/mod_dav_svn/mod_dav_svn.c
Include mod_dav_svn.h.
(dav_svn_get_repos_path): New.
> 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 set *REPOS_PATH to the path of the repository on disk.
> +*/
> +AP_MODULE_DECLARE(dav_error *) dav_svn_get_repos_path(request_rec *r,
> + const char *root_path,
> + const char **repos_path);
Docstring is fine, I guess. Were it my patch, I might do the
following instead:
/* Set *REPOS_PATH to the on-disk path of the repository associated
with Apache request R and a <Location> block ROOT_PATH. */
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] dav_svn_get_repos_path
Posted by Marcus Rueckert <da...@web.de>.
hi,
> 17:19:49 <@sussman:#svn-dev> never say, "return blah"
> 17:19:56 <@sussman:#svn-dev> say, "set *RETURN_VALUE to blah."
> 17:20:11 <@sussman:#svn-dev> because almost every function returns
> dav_error or svn_error_t anyway.
fixed log message accordingly. header file should be fine.
darix
--
irssi - the client of the smart and beautiful people
http://www.irssi.de/
Re: [PATCH] dav_svn_get_repos_path
Posted by Marcus Rueckert <da...@web.de>.
On 2005-04-01 14:35:01 +0100, Julian Foad wrote:
> 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."
thanks. :) i just took your text :)
> No indentation needed before "*".
fixed. i copy pasted the examples from the hacking file and didnt look
at the log inbetween. :)
> Full stops would be nice.
i added some. :)
> Delete "for" (or "to the").
done
> "block, set *REPOS_PATH to the path to the repository on disk."
> (It returns an error code.)
done.
> 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."
was an old comment. i removed the apr_pstrdup() but not the comment.
> We can? Then what was the purpose of the previous function call?
can i have an old comment obsoleter pleae. removed.
> "Now let's"
fixed
thanks for your review julian.
ready for the next round. new patch attached. :)
darix
--
irssi - the client of the smart and beautiful people
http://www.irssi.de/