ship core.autocrlf=true value in dugite-native for Windows

stuartpb commented 6 years ago

This was a useful feature in GitHub for Windows, which was built on an internal version of msysgit and, as such, had its own global Git configuration that could have autocrlf globally defaulted on.

It looks like GitHub Desktop is designed to only respect .gitattributes in a repository for CRLF handling (initializing every repository it creates with one); it would be nice if there were an option to enable automatic CRLF translation when .gitattributes is not present (an option that, I would add, should be enabled by default, as it was in GitHub for Windows), as I have a lot of existing repositories without .gitattributes that expect the platform's Git client to manage this itself (a reasonable expectation that, again, was followed by For Windows).

stuartpb commented 6 years ago

Also, I'm a little bit baffled why prior discussion around this such as #1906 and #1964 doesn't consider this as the simplest way to resolve this problem, one significantly simpler than requiring every single repository you come across to take Windows' weird line-ending behavior into account.

shiftkey commented 6 years ago

@stuartpb thanks for the feedback!

First off, some context/history:

the new Git distribution we're using in Desktop is over at desktop/dugite-native which does ship a simple system gitconfig (only on Windows, I have some headaches in https://github.com/desktop/dugite-native/pull/73 about getting this working on other platforms)
as this package is being used across Windows, macOS and Linux I wanted to avoid as much platform-specific differences as possible - we no longer ship a Git Shell on Windows, which was what much of the system gitconfig was about
if you think we should add back core.autocrlf=true on Windows, that's where we need to fix it

Also, I'm a little bit baffled why prior discussion around this such as #1906 and #1964 doesn't consider this as the simplest way to resolve this problem, one significantly simpler than requiring every single repository you come across to take Windows' weird line-ending behavior into account.

To be honest, I'd forgotten what was in that system config. For reference, it's still available here.

I just checked what the vanilla Git for Windows ships currently for their system gitconfig and it contains these values in it's "shared" location under %PROGRAMDATA%:

file:"C:\\ProgramData/Git/config"       core.symlinks=false
file:"C:\\ProgramData/Git/config"       core.autocrlf=true
file:"C:\\ProgramData/Git/config"       core.fscache=true
file:"C:\\ProgramData/Git/config"       color.diff=auto
file:"C:\\ProgramData/Git/config"       color.status=auto
file:"C:\\ProgramData/Git/config"       color.branch=auto
file:"C:\\ProgramData/Git/config"       color.interactive=true
file:"C:\\ProgramData/Git/config"       help.format=html
file:"C:\\ProgramData/Git/config"       rebase.autosquash=true

@stuartpb I'm pretty sure if you have Desktop installed alongside Git for Windows, you'll get these values. But for those who just have Desktop this file isn't created. Some of these are for UI operations, but the first three make sense for us to enable in Desktop:

file:"C:\\ProgramData/Git/config"       core.symlinks=false
file:"C:\\ProgramData/Git/config"       core.autocrlf=true
file:"C:\\ProgramData/Git/config"       core.fscache=true

If you agree, let's move this to desktop/dugite-native and prepare a new release of Git with these enabled.

stuartpb commented 6 years ago

That all sounds good to me.

Under that plan, the issue on this side should still be kept around, to track introducing a UI toggle for the global core.autocrlf setting (since there are inevitably going to be some developers out there who want CRLFs in their repos implicitly, and fixing this issue for the cross-platform developers without making it possible to opt out would be a problem for them).

stuartpb commented 6 years ago

@stuartpb I'm pretty sure if you have Desktop installed alongside Git for Windows, you'll get these values.

Are you sure that's where dugite-native looks for its global configuration? I can't really tell if Desktop's telling me it's going to respect core.autocrlf or not, due to the confusing messaging I described in https://github.com/desktop/desktop/pull/1906#issuecomment-359083709.

shiftkey commented 6 years ago

Are you sure that's where dugite-native looks for its global configuration?

We build upon Git for Windows - please refer to it's documentation around config files.

If you define this globally in your ~/.gitconfig that will override whatever exists in system configuration.

shiftkey commented 6 years ago

Under that plan, the issue on this side should still be kept around, to track introducing a UI toggle for the global core.autocrlf setting (since there are inevitably going to be some developers out there who want CRLFs in their repos implicitly, and fixing this issue for the cross-platform developers without making it possible to opt out would be a problem for them).

The only global Git configuration we expose in the app UI currently is user.name and user.email, which are important for committing. Please open a fresh issue so we can discuss this further separate to the underlying Git work.

I'm going to update the title to reflect this is about the system configuration changes.

stuartpb commented 6 years ago

If the Git that GitHub Desktop is shipping right now actually respects the global core.autocrlf setting that would have been set up by GHFW, I've still got an issue with the way that's communicated I mentioned earlier in https://github.com/desktop/desktop/pull/1906#issuecomment-359083709: I can file a new issue for that.

