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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 25, 2025, 08:32 UTC
Message-ID: <Z5ShoLCWREAsWZVh@pks.im>
In-Reply-To: <6acb9e8a-7014-4605-b8af-59c4584e9fe6@kdbg.org>

On Sat, Jan 25, 2025 at 09:19:57AM +0100, Johannes Sixt wrote:

Show 20 quoted lines

> Am 25.01.25 um 06:41 schrieb Patrick Steinhardt:
> > 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:
> 
> Why don't the changes that your commits ending at 391bceae4350
> ("compat/mingw: support POSIX semantics for atomic renames", 2024-10-27)
> help in this case, too?

The user report was explicitly about compatibility with JGit, which still had these files open. We don't have control over third-party clients and how exactly they open files, so it is expected that we may still see failures with the deletion of in-use files.

> Since the reftable layer is aware of the problem, why don't we just fix
> it there and instead sweep it under the rug in the compat layer?

I didn't really have a good idea for _how_ to do that. We automatically pull in the compat version of `unlink()` via "git-compat-util.h", and we cannot easily change that. So the reftable library is basically unable to handle it there, because the assumed POSIX-behavior of `unlink()` is different. I also don't want to reimplement the "compat/" layer for Windows, because it brings us a couple of features for free, like wide-character handling and handling the deletion of read-only files.

Another alternative would be to provide `reftable_unlink()` in the reftable system layer, implement it via `mingw_unlink()` with that second argument I'm introducing and then convert all callsites of unlink to use that function instead. But that felt unnecessarily complex to me.

I'd be happy to hear about alternative ideas that didn't came to my mind.

Patrick