When the commit-graph is written we end up calling
parse_commit(). This will in turn invoke code that'll consult the
existing commit-graph about the commit, if the graph is corrupted we
die.
We thus get into a state where a failing "commit-graph verify" can't
be followed-up with a "commit-graph write" if core.commitGraph=true is
set, the graph either needs to be manually removed to proceed, or
core.commitGraph needs to be set to "false".
Change the "commit-graph write" codepath to use a new
parse_commit_no_graph() helper instead of parse_commit() to avoid
this. The latter will call repo_parse_commit_internal() with
use_commit_graph=1 as seen in 177722b344 ("commit: integrate commit
graph with commit parsing", 2018-04-10).
Just fixing the current issue would be likely to result in code that's
inadvertently broken in the future. New code might use the
commit-graph at a distance. To detect such cases introduce a
"GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD" setting used when we do our
corruption tests, and test that a "write/verify" combo works after
every one of our current test cases where we now detect commit-graph
corruption.
Some of the code changes here might be strictly unnecessary, e.g. I
was unable to find cases where the parse_commit() called from
write_graph_chunk_data() didn't exit early due to
"item->object.parsed" being true in
repo_parse_commit_internal() (before the use_commit_graph=1 has any
effect). But let's also convert those cases for good measure, we do
not have exhaustive tests for all possible types of commit-graph
corruption.
This might need to be re-visited if we learn to write the commit-graph
incrementally.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
---
commit-graph.c | 10 +++++++---
commit-graph.h | 1 +
commit.h | 6 ++++++
t/t5318-commit-graph.sh | 11 +++++++++--
4 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index d945e8f3e0..6b3ade9496 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -281,6 +281,10 @@ static int prepare_commit_graph(struct repository *r)
struct object_directory *odb;
int config_value;
+ if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))
+ die("Dying as requested by the '%s' variable on commit-graph load!",
+ GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);
+
if (r->objects->commit_graph_attempted)
return !!r->objects->commit_graph;
r->objects->commit_graph_attempted = 1;
@@ -545,7 +549,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
uint32_t packedDate[2];
display_progress(progress, ++*progress_cnt);
- parse_commit(*list);
+ parse_commit_no_graph(*list);
hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
parent = (*list)->parents;
@@ -742,7 +746,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)
display_progress(progress, i + 1);
commit = lookup_commit(the_repository, &oids->list[i]);
- if (commit && !parse_commit(commit))
+ if (commit && !parse_commit_no_graph(commit))
add_missing_parents(oids, commit);
}
stop_progress(&progress);
@@ -991,7 +995,7 @@ void write_commit_graph(const char *obj_dir,
continue;
commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);
- parse_commit(commits.list[commits.nr]);
+ parse_commit_no_graph(commits.list[commits.nr]);
for (parent = commits.list[commits.nr]->parents;
parent; parent = parent->next)
diff --git a/commit-graph.h b/commit-graph.h
index 36d8109901..6021ababa2 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -7,6 +7,7 @@
#include "cache.h"
#define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
+#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
struct commit;
diff --git a/commit.h b/commit.h
index 42728c2906..5d33477e78 100644
--- a/commit.h
+++ b/commit.h
@@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)
{
return repo_parse_commit_gently(r, item, 0);
}
+
+static inline int parse_commit_no_graph(struct commit *commit)
+{
+ return repo_parse_commit_internal(the_repository, commit, 0, 0);
+}
+
#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
#define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)
#define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 1ee00fa333..6db658ed66 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -377,7 +377,13 @@ corrupt_graph_verify() {
test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "$grepstr" err &&
- git status --short
+ if test -z "$NO_WRITE_TEST_BACKUP"
+ then
+ cp $objdir/info/commit-graph commit-graph-pre-write-test
+ fi &&
+ git status --short &&
+ GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
+ git commit-graph verify
}
# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
@@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' '
# "chmod 000 file" does not yield EACCES on e.g. "cat file"
if ! test -r $objdir/info/commit-graph
then
- corrupt_graph_verify "Could not open"
+ NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open"
fi
'
@@ -528,6 +534,7 @@ test_expect_success 'git fsck (checks commit-graph)' '
git fsck &&
corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
"incorrect checksum" &&
+ cp commit-graph-pre-write-test $objdir/info/commit-graph &&
test_must_fail git fsck
'
--
2.21.0.rc0.258.g878e2cd30e
