On Wed, Aug 12, 2020 at 01:49:29PM +0200, SZEDER Gábor wrote:
> On Tue, Aug 11, 2020 at 04:52:14PM -0400, Taylor Blau wrote:
>
> > diff --git a/commit-graph.c b/commit-graph.c
> > index 6886f319a5..4aae5471e3 100644
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@ -954,7 +954,8 @@ struct tree *get_commit_tree_in_graph(struct repository *r, const struct commit
> > }
> >
> > static int get_bloom_filter_large_in_graph(struct commit_graph *g,
> > - const struct commit *c)
> > + const struct commit *c,
> > + uint32_t max_changed_paths)
> > {
> > uint32_t graph_pos = commit_graph_position(c);
> > if (graph_pos == COMMIT_NOT_FROM_GRAPH)
> > @@ -965,6 +966,17 @@ static int get_bloom_filter_large_in_graph(struct commit_graph *g,
> >
> > if (!(g && g->bloom_large))
> > return 0;
> > + if (g->bloom_filter_settings->max_changed_paths != max_changed_paths) {
> > + /*
> > + * Force all commits which are subject to a different
> > + * 'max_changed_paths' limit to be recomputed from scratch.
> > + *
> > + * Note that this could likely be improved, but is ignored since
> > + * all real-world graphs set the maximum number of changed paths
> > + * at 512.
> > + */
> > + return 0;
> > + }
> > return bitmap_get(g->bloom_large, graph_pos - g->num_commits_in_base);
> > }
> >
> > @@ -1470,6 +1482,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
> > int i;
> > struct progress *progress = NULL;
> > int *sorted_commits;
> > + int max_new_filters;
> >
> > init_bloom_filters();
> > ctx->bloom_large = bitmap_word_alloc(ctx->commits.nr / BITS_IN_EWORD + 1);
> > @@ -1486,10 +1499,15 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
> > ctx->order_by_pack ? commit_pos_cmp : commit_gen_cmp,
> > &ctx->commits);
> >
> > + max_new_filters = ctx->opts->max_new_filters >= 0 ?
> > + ctx->opts->max_new_filters : ctx->commits.nr;
>
> git_test_write_commit_graph_or_die() invokes
> write_commit_graph_reachable() with opts=0x0, so 'ctx->opts' is NULL,
> and we get segfault. This breaks a lot of tests when run with
> GIT_TEST_COMMIT_GRAPH=1 GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1.
Great catch, thanks. Fixing this is as simple as adding 'ctx->opts &&'
right before we dereference 'ctx->opts', since setting this variable
equal to 'ctx->commits.nr' is the right thing to do in that case.
Unrelated to this comment, I am hoping to send out a final version of
this series sometime next week so that we can keep moving forward with
Bloom filter improvements.
Have you had a chance to review the rest of the patches? I'll happily
wait until you have had a chance to do so before sending v5 so that we
can avoid a v6.
Thanks,
Taylor
