Fix the refdb backend rename callback

pygit2_refdb_backend_rename() had an inverted success check on
build_signature(): it returned an error precisely when the signature
was built successfully, and fell through with a NULL signature when
it failed, so the callback could never work. It also decref'd the
signature object twice (it is stolen into the argument tuple by
Py_BuildValue()'s N format), passed a borrowed reference to the
Py_True/Py_False singleton with N, and leaked the callback result
when it was not a Reference.

Two more bugs kept the function from working once the check was
fixed. The Python Signature takes ownership of the git_signature it
wraps, but _who belongs to libgit2 and is freed again on return;
pass a git_signature_dup() copy instead. And git_reference_dup()
cannot be used on the returned reference, because its db field is
only set by git_refdb_rename() after the callback returns; transfer
the pointer directly, as pygit2_refdb_backend_lookup() already does.

Also accept None for the message argument of RefdbBackend.rename():
libgit2 passes NULL when no log message is given (pygit2's
Reference.rename() always does), Py_BuildValue() maps that to None,
and the Python-side method must round-trip it back to NULL. This
matches the z format already used by write() and delete().

Add test_rename_callback, the first test to drive a custom refdb
backend through libgit2: assigning repo.backend only sets a Python
attribute, so the existing tests never exercised the C callbacks.

Fixes #1474

Assisted-by: Kimi Code

Author: jdavid

pygit2/_pygit2.pyi

@@ -599,7 +599,12 @@ class RefdbBackend:
     def has_log(self, ref_name: str, /) -> bool: ...
     def lookup(self, refname: str, /) -> Reference: ...
     def rename(
-        self, old_name: str, new_name: str, force: bool, who: Signature, message: str
+        self,
+        old_name: str,
+        new_name: str,
+        force: bool,
+        who: Signature,
+        message: str | None,
     ) -> Reference: ...
     def write(
         self,

src/refdb_backend.c

@@ -249,31 +249,47 @@ pygit2_refdb_backend_rename(git_reference **out, git_refdb_backend *_be,
         const char *old_name, const char *new_name, int force,
         const git_signature *_who, const char *message)
 {
-    int err;
-    PyObject *args, *who;
     struct pygit2_refdb_backend *be = (struct pygit2_refdb_backend *)_be;
 
-    if ((who = build_signature(NULL, _who, "utf-8")) != NULL)
+    // The Python object takes ownership of the signature, so pass it a copy;
+    // _who belongs to the caller (libgit2).
+    git_signature *signature;
+    int err = git_signature_dup(&signature, _who);
+    if (err != 0) {
+        return err;
+    }
+
+    PyObject *who = build_signature(NULL, signature, "utf-8");
+    if (who == NULL) {
         return GIT_EUSER;
-    if ((args = Py_BuildValue("(ssNNs)", old_name, new_name,
-            force ? Py_True : Py_False, who, message)) == NULL) {
-        Py_DECREF(who);
+    }
+
+    // Py_BuildValue takes ownership of who (N format), even on failure, so
+    // it must not be decref'd past this point.
+    PyObject *args = Py_BuildValue("(ssNNs)", old_name, new_name,
+                                   PyBool_FromLong(force), who, message);
+    if (args == NULL) {
         return GIT_EUSER;
     }
+
     Reference *ref = (Reference *)PyObject_CallObject(be->rename, args);
-    Py_DECREF(who);
     Py_DECREF(args);
 
-    if ((err = git_error_for_exc()) != 0)
+    err = git_error_for_exc();
+    if (err != 0) {
         return err;
+    }
 
     if (!PyObject_IsInstance((PyObject *)ref, (PyObject *)&ReferenceType)) {
         PyErr_SetString(PyExc_TypeError, "Expected object of type pygit2.Reference");
+        Py_DECREF(ref);
         return GIT_EUSER;
     }
 
-    git_reference_dup(out, ref->reference);
-    Py_DECREF(ref);
+    // Ownership of the underlying git_reference is transferred to libgit2,
+    // which sets its db field itself once the callback returns, so the Python
+    // object must not be decref'd; same as in pygit2_refdb_backend_lookup.
+    *out = ref->reference;
     return 0;
 }
 
@@ -625,7 +641,7 @@ RefdbBackend_write(RefdbBackend *self, PyObject *args)
 }
 
 PyDoc_STRVAR(RefdbBackend_rename__doc__,
-    "rename(old_name: str, new_name: str, force: bool, who: Signature, message: str) -> Reference\n"
+    "rename(old_name: str, new_name: str, force: bool, who: Signature, message: str | None) -> Reference\n"
     "\n"
     "Renames a reference.");
 
@@ -643,7 +659,7 @@ RefdbBackend_rename(RefdbBackend *self, PyObject *args)
         return Py_NotImplemented;
     }
 
-    if (!PyArg_ParseTuple(args, "sspO!s", &old_name, &new_name,
+    if (!PyArg_ParseTuple(args, "sspO!z", &old_name, &new_name,
                 &force, &SignatureType, &who, &message))
         return NULL;
 

test/test_refdb_backend.py

@@ -41,8 +41,15 @@
 # design.
 class ProxyRefdbBackend(pygit2.RefdbBackend):
     def __init__(self, source: pygit2.RefdbBackend) -> None:
+        super().__init__()
         self.source = source
 
+    def __iter__(self) -> 'ProxyRefdbBackend':
+        return self
+
+    def __next__(self) -> Reference:
+        raise StopIteration
+
     def exists(self, ref: str) -> bool:
         return self.source.exists(ref)
 
@@ -61,7 +68,12 @@ def write(
         return self.source.write(ref, force, who, message, old, old_target)
 
     def rename(
-        self, old_name: str, new_name: str, force: bool, who: Signature, message: str
+        self,
+        old_name: str,
+        new_name: str,
+        force: bool,
+        who: Signature,
+        message: str | None,
     ) -> Reference:
         return self.source.rename(old_name, new_name, force, who, message)
 
@@ -112,6 +124,18 @@ def test_rename(repo: Repository) -> None:
     assert repo.backend.lookup('refs/heads/intl').target == target.id
 
 
+def test_rename_callback(repo: Repository) -> None:
+    # Exercise the custom backend's rename callback through libgit2's
+    # git_reference_rename; calling repo.backend.rename() directly bypasses it.
+    refdb = pygit2.Refdb.new(repo)
+    refdb.set_backend(repo.backend)
+    repo.set_refdb(refdb)
+    ref = repo.references['refs/heads/i18n']
+    target = ref.target
+    ref.rename('refs/heads/intl')
+    assert repo.references['refs/heads/intl'].target == target
+
+
 def test_delete(repo: Repository) -> None:
     old = repo.backend.lookup('refs/heads/i18n')
     repo.backend.delete('refs/heads/i18n', old.target, None)