Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c

On 24 June 2015 at 10:41, Branko Čibej <brane_at_wandisco.com> 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?
>
You're right: I didn't updated doc-string from previous approach.

>> +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?
>
Ripping OS/2 specific code is unrelated and I wanted to keep code for
other platform unchanged, since I don't have OS/2 build environment :)

Thanks for review. I've committed slightly fixed patch in r1687583.

-- 
Ivan Zhakov