diff mbox series

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

Message ID 3e19d50148c8d53b30f8f0036a2d3af9f4bb3499.1580862307.git.me@ttaylorr.com (mailing list archive)
State New, archived
Headers show
Series builtin/commit-graph.c: new split/merge options | expand

Commit Message

Taylor Blau Feb. 5, 2020, 12:28 a.m. UTC
With '--split', the commit-graph machinery writes new commits in another
incremental commit-graph which is part of the existing chain, and
optionally decides to condense the chain into a single commit-graph.
This is done to ensure that the asymptotic behavior of looking up a
commit in an incremental chain is dominated by the number of
incrementals in that chain. It can be controlled by the '--max-commits'
and '--size-multiple' options.

On occasion, callers may want to ensure that 'git commit-graph write
--split' always writes an incremental, and never spends effort
condensing the incremental chain [1]. Previously, this was possible by
passing '--size-multiple=0', but this no longer the case following
63020f175f (commit-graph: prefer default size_mult when given zero,
2020-01-02).

Reintroduce a less-magical variant of the above with a new pair of
arguments to '--split': '--split=no-merge' and '--split=merge-all'. When
'--split=no-merge' is given, the commit-graph machinery will never
condense an existing chain and will always write a new incremental.
Conversely, if '--split=merge-all' is given, any invocation including it
will always condense a chain if one exists.  If '--split' is given with
no arguments, it behaves as before and defers to '--size-multiple', and
so on.

[1]: This might occur when, for example, a server administrator running
some program after each push may want to ensure that each job runs
proportional in time to the size of the push, and does not "jump" when
the commit-graph machinery decides to trigger a merge.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-commit-graph.txt | 16 +++++++++-----
 builtin/commit-graph.c             | 35 ++++++++++++++++++++++++++----
 commit-graph.c                     | 22 ++++++++++++-------
 commit-graph.h                     |  7 ++++++
 t/t5324-split-commit-graph.sh      | 25 +++++++++++++++++++++
 5 files changed, 88 insertions(+), 17 deletions(-)

Comments

Martin Ågren Feb. 6, 2020, 7:41 p.m. UTC | #1
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
Derrick Stolee Feb. 7, 2020, 3:48 p.m. UTC | #2
On 2/6/2020 2:41 PM, Martin Ågren wrote:
> On Wed, 5 Feb 2020 at 01:28, Taylor Blau <me@ttaylorr.com> wrote:
>>  * 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?

That certainly is unfortunate. Hopefully no one is taking a dependence on
this, which only means something if they had a `--split` previously in
the command-line arguments.

> 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 some ways, the split is now the default because that is how it is
written during 'git fetch' using fetch.writeCommitGraph. However, I
don't think that it will ever become the default for the commit-graph
builtin.

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

In some ways, the --split=merge-all is similar, except it writes a one-line
commit-graph-chain file and puts a .graph file in
.git/objects/info/commit-graphs instead of writing to .git/objects/commit-graph.

> To try to be concrete, here's a suggestion: `--format=split` and
> `--split-strategy=<strategy>`.

Why --format=split instead of leaving it as --[no-]split? Is there a reason to
introduce this string-based option when there are only two options right now?

Perhaps using --split-strategy=<strategy> is the most backwards-compatible
option, especially because we won't need --split="" to substitute for
"auto-merge". However, I wonder if this is a case where we should make the
hard choice to sacrifice a narrow backwards-compatibility in favor of a
simplified set of options?