The only global Git configuration we expose in the app UI currently is user.name and user.email, which are important for committing.

Meh, apparently this is the same case as Github for Windows: I could have sworn there used to be UI chrome for this there, but I just looked now and, if there used to be a configurator for it, it's gone now. (I thought there used to be an "edit global git config in text editor" button, too, but again, if that used to be the case, it appears to have vanished by the current release.)

Whatever - as long as developers who want to turn off core.autocrlf have some way of doing it, I'm fine with the prospect of them doing it by doing a Google search for "GitHub Desktop core.autocrlf false" or whatever, finding this issue describing what to change and where, and making the change manually: the un-cross-platform-friendly behavior they'd be setting is pretty much an edge case, as far as I'm concerned, and one I'm fine with the prospect of making hard to do.

shiftkey commented 6 years ago

If the Git that GitHub Desktop is shipping right now actually respects the global core.autocrlf setting that would have been set up by GHFW, I've still got an issue with the way that's communicated I mentioned earlier in https://github.com/desktop/desktop/pull/1906#issuecomment-359083709

Oh, I missed that comment.

.gitattributes will supersede whatever you have configured for core.autocrlf, so that makes sense that it reports a warning.

The intent of the rule for * text=auto is mentioned here:

When text is set to "auto", the path is marked for automatic end-of-line conversion. If Git decides that the content is text, its line endings are converted to LF on checkin. When the file has been committed with CRLF, no conversion is done.

Note that this is very similar to core.autocrlf=true. From the docs (emphasis mine):

Setting this variable to "true" is the same as setting the text attribute to "auto" on all files and core.eol to "crlf". Set to true if you want to have CRLF line endings in your working directory and the repository has LF line endings. This variable can be set to input, in which case no output conversion is performed.

Does that mean I'm about to commit the file, which has LF line endings in the repo, with CRLF line endings if I commit this change?

The warning message here suggests the opposite - the working directory file on disk has CRLF line endings, but the .gitattributes file suggests that it was expecting LF. This often happens when adding a new .gitattributes file to the repository which was previously managed by config values, as Git will need to normalize it's index (where it tracks the differences between it's object database and the working directory on disk) .

We have a helpful guide on GitHub for guiding you through the process of normalizing a repository after adding a .gitattributes file. I'd recommend going through that to resolve these warnings.

shiftkey commented 6 years ago

@stuartpb if some files must be committed with CRLF endings, that can be specified in your .gitattributes file too:

*.vcproj    text eol=crlf

stuartpb commented 6 years ago

The warning message here suggests the opposite - the working directory file on disk has CRLF line endings, but the .gitattributes file suggests that it was expecting LF.

Yeah, and I'm saying there appears to be a bug somewhere in here: if the core.autocrlf=true that's set on my system is being respected (meaning that the index should already have and expect the working file with CRLFs to be translated to LFs), then this message is in error; if the message isn't in error, then the core.autocrlf=true that's in my global config wasn't being respected.

stuartpb commented 6 years ago

