-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Ensure that shell scripts and Dockerfiles use Unix line-endings #5949
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on that link isn't it easier to just add core.autocrlf = input
to your gitconfig?
.gitattributes
Outdated
# Auto detect text files and perform LF normalization | ||
* text=auto | ||
|
||
*.sh eol=lf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only these files? We only use unix line-endings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonistiigi Because most other text files, like source-code, are expected to have CRLF line endings on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @tonistiigi — enforcing LF consistently is important.
I usually work in teams with a mix of Windows and Linux devs, and setting the following helps avoid cross-platform line-ending issues:
In .gitattributes
:
* text = lf
And in .editorconfig
:
root = true
[*]
end_of_line = lf
This makes sure both Git and editors enforce LF, which avoids diffs from accidental CRLF on Windows. This ensures editors (like VS Code, IntelliJ, etc.) also respect LF line endings, regardless of local defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I read, not all Windows editors support LF endings but you're right that all the major coding editors certainly do.
I'm fine with defaulting to LF endings if you guys prefer. Let me know if you want me to update the PR accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change it to lf
for all text files if we want to get this in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Try now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexgwolff PTAL
@cowwoc Please squash commits as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually work in teams with a mix of Windows and Linux devs, and setting the following helps avoid cross-platform line-ending issues
text
attribute only affects check-in normalization, which while important does not prevent files being normalized to CRLF on checkout for Windows by default. You will need to set eol=lf
for check-out. Usually there are only a few files that explicitly require this so it's better to be explicit IMO.
The editorconfig is complimentary, but last I recall wasn't used by default by VSCode without a plugin/extension active 🤔 (perhaps that has changed since, or you just haven't had to think about it since it's set and forget), I vaguely mention it here:
Another alternative is leveraging our
.editorconfig
with an editor that supports it (such as VSCode with the official extension installed).This only applies the EOL conversion upon save however, thus not foolproof of working-tree gotchas.
On Windows a new blank file would default to CRLF last I recall, that's partially helpful should you run any tests before committing changes you make.
- However, like
text=lf
/text=auto
in.gitattributes
it wouldn't switch to LF from CRLF when you checkout a commit on Windows, you'd have to open the file and save it (which won't be visible to git as a difference since it's only concerned with line-endings when the changes are staged). - If you do get LF at checkout and haven't adjusted git config on Windows then whatever you're using to perform the checkout would be doing that for you.
Because most other text files, like source-code, are expected to have CRLF line endings on Windows.
From what I read, not all Windows editors support LF endings but you're right that all the major coding editors certainly do.
It shouldn't be an issue these days. Only when a file should be CRLF (.bat
) or LF (.sh
/ Dockerfile
with Linux base image) does it really matter and these are affected by eol
attribute.
You use the text=lf
/ text=auto
attribute for git check-in normalization since by default an editor may default to CRLF (VSCode can do this when creating a new file), and some editors may not normalize the file to a specific line-ending IIRC, this can result in mixed line-endings which is more problematic, normalizing line-endings at check-in resolves that (unless files are already committed with mixed line-endings).
In my review comment I provided additional links to back up what I'm saying, since all involved in this review discussion thus far don't seem to understand .gitattributes
well enough.
This:
# Only set `text=lf` if file appears to be text content:
* text=auto
# Check-out files with the `.sh` extension as LF, even on Windows (default CRLF):
*.sh eol=lf
is not equivalent to:
# Commit every file with line-endings bytes normalized to LF:
* text=lf
# Unset `text=lf` via `binary` attribute (aka `-text -diff`):
*.gif binary
The change suggested/applied is not an improvement but a regression to the PR.
Signed-off-by: Gili Tzabari <cowwoc2020@gmail.com>
e5bce49
to
4c48984
Compare
missing |
Signed-off-by: Gili Tzabari <cowwoc2020@gmail.com>
@alexgwolff Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: You might want to consider eol=lf
too, but I'm not sure if that's ok as * text=auto eol=lf
if it could affect binary files 🤔
For additional reference if anyone is interested about .gitattributes
, I documented it rather thoroughly (related PR for more info).
You'll notice that while * text=auto
does the equivalent of * text=lf
with binary detection (thus the .gif
line is probably not necessary if you trust git to distinguish between binary/text content), that I still have files that need to be lf
are assigned an explicit eol=lf
attribute.
That is because text=lf
/ text=auto
is only adjusting to LF on check-in. That will not ensure files are checked out via git with LF on Windows, you'd still get CRLF by default (Dockerfile
using RUN
instructions with HereDoc strings fails without LF for example, another is interactions with formatting tools). See my earlier linked reference notes for more context.
If CRLF has been committed in the past, you may need to perform a normalization to convert those to LF. It is possible some files may even have mixed line-endings (as I encountered before, link provides full details and solution).
@@ -0,0 +1,4 @@ | |||
* text=lf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* text=lf | |
# Normalize line endings of all non-binary files to LF upon check-in (`git add` / `git commit`): | |
* text=auto |
NOTE:
- This should technically make the
*.gif binary
line redundant. text=auto
/text=lf
only affects check-in. Should anyone check-out on Windows it'll likely normalize to CRLF (only matters if they try to run/process files with CRLF that should otherwise be LF).
[*] | ||
end_of_line = lf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[*] | |
end_of_line = lf | |
[*] | |
charset = utf-8 | |
end_of_line = lf | |
insert_final_newline = true |
@@ -0,0 +1,8 @@ | |||
# https://editorconfig.org | |||
|
|||
# top-most EditorConfig file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# top-most EditorConfig file | |
# Treat this config file as the top-level (root) EditorConfig file: |
I'm fine with the proposed changes if the rest of you agree. While we're on the topic, what (else) is preventing this PR from finally getting merged? |
No description provided.