kfogel@collab.net wrote:
> Also, instead of describing the implementation of
> svn_repos_authz_read(), maybe just say "If FILE is not a valid authz
> rule file, then return SVN_ERR_AUTHZ_INVALID_CONFIG, and the effect on
> *AUTHZP is undefined" or something like that.
Okay. I've corrected this docstring and also documented all functions
and structures in authz.c as you requested.
> Anyway, if we're going to depend on the behavior, then it should be
> documented all the way down.
After looking it up and discussing it, this behaviour was documented and
commited in r15447.
> is accurate. Too bad authz_group_contains_user() isn't documented,
> but that was true before you got here.
Partially true. It wasn't documented in mod_authz_svn, but as I ported
it into libsvn_repos in r15242, I should have documented it then. Corrected.
> This isn't really Dave's fault -- there are a lot of missing or
> inadequate (in the sense of not saying what each parameter does) doc
> strings in this file. However, let's try not to make the problem
> bigger.
See above. It really is my fault for not seeing to it in 15242 :-).
>>+ svn_config_get (b->config, &val, "groups",
>>+ &name[1], NULL);
>
> Any reason to wrap that line there?
The kwyglz people from Globulon IV told me to do it.
Okay, unwrapped.
>>+ /* Step through the entire rule file, aborting on error. */
>>+ svn_config_enumerate_sections (authz->cfg, authz_validate_section,
>>+ &baton);
>
> There now exists svn_config_enumerate_sections2(), the older
> function is deprecated.
This deprecation actually happened 36 revisions after the commit of my
patch. But I'll adjust my time machine to make sure this doesn't happen
again ;-)
> In most places, we use "_p" instead of just "p" as the suffix for
> return-by-reference pointers. As we discussed in IRC, you just
> happened to see some exceptional code (e.g., "configp") that led you
> to omit the underscore. No big deal, just noting it for next time.
Corrected.
> I don't think you meant "aborting on error", by the way. Returning an
> error is not the same as aborting.
Indeed. Also corrected.
>>- /* Work back to the parent path. */
>>- svn_path_split (current_path, ¤t_path, &base_name, pool);
>
> Since the call to authz_get_path_access() is used as the conditional
> to the 'while' loop, you could do away with base_name entirely (just
> pass NULL) and use current_path for the return check, no?
True. In my defense, I'd tweaked this loop around several times, and
missed the optimisation when it got into this shape. Corrected.
> Nice job upgrading the tests like that!
Not that nice it seems. svn-breakage reports the authz tests fail on
win32 because of the whole write-and-read-back thing in
authz_get_handle. Something to do with APR, stdio and windows being stupid.
Given the nature of the code and tests, it is probably safe to assume
that the actual tests would succeed if given the chance to run. Until I
finish integrating authz into svnserve and come back to kill the bug
that is.
Attached is the patch correcting the issues you found that are not
already corrected in trunk.
- Dave
[[[
Followup to r15400 with docstrings and small implementation fixes,
after review by kfogel.
Patch by: David Anderson <david.anderson@calixo.net>
* subversion/include/svn_repos.h
(svn_repos_authz_read): Rewrite the docstring to be less
implementation oriented.
* subversion/libsvn_repos/authz.c
(authz_lookup_baton, authz_validate_baton,
authz_group_contains_user):
Document.
(svn_authz_t): Remove repeat of the public API docstring. Document
implementation info only.
(authz_access_is_granted, authz_access_is_determined,
authz_group_walk, authz_validate_group, authz_validate_section):
Document properly.
(authz_validate_rule): Document properly. Change function parameter
name for clarity. Do not needlessly wrap line.
(svn_repos_authz_read): Change function parameter name for style
consistency. Correct implementation comment within the function.
(svn_repos_authz_check_access): Remove base_name and use
current_path in its stead.
]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 27 05:52:39 2005