patch, 6 partst: fix unused files, part 2

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

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

On Sat, Apr 1, 2023 at 5:34 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:

s/contaisn/contains/

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

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

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

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

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

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

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

On Tue, Apr 04 2023, Andrei Rybak wrote:

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?

On 06/04/2023 10:34, Ævar Arnfjörð Bjarmason wrote:

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/

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

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

... these partial URLs ...

... but doesn't match these.

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

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.

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

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

On Tue, Apr 04 2023, Andrei Rybak wrote:

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

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

On 06/04/2023 10:41, Ævar Arnfjörð Bjarmason wrote:

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:

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?

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

On Tue, Apr 04 2023, Andrei Rybak wrote:

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?

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

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:

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)

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

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:

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

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

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.

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

Hi Andrei

On Tue, 4 Apr 2023 at 00:33, Andrei Rybak <rybak.a.v@gmail.com> wrote:

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

On Tue, Apr 04 2023, Andrei Rybak wrote:

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

On 06/04/2023 10:47, Ævar Arnfjörð Bjarmason wrote:

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

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

On Tue, Apr 04 2023, Andrei Rybak 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?

On 06/04/2023 10:38, Ævar Arnfjörð Bjarmason wrote:

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.

On 06/04/2023 23:30, 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?

On 2023-04-06T23:35, Andrei Rybak wrote:

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

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

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

On Fri, 14 Apr 2023 at 10:13, Andrei Rybak <rybak.a.v@gmail.com> wrote:

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.

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

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

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

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

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.

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

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

Correct.

Looks good.

Looks good too.

Looks good too.

Will queue.  Thanks.

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

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

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.

On 18/04/2023 19:50, Andrei Rybak wrote:

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

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]

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

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

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

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

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

Thanks.

On 02/05/2023 00:02, Junio C Hamano wrote:

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/

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

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

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

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

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

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

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

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

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

Thanks.

On 01/05/2023 23:52, Junio C Hamano wrote:

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.

On Mon, May 1, 2023 at 3:04 PM Junio C Hamano <gitster@pobox.com> wrote:

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>

Elijah Newren <newren@gmail.com> writes:

Thanks.