On Thu, Dec 01, 2022 at 01:26:50PM -0800, Jonathan Tan wrote:
> > 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