Thanks,
-Stolee
Taylor Blau Feb. 9, 2020, 11:30 p.m. UTC | #3
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
Taylor Blau Feb. 9, 2020, 11:32 p.m. UTC | #4
On Fri, Feb 07, 2020 at 10:48:39AM -0500, Derrick Stolee wrote:
> On 2/6/2020 2:41 PM, Martin Ågren wrote:
> > On Wed, 5 Feb 2020 at 01:28, Taylor Blau <me@ttaylorr.com> wrote:
> >>  * 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?
>
> That certainly is unfortunate. Hopefully no one is taking a dependence on
> this, which only means something if they had a `--split` previously in
> the command-line arguments.
>
> > 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 some ways, the split is now the default because that is how it is
> written during 'git fetch' using fetch.writeCommitGraph. However, I
> don't think that it will ever become the default for the commit-graph
> builtin.
>
> > 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?
>
> In some ways, the --split=merge-all is similar, except it writes a one-line
> commit-graph-chain file and puts a .graph file in
> .git/objects/info/commit-graphs instead of writing to .git/objects/commit-graph.
>
> > To try to be concrete, here's a suggestion: `--format=split` and
> > `--split-strategy=<strategy>`.
>
> Why --format=split instead of leaving it as --[no-]split? Is there a reason to
> introduce this string-based option when there are only two options right now?
>
> Perhaps using --split-strategy=<strategy> is the most backwards-compatible
> option, especially because we won't need --split="" to substitute for
> "auto-merge". However, I wonder if this is a case where we should make the
> hard choice to sacrifice a narrow backwards-compatibility in favor of a
> simplified set of options?

My preference would be the latter, which I vaguely indicated in my last
email to Martin. Like I said, I think that the number of hypothetical
cases that we're breaking is pretty small, if not zero, and so I don't
feel too worried about changing the behavior like this.

If others feel strongly that keeping '--no-split' functional in the
classical sense is worthwhile, then I'm certainly happy to introduce
'--split-strategy' as another option, but I think that we agree that the
simplicity is worth the tradeoff here.

> Thanks,
> -Stolee

Thanks,
Taylor
Martin Ågren Feb. 12, 2020, 6:03 a.m. UTC | #5
On Fri, 7 Feb 2020 at 16:48, Derrick Stolee <stolee@gmail.com> wrote:
>
> On 2/6/2020 2:41 PM, Martin Ågren wrote:
> > On Wed, 5 Feb 2020 at 01:28, Taylor Blau <me@ttaylorr.com> wrote:
> >> -               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?
>
> That certainly is unfortunate. Hopefully no one is taking a dependence on
> this, which only means something if they had a `--split` previously in
> the command-line arguments.
>
> > 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 some ways, the split is now the default because that is how it is
> written during 'git fetch' using fetch.writeCommitGraph. However, I
> don't think that it will ever become the default for the commit-graph
> builtin.

Thanks for giving this piece of background.

> > To try to be concrete, here's a suggestion: `--format=split` and
> > `--split-strategy=<strategy>`.
>
> Why --format=split instead of leaving it as --[no-]split? Is there a reason to
> introduce this string-based option when there are only two options right now?

My thinking was, if my concern is "--split" being overloaded, what would
it look like to "unload" it entirely? From "--split" it isn't obvious
whether it's a verb or an adjective (shall we split, or shall we do
things the split way?). Having "--format=split" would help avoid *that*,
possibly leaving a cleaner field for the issue of "do we
allow/force/forbid the 'merging' to happen?". But I'm happy to accept
"--split=<strategy>" and move on. :-)

I see that Taylor juuust posted a v3. I'll try to find time to look it
over, but I won't be raising this point again.

