threads / patch / 59714

patch, 3 partsdropping "verbose" test helper function

Subject: [PATCH 0/3] dropping "verbose" test helper function

## tl;dr

6 messages between May 8, 2023 and May 8, 2023. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Jeff King· May 8, 2023, 18:59 UTC · lore

I happened across an instance of the "verbose" helper function in a test recently. I think it's clear we prefer the "-x" tracing to it these days, and we've been slowly removing instances. This series gets rid of the last few.

The actual change (and rationale) is in patch 3. Patch 1 just fixes pipe problems on lines we'll be touching patch 3, and patch 2 is a cute optimization enabled by patch 1. I don't think any of it should be very controversial, but it can all be split up if need be.

  [1/3]: t7001: avoid git on upstream of pipe
  [2/3]: t7001: use "ls-files --format" instead of "cut"
  [3/3]: t: drop "verbose" helper function
 t/t0020-crlf.sh            | 38 +++++++++++++++++++-------------------
 t/t1301-shared-repo.sh     |  4 ++--
 t/t3427-rebase-subtree.sh  | 12 ++++++------
 t/t4022-diff-rewrite.sh    |  2 +-
 t/t4062-diff-pickaxe.sh    |  2 +-
 t/t5304-prune.sh           | 16 ++++++++--------
 t/t6006-rev-list-format.sh |  2 +-
 t/t6501-freshen-objects.sh |  2 +-
 t/t7001-mv.sh              | 37 +++++++++++++++++++++----------------
 t/t7300-clean.sh           |  4 ++--
 t/t9902-completion.sh      | 30 +++++++++++++++---------------
 t/test-lib-functions.sh    |  9 ---------
 12 files changed, 77 insertions(+), 81 deletions(-)
-Peff
Jeff King· May 8, 2023, 19:01 UTC · re: Jeff King · lore

[PATCH 1/3] t7001: avoid git on upstream of pipe

We generally avoid git on the left-hand side of a pipe, because it loses the exit code of the command (and thus we'd miss things like segfaults or unexpected failures). In the cases in t7001, we wouldn't expect failures (they are just inspecting the repository state, and are not the main point of the test), but it doesn't hurt to be careful.

In all but one case here we're piping "ls-files --stage" to cut off the pathname (since we compare entries before and after moving). Let's pull that into a helper function to avoid repeating the slightly awkward replacement.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7001-mv.sh | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)
Show changes to t/t7001-mv.sh +22 −16
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index d72cef8826..ea70419928 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -4,6 +4,11 @@ test_description='git mv in subdirs'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff-data.sh
 
+index_at_path () {
+	entry=$(git ls-files --stage "$@") &&
+	echo "$entry" | cut -f 1
+}
+
 test_expect_success 'mv -f refreshes updated index entry' '
 	echo test >bar &&
 	git add bar &&
@@ -187,7 +192,8 @@ test_expect_success "Michael Cassar's test case" '
 	git mv papers/unsorted/Thesis.pdf papers/all-papers/moo-blah.pdf &&
 
 	T=$(git write-tree) &&
-	git ls-tree -r $T | verbose grep partA/outline.txt
+	git ls-tree -r $T >out &&
+	verbose grep partA/outline.txt out
 '
 
 rm -fr papers partA path?
@@ -260,12 +266,12 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '
 	git init &&
 	echo 1 >dirty &&
 	git add dirty &&
-	entry="$(git ls-files --stage dirty | cut -f 1)" &&
+	entry="$(index_at_path dirty)" &&
 	git mv dirty dirty2 &&
-	test "$entry" = "$(git ls-files --stage dirty2 | cut -f 1)" &&
+	test "$entry" = "$(index_at_path dirty2)" &&
 	echo 2 >dirty2 &&
 	git mv dirty2 dirty &&
-	test "$entry" = "$(git ls-files --stage dirty | cut -f 1)"
+	test "$entry" = "$(index_at_path dirty)"
 '
 
 rm -f dirty dirty2
@@ -342,7 +348,7 @@ test_expect_success 'git mv cannot move a submodule in a file' '
 '
 
 test_expect_success 'git mv moves a submodule with a .git directory and no .gitmodules' '
