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

Karthik Nayak <[email protected]> Thu, 20 Aug 2026 13:59:48 -0700

Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZTKqPa-9j8UzqQQCVwVk=Yt7aQVxg9qSi41Q+=A-6dAow@mail.gmail.com>

Patrick Steinhardt <[email protected]> writes:

> On Thu, Aug 20, 2026 at 08:41:10AM -0400, Karthik Nayak wrote:
>> Patrick Steinhardt <[email protected]> writes:
>>
>> > 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.
>
>> > 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.

signature.asc (application/pgp-signature, 690 B)