[PATCH] compat/mingw.c: Teach mingw_rename() to replace read-only files

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Nov 19, 2008, 16:25 UTC
Message-ID: <49243DF7.7000604@viscovery.net>
In-Reply-To: <49242821.50604@viscovery.net>

From: Johannes Sixt <j6t@kdbg.org>

On POSIX, rename() can replace files that are not writable. On Windows, however, read-only files cannot be replaced without additional efforts: We have to make the destination writable first.

Since the situations where the destination is read-only are rare, we do not make the destination writable on every invocation, but only if the first try to rename a file failed with an "access denied" error.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Johannes Sixt schrieb:

Show 12 quoted lines

> Nicolas Pitre schrieb:
>> On Wed, 19 Nov 2008, Johannes Sixt wrote:
>>> The unusual case is where you do this:
>>>
>>>  $ git rev-list -10 HEAD | git pack-objects foobar
>>>
>>> twice in a row: In this case the second invocation fails on Windows
>>> because the destination pack file already exists *and* is open. But not
>>> even git-repack does this even if it is called twice. OTOH, the test case
>>> *does* exactly this.
>> OK.... Well, despite my earlier assertion, I think the above should be a 
>> valid operation.

Would you please clarify which operation you exactly mean? As is written above, or with .git/objects/pack/foobar instead like in the test case?

Show 5 quoted lines

>> I'm looking at it now.  I'm therefore revoking my earlier ACK as well 
>> (better keep that test case alive).
> 
> Hold on a moment: When I tested the above sequence, I was fooled by a flaw
> in mingw_rename() (it doesn't replace read-only files). With that fixed...

Here is this fix.

-- Hannes

 compat/mingw.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index b534a8a..3dbe6a7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -819,6 +819,8 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
+	DWORD attrs;
+
 	/*
 	 * Try native rename() first to get errno right.
 	 * It is based on MoveFile(), which cannot overwrite existing files.
@@ -830,12 +832,19 @@ int mingw_rename(const char *pold, const char *pnew)
 	if (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))
 		return 0;
 	/* TODO: translate more errors */
-	if (GetLastError() == ERROR_ACCESS_DENIED) {
-		DWORD attrs = GetFileAttributes(pnew);
-		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY)) {
+	if (GetLastError() == ERROR_ACCESS_DENIED &&
+	    (attrs = GetFileAttributes(pnew)) != INVALID_FILE_ATTRIBUTES) {
+		if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
 			errno = EISDIR;
 			return -1;
 		}
+		if ((attrs & FILE_ATTRIBUTE_READONLY) &&
+		    SetFileAttributes(pnew, attrs & ~FILE_ATTRIBUTE_READONLY)) {
+			if (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))
+				return 0;
+			/* revert file attributes on failure */
+			SetFileAttributes(pnew, attrs);
+		}
 	}
 	errno = EACCES;
 	return -1;

-- 
1.6.0.4.1694.g07ac