On Fri, Apr 23 2021, Jeff King wrote:
> On Tue, Apr 13, 2021 at 11:43:08AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Continue the work in the preceding commit and improve the error on:
>>
>> $ git hash-object --stdin -w -t garbage --literally </dev/null
>> $ git fsck
>> error: hash mismatch for <OID_PATH> (expected <OID>)
>> error: <OID>: object corrupt or missing: <OID_PATH>
>> [ other fsck output ]
>>
>> To instead emit:
>>
>> $ git fsck
>> error: <OID>: object is of unknown type 'garbage': <OID_PATH>
>> [ other fsck output ]
>
> Sounds good.
>
>> @@ -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'll sort that out in some sensible way. Starting with adding meaningful
test coverage for the existing behavior.
