Skip to content

Fix for TypeError in Repository.ident (plus typo fixes)#1461

Open
rapidcow wants to merge 3 commits intolibgit2:masterfrom
rapidcow:fix-repo-ident
Open

Fix for TypeError in Repository.ident (plus typo fixes)#1461
rapidcow wants to merge 3 commits intolibgit2:masterfrom
rapidcow:fix-repo-ident

Conversation

@rapidcow
Copy link
Copy Markdown

As of pygit2 1.19.2 (on my Python 3.14.4, Linux 6.19.14-artix1-1 x86_64), the Repository.ident property triggers TypeError:

Python 3.14.4 (main, Apr 15 2026, 07:34:47) [GCC 15.2.1 20260209] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pygit2; pygit2.__version__
'1.19.2'
>>> repo = pygit2.init_repository('/tmp/foo')
>>> repo.ident
Traceback (most recent call last):
  File "<python-input-2>", line 1, in <module>
    repo.ident
  File "/usr/lib/python3.14/site-packages/pygit2/repository.py", line 1588, in ident
    return (ffi.string(cname).decode('utf-8'), ffi.string(cemail).decode('utf-8'))
            ~~~~~~~~~~^^^^^^^
TypeError: string(): unexpected cdata 'char * *' argument

cname and cemail are char ** variables, which are pointers to C-strings, not the C-strings themselves. So they should be passed as cname[0] and cemail[0].

In addition, cname[0] and cemail[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...)

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>
@rapidcow rapidcow marked this pull request as ready for review April 30, 2026 23:28
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>
@rapidcow rapidcow marked this pull request as draft April 30, 2026 23:32
@rapidcow
Copy link
Copy Markdown
Author

The commit wording was a bit bad, sorry...

# git range-diff @{u}~3..@{u} @~3..@
1:  ea5b51b7 ! 1:  d32573ec fix(repository): dereference reflog identity correctly
    @@ Commit message
         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 libgit2 holds
    -    a valid C-string, when the reflog identity is initialized
    +    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...).
2:  9b65c2d6 = 2:  379b4d82 fix(repository): amend annotations on reflog identity
3:  1deb18ba = 3:  e7bd6d26 docs(repository): fix default_signature typos

@rapidcow rapidcow marked this pull request as ready for review April 30, 2026 23:36
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.

1 participant