Fixing the git attrs for all the people and all the files and all future 🙌 #31128

pvinis · 2021-03-10T12:47:22Z

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 😅!

To fix the second, I added the .gitattributes instruction and renormalized 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 👓.

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

[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.

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 💜.

matt-oakes · 2021-03-10T12:55:25Z

I think the template/_gitattributes file should have the line which handles Xcode project files as text back in to keep it consistent with previous versions:

*.pbxproj -text

pvinis · 2021-03-10T12:57:58Z

I'm not sure since when this was fine. I guess we could keep it 🤷? But I think it's been a while, so I would opt for not having it 🤔.

analysis-bot · 2021-03-10T13:07:33Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: cdf3182

analysis-bot · 2021-03-10T13:18:34Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,908,406	+0
android	hermes	armeabi-v7a	8,407,477	+0
android	hermes	x86	9,398,953	+0
android	hermes	x86_64	9,342,879	+0
android	jsc	arm64-v8a	10,642,320	+0
android	jsc	armeabi-v7a	10,124,479	+0
android	jsc	x86	10,694,366	+0
android	jsc	x86_64	11,278,558	+0

Base commit: cdf3182

matt-oakes · 2021-03-10T13:47:21Z

Yes, that's true. It's actually not the settings that are recommended by the iOS community anyway.

liamjones · 2021-03-10T14:34:16Z

Allowing pbxproj files to diff can be problematic. Due to autogenerated IDs you can't easily merge changesets from different people.

The git docs even call out marking them as binary to prevent diffing: https://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes#_binary_files

Although it’s technically a text file (because it’s all UTF-8), you don’t want to treat it as such because it’s really a lightweight database – you can’t merge the contents if two people change it, and diffs generally aren’t helpful. The file is meant to be consumed by a machine. In essence, you want to treat it like a binary file.

pvinis · 2021-03-10T14:46:49Z

Yup. This is a known issue, and I hope sometime we will solve it with xcodegen or something similar. But these files are changing and it's important to know what is changed. They are too fragile to just commit without paying attention. The upgrade helper diffs them and even with ids changing, it's important to know what's there and what's changed.

(Oof, never seen that the git docs have the xcode file as example. That's bad. You can and should merge things in that file. Otherwise you lose changes, and that's the point of git. Yes, it's hard because that file was not well-designed. But it's the same as package.json, just in a worse format. I will make a PR for the git docs. I think the docs are open source 🤔.)

liamjones · 2021-03-10T17:04:45Z

Oh, I agree the changes are important. 😄

Personally, I just subscribe to a different resolution method - look at what the changes produce in the xcodeproj and then use Xcode itself to generate the changes again via the UI. That way there's no chance of misaligned IDs, 'Recovered References', etc.

matt-oakes · 2021-03-10T17:18:33Z

Not reliant for this PR, but there was a get mergetool tool that would parse, attempt to understand, and then merge the project files automatically:

It's not been updated for a long time though, so I have no idea if it would still work.

Anyway! That's not what this PR is about 🙂

friederbluemle · 2021-03-10T20:34:31Z

Thanks for the PR @pvinis - Before merging this, I would like to understand the problems you were facing a bit better. Let's have a quick look at the Gradle Wrapper scripts on the current master branch in this repo:

$ git ls-files | grep gradlew | xargs file
gradlew:                                           a /usr/bin/env sh script text executable, ASCII text
gradlew.bat:                                       DOS batch file text, ASCII text, with CRLF line terminators
packages/react-native-codegen/android/gradlew:     a /usr/bin/env sh script text executable, ASCII text
packages/react-native-codegen/android/gradlew.bat: DOS batch file text, ASCII text, with CRLF line terminators
template/android/gradlew:                          a /usr/bin/env sh script text executable, ASCII text
template/android/gradlew.bat:                      DOS batch file text, ASCII text, with CRLF line terminators

Everything looks correct to me.

So first question: Why is there a need to "renormalize" the files, and why do they show up as changed in the PR diff?

friederbluemle · 2021-03-10T20:54:48Z

I just checked the third commit on your branch (renormalize files) locally, and it looks like it replaces CRLF with LF in four .bat files. So it's clearly not what we want, and I'm pretty sure that was not intentional? Can you remove the commit please?

I think (please correct me if I'm wrong), the purpose of this PR is to add the [*.bat] rule to editorconfig. Which is fine with me 👍 For the reasons explained in #29792 I strongly suggest not to add eol=crlf rules to several gitattributes files in source control. We might, however, consider adding *.pbxproj binary as suggested by @liamjones.

friederbluemle · 2021-03-10T20:58:55Z

Also just a minor addition regarding *.pbxproj -text (@matt-oakes) - To my knowledge, .pbxproj files do not normally contain a \0 character in its first 8000 bytes (i.e. Git would not treat it as binary), so that configuration would have no effect.

pvinis · 2021-03-10T22:28:02Z

Right, so for pbxproj, it's cool, we keep it away. No need for it anymore.

For the rest, the problem we faced was that everytime someone did a change in these bat files, the whole thing would change because the line endings would change depending if people did the change in win or mac.
That is solved by the editorconfig rule. That part is fine now.

The normalization is something that apparently git needs. I tried to look at the code of git but it got a bit tricky, and the docs are not suuuuuuper helpful, but from what I see, if a file was committed at some point with LF, git thinks it's always LF from then on. So from my understanding, after renormalizing, git now understands the right line endings. So after doing the renormalization, I tried to edit files again on mac and win, and there was only my changes that time. No more changing the whole file etc.

So I think now everything is good. Could you try to edit a bat file on mac (and win if you have) and see if they come up as just the diff you did? If it is, then all is fixed and well. If you get the whole file changing again, then something is up.

Try and let me know?

friederbluemle · 2021-03-16T19:33:24Z

The normalization is something that apparently git needs.

Sorry I don't quite understand. Can you explain in more detail? If you check my message above, you can see that these files already have the correct line endings.

What I think is happening here is your editor is converting entire files when you are only editing single lines as a side effect. That's clearly not what should happen.

Can you please try:

Start with a fresh/clean version of the repo
Remove the line end_of_line = lf from .editorconfig
Edit some files, and verify that everything is working correctly

Again, as I mentioned, all files are already in their expected standard format - there is no need for any conversion and/or addition of new files that configure settings to the default value.

pvinis · 2021-03-16T22:31:44Z

I can try tomorrow. Are you in the RN discord or somewhere else where we could talk faster, so I can try and then we can talk and see what's up?

hramos · 2021-04-21T17:50:33Z

@pvinis @friederbluemle did you get a chance to discuss this PR? Let us know if we should consider it for merging, or if there's any more work that needs to be done.

facebook-github-bot · 2021-04-21T17:56:26Z

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hramos · 2021-04-21T18:11:13Z

Considering the contents of this PR have already been incorporated onto 0.64 and no issues have been reported, we are going to proceed with merging this PR.

facebook-github-bot · 2021-04-21T18:28:09Z

@hramos merged this pull request in 7384471.

… all future (facebook#31128)" This reverts commit 7384471.

friederbluemle · 2021-04-21T20:03:22Z

@hramos - Thanks for the reminder. Can you please check my comment above? As explained - The files already had the correct line endings in the repository (there is no need to add gitattributes or editorconfig). Now that it's merged, the Windows .bat files have their CRLF endings removed, which is incorrect. I've reverted it here: #31398
@pvinis - Please confirm you followed the steps in my previous message and everything is working for you locally. Also make sure your editor does not change line endings as a side effect of editing single lines in a file (some editors might have an option in settings).
It looks like the one thing that might be useful are the three lines in the existing .editorconfig in the root of the repo:

# Windows files
[*.bat]
end_of_line = crlf

Again, @pvinis to confirm if that works locally. Thanks!

pvinis · 2021-04-23T08:49:30Z

I'll check this right now.

pvinis · 2021-04-23T09:31:44Z

Ok let's go.
I have commented out all custom git configs and editor configs.

Cloned rn repo fresh, on master, with the changes in this PR merged.
First thing I did, confirm the files are correct right now.

They are. `gradlew` has `LF`, `gradlew.bat` has `CRLF`.

As extras, I will try to edit these files and commit, and see if they stay with the correct line endings. I will also take a look at your new PR.

Editing the files. I edited the bat file with clean neovim, and it kept all line endings, with a line removed and a line added.

- Git shows the diff correctly, not changing the whole file like before.

- I will also edit this file with vscode, to hopefully get some line ending breakage, to see if git handles it correctly. First thing, is that vscode correctly understands it's CRLF and things work as before with neovim, but I will force it to change to LF.

So here we see the line endings are now LF.

Git diff correctly shows only the changes, not making the diff 91 lines like the whole file. That is already a gain for diffing. To force it, I had to change the editorconfig, so I would say with the right editorconfig we are pretty safe, and even if that breaks, git still handled it well.

So my final thoughts here are:

We keep the editorconfig rule for bat files. In the root of the repo for people working on RN, and in the template dir for people init-ing an RN project.
We keep the gitattributes rule for bat files. That way git will handle the diffs well, no matter what the line endings end up being. editorconfig will keep them correct, and even if they are not correct because of a broken editorconfig, it will still handle the diffing correctly.

I think the new PR you made is not actually needed, but please, if you try to replicate all the above in a fresh master with my changes in there, and maybe even in a fresh repo on your branch, and something is handled in a better way on yours, then let me know! I am also happy to check stuff together on discord or wherever else.

friederbluemle · 2021-04-23T23:11:10Z

@pvinis - Thanks a lot for the detailed response and analysis 👍

First thing I did, confirm the files are correct right now.

I admit, it's not immediately obvious, but what you checked here are the files in your working directory, not the files stored in the repository. That's because now that there is a .gitattributes file, Git performs an additional conversion when the files are placed in the working directory, hiding the fact that they are committed with the wrong line endings.

If you want to see how these files are actually stored:

Remove .gitattributes (so that no conversion happens)

git checkout -b temp
git rm .gitattributes
git commit -m"Remove .gitattributes"

Now, delete and restore the existing gradlew.bat file in the working directory

rm gradlew.bat
git checkout gradlew.bat

Check line endings using file (or use bat -A, as in your examples)

$ file gradlew.bat
gradlew.bat: DOS batch file text, ASCII text

The file only has LF line endings. Expected would be:

$ file gradlew.bat
gradlew.bat: DOS batch file text, ASCII text, with CRLF line terminators

Alternatively, if you use something like diff-so-fancy the removed carriage return characters will also show up in git diff:

$ git -c core.pager='diff-so-fancy | less --tabs=4 -RFX' show -p 73844712b62 -- gradlew.bat

-@rem^M
-@rem Copyright 2015 the original author or authors.^M
-@rem^M
...
+@rem
+@rem Copyright 2015 the original author or authors.
+@rem
...

^M represents CR, which was removed in 7384471 (not visible on GitHub's UI).

pvinis · 2021-04-24T21:35:04Z

I will check these again and report back.

But just as a quick response, I definitely care in order:

easy and small diffs (just the actual diffs, not the whole file)
setup that doesn't break easily for users (rn devs and/or rn project creators)
"correct" line endings for windows (especially when things work with editorconfig. I have seen a few "best practices" that actually suggest to store everything with LF and only locally on windows machines make sure to keep them with CRLF, which is happening with git.)

… all future (facebook#31128)" This reverts commit 7384471.

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

More info at fabOnReact/react-native-notes#8 facebook#31128 (comment) >If you want to see how these files are actually stored: >Remove .gitattributes (so that no conversion happens) I solved the problem with the following steps: 1) checkout branch I want to rebase 2) the branch was already rebased and included commit facebook@7384471 which brought `.gitattributes` file in the branch The file below triggers the error `error: add_cacheinfo failed to refresh for path 'packages/react-native-gradle-plugin/gradlew.bat'; merge aborting.` when running `git merge master`. ``` -# Windows files should use crlf line endings -# https://help.github.com/articles/dealing-with-line-endings/ -*.bat text eol=crlf ``` 3) I remove the `.gitattributes` files from the branch. 4) `git` stops converting the line ending 5) I merge main branch. `.gitattributes` files was already removed from main branch with https://github.com/facebook/react-native/pull/31398/files. Git runs the merge without problems. Also no diff because the file already deleted from main.

pvinis added 4 commits March 10, 2021 11:51

add a rule for .bat files

ff1d927

add correct windows files attributes

bfea514

renormalize files

ef06b09

mirror changes in template too

944039d

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 10, 2021

pvinis mentioned this pull request Mar 10, 2021

Road to v0.64 Discussion react-native-community/releases#214

Closed

36 tasks

kelset mentioned this pull request Mar 15, 2021

Upcoming v0.64.1 cherrypicks discussion react-native-community/releases#221

Closed

15 tasks

nadiia mentioned this pull request Apr 21, 2021

Road to v0.65 Discussion react-native-community/releases#223

Closed

38 tasks

hramos approved these changes Apr 21, 2021

View reviewed changes

facebook-github-bot closed this in 7384471 Apr 21, 2021

facebook-github-bot added the Merged This PR has been merged. label Apr 21, 2021

friederbluemle added a commit to friederbluemle/react-native that referenced this pull request Apr 21, 2021

Revert "Fixing the git attrs for all the people and all the files and…

870b308

… all future (facebook#31128)" This reverts commit 7384471.

friederbluemle mentioned this pull request Apr 21, 2021

Revert removal of CRLF from Windows bat files #31398

Closed

friederbluemle added a commit to friederbluemle/react-native that referenced this pull request Aug 31, 2021

Revert "Fixing the git attrs for all the people and all the files and…

06f8c8e

… all future (facebook#31128)" This reverts commit 7384471.

friederbluemle added a commit to friederbluemle/react-native that referenced this pull request Sep 2, 2021

Revert "Fixing the git attrs for all the people and all the files and…

d56cb11

… all future (facebook#31128)" This reverts commit 7384471.

fabOnReact mentioned this pull request Feb 18, 2022

error: add_cacheinfo failed to refresh for path 'packages/react-native-gradle-plugin/gradlew.bat'; merge aborting. fabOnReact/react-native-notes#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing the git attrs for all the people and all the files and all future 🙌 #31128

Fixing the git attrs for all the people and all the files and all future 🙌 #31128

pvinis commented Mar 10, 2021 •

edited

Loading

matt-oakes commented Mar 10, 2021

pvinis commented Mar 10, 2021

analysis-bot commented Mar 10, 2021

analysis-bot commented Mar 10, 2021

matt-oakes commented Mar 10, 2021

liamjones commented Mar 10, 2021

pvinis commented Mar 10, 2021 •

edited

Loading

liamjones commented Mar 10, 2021

matt-oakes commented Mar 10, 2021

friederbluemle commented Mar 10, 2021

friederbluemle commented Mar 10, 2021

friederbluemle commented Mar 10, 2021 •

edited

Loading

pvinis commented Mar 10, 2021

friederbluemle commented Mar 16, 2021

pvinis commented Mar 16, 2021

hramos commented Apr 21, 2021

facebook-github-bot commented Apr 21, 2021

hramos commented Apr 21, 2021

facebook-github-bot commented Apr 21, 2021

friederbluemle commented Apr 21, 2021

pvinis commented Apr 23, 2021

pvinis commented Apr 23, 2021

friederbluemle commented Apr 23, 2021

pvinis commented Apr 24, 2021

Fixing the git attrs for all the people and all the files and all future 🙌 #31128

Fixing the git attrs for all the people and all the files and all future 🙌 #31128

Conversation

pvinis commented Mar 10, 2021 • edited Loading

Summary

Changelog

Test Plan

matt-oakes commented Mar 10, 2021

pvinis commented Mar 10, 2021

analysis-bot commented Mar 10, 2021

analysis-bot commented Mar 10, 2021

matt-oakes commented Mar 10, 2021

liamjones commented Mar 10, 2021

pvinis commented Mar 10, 2021 • edited Loading

liamjones commented Mar 10, 2021

matt-oakes commented Mar 10, 2021

friederbluemle commented Mar 10, 2021

friederbluemle commented Mar 10, 2021

friederbluemle commented Mar 10, 2021 • edited Loading

pvinis commented Mar 10, 2021

friederbluemle commented Mar 16, 2021

pvinis commented Mar 16, 2021

hramos commented Apr 21, 2021

facebook-github-bot commented Apr 21, 2021

hramos commented Apr 21, 2021

facebook-github-bot commented Apr 21, 2021

friederbluemle commented Apr 21, 2021

pvinis commented Apr 23, 2021

pvinis commented Apr 23, 2021

friederbluemle commented Apr 23, 2021

pvinis commented Apr 24, 2021

pvinis commented Mar 10, 2021 •

edited

Loading

pvinis commented Mar 10, 2021 •

edited

Loading

friederbluemle commented Mar 10, 2021 •

edited

Loading