Subject: Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

From: Jonathan Tan <[email-masked]>
Date: Thu, 1 Feb 2018 15:33:49 -0800

On Tue, 30 Jan 2018 16:39:34 -0500
Derrick Stolee <[email-masked]> wrote:

> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index c8ea548dfb..3f3790d9a8 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -5,3 +5,21 @@ NAME
>  ----
>  git-commit-graph - Write and verify Git commit graphs (.graph files)
>  
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git commit-graph' --write <options> [--pack-dir <pack_dir>]

Subcommands (like those in git submodule) generally don't take "--", as
far as I know.

> +static int graph_write(void)
> +{
> +	struct object_id *graph_hash = construct_commit_graph(opts.pack_dir);

I should have noticed this when I was reviewing the previous patch, but
this is probably better named write_commit_graph.

> +test_expect_success 'create commits and repack' \
> +    'for i in $(test_seq 5)
> +     do
> +        echo $i >$i.txt &&
> +        git add $i.txt &&
> +        git commit -m "commit $i" &&

You can generate commits more easily with test_commit. Also, the final
character of the test_expect_success line should be the apostrophe that
starts the big text block, like in other files. (That also means that
the backslash is unnecessary.)

> +# Current graph structure:
> +#
> +#      M3
> +#     / |\_____
> +#    / 10      15
> +#   /   |      |
> +#  /    9 M2   14
> +# |     |/  \  |
> +# |     8 M1 | 13
> +# |     |/ | \_|
> +# 5     7  |   12
> +# |     |   \__|
> +# 4     6      11
> +# |____/______/
> +# 3
> +# |
> +# 2
> +# |
> +# 1

I don't think we need such a complicated graph structure - maybe it's
sufficient to have one 2-way merge and one 3-way merge. E.g.

6
|\-.
5 \ \
|\ \ \
1 2 3 4

Also, I wonder if it is possible to test the output to a greater extent.
We don't want anything that relies on the ordering of commits
(especially since we plan on transitioning away from using SHA-1 as the
hash function) but we could still test, for example, that a 3-way merge
results in an edge list of the form "EDGE_..._..." (where the first _
does not have its high bit set, but the second one does). This could be
done by using my hex_unpack() function as seen here [1] with grep (e.g.
"45444745[0-7].......[8-9a-f].......").

[1] https://public-inbox.org/git/b9ea9[email-masked]/