Skip to content

Comments

This .editorconfig is incompatible with git on windows.#8656

Merged
dlang-bot merged 1 commit intodlang:masterfrom
TurkeyMan:fix_line_endings
Sep 16, 2018
Merged

This .editorconfig is incompatible with git on windows.#8656
dlang-bot merged 1 commit intodlang:masterfrom
TurkeyMan:fix_line_endings

Conversation

@TurkeyMan
Copy link
Contributor

git core.autocrlf works, and it is default. This setting constantly makes a mess of files, and it's really annoying.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @TurkeyMan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8656"

@TurkeyMan
Copy link
Contributor Author

I'm really tired of carrying local changes to this file around in a stash whenever I'm swapping around and working between multiple branches.

@Geod24
Copy link
Member

Geod24 commented Sep 3, 2018

Isn't autocrlf the exact problem here ? Since it converts LF to CRLF on checkout.

@TurkeyMan
Copy link
Contributor Author

Absolutely not. It's the default and normal way to interact with git. I've been using git for 6-7 years, and this is the only repo I've ever had trouble with.
.editorconfig is objectively they problem.

@TurkeyMan
Copy link
Contributor Author

My guess is that there's few or no windows contributors that use .editorconfig enabled editors.

@rainers
Copy link
Member

rainers commented Sep 3, 2018

When starting using git, autocrlf has been a lot of trouble for me because it caused differences that you could not commit nor revert. Maybe that problem doesn't exist anymore with current versions of git for windows.

I think not messing with newlines is the correct way, but VS sometimes isn't helpful: AFAICT if you paste lines from other sources, it might insert CRLF into a text file that has only LF line endings. .editorConfig doesn't seem to help with that.

@TurkeyMan
Copy link
Contributor Author

Right, .editorconfig just makes a big mess. I have to comment out that line, but it's super annoying to stash/pop changes every time I navigate the repo.

Windows doesn't use LF, so forcing editors to insert LF among CRLF's whenever you press enter, or paste blocks it just a disaster. Occasionally I forget to comment out the line before doing a bunch of work, and then I have to go back and manually correct the mess!

This patch won't break state for LF users either; if you configure git to retain LF when you checkout, competent editors (certainly, any editor that supports .editorconfig) will detect the file's line ending and continue to follow. If you use autocrlf, then all editors work, just as if you're like a normal Windows user ;)

@PetarKirov
Copy link
Member

PetarKirov commented Sep 4, 2018

The only Windows editor that doesn't work correctly is Notepad. All other editors work correctly with \n line ending. No one should ever use core.autocrlf = true even on Windows. I can't stress this enough. The only sane thing is core.autocrlf = input + .editorconfig with end_of_line = lf. I have worked this way for years on Windows projects and is the right thing™ to do.
Please fix your git config!

(Yes, the rule applies to *.sln and *.*proj files as well.)

@rainers
Copy link
Member

rainers commented Sep 4, 2018

(Yes, the rule applies to *.sln and *.*proj files as well.)

I agree, but the .editorConfig is wrong for these files.

I think it is probably ok to actually avoid enforcing LF:

  • on non-Windows OS editors just use LF
  • Windows editors (apart from notepad) usually just adopt to the existing line endings
  • pasting code with CRLF fails in that regard in VS, but .editorConfig doesn't change that (ok in notepad++). autocrlf helps in that case, but I'm not sure what happens if you paste code with LF only.

The only issue might be creating new files, but the build checks will reject these if committed with CRLF (not enforced for test files, though).

@TurkeyMan
Copy link
Contributor Author

@ZombineDev Thanks for the helpful opinions.
You must be very distressed that autocrlf is the default, standard practice, and that everybody ever uses it. It's objectively the 'correct' thing to do on windows.... "I can't stress this enough."

This change won't affect anyone except windows users who do use .editorconfig enabled editors and autocrlf (the default).
I suspect that's likely to be most windows users honestly. I work in a company of 4500, and everyone is configured that way. My prior company was configured that way. My desktop has been configured that way as long as I've used git. My former companies Perforce server is configured that way. It's the normal way; Windows is CRLF.

