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 so line endings is consistent across systems #5731

Merged
merged 2 commits into from Jan 29, 2023

Conversation

trejjam
Copy link
Contributor

@trejjam trejjam commented Sep 15, 2022

No description provided.

@jeffwidman
Copy link
Member

@trejjam can you click the "Allow edits from Maintainers"?

We require branches to be up to date before merging, and without those rights, I can't rebase whenever we're ready to merge.

@trejjam
Copy link
Contributor Author

trejjam commented Sep 16, 2022

Sure, somehow the option was not there when the repo was part of an organization

@jeffwidman
Copy link
Member

jeffwidman commented Jan 19, 2023

Thanks for splitting this one out.

I just checked and there's only one file existing in the repo with a CRLF ending:

~/C/g/dependabot-core ❯❯❯ git ls-files --eol | grep crlf                                                                                                                                                              
i/crlf  w/crlf  attr/                 nuget/spec/fixtures/nuspecs/Microsoft.Extensions.DependencyModel.nuspec

Perhaps we'd want to specifically call out that file by name in this gitattributes file as one that should always use CRLF ?

I did some reserach, and looks like eol=lf overrides any text setting, so setting the text value is unnecessary.

Additionally, Microsoft changed Notepad from 2018 Windows 10 onward to support Unix / Mac line endings. And sounds like Visual Studio code will check out the files with CR+LF but then check them back in with LF (Unix ending) so existing files should be okay.

Given the above, I'm not too concerned about existing files changing their line endings... so the main potential problem is if new files might be added from Windows-based contributors and put in the wrong line endings...

I'm actually not too concerned about that given that we normally point folks at the Docker-based development experience, so for the most part they'll be on Linux and not Windows. And I assume that most devs who have enough experience to navigate the hurdles of running this code directly on Windows will also be experienced enough to be aware of line endings when contributing back to open source.

So overall, I'd probably lean slightly towards not adding any of the wildcard lines right now--I think it doesn't really solve an existing problem, and instead could potentially cause confusion down the road.

What say you @deivid-rodriguez ?

@trejjam
Copy link
Contributor Author

trejjam commented Jan 19, 2023

Hi, I do no longer remember what was cause of this PR. But for sure it was based on making a PR on widows. Than I accidentally commit file with wrong line endings. It was "annoying" to go back and fix things, sure it can be fixed in different ways. I did not experienced any issues with this approach.

@deivid-rodriguez
Copy link
Contributor

As per the link you shared @jeffwidman, I believe text eol=lf is different from text=auto eolf=lf (at least on recent git versions). The former means: "For matched files, consider them as text files and apply lf to them". The latter means: "For matched files, let git figure out whether they are text files or not, and apply lf to them if they are". The current version in this PR seems better, since the problem Windows users face is not git file type detection, but eol conversion.

I sometimes need to spin a Windows VM to troubleshoot things, and I've felt the pain of line conversion of Windows. So I'm happy to introduce any changes that makes lifes of Windows contributors easier. We can revert anything that does not work as expected if something happens to get in the middle.

The current solution feels not great to maintain though. How about using this suggestion plus the specific call out to the one file we want to use crlf. It'd be something like this I guess:

* text=auto eol=lf
nuget/spec/fixtures/nuspecs/Microsoft.Extensions.DependencyModel.nuspec text eol=crlf

@jeffwidman
Copy link
Member

jeffwidman commented Jan 19, 2023

The idea sounds reasonable. Happy to try it!

However, not sure the implementation is quite right... I remember spending two hours reading before, and got very conflicting info which of the following is ideal:

I'm really hazy, but based on my reading it looks like we could get away with * text=auto (plus the one hardcoded exception)??

@deivid-rodriguez
Copy link
Contributor

I see! It sounds like just * text=auto would fix what @trejjam reported as the issue he run into.

However, I believe the problem I've personally run into was git diff showing source control changes on my working copy. Even if they're properly normalized on the server side, they are annoying, and I believe the eol=xx part fixes that.

From the answer to the link in the second bullet point of your answer:

This lf mentioned in this quote is referring to what happens when a file is added to the index (and finally pushed upstream). The additional eol=xx says what these files should be in your working tree, ie how they should appear locally on your filesystem after checking out files that git has automatically detected as text via the * text=auto portion

Anyways, I think we cannot know this for sure unless we try it, so I guess we can close this PR and resurrect it using what we've learnt in case someone runs into issues in the future?

@deivid-rodriguez
Copy link
Contributor

@trejjam Just to clarify my previous message, if you still have that Windows dev environment around and want to try to validate what we think we have learnt and update this PR, we are still very happy to take it :)

But if you're no longer interested (which would also be understandable), then I think we can close this and tackle it in the future when/if some other Windows developer runs into issues.

@trejjam
Copy link
Contributor Author

trejjam commented Jan 19, 2023

The important part of the PR is eol=lf which makes sure you push files with LF line-endings. On linux it should be noop. On windows, it makes sure that it's correct.

Sure if you dislike it, just close it. I am not a huge contributor, I do not work with this code base enough

@trejjam
Copy link
Contributor Author

trejjam commented Jan 20, 2023

This solves this kind of issues:
image
#5988

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Jan 20, 2023

I was curious so I just did some testing on Windows. And I think @jeffwidman's suggestion fixes the problem. This is what I did:

  • Pushed a branch with a .gitattributes file including * text=auto.
  • Configure my git on Windows with git config core.autocrlf false. Otherwise it seems to just work as expected no matter what I do.
  • Then from the main branch convert the README to dos line endings. Indeed the "bad README.md" may end up in the index.
  • However in the branch with the .gitattributes file, it's automatically converted to unix endings before when you try to add it to the index.

Captura de pantalla de 2023-01-20 20-52-00

@trejjam
Copy link
Contributor Author

trejjam commented Jan 25, 2023

Thanks for testing, what is conclusion? Use only * text=auto? Or current .gitattributes without eol?

@deivid-rodriguez
Copy link
Contributor

I think the following .gitattributes file should work

* text=auto
nuget/spec/fixtures/nuspecs/Microsoft.Extensions.DependencyModel.nuspec text eol=crlf

That way we don't need to keep a explicit list of file extensions that's likely to get out of date.

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you!

.gitattributes Outdated Show resolved Hide resolved
.gitattributes Outdated Show resolved Hide resolved
@jeffwidman jeffwidman enabled auto-merge (squash) January 29, 2023 05:54
@jeffwidman jeffwidman merged commit 0e05f68 into dependabot:main Jan 29, 2023
@deivid-rodriguez deivid-rodriguez mentioned this pull request Feb 16, 2023
alcere pushed a commit that referenced this pull request Feb 20, 2023
Add `.gitattributes` with enforced LF so line endings is consistent across systems.

We went back and forth on how to implement this, see the pull request thread for detailed discussion, it's complicated.
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.

None yet

3 participants