[PATCH v1 0/7] t: fix unused files, part 1

[PATCH v1 0/7] t: fix unused files, part 1

[Andrei Rybak]

I've noticed that several tests in t9001-send-email.sh don't use the files
created from redirecting output of git commands.  So I wrote a crude script to
find similar issues in other tests:

    from sys import argv
    from sys import exit
    import re
    
    script = argv[1]
    
    filename_pattern = re.compile('([-a-z_A-Z]+|&1)')
    git_with_output_pattern = re.compile('git [^&"]*[ 2][>](?!/)')
    
    while True:
        res = git_with_output_pattern.search(script)
        if res is None:
            break
        filename_index = res.span()[1]
        res = filename_pattern.search(script[filename_index:])
        filename = res.group()
    
        script = script[filename_index + len(filename):]
    
        if filename == '&1':
            continue
    
        read_index = script.find(filename)
        if read_index < 0:
            print("File '" + filename + "' is unused")
            print("Script: ")
            print(script)
            exit(1)
        script = script[read_index + len(filename):]

It doesn't check the tests very throughly and has a lot of false-positives, but
this is enough for now.  I invoke it from test_expect_success() like so:

---- 8< ----
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 999d46fafe..ac2614009d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -836,6 +836,14 @@ test_expect_success () {
 		if test_run_ "$2"
 		then
 			test_ok_ "$1"
+			if ! echo "$1" | grep -q -E '(setup|preparation)'
+			then
+				if ! python3 "$TEST_DIRECTORY/../check_unused_files.py" "$2"
+				then
+					BUG "check_unused_files.py found an unused file in test '$1'"
+					return 1
+				fi
+			fi
 		else
 			test_failure_ "$@"
 		fi
---- >8 ----

Here are the fixes for the issues I've found so far -- I've gone through t0???
and t1???.

Andrei Rybak (7):
  t1005: assert output of ls-files
  t1006: assert error output of cat-file
  t1010: assert empty output of mktree
  t1302: don't create unused file
  t1400: assert output of update-ref
  t1404: don't create unused file
  t1507: assert output of rev-parse

 t/t1005-read-tree-reset.sh    | 15 ++++++++++-----
 t/t1006-cat-file.sh           |  3 ++-
 t/t1010-mktree.sh             |  6 ++++--
 t/t1302-repo-version.sh       |  2 +-
 t/t1400-update-ref.sh         |  3 +++
 t/t1404-update-ref-errors.sh  |  1 -
 t/t1507-rev-parse-upstream.sh |  6 ++++--
 7 files changed, 24 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH v1 1/7] t1005: assert output of ls-files

[Andrei Rybak]

Test 'reset should work' in t1005-read-tree-reset.sh compares two files
"expect" and "actual" to assert the expected output of "git ls-files".
Several other tests in the same file also create files "expect" and
"actual", but don't use them in assertions.

Assert output of "git ls-files" in t1005-read-tree-reset.sh to improve
test coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1005-read-tree-reset.sh | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
index 12e30d77d0..26be4a2b5a 100755
--- a/t/t1005-read-tree-reset.sh
+++ b/t/t1005-read-tree-reset.sh
@@ -41,7 +41,8 @@ test_expect_success 'reset should remove remnants from a failed merge' '
 	git ls-files -s &&
 	read_tree_u_must_succeed --reset -u HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'two-way reset should remove remnants too' '
@@ -56,7 +57,8 @@ test_expect_success 'two-way reset should remove remnants too' '
 	git ls-files -s &&
 	read_tree_u_must_succeed --reset -u HEAD HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain reset should remove remnants too' '
@@ -71,7 +73,8 @@ test_expect_success 'Porcelain reset should remove remnants too' '
 	git ls-files -s &&
 	git reset --hard &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain checkout -f should remove remnants too' '
@@ -86,7 +89,8 @@ test_expect_success 'Porcelain checkout -f should remove remnants too' '
 	git ls-files -s &&
 	git checkout -f &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
@@ -101,7 +105,8 @@ test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
 	git ls-files -s &&
 	git checkout -f HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.39.2

[PATCH v1 1/1] t1507: assert output of rev-parse

[Andrei Rybak]

Tests in t1507-rev-parse-upstream.sh compare files "expect" and "actual"
to assert the output of "git rev-parse", "git show", and "git log".
However, two of the tests '@{reflog}-parsing does not look beyond colon'
and '@{upstream}-parsing does not look beyond colon' don't inspect the
contents of the created files.

Assert output of "git rev-parse" in tests in t1507-rev-parse-upstream.sh
to improve test coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index c34714ffe3..4458820168 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -258,7 +258,8 @@ test_expect_success '@{reflog}-parsing does not look beyond colon' '
 	git add @{yesterday} &&
 	git commit -m "funny reflog file" &&
 	git hash-object @{yesterday} >expect &&
-	git rev-parse HEAD:@{yesterday} >actual
+	git rev-parse HEAD:@{yesterday} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '@{upstream}-parsing does not look beyond colon' '
@@ -266,7 +267,8 @@ test_expect_success '@{upstream}-parsing does not look beyond colon' '
 	git add @{upstream} &&
 	git commit -m "funny upstream file" &&
 	git hash-object @{upstream} >expect &&
-	git rev-parse HEAD:@{upstream} >actual
+	git rev-parse HEAD:@{upstream} >actual &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.39.2

[PATCH v1 2/7] t1006: assert error output of cat-file

[Andrei Rybak]

Test "cat-file $arg1 $arg2 error on missing full OID" in
t1006-cat-file.sh compares files "expect.err" and "err.actual" to assert
the expected error output of "git cat-file".  A similar test in the same
file named "cat-file $arg1 $arg2 error on missing short OID" also
creates these two files, but doesn't use them in assertions.

Assert error output of "git cat-file" in test "cat-file $arg1 $arg2
error on missing short OID" of t1006-cat-file.sh to improve test
coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1006-cat-file.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 2d875b17d8..8eac74b59c 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -603,7 +603,8 @@ do
 			fatal: Not a valid object name $(test_oid deadbeef_short)
 			EOF
 			test_must_fail git cat-file $arg1 $arg2 $(test_oid deadbeef_short) >out 2>err.actual &&
