-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Updates for issue #150 to use UTF8 instead of String when dealing with data from the git repository #229
Conversation
…ealing with data from the git repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me (I haven't tested it though).
Great, what does it take to release it into the asset library? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following places probably should have UTF-8 conversion as well:
Old path:
godot::String old_path = entry->head_to_index->old_file.path; |
Both "name" here:
godot-git-plugin/godot-git-plugin/src/git_plugin.cpp
Lines 276 to 278 in 2dc9e61
branch_names.push_front(name); | |
} else { | |
branch_names.push_back(godot::String(name)); |
"name" here:
godot::String author = godot::String() + sig->name + " <" + sig->email + ">"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And few more:
Return value:
return name; |
Remotes:
remotes.push_back(remote_array.strings[i]); |
Bunch of godot::String(...)
uses in git_callbacks.cpp
Username:
godot::String proper_username = username_from_url ? username_from_url : creds->username; |
@markeel Can you make the requested changes? I'll merge this afterwards. |
Yes, I'll take a look at it tomorrow morning.
…On Wed, Apr 3, 2024, 8:36 AM Hugo Locurcio ***@***.***> wrote:
@markeel <https://github.com/markeel> Can you make the requested changes?
I'll merge this afterwards.
—
Reply to this email directly, view it on GitHub
<#229 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJWU3MV2D7X2FWXYKI6K6LDY3QOXHAVCNFSM6AAAAABFSFTIM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZUHE2DKMZTG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I incorporated the items @bruvzg identified in his review. Those were subtle impacts. The only change I didn't apply was the one in git_callbacks related to username. The trickiness with this fix, is whether the string was already a "godot" string. In the case of the username it was coming from credentials and therefore was a "godot" string. When it comes from the URL the string cannot be UTF-8 since those are invalid characters in a URL. During testing (where I created repositories and branch names with emojis in the name) I ran into problems where the messages being logged to the "Output" window using the UtilityFunction::print(...) methods needed to be updated. Many of the calls were converting from a "godot" string back to a CString, but then the print function would convert it back to a "godot" string but not convert the C string as a UTF-8. Since the print() method takes godot string it was more accurate and effective to remove this extraneous conversion to CString. I also tested error legs, by shutting down the server running my remote repositories to see how pull and push reported errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Re-implemented the original update for UTF-8 based on latest master, for a clean merge.
Replaced the places where the libgit2 API used godot::String with godot::String::utf8
Tested using emojis as place holders for language specific Unicode characters