Re: [PATCH v2 3/6] fsck: don't hard die on invalid object types

From: Jeff King <peff@peff.net>
Date: Apr 23, 2021, 14:26 UTC
Message-ID: <YILZHiuUyj0mt958@coredump.intra.peff.net>
In-Reply-To: <patch-3.6-d0d9cb33315-20210413T093734Z-avarab@gmail.com>

On Tue, Apr 13, 2021 at 11:43:06AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 27 quoted lines

> Change builtin/fsck.c to pass down a
> OBJECT_INFO_ALLOW_UNKNOWN_TYPE. This changes this very ungraceful
> error:
> 
>     $ git hash-object --stdin -w -t garbage --literally </dev/null
>     <OID>
>     $ git fsck
>     fatal: invalid object type
>     $
> 
> Into:
> 
>     $ git fsck
>     error: hash mismatch for <OID_PATH> (expected <OID>)
>     error: <OID>: object corrupt or missing: <OID_PATH>
>     [ the rest of the fsck output here, i.e. it didn't hard die ]
> 
> We'll still exit with non-zero, but now we'll finish the rest of the
> traversal. The tests that's being added here asserts that we'll still
> complain about other fsck issues (e.g. an unrelated dangling blob).
> 
> But why are we complaining about a "hash mismatch" for an object of a
> type we don't know about? We shouldn't. This is the bare minimal
> change needed to not make fsck hard die on a repository that's been
> corrupted in this manner. In subsequent commits we'll teach fsck to
> recognize this particular type of corruption and emit a better error
> message.

OK. The overall goal makes sense.

> The parse_loose_header() function being changed here is only used in
> builtin/fsck.c, see f6371f92104 (sha1_file: add read_loose_object()
> function, 2017-01-13) for its introduction.

This left me scratching my head for a long time. Did you mean read_loose_object() in the beginning of the sentence?

Show 5 quoted lines

> -static int parse_loose_header_extended(const char *hdr, struct object_info *oi,
> -				       unsigned int flags)
> +int parse_loose_header(const char *hdr,
> +		       struct object_info *oi,
> +		       unsigned int flags)

So we are getting rid of the "extended" form and just making the non-extended way take an OI. That seems kind of orthogonal...

Show 9 quoted lines

> --- a/streaming.c
> +++ b/streaming.c
> @@ -341,6 +341,9 @@ static struct stream_vtbl loose_vtbl = {
>  
>  static open_method_decl(loose)
>  {
> +	struct object_info oi2 = OBJECT_INFO_INIT;
> +	oi2.sizep = &st->size;
> +

...and is what leads us to having to touch this otherwise unrelated function.

I don't mind _too_ much getting rid of a helper function that would have only one caller remaining (though "oi2" is a bit mysterious here). But it seems like the patch would have been a lot easier to understand if that were separately done (and explained). AFAICT the functional change here is just passing the flag to read_loose_object(), which could be calling parse_loose_header_extended(). I guess that would have to become public, but that seems reasonable.

-Peff