From: Derrick Stolee Date: Thu, 24 Jan 2019 14:34:40 GMT Subject: Re: [PATCH 5/6] commit-graph: implement file format version 2 Message-ID: In-Reply-To: <878sza4cna.fsf@evledraar.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 >> --- >> .../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