Fix problem where clone adds core.worktree due to path case differences

[SyntevoAlex]

A fix for #2569

[dscho]

Hmm. This patch will retry under more circumstances than just when the file does not exist. I would have expected the patch to check whether the GetFinalPathByHandle() function failed because the item (it's not necessarily a file, right? Could be a directory or symlink, too) doesn't exist and only do the last component magic in that case.

Also, [:upper:]? I seem to remember that we use A-Z instead...

[SyntevoAlex]

Hmm. This patch will retry under more circumstances than just when the file does not exist. I would have expected the patch to check whether the GetFinalPathByHandle() function failed because the item (it's not necessarily a file, right? Could be a directory or symlink, too) doesn't exist and only do the last component magic in that case.

Correct, it always retries. I could change it to only retry on ERROR_FILE_NOT_FOUND, but it seems that is going to be a harmful change. The only benefit I see is optimization, but this function only seems to be called very rarely and in the majority of cases it will not fail, so optimization value is near zero. At the same time, code will be more complicated (now the helper function will need to return 3 results), and I'm not quite sure if ERROR_FILE_NOT_FOUND is the only possible "interesting" error here.

I will implement that if you still think that it should be done.

Also, [:upper:]? I seem to remember that we use A-Z instead...

Thanks! I'm quite a newbie in shell scripts, so I just grabbed something from Stack Overflow and made sure it works. I wonder how A-Z is better, but nevermind, it indeed seems to be the agreed way in test scripts.

I have updated the patch.

[SyntevoAlex]

Hmm, I didn't figure that other errors in last component could introduce difference between mingw_strbuf_realpath() and strbuf_realpath().

I have tested 4 cases:

1. C:\access_denied_folder\1.txt
mingw_strbuf_realpath() fails on path 1 with ERROR_ACCESS_DENIED.
   mingw_strbuf_realpath() fails on path 2 with ERROR_ACCESS_DENIED.
   strbuf_realpath() fails with ERROR_ACCESS_DENIED.
2. C:\access_denied_folder
mingw_strbuf_realpath() fails on path 1 with ERROR_ACCESS_DENIED.
   mingw_strbuf_realpath() succeeds on path 2.
   strbuf_realpath(), if forced to continue, succeeds.
3. C:\broken_symlink\1.txt
mingw_strbuf_realpath() fails on path 1 with ERROR_PATH_NOT_FOUND.
   mingw_strbuf_realpath() fails on path 2 with ERROR_FILE_NOT_FOUND.
   strbuf_realpath() fails with ERROR_PATH_NOT_FOUND.
4. C:\broken_symlink
mingw_strbuf_realpath() fails on path 1 with ERROR_FILE_NOT_FOUND.
   mingw_strbuf_realpath() succeeds on path 2.
   strbuf_realpath(), if forced to continue, succeeds.

Observations:

1. ERROR_FILE_NOT_FOUND is also returned for broken symlinks.
2. strbuf_realpath() uses GetFileAttributesEx() instead of CreateFile(), which allows to dodge both problems in last component.
3. Current behavior of mingw_strbuf_realpath() matches that of strbuf_realpath().
4. If only ERROR_FILE_NOT_FOUND is retried, behavior of mingw_strbuf_realpath() will not match strbuf_realpath().

Conclusion:
I understand that it should continue to retry on all errors.

[dscho]

I wonder how A-Z is better

You probably tested with a GNU version of tr, right? These tests need to run in all kinds of settings, also using older tr, and BSD versions of tr. In general, it is always a good idea to imitate existing code when patching a legacy code base. There is no tr call in Git that uses [:upper:].

Correct, it always retries.

That is bad. If the first call fails for unrelated reasons, we should return that error, not overwrite it with a second call's error.

I could change it to only retry on ERROR_FILE_NOT_FOUND, but it seems that is going to be a harmful change.

I fail to see how this would be harmful.

At the same time, code will be more complicated (now the helper function will need to return 3 results)

Oh, I see. I did not actually mean to keep that helper. It is misnamed, and I think it is also slightly overkill ;-)

[dscho]

So is it this function call that is failing, or the GetFinalPathNameByHandleW() one?

[SyntevoAlex]

It's CreateFile() that fails.

[dscho]

Of course, because of the OPEN_EXISTING flag.

So here would be a splendid spot to insert if (h == INVALID_HANDLE_VALUE && GetLastError()) .... The easiest way would actually to look for the last path separator in both path and wpath; That way, you do not have to duplicate the last component, but can trim the wpath and by storing its position within path in a new local variable (that can be initialized to ""), you can then append it after the xwcstoutf() call below (actually after verifying that that call worked successfully).

That's not a ton more code, actually. Essentially something like this:

if (h == INVALID_HANDLE_VALUE && GetLastError() == FILE_NOT_FOUND) {
    /* remember the last component and trim `wpath` for now */
    wchar_t *p = wpath + wcslen(wpath);

    while (p != wpath)
        if (*(--p) == L'/' || *p == L'\\')
            break; /* found start of last component */
    if (p != wpath && (last_component = find_last_dir_sep(path)) {
        *p = L'\0';
        h = CreateFileW(wpath, 0,
			FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
			OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
    }
}

And then later

	len = xwcstoutf(resolved->buf, normalize_ntpath(wpath), len);
	if (len < 0)
		return NULL;
	if (last_component)
		strbuf_addstr(resolved, last_component);

[SyntevoAlex]

It seems that the core question here is whether to narrow retrying to FILE_NOT_FOUND or not. Previously I already shown that with FILE_NOT_FOUND only, mingw_strbuf_realpath() may fail where strbuf_realpath() does not, which could again invite issues like the PR is trying to solve. Just to remind, core.worktree problem begins when mingw_strbuf_realpath() fails and strbuf_realpath() succeeds.

I would say that mingw_strbuf_realpath() shall never fail where strbuf_realpath() succeeds.

Now, what other errors could happen? For instance, xutftowcs_path_ex() may fail, because the last component is too long. Again, strbuf_realpath() will probably succeed here, and my design will succeed, but your design will fail and fall back to strbuf_realpath(). It's a bit hard to really judge if this is a good or bad thing, but again, I would rather prefer to stay closer to strbuf_realpath().

It's a bit hard to predict which design may have more unexpected problems in it. Most of this comes from the difference between lstat() (used in original strbuf_realpath()) and CreateFile(), where the first is more forgiving - it can tolerate things like no-access folders and broken symlinks.

If we agree to retry on all errors, then I'm not sure if your design is better. It's roughly the same amount of code, but it's harder to follow, as it has more parallel things to keep in mind while reading. For example, simultaneously removing (in different ways) the last component from two strings instills doubt and takes an extra read to verify. In my design, I think it's easier to grasp the second function once the first one is understood. It it's not about code clarity, then the other possible reason is performance. Performance-wise, the only difference is that my design will execute an extra xutftowcs_path_ex(), and your design will instead search for last component in wchar*. That's probably a very small difference compared to cost of CreateFile().

To summarize, if you're confident and want your way, say so and I will implement.

[SyntevoAlex]

A different observation: I'm suspecting that the root cause is even deeper. To my understanding, you implemented mingw_strbuf_realpath() to solve problem with junctions. During my debugging adventures, I noticed that file_attr_to_st_mode() does not recognize junctions as S_IFLNK. That definitely raised my eyebrow.

Could it be that entire mingw_strbuf_realpath() needs to be removed again, and file_attr_to_st_mode() fixed so that strbuf_realpath() works as intended? That will put an end to all cases where mingw_strbuf_realpath() and strbuf_realpath() disagree.

Afterall, I have a bad gut feeling about two different strbuf_realpath() implementations, which are prone to disagreeing because they use quite different APIs.

What do you think?

[dscho]

During my debugging adventures, I noticed that file_attr_to_st_mode() does not recognize junctions as S_IFLNK.

Yes, that is intentional. see e.g. the discussion in #437.

It seems that the core question here is whether to narrow retrying to FILE_NOT_FOUND or not.

I guess so.

Previously I already shown that with FILE_NOT_FOUND only, mingw_strbuf_realpath() may fail where strbuf_realpath() does not

And if mingw_strbuf_realpath() fails, that's okay, because it then returns NULL and strbuf_realpath() continues with the non-Windows-specific code. Remember, mingw_strbuf_realpath() is not a replacement of strbuf_realpath(), it is a potential shortcut called by the latter.

Just to remind, core.worktree problem begins when mingw_strbuf_realpath() fails and strbuf_realpath() succeeds.

Sure, but the changes I proposed fix that, too. And their scope is so narrow that I am relatively confident that no unintended side effects are introduced. Retrying on all errors, on the other hand, does strike me as asking for unintended side effects.

I would say that mingw_strbuf_realpath() shall never fail where strbuf_realpath() succeeds.

That was not the intention when I introduced the former as a potential shortcut by calling it from the latter.

To my understanding, you implemented mingw_strbuf_realpath() to solve problem with junctions.

Yes, but I also used the opportunity to use native Win32 API to resolve symlinks rather than doing it (tediously) "by hand".

It's roughly the same amount of code,

Not really:

 compat/mingw.c   | 42 +++++++++++++++++++++++++++++++++++-------

vs

 compat/mingw.c   | 32 ++++++++++++++++++++++++++++++++

but it's harder to follow

I find it harder to follow the two-function version, actually, especially because the return value type was changed (which was unnecessary) and because it is so unclear in the two-function version whether we are retrying for a legitimate reason or not.

Afterall, I have a bad gut feeling about two different strbuf_realpath() implementations, which are prone to disagreeing because they use quite different APIs.

First of all, they are not different implementations. One function calls the latter (a platform-specific shortcut, where available).

And the only reason why they use different APIs is because strbuf_realpath() can only rely on a subset of POSIX functionality, and therefore cannot use platform-specific functions. That's why platform_strbuf_realpath is a thing, to abstract that away from the generic implementation.

I do agree, however, that it is really tricky to navigate all of the issues with junction points vs symlinks. Which is the reason why I want to stay away from making sweeping changes as much as I can, and use more surgical ways to address the issue you found.

I pushed my proposed fixup! to my fork (under the same branch name you chose, to make it easier for you) and the test suite is currently running: https://github.com/dscho/git/runs/833879717?check_suite_focus=true.

[SyntevoAlex]

Thanks for taking time to prepare a fixup! It looks reasonable to me, and I squashed it as is.

[dscho]

Awesome work, @SyntevoAlex, keep it up! I'm crashing right now, but will come back tomorrow.

[SyntevoAlex]

Thanks for a quick review!

On a different topic, this patch had me thinking: why don't you strike an agreement with Junio to just wildcard upstream everything that only affects MINGW? Or maybe with only a surface review? Afterall, windows users use git-for-windows, so all the patches are battle tested already.