[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: rev 8081 - trunk/subversion/libsvn_repos

From: <kfogel_at_collab.net>
Date: 2003-12-26 02:27:27 CET

ghudson@tigris.org writes:
> +/* If authorized, emit a delta to create the entry named TARGET_ENTRY
> + at the location EDIT_PATH. If not authorized, indicate that
> + EDIT_PATH is absent. Pass DIR_BATON through to editor functions
> + that require it. */
> static svn_error_t *
> add_file_or_dir (struct context *c, void *dir_baton,
> const char *target_path,
> @@ -835,10 +789,19 @@
> apr_pool_t *pool)
> {
> struct context *context = c;
> + svn_boolean_t allowed;
>
> /* 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);
> + }

I'd declare 'allowed' inside the if's body. Right now, a reader can
easily fear that 'allowed' might be used uninitialized later. Plus,
it's slightly easier to add code later which *does* use it
uninitialized, if one doesn't read the conditional structure
carefully.

I know it's not a matter of correctness, and anyone adding code
*should* read the conditionals carefully, of course. It's just nice
when code doesn't raise unnecessary questions/worries that then have
to be quelled.

(Yes, I remember we talked about these two styles on IRC :-) ).

> +/* If authorized, emit a delta to modify EDIT_PATH with the changes
> + from SOURCE_PATH to TARGET_PATH. If not authorized, indicate that
> + EDIT_PATH is absent. Pass DIR_BATON through to editor functions
> + that require it. */
> static svn_error_t *
> replace_file_or_dir (struct context *c,
> void *dir_baton,
> @@ -882,10 +846,19 @@
> apr_pool_t *pool)
> {
> svn_revnum_t base_revision = SVN_INVALID_REVNUM;
> + svn_boolean_t allowed;
>
> /* 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);
> + }

Same comment applies here.

+1 on the change as a whole, though! This factorization feels very
right.

-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 03:18:39 2003

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.