Skip to content
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.

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

Various system gitconfig-related fixes, in particular for MinGit #267

Merged
merged 3 commits into from
Nov 8, 2019

Conversation

dscho
Copy link
Member

@dscho dscho commented Nov 8, 2019

I made a mistake when I added those include.path settings to MinGit's system config which are intended to inherit the installed Git for Windows' defaults: these includes are added to the beginning, which means that MinGit's hard-coded core.autocrlf and friends always override Git for Windows' defaults. Which defeats the purpose.

Let's fix that, and while at it also remove the no longer needed http.sslCAInfo from the system gitconfig of Git for Windows' SDK (the installer will still set it, to help Git LFS use Git for Windows' certificate bundle).

dscho added 3 commits November 8, 2019 09:11
The original idea of the now-discontinued support for
`C:\ProgramData\Git\config` was to have a central place where Git for
Windows writes the settings configured via its installer, so that e.g.
MinGit can pick them up.

This is particularly important when the user changed the default of e.g.
`core.autocrlf` because running `git status` with the Git Bash and with
MinGit would otherwise produce inconsistent results.

To continue with this idea, MinGit now adds two `include.path` settings
that pick up the system gitconfig of the 32-bit and the 64-bit Git for
Windows, respectively, if any exist.

There is just one problem with that: we include those system gitconfigs
_first_, and then configure things like `core.autocrlf`... which
overrides Git for Windows' settings.

Let's instead include Git for Windows' system gitconfig _after_ setting
our defaults.

Signed-off-by: Johannes Schindelin <[email protected]>
We patch the cURL shipped in Git for Windows since
git-for-windows/MINGW-packages@2e8f4580eb4d to
allow it to find the `ca-bundle.crt` file automatically, without the
need for `http.sslCAInfo` to be set.

So let's no longer initialize that config setting when `git-extra`'s
post-install script detects that there is no system gitconfig yet.

Signed-off-by: Johannes Schindelin <[email protected]>
The previous commit stopped initializing the system gitconfig with that
setting that we no longer need.

With this patch, we now actively unset this setting in existing
gitconfigs.

Note: we are extra careful _not_ to use `git config` to do the editing
so that we allow for users who absolutely definitely want to have that
setting (they just need to add an extra space or something like that).

While at it, we also remove the hack where we installed a symlink
`/ssl/` -> `/usr/ssl/` to allow for sharing that setting between the
MSYS and the MINGW version of `git.exe`.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the mingit-system-gitconfig-fix branch from b1df236 to 27678c3 Compare November 8, 2019 09:05
@dscho
Copy link
Member Author

dscho commented Nov 8, 2019

@shiftkey could you have a look?

@shiftkey
Copy link
Contributor

shiftkey commented Nov 8, 2019

I think this is fine for GitHub Desktop's packaging, as we unset http.sslCAInfo and set http.schannelUseSSLCAInfo to false on our end

https://github.com/desktop/dugite-native/blob/9ce314bb2dcaa233a3d576f4b797ca2a57c11808/script/build-win32.sh#L74-L86

cc @outofambit @niik for visibility

@dscho dscho merged commit cf72484 into git-for-windows:master Nov 8, 2019
@dscho dscho deleted the mingit-system-gitconfig-fix branch November 8, 2019 13:51
dscho added a commit that referenced this pull request Nov 8, 2019
MinGit [no longer overrides an installed Git for Windows' system
gitconfig](#267).

Signed-off-by: Johannes Schindelin <[email protected]>
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