Re: [PATCH v3 01/14] commit-graph: introduce 'get_bloom_filter_settings()'

On Tue, Aug 11, 2020 at 11:27:16PM +0200, SZEDER Gábor wrote:

On Tue, Aug 11, 2020 at 05:21:18PM -0400, Taylor Blau wrote:

quoted

On Tue, Aug 11, 2020 at 11:18:30PM +0200, SZEDER Gábor wrote:

quoted

On Tue, Aug 11, 2020 at 04:51:19PM -0400, Taylor Blau wrote:

quoted

Many places in the code often need a pointer to the commit-graph's
'struct bloom_filter_settings', in which case they often take the value
from the top-most commit-graph.

In the non-split case, this works as expected. In the split case,
however, things get a little tricky. Not all layers in a chain of
incremental commit-graphs are required to themselves have Bloom data,
and so whether or not some part of the code uses Bloom filters depends
entirely on whether or not the top-most level of the commit-graph chain
has Bloom filters.

This has been the behavior since Bloom filters were introduced, and has
been codified into the tests since a759bfa9ee (t4216: add end to end
tests for git log with Bloom filters, 2020-04-06). In fact, t4216.130
requires that Bloom filters are not used in exactly the case described
earlier.

There is no reason that this needs to be the case, since it is perfectly
valid for commits in an earlier layer to have Bloom filters when commits
in a newer layer do not.

Since Bloom settings are guaranteed to be the same for any layer in a
chain that has Bloom data,

Is it?  Where is that guaranteed?

There is no mechanism whatsoever to customize these settings that is
exposed to the user (except for the undocumented 'GIT_TEST' environment
variables).

Let me rephrase it, then: where is it written in the commit-graph
format specification that these must be the same in all layers?

Nowhere.

OK. We can certainly document that this is the case. For this purpose,
all we really care about is that the graph _has_ Bloom filters anywhere.
If you wanted to return the exact matching settings, you could also
provide a commit and return the settings belonging to the graph that
contains that commit.

In the case where we don't have a commit, we could use the default
settings instead.

I think that we are a little bit dealing with a problem that doesn't
exist, since we do not document the sole method by which you would
change these settings. So, maybe we can think more about this, but my
preference would be to leave this patch alone.

Maybe Stolee can chime in, too.

quoted

quoted

quoted

it is sufficient to traverse the
'->base_graph' pointer until either (1) a non-null 'struct
bloom_filter_settings *' is found, or (2) until we are at the root of
the commit-graph chain.

Introduce a 'get_bloom_filter_settings()' function that does just this,
and use it instead of purely dereferencing the top-most graph's
'->bloom_filter_settings' pointer.

While we're at it, add an additional test in t5324 to guard against code
in the commit-graph writing machinery that doesn't correctly handle a
NULL 'struct bloom_filter *'.

Co-authored-by: Derrick Stolee [off-list ref]
Signed-off-by: Taylor Blau <redacted>

Thanks,
Taylor

Thanks,
Taylor