I have worked this way for years on Windows projects and is the right thing™ to do.

I'm happy for you. I have been vegetarian for years, and I also ride a bicycle to work... it's clearly the right thing™ to do.
I'm not asking you to do that though... but I am asking to not ruin my workflow. This fix has no effect on anyone else unless they are also a windows user with a standard configuration.

Please fix your git config!

I can't fix it. It's already perfect.

The only issue might be creating new files

Not an issue, new files are created CRLF, like they should be, and autocrlf translates to LF when you commit.

I'm not sure what happens if you paste code with LF only.

Everything breaks, and your files are left in a state of ruin >_<

@WalterBright
Copy link
Member

The repo versions of files must be LF terminated, not CRLF. I generally develop on Windows. So how do I manage this?

My checkin script uses tolf on the files before sending them to git. Voila, problem solved.

https://github.com/dlang/tools/blob/master/tolf.d

Please don't check in files with CRLF in them.

@dnadlinger
Copy link
Contributor

It's objectively the 'correct' thing to do on windows....

You are playing with your credibility here.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Sep 5, 2018

You are playing with your credibility here.

Oh I burned that guy alive decades ago ;)

To be fair though, I'm responding in kind to comments like "The only sane thing is: [...]" .. "the right thing™ to do", when it's just incorrect advice.

I'm certain of my claims though; it is objectively correct. I'm not sure it's possible to find your way to a different conclusion if you work it through.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Sep 5, 2018

The repo versions of files must be LF terminated, not CRLF.

Yes, git is configured by default to work well on windows. That's why autocrlf exists, and is default.
It's the normal and intended way to work on windows. That's how your git works right now, unless you have deliberately overridden it (and I can't imagine why).

Windows uses CRLF, and denying that creates problems.
If you paste text from a browser into a source document, or working between repos (3rd party libs?), or copy-pasting between documents. Text in windows should be CRLF for consistent experience, because that's how Windows defines text to be.

Text in git repos is de-facto defined to be LF; and as such, autocrlf is used on Windows, that way Windows users work with Windows text, and git repos store unix text.

Git works perfectly in its intended default configuration, and we don't do anything unusual here that should require special handling. We're just creating a problem where no problem exists.

I generally develop on Windows. So how do I manage this?

I suspect you don't use an editorconfig enabled editor, or you have overridden git to checkout with unix endings. And from there, perhaps you must be careful every time you copy&paste text? Or perhaps your editor performs further intelligent translation on copy/paste...

My checkin script uses tolf on the files before sending them to git. Voila, problem solved.

Well you just said it... that's how you resolve this problem.
That sounds unnecessary though, since git already does exactly that by default.
You shouldn't push an awkward 'solution' onto developers though where no problem actually exists in the first place. There's literally no problem to 'solve' here, other than the mess that's only created by this one line in the .editorconfig file.

The problem with .editorconfig is that it doesn't apply that logic on commit, it applies it while editing, and you accumulate mess in files you edit as you work (at least, that's true in VS's editor).
From there, VS occasionally notices that your file has accumulated mismatching line-endings, and then auto-corrects them, and then the file changes which leads to a project rebuild, which is a really annoying process to be happening constantly.

It also interferes with other aspects of git workflow; since you might perceive your local tree as 'clean' but line endings may be a mess (ie, git status/git diff show 'clean' despite line endings), git then complains when performing various operations about an unclean working tree. If you try and rebase, it initially fails, then you need to reset to fix the line endings, and try again.

Please don't check in files with CRLF in them.

I will never do that... nobody would, that's not how git works, even on Windows.
But that's beside the point; this line in the editorconfig doesn't actually help to that end, because git handles this problem by default. It just creates problems for editors on the Windows side.

The only way you could get a CRLF into a git repo is if someone specifically configures git to commit windows line endings, and in that case, editorconfig can't help you anyway.

@WalterBright
Copy link
Member

I will never do that... nobody would, that's not how git works, even on Windows.

We've had problems from time to time with people checking in CRLF files and generating enormous diffs.

I do nearly all my dev on Windows, with pretty much zero line ending problems. The solution is simple - my text editor (MicroEmacs) recognizes LF, CRLF, and CR all as "end of line". When it writes files, on Windows it writes CRLF, on Linux it writes LF, and on the ancient Mac, it writes CR.

I've been doing this for decades, it works so darned well I'm amazed that anyone uses an editor that works differently. The only brain-dead program I know of that fails at this is Notepad.

But this means that all the files I write on Windows have CRLF. I run tolf to fix that when they're checked in. No problems.

As an aside, I never understood what the various git autocrlf settings. The git documentation for it was apparently translated from Klingon by gerbils. After randomly trying a few settings, and wrecking my git repo multiple times, I abandoned it.

@WalterBright
Copy link
Member

Oh, another stupid problem with git autocrlf. git diff loved to show the entire file was different, rendering it useless. It's also why I never use windows git. I push the stuff to a linux box where my git repo lies. Linux software ported to windows never works right, because the porter never gets these right consistently:

  1. line endings
  2. \ vs / path separators
  3. case sensitivity/insensitivity in file names

DMD, on the other hand, works perfectly with this stuff, because I made damn sure it did :-)

