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

[Patrick Steinhardt <ps@pks.im>]

On Thu, Aug 20, 2026 at 08:56:50AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> 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