Remove .gitattributes and normalize line endings #29792
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
Base commit: 5acf7c9 |
Base commit: 5acf7c9 |
friederbluemle
force-pushed
the
fix-eol
branch
from
d9c1dc3
to
b25e643
Compare
friederbluemle
force-pushed
the
fix-eol
branch
from
b25e643
to
c1e7d6f
Compare
mdvacca
reviewed
Sep 4, 2020
There was a problem hiding this comment.
thanks for the PR @friederbluemle, importing to review internally
There was a problem hiding this comment.
@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request was successfully merged by @friederbluemle in fb354cb. When will my fix make it into a release? | Upcoming Releases |
grabbou
pushed a commit
to callstack/react-native
that referenced
this pull request
Sep 14, 2020
Summary: The nested `.gitattributes` file in `packages/react-native-codegen/android/` caused some confusion on Linux and macOS, causing Git to show `packages/react-native-codegen/android/gradlew.bat` as modified (CRLF removed, LF added). Instead of relying on repo-local `.gitattributes` files to convert endings in the working directory, the files should be committed to source control with the correct line endings in the first place. There is no reason to convert LF endings in .sh and many other file to CRLF on Windows (maybe this was an issue a long time ago, but unless Notepad is used this won't be a problem for practically all modern editors). Also fixed the line endings of `scripts/launchPackager.bat` which was incorrectly committed as LF. ## Changelog [Internal] [Fixed] - Line endings and .gitattributes Pull Request resolved: facebook#29792 Test Plan: Clone repo on Linux, macOS, and Windows, and make sure no modified files show up. Reviewed By: fkgozali Differential Revision: D23546135 Pulled By: mdvacca fbshipit-source-id: 1572fcb959212f212b137066f1aa66f0bb6e86c3
36 tasks
|
I totally missed this PR. I think it should be reverted and/or fixed. Here's the explanation:
I will make a PR now and see what's up. |
|
PR opened! I think this fixed everything for us and everyone using the template from now on, and of course any feedback is welcome 👍. |
|
Thanks for the comments and concerns! Like you, I am interested in getting this fixed the "right way" :) I left a comment on the PR you opened, please check. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 21, 2021
…ure (#31128) Summary: We have had problems with `.gitattributes`, `.bat` and `.pbxproj` (Xcode) files for a while. The two main concerns were: - Xcode project files not diffing correctly. - Windows files having messed up line endings. This PR fixes both issues, hopefully forever. After seeing the diffs from v0.63 -> v0.64 and the changes in #29792, I, again, felt that this is going to cause problems, so I looked into both issues. I started with `git check-attr -a Artsy.xcodeproj/project.pbxproj` after removing the `.gitattributes` file that contained `*.pbxproj -text` and there are no "guessed" attributes that would break things, and diffing and checking in worked well with the current git version. I agree this is not needed, so I left it out. I looked into what it was doing before, and it was telling git (for the xcode project file) to "unset text", which means (according to https://git-scm.com/docs/gitattributes#_text) that it should not try to do any line ending changes when checking in that file. At some point git must have done this, and that's why it was needed, but no more, so it's safe and good to get rid of this, as it helps with nothing anymore. Now for the bat files. We don't need any extra instructions for `gradle` and `*.sh` files as they are guessed correctly, so these are also safe to keep removed (https://github.com/facebook/react-native/pull/29792/files#diff-618cd5b83d62060ba3d027e314a21ceaf75d36067ff820db126642944145393eL5). But we do need the `*.bat` instruction. I noticed that when working on macOS there were two problems that made things funky. - One, is that the editor (usually vscode but not the important) would convert line endings to lf when editing a bat file. - Two, is that git thought the files are lf line endings. To fix the first one, I added a rule in `.editorconfig` (that's whats important, when any editor just supports editorconfig, but all my editors do). I can't believe how we missed that for sooooo long {emoji:1f605}! To fix the second, I added the `.gitattributes` instruction **and** `renormalize`d the files (https://git-scm.com/docs/git-add#Documentation/git-add.txt---renormalize and https://docs.github.com/en/github/using-git/configuring-git-to-handle-line-endings#refreshing-a-repository-after-changing-line-endings). I think the problem all along was that even though the files had crlf, git still thought they were using lf. After the editorconfig change and the renormalization, everything behaves correctly and as expected. Changing bat files on macOS and Windows is fine now, producing only the smallest change needed, no random line ending diffs. Also here is a screenshot of one of the files actually crlf. It's these tiny things at the end of each line {emoji:1f453}. <img width="612" alt="Screenshot 2021-03-10 at 12 20 28" src="https://user-images.githubusercontent.com/100233/110630943-ef536280-819d-11eb-9212-dbd70f038a44.png"> I have tested this on macOS and Windows, doing changes in both bat and xcode files, and verified that diffing and checking in files works well. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [General] [Added] - Added an `.editorconfig` file to help with default line endings for Windows files. [Internal] [Fixed] - Added a rule in `.editorconfig` and `.gitattributes` to help with default line endings for Windows files. Pull Request resolved: #31128 Test Plan: Feel free to go on a macOS or Windows machine (or both) and, using any editor that supports editorconfig, do any change in a `.bat` file. Then look at your git diff in terminal or gui or whatever you use, and then look at the sky and smile. It's fixed. The diff is just your change. Everything is as it should {emoji:1f49c}. Reviewed By: nadiia Differential Revision: D27914636 Pulled By: hramos fbshipit-source-id: fc4e53a4fa42cb13e29686669e8de1679c2242e7
philippeauriach
pushed a commit
to philippeauriach/react-native
that referenced
this pull request
May 5, 2021
Summary: The nested `.gitattributes` file in `packages/react-native-codegen/android/` caused some confusion on Linux and macOS, causing Git to show `packages/react-native-codegen/android/gradlew.bat` as modified (CRLF removed, LF added). Instead of relying on repo-local `.gitattributes` files to convert endings in the working directory, the files should be committed to source control with the correct line endings in the first place. There is no reason to convert LF endings in .sh and many other file to CRLF on Windows (maybe this was an issue a long time ago, but unless Notepad is used this won't be a problem for practically all modern editors). Also fixed the line endings of `scripts/launchPackager.bat` which was incorrectly committed as LF. ## Changelog [Internal] [Fixed] - Line endings and .gitattributes Pull Request resolved: facebook#29792 Test Plan: Clone repo on Linux, macOS, and Windows, and make sure no modified files show up. Reviewed By: fkgozali Differential Revision: D23546135 Pulled By: mdvacca fbshipit-source-id: 1572fcb959212f212b137066f1aa66f0bb6e86c3
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 10, 2021
Summary: This reverts #31128 - For the reasons stated in the thread. Files should have the correct endings in the repo (i.e. Windows .bat CRLF). There is no reason to perform additional conversion with attributes and/or an editorconfig. It was originally fixed in #29792 in August 2020. :warning: **EDIT 2021-08-31** Commits 85249ca and 13107fa accidentally converted the gradlew.bat files to LF again, resulting in modified files to appear in the working directory: ``` $ git status -s M gradlew.bat M packages/react-native-codegen/android/gradlew.bat M template/android/gradlew.bat ``` The reasons why this is happening are explained in detail in the two PRs linked above. I've added an additional (new) commit to the PR head branch to fix the line endings in all three `gradlew.bat` files of the repo and rebased it. It should be ready for merge. CC cortinico EDIT 2021-09-02 The additional commit was removed again, but the original one remains. To test the scenario locally run the following commands on a clean `main` branch (currently 455433f): ``` $ rm gradlew.bat $ git status -s D gradlew.bat # Git shows the file as (D)eleted, as expected $ git checkout gradlew.bat # This should restore the file $ git status -s M gradlew.bat # The file still shows up, now as (M)odified with all line endings changed ``` The modified file will remain in the working directory until they are committed, or a different branch is _force_ checked out. `gradlew.bat` files are generated automatically by Gradle (with the correct line endings in the first place). There is no need to special case them and perform line ending conversion using Git and/or editorconfig. ## Changelog [General] [Fixed] - Line endings in Windows files, Git/EditorConfig related conversions Pull Request resolved: #31398 Test Plan: Verify files are stored correctly in the repository (e.g. using the `file` command). Reviewed By: yungsters Differential Revision: D30839864 Pulled By: cortinico fbshipit-source-id: dfc53e8c5d9276d2f9bfd4d4a4e6b44c3143a164
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The nested
.gitattributesfile inpackages/react-native-codegen/android/caused some confusion on Linux and macOS, causing Git to showpackages/react-native-codegen/android/gradlew.batas modified (CRLF removed, LF added).Instead of relying on repo-local
.gitattributesfiles to convert endings in the working directory, the files should be committed to source control with the correct line endings in the first place. There is no reason to convert LF endings in .sh and many other file to CRLF on Windows (maybe this was an issue a long time ago, but unless Notepad is used this won't be a problem for practically all modern editors).Also fixed the line endings of
scripts/launchPackager.batwhich was incorrectly committed as LF.Changelog
[Internal] [Fixed] - Line endings and .gitattributes
Test Plan
Clone repo on Linux, macOS, and Windows, and make sure no modified files show up.