On 21.02.2019 23:37, Evgeny Kotkov wrote: > Branko Čibej <[email protected]> writes: > >> @@ -4493,6 +4571,12 @@ svn_io_dir_remove_nonrecursive(const cha >> >> SVN_ERR(cstring_from_utf8(&dirname_apr, dirname, pool)); >> >> + /* On Windows, a read-only directory cannot be removed. */ >> +#if defined(WIN32) || defined(__OS2__) >> + SVN_ERR(io_set_readonly_flag(dirname_apr, dirname, >> + FALSE, FALSE, FALSE, pool)); >> +#endif >> + >> status = apr_dir_remove(dirname_apr, pool); > Would it be feasible for us to only attempt to remove the read-only > attribute after receiving an error? (along the lines of the attached patch) > > The reason is that getting and setting file attributes usually results in > CreateFile() syscalls, whereas opening files and the I/O-related syscalls are > not cheap on Win32 in general. So changing the directory attributes per every > remove could increase the amount of I/O operations and maybe slow down the > cases where we have to remove multiple directories, with the post-commit > transaction directory cleanup being an example of such case.
If your patch works, then just commit it; io-test.exe covers these cases now. I agree it's better to try to remove the directory first (something we can't do on Unix, where we need a writable directory in order to delete its children). Please update the backport proposals, too. By the way, I'm not sure why we carry around the "defined(__OS2__)" check in io.c. As far as I'm aware, no-one has ever actually tested Subversion on OS/2 ... these checks are probably just lifted out of APR, but don't do anything useful. -- Brane
