In light of Greg Hudson's comments, I'm going back to the drawing board.
Thank you for being gentle, and I'll make sure to do it right and test it if
I'm going to attempt this further.
Regards and happy holidays,
Philip Miller
kfogel@collab.net wrote:
> 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 :-).
Yes, indeed it is. I didn't compile and test before emailing the list. I
really should have.
>> 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
>
>
> ---------- Your email is protected by Mailshell ----------
> To block spam or change delivery options: http://www.mailshell.com/control.html?a=bsb95higoezmlcg0nqmqmlptvejqdnuytwnrt35o
>
> FreshAddress.com http://rd.mailshell.com/ad482
> Earn up to $3 for each of your friends who signs up with Mailshell! http://rd.mailshell.com/sp5
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 26 07:29:18 2003