Re: [PATCH 2/2] bloom: enable multiple pathspec bloom keys

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

Lidong Yan <yldhome2d2@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> I do not know about icase; though.  Asking about "Dir/Path" and
>> getting "Dir/ or Dir/Path cannot possibly be in the set of paths
>> that were modified" from the changed-path Bloom filter would not
>> help us optimize the tree comparison out, when we do not want to
>> miss modifications for "dir/path".
>
> Make sense, both PATHSPEC_EXCLUDE and PATHSPEC_ICASE shouldn’t be
> optimized by bloom filter.

Before concluding so, we may want to double check how Bloom filters
are built on case insensitive systems, though.  If we normalize the
string by downcasing before murmuring the string, the resulting
Bloom filter may have more false positives for those who want to
(ab)use it to optimize case sensitive queries (without affecting
correctness), but case insensitive queries would be helped.  I do
not think we support (or want to support) a repository that spans
across two filesystems with different case sensitivity, so those who
worked on our changed-path Bloom filter subsystem may have already
placed such an optimization, based on the case sensitivity recorded
in the repository (core.ignorecase).

> I found that my [PATCH v3 2/2] contains two unaligned parameters. Should I reroll
> this patch and introduce the nowildcard_len change in a separate commit?

Updating a patch with a fix to obvious known problems is good.

Extending the scope of the series should be left out for a new
separate commit.  It may even be a better idea to hold it while
the current set of patches are still being polished, and then sent
out as a new series after the dust settles (even if you internally
developed that part as a direct extension to the current effort).

Thanks.