-			test_must_be_empty out
+			test_must_be_empty out &&
+			test_cmp expect.err err.actual
 		'
 
 		test_expect_success "cat-file $arg1 $arg2 error on missing full OID" '
-- 
2.39.2

[PATCH v1 3/7] t1010: assert empty output of mktree

[Andrei Rybak]

Builtin "git mktree" writes the the object name of the tree object built
to the standard output.  Tests 'mktree refuses to read ls-tree -r output
(1)' and 'mktree refuses to read ls-tree -r output (2)' in
"t1010-mktree.sh" redirect output of "git mktree" to a file, but don't
use its contents in assertions.

Assert that the output of "git mktree" is empty when it refuses to build
a tree object.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1010-mktree.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t1010-mktree.sh b/t/t1010-mktree.sh
index 3c08194526..31ea2ec0bb 100755
--- a/t/t1010-mktree.sh
+++ b/t/t1010-mktree.sh
@@ -60,11 +60,13 @@ test_expect_success 'allow missing object with --missing' '
 '
 
 test_expect_success 'mktree refuses to read ls-tree -r output (1)' '
-	test_must_fail git mktree <all >actual
+	test_must_fail git mktree <all >actual &&
+	test_must_be_empty actual
 '
 
 test_expect_success 'mktree refuses to read ls-tree -r output (2)' '
-	test_must_fail git mktree <all.withsub >actual
+	test_must_fail git mktree <all.withsub >actual &&
+	test_must_be_empty actual
 '
 
 test_done
-- 
2.39.2

[PATCH v1 4/7] t1302: don't create unused file

[Andrei Rybak]

Test 'gitdir selection on unsupported repo' in t1302-repo-version.sh
writes output of a "git config" invocation to file "actual".  However,
the test doesn't have any assertions for the file.  The file was used by
this test until commit b9605bc4f2 (config: only read .git/config from
configured repos, 2016-09-12), before which "git config" was expected to
print the bogus value of "core.repositoryformatversion" to standard
output.

Don't redirect output of "git config" to file "actual" in test 'gitdir
selection on unsupported repo'.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1302-repo-version.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 70389fa2eb..179474fa65 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -37,7 +37,7 @@ test_expect_success 'gitdir selection on normal repos' '
 
 test_expect_success 'gitdir selection on unsupported repo' '
 	# Make sure it would stop at test2, not trash
-	test_expect_code 1 git -C test2 config core.repositoryformatversion >actual
+	test_expect_code 1 git -C test2 config core.repositoryformatversion
 '
 
 test_expect_success 'gitdir not required mode' '
-- 
2.39.2

[PATCH v1 5/7] t1400: assert output of update-ref

[Andrei Rybak]

In t1400-update-ref.sh test 'transaction can create and delete' creates
files "expect" and "actual", but doesn't compare them.  Similarly, test
'transaction cannot restart ongoing transaction' redirects output of
"git update-ref" to file "actual", but doesn't check its contents with
any assertions.

Assert output of "git update-ref" in tests to improve test coverage in
t1400-update-ref.sh.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1400-update-ref.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index cf58cf025c..4d66cd7f4a 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1568,6 +1568,7 @@ test_expect_success 'transaction can create and delete' '
 	EOF
 	git update-ref --stdin <stdin >actual &&
 	printf "%s: ok\n" start commit start commit >expect &&
+	test_cmp expect actual &&
 	test_must_fail git show-ref --verify refs/heads/create-and-delete
 '
 
@@ -1595,6 +1596,8 @@ test_expect_success 'transaction cannot restart ongoing transaction' '
 	commit
 	EOF
 	test_must_fail git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start >expect &&
+	test_cmp expect actual &&
 	test_must_fail git show-ref --verify refs/heads/restart
 '
 
-- 
2.39.2

[PATCH v1 6/7] t1404: don't create unused file

[Andrei Rybak]

Some tests in file t1404-update-ref-errors.sh create file "unchanged" as
the expected side for a test_cmp assertion at the end of the test for
output of "git for-each-ref".  The filename conveys the expectation that
the output won't change between two invocations of "git for-each-ref".

Test 'no bogus intermediate values during delete' also creates a file
named "unchanged".  However, in this test the reference is being
deleted, i.e. it _does change_.  The file itself isn't used for any
assertions in the test.

Don't create the unused and slightly misleading file "unchanged".

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1404-update-ref-errors.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index b5606d93b5..937ae0d733 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -551,7 +551,6 @@ test_expect_success REFFILES 'no bogus intermediate values during delete' '
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	git update-ref $prefix/foo $D &&
-	git for-each-ref $prefix >unchanged &&
 	# Now try to update the reference, but hold the `packed-refs` lock
 	# for a while to see what happens while the process is blocked:
 	: >.git/packed-refs.lock &&
-- 
2.39.2

[PATCH v1 7/7] t1507: assert output of rev-parse

[Andrei Rybak]

Tests in t1507-rev-parse-upstream.sh compare files "expect" and "actual"
to assert the output of "git rev-parse", "git show", and "git log".
However, two of the tests '@{reflog}-parsing does not look beyond colon'
and '@{upstream}-parsing does not look beyond colon' don't inspect the
contents of the created files.

Assert output of "git rev-parse" in tests in t1507-rev-parse-upstream.sh
to improve test coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index c34714ffe3..4458820168 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -258,7 +258,8 @@ test_expect_success '@{reflog}-parsing does not look beyond colon' '
 	git add @{yesterday} &&
 	git commit -m "funny reflog file" &&
 	git hash-object @{yesterday} >expect &&
-	git rev-parse HEAD:@{yesterday} >actual
+	git rev-parse HEAD:@{yesterday} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '@{upstream}-parsing does not look beyond colon' '
@@ -266,7 +267,8 @@ test_expect_success '@{upstream}-parsing does not look beyond colon' '
 	git add @{upstream} &&
 	git commit -m "funny upstream file" &&
 	git hash-object @{upstream} >expect &&
-	git rev-parse HEAD:@{upstream} >actual
+	git rev-parse HEAD:@{upstream} >actual &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.39.2

Re: [PATCH v1 1/1] t1507: assert output of rev-parse

[Andrei Rybak]

I've sent this patch twice by accident.  Please ignore this version.

