On Tue, Aug 18, 2020 at 12:50:04AM +0200, SZEDER Gábor wrote:
> 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.
I wouldn't mind that either, but this is definitely a common pattern
throughout the commit-graph machinery. So, if/when we do get away from
it, I'd rather do so uniformly than in some spots.
> > 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.
Sorry, I clearly had too much on my mind when I was writing this ;). I'm
hopeful that with your careful review that v4 will be the last of this
topic.
Thanks,
Taylor