-	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	entry="$(index_at_path sub)" &&
 	git rm .gitmodules &&
 	(
 		cd sub &&
@@ -353,7 +359,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and no .gitm
 	mkdir mod &&
 	git mv sub mod/sub &&
 	test_path_is_missing sub &&
-	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
+	test "$entry" = "$(index_at_path mod/sub)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
 	git diff-files --quiet
@@ -363,7 +369,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu
 	rm -rf mod &&
 	git reset --hard &&
 	git submodule update &&
-	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	entry="$(index_at_path sub)" &&
 	(
 		cd sub &&
 		rm -f .git &&
@@ -373,7 +379,7 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu
 	mkdir mod &&
 	git mv sub mod/sub &&
 	test_path_is_missing sub &&
-	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
+	test "$entry" = "$(index_at_path mod/sub)" &&
 	git -C mod/sub status &&
 	echo mod/sub >expected &&
 	git config -f .gitmodules submodule.sub.path >actual &&
@@ -386,11 +392,11 @@ test_expect_success 'git mv moves a submodule with gitfile' '
 	rm -rf mod &&
 	git reset --hard &&
 	git submodule update &&
-	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	entry="$(index_at_path sub)" &&
 	mkdir mod &&
 	git -C mod mv ../sub/ . &&
 	test_path_is_missing sub &&
-	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
+	test "$entry" = "$(index_at_path mod/sub)" &&
 	git -C mod/sub status &&
 	echo mod/sub >expected &&
 	git config -f .gitmodules submodule.sub.path >actual &&
@@ -404,12 +410,12 @@ test_expect_success 'mv does not complain when no .gitmodules file is found' '
 	git reset --hard &&
 	git submodule update &&
 	git rm .gitmodules &&
-	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	entry="$(index_at_path sub)" &&
 	mkdir mod &&
 	git mv sub mod/sub 2>actual.err &&
 	test_must_be_empty actual.err &&
 	test_path_is_missing sub &&
-	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
+	test "$entry" = "$(index_at_path mod/sub)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
 	git diff-files --quiet
@@ -420,7 +426,7 @@ test_expect_success 'mv will error out on a modified .gitmodules file unless sta
 	git reset --hard &&
 	git submodule update &&
 	git config -f .gitmodules foo.bar true &&
-	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	entry="$(index_at_path sub)" &&
 	mkdir mod &&
 	test_must_fail git mv sub mod/sub 2>actual.err &&
 	test_file_not_empty actual.err &&
@@ -430,7 +436,7 @@ test_expect_success 'mv will error out on a modified .gitmodules file unless sta
 	git mv sub mod/sub 2>actual.err &&
 	test_must_be_empty actual.err &&
 	test_path_is_missing sub &&
-	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
+	test "$entry" = "$(index_at_path mod/sub)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
 	git diff-files --quiet
@@ -442,13 +448,13 @@ test_expect_success 'mv issues a warning when section is not found in .gitmodule
 	git submodule update &&
 	git config -f .gitmodules --remove-section submodule.sub &&
 	git add .gitmodules &&
-	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	entry="$(index_at_path sub)" &&
 	echo "warning: Could not find section in .gitmodules where path=sub" >expect.err &&
 	mkdir mod &&
 	git mv sub mod/sub 2>actual.err &&
 	test_cmp expect.err actual.err &&
 	test_path_is_missing sub &&
-	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
+	test "$entry" = "$(index_at_path mod/sub)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
 	git diff-files --quiet
-- 
2.40.1.802.gdef2a8734a
Jeff King· May 8, 2023, 19:01 UTC · re: Jeff King · lore

[PATCH 2/3] t7001: use "ls-files --format" instead of "cut"

Since ls-files recently learned a "--format" option, we can use that rather than asking for all of "--stage" and then pulling out the bits we want with "cut". That's simpler and avoids two extra processes (one for cut, and one for the subshell to hold the intermediate result).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7001-mv.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to t/t7001-mv.sh +1 −2
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index ea70419928..2e6a3c0a54 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -5,8 +5,7 @@ test_description='git mv in subdirs'
 . "$TEST_DIRECTORY"/lib-diff-data.sh
 
 index_at_path () {
-	entry=$(git ls-files --stage "$@") &&
-	echo "$entry" | cut -f 1
+	git ls-files --format='%(objectmode) %(objectname) %(stage)' "$@"
 }
 
 test_expect_success 'mv -f refreshes updated index entry' '
-- 
2.40.1.802.gdef2a8734a
Jeff King· May 8, 2023, 19:04 UTC · re: Jeff King · lore

[PATCH 3/3] t: drop "verbose" helper function

We have a small helper function called "verbose", with the idea that you can write:

  verbose foo

to get a message to stderr when the "foo" command fails, even if it does not produce any output itself. This goes back to 8ad1652418 (t5304: use helper to report failure of "test foo = bar", 2014-10-10). It does work, but overall it has not been a big success for two reasons:

  1. Test writers have to remember to put it there (and the resulting
     test code is longer as a result).
  2. It doesn't handle the opposite case (we expect "foo" to fail, but
     it succeeds), leading to inconsistencies in tests (which you can
     see in many hunks of this patch, e.g. ones involving "has_cr").

Most importantly, we added a136f6d8ff (test-lib.sh: support -x option for shell-tracing, 2014-10-10) at the same time, and it does roughly the same thing. The output is not quite as succinct as "verbose", and you have to watch out for stray shell-traces ending up in stderr. But it solves both of the problems above, and has clearly become the preferred tool.

Let's consider the "verbose" function a failed experiment and remove the last few callers (which are all many years old, and have been dwindling as we remove them from scripts we touch for other reasons). It will be one less thing for new test writers to see and wonder if they should be using themselves.

Signed-off-by: Jeff King <peff@peff.net>
---
I think this should be all calls. Most tests will fail after removing
the function, of course, but it's possible one could be hiding in an
expect_failure or something. I didn't see any after grepping for
'[^-]verbose' in t/, which is not too long to look through.
 t/t0020-crlf.sh            | 38 +++++++++++++++++++-------------------
 t/t1301-shared-repo.sh     |  4 ++--
 t/t3427-rebase-subtree.sh  | 12 ++++++------
 t/t4022-diff-rewrite.sh    |  2 +-
 t/t4062-diff-pickaxe.sh    |  2 +-
 t/t5304-prune.sh           | 16 ++++++++--------
 t/t6006-rev-list-format.sh |  2 +-
 t/t6501-freshen-objects.sh |  2 +-
 t/t7001-mv.sh              |  2 +-
 t/t7300-clean.sh           |  4 ++--
 t/t9902-completion.sh      | 30 +++++++++++++++---------------
 t/test-lib-functions.sh    |  9 ---------
 12 files changed, 57 insertions(+), 66 deletions(-)
Show changes to 12 files +57 −66

t/t0020-crlf.sh, t/t1301-shared-repo.sh, t/t3427-rebase-subtree.sh, t/t4022-diff-rewrite.sh, t/t4062-diff-pickaxe.sh, t/t5304-prune.sh, t/t6006-rev-list-format.sh, t/t6501-freshen-objects.sh, t/t7001-mv.sh, t/t7300-clean.sh, t/t9902-completion.sh, t/test-lib-functions.sh

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 35cc8c3b39..81946e87cc 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -125,7 +125,7 @@ test_expect_success 'update with autocrlf=input' '
 	munge_cr append dir/two &&
 	git update-index -- one dir/two &&
 	differs=$(git diff-index --cached HEAD) &&
-	verbose test -z "$differs"
+	test -z "$differs"
 
 '
 
@@ -138,7 +138,7 @@ test_expect_success 'update with autocrlf=true' '
 	munge_cr append dir/two &&
 	git update-index -- one dir/two &&
 	differs=$(git diff-index --cached HEAD) &&
-	verbose test -z "$differs"
+	test -z "$differs"
 
 '
 
@@ -153,7 +153,7 @@ test_expect_success 'checkout with autocrlf=true' '
 	test "$one" = $(git hash-object --stdin <one) &&
 	test "$two" = $(git hash-object --stdin <dir/two) &&
 	differs=$(git diff-index --cached HEAD) &&
-	verbose test -z "$differs"
+	test -z "$differs"
 '
 
 test_expect_success 'checkout with autocrlf=input' '
@@ -167,7 +167,7 @@ test_expect_success 'checkout with autocrlf=input' '
 	test "$one" = $(git hash-object --stdin <one) &&
 	test "$two" = $(git hash-object --stdin <dir/two) &&
 	differs=$(git diff-index --cached HEAD) &&
-	verbose test -z "$differs"
+	test -z "$differs"
 '
 
 test_expect_success 'apply patch (autocrlf=input)' '
@@ -177,7 +177,7 @@ test_expect_success 'apply patch (autocrlf=input)' '
 	git read-tree --reset -u HEAD &&
 
 	git apply patch.file &&
-	verbose test "$patched" = "$(git hash-object --stdin <one)"
+	test "$patched" = "$(git hash-object --stdin <one)"
 '
 
 test_expect_success 'apply patch --cached (autocrlf=input)' '
@@ -187,7 +187,7 @@ test_expect_success 'apply patch --cached (autocrlf=input)' '
 	git read-tree --reset -u HEAD &&
 
 	git apply --cached patch.file &&
-	verbose test "$patched" = $(git rev-parse :one)
+	test "$patched" = $(git rev-parse :one)
 '
 
 test_expect_success 'apply patch --index (autocrlf=input)' '
@@ -197,8 +197,8 @@ test_expect_success 'apply patch --index (autocrlf=input)' '
 	git read-tree --reset -u HEAD &&
 
 	git apply --index patch.file &&
-	verbose test "$patched" = $(git rev-parse :one) &&
-	verbose test "$patched" = $(git hash-object --stdin <one)
+	test "$patched" = $(git rev-parse :one) &&
+	test "$patched" = $(git hash-object --stdin <one)
 '
 
 test_expect_success 'apply patch (autocrlf=true)' '
@@ -208,7 +208,7 @@ test_expect_success 'apply patch (autocrlf=true)' '
 	git read-tree --reset -u HEAD &&
 
 	git apply patch.file &&
-	verbose test "$patched" = "$(remove_cr <one | git hash-object --stdin)"
+	test "$patched" = "$(remove_cr <one | git hash-object --stdin)"
 '
 
 test_expect_success 'apply patch --cached (autocrlf=true)' '
@@ -218,7 +218,7 @@ test_expect_success 'apply patch --cached (autocrlf=true)' '
 	git read-tree --reset -u HEAD &&
 
 	git apply --cached patch.file &&
-	verbose test "$patched" = $(git rev-parse :one)
+	test "$patched" = $(git rev-parse :one)
 '
 
 test_expect_success 'apply patch --index (autocrlf=true)' '
@@ -228,8 +228,8 @@ test_expect_success 'apply patch --index (autocrlf=true)' '
 	git read-tree --reset -u HEAD &&
 
 	git apply --index patch.file &&
-	verbose test "$patched" = $(git rev-parse :one) &&
-	verbose test "$patched" = "$(remove_cr <one | git hash-object --stdin)"
+	test "$patched" = $(git rev-parse :one) &&
+	test "$patched" = "$(remove_cr <one | git hash-object --stdin)"
 '
 
 test_expect_success '.gitattributes says two is binary' '
@@ -240,7 +240,7 @@ test_expect_success '.gitattributes says two is binary' '
 	git read-tree --reset -u HEAD &&
 
 	! has_cr dir/two &&
-	verbose has_cr one &&
+	has_cr one &&
 	! has_cr three
 '
 
@@ -259,8 +259,8 @@ test_expect_success '.gitattributes says two and three are text' '
 	echo "t* crlf" >.gitattributes &&
 	git read-tree --reset -u HEAD &&
 
-	verbose has_cr dir/two &&
-	verbose has_cr three
+	has_cr dir/two &&
+	has_cr three
 '
 
 test_expect_success 'in-tree .gitattributes (1)' '
@@ -273,7 +273,7 @@ test_expect_success 'in-tree .gitattributes (1)' '
 	git read-tree --reset -u HEAD &&
 
 	! has_cr one &&
-	verbose has_cr three
+	has_cr three
 '
 
 test_expect_success 'in-tree .gitattributes (2)' '
@@ -283,7 +283,7 @@ test_expect_success 'in-tree .gitattributes (2)' '
 	git checkout-index -f -q -u -a &&
 
 	! has_cr one &&
-	verbose has_cr three
+	has_cr three
 '
 
 test_expect_success 'in-tree .gitattributes (3)' '
@@ -294,7 +294,7 @@ test_expect_success 'in-tree .gitattributes (3)' '
 	git checkout-index -u one dir/two three &&
 
 	! has_cr one &&
-	verbose has_cr three
+	has_cr three
 '
 
 test_expect_success 'in-tree .gitattributes (4)' '
@@ -305,7 +305,7 @@ test_expect_success 'in-tree .gitattributes (4)' '
 	git checkout-index -u .gitattributes &&
 
 	! has_cr one &&
-	verbose has_cr three
+	has_cr three
 '
 
 test_expect_success 'checkout with existing .gitattributes' '
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 1b6437ec07..ae5cd3f5a0 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -89,7 +89,7 @@ do
 		rm -f .git/info/refs &&
 		git update-server-info &&
 		actual="$(test_modebits .git/info/refs)" &&
-		verbose test "x$actual" = "x-$y"
+		test "x$actual" = "x-$y"
 
 	'
 
@@ -99,7 +99,7 @@ do
 		rm -f .git/info/refs &&
 		git update-server-info &&
 		actual="$(test_modebits .git/info/refs)" &&
-		verbose test "x$actual" = "x-$x"
+		test "x$actual" = "x-$x"
 
 	'
 
diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
index 48b76f8232..1b3e97c875 100755
--- a/t/t3427-rebase-subtree.sh
+++ b/t/t3427-rebase-subtree.sh
@@ -74,9 +74,9 @@ test_expect_success 'Rebase -Xsubtree --empty=ask --onto commit' '
 	test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --onto files-main main &&
 	: first pick results in no changes &&
 	git rebase --skip &&
-	verbose test "$(commit_message HEAD~2)" = "topic_4" &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/topic_5" &&
-	verbose test "$(commit_message HEAD)" = "Empty commit"
+	test "$(commit_message HEAD~2)" = "topic_4" &&
+	test "$(commit_message HEAD~)" = "files_subtree/topic_5" &&
+	test "$(commit_message HEAD)" = "Empty commit"
 '
 
 test_expect_success 'Rebase -Xsubtree --empty=ask --rebase-merges --onto commit' '
@@ -85,9 +85,9 @@ test_expect_success 'Rebase -Xsubtree --empty=ask --rebase-merges --onto commit'
 	test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --rebase-merges --onto files-main --root &&
 	: first pick results in no changes &&
 	git rebase --skip &&
-	verbose test "$(commit_message HEAD~2)" = "topic_4" &&
-	verbose test "$(commit_message HEAD~)" = "files_subtree/topic_5" &&
-	verbose test "$(commit_message HEAD)" = "Empty commit"
+	test "$(commit_message HEAD~2)" = "topic_4" &&
+	test "$(commit_message HEAD~)" = "files_subtree/topic_5" &&
+	test "$(commit_message HEAD)" = "Empty commit"
 '
 
 test_done
diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index 1c89050a97..6fed993ea0 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -24,7 +24,7 @@ test_expect_success setup '
 test_expect_success 'detect rewrite' '
 
 	actual=$(git diff-files -B --summary test) &&
-	verbose expr "$actual" : " rewrite test ([0-9]*%)$"
+	expr "$actual" : " rewrite test ([0-9]*%)$"
 
 '
 
diff --git a/t/t4062-diff-pickaxe.sh b/t/t4062-diff-pickaxe.sh
index 9aaa068ed9..a90b46b678 100755
--- a/t/t4062-diff-pickaxe.sh
+++ b/t/t4062-diff-pickaxe.sh
@@ -24,7 +24,7 @@ test_expect_success '-G matches' '
 
 test_expect_success '-S --pickaxe-regex' '
 	git diff --name-only -S0 --pickaxe-regex HEAD^ >out &&
-	verbose test 4096-zeroes.txt = "$(cat out)"
+	test 4096-zeroes.txt = "$(cat out)"
 '
 
 test_done
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 662ae9b152..f367327441 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -16,7 +16,7 @@ add_blob() {
 	before=$(git count-objects | sed "s/ .*//") &&
 	BLOB=$(echo aleph_0 | git hash-object -w --stdin) &&
 	BLOB_FILE=.git/objects/$(echo $BLOB | sed "s/^../&\//") &&
-	verbose test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
 	test_path_is_file $BLOB_FILE &&
 	test-tool chmtime =+0 $BLOB_FILE
 }
@@ -51,23 +51,23 @@ test_expect_success 'prune stale packs' '
 test_expect_success 'prune --expire' '
 	add_blob &&
 	git prune --expire=1.hour.ago &&
-	verbose test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
 	test_path_is_file $BLOB_FILE &&
 	test-tool chmtime =-86500 $BLOB_FILE &&
 	git prune --expire 1.day &&
-	verbose test $before = $(git count-objects | sed "s/ .*//") &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
 	test_path_is_missing $BLOB_FILE
 '
 
 test_expect_success 'gc: implicit prune --expire' '
 	add_blob &&
 	test-tool chmtime =-$((2*$week-30)) $BLOB_FILE &&
 	git gc --no-cruft &&
-	verbose test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
 	test_path_is_file $BLOB_FILE &&
 	test-tool chmtime =-$((2*$week+1)) $BLOB_FILE &&
 	git gc --no-cruft &&
-	verbose test $before = $(git count-objects | sed "s/ .*//") &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
 	test_path_is_missing $BLOB_FILE
 '
 
@@ -138,7 +138,7 @@ test_expect_success 'gc --no-prune' '
 	test-tool chmtime =-$((5001*$day)) $BLOB_FILE &&
 	git config gc.pruneExpire 2.days.ago &&
 	git gc --no-prune --no-cruft &&
-	verbose test 1 = $(git count-objects | sed "s/ .*//") &&
+	test 1 = $(git count-objects | sed "s/ .*//") &&
 	test_path_is_file $BLOB_FILE
 '
 
@@ -192,10 +192,10 @@ test_expect_success 'gc: prune old objects after local clone' '
 	git clone --no-hardlinks . aclone &&
 	(
 		cd aclone &&
-		verbose test 1 = $(git count-objects | sed "s/ .*//") &&
+		test 1 = $(git count-objects | sed "s/ .*//") &&
 		test_path_is_file $BLOB_FILE &&
 		git gc --prune --no-cruft &&
-		verbose test 0 = $(git count-objects | sed "s/ .*//") &&
+		test 0 = $(git count-objects | sed "s/ .*//") &&
 		test_path_is_missing $BLOB_FILE
 	)
 '
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 41d0ca00b1..573eb97a0f 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -493,7 +493,7 @@ test_expect_success 'empty email' '
 	test_tick &&
 	C=$(GIT_AUTHOR_EMAIL= git commit-tree HEAD^{tree} </dev/null) &&
 	A=$(git show --pretty=format:%an,%ae,%ad%n -s $C) &&
-	verbose test "$A" = "$GIT_AUTHOR_NAME,,Thu Apr 7 15:14:13 2005 -0700"
+	test "$A" = "$GIT_AUTHOR_NAME,,Thu Apr 7 15:14:13 2005 -0700"
 '
 
 test_expect_success 'del LF before empty (1)' '
diff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh
index dbfa8a4d4c..4521508b83 100755
--- a/t/t6501-freshen-objects.sh
+++ b/t/t6501-freshen-objects.sh
@@ -152,7 +152,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '
 	EOF
 	commit=$(git hash-object -t commit -w broken-commit) &&
 	git gc --no-cruft -q 2>stderr &&
-	verbose git cat-file -e $commit &&
+	git cat-file -e $commit &&
 	test_must_be_empty stderr
 '
 
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 2e6a3c0a54..898a920532 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -192,7 +192,7 @@ test_expect_success "Michael Cassar's test case" '
 
 	T=$(git write-tree) &&
 	git ls-tree -r $T >out &&
-	verbose grep partA/outline.txt out
+	grep partA/outline.txt out
 '
 
 rm -fr papers partA path?
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index c975eb54d2..0ef7b78457 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -120,7 +120,7 @@ test_expect_success 'git clean with relative prefix' '
 		grep part3 |
 		sed -n -e "s|^Would remove ||p"
 	) &&
