Re: [PATCH] t3070: make chain lint tester happy

From: Jeff King <peff@peff.net>
Date: Mar 25, 2023, 06:37 UTC
Message-ID: <20230325063731.GB562387@coredump.intra.peff.net>
In-Reply-To: <3714ba2f6528c38eb9c438126316a08b0cefca7c.1679696180.git.git@grubix.eu>

On Fri, Mar 24, 2023 at 11:17:11PM +0100, Michael J Gruber wrote:

Show 7 quoted lines

> 1f2e05f0b7 ("wildmatch: fix exponential behavior", 2023-03-20)
> introduced a new test with a background process. Backgrounding
> necessarily gives a result of 0, so that a seemingly broken && chain is
> not really broken.
> 
> Adjust t3070 slightly so that our chain list test recognizes the
> construct for what it is and does not raise a false positive.

Good catch. While I agree that there's no missed exit code here, I'd say that this is more than just a false positive. If there were any lines above the "&", like:

  foo &&
  bar &
  pid=$! &&
  ...etc...

then we'd be losing the exit value of "foo". It's OK here because the backgrounded command is the first line of the test, but it definitely violates our guidelines.

Which isn't to say that your patch needs to do anything differently. I just wondered if it meant we should be improving the chain linter, but I think it is doing the right thing to alert us here.

Show 20 quoted lines

> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> index b91a7cb712..4dd42df38c 100755
> --- a/t/t3070-wildmatch.sh
> +++ b/t/t3070-wildmatch.sh
> @@ -432,10 +432,12 @@ match 0 1 0 1 'z' '[Z-y]'
>  match 1 1 1 1 'Z' '[Z-y]'
>  
>  test_expect_success 'matching does not exhibit exponential behavior' '
> -	test-tool wildmatch wildmatch \
> -		aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab \
> -		"*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a" &
> -	pid=$! &&
> +	{
> +		test-tool wildmatch wildmatch \
> +			aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab \
> +			"*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a*a" &
> +		pid=$!
> +	} &&
>  	sleep 2 &&
>  	! kill $!

This looks like the right solution. I do wonder how Phillip managed to miss it, though, since the test script complains loudly.

-Peff