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

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:

quoted

quoted

quoted

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/ (local)
[2] https://lore.kernel.org/git/20230829220432.558674-1-jonathantanmy@google.com/ (local)

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.