On Fri, Aug 14, 2020 at 04:20:21PM -0400, Taylor Blau wrote:
> > > @@ -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.
That would avoid the segfault, sure, but I would rather see all
callers of write_commit_graph{_reachable}() passing a valid opts
instance. Just like we don't call the diff machinery with a NULL
diff_options, or the revision walking machinery with a NULL rev_info.
> 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
v5? This is v3, and I'm unable to a find a v4.
> can avoid a v6.
>
> Thanks,
> Taylor
