On 2/10/06, Garrett Rooney <[EMAIL PROTECTED]> 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
