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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Feb 06, 2020 at 08:41:28PM +0100, Martin Ågren wrote:
> On Wed, 5 Feb 2020 at 01:28, Taylor Blau <me@ttaylorr.com> wrote:
> > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> > index 28d1fee505..b7fe65ef21 100644
> > --- a/Documentation/git-commit-graph.txt
> > +++ b/Documentation/git-commit-graph.txt
> > @@ -57,11 +57,17 @@ or `--stdin-packs`.)
> >  With the `--append` option, include all commits that are present in the
> >  existing commit-graph file.
> >  +
> > -With the `--split` option, write the commit-graph as a chain of multiple
> > -commit-graph files stored in `<dir>/info/commit-graphs`. The new commits
> > -not already in the commit-graph are added in a new "tip" file. This file
> > -is merged with the existing file if the following merge conditions are
> > -met:
> > +With the `--split[=<strategy>]` option, write the commit-graph as a
> > +chain of multiple commit-graph files stored in
> > +`<dir>/info/commit-graphs`. Commit-graph layers are merged based on the
> > +strategy and other splitting options. The new commits not already in the
> > +commit-graph are added in a new "tip" file. This file is merged with the
> > +existing file if the following merge conditions are met:
>
> Please add a lone "+" here.

Sure, thanks for noticing.

> > +* If `--split=merge-always` is specified, then a merge is always
> > +conducted, and the remaining options are ignored. Conversely, if
> > +`--split=no-merge` is specified, a merge is never performed, and the
> > +remaining options are ignored. A bare `--split` defers to the remaining
> > +options.
> >  +
>
> Similar to this existing one here. There's some minor misrendering here
> otherwise.
>
> >  * If `--size-multiple=<X>` is not specified, let `X` equal 2. If the new
> >  tip file would have `N` commits and the previous tip has `M` commits and
>
> > -               OPT_BOOL(0, "split", &opts.split,
> > -                       N_("allow writing an incremental commit-graph file")),
> > +               OPT_CALLBACK_F(0, "split", &split_opts.flags, NULL,
> > +                       N_("allow writing an incremental commit-graph file"),
> > +                       PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
> > +                       write_option_parse_split),
>
>
> I keep getting back to this -- sorry! So this actually forbids
> "--no-split", which used to work before. Unfortunate?

Ah, I see. Yes, this definitely *does* forbid that. My thinking when I
decided to give this 'PARSE_OPT_NONEG' was that '--no-split' is
confusing for users: does it mean "don't split" or "unset any split
options"?

This probably would be ameliorated by your suggestion below, maybe of
'--split-strategy', specifically (I could probably go either way on
'--format=split', but it really depends on what Stolee has planned
long-term). Then, '--[no-]split' remains clear, as does
'--no-split-strategy' (although I suppose that you could make the
argument that '--no-split-strategy' sounds a little bit like letting the
machinery use its defaults, which may or may not be true depending on
how it's implemented.)

But, I'm not sure that it's all worth it to add another option here.
This sub-builtin has a plethora of options already, and I'm skeptical
that there are a lot of real-world uses of '--no-split' in the wild that
we'd be breaking.

> I have to ask, what is the long-term plan for the two formats (split and
> non-split)? As I understand it, and I might well be wrong, the non-split
> format came first and the split format was a user-experience
> improvement. Should we expect that `--split` becomes the default? In
> which case `--no-split` would be needed. Or might the non-split format
> go away entirely, leaving `--split` a no-op and `--split=<strategy>` a
> pretty funky way of choosing a strategy for the one-and-only file
> format?
>
> To try to be concrete, here's a suggestion: `--format=split` and
> `--split-strategy=<strategy>`.
>
> Martin

Thanks,
Taylor