[PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6604 bytes --]

In 7a5d604443 (commit: detect commits that exist in commit-graph but not
in the ODB, 2023-10-31), we have introduced a new object existence check
into `repo_parse_commit_internal()` so that we do not parse commits via
the commit-graph that don't have a corresponding object in the object
database. This new check of course comes with a performance penalty,
which the commit put at around 30% for `git rev-list --topo-order`. But
there are in fact scenarios where the performance regression is even
higher. The following benchmark against linux.git with a fully-build
commit-graph:

  Benchmark 1: git.v2.42.1 rev-list --count HEAD
    Time (mean ± σ):     658.0 ms ±   5.2 ms    [User: 613.5 ms, System: 44.4 ms]
    Range (min … max):   650.2 ms … 666.0 ms    10 runs

  Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
    Time (mean ± σ):      1.333 s ±  0.019 s    [User: 1.263 s, System: 0.069 s]
    Range (min … max):    1.302 s …  1.361 s    10 runs

  Summary
    git.v2.42.1 rev-list --count HEAD ran
      2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD

While it's a noble goal to ensure that results are the same regardless
of whether or not we have a potentially stale commit-graph, taking twice
as much time is a tough sell. Furthermore, we can generally assume that
the commit-graph will be updated by git-gc(1) or git-maintenance(1) as
required so that the case where the commit-graph is stale should not at
all be common.

With that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore
the behaviour and thus performance previous to the mentioned commit. In
order to not be inconsistent, also disable this behaviour by default in
`lookup_commit_in_graph()`, where the object existence check has been
introduced right at its inception via f559d6d45e (revision: avoid
hitting packfiles when commits are in commit-graph, 2021-08-09).

This results in another speedup in commands that end up calling this
function, even though it's less pronounced compared to the above
benchmark. The following has been executed in linux.git with ~1.2
million references:

  Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
    Time (mean ± σ):      2.947 s ±  0.003 s    [User: 2.412 s, System: 0.534 s]
    Range (min … max):    2.943 s …  2.949 s    3 runs

  Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted
    Time (mean ± σ):      2.724 s ±  0.030 s    [User: 2.207 s, System: 0.514 s]
    Range (min … max):    2.704 s …  2.759 s    3 runs

  Summary
    GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran
      1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted

So whereas 7a5d604443 initially introduced the logic to start doing an
object existence check in `repo_parse_commit_internal()` by default, the
updated logic will now instead cause `lookup_commit_in_graph()` to stop
doing the check by default. This behaviour continues to be tweakable by
the user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git.txt   | 6 +++---
 commit-graph.c          | 2 +-
 commit.c                | 2 +-
 t/t5318-commit-graph.sh | 8 ++++----
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2535a30194..6c19fd1d76 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -917,9 +917,9 @@ for full details.
 	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.
