[PATCH v1 0/6] t: fix unused files, part 2

[PATCH v1 0/6] t: fix unused files, part 2

[Andrei Rybak]

Creation of files from redirecting output of Git commands in tests has been
removed for files which aren't being used for assertions.  CC'ed are authors of
the affected tests.

This is a continuation of part 1:
  https://lore.kernel.org/git/20230312201520.370234-1-rybak.a.v@gmail.com/T/#u

Overall, this is similar to part 1, except that additionally in patch 4/6
creation of an unused file that isn't output of a git command has been removed.
Patch 2/6 fixes broken comments, found during development of this series.

Since part 1 I've upgraded my 'check_unused_files.py' script (invoked in the
same way as in part 1) and found a bit more issues in t0??? and t1???, and
overall have gone up to t2???.

Upgraded script:

    from sys import argv
    import re

    script = argv[1]

    git_with_outputs_pattern = re.compile('git [^&\"]*?[>](?!/)([-a-z_./]*)( 2[>](?!/)([a-z_.-]*))?')

    out_read_index = -1
    err_read_index = -1

    def clean_up_filename(fn):
        if fn and '/' in fn:
            from os import path
            return path.basename(fn)
        return fn

    while True:
        res = git_with_outputs_pattern.search(script)
        if res is None:
            break
        end_index = res.span()[1]
        out_filename = clean_up_filename(res.group(1))
        err_filename = clean_up_filename(res.group(3))

        script = script[end_index:]

        if out_filename:
            out_read_index = script.find(out_filename)
            if out_read_index < 0:
                print("File '" + out_filename + "' is unused")
                print("Script: ")
                print(script)
                from sys import exit
                exit(1)
        if err_filename and err_filename != '&1':
            err_read_index = script.find(err_filename)
            if err_read_index < 0:
                print("File '" + err_filename + "' is unused")
                print("Script: ")
                print(script)
                from sys import exit
                exit(2)
        if out_read_index >= 0:
            script = script[out_read_index + len(out_filename):]
        elif err_read_index >= 0:
            script = script[err_read_index + len(err_filename):]

Andrei Rybak (6):
  t0300: don't create unused file
  t1300: fix config file syntax error descriptions
  t1300: don't create unused files
  t1450: don't create unused files
  t1502: don't create unused files
  t2019: don't create unused files

 t/t0300-credentials.sh            |  2 +-
 t/t1300-config.sh                 | 10 +++++-----
 t/t1450-fsck.sh                   |  5 +----
 t/t1502-rev-parse-parseopt.sh     |  6 +++---
 t/t2019-checkout-ambiguous-ref.sh |  4 ++--
 5 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.40.0

[PATCH v1 1/6] t0300: don't create unused file

[Andrei Rybak]

Test 'credential config with partial URLs' in t0300-credentials.sh
contaisn three "git credential fill" invocations.  For two of the
invocations, the test asserts presence or absence of string "yep" in the
standard output.  For the third test it checks for an error message in
standard error.

Don't redirect standard output of "git credential" to file "stdout" in
t0300-credentials.sh to avoid creating an unnecessary file when only
standard error is checked.

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

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index c66d91e82d..b8612ede95 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '
 
 	git -c credential.$partial.helper=yep \
 		-c credential.with%0anewline.username=uh-oh \
-		credential fill <stdin >stdout 2>stderr &&
+		credential fill <stdin 2>stderr &&
 	test_i18ngrep "skipping credential lookup for key" stderr
 '
 
-- 
2.40.0

[PATCH v1 2/6] t1300: fix config file syntax error descriptions

[Andrei Rybak]

Three tests in t1300-config.sh check that "git config --get" barfs when
the config file contains various syntax errors: key=value pair without
equals sign, broken section line, and broken value string.  The sample
config files include a comment describing the kind of broken syntax.
This description seems to have been copy-pasted from the "broken section
line" sample to the other two samples.

Fix descriptions of broken config file syntax in samples used in
t1300-config.sh.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 2575279ab8..d566729d74 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1571,7 +1571,7 @@ test_expect_success 'git config --edit respects core.editor' '
 # malformed configuration files
 test_expect_success 'barf on syntax error' '
 	cat >.git/config <<-\EOF &&
-	# broken section line
+	# broken key=value
 	[section]
 	key garbage
 	EOF
@@ -1591,7 +1591,7 @@ test_expect_success 'barf on incomplete section header' '
 
 test_expect_success 'barf on incomplete string' '
 	cat >.git/config <<-\EOF &&
-	# broken section line
+	# broken value string
 	[section]
 	key = "value string
 	EOF
-- 
2.40.0

[PATCH v1 3/6] t1300: don't create unused files

[Andrei Rybak]

Three tests in t1300-config.sh check that "git config --get" barfs when
syntax errors are present in the config file.  The tests redirect
standard output and standard error of "git config --get" to files,
"actual" and "error" correspondingly.  They assert presence of an error
message in file "error".  However, these tests don't use file "actual"
for assertions.

Don't redirect standard output of "git config --get" to file "actual" in
t1300-config.sh to avoid creating unnecessary files.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index d566729d74..8ac4531c1b 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '
 	[section]
 	key garbage
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 3 " error
 '
 