The correct version has subject "[PATCH v1 7/7] t1507: assert output of rev-parse".
https://lore.kernel.org/git/20230312201520.370234-9-rybak.a.v@gmail.com/

Re: [PATCH v1 3/7] t1010: assert empty output of mktree

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

>  test_expect_success 'mktree refuses to read ls-tree -r output (1)' '
> -	test_must_fail git mktree <all >actual
> +	test_must_fail git mktree <all >actual &&
> +	test_must_be_empty actual
>  '
>  
>  test_expect_success 'mktree refuses to read ls-tree -r output (2)' '
> -	test_must_fail git mktree <all.withsub >actual
> +	test_must_fail git mktree <all.withsub >actual &&
> +	test_must_be_empty actual
>  '

I am ambivalent.  As long as a failing command signals its failure
with its non-zero exit status value, the consumer of the output
should not blindly use the output from such a failing command.  Is
there a strong reason why we want users rely on the command to be
silent when it fails?

An obvious alternative is to stop producing "actual" file, and it
might be a better idea; unless there is a good reason why we should
expect the command to be silent, that is.

Thanks.

Re: [PATCH v1 6/7] t1404: don't create unused file

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> Some tests in file t1404-update-ref-errors.sh create file "unchanged" as
> the expected side for a test_cmp assertion at the end of the test for
> output of "git for-each-ref".  The filename conveys the expectation that
> the output won't change between two invocations of "git for-each-ref".
>
> Test 'no bogus intermediate values during delete' also creates a file
> named "unchanged".  However, in this test the reference is being
> deleted, i.e. it _does change_.  The file itself isn't used for any
> assertions in the test.

I think the name "unchanged" is a reference to: the state recorded
in this file is before all the interesting changes done in this
test.

So another for-each-ref, after the "lock, start a process that waits
for and then removes the ref" begins but while the other process is
still waiting, whose output is compared with "unchanged" may have
been another way to perform this test, but we have "it could be $D
that is what we want, and two plausible 'wrong' answers are $C and
undefined" that is sufficient.  So I agree with removing the line
that creates "unchanged".

The other test to the file added by the same commit 6a2a7736 (t1404:
demonstrate two problems with reference transactions, 2017-09-08)
creates the "unchanged" file in the same way, but it does get used
after running "update-ref" that is tested.  I would not be surprised
if the one removed by this patch was created by a cut-and-paste by
mistake.

Thanks.

> Don't create the unused and slightly misleading file "unchanged".
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1404-update-ref-errors.sh | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
> index b5606d93b5..937ae0d733 100755
> --- a/t/t1404-update-ref-errors.sh
> +++ b/t/t1404-update-ref-errors.sh
> @@ -551,7 +551,6 @@ test_expect_success REFFILES 'no bogus intermediate values during delete' '
>  	git update-ref $prefix/foo $C &&
>  	git pack-refs --all &&
>  	git update-ref $prefix/foo $D &&
> -	git for-each-ref $prefix >unchanged &&
>  	# Now try to update the reference, but hold the `packed-refs` lock
>  	# for a while to see what happens while the process is blocked:
>  	: >.git/packed-refs.lock &&

Re: [PATCH v1 0/7] t: fix unused files, part 1

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> Here are the fixes for the issues I've found so far -- I've gone through t0???
> and t1???.

I think it is better not to insist that a failing 'mktree' is
silent, and I think the filename "unchanged" is understandable and
is unfair to call it "misleading" (but the patch itself to remove
the line that creates the unused file makes perfect sense).  Other
than these two small nits, I found everything else in the series
good changes.

Thanks for a pleasant read.

>
> Andrei Rybak (7):
>   t1005: assert output of ls-files
>   t1006: assert error output of cat-file
>   t1010: assert empty output of mktree
>   t1302: don't create unused file
>   t1400: assert output of update-ref
>   t1404: don't create unused file
>   t1507: assert output of rev-parse
>
>  t/t1005-read-tree-reset.sh    | 15 ++++++++++-----
>  t/t1006-cat-file.sh           |  3 ++-
>  t/t1010-mktree.sh             |  6 ++++--
>  t/t1302-repo-version.sh       |  2 +-
>  t/t1400-update-ref.sh         |  3 +++
>  t/t1404-update-ref-errors.sh  |  1 -
>  t/t1507-rev-parse-upstream.sh |  6 ++++--
>  7 files changed, 24 insertions(+), 12 deletions(-)

Re: [PATCH v1 1/7] t1005: assert output of ls-files

[Michael J Gruber]

Andrei Rybak venit, vidit, dixit 2023-03-12 21:15:13:
> Test 'reset should work' in t1005-read-tree-reset.sh compares two files
> "expect" and "actual" to assert the expected output of "git ls-files".
> Several other tests in the same file also create files "expect" and
> "actual", but don't use them in assertions.
> 
> Assert output of "git ls-files" in t1005-read-tree-reset.sh to improve
> test coverage.
> 
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1005-read-tree-reset.sh | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
> index 12e30d77d0..26be4a2b5a 100755
> --- a/t/t1005-read-tree-reset.sh
> +++ b/t/t1005-read-tree-reset.sh
> @@ -41,7 +41,8 @@ test_expect_success 'reset should remove remnants from a failed merge' '
>         git ls-files -s &&
>         read_tree_u_must_succeed --reset -u HEAD &&
>         git ls-files -s >actual &&
> -       ! test -f old
> +       ! test -f old &&
> +       test_cmp expect actual
>  '
>  
>  test_expect_success 'two-way reset should remove remnants too' '
> @@ -56,7 +57,8 @@ test_expect_success 'two-way reset should remove remnants too' '
>         git ls-files -s &&
>         read_tree_u_must_succeed --reset -u HEAD HEAD &&
>         git ls-files -s >actual &&
> -       ! test -f old
> +       ! test -f old &&
> +       test_cmp expect actual
>  '
>  
>  test_expect_success 'Porcelain reset should remove remnants too' '
> @@ -71,7 +73,8 @@ test_expect_success 'Porcelain reset should remove remnants too' '
>         git ls-files -s &&
>         git reset --hard &&
>         git ls-files -s >actual &&
> -       ! test -f old
> +       ! test -f old &&
> +       test_cmp expect actual
>  '
>  
>  test_expect_success 'Porcelain checkout -f should remove remnants too' '
> @@ -86,7 +89,8 @@ test_expect_success 'Porcelain checkout -f should remove remnants too' '
>         git ls-files -s &&
>         git checkout -f &&
>         git ls-files -s >actual &&
> -       ! test -f old
> +       ! test -f old &&
> +       test_cmp expect actual
>  '
>  
>  test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
> @@ -101,7 +105,8 @@ test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
>         git ls-files -s &&
>         git checkout -f HEAD &&
>         git ls-files -s >actual &&
> -       ! test -f old
> +       ! test -f old &&
> +       test_cmp expect actual
>  '
>  
>  test_done
> -- 
> 2.39.2
> 

