Re: [PATCH 5/6] commit-graph: implement file format version 2
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jan 24, 2019, 14:34 UTC
- Message-ID
- <cd07c73f-e839-844c-9652-1bdf95724f5a@gmail.com>
- In-Reply-To
- <878sza4cna.fsf@evledraar.gmail.com>
On 1/24/2019 4:40 AM, Ævar Arnfjörð Bjarmason wrote:
Show 11 quoted lines
> 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.
Show 5 quoted lines
>> 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.
Show 24 quoted lines
>> 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