Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add repo files to help maintain conventions #715

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+25 −0
Open

Add repo files to help maintain conventions #715

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7580373
Add repo files to help maintain conventions
AArnott Sep 8, 2023
8629e5f
Address PR comments
AArnott Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .editorconfig
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is all fine.

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# EditorConfig is awesome:http://EditorConfig.org

# top-most EditorConfig file
root = true

[*]
indent_style = space
insert_final_newline = true
trim_trailing_whitespace = true

[*.sh]
indent_size = 4

[*.json]
indent_size = 2

[*.css]
indent_size = 2
8 changes: 8 additions & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
###############################################################################
# Set default behavior to automatically normalize line endings.
Copy link
Collaborator

@daira daira Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
# Set default behavior to automatically normalize line endings.
# Set default behaviour to not attempt to normalize line endings.

###############################################################################
* text=auto

Copy link
Collaborator

@daira daira Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't agree we should change the default. For reference, what this does is convert files that git detects as text to have CRLF line endings when checked out on Windows. But I've never seen a programming-oriented text editor on Windows in the past two decades that didn't work just fine with LF-only files, preserving their LF-onlyness. The auto-conversion behaviour, on the other hand, is liable to cause things to break in unpredictable ways when building or testing from a checkout that has been converted.

If someone wants the conversion behaviour, they can and should set the core.autocrlf config variable to auto. I have no objection to the *.sh eol=lf line below that ensures that is all they have to do, i.e. that doing so does not break shell scripts.

Suggested change
###############################################################################
# Set default behavior to automatically normalize line endings.
###############################################################################
* text=auto

From the PR description:

Without this file, each git client follows the rules configured by the machine that installed it. This can lead to large walls of diffs in PRs where nothing but the line endings changed.

Not in practice; I don't remember us ever receiving such a PR, and if we did we'd just tell the PR author that they should not be using any editor that does that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree we should change the default.

The default is that the client installation sets the policy. If you care about line endings being consistent for your repo (and it sounds like you do), you should establish a policy via .gitattributes.
If you disagree with this particular policy, that's totally fair and I'm happy to change it.

what this does is convert files that git detects as text to have CRLF line endings when checked out on Windows. But I've never seen a programming-oriented text editor on Windows in the past two decades that didn't work just fine with LF-only files, preserving their LF-onlyness. The auto-conversion behaviour, on the other hand, is liable to cause things to break in unpredictable ways when building or testing from a checkout that has been converted.

I've never seen that happen, but it sounds like you'd prefer LF to be checked in and LF to be checked out. That's a fine policy and I'll figure out what change to make to this file to encode that as a rule.

Without this file, each git client follows the rules configured by the machine that installed it. This can lead to large walls of diffs in PRs where nothing but the line endings changed.

Not in practice; I don't remember us ever receiving such a PR

The default behavior of a Git for Windows install is to check out CRLF line endings and check in LF. But as some folks feel that version control shouldn't ever mess with files' line endings, if they were to configure their client this way, and created a new text file on Windows, they'd introduce a CRLF line ending file to your repo and you'd probably never notice.

To be fair, you probably don't get that many PRs in a repo as highly specialized as this. And most of your contributors probably don't use Windows machines (which are more likely to try to rewrite things as CRLF. So while I accept that you haven't seen the PR that I describe so far, you can still save yourself and others the trouble of ever encountering that problem by encoding your line ending policy in your repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Case in point, I just cloned this repo on linux WSL and the script failed because of a line ending issue:

andrew@ryzen9:~/git/zips$ ./render-via-docker.sh
-bash: ./render-via-docker.sh: /bin/bash^M: bad interpreter: No such file or directory

Once this .gitattributes file is checked in, I expect this to never happen again.

Copy link
Collaborator

@daira daira Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The default is that the client installation sets the policy.

No, the default is to leave line endings alone. The client installation can override that, but this is by far not the only client git configuration override that can end up breaking things.

Case in point, I just cloned this repo on linux WSL and the script failed because of a line ending issue: [...]

It must have failed because you overrode the default policy in your user settings. (Note that "leave line endings alone" is also the default on Windows, for good reason.)

That will break scripts in checked-out git repos on WSL in general, because WSL is Linux (just running in a VM), and it uses the Unix line-ending convention. There's nothing special about this repo that should require it to be defensive about user settings that are poorly chosen for the environment they're running in, when the vast majority of git repos aren't defensive in that way.

In any case:

I have no objection to the *.sh eol=lf line below that ensures that [overriding the text setting to text=auto] does not break shell scripts.

# Ensure shell scripts use LF line endings (linux only accepts LF)
*.sh eol=lf
*.ps1 eol=lf
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unnecessary:

Suggested change
*.ps1 eol=lf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it on the basis that you have no .ps1 scripts.
But FYI, while powershell doesn't care about line endings, linux does. And if you start your ps1 script with #!/env/pwsh or something similar, that file had better use LF line endings or linux will be unable to find pwsh due to the extra CR at the end of that line. That's why I added this line to all my .gitattributes files.