Fix stale libgit2 errors clobbering Python backend exceptions

When a custom ODB or refdb callback raised an arbitrary Python exception,
git_error_for_exc() returned GIT_EUSER and left the real exception pending.
The entry points then called Error_set/err/str/oid, which read
git_error_last() and overwrote the pending exception with whatever stale
text happened to be in libgit2's buffer (commonly an OSError about
.git/shallow).

Preserve the pending Python exception when err == GIT_EUSER in the three
Error_set* helpers, so the callback's original exception propagates. This
does not affect the KeyError -> GIT_ENOTFOUND or ValueError -> GIT_EAMBIGUOUS
mappings, which return different error codes.

Add regression tests for refdb exists/lookup and ODB read/read_prefix/
read_header/exists/exists_prefix callbacks, all expecting the original
RuntimeError to propagate.

Assisted-by: Kimi Code

Author: jdavid

CHANGELOG.md

@@ -6,6 +6,9 @@
   built-in exception (`ValueError`/`KeyError`) for backward compatibility
   [#830](https://github.com/libgit2/pygit2/issues/830).
 
+- Fix custom ODB and refdb backend callbacks overwriting a pending Python
+  exception (e.g. `RuntimeError`) with a stale libgit2 error message.
+
 - Fix `DiffDelta.is_binary` and `DiffDelta.flags` returning stale values for
   deltas obtained from `Diff.deltas`; flags are now loaded lazily
   [#962](https://github.com/libgit2/pygit2/issues/962)

src/error.c

@@ -104,6 +104,11 @@ Error_set(int err)
 {
     assert(err < 0);
 
+    /* GIT_EUSER means a Python callback raised an exception. Preserve that
+     * exception instead of overwriting it with a stale libgit2 error message. */
+    if (err == GIT_EUSER && PyErr_Occurred())
+        return NULL;
+
     return Error_set_exc(Error_type(err));
 }
 
@@ -122,6 +127,10 @@ Error_set_exc(PyObject* exception)
 PyObject *
 Error_set_str(int err, const char *str)
 {
+    /* GIT_EUSER means a Python callback raised an exception. Preserve it. */
+    if (err == GIT_EUSER && PyErr_Occurred())
+        return NULL;
+
     if (err == GIT_ENOTFOUND) {
         /* NotFoundError inherits from KeyError; the argument is the missing key. */
         PyErr_SetString(NotFoundError, str);
@@ -138,6 +147,10 @@ Error_set_str(int err, const char *str)
 PyObject *
 Error_set_oid(int err, const git_oid *oid, size_t len)
 {
+    /* GIT_EUSER means a Python callback raised an exception. Preserve it. */
+    if (err == GIT_EUSER && PyErr_Occurred())
+        return NULL;
+
     char hex[GIT_OID_HEXSZ + 1];
 
     git_oid_fmt(hex, oid);

test/test_odb_backend.py

@@ -95,6 +95,29 @@ def __iter__(self) -> Iterator[Oid]:
         return iter(self.source)
 
 
+class RaisingOdbBackend(pygit2.OdbBackend):
+    """A backend whose callbacks always raise a configurable exception."""
+
+    def __init__(self, exc: Exception) -> None:
+        super().__init__()
+        self.exc = exc
+
+    def read_cb(self, oid: Oid | str) -> tuple[int, bytes]:
+        raise self.exc
+
+    def read_prefix_cb(self, oid: Oid | str) -> tuple[int, bytes, Oid]:
+        raise self.exc
+
+    def read_header_cb(self, oid: Oid | str) -> tuple[int, int]:
+        raise self.exc
+
+    def exists_cb(self, oid: Oid | str) -> bool:
+        raise self.exc
+
+    def exists_prefix_cb(self, oid: Oid | str) -> Oid:
+        raise self.exc
+
+
 #
 # Test a custom object backend alone (without adding it to an ODB)
 # This doesn't make much sense, but it's possible.
@@ -141,6 +164,44 @@ def test_exists_prefix(proxy: ProxyBackend) -> None:
     assert BLOB_HEX == proxy.exists_prefix(a_hex_prefix)
 
 
+@pytest.fixture
+def raising_backend() -> Generator[RaisingOdbBackend, None, None]:
+    yield RaisingOdbBackend(RuntimeError('boom'))
+
+
+def test_read_cb_raises_runtime_error(raising_backend: RaisingOdbBackend) -> None:
+    # Regression test: a RuntimeError in read_cb must propagate as RuntimeError,
+    # not be overwritten by a stale libgit2 error message.
+    with pytest.raises(RuntimeError, match='boom'):
+        pygit2.OdbBackend.read(raising_backend, BLOB_OID)
+
+
+def test_read_prefix_cb_raises_runtime_error(
+    raising_backend: RaisingOdbBackend,
+) -> None:
+    with pytest.raises(RuntimeError, match='boom'):
+        pygit2.OdbBackend.read_prefix(raising_backend, BLOB_HEX[:4])
+
+
+def test_read_header_cb_raises_runtime_error(
+    raising_backend: RaisingOdbBackend,
+) -> None:
+    with pytest.raises(RuntimeError, match='boom'):
+        pygit2.OdbBackend.read_header(raising_backend, BLOB_OID)
+
+
+def test_exists_cb_raises_runtime_error(raising_backend: RaisingOdbBackend) -> None:
+    with pytest.raises(RuntimeError, match='boom'):
+        pygit2.OdbBackend.exists(raising_backend, BLOB_OID)
+
+
+def test_exists_prefix_cb_raises_runtime_error(
+    raising_backend: RaisingOdbBackend,
+) -> None:
+    with pytest.raises(RuntimeError, match='boom'):
+        pygit2.OdbBackend.exists_prefix(raising_backend, BLOB_HEX[:4])
+
+
 #
 # Test a custom object backend, through a Repository.
 #

test/test_refdb_backend.py

@@ -140,24 +140,36 @@ def test_exists(repo: Repository) -> None:
 
 
 class RaisingRefdbBackend(ProxyRefdbBackend):
-    """A backend whose exists callback always raises."""
+    """A backend whose callbacks always raise RuntimeError."""
+
+    def __init__(self, source: pygit2.RefdbBackend, exc: Exception) -> None:
+        super().__init__(source)
+        self.exc = exc
 
     def exists(self, ref: str) -> bool:
-        raise RuntimeError('boom')
-
-
-def test_exists_callback_raises(testrepo: Repository) -> None:
-    # Regression test (issue #1476): when the exists callback raises, the C
-    # wrapper must not crash on a NULL result, and must propagate the error.
-    backend = RaisingRefdbBackend(pygit2.RefdbFsBackend(testrepo))
-    # Call the unbound C method so the call goes through the C wrapper
-    # (pygit2_refdb_backend_exists), not directly to the Python override.
-    # The error propagates as GitError, or as OSError/ValueError when a stale
-    # libgit2 error (with a matching class) is left over from an earlier call.
-    with pytest.raises((pygit2.GitError, OSError)):
+        raise self.exc
+
+    def lookup(self, ref: str) -> Reference:
+        raise self.exc
+
+
+def test_exists_callback_raises_runtime_error(testrepo: Repository) -> None:
+    # Regression test: when the exists callback raises RuntimeError, the C
+    # wrapper must propagate the original Python exception, not overwrite it
+    # with a stale libgit2 error message.
+    backend = RaisingRefdbBackend(pygit2.RefdbFsBackend(testrepo), RuntimeError('boom'))
+    with pytest.raises(RuntimeError, match='boom'):
         pygit2.RefdbBackend.exists(backend, 'refs/heads/master')
 
 
+def test_lookup_callback_raises_runtime_error(testrepo: Repository) -> None:
+    # Regression test: when the lookup callback raises RuntimeError, the C
+    # wrapper must propagate the original Python exception.
+    backend = RaisingRefdbBackend(pygit2.RefdbFsBackend(testrepo), RuntimeError('boom'))
+    with pytest.raises(RuntimeError, match='boom'):
+        pygit2.RefdbBackend.lookup(backend, 'refs/heads/master')
+
+
 def test_lookup(repo: Repository) -> None:
     assert repo.backend.lookup('refs/heads/does-not-exist') is None
     assert repo.backend.lookup('refs/heads/master').name == 'refs/heads/master'