From: Christian Reich Date: Fri, 14 Mar 2025 14:18:55 GMT Subject: Re: [PATCH v2] reftable: ignore file-in-use errors when unlink(3p) fails on Windows Message-ID: In-Reply-To: <20250206-b4-pks-reftable-win32-in-use-errors-v2-1-56985a4f6186@pks.im> Greetings, the patch didn't find the way in next git version. But there is a change in git for windows, which prevents multiple asks the same questions: https://github.com/git-for-windows/git/commit/dba1473162b4319a0a2c7f74ab48ed4a826f7ac7 The other option to prevent the asks, is to set GIT_ASK_YESNO but the sleeps slow down the process. Best Regards Christian Am 06.02.2025 um 08:53 schrieb Patrick Steinhardt: > 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 > Signed-off-by: Patrick Steinhardt > --- > 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]: > --- > 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 > > > --- > base-commit: 0fb5c2116049c665c6550d7e0419971a277af345 > change-id: 20250124-b4-pks-reftable-win32-in-use-errors-969494f2fdf7 >