@Geod24
Copy link
Member

Geod24 commented Sep 6, 2018

@TurkeyMan :
It's the default and normal way to interact with git.

Default yes, but it's configurable for a reason.

@WalterBright
I push the stuff to a linux box where my git repo lies.
[...]
I do nearly all my dev on Windows

Those two statements are quite contradictory.

git got line endings right. Millions of people use it without issue, and Microsoft is the biggest organization on Github since a few years. They are not committing CRLF in every projects.

While I don't agree that the workflow @TurkeyMan preaches is the One True Workflow, I don't like the idea of enforcing a workflow if there are sane alternatives.
Since the auto-tester checks for CRLF, I'd be fine with merging this PR.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Sep 6, 2018

The solution is simple ... I've been doing this for decades ... I run tolf to fix that

The process you describe is almost exactly a bespoke implementation of autocrlf.
You don't need to understand autocrlf, or even know it exists at all; on windows, it will just do the right thing by default, and everything will work. There is no need for manually solving this problem, or gymnastics of any kind.

You're welcome to your workflow, but I think you'll find that nobody else will come to that party, and surely, 99.9% of Windows users will operate in the default environment.

Either way, this line in .editorconfig doesn't affect your editor, so this issue has no affect on you.

git diff loved to show the entire file was different

Can you tell how to reproduce that problem? I've never seen anything like that.

If I had to imagine how this could be possible, I think it sounds like this:

  • you have forced checkout to use LF (perhaps you explicitly disabled autocrlf?)
  • your files are LF on checkout, then they all convert to CRLF when you save because your editor does that
  • git now see's the file as different from the one you checked out

Assuming this is the scenario, this problem is self-inflicted. But there's a very simple solution here; reset autocrlf to default which will check them out as CRLF, then they won't appear different when you save them as CRLF.
As a bonus, you will also never need to run your tolf script again, because that's default git functionality. (which I'm suspecting you explicitly disabled such that you find the need to run the script?)
The diff problem, and your tolf script, are necessitated because you've manually configured an unnatural conflict between git and your editor. Git with default settings will not present this issue.

The good news is, all you need to do it git config --global core.autocrlf true, and then everything will be perfect, and you can even stop worrying about running your tolf script.

@Geod24: but it's configurable for a reason.

Sure... but what reason? Definitely not for this reason. I can indeed imagine specific scenarios where you might want to deviate from the default; perhaps your build interacts with some tooling which is not line-ending agnostic. As Walters says, he made absolutely sure our entire ecosystem is line-ending agnostic, so that's totally not an issue here.
By deviating from the standard here, in best case, there's no difference/advantage, worse case; you're making things harder by requiring you maintain some local custom configuration for no reason, and raising the opportunity for your unusual configuration to conflict with other developers who run default configuration (as we see here).

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Sep 6, 2018

