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

[PATCH v2 4/5] repo-settings.c: simplify the setup

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Sep 16, 2021, 18:30 UTC
Message-ID
<patch-v2-4.5-b28ad2b2607-20210916T182918Z-avarab@gmail.com>
In-Reply-To
<cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com>

Simplify the setup code in repo-settings.c in various ways, making the code shorter, easier to read, and requiring fewer hacks to do the same thing as it did before:

Since 7211b9e7534 (repo-settings: consolidate some config settings, 2019-08-13) we have memset() the whole "settings" structure to -1 in prepare_repo_settings(), and subsequently relied on the -1 value.

Most of the fields did not need to be initialized to -1, and because we were doing that we had the enum labels "UNTRACKED_CACHE_UNSET" and "FETCH_NEGOTIATION_UNSET" purely to reflect the resulting state created this memset() in prepare_repo_settings(). No other code used or relied on them, more on that below.

For the rest most of the subsequent "are we -1, then read xyz" can simply be removed by re-arranging what we read first. E.g. when setting the "index.version" setting we should have first read "feature.experimental", so that it (and "feature.manyfiles") can provide a default for our "index.version".

Instead the code setting it, added when "feature.manyFiles"[1] was created, was using the UPDATE_DEFAULT_BOOL() macro added in an earlier commit[2]. That macro is now gone, since it was only needed for this pattern of reading things in the wrong order.

This also fixes an (admittedly obscure) logic error where we'd conflate an explicit "-1" value in the config with our own earlier memset() -1.

We can also remove the UPDATE_DEFAULT_BOOL() wrapper added in [3]. Using it is redundant to simply using the return value from repo_config_get_bool(), which is non-zero if the provided key exists in the config.

Details on edge cases relating to the memset() to -1, continued from "more on that below" above:

 * UNTRACKED_CACHE_KEEP:
   In [4] the "unset" and "keep" handling for core.untrackedCache was
   consolidated. But it while we understand the "keep" value, we don't
   handle it differently than the case of any other unknown value. So
   we can remove UNTRACKED_CACHE_KEEP. It's not handled any
   differently than UNTRACKED_CACHE_UNSET once we get past the config
   parsing step.
 * FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:
   Since these two two enum fields added in [5] don't rely on the
   memzero() setting them to "-1" anymore we don't have to provide
   them with explicit values.
1. c6cc4c5afd2 (repo-settings: create feature.manyFiles setting,
   2019-08-13)
2. 31b1de6a09b (commit-graph: turn on commit-graph by default,
   2019-08-13)
3. 31b1de6a09b (commit-graph: turn on commit-graph by default,
   2019-08-13)
4. ad0fb659993 (repo-settings: parse core.untrackedCache,
   2019-08-13)
5. aaf633c2ad1 (repo-settings: create feature.experimental setting,
   2019-08-13)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 fetch-negotiator.c |   2 -
 read-cache.c       |   2 +-
 repo-settings.c    | 102 +++++++++++++++++++++++++--------------------
 repository.h       |  15 +++----
 4 files changed, 63 insertions(+), 58 deletions(-)
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index e7b5878be7c..273390229fe 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -19,8 +19,6 @@ void fetch_negotiator_init(struct repository *r,
 		return;
 
 	case FETCH_NEGOTIATION_DEFAULT:
-	case FETCH_NEGOTIATION_UNSET:
-	case FETCH_NEGOTIATION_NONE:
 		default_negotiator_init(negotiator);
 		return;
 	}
diff --git a/read-cache.c b/read-cache.c
index 9dd84d69f00..9bbbbbacabe 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1952,7 +1952,7 @@ static void tweak_untracked_cache(struct index_state *istate)
 		add_untracked_cache(istate);
 		break;
 	case UNTRACKED_CACHE_UNSET:
-	case UNTRACKED_CACHE_KEEP:
+		/* This includes core.untrackedCache=keep */
 		break;
 	}
 	return;
