Re: [PATCH 1/2] p7821: fix test_perf invocation for prereqs
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.
Philippe Blain via GitGitGadget
