[RFC PATCH] repo-settings: set defaults even when not in a repo

[Josh Steadmon <steadmon@google.com>]

prepare_repo_settings() initializes a `struct repository` with various
default config options and settings read from a repository-local config
file. In 44c7e62 (2021-12-06, repo-settings:prepare_repo_settings only
in git repos), prepare_repo_settings was changed to issue a BUG() if it
is called by a process whose CWD is not a Git repository. This approach
was suggested in [1].

This breaks fuzz-commit-graph, which attempts to parse arbitrary
fuzzing-engine-provided bytes as a commit graph file.
commit-graph.c:parse_commit_graph() calls prepare_repo_settings(), but
since we run the fuzz tests without a valid repository, we are hitting
the BUG() from 44c7e62 for every test case.

Fix this by refactoring prepare_repo_settings() such that it sets
default options unconditionally; if its process is in a Git repository,
it will also load settings from the local config. This eliminates the
need for a BUG() when not in a repository.

Concerns:

Are any callers strictly dependent on having a BUG() here? I suspect
that the worst that would happen is that rather than this BUG(), the
caller would later hit its own BUG() or die(), so I do not think this is
a blocker. Additionally, every builtin that directly calls
prepare_repo_settings is either marked as RUN_SETUP, which means we
would die() prior to calling it anyway, or checks on its own before
calling it (builtin/diff.c). There are several callers in library code,
though, and I have not tracked down how all of those are used.

Alternatives considered:

Setting up a valid repository for fuzz testing would avoid triggering
this bug, but would unacceptably slow down the test cases.

Refactoring parse_commit_graph() in such a way that the fuzz test has an
alternate entry point that avoids calling prepare_repo_settings() might
be possible, but would be a much larger change than this one. It would
also run the risk that the changes would be so extensive that the fuzzer
would be merely testing its own custom commit-graph implementation,
rather than the one that's actually used in the real world.

[1]: https://lore.kernel.org/git/xmqqcznh8913.fsf@gitster.g/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 repo-settings.c | 111 ++++++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 56 deletions(-)

diff --git a/repo-settings.c b/repo-settings.c
index b4fbd16cdc..d154b37007 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -17,9 +17,6 @@ void prepare_repo_settings(struct repository *r)
 	char *strval;
 	int manyfiles;
 
-	if (!r->gitdir)
-		BUG("Cannot add settings for uninitialized repository");
-
 	if (r->settings.initialized++)
 		return;
 
@@ -28,27 +25,63 @@ void prepare_repo_settings(struct repository *r)
 	r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
 	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
 
-	/* Booleans config or default, cascades to other settings */
-	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
-	repo_cfg_bool(r, "feature.experimental", &experimental, 0);
+	if (r->gitdir) {
+		/* Booleans config or default, cascades to other settings */
+		repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
+		repo_cfg_bool(r, "feature.experimental", &experimental, 0);
 
-	/* Defaults modified by feature.* */
-	if (experimental) {
-		r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
-	}
-	if (manyfiles) {
-		r->settings.index_version = 4;
-		r->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;
-	}
+		/* Defaults modified by feature.* */
+		if (experimental) {
+			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
+		}
+		if (manyfiles) {
+			r->settings.index_version = 4;
+			r->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;
+		}
+
+		/* Boolean config or default, does not cascade (simple)  */
+		repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
+		repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
+		repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
+		repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
+		repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
+		repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
+		repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);
 
-	/* Boolean config or default, does not cascade (simple)  */
-	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
-	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
-	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
-	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
-	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
-	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
-	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);
+		/*
+		 * Non-boolean config
+		 */
+		if (!repo_config_get_int(r, "index.version", &value))
+			r->settings.index_version = value;
+
+		if (!repo_config_get_string(r, "core.untrackedcache", &strval)) {
+			int v = git_parse_maybe_bool(strval);
+
+			/*
+			 * If it's set to "keep", or some other non-boolean
+			 * value then "v < 0". Then we do nothing and keep it
+			 * at the default of UNTRACKED_CACHE_KEEP.
+			 */
+			if (v >= 0)
+				r->settings.core_untracked_cache = v ?
+					UNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;
+			free(strval);
+		}
+
+		if (!repo_config_get_string(r, "fetch.negotiationalgorithm", &strval)) {
+			int fetch_default = r->settings.fetch_negotiation_algorithm;
+			if (!strcasecmp(strval, "skipping"))
+				r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
+			else if (!strcasecmp(strval, "noop"))
+				r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;
+			else if (!strcasecmp(strval, "consecutive"))
+				r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
+			else if (!strcasecmp(strval, "default"))
+				r->settings.fetch_negotiation_algorithm = fetch_default;
+			else
+				die("unknown fetch negotiation algorithm '%s'", strval);
+		}
+	}
 
 	/*
 	 * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
@@ -60,40 +93,6 @@ void prepare_repo_settings(struct repository *r)
 	if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))
 		r->settings.core_multi_pack_index = 1;
 
-	/*
-	 * Non-boolean config
-	 */
-	if (!repo_config_get_int(r, "index.version", &value))
-		r->settings.index_version = value;
-
-	if (!repo_config_get_string(r, "core.untrackedcache", &strval)) {
-		int v = git_parse_maybe_bool(strval);
-
-		/*
-		 * If it's set to "keep", or some other non-boolean
-		 * value then "v < 0". Then we do nothing and keep it
-		 * at the default of UNTRACKED_CACHE_KEEP.
-		 */
-		if (v >= 0)
-			r->settings.core_untracked_cache = v ?
-				UNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;
-		free(strval);
-	}
-
-	if (!repo_config_get_string(r, "fetch.negotiationalgorithm", &strval)) {
-		int fetch_default = r->settings.fetch_negotiation_algorithm;
-		if (!strcasecmp(strval, "skipping"))
-			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
-		else if (!strcasecmp(strval, "noop"))
-			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;
-		else if (!strcasecmp(strval, "consecutive"))
-			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
-		else if (!strcasecmp(strval, "default"))
-			r->settings.fetch_negotiation_algorithm = fetch_default;
-		else
-			die("unknown fetch negotiation algorithm '%s'", strval);
-	}
-
 	/*
 	 * This setting guards all index reads to require a full index
 	 * over a sparse index. After suitable guards are placed in the

base-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7
-- 
2.35.1.894.gb6a874cedc-goog