Re: [PATCH v2 2/5] odb/source: introduce error status when reading objects

Patrick Steinhardt [off-list ref] writes:

On Thu, Aug 20, 2026 at 08:41:10AM -0400, Karthik Nayak wrote:

quoted

Patrick Steinhardt [off-list ref] writes:

quoted

The `read_object_info()` callback of `struct odb_source` is documented
to return a negative error code in case reading the object has failed,
and zero otherwise. This is overly broad though, as there are two very
different kinds of failures:

  - The object may not exist in the source at all.

  - The object exists, but reading it has failed, for example because
    its on-disk state is corrupt.

This distinction matters to callers: when an object is corrupt in one
source we may still find a good copy of it in another source, so we may
still be able to proceed with a given operation.

But isn't that the same for an object not existing in a source? If it
doesn't exist in one source, we may find a good copy of it in another?

Yeah, that paragraph is a bit odd indeed. What I really wanted to say is
that the failure mode is different depending on whether the object is
found at all: if it's not then we'd fail gracefully, if it is but it's
corrupt then we die.

quoted

quoted

diff --git a/odb.c b/odb.c
index caf1d0f542..1b37b26376 100644
--- a/odb.c
+++ b/odb.c

@@ -696,12 +696,12 @@ static int oid_object_info_convert(struct repository *r,
 	return ret;
 }

Here and elsewhere. Shouldn't we explicitly return ODB_READ_OK or
ODB_READ_ERROR instead of relying on implicit conversion?

I didn't want to go through the complete callchain to make sure that we
explicitly return those values. I think it'd be mostly pointless: the
return code convention is established enough, and all callers already
return the expected values anyway, even though they're not using the
enum now.

Patrick

I think logically it is correct already, but returning an enum type but
seeing -1,0,1 in the return statements means that we have to either
remember the different enum values or we need to cross reference each
time. Eventually someone would start using 'return ODB_READ_OK' and so
on and then we'd have a mix of both (this argument does go both way).
Anyway, it is fine as is.