-	verbose test "$would_clean" = ../src/part3.c
+	test "$would_clean" = ../src/part3.c
 '
 
 test_expect_success 'git clean with absolute path' '
@@ -133,7 +133,7 @@ test_expect_success 'git clean with absolute path' '
 		grep part3 |
 		sed -n -e "s|^Would remove ||p"
 	) &&
-	verbose test "$would_clean" = ../src/part3.c
+	test "$would_clean" = ../src/part3.c
 '
 
 test_expect_success 'git clean with out of work tree relative path' '
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index d6c0478d98..8835e16e81 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -405,40 +405,40 @@ test_expect_success '__gitdir - remote as argument' '
 
 test_expect_success '__git_dequote - plain unquoted word' '
 	__git_dequote unquoted-word &&
-	verbose test unquoted-word = "$dequoted_word"
+	test unquoted-word = "$dequoted_word"
 '
 
 # input:    b\a\c\k\'\\\"s\l\a\s\h\es
 # expected: back'\"slashes
 test_expect_success '__git_dequote - backslash escaped' '
 	__git_dequote "b\a\c\k\\'\''\\\\\\\"s\l\a\s\h\es" &&
-	verbose test "back'\''\\\"slashes" = "$dequoted_word"
+	test "back'\''\\\"slashes" = "$dequoted_word"
 '
 
 # input:    sin'gle\' '"quo'ted
 # expected: single\ "quoted
 test_expect_success '__git_dequote - single quoted' '
 	__git_dequote "'"sin'gle\\\\' '\\\"quo'ted"'" &&
