git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 23, 2013, 07:42 UTC
Message-ID
<7vehbtgto1.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1371887901-5659-2-git-send-email-artagnon@gmail.com>
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
Likewise.
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(-)
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
Previous: Ramkumar RamachandraNext: Ramkumar Ramachandra
Message 3 of 25 in “Classify {branches,remotes}-file as fringe features”
  1. 00/14 Classify {branches,remotes}-file as fringe featuresRamkumar Ramachandra, Jun 22, 2013
  2. 01/14 t/t5505-remote: modernize styleRamkumar Ramachandra, Jun 22, 2013
  3. Junio C HamanoJun 23, 2013
  4. Ramkumar RamachandraJun 23, 2013
  5. 02/14 t/t5505-remote: test push-refspec in branches-fileRamkumar Ramachandra, Jun 22, 2013
  6. 03/14 t/t5505-remote: use test_path_is_missingRamkumar Ramachandra, Jun 22, 2013
  7. 04/14 t/t5505-remote: remove dependency on $origin_urlRamkumar Ramachandra, Jun 22, 2013
  8. 05/14 remote: remove dead code in read_branches_file()Ramkumar Ramachandra, Jun 22, 2013
  9. Junio C HamanoJun 23, 2013
  10. 06/14 t/t5505-remote: test url-with-# in branches-fileRamkumar Ramachandra, Jun 22, 2013
  11. 07/14 t/t5516-fetch-push: don't use branches-fileRamkumar Ramachandra, Jun 22, 2013
  12. 08/14 ls-remote doc: fix example invocation on git.gitRamkumar Ramachandra, Jun 22, 2013
  13. Junio C HamanoJun 23, 2013
  14. Ramkumar RamachandraJun 23, 2013
  15. Junio C HamanoJun 23, 2013
  16. 09/14 ls-remote doc: rewrite <repository> paragraphRamkumar Ramachandra, Jun 22, 2013
  17. 10/14 ls-remote doc: don't encourage use of branches-fileRamkumar Ramachandra, Jun 22, 2013
  18. 11/14 t/t5505-remote: test multiple push/pull in remotes-fileRamkumar Ramachandra, Jun 22, 2013
  19. Junio C HamanoJun 23, 2013
  20. Ramkumar RamachandraJun 23, 2013
  21. Junio C HamanoJun 23, 2013
  22. 12/14 t/t5510-fetch: don't use remotes-fileRamkumar Ramachandra, Jun 22, 2013
  23. 13/14 t/t5515-fetch-merge-logic: don't use {branches,remotes}-fileRamkumar Ramachandra, Jun 22, 2013
  24. 14/14 remote: add comment about read_{branches,remotes}_fileRamkumar Ramachandra, Jun 22, 2013
  25. Ramkumar RamachandraJun 25, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.