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

[Taylor Blau <me@ttaylorr.com>]

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:
> > On Tue, Aug 11, 2020 at 11:18:30PM +0200, SZEDER Gábor wrote:
> > > On Tue, Aug 11, 2020 at 04:51:19PM -0400, Taylor Blau wrote:
> > > > 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.

> > > > 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 <dstolee@microsoft.com>
> > > > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > Thanks,
> > Taylor
Thanks,
Taylor