[PATCH 0/4] t: new helper test_line_count_cmd

[PATCH 0/4] t: new helper test_line_count_cmd

[Đoàn Trần Công Danh]

This is a rewrite of a series I sent earlier [1].
Which was a response to a false positive when applying Junio's suggestion
to a series written by Ævar [2].

1: https://lore.kernel.org/git/20210421104102.3409-1-congdanhqx@gmail.com/
2: https://lore.kernel.org/git/87r1j42ffz.fsf@evledraar.gmail.com/

Đoàn Trần Công Danh (4):
  test-lib-functions: introduce test_line_count_cmd
  t6402: use find(1) builtin to filter instead of grep
  t6400: use test_line_count_cmd to count # of lines in stdout
  t6402: use test_line_count_cmd to count # of lines in stdout

 t/t6400-merge-df.sh     |  16 ++---
 t/t6402-merge-rename.sh | 132 +++++++++++++++++++---------------------
 t/test-lib-functions.sh |  64 +++++++++++++++++++
 3 files changed, 134 insertions(+), 78 deletions(-)

-- 
2.32.0.278.gd42b80f139

[PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Đoàn Trần Công Danh]

In Git project, we have multiple occasions that requires checking number
of lines of text in stdout and/or stderr of a command. One of such
example is t6400, which checks number of files in various states.

Some of those commands are Git command, and we would like to check their
exit status.  In some of those checks, we pipe the stdout of those
commands to "wc -l" to check for line count, thus loosing the exit status.

Introduce a helper function to check for number of lines in stdout and
stderr from those commands.

This helper will create 2 temporary files in process, thus it may affect
output of some checks.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

Notes:
    Theoretically, we could avoid those temporary files by this shenanigan:
    
    	! (
    	  test $(
    		(
    		  test $(
    			  ( "$@" || echo "'$*' run into failure" >&3) | wc -l
    			)
    			"$out_ops" "$out_val" ||
    		  echo "stdout: !$outop $outval '$*'" >&3
    		) 2>&1 | wc -l
    	  ) "$errop" "$errval" ||
    	  echo "stderr: !$errop $errval '$*'" >&3
    	) 3>&1 | grep .
    
    However, it looks too complicated.

 t/test-lib-functions.sh | 64 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b823c14027..85bb31ea4c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -817,6 +817,70 @@ test_line_count () {
 	fi
 }
 
