Re: [PATCH v3 0/2] commit-graph: detect commits missing in ODB

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

>> Thanks, the range-diff here looks exactly as expected. Thanks for
>> working on this, this version LGTM.
>
> OK, I'd like a version as incremental to v2 (since it already is in
> 'next') that results in the same tree state as v3 then.
>
> Thanks for working on it, and reviewing it.

In the meantime, here is a mechanically produced incremental I'll
tentatively queue.  Hopefully I did not screw up while generating
it.

Thanks.

--- >8 ---
From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 31 Oct 2023 08:16:09 +0100
Subject: [PATCH] commit-graph: clarify GIT_COMMIT_GRAPH_PARANOIA documentation

In response to reviews of the previous round that has already hit
'next', clarify the help text for GIT_COMMIT_GRAPH_PARANOIA and
rename object_paranoia variable to commit_graph_paranoia for
consistency.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git.txt   | 15 ++++++++-------
 commit-graph.c          |  8 ++++----
 commit.c                |  8 ++++----
 t/t5318-commit-graph.sh |  2 +-
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 22c2b537aa..3bac24cf8a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -912,13 +912,14 @@ for full details.
 	useful when trying to salvage data from a corrupted repository.
 
 `GIT_COMMIT_GRAPH_PARANOIA`::
-	If this Boolean environment variable is set to false, ignore the
-	case where commits exist in the commit graph but not in the
-	object database. Normally, Git will check whether commits loaded
-	from the commit graph exist in the object database to avoid
-	issues with stale commit graphs, but this check comes with a
-	performance penalty. The default is `1` (i.e., be paranoid about
-	stale commits in the commit graph).
+	When loading a commit object from the commit-graph, Git performs an
+	existence check on the object in the object database. This is done to
+	avoid issues with stale commit-graphs that contain references to
+	already-deleted commits, but comes with a performance penalty.
++
+The default is "true", which enables the aforementioned behavior.
+Setting this to "false" disables the existence check. This can lead to
+a performance improvement at the cost of consistency.
 
 `GIT_ALLOW_PROTOCOL`::
 	If set to a colon-separated list of protocols, behave as if
diff --git a/commit-graph.c b/commit-graph.c
index 376f59af73..b37fdcb214 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -907,18 +907,18 @@ int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,
 
 struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)
 {
-	static int object_paranoia = -1;
+	static int commit_graph_paranoia = -1;
 	struct commit *commit;
 	uint32_t pos;
 
-	if (object_paranoia == -1)
-		object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+	if (commit_graph_paranoia == -1)
+		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
 
 	if (!prepare_commit_graph(repo))
 		return NULL;
 	if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))
 		return NULL;
-	if (object_paranoia && !has_object(repo, id, 0))
+	if (commit_graph_paranoia && !has_object(repo, id, 0))
 		return NULL;
 
 	commit = lookup_commit(repo, id);
diff --git a/commit.c b/commit.c
index 7399e90212..8405d7c3fc 100644
--- a/commit.c
+++ b/commit.c
@@ -574,12 +574,12 @@ int repo_parse_commit_internal(struct repository *r,
 	if (item->object.parsed)
 		return 0;
 	if (use_commit_graph && parse_commit_in_graph(r, item)) {
-		static int object_paranoia = -1;
+		static int commit_graph_paranoia = -1;
 
-		if (object_paranoia == -1)
-			object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+		if (commit_graph_paranoia == -1)
+			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
 
-		if (object_paranoia && !has_object(r, &item->object.oid, 0)) {
+		if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
 			unparse_commit(r, &item->object.oid);
 			return quiet_on_missing ? -1 :
 				error(_("commit %s exists in commit-graph but not in the object database"),
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 55e3c7ec78..2c62b91ef9 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -847,7 +847,7 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
 		test_commit C &&
 		git commit-graph write --reachable &&
 
-		# Corrupt the repository by deleting the intermittent commit
+		# Corrupt the repository by deleting the intermediate commit
 		# object. Commands should notice that this object is absent and
 		# thus that the repository is corrupt even if the commit graph
 		# exists.
-- 
2.42.0-530-g692be87cbb