{"thread":{"id":"60518","subject":"[PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","startedAt":"2023-11-14T10:23:12Z","lastAt":"2023-12-06T19:49:54Z","messageCount":16,"participants":["Patrick Steinhardt","Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"484856","messageId":"7e2d300c4af9a7853201121d66f982afa421bbba.1699957350.git.ps@pks.im","threadId":"60518","inReplyTo":null,"subject":"[PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-14T10:23:05Z","receivedAt":"2023-11-14T10:23:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 7a5d604443 (commit: detect commits that exist in commit-graph but not\nin the ODB, 2023-10-31), we have introduced a new object existence check\ninto `repo_parse_commit_internal()` so that we do not parse commits via\nthe commit-graph that don't have a corresponding object in the object\ndatabase. This new check of course comes with a performance penalty,\nwhich the commit put at around 30% for `git rev-list --topo-order`. But\nthere are in fact scenarios where the performance regression is even\nhigher. The following benchmark against linux.git with a fully-build\ncommit-graph:\n\n  Benchmark 1: git.v2.42.1 rev-list --count HEAD\n    Time (mean ± σ):     658.0 ms ±   5.2 ms    [User: 613.5 ms, System: 44.4 ms]\n    Range (min … max):   650.2 ms … 666.0 ms    10 runs\n\n  Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD\n    Time (mean ± σ):      1.333 s ±  0.019 s    [User: 1.263 s, System: 0.069 s]\n    Range (min … max):    1.302 s …  1.361 s    10 runs\n\n  Summary\n    git.v2.42.1 rev-list --count HEAD ran\n      2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD\n\nWhile it's a noble goal to ensure that results are the same regardless\nof whether or not we have a potentially stale commit-graph, taking twice\nas much time is a tough sell. Furthermore, we can generally assume that\nthe commit-graph will be updated by git-gc(1) or git-maintenance(1) as\nrequired so that the case where the commit-graph is stale should not at\nall be common.\n\nWith that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore\nthe behaviour and thus performance previous to the mentioned commit. In\norder to not be inconsistent, also disable this behaviour by default in\n`lookup_commit_in_graph()`, where the object existence check has been\nintroduced right at its inception via f559d6d45e (revision: avoid\nhitting packfiles when commits are in commit-graph, 2021-08-09).\n\nThis results in another speedup in commands that end up calling this\nfunction, even though it's less pronounced compared to the above\nbenchmark. The following has been executed in linux.git with ~1.2\nmillion references:\n\n  Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted\n    Time (mean ± σ):      2.947 s ±  0.003 s    [User: 2.412 s, System: 0.534 s]\n    Range (min … max):    2.943 s …  2.949 s    3 runs\n\n  Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted\n    Time (mean ± σ):      2.724 s ±  0.030 s    [User: 2.207 s, System: 0.514 s]\n    Range (min … max):    2.704 s …  2.759 s    3 runs\n\n  Summary\n    GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran\n      1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted\n\nSo whereas 7a5d604443 initially introduced the logic to start doing an\nobject existence check in `repo_parse_commit_internal()` by default, the\nupdated logic will now instead cause `lookup_commit_in_graph()` to stop\ndoing the check by default. This behaviour continues to be tweakable by\nthe user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/git.txt   | 6 +++---\n commit-graph.c          | 2 +-\n commit.c                | 2 +-\n t/t5318-commit-graph.sh | 8 ++++----\n 4 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 2535a30194..6c19fd1d76 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -917,9 +917,9 @@ for full details.\n \tavoid issues with stale commit-graphs that contain references to\n \talready-deleted commits, but comes with a performance penalty.\n +\n-The default is \"true\", which enables the aforementioned behavior.\n-Setting this to \"false\" disables the existence check. This can lead to\n-a performance improvement at the cost of consistency.\n+The default is \"false\", which disables the aforementioned behavior.\n+Setting this to \"true\" enables the existence check so that stale commits\n+will never be returned from the commit-graph at the cost of performance.\n \n `GIT_ALLOW_PROTOCOL`::\n \tIf set to a colon-separated list of protocols, behave as if\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ee66098e07..a712917356 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1029,7 +1029,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \tuint32_t pos;\n \n \tif (commit_graph_paranoia == -1)\n-\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);\n+\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);\n \n \tif (!prepare_commit_graph(repo))\n \t\treturn NULL;\ndiff --git a/commit.c b/commit.c\nindex 8405d7c3fc..37956b836c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,\n \t\tstatic int commit_graph_paranoia = -1;\n \n \t\tif (commit_graph_paranoia == -1)\n-\t\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);\n+\t\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);\n \n \t\tif (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {\n \t\t\tunparse_commit(r, &item->object.oid);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d4fc65e078..4c751a6871 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -909,10 +909,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '\n \n \t\t# Verify that it is possible to read the commit from the\n \t\t# commit graph when not being paranoid, ...\n-\t\tGIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&\n+\t\tgit rev-list B &&\n \t\t# ... but parsing the commit when double checking that\n \t\t# it actually exists in the object database should fail.\n-\t\ttest_must_fail git rev-list -1 B\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B\n \t)\n '\n \n@@ -936,9 +936,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '\n \n \t\t# Again, we should be able to parse the commit when not\n \t\t# being paranoid about commit graph staleness...\n-\t\tGIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&\n+\t\tgit rev-parse HEAD~2 &&\n \t\t# ... but fail when we are paranoid.\n-\t\ttest_must_fail git rev-parse HEAD~2 2>error &&\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&\n \t\tgrep \"error: commit $oid exists in commit-graph but not in the object database\" error\n \t)\n '\n-- \n2.42.0\n\n"},{"id":"484857","messageId":"ZVNNXNRfrwc_0Sj3@tanuki","threadId":"60518","inReplyTo":"7e2d300c4af9a7853201121d66f982afa421bbba.1699957350.git.ps@pks.im","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-14T10:35:08Z","receivedAt":"2023-11-14T10:35:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 14, 2023 at 11:23:05AM +0100, Patrick Steinhardt wrote:\n> In 7a5d604443 (commit: detect commits that exist in commit-graph but not\n> in the ODB, 2023-10-31), we have introduced a new object existence check\n> into `repo_parse_commit_internal()` so that we do not parse commits via\n> the commit-graph that don't have a corresponding object in the object\n> database. This new check of course comes with a performance penalty,\n> which the commit put at around 30% for `git rev-list --topo-order`. But\n> there are in fact scenarios where the performance regression is even\n> higher. The following benchmark against linux.git with a fully-build\n> commit-graph:\n> \n>   Benchmark 1: git.v2.42.1 rev-list --count HEAD\n>     Time (mean ± σ):     658.0 ms ±   5.2 ms    [User: 613.5 ms, System: 44.4 ms]\n>     Range (min … max):   650.2 ms … 666.0 ms    10 runs\n> \n>   Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD\n>     Time (mean ± σ):      1.333 s ±  0.019 s    [User: 1.263 s, System: 0.069 s]\n>     Range (min … max):    1.302 s …  1.361 s    10 runs\n> \n>   Summary\n>     git.v2.42.1 rev-list --count HEAD ran\n>       2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD\n> \n> While it's a noble goal to ensure that results are the same regardless\n> of whether or not we have a potentially stale commit-graph, taking twice\n> as much time is a tough sell. Furthermore, we can generally assume that\n> the commit-graph will be updated by git-gc(1) or git-maintenance(1) as\n> required so that the case where the commit-graph is stale should not at\n> all be common.\n> \n> With that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore\n> the behaviour and thus performance previous to the mentioned commit. In\n> order to not be inconsistent, also disable this behaviour by default in\n> `lookup_commit_in_graph()`, where the object existence check has been\n> introduced right at its inception via f559d6d45e (revision: avoid\n> hitting packfiles when commits are in commit-graph, 2021-08-09).\n> \n> This results in another speedup in commands that end up calling this\n> function, even though it's less pronounced compared to the above\n> benchmark. The following has been executed in linux.git with ~1.2\n> million references:\n> \n>   Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted\n>     Time (mean ± σ):      2.947 s ±  0.003 s    [User: 2.412 s, System: 0.534 s]\n>     Range (min … max):    2.943 s …  2.949 s    3 runs\n> \n>   Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted\n>     Time (mean ± σ):      2.724 s ±  0.030 s    [User: 2.207 s, System: 0.514 s]\n>     Range (min … max):    2.704 s …  2.759 s    3 runs\n> \n>   Summary\n>     GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran\n>       1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted\n> \n> So whereas 7a5d604443 initially introduced the logic to start doing an\n> object existence check in `repo_parse_commit_internal()` by default, the\n> updated logic will now instead cause `lookup_commit_in_graph()` to stop\n> doing the check by default. This behaviour continues to be tweakable by\n> the user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.\n> \n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n\nGah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending\nthis patch. There are two test failures that this change introduces:\n\n  - t6022-rev-list-missing.sh, where we test for the `--missing=` option\n    of git-rev-list(1).\n\n  - t7700-repack.sh, where we also manually delete objects.\n\nBoth of these are expected failures: we knowingly corrupt the repository\nand circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are\nupdated. If we stick with the new stance that repository corruption\nshould not require us to pessimize the common case, then we'd have to\nsquash in something like the below.\n\nPatrick\n\ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nindex 40265a4f66..9e3f063d08 100755\n--- a/t/t6022-rev-list-missing.sh\n+++ b/t/t6022-rev-list-missing.sh\n@@ -27,6 +27,12 @@ do\n \t'\n done\n \n+# When running with GIT_TEST_COMMIT_GRAPH=true we write a commit-graph, but\n+# don't update it before forcefully deleting the commit object. We thus enable\n+# GIT_COMMIT_GRAPH_PARANOIA so that this case is detected with such a stale\n+# commit-graph.\n+export GIT_COMMIT_GRAPH_PARANOIA=true\n+\n for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n do\n \tfor action in \"allow-any\" \"print\"\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex d2975e6c93..ca8ad9c420 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -271,7 +271,7 @@ test_expect_success 'repacking fails when missing .pack actually means missing o\n \t\tls .git/objects/pack/*.pack >before-pack-dir &&\n \n \t\ttest_must_fail git fsck &&\n-\t\ttest_must_fail git repack --cruft -d 2>err &&\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=1 git repack --cruft -d 2>err &&\n \t\tgrep \"bad object\" err &&\n \n \t\t# Before failing, the repack did not modify the\n\n> ---\n>  Documentation/git.txt   | 6 +++---\n>  commit-graph.c          | 2 +-\n>  commit.c                | 2 +-\n>  t/t5318-commit-graph.sh | 8 ++++----\n>  4 files changed, 9 insertions(+), 9 deletions(-)\n> \n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 2535a30194..6c19fd1d76 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -917,9 +917,9 @@ for full details.\n>  \tavoid issues with stale commit-graphs that contain references to\n>  \talready-deleted commits, but comes with a performance penalty.\n>  +\n> -The default is \"true\", which enables the aforementioned behavior.\n> -Setting this to \"false\" disables the existence check. This can lead to\n> -a performance improvement at the cost of consistency.\n> +The default is \"false\", which disables the aforementioned behavior.\n> +Setting this to \"true\" enables the existence check so that stale commits\n> +will never be returned from the commit-graph at the cost of performance.\n>  \n>  `GIT_ALLOW_PROTOCOL`::\n>  \tIf set to a colon-separated list of protocols, behave as if\n> diff --git a/commit-graph.c b/commit-graph.c\n> index ee66098e07..a712917356 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1029,7 +1029,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n>  \tuint32_t pos;\n>  \n>  \tif (commit_graph_paranoia == -1)\n> -\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);\n> +\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);\n>  \n>  \tif (!prepare_commit_graph(repo))\n>  \t\treturn NULL;\n> diff --git a/commit.c b/commit.c\n> index 8405d7c3fc..37956b836c 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,\n>  \t\tstatic int commit_graph_paranoia = -1;\n>  \n>  \t\tif (commit_graph_paranoia == -1)\n> -\t\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);\n> +\t\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);\n>  \n>  \t\tif (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {\n>  \t\t\tunparse_commit(r, &item->object.oid);\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index d4fc65e078..4c751a6871 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -909,10 +909,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '\n>  \n>  \t\t# Verify that it is possible to read the commit from the\n>  \t\t# commit graph when not being paranoid, ...\n> -\t\tGIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&\n> +\t\tgit rev-list B &&\n>  \t\t# ... but parsing the commit when double checking that\n>  \t\t# it actually exists in the object database should fail.\n> -\t\ttest_must_fail git rev-list -1 B\n> +\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B\n>  \t)\n>  '\n>  \n> @@ -936,9 +936,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '\n>  \n>  \t\t# Again, we should be able to parse the commit when not\n>  \t\t# being paranoid about commit graph staleness...\n> -\t\tGIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&\n> +\t\tgit rev-parse HEAD~2 &&\n>  \t\t# ... but fail when we are paranoid.\n> -\t\ttest_must_fail git rev-parse HEAD~2 2>error &&\n> +\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&\n>  \t\tgrep \"error: commit $oid exists in commit-graph but not in the object database\" error\n>  \t)\n>  '\n> -- \n> 2.42.0\n> \n\n\n"},{"id":"484861","messageId":"ZVOHQJaXcV6A7588@nand.local","threadId":"60518","inReplyTo":"ZVNNXNRfrwc_0Sj3@tanuki","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-14T14:42:08Z","receivedAt":"2023-11-14T14:42:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 14, 2023 at 11:35:08AM +0100, Patrick Steinhardt wrote:\n> Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending\n> this patch. There are two test failures that this change introduces:\n>\n>   - t6022-rev-list-missing.sh, where we test for the `--missing=` option\n>     of git-rev-list(1).\n>\n>   - t7700-repack.sh, where we also manually delete objects.\n>\n> Both of these are expected failures: we knowingly corrupt the repository\n> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are\n> updated. If we stick with the new stance that repository corruption\n> should not require us to pessimize the common case, then we'd have to\n> squash in something like the below.\n\nGood catch. Thanks for investigating and providing a thoughtful analysis\nof why we should turn off GIT_COMMIT_GRAPH_PARANOIA by default. I agree\nwith your conclusion (with this follow-up patch squashed in, of course).\n\nMy hunch is that it would be easier for the maintainer to have a single\npatch to queue instead of squashing this in themself. You may want to\nsend a \"v2\" to that effect.\n\nThanks,\nTaylor\n"},{"id":"484864","messageId":"xmqq7cmkz3fi.fsf@gitster.g","threadId":"60518","inReplyTo":"ZVNNXNRfrwc_0Sj3@tanuki","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-14T16:48:33Z","receivedAt":"2023-11-14T16:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending\n> this patch. There are two test failures that this change introduces:\n>\n>   - t6022-rev-list-missing.sh, where we test for the `--missing=` option\n>     of git-rev-list(1).\n\nI would have expected you to enable the paranoia mode automatically\nwhen this option is in effect.\n\n> Both of these are expected failures: we knowingly corrupt the repository\n> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are\n> updated. If we stick with the new stance that repository corruption\n> should not require us to pessimize the common case,...\n\nYeah, just like we try to be extra careful while running fsck,\nbecause \"--missing\" is about finding these \"corrupt\" cases,\ntriggering the paranoia mode upon seeing the option would make\nsense, no?  It would fix the failure in 6022, right?\n\nThanks for working on this.\n"},{"id":"484865","messageId":"xmqqzfzgxops.fsf@gitster.g","threadId":"60518","inReplyTo":"xmqq7cmkz3fi.fsf@gitster.g","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-14T16:51:43Z","receivedAt":"2023-11-14T16:51:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>> Gah, I forgot to run this with GIT_TEST_COMMIT_GRAPH=1 before sending\n>> this patch. There are two test failures that this change introduces:\n>>\n>>   - t6022-rev-list-missing.sh, where we test for the `--missing=` option\n>>     of git-rev-list(1).\n>\n> I would have expected you to enable the paranoia mode automatically\n> when this option is in effect.\n>\n>> Both of these are expected failures: we knowingly corrupt the repository\n>> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are\n>> updated. If we stick with the new stance that repository corruption\n>> should not require us to pessimize the common case,...\n>\n> Yeah, just like we try to be extra careful while running fsck,\n> because \"--missing\" is about finding these \"corrupt\" cases,\n> triggering the paranoia mode upon seeing the option would make\n> sense, no?  It would fix the failure in 6022, right?\n>\n> Thanks for working on this.\n\nJust to make sure we do not miscommunicate, I do not think we want\nto trigger the paranoia mode only in our tests.  We want to be\nparanoid to help real users who used \"--missing\" for their real use,\nso enabling PARANOIA in the test script is a wrong approach.  We\nshould enable it inside \"rev-list --missing\" codepath.\n\n\n"},{"id":"484870","messageId":"20231114194310.GC2092538@coredump.intra.peff.net","threadId":"60518","inReplyTo":"xmqqzfzgxops.fsf@gitster.g","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-14T19:43:10Z","receivedAt":"2023-11-14T19:43:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 15, 2023 at 01:51:43AM +0900, Junio C Hamano wrote:\n\n> >> Both of these are expected failures: we knowingly corrupt the repository\n> >> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are\n> >> updated. If we stick with the new stance that repository corruption\n> >> should not require us to pessimize the common case,...\n> >\n> > Yeah, just like we try to be extra careful while running fsck,\n> > because \"--missing\" is about finding these \"corrupt\" cases,\n> > triggering the paranoia mode upon seeing the option would make\n> > sense, no?  It would fix the failure in 6022, right?\n> >\n> > Thanks for working on this.\n> \n> Just to make sure we do not miscommunicate, I do not think we want\n> to trigger the paranoia mode only in our tests.  We want to be\n> paranoid to help real users who used \"--missing\" for their real use,\n> so enabling PARANOIA in the test script is a wrong approach.  We\n> should enable it inside \"rev-list --missing\" codepath.\n\nYeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I\nthink we should do the same here.\n\nAs we are closing in on the v2.43 release, there's one thing I'm not\nsure about regarding release planning. Are these test cases that _used_\nto detect the corruption, but now don't? I.e., I would have expected\nthat disabling GIT_COMMIT_GRAPH_PARANOIA would take us back to the same\nstate as v2.42. But I think it doesn't because of the hunk in e04838ea82\n(commit-graph: introduce envvar to disable commit existence checks,\n2023-10-31) that makes the has_object() call conditional (and now\ndefaults to off).\n\nWhat I'm getting as it that I think we have three options for v2.43:\n\n  1. Ship what has been in the release candidates, which has a known\n     performance regression (though the severity is up for debate).\n\n  2. Flip the default to \"0\" (i.e., Patrick's patch in this thread). We\n     know that loosens some cases versus 2.42, which may be considered a\n     regression.\n\n  3. Sort it out before the release. We're getting pretty close to do\n     a lot new work there, but I think the changes should be small-ish.\n     The nuclear option is ejecting the topic and re-doing it in the\n     next cycle.\n\nI don't have a really strong preference between the three.\n\n-Peff\n"},{"id":"484902","messageId":"xmqq4jhnyhe1.fsf@gitster.g","threadId":"60518","inReplyTo":"20231114194310.GC2092538@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-15T00:44:38Z","receivedAt":"2023-11-15T00:44:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> What I'm getting as it that I think we have three options for v2.43:\n>\n>   1. Ship what has been in the release candidates, which has a known\n>      performance regression (though the severity is up for debate).\n>\n>   2. Flip the default to \"0\" (i.e., Patrick's patch in this thread). We\n>      know that loosens some cases versus 2.42, which may be considered a\n>      regression.\n>\n>   3. Sort it out before the release. We're getting pretty close to do\n>      a lot new work there, but I think the changes should be small-ish.\n>      The nuclear option is ejecting the topic and re-doing it in the\n>      next cycle.\n>\n> I don't have a really strong preference between the three.\n\nI've been (naively) assuming that #1 is everybody's preference,\nsimply because #2 does introduce a regression in the correctness\ndepartment (as opposed to a possible performance regression caused\nby #1), and because #3 has a high risk of screwing up.\n\nAs long as the performance regression is known and on our radar,\nI'd say that working on a maintenance release after Thanksgiving\nwould be sufficient.\n\nI might be underestimating the impact of the loss of performance,\nthough, in which case I'd consider that nuclear one, which is the\nsimplest and least risky.\n\nThanks.\n\n"},{"id":"484905","messageId":"20231115013646.GA15245@coredump.intra.peff.net","threadId":"60518","inReplyTo":"xmqq4jhnyhe1.fsf@gitster.g","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-15T01:36:46Z","receivedAt":"2023-11-15T01:36:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 15, 2023 at 09:44:38AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > What I'm getting as it that I think we have three options for v2.43:\n> >\n> >   1. Ship what has been in the release candidates, which has a known\n> >      performance regression (though the severity is up for debate).\n> >\n> >   2. Flip the default to \"0\" (i.e., Patrick's patch in this thread). We\n> >      know that loosens some cases versus 2.42, which may be considered a\n> >      regression.\n> >\n> >   3. Sort it out before the release. We're getting pretty close to do\n> >      a lot new work there, but I think the changes should be small-ish.\n> >      The nuclear option is ejecting the topic and re-doing it in the\n> >      next cycle.\n> >\n> > I don't have a really strong preference between the three.\n> \n> I've been (naively) assuming that #1 is everybody's preference,\n> simply because #2 does introduce a regression in the correctness\n> department (as opposed to a possible performance regression caused\n> by #1), and because #3 has a high risk of screwing up.\n> \n> As long as the performance regression is known and on our radar,\n> I'd say that working on a maintenance release after Thanksgiving\n> would be sufficient.\n> \n> I might be underestimating the impact of the loss of performance,\n> though, in which case I'd consider that nuclear one, which is the\n> simplest and least risky.\n\nI am fine with #1 for the release. Mostly I just wanted to understand\nwhat the plan was (and if we needed to be hurrying to try to make the\nnon-nuclear #3 work).\n\n-Peff\n"},{"id":"484918","messageId":"ZVTJFOSnVonoPgZk@tanuki","threadId":"60518","inReplyTo":"20231114194310.GC2092538@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-15T13:35:16Z","receivedAt":"2023-11-15T13:35:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 14, 2023 at 02:43:10PM -0500, Jeff King wrote:\n> On Wed, Nov 15, 2023 at 01:51:43AM +0900, Junio C Hamano wrote:\n> \n> > >> Both of these are expected failures: we knowingly corrupt the repository\n> > >> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are\n> > >> updated. If we stick with the new stance that repository corruption\n> > >> should not require us to pessimize the common case,...\n> > >\n> > > Yeah, just like we try to be extra careful while running fsck,\n> > > because \"--missing\" is about finding these \"corrupt\" cases,\n> > > triggering the paranoia mode upon seeing the option would make\n> > > sense, no?  It would fix the failure in 6022, right?\n> > >\n> > > Thanks for working on this.\n> > \n> > Just to make sure we do not miscommunicate, I do not think we want\n> > to trigger the paranoia mode only in our tests.  We want to be\n> > paranoid to help real users who used \"--missing\" for their real use,\n> > so enabling PARANOIA in the test script is a wrong approach.  We\n> > should enable it inside \"rev-list --missing\" codepath.\n> \n> Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I\n> think we should do the same here.\n\nI'm honestly still torn on this one. There are two cases that I can\nthink of where missing objects would be benign and where one wants to\nuse `git rev-list --missing`:\n\n    - Repositories with promisor remotes, to find the boundary of where\n      we need to fetch new objects.\n\n    - Quarantine directories where you only intend to list new objects\n      or find the boundary.\n\nAnd in neither of those cases I can see a path for how the commit-graph\nwould contain such missing commits when using regular tooling to perform\nrepository maintenance.\n\nSo I'm still not sure why we think that this case is so much more\nspecial than others. If a user wants to check for repository corruption\nthe tool shouldn't be `git rev-list --missing`, but git-fsck(1). To me,\nthe former is only useful in very specific circumstances where the user\nknows what they are doing, and in none of the usecases I can think of\nshould we have a stale commit-graph _unless_ we have actual repository\ncorruption.\n\nIn reverse, to me this means that `--missing` is no more special than\nany of the other low-level tooling, where our stance seems to be \"We\nassume that the repository is not corrupt\". In that spirit, I'd argue\nthat the same default value should apply here as for all the other\ncases.\n\nBut based on the discussion it very much feels like I'm alone with this\ntrain of thought, which may indicate that I'm missing a quintessential\npart of your arguments. May just as well be that I'm too focussed on the\nusecases we at GitLab have for the new `--missing` behaviour around\ncommits that Karthik has just introduced.\n\nOh, well. I'll wait for answers to this reply until tomorrow, and if I\nstill haven't been able to convince anybody then I'll assume it's just\nme and adapt accordingly :)\n\n> As we are closing in on the v2.43 release, there's one thing I'm not\n> sure about regarding release planning. Are these test cases that _used_\n> to detect the corruption, but now don't? I.e., I would have expected\n> that disabling GIT_COMMIT_GRAPH_PARANOIA would take us back to the same\n> state as v2.42. But I think it doesn't because of the hunk in e04838ea82\n> (commit-graph: introduce envvar to disable commit existence checks,\n> 2023-10-31) that makes the has_object() call conditional (and now\n> defaults to off).\n> \n> What I'm getting as it that I think we have three options for v2.43:\n> \n>   1. Ship what has been in the release candidates, which has a known\n>      performance regression (though the severity is up for debate).\n\nThis seems like the best option for now in my opinion. The new behaviour\nis not a bug, quite on the contrary, even though it is slower.\n\nAs Junio once said, we are not a \"performance is king\" project [1]. This\nhas burnt itself into my mind, and funny enough it was in the vicinity\nof the change where I originally introduced the other object existence\ncheck into `lookup_commit_in_graph()`.\n\n[1]: <xmqqr1i1t6zl.fsf@gitster.g>\n\n>   2. Flip the default to \"0\" (i.e., Patrick's patch in this thread). We\n>      know that loosens some cases versus 2.42, which may be considered a\n>      regression.\n\nIf we consider this to be a regression then I'd rather want to drop this\npatch completely and leave it be. Ultimately, the question is how much\nwe trust our tooling to keep the commit-graph up-to-date, and whether or\nnot we need to account for corrupted repositories.\n\nI for myself do trust the tooling, otherwise I wouldn't have sent this\npatch. But I'm also happy to accept the current status where we are\nbeing more thorough at the cost of performance.\n\n>   3. Sort it out before the release. We're getting pretty close to do\n>      a lot new work there, but I think the changes should be small-ish.\n>      The nuclear option is ejecting the topic and re-doing it in the\n>      next cycle.\n\nI would be comfortable with this option if we simply switch the default\nwithout adding special-casing for specific options like `--missing`. But\notherwise I'd rather not rush such a change.\n\nPatrick\n\n> I don't have a really strong preference between the three.\n> \n> -Peff\n"},{"id":"484954","messageId":"xmqqh6lmwogq.fsf@gitster.g","threadId":"60518","inReplyTo":"ZVTJFOSnVonoPgZk@tanuki","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-16T00:07:01Z","receivedAt":"2023-11-16T00:07:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I\n>> think we should do the same here.\n>\n> I'm honestly still torn on this one. There are two cases that I can\n> think of where missing objects would be benign and where one wants to\n> use `git rev-list --missing`:\n>\n>     - Repositories with promisor remotes, to find the boundary of where\n>       we need to fetch new objects.\n>\n>     - Quarantine directories where you only intend to list new objects\n>       or find the boundary.\n>\n> And in neither of those cases I can see a path for how the commit-graph\n> would contain such missing commits when using regular tooling to perform\n> repository maintenance.\n\nI can buy the promisor remotes use case---we may expect boundary\nobjects missing without any repository corruption.  I do not know\nabout the other one---where does our \"rev-list --missing\" start\ntraversing in a quarantine directory is unclear.  Objects that are\nstill in quarantine are not (yet) made reachable from any refs, so\neven \"rev-list --missing --all\" would not make a useful traversal,\nno?\n\nIn any case, it sounds like you are not torn but are convinced that\nwe should leave this loose by default ;-) and after thinking it over\nagain, I tend to agree that it would be a better choice, as long as\nthe feature \"rev-list --missing\" has any good use case other than\ncorruption in repository.\n\nSo,... thanks for pushing back.\n"},{"id":"484975","messageId":"ZVX6so-Gj_ES_FR8@tanuki","threadId":"60518","inReplyTo":"xmqqh6lmwogq.fsf@gitster.g","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-16T11:19:14Z","receivedAt":"2023-11-16T11:19:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 16, 2023 at 09:07:01AM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> >> Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I\n> >> think we should do the same here.\n> >\n> > I'm honestly still torn on this one. There are two cases that I can\n> > think of where missing objects would be benign and where one wants to\n> > use `git rev-list --missing`:\n> >\n> >     - Repositories with promisor remotes, to find the boundary of where\n> >       we need to fetch new objects.\n> >\n> >     - Quarantine directories where you only intend to list new objects\n> >       or find the boundary.\n> >\n> > And in neither of those cases I can see a path for how the commit-graph\n> > would contain such missing commits when using regular tooling to perform\n> > repository maintenance.\n> \n> I can buy the promisor remotes use case---we may expect boundary\n> objects missing without any repository corruption.  I do not know\n> about the other one---where does our \"rev-list --missing\" start\n> traversing in a quarantine directory is unclear. Objects that are\n> still in quarantine are not (yet) made reachable from any refs, so\n> even \"rev-list --missing --all\" would not make a useful traversal,\n> no?\n\nYou typically know about the new tips when having a quarantine\ndirectory. So you can discover the boundary between objects in your\nquarantine directory and your main object database by executing `git\nrev-list $NEWREVS --missing=print` and execute that command with\n`GIT_OBJECT_DIRECTORY=$quarantine`. The benefit is that this scales with\nthe number of objects in the quarantine, and not with the size of the\noverall repository.\n\nAs mention, this is really niche, but we do plan to use this in Gitaly\neventually.\n\n> In any case, it sounds like you are not torn but are convinced that\n> we should leave this loose by default ;-) and after thinking it over\n> again, I tend to agree that it would be a better choice, as long as\n> the feature \"rev-list --missing\" has any good use case other than\n> corruption in repository.\n> \n> So,... thanks for pushing back.\n\nOkay, glad to hear that I'm not totally bonkers then. I'm going to wait\nanother few days for additional feedback before sending a v2. But if\nwhat I'm saying also makes sense to others then v2 would only squash in\nthe diff I sent to run the subset of tests that is now failing  with\n`GIT_COMMIT_GRAPH_PARANOIA=true`.\n\nPatrick\n"},{"id":"485015","messageId":"f1235741cea5866e67c83aca83a760e0cdde8730.1700478031.git.ps@pks.im","threadId":"60518","inReplyTo":"7e2d300c4af9a7853201121d66f982afa421bbba.1699957350.git.ps@pks.im","subject":"[PATCH v2] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-20T11:01:11Z","receivedAt":"2023-11-20T11:01:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 7a5d604443 (commit: detect commits that exist in commit-graph but not\nin the ODB, 2023-10-31), we have introduced a new object existence check\ninto `repo_parse_commit_internal()` so that we do not parse commits via\nthe commit-graph that don't have a corresponding object in the object\ndatabase. This new check of course comes with a performance penalty,\nwhich the commit put at around 30% for `git rev-list --topo-order`. But\nthere are in fact scenarios where the performance regression is even\nhigher. The following benchmark against linux.git with a fully-build\ncommit-graph:\n\n  Benchmark 1: git.v2.42.1 rev-list --count HEAD\n    Time (mean ± σ):     658.0 ms ±   5.2 ms    [User: 613.5 ms, System: 44.4 ms]\n    Range (min … max):   650.2 ms … 666.0 ms    10 runs\n\n  Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD\n    Time (mean ± σ):      1.333 s ±  0.019 s    [User: 1.263 s, System: 0.069 s]\n    Range (min … max):    1.302 s …  1.361 s    10 runs\n\n  Summary\n    git.v2.42.1 rev-list --count HEAD ran\n      2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD\n\nWhile it's a noble goal to ensure that results are the same regardless\nof whether or not we have a potentially stale commit-graph, taking twice\nas much time is a tough sell. Furthermore, we can generally assume that\nthe commit-graph will be updated by git-gc(1) or git-maintenance(1) as\nrequired so that the case where the commit-graph is stale should not at\nall be common.\n\nWith that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore\nthe behaviour and thus performance previous to the mentioned commit. In\norder to not be inconsistent, also disable this behaviour by default in\n`lookup_commit_in_graph()`, where the object existence check has been\nintroduced right at its inception via f559d6d45e (revision: avoid\nhitting packfiles when commits are in commit-graph, 2021-08-09).\n\nThis results in another speedup in commands that end up calling this\nfunction, even though it's less pronounced compared to the above\nbenchmark. The following has been executed in linux.git with ~1.2\nmillion references:\n\n  Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted\n    Time (mean ± σ):      2.947 s ±  0.003 s    [User: 2.412 s, System: 0.534 s]\n    Range (min … max):    2.943 s …  2.949 s    3 runs\n\n  Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted\n    Time (mean ± σ):      2.724 s ±  0.030 s    [User: 2.207 s, System: 0.514 s]\n    Range (min … max):    2.704 s …  2.759 s    3 runs\n\n  Summary\n    GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran\n      1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted\n\nSo whereas 7a5d604443 initially introduced the logic to start doing an\nobject existence check in `repo_parse_commit_internal()` by default, the\nupdated logic will now instead cause `lookup_commit_in_graph()` to stop\ndoing the check by default. This behaviour continues to be tweakable by\nthe user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.\n\nNote that this requires us to amend some tests to manually turn on the\nparanoid checks again. This is because we cause repository corruption by\nmanually deleting objects which are part of the commit graph already.\nThese circumstances shouldn't usually happen in repositories.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/git.txt       | 6 +++---\n commit-graph.c              | 2 +-\n commit.c                    | 2 +-\n t/t5318-commit-graph.sh     | 8 ++++----\n t/t6022-rev-list-missing.sh | 5 +++++\n t/t7700-repack.sh           | 2 +-\n 6 files changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 2535a30194..6c19fd1d76 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -917,9 +917,9 @@ for full details.\n \tavoid issues with stale commit-graphs that contain references to\n \talready-deleted commits, but comes with a performance penalty.\n +\n-The default is \"true\", which enables the aforementioned behavior.\n-Setting this to \"false\" disables the existence check. This can lead to\n-a performance improvement at the cost of consistency.\n+The default is \"false\", which disables the aforementioned behavior.\n+Setting this to \"true\" enables the existence check so that stale commits\n+will never be returned from the commit-graph at the cost of performance.\n \n `GIT_ALLOW_PROTOCOL`::\n \tIf set to a colon-separated list of protocols, behave as if\ndiff --git a/commit-graph.c b/commit-graph.c\nindex acac9bf6e1..6fad9d195d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1005,7 +1005,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \tuint32_t pos;\n \n \tif (commit_graph_paranoia == -1)\n-\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);\n+\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);\n \n \tif (!prepare_commit_graph(repo))\n \t\treturn NULL;\ndiff --git a/commit.c b/commit.c\nindex 8405d7c3fc..37956b836c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,\n \t\tstatic int commit_graph_paranoia = -1;\n \n \t\tif (commit_graph_paranoia == -1)\n-\t\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);\n+\t\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);\n \n \t\tif (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {\n \t\t\tunparse_commit(r, &item->object.oid);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 7fe7c72a87..a2b4442660 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -911,10 +911,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '\n \n \t\t# Verify that it is possible to read the commit from the\n \t\t# commit graph when not being paranoid, ...\n-\t\tGIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&\n+\t\tgit rev-list B &&\n \t\t# ... but parsing the commit when double checking that\n \t\t# it actually exists in the object database should fail.\n-\t\ttest_must_fail git rev-list -1 B\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B\n \t)\n '\n \n@@ -938,9 +938,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '\n \n \t\t# Again, we should be able to parse the commit when not\n \t\t# being paranoid about commit graph staleness...\n-\t\tGIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&\n+\t\tgit rev-parse HEAD~2 &&\n \t\t# ... but fail when we are paranoid.\n-\t\ttest_must_fail git rev-parse HEAD~2 2>error &&\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&\n \t\tgrep \"error: commit $oid exists in commit-graph but not in the object database\" error\n \t)\n '\ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nindex 40265a4f66..1ca4eb5a36 100755\n--- a/t/t6022-rev-list-missing.sh\n+++ b/t/t6022-rev-list-missing.sh\n@@ -13,6 +13,11 @@ test_expect_success 'create repository and alternate directory' '\n \ttest_commit 3\n '\n \n+# We manually corrupt the repository, which means that the commit-graph may\n+# contain references to already-deleted objects. We thus need to enable\n+# commit-graph paranoia to not returned these deleted commits from the graph.\n+export GIT_COMMIT_GRAPH_PARANOIA=true\n+\n for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n do\n \ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex d2975e6c93..94f9f4a1da 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -271,7 +271,7 @@ test_expect_success 'repacking fails when missing .pack actually means missing o\n \t\tls .git/objects/pack/*.pack >before-pack-dir &&\n \n \t\ttest_must_fail git fsck &&\n-\t\ttest_must_fail git repack --cruft -d 2>err &&\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git repack --cruft -d 2>err &&\n \t\tgrep \"bad object\" err &&\n \n \t\t# Before failing, the repack did not modify the\n-- \n2.42.0\n\n"},{"id":"485062","messageId":"xmqq7cm8bsny.fsf@gitster.g","threadId":"60518","inReplyTo":"f1235741cea5866e67c83aca83a760e0cdde8730.1700478031.git.ps@pks.im","subject":"Re: [PATCH v2] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-23T11:44:33Z","receivedAt":"2023-11-23T11:44:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Note that this requires us to amend some tests to manually turn on the\n> paranoid checks again. This is because we cause repository corruption by\n> manually deleting objects which are part of the commit graph already.\n> These circumstances shouldn't usually happen in repositories.\n> ...\n> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n> index 40265a4f66..1ca4eb5a36 100755\n> --- a/t/t6022-rev-list-missing.sh\n> +++ b/t/t6022-rev-list-missing.sh\n> @@ -13,6 +13,11 @@ test_expect_success 'create repository and alternate directory' '\n>  \ttest_commit 3\n>  '\n>  \n> +# We manually corrupt the repository, which means that the commit-graph may\n> +# contain references to already-deleted objects. We thus need to enable\n> +# commit-graph paranoia to not returned these deleted commits from the graph.\n> +export GIT_COMMIT_GRAPH_PARANOIA=true\n\ntest-lint-shell-syntax is a bit overly strict and complains against\nthis line, so until it is loosened, I'd suggest to do\n\n\tGIT_COMMIT_GRAPH_PARANOIA=true\n\texport GIT_COMMIT_GRAPH_PARANOIA\n\ninstead here.\n\n"},{"id":"485105","messageId":"ZWCD-cDbhTCU0aU-@tanuki","threadId":"60518","inReplyTo":"xmqq7cm8bsny.fsf@gitster.g","subject":"Re: [PATCH v2] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-24T11:07:37Z","receivedAt":"2023-11-24T11:07:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 23, 2023 at 08:44:33PM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Note that this requires us to amend some tests to manually turn on the\n> > paranoid checks again. This is because we cause repository corruption by\n> > manually deleting objects which are part of the commit graph already.\n> > These circumstances shouldn't usually happen in repositories.\n> > ...\n> > diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\n> > index 40265a4f66..1ca4eb5a36 100755\n> > --- a/t/t6022-rev-list-missing.sh\n> > +++ b/t/t6022-rev-list-missing.sh\n> > @@ -13,6 +13,11 @@ test_expect_success 'create repository and alternate directory' '\n> >  \ttest_commit 3\n> >  '\n> >  \n> > +# We manually corrupt the repository, which means that the commit-graph may\n> > +# contain references to already-deleted objects. We thus need to enable\n> > +# commit-graph paranoia to not returned these deleted commits from the graph.\n> > +export GIT_COMMIT_GRAPH_PARANOIA=true\n> \n> test-lint-shell-syntax is a bit overly strict and complains against\n> this line, so until it is loosened, I'd suggest to do\n> \n> \tGIT_COMMIT_GRAPH_PARANOIA=true\n> \texport GIT_COMMIT_GRAPH_PARANOIA\n> \n> instead here.\n\nFair. I was pondering whether to do this when writing this line, but\nremembering the recent discussion about it being in POSIX [1] I didn't.\nDidn't know though we had a linting rule for this, so I'll send a v3 to\nsplit up the statement.\n\nPatrick\n\n[1]: <87430c6c-91c0-4be1-b89d-bf442b3f018b@gmail.com>\n"},{"id":"485106","messageId":"17e08289cd59d20de0de9b4e18f5e6bf77987351.1700823746.git.ps@pks.im","threadId":"60518","inReplyTo":"7e2d300c4af9a7853201121d66f982afa421bbba.1699957350.git.ps@pks.im","subject":"[PATCH v3] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-24T11:08:21Z","receivedAt":"2023-11-24T11:08:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 7a5d604443 (commit: detect commits that exist in commit-graph but not\nin the ODB, 2023-10-31), we have introduced a new object existence check\ninto `repo_parse_commit_internal()` so that we do not parse commits via\nthe commit-graph that don't have a corresponding object in the object\ndatabase. This new check of course comes with a performance penalty,\nwhich the commit put at around 30% for `git rev-list --topo-order`. But\nthere are in fact scenarios where the performance regression is even\nhigher. The following benchmark against linux.git with a fully-build\ncommit-graph:\n\n  Benchmark 1: git.v2.42.1 rev-list --count HEAD\n    Time (mean ± σ):     658.0 ms ±   5.2 ms    [User: 613.5 ms, System: 44.4 ms]\n    Range (min … max):   650.2 ms … 666.0 ms    10 runs\n\n  Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD\n    Time (mean ± σ):      1.333 s ±  0.019 s    [User: 1.263 s, System: 0.069 s]\n    Range (min … max):    1.302 s …  1.361 s    10 runs\n\n  Summary\n    git.v2.42.1 rev-list --count HEAD ran\n      2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD\n\nWhile it's a noble goal to ensure that results are the same regardless\nof whether or not we have a potentially stale commit-graph, taking twice\nas much time is a tough sell. Furthermore, we can generally assume that\nthe commit-graph will be updated by git-gc(1) or git-maintenance(1) as\nrequired so that the case where the commit-graph is stale should not at\nall be common.\n\nWith that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore\nthe behaviour and thus performance previous to the mentioned commit. In\norder to not be inconsistent, also disable this behaviour by default in\n`lookup_commit_in_graph()`, where the object existence check has been\nintroduced right at its inception via f559d6d45e (revision: avoid\nhitting packfiles when commits are in commit-graph, 2021-08-09).\n\nThis results in another speedup in commands that end up calling this\nfunction, even though it's less pronounced compared to the above\nbenchmark. The following has been executed in linux.git with ~1.2\nmillion references:\n\n  Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted\n    Time (mean ± σ):      2.947 s ±  0.003 s    [User: 2.412 s, System: 0.534 s]\n    Range (min … max):    2.943 s …  2.949 s    3 runs\n\n  Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted\n    Time (mean ± σ):      2.724 s ±  0.030 s    [User: 2.207 s, System: 0.514 s]\n    Range (min … max):    2.704 s …  2.759 s    3 runs\n\n  Summary\n    GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran\n      1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted\n\nSo whereas 7a5d604443 initially introduced the logic to start doing an\nobject existence check in `repo_parse_commit_internal()` by default, the\nupdated logic will now instead cause `lookup_commit_in_graph()` to stop\ndoing the check by default. This behaviour continues to be tweakable by\nthe user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.\n\nNote that this requires us to amend some tests to manually turn on the\nparanoid checks again. This is because we cause repository corruption by\nmanually deleting objects which are part of the commit graph already.\nThese circumstances shouldn't usually happen in repositories.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nThe only change compared to v2 is that I've split up the `export\nGIT_COMMIT_GRAPH_PARANOIA=true` line into two lines as suggested by\nJunio.\n\n Documentation/git.txt       | 6 +++---\n commit-graph.c              | 2 +-\n commit.c                    | 2 +-\n t/t5318-commit-graph.sh     | 8 ++++----\n t/t6022-rev-list-missing.sh | 6 ++++++\n t/t7700-repack.sh           | 2 +-\n 6 files changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 2535a30194..6c19fd1d76 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -917,9 +917,9 @@ for full details.\n \tavoid issues with stale commit-graphs that contain references to\n \talready-deleted commits, but comes with a performance penalty.\n +\n-The default is \"true\", which enables the aforementioned behavior.\n-Setting this to \"false\" disables the existence check. This can lead to\n-a performance improvement at the cost of consistency.\n+The default is \"false\", which disables the aforementioned behavior.\n+Setting this to \"true\" enables the existence check so that stale commits\n+will never be returned from the commit-graph at the cost of performance.\n \n `GIT_ALLOW_PROTOCOL`::\n \tIf set to a colon-separated list of protocols, behave as if\ndiff --git a/commit-graph.c b/commit-graph.c\nindex acac9bf6e1..6fad9d195d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1005,7 +1005,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \tuint32_t pos;\n \n \tif (commit_graph_paranoia == -1)\n-\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);\n+\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);\n \n \tif (!prepare_commit_graph(repo))\n \t\treturn NULL;\ndiff --git a/commit.c b/commit.c\nindex 8405d7c3fc..37956b836c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r,\n \t\tstatic int commit_graph_paranoia = -1;\n \n \t\tif (commit_graph_paranoia == -1)\n-\t\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);\n+\t\t\tcommit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0);\n \n \t\tif (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {\n \t\t\tunparse_commit(r, &item->object.oid);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 7fe7c72a87..a2b4442660 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -911,10 +911,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' '\n \n \t\t# Verify that it is possible to read the commit from the\n \t\t# commit graph when not being paranoid, ...\n-\t\tGIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&\n+\t\tgit rev-list B &&\n \t\t# ... but parsing the commit when double checking that\n \t\t# it actually exists in the object database should fail.\n-\t\ttest_must_fail git rev-list -1 B\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B\n \t)\n '\n \n@@ -938,9 +938,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '\n \n \t\t# Again, we should be able to parse the commit when not\n \t\t# being paranoid about commit graph staleness...\n-\t\tGIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&\n+\t\tgit rev-parse HEAD~2 &&\n \t\t# ... but fail when we are paranoid.\n-\t\ttest_must_fail git rev-parse HEAD~2 2>error &&\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error &&\n \t\tgrep \"error: commit $oid exists in commit-graph but not in the object database\" error\n \t)\n '\ndiff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh\nindex 40265a4f66..211672759a 100755\n--- a/t/t6022-rev-list-missing.sh\n+++ b/t/t6022-rev-list-missing.sh\n@@ -13,6 +13,12 @@ test_expect_success 'create repository and alternate directory' '\n \ttest_commit 3\n '\n \n+# We manually corrupt the repository, which means that the commit-graph may\n+# contain references to already-deleted objects. We thus need to enable\n+# commit-graph paranoia to not returned these deleted commits from the graph.\n+GIT_COMMIT_GRAPH_PARANOIA=true\n+export GIT_COMMIT_GRAPH_PARANOIA\n+\n for obj in \"HEAD~1\" \"HEAD~1^{tree}\" \"HEAD:1.t\"\n do\n \ttest_expect_success \"rev-list --missing=error fails with missing object $obj\" '\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex d2975e6c93..94f9f4a1da 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -271,7 +271,7 @@ test_expect_success 'repacking fails when missing .pack actually means missing o\n \t\tls .git/objects/pack/*.pack >before-pack-dir &&\n \n \t\ttest_must_fail git fsck &&\n-\t\ttest_must_fail git repack --cruft -d 2>err &&\n+\t\ttest_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git repack --cruft -d 2>err &&\n \t\tgrep \"bad object\" err &&\n \n \t\t# Before failing, the repack did not modify the\n-- \n2.43.0\n\n"},{"id":"485393","messageId":"20231206194952.GC103708@coredump.intra.peff.net","threadId":"60518","inReplyTo":"ZVTJFOSnVonoPgZk@tanuki","subject":"Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-06T19:49:52Z","receivedAt":"2023-12-06T19:49:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 15, 2023 at 02:35:16PM +0100, Patrick Steinhardt wrote:\n\n> > > Just to make sure we do not miscommunicate, I do not think we want\n> > > to trigger the paranoia mode only in our tests.  We want to be\n> > > paranoid to help real users who used \"--missing\" for their real use,\n> > > so enabling PARANOIA in the test script is a wrong approach.  We\n> > > should enable it inside \"rev-list --missing\" codepath.\n> > \n> > Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I\n> > think we should do the same here.\n> \n> I'm honestly still torn on this one. There are two cases that I can\n> think of where missing objects would be benign and where one wants to\n> use `git rev-list --missing`:\n> \n>     - Repositories with promisor remotes, to find the boundary of where\n>       we need to fetch new objects.\n> \n>     - Quarantine directories where you only intend to list new objects\n>       or find the boundary.\n> \n> And in neither of those cases I can see a path for how the commit-graph\n> would contain such missing commits when using regular tooling to perform\n> repository maintenance.\n\nSorry for being unclear (and for the very slow response!). What I meant\nby \"here\" was not \"rev-list --missing\" in particular, but rather that\n\"here\" is \"GIT_COMMIT_GRAPH_PARANOIA\". And like GIT_REF_PARANOIA, we\nshould make sure it is turned on when checking for repository\ncorruption.\n\nSo specifically I meant that turning it off should:\n\n  1. Not cause us to miss corruption with fsck.\n\n  2. Not cause us to make corruption worse during a destructive repack\n     (e.g., \"repack -ad\").\n\n  3. Not admit corruption into the repository by fooling the rev-list\n     invocation for check_connected().\n\nI don't think the third one uses --missing at all, but even if it did,\nthe interesting thing to me is not \"--missing\", but rather that the\ncaller knows it is doing a corruption check. So it would set the\nenvironment variable itself.\n\nSo in your loosening patch, I would have expected to see a couple of\nthose cases overriding the new \"default to off\" behavior. But it may be\nthat they are not necessary (e.g., I think fsck may turn off the commit\ngraph entirely already).\n\n> So I'm still not sure why we think that this case is so much more\n> special than others. If a user wants to check for repository corruption\n> the tool shouldn't be `git rev-list --missing`, but git-fsck(1). To me,\n> the former is only useful in very specific circumstances where the user\n> knows what they are doing, and in none of the usecases I can think of\n> should we have a stale commit-graph _unless_ we have actual repository\n> corruption.\n> \n> In reverse, to me this means that `--missing` is no more special than\n> any of the other low-level tooling, where our stance seems to be \"We\n> assume that the repository is not corrupt\". In that spirit, I'd argue\n> that the same default value should apply here as for all the other\n> cases.\n\nYeah, I think we are on the same page here. The need for paranoia is\nreally in the eyes of the caller, because only they know how careful\nthey want the operation to be.\n\n> Oh, well. I'll wait for answers to this reply until tomorrow, and if I\n> still haven't been able to convince anybody then I'll assume it's just\n> me and adapt accordingly :)\n\nSorry, better late than never. ;)\n\n-Peff\n"}]}