Re: [PATCH v3 10/17] commit-graph: new filter ver. that fixes murmur3

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 18, 2023, 17:46 UTC
Message-ID: <ZTAaEffz2MkkTjzD@nand.local>
In-Reply-To: <ZS5JpJCw-MY256xo@tanuki>

On Tue, Oct 17, 2023 at 10:45:24AM +0200, Patrick Steinhardt wrote:

Show 30 quoted lines

> > @@ -314,17 +314,26 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
> >  	return 0;
> >  }
> >
> > +struct graph_read_bloom_data_context {
> > +	struct commit_graph *g;
> > +	int *commit_graph_changed_paths_version;
> > +};
> > +
> >  static int graph_read_bloom_data(const unsigned char *chunk_start,
> >  				  size_t chunk_size, void *data)
> >  {
> > -	struct commit_graph *g = data;
> > +	struct graph_read_bloom_data_context *c = data;
> > +	struct commit_graph *g = c->g;
> >  	uint32_t hash_version;
> > -	g->chunk_bloom_data = chunk_start;
> >  	hash_version = get_be32(chunk_start);
> >
> > -	if (hash_version != 1)
> > +	if (*c->commit_graph_changed_paths_version == -1) {
> > +		*c->commit_graph_changed_paths_version = hash_version;
> > +	} else if (hash_version != *c->commit_graph_changed_paths_version) {
> >  		return 0;
> > +	}
>
> In case we have `c->commit_graph_changed_paths_version == -1` we lose
> the check that the hash version is something that we know and support,
> don't we? And while we do start to handle `-1` in the writing path, I
> think we don't in the reading path unless I missed something.

We don't have to deal with c->commit_graph_changed_paths_version being -1 here, since we normalize it when reading the BDAT chunk. See commit-graph.c::graph_read_bloom_data(), particularly:

    if (*c->commit_graph_changed_paths_version == -1)
        *c->commit_graph_changed_paths_version = hash_version;
    else if (hash_version != *c->commit_graph_changed_paths_version)
        return 0;

Show 18 quoted lines

> > +test_expect_success 'when writing commit graph, do not reuse changed-path of another version' '
> > +	git init doublewrite &&
> > +	test_commit -C doublewrite c "$CENT" &&
> > +	git -C doublewrite config --add commitgraph.changedPathsVersion 1 &&
> > +	git -C doublewrite commit-graph write --reachable --changed-paths &&
> > +	git -C doublewrite config --add commitgraph.changedPathsVersion 2 &&
> > +	git -C doublewrite commit-graph write --reachable --changed-paths &&
> > +	(
> > +		cd doublewrite &&
> > +		echo "c01f" >expect &&
> > +		get_first_changed_path_filter >actual &&
> > +		test_cmp expect actual
> > +	)
> > +'
> > +
>
> With the supposedly missing check in mind, should we also add tests for
> currently unknown versions like 3 or -2?

Good idea, I'll update the test to reflect.

Thanks, Taylor