{"thread":{"id":"59519","subject":"[PATCH v1 0/6] t: fix unused files, part 2","startedAt":"2023-04-01T21:29:07Z","lastAt":"2023-05-03T15:55:05Z","messageCount":58,"participants":["Andrei Rybak","Eric Sunshine","Øystein Walle","Ævar Arnfjörð Bjarmason","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"474644","messageId":"20230401212858.266508-1-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":null,"subject":"[PATCH v1 0/6] t: fix unused files, part 2","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-01T21:28:52Z","receivedAt":"2023-04-01T21:29:07Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Creation of files from redirecting output of Git commands in tests has been\nremoved for files which aren't being used for assertions. CC'ed are authors of\nthe affected tests.\n\nThis is a continuation of part 1:\n https://lore.kernel.org/git/20230312201520.370234-1-rybak.a.v@gmail.com/T/#u\n\nOverall, this is similar to part 1, except that additionally in patch 4/6\ncreation of an unused file that isn't output of a git command has been removed.\nPatch 2/6 fixes broken comments, found during development of this series.\n\nSince part 1 I've upgraded my 'check_unused_files.py' script (invoked in the\nsame way as in part 1) and found a bit more issues in t0??? and t1???, and\noverall have gone up to t2???.\n\nUpgraded script:\n\n from sys import argv\n import re\n\n script = argv[1]\n\n git_with_outputs_pattern = re.compile('git [^&\\\"]*?[>](?!/)([-a-z_./]*)( 2[>](?!/)([a-z_.-]*))?')\n\n out_read_index = -1\n err_read_index = -1\n\n def clean_up_filename(fn):\n if fn and '/' in fn:\n from os import path\n return path.basename(fn)\n return fn\n\n while True:\n res = git_with_outputs_pattern.search(script)\n if res is None:\n break\n end_index = res.span()[1]\n out_filename = clean_up_filename(res.group(1))\n err_filename = clean_up_filename(res.group(3))\n\n script = script[end_index:]\n\n if out_filename:\n out_read_index = script.find(out_filename)\n if out_read_index < 0:\n print(\"File '\" + out_filename + \"' is unused\")\n print(\"Script: \")\n print(script)\n from sys import exit\n exit(1)\n if err_filename and err_filename != '&1':\n err_read_index = script.find(err_filename)\n if err_read_index < 0:\n print(\"File '\" + err_filename + \"' is unused\")\n print(\"Script: \")\n print(script)\n from sys import exit\n exit(2)\n if out_read_index >= 0:\n script = script[out_read_index + len(out_filename):]\n elif err_read_index >= 0:\n script = script[err_read_index + len(err_filename):]\n\nAndrei Rybak (6):\n t0300: don't create unused file\n t1300: fix config file syntax error descriptions\n t1300: don't create unused files\n t1450: don't create unused files\n t1502: don't create unused files\n t2019: don't create unused files\n\n t/t0300-credentials.sh | 2 +-\n t/t1300-config.sh | 10 +++++-----\n t/t1450-fsck.sh | 5 +----\n t/t1502-rev-parse-parseopt.sh | 6 +++---\n t/t2019-checkout-ambiguous-ref.sh | 4 ++--\n 5 files changed, 12 insertions(+), 15 deletions(-)\n\n-- \n2.40.0\n\n"},{"id":"474645","messageId":"20230401212858.266508-2-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230401212858.266508-1-rybak.a.v@gmail.com","subject":"[PATCH v1 1/6] t0300: don't create unused file","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-01T21:28:53Z","receivedAt":"2023-04-01T21:29:11Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Test 'credential config with partial URLs' in t0300-credentials.sh\ncontaisn three \"git credential fill\" invocations. For two of the\ninvocations, the test asserts presence or absence of string \"yep\" in the\nstandard output. For the third test it checks for an error message in\nstandard error.\n\nDon't redirect standard output of \"git credential\" to file \"stdout\" in\nt0300-credentials.sh to avoid creating an unnecessary file when only\nstandard error is checked.\n\nSigned-off-by: Andrei Rybak \n---\n t/t0300-credentials.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex c66d91e82d..b8612ede95 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '\n \n \tgit -c credential.$partial.helper=yep \\\n \t\t-c credential.with%0anewline.username=uh-oh \\\n-\t\tcredential fill stdout 2>stderr &&\n+\t\tcredential fill stderr &&\n \ttest_i18ngrep \"skipping credential lookup for key\" stderr\n '\n \n-- \n2.40.0\n\n"},{"id":"474646","messageId":"20230401212858.266508-6-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230401212858.266508-1-rybak.a.v@gmail.com","subject":"[PATCH v1 5/6] t1502: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-01T21:28:57Z","receivedAt":"2023-04-01T21:29:12Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in file t1502-rev-parse-parseopt.sh use three redirections\nwith invocation of \"git rev-parse --parseopt --\". All three tests\nredirect standard output to file \"out\" and file \"spec\" to standard\ninput. Two of the tests redirect standard output a second time to file\n\"actual\", and the third test redirects standard error to file \"err\".\nThese tests check contents of files \"actual\" and \"err\", but don't use\nthe files named \"out\" for assertions. The two tests that redirect to\nstandard output twice might also be confusing to the reader.\n\nDon't redirect standard output of \"git rev-parse\" to file \"out\" in\nt1502-rev-parse-parseopt.sh to avoid creating unnecessary files.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1502-rev-parse-parseopt.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex de1d48f3ba..dd811b7fb4 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \t|EOF\n \tEND_EXPECT\n \n-\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'test --parseopt invalid opt-spec' '\n \ttest_write_lines x -- \"=, x\" >spec &&\n \techo \"fatal: missing opt-spec before option flags\" >expect &&\n-\ttest_must_fail git rev-parse --parseopt -- >out err &&\n+\ttest_must_fail git rev-parse --parseopt -- err &&\n \ttest_cmp expect err\n '\n \n@@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n \t|EOF\n \tEND_EXPECT\n \n-\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.40.0\n\n"},{"id":"474647","messageId":"20230401212858.266508-3-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230401212858.266508-1-rybak.a.v@gmail.com","subject":"[PATCH v1 2/6] t1300: fix config file syntax error descriptions","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-01T21:28:54Z","receivedAt":"2023-04-01T21:29:12Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in t1300-config.sh check that \"git config --get\" barfs when\nthe config file contains various syntax errors: key=value pair without\nequals sign, broken section line, and broken value string. The sample\nconfig files include a comment describing the kind of broken syntax.\nThis description seems to have been copy-pasted from the \"broken section\nline\" sample to the other two samples.\n\nFix descriptions of broken config file syntax in samples used in\nt1300-config.sh.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 2575279ab8..d566729d74 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1571,7 +1571,7 @@ test_expect_success 'git config --edit respects core.editor' '\n # malformed configuration files\n test_expect_success 'barf on syntax error' '\n \tcat >.git/config <<-\\EOF &&\n-\t# broken section line\n+\t# broken key=value\n \t[section]\n \tkey garbage\n \tEOF\n@@ -1591,7 +1591,7 @@ test_expect_success 'barf on incomplete section header' '\n \n test_expect_success 'barf on incomplete string' '\n \tcat >.git/config <<-\\EOF &&\n-\t# broken section line\n+\t# broken value string\n \t[section]\n \tkey = \"value string\n \tEOF\n-- \n2.40.0\n\n"},{"id":"474648","messageId":"20230401212858.266508-4-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230401212858.266508-1-rybak.a.v@gmail.com","subject":"[PATCH v1 3/6] t1300: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-01T21:28:55Z","receivedAt":"2023-04-01T21:29:12Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in t1300-config.sh check that \"git config --get\" barfs when\nsyntax errors are present in the config file. The tests redirect\nstandard output and standard error of \"git config --get\" to files,\n\"actual\" and \"error\" correspondingly. They assert presence of an error\nmessage in file \"error\". However, these tests don't use file \"actual\"\nfor assertions.\n\nDon't redirect standard output of \"git config --get\" to file \"actual\" in\nt1300-config.sh to avoid creating unnecessary files.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex d566729d74..8ac4531c1b 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '\n \t[section]\n \tkey garbage\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 3 \" error\n '\n \n@@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '\n \t[section\n \tkey = value\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 2 \" error\n '\n \n@@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '\n \t[section]\n \tkey = \"value string\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 3 \" error\n '\n \n-- \n2.40.0\n\n"},{"id":"474649","messageId":"20230401212858.266508-5-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230401212858.266508-1-rybak.a.v@gmail.com","subject":"[PATCH v1 4/6] t1450: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-01T21:28:56Z","receivedAt":"2023-04-01T21:29:17Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Test 'fsck error and recovery on invalid object type' in file\nt1450-fsck.sh redirects output of a failing \"git fsck\" invocation to\nfiles \"out\" and \"err\" to assert presence of error messages in the output\nof the command. Commit 31deb28f5e (fsck: don't hard die on invalid\nobject types, 2021-10-01) changed the way assertions in this test are\nperformed. The test doesn't compare the whole standard error with\nprepared file \"err.expect\" and it doesn't assert that standard output is\nempty.\n\nDon't create unused files \"err.expect\" and \"out\" in test 'fsck error and\nrecovery on invalid object type'.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1450-fsck.sh | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex bca46378b2..8c442adb1a 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '\n \n \t\tgarbage_blob=$(git hash-object --stdin -w -t garbage --literally err.expect <<-\\EOF &&\n-\t\tfatal: invalid object type\n-\t\tEOF\n-\t\ttest_must_fail git fsck >out 2>err &&\n+\t\ttest_must_fail git fsck 2>err &&\n \t\tgrep -e \"^error\" -e \"^fatal\" err >errors &&\n \t\ttest_line_count = 1 errors &&\n \t\tgrep \"$garbage_blob: object is of unknown type '\"'\"'garbage'\"'\"':\" err\n-- \n2.40.0\n\n"},{"id":"474650","messageId":"20230401212858.266508-7-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230401212858.266508-1-rybak.a.v@gmail.com","subject":"[PATCH v1 6/6] t2019: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-01T21:28:58Z","receivedAt":"2023-04-01T21:29:19Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of\n\"git checkout\" to files \"stdout\" and \"stderr\". Several assertions are\nmade using file \"stderr\". File \"stdout\", however, is unused.\n\nDon't redirect standard output of \"git checkout\" to file \"stdout\" in\nt2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.\n\nSigned-off-by: Andrei Rybak \n---\n t/t2019-checkout-ambiguous-ref.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh\nindex 2c8c926b4d..9540588664 100755\n--- a/t/t2019-checkout-ambiguous-ref.sh\n+++ b/t/t2019-checkout-ambiguous-ref.sh\n@@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '\n '\n \n test_expect_success 'checkout ambiguous ref succeeds' '\n-\tgit checkout ambiguity >stdout 2>stderr\n+\tgit checkout ambiguity 2>stderr\n '\n \n test_expect_success 'checkout produces ambiguity warning' '\n@@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '\n '\n \n test_expect_success 'checkout vague ref succeeds' '\n-\tgit checkout vagueness >stdout 2>stderr &&\n+\tgit checkout vagueness 2>stderr &&\n \ttest_set_prereq VAGUENESS_SUCCESS\n '\n \n-- \n2.40.0\n\n"},{"id":"474653","messageId":"CAPig+cR0itJ3x9zmfTG-4MPSfRAenO__yYZ4OVMLYT1qgsKKcg@mail.gmail.com","threadId":"59519","inReplyTo":"20230401212858.266508-2-rybak.a.v@gmail.com","subject":"Re: [PATCH v1 1/6] t0300: don't create unused file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-04-02T01:30:15Z","receivedAt":"2023-04-02T01:30:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 1, 2023 at 5:34 PM Andrei Rybak wrote:\n> Test 'credential config with partial URLs' in t0300-credentials.sh\n> contaisn three \"git credential fill\" invocations. For two of the\n> invocations, the test asserts presence or absence of string \"yep\" in the\n> standard output. For the third test it checks for an error message in\n> standard error.\n\ns/contaisn/contains/\n\n> Don't redirect standard output of \"git credential\" to file \"stdout\" in\n> t0300-credentials.sh to avoid creating an unnecessary file when only\n> standard error is checked.\n>\n> Signed-off-by: Andrei Rybak \n"},{"id":"474719","messageId":"20230403223338.468025-2-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-1-rybak.a.v@gmail.com","subject":"[PATCH v2 1/6] t0300: don't create unused file","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-03T22:33:33Z","receivedAt":"2023-04-03T22:33:49Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Test 'credential config with partial URLs' in t0300-credentials.sh\ncontains three \"git credential fill\" invocations. For two of the\ninvocations, the test asserts presence or absence of string \"yep\" in the\nstandard output. For the third test it checks for an error message in\nstandard error.\n\nDon't redirect standard output of \"git credential\" to file \"stdout\" in\nt0300-credentials.sh to avoid creating an unnecessary file when only\nstandard error is checked.\n\nSigned-off-by: Andrei Rybak \n---\n t/t0300-credentials.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex c66d91e82d..b8612ede95 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '\n \n \tgit -c credential.$partial.helper=yep \\\n \t\t-c credential.with%0anewline.username=uh-oh \\\n-\t\tcredential fill stdout 2>stderr &&\n+\t\tcredential fill stderr &&\n \ttest_i18ngrep \"skipping credential lookup for key\" stderr\n '\n \n-- \n2.40.0\n\n"},{"id":"474720","messageId":"20230403223338.468025-1-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230401212858.266508-1-rybak.a.v@gmail.com","subject":"[PATCH v2 0/6] t: fix unused files, part 2","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-03T22:33:32Z","receivedAt":"2023-04-03T22:33:51Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Creation of files from redirecting output of Git commands in tests has been\nremoved for files which aren't being used for assertions. CC'ed are authors of\nthe affected tests.\n\nv1 cover letter:\n https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/\n\nThanks to Eric Sunshine for reviewing v1.\n\nChanges since v1:\n\n - Fixed typo in commit message of 1/6 found by Eric Sunshine\n\nRange diff:\n\n1: 516cc3fe83 ! 1: 828bb18bd7 t0300: don't create unused file\n @@ Commit message\n t0300: don't create unused file\n\n Test 'credential config with partial URLs' in t0300-credentials.sh\n - contaisn three \"git credential fill\" invocations. For two of the\n + contains three \"git credential fill\" invocations. For two of the\n invocations, the test asserts presence or absence of string \"yep\" in the\n standard output. For the third test it checks for an error message in\n standard error.\n2: 9282c9bb07 = 2: a5b299a0c6 t1300: fix config file syntax error descriptions\n3: df0ec7ebf9 = 3: 806df16415 t1300: don't create unused files\n4: 408e971b43 = 4: 6742c957e5 t1450: don't create unused files\n5: 8464d0f435 = 5: 6c173a5c46 t1502: don't create unused files\n6: de2b5339d7 = 6: d508c1def3 t2019: don't create unused files\n\nAndrei Rybak (6):\n t0300: don't create unused file\n t1300: fix config file syntax error descriptions\n t1300: don't create unused files\n t1450: don't create unused files\n t1502: don't create unused files\n t2019: don't create unused files\n\n t/t0300-credentials.sh | 2 +-\n t/t1300-config.sh | 10 +++++-----\n t/t1450-fsck.sh | 5 +----\n t/t1502-rev-parse-parseopt.sh | 6 +++---\n t/t2019-checkout-ambiguous-ref.sh | 4 ++--\n 5 files changed, 12 insertions(+), 15 deletions(-)\n\n-- \n2.40.0\n\n"},{"id":"474721","messageId":"20230403223338.468025-3-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-1-rybak.a.v@gmail.com","subject":"[PATCH v2 2/6] t1300: fix config file syntax error descriptions","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-03T22:33:34Z","receivedAt":"2023-04-03T22:33:52Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in t1300-config.sh check that \"git config --get\" barfs when\nthe config file contains various syntax errors: key=value pair without\nequals sign, broken section line, and broken value string. The sample\nconfig files include a comment describing the kind of broken syntax.\nThis description seems to have been copy-pasted from the \"broken section\nline\" sample to the other two samples.\n\nFix descriptions of broken config file syntax in samples used in\nt1300-config.sh.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 2575279ab8..d566729d74 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1571,7 +1571,7 @@ test_expect_success 'git config --edit respects core.editor' '\n # malformed configuration files\n test_expect_success 'barf on syntax error' '\n \tcat >.git/config <<-\\EOF &&\n-\t# broken section line\n+\t# broken key=value\n \t[section]\n \tkey garbage\n \tEOF\n@@ -1591,7 +1591,7 @@ test_expect_success 'barf on incomplete section header' '\n \n test_expect_success 'barf on incomplete string' '\n \tcat >.git/config <<-\\EOF &&\n-\t# broken section line\n+\t# broken value string\n \t[section]\n \tkey = \"value string\n \tEOF\n-- \n2.40.0\n\n"},{"id":"474722","messageId":"20230403223338.468025-5-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-1-rybak.a.v@gmail.com","subject":"[PATCH v2 4/6] t1450: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-03T22:33:36Z","receivedAt":"2023-04-03T22:33:53Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Test 'fsck error and recovery on invalid object type' in file\nt1450-fsck.sh redirects output of a failing \"git fsck\" invocation to\nfiles \"out\" and \"err\" to assert presence of error messages in the output\nof the command. Commit 31deb28f5e (fsck: don't hard die on invalid\nobject types, 2021-10-01) changed the way assertions in this test are\nperformed. The test doesn't compare the whole standard error with\nprepared file \"err.expect\" and it doesn't assert that standard output is\nempty.\n\nDon't create unused files \"err.expect\" and \"out\" in test 'fsck error and\nrecovery on invalid object type'.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1450-fsck.sh | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex bca46378b2..8c442adb1a 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '\n \n \t\tgarbage_blob=$(git hash-object --stdin -w -t garbage --literally err.expect <<-\\EOF &&\n-\t\tfatal: invalid object type\n-\t\tEOF\n-\t\ttest_must_fail git fsck >out 2>err &&\n+\t\ttest_must_fail git fsck 2>err &&\n \t\tgrep -e \"^error\" -e \"^fatal\" err >errors &&\n \t\ttest_line_count = 1 errors &&\n \t\tgrep \"$garbage_blob: object is of unknown type '\"'\"'garbage'\"'\"':\" err\n-- \n2.40.0\n\n"},{"id":"474723","messageId":"20230403223338.468025-7-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-1-rybak.a.v@gmail.com","subject":"[PATCH v2 6/6] t2019: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-03T22:33:38Z","receivedAt":"2023-04-03T22:33:55Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of\n\"git checkout\" to files \"stdout\" and \"stderr\". Several assertions are\nmade using file \"stderr\". File \"stdout\", however, is unused.\n\nDon't redirect standard output of \"git checkout\" to file \"stdout\" in\nt2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.\n\nSigned-off-by: Andrei Rybak \n---\n t/t2019-checkout-ambiguous-ref.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh\nindex 2c8c926b4d..9540588664 100755\n--- a/t/t2019-checkout-ambiguous-ref.sh\n+++ b/t/t2019-checkout-ambiguous-ref.sh\n@@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '\n '\n \n test_expect_success 'checkout ambiguous ref succeeds' '\n-\tgit checkout ambiguity >stdout 2>stderr\n+\tgit checkout ambiguity 2>stderr\n '\n \n test_expect_success 'checkout produces ambiguity warning' '\n@@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '\n '\n \n test_expect_success 'checkout vague ref succeeds' '\n-\tgit checkout vagueness >stdout 2>stderr &&\n+\tgit checkout vagueness 2>stderr &&\n \ttest_set_prereq VAGUENESS_SUCCESS\n '\n \n-- \n2.40.0\n\n"},{"id":"474724","messageId":"20230403223338.468025-6-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-1-rybak.a.v@gmail.com","subject":"[PATCH v2 5/6] t1502: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-03T22:33:37Z","receivedAt":"2023-04-03T22:33:56Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in file t1502-rev-parse-parseopt.sh use three redirections\nwith invocation of \"git rev-parse --parseopt --\". All three tests\nredirect standard output to file \"out\" and file \"spec\" to standard\ninput. Two of the tests redirect standard output a second time to file\n\"actual\", and the third test redirects standard error to file \"err\".\nThese tests check contents of files \"actual\" and \"err\", but don't use\nthe files named \"out\" for assertions. The two tests that redirect to\nstandard output twice might also be confusing to the reader.\n\nDon't redirect standard output of \"git rev-parse\" to file \"out\" in\nt1502-rev-parse-parseopt.sh to avoid creating unnecessary files.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1502-rev-parse-parseopt.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex de1d48f3ba..dd811b7fb4 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \t|EOF\n \tEND_EXPECT\n \n-\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'test --parseopt invalid opt-spec' '\n \ttest_write_lines x -- \"=, x\" >spec &&\n \techo \"fatal: missing opt-spec before option flags\" >expect &&\n-\ttest_must_fail git rev-parse --parseopt -- >out err &&\n+\ttest_must_fail git rev-parse --parseopt -- err &&\n \ttest_cmp expect err\n '\n \n@@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n \t|EOF\n \tEND_EXPECT\n \n-\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.40.0\n\n"},{"id":"474725","messageId":"20230403223338.468025-4-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-1-rybak.a.v@gmail.com","subject":"[PATCH v2 3/6] t1300: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-03T22:33:35Z","receivedAt":"2023-04-03T22:33:58Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in t1300-config.sh check that \"git config --get\" barfs when\nsyntax errors are present in the config file. The tests redirect\nstandard output and standard error of \"git config --get\" to files,\n\"actual\" and \"error\" correspondingly. They assert presence of an error\nmessage in file \"error\". However, these tests don't use file \"actual\"\nfor assertions.\n\nDon't redirect standard output of \"git config --get\" to file \"actual\" in\nt1300-config.sh to avoid creating unnecessary files.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex d566729d74..8ac4531c1b 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '\n \t[section]\n \tkey garbage\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 3 \" error\n '\n \n@@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '\n \t[section\n \tkey = value\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 2 \" error\n '\n \n@@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '\n \t[section]\n \tkey = \"value string\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 3 \" error\n '\n \n-- \n2.40.0\n\n"},{"id":"474914","messageId":"CAFaJEqug4bghEMnEQzGDN10EqM8e8iSf5i12AvOm+NZzDCQKOw@mail.gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-6-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 5/6] t1502: don't create unused files","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2023-04-06T08:15:40Z","receivedAt":"2023-04-06T08:16:25Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Hi Andrei\n\nOn Tue, 4 Apr 2023 at 00:33, Andrei Rybak wrote:\n>\n> test_expect_success 'test --parseopt invalid opt-spec' '\n> test_write_lines x -- \"=, x\" >spec &&\n> echo \"fatal: missing opt-spec before option flags\" >expect &&\n> - test_must_fail git rev-parse --parseopt -- >out err &&\n> + test_must_fail git rev-parse --parseopt -- err &&\n> test_cmp expect err\n> '\n\nThis is the one that was touched by me. At the time I just cargo-culted other\ntests. This looks obviously correct to me\n\nFor what it's worth:\n\nAcked-by: Øystein Walle \n\nØsse\n"},{"id":"474918","messageId":"230406.86ttxttnir.gmgdl@evledraar.gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-2-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 1/6] t0300: don't create unused file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T08:34:00Z","receivedAt":"2023-04-06T08:37:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 04 2023, Andrei Rybak wrote:\n\n> Test 'credential config with partial URLs' in t0300-credentials.sh\n> contains three \"git credential fill\" invocations. For two of the\n> invocations, the test asserts presence or absence of string \"yep\" in the\n> standard output. For the third test it checks for an error message in\n> standard error.\n>\n> Don't redirect standard output of \"git credential\" to file \"stdout\" in\n> t0300-credentials.sh to avoid creating an unnecessary file when only\n> standard error is checked.\n>\n> Signed-off-by: Andrei Rybak \n> ---\n> t/t0300-credentials.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> index c66d91e82d..b8612ede95 100755\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '\n> \n> \tgit -c credential.$partial.helper=yep \\\n> \t\t-c credential.with%0anewline.username=uh-oh \\\n> -\t\tcredential fill stdout 2>stderr &&\n> +\t\tcredential fill stderr &&\n> \ttest_i18ngrep \"skipping credential lookup for key\" stderr\n> '\n\nThis goes for these changes in this series general: You're correct that\nthis is useless now, but I don't think it follows that we should be\nremoving the \"redundant\" code in all cases, rather than fixing the test\nto actually check these.\n\nE.g. this will also make this test pass:\n\t\n\tdiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n\tindex c66d91e82d8..62c2a0fd50e 100755\n\t--- a/t/t0300-credentials.sh\n\t+++ b/t/t0300-credentials.sh\n\t@@ -806,9 +806,11 @@ test_expect_success 'credential config with partial URLs' '\n\t \t\treturn 1\n\t \tdone &&\n\t \n\t+\tcp stdout stdout.last &&\n\t \tgit -c credential.$partial.helper=yep \\\n\t \t\t-c credential.with%0anewline.username=uh-oh \\\n\t \t\tcredential fill stdout 2>stderr &&\n\t+\ttest_cmp stdout.last stdout &&\n\t \ttest_i18ngrep \"skipping credential lookup for key\" stderr\n\t '\n\t \n\nDoes that make sense? No idea, I don't know the credential system well.\n\nBut isn't it worth testing that when we ask for this that we're getting\nsome known output along with the warning?\n"},{"id":"474919","messageId":"230406.86pm8htnfk.gmgdl@evledraar.gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-4-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 3/6] t1300: don't create unused files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T08:38:00Z","receivedAt":"2023-04-06T08:39:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 04 2023, Andrei Rybak wrote:\n\n> Three tests in t1300-config.sh check that \"git config --get\" barfs when\n> syntax errors are present in the config file. The tests redirect\n> standard output and standard error of \"git config --get\" to files,\n> \"actual\" and \"error\" correspondingly. They assert presence of an error\n> message in file \"error\". However, these tests don't use file \"actual\"\n> for assertions.\n>\n> Don't redirect standard output of \"git config --get\" to file \"actual\" in\n> t1300-config.sh to avoid creating unnecessary files.\n>\n> Signed-off-by: Andrei Rybak \n> ---\n> t/t1300-config.sh | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index d566729d74..8ac4531c1b 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '\n> \t[section]\n> \tkey garbage\n> \tEOF\n> -\ttest_must_fail git config --get section.key >actual 2>error &&\n> +\ttest_must_fail git config --get section.key 2>error &&\n> \ttest_i18ngrep \" line 3 \" error\n> '\n> \n> @@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '\n> \t[section\n> \tkey = value\n> \tEOF\n> -\ttest_must_fail git config --get section.key >actual 2>error &&\n> +\ttest_must_fail git config --get section.key 2>error &&\n> \ttest_i18ngrep \" line 2 \" error\n> '\n> \n> @@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '\n> \t[section]\n> \tkey = \"value string\n> \tEOF\n> -\ttest_must_fail git config --get section.key >actual 2>error &&\n> +\ttest_must_fail git config --get section.key 2>error &&\n> \ttest_i18ngrep \" line 3 \" error\n> '\n\nDitto my comment on 1/6, shouldn't we instead be doing e.g.:\n\t\n\tdiff --git a/t/t1300-config.sh b/t/t1300-config.sh\n\tindex 2575279ab84..df2070c2f09 100755\n\t--- a/t/t1300-config.sh\n\t+++ b/t/t1300-config.sh\n\t@@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' '\n\t \t[section]\n\t \tkey garbage\n\t \tEOF\n\t-\ttest_must_fail git config --get section.key >actual 2>error &&\n\t+\ttest_must_fail git config --get section.key >out 2>error &&\n\t+\ttest_must_be_empty out &&\n\t \ttest_i18ngrep \" line 3 \" error\n\t '\n\t \nI.e. before this we had no coverage on the error being the only output,\nbut seemingly by mistake. Let's just assert that, rather than dropping\nthe redirection entirely, no?\n"},{"id":"474920","messageId":"230406.86lej5tn8c.gmgdl@evledraar.gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-5-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 4/6] t1450: don't create unused files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T08:41:24Z","receivedAt":"2023-04-06T08:43:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 04 2023, Andrei Rybak wrote:\n\n> Test 'fsck error and recovery on invalid object type' in file\n> t1450-fsck.sh redirects output of a failing \"git fsck\" invocation to\n> files \"out\" and \"err\" to assert presence of error messages in the output\n> of the command. Commit 31deb28f5e (fsck: don't hard die on invalid\n> object types, 2021-10-01) changed the way assertions in this test are\n> performed. The test doesn't compare the whole standard error with\n> prepared file \"err.expect\" and it doesn't assert that standard output is\n> empty.\n\nThat's my commit, and thanks for catching this...\n\n> Don't create unused files \"err.expect\" and \"out\" in test 'fsck error and\n> recovery on invalid object type'.\n>\n> Signed-off-by: Andrei Rybak \n> ---\n> t/t1450-fsck.sh | 5 +----\n> 1 file changed, 1 insertion(+), 4 deletions(-)\n>\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index bca46378b2..8c442adb1a 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '\n> \n> \t\tgarbage_blob=$(git hash-object --stdin -w -t garbage --literally \n> -\t\tcat >err.expect <<-\\EOF &&\n> -\t\tfatal: invalid object type\n> -\t\tEOF\n> -\t\ttest_must_fail git fsck >out 2>err &&\n> +\t\ttest_must_fail git fsck 2>err &&\n> \t\tgrep -e \"^error\" -e \"^fatal\" err >errors &&\n> \t\ttest_line_count = 1 errors &&\n> \t\tgrep \"$garbage_blob: object is of unknown type '\"'\"'garbage'\"'\"':\" err\n\n...but ditto my review on other patches, this just seems like a mistake\nof mine, i.e. if I add the \"test_must_be_empty out\" the test passes.\n\nSo isn't the answer here that my 31deb28f5e had an unintentional\nregression, and we should bring the assertion back? Its commit message\nsays nothing about wanting to stop asserting stdout.\n\nMaybe there was a reason I'm missing for why I remved it, it's since\nbeen paged out of my wetware, but looking at it briefly now it just\nseems like an unintentional bug / loss of test coverage that we should\nfix.\n"},{"id":"474921","messageId":"230406.86h6tttn21.gmgdl@evledraar.gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-7-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 6/6] t2019: don't create unused files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T08:44:06Z","receivedAt":"2023-04-06T08:47:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 04 2023, Andrei Rybak wrote:\n\n> Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of\n> \"git checkout\" to files \"stdout\" and \"stderr\". Several assertions are\n> made using file \"stderr\". File \"stdout\", however, is unused.\n>\n> Don't redirect standard output of \"git checkout\" to file \"stdout\" in\n> t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.\n>\n> Signed-off-by: Andrei Rybak \n> ---\n> t/t2019-checkout-ambiguous-ref.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh\n> index 2c8c926b4d..9540588664 100755\n> --- a/t/t2019-checkout-ambiguous-ref.sh\n> +++ b/t/t2019-checkout-ambiguous-ref.sh\n> @@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '\n> '\n> \n> test_expect_success 'checkout ambiguous ref succeeds' '\n> -\tgit checkout ambiguity >stdout 2>stderr\n> +\tgit checkout ambiguity 2>stderr\n> '\n\nDitto earlier comments that we should just fix this, if I make this\n\">out\" and \"test_must_be_empty out\" this succeeds, shouldn't we just use\nthat?\n\n> test_expect_success 'checkout produces ambiguity warning' '\n\nAs an aside, we should really just combine these two tests.\n\n> @@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '\n> '\n> \n> test_expect_success 'checkout vague ref succeeds' '\n> -\tgit checkout vagueness >stdout 2>stderr &&\n> +\tgit checkout vagueness 2>stderr &&\n> \ttest_set_prereq VAGUENESS_SUCCESS\n> '\n\n"},{"id":"474922","messageId":"230406.86cz4htmzz.gmgdl@evledraar.gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-6-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 5/6] t1502: don't create unused files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T08:47:43Z","receivedAt":"2023-04-06T08:48:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 04 2023, Andrei Rybak wrote:\n\n> Three tests in file t1502-rev-parse-parseopt.sh use three redirections\n> with invocation of \"git rev-parse --parseopt --\". All three tests\n> redirect standard output to file \"out\" and file \"spec\" to standard\n> input. Two of the tests redirect standard output a second time to file\n> \"actual\", and the third test redirects standard error to file \"err\".\n> These tests check contents of files \"actual\" and \"err\", but don't use\n> the files named \"out\" for assertions. The two tests that redirect to\n> standard output twice might also be confusing to the reader.\n>\n> Don't redirect standard output of \"git rev-parse\" to file \"out\" in\n> t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.\n>\n> Signed-off-by: Andrei Rybak \n> ---\n> t/t1502-rev-parse-parseopt.sh | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n> index de1d48f3ba..dd811b7fb4 100755\n> --- a/t/t1502-rev-parse-parseopt.sh\n> +++ b/t/t1502-rev-parse-parseopt.sh\n> @@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n> \t|EOF\n> \tEND_EXPECT\n> \n> -\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n> +\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n> \ttest_cmp expect actual\n> '\n> \n> test_expect_success 'test --parseopt invalid opt-spec' '\n> \ttest_write_lines x -- \"=, x\" >spec &&\n> \techo \"fatal: missing opt-spec before option flags\" >expect &&\n> -\ttest_must_fail git rev-parse --parseopt -- >out err &&\n> +\ttest_must_fail git rev-parse --parseopt -- err &&\n> \ttest_cmp expect err\n> '\n> \n> @@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n> \t|EOF\n> \tEND_EXPECT\n> \n> -\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n> +\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n> \ttest_cmp expect actual\n> '\n\nDitto earlier comments: When we fail, we should assert what we emitted\non stdout, surely this should also be a \"test_must_be_empty out\".\n\n(I didn't test that, but if that fails wes hould be testing whatever it\nis that we emit here, surely..)\n"},{"id":"474949","messageId":"abaa6151-0f4d-1aab-2249-a2bdac2bebef@gmail.com","threadId":"59519","inReplyTo":"230406.86cz4htmzz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 5/6] t1502: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-06T19:51:26Z","receivedAt":"2023-04-06T19:51:36Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 06/04/2023 10:47, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Apr 04 2023, Andrei Rybak wrote:\n> \n>> Three tests in file t1502-rev-parse-parseopt.sh use three redirections\n>> with invocation of \"git rev-parse --parseopt --\". All three tests\n>> redirect standard output to file \"out\" and file \"spec\" to standard\n>> input. Two of the tests redirect standard output a second time to file\n>> \"actual\", and the third test redirects standard error to file \"err\".\n>> These tests check contents of files \"actual\" and \"err\", but don't use\n>> the files named \"out\" for assertions. The two tests that redirect to\n>> standard output twice might also be confusing to the reader.\n>>\n>> Don't redirect standard output of \"git rev-parse\" to file \"out\" in\n>> t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.\n>>\n>> Signed-off-by: Andrei Rybak \n>> ---\n>> t/t1502-rev-parse-parseopt.sh | 6 +++---\n>> 1 file changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n>> index de1d48f3ba..dd811b7fb4 100755\n>> --- a/t/t1502-rev-parse-parseopt.sh\n>> +++ b/t/t1502-rev-parse-parseopt.sh\n>> @@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n>> \t|EOF\n>> \tEND_EXPECT\n>> \n>> -\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n>> +\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n>> \ttest_cmp expect actual\n>> '\n>> \n>> test_expect_success 'test --parseopt invalid opt-spec' '\n>> \ttest_write_lines x -- \"=, x\" >spec &&\n>> \techo \"fatal: missing opt-spec before option flags\" >expect &&\n>> -\ttest_must_fail git rev-parse --parseopt -- >out err &&\n>> +\ttest_must_fail git rev-parse --parseopt -- err &&\n>> \ttest_cmp expect err\n>> '\n>> \n>> @@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n>> \t|EOF\n>> \tEND_EXPECT\n>> \n>> -\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n>> +\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n>> \ttest_cmp expect actual\n>> '\n> \n> Ditto earlier comments: When we fail, we should assert what we emitted\n> on stdout, surely this should also be a \"test_must_be_empty out\".\n> \n> (I didn't test that, but if that fails wes hould be testing whatever it\n> is that we emit here, surely..)\n\nThis patch is not like the others. File \"out\" is indeed empty, because\nfile \"actual\" isn't empty. Redirect to \"out\" is overwritten by redirect\nto \"actual\".\n"},{"id":"474952","messageId":"db2de983-9b1f-5efb-0fdc-cc704e6b875b@gmail.com","threadId":"59519","inReplyTo":"230406.86ttxttnir.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/6] t0300: don't create unused file","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-06T21:01:07Z","receivedAt":"2023-04-06T21:01:48Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 06/04/2023 10:34, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Apr 04 2023, Andrei Rybak wrote:\n> \n>> Test 'credential config with partial URLs' in t0300-credentials.sh\n>> contains three \"git credential fill\" invocations. For two of the\n>> invocations, the test asserts presence or absence of string \"yep\" in the\n>> standard output. For the third test it checks for an error message in\n>> standard error.\n>>\n>> Don't redirect standard output of \"git credential\" to file \"stdout\" in\n>> t0300-credentials.sh to avoid creating an unnecessary file when only\n>> standard error is checked.\n>>\n>> Signed-off-by: Andrei Rybak \n>> ---\n>> t/t0300-credentials.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n>> index c66d91e82d..b8612ede95 100755\n>> --- a/t/t0300-credentials.sh\n>> +++ b/t/t0300-credentials.sh\n>> @@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '\n>> \n>> \tgit -c credential.$partial.helper=yep \\\n>> \t\t-c credential.with%0anewline.username=uh-oh \\\n>> -\t\tcredential fill stdout 2>stderr &&\n>> +\t\tcredential fill stderr &&\n>> \ttest_i18ngrep \"skipping credential lookup for key\" stderr\n>> '\n> \n> This goes for these changes in this series general: You're correct that\n> this is useless now, but I don't think it follows that we should be\n> removing the \"redundant\" code in all cases, rather than fixing the test\n> to actually check these.\n\nI'll reply to these on one-by-one. See also this review of a patch in part 1\nof this series from Junio C Hamano:\n\n https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/\n\n> E.g. this will also make this test pass:\n> \t\n> \tdiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> \tindex c66d91e82d8..62c2a0fd50e 100755\n> \t--- a/t/t0300-credentials.sh\n> \t+++ b/t/t0300-credentials.sh\n> \t@@ -806,9 +806,11 @@ test_expect_success 'credential config with partial URLs' '\n> \t \t\treturn 1\n> \t \tdone &&\n> \t\n> \t+\tcp stdout stdout.last &&\n> \t \tgit -c credential.$partial.helper=yep \\\n> \t \t\t-c credential.with%0anewline.username=uh-oh \\\n> \t \t\tcredential fill stdout 2>stderr &&\n> \t+\ttest_cmp stdout.last stdout &&\n> \t \ttest_i18ngrep \"skipping credential lookup for key\" stderr\n> \t '\n> \t\n> \n> Does that make sense? No idea, I don't know the credential system well.\n> \n> But isn't it worth testing that when we ask for this that we're getting\n> some known output along with the warning?\n\nCurrent version from branch \"master\" with more context lines:\n\n> test_expect_success 'credential config with partial URLs' '\n> \techo \"echo password=yep\" | write_script git-credential-yep &&\n> \ttest_write_lines url=https://user@example.com/repo.git >stdin &&\n\nThe test is checking that \"url=https://user@example.com/repo.git\" matches ...\n\n> \tfor partial in \\\n> \t\texample.com \\\n> \t\tuser@example.com \\\n> \t\thttps:// \\\n> \t\thttps://example.com \\\n> \t\thttps://example.com/ \\\n> \t\thttps://user@example.com \\\n> \t\thttps://user@example.com/ \\\n> \t\thttps://example.com/repo.git \\\n> \t\thttps://user@example.com/repo.git \\\n> \t\t/repo.git\n\n... these partial URLs ...\n\n> \tdo\n> \t\tgit -c credential.$partial.helper=yep \\\n> \t\t\tcredential fill stdout &&\n> \t\tgrep yep stdout ||\n> \t\treturn 1\n> \tdone &&\n> \n> \tfor partial in \\\n> \t\tdont.use.this \\\n> \t\thttp:// \\\n> \t\t/repo\n\n... but doesn't match these.\n\n> \tdo\n> \t\tgit -c credential.$partial.helper=yep \\\n> \t\t\tcredential fill stdout &&\n> \t\t! grep yep stdout ||\n\nHere \"! grep yep stdout\" ensures that git-credential-yep isn't launched for the\nthree kinds of partial URLs. Otherwise, the actual content of \"stdout\" is\nignored. It comes from script \"askpass\" (see bottom of file \"t/lib-credential.sh\").\n\nAbsence or presence of \"yep\" is a proxy for whether or not partial URL got\nmatched or not, and that is what's important for this test. Adding assertions\nfor output of \"askpass\" here would only obscure this fact, I think.\n\nThere are other tests in t030[0-3] that do check standard output for what\nthe helper script \"askpass\" prints out -- those tests validate that the\n\"git credentials\" fallbacks to asking for credentials in the terminal in\nvarious situations. This is done via functions helper_test and\nhelper_test_timeout from \"t/lib-credential.sh\".\n\n> \t\treturn 1\n> \tdone &&\n> \n> \tgit -c credential.$partial.helper=yep \\\n> \t\t-c credential.with%0anewline.username=uh-oh \\\n> \t\tcredential fill stdout 2>stderr &&\n> \ttest_i18ngrep \"skipping credential lookup for key\" stderr\n\nHere, the important part is that \"git credential\" reacts to invalid key being\nused: \"credential.with%0anewline.username=uh-oh\", and similarly, I think that\nadding assertions about \"stdout\" might not be a good idea.\n\n>'\n\nPerhaps Johannes Schindelin, author of commit 9a121b0d22 (\"credential: handle\n`credential..` again\", 2020-04-24), could chime in.\n"},{"id":"474955","messageId":"c43e6b71-075a-e39a-7351-8595e145dacf@gmail.com","threadId":"59519","inReplyTo":"230406.86pm8htnfk.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/6] t1300: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-06T21:30:21Z","receivedAt":"2023-04-06T21:30:28Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 06/04/2023 10:38, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Apr 04 2023, Andrei Rybak wrote:\n> \n>> Three tests in t1300-config.sh check that \"git config --get\" barfs when\n>> syntax errors are present in the config file. The tests redirect\n>> standard output and standard error of \"git config --get\" to files,\n>> \"actual\" and \"error\" correspondingly. They assert presence of an error\n>> message in file \"error\". However, these tests don't use file \"actual\"\n>> for assertions.\n>>\n>> Don't redirect standard output of \"git config --get\" to file \"actual\" in\n>> t1300-config.sh to avoid creating unnecessary files.\n>>\n>> Signed-off-by: Andrei Rybak \n>> ---\n>> t/t1300-config.sh | 6 +++---\n>> 1 file changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n>> index d566729d74..8ac4531c1b 100755\n>> --- a/t/t1300-config.sh\n>> +++ b/t/t1300-config.sh\n>> @@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '\n>> \t[section]\n>> \tkey garbage\n>> \tEOF\n>> -\ttest_must_fail git config --get section.key >actual 2>error &&\n>> +\ttest_must_fail git config --get section.key 2>error &&\n>> \ttest_i18ngrep \" line 3 \" error\n>> '\n>> \n>> @@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '\n>> \t[section\n>> \tkey = value\n>> \tEOF\n>> -\ttest_must_fail git config --get section.key >actual 2>error &&\n>> +\ttest_must_fail git config --get section.key 2>error &&\n>> \ttest_i18ngrep \" line 2 \" error\n>> '\n>> \n>> @@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '\n>> \t[section]\n>> \tkey = \"value string\n>> \tEOF\n>> -\ttest_must_fail git config --get section.key >actual 2>error &&\n>> +\ttest_must_fail git config --get section.key 2>error &&\n>> \ttest_i18ngrep \" line 3 \" error\n>> '\n> \n> Ditto my comment on 1/6, shouldn't we instead be doing e.g.:\n> \t\n> \tdiff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> \tindex 2575279ab84..df2070c2f09 100755\n> \t--- a/t/t1300-config.sh\n> \t+++ b/t/t1300-config.sh\n> \t@@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' '\n> \t \t[section]\n> \t \tkey garbage\n> \t \tEOF\n> \t-\ttest_must_fail git config --get section.key >actual 2>error &&\n> \t+\ttest_must_fail git config --get section.key >out 2>error &&\n> \t+\ttest_must_be_empty out &&\n> \t \ttest_i18ngrep \" line 3 \" error\n> \t '\n> \t\n> I.e. before this we had no coverage on the error being the only output,\n> but seemingly by mistake. Let's just assert that, rather than dropping\n> the redirection entirely, no?\n\nHere, failing invocations of \"git config\" are tested, and an argument,\nas Junio C Hamano outlined in https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/\nfor output of failing \"git mktree\", could be applied here.\n\nThinking about it more, such assertions enforcing empty standard output for\nthese commands might be helpful if some tools and/or scripts rely on empty\nstandard output instead of checking the exit code. Hyrum's Law applies here,\nI guess.\n"},{"id":"474957","messageId":"7a5de047-3535-3b87-f023-43c400d57131@gmail.com","threadId":"59519","inReplyTo":"c43e6b71-075a-e39a-7351-8595e145dacf@gmail.com","subject":"git config tests for \"'git config ignores pairs ...\" (was Re: [PATCH v2 3/6] t1300: don't create unused files)","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-06T21:35:42Z","receivedAt":"2023-04-06T21:35:48Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 06/04/2023 23:30, Andrei Rybak wrote:\n> On 06/04/2023 10:38, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> Ditto my comment on 1/6, shouldn't we instead be doing e.g.:\n>>\n>>     diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n>>     index 2575279ab84..df2070c2f09 100755\n>>     --- a/t/t1300-config.sh\n>>     +++ b/t/t1300-config.sh\n>>     @@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' '\n>>          [section]\n>>          key garbage\n>>          EOF\n>>     -    test_must_fail git config --get section.key >actual 2>error &&\n>>     +    test_must_fail git config --get section.key >out 2>error &&\n>>     +    test_must_be_empty out &&\n>>          test_i18ngrep \" line 3 \" error\n>>      '\n>>\n>> I.e. before this we had no coverage on the error being the only output,\n>> but seemingly by mistake. Let's just assert that, rather than dropping\n>> the redirection entirely, no?\n> \n> Here, failing invocations of \"git config\" are tested, and an argument,\n> as Junio C Hamano outlined in \n> https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/\n> for output of failing \"git mktree\", could be applied here.\n> \n> Thinking about it more, such assertions enforcing empty standard output for\n> these commands might be helpful if some tools and/or scripts rely on empty\n> standard output instead of checking the exit code.  Hyrum's Law applies \n> here,\n> I guess.\n\nThere are some tests in t/t1300-config.sh that do check that standard output\nor standard error is empty. And I think I stumbled some other broken tests,\nwhile checking those.\n\nTest 'git config ignores pairs without count' checks that standard error\n(2>error) is empty. Just below it, there seems to be a copy-paste error:\nthere are two tests titled 'git config ignores pairs with zero count'.\nFirst one doesn't check any output, but the second checks standard output,\nwhile calling the file \"error\" (>error). Test 'git config ignores pairs\nwith empty count' checks >error as well.\n\nThey were all introduced in d8d77153ea (config: allow specifying config entries\nvia envvar pairs, 2021-01-12) by Patrick Steinhardt. Patrick, what do you think?\n"},{"id":"474959","messageId":"35bc2dc5-d5cb-3492-ff94-41b93b7563d4@gmail.com","threadId":"59519","inReplyTo":"230406.86lej5tn8c.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 4/6] t1450: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-06T22:19:03Z","receivedAt":"2023-04-06T22:19:26Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 06/04/2023 10:41, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Apr 04 2023, Andrei Rybak wrote:\n>> ---\n>> t/\t | 5 +----\n>> 1 file changed, 1 insertion(+), 4 deletions(-)\n>>\n>> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n>> index bca46378b2..8c442adb1a 100755\n>> --- a/t/t1450-fsck.sh\n>> +++ b/t/t1450-fsck.sh\n>> @@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '\n>> \n>> \t\tgarbage_blob=$(git hash-object --stdin -w -t garbage --literally > \n>> -\t\tcat >err.expect <<-\\EOF &&\n>> -\t\tfatal: invalid object type\n>> -\t\tEOF\n>> -\t\ttest_must_fail git fsck >out 2>err &&\n>> +\t\ttest_must_fail git fsck 2>err &&\n>> \t\tgrep -e \"^error\" -e \"^fatal\" err >errors &&\n>> \t\ttest_line_count = 1 errors &&\n>> \t\tgrep \"$garbage_blob: object is of unknown type '\"'\"'garbage'\"'\"':\" err\n> \n> ...but ditto my review on other patches, this just seems like a mistake\n> of mine, i.e. if I add the \"test_must_be_empty out\" the test passes.\n> \n> So isn't the answer here that my 31deb28f5e had an unintentional\n> regression, and we should bring the assertion back? Its commit message\n> says nothing about wanting to stop asserting stdout.\n> \n> Maybe there was a reason I'm missing for why I remved it, it's since\n> been paged out of my wetware, but looking at it briefly now it just\n> seems like an unintentional bug / loss of test coverage that we should\n> fix.\n\nTests in t1450-fsck.sh that do enforce empty standard output do it mostly\nvia \">../actual 2>&1\" and then a \"test_must_be_empty actual\".\n\nFor 'fsck error and recovery on invalid object type', the question is:\nis having this assertion useful for a developer using this test? The test\nis about invalid object types and what error messages \"git fsck\" prints\nabout them. The test creates a fresh repository for it:\n\n> \tgit init --bare garbage-type &&\n\nIs it useful to a developer working on this part of \"git fsck\" to have\na \"reminder\" that no dangling objects should be found in such a fresh\nrepository? Speaking of which, should there be such a test:\n\n\ttest_expect_success 'fresh repository has no dangling objects' '\n\t\tgit init fresh &&\n\t\tgit -C fresh fsck >out\n\t\ttest_must_be_empty out\n\t'\n\n? Maybe even in t0001-init.sh?\n"},{"id":"474964","messageId":"4ef5464b-31dd-3c3e-05be-9891162e4f05@gmail.com","threadId":"59519","inReplyTo":"230406.86h6tttn21.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 6/6] t2019: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-07T02:19:39Z","receivedAt":"2023-04-07T02:19:47Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Disclaimer: while trying to write a response to this email, I went a bit\noff track, and a big portion of message below is an investigation of\ntest coverage of what is printed to standard output and standard error\nby \"git checkout\". It's still mostly relevant to the discussion about\nt2019, or at least I hope so.\n\nThere are four parts to this investigation, starting with:\n\n\n1. Standard output of \"git checkout\"\nOther than \"git checkout -p\" (tested in t3701-add-interactive.sh), it\nseems that the only thing that \"git checkout\" prints to standard output\nis in:\n\n a) function \"show_local_changes\" in builtin/checkout.c -- a couple\n of tests in t7201-co.sh validate this\n b) function \"report_tracking\" in builtin/checkout.c -- there are\n tests that validate tracking information in output of \"git\n checkout\" in t6040-tracking-info.sh. One test in\n t2020-checkout-detach.sh, titled 'tracking count is accurate after\n orphan check' validates it as well.\n\nWhile trying to find all tests that validate standard output of\n\"git checkout\", I found out a couple of things.\n\n\n2. Standard error of \"git checkout\"\nHonestly, I haven't noticed it before, but I found it surprising that\nmessages about branch switching:\n\n - \"Switched to branch '%s'\\n\"\n - \"Switched to a new branch '%s'\\n\"\n - \"Switched to and reset branch '%s'\\n\"\n\nare printed to standard error.\n\nSeveral tests in t2020-checkout-detach.sh validate what is printed into\nstandard error by \"git checkout\" via a variation of \">actual 2>&1\".\nTests for advice printed by \"git checkout\" (looking at \"advice.c\", it\nall goes to stderr), also do a variation of \">actual 2>&1\".\n\n\n3. t2024-checkout-dwim.sh\nTest 'loosely defined local base branch is reported correctly' in t2024\nhas an interesting validation of output of \"git checkout\":\n\n\tgit checkout strict | sed -e \"s/strict/BRANCHNAME/g\" >expect &&\n\tstatus_uno_is_clean &&\n\tgit checkout loose | sed -e \"s/loose/BRANCHNAME/g\" >actual &&\n\tstatus_uno_is_clean &&\n\n\ttest_cmp expect actual\n\nwhich is fine, except that neither file \"expect\" nor \"actual\" contain\nthe string \"BRANCHNAME\". And this test was broken when it was\nintroduced in 05e73682cd (checkout: report upstream correctly even with\nloosely defined branch.*.merge, 2014-10-14). It was probably intended\nfor this test to redirect standard error of \"git checkout\". It should\nbe cleaned up as a separate patch/topic.\n\nOn 06/04/2023 10:44, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Apr 04 2023, Andrei Rybak wrote:\n> \n>> Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of\n>> \"git checkout\" to files \"stdout\" and \"stderr\". Several assertions are\n>> made using file \"stderr\". File \"stdout\", however, is unused.\n>>\n>> Don't redirect standard output of \"git checkout\" to file \"stdout\" in\n>> t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.\n>>\n>> Signed-off-by: Andrei Rybak \n>> ---\n>> t/t2019-checkout-ambiguous-ref.sh | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh\n>> index 2c8c926b4d..9540588664 100755\n>> --- a/t/t2019-checkout-ambiguous-ref.sh\n>> +++ b/t/t2019-checkout-ambiguous-ref.sh\n>> @@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '\n>> '\n>> \n>> test_expect_success 'checkout ambiguous ref succeeds' '\n>> -\tgit checkout ambiguity >stdout 2>stderr\n>> +\tgit checkout ambiguity 2>stderr\n>> '\n> \n> Ditto earlier comments that we should just fix this, if I make this\n> \">out\" and \"test_must_be_empty out\" this succeeds, shouldn't we just use\n> that?\n\n4. t2019-checkout-ambiguous-ref.sh\nBack on topic: is empty standard output something that this test in\nt2019 should worry about? Let's take a look at other tests.\n\nAside from what was mentioned in section 1, tests in t7201 don't look\nat standard output of \"git checkout\". There isn't a lot of\n\"test_must_be_empty\" in t/t2*check*:\n\n $ git grep 'test_must_be_empty' t/t2*check*\n t/t2004-checkout-cache-temp.sh: test_must_be_empty stderr &&\n t/t2013-checkout-submodule.sh: test_must_be_empty actual\n t/t2013-checkout-submodule.sh: test_must_be_empty actual\n t/t2013-checkout-submodule.sh: test_must_be_empty actual\n t/t2024-checkout-dwim.sh: test_must_be_empty status.actual\n\nThe first one, in t2004, asserts output of \"git checkout-index\".\nAll three in t2013 assert output of \"git checkout HEAD >actual 2>&1\".\nThe last one, in t2024, asserts output of \"git status\".\n\n(There's also one \"test_line_count = 0\" in the same test in t2004,\n but otherwise these tests seem to be pretty up-to-date w.r.t.\n to using test_must_be_empty helper)\n\n> \n>> test_expect_success 'checkout produces ambiguity warning' '\n> \n> As an aside, we should really just combine these two tests.\n\nMy dumb script for finding unused files gives false-positives for such\ntests. And there a lot of tests that got split during introduction of\nC_LOCALE_OUTPUT prerequisite or were introduced before C_LOCALE_OUTPUT\nwas phased out.\n\nFor t2019, however, the tests were created this way before\nC_LOCALE_OUTPUT in 0cb6ad3c3d (\"checkout: fix bug with ambiguous refs\",\n2011-01-11). Then the prerequisite was added in 6b3d83efac\n(\"t2019-checkout-ambiguous-ref.sh: depend on C_LOCALE_OUTPUT\",\n2011-04-03) and removed in d3bd0425b2 (\"i18n: use test_i18ngrep in\nlib-httpd and t2019\", 2011-04-12).\n\n> \n>> @@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '\n>> '\n>> \n>> test_expect_success 'checkout vague ref succeeds' '\n>> -\tgit checkout vagueness >stdout 2>stderr &&\n>> +\tgit checkout vagueness 2>stderr &&\n>> \ttest_set_prereq VAGUENESS_SUCCESS\n>> '\n> \n\n"},{"id":"475038","messageId":"20230408205450.569548-1-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"230406.86h6tttn21.gmgdl@evledraar.gmail.com","subject":"[PATCH] t2024: fix loose/strict local base branch DWIM test","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-08T20:54:50Z","receivedAt":"2023-04-08T20:54:58Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Test 'loosely defined local base branch is reported correctly' in\nt2024-checkout-dwim.sh, which was introduced in [1] compares output of\ntwo invocations of \"git checkout\", invoked with two different branches\nnamed \"strict\" and \"loose\". As per description in [1], the test is\nvalidating that output of tracking information for these two branches.\nThis tracking information is printed to standard output:\n\n Your branch is behind 'main' by 1 commit, and can be fast-forwarded.\n (use \"git pull\" to update your local branch)\n\nThe test assumes that the names of the two branches (strict and loose)\nare in that output, and pipes the output through sed to replace names of\nthe branches with \"BRANCHNAME\". Command \"git checkout\", however,\noutputs the branch name to standard error, not standard output -- see\nmessage \"Switched to branch '%s'\\n\" in function \"update_refs_for_switch\"\nin \"builtin/checkout.c\". This means that the two invocations of sed do\nnothing.\n\nRedirect both the standard output and the standard error of \"git\ncheckout\" for these assertions. Ensure that compared files have the\nstring \"BRANCHNAME\".\n\nIn a series of piped commands, only the return code of the last command\nis used. Thus, all other commands will have their return codes masked.\nAvoid piping of output of git directly into sed to preserve the exit\nstatus code of \"git checkout\", while we're here.\n\n[1] 05e73682cd (checkout: report upstream correctly even with loosely\n defined branch.*.merge, 2014-10-14)\n\nSigned-off-by: Andrei Rybak \n---\n\nOn 2023-04-07T04:19, Andrei Rybak wrote:\n> 3. t2024-checkout-dwim.sh\n> Test 'loosely defined local base branch is reported correctly' in t2024\n> has an interesting validation of output of \"git checkout\":\n> \n> git checkout strict | sed -e \"s/strict/BRANCHNAME/g\" >expect &&\n> status_uno_is_clean &&\n> git checkout loose | sed -e \"s/loose/BRANCHNAME/g\" >actual &&\n> status_uno_is_clean &&\n> \n> test_cmp expect actual\n> \n> which is fine, except that neither file \"expect\" nor \"actual\" contain\n> the string \"BRANCHNAME\". And this test was broken when it was\n> introduced in 05e73682cd (checkout: report upstream correctly even with\n> loosely defined branch.*.merge, 2014-10-14). It was probably intended\n> for this test to redirect standard error of \"git checkout\". It should\n> be cleaned up as a separate patch/topic.\n\nHere's the patch. Alternatively, the fix could be to just drop the sed\ninvocation from this test.\n\n t/t2024-checkout-dwim.sh | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex 4a1c901456..74049a9812 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -305,10 +305,13 @@ test_expect_success 'loosely defined local base branch is reported correctly' '\n \ttest_config branch.strict.merge refs/heads/main &&\n \ttest_config branch.loose.merge main &&\n \n-\tgit checkout strict | sed -e \"s/strict/BRANCHNAME/g\" >expect &&\n+\tgit checkout strict >expect.raw 2>&1 &&\n+\tsed -e \"s/strict/BRANCHNAME/g\" expect &&\n \tstatus_uno_is_clean &&\n-\tgit checkout loose | sed -e \"s/loose/BRANCHNAME/g\" >actual &&\n+\tgit checkout loose >actual.raw 2>&1 &&\n+\tsed -e \"s/loose/BRANCHNAME/g\" actual &&\n \tstatus_uno_is_clean &&\n+\tgrep BRANCHNAME actual &&\n \n \ttest_cmp expect actual\n '\n-- \n2.40.0\n\n"},{"id":"475074","messageId":"xmqqfs97ej09.fsf@gitster.g","threadId":"59519","inReplyTo":"20230408205450.569548-1-rybak.a.v@gmail.com","subject":"Re: [PATCH] t2024: fix loose/strict local base branch DWIM test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-10T17:37:10Z","receivedAt":"2023-04-10T17:37:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak writes:\n\n>> 3. t2024-checkout-dwim.sh\n>> Test 'loosely defined local base branch is reported correctly' in t2024\n>> has an interesting validation of output of \"git checkout\":\n>> \n>> git checkout strict | sed -e \"s/strict/BRANCHNAME/g\" >expect &&\n>> status_uno_is_clean &&\n>> git checkout loose | sed -e \"s/loose/BRANCHNAME/g\" >actual &&\n>> status_uno_is_clean &&\n>> \n>> test_cmp expect actual\n>> \n>> which is fine, except that neither file \"expect\" nor \"actual\" contain\n>> the string \"BRANCHNAME\". And this test was broken when it was\n>> introduced in 05e73682cd (checkout: report upstream correctly even with\n>> loosely defined branch.*.merge, 2014-10-14). It was probably intended\n>> for this test to redirect standard error of \"git checkout\". It should\n>> be cleaned up as a separate patch/topic.\n>\n> Here's the patch. Alternatively, the fix could be to just drop the sed\n> invocation from this test.\n\nOr we could check both stdout and stderr separately. I think the\nchange in this patch reflects the intention of the original most\nclosely, so let's queue it as-is.\n\nThanks.\n\n\n>\n> t/t2024-checkout-dwim.sh | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\n> index 4a1c901456..74049a9812 100755\n> --- a/t/t2024-checkout-dwim.sh\n> +++ b/t/t2024-checkout-dwim.sh\n> @@ -305,10 +305,13 @@ test_expect_success 'loosely defined local base branch is reported correctly' '\n> \ttest_config branch.strict.merge refs/heads/main &&\n> \ttest_config branch.loose.merge main &&\n> \n> -\tgit checkout strict | sed -e \"s/strict/BRANCHNAME/g\" >expect &&\n> +\tgit checkout strict >expect.raw 2>&1 &&\n> +\tsed -e \"s/strict/BRANCHNAME/g\" expect &&\n> \tstatus_uno_is_clean &&\n> -\tgit checkout loose | sed -e \"s/loose/BRANCHNAME/g\" >actual &&\n> +\tgit checkout loose >actual.raw 2>&1 &&\n> +\tsed -e \"s/loose/BRANCHNAME/g\" actual &&\n> \tstatus_uno_is_clean &&\n> +\tgrep BRANCHNAME actual &&\n> \n> \ttest_cmp expect actual\n> '\n"},{"id":"475346","messageId":"20230414081352.810296-1-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"7a5de047-3535-3b87-f023-43c400d57131@gmail.com","subject":"[PATCH v1 0/2] git config tests for \"'git config ignores pairs ...\"","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-14T08:13:50Z","receivedAt":"2023-04-14T08:14:02Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2023-04-06T23:35, Andrei Rybak wrote:\n> There are some tests in t/t1300-config.sh that do check that standard\n> output\n> or standard error is empty. And I think I stumbled some other broken\n> tests,\n> while checking those.\n>\n> Test 'git config ignores pairs without count' checks that standard error\n> (2>error) is empty. Just below it, there seems to be a copy-paste error:\n> there are two tests titled 'git config ignores pairs with zero count'.\n> First one doesn't check any output, but the second checks standard output,\n> while calling the file \"error\" (>error). Test 'git config ignores pairs\n> with empty count' checks >error as well.\n>\n> They were all introduced in d8d77153ea (config: allow specifying config\n> entries\n> via envvar pairs, 2021-01-12) by Patrick Steinhardt. Patrick, what do\n> you think?\n\nHere are patches to fix these.\n\nAndrei Rybak (2):\n t1300: drop duplicate test\n t1300: check stderr for \"ignores pairs\" tests\n\n t/t1300-config.sh | 16 +++++-----------\n 1 file changed, 5 insertions(+), 11 deletions(-)\n\n-- \n2.40.0\n\n"},{"id":"475347","messageId":"20230414081352.810296-2-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230414081352.810296-1-rybak.a.v@gmail.com","subject":"[PATCH v1 1/2] t1300: drop duplicate test","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-14T08:13:51Z","receivedAt":"2023-04-14T08:14:03Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"There are two almost identical tests called 'git config ignores pairs\nwith zero count' in file t1300-config.sh. Drop the first of these and\nkeep the one that contains more assertions.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 7 -------\n 1 file changed, 7 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 2575279ab8..696dca17c6 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1458,13 +1458,6 @@ test_expect_success 'git config ignores pairs without count' '\n \ttest_must_be_empty error\n '\n \n-test_expect_success 'git config ignores pairs with zero count' '\n-\ttest_must_fail env \\\n-\t\tGIT_CONFIG_COUNT=0 \\\n-\t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one\n-'\n-\n test_expect_success 'git config ignores pairs exceeding count' '\n \tGIT_CONFIG_COUNT=1 \\\n \t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-- \n2.40.0\n\n"},{"id":"475348","messageId":"20230414081352.810296-3-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230414081352.810296-1-rybak.a.v@gmail.com","subject":"[PATCH v1 2/2] t1300: check stderr for \"ignores pairs\" tests","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-14T08:13:52Z","receivedAt":"2023-04-14T08:14:05Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests \"git config ignores pairs ...\" in t1300-config.sh validate that\n\"git config\" ignores with various kinds of supplied pairs of environment\nvariables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* that should be ingored.\nBy \"ignores\" here we mean that \"git config\" doesn't complain about them\nto standard error. This is validated by redirecting the standard error\nto a file called \"error\" and asserting that it is empty. However, two\nof these tests incorrectly redirect to standard output while calling the\nfile \"error\", and test 'git config ignores pairs exceeding count'\ndoesn't validate standard error at all.\n\nFix it by redirecting standard error to file \"error\" and asserting its\nemptiness.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 696dca17c6..20a15ede5c 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '\n \tGIT_CONFIG_COUNT=1 \\\n \t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n \t\tGIT_CONFIG_KEY_1=\"pair.two\" GIT_CONFIG_VALUE_1=\"value\" \\\n-\t\tgit config --get-regexp \"pair.*\" >actual &&\n+\t\tgit config --get-regexp \"pair.*\" >actual 2>error &&\n \tcat >expect <<-EOF &&\n \tpair.one value\n \tEOF\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty error\n '\n \n test_expect_success 'git config ignores pairs with zero count' '\n \ttest_must_fail env \\\n \t\tGIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one >error &&\n+\t\tgit config pair.one 2>error &&\n \ttest_must_be_empty error\n '\n \n test_expect_success 'git config ignores pairs with empty count' '\n \ttest_must_fail env \\\n \t\tGIT_CONFIG_COUNT= GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one >error &&\n+\t\tgit config pair.one 2>error &&\n \ttest_must_be_empty error\n '\n \n-- \n2.40.0\n\n"},{"id":"475378","messageId":"CACayv=jL4t3cUVS=xXQ3fLxF26vDXRJ3khs2y4UjzBw947JVkw@mail.gmail.com","threadId":"59519","inReplyTo":"20230414081352.810296-3-rybak.a.v@gmail.com","subject":"Re: [PATCH v1 2/2] t1300: check stderr for \"ignores pairs\" tests","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-14T13:28:17Z","receivedAt":"2023-04-14T13:28:38Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On Fri, 14 Apr 2023 at 10:13, Andrei Rybak wrote:\n>\n> Tests \"git config ignores pairs ...\" in t1300-config.sh validate that\n> \"git config\" ignores with various kinds of supplied pairs of environment\n> variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* that should be ingored.\n> By \"ignores\" here we mean that \"git config\" doesn't complain about them\n> to standard error.\n\nAfter thinking about this some more, I've realized that this is an\nincorrect interpretation\nof the titles of these tests. The correct interpretation is more\nobvious from another test:\n\n test_expect_success 'git config ignores pairs exceeding count' '\n GIT_CONFIG_COUNT=1 \\\n GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n GIT_CONFIG_KEY_1=\"pair.two\" GIT_CONFIG_VALUE_1=\"value\" \\\n git config --get-regexp \"pair.*\" >actual &&\n cat >expect <<-EOF &&\n pair.one value\n EOF\n test_cmp expect actual\n '\n\nKey-value pair \"pair.two=value\" is ignored because it's outside of the\nrange of the\nsupplied value of GIT_CONFIG_COUNT. That is, these tests validate that reading\nof these environment variables reads GIT_CONFIG_COUNT first and only loads\nGIT_CONFIG_KEY_ and GIT_CONFIG_VALUE_ that fit in the range.\n\n> This is validated by redirecting the standard error\n> to a file called \"error\" and asserting that it is empty. However, two\n> of these tests incorrectly redirect to standard output while calling the\n> file \"error\", and test 'git config ignores pairs exceeding count'\n> doesn't validate standard error at all.\n>\n> Fix it by redirecting standard error to file \"error\" and asserting its\n> emptiness.\n>\n> Signed-off-by: Andrei Rybak \n> ---\n> t/t1300-config.sh | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 696dca17c6..20a15ede5c 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '\n> GIT_CONFIG_COUNT=1 \\\n> GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n> GIT_CONFIG_KEY_1=\"pair.two\" GIT_CONFIG_VALUE_1=\"value\" \\\n> - git config --get-regexp \"pair.*\" >actual &&\n> + git config --get-regexp \"pair.*\" >actual 2>error &&\n> cat >expect <<-EOF &&\n> pair.one value\n> EOF\n> - test_cmp expect actual\n> + test_cmp expect actual &&\n> + test_must_be_empty error\n> '\n>\n> test_expect_success 'git config ignores pairs with zero count' '\n> test_must_fail env \\\n> GIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n> - git config pair.one >error &&\n> + git config pair.one 2>error &&\n> test_must_be_empty error\n> '\n>\n> test_expect_success 'git config ignores pairs with empty count' '\n> test_must_fail env \\\n> GIT_CONFIG_COUNT= GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n> - git config pair.one >error &&\n> + git config pair.one 2>error &&\n\n\nSame question as in Ævar's\nhttps://lore.kernel.org/git/230406.86pm8htnfk.gmgdl@evledraar.gmail.com/\nand my reply https://lore.kernel.org/git/c43e6b71-075a-e39a-7351-8595e145dacf@gmail.com/\napplies here, though. In tests 'git config ignores pairs with zero count' and\n 'git config ignores pairs with empty count' test_must_fail already asserts that\n\"git config\" couldn't get the value. Should we be also inspecting\nboth stdout and\nstderr, as the test 'git config ignores pairs exceeding count' does\n(after this patch)?\n\n> test_must_be_empty error\n> '\n\n>\n> --\n> 2.40.0\n>\n"},{"id":"475539","messageId":"20230417191044.909094-1-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230403223338.468025-1-rybak.a.v@gmail.com","subject":"[PATCH v3 0/6] t: fix unused files, part 2","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-17T19:10:38Z","receivedAt":"2023-04-17T19:10:52Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Creation of files from redirecting output of Git commands in tests has been\nremoved for files which aren't being used for assertions. CC'ed are authors of\nthe affected tests.\n\nv1 cover letter:\n https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/\nv2 cover letter:\n https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/\n\nChanges since v2:\n\n - Added \"Acked-by\" of Øystein Walle to patch 5/6\n Cf. https://lore.kernel.org/git/CAFaJEqug4bghEMnEQzGDN10EqM8e8iSf5i12AvOm+NZzDCQKOw@mail.gmail.com/\n\nRange diff:\n\n1: 828bb18bd7 = 1: 828bb18bd7 t0300: don't create unused file\n2: a5b299a0c6 = 2: a5b299a0c6 t1300: fix config file syntax error descriptions\n3: 806df16415 = 3: 806df16415 t1300: don't create unused files\n4: 6742c957e5 = 4: 6742c957e5 t1450: don't create unused files\n5: 6c173a5c46 ! 5: 19ac488922 t1502: don't create unused files\n @@ Commit message\n Don't redirect standard output of \"git rev-parse\" to file \"out\" in\n t1502-rev-parse-parseopt.sh to avoid creating unnecessary files.\n\n + Acked-by: Øystein Walle \n Signed-off-by: Andrei Rybak \n\n ## t/t1502-rev-parse-parseopt.sh ##\n6: d508c1def3 = 6: c41657be88 t2019: don't create unused files\n\nAndrei Rybak (6):\n t0300: don't create unused file\n t1300: fix config file syntax error descriptions\n t1300: don't create unused files\n t1450: don't create unused files\n t1502: don't create unused files\n t2019: don't create unused files\n\n t/t0300-credentials.sh | 2 +-\n t/t1300-config.sh | 10 +++++-----\n t/t1450-fsck.sh | 5 +----\n t/t1502-rev-parse-parseopt.sh | 6 +++---\n t/t2019-checkout-ambiguous-ref.sh | 4 ++--\n 5 files changed, 12 insertions(+), 15 deletions(-)\n\n-- \n2.40.0\n\n"},{"id":"475540","messageId":"20230417191044.909094-2-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230417191044.909094-1-rybak.a.v@gmail.com","subject":"[PATCH v3 1/6] t0300: don't create unused file","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-17T19:10:39Z","receivedAt":"2023-04-17T19:10:55Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Test 'credential config with partial URLs' in t0300-credentials.sh\ncontains three \"git credential fill\" invocations. For two of the\ninvocations, the test asserts presence or absence of string \"yep\" in the\nstandard output. For the third test it checks for an error message in\nstandard error.\n\nDon't redirect standard output of \"git credential\" to file \"stdout\" in\nt0300-credentials.sh to avoid creating an unnecessary file when only\nstandard error is checked.\n\nSigned-off-by: Andrei Rybak \n---\n t/t0300-credentials.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex c66d91e82d..b8612ede95 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '\n \n \tgit -c credential.$partial.helper=yep \\\n \t\t-c credential.with%0anewline.username=uh-oh \\\n-\t\tcredential fill stdout 2>stderr &&\n+\t\tcredential fill stderr &&\n \ttest_i18ngrep \"skipping credential lookup for key\" stderr\n '\n \n-- \n2.40.0\n\n"},{"id":"475541","messageId":"20230417191044.909094-3-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230417191044.909094-1-rybak.a.v@gmail.com","subject":"[PATCH v3 2/6] t1300: fix config file syntax error descriptions","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-17T19:10:40Z","receivedAt":"2023-04-17T19:10:56Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in t1300-config.sh check that \"git config --get\" barfs when\nthe config file contains various syntax errors: key=value pair without\nequals sign, broken section line, and broken value string. The sample\nconfig files include a comment describing the kind of broken syntax.\nThis description seems to have been copy-pasted from the \"broken section\nline\" sample to the other two samples.\n\nFix descriptions of broken config file syntax in samples used in\nt1300-config.sh.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 2575279ab8..d566729d74 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1571,7 +1571,7 @@ test_expect_success 'git config --edit respects core.editor' '\n # malformed configuration files\n test_expect_success 'barf on syntax error' '\n \tcat >.git/config <<-\\EOF &&\n-\t# broken section line\n+\t# broken key=value\n \t[section]\n \tkey garbage\n \tEOF\n@@ -1591,7 +1591,7 @@ test_expect_success 'barf on incomplete section header' '\n \n test_expect_success 'barf on incomplete string' '\n \tcat >.git/config <<-\\EOF &&\n-\t# broken section line\n+\t# broken value string\n \t[section]\n \tkey = \"value string\n \tEOF\n-- \n2.40.0\n\n"},{"id":"475542","messageId":"20230417191044.909094-4-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230417191044.909094-1-rybak.a.v@gmail.com","subject":"[PATCH v3 3/6] t1300: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-17T19:10:41Z","receivedAt":"2023-04-17T19:11:00Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in t1300-config.sh check that \"git config --get\" barfs when\nsyntax errors are present in the config file. The tests redirect\nstandard output and standard error of \"git config --get\" to files,\n\"actual\" and \"error\" correspondingly. They assert presence of an error\nmessage in file \"error\". However, these tests don't use file \"actual\"\nfor assertions.\n\nDon't redirect standard output of \"git config --get\" to file \"actual\" in\nt1300-config.sh to avoid creating unnecessary files.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex d566729d74..8ac4531c1b 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' '\n \t[section]\n \tkey garbage\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 3 \" error\n '\n \n@@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' '\n \t[section\n \tkey = value\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 2 \" error\n '\n \n@@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' '\n \t[section]\n \tkey = \"value string\n \tEOF\n-\ttest_must_fail git config --get section.key >actual 2>error &&\n+\ttest_must_fail git config --get section.key 2>error &&\n \ttest_i18ngrep \" line 3 \" error\n '\n \n-- \n2.40.0\n\n"},{"id":"475543","messageId":"20230417191044.909094-5-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230417191044.909094-1-rybak.a.v@gmail.com","subject":"[PATCH v3 4/6] t1450: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-17T19:10:42Z","receivedAt":"2023-04-17T19:11:13Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Test 'fsck error and recovery on invalid object type' in file\nt1450-fsck.sh redirects output of a failing \"git fsck\" invocation to\nfiles \"out\" and \"err\" to assert presence of error messages in the output\nof the command. Commit 31deb28f5e (fsck: don't hard die on invalid\nobject types, 2021-10-01) changed the way assertions in this test are\nperformed. The test doesn't compare the whole standard error with\nprepared file \"err.expect\" and it doesn't assert that standard output is\nempty.\n\nDon't create unused files \"err.expect\" and \"out\" in test 'fsck error and\nrecovery on invalid object type'.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1450-fsck.sh | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex bca46378b2..8c442adb1a 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -989,10 +989,7 @@ test_expect_success 'fsck error and recovery on invalid object type' '\n \n \t\tgarbage_blob=$(git hash-object --stdin -w -t garbage --literally err.expect <<-\\EOF &&\n-\t\tfatal: invalid object type\n-\t\tEOF\n-\t\ttest_must_fail git fsck >out 2>err &&\n+\t\ttest_must_fail git fsck 2>err &&\n \t\tgrep -e \"^error\" -e \"^fatal\" err >errors &&\n \t\ttest_line_count = 1 errors &&\n \t\tgrep \"$garbage_blob: object is of unknown type '\"'\"'garbage'\"'\"':\" err\n-- \n2.40.0\n\n"},{"id":"475544","messageId":"20230417191044.909094-6-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230417191044.909094-1-rybak.a.v@gmail.com","subject":"[PATCH v3 5/6] t1502: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-17T19:10:43Z","receivedAt":"2023-04-17T19:11:15Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Three tests in file t1502-rev-parse-parseopt.sh use three redirections\nwith invocation of \"git rev-parse --parseopt --\". All three tests\nredirect standard output to file \"out\" and file \"spec\" to standard\ninput. Two of the tests redirect standard output a second time to file\n\"actual\", and the third test redirects standard error to file \"err\".\nThese tests check contents of files \"actual\" and \"err\", but don't use\nthe files named \"out\" for assertions. The two tests that redirect to\nstandard output twice might also be confusing to the reader.\n\nDon't redirect standard output of \"git rev-parse\" to file \"out\" in\nt1502-rev-parse-parseopt.sh to avoid creating unnecessary files.\n\nAcked-by: Øystein Walle \nSigned-off-by: Andrei Rybak \n---\n t/t1502-rev-parse-parseopt.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex de1d48f3ba..dd811b7fb4 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \t|EOF\n \tEND_EXPECT\n \n-\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'test --parseopt invalid opt-spec' '\n \ttest_write_lines x -- \"=, x\" >spec &&\n \techo \"fatal: missing opt-spec before option flags\" >expect &&\n-\ttest_must_fail git rev-parse --parseopt -- >out err &&\n+\ttest_must_fail git rev-parse --parseopt -- err &&\n \ttest_cmp expect err\n '\n \n@@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n \t|EOF\n \tEND_EXPECT\n \n-\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_must_fail git rev-parse --parseopt -- -h actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.40.0\n\n"},{"id":"475545","messageId":"20230417191044.909094-7-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230417191044.909094-1-rybak.a.v@gmail.com","subject":"[PATCH v3 6/6] t2019: don't create unused files","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-17T19:10:44Z","receivedAt":"2023-04-17T19:11:16Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of\n\"git checkout\" to files \"stdout\" and \"stderr\". Several assertions are\nmade using file \"stderr\". File \"stdout\", however, is unused.\n\nDon't redirect standard output of \"git checkout\" to file \"stdout\" in\nt2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.\n\nSigned-off-by: Andrei Rybak \n---\n t/t2019-checkout-ambiguous-ref.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh\nindex 2c8c926b4d..9540588664 100755\n--- a/t/t2019-checkout-ambiguous-ref.sh\n+++ b/t/t2019-checkout-ambiguous-ref.sh\n@@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '\n '\n \n test_expect_success 'checkout ambiguous ref succeeds' '\n-\tgit checkout ambiguity >stdout 2>stderr\n+\tgit checkout ambiguity 2>stderr\n '\n \n test_expect_success 'checkout produces ambiguity warning' '\n@@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '\n '\n \n test_expect_success 'checkout vague ref succeeds' '\n-\tgit checkout vagueness >stdout 2>stderr &&\n+\tgit checkout vagueness 2>stderr &&\n \ttest_set_prereq VAGUENESS_SUCCESS\n '\n \n-- \n2.40.0\n\n"},{"id":"475635","messageId":"20230418175034.982433-1-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230414081352.810296-1-rybak.a.v@gmail.com","subject":"[PATCH v2 0/3] git config tests for \"'git config ignores pairs ...\"","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-18T17:50:31Z","receivedAt":"2023-04-18T17:50:48Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"v1 cover letter:\n https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/\n\nWhile preparing v2, I've realized that I've failed to click \"Reply All\" when\nwriting my previous email in this subthread. I apologize.\n https://lore.kernel.org/git/CACayv=jL4t3cUVS=xXQ3fLxF26vDXRJ3khs2y4UjzBw947JVkw@mail.gmail.com/\n\nChanges since v2:\n - Rewritten commit message for patch 2.\n - New RFC patch 3.\n\nAndrei Rybak (3):\n t1300: drop duplicate test\n t1300: check stderr for \"ignores pairs\" tests\n t1300: add tests for missing keys\n\n t/t1300-config.sh | 30 +++++++++++++++++++-----------\n 1 file changed, 19 insertions(+), 11 deletions(-)\n\n-- \n2.40.0\n\n"},{"id":"475636","messageId":"20230418175034.982433-2-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230418175034.982433-1-rybak.a.v@gmail.com","subject":"[PATCH v2 1/3] t1300: drop duplicate test","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-18T17:50:32Z","receivedAt":"2023-04-18T17:50:49Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"There are two almost identical tests called 'git config ignores pairs\nwith zero count' in file t1300-config.sh. Drop the first of these and\nkeep the one that contains more assertions.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 7 -------\n 1 file changed, 7 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 2575279ab8..696dca17c6 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1458,13 +1458,6 @@ test_expect_success 'git config ignores pairs without count' '\n \ttest_must_be_empty error\n '\n \n-test_expect_success 'git config ignores pairs with zero count' '\n-\ttest_must_fail env \\\n-\t\tGIT_CONFIG_COUNT=0 \\\n-\t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one\n-'\n-\n test_expect_success 'git config ignores pairs exceeding count' '\n \tGIT_CONFIG_COUNT=1 \\\n \t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-- \n2.40.0\n\n"},{"id":"475637","messageId":"20230418175034.982433-3-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230418175034.982433-1-rybak.a.v@gmail.com","subject":"[PATCH v2 2/3] t1300: check stderr for \"ignores pairs\" tests","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-18T17:50:33Z","receivedAt":"2023-04-18T17:50:51Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests \"git config ignores pairs ...\" in t1300-config.sh validate that\n\"git config\" ignores various kinds of supplied pairs of environment\nvariables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* depending on\nGIT_CONFIG_COUNT. By \"ignores\" here we mean that \"git config\" abides by\nthe value of environment variable GIT_CONFIG_COUNT and doesn't use\nkey-value pairs outside of the supplied GIT_CONFIG_COUNT when trying to\nproduce a value for config key \"pair.one\".\n\nThese tests also validate that \"git config\" doesn't complain about\nmismatched environment variables to standard error. This is validated\nby redirecting the standard error to a file called \"error\" and asserting\nthat it is empty. However, two of these tests incorrectly redirect to\nstandard output while calling the file \"error\", and test 'git config\nignores pairs exceeding count' doesn't validate standard error at all.\n\nFix these tests by redirecting standard error to file \"error\" and\nasserting its emptiness.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 696dca17c6..20a15ede5c 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '\n \tGIT_CONFIG_COUNT=1 \\\n \t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n \t\tGIT_CONFIG_KEY_1=\"pair.two\" GIT_CONFIG_VALUE_1=\"value\" \\\n-\t\tgit config --get-regexp \"pair.*\" >actual &&\n+\t\tgit config --get-regexp \"pair.*\" >actual 2>error &&\n \tcat >expect <<-EOF &&\n \tpair.one value\n \tEOF\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty error\n '\n \n test_expect_success 'git config ignores pairs with zero count' '\n \ttest_must_fail env \\\n \t\tGIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one >error &&\n+\t\tgit config pair.one 2>error &&\n \ttest_must_be_empty error\n '\n \n test_expect_success 'git config ignores pairs with empty count' '\n \ttest_must_fail env \\\n \t\tGIT_CONFIG_COUNT= GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one >error &&\n+\t\tgit config pair.one 2>error &&\n \ttest_must_be_empty error\n '\n \n-- \n2.40.0\n\n"},{"id":"475638","messageId":"20230418175034.982433-4-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230418175034.982433-1-rybak.a.v@gmail.com","subject":"[RFC PATCH v2 3/3] t1300: add tests for missing keys","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-18T17:50:34Z","receivedAt":"2023-04-18T17:50:54Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"There several tests in t1300-config.sh that validate failing invocations\nof \"git config\". However, there are no tests that check what happens\nwhen \"git config\" is asked to retrieve a value for a missing key.\n\nAdd tests that check this for various combinations of \"
.\".\n---\n\nThe patch is marked as RFC, because I'm not sure if such a test is useful or\nnot. Even if it is not useful as a test, maybe it would be useful as\ndocumentation of behavior for people reading \"t1300-config.sh\"?\n\nHaving multiple tests for different kinds of keys might also be overkill, and\njust one test for a single key is enough. Conversely, there aren't enough\ntests -- tests for keys with subsections are missing.\n\n t/t1300-config.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 20a15ede5c..a646ddd231 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -98,6 +98,20 @@ test_expect_success 'subsections are not canonicalized by git-config' '\n \ttest_cmp_config two section.SubSection.key\n '\n \n+test_missing_key () {\n+\tlocal key=$1\n+\tlocal title=$2\n+\ttest_expect_success \"value for $title is not printed\" \"\n+\t\ttest_must_fail git config $key >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_must_be_empty err\n+\t\"\n+}\n+\n+test_missing_key 'missingsection.missingkey' 'missing section and missing key'\n+test_missing_key 'missingsection.penguin' 'missing section and existing key'\n+test_missing_key 'section.missingkey' 'existing section and missing key'\n+\n cat > .git/config <<\\EOF\n [alpha]\n bar = foo\n-- \n2.40.0\n\n"},{"id":"475639","messageId":"e2cdb2f3-2cd1-c7fc-be9e-c518f17fc175@gmail.com","threadId":"59519","inReplyTo":"20230418175034.982433-1-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 0/3] git config tests for \"'git config ignores pairs ...\"","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-18T17:53:18Z","receivedAt":"2023-04-18T17:53:53Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 18/04/2023 19:50, Andrei Rybak wrote:\n> v1 cover letter:\n> https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/\n> \n> While preparing v2, I've realized that I've failed to click \"Reply All\" when\n> writing my previous email in this subthread. I apologize.\n> https://lore.kernel.org/git/CACayv=jL4t3cUVS=xXQ3fLxF26vDXRJ3khs2y4UjzBw947JVkw@mail.gmail.com/\n> \n> Changes since v2:\n\nSorry, I meant to say \"since v1\" here.\n\n> - Rewritten commit message for patch 2.\n> - New RFC patch 3.\n> \n> Andrei Rybak (3):\n> t1300: drop duplicate test\n> t1300: check stderr for \"ignores pairs\" tests\n> t1300: add tests for missing keys\n> \n> t/t1300-config.sh | 30 +++++++++++++++++++-----------\n> 1 file changed, 19 insertions(+), 11 deletions(-)\n> \n\n"},{"id":"475642","messageId":"xmqqfs8xdokb.fsf@gitster.g","threadId":"59519","inReplyTo":"20230418175034.982433-2-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 1/3] t1300: drop duplicate test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-18T18:37:24Z","receivedAt":"2023-04-18T18:37:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak writes:\n\n> There are two almost identical tests called 'git config ignores pairs\n> with zero count' in file t1300-config.sh. Drop the first of these and\n> keep the one that contains more assertions.\n\nGood eyes. I can see that the other one catches the error output\nfrom the identical command and makes sure it exits silently.\n\nWill queue. Thanks.\n\n\n> Signed-off-by: Andrei Rybak \n> ---\n> t/t1300-config.sh | 7 -------\n> 1 file changed, 7 deletions(-)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 2575279ab8..696dca17c6 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1458,13 +1458,6 @@ test_expect_success 'git config ignores pairs without count' '\n> \ttest_must_be_empty error\n> '\n> \n> -test_expect_success 'git config ignores pairs with zero count' '\n> -\ttest_must_fail env \\\n> -\t\tGIT_CONFIG_COUNT=0 \\\n> -\t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n> -\t\tgit config pair.one\n> -'\n> -\n> test_expect_success 'git config ignores pairs exceeding count' '\n> \tGIT_CONFIG_COUNT=1 \\\n> \t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n"},{"id":"475643","messageId":"xmqqa5z5dog5.fsf@gitster.g","threadId":"59519","inReplyTo":"20230418175034.982433-3-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 2/3] t1300: check stderr for \"ignores pairs\" tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-18T18:39:54Z","receivedAt":"2023-04-18T18:39:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak writes:\n\n> Tests \"git config ignores pairs ...\" in t1300-config.sh validate that\n> \"git config\" ignores various kinds of supplied pairs of environment\n> variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* depending on\n> GIT_CONFIG_COUNT. By \"ignores\" here we mean that \"git config\" abides by\n> the value of environment variable GIT_CONFIG_COUNT and doesn't use\n> key-value pairs outside of the supplied GIT_CONFIG_COUNT when trying to\n> produce a value for config key \"pair.one\".\n\nCorrect.\n\n> \tGIT_CONFIG_COUNT=1 \\\n> \t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n> \t\tGIT_CONFIG_KEY_1=\"pair.two\" GIT_CONFIG_VALUE_1=\"value\" \\\n> -\t\tgit config --get-regexp \"pair.*\" >actual &&\n> +\t\tgit config --get-regexp \"pair.*\" >actual 2>error &&\n> \tcat >expect <<-EOF &&\n> \tpair.one value\n> \tEOF\n> -\ttest_cmp expect actual\n> +\ttest_cmp expect actual &&\n> +\ttest_must_be_empty error\n> '\n\nLooks good.\n\n> test_expect_success 'git config ignores pairs with zero count' '\n> \ttest_must_fail env \\\n> \t\tGIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n> -\t\tgit config pair.one >error &&\n> +\t\tgit config pair.one 2>error &&\n> \ttest_must_be_empty error\n> '\n\nLooks good too.\n\n> test_expect_success 'git config ignores pairs with empty count' '\n> \ttest_must_fail env \\\n> \t\tGIT_CONFIG_COUNT= GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n> -\t\tgit config pair.one >error &&\n> +\t\tgit config pair.one 2>error &&\n> \ttest_must_be_empty error\n> '\n\nLooks good too.\n\nWill queue. Thanks.\n"},{"id":"475651","messageId":"xmqqr0shc7i7.fsf@gitster.g","threadId":"59519","inReplyTo":"20230418175034.982433-4-rybak.a.v@gmail.com","subject":"Re: [RFC PATCH v2 3/3] t1300: add tests for missing keys","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-18T19:31:12Z","receivedAt":"2023-04-18T19:31:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak writes:\n\n> +test_missing_key () {\n> +\tlocal key=$1\n> +\tlocal title=$2\n> +\ttest_expect_success \"value for $title is not printed\" \"\n> +\t\ttest_must_fail git config $key >out 2>err &&\n> +\t\ttest_must_be_empty out &&\n> +\t\ttest_must_be_empty err\n> +\t\"\n> +}\n\nIn this case it would not make any difference only because no caller\nwill feed $key that can be split at $IFS, but it is a good practice\nto enclose the executed body of test_expect_* script inside a single\nquote *and* let variable substitution to happen inside eval; in\nother words, you should write the above like so:\n\n(GOOD)\ttest_expect_success \"value for $title is not printed\" '\n\t\ttest_must_fail git config \"$key\" >out 2>err &&\n\t\ttest_must_be_empty out &&\n\t\ttest_must_be_empty err\n\t'\n\nThe difference is a bit subtle. In the original version, the string\nfed to \"eval\" when \"test_expect_success\" runs has $key already\nsubstituted. So if $key has $IFS whitespace in it, the command line\narguments of the \"git config\" would not be what you think your callers\nare feeding. An attempt to keep it together by enclosing inside a\ndouble quote, still trying to salvage the pattern to enclose the\nexecuted body inside a pair of double quotes, i.e.\n\n(BAD)\ttest_expect_success \"value for $title is not printed\" \"\n\t\ttest_must_fail git config \\\"$key\\\" >out 2>err &&\n\t\ttest_must_be_empty out &&\n\t\ttest_must_be_empty err\n\t\"\n\nwould fail when $key has an unbalanced double quotes and cause a\nsyntax error.\n\nAgain, in this case it would be OK due to the limited callers, but\nit is a good discipline to follow, as others less familiar with our\ntests scripts than you would copy from what you write in your\npatches.\n\nThe title string (e.g. \"value for $title is not printed\") does not\ngo through eval, and it needs to be quoted with double quotes for\nthe $title to be substituted, by the way.\n\n> +test_missing_key 'missingsection.missingkey' 'missing section and missing key'\n> +test_missing_key 'missingsection.penguin' 'missing section and existing key'\n> +test_missing_key 'section.missingkey' 'existing section and missing key'\n\n"},{"id":"475900","messageId":"20230423134649.431783-2-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230423134649.431783-1-rybak.a.v@gmail.com","subject":"[PATCH v3 1/3] t1300: drop duplicate test","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-23T13:46:47Z","receivedAt":"2023-04-23T13:46:59Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"There are two almost identical tests called 'git config ignores pairs\nwith zero count' in file t1300-config.sh. Drop the first of these and\nkeep the one that contains more assertions.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 7 -------\n 1 file changed, 7 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 2575279ab8..696dca17c6 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1458,13 +1458,6 @@ test_expect_success 'git config ignores pairs without count' '\n \ttest_must_be_empty error\n '\n \n-test_expect_success 'git config ignores pairs with zero count' '\n-\ttest_must_fail env \\\n-\t\tGIT_CONFIG_COUNT=0 \\\n-\t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one\n-'\n-\n test_expect_success 'git config ignores pairs exceeding count' '\n \tGIT_CONFIG_COUNT=1 \\\n \t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-- \n2.40.0\n\n"},{"id":"475901","messageId":"20230423134649.431783-1-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230418175034.982433-1-rybak.a.v@gmail.com","subject":"[PATCH v3 0/3] git config tests for \"'git config ignores pairs ...\"","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-23T13:46:46Z","receivedAt":"2023-04-23T13:47:00Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Clean up existing tests and add new tests in t1300-config.sh.\n\nv1 cover letter:\n https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/\nv2 cover letter:\n https://lore.kernel.org/git/20230418175034.982433-1-rybak.a.v@gmail.com/\n\nChanges since v2:\n - Patch 3 updates:\n - Added missing word \"are\" in commit message\n - Added signoff\n - Wrapped $1 and $2 in double quotes for consistency with\n `git grep -P 'local .*=\"?[$][1-9]\"? &&$' -- t`\n - Added &&-chainging to function \"test_missing_key\"\n - Fixed quoting so that substitution of $key happens inside eval\n - Added tests about config subsections\n\nChanges since v1:\n - Rewritten commit message for patch 2.\n - New RFC patch 3.\n\nAndrei Rybak (3):\n t1300: drop duplicate test\n t1300: check stderr for \"ignores pairs\" tests\n t1300: add tests for missing keys\n\n t/t1300-config.sh | 33 ++++++++++++++++++++++-----------\n 1 file changed, 22 insertions(+), 11 deletions(-)\n\nRange diff against v2:\n1: cf812f4fa8 = 1: cf812f4fa8 t1300: drop duplicate test\n2: b4132adea8 = 2: b4132adea8 t1300: check stderr for \"ignores pairs\" tests\n3: 6126f42449 ! 3: ba536bf855 t1300: add tests for missing keys\n @@ Metadata\n ## Commit message ##\n t1300: add tests for missing keys\n \n - There several tests in t1300-config.sh that validate failing invocations\n - of \"git config\". However, there are no tests that check what happens\n - when \"git config\" is asked to retrieve a value for a missing key.\n + There are several tests in t1300-config.sh that validate failing\n + invocations of \"git config\". However, there are no tests that check\n + what happens when \"git config\" is asked to retrieve a value for a\n + missing key.\n \n - Add tests that check this for various combinations of \"
.\".\n + Add tests that check this for various combinations of \"
.\"\n + and \"
..\".\n +\n + Signed-off-by: Andrei Rybak \n \n ## t/t1300-config.sh ##\n @@ t/t1300-config.sh: test_expect_success 'subsections are not canonicalized by git-config' '\n @@ t/t1300-config.sh: test_expect_success 'subsections are not canonicalized by git\n '\n \n +test_missing_key () {\n -+\tlocal key=$1\n -+\tlocal title=$2\n -+\ttest_expect_success \"value for $title is not printed\" \"\n -+\t\ttest_must_fail git config $key >out 2>err &&\n ++\tlocal key=\"$1\" &&\n ++\tlocal title=\"$2\" &&\n ++\ttest_expect_success \"value for $title is not printed\" '\n ++\t\ttest_must_fail git config \"$key\" >out 2>err &&\n +\t\ttest_must_be_empty out &&\n +\t\ttest_must_be_empty err\n -+\t\"\n ++\t'\n +}\n +\n +test_missing_key 'missingsection.missingkey' 'missing section and missing key'\n +test_missing_key 'missingsection.penguin' 'missing section and existing key'\n +test_missing_key 'section.missingkey' 'existing section and missing key'\n ++test_missing_key 'section.MissingSubSection.missingkey' 'missing subsection and missing key'\n ++test_missing_key 'section.SubSection.missingkey' 'existing subsection and missing key'\n ++test_missing_key 'section.MissingSubSection.key' 'missing subsection and existing key'\n +\n cat > .git/config <<\\EOF\n [alpha]\n\n-- \n2.40.0\n"},{"id":"475902","messageId":"20230423134649.431783-3-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230423134649.431783-1-rybak.a.v@gmail.com","subject":"[PATCH v3 2/3] t1300: check stderr for \"ignores pairs\" tests","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-23T13:46:48Z","receivedAt":"2023-04-23T13:47:01Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests \"git config ignores pairs ...\" in t1300-config.sh validate that\n\"git config\" ignores various kinds of supplied pairs of environment\nvariables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* depending on\nGIT_CONFIG_COUNT. By \"ignores\" here we mean that \"git config\" abides by\nthe value of environment variable GIT_CONFIG_COUNT and doesn't use\nkey-value pairs outside of the supplied GIT_CONFIG_COUNT when trying to\nproduce a value for config key \"pair.one\".\n\nThese tests also validate that \"git config\" doesn't complain about\nmismatched environment variables to standard error. This is validated\nby redirecting the standard error to a file called \"error\" and asserting\nthat it is empty. However, two of these tests incorrectly redirect to\nstandard output while calling the file \"error\", and test 'git config\nignores pairs exceeding count' doesn't validate standard error at all.\n\nFix these tests by redirecting standard error to file \"error\" and\nasserting its emptiness.\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 696dca17c6..20a15ede5c 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '\n \tGIT_CONFIG_COUNT=1 \\\n \t\tGIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n \t\tGIT_CONFIG_KEY_1=\"pair.two\" GIT_CONFIG_VALUE_1=\"value\" \\\n-\t\tgit config --get-regexp \"pair.*\" >actual &&\n+\t\tgit config --get-regexp \"pair.*\" >actual 2>error &&\n \tcat >expect <<-EOF &&\n \tpair.one value\n \tEOF\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty error\n '\n \n test_expect_success 'git config ignores pairs with zero count' '\n \ttest_must_fail env \\\n \t\tGIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one >error &&\n+\t\tgit config pair.one 2>error &&\n \ttest_must_be_empty error\n '\n \n test_expect_success 'git config ignores pairs with empty count' '\n \ttest_must_fail env \\\n \t\tGIT_CONFIG_COUNT= GIT_CONFIG_KEY_0=\"pair.one\" GIT_CONFIG_VALUE_0=\"value\" \\\n-\t\tgit config pair.one >error &&\n+\t\tgit config pair.one 2>error &&\n \ttest_must_be_empty error\n '\n \n-- \n2.40.0\n\n"},{"id":"475903","messageId":"20230423134649.431783-4-rybak.a.v@gmail.com","threadId":"59519","inReplyTo":"20230423134649.431783-1-rybak.a.v@gmail.com","subject":"[PATCH v3 3/3] t1300: add tests for missing keys","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-04-23T13:46:49Z","receivedAt":"2023-04-23T13:47:04Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"There are several tests in t1300-config.sh that validate failing\ninvocations of \"git config\". However, there are no tests that check\nwhat happens when \"git config\" is asked to retrieve a value for a\nmissing key.\n\nAdd tests that check this for various combinations of \"
.\"\nand \"
..\".\n\nSigned-off-by: Andrei Rybak \n---\n t/t1300-config.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 20a15ede5c..423948f384 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -98,6 +98,23 @@ test_expect_success 'subsections are not canonicalized by git-config' '\n \ttest_cmp_config two section.SubSection.key\n '\n \n+test_missing_key () {\n+\tlocal key=\"$1\" &&\n+\tlocal title=\"$2\" &&\n+\ttest_expect_success \"value for $title is not printed\" '\n+\t\ttest_must_fail git config \"$key\" >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_must_be_empty err\n+\t'\n+}\n+\n+test_missing_key 'missingsection.missingkey' 'missing section and missing key'\n+test_missing_key 'missingsection.penguin' 'missing section and existing key'\n+test_missing_key 'section.missingkey' 'existing section and missing key'\n+test_missing_key 'section.MissingSubSection.missingkey' 'missing subsection and missing key'\n+test_missing_key 'section.SubSection.missingkey' 'existing subsection and missing key'\n+test_missing_key 'section.MissingSubSection.key' 'missing subsection and existing key'\n+\n cat > .git/config <<\\EOF\n [alpha]\n bar = foo\n-- \n2.40.0\n\n"},{"id":"476383","messageId":"xmqq8re74t3j.fsf@gitster.g","threadId":"59519","inReplyTo":"20230417191044.909094-1-rybak.a.v@gmail.com","subject":"Re: [PATCH v3 0/6] t: fix unused files, part 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T21:52:48Z","receivedAt":"2023-05-01T21:52:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak writes:\n\n> Creation of files from redirecting output of Git commands in tests has been\n> removed for files which aren't being used for assertions. CC'ed are authors of\n> the affected tests.\n>\n> v1 cover letter:\n> https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/\n> v2 cover letter:\n> https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/\n\nThis round has not seen any further comments; shall we consider it\npretty much done and ready to move to 'next' by now?\n\nThanks.\n"},{"id":"476385","messageId":"xmqqy1m73e3o.fsf@gitster.g","threadId":"59519","inReplyTo":"20230423134649.431783-1-rybak.a.v@gmail.com","subject":"Re: [PATCH v3 0/3] git config tests for \"'git config ignores pairs ...\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T22:02:03Z","receivedAt":"2023-05-01T22:02:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak writes:\n\n> Clean up existing tests and add new tests in t1300-config.sh.\n>\n> v1 cover letter:\n> https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/\n> v2 cover letter:\n> https://lore.kernel.org/git/20230418175034.982433-1-rybak.a.v@gmail.com/\n\nThis hasn't seen any further reactions. Is everybody happy to see\nus declare victory and merge it down to 'next'?\n\nThanks.\n"},{"id":"476456","messageId":"d0ff3e23-480b-4292-85fd-3af5f065ef33@gmail.com","threadId":"59519","inReplyTo":"xmqqy1m73e3o.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] git config tests for \"'git config ignores pairs ...\"","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-05-02T19:58:01Z","receivedAt":"2023-05-02T19:58:14Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 02/05/2023 00:02, Junio C Hamano wrote:\n> Andrei Rybak writes:\n> \n>> Clean up existing tests and add new tests in t1300-config.sh.\n>>\n>> v1 cover letter:\n>> https://lore.kernel.org/git/20230414081352.810296-1-rybak.a.v@gmail.com/\n>> v2 cover letter:\n>> https://lore.kernel.org/git/20230418175034.982433-1-rybak.a.v@gmail.com/\n> \n> This hasn't seen any further reactions. Is everybody happy to see\n> us declare victory and merge it down to 'next'?\n\nI'm happy with this series, especially because new tests in patch 3/3 indirectly\naddress some of the concerns for unused files removal in t1300 in the original\nseries of this thread, \"t: fix unused files, part 2\".\n\n https://lore.kernel.org/git/230406.86pm8htnfk.gmgdl@evledraar.gmail.com/\n\n> Thanks.\n\n"},{"id":"476459","messageId":"73ead9f4-4e12-09c6-8abb-e338f71efceb@gmail.com","threadId":"59519","inReplyTo":"xmqq8re74t3j.fsf@gitster.g","subject":"Re: [PATCH v3 0/6] t: fix unused files, part 2","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-05-02T21:03:36Z","receivedAt":"2023-05-02T21:04:11Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 01/05/2023 23:52, Junio C Hamano wrote:\n> Andrei Rybak writes:\n> \n>> Creation of files from redirecting output of Git commands in tests has been\n>> removed for files which aren't being used for assertions. CC'ed are authors of\n>> the affected tests.\n>>\n>> v1 cover letter:\n>> https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/\n>> v2 cover letter:\n>> https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/\n> \n> This round has not seen any further comments; shall we consider it\n> pretty much done and ready to move to 'next' by now?\n\nIn general, I'm OK with the series as is.\n\nWhile answering Ævar's questions to some of the patches in v2, I\nwent quite deep in trying to investigate what is and isn't important\nto validate/assert in particular tests, but I haven't come up with\na good way to include this information in commit messages for this\nseries.\n\nNotes per patch:\n\n - 1/6 for t0300 is just an explanation about why one out of three\n cases in one test does not check stdout (and doesn't need to).\n https://lore.kernel.org/git/db2de983-9b1f-5efb-0fdc-cc704e6b875b@gmail.com/\n\n - 3/6 for t1300 lead to a separate series\n https://lore.kernel.org/git/20230423134649.431783-1-rybak.a.v@gmail.com/\n\n - 4/6 for t1450 had an idea for a test 'fresh repository has no\n dangling objects'. I'm doubtful about usefulness of such a test,\n so hasn't sent it as a patch yet.\n https://lore.kernel.org/git/35bc2dc5-d5cb-3492-ff94-41b93b7563d4@gmail.com/\n\n - 6/6 for t2019 -- a dive into how output of \"git checkout\" is tested\n https://lore.kernel.org/git/4ef5464b-31dd-3c3e-05be-9891162e4f05@gmail.com/#t\n\nPatches 2/6 and 5/6 are different from others, because they fix\nmore obvious issues.\n\n> Thanks.\n\n"},{"id":"476486","messageId":"CABPp-BFCBPTBc9savP13gNFmBo_WCnZHUv_Z8bWE6T6GZOrs-Q@mail.gmail.com","threadId":"59519","inReplyTo":"xmqq8re74t3j.fsf@gitster.g","subject":"Re: [PATCH v3 0/6] t: fix unused files, part 2","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-05-03T04:11:16Z","receivedAt":"2023-05-03T04:11:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, May 1, 2023 at 3:04 PM Junio C Hamano wrote:\n>\n> Andrei Rybak writes:\n>\n> > Creation of files from redirecting output of Git commands in tests has been\n> > removed for files which aren't being used for assertions. CC'ed are authors of\n> > the affected tests.\n> >\n> > v1 cover letter:\n> > https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/\n> > v2 cover letter:\n> > https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/\n>\n> This round has not seen any further comments; shall we consider it\n> pretty much done and ready to move to 'next' by now?\n\nI think so. I read through the series. I also read Ævar's and\nAndrei's extended comments on v2. Ævar does bring up good points\nabout whether we should be testing more, but Andrei I think did a good\ninvestigation, cc'ed original code authors (who would be the right\nones to comment on whether those other things should be tested), etc.\nThe tests as-is before this series are harder than necessary to\nunderstand, and Andrei cleans them up. It feels like good forward\nprogress to me, even if there _might_ be a better eventual optimal.\n\nReviewed-by: Elijah Newren \n"},{"id":"476514","messageId":"xmqqfs8dwgtt.fsf@gitster.g","threadId":"59519","inReplyTo":"CABPp-BFCBPTBc9savP13gNFmBo_WCnZHUv_Z8bWE6T6GZOrs-Q@mail.gmail.com","subject":"Re: [PATCH v3 0/6] t: fix unused files, part 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-03T15:54:54Z","receivedAt":"2023-05-03T15:55:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Mon, May 1, 2023 at 3:04 PM Junio C Hamano wrote:\n>>\n>> Andrei Rybak writes:\n>>\n>> > Creation of files from redirecting output of Git commands in tests has been\n>> > removed for files which aren't being used for assertions. CC'ed are authors of\n>> > the affected tests.\n>> >\n>> > v1 cover letter:\n>> > https://lore.kernel.org/git/20230401212858.266508-1-rybak.a.v@gmail.com/\n>> > v2 cover letter:\n>> > https://lore.kernel.org/git/20230403223338.468025-1-rybak.a.v@gmail.com/\n>>\n>> This round has not seen any further comments; shall we consider it\n>> pretty much done and ready to move to 'next' by now?\n> \n> I think so. ...\n>\n> Reviewed-by: Elijah Newren \n\nThanks.\n"}]}