Thanks for the feedback on v3. There were several small cleanups, but
perhaps the biggest change is the addition of "commit-graph: use
string-list API for input" which makes "commit-graph: add '--reachable'
option" much simpler.
The inter-diff is still reasonably large, but I'll send it in a
follow-up PR.
Thanks,
-Stolee
Derrick Stolee (21):
commit-graph: UNLEAK before die()
commit-graph: fix GRAPH_MIN_SIZE
commit-graph: parse commit from chosen graph
commit: force commit to parse from object database
commit-graph: load a root tree from specific graph
commit-graph: add 'verify' subcommand
commit-graph: verify catches corrupt signature
commit-graph: verify required chunks are present
commit-graph: verify corrupt OID fanout and lookup
commit-graph: verify objects exist
commit-graph: verify root tree OIDs
commit-graph: verify parent list
commit-graph: verify generation number
commit-graph: verify commit date
commit-graph: test for corrupted octopus edge
commit-graph: verify contents match checksum
fsck: verify commit-graph
commit-graph: use string-list API for input
commit-graph: add '--reachable' option
gc: automatically write commit-graph files
commit-graph: update design document
Documentation/config.txt | 10 +-
Documentation/git-commit-graph.txt | 14 +-
Documentation/git-fsck.txt | 3 +
Documentation/git-gc.txt | 4 +
Documentation/technical/commit-graph.txt | 22 --
builtin/commit-graph.c | 98 ++++++---
builtin/fsck.c | 21 ++
builtin/gc.c | 6 +
commit-graph.c | 248 +++++++++++++++++++++--
commit-graph.h | 10 +-
commit.c | 9 +-
commit.h | 1 +
t/t5318-commit-graph.sh | 201 ++++++++++++++++++
13 files changed, 569 insertions(+), 78 deletions(-)
--
2.18.0.rc1
The GRAPH_MIN_SIZE macro should be the smallest size of a parsable
commit-graph file. However, the minimum number of chunks was wrong.
It is possible to write a commit-graph file with zero commits, and
that violates this macro's value.
Rewrite the macro, and use extra macros to better explain the magic
constants.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Before verifying a commit-graph file against the object database, we
need to parse all commits from the given commit-graph file. Create
parse_commit_in_graph_one() to target a given struct commit_graph.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
When lazy-loading a tree for a commit, it will be important to select
the tree from a specific struct commit_graph. Create a new method that
specifies the commit-graph file and use that in
get_commit_tree_in_graph().
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
@@ -359,14 +359,20 @@ static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *returnc->maybe_tree;}-structtree*get_commit_tree_in_graph(conststructcommit*c)+staticstructtree*get_commit_tree_in_graph_one(structcommit_graph*g,+conststructcommit*c){if(c->maybe_tree)returnc->maybe_tree;if(c->graph_pos==COMMIT_NOT_FROM_GRAPH)-BUG("get_commit_tree_in_graph called from non-commit-graph commit");+BUG("get_commit_tree_in_graph_one called from non-commit-graph commit");++returnload_tree_for_commit(g,(structcommit*)c);+}-returnload_tree_for_commit(commit_graph,(structcommit*)c);+structtree*get_commit_tree_in_graph(conststructcommit*c)+{+returnget_commit_tree_in_graph_one(commit_graph,c);}staticvoidwrite_graph_chunk_fanout(structhashfile*f,
This is the first of several commits that add a test to check that
'git commit-graph verify' catches corruption in the commit-graph
file. The first test checks that the command catches an error in
the file signature. This is a check that exists in the existing
commit-graph reading code.
Add a helper method 'corrupt_graph_and_verify' to the test script
t5318-commit-graph.sh. This helper corrupts the commit-graph file
at a certain location, runs 'git commit-graph verify', and reports
the output to the 'err' file. This data is filtered to remove the
lines added by 'test_must_fail' when the test is run verbosely.
Then, the output is checked to contain a specific error message.
Most messages from 'git commit-graph verify' will not be marked
for translation. There will be one exception: the message that
reports an invalid checksum will be marked for translation, as that
is the only message that is intended for a typical user.
Helped-by: Szeder Gábor [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
t/t5318-commit-graph.sh | 43 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -235,9 +235,52 @@ test_expect_success 'perform fast-forward merge in full repo' 'test_cmpexpectoutput'+# the verify tests below expect the commit-graph to contain+# exactly the commits reachable from the commits/8 branch.+# If the file changes the set of commits in the list, then the+# offsets into the binary file will result in different edits+# and the tests will likely break.+ test_expect_success'git commit-graph verify''cd"$TRASH_DIRECTORY/full"&&+gitrev-parsecommits/8|gitcommit-graphwrite--stdin-commits&&gitcommit-graphverify>output'+GRAPH_BYTE_VERSION=4+GRAPH_BYTE_HASH=5++# usage: corrupt_graph_and_verify <position> <data> <string>+# Manipulates the commit-graph file at the position+# by inserting the data, then runs 'git commit-graph verify'+# and places the output in the file 'err'. Test 'err' for+# the given string.+corrupt_graph_and_verify(){+pos=$1+data="${2:-\0}"+grepstr=$3+cd"$TRASH_DIRECTORY/full"&&+test_when_finishedmvcommit-graph-backup$objdir/info/commit-graph&&+cp$objdir/info/commit-graphcommit-graph-backup&&+printf"$data"|ddof="$objdir/info/commit-graph"bs=1seek="$pos"conv=notrunc&&+test_must_failgitcommit-graphverify2>test_err&&+grep-v"^+"test_err>err+test_i18ngrep"$grepstr"err+}++test_expect_success'detect bad signature''+corrupt_graph_and_verify0"\0"\+"graph signature"+'++test_expect_success'detect bad version''+corrupt_graph_and_verify$GRAPH_BYTE_VERSION"\02"\+"graph version"+'++test_expect_success'detect bad hash version''+corrupt_graph_and_verify$GRAPH_BYTE_HASH"\02"\+"hash version"+'+ test_done
If the commit-graph file becomes corrupt, we need a way to verify
that its contents match the object database. In the manner of
'git fsck' we will implement a 'git commit-graph verify' subcommand
to report all issues with the file.
Add the 'verify' subcommand to the 'commit-graph' builtin and its
documentation. The subcommand is currently a no-op except for
loading the commit-graph into memory, which may trigger run-time
errors that would be caught by normal use. Add a simple test that
ensures the command returns a zero error code.
If no commit-graph file exists, this is an acceptable state. Do
not report any errors.
Helped-by: Ramsay Jones [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-commit-graph.txt | 6 +++++
builtin/commit-graph.c | 38 ++++++++++++++++++++++++++++++
commit-graph.c | 23 ++++++++++++++++++
commit-graph.h | 2 ++
t/t5318-commit-graph.sh | 10 ++++++++
5 files changed, 79 insertions(+)
@@ -52,6 +53,11 @@ existing commit-graph file. Read a graph file given by the commit-graph file and output basic details about the graph file. Used for debugging purposes.+'verify'::++Read the commit-graph file and verify its contents against the object+database. Used to check for corrupted data.+ EXAMPLES --------
@@ -29,6 +35,36 @@ static struct opts_commit_graph {intappend;}opts;++staticintgraph_verify(intargc,constchar**argv)+{+structcommit_graph*graph=NULL;+char*graph_name;++staticstructoptionbuiltin_commit_graph_verify_options[]={+OPT_STRING(0,"object-dir",&opts.obj_dir,+N_("dir"),+N_("The object directory to store the graph")),+OPT_END(),+};++argc=parse_options(argc,argv,NULL,+builtin_commit_graph_verify_options,+builtin_commit_graph_verify_usage,0);++if(!opts.obj_dir)+opts.obj_dir=get_object_directory();++graph_name=get_commit_graph_filename(opts.obj_dir);+graph=load_commit_graph_one(graph_name);+FREE_AND_NULL(graph_name);++if(!graph)+return0;++returnverify_commit_graph(graph);+}+staticintgraph_read(intargc,constchar**argv){structcommit_graph*graph=NULL;
@@ -11,6 +11,11 @@ test_expect_success 'setup full repo' 'objdir=".git/objects"'+test_expect_success'verify graph with no graph file''+cd"$TRASH_DIRECTORY/full"&&+gitcommit-graphverify+'+ test_expect_success'write graph with no packs''cd"$TRASH_DIRECTORY/full"&&gitcommit-graphwrite--object-dir.&&
@@ -230,4 +235,9 @@ test_expect_success 'perform fast-forward merge in full repo' 'test_cmpexpectoutput'+test_expect_success'git commit-graph verify''+cd"$TRASH_DIRECTORY/full"&&+gitcommit-graphverify>output+'+ test_done
The commit-graph file requires the following three chunks:
* OID Fanout
* OID Lookup
* Commit Data
If any of these are missing, then the 'verify' subcommand should
report a failure. This includes the chunk IDs malformed or the
chunk count is truncated.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 9 +++++++++
t/t5318-commit-graph.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
@@ -845,5 +845,14 @@ int verify_commit_graph(struct commit_graph *g)return1;}+verify_commit_graph_error=0;++if(!g->chunk_oid_fanout)+graph_report("commit-graph is missing the OID Fanout chunk");+if(!g->chunk_oid_lookup)+graph_report("commit-graph is missing the OID Lookup chunk");+if(!g->chunk_commit_data)+graph_report("commit-graph is missing the Commit Data chunk");+returnverify_commit_graph_error;}
@@ -249,6 +249,15 @@ test_expect_success 'git commit-graph verify' 'GRAPH_BYTE_VERSION=4GRAPH_BYTE_HASH=5+GRAPH_BYTE_CHUNK_COUNT=6+GRAPH_CHUNK_LOOKUP_OFFSET=8+GRAPH_CHUNK_LOOKUP_WIDTH=12+GRAPH_CHUNK_LOOKUP_ROWS=5+GRAPH_BYTE_OID_FANOUT_ID=$GRAPH_CHUNK_LOOKUP_OFFSET+GRAPH_BYTE_OID_LOOKUP_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+1\*$GRAPH_CHUNK_LOOKUP_WIDTH))+GRAPH_BYTE_COMMIT_DATA_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+2\*$GRAPH_CHUNK_LOOKUP_WIDTH))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -283,4 +292,24 @@ test_expect_success 'detect bad hash version' '"hash version"'+test_expect_success'detect low chunk count''+corrupt_graph_and_verify$GRAPH_BYTE_CHUNK_COUNT"\02"\+"missing the .* chunk"+'++test_expect_success'detect missing OID fanout chunk''+corrupt_graph_and_verify$GRAPH_BYTE_OID_FANOUT_ID"\0"\+"missing the OID Fanout chunk"+'++test_expect_success'detect missing OID lookup chunk''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_ID"\0"\+"missing the OID Lookup chunk"+'++test_expect_success'detect missing commit data chunk''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_DATA_ID"\0"\+"missing the Commit Data chunk"+'+ test_done
In the commit-graph file, the OID fanout chunk provides an index into
the OID lookup. The 'verify' subcommand should find incorrect values
in the fanout.
Similarly, the 'verify' subcommand should find out-of-order values in
the OID lookup.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 36 ++++++++++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
2 files changed, 58 insertions(+)
@@ -854,5 +857,38 @@ int verify_commit_graph(struct commit_graph *g)if(!g->chunk_commit_data)graph_report("commit-graph is missing the Commit Data chunk");+if(verify_commit_graph_error)+returnverify_commit_graph_error;++for(i=0;i<g->num_commits;i++){+hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);++if(i&&oidcmp(&prev_oid,&cur_oid)>=0)+graph_report("commit-graph has incorrect OID order: %s then %s",+oid_to_hex(&prev_oid),+oid_to_hex(&cur_oid));++oidcpy(&prev_oid,&cur_oid);++while(cur_oid.hash[0]>cur_fanout_pos){+uint32_tfanout_value=get_be32(g->chunk_oid_fanout+cur_fanout_pos);+if(i!=fanout_value)+graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",+cur_fanout_pos,fanout_value,i);++cur_fanout_pos++;+}+}++while(cur_fanout_pos<256){+uint32_tfanout_value=get_be32(g->chunk_oid_fanout+cur_fanout_pos);++if(g->num_commits!=fanout_value)+graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",+cur_fanout_pos,fanout_value,i);++cur_fanout_pos++;+}+returnverify_commit_graph_error;}
@@ -258,6 +259,12 @@ GRAPH_BYTE_OID_LOOKUP_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET + \1\*$GRAPH_CHUNK_LOOKUP_WIDTH))GRAPH_BYTE_COMMIT_DATA_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\2\*$GRAPH_CHUNK_LOOKUP_WIDTH))+GRAPH_FANOUT_OFFSET=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+$GRAPH_CHUNK_LOOKUP_WIDTH\*$GRAPH_CHUNK_LOOKUP_ROWS))+GRAPH_BYTE_FANOUT1=$(($GRAPH_FANOUT_OFFSET+4\*4))+GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET+4\*255))+GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4\*256))+GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*8))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -312,4 +319,19 @@ test_expect_success 'detect missing commit data chunk' '"missing the Commit Data chunk"'+test_expect_success'detect incorrect fanout''+corrupt_graph_and_verify$GRAPH_BYTE_FANOUT1"\01"\+"fanout value"+'++test_expect_success'detect incorrect fanout final value''+corrupt_graph_and_verify$GRAPH_BYTE_FANOUT2"\01"\+"fanout value"+'++test_expect_success'detect incorrect OID order''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_ORDER"\01"\+"incorrect OID order"+'+ test_done
The 'verify' subcommand must compare the commit content parsed from the
commit-graph against the content in the object database. Use
lookup_commit() and parse_commit_in_graph_one() to parse the commits
from the graph and compare against a commit that is loaded separately
and parsed directly from the object database.
Add checks for the root tree OID.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 17 ++++++++++++++++-
t/t5318-commit-graph.sh | 7 +++++++
2 files changed, 23 insertions(+), 1 deletion(-)
@@ -862,6 +862,8 @@ int verify_commit_graph(struct commit_graph *g)returnverify_commit_graph_error;for(i=0;i<g->num_commits;i++){+structcommit*graph_commit;+hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);if(i&&oidcmp(&prev_oid,&cur_oid)>=0)
@@ -879,6 +881,11 @@ int verify_commit_graph(struct commit_graph *g)cur_fanout_pos++;}++graph_commit=lookup_commit(&cur_oid);+if(!parse_commit_in_graph_one(g,graph_commit))+graph_report("failed to parse %s from commit-graph",+oid_to_hex(&cur_oid));}while(cur_fanout_pos<256){
@@ -895,16 +902,24 @@ int verify_commit_graph(struct commit_graph *g)returnverify_commit_graph_error;for(i=0;i<g->num_commits;i++){-structcommit*odb_commit;+structcommit*graph_commit,*odb_commit;hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);+graph_commit=lookup_commit(&cur_oid);odb_commit=(structcommit*)create_object(cur_oid.hash,alloc_commit_node());if(parse_commit_internal(odb_commit,0,0)){graph_report("failed to parse %s from object database",oid_to_hex(&cur_oid));continue;}++if(oidcmp(&get_commit_tree_in_graph_one(g,graph_commit)->object.oid,+get_commit_tree_oid(odb_commit)))+graph_report("root tree OID for commit %s in commit-graph is %s != %s",+oid_to_hex(&cur_oid),+oid_to_hex(get_commit_tree_oid(graph_commit)),+oid_to_hex(get_commit_tree_oid(odb_commit)));}returnverify_commit_graph_error;
@@ -267,6 +267,8 @@ GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET + 4 \* 255))GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4\*256))GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*8))GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*4+10))+GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*$NUM_COMMITS))+GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -341,4 +343,9 @@ test_expect_success 'detect OID not in object database' '"from object database"'+test_expect_success'detect incorrect tree OID''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_TREE"\01"\+"root tree OID for commit"+'+ test_done
The commit-graph file stores parents in a two-column portion of the
commit data chunk. If there is only one parent, then the second column
stores 0xFFFFFFFF to indicate no second parent.
The 'verify' subcommand checks the parent list for the commit loaded
from the commit-graph and the one parsed from the object database. Test
these checks for corrupt parents, too many parents, and wrong parents.
Add a boundary check to insert_parent_or_die() for when the parent
position value is out of range.
The octopus merge will be tested in a later commit.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 28 ++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 18 ++++++++++++++++++
2 files changed, 46 insertions(+)
@@ -903,6 +906,7 @@ int verify_commit_graph(struct commit_graph *g)for(i=0;i<g->num_commits;i++){structcommit*graph_commit,*odb_commit;+structcommit_list*graph_parents,*odb_parents;hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);
@@ -920,6 +924,30 @@ int verify_commit_graph(struct commit_graph *g)oid_to_hex(&cur_oid),oid_to_hex(get_commit_tree_oid(graph_commit)),oid_to_hex(get_commit_tree_oid(odb_commit)));++graph_parents=graph_commit->parents;+odb_parents=odb_commit->parents;++while(graph_parents){+if(odb_parents==NULL){+graph_report("commit-graph parent list for commit %s is too long",+oid_to_hex(&cur_oid));+break;+}++if(oidcmp(&graph_parents->item->object.oid,&odb_parents->item->object.oid))+graph_report("commit-graph parent for %s is %s != %s",+oid_to_hex(&cur_oid),+oid_to_hex(&graph_parents->item->object.oid),+oid_to_hex(&odb_parents->item->object.oid));++graph_parents=graph_parents->next;+odb_parents=odb_parents->next;+}++if(odb_parents!=NULL)+graph_report("commit-graph parent list for commit %s terminates early",+oid_to_hex(&cur_oid));}returnverify_commit_graph_error;
@@ -269,6 +269,9 @@ GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 8))GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*4+10))GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*$NUM_COMMITS))GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET+GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN))+GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))+GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -348,4 +351,19 @@ test_expect_success 'detect incorrect tree OID' '"root tree OID for commit"'+test_expect_success'detect incorrect parent int-id''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_PARENT"\01"\+"invalid parent"+'++test_expect_success'detect extra parent int-id''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_EXTRA_PARENT"\00"\+"is too long"+'++test_expect_success'detect wrong parent''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_WRONG_PARENT"\01"\+"commit-graph parent for"+'+ test_done
In the 'verify' subcommand, load commits directly from the object
database to ensure they exist. Parse by skipping the commit-graph.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 17 +++++++++++++++++
t/t5318-commit-graph.sh | 7 +++++++
2 files changed, 24 insertions(+)
@@ -890,5 +891,21 @@ int verify_commit_graph(struct commit_graph *g)cur_fanout_pos++;}+if(verify_commit_graph_error)+returnverify_commit_graph_error;++for(i=0;i<g->num_commits;i++){+structcommit*odb_commit;++hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);++odb_commit=(structcommit*)create_object(cur_oid.hash,alloc_commit_node());+if(parse_commit_internal(odb_commit,0,0)){+graph_report("failed to parse %s from object database",+oid_to_hex(&cur_oid));+continue;+}+}+returnverify_commit_graph_error;}
@@ -265,6 +266,7 @@ GRAPH_BYTE_FANOUT1=$(($GRAPH_FANOUT_OFFSET + 4 \* 4))GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET+4\*255))GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4\*256))GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*8))+GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*4+10))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -334,4 +336,9 @@ test_expect_success 'detect incorrect OID order' '"incorrect OID order"'+test_expect_success'detect OID not in object database''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_MISSING"\01"\+"from object database"+'+ test_done
The commit-graph file is a very helpful feature for speeding up git
operations. In order to make it more useful, make it possible to
write the commit-graph file during standard garbage collection
operations.
Add a 'gc.commitGraph' config setting that triggers writing a
commit-graph file after any non-trivial 'git gc' command. Defaults to
false while the commit-graph feature matures. We specifically do not
want to have this on by default until the commit-graph feature is fully
integrated with history-modifying features like shallow clones.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/config.txt | 10 +++++++++-
Documentation/git-gc.txt | 4 ++++
builtin/gc.c | 6 ++++++
t/t5318-commit-graph.sh | 14 ++++++++++++++
4 files changed, 33 insertions(+), 1 deletion(-)
@@ -900,7 +900,8 @@ the `GIT_NOTES_REF` environment variable. See linkgit:git-notes[1]. core.commitGraph:: Enable git commit graph feature. Allows reading from the- commit-graph file.+ commit-graph file. See `gc.commitGraph` for automatically+ maintaining the file. core.sparseCheckout:: Enable "sparse checkout" feature. See section "Sparse checkout" in
@@ -1553,6 +1554,13 @@ gc.autoDetach:: Make `git gc --auto` return immediately and run in background if the system supports it. Default is true.+gc.commitGraph::+ If true, then gc will rewrite the commit-graph file when+ linkgit:git-gc[1] is run. When using linkgit:git-gc[1]+ '--auto' the commit-graph will be updated if housekeeping is+ required. Default is false. See linkgit:git-commit-graph[1]+ for details.+ gc.logExpiry:: If the file gc.log exists, then `git gc --auto` won't run unless that file is more than 'gc.logExpiry' old. Default is
@@ -119,6 +119,10 @@ The optional configuration variable `gc.packRefs` determines if it within all non-bare repos or it can be set to a boolean value. This defaults to true.+The optional configuration variable `gc.commitGraph` determines if+'git gc' should run 'git commit-graph write'. This can be set to a+boolean value. This defaults to false.+ The optional configuration variable `gc.aggressiveWindow` controls how much time is spent optimizing the delta compression of the objects in the repository when the --aggressive option is specified. The larger
@@ -480,6 +483,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)if(pack_garbage.nr>0)clean_pack_garbage();+if(gc_commit_graph)+write_commit_graph_reachable(get_object_directory(),0);+if(auto_gc&&too_many_loose_objects())warning(_("There are too many unreachable loose objects; ""run 'git prune' to remove them."));
@@ -245,6 +245,20 @@ test_expect_success 'perform fast-forward merge in full repo' 'test_cmpexpectoutput'+test_expect_success'check that gc computes commit-graph''+cd"$TRASH_DIRECTORY/full"&&+gitcommit--allow-empty-m"blank"&&+gitcommit-graphwrite--reachable&&+cp$objdir/info/commit-graphcommit-graph-before-gc&&+gitreset--hardHEAD~1&&+gitconfiggc.commitGraphtrue&&+gitgc&&+cp$objdir/info/commit-graphcommit-graph-after-gc&&+!test_cmpcommit-graph-before-gccommit-graph-after-gc&&+gitcommit-graphwrite--reachable&&+test_cmpcommit-graph-after-gc$objdir/info/commit-graph+'+# the verify tests below expect the commit-graph to contain# exactly the commits reachable from the commits/8 branch.# If the file changes the set of commits in the list, then the
When writing commit-graph files, it can be convenient to ask for all
reachable commits (starting at the ref set) in the resulting file. This
is particularly helpful when writing to stdin is complicated, such as a
future integration with 'git gc'.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-commit-graph.txt | 8 ++++++--
builtin/commit-graph.c | 16 ++++++++++++----
commit-graph.c | 18 ++++++++++++++++++
commit-graph.h | 1 +
t/t5318-commit-graph.sh | 10 ++++++++++
5 files changed, 47 insertions(+), 6 deletions(-)
@@ -38,12 +38,16 @@ Write a commit graph file based on the commits found in packfiles. + With the `--stdin-packs` option, generate the new commit graph by walking objects only in the specified pack-indexes. (Cannot be combined-with --stdin-commits.)+with `--stdin-commits` or `--reachable`.) + With the `--stdin-commits` option, generate the new commit graph by walking commits starting at the commits specified in stdin as a list of OIDs in hex, one OID per line. (Cannot be combined with---stdin-packs.)+`--stdin-packs` or `--reachable`.)+++With the `--reachable` option, generate the new commit graph by walking+commits starting at all refs. (Cannot be combined with `--stdin-commits`+or `--stdin-packs`.) + With the `--append` option, include all commits that are present in the existing commit-graph file.
@@ -126,6 +127,8 @@ static int graph_write(int argc, const char **argv)OPT_STRING(0,"object-dir",&opts.obj_dir,N_("dir"),N_("The object directory to store the graph")),+OPT_BOOL(0,"reachable",&opts.reachable,+N_("start walk at all refs")),OPT_BOOL(0,"stdin-packs",&opts.stdin_packs,N_("scan pack-indexes listed by stdin for commits")),OPT_BOOL(0,"stdin-commits",&opts.stdin_commits,
@@ -139,11 +142,16 @@ static int graph_write(int argc, const char **argv)builtin_commit_graph_write_options,builtin_commit_graph_write_usage,0);-if(opts.stdin_packs&&opts.stdin_commits)-die(_("cannot use both --stdin-commits and --stdin-packs"));+if(opts.reachable+opts.stdin_packs+opts.stdin_commits>1)+die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs"));if(!opts.obj_dir)opts.obj_dir=get_object_directory();+if(opts.reachable){+write_commit_graph_reachable(opts.obj_dir,opts.append);+return0;+}+if(opts.stdin_packs||opts.stdin_commits){structstrbufbuf=STRBUF_INIT;string_list_init(&lines,0);
The commit-graph feature is now integrated with 'fsck' and 'gc',
so remove those items from the "Future Work" section of the
commit-graph design document.
Also remove the section on lazy-loading trees, as that was completed
in an earlier patch series.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/technical/commit-graph.txt | 22 ----------------------
1 file changed, 22 deletions(-)
@@ -118,9 +118,6 @@ Future Work - The commit graph feature currently does not honor commit grafts. This can be remedied by duplicating or refactoring the current graft logic.-- The 'commit-graph' subcommand does not have a "verify" mode that is- necessary for integration with fsck.- - After computing and storing generation numbers, we must make graph walks aware of generation numbers to gain the performance benefits they enable. This will mostly be accomplished by swapping a commit-date-ordered
@@ -130,25 +127,6 @@ Future Work - 'log --topo-order' - 'tag --merged'-- Currently, parse_commit_gently() requires filling in the root tree- object for a commit. This passes through lookup_tree() and consequently- lookup_object(). Also, it calls lookup_commit() when loading the parents.- These method calls check the ODB for object existence, even if the- consumer does not need the content. For example, we do not need the- tree contents when computing merge bases. Now that commit parsing is- removed from the computation time, these lookup operations are the- slowest operations keeping graph walks from being fast. Consider- loading these objects without verifying their existence in the ODB and- only loading them fully when consumers need them. Consider a method- such as "ensure_tree_loaded(commit)" that fully loads a tree before- using commit->tree.--- The current design uses the 'commit-graph' subcommand to generate the graph.- When this feature stabilizes enough to recommend to most users, we should- add automatic graph writes to common operations that create many commits.- For example, one could compute a graph on 'clone', 'fetch', or 'repack'- commands.- - A server could provide a commit graph file as part of the network protocol to avoid extra calculations by clients. This feature is only of benefit if the user is willing to trust the file, because verifying the file is correct
While iterating through the commit parents, perform the generation
number calculation and compare against the value stored in the
commit-graph.
The tests demonstrate that having a different set of parents affects
the generation number calculation, and this value propagates to
descendants. Hence, we drop the single-line condition on the output.
Since Git will ship with the commit-graph feature without generation
numbers, we need to accept commit-graphs with all generation numbers
equal to zero. In this case, ignore the generation number calculation.
However, verify that we should never have a mix of zero and non-zero
generation numbers. Create a test that sets one commit to generation
zero and all following commits report a failure as they have non-zero
generation in a file that contains generation number zero.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 34 ++++++++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 11 +++++++++++
2 files changed, 45 insertions(+)
@@ -907,6 +911,7 @@ int verify_commit_graph(struct commit_graph *g)for(i=0;i<g->num_commits;i++){structcommit*graph_commit,*odb_commit;structcommit_list*graph_parents,*odb_parents;+uint32_tmax_generation=0;hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);
@@ -941,6 +946,9 @@ int verify_commit_graph(struct commit_graph *g)oid_to_hex(&graph_parents->item->object.oid),oid_to_hex(&odb_parents->item->object.oid));+if(graph_parents->item->generation>max_generation)+max_generation=graph_parents->item->generation;+graph_parents=graph_parents->next;odb_parents=odb_parents->next;}
@@ -948,6 +956,32 @@ int verify_commit_graph(struct commit_graph *g)if(odb_parents!=NULL)graph_report("commit-graph parent list for commit %s terminates early",oid_to_hex(&cur_oid));++if(!graph_commit->generation){+if(generation_zero==GENERATION_NUMBER_EXISTS)+graph_report("commit-graph has generation number zero for commit %s, but non-zero elsewhere",+oid_to_hex(&cur_oid));+generation_zero=GENERATION_ZERO_EXISTS;+}elseif(generation_zero==GENERATION_ZERO_EXISTS)+graph_report("commit-graph has non-zero generation number for commit %s, but zero elsewhere",+oid_to_hex(&cur_oid));++if(generation_zero==GENERATION_ZERO_EXISTS)+continue;++/*+*IfoneofourparentshasgenerationGENERATION_NUMBER_MAX,then+*ourgenerationisalsoGENERATION_NUMBER_MAX.Decrementtoavoid+*extralogicinthefollowingcondition.+*/+if(max_generation==GENERATION_NUMBER_MAX)+max_generation--;++if(graph_commit->generation!=max_generation+1)+graph_report("commit-graph generation for commit %s is %u != %u",+oid_to_hex(&cur_oid),+graph_commit->generation,+max_generation+1);}returnverify_commit_graph_error;
@@ -272,6 +272,7 @@ GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSETGRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN))GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))+GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+11))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
If core.commitGraph is true, verify the contents of the commit-graph
during 'git fsck' using the 'git commit-graph verify' subcommand. Run
this check on all alternates, as well.
We use a new process for two reasons:
1. The subcommand decouples the details of loading and verifying a
commit-graph file from the other fsck details.
2. The commit-graph verification requires the commits to be loaded
in a specific order to guarantee we parse from the commit-graph
file for some objects and from the object database for others.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-fsck.txt | 3 +++
builtin/fsck.c | 21 +++++++++++++++++++++
t/t5318-commit-graph.sh | 8 ++++++++
3 files changed, 32 insertions(+)
@@ -110,6 +110,9 @@ Any corrupt objects you will have to find in backups or other archives (i.e., you can just remove them and do an 'rsync' with some other site in the hopes that somebody else has the object you have corrupted).+If core.commitGraph is true, the commit-graph file will also be inspected+using 'git commit-graph verify'. See linkgit:git-commit-graph[1].+ Extracted Diagnostics ---------------------
The commit-graph file ends with a SHA1 hash of the previous contents. If
a commit-graph file has errors but the checksum hash is correct, then we
know that the problem is a bug in Git and not simply file corruption
after-the-fact.
Compute the checksum right away so it is the first error that appears,
and make the message translatable since this error can be "corrected" by
a user by simply deleting the file and recomputing. The rest of the
errors are useful only to developers.
Be sure to continue checking the rest of the file data if the checksum
is wrong. This is important for our tests, as we break the checksum as
we modify bytes of the commit-graph file.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 16 ++++++++++++++--
t/t5318-commit-graph.sh | 6 ++++++
2 files changed, 20 insertions(+), 2 deletions(-)
@@ -868,6 +871,15 @@ int verify_commit_graph(struct commit_graph *g)if(verify_commit_graph_error)returnverify_commit_graph_error;+devnull=open("/dev/null",O_WRONLY);+f=hashfd(devnull,NULL);+hashwrite(f,g->data,g->data_len-g->hash_len);+finalize_hashfile(f,checksum.hash,CSUM_CLOSE);+if(hashcmp(checksum.hash,g->data+g->data_len-g->hash_len)){+graph_report(_("the commit-graph file has incorrect checksum and is likely corrupt"));+verify_commit_graph_error=VERIFY_COMMIT_GRAPH_ERROR_HASH;+}+for(i=0;i<g->num_commits;i++){structcommit*graph_commit;
@@ -905,7 +917,7 @@ int verify_commit_graph(struct commit_graph *g)cur_fanout_pos++;}-if(verify_commit_graph_error)+if(verify_commit_graph_error&~VERIFY_COMMIT_GRAPH_ERROR_HASH)returnverify_commit_graph_error;for(i=0;i<g->num_commits;i++){
@@ -279,6 +279,7 @@ GRAPH_COMMIT_DATA_WIDTH=$(($HASH_LEN + 16))GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET+\$GRAPH_COMMIT_DATA_WIDTH\*$NUM_COMMITS))GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET+4))+GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET+4\*$NUM_OCTOPUS_EDGES))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -982,6 +982,12 @@ int verify_commit_graph(struct commit_graph *g)oid_to_hex(&cur_oid),graph_commit->generation,max_generation+1);++if(graph_commit->date!=odb_commit->date)+graph_report("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime,+oid_to_hex(&cur_oid),+graph_commit->date,+odb_commit->date);}returnverify_commit_graph_error;
@@ -273,6 +273,7 @@ GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN))GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+11))+GRAPH_BYTE_COMMIT_DATE=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+12))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
The commit-graph file has an extra chunk to store the parent int-ids for
parents beyond the first parent for octopus merges. Our test repo has a
single octopus merge that we can manipulate to demonstrate the 'verify'
subcommand detects incorrect values in that chunk.
Signed-off-by: Derrick Stolee <redacted>
---
t/t5318-commit-graph.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
In anticipation of verifying commit-graph file contents against the
object database, create parse_commit_internal() to allow side-stepping
the commit-graph file and parse directly from the object database.
Due to the use of generation numbers, this method should not be called
unless the intention is explicit in avoiding commits from the
commit-graph file.
Signed-off-by: Derrick Stolee <redacted>
---
commit.c | 9 +++++++--
commit.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
@@ -392,7 +392,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long sreturn0;}-intparse_commit_gently(structcommit*item,intquiet_on_missing)+intparse_commit_internal(structcommit*item,intquiet_on_missing,intuse_commit_graph){enumobject_typetype;void*buffer;
@@ -403,7 +403,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)return-1;if(item->object.parsed)return0;-if(parse_commit_in_graph(item))+if(use_commit_graph&&parse_commit_in_graph(item))return0;buffer=read_sha1_file(item->object.oid.hash,&type,&size);if(!buffer)
@@ -424,6 +424,11 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)returnret;}+intparse_commit_gently(structcommit*item,intquiet_on_missing)+{+returnparse_commit_internal(item,quiet_on_missing,1);+}+voidparse_commit_or_die(structcommit*item){if(parse_commit(item))
Thanks for the feedback on v3. There were several small cleanups, but
perhaps the biggest change is the addition of "commit-graph: use
string-list API for input" which makes "commit-graph: add '--reachable'
option" much simpler.
The inter-diff is still reasonably large, but I'll send it in a
follow-up PR.
@@ -900,7 +900,8 @@ the `GIT_NOTES_REF` environment variable. See linkgit:git-notes[1].
core.commitGraph::
Enable git commit graph feature. Allows reading from the
- commit-graph file.
+ commit-graph file. See `gc.commitGraph` for automatically
+ maintaining the file.
core.sparseCheckout::
Enable "sparse checkout" feature. See section "Sparse checkout" in
@@ -1554,10 +1555,11 @@ gc.autoDetach::
if the system supports it. Default is true.
gc.commitGraph::
- If true, then gc will rewrite the commit-graph file after any
- change to the object database. If '--auto' is used, then the
- commit-graph will not be updated unless the threshold is met.
- See linkgit:git-commit-graph[1] for details.
+ If true, then gc will rewrite the commit-graph file when
+ linkgit:git-gc[1] is run. When using linkgit:git-gc[1]
+ '--auto' the commit-graph will be updated if housekeeping is
+ required. Default is false. See linkgit:git-commit-graph[1]
+ for details.
gc.logExpiry::
If the file gc.log exists, then `git gc --auto` won't run
@@ -119,9 +119,9 @@ The optional configuration variable `gc.packRefs` determines if
it within all non-bare repos or it can be set to a boolean value.
This defaults to true.
-The optional configuration variable 'gc.commitGraph' determines if
-'git gc' runs 'git commit-graph write'. This can be set to a boolean
-value. This defaults to false.
+The optional configuration variable `gc.commitGraph` determines if
+'git gc' should run 'git commit-graph write'. This can be set to a
+boolean value. This defaults to false.
The optional configuration variable `gc.aggressiveWindow` controls how
much time is spent optimizing the delta compression of the objects in
@@ -1012,6 +986,18 @@ int verify_commit_graph(struct commit_graph *g)
graph_report("commit-graph parent list for commit %s terminates early",
oid_to_hex(&cur_oid));
+ if (!graph_commit->generation) {
+ if (generation_zero == GENERATION_NUMBER_EXISTS)
+ graph_report("commit-graph has generation number zero for commit %s, but non-zero elsewhere",
+ oid_to_hex(&cur_oid));
+ generation_zero = GENERATION_ZERO_EXISTS;
+ } else if (generation_zero == GENERATION_ZERO_EXISTS)
+ graph_report("commit-graph has non-zero generation number for commit %s, but zero elsewhere",
+ oid_to_hex(&cur_oid));
+
+ if (generation_zero == GENERATION_ZERO_EXISTS)
+ continue;
+
/*
* If one of our parents has generation GENERATION_NUMBER_MAX, then
* our generation is also GENERATION_NUMBER_MAX. Decrement to avoid
On Mon, Jun 4, 2018 at 6:52 PM, Derrick Stolee [off-list ref] wrote:
Thanks for the feedback on v3. There were several small cleanups, but
perhaps the biggest change is the addition of "commit-graph: use
string-list API for input" which makes "commit-graph: add '--reachable'
option" much simpler.
Do you have a version of this pushed somewhere? Your fsck/upstream
branch conflicts with e.g. long-since-merged nd/repack-keep-pack.
On 6/5/2018 10:51 AM, Ævar Arnfjörð Bjarmason wrote:
On Mon, Jun 4, 2018 at 6:52 PM, Derrick Stolee [off-list ref] wrote:
quoted
Thanks for the feedback on v3. There were several small cleanups, but
perhaps the biggest change is the addition of "commit-graph: use
string-list API for input" which makes "commit-graph: add '--reachable'
option" much simpler.
Do you have a version of this pushed somewhere? Your fsck/upstream
branch conflicts with e.g. long-since-merged nd/repack-keep-pack.
+ prepare_alt_odb();+ for (alt = alt_odb_list; alt; alt = alt->next) {+ verify_argv[2] = "--object-dir";+ verify_argv[3] = alt->path;+ if (run_command(&commit_graph_verify))+ errors_found |= ERROR_COMMIT_GRAPH;+ }+ }+
This doesn't compile under clang on master. It needs to account for
0b20903405 ("sha1_file: add repository argument to prepare_alt_odb",
2018-03-23).
builtin/fsck.c:837:19: error: too few arguments to function call, single argument 'r' was not specified
prepare_alt_odb();
~~~~~~~~~~~~~~~ ^
Ditto this error due to a missing resolution with 031dc927f4
("object-store: move alt_odb_list and alt_odb_tail to object store",
2018-03-23):
builtin/fsck.c:838:14: error: use of undeclared identifier 'alt_odb_list'
for (alt = alt_odb_list; alt; alt = alt->next) {
@@ -900,7 +900,8 @@ the `GIT_NOTES_REF` environment variable. See linkgit:git-notes[1]. core.commitGraph:: Enable git commit graph feature. Allows reading from the- commit-graph file.+ commit-graph file. See `gc.commitGraph` for automatically+ maintaining the file. core.sparseCheckout:: Enable "sparse checkout" feature. See section "Sparse checkout" in
@@ -1553,6 +1554,13 @@ gc.autoDetach:: Make `git gc --auto` return immediately and run in background if the system supports it. Default is true.+gc.commitGraph::+ If true, then gc will rewrite the commit-graph file when+ linkgit:git-gc[1] is run. When using linkgit:git-gc[1]+ '--auto' the commit-graph will be updated if housekeeping is+ required. Default is false. See linkgit:git-commit-graph[1]+ for details.+ gc.logExpiry:: If the file gc.log exists, then `git gc --auto` won't run unless that file is more than 'gc.logExpiry' old. Default is
On 6/6/2018 7:08 AM, Ævar Arnfjörð Bjarmason wrote:
On Mon, Jun 04 2018, Derrick Stolee wrote:
quoted
+ prepare_alt_odb();+ for (alt = alt_odb_list; alt; alt = alt->next) {+ verify_argv[2] = "--object-dir";+ verify_argv[3] = alt->path;+ if (run_command(&commit_graph_verify))+ errors_found |= ERROR_COMMIT_GRAPH;+ }+ }+
This doesn't compile under clang on master. It needs to account for
0b20903405 ("sha1_file: add repository argument to prepare_alt_odb",
2018-03-23).
builtin/fsck.c:837:19: error: too few arguments to function call, single argument 'r' was not specified
prepare_alt_odb();
~~~~~~~~~~~~~~~ ^
Ditto this error due to a missing resolution with 031dc927f4
("object-store: move alt_odb_list and alt_odb_tail to object store",
2018-03-23):
builtin/fsck.c:838:14: error: use of undeclared identifier 'alt_odb_list'
for (alt = alt_odb_list; alt; alt = alt->next) {
Thanks, Ævar. I forgot to rebase onto 'next'. Doing so now and will send v5 shortly.
-Stolee
Thanks, Ævar, for pointing out that I forgot to rebase onto 'next'.
There were a few collisions with the object-store refactoring. Junio
even pointed them out, so I'm sorry to forget that. I also did a test
merge with 'pu' and it seems the only conflicts are with the earlier
version of this patch.
Thanks,
-Stolee
Derrick Stolee (21):
commit-graph: UNLEAK before die()
commit-graph: fix GRAPH_MIN_SIZE
commit-graph: parse commit from chosen graph
commit: force commit to parse from object database
commit-graph: load a root tree from specific graph
commit-graph: add 'verify' subcommand
commit-graph: verify catches corrupt signature
commit-graph: verify required chunks are present
commit-graph: verify corrupt OID fanout and lookup
commit-graph: verify objects exist
commit-graph: verify root tree OIDs
commit-graph: verify parent list
commit-graph: verify generation number
commit-graph: verify commit date
commit-graph: test for corrupted octopus edge
commit-graph: verify contents match checksum
fsck: verify commit-graph
commit-graph: use string-list API for input
commit-graph: add '--reachable' option
gc: automatically write commit-graph files
commit-graph: update design document
Documentation/config.txt | 10 +-
Documentation/git-commit-graph.txt | 14 +-
Documentation/git-fsck.txt | 3 +
Documentation/git-gc.txt | 4 +
Documentation/technical/commit-graph.txt | 22 --
builtin/commit-graph.c | 98 ++++++---
builtin/fsck.c | 21 ++
builtin/gc.c | 6 +
commit-graph.c | 248 +++++++++++++++++++++--
commit-graph.h | 10 +-
commit.c | 9 +-
commit.h | 1 +
t/t5318-commit-graph.sh | 201 ++++++++++++++++++
13 files changed, 569 insertions(+), 78 deletions(-)
--
2.18.0.rc1
When lazy-loading a tree for a commit, it will be important to select
the tree from a specific struct commit_graph. Create a new method that
specifies the commit-graph file and use that in
get_commit_tree_in_graph().
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
@@ -362,14 +362,20 @@ static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *returnc->maybe_tree;}-structtree*get_commit_tree_in_graph(conststructcommit*c)+staticstructtree*get_commit_tree_in_graph_one(structcommit_graph*g,+conststructcommit*c){if(c->maybe_tree)returnc->maybe_tree;if(c->graph_pos==COMMIT_NOT_FROM_GRAPH)-BUG("get_commit_tree_in_graph called from non-commit-graph commit");+BUG("get_commit_tree_in_graph_one called from non-commit-graph commit");++returnload_tree_for_commit(g,(structcommit*)c);+}-returnload_tree_for_commit(commit_graph,(structcommit*)c);+structtree*get_commit_tree_in_graph(conststructcommit*c)+{+returnget_commit_tree_in_graph_one(commit_graph,c);}staticvoidwrite_graph_chunk_fanout(structhashfile*f,
If core.commitGraph is true, verify the contents of the commit-graph
during 'git fsck' using the 'git commit-graph verify' subcommand. Run
this check on all alternates, as well.
We use a new process for two reasons:
1. The subcommand decouples the details of loading and verifying a
commit-graph file from the other fsck details.
2. The commit-graph verification requires the commits to be loaded
in a specific order to guarantee we parse from the commit-graph
file for some objects and from the object database for others.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-fsck.txt | 3 +++
builtin/fsck.c | 21 +++++++++++++++++++++
t/t5318-commit-graph.sh | 8 ++++++++
3 files changed, 32 insertions(+)
@@ -110,6 +110,9 @@ Any corrupt objects you will have to find in backups or other archives (i.e., you can just remove them and do an 'rsync' with some other site in the hopes that somebody else has the object you have corrupted).+If core.commitGraph is true, the commit-graph file will also be inspected+using 'git commit-graph verify'. See linkgit:git-commit-graph[1].+ Extracted Diagnostics ---------------------
The commit-graph file has an extra chunk to store the parent int-ids for
parents beyond the first parent for octopus merges. Our test repo has a
single octopus merge that we can manipulate to demonstrate the 'verify'
subcommand detects incorrect values in that chunk.
Signed-off-by: Derrick Stolee <redacted>
---
t/t5318-commit-graph.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
The commit-graph feature is now integrated with 'fsck' and 'gc',
so remove those items from the "Future Work" section of the
commit-graph design document.
Also remove the section on lazy-loading trees, as that was completed
in an earlier patch series.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/technical/commit-graph.txt | 22 ----------------------
1 file changed, 22 deletions(-)
@@ -118,9 +118,6 @@ Future Work - The commit graph feature currently does not honor commit grafts. This can be remedied by duplicating or refactoring the current graft logic.-- The 'commit-graph' subcommand does not have a "verify" mode that is- necessary for integration with fsck.- - After computing and storing generation numbers, we must make graph walks aware of generation numbers to gain the performance benefits they enable. This will mostly be accomplished by swapping a commit-date-ordered
@@ -130,25 +127,6 @@ Future Work - 'log --topo-order' - 'tag --merged'-- Currently, parse_commit_gently() requires filling in the root tree- object for a commit. This passes through lookup_tree() and consequently- lookup_object(). Also, it calls lookup_commit() when loading the parents.- These method calls check the ODB for object existence, even if the- consumer does not need the content. For example, we do not need the- tree contents when computing merge bases. Now that commit parsing is- removed from the computation time, these lookup operations are the- slowest operations keeping graph walks from being fast. Consider- loading these objects without verifying their existence in the ODB and- only loading them fully when consumers need them. Consider a method- such as "ensure_tree_loaded(commit)" that fully loads a tree before- using commit->tree.--- The current design uses the 'commit-graph' subcommand to generate the graph.- When this feature stabilizes enough to recommend to most users, we should- add automatic graph writes to common operations that create many commits.- For example, one could compute a graph on 'clone', 'fetch', or 'repack'- commands.- - A server could provide a commit graph file as part of the network protocol to avoid extra calculations by clients. This feature is only of benefit if the user is willing to trust the file, because verifying the file is correct
While iterating through the commit parents, perform the generation
number calculation and compare against the value stored in the
commit-graph.
The tests demonstrate that having a different set of parents affects
the generation number calculation, and this value propagates to
descendants. Hence, we drop the single-line condition on the output.
Since Git will ship with the commit-graph feature without generation
numbers, we need to accept commit-graphs with all generation numbers
equal to zero. In this case, ignore the generation number calculation.
However, verify that we should never have a mix of zero and non-zero
generation numbers. Create a test that sets one commit to generation
zero and all following commits report a failure as they have non-zero
generation in a file that contains generation number zero.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 34 ++++++++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 11 +++++++++++
2 files changed, 45 insertions(+)
@@ -910,6 +914,7 @@ int verify_commit_graph(struct commit_graph *g)for(i=0;i<g->num_commits;i++){structcommit*graph_commit,*odb_commit;structcommit_list*graph_parents,*odb_parents;+uint32_tmax_generation=0;hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);
@@ -944,6 +949,9 @@ int verify_commit_graph(struct commit_graph *g)oid_to_hex(&graph_parents->item->object.oid),oid_to_hex(&odb_parents->item->object.oid));+if(graph_parents->item->generation>max_generation)+max_generation=graph_parents->item->generation;+graph_parents=graph_parents->next;odb_parents=odb_parents->next;}
@@ -951,6 +959,32 @@ int verify_commit_graph(struct commit_graph *g)if(odb_parents!=NULL)graph_report("commit-graph parent list for commit %s terminates early",oid_to_hex(&cur_oid));++if(!graph_commit->generation){+if(generation_zero==GENERATION_NUMBER_EXISTS)+graph_report("commit-graph has generation number zero for commit %s, but non-zero elsewhere",+oid_to_hex(&cur_oid));+generation_zero=GENERATION_ZERO_EXISTS;+}elseif(generation_zero==GENERATION_ZERO_EXISTS)+graph_report("commit-graph has non-zero generation number for commit %s, but zero elsewhere",+oid_to_hex(&cur_oid));++if(generation_zero==GENERATION_ZERO_EXISTS)+continue;++/*+*IfoneofourparentshasgenerationGENERATION_NUMBER_MAX,then+*ourgenerationisalsoGENERATION_NUMBER_MAX.Decrementtoavoid+*extralogicinthefollowingcondition.+*/+if(max_generation==GENERATION_NUMBER_MAX)+max_generation--;++if(graph_commit->generation!=max_generation+1)+graph_report("commit-graph generation for commit %s is %u != %u",+oid_to_hex(&cur_oid),+graph_commit->generation,+max_generation+1);}returnverify_commit_graph_error;
@@ -272,6 +272,7 @@ GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSETGRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN))GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))+GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+11))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
The commit-graph file is a very helpful feature for speeding up git
operations. In order to make it more useful, make it possible to
write the commit-graph file during standard garbage collection
operations.
Add a 'gc.commitGraph' config setting that triggers writing a
commit-graph file after any non-trivial 'git gc' command. Defaults to
false while the commit-graph feature matures. We specifically do not
want to have this on by default until the commit-graph feature is fully
integrated with history-modifying features like shallow clones.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/config.txt | 10 +++++++++-
Documentation/git-gc.txt | 4 ++++
builtin/gc.c | 6 ++++++
t/t5318-commit-graph.sh | 14 ++++++++++++++
4 files changed, 33 insertions(+), 1 deletion(-)
@@ -906,7 +906,8 @@ the `GIT_NOTES_REF` environment variable. See linkgit:git-notes[1]. core.commitGraph:: Enable git commit graph feature. Allows reading from the- commit-graph file.+ commit-graph file. See `gc.commitGraph` for automatically+ maintaining the file. core.sparseCheckout:: Enable "sparse checkout" feature. See section "Sparse checkout" in
@@ -1647,6 +1648,13 @@ this configuration variable is ignored, all packs except the base pack will be repacked. After this the number of packs should go below gc.autoPackLimit and gc.bigPackThreshold should be respected again.+gc.commitGraph::+ If true, then gc will rewrite the commit-graph file when+ linkgit:git-gc[1] is run. When using linkgit:git-gc[1]+ '--auto' the commit-graph will be updated if housekeeping is+ required. Default is false. See linkgit:git-commit-graph[1]+ for details.+ gc.logExpiry:: If the file gc.log exists, then `git gc --auto` won't run unless that file is more than 'gc.logExpiry' old. Default is
@@ -136,6 +136,10 @@ The optional configuration variable `gc.packRefs` determines if it within all non-bare repos or it can be set to a boolean value. This defaults to true.+The optional configuration variable `gc.commitGraph` determines if+'git gc' should run 'git commit-graph write'. This can be set to a+boolean value. This defaults to false.+ The optional configuration variable `gc.aggressiveWindow` controls how much time is spent optimizing the delta compression of the objects in the repository when the --aggressive option is specified. The larger
@@ -641,6 +644,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)if(pack_garbage.nr>0)clean_pack_garbage();+if(gc_commit_graph)+write_commit_graph_reachable(get_object_directory(),0);+if(auto_gc&&too_many_loose_objects())warning(_("There are too many unreachable loose objects; ""run 'git prune' to remove them."));
@@ -245,6 +245,20 @@ test_expect_success 'perform fast-forward merge in full repo' 'test_cmpexpectoutput'+test_expect_success'check that gc computes commit-graph''+cd"$TRASH_DIRECTORY/full"&&+gitcommit--allow-empty-m"blank"&&+gitcommit-graphwrite--reachable&&+cp$objdir/info/commit-graphcommit-graph-before-gc&&+gitreset--hardHEAD~1&&+gitconfiggc.commitGraphtrue&&+gitgc&&+cp$objdir/info/commit-graphcommit-graph-after-gc&&+!test_cmpcommit-graph-before-gccommit-graph-after-gc&&+gitcommit-graphwrite--reachable&&+test_cmpcommit-graph-after-gc$objdir/info/commit-graph+'+# the verify tests below expect the commit-graph to contain# exactly the commits reachable from the commits/8 branch.# If the file changes the set of commits in the list, then the
In the 'verify' subcommand, load commits directly from the object
database to ensure they exist. Parse by skipping the commit-graph.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 17 +++++++++++++++++
t/t5318-commit-graph.sh | 7 +++++++
2 files changed, 24 insertions(+)
@@ -893,5 +894,21 @@ int verify_commit_graph(struct commit_graph *g)cur_fanout_pos++;}+if(verify_commit_graph_error)+returnverify_commit_graph_error;++for(i=0;i<g->num_commits;i++){+structcommit*odb_commit;++hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);++odb_commit=(structcommit*)create_object(cur_oid.hash,alloc_commit_node());+if(parse_commit_internal(odb_commit,0,0)){+graph_report("failed to parse %s from object database",+oid_to_hex(&cur_oid));+continue;+}+}+returnverify_commit_graph_error;}
@@ -265,6 +266,7 @@ GRAPH_BYTE_FANOUT1=$(($GRAPH_FANOUT_OFFSET + 4 \* 4))GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET+4\*255))GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4\*256))GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*8))+GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*4+10))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -334,4 +336,9 @@ test_expect_success 'detect incorrect OID order' '"incorrect OID order"'+test_expect_success'detect OID not in object database''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_MISSING"\01"\+"from object database"+'+ test_done
When writing commit-graph files, it can be convenient to ask for all
reachable commits (starting at the ref set) in the resulting file. This
is particularly helpful when writing to stdin is complicated, such as a
future integration with 'git gc'.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-commit-graph.txt | 8 ++++++--
builtin/commit-graph.c | 16 ++++++++++++----
commit-graph.c | 18 ++++++++++++++++++
commit-graph.h | 1 +
t/t5318-commit-graph.sh | 10 ++++++++++
5 files changed, 47 insertions(+), 6 deletions(-)
@@ -38,12 +38,16 @@ Write a commit graph file based on the commits found in packfiles. + With the `--stdin-packs` option, generate the new commit graph by walking objects only in the specified pack-indexes. (Cannot be combined-with --stdin-commits.)+with `--stdin-commits` or `--reachable`.) + With the `--stdin-commits` option, generate the new commit graph by walking commits starting at the commits specified in stdin as a list of OIDs in hex, one OID per line. (Cannot be combined with---stdin-packs.)+`--stdin-packs` or `--reachable`.)+++With the `--reachable` option, generate the new commit graph by walking+commits starting at all refs. (Cannot be combined with `--stdin-commits`+or `--stdin-packs`.) + With the `--append` option, include all commits that are present in the existing commit-graph file.
@@ -126,6 +127,8 @@ static int graph_write(int argc, const char **argv)OPT_STRING(0,"object-dir",&opts.obj_dir,N_("dir"),N_("The object directory to store the graph")),+OPT_BOOL(0,"reachable",&opts.reachable,+N_("start walk at all refs")),OPT_BOOL(0,"stdin-packs",&opts.stdin_packs,N_("scan pack-indexes listed by stdin for commits")),OPT_BOOL(0,"stdin-commits",&opts.stdin_commits,
@@ -139,11 +142,16 @@ static int graph_write(int argc, const char **argv)builtin_commit_graph_write_options,builtin_commit_graph_write_usage,0);-if(opts.stdin_packs&&opts.stdin_commits)-die(_("cannot use both --stdin-commits and --stdin-packs"));+if(opts.reachable+opts.stdin_packs+opts.stdin_commits>1)+die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs"));if(!opts.obj_dir)opts.obj_dir=get_object_directory();+if(opts.reachable){+write_commit_graph_reachable(opts.obj_dir,opts.append);+return0;+}+if(opts.stdin_packs||opts.stdin_commits){structstrbufbuf=STRBUF_INIT;string_list_init(&lines,0);
Before verifying a commit-graph file against the object database, we
need to parse all commits from the given commit-graph file. Create
parse_commit_in_graph_one() to target a given struct commit_graph.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
If the commit-graph file becomes corrupt, we need a way to verify
that its contents match the object database. In the manner of
'git fsck' we will implement a 'git commit-graph verify' subcommand
to report all issues with the file.
Add the 'verify' subcommand to the 'commit-graph' builtin and its
documentation. The subcommand is currently a no-op except for
loading the commit-graph into memory, which may trigger run-time
errors that would be caught by normal use. Add a simple test that
ensures the command returns a zero error code.
If no commit-graph file exists, this is an acceptable state. Do
not report any errors.
Helped-by: Ramsay Jones [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-commit-graph.txt | 6 +++++
builtin/commit-graph.c | 38 ++++++++++++++++++++++++++++++
commit-graph.c | 23 ++++++++++++++++++
commit-graph.h | 2 ++
t/t5318-commit-graph.sh | 10 ++++++++
5 files changed, 79 insertions(+)
@@ -52,6 +53,11 @@ existing commit-graph file. Read a graph file given by the commit-graph file and output basic details about the graph file. Used for debugging purposes.+'verify'::++Read the commit-graph file and verify its contents against the object+database. Used to check for corrupted data.+ EXAMPLES --------
@@ -29,6 +35,36 @@ static struct opts_commit_graph {intappend;}opts;++staticintgraph_verify(intargc,constchar**argv)+{+structcommit_graph*graph=NULL;+char*graph_name;++staticstructoptionbuiltin_commit_graph_verify_options[]={+OPT_STRING(0,"object-dir",&opts.obj_dir,+N_("dir"),+N_("The object directory to store the graph")),+OPT_END(),+};++argc=parse_options(argc,argv,NULL,+builtin_commit_graph_verify_options,+builtin_commit_graph_verify_usage,0);++if(!opts.obj_dir)+opts.obj_dir=get_object_directory();++graph_name=get_commit_graph_filename(opts.obj_dir);+graph=load_commit_graph_one(graph_name);+FREE_AND_NULL(graph_name);++if(!graph)+return0;++returnverify_commit_graph(graph);+}+staticintgraph_read(intargc,constchar**argv){structcommit_graph*graph=NULL;
@@ -11,6 +11,11 @@ test_expect_success 'setup full repo' 'objdir=".git/objects"'+test_expect_success'verify graph with no graph file''+cd"$TRASH_DIRECTORY/full"&&+gitcommit-graphverify+'+ test_expect_success'write graph with no packs''cd"$TRASH_DIRECTORY/full"&&gitcommit-graphwrite--object-dir.&&
@@ -230,4 +235,9 @@ test_expect_success 'perform fast-forward merge in full repo' 'test_cmpexpectoutput'+test_expect_success'git commit-graph verify''+cd"$TRASH_DIRECTORY/full"&&+gitcommit-graphverify>output+'+ test_done
The commit-graph file ends with a SHA1 hash of the previous contents. If
a commit-graph file has errors but the checksum hash is correct, then we
know that the problem is a bug in Git and not simply file corruption
after-the-fact.
Compute the checksum right away so it is the first error that appears,
and make the message translatable since this error can be "corrected" by
a user by simply deleting the file and recomputing. The rest of the
errors are useful only to developers.
Be sure to continue checking the rest of the file data if the checksum
is wrong. This is important for our tests, as we break the checksum as
we modify bytes of the commit-graph file.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 16 ++++++++++++++--
t/t5318-commit-graph.sh | 6 ++++++
2 files changed, 20 insertions(+), 2 deletions(-)
@@ -871,6 +874,15 @@ int verify_commit_graph(struct commit_graph *g)if(verify_commit_graph_error)returnverify_commit_graph_error;+devnull=open("/dev/null",O_WRONLY);+f=hashfd(devnull,NULL);+hashwrite(f,g->data,g->data_len-g->hash_len);+finalize_hashfile(f,checksum.hash,CSUM_CLOSE);+if(hashcmp(checksum.hash,g->data+g->data_len-g->hash_len)){+graph_report(_("the commit-graph file has incorrect checksum and is likely corrupt"));+verify_commit_graph_error=VERIFY_COMMIT_GRAPH_ERROR_HASH;+}+for(i=0;i<g->num_commits;i++){structcommit*graph_commit;
@@ -908,7 +920,7 @@ int verify_commit_graph(struct commit_graph *g)cur_fanout_pos++;}-if(verify_commit_graph_error)+if(verify_commit_graph_error&~VERIFY_COMMIT_GRAPH_ERROR_HASH)returnverify_commit_graph_error;for(i=0;i<g->num_commits;i++){
@@ -279,6 +279,7 @@ GRAPH_COMMIT_DATA_WIDTH=$(($HASH_LEN + 16))GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET+\$GRAPH_COMMIT_DATA_WIDTH\*$NUM_COMMITS))GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET+4))+GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET+4\*$NUM_OCTOPUS_EDGES))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
In the commit-graph file, the OID fanout chunk provides an index into
the OID lookup. The 'verify' subcommand should find incorrect values
in the fanout.
Similarly, the 'verify' subcommand should find out-of-order values in
the OID lookup.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 36 ++++++++++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
2 files changed, 58 insertions(+)
@@ -857,5 +860,38 @@ int verify_commit_graph(struct commit_graph *g)if(!g->chunk_commit_data)graph_report("commit-graph is missing the Commit Data chunk");+if(verify_commit_graph_error)+returnverify_commit_graph_error;++for(i=0;i<g->num_commits;i++){+hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);++if(i&&oidcmp(&prev_oid,&cur_oid)>=0)+graph_report("commit-graph has incorrect OID order: %s then %s",+oid_to_hex(&prev_oid),+oid_to_hex(&cur_oid));++oidcpy(&prev_oid,&cur_oid);++while(cur_oid.hash[0]>cur_fanout_pos){+uint32_tfanout_value=get_be32(g->chunk_oid_fanout+cur_fanout_pos);+if(i!=fanout_value)+graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",+cur_fanout_pos,fanout_value,i);++cur_fanout_pos++;+}+}++while(cur_fanout_pos<256){+uint32_tfanout_value=get_be32(g->chunk_oid_fanout+cur_fanout_pos);++if(g->num_commits!=fanout_value)+graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",+cur_fanout_pos,fanout_value,i);++cur_fanout_pos++;+}+returnverify_commit_graph_error;}
@@ -258,6 +259,12 @@ GRAPH_BYTE_OID_LOOKUP_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET + \1\*$GRAPH_CHUNK_LOOKUP_WIDTH))GRAPH_BYTE_COMMIT_DATA_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\2\*$GRAPH_CHUNK_LOOKUP_WIDTH))+GRAPH_FANOUT_OFFSET=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+$GRAPH_CHUNK_LOOKUP_WIDTH\*$GRAPH_CHUNK_LOOKUP_ROWS))+GRAPH_BYTE_FANOUT1=$(($GRAPH_FANOUT_OFFSET+4\*4))+GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET+4\*255))+GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4\*256))+GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*8))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -312,4 +319,19 @@ test_expect_success 'detect missing commit data chunk' '"missing the Commit Data chunk"'+test_expect_success'detect incorrect fanout''+corrupt_graph_and_verify$GRAPH_BYTE_FANOUT1"\01"\+"fanout value"+'++test_expect_success'detect incorrect fanout final value''+corrupt_graph_and_verify$GRAPH_BYTE_FANOUT2"\01"\+"fanout value"+'++test_expect_success'detect incorrect OID order''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_ORDER"\01"\+"incorrect OID order"+'+ test_done
In anticipation of verifying commit-graph file contents against the
object database, create parse_commit_internal() to allow side-stepping
the commit-graph file and parse directly from the object database.
Due to the use of generation numbers, this method should not be called
unless the intention is explicit in avoiding commits from the
commit-graph file.
Signed-off-by: Derrick Stolee <redacted>
---
commit.c | 9 +++++++--
commit.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
@@ -405,7 +405,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long sreturn0;}-intparse_commit_gently(structcommit*item,intquiet_on_missing)+intparse_commit_internal(structcommit*item,intquiet_on_missing,intuse_commit_graph){enumobject_typetype;void*buffer;
@@ -416,7 +416,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)return-1;if(item->object.parsed)return0;-if(parse_commit_in_graph(item))+if(use_commit_graph&&parse_commit_in_graph(item))return0;buffer=read_object_file(&item->object.oid,&type,&size);if(!buffer)
@@ -437,6 +437,11 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)returnret;}+intparse_commit_gently(structcommit*item,intquiet_on_missing)+{+returnparse_commit_internal(item,quiet_on_missing,1);+}+voidparse_commit_or_die(structcommit*item){if(parse_commit(item))
@@ -985,6 +985,12 @@ int verify_commit_graph(struct commit_graph *g)oid_to_hex(&cur_oid),graph_commit->generation,max_generation+1);++if(graph_commit->date!=odb_commit->date)+graph_report("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime,+oid_to_hex(&cur_oid),+graph_commit->date,+odb_commit->date);}returnverify_commit_graph_error;
@@ -273,6 +273,7 @@ GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN))GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+11))+GRAPH_BYTE_COMMIT_DATE=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+12))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
This is the first of several commits that add a test to check that
'git commit-graph verify' catches corruption in the commit-graph
file. The first test checks that the command catches an error in
the file signature. This is a check that exists in the existing
commit-graph reading code.
Add a helper method 'corrupt_graph_and_verify' to the test script
t5318-commit-graph.sh. This helper corrupts the commit-graph file
at a certain location, runs 'git commit-graph verify', and reports
the output to the 'err' file. This data is filtered to remove the
lines added by 'test_must_fail' when the test is run verbosely.
Then, the output is checked to contain a specific error message.
Most messages from 'git commit-graph verify' will not be marked
for translation. There will be one exception: the message that
reports an invalid checksum will be marked for translation, as that
is the only message that is intended for a typical user.
Helped-by: Szeder Gábor [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
t/t5318-commit-graph.sh | 43 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -235,9 +235,52 @@ test_expect_success 'perform fast-forward merge in full repo' 'test_cmpexpectoutput'+# the verify tests below expect the commit-graph to contain+# exactly the commits reachable from the commits/8 branch.+# If the file changes the set of commits in the list, then the+# offsets into the binary file will result in different edits+# and the tests will likely break.+ test_expect_success'git commit-graph verify''cd"$TRASH_DIRECTORY/full"&&+gitrev-parsecommits/8|gitcommit-graphwrite--stdin-commits&&gitcommit-graphverify>output'+GRAPH_BYTE_VERSION=4+GRAPH_BYTE_HASH=5++# usage: corrupt_graph_and_verify <position> <data> <string>+# Manipulates the commit-graph file at the position+# by inserting the data, then runs 'git commit-graph verify'+# and places the output in the file 'err'. Test 'err' for+# the given string.+corrupt_graph_and_verify(){+pos=$1+data="${2:-\0}"+grepstr=$3+cd"$TRASH_DIRECTORY/full"&&+test_when_finishedmvcommit-graph-backup$objdir/info/commit-graph&&+cp$objdir/info/commit-graphcommit-graph-backup&&+printf"$data"|ddof="$objdir/info/commit-graph"bs=1seek="$pos"conv=notrunc&&+test_must_failgitcommit-graphverify2>test_err&&+grep-v"^+"test_err>err+test_i18ngrep"$grepstr"err+}++test_expect_success'detect bad signature''+corrupt_graph_and_verify0"\0"\+"graph signature"+'++test_expect_success'detect bad version''+corrupt_graph_and_verify$GRAPH_BYTE_VERSION"\02"\+"graph version"+'++test_expect_success'detect bad hash version''+corrupt_graph_and_verify$GRAPH_BYTE_HASH"\02"\+"hash version"+'+ test_done
The commit-graph file requires the following three chunks:
* OID Fanout
* OID Lookup
* Commit Data
If any of these are missing, then the 'verify' subcommand should
report a failure. This includes the chunk IDs malformed or the
chunk count is truncated.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 9 +++++++++
t/t5318-commit-graph.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
@@ -848,5 +848,14 @@ int verify_commit_graph(struct commit_graph *g)return1;}+verify_commit_graph_error=0;++if(!g->chunk_oid_fanout)+graph_report("commit-graph is missing the OID Fanout chunk");+if(!g->chunk_oid_lookup)+graph_report("commit-graph is missing the OID Lookup chunk");+if(!g->chunk_commit_data)+graph_report("commit-graph is missing the Commit Data chunk");+returnverify_commit_graph_error;}
@@ -249,6 +249,15 @@ test_expect_success 'git commit-graph verify' 'GRAPH_BYTE_VERSION=4GRAPH_BYTE_HASH=5+GRAPH_BYTE_CHUNK_COUNT=6+GRAPH_CHUNK_LOOKUP_OFFSET=8+GRAPH_CHUNK_LOOKUP_WIDTH=12+GRAPH_CHUNK_LOOKUP_ROWS=5+GRAPH_BYTE_OID_FANOUT_ID=$GRAPH_CHUNK_LOOKUP_OFFSET+GRAPH_BYTE_OID_LOOKUP_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+1\*$GRAPH_CHUNK_LOOKUP_WIDTH))+GRAPH_BYTE_COMMIT_DATA_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+2\*$GRAPH_CHUNK_LOOKUP_WIDTH))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -283,4 +292,24 @@ test_expect_success 'detect bad hash version' '"hash version"'+test_expect_success'detect low chunk count''+corrupt_graph_and_verify$GRAPH_BYTE_CHUNK_COUNT"\02"\+"missing the .* chunk"+'++test_expect_success'detect missing OID fanout chunk''+corrupt_graph_and_verify$GRAPH_BYTE_OID_FANOUT_ID"\0"\+"missing the OID Fanout chunk"+'++test_expect_success'detect missing OID lookup chunk''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_ID"\0"\+"missing the OID Lookup chunk"+'++test_expect_success'detect missing commit data chunk''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_DATA_ID"\0"\+"missing the Commit Data chunk"+'+ test_done
The commit-graph file stores parents in a two-column portion of the
commit data chunk. If there is only one parent, then the second column
stores 0xFFFFFFFF to indicate no second parent.
The 'verify' subcommand checks the parent list for the commit loaded
from the commit-graph and the one parsed from the object database. Test
these checks for corrupt parents, too many parents, and wrong parents.
Add a boundary check to insert_parent_or_die() for when the parent
position value is out of range.
The octopus merge will be tested in a later commit.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 28 ++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 18 ++++++++++++++++++
2 files changed, 46 insertions(+)
@@ -906,6 +909,7 @@ int verify_commit_graph(struct commit_graph *g)for(i=0;i<g->num_commits;i++){structcommit*graph_commit,*odb_commit;+structcommit_list*graph_parents,*odb_parents;hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);
@@ -923,6 +927,30 @@ int verify_commit_graph(struct commit_graph *g)oid_to_hex(&cur_oid),oid_to_hex(get_commit_tree_oid(graph_commit)),oid_to_hex(get_commit_tree_oid(odb_commit)));++graph_parents=graph_commit->parents;+odb_parents=odb_commit->parents;++while(graph_parents){+if(odb_parents==NULL){+graph_report("commit-graph parent list for commit %s is too long",+oid_to_hex(&cur_oid));+break;+}++if(oidcmp(&graph_parents->item->object.oid,&odb_parents->item->object.oid))+graph_report("commit-graph parent for %s is %s != %s",+oid_to_hex(&cur_oid),+oid_to_hex(&graph_parents->item->object.oid),+oid_to_hex(&odb_parents->item->object.oid));++graph_parents=graph_parents->next;+odb_parents=odb_parents->next;+}++if(odb_parents!=NULL)+graph_report("commit-graph parent list for commit %s terminates early",+oid_to_hex(&cur_oid));}returnverify_commit_graph_error;
@@ -269,6 +269,9 @@ GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 8))GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*4+10))GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*$NUM_COMMITS))GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET+GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN))+GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))+GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -348,4 +351,19 @@ test_expect_success 'detect incorrect tree OID' '"root tree OID for commit"'+test_expect_success'detect incorrect parent int-id''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_PARENT"\01"\+"invalid parent"+'++test_expect_success'detect extra parent int-id''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_EXTRA_PARENT"\00"\+"is too long"+'++test_expect_success'detect wrong parent''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_WRONG_PARENT"\01"\+"commit-graph parent for"+'+ test_done
The 'verify' subcommand must compare the commit content parsed from the
commit-graph against the content in the object database. Use
lookup_commit() and parse_commit_in_graph_one() to parse the commits
from the graph and compare against a commit that is loaded separately
and parsed directly from the object database.
Add checks for the root tree OID.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 17 ++++++++++++++++-
t/t5318-commit-graph.sh | 7 +++++++
2 files changed, 23 insertions(+), 1 deletion(-)
@@ -865,6 +865,8 @@ int verify_commit_graph(struct commit_graph *g)returnverify_commit_graph_error;for(i=0;i<g->num_commits;i++){+structcommit*graph_commit;+hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);if(i&&oidcmp(&prev_oid,&cur_oid)>=0)
@@ -882,6 +884,11 @@ int verify_commit_graph(struct commit_graph *g)cur_fanout_pos++;}++graph_commit=lookup_commit(&cur_oid);+if(!parse_commit_in_graph_one(g,graph_commit))+graph_report("failed to parse %s from commit-graph",+oid_to_hex(&cur_oid));}while(cur_fanout_pos<256){
@@ -898,16 +905,24 @@ int verify_commit_graph(struct commit_graph *g)returnverify_commit_graph_error;for(i=0;i<g->num_commits;i++){-structcommit*odb_commit;+structcommit*graph_commit,*odb_commit;hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);+graph_commit=lookup_commit(&cur_oid);odb_commit=(structcommit*)create_object(cur_oid.hash,alloc_commit_node());if(parse_commit_internal(odb_commit,0,0)){graph_report("failed to parse %s from object database",oid_to_hex(&cur_oid));continue;}++if(oidcmp(&get_commit_tree_in_graph_one(g,graph_commit)->object.oid,+get_commit_tree_oid(odb_commit)))+graph_report("root tree OID for commit %s in commit-graph is %s != %s",+oid_to_hex(&cur_oid),+oid_to_hex(get_commit_tree_oid(graph_commit)),+oid_to_hex(get_commit_tree_oid(odb_commit)));}returnverify_commit_graph_error;
@@ -267,6 +267,8 @@ GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET + 4 \* 255))GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4\*256))GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*8))GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*4+10))+GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN\*$NUM_COMMITS))+GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -341,4 +343,9 @@ test_expect_success 'detect OID not in object database' '"from object database"'+test_expect_success'detect incorrect tree OID''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_TREE"\01"\+"root tree OID for commit"+'+ test_done
The GRAPH_MIN_SIZE macro should be the smallest size of a parsable
commit-graph file. However, the minimum number of chunks was wrong.
It is possible to write a commit-graph file with zero commits, and
that violates this macro's value.
Rewrite the macro, and use extra macros to better explain the magic
constants.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
This results in an invalid free() & segfault because you're freeing
&lines which may not have been allocated by string_list_init().
Monkeypatch on top which I used to fix it:
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 76423b3fa5..c7eb68aa3a 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -122,6 +122,7 @@ static int graph_write(int argc, const char **argv)
struct string_list *pack_indexes = NULL;
struct string_list *commit_hex = NULL;
struct string_list lines;
+ int free_lines = 0;
static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -155,6 +156,7 @@ static int graph_write(int argc, const char **argv)
if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
string_list_init(&lines, 0);
+ free_lines = 1;
while (strbuf_getline(&buf, stdin) != EOF)
string_list_append(&lines, strbuf_detach(&buf, NULL));
@@ -170,7 +172,8 @@ static int graph_write(int argc, const char **argv)
commit_hex,
opts.append);
- string_list_clear(&lines, 0);
+ if (free_lines)
+ string_list_clear(&lines, 0);
return 0;
}
But probably having a pointer to the struct which is NULL etc. is
better.
On GNU bash, version 4.1.2(2)-release (x86_64-redhat-linux-gnu) this
emits:
./t5318-commit-graph.sh: line 285: 8 + 1 \* 12: syntax error: invalid arithmetic operator (error token is "\* 12")
The same goes for the rest of this "\*" within $((...)) in this file, it
should just be "*"..
This results in an invalid free() & segfault because you're freeing
&lines which may not have been allocated by string_list_init().
Good point. Did my tests not catch this? (seems it requires calling
`git commit-graph write` with no `--stdin-packs` or
`--stdin-commits`).
Most of your tests (t5318-commit-graph.sh) segfaulted, but presumably
you're on a more forgiving compiler/platform/options. I compiled with
-O0 -g on clang 4.0.1-8 + Debian testing.
quoted
Monkeypatch on top which I used to fix it:
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 76423b3fa5..c7eb68aa3a 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -122,6 +122,7 @@ static int graph_write(int argc, const char **argv)
struct string_list *pack_indexes = NULL;
struct string_list *commit_hex = NULL;
struct string_list lines;
+ int free_lines = 0;
static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -155,6 +156,7 @@ static int graph_write(int argc, const char **argv)
if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
string_list_init(&lines, 0);
+ free_lines = 1;
while (strbuf_getline(&buf, stdin) != EOF)
string_list_append(&lines, strbuf_detach(&buf, NULL));
@@ -170,7 +172,8 @@ static int graph_write(int argc, const char **argv)
commit_hex,
opts.append);
- string_list_clear(&lines, 0);
+ if (free_lines)
+ string_list_clear(&lines, 0);
return 0;
}
But probably having a pointer to the struct which is NULL etc. is
better.
Wouldn't the easiest fix be to call `string_list_init(&lines, 0)`
outside of any conditional?
Sure that works too. We'd be doing the init when we don't need it, but
it's not like this part is performance critical or anything...
This results in an invalid free() & segfault because you're freeing
&lines which may not have been allocated by string_list_init().
Good point. Did my tests not catch this? (seems it requires calling
`git commit-graph write` with no `--stdin-packs` or
`--stdin-commits`).
Most of your tests (t5318-commit-graph.sh) segfaulted, but presumably
you're on a more forgiving compiler/platform/options. I compiled with
-O0 -g on clang 4.0.1-8 + Debian testing.
I appreciate the extra platform testing. I'm using GCC on Ubuntu (gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0).
quoted
quoted
Monkeypatch on top which I used to fix it:
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 76423b3fa5..c7eb68aa3a 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -122,6 +122,7 @@ static int graph_write(int argc, const char **argv)
struct string_list *pack_indexes = NULL;
struct string_list *commit_hex = NULL;
struct string_list lines;
+ int free_lines = 0;
static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -155,6 +156,7 @@ static int graph_write(int argc, const char **argv)
if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
string_list_init(&lines, 0);
+ free_lines = 1;
while (strbuf_getline(&buf, stdin) != EOF)
string_list_append(&lines, strbuf_detach(&buf, NULL));
@@ -170,7 +172,8 @@ static int graph_write(int argc, const char **argv)
commit_hex,
opts.append);
- string_list_clear(&lines, 0);
+ if (free_lines)
+ string_list_clear(&lines, 0);
return 0;
}
But probably having a pointer to the struct which is NULL etc. is
better.
Wouldn't the easiest fix be to call `string_list_init(&lines, 0)`
outside of any conditional?
Sure that works too. We'd be doing the init when we don't need it, but
it's not like this part is performance critical or anything...
This results in an invalid free() & segfault because you're freeing
&lines which may not have been allocated by string_list_init().
Good point. Did my tests not catch this? (seems it requires calling
`git commit-graph write` with no `--stdin-packs` or
`--stdin-commits`).
Most of your tests (t5318-commit-graph.sh) segfaulted, but presumably
you're on a more forgiving compiler/platform/options. I compiled with
-O0 -g on clang 4.0.1-8 + Debian testing.
I appreciate the extra platform testing. I'm using GCC on Ubuntu (gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0).
While the errors didn't repro on my box, they did on the VSTS Linux build machines [1]. I created an internal PR just so I could run those tests (and on our OSX agents which use clang) and so I can verify I fixed the situation. I'll send a v6 soon so Junio only picks up a version that succeeds in tests.
Thanks,
-Stolee
[1] https://github.com/Microsoft/vsts-agent-docker/blob/master/ubuntu/16.04/standard/Dockerfile
What's installed on a VSTS Linux build agent
Sorry for the fast rerolls, but Ævar found some runtime issues with the
string-list use and '\*' as multiplication on some platforms. Thus, v4
and v5 are broken. I did get a repro of those issues using VSTS Linux
build servers and checked that they are fixed in this version.
This version depends on 'next' and sb/object-store-alloc. To accommodate
the repository arguments added by sb/object-store-alloc, the following
diff occurs from the previous patch:
@@ -948,7 +949,7 @@ int verify_commit_graph(struct commit_graph *g)hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);graph_commit=lookup_commit(&cur_oid);-odb_commit=(structcommit*)create_object(cur_oid.hash,alloc_commit_node());+odb_commit=(structcommit*)create_object(r,cur_oid.hash,alloc_commit_node(r));if(parse_commit_internal(odb_commit,0,0)){graph_report("failed to parse %s from object database",oid_to_hex(&cur_oid));
The GRAPH_MIN_SIZE macro should be the smallest size of a parsable
commit-graph file. However, the minimum number of chunks was wrong.
It is possible to write a commit-graph file with zero commits, and
that violates this macro's value.
Rewrite the macro, and use extra macros to better explain the magic
constants.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Before verifying a commit-graph file against the object database, we
need to parse all commits from the given commit-graph file. Create
parse_commit_in_graph_one() to target a given struct commit_graph.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
When lazy-loading a tree for a commit, it will be important to select
the tree from a specific struct commit_graph. Create a new method that
specifies the commit-graph file and use that in
get_commit_tree_in_graph().
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
@@ -362,14 +362,20 @@ static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *returnc->maybe_tree;}-structtree*get_commit_tree_in_graph(conststructcommit*c)+staticstructtree*get_commit_tree_in_graph_one(structcommit_graph*g,+conststructcommit*c){if(c->maybe_tree)returnc->maybe_tree;if(c->graph_pos==COMMIT_NOT_FROM_GRAPH)-BUG("get_commit_tree_in_graph called from non-commit-graph commit");+BUG("get_commit_tree_in_graph_one called from non-commit-graph commit");++returnload_tree_for_commit(g,(structcommit*)c);+}-returnload_tree_for_commit(commit_graph,(structcommit*)c);+structtree*get_commit_tree_in_graph(conststructcommit*c)+{+returnget_commit_tree_in_graph_one(commit_graph,c);}staticvoidwrite_graph_chunk_fanout(structhashfile*f,
If the commit-graph file becomes corrupt, we need a way to verify
that its contents match the object database. In the manner of
'git fsck' we will implement a 'git commit-graph verify' subcommand
to report all issues with the file.
Add the 'verify' subcommand to the 'commit-graph' builtin and its
documentation. The subcommand is currently a no-op except for
loading the commit-graph into memory, which may trigger run-time
errors that would be caught by normal use. Add a simple test that
ensures the command returns a zero error code.
If no commit-graph file exists, this is an acceptable state. Do
not report any errors.
Helped-by: Ramsay Jones [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-commit-graph.txt | 6 +++++
builtin/commit-graph.c | 39 ++++++++++++++++++++++++++++++
commit-graph.c | 23 ++++++++++++++++++
commit-graph.h | 3 +++
t/t5318-commit-graph.sh | 10 ++++++++
5 files changed, 81 insertions(+)
@@ -52,6 +53,11 @@ existing commit-graph file. Read a graph file given by the commit-graph file and output basic details about the graph file. Used for debugging purposes.+'verify'::++Read the commit-graph file and verify its contents against the object+database. Used to check for corrupted data.+ EXAMPLES --------
@@ -29,6 +36,36 @@ static struct opts_commit_graph {intappend;}opts;++staticintgraph_verify(intargc,constchar**argv)+{+structcommit_graph*graph=NULL;+char*graph_name;++staticstructoptionbuiltin_commit_graph_verify_options[]={+OPT_STRING(0,"object-dir",&opts.obj_dir,+N_("dir"),+N_("The object directory to store the graph")),+OPT_END(),+};++argc=parse_options(argc,argv,NULL,+builtin_commit_graph_verify_options,+builtin_commit_graph_verify_usage,0);++if(!opts.obj_dir)+opts.obj_dir=get_object_directory();++graph_name=get_commit_graph_filename(opts.obj_dir);+graph=load_commit_graph_one(graph_name);+FREE_AND_NULL(graph_name);++if(!graph)+return0;++returnverify_commit_graph(the_repository,graph);+}+staticintgraph_read(intargc,constchar**argv){structcommit_graph*graph=NULL;
@@ -11,6 +11,11 @@ test_expect_success 'setup full repo' 'objdir=".git/objects"'+test_expect_success'verify graph with no graph file''+cd"$TRASH_DIRECTORY/full"&&+gitcommit-graphverify+'+ test_expect_success'write graph with no packs''cd"$TRASH_DIRECTORY/full"&&gitcommit-graphwrite--object-dir.&&
@@ -230,4 +235,9 @@ test_expect_success 'perform fast-forward merge in full repo' 'test_cmpexpectoutput'+test_expect_success'git commit-graph verify''+cd"$TRASH_DIRECTORY/full"&&+gitcommit-graphverify>output+'+ test_done
In the commit-graph file, the OID fanout chunk provides an index into
the OID lookup. The 'verify' subcommand should find incorrect values
in the fanout.
Similarly, the 'verify' subcommand should find out-of-order values in
the OID lookup.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 36 ++++++++++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
2 files changed, 58 insertions(+)
@@ -857,5 +860,38 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)if(!g->chunk_commit_data)graph_report("commit-graph is missing the Commit Data chunk");+if(verify_commit_graph_error)+returnverify_commit_graph_error;++for(i=0;i<g->num_commits;i++){+hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);++if(i&&oidcmp(&prev_oid,&cur_oid)>=0)+graph_report("commit-graph has incorrect OID order: %s then %s",+oid_to_hex(&prev_oid),+oid_to_hex(&cur_oid));++oidcpy(&prev_oid,&cur_oid);++while(cur_oid.hash[0]>cur_fanout_pos){+uint32_tfanout_value=get_be32(g->chunk_oid_fanout+cur_fanout_pos);+if(i!=fanout_value)+graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",+cur_fanout_pos,fanout_value,i);++cur_fanout_pos++;+}+}++while(cur_fanout_pos<256){+uint32_tfanout_value=get_be32(g->chunk_oid_fanout+cur_fanout_pos);++if(g->num_commits!=fanout_value)+graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",+cur_fanout_pos,fanout_value,i);++cur_fanout_pos++;+}+returnverify_commit_graph_error;}
@@ -258,6 +259,12 @@ GRAPH_BYTE_OID_LOOKUP_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET + \1*$GRAPH_CHUNK_LOOKUP_WIDTH))GRAPH_BYTE_COMMIT_DATA_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\2*$GRAPH_CHUNK_LOOKUP_WIDTH))+GRAPH_FANOUT_OFFSET=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+$GRAPH_CHUNK_LOOKUP_WIDTH*$GRAPH_CHUNK_LOOKUP_ROWS))+GRAPH_BYTE_FANOUT1=$(($GRAPH_FANOUT_OFFSET+4*4))+GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET+4*255))+GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4*256))+GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN*8))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -312,4 +319,19 @@ test_expect_success 'detect missing commit data chunk' '"missing the Commit Data chunk"'+test_expect_success'detect incorrect fanout''+corrupt_graph_and_verify$GRAPH_BYTE_FANOUT1"\01"\+"fanout value"+'++test_expect_success'detect incorrect fanout final value''+corrupt_graph_and_verify$GRAPH_BYTE_FANOUT2"\01"\+"fanout value"+'++test_expect_success'detect incorrect OID order''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_ORDER"\01"\+"incorrect OID order"+'+ test_done
The commit-graph file stores parents in a two-column portion of the
commit data chunk. If there is only one parent, then the second column
stores 0xFFFFFFFF to indicate no second parent.
The 'verify' subcommand checks the parent list for the commit loaded
from the commit-graph and the one parsed from the object database. Test
these checks for corrupt parents, too many parents, and wrong parents.
Add a boundary check to insert_parent_or_die() for when the parent
position value is out of range.
The octopus merge will be tested in a later commit.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 28 ++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 18 ++++++++++++++++++
2 files changed, 46 insertions(+)
@@ -924,6 +928,30 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)oid_to_hex(&cur_oid),oid_to_hex(get_commit_tree_oid(graph_commit)),oid_to_hex(get_commit_tree_oid(odb_commit)));++graph_parents=graph_commit->parents;+odb_parents=odb_commit->parents;++while(graph_parents){+if(odb_parents==NULL){+graph_report("commit-graph parent list for commit %s is too long",+oid_to_hex(&cur_oid));+break;+}++if(oidcmp(&graph_parents->item->object.oid,&odb_parents->item->object.oid))+graph_report("commit-graph parent for %s is %s != %s",+oid_to_hex(&cur_oid),+oid_to_hex(&graph_parents->item->object.oid),+oid_to_hex(&odb_parents->item->object.oid));++graph_parents=graph_parents->next;+odb_parents=odb_parents->next;+}++if(odb_parents!=NULL)+graph_report("commit-graph parent list for commit %s terminates early",+oid_to_hex(&cur_oid));}returnverify_commit_graph_error;
@@ -269,6 +269,9 @@ GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN * 8))GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN*4+10))GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN*$NUM_COMMITS))GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET+GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN))+GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))+GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -348,4 +351,19 @@ test_expect_success 'detect incorrect tree OID' '"root tree OID for commit"'+test_expect_success'detect incorrect parent int-id''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_PARENT"\01"\+"invalid parent"+'++test_expect_success'detect extra parent int-id''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_EXTRA_PARENT"\00"\+"is too long"+'++test_expect_success'detect wrong parent''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_WRONG_PARENT"\01"\+"commit-graph parent for"+'+ test_done
In the 'verify' subcommand, load commits directly from the object
database to ensure they exist. Parse by skipping the commit-graph.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 18 ++++++++++++++++++
t/t5318-commit-graph.sh | 7 +++++++
2 files changed, 25 insertions(+)
@@ -265,6 +266,7 @@ GRAPH_BYTE_FANOUT1=$(($GRAPH_FANOUT_OFFSET + 4 * 4))GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET+4*255))GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4*256))GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN*8))+GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN*4+10))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -334,4 +336,9 @@ test_expect_success 'detect incorrect OID order' '"incorrect OID order"'+test_expect_success'detect OID not in object database''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_MISSING"\01"\+"from object database"+'+ test_done
If core.commitGraph is true, verify the contents of the commit-graph
during 'git fsck' using the 'git commit-graph verify' subcommand. Run
this check on all alternates, as well.
We use a new process for two reasons:
1. The subcommand decouples the details of loading and verifying a
commit-graph file from the other fsck details.
2. The commit-graph verification requires the commits to be loaded
in a specific order to guarantee we parse from the commit-graph
file for some objects and from the object database for others.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-fsck.txt | 3 +++
builtin/fsck.c | 21 +++++++++++++++++++++
t/t5318-commit-graph.sh | 8 ++++++++
3 files changed, 32 insertions(+)
@@ -110,6 +110,9 @@ Any corrupt objects you will have to find in backups or other archives (i.e., you can just remove them and do an 'rsync' with some other site in the hopes that somebody else has the object you have corrupted).+If core.commitGraph is true, the commit-graph file will also be inspected+using 'git commit-graph verify'. See linkgit:git-commit-graph[1].+ Extracted Diagnostics ---------------------
The commit-graph feature is now integrated with 'fsck' and 'gc',
so remove those items from the "Future Work" section of the
commit-graph design document.
Also remove the section on lazy-loading trees, as that was completed
in an earlier patch series.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/technical/commit-graph.txt | 22 ----------------------
1 file changed, 22 deletions(-)
@@ -118,9 +118,6 @@ Future Work - The commit graph feature currently does not honor commit grafts. This can be remedied by duplicating or refactoring the current graft logic.-- The 'commit-graph' subcommand does not have a "verify" mode that is- necessary for integration with fsck.- - After computing and storing generation numbers, we must make graph walks aware of generation numbers to gain the performance benefits they enable. This will mostly be accomplished by swapping a commit-date-ordered
@@ -130,25 +127,6 @@ Future Work - 'log --topo-order' - 'tag --merged'-- Currently, parse_commit_gently() requires filling in the root tree- object for a commit. This passes through lookup_tree() and consequently- lookup_object(). Also, it calls lookup_commit() when loading the parents.- These method calls check the ODB for object existence, even if the- consumer does not need the content. For example, we do not need the- tree contents when computing merge bases. Now that commit parsing is- removed from the computation time, these lookup operations are the- slowest operations keeping graph walks from being fast. Consider- loading these objects without verifying their existence in the ODB and- only loading them fully when consumers need them. Consider a method- such as "ensure_tree_loaded(commit)" that fully loads a tree before- using commit->tree.--- The current design uses the 'commit-graph' subcommand to generate the graph.- When this feature stabilizes enough to recommend to most users, we should- add automatic graph writes to common operations that create many commits.- For example, one could compute a graph on 'clone', 'fetch', or 'repack'- commands.- - A server could provide a commit graph file as part of the network protocol to avoid extra calculations by clients. This feature is only of benefit if the user is willing to trust the file, because verifying the file is correct
The commit-graph file ends with a SHA1 hash of the previous contents. If
a commit-graph file has errors but the checksum hash is correct, then we
know that the problem is a bug in Git and not simply file corruption
after-the-fact.
Compute the checksum right away so it is the first error that appears,
and make the message translatable since this error can be "corrected" by
a user by simply deleting the file and recomputing. The rest of the
errors are useful only to developers.
Be sure to continue checking the rest of the file data if the checksum
is wrong. This is important for our tests, as we break the checksum as
we modify bytes of the commit-graph file.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 16 ++++++++++++++--
t/t5318-commit-graph.sh | 6 ++++++
2 files changed, 20 insertions(+), 2 deletions(-)
@@ -872,6 +875,15 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)if(verify_commit_graph_error)returnverify_commit_graph_error;+devnull=open("/dev/null",O_WRONLY);+f=hashfd(devnull,NULL);+hashwrite(f,g->data,g->data_len-g->hash_len);+finalize_hashfile(f,checksum.hash,CSUM_CLOSE);+if(hashcmp(checksum.hash,g->data+g->data_len-g->hash_len)){+graph_report(_("the commit-graph file has incorrect checksum and is likely corrupt"));+verify_commit_graph_error=VERIFY_COMMIT_GRAPH_ERROR_HASH;+}+for(i=0;i<g->num_commits;i++){structcommit*graph_commit;
@@ -279,6 +279,7 @@ GRAPH_COMMIT_DATA_WIDTH=$(($HASH_LEN + 16))GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET+\$GRAPH_COMMIT_DATA_WIDTH*$NUM_COMMITS))GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET+4))+GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET+4*$NUM_OCTOPUS_EDGES))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
When writing commit-graph files, it can be convenient to ask for all
reachable commits (starting at the ref set) in the resulting file. This
is particularly helpful when writing to stdin is complicated, such as a
future integration with 'git gc'.
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/git-commit-graph.txt | 8 ++++++--
builtin/commit-graph.c | 16 ++++++++++++----
commit-graph.c | 18 ++++++++++++++++++
commit-graph.h | 1 +
t/t5318-commit-graph.sh | 10 ++++++++++
5 files changed, 47 insertions(+), 6 deletions(-)
@@ -38,12 +38,16 @@ Write a commit graph file based on the commits found in packfiles. + With the `--stdin-packs` option, generate the new commit graph by walking objects only in the specified pack-indexes. (Cannot be combined-with --stdin-commits.)+with `--stdin-commits` or `--reachable`.) + With the `--stdin-commits` option, generate the new commit graph by walking commits starting at the commits specified in stdin as a list of OIDs in hex, one OID per line. (Cannot be combined with---stdin-packs.)+`--stdin-packs` or `--reachable`.)+++With the `--reachable` option, generate the new commit graph by walking+commits starting at all refs. (Cannot be combined with `--stdin-commits`+or `--stdin-packs`.) + With the `--append` option, include all commits that are present in the existing commit-graph file.
@@ -127,6 +128,8 @@ static int graph_write(int argc, const char **argv)OPT_STRING(0,"object-dir",&opts.obj_dir,N_("dir"),N_("The object directory to store the graph")),+OPT_BOOL(0,"reachable",&opts.reachable,+N_("start walk at all refs")),OPT_BOOL(0,"stdin-packs",&opts.stdin_packs,N_("scan pack-indexes listed by stdin for commits")),OPT_BOOL(0,"stdin-commits",&opts.stdin_commits,
@@ -140,11 +143,16 @@ static int graph_write(int argc, const char **argv)builtin_commit_graph_write_options,builtin_commit_graph_write_usage,0);-if(opts.stdin_packs&&opts.stdin_commits)-die(_("cannot use both --stdin-commits and --stdin-packs"));+if(opts.reachable+opts.stdin_packs+opts.stdin_commits>1)+die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs"));if(!opts.obj_dir)opts.obj_dir=get_object_directory();+if(opts.reachable){+write_commit_graph_reachable(opts.obj_dir,opts.append);+return0;+}+string_list_init(&lines,0);if(opts.stdin_packs||opts.stdin_commits){structstrbufbuf=STRBUF_INIT;
The commit-graph file has an extra chunk to store the parent int-ids for
parents beyond the first parent for octopus merges. Our test repo has a
single octopus merge that we can manipulate to demonstrate the 'verify'
subcommand detects incorrect values in that chunk.
Signed-off-by: Derrick Stolee <redacted>
---
t/t5318-commit-graph.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
The commit-graph file is a very helpful feature for speeding up git
operations. In order to make it more useful, make it possible to
write the commit-graph file during standard garbage collection
operations.
Add a 'gc.commitGraph' config setting that triggers writing a
commit-graph file after any non-trivial 'git gc' command. Defaults to
false while the commit-graph feature matures. We specifically do not
want to have this on by default until the commit-graph feature is fully
integrated with history-modifying features like shallow clones.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/config.txt | 10 +++++++++-
Documentation/git-gc.txt | 4 ++++
builtin/gc.c | 6 ++++++
t/t5318-commit-graph.sh | 14 ++++++++++++++
4 files changed, 33 insertions(+), 1 deletion(-)
@@ -906,7 +906,8 @@ the `GIT_NOTES_REF` environment variable. See linkgit:git-notes[1]. core.commitGraph:: Enable git commit graph feature. Allows reading from the- commit-graph file.+ commit-graph file. See `gc.commitGraph` for automatically+ maintaining the file. core.sparseCheckout:: Enable "sparse checkout" feature. See section "Sparse checkout" in
@@ -1647,6 +1648,13 @@ this configuration variable is ignored, all packs except the base pack will be repacked. After this the number of packs should go below gc.autoPackLimit and gc.bigPackThreshold should be respected again.+gc.commitGraph::+ If true, then gc will rewrite the commit-graph file when+ linkgit:git-gc[1] is run. When using linkgit:git-gc[1]+ '--auto' the commit-graph will be updated if housekeeping is+ required. Default is false. See linkgit:git-commit-graph[1]+ for details.+ gc.logExpiry:: If the file gc.log exists, then `git gc --auto` won't run unless that file is more than 'gc.logExpiry' old. Default is
@@ -136,6 +136,10 @@ The optional configuration variable `gc.packRefs` determines if it within all non-bare repos or it can be set to a boolean value. This defaults to true.+The optional configuration variable `gc.commitGraph` determines if+'git gc' should run 'git commit-graph write'. This can be set to a+boolean value. This defaults to false.+ The optional configuration variable `gc.aggressiveWindow` controls how much time is spent optimizing the delta compression of the objects in the repository when the --aggressive option is specified. The larger
@@ -641,6 +644,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)if(pack_garbage.nr>0)clean_pack_garbage();+if(gc_commit_graph)+write_commit_graph_reachable(get_object_directory(),0);+if(auto_gc&&too_many_loose_objects())warning(_("There are too many unreachable loose objects; ""run 'git prune' to remove them."));
@@ -245,6 +245,20 @@ test_expect_success 'perform fast-forward merge in full repo' 'test_cmpexpectoutput'+test_expect_success'check that gc computes commit-graph''+cd"$TRASH_DIRECTORY/full"&&+gitcommit--allow-empty-m"blank"&&+gitcommit-graphwrite--reachable&&+cp$objdir/info/commit-graphcommit-graph-before-gc&&+gitreset--hardHEAD~1&&+gitconfiggc.commitGraphtrue&&+gitgc&&+cp$objdir/info/commit-graphcommit-graph-after-gc&&+!test_cmpcommit-graph-before-gccommit-graph-after-gc&&+gitcommit-graphwrite--reachable&&+test_cmpcommit-graph-after-gc$objdir/info/commit-graph+'+# the verify tests below expect the commit-graph to contain# exactly the commits reachable from the commits/8 branch.# If the file changes the set of commits in the list, then the
The 'verify' subcommand must compare the commit content parsed from the
commit-graph against the content in the object database. Use
lookup_commit() and parse_commit_in_graph_one() to parse the commits
from the graph and compare against a commit that is loaded separately
and parsed directly from the object database.
Add checks for the root tree OID.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 17 ++++++++++++++++-
t/t5318-commit-graph.sh | 7 +++++++
2 files changed, 23 insertions(+), 1 deletion(-)
@@ -883,6 +885,11 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)cur_fanout_pos++;}++graph_commit=lookup_commit(&cur_oid);+if(!parse_commit_in_graph_one(g,graph_commit))+graph_report("failed to parse %s from commit-graph",+oid_to_hex(&cur_oid));}while(cur_fanout_pos<256){
@@ -899,16 +906,24 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)returnverify_commit_graph_error;for(i=0;i<g->num_commits;i++){-structcommit*odb_commit;+structcommit*graph_commit,*odb_commit;hashcpy(cur_oid.hash,g->chunk_oid_lookup+g->hash_len*i);+graph_commit=lookup_commit(&cur_oid);odb_commit=(structcommit*)create_object(r,cur_oid.hash,alloc_commit_node(r));if(parse_commit_internal(odb_commit,0,0)){graph_report("failed to parse %s from object database",oid_to_hex(&cur_oid));continue;}++if(oidcmp(&get_commit_tree_in_graph_one(g,graph_commit)->object.oid,+get_commit_tree_oid(odb_commit)))+graph_report("root tree OID for commit %s in commit-graph is %s != %s",+oid_to_hex(&cur_oid),+oid_to_hex(get_commit_tree_oid(graph_commit)),+oid_to_hex(get_commit_tree_oid(odb_commit)));}returnverify_commit_graph_error;
@@ -267,6 +267,8 @@ GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET + 4 * 255))GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET+4*256))GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN*8))GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN*4+10))+GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET+$HASH_LEN*$NUM_COMMITS))+GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -341,4 +343,9 @@ test_expect_success 'detect OID not in object database' '"from object database"'+test_expect_success'detect incorrect tree OID''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_TREE"\01"\+"root tree OID for commit"+'+ test_done
@@ -986,6 +986,12 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)oid_to_hex(&cur_oid),graph_commit->generation,max_generation+1);++if(graph_commit->date!=odb_commit->date)+graph_report("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime,+oid_to_hex(&cur_oid),+graph_commit->date,+odb_commit->date);}returnverify_commit_graph_error;
@@ -273,6 +273,7 @@ GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN))GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+11))+GRAPH_BYTE_COMMIT_DATE=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+12))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
While iterating through the commit parents, perform the generation
number calculation and compare against the value stored in the
commit-graph.
The tests demonstrate that having a different set of parents affects
the generation number calculation, and this value propagates to
descendants. Hence, we drop the single-line condition on the output.
Since Git will ship with the commit-graph feature without generation
numbers, we need to accept commit-graphs with all generation numbers
equal to zero. In this case, ignore the generation number calculation.
However, verify that we should never have a mix of zero and non-zero
generation numbers. Create a test that sets one commit to generation
zero and all following commits report a failure as they have non-zero
generation in a file that contains generation number zero.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 34 ++++++++++++++++++++++++++++++++++
t/t5318-commit-graph.sh | 11 +++++++++++
2 files changed, 45 insertions(+)
@@ -952,6 +960,32 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)if(odb_parents!=NULL)graph_report("commit-graph parent list for commit %s terminates early",oid_to_hex(&cur_oid));++if(!graph_commit->generation){+if(generation_zero==GENERATION_NUMBER_EXISTS)+graph_report("commit-graph has generation number zero for commit %s, but non-zero elsewhere",+oid_to_hex(&cur_oid));+generation_zero=GENERATION_ZERO_EXISTS;+}elseif(generation_zero==GENERATION_ZERO_EXISTS)+graph_report("commit-graph has non-zero generation number for commit %s, but zero elsewhere",+oid_to_hex(&cur_oid));++if(generation_zero==GENERATION_ZERO_EXISTS)+continue;++/*+*IfoneofourparentshasgenerationGENERATION_NUMBER_MAX,then+*ourgenerationisalsoGENERATION_NUMBER_MAX.Decrementtoavoid+*extralogicinthefollowingcondition.+*/+if(max_generation==GENERATION_NUMBER_MAX)+max_generation--;++if(graph_commit->generation!=max_generation+1)+graph_report("commit-graph generation for commit %s is %u != %u",+oid_to_hex(&cur_oid),+graph_commit->generation,+max_generation+1);}returnverify_commit_graph_error;
@@ -272,6 +272,7 @@ GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSETGRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN))GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+4))GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+3))+GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET+$HASH_LEN+11))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
The commit-graph file requires the following three chunks:
* OID Fanout
* OID Lookup
* Commit Data
If any of these are missing, then the 'verify' subcommand should
report a failure. This includes the chunk IDs malformed or the
chunk count is truncated.
Signed-off-by: Derrick Stolee <redacted>
---
commit-graph.c | 9 +++++++++
t/t5318-commit-graph.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
@@ -848,5 +848,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)return1;}+verify_commit_graph_error=0;++if(!g->chunk_oid_fanout)+graph_report("commit-graph is missing the OID Fanout chunk");+if(!g->chunk_oid_lookup)+graph_report("commit-graph is missing the OID Lookup chunk");+if(!g->chunk_commit_data)+graph_report("commit-graph is missing the Commit Data chunk");+returnverify_commit_graph_error;}
@@ -249,6 +249,15 @@ test_expect_success 'git commit-graph verify' 'GRAPH_BYTE_VERSION=4GRAPH_BYTE_HASH=5+GRAPH_BYTE_CHUNK_COUNT=6+GRAPH_CHUNK_LOOKUP_OFFSET=8+GRAPH_CHUNK_LOOKUP_WIDTH=12+GRAPH_CHUNK_LOOKUP_ROWS=5+GRAPH_BYTE_OID_FANOUT_ID=$GRAPH_CHUNK_LOOKUP_OFFSET+GRAPH_BYTE_OID_LOOKUP_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+1*$GRAPH_CHUNK_LOOKUP_WIDTH))+GRAPH_BYTE_COMMIT_DATA_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET+\+2*$GRAPH_CHUNK_LOOKUP_WIDTH))# usage: corrupt_graph_and_verify <position> <data> <string># Manipulates the commit-graph file at the position
@@ -283,4 +292,24 @@ test_expect_success 'detect bad hash version' '"hash version"'+test_expect_success'detect low chunk count''+corrupt_graph_and_verify$GRAPH_BYTE_CHUNK_COUNT"\02"\+"missing the .* chunk"+'++test_expect_success'detect missing OID fanout chunk''+corrupt_graph_and_verify$GRAPH_BYTE_OID_FANOUT_ID"\0"\+"missing the OID Fanout chunk"+'++test_expect_success'detect missing OID lookup chunk''+corrupt_graph_and_verify$GRAPH_BYTE_OID_LOOKUP_ID"\0"\+"missing the OID Lookup chunk"+'++test_expect_success'detect missing commit data chunk''+corrupt_graph_and_verify$GRAPH_BYTE_COMMIT_DATA_ID"\0"\+"missing the Commit Data chunk"+'+ test_done