Re: [PATCH v3 13/14] commit-graph: rename 'split_commit_graph_opts'

[Taylor Blau <me@ttaylorr.com>]

On Wed, Aug 19, 2020 at 11:56:56AM +0200, SZEDER Gábor wrote:
> On Tue, Aug 11, 2020 at 04:52:11PM -0400, Taylor Blau wrote:
> > In the subsequent commit, additional options will be added to the
> > commit-graph API which have nothing to do with splitting.
> >
> > Rename the 'split_commit_graph_opts' structure to the more-generic
> > 'commit_graph_opts' to encompass both.
>
> Good.  Note, however, that write_commit_graph() has a 'flags'
> parameter as well, and when a feature is enabled via this 'flags',
> then first a corresponding 'ctx->foo' field is set, and that
> 'ctx->foo' is checked while computing and writing the commit-graph.
> With the generic options struct some other feature will be enabled via
> the 'opts->bar' field, so simply 'ctx->opts->bar' is checked while
> writing the commit-graph.
>
> With the generic options struct there really is no need for a separate
> flags parameter, the values in the flags can be stored in the options
> struct, and we can eliminate this inconsistency instead of adding even
> more.

I like the direction that you're headed in, but I'm not entirely sure
what you're suggesting. Do you want to make the
'enum commit_graph_write_flags flags' part of the new options struct?
Break out the fields into individual bits on that struct?

I'm not opposed to either, but note that there is also already a 'flags'
field on the options structure related to splitting, so that would have
to be untangled, too.

What I'm trying to say is that I think there's more complexity here than
you're giving it credit for. I'd rather press on with what we have here,
and devote adequate time to unraveling the complexity appropriately than
try to shove in another patch that takes a half-step in the right
direction.

>
> > diff --git a/commit-graph.h b/commit-graph.h
> > index ddbca1b59d..af08c4505d 100644
> > --- a/commit-graph.h
> > +++ b/commit-graph.h
> > @@ -109,7 +109,7 @@ enum commit_graph_split_flags {
> >  	COMMIT_GRAPH_SPLIT_REPLACE          = 2
> >  };
> >
> > -struct split_commit_graph_opts {
> > +struct commit_graph_opts {
> >  	int size_multiple;
> >  	int max_commits;
> >  	timestamp_t expire_time;
>         enum commit_graph_split_flags flags;
>
> While this was 'struct split_commit_graph_opts *split_opts' it was
> clear what kind of flags were in this 'flags' field.  Now that the
> struct is generic it's not clear anymore, so perhaps it should be
> renamed as well (e.g. 'split_flags'), or even turned into a couple of
> bit fields.

This I can definitely vouch for, so I'll 's/flags/split_flags' in the
next revision.

Thanks,
Taylor