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

[Derrick Stolee <stolee@gmail.com>]

On 1/24/2019 4:40 AM, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Jan 23 2019, Derrick Stolee via GitGitGadget wrote:
>
>> 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.

Can do.

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

You're right that we should have an explicit "upgrade" test:

  1. Write a v1 commit-graph
  2. Add a commit
  3. Write a v2 commit-graph

As for a new GIT_TEST_ variable, we should only need to continue relying 
on GIT_TEST_COMMIT_GRAPH to test v2. I can add a 'graph_git_behavior' 
call on an explicitly v1 commit-graph file to get most of the coverage 
we need.

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

Will clean up.

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

When we add a new reachability index version, then the content of the 
data chunk will change. Since we have a separate byte for versioning 
that data, we do not need a v3 for the file format as a whole. A similar 
statement applies to the unused byte reserved for the incremental file 
format: we won't need to increase the file format version as we will 
make that number non-zero and add a chunk with extra data.

Thanks,
-Stolee