Re: [PATCH] refs: work around network caching on Windows

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Eric,

On Fri, 15 Jul 2022, Eric Sunshine wrote:

> On Fri, Jul 15, 2022 at 4:18 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Network shares sometimes use aggressive caching, in which case a
> > just-created directory might not be immediately visible to Git.
> >
> > One symptom of this scenario is the following error:
> >
> >         $ git tag -a -m "automatic tag creation"  test_dir/test_tag
> >         fatal: cannot lock ref 'refs/tags/test_dir/test_tag': unable to resolve reference 'refs/tags/test_dir/test_tag': Not a directory
> >
> > Note: This does not necessarily happen in all Windows setups. One setup
> > where it _did_ happen is a Windows Server 2019 VM, and as hinted in
> >
> >         http://woshub.com/slow-network-shared-folder-refresh-windows-server/
> >
> > the following commands worked around it:
> >
> >         Set-SmbClientConfiguration -DirectoryCacheLifetime 0
> >         Set-SmbClientConfiguration -FileInfoCacheLifetime 0
> >         Set-SmbClientConfiguration -FileNotFoundCacheLifetime 0
> >
> > This would impact performance negatively, though, as it essentially
> > turns off all caching, therefore we do not want to require users to do
> > that just to be able to use Git on Windows.
> >
> > The underlying culprit is that `GetFileAttributesExW()` that is called from
> > `mingw_lstat()` can raise an error `ERROR_PATH_NOT_FOUND`, which is
> > translated to `ENOTDIR`, as opposed to `ENOENT` as expected on Linux.
> >
> > Therefore, when trying to read a ref, let's allow `ENOTDIR` in addition
> > to `ENOENT` to indicate that said ref is missing.
> >
> > This fixes https://github.com/git-for-windows/git/issues/3727
> >
> > Signed-off-by: Pierre Garnier <pgarnier@mega.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > @@ -381,7 +381,7 @@ stat_ref:
> > -               if (myerr != ENOENT || skip_packed_refs)
> > +               if ((myerr != ENOENT && myerr != ENOTDIR) || skip_packed_refs)
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > @@ -480,7 +480,7 @@ static int load_contents(struct snapshot *snapshot)
> > -               if (errno == ENOENT) {
> > +               if (errno == ENOENT || errno == ENOTDIR) {
>
> The first question which popped into my mind upon reading the patch
> was why these changes need to be made to files-backend.c and
> packed-backend.c rather than "fixing" mingw_lstat() to return ENOENT
> instead of ENOTDIR.

I already had started crafting a mail to explain that I do not want to
take the risk of changing the code to map `ERROR_PATH_NOT_FOUND` to
`ENOENT` instead of `ENOTDIR`, as we only have one central function to map
Windows' error codes to POSIX `errno` values. It would therefore affect
many more code paths than just `mingw_lstat()`.

Can you imagine my surprise when I looked up the link to that mapping
function so that I could paste it in my reply to make my point, only to
see that that error is _already_ mapped to `ENOENT`? Look here for
yourself: https://github.com/git/git/blob/v2.37.1/compat/mingw.c#L121

So where is the bug? It is somewhere completely different:
https://github.com/git/git/blob/v2.37.1/compat/mingw.c#L847-L851

Essentially, Windows does not give us the equivalent of POSIX' `ENOTDIR`:
For something like `C:\a\b\c` we only get `ERROR_PATH_NOT_FOUND`, no
matter whether `C:\a` is missing or whether it is a file (POSIX would want
an `ENOENT` in the former and `ENOTDIR` in the latter case, and
unfortunately Git fully relies on these POSIX semantics).

In 4b0abd5c695 (mingw: let lstat() fail with errno == ENOTDIR when
appropriate, 2016-01-26), we introduced code to emulate POSIX semantics:
we laboriously look through the path components to see whether there is
anything in the way that would prevent us from creating the parent
directory.

It must be somewhere in that emulation where things go wrong, still. I
asked Pierre to debug a bit more to get to the bottom of the problem.

So: thank you for your "silly" question, it did point to the need for more
investigation.

Ciao,
Dscho