Skip to content

Conversation

@jeffra
Copy link
Collaborator

@jeffra jeffra commented Nov 29, 2021

  • Primary change is to .pre-commit-config.yaml
  • Most of our files currently use LF but there are several that use CRLF, this PR intends to align all files to the same style to prevent issues that we've been recently seeing in PRs.

https://github.com/pre-commit/pre-commit-hooks#mixed-line-ending

@jeffra jeffra merged commit a10e481 into master Nov 29, 2021
@jeffra jeffra deleted the auto-fix-lf branch November 29, 2021 23:41
@lostmsu
Copy link
Contributor

lostmsu commented Dec 8, 2021

There goes the ability to use git blame/annotate without pain :/

@lostmsu
Copy link
Contributor

lostmsu commented Dec 8, 2021

Most of our files currently use LF but there are several that use CRLF, this PR intends to align all files to the same style to prevent issues that we've been recently seeing in PRs.

Just run a local script against the commit before this has been checked in, and it seems like all code files were CRLF.

All line counts:

CRLF LF
csrc 16186 293
deepspeed 32453 8263

@tjruwase
Copy link
Contributor

tjruwase commented Dec 8, 2021

There goes the ability to use git blame/annotate without pain :/

@lostmsu, that sounds serious. Can you please explain your concern on the impact of LF vs CRLF on git blame/annotate?

@jeffra
Copy link
Collaborator Author

jeffra commented Dec 8, 2021

I originally did this check on a per-file basis, since it seemed line style was consistent within a file. I just re-ran those numbers:

directory CRLF LF
deepspeed 16 130
csrc 49 26

I agree it is troubling that git blame will be a bit difficult for a while after this, I apologize for any inconvenience there. We have been seeing many issues with git diffs when users change line endings so wanted to fix this via our formatter to ensure no one commits changes like this in the future. We debated on what to do here and felt aligning the entire code base on a single standard was probably best to avoid future issues here.

In hindsight maybe we should have gone with the line count method you took, which would probably have reduced some of the git-blame side effects. Not sure if we can change this at this point unfortunately.

@lostmsu
Copy link
Contributor

lostmsu commented Dec 9, 2021

@jeffra no worries, I was just ranting 😅

@lostmsu
Copy link
Contributor

lostmsu commented Dec 9, 2021

@tjruwase well, you just won't see past edits in blame because the line ending change will be the last edit in most of the code. It already is.

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.

4 participants