refs: work around network caching on Windows

From: Pierre Garnier <[email protected]>

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

---
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(-)

index 8db7882aacb..b2a880f62f0 100644
@@ -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)) {

index 97b68377673..23d478627a7 100644
@@ -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

Hi Eric,

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

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.

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?

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?

From: Johannes Schindelin <[email protected]>

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

---
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(-)

index 545e952a588..3b85bb02536 100644
@@ -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

"Johannes Schindelin via GitGitGadget" <[email protected]>

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.

Yeah, thanks all.

Will queue.

s/let/led/

(not worth a re-roll)

From: Johannes Schindelin <[email protected]>

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

---
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(-)

index 545e952a588..3b85bb02536 100644
@@ -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