{"thread":{"id":"56494","subject":"[PATCH 0/3] Use default values from settings instead of config","startedAt":"2021-09-13T18:12:32Z","lastAt":"2021-10-15T21:31:53Z","messageCount":65,"participants":["Glen Choo","Taylor Blau","Eric Sunshine","Bagas Sanjaya","Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"435761","messageId":"20210913181221.42635-1-chooglen@google.com","threadId":"56494","inReplyTo":null,"subject":"[PATCH 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-13T18:12:18Z","receivedAt":"2021-09-13T18:12:32Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Hi everyone! This patch was created in response to something we observed in\nGoogle, where fsck failed to detect that the commit graph was invalid. We\ninitially assumed that fsck never checked the commit graph, but it turns out\nthat it does so only when core.commitgraph is set, even though we set defaults\nfor \"whether to use the commit graph\" in the repository settings.\n\nInstead of using the config, let's use repository settings where\navailable. Replace core.commitGraph and core.multiPackIndex with their\nequivalent repository settings in fsck and gc.\n\nI'm fairly new to the codebase (this is my first patch!), so I have some\nquestions/concerns that I wasn't able to figure out:\n- prepare_repo_settings() may have undesirable side effects or may\n not always be callable\n- I can't find tests that cover the changes to gc, so I'm not sure if my\n understanding of gc is correct\n\nLet me know if there is something I'm missing :)\n\nCC-ing Derrick Stolee who has done work on both prepare_repo_settings and gc.\n\nGlen Choo (3):\n fsck: verify commit graph when implicitly enabled\n fsck: verify multi-pack-index when implictly enabled\n gc: perform incremental repack when implictly enabled\n\n builtin/fsck.c | 5 +++--\n builtin/gc.c | 5 ++---\n t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-\n t/t5319-multi-pack-index.sh | 15 ++++++++++++++-\n 4 files changed, 41 insertions(+), 7 deletions(-)\n\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435762","messageId":"20210913181221.42635-2-chooglen@google.com","threadId":"56494","inReplyTo":"20210913181221.42635-1-chooglen@google.com","subject":"[PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-13T18:12:19Z","receivedAt":"2021-09-13T18:12:36Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"the_repository->settings() is the preferred way to get certain\nsettings (such as \"core.commitGraph\") because it gets default values\nfrom prepare_repo_settings(). However, cmd_fsck() reads the config\ndirectly via git_config_get_bool(), which bypasses these default values.\nThis causes fsck to ignore the commit graph if \"core.commitgraph\" is not\nexplicitly set in the config, even though commit graph is enabled by\ndefault.\n\nReplace git_config_get_bool(\"core.commitgraph\") in fsck with the\nequivalent call to the_repository->settings.core_commit_graph.\n\nThe expected behavior is that fsck respects the config value when it is\nset, but uses the default value when it is unset. For example, for\ncore.commitGraph, there are three cases:\n\n- Config value is set to true -> enabled\n- Config value is set to false -> disabled\n- Config value is unset -> enabled\n\nAs such, tests cover all three cases.\n\nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 3 ++-\n t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex b42b6fe21f..1c4e485b66 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -803,6 +803,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tfsck_enable_object_names(&fsck_walk_options);\n \n \tgit_config(git_fsck_config, &fsck_obj_options);\n+\tprepare_repo_settings(the_repository);\n \n \tif (connectivity_only) {\n \t\tfor_each_loose_object(mark_loose_for_connectivity, NULL, 0);\n@@ -908,7 +909,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tcheck_connectivity();\n \n-\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n+\tif (the_repository->settings.core_commit_graph) {\n \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex af88f805aa..48c5096757 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -674,7 +674,7 @@ test_expect_success 'detect incorrect chunk count' '\n \t\t$GRAPH_CHUNK_LOOKUP_OFFSET\n '\n \n-test_expect_success 'git fsck (checks commit-graph)' '\n+test_expect_success 'git fsck (checks commit-graph when config set to true)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n@@ -683,6 +683,27 @@ test_expect_success 'git fsck (checks commit-graph)' '\n \ttest_must_fail git fsck\n '\n \n+test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\tgit -c core.commitGraph=false fsck\n+'\n+\n+test_expect_success 'git fsck (checks commit-graph when config unset)' '\n+\ttest_when_finished \"git config core.commitGraph true\" &&\n+\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+ git config --unset core.commitGraph &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\ttest_must_fail git fsck\n+'\n+\n test_expect_success 'setup non-the_repository tests' '\n \trm -rf repo &&\n \tgit init repo &&\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435763","messageId":"20210913181221.42635-3-chooglen@google.com","threadId":"56494","inReplyTo":"20210913181221.42635-1-chooglen@google.com","subject":"[PATCH 2/3] fsck: verify multi-pack-index when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-13T18:12:20Z","receivedAt":"2021-09-13T18:12:41Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Like the previous commit, the_repository->settings.core_multi_pack_index\nis preferable to reading \"core.multiPackIndex\" from the config because\nprepare_repo_settings() sets a default value for\nthe_repository->settings.\n\nReplace git_config_get_bool(\"core.multiPackIndex\") in fsck with the\nequivalent call to the_repository->settings.multi_pack_index.\n\nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 2 +-\n t/t5319-multi-pack-index.sh | 15 ++++++++++++++-\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1c4e485b66..5bbe8068ec 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -925,7 +925,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (!git_config_get_bool(\"core.multipackindex\", &i) && i) {\n+\tif (the_repository->settings.core_multi_pack_index) {\n \t\tstruct child_process midx_verify = CHILD_PROCESS_INIT;\n \t\tconst char *midx_argv[] = { \"multi-pack-index\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3d4d9f10c3..1a17446cf0 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -404,12 +404,25 @@ test_expect_success 'verify incorrect offset' '\n \t\t\"incorrect object offset\"\n '\n \n-test_expect_success 'git-fsck incorrect offset' '\n+test_expect_success 'git-fsck incorrect offset (config set to true)' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" \\\n \t\t\"git -c core.multipackindex=true fsck\"\n '\n \n+test_expect_success 'git-fsck incorrect offset (config set to false)' '\n+\tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n+\t\t\"incorrect object offset\" \\\n+\t\t\"git -c core.multipackindex=true fsck\" &&\n+\t\tgit -c core.multipackindex=false fsck\n+'\n+\n+test_expect_success 'git-fsck incorrect offset (config unset)' '\n+\tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n+\t\t\"incorrect object offset\" \\\n+\t\t\"git fsck\"\n+'\n+\n test_expect_success 'corrupt MIDX is not reused' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" &&\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435764","messageId":"20210913181221.42635-4-chooglen@google.com","threadId":"56494","inReplyTo":"20210913181221.42635-1-chooglen@google.com","subject":"[PATCH 3/3] gc: perform incremental repack when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-13T18:12:21Z","receivedAt":"2021-09-13T18:12:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"builtin/gc.c has two ways of checking of multi-pack-index is enabled:\n- git_config_get_bool() in incremental_repack_auto_condition()\n- the_repository->settings.core_multi_pack_index in\n maintenance_task_incremental_repack()\n\nthe_repository->settings.core_multi_pack_index should be preferred\nbecause it has default values from prepare_repo_settings().\n\nStandardize on the_repository->settings.core_multi_pack_index to check\nif multi-pack-index is enabled.\n\nSigned-off-by: Glen Choo \n---\n builtin/gc.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex f05d2f0a1a..070b7dccb1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1051,12 +1051,11 @@ static int maintenance_task_loose_objects(struct maintenance_run_opts *opts)\n static int incremental_repack_auto_condition(void)\n {\n \tstruct packed_git *p;\n-\tint enabled;\n \tint incremental_repack_auto_limit = 10;\n \tint count = 0;\n \n-\tif (git_config_get_bool(\"core.multiPackIndex\", &enabled) ||\n-\t !enabled)\n+\tprepare_repo_settings(the_repository);\n+\tif (!the_repository->settings.core_multi_pack_index)\n \t\treturn 0;\n \n \tgit_config_get_int(\"maintenance.incremental-repack.auto\",\n-- \n2.33.0.309.g3052b89438-goog\n\n"},{"id":"435770","messageId":"YT+kJ5jhzn6rghc9@nand.local","threadId":"56494","inReplyTo":"20210913181221.42635-1-chooglen@google.com","subject":"Re: [PATCH 0/3] Use default values from settings instead of config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-13T19:19:03Z","receivedAt":"2021-09-13T19:19:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 13, 2021 at 11:12:18AM -0700, Glen Choo wrote:\n> I'm fairly new to the codebase (this is my first patch!), so I have some\n> questions/concerns that I wasn't able to figure out:\n\nWelcome to the Git community! :-).\n\n> - prepare_repo_settings() may have undesirable side effects or may\n> not always be callable\n\nCalling prepare_repo_settings() is definitely the right thing to do,\nbecause (as you note) it centralizes the default values for settings\nthat it keeps track of.\n\nYou can call prepare_repo_settings() so long as you have a repository to\ncall it on. Since fsck and gc must run inside of a repository, the\ncallers you added are safe. And note that prepare_repo_settings() is\nidempotent, i.e., that it is a noop when called more than once.\n\nThanks,\nTaylor\n"},{"id":"435776","messageId":"YT+mlW851sRyt842@nand.local","threadId":"56494","inReplyTo":"20210913181221.42635-2-chooglen@google.com","subject":"Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-13T19:29:25Z","receivedAt":"2021-09-13T19:29:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 13, 2021 at 11:12:19AM -0700, Glen Choo wrote:\n> the_repository->settings() is the preferred way to get certain\n> settings (such as \"core.commitGraph\") because it gets default values\n> from prepare_repo_settings(). However, cmd_fsck() reads the config\n> directly via git_config_get_bool(), which bypasses these default values.\n> This causes fsck to ignore the commit graph if \"core.commitgraph\" is not\n> explicitly set in the config, even though commit graph is enabled by\n> default.\n\nSmall nit; \"the_repository->settings()\" should be spelled as\n\"the_repository->settings\", since \"settings\" is not a function.\n\nIt may be worth noting that this was totally fine before\ncore.commitGraph's default changed to true. That happened in 31b1de6a09\n(commit-graph: turn on commit-graph by default, 2019-08-13), which is\nthe first time this sort of regression would have appeared.\n\n> \tif (connectivity_only) {\n> \t\tfor_each_loose_object(mark_loose_for_connectivity, NULL, 0);\n> @@ -908,7 +909,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n>\n> \tcheck_connectivity();\n>\n> -\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n> +\tif (the_repository->settings.core_commit_graph) {\n> \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n> \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n\nHere's the main part of your change, which is obviously correct (I'm\nglossing over the earlier part where you call prepare_repo_settings();\nalso required and obviously correct).\n\n> +test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n> +\tcd \"$TRASH_DIRECTORY/full\" &&\n> +\tgit fsck &&\n> +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> +\t\t\"incorrect checksum\" &&\n> +\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n> +\tgit -c core.commitGraph=false fsck\n\nNit; I recommend replacing the `-c` style configuration with\n`test_config`, which modifies `$GIT_DIR/config` but only for the\nduration of the sub-shell.\n\n> +'\n> +\n> +test_expect_success 'git fsck (checks commit-graph when config unset)' '\n> +\ttest_when_finished \"git config core.commitGraph true\" &&\n\n... which would allow you to get rid of something like this (since you\ncan avoid modifying the state visible outside of this test).\n\n> +\tcd \"$TRASH_DIRECTORY/full\" &&\n> +\tgit fsck &&\n> +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> +\t\t\"incorrect checksum\" &&\n> + git config --unset core.commitGraph &&\n\nBut I'm not aware of a way to temporarily unset a configuration variable\nfor the duration of a test, so here I would probably write:\n\n test_must_fail git -c core.commitGraph= fsck\n\nwhich Git interprets as \"pretend this variable is unset in-core\".\n\nThanks,\nTaylor\n"},{"id":"435779","messageId":"CAPig+cRbSUaahdRr9pj9==_7g0Om-Zax93zBRBiJg0-X7zXnOA@mail.gmail.com","threadId":"56494","inReplyTo":"YT+mlW851sRyt842@nand.local","subject":"Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-13T19:33:40Z","receivedAt":"2021-09-13T19:33:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 13, 2021 at 3:29 PM Taylor Blau wrote:\n> On Mon, Sep 13, 2021 at 11:12:19AM -0700, Glen Choo wrote:\n> > +test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n> > + cd \"$TRASH_DIRECTORY/full\" &&\n> > + git fsck &&\n> > + corrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> > + \"incorrect checksum\" &&\n> > + cp commit-graph-pre-write-test $objdir/info/commit-graph &&\n> > + git -c core.commitGraph=false fsck\n>\n> Nit; I recommend replacing the `-c` style configuration with\n> `test_config`, which modifies `$GIT_DIR/config` but only for the\n> duration of the sub-shell.\n\nSmall correction: There is no subshell here, so it would be more\naccurate to say \"... for the duration of the test\".\n\nAside: In fact, test_config() can't be used in subshells due to\nimplementation limitations which I won't go into.\n"},{"id":"435780","messageId":"YT+n81yGPYEiMXwQ@nand.local","threadId":"56494","inReplyTo":"20210913181221.42635-3-chooglen@google.com","subject":"Re: [PATCH 2/3] fsck: verify multi-pack-index when implictly enabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-13T19:35:15Z","receivedAt":"2021-09-13T19:35:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 13, 2021 at 11:12:20AM -0700, Glen Choo wrote:\n> Like the previous commit, the_repository->settings.core_multi_pack_index\n> is preferable to reading \"core.multiPackIndex\" from the config because\n> prepare_repo_settings() sets a default value for\n> the_repository->settings.\n\nMy same suggestion about referencing the first commit for which this\nwould have been broken applies here, too. In this case, that'd be\n18e449f86b (midx: enable core.multiPackIndex by default, 2020-09-25).\n\n> Replace git_config_get_bool(\"core.multiPackIndex\") in fsck with the\n> equivalent call to the_repository->settings.multi_pack_index.\n>\n> Signed-off-by: Glen Choo \n> ---\n> builtin/fsck.c | 2 +-\n> t/t5319-multi-pack-index.sh | 15 ++++++++++++++-\n> 2 files changed, 15 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index 1c4e485b66..5bbe8068ec 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -925,7 +925,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n>\n> -\tif (!git_config_get_bool(\"core.multipackindex\", &i) && i) {\n> +\tif (the_repository->settings.core_multi_pack_index) {\n> \t\tstruct child_process midx_verify = CHILD_PROCESS_INIT;\n> \t\tconst char *midx_argv[] = { \"multi-pack-index\", \"verify\", NULL, NULL, NULL };\n\nGood.\n\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 3d4d9f10c3..1a17446cf0 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -404,12 +404,25 @@ test_expect_success 'verify incorrect offset' '\n> \t\t\"incorrect object offset\"\n> '\n>\n> -test_expect_success 'git-fsck incorrect offset' '\n> +test_expect_success 'git-fsck incorrect offset (config set to true)' '\n> \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n> \t\t\"incorrect object offset\" \\\n> \t\t\"git -c core.multipackindex=true fsck\"\n> '\n>\n> +test_expect_success 'git-fsck incorrect offset (config set to false)' '\n> +\tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n> +\t\t\"incorrect object offset\" \\\n> +\t\t\"git -c core.multipackindex=true fsck\" &&\n> +\t\tgit -c core.multipackindex=false fsck\n> +'\n\nThis test is a little awkward, since it looks like the first command is\nthe same as the previous test (making it unnecessary). But it turns out\nthat it *is* necessary, since it sets up the corrupt MIDX (which is\nthrown away at the end of each test).\n\nSo I would suggest one of two things:\n\n - Either replace this with a more flexible version of\n corrupt_midx_and_verify that allows the enclosing test to pass. Note\n that this may be awkward, since the whole point is that we want to\n notice the corruption in the first place.\n\n - Or, (more favorably) combine this all into a single test, like so:\n\n corrupt_midx_and_verify \"$MIDX_BYTE_OFFSET\" \"\\377\" $objdir \\\n \"incorrect object offset\" \\\n \"git fsck\" &&\n test_must_fail git -c core.multiPackIndex=true fsck &&\n git -c core.multiPackIndex=false fsck\n\nWhich does all of the setup for the three tests at once, and then\nspecifies each behavior individually.\n\nThanks,\nTaylor\n"},{"id":"435781","messageId":"YT+oL15j6Xq05RmS@nand.local","threadId":"56494","inReplyTo":"CAPig+cRbSUaahdRr9pj9==_7g0Om-Zax93zBRBiJg0-X7zXnOA@mail.gmail.com","subject":"Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-13T19:36:15Z","receivedAt":"2021-09-13T19:36:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 13, 2021 at 03:33:40PM -0400, Eric Sunshine wrote:\n> On Mon, Sep 13, 2021 at 3:29 PM Taylor Blau wrote:\n> > On Mon, Sep 13, 2021 at 11:12:19AM -0700, Glen Choo wrote:\n> > > +test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n> > > + cd \"$TRASH_DIRECTORY/full\" &&\n> > > + git fsck &&\n> > > + corrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> > > + \"incorrect checksum\" &&\n> > > + cp commit-graph-pre-write-test $objdir/info/commit-graph &&\n> > > + git -c core.commitGraph=false fsck\n> >\n> > Nit; I recommend replacing the `-c` style configuration with\n> > `test_config`, which modifies `$GIT_DIR/config` but only for the\n> > duration of the sub-shell.\n>\n> Small correction: There is no subshell here, so it would be more\n> accurate to say \"... for the duration of the test\".\n\nYes, sorry about that: I did not mean to say \"subshell\" here (Glen:\nhopefully you didn't read too far into that, since test_config() does\nnot work in subshells).\n\nThanks for catching that, Eric.\n\nThanks,\nTaylor\n"},{"id":"435782","messageId":"YT+odsA17BqtD+WJ@nand.local","threadId":"56494","inReplyTo":"20210913181221.42635-4-chooglen@google.com","subject":"Re: [PATCH 3/3] gc: perform incremental repack when implictly enabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-13T19:37:26Z","receivedAt":"2021-09-13T19:37:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 13, 2021 at 11:12:21AM -0700, Glen Choo wrote:\n> builtin/gc.c has two ways of checking of multi-pack-index is enabled:\n> - git_config_get_bool() in incremental_repack_auto_condition()\n> - the_repository->settings.core_multi_pack_index in\n> maintenance_task_incremental_repack()\n\nIs this hinting at why there are no new tests added here? If so, it may\nneed to be explained more clearly, since I was a tad confused after\nreading it. But if not, this patch message deserves an extra sentence or\ntwo saying why tests aren't necessary.\n\nOr if none of that is the case, and tests *are* necessary, then they\nshould be added ;).\n\nThanks,\nTaylor\n"},{"id":"435797","messageId":"YT/bfZu5mnMMqq0z@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"YT+mlW851sRyt842@nand.local","subject":"Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-13T23:15:09Z","receivedAt":"2021-09-13T23:19:37Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks for the thorough review! I really appreciate it :)\n\nOn Mon, Sep 13, 2021 at 03:29:25PM -0400, Taylor Blau wrote:\n> Small nit; \"the_repository->settings()\" should be spelled as\n> \"the_repository->settings\", since \"settings\" is not a function.\n\nOh that's a good catch. Thanks!\n\n> It may be worth noting that this was totally fine before\n> core.commitGraph's default changed to true. That happened in 31b1de6a09\n> (commit-graph: turn on commit-graph by default, 2019-08-13), which is\n> the first time this sort of regression would have appeared.\n\nSounds good, I'll note that down.\n\n> Nit; I recommend replacing the `-c` style configuration with\n> `test_config`, which modifies `$GIT_DIR/config` but only for the\n> duration of the sub-shell.\n\nSounds good. This works great :)\n\n> > +\tcd \"$TRASH_DIRECTORY/full\" &&\n> > +\tgit fsck &&\n> > +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> > +\t\t\"incorrect checksum\" &&\n> > + git config --unset core.commitGraph &&\n> \n> But I'm not aware of a way to temporarily unset a configuration variable\n> for the duration of a test, so here I would probably write:\n> \n> test_must_fail git -c core.commitGraph= fsck\n> \n> which Git interprets as \"pretend this variable is unset in-core\".\n\nFrom my testing, I suspect that git does not pretend the variable is\nunset, but rather, it pretends that the variable is set to the empty\nstring. It seems that git behaves as if the variable is set to \"false\".\nThis is called out in Documentation/git.txt:\n\n Including the equals but with an empty value (like `git -c\n foo.bar= ...`) sets `foo.bar` to the empty string which `git config\n --type=bool` will convert to `false`.\n\nIf the variable really is set to false, how might we proceed here? Shall\nwe stick with test_when_finished?\n"},{"id":"435798","messageId":"CAPig+cS2E6c9YYKTThPSVpyBkouoL+nK2rgpsZxNODPmTr2hzQ@mail.gmail.com","threadId":"56494","inReplyTo":"YT/bfZu5mnMMqq0z@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-13T23:32:07Z","receivedAt":"2021-09-13T23:32:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 13, 2021 at 7:19 PM Glen Choo wrote:\n> On Mon, Sep 13, 2021 at 03:29:25PM -0400, Taylor Blau wrote:\n> > > + git config --unset core.commitGraph &&\n> >\n> > But I'm not aware of a way to temporarily unset a configuration variable\n> > for the duration of a test, so here I would probably write:\n> >\n> > test_must_fail git -c core.commitGraph= fsck\n> >\n> > which Git interprets as \"pretend this variable is unset in-core\".\n>\n> From my testing, I suspect that git does not pretend the variable is\n> unset, but rather, it pretends that the variable is set to the empty\n> string. It seems that git behaves as if the variable is set to \"false\".\n> This is called out in Documentation/git.txt:\n>\n> Including the equals but with an empty value (like `git -c\n> foo.bar= ...`) sets `foo.bar` to the empty string which `git config\n> --type=bool` will convert to `false`.\n>\n> If the variable really is set to false, how might we proceed here? Shall\n> we stick with test_when_finished?\n\nThat's probably reasonable, however, for robustness, you should\nprobably use test_unconfig() rather than raw `git config --unset` to\nclear the variable.\n\nAside: This certainly makes one wonder if we should have a new\nfunction in t/test-lib-functions.sh which unsets a variable for the\nduration of a test only. However, that's outside the scope of this\nsubmission.\n"},{"id":"435799","messageId":"YT/1tGxiCNTsWPBz@nand.local","threadId":"56494","inReplyTo":"YT/bfZu5mnMMqq0z@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T01:07:00Z","receivedAt":"2021-09-14T01:07:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 13, 2021 at 04:15:09PM -0700, Glen Choo wrote:\n> > > +\tcd \"$TRASH_DIRECTORY/full\" &&\n> > > +\tgit fsck &&\n> > > +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> > > +\t\t\"incorrect checksum\" &&\n> > > + git config --unset core.commitGraph &&\n> >\n> > But I'm not aware of a way to temporarily unset a configuration variable\n> > for the duration of a test, so here I would probably write:\n> >\n> > test_must_fail git -c core.commitGraph= fsck\n> >\n> > which Git interprets as \"pretend this variable is unset in-core\".\n>\n> From my testing, I suspect that git does not pretend the variable is\n> unset, but rather, it pretends that the variable is set to the empty\n> string. It seems that git behaves as if the variable is set to \"false\".\n> This is called out in Documentation/git.txt:\n>\n> Including the equals but with an empty value (like `git -c\n> foo.bar= ...`) sets `foo.bar` to the empty string which `git config\n> --type=bool` will convert to `false`.\n>\n> If the variable really is set to false, how might we proceed here? Shall\n> we stick with test_when_finished?\n\nHmm, I thought that we did support `git -c foo.bar=` as shorthand to\nunset the key `foo.bar`, but I must have been mistaken, because the\ndocumentation there is quite clear.\n\nIn that case, I think your original approach to use test_when_finished\nmakes sense and is good. Thanks for double checking!\n\nThanks,\nTaylor\n"},{"id":"435800","messageId":"YT/2PzbfE2p6TZ9r@nand.local","threadId":"56494","inReplyTo":"CAPig+cS2E6c9YYKTThPSVpyBkouoL+nK2rgpsZxNODPmTr2hzQ@mail.gmail.com","subject":"Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T01:09:19Z","receivedAt":"2021-09-14T01:09:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 13, 2021 at 07:32:07PM -0400, Eric Sunshine wrote:\n> On Mon, Sep 13, 2021 at 7:19 PM Glen Choo wrote:\n> > On Mon, Sep 13, 2021 at 03:29:25PM -0400, Taylor Blau wrote:\n> > > > + git config --unset core.commitGraph &&\n> > >\n> > > But I'm not aware of a way to temporarily unset a configuration variable\n> > > for the duration of a test, so here I would probably write:\n> > >\n> > > test_must_fail git -c core.commitGraph= fsck\n> > >\n> > > which Git interprets as \"pretend this variable is unset in-core\".\n> >\n> > From my testing, I suspect that git does not pretend the variable is\n> > unset, but rather, it pretends that the variable is set to the empty\n> > string. It seems that git behaves as if the variable is set to \"false\".\n> > This is called out in Documentation/git.txt:\n> >\n> > Including the equals but with an empty value (like `git -c\n> > foo.bar= ...`) sets `foo.bar` to the empty string which `git config\n> > --type=bool` will convert to `false`.\n> >\n> > If the variable really is set to false, how might we proceed here? Shall\n> > we stick with test_when_finished?\n>\n> That's probably reasonable, however, for robustness, you should\n> probably use test_unconfig() rather than raw `git config --unset` to\n> clear the variable.\n\nHmm. I'm not so sure, do other tests rely on the value of that variable?\nIf so, test_unconfig() won't restore them.\n\nEven if we don't have any such tests now, it seems like we should err on\nthe side of leaving it alone (although I suppose that future tests could\nset core.commitGraph to whatever value they need as long as they use the\ntest_config function).\n\n> Aside: This certainly makes one wonder if we should have a new\n> function in t/test-lib-functions.sh which unsets a variable for the\n> duration of a test only. However, that's outside the scope of this\n> submission.\n\n:-). I thought the same thing to myself when reviewing earlier today.\nThat's why I recommended using test_when_finished upthread, but either\napproach is fine (my comments are definitely cosmetic, and don't matter\nto the substance of this thread, so ultimately I am fine with either).\n\nThanks,\nTaylor\n"},{"id":"435806","messageId":"CAPig+cQ8q_nNgRd3taVP4RPpxK3xcDVQ+57Dw0biA0qnghUMkg@mail.gmail.com","threadId":"56494","inReplyTo":"YT/2PzbfE2p6TZ9r@nand.local","subject":"Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-14T02:05:51Z","receivedAt":"2021-09-14T02:06:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 13, 2021 at 9:09 PM Taylor Blau wrote:\n> On Mon, Sep 13, 2021 at 07:32:07PM -0400, Eric Sunshine wrote:\n> > On Mon, Sep 13, 2021 at 7:19 PM Glen Choo wrote:\n> > > If the variable really is set to false, how might we proceed here? Shall\n> > > we stick with test_when_finished?\n> >\n> > That's probably reasonable, however, for robustness, you should\n> > probably use test_unconfig() rather than raw `git config --unset` to\n> > clear the variable.\n>\n> Hmm. I'm not so sure, do other tests rely on the value of that variable?\n> If so, test_unconfig() won't restore them.\n\nThere may be a misunderstanding. I wasn't saying that test_unconfig()\nalone would work. My \"That's probably reasonable\" referred to Glen's\nproposal of combining `git config --unset` with test_when_finished()\nto restore the variable. In addition to that, I suggested\ntest_unconfig() as being a more robust choice in that recipe.\n\n> > Aside: This certainly makes one wonder if we should have a new\n> > function in t/test-lib-functions.sh which unsets a variable for the\n> > duration of a test only. However, that's outside the scope of this\n> > submission.\n>\n> :-). I thought the same thing to myself when reviewing earlier today.\n> That's why I recommended using test_when_finished upthread, but either\n> approach is fine (my comments are definitely cosmetic, and don't matter\n> to the substance of this thread, so ultimately I am fine with either).\n\nYep.\n"},{"id":"435818","messageId":"e890300b-4a9d-8c08-84f2-c20b28214da2@gmail.com","threadId":"56494","inReplyTo":"20210913181221.42635-4-chooglen@google.com","subject":"Re: [PATCH 3/3] gc: perform incremental repack when implictly enabled","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-09-14T04:00:34Z","receivedAt":"2021-09-14T04:00:40Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 14/09/21 01.12, Glen Choo wrote:\n> builtin/gc.c has two ways of checking of multi-pack-index is enabled:\n> - git_config_get_bool() in incremental_repack_auto_condition()\n> - the_repository->settings.core_multi_pack_index in\n> maintenance_task_incremental_repack()\n> \n> the_repository->settings.core_multi_pack_index should be preferred\n> because it has default values from prepare_repo_settings().\n> \n> Standardize on the_repository->settings.core_multi_pack_index to check\n> if multi-pack-index is enabled.\n\nI was wonder what the correlations between checking if MIDX is enabled \nand performing incremental repack are. What are these?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"435894","messageId":"YUDe1Spvh4r7VyA5@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"YT+odsA17BqtD+WJ@nand.local","subject":"Re: [PATCH 3/3] gc: perform incremental repack when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-14T17:41:41Z","receivedAt":"2021-09-14T17:41:53Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"On Mon, Sep 13, 2021 at 03:37:26PM -0400, Taylor Blau wrote:\n> > builtin/gc.c has two ways of checking of multi-pack-index is enabled:\n> > - git_config_get_bool() in incremental_repack_auto_condition()\n> > - the_repository->settings.core_multi_pack_index in\n> > maintenance_task_incremental_repack()\n> \n> Is this hinting at why there are no new tests added here? If so, it may\n> need to be explained more clearly, since I was a tad confused after\n> reading it.\n\nLooks like I'll need to be clearer; this wasn't my intention at all. I\nwas hoping to describe the current state of affairs and to show that\nthere are two different approaches. Thus, this patch is an attempt to\n'standardize' the two approaches.\n\n> But if not, this patch message deserves an extra sentence or\n> two saying why tests aren't necessary.\n>\n> Or if none of that is the case, and tests *are* necessary, then they\n> should be added ;).\n\nI initially did not include tests because it *seemed* to me that there\nwere no tests for this. But on a second pass, it looks like that\nassumption was completely wrong (the tests are in t7900-maintenance.sh).\n\nSo, tests are necessary, and so I will add them :)\n"},{"id":"436134","messageId":"YUN7qEJ2C9wuAuEu@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"e890300b-4a9d-8c08-84f2-c20b28214da2@gmail.com","subject":"Re: [PATCH 3/3] gc: perform incremental repack when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-16T17:15:20Z","receivedAt":"2021-09-16T18:43:58Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"On Tue, Sep 14, 2021 at 11:00:34AM +0700, Bagas Sanjaya wrote:\n> I was wonder what the correlations between checking if MIDX is enabled and\n> performing incremental repack are. What are these?\n\n(I'm not an expert on the issue, so don't take what I say as definitive :))\n\nHere's what I was able to find in Documentation/git-maintenance.txt:\n\n The `incremental-repack` job repacks the object directory\n\tusing the `multi-pack-index` feature.[...]\n\nMy guess at the intended behavior is that disabling midx should make\ngit-maintenance behave as if it were unaware of midx features, such as\nincremental repack. This is quite akin to the documented behavior of fsck in\nthis patch series, where fsck only verifies midx and commit-graphs when the\nrespective features are enabled.\n"},{"id":"436277","messageId":"20210917225459.68086-1-chooglen@google.com","threadId":"56494","inReplyTo":"20210913181221.42635-1-chooglen@google.com","subject":"[PATCH v2 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-17T22:54:56Z","receivedAt":"2021-09-17T22:55:11Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks so much for the review, the comments have been really helpful!\nv2 doesn't change any business logic, it's mostly focused on\nincorporating the feedback around commit messages and tests.\n\nChanges since v1:\n\n* clean up typo in patch 1 commit message \n* document the commits that patches 1 and 2 address\n* use test helpers in patch 1\n* rewrite patch 2's tests so that it's easier to tell that each test\n does something different\n* reword patch 3 commit message to explain the bug\n* add tests to patch 3\n\nGlen Choo (3):\n fsck: verify commit graph when implicitly enabled\n fsck: verify multi-pack-index when implictly enabled\n gc: perform incremental repack when implictly enabled\n\n builtin/fsck.c | 5 ++-\n builtin/gc.c | 5 +--\n t/t5318-commit-graph.sh | 24 +++++++++-\n t/t5319-multi-pack-index.sh | 4 +-\n t/t7900-maintenance.sh | 88 ++++++++++++++++++++++++++-----------\n 5 files changed, 94 insertions(+), 32 deletions(-)\n\nRange-diff against v1:\n1: 6bddc4e158 ! 1: 97ab2bb529 fsck: verify commit graph when implicitly enabled\n @@ Metadata\n ## Commit message ##\n fsck: verify commit graph when implicitly enabled\n \n - the_repository->settings() is the preferred way to get certain\n + the_repository->settings is the preferred way to get certain\n settings (such as \"core.commitGraph\") because it gets default values\n from prepare_repo_settings(). However, cmd_fsck() reads the config\n directly via git_config_get_bool(), which bypasses these default values.\n This causes fsck to ignore the commit graph if \"core.commitgraph\" is not\n - explicitly set in the config, even though commit graph is enabled by\n - default.\n + explicitly set in the config. This worked fine until commit-graph was\n + enabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by\n + default, 2019-08-13).\n \n Replace git_config_get_bool(\"core.commitgraph\") in fsck with the\n equivalent call to the_repository->settings.core_commit_graph.\n @@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '\n +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n +\t\t\"incorrect checksum\" &&\n +\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n -+\tgit -c core.commitGraph=false fsck\n ++\ttest_config core.commitGraph false &&\n ++\tgit fsck\n +'\n +\n +test_expect_success 'git fsck (checks commit-graph when config unset)' '\n @@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '\n +\tgit fsck &&\n +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n +\t\t\"incorrect checksum\" &&\n -+ git config --unset core.commitGraph &&\n ++\ttest_unconfig core.commitGraph &&\n +\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n +\ttest_must_fail git fsck\n +'\n2: 3b86afc45b ! 2: ace92453ca fsck: verify multi-pack-index when implictly enabled\n @@ Commit message\n Like the previous commit, the_repository->settings.core_multi_pack_index\n is preferable to reading \"core.multiPackIndex\" from the config because\n prepare_repo_settings() sets a default value for\n - the_repository->settings.\n + the_repository->settings. This worked fine until core.multiPackIndex was\n + enabled by default in 18e449f86b (midx: enable core.multiPackIndex by\n + default, 2020-09-25).\n \n Replace git_config_get_bool(\"core.multiPackIndex\") in fsck with the\n equivalent call to the_repository->settings.multi_pack_index.\n @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n ## t/t5319-multi-pack-index.sh ##\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'verify incorrect offset' '\n - \t\t\"incorrect object offset\"\n - '\n - \n --test_expect_success 'git-fsck incorrect offset' '\n -+test_expect_success 'git-fsck incorrect offset (config set to true)' '\n + test_expect_success 'git-fsck incorrect offset' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" \\\n - \t\t\"git -c core.multipackindex=true fsck\"\n - '\n - \n -+test_expect_success 'git-fsck incorrect offset (config set to false)' '\n -+\tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n -+\t\t\"incorrect object offset\" \\\n +-\t\t\"git -c core.multipackindex=true fsck\"\n +\t\t\"git -c core.multipackindex=true fsck\" &&\n ++\t\ttest_must_fail git fsck &&\n +\t\tgit -c core.multipackindex=false fsck\n -+'\n -+\n -+test_expect_success 'git-fsck incorrect offset (config unset)' '\n -+\tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n -+\t\t\"incorrect object offset\" \\\n -+\t\t\"git fsck\"\n -+'\n -+\n + '\n + \n test_expect_success 'corrupt MIDX is not reused' '\n - \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n - \t\t\"incorrect object offset\" &&\n3: d122ce3bf8 ! 3: d6959d61c1 gc: perform incremental repack when implictly enabled\n @@ Metadata\n ## Commit message ##\n gc: perform incremental repack when implictly enabled\n \n - builtin/gc.c has two ways of checking of multi-pack-index is enabled:\n + builtin/gc.c has two ways of checking if multi-pack-index is enabled:\n - git_config_get_bool() in incremental_repack_auto_condition()\n - the_repository->settings.core_multi_pack_index in\n maintenance_task_incremental_repack()\n \n - the_repository->settings.core_multi_pack_index should be preferred\n - because it has default values from prepare_repo_settings().\n + The two implementations have existed since the incremental-repack task\n + was introduced in e841a79a13 (maintenance: add incremental-repack auto\n + condition, 2020-09-25). These two values can diverge because\n + prepare_repo_settings() enables the feature in the_repository->settings\n + by default.\n \n - Standardize on the_repository->settings.core_multi_pack_index to check\n - if multi-pack-index is enabled.\n + In the case where core.multiPackIndex is not set in the config, the auto\n + condition would fail, causing the incremental-repack task to not be\n + run. Because we always want to consider the default values, we should\n + just always just use the_repository->settings.\n +\n + Standardize on using the_repository->settings.core_multi_pack_index to\n + check if multi-pack-index is enabled.\n \n Signed-off-by: Glen Choo \n \n @@ builtin/gc.c: static int maintenance_task_loose_objects(struct maintenance_run_o\n \t\treturn 0;\n \n \tgit_config_get_int(\"maintenance.incremental-repack.auto\",\n +\n + ## t/t7900-maintenance.sh ##\n +@@ t/t7900-maintenance.sh: test_expect_success EXPENSIVE 'incremental-repack 2g limit' '\n + '\n + \n + test_expect_success 'maintenance.incremental-repack.auto' '\n +-\tgit repack -adk &&\n +-\tgit config core.multiPackIndex true &&\n +-\tgit multi-pack-index write &&\n +-\tGIT_TRACE2_EVENT=\"$(pwd)/midx-init.txt\" git \\\n +-\t\t-c maintenance.incremental-repack.auto=1 \\\n +-\t\tmaintenance run --auto --task=incremental-repack 2>/dev/null &&\n +-\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n +-\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n +-\ttest_subcommand git multi-pack-index write --no-progress /dev/null &&\n ++\t\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n ++\t\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n ++\t\ttest_subcommand git multi-pack-index write --no-progress /dev/null &&\n ++\t\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n ++\t\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n ++\t\ttest_subcommand git multi-pack-index write --no-progress settings is the preferred way to get certain\nsettings (such as \"core.commitGraph\") because it gets default values\nfrom prepare_repo_settings(). However, cmd_fsck() reads the config\ndirectly via git_config_get_bool(), which bypasses these default values.\nThis causes fsck to ignore the commit graph if \"core.commitgraph\" is not\nexplicitly set in the config. This worked fine until commit-graph was\nenabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by\ndefault, 2019-08-13).\n\nReplace git_config_get_bool(\"core.commitgraph\") in fsck with the\nequivalent call to the_repository->settings.core_commit_graph.\n\nThe expected behavior is that fsck respects the config value when it is\nset, but uses the default value when it is unset. For example, for\ncore.commitGraph, there are three cases:\n\n- Config value is set to true -> enabled\n- Config value is set to false -> disabled\n- Config value is unset -> enabled\n\nAs such, tests cover all three cases.\n\nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 3 ++-\n t/t5318-commit-graph.sh | 24 +++++++++++++++++++++++-\n 2 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex b42b6fe21f..1c4e485b66 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -803,6 +803,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tfsck_enable_object_names(&fsck_walk_options);\n \n \tgit_config(git_fsck_config, &fsck_obj_options);\n+\tprepare_repo_settings(the_repository);\n \n \tif (connectivity_only) {\n \t\tfor_each_loose_object(mark_loose_for_connectivity, NULL, 0);\n@@ -908,7 +909,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tcheck_connectivity();\n \n-\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n+\tif (the_repository->settings.core_commit_graph) {\n \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex af88f805aa..42e785cb6e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -674,7 +674,7 @@ test_expect_success 'detect incorrect chunk count' '\n \t\t$GRAPH_CHUNK_LOOKUP_OFFSET\n '\n \n-test_expect_success 'git fsck (checks commit-graph)' '\n+test_expect_success 'git fsck (checks commit-graph when config set to true)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n@@ -683,6 +683,28 @@ test_expect_success 'git fsck (checks commit-graph)' '\n \ttest_must_fail git fsck\n '\n \n+test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\ttest_config core.commitGraph false &&\n+\tgit fsck\n+'\n+\n+test_expect_success 'git fsck (checks commit-graph when config unset)' '\n+\ttest_when_finished \"git config core.commitGraph true\" &&\n+\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\ttest_unconfig core.commitGraph &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\ttest_must_fail git fsck\n+'\n+\n test_expect_success 'setup non-the_repository tests' '\n \trm -rf repo &&\n \tgit init repo &&\n-- \n2.33.0.464.g1972c5931b-goog\n\n"},{"id":"436279","messageId":"20210917225459.68086-3-chooglen@google.com","threadId":"56494","inReplyTo":"20210917225459.68086-1-chooglen@google.com","subject":"[PATCH v2 2/3] fsck: verify multi-pack-index when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-17T22:54:58Z","receivedAt":"2021-09-17T22:55:19Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Like the previous commit, the_repository->settings.core_multi_pack_index\nis preferable to reading \"core.multiPackIndex\" from the config because\nprepare_repo_settings() sets a default value for\nthe_repository->settings. This worked fine until core.multiPackIndex was\nenabled by default in 18e449f86b (midx: enable core.multiPackIndex by\ndefault, 2020-09-25).\n\nReplace git_config_get_bool(\"core.multiPackIndex\") in fsck with the\nequivalent call to the_repository->settings.multi_pack_index.\n\nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 2 +-\n t/t5319-multi-pack-index.sh | 4 +++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1c4e485b66..5bbe8068ec 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -925,7 +925,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (!git_config_get_bool(\"core.multipackindex\", &i) && i) {\n+\tif (the_repository->settings.core_multi_pack_index) {\n \t\tstruct child_process midx_verify = CHILD_PROCESS_INIT;\n \t\tconst char *midx_argv[] = { \"multi-pack-index\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3d4d9f10c3..704db122b1 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -407,7 +407,9 @@ test_expect_success 'verify incorrect offset' '\n test_expect_success 'git-fsck incorrect offset' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" \\\n-\t\t\"git -c core.multipackindex=true fsck\"\n+\t\t\"git -c core.multipackindex=true fsck\" &&\n+\t\ttest_must_fail git fsck &&\n+\t\tgit -c core.multipackindex=false fsck\n '\n \n test_expect_success 'corrupt MIDX is not reused' '\n-- \n2.33.0.464.g1972c5931b-goog\n\n"},{"id":"436280","messageId":"20210917225459.68086-4-chooglen@google.com","threadId":"56494","inReplyTo":"20210917225459.68086-1-chooglen@google.com","subject":"[PATCH v2 3/3] gc: perform incremental repack when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-17T22:54:59Z","receivedAt":"2021-09-17T22:55:22Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"builtin/gc.c has two ways of checking if multi-pack-index is enabled:\n- git_config_get_bool() in incremental_repack_auto_condition()\n- the_repository->settings.core_multi_pack_index in\n maintenance_task_incremental_repack()\n\nThe two implementations have existed since the incremental-repack task\nwas introduced in e841a79a13 (maintenance: add incremental-repack auto\ncondition, 2020-09-25). These two values can diverge because\nprepare_repo_settings() enables the feature in the_repository->settings\nby default.\n\nIn the case where core.multiPackIndex is not set in the config, the auto\ncondition would fail, causing the incremental-repack task to not be\nrun. Because we always want to consider the default values, we should\njust always just use the_repository->settings.\n\nStandardize on using the_repository->settings.core_multi_pack_index to\ncheck if multi-pack-index is enabled.\n\nSigned-off-by: Glen Choo \n---\n builtin/gc.c | 5 +--\n t/t7900-maintenance.sh | 88 ++++++++++++++++++++++++++++++------------\n 2 files changed, 65 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex f05d2f0a1a..070b7dccb1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1051,12 +1051,11 @@ static int maintenance_task_loose_objects(struct maintenance_run_opts *opts)\n static int incremental_repack_auto_condition(void)\n {\n \tstruct packed_git *p;\n-\tint enabled;\n \tint incremental_repack_auto_limit = 10;\n \tint count = 0;\n \n-\tif (git_config_get_bool(\"core.multiPackIndex\", &enabled) ||\n-\t !enabled)\n+\tprepare_repo_settings(the_repository);\n+\tif (!the_repository->settings.core_multi_pack_index)\n \t\treturn 0;\n \n \tgit_config_get_int(\"maintenance.incremental-repack.auto\",\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 58f46c77e6..2c77ddded1 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -322,31 +322,69 @@ test_expect_success EXPENSIVE 'incremental-repack 2g limit' '\n '\n \n test_expect_success 'maintenance.incremental-repack.auto' '\n-\tgit repack -adk &&\n-\tgit config core.multiPackIndex true &&\n-\tgit multi-pack-index write &&\n-\tGIT_TRACE2_EVENT=\"$(pwd)/midx-init.txt\" git \\\n-\t\t-c maintenance.incremental-repack.auto=1 \\\n-\t\tmaintenance run --auto --task=incremental-repack 2>/dev/null &&\n-\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n-\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n-\ttest_subcommand git multi-pack-index write --no-progress /dev/null &&\n+\t\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n+\t\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n+\t\ttest_subcommand git multi-pack-index write --no-progress /dev/null &&\n+\t\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n+\t\ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n+\t\ttest_subcommand git multi-pack-index write --no-progress the_repository->settings is the preferred way to get certain\n> settings (such as \"core.commitGraph\") because it gets default values\n> from prepare_repo_settings(). However, cmd_fsck() reads the config\n> directly via git_config_get_bool(), which bypasses these default values.\n> This causes fsck to ignore the commit graph if \"core.commitgraph\" is not\n> explicitly set in the config. This worked fine until commit-graph was\n> enabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by\n> default, 2019-08-13).\n> \n> Replace git_config_get_bool(\"core.commitgraph\") in fsck with the\n> equivalent call to the_repository->settings.core_commit_graph.\n> [...]\n> Signed-off-by: Glen Choo \n> ---\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> @@ -674,7 +674,7 @@ test_expect_success 'detect incorrect chunk count' '\n> -test_expect_success 'git fsck (checks commit-graph)' '\n> +test_expect_success 'git fsck (checks commit-graph when config set to true)' '\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> \tgit fsck &&\n> \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> @@ -683,6 +683,28 @@ test_expect_success 'git fsck (checks commit-graph)' '\n> \ttest_must_fail git fsck\n> '\n\nBecause I took the time to scan backward through this test script, I \nunderstand that `core.commitGraph` is already `true` by the time this \ntest is reached, thus the new test title is accurate (for now). However, \nit would be a bit easier to reason about this and make it more robust by \nhaving the test itself guarantee that it is set to `true` by invoking \n`git config core.commitGraph true`.\n\n> +test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n> +\tcd \"$TRASH_DIRECTORY/full\" &&\n> +\tgit fsck &&\n> +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> +\t\t\"incorrect checksum\" &&\n> +\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n> +\ttest_config core.commitGraph false &&\n> +\tgit fsck\n> +'\n\ntest_config() unsets the config variable when the test completes, so I'm \nwondering if its use is in fact desirable here. I ask because, from a \nquick scan through the file, it appears that many tests in this script \nassume that `core.commitGraph` is `true`, so it's not clear that \nunsetting it at the end of this test is a good idea in general.\n\n> +test_expect_success 'git fsck (checks commit-graph when config unset)' '\n> +\ttest_when_finished \"git config core.commitGraph true\" &&\n> +\n> +\tcd \"$TRASH_DIRECTORY/full\" &&\n\nI find it somewhat confusing to reason about the behavior of \ntest_when_finished() when it is invoked like this before the `cd`. It's \ntrue that the end-of-test `git config core.commitGraph true` will be \ninvoked within `full` as desired (except in the very unusual case of the \n`cd` failing), so it's probably correct, but it requires extra thought \nto come to that conclusion. Switching the order of these two lines might \nlower the cognitive load a bit.\n\n> +\tgit fsck &&\n> +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n> +\t\t\"incorrect checksum\" &&\n> +\ttest_unconfig core.commitGraph &&\n> +\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n> +\ttest_must_fail git fsck\n> +'\n\nTaking Taylor's comment[1] on v1 patch [2/3] into account, I wonder if \nit would be simpler for these all to be combined into a single test. \nSomething like (untested):\n\n cd \"$TRASH_DIRECTORY/full\" &&\n test_when_finished \"git config core.commitGraph true\" &&\n git config core.commitGraph true &&\n git fsck &&\n corrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \"incorrect checksum\" &&\n cp commit-graph-pre-write-test $objdir/info/commit-graph &&\n test_must_fail git fsck &&\n git config core.commitGraph false &&\n git fsck &&\n git config --unset-all core.commitGraph &&\n test_must_fail git fsck\n\nI didn't bother using test_unconfig() since, in this case, we _know_ \nthat the config variable is set, thus don't have to worry about `git \nconfig --unset-all` failing.\n\nA downside of combining the tests like this is that it makes it a bit \nmore cumbersome to diagnose a failure (because there is more code and \nmore cases to wade through).\n\nAnyhow, I don't have a strong feeling one way or the other about \ncombining the test or leaving them separate.\n\n[1]: https://lore.kernel.org/git/YT+n81yGPYEiMXwQ@nand.local/\n"},{"id":"437435","messageId":"ebe894b4-af25-f79e-c43f-1839e075908d@sunshineco.com","threadId":"56494","inReplyTo":"20210917225459.68086-3-chooglen@google.com","subject":"Re: [PATCH v2 2/3] fsck: verify multi-pack-index when implictly enabled","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-29T06:20:01Z","receivedAt":"2021-09-29T06:20:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On 9/17/21 6:54 PM, Glen Choo wrote:\n> Like the previous commit, the_repository->settings.core_multi_pack_index\n> is preferable to reading \"core.multiPackIndex\" from the config because\n> prepare_repo_settings() sets a default value for\n> the_repository->settings. This worked fine until core.multiPackIndex was\n> enabled by default in 18e449f86b (midx: enable core.multiPackIndex by\n> default, 2020-09-25).\n> \n> Replace git_config_get_bool(\"core.multiPackIndex\") in fsck with the\n> equivalent call to the_repository->settings.multi_pack_index.\n> \n> Signed-off-by: Glen Choo \n> ---\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> @@ -407,7 +407,9 @@ test_expect_success 'verify incorrect offset' '\n> test_expect_success 'git-fsck incorrect offset' '\n> \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n> \t\t\"incorrect object offset\" \\\n> -\t\t\"git -c core.multipackindex=true fsck\"\n> +\t\t\"git -c core.multipackindex=true fsck\" &&\n> +\t\ttest_must_fail git fsck &&\n> +\t\tgit -c core.multipackindex=false fsck\n> '\n\nI guess the newly-added `test_must_fail git fsck` line is checking the \nfallback case then `core.multipackindex` is not set. We could make this \ncheck a bit more robust by _ensuring_ that it is unset rather than \nrelying upon whatever state the configuration is in by the time this \ntest is reached. Perhaps something like this:\n\n ...\n \"git -c core.multipackindex=true fsck\" &&\n test_unconfig core.multipackindex &&\n test_must_fail git fsck &&\n git -c core.multipackindex=false fsck\n\nThe indentation is a bit unusual. It aligns nicely and is, in some \nsense, easy to read, but the two new lines are over-indented considering \nthat they are siblings of the corrupt_midx_and_verify() call.\n\nDon't know if any of this is worth a re-roll, though.\n"},{"id":"437438","messageId":"f0eb2fd0-ce1e-5833-4d64-8569ba3bbedc@sunshineco.com","threadId":"56494","inReplyTo":"20210917225459.68086-4-chooglen@google.com","subject":"Re: [PATCH v2 3/3] gc: perform incremental repack when implictly enabled","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-29T06:39:39Z","receivedAt":"2021-09-29T06:39:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On 9/17/21 6:54 PM, Glen Choo wrote:\n> builtin/gc.c has two ways of checking if multi-pack-index is enabled:\n> - git_config_get_bool() in incremental_repack_auto_condition()\n> - the_repository->settings.core_multi_pack_index in\n> maintenance_task_incremental_repack()\n> \n> The two implementations have existed since the incremental-repack task\n> was introduced in e841a79a13 (maintenance: add incremental-repack auto\n> condition, 2020-09-25). These two values can diverge because\n> prepare_repo_settings() enables the feature in the_repository->settings\n> by default.\n> \n> In the case where core.multiPackIndex is not set in the config, the auto\n> condition would fail, causing the incremental-repack task to not be\n> run. Because we always want to consider the default values, we should\n> just always just use the_repository->settings.\n> \n> Standardize on using the_repository->settings.core_multi_pack_index to\n> check if multi-pack-index is enabled.\n> \n> Signed-off-by: Glen Choo \n> ---\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> @@ -322,31 +322,69 @@ test_expect_success EXPENSIVE 'incremental-repack 2g limit' '\n> test_expect_success 'maintenance.incremental-repack.auto' '\n> +\t(\n> +\t\tgit init incremental-repack-true &&\n> +\t\tcd incremental-repack-true &&\n> +\t\tgit config core.multiPackIndex true &&\n> +\t\ttest_commit A &&\n> +\t\tgit repack -adk &&\n> +\t\tgit multi-pack-index write &&\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/midx-init.txt\" git \\\n> +\t\t\t-c maintenance.incremental-repack.auto=1 \\\n> +\t\t\tmaintenance run --auto --task=incremental-repack 2>/dev/null &&\n> +\t\ttest_subcommand ! git multi-pack-index write --no-progress +\t\ttest_commit B &&\n> +\t\tgit pack-objects --revs .git/objects/pack/pack <<-\\EOF &&\n> +\t\tHEAD\n> +\t\t^HEAD~1\n> +\t\tEOF\n> +\t\tGIT_TRACE2_EVENT=$(pwd)/trace-A git \\\n> +\t\t\t-c maintenance.incremental-repack.auto=2 \\\n> +\t\t\tmaintenance run --auto --task=incremental-repack 2>/dev/null &&\n> +\t\ttest_subcommand ! git multi-pack-index write --no-progress +\t\ttest_commit C &&\n> +\t\tgit pack-objects --revs .git/objects/pack/pack <<-\\EOF &&\n> +\t\tHEAD\n> +\t\t^HEAD~1\n> +\t\tEOF\n> +\t\tGIT_TRACE2_EVENT=$(pwd)/trace-B git \\\n> +\t\t\t-c maintenance.incremental-repack.auto=2 \\\n> +\t\t\tmaintenance run --auto --task=incremental-repack 2>/dev/null &&\n> +\t\ttest_subcommand git multi-pack-index write --no-progress +\t)\n> +'\n> +\n> +test_expect_success 'maintenance.incremental-repack.auto (when config is unset)' '\n> +\t(\n> +\t\tgit init incremental-repack-unset &&\n> +\t\tcd incremental-repack-unset &&\n> +\t\ttest_unconfig core.multiPackIndex &&\n> +\t\ttest_commit A &&\n> +\t\tgit repack -adk &&\n> +\t\tgit multi-pack-index write &&\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/midx-init.txt\" git \\\n> +\t\t\t-c maintenance.incremental-repack.auto=1 \\\n> +\t\t\tmaintenance run --auto --task=incremental-repack 2>/dev/null &&\n> +\t\ttest_subcommand ! git multi-pack-index write --no-progress +\t\ttest_commit B &&\n> +\t\tgit pack-objects --revs .git/objects/pack/pack <<-\\EOF &&\n> +\t\tHEAD\n> +\t\t^HEAD~1\n> +\t\tEOF\n> +\t\tGIT_TRACE2_EVENT=$(pwd)/trace-A git \\\n> +\t\t\t-c maintenance.incremental-repack.auto=2 \\\n> +\t\t\tmaintenance run --auto --task=incremental-repack 2>/dev/null &&\n> +\t\ttest_subcommand ! git multi-pack-index write --no-progress +\t\ttest_commit C &&\n> +\t\tgit pack-objects --revs .git/objects/pack/pack <<-\\EOF &&\n> +\t\tHEAD\n> +\t\t^HEAD~1\n> +\t\tEOF\n> +\t\tGIT_TRACE2_EVENT=$(pwd)/trace-B git \\\n> +\t\t\t-c maintenance.incremental-repack.auto=2 \\\n> +\t\t\tmaintenance run --auto --task=incremental-repack 2>/dev/null &&\n> +\t\ttest_subcommand git multi-pack-index write --no-progress +\t)\n> '\n\nThe amount of code common between the two tests is significant. In \nsimple cases, such duplication may not be worth worrying about, but it \nfeels like we can do better here. There are a number of ways to re-use \nthe code between tests. One such way would be to do something like this:\n\n run_incremental_repack () {\n test_commit A &&\n ...\n test_subcommand ...\n }\n\n test_expect_success 'maintenance.incremental-repack.auto' '\n rm -rf incremental-repack &&\n git init incremental-repack &&\n (\n cd incremental-repack &&\n git config core.multiPackIndex true &&\n run_incremental_repack\n )\n '\n\n test_expect_success 'maintenance.incremental-repack.auto (config \nunset)' '\n rm -rf incremental-repack &&\n git init incremental-repack &&\n (\n cd incremental-repack &&\n test_unconfig core.multiPackIndex &&\n run_incremental_repack\n )\n '\n"},{"id":"437440","messageId":"CAPig+cS7PF=F0XWO-EyVz_KuLnbnhQi3N01gyozSv99PmB_1RA@mail.gmail.com","threadId":"56494","inReplyTo":"kl6lh7e53nnn.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2 0/3] Use default values from settings instead of config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-29T06:43:54Z","receivedAt":"2021-09-29T06:44:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 27, 2021 at 1:59 PM Glen Choo wrote:\n> I wasn't clear about this in the original message, but I think this is\n> ready to merge in its current form. I'd love to hear from reviewers who\n> can poke holes in that :)\n>\n> Cc Eric and Taylor who have given the most review on v1 (thanks again!).\n> I believe I've addressed your comments and I'd be interesting in hearing\n> your thoughts on v2.\n\nI re-read the entire thread and left a few comments on the v2 patches.\nOne or two of the comments might be actionable (i.e. deserve a\nre-roll), but most are probably subjective.\n\nOh, I forgot to mention in my review of [3/3] that in:\n\n In the case where core.multiPackIndex is not set in the config, the auto\n condition would fail, causing the incremental-repack task to not be\n run. Because we always want to consider the default values, we should\n just always just use the_repository->settings.\n\nyou can probably drop one or both \"just\"s.\n"},{"id":"437481","messageId":"kl6ltui3ouxj.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"CAPig+cS7PF=F0XWO-EyVz_KuLnbnhQi3N01gyozSv99PmB_1RA@mail.gmail.com","subject":"Re: [PATCH v2 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-29T22:53:28Z","receivedAt":"2021-09-29T22:53:39Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Eric Sunshine writes:\n\n> I re-read the entire thread and left a few comments on the v2 patches.\n> One or two of the comments might be actionable (i.e. deserve a\n> re-roll), but most are probably subjective.\n\nThanks so much again!\n\n> Oh, I forgot to mention in my review of [3/3] that in:\n>\n> In the case where core.multiPackIndex is not set in the config, the auto\n> condition would fail, causing the incremental-repack task to not be\n> run. Because we always want to consider the default values, we should\n> just always just use the_repository->settings.\n>\n> you can probably drop one or both \"just\"s.\n\nYes, this is definitely a typo from me.\n"},{"id":"437482","messageId":"kl6lr1d7ourp.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"ebe894b4-af25-f79e-c43f-1839e075908d@sunshineco.com","subject":"Re: [PATCH v2 2/3] fsck: verify multi-pack-index when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-29T22:56:58Z","receivedAt":"2021-09-29T22:57:02Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Eric Sunshine writes:\n\n>> test_expect_success 'git-fsck incorrect offset' '\n>> \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n>> \t\t\"incorrect object offset\" \\\n>> -\t\t\"git -c core.multipackindex=true fsck\"\n>> +\t\t\"git -c core.multipackindex=true fsck\" &&\n>> +\t\ttest_must_fail git fsck &&\n>> +\t\tgit -c core.multipackindex=false fsck\n>> '\n>\n> I guess the newly-added `test_must_fail git fsck` line is checking the \n> fallback case then `core.multipackindex` is not set. We could make this \n> check a bit more robust by _ensuring_ that it is unset rather than \n> relying upon whatever state the configuration is in by the time this \n> test is reached. Perhaps something like this:\n>\n> ...\n> \"git -c core.multipackindex=true fsck\" &&\n> test_unconfig core.multipackindex &&\n> test_must_fail git fsck &&\n> git -c core.multipackindex=false fsck\n\nI think this extra robustness is worth it. I sometimes find that the\ntests are a bit too interdependent to read on their own, so this is a\ngood step forward.\n\n> The indentation is a bit unusual. It aligns nicely and is, in some \n> sense, easy to read, but the two new lines are over-indented considering \n> that they are siblings of the corrupt_midx_and_verify() call.\n\nI agree. This was a typo from me, not a conscious choice.\n"},{"id":"437564","messageId":"kl6lmtnu7xle.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"e1447d0e-091d-c659-b702-c5b33fffdb59@sunshineco.com","subject":"Re: [PATCH v2 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-30T18:00:13Z","receivedAt":"2021-09-30T18:00:20Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Eric Sunshine writes:\n\n> Because I took the time to scan backward through this test script, I \n> understand that `core.commitGraph` is already `true` by the time this \n> test is reached, thus the new test title is accurate (for now). However, \n> it would be a bit easier to reason about this and make it more robust by \n> having the test itself guarantee that it is set to `true` by invoking \n> `git config core.commitGraph true`.\n\nReading this and..\n\n> I find it somewhat confusing to reason about the behavior of \n> test_when_finished() when it is invoked like this before the `cd`. It's \n> true that the end-of-test `git config core.commitGraph true` will be \n> invoked within `full` as desired (except in the very unusual case of the \n> `cd` failing), so it's probably correct, but it requires extra thought \n> to come to that conclusion. Switching the order of these two lines might \n> lower the cognitive load a bit.\n\nI'll make both of these changes. Test readability is important and\npeople shouldn't be wasting time thinking about test correctness.\n\n> test_config() unsets the config variable when the test completes, so I'm \n> wondering if its use is in fact desirable here. I ask because, from a \n> quick scan through the file, it appears that many tests in this script \n> assume that `core.commitGraph` is `true`, so it's not clear that \n> unsetting it at the end of this test is a good idea in general.\n\nThis is a good point. I made the original change in response to Taylor's\ncomment, but I think we both didn't consider that this script assumes\n`core.commitGraph` is `true`. The rest of the tests pass, but only\nbecause the default value is true and I'd rather not have tests rely on\na happy accident.\n\n> Taking Taylor's comment[1] on v1 patch [2/3] into account, I wonder if \n> it would be simpler for these all to be combined into a single test. \n\nInteresting thought. The marginal benefit here is much lower than in\npatch [2/3]. The difference is that corrupt_midx_and_verify() uses an\nadditional $COMMAND to perform verification, but\ncorrupt_graph_and_verify() does not. The result is that\ncorrupt_midx_and_verify() is subtle and confusing when used in separate\ntests, but corrupt_graph_and_verify() is not so bad.\n\n> A downside of combining the tests like this is that it makes it a bit \n> more cumbersome to diagnose a failure (because there is more code and \n> more cases to wade through).\n\nYes, and we also lose the test labels that would guide the reader. It\nmight not be that obvious why the tests for a boolean value has 3\ncases {true,false,unset}.\n\nTaking churn into account, I'm inclined to keep the tests separate.\n"},{"id":"437566","messageId":"kl6lk0ix9aju.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"kl6lmtnu7xle.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-30T18:35:01Z","receivedAt":"2021-09-30T18:35:06Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\n>> Because I took the time to scan backward through this test script, I \n>> understand that `core.commitGraph` is already `true` by the time this \n>> test is reached, thus the new test title is accurate (for now). However, \n>> it would be a bit easier to reason about this and make it more robust by \n>> having the test itself guarantee that it is set to `true` by invoking \n>> `git config core.commitGraph true`.\n\n>> test_config() unsets the config variable when the test completes, so I'm \n>> wondering if its use is in fact desirable here. I ask because, from a \n>> quick scan through the file, it appears that many tests in this script \n>> assume that `core.commitGraph` is `true`, so it's not clear that \n>> unsetting it at the end of this test is a good idea in general.\n>\n> This is a good point. I made the original change in response to Taylor's\n> comment, but I think we both didn't consider that this script assumes\n> `core.commitGraph` is `true`. The rest of the tests pass, but only\n> because the default value is true and I'd rather not have tests rely on\n> a happy accident.\n\nI said I would incorporate these suggestions, but I didn't propose what\nchanges I would actually make.\n\nGiven that each test depends on a global config value in the setup\nphase, it might be simplest to read if we try to avoid anything that\ntouches that value. The easiest way to achieve this is to use git -c\ncore.commitGraph {true,false} for the {true,false} cases. Since there is\nno -c equivalent for the unset case, I'll continue to use\ntest_unconfig() + test_when_finished() to temporarily unset the value.\n"},{"id":"437567","messageId":"CAPig+cSTEwObVOajvtVA0BQo0vB74BDN4-f6Gi1tYrEt-BzceQ@mail.gmail.com","threadId":"56494","inReplyTo":"kl6lk0ix9aju.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-30T18:39:28Z","receivedAt":"2021-09-30T18:39:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 30, 2021 at 2:35 PM Glen Choo wrote:\n> >> test_config() unsets the config variable when the test completes, so I'm\n> >> wondering if its use is in fact desirable here. I ask because, from a\n> >> quick scan through the file, it appears that many tests in this script\n> >> assume that `core.commitGraph` is `true`, so it's not clear that\n> >> unsetting it at the end of this test is a good idea in general.\n> >\n> > This is a good point. I made the original change in response to Taylor's\n> > comment, but I think we both didn't consider that this script assumes\n> > `core.commitGraph` is `true`. The rest of the tests pass, but only\n> > because the default value is true and I'd rather not have tests rely on\n> > a happy accident.\n>\n> I said I would incorporate these suggestions, but I didn't propose what\n> changes I would actually make.\n>\n> Given that each test depends on a global config value in the setup\n> phase, it might be simplest to read if we try to avoid anything that\n> touches that value. The easiest way to achieve this is to use git -c\n> core.commitGraph {true,false} for the {true,false} cases. Since there is\n> no -c equivalent for the unset case, I'll continue to use\n> test_unconfig() + test_when_finished() to temporarily unset the value.\n\nThat was my thought, as well. (I wasn't quite sure why Taylor\nrecommended test_config() over `-c` which you used originally. It may\njust be his personal preference. Perhaps he can chime in?)\n"},{"id":"437714","messageId":"kl6lpmsoac2w.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"CAPig+cSTEwObVOajvtVA0BQo0vB74BDN4-f6Gi1tYrEt-BzceQ@mail.gmail.com","subject":"Re: [PATCH v2 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-01T17:28:55Z","receivedAt":"2021-10-01T17:29:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Eric Sunshine writes:\n\n> That was my thought, as well. (I wasn't quite sure why Taylor\n> recommended test_config() over `-c` which you used originally. It may\n> just be his personal preference. Perhaps he can chime in?)\n\nI'd also appreciate thoughts on test_config() over `-c` :). I won't\nre-roll this series yet in case there's more to the test_config() story,\nbut I've included a patch that shows the `-c` work that we discussed.\n\n---\n t/t5318-commit-graph.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 42e785cb6e..4fcfc2ebbc 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -680,7 +680,7 @@ test_expect_success 'git fsck (checks commit-graph when config set to true)' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n \tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n-\ttest_must_fail git fsck\n+\ttest_must_fail git -c core.commitGraph=true fsck\n '\n \n test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n@@ -689,14 +689,13 @@ test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n \tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n-\ttest_config core.commitGraph false &&\n-\tgit fsck\n+\tgit -c core.commitGraph=false fsck\n '\n \n test_expect_success 'git fsck (checks commit-graph when config unset)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n \ttest_when_finished \"git config core.commitGraph true\" &&\n \n-\tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n-- \n2.33.0.800.g4c38ced690-goog\n\n"},{"id":"437950","messageId":"20211005001931.13932-1-chooglen@google.com","threadId":"56494","inReplyTo":"20210917225459.68086-1-chooglen@google.com","subject":"[PATCH v3 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-05T00:19:28Z","receivedAt":"2021-10-05T00:19:44Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Hi everyone! This patch was created in response to something we observed in\nGoogle, where fsck failed to detect that the commit graph was invalid. We\ninitially assumed that fsck never checked the commit graph, but it turns out\nthat it does so only when core.commitgraph is set, even though we set defaults\nfor \"whether to use the commit graph\" in the repository settings.\n\nInstead of using the config, let's use repository settings where\navailable. Replace core.commitGraph and core.multiPackIndex with their\nequivalent repository settings in fsck and gc.\n\nI don't anticipate any more business logic changes and the previous comments\nwere focused on testing style, so hopefully this should be good to merge.\n\nChanges since v2:\n* Various small test fixes (thanks Eric!). Most notably, I've used -c instead of\n test_config because test_config can affect the values in subsequent tests.\n* Rewording fix in patch 3 commit message\n* Refactor tests in patch 3 so that we use a single helper function instead of\n copy-pasted code\n\nChanges since v1:\n* clean up typo in patch 1 commit message \n* document the commits that patches 1 and 2 address\n* use test helpers in patch 1\n* rewrite patch 2's tests so that it's easier to tell that each test\n does something different\n* reword patch 3 commit message to explain the bug\n* add tests to patch 3\n\nGlen Choo (3):\n fsck: verify commit graph when implicitly enabled\n fsck: verify multi-pack-index when implictly enabled\n gc: perform incremental repack when implictly enabled\n\n builtin/fsck.c | 5 +++--\n builtin/gc.c | 5 ++---\n t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-\n t/t5319-multi-pack-index.sh | 5 ++++-\n t/t7900-maintenance.sh | 28 ++++++++++++++++++++++++----\n 5 files changed, 55 insertions(+), 11 deletions(-)\n\nRange-diff against v2:\n1: 97ab2bb529 ! 1: 2f9ff949e6 fsck: verify commit graph when implicitly enabled\n @@ t/t5318-commit-graph.sh: test_expect_success 'detect incorrect chunk count' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n -@@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '\n - \ttest_must_fail git fsck\n - '\n - \n + \t\t\"incorrect checksum\" &&\n + \tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n ++\ttest_must_fail git -c core.commitGraph=true fsck\n ++'\n ++\n +test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n +\tcd \"$TRASH_DIRECTORY/full\" &&\n +\tgit fsck &&\n +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n +\t\t\"incorrect checksum\" &&\n +\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n -+\ttest_config core.commitGraph false &&\n -+\tgit fsck\n ++\tgit -c core.commitGraph=false fsck\n +'\n +\n +test_expect_success 'git fsck (checks commit-graph when config unset)' '\n ++\tcd \"$TRASH_DIRECTORY/full\" &&\n +\ttest_when_finished \"git config core.commitGraph true\" &&\n +\n -+\tcd \"$TRASH_DIRECTORY/full\" &&\n +\tgit fsck &&\n +\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n +\t\t\"incorrect checksum\" &&\n +\ttest_unconfig core.commitGraph &&\n +\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n -+\ttest_must_fail git fsck\n -+'\n -+\n - test_expect_success 'setup non-the_repository tests' '\n - \trm -rf repo &&\n - \tgit init repo &&\n + \ttest_must_fail git fsck\n + '\n + \n2: ace92453ca ! 2: b13ca2a695 fsck: verify multi-pack-index when implictly enabled\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'verify incorrect offset' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" \\\n -\t\t\"git -c core.multipackindex=true fsck\"\n -+\t\t\"git -c core.multipackindex=true fsck\" &&\n -+\t\ttest_must_fail git fsck &&\n -+\t\tgit -c core.multipackindex=false fsck\n ++\t\t\"git -c core.multiPackIndex=true fsck\" &&\n ++\ttest_unconfig core.multiPackIndex &&\n ++\ttest_must_fail git fsck &&\n ++\tgit -c core.multiPackIndex=false fsck\n '\n \n test_expect_success 'corrupt MIDX is not reused' '\n3: d6959d61c1 < -: ---------- gc: perform incremental repack when implictly enabled\n-: ---------- > 3: 76943aff80 gc: perform incremental repack when implictly enabled\n-- \n2.33.0.800.g4c38ced690-goog\n\n"},{"id":"437951","messageId":"20211005001931.13932-2-chooglen@google.com","threadId":"56494","inReplyTo":"20211005001931.13932-1-chooglen@google.com","subject":"[PATCH v3 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-05T00:19:29Z","receivedAt":"2021-10-05T00:19:50Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"the_repository->settings is the preferred way to get certain\nsettings (such as \"core.commitGraph\") because it gets default values\nfrom prepare_repo_settings(). However, cmd_fsck() reads the config\ndirectly via git_config_get_bool(), which bypasses these default values.\nThis causes fsck to ignore the commit graph if \"core.commitgraph\" is not\nexplicitly set in the config. This worked fine until commit-graph was\nenabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by\ndefault, 2019-08-13).\n\nReplace git_config_get_bool(\"core.commitgraph\") in fsck with the\nequivalent call to the_repository->settings.core_commit_graph.\n\nThe expected behavior is that fsck respects the config value when it is\nset, but uses the default value when it is unset. For example, for\ncore.commitGraph, there are three cases:\n\n- Config value is set to true -> enabled\n- Config value is set to false -> disabled\n- Config value is unset -> enabled\n\nAs such, tests cover all three cases.\n\nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 3 ++-\n t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex b42b6fe21f..1c4e485b66 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -803,6 +803,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tfsck_enable_object_names(&fsck_walk_options);\n \n \tgit_config(git_fsck_config, &fsck_obj_options);\n+\tprepare_repo_settings(the_repository);\n \n \tif (connectivity_only) {\n \t\tfor_each_loose_object(mark_loose_for_connectivity, NULL, 0);\n@@ -908,7 +909,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tcheck_connectivity();\n \n-\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n+\tif (the_repository->settings.core_commit_graph) {\n \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex af88f805aa..4fcfc2ebbc 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -674,12 +674,33 @@ test_expect_success 'detect incorrect chunk count' '\n \t\t$GRAPH_CHUNK_LOOKUP_OFFSET\n '\n \n-test_expect_success 'git fsck (checks commit-graph)' '\n+test_expect_success 'git fsck (checks commit-graph when config set to true)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n \tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\ttest_must_fail git -c core.commitGraph=true fsck\n+'\n+\n+test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\tgit -c core.commitGraph=false fsck\n+'\n+\n+test_expect_success 'git fsck (checks commit-graph when config unset)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\ttest_when_finished \"git config core.commitGraph true\" &&\n+\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\ttest_unconfig core.commitGraph &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n \ttest_must_fail git fsck\n '\n \n-- \n2.33.0.800.g4c38ced690-goog\n\n"},{"id":"437952","messageId":"20211005001931.13932-3-chooglen@google.com","threadId":"56494","inReplyTo":"20211005001931.13932-1-chooglen@google.com","subject":"[PATCH v3 2/3] fsck: verify multi-pack-index when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-05T00:19:30Z","receivedAt":"2021-10-05T00:19:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Like the previous commit, the_repository->settings.core_multi_pack_index\nis preferable to reading \"core.multiPackIndex\" from the config because\nprepare_repo_settings() sets a default value for\nthe_repository->settings. This worked fine until core.multiPackIndex was\nenabled by default in 18e449f86b (midx: enable core.multiPackIndex by\ndefault, 2020-09-25).\n\nReplace git_config_get_bool(\"core.multiPackIndex\") in fsck with the\nequivalent call to the_repository->settings.multi_pack_index.\n\nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 2 +-\n t/t5319-multi-pack-index.sh | 5 ++++-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1c4e485b66..5bbe8068ec 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -925,7 +925,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (!git_config_get_bool(\"core.multipackindex\", &i) && i) {\n+\tif (the_repository->settings.core_multi_pack_index) {\n \t\tstruct child_process midx_verify = CHILD_PROCESS_INIT;\n \t\tconst char *midx_argv[] = { \"multi-pack-index\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3d4d9f10c3..53958bfd1f 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -407,7 +407,10 @@ test_expect_success 'verify incorrect offset' '\n test_expect_success 'git-fsck incorrect offset' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" \\\n-\t\t\"git -c core.multipackindex=true fsck\"\n+\t\t\"git -c core.multiPackIndex=true fsck\" &&\n+\ttest_unconfig core.multiPackIndex &&\n+\ttest_must_fail git fsck &&\n+\tgit -c core.multiPackIndex=false fsck\n '\n \n test_expect_success 'corrupt MIDX is not reused' '\n-- \n2.33.0.800.g4c38ced690-goog\n\n"},{"id":"437953","messageId":"20211005001931.13932-4-chooglen@google.com","threadId":"56494","inReplyTo":"20211005001931.13932-1-chooglen@google.com","subject":"[PATCH v3 3/3] gc: perform incremental repack when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-05T00:19:31Z","receivedAt":"2021-10-05T00:19:58Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"builtin/gc.c has two ways of checking if multi-pack-index is enabled:\n- git_config_get_bool() in incremental_repack_auto_condition()\n- the_repository->settings.core_multi_pack_index in\n maintenance_task_incremental_repack()\n\nThe two implementations have existed since the incremental-repack task\nwas introduced in e841a79a13 (maintenance: add incremental-repack auto\ncondition, 2020-09-25). These two values can diverge because\nprepare_repo_settings() enables the feature in the_repository->settings\nby default.\n\nIn the case where core.multiPackIndex is not set in the config, the auto\ncondition would fail, causing the incremental-repack task to not be\nrun. Because we always want to consider the default values, we should\nalways use the_repository->settings.\n\nStandardize on using the_repository->settings.core_multi_pack_index to\ncheck if multi-pack-index is enabled.\n\nSigned-off-by: Glen Choo \n---\n builtin/gc.c | 5 ++---\n t/t7900-maintenance.sh | 28 ++++++++++++++++++++++++----\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex f05d2f0a1a..070b7dccb1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1051,12 +1051,11 @@ static int maintenance_task_loose_objects(struct maintenance_run_opts *opts)\n static int incremental_repack_auto_condition(void)\n {\n \tstruct packed_git *p;\n-\tint enabled;\n \tint incremental_repack_auto_limit = 10;\n \tint count = 0;\n \n-\tif (git_config_get_bool(\"core.multiPackIndex\", &enabled) ||\n-\t !enabled)\n+\tprepare_repo_settings(the_repository);\n+\tif (!the_repository->settings.core_multi_pack_index)\n \t\treturn 0;\n \n \tgit_config_get_int(\"maintenance.incremental-repack.auto\",\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 58f46c77e6..7b1b2581af 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -321,15 +321,15 @@ test_expect_success EXPENSIVE 'incremental-repack 2g limit' '\n \t\t --no-progress --batch-size=2147483647 /dev/null &&\n \ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n \ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n \ttest_subcommand git multi-pack-index write --no-progress Hi everyone! This patch was created in response to something we observed in\n> Google, where fsck failed to detect that the commit graph was invalid. We\n> initially assumed that fsck never checked the commit graph, but it turns out\n> that it does so only when core.commitgraph is set, even though we set defaults\n> for \"whether to use the commit graph\" in the repository settings.\n>\n> Instead of using the config, let's use repository settings where\n> available. Replace core.commitGraph and core.multiPackIndex with their\n> equivalent repository settings in fsck and gc.\n>\n> I don't anticipate any more business logic changes and the previous comments\n> were focused on testing style, so hopefully this should be good to merge.\n>\n> Changes since v2:\n> * Various small test fixes (thanks Eric!). Most notably, I've used -c instead of\n> test_config because test_config can affect the values in subsequent tests.\n> * Rewording fix in patch 3 commit message\n> * Refactor tests in patch 3 so that we use a single helper function instead of\n> copy-pasted code\n>\n> Changes since v1:\n> * clean up typo in patch 1 commit message \n> * document the commits that patches 1 and 2 address\n> * use test helpers in patch 1\n> * rewrite patch 2's tests so that it's easier to tell that each test\n> does something different\n> * reword patch 3 commit message to explain the bug\n> * add tests to patch 3\n>\n> Glen Choo (3):\n> fsck: verify commit graph when implicitly enabled\n> fsck: verify multi-pack-index when implictly enabled\n> gc: perform incremental repack when implictly enabled\n\nI've looked this over carefully, it LGTM. I noticed some bugs in this\narea, but they pre-date your commits. I've pushed a\n\"gc/use-repo-settings\" to https://github.com/avar/git.git that you can\ncheck out, consider that branch partially as commentary, but feel free\nto pick/squash anything you find there.\n\nThe range-diff for that is quoted below. Notes:\n\n * This is somewhat of a case of throwing rocks from a glass house, but\n I think the commit messages could really be shorter/get to the\n point. I.e. say right away what's being changed and why, instead of\n giving the reader an overview of the whole control flow.\n\n I just adjusted adusted 1/3 in that direction below, I think 2/3\n could do with being squashed into it & adjusted, it's really the\n exact same bug/improvement, just with a different config key that\n behaves the same.\n\n * The \"test_unconfig\" in your 2/3 is redundant, so is the \"rm -rf\" of\n directories that can't exist by that point in 3/3.\n\n * I wondered if we could just call prepare_repo_settings() in one place\n earlier in gc/maintenance instead of adding yet another call deeper\n in the control flow. I've got a 4/3 below that tries to do that, and\n (as noted in its commit message) intentionally breaks \"gc.c\" in a way\n that our tests don't catch, which is perhaps fallout from\n 31b1de6a09b?\n\n So that's very much on the topic of extra improvements, i.e. your\n patches don't break that anymore than it is now, but I for one would\n very much like to see an end-state where we're confident that\n gc/maintenance do the right thing on each of the tri-states of these\n boolean variables.\n\n It's also a point about how to write reliable tests (again,\n pre-dating your series). I.e. instead of asserting \"we do X when Y is\n true\" have the test assert all of Y=[unset|false|true] in a way that\n breaks if any are different than expected.\n\n One of the tests you're changing has a \"graph_git_two_modes\" helper\n does 2/3 of that, maybe it would be easy to extend it?\n\n * You use corrupting the graph as a shorthand for \"did we run this\n command?\". See test_region for an approach that might be better,\n i.e. just log with trace2 and grep that to see if we started the\n relevant command. The test_region helper can't be used as-is for that\n (you could manually grep the emitted PERF/EVENT output), but perhaps\n that's easier/more reliable?\n\n * This is again, something that pre-dates your patches, but I think\n following the \"is enabled?\" behavior here for these variables in \"git\n fsck\" is highly questionable. So just some thoughts after having\n looked at that:\n\n Just because your config doen't say \"use the graph\" doesn't mean\n another process won't do so, and if the graph is broken they might\n probably encounter an error.\n\n Although I think we *should* have cleaned up those being fatal a\n while ago). See 43d35618055 (commit-graph write: don't die if the\n existing graph is corrupt, 2019-03-25).\n\n But in general for these auxiliary files (commit-graph, midix, *.rev\n etc.) I think there's a good argument to be made that fsck should\n *always* check them, and at most use the config to say something\n like:\n\n Hey, you've got a 100% broken commit-graph/midx/rev in your .git,\n looks like the core.XYZ enabling it is off *right now* though, so\n maybe we won't use it. I hope you're feeling lucky...\n\n :)\n\n\n1: fd9a58e2c7b ! 1: c6041f3b633 fsck: verify commit graph when implicitly enabled\n @@ Metadata\n ## Commit message ##\n fsck: verify commit graph when implicitly enabled\n \n - the_repository->settings is the preferred way to get certain\n - settings (such as \"core.commitGraph\") because it gets default values\n - from prepare_repo_settings(). However, cmd_fsck() reads the config\n - directly via git_config_get_bool(), which bypasses these default values.\n - This causes fsck to ignore the commit graph if \"core.commitgraph\" is not\n - explicitly set in the config. This worked fine until commit-graph was\n - enabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by\n - default, 2019-08-13).\n + Change fsck to check the \"core_commit_graph\" variable set in\n + \"repo-settings.c\" instead of reading the \"core.commitGraph\" variable,\n + this fixes a bug where we wouldn't start \"commit-graph verify\" if the\n + config key was missing, which since 31b1de6a09 (commit-graph: turn on\n + commit-graph by default, 2019-08-13) has meant that it's on by\n + default.\n \n - Replace git_config_get_bool(\"core.commitgraph\") in fsck with the\n - equivalent call to the_repository->settings.core_commit_graph.\n -\n - The expected behavior is that fsck respects the config value when it is\n - set, but uses the default value when it is unset. For example, for\n - core.commitGraph, there are three cases:\n -\n - - Config value is set to true -> enabled\n - - Config value is set to false -> disabled\n - - Config value is unset -> enabled\n -\n - As such, tests cover all three cases.\n + Add tests to \"t5318-commit-graph.sh\" to check that this works, using\n + the detection of a corrupt commit-graph as a method of seeing if\n + \"commit-graph verify\" was invoked.\n \n Signed-off-by: Glen Choo \n Signed-off-by: Junio C Hamano \n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/fsck.c ##\n @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)\n2: 9d6813bc280 ! 2: d11209ebfe1 fsck: verify multi-pack-index when implictly enabled\n @@ Commit message\n \n Signed-off-by: Glen Choo \n Signed-off-by: Junio C Hamano \n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/fsck.c ##\n @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'verify incorrect offset' '\n \t\t\"incorrect object offset\" \\\n -\t\t\"git -c core.multipackindex=true fsck\"\n +\t\t\"git -c core.multiPackIndex=true fsck\" &&\n -+\ttest_unconfig core.multiPackIndex &&\n +\ttest_must_fail git fsck &&\n +\tgit -c core.multiPackIndex=false fsck\n '\n3: 260f46568c6 ! 3: 017e2003e85 gc: perform incremental repack when implictly enabled\n @@ Commit message\n \n Signed-off-by: Glen Choo \n Signed-off-by: Junio C Hamano \n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/gc.c ##\n @@ builtin/gc.c: static int maintenance_task_loose_objects(struct maintenance_run_opts *opts)\n @@ t/t7900-maintenance.sh: test_expect_success 'maintenance.incremental-repack.auto\n +}\n +\n +test_expect_success 'maintenance.incremental-repack.auto' '\n -+\trm -rf incremental-repack-true &&\n +\tgit init incremental-repack-true &&\n +\t(\n +\t\tcd incremental-repack-true &&\n @@ t/t7900-maintenance.sh: test_expect_success 'maintenance.incremental-repack.auto\n +'\n +\n +test_expect_success 'maintenance.incremental-repack.auto (when config is unset)' '\n -+\trm -rf incremental-repack-unset &&\n +\tgit init incremental-repack-unset &&\n +\t(\n +\t\tcd incremental-repack-unset &&\n-: ----------- > 4: 04d1527f180 WIP gc/maintenance: just call prepare_repo_settings() earlier\n\n-- >8 --\nSubject: [PATCH] WIP gc/maintenance: just call prepare_repo_settings() earlier\n\nConsolidate the various calls to prepare_repo_settings() to happen at\nthe start of cmd_maintenance() (TODO: and cmd_gc()). This WIP commit\nintentionally breaks things, we seem to be lacking test coverage for\ncmd_gc(), perhaps since 31b1de6a09b (commit-graph: turn on\ncommit-graph by default, 2019-08-13)?\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 5 +----\n t/t5318-commit-graph.sh | 2 +-\n 2 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 26709311601..f59dbedc1fe 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -695,7 +695,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tclean_pack_garbage();\n \t}\n \n-\tprepare_repo_settings(the_repository);\n \tif (the_repository->settings.gc_write_commit_graph == 1)\n \t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\n@@ -860,7 +859,6 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n \n static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)\n {\n-\tprepare_repo_settings(the_repository);\n \tif (!the_repository->settings.core_commit_graph)\n \t\treturn 0;\n \n@@ -1052,7 +1050,6 @@ static int incremental_repack_auto_condition(void)\n \tint incremental_repack_auto_limit = 10;\n \tint count = 0;\n \n-\tprepare_repo_settings(the_repository);\n \tif (!the_repository->settings.core_multi_pack_index)\n \t\treturn 0;\n \n@@ -1167,7 +1164,6 @@ static int multi_pack_index_repack(struct maintenance_run_opts *opts)\n \n static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts)\n {\n-\tprepare_repo_settings(the_repository);\n \tif (!the_repository->settings.core_multi_pack_index) {\n \t\twarning(_(\"skipping incremental-repack task because core.multiPackIndex is disabled\"));\n \t\treturn 0;\n@@ -2492,6 +2488,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)\n \t (argc == 2 && !strcmp(argv[1], \"-h\")))\n \t\tusage(builtin_maintenance_usage);\n \n+\tprepare_repo_settings(the_repository);\n \tif (!strcmp(argv[1], \"run\"))\n \t\treturn maintenance_run(argc - 1, argv + 1, prefix);\n \tif (!strcmp(argv[1], \"start\"))\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 4e4450af1ef..98123191e1e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -368,7 +368,7 @@ test_expect_success 'check that gc computes commit-graph' '\n \tgit commit-graph write --reachable &&\n \tcp $objdir/info/commit-graph commit-graph-before-gc &&\n \tgit reset --hard HEAD~1 &&\n-\tgit config gc.writeCommitGraph true &&\n+\t# BUG: Due to 31b1de6a09b (commit-graph: turn on commit-graph by default, 2019-08-13)?\n \tgit gc &&\n \tcp $objdir/info/commit-graph commit-graph-after-gc &&\n \t! test_cmp_bin commit-graph-before-gc commit-graph-after-gc &&\n-- \n2.33.0.1441.gbbcdb4c3c66\n\n\n\n\n\n"},{"id":"437991","messageId":"70aca052-716f-40ed-47c4-1882fdbd221e@gmail.com","threadId":"56494","inReplyTo":"87lf37ll4k.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 0/3] Use default values from settings instead of config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-05T17:43:21Z","receivedAt":"2021-10-05T17:49:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/5/2021 7:57 AM, Ævar Arnfjörð Bjarmason wrote:\n\n> Subject: [PATCH] WIP gc/maintenance: just call prepare_repo_settings() earlier\n> \n> Consolidate the various calls to prepare_repo_settings() to happen at\n> the start of cmd_maintenance() (TODO: and cmd_gc()). This WIP commit\n> intentionally breaks things, we seem to be lacking test coverage for\n> cmd_gc(), perhaps since 31b1de6a09b (commit-graph: turn on\n> commit-graph by default, 2019-08-13)?\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/gc.c | 5 +----\n> t/t5318-commit-graph.sh | 2 +-\n> 2 files changed, 2 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 26709311601..f59dbedc1fe 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -695,7 +695,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \t\tclean_pack_garbage();\n> \t}\n> \n> -\tprepare_repo_settings(the_repository);\n> \tif (the_repository->settings.gc_write_commit_graph == 1)\n\nI think that removing these calls is dangerous. prepare_repo_settings()\nalready returns immediately if the repository already has its settings\npopulated. The pattern of \"call prepare before using a setting\" is a\nsafe way to future-proof the check from a movement of the call.\n\nPutting that potential-future-problem aside, I don't see how this\nchange is a _benefit_ other than fewer lines of code, which is not a\nquality measure in itself.\n\nThanks,\n-Stolee\n"},{"id":"437994","messageId":"87ilybjn9x.fsf@evledraar.gmail.com","threadId":"56494","inReplyTo":"70aca052-716f-40ed-47c4-1882fdbd221e@gmail.com","subject":"Re: [PATCH v3 0/3] Use default values from settings instead of config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-05T19:10:06Z","receivedAt":"2021-10-05T19:16:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 05 2021, Derrick Stolee wrote:\n\n> On 10/5/2021 7:57 AM, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Subject: [PATCH] WIP gc/maintenance: just call prepare_repo_settings() earlier\n>> \n>> Consolidate the various calls to prepare_repo_settings() to happen at\n>> the start of cmd_maintenance() (TODO: and cmd_gc()). This WIP commit\n>> intentionally breaks things, we seem to be lacking test coverage for\n>> cmd_gc(), perhaps since 31b1de6a09b (commit-graph: turn on\n>> commit-graph by default, 2019-08-13)?\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/gc.c | 5 +----\n>> t/t5318-commit-graph.sh | 2 +-\n>> 2 files changed, 2 insertions(+), 5 deletions(-)\n>> \n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index 26709311601..f59dbedc1fe 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -695,7 +695,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>> \t\tclean_pack_garbage();\n>> \t}\n>> \n>> -\tprepare_repo_settings(the_repository);\n>> \tif (the_repository->settings.gc_write_commit_graph == 1)\n>\n> I think that removing these calls is dangerous. prepare_repo_settings()\n> already returns immediately if the repository already has its settings\n> populated. The pattern of \"call prepare before using a setting\" is a\n> safe way to future-proof the check from a movement of the call.\n>\n> Putting that potential-future-problem aside, I don't see how this\n> change is a _benefit_ other than fewer lines of code, which is not a\n> quality measure in itself.\n\nVery dangerous, yes :) As I noted this hunk intentionaly breaks the \"git\ngc\" command. Search for \"I wondered if we could just call\nprepare_repo_settings[...]\" upthread.\n\nI.e. I was commenting on how Glen's patch makes an attempt to test this\ntri-state config for some cases, but not others, and that in this case\nwe can break \"git gc\" without any existing test catching it.\n\nWhich as noted might have started happening when you flipped the\ncore.commitGraph default, but I didn't test or confirm that, it just\nseemed the most likely place to start digging from a quick \"git log\n-G...\".\n\nAnyway, as also noted I think this series is fine as-is, just some\nfeedback in case Glen or anyone would be interested in either doing a v4\nor a follow-up, i.e. when I tested this I found that some code/tests in\nthis area were (probably) either fragile or already broken.\n"},{"id":"438014","messageId":"kl6lh7dvw1lp.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"87lf37ll4k.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-05T22:25:38Z","receivedAt":"2021-10-05T22:25:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I've looked this over carefully, it LGTM. I noticed some bugs in this\n> area, but they pre-date your commits. I've pushed a\n> \"gc/use-repo-settings\" to https://github.com/avar/git.git that you can\n> check out, consider that branch partially as commentary, but feel free\n> to pick/squash anything you find there.\n\nYour review is very thoughtful, thank you :)\n\n> * This is somewhat of a case of throwing rocks from a glass house, but\n> I think the commit messages could really be shorter/get to the\n> point. I.e. say right away what's being changed and why, instead of\n> giving the reader an overview of the whole control flow.\n>\n> I just adjusted adusted 1/3 in that direction below, I think 2/3\n> could do with being squashed into it & adjusted, it's really the\n> exact same bug/improvement, just with a different config key that\n> behaves the same.\n\nIn hindsight, I think this is how I should have initially structured my\ncommits because this improves the signal-noise ratio. I'll keep it in\nmind in the future for sure. I'm hesitant to send a v4, but I can be\npersuaded to do so :)\n\n> * The \"test_unconfig\" in your 2/3 is redundant, so is the \"rm -rf\" of\n> directories that can't exist by that point in 3/3.\n\nThese are redundant, but they serve a defensive purpose. By being extra\ndefensive, I was hoping to make it clear to the reader that the test is\n*guaranteed* not to conflict with/rely on previous state and reduce the\nnumber of greps needed.\n\n> * You use corrupting the graph as a shorthand for \"did we run this\n> command?\". See test_region for an approach that might be better,\n> i.e. just log with trace2 and grep that to see if we started the\n> relevant command. The test_region helper can't be used as-is for that\n> (you could manually grep the emitted PERF/EVENT output), but perhaps\n> that's easier/more reliable?\n\nMy intention here was not to assert \"did git fsck call the function I\nexpected?\", but something more along the lines of \"did git fsck catch the\ncorrupted commit-graph?\". Calling the function seems like an\nimplementation detail, but the actual desired behavior is that git fsck\ncatches the breakage.\n\nPerhaps the test title should be reflect what I meant, like\n\n - git fsck (checks commit-graph when config set to true)\n + git fsck detects corrupted commit-graph\n\n> * This is again, something that pre-dates your patches, but I think\n> following the \"is enabled?\" behavior here for these variables in \"git\n> fsck\" is highly questionable. So just some thoughts after having\n> looked at that:\n\nI think it depends on how we think about core.commitGraph (and others).\nDo we want Git to be able to pretend that commit-graph doesn't exist at\nall? If so, then we should skip the check in git fsck. Alternatively, is\ncommit-graph actually part of Git's core offering? If so,\ncore.commitGraph is more like a way to opt *out* of some commit-graph\nbehavior, but fsck should still verify our core data structures. Given\nthat we default to true, it sounds like more of the latter, but I'm not\nsure what use case this serves.\n\n> Just because your config doen't say \"use the graph\" doesn't mean\n> another process won't do so, and if the graph is broken they might\n> probably encounter an error.\n\nWhile that's true, I think that the user assumes a certain degree of\nrisk if they are using different commands with different config values.\nI don't think it's unreasonable to say that \"git -c\ncore.commitGraph=false fsck\" won't catch issues that will arise in \"git\n-c core.commitGraph=true foo\".\n\n> But in general for these auxiliary files (commit-graph, midix, *.rev\n> etc.) I think there's a good argument to be made that fsck should\n> *always* check them, and at most use the config to say something\n> like:\n>\n> Hey, you've got a 100% broken commit-graph/midx/rev in your .git,\n> looks like the core.XYZ enabling it is off *right now* though, so\n> maybe we won't use it. I hope you're feeling lucky...\n\nI think \"always check\" sounds like the best way to maximize visibility\nfor users, though I don't think core.commitGraph should control the\nseverity of the warning/error. If we pursue \"always check\", I think we\ncould adopt fsck. instead.\n"},{"id":"438394","messageId":"xmqqfstafyox.fsf@gitster.g","threadId":"56494","inReplyTo":"20211005001931.13932-1-chooglen@google.com","subject":"Re: [PATCH v3 0/3] Use default values from settings instead of config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-09T07:24:14Z","receivedAt":"2021-10-09T07:24:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Hi everyone! This patch was created in response to something we observed in\n> Google, where fsck failed to detect that the commit graph was invalid. We\n> initially assumed that fsck never checked the commit graph, but it turns out\n> that it does so only when core.commitgraph is set, even though we set defaults\n> for \"whether to use the commit graph\" in the repository settings.\n\nWith this merged to 'seen', the CI job with the extra set of\nGIT_TEST_X settings fail. When this topic is excluded, with\nall the other topics in flight in 'seen', everything seems to\nbe OK.\n\nFor which GIT_TEST_X environment variables to set and export while\ntesting to trigger the problem, see [*1*]\n\nFor a successful test run of 'seen' without this topic, see [*2*]\n\nFor the test log of the failing run with this topic, see [*3*];\nyou'd need to be logged into GitHub to see the details of the errors\n(e.g. click on \"regular (linux-gcc...)\" with red X sign on the left\nhand side, then open \"Run ci/print-test-failures.sh\" and look for\n\"not ok\").\n\n\n[References]\n\n*1* https://github.com/git/git/runs/3843549095?check_suite_focus=true#step:4:1677\n\n export GIT_TEST_SPLIT_INDEX=yes\n export GIT_TEST_MERGE_ALGORITHM=recursive\n export GIT_TEST_FULL_IN_PACK_ARRAY=true\n export GIT_TEST_OE_SIZE=10\n export GIT_TEST_OE_DELTA_SIZE=5\n export GIT_TEST_COMMIT_GRAPH=1\n export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1\n export GIT_TEST_MULTI_PACK_INDEX=1\n export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n export GIT_TEST_ADD_I_USE_BUILTIN=1\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n export GIT_TEST_WRITE_REV_INDEX=1\n export GIT_TEST_CHECKOUT_WORKERS=2\n\n*2* https://github.com/git/git/actions/runs/1322907901 (feff65d)\n\n A successful CI run of 'seen' without gc/use-repo-settings.\n\n*3* https://github.com/git/git/actions/runs/1322842689 (54a31af)\n\n CI run of 'seen' with gc/use-repo-settings that fails.\n\n The commits that is in the failing 'seen' but not in the\n succeeding tree are those from this topic, as can be seen here:\n\n $ git shortlog --no-merges 54a31af ^feff65d\n Glen Choo (3):\n fsck: verify commit graph when implicitly enabled\n fsck: verify multi-pack-index when implictly enabled\n gc: perform incremental repack when implictly enabled\n\n"},{"id":"438486","messageId":"kl6lfst72v1e.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"xmqqfstafyox.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-11T19:58:21Z","receivedAt":"2021-10-11T19:58:27Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> With this merged to 'seen', the CI job with the extra set of\n> GIT_TEST_X settings fail. When this topic is excluded, with\n> all the other topics in flight in 'seen', everything seems to\n> be OK.\n\nThanks for the feedback, I did two CI runs on my fork in response:\n\n* [1] uses the same tree as seen. Looks like osx-clang passes but\n linux-gcc fails.\n* [2] is rebased onto master. Again, osx-clang passes but linux-gcc\n fails.\n\nThis should be enough for me to hunt down the issue, though I would\nappreciate any hints if anyone has any :)\n\n[1] https://github.com/chooglen/git/runs/3862097340?check_suite_focus=true\n[2] https://github.com/chooglen/git/runs/3862670035?check_suite_focus=true\n"},{"id":"438490","messageId":"xmqqzgrfb9z6.fsf@gitster.g","threadId":"56494","inReplyTo":"kl6lfst72v1e.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v3 0/3] Use default values from settings instead of config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-11T20:08:29Z","receivedAt":"2021-10-11T20:08:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Junio C Hamano writes:\n>\n>> With this merged to 'seen', the CI job with the extra set of\n>> GIT_TEST_X settings fail. When this topic is excluded, with\n>> all the other topics in flight in 'seen', everything seems to\n>> be OK.\n>\n> Thanks for the feedback, I did two CI runs on my fork in response:\n>\n> * [1] uses the same tree as seen. Looks like osx-clang passes but\n> linux-gcc fails.\n> * [2] is rebased onto master. Again, osx-clang passes but linux-gcc\n> fails.\n>\n> This should be enough for me to hunt down the issue, though I would\n> appreciate any hints if anyone has any :)\n\nUsing the set of GIT_TEST_FOO... settings I cited in the\nReference#1, in the message you are responding to, will let you\nreproduce the issue locally on glinux boxes.\n"},{"id":"438503","messageId":"kl6lczob2spv.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"xmqqzgrfb9z6.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-11T20:48:28Z","receivedAt":"2021-10-11T20:48:39Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Using the set of GIT_TEST_FOO... settings I cited in the\n> Reference#1, in the message you are responding to, will let you\n> reproduce the issue locally on glinux boxes.\n\nAh, I didn't notice that ci/run-build-and-tests.sh uses different\nsettings for different environments. With those settings, I can\nreproduce the failure on my OS X machine.\n\nThanks so much!\n"},{"id":"438576","messageId":"20211012174208.95161-1-chooglen@google.com","threadId":"56494","inReplyTo":"20211005001931.13932-1-chooglen@google.com","subject":"[PATCH v4 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-12T17:42:05Z","receivedAt":"2021-10-12T17:42:38Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Hi everyone! This patch was created in response to something we observed in\nGoogle, where fsck failed to detect that the commit graph was invalid. We\ninitially assumed that fsck never checked the commit graph, but it turns out\nthat it does so only when core.commitgraph is set, even though we set defaults\nfor \"whether to use the commit graph\" in the repository settings.\n\nInstead of using the config, let's use repository settings where\navailable. Replace core.commitGraph and core.multiPackIndex with their\nequivalent repository settings in fsck and gc.\n\nThis re-roll is primarily motivated by the CI failures noted by Junio in [1].\nThe underlying cause is that GIT_TEST_COMMIT_GRAPH=1 (enabled in the linux-gcc\njob) causes commands like \"git commit\" to write commit-graphs, but certain tests\nlike t/t0410-partial-clone.sh and t/t3800-mktag.sh intentionally create\ncorruptions that cause commit-graphs to become out-of-sync/invalid. Patch 1\nfixes a bug where fsck did not check commit-graphs by default, which means that\nthese tests will now fail because they have invalid commit-graphs. The easiest\nsolution I found is to disable this confusing and noisy behavior with\nGIT_TEST_COMMIT_GRAPH=0.\n\nAnd since I am re-rolling, I incorporated Ævar's feedback regarding the commit\nmessages (thanks!). I considered combining patches 1 and 2, but patch 1 has a\ngrown a little to fix the CI issues, so I've decided to keep patches 1 and 2\nseparate.\n\nThis series has also seen a healthy amount of interest in test style and\ncoverage. We haven't converged on a path for the future, but I'd like to think\nthat the tests here are still a step in (approximately) the right direction.\nHopefully this version LGT us while we figure out what to do with tests :)\n\n[1] https://lore.kernel.org/git/xmqqfstafyox.fsf@gitster.g/\n\nChanges since v3\n* Disable GIT_TEST_COMMIT_GRAPH in tests that intentionally corrupt things in a\n way that is incompatible with commit-graphs.\n* Make patch 1 and 2's commit messages more concise (thanks Ævar!).\n\nChanges since v2:\n* Various small test fixes (thanks Eric!). Most notably, I've used -c instead of\n test_config because test_config can affect the values in subsequent tests.\n* Rewording fix in patch 3 commit message\n* Refactor tests in patch 3 so that we use a single helper function instead of\n copy-pasted code\n\nChanges since v1:\n* clean up typo in patch 1 commit message \n* document the commits that patches 1 and 2 address\n* use test helpers in patch 1\n* rewrite patch 2's tests so that it's easier to tell that each test\n does something different\n* reword patch 3 commit message to explain the bug\n* add tests to patch 3\n\nGlen Choo (3):\n fsck: verify commit graph when implicitly enabled\n fsck: verify multi-pack-index when implictly enabled\n gc: perform incremental repack when implictly enabled\n\n builtin/fsck.c | 5 +++--\n builtin/gc.c | 5 ++---\n t/t0410-partial-clone.sh | 6 +++++-\n t/t3800-mktag.sh | 5 +++++\n t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-\n t/t5319-multi-pack-index.sh | 5 ++++-\n t/t7900-maintenance.sh | 28 ++++++++++++++++++++++++----\n 7 files changed, 65 insertions(+), 12 deletions(-)\n\nRange-diff against v3:\n1: 2f9ff949e6 ! 1: aac1253e7b fsck: verify commit graph when implicitly enabled\n @@ Metadata\n ## Commit message ##\n fsck: verify commit graph when implicitly enabled\n \n - the_repository->settings is the preferred way to get certain\n - settings (such as \"core.commitGraph\") because it gets default values\n - from prepare_repo_settings(). However, cmd_fsck() reads the config\n - directly via git_config_get_bool(), which bypasses these default values.\n - This causes fsck to ignore the commit graph if \"core.commitgraph\" is not\n - explicitly set in the config. This worked fine until commit-graph was\n - enabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by\n - default, 2019-08-13).\n + Change fsck to check the \"core_commit_graph\" variable set in\n + \"repo-settings.c\" instead of reading the \"core.commitGraph\" variable.\n + This fixes a bug where we wouldn't verify the commit-graph if the\n + config key was missing. This bug was introduced in\n + 31b1de6a09 (commit-graph: turn on commit-graph by default, 2019-08-13),\n + where core.commitGraph was turned on by default.\n \n - Replace git_config_get_bool(\"core.commitgraph\") in fsck with the\n - equivalent call to the_repository->settings.core_commit_graph.\n -\n - The expected behavior is that fsck respects the config value when it is\n - set, but uses the default value when it is unset. For example, for\n - core.commitGraph, there are three cases:\n -\n - - Config value is set to true -> enabled\n - - Config value is set to false -> disabled\n - - Config value is unset -> enabled\n -\n - As such, tests cover all three cases.\n + Add tests to \"t5318-commit-graph.sh\" to verify that fsck checks the\n + commit-graph as expected for the 3 values of core.commitGraph. Also,\n + disable GIT_TEST_COMMIT_GRAPH for tests that use fsck in ways that\n + assume that commit-graph checking is disabled (t/t3800-mktag.sh,\n + t/t0410-partial-clone.sh).\n \n + Helped-by: Ævar Arnfjörð Bjarmason \n Signed-off-by: Glen Choo \n \n ## builtin/fsck.c ##\n @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n \n \n + ## t/t0410-partial-clone.sh ##\n +@@ t/t0410-partial-clone.sh: test_description='partial clone'\n + \n + # missing promisor objects cause repacks which write bitmaps to fail\n + GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n ++# When enabled, some commands will write commit-graphs. This causes fsck\n ++# to fail when delete_object() is called because fsck will attempt to\n ++# verify the out-of-sync commit graph.\n ++GIT_TEST_COMMIT_GRAPH=0\n + \n + delete_object () {\n + \trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n +@@ t/t0410-partial-clone.sh: test_expect_success 'rev-list stops traversal at missing and promised commit' '\n + \n + \tgit -C repo config core.repositoryformatversion 1 &&\n + \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n +-\tGIT_TEST_COMMIT_GRAPH=0 git -C repo -c core.commitGraph=false rev-list --exclude-promisor-objects --objects bar >out &&\n ++\tgit -C repo rev-list --exclude-promisor-objects --objects bar >out &&\n + \tgrep $(git -C repo rev-parse bar) out &&\n + \t! grep $FOO out\n + '\n +\n + ## t/t3800-mktag.sh ##\n +@@ t/t3800-mktag.sh: test_description='git mktag: tag object verify test'\n + \n + . ./test-lib.sh\n + \n ++# When enabled, some commands will automatically write commit-graphs.\n ++# This will cause the mktag tests to fail because fsck will attempt to\n ++# verify the out-of-sync commit graph.\n ++GIT_TEST_COMMIT_GRAPH=0\n ++\n + ###########################################################\n + # check the tag.sig file, expecting verify_tag() to fail,\n + # and checking that the error message matches the pattern\n +\n ## t/t5318-commit-graph.sh ##\n @@ t/t5318-commit-graph.sh: test_expect_success 'detect incorrect chunk count' '\n \t\t$GRAPH_CHUNK_LOOKUP_OFFSET\n2: b13ca2a695 ! 2: ed64983430 fsck: verify multi-pack-index when implictly enabled\n @@ Metadata\n ## Commit message ##\n fsck: verify multi-pack-index when implictly enabled\n \n - Like the previous commit, the_repository->settings.core_multi_pack_index\n - is preferable to reading \"core.multiPackIndex\" from the config because\n - prepare_repo_settings() sets a default value for\n - the_repository->settings. This worked fine until core.multiPackIndex was\n - enabled by default in 18e449f86b (midx: enable core.multiPackIndex by\n - default, 2020-09-25).\n -\n - Replace git_config_get_bool(\"core.multiPackIndex\") in fsck with the\n - equivalent call to the_repository->settings.multi_pack_index.\n + Like the previous commit, change fsck to check the\n + \"core_multi_pack_index\" variable set in \"repo-settings.c\" instead of\n + reading the \"core.multiPackIndex\" config variable. This fixes a bug\n + where we wouldn't verify midx if the config key was missing. This bug\n + was introduced in 18e449f86b (midx: enable core.multiPackIndex by\n + default, 2020-09-25) where core.multiPackIndex was turned on by default.\n \n + Helped-by: Ævar Arnfjörð Bjarmason \n Signed-off-by: Glen Choo \n \n ## builtin/fsck.c ##\n3: 76943aff80 = 3: 821b492d8b gc: perform incremental repack when implictly enabled\n-- \n2.33.0.882.g93a45727a2-goog\n\n"},{"id":"438577","messageId":"20211012174208.95161-2-chooglen@google.com","threadId":"56494","inReplyTo":"20211012174208.95161-1-chooglen@google.com","subject":"[PATCH v4 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-12T17:42:06Z","receivedAt":"2021-10-12T17:42:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Change fsck to check the \"core_commit_graph\" variable set in\n\"repo-settings.c\" instead of reading the \"core.commitGraph\" variable.\nThis fixes a bug where we wouldn't verify the commit-graph if the\nconfig key was missing. This bug was introduced in\n31b1de6a09 (commit-graph: turn on commit-graph by default, 2019-08-13),\nwhere core.commitGraph was turned on by default.\n\nAdd tests to \"t5318-commit-graph.sh\" to verify that fsck checks the\ncommit-graph as expected for the 3 values of core.commitGraph. Also,\ndisable GIT_TEST_COMMIT_GRAPH for tests that use fsck in ways that\nassume that commit-graph checking is disabled (t/t3800-mktag.sh,\nt/t0410-partial-clone.sh).\n\nHelped-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 3 ++-\n t/t0410-partial-clone.sh | 6 +++++-\n t/t3800-mktag.sh | 5 +++++\n t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-\n 4 files changed, 34 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex b42b6fe21f..1c4e485b66 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -803,6 +803,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tfsck_enable_object_names(&fsck_walk_options);\n \n \tgit_config(git_fsck_config, &fsck_obj_options);\n+\tprepare_repo_settings(the_repository);\n \n \tif (connectivity_only) {\n \t\tfor_each_loose_object(mark_loose_for_connectivity, NULL, 0);\n@@ -908,7 +909,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tcheck_connectivity();\n \n-\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n+\tif (the_repository->settings.core_commit_graph) {\n \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex bba679685f..c76485b1b6 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -6,6 +6,10 @@ test_description='partial clone'\n \n # missing promisor objects cause repacks which write bitmaps to fail\n GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n+# When enabled, some commands will write commit-graphs. This causes fsck\n+# to fail when delete_object() is called because fsck will attempt to\n+# verify the out-of-sync commit graph.\n+GIT_TEST_COMMIT_GRAPH=0\n \n delete_object () {\n \trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n@@ -322,7 +326,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tGIT_TEST_COMMIT_GRAPH=0 git -C repo -c core.commitGraph=false rev-list --exclude-promisor-objects --objects bar >out &&\n+\tgit -C repo rev-list --exclude-promisor-objects --objects bar >out &&\n \tgrep $(git -C repo rev-parse bar) out &&\n \t! grep $FOO out\n '\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex 0544d58a6e..27fb15c83b 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -6,6 +6,11 @@ test_description='git mktag: tag object verify test'\n \n . ./test-lib.sh\n \n+# When enabled, some commands will automatically write commit-graphs.\n+# This will cause the mktag tests to fail because fsck will attempt to\n+# verify the out-of-sync commit graph.\n+GIT_TEST_COMMIT_GRAPH=0\n+\n ###########################################################\n # check the tag.sig file, expecting verify_tag() to fail,\n # and checking that the error message matches the pattern\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 295c5bd94d..4e4450af1e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -693,12 +693,33 @@ test_expect_success 'detect incorrect chunk count' '\n \t\t$GRAPH_CHUNK_LOOKUP_OFFSET\n '\n \n-test_expect_success 'git fsck (checks commit-graph)' '\n+test_expect_success 'git fsck (checks commit-graph when config set to true)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n \tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\ttest_must_fail git -c core.commitGraph=true fsck\n+'\n+\n+test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\tgit -c core.commitGraph=false fsck\n+'\n+\n+test_expect_success 'git fsck (checks commit-graph when config unset)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\ttest_when_finished \"git config core.commitGraph true\" &&\n+\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\ttest_unconfig core.commitGraph &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n \ttest_must_fail git fsck\n '\n \n-- \n2.33.0.882.g93a45727a2-goog\n\n"},{"id":"438578","messageId":"20211012174208.95161-3-chooglen@google.com","threadId":"56494","inReplyTo":"20211012174208.95161-1-chooglen@google.com","subject":"[PATCH v4 2/3] fsck: verify multi-pack-index when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-12T17:42:07Z","receivedAt":"2021-10-12T17:42:47Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Like the previous commit, change fsck to check the\n\"core_multi_pack_index\" variable set in \"repo-settings.c\" instead of\nreading the \"core.multiPackIndex\" config variable. This fixes a bug\nwhere we wouldn't verify midx if the config key was missing. This bug\nwas introduced in 18e449f86b (midx: enable core.multiPackIndex by\ndefault, 2020-09-25) where core.multiPackIndex was turned on by default.\n\nHelped-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 2 +-\n t/t5319-multi-pack-index.sh | 5 ++++-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1c4e485b66..5bbe8068ec 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -925,7 +925,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (!git_config_get_bool(\"core.multipackindex\", &i) && i) {\n+\tif (the_repository->settings.core_multi_pack_index) {\n \t\tstruct child_process midx_verify = CHILD_PROCESS_INIT;\n \t\tconst char *midx_argv[] = { \"multi-pack-index\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex bd17f308b3..38999f115f 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -452,7 +452,10 @@ test_expect_success 'verify incorrect offset' '\n test_expect_success 'git-fsck incorrect offset' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" \\\n-\t\t\"git -c core.multipackindex=true fsck\"\n+\t\t\"git -c core.multiPackIndex=true fsck\" &&\n+\ttest_unconfig core.multiPackIndex &&\n+\ttest_must_fail git fsck &&\n+\tgit -c core.multiPackIndex=false fsck\n '\n \n test_expect_success 'corrupt MIDX is not reused' '\n-- \n2.33.0.882.g93a45727a2-goog\n\n"},{"id":"438579","messageId":"20211012174208.95161-4-chooglen@google.com","threadId":"56494","inReplyTo":"20211012174208.95161-1-chooglen@google.com","subject":"[PATCH v4 3/3] gc: perform incremental repack when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-12T17:42:08Z","receivedAt":"2021-10-12T17:42:48Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"builtin/gc.c has two ways of checking if multi-pack-index is enabled:\n- git_config_get_bool() in incremental_repack_auto_condition()\n- the_repository->settings.core_multi_pack_index in\n maintenance_task_incremental_repack()\n\nThe two implementations have existed since the incremental-repack task\nwas introduced in e841a79a13 (maintenance: add incremental-repack auto\ncondition, 2020-09-25). These two values can diverge because\nprepare_repo_settings() enables the feature in the_repository->settings\nby default.\n\nIn the case where core.multiPackIndex is not set in the config, the auto\ncondition would fail, causing the incremental-repack task to not be\nrun. Because we always want to consider the default values, we should\nalways use the_repository->settings.\n\nStandardize on using the_repository->settings.core_multi_pack_index to\ncheck if multi-pack-index is enabled.\n\nSigned-off-by: Glen Choo \n---\n builtin/gc.c | 5 ++---\n t/t7900-maintenance.sh | 28 ++++++++++++++++++++++++----\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6b3de3dd51..2670931160 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1049,12 +1049,11 @@ static int maintenance_task_loose_objects(struct maintenance_run_opts *opts)\n static int incremental_repack_auto_condition(void)\n {\n \tstruct packed_git *p;\n-\tint enabled;\n \tint incremental_repack_auto_limit = 10;\n \tint count = 0;\n \n-\tif (git_config_get_bool(\"core.multiPackIndex\", &enabled) ||\n-\t !enabled)\n+\tprepare_repo_settings(the_repository);\n+\tif (!the_repository->settings.core_multi_pack_index)\n \t\treturn 0;\n \n \tgit_config_get_int(\"maintenance.incremental-repack.auto\",\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 9b9f11a8e7..74aa638475 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -336,15 +336,15 @@ test_expect_success EXPENSIVE 'incremental-repack 2g limit' '\n \t\t --no-progress --batch-size=2147483647 /dev/null &&\n \ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n \ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n \ttest_subcommand git multi-pack-index write --no-progress writes:\n\n> And since I am re-rolling, I incorporated Ævar's feedback regarding the commit\n> messages (thanks!). I considered combining patches 1 and 2, but patch 1 has a\n> grown a little to fix the CI issues, so I've decided to keep patches 1 and 2\n> separate.\n\nOK. Will replace. Thanks.\n"},{"id":"438593","messageId":"87wnmihswp.fsf@evledraar.gmail.com","threadId":"56494","inReplyTo":"20211012174208.95161-1-chooglen@google.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-12T20:34:59Z","receivedAt":"2021-10-12T20:47:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 12 2021, Glen Choo wrote:\n\n> Hi everyone! This patch was created in response to something we observed in\n> Google, where fsck failed to detect that the commit graph was invalid. We\n> initially assumed that fsck never checked the commit graph, but it turns out\n> that it does so only when core.commitgraph is set, even though we set defaults\n> for \"whether to use the commit graph\" in the repository settings.\n>\n> Instead of using the config, let's use repository settings where\n> available. Replace core.commitGraph and core.multiPackIndex with their\n> equivalent repository settings in fsck and gc.\n>\n> This re-roll is primarily motivated by the CI failures noted by Junio in [1].\n> The underlying cause is that GIT_TEST_COMMIT_GRAPH=1 (enabled in the linux-gcc\n> job) causes commands like \"git commit\" to write commit-graphs, but certain tests\n> like t/t0410-partial-clone.sh and t/t3800-mktag.sh intentionally create\n> corruptions that cause commit-graphs to become out-of-sync/invalid. Patch 1\n> fixes a bug where fsck did not check commit-graphs by default, which means that\n> these tests will now fail because they have invalid commit-graphs. The easiest\n> solution I found is to disable this confusing and noisy behavior with\n> GIT_TEST_COMMIT_GRAPH=0.\n\nThat's easy, but...\n\n> And since I am re-rolling, I incorporated Ævar's feedback regarding the commit\n> messages (thanks!). I considered combining patches 1 and 2, but patch 1 has a\n> grown a little to fix the CI issues, so I've decided to keep patches 1 and 2\n> separate.\n>\n> This series has also seen a healthy amount of interest in test style and\n> coverage. We haven't converged on a path for the future, but I'd like to think\n> that the tests here are still a step in (approximately) the right direction.\n> Hopefully this version LGT us while we figure out what to do with tests :)\n>\n> [1] https://lore.kernel.org/git/xmqqfstafyox.fsf@gitster.g/\n>\n> Changes since v3\n> * Disable GIT_TEST_COMMIT_GRAPH in tests that intentionally corrupt things in a\n> way that is incompatible with commit-graphs.\n> * Make patch 1 and 2's commit messages more concise (thanks Ævar!).\n\n...how isn't disabling those t3800-mktag.sh tests just plasting over\ncorruption that we're noticing because of your changes to (rightly) fix\nthe bug where \"fsck\" wasn't checking the graph at all?\n\nIOW haven't we just found exactly the sort of bug that\n\"GIT_TEST_COMMIT_GRAPH\" is put in place to find for us, but now instead\nof fixing it we're hiding it?\n\nIf I comment yout your addition of GIT_TEST_COMMIT_GRAPH=0 in that file\nI see that we fail N number of tests, but all of them are actually\nfallout of just this test:\n\n git replace $head_parent $head && \n git replace -f $tree $blob \n\nI.e. we've created a replacement object replacing a tree with a blob, as\npart of tests I added to test how mktag handles those sorts of weird\nedge cases.\n\nThis then causes the graph verify code to throw a hissy fit with:\n\n root tree OID for commit 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 in\n commit-graph is da5497437fd67ca928333aab79c4b4b55036ea66 !=\n 0fbca9850869684085d654f9e1380c9780802570\n\nI.e. when we wrote the graph we somehow didn't notice that the root tree\nnode we wrote is to an object that's not actually a tree? Isn't this a\nbug where some part of the commit graph writing should be doing its own\nextended OID lookup that's replacement-object aware, it didn't, and we\nwrote a corrupt graph as a result?\n\nIf there is a legitimate reason why we're not just hiding a bug we've\nturned up with these fixes let's disable that one test, not the entire\ntest file.\n\nIf you don't run the one test that fails (which is split up into 3\nindividual pieces) there's still 143 other tests that are run, all of\nthose presumably benefit from finding future bugs with\nGIT_TEST_COMMIT_GRAPH=true, particularly since the test file seems to\njust have turned up one just now...\n"},{"id":"438609","messageId":"kl6l1r4p3mi3.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"87wnmihswp.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-12T22:29:40Z","receivedAt":"2021-10-12T22:29:44Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> ...how isn't disabling those t3800-mktag.sh tests just plasting over\n> corruption that we're noticing because of your changes to (rightly) fix\n> the bug where \"fsck\" wasn't checking the graph at all?\n>\n> IOW haven't we just found exactly the sort of bug that\n> \"GIT_TEST_COMMIT_GRAPH\" is put in place to find for us, but now instead\n> of fixing it we're hiding it?\n>\n> If I comment yout your addition of GIT_TEST_COMMIT_GRAPH=0 in that file\n> I see that we fail N number of tests, but all of them are actually\n> fallout of just this test:\n>\n> git replace $head_parent $head && \n> git replace -f $tree $blob \n>\n> I.e. we've created a replacement object replacing a tree with a blob, as\n> part of tests I added to test how mktag handles those sorts of weird\n> edge cases.\n>\n> This then causes the graph verify code to throw a hissy fit with:\n>\n> root tree OID for commit 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 in\n> commit-graph is da5497437fd67ca928333aab79c4b4b55036ea66 !=\n> 0fbca9850869684085d654f9e1380c9780802570\n>\n> I.e. when we wrote the graph we somehow didn't notice that the root tree\n> node we wrote is to an object that's not actually a tree? Isn't this a\n> bug where some part of the commit graph writing should be doing its own\n> extended OID lookup that's replacement-object aware, it didn't, and we\n> wrote a corrupt graph as a result?\n>\n> If there is a legitimate reason why we're not just hiding a bug we've\n> turned up with these fixes let's disable that one test, not the entire\n> test file.\n>\n> If you don't run the one test that fails (which is split up into 3\n> individual pieces) there's still 143 other tests that are run, all of\n> those presumably benefit from finding future bugs with\n> GIT_TEST_COMMIT_GRAPH=true, particularly since the test file seems to\n> just have turned up one just now...\n\nI think this falls on my shoulders. I assumed that the failures were\nexpected behavior, not bugs. You are right that we shouldn't be\nplastering over bugs.\n\nI'll have to ask for help here because I don't know enough about mktag\nto distinguish between 'expected' and 'unexpected' failures. The best I\ncan do is to add GIT_TEST_COMMIT_GRAPH=0 + NEEDSWORK for the failing\ntests. But if that's good enough for now, I'll just do that :)\n"},{"id":"438650","messageId":"02947b5e-7ce3-4760-0d27-621c7362f839@gmail.com","threadId":"56494","inReplyTo":"87wnmihswp.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-13T13:12:57Z","receivedAt":"2021-10-13T13:13:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/12/2021 4:34 PM, Ævar Arnfjörð Bjarmason wrote:...\n> If I comment yout your addition of GIT_TEST_COMMIT_GRAPH=0 in that file\n> I see that we fail N number of tests, but all of them are actually\n> fallout of just this test:\n> \n> git replace $head_parent $head && \n> git replace -f $tree $blob \n> \n> I.e. we've created a replacement object replacing a tree with a blob, as\n> part of tests I added to test how mktag handles those sorts of weird\n> edge cases.\n> \n> This then causes the graph verify code to throw a hissy fit with:\n> \n> root tree OID for commit 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 in\n> commit-graph is da5497437fd67ca928333aab79c4b4b55036ea66 !=\n> 0fbca9850869684085d654f9e1380c9780802570\n> \n> I.e. when we wrote the graph we somehow didn't notice that the root tree\n> node we wrote is to an object that's not actually a tree? Isn't this a\n> bug where some part of the commit graph writing should be doing its own\n> extended OID lookup that's replacement-object aware, it didn't, and we\n> wrote a corrupt graph as a result?\n> \n> If there is a legitimate reason why we're not just hiding a bug we've\n> turned up with these fixes let's disable that one test, not the entire\n> test file.\n\nThe commit-graph should be disabled if replace-objects are enabled. If\nthere is a bug being introduced here it is because the commit-graph is\nbeing checked during fsck even though it would never be read when the\nreplace-objects exist.\n\nThanks,\n-Stolee\n"},{"id":"438663","messageId":"87pms8hq4s.fsf@evledraar.gmail.com","threadId":"56494","inReplyTo":"02947b5e-7ce3-4760-0d27-621c7362f839@gmail.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-13T15:57:57Z","receivedAt":"2021-10-13T15:59:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 13 2021, Derrick Stolee wrote:\n\n> On 10/12/2021 4:34 PM, Ævar Arnfjörð Bjarmason wrote:...\n>> If I comment yout your addition of GIT_TEST_COMMIT_GRAPH=0 in that file\n>> I see that we fail N number of tests, but all of them are actually\n>> fallout of just this test:\n>> \n>> git replace $head_parent $head && \n>> git replace -f $tree $blob \n>> \n>> I.e. we've created a replacement object replacing a tree with a blob, as\n>> part of tests I added to test how mktag handles those sorts of weird\n>> edge cases.\n>> \n>> This then causes the graph verify code to throw a hissy fit with:\n>> \n>> root tree OID for commit 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 in\n>> commit-graph is da5497437fd67ca928333aab79c4b4b55036ea66 !=\n>> 0fbca9850869684085d654f9e1380c9780802570\n>> \n>> I.e. when we wrote the graph we somehow didn't notice that the root tree\n>> node we wrote is to an object that's not actually a tree? Isn't this a\n>> bug where some part of the commit graph writing should be doing its own\n>> extended OID lookup that's replacement-object aware, it didn't, and we\n>> wrote a corrupt graph as a result?\n>> \n>> If there is a legitimate reason why we're not just hiding a bug we've\n>> turned up with these fixes let's disable that one test, not the entire\n>> test file.\n>\n> The commit-graph should be disabled if replace-objects are enabled. If\n> there is a bug being introduced here it is because the commit-graph is\n> being checked during fsck even though it would never be read when the\n> replace-objects exist.\n>\n> Thanks,\n> -Stolee\n\nThanks, isn't the obvious fix for this to extend your d6538246d3d\n(commit-graph: not compatible with replace objects, 2018-08-20) to do\n\"read_replace_refs = 0;\" in graph_verify()? That works for me on this\ncase.\n\nI.e. if we ignore replace refs when writing the graph, and we don't use\nit if there are any due to commit_graph_compatible(), why would we\nconsider them when verifying it?\n"},{"id":"438759","messageId":"874k9jha8h.fsf@evledraar.gmail.com","threadId":"56494","inReplyTo":"kl6l1r4p3mi3.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T15:53:22Z","receivedAt":"2021-10-14T15:55:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 12 2021, Glen Choo wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> ...how isn't disabling those t3800-mktag.sh tests just plasting over\n>> corruption that we're noticing because of your changes to (rightly) fix\n>> the bug where \"fsck\" wasn't checking the graph at all?\n>>\n>> IOW haven't we just found exactly the sort of bug that\n>> \"GIT_TEST_COMMIT_GRAPH\" is put in place to find for us, but now instead\n>> of fixing it we're hiding it?\n>>\n>> If I comment yout your addition of GIT_TEST_COMMIT_GRAPH=0 in that file\n>> I see that we fail N number of tests, but all of them are actually\n>> fallout of just this test:\n>>\n>> git replace $head_parent $head && \n>> git replace -f $tree $blob \n>>\n>> I.e. we've created a replacement object replacing a tree with a blob, as\n>> part of tests I added to test how mktag handles those sorts of weird\n>> edge cases.\n>>\n>> This then causes the graph verify code to throw a hissy fit with:\n>>\n>> root tree OID for commit 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 in\n>> commit-graph is da5497437fd67ca928333aab79c4b4b55036ea66 !=\n>> 0fbca9850869684085d654f9e1380c9780802570\n>>\n>> I.e. when we wrote the graph we somehow didn't notice that the root tree\n>> node we wrote is to an object that's not actually a tree? Isn't this a\n>> bug where some part of the commit graph writing should be doing its own\n>> extended OID lookup that's replacement-object aware, it didn't, and we\n>> wrote a corrupt graph as a result?\n>>\n>> If there is a legitimate reason why we're not just hiding a bug we've\n>> turned up with these fixes let's disable that one test, not the entire\n>> test file.\n>>\n>> If you don't run the one test that fails (which is split up into 3\n>> individual pieces) there's still 143 other tests that are run, all of\n>> those presumably benefit from finding future bugs with\n>> GIT_TEST_COMMIT_GRAPH=true, particularly since the test file seems to\n>> just have turned up one just now...\n>\n> I think this falls on my shoulders. I assumed that the failures were\n> expected behavior, not bugs. You are right that we shouldn't be\n> plastering over bugs.\n>\n> I'll have to ask for help here because I don't know enough about mktag\n> to distinguish between 'expected' and 'unexpected' failures. The best I\n> can do is to add GIT_TEST_COMMIT_GRAPH=0 + NEEDSWORK for the failing\n> tests. But if that's good enough for now, I'll just do that :)\n\nI don't think your series needs to be tasked with fixing a generic issue\nin the commit-graph you stumbled across.\n\nSo for your series I'd think the only required fix would be to narrowlry\nskip *just* the broken tests, not the entire test file.\n\nBut in this case (see\nhttp://lore.kernel.org/git/87pms8hq4s.fsf@evledraar.gmail.com) the fix\nseems rather trivial to me.\n\nI.e. just adding one line to builtin/commit-graph.c, so perhaps you can\nsee if that works for you and such a \"this bug was missed because this\nmode didn't work\" fix could be part of a re-roll of yours?\n\n"},{"id":"438764","messageId":"d553054f-a484-ba34-ef3b-1e6778211a47@gmail.com","threadId":"56494","inReplyTo":"87pms8hq4s.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-14T16:53:58Z","receivedAt":"2021-10-14T16:54:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/13/2021 11:57 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Oct 13 2021, Derrick Stolee wrote:\n> \n>> On 10/12/2021 4:34 PM, Ævar Arnfjörð Bjarmason wrote:...\n>>> If I comment yout your addition of GIT_TEST_COMMIT_GRAPH=0 in that file\n>>> I see that we fail N number of tests, but all of them are actually\n>>> fallout of just this test:\n>>>\n>>> git replace $head_parent $head && \n>>> git replace -f $tree $blob \n>>>\n>>> I.e. we've created a replacement object replacing a tree with a blob, as\n>>> part of tests I added to test how mktag handles those sorts of weird\n>>> edge cases.\n>>>\n>>> This then causes the graph verify code to throw a hissy fit with:\n>>>\n>>> root tree OID for commit 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 in\n>>> commit-graph is da5497437fd67ca928333aab79c4b4b55036ea66 !=\n>>> 0fbca9850869684085d654f9e1380c9780802570\n>>>\n>>> I.e. when we wrote the graph we somehow didn't notice that the root tree\n>>> node we wrote is to an object that's not actually a tree? Isn't this a\n>>> bug where some part of the commit graph writing should be doing its own\n>>> extended OID lookup that's replacement-object aware, it didn't, and we\n>>> wrote a corrupt graph as a result?\n>>>\n>>> If there is a legitimate reason why we're not just hiding a bug we've\n>>> turned up with these fixes let's disable that one test, not the entire\n>>> test file.\n>>\n>> The commit-graph should be disabled if replace-objects are enabled. If\n>> there is a bug being introduced here it is because the commit-graph is\n>> being checked during fsck even though it would never be read when the\n>> replace-objects exist.\n>>\n>> Thanks,\n>> -Stolee\n> \n> Thanks, isn't the obvious fix for this to extend your d6538246d3d\n> (commit-graph: not compatible with replace objects, 2018-08-20) to do\n> \"read_replace_refs = 0;\" in graph_verify()? That works for me on this\n> case.\n\nIgnoring the replace refs while verifying will allow you to verify the\non-disk commit-graph file without issue.\n \n> I.e. if we ignore replace refs when writing the graph, and we don't use\n> it if there are any due to commit_graph_compatible(), why would we\n> consider them when verifying it?\n\nI generally consider any interaction with the commit-graph while\nreplace refs are enabled to be the bug. We don't read the commit-graph\nwhen replace refs are on, so why are we verifying it?\n\nBut, I understand the desire to check the on-disk content even though\nit is not being used, so disabling replace refs to do the verify should\nbe sufficient.\n\nThanks,\n-Stolee\n\n"},{"id":"438794","messageId":"kl6l7def1c4h.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56494","inReplyTo":"d553054f-a484-ba34-ef3b-1e6778211a47@gmail.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-14T22:21:18Z","receivedAt":"2021-10-14T22:21:22Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Derrick Stolee writes:\n\n>>> The commit-graph should be disabled if replace-objects are enabled. If\n>>> there is a bug being introduced here it is because the commit-graph is\n>>> being checked during fsck even though it would never be read when the\n>>> replace-objects exist.\n>>>\n>>> Thanks,\n>>> -Stolee\n>> \n>> Thanks, isn't the obvious fix for this to extend your d6538246d3d\n>> (commit-graph: not compatible with replace objects, 2018-08-20) to do\n>> \"read_replace_refs = 0;\" in graph_verify()? That works for me on this\n>> case.\n>\n> Ignoring the replace refs while verifying will allow you to verify the\n> on-disk commit-graph file without issue.\n\nIt seems like we've converged on doing read_replace_refs = 0 \\o/\n\nIf we are going to do this twice in graph_verify() and graph_write(), is\nthere any reason why I shouldn't just do \"read_replace_refs = 0\" once in\ncmd_commit_graph()? IOW any time we do anything with commit-graphs, we\nshould just ignore replace refs because they're incompatible.\n"},{"id":"438795","messageId":"xmqq35p3b5xb.fsf@gitster.g","threadId":"56494","inReplyTo":"d553054f-a484-ba34-ef3b-1e6778211a47@gmail.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-14T22:25:04Z","receivedAt":"2021-10-14T22:25:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> I generally consider any interaction with the commit-graph while\n> replace refs are enabled to be the bug. We don't read the commit-graph\n> when replace refs are on, so why are we verifying it?\n>\n> But, I understand the desire to check the on-disk content even though\n> it is not being used, so disabling replace refs to do the verify should\n> be sufficient.\n\nThat's quite a sensible argument. Sounds like we have a way to\nsolve this that everybody agrees with.\n\nThanks.\n"},{"id":"438801","messageId":"87ilxzfa7s.fsf@evledraar.gmail.com","threadId":"56494","inReplyTo":"kl6l7def1c4h.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T23:38:02Z","receivedAt":"2021-10-14T23:38:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 14 2021, Glen Choo wrote:\n\n> Derrick Stolee writes:\n>\n>>>> The commit-graph should be disabled if replace-objects are enabled. If\n>>>> there is a bug being introduced here it is because the commit-graph is\n>>>> being checked during fsck even though it would never be read when the\n>>>> replace-objects exist.\n>>>>\n>>>> Thanks,\n>>>> -Stolee\n>>> \n>>> Thanks, isn't the obvious fix for this to extend your d6538246d3d\n>>> (commit-graph: not compatible with replace objects, 2018-08-20) to do\n>>> \"read_replace_refs = 0;\" in graph_verify()? That works for me on this\n>>> case.\n>>\n>> Ignoring the replace refs while verifying will allow you to verify the\n>> on-disk commit-graph file without issue.\n>\n> It seems like we've converged on doing read_replace_refs = 0 \\o/\n>\n> If we are going to do this twice in graph_verify() and graph_write(), is\n> there any reason why I shouldn't just do \"read_replace_refs = 0\" once in\n> cmd_commit_graph()? IOW any time we do anything with commit-graphs, we\n> should just ignore replace refs because they're incompatible.\n\nNo reason, I think that's the best way to do this.\n\nI've submitted a series to fix that verify issue, as noted in the CL\nthese patches on top of it without that disabling of the mktag tests\nwill pass with GIT_TEST_COMMIT_GRAPH=true):\nhttps://lore.kernel.org/git/cover-0.3-00000000000-20211014T233343Z-avarab@gmail.com\n"},{"id":"438855","messageId":"xmqqee8m9t72.fsf@gitster.g","threadId":"56494","inReplyTo":"xmqq35p3b5xb.fsf@gitster.g","subject":"Re: [PATCH v4 0/3] Use default values from settings instead of config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-15T15:57:37Z","receivedAt":"2021-10-15T15:57:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Derrick Stolee writes:\n>\n>> I generally consider any interaction with the commit-graph while\n>> replace refs are enabled to be the bug. We don't read the commit-graph\n>> when replace refs are on, so why are we verifying it?\n>>\n>> But, I understand the desire to check the on-disk content even though\n>> it is not being used, so disabling replace refs to do the verify should\n>> be sufficient.\n>\n> That's quite a sensible argument. Sounds like we have a way to\n> solve this that everybody agrees with.\n\nWould it be sensible to silently ignore, though? Would a warning,\nwhen we see replacement is in use and an explicit request by an end\nuser is made to make use of or update the commit-graph, be too loud?\n"},{"id":"438905","messageId":"20211015201631.91524-1-chooglen@google.com","threadId":"56494","inReplyTo":"20211012174208.95161-1-chooglen@google.com","subject":"[PATCH v5 0/3] Use default values from settings instead of config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-15T20:16:28Z","receivedAt":"2021-10-15T20:16:38Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":" Hi everyone! This patch was created in response to something we observed in\n Google, where fsck failed to detect that the commit graph was invalid. We\n initially assumed that fsck never checked the commit graph, but it turns out\n that it does so only when core.commitgraph is set, even though we set defaults\n for \"whether to use the commit graph\" in the repository settings.\n\n Instead of using the config, let's use repository settings where\n available. Replace core.commitGraph and core.multiPackIndex with their\n equivalent repository settings in fsck and gc.\n\nv5 is based off Ævar's fix (commit-graph: make \"verify\" work with replace refs,\n[1]). As Ævar noted in v4, the mktag test failures indicated buggy behavior\n\"git commit-graph verify\" when replace refs are enabled, so we should fix the\nbug instead of just disabling commit-graph.\n\nÆvar, Derrick, thanks for the productive discussion, I feel like I understand\nthis area a little better now.\n\nBarring any unseen bugs, this should be ready to merge.\n\n[1] https://lore.kernel.org/git/cover-0.3-00000000000-20211014T233343Z-avarab@gmail.com/\n\nChanges since v4\n* Rebase onto [1], which fixes a bug with commit-graph verify with replace refs\n enabled.\n* Remove GIT_TEST_COMMIT_GRAPH=0 in t3800-mktag.sh because the bug that it was\n covering up is now fixed.\n\nChanges since v3\n* Disable GIT_TEST_COMMIT_GRAPH in tests that intentionally corrupt things in a\n way that is incompatible with commit-graphs.\n* Make patch 1 and 2's commit messages more concise (thanks Ævar!).\n\nChanges since v2:\n* Various small test fixes (thanks Eric!). Most notably, I've used -c instead of\n test_config because test_config can affect the values in subsequent tests.\n* Rewording fix in patch 3 commit message\n* Refactor tests in patch 3 so that we use a single helper function instead of\n copy-pasted code\n\nChanges since v1:\n* clean up typo in patch 1 commit message \n* document the commits that patches 1 and 2 address\n* use test helpers in patch 1\n* rewrite patch 2's tests so that it's easier to tell that each test\n does something different\n* reword patch 3 commit message to explain the bug\n* add tests to patch 3\n\nGlen Choo (3):\n fsck: verify commit graph when implicitly enabled\n fsck: verify multi-pack-index when implictly enabled\n gc: perform incremental repack when implictly enabled\n\n builtin/fsck.c | 5 +++--\n builtin/gc.c | 5 ++---\n t/t0410-partial-clone.sh | 6 +++++-\n t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-\n t/t5319-multi-pack-index.sh | 5 ++++-\n t/t7900-maintenance.sh | 28 ++++++++++++++++++++++++----\n 6 files changed, 60 insertions(+), 12 deletions(-)\n\nRange-diff against v4:\n1: aac1253e7b ! 1: 567d40849a fsck: verify commit graph when implicitly enabled\n @@ Commit message\n \n Add tests to \"t5318-commit-graph.sh\" to verify that fsck checks the\n commit-graph as expected for the 3 values of core.commitGraph. Also,\n - disable GIT_TEST_COMMIT_GRAPH for tests that use fsck in ways that\n - assume that commit-graph checking is disabled (t/t3800-mktag.sh,\n - t/t0410-partial-clone.sh).\n + disable GIT_TEST_COMMIT_GRAPH in t/t0410-partial-clone.sh because some\n + test cases use fsck in ways that assume that commit-graph checking is\n + disabled.\n \n Helped-by: Ævar Arnfjörð Bjarmason \n Signed-off-by: Glen Choo \n @@ t/t0410-partial-clone.sh: test_expect_success 'rev-list stops traversal at missi\n \t! grep $FOO out\n '\n \n - ## t/t3800-mktag.sh ##\n -@@ t/t3800-mktag.sh: test_description='git mktag: tag object verify test'\n - \n - . ./test-lib.sh\n - \n -+# When enabled, some commands will automatically write commit-graphs.\n -+# This will cause the mktag tests to fail because fsck will attempt to\n -+# verify the out-of-sync commit graph.\n -+GIT_TEST_COMMIT_GRAPH=0\n -+\n - ###########################################################\n - # check the tag.sig file, expecting verify_tag() to fail,\n - # and checking that the error message matches the pattern\n -\n ## t/t5318-commit-graph.sh ##\n @@ t/t5318-commit-graph.sh: test_expect_success 'detect incorrect chunk count' '\n \t\t$GRAPH_CHUNK_LOOKUP_OFFSET\n2: ed64983430 = 2: 7cb44a7aeb fsck: verify multi-pack-index when implictly enabled\n3: 821b492d8b = 3: a436c45af7 gc: perform incremental repack when implictly enabled\n-- \n2.33.0.1079.g6e70778dc9-goog\n\n"},{"id":"438906","messageId":"20211015201631.91524-2-chooglen@google.com","threadId":"56494","inReplyTo":"20211015201631.91524-1-chooglen@google.com","subject":"[PATCH v5 1/3] fsck: verify commit graph when implicitly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-15T20:16:29Z","receivedAt":"2021-10-15T20:16:41Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Change fsck to check the \"core_commit_graph\" variable set in\n\"repo-settings.c\" instead of reading the \"core.commitGraph\" variable.\nThis fixes a bug where we wouldn't verify the commit-graph if the\nconfig key was missing. This bug was introduced in\n31b1de6a09 (commit-graph: turn on commit-graph by default, 2019-08-13),\nwhere core.commitGraph was turned on by default.\n\nAdd tests to \"t5318-commit-graph.sh\" to verify that fsck checks the\ncommit-graph as expected for the 3 values of core.commitGraph. Also,\ndisable GIT_TEST_COMMIT_GRAPH in t/t0410-partial-clone.sh because some\ntest cases use fsck in ways that assume that commit-graph checking is\ndisabled.\n\nHelped-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 3 ++-\n t/t0410-partial-clone.sh | 6 +++++-\n t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-\n 3 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex b42b6fe21f..1c4e485b66 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -803,6 +803,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tfsck_enable_object_names(&fsck_walk_options);\n \n \tgit_config(git_fsck_config, &fsck_obj_options);\n+\tprepare_repo_settings(the_repository);\n \n \tif (connectivity_only) {\n \t\tfor_each_loose_object(mark_loose_for_connectivity, NULL, 0);\n@@ -908,7 +909,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tcheck_connectivity();\n \n-\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n+\tif (the_repository->settings.core_commit_graph) {\n \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex bba679685f..c76485b1b6 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -6,6 +6,10 @@ test_description='partial clone'\n \n # missing promisor objects cause repacks which write bitmaps to fail\n GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0\n+# When enabled, some commands will write commit-graphs. This causes fsck\n+# to fail when delete_object() is called because fsck will attempt to\n+# verify the out-of-sync commit graph.\n+GIT_TEST_COMMIT_GRAPH=0\n \n delete_object () {\n \trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n@@ -322,7 +326,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tGIT_TEST_COMMIT_GRAPH=0 git -C repo -c core.commitGraph=false rev-list --exclude-promisor-objects --objects bar >out &&\n+\tgit -C repo rev-list --exclude-promisor-objects --objects bar >out &&\n \tgrep $(git -C repo rev-parse bar) out &&\n \t! grep $FOO out\n '\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 84d122a7ae..f516fda7cc 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -694,12 +694,33 @@ test_expect_success 'detect incorrect chunk count' '\n \t\t$GRAPH_CHUNK_LOOKUP_OFFSET\n '\n \n-test_expect_success 'git fsck (checks commit-graph)' '\n+test_expect_success 'git fsck (checks commit-graph when config set to true)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n \t\t\"incorrect checksum\" &&\n \tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\ttest_must_fail git -c core.commitGraph=true fsck\n+'\n+\n+test_expect_success 'git fsck (ignores commit-graph when config set to false)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n+\tgit -c core.commitGraph=false fsck\n+'\n+\n+test_expect_success 'git fsck (checks commit-graph when config unset)' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\ttest_when_finished \"git config core.commitGraph true\" &&\n+\n+\tgit fsck &&\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_FOOTER \"\\00\" \\\n+\t\t\"incorrect checksum\" &&\n+\ttest_unconfig core.commitGraph &&\n+\tcp commit-graph-pre-write-test $objdir/info/commit-graph &&\n \ttest_must_fail git fsck\n '\n \n-- \n2.33.0.1079.g6e70778dc9-goog\n\n"},{"id":"438907","messageId":"20211015201631.91524-3-chooglen@google.com","threadId":"56494","inReplyTo":"20211015201631.91524-1-chooglen@google.com","subject":"[PATCH v5 2/3] fsck: verify multi-pack-index when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-15T20:16:30Z","receivedAt":"2021-10-15T20:16:43Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Like the previous commit, change fsck to check the\n\"core_multi_pack_index\" variable set in \"repo-settings.c\" instead of\nreading the \"core.multiPackIndex\" config variable. This fixes a bug\nwhere we wouldn't verify midx if the config key was missing. This bug\nwas introduced in 18e449f86b (midx: enable core.multiPackIndex by\ndefault, 2020-09-25) where core.multiPackIndex was turned on by default.\n\nHelped-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n builtin/fsck.c | 2 +-\n t/t5319-multi-pack-index.sh | 5 ++++-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1c4e485b66..5bbe8068ec 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -925,7 +925,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (!git_config_get_bool(\"core.multipackindex\", &i) && i) {\n+\tif (the_repository->settings.core_multi_pack_index) {\n \t\tstruct child_process midx_verify = CHILD_PROCESS_INIT;\n \t\tconst char *midx_argv[] = { \"multi-pack-index\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex bd17f308b3..38999f115f 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -452,7 +452,10 @@ test_expect_success 'verify incorrect offset' '\n test_expect_success 'git-fsck incorrect offset' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n \t\t\"incorrect object offset\" \\\n-\t\t\"git -c core.multipackindex=true fsck\"\n+\t\t\"git -c core.multiPackIndex=true fsck\" &&\n+\ttest_unconfig core.multiPackIndex &&\n+\ttest_must_fail git fsck &&\n+\tgit -c core.multiPackIndex=false fsck\n '\n \n test_expect_success 'corrupt MIDX is not reused' '\n-- \n2.33.0.1079.g6e70778dc9-goog\n\n"},{"id":"438908","messageId":"20211015201631.91524-4-chooglen@google.com","threadId":"56494","inReplyTo":"20211015201631.91524-1-chooglen@google.com","subject":"[PATCH v5 3/3] gc: perform incremental repack when implictly enabled","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-10-15T20:16:31Z","receivedAt":"2021-10-15T20:16:47Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"builtin/gc.c has two ways of checking if multi-pack-index is enabled:\n- git_config_get_bool() in incremental_repack_auto_condition()\n- the_repository->settings.core_multi_pack_index in\n maintenance_task_incremental_repack()\n\nThe two implementations have existed since the incremental-repack task\nwas introduced in e841a79a13 (maintenance: add incremental-repack auto\ncondition, 2020-09-25). These two values can diverge because\nprepare_repo_settings() enables the feature in the_repository->settings\nby default.\n\nIn the case where core.multiPackIndex is not set in the config, the auto\ncondition would fail, causing the incremental-repack task to not be\nrun. Because we always want to consider the default values, we should\nalways use the_repository->settings.\n\nStandardize on using the_repository->settings.core_multi_pack_index to\ncheck if multi-pack-index is enabled.\n\nSigned-off-by: Glen Choo \n---\n builtin/gc.c | 5 ++---\n t/t7900-maintenance.sh | 28 ++++++++++++++++++++++++----\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6b3de3dd51..2670931160 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1049,12 +1049,11 @@ static int maintenance_task_loose_objects(struct maintenance_run_opts *opts)\n static int incremental_repack_auto_condition(void)\n {\n \tstruct packed_git *p;\n-\tint enabled;\n \tint incremental_repack_auto_limit = 10;\n \tint count = 0;\n \n-\tif (git_config_get_bool(\"core.multiPackIndex\", &enabled) ||\n-\t !enabled)\n+\tprepare_repo_settings(the_repository);\n+\tif (!the_repository->settings.core_multi_pack_index)\n \t\treturn 0;\n \n \tgit_config_get_int(\"maintenance.incremental-repack.auto\",\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 9b9f11a8e7..74aa638475 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -336,15 +336,15 @@ test_expect_success EXPENSIVE 'incremental-repack 2g limit' '\n \t\t --no-progress --batch-size=2147483647 /dev/null &&\n \ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n \ttest_subcommand ! git multi-pack-index write --no-progress /dev/null &&\n \ttest_subcommand git multi-pack-index write --no-progress