the workflow @TurkeyMan preaches

I'm not preaching 'a workflow'. There's nothing remarkable or noteworthy about program defaults.
I'm talking about absolute default configuration of git and editors. This couldn't be more standard if you tried.

I think a good litmus test for any workflows robustness is that it works with default configuration (even notepad). If you want to enhance your workflow to your liking, you're welcome to that, and a good workflow design should support that, but like, if the workflow design fails to account for default configuration, you kinda failed at the gate.

And seriously, this is trivial, and not worth any of our time. Apparently this change will affect nobody other than me... I'm tired of juggling stashes every time I have to amend my PR branches.
Can we move on? I'm still twiddling my thumbs waiting on a bunch of other real patches.

@WalterBright
Copy link
Member

Those two statements are quite contradictory.

Not really. I have a network connection to a linux box, which I ssh into to run git. I don't dev on it, unless there's a problem specific to linux.

Can you tell how to reproduce that problem?

No. It was years ago and I got tired of futzing with it. Perhaps it was this: "which will check them out as CRLF" and then they differed from what was in the repository. I hate **** like that.

everything will be perfect

I don't believe that for a minute :-) And I don't just check in CRLF files. Sometimes its LF files. Sometimes it's files with both CRLF and LF endings. But the result I want is everything in the repository is LF, and everything I check out is LF. Not converted to CRLF. I don't want files in my fork different from the ones in the repository. I really don't know how people put up with that nonsense, it just causes my "red alert" flags to go up.

@WalterBright
Copy link
Member

By the way, does your text editor not have an option to use LF files?

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Sep 6, 2018

I check out is LF. Not converted to CRLF

But you convert to CRLF when you save... so you're still doing the conversion, you're just delaying the conversion. That's the moment where you're shooting yourself int he foot, and only because you didn't do the conversion eagerly.
I understand you don't want CRLF in the repo under any circumstances... of course, this is natural, and normal, and autocrlf is the best defence against it... it will do exactly what your tolf script does when you commit, except it's fool-proof, and it's impossible to forget to run it... and in addition, it will convert to CRLF on checkout, that way it doesn't conflict with Windows or any Windows editor environments, and it also won't perceive your files as 'dirty' when you save,

By the way, does your text editor not have an option to use LF files?

Of course it does, but that's not the issue here.

  1. I don't checkout LF, because if I did...
  2. Text in Windows is CRLF. If you cut and paste from various sources, the clipboard is often populated with CRLF text.
  3. Pasting may often insert CRLF text blocks into LF files.
  4. Or even more annoying, pasting in the other direction may insert LF text into innocent bystander CRLF files.
  5. My particular editor (VS) accepts the file as you write it. It doesn't do a full-file conversion to LF on save... and even if it could (it can) I certainly wouldn't configure it that way, because I don't want LF files anywhere in sight on a Windows machine (see DMD Patch to fix build on OpenIndiana (and Solaris) #2-4).
  6. I use a variety of editors, and while I could coerce VS to do the thing, I don't want to have to coerce ALL my editors to do the thing.

I don't want problems with line endings. This .editorconfig file is manifesting such problems out of nowhere for absolutely no reason. Such problems don't otherwise exist.

I work on a whole lot of projects, and zero of them require this sort of shenanigans. I don't like configuring software, that's just tedious an time consuming. I should be able to format my PC, reinstall the things and get to work. I work across far too many different computers (many of which are corporate controlled and have standard operating environments), to resist the defaults. I don't want to configure software in a non-standard way to interact with a non-standard environment. Configuring my software in a non-standard way would just create friction on every other project, which deserve a lot more of my time.

dlang does NOT have a non-standard environment, quite the contrary, it has an absolutely couldn't-be-more-standard environment... all except for this .editorconfig file which specifies a single line that manifests this issue. It doesn't need to do that. It isn't helping anyone.

@jacob-carlborg
Copy link
Contributor

I push the stuff to a linux box where my git repo lies.

