v2, 5 partst: new helper test_line_count_cmd

This is a series to clear false positive when applying Junio's suggestion to to a series written by Ævar [1].

Change since v1:
* Documentation for test_line_count_cmd has been written in more detail
  with examples
* The outfile and errfile will be created only if --out and/or --err was
  specified for better "-v"
* outfile and errfile will be created in $TRASH_DIRECTORY/.git/trash
  iff $TRASH_DIRECTORY/.git is a directory, otherwise $TRASH_DIRECTORY,
  avoid "git rev-parse --git-dir" because we may want to test it, too.
* Use test_when_finished to clean those files instead of manual "rm -f",
  also for better "-v"
* Merge multiple instance of "$@" run into one, for better auditing
* t0041 is also converted to use new helper
* With the change to location of outfile and errfile,
  output of "git ls-files -o" has been restored.
* Fix double space before "&&" in the end of test command.

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

Đoàn Trần Công Danh (5):
  test-lib-functions: introduce test_line_count_cmd
  t6402: use find(1) builtin to filter instead of grep
  t0041: use test_line_count_cmd to check std{out,err}
  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/t0041-usage.sh        |  53 ++++++----------
 t/t6400-merge-df.sh     |  16 ++---
 t/t6402-merge-rename.sh | 132 +++++++++++++++++++---------------------
 t/test-lib-functions.sh | 117 +++++++++++++++++++++++++++++++++++
 4 files changed, 204 insertions(+), 114 deletions(-)

Range-diff against v1:
1:  bdce5e51ff < -:  ---------- test-lib-functions: introduce test_line_count_cmd
-:  ---------- > 1:  a823312b19 test-lib-functions: introduce test_line_count_cmd
2:  09a440ed25 = 2:  6e8f2d4289 t6402: use find(1) builtin to filter instead of grep
-:  ---------- > 3:  33daa5ee2f t0041: use test_line_count_cmd to check std{out,err}
3:  98a335a442 ! 4:  729ebb8f50 t6400: use test_line_count_cmd to count # of lines in stdout
    @@ t/t6400-merge-df.sh: test_expect_success 'modify/delete + directory/file conflic
     -	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  &&
    ++	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
    ++		test_line_count_cmd --out = 0 git ls-files -o
      	else
     -		test 1 -eq $(git ls-files -o | wc -l)
    -+		test_line_count_cmd --out = 3 git ls-files -o
    ++		test_line_count_cmd --out = 1 git ls-files -o
      	fi &&
      
      	test_path_is_file letters/file &&
    @@ t/t6400-merge-df.sh: test_expect_success 'modify/delete + directory/file conflic
      
     -	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  &&
    ++	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
    ++		test_line_count_cmd --out = 0 git ls-files -o
      	else
     -		test 1 -eq $(git ls-files -o | wc -l)
    -+		test_line_count_cmd --out = 3 git ls-files -o
    ++		test_line_count_cmd --out = 1 git ls-files -o
      	fi &&
      
      	test_path_is_file letters/file &&
4:  69e4a0b6d7 ! 5:  1b450e4148 t6402: use test_line_count_cmd to count # of lines in stdout
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      
     -	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_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 &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Same as previous, but merged other
      
     -	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_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 &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      
     -	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_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 &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      	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  &&
    ++	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)"
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
     +		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_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 &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Same as previous, but merged other
      	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  &&
    ++	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)"
    @@ t/t6402-merge-rename.sh: test_expect_success 'Same as previous, but merged other
     +		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_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 &&
    @@ t/t6402-merge-rename.sh: 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  &&
    ++		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 &&
      
    @@ t/t6402-merge-rename.sh: 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  &&
    ++		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 &&
      
    @@ t/t6402-merge-rename.sh: test_expect_success 'pair rename to parent of other (D/
     -		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 = 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 = 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 &&
      
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -		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
    ++		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 = 0 git ls-files -o
      	else
     -		test 5 -eq "$(git ls-files -s | wc -l)" &&
     -		test 3 -eq "$(git ls-files -u | wc -l)" &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -		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
    ++		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 = 2 git ls-files -o
      	fi &&
      
      	test_path_is_file one/file &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -	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_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 = 0 git ls-files -o &&
      
      	test_path_is_file one &&
      	test_path_is_file two &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'setup merge of rename + small chan
      
     -	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_line_count_cmd --out = 1 git ls-files -s &&
    ++	test_line_count_cmd --out = 0 git ls-files -o &&
      	test $(git rev-parse HEAD:renamed_file) = $(git rev-parse HEAD~1:file)
      '
      

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 | 117 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

Đoàn Trần Công Danh wrote:

In _the_ Git project

multiple instances that require checking the number

One of such examples

that checks the number of files (see 'that' vs. 'which' [1]).

are git commands,

for a line count,

losing

the number of lines

the process

the output

the exit status

the captured stdout...

the command under...

if $TRASH_DIRECTORY/.git/ exists

will be cleaned by ...

Cheers.

[1] https://www.merriam-webster.com/words-at-play/when-to-use-that-and-which

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

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t0041-usage.sh | 53 ++++++++++++++++--------------------------------
 1 file changed, 17 insertions(+), 36 deletions(-)

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Sorry, but I am not impressed if this is a typical/prime example of how the new helper helps in writing our tests.

