{"thread":{"id":"57605","subject":"[RFC PATCH] repo-settings: set defaults even when not in a repo","startedAt":"2022-03-23T18:03:12Z","lastAt":"2022-07-14T22:48:39Z","messageCount":27,"participants":["Josh Steadmon","Derrick Stolee","Taylor Blau","Victoria Dye","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"452008","messageId":"1b27e0b115f858a422e0a2891688227be8f3db01.1648055915.git.steadmon@google.com","threadId":"57605","inReplyTo":null,"subject":"[RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-03-23T18:03:05Z","receivedAt":"2022-03-23T18:03:12Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"prepare_repo_settings() initializes a `struct repository` with various\ndefault config options and settings read from a repository-local config\nfile. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\nin git repos), prepare_repo_settings was changed to issue a BUG() if it\nis called by a process whose CWD is not a Git repository. This approach\nwas suggested in [1].\n\nThis breaks fuzz-commit-graph, which attempts to parse arbitrary\nfuzzing-engine-provided bytes as a commit graph file.\ncommit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\nsince we run the fuzz tests without a valid repository, we are hitting\nthe BUG() from 44c7e62 for every test case.\n\nFix this by refactoring prepare_repo_settings() such that it sets\ndefault options unconditionally; if its process is in a Git repository,\nit will also load settings from the local config. This eliminates the\nneed for a BUG() when not in a repository.\n\nConcerns:\n\nAre any callers strictly dependent on having a BUG() here? I suspect\nthat the worst that would happen is that rather than this BUG(), the\ncaller would later hit its own BUG() or die(), so I do not think this is\na blocker. Additionally, every builtin that directly calls\nprepare_repo_settings is either marked as RUN_SETUP, which means we\nwould die() prior to calling it anyway, or checks on its own before\ncalling it (builtin/diff.c). There are several callers in library code,\nthough, and I have not tracked down how all of those are used.\n\nAlternatives considered:\n\nSetting up a valid repository for fuzz testing would avoid triggering\nthis bug, but would unacceptably slow down the test cases.\n\nRefactoring parse_commit_graph() in such a way that the fuzz test has an\nalternate entry point that avoids calling prepare_repo_settings() might\nbe possible, but would be a much larger change than this one. It would\nalso run the risk that the changes would be so extensive that the fuzzer\nwould be merely testing its own custom commit-graph implementation,\nrather than the one that's actually used in the real world.\n\n[1]: https://lore.kernel.org/git/xmqqcznh8913.fsf@gitster.g/\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\n repo-settings.c | 111 ++++++++++++++++++++++++------------------------\n 1 file changed, 55 insertions(+), 56 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex b4fbd16cdc..d154b37007 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -17,9 +17,6 @@ void prepare_repo_settings(struct repository *r)\n \tchar *strval;\n \tint manyfiles;\n \n-\tif (!r->gitdir)\n-\t\tBUG(\"Cannot add settings for uninitialized repository\");\n-\n \tif (r->settings.initialized++)\n \t\treturn;\n \n@@ -28,27 +25,63 @@ void prepare_repo_settings(struct repository *r)\n \tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n \tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n \n-\t/* Booleans config or default, cascades to other settings */\n-\trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n-\trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n+\tif (r->gitdir) {\n+\t\t/* Booleans config or default, cascades to other settings */\n+\t\trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n+\t\trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n \n-\t/* Defaults modified by feature.* */\n-\tif (experimental) {\n-\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n-\t}\n-\tif (manyfiles) {\n-\t\tr->settings.index_version = 4;\n-\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n-\t}\n+\t\t/* Defaults modified by feature.* */\n+\t\tif (experimental) {\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n+\t\t}\n+\t\tif (manyfiles) {\n+\t\t\tr->settings.index_version = 4;\n+\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n+\t\t}\n+\n+\t\t/* Boolean config or default, does not cascade (simple)  */\n+\t\trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n+\t\trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n+\t\trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n+\t\trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\n+\t\trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n+\t\trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n+\t\trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n \n-\t/* Boolean config or default, does not cascade (simple)  */\n-\trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n-\trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n-\trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n-\trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\n-\trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n-\trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n-\trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n+\t\t/*\n+\t\t * Non-boolean config\n+\t\t */\n+\t\tif (!repo_config_get_int(r, \"index.version\", &value))\n+\t\t\tr->settings.index_version = value;\n+\n+\t\tif (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n+\t\t\tint v = git_parse_maybe_bool(strval);\n+\n+\t\t\t/*\n+\t\t\t * If it's set to \"keep\", or some other non-boolean\n+\t\t\t * value then \"v < 0\". Then we do nothing and keep it\n+\t\t\t * at the default of UNTRACKED_CACHE_KEEP.\n+\t\t\t */\n+\t\t\tif (v >= 0)\n+\t\t\t\tr->settings.core_untracked_cache = v ?\n+\t\t\t\t\tUNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;\n+\t\t\tfree(strval);\n+\t\t}\n+\n+\t\tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n+\t\t\tint fetch_default = r->settings.fetch_negotiation_algorithm;\n+\t\t\tif (!strcasecmp(strval, \"skipping\"))\n+\t\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n+\t\t\telse if (!strcasecmp(strval, \"noop\"))\n+\t\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n+\t\t\telse if (!strcasecmp(strval, \"consecutive\"))\n+\t\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n+\t\t\telse if (!strcasecmp(strval, \"default\"))\n+\t\t\t\tr->settings.fetch_negotiation_algorithm = fetch_default;\n+\t\t\telse\n+\t\t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n+\t\t}\n+\t}\n \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\n@@ -60,40 +93,6 @@ void prepare_repo_settings(struct repository *r)\n \tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))\n \t\tr->settings.core_multi_pack_index = 1;\n \n-\t/*\n-\t * Non-boolean config\n-\t */\n-\tif (!repo_config_get_int(r, \"index.version\", &value))\n-\t\tr->settings.index_version = value;\n-\n-\tif (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n-\t\tint v = git_parse_maybe_bool(strval);\n-\n-\t\t/*\n-\t\t * If it's set to \"keep\", or some other non-boolean\n-\t\t * value then \"v < 0\". Then we do nothing and keep it\n-\t\t * at the default of UNTRACKED_CACHE_KEEP.\n-\t\t */\n-\t\tif (v >= 0)\n-\t\t\tr->settings.core_untracked_cache = v ?\n-\t\t\t\tUNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;\n-\t\tfree(strval);\n-\t}\n-\n-\tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n-\t\tint fetch_default = r->settings.fetch_negotiation_algorithm;\n-\t\tif (!strcasecmp(strval, \"skipping\"))\n-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n-\t\telse if (!strcasecmp(strval, \"noop\"))\n-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n-\t\telse if (!strcasecmp(strval, \"consecutive\"))\n-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n-\t\telse if (!strcasecmp(strval, \"default\"))\n-\t\t\tr->settings.fetch_negotiation_algorithm = fetch_default;\n-\t\telse\n-\t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n-\t}\n-\n \t/*\n \t * This setting guards all index reads to require a full index\n \t * over a sparse index. After suitable guards are placed in the\n\nbase-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7\n-- \n2.35.1.894.gb6a874cedc-goog\n\n"},{"id":"452018","messageId":"471ca70d-0da1-8c4f-16bc-3019706931bd@github.com","threadId":"57605","inReplyTo":"1b27e0b115f858a422e0a2891688227be8f3db01.1648055915.git.steadmon@google.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-23T19:22:13Z","receivedAt":"2022-03-23T19:22:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/23/2022 2:03 PM, Josh Steadmon wrote:\n> prepare_repo_settings() initializes a `struct repository` with various\n> default config options and settings read from a repository-local config\n> file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\n> in git repos), prepare_repo_settings was changed to issue a BUG() if it\n> is called by a process whose CWD is not a Git repository. This approach\n> was suggested in [1].\n> \n> This breaks fuzz-commit-graph, which attempts to parse arbitrary\n> fuzzing-engine-provided bytes as a commit graph file.\n> commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n> since we run the fuzz tests without a valid repository, we are hitting\n> the BUG() from 44c7e62 for every test case.\n> \n> Fix this by refactoring prepare_repo_settings() such that it sets\n> default options unconditionally; if its process is in a Git repository,\n> it will also load settings from the local config. This eliminates the\n> need for a BUG() when not in a repository.\n\nI think you have the right idea and this can work.\n \n> -\t/* Booleans config or default, cascades to other settings */\n> -\trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n> -\trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n> +\tif (r->gitdir) {\n> +\t\t/* Booleans config or default, cascades to other settings */\n> +\t\trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n> +\t\trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n\nHowever, this giant if block is going to be a bit unwieldy,\nespecially as we add settings in the future.\n\nIgnoring whitespace, this is your diff:\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex b4fbd16cdc..d154b37007 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -17,9 +17,6 @@ void prepare_repo_settings(struct repository *r)\n \tchar *strval;\n \tint manyfiles;\n \n-\tif (!r->gitdir)\n-\t\tBUG(\"Cannot add settings for uninitialized repository\");\n-\n \tif (r->settings.initialized++)\n \t\treturn;\n \n@@ -28,6 +25,7 @@ void prepare_repo_settings(struct repository *r)\n \tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n \tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n \n+\tif (r->gitdir) {\n \t\t/* Booleans config or default, cascades to other settings */\n \t\trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n \t\trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n@@ -50,16 +48,6 @@ void prepare_repo_settings(struct repository *r)\n \t\trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \t\trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n\n(Adding the if)\n\n-\t/*\n-\t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\n-\t * either it *or* the config sets\n-\t * r->settings.core_multi_pack_index if true. We don't take\n-\t * the environment variable if it exists (even if false) over\n-\t * any config, as in most other cases.\n-\t */\n-\tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))\n-\t\tr->settings.core_multi_pack_index = 1;\n-\n \t\t/*\n \t\t * Non-boolean config\n \t\t */\n@@ -93,6 +81,17 @@ void prepare_repo_settings(struct repository *r)\n \t\t\telse\n \t\t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n \t\t}\n+\t}\n+\n+\t/*\n+\t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\n+\t * either it *or* the config sets\n+\t * r->settings.core_multi_pack_index if true. We don't take\n+\t * the environment variable if it exists (even if false) over\n+\t * any config, as in most other cases.\n+\t */\n+\tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))\n+\t\tr->settings.core_multi_pack_index = 1;\n \n(Moving this to group with other no-dir settings.)\n\nI think what you're really trying to do is\n\n\tif (r->gitdir)\n\t\t(Load settings that require a repo)\n\n\t(Load settings that work without a repo.)\n\nI think that you'd be better off extracting the two types of config\ninto helper methods (prepare_gitdir_settings()/prepare_nodir_settings()?)\nacross two patches, then in a third removing the BUG and inserting the\nif (r->gitdir) above.\n\nDoes that seem reasonable?\n\nThanks,\n-Stolee\n"},{"id":"452020","messageId":"Yjt6mLIfw0V3aVTO@nand.local","threadId":"57605","inReplyTo":"471ca70d-0da1-8c4f-16bc-3019706931bd@github.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-23T19:52:56Z","receivedAt":"2022-03-23T19:53:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 23, 2022 at 03:22:13PM -0400, Derrick Stolee wrote:\n> On 3/23/2022 2:03 PM, Josh Steadmon wrote:\n> > prepare_repo_settings() initializes a `struct repository` with various\n> > default config options and settings read from a repository-local config\n> > file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\n> > in git repos), prepare_repo_settings was changed to issue a BUG() if it\n> > is called by a process whose CWD is not a Git repository. This approach\n> > was suggested in [1].\n> >\n> > This breaks fuzz-commit-graph, which attempts to parse arbitrary\n> > fuzzing-engine-provided bytes as a commit graph file.\n> > commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n> > since we run the fuzz tests without a valid repository, we are hitting\n> > the BUG() from 44c7e62 for every test case.\n> >\n> > Fix this by refactoring prepare_repo_settings() such that it sets\n> > default options unconditionally; if its process is in a Git repository,\n> > it will also load settings from the local config. This eliminates the\n> > need for a BUG() when not in a repository.\n>\n> I think you have the right idea and this can work.\n\nHmmm. To me this feels like bending over backwards in\n`prepare_repo_settings()` to accommodate one particular caller. I'm not\nnecessarily opposed to it, but it does feel strange to make\n`prepare_repo_settings()` a noop here, since I would expect that any\ncallers who do want to call `prepare_repo_settings()` are likely\nconvinced that they are inside of a repository, and it probably should\nbe a BUG() if they aren't.\n\nI was initially thinking that Josh's alternative of introducing and\ncalling a lower-level version of `prepare_commit_graph()` that doesn't\nrequire the use of any repository settings would make sense.\n\nBut when I started looking at implementing it, I became confused at how\nthis is supposed to work at all without using a repository. We depend on\nthe values parsed from:\n\n  - commitGraph.generationVersion\n  - commitGraph.readChangedPaths\n\nto determine which part(s) of the commit graph we do and don't read.\n\nLooking around, I think I probably inadvertently broke this in\nab14d0676c (commit-graph: pass a 'struct repository *' in more places,\n2020-09-09). But prior to ab14d0676c, neither of those settings existed,\nso parsing the commit graph was a pure function of the commit graph's\ncontents alone, and didn't rely on the existence of a repository.\n\nWe could pretend as if `commitGraph.generationVersion` is always \"2\" and\n`commitGraph.readChangedPaths` is always \"true\", and I think that would\nstill give us good-enough coverage.\n\nI assume that libFuzzer doesn't give us a convenient way to stub out a\nrepository. Maybe we should have a variant of `parse_commit_graph` that\ninstead takes a `struct repo_settings` filled out with the defaults?\n\nWe could use that to teach libFuzzer about additional states that the\ncommit graph parser can be in, too (though probably outside the scope of\nthis patch).\n\nI tried to sketch all of this out, which seems to work. Let me know what\nyou think:\n\n--- 8< ---\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex adffd020dd..3d5aa536c2 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -96,13 +96,6 @@ define_commit_slab(commit_graph_data_slab, struct commit_graph_data);\n static struct commit_graph_data_slab commit_graph_data_slab =\n \tCOMMIT_SLAB_INIT(1, commit_graph_data_slab);\n\n-static int get_configured_generation_version(struct repository *r)\n-{\n-\tint version = 2;\n-\trepo_config_get_int(r, \"commitgraph.generationversion\", &version);\n-\treturn version;\n-}\n-\n uint32_t commit_graph_position(const struct commit *c)\n {\n \tstruct commit_graph_data *data =\n@@ -335,6 +328,13 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,\n\n struct commit_graph *parse_commit_graph(struct repository *r,\n \t\t\t\t\tvoid *graph_map, size_t graph_size)\n+{\n+\tprepare_repo_settings(r);\n+\treturn parse_commit_graph_settings(&r->settings, graph_map, graph_size);\n+}\n+\n+struct commit_graph *parse_commit_graph_settings(struct repo_settings *s,\n+\t\t\t\t\t\t void *graph_map, size_t graph_size)\n {\n \tconst unsigned char *data;\n \tstruct commit_graph *graph;\n@@ -371,8 +371,6 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \t\treturn NULL;\n \t}\n\n-\tprepare_repo_settings(r);\n-\n \tgraph = alloc_commit_graph();\n\n \tgraph->hash_len = the_hash_algo->rawsz;\n@@ -402,7 +400,7 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\n \tpair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n\n-\tif (get_configured_generation_version(r) >= 2) {\n+\tif (s->commit_graph_generation_version >= 2) {\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n \t\t\t&graph->chunk_generation_data);\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n@@ -412,7 +410,7 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \t\t\tgraph->read_generation_data = 1;\n \t}\n\n-\tif (r->settings.commit_graph_read_changed_paths) {\n+\tif (s->commit_graph_read_changed_paths) {\n \t\tpair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n \t\t\t   &graph->chunk_bloom_indexes);\n \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n@@ -2299,7 +2297,7 @@ int write_commit_graph(struct object_directory *odb,\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n \tctx->opts = opts;\n \tctx->total_bloom_filter_data_size = 0;\n-\tctx->write_generation_data = (get_configured_generation_version(r) == 2);\n+\tctx->write_generation_data = (r->settings.commit_graph_generation_version == 2);\n \tctx->num_generation_data_overflows = 0;\n\n \tbloom_settings.bits_per_entry = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY\",\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 2e3ac35237..1fdca7a927 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -95,6 +95,8 @@ struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n struct commit_graph *parse_commit_graph(struct repository *r,\n \t\t\t\t\tvoid *graph_map, size_t graph_size);\n+struct commit_graph *parse_commit_graph_settings(struct repo_settings *s,\n+\t\t\t\t\tvoid *graph_map, size_t graph_size);\n\n /*\n  * Return 1 if and only if the repository has a commit-graph\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nindex e7cf6d5b0f..07ef4643d8 100644\n--- a/fuzz-commit-graph.c\n+++ b/fuzz-commit-graph.c\n@@ -11,7 +11,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tstruct commit_graph *g;\n\n \tinitialize_the_repository();\n-\tg = parse_commit_graph(the_repository, (void *)data, size);\n+\tg = parse_commit_graph_settings(the_repository->settings, (void *)data, size);\n \trepo_clear(the_repository);\n \tfree_commit_graph(g);\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex b4fbd16cdc..a7e5d10b27 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -10,6 +10,13 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n \t\t*dest = def;\n }\n\n+static void repo_cfg_int(struct repository *r, const char *key, int *dest,\n+\t\t\t int def)\n+{\n+\tif (repo_config_get_int(r, key, dest))\n+\t\t*dest = def;\n+}\n+\n void prepare_repo_settings(struct repository *r)\n {\n \tint experimental;\n@@ -43,6 +50,7 @@ void prepare_repo_settings(struct repository *r)\n\n \t/* Boolean config or default, does not cascade (simple)  */\n \trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n+\trepo_cfg_int(r, \"commitgraph.generationversion\", &r->settings.commit_graph_generation_version, 2);\n \trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n \trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n \trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\ndiff --git a/repository.h b/repository.h\nindex e29f361703..6c40ea3f1b 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -29,6 +29,7 @@ struct repo_settings {\n \tint initialized;\n\n \tint core_commit_graph;\n+\tint commit_graph_generation_version;\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n \tint fetch_write_commit_graph;\n\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"452023","messageId":"fcfdcbb9-761a-0d34-7d36-61e0ef279922@github.com","threadId":"57605","inReplyTo":"1b27e0b115f858a422e0a2891688227be8f3db01.1648055915.git.steadmon@google.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-23T20:11:10Z","receivedAt":"2022-03-23T20:11:16Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Josh Steadmon wrote:\n> prepare_repo_settings() initializes a `struct repository` with various\n> default config options and settings read from a repository-local config\n> file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\n> in git repos), prepare_repo_settings was changed to issue a BUG() if it\n> is called by a process whose CWD is not a Git repository. This approach\n> was suggested in [1].\n> \n> This breaks fuzz-commit-graph, which attempts to parse arbitrary\n> fuzzing-engine-provided bytes as a commit graph file.\n> commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n> since we run the fuzz tests without a valid repository, we are hitting\n> the BUG() from 44c7e62 for every test case.\n> \n> Fix this by refactoring prepare_repo_settings() such that it sets\n> default options unconditionally; if its process is in a Git repository,\n> it will also load settings from the local config. This eliminates the\n> need for a BUG() when not in a repository.\n> \n\nI think the decision of whether to go with this approach or the alternative\nlisted below depends on the validity of a 'repository' without a gitdir. \n\nAs far as I can tell, there is an implicit conflict between the changes in:\n\n1. b66d84756f (commit-graph: respect 'commitGraph.readChangedPaths',\n   2020-09-09)\n2. 44c7e62e51 (repo-settings: prepare_repo_settings only in git repos,\n   2021-12-06) (as you pointed out in your message)\n\nThe former says that commit-graph should use a repository setting (implying\nit needs a valid repository), and the latter says that you need a valid\ngitdir to get repository settings.\n\nSo to me, how to proceed depends on whether a repository can be \"valid\"\nwithout a gitdir or not:\n\n* If it is valid (or not-completely-broken/semi-valid - i.e., not a BUG()),\n  then your approach is probably fine - the 'BUG()' should be removed from\n  'prepare_repo_settings()'. But, to make sure we don't run into this\n  question again, 'prepare_repo_settings()' should be audited for any\n  settings/setup that implicitly require a gitdir and updated accordingly,\n  along with a comment somewhere saying that the 'gitdir' isn't required.\n* If it is not valid, then this use in 'fuzz-commit-graph' tests is fragile\n  (even if it's not a problem right now) - 'prepare_repo_settings()' could\n  start needing the gitdir, or \"the_hash_algo\" could become invalid, or some\n  other problem could arise from the invalid repo and the tests would break.\n\nI don't have a good answer to this question, but someone with more\nexperience in this area might be able to help.\n\n> Concerns:\n> \n> Are any callers strictly dependent on having a BUG() here? I suspect\n> that the worst that would happen is that rather than this BUG(), the\n> caller would later hit its own BUG() or die(), so I do not think this is\n> a blocker. Additionally, every builtin that directly calls\n> prepare_repo_settings is either marked as RUN_SETUP, which means we\n> would die() prior to calling it anyway, or checks on its own before\n> calling it (builtin/diff.c). There are several callers in library code,\n> though, and I have not tracked down how all of those are used.\n> \n> Alternatives considered:\n> \n> Setting up a valid repository for fuzz testing would avoid triggering\n> this bug, but would unacceptably slow down the test cases.\n> \n> Refactoring parse_commit_graph() in such a way that the fuzz test has an\n> alternate entry point that avoids calling prepare_repo_settings() might\n> be possible, but would be a much larger change than this one. It would\n> also run the risk that the changes would be so extensive that the fuzzer\n> would be merely testing its own custom commit-graph implementation,\n> rather than the one that's actually used in the real world.\n> \n\nIf the 'repository' really is *completely* invalid without a gitdir, I don't\nthink the refactor of 'commit-graph.c' would necessarily be too bad -\ncertainly not so extensive that it's not testing \"real world\" behavior:\n\n* Make 'parse_commit_graph()' only call 'prepare_repo_settings()' if the\n  repo has a valid gitdir (guarded like 'git diff' is)\n* Guard all use use of 'r' in 'parse_commit_graph()' based on the existence\n  of 'gitdir', with fallbacks on e.g. global config settings. From what I\n  can see, the only usage is:\n    * The check for 'r->settings.commit_graph_read_changed_paths'\n    * The call to 'get_configured_generation_version()'\n    * The use of 'the_hash_algo->raw_sz'\n\n> [1]: https://lore.kernel.org/git/xmqqcznh8913.fsf@gitster.g/\n> \n> Signed-off-by: Josh Steadmon <steadmon@google.com>\n> ---\n>  repo-settings.c | 111 ++++++++++++++++++++++++------------------------\n>  1 file changed, 55 insertions(+), 56 deletions(-)\n> \n> diff --git a/repo-settings.c b/repo-settings.c\n> index b4fbd16cdc..d154b37007 100644\n> --- a/repo-settings.c\n> +++ b/repo-settings.c\n> @@ -17,9 +17,6 @@ void prepare_repo_settings(struct repository *r)\n>  \tchar *strval;\n>  \tint manyfiles;\n>  \n> -\tif (!r->gitdir)\n> -\t\tBUG(\"Cannot add settings for uninitialized repository\");\n> -\n>  \tif (r->settings.initialized++)\n>  \t\treturn;\n>  \n> @@ -28,27 +25,63 @@ void prepare_repo_settings(struct repository *r)\n>  \tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n>  \tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n>  \n> -\t/* Booleans config or default, cascades to other settings */\n> -\trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n> -\trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n> +\tif (r->gitdir) {\n> +\t\t/* Booleans config or default, cascades to other settings */\n> +\t\trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n> +\t\trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n>  \n> -\t/* Defaults modified by feature.* */\n> -\tif (experimental) {\n> -\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> -\t}\n> -\tif (manyfiles) {\n> -\t\tr->settings.index_version = 4;\n> -\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n> -\t}\n> +\t\t/* Defaults modified by feature.* */\n> +\t\tif (experimental) {\n> +\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> +\t\t}\n> +\t\tif (manyfiles) {\n> +\t\t\tr->settings.index_version = 4;\n> +\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n> +\t\t}\n> +\n> +\t\t/* Boolean config or default, does not cascade (simple)  */\n> +\t\trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n> +\t\trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n> +\t\trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n> +\t\trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\n> +\t\trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n> +\t\trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n> +\t\trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n>  \n> -\t/* Boolean config or default, does not cascade (simple)  */\n> -\trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n> -\trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n> -\trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n> -\trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\n> -\trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n> -\trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n> -\trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n> +\t\t/*\n> +\t\t * Non-boolean config\n> +\t\t */\n> +\t\tif (!repo_config_get_int(r, \"index.version\", &value))\n> +\t\t\tr->settings.index_version = value;\n> +\n> +\t\tif (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n> +\t\t\tint v = git_parse_maybe_bool(strval);\n> +\n> +\t\t\t/*\n> +\t\t\t * If it's set to \"keep\", or some other non-boolean\n> +\t\t\t * value then \"v < 0\". Then we do nothing and keep it\n> +\t\t\t * at the default of UNTRACKED_CACHE_KEEP.\n> +\t\t\t */\n> +\t\t\tif (v >= 0)\n> +\t\t\t\tr->settings.core_untracked_cache = v ?\n> +\t\t\t\t\tUNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;\n> +\t\t\tfree(strval);\n> +\t\t}\n> +\n> +\t\tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n> +\t\t\tint fetch_default = r->settings.fetch_negotiation_algorithm;\n> +\t\t\tif (!strcasecmp(strval, \"skipping\"))\n> +\t\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> +\t\t\telse if (!strcasecmp(strval, \"noop\"))\n> +\t\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n> +\t\t\telse if (!strcasecmp(strval, \"consecutive\"))\n> +\t\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n> +\t\t\telse if (!strcasecmp(strval, \"default\"))\n> +\t\t\t\tr->settings.fetch_negotiation_algorithm = fetch_default;\n> +\t\t\telse\n> +\t\t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n> +\t\t}\n> +\t}\n>  \n>  \t/*\n>  \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\n> @@ -60,40 +93,6 @@ void prepare_repo_settings(struct repository *r)\n>  \tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))\n>  \t\tr->settings.core_multi_pack_index = 1;\n>  \n> -\t/*\n> -\t * Non-boolean config\n> -\t */\n> -\tif (!repo_config_get_int(r, \"index.version\", &value))\n> -\t\tr->settings.index_version = value;\n> -\n> -\tif (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n> -\t\tint v = git_parse_maybe_bool(strval);\n> -\n> -\t\t/*\n> -\t\t * If it's set to \"keep\", or some other non-boolean\n> -\t\t * value then \"v < 0\". Then we do nothing and keep it\n> -\t\t * at the default of UNTRACKED_CACHE_KEEP.\n> -\t\t */\n> -\t\tif (v >= 0)\n> -\t\t\tr->settings.core_untracked_cache = v ?\n> -\t\t\t\tUNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;\n> -\t\tfree(strval);\n> -\t}\n> -\n> -\tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n> -\t\tint fetch_default = r->settings.fetch_negotiation_algorithm;\n> -\t\tif (!strcasecmp(strval, \"skipping\"))\n> -\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> -\t\telse if (!strcasecmp(strval, \"noop\"))\n> -\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n> -\t\telse if (!strcasecmp(strval, \"consecutive\"))\n> -\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n> -\t\telse if (!strcasecmp(strval, \"default\"))\n> -\t\t\tr->settings.fetch_negotiation_algorithm = fetch_default;\n> -\t\telse\n> -\t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n> -\t}\n> -\n>  \t/*\n>  \t * This setting guards all index reads to require a full index\n>  \t * over a sparse index. After suitable guards are placed in the\n> \n> base-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7\n\n"},{"id":"452063","messageId":"xmqqtuboqto5.fsf@gitster.g","threadId":"57605","inReplyTo":"1b27e0b115f858a422e0a2891688227be8f3db01.1648055915.git.steadmon@google.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-23T20:51:22Z","receivedAt":"2022-03-23T20:51:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> prepare_repo_settings() initializes a `struct repository` with various\n> default config options and settings read from a repository-local config\n> file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\n> in git repos), prepare_repo_settings was changed to issue a BUG() if it\n> is called by a process whose CWD is not a Git repository. This approach\n> was suggested in [1].\n>\n> This breaks fuzz-commit-graph, which attempts to parse arbitrary\n> fuzzing-engine-provided bytes as a commit graph file.\n> commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n> since we run the fuzz tests without a valid repository, we are hitting\n> the BUG() from 44c7e62 for every test case.\n\nI think the right approach for such a breakage is to ensure it is in\na repository, not to force prepare_repo_settings() to lie.  In the\nday-to-day real code paths the end user uses, we do want to catch\nmistakes in our code that calls prepare_repo_settings() when we are\nnot in a repository.\n\n"},{"id":"452065","messageId":"xmqqpmmcqtii.fsf@gitster.g","threadId":"57605","inReplyTo":"fcfdcbb9-761a-0d34-7d36-61e0ef279922@github.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-23T20:54:45Z","receivedAt":"2022-03-23T20:54:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye <vdye@github.com> writes:\n\n> I think the decision of whether to go with this approach or the alternative\n> listed below depends on the validity of a 'repository' without a gitdir. \n>\n> As far as I can tell, there is an implicit conflict between the changes in:\n>\n> 1. b66d84756f (commit-graph: respect 'commitGraph.readChangedPaths',\n>    2020-09-09)\n> 2. 44c7e62e51 (repo-settings: prepare_repo_settings only in git repos,\n>    2021-12-06) (as you pointed out in your message)\n>\n> The former says that commit-graph should use a repository setting (implying\n> it needs a valid repository), and the latter says that you need a valid\n> gitdir to get repository settings.\n>\n> So to me, how to proceed depends on whether a repository can be \"valid\"\n> without a gitdir or not:\n\nSorry, I do not get it.  What does \"a repository without a git dir\"\nlook like?  It does not make any sense to me.  A repository without\nworking tree, I can understand.\n"},{"id":"452068","messageId":"1e69a59f-aa71-fb04-971b-6cffbd000512@github.com","threadId":"57605","inReplyTo":"xmqqpmmcqtii.fsf@gitster.g","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-23T21:19:23Z","receivedAt":"2022-03-23T21:19:28Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> Victoria Dye <vdye@github.com> writes:\n> \n>> I think the decision of whether to go with this approach or the alternative\n>> listed below depends on the validity of a 'repository' without a gitdir. \n>>\n>> As far as I can tell, there is an implicit conflict between the changes in:\n>>\n>> 1. b66d84756f (commit-graph: respect 'commitGraph.readChangedPaths',\n>>    2020-09-09)\n>> 2. 44c7e62e51 (repo-settings: prepare_repo_settings only in git repos,\n>>    2021-12-06) (as you pointed out in your message)\n>>\n>> The former says that commit-graph should use a repository setting (implying\n>> it needs a valid repository), and the latter says that you need a valid\n>> gitdir to get repository settings.\n>>\n>> So to me, how to proceed depends on whether a repository can be \"valid\"\n>> without a gitdir or not:\n> \n> Sorry, I do not get it.  What does \"a repository without a git dir\"\n> look like?  It does not make any sense to me.  A repository without\n> working tree, I can understand.\n\nI think that answers my question - if it doesn't make sense to have a\n\"repository without a git dir\", then the code shouldn't allow\n'the_repository' to be used without a valid 'the_repository.gitdir'. The\n'BUG()' in 'prepare_repo_settings()' enforces that condition right now, so\nremoving it like this RFC does would make the tests fragile and the code\nmore prone to future bugs.\n\nThat said, if it's conceptually sensible for 'fuzz-commit-graph' to work\nwithout a repository, then it could be updated to get its defaults from\nsomewhere other than 'the_repository' when the repo doesn't exist (like what\nI mentioned in [1] (\"If the 'repository' really is...\"), or Taylor's\nsuggestion in [2]).\n\n[1] https://lore.kernel.org/git/fcfdcbb9-761a-0d34-7d36-61e0ef279922@github.com/\n[2] https://lore.kernel.org/git/Yjt6mLIfw0V3aVTO@nand.local/\n"},{"id":"452499","messageId":"YkIJacCaaqFk1MDa@google.com","threadId":"57605","inReplyTo":"Yjt6mLIfw0V3aVTO@nand.local","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-03-28T19:15:53Z","receivedAt":"2022-03-28T19:16:03Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.03.23 15:52, Taylor Blau wrote:\n> On Wed, Mar 23, 2022 at 03:22:13PM -0400, Derrick Stolee wrote:\n> > On 3/23/2022 2:03 PM, Josh Steadmon wrote:\n> > > prepare_repo_settings() initializes a `struct repository` with various\n> > > default config options and settings read from a repository-local config\n> > > file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\n> > > in git repos), prepare_repo_settings was changed to issue a BUG() if it\n> > > is called by a process whose CWD is not a Git repository. This approach\n> > > was suggested in [1].\n> > >\n> > > This breaks fuzz-commit-graph, which attempts to parse arbitrary\n> > > fuzzing-engine-provided bytes as a commit graph file.\n> > > commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n> > > since we run the fuzz tests without a valid repository, we are hitting\n> > > the BUG() from 44c7e62 for every test case.\n> > >\n> > > Fix this by refactoring prepare_repo_settings() such that it sets\n> > > default options unconditionally; if its process is in a Git repository,\n> > > it will also load settings from the local config. This eliminates the\n> > > need for a BUG() when not in a repository.\n> >\n> > I think you have the right idea and this can work.\n> \n> Hmmm. To me this feels like bending over backwards in\n> `prepare_repo_settings()` to accommodate one particular caller. I'm not\n> necessarily opposed to it, but it does feel strange to make\n> `prepare_repo_settings()` a noop here, since I would expect that any\n> callers who do want to call `prepare_repo_settings()` are likely\n> convinced that they are inside of a repository, and it probably should\n> be a BUG() if they aren't.\n> \n> I was initially thinking that Josh's alternative of introducing and\n> calling a lower-level version of `prepare_commit_graph()` that doesn't\n> require the use of any repository settings would make sense.\n> \n> But when I started looking at implementing it, I became confused at how\n> this is supposed to work at all without using a repository. We depend on\n> the values parsed from:\n> \n>   - commitGraph.generationVersion\n>   - commitGraph.readChangedPaths\n> \n> to determine which part(s) of the commit graph we do and don't read.\n> \n> Looking around, I think I probably inadvertently broke this in\n> ab14d0676c (commit-graph: pass a 'struct repository *' in more places,\n> 2020-09-09). But prior to ab14d0676c, neither of those settings existed,\n> so parsing the commit graph was a pure function of the commit graph's\n> contents alone, and didn't rely on the existence of a repository.\n\nYeah, I have not done a great job keeping the fuzzers up to date with\ncommit-graph changes :(.\n\n> We could pretend as if `commitGraph.generationVersion` is always \"2\" and\n> `commitGraph.readChangedPaths` is always \"true\", and I think that would\n> still give us good-enough coverage.\n\nIt might also be worthwhile for the fuzzer to test each interesting\ncombination of settings, using the same arbitrary input.\n\n> I assume that libFuzzer doesn't give us a convenient way to stub out a\n> repository. Maybe we should have a variant of `parse_commit_graph` that\n> instead takes a `struct repo_settings` filled out with the defaults?\n> \n> We could use that to teach libFuzzer about additional states that the\n> commit graph parser can be in, too (though probably outside the scope of\n> this patch).\n> \n> I tried to sketch all of this out, which seems to work. Let me know what\n> you think:\n\nYes, your patch looks like a much smaller change than I feared would be\nthe case for a parse_commit_graph refactor. I'll test it out, and\nprobably add some additional fuzzer improvements on top. Thanks for the\npatch!\n"},{"id":"452503","messageId":"YkISPUe4NKvzwrjj@google.com","threadId":"57605","inReplyTo":"Yjt6mLIfw0V3aVTO@nand.local","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-03-28T19:53:33Z","receivedAt":"2022-03-28T19:54:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.03.23 15:52, Taylor Blau wrote:\n> I tried to sketch all of this out, which seems to work. Let me know what\n> you think:\n\nBTW, is it OK if I include your Signed-off-by on this when I send my V2?\n"},{"id":"452541","messageId":"YkJfNtcXiKWQOtQA@nand.local","threadId":"57605","inReplyTo":"YkIJacCaaqFk1MDa@google.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-29T01:21:58Z","receivedAt":"2022-03-29T01:22:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 28, 2022 at 12:15:53PM -0700, Josh Steadmon wrote:\n> > Looking around, I think I probably inadvertently broke this in\n> > ab14d0676c (commit-graph: pass a 'struct repository *' in more places,\n> > 2020-09-09). But prior to ab14d0676c, neither of those settings existed,\n> > so parsing the commit graph was a pure function of the commit graph's\n> > contents alone, and didn't rely on the existence of a repository.\n>\n> Yeah, I have not done a great job keeping the fuzzers up to date with\n> commit-graph changes :(.\n\nI think that puts you and I in the same boat, since the original\nbreakage from ab14d0676c blames back to me. I'm sorry that I didn't\nnotice that my change had broken the fuzzing code at the time, and I\nappreciate you working on fixing it!\n\n> > We could pretend as if `commitGraph.generationVersion` is always \"2\" and\n> > `commitGraph.readChangedPaths` is always \"true\", and I think that would\n> > still give us good-enough coverage.\n>\n> It might also be worthwhile for the fuzzer to test each interesting\n> combination of settings, using the same arbitrary input.\n\nDefinitely. I don't think it hurts to just focus on getting the common\ncase (\"2\", \"true\") working again. And if libFuzzer makes it easy-ish to\ntest more of the possible input space, I'm all for it.\n\nThanks,\nTaylor\n"},{"id":"452542","messageId":"YkJfVnVSaO0IRJe9@nand.local","threadId":"57605","inReplyTo":"YkISPUe4NKvzwrjj@google.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-29T01:22:30Z","receivedAt":"2022-03-29T01:22:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 28, 2022 at 12:53:33PM -0700, Josh Steadmon wrote:\n> On 2022.03.23 15:52, Taylor Blau wrote:\n> > I tried to sketch all of this out, which seems to work. Let me know what\n> > you think:\n>\n> BTW, is it OK if I include your Signed-off-by on this when I send my V2?\n\nYes, absolutely. Thanks for asking; it's fine to include my\nSigned-off-by on any patches / diffs that I send to the mailing list.\n\nThanks,\nTaylor\n"},{"id":"452548","messageId":"220329.86lewt2kqp.gmgdl@evledraar.gmail.com","threadId":"57605","inReplyTo":"Yjt6mLIfw0V3aVTO@nand.local","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-29T09:03:14Z","receivedAt":"2022-03-29T09:04:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 23 2022, Taylor Blau wrote:\n\n>  \t/* Boolean config or default, does not cascade (simple)  */\n>  \trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n> +\trepo_cfg_int(r, \"commitgraph.generationversion\", &r->settings.commit_graph_generation_version, 2);\n\nIf this ends up being kept let's add it to its own commented \"section\",\nthe comment 2-lines above it is now incorrect.\n"},{"id":"452549","messageId":"220329.86h77h2ju3.gmgdl@evledraar.gmail.com","threadId":"57605","inReplyTo":"Yjt6mLIfw0V3aVTO@nand.local","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-29T09:04:18Z","receivedAt":"2022-03-29T09:23:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 23 2022, Taylor Blau wrote:\n\n> On Wed, Mar 23, 2022 at 03:22:13PM -0400, Derrick Stolee wrote:\n>> On 3/23/2022 2:03 PM, Josh Steadmon wrote:\n>> > prepare_repo_settings() initializes a `struct repository` with various\n>> > default config options and settings read from a repository-local config\n>> > file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\n>> > in git repos), prepare_repo_settings was changed to issue a BUG() if it\n>> > is called by a process whose CWD is not a Git repository. This approach\n>> > was suggested in [1].\n>> >\n>> > This breaks fuzz-commit-graph, which attempts to parse arbitrary\n>> > fuzzing-engine-provided bytes as a commit graph file.\n>> > commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n>> > since we run the fuzz tests without a valid repository, we are hitting\n>> > the BUG() from 44c7e62 for every test case.\n>> >\n>> > Fix this by refactoring prepare_repo_settings() such that it sets\n>> > default options unconditionally; if its process is in a Git repository,\n>> > it will also load settings from the local config. This eliminates the\n>> > need for a BUG() when not in a repository.\n>>\n>> I think you have the right idea and this can work.\n>\n> Hmmm. To me this feels like bending over backwards in\n> `prepare_repo_settings()` to accommodate one particular caller. I'm not\n> necessarily opposed to it, but it does feel strange to make\n> `prepare_repo_settings()` a noop here, since I would expect that any\n> callers who do want to call `prepare_repo_settings()` are likely\n> convinced that they are inside of a repository, and it probably should\n> be a BUG() if they aren't.\n\nI think adding that BUG() was overzelous in the first place, per\nhttps://lore.kernel.org/git/211207.86r1apow9f.gmgdl@evledraar.gmail.com/;\n\nI don't see what purpose it solves to be this overly anal in this code,\nand 44c7e62e51e (repo-settings: prepare_repo_settings only in git repos,\n2021-12-06) just discusses \"what\" and not \"why\".\n\nI think a perfectly fine solution to this is just to revert it:\n\t\n\tdiff --git a/repo-settings.c b/repo-settings.c\n\tindex b4fbd16cdcc..e162c1479bf 100644\n\t--- a/repo-settings.c\n\t+++ b/repo-settings.c\n\t@@ -18,7 +18,7 @@ void prepare_repo_settings(struct repository *r)\n\t \tint manyfiles;\n\t \n\t \tif (!r->gitdir)\n\t-\t\tBUG(\"Cannot add settings for uninitialized repository\");\n\t+\t\treturn;\n\t \n\t \tif (r->settings.initialized++)\n\t \t\treturn;\n\nI have that in my local integration branch, because I ended up wanting\nto add prepare_repo_settings() to usage.c, which may or may not run\ninside a repo (and maybe we'll have that config, maybe not).\n\nBut really, in common-main.c we do a initialize_the_repository(), so a\n\"struct repository\" is already a thing we have before we get to the\n\"RUN_SETUP_GENTLY\" or whatever in git.c, and a bunch of things all over\nthe place assume that it's the equivalent of { 0 }-initialized.\n\nIf we actually want to turn repository.[ch] into some strict API where\n\"Tho Shalt Not Use the_repository unless\" we're actually in a repo\nsurely we should have it be NULL then, and to add that BUG() to the\nlikes of initialize_the_repository().\n\nExcept I think there's no point in that, and it would just lead to\npointless churn, so why do it for the settings in particular? Why can't\nthey just be { 0 }-init'd too?\n\nIf some caller cares about the distinction between r->settings being\nlike it is because of us actually having a repo, or us using the\ndefaults why can't they just check r->gitdir themselves?\n\nFor the rest the default of \"just provide the defaults then\" is a much\nsaner API.\n\nI think *maybe* what this actually wanted to do was to bridge the gap\nbetween \"startup_info->have_repository\" and a caller in builtin/ calling\nprepare_repo_settings(), i.e. that it was a logic error to have that\nRUN_SETUP_GENTLY caller do that.\n\nI can see how that *might* be useful as some sanity assertion, but then\nmaybe we could add a more narrow BUG() just for that case, even having a\nbuiltin_prepare_repo_settings() wrapper in builtin.h or whatever.\n"},{"id":"452634","messageId":"YkO/5/TVQvqyVQly@nand.local","threadId":"57605","inReplyTo":"220329.86lewt2kqp.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-30T02:26:47Z","receivedAt":"2022-03-30T02:26:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 29, 2022 at 11:03:14AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Wed, Mar 23 2022, Taylor Blau wrote:\n>\n> >  \t/* Boolean config or default, does not cascade (simple)  */\n> >  \trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n> > +\trepo_cfg_int(r, \"commitgraph.generationversion\", &r->settings.commit_graph_generation_version, 2);\n>\n> If this ends up being kept let's add it to its own commented \"section\",\n> the comment 2-lines above it is now incorrect.\n\nThanks for pointing it out; indeed that comment is definitely stale with\nrespect to the newer code. This patch was just a sketch of an\nalternative approach for Josh to consider, so I can't guarantee that it\nisn't immune to nit-picks ;).\n\nI think that Josh is planning on picking this up, so hopefully he\ndoesn't mind applying these and any other touch-ups locally before\nresubmitting.\n\n(Josh, if you do: thank you very much in advance!)\n\nThanks,\nTaylor\n"},{"id":"452635","messageId":"YkPBnIt6K0crowpb@nand.local","threadId":"57605","inReplyTo":"220329.86h77h2ju3.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-30T02:34:04Z","receivedAt":"2022-03-30T02:34:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 29, 2022 at 11:04:18AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Wed, Mar 23 2022, Taylor Blau wrote:\n>\n> > On Wed, Mar 23, 2022 at 03:22:13PM -0400, Derrick Stolee wrote:\n> >> On 3/23/2022 2:03 PM, Josh Steadmon wrote:\n> >> > prepare_repo_settings() initializes a `struct repository` with various\n> >> > default config options and settings read from a repository-local config\n> >> > file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\n> >> > in git repos), prepare_repo_settings was changed to issue a BUG() if it\n> >> > is called by a process whose CWD is not a Git repository. This approach\n> >> > was suggested in [1].\n> >> >\n> >> > This breaks fuzz-commit-graph, which attempts to parse arbitrary\n> >> > fuzzing-engine-provided bytes as a commit graph file.\n> >> > commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n> >> > since we run the fuzz tests without a valid repository, we are hitting\n> >> > the BUG() from 44c7e62 for every test case.\n> >> >\n> >> > Fix this by refactoring prepare_repo_settings() such that it sets\n> >> > default options unconditionally; if its process is in a Git repository,\n> >> > it will also load settings from the local config. This eliminates the\n> >> > need for a BUG() when not in a repository.\n> >>\n> >> I think you have the right idea and this can work.\n> >\n> > Hmmm. To me this feels like bending over backwards in\n> > `prepare_repo_settings()` to accommodate one particular caller. I'm not\n> > necessarily opposed to it, but it does feel strange to make\n> > `prepare_repo_settings()` a noop here, since I would expect that any\n> > callers who do want to call `prepare_repo_settings()` are likely\n> > convinced that they are inside of a repository, and it probably should\n> > be a BUG() if they aren't.\n>\n> I think adding that BUG() was overzelous in the first place, per\n> https://lore.kernel.org/git/211207.86r1apow9f.gmgdl@evledraar.gmail.com/;\n\nI think Junio raised a good point in\n\n    https://lore.kernel.org/git/xmqqcznh8913.fsf@gitster.g/\n\n, though some of the detail was lost in 44c7e62e51 (repo-settings:\nprepare_repo_settings only in git repos, 2021-12-06).\n\n> I have that in my local integration branch, because I ended up wanting\n> to add prepare_repo_settings() to usage.c, which may or may not run\n> inside a repo (and maybe we'll have that config, maybe not).\n\nI see what you're saying, though I think we would be equally OK to have\na default value of the repo_settings struct that we could rely on. I\nsaid some of this back in\n\n    https://lore.kernel.org/git/Yjt6mLIfw0V3aVTO@nand.local/\n\n, namely the parts around \"I would expect that any callers who do want\nto call `prepare_repo_settings()` are likely convinced that they are\ninside of a repository, and it probably should be a BUG() if they\naren't.\"\n\nThinking in terms of your message, though, I think the distinction (from\nmy perspective, at least) is between (a) using something called\n_repo_-settings in a non-repo context, and (b) calling a function which\nis supposed to fill in its values from a repository (which the caller\nimplicitly expects to exist).\n\nNeither feel _good_ to me, but (b) feels worse, since it is making it OK\nto operate in a likely-unexpected context with respect to the caller's\nexpectations.\n\nAnyway, I think that we are pretty far into the weeds, and it's likely\ntime to turn around. I don't have that strong a feeling either way, and\nin all honesty either approach is probably just fine.\n\nThanks,\nTaylor\n"},{"id":"452681","messageId":"220330.86ilrvnxb6.gmgdl@evledraar.gmail.com","threadId":"57605","inReplyTo":"YkPBnIt6K0crowpb@nand.local","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-30T17:38:29Z","receivedAt":"2022-03-30T17:51:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 29 2022, Taylor Blau wrote:\n\n> On Tue, Mar 29, 2022 at 11:04:18AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Wed, Mar 23 2022, Taylor Blau wrote:\n>>\n>> > On Wed, Mar 23, 2022 at 03:22:13PM -0400, Derrick Stolee wrote:\n>> >> On 3/23/2022 2:03 PM, Josh Steadmon wrote:\n>> >> > prepare_repo_settings() initializes a `struct repository` with various\n>> >> > default config options and settings read from a repository-local config\n>> >> > file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only\n>> >> > in git repos), prepare_repo_settings was changed to issue a BUG() if it\n>> >> > is called by a process whose CWD is not a Git repository. This approach\n>> >> > was suggested in [1].\n>> >> >\n>> >> > This breaks fuzz-commit-graph, which attempts to parse arbitrary\n>> >> > fuzzing-engine-provided bytes as a commit graph file.\n>> >> > commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n>> >> > since we run the fuzz tests without a valid repository, we are hitting\n>> >> > the BUG() from 44c7e62 for every test case.\n>> >> >\n>> >> > Fix this by refactoring prepare_repo_settings() such that it sets\n>> >> > default options unconditionally; if its process is in a Git repository,\n>> >> > it will also load settings from the local config. This eliminates the\n>> >> > need for a BUG() when not in a repository.\n>> >>\n>> >> I think you have the right idea and this can work.\n>> >\n>> > Hmmm. To me this feels like bending over backwards in\n>> > `prepare_repo_settings()` to accommodate one particular caller. I'm not\n>> > necessarily opposed to it, but it does feel strange to make\n>> > `prepare_repo_settings()` a noop here, since I would expect that any\n>> > callers who do want to call `prepare_repo_settings()` are likely\n>> > convinced that they are inside of a repository, and it probably should\n>> > be a BUG() if they aren't.\n>>\n>> I think adding that BUG() was overzelous in the first place, per\n>> https://lore.kernel.org/git/211207.86r1apow9f.gmgdl@evledraar.gmail.com/;\n>\n> I think Junio raised a good point in\n>\n>     https://lore.kernel.org/git/xmqqcznh8913.fsf@gitster.g/\n>\n> , though some of the detail was lost in 44c7e62e51 (repo-settings:\n> prepare_repo_settings only in git repos, 2021-12-06).\n>\n>> I have that in my local integration branch, because I ended up wanting\n>> to add prepare_repo_settings() to usage.c, which may or may not run\n>> inside a repo (and maybe we'll have that config, maybe not).\n>\n> I see what you're saying, though I think we would be equally OK to have\n> a default value of the repo_settings struct that we could rely on. I\n> said some of this back in\n>\n>     https://lore.kernel.org/git/Yjt6mLIfw0V3aVTO@nand.local/\n>\n> , namely the parts around \"I would expect that any callers who do want\n> to call `prepare_repo_settings()` are likely convinced that they are\n> inside of a repository, and it probably should be a BUG() if they\n> aren't.\"\n>\n> Thinking in terms of your message, though, I think the distinction (from\n> my perspective, at least) is between (a) using something called\n> _repo_-settings in a non-repo context, and (b) calling a function which\n> is supposed to fill in its values from a repository (which the caller\n> implicitly expects to exist).\n>\n> Neither feel _good_ to me, but (b) feels worse, since it is making it OK\n> to operate in a likely-unexpected context with respect to the caller's\n> expectations.\n\nI agree that it's a bit iffy. I'm basically advocating for treating\n\"the_repository->settings\" as though it's a new \"the_config\" or\nwhatever.\n\nMaybe we'd be better off just making that move, or having\nthe_repository->settings contain only settings relevant to cases where\nwe only have a repository.\n\nBut I think predicating useful uses of it on that refactoring is\noverdoing it a bit, especially as I think your \"(b)\" concern here is\nalready something we deal with when it comes to\ninitialize_the_repository() and checks for\n\"the_repository->gitdir\".\n\nCan't we just have callers that really care about the distinction check\n\"->gitdir\" instead? As they're already doing in some cases already?\n\nOr just:\n\n    git mv {repo,global}-settings.c\n\nSince that's what it seems to want to be anyway.\n\n> Anyway, I think that we are pretty far into the weeds, and it's likely\n> time to turn around. I don't have that strong a feeling either way, and\n> in all honesty either approach is probably just fine.\n\n*nod*\n"},{"id":"452700","messageId":"xmqqwngb6vum.fsf@gitster.g","threadId":"57605","inReplyTo":"220330.86ilrvnxb6.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-30T20:14:57Z","receivedAt":"2022-03-30T20:15:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Or just:\n>\n>     git mv {repo,global}-settings.c\n>\n> Since that's what it seems to want to be anyway.\n\nHmph, care to elaborate a bit more on \"seems to\"?\n\nHere is my take\n\n - The code makes extensive use of repo_cfg_bool(), which is a thin\n   wrapper around repo_config_get_bool(); despite its name, it is\n   not about reading from the configuration file of that repository\n   and nowhere else.  It can be affected by global configuration.\n\n - Other uses of repo_config_get_*() it uses is the same way.\n\nSo, it wants to grab a set of configuration that would +apply+ to\nthis specific instance of \"struct repository\".\n\nBut that is quite different from \"give us settings that would apply\nin general, I do not have a specific repository in mind\", which is\nwhat \"global-settings.c\" would imply at least to me.\n\nAnd in order for the \"this specific instance\" to make sense, the\ncaller should have made sure that it is indeed a repository.\nLifting that BUG() from the code path not only smells sloppy way to\nwork around some corner case code that does not prepare the\nrepository properly, but does not make much sense, at least to me.\nIn exchange for scrapping the safety to help a caller that forgets\nto prepare repository before it is ready to call this function, what\nare we gaining?\n\nI went back to the thread-starter message and re-read its\njustification.  It talks about:\n\n> Concerns:\n>\n> Are any callers strictly dependent on having a BUG() here? I suspect\n> that the worst that would happen is that rather than this BUG(), the\n> caller would later hit its own BUG() or die(), so I do not think this is\n> a blocker. Additionally, every builtin that directly calls\n> prepare_repo_settings is either marked as RUN_SETUP, which means we\n> would die() prior to calling it anyway, or checks on its own before\n> calling it (builtin/diff.c). There are several callers in library code,\n> though, and I have not tracked down how all of those are used.\n\nAsking for existing callers being dependent on having a BUG() is a\npure nonsense.  The existing callers are there in shipped versions\nof Git exactly because they do things correctly not to hit the BUG(),\nso BY DEFINITION, they do not care if the BUG() is there or not.\n\nSo that is not \"a blocker\", but is a non-argument to ask if existing\ncode paths care if the BUG() is gone.\n\nWhat BUG() is protecting us against is a careless developer who\nwrites a new code or alters an existing code path that ends up\nmaking the control flow in such a way that a proper set-up of the\nrepository structure is bypassed by mistake before calling this\nfunction.  The function is call-once by r->settings.initialized\nguarding it, calling it and then doing a set-up will result in an\nunexplainable bug even if the caller tries to compensate by calling\nit twice, as r->settings that is set incorrectly will be sticky.\n\nHaving said all that, I can be pursuaded to consider an approach to\nallow callers to explicitly ask for running outside repository, just\nlike the more strict setup_git_directory() for majority of callers\nhas looser setup_git_directory_gently() counterpart.  The current\ncallers should retain the \"you must have discovered gitdir\" there,\nbut a special purpose code that is not even Git (like fuzzer) can\nsay\n\n    prepare_repo_settings_gently(r, &nongit_ok);\n\ninstead.\n\ndiff --git c/repo-settings.c w/repo-settings.c\nindex b4fbd16cdc..c492bc7671 100644\n--- c/repo-settings.c\n+++ w/repo-settings.c\n@@ -10,15 +10,24 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n \t\t*dest = def;\n }\n \n-void prepare_repo_settings(struct repository *r)\n+void prepare_repo_settings_gently(struct repository *r, int *nongit)\n {\n \tint experimental;\n \tint value;\n \tchar *strval;\n \tint manyfiles;\n \n-\tif (!r->gitdir)\n-\t\tBUG(\"Cannot add settings for uninitialized repository\");\n+\tif (!r->gitdir) {\n+\t\t/*\n+\t\t * The caller can pass nongit (out paremeter) to ask if r is already\n+\t\t * initialized (and act on it after this function returns).\n+\t\t */\n+\t\tif (!nongit)\n+\t\t\tBUG(\"Cannot add settings for uninitialized repository\");\n+\t\t*nongit = 1;\n+\t} else if (nongit) {\n+\t\t*nongit = 0;\n+\t}\n \n \tif (r->settings.initialized++)\n \t\treturn;\ndiff --git c/repository.h w/repository.h\nindex e29f361703..98f6ec12cc 100644\n--- c/repository.h\n+++ w/repository.h\n@@ -222,7 +222,8 @@ int repo_read_index_unmerged(struct repository *);\n  */\n void repo_update_index_if_able(struct repository *, struct lock_file *);\n \n-void prepare_repo_settings(struct repository *r);\n+#define prepare_repo_settings(r) prepare_repo_settings_gently((r), NULL)\n+void prepare_repo_settings_gently(struct repository *r, int *nongit);\n \n /*\n  * Return 1 if upgrade repository format to target_version succeeded,\n\n\n"},{"id":"453364","messageId":"YlEotqSn/3Ie6FzD@google.com","threadId":"57605","inReplyTo":"YkO/5/TVQvqyVQly@nand.local","subject":"Re: [RFC PATCH] repo-settings: set defaults even when not in a repo","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-04-09T06:33:26Z","receivedAt":"2022-04-09T06:33:38Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.03.29 22:26, Taylor Blau wrote:\n> On Tue, Mar 29, 2022 at 11:03:14AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >\n> > On Wed, Mar 23 2022, Taylor Blau wrote:\n> >\n> > >  \t/* Boolean config or default, does not cascade (simple)  */\n> > >  \trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n> > > +\trepo_cfg_int(r, \"commitgraph.generationversion\", &r->settings.commit_graph_generation_version, 2);\n> >\n> > If this ends up being kept let's add it to its own commented \"section\",\n> > the comment 2-lines above it is now incorrect.\n> \n> Thanks for pointing it out; indeed that comment is definitely stale with\n> respect to the newer code. This patch was just a sketch of an\n> alternative approach for Josh to consider, so I can't guarantee that it\n> isn't immune to nit-picks ;).\n> \n> I think that Josh is planning on picking this up, so hopefully he\n> doesn't mind applying these and any other touch-ups locally before\n> resubmitting.\n> \n> (Josh, if you do: thank you very much in advance!)\n> \n> Thanks,\n> Taylor\n\nDone in V2, to be sent out shortly.\n"},{"id":"453365","messageId":"2c2bfc7b43c92dc7c0a4c47a2c88dadeb39c763b.1649486398.git.steadmon@google.com","threadId":"57605","inReplyTo":"Yjt6mLIfw0V3aVTO@nand.local","subject":"[RFC PATCH v2] commit-graph: refactor to avoid prepare_repo_settings","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-04-09T06:52:10Z","receivedAt":"2022-04-09T06:52:18Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Taylor Blau <me@ttaylorr.com>\n\nPrior to ab14d0676c (commit-graph: pass a 'struct repository *' in more\nplaces, 2020-09-09), parsing a commit-graph was a pure function\ndepending only on the contents of the commit-graph itself. Commit\nab14d0676c introduced a dependency on a `struct repository` pointer, and\nlater commits such as b66d84756f (commit-graph: respect\n'commitGraph.readChangedPaths', 2020-09-09) added dependencies on config\nsettings, which were accessed through the `settings` field of the\nrepository pointer. This field was initialized via a call to\n`prepare_repo_settings()`.\n\nIn 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only in git\nrepos), prepare_repo_settings was changed to issue a BUG() if it is\ncalled by a process whose CWD is not a Git repository.\n\nThis series of changes broke fuzz-commit-graph, which attempts to parse\narbitrary fuzzing-engine-provided bytes as a commit graph file.\ncommit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\nsince we run the fuzz tests without a valid repository, we are hitting\nthe BUG() from 44c7e62 for every test case.\n\nFix this by moving the majority of the implementaiton of\n`parse_commit_graph()` into a new function,\n`parse_commit_graph_settings()` that accepts a repo_settings pointer.\nThis allows fuzz-commit-graph to continue to test the commit-graph\nparser implementation without relying on prepare_repo_settings().\n\nAdditionally, properly initialize the\nrepo_settings.commit_graph_generation_version field in\nprepare_repo_settings(). Load the value from the config if present, and\ndefault to version 2 otherwise.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nI've taken the diff from Taylor's message Yjt6mLIfw0V3aVTO@nand.local\nwith a small tweak to the fuzzer: I didn't see that the commit graph\nsettings were being initialized anywhere outside of\nprepare_repo_settings(), so I manually initialized them in\nfuzz-commit-graph. I've also moved the commit-graph settings in\nprepare_repo_settings() to their own section, as suggested by Ævar.\n\nI've tried to combine Taylor's explanation from his email with the\ncommit message from my original patch. Taylor, if you feel like anything\nneeds to be changed please let me know or feel free to resend with your\nchanges. Thanks again for providing this fix!\n\n commit-graph.c      | 22 ++++++++++------------\n commit-graph.h      |  2 ++\n fuzz-commit-graph.c |  8 +++++++-\n repo-settings.c     | 12 +++++++++++-\n repository.h        |  1 +\n 5 files changed, 31 insertions(+), 14 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 265c010122..c54a734619 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -96,13 +96,6 @@ define_commit_slab(commit_graph_data_slab, struct commit_graph_data);\n static struct commit_graph_data_slab commit_graph_data_slab =\n \tCOMMIT_SLAB_INIT(1, commit_graph_data_slab);\n \n-static int get_configured_generation_version(struct repository *r)\n-{\n-\tint version = 2;\n-\trepo_config_get_int(r, \"commitgraph.generationversion\", &version);\n-\treturn version;\n-}\n-\n uint32_t commit_graph_position(const struct commit *c)\n {\n \tstruct commit_graph_data *data =\n@@ -335,6 +328,13 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,\n \n struct commit_graph *parse_commit_graph(struct repository *r,\n \t\t\t\t\tvoid *graph_map, size_t graph_size)\n+{\n+\tprepare_repo_settings(r);\n+\treturn parse_commit_graph_settings(&r->settings, graph_map, graph_size);\n+}\n+\n+struct commit_graph *parse_commit_graph_settings(struct repo_settings *s,\n+\t\t\t\t\t\t void *graph_map, size_t graph_size)\n {\n \tconst unsigned char *data;\n \tstruct commit_graph *graph;\n@@ -371,8 +371,6 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \t\treturn NULL;\n \t}\n \n-\tprepare_repo_settings(r);\n-\n \tgraph = alloc_commit_graph();\n \n \tgraph->hash_len = the_hash_algo->rawsz;\n@@ -402,14 +400,14 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\n \tpair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n \n-\tif (get_configured_generation_version(r) >= 2) {\n+\tif (s->commit_graph_generation_version >= 2) {\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n \t\t\t&graph->chunk_generation_data);\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n \t\t\t&graph->chunk_generation_data_overflow);\n \t}\n \n-\tif (r->settings.commit_graph_read_changed_paths) {\n+\tif (s->commit_graph_read_changed_paths) {\n \t\tpair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n \t\t\t   &graph->chunk_bloom_indexes);\n \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n@@ -2288,7 +2286,7 @@ int write_commit_graph(struct object_directory *odb,\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n \tctx->opts = opts;\n \tctx->total_bloom_filter_data_size = 0;\n-\tctx->write_generation_data = (get_configured_generation_version(r) == 2);\n+\tctx->write_generation_data = (r->settings.commit_graph_generation_version == 2);\n \tctx->num_generation_data_overflows = 0;\n \n \tbloom_settings.bits_per_entry = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY\",\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 04a94e1830..0f0d28b129 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -95,6 +95,8 @@ struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n struct commit_graph *parse_commit_graph(struct repository *r,\n \t\t\t\t\tvoid *graph_map, size_t graph_size);\n+struct commit_graph *parse_commit_graph_settings(struct repo_settings *s,\n+\t\t\t\t\tvoid *graph_map, size_t graph_size);\n \n /*\n  * Return 1 if and only if the repository has a commit-graph\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nindex e7cf6d5b0f..e53a2635f6 100644\n--- a/fuzz-commit-graph.c\n+++ b/fuzz-commit-graph.c\n@@ -11,7 +11,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tstruct commit_graph *g;\n \n \tinitialize_the_repository();\n-\tg = parse_commit_graph(the_repository, (void *)data, size);\n+\t/*\n+\t * Manually initialize commit-graph settings, to avoid the need to run\n+\t * in an actual repository.\n+\t */\n+\tthe_repository->settings.commit_graph_generation_version = 2;\n+\tthe_repository->settings.commit_graph_read_changed_paths = 1;\n+\tg = parse_commit_graph_settings(&the_repository->settings, (void *)data, size);\n \trepo_clear(the_repository);\n \tfree_commit_graph(g);\n \ndiff --git a/repo-settings.c b/repo-settings.c\nindex b4fbd16cdc..26241c1c2c 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -10,6 +10,13 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n \t\t*dest = def;\n }\n \n+static void repo_cfg_int(struct repository *r, const char *key, int *dest,\n+\t\t\t int def)\n+{\n+\tif (repo_config_get_int(r, key, dest))\n+\t\t*dest = def;\n+}\n+\n void prepare_repo_settings(struct repository *r)\n {\n \tint experimental;\n@@ -41,11 +48,14 @@ void prepare_repo_settings(struct repository *r)\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n \t}\n \n-\t/* Boolean config or default, does not cascade (simple)  */\n+\t/* Commit graph config or default, does not cascade (simple) */\n \trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n+\trepo_cfg_int(r, \"commitgraph.generationversion\", &r->settings.commit_graph_generation_version, 2);\n \trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n \trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n \trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\n+\n+\t/* Boolean config or default, does not cascade (simple)  */\n \trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\ndiff --git a/repository.h b/repository.h\nindex ca837cb9e9..4f8275f97c 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -29,6 +29,7 @@ struct repo_settings {\n \tint initialized;\n \n \tint core_commit_graph;\n+\tint commit_graph_generation_version;\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n \tint fetch_write_commit_graph;\n\nbase-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7\n-- \n2.35.1.1178.g4f1659d476-goog\n\n"},{"id":"456818","messageId":"20220607200231.1958109-1-jonathantanmy@google.com","threadId":"57605","inReplyTo":"2c2bfc7b43c92dc7c0a4c47a2c88dadeb39c763b.1649486398.git.steadmon@google.com","subject":"Re: [RFC PATCH v2] commit-graph: refactor to avoid prepare_repo_settings","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-07T20:02:31Z","receivedAt":"2022-06-08T00:28:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Josh Steadmon <steadmon@google.com> writes:\n> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> index e7cf6d5b0f..e53a2635f6 100644\n> --- a/fuzz-commit-graph.c\n> +++ b/fuzz-commit-graph.c\n> @@ -11,7 +11,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n>  \tstruct commit_graph *g;\n>  \n>  \tinitialize_the_repository();\n> -\tg = parse_commit_graph(the_repository, (void *)data, size);\n> +\t/*\n> +\t * Manually initialize commit-graph settings, to avoid the need to run\n> +\t * in an actual repository.\n> +\t */\n> +\tthe_repository->settings.commit_graph_generation_version = 2;\n> +\tthe_repository->settings.commit_graph_read_changed_paths = 1;\n> +\tg = parse_commit_graph_settings(&the_repository->settings, (void *)data, size);\n>  \trepo_clear(the_repository);\n>  \tfree_commit_graph(g);\n\nThe comment doesn't explain why we need to avoid an actual repository. Maybe\nbetter: Initialize the commit-graph settings that would normally be read from\nthe repository's gitdir.\n\nOther than that, this patch looks good to me. Isolating a part of the\ncommit graph mechanism that can be fuzzed without access to the disk is,\nI think, a good idea, and this patch is a good way to do it.\n"},{"id":"457226","messageId":"9b56496b0809cc8a25af877ea97042e2cb7f2af6.1655246092.git.steadmon@google.com","threadId":"57605","inReplyTo":"Yjt6mLIfw0V3aVTO@nand.local","subject":"[PATCH v3] commit-graph: refactor to avoid prepare_repo_settings","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-06-14T22:37:21Z","receivedAt":"2022-06-14T22:37:31Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Taylor Blau <me@ttaylorr.com>\n\nPrior to ab14d0676c (commit-graph: pass a 'struct repository *' in more\nplaces, 2020-09-09), parsing a commit-graph was a pure function\ndepending only on the contents of the commit-graph itself. Commit\nab14d0676c introduced a dependency on a `struct repository` pointer, and\nlater commits such as b66d84756f (commit-graph: respect\n'commitGraph.readChangedPaths', 2020-09-09) added dependencies on config\nsettings, which were accessed through the `settings` field of the\nrepository pointer. This field was initialized via a call to\n`prepare_repo_settings()`.\n\nIn 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only in git\nrepos), prepare_repo_settings was changed to issue a BUG() if it is\ncalled by a process whose CWD is not a Git repository.\n\nThis series of changes broke fuzz-commit-graph, which attempts to parse\narbitrary fuzzing-engine-provided bytes as a commit graph file.\ncommit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\nsince we run the fuzz tests without a valid repository, we are hitting\nthe BUG() from 44c7e62 for every test case.\n\nFix this by moving the majority of the implementaiton of\n`parse_commit_graph()` into a new function,\n`parse_commit_graph_settings()` that accepts a repo_settings pointer.\nThis allows fuzz-commit-graph to continue to test the commit-graph\nparser implementation without relying on prepare_repo_settings().\n\nAdditionally, properly initialize the\nrepo_settings.commit_graph_generation_version field in\nprepare_repo_settings(). Load the value from the config if present, and\ndefault to version 2 otherwise.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\nRange-diff against v2:\n1:  2c2bfc7b43 ! 1:  9b56496b08 commit-graph: refactor to avoid prepare_repo_settings\n    @@ fuzz-commit-graph.c: int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size\n      \tinitialize_the_repository();\n     -\tg = parse_commit_graph(the_repository, (void *)data, size);\n     +\t/*\n    -+\t * Manually initialize commit-graph settings, to avoid the need to run\n    -+\t * in an actual repository.\n    ++\t * Initialize the_repository with commit-graph settings that would\n    ++\t * normally be read from the repository's gitdir. We want to avoid\n    ++\t * touching the disk to keep the individual fuzz-test cases as fast as\n    ++\t * possible.\n     +\t */\n     +\tthe_repository->settings.commit_graph_generation_version = 2;\n     +\tthe_repository->settings.commit_graph_read_changed_paths = 1;\n\n commit-graph.c      | 22 ++++++++++------------\n commit-graph.h      |  2 ++\n fuzz-commit-graph.c | 10 +++++++++-\n repo-settings.c     | 12 +++++++++++-\n repository.h        |  1 +\n 5 files changed, 33 insertions(+), 14 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 265c010122..c54a734619 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -96,13 +96,6 @@ define_commit_slab(commit_graph_data_slab, struct commit_graph_data);\n static struct commit_graph_data_slab commit_graph_data_slab =\n \tCOMMIT_SLAB_INIT(1, commit_graph_data_slab);\n \n-static int get_configured_generation_version(struct repository *r)\n-{\n-\tint version = 2;\n-\trepo_config_get_int(r, \"commitgraph.generationversion\", &version);\n-\treturn version;\n-}\n-\n uint32_t commit_graph_position(const struct commit *c)\n {\n \tstruct commit_graph_data *data =\n@@ -335,6 +328,13 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,\n \n struct commit_graph *parse_commit_graph(struct repository *r,\n \t\t\t\t\tvoid *graph_map, size_t graph_size)\n+{\n+\tprepare_repo_settings(r);\n+\treturn parse_commit_graph_settings(&r->settings, graph_map, graph_size);\n+}\n+\n+struct commit_graph *parse_commit_graph_settings(struct repo_settings *s,\n+\t\t\t\t\t\t void *graph_map, size_t graph_size)\n {\n \tconst unsigned char *data;\n \tstruct commit_graph *graph;\n@@ -371,8 +371,6 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \t\treturn NULL;\n \t}\n \n-\tprepare_repo_settings(r);\n-\n \tgraph = alloc_commit_graph();\n \n \tgraph->hash_len = the_hash_algo->rawsz;\n@@ -402,14 +400,14 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\n \tpair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n \n-\tif (get_configured_generation_version(r) >= 2) {\n+\tif (s->commit_graph_generation_version >= 2) {\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n \t\t\t&graph->chunk_generation_data);\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n \t\t\t&graph->chunk_generation_data_overflow);\n \t}\n \n-\tif (r->settings.commit_graph_read_changed_paths) {\n+\tif (s->commit_graph_read_changed_paths) {\n \t\tpair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n \t\t\t   &graph->chunk_bloom_indexes);\n \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n@@ -2288,7 +2286,7 @@ int write_commit_graph(struct object_directory *odb,\n \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n \tctx->opts = opts;\n \tctx->total_bloom_filter_data_size = 0;\n-\tctx->write_generation_data = (get_configured_generation_version(r) == 2);\n+\tctx->write_generation_data = (r->settings.commit_graph_generation_version == 2);\n \tctx->num_generation_data_overflows = 0;\n \n \tbloom_settings.bits_per_entry = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY\",\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 04a94e1830..0f0d28b129 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -95,6 +95,8 @@ struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n struct commit_graph *parse_commit_graph(struct repository *r,\n \t\t\t\t\tvoid *graph_map, size_t graph_size);\n+struct commit_graph *parse_commit_graph_settings(struct repo_settings *s,\n+\t\t\t\t\tvoid *graph_map, size_t graph_size);\n \n /*\n  * Return 1 if and only if the repository has a commit-graph\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nindex e7cf6d5b0f..810b1a8001 100644\n--- a/fuzz-commit-graph.c\n+++ b/fuzz-commit-graph.c\n@@ -11,7 +11,15 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tstruct commit_graph *g;\n \n \tinitialize_the_repository();\n-\tg = parse_commit_graph(the_repository, (void *)data, size);\n+\t/*\n+\t * Initialize the_repository with commit-graph settings that would\n+\t * normally be read from the repository's gitdir. We want to avoid\n+\t * touching the disk to keep the individual fuzz-test cases as fast as\n+\t * possible.\n+\t */\n+\tthe_repository->settings.commit_graph_generation_version = 2;\n+\tthe_repository->settings.commit_graph_read_changed_paths = 1;\n+\tg = parse_commit_graph_settings(&the_repository->settings, (void *)data, size);\n \trepo_clear(the_repository);\n \tfree_commit_graph(g);\n \ndiff --git a/repo-settings.c b/repo-settings.c\nindex b4fbd16cdc..26241c1c2c 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -10,6 +10,13 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n \t\t*dest = def;\n }\n \n+static void repo_cfg_int(struct repository *r, const char *key, int *dest,\n+\t\t\t int def)\n+{\n+\tif (repo_config_get_int(r, key, dest))\n+\t\t*dest = def;\n+}\n+\n void prepare_repo_settings(struct repository *r)\n {\n \tint experimental;\n@@ -41,11 +48,14 @@ void prepare_repo_settings(struct repository *r)\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n \t}\n \n-\t/* Boolean config or default, does not cascade (simple)  */\n+\t/* Commit graph config or default, does not cascade (simple) */\n \trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n+\trepo_cfg_int(r, \"commitgraph.generationversion\", &r->settings.commit_graph_generation_version, 2);\n \trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n \trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n \trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\n+\n+\t/* Boolean config or default, does not cascade (simple)  */\n \trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\ndiff --git a/repository.h b/repository.h\nindex ca837cb9e9..4f8275f97c 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -29,6 +29,7 @@ struct repo_settings {\n \tint initialized;\n \n \tint core_commit_graph;\n+\tint commit_graph_generation_version;\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n \tint fetch_write_commit_graph;\n\nbase-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7\n-- \n2.36.1.476.g0c4daa206d-goog\n\n"},{"id":"457227","messageId":"YqkN9F9zlo6WEBvJ@google.com","threadId":"57605","inReplyTo":"20220607200231.1958109-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH v2] commit-graph: refactor to avoid prepare_repo_settings","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-06-14T22:38:44Z","receivedAt":"2022-06-14T22:38:55Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.06.07 13:02, Jonathan Tan wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> > diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> > index e7cf6d5b0f..e53a2635f6 100644\n> > --- a/fuzz-commit-graph.c\n> > +++ b/fuzz-commit-graph.c\n> > @@ -11,7 +11,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n> >  \tstruct commit_graph *g;\n> >  \n> >  \tinitialize_the_repository();\n> > -\tg = parse_commit_graph(the_repository, (void *)data, size);\n> > +\t/*\n> > +\t * Manually initialize commit-graph settings, to avoid the need to run\n> > +\t * in an actual repository.\n> > +\t */\n> > +\tthe_repository->settings.commit_graph_generation_version = 2;\n> > +\tthe_repository->settings.commit_graph_read_changed_paths = 1;\n> > +\tg = parse_commit_graph_settings(&the_repository->settings, (void *)data, size);\n> >  \trepo_clear(the_repository);\n> >  \tfree_commit_graph(g);\n> \n> The comment doesn't explain why we need to avoid an actual repository. Maybe\n> better: Initialize the commit-graph settings that would normally be read from\n> the repository's gitdir.\n> \n> Other than that, this patch looks good to me. Isolating a part of the\n> commit graph mechanism that can be fuzzed without access to the disk is,\n> I think, a good idea, and this patch is a good way to do it.\n\nDone in V3, thanks for taking a look!\n"},{"id":"457228","messageId":"YqkamW45+VbIfJI4@nand.local","threadId":"57605","inReplyTo":"9b56496b0809cc8a25af877ea97042e2cb7f2af6.1655246092.git.steadmon@google.com","subject":"Re: [PATCH v3] commit-graph: refactor to avoid prepare_repo_settings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-14T23:32:41Z","receivedAt":"2022-06-14T23:32:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 14, 2022 at 03:37:21PM -0700, Josh Steadmon wrote:\n> Range-diff against v2:\n> 1:  2c2bfc7b43 ! 1:  9b56496b08 commit-graph: refactor to avoid prepare_repo_settings\n>     @@ fuzz-commit-graph.c: int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size\n>       \tinitialize_the_repository();\n>      -\tg = parse_commit_graph(the_repository, (void *)data, size);\n>      +\t/*\n>     -+\t * Manually initialize commit-graph settings, to avoid the need to run\n>     -+\t * in an actual repository.\n>     ++\t * Initialize the_repository with commit-graph settings that would\n>     ++\t * normally be read from the repository's gitdir. We want to avoid\n>     ++\t * touching the disk to keep the individual fuzz-test cases as fast as\n>     ++\t * possible.\n>      +\t */\n>      +\tthe_repository->settings.commit_graph_generation_version = 2;\n>      +\tthe_repository->settings.commit_graph_read_changed_paths = 1;\n\nThis version looks good to me. Thanks for working on this, Josh!\n\nThanks,\nTaylor\n"},{"id":"457814","messageId":"xmqq1qvfyrbb.fsf@gitster.g","threadId":"57605","inReplyTo":"9b56496b0809cc8a25af877ea97042e2cb7f2af6.1655246092.git.steadmon@google.com","subject":"Re: [PATCH v3] commit-graph: refactor to avoid prepare_repo_settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-23T21:59:20Z","receivedAt":"2022-06-23T21:59:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> This series of changes broke fuzz-commit-graph, which attempts to parse\n> arbitrary fuzzing-engine-provided bytes as a commit graph file.\n> commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n> since we run the fuzz tests without a valid repository, we are hitting\n> the BUG() from 44c7e62 for every test case.\n>\n> Fix this by moving the majority of the implementaiton of\n> `parse_commit_graph()` into a new function,\n> `parse_commit_graph_settings()` that accepts a repo_settings pointer.\n> This allows fuzz-commit-graph to continue to test the commit-graph\n> parser implementation without relying on prepare_repo_settings().\n\nIt sounds like this is not a \"fix\" but a workaround to bend the\nproduction code so that a non-production test shim can be inserted\nmore easily.\n\nI am OK with the idea, but have a huge problem with the new name.\n\nIs it just me who thinks parse_commit_graph_settings() is a function\nthat parses some kind of settings that affects the way the commit\ngraph gets used or written?\n\nStepping back a bit, why can't fuzz-commit-graph prepare a\nrepository object that looks sufficiently real?  Something along the\nlines of...\n\n                struct repository fake_repo;\n\n                fake_repo.settings.initialized = 1;\n                fake_repo.gitdir = \".\";\n                parse_commit_graph(&fake_repo, (void *)data, size);\n\t\t...\n\nAlso, I feel somewhat uneasy to see these changes:\n\n> -\tif (get_configured_generation_version(r) >= 2) {\n> +\tif (s->commit_graph_generation_version >= 2) {\n> -\tif (r->settings.commit_graph_read_changed_paths) {\n> +\tif (s->commit_graph_read_changed_paths) {\n> -\tctx->write_generation_data = (get_configured_generation_version(r) == 2);\n> +\tctx->write_generation_data = (r->settings.commit_graph_generation_version == 2);\n>  \tctx->num_generation_data_overflows = 0;\n\nthat makes the production code bend over backwards to _avoid_\nreferencing 'r', only to cater to the test shim.  That's an\nartificial limitation we are forcing on our developers who works on\nthis code.  It might be that what is in the repository settings is\nsufficient for today's code to work, but I do not think needs for\nfuzz tests should tie the hand of this production code by forbidding\nit to look at other things in the repository in the future.  After\nall, tests are to serve the production code, not the other way\naround.\n\nOn the other hand, I think a change that is slightly smaller than\nthe posted patch, which justifies itself with a completely different\nrationale, would be totally acceptable.  You can justify this change\nwith NO mention of fuzzers.\n\n    The parse_commit_graph() function takes a \"struct repository *\"\n    pointer, but all it cares about is the .settings member of it.\n\n    Update the function and all its existing callers so that it\n    takes \"struct repo_settings *\" instead.\n\nNow, in the future, some developers _might_ find it necessary to\naccess stuff other than the repository settings to validate the\ncontents of the graph file, and we may need to change it to take a\nfull repository structure again.  The test should adjust to such\nneeds of the production code, not the other way around.  But until\nthen...\n\nThanks.\n"},{"id":"459095","messageId":"fd70b6119153b165a62ee4f693dbe47031cfb2be.1657834657.git.steadmon@google.com","threadId":"57605","inReplyTo":"Yjt6mLIfw0V3aVTO@nand.local","subject":"[PATCH v4] commit-graph: pass repo_settings instead of repository","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-07-14T21:43:06Z","receivedAt":"2022-07-14T21:43:13Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Taylor Blau <me@ttaylorr.com>\n\nThe parse_commit_graph() function takes a 'struct repository *' pointer,\nbut it only ever accesses config settings (either directly or through\nthe .settings field of the repo struct). Move all relevant config\nsettings into the repo_settings struct, and update parse_commit_graph()\nand its existing callers so that it takes 'struct repo_settings *'\ninstead.\n\nCallers of parse_commit_graph() will now need to call\nprepare_repo_settings() themselves, or initialize a 'struct\nrepo_settings' directly.\n\nPrior to ab14d0676c (commit-graph: pass a 'struct repository *' in more\nplaces, 2020-09-09), parsing a commit-graph was a pure function\ndepending only on the contents of the commit-graph itself. Commit\nab14d0676c introduced a dependency on a `struct repository` pointer, and\nlater commits such as b66d84756f (commit-graph: respect\n'commitGraph.readChangedPaths', 2020-09-09) added dependencies on config\nsettings, which were accessed through the `settings` field of the\nrepository pointer. This field was initialized via a call to\n`prepare_repo_settings()`.\n\nAdditionally, this fixes an issue in fuzz-commit-graph: In 44c7e62\n(2021-12-06, repo-settings:prepare_repo_settings only in git repos),\nprepare_repo_settings was changed to issue a BUG() if it is called by a\nprocess whose CWD is not a Git repository.\n\nThe combination of commits mentioned above broke fuzz-commit-graph,\nwhich attempts to parse arbitrary fuzzing-engine-provided bytes as a\ncommit graph file. Prior to this change, parse_commit_graph() called\nprepare_repo_settings(), but since we run the fuzz tests without a valid\nrepository, we are hitting the BUG() from 44c7e62 for every test case.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\nRange-diff against v3:\n1:  9b56496b08 ! 1:  fd70b61191 commit-graph: refactor to avoid prepare_repo_settings\n    @@ Metadata\n     Author: Taylor Blau <me@ttaylorr.com>\n     \n      ## Commit message ##\n    -    commit-graph: refactor to avoid prepare_repo_settings\n    +    commit-graph: pass repo_settings instead of repository\n    +\n    +    The parse_commit_graph() function takes a 'struct repository *' pointer,\n    +    but it only ever accesses config settings (either directly or through\n    +    the .settings field of the repo struct). Move all relevant config\n    +    settings into the repo_settings struct, and update parse_commit_graph()\n    +    and its existing callers so that it takes 'struct repo_settings *'\n    +    instead.\n    +\n    +    Callers of parse_commit_graph() will now need to call\n    +    prepare_repo_settings() themselves, or initialize a 'struct\n    +    repo_settings' directly.\n     \n         Prior to ab14d0676c (commit-graph: pass a 'struct repository *' in more\n         places, 2020-09-09), parsing a commit-graph was a pure function\n    @@ Commit message\n         repository pointer. This field was initialized via a call to\n         `prepare_repo_settings()`.\n     \n    -    In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only in git\n    -    repos), prepare_repo_settings was changed to issue a BUG() if it is\n    -    called by a process whose CWD is not a Git repository.\n    -\n    -    This series of changes broke fuzz-commit-graph, which attempts to parse\n    -    arbitrary fuzzing-engine-provided bytes as a commit graph file.\n    -    commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n    -    since we run the fuzz tests without a valid repository, we are hitting\n    -    the BUG() from 44c7e62 for every test case.\n    +    Additionally, this fixes an issue in fuzz-commit-graph: In 44c7e62\n    +    (2021-12-06, repo-settings:prepare_repo_settings only in git repos),\n    +    prepare_repo_settings was changed to issue a BUG() if it is called by a\n    +    process whose CWD is not a Git repository.\n     \n    -    Fix this by moving the majority of the implementaiton of\n    -    `parse_commit_graph()` into a new function,\n    -    `parse_commit_graph_settings()` that accepts a repo_settings pointer.\n    -    This allows fuzz-commit-graph to continue to test the commit-graph\n    -    parser implementation without relying on prepare_repo_settings().\n    -\n    -    Additionally, properly initialize the\n    -    repo_settings.commit_graph_generation_version field in\n    -    prepare_repo_settings(). Load the value from the config if present, and\n    -    default to version 2 otherwise.\n    +    The combination of commits mentioned above broke fuzz-commit-graph,\n    +    which attempts to parse arbitrary fuzzing-engine-provided bytes as a\n    +    commit graph file. Prior to this change, parse_commit_graph() called\n    +    prepare_repo_settings(), but since we run the fuzz tests without a valid\n    +    repository, we are hitting the BUG() from 44c7e62 for every test case.\n     \n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n      ## commit-graph.c ##\n    -@@ commit-graph.c: define_commit_slab(commit_graph_data_slab, struct commit_graph_data);\n    - static struct commit_graph_data_slab commit_graph_data_slab =\n    - \tCOMMIT_SLAB_INIT(1, commit_graph_data_slab);\n    +@@ commit-graph.c: struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n    + \t}\n    + \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n    + \tclose(fd);\n    +-\tret = parse_commit_graph(r, graph_map, graph_size);\n    ++\tprepare_repo_settings(r);\n    ++\tret = parse_commit_graph(&r->settings, graph_map, graph_size);\n      \n    --static int get_configured_generation_version(struct repository *r)\n    --{\n    --\tint version = 2;\n    --\trepo_config_get_int(r, \"commitgraph.generationversion\", &version);\n    --\treturn version;\n    --}\n    --\n    - uint32_t commit_graph_position(const struct commit *c)\n    - {\n    - \tstruct commit_graph_data *data =\n    + \tif (ret)\n    + \t\tret->odb = odb;\n     @@ commit-graph.c: static int graph_read_bloom_data(const unsigned char *chunk_start,\n    + \treturn 0;\n    + }\n      \n    - struct commit_graph *parse_commit_graph(struct repository *r,\n    +-struct commit_graph *parse_commit_graph(struct repository *r,\n    ++struct commit_graph *parse_commit_graph(struct repo_settings *s,\n      \t\t\t\t\tvoid *graph_map, size_t graph_size)\n    -+{\n    -+\tprepare_repo_settings(r);\n    -+\treturn parse_commit_graph_settings(&r->settings, graph_map, graph_size);\n    -+}\n    -+\n    -+struct commit_graph *parse_commit_graph_settings(struct repo_settings *s,\n    -+\t\t\t\t\t\t void *graph_map, size_t graph_size)\n      {\n      \tconst unsigned char *data;\n    - \tstruct commit_graph *graph;\n     @@ commit-graph.c: struct commit_graph *parse_commit_graph(struct repository *r,\n      \t\treturn NULL;\n      \t}\n    @@ commit-graph.c: struct commit_graph *parse_commit_graph(struct repository *r,\n      \t\tpair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n      \t\t\t   &graph->chunk_bloom_indexes);\n      \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n    -@@ commit-graph.c: int write_commit_graph(struct object_directory *odb,\n    - \tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n    - \tctx->opts = opts;\n    - \tctx->total_bloom_filter_data_size = 0;\n    --\tctx->write_generation_data = (get_configured_generation_version(r) == 2);\n    -+\tctx->write_generation_data = (r->settings.commit_graph_generation_version == 2);\n    - \tctx->num_generation_data_overflows = 0;\n    - \n    - \tbloom_settings.bits_per_entry = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY\",\n     \n      ## commit-graph.h ##\n    -@@ commit-graph.h: struct commit_graph *read_commit_graph_one(struct repository *r,\n    +@@ commit-graph.h: struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n    + \t\t\t\t\t\t struct object_directory *odb);\n    + struct commit_graph *read_commit_graph_one(struct repository *r,\n      \t\t\t\t\t   struct object_directory *odb);\n    - struct commit_graph *parse_commit_graph(struct repository *r,\n    +-struct commit_graph *parse_commit_graph(struct repository *r,\n    ++\n    ++/*\n    ++ * Callers should initialize the repo_settings with prepare_repo_settings()\n    ++ * prior to calling parse_commit_graph().\n    ++ */\n    ++struct commit_graph *parse_commit_graph(struct repo_settings *s,\n      \t\t\t\t\tvoid *graph_map, size_t graph_size);\n    -+struct commit_graph *parse_commit_graph_settings(struct repo_settings *s,\n    -+\t\t\t\t\tvoid *graph_map, size_t graph_size);\n      \n      /*\n    -  * Return 1 if and only if the repository has a commit-graph\n     \n      ## fuzz-commit-graph.c ##\n    +@@\n    + #include \"commit-graph.h\"\n    + #include \"repository.h\"\n    + \n    +-struct commit_graph *parse_commit_graph(struct repository *r,\n    ++struct commit_graph *parse_commit_graph(struct repo_settings *s,\n    + \t\t\t\t\tvoid *graph_map, size_t graph_size);\n    + \n    + int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n     @@ fuzz-commit-graph.c: int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n      \tstruct commit_graph *g;\n      \n    @@ fuzz-commit-graph.c: int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size\n     +\t */\n     +\tthe_repository->settings.commit_graph_generation_version = 2;\n     +\tthe_repository->settings.commit_graph_read_changed_paths = 1;\n    -+\tg = parse_commit_graph_settings(&the_repository->settings, (void *)data, size);\n    ++\tg = parse_commit_graph(&the_repository->settings, (void *)data, size);\n      \trepo_clear(the_repository);\n      \tfree_commit_graph(g);\n      \n\n commit-graph.c      | 11 +++++------\n commit-graph.h      |  7 ++++++-\n fuzz-commit-graph.c | 12 ++++++++++--\n repo-settings.c     | 12 +++++++++++-\n repository.h        |  1 +\n 5 files changed, 33 insertions(+), 10 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 265c010122..f305b65117 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -264,7 +264,8 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tret = parse_commit_graph(r, graph_map, graph_size);\n+\tprepare_repo_settings(r);\n+\tret = parse_commit_graph(&r->settings, graph_map, graph_size);\n \n \tif (ret)\n \t\tret->odb = odb;\n@@ -333,7 +334,7 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n-struct commit_graph *parse_commit_graph(struct repository *r,\n+struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t\t\tvoid *graph_map, size_t graph_size)\n {\n \tconst unsigned char *data;\n@@ -371,8 +372,6 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \t\treturn NULL;\n \t}\n \n-\tprepare_repo_settings(r);\n-\n \tgraph = alloc_commit_graph();\n \n \tgraph->hash_len = the_hash_algo->rawsz;\n@@ -402,14 +401,14 @@ struct commit_graph *parse_commit_graph(struct repository *r,\n \tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\n \tpair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n \n-\tif (get_configured_generation_version(r) >= 2) {\n+\tif (s->commit_graph_generation_version >= 2) {\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n \t\t\t&graph->chunk_generation_data);\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n \t\t\t&graph->chunk_generation_data_overflow);\n \t}\n \n-\tif (r->settings.commit_graph_read_changed_paths) {\n+\tif (s->commit_graph_read_changed_paths) {\n \t\tpair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n \t\t\t   &graph->chunk_bloom_indexes);\n \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 04a94e1830..c89b336791 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -93,7 +93,12 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n \t\t\t\t\t\t struct object_directory *odb);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n-struct commit_graph *parse_commit_graph(struct repository *r,\n+\n+/*\n+ * Callers should initialize the repo_settings with prepare_repo_settings()\n+ * prior to calling parse_commit_graph().\n+ */\n+struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t\t\tvoid *graph_map, size_t graph_size);\n \n /*\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nindex e7cf6d5b0f..914026f5d8 100644\n--- a/fuzz-commit-graph.c\n+++ b/fuzz-commit-graph.c\n@@ -1,7 +1,7 @@\n #include \"commit-graph.h\"\n #include \"repository.h\"\n \n-struct commit_graph *parse_commit_graph(struct repository *r,\n+struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t\t\tvoid *graph_map, size_t graph_size);\n \n int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n@@ -11,7 +11,15 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tstruct commit_graph *g;\n \n \tinitialize_the_repository();\n-\tg = parse_commit_graph(the_repository, (void *)data, size);\n+\t/*\n+\t * Initialize the_repository with commit-graph settings that would\n+\t * normally be read from the repository's gitdir. We want to avoid\n+\t * touching the disk to keep the individual fuzz-test cases as fast as\n+\t * possible.\n+\t */\n+\tthe_repository->settings.commit_graph_generation_version = 2;\n+\tthe_repository->settings.commit_graph_read_changed_paths = 1;\n+\tg = parse_commit_graph(&the_repository->settings, (void *)data, size);\n \trepo_clear(the_repository);\n \tfree_commit_graph(g);\n \ndiff --git a/repo-settings.c b/repo-settings.c\nindex b4fbd16cdc..26241c1c2c 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -10,6 +10,13 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n \t\t*dest = def;\n }\n \n+static void repo_cfg_int(struct repository *r, const char *key, int *dest,\n+\t\t\t int def)\n+{\n+\tif (repo_config_get_int(r, key, dest))\n+\t\t*dest = def;\n+}\n+\n void prepare_repo_settings(struct repository *r)\n {\n \tint experimental;\n@@ -41,11 +48,14 @@ void prepare_repo_settings(struct repository *r)\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n \t}\n \n-\t/* Boolean config or default, does not cascade (simple)  */\n+\t/* Commit graph config or default, does not cascade (simple) */\n \trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n+\trepo_cfg_int(r, \"commitgraph.generationversion\", &r->settings.commit_graph_generation_version, 2);\n \trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n \trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n \trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\n+\n+\t/* Boolean config or default, does not cascade (simple)  */\n \trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\ndiff --git a/repository.h b/repository.h\nindex ca837cb9e9..4f8275f97c 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -29,6 +29,7 @@ struct repo_settings {\n \tint initialized;\n \n \tint core_commit_graph;\n+\tint commit_graph_generation_version;\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n \tint fetch_write_commit_graph;\n\nbase-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7\n-- \n2.37.0.170.g444d1eabd0-goog\n\n"},{"id":"459096","messageId":"YtCOMegkRgaY3gnX@google.com","threadId":"57605","inReplyTo":"xmqq1qvfyrbb.fsf@gitster.g","subject":"Re: [PATCH v3] commit-graph: refactor to avoid prepare_repo_settings","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-07-14T21:44:17Z","receivedAt":"2022-07-14T21:44:34Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.06.23 14:59, Junio C Hamano wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n> > This series of changes broke fuzz-commit-graph, which attempts to parse\n> > arbitrary fuzzing-engine-provided bytes as a commit graph file.\n> > commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but\n> > since we run the fuzz tests without a valid repository, we are hitting\n> > the BUG() from 44c7e62 for every test case.\n> >\n> > Fix this by moving the majority of the implementaiton of\n> > `parse_commit_graph()` into a new function,\n> > `parse_commit_graph_settings()` that accepts a repo_settings pointer.\n> > This allows fuzz-commit-graph to continue to test the commit-graph\n> > parser implementation without relying on prepare_repo_settings().\n> \n> It sounds like this is not a \"fix\" but a workaround to bend the\n> production code so that a non-production test shim can be inserted\n> more easily.\n> \n> I am OK with the idea, but have a huge problem with the new name.\n> \n> Is it just me who thinks parse_commit_graph_settings() is a function\n> that parses some kind of settings that affects the way the commit\n> graph gets used or written?\n> \n> Stepping back a bit, why can't fuzz-commit-graph prepare a\n> repository object that looks sufficiently real?  Something along the\n> lines of...\n> \n>                 struct repository fake_repo;\n> \n>                 fake_repo.settings.initialized = 1;\n>                 fake_repo.gitdir = \".\";\n>                 parse_commit_graph(&fake_repo, (void *)data, size);\n> \t\t...\n> \n> Also, I feel somewhat uneasy to see these changes:\n> \n> > -\tif (get_configured_generation_version(r) >= 2) {\n> > +\tif (s->commit_graph_generation_version >= 2) {\n> > -\tif (r->settings.commit_graph_read_changed_paths) {\n> > +\tif (s->commit_graph_read_changed_paths) {\n> > -\tctx->write_generation_data = (get_configured_generation_version(r) == 2);\n> > +\tctx->write_generation_data = (r->settings.commit_graph_generation_version == 2);\n> >  \tctx->num_generation_data_overflows = 0;\n> \n> that makes the production code bend over backwards to _avoid_\n> referencing 'r', only to cater to the test shim.  That's an\n> artificial limitation we are forcing on our developers who works on\n> this code.  It might be that what is in the repository settings is\n> sufficient for today's code to work, but I do not think needs for\n> fuzz tests should tie the hand of this production code by forbidding\n> it to look at other things in the repository in the future.  After\n> all, tests are to serve the production code, not the other way\n> around.\n> \n> On the other hand, I think a change that is slightly smaller than\n> the posted patch, which justifies itself with a completely different\n> rationale, would be totally acceptable.  You can justify this change\n> with NO mention of fuzzers.\n> \n>     The parse_commit_graph() function takes a \"struct repository *\"\n>     pointer, but all it cares about is the .settings member of it.\n> \n>     Update the function and all its existing callers so that it\n>     takes \"struct repo_settings *\" instead.\n> \n> Now, in the future, some developers _might_ find it necessary to\n> access stuff other than the repository settings to validate the\n> contents of the graph file, and we may need to change it to take a\n> full repository structure again.  The test should adjust to such\n> needs of the production code, not the other way around.  But until\n> then...\n> \n> Thanks.\n\nI trimmed down the changes and reworded the commit message for V4. I'm\nassuming you don't object to mentioning the fuzzer, just that it\nshouldn't be the main justifiation for the change.\n\nSorry for the delayed response, and thanks for the feedback.\n"},{"id":"459100","messageId":"xmqqtu7j2tvl.fsf@gitster.g","threadId":"57605","inReplyTo":"fd70b6119153b165a62ee4f693dbe47031cfb2be.1657834657.git.steadmon@google.com","subject":"Re: [PATCH v4] commit-graph: pass repo_settings instead of repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-14T22:48:30Z","receivedAt":"2022-07-14T22:48:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> From: Taylor Blau <me@ttaylorr.com>\n>\n> The parse_commit_graph() function takes a 'struct repository *' pointer,\n> but it only ever accesses config settings (either directly or through\n> the .settings field of the repo struct). Move all relevant config\n> settings into the repo_settings struct, and update parse_commit_graph()\n> and its existing callers so that it takes 'struct repo_settings *'\n> instead.\n\nSounds good.  Will queue.\n\nI think this is ready to be merged down to 'next' by now?\n\nThanks.\n"}]}