+The default is "false", which disables the aforementioned behavior.
+Setting this to "true" enables the existence check so that stale commits
+will never be returned from the commit-graph at the cost of performance.
 
 `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 ee66098e07..a712917356 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1029,7 +1029,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje
 	uint32_t pos;
 
 	if (commit_graph_paranoia == -1)
-		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
 
 	if (!prepare_commit_graph(repo))
 		return NULL;
diff --git a/commit.c b/commit.c
index 8405d7c3fc..37956b836c 100644
--- a/commit.c
+++ b/commit.c
@@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,
 		static int commit_graph_paranoia = -1;
 
 		if (commit_graph_paranoia == -1)
-			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
 
 		if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
 			unparse_commit(r, &item->object.oid);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d4fc65e078..4c751a6871 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -909,10 +909,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
 
 		# Verify that it is possible to read the commit from the
 		# commit graph when not being paranoid, ...
-		GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&
+		git rev-list B &&
 		# ... but parsing the commit when double checking that
 		# it actually exists in the object database should fail.
-		test_must_fail git rev-list -1 B
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B
 	)
 '
 
@@ -936,9 +936,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
 
 		# Again, we should be able to parse the commit when not
 		# being paranoid about commit graph staleness...
-		GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
+		git rev-parse HEAD~2 &&
 		# ... but fail when we are paranoid.
-		test_must_fail git rev-parse HEAD~2 2>error &&
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&
 		grep "error: commit $oid exists in commit-graph but not in the object database" error
 	)
 '
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 8759 bytes --]

On Tue, Nov 14, 2023 at 11:23:05AM +0100, Patrick Steinhardt wrote:
> In 7a5d604443 (commit: detect commits that exist in commit-graph but not
> in the ODB, 2023-10-31), we have introduced a new object existence check
> into `repo_parse_commit_internal()` so that we do not parse commits via
> the commit-graph that don't have a corresponding object in the object
> database. This new check of course comes with a performance penalty,
> which the commit put at around 30% for `git rev-list --topo-order`. But
> there are in fact scenarios where the performance regression is even
> higher. The following benchmark against linux.git with a fully-build
> commit-graph:
> 
>   Benchmark 1: git.v2.42.1 rev-list --count HEAD
>     Time (mean ± σ):     658.0 ms ±   5.2 ms    [User: 613.5 ms, System: 44.4 ms]
>     Range (min … max):   650.2 ms … 666.0 ms    10 runs
> 
>   Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
>     Time (mean ± σ):      1.333 s ±  0.019 s    [User: 1.263 s, System: 0.069 s]
>     Range (min … max):    1.302 s …  1.361 s    10 runs
> 
>   Summary
>     git.v2.42.1 rev-list --count HEAD ran
>       2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD
> 
> While it's a noble goal to ensure that results are the same regardless
> of whether or not we have a potentially stale commit-graph, taking twice
> as much time is a tough sell. Furthermore, we can generally assume that
> the commit-graph will be updated by git-gc(1) or git-maintenance(1) as
> required so that the case where the commit-graph is stale should not at
> all be common.
> 
> With that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore
> the behaviour and thus performance previous to the mentioned commit. In
> order to not be inconsistent, also disable this behaviour by default in
> `lookup_commit_in_graph()`, where the object existence check has been
> introduced right at its inception via f559d6d45e (revision: avoid
> hitting packfiles when commits are in commit-graph, 2021-08-09).
> 
> This results in another speedup in commands that end up calling this
> function, even though it's less pronounced compared to the above
> benchmark. The following has been executed in linux.git with ~1.2
> million references:
> 
>   Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
>     Time (mean ± σ):      2.947 s ±  0.003 s    [User: 2.412 s, System: 0.534 s]
>     Range (min … max):    2.943 s …  2.949 s    3 runs
> 
>   Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted
>     Time (mean ± σ):      2.724 s ±  0.030 s    [User: 2.207 s, System: 0.514 s]
>     Range (min … max):    2.704 s …  2.759 s    3 runs
> 
>   Summary
>     GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran
>       1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
> 
> So whereas 7a5d604443 initially introduced the logic to start doing an
> object existence check in `repo_parse_commit_internal()` by default, the
> updated logic will now instead cause `lookup_commit_in_graph()` to stop
> doing the check by default. This behaviour continues to be tweakable by
> the user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.
> 
> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending
this patch. There are two test failures that this change introduces:

  - t6022-rev-list-missing.sh, where we test for the `--missing=` option
    of git-rev-list(1).

  - t7700-repack.sh, where we also manually delete objects.

Both of these are expected failures: we knowingly corrupt the repository
and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
updated. If we stick with the new stance that repository corruption
should not require us to pessimize the common case, then we'd have to
squash in something like the below.

Patrick

diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 40265a4f66..9e3f063d08 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -27,6 +27,12 @@ do
 	'
 done
 
+# When running with GIT_TEST_COMMIT_GRAPH=true we write a commit-graph, but
+# don't update it before forcefully deleting the commit object. We thus enable
+# GIT_COMMIT_GRAPH_PARANOIA so that this case is detected with such a stale
+# commit-graph.
+export GIT_COMMIT_GRAPH_PARANOIA=true
+
 for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
 do
 	for action in "allow-any" "print"
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index d2975e6c93..ca8ad9c420 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -271,7 +271,7 @@ test_expect_success 'repacking fails when missing .pack actually means missing o
 		ls .git/objects/pack/*.pack >before-pack-dir &&
 
 		test_must_fail git fsck &&
-		test_must_fail git repack --cruft -d 2>err &&
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=1 git repack --cruft -d 2>err &&
 		grep "bad object" err &&
 
 		# Before failing, the repack did not modify the

> ---
>  Documentation/git.txt   | 6 +++---
>  commit-graph.c          | 2 +-
>  commit.c                | 2 +-
>  t/t5318-commit-graph.sh | 8 ++++----
>  4 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 2535a30194..6c19fd1d76 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -917,9 +917,9 @@ for full details.
>  	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.
> +The default is "false", which disables the aforementioned behavior.
> +Setting this to "true" enables the existence check so that stale commits
> +will never be returned from the commit-graph at the cost of performance.
>  
>  `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 ee66098e07..a712917356 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1029,7 +1029,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje
>  	uint32_t pos;
>  
>  	if (commit_graph_paranoia == -1)
> -		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> +		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
>  
>  	if (!prepare_commit_graph(repo))
>  		return NULL;
> diff --git a/commit.c b/commit.c
> index 8405d7c3fc..37956b836c 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,
>  		static int commit_graph_paranoia = -1;
>  
>  		if (commit_graph_paranoia == -1)
> -			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> +			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
>  
>  		if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
>  			unparse_commit(r, &item->object.oid);
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index d4fc65e078..4c751a6871 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -909,10 +909,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
>  
>  		# Verify that it is possible to read the commit from the
>  		# commit graph when not being paranoid, ...
> -		GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&
> +		git rev-list B &&
>  		# ... but parsing the commit when double checking that
>  		# it actually exists in the object database should fail.
> -		test_must_fail git rev-list -1 B
> +		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B
>  	)
>  '
>  
> @@ -936,9 +936,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
>  
>  		# Again, we should be able to parse the commit when not
>  		# being paranoid about commit graph staleness...
> -		GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
> +		git rev-parse HEAD~2 &&
>  		# ... but fail when we are paranoid.
> -		test_must_fail git rev-parse HEAD~2 2>error &&
> +		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&
>  		grep "error: commit $oid exists in commit-graph but not in the object database" error
>  	)
>  '
> -- 
> 2.42.0
> 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Taylor Blau]

