Re: [PATCH v2 01/14] t/t5505-remote: modernize style

[Junio C Hamano <gitster@pobox.com>]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Modernize the style of all tests throughout the file:
> - Remove spurious blank lines.
> - Indent the test body.
> - Make sure that all lines end with &&, to make it easier to spot breaks
>   in the chain.
> - When executing something in a subshell, put the parenthesis on
>   separate lines and indent the body.  Also make sure that the first
>   statement in the subshell is a 'cd'.
> - When redirecting output, replace the > output forms with >output.
> - Use the <<-\EOF and <<-EOF forms of heredoc, not <<EOF.  Also, don't
>   de-indent the heredoc body.
> - When creating an empty file, use : >output form over >output for
>   clarity.

Everything except the last one is good, I think.

For the last one, I prefer ">output" moderately over ": >output", as
both are equally clear to people who write shell scripts, but this
preference is not strong enough to make me change ": >output" that
is originally in the file to ">output" in a patch.  In the very old
days, there were some implementations of shells that mishandled
">output" but those days are long gone.

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5505-remote.sh | 813 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 440 insertions(+), 373 deletions(-)
>
> -cat > test/expect << EOF
> ...
> +cat >test/expect <<-EOF
>  * remote origin
>    Fetch URL: $(pwd)/one
>    Push  URL: $(pwd)/one

This one is questionable; if it is not indented with HT, there is no
point using the -EOF form.

> -cat > test/expect << EOF
> ...
> +cat >test/expect <<-EOF

Likewise.

> @@ -219,152 +220,187 @@ cat > test/expect << EOF
>  EOF
>  
>  test_expect_success 'show -n' '
> -	(mv one one.unreachable &&
> -	 cd test &&
> ...
> +	mv one one.unreachable &&
> +	(
> +		cd test &&

This is more than an indentation change, but I think it is a good
one.  I saw some others that moved "mkdir" out of the subshell to
chdir into that directory, which are also good changes.

>  cat >test/expect <<\EOF

Funny that you did not touch this one.

> -cat > one/expect << EOF
> +cat >one/expect <<-\EOF
>    apis/master

But this again is an unnecessary use of <<-

This smells like a largely blind conversion done with a script,
which needs a human proof-reader other than the submitter;
everything else looked good.


I'll queue with this fix-up squashed in.


 t/t5505-remote.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index f679ded..0e7dfa2 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -107,7 +107,7 @@ test_expect_success C_LOCALE_OUTPUT 'remove remote' '
 		check_remote_track origin master side &&
 		git for-each-ref "--format=%(refname)" refs/remotes |
 		sed -e "/^refs\/remotes\/origin\//d" >actual &&
-		: >expect &&
+		>expect &&
 		test_cmp expect actual
 	)
 '
@@ -139,7 +139,7 @@ test_expect_success 'remove remote protects local branches' '
 	)
 '
 
-cat >test/expect <<-EOF
+cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one
   Push  URL: $(pwd)/one
@@ -202,7 +202,7 @@ test_expect_success 'show' '
 	)
 '
 
-cat >test/expect <<-EOF
+cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one
   Push  URL: $(pwd)/one
@@ -262,7 +262,7 @@ test_expect_success 'set-head --auto' '
 	)
 '
 
-cat >test/expect <<-\EOF
+cat >test/expect <<\EOF
 error: Multiple remote HEAD branches. Please choose one explicitly with:
   git remote set-head two another
   git remote set-head two master
@@ -276,7 +276,7 @@ test_expect_success 'set-head --auto fails w/multiple HEADs' '
 	)
 '
 
-cat >test/expect <<-\EOF
+cat >test/expect <<\EOF
 refs/remotes/origin/side2
 EOF
 
@@ -290,7 +290,7 @@ test_expect_success 'set-head explicit' '
 	)
 '
 
-cat >test/expect <<-EOF
+cat >test/expect <<EOF
 Pruning origin
 URL: $(pwd)/one
  * [would prune] origin/side2
@@ -483,7 +483,7 @@ EOF
 test_expect_success 'add with reachable tags (default)' '
 	(
 		cd one &&
-		: >foobar &&
+		>foobar &&
 		git add foobar &&
 		git commit -m "Foobar" &&
 		git tag -a -m "Foobar tag" foobar-tag &&
@@ -551,7 +551,7 @@ test_expect_success 'reject --no-no-tags' '
 	)
 '
 
-cat >one/expect <<-\EOF
+cat >one/expect <<\EOF
   apis/master
   apis/side
   drosophila/another
@@ -570,7 +570,7 @@ test_expect_success 'update' '
 	)
 '
 
-cat >one/expect <<-\EOF
+cat >one/expect <<\EOF
   drosophila/another
   drosophila/master
   drosophila/side
@@ -637,7 +637,7 @@ test_expect_success 'update default' '
 	)
 '
 
-cat >one/expect <<-\EOF
+cat >one/expect <<\EOF
   drosophila/another
   drosophila/master
   drosophila/side
@@ -732,7 +732,7 @@ test_expect_success 'rename a remote with name prefix of other remote' '
 	)
 '
 
-cat >remotes_origin <<-EOF
+cat >remotes_origin <<EOF
 URL: $(pwd)/one
 Push: refs/heads/master:refs/heads/upstream
 Pull: refs/heads/master:refs/heads/origin