Subject: Re: [PATCH v3 5/7] perf lint: check test-lint-shell-syntax in perf tests

From: Taylor Blau <[email-masked]>
Date: Mon, 19 Oct 2020 22:38:57 -0400

On Mon, Oct 19, 2020 at 10:47:35PM +0000, Nipunn Koorapati via GitGitGadget wrote:
>  test-lint-shell-syntax:
> -	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
> +	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)

I really appreciate your initiative to modify t/Makefile to start
linting t/perf/p????-*.sh files, too. Could I bother you to elaborate a
little bit on why you chose to modify a recipe in t/Makefile instead of
t/perf/Makefile?

I'm not necessarily opposed, but having this in t/perf/Makefile would
allow me to just run 'make' in 't/perf' and still have the scripts
linted there without having to involve a 'make' in 't'.

For what it's worth, I suspect that this is because 't/Makefile' already
has a 'test-lint-shell-syntax' target, and 't/perf/Makefile' does not. I
think it would be OK to add it there, too, and move this change into
t/perf.

> diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh
> index d202aaed06..7a0bb29448 100755
> --- a/t/perf/p3400-rebase.sh
> +++ b/t/perf/p3400-rebase.sh
> @@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '
>  	git checkout -f -B base &&
>  	git checkout -B to-rebase &&
>  	git checkout -B upstream &&
> -	for i in $(seq 100)
> +	for i in $(test_seq 100)
>  	do
>  		# simulate huge diffs
>  		echo change$i >unrelated-file$i &&
> -		seq 1000 >>unrelated-file$i &&
> +		test_seq 1000 >>unrelated-file$i &&
>  		git add unrelated-file$i &&
>  		test_tick &&
>  		git commit -m commit$i unrelated-file$i &&
>  		echo change$i >unrelated-file$i &&
> -		seq 1000 | tac >>unrelated-file$i &&
> +		test_seq 1000 | tac >>unrelated-file$i &&

Makes sense. I wouldn't be opposed to breaking this out into an earlier
change (e.g., "it's about to become not OK to use seq in t/perf, so
prepare for that by replacing any invocations with test_seq()"), but I
think it's probably not worth it, since this patch is small as it is.

Thanks,
Taylor