On Mon, Feb 03, 2020 at 11:47:30AM +0100, SZEDER Gábor wrote:
> > -- snip --
> > diff --git a/commit-graph.c b/commit-graph.c
> > index a5d7624073f..af5c58833cf 100644
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@ -1566,7 +1566,8 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
> > num_commits = ctx->commits.nr;
> > ctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1;
> >
> > - if (ctx->split_opts->flags != COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED) {
> > + if (ctx->split_opts &&
> > + ctx->split_opts->flags != COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED) {
> > while (g && (g->num_commits <= size_mult * num_commits ||
> > (max_commits && num_commits > max_commits) ||
> > (ctx->split_opts->flags == COMMIT_GRAPH_SPLIT_MERGE_REQUIRED))) {
> > -- snap --
>
> Yeah, that's what I noted below, but I'm not sure that this is the
> right solution. Why doesn't cmd_fetch() call
> write_commit_graph_reachable() with a real 'split_opts' parameter in
> the first place? Wouldn't it be better if it did?
It used to provide a "blank" split_opts until 63020f175f (commit-graph:
prefer default size_mult when given zero, 2020-01-02), which caused a
bug. That bug was since fixed, but the idea was to keep things
convenient for callers.
Whether that's a good idea or not I guess is up for debate, but it
certainly is what the commit-graph code has tried to provide for some
time. If we're not going to follow that in this new code, then we should
presumably also rip out all of the other "if (ctx->split_opts)" lines.
-Peff