+# test_line_count_cmd checks the number of lines of captured stdout and/or
+# stderr of a command.
+#
+# NOTE: this helper function will create 2 temporary files named:
+# * test_line_count_cmd_.out; and
+# * test_line_count_cmd_.err
+#
+# And this helper function will remove those 2 files if the check is succeed.
+# In case of failure, those files will be preserved.
+test_line_count_cmd () {
+	local outop outval
+	local errop errval
+
+	while test $# -ge 3
+	do
+		case "$1" in
+		--out)
+			outop="$2"
+			outval="$3"
+			;;
+		--err)
+			errop="$2"
+			errval="$3"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift 3
+	done &&
+	if test $# = 0 ||
+	   { test "x$1" = "x!" && test $# = 1 ; }
+	then
+		BUG "test_line_count_cmd: no command to be run"
+	fi &&
+	if test -z "$outop$errop"
+	then
+		BUG "test_line_count_cmd: check which stream?"
+	fi &&
+
+	if test "x$1" = "x!" 
+	then
+		shift &&
+		if "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
+		then
+			echo "error: '$@' succeed!"
+			return 1
+		fi
+	elif ! "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
+	then
+		echo "error: '$@' run into failure!"
+		return 1
+	fi &&
+	if test -n "$outop"
+	then
+		test_line_count "$outop" "$outval" test_line_count_cmd_.out
+	fi &&
+	if test -n "$errop"
+	then
+		test_line_count "$errop" "$errval" test_line_count_cmd_.err
+	fi &&
+	rm -f test_line_count_cmd_.out test_line_count_cmd_.err
+}
+
 test_file_size () {
 	test "$#" -ne 1 && BUG "1 param"
 	test-tool path-utils file-size "$1"
-- 
2.32.0.278.gd42b80f139

[PATCH 2/4] t6402: use find(1) builtin to filter instead of grep

[Đoàn Trần Công Danh]

find(1) has a builtin (-prune) to filter its output, save a bit of time
for invoking grep(1).

In addition, in a later change, we will try to use test_line_count_cmd
to count number of lines in stdout and/or stderr of a command, due to
limitation of current implementation, it can handle pipe.

Let's replace grep(1)'s usage with find(1) builtin filter.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6402-merge-rename.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
index 425dad97d5..5d76cd6414 100755
--- a/t/t6402-merge-rename.sh
+++ b/t/t6402-merge-rename.sh
@@ -546,7 +546,7 @@ then
 
 		test_must_fail git diff --quiet &&
 
-		test 3 -eq $(find . | grep -v .git | wc -l) &&
+		test 3 -eq $(find . -name .git -prune -o -print | wc -l) &&
 
 		test_path_is_file one &&
 		test_path_is_file two &&
@@ -565,7 +565,7 @@ else
 
 		test_must_fail git diff --quiet &&
 
-		test 4 -eq $(find . | grep -v .git | wc -l) &&
+		test 4 -eq $(find . -name .git -prune -o -print | wc -l) &&
 
 		test_path_is_dir one &&
 		test_path_is_file one~rename-two &&
@@ -593,7 +593,7 @@ test_expect_success 'pair rename to parent of other (D/F conflicts) w/ clean sta
 
 	test_must_fail git diff --quiet &&
 
-	test 3 -eq $(find . | grep -v .git | wc -l) &&
+	test 3 -eq $(find . -name .git -prune -o -print | wc -l) &&
 
 	test_path_is_file one &&
 	test_path_is_file two &&
-- 
2.32.0.278.gd42b80f139

[PATCH 3/4] t6400: use test_line_count_cmd to count # of lines in stdout

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6400-merge-df.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t6400-merge-df.sh b/t/t6400-merge-df.sh
index 38700d29b5..9d7b9354c5 100755
--- a/t/t6400-merge-df.sh
+++ b/t/t6400-merge-df.sh
@@ -82,13 +82,13 @@ test_expect_success 'modify/delete + directory/file conflict' '
 	git checkout delete^0 &&
 	test_must_fail git merge modify &&
 
-	test 5 -eq $(git ls-files -s | wc -l) &&
-	test 4 -eq $(git ls-files -u | wc -l) &&
+	test_line_count_cmd --out = 5 git ls-files -s &&
+	test_line_count_cmd --out = 4 git ls-files -u  &&
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	then
-		test 0 -eq $(git ls-files -o | wc -l)
+		test_line_count_cmd --out = 2 git ls-files -o
 	else
-		test 1 -eq $(git ls-files -o | wc -l)
+		test_line_count_cmd --out = 3 git ls-files -o
 	fi &&
 
 	test_path_is_file letters/file &&
@@ -103,13 +103,13 @@ test_expect_success 'modify/delete + directory/file conflict; other way' '
 
 	test_must_fail git merge delete &&
 
-	test 5 -eq $(git ls-files -s | wc -l) &&
-	test 4 -eq $(git ls-files -u | wc -l) &&
+	test_line_count_cmd --out = 5 git ls-files -s  &&
+	test_line_count_cmd --out = 4 git ls-files -u  &&
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	then
-		test 0 -eq $(git ls-files -o | wc -l)
+		test_line_count_cmd --out = 2 git ls-files -o
 	else
-		test 1 -eq $(git ls-files -o | wc -l)
+		test_line_count_cmd --out = 3 git ls-files -o
 	fi &&
 
 	test_path_is_file letters/file &&
-- 
2.32.0.278.gd42b80f139

[PATCH 4/4] t6402: use test_line_count_cmd to count # of lines in stdout

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6402-merge-rename.sh | 126 +++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 67 deletions(-)

diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
index 5d76cd6414..81502dba84 100755
--- a/t/t6402-merge-rename.sh
+++ b/t/t6402-merge-rename.sh
@@ -105,10 +105,8 @@ test_expect_success 'pull renaming branch into unrenaming one' \
 	git show-branch &&
 	test_expect_code 1 git pull . white &&
 	git ls-files -s &&
-	git ls-files -u B >b.stages &&
-	test_line_count = 3 b.stages &&
-	git ls-files -s N >n.stages &&
-	test_line_count = 1 n.stages &&
+	test_line_count_cmd --out = 3 git ls-files -u B &&
+	test_line_count_cmd --out = 1 git ls-files -s N &&
 	sed -ne "/^g/{
 	p
 	q
@@ -122,10 +120,8 @@ test_expect_success 'pull renaming branch into another renaming one' \
 	git reset --hard &&
 	git checkout red &&
 	test_expect_code 1 git pull . white &&
-	git ls-files -u B >b.stages &&
-	test_line_count = 3 b.stages &&
-	git ls-files -s N >n.stages &&
-	test_line_count = 1 n.stages &&
+	test_line_count_cmd --out = 3 git ls-files -u B &&
+	test_line_count_cmd --out = 1 git ls-files -s N &&
 	sed -ne "/^g/{
 	p
 	q
@@ -138,10 +134,8 @@ test_expect_success 'pull unrenaming branch into renaming one' \
 	git reset --hard &&
 	git show-branch &&
 	test_expect_code 1 git pull . main &&
-	git ls-files -u B >b.stages &&
-	test_line_count = 3 b.stages &&
-	git ls-files -s N >n.stages &&
-	test_line_count = 1 n.stages &&
+	test_line_count_cmd --out = 3 git ls-files -u B &&
+	test_line_count_cmd --out = 1 git ls-files -s N &&
 	sed -ne "/^g/{
 	p
 	q
@@ -154,14 +148,10 @@ test_expect_success 'pull conflicting renames' \
 	git reset --hard &&
 	git show-branch &&
 	test_expect_code 1 git pull . blue &&
-	git ls-files -u A >a.stages &&
-	test_line_count = 1 a.stages &&
-	git ls-files -u B >b.stages &&
-	test_line_count = 1 b.stages &&
-	git ls-files -u C >c.stages &&
-	test_line_count = 1 c.stages &&
-	git ls-files -s N >n.stages &&
-	test_line_count = 1 n.stages &&
+	test_line_count_cmd --out = 1 git ls-files -u A &&
+	test_line_count_cmd --out = 1 git ls-files -u B &&
+	test_line_count_cmd --out = 1 git ls-files -u C &&
+	test_line_count_cmd --out = 1 git ls-files -s N &&
 	sed -ne "/^g/{
 	p
 	q
@@ -330,8 +320,8 @@ test_expect_success 'Rename+D/F conflict; renamed file merges but dir in way' '
 		test_i18ngrep "Adding as dir~HEAD instead" output
 	fi &&
 
-	test 3 -eq "$(git ls-files -u | wc -l)" &&
-	test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
+	test_line_count_cmd --out = 3 git ls-files -u  &&
+	test_line_count_cmd --out = 2 git ls-files -u dir/file-in-the-way  &&
 
 	test_must_fail git diff --quiet &&
 	test_must_fail git diff --cached --quiet &&
@@ -357,8 +347,8 @@ test_expect_success 'Same as previous, but merged other way' '
 		test_i18ngrep "Adding as dir~renamed-file-has-no-conflicts instead" output
 	fi &&
 
-	test 3 -eq "$(git ls-files -u | wc -l)" &&
-	test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
+	test_line_count_cmd --out = 3 git ls-files -u  &&
+	test_line_count_cmd --out = 2 git ls-files -u dir/file-in-the-way  &&
 
 	test_must_fail git diff --quiet &&
 	test_must_fail git diff --cached --quiet &&
@@ -374,8 +364,8 @@ test_expect_success 'Rename+D/F conflict; renamed file cannot merge, dir not in
 	git checkout -q renamed-file-has-conflicts^0 &&
 	test_must_fail git merge --strategy=recursive dir-not-in-way &&
 
-	test 3 -eq "$(git ls-files -u | wc -l)" &&
-	test 3 -eq "$(git ls-files -u dir | wc -l)" &&
+	test_line_count_cmd --out = 3 git ls-files -u  &&
+	test_line_count_cmd --out = 3 git ls-files -u dir  &&
 
 	test_must_fail git diff --quiet &&
 	test_must_fail git diff --cached --quiet &&
@@ -409,14 +399,15 @@ test_expect_success 'Rename+D/F conflict; renamed file cannot merge and dir in t
 	git checkout -q renamed-file-has-conflicts^0 &&
 	test_must_fail git merge --strategy=recursive dir-in-way &&
 
-	test 5 -eq "$(git ls-files -u | wc -l)" &&
+	test_line_count_cmd --out = 5 git ls-files -u  &&
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	then
-		test 3 -eq "$(git ls-files -u dir~HEAD | wc -l)"
+		test_line_count_cmd --out = 3 git ls-files -u dir~HEAD
 	else
-		test 3 -eq "$(git ls-files -u dir | grep -v file-in-the-way | wc -l)"
+		git ls-files -u dir >out &&
+		test_line_count_cmd --out = 3 grep -v file-in-the-way out
 	fi &&
-	test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
+	test_line_count_cmd --out = 2 git ls-files -u dir/file-in-the-way  &&
 
 	test_must_fail git diff --quiet &&
 	test_must_fail git diff --cached --quiet &&
@@ -432,14 +423,15 @@ test_expect_success 'Same as previous, but merged other way' '
 	git checkout -q dir-in-way^0 &&
 	test_must_fail git merge --strategy=recursive renamed-file-has-conflicts &&
 
-	test 5 -eq "$(git ls-files -u | wc -l)" &&
+	test_line_count_cmd --out = 5 git ls-files -u  &&
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	then
-		test 3 -eq "$(git ls-files -u dir~renamed-file-has-conflicts | wc -l)"
+		test_line_count_cmd --out = 3 git ls-files -u dir~renamed-file-has-conflicts
 	else
-		test 3 -eq "$(git ls-files -u dir | grep -v file-in-the-way | wc -l)"
+		git ls-files -u dir >out &&
+		test_line_count_cmd --out = 3 grep -v file-in-the-way out
 	fi &&
-	test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
+	test_line_count_cmd --out = 2 git ls-files -u dir/file-in-the-way  &&
 
 	test_must_fail git diff --quiet &&
 	test_must_fail git diff --cached --quiet &&
@@ -496,9 +488,9 @@ test_expect_success 'both rename source and destination involved in D/F conflict
 
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	then
-		test 2 -eq "$(git ls-files -u | wc -l)"
+		test_line_count_cmd --out = 2 git ls-files -u
 	else
-		test 1 -eq "$(git ls-files -u | wc -l)"
+		test_line_count_cmd --out = 1 git ls-files -u
 	fi &&
 
 	test_must_fail git diff --quiet &&
@@ -540,9 +532,9 @@ then
 		mkdir one &&
 		test_must_fail git merge --strategy=recursive rename-two &&
 
-		test 4 -eq "$(git ls-files -u | wc -l)" &&
-		test 2 -eq "$(git ls-files -u one | wc -l)" &&
-		test 2 -eq "$(git ls-files -u two | wc -l)" &&
+		test_line_count_cmd --out = 4 git ls-files -u  &&
+		test_line_count_cmd --out = 2 git ls-files -u one  &&
+		test_line_count_cmd --out = 2 git ls-files -u two  &&
 
 		test_must_fail git diff --quiet &&
 
@@ -559,9 +551,9 @@ else
 		mkdir one &&
 		test_must_fail git merge --strategy=recursive rename-two &&
 
-		test 2 -eq "$(git ls-files -u | wc -l)" &&
-		test 1 -eq "$(git ls-files -u one | wc -l)" &&
-		test 1 -eq "$(git ls-files -u two | wc -l)" &&
+		test_line_count_cmd --out = 2 git ls-files -u  &&
+		test_line_count_cmd --out = 1 git ls-files -u one  &&
+		test_line_count_cmd --out = 1 git ls-files -u two  &&
 
 		test_must_fail git diff --quiet &&
 
@@ -582,13 +574,13 @@ test_expect_success 'pair rename to parent of other (D/F conflicts) w/ clean sta
 
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	then
-		test 4 -eq "$(git ls-files -u | wc -l)" &&
-		test 2 -eq "$(git ls-files -u one | wc -l)" &&
-		test 2 -eq "$(git ls-files -u two | wc -l)"
+		test_line_count_cmd --out = 4 git ls-files -u  &&
+		test_line_count_cmd --out = 2 git ls-files -u one  &&
+		test_line_count_cmd --out = 2 git ls-files -u two
 	else
-		test 2 -eq "$(git ls-files -u | wc -l)" &&
-		test 1 -eq "$(git ls-files -u one | wc -l)" &&
-		test 1 -eq "$(git ls-files -u two | wc -l)"
+		test_line_count_cmd --out = 2 git ls-files -u  &&
+		test_line_count_cmd --out = 1 git ls-files -u one  &&
+		test_line_count_cmd --out = 1 git ls-files -u two
 	fi &&
 
 	test_must_fail git diff --quiet &&
@@ -631,19 +623,19 @@ test_expect_success 'check handling of differently renamed file with D/F conflic
 
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	then
-		test 5 -eq "$(git ls-files -s | wc -l)" &&
-		test 3 -eq "$(git ls-files -u | wc -l)" &&
-		test 1 -eq "$(git ls-files -u one~HEAD | wc -l)" &&
-		test 1 -eq "$(git ls-files -u two~second-rename | wc -l)" &&
-		test 1 -eq "$(git ls-files -u original | wc -l)" &&
-		test 0 -eq "$(git ls-files -o | wc -l)"
+		test_line_count_cmd --out = 5 git ls-files -s  &&
+		test_line_count_cmd --out = 3 git ls-files -u  &&
+		test_line_count_cmd --out = 1 git ls-files -u one~HEAD  &&
+		test_line_count_cmd --out = 1 git ls-files -u two~second-rename  &&
+		test_line_count_cmd --out = 1 git ls-files -u original  &&
+		test_line_count_cmd --out = 2 git ls-files -o
 	else
-		test 5 -eq "$(git ls-files -s | wc -l)" &&
-		test 3 -eq "$(git ls-files -u | wc -l)" &&
-		test 1 -eq "$(git ls-files -u one | wc -l)" &&
-		test 1 -eq "$(git ls-files -u two | wc -l)" &&
-		test 1 -eq "$(git ls-files -u original | wc -l)" &&
-		test 2 -eq "$(git ls-files -o | wc -l)"
+		test_line_count_cmd --out = 5 git ls-files -s  &&
+		test_line_count_cmd --out = 3 git ls-files -u  &&
+		test_line_count_cmd --out = 1 git ls-files -u one  &&
+		test_line_count_cmd --out = 1 git ls-files -u two  &&
+		test_line_count_cmd --out = 1 git ls-files -u original  &&
+		test_line_count_cmd --out = 4 git ls-files -o
 	fi &&
 
 	test_path_is_file one/file &&
@@ -679,11 +671,11 @@ test_expect_success 'check handling of differently renamed file with D/F conflic
 	git checkout -q first-rename-redo^0 &&
 	test_must_fail git merge --strategy=recursive second-rename-redo &&
 
-	test 3 -eq "$(git ls-files -u | wc -l)" &&
-	test 1 -eq "$(git ls-files -u one | wc -l)" &&
-	test 1 -eq "$(git ls-files -u two | wc -l)" &&
-	test 1 -eq "$(git ls-files -u original | wc -l)" &&
-	test 0 -eq "$(git ls-files -o | wc -l)" &&
+	test_line_count_cmd --out = 3 git ls-files -u  &&
+	test_line_count_cmd --out = 1 git ls-files -u one  &&
+	test_line_count_cmd --out = 1 git ls-files -u two  &&
+	test_line_count_cmd --out = 1 git ls-files -u original  &&
+	test_line_count_cmd --out = 2 git ls-files -o  &&
 
 	test_path_is_file one &&
 	test_path_is_file two &&
@@ -861,8 +853,8 @@ test_expect_success 'setup merge of rename + small change' '
 test_expect_success 'merge rename + small change' '
 	git merge rename_branch &&
 
-	test 1 -eq $(git ls-files -s | wc -l) &&
-	test 0 -eq $(git ls-files -o | wc -l) &&
+	test_line_count_cmd --out = 1 git ls-files -s  &&
+	test_line_count_cmd --out = 2 git ls-files -o  &&
 	test $(git rev-parse HEAD:renamed_file) = $(git rev-parse HEAD~1:file)
 '
 
-- 
2.32.0.278.gd42b80f139

Re: [PATCH 3/4] t6400: use test_line_count_cmd to count # of lines in stdout

[Bagas Sanjaya]

Hi Đoàn,

> -	test 5 -eq $(git ls-files -s | wc -l) &&
> -	test 4 -eq $(git ls-files -u | wc -l) &&
> +	test_line_count_cmd --out = 5 git ls-files -s &&
> +	test_line_count_cmd --out = 4 git ls-files -u  &&

I read lines above as "Formerly I tested that 5/4 should be equal to 
output of git ls-files -s/-u piped to wc -l, now I do the same with 
test_line_count_cmd".

Am I right?

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Eric Sunshine]

