patch, 3 partstests: improve misc run-command, hook, submodule tests

These are small tests changes left out of[1], which has since graduated as 6ae1a6eaf26 (Merge branch 'ab/run-hook-api-cleanup', 2022-10-27). The range-diff is to the relevant part of v2 of that topic.

I updated the commit message of 2/3, and adjusted 3/3 to make the diff smaller, but functionally the same as far as how the test works.

These changes have already been reviewed, and the only (understandable) objection was that they were side-tracks in v2 of [1] that we could do without in the context of that topic.

1. https://lore.kernel.org/git/cover-v3-00.15-00000000000-20221012T205712Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  hook tests: fix redirection logic error in 96e7225b310
  submodule tests: reset "trace.out" between "grep" invocations
  run-command tests: test stdout of run_command_parallel()

 t/t0061-run-command.sh      | 15 ++++++++++-----
 t/t1800-hook.sh             |  2 +-
 t/t5526-fetch-submodules.sh |  8 ++++++++
 3 files changed, 19 insertions(+), 6 deletions(-)

Range-diff:
1:  bc51dfcb1be = 1:  1ba41a5842c hook tests: fix redirection logic error in 96e7225b310
2:  3027f5587a7 ! 2:  708375e3104 submodule tests: reset "trace.out" between "grep" invocations
    @@ Commit message
         not print out a "9 tasks" line, but let's be paranoid and stop
         implicitly assuming that that's the case.
     
    +    This change was originally left out of 51243f9f0f6 (run-command API:
    +    don't fall back on online_cpus(), 2022-10-12), which added the
    +    ">trace.out" seen at the end of the context.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/t5526-fetch-submodules.sh ##
    @@ t/t5526-fetch-submodules.sh: test_expect_success "'fetch.recurseSubmodules=on-de
     +		>trace.out &&
     +
      		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
    --		grep "9 tasks" trace.out
    -+		grep "9 tasks" trace.out &&
    -+		>trace.out &&
    - 	)
    - '
    - 
    + 		grep "9 tasks" trace.out &&
    + 		>trace.out &&
3:  c4923358bbd ! 3:  6d6c2241bf2 run-command tests: test stdout of run_command_parallel()
    @@ t/t0061-run-command.sh: World
      
      test_expect_success 'run_command runs in parallel with more jobs available than tasks' '
     -	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs ungrouped in parallel with more jobs avail
      '
      
      test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
     -	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
     @@ t/t0061-run-command.sh: test_expect_success 'run_command runs ungrouped in parallel with as many jobs as
      '
      
      test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
     -	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
     @@ t/t0061-run-command.sh: asking for a quick stop
      EOF
      
      test_expect_success 'run_command is asked to abort gracefully' '
     -	test-tool run-command run-command-abort 3 false 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-abort 3 false >out 2>err &&
    ++	test-tool run-command run-command-abort 3 false >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
     @@ t/t0061-run-command.sh: no further jobs available
      EOF
      
      test_expect_success 'run_command outputs ' '
     -	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    --	test_cmp expect actual
    -+	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
    ++	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
     +	test_must_be_empty out &&
    -+	test_cmp expect err
    + 	test_cmp expect actual
      '
      
    - test_expect_success 'run_command outputs (ungroup) ' '

The tests added in 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) were redirecting to "actual" both in the body of the hook itself and in the testing code below.

The net result was that the "2>>actual" redirection later in the test wasn't doing anything. Let's have those redirection do what it looks like they're doing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1800-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

[+cc Emily]

On Sat, Oct 29, 2022 at 04:59:45AM +0200, Ævar Arnfjörð Bjarmason wrote:

Looking reasonable to me. Though let's see what Emily thinks, too...

Thanks, Taylor

On Sat, Oct 29 2022, Taylor Blau wrote:

FWIW Junio looked at it back in https://lore.kernel.org/git/xmqqczh84fpy.fsf@gitster.g/

On Mon, Oct 31, 2022 at 01:44:44PM +0100, Ævar Arnfjörð Bjarmason wrote:

Good, though I would like some feedback from Emily, too, seeing as most of this code blames back to her.

Thanks, Taylor

Fix test patterns added in 62104ba14af (submodules: allow parallel fetching, add tests and documentation, 2015-12-15) and a028a1930c6 (fetching submodules: respect `submodule.fetchJobs` config option, 2016-02-29).

In the former case we were leaving a trace.out file at the top-level for any subsequent tests (there are none, currently). Let's clean the file up instead.

In the latter case we were testing that a given configuration would result in "N tasks" in the log, but we were grepping through the log for all previous such tests, when we really meant to clear the logs between the "grep" invocations.

In practice this resulted in no logic error, as e.g. "--fetch 7" would not print out a "9 tasks" line, but let's be paranoid and stop implicitly assuming that that's the case.

This change was originally left out of 51243f9f0f6 (run-command API: don't fall back on online_cpus(), 2022-10-12), which added the ">trace.out" seen at the end of the context.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5526-fetch-submodules.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

On Sat, Oct 29, 2022 at 04:59:46AM +0200, Ævar Arnfjörð Bjarmason wrote:

These two seem OK to me, but...

I fail to see why these hunks are necessary. If we specify GIT_TRACE, and don't have a test_must_fail around the execution, then why should we feel obligated to clean up the trace.out after every execution?

If we really are concerned about not cleaning up after ourselves, how about writing to a separate file each time?

Thanks, Taylor

On Sat, Oct 29 2022, Taylor Blau wrote:

Because the trace file isn't clobbered by each git command that specifies GIT_TRACE, so these tests are basically doing:

	(echo foo; echo bar) >>trace &&
	grep foo trace &&

        (echo bar) >>trace &&
	grep bar trace

Now, it just so happens that the earlier command isn't echoing "bar" to the file, so this is currently working out.

But it's a bad pattern to be pretending as though you care about the last output (which was the intent of the test), when really what you're testing is the combined output of all preceding commands.

This would also be a potenital landmine with "test_must_fail", just because the command failed we're not guaranteed to have written nothing to the log (and usually we'd get as far as to write something).

Better yet would be to refactor this into a function, set that up and "test_when_finished" nuke it every time.

But I'm just going for the most minimal change here to not leave this landmine in place. My 51243f9f0f6 (run-command API: don't fall back on online_cpus(), 2022-10-12) already started using this pattern, and is on "master". I think a good incremental step is to do the bare minimum to do the same for the rest, and guard any subsequent test from being affected by the "trace.out".

So, unless you insist I'd rather just change nothing about this series, and not get stuck in various re-rolls/commentary about what's the ideal refactoring, once we're starting to do that anyway...

On Mon, Oct 31, 2022 at 01:50:07PM +0100, Ævar Arnfjörð Bjarmason wrote:

Ah, nicely explained. This is new to me, since I thought the behavior of both GIT_TRACE and GIT_TRACE2 was to clobber the file.

Yeah, both make sense, thanks.

Thanks, Taylor

Extend the tests added in c553c72eed6 (run-command: add an asynchronous parallel child processor, 2015-12-15) to test stdout in addition to stderr.

When the "ungroup" feature was added in fd3aaf53f71 (run-command: add an "ungroup" option to run_process_parallel(), 2022-06-07) its tests were made to test both the stdout and stderr, but these existing tests were left alone. Let's also exhaustively test our expected output here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0061-run-command.sh | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

On Sat, Oct 29, 2022 at 04:59:47AM +0200, Ævar Arnfjörð Bjarmason wrote:

Makes sense, this patch looks good. Thanks.

Thanks, Taylor