Subject: Re: [PATCH v3 08/13] bloom: use provided 'struct bloom_filter_settings'

From: SZEDER Gábor <[email-masked]>
Date: Fri, 18 Sep 2020 18:27:40 +0200

On Thu, Sep 17, 2020 at 10:59:27PM -0400, Taylor Blau wrote:
> When 'get_or_compute_bloom_filter()' needs to compute a Bloom filter
> from scratch, it looks to the default 'struct bloom_filter_settings' in
> order to determine the maximum number of changed paths, number of bits
> per entry, and so on.
> 
> All of these values have so far been constant, and so there was no need
> to pass in a pointer from the caller (eg., the one that is stored in the
> 'struct write_commit_graph_context').
> 
> Start passing in a 'struct bloom_filter_settings *' instead of using the
> default values to respect graph-specific settings (eg., in the case of
> setting 'GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS').

I think this description misses the most important aspect of this
patch: it fixes, even if only partially, the half-broken fix in
0087a87ba8 (commit-graph: persist existence of changed-paths,
2020-07-01).

That commit, among other things, tried to make sure that we use the
same Bloom filter settings to compute new Bloom filters that have been
used in already existing filters.  However, it only read those
settings from the header of the existing BDAT chunk and wrote them to
the header of the new BDAT chunk (and printed them to trace2 output).
Unfortunately, it didn't actually use those settings to compute Bloom
filters, because it left get_bloom_filter() unchanged to use the
hardcoded default Bloom filter settings.  This can result in bogus
commits-graphs and, in turn, pathspec-limited revision walks omitting
commits that do modify the specified path.