On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
> In Git project, we have multiple occasions that requires checking number
> of lines of text in stdout and/or stderr of a command. One of such
> example is t6400, which checks number of files in various states.
>
> Some of those commands are Git command, and we would like to check their
> exit status.  In some of those checks, we pipe the stdout of those
> commands to "wc -l" to check for line count, thus loosing the exit status.
>
> Introduce a helper function to check for number of lines in stdout and
> stderr from those commands.
>
> This helper will create 2 temporary files in process, thus it may affect
> output of some checks.

If the presence of these files is a concern, I wonder if it would make
sense to turn these into dot-files (leading dot in name) or shove them
into the .git/ directory? (Not necessarily an actionable comment; just
tossing around some thoughts.)

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -817,6 +817,70 @@ test_line_count () {
> +# test_line_count_cmd checks the number of lines of captured stdout and/or
> +# stderr of a command.
> +#
> +# NOTE: this helper function will create 2 temporary files named:
> +# * test_line_count_cmd_.out; and
> +# * test_line_count_cmd_.err
> +#
> +# And this helper function will remove those 2 files if the check is succeed.
> +# In case of failure, those files will be preserved.
> +test_line_count_cmd () {

It would be good to have some usage information in the above comment
so that people aren't forced to consult the implementation to learn
what options this function takes. At minimum, it should mention
`--out`, `--err`, and `!`, and should explain the arguments each
option takes (even if just through examples).

> +       local outop outval
> +       local errop errval
> +
> +       while test $# -ge 3
> +       do
> +               case "$1" in
> +               --out)
> +                       outop="$2"
> +                       outval="$3"
> +                       ;;
> +               --err)
> +                       errop="$2"
> +                       errval="$3"
> +                       ;;
> +               *)
> +                       break
> +                       ;;
> +               esac
> +               shift 3
> +       done &&

