Derrick Stolee <dsto...@microsoft.com> writes:
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.
All right, so we have one config variable to control the use of
serialized commit-graph feaature. Nice.
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.
All right, I can agree with that.
On the other hand using subcommand makes debugging harder, though not in
this case (well separated functionality that can be easily called with a
standalone command to be debugged).
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.
I don't quite understand this. Could you explain it in more detail?
Signed-off-by: Derrick Stolee <dsto...@microsoft.com>
---
Documentation/git-fsck.txt | 3 +++
builtin/fsck.c | 21 +++++++++++++++++++++
t/t5318-commit-graph.sh | 8 ++++++++
3 files changed, 32 insertions(+)
diff --git a/Documentation/git-fsck.txt b/Documentation/git-fsck.txt
index b9f060e3b2..ab9a93fb9b 100644
--- a/Documentation/git-fsck.txt
+++ b/Documentation/git-fsck.txt
@@ -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
Shouldn't we use `core.commitGraph` here?
+using 'git commit-graph verify'. See linkgit:git-commit-graph[1].
+
Extracted Diagnostics
---------------------
diff --git a/builtin/fsck.c b/builtin/fsck.c
index ef78c6c00c..a6d5045b77 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -16,6 +16,7 @@
#include "streaming.h"
#include "decorate.h"
#include "packfile.h"
+#include "run-command.h"
#define REACHABLE 0x0001
#define SEEN 0x0002
@@ -45,6 +46,7 @@ static int name_objects;
#define ERROR_REACHABLE 02
#define ERROR_PACK 04
#define ERROR_REFS 010
+#define ERROR_COMMIT_GRAPH 020
Minor nitpick and a sidenote: I wonder if it wouldn't be better to
either use hexadecimal constants, or use (1 << n) for all ERROR_*
preprocesor constants.
static const char *describe_object(struct object *obj)
{
@@ -815,5 +817,24 @@ int cmd_fsck(int argc, const char **argv, const char
*prefix)
}
check_connectivity();
+
+ if (core_commit_graph) {
+ struct child_process commit_graph_verify = CHILD_PROCESS_INIT;
+ const char *verify_argv[] = { "commit-graph", "verify", NULL,
NULL, NULL, NULL };
I see that NULL at index 2 and 3 (at 3rd and 4th place) are here for
"--object-dir" and <alternates-object-dir-path>, the last one is
terminator for that case, but what is next to last NULL (at 5th place)
for?
+ commit_graph_verify.argv = verify_argv;
+ commit_graph_verify.git_cmd = 1;
+
+ if (run_command(&commit_graph_verify))
+ errors_found |= ERROR_COMMIT_GRAPH;
+
+ 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;
+ }
+ }
For performance reasons it may be better to start those 'git
commit-graph verify' commands asynchronously earlier, so that they can
run in parallel / concurrently wth other checks, and wait for them and
get their error code at the end of git-fsck run.
But that is probably better left for a separate commit.
+
return errors_found;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 2680a2ebff..4941937163 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -394,4 +394,12 @@ test_expect_success 'detect invalid checksum hash' '
"incorrect checksum"
'
+test_expect_success 'git fsck (checks commit-graph)' '
+ cd "$TRASH_DIRECTORY/full" &&
+ git fsck &&
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum" &&
+ test_must_fail git fsck
+'
All right; though the same caveats apply as with previous commit in
series. Perhaps it would be better to truncate commit-graph file, or
corrupt it in some 'random' place.
+
test_done
Best,
