Re: [PATCH v1 1/2] t/*: fix pipe placement and remove \'s

[Jonathan Nieder <jrnieder@gmail.com>]

Matthew DeVore wrote:

> Subject: t/*: fix pipe placement and remove \'s
>
> Where ever there was code in the tests like this:
>
> 	foo \
> 		| bar

Language nits:
- s/Where ever/Wherever/
- Git's commit messages use the present tense to describe the existing
  previous state of the codebase, as though reporting a bug.

Maybe something like

	tests: standardize pipe placement

	Instead of using a line-continuation and pipe on the second
	line, take advantage of the shell's implicit line continuation
	after a pipe character.  So for example, instead of

		some long line \
			| next line

	use

		some long line |
		next line

At this point, it would be useful to say something about rationale ---
for example,

	This better matches the coding style documented in
	Documentation/CodingGuidelines and used in shell scripts
	elsewhere in Git.

Except: is this documented in Documentation/CodingGuidelines?  Or,
better, is there a linter that we can run in the test-lint target of
t/Makefile to ensure we keep sticking to this style?

[...]
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -57,8 +57,8 @@ then
>  		echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
>  			--passphrase-fd 0 --pinentry-mode loopback \
>  			--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
> -		gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K \
> -			| grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
> +		gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
> +		grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
>  			${GNUPGHOME}/trustlist.txt &&

I think this would be more readable with one item from the pipeline
per line:

		gpgsm --homedir ... |
		grep ... |
		cut ... |
		tr ... >... &&

[...]
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -218,8 +218,8 @@ test_expect_success "--batch-check for a non-existent hash" '
>      test "0000000000000000000000000000000000000042 missing
>  0000000000000000000000000000000000000084 missing" = \
>      "$( ( echo 0000000000000000000000000000000000000042;
> -         echo_without_newline 0000000000000000000000000000000000000084; ) \
> -       | git cat-file --batch-check)"
> +         echo_without_newline 0000000000000000000000000000000000000084; ) |
> +       git cat-file --batch-check)"

This test is problematic in a lot of ways.  Most importantly, it ignores
the exist status from git cat-file.

So it should say something like:

	cat >expect <<-\EOF &&
		foobar42 missing
		foobar84 missing
	EOF
	printf "foobar42\nfoobar84" |
	git cat-file --batch-check >actual &&
	test_cmp expect actual

If we want to restrict to the pipeline style fixes, we could do

	test "..." = "$(
		{	# Couldn't resist changing the ( to {!
			echo ... &&	# Couldn't resist changing the ; to &&!
			echo_without_newline ...
		} |
		git cat-file --batch-check
	)"

but unless there's a linter that we're helping support, it's probably
better to skip this file and use a dedicated patch to modernize its
style more generally.

[...]
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1770,8 +1770,9 @@ test_expect_success '--show-origin stdin with file include' '
>  	cat >expect <<-EOF &&
>  		file:$INCLUDE_DIR/stdin.include	include
>  	EOF
> -	echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" \
> -		| git config --show-origin --includes --file - user.stdin >output &&
> +	echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" |
> +	git config --show-origin --includes --file - user.stdin >output &&

Okay.

[...]
> --- a/t/t5317-pack-objects-filter-objects.sh
> +++ b/t/t5317-pack-objects-filter-objects.sh
> @@ -20,17 +20,20 @@ test_expect_success 'setup r1' '
>  '
>  
>  test_expect_success 'verify blob count in normal packfile' '
> -	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
> -		| awk -f print_2.awk \
> -		| sort >expected &&
> +	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
> +	awk -f print_2.awk |
> +	sort >expected &&

This loses the exit status from git, so we should make it write to a
temporary file instead (as a separate patch).

[...]
> -	git -C r1 verify-pack -v ../all.pack \
> -		| grep blob \
> -		| awk -f print_1.awk \
> -		| sort >observed &&
> +
> +	git -C r1 verify-pack -v ../all.pack |

Likewise (and likewise for the rest in this file).

[...]
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -50,8 +50,9 @@ pull_to_client () {
>  			case "$heads" in *B*)
>  			    git update-ref refs/heads/B "$BTIP";;
>  			esac &&
> -			git symbolic-ref HEAD refs/heads/$(echo $heads \
> -				| sed -e "s/^\(.\).*$/\1/") &&
> +
> +			git symbolic-ref HEAD refs/heads/$(echo $heads |
> +			sed -e "s/^\(.\).*$/\1/") &&

It would be better to use a temporary variable.  If we're just
changing line wrapping, then this would be

			git symbolic-ref HAD refs/heads/$(
				echo $heads |
				sed ...
			) &&

[...]
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -34,10 +34,12 @@ test_expect_success 'setup bare clone for server' '
>  # confirm partial clone was registered in the local config.
>  test_expect_success 'do partial clone 1' '
>  	git clone --no-checkout --filter=blob:none "file://$(pwd)/srv.bare" pc1 &&
> -	git -C pc1 rev-list HEAD --quiet --objects --missing=print \
> -		| awk -f print_1.awk \
> -		| sed "s/?//" \
> -		| sort >observed.oids &&
> +
> +	git -C pc1 rev-list HEAD --quiet --objects --missing=print |

Also needs to write to a temporary to avoid losing the exist status
(and likewise for the rest of this file).

[...]
> --- a/t/t6112-rev-list-filters-objects.sh
> +++ b/t/t6112-rev-list-filters-objects.sh
> @@ -20,24 +20,28 @@ test_expect_success 'setup r1' '
>  '
>  
>  test_expect_success 'verify blob:none omits all 5 blobs' '
> -	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
> -		| awk -f print_2.awk \
> -		| sort >expected &&
> -	git -C r1 rev-list HEAD --quiet --objects --filter-print-omitted --filter=blob:none \
> -		| awk -f print_1.awk \
> -		| sed "s/~//" \
> -		| sort >observed &&
> +	git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
> +	awk -f print_2.awk |
> +	sort >expected &&

Likewise.

[...]
> --- a/t/t9101-git-svn-props.sh
> +++ b/t/t9101-git-svn-props.sh
> @@ -193,8 +193,8 @@ test_expect_success 'test propget' "
>  	git svn propget svn:ignore . | cmp - prop.expect &&
>  	cd deeply &&
>  	git svn propget svn:ignore . | cmp - ../prop.expect &&
> -	git svn propget svn:entry:committed-rev nested/directory/.keep \
> -	  | cmp - ../prop2.expect &&
> +	git svn propget svn:entry:committed-rev nested/directory/.keep |
> +	cmp - ../prop2.expect &&

Likewise.

Thanks and hope that helps,
Jonathan