Re: [PATCH 1/2] p7821: fix test_perf invocation for prereqs

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Apr 13, 2025, 19:00 UTC
Message-ID: <90b3f122-8b94-3b45-08e0-32af95f9cea5@gmail.com>
In-Reply-To: <Z-pD1puYT87YKAd4@pks.im>

Hi Patrick,

Le 2025-03-31 à 03:27, Patrick Steinhardt a écrit :

Show 25 quoted lines

> On Fri, Mar 28, 2025 at 05:07:48PM +0000, Philippe Blain via GitGitGadget wrote:
>> diff --git a/t/perf/p7821-grep-engines-fixed.sh b/t/perf/p7821-grep-engines-fixed.sh
>> index 61e41b82cff..1d126c7b039 100755
>> --- a/t/perf/p7821-grep-engines-fixed.sh
>> +++ b/t/perf/p7821-grep-engines-fixed.sh
>> @@ -33,13 +33,13 @@ do
>>  		fi
>>  		if ! test_have_prereq PERF_GREP_ENGINES_THREADS
>>  		then
>> -			test_perf $prereq "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" "
>> +			test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern" --prereq "$prereq" "
>>  				git -c grep.patternType=$engine grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine' || :
>>  			"
>>  		else
>>  			for threads in $GIT_PERF_GREP_THREADS
>>  			do
>> -				test_perf PTHREADS,$prereq "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads" "
>> +				test_perf "$engine grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads" --prereq "PTHREADS,$prereq" "
>>  					git -c grep.patternType=$engine -c grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine.$threads' || :
>>  				"
>>  			done
> 
> "$prereq" can be empty here as it depends on which regexp engine we're
> using. The second case you adapt already looked weird before because we
> potentially checked for "PTHREADS,",

Indeed, the reason why the second case did not fail even when built with PCRE (which would not fail the first 'test_perf' since '$prereq' would be empty) is that this second test (in fact the whole loop) is only reached if GIT_PERF_GREP_THREADS is set in the environment, which sets the PERF_GREP_ENGINES_THREADS prereq. So just running 'make perf' or './p7821-*' would not enter this part of the test.

> but the first case was correct
> before but is now potentially checking for the empty prerequisite. Does
> that actually work as expected?

Yes, it was correct when built with PCRE, but not without, as then $prereq would not be empty. I did check that it works correctly before sending the patch, both when built with and without PCRE.

Thank you for the review and also checking that the patch works correctly. I just checked 'test_skip' which is the function that checks the prereq and indeed and empty 'test_prereq' is treated as no prereq.

Cheers,

Philippe.