Page MenuHomePhabricator

[doc] Show the git config for Windows to do line-endings correctly
ClosedPublic

Authored by probinson on Jun 22 2018, 10:38 AM.

Details

Summary

Failing to do this before cloning causes various hassles that are easily avoided.

I thought about putting this in the getting-started-with-windows page, but it seemed better to put it right with the git info.

Should it also be listed in the mono-repo section? That's probably the more popular method now.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson created this revision.Jun 22 2018, 10:38 AM

I actually strongly disagree with this guidance. You should use
core.autocrlf false. A source control system should not be modifying the
content of the sources. There are some tests that specifically depend on
crlf and some that specifically depend on lf. All reasonable editors will
correctly maintain line endings, and if someone accidentally checks in crlf
line endings it’s noticed and fixed pretty quickly

You should use core.autocrlf false.

Isn't that the default? Whatever the default is, I promise you that it fails miserably.

I’m not sure what the default is (i think it’s true), but it’s definitely
not false.

What problems do you see with the default? I’ve been using false for years
and never had a problem, and many other open source projects require it
because having the files on your disk not match the files in the repo
creates its own set of issues

https://stackoverflow.com/questions/10418975/how-to-change-line-ending-settings

Shows the 3 different settings. I believe #1 is default. You are proposing
#2, which is better but still incorrect in the presence of input files that
are purposely crlf (in case we need to test that our tools can read them).

I'm sure that I had 50-some clang test failures until I set autocrlf=input and forced a new checkout.
I probably also had the wants-to-commit-the-universe problem but I'm less clear on that; this was 2-3 months ago.

Using false might work. I'll try a brand new clone and see what happens.

I mostly agree with Zach. In practice, setting core.autocrlf to false on Windows seems the most practical solution.

One could argue that test files that depend specifically on CR+LF or LF should be saved as binary files, in which case true might work, but I think it'd be hard to get to that point for relatively low value. As I recall, we have tests where if the test file gets switched to the other scheme, the test passes and effectively gives us no feedback.

I like the idea of input, but I don't have enough experience with it to know if it would actually be practical. It seems like a good idea for that to happen once on new files, but not on every commit that updates a file.

So, input is like false on checkout, and fixes crlfs on checkin. The set of test files with actual CRLFs that are required is pretty small, I would think.

We settled on input in my team because it caused the least problems all around. Zach may sneer at me for not using a "reasonable" editor but "reasonable" means "works the way *I* want" and is not particularly useful guidance. The question is, what editors don't produce files with mixed LF/CRLF endings, either by auto-detecting or being told what to do; if there are such, we can add that kind of guidance as well.

Well by reasonable I meant “I don’t know of any other than notepad that
won’t autodetect or provide a global setting”. What are you using?

Personally I've been using the Visual Studio editor. I don't know what other people on the team use. I do know we had some CRLF issues at one point. It has been a long time so I don't remember the details.

That’s what i use. It will save by default in whatever it autodetects. New
files are crlf by default, but adding a new file is relatively rare. I just
run dos2unix on them before committing

Awesome. I'm fine with changing the advice to use false then. Anything else?

rnk added a comment.Jun 22 2018, 12:49 PM

I personally use false, but maybe input would be a good way to go going forward. Honestly, let's go ahead and recommend input. Even if our .gitattributes files don't say the right thing today, we can fix that over time, and then things will be more likely to work out of the box.

"autocrlf true" is obviously broken (I think it causes LLVM tests to fail), so we need to recommend something. It might as well be the way forward that doesn't rely on new contributors all configuring their editors correctly.

I’m personally against recommending input for the aforementioned reasons

You guys duke it out and let me know.

You guys duke it out and let me know.

Although IMO, machines should do as much of the dull stuff as possible (e.g., remembering to convert CRLF to LF on new files, and leaving CRLF in place on the files that need it). So I lean to input and the right set of git attributes on the necessary files, for that reason.

Sorry for wading in here, but would it be possible to come up with a solution that would permit both a windows side visual studio and wsl side linux builds to share a single git folder (prefererably stored/maintained on the windows side)? Being able to test a patch on msvc/gcc/clang from a single machine is bliss, but in my previous feeble attempts to get this to work its been autoclrf or whatever that caused problems with either the build or the lit tests and I've just ran away screaming.

Simon,

I do exactly that with autocrlf false.

hans added a comment.Jun 25 2018, 12:51 AM

I've only ever had bad experiences from the version control system trying to fix line endings. I'd prefer recommending people staying with the default, core.autocrlf = false.

The default is actually true on Windows I think. I know I always have to
manually set false.

probinson updated this revision to Diff 153159.Jun 27 2018, 1:01 PM

Recommend false instead of input.

zturner accepted this revision.Jun 27 2018, 1:01 PM
This revision is now accepted and ready to land.Jun 27 2018, 1:01 PM
This revision was automatically updated to reflect the committed changes.