On 24.06.2015 09:41, Branko Čibej wrote:
> On 23.06.2015 19:29, Ivan Zhakov wrote:
>> I've prepared a patch that fixes svn_io_file_rename() to consistently
>> return error for cross volume renames on Windows using platform
>> specific code. See svn-rename-no-copy-allowed-v1.patch.txt.
>> Review/testing will be really appreciated since it contains platform
>> specific code.
>>
>> While I think that we should generally avoid platform specific code,
>> in this case using the native API gives us an opportunity to use
>> MOVEFILE_WRITE_THROUGH flag for MoveFileEx later to avoid potential
>> problems on network shares and non-NTFS filesystems.
>>
>> I've also prepared patch for APR that changes apr_file_rename()
>> behavior to fail for cross-volume renames on Windows (see
>> apr-file-rename-no-copy-allowed-v1.patch), but I'm not sure that it
>> could be committed to APR (and backported to release branches) since
>> it changes behavior and some APR users may depend on current behavior.
>> Index: subversion/libsvn_subr/io.c
>> =======================================
>> --- subversion/libsvn_subr/io.c (revision 1687052)
>> +++ subversion/libsvn_subr/io.c (working copy)
>> @@ -4024,7 +4024,25 @@
>> return SVN_NO_ERROR;
>> }
>> +#if defined(WIN32)
>> +/* Compatibility wrapper around apr_file_rename() to workaround
>> + APR problems on Windows. */
> This isn't really a wrapper for apr_file_rename at all, is it?
>
>> +static apr_status_t
>> +win32_file_rename(const WCHAR *from_path_w,
>> + const WCHAR *to_path_w,
>> + apr_pool_t *pool)
>> +{
>> + /* APR calls MoveFileExW() with MOVEFILE_COPY_ALLOWED, while we rely
>> + * that rename is atomic operation. Use MoveFileEx directly on Windows
>> + * without MOVEFILE_COPY_ALLOWED flag to workaround it.
>> + */
>> + if (!MoveFileExW(from_path_w, to_path_w, MOVEFILE_REPLACE_EXISTING))
>> + return apr_get_os_error();
>> + return APR_SUCCESS;
>> +}
>> +#endif
>> +
> The rest of the patch looks good. I wonder though if we shouldn't just
> rip out OS/2-specific bits ... is that thing still alive and is it
> remotely possible that someone's running Subversion on OS/2?
>
>
> Regarding the APR patch: IMO it should go into APR 2.0 (that's trunk)
> but, as you say, can't be backported to the 1.x series.
Actually: we could revise the function, similarly to what we do in the
SVN API; for example, write apr_file_move that takes a boolean parameter
that says whether or not the move should be guaranteed to be atomic, and
rewrite apr_file_rename in as a wrapper to that: this new function could
go into APR trunk (2.0) and onto the 1.6.x branch.
But this is a bit more work because we'd need implementations not just
for Windows but also for Unix and possibly Netware.
-- Brane
