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

Patrick Steinhardt [off-list ref] writes:

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

quoted

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

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

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