Re: [PATCH 1/7] odb/source: discern missing and corrupt objects

On Tue, Aug 18, 2026 at 11:00:40AM -0700, Junio C Hamano 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.
> >
> > The "packed" source already distinguishes these cases by returning a
> > positive value for missing objects and a negative value in case reading
> > the object has failed. But all the other sources conflate them into a
> > single negative return value.
> 
> In other words, "packed" did not honor the documented contract with
> the callers and nobody noticed?  It gives us a usable escape hatch ;-)

Yes, kind of. It didn't matter much though, as the "files" backend
knew to translate the positive value into a negative one.

> Do we need to support many other "it is an error but we treat as non
> error in some context" values, like the "does not exist"?  If so, it
> does make sense to say 0 is absolute success, positive values are
> such half-errors, and negative values are absolute failures.  If
> not, it would have been much nicer if "you asked me about this
> information but there is no such object" were still signalled as an
> error (i.e., negative return value) that is distinct from other
> kinds of errors like I/O error (which also should be signalled by a
> negative return value), instead of a positive value whose meanings
> were not defined, though.

I cannot think of any other classes of errors where we'd want to fail
gracefully from the top of my head. The only one that's potentially
worth thinking about is in case an object disappears right while we are
looking at it. But that's basically just another edge case of a missing
object.

In any case, I think I'm aligned with the proposal to turn this into a
proper enum and then use negative values exclusively. Thanks!

Patrick