Support symlinks on Windows when core.symlinks=true by ethomson · Pull Request #4713 · libgit2/libgit2

ethomson · 2018-07-03T10:00:08Z

Windows 10 enables non-Administrator users to create symbolic links when Developer Mode is enabled. Git for Windows supports this scenario when core.symlinks=true.

Similarly, we should support creation of proper symbolic links on Windows when core.symlinks=true. This is as simple as implementing p_symlink properly, as checkout already behaves properly, examining core.symlinks and creating a fake symlink when core.symlinks=false (and creating a true symbolic link otherwise).

This is not ready for merge: at present, Git for Windows requires a configuration setting core.symlinks in a global configuration area to enable automatic detection of symbolic links during repository initialization.

This probably makes sense: if a repository is created by Administrator - who has symbolic link creation privileges - you may still want to set core.symlinks=false on Windows since there is no suggestion that non-Administrator users will be able to use symbolic links.

However, there is some thought of changing this in Git for Windows (git-for-windows/git#1744). So let's wait for that to be decided - if they change their behavior, then this will be ready for merge. If not, we will need to modify this PR to adopt their existing behavior.

pks-t

Nicely done and a breeze to review, thanks! I do have some remarks, but they're all only really minor stuff which you can safely ignore if you want to

pks-t · 2018-08-02T10:53:58Z

tests/checkout/icase.c

-	git_path_mkposix(filename);
-
-	return filename;
-#else


I'm curious: what is the downside of removing this code? I guess we haven't added this for fun if our POSIX emulation layer was already able to resolve these paths correctly. I guess the reason is performance?

There might be a bit of a perf hit here. This Windows-bit being removed was, in fact, written first, IIRC. And then we had to emulate the GetFinalPathNameByHandle call on POSIX. I expect that enumerating the directory will be a little bit slower, but:

the old behavior wasn't correct with symlinks brought into the picture

removal of code, yay

it can't possibly be that much slower, I wouldn't think, and

it's test code not production. 😀

pks-t · 2018-08-02T11:02:02Z

tests/checkout/index.c

-		cl_assert_equal_s(link_data, "new.txt");
-		check_file_contents("./testrepo/link_to_new.txt", "my new file\n");
-	}
-#endif


It's nice to get rid of all those ifdefs

pks-t · 2018-08-02T11:03:44Z

