diff mbox series

[v4,3/6] tests: don't lose exit status with "(cd ...; test <op> $(git ...))"

Message ID patch-v4-3.6-f5b2489609c-20221219T101240Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series tests: fix ignored & hidden exit codes | expand

Commit Message

Ævar Arnfjörð Bjarmason Dec. 19, 2022, 10:19 a.m. UTC
Rewrite tests that ran "git" inside command substitution and lost the
exit status of "git" so that we notice the failing "git".

Have them use modern patterns such as a "test_cmp" of the expected
outputs instead, and avoid needlessly spawning sub-shell in favor of
using "git -C <dir>".

We'll fix more of these these in the subsequent commit, for now we're
only converting the cases where this loss of exit code was combined
with spawning a sub-shell. The one exception to that is the casein
"t3200-branch.sh" where adjacent code didn't use a sub-shell, let's
convert that here as it's within the same hunk.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-httpd.sh              |  5 +++--
 t/lib-submodule-update.sh   | 22 +++++++++-------------
 t/t0060-path-utils.sh       |  4 +++-
 t/t3200-branch.sh           | 13 +++++++------
 t/t5605-clone-local.sh      | 15 ++++++++++-----
 t/t7402-submodule-rebase.sh | 14 +++++++++++---
 6 files changed, 43 insertions(+), 30 deletions(-)

Comments

Junio C Hamano Dec. 20, 2022, 12:20 a.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -	test "a" = "$(cd repo && test-tool path-utils prefix_path prefix "$(pwd)/../repolink/a")"
> +	echo "a" >expect &&
> +	test-tool -C repo path-utils prefix_path prefix "$(cd repo && pwd)/../repolink/a" >actual &&

If we fail to cd to 'repo', "$(cd repo && pwd)/../repolink/a" would
silently expand to nonsense, but presumably "test-tool -C repo"
would fail loudly in such a case, so we should be OK here?

> @@ -28,9 +32,10 @@ test_expect_success 'preparing origin repository' '
>  
>  test_expect_success 'local clone without .git suffix' '
>  	git clone -l -s a b &&
> -	(cd b &&
> -	test "$(git config --bool core.bare)" = false &&
> -	git fetch)
> +	echo false >expect &&
> +	git -C b config --bool core.bare >actual &&
> +	test_cmp expect actual &&
> +	git -C b fetch
>  '

I am not sure if the above with full of "git -C" is strictly an
improvement over

	(
		cd b &&
		echo false >expect &&
		git config --bool core.bare >actual &&
		test_cmp expect actual &&
		git fetch
	)

and even if it were, the reason why it is better would be vastly
different from the reason why it is better that we no longer do
"test $(cmd) = false".  I very much hate the pattern described on
the commit title of this step (which by definition this patch fixes
many instances of).  I.e.

	(cd ...; test <op> $(git ...))

might be something you find that needs improvements, but "don't lose
exit status" ONLY applies to the "test <op> $(git ...)" part, and
the other half, i.e. (cd ...), has nothing to do with "don't lose
exit status" badness.
diff mbox series