Just in case someone else was wondering, too:

All these subtests write to `expect` just before the provided context
lines, so there indeed is something to compare to, and it is the output
of `git ls-files -s` before any changes and resets. As a consequence,
these subtests checked only removal of remnants in the woring tree
before the patch, and they check removal of index bits after the patch
(additionally).

Looks fine to me - though one could probably use `git ls-files -s -o` or
such instead, the suggested version is more "explicit".

Michael

Re: [PATCH v1 0/7] t: fix unused files, part 1

[Andrei Rybak]

On 13/03/2023 23:41, Junio C Hamano wrote:
> 
> I think it is better not to insist that a failing 'mktree' is
> silent, and I think the filename "unchanged" is understandable and
> is unfair to call it "misleading" (but the patch itself to remove
> the line that creates the unused file makes perfect sense).  Other
> than these two small nits, I found everything else in the series
> good changes.
> 
> Thanks for a pleasant read.

Thank you for review.  I have limited bandwidth this week, but I'll
try to send a v2 incorporating this feedback on the weekend.

Re: [PATCH v1 1/7] t1005: assert output of ls-files

[Andrei Rybak]

On 14/03/2023 09:51, Michael J Gruber wrote:
> Andrei Rybak venit, vidit, dixit 2023-03-12 21:15:13:
>>   t/t1005-read-tree-reset.sh | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> [...]
> 
> Just in case someone else was wondering, too:
> 
> All these subtests write to `expect` just before the provided context
> lines, so there indeed is something to compare to, and it is the output
> of `git ls-files -s` before any changes and resets. As a consequence,
> these subtests checked only removal of remnants in the woring tree
> before the patch, and they check removal of index bits after the patch
> (additionally).
> 
> Looks fine to me - though one could probably use `git ls-files -s -o` or
> such instead, the suggested version is more "explicit".

Thank you for reviewing this.

None the tests (both in t1005 and other files) use this combination of flags
for `ls-files` right now.  Checked using:

     $ git grep 'ls-files.*[^a-z-][-][os][^|]*[^a-z-][-][so]' || echo none
     none
     $ git grep -E 'ls-files .* --(stage|others)'
     Documentation/git-ls-files.txt:'git ls-files --unmerged' and 'git ls-files --stage' can be used to examine
     contrib/hg-to-git/hg-to-git.py:    os.system('git ls-files -x .hg --others | git update-index --add --stdin')
     t/t1092-sparse-checkout-compatibility.sh:       git -C sparse-index ls-files --sparse --stage >cache &&
     t/t1092-sparse-checkout-compatibility.sh:       git -C sparse-index ls-files --sparse --stage >cache &&
     t/t1306-xdg-files.sh:    git ls-files --exclude-standard --ignored --others >actual) &&

In this topic, I plan to focus on just fixes of unused files and to leave
flags of `git ls-files` as is for now.

[PATCH v2 0/7] t: fix unused files, part 1

[Andrei Rybak]

Here's a v2 for various fixes of unused files in t0??? and t1???.
Original cover letter is at
  https://lore.kernel.org/git/20230312201520.370234-1-rybak.a.v@gmail.com/T/#m63071efd1e2f876fdcbd0c384130b0ec7859f885

Changes since v1 address Junio's review:

  - just don't redirect output of mktree in patch 3/7
  - rewritten commit message of patch 6/7, where I'd completely misunderstood
    the code of the tests.

Range diff:

1:  017f1d8173 = 1:  59a868c0b8 t1005: assert output of ls-files
2:  7a75864e00 = 2:  a3514687ad t1006: assert error output of cat-file
3:  e23a16e560 ! 3:  4cb07fa965 t1010: assert empty output of mktree
    @@ Metadata
     Author: Andrei Rybak <rybak.a.v@gmail.com>
     
      ## Commit message ##
    -    t1010: assert empty output of mktree
    +    t1010: don't create unused files
     
         Builtin "git mktree" writes the the object name of the tree object built
         to the standard output.  Tests 'mktree refuses to read ls-tree -r output
    @@ Commit message
         "t1010-mktree.sh" redirect output of "git mktree" to a file, but don't
         use its contents in assertions.
     
    -    Assert that the output of "git mktree" is empty when it refuses to build
    -    a tree object.
    +    Don't redirect output of "git mktree" to file "actual" in tests that
    +    assert that an invocation of "git mktree" must fail.
    +
    +    Output of "git mktree" is empty when it refuses to build a tree object.
    +    So, alternatively, the test could assert that the output is empty.
    +    However, there isn't a good reason for the user to expect the command to
    +    be silent in such cases, so we shouldn't enforce it.  The user shouldn't
    +    use the output of a failing command anyway.
     
         Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
     
    @@ t/t1010-mktree.sh: test_expect_success 'allow missing object with --missing' '
      
      test_expect_success 'mktree refuses to read ls-tree -r output (1)' '
     -  test_must_fail git mktree <all >actual
    -+  test_must_fail git mktree <all >actual &&
    -+  test_must_be_empty actual
    ++  test_must_fail git mktree <all
      '
      
      test_expect_success 'mktree refuses to read ls-tree -r output (2)' '
     -  test_must_fail git mktree <all.withsub >actual
    -+  test_must_fail git mktree <all.withsub >actual &&
    -+  test_must_be_empty actual
    ++  test_must_fail git mktree <all.withsub
      '
      
      test_done
