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

From: Martin Ågren <martin.agren@gmail.com>
Date: Jan 31, 2020, 19:27 UTC
Message-ID: <CAN0heSo29+sf1352EfGv=qFsir-e=Ompja445bh5z3vpOfkaJA@mail.gmail.com>
In-Reply-To: <4f5bc19903f8a1f5b153b5665de378e743e12744.1580430057.git.me@ttaylorr.com>

On Fri, 31 Jan 2020 at 01:29, Taylor Blau <me@ttaylorr.com> wrote:

> 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 aysmptotic behavior of looking up a

asymptotic

Show 19 quoted lines

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

I understand your motivation for doing this -- it all seems quite sound to me. Not being too familiar with this commit-graph splitting and merging, I had a hard time groking this terminology though. From what I understand, before this patch, `--split` means "write the commit-graph using the 'split' file-format / in a split fashion". Ok, that makes sense.

From seeing `--split=no-merge`, I have no idea how to even parse that. Of course I don't want to merge, I want to split! Well not split, but write split files.

I think it would help me (and others like me) if we could somehow separate "I want to use 'split' files" from "and here's how I want you to decide on the merging". That is, which "strategy" to use. Obviously, talking about a "merge strategy" would be stupid and "split strategy" also seems a bit odd. "Coalescing strategy"? "Joining strategy"?

Or can you convince me otherwise? From which angle should I look at this?

Show 18 quoted lines

> -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. (Note that merging a chain of commit graphs replaces the
> +existing chain with a length-1 chain where the first and only
> +incremental holds the entire graph).

To better understand the background for this patch, I read the manpage as it stands today. From the section on `--split`, I got this impression: Let's say that `--max-commits` is huge, so all that matters is the `--size-multiple`. Let's say it's two. If the current tip contains three commits and we're about to write one with two, then 2*2 > 3 so we will merge, i.e., write a tip file with five commits. Unless of course *that* is more than half the size of the file before. And so on. We might just merge once, or maybe "many" files in an avalanche effect. Every now and then, such avalanches will go all the way back to the first file.

Now this says something different, namely that once we decide to merge, we do it all the way back, no matter what.

The commit message of 1771be90c8 ("commit-graph: merge commit-graph chains", 2019-06-18) seems to support my original understanding, at least for `--size-multiple`, but not `--max-commits`, curiously enough.

Can you clarify?

> -               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"),

This still sounds very boolean. Cramming in the "strategy" might be hard -- is this an argument in favor of having two separate options? ;-)

Show 5 quoted lines

> +enum commit_graph_split_flags {
> +       COMMIT_GRAPH_SPLIT_UNSPECIFIED      = 0,
> +       COMMIT_GRAPH_SPLIT_MERGE_REQUIRED   = 1,
> +       COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED = 2
> +};

I wonder if this should be "MERGE_AUTO" rather than "UNSPECIFIED". This is related to Stolee's comment, I think.

Martin