You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Evgeny Kotkov <ev...@visualsvn.com> on 2019/02/21 22:37:56 UTC
Re: svn commit: r1854072 - in /subversion/trunk/subversion:
libsvn_subr/io.c tests/libsvn_subr/io-test.c
Branko Čibej <br...@apache.org> 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.
Thanks,
Evgeny Kotkov
Re: svn commit: r1854072 - in /subversion/trunk/subversion:
libsvn_subr/io.c tests/libsvn_subr/io-test.c
Posted by Branko Čibej <br...@apache.org>.
On 21.02.2019 23:37, Evgeny Kotkov wrote:
> Branko Čibej <br...@apache.org> 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.
Committed, with tweaks, in r1854216.
-- Brane
Re: svn commit: r1854072 - in /subversion/trunk/subversion:
libsvn_subr/io.c tests/libsvn_subr/io-test.c
Posted by Erik Huelsmann <eh...@gmail.com>.
>
> 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.
>
Maybe not tested, but there are supposedly floating OS/2 binaries around:
https://os2ports.smedley.id.au/index.php?page=subversion
Lacking an OS/2 installation, I have no idea if they actually work...
>
> -- Brane
>
>
Regards,
--
Bye,
Erik.
http://efficito.com -- Hosted accounting and ERP.
Robust and Flexible. No vendor lock-in.
Re: svn commit: r1854072 - in /subversion/trunk/subversion:
libsvn_subr/io.c tests/libsvn_subr/io-test.c
Posted by Branko Čibej <br...@apache.org>.
On 21.02.2019 23:37, Evgeny Kotkov wrote:
> Branko Čibej <br...@apache.org> 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