Martin
Taylor Blau Feb. 12, 2020, 8:50 p.m. UTC | #6
On Wed, Feb 12, 2020 at 07:03:46AM +0100, Martin Ågren wrote:
> On Fri, 7 Feb 2020 at 16:48, Derrick Stolee <stolee@gmail.com> wrote:
> >
> > On 2/6/2020 2:41 PM, Martin Ågren wrote:
> > > On Wed, 5 Feb 2020 at 01:28, Taylor Blau <me@ttaylorr.com> wrote:
> > >> -               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?
> >
> > That certainly is unfortunate. Hopefully no one is taking a dependence on
> > this, which only means something if they had a `--split` previously in
> > the command-line arguments.
> >
> > > 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 some ways, the split is now the default because that is how it is
> > written during 'git fetch' using fetch.writeCommitGraph. However, I
> > don't think that it will ever become the default for the commit-graph
> > builtin.
>
> Thanks for giving this piece of background.
>
> > > To try to be concrete, here's a suggestion: `--format=split` and
> > > `--split-strategy=<strategy>`.
> >
> > Why --format=split instead of leaving it as --[no-]split? Is there a reason to
> > introduce this string-based option when there are only two options right now?
>
> My thinking was, if my concern is "--split" being overloaded, what would
> it look like to "unload" it entirely? From "--split" it isn't obvious
> whether it's a verb or an adjective (shall we split, or shall we do
> things the split way?). Having "--format=split" would help avoid *that*,
> possibly leaving a cleaner field for the issue of "do we
> allow/force/forbid the 'merging' to happen?". But I'm happy to accept
> "--split=<strategy>" and move on. :-)
>
> I see that Taylor juuust posted a v3. I'll try to find time to look it
> over, but I won't be raising this point again.

It looks like we raced :-). Sorry about that. I didn't see your email
until after I sent, and I certainly would have waited if I knew that you
were writing an email to the same thread as I was working in at the same
time.

I'm still fairly happy with the '--split[=<strategy>]' approach that is
implemented in all versions of this patch series, although I do
understand your suggestions.

My preference would be to see if anybody else feels like the trade-off
*is* worth it (I explained earlier in the thread some reasons why I feel
that the trade-off is *not* worth it), but I'd be happy to move this
series forward as-is unless others echo this idea.

> Martin

Thanks,
Taylor
diff mbox series

Patch

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:
+* 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.
 +
 * 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
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4a70b33fb5..4d3c1c46c2 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -9,7 +9,9 @@ 
 
 static char const * const builtin_commit_graph_usage[] = {
 	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
-	N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
+	N_("git commit-graph write [--object-dir <objdir>] [--append] "
+	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
+	   "[--[no-]progress] <split options>"),
 	NULL
 };
 
@@ -19,7 +21,9 @@  static const char * const builtin_commit_graph_verify_usage[] = {
 };
 
 static const char * const builtin_commit_graph_write_usage[] = {
-	N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
+	N_("git commit-graph write [--object-dir <objdir>] [--append] "
+	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
+	   "[--[no-]progress] <split options>"),
 	NULL
 };
 
@@ -111,6 +115,27 @@  static int graph_verify(int argc, const char **argv)
 extern int read_replace_refs;
 static struct split_commit_graph_opts split_opts;
 
