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

[iOS] Make .pbxproj file a non-binary #8507

Merged
merged 4 commits into from Jun 4, 2020

Conversation

bbarthec
Copy link
Contributor

@bbarthec bbarthec commented May 27, 2020

@bbarthec bbarthec marked this pull request as ready for review May 27, 2020 09:16
.gitattributes Outdated
@@ -1,4 +1,4 @@
*.pbxproj -diff linguist-generated
*.pbxproj text -merge union linguist-generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

I think James mentioned that we might want to keep Pods' pbxproj as it was (because it's generated) and change git attributes only for this one under ios/Exponent.xcodeproj

.gitattributes Outdated
@@ -1,3 +1,4 @@
ios/Exponent.xcodeproj/project.pbxproj merge=mergepbx -text linguist-generated
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, still, what about other project.pbxproj? Especially from Pods folder, in the client and bare-expo as well.

@bbarthec bbarthec force-pushed the @bbarthec/mark-pbxproj-as-text branch 2 times, most recently from 7d840fc to 04cc243 Compare May 28, 2020 16:40
@bbarthec bbarthec force-pushed the @bbarthec/mark-pbxproj-as-text branch from 04cc243 to 9de628f Compare May 28, 2020 16:41
.gitattributes Outdated
Comment on lines 1 to 3
*.pbxproj binary linguist-generated
ios/Exponent.xcodeproj/project.pbxproj merge=mergepbx -text diff linguist-generated
apps/bare-expo/ios/BareExpo.xcodeproj/project.pbxproj merge=mergepbx -text diff linguist-generated
Copy link
Contributor Author

@bbarthec bbarthec May 28, 2020

Choose a reason for hiding this comment

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

These lines result in being able to see diff for only two specified .pbxproj files (every else .pbxproj is treated as binary (-diff -text):

diff --git a/apps/bare-expo/ios/BareExpo.xcodeproj/project.pbxproj b/apps/bare-expo/ios/BareExpo.xcodeproj/project.pbxproj
index af29fdbeed..664369f871 100644
--- a/apps/bare-expo/ios/BareExpo.xcodeproj/project.pbxproj
+++ b/apps/bare-expo/ios/BareExpo.xcodeproj/project.pbxproj
@@ -6,8 +6,6 @@
        objectVersion = 46;
        objects = {
-
-
 /* Begin PBXBuildFile section */
                00E356F31AD99517003FC87E /* BareExpoTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 00E356F21AD99517003FC87E /* BareExpoTests.m */; };
                13B07FBC1A68108700A75B9A /* AppDelegate.m in Sources */ = {isa = PBXBuildFile; fileRef = 13B07FB01A68108700A75B9A /* AppDelegate.m */; };

diff --git a/apps/bare-expo/ios/Pods/Pods.xcodeproj/project.pbxproj b/apps/bare-expo/ios/Pods/Pods.xcodeproj/project.pbxproj
index 85e3e54d20..1d3bbf20b5 100644
Binary files a/apps/bare-expo/ios/Pods/Pods.xcodeproj/project.pbxproj and b/apps/bare-expo/ios/Pods/Pods.xcodeproj/project.pbxproj differ

diff --git a/ios/Exponent.xcodeproj/project.pbxproj b/ios/Exponent.xcodeproj/project.pbxproj
index 5b42fb5434..bc57b61db4 100644
--- a/ios/Exponent.xcodeproj/project.pbxproj
+++ b/ios/Exponent.xcodeproj/project.pbxproj
@@ -1,6 +1,8 @@
 // !$*UTF8*$!
 {
        archiveVersion = 1;
+
+
        classes = {
        };
        objectVersion = 46;

.gitattributes Outdated
@@ -1,4 +1,6 @@
*.pbxproj -diff linguist-generated
*.pbxproj binary linguist-generated
ios/Exponent.xcodeproj/project.pbxproj merge=mergepbx -text diff linguist-generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for applying my suggestion! I've inspected the mergepbx library and started to wonder if… this will work automatically? The setup on https://github.com/simonwagner/mergepbx#using-mergepbx-as-a-merge-driver suggests to also edit ~/.gitconfig — I don't suppose we'd want to make all the contributors have to modify it manually? Or is it somehow automatically taken care of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, after a second thought I think I'm going to remove it.
Why?

  1. You have to have it installed to be available
  2. Even though it probably might come in handy (I believe every pod install modifies whole .pbxproj file and especially UUIDs are generated from scratch) and union merge would possibly fail miserably in almost every case, I don't think we need sth like this and I haven't tested it so ¯_(ツ)_/¯

@@ -1,4 +1,6 @@
*.pbxproj -diff linguist-generated
*.pbxproj binary linguist-generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Does making pbxproj a binary also hide it from the diff? Is it still possible to expand the diff to see the changes?

IMHO we would like to hide it from the diff 👍, but I would also like to see the changes if I want to. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sjchmiela, yeap, marking file as binary disables diff and CRLF conversion, but from what I've experienced only in git tool. That means neither git show nor git diff would show file content. I believed that this would cause github to show this kind of files (with binary attribute) to be shown as binary file in PRs and previews as well, but it turned out that github is not working like this 🤔 So, if you navigate to https://github.com/expo/expo/pull/8508/files you'll see that all .pbxproj files contents are perfectly visible if you expand the diff 🤔 Maybe I'm missing sth somewhere, but I dunno where and what 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great! 🥇

Copy link
Contributor

@sjchmiela sjchmiela left a comment

Choose a reason for hiding this comment

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

Thank you for thinking it over! 🧠

@@ -1,4 +1,6 @@
*.pbxproj -diff linguist-generated
*.pbxproj binary linguist-generated
Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great! 🥇

@bbarthec bbarthec merged commit d3a5cdb into master Jun 4, 2020
@bbarthec bbarthec deleted the @bbarthec/mark-pbxproj-as-text branch June 4, 2020 16:57
@Kudo Kudo mentioned this pull request Jul 24, 2022
3 tasks
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