Patch

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 608949ea80b..31e7fa3010c 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -217,8 +217,9 @@  test_http_push_nonff () {
 		git commit -a -m path2 --amend &&
 
 		test_must_fail git push -v origin >output 2>&1 &&
-		(cd "$REMOTE_REPO" &&
-		 test $HEAD = $(git rev-parse --verify HEAD))
+		echo "$HEAD" >expect &&
+		git -C "$REMOTE_REPO" rev-parse --verify HEAD >actual &&
+		test_cmp expect actual
 	'
 
 	test_expect_success 'non-fast-forward push show ref status' '
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 2d31fcfda1f..d7c2b670b4a 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -168,20 +168,16 @@  replace_gitfile_with_git_dir () {
 # Note that this only supports submodules at the root level of the
 # superproject, with the default name, i.e. same as its path.
 test_git_directory_is_unchanged () {
-	(
-		cd ".git/modules/$1" &&
-		# does core.worktree point at the right place?
-		test "$(git config core.worktree)" = "../../../$1" &&
-		# remove it temporarily before comparing, as
-		# "$1/.git/config" lacks it...
-		git config --unset core.worktree
-	) &&
+	# does core.worktree point at the right place?
+	echo "../../../$1" >expect &&
+	git -C ".git/modules/$1" config core.worktree >actual &&
+	test_cmp expect actual &&
+	# remove it temporarily before comparing, as
+	# "$1/.git/config" lacks it...
+	git -C ".git/modules/$1" config --unset core.worktree &&
 	diff -r ".git/modules/$1" "$1/.git" &&
-	(
-		# ... and then restore.
-		cd ".git/modules/$1" &&
-		git config core.worktree "../../../$1"
-	)
+	# ... and then restore.
+	git -C ".git/modules/$1" config core.worktree "../../../$1"
 }
 
 test_git_directory_exists () {
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 68e29c904a6..53ec717cbca 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -255,7 +255,9 @@  test_expect_success 'prefix_path rejects absolute path to dir with same beginnin
 test_expect_success SYMLINKS 'prefix_path works with absolute path to a symlink to work tree having  same beginning as work tree' '
 	git init repo &&
 	ln -s repo repolink &&
-	test "a" = "$(cd repo && test-tool path-utils prefix_path prefix "$(pwd)/../repolink/a")"
+	echo "a" >expect &&
+	test-tool -C repo path-utils prefix_path prefix "$(cd repo && pwd)/../repolink/a" >actual &&
+	test_cmp expect actual
 '
 
 relative_path /foo/a/b/c/	/foo/a/b/	c/
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5a169b68d6a..f5fbb84262b 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -242,12 +242,13 @@  test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
 test_expect_success 'git branch -M baz bam should succeed within a worktree in which baz is checked out' '
 	git checkout -b baz &&
 	git worktree add -f bazdir baz &&
-	(
-		cd bazdir &&
-		git branch -M baz bam &&
-		test $(git rev-parse --abbrev-ref HEAD) = bam
-	) &&
-	test $(git rev-parse --abbrev-ref HEAD) = bam &&
+	git -C "$bazdir" branch -M baz bam &&
+	echo "bam" >expect &&
+	git -C "$bazdir" rev-parse --abbrev-ref HEAD >actual &&
+	test_cmp expect actual &&
+	echo "bam" >expect &&
+	git rev-parse --abbrev-ref HEAD >actual &&
+	test_cmp expect actual &&
 	rm -r bazdir &&
 	git worktree prune
 '
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index 38b850c10ef..61a2342bc2c 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -15,8 +15,12 @@  test_expect_success 'preparing origin repository' '
 	: >file && git add . && git commit -m1 &&
 	git clone --bare . a.git &&
 	git clone --bare . x &&
-	test "$(cd a.git && git config --bool core.bare)" = true &&
-	test "$(cd x && git config --bool core.bare)" = true &&
+	echo true >expect &&
+	git -C a.git config --bool core.bare >actual &&
+	test_cmp expect actual &&
+	echo true >expect &&
+	git -C x config --bool core.bare >actual &&
+	test_cmp expect actual &&
 	git bundle create b1.bundle --all &&
 	git bundle create b2.bundle main &&
 	mkdir dir &&
@@ -28,9 +32,10 @@  test_expect_success 'preparing origin repository' '
 
 test_expect_success 'local clone without .git suffix' '
 	git clone -l -s a b &&
-	(cd b &&
-	test "$(git config --bool core.bare)" = false &&
-	git fetch)
+	echo false >expect &&
+	git -C b config --bool core.bare >actual &&
+	test_cmp expect actual &&
+	git -C b fetch
 '
 
 test_expect_success 'local clone with .git suffix' '
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index ebeca12a711..1927a862839 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -82,11 +82,19 @@  test_expect_success 'stash with a dirty submodule' '
 	CURRENT=$(cd submodule && git rev-parse HEAD) &&
 	git stash &&
 	test new != $(cat file) &&
-	test submodule = $(git diff --name-only) &&
-	test $CURRENT = $(cd submodule && git rev-parse HEAD) &&
+	echo submodule >expect &&
+	git diff --name-only >actual &&
+	test_cmp expect actual &&
+
+	echo "$CURRENT" >expect &&
+	git -C submodule rev-parse HEAD >actual &&
+	test_cmp expect actual &&
+
 	git stash apply &&
 	test new = $(cat file) &&
-	test $CURRENT = $(cd submodule && git rev-parse HEAD)
+	echo "$CURRENT" >expect &&
+	git -C submodule rev-parse HEAD >actual &&
+	test_cmp expect actual
 
 '