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.

Sign up for GitHub

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

Jump to bottom

Fix "can't modify frozen string" errors when pods are integrated using the branch option (#10920) #119

Merged
merged 1 commit into from Sep 7, 2021
Merged

Fix "can't modify frozen string" errors when pods are integrated using the branch option (#10920) #119

merged 1 commit into from Sep 7, 2021

Conversation

Buju77
Copy link

@Buju77 Buju77 commented Sep 7, 2021

@Buju77 Buju77 mentioned this pull request Sep 7, 2021
Closed
1 task
dnkoutso
dnkoutso reviewed Sep 7, 2021
View reviewed changes
lib/cocoapods-downloader/git.rb Outdated
@@ -52,7 +52,7 @@ def self.preprocess_options(options)
#
def self.commit_from_ls_remote(output, branch_name)
return nil if branch_name.nil?
encoded_branch_name = branch_name.force_encoding(Encoding::ASCII_8BIT)
encoded_branch_name = branch_name.encode(Encoding::ASCII_8BIT)
Copy link
Contributor

Choose a reason for hiding this comment

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

let me see what I can do because I think we do not want to change the underlying encoding of this string.

@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 7, 2021

Follow up to #116 (for posterity).

@Buju77 can you also try this fix instead?

encoded_branch_name = branch_name.dup.force_encoding(Encoding::ASCII_8BIT)

?

@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 7, 2021

Also lets point to 1-5-stable branch since we will do a quick 1.5.1 release for this fix.

@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 7, 2021

I think this fix will suffice.

@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 7, 2021

@Buju77 lets make sure we point to 1-5-stable branch and rebase this PR before we merge it.

dnkoutso
dnkoutso reviewed Sep 7, 2021
View reviewed changes
CHANGELOG.md Outdated
@@ -8,8 +8,9 @@

##### Bug Fixes

* None.

* Fix "can't modify frozen string" errors when pods are integrated using the `branch` option
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please add two empty spaces at the end of this line for markdown formatting.

@Buju77
Copy link
Author

Buju77 commented Sep 7, 2021

Follow up to #116 (for posterity).

@Buju77 can you also try this fix instead?

encoded_branch_name = branch_name.dup.force_encoding(Encoding::ASCII_8BIT)

yes, this works as well.

But I thought that branch_name.encode() would already return a copy and doesn't modify branch_name itself?

@Buju77 Buju77 changed the base branch from master to 1-5-stable September 7, 2021 15:36
@Buju77
Copy link
Author

Buju77 commented Sep 7, 2021

@Buju77 lets make sure we point to 1-5-stable branch and rebase this PR before we merge it.

ok, I've now rebased on top of 1-5-stable

@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 7, 2021

@Buju77 I think you are right and we can fix it your way

@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 7, 2021

oh well we can land this now! :D

@dnkoutso dnkoutso merged commit 6564809 into CocoaPods:1-5-stable Sep 7, 2021
@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 7, 2021

I will ship 1.5.1 of cocoapods-downloader

@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 7, 2021

@Buju77 shipped! https://github.com/CocoaPods/cocoapods-downloader/releases/1.5.1

@Buju77
Copy link
Author

Buju77 commented Sep 7, 2021

@dnkoutso thank you very much! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnkoutso dnkoutso dnkoutso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[!] Failed to download 'profile': can't modify frozen String
2 participants
@Buju77 @dnkoutso