This is really minor, but if test_line_count_cmd() ever learns some
new option and that option does not consume two arguments, then the
`shift 3` at the end of the `case/esac` will need to be adjusted in
some fashion. It might make more sense, therefore, to perform the
`shift 3` closer to where it is needed (that is, in the `--out` case
and in the `--err` case) rather than delaying it as is done here. (Not
necessarily worth a re-roll.)

Another minor comment: Since you're &&-chaining everything else in the
function, it wouldn't hurt to also do so for the `local` declarations
and the assignments within each `case` arm, and to chain `esac` with
`shift 3`. Doing so could help some future programmer who might (for
some reason) insert code above the `while` loop, thinking that a
failure in the new code will abort the function, but not realizing
that the &&-chain is not intact in this area of the code.

> +       if test $# = 0 ||
> +          { test "x$1" = "x!" && test $# = 1 ; }
> +       then
> +               BUG "test_line_count_cmd: no command to be run"
> +       fi &&
> +       if test -z "$outop$errop"
> +       then
> +               BUG "test_line_count_cmd: check which stream?"
> +       fi &&
> +
> +       if test "x$1" = "x!"
> +       then
> +               shift &&
> +               if "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> +               then
> +                       echo "error: '$@' succeed!"
> +                       return 1
> +               fi
> +       elif ! "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> +       then
> +               echo "error: '$@' run into failure!"
> +               return 1
> +       fi &&

Do we care that the "!" negated case doesn't have the same semantics
as test_must_fail()? If we do care, then should there be a way to tell
the function whether we want test_must_fail() semantics or `!`
semantics (i.e. whether we're running a Git command or a non-Git
command) or should it infer it on its own? (These are genuine
questions -- not necessarily requests for changes -- as I'm trying to
reason through the implications of this implementation choice.)

> +       if test -n "$outop"
> +       then
> +               test_line_count "$outop" "$outval" test_line_count_cmd_.out
> +       fi &&
> +       if test -n "$errop"
> +       then
> +               test_line_count "$errop" "$errval" test_line_count_cmd_.err
> +       fi &&
> +       rm -f test_line_count_cmd_.out test_line_count_cmd_.err
> +}

Re: [PATCH 3/4] t6400: use test_line_count_cmd to count # of lines in stdout

[Eric Sunshine]

On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
> diff --git a/t/t6400-merge-df.sh b/t/t6400-merge-df.sh
> @@ -82,13 +82,13 @@ test_expect_success 'modify/delete + directory/file conflict' '
> -       test 5 -eq $(git ls-files -s | wc -l) &&
> -       test 4 -eq $(git ls-files -u | wc -l) &&
> +       test_line_count_cmd --out = 5 git ls-files -s &&
> +       test_line_count_cmd --out = 4 git ls-files -u  &&

Nit: too many spaces before the `&&` on the second line: s/\s+/ /

> @@ -103,13 +103,13 @@ test_expect_success 'modify/delete + directory/file conflict; other way' '
> -       test 5 -eq $(git ls-files -s | wc -l) &&
> -       test 4 -eq $(git ls-files -u | wc -l) &&
> +       test_line_count_cmd --out = 5 git ls-files -s  &&
> +       test_line_count_cmd --out = 4 git ls-files -u  &&

Nit: too many spaces before the `&&` on both lines.

Re: [PATCH 4/4] t6402: use test_line_count_cmd to count # of lines in stdout

[Eric Sunshine]

On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
> diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
> @@ -330,8 +320,8 @@ test_expect_success 'Rename+D/F conflict; renamed file merges but dir in way' '
> -       test 3 -eq "$(git ls-files -u | wc -l)" &&
> -       test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
> +       test_line_count_cmd --out = 3 git ls-files -u  &&
> +       test_line_count_cmd --out = 2 git ls-files -u dir/file-in-the-way  &&

Nit: too many spaces before `&&`: s/\s+/ /

> @@ -357,8 +347,8 @@ test_expect_success 'Same as previous, but merged other way' '
> -       test 3 -eq "$(git ls-files -u | wc -l)" &&
> -       test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
> +       test_line_count_cmd --out = 3 git ls-files -u  &&
> +       test_line_count_cmd --out = 2 git ls-files -u dir/file-in-the-way  &&

Ditto.

> @@ -374,8 +364,8 @@ test_expect_success 'Rename+D/F conflict; renamed file cannot merge, dir not in
> -       test 3 -eq "$(git ls-files -u | wc -l)" &&
> -       test 3 -eq "$(git ls-files -u dir | wc -l)" &&
> +       test_line_count_cmd --out = 3 git ls-files -u  &&
> +       test_line_count_cmd --out = 3 git ls-files -u dir  &&

Ditto.

> @@ -409,14 +399,15 @@ test_expect_success 'Rename+D/F conflict; renamed file cannot merge and dir in t
> -       test 5 -eq "$(git ls-files -u | wc -l)" &&
> +       test_line_count_cmd --out = 5 git ls-files -u  &&
>         if test "$GIT_TEST_MERGE_ALGORITHM" = ort
>         then
> -               test 3 -eq "$(git ls-files -u dir~HEAD | wc -l)"
> +               test_line_count_cmd --out = 3 git ls-files -u dir~HEAD
>         else
> -               test 3 -eq "$(git ls-files -u dir | grep -v file-in-the-way | wc -l)"
> +               git ls-files -u dir >out &&
> +               test_line_count_cmd --out = 3 grep -v file-in-the-way out
>         fi &&
> -       test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
> +       test_line_count_cmd --out = 2 git ls-files -u dir/file-in-the-way  &&

