Re: [PATCH 07/15] commit-graph: new filter ver. that fixes murmur3

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Mon, Sep 25, 2023 at 07:03:18PM -0400, Taylor Blau wrote:
> On Fri, Sep 01, 2023 at 01:56:15PM -0700, Jonathan Tan wrote:
> > > > My original design (up to patch 7 in this patch set) defends against
> > > > this by taking the very first version detected and rejecting every
> > > > other version, and Taylor's subsequent design reads every version, but
> > > > annotates filters with its version. So I think we're covered.
> > >
> > > In the meantime I adapted the test cases from the above linked message
> > > to write commit-graph layers with different Bloom filter versions, and
> > > it does fail, because commits from the bottom commit-graph layer are
> > > omitted from the revision walk's output.  And the test case doesn't
> > > even need a middle layer without modified path Bloom filters to "hide"
> > > the different version in the bottom layer.  Merging the layers seems
> > > to work, though.
> >
> > For what it's worth, your test case below (with test_expect_success
> > instead of test_expect_failure) passes with my original design. With the
> > full patch set, it does fail, but for what it's worth, I did spot this,
> > providing an incomplete solution [1] and then a complete one [2]. Your
> > test case passes if I also include the following:
> >
> >   diff --git a/bloom.c b/bloom.c
> >   index ff131893cd..1bafd62a4e 100644
> >   --- a/bloom.c
> >   +++ b/bloom.c
> >   @@ -344,6 +344,10 @@ struct bloom_filter *get_bloom_filter(struct repository *r, struct commit *c)
> >
> >           prepare_repo_settings(r);
> >           hash_version = r->settings.commit_graph_changed_paths_version;
> >   +       if (hash_version == -1) {
> >   +               struct bloom_filter_settings *s = get_bloom_filter_settings(r);
> >   +               hash_version = (s && s->hash_version == 2) ? 2 : 1;
> >   +       }
> >
> >           if (!(hash_version == -1 || hash_version == filter->version))
> >                   return NULL; /* unusable filter */
> >
> > [1] https://lore.kernel.org/git/20230824222051.2320003-1-jonathantanmy@google.com/
> > [2] https://lore.kernel.org/git/20230829220432.558674-1-jonathantanmy@google.com/
> 
> Hmm. I am confused -- are you saying that this series breaks existing
> functionality, or merely does not patch an existing breakage? I *think*
> that it's the latter,

It's neither: the new functionality added in this series is broken.

> since this test case fails identically on master,
> but I am not sure.

Not sure what test you are referring to.  My test demonstrating the
breakage succeeds when adaped to master, because master doesn't
understand the commitgraph.changedPathsVersion=2 setting, and keeps
writing v1 Bloom filter chunks instead, so all commit-graphs layers
contain the same version.

> If my understanding is correct, I think that patching this would involve
> annotating each Bloom filter with a pointer to the bloom_filter_settings
> it was written with, and then using those settings when querying it.

In general we are better off when we don't write Bloom filter chunks
with different settings in the first place.