Re: [RFC PATCH 2/6] bloom: prepare to discard incompatible Bloom filters

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 25, 2023, 19:00 UTC
Message-ID: <ZOj6PazHzDeQrY88@nand.local>
In-Reply-To: <20230824230527.2332958-1-jonathantanmy@google.com>

On Thu, Aug 24, 2023 at 04:05:27PM -0700, Jonathan Tan wrote:

Show 9 quoted lines

> Up to here is fine.
>
> > +	hash_version = r->settings.commit_graph_changed_paths_version;
>
> Instead of doing this, do this (well, move the struct declaration to
> the top):
>
>   struct bloom_filter_settings *s = get_bloom_filter_settings(r);
>   hash_version = s->hash_version == 2 ? 2 : 1;

Do we need this normalization? We assign s->hash_version in commit-graph.c::graph_read_bloom_data() by reading it from the start of the BDAT chunk, so this should only ever be 1 or 2.

> > +	if (!(hash_version == -1 || hash_version == filter->version))
>
> No need for the comparison to -1 here.

I'm not sure I understand your suggestion. When we fetch the filter from get_or_compute_bloom_filter(), we have filter->version set to the hash_version from the containing graph's Bloom filter settings.

So (besides the normalization), I would think that:

    struct bloom_filter_settings *s = get_bloom_filter_settings(r);
    struct bloom_filter *f = get_bloom_filter(...);

    assert(s->hash_version == f->version);

would hold.

I think the check that we want to make is instead: is this Bloom filter's version (or equivalently, the hash version indicated by that graph's BDAT chunk) something that we can read? And I think "what we can read" here is dictated by the commit_graph_changed_paths_version member of our repository_settings, no?

Thanks, Taylor