Re: [PATCH v2] 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>
> ---
> Hi,
>
> This patch fixes the issue reported in [1].
>
> Changes in v2:
>    - Rebased the patch on top of ps/reftable-sans-compat-util at
>      3f172f1391 (Makefile: skip reftable library for Coccinelle,
>      2025-02-03). This is done to fix a semantic merge conflict.
>    - Link to v1: https://lore.kernel.org/r/20250125-b4-pks-reftable-win32-in-use-errors-v1-1-356dbc783b4f@pks.im
>
> Thanks!
>
> Patrick
>
> [1]: <d7fd0b1c-98fe-4cc3-b657-c2c3d0bc5c47@t-online.de>
> ---
>   compat/mingw/compat-util.c | 5 ++++-
>   compat/mingw/posix.h       | 8 ++++++--
>   reftable/system.h          | 1 +
>   3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/compat/mingw/compat-util.c b/compat/mingw/compat-util.c
> index 1d5b211b54..0e4b6a70a4 100644
> --- a/compat/mingw/compat-util.c
> +++ b/compat/mingw/compat-util.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/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/reftable/system.h b/reftable/system.h
> index dccdf11f76..1492bf6d70 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at
>   
>   /* This header glues the reftable library to the rest of Git */
>   
> +#define MINGW_DONT_HANDLE_IN_USE_ERROR
>   #include "../compat/posix.h"
>   #include <zlib.h>
>   
>
> ---
> base-commit: 0fb5c2116049c665c6550d7e0419971a277af345
> change-id: 20250124-b4-pks-reftable-win32-in-use-errors-969494f2fdf7
>