Notice that so many tests that you touched only care about 0 lines?

Instead of this new helper, I think it would be a more useful improvement if we check the emptyness in a more direct way, i.e.

I think this helper may be misnamed and test_file_is_empty would sit better with test_dir_is_empty and test_file_not_empty that already exist, though.

By the way, my opinion would be quite different if example like this one ...

... were the majority, but I do not think that is the case. Most tests that employ the new test_line_count_cmd in this patch still create either actual or actual.err in the working tree anyway, so I do not see much point in adding this new helper---it is hard to explain to new test writers when to use it.

On 2021-06-16 12:06:14+0900, Junio C Hamano <gitster@pobox.com> wrote:

Yay, reading through the patch again, and I'm become less enthusiastic with persuading --err. The only useful application of --err is t4068. ----8<---

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

I did not see much point in adding test_line_count_cmd with --out and/or --err options; the upside of having it was dubious after looking at the users of it in the patch that we are discussing.

I did not consider test_line_count_cmd that only works on the standard output stream. From the patch under discussion, it is not immediately obvious how much such a simplified helper would help clean up the existing tests, so I have no opinion without seeing at least some sample uses.

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

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

This is a series to clear false positive when applying Junio's suggestion to to a series written by Ævar [1].

Change in v3 since v2:
* --err was dropped entirely
* --out is not an option anymore, <binops> and <value> is the first two
  arguments that fed into test_line_count_cmd

Change in v2 since v1:
* Documentation for test_line_count_cmd has been written in more detail
  with examples
* The outfile and errfile will be created only if --out and/or --err was
  specified for better "-v"
* outfile and errfile will be created in $TRASH_DIRECTORY/.git/trash
  iff $TRASH_DIRECTORY/.git is a directory, otherwise $TRASH_DIRECTORY,
  avoid "git rev-parse --git-dir" because we may want to test it, too.
* Use test_when_finished to clean those files instead of manual "rm -f",
  also for better "-v"
* Merge multiple instance of "$@" run into one, for better auditing
* t0041 is also converted to use new helper
* With the change to location of outfile and errfile,
  output of "git ls-files -o" has been restored.
* Fix double space before "&&" in the end of test command.

1: 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 |  80 ++++++++++++++++++++++++
 3 files changed, 150 insertions(+), 78 deletions(-)

Range-diff against v2:
1:  a823312b19 < -:  ---------- test-lib-functions: introduce test_line_count_cmd
-:  ---------- > 1:  255ba9b067 test-lib-functions: introduce test_line_count_cmd
2:  6e8f2d4289 = 2:  38cd3f93a0 t6402: use find(1) builtin to filter instead of grep
3:  33daa5ee2f < -:  ---------- t0041: use test_line_count_cmd to check std{out,err}
4:  729ebb8f50 ! 3:  efb0905dd3 t6400: use test_line_count_cmd to count # of lines in stdout
    @@ t/t6400-merge-df.sh: test_expect_success 'modify/delete + directory/file conflic
      
     -	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 &&
    ++	test_line_count_cmd = 5 git ls-files -s &&
    ++	test_line_count_cmd = 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 = 0 git ls-files -o
    ++		test_line_count_cmd = 0 git ls-files -o
      	else
     -		test 1 -eq $(git ls-files -o | wc -l)
    -+		test_line_count_cmd --out = 1 git ls-files -o
    ++		test_line_count_cmd = 1 git ls-files -o
      	fi &&
      
      	test_path_is_file letters/file &&
    @@ t/t6400-merge-df.sh: test_expect_success 'modify/delete + directory/file conflic
      
     -	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 &&
    ++	test_line_count_cmd = 5 git ls-files -s &&
    ++	test_line_count_cmd = 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 = 0 git ls-files -o
    ++		test_line_count_cmd = 0 git ls-files -o
      	else
     -		test 1 -eq $(git ls-files -o | wc -l)
    -+		test_line_count_cmd --out = 1 git ls-files -o
    ++		test_line_count_cmd = 1 git ls-files -o
      	fi &&
      
      	test_path_is_file letters/file &&
5:  1b450e4148 ! 4:  8fc0f3ffd2 t6402: use test_line_count_cmd to count # of lines in stdout
    @@ t/t6402-merge-rename.sh: test_expect_success 'pull renaming branch into unrenami
     -	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 &&
    ++	test_line_count_cmd = 3 git ls-files -u B &&
    ++	test_line_count_cmd = 1 git ls-files -s N &&
      	sed -ne "/^g/{
      	p
      	q
    @@ t/t6402-merge-rename.sh: test_expect_success 'pull renaming branch into another
     -	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 &&
    ++	test_line_count_cmd = 3 git ls-files -u B &&
    ++	test_line_count_cmd = 1 git ls-files -s N &&
      	sed -ne "/^g/{
      	p
      	q
    @@ t/t6402-merge-rename.sh: test_expect_success 'pull unrenaming branch into renami
     -	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 &&
    ++	test_line_count_cmd = 3 git ls-files -u B &&
    ++	test_line_count_cmd = 1 git ls-files -s N &&
      	sed -ne "/^g/{
      	p
      	q
    @@ t/t6402-merge-rename.sh: test_expect_success 'pull conflicting renames' \
     -	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 &&
    ++	test_line_count_cmd = 1 git ls-files -u A &&
    ++	test_line_count_cmd = 1 git ls-files -u B &&
    ++	test_line_count_cmd = 1 git ls-files -u C &&
    ++	test_line_count_cmd = 1 git ls-files -s N &&
      	sed -ne "/^g/{
      	p
      	q
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      
     -	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_line_count_cmd = 3 git ls-files -u &&
    ++	test_line_count_cmd = 2 git ls-files -u dir/file-in-the-way &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Same as previous, but merged other
      
     -	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_line_count_cmd = 3 git ls-files -u &&
    ++	test_line_count_cmd = 2 git ls-files -u dir/file-in-the-way &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      
     -	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_line_count_cmd = 3 git ls-files -u &&
    ++	test_line_count_cmd = 3 git ls-files -u dir &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      	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 &&
    ++	test_line_count_cmd = 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
    ++		test_line_count_cmd = 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
    ++		test_line_count_cmd = 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_line_count_cmd = 2 git ls-files -u dir/file-in-the-way &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Same as previous, but merged other
      	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 &&
    ++	test_line_count_cmd = 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
    ++		test_line_count_cmd = 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
    ++		test_line_count_cmd = 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_line_count_cmd = 2 git ls-files -u dir/file-in-the-way &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'both rename source and destination
      	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
    ++		test_line_count_cmd = 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
    ++		test_line_count_cmd = 1 git ls-files -u
      	fi &&
      
      	test_must_fail git diff --quiet &&
    @@ t/t6402-merge-rename.sh: 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 &&
    ++		test_line_count_cmd = 4 git ls-files -u &&
    ++		test_line_count_cmd = 2 git ls-files -u one &&
    ++		test_line_count_cmd = 2 git ls-files -u two &&
      
      		test_must_fail git diff --quiet &&
      
    @@ t/t6402-merge-rename.sh: 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 &&
    ++		test_line_count_cmd = 2 git ls-files -u &&
    ++		test_line_count_cmd = 1 git ls-files -u one &&
    ++		test_line_count_cmd = 1 git ls-files -u two &&
      
      		test_must_fail git diff --quiet &&
      
    @@ t/t6402-merge-rename.sh: test_expect_success 'pair rename to parent of other (D/
     -		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_line_count_cmd = 4 git ls-files -u &&
    ++		test_line_count_cmd = 2 git ls-files -u one &&
    ++		test_line_count_cmd = 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
    ++		test_line_count_cmd = 2 git ls-files -u &&
    ++		test_line_count_cmd = 1 git ls-files -u one &&
    ++		test_line_count_cmd = 1 git ls-files -u two
      	fi &&
      
      	test_must_fail git diff --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -		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 = 0 git ls-files -o
    ++		test_line_count_cmd = 5 git ls-files -s &&
    ++		test_line_count_cmd = 3 git ls-files -u &&
    ++		test_line_count_cmd = 1 git ls-files -u one~HEAD &&
    ++		test_line_count_cmd = 1 git ls-files -u two~second-rename &&
    ++		test_line_count_cmd = 1 git ls-files -u original &&
    ++		test_line_count_cmd = 0 git ls-files -o
      	else
     -		test 5 -eq "$(git ls-files -s | wc -l)" &&
     -		test 3 -eq "$(git ls-files -u | wc -l)" &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -		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 = 2 git ls-files -o
    ++		test_line_count_cmd = 5 git ls-files -s &&
    ++		test_line_count_cmd = 3 git ls-files -u &&
    ++		test_line_count_cmd = 1 git ls-files -u one &&
    ++		test_line_count_cmd = 1 git ls-files -u two &&
    ++		test_line_count_cmd = 1 git ls-files -u original &&
    ++		test_line_count_cmd = 2 git ls-files -o
      	fi &&
      
      	test_path_is_file one/file &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -	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 = 0 git ls-files -o &&
    ++	test_line_count_cmd = 3 git ls-files -u &&
    ++	test_line_count_cmd = 1 git ls-files -u one &&
    ++	test_line_count_cmd = 1 git ls-files -u two &&
    ++	test_line_count_cmd = 1 git ls-files -u original &&
    ++	test_line_count_cmd = 0 git ls-files -o &&
      
      	test_path_is_file one &&
      	test_path_is_file two &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'setup merge of rename + small chan
      
     -	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 = 0 git ls-files -o &&
    ++	test_line_count_cmd = 1 git ls-files -s &&
    ++	test_line_count_cmd = 0 git ls-files -o &&
      	test $(git rev-parse HEAD:renamed_file) = $(git rev-parse HEAD~1:file)
      '
      

On Fri, Jun 18, 2021 at 9:31 PM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

When I read the previous version of this series, I found that many of the instances where test_line_count_cmd() were used became quite noisy, to the point that it was difficult to see at-a-glance the command being tested. As an experiment, on top of your patch 1, I crafted a patch which made `--out` and `--err` optional (defaulting to `--out`). That helped reduce the noise level a good deal, however, I still found it too noisy. Consequently, I crafted a second patch which renamed the function to test_output_count(), and only then did the noise level drop sufficiently that the command being tested didn't entirely disappear into the background.

