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

On Thu, Aug 20, 2026 at 08:56:50AM -0400, Karthik Nayak wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

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.

quoted

-	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