Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them

From: Jeff King <peff@peff.net>
Date: Dec 2, 2022, 00:23 UTC
Message-ID: <Y4lFbemK4HHiCsyJ@coredump.intra.peff.net>
In-Reply-To: <20221201212650.414069-1-jonathantanmy@google.com>

On Thu, Dec 01, 2022 at 01:26:50PM -0800, Jonathan Tan wrote:

Show 22 quoted lines

> > 1734 void die_if_corrupt(struct repository *r,
> > 1735                     const struct object_id *oid,
> > 1736                     const struct object_id *real_oid)
> > 1737 {
> > 1738         const struct packed_git *p;
> > 1739         const char *path;
> > 1740         struct stat st;
> > 1741
> > 1742         obj_read_lock();
> > 1743         if (errno && errno != ENOENT)
> > 1744                 die_errno(_("failed to read object %s"), oid_to_hex(oid));
> 
> I wonder if we could just remove this check. Even as it is, I don't think that
> there is any guarantee that obj_read_lock() would not clobber errno. Removing
> it makes all tests pass locally, but I haven't tried it on CI.
> 
> (One argument that could be made is that we shouldn't have any die_if_corrupt()
> refactoring or other refactoring of the sort, because previously its contents
> was part of a function and it could thus rely on the errno of what has happened
> previously. But I think that even without my patches, we couldn't rely on it
> in the first place - looking at obj_read_lock(), it looks like it could init a
> mutex, and depending on the implementation of that, it could clobber errno.)

Yeah, I don't see any difference in the new caller versus what the original was doing. The errno we care about comes from inside oid_object_info_extended(). So in any case, we'll see at least obj_read_unlock() followed by obj_read_lock() between the syscalls of interest and this check. And I don't even really see any indication that oid_object_info_extended() tries to set or preserve errno itself. The likely sequence is:

  - find_pack_entry() fails to find it; errno isn't set at all
  - loose_object_info() tries to open it and probably gets ENOENT
  - we check find_pack_entry() again after reprepare_packed_git()
  - that fails so we return -1, barring submodule or partial clone
    tricks

So it really seems like we're quite likely to get an errno from opening or mapping packs. Which implies the original suffers from the same issue, but we simply never triggered it meaningfully in a test.

I'm not entirely sure on just removing the check. It comes from 3ba7a06552 (A loose object is not corrupt if it cannot be read due to EMFILE, 2010-10-28), so we'd lose what that commit is trying to do. Though I think even back then, I think it would have suffered from the same problems (minus the lock/unlock; I'm still unclear which syscall is the actual culprit here).

If we assume that errno from reading the object isn't reliable, I think you'd have to actually re-check things. Something like:

  if (find_pack_entry(...) || !stat_loose_object(...))
    /* ok, it's not missing */

but of course we don't have the actual errno that _did_ cause us to fail, which makes the error message we'd print a lot less useful. Maybe this check should be ditched and we should complain much closer to the source of the problem:

diff --git a/object-file.c b/object-file.c
index 26290554bb..743ba8210e 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1460,8 +1460,12 @@ static int loose_object_info(struct repository *r,
 	}
 
 	map = map_loose_object(r, oid, &mapsize);
-	if (!map)
+	if (!map) {
+		if (errno != ENOENT)
+			error_errno("unable to open loose object %s",
+				    oid_to_hex(oid));
 		return -1;
+	}
 
 	if (!oi->sizep)
 		oi->sizep = &size_scratch;

That might make things more verbose for other code paths, but that kind
of seems like a good thing. If you have an object file that we can't
open, we probably _should_ be complaining loudly about it.

We may need to be a little more careful about preserving errno in
map_loose_object_1(), though (gee, another place where the existing
check could run into trouble).

-Peff