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

On Thu, Aug 24, 2023 at 03:20:51PM -0700, Jonathan Tan wrote:

Taylor Blau [off-list ref] writes:

quoted

diff --git a/bloom.c b/bloom.c
index 9b6a30f6f6..739fa093ba 100644
--- a/bloom.c
+++ b/bloom.c

@@ -250,6 +250,23 @@ static void init_truncated_large_filter(struct bloom_filter *filter,
 	filter->version = version;
 }

+struct bloom_filter *get_bloom_filter(struct repository *r, struct commit *c)
+{
+	struct bloom_filter *filter;
+	int hash_version;
+
+	filter = get_or_compute_bloom_filter(r, c, 0, NULL, NULL);
+	if (!filter)
+		return NULL;
+
+	prepare_repo_settings(r);
+	hash_version = r->settings.commit_graph_changed_paths_version;
+
+	if (!(hash_version == -1 || hash_version == filter->version))
+		return NULL; /* unusable filter */
+	return filter;
+}

I missed this the last time, but this should match what fill_bloom_key()
does. Use get_bloom_filter_settings(),

I'm not sure I'm following. Are you saying that we should use
get_bloom_filter_settings() instead of reading the value from
r->settings directly?

then compare filter->version to version 2 if hash_version is 2, and to
version 1 otherwise.

Hmm. I think we're already doing the right thing here, no? IIUC, you're
saying to do something like:

    struct bloom_filter_settings *s = get_bloom_filter_settings(r);
    struct bloom_filter *f = get_or_compute_bloom_filter(r, c, ...);
    int hash_version;

    if (!f)
      return NULL;

    prepare_repo_settings(r);
    hash_version = r->settings.commit_graph_changed_paths_version;

    if (!(hash_version == -1 || hash_version == s->hash_version))
      return NULL; /* incompatible */
    return f;

?

If so, I think that we're already OK here, since s->hash_version is
always going to take the same value as f->version, since f->version is
assigned in bloom.c::load_bloom_filter_from_graph(), which does:

    filter->version = g->bloom_filter_settings->hash_version;

Or are we talking about two different things entirely? ;-)

Thanks,
Taylor