Re: [PATCH v3 14/14] builtin/commit-graph.c: introduce '--max-new-filters=<n>'

["SZEDER Gábor" <szeder.dev@gmail.com>]

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