4:  462cfa7025 = 4:  1ed6030a4f t1302: don't create unused file
5:  9fa04e479c = 5:  002942d81c t1400: assert output of update-ref
6:  e79566cc32 ! 6:  2e3446fc2f t1404: don't create unused file
    @@ Commit message
     
         Some tests in file t1404-update-ref-errors.sh create file "unchanged" as
         the expected side for a test_cmp assertion at the end of the test for
    -    output of "git for-each-ref".  The filename conveys the expectation that
    -    the output won't change between two invocations of "git for-each-ref".
    +    output of "git for-each-ref".  Test 'no bogus intermediate values during
    +    delete' also creates a file named "unchanged" using "git for-each-ref".
    +    However, the file isn't used for any assertions in the test.  Instead,
    +    "git rev-parse" is used to compare the reference with variable $D.
     
    -    Test 'no bogus intermediate values during delete' also creates a file
    -    named "unchanged".  However, in this test the reference is being
    -    deleted, i.e. it _does change_.  The file itself isn't used for any
    -    assertions in the test.
    -
    -    Don't create the unused and slightly misleading file "unchanged".
    +    Don't create unused file "unchanged" in test 'no bogus intermediate
    +    values during delete' of t1404-update-ref-errors.sh.
     
         Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
     
7:  5a7df840a8 = 7:  e11a7d8e02 t1507: assert output of rev-parse


Andrei Rybak (7):
  t1005: assert output of ls-files
  t1006: assert error output of cat-file
  t1010: don't create unused files
  t1302: don't create unused file
  t1400: assert output of update-ref
  t1404: don't create unused file
  t1507: assert output of rev-parse

 t/t1005-read-tree-reset.sh    | 15 ++++++++++-----
 t/t1006-cat-file.sh           |  3 ++-
 t/t1010-mktree.sh             |  4 ++--
 t/t1302-repo-version.sh       |  2 +-
 t/t1400-update-ref.sh         |  3 +++
 t/t1404-update-ref-errors.sh  |  1 -
 t/t1507-rev-parse-upstream.sh |  6 ++++--
 7 files changed, 22 insertions(+), 12 deletions(-)

-- 
2.40.0

[PATCH v2 1/7] t1005: assert output of ls-files

[Andrei Rybak]

Test 'reset should work' in t1005-read-tree-reset.sh compares two files
"expect" and "actual" to assert the expected output of "git ls-files".
Several other tests in the same file also create files "expect" and
"actual", but don't use them in assertions.

Assert output of "git ls-files" in t1005-read-tree-reset.sh to improve
test coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1005-read-tree-reset.sh | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
index 12e30d77d0..26be4a2b5a 100755
--- a/t/t1005-read-tree-reset.sh
+++ b/t/t1005-read-tree-reset.sh
@@ -41,7 +41,8 @@ test_expect_success 'reset should remove remnants from a failed merge' '
 	git ls-files -s &&
 	read_tree_u_must_succeed --reset -u HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'two-way reset should remove remnants too' '
@@ -56,7 +57,8 @@ test_expect_success 'two-way reset should remove remnants too' '
 	git ls-files -s &&
 	read_tree_u_must_succeed --reset -u HEAD HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain reset should remove remnants too' '
@@ -71,7 +73,8 @@ test_expect_success 'Porcelain reset should remove remnants too' '
 	git ls-files -s &&
 	git reset --hard &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain checkout -f should remove remnants too' '
@@ -86,7 +89,8 @@ test_expect_success 'Porcelain checkout -f should remove remnants too' '
 	git ls-files -s &&
 	git checkout -f &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
@@ -101,7 +105,8 @@ test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
 	git ls-files -s &&
 	git checkout -f HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.40.0

[PATCH v2 2/7] t1006: assert error output of cat-file

[Andrei Rybak]

Test "cat-file $arg1 $arg2 error on missing full OID" in
t1006-cat-file.sh compares files "expect.err" and "err.actual" to assert
the expected error output of "git cat-file".  A similar test in the same
file named "cat-file $arg1 $arg2 error on missing short OID" also
creates these two files, but doesn't use them in assertions.

Assert error output of "git cat-file" in test "cat-file $arg1 $arg2
error on missing short OID" of t1006-cat-file.sh to improve test
coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1006-cat-file.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 2d875b17d8..8eac74b59c 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -603,7 +603,8 @@ do
 			fatal: Not a valid object name $(test_oid deadbeef_short)
 			EOF
 			test_must_fail git cat-file $arg1 $arg2 $(test_oid deadbeef_short) >out 2>err.actual &&
-			test_must_be_empty out
+			test_must_be_empty out &&
+			test_cmp expect.err err.actual
 		'
 
 		test_expect_success "cat-file $arg1 $arg2 error on missing full OID" '
-- 
2.40.0

[PATCH v2 3/7] t1010: don't create unused files

[Andrei Rybak]

Builtin "git mktree" writes the the object name of the tree object built
to the standard output.  Tests 'mktree refuses to read ls-tree -r output
(1)' and 'mktree refuses to read ls-tree -r output (2)' in
"t1010-mktree.sh" redirect output of "git mktree" to a file, but don't
use its contents in assertions.

Don't redirect output of "git mktree" to file "actual" in tests that
assert that an invocation of "git mktree" must fail.

Output of "git mktree" is empty when it refuses to build a tree object.
So, alternatively, the test could assert that the output is empty.
However, there isn't a good reason for the user to expect the command to
be silent in such cases, so we shouldn't enforce it.  The user shouldn't
use the output of a failing command anyway.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1010-mktree.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1010-mktree.sh b/t/t1010-mktree.sh
index 3c08194526..22875ba598 100755
--- a/t/t1010-mktree.sh
+++ b/t/t1010-mktree.sh
@@ -60,11 +60,11 @@ test_expect_success 'allow missing object with --missing' '
 '
 
 test_expect_success 'mktree refuses to read ls-tree -r output (1)' '
-	test_must_fail git mktree <all >actual
+	test_must_fail git mktree <all
 '
 
 test_expect_success 'mktree refuses to read ls-tree -r output (2)' '
-	test_must_fail git mktree <all.withsub >actual
+	test_must_fail git mktree <all.withsub
 '
 
 test_done
-- 
2.40.0

[PATCH v2 4/7] t1302: don't create unused file

[Andrei Rybak]

