Re: [PATCH 5/6] commit-graph: implement file format version 2

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 5/6] commit-graph: implement file format version 2
Date: Thu, 24 Jan 2019 10:40:09 +0100	[thread overview]
Message-ID: <878sza4cna.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <c55e0a738c79c30affcf2cb68564c01c36c66dff.1548280753.git.gitgitgadget@gmail.com>

On Wed, Jan 23 2019, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The commit-graph file format had some shortcomings which we now
> correct:
>
>   1. The hash algorithm was determined by a single byte, instead
>      of the 4-byte format identifier.
>
>   2. There was no way to update the reachability index we used.
>      We currently only support generation numbers, but that will
>      change in the future.
>
>   3. Git did not fail with error if the unused eighth byte was
>      non-zero, so we could not use that to indicate an incremental
>      file format without breaking compatibility across versions.
>
> The new format modifies the header of the commit-graph to solve
> these problems. We use the 4-byte hash format id, freeing up a byte
> in our 32-bit alignment to introduce a reachability index version.
> We can also fail to read the commit-graph if the eighth byte is
> non-zero.

I haven't tested, but it looks from the patch like we can transparently
read existing v1 data and then will write v2 the next time. Would be
helpful for reviewers if this was noted explicitly in the commit
message.

Should there be a GIT_TEST_COMMIT_GRAPH_VERSION=[12] going forward to
test the non-default version, or do you feel confident the tests added
here test the upgrade path & old code well enough?

> The 'git commit-graph read' subcommand needs updating to show the
> new data.

Let's say "The ... subcommand has been updated to show the new
data". This sounds like a later patch is going to do that, but in fact
it's done here.

> Set the default file format version to 2, and adjust the tests to
> expect the new 'git commit-graph read' output.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  .../technical/commit-graph-format.txt         | 26 +++++++++-
>  builtin/commit-graph.c                        |  9 ++++
>  commit-graph.c                                | 47 ++++++++++++++++---
>  commit-graph.h                                |  1 +
>  t/t5318-commit-graph.sh                       |  9 +++-
>  5 files changed, 83 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/technical/commit-graph-format.txt b/Documentation/technical/commit-graph-format.txt
> index 16452a0504..e367aa94b1 100644
> --- a/Documentation/technical/commit-graph-format.txt
> +++ b/Documentation/technical/commit-graph-format.txt
> @@ -31,13 +31,22 @@ and hash type.
>
>  All 4-byte numbers are in network order.
>
> +There are two versions available, 1 and 2. These currently differ only in
> +the header.

Shouldn't this be s/currently/ / ? Won't we add a version 3 if we make
new changes?