Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 3, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions compat/mingw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1293,13 +1293,38 @@ char *mingw_strbuf_realpath(struct strbuf *resolved, const char *path)
HANDLE h;
DWORD ret;
int len;
const char *last_component = NULL;

if (xutftowcs_path(wpath, path) < 0)
return NULL;

h = CreateFileW(wpath, 0,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);

/*
* strbuf_realpath() allows the last path component to not exist. If
* that is the case, now it's time to try without last component.
*/
if (h == INVALID_HANDLE_VALUE &&
GetLastError() == ERROR_FILE_NOT_FOUND) {
/* cut last component off of `wpath` */
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))) {
last_component++; /* skip directory separator */
*p = L'\0';
h = CreateFileW(wpath, 0, FILE_SHARE_READ |
FILE_SHARE_WRITE | FILE_SHARE_DELETE,
NULL, OPEN_EXISTING,
FILE_FLAG_BACKUP_SEMANTICS, NULL);
}
}

if (h == INVALID_HANDLE_VALUE)
return NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's CreateFile() that fails.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@SyntevoAlex SyntevoAlex Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@SyntevoAlex SyntevoAlex Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Expand All @@ -1314,6 +1339,13 @@ char *mingw_strbuf_realpath(struct strbuf *resolved, const char *path)
if (len < 0)
return NULL;
resolved->len = len;

if (last_component) {
/* Use forward-slash, like `normalize_ntpath()` */
strbuf_addch(resolved, '/');
strbuf_addstr(resolved, last_component);
}

return resolved->buf;

}
Expand Down
7 changes: 7 additions & 0 deletions t/t5601-clone.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ test_expect_success 'clone respects GIT_WORK_TREE' '

'

test_expect_success CASE_INSENSITIVE_FS 'core.worktree is not added due to path case' '

mkdir UPPERCASE &&
git clone src "$(pwd)/uppercase" &&
test "unset" = "$(git -C UPPERCASE config --default unset core.worktree)"
'

test_expect_success 'clone from hooks' '

test_create_repo r0 &&
Expand Down