Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 23, 2020, 01:14 UTC
Message-ID: <20200423011444.GG140314@google.com>
In-Reply-To: <5c9217ad8fc594fbff46507c4be7961eb5a478e2.1587601501.git.me@ttaylorr.com>

Hi,

Taylor Blau wrote:

Show 13 quoted lines

> A number of spots in t5537 use the non-indented heredoc '<<EOF' when
> they would benefit from instead using '<<-EOF' or simply
> test_write_lines.
>
> In preparation for adding new tests in a good style and being consistent
> with the surrounding code, update the existing tests to improve their
> readability.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  t/t5537-fetch-shallow.sh | 70 +++++++++++-----------------------------
>  1 file changed, 18 insertions(+), 52 deletions(-)

Sounds like a good idea. Some nitpicks --- please don't act on them all, but only the ones that seem appropriate to you:

[...]

Show 12 quoted lines

> +++ b/t/t5537-fetch-shallow.sh
> @@ -25,10 +25,7 @@ test_expect_success 'setup' '
>  test_expect_success 'setup shallow clone' '
>  	git clone --no-local --depth=2 .git shallow &&
>  	git --git-dir=shallow/.git log --format=%s >actual &&
> -	cat <<EOF >expect &&
> -4
> -3
> -EOF
> +	test_write_lines 4 3 >expect &&
>  	test_cmp expect actual
>  '

Nice.

[...]

> @@ -133,14 +110,12 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '

[...]

Show 6 quoted lines

> -	cat <<EOF >expect &&
> -no-shallow
> -EOF
> +	cat <<-EOF >expect &&
> +	no-shallow
> +	EOF

Can this use "echo"? Or if using cat, please quote the EOF in <<-EOF so the reader doesn't have to check for $substitutions in the body:

		cat >expect <<-\EOF &&

[...]

Show 11 quoted lines

> @@ -158,21 +133,15 @@ test_expect_success 'fetch --update-shallow' '
>  	git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
>  	git fsck &&
>  	git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
> -	cat <<EOF >expect.refs &&
> -refs/remotes/shallow/master
> -refs/remotes/shallow/no-shallow
> -refs/tags/heavy-tag
> -refs/tags/light-tag
> -EOF
> +	cat <<-EOF >expect.refs &&

Likewise (missing \ before EOF).

A few more nits, that probably don't belong in the same patch:

- the code in subshells would be more readable if indented
- existing <<-EOF here blocks should \quote the EOF
- the resulting history would be more realistic if it uses test_tick
  before running "git commit".  Or perhaps this can use the
  test_commit helper to handle that
- should use test_must_fail in preference to ! git
- might be simpler if http tests go in a different file

Thanks, Jonathan