Hi Gábor, Thanks very much for your patience while I figure all of this out. I'm confident that with the fixup! below that your test passes in the next round of this series. On Sat, Jan 13, 2024 at 07:35:44PM +0100, SZEDER Gábor wrote: > On Wed, Jan 03, 2024 at 10:08:18AM -0800, Junio C Hamano wrote: > > Taylor Blau <me@xxxxxxxxxxxx> writes: > > > > >> * tb/path-filter-fix (2023-10-18) 17 commits > > >> - bloom: introduce `deinit_bloom_filters()` > > >> ... > > >> - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()` > > >> > > >> The Bloom filter used for path limited history traversal was broken > > >> on systems whose "char" is unsigned; update the implementation and > > >> bump the format version to 2. > > >> > > >> Expecting a reroll. > > >> cf. <20231023202212.GA5470@xxxxxxxxxx> > > >> source: <cover.1697653929.git.me@xxxxxxxxxxxx> > > > > > > I was confused by this one, since I couldn't figure out which tests > > > Gábor was referring to here. I responded in [1], but haven't heard back > > > since the end of October. > > > ... > > > [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/ > > I keep referring to the test in: > > https://public-inbox.org/git/20230830200218.GA5147@xxxxxxxxxx/ > > which, rather disappointingly, is still the only test out there > exercising the interaction between split commit graphs and different > modified path Bloom filter versions. Note that in that message I > mentioned that merging layers with differenet Bloom filter versions > seemed to work, but that, alas, is no longer the case, because it's > now broken in Taylor's recent iterations of the patch series. Thanks for clarifying. With all of the different versions of this series and the couple of tests that you and I were talking about, I think that I got confused and thought you were referring to a different test. Indeed, the current version of this series (v4) does not pass the test in <20230830200218.GA5147@xxxxxxxxxx>, but the fix in order for it to pass is relatively straightforward. I have this on top of my local branch as a fixup! and I'll squash it down on Tuesday[^1] and send a reroll after double-checking that the fix is correct and doesn't conflict with any other parts of the series. Since it's been so long since I've looked at this, I'll re-read the series as a whole with fresh eyes to make sure that everything still makes sense. In any case, here's the patch on top (with a lightly modified version of the test you wrote in <20230830200218.GA5147@xxxxxxxxxx>: Subject: [PATCH] fixup! commit-graph: ensure Bloom filters are read with consistent settings Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx> --- commit-graph.c | 3 ++- t/t4216-log-bloom.sh | 29 ++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 60fa64d956..82a4632c4e 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -507,7 +507,8 @@ static void validate_mixed_bloom_settings(struct commit_graph *g) } if (g->bloom_filter_settings->bits_per_entry != settings->bits_per_entry || - g->bloom_filter_settings->num_hashes != settings->num_hashes) { + g->bloom_filter_settings->num_hashes != settings->num_hashes || + g->bloom_filter_settings->hash_version != settings->hash_version) { g->chunk_bloom_indexes = NULL; g->chunk_bloom_data = NULL; FREE_AND_NULL(g->bloom_filter_settings); diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh index 569f2b6f8b..d8c42e2e27 100755 --- a/t/t4216-log-bloom.sh +++ b/t/t4216-log-bloom.sh @@ -438,7 +438,7 @@ test_expect_success 'setup for mixed Bloom setting tests' ' done ' -test_expect_success 'ensure incompatible Bloom filters are ignored' ' +test_expect_success 'ensure Bloom filters with incompatible settings are ignored' ' # Compute Bloom filters with "unusual" settings. git -C $repo rev-parse one >in && GIT_TEST_BLOOM_SETTINGS_NUM_HASHES=3 git -C $repo commit-graph write \ @@ -488,6 +488,33 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' ' test_must_be_empty err ' +test_expect_success 'ensure Bloom filter with incompatible versions are ignored' ' + rm "$repo/$graph" && + + git -C $repo log --oneline --no-decorate -- $CENT >expect && + + # Compute v1 Bloom filters for commits at the bottom. + git -C $repo rev-parse HEAD^ >in && + git -C $repo commit-graph write --stdin-commits --changed-paths \ + --split <in && + + # Compute v2 Bloomfilters for the rest of the commits at the top. + git -C $repo rev-parse HEAD >in && + git -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write \ + --stdin-commits --changed-paths --split=no-merge <in && + + test_line_count = 2 $repo/$chain && + + git -C $repo log --oneline --no-decorate -- $CENT >actual 2>err && + test_cmp expect actual && + + layer="$(head -n 1 $repo/$chain)" && + cat >expect.err <<-EOF && + warning: disabling Bloom filters for commit-graph layer $SQ$layer$SQ due to incompatible settings + EOF + test_cmp expect.err err +' + get_first_changed_path_filter () { test-tool read-graph bloom-filters >filters.dat && head -n 1 filters.dat -- 2.38.0.16.g393fd4c6db (Excuse the old version of Git here, I'm in the middle of a long-running process which checks out older versions of the tree to count the number of unused-parameters over time.) Thanks, Taylor [^1]: Monday is a US holiday, so I likely won't be at my computer.
