subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko ─îibej <br...@wandisco.com>
Subject Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Date Wed, 24 Jun 2015 07:41:10 GMT
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

Mime
View raw message