tests/checkout/index.c

 {
 	git_checkout_options opts = GIT_CHECKOUT_OPTIONS_INIT;
+	char link_data[1024];
+	size_t link_size = 1024;


We could use GIT_PATH_MAX instead here to make this a bit less magical. But it's not like it'd matter much anyway, so feel free to ignore this remark

Agreed and done.

pks-t · 2018-08-02T11:05:33Z

src/win32/posix_w32.c

-	/* Real symlinks on NTFS require admin privileges. Until this changes,
-	 * libgit2 just creates a text file with the link target in the contents.
-	 */
-	return git_futils_fake_symlink(old, new);


Do you know off-hand how we treated these fake symlink in our diff machinery? Were those files shown as modified or was there some "magic" around fake symlinks?

They weren't shown as different. The diff mechanism will detect if symlinks are supported and either stat the link to determine its destination, or read the file if symlinks are not supported.

pks-t · 2018-08-02T11:07:44Z

src/win32/posix_w32.c

+
+	if (!CreateSymbolicLinkW(path_w, target_w,
+	    SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE))
+		return -1;


Don't we have to handle the error case a bit more verbosely? I'm not really familiar with the win32 POSIX layer, but we usually invoke set_errno() in case where the call fails, don't we?

We should be good here. For the posix emulation functions, they can generally fail with an error code without doing anything else. Callers should then giterr_set(GITERR_OS... and giterr_set is smart enough to read the win32 system error message.

We only need to set_errno when callers actually read the errno back out and make a decision about how to handle the error. (eg, looking for EAGAIN). We try to carefully map the Windows failure into something that callers can use. And we do it carefully to ensure that callers can understand that errno in the way they want to proceed for both Win32 and the actual POSIX. Trying to make it general purpose instead would be 😓 .

Since we're not doing that here, we're okay to skip that step.

pks-t · 2018-08-02T11:10:18Z

tests/repo/init.c

+
+	no_symlinks = (p_symlink("target", "link") < 0 ||
+	    p_lstat("link", &st) < 0 ||
+	    ! (S_ISLNK(st.st_mode)));


It might make sense to have a common helper function to detect symlink support which unifies these three lines here and in "checkout::index".

Agreed and done.

Increase the WIN32_WINNT level to 0x0600, which enables support for new APIs from Windows 6.0 (Vista). We had previously set this to 0x0501, which was Windows XP. Although we removed XP support many years ago, there was no need to update this level previously. We're doing so now explicitly so that we can get support for the `CreateSymbolicLink` API.

Now that we've updated to WIN32_WINNT version of Vista or better, we don't need to dynamically load GetFinalPathNameByHandle and can simply invoke it directly.

To determine the canonical filename for a given path, we previously looked at the directory entries on POSIX systems and used GetFinalPathNameByHandle on Windows. However, GetFinalPathNameByHandle requires a HANDLE - the results of CreateFile - and you cannot CreateFile on a symbolic link. To support finding the canonical path of a symbolic link, simply use the existing POSIX code to look at the directory entries.

When testing whether symlinks are correctly checked out, examine the `core.symlinks` configuration option to determine if symlinks are supported in a repository, don't simply assume that Windows means that symbolic links are not supported. Further, when testing the expected default behavior of `core.symlinks`, test the filesystem's support to determine if symlinks are supported. Finally, ensure that `core.symlinks=true` fails on a system where symlinks are actually not supported. This aligns with the behavior of Git for Windows.

Enable `p_symlink` to actually create symbolic links, not just create a fake link (a text file containing the link target). This now means that `core.symlinks=true` works on Windows platforms where symbolic links are enabled (likely due to running in Developer Mode).

Ensure that `core.symlinks` is set correctly. By default, it is unset, but it is explicitly set to `false` if the platform was detected to not support symlinks during repository initialization.

Don't try to use `link_size` as an index into a string if `p_readlink` returned <0. That will - obviously - fail and we'll write out of bounds.

Emulate the Git for Windows `core.symlinks` support. Since symbolic links are generally enabled for Administrator (and _may_ be enabled due to enabling Developer mode) but symbolic links are still sufficiently uncommon on Windows that Git users are expected to explicitly opt-in to symbolic links by enabling `core.symlinks=true` in a global (or xdg or system) configuration. When `core.symlinks=true` is set globally _and_ symbolic links support is detected then new repositories created will not have a `core.symlinks` set. If `core.symlinks` is _not_ set then no detection will be performed, and `core.symlinks=false` will be set in the repository configuration.

ethomson · 2018-10-20T13:18:04Z

I've updated this to use the Git for Windows behavior. If core.symlinks=true is set in a global/xdg/system configuration then symlink support will be detected when creating a new repository. (If the underlying filesystem does support symbolic links then there will be nothing set in the repository configuration. If the underlying filesystem does not support symbolic links then core.symlinks will be set to false.)

On non-Windows systems, the filesystem will be examined like before.

Teach `load_config` how to load all the configurations except (optionally) the repository configuration. This allows the new repository codepath to load the global/xdg/system configuration paths so that they can be inspected during repository initialization.

Provide a function that allows tests to set up a bespoke global configuration path.

Test updated symbolic link creation on Windows. Ensure that we emulate Git for Windows behavior. Ensure that when `core.symlinks=true` is set in a global configuration that new repositories are created without a `core.symlinks` setting, and that when `core.symlinks` is unset that `core.symlinks=false` in set in the repository. Further ensure that checkout honors the expected `core.symlinks` defaults on Windows.

ethomson · 2018-10-21T02:15:27Z

Fixes #4107

ethomson · 2018-10-26T14:26:59Z

@pks-t I've added the 3 newest commits to reflect the Git for Windows behavior - a quick sanity check on those would be 👍 when you have some time.

pks-t · 2018-11-16T08:49:08Z

On Fri, Oct 26, 2018 at 02:27:08PM +0000, Edward Thomson wrote: @pks-t I've added the 3 newest commits to reflect the Git for Windows behavior - a quick sanity check on those would be 👍 when you have some time.

Sorry for not having reviewed this -- have quite a lot of travelling to do currently, so I'm a bit short on time right now.

danielgindi · 2019-03-05T10:10:15Z

What about SYMBOLIC_LINK_FLAG_DIRECTORY?

ethomson · 2019-03-05T10:44:17Z

What about SYMBOLIC_LINK_FLAG_DIRECTORY?

What about it? Is there something deficient in this PR? Can you provide a simple repro case?

danielgindi · 2019-03-05T13:26:03Z

The problem is with symlinks to directories.
When creating a symlink you should test the target, see if it's a directory or a file, and add the flag accordingly.
This is because in Windows the symlink is "typed", and right now git creates a symlink to a file, which does not work.

Something like:

  DWORD dwFlags = SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE;

  if (std::filesystem::is_directory(target_w))
    dwFlags |= SYMBOLIC_LINK_FLAG_DIRECTORY;

  if (!CreateSymbolicLinkW(path_w, target_w, dwFlags))
    ...

Or if c++17 is not available:

  DWORD dwFlags = SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE;

  if (GetFileAttributesW(target_w) & FILE_ATTRIBUTE_DIRECTORY)
    dwFlags |= SYMBOLIC_LINK_FLAG_DIRECTORY;

  if (!CreateSymbolicLinkW(path_w, target_w, dwFlags))
    ...

ethomson · 2019-03-05T15:18:55Z

My question wasn't how to do it, but if we should do it. I ask explicitly because omitting that flag will still give you a symbolic link, one that behaves more like a Unix symbolic link:

C:\Temp\TestRepos\004>mklink link_to_folder folder
symbolic link created for link_to_folder <<===>> folder

C:\Temp\TestRepos\004>dir
 Volume in drive C has no label.
 Volume Serial Number is DA35-0653

 Directory of C:\Temp\TestRepos\004

03/05/2019  03:15 PM    <DIR>          .
03/05/2019  03:15 PM    <DIR>          ..
03/05/2019  03:14 PM    <DIR>          folder
03/05/2019  03:15 PM    <SYMLINK>      link_to_folder [folder]
               1 File(s)              0 bytes
               3 Dir(s)  39,293,509,632 bytes free

(Note the omission of the /d flag, which propagates the SYMBOLIC_LINK_FLAG_DIRECTORY option.)

But the question is: what does git do. And indeed, checking this folder out with git does create a directory-style link:

C:\Temp\TestRepos\004>dir
 Volume in drive C has no label.
 Volume Serial Number is DA35-0653

 Directory of C:\Temp\TestRepos\004

03/05/2019  03:14 PM    <DIR>          .
03/05/2019  03:14 PM    <DIR>          ..
03/05/2019  03:14 PM    <DIR>          folder
03/05/2019  03:14 PM    <SYMLINKD>     link_to_folder [folder]
               0 File(s)              0 bytes
               4 Dir(s)  39,293,509,632 bytes free

I thought when created this PR that I validated that git itself did not create SYMBOLIC_LINK_FLAG_DIRECTORY links; it's possible either that they changed their behavior, but probably more likely that I got this wrong.

I agree with your analysis (and indeed, we don't do C++). Would you be interested in submitting a pull request?

danielgindi · 2019-03-06T07:22:03Z

@ethomson I just tried to answer both questions at the same time (although the latter was not really asked :-)

I'd love to send a PR, but as you already covered this area I think I'll leave it for you to do.
You'll be able to identify if we're messing something up you know.

ethomson force-pushed the ethomson/win_symlinks branch 2 times, most recently from 8e03c75 to 35504ee Compare July 3, 2018 10:22

pks-t approved these changes Aug 2, 2018

View reviewed changes

ethomson added 8 commits October 20, 2018 05:50

win32: use GetFinalPathNameByHandle directly

3077126

Now that we've updated to WIN32_WINNT version of Vista or better, we don't need to dynamically load GetFinalPathNameByHandle and can simply invoke it directly.

repo tests: ensure core.symlinks is set correctly

8533c80

Ensure that `core.symlinks` is set correctly. By default, it is unset, but it is explicitly set to `false` if the platform was detected to not support symlinks during repository initialization.

checkout tests: ensure readlink succeeds

df1733d

Don't try to use `link_size` as an index into a string if `p_readlink` returned <0. That will - obviously - fail and we'll write out of bounds.

tests: provide symlink support helper function

628dae8

ethomson force-pushed the ethomson/win_symlinks branch from a2df44e to 208982d Compare October 20, 2018 13:03

ethomson force-pushed the ethomson/win_symlinks branch from 208982d to b17d170 Compare October 20, 2018 13:13

ethomson added 2 commits October 20, 2018 14:37

repo::init tests: refactor global config path override

3f0caa1

Provide a function that allows tests to set up a bespoke global configuration path.

ethomson force-pushed the ethomson/win_symlinks branch 2 times, most recently from a3a445d to a69d701 Compare October 20, 2018 14:59

ethomson force-pushed the ethomson/win_symlinks branch from a69d701 to da500cc Compare October 20, 2018 16:07

ethomson merged commit 7321cff into master Nov 15, 2018

ethomson deleted the ethomson/win_symlinks branch January 9, 2019 10:16

ethomson mentioned this pull request Feb 17, 2019

Cloning a repo on Windows breaks symlinks #4107

Closed

danielgindi mentioned this pull request May 5, 2019

Support symlinks for directories in win32 #5065

Merged

ethomson mentioned this pull request Oct 15, 2019

_git2_ junction directory is throwing off dirty checks libgit2/libgit2sharp#1726

Closed

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

mathemancer mentioned this pull request Oct 1, 2020

Deduplicate /www/l creativecommons/licensebuttons#33

Closed

Conversation

ethomson commented Jul 3, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Oct 20, 2018

Uh oh!

ethomson commented Oct 21, 2018

Uh oh!

ethomson commented Oct 26, 2018

Uh oh!

pks-t commented Nov 16, 2018 via email

Uh oh!

danielgindi commented Mar 5, 2019

Uh oh!

ethomson commented Mar 5, 2019

Uh oh!

danielgindi commented Mar 5, 2019

Uh oh!

ethomson commented Mar 5, 2019

Uh oh!

danielgindi commented Mar 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ethomson Oct 19, 2018 •

edited

Loading