-	verbose test '\''single\ "quoted'\'' = "$dequoted_word"
+	test '\''single\ "quoted'\'' = "$dequoted_word"
 '
 
 # input:    dou"ble\\" "\"\quot"ed
 # expected: double\ "\quoted
 test_expect_success '__git_dequote - double quoted' '
 	__git_dequote '\''dou"ble\\" "\"\quot"ed'\'' &&
-	verbose test '\''double\ "\quoted'\'' = "$dequoted_word"
+	test '\''double\ "\quoted'\'' = "$dequoted_word"
 '
 
 # input: 'open single quote
 test_expect_success '__git_dequote - open single quote' '
 	__git_dequote "'\''open single quote" &&
-	verbose test "open single quote" = "$dequoted_word"
+	test "open single quote" = "$dequoted_word"
 '
 
 # input: "open double quote
 test_expect_success '__git_dequote - open double quote' '
 	__git_dequote "\"open double quote" &&
-	verbose test "open double quote" = "$dequoted_word"
+	test "open double quote" = "$dequoted_word"
 '
 
 
@@ -616,7 +616,7 @@ test_expect_success '__git_is_configured_remote' '
 	test_when_finished "git remote remove remote_2" &&
 	git remote add remote_2 git://remote_2 &&
 	(
-		verbose __git_is_configured_remote remote_2 &&
+		__git_is_configured_remote remote_2 &&
 		test_must_fail __git_is_configured_remote non-existent
 	)
 '
@@ -2596,30 +2596,30 @@ test_expect_success 'options with value' '
 test_expect_success 'sourcing the completion script clears cached commands' '
 	(
 		__git_compute_all_commands &&
-		verbose test -n "$__git_all_commands" &&
+		test -n "$__git_all_commands" &&
 		. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
-		verbose test -z "$__git_all_commands"
+		test -z "$__git_all_commands"
 	)
 '
 
 test_expect_success 'sourcing the completion script clears cached merge strategies' '
 	(
 		__git_compute_merge_strategies &&
-		verbose test -n "$__git_merge_strategies" &&
+		test -n "$__git_merge_strategies" &&
 		. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
-		verbose test -z "$__git_merge_strategies"
+		test -z "$__git_merge_strategies"
 	)
 '
 
 test_expect_success 'sourcing the completion script clears cached --options' '
 	(
 		__gitcomp_builtin checkout &&
-		verbose test -n "$__gitcomp_builtin_checkout" &&
+		test -n "$__gitcomp_builtin_checkout" &&
 		__gitcomp_builtin notes_edit &&
-		verbose test -n "$__gitcomp_builtin_notes_edit" &&
+		test -n "$__gitcomp_builtin_notes_edit" &&
 		. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
-		verbose test -z "$__gitcomp_builtin_checkout" &&
-		verbose test -z "$__gitcomp_builtin_notes_edit"
+		test -z "$__gitcomp_builtin_checkout" &&
+		test -z "$__gitcomp_builtin_notes_edit"
 	)
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 999d46fafe..6e19ebc922 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1227,15 +1227,6 @@ test_i18ngrep () {
 	return 1
 }
 
