> This is only a minor nit, just mentioning it first since the log
> message comes first: your log style is slightly different from most
> other log messages. I rewrote the message like this:
I'm still trying to get a handle on stylistic things with the project. I have
no objections to any of the log entry changes (and I'll try and match that style
> You put all of the code inside a conditional based on
> opt_state->owner_id. If someone just wants gid, then won't this fail?
In practice, I assumed it was unlikely that --group alone would ever be
desireable. Since setuid() requires root, all of the repository files would be
owned by root, and the assigned group. Is that ever a desirable state? The
idea (not apparently explicitely documented ;~) was that the options would be like:
svnadmin create [--owner ARG [--group ARG]]
so that --group only made sense in the context of --owner.
> + mode = 0770; /* mode is roughly !umask */
> did you really mean "~umask" not "!umask"?
> Don't use raw 'mkdir()', use apr_mkdir() or some svn_io_* function.
You mean apr_dir_make() don't you? Sorry, I didn't see that one for some reason.
> But there's a larger question. How does svn_repos_create() behave if
> the new directory already exists? Tracing it down to
> create_repos_dir(), it appears that it is okay as long as the new
> directory is empty. The doc string for svn_repos_create() should say
> this, however. Making that doc fix as part of your change is fine,
> since yours is the first code to depend explicitly on that aspect of
> the API. The doc change should specify what error will be returned if
> the directory is not empty.
Since create_repos_dir() already throws an error for a non-empty directory, I
didn't add that to svn_repos_create().
> Also, please keep lines under 80 columns -- even without all the extra
> indentation of the patch format and my quoting, that line above
> exceeds 80.
Sorry, I thought I had gotten all of those.
> Does this always result in the correct ownership and permissions being
> inherited by the subdirectories and files of the repository,
> especially the db/ subdir? Not asking because I'm suspicious, just
> ignorant. I assume you did testing and confirmed that it works as it
> should. Will it do so consistently across all Unix-like platforms,
> though? I'm not enough of an expert in this to say for sure.
My testing revealed that if the top level directory was not pre-created and
chown'd, then the inheritance would not follow (on FreeBSD at least). That's
why I chose to create an empty dir first.
However, since the process has setgid/setuid'd, as long as the top level
directory permit write access (which is does since I just created it), I am not
aware of any way for the child directories to _not_ be created appropriately.
My testing was not exhaustive (FreeBSD-4.x-STABLE, Mandrake 9.0, and Cobalt [a
mostly RedHatish abomination^Hdistro]).
Thanks for the review. Expect an updated patch soon...
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Lanham, MD 20706
To unsubscribe, e-mail: firstname.lastname@example.org
For additional commands, e-mail: email@example.com
Received on Mon Nov 24 19:11:32 2003