And again.

> @@ -432,14 +423,15 @@ test_expect_success 'Same as previous, but merged other way' '
> -       test 5 -eq "$(git ls-files -u | wc -l)" &&
> +       test_line_count_cmd --out = 5 git ls-files -u  &&

Here too.

> -       test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
> +       test_line_count_cmd --out = 2 git ls-files -u dir/file-in-the-way  &&

Blorp.

And the rest of the changes suffer the same problem, so I won't
continue commenting...

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Đoàn Trần Công Danh]

On 2021-06-12 23:10:19-0400, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh
> <congdanhqx@gmail.com> wrote:
> > In Git project, we have multiple occasions that requires checking number
> > of lines of text in stdout and/or stderr of a command. One of such
> > example is t6400, which checks number of files in various states.
> >
> > Some of those commands are Git command, and we would like to check their
> > exit status.  In some of those checks, we pipe the stdout of those
> > commands to "wc -l" to check for line count, thus loosing the exit status.
> >
> > Introduce a helper function to check for number of lines in stdout and
> > stderr from those commands.
> >
> > This helper will create 2 temporary files in process, thus it may affect
> > output of some checks.
> 
> If the presence of these files is a concern, I wonder if it would make
> sense to turn these into dot-files (leading dot in name) or shove them
> into the .git/ directory? (Not necessarily an actionable comment; just
> tossing around some thoughts.)

The presence of those files is concern to some command likes:

* git ls-files -o; or
* ls -a

Shoving into ".git" would be actionable if we are testing inside a git
repository. Should we testing outside, we don't have that luxury.

I think we can accept that limitation (only allow this helper inside
a Git worktree to avoid surpising behavior).

> 
> > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> > ---
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > @@ -817,6 +817,70 @@ test_line_count () {
> > +# test_line_count_cmd checks the number of lines of captured stdout and/or
> > +# stderr of a command.
> > +#
> > +# NOTE: this helper function will create 2 temporary files named:
> > +# * test_line_count_cmd_.out; and
> > +# * test_line_count_cmd_.err
> > +#
> > +# And this helper function will remove those 2 files if the check is succeed.
> > +# In case of failure, those files will be preserved.
> > +test_line_count_cmd () {
> 
> It would be good to have some usage information in the above comment
> so that people aren't forced to consult the implementation to learn
> what options this function takes. At minimum, it should mention
> `--out`, `--err`, and `!`, and should explain the arguments each
> option takes (even if just through examples).

Make sense!

> 
> > +       local outop outval
> > +       local errop errval
> > +
> > +       while test $# -ge 3
> > +       do
> > +               case "$1" in
> > +               --out)
> > +                       outop="$2"
> > +                       outval="$3"
> > +                       ;;
> > +               --err)
> > +                       errop="$2"
> > +                       errval="$3"
> > +                       ;;
> > +               *)
> > +                       break
> > +                       ;;
> > +               esac
> > +               shift 3
> > +       done &&
> 
> This is really minor, but if test_line_count_cmd() ever learns some
> new option and that option does not consume two arguments, then the
> `shift 3` at the end of the `case/esac` will need to be adjusted in
> some fashion. It might make more sense, therefore, to perform the
> `shift 3` closer to where it is needed (that is, in the `--out` case
> and in the `--err` case) rather than delaying it as is done here. (Not
> necessarily worth a re-roll.)

Make sense, too. I think I'll add another leg here for "-*" to avoid
potential break when adding/removing options. Not that I imagine about
new options, but I can't think of any command starts with "-"

> Another minor comment: Since you're &&-chaining everything else in the
> function, it wouldn't hurt to also do so for the `local` declarations
> and the assignments within each `case` arm, and to chain `esac` with
> `shift 3`. Doing so could help some future programmer who might (for
> some reason) insert code above the `while` loop, thinking that a
> failure in the new code will abort the function, but not realizing
> that the &&-chain is not intact in this area of the code.

Will do, too!

> > +       if test $# = 0 ||
> > +          { test "x$1" = "x!" && test $# = 1 ; }
> > +       then
> > +               BUG "test_line_count_cmd: no command to be run"
> > +       fi &&
> > +       if test -z "$outop$errop"
> > +       then
> > +               BUG "test_line_count_cmd: check which stream?"
> > +       fi &&
> > +
> > +       if test "x$1" = "x!"
> > +       then
> > +               shift &&
> > +               if "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> > +               then
> > +                       echo "error: '$@' succeed!"
> > +                       return 1
> > +               fi
> > +       elif ! "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> > +       then
> > +               echo "error: '$@' run into failure!"
> > +               return 1
> > +       fi &&
> 
> Do we care that the "!" negated case doesn't have the same semantics
> as test_must_fail()? If we do care, then should there be a way to tell
> the function whether we want test_must_fail() semantics or `!`
> semantics (i.e. whether we're running a Git command or a non-Git
> command) or should it infer it on its own? (These are genuine
> questions -- not necessarily requests for changes -- as I'm trying to
> reason through the implications of this implementation choice.)

I think that we shouldn't care, in my mind, I would let users to chain
test_line_count and test_must_fail if they do care about that sematic.
So, we have the freedom to use test_line_count_cmd with both Git and
non-Git commands.  E.g:

---------8<---------------
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ad4746d899..94c8cfc9f2 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -288,9 +288,8 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 '
 
 test_expect_success 'OPT_CALLBACK() and callback errors work' '
-	test_must_fail test-tool parse-options --no-length >output 2>output.err &&
-	test_must_be_empty output &&
-	test_must_be_empty output.err
+	test_line_count_cmd --out = 0 --err = 0 \
+		test_must_fail test-tool parse-options --no-length
 '
 
 cat >expect <<\EOF
------------>8--------------
> 
> > +       if test -n "$outop"
> > +       then
> > +               test_line_count "$outop" "$outval" test_line_count_cmd_.out
> > +       fi &&
> > +       if test -n "$errop"
> > +       then
> > +               test_line_count "$errop" "$errval" test_line_count_cmd_.err
> > +       fi &&
> > +       rm -f test_line_count_cmd_.out test_line_count_cmd_.err
> > +}

-- 
Danh

Re: [PATCH 3/4] t6400: use test_line_count_cmd to count # of lines in stdout

[Đoàn Trần Công Danh]

On 2021-06-12 11:33:49+0700, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> Hi Đoàn,

It's better to call me Danh. Thanks. Don't worry, I was called Dan all
the time, too ;)

> 
> > -	test 5 -eq $(git ls-files -s | wc -l) &&
> > -	test 4 -eq $(git ls-files -u | wc -l) &&
> > +	test_line_count_cmd --out = 5 git ls-files -s &&
> > +	test_line_count_cmd --out = 4 git ls-files -u  &&
> 
> I read lines above as "Formerly I tested that 5/4 should be equal to output
> of git ls-files -s/-u piped to wc -l, now I do the same with
> test_line_count_cmd".
> 
> Am I right?