Test 'gitdir selection on unsupported repo' in t1302-repo-version.sh
writes output of a "git config" invocation to file "actual".  However,
the test doesn't have any assertions for the file.  The file was used by
this test until commit b9605bc4f2 (config: only read .git/config from
configured repos, 2016-09-12), before which "git config" was expected to
print the bogus value of "core.repositoryformatversion" to standard
output.

Don't redirect output of "git config" to file "actual" in test 'gitdir
selection on unsupported repo'.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1302-repo-version.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 70389fa2eb..179474fa65 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -37,7 +37,7 @@ test_expect_success 'gitdir selection on normal repos' '
 
 test_expect_success 'gitdir selection on unsupported repo' '
 	# Make sure it would stop at test2, not trash
-	test_expect_code 1 git -C test2 config core.repositoryformatversion >actual
+	test_expect_code 1 git -C test2 config core.repositoryformatversion
 '
 
 test_expect_success 'gitdir not required mode' '
-- 
2.40.0

[PATCH v2 5/7] t1400: assert output of update-ref

[Andrei Rybak]

In t1400-update-ref.sh test 'transaction can create and delete' creates
files "expect" and "actual", but doesn't compare them.  Similarly, test
'transaction cannot restart ongoing transaction' redirects output of
"git update-ref" to file "actual", but doesn't check its contents with
any assertions.

Assert output of "git update-ref" in tests to improve test coverage in
t1400-update-ref.sh.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1400-update-ref.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index cf58cf025c..4d66cd7f4a 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1568,6 +1568,7 @@ test_expect_success 'transaction can create and delete' '
 	EOF
 	git update-ref --stdin <stdin >actual &&
 	printf "%s: ok\n" start commit start commit >expect &&
+	test_cmp expect actual &&
 	test_must_fail git show-ref --verify refs/heads/create-and-delete
 '
 
@@ -1595,6 +1596,8 @@ test_expect_success 'transaction cannot restart ongoing transaction' '
 	commit
 	EOF
 	test_must_fail git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start >expect &&
+	test_cmp expect actual &&
 	test_must_fail git show-ref --verify refs/heads/restart
 '
 
-- 
2.40.0

[PATCH v2 6/7] t1404: don't create unused file

[Andrei Rybak]

Some tests in file t1404-update-ref-errors.sh create file "unchanged" as
the expected side for a test_cmp assertion at the end of the test for
output of "git for-each-ref".  Test 'no bogus intermediate values during
delete' also creates a file named "unchanged" using "git for-each-ref".
However, the file isn't used for any assertions in the test.  Instead,
"git rev-parse" is used to compare the reference with variable $D.

Don't create unused file "unchanged" in test 'no bogus intermediate
values during delete' of t1404-update-ref-errors.sh.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1404-update-ref-errors.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index b5606d93b5..937ae0d733 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -551,7 +551,6 @@ test_expect_success REFFILES 'no bogus intermediate values during delete' '
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	git update-ref $prefix/foo $D &&
-	git for-each-ref $prefix >unchanged &&
 	# Now try to update the reference, but hold the `packed-refs` lock
 	# for a while to see what happens while the process is blocked:
 	: >.git/packed-refs.lock &&
-- 
2.40.0

[PATCH v2 7/7] t1507: assert output of rev-parse

[Andrei Rybak]

Tests in t1507-rev-parse-upstream.sh compare files "expect" and "actual"
to assert the output of "git rev-parse", "git show", and "git log".
However, two of the tests '@{reflog}-parsing does not look beyond colon'
and '@{upstream}-parsing does not look beyond colon' don't inspect the
contents of the created files.

Assert output of "git rev-parse" in tests in t1507-rev-parse-upstream.sh
to improve test coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index c34714ffe3..4458820168 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -258,7 +258,8 @@ test_expect_success '@{reflog}-parsing does not look beyond colon' '
 	git add @{yesterday} &&
 	git commit -m "funny reflog file" &&
 	git hash-object @{yesterday} >expect &&
-	git rev-parse HEAD:@{yesterday} >actual
+	git rev-parse HEAD:@{yesterday} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '@{upstream}-parsing does not look beyond colon' '
@@ -266,7 +267,8 @@ test_expect_success '@{upstream}-parsing does not look beyond colon' '
 	git add @{upstream} &&
 	git commit -m "funny upstream file" &&
 	git hash-object @{upstream} >expect &&
-	git rev-parse HEAD:@{upstream} >actual
+	git rev-parse HEAD:@{upstream} >actual &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.40.0

[PATCH v3 0/7] t: fix unused files, part 1

[Andrei Rybak]

I messed up when sending v2 (incorrectly used format-patch with --in-reply-to),
and v2 didn't get any response.  So here's v3 which is just a resend of v2.

Original cover letter is at
  https://lore.kernel.org/git/20230312201520.370234-1-rybak.a.v@gmail.com/T/#m63071efd1e2f876fdcbd0c384130b0ec7859f885

Changes since v1 address Junio's review:

  - just don't redirect output of mktree in patch 3/7
    Cf. https://lore.kernel.org/git/20230312201520.370234-1-rybak.a.v@gmail.com/T/#m0c7b88dcf0af59f65262c00f30dee353170c3b2e
  - rewritten commit message of patch 6/7, where I'd completely misunderstood
    the code of the tests.
    Cf. https://lore.kernel.org/git/20230312201520.370234-1-rybak.a.v@gmail.com/T/#m65f0666a8e94a1d44434dab58ee7002491b13df0

Range diff:

