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

Re: [PATCH] Move authz access control to libsvn_repos

From: David Anderson <david.anderson_at_calixo.net>
Date: 2005-07-03 03:36:38 CEST

> 1. You're using tabs. We don't use tabs in the Subversion code base

Oops, my bad. Badly configured emacs. Should be resolved in this resubmit.

> 2. The access types enum should be in svn_repos.h, not svn_types.h.
> There's no rule that says all types are defined in svn_types.h,
> despite the name.

Moved. When should types be defined in svn_types.h ?

> 3. I'm dubious about AUTHZ_ACCESS_GRANTED and AUTHZ_ACCESS_DETERMINED
> being defined as macros.

Changed to functions. The code is a little more spread out in the
functions; that helps readability.

> 4. [...] I think introducing a no-space-before-paren file into
> libsvn_repos would generate a lot of confusion.

Changed to space-before-paren.

> 5. I can't tell what changes you made between the old mod_authz_svn
> code and the new libsvn_repos code. Clearly there are some
> substantial changes, since the macros I mentioned in (3) above don't
> exist in the old code. Your log message should provide some
> guidance.

As stated in the first sentence of the commit log message, I reworked
the code to attempt to get it to fit with the svn coding style. This
implied a few variable renamings, renaming the functions to fit in the
svn namespace, commenting the code, etc. . Aside from the "cosmetic"
changes, the two macros are the only logical change I can recall.

The macros aren't in mod_authz_svn because the expansion is directly
used in a few places. I found this confusing (and spent a fair amount of
time working out why the hell they did what they did), and so moved it
to documented macros with a nice names. Well, documented functions now.

How can I explain all this in the commit message? I thought that for the
purposes of the commit message stating that the functionality has moved
and has been globally revamped to fit in with the destination was
sufficient; the comments in the new file are, I think, in sufficient
number to clear up any residual problems.

Anyhow, here's another attempt at the commit message, along with the
shiny, corrected patch.

[[[
Move authz routines from mod_authz_svn to libsvn_repos; rework authz
code to fit in with libsvn_repos; update mod_authz_svn to reflect
the move.

* subversion/mod_authz_svn/mod_authz_svn.c: Add includes, remove
     nameless enum. Update copyright notice.
   (parse_authz_baton, group_contains_user_internal,
    group_contains_user, parse_authz_line,
    parse_authz_lines, parse_authz_section,
    parse_authz_sections, check_access): Remove, functionality
     moves to libsvn_repos.
   (req_check_access): Use the authz routines in libsvn_repos and wrap
     any errors reported by the authz layer.

* subversion/include/svn_repos.h: Add include. Update copyright notice.
   (svn_authz_access_kind_t): New enum type.
   (svn_repos_authz_check_access): New public API.

* subversion/libsvn_repos/authz.c: New file. Contains the functionality
     moved from mod_authz_svn. Rename functions and variable names to
     better fit libsvn_repos. Add internal API and in-function
     documentation.

* subversion/tests/libsvn_repos/repos-test.c: Add includes. Update
     copyright notice.
   (authz): New test.
]]]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Received on Sun Jul 3 03:37:57 2005

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