On Tue, Nov 14, 2023 at 11:35:08AM +0100, Patrick Steinhardt wrote:
> Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending
> this patch. There are two test failures that this change introduces:
>
>   - t6022-rev-list-missing.sh, where we test for the `--missing=` option
>     of git-rev-list(1).
>
>   - t7700-repack.sh, where we also manually delete objects.
>
> Both of these are expected failures: we knowingly corrupt the repository
> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
> updated. If we stick with the new stance that repository corruption
> should not require us to pessimize the common case, then we'd have to
> squash in something like the below.

Good catch. Thanks for investigating and providing a thoughtful analysis
of why we should turn off GIT_COMMIT_GRAPH_PARANOIA by default. I agree
with your conclusion (with this follow-up patch squashed in, of course).

My hunch is that it would be easier for the maintainer to have a single
patch to queue instead of squashing this in themself. You may want to
send a "v2" to that effect.

Thanks,
Taylor

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending
> this patch. There are two test failures that this change introduces:
>
>   - t6022-rev-list-missing.sh, where we test for the `--missing=` option
>     of git-rev-list(1).

I would have expected you to enable the paranoia mode automatically
when this option is in effect.

> Both of these are expected failures: we knowingly corrupt the repository
> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
> updated. If we stick with the new stance that repository corruption
> should not require us to pessimize the common case,...

Yeah, just like we try to be extra careful while running fsck,
because "--missing" is about finding these "corrupt" cases,
triggering the paranoia mode upon seeing the option would make
sense, no?  It would fix the failure in 6022, right?

Thanks for working on this.

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Junio C Hamano]

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

> Patrick Steinhardt <ps@pks.im> writes:
>
>> Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending
>> this patch. There are two test failures that this change introduces:
>>
>>   - t6022-rev-list-missing.sh, where we test for the `--missing=` option
>>     of git-rev-list(1).
>
> I would have expected you to enable the paranoia mode automatically
> when this option is in effect.
>
>> Both of these are expected failures: we knowingly corrupt the repository
>> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
>> updated. If we stick with the new stance that repository corruption
>> should not require us to pessimize the common case,...
>
> Yeah, just like we try to be extra careful while running fsck,
> because "--missing" is about finding these "corrupt" cases,
> triggering the paranoia mode upon seeing the option would make
> sense, no?  It would fix the failure in 6022, right?
>
> Thanks for working on this.

Just to make sure we do not miscommunicate, I do not think we want
to trigger the paranoia mode only in our tests.  We want to be
paranoid to help real users who used "--missing" for their real use,
so enabling PARANOIA in the test script is a wrong approach.  We
should enable it inside "rev-list --missing" codepath.

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Jeff King]

On Wed, Nov 15, 2023 at 01:51:43AM +0900, Junio C Hamano wrote:

> >> Both of these are expected failures: we knowingly corrupt the repository
> >> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
> >> updated. If we stick with the new stance that repository corruption
> >> should not require us to pessimize the common case,...
> >
> > Yeah, just like we try to be extra careful while running fsck,
> > because "--missing" is about finding these "corrupt" cases,
> > triggering the paranoia mode upon seeing the option would make
> > sense, no?  It would fix the failure in 6022, right?
> >
> > Thanks for working on this.
> 
> Just to make sure we do not miscommunicate, I do not think we want
> to trigger the paranoia mode only in our tests.  We want to be
> paranoid to help real users who used "--missing" for their real use,
> so enabling PARANOIA in the test script is a wrong approach.  We
> should enable it inside "rev-list --missing" codepath.

Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I
think we should do the same here.

As we are closing in on the v2.43 release, there's one thing I'm not
sure about regarding release planning. Are these test cases that _used_
to detect the corruption, but now don't? I.e., I would have expected
that disabling GIT_COMMIT_GRAPH_PARANOIA would take us back to the same
state as v2.42. But I think it doesn't because of the hunk in e04838ea82
(commit-graph: introduce envvar to disable commit existence checks,
2023-10-31) that makes the has_object() call conditional (and now
defaults to off).

