Hi Ævar,

On Tue, 31 May 2022, Ævar Arnfjörð Bjarmason wrote:

> Add a test for the regression introduced in my 9c4d58ff2c3 (ls-tree:
> split up "fast path" callbacks, 2022-03-23) and fixed in
> 350296cc789 (ls-tree: `-l` should not imply recursive listing,
> 2022-04-04), and test for the test of ls-tree option/mode combinations
> to make sure we don't have other blind spots.

That's adding a lot of new test cases, which is not free of cost.

> Range-diff:
> [... 433 lines, essentially a rewrite...]

Pasting 443 lines of range-diff also is not free of cost.

> diff --git a/t/t3105-ls-tree-output.sh b/t/t3105-ls-tree-output.sh
> new file mode 100755
> index 00000000000..29511d9331b
> --- /dev/null
> +++ b/t/t3105-ls-tree-output.sh
> @@ -0,0 +1,192 @@
> +#!/bin/sh
> +
> +test_description='ls-tree output'
> +
> [...]
> +'
> +
> +test_ls_tree_format_mode_output () {
> +	local opts=$1 &&

This line does not quote `$1`, but...

> +	shift &&
> +	cat >expect &&
> +
> +	while test $# -gt 0
> +	do
> +		local mode="$1" &&
> +		shift &&
> +
> +		test_expect_success "'ls-tree $opts${mode:+ $mode}' output" '
> +			git ls-tree ${mode:+$mode }$opts HEAD >actual &&
> +			test_cmp expect actual
> +		'
> +
> +		case "$opts" in
> +		--full-tree)
> +			test_expect_success "'ls-tree $opts${mode:+ $mode}' output (via subdir, fails)" '
> +				test_must_fail git -C dir ls-tree --full-name ${mode:+$mode }$opts HEAD -- ../
> +			'
> +			;;
> +		*)
> +			test_expect_success "'ls-tree $opts${mode:+ $mode}' output (via subdir)" '
> +				git -C dir ls-tree --full-name ${mode:+$mode }$opts HEAD -- ../ >actual &&
> +				test_cmp expect actual
> +			'
> +			;;
> +		esac
> +	done
> +}
> +
> [...]
> +## opt = --object-only --abbrev
> +test_expect_success 'setup: HEAD_short_* variables' '
> +	HEAD_short_gitmodules=$(git rev-parse --short HEAD:.gitmodules) &&
> +	HEAD_short_dir=$(git rev-parse --short HEAD:dir) &&
> +	HEAD_short_top_file=$(git rev-parse --short HEAD:top-file.t) &&
> +	HEAD_short_submodule=$(git rev-parse --short HEAD:submodule) &&
> +	HEAD_short_dir_sub_file=$(git rev-parse --short HEAD:dir/sub-file.t)
> +'
> +test_ls_tree_format_mode_output "--object-only --abbrev" "" "-t" <<-EOF

... this line passes a first argument that contains spaces. Hence the
tests fail in CI:
https://github.com/git/git/runs/6703333447?check_suite_focus=true

Further, since this failure is outside of any `test_expect_success` or
`test_expect_failure`, the error message about this is not even included
in the weblogs (but of course it is included in the full logs that are
included in the build artifacts). For the record, here is the error
message:

	[...]

	ok 35 - 'ls-tree --object-only -r' output (via subdir)
	+ git rev-parse --short HEAD:.gitmodules
	+ HEAD_short_gitmodules=6da7993
	+ git rev-parse --short HEAD:dir
	+ HEAD_short_dir=aba57e0
	+ git rev-parse --short HEAD:top-file.t
	+ HEAD_short_top_file=02dad95
	+ git rev-parse --short HEAD:submodule
	+ HEAD_short_submodule=61a63ac
	+ git rev-parse --short HEAD:dir/sub-file.t
	+ HEAD_short_dir_sub_file=a150abd

	ok 36 - setup: HEAD_short_* variables
	t3105-ls-tree-output.sh: 20: local: --abbrev: bad variable name
	FATAL: Unexpected exit with code 2

Note: I am only pointing out why the CI/PR run fails, I have not formed
any opinion on the actual code other than noticing a lot of what might be
repetitive lines and suspecting that adding this many new test cases
increases the runtime of the test suite and thereby adds to developer
friction and the benefit (namely, to catch future regressions) might not
justify that. But again, I have not fully formed an opinion about this
patch.

Ciao,
Johannes