Taylor Blau <me@xxxxxxxxxxxx> writes:
> 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
Ah, sorry for not being clear. What I meant is in how you compute
hash_version after we have found that filter is not NULL.
> +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);
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;
> + if (!(hash_version == -1 || hash_version == filter->version))
No need for the comparison to -1 here.
> + return NULL; /* unusable filter */
> + return filter;
> +}
This is fine.