1:  017f1d8173 = 1:  59a868c0b8 t1005: assert output of ls-files
2:  7a75864e00 = 2:  a3514687ad t1006: assert error output of cat-file
3:  e23a16e560 ! 3:  4cb07fa965 t1010: assert empty output of mktree
    @@ Metadata
     Author: Andrei Rybak <rybak.a.v@gmail.com>

      ## Commit message ##
    -    t1010: assert empty output of mktree
    +    t1010: don't create unused files

         Builtin "git mktree" writes the the object name of the tree object built
         to the standard output.  Tests 'mktree refuses to read ls-tree -r output
    @@ Commit message
         "t1010-mktree.sh" redirect output of "git mktree" to a file, but don't
         use its contents in assertions.

    -    Assert that the output of "git mktree" is empty when it refuses to build
    -    a tree object.
    +    Don't redirect output of "git mktree" to file "actual" in tests that
    +    assert that an invocation of "git mktree" must fail.
    +
    +    Output of "git mktree" is empty when it refuses to build a tree object.
    +    So, alternatively, the test could assert that the output is empty.
    +    However, there isn't a good reason for the user to expect the command to
    +    be silent in such cases, so we shouldn't enforce it.  The user shouldn't
    +    use the output of a failing command anyway.

         Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>

    @@ t/t1010-mktree.sh: test_expect_success 'allow missing object with --missing' '

      test_expect_success 'mktree refuses to read ls-tree -r output (1)' '
     -  test_must_fail git mktree <all >actual
    -+  test_must_fail git mktree <all >actual &&
    -+  test_must_be_empty actual
    ++  test_must_fail git mktree <all
      '

      test_expect_success 'mktree refuses to read ls-tree -r output (2)' '
     -  test_must_fail git mktree <all.withsub >actual
    -+  test_must_fail git mktree <all.withsub >actual &&
    -+  test_must_be_empty actual
    ++  test_must_fail git mktree <all.withsub
      '

      test_done
4:  462cfa7025 = 4:  1ed6030a4f t1302: don't create unused file
5:  9fa04e479c = 5:  002942d81c t1400: assert output of update-ref
6:  e79566cc32 ! 6:  2e3446fc2f t1404: don't create unused file
    @@ Commit message

         Some tests in file t1404-update-ref-errors.sh create file "unchanged" as
         the expected side for a test_cmp assertion at the end of the test for
    -    output of "git for-each-ref".  The filename conveys the expectation that
    -    the output won't change between two invocations of "git for-each-ref".
    +    output of "git for-each-ref".  Test 'no bogus intermediate values during
    +    delete' also creates a file named "unchanged" using "git for-each-ref".
    +    However, the file isn't used for any assertions in the test.  Instead,
    +    "git rev-parse" is used to compare the reference with variable $D.

    -    Test 'no bogus intermediate values during delete' also creates a file
    -    named "unchanged".  However, in this test the reference is being
    -    deleted, i.e. it _does change_.  The file itself isn't used for any
    -    assertions in the test.
    -
    -    Don't create the unused and slightly misleading file "unchanged".
    +    Don't create unused file "unchanged" in test 'no bogus intermediate
    +    values during delete' of t1404-update-ref-errors.sh.

         Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>

7:  5a7df840a8 = 7:  e11a7d8e02 t1507: assert output of rev-parse

Andrei Rybak (7):
  t1005: assert output of ls-files
  t1006: assert error output of cat-file
  t1010: don't create unused files
  t1302: don't create unused file
  t1400: assert output of update-ref
  t1404: don't create unused file
  t1507: assert output of rev-parse

 t/t1005-read-tree-reset.sh    | 15 ++++++++++-----
 t/t1006-cat-file.sh           |  3 ++-
 t/t1010-mktree.sh             |  4 ++--
 t/t1302-repo-version.sh       |  2 +-
 t/t1400-update-ref.sh         |  3 +++
 t/t1404-update-ref-errors.sh  |  1 -
 t/t1507-rev-parse-upstream.sh |  6 ++++--
 7 files changed, 22 insertions(+), 12 deletions(-)

-- 
2.40.0

[PATCH v3 1/7] t1005: assert output of ls-files

[Andrei Rybak]

Test 'reset should work' in t1005-read-tree-reset.sh compares two files
"expect" and "actual" to assert the expected output of "git ls-files".
Several other tests in the same file also create files "expect" and
"actual", but don't use them in assertions.

Assert output of "git ls-files" in t1005-read-tree-reset.sh to improve
test coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1005-read-tree-reset.sh | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
index 12e30d77d0..26be4a2b5a 100755
--- a/t/t1005-read-tree-reset.sh
+++ b/t/t1005-read-tree-reset.sh
@@ -41,7 +41,8 @@ test_expect_success 'reset should remove remnants from a failed merge' '
 	git ls-files -s &&
 	read_tree_u_must_succeed --reset -u HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'two-way reset should remove remnants too' '
@@ -56,7 +57,8 @@ test_expect_success 'two-way reset should remove remnants too' '
 	git ls-files -s &&
 	read_tree_u_must_succeed --reset -u HEAD HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain reset should remove remnants too' '
@@ -71,7 +73,8 @@ test_expect_success 'Porcelain reset should remove remnants too' '
 	git ls-files -s &&
 	git reset --hard &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain checkout -f should remove remnants too' '
@@ -86,7 +89,8 @@ test_expect_success 'Porcelain checkout -f should remove remnants too' '
 	git ls-files -s &&
 	git checkout -f &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
@@ -101,7 +105,8 @@ test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
 	git ls-files -s &&
 	git checkout -f HEAD &&
 	git ls-files -s >actual &&
-	! test -f old
+	! test -f old &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.40.0

[PATCH v3 2/7] t1006: assert error output of cat-file

[Andrei Rybak]

Test "cat-file $arg1 $arg2 error on missing full OID" in
t1006-cat-file.sh compares files "expect.err" and "err.actual" to assert
the expected error output of "git cat-file".  A similar test in the same
file named "cat-file $arg1 $arg2 error on missing short OID" also
creates these two files, but doesn't use them in assertions.

Assert error output of "git cat-file" in test "cat-file $arg1 $arg2
error on missing short OID" of t1006-cat-file.sh to improve test
coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1006-cat-file.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 2d875b17d8..8eac74b59c 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -603,7 +603,8 @@ do
 			fatal: Not a valid object name $(test_oid deadbeef_short)
 			EOF
 			test_must_fail git cat-file $arg1 $arg2 $(test_oid deadbeef_short) >out 2>err.actual &&
-			test_must_be_empty out
+			test_must_be_empty out &&
+			test_cmp expect.err err.actual
 		'
 
 		test_expect_success "cat-file $arg1 $arg2 error on missing full OID" '
-- 
2.40.0

[PATCH v3 3/7] t1010: don't create unused files

[Andrei Rybak]

