Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (nike.apache.org: domain of brane@wandisco.com designates
 209.85.212.178 as permitted sender)
Message-ID: <558A5F16.7080700@wandisco.com>
Date: Wed, 24 Jun 2015 09:41:10 +0200
From: =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <brane@wandisco.com>
Organization: WANdisco
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10;
 rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To: dev@subversion.apache.org
Subject: Re: svn commit: r1682265 -
 /subversion/trunk/subversion/libsvn_fs_fs/util.c
References: <20150528154555.D1D0BAC06F9@hades.apache.org>
 <CABw-3YerzerVqW1CdYYm8vbWRuKy3-EAKrhmO=xb7OY=S8ETzw@mail.gmail.com>
 <CA+t0gk3yQm+nYgH4C_Lu=fGMFShojEiwbZ05rb=CnhoHFpv5DQ@mail.gmail.com>
 <CABw-3YehnUheqHgq9PbLPyh65xdYQWDXvahurfW1KOTkQmc5nA@mail.gmail.com>
 <CABw-3YecLjcdd2pc=jc38M3eJT2-XUFJWsH6wLYXSFphn-ip1w@mail.gmail.com>
In-Reply-To: 
 <CABw-3YecLjcdd2pc=jc38M3eJT2-XUFJWsH6wLYXSFphn-ip1w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

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