From: "SZEDER Gábor" <szeder.dev@gmail•com>
To: Taylor Blau <me@ttaylorr•com>
Cc: Junio C Hamano <gitster@pobox•com>, git@vger•kernel.org
Subject: Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
Date: Sun, 14 Jan 2024 00:41:34 +0100 [thread overview]
Message-ID: <20240113234134.GE3000857@szeder.dev> (raw)
In-Reply-To: <ZaMJU6MJ5wZxyLeM@nand.local>
On Sat, Jan 13, 2024 at 05:06:11PM -0500, Taylor Blau wrote:
> 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@ttaylorr•com> 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@szeder•dev>
> > > >> source: <cover.1697653929.git.me@ttaylorr•com>
> > > >
> > > > 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@szeder.dev/
> >
> > 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@szeder•dev>, 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@szeder•dev>:
I certainly hope that I'm just misunderstanding, and you don't
actually imply that this one test in its current form would qualify as
thorough testing... :)
> Subject: [PATCH] fixup! commit-graph: ensure Bloom filters are read with
> consistent settings
>
> Signed-off-by: Taylor Blau <me@ttaylorr•com>
> ---
> 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.
next prev parent reply other threads:[~2024-01-13 23:41 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-03 1:02 What's cooking in git.git (Jan 2024, #01; Tue, 2) Junio C Hamano
2024-01-03 5:53 ` ps/refstorage-extension (was: What's cooking in git.git (Jan 2024, #01; Tue, 2)) Patrick Steinhardt
2024-01-03 9:01 ` What's cooking in git.git (Jan 2024, #01; Tue, 2) Jeff King
2024-01-03 16:37 ` Junio C Hamano
2024-01-05 8:59 ` Jeff King
2024-01-05 16:34 ` Junio C Hamano
2024-01-03 17:14 ` René Scharfe
2024-01-03 16:43 ` Taylor Blau
2024-01-03 18:08 ` Junio C Hamano
2024-01-13 18:35 ` SZEDER Gábor
2024-01-13 22:06 ` Taylor Blau
2024-01-13 23:41 ` SZEDER Gábor [this message]
2024-01-16 20:37 ` Taylor Blau
2024-02-25 22:59 ` SZEDER Gábor
2024-02-26 14:44 ` Taylor Blau
2024-01-13 22:51 ` SZEDER Gábor
2024-01-16 20:49 ` Taylor Blau
2024-01-16 22:45 ` Junio C Hamano
2024-01-16 23:31 ` Taylor Blau
2024-01-16 23:42 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240113234134.GE3000857@szeder.dev \
--to=szeder.dev@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox