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.

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

add .gitattributes file for standardized line endings #714

Closed

Conversation

Ste1io
Copy link
Collaborator

@Ste1io Ste1io commented Dec 30, 2023

Fixes #713

@asherber
Copy link
Collaborator

This feels a little fishy to me, something that likely isn't needed and could possibly have unintended side effects. Unless someone can show a bug which exists without this change and which is fixed by this change, I would recommend not merging it.

@Ste1io
Copy link
Collaborator Author

Ste1io commented Dec 30, 2023

This is standard practice, just like .gitignore and .editorconfig files; the entire reason for adding it is to prevent unintended side effects. If a contributor clones a repo containing files with Windows line endings, makes changes locally on a Linux machine, then commits the changes, you end up with inconsistent line endings, which can cause subtle bugs in cases such as with the t4 templates.

https://git-scm.com/docs/gitattributes/2.5.6

It would be prudent, on after thought, to normalize the code base prior to merging this PR, which I can take care of.

@asherber
Copy link
Collaborator

Personally, I've only ever added a .gitattributes file when it's been needed to solve a particular issue, not as a general part of all repos.

The default behavior of git is that all files are committed with LF line endings only. You'd need to take specific steps to commit a file with CRLF.

@Ste1io
Copy link
Collaborator Author

Ste1io commented Jan 1, 2024

The default behavior of git is that all files are committed with LF line endings only. You'd need to take specific steps to commit a file with CRLF.

Assuming that the user-level git configuration does not overrides this default behavior.

To reiterate, there is no current issue this is fixing, as it was recently closed by #712. This is a preventative measure moving forward. I'm fairly certain that having this in place from the beginning would have prevented a lot of time and frustration for a lot of users and contributors.

If you foresee a problem with introducing this to the repo based on your experience that I haven't encountered personally, I'm definitely open to reconsideration.

@Ste1io Ste1io marked this pull request as draft January 1, 2024 16:13
@Ste1io
Copy link
Collaborator Author

Ste1io commented Jan 1, 2024

I've converted this PR to a draft for the time being, pending additional feedback on this. Provided it moves forward, I'll reopen after ensuring the files are correctly normalized.

@asherber
Copy link
Collaborator

asherber commented Jan 1, 2024

I don't feel that strongly about it; I'm just a little wary about changing a configuration option without a good reason. And I'm not convinced that the lack of a .gitattributes has caused any issues for this repo.

As far as I can tell, the text files in this repo are all committed with LF line endings, as expected.

@pleb
Copy link
Member

pleb commented Jan 7, 2024

This has always caused a little pain, but usually just with diff'ing when the diff logic doesn't ignore line endings. But this isn't a project concern but a user concern.

With the T4 templates, however, I think from memory the line ends must be crlf (windows) or it doesn't work. T4 is super picky.

I'm happy to add a .gitattributes file.

Maybe we can follow the github docs for this

# Set the default behaviour in case people don't have core.autocrlf set.
* text=auto

# The above will handle all files NOT found below

# These files are text and should be normalized (Convert lf => crlf)
*.tt text eol=crlf
*.ttinclude text eol=crlf

@asherber
Copy link
Collaborator

asherber commented Jan 7, 2024

@pleb Your comment about the tt files needing CRLF is what I glean from #713. However, as best as I can tell, the file currently in the repo just have LF (download a zip of the repo from GH and check those files), and the latest comments in the issue indicate that the problem does not currently exist. That's why I wasn't sure this was needed right now.

@pleb
Copy link
Member

pleb commented Jan 8, 2024

@asherber agree. Let's leave this for now, and if it becomes an issue in the future, we address it again.

The T4 templates are probably due to being retired anyway, as I'm unsure of their value over @hippasus's https://github.com/hippasus/PetaPoco.DBEntityGenerator. Plus, given we have a replacement and the fact they're super annoying to use and maintain I'd vote for their removal

@Ste1io Ste1io closed this Jan 11, 2024
@Ste1io Ste1io deleted the Ste1io/issue713 branch January 11, 2024 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .gitattributes for standardized line endings
3 participants