Re: [PATCH v2 5/6] fsck: report invalid types recorded in objects

From: Jeff King <peff@peff.net>
Date: Apr 26, 2021, 15:45 UTC
Message-ID: <YIbgEaY+cie/4h2X@coredump.intra.peff.net>
In-Reply-To: <87im493yos.fsf@evledraar.gmail.com>

On Mon, Apr 26, 2021 at 04:28:30PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 50 quoted lines

> >> @@ -92,7 +93,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
> >>  	switch (opt) {
> >>  	case 't':
> >>  		oi.type_name = &sb;
> >> -		if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
> >> +		ret = oid_object_info_extended(the_repository, &oid, &oi, flags);
> >> +		if (!unknown_type && ret < 0)
> >>  			die("git cat-file: could not get object info");
> >>  		if (sb.len) {
> >>  			printf("%s\n", sb.buf);
> >
> > Surprised to see changes to cat-file here, since the commit message is
> > all about fsck. Did the semantics of oid_object_info_extended() change?
> > I.e., this hunk implies to me that it is now returning -1 when we said
> > unknown types were OK, and we got one. But in that case, how do we
> > distinguish that from a real error?
> >
> > Or more concretely, this patch causes this:
> >
> >   $ git cat-file -t 1234567890123456789012345678901234567890
> >   fatal: git cat-file: could not get object info
> >
> >   $ git.compile cat-file --allow-unknown-type -t 1234567890123456789012345678901234567890
> >   fatal: git cat-file 1234567890123456789012345678901234567890: bad file
> >
> > Or much worse, from the next hunk:
> >
> >   $ git cat-file -s 1234567890123456789012345678901234567890
> >   fatal: git cat-file: could not get object info
> >
> >   $ git cat-file --allow-unknown-type -s 1234567890123456789012345678901234567890
> >   140732113568960
> >
> > That seems wrong (so I think my "this hunk implies" is not true, but
> > then I am left with: what is the point of this hunk?).
> 
> That's very well spotted.
> 
> I started re-rolling this today but ran out of time. For what it's worth
> the combination of this and 6/6 "makes sense" in the sense that all
> tests pass at the end of this series.
> 
> But the cases you're pointing out are ones we don't have tests for,
> i.e. the combination of "allow unknown" and a non-existing object, as
> opposed to a garbage one.
> 
> Hence the bug with passing up an invalid (uninitialized) size in that
> case. It's fallout from other partial lib-ification changes of these
> APIs, i.e. making them return bad values upstream instead of dying right
> away.

I'm not sure I understand. The problem seems solely in the hunk above. Before, if we got an error from oid_object_info_extended(), we stopped immediately. But after, we look at the results even though it told us there was an error. In general, I would think that a "-1" return value from oid_object_info_extended() is "all bets are off" (remember that unlike oid_object_info(), this is a strict error return, and not trying to force the object type into the return value).

And that's independent of what the other patches in the series are doing, I think.

> I'll sort that out in some sensible way. Starting with adding meaningful
> test coverage for the existing behavior.

Yeah, that sounds fine. I think the current behavior there is perfectly reasonable (fail with "could not get object info").

-Peff