Since you've dropped the `--out` and `--err` options entirely, I wonder if now would be a good time to shorten the function name, as well, in order to further reduce the noise level. Since it now only tests stdout (and doesn't deal with stderr), a name even shorter than what I tried for the last version might be even better. So, for instance, the name test_out_count() might not be too bad:

    test_out_count = 0 git ls-files -o &&

Eric Sunshine <sunshine@sunshineco.com> writes:

"Test out" to me sound like trying something out and the part "out" in the name no longer hints it is about "output"; you may have shortened the name too much to be meaningful, I am afraid.

Is the helper used to check with anything but equality? Otherwise you can lose "= " to make it shorter.

Having said all that, as an external interface, I wonder

	test_line_count -e = 0 git-ls-files -o

would work better. It usually takes <op> <num> <file>, but when $1 is a magic "-e", we shift it out and it becomes <op> <num> <cmd>...

On Sat, Jun 19, 2021 at 2:17 AM Junio C Hamano <gitster@pobox.com> wrote:

Indeed, I have no problem seeing this as a new mode of test_line_count() triggered by an option. In fact, I suggested exactly that[1] when this idea first arose (except I named the option `-c` rather than `-e`, but the latter is fine). However, my suggestion was pretty much shot down[2] (and I don't entirely disagree with [2], which is why I didn't pursue the idea in [1]).

[1]: https://lore.kernel.org/git/CAPig+cS4tkXZLPDEWgEytzEOCR7oGrXyg1CZVKVPSXuJOifLjQ@mail.gmail.com/ [2]: https://lore.kernel.org/git/xmqq5z0fxlgn.fsf@gitster.g/

Eric Sunshine <sunshine@sunshineco.com> writes:

;-)  

Yeah, I still am skeptical that we'd gain much by hiding the redirection to >actual behind the helper, so as I said in response to the v2 series, I am fine without this new helper or an enhanced test_line_count, but go with more use of test_must_be_empty etc.

On 2021-06-19 15:50:17+0900, Junio C Hamano <gitster@pobox.com> wrote:

I guess the overall feedback for this new helper is negative. I think the consensus here is a local helper in t640{0,2} for counting ls-files?

On Mon, Jun 21, 2021 at 7:52 PM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

A local specialized function makes sense to me.

In the Git project, we have multiple instances that requires checking number of lines of text in the stdout of a command. One of such examples is t6400, that 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 count the number lines, thus losing the exit status.

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

This helper will create a temporary file in the process, thus it may affect the output of some checks.

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

On 19/06/2021 03:30, Đoàn Trần Công Danh wrote:

s/requires/require/

Nit: s/binary-ops/binop/ for consistency with documentation comment
above.  Also, technically the invocation of test_line_count_cmd could be
missing any of its required three parameters, including "cmd".  How
about something similar to the call to BUG in test_i18ngrep:

	BUG "too few parameters to test_line_count_cmd"

?

It seems that function error() could be used here and below instead of "echo >&4".

s/succeed/succeeded/ --- it might be worth borrowing wording from test_must_fail(). Something like:

	error "test_line_count_cmd: command succeeded: '$@'"

Missing ";;" in the last branch of `case`.

On 21/06/2021 11:08, Andrei Rybak wrote:

After spending some time reading t/test-lib-functions.sh, now I don't think that using error() is a good suggestion. Closest relatives of test_line_count_cmd -- test_line_count and test_must_be_empty -- both just use "echo". Other usages of error() in t/test-lib.sh and t/test-lib-functions.sh suggest that error() is not meant to be used for reporting test failure messages, but internal failures. For example:

	error "You haven't built things yet, have you?"

and

	error "Internal error: $stderr disappeared."

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

On 19/06/2021 03:30, Đoàn Trần Công Danh wrote:

Looking at [PATCH v3 1/4] of this series, mention of "stderr" here is no longer relevant.

Seems like a typo s/can/can't/ ?

Because in the original `grep` wasn't invoked with `-F` it means that ".git" is a regex which would match any path which contains the word "git" in it, because "." matches any character, including the leading slash that `find` outputs. Such narrowing of what we intend to filter out is a good change.

This semantic change in filtering doesn't affect tests in t6402, as the test directory doesn't have paths with the word "git" except for the ".git" directory. It might be worth mentioning in the commit message.

On 2021-06-21 10:17:52+0200, Andrei Rybak <rybak.a.v@gmail.com> wrote:

Yes, you're correct.

This is correct, too.

I think the original intention is using "grep -F". I'll add that information into the commit message.

Thanks.

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

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

This is a series to clear false positive when applying Junio's suggestion to to a series written by Ævar [1].

Not that we have any conclusion on that suggestion, just to clear the way out.

In v4, I dropped the test_line_count_cmd completely. A local to t640{0,2} helper was written instead.

Hence, the changelog and range-diff for v2 and v3 is dropped.

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

