Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Sep 24, 2023, 19:59 UTC
Message-ID: <20230924195900.GA1156862@szeder.dev>
In-Reply-To: <ZQnpIBR4hEbOLCwP@nand.local>

On Tue, Sep 19, 2023 at 02:32:00PM -0400, Taylor Blau wrote:

Show 29 quoted lines

> > * tb/path-filter-fix (2023-08-30) 15 commits
> >  - bloom: introduce `deinit_bloom_filters()`
> >  - commit-graph: reuse existing Bloom filters where possible
> >  - object.h: fix mis-aligned flag bits table
> >  - commit-graph: drop unnecessary `graph_read_bloom_data_context`
> >  - commit-graph.c: unconditionally load Bloom filters
> >  - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
> >  - bloom: prepare to discard incompatible Bloom filters
> >  - bloom: annotate filters with hash version
> >  - commit-graph: new filter ver. that fixes murmur3
> >  - repo-settings: introduce commitgraph.changedPathsVersion
> >  - t4216: test changed path filters with high bit paths
> >  - t/helper/test-read-graph: implement `bloom-filters` mode
> >  - bloom.h: make `load_bloom_filter_from_graph()` public
> >  - t/helper/test-read-graph.c: extract `dump_graph_info()`
> >  - gitformat-commit-graph: describe version 2 of BDAT
> >
> >  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.
> >
> >  Needs more work.
> >  cf. <20230830200218.GA5147@szeder.dev>
> >  source: <cover.1693413637.git.jonathantanmy@google.com>
> 
> I think that Jonathan's most recent round of this is ready to get merged
> up, cf. [3]. The outstanding issue you note in
> <20230830200218.GA5147@szeder.dev> can be addressed separately, I
> believe. To that end, I have a RFC-level patch proposed here [4].

Besides the issue of not reading Bloom filter for root commits, that message of mine also includes a test demonstrating that handling split commit graph with layers containing different versions of Bloom filters is broken. That test still fails with the current version of this patch series, i.e. what is currently in seen. Jonathan provided a patch that makes that test pass, and also noted that that test did pass with his original design:

  https://public-inbox.org/git/20230901205616.3572722-1-jonathantanmy@google.com/

I maintain that without test cases thoroughly covering the interaction of different Bloom filter versions with split commit graphs this series should not be merged.

Show 5 quoted lines

> [3]: https://lore.kernel.org/git/xmqqo7io8gmo.fsf@gitster.g/
> [4]: https://lore.kernel.org/git/ZQnmTXUO94%2FQy8mq@nand.local/
> 
> Thanks,
> Taylor