What I'm getting as it that I think we have three options for v2.43:

  1. Ship what has been in the release candidates, which has a known
     performance regression (though the severity is up for debate).

  2. Flip the default to "0" (i.e., Patrick's patch in this thread). We
     know that loosens some cases versus 2.42, which may be considered a
     regression.

  3. Sort it out before the release. We're getting pretty close to do
     a lot new work there, but I think the changes should be small-ish.
     The nuclear option is ejecting the topic and re-doing it in the
     next cycle.

I don't have a really strong preference between the three.

-Peff

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> What I'm getting as it that I think we have three options for v2.43:
>
>   1. Ship what has been in the release candidates, which has a known
>      performance regression (though the severity is up for debate).
>
>   2. Flip the default to "0" (i.e., Patrick's patch in this thread). We
>      know that loosens some cases versus 2.42, which may be considered a
>      regression.
>
>   3. Sort it out before the release. We're getting pretty close to do
>      a lot new work there, but I think the changes should be small-ish.
>      The nuclear option is ejecting the topic and re-doing it in the
>      next cycle.
>
> I don't have a really strong preference between the three.

I've been (naively) assuming that #1 is everybody's preference,
simply because #2 does introduce a regression in the correctness
department (as opposed to a possible performance regression caused
by #1), and because #3 has a high risk of screwing up.

As long as the performance regression is known and on our radar,
I'd say that working on a maintenance release after Thanksgiving
would be sufficient.

I might be underestimating the impact of the loss of performance,
though, in which case I'd consider that nuclear one, which is the
simplest and least risky.

Thanks.

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Jeff King]

On Wed, Nov 15, 2023 at 09:44:38AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > What I'm getting as it that I think we have three options for v2.43:
> >
> >   1. Ship what has been in the release candidates, which has a known
> >      performance regression (though the severity is up for debate).
> >
> >   2. Flip the default to "0" (i.e., Patrick's patch in this thread). We
> >      know that loosens some cases versus 2.42, which may be considered a
> >      regression.
> >
> >   3. Sort it out before the release. We're getting pretty close to do
> >      a lot new work there, but I think the changes should be small-ish.
> >      The nuclear option is ejecting the topic and re-doing it in the
> >      next cycle.
> >
> > I don't have a really strong preference between the three.
> 
> I've been (naively) assuming that #1 is everybody's preference,
> simply because #2 does introduce a regression in the correctness
> department (as opposed to a possible performance regression caused
> by #1), and because #3 has a high risk of screwing up.
> 
> As long as the performance regression is known and on our radar,
> I'd say that working on a maintenance release after Thanksgiving
> would be sufficient.
> 
> I might be underestimating the impact of the loss of performance,
> though, in which case I'd consider that nuclear one, which is the
> simplest and least risky.

I am fine with #1 for the release. Mostly I just wanted to understand
what the plan was (and if we needed to be hurrying to try to make the
non-nuclear #3 work).

-Peff

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5369 bytes --]

On Tue, Nov 14, 2023 at 02:43:10PM -0500, Jeff King wrote:
> On Wed, Nov 15, 2023 at 01:51:43AM +0900, Junio C Hamano wrote:
> 
> > >> Both of these are expected failures: we knowingly corrupt the repository
> > >> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
> > >> updated. If we stick with the new stance that repository corruption
> > >> should not require us to pessimize the common case,...
> > >
> > > Yeah, just like we try to be extra careful while running fsck,
> > > because "--missing" is about finding these "corrupt" cases,
> > > triggering the paranoia mode upon seeing the option would make
> > > sense, no?  It would fix the failure in 6022, right?
> > >
> > > Thanks for working on this.
> > 
> > Just to make sure we do not miscommunicate, I do not think we want
> > to trigger the paranoia mode only in our tests.  We want to be
> > paranoid to help real users who used "--missing" for their real use,
> > so enabling PARANOIA in the test script is a wrong approach.  We
> > should enable it inside "rev-list --missing" codepath.
> 
> Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I
> think we should do the same here.

I'm honestly still torn on this one. There are two cases that I can
think of where missing objects would be benign and where one wants to
use `git rev-list --missing`:

    - Repositories with promisor remotes, to find the boundary of where
      we need to fetch new objects.

    - Quarantine directories where you only intend to list new objects
      or find the boundary.

And in neither of those cases I can see a path for how the commit-graph
would contain such missing commits when using regular tooling to perform
repository maintenance.

So I'm still not sure why we think that this case is so much more
special than others. If a user wants to check for repository corruption
the tool shouldn't be `git rev-list --missing`, but git-fsck(1). To me,
the former is only useful in very specific circumstances where the user
knows what they are doing, and in none of the usecases I can think of
should we have a stale commit-graph _unless_ we have actual repository
corruption.

In reverse, to me this means that `--missing` is no more special than
any of the other low-level tooling, where our stance seems to be "We
assume that the repository is not corrupt". In that spirit, I'd argue
that the same default value should apply here as for all the other
cases.

But based on the discussion it very much feels like I'm alone with this
train of thought, which may indicate that I'm missing a quintessential
part of your arguments. May just as well be that I'm too focussed on the
usecases we at GitLab have for the new `--missing` behaviour around
commits that Karthik has just introduced.

Oh, well. I'll wait for answers to this reply until tomorrow, and if I
still haven't been able to convince anybody then I'll assume it's just
me and adapt accordingly :)

> As we are closing in on the v2.43 release, there's one thing I'm not
> sure about regarding release planning. Are these test cases that _used_
> to detect the corruption, but now don't? I.e., I would have expected
> that disabling GIT_COMMIT_GRAPH_PARANOIA would take us back to the same
> state as v2.42. But I think it doesn't because of the hunk in e04838ea82
> (commit-graph: introduce envvar to disable commit existence checks,
> 2023-10-31) that makes the has_object() call conditional (and now
> defaults to off).
> 
> What I'm getting as it that I think we have three options for v2.43:
> 
>   1. Ship what has been in the release candidates, which has a known
>      performance regression (though the severity is up for debate).

This seems like the best option for now in my opinion. The new behaviour
is not a bug, quite on the contrary, even though it is slower.

As Junio once said, we are not a "performance is king" project [1]. This
has burnt itself into my mind, and funny enough it was in the vicinity
of the change where I originally introduced the other object existence
check into `lookup_commit_in_graph()`.

[1]: <xmqqr1i1t6zl.fsf@gitster.g>

>   2. Flip the default to "0" (i.e., Patrick's patch in this thread). We
>      know that loosens some cases versus 2.42, which may be considered a
>      regression.

If we consider this to be a regression then I'd rather want to drop this
patch completely and leave it be. Ultimately, the question is how much
we trust our tooling to keep the commit-graph up-to-date, and whether or
not we need to account for corrupted repositories.

I for myself do trust the tooling, otherwise I wouldn't have sent this
patch. But I'm also happy to accept the current status where we are
being more thorough at the cost of performance.

>   3. Sort it out before the release. We're getting pretty close to do
>      a lot new work there, but I think the changes should be small-ish.
>      The nuclear option is ejecting the topic and re-doing it in the
>      next cycle.

I would be comfortable with this option if we simply switch the default
without adding special-casing for specific options like `--missing`. But
otherwise I'd rather not rush such a change.

Patrick

> I don't have a really strong preference between the three.
> 
> -Peff

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I
>> think we should do the same here.
>
> I'm honestly still torn on this one. There are two cases that I can
> think of where missing objects would be benign and where one wants to
> use `git rev-list --missing`:
>
>     - Repositories with promisor remotes, to find the boundary of where
>       we need to fetch new objects.
>
>     - Quarantine directories where you only intend to list new objects
>       or find the boundary.
>
> And in neither of those cases I can see a path for how the commit-graph
> would contain such missing commits when using regular tooling to perform
> repository maintenance.

I can buy the promisor remotes use case---we may expect boundary
objects missing without any repository corruption.  I do not know
about the other one---where does our "rev-list --missing" start
traversing in a quarantine directory is unclear.  Objects that are
still in quarantine are not (yet) made reachable from any refs, so
even "rev-list --missing --all" would not make a useful traversal,
no?

In any case, it sounds like you are not torn but are convinced that
we should leave this loose by default ;-) and after thinking it over
again, I tend to agree that it would be a better choice, as long as
the feature "rev-list --missing" has any good use case other than
corruption in repository.

So,... thanks for pushing back.

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

On Thu, Nov 16, 2023 at 09:07:01AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I
> >> think we should do the same here.
> >
> > I'm honestly still torn on this one. There are two cases that I can
> > think of where missing objects would be benign and where one wants to
> > use `git rev-list --missing`:
> >
> >     - Repositories with promisor remotes, to find the boundary of where
> >       we need to fetch new objects.
> >
> >     - Quarantine directories where you only intend to list new objects
> >       or find the boundary.
> >
> > And in neither of those cases I can see a path for how the commit-graph
> > would contain such missing commits when using regular tooling to perform
> > repository maintenance.
> 
> I can buy the promisor remotes use case---we may expect boundary
> objects missing without any repository corruption.  I do not know
> about the other one---where does our "rev-list --missing" start
> traversing in a quarantine directory is unclear. Objects that are
> still in quarantine are not (yet) made reachable from any refs, so
> even "rev-list --missing --all" would not make a useful traversal,
> no?

You typically know about the new tips when having a quarantine
directory. So you can discover the boundary between objects in your
quarantine directory and your main object database by executing `git
rev-list $NEWREVS --missing=print` and execute that command with
`GIT_OBJECT_DIRECTORY=$quarantine`. The benefit is that this scales with
the number of objects in the quarantine, and not with the size of the
overall repository.

As mention, this is really niche, but we do plan to use this in Gitaly
eventually.

> In any case, it sounds like you are not torn but are convinced that
> we should leave this loose by default ;-) and after thinking it over
> again, I tend to agree that it would be a better choice, as long as
> the feature "rev-list --missing" has any good use case other than
> corruption in repository.
> 
> So,... thanks for pushing back.

Okay, glad to hear that I'm not totally bonkers then. I'm going to wait
another few days for additional feedback before sending a v2. But if
what I'm saying also makes sense to others then v2 would only squash in
the diff I sent to run the subset of tests that is now failing  with
`GIT_COMMIT_GRAPH_PARANOIA=true`.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 8228 bytes --]

In 7a5d604443 (commit: detect commits that exist in commit-graph but not
in the ODB, 2023-10-31), we have introduced a new object existence check
into `repo_parse_commit_internal()` so that we do not parse commits via
the commit-graph that don't have a corresponding object in the object
database. This new check of course comes with a performance penalty,
which the commit put at around 30% for `git rev-list --topo-order`. But
there are in fact scenarios where the performance regression is even
higher. The following benchmark against linux.git with a fully-build
commit-graph:

  Benchmark 1: git.v2.42.1 rev-list --count HEAD
    Time (mean ± σ):     658.0 ms ±   5.2 ms    [User: 613.5 ms, System: 44.4 ms]
    Range (min … max):   650.2 ms … 666.0 ms    10 runs

  Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
    Time (mean ± σ):      1.333 s ±  0.019 s    [User: 1.263 s, System: 0.069 s]
    Range (min … max):    1.302 s …  1.361 s    10 runs

  Summary
    git.v2.42.1 rev-list --count HEAD ran
      2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD

While it's a noble goal to ensure that results are the same regardless
of whether or not we have a potentially stale commit-graph, taking twice
as much time is a tough sell. Furthermore, we can generally assume that
the commit-graph will be updated by git-gc(1) or git-maintenance(1) as
required so that the case where the commit-graph is stale should not at
all be common.

With that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore
the behaviour and thus performance previous to the mentioned commit. In
order to not be inconsistent, also disable this behaviour by default in
`lookup_commit_in_graph()`, where the object existence check has been
introduced right at its inception via f559d6d45e (revision: avoid
hitting packfiles when commits are in commit-graph, 2021-08-09).

This results in another speedup in commands that end up calling this
function, even though it's less pronounced compared to the above
benchmark. The following has been executed in linux.git with ~1.2
million references:

  Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
    Time (mean ± σ):      2.947 s ±  0.003 s    [User: 2.412 s, System: 0.534 s]
    Range (min … max):    2.943 s …  2.949 s    3 runs

  Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted
    Time (mean ± σ):      2.724 s ±  0.030 s    [User: 2.207 s, System: 0.514 s]
    Range (min … max):    2.704 s …  2.759 s    3 runs

  Summary
    GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran
      1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted

So whereas 7a5d604443 initially introduced the logic to start doing an
object existence check in `repo_parse_commit_internal()` by default, the
updated logic will now instead cause `lookup_commit_in_graph()` to stop
doing the check by default. This behaviour continues to be tweakable by
the user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.

Note that this requires us to amend some tests to manually turn on the
paranoid checks again. This is because we cause repository corruption by
manually deleting objects which are part of the commit graph already.
These circumstances shouldn't usually happen in repositories.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git.txt       | 6 +++---
 commit-graph.c              | 2 +-
 commit.c                    | 2 +-
 t/t5318-commit-graph.sh     | 8 ++++----
 t/t6022-rev-list-missing.sh | 5 +++++
 t/t7700-repack.sh           | 2 +-
 6 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2535a30194..6c19fd1d76 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -917,9 +917,9 @@ for full details.
 	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.
+The default is "false", which disables the aforementioned behavior.
+Setting this to "true" enables the existence check so that stale commits
+will never be returned from the commit-graph at the cost of performance.
 
 `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 acac9bf6e1..6fad9d195d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1005,7 +1005,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje
 	uint32_t pos;
 
 	if (commit_graph_paranoia == -1)
-		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
 
 	if (!prepare_commit_graph(repo))
 		return NULL;
diff --git a/commit.c b/commit.c
index 8405d7c3fc..37956b836c 100644
--- a/commit.c
+++ b/commit.c
@@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,
 		static int commit_graph_paranoia = -1;
 
 		if (commit_graph_paranoia == -1)
-			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
 
 		if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
 			unparse_commit(r, &item->object.oid);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 7fe7c72a87..a2b4442660 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -911,10 +911,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
 
 		# Verify that it is possible to read the commit from the
 		# commit graph when not being paranoid, ...
-		GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&
+		git rev-list B &&
 		# ... but parsing the commit when double checking that
 		# it actually exists in the object database should fail.
-		test_must_fail git rev-list -1 B
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B
 	)
 '
 
@@ -938,9 +938,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
 
 		# Again, we should be able to parse the commit when not
 		# being paranoid about commit graph staleness...
-		GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
+		git rev-parse HEAD~2 &&
 		# ... but fail when we are paranoid.
-		test_must_fail git rev-parse HEAD~2 2>error &&
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&
 		grep "error: commit $oid exists in commit-graph but not in the object database" error
 	)
 '
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 40265a4f66..1ca4eb5a36 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -13,6 +13,11 @@ test_expect_success 'create repository and alternate directory' '
 	test_commit 3
 '
 
+# We manually corrupt the repository, which means that the commit-graph may
+# contain references to already-deleted objects. We thus need to enable
+# commit-graph paranoia to not returned these deleted commits from the graph.
+export GIT_COMMIT_GRAPH_PARANOIA=true
+
 for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
 do
 	test_expect_success "rev-list --missing=error fails with missing object $obj" '
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index d2975e6c93..94f9f4a1da 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -271,7 +271,7 @@ test_expect_success 'repacking fails when missing .pack actually means missing o
 		ls .git/objects/pack/*.pack >before-pack-dir &&
 
 		test_must_fail git fsck &&
-		test_must_fail git repack --cruft -d 2>err &&
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git repack --cruft -d 2>err &&
 		grep "bad object" err &&
 
 		# Before failing, the repack did not modify the
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Note that this requires us to amend some tests to manually turn on the
> paranoid checks again. This is because we cause repository corruption by
> manually deleting objects which are part of the commit graph already.
> These circumstances shouldn't usually happen in repositories.
> ...
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> index 40265a4f66..1ca4eb5a36 100755
> --- a/t/t6022-rev-list-missing.sh
> +++ b/t/t6022-rev-list-missing.sh
> @@ -13,6 +13,11 @@ test_expect_success 'create repository and alternate directory' '
>  	test_commit 3
>  '
>  
> +# We manually corrupt the repository, which means that the commit-graph may
> +# contain references to already-deleted objects. We thus need to enable
> +# commit-graph paranoia to not returned these deleted commits from the graph.
> +export GIT_COMMIT_GRAPH_PARANOIA=true

test-lint-shell-syntax is a bit overly strict and complains against
this line, so until it is loosened, I'd suggest to do

	GIT_COMMIT_GRAPH_PARANOIA=true
	export GIT_COMMIT_GRAPH_PARANOIA

instead here.

Re: [PATCH v2] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

On Thu, Nov 23, 2023 at 08:44:33PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Note that this requires us to amend some tests to manually turn on the
> > paranoid checks again. This is because we cause repository corruption by
> > manually deleting objects which are part of the commit graph already.
> > These circumstances shouldn't usually happen in repositories.
> > ...
> > diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> > index 40265a4f66..1ca4eb5a36 100755
> > --- a/t/t6022-rev-list-missing.sh
> > +++ b/t/t6022-rev-list-missing.sh
> > @@ -13,6 +13,11 @@ test_expect_success 'create repository and alternate directory' '
> >  	test_commit 3
> >  '
> >  
> > +# We manually corrupt the repository, which means that the commit-graph may
> > +# contain references to already-deleted objects. We thus need to enable
> > +# commit-graph paranoia to not returned these deleted commits from the graph.
> > +export GIT_COMMIT_GRAPH_PARANOIA=true
> 
> test-lint-shell-syntax is a bit overly strict and complains against
> this line, so until it is loosened, I'd suggest to do
> 
> 	GIT_COMMIT_GRAPH_PARANOIA=true
> 	export GIT_COMMIT_GRAPH_PARANOIA
> 
> instead here.

Fair. I was pondering whether to do this when writing this line, but
remembering the recent discussion about it being in POSIX [1] I didn't.
Didn't know though we had a linting rule for this, so I'll send a v3 to
split up the statement.

Patrick

[1]: <87430c6c-91c0-4be1-b89d-bf442b3f018b@gmail.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 8404 bytes --]

In 7a5d604443 (commit: detect commits that exist in commit-graph but not
in the ODB, 2023-10-31), we have introduced a new object existence check
into `repo_parse_commit_internal()` so that we do not parse commits via
the commit-graph that don't have a corresponding object in the object
database. This new check of course comes with a performance penalty,
which the commit put at around 30% for `git rev-list --topo-order`. But
there are in fact scenarios where the performance regression is even
higher. The following benchmark against linux.git with a fully-build
commit-graph:

  Benchmark 1: git.v2.42.1 rev-list --count HEAD
    Time (mean ± σ):     658.0 ms ±   5.2 ms    [User: 613.5 ms, System: 44.4 ms]
    Range (min … max):   650.2 ms … 666.0 ms    10 runs

  Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
    Time (mean ± σ):      1.333 s ±  0.019 s    [User: 1.263 s, System: 0.069 s]
    Range (min … max):    1.302 s …  1.361 s    10 runs

  Summary
    git.v2.42.1 rev-list --count HEAD ran
      2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD

While it's a noble goal to ensure that results are the same regardless
of whether or not we have a potentially stale commit-graph, taking twice
as much time is a tough sell. Furthermore, we can generally assume that
the commit-graph will be updated by git-gc(1) or git-maintenance(1) as
required so that the case where the commit-graph is stale should not at
all be common.

With that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore
the behaviour and thus performance previous to the mentioned commit. In
order to not be inconsistent, also disable this behaviour by default in
`lookup_commit_in_graph()`, where the object existence check has been
introduced right at its inception via f559d6d45e (revision: avoid
hitting packfiles when commits are in commit-graph, 2021-08-09).

This results in another speedup in commands that end up calling this
function, even though it's less pronounced compared to the above
benchmark. The following has been executed in linux.git with ~1.2
million references:

  Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
    Time (mean ± σ):      2.947 s ±  0.003 s    [User: 2.412 s, System: 0.534 s]
    Range (min … max):    2.943 s …  2.949 s    3 runs

  Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted
    Time (mean ± σ):      2.724 s ±  0.030 s    [User: 2.207 s, System: 0.514 s]
    Range (min … max):    2.704 s …  2.759 s    3 runs

  Summary
    GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran
      1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted

So whereas 7a5d604443 initially introduced the logic to start doing an
object existence check in `repo_parse_commit_internal()` by default, the
updated logic will now instead cause `lookup_commit_in_graph()` to stop
doing the check by default. This behaviour continues to be tweakable by
the user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.

Note that this requires us to amend some tests to manually turn on the
paranoid checks again. This is because we cause repository corruption by
manually deleting objects which are part of the commit graph already.
These circumstances shouldn't usually happen in repositories.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

The only change compared to v2 is that I've split up the `export
GIT_COMMIT_GRAPH_PARANOIA=true` line into two lines as suggested by
Junio.

 Documentation/git.txt       | 6 +++---
 commit-graph.c              | 2 +-
 commit.c                    | 2 +-
 t/t5318-commit-graph.sh     | 8 ++++----
 t/t6022-rev-list-missing.sh | 6 ++++++
 t/t7700-repack.sh           | 2 +-
 6 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2535a30194..6c19fd1d76 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -917,9 +917,9 @@ for full details.
 	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.
+The default is "false", which disables the aforementioned behavior.
+Setting this to "true" enables the existence check so that stale commits
+will never be returned from the commit-graph at the cost of performance.
 
 `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 acac9bf6e1..6fad9d195d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1005,7 +1005,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje
 	uint32_t pos;
 
 	if (commit_graph_paranoia == -1)
-		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
 
 	if (!prepare_commit_graph(repo))
 		return NULL;
diff --git a/commit.c b/commit.c
index 8405d7c3fc..37956b836c 100644
--- a/commit.c
+++ b/commit.c
@@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,
 		static int commit_graph_paranoia = -1;
 
 		if (commit_graph_paranoia == -1)
-			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);
 
 		if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
 			unparse_commit(r, &item->object.oid);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 7fe7c72a87..a2b4442660 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -911,10 +911,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
 
 		# Verify that it is possible to read the commit from the
 		# commit graph when not being paranoid, ...