To be clear, the .gitattributes I was adding to the repository should have been redundant: I added it to see if it would change the presence of this warning (ie. if the global core.autocrlf=true wasn't recognized but * text=auto in .gitattributes would be), which it didn't.

shiftkey commented 6 years ago

@stuartpb it depends on a few things, like what core.autocrlf was when the repository was cloned. If you cloned it through Desktop and didn't have core.autocrlf=true set, Git will fall back to whatever core.eol is - and that will default to native (which is CRLF on Windows). That's what I suspect has happened here.

Also, subsequent config changes won't affect the working directory files unless you force Git to checkout the files again. To force this, delete your index and reset the repository:

rm .git/index
git reset

shiftkey commented 6 years ago

If you don't think that's the case, please open a new issue with details about your global configuration and steps to reproduce it (getting access to the problem repository would also be ideal).

stuartpb commented 6 years ago

Okay, I've tracked down the issue, and I'll file a new ticket for it in a second, but the actual source of the problem is that GfD displays this warning in the presence of a CRLF translation rule (ie. core.autocrlf=true, and probably * text=auto in .gitattributes too) when a file in the working directory has LF line endings (which won't cause a problem, and is basically the direct opposite of what the warning describes).

Wait... this might be the result of core.safecrlf being defined, which I'm pretty sure it was in earlier releases of GHfD, and as such would be in the config created from when I originally installed it. Let me just check if that's set...

stuartpb commented 6 years ago

Okay, so, the important thing to note here is that I don't have a C:\ProgramData\Git directory, and when I open up the Git Shell included with GitHub for Windows and run git config --list --show-origin, this is what I get for system variables:

file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   alias.c=commit
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   alias.co=checkout
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   alias.dt=difftool
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   alias.mt=mergetool
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   alias.praise=blame
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   alias.ff=merge --ff-only
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   alias.st=status
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   alias.sync=!git pull && git push
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   apply.whitespace=nowarn
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   core.symlinks=false
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   core.autocrlf=true
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   core.editor=gitpad
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   core.preloadindex=true
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   core.fscache=true
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   color.diff=auto
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   color.status=auto
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   color.branch=auto
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   color.interactive=true
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   color.ui=true
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   pack.packsizelimit=2g
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   help.format=html
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   http.sslcainfo=/ssl/certs/ca-bundle-ghfw.crt
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   diff.astextplain.textconv=astextplain
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   rebase.autosquash=true
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   credential.helper=!github --credentials
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   filter.ghcleansmudge.clean=cat
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   filter.ghcleansmudge.smudge=cat
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   push.default=upstream
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   diff.tool=vs2013
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   diff.algorithm=histogram
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   difftool.prompt=false
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   difftool.bc4.cmd="c:/program files (x86)/beyond compare 3/bcomp.exe" "$LOCAL" "$REMOTE"
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   difftool.p4.cmd="c:/program files/Perforce/p4merge.exe" "$LOCAL" "$REMOTE"
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   difftool.vs2012.cmd="c:/program files (x86)/microsoft visual studio 11.0/common7/ide/devenv.exe" '//diff' "$LOCAL" "$REMOTE"
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   difftool.vs2013.cmd="c:/program files (x86)/microsoft visual studio 12.0/common7/ide/devenv.exe" '//diff' "$LOCAL" "$REMOTE"
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   merge.tool=bc3
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   mergetool.prompt=false
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   mergetool.keepbackup=false
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   mergetool.bc3.cmd="c:/program files (x86)/beyond compare 3/bcomp.exe" "$LOCAL" "$REMOTE" "$BASE" "$MERGED"
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   mergetool.bc3.trustexitcode=true
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   mergetool.p4.cmd="c:/program files/Perforce/p4merge.exe" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
file:"C:\\Users\\Stuart\\AppData\\Local\\GitHub\\PortableGit_f02737a78695063deace08e96d5042710d3e32db\\mingw32/etc/gitconfig"   mergetool.p4.trustexitcode=false

My release is Chocolate-Covered Yaks (3.3.4.0) 50415df, and the About page I'm looking at is also calling itself GitHub Desktop (even though it's nothing like the Electron-based GitHub Desktop I just installed recently)?

shiftkey commented 6 years ago

@stuartpb that's interesting, but those system config settings won't be applied to the Desktop repository when working with it in the Electron UI. What's in your ~/.gitconfig file?

stuartpb commented 6 years ago

Just my user info and LFS settings from somewhere.

Oh, you were talking here about the config Git for Windows (ie. this) ships, not the one GitHub for Windows (ie. the old GitHub Desktop) ships, that's why I was confused when you were saying that GHD should inherit the config from GfW - I was talking about the latter, you were talking about the former.

shiftkey commented 6 years ago

Ah yes. I didn't realise you were running the classic and Electron builds side-by-side on the same machine. If you're cloning repositories through Desktop, they won't get the config values defined in Git Shell (that's what we call the shell environment in the classic Desktop app). That's because our system gitconfig lacks these values and will only inherit from %PROGRAMDATA%\Git\config.

To workaround this issue, I recommend setting your desired values for core.autocrlf and core.safecrlf in your global ~/.gitconfig before doing any subsequent clones until we publish a Git update that contains https://github.com/desktop/dugite-native/pull/77.

Let me know if there's anything else to clarify around this area.

stuartpb commented 6 years ago

Let me go ahead and install Git for Windows and see how that impacts the behavior I'm seeing: I suspect there's still some sort of mismatch between the Git behavior and the warning messages (because one of the files I was getting warnings about CRLFs for has LF line endings in its working directory), but I wouldn't want to be filing bugs only present in a mode of operation that's about to be obsoleted.

Also, this answers one of the things I've been confused about since I started looking at the new GitHub Desktop, ie. why it doesn't ship a Git Shell: sounds like the intention now is for any users who need the full Git shell to install Git for Windows separately (which I would agree is a better approach than the expose-the-shell-for-the-vendored-in-Portable-Git-with-each-release approach taken by the old GHD).

shiftkey commented 6 years ago

@stuartpb we had a long discussion about this in #340 and added steps to get Git for Windows as close to Git Shell as possible in https://github.com/desktop/desktop/issues/340#issuecomment-302021683. We've decided to focus our resources on the UI side of Git and help support upstream efforts wherever possible, rather than competing with Git for Windows.

stuartpb commented 6 years ago

I just installed Git for Windows (with its autocrlf config), and I'm still seeing the out-of-place warnings I described earlier (saying line endings have changed specifically for files where they haven't). I'll file a new issue with repro steps for that.

desktop / desktop

ship core.autocrlf=true value in dugite-native for Windows #3828