Re: [PATCH v3 6/6] last-modified: keep per-path Bloom filters for wildcard pathspecs

[Junio C Hamano <gitster@pobox.com>]

Toon Claes <toon@iotcl.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Toon Claes <toon@iotcl.com> writes:
>>
>>> diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
>>> index df73c7d0d0..75b18ee83b 100755
>>> --- a/t/t8020-last-modified.sh
>>> +++ b/t/t8020-last-modified.sh
>>> @@ -290,6 +290,32 @@ test_expect_success 'last-modified with Bloom filters and --show-trees' '
>>>  	)
>>>  '
>>>  
>>> +test_expect_success 'last-modified with Bloom filters and top-level wildcard' '
>>> ...
>>> +		GIT_TRACE2_PERF="$(pwd)/off.perf" \
>>> +			git -c core.commitGraph=false last-modified -r HEAD \
>>> +			-- "*.c" >expect &&
>>> +		test_grep "data .* bloom_queries:0$" off.perf &&
>>
>> Ah, OK.  With GIT_TEST_COMMIT_GRAPH=1 exported from the test harness
>> environment, 'git -c core.commitGraph=false' would not be effective
>> here.  You would need to do something like:
>>
>> 		GIT_TEST_COMMIT_GRAPH=0 GIT_TRACE2_PERF="$(pwd)/off.perf" \
>> 			git -c core.commitGraph=false last-modified -r HEAD \
>> 			-- "*.c" >expect &&
>>
>>> +		GIT_TRACE2_PERF="$(pwd)/on.perf" \
>>> +			git -c core.commitGraph=true last-modified -r HEAD \
>>> +			-- "*.c" >actual &&
>>
>> And in the case where GIT_TEST_COMMIT_GRAPH=0 comes from the
>> environment, you might want to be explicit about setting the
>> variable here as well.
>
> Thanks for this suggestion. Yeah, I didn't notice it makes CI fail.
>
> But a little bit of a followup question, I noticed I also should be
> setting these in [PATCH 5/6], but test don't fail if not set
> appropriately.

Yeah, I noticed it when I queued the two fixup commits near the tip
of 'seen'.  I wrote it off as the test *not* checking everything.
If the test is about what the command does and not about how the
command exactly does its thing, you may not notice the difference
as long as two code paths both produce the right results.

So some tightening of tests might be needed, if we care.