subversion@millenix.mailshell.com writes:
> The authz checks and associated call to absent_file_or_dir() appear in both
> functions.
> This patch factors that out. This is the first real work I've done with the
> subversion code, so if it makes sense to do this, could someone check that I
> got the semantics right? Oh yea, it probably needs a better (shorter)
> function name :-)
Not necessarily a shorter name, but a less awkward one :-). No bright
ideas at the moment, though.
A few comments:
Any patch, even one this trivial, is easier to review if it comes with
a log message. You can get guidance on writing log messages (and on
other things) by reading the HACKING file. Another good thing is to
run 'svn log' and browse the output, to get a sense of the prevailing
style.
No big deal in this case, since we know what your patch is meant to
do, and it's pretty simple. On to the patch itself:
> ---- PATCH ----
> Index: delta.c
> ===================================================================
> --- delta.c (revision 8083)
> +++ delta.c (working copy)
> @@ -144,6 +144,11 @@
> svn_node_kind_t tgt_kind,
> apr_pool_t *pool);
>
> +static svn_error_t *check_authz_read_func_or_absent
> + (struct context *c, void *dir_baton,
> + const char *target_path, const char *edit_path,
> + svn_node_kind_t tgt_kind, apr_pool_t *pool);
> +
There shouldn't be any need for a prototype. Just define the function
before the first of its callers.
> static svn_error_t *delta_dirs (struct context *c,
> void *dir_baton,
> const char *source_path,
> @@ -794,13 +799,8 @@
> /* Sanity-check our input. */
> assert (target_path && edit_path);
>
> - if (c->authz_read_func)
> - {
> - SVN_ERR (c->authz_read_func (&allowed, c->target_root, target_path,
> - c->authz_read_baton, pool));
> - if (!allowed)
> - return absent_file_or_dir (c, dir_baton, edit_path, tgt_kind, pool);
> - }
> + SVN_ERR (check_authz_read_func_or_absent (c, dir_baton, target_path,
> + edit_path, tgt_kind, pool));
Hmmm. If I read this right, the caller no longer returns if the authz
check fails. Sure, check_authz_read_func_or_absent will correctly
invoke absent_file_or_dir, but then check_authz_read_func_or_absent's
caller will go on and do whatever it was going to do in the allowed
case as well!
That might be more change than you wanted to make :-).
> if (tgt_kind == svn_node_dir)
> {
> @@ -851,13 +851,8 @@
> /* Sanity-check our input. */
> assert (target_path && source_path && edit_path);
>
> - if (c->authz_read_func)
> - {
> - SVN_ERR (c->authz_read_func (&allowed, c->target_root, target_path,
> - c->authz_read_baton, pool));
> - if (!allowed)
> - return absent_file_or_dir (c, dir_baton, edit_path, tgt_kind, pool);
> - }
> + SVN_ERR (check_authz_read_func_or_absent (c, dir_baton, target_path,
> + edit_path, tgt_kind, pool));
Same here.
> +/* Check if we're authorized to edit @a target_path and indicate that it's
> + absent if we're not. */
> +static svn_error_t *check_authz_read_func_or_absent
> + (struct context *c, void *dir_baton,
> + const char *target_path, const char *edit_path,
> + svn_node_kind_t tgt_kind, apr_pool_t *pool);
> +{
There's a semicolon at the end of the parameter list, followed by a
curly brace. Does this compile for you? (My next question would have
been: did you test authz behaviors with your patch, using
mod_authz_svn? Given the semantic change noted above, I presume
things would act a little funny! :-) ).
Bravo for writing a doc string for the static function. But, it
should be as thorough as a public doc string. Name every parameter,
say what each means, be very specific about what you do if the path is
authorized or if it's not.
In the doc string, you didn't mean "edit", but rather "read", I presume?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 26 04:19:00 2003