Re: [PATCH v2 4/6] t1450: don't create unused files

From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Apr 6, 2023, 22:19 UTC
Message-ID: <35bc2dc5-d5cb-3492-ff94-41b93b7563d4@gmail.com>
In-Reply-To: <230406.86lej5tn8c.gmgdl@evledraar.gmail.com>

On 06/04/2023 10:41, Ævar Arnfjörð Bjarmason wrote:

Show 34 quoted lines

> 
> On Tue, Apr 04 2023, Andrei Rybak wrote:
>> ---
>>   t/	 | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
>> index bca46378b2..8c442adb1a 100755
>> --- a/t/t1450-fsck.sh
>> +++ b/t/t1450-fsck.sh
>> @@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '
>>   
>>   		garbage_blob=$(git hash-object --stdin -w -t garbage --literally </dev/null) &&
>>   
>> -		cat >err.expect <<-\EOF &&
>> -		fatal: invalid object type
>> -		EOF
>> -		test_must_fail git fsck >out 2>err &&
>> +		test_must_fail git fsck 2>err &&
>>   		grep -e "^error" -e "^fatal" err >errors &&
>>   		test_line_count = 1 errors &&
>>   		grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err
> 
> ...but ditto my review on other patches, this just seems like a mistake
> of mine, i.e. if I add the "test_must_be_empty out" the test passes.
> 
> So isn't the answer here that my 31deb28f5e had an unintentional
> regression, and we should bring the assertion back? Its commit message
> says nothing about wanting to stop asserting stdout.
> 
> Maybe there was a reason I'm missing for why I remved it, it's since
> been paged out of my wetware, but looking at it briefly now it just
> seems like an unintentional bug / loss of test coverage that we should
> fix.

Tests in t1450-fsck.sh that do enforce empty standard output do it mostly via ">../actual 2>&1" and then a "test_must_be_empty actual".

For 'fsck error and recovery on invalid object type', the question is: is having this assertion useful for a developer using this test? The test is about invalid object types and what error messages "git fsck" prints about them. The test creates a fresh repository for it:

> 	git init --bare garbage-type &&

Is it useful to a developer working on this part of "git fsck" to have a "reminder" that no dangling objects should be found in such a fresh repository? Speaking of which, should there be such a test:

	test_expect_success 'fresh repository has no dangling objects' '
		git init fresh &&
		git -C fresh fsck >out
		test_must_be_empty out
	'

? Maybe even in t0001-init.sh?