git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Josh Steadmon <steadmon@google.com>
Date
Mar 23, 2022, 18:03 UTC
Message-ID
<1b27e0b115f858a422e0a2891688227be8f3db01.1648055915.git.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
Next: Derrick Stolee
Message 1 of 27 in “repo-settings: set defaults even when not in a repo”
  1. repo-settings: set defaults even when not in a repoJosh Steadmon, Mar 23, 2022
  2. Derrick StoleeMar 23, 2022
  3. Taylor BlauMar 23, 2022
  4. Josh SteadmonMar 28, 2022
  5. Taylor BlauMar 29, 2022
  6. Josh SteadmonMar 28, 2022
  7. Taylor BlauMar 29, 2022
  8. Ævar Arnfjörð BjarmasonMar 29, 2022
  9. Taylor BlauMar 30, 2022
  10. Josh SteadmonApr 9, 2022
  11. Ævar Arnfjörð BjarmasonMar 29, 2022
  12. Taylor BlauMar 30, 2022
  13. Ævar Arnfjörð BjarmasonMar 30, 2022
  14. Junio C HamanoMar 30, 2022
  15. commit-graph: refactor to avoid prepare_repo_settingsJosh Steadmon, Apr 9, 2022
  16. Jonathan TanJun 7, 2022
  17. Josh SteadmonJun 14, 2022
  18. commit-graph: refactor to avoid prepare_repo_settingsJosh Steadmon, Jun 14, 2022
  19. Taylor BlauJun 14, 2022
  20. Junio C HamanoJun 23, 2022
  21. Josh SteadmonJul 14, 2022
  22. commit-graph: pass repo_settings instead of repositoryJosh Steadmon, Jul 14, 2022
  23. Junio C HamanoJul 14, 2022
  24. Victoria DyeMar 23, 2022
  25. Junio C HamanoMar 23, 2022
  26. Victoria DyeMar 23, 2022
  27. Junio C HamanoMar 23, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.