-		GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&
+		git rev-list B &&
 		# ... but parsing the commit when double checking that
 		# it actually exists in the object database should fail.
-		test_must_fail git rev-list -1 B
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B
 	)
 '
 
@@ -938,9 +938,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
 
 		# Again, we should be able to parse the commit when not
 		# being paranoid about commit graph staleness...
-		GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
+		git rev-parse HEAD~2 &&
 		# ... but fail when we are paranoid.
-		test_must_fail git rev-parse HEAD~2 2>error &&
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&
 		grep "error: commit $oid exists in commit-graph but not in the object database" error
 	)
 '
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 40265a4f66..211672759a 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -13,6 +13,12 @@ test_expect_success 'create repository and alternate directory' '
 	test_commit 3
 '
 
+# We manually corrupt the repository, which means that the commit-graph may
+# contain references to already-deleted objects. We thus need to enable
+# commit-graph paranoia to not returned these deleted commits from the graph.
+GIT_COMMIT_GRAPH_PARANOIA=true
+export GIT_COMMIT_GRAPH_PARANOIA
+
 for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
 do
 	test_expect_success "rev-list --missing=error fails with missing object $obj" '
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index d2975e6c93..94f9f4a1da 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -271,7 +271,7 @@ test_expect_success 'repacking fails when missing .pack actually means missing o
 		ls .git/objects/pack/*.pack >before-pack-dir &&
 
 		test_must_fail git fsck &&
-		test_must_fail git repack --cruft -d 2>err &&
+		test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git repack --cruft -d 2>err &&
 		grep "bad object" err &&
 
 		# Before failing, the repack did not modify the
-- 
2.43.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

[Jeff King]

On Wed, Nov 15, 2023 at 02:35:16PM +0100, Patrick Steinhardt wrote:

> > > Just to make sure we do not miscommunicate, I do not think we want
> > > to trigger the paranoia mode only in our tests.  We want to be
> > > paranoid to help real users who used "--missing" for their real use,
> > > so enabling PARANOIA in the test script is a wrong approach.  We
> > > should enable it inside "rev-list --missing" codepath.
> > 
> > Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I
> > think we should do the same here.
> 
> I'm honestly still torn on this one. There are two cases that I can
> think of where missing objects would be benign and where one wants to
> use `git rev-list --missing`:
> 
>     - Repositories with promisor remotes, to find the boundary of where
>       we need to fetch new objects.
> 
>     - Quarantine directories where you only intend to list new objects
>       or find the boundary.
> 
> And in neither of those cases I can see a path for how the commit-graph
> would contain such missing commits when using regular tooling to perform
> repository maintenance.

Sorry for being unclear (and for the very slow response!). What I meant
by "here" was not "rev-list --missing" in particular, but rather that
"here" is "GIT_COMMIT_GRAPH_PARANOIA". And like GIT_REF_PARANOIA, we
should make sure it is turned on when checking for repository
corruption.

So specifically I meant that turning it off should:

  1. Not cause us to miss corruption with fsck.

  2. Not cause us to make corruption worse during a destructive repack
     (e.g., "repack -ad").

  3. Not admit corruption into the repository by fooling the rev-list
     invocation for check_connected().

I don't think the third one uses --missing at all, but even if it did,
the interesting thing to me is not "--missing", but rather that the
caller knows it is doing a corruption check. So it would set the
environment variable itself.

So in your loosening patch, I would have expected to see a couple of
those cases overriding the new "default to off" behavior. But it may be
that they are not necessary (e.g., I think fsck may turn off the commit
graph entirely already).

> So I'm still not sure why we think that this case is so much more
> special than others. If a user wants to check for repository corruption
> the tool shouldn't be `git rev-list --missing`, but git-fsck(1). To me,
> the former is only useful in very specific circumstances where the user
> knows what they are doing, and in none of the usecases I can think of
> should we have a stale commit-graph _unless_ we have actual repository
> corruption.
> 
> In reverse, to me this means that `--missing` is no more special than
> any of the other low-level tooling, where our stance seems to be "We
> assume that the repository is not corrupt". In that spirit, I'd argue
> that the same default value should apply here as for all the other
> cases.

Yeah, I think we are on the same page here. The need for paranoia is
really in the eyes of the caller, because only they know how careful
they want the operation to be.

> Oh, well. I'll wait for answers to this reply until tomorrow, and if I
> still haven't been able to convince anybody then I'll assume it's just
> me and adapt accordingly :)

Sorry, better late than never. ;)

-Peff