Đoàn Trần Công Danh (2):
  t6400: preserve git ls-files exit status code
  t6402: preserve git exit status code

 t/t6400-merge-df.sh     |  30 ++++++---
 t/t6402-merge-rename.sh | 146 +++++++++++++++++++++-------------------
 2 files changed, 100 insertions(+), 76 deletions(-)

In t6400, we're checking number of files in the index and the working tree by piping the output of "git ls-files" to "wc -l", thus losing the exit status code of git.

Let's write the output of "git ls-files" to a temporary file, in order to check exit status code of "git ls-files" properly.

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

On Tue, Jun 29, 2021 at 9:57 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

Thanks, the simplicity of this version over the previous attempts is appealing.

Just a few extremely minor style nits below; don't know if any of them are worth a re-roll.

style: funcname () {

I also &&-chain the `local` declaration:

    local ops val &&
    if test "$#" -le 2

By making it easy to see the `&&` upfront, when new code is inserted, there is a better chance that the &&-chain will be kept intact:

    local ops val &&
    my new code &&
    if test "$#" -le 2

A quick grep of the tests indicates that they are consistent about using lowercase for the first word in a BUG():

    BUG "expect 2 or more arguments"

Eric Sunshine <sunshine@sunshineco.com> writes:

Thanks for a pair of sharp eyes, Eric, in your review.

I have one more comment on the main part of the patch. It is easy to see that this conversion is correctly done in this particular patch from the way 5/4 and -s/u are reproduced from the preimage to the postimage, but I doubt that readers in the future, who long have forgotten that the "-s" came from "ls-files -s", would find the new form easy to read and understand.

Do we have the same helper duplicated across two test scripts?

I wonder if it is worth adding a single copy that forces the callers to spell out the command name in test-lib.sh and make the above into something like

	test_output_wc_l = 5 ls-files -s

or even

	test_output_wc_l = 5 git ls-files -s

That way, it is easier to see what command is being run (yes, I know you have _ls_files_ in the middle of the name of the custom helper, but the thing is that "-s" and "_ls_files_" in the middle of the helper are so far apart that it is not immediately obvious what the argument "-s" is about), and by not having two identical copies, we have less risk of them drifting apart.

Hmm?

On Tue, Jun 29, 2021 at 6:49 PM Junio C Hamano <gitster@pobox.com> wrote:

I may be misunderstanding your suggestion, but isn't the proposed test_output_wc_l() function the same as what Danh had originally implemented several re-rolls back (though he named it test_line_count_cmd())?

Eric Sunshine <sunshine@sunshineco.com> writes:

Could be, except that I recall we saw extra noises like --out/--err that weren't used and contaminating the current working directory, which are gone from the latest iteration. The simplification compared to that iteration is quite welcome---it made the resulting code that uses the helper easier to follow compared to the earlier attempts. But this round simplifies it too much and the results got harder to follow by burying the command name in the helper and made it less obvious that the last part of the helper's parameters are arguments given to ls-files, I would think.

On 2021-06-29 20:36:36-0700, Junio C Hamano <gitster@pobox.com> wrote:

Yes, in v{1,2}, there's the extra noises of --out and --err, but it's gone in v3.

I guess you're thinking about the contamination of $PWD iff it's not a git worktree. That could be simplified by BUG-ging if $PWD is not a git worktree.

Maybe, you're thinking about the extra noises to handle the failure run of command under check? That could be dropped, too.

Would you mind looking at v3 1/4 again to see what is your opinion there? I don't mind re-roll a same or simplified version of v3, with s/test_line_count_cmd/test_output_wc_l/ if you see fit.

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

No. I am not thinking about that. I do not think it is a big loss if the helper cannot be used in non-repository.

No.  I am not thinking about that, either.

Let's not go back that far.  This is taken from v4 (t/t6400) ...

	local ops val &&
	if test "$#" -le 2
	then
		BUG "Expect 2 or more arguments"
	fi &&
	ops="$1" &&
	val="$2" &&
	shift 2 &&
	mkdir -p .git/trash &&
	"$@" >.git/trash/output &&
	test_line_count "$ops" "$val" .git/trash/output

... except that it runs '"$@"' instead of 'git ls-files "$@"', so that we could try running things other than ls-files, and that would be mostly good enough. We may want to be prepared for a caller who wants to use the helper from within a subdirectory by not hardcoding ".git/trash", though. Something along the lines of ...

	local ops val &&
+	local trashdir=$(git rev-parse --git-dir)/trash &&
	if test ...
	...
-	mkdir -p .git/trash &&
-	"$@" >".git/trash/output" &&
-	test_line_count "$ops" "$val" .git/trash/output
+	mkdir -p "$trashdir" &&
+	"$@" >"$trashdir/output" &&
+	test_line_count "$ops" "$val" "$trashdir/output"

In t6402, we're checking number of files in the index and the working tree by piping the output of "git ls-files" to "wc -l", thus losing the exit status code of git.

Let's write the output of "git ls-files" to a temporary file, in order to check exit status code of "git ls-files" properly.

While we're at it, also check exit status code of an invocation of git-rev-parse, too.

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

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

I do not think the careless and loose pattern I suggested in the old thread has much value, so any change whose purpose is to reduce false positive from the pattern is not needed.

But if these two patches are genuine improvement for other reasons (like "we avoid feeding output from 'git' into pipe"), they are very much welcome.

Thanks.

This series was started to clear false positive when applying Junio's suggestion to to a series written by Ævar [1].

Despite, we don't have much interest in that direction. I think it's still an improvement with this series, namely, we're able to always check Git's exit status code.

A naive: "git grep -l 'git.* | wc -l' t????-* |wc -l" reveals 38 other tests also have that pattern. Those tests could be cleaned up later on

Change in v5:
- restore (albeit with new name and simplified) test_stdout_line_count
- trash files will be written in $(git rev-parse --git-dir)/trash

In v4, I dropped the test_line_count_cmd completely. A local to t640{0,2} helper was written instead.

Đoàn Trần Công Danh (3):
  test-lib-functions: introduce test_stdout_line_count
  t6400: preserve git ls-files exit status code
  t6402: preserve git exit status code

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

Range-diff against v4:
1:  49104273b8 < -:  ---------- t6400: preserve git ls-files exit status code
-:  ---------- > 1:  ab542ae9aa test-lib-functions: introduce test_stdout_line_count
-:  ---------- > 2:  f7a06994cd t6400: preserve git ls-files exit status code
2:  6d3ebcb255 ! 3:  f8ed19f858 t6402: preserve git exit status code
    @@ Commit message
         t6402: preserve git exit status code
     
         In t6402, we're checking number of files in the index and the working
    -    tree by piping the output of "git ls-files" to "wc -l", thus losing the
    +    tree by piping the output of Git's command to "wc -l", thus losing the
         exit status code of git.
     
    -    Let's write the output of "git ls-files" to a temporary file, in order
    -    to check exit status code of "git ls-files" properly.
    -
    -    While we're at it, also check exit status code of an invocation of
    -    git-rev-parse, too.
    +    Let's use the new helper test_stdout_line_count in order to preserve
    +    Git's exit status code.
     
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
      ## t/t6402-merge-rename.sh ##
    -@@ t/t6402-merge-rename.sh: modify () {
    - 	mv "$2.x" "$2"
    - }
    - 
    -+check_ls_files_count() {
    -+	local ops val
    -+	if test "$#" -le 2
    -+	then
    -+		BUG "Expect 2 or more arguments"
    -+	fi &&
    -+	ops="$1" &&
    -+	val="$2" &&
    -+	shift 2 &&
    -+	mkdir -p .git/trash &&
    -+	git ls-files "$@" >.git/trash/output &&
    -+	test_line_count "$ops" "$val" .git/trash/output
    -+}
    -+
    - test_expect_success 'setup' '
    - 	cat >A <<-\EOF &&
    - 	a aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
     @@ t/t6402-merge-rename.sh: test_expect_success 'pull renaming branch into unrenaming one' \
      	git show-branch &&
      	test_expect_code 1 git pull . white &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'pull renaming branch into unrenami
     -	test_line_count = 3 b.stages &&
     -	git ls-files -s N >n.stages &&
     -	test_line_count = 1 n.stages &&
    -+	check_ls_files_count = 3 -u B &&
    -+	check_ls_files_count = 1 -s N &&
    ++	test_stdout_line_count = 3 git ls-files -u B &&
    ++	test_stdout_line_count = 1 git ls-files -s N &&
      	sed -ne "/^g/{
      	p
      	q
    @@ t/t6402-merge-rename.sh: test_expect_success 'pull renaming branch into another
     -	test_line_count = 3 b.stages &&
     -	git ls-files -s N >n.stages &&
     -	test_line_count = 1 n.stages &&
    -+	check_ls_files_count = 3 -u B &&
    -+	check_ls_files_count = 1 -s N &&
    ++	test_stdout_line_count = 3 git ls-files -u B &&
    ++	test_stdout_line_count = 1 git ls-files -s N &&
      	sed -ne "/^g/{
      	p
      	q
    @@ t/t6402-merge-rename.sh: test_expect_success 'pull unrenaming branch into renami
     -	test_line_count = 3 b.stages &&
     -	git ls-files -s N >n.stages &&
     -	test_line_count = 1 n.stages &&
    -+	check_ls_files_count = 3 -u B &&
    -+	check_ls_files_count = 1 -s N &&
    ++	test_stdout_line_count = 3 git ls-files -u B &&
    ++	test_stdout_line_count = 1 git ls-files -s N &&
      	sed -ne "/^g/{
      	p
      	q
    @@ t/t6402-merge-rename.sh: test_expect_success 'pull conflicting renames' \
     -	test_line_count = 1 c.stages &&
     -	git ls-files -s N >n.stages &&
     -	test_line_count = 1 n.stages &&
    -+	check_ls_files_count = 1 -u A &&
    -+	check_ls_files_count = 1 -u B &&
    -+	check_ls_files_count = 1 -u C &&
    -+	check_ls_files_count = 1 -s N &&
    ++	test_stdout_line_count = 1 git ls-files -u A &&
    ++	test_stdout_line_count = 1 git ls-files -u B &&
    ++	test_stdout_line_count = 1 git ls-files -u C &&
    ++	test_stdout_line_count = 1 git ls-files -s N &&
      	sed -ne "/^g/{
      	p
      	q
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      
     -	test 3 -eq "$(git ls-files -u | wc -l)" &&
     -	test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
    -+	check_ls_files_count = 3 -u &&
    -+	check_ls_files_count = 2 -u dir/file-in-the-way &&
    ++	test_stdout_line_count = 3 git ls-files -u &&
    ++	test_stdout_line_count = 2 git ls-files -u dir/file-in-the-way &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Same as previous, but merged other
      
     -	test 3 -eq "$(git ls-files -u | wc -l)" &&
     -	test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
    -+	check_ls_files_count = 3 -u &&
    -+	check_ls_files_count = 2 -u dir/file-in-the-way &&
    ++	test_stdout_line_count = 3 git ls-files -u &&
    ++	test_stdout_line_count = 2 git ls-files -u dir/file-in-the-way &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      
     -	test 3 -eq "$(git ls-files -u | wc -l)" &&
     -	test 3 -eq "$(git ls-files -u dir | wc -l)" &&
    -+	check_ls_files_count = 3 -u &&
    -+	check_ls_files_count = 3 -u dir &&
    ++	test_stdout_line_count = 3 git ls-files -u &&
    ++	test_stdout_line_count = 3 git ls-files -u dir &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
      	test_must_fail git merge --strategy=recursive dir-in-way &&
      
     -	test 5 -eq "$(git ls-files -u | wc -l)" &&
    -+	check_ls_files_count = 5 -u &&
    ++	test_stdout_line_count = 5 git ls-files -u &&
      	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
      	then
     -		test 3 -eq "$(git ls-files -u dir~HEAD | wc -l)"
    -+		check_ls_files_count = 3 -u dir~HEAD
    ++		test_stdout_line_count = 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 &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Rename+D/F conflict; renamed file
     +		rm -f out
      	fi &&
     -	test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
    -+	check_ls_files_count = 2 -u dir/file-in-the-way &&
    ++	test_stdout_line_count = 2 git ls-files -u dir/file-in-the-way &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Same as previous, but merged other
      	test_must_fail git merge --strategy=recursive renamed-file-has-conflicts &&
      
     -	test 5 -eq "$(git ls-files -u | wc -l)" &&
    -+	check_ls_files_count = 5 -u &&
    ++	test_stdout_line_count = 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)"
    -+		check_ls_files_count = 3 -u dir~renamed-file-has-conflicts
    ++		test_stdout_line_count = 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 &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'Same as previous, but merged other
     +		rm -f out
      	fi &&
     -	test 2 -eq "$(git ls-files -u dir/file-in-the-way | wc -l)" &&
    -+	check_ls_files_count = 2 -u dir/file-in-the-way &&
    ++	test_stdout_line_count = 2 git ls-files -u dir/file-in-the-way &&
      
      	test_must_fail git diff --quiet &&
      	test_must_fail git diff --cached --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'both rename source and destination
      	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
      	then
     -		test 2 -eq "$(git ls-files -u | wc -l)"
    -+		check_ls_files_count = 2 -u
    ++		test_stdout_line_count = 2 git ls-files -u
      	else
     -		test 1 -eq "$(git ls-files -u | wc -l)"
    -+		check_ls_files_count = 1 -u
    ++		test_stdout_line_count = 1 git ls-files -u
      	fi &&
      
      	test_must_fail git diff --quiet &&
    @@ t/t6402-merge-rename.sh: 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)" &&
    -+		check_ls_files_count = 4 -u &&
    -+		check_ls_files_count = 2 -u one &&
    -+		check_ls_files_count = 2 -u two &&
    ++		test_stdout_line_count = 4 git ls-files -u &&
    ++		test_stdout_line_count = 2 git ls-files -u one &&
    ++		test_stdout_line_count = 2 git ls-files -u two &&
      
      		test_must_fail git diff --quiet &&
      
    @@ t/t6402-merge-rename.sh: 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)" &&
    -+		check_ls_files_count = 2 -u &&
    -+		check_ls_files_count = 1 -u one &&
    -+		check_ls_files_count = 1 -u two &&
    ++		test_stdout_line_count = 2 git ls-files -u &&
    ++		test_stdout_line_count = 1 git ls-files -u one &&
    ++		test_stdout_line_count = 1 git ls-files -u two &&
      
      		test_must_fail git diff --quiet &&
      
    @@ t/t6402-merge-rename.sh: test_expect_success 'pair rename to parent of other (D/
     -		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)"
    -+		check_ls_files_count = 4 -u &&
    -+		check_ls_files_count = 2 -u one &&
    -+		check_ls_files_count = 2 -u two
    ++		test_stdout_line_count = 4 git ls-files -u &&
    ++		test_stdout_line_count = 2 git ls-files -u one &&
    ++		test_stdout_line_count = 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)"
    -+		check_ls_files_count = 2 -u &&
    -+		check_ls_files_count = 1 -u one &&
    -+		check_ls_files_count = 1 -u two
    ++		test_stdout_line_count = 2 git ls-files -u &&
    ++		test_stdout_line_count = 1 git ls-files -u one &&
    ++		test_stdout_line_count = 1 git ls-files -u two
      	fi &&
      
      	test_must_fail git diff --quiet &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -		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)"
    -+		check_ls_files_count = 5 -s &&
    -+		check_ls_files_count = 3 -u &&
    -+		check_ls_files_count = 1 -u one~HEAD &&
    -+		check_ls_files_count = 1 -u two~second-rename &&
    -+		check_ls_files_count = 1 -u original &&
    -+		check_ls_files_count = 0 -o
    ++		test_stdout_line_count = 5 git ls-files -s &&
    ++		test_stdout_line_count = 3 git ls-files -u &&
    ++		test_stdout_line_count = 1 git ls-files -u one~HEAD &&
    ++		test_stdout_line_count = 1 git ls-files -u two~second-rename &&
    ++		test_stdout_line_count = 1 git ls-files -u original &&
    ++		test_stdout_line_count = 0 git ls-files -o
      	else
     -		test 5 -eq "$(git ls-files -s | wc -l)" &&
     -		test 3 -eq "$(git ls-files -u | wc -l)" &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -		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)"
    -+		check_ls_files_count = 5 -s &&
    -+		check_ls_files_count = 3 -u &&
    -+		check_ls_files_count = 1 -u one &&
    -+		check_ls_files_count = 1 -u two &&
    -+		check_ls_files_count = 1 -u original &&
    -+		check_ls_files_count = 2 -o
    ++		test_stdout_line_count = 5 git ls-files -s &&
    ++		test_stdout_line_count = 3 git ls-files -u &&
    ++		test_stdout_line_count = 1 git ls-files -u one &&
    ++		test_stdout_line_count = 1 git ls-files -u two &&
    ++		test_stdout_line_count = 1 git ls-files -u original &&
    ++		test_stdout_line_count = 2 git ls-files -o
      	fi &&
      
      	test_path_is_file one/file &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'check handling of differently rena
     -	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)" &&
    -+	check_ls_files_count = 3 -u &&
    -+	check_ls_files_count = 1 -u one &&
    -+	check_ls_files_count = 1 -u two &&
    -+	check_ls_files_count = 1 -u original &&
    -+	check_ls_files_count = 0 -o &&
    ++	test_stdout_line_count = 3 git ls-files -u &&
    ++	test_stdout_line_count = 1 git ls-files -u one &&
    ++	test_stdout_line_count = 1 git ls-files -u two &&
    ++	test_stdout_line_count = 1 git ls-files -u original &&
    ++	test_stdout_line_count = 0 git ls-files -o &&
      
      	test_path_is_file one &&
      	test_path_is_file two &&
    @@ t/t6402-merge-rename.sh: test_expect_success 'setup merge of rename + small chan
     -	test 1 -eq $(git ls-files -s | wc -l) &&
     -	test 0 -eq $(git ls-files -o | wc -l) &&
     -	test $(git rev-parse HEAD:renamed_file) = $(git rev-parse HEAD~1:file)
    -+	check_ls_files_count = 1 -s &&
    -+	check_ls_files_count = 0 -o &&
    ++	test_stdout_line_count = 1 git ls-files -s &&
    ++	test_stdout_line_count = 0 git ls-files -o &&
     +	newhash=$(git rev-parse HEAD:renamed_file) &&
     +	oldhash=$(git rev-parse HEAD~1:file) &&
     +	test $newhash = $oldhash

