Hi Patrick, On Thu, 6 Feb 2025, Patrick Steinhardt wrote: > 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@xxxxxxxxxxx> > Signed-off-by: Patrick Steinhardt <ps@xxxxxx> > --- > 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@xxxxxx > > Thanks! > > Patrick > > [1]: <d7fd0b1c-98fe-4cc3-b657-c2c3d0bc5c47@xxxxxxxxxxx> > --- > 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> It's the pragmatic thing to do. A cleaner fix would be to introduce a proper function that says in its name that it tries to delete a file on a best-effort basis but won't insist if that's not possible at the time. As I have communicated with you on similar issues in the past, my perspective is that Git fails to abstract operating system-specific functionality correctly, and instead (ab-)uses POSIX/libc functions trying to do so. This is one of the things Subversion still does better. That's why we now have the unfortunate need to change the function signature of `mingw_unlink()` to something distinctly unlike `unlink()` (which `mingw_unlink()` originally clearly tried to emulate in an effort to pretend to Git that it's okay to assume that every operating system behaves like Linux). What strikes me as even less desirable is to _still_ keep the function signature of `unlink()` the same, changing instead the behavior per-file via that macro definition. Unfortunately, the cleaner fix would require to undo years of working around the lack of a proper platform abstraction layer, which would not only be a huge amount of effort, but once again cause a lot of downstream pain in Git for Windows, therefore I would actually oppose such an effort. tl;dr I will live with this patch. Ciao, Johannes > > > --- > base-commit: 0fb5c2116049c665c6550d7e0419971a277af345 > change-id: 20250124-b4-pks-reftable-win32-in-use-errors-969494f2fdf7 > >