-# Call any command "$@" but be more verbose about its
-# failure. This is handy for commands like "test" which do
-# not output anything when they fail.
-verbose () {
-	"$@" && return 0
-	echo >&4 "command failed: $(git rev-parse --sq-quote "$@")"
-	return 1
-}
-
 # Check if the file expected to be empty is indeed empty, and barfs
 # otherwise.
 
-- 
2.40.1.802.gdef2a8734a
Taylor Blau· May 8, 2023, 22:26 UTC · re: Jeff King · lore

Re: [PATCH 3/3] t: drop "verbose" helper function

On Mon, May 08, 2023 at 03:04:57PM -0400, Jeff King wrote:
Show 5 quoted lines
> Let's consider the "verbose" function a failed experiment and remove the
> last few callers (which are all many years old, and have been dwindling
> as we remove them from scripts we touch for other reasons). It will be
> one less thing for new test writers to see and wonder if they should be
> using themselves.
Well put.
Show 18 quoted lines
> I think this should be all calls. Most tests will fail after removing
> the function, of course, but it's possible one could be hiding in an
> expect_failure or something. I didn't see any after grepping for
> '[^-]verbose' in t/, which is not too long to look through.
>
>  t/t0020-crlf.sh            | 38 +++++++++++++++++++-------------------
>  t/t1301-shared-repo.sh     |  4 ++--
>  t/t3427-rebase-subtree.sh  | 12 ++++++------
>  t/t4022-diff-rewrite.sh    |  2 +-
>  t/t4062-diff-pickaxe.sh    |  2 +-
>  t/t5304-prune.sh           | 16 ++++++++--------
>  t/t6006-rev-list-format.sh |  2 +-
>  t/t6501-freshen-objects.sh |  2 +-
>  t/t7001-mv.sh              |  2 +-
>  t/t7300-clean.sh           |  4 ++--
>  t/t9902-completion.sh      | 30 +++++++++++++++---------------
>  t/test-lib-functions.sh    |  9 ---------
>  12 files changed, 57 insertions(+), 66 deletions(-)

I applied these myself and grepped around myself, and also could not find any stragglers. So I'd be happy to drop the implementation of verbose() in this series, too, to avoid the appearance of it continuing to be a blessed path.

Thanks, Taylor

Taylor Blau· May 8, 2023, 22:26 UTC · re: Jeff King · lore

Re: [PATCH 0/3] dropping "verbose" test helper function

On Mon, May 08, 2023 at 02:59:53PM -0400, Jeff King wrote:
Show 13 quoted lines
> I happened across an instance of the "verbose" helper function in a
> test recently. I think it's clear we prefer the "-x" tracing to it these
> days, and we've been slowly removing instances. This series gets rid of
> the last few.
>
> The actual change (and rationale) is in patch 3. Patch 1 just fixes pipe
> problems on lines we'll be touching patch 3, and patch 2 is a cute
> optimization enabled by patch 1. I don't think any of it should be very
> controversial, but it can all be split up if need be.
>
>   [1/3]: t7001: avoid git on upstream of pipe
>   [2/3]: t7001: use "ls-files --format" instead of "cut"
>   [3/3]: t: drop "verbose" helper function
All look good to me, I'd be happy to see this get picked up.

Thanks, Taylor

← back to recent threads