Builtin "git mktree" writes the the object name of the tree object built
to the standard output.  Tests 'mktree refuses to read ls-tree -r output
(1)' and 'mktree refuses to read ls-tree -r output (2)' in
"t1010-mktree.sh" redirect output of "git mktree" to a file, but don't
use its contents in assertions.

Don't redirect output of "git mktree" to file "actual" in tests that
assert that an invocation of "git mktree" must fail.

Output of "git mktree" is empty when it refuses to build a tree object.
So, alternatively, the test could assert that the output is empty.
However, there isn't a good reason for the user to expect the command to
be silent in such cases, so we shouldn't enforce it.  The user shouldn't
use the output of a failing command anyway.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1010-mktree.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1010-mktree.sh b/t/t1010-mktree.sh
index 3c08194526..22875ba598 100755
--- a/t/t1010-mktree.sh
+++ b/t/t1010-mktree.sh
@@ -60,11 +60,11 @@ test_expect_success 'allow missing object with --missing' '
 '
 
 test_expect_success 'mktree refuses to read ls-tree -r output (1)' '
-	test_must_fail git mktree <all >actual
+	test_must_fail git mktree <all
 '
 
 test_expect_success 'mktree refuses to read ls-tree -r output (2)' '
-	test_must_fail git mktree <all.withsub >actual
+	test_must_fail git mktree <all.withsub
 '
 
 test_done
-- 
2.40.0

[PATCH v3 4/7] t1302: don't create unused file

[Andrei Rybak]

Test 'gitdir selection on unsupported repo' in t1302-repo-version.sh
writes output of a "git config" invocation to file "actual".  However,
the test doesn't have any assertions for the file.  The file was used by
this test until commit b9605bc4f2 (config: only read .git/config from
configured repos, 2016-09-12), before which "git config" was expected to
print the bogus value of "core.repositoryformatversion" to standard
output.

Don't redirect output of "git config" to file "actual" in test 'gitdir
selection on unsupported repo'.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1302-repo-version.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 70389fa2eb..179474fa65 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -37,7 +37,7 @@ test_expect_success 'gitdir selection on normal repos' '
 
 test_expect_success 'gitdir selection on unsupported repo' '
 	# Make sure it would stop at test2, not trash
-	test_expect_code 1 git -C test2 config core.repositoryformatversion >actual
+	test_expect_code 1 git -C test2 config core.repositoryformatversion
 '
 
 test_expect_success 'gitdir not required mode' '
-- 
2.40.0

[PATCH v3 5/7] t1400: assert output of update-ref

[Andrei Rybak]

In t1400-update-ref.sh test 'transaction can create and delete' creates
files "expect" and "actual", but doesn't compare them.  Similarly, test
'transaction cannot restart ongoing transaction' redirects output of
"git update-ref" to file "actual", but doesn't check its contents with
any assertions.

Assert output of "git update-ref" in tests to improve test coverage in
t1400-update-ref.sh.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1400-update-ref.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index cf58cf025c..4d66cd7f4a 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1568,6 +1568,7 @@ test_expect_success 'transaction can create and delete' '
 	EOF
 	git update-ref --stdin <stdin >actual &&
 	printf "%s: ok\n" start commit start commit >expect &&
+	test_cmp expect actual &&
 	test_must_fail git show-ref --verify refs/heads/create-and-delete
 '
 
@@ -1595,6 +1596,8 @@ test_expect_success 'transaction cannot restart ongoing transaction' '
 	commit
 	EOF
 	test_must_fail git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start >expect &&
+	test_cmp expect actual &&
 	test_must_fail git show-ref --verify refs/heads/restart
 '
 
-- 
2.40.0

[PATCH v3 6/7] t1404: don't create unused file

[Andrei Rybak]

Some tests in file t1404-update-ref-errors.sh create file "unchanged" as
the expected side for a test_cmp assertion at the end of the test for
output of "git for-each-ref".  Test 'no bogus intermediate values during
delete' also creates a file named "unchanged" using "git for-each-ref".
However, the file isn't used for any assertions in the test.  Instead,
"git rev-parse" is used to compare the reference with variable $D.

Don't create unused file "unchanged" in test 'no bogus intermediate
values during delete' of t1404-update-ref-errors.sh.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1404-update-ref-errors.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index b5606d93b5..937ae0d733 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -551,7 +551,6 @@ test_expect_success REFFILES 'no bogus intermediate values during delete' '
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	git update-ref $prefix/foo $D &&
-	git for-each-ref $prefix >unchanged &&
 	# Now try to update the reference, but hold the `packed-refs` lock
 	# for a while to see what happens while the process is blocked:
 	: >.git/packed-refs.lock &&
-- 
2.40.0

[PATCH v3 7/7] t1507: assert output of rev-parse

[Andrei Rybak]

Tests in t1507-rev-parse-upstream.sh compare files "expect" and "actual"
to assert the output of "git rev-parse", "git show", and "git log".
However, two of the tests '@{reflog}-parsing does not look beyond colon'
and '@{upstream}-parsing does not look beyond colon' don't inspect the
contents of the created files.

Assert output of "git rev-parse" in tests in t1507-rev-parse-upstream.sh
to improve test coverage.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index c34714ffe3..4458820168 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -258,7 +258,8 @@ test_expect_success '@{reflog}-parsing does not look beyond colon' '
 	git add @{yesterday} &&
 	git commit -m "funny reflog file" &&
 	git hash-object @{yesterday} >expect &&
-	git rev-parse HEAD:@{yesterday} >actual
+	git rev-parse HEAD:@{yesterday} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '@{upstream}-parsing does not look beyond colon' '
@@ -266,7 +267,8 @@ test_expect_success '@{upstream}-parsing does not look beyond colon' '
 	git add @{upstream} &&
 	git commit -m "funny upstream file" &&
 	git hash-object @{upstream} >expect &&
-	git rev-parse HEAD:@{upstream} >actual
+	git rev-parse HEAD:@{upstream} >actual &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.40.0

Re: [PATCH v3 0/7] t: fix unused files, part 1

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> I messed up when sending v2 (incorrectly used format-patch with --in-reply-to),
> and v2 didn't get any response.  So here's v3 which is just a resend of v2.

Everything in this iteration looks good to me.

Let's merge the topic down to 'next'.

Thanks for cleaning up these tests.