On Windows 10 there’s the Subsystem for Linux, you should give it a try. It runs the same ELF binaries as on Linux.

@TurkeyMan
Copy link
Contributor Author

It's also why I never use windows git. I push the stuff to a linux box where my git repo lies.

Sorry, I think I just understood this text... do you mean you 'push' (as in git push) to linux, or are you saying you somehow get your windows files to a linux machine where you operate git from the linux git installation there? O_o

@jacob-carlborg
Copy link
Contributor

Since he doesn't use Git on Windows it can't be git push.

@WalterBright
Copy link
Member

and in addition, it will convert to CRLF on checkout

That part I don't want. I don't care for magical conversions. When I get a file from git, I want it to be the same file that is in git.

@WalterBright
Copy link
Member

are you saying you somehow get your windows files to a linux machine where you operate git from the linux git installation there?

Yes. And the 'somehow' is pscp from https://www.putty.org/ It's quite a handy program. I have a little script called putfork.bat:

detab %*
tolf %*
\putty\pscp -i c:\ssh\something.ppk %* name@machine:forks/dmd/src/dmd/

@Geod24
Copy link
Member

Geod24 commented Sep 6, 2018

@WalterBright :

git config --global core.autocrlf false
git config --global core.eol lf

This will achieve exactly what you want.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Sep 6, 2018

That part I don't want. I don't care for magical conversions.

'Magical conversions' like your editor converting to CRLF on save, or tolf converting to LF when you run it?
I mean, it's no more-or-less magical; executing git checkout is not really magic, and git commit is also not magic... you manually execute git commit just as deliberately as executing tolf.

But you're right; autocrlf will not work for you if you're running from a linux machine. The 'auto' in that case would convert the repo's LF to the linux workstation's native LF... ie, no-op.

When I get a file from git, I want it to be the same file that is in git.

But then you said you just convert it to CRLF anyway...? This is the moment where every line of your diff becomes dirty.
Anyway, whatever works for you, but I don't think it's reasonable to criticise my everything-set-to-working-defaults strategy as complicated ;)

It sounds like you could reduce the number of steps in your workflow a whole lot if you ran git from windows.

git `core.autocrlf` works, and it is default. This setting constantly makes a mess of files, and it's really annoying.
@TurkeyMan
Copy link
Contributor Author

Why is there here for 2 weeks?
Merge it or close it...

@rainers
Copy link
Member

rainers commented Sep 16, 2018

I'm inclined to merge this:

  • non-windows users are not affected
  • people using .editorConfig agnostic editors are not affected
  • editing existing files should keep and use the current line endings in most editors (VS is a bad exception here, but I've modified Visual D to support this for D files, too, including pasting code from other sources).
  • new files with bad line endings will be rejected by the auto-tester
  • some Windows editors/tools don't work too well with LF only (e.g. notepad).
  • git workflow regarding autocrlf is not enforced on user

@Geod24
Copy link
Member

Geod24 commented Sep 16, 2018

I agree with Rainers here. I won't be affected by this change and I can't see how anyone would be affected in a negative way.

@Geod24 Geod24 closed this Sep 16, 2018
@Geod24 Geod24 reopened this Sep 16, 2018
@Geod24 Geod24 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Sep 16, 2018
Copy link
Contributor

@dnadlinger dnadlinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is not an endorsement of autocrlf – which is still the wrong thing to do, and in my experience leads to a life of flames and agony –, but we can remove this piece of auto-configurarion and rely on users to set their editors correctly instead.

@dlang-bot dlang-bot merged commit f91ad10 into dlang:master Sep 16, 2018
@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Sep 16, 2018

which is still the wrong thing to do,

Hehe, what was that thing you said about credibility? :P

@TurkeyMan TurkeyMan deleted the fix_line_endings branch September 19, 2018 06:11
dlang-bot pushed a commit that referenced this pull request May 28, 2020
The comment is outdated since #8656 was
merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merge:auto-merge Merge:72h no objection -> merge The PR will be merged if there are no objections raised.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants