D.J. Heap wrote:
> Any objections to the attached patch? Its a step toward supporting
> Apache 2.2/APR 1.x on Win32.
[First of all, I must apologise to you for dropping the ball on solving
this problem. What follows is not intended as a criticism of your patch,
but a discussion of the perms-modifying code in general.]
IMNSHO, there is not a single logical reason to keep this function in
the code, whether we call it blah_blah_carefully or blah_blah_perms.
Originally, we had svn_io_set_file_read_write and
svn_io_set_file_read_only, which called apr_file_attrs_set, and were
used only on the files within the .svn/ directory tree. Because the
umask(2) semantics are slightly broken (i.e., there's no way to figure
out the current process' umask without changing it, and it _is_ a
process-global parameter), apr_file_attrs_set doesn't take the current
umask into account. As a result, svn_io_set_file_read_write would make a
file globally writable on Unix-like systems. This was deemed acceptable
for working-copy-metadata files.
At about the same time (historically speaking :) we decided to start
fiddling with the executable bit. Obviously, what apr_file_attrs_set
does was no longer acceptable, so svn_io_set_file_executable jumps
through hoops in order to figure out what the umask might be relatively
safely. Those hoops don't exist on Windows, but that didn't matter,
because we ignore svn:executable there anyway.
Then along came locking support and the need to manipulate the write
protection bits on "real" working-copy files. The current semantics of
svn_io_set_file_read_(write|only) were no longer sufficient, so along
came svn_io_set_file_read_write_carefully, which is a copy-paste clone
of svn_io_set_file_executable, but tweaked to handle the write perms
instead of execute perms. (I won't go into the copy-paset part here,
grrr, since it's as much my fault as anyone else's for not noticing that
during commit review.)
The real trouble is that this new function had the exact semantics that
svn_io_set_file_read_(write|only) should have had from the beginning.
But, instead of it being a private implementation of those semantics for
Unix only, it's a public API that's only used in the locking-support
code, while other parts of the WC-management code still uses the older
two functions. The fact that it breaks on Windows with newer versions of
APR is only a minor side-effect at this time.
Now here's my proposal for remedying this situation:
1. Create a *private*, Unix-specific function in libsvn_subr that does
the jumping-through-hoops that svn_io_set_file_executable and
svn_io_set_file_read_write_carefully do now.
2. Reimplement svn_io_set_file_executable, svn_io_set_file_read_write
and svn_io_set_file_read_only so that, on Unix, they call this new
hoop-jumping function. On Windows, they should either do nothing (as
...set_file_executable does now) or call apr_file_attrs_set.
3. Deprecate svn_io_set_file_read_write_carefully and reimplement it as
a wrapper for svn_io_set_file_read_(write|only).
Yes, this would cause a somewhat larger overhead in WC metadata
management on Unix. On the other hand, we'd really only have to jump
through hoops to figure out the current umask once per WC, and could
cache the result. That's a low-priority optimisation though, cleaning up
the API (and certainly supporting httpd-2.2 on Windows) is a higher
priority.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jun 3 23:40:35 2006