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

From: "Martin Ågren" <martin.agren@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: Git Mailing List <git@vger.kernel.org>, Jeff King <peff@peff.net>,
	Derrick Stolee <dstolee@microsoft.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 1/3] builtin/commit-graph.c: support '--split[=<strategy>]'
Date: Thu, 6 Feb 2020 20:41:28 +0100	[thread overview]
Message-ID: <CAN0heSrXZd7ktBTHaYFWjhW=NcGx5gL52-unSDaC4ZoNf96HFA@mail.gmail.com> (raw)
In-Reply-To: <3e19d50148c8d53b30f8f0036a2d3af9f4bb3499.1580862307.git.me@ttaylorr.com>

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.

> +* 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?

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