Re: [PATCH 1/3] builtin/commit-graph.c: support '--split[=<strategy>]'

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 4, 2020, 03:58 UTC
Message-ID: <20200204035821.GB5790@syl.local>
In-Reply-To: <20200203111117.GC4153124@coredump.intra.peff.net>

On Mon, Feb 03, 2020 at 06:11:17AM -0500, Jeff King wrote:

Show 33 quoted lines

> 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.

I think that this seems like a good step that we should probably take, but I don't think that it's necessary for the series at hand. The pattern in this function is to define a local variable which has the same value as in split_opts, or a reasonable default if split_opts is NULL (c.f., 'max_commits' and 'size_mult').

So, I think that a safe thing to do which prevents the segv and doesn't change the pattern too much is to write:

  enum commit_graph_split_flags flags = COMMIT_GRAPH_SPLIT_MERGE_AUTO;
  if (ctx->split_opts) {
    /* ... */
    flags = ctx->split_opts->flags;
  }

  /* ... */

  if (flags != COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED) {
    while ( ... ) { ... }
  }

This is adding another local variable, which seems like an odd thing to do *every* time that we add another member to split_opts. So for that reason it seems like in the longer-term we should either force the caller to pass in a blank, or do something else that doesn't require this, but I think that the intermediate cost isn't too bad.

> -Peff

Thanks, Taylor