Re: [PATCH] svnadmin create --owner --group (Third time's a charm???)

kfogel@collab.net wrote:

> 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
next time).

> 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"?

Yes, sorry.

>
> 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...

John

-- 
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD  20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org