Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Feb 1, 2018, 23:33 UTC
- Message-ID
- <20180201153349.c94fe3de6b632e2fd8f843cf@google.com>
- In-Reply-To
- <1517348383-112294-6-git-send-email-dstolee@microsoft.com>
On Tue, 30 Jan 2018 16:39:34 -0500 Derrick Stolee <stolee@gmail.com> wrote:
Show 13 quoted lines
> 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.
Show 6 quoted lines
> +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.)
Show 19 quoted lines
> +# 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/b9ea93edabc42754dc3643d6307c22a947eabaf3.1506714999.git.jonathantanmy@google.com/