Skip to content

fixup! Win32: change default of 'core.symlinks' to false#1744

Closed
dscho wants to merge 1 commit into
git-for-windows:masterfrom
dscho:fix-core-symlinks-default-setting
Closed

fixup! Win32: change default of 'core.symlinks' to false#1744
dscho wants to merge 1 commit into
git-for-windows:masterfrom
dscho:fix-core-symlinks-default-setting

Conversation

@dscho

@dscho dscho commented Jul 2, 2018

Copy link
Copy Markdown
Member

Revert this change, as it does not take into account that Windows 10 now
allows all users to create symbolic links in developer mode.

Noticed by Ed Thomson.

Signed-off-by: Johannes Schindelin johannes.schindelin@gmx.de

Thanks for taking the time to contribute to Git!

Those seeking to contribute to the Git for Windows fork should see
http://gitforwindows.org/#contribute on how to contribute Windows specific enhancements.

If your contribution is for the core Git functions and documentation
please be aware that the Git community does not use the github.com issues
or pull request mechanism for their contributions.

Instead, we use the Git mailing list (git@vger.kernel.org) for code and
documenatation submissions, code reviews, and bug reports. The
mailing list is plain text only (anything with HTML is sent directly
to the spam folder).

Nevertheless, you can use submitGit to conveniently send your Pull
Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

Revert this change, as it does not take into account that Windows 10 now
allows all users to create symbolic links in developer mode.

Noticed by Ed Thomson.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho requested a review from ethomson July 2, 2018 14:28
@ethomson

ethomson commented Jul 3, 2018

Copy link
Copy Markdown
Member

🤔

I don't have the background to make intelligent comments about this patch - that an environment variable sets up some larger behavioral changes down the road is a little opaque to me. I'll trust that it updates the behaviors as described.

But thinking about this further: while I think that detecting core.symlinks based purely on capabilities probably makes sense for the general case (users running Git for Windows as a normal user with Developer Mode enabled), it does get a little trickier in edge cases. For example, if you init a repository as Administrator, now are core.symlinks always enabled? That really does seem a bit unexpected and I better understand why one might have to re-configure the default default.

@ethomson

ethomson commented Jul 3, 2018

Copy link
Copy Markdown
Member

To clarify my last point further: I can imagine that there are workflows where an administrator runs git init or git clone and a normal user then interacts with the repository. In fact, this sounds like my workflow at $DAYJOB. 😀

@dscho

dscho commented Jul 3, 2018

Copy link
Copy Markdown
Member Author

if you init a repository as Administrator, now are core.symlinks always enabled?

Yep.

And if you care to look at the test failures, that seems to be exactly what is happening, with the predictable problems on Windows: the idea of MSYS2's Bash of what a symbolic link is differs drastically from what git.exe thinks a symbolic link is.

I would now usually point out how inappropriate the choice of Unix shell scripting is to implement a test framework, but you probably 😪 of my complaint by now.

@ethomson

ethomson commented Jul 3, 2018

Copy link
Copy Markdown
Member

The more I think about the status quo, the happier that I am with it. Opt-in support for core.symlinks for the system seems like a reasonable strategy. Shall we keep things as-is?

@dscho

dscho commented Jul 3, 2018

Copy link
Copy Markdown
Member Author

Shall we keep things as-is?

Okay!

@dscho dscho closed this Jul 3, 2018
@dscho dscho deleted the fix-core-symlinks-default-setting branch July 3, 2018 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants