diff mbox series

[v3,03/20] t1092: clean up script quoting

Message ID d3cfd34b84184bef42fe0892790d80091c9ca01b.1615912983.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Sparse Index: Design, Format, Tests | expand

Commit Message

Derrick Stolee March 16, 2021, 4:42 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

This test was introduced in 19a0acc83e4 (t1092: test interesting
sparse-checkout scenarios, 2021-01-23), but these issues with quoting
were not noticed until starting this follow-up series. The old mechanism
would drop quoting such as in

   test_all_match git commit -m "touch README.md"

The above happened to work because README.md is a file in the
repository, so 'git commit -m touch REAMDE.md' would succeed by
accident.

Other cases included quoting for no good reason, so clean that up now.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Ævar Arnfjörð Bjarmason March 17, 2021, 8:47 a.m. UTC | #1
On Tue, Mar 16 2021, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> This test was introduced in 19a0acc83e4 (t1092: test interesting
> sparse-checkout scenarios, 2021-01-23), but these issues with quoting
> were not noticed until starting this follow-up series. The old mechanism
> would drop quoting such as in

the "but these issues" follows a partial sentence where we haven't
introduces "what issues?".

Perhaps leading with some summary about $@ v.s. $*:

    Fix a bug in the sparse checkout tests of "$@" being conflated with
    "$*". The bug was introduced in 19a0acc83e4 ([...]), but had no
    effect until now because XYZ ...


>    test_all_match git commit -m "touch README.md"
>
> The above happened to work because README.md is a file in the
> repository, so 'git commit -m touch REAMDE.md' would succeed by
> accident.
>
> Other cases included quoting for no good reason, so clean that up now.

Maybe just my taste, per your comment on another series of mine we might
not have the same sense of splitting up commits, but...

I think in this case it's clearer to have these be two commits. We have
3 hunks fixing the bug, and 6 on an unrelated cleanup. It's a lot easier
for eyeballing a fix to be able to glance just at the 3, especially with
something like $@ v.s. $*.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 8cd3e5a8d227..3725d3997e70 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -96,20 +96,20 @@ init_repos () {
>  run_on_sparse () {
>  	(
>  		cd sparse-checkout &&
> -		$* >../sparse-checkout-out 2>../sparse-checkout-err
> +		"$@" >../sparse-checkout-out 2>../sparse-checkout-err
>  	)
>  }
>  
>  run_on_all () {
>  	(
>  		cd full-checkout &&
> -		$* >../full-checkout-out 2>../full-checkout-err
> +		"$@" >../full-checkout-out 2>../full-checkout-err
>  	) &&
> -	run_on_sparse $*
> +	run_on_sparse "$@"
>  }
>  
>  test_all_match () {
> -	run_on_all $* &&
> +	run_on_all "$@" &&
>  	test_cmp full-checkout-out sparse-checkout-out &&
>  	test_cmp full-checkout-err sparse-checkout-err
>  }
> @@ -119,7 +119,7 @@ test_expect_success 'status with options' '
>  	test_all_match git status --porcelain=v2 &&
>  	test_all_match git status --porcelain=v2 -z -u &&
>  	test_all_match git status --porcelain=v2 -uno &&
> -	run_on_all "touch README.md" &&
> +	run_on_all touch README.md &&
>  	test_all_match git status --porcelain=v2 &&
>  	test_all_match git status --porcelain=v2 -z -u &&
>  	test_all_match git status --porcelain=v2 -uno &&
> @@ -135,7 +135,7 @@ test_expect_success 'add, commit, checkout' '
>  	write_script edit-contents <<-\EOF &&
>  	echo text >>$1
>  	EOF
> -	run_on_all "../edit-contents README.md" &&
> +	run_on_all ../edit-contents README.md &&
>  
>  	test_all_match git add README.md &&
>  	test_all_match git status --porcelain=v2 &&
> @@ -144,7 +144,7 @@ test_expect_success 'add, commit, checkout' '
>  	test_all_match git checkout HEAD~1 &&
>  	test_all_match git checkout - &&
>  
> -	run_on_all "../edit-contents README.md" &&
> +	run_on_all ../edit-contents README.md &&
>  
>  	test_all_match git add -A &&
>  	test_all_match git status --porcelain=v2 &&
> @@ -153,7 +153,7 @@ test_expect_success 'add, commit, checkout' '
>  	test_all_match git checkout HEAD~1 &&
>  	test_all_match git checkout - &&
>  
> -	run_on_all "../edit-contents deep/newfile" &&
> +	run_on_all ../edit-contents deep/newfile &&
>  
>  	test_all_match git status --porcelain=v2 -uno &&
>  	test_all_match git status --porcelain=v2 &&
> @@ -186,7 +186,7 @@ test_expect_success 'diff --staged' '
>  	write_script edit-contents <<-\EOF &&
>  	echo text >>README.md
>  	EOF
> -	run_on_all "../edit-contents" &&
> +	run_on_all ../edit-contents &&
>  
>  	test_all_match git diff &&
>  	test_all_match git diff --staged &&
> @@ -280,7 +280,7 @@ test_expect_success 'clean' '
>  	echo bogus >>.gitignore &&
>  	run_on_all cp ../.gitignore . &&
>  	test_all_match git add .gitignore &&
> -	test_all_match git commit -m ignore-bogus-files &&
> +	test_all_match git commit -m "ignore bogus files" &&
>  
>  	run_on_sparse mkdir folder1 &&
>  	run_on_all touch folder1/bogus &&
diff mbox series

Patch

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 8cd3e5a8d227..3725d3997e70 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -96,20 +96,20 @@  init_repos () {
 run_on_sparse () {
 	(
 		cd sparse-checkout &&
-		$* >../sparse-checkout-out 2>../sparse-checkout-err
+		"$@" >../sparse-checkout-out 2>../sparse-checkout-err
 	)
 }
 
 run_on_all () {
 	(
 		cd full-checkout &&
-		$* >../full-checkout-out 2>../full-checkout-err
+		"$@" >../full-checkout-out 2>../full-checkout-err
 	) &&
-	run_on_sparse $*
+	run_on_sparse "$@"
 }
 
 test_all_match () {
-	run_on_all $* &&
+	run_on_all "$@" &&
 	test_cmp full-checkout-out sparse-checkout-out &&
 	test_cmp full-checkout-err sparse-checkout-err
 }
@@ -119,7 +119,7 @@  test_expect_success 'status with options' '
 	test_all_match git status --porcelain=v2 &&
 	test_all_match git status --porcelain=v2 -z -u &&
 	test_all_match git status --porcelain=v2 -uno &&
-	run_on_all "touch README.md" &&
+	run_on_all touch README.md &&
 	test_all_match git status --porcelain=v2 &&
 	test_all_match git status --porcelain=v2 -z -u &&
 	test_all_match git status --porcelain=v2 -uno &&
@@ -135,7 +135,7 @@  test_expect_success 'add, commit, checkout' '
 	write_script edit-contents <<-\EOF &&
 	echo text >>$1
 	EOF
-	run_on_all "../edit-contents README.md" &&
+	run_on_all ../edit-contents README.md &&
 
 	test_all_match git add README.md &&
 	test_all_match git status --porcelain=v2 &&
@@ -144,7 +144,7 @@  test_expect_success 'add, commit, checkout' '
 	test_all_match git checkout HEAD~1 &&
 	test_all_match git checkout - &&
 
-	run_on_all "../edit-contents README.md" &&
+	run_on_all ../edit-contents README.md &&
 
 	test_all_match git add -A &&
 	test_all_match git status --porcelain=v2 &&
@@ -153,7 +153,7 @@  test_expect_success 'add, commit, checkout' '
 	test_all_match git checkout HEAD~1 &&
 	test_all_match git checkout - &&
 
-	run_on_all "../edit-contents deep/newfile" &&
+	run_on_all ../edit-contents deep/newfile &&
 
 	test_all_match git status --porcelain=v2 -uno &&
 	test_all_match git status --porcelain=v2 &&
@@ -186,7 +186,7 @@  test_expect_success 'diff --staged' '
 	write_script edit-contents <<-\EOF &&
 	echo text >>README.md
 	EOF
-	run_on_all "../edit-contents" &&
+	run_on_all ../edit-contents &&
 
 	test_all_match git diff &&
 	test_all_match git diff --staged &&
@@ -280,7 +280,7 @@  test_expect_success 'clean' '
 	echo bogus >>.gitignore &&
 	run_on_all cp ../.gitignore . &&
 	test_all_match git add .gitignore &&
-	test_all_match git commit -m ignore-bogus-files &&
+	test_all_match git commit -m "ignore bogus files" &&
 
 	run_on_sparse mkdir folder1 &&
 	run_on_all touch folder1/bogus &&