On Mon, Apr 26, 2021 at 04:28:30PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> @@ -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