In some tests, we're checking the number of lines in output of some commands, including but not limited to Git's command.

We're doing the check by running those commands in the left side of a pipe, thus losing the exit status code of those commands. Meanwhile, we really want to check the exit status code of Git's command.

Let's write the output of those commands to a temporary file, and use test_line_count separately in order to check exit status code of those commands properly.

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

On Sun, Jul 4, 2021 at 1:46 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

Nit: one too many blank lines after the test body.

A minor think-out-loud: I wonder if there would be value in deriving the name of the output file from the command being run (perhaps by using `tr` to translate oddball characters to underscore or to fold them out). This might or might not help someone debugging a test failure since there would be less chance of "$trashdir/output" being repeatedly clobbered. Anyhow, it's something that could be done later if deemed useful, not something for the present series. (I'm not interested in seeing this series re-rolled endlessly.)

Eric Sunshine <sunshine@sunshineco.com> writes:

Probably not.

The iterations of output that are clobbered are all from the passing call to test_stdout_count_lines helper we made previously.

On 2021-07-06 12:24:05-0700, Junio C Hamano <gitster@pobox.com> wrote:

Yay, I also think it doesn't add much value. Since we're chaining command in a single test-case.

I think most people with try to debug with "-i", which exits immediately.

The only place it would help is debugging test failures with GitHub Actions, where developers could download artifarts for test failures from GitHub Actions.

In t6400, we're checking number of files in the index and the working tree by piping the output of "git ls-files" to "wc -l", thus losing the exit status code of git.

Let's use the newly introduced test_stdout_line_count in order to check the exit status code of Git's command.

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

In t6402, we're checking number of files in the index and the working tree by piping the output of Git's command to "wc -l", thus losing the exit status code of git.

Let's use the new helper test_stdout_line_count in order to preserve Git's exit status code.

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