Subject: Re: [PATCH v6 02/22] fsck tests: add test for fsck-ing an unknown type

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 17 Sep 2021 11:39:02 +0200

On Thu, Sep 16 2021, Taylor Blau wrote:

> On Tue, Sep 07, 2021 at 12:57:57PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> Fix a blindspot in the fsck tests by checking what we do when we
>> encounter an unknown "garbage" type produced with hash-object's
>> --literally option.
>>
>> This behavior needs to be improved, which'll be done in subsequent
>> patches, but for now let's test for the current behavior.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
>> ---
>>  t/t1450-fsck.sh | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
>> index 7becab5ba1e..f10d6f7b7e8 100755
>> --- a/t/t1450-fsck.sh
>> +++ b/t/t1450-fsck.sh
>> @@ -863,4 +863,16 @@ test_expect_success 'detect corrupt index file in fsck' '
>>  	test_i18ngrep "bad index file" errors
>>  '
>>
>> +test_expect_success 'fsck hard errors on an invalid object type' '
>> +	git init --bare garbage-type &&
>
> I wondered whether it was really possible to not cover this, since I
> figured such a test may have just been hiding elsewhere. But we really
> do seem to be lacking coverage. So, adding this test is good.
>
>> +	empty_blob=$(git -C garbage-type hash-object --stdin -w -t blob </dev/null) &&
>> +	garbage_blob=$(git -C garbage-type hash-object --stdin -w -t garbage --literally </dev/null) &&
>
> I'm nitpicking, but I find the -C garbage-type pattern less than ideal
> for two reasons:
>
>   - It makes every line longer (since "-C garbage type" is wider than an
>     8-wide tab, even indenting this in a subshell would take up fewer
>     characters visually)
>
>   - It pollutes the current directory with things like "err.expect" and
>     "err.actual" that have nothing to do with the current directory (and
>     much more to do with the garbage-type repository within it).
>
> So I don't care, really, but it may be better to just put all of this in
> a subshell.

Yes, it does look much nicer like that. Thanks!

Some aspects of style I use I have some informed/strong opinion about,
like the teardown/setup pattern noted in [1], but for some other stuff
like this ... I think I was just following the pattern of some recent
test I'd read or something.

Well, one advantage of using "git -C" is that if it fails you can cd to
the trash directory and run the command you saw fail as-is without
cd-ing further, and in that case the "polluting" is a feature, you can
cat the top-level expect/actual consistently.

But I think on balance having the test itself be easier to read is more
important, so I'm going with the subshell.

1. https://lore.kernel.org/git/[email-masked]/