Yes, you read that right. This series is mostly a cleanup for
linting "test .* -o" (But that regex wouldn't be put to be used
anytime soon).

-- 
Danh

Re: [PATCH 3/4] t6400: use test_line_count_cmd to count # of lines in stdout

[Đoàn Trần Công Danh]

On 2021-06-12 23:39:51-0400, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh
> <congdanhqx@gmail.com> wrote:
> >
> > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> > ---
> > diff --git a/t/t6400-merge-df.sh b/t/t6400-merge-df.sh
> > @@ -82,13 +82,13 @@ test_expect_success 'modify/delete + directory/file conflict' '
> > -       test 5 -eq $(git ls-files -s | wc -l) &&
> > -       test 4 -eq $(git ls-files -u | wc -l) &&
> > +       test_line_count_cmd --out = 5 git ls-files -s &&
> > +       test_line_count_cmd --out = 4 git ls-files -u  &&
> 
> Nit: too many spaces before the `&&` on the second line: s/\s+/ /

Thanks, I'll update them, later.
In my defence, this patch was generated mechanically with:

	:%s/test \([0-9]\+\) -eq "*[$][(]\(.*\)|.*[)]"*/test_line_count_cmd --out = \1 \2/gc

I forgot to include the space after final ")"

> > @@ -103,13 +103,13 @@ test_expect_success 'modify/delete + directory/file conflict; other way' '
> > -       test 5 -eq $(git ls-files -s | wc -l) &&
> > -       test 4 -eq $(git ls-files -u | wc -l) &&
> > +       test_line_count_cmd --out = 5 git ls-files -s  &&
> > +       test_line_count_cmd --out = 4 git ls-files -u  &&
> 
> Nit: too many spaces before the `&&` on both lines.

-- 
Danh

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Ævar Arnfjörð Bjarmason]

On Sat, Jun 12 2021, Đoàn Trần Công Danh wrote:

> +	   { test "x$1" = "x!" && test $# = 1 ; }
> [...]
> +	if test "x$1" = "x!" 

We don't use this test idiom in other places, it's OK to just use "$1" =
"!". I think we're past whatever portability concern made that popular
in some older shell code in the wild.

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Ævar Arnfjörð Bjarmason]

On Sat, Jun 12 2021, Đoàn Trần Công Danh wrote:

> In Git project, we have multiple occasions that requires checking number
> of lines of text in stdout and/or stderr of a command. One of such
> example is t6400, which checks number of files in various states.

Thanks for following up on this.

> Some of those commands are Git command, and we would like to check their
> exit status.  In some of those checks, we pipe the stdout of those
> commands to "wc -l" to check for line count, thus loosing the exit status.
>
> Introduce a helper function to check for number of lines in stdout and
> stderr from those commands.
>
> This helper will create 2 temporary files in process, thus it may affect
> output of some checks.

I think it's fine to just blindly stick the name into a file in the CWD
as long as it's not "actual" or some other such obviously likely to
conflict name.

The convention you've picked here (I'm not sure if it's existing
already) of naming the temp files after the test lib function name is a
good one.

More generally speaking we have a bunch of helpers that have this
potential issue/bug, in practice it's not a big deal.  A test that's
being overly specific and doing a test_cmp on unbounded "find" output or
whatever is likely buggy anyway.

If it ever becomes a bigger issue we can easily set up two scratch
directories during the test, one for the use of the test itself, and one
for the internals of the test run itself.

> +# test_line_count_cmd checks the number of lines of captured stdout and/or
> +# stderr of a command.
> +#
> +# NOTE: this helper function will create 2 temporary files named:
> +# * test_line_count_cmd_.out; and
> +# * test_line_count_cmd_.err
> +#
> +# And this helper function will remove those 2 files if the check is succeed.
> +# In case of failure, those files will be preserved.
> +test_line_count_cmd () {
> +	local outop outval
> +	local errop errval
> +
> +	while test $# -ge 3
> +	do
> +		case "$1" in
> +		--out)
> +			outop="$2"
> +			outval="$3"
> +			;;
> +		--err)
> +			errop="$2"
> +			errval="$3"
> +			;;

It looks like the end-state of the series leaves us with no user of the
--err option; Maybe it's good to have it anyway for completeness, or
just skip it? ...

> +		*)
> +			break
> +			;;
> +		esac
> +		shift 3
> +	done &&
> +	if test $# = 0 ||
> +	   { test "x$1" = "x!" && test $# = 1 ; }
> +	then
> +		BUG "test_line_count_cmd: no command to be run"
> +	fi &&
> +	if test -z "$outop$errop"
> +	then
> +		BUG "test_line_count_cmd: check which stream?"
> +	fi &&
> +
> +	if test "x$1" = "x!" 
> +	then
> +		shift &&
> +		if "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> +		then
> +			echo "error: '$@' succeed!"
> +			return 1
> +		fi
> +	elif ! "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> +	then
> +		echo "error: '$@' run into failure!"
> +		return 1
> +	fi &&

...I think it's better to not pipe to *.err if we haven't requested it,
so under "-v" etc. we can get the stderr.

If we're unifying them I think a better pattern is to only run that "$@"
once, get $?, and then act differently on that in the "!" and ""
cases. It requires less careful reading of the critical function path,
especially with the differing indentation.

> +	if test -n "$outop"
> +	then
> +		test_line_count "$outop" "$outval" test_line_count_cmd_.out
> +	fi &&
> +	if test -n "$errop"
> +	then
> +		test_line_count "$errop" "$errval" test_line_count_cmd_.err
> +	fi &&
> +	rm -f test_line_count_cmd_.out test_line_count_cmd_.err

Let's do that "rm -f" as a "test_when_finished" before we first pipe to
them. It's fine to do that in a test lib function, see e.g. test_config.

We'll get the benefit of preseving these files under -di etc.

> +}
> +
>  test_file_size () {
>  	test "$#" -ne 1 && BUG "1 param"
>  	test-tool path-utils file-size "$1"

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Đoàn Trần Công Danh]

On 2021-06-13 15:28:58+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 
> On Sat, Jun 12 2021, Đoàn Trần Công Danh wrote:
> 
> > +	   { test "x$1" = "x!" && test $# = 1 ; }
> > [...]
> > +	if test "x$1" = "x!" 
> 
> We don't use this test idiom in other places, it's OK to just use "$1" =
> "!". I think we're past whatever portability concern made that popular
> in some older shell code in the wild.

We actually use this test idiom in test_i18ngrep and test_cmp_rev.

I don't know enough to see where should I cut the base line.

-- 
Danh

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Phillip Wood]

On 13/06/2021 14:28, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sat, Jun 12 2021, Đoàn Trần Công Danh wrote:
> 
>> +	   { test "x$1" = "x!" && test $# = 1 ; }
>> [...]
>> +	if test "x$1" = "x!"
> 
> We don't use this test idiom in other places, it's OK to just use "$1" =
> "!". I think we're past whatever portability concern made that popular
> in some older shell code in the wild.

Slightly off topic but if anyone is interested in the history of this 
test idiom and why it is no longer needed there is a good article at 
https://www.vidarholen.net/contents/blog/?p=1035

Best Wishes

Phillip

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Felipe Contreras]

