Fix when lazy caching of network shares tags directories were just created with previously-unhandled attributes

[pgrmega]

This fix for the issue : #3727

Use case :
Creation of a tag in the form "parent/child".

Exemple :

git tag -a -m "my message" tagdir/mytag

Context :
$ git --version --build-options
git version 2.35.1.windows.2
cpu: x86_64
built from commit: 5437f0f
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
$ cmd.exe /c ver
Microsoft Windows [Version 10.0.17763.2565]

Error :
fatal: cannot lock ref 'refs/tags/tagdir/mytag': unable to resolve reference 'refs/tags/tagdir/mytag': Not a directory

Problem analysis:
GetFileAttributesExW used in mingw_lstat function in git/compat/mingw.c can raise an error ERROR_PATH_NOT_FOUND.
In this case, the has_valid_directory_prefix is used to check if the parent directory exists.
So that, when the parent directory exists, mingw_lstat returns ENOTDIR.
ENOTDIR is not managed by the caller code (files_read_raw_ref in git/refs/files-backend.c).
It probably should.

Conclusion
This commit enables to take into account the case when ENOTDIR is returned by mingw_lstat.

[hazmee007]

แก้ไขข้อผิดพลาด "ร้ายแรง: ไม่สามารถล็อคการอ้างอิง ... ไม่ใช่ไดเรกทอรี"

(English: Fixes "Serious: References cannot be locked ... not directories" error. )

[dscho]

@pgrmega this looks good! Could you still sign off the patch to allow me to upstream this to the Git project?

[pgrmega]

I amended the commit to add signed-off.

[dscho]

Hmm. There seem to be a couple of problems:

• https://github.com/git-for-windows/git/runs/5829257806?check_suite_focus=true#step:5:1163
• https://github.com/git-for-windows/git/runs/5829257806?check_suite_focus=true#step:5:1201
• https://github.com/git-for-windows/git/runs/5829257806?check_suite_focus=true#step:5:75555

That's the breakages from the linux-clang job. Could you have a look @pgrmega?

[pgrmega]

Hello @dscho ,
I don't know how to manage this situation.

[dscho]

From https://github.com/git-for-windows/git/runs/5883049437?check_suite_focus=true#step:5:1163:

+ git init my_submodule
fatal: not a git repository: /home/runner/work/git/git/t/trash directory.t2501-cwd-empty/my_submodule/../.git/modules/my_submodule
error: last command exited with $?=128

I guess that there is code that actually relies on errno == ENOTDIR causing read_refs_internal() to return with an error.

Do you have access to a WSL setup? You could debug this issue in there, as it obviously does not reproduce on Windows itself.

[dscho]

Do you have access to a WSL setup?

@pgrmega do you?

[pgrmega]

Do you have access to a WSL setup?

@pgrmega do you?

Hello, sorry, no, I hadn't have the time to search how to debug in a linux environment.

[dscho]

Maybe https://github.com/git-for-windows/git/wiki/Debugging-Git#debugging-git-using-the-gnu-debugger-gdb is helpful?

[dscho]

Do you have access to a WSL setup?

@pgrmega do you?

Hello, sorry, no, I hadn't have the time to search how to debug in a linux environment.

I finally got around to spend some time on this, and it turns out that you found the right spot to fix this, but there was another spot, too.

The way I debugged this was with one of the more tedious tools in my debugging toolbox:

int x = 0;

fprintf(stderr, "current pid: %d\n", (int)getpid());

while (!x) {
    sleep(1);
}

I inserted this at the code site you had touched, ran the test script t/t2501-cwd-empty.sh, attached a gdb to the process, and noticed that both with and without the new ENOTDIR treatment the process was erroring out, but the error message was different in both cases. Stopping at die_builtin() then pointed to the correct location for the second change that was required.

The load_contents() function is hit because we change read_ref_internal() to continue in case of ENOTDIR which means that refs_read_raw_ref() is called which eventually tries to create_snapshot().

[dscho]

FWIW I just submitted this patch to the Git mailing list for review: gitgitgadget#1291

[dscho]

@pgrmega could you test something for me? Could you apply this patch and tell me what it outputs when trying to tag?

diff --git a/compat/mingw.c b/compat/mingw.c
index 2607de93af5..5bac330e56e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -768,6 +768,7 @@ static int has_valid_directory_prefix(wchar_t *wfilename)
 		wfilename[n] = L'\0';
 		attributes = GetFileAttributesW(wfilename);
 		wfilename[n] = c;
+error("attributes for '%ls': %ld, last error: %ld", wfilename, attributes, GetLastError());
 		if (attributes == FILE_ATTRIBUTE_DIRECTORY ||
 				attributes == FILE_ATTRIBUTE_DEVICE)
 			return 1;

The problem I am trying to figure out is why we run into this code path, where Git thinks that there is a non-directory in the way, but there actually isn't.

[dscho]

@pgrmega while I am still very interested in the output I asked for, I now have a hunch that I might have found the real fix. Could you also test this patch (replacing the one adding the || myerr == ENOTDIR part)?

diff --git a/compat/mingw.c b/compat/mingw.c
index 2607de93af5..2678302c6a9 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -768,8 +768,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 |
+				  attributes == FILE_ATTRIBUTE_DEVICE))
 			return 1;
 		if (attributes == INVALID_FILE_ATTRIBUTES)
 			switch (GetLastError()) {

[rimrul]

+		if (attributes & (FILE_ATTRIBUTE_DIRECTORY |
+				  attributes == FILE_ATTRIBUTE_DEVICE))

This feels a little weird. Should this perhaps be something like this?

if (attributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_DEVICE))

[dscho]

+		if (attributes & (FILE_ATTRIBUTE_DIRECTORY |
+				  attributes == FILE_ATTRIBUTE_DEVICE))

This feels a little weird. Should this perhaps be something like this?

if (attributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_DEVICE))

D'oh. Of course! You're absolutely right, that's what I meant but failed to re-read properly when I tried to write it down.

[pgrmega]

@dscho, is this still usefull that I test your patch ?
If yes, I'll try to find time...

@pgrmega while I am still very interested in the output I asked for, I now have a hunch that I might have found the real fix. Could you also test this patch (replacing the one adding the || myerr == ENOTDIR part)?

diff --git a/compat/mingw.c b/compat/mingw.c
index 2607de93af5..2678302c6a9 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -768,8 +768,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 |
+				  attributes == FILE_ATTRIBUTE_DEVICE))
 			return 1;
 		if (attributes == INVALID_FILE_ATTRIBUTES)
 			switch (GetLastError()) {

[pgrmega]

@pgrmega while I am still very interested in the output I asked for, I now have a hunch that I might have found the real fix. Could you also test this patch (replacing the one adding the || myerr == ENOTDIR part)?

diff --git a/compat/mingw.c b/compat/mingw.c
index 2607de93af5..2678302c6a9 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -768,8 +768,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 |
+				  attributes == FILE_ATTRIBUTE_DEVICE))
 			return 1;
 		if (attributes == INVALID_FILE_ATTRIBUTES)
 			switch (GetLastError()) {