Fix for TypeError in Repository.ident (plus typo fixes)#1461
Open
rapidcow wants to merge 3 commits intolibgit2:masterfrom
Open
Fix for TypeError in Repository.ident (plus typo fixes)#1461rapidcow wants to merge 3 commits intolibgit2:masterfrom
rapidcow wants to merge 3 commits intolibgit2:masterfrom
Conversation
Since its introduction in commit 81520c9 (Update to libgit2 v0.23), the reflog identity getter (Repository.ident) has been passing the (char **) variables directly to ffi.string() as C-strings, when they really are pointers to C-strings. Fix by dereferencing the pointer to a C-string first. Furthermore, Repository.ident assumes that said variables hold valid C-strings, when the reflog identity is initialized to NULL, and can be explicitly set to such with `set_ident (None, None)`. So map None to ffi.NULL (which maybe_string() happens to already do, so I simply delegated to that...). I also added a few regression tests to ensure neither issue shows up again. Signed-off-by: Ethan Meng <ethan@rapidcow.org>
Since set_ident() already has type annotations, it seems fitting that we adorn ident() with those too. :) Also, to_bytes() is perfectly capable of mapping None to ffi.NULL, so the parameters on set_ident() should be `str | None` (aka Optional[str]). Signed-off-by: Ethan Meng <ethan@rapidcow.org>
Repository.default_signature is a read-only descriptor, not a getter method. Signed-off-by: Ethan Meng <ethan@rapidcow.org>
Author
|
The commit wording was a bit bad, sorry... |
This file contains hidden or 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
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.
As of pygit2 1.19.2 (on my Python 3.14.4, Linux 6.19.14-artix1-1 x86_64), the
Repository.identproperty triggers TypeError:cnameandcemailarechar **variables, which are pointers to C-strings, not the C-strings themselves. So they should be passed ascname[0]andcemail[0].In addition,
cname[0]andcemail[0]appear to be NULL by default... so clearly we have to handle that separately.Either way, I wrote up a patch (plus a typo I happened to catch when trying to skim the docs for other mentions of identity). Not sure if this is the right way to contribute here... any feedback* is appreciated! :)
*Specifically, (1) I'm not certain if
utils.maybe_string()is the right tool for this, since the original UTF-8 decode mode was"strict", but is now"surrogateescape"(but I don't want to introduce a separate function just for this, so...) Also (2) I used PEP 585 syntax for the tuple annotation, while keeping Optional since it has already been imported (pygit2 itself seems to use a mix of pre- and post-PEP 585 syntax, and ruff doesn't appear to complain either way, so I picked the ones I liked the best without importing more names...)