@@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '
 	[section
 	key = value
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 2 " error
 '
 
@@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '
 	[section]
 	key = "value string
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 3 " error
 '
 
-- 
2.40.0

[PATCH v1 4/6] t1450: don't create unused files

[Andrei Rybak]

Test 'fsck error and recovery on invalid object type' in file
t1450-fsck.sh redirects output of a failing "git fsck" invocation to
files "out" and "err" to assert presence of error messages in the output
of the command.  Commit 31deb28f5e (fsck: don't hard die on invalid
object types, 2021-10-01) changed the way assertions in this test are
performed.  The test doesn't compare the whole standard error with
prepared file "err.expect" and it doesn't assert that standard output is
empty.

Don't create unused files "err.expect" and "out" in test 'fsck error and
recovery on invalid object type'.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1450-fsck.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index bca46378b2..8c442adb1a 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '
 
 		garbage_blob=$(git hash-object --stdin -w -t garbage --literally </dev/null) &&
 
-		cat >err.expect <<-\EOF &&
-		fatal: invalid object type
-		EOF
-		test_must_fail git fsck >out 2>err &&
+		test_must_fail git fsck 2>err &&
 		grep -e "^error" -e "^fatal" err >errors &&
 		test_line_count = 1 errors &&
 		grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err
-- 
2.40.0

[PATCH v1 5/6] t1502: don't create unused files

[Andrei Rybak]

Three tests in file t1502-rev-parse-parseopt.sh use three redirections
with invocation of "git rev-parse --parseopt --".  All three tests
redirect standard output to file "out" and file "spec" to standard
input.  Two of the tests redirect standard output a second time to file
"actual", and the third test redirects standard error to file "err".
These tests check contents of files "actual" and "err", but don't use
the files named "out" for assertions.  The two tests that redirect to
standard output twice might also be confusing to the reader.

Don't redirect standard output of "git rev-parse" to file "out" in
t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.

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

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index de1d48f3ba..dd811b7fb4 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|EOF
 	END_EXPECT
 
-	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
+	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'test --parseopt invalid opt-spec' '
 	test_write_lines x -- "=, x" >spec &&
 	echo "fatal: missing opt-spec before option flags" >expect &&
-	test_must_fail git rev-parse --parseopt -- >out <spec 2>err &&
+	test_must_fail git rev-parse --parseopt -- <spec 2>err &&
 	test_cmp expect err
 '
 
@@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|EOF
 	END_EXPECT
 
-	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
+	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
 	test_cmp expect actual
 '
 
-- 
2.40.0

[PATCH v1 6/6] t2019: don't create unused files

[Andrei Rybak]

Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of
"git checkout" to files "stdout" and "stderr".  Several assertions are
made using file "stderr".  File "stdout", however, is unused.

Don't redirect standard output of "git checkout" to file "stdout" in
t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.

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

diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh
index 2c8c926b4d..9540588664 100755
--- a/t/t2019-checkout-ambiguous-ref.sh
+++ b/t/t2019-checkout-ambiguous-ref.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '
 '
 
 test_expect_success 'checkout ambiguous ref succeeds' '
-	git checkout ambiguity >stdout 2>stderr
+	git checkout ambiguity 2>stderr
 '
 
 test_expect_success 'checkout produces ambiguity warning' '
@@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '
 '
 
 test_expect_success 'checkout vague ref succeeds' '
-	git checkout vagueness >stdout 2>stderr &&
+	git checkout vagueness 2>stderr &&
 	test_set_prereq VAGUENESS_SUCCESS
 '
 
-- 
2.40.0

Re: [PATCH v1 1/6] t0300: don't create unused file

[Eric Sunshine]

On Sat, Apr 1, 2023 at 5:34 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
> Test 'credential config with partial URLs' in t0300-credentials.sh
> contaisn three "git credential fill" invocations.  For two of the
> invocations, the test asserts presence or absence of string "yep" in the
> standard output.  For the third test it checks for an error message in
> standard error.

s/contaisn/contains/

> Don't redirect standard output of "git credential" to file "stdout" in
> t0300-credentials.sh to avoid creating an unnecessary file when only
> standard error is checked.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>

[PATCH v2 0/6] t: fix unused files, part 2

[Andrei Rybak]

Creation of files from redirecting output of Git commands in tests has been
removed for files which aren't being used for assertions.  CC'ed are authors of
the affected tests.

v1 cover letter:
  https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/

Thanks to Eric Sunshine for reviewing v1.

Changes since v1:

  - Fixed typo in commit message of 1/6 found by Eric Sunshine

Range diff:

1:  516cc3fe83 ! 1:  828bb18bd7 t0300: don't create unused file
    @@ Commit message
         t0300: don't create unused file

         Test 'credential config with partial URLs' in t0300-credentials.sh
    -    contaisn three "git credential fill" invocations.  For two of the
    +    contains three "git credential fill" invocations.  For two of the
         invocations, the test asserts presence or absence of string "yep" in the
         standard output.  For the third test it checks for an error message in
         standard error.
2:  9282c9bb07 = 2:  a5b299a0c6 t1300: fix config file syntax error descriptions
3:  df0ec7ebf9 = 3:  806df16415 t1300: don't create unused files
4:  408e971b43 = 4:  6742c957e5 t1450: don't create unused files
5:  8464d0f435 = 5:  6c173a5c46 t1502: don't create unused files
6:  de2b5339d7 = 6:  d508c1def3 t2019: don't create unused files

Andrei Rybak (6):
  t0300: don't create unused file
  t1300: fix config file syntax error descriptions
  t1300: don't create unused files
  t1450: don't create unused files
  t1502: don't create unused files
  t2019: don't create unused files

 t/t0300-credentials.sh            |  2 +-
 t/t1300-config.sh                 | 10 +++++-----
 t/t1450-fsck.sh                   |  5 +----
 t/t1502-rev-parse-parseopt.sh     |  6 +++---
 t/t2019-checkout-ambiguous-ref.sh |  4 ++--
 5 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.40.0

[PATCH v2 1/6] t0300: don't create unused file

[Andrei Rybak]

Test 'credential config with partial URLs' in t0300-credentials.sh
contains three "git credential fill" invocations.  For two of the
invocations, the test asserts presence or absence of string "yep" in the
standard output.  For the third test it checks for an error message in
standard error.

Don't redirect standard output of "git credential" to file "stdout" in
t0300-credentials.sh to avoid creating an unnecessary file when only
standard error is checked.

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

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index c66d91e82d..b8612ede95 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '
 
 	git -c credential.$partial.helper=yep \
 		-c credential.with%0anewline.username=uh-oh \
-		credential fill <stdin >stdout 2>stderr &&
+		credential fill <stdin 2>stderr &&
 	test_i18ngrep "skipping credential lookup for key" stderr
 '
 
-- 
2.40.0

[PATCH v2 2/6] t1300: fix config file syntax error descriptions

[Andrei Rybak]

Three tests in t1300-config.sh check that "git config --get" barfs when
the config file contains various syntax errors: key=value pair without
equals sign, broken section line, and broken value string.  The sample
config files include a comment describing the kind of broken syntax.
This description seems to have been copy-pasted from the "broken section
line" sample to the other two samples.

Fix descriptions of broken config file syntax in samples used in
t1300-config.sh.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 2575279ab8..d566729d74 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1571,7 +1571,7 @@ test_expect_success 'git config --edit respects core.editor' '
 # malformed configuration files
 test_expect_success 'barf on syntax error' '
 	cat >.git/config <<-\EOF &&
-	# broken section line
+	# broken key=value
 	[section]
 	key garbage
 	EOF
@@ -1591,7 +1591,7 @@ test_expect_success 'barf on incomplete section header' '
 
 test_expect_success 'barf on incomplete string' '
 	cat >.git/config <<-\EOF &&
-	# broken section line
+	# broken value string
 	[section]
 	key = "value string
 	EOF
-- 
2.40.0

[PATCH v2 3/6] t1300: don't create unused files

[Andrei Rybak]

Three tests in t1300-config.sh check that "git config --get" barfs when
syntax errors are present in the config file.  The tests redirect
standard output and standard error of "git config --get" to files,
"actual" and "error" correspondingly.  They assert presence of an error
message in file "error".  However, these tests don't use file "actual"
for assertions.

Don't redirect standard output of "git config --get" to file "actual" in
t1300-config.sh to avoid creating unnecessary files.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index d566729d74..8ac4531c1b 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '
 	[section]
 	key garbage
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 3 " error
 '
 
@@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '
 	[section
 	key = value
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 2 " error
 '
 
@@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '
 	[section]
 	key = "value string
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 3 " error
 '
 
-- 
2.40.0

[PATCH v2 4/6] t1450: don't create unused files

[Andrei Rybak]

Test 'fsck error and recovery on invalid object type' in file
t1450-fsck.sh redirects output of a failing "git fsck" invocation to
files "out" and "err" to assert presence of error messages in the output
of the command.  Commit 31deb28f5e (fsck: don't hard die on invalid
object types, 2021-10-01) changed the way assertions in this test are
performed.  The test doesn't compare the whole standard error with
prepared file "err.expect" and it doesn't assert that standard output is
empty.

Don't create unused files "err.expect" and "out" in test 'fsck error and
recovery on invalid object type'.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1450-fsck.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index bca46378b2..8c442adb1a 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '
 
 		garbage_blob=$(git hash-object --stdin -w -t garbage --literally </dev/null) &&
 
-		cat >err.expect <<-\EOF &&
-		fatal: invalid object type
-		EOF
-		test_must_fail git fsck >out 2>err &&
+		test_must_fail git fsck 2>err &&
 		grep -e "^error" -e "^fatal" err >errors &&
 		test_line_count = 1 errors &&
 		grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err
-- 
2.40.0

[PATCH v2 5/6] t1502: don't create unused files

[Andrei Rybak]

Three tests in file t1502-rev-parse-parseopt.sh use three redirections
with invocation of "git rev-parse --parseopt --".  All three tests
redirect standard output to file "out" and file "spec" to standard
input.  Two of the tests redirect standard output a second time to file
"actual", and the third test redirects standard error to file "err".
These tests check contents of files "actual" and "err", but don't use
the files named "out" for assertions.  The two tests that redirect to
standard output twice might also be confusing to the reader.

Don't redirect standard output of "git rev-parse" to file "out" in
t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.

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

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index de1d48f3ba..dd811b7fb4 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|EOF
 	END_EXPECT
 
-	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
+	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'test --parseopt invalid opt-spec' '
 	test_write_lines x -- "=, x" >spec &&
 	echo "fatal: missing opt-spec before option flags" >expect &&
-	test_must_fail git rev-parse --parseopt -- >out <spec 2>err &&
+	test_must_fail git rev-parse --parseopt -- <spec 2>err &&
 	test_cmp expect err
 '
 
@@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|EOF
 	END_EXPECT
 
-	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
+	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
 	test_cmp expect actual
 '
 
-- 
2.40.0

[PATCH v2 6/6] t2019: don't create unused files

[Andrei Rybak]

Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of
"git checkout" to files "stdout" and "stderr".  Several assertions are
made using file "stderr".  File "stdout", however, is unused.

Don't redirect standard output of "git checkout" to file "stdout" in
t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.

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

diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh
index 2c8c926b4d..9540588664 100755
--- a/t/t2019-checkout-ambiguous-ref.sh
+++ b/t/t2019-checkout-ambiguous-ref.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '
 '
 
 test_expect_success 'checkout ambiguous ref succeeds' '
-	git checkout ambiguity >stdout 2>stderr
+	git checkout ambiguity 2>stderr
 '
 
 test_expect_success 'checkout produces ambiguity warning' '
@@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '
 '
 
 test_expect_success 'checkout vague ref succeeds' '
-	git checkout vagueness >stdout 2>stderr &&
+	git checkout vagueness 2>stderr &&
 	test_set_prereq VAGUENESS_SUCCESS
 '
 
-- 
2.40.0

Re: [PATCH v2 5/6] t1502: don't create unused files

[Øystein Walle]

Hi Andrei

On Tue, 4 Apr 2023 at 00:33, Andrei Rybak <rybak.a.v@gmail.com> wrote:
>
>  test_expect_success 'test --parseopt invalid opt-spec' '
>         test_write_lines x -- "=, x" >spec &&
>         echo "fatal: missing opt-spec before option flags" >expect &&
> -       test_must_fail git rev-parse --parseopt -- >out <spec 2>err &&
> +       test_must_fail git rev-parse --parseopt -- <spec 2>err &&
>         test_cmp expect err
>  '

This is the one that was touched by me. At the time I just cargo-culted other
tests. This looks obviously correct to me

For what it's worth:

Acked-by: Øystein Walle <oystwa@gmail.com>

Øsse

Re: [PATCH v2 1/6] t0300: don't create unused file

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 04 2023, Andrei Rybak wrote:

> Test 'credential config with partial URLs' in t0300-credentials.sh
> contains three "git credential fill" invocations.  For two of the
> invocations, the test asserts presence or absence of string "yep" in the
> standard output.  For the third test it checks for an error message in
> standard error.
>
> Don't redirect standard output of "git credential" to file "stdout" in
> t0300-credentials.sh to avoid creating an unnecessary file when only
> standard error is checked.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t0300-credentials.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> index c66d91e82d..b8612ede95 100755
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '
>  
>  	git -c credential.$partial.helper=yep \
>  		-c credential.with%0anewline.username=uh-oh \
> -		credential fill <stdin >stdout 2>stderr &&
> +		credential fill <stdin 2>stderr &&
>  	test_i18ngrep "skipping credential lookup for key" stderr
>  '

This goes for these changes in this series general: You're correct that
this is useless now, but I don't think it follows that we should be
removing the "redundant" code in all cases, rather than fixing the test
to actually check these.

E.g. this will also make this test pass:
	
	diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
	index c66d91e82d8..62c2a0fd50e 100755
	--- a/t/t0300-credentials.sh
	+++ b/t/t0300-credentials.sh
	@@ -806,9 +806,11 @@ test_expect_success 'credential config with partial URLs' '
	 		return 1
	 	done &&
	 
	+	cp stdout stdout.last &&
	 	git -c credential.$partial.helper=yep \
	 		-c credential.with%0anewline.username=uh-oh \
	 		credential fill <stdin >stdout 2>stderr &&
	+	test_cmp stdout.last stdout &&
	 	test_i18ngrep "skipping credential lookup for key" stderr
	 '
	 

Does that make sense? No idea, I don't know the credential system well.

But isn't it worth testing that when we ask for this that we're getting
some known output along with the warning?

Re: [PATCH v2 3/6] t1300: don't create unused files

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 04 2023, Andrei Rybak wrote:

> Three tests in t1300-config.sh check that "git config --get" barfs when
> syntax errors are present in the config file.  The tests redirect
> standard output and standard error of "git config --get" to files,
> "actual" and "error" correspondingly.  They assert presence of an error
> message in file "error".  However, these tests don't use file "actual"
> for assertions.
>
> Don't redirect standard output of "git config --get" to file "actual" in
> t1300-config.sh to avoid creating unnecessary files.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1300-config.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index d566729d74..8ac4531c1b 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '
>  	[section]
>  	key garbage
>  	EOF
> -	test_must_fail git config --get section.key >actual 2>error &&
> +	test_must_fail git config --get section.key 2>error &&
>  	test_i18ngrep " line 3 " error
>  '
>  
> @@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '
>  	[section
>  	key = value
>  	EOF
> -	test_must_fail git config --get section.key >actual 2>error &&
> +	test_must_fail git config --get section.key 2>error &&
>  	test_i18ngrep " line 2 " error
>  '
>  
> @@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '
>  	[section]
>  	key = "value string
>  	EOF
> -	test_must_fail git config --get section.key >actual 2>error &&
> +	test_must_fail git config --get section.key 2>error &&
>  	test_i18ngrep " line 3 " error
>  '

Ditto my comment on 1/6, shouldn't we instead be doing e.g.:
	
	diff --git a/t/t1300-config.sh b/t/t1300-config.sh
	index 2575279ab84..df2070c2f09 100755
	--- a/t/t1300-config.sh
	+++ b/t/t1300-config.sh
	@@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' '
	 	[section]
	 	key garbage
	 	EOF
	-	test_must_fail git config --get section.key >actual 2>error &&
	+	test_must_fail git config --get section.key >out 2>error &&
	+	test_must_be_empty out &&
	 	test_i18ngrep " line 3 " error
	 '
	 
I.e. before this we had no coverage on the error being the only output,
but seemingly by mistake. Let's just assert that, rather than dropping
the redirection entirely, no?

Re: [PATCH v2 4/6] t1450: don't create unused files

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 04 2023, Andrei Rybak wrote:

> Test 'fsck error and recovery on invalid object type' in file
> t1450-fsck.sh redirects output of a failing "git fsck" invocation to
> files "out" and "err" to assert presence of error messages in the output
> of the command.  Commit 31deb28f5e (fsck: don't hard die on invalid
> object types, 2021-10-01) changed the way assertions in this test are
> performed.  The test doesn't compare the whole standard error with
> prepared file "err.expect" and it doesn't assert that standard output is
> empty.

That's my commit, and thanks for catching this...

> Don't create unused files "err.expect" and "out" in test 'fsck error and
> recovery on invalid object type'.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1450-fsck.sh | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index bca46378b2..8c442adb1a 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '
>  
>  		garbage_blob=$(git hash-object --stdin -w -t garbage --literally </dev/null) &&
>  
> -		cat >err.expect <<-\EOF &&
> -		fatal: invalid object type
> -		EOF
> -		test_must_fail git fsck >out 2>err &&
> +		test_must_fail git fsck 2>err &&
>  		grep -e "^error" -e "^fatal" err >errors &&
>  		test_line_count = 1 errors &&
>  		grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err

...but ditto my review on other patches, this just seems like a mistake
of mine, i.e. if I add the "test_must_be_empty out" the test passes.

So isn't the answer here that my 31deb28f5e had an unintentional
regression, and we should bring the assertion back? Its commit message
says nothing about wanting to stop asserting stdout.

Maybe there was a reason I'm missing for why I remved it, it's since
been paged out of my wetware, but looking at it briefly now it just
seems like an unintentional bug / loss of test coverage that we should
fix.

Re: [PATCH v2 6/6] t2019: don't create unused files

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 04 2023, Andrei Rybak wrote:

> Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of
> "git checkout" to files "stdout" and "stderr".  Several assertions are
> made using file "stderr".  File "stdout", however, is unused.
>
> Don't redirect standard output of "git checkout" to file "stdout" in
> t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t2019-checkout-ambiguous-ref.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh
> index 2c8c926b4d..9540588664 100755
> --- a/t/t2019-checkout-ambiguous-ref.sh
> +++ b/t/t2019-checkout-ambiguous-ref.sh
> @@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '
>  '
>  
>  test_expect_success 'checkout ambiguous ref succeeds' '
> -	git checkout ambiguity >stdout 2>stderr
> +	git checkout ambiguity 2>stderr
>  '

Ditto earlier comments that we should just fix this, if I make this
">out" and "test_must_be_empty out" this succeeds, shouldn't we just use
that?

>  test_expect_success 'checkout produces ambiguity warning' '

As an aside, we should really just combine these two tests.

> @@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '
>  '
>  
>  test_expect_success 'checkout vague ref succeeds' '
> -	git checkout vagueness >stdout 2>stderr &&
> +	git checkout vagueness 2>stderr &&
>  	test_set_prereq VAGUENESS_SUCCESS
>  '

Re: [PATCH v2 5/6] t1502: don't create unused files

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 04 2023, Andrei Rybak wrote:

> Three tests in file t1502-rev-parse-parseopt.sh use three redirections
> with invocation of "git rev-parse --parseopt --".  All three tests
> redirect standard output to file "out" and file "spec" to standard
> input.  Two of the tests redirect standard output a second time to file
> "actual", and the third test redirects standard error to file "err".
> These tests check contents of files "actual" and "err", but don't use
> the files named "out" for assertions.  The two tests that redirect to
> standard output twice might also be confusing to the reader.
>
> Don't redirect standard output of "git rev-parse" to file "out" in
> t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1502-rev-parse-parseopt.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> index de1d48f3ba..dd811b7fb4 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
>  	|EOF
>  	END_EXPECT
>  
> -	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
> +	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success 'test --parseopt invalid opt-spec' '
>  	test_write_lines x -- "=, x" >spec &&
>  	echo "fatal: missing opt-spec before option flags" >expect &&
> -	test_must_fail git rev-parse --parseopt -- >out <spec 2>err &&
> +	test_must_fail git rev-parse --parseopt -- <spec 2>err &&
>  	test_cmp expect err
>  '
>  
> @@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
>  	|EOF
>  	END_EXPECT
>  
> -	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
> +	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
>  	test_cmp expect actual
>  '

Ditto earlier comments: When we fail, we should assert what we emitted
on stdout, surely this should also be a "test_must_be_empty out".

(I didn't test that, but if that fails wes hould be testing whatever it
is that we emit here, surely..)

Re: [PATCH v2 5/6] t1502: don't create unused files

[Andrei Rybak]

On 06/04/2023 10:47, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Apr 04 2023, Andrei Rybak wrote:
> 
>> Three tests in file t1502-rev-parse-parseopt.sh use three redirections
>> with invocation of "git rev-parse --parseopt --".  All three tests
>> redirect standard output to file "out" and file "spec" to standard
>> input.  Two of the tests redirect standard output a second time to file
>> "actual", and the third test redirects standard error to file "err".
>> These tests check contents of files "actual" and "err", but don't use
>> the files named "out" for assertions.  The two tests that redirect to
>> standard output twice might also be confusing to the reader.
>>
>> Don't redirect standard output of "git rev-parse" to file "out" in
>> t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.
>>
>> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
>> ---
>>   t/t1502-rev-parse-parseopt.sh | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
>> index de1d48f3ba..dd811b7fb4 100755
>> --- a/t/t1502-rev-parse-parseopt.sh
>> +++ b/t/t1502-rev-parse-parseopt.sh
>> @@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
>>   	|EOF
>>   	END_EXPECT
>>   
>> -	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
>> +	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
>>   	test_cmp expect actual
>>   '
>>   
>>   test_expect_success 'test --parseopt invalid opt-spec' '
>>   	test_write_lines x -- "=, x" >spec &&
>>   	echo "fatal: missing opt-spec before option flags" >expect &&
>> -	test_must_fail git rev-parse --parseopt -- >out <spec 2>err &&
>> +	test_must_fail git rev-parse --parseopt -- <spec 2>err &&
>>   	test_cmp expect err
>>   '
>>   
>> @@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
>>   	|EOF
>>   	END_EXPECT
>>   
>> -	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
>> +	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
>>   	test_cmp expect actual
>>   '
> 
> Ditto earlier comments: When we fail, we should assert what we emitted
> on stdout, surely this should also be a "test_must_be_empty out".
> 
> (I didn't test that, but if that fails wes hould be testing whatever it
> is that we emit here, surely..)

This patch is not like the others.  File "out" is indeed empty, because
file "actual" isn't empty.  Redirect to "out" is overwritten by redirect
to "actual".

Re: [PATCH v2 1/6] t0300: don't create unused file

[Andrei Rybak]

On 06/04/2023 10:34, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Apr 04 2023, Andrei Rybak wrote:
> 
>> Test 'credential config with partial URLs' in t0300-credentials.sh
>> contains three "git credential fill" invocations.  For two of the
>> invocations, the test asserts presence or absence of string "yep" in the
>> standard output.  For the third test it checks for an error message in
>> standard error.
>>
>> Don't redirect standard output of "git credential" to file "stdout" in
>> t0300-credentials.sh to avoid creating an unnecessary file when only
>> standard error is checked.
>>
>> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
>> ---
>>   t/t0300-credentials.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
>> index c66d91e82d..b8612ede95 100755
>> --- a/t/t0300-credentials.sh
>> +++ b/t/t0300-credentials.sh
>> @@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '
>>   
>>   	git -c credential.$partial.helper=yep \
>>   		-c credential.with%0anewline.username=uh-oh \
>> -		credential fill <stdin >stdout 2>stderr &&
>> +		credential fill <stdin 2>stderr &&
>>   	test_i18ngrep "skipping credential lookup for key" stderr
>>   '
> 
> This goes for these changes in this series general: You're correct that
> this is useless now, but I don't think it follows that we should be
> removing the "redundant" code in all cases, rather than fixing the test
> to actually check these.

I'll reply to these on one-by-one.  See also this review of a patch in part 1
of this series from Junio C Hamano:

   https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/

> E.g. this will also make this test pass:
> 	
> 	diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> 	index c66d91e82d8..62c2a0fd50e 100755
> 	--- a/t/t0300-credentials.sh
> 	+++ b/t/t0300-credentials.sh
> 	@@ -806,9 +806,11 @@ test_expect_success 'credential config with partial URLs' '
> 	 		return 1
> 	 	done &&
> 	
> 	+	cp stdout stdout.last &&
> 	 	git -c credential.$partial.helper=yep \
> 	 		-c credential.with%0anewline.username=uh-oh \
> 	 		credential fill <stdin >stdout 2>stderr &&
> 	+	test_cmp stdout.last stdout &&
> 	 	test_i18ngrep "skipping credential lookup for key" stderr
> 	 '
> 	
> 
> Does that make sense? No idea, I don't know the credential system well.
> 
> But isn't it worth testing that when we ask for this that we're getting
> some known output along with the warning?

Current version from branch "master" with more context lines:

> test_expect_success 'credential config with partial URLs' '
> 	echo "echo password=yep" | write_script git-credential-yep &&
> 	test_write_lines url=https://user@example.com/repo.git >stdin &&

The test is checking that "url=https://user@example.com/repo.git" matches ...

> 	for partial in \
> 		example.com \
> 		user@example.com \
> 		https:// \
> 		https://example.com \
> 		https://example.com/ \
> 		https://user@example.com \
> 		https://user@example.com/ \
> 		https://example.com/repo.git \
> 		https://user@example.com/repo.git \
> 		/repo.git

... these partial URLs ...

> 	do
> 		git -c credential.$partial.helper=yep \
> 			credential fill <stdin >stdout &&
> 		grep yep stdout ||
> 		return 1
> 	done &&
> 
> 	for partial in \
> 		dont.use.this \
> 		http:// \
> 		/repo

... but doesn't match these.

> 	do
> 		git -c credential.$partial.helper=yep \
> 			credential fill <stdin >stdout &&
> 		! grep yep stdout ||

Here "! grep yep stdout" ensures that git-credential-yep isn't launched for the
three kinds of partial URLs.  Otherwise, the actual content of "stdout" is
ignored.  It comes from script "askpass" (see bottom of file "t/lib-credential.sh").

Absence or presence of "yep" is a proxy for whether or not partial URL got
matched or not, and that is what's important for this test.  Adding assertions
for output of "askpass" here would only obscure this fact, I think.

There are other tests in t030[0-3] that do check standard output for what
the helper script "askpass" prints out -- those tests validate that the
"git credentials" fallbacks to asking for credentials in the terminal in
various situations.  This is done via functions helper_test and
helper_test_timeout from "t/lib-credential.sh".

> 		return 1
> 	done &&
> 
> 	git -c credential.$partial.helper=yep \
> 		-c credential.with%0anewline.username=uh-oh \
> 		credential fill <stdin >stdout 2>stderr &&
> 	test_i18ngrep "skipping credential lookup for key" stderr

Here, the important part is that "git credential" reacts to invalid key being
used: "credential.with%0anewline.username=uh-oh", and similarly, I think that
adding assertions about "stdout" might not be a good idea.

>'

Perhaps Johannes Schindelin, author of commit 9a121b0d22 ("credential: handle
`credential.<partial-URL>.<key>` again", 2020-04-24), could chime in.

Re: [PATCH v2 3/6] t1300: don't create unused files

[Andrei Rybak]

On 06/04/2023 10:38, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Apr 04 2023, Andrei Rybak wrote:
> 
>> Three tests in t1300-config.sh check that "git config --get" barfs when
>> syntax errors are present in the config file.  The tests redirect
>> standard output and standard error of "git config --get" to files,
>> "actual" and "error" correspondingly.  They assert presence of an error
>> message in file "error".  However, these tests don't use file "actual"
>> for assertions.
>>
>> Don't redirect standard output of "git config --get" to file "actual" in
>> t1300-config.sh to avoid creating unnecessary files.
>>
>> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
>> ---
>>   t/t1300-config.sh | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>> index d566729d74..8ac4531c1b 100755
>> --- a/t/t1300-config.sh
>> +++ b/t/t1300-config.sh
>> @@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '
>>   	[section]
>>   	key garbage
>>   	EOF
>> -	test_must_fail git config --get section.key >actual 2>error &&
>> +	test_must_fail git config --get section.key 2>error &&
>>   	test_i18ngrep " line 3 " error
>>   '
>>   
>> @@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '
>>   	[section
>>   	key = value
>>   	EOF
>> -	test_must_fail git config --get section.key >actual 2>error &&
>> +	test_must_fail git config --get section.key 2>error &&
>>   	test_i18ngrep " line 2 " error
>>   '
>>   
>> @@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '
>>   	[section]
>>   	key = "value string
>>   	EOF
>> -	test_must_fail git config --get section.key >actual 2>error &&
>> +	test_must_fail git config --get section.key 2>error &&
>>   	test_i18ngrep " line 3 " error
>>   '
> 
> Ditto my comment on 1/6, shouldn't we instead be doing e.g.:
> 	
> 	diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> 	index 2575279ab84..df2070c2f09 100755
> 	--- a/t/t1300-config.sh
> 	+++ b/t/t1300-config.sh
> 	@@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' '
> 	 	[section]
> 	 	key garbage
> 	 	EOF
> 	-	test_must_fail git config --get section.key >actual 2>error &&
> 	+	test_must_fail git config --get section.key >out 2>error &&
> 	+	test_must_be_empty out &&
> 	 	test_i18ngrep " line 3 " error
> 	 '
> 	
> I.e. before this we had no coverage on the error being the only output,
> but seemingly by mistake. Let's just assert that, rather than dropping
> the redirection entirely, no?

Here, failing invocations of "git config" are tested, and an argument,
as Junio C Hamano outlined in https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/
for output of failing "git mktree", could be applied here.

Thinking about it more, such assertions enforcing empty standard output for
these commands might be helpful if some tools and/or scripts rely on empty
standard output instead of checking the exit code.  Hyrum's Law applies here,
I guess.

git config tests for "'git config ignores pairs ..." (was Re: [PATCH v2 3/6] t1300: don't create unused files)

[Andrei Rybak]

On 06/04/2023 23:30, Andrei Rybak wrote:
> On 06/04/2023 10:38, Ævar Arnfjörð Bjarmason wrote:
>>
>> Ditto my comment on 1/6, shouldn't we instead be doing e.g.:
>>
>>     diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>>     index 2575279ab84..df2070c2f09 100755
>>     --- a/t/t1300-config.sh
>>     +++ b/t/t1300-config.sh
>>     @@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' '
>>          [section]
>>          key garbage
>>          EOF
>>     -    test_must_fail git config --get section.key >actual 2>error &&
>>     +    test_must_fail git config --get section.key >out 2>error &&
>>     +    test_must_be_empty out &&
>>          test_i18ngrep " line 3 " error
>>      '
>>
>> I.e. before this we had no coverage on the error being the only output,
>> but seemingly by mistake. Let's just assert that, rather than dropping
>> the redirection entirely, no?
> 
> Here, failing invocations of "git config" are tested, and an argument,
> as Junio C Hamano outlined in 
> https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/
> for output of failing "git mktree", could be applied here.
> 
> Thinking about it more, such assertions enforcing empty standard output for
> these commands might be helpful if some tools and/or scripts rely on empty
> standard output instead of checking the exit code.  Hyrum's Law applies 
> here,
> I guess.

There are some tests in t/t1300-config.sh that do check that standard output
or standard error is empty.  And I think I stumbled some other broken tests,
while checking those.

Test 'git config ignores pairs without count' checks that standard error
(2>error) is empty.  Just below it, there seems to be a copy-paste error:
there are two tests titled 'git config ignores pairs with zero count'.
First one doesn't check any output, but the second checks standard output,
while calling the file "error" (>error). Test 'git config ignores pairs
with empty count' checks >error as well.

They were all introduced in d8d77153ea (config: allow specifying config entries
via envvar pairs, 2021-01-12) by Patrick Steinhardt.  Patrick, what do you think?

Re: [PATCH v2 4/6] t1450: don't create unused files

[Andrei Rybak]

On 06/04/2023 10:41, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Apr 04 2023, Andrei Rybak wrote:
>> ---
>>   t/	 | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
>> index bca46378b2..8c442adb1a 100755
>> --- a/t/t1450-fsck.sh
>> +++ b/t/t1450-fsck.sh
>> @@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '
>>   
>>   		garbage_blob=$(git hash-object --stdin -w -t garbage --literally </dev/null) &&
>>   
>> -		cat >err.expect <<-\EOF &&
>> -		fatal: invalid object type
>> -		EOF
>> -		test_must_fail git fsck >out 2>err &&
>> +		test_must_fail git fsck 2>err &&
>>   		grep -e "^error" -e "^fatal" err >errors &&
>>   		test_line_count = 1 errors &&
>>   		grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err
> 
> ...but ditto my review on other patches, this just seems like a mistake
> of mine, i.e. if I add the "test_must_be_empty out" the test passes.
> 
> So isn't the answer here that my 31deb28f5e had an unintentional
> regression, and we should bring the assertion back? Its commit message
> says nothing about wanting to stop asserting stdout.
> 
> Maybe there was a reason I'm missing for why I remved it, it's since
> been paged out of my wetware, but looking at it briefly now it just
> seems like an unintentional bug / loss of test coverage that we should
> fix.

Tests in t1450-fsck.sh that do enforce empty standard output do it mostly
via ">../actual 2>&1" and then a "test_must_be_empty actual".

For 'fsck error and recovery on invalid object type', the question is:
is having this assertion useful for a developer using this test? The test
is about invalid object types and what error messages "git fsck" prints
about them.  The test creates a fresh repository for it:

> 	git init --bare garbage-type &&

Is it useful to a developer working on this part of "git fsck" to have
a "reminder" that no dangling objects should be found in such a fresh
repository?  Speaking of which, should there be such a test:

	test_expect_success 'fresh repository has no dangling objects' '
		git init fresh &&
		git -C fresh fsck >out
		test_must_be_empty out
	'

? Maybe even in t0001-init.sh?

Re: [PATCH v2 6/6] t2019: don't create unused files

[Andrei Rybak]

Disclaimer: while trying to write a response to this email, I went a bit
off track, and a big portion of message below is an investigation of
test coverage of what is printed to standard output and standard error
by "git checkout".  It's still mostly relevant to the discussion about
t2019, or at least I hope so.

There are four parts to this investigation, starting with:


1. Standard output of "git checkout"
Other than "git checkout -p" (tested in t3701-add-interactive.sh), it
seems that the only thing that "git checkout" prints to standard output
is in:

   a) function "show_local_changes" in builtin/checkout.c -- a couple
      of tests in t7201-co.sh validate this
   b) function "report_tracking" in builtin/checkout.c -- there are
      tests that validate tracking information in output of "git
      checkout" in t6040-tracking-info.sh.  One test in
      t2020-checkout-detach.sh, titled 'tracking count is accurate after
      orphan check' validates it as well.

While trying to find all tests that validate standard output of
"git checkout", I found out a couple of things.


2. Standard error of "git checkout"
Honestly, I haven't noticed it before, but I found it surprising that
messages about branch switching:

    - "Switched to branch '%s'\n"
    - "Switched to a new branch '%s'\n"
    - "Switched to and reset branch '%s'\n"

are printed to standard error.

Several tests in t2020-checkout-detach.sh validate what is printed into
standard error by "git checkout" via a variation of ">actual 2>&1".
Tests for advice printed by "git checkout" (looking at "advice.c", it
all goes to stderr), also do a variation of ">actual 2>&1".


3. t2024-checkout-dwim.sh
Test 'loosely defined local base branch is reported correctly' in t2024
has an interesting validation of output of "git checkout":

	git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
	status_uno_is_clean &&
	git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
	status_uno_is_clean &&

	test_cmp expect actual

which is fine, except that neither file "expect" nor "actual" contain
the string "BRANCHNAME".  And this test was broken when it was
introduced in 05e73682cd (checkout: report upstream correctly even with
loosely defined branch.*.merge, 2014-10-14).  It was probably intended
for this test to redirect standard error of "git checkout".  It should
be cleaned up as a separate patch/topic.

On 06/04/2023 10:44, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Apr 04 2023, Andrei Rybak wrote:
> 
>> Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of
>> "git checkout" to files "stdout" and "stderr".  Several assertions are
>> made using file "stderr".  File "stdout", however, is unused.
>>
>> Don't redirect standard output of "git checkout" to file "stdout" in
>> t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.
>>
>> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
>> ---
>>   t/t2019-checkout-ambiguous-ref.sh | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh
>> index 2c8c926b4d..9540588664 100755
>> --- a/t/t2019-checkout-ambiguous-ref.sh
>> +++ b/t/t2019-checkout-ambiguous-ref.sh
>> @@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '
>>   '
>>   
>>   test_expect_success 'checkout ambiguous ref succeeds' '
>> -	git checkout ambiguity >stdout 2>stderr
>> +	git checkout ambiguity 2>stderr
>>   '
> 
> Ditto earlier comments that we should just fix this, if I make this
> ">out" and "test_must_be_empty out" this succeeds, shouldn't we just use
> that?

4. t2019-checkout-ambiguous-ref.sh
Back on topic: is empty standard output something that this test in
t2019 should worry about?  Let's take a look at other tests.

Aside from what was mentioned in section 1, tests in t7201 don't look
at standard output of "git checkout".  There isn't a lot of
"test_must_be_empty" in t/t2*check*:

     $ git grep 'test_must_be_empty' t/t2*check*
     t/t2004-checkout-cache-temp.sh: test_must_be_empty stderr &&
     t/t2013-checkout-submodule.sh:  test_must_be_empty actual
     t/t2013-checkout-submodule.sh:  test_must_be_empty actual
     t/t2013-checkout-submodule.sh:  test_must_be_empty actual
     t/t2024-checkout-dwim.sh:       test_must_be_empty status.actual

The first one, in t2004, asserts output of "git checkout-index".
All three in t2013 assert output of "git checkout HEAD >actual 2>&1".
The last one, in t2024, asserts output of "git status".

(There's also one "test_line_count = 0" in the same test in t2004,
  but otherwise these tests seem to be pretty up-to-date w.r.t.
  to using test_must_be_empty helper)

> 
>>   test_expect_success 'checkout produces ambiguity warning' '
> 
> As an aside, we should really just combine these two tests.

My dumb script for finding unused files gives false-positives for such
tests.  And there a lot of tests that got split during introduction of
C_LOCALE_OUTPUT prerequisite or were introduced before C_LOCALE_OUTPUT
was phased out.

For t2019, however, the tests were created this way before
C_LOCALE_OUTPUT in 0cb6ad3c3d ("checkout: fix bug with ambiguous refs",
2011-01-11).  Then the prerequisite was added in 6b3d83efac
("t2019-checkout-ambiguous-ref.sh: depend on C_LOCALE_OUTPUT",
2011-04-03) and removed in d3bd0425b2 ("i18n: use test_i18ngrep in
lib-httpd and t2019", 2011-04-12).

> 
>> @@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '
>>   '
>>   
>>   test_expect_success 'checkout vague ref succeeds' '
>> -	git checkout vagueness >stdout 2>stderr &&
>> +	git checkout vagueness 2>stderr &&
>>   	test_set_prereq VAGUENESS_SUCCESS
>>   '
> 

[PATCH] t2024: fix loose/strict local base branch DWIM test

[Andrei Rybak]

Test 'loosely defined local base branch is reported correctly' in
t2024-checkout-dwim.sh, which was introduced in [1] compares output of
two invocations of "git checkout", invoked with two different branches
named "strict" and "loose".  As per description in [1], the test is
validating that output of tracking information for these two branches.
This tracking information is printed to standard output:

    Your branch is behind 'main' by 1 commit, and can be fast-forwarded.
      (use "git pull" to update your local branch)

The test assumes that the names of the two branches (strict and loose)
are in that output, and pipes the output through sed to replace names of
the branches with "BRANCHNAME".  Command "git checkout", however,
outputs the branch name to standard error, not standard output -- see
message "Switched to branch '%s'\n" in function "update_refs_for_switch"
in "builtin/checkout.c".  This means that the two invocations of sed do
nothing.

Redirect both the standard output and the standard error of "git
checkout" for these assertions.  Ensure that compared files have the
string "BRANCHNAME".

In a series of piped commands, only the return code of the last command
is used.  Thus, all other commands will have their return codes masked.
Avoid piping of output of git directly into sed to preserve the exit
status code of "git checkout", while we're here.

[1] 05e73682cd (checkout: report upstream correctly even with loosely
    defined branch.*.merge, 2014-10-14)

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

On 2023-04-07T04:19, Andrei Rybak wrote:
> 3. t2024-checkout-dwim.sh
> Test 'loosely defined local base branch is reported correctly' in t2024
> has an interesting validation of output of "git checkout":
> 
>      git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
>      status_uno_is_clean &&
>      git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
>      status_uno_is_clean &&
> 
>      test_cmp expect actual
> 
> which is fine, except that neither file "expect" nor "actual" contain
> the string "BRANCHNAME".  And this test was broken when it was
> introduced in 05e73682cd (checkout: report upstream correctly even with
> loosely defined branch.*.merge, 2014-10-14).  It was probably intended
> for this test to redirect standard error of "git checkout".  It should
> be cleaned up as a separate patch/topic.

Here's the patch.  Alternatively, the fix could be to just drop the sed
invocation from this test.

 t/t2024-checkout-dwim.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 4a1c901456..74049a9812 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -305,10 +305,13 @@ test_expect_success 'loosely defined local base branch is reported correctly' '
 	test_config branch.strict.merge refs/heads/main &&
 	test_config branch.loose.merge main &&
 
-	git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
+	git checkout strict >expect.raw 2>&1 &&
+	sed -e "s/strict/BRANCHNAME/g" <expect.raw >expect &&
 	status_uno_is_clean &&
-	git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
+	git checkout loose >actual.raw 2>&1 &&
+	sed -e "s/loose/BRANCHNAME/g" <actual.raw >actual &&
 	status_uno_is_clean &&
+	grep BRANCHNAME actual &&
 
 	test_cmp expect actual
 '
-- 
2.40.0

Re: [PATCH] t2024: fix loose/strict local base branch DWIM test

[Junio C Hamano]

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

>> 3. t2024-checkout-dwim.sh
>> Test 'loosely defined local base branch is reported correctly' in t2024
>> has an interesting validation of output of "git checkout":
>> 
>>      git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
>>      status_uno_is_clean &&
>>      git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
>>      status_uno_is_clean &&
>> 
>>      test_cmp expect actual
>> 
>> which is fine, except that neither file "expect" nor "actual" contain
>> the string "BRANCHNAME".  And this test was broken when it was
>> introduced in 05e73682cd (checkout: report upstream correctly even with
>> loosely defined branch.*.merge, 2014-10-14).  It was probably intended
>> for this test to redirect standard error of "git checkout".  It should
>> be cleaned up as a separate patch/topic.
>
> Here's the patch.  Alternatively, the fix could be to just drop the sed
> invocation from this test.

Or we could check both stdout and stderr separately.  I think the
change in this patch reflects the intention of the original most
closely, so let's queue it as-is.

Thanks.


>
>  t/t2024-checkout-dwim.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
> index 4a1c901456..74049a9812 100755
> --- a/t/t2024-checkout-dwim.sh
> +++ b/t/t2024-checkout-dwim.sh
> @@ -305,10 +305,13 @@ test_expect_success 'loosely defined local base branch is reported correctly' '
>  	test_config branch.strict.merge refs/heads/main &&
>  	test_config branch.loose.merge main &&
>  
> -	git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
> +	git checkout strict >expect.raw 2>&1 &&
> +	sed -e "s/strict/BRANCHNAME/g" <expect.raw >expect &&
>  	status_uno_is_clean &&
> -	git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
> +	git checkout loose >actual.raw 2>&1 &&
> +	sed -e "s/loose/BRANCHNAME/g" <actual.raw >actual &&
>  	status_uno_is_clean &&
> +	grep BRANCHNAME actual &&
>  
>  	test_cmp expect actual
>  '

[PATCH v1 0/2] git config tests for "'git config ignores pairs ..."

[Andrei Rybak]

On 2023-04-06T23:35, Andrei Rybak wrote:
> There are some tests in t/t1300-config.sh that do check that standard
> output
> or standard error is empty.  And I think I stumbled some other broken
> tests,
> while checking those.
>
> Test 'git config ignores pairs without count' checks that standard error
> (2>error) is empty.  Just below it, there seems to be a copy-paste error:
> there are two tests titled 'git config ignores pairs with zero count'.
> First one doesn't check any output, but the second checks standard output,
> while calling the file "error" (>error). Test 'git config ignores pairs
> with empty count' checks >error as well.
>
> They were all introduced in d8d77153ea (config: allow specifying config
> entries
> via envvar pairs, 2021-01-12) by Patrick Steinhardt.  Patrick, what do
> you think?

Here are patches to fix these.

Andrei Rybak (2):
  t1300: drop duplicate test
  t1300: check stderr for "ignores pairs" tests

 t/t1300-config.sh | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

-- 
2.40.0

[PATCH v1 1/2] t1300: drop duplicate test

[Andrei Rybak]

There are two almost identical tests called 'git config ignores pairs
with zero count' in file t1300-config.sh.  Drop the first of these and
keep the one that contains more assertions.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1300-config.sh | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 2575279ab8..696dca17c6 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1458,13 +1458,6 @@ test_expect_success 'git config ignores pairs without count' '
 	test_must_be_empty error
 '
 
-test_expect_success 'git config ignores pairs with zero count' '
-	test_must_fail env \
-		GIT_CONFIG_COUNT=0 \
-		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one
-'
-
 test_expect_success 'git config ignores pairs exceeding count' '
 	GIT_CONFIG_COUNT=1 \
 		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-- 
2.40.0

[PATCH v1 2/2] t1300: check stderr for "ignores pairs" tests

[Andrei Rybak]

Tests "git config ignores pairs ..." in t1300-config.sh validate that
"git config" ignores with various kinds of supplied pairs of environment
variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* that should be ingored.
By "ignores" here we mean that "git config" doesn't complain about them
to standard error.  This is validated by redirecting the standard error
to a file called "error" and asserting that it is empty.  However, two
of these tests incorrectly redirect to standard output while calling the
file "error", and test 'git config ignores pairs exceeding count'
doesn't validate standard error at all.

Fix it by redirecting standard error to file "error" and asserting its
emptiness.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 696dca17c6..20a15ede5c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '
 	GIT_CONFIG_COUNT=1 \
 		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
 		GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \
-		git config --get-regexp "pair.*" >actual &&
+		git config --get-regexp "pair.*" >actual 2>error &&
 	cat >expect <<-EOF &&
 	pair.one value
 	EOF
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_be_empty error
 '
 
 test_expect_success 'git config ignores pairs with zero count' '
 	test_must_fail env \
 		GIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one >error &&
+		git config pair.one 2>error &&
 	test_must_be_empty error
 '
 
 test_expect_success 'git config ignores pairs with empty count' '
 	test_must_fail env \
 		GIT_CONFIG_COUNT= GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one >error &&
+		git config pair.one 2>error &&
 	test_must_be_empty error
 '
 
-- 
2.40.0

Re: [PATCH v1 2/2] t1300: check stderr for "ignores pairs" tests

[Andrei Rybak]

On Fri, 14 Apr 2023 at 10:13, Andrei Rybak <rybak.a.v@gmail.com> wrote:
>
> Tests "git config ignores pairs ..." in t1300-config.sh validate that
> "git config" ignores with various kinds of supplied pairs of environment
> variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* that should be ingored.
> By "ignores" here we mean that "git config" doesn't complain about them
> to standard error.

After thinking about this some more, I've realized that this is an
incorrect interpretation
of the titles of these tests. The correct interpretation is more
obvious from another test:

    test_expect_success 'git config ignores pairs exceeding count' '
           GIT_CONFIG_COUNT=1 \
                  GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
                  GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \
                  git config --get-regexp "pair.*" >actual &&
           cat >expect <<-EOF &&
           pair.one value
           EOF
           test_cmp expect actual
    '

Key-value pair "pair.two=value" is ignored because it's outside of the
range of the
supplied value of GIT_CONFIG_COUNT.  That is, these tests validate that reading
of these environment variables reads GIT_CONFIG_COUNT first and only loads
GIT_CONFIG_KEY_<n> and GIT_CONFIG_VALUE_<n> that fit in the range.

> This is validated by redirecting the standard error
> to a file called "error" and asserting that it is empty.  However, two
> of these tests incorrectly redirect to standard output while calling the
> file "error", and test 'git config ignores pairs exceeding count'
> doesn't validate standard error at all.
>
> Fix it by redirecting standard error to file "error" and asserting its
> emptiness.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1300-config.sh | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 696dca17c6..20a15ede5c 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '
>         GIT_CONFIG_COUNT=1 \
>                 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
>                 GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \
> -               git config --get-regexp "pair.*" >actual &&
> +               git config --get-regexp "pair.*" >actual 2>error &&
>         cat >expect <<-EOF &&
>         pair.one value
>         EOF
> -       test_cmp expect actual
> +       test_cmp expect actual &&
> +       test_must_be_empty error
>  '
>
>  test_expect_success 'git config ignores pairs with zero count' '
>         test_must_fail env \
>                 GIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
> -               git config pair.one >error &&
> +               git config pair.one 2>error &&
>         test_must_be_empty error
>  '
>
>  test_expect_success 'git config ignores pairs with empty count' '
>         test_must_fail env \
>                 GIT_CONFIG_COUNT= GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
> -               git config pair.one >error &&
> +               git config pair.one 2>error &&


Same question as in Ævar's
https://lore.kernel.org/git/230406.86pm8htnfk.gmgdl@evledraar.gmail.com/
and my reply https://lore.kernel.org/git/c43e6b71-075a-e39a-7351-8595e145dacf@gmail.com/
applies here, though.  In tests 'git config ignores pairs with zero count' and
 'git config ignores pairs with empty count' test_must_fail already asserts that
"git config" couldn't get the value.  Should we be also inspecting
both stdout and
stderr, as the test  'git config ignores pairs exceeding count' does
(after this patch)?

>         test_must_be_empty error
>  '

>
> --
> 2.40.0
>

[PATCH v3 0/6] t: fix unused files, part 2

[Andrei Rybak]

Creation of files from redirecting output of Git commands in tests has been
removed for files which aren't being used for assertions.  CC'ed are authors of
the affected tests.

v1 cover letter:
  https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/
v2 cover letter:
  https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/

Changes since v2:

  - Added "Acked-by" of Øystein Walle to patch 5/6
    Cf. https://lore.kernel.org/git/CAFaJEqug4bghEMnEQzGDN10EqM8e8iSf5i12AvOm+NZzDCQKOw@mail.gmail.com/

Range diff:

1:  828bb18bd7 = 1:  828bb18bd7 t0300: don't create unused file
2:  a5b299a0c6 = 2:  a5b299a0c6 t1300: fix config file syntax error descriptions
3:  806df16415 = 3:  806df16415 t1300: don't create unused files
4:  6742c957e5 = 4:  6742c957e5 t1450: don't create unused files
5:  6c173a5c46 ! 5:  19ac488922 t1502: don't create unused files
    @@ Commit message
         Don't redirect standard output of "git rev-parse" to file "out" in
         t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.

    +    Acked-by: Øystein Walle <oystwa@gmail.com>
         Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>

      ## t/t1502-rev-parse-parseopt.sh ##
6:  d508c1def3 = 6:  c41657be88 t2019: don't create unused files

Andrei Rybak (6):
  t0300: don't create unused file
  t1300: fix config file syntax error descriptions
  t1300: don't create unused files
  t1450: don't create unused files
  t1502: don't create unused files
  t2019: don't create unused files

 t/t0300-credentials.sh            |  2 +-
 t/t1300-config.sh                 | 10 +++++-----
 t/t1450-fsck.sh                   |  5 +----
 t/t1502-rev-parse-parseopt.sh     |  6 +++---
 t/t2019-checkout-ambiguous-ref.sh |  4 ++--
 5 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.40.0

[PATCH v3 1/6] t0300: don't create unused file

[Andrei Rybak]

Test 'credential config with partial URLs' in t0300-credentials.sh
contains three "git credential fill" invocations.  For two of the
invocations, the test asserts presence or absence of string "yep" in the
standard output.  For the third test it checks for an error message in
standard error.

Don't redirect standard output of "git credential" to file "stdout" in
t0300-credentials.sh to avoid creating an unnecessary file when only
standard error is checked.

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

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index c66d91e82d..b8612ede95 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '
 
 	git -c credential.$partial.helper=yep \
 		-c credential.with%0anewline.username=uh-oh \
-		credential fill <stdin >stdout 2>stderr &&
+		credential fill <stdin 2>stderr &&
 	test_i18ngrep "skipping credential lookup for key" stderr
 '
 
-- 
2.40.0

[PATCH v3 2/6] t1300: fix config file syntax error descriptions

[Andrei Rybak]

Three tests in t1300-config.sh check that "git config --get" barfs when
the config file contains various syntax errors: key=value pair without
equals sign, broken section line, and broken value string.  The sample
config files include a comment describing the kind of broken syntax.
This description seems to have been copy-pasted from the "broken section
line" sample to the other two samples.

Fix descriptions of broken config file syntax in samples used in
t1300-config.sh.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 2575279ab8..d566729d74 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1571,7 +1571,7 @@ test_expect_success 'git config --edit respects core.editor' '
 # malformed configuration files
 test_expect_success 'barf on syntax error' '
 	cat >.git/config <<-\EOF &&
-	# broken section line
+	# broken key=value
 	[section]
 	key garbage
 	EOF
@@ -1591,7 +1591,7 @@ test_expect_success 'barf on incomplete section header' '
 
 test_expect_success 'barf on incomplete string' '
 	cat >.git/config <<-\EOF &&
-	# broken section line
+	# broken value string
 	[section]
 	key = "value string
 	EOF
-- 
2.40.0

[PATCH v3 3/6] t1300: don't create unused files

[Andrei Rybak]

Three tests in t1300-config.sh check that "git config --get" barfs when
syntax errors are present in the config file.  The tests redirect
standard output and standard error of "git config --get" to files,
"actual" and "error" correspondingly.  They assert presence of an error
message in file "error".  However, these tests don't use file "actual"
for assertions.

Don't redirect standard output of "git config --get" to file "actual" in
t1300-config.sh to avoid creating unnecessary files.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index d566729d74..8ac4531c1b 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '
 	[section]
 	key garbage
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 3 " error
 '
 
@@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '
 	[section
 	key = value
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 2 " error
 '
 
@@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '
 	[section]
 	key = "value string
 	EOF
-	test_must_fail git config --get section.key >actual 2>error &&
+	test_must_fail git config --get section.key 2>error &&
 	test_i18ngrep " line 3 " error
 '
 
-- 
2.40.0

[PATCH v3 4/6] t1450: don't create unused files

[Andrei Rybak]

Test 'fsck error and recovery on invalid object type' in file
t1450-fsck.sh redirects output of a failing "git fsck" invocation to
files "out" and "err" to assert presence of error messages in the output
of the command.  Commit 31deb28f5e (fsck: don't hard die on invalid
object types, 2021-10-01) changed the way assertions in this test are
performed.  The test doesn't compare the whole standard error with
prepared file "err.expect" and it doesn't assert that standard output is
empty.

Don't create unused files "err.expect" and "out" in test 'fsck error and
recovery on invalid object type'.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1450-fsck.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index bca46378b2..8c442adb1a 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '
 
 		garbage_blob=$(git hash-object --stdin -w -t garbage --literally </dev/null) &&
 
-		cat >err.expect <<-\EOF &&
-		fatal: invalid object type
-		EOF
-		test_must_fail git fsck >out 2>err &&
+		test_must_fail git fsck 2>err &&
 		grep -e "^error" -e "^fatal" err >errors &&
 		test_line_count = 1 errors &&
 		grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err
-- 
2.40.0

[PATCH v3 5/6] t1502: don't create unused files

[Andrei Rybak]

Three tests in file t1502-rev-parse-parseopt.sh use three redirections
with invocation of "git rev-parse --parseopt --".  All three tests
redirect standard output to file "out" and file "spec" to standard
input.  Two of the tests redirect standard output a second time to file
"actual", and the third test redirects standard error to file "err".
These tests check contents of files "actual" and "err", but don't use
the files named "out" for assertions.  The two tests that redirect to
standard output twice might also be confusing to the reader.

Don't redirect standard output of "git rev-parse" to file "out" in
t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.

Acked-by: Øystein Walle <oystwa@gmail.com>
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1502-rev-parse-parseopt.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index de1d48f3ba..dd811b7fb4 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|EOF
 	END_EXPECT
 
-	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
+	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'test --parseopt invalid opt-spec' '
 	test_write_lines x -- "=, x" >spec &&
 	echo "fatal: missing opt-spec before option flags" >expect &&
-	test_must_fail git rev-parse --parseopt -- >out <spec 2>err &&
+	test_must_fail git rev-parse --parseopt -- <spec 2>err &&
 	test_cmp expect err
 '
 
@@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|EOF
 	END_EXPECT
 
-	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
+	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
 	test_cmp expect actual
 '
 
-- 
2.40.0

[PATCH v3 6/6] t2019: don't create unused files

[Andrei Rybak]

Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of
"git checkout" to files "stdout" and "stderr".  Several assertions are
made using file "stderr".  File "stdout", however, is unused.

Don't redirect standard output of "git checkout" to file "stdout" in
t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.

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

diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh
index 2c8c926b4d..9540588664 100755
--- a/t/t2019-checkout-ambiguous-ref.sh
+++ b/t/t2019-checkout-ambiguous-ref.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '
 '
 
 test_expect_success 'checkout ambiguous ref succeeds' '
-	git checkout ambiguity >stdout 2>stderr
+	git checkout ambiguity 2>stderr
 '
 
 test_expect_success 'checkout produces ambiguity warning' '
@@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '
 '
 
 test_expect_success 'checkout vague ref succeeds' '
-	git checkout vagueness >stdout 2>stderr &&
+	git checkout vagueness 2>stderr &&
 	test_set_prereq VAGUENESS_SUCCESS
 '
 
-- 
2.40.0

[PATCH v2 0/3] git config tests for "'git config ignores pairs ..."

[Andrei Rybak]

v1 cover letter:
  https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/

While preparing v2, I've realized that I've failed to click "Reply All" when
writing my previous email in this subthread.  I apologize.
  https://lore.kernel.org/git/CACayv=jL4t3cUVS=xXQ3fLxF26vDXRJ3khs2y4UjzBw947JVkw@mail.gmail.com/

Changes since v2:
  - Rewritten commit message for patch 2.
  - New RFC patch 3.

Andrei Rybak (3):
  t1300: drop duplicate test
  t1300: check stderr for "ignores pairs" tests
  t1300: add tests for missing keys

 t/t1300-config.sh | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

-- 
2.40.0

[PATCH v2 1/3] t1300: drop duplicate test

[Andrei Rybak]

There are two almost identical tests called 'git config ignores pairs
with zero count' in file t1300-config.sh.  Drop the first of these and
keep the one that contains more assertions.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1300-config.sh | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 2575279ab8..696dca17c6 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1458,13 +1458,6 @@ test_expect_success 'git config ignores pairs without count' '
 	test_must_be_empty error
 '
 
-test_expect_success 'git config ignores pairs with zero count' '
-	test_must_fail env \
-		GIT_CONFIG_COUNT=0 \
-		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one
-'
-
 test_expect_success 'git config ignores pairs exceeding count' '
 	GIT_CONFIG_COUNT=1 \
 		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-- 
2.40.0

[PATCH v2 2/3] t1300: check stderr for "ignores pairs" tests

[Andrei Rybak]

Tests "git config ignores pairs ..." in t1300-config.sh validate that
"git config" ignores various kinds of supplied pairs of environment
variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* depending on
GIT_CONFIG_COUNT.  By "ignores" here we mean that "git config" abides by
the value of environment variable GIT_CONFIG_COUNT and doesn't use
key-value pairs outside of the supplied GIT_CONFIG_COUNT when trying to
produce a value for config key "pair.one".

These tests also validate that "git config" doesn't complain about
mismatched environment variables to standard error.  This is validated
by redirecting the standard error to a file called "error" and asserting
that it is empty.  However, two of these tests incorrectly redirect to
standard output while calling the file "error", and test 'git config
ignores pairs exceeding count' doesn't validate standard error at all.

Fix these tests by redirecting standard error to file "error" and
asserting its emptiness.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 696dca17c6..20a15ede5c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '
 	GIT_CONFIG_COUNT=1 \
 		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
 		GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \
-		git config --get-regexp "pair.*" >actual &&
+		git config --get-regexp "pair.*" >actual 2>error &&
 	cat >expect <<-EOF &&
 	pair.one value
 	EOF
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_be_empty error
 '
 
 test_expect_success 'git config ignores pairs with zero count' '
 	test_must_fail env \
 		GIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one >error &&
+		git config pair.one 2>error &&
 	test_must_be_empty error
 '
 
 test_expect_success 'git config ignores pairs with empty count' '
 	test_must_fail env \
 		GIT_CONFIG_COUNT= GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one >error &&
+		git config pair.one 2>error &&
 	test_must_be_empty error
 '
 
-- 
2.40.0

[RFC PATCH v2 3/3] t1300: add tests for missing keys

[Andrei Rybak]

There several tests in t1300-config.sh that validate failing invocations
of "git config".  However, there are no tests that check what happens
when "git config" is asked to retrieve a value for a missing key.

Add tests that check this for various combinations of "<section>.<key>".
---

The patch is marked as RFC, because I'm not sure if such a test is useful or
not.  Even if it is not useful as a test, maybe it would be useful as
documentation of behavior for people reading "t1300-config.sh"?

Having multiple tests for different kinds of keys might also be overkill, and
just one test for a single key is enough.  Conversely, there aren't enough
tests -- tests for keys with subsections are missing.

 t/t1300-config.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 20a15ede5c..a646ddd231 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -98,6 +98,20 @@ test_expect_success 'subsections are not canonicalized by git-config' '
 	test_cmp_config two section.SubSection.key
 '
 
+test_missing_key () {
+	local key=$1
+	local title=$2
+	test_expect_success "value for $title is not printed" "
+		test_must_fail git config $key >out 2>err &&
+		test_must_be_empty out &&
+		test_must_be_empty err
+	"
+}
+
+test_missing_key 'missingsection.missingkey' 'missing section and missing key'
+test_missing_key 'missingsection.penguin' 'missing section and existing key'
+test_missing_key 'section.missingkey' 'existing section and missing key'
+
 cat > .git/config <<\EOF
 [alpha]
 bar = foo
-- 
2.40.0

Re: [PATCH v2 0/3] git config tests for "'git config ignores pairs ..."

[Andrei Rybak]

On 18/04/2023 19:50, Andrei Rybak wrote:
> v1 cover letter:
>    https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/
> 
> While preparing v2, I've realized that I've failed to click "Reply All" when
> writing my previous email in this subthread.  I apologize.
>    https://lore.kernel.org/git/CACayv=jL4t3cUVS=xXQ3fLxF26vDXRJ3khs2y4UjzBw947JVkw@mail.gmail.com/
> 
> Changes since v2:

Sorry, I meant to say "since v1" here.

>    - Rewritten commit message for patch 2.
>    - New RFC patch 3.
> 
> Andrei Rybak (3):
>    t1300: drop duplicate test
>    t1300: check stderr for "ignores pairs" tests
>    t1300: add tests for missing keys
> 
>   t/t1300-config.sh | 30 +++++++++++++++++++-----------
>   1 file changed, 19 insertions(+), 11 deletions(-)
> 

Re: [PATCH v2 1/3] t1300: drop duplicate test

[Junio C Hamano]

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

> There are two almost identical tests called 'git config ignores pairs
> with zero count' in file t1300-config.sh.  Drop the first of these and
> keep the one that contains more assertions.

Good eyes.  I can see that the other one catches the error output
from the identical command and makes sure it exits silently.

Will queue.  Thanks.


> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1300-config.sh | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 2575279ab8..696dca17c6 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1458,13 +1458,6 @@ test_expect_success 'git config ignores pairs without count' '
>  	test_must_be_empty error
>  '
>  
> -test_expect_success 'git config ignores pairs with zero count' '
> -	test_must_fail env \
> -		GIT_CONFIG_COUNT=0 \
> -		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
> -		git config pair.one
> -'
> -
>  test_expect_success 'git config ignores pairs exceeding count' '
>  	GIT_CONFIG_COUNT=1 \
>  		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \

Re: [PATCH v2 2/3] t1300: check stderr for "ignores pairs" tests

[Junio C Hamano]

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

> Tests "git config ignores pairs ..." in t1300-config.sh validate that
> "git config" ignores various kinds of supplied pairs of environment
> variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* depending on
> GIT_CONFIG_COUNT.  By "ignores" here we mean that "git config" abides by
> the value of environment variable GIT_CONFIG_COUNT and doesn't use
> key-value pairs outside of the supplied GIT_CONFIG_COUNT when trying to
> produce a value for config key "pair.one".

Correct.

>  	GIT_CONFIG_COUNT=1 \
>  		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
>  		GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \
> -		git config --get-regexp "pair.*" >actual &&
> +		git config --get-regexp "pair.*" >actual 2>error &&
>  	cat >expect <<-EOF &&
>  	pair.one value
>  	EOF
> -	test_cmp expect actual
> +	test_cmp expect actual &&
> +	test_must_be_empty error
>  '

Looks good.

>  test_expect_success 'git config ignores pairs with zero count' '
>  	test_must_fail env \
>  		GIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
> -		git config pair.one >error &&
> +		git config pair.one 2>error &&
>  	test_must_be_empty error
>  '

Looks good too.

>  test_expect_success 'git config ignores pairs with empty count' '
>  	test_must_fail env \
>  		GIT_CONFIG_COUNT= GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
> -		git config pair.one >error &&
> +		git config pair.one 2>error &&
>  	test_must_be_empty error
>  '

Looks good too.

Will queue.  Thanks.

Re: [RFC PATCH v2 3/3] t1300: add tests for missing keys

[Junio C Hamano]

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

> +test_missing_key () {
> +	local key=$1
> +	local title=$2
> +	test_expect_success "value for $title is not printed" "
> +		test_must_fail git config $key >out 2>err &&
> +		test_must_be_empty out &&
> +		test_must_be_empty err
> +	"
> +}

In this case it would not make any difference only because no caller
will feed $key that can be split at $IFS, but it is a good practice
to enclose the executed body of test_expect_* script inside a single
quote *and* let variable substitution to happen inside eval; in
other words, you should write the above like so:

(GOOD)	test_expect_success "value for $title is not printed" '
		test_must_fail git config "$key" >out 2>err &&
		test_must_be_empty out &&
		test_must_be_empty err
	'

The difference is a bit subtle.  In the original version, the string
fed to "eval" when "test_expect_success" runs has $key already
substituted.  So if $key has $IFS whitespace in it, the command line
arguments of the "git config" would not be what you think your callers
are feeding.  An attempt to keep it together by enclosing inside a
double quote, still trying to salvage the pattern to enclose the
executed body inside a pair of double quotes, i.e.

(BAD)	test_expect_success "value for $title is not printed" "
		test_must_fail git config \"$key\" >out 2>err &&
		test_must_be_empty out &&
		test_must_be_empty err
	"

would fail when $key has an unbalanced double quotes and cause a
syntax error.

Again, in this case it would be OK due to the limited callers, but
it is a good discipline to follow, as others less familiar with our
tests scripts than you would copy from what you write in your
patches.

The title string (e.g. "value for $title is not printed") does not
go through eval, and it needs to be quoted with double quotes for
the $title to be substituted, by the way.

> +test_missing_key 'missingsection.missingkey' 'missing section and missing key'
> +test_missing_key 'missingsection.penguin' 'missing section and existing key'
> +test_missing_key 'section.missingkey' 'existing section and missing key'

[PATCH v3 0/3] git config tests for "'git config ignores pairs ..."

[Andrei Rybak]

Clean up existing tests and add new tests in t1300-config.sh.

v1 cover letter:
  https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/
v2 cover letter:
  https://lore.kernel.org/git/20230418175034.982433-1-rybak.a.v@gmail.com/

Changes since v2:
  - Patch 3 updates:
    - Added missing word "are" in commit message
    - Added signoff
    - Wrapped $1 and $2 in double quotes for consistency with
      `git grep -P 'local .*="?[$][1-9]"? &&$' -- t`
    - Added &&-chainging to function "test_missing_key"
    - Fixed quoting so that substitution of $key happens inside eval
    - Added tests about config subsections

Changes since v1:
  - Rewritten commit message for patch 2.
  - New RFC patch 3.

Andrei Rybak (3):
  t1300: drop duplicate test
  t1300: check stderr for "ignores pairs" tests
  t1300: add tests for missing keys

 t/t1300-config.sh | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

Range diff against v2:
1:  cf812f4fa8 = 1:  cf812f4fa8 t1300: drop duplicate test
2:  b4132adea8 = 2:  b4132adea8 t1300: check stderr for "ignores pairs" tests
3:  6126f42449 ! 3:  ba536bf855 t1300: add tests for missing keys
    @@ Metadata
      ## Commit message ##
         t1300: add tests for missing keys
     
    -    There several tests in t1300-config.sh that validate failing invocations
    -    of "git config".  However, there are no tests that check what happens
    -    when "git config" is asked to retrieve a value for a missing key.
    +    There are several tests in t1300-config.sh that validate failing
    +    invocations of "git config".  However, there are no tests that check
    +    what happens when "git config" is asked to retrieve a value for a
    +    missing key.
     
    -    Add tests that check this for various combinations of "<section>.<key>".
    +    Add tests that check this for various combinations of "<section>.<key>"
    +    and "<section>.<subsection>.<key>".
    +
    +    Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
     
      ## t/t1300-config.sh ##
     @@ t/t1300-config.sh: test_expect_success 'subsections are not canonicalized by git-config' '
    @@ t/t1300-config.sh: test_expect_success 'subsections are not canonicalized by git
      '
      
     +test_missing_key () {
    -+	local key=$1
    -+	local title=$2
    -+	test_expect_success "value for $title is not printed" "
    -+		test_must_fail git config $key >out 2>err &&
    ++	local key="$1" &&
    ++	local title="$2" &&
    ++	test_expect_success "value for $title is not printed" '
    ++		test_must_fail git config "$key" >out 2>err &&
     +		test_must_be_empty out &&
     +		test_must_be_empty err
    -+	"
    ++	'
     +}
     +
     +test_missing_key 'missingsection.missingkey' 'missing section and missing key'
     +test_missing_key 'missingsection.penguin' 'missing section and existing key'
     +test_missing_key 'section.missingkey' 'existing section and missing key'
    ++test_missing_key 'section.MissingSubSection.missingkey' 'missing subsection and missing key'
    ++test_missing_key 'section.SubSection.missingkey' 'existing subsection and missing key'
    ++test_missing_key 'section.MissingSubSection.key' 'missing subsection and existing key'
     +
      cat > .git/config <<\EOF
      [alpha]

-- 
2.40.0

[PATCH v3 1/3] t1300: drop duplicate test

[Andrei Rybak]

There are two almost identical tests called 'git config ignores pairs
with zero count' in file t1300-config.sh.  Drop the first of these and
keep the one that contains more assertions.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t1300-config.sh | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 2575279ab8..696dca17c6 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1458,13 +1458,6 @@ test_expect_success 'git config ignores pairs without count' '
 	test_must_be_empty error
 '
 
-test_expect_success 'git config ignores pairs with zero count' '
-	test_must_fail env \
-		GIT_CONFIG_COUNT=0 \
-		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one
-'
-
 test_expect_success 'git config ignores pairs exceeding count' '
 	GIT_CONFIG_COUNT=1 \
 		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-- 
2.40.0

[PATCH v3 2/3] t1300: check stderr for "ignores pairs" tests

[Andrei Rybak]

Tests "git config ignores pairs ..." in t1300-config.sh validate that
"git config" ignores various kinds of supplied pairs of environment
variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* depending on
GIT_CONFIG_COUNT.  By "ignores" here we mean that "git config" abides by
the value of environment variable GIT_CONFIG_COUNT and doesn't use
key-value pairs outside of the supplied GIT_CONFIG_COUNT when trying to
produce a value for config key "pair.one".

These tests also validate that "git config" doesn't complain about
mismatched environment variables to standard error.  This is validated
by redirecting the standard error to a file called "error" and asserting
that it is empty.  However, two of these tests incorrectly redirect to
standard output while calling the file "error", and test 'git config
ignores pairs exceeding count' doesn't validate standard error at all.

Fix these tests by redirecting standard error to file "error" and
asserting its emptiness.

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 696dca17c6..20a15ede5c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '
 	GIT_CONFIG_COUNT=1 \
 		GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
 		GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \
-		git config --get-regexp "pair.*" >actual &&
+		git config --get-regexp "pair.*" >actual 2>error &&
 	cat >expect <<-EOF &&
 	pair.one value
 	EOF
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_be_empty error
 '
 
 test_expect_success 'git config ignores pairs with zero count' '
 	test_must_fail env \
 		GIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one >error &&
+		git config pair.one 2>error &&
 	test_must_be_empty error
 '
 
 test_expect_success 'git config ignores pairs with empty count' '
 	test_must_fail env \
 		GIT_CONFIG_COUNT= GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
-		git config pair.one >error &&
+		git config pair.one 2>error &&
 	test_must_be_empty error
 '
 
-- 
2.40.0

[PATCH v3 3/3] t1300: add tests for missing keys

[Andrei Rybak]

There are several tests in t1300-config.sh that validate failing
invocations of "git config".  However, there are no tests that check
what happens when "git config" is asked to retrieve a value for a
missing key.

Add tests that check this for various combinations of "<section>.<key>"
and "<section>.<subsection>.<key>".

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

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 20a15ede5c..423948f384 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -98,6 +98,23 @@ test_expect_success 'subsections are not canonicalized by git-config' '
 	test_cmp_config two section.SubSection.key
 '
 
+test_missing_key () {
+	local key="$1" &&
+	local title="$2" &&
+	test_expect_success "value for $title is not printed" '
+		test_must_fail git config "$key" >out 2>err &&
+		test_must_be_empty out &&
+		test_must_be_empty err
+	'
+}
+
+test_missing_key 'missingsection.missingkey' 'missing section and missing key'
+test_missing_key 'missingsection.penguin' 'missing section and existing key'
+test_missing_key 'section.missingkey' 'existing section and missing key'
+test_missing_key 'section.MissingSubSection.missingkey' 'missing subsection and missing key'
+test_missing_key 'section.SubSection.missingkey' 'existing subsection and missing key'
+test_missing_key 'section.MissingSubSection.key' 'missing subsection and existing key'
+
 cat > .git/config <<\EOF
 [alpha]
 bar = foo
-- 
2.40.0

Re: [PATCH v3 0/6] t: fix unused files, part 2

[Junio C Hamano]

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

> Creation of files from redirecting output of Git commands in tests has been
> removed for files which aren't being used for assertions.  CC'ed are authors of
> the affected tests.
>
> v1 cover letter:
>   https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/
> v2 cover letter:
>   https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/

This round has not seen any further comments; shall we consider it
pretty much done and ready to move to 'next' by now?

Thanks.

Re: [PATCH v3 0/3] git config tests for "'git config ignores pairs ..."

[Junio C Hamano]

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

> Clean up existing tests and add new tests in t1300-config.sh.
>
> v1 cover letter:
>   https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/
> v2 cover letter:
>   https://lore.kernel.org/git/20230418175034.982433-1-rybak.a.v@gmail.com/

This hasn't seen any further reactions.  Is everybody happy to see
us declare victory and merge it down to 'next'?

Thanks.

Re: [PATCH v3 0/3] git config tests for "'git config ignores pairs ..."

[Andrei Rybak]

On 02/05/2023 00:02, Junio C Hamano wrote:
> Andrei Rybak <rybak.a.v@gmail.com> writes:
> 
>> Clean up existing tests and add new tests in t1300-config.sh.
>>
>> v1 cover letter:
>>    https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/
>> v2 cover letter:
>>    https://lore.kernel.org/git/20230418175034.982433-1-rybak.a.v@gmail.com/
> 
> This hasn't seen any further reactions.  Is everybody happy to see
> us declare victory and merge it down to 'next'?

I'm happy with this series, especially because new tests in patch 3/3 indirectly
address some of the concerns for unused files removal in t1300 in the original
series of this thread, "t: fix unused files, part 2".

   https://lore.kernel.org/git/230406.86pm8htnfk.gmgdl@evledraar.gmail.com/

> Thanks.

Re: [PATCH v3 0/6] t: fix unused files, part 2

[Andrei Rybak]

On 01/05/2023 23:52, Junio C Hamano wrote:
> Andrei Rybak <rybak.a.v@gmail.com> writes:
> 
>> Creation of files from redirecting output of Git commands in tests has been
>> removed for files which aren't being used for assertions.  CC'ed are authors of
>> the affected tests.
>>
>> v1 cover letter:
>>    https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/
>> v2 cover letter:
>>    https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/
> 
> This round has not seen any further comments; shall we consider it
> pretty much done and ready to move to 'next' by now?

In general, I'm OK with the series as is.

While answering Ævar's questions to some of the patches in v2, I
went quite deep in trying to investigate what is and isn't important
to validate/assert in particular tests, but I haven't come up with
a good way to include this information in commit messages for this
series.

Notes per patch:

   - 1/6 for t0300 is just an explanation about why one out of three
     cases in one test does not check stdout (and doesn't need to).
     https://lore.kernel.org/git/db2de983-9b1f-5efb-0fdc-cc704e6b875b@gmail.com/

   - 3/6 for t1300 lead to a separate series
     https://lore.kernel.org/git/20230423134649.431783-1-rybak.a.v@gmail.com/

   - 4/6 for t1450 had an idea for a test 'fresh repository has no
     dangling objects'.  I'm doubtful about usefulness of such a test,
     so hasn't sent it as a patch yet.
     https://lore.kernel.org/git/35bc2dc5-d5cb-3492-ff94-41b93b7563d4@gmail.com/

   - 6/6 for t2019 -- a dive into how output of "git checkout" is tested
     https://lore.kernel.org/git/4ef5464b-31dd-3c3e-05be-9891162e4f05@gmail.com/#t

Patches 2/6 and 5/6 are different from others, because they fix
more obvious issues.

> Thanks.

Re: [PATCH v3 0/6] t: fix unused files, part 2

[Elijah Newren]

On Mon, May 1, 2023 at 3:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Andrei Rybak <rybak.a.v@gmail.com> writes:
>
> > Creation of files from redirecting output of Git commands in tests has been
> > removed for files which aren't being used for assertions.  CC'ed are authors of
> > the affected tests.
> >
> > v1 cover letter:
> >   https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/
> > v2 cover letter:
> >   https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/
>
> This round has not seen any further comments; shall we consider it
> pretty much done and ready to move to 'next' by now?

I think so.  I read through the series.  I also read Ævar's and
Andrei's extended comments on v2.  Ævar does bring up good points
about whether we should be testing more, but Andrei I think did a good
investigation, cc'ed original code authors (who would be the right
ones to comment on whether those other things should be tested), etc.
The tests as-is before this series are harder than necessary to
understand, and Andrei cleans them up.  It feels like good forward
progress to me, even if there _might_ be a better eventual optimal.

Reviewed-by: Elijah Newren <newren@gmail.com>

Re: [PATCH v3 0/6] t: fix unused files, part 2

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Mon, May 1, 2023 at 3:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Andrei Rybak <rybak.a.v@gmail.com> writes:
>>
>> > Creation of files from redirecting output of Git commands in tests has been
>> > removed for files which aren't being used for assertions.  CC'ed are authors of
>> > the affected tests.
>> >
>> > v1 cover letter:
>> >   https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/
>> > v2 cover letter:
>> >   https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/
>>
>> This round has not seen any further comments; shall we consider it
>> pretty much done and ready to move to 'next' by now?
> 
> I think so.  ...
>
> Reviewed-by: Elijah Newren <newren@gmail.com>

Thanks.