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.
-- Brane
Received on 2015-06-24 09:41:42 CEST