From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 6 Feb 2025 07:53:58 +0000 (+0100)
Subject: reftable: ignore file-in-use errors when unlink(3p) fails on Windows
X-Git-Tag: v2.50.0-rc0~128^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3262a53c123844b3b9140f4e35b86f1444555aad;p=thirdparty%2Fgit.git

reftable: ignore file-in-use errors when unlink(3p) fails on Windows

Unlinking a file may fail on Windows systems when the file is still held
open by another process. This is incompatible with POSIX semantics and
by extension with Git's assumed semantics when unlinking files, which
is that files can be unlinked regardless of whether they are still open
or not. To counteract this incompatibility, we have some custom error
handling in the `mingw_unlink()` wrapper that first retries the deletion
with some delay, and then asks the user whether we should continue to
retry.

While this logic might be sensible in many callsites throughout Git, it
is less when used in the reftable library. We only use unlink(3) there
to delete tables which aren't referenced anymore, and the code is very
aware of the limitations on Windows. As such, all calls to unlink(3p)
don't perform any error checking at all and are fine with the call
failing.

Instead, the library provides the `reftable_stack_clean()` function,
which Git knows to execute in git-pack-refs(1) after compacting a stack.
The effect of this function is that all stale tables will eventually get
deleted once they aren't kept open anymore.

So while we're fine with unlink(3p) failing, the Windows-emulation of
that function will still perform several sleeps and ultimately end up
asking the user:

    $ git pack-refs
    Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n
    Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n
    Unlink of file 'C:/temp/jgittest/jgit/.git/reftable/0x000000000002-0x000000000004-50486d0e.ref' failed. Should I try again? (y/n) n

It even asks multiple times, which is doubly annoying and puzzling to
the user:

  1. It asks when trying to delete the old file after having written the
     compacted stack.

  2. It asks when reloading the stack, where it will try to unlink
     now-unreferenced tables.

  3. It asks when calling `reftable_stack_clean()`, where it will try to
     unlink now-stale tables.

Fix the issue by making it possible to disable this behaviour with a
preprocessor define. As "git-compat-util.h" is only included from
"system.h", and given that "system.h" is only ever included by headers
and code that are internal to the reftable library, we can set that
macro in this header without impacting anything else but the reftable
library.

Reported-by: Christian Reich <Zottelbart@t-online.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
index 8dddfa818d..88e0cf9292 100644
--- a/compat/mingw-posix.h
+++ b/compat/mingw-posix.h
@@ -201,8 +201,12 @@ int uname(struct utsname *buf);
  * replacements of existing functions
  */
 
-int mingw_unlink(const char *pathname);
-#define unlink mingw_unlink
+int mingw_unlink(const char *pathname, int handle_in_use_error);
+#ifdef MINGW_DONT_HANDLE_IN_USE_ERROR
+# define unlink(path) mingw_unlink(path, 0)
+#else
+# define unlink(path) mingw_unlink(path, 1)
+#endif
 
 int mingw_rmdir(const char *path);
 #define rmdir mingw_rmdir
diff --git a/compat/mingw.c b/compat/mingw.c
index 1d5b211b54..0e4b6a70a4 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -302,7 +302,7 @@ static wchar_t *normalize_ntpath(wchar_t *wbuf)
 	return wbuf;
 }
 
-int mingw_unlink(const char *pathname)
+int mingw_unlink(const char *pathname, int handle_in_use_error)
 {
 	int ret, tries = 0;
 	wchar_t wpathname[MAX_PATH];
@@ -317,6 +317,9 @@ int mingw_unlink(const char *pathname)
 	while ((ret = _wunlink(wpathname)) == -1 && tries < ARRAY_SIZE(delay)) {
 		if (!is_file_in_use_error(GetLastError()))
 			break;
+		if (!handle_in_use_error)
+			return ret;
+
 		/*
 		 * We assume that some other process had the source or
 		 * destination file open at the wrong moment and retry.
diff --git a/reftable/system.h b/reftable/system.h
index 10055fbff2..072d9daea0 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -11,6 +11,7 @@ https://developers.google.com/open-source/licenses/bsd
 
 /* This header glues the reftable library to the rest of Git */
 
+#define MINGW_DONT_HANDLE_IN_USE_ERROR
 #include "compat/posix.h"
 #include "compat/zlib-compat.h"