Re: [PATCH v2 01/14] t/t5505-remote: modernize style
Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 13 quoted lines
> 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.
Show 11 quoted lines
> 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
Show 10 quoted lines
> @@ -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(-)
Show changes to t/t5505-remote.sh +11 −11
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
Ramkumar Ramachandra