{"thread":{"id":"58713","subject":"[PATCH 0/3] tests: improve misc run-command, hook, submodule tests","startedAt":"2022-10-29T03:02:18Z","lastAt":"2022-10-31T23:59:03Z","messageCount":11,"participants":["Ævar Arnfjörð Bjarmason","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"466000","messageId":"cover-0.3-00000000000-20221029T025520Z-avarab@gmail.com","threadId":"58713","inReplyTo":null,"subject":"[PATCH 0/3] tests: improve misc run-command, hook, submodule tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-29T02:59:44Z","receivedAt":"2022-10-29T03:02:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"These are small tests changes left out of[1], which has since\ngraduated as 6ae1a6eaf26 (Merge branch 'ab/run-hook-api-cleanup',\n2022-10-27). The range-diff is to the relevant part of v2 of that\ntopic.\n\nI updated the commit message of 2/3, and adjusted 3/3 to make the diff\nsmaller, but functionally the same as far as how the test works.\n\nThese changes have already been reviewed, and the only\n(understandable) objection was that they were side-tracks in v2 of [1]\nthat we could do without in the context of that topic.\n\n1. https://lore.kernel.org/git/cover-v3-00.15-00000000000-20221012T205712Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (3):\n  hook tests: fix redirection logic error in 96e7225b310\n  submodule tests: reset \"trace.out\" between \"grep\" invocations\n  run-command tests: test stdout of run_command_parallel()\n\n t/t0061-run-command.sh      | 15 ++++++++++-----\n t/t1800-hook.sh             |  2 +-\n t/t5526-fetch-submodules.sh |  8 ++++++++\n 3 files changed, 19 insertions(+), 6 deletions(-)\n\nRange-diff:\n1:  bc51dfcb1be = 1:  1ba41a5842c hook tests: fix redirection logic error in 96e7225b310\n2:  3027f5587a7 ! 2:  708375e3104 submodule tests: reset \"trace.out\" between \"grep\" invocations\n    @@ Commit message\n         not print out a \"9 tasks\" line, but let's be paranoid and stop\n         implicitly assuming that that's the case.\n     \n    +    This change was originally left out of 51243f9f0f6 (run-command API:\n    +    don't fall back on online_cpus(), 2022-10-12), which added the\n    +    \">trace.out\" seen at the end of the context.\n    +\n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## t/t5526-fetch-submodules.sh ##\n    @@ t/t5526-fetch-submodules.sh: test_expect_success \"'fetch.recurseSubmodules=on-de\n     +\t\t>trace.out &&\n     +\n      \t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n    --\t\tgrep \"9 tasks\" trace.out\n    -+\t\tgrep \"9 tasks\" trace.out &&\n    -+\t\t>trace.out &&\n    - \t)\n    - '\n    - \n    + \t\tgrep \"9 tasks\" trace.out &&\n    + \t\t>trace.out &&\n3:  c4923358bbd ! 3:  6d6c2241bf2 run-command tests: test stdout of run_command_parallel()\n    @@ t/t0061-run-command.sh: World\n      \n      test_expect_success 'run_command runs in parallel with more jobs available than tasks' '\n     -\ttest-tool run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n    --\ttest_cmp expect actual\n    -+\ttest-tool run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n    ++\ttest-tool run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>actual &&\n     +\ttest_must_be_empty out &&\n    -+\ttest_cmp expect err\n    + \ttest_cmp expect actual\n      '\n      \n    - test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '\n     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs ungrouped in parallel with more jobs avail\n      '\n      \n      test_expect_success 'run_command runs in parallel with as many jobs as tasks' '\n     -\ttest-tool run-command run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n    --\ttest_cmp expect actual\n    -+\ttest-tool run-command run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n    ++\ttest-tool run-command run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>actual &&\n     +\ttest_must_be_empty out &&\n    -+\ttest_cmp expect err\n    + \ttest_cmp expect actual\n      '\n      \n    - test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '\n     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs ungrouped in parallel with as many jobs as\n      '\n      \n      test_expect_success 'run_command runs in parallel with more tasks than jobs available' '\n     -\ttest-tool run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n    --\ttest_cmp expect actual\n    -+\ttest-tool run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n    ++\ttest-tool run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>actual &&\n     +\ttest_must_be_empty out &&\n    -+\ttest_cmp expect err\n    + \ttest_cmp expect actual\n      '\n      \n    - test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '\n     @@ t/t0061-run-command.sh: asking for a quick stop\n      EOF\n      \n      test_expect_success 'run_command is asked to abort gracefully' '\n     -\ttest-tool run-command run-command-abort 3 false 2>actual &&\n    --\ttest_cmp expect actual\n    -+\ttest-tool run-command run-command-abort 3 false >out 2>err &&\n    ++\ttest-tool run-command run-command-abort 3 false >out 2>actual &&\n     +\ttest_must_be_empty out &&\n    -+\ttest_cmp expect err\n    + \ttest_cmp expect actual\n      '\n      \n    - test_expect_success 'run_command is asked to abort gracefully (ungroup)' '\n     @@ t/t0061-run-command.sh: no further jobs available\n      EOF\n      \n      test_expect_success 'run_command outputs ' '\n     -\ttest-tool run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n    --\ttest_cmp expect actual\n    -+\ttest-tool run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n    ++\ttest-tool run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>actual &&\n     +\ttest_must_be_empty out &&\n    -+\ttest_cmp expect err\n    + \ttest_cmp expect actual\n      '\n      \n    - test_expect_success 'run_command outputs (ungroup) ' '\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466001","messageId":"patch-1.3-1ba41a5842c-20221029T025520Z-avarab@gmail.com","threadId":"58713","inReplyTo":"cover-0.3-00000000000-20221029T025520Z-avarab@gmail.com","subject":"[PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-29T02:59:45Z","receivedAt":"2022-10-29T03:02:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The tests added in 96e7225b310 (hook: add 'run' subcommand,\n2021-12-22) were redirecting to \"actual\" both in the body of the hook\nitself and in the testing code below.\n\nThe net result was that the \"2>>actual\" redirection later in the test\nwasn't doing anything. Let's have those redirection do what it looks\nlike they're doing.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t1800-hook.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 43fcb7c0bfc..2ef3579fa7c 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -95,7 +95,7 @@ test_expect_success 'git hook run -- out-of-repo runs excluded' '\n test_expect_success 'git -c core.hooksPath=<PATH> hook run' '\n \tmkdir my-hooks &&\n \twrite_script my-hooks/test-hook <<-\\EOF &&\n-\techo Hook ran $1 >>actual\n+\techo Hook ran $1\n \tEOF\n \n \tcat >expect <<-\\EOF &&\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466002","messageId":"patch-2.3-708375e3104-20221029T025520Z-avarab@gmail.com","threadId":"58713","inReplyTo":"cover-0.3-00000000000-20221029T025520Z-avarab@gmail.com","subject":"[PATCH 2/3] submodule tests: reset \"trace.out\" between \"grep\" invocations","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-29T02:59:46Z","receivedAt":"2022-10-29T03:02:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix test patterns added in 62104ba14af (submodules: allow parallel\nfetching, add tests and documentation, 2015-12-15) and\na028a1930c6 (fetching submodules: respect `submodule.fetchJobs` config\noption, 2016-02-29).\n\nIn the former case we were leaving a trace.out file at the top-level\nfor any subsequent tests (there are none, currently). Let's clean the\nfile up instead.\n\nIn the latter case we were testing that a given configuration would\nresult in \"N tasks\" in the log, but we were grepping through the log\nfor all previous such tests, when we really meant to clear the logs\nbetween the \"grep\" invocations.\n\nIn practice this resulted in no logic error, as e.g. \"--fetch 7\" would\nnot print out a \"9 tasks\" line, but let's be paranoid and stop\nimplicitly assuming that that's the case.\n\nThis change was originally left out of 51243f9f0f6 (run-command API:\ndon't fall back on online_cpus(), 2022-10-12), which added the\n\">trace.out\" seen at the end of the context.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t5526-fetch-submodules.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 75da8acf8f4..b9546ef8e5e 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -178,6 +178,7 @@ test_expect_success \"submodule.recurse option triggers recursive fetch\" '\n '\n \n test_expect_success \"fetch --recurse-submodules -j2 has the same output behaviour\" '\n+\ttest_when_finished \"rm -f trace.out\" &&\n \tadd_submodule_commits &&\n \t(\n \t\tcd downstream &&\n@@ -705,15 +706,22 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' works also without .git\n \n test_expect_success 'fetching submodules respects parallel settings' '\n \tgit config fetch.recurseSubmodules true &&\n+\ttest_when_finished \"rm -f downstream/trace.out\" &&\n \t(\n \t\tcd downstream &&\n \t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n \t\tgrep \"1 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n \t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&\n \t\tgrep \"7 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n \t\tgit config submodule.fetchJobs 8 &&\n \t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n \t\tgrep \"8 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n \t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n \t\tgrep \"9 tasks\" trace.out &&\n \t\t>trace.out &&\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466003","messageId":"patch-3.3-6d6c2241bf2-20221029T025520Z-avarab@gmail.com","threadId":"58713","inReplyTo":"cover-0.3-00000000000-20221029T025520Z-avarab@gmail.com","subject":"[PATCH 3/3] run-command tests: test stdout of run_command_parallel()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-29T02:59:47Z","receivedAt":"2022-10-29T03:02:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Extend the tests added in c553c72eed6 (run-command: add an\nasynchronous parallel child processor, 2015-12-15) to test stdout in\naddition to stderr.\n\nWhen the \"ungroup\" feature was added in fd3aaf53f71 (run-command: add\nan \"ungroup\" option to run_process_parallel(), 2022-06-07) its tests\nwere made to test both the stdout and stderr, but these existing tests\nwere left alone. Let's also exhaustively test our expected output\nhere.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t0061-run-command.sh | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 7b5423eebda..e2411f6a9bd 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -130,7 +130,8 @@ World\n EOF\n \n test_expect_success 'run_command runs in parallel with more jobs available than tasks' '\n-\ttest-tool run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest-tool run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>actual &&\n+\ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n@@ -141,7 +142,8 @@ test_expect_success 'run_command runs ungrouped in parallel with more jobs avail\n '\n \n test_expect_success 'run_command runs in parallel with as many jobs as tasks' '\n-\ttest-tool run-command run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest-tool run-command run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>actual &&\n+\ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n@@ -152,7 +154,8 @@ test_expect_success 'run_command runs ungrouped in parallel with as many jobs as\n '\n \n test_expect_success 'run_command runs in parallel with more tasks than jobs available' '\n-\ttest-tool run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest-tool run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>actual &&\n+\ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n@@ -172,7 +175,8 @@ asking for a quick stop\n EOF\n \n test_expect_success 'run_command is asked to abort gracefully' '\n-\ttest-tool run-command run-command-abort 3 false 2>actual &&\n+\ttest-tool run-command run-command-abort 3 false >out 2>actual &&\n+\ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n@@ -187,7 +191,8 @@ no further jobs available\n EOF\n \n test_expect_success 'run_command outputs ' '\n-\ttest-tool run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest-tool run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>actual &&\n+\ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466037","messageId":"Y12I7zSohPu2cyJt@nand.local","threadId":"58713","inReplyTo":"patch-1.3-1ba41a5842c-20221029T025520Z-avarab@gmail.com","subject":"Re: [PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-29T20:11:27Z","receivedAt":"2022-10-29T20:13:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"[+cc Emily]\n\nOn Sat, Oct 29, 2022 at 04:59:45AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\n> index 43fcb7c0bfc..2ef3579fa7c 100755\n> --- a/t/t1800-hook.sh\n> +++ b/t/t1800-hook.sh\n> @@ -95,7 +95,7 @@ test_expect_success 'git hook run -- out-of-repo runs excluded' '\n>  test_expect_success 'git -c core.hooksPath=<PATH> hook run' '\n>  \tmkdir my-hooks &&\n>  \twrite_script my-hooks/test-hook <<-\\EOF &&\n> -\techo Hook ran $1 >>actual\n> +\techo Hook ran $1\n>  \tEOF\n\nLooking reasonable to me. Though let's see what Emily thinks, too...\n\nThanks,\nTaylor\n"},{"id":"466038","messageId":"Y12JfADN/YRr9IaJ@nand.local","threadId":"58713","inReplyTo":"patch-2.3-708375e3104-20221029T025520Z-avarab@gmail.com","subject":"Re: [PATCH 2/3] submodule tests: reset \"trace.out\" between \"grep\" invocations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-29T20:13:48Z","receivedAt":"2022-10-29T20:13:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 29, 2022 at 04:59:46AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 75da8acf8f4..b9546ef8e5e 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -178,6 +178,7 @@ test_expect_success \"submodule.recurse option triggers recursive fetch\" '\n>  '\n>\n>  test_expect_success \"fetch --recurse-submodules -j2 has the same output behaviour\" '\n> +\ttest_when_finished \"rm -f trace.out\" &&\n>  \tadd_submodule_commits &&\n>  \t(\n>  \t\tcd downstream &&\n> @@ -705,15 +706,22 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' works also without .git\n>\n>  test_expect_success 'fetching submodules respects parallel settings' '\n>  \tgit config fetch.recurseSubmodules true &&\n> +\ttest_when_finished \"rm -f downstream/trace.out\" &&\n\nThese two seem OK to me, but...\n\n>  \t(\n>  \t\tcd downstream &&\n>  \t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n>  \t\tgrep \"1 tasks\" trace.out &&\n> +\t\t>trace.out &&\n> +\n\nI fail to see why these hunks are necessary. If we specify GIT_TRACE,\nand don't have a test_must_fail around the execution, then why should we\nfeel obligated to clean up the trace.out after every execution?\n\nIf we really are concerned about not cleaning up after ourselves, how\nabout writing to a separate file each time?\n\nThanks,\nTaylor\n"},{"id":"466039","messageId":"Y12JyOQZT8xKHC7k@nand.local","threadId":"58713","inReplyTo":"patch-3.3-6d6c2241bf2-20221029T025520Z-avarab@gmail.com","subject":"Re: [PATCH 3/3] run-command tests: test stdout of run_command_parallel()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-29T20:15:04Z","receivedAt":"2022-10-29T20:15:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 29, 2022 at 04:59:47AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Extend the tests added in c553c72eed6 (run-command: add an\n> asynchronous parallel child processor, 2015-12-15) to test stdout in\n> addition to stderr.\n>\n> When the \"ungroup\" feature was added in fd3aaf53f71 (run-command: add\n> an \"ungroup\" option to run_process_parallel(), 2022-06-07) its tests\n> were made to test both the stdout and stderr, but these existing tests\n> were left alone. Let's also exhaustively test our expected output\n> here.\n\nMakes sense, this patch looks good. Thanks.\n\nThanks,\nTaylor\n"},{"id":"466084","messageId":"221031.86tu3k6u2d.gmgdl@evledraar.gmail.com","threadId":"58713","inReplyTo":"Y12I7zSohPu2cyJt@nand.local","subject":"Re: [PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-31T12:44:44Z","receivedAt":"2022-10-31T12:46:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Oct 29 2022, Taylor Blau wrote:\n\n> [+cc Emily]\n>\n> On Sat, Oct 29, 2022 at 04:59:45AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\n>> index 43fcb7c0bfc..2ef3579fa7c 100755\n>> --- a/t/t1800-hook.sh\n>> +++ b/t/t1800-hook.sh\n>> @@ -95,7 +95,7 @@ test_expect_success 'git hook run -- out-of-repo runs excluded' '\n>>  test_expect_success 'git -c core.hooksPath=<PATH> hook run' '\n>>  \tmkdir my-hooks &&\n>>  \twrite_script my-hooks/test-hook <<-\\EOF &&\n>> -\techo Hook ran $1 >>actual\n>> +\techo Hook ran $1\n>>  \tEOF\n>\n> Looking reasonable to me. Though let's see what Emily thinks, too...\n\nFWIW Junio looked at it back in\nhttps://lore.kernel.org/git/xmqqczh84fpy.fsf@gitster.g/\n"},{"id":"466085","messageId":"221031.86pme86tcg.gmgdl@evledraar.gmail.com","threadId":"58713","inReplyTo":"Y12JfADN/YRr9IaJ@nand.local","subject":"Re: [PATCH 2/3] submodule tests: reset \"trace.out\" between \"grep\" invocations","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-31T12:50:07Z","receivedAt":"2022-10-31T13:00:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Oct 29 2022, Taylor Blau wrote:\n\n> On Sat, Oct 29, 2022 at 04:59:46AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n>> index 75da8acf8f4..b9546ef8e5e 100755\n>> --- a/t/t5526-fetch-submodules.sh\n>> +++ b/t/t5526-fetch-submodules.sh\n>> @@ -178,6 +178,7 @@ test_expect_success \"submodule.recurse option triggers recursive fetch\" '\n>>  '\n>>\n>>  test_expect_success \"fetch --recurse-submodules -j2 has the same output behaviour\" '\n>> +\ttest_when_finished \"rm -f trace.out\" &&\n>>  \tadd_submodule_commits &&\n>>  \t(\n>>  \t\tcd downstream &&\n>> @@ -705,15 +706,22 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' works also without .git\n>>\n>>  test_expect_success 'fetching submodules respects parallel settings' '\n>>  \tgit config fetch.recurseSubmodules true &&\n>> +\ttest_when_finished \"rm -f downstream/trace.out\" &&\n>\n> These two seem OK to me, but...\n>\n>>  \t(\n>>  \t\tcd downstream &&\n>>  \t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n>>  \t\tgrep \"1 tasks\" trace.out &&\n>> +\t\t>trace.out &&\n>> +\n>\n> I fail to see why these hunks are necessary. If we specify GIT_TRACE,\n> and don't have a test_must_fail around the execution, then why should we\n> feel obligated to clean up the trace.out after every execution?\n\nBecause the trace file isn't clobbered by each git command that\nspecifies GIT_TRACE, so these tests are basically doing:\n\n\t(echo foo; echo bar) >>trace &&\n\tgrep foo trace &&\n\n        (echo bar) >>trace &&\n\tgrep bar trace\n\nNow, it just so happens that the earlier command isn't echoing \"bar\" to\nthe file, so this is currently working out.\n\nBut it's a bad pattern to be pretending as though you care about the\nlast output (which was the intent of the test), when really what you're\ntesting is the combined output of all preceding commands.\n\nThis would also be a potenital landmine with \"test_must_fail\", just\nbecause the command failed we're not guaranteed to have written nothing\nto the log (and usually we'd get as far as to write something).\n\n>\n> If we really are concerned about not cleaning up after ourselves, how\n> about writing to a separate file each time?\n\nBetter yet would be to refactor this into a function, set that up and\n\"test_when_finished\" nuke it every time.\n\nBut I'm just going for the most minimal change here to not leave this\nlandmine in place. My 51243f9f0f6 (run-command API: don't fall back on\nonline_cpus(), 2022-10-12) already started using this pattern, and is on\n\"master\". I think a good incremental step is to do the bare minimum to\ndo the same for the rest, and guard any subsequent test from being\naffected by the \"trace.out\".\n\nSo, unless you insist I'd rather just change nothing about this series,\nand not get stuck in various re-rolls/commentary about what's the ideal\nrefactoring, once we're starting to do that anyway...\n"},{"id":"466141","messageId":"Y2BgwZ45C+Oqs+W0@nand.local","threadId":"58713","inReplyTo":"221031.86tu3k6u2d.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] hook tests: fix redirection logic error in 96e7225b310","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-31T23:56:49Z","receivedAt":"2022-10-31T23:56:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 01:44:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Sat, Oct 29 2022, Taylor Blau wrote:\n>\n> > [+cc Emily]\n> >\n> > On Sat, Oct 29, 2022 at 04:59:45AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\n> >> index 43fcb7c0bfc..2ef3579fa7c 100755\n> >> --- a/t/t1800-hook.sh\n> >> +++ b/t/t1800-hook.sh\n> >> @@ -95,7 +95,7 @@ test_expect_success 'git hook run -- out-of-repo runs excluded' '\n> >>  test_expect_success 'git -c core.hooksPath=<PATH> hook run' '\n> >>  \tmkdir my-hooks &&\n> >>  \twrite_script my-hooks/test-hook <<-\\EOF &&\n> >> -\techo Hook ran $1 >>actual\n> >> +\techo Hook ran $1\n> >>  \tEOF\n> >\n> > Looking reasonable to me. Though let's see what Emily thinks, too...\n>\n> FWIW Junio looked at it back in\n> https://lore.kernel.org/git/xmqqczh84fpy.fsf@gitster.g/\n\nGood, though I would like some feedback from Emily, too, seeing as most\nof this code blames back to her.\n\nThanks,\nTaylor\n"},{"id":"466142","messageId":"Y2BhQHynNWkl6cbY@nand.local","threadId":"58713","inReplyTo":"221031.86pme86tcg.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/3] submodule tests: reset \"trace.out\" between \"grep\" invocations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-31T23:58:56Z","receivedAt":"2022-10-31T23:59:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 01:50:07PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >>  \t(\n> >>  \t\tcd downstream &&\n> >>  \t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n> >>  \t\tgrep \"1 tasks\" trace.out &&\n> >> +\t\t>trace.out &&\n> >> +\n> >\n> > I fail to see why these hunks are necessary. If we specify GIT_TRACE,\n> > and don't have a test_must_fail around the execution, then why should we\n> > feel obligated to clean up the trace.out after every execution?\n>\n> Because the trace file isn't clobbered by each git command that\n> specifies GIT_TRACE, so these tests are basically doing:\n>\n> \t(echo foo; echo bar) >>trace &&\n> \tgrep foo trace &&\n>\n>         (echo bar) >>trace &&\n> \tgrep bar trace\n>\n> Now, it just so happens that the earlier command isn't echoing \"bar\" to\n> the file, so this is currently working out.\n\nAh, nicely explained. This is new to me, since I thought the behavior of\nboth GIT_TRACE and GIT_TRACE2 was to clobber the file.\n\n> But it's a bad pattern to be pretending as though you care about the\n> last output (which was the intent of the test), when really what you're\n> testing is the combined output of all preceding commands.\n>\n> This would also be a potenital landmine with \"test_must_fail\", just\n> because the command failed we're not guaranteed to have written nothing\n> to the log (and usually we'd get as far as to write something).\n\nYeah, both make sense, thanks.\n\nThanks,\nTaylor\n"}]}