From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 30 Jan 2026 16:27:14 GMT
Subject: Re: [PATCH v2] t/perf/p3400: speed up setup using fast-import
Message-ID: <xmqqcy2r83rx.fsf@gitster.g>
In-Reply-To: <04005f67-3ec5-4fa7-9fd7-37ef9034286a@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

> Am 28.01.26 um 17:07 schrieb Tian Yuchen:
>> +	test_seq 1000 >content_fwd &&
>> +	test_seq 1000 | sort -nr >content_rev &&
>
> Remove another pipe with
>
> 	sort -nr content_fwd >content_rev &&

I agree with the outcome (i.e., we do not run test_seq twice) but
not necessarily with the motivation (i.e., we do not assume test_seq
may fail and care about its failure code, so no need to avoid pipes
for the sake of avoiding pipes).

>> +	(
>> +		for i in $(test_seq 100)
>> +		do
>> +			...
>> +			cat content_rev &&
>> +			echo "EOF" || return 1
>
> This is an incorrect use of `return` outside a function. This must be
> `|| exit 1` (or just `|| exit`) to actually break out of the loop.

Good.

> Of course, the error code is ignored, because it is in the upstream of a
> pipe, so in order to fail the complete command, it would be necessary
> that the original error left an incorrect fast-import stream such that
> the down-stream `git fast-import` fails. Otherwise, the entire command
> can complete successfully in an unexpected way.
>
> Maybe instead of a pipe, dump the stream into a temporary file, so that
> the loop's exit code can be captured?

I like this suggestion.  A temporary file, unless it is excessively
large, has an additional advantage that it allows you to inspect
after "git fast-import" dies on it.

>> +		done
>> +	) | git fast-import &&
> -- Hannes