+static int write_option_parse_split(const struct option *opt, const char *arg,
+				    int unset)
+{
+	enum commit_graph_split_flags *flags = opt->value;
+
+	opts.split = 1;
+	if (!arg) {
+		*flags = COMMIT_GRAPH_SPLIT_MERGE_AUTO;
+		return 0;
+	}
+
+	if (!strcmp(arg, "merge-all"))
+		*flags = COMMIT_GRAPH_SPLIT_MERGE_REQUIRED;
+	else if (!strcmp(arg, "no-merge"))
+		*flags = COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED;
+	else
+		die(_("unrecognized --split argument, %s"), arg);
+
+	return 0;
+}
+
 static int graph_write(int argc, const char **argv)
 {
 	struct string_list *pack_indexes = NULL;
@@ -133,8 +158,10 @@  static int graph_write(int argc, const char **argv)
 		OPT_BOOL(0, "append", &opts.append,
 			N_("include all commits already in the commit-graph file")),
 		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
-		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),
 		OPT_INTEGER(0, "max-commits", &split_opts.max_commits,
 			N_("maximum number of commits in a non-base split commit-graph")),
 		OPT_INTEGER(0, "size-multiple", &split_opts.size_multiple,
diff --git a/commit-graph.c b/commit-graph.c
index 656dd647d5..3a5cb23cd7 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1533,27 +1533,33 @@  static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
 
 	int max_commits = 0;
 	int size_mult = 2;
+	enum commit_graph_split_flags flags = COMMIT_GRAPH_SPLIT_MERGE_AUTO;
 
 	if (ctx->split_opts) {
 		max_commits = ctx->split_opts->max_commits;
 
 		if (ctx->split_opts->size_multiple)
 			size_mult = ctx->split_opts->size_multiple;
+
+		flags = ctx->split_opts->flags;
 	}
 
 	g = ctx->r->objects->commit_graph;
 	num_commits = ctx->commits.nr;
 	ctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1;
 
-	while (g && (g->num_commits <= size_mult * num_commits ||
-		    (max_commits && num_commits > max_commits))) {
-		if (g->odb != ctx->odb)
-			break;
+	if (flags != COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED) {
+		while (g && (g->num_commits <= size_mult * num_commits ||
+			    (max_commits && num_commits > max_commits) ||
+			    (flags == COMMIT_GRAPH_SPLIT_MERGE_REQUIRED))) {
+			if (g->odb != ctx->odb)
+				break;
 
-		num_commits += g->num_commits;
-		g = g->base_graph;
+			num_commits += g->num_commits;
+			g = g->base_graph;
 
-		ctx->num_commit_graphs_after--;
+			ctx->num_commit_graphs_after--;
+		}
 	}
 
 	ctx->new_base_graph = g;
@@ -1861,7 +1867,7 @@  int write_commit_graph(struct object_directory *odb,
 		goto cleanup;
 	}
 
-	if (!ctx->commits.nr)
+	if (!ctx->commits.nr && (!ctx->split_opts || ctx->split_opts->flags != COMMIT_GRAPH_SPLIT_MERGE_REQUIRED))
 		goto cleanup;
 
 	if (ctx->split) {
diff --git a/commit-graph.h b/commit-graph.h
index e87a6f6360..65a7d2edae 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -82,10 +82,17 @@  enum commit_graph_write_flags {
 	COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3)
 };
 
+enum commit_graph_split_flags {
+	COMMIT_GRAPH_SPLIT_MERGE_AUTO       = 0,
+	COMMIT_GRAPH_SPLIT_MERGE_REQUIRED   = 1,
+	COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED = 2
+};
+
 struct split_commit_graph_opts {
 	int size_multiple;
 	int max_commits;
 	timestamp_t expire_time;
+	enum commit_graph_split_flags flags;
 };
 
 /*
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index c24823431f..a165b48afe 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -344,4 +344,29 @@  test_expect_success 'split across alternate where alternate is not split' '
 	test_cmp commit-graph .git/objects/info/commit-graph
 '
 
+test_expect_success '--split=merge-all always merges incrementals' '
+	test_when_finished rm -rf a b c &&
+	rm -rf $graphdir $infodir/commit-graph &&
+	git reset --hard commits/10 &&
+	git rev-list -3 HEAD~4 >a &&
+	git rev-list -2 HEAD~2 >b &&
+	git rev-list -2 HEAD >c &&
+	git commit-graph write --split=no-merge --stdin-commits <a &&
+	git commit-graph write --split=no-merge --stdin-commits <b &&
+	test_line_count = 2 $graphdir/commit-graph-chain &&
+	git commit-graph write --split=merge-all --stdin-commits <c &&
+	test_line_count = 1 $graphdir/commit-graph-chain
+'
+
+test_expect_success '--split=no-merge always writes an incremental' '
+	test_when_finished rm -rf a b &&
+	rm -rf $graphdir &&
+	git reset --hard commits/2 &&
+	git rev-list HEAD~1 >a &&
+	git rev-list HEAD >b &&
+	git commit-graph write --split --stdin-commits <a &&
+	git commit-graph write --split=no-merge --stdin-commits <b &&
+	test_line_count = 2 $graphdir/commit-graph-chain
+'
+
 test_done