Re: [PATCH 14/22] t/t9*: avoid redundant uses of cat

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2024, 00:43 UTC
Message-ID: <xmqqwmqgw5oq.fsf@gitster.g>
In-Reply-To: <8b9667e5-0d2f-4624-8f7c-f8400250a21e@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

Show 8 quoted lines

>>  	test_when_finished "git update-ref -d refs/heads/L2" &&
>>  	git fast-import <input &&
>>  	git ls-tree L2 g/b/ >tmp &&
>> -	cat tmp | cut -f 2 >actual &&
>> +	cut -f 2 <tmp >actual &&
>>  	test_cmp expect actual &&
>
> Nit: Maybe we can avoid tmp.

Piping "git ls-tree" output to "cut" would hide the exit status of "git ls-tree" if it fails, which is not a good idea, so I do not think of a way to avoid tmp so easily.

Show 11 quoted lines

>
>>  	git fsck $(git rev-parse L2)
>>  '
>> @@ -2012,7 +2012,7 @@ test_expect_success 'Q: verify first notes tree' '
>>  	100644 blob $commit2
>>  	100644 blob $commit3
>>  	EOF
>> -	cat expect.unsorted | sort >expect &&
>> +	sort expect.unsorted >expect &&
>
> Nit: I wonder if we can also avoid the cat that just precedes this hunk.

The whole thing reads like this:

test_expect_success 'Q: verify first notes tree' '
	cat >expect.unsorted <<-EOF &&
	100644 blob $commit1
	100644 blob $commit2
	100644 blob $commit3
	EOF
	cat expect.unsorted | sort >expect &&
	git cat-file -p refs/notes/foobar~2^{tree} | sed "s/ [0-9a-f]*	/ /" >actual &&
	test_cmp expect actual
'

As we are not in the business of debugging system-provided "sort", I agree that

	sort >expect <<-EOF &&
	100644 blob $commit1
	100644 blob $commit2
	100644 blob $commit3
	EOF

without having to use expect.unsorted would probably make sense. Well spotted.

This is outside the topic, but this test has different clean-up opportunities that are not related to the "do not run cat a single file and send its output into a pipe" pattern. The expected output we see here implicitly depends on the fact that the notes tree is so small that it hasn't been reorganized using fan-out levels. If the algorithm to decide when to start using fan-out directories changes, this test can break. To avoid that, we may need to do something like

	git ls-tree -r refs/notes/foobar~2 >ls-tree &&
	sed -e "s/ [0-9a-f]*               / /" -e "s|/||g" >actual &&

so that we will get the commit object name without the slashes that show the fan-out directories. Also, the current "actual" generation hides the exit status from "git cat-file".