On Tue, Oct 17, 2023 at 10:45:24AM +0200, Patrick Steinhardt wrote:
> > @@ -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;
> > +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
