fix: Ensure files are committed with eol=lf via .gitattributes
#3527
Conversation
Both of these files support the `.yml` extension. Normalize on that.
Avoids accidentally committing files with `CRLF` when they're created on Windows. Or worse, if some editors don't detect `LF` and would introduce mixed line-endings with `CRLF`. Shouldn't be a problem in practice as we already have a linting check to catch this via CI during PRs. This file is complimentary, in that it should automate that concern away.
polarathene
added
area/ci
kind/improvement
|
Disclaimer: Not going to block this / discuss to death 😆 I read a bit about .gitattributes and have mixed feelings. Personally I don't like it, when files are automatically changed (LF/CRLF in this case) on commit/checkout. If there are erroneous line endings, I prefer to get to know this by a failed lint test. So that I am informed that something did not fit and I can make it better in the future - instead of getting it silently fixed. Every modern editor should be able to handle LF/CRLF correctly. It feels weird, to convert/alter all text files on checkout to another line ending (CRLF, for example on windows). And later after the work is done, convert it back to LF when commiting. Instead, a proper editor should be used, so that converting beforehand is not necessary at all.
If the first 1K bytes contains a null byte, it's considered binary. BTW: Any idea, why the mentioned test failure wasn't caught by eclint? In .editorconfig we have I would have expected, that all text files are checked for the correct (LF) line ending. |
|
TL;DR:
I had read a lot on the topic myself and have had a few experiences where it was problematic over the years. To be fair, I've been very verbose/explicit in the config here so you aren't really getting any surprises. It's similar to the linting that would catch it, but if you commit the change then the CI won't pick up on it via PR as the issue was prevented. This just ensures that line endings are converted to LF during commit, as you really don't want CRLF (especially mixed) in the repo. If the file does not explicitly have Especially since this is a project focused on files being copied into a linux container image, you have very little reason to want CRLF there. It will cause weird bugs like
The lint should work that way if you run it before committing. I often forget to, but since we contribute via PR instead of direct push to master it can be prevented sneaking into master. I had no idea when the CI tests failed initially as the lint test was bailing at ShellCheck before EditorConfig, plus some tests were intermittent/unrelated test failures, so line nedings didn't cross my mind until EC Lint pointed it out. That said I'm working on rewriting a docs page. Normally these say LF, but I just noticed that it's reporting CRLF in the editor right now, possibly it's mixed endings and after rewriting more than half of the page it detects as CRLF now, or perhaps it changed when I saved it 🤷♂️ If you're not working on Windows you're not going to have this surprise gotcha, it's better to prevent it for those that do, lints are great, but leveraging this git feature to fix all files when they're committed is even better.
I'm using VSCode, while for git it's one of the few other dev tools I like to use a GUI for instead of the CLI, I use GitKraken there. I consider both of these modern and quite capable. In both cases I think they're just respecting the Git defaults, which is to use CRLF unless told otherwise. Regardless, we get other contributors and can't expect it'd be any better on their end. The CI lint tests will still catch that, but I can't think of a reason where
It did catch it, like mentioned above. Just requires the linter to reach that check before failing. https://github.com/docker-mailserver/docker-mailserver/actions/runs/6070286374/job/16466057026 The test run before that one skipped ECLint due to ShellCheck failure: https://github.com/docker-mailserver/docker-mailserver/actions/runs/6068011319/job/16460416288 While the test-case was failing it wasn't for that same reason as I hadn't yet adjusted for the white-space difference, so I wasn't confused at that point either. |
|
Thanks for the detailed explanation 👍 |
Description
Slight project improvement, mainly beneficial if developing from a Windows machine.
Open to alternatives:
* text=auto.CRLFcheck-in is probably a good thing though?).Although our EditorConfig linter check can catch this mistake, it's probably useful to have a
.gitattributesfile to automatically convert toLFwhen necessary? (Default allows checking inCRLFinto the repo, as this non-obvious CI test failure running on Linux demonstrates).The current commit is a bit overkill / verbose:
gitto make the decision with just* text=autoand rely on tests and linting to communicate failures? 🤷♂️gitwill decide if a file is binary or not by it's content, and since we already git diffs generated for all relevant text files, those are not treated as binary.The inline docs at the bottom, I'm not sure what your opinion is on keeping them. It's harmless and not really going to bother anyone, while providing a decent refresher/overview of the file format and rules (vs the upstream docs). They are revised from an earlier
.gitattributesI wrote years ago.Additional reference:
Type of change
Checklist: