On Tue, Apr 13, 2021 at 11:43:06AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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?
> -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...
> --- 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
