threads / patch / 55907

patch, 4 partst: new helper test_line_count_cmd

Subject: [PATCH 0/4] t: new helper test_line_count_cmd

## tl;dr

22 messages between Jun 12, 2021 and Jun 24, 2021. Diffs are folded; open one to read it.

replies: 21people: 7as markdown or json

Đoàn Trần Công Danh· Jun 12, 2021, 04:27 UTC · lore

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
Đoàn Trần Công Danh· Jun 12, 2021, 04:27 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to t/t6400-merge-df.sh +8 −8
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
Bagas Sanjaya· Jun 12, 2021, 04:33 UTC · re: Đoàn Trần Công Danh · lore

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

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
Đoàn Trần Công Danh· Jun 13, 2021, 07:39 UTC · re: Bagas Sanjaya · lore

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

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 ;)

Show 11 quoted lines
> 
> > -	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
Eric Sunshine· Jun 13, 2021, 03:39 UTC · re: Đoàn Trần Công Danh · lore

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

On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

Show 9 quoted lines
>
> 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+/ /
Show 5 quoted lines
> @@ -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.
Đoàn Trần Công Danh· Jun 13, 2021, 07:42 UTC · re: Eric Sunshine · lore

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

On 2021-06-12 23:39:51-0400, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 13 quoted lines
> 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 ")"
Show 7 quoted lines
> > @@ -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
Đoàn Trần Công Danh· Jun 12, 2021, 04:27 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to t/t6402-merge-rename.sh +3 −3
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
Đoàn Trần Công Danh· Jun 12, 2021, 04:27 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to t/t6402-merge-rename.sh +59 −67
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
Eric Sunshine· Jun 13, 2021, 03:43 UTC · re: Đoàn Trần Công Danh · lore

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

On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

Show 8 quoted lines
> 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+/ /
Show 5 quoted lines
> @@ -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.
Show 5 quoted lines
> @@ -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.
Show 14 quoted lines
> @@ -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...

Đoàn Trần Công Danh· Jun 12, 2021, 04:27 UTC · re: Đoàn Trần Công Danh · lore

[PATCH 1/4] 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>
---
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(+)
Show changes to t/test-lib-functions.sh +64 −0
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
Eric Sunshine· Jun 13, 2021, 03:10 UTC · re: Đoàn Trần Công Danh · lore

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

On Sat, Jun 12, 2021 at 12:28 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

Show 13 quoted lines
> 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.)

Show 14 quoted lines
> 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).

Show 20 quoted lines
> +       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.

Show 23 quoted lines
> +       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.)

Show 10 quoted lines
> +       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
> +}
Đoàn Trần Công Danh· Jun 13, 2021, 07:36 UTC · re: Eric Sunshine · lore

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

On 2021-06-12 23:10:19-0400, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 20 quoted lines
> 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).

Show 21 quoted lines
> 
> > 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!
Show 29 quoted lines
> 
> > +       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 "-"

Show 7 quoted lines
> 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!
Show 31 quoted lines
> > +       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<---------------
Show changes to t/t0040-parse-options.sh +2 −3
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
Ævar Arnfjörð Bjarmason· Jun 13, 2021, 13:28 UTC · re: Đoàn Trần Công Danh · lore

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

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.

Đoàn Trần Công Danh· Jun 13, 2021, 16:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 2021-06-13 15:28:58+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 10 quoted lines
> 
> 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
Phillip Wood· Jun 13, 2021, 18:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 13/06/2021 14:28, Ævar Arnfjörð Bjarmason wrote:
Show 10 quoted lines
> 
> 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
Felipe Contreras· Jun 13, 2021, 21:42 UTC · re: Phillip Wood · lore

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

Phillip Wood wrote:
Show 15 quoted lines
> 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
Eric Sunshine· Jun 13, 2021, 23:43 UTC · re: Phillip Wood · lore

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

On Sun, Jun 13, 2021 at 2:18 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 13 quoted lines
> 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.

Junio C Hamano· Jun 14, 2021, 02:56 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 7 quoted lines
> ... 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 '!'.

Ævar Arnfjörð Bjarmason· Jun 24, 2021, 23:19 UTC · re: Eric Sunshine · lore

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

On Sun, Jun 13 2021, Eric Sunshine wrote:
Show 25 quoted lines
> 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.

Ævar Arnfjörð Bjarmason· Jun 13, 2021, 13:36 UTC · re: Đoàn Trần Công Danh · lore

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

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.
Show 9 quoted lines
> 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.

Show 24 quoted lines
> +# 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? ...

Show 29 quoted lines
> +		*)
> +			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.

Show 9 quoted lines
> +	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.
Show 5 quoted lines
> +}
> +
>  test_file_size () {
>  	test "$#" -ne 1 && BUG "1 param"
>  	test-tool path-utils file-size "$1"
Junio C Hamano· Jun 14, 2021, 03:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Æ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.

Show 13 quoted lines
>> +		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.

Đoàn Trần Công Danh· Jun 15, 2021, 15:40 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 2021-06-13 15:36:11+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 31 quoted lines
> 
> 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.

Show 32 quoted lines
> 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.

Show 32 quoted lines
> > +		*)
> > +			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(+)
Show changes to t/test-lib-functions.sh +116 −0
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

← back to recent threads