Skip to content

Fix line endings to newline #452

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

Closed
ErikSchierboom opened this issue Oct 3, 2017 · 14 comments
Closed

Fix line endings to newline #452

ErikSchierboom opened this issue Oct 3, 2017 · 14 comments

Comments

@ErikSchierboom
Copy link
Member

To prevent cross-platform issues, we should use \n for all our source code files, both for exercises and generators, as well as generated test files.

@jpreese
Copy link
Contributor

jpreese commented Oct 4, 2017

I played with this a little bit on a local branch, and the change itself is pretty straightforward. Though after the fact, every stage you perform, you're going to get warnings about CRLF to LF conversions.

I'd be curious to see how typical it is to deviate away from text=auto, which should handle it for us.

@ErikSchierboom
Copy link
Member Author

@jpreese Hmmm, if it proves to be more of a nuisance than a boon, we should just skip it.

@jpreese
Copy link
Contributor

jpreese commented Oct 4, 2017

Yeah I'd need to look into it more, but it's my understanding that auto will do the transform for us, without the warning. Linux users shouldn't have a problem as they're using LF anyway. I'm not honestly sure why it was impacting @felix91gr.

I then looked at a random sampling of repositories and really couldn't find any that explicitly set LF.

@ErikSchierboom
Copy link
Member Author

Okay, let's close this then.

@felix91gr
Copy link
Contributor

The problem was that my git changed some files that had CRLF instead of LF. Some Windows user must’ve accidentally created those.

My point is, if it happened once, maybe it’ll happen again. Better enforce the LF everywhere so that we don’t get spurious file changes when git notices CRLF and decides to normalize it.

@ErikSchierboom
Copy link
Member Author

Well, I've tried to reproduce this on my Mac but have failed. So I suspect it might be some environment-specific issue with your machine. If more people have this issue, we can always re-open.

@hymccord
Copy link
Contributor

hymccord commented Oct 4, 2017

I actually encountered the same problem @felix91gr did with those two specific files in the largest-series-product exercise folder. I tried a whole heap of solutions, but nothing would allow me to discard those changes.

I tried changing my global config autoclrf (which was originally set to true)

git config --global core.autoclrf false 
git config --global core.autoclrf true  
git config --global core.autoclrf input  

Every time I changed the setting, I removed the directory and recloned it, but as soon as a cloned it would say I had changed the line endings in those two files. I made sure it was only line endings comparing the git diff and git diff --ignore-all-space

The only thing I remember doing to solve it (my fork was 3 commits behind at the time since it was based off of 6677ba9) was to delete my fork, refork, and clone. Somehow this fixed it.

My environment is windows. Using VSCode and VS2017.3.

@felix91gr
Copy link
Contributor

About .gitattributes

Regarding this file, I think we should change it, but if you guys prefer leaving it as-is, I don't think that would cause problems either. We shouldn't see more CRLF line endings appearing out of nowhere, because it appears to be that the default config for almost everyone takes them away before commiting or saving.

About the problematic files currently present in the repo

Those files must have gotten CRLF chars by accident. We should at least turn them back to LF to avoid having these kind of problems in the future.

If you agree with me: should I leave the corrections in my current PR, or should I make another one just for that "hotfix"? Any one is fine by me :)
But please let me change them... I don't wanna have to hack around with git (which is already kind of a mess by itself) just to help with the development of Exercism :)

@jpreese
Copy link
Contributor

jpreese commented Oct 5, 2017

I'm not opposed to changing those couple of files. Those could have gotten in a wonky state from who knows how long ago.

My stance is that if we change .gitattributes to force LF, every stage from there on out will have this warning and it'll probably take some local finagling to get them to go away.

If we fix those files, the text=auto property that already exists should handle everything gracefully for us.

@felix91gr
Copy link
Contributor

Should I let the change propagate from my original PR? Or should I make a new one for this?

My stance is that if we change .gitattributes to force LF, every stage from there on out will have this warning and it'll probably take some local finagling to get them to go away.

How so? I'm missing something.

But here:

If we fix those files, the text=auto property that already exists should handle everything gracefully for us.

You are right. This shouldn't happen again for a long time anyways :)

@jpreese
Copy link
Contributor

jpreese commented Oct 5, 2017

  1. That was my experience. I can play with it some more, but I followed the steps listed here https://help.github.com/articles/dealing-with-line-endings/ and then every time I staged a file in the repo, EVERY file warned me about the conversion, instead of just those few.

  2. I'd probably just open another to keep the issues distinct, the files aren't even the same.

@felix91gr
Copy link
Contributor

EVERY file warned me about the conversion, instead of just those few.

Oh shit. That's no good.

the files aren't even the same

Oki doke. I'll make another one then, and after that I'll finish the Difference of Squares PR without having to hack around with git :)

@jpreese
Copy link
Contributor

jpreese commented Oct 5, 2017

You're more than welcome to follow those steps and play with it yourself. I really haven't explicitly set .gitattributes outside of using auto before, and I haven't found many other repos that do it.

@felix91gr
Copy link
Contributor

Fixed this in #461 👍

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

No branches or pull requests

4 participants