Phillip Wood wrote:
> On 13/06/2021 14:28, Ævar Arnfjörð Bjarmason wrote:
> > 
> > On Sat, Jun 12 2021, Đoàn Trần Công Danh wrote:
> > 
> >> +	   { test "x$1" = "x!" && test $# = 1 ; }
> >> [...]
> >> +	if test "x$1" = "x!"
> > 
> > We don't use this test idiom in other places, it's OK to just use "$1" =
> > "!". I think we're past whatever portability concern made that popular
> > in some older shell code in the wild.
> 
> Slightly off topic but if anyone is interested in the history of this 
> test idiom and why it is no longer needed there is a good article at 
> https://www.vidarholen.net/contents/blog/?p=1035

Very nice! I often wondered why people did that.

-- 
Felipe Contreras

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Eric Sunshine]

On Sun, Jun 13, 2021 at 2:18 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 13/06/2021 14:28, Ævar Arnfjörð Bjarmason wrote:
> > On Sat, Jun 12 2021, Đoàn Trần Công Danh wrote:
> >> +       { test "x$1" = "x!" && test $# = 1 ; }
> >> [...]
> >> +    if test "x$1" = "x!"
> >
> > We don't use this test idiom in other places, it's OK to just use "$1" =
> > "!". I think we're past whatever portability concern made that popular
> > in some older shell code in the wild.
>
> Slightly off topic but if anyone is interested in the history of this
> test idiom and why it is no longer needed there is a good article at
> https://www.vidarholen.net/contents/blog/?p=1035

Thanks for the link to the article; it was an interesting read.
However, the article does seem to say that such idioms and care may
still be warranted. In particular, the epilog gives an example which
is still relevant on macOS today. (Indeed, I just tried it and it does
error out as the article states.) Even discounting macOS, it also
talks about such bugs existing as late as 2015, which isn't long ago
by any stretch. (And, as someone whose primary -- indeed only --
development machine is ten years old, some of the other bugs it
mentions -- which existed as recently as ten years ago -- don't seem
all that long ago either.)

At any rate, for those of us who are old-timers, the `"x$foo"` idiom
is habit and only costs a couple extra characters, so I for one have
no problem with its presence in the proposed patch.

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> ... However, the article does seem to say that such idioms and care may
> still be warranted. In particular, the epilog gives an example which
> is still relevant on macOS today. ...
>
> At any rate, for those of us who are old-timers, the `"x$foo"` idiom
> is habit and only costs a couple extra characters, so I for one have
> no problem with its presence in the proposed patch.

Yes.  I think it is prudent to use the x-prefix idiom, especially
for the case cited upthread like comparing $1 with an exclamation
mark '!'.

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> More generally speaking we have a bunch of helpers that have this
> potential issue/bug, in practice it's not a big deal.  A test that's
> being overly specific and doing a test_cmp on unbounded "find" output or
> whatever is likely buggy anyway.

Unless we are testing ".gitignore", "ls-files -o", or "git status".
Carving out $TRASH_DIRECTORY/.git/trash/ directory to store these
throwaway files would be less error prone from that point of view.

>> +		case "$1" in
>> +		--out)
>> +			outop="$2"
>> +			outval="$3"
>> +			;;
>> +		--err)
>> +			errop="$2"
>> +			errval="$3"
>> +			;;
>
> It looks like the end-state of the series leaves us with no user of the
> --err option; Maybe it's good to have it anyway for completeness, or
> just skip it? ...

I'd rather not to see --out added if we have not yet any use for the
other one.  When the time comes that we want to validate the error
stream, we may find that we want to check _both_, and we'd have to
discard dead code and replace with what we want, instead of just
adding what we want to a working code without dead code.

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Đoàn Trần Công Danh]

On 2021-06-13 15:36:11+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 
> On Sat, Jun 12 2021, Đoàn Trần Công Danh wrote:
> 
> > In Git project, we have multiple occasions that requires checking number
> > of lines of text in stdout and/or stderr of a command. One of such
> > example is t6400, which checks number of files in various states.
> 
> Thanks for following up on this.
> 
> > Some of those commands are Git command, and we would like to check their
> > exit status.  In some of those checks, we pipe the stdout of those
> > commands to "wc -l" to check for line count, thus loosing the exit status.
> >
> > Introduce a helper function to check for number of lines in stdout and
> > stderr from those commands.
> >
> > This helper will create 2 temporary files in process, thus it may affect
> > output of some checks.
> 
> I think it's fine to just blindly stick the name into a file in the CWD
> as long as it's not "actual" or some other such obviously likely to
> conflict name.
> 
> The convention you've picked here (I'm not sure if it's existing
> already) of naming the temp files after the test lib function name is a
> good one.
> 
> More generally speaking we have a bunch of helpers that have this
> potential issue/bug, in practice it's not a big deal.  A test that's
> being overly specific and doing a test_cmp on unbounded "find" output or
> whatever is likely buggy anyway.

I'm going to write into $TRASH_DIRECTORY/.git/trash if .git is-a-dir,
otherwise to $TRASH_DIRECTORY in the next reroll.

> If it ever becomes a bigger issue we can easily set up two scratch
> directories during the test, one for the use of the test itself, and one
> for the internals of the test run itself.
> 
> > +# test_line_count_cmd checks the number of lines of captured stdout and/or
> > +# stderr of a command.
> > +#
> > +# NOTE: this helper function will create 2 temporary files named:
> > +# * test_line_count_cmd_.out; and
> > +# * test_line_count_cmd_.err
> > +#
> > +# And this helper function will remove those 2 files if the check is succeed.
> > +# In case of failure, those files will be preserved.
> > +test_line_count_cmd () {
> > +	local outop outval
> > +	local errop errval
> > +
> > +	while test $# -ge 3
> > +	do
> > +		case "$1" in
> > +		--out)
> > +			outop="$2"
> > +			outval="$3"
> > +			;;
> > +		--err)
> > +			errop="$2"
> > +			errval="$3"
> > +			;;
> 
> It looks like the end-state of the series leaves us with no user of the
> --err option; Maybe it's good to have it anyway for completeness, or
> just skip it? ...

In the re-roll that being prepared, t0041 will be converted, too.
So --err will have a user.

