[PATCH] refs: work around network caching on Windows

[PATCH] refs: work around network caching on Windows

[Johannes Schindelin via GitGitGadget]

From: Pierre Garnier <pgarnier@mega.com>

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>
---
    refs: work around network caching on Windows
    
    While it is enough on Linux to look at ENOENT when allowing for missing
    refs, on Windows we also need to allow ENOTDIR.
    
    This fixes https://github.com/git-for-windows/git/issues/3727

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1291%2Fdscho%2Fenotdir-and-enoent-can-indicate-missing-refs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1291/dscho/enotdir-and-enoent-can-indicate-missing-refs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1291

 refs/files-backend.c  | 2 +-
 refs/packed-backend.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8db7882aacb..b2a880f62f0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -381,7 +381,7 @@ stat_ref:
 	if (lstat(path, &st) < 0) {
 		int ignore_errno;
 		myerr = errno;
-		if (myerr != ENOENT || skip_packed_refs)
+		if ((myerr != ENOENT && myerr != ENOTDIR) || skip_packed_refs)
 			goto out;
 		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
 				      referent, type, &ignore_errno)) {
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 97b68377673..23d478627a7 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -480,7 +480,7 @@ static int load_contents(struct snapshot *snapshot)
 
 	fd = open(snapshot->refs->path, O_RDONLY);
 	if (fd < 0) {
-		if (errno == ENOENT) {
+		if (errno == ENOENT || errno == ENOTDIR) {
 			/*
 			 * This is OK; it just means that no
 			 * "packed-refs" file has been written yet,

base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
-- 
gitgitgadget

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

[Eric Sunshine]

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. Patching mingw_lstat() seems more tractable and
less likely to lead to discovery of other code in the future which
needs to be patched in a similar way to how files-backend.c and
packed-backend.c are being patched here.

Perhaps it's a silly question and the answer is perfectly obvious to
folks directly involved in Git development on Windows, but the commit
message doesn't seem to answer it for people who don't have such
inside knowledge.

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

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 15 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Pierre Garnier <pgarnier@mega.com>
>
> 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

This really has much wider implications, as we hard depend on POSIX
semantics in various other places. E.g. we'll the SHA-1 collision
detection sanity check (not sha1dc, the "does it exist?") would be racy
on such a system, wouldn't it?

>  refs/files-backend.c  | 2 +-
>  refs/packed-backend.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8db7882aacb..b2a880f62f0 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -381,7 +381,7 @@ stat_ref:
>  	if (lstat(path, &st) < 0) {
>  		int ignore_errno;
>  		myerr = errno;
> -		if (myerr != ENOENT || skip_packed_refs)
> +		if ((myerr != ENOENT && myerr != ENOTDIR) || skip_packed_refs)
>  			goto out;
>  		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
>  				      referent, type, &ignore_errno)) {
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 97b68377673..23d478627a7 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -480,7 +480,7 @@ static int load_contents(struct snapshot *snapshot)
>  
>  	fd = open(snapshot->refs->path, O_RDONLY);
>  	if (fd < 0) {
> -		if (errno == ENOENT) {
> +		if (errno == ENOENT || errno == ENOTDIR) {
>  			/*
>  			 * This is OK; it just means that no
>  			 * "packed-refs" file has been written yet,
>
> base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca

So I'm skeptical that this can work at all, but in any case wrapping
this non-POSIX hack in an #ifdef for the relevant platform is somtething
I really think we should have here, or "#ifdef NON_POSIX_FS_HACK" or
something.

You don't want to be carefully reviewing this code thinking wtf, only to
discover later that it's impossible on a well-behaved FS.

Also, NFS has similar options (which I've seen hard break git repos &
corrupt them in the past)< how do its various dangerous caching options
behave in these scenarios?

IOW if we're supporting non-POSIX behavior on platform A, are we
inadvertently making the non-POSIX behavior on platform B even worse?
Even more of a reason to wrap it in ifdefs...

But I really think the answer to this is similar to brian's FAQ patches
for git repos on "cloud mounts", I.e. document carefully that it's
likely to corrupt your repo in unexpected ways.

So I'd be much more comfortable with a workaround that stole what we do
for the *.lock spinning here, i.e. we'd detect this errno, say "wtf,
non-POSIX?" then spin for N ms, and hope to get "past the race".

That would be guaranteed not to suffer from odd corruption issues (as
the behavior wouldn't change, we'd just wait and hope to "catch up")>

Wouldn't that be narrower & better here?

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> 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.
>> ...
>> @@ -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. Patching mingw_lstat() seems more tractable and
> less likely to lead to discovery of other code in the future which
> needs to be patched in a similar way to how files-backend.c and
> packed-backend.c are being patched here.
>
> Perhaps it's a silly question and the answer is perfectly obvious to
> folks directly involved in Git development on Windows, but the commit
> message doesn't seem to answer it for people who don't have such
> inside knowledge.

FWIW, I had the same reaction.  ENOTDIR does not mean an attempt to
access "test_dir/test_tag" failed because "test_dir" did not
exist---it means "test_dir" exists as something that is not a
directory (hence there is no way "test_dir/test_file" to exist).

In the example scenario in the proposed log message, a new tag
"test_dir/test_tag" is created, and (even though the proposed log
message does not explicitly say so) presumably, "test_dir" needs to
be created while doing so.  A failure to access "test_dir/test_file"
due to lack of "test_dir" shouldn't report ENOTDIR but should report
ENOENT.  So something is fishy.

Unless the internal implementation of the filesystem creates a
placeholder that is not a directory at "test_dir", returns the
control to the application, and does further work to turn that
placeholder object into a directory in the background, and the
application attempts to create "test_dir/test_tag" in the meantime,
racing with the filesystem, or something silly like that?

It sounds like a platform specific bug that is not specific to the
ref-files subsystem.  If it can be fixed at lstat() emulation, that
would benefit other checks for ENOENT.

Having said all that.

Stepping back a bit, one situation where we would want to special
case ENOENT is when we have an optional file at known location and
it is OK for the file to be missing.  We may attempt to read from
"$XDG_CONFIG_HOME/git/config" and it may fail due to ENOENT or
ENOTDIR because the user may not be using XDG config location at all
(hence $XDG_CONFIG_HOME or XDG_CONFIG_HOME/git may be missing) or
the leading directories may be there but not the file at the leaf
level.  In such a use case, we should ignore ENOTDIR just like we
ignore ENOENT as an error that does not matter.

In any case, the posted patch may hide a repository corruption from
the codepath affected and cause it to silently return a bogus
answer.  The first hunk touches read_ref_internal() where "path"
variable contains the path we expect to find the on-disk loose ref
file

	if (lstat(path, &st) < 0) {
		int ignore_errno;
		myerr = errno;
		if (myerr != ENOENT || skip_packed_refs)
			goto out;
		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
				      referent, type, &ignore_errno)) {
			myerr = ENOENT;
			goto out;
		}
		ret = 0;
		goto out;
	}

The idea is that a ref does not have to exist as a loose ref file,
so an error from lstat() is not immediately an error.  If the ref
were previously packed, then we should fall back to read it from the
packed-ref file.

So we say that if the error is *NOT* ENOENT, we jump to 'out' label
to report the failed lstat() as an error".  Otherwise we proceed to
attempt to read from the packed-ref file.  Is there any case where
an attempt to read from a loose ref fails with ENOTDIR and it is OK
that we can proceed without reading from the packed-refs file?

If we have a branch "js/foo" that is packed, and then if we removed
it, and then created a branch "js", the original "js/foo" should not
be in the packed-refs file, but supposed a repository is corrupt and
"js/foo" remains in the packed-refs file.  Now imagine that we ask
for "js/foo".  What happens?

We fail to lstat ".git/refs/heads/js/foo" due to ENOTDIR, because
the "js" branch exists loose at ".git/refs/heads/js".  In the
original, because ENOTDIR is not ENOENT, we jumped to "out" to
report the error.  The patched code to allow ENOTDIR will instead
happily read the stale version of "js/foo" out of the packed-refs
file.

[PATCH v2] lstat(mingw): correctly detect ENOTDIR scenarios

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Files' attributes can indicate more than just whether they are files or
directories. It was reported in Git for Windows that on certain network
shares, this let to a nasty problem trying to create tags:

	$ 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 with all types of network shares.
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/

in the indicated instance the following commands worked around the bug:

	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 bug is in the code added in 4b0abd5c695 (mingw: let
lstat() fail with errno == ENOTDIR when appropriate, 2016-01-26) that
emulates the POSIX behavior where `lstat()` should return `ENOENT` if
the file or directory simply does not exist but could be created, and
`ENOTDIR` if there is no file or directory nor could there be because a
leading path already exists and is not a directory.

In that code, the return value of `GetFileAttributesW()` is interpreted
as an enum value, not as a bit field, so that a perfectly fine leading
directory can be misdetected as "not a directory".

As a consequence, the `read_refs_internal()` function would return
`ENOTDIR`, suggesting not only that the tag in the `git tag` invocation
above does not exist, but that it cannot even be created.

Let's fix the code so that it interprets the return value of the
`GetFileAtrtibutesW()` call correctly.

This fixes https://github.com/git-for-windows/git/issues/3727

Reported-by: Pierre Garnier <pgarnier@mega.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Fix the lstat() emulation on Windows
    
    One particular code path in the lstat() emulation on Windows was broken.
    
    This fixes https://github.com/git-for-windows/git/issues/3727
    
    Changes since v1:
    
     * Thanks to Eric's excellent review, the reporter and I dug deeper and
       figured out the real bug (and fix).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1291%2Fdscho%2Fenotdir-and-enoent-can-indicate-missing-refs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1291/dscho/enotdir-and-enoent-can-indicate-missing-refs-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1291

Range-diff vs v1:

 1:  c3d51a755ba < -:  ----------- refs: work around network caching on Windows
 -:  ----------- > 1:  2ebe899736e lstat(mingw): correctly detect ENOTDIR scenarios


 compat/mingw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 545e952a588..3b85bb02536 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -471,8 +471,8 @@ static int has_valid_directory_prefix(wchar_t *wfilename)
 		wfilename[n] = L'\0';
 		attributes = GetFileAttributesW(wfilename);
 		wfilename[n] = c;
-		if (attributes == FILE_ATTRIBUTE_DIRECTORY ||
-				attributes == FILE_ATTRIBUTE_DEVICE)
+		if (attributes &
+		    (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_DEVICE))
 			return 1;
 		if (attributes == INVALID_FILE_ATTRIBUTES)
 			switch (GetLastError()) {

base-commit: 4b0abd5c695c87bf600e57b6a5c7d6844707d34c
-- 
gitgitgadget

Re: [PATCH v2] lstat(mingw): correctly detect ENOTDIR scenarios

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> In that code, the return value of `GetFileAttributesW()` is interpreted
> as an enum value, not as a bit field, so that a perfectly fine leading
> directory can be misdetected as "not a directory".

Nicely analyzed.  So after all ENOTDIR was a buggy response and by
fixing it we help all callers of lstat(), as pointed out in the
earlier review.

>      * Thanks to Eric's excellent review, the reporter and I dug deeper and
>        figured out the real bug (and fix).

Yeah, thanks all.

Will queue.

Re: [PATCH v2] lstat(mingw): correctly detect ENOTDIR scenarios

[Eric Sunshine]

On Thu, Jul 28, 2022 at 10:29 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Files' attributes can indicate more than just whether they are files or
> directories. It was reported in Git for Windows that on certain network
> shares, this let to a nasty problem trying to create tags:

s/let/led/

(not worth a re-roll)

>         $ 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 with all types of network shares.
> 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/
>
> in the indicated instance the following commands worked around the bug:
>
>         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 bug is in the code added in 4b0abd5c695 (mingw: let
> lstat() fail with errno == ENOTDIR when appropriate, 2016-01-26) that
> emulates the POSIX behavior where `lstat()` should return `ENOENT` if
> the file or directory simply does not exist but could be created, and
> `ENOTDIR` if there is no file or directory nor could there be because a
> leading path already exists and is not a directory.
>
> In that code, the return value of `GetFileAttributesW()` is interpreted
> as an enum value, not as a bit field, so that a perfectly fine leading
> directory can be misdetected as "not a directory".
>
> As a consequence, the `read_refs_internal()` function would return
> `ENOTDIR`, suggesting not only that the tag in the `git tag` invocation
> above does not exist, but that it cannot even be created.
>
> Let's fix the code so that it interprets the return value of the
> `GetFileAtrtibutesW()` call correctly.
>
> This fixes https://github.com/git-for-windows/git/issues/3727
>
> Reported-by: Pierre Garnier <pgarnier@mega.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

[PATCH v3] lstat(mingw): correctly detect ENOTDIR scenarios

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Files' attributes can indicate more than just whether they are files or
directories. It was reported in Git for Windows that on certain network
shares, this led to a nasty problem trying to create tags:

	$ 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 with all types of network shares.
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/

in the indicated instance the following commands worked around the bug:

	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 bug is in the code added in 4b0abd5c695 (mingw: let
lstat() fail with errno == ENOTDIR when appropriate, 2016-01-26) that
emulates the POSIX behavior where `lstat()` should return `ENOENT` if
the file or directory simply does not exist but could be created, and
`ENOTDIR` if there is no file or directory nor could there be because a
leading path already exists and is not a directory.

In that code, the return value of `GetFileAttributesW()` is interpreted
as an enum value, not as a bit field, so that a perfectly fine leading
directory can be misdetected as "not a directory".

As a consequence, the `read_refs_internal()` function would return
`ENOTDIR`, suggesting not only that the tag in the `git tag` invocation
above does not exist, but that it cannot even be created.

Let's fix the code so that it interprets the return value of the
`GetFileAttributesW()` call correctly.

This fixes https://github.com/git-for-windows/git/issues/3727

Reported-by: Pierre Garnier <pgarnier@mega.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Fix the lstat() emulation on Windows
    
    One particular code path in the lstat() emulation on Windows was broken.
    
    This fixes https://github.com/git-for-windows/git/issues/3727
    
    Changes since v2:
    
     * Fixed typos in the commit message (I wanted to write "led" but had
       written "let" instead, thanks once again, Eric!, and I managed to
       misspell a function name).
    
    Changes since v1:
    
     * Thanks to Eric's excellent review, the reporter and I dug deeper and
       figured out the real bug (and fix).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1291%2Fdscho%2Fenotdir-and-enoent-can-indicate-missing-refs-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1291/dscho/enotdir-and-enoent-can-indicate-missing-refs-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1291

Range-diff vs v2:

 1:  2ebe899736e ! 1:  b4f08ee0d7c lstat(mingw): correctly detect ENOTDIR scenarios
     @@ Commit message
      
          Files' attributes can indicate more than just whether they are files or
          directories. It was reported in Git for Windows that on certain network
     -    shares, this let to a nasty problem trying to create tags:
     +    shares, this led to a nasty problem trying to create tags:
      
                  $ 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
     @@ Commit message
          above does not exist, but that it cannot even be created.
      
          Let's fix the code so that it interprets the return value of the
     -    `GetFileAtrtibutesW()` call correctly.
     +    `GetFileAttributesW()` call correctly.
      
          This fixes https://github.com/git-for-windows/git/issues/3727
      


 compat/mingw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 545e952a588..3b85bb02536 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -471,8 +471,8 @@ static int has_valid_directory_prefix(wchar_t *wfilename)
 		wfilename[n] = L'\0';
 		attributes = GetFileAttributesW(wfilename);
 		wfilename[n] = c;
-		if (attributes == FILE_ATTRIBUTE_DIRECTORY ||
-				attributes == FILE_ATTRIBUTE_DEVICE)
+		if (attributes &
+		    (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_DEVICE))
 			return 1;
 		if (attributes == INVALID_FILE_ATTRIBUTES)
 			switch (GetLastError()) {

base-commit: 4b0abd5c695c87bf600e57b6a5c7d6844707d34c
-- 
gitgitgadget