diff --git a/repo-settings.c b/repo-settings.c
index b0df8b93b86..6355bc67517 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -3,40 +3,76 @@
 #include "repository.h"
 #include "midx.h"
 
-#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
+static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
+			  int def)
+{
+	if (repo_config_get_bool(r, key, dest))
+		*dest = def;
+}
 
 void prepare_repo_settings(struct repository *r)
 {
+	int experimental;
 	int value;
 	char *strval;
+	int manyfiles;
 
-	if (r->settings.initialized)
+	if (r->settings.initialized++)
 		return;
 
 	/* Defaults */
-	memset(&r->settings, -1, sizeof(r->settings));
+	r->settings.index_version = -1;
+	r->settings.core_untracked_cache = UNTRACKED_CACHE_UNSET;
+	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+
+	/* 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 (!repo_config_get_bool(r, "core.commitgraph", &value))
-		r->settings.core_commit_graph = value;
-	if (!repo_config_get_bool(r, "commitgraph.readchangedpaths", &value))
-		r->settings.commit_graph_read_changed_paths = value;
-	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
-		r->settings.gc_write_commit_graph = value;
-	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
-	UPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);
-	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
+	/* 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);
+
+	/*
+	 * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
+	 * either it *or* the config sets
+	 * r->settings.core_multi_pack_index if true. We don't take
+	 * the environment variable if it exists (even if false) over
+	 * any config, as in most other cases.
+	 */
+	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_maybe_bool(r, "core.untrackedcache", &value)) {
-		if (value == 0)
-			r->settings.core_untracked_cache = UNTRACKED_CACHE_REMOVE;
-		else
-			r->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;
-	} else if (!repo_config_get_string(r, "core.untrackedcache", &strval)) {
-		if (!strcasecmp(strval, "keep"))
-			r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
 
+	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 UNTRACKED_CACHE_UNSET.
+		 */
+		if (v >= 0)
+			r->settings.core_untracked_cache = v ?
+				UNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;
 		free(strval);
 	}
 
@@ -45,34 +81,8 @@ void prepare_repo_settings(struct repository *r)
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
 		else if (!strcasecmp(strval, "noop"))
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;
-		else
-			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
 	}
 
