On 2/10/06, Garrett Rooney <rooneg@electricjellyfish.net> wrote:
[snip]
> Well, it depends. 'incomplete' means you have to look at the 'valid'
> member and see if the parts you care about are valid or not, IIRC.
> But yes, the reason this set of patches haven't been committed (in the
> case of this patch) and merged back to the older branches (in the case
> of the already committed rev in question) is that nobody has done the
> leg work on it. If you're willing to do the work of confirming that
> the changes work correctly and makes sense (i.e. makes the code do
> roughly the same thing on windows as it does on Unix) I'd be happy to
> commit the changes.
>
Ok, this APR fix is quite straightforward and looks like an oversight
in the original code, really -- without it the function doesn't bother
to get the permissions (unless you also happen ask for the user/group
info) and so it will always return 'incomplete'.
Now, although this fixes these functions in APR on Windows, Subversion
seems to have a different problem when it comes to setting files
readonly or read-write. It seems to be assuming that the file
attribute 'readonly' is the same as permissions. On Windows NTFS
filesystems they are not the same (the readonly attribute is
independent of the real permissions), but they *are* the same on FAT
filesystems and Unix, I think, right?
So even though it appears that APR tries to maintain the distinction
with different functions for attributes, Subversion is not (or at
least not completely) following that distinction and so when it finds
that the permissions on source and dest files are the same in
svn_io_set_file_read_write_carefully, it returns early and doesn't
actually change the readonly attribute. Which causes troubles with
svn:needs-lock, at least.
Does that sound correct? File attributes and file permissions are
distinct on Windows NTFS, but not really on Unix? And even though APR
has separate functions to read/manipulate each, Subversion is kind of
munging them together?
It used to sort of work in old APRs because the calls to get security
failed on Win32 (because the file wasn't opened with READ_CONTROL
access) and so it called guess_protection_bits which just looks at the
readonly file attribute and makes up some permissions based on that.
Okay, that is confusing even to me as the writer. Sooo, here is what
I think happened:
1. Old APR was never reading file permissions on Win32 -- it just
guessed based on the readonly file attribute.
2. Subversion treats readonly-ness mostly based on permissions
(svn_io_set_file_read_write_carefully) but since APR is only looking
at the file attribute, Subversion is fooled into thinking it is
behaving correctly on Win32.
3. New APR is semi-fixed to pass READ_CONTROL on open so that it can
query permissions, but it doesn't actually query the permissions
causing 'incomplete' return codes.
4. With the patch for new APR, permissions are queryed and returned
successfully.
5. Even with the patch for new APR, Subversion is still semi-broken
in regards to setting the readonly file attribute on Win32 because it
is looking at permissions -- and the user's permissions are the same,
even if the readonly file attribute is not, so it never changes the
readonly attribute.
6. I guess Subversion needs to be fixed to look at/change the
readonly file attribute even if permissions are the same (in
svn_io_set_file_read_write_carefully)?
I hope that makes sense...
DJ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 10 21:37:28 2006