> > +		*)
> > +			break
> > +			;;
> > +		esac
> > +		shift 3
> > +	done &&
> > +	if test $# = 0 ||
> > +	   { test "x$1" = "x!" && test $# = 1 ; }
> > +	then
> > +		BUG "test_line_count_cmd: no command to be run"
> > +	fi &&
> > +	if test -z "$outop$errop"
> > +	then
> > +		BUG "test_line_count_cmd: check which stream?"
> > +	fi &&
> > +
> > +	if test "x$1" = "x!" 
> > +	then
> > +		shift &&
> > +		if "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> > +		then
> > +			echo "error: '$@' succeed!"
> > +			return 1
> > +		fi
> > +	elif ! "$@" >test_line_count_cmd_.out 2>test_line_count_cmd_.err
> > +	then
> > +		echo "error: '$@' run into failure!"
> > +		return 1
> > +	fi &&
> 
> ...I think it's better to not pipe to *.err if we haven't requested it,
> so under "-v" etc. we can get the stderr.

And with t0041 converted, it reveals some difficulty when implementing
this suggestion.

Yes, it would be nice if we can get the stderr under -v,
however, -x will make our life hard, since -x also writes into
/dev/stderr, and below command is broken:

	test_line_count_cmd --out = 0 test_must_fail git for-each-ref --no-contains something 2>actual.err &&
	test_i18ngrep ! usage actual.err

> If we're unifying them I think a better pattern is to only run that "$@"
> once, get $?, and then act differently on that in the "!" and ""
> cases. It requires less careful reading of the critical function path,
> especially with the differing indentation.

This is definitely doable.

This is a replacement patch that still has trouble with "-x",
I guess I should always exec 2>*.err
---------8<--------
Subject: [PATCH] test-lib-functions: introduce test_line_count_cmd

In Git project, we have multiple occasions that requires checking number
of lines of text in stdout and/or stderr of a command. One of such
example is t6400, which checks number of files in various states.

Some of those commands are Git command, and we would like to check their
exit status.  In some of those checks, we pipe the stdout of those
commands to "wc -l" to check for line count, thus loosing the exit status.

Introduce a helper function to check for number of lines in stdout and
stderr from those commands.

This helper will create 2 temporary files in process, thus it may affect
output of some checks.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/test-lib-functions.sh | 116 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f0448daa74..2261de7caa 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -845,6 +845,122 @@ test_line_count () {
 	fi
 }
 
+# test_line_count_cmd checks exit status, and the number of lines in
+# captured stdout and/or stderr of a command.
+#
+# Usage:
+#
+# test_line_count_cmd [--[out|err] <binop> <value>]... [--] [!] cmd [args...]
+#
+# Options:
+# --out <binop> <value>:
+# --err <binop> <value>:
+#	Run sh's "test <# of lines> <binop> <value>" on # of lines in stdout
+#	(for --out) or stderr (for --err)
+# !:
+#	Instead of expecting "cmd [args...]" succeed, expect its failure.
+#	Note, if command under testing is "git", test_must_fail should be used
+#	instead of "!".
+#
+# Example:
+#	test_line_count_cmd --out -ge 10 --err = 0 git tag --no-contains v1.0.0
+#	test_line_count_cmd --out -le 10 ! grep some-text a-file
+#	test_line_count_cmd --out = 0 test_must_fail git rev-parse --verify abcd1234
+#
+# NOTE:
+# * if "--out" is specified, a temporary file named test_line_count_cmd_.out
+#   will be created.
+# * if "--err" is specified, a temporary file named test_line_count_cmd_.err
+#   will be created.
+# Those temporary files will be created under $TRASH_DIRECTORY/.git/trash
+# if $TRASH_DIRECTORY/.git directory existed.
+# Otherwise, they will be created in $TRASH_DIRECTORY.
+# Those temporary files will be cleant by test_when_finished
+test_line_count_cmd () {
+	local outop outval outfile
+	local errop errval errfile
+	local expect_failure actual_failure
+	local trashdir="$TRASH_DIRECTORY"
+
+	if test -d "$TRASH_DIRECTORY/.git"
+	then
+		trashdir="$TRASH_DIRECTORY/.git/trash" &&
+		mkdir -p "$trashdir"
+	fi &&
+	while test $# != 0
+	do
+		case "$1" in
+		--out)
+			outop="$2" &&
+			outval="$3" &&
+			outfile="$trashdir/test_line_count_cmd_.out" &&
+			shift 3
+			;;
+		--err)
+			errop="$2" &&
+			errval="$3" &&
+			errfile="$trashdir/test_line_count_cmd_.err" &&
+			shift 3
+			;;
+		--)
+			shift &&
+			break
+			;;
+		-*)
+			BUG "test_line_count_cmd: unknown options: '$1'"
+			;;
+		*)
+			break
+			;;
+		esac
+	done &&
+	if test "x$1" = "x!"
+	then
+		shift &&
+		expect_failure=yes
+	fi &&
+	if test $# = 0
+	then
+		BUG "test_line_count_cmd: no command to be run"
+	elif test -z "$outop$errop"
+	then
+		BUG "test_line_count_cmd: check which stream?"
+	else
+		if test -n "$outfile"
+		then
+			test_when_finished "rm -f '$outfile'" &&
+			exec 3>"$outfile"
+		fi &&
+		if test -n "$errfile"
+		then
+			test_when_finished "rm -f '$errfile'" &&
+			exec 5>"$errfile"
+		fi &&
+		if ! "$@" >&3 2>&5
+		then
+			actual_failure=yes
+		fi
+	# Don't use &4, it's used for test_must_fail
+	fi 3>&1 5>&2 &&
+	case "$expect_failure,$actual_failure" in
+	yes,)
+		echo "error: '$@' succeed!"
+		return 1
+		;;
+	,yes)
+		echo "error: '$@' run into failure!"
+		return 1
+	esac &&
+	if test -n "$outop"
+	then
+		test_line_count "$outop" "$outval" "$outfile"
+	fi &&
+	if test -n "$errop"
+	then
+		test_line_count "$errop" "$errval" "$errfile"
+	fi
+}
+
 test_file_size () {
 	test "$#" -ne 1 && BUG "1 param"
 	test-tool path-utils file-size "$1"
-- 


-- 
Danh

Re: [PATCH 1/4] test-lib-functions: introduce test_line_count_cmd

[Ævar Arnfjörð Bjarmason]

On Sun, Jun 13 2021, Eric Sunshine wrote:

> On Sun, Jun 13, 2021 at 2:18 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 13/06/2021 14:28, Ævar Arnfjörð Bjarmason wrote:
>> > On Sat, Jun 12 2021, Đoàn Trần Công Danh wrote:
>> >> +       { test "x$1" = "x!" && test $# = 1 ; }
>> >> [...]
>> >> +    if test "x$1" = "x!"
>> >
>> > We don't use this test idiom in other places, it's OK to just use "$1" =
>> > "!". I think we're past whatever portability concern made that popular
>> > in some older shell code in the wild.
>>
>> Slightly off topic but if anyone is interested in the history of this
>> test idiom and why it is no longer needed there is a good article at
>> https://www.vidarholen.net/contents/blog/?p=1035
>
> Thanks for the link to the article; it was an interesting read.
> However, the article does seem to say that such idioms and care may
> still be warranted. In particular, the epilog gives an example which
> is still relevant on macOS today. (Indeed, I just tried it and it does
> error out as the article states.) Even discounting macOS, it also
> talks about such bugs existing as late as 2015, which isn't long ago
> by any stretch. (And, as someone whose primary -- indeed only --
> development machine is ten years old, some of the other bugs it
> mentions -- which existed as recently as ten years ago -- don't seem
> all that long ago either.)

It's only for the case where the first byte is "(" or ")" though, so
e.g. the use of this to compare things like command-line options and
other things that don't start with those characters is portable, if I'm
understanding the article correctly.

> At any rate, for those of us who are old-timers, the `"x$foo"` idiom
> is habit and only costs a couple extra characters, so I for one have
> no problem with its presence in the proposed patch.

Indeed, but it's interesting to dig and see if there's any reason for
such workarounds still.