-	if (!repo_config_get_bool(r, "pack.usesparse", &value))
-		r->settings.pack_use_sparse = value;
-	UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);
-
-	value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);
-	if (value || !repo_config_get_bool(r, "core.multipackindex", &value))
-		r->settings.core_multi_pack_index = value;
-	UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);
-
-	if (!repo_config_get_bool(r, "feature.manyfiles", &value) && value) {
-		UPDATE_DEFAULT_BOOL(r->settings.index_version, 4);
-		UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);
-	}
-
-	if (!repo_config_get_bool(r, "fetch.writecommitgraph", &value))
-		r->settings.fetch_write_commit_graph = value;
-	UPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0);
-
-	if (!repo_config_get_bool(r, "feature.experimental", &value) && value)
-		UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);
-
-	UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);
-	UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);
-
 	/*
 	 * This setting guards all index reads to require a full index
 	 * over a sparse index. After suitable guards are placed in the
diff --git a/repository.h b/repository.h
index 3740c93bc0f..c03fe8da8d0 100644
--- a/repository.h
+++ b/repository.h
@@ -13,18 +13,15 @@ struct submodule_cache;
 struct promisor_remote_config;
 
 enum untracked_cache_setting {
-	UNTRACKED_CACHE_UNSET = -1,
-	UNTRACKED_CACHE_REMOVE = 0,
-	UNTRACKED_CACHE_KEEP = 1,
-	UNTRACKED_CACHE_WRITE = 2
+	UNTRACKED_CACHE_UNSET,
+	UNTRACKED_CACHE_REMOVE,
+	UNTRACKED_CACHE_WRITE,
 };
 
 enum fetch_negotiation_setting {
-	FETCH_NEGOTIATION_UNSET = -1,
-	FETCH_NEGOTIATION_NONE = 0,
-	FETCH_NEGOTIATION_DEFAULT = 1,
-	FETCH_NEGOTIATION_SKIPPING = 2,
-	FETCH_NEGOTIATION_NOOP = 3,
+	FETCH_NEGOTIATION_DEFAULT,
+	FETCH_NEGOTIATION_SKIPPING,
+	FETCH_NEGOTIATION_NOOP,
 };
 
 struct repo_settings {

-- 
2.33.0.1092.g44c994ea1be
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 10 of 34 in “repo-settings.c: refactor for clarity, get rid of hacks etc.”
  1. 0/5 repo-settings.c: refactor for clarity, get rid of hacks etc.Ævar Arnfjörð Bjarmason, Sep 16, 2021
  2. 1/5 wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.cÆvar Arnfjörð Bjarmason, Sep 16, 2021
  3. Junio C HamanoSep 17, 2021
  4. Ævar Arnfjörð BjarmasonSep 17, 2021
  5. 2/5 environment.c: remove test-specific "ignore_untracked..." variableÆvar Arnfjörð Bjarmason, Sep 16, 2021
  6. Junio C HamanoSep 17, 2021
  7. 3/5 read-cache & fetch-negotiator: check "enum" values in switch()Ævar Arnfjörð Bjarmason, Sep 16, 2021
  8. Junio C HamanoSep 17, 2021
  9. 5/5 repository.h: don't use a mix of int and bitfieldsÆvar Arnfjörð Bjarmason, Sep 16, 2021
  10. 4/5 repo-settings.c: simplify the setupÆvar Arnfjörð Bjarmason, Sep 16, 2021
  11. 0/5 repo-settings.c: refactor for clarity, get rid of hacks etc.Ævar Arnfjörð Bjarmason, Sep 19, 2021
  12. 2/5 environment.c: remove test-specific "ignore_untracked..." variableÆvar Arnfjörð Bjarmason, Sep 19, 2021
  13. Taylor BlauSep 20, 2021
  14. Ævar Arnfjörð BjarmasonSep 20, 2021
  15. 1/5 wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.cÆvar Arnfjörð Bjarmason, Sep 19, 2021
  16. Taylor BlauSep 20, 2021
  17. Ævar Arnfjörð BjarmasonSep 20, 2021
  18. 3/5 read-cache & fetch-negotiator: check "enum" values in switch()Ævar Arnfjörð Bjarmason, Sep 19, 2021
  19. Taylor BlauSep 20, 2021
  20. Ævar Arnfjörð BjarmasonSep 20, 2021
  21. 5/5 repository.h: don't use a mix of int and bitfieldsÆvar Arnfjörð Bjarmason, Sep 19, 2021
  22. Taylor BlauSep 20, 2021
  23. 4/5 repo-settings.c: simplify the setupÆvar Arnfjörð Bjarmason, Sep 19, 2021
  24. Derrick StoleeSep 20, 2021
  25. Taylor BlauSep 20, 2021
  26. 0/5 repo-settings.c: refactor for clarity, get rid of hacks etc.Ævar Arnfjörð Bjarmason, Sep 21, 2021
  27. 1/5 wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.cÆvar Arnfjörð Bjarmason, Sep 21, 2021
  28. 2/5 environment.c: remove test-specific "ignore_untracked..." variableÆvar Arnfjörð Bjarmason, Sep 21, 2021
  29. 3/5 read-cache & fetch-negotiator: check "enum" values in switch()Ævar Arnfjörð Bjarmason, Sep 21, 2021
  30. 4/5 repo-settings.c: simplify the setupÆvar Arnfjörð Bjarmason, Sep 21, 2021
  31. 5/5 repository.h: don't use a mix of int and bitfieldsÆvar Arnfjörð Bjarmason, Sep 21, 2021
  32. Derrick StoleeSep 21, 2021
  33. Taylor BlauSep 21, 2021
  34. Junio C HamanoSep 22, 2021

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.