Re: [PATCH v2 3/5] odb/source: let callers discern missing and corrupt objects

Karthik Nayak <[email protected]> Thu, 20 Aug 2026 17:09:51 -0400
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZSs-9VU2eKT8DUJ7FzZCAkgRzZ6_XQZBP=x7avxpFp7qw@mail.gmail.com>
Patrick Steinhardt <[email protected]> writes:

> On Thu, Aug 20, 2026 at 08:56:50AM -0400, Karthik Nayak wrote:
>> Patrick Steinhardt <[email protected]> writes:
>> > diff --git a/odb/source-files.c b/odb/source-files.c
>> > index a28aa5042d..e88fd1d399 100644
>> > --- a/odb/source-files.c
>> > +++ b/odb/source-files.c
>> > @@ -65,12 +65,26 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>> >  							      enum object_info_flags flags)
>> >  {
>> >  	struct odb_source_files *files = odb_source_files_downcast(source);
>> > +	enum odb_read_status ret_packed, ret_loose;
>> >
>> > -	if (!odb_source_read_object_info(&files->packed->base, oid, oi, flags) ||
>> > -	    !odb_source_read_object_info(&files->loose->base, oid, oi, flags))
>> > +	ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags);
>> > +	if (!ret_packed)
>> >  		return 0;
>> >
>>
>> Nit: Similar to my previous comment, wouldn't it be nicer to do
>>
>>      if (ret_packed == ODB_READ_OK)
>>         return 0;
>
> As mentioned in the preceding commit, I think it would be somewhat
> pointless and only make the code more verbose without much of a purpose.
>
>> > -	return -1;
>> > +	ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags);
>> > +	if (!ret_loose)
>> > +		return 0;
>> > +
>> > +	/*
>> > +	 * Reading the packed object may have failed even though the object
>> > +	 * exists, for example because it is corrupt. Report this failure to
>> > +	 * the caller in case neither of the sources was able to read the
>> > +	 * object, and prefer the error of the packed source in case both
>> > +	 * reads have failed.
>> > +	 */
>> > +	if (ret_packed != ODB_READ_NOT_FOUND)
>> > +		return ret_packed;
>> > +	return ret_loose;
>> >  }
>> >
>>
>> So if we already found the source we return early and only come here for
>> errors. What I don't understand is why we filter out ODB_READ_NOT_FOUND
>> for packed. Wouldn't that leave us with
>>
>>     ret_packed => ODB_READ_ERROR
>>     ret_loose  => ODB_READ_ERROR or ODB_READ_NOT_FOUND
>>
>> Doesn't this come down to preferring to propagate ODB_READ_NOT_FOUND over
>> ODB_READ_ERROR and now packed error over loose?
>
> So here we know that we didn't find the object. So there's four cases:
>
>   - The object was not found in either, and we'll return
>     ODB_READ_NOT_FOUND.
>
>   - The object was not found in the "packed" source but was found in the
>     "loose" source. So we'd have `ret_packed == ODB_READ_NOT_FOUND` and
>     `ret_loose` at any other error code. And consequently this block:
>
>         if (ret_packed != ODB_READ_NOT_FOUND)
>             return ret_packed;
>
>     Would not trigger as `ret_packed` _is_ ODB_READ_NOT_FOUND. Hence, we
>     favor the error from `ret_loose`, which contains our corruption
>     error.
>
>   - The reverse case, where the object exists in the "packed" backend
>     but is corrupt. In that case `ret_packed != ODB_READ_NOT_FOUND`
>     evaluates true, and we bubble up that error.
>
>   - Both sources have a corrupt object. If so, we simply favor the
>     packed error because we have to pick one.
>
> I think you've simply misread the condition, as we do exactly the
> reverse.
>
> Patrick

Oops. Thanks for the detailed response.

I think I made my case in reverse, but my original argument still
holds.

ret_packed   ret_loose    ret_packed != NOT_FOUND ?   returned
-----------  -----------  ---------------------------  -----------------
NOT_FOUND    NOT_FOUND    false                        ret_loose (NOT_FOUND)
NOT_FOUND    ERROR        false                        ret_loose  (ERROR)
ERROR        NOT_FOUND    true                         ret_packed (ERROR)
ERROR        ERROR        true                         ret_packed (ERROR)

So since we return ret_loose as many times as ret_packed. The comment:

> and prefer the error of the packed source in case both reads have
> failed.

isn't true entirely. So isn't it better modified to something like
"prefer other errors over not found errors" or something. I hope that
makes sense?

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