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

Karthik Nayak <[email protected]> Thu, 20 Aug 2026 08:56:50 -0400
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZSSzR+qKh4Do-F7xZQMO-pE+t4N8qM5hsbfM4Uh7i3d1A@mail.gmail.com>
Patrick Steinhardt <[email protected]> writes:

> As explained in the preceding commits, reading objects can either fail
> because the object truly does not exist or because it exists, but its
> data is corrupt. Some callers do care about this distinction, but there
> is no way to tell these two cases apart right now.
>
> Introduce a new `ODB_READ_NOT_FOUND` value that ought to be returned by
> the backends in case the object truly does not exist and adapt backends
> to use it.
>
> Note that we don't yet return this error from `odb_read_object_info()`
> itself. This will be fixed in a subsequent commit.
>
> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  odb.h                         |  2 ++
>  odb/source-files.c            | 20 +++++++++++++++++---
>  odb/source-inmemory.c         |  2 +-
>  odb/source-loose.c            | 31 +++++++++++++++++++------------
>  odb/source-packed.c           |  2 +-
>  t/unit-tests/u-odb-inmemory.c |  3 ++-
>  6 files changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/odb.h b/odb.h
> index 43cbcc3aba..1264d4ce7d 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -440,6 +440,8 @@ enum odb_read_status {
>  	ODB_READ_OK = 0,
>  	/* The read resulted in a generic error. */
>  	ODB_READ_ERROR = -1,
> +	/* The object could not be found. */
> +	ODB_READ_NOT_FOUND = -2,
>  };
>
>  /*
> 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;


> -	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?

[snip]

The rest look in order.
signature.asc (application/pgp-signature, 690 B)