On Sat, Jan 25, 2025 at 09:19:57AM +0100, Johannes Sixt wrote:
> 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
