{"thread":{"id":"56517","subject":"[PATCH v2 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c","startedAt":"2021-09-16T18:59:53Z","lastAt":"2021-09-22T20:23:58Z","messageCount":34,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Derrick Stolee","Taylor Blau"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"436137","messageId":"patch-v2-1.5-49706b26642-20210916T182918Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com","subject":"[PATCH v2 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T18:30:32Z","receivedAt":"2021-09-16T18:59:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add\nset_git_dir() function, 2007-08-01) we started checking its return\nvalue, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,\n2018-03-30) we've had set_git_dir_1() die if we couldn't set it.\n\nLet's provide a wrapper for both, this will be useful in many other\nplaces, a subsequent patch will make another use of xsetenv().\n\nThe checking of the return value here is over-eager according to\nsetenv(3) and POSIX. It's documented as returning just -1 or 0, so\nperhaps we should be checking -1 explicitly.\n\nLet's just instead die on any non-zero, if our C library is so broken\nas to return something else than -1 on error (and perhaps not set\nerrno?) the worst we'll do is die with a nonsensical errno value, but\nwe'll want to die in either case.\n\nWe could make these return \"void\" (as far as I can tell there's no\nother x*() wrappers that needed to make that decision before),\ni.e. our \"return 0\" is only to indicate that we didn't error, which we\nwould have died on. Let's return \"int\" instead to be consistent with\nthe C library function signatures, including for any future code that\nexpects a pointer to a setenv()-like function.\n\n1. https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n environment.c | 3 +--\n git-compat-util.h | 2 ++\n wrapper.c | 18 ++++++++++++++++++\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex d6b22ede7ea..7d8a949285c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -330,8 +330,7 @@ char *get_graft_file(struct repository *r)\n \n static void set_git_dir_1(const char *path)\n {\n-\tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n-\t\tdie(_(\"could not set GIT_DIR to '%s'\"), path);\n+\txsetenv(GIT_DIR_ENVIRONMENT, path, 1);\n \tsetup_git_env(path);\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b46605300ab..0b0c0305165 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -875,6 +875,8 @@ void *xmemdupz(const void *data, size_t len);\n char *xstrndup(const char *str, size_t len);\n void *xrealloc(void *ptr, size_t size);\n void *xcalloc(size_t nmemb, size_t size);\n+int xsetenv(const char *name, const char *value, int overwrite);\n+int xunsetenv(const char *name);\n void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n const char *mmap_os_err(void);\n void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\ndiff --git a/wrapper.c b/wrapper.c\nindex 7c6586af321..f784904fd67 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -145,6 +145,24 @@ void *xcalloc(size_t nmemb, size_t size)\n \treturn ret;\n }\n \n+int xsetenv(const char *name, const char *value, int overwrite)\n+{\n+\tif (!name)\n+\t\tdie(\"xsetenv() got a NULL name, setenv() would return EINVAL\");\n+\tif (setenv(name, value, overwrite))\n+\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name, value, overwrite);\n+\treturn 0;\n+}\n+\n+int xunsetenv(const char *name)\n+{\n+\tif (!name)\n+\t\tdie(\"xunsetenv() got a NULL name, xunsetenv() would return EINVAL\");\n+\tif (!unsetenv(name))\n+\t\tdie_errno(\"unsetenv(%s) failed\", name);\n+\treturn 0;\n+}\n+\n /*\n * Limit size of IO chunks, because huge chunks only cause pain. OS X\n * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436138","messageId":"cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com","threadId":"56517","inReplyTo":null,"subject":"[PATCH v2 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T18:30:31Z","receivedAt":"2021-09-16T18:59:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Every time I look at repo-settings.c during a review I get distracted\njust trying to figure out how the proposed change interacts with it.\n\nWe've got a global extern variable there, a memset() to -1 of \"struct\nrepo_settings\" (including of an unsigned one-bit, so we'll set it to\n1, not -1), setting defaults, then reading config, then deciding which\nwe'd prefer instead of setting a default, and then reading config\netc. Then we read some boolean variables and conflate \"does the config\nexist\" with \"is the setting 'false''?\n\nThis series cleans up the control flow in in repo-settings.c, this is\na net code deletion when accounting for commetns I added. Much of the\ncode didn't need to exist, e.g. the global variable is replaced by\nPatrick's recent \"config via env\" feature.\n\nI originally submitted this as one patch back in April[1], but since\nit conflicted with the in-flight fsmonitor topic I held it back.\n\nNow that the new fsmonitor topic is being submitted piecemeal there's\nno in-flight conflict with anything else changing repo-settings.c,\nwhich is why I wanted to submit this now, despite saying I'd try to\nhold off on new such topics in general[2]. I'd like the code not to\nlure authors into some of the confusing patterns noted here, and in\nearlier patch review[3].\n\n1. https://lore.kernel.org/git/patch-1.1-e1d8c842c70-20210428T161817Z-avarab@gmail.com/#t\n2. https://lore.kernel.org/git/87mtog4pai.fsf@evledraar.gmail.com/\n3. https://lore.kernel.org/git/87k0omzv3h.fsf@evledraar.gmail.com/\n\nÆvar Arnfjörð Bjarmason (5):\n wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c\n environment.c: remove test-specific \"ignore_untracked...\" variable\n read-cache & fetch-negotiator: check \"enum\" values in switch()\n repo-settings.c: simplify the setup\n repository.h: don't use a mix of int and bitfields\n\n cache.h | 7 --\n environment.c | 10 +--\n fetch-negotiator.c | 1 -\n git-compat-util.h | 2 +\n read-cache.c | 15 ++--\n repo-settings.c | 115 +++++++++++++--------------\n repository.h | 20 ++---\n t/helper/test-dump-untracked-cache.c | 6 +-\n wrapper.c | 18 +++++\n 9 files changed, 100 insertions(+), 94 deletions(-)\n\nRange-diff against v1:\n-: ----------- > 1: 49706b26642 wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c\n-: ----------- > 2: 57290842f0f environment.c: remove test-specific \"ignore_untracked...\" variable\n-: ----------- > 3: 9f1bb0496ed read-cache & fetch-negotiator: check \"enum\" values in switch()\n1: 29406a40f9b ! 4: b28ad2b2607 repo-settings.c: simplify the setup\n @@ Commit message\n thing as it did before:\n \n Since 7211b9e7534 (repo-settings: consolidate some config settings,\n - 2019-08-13) we have memset() the whole \"settings\" structure to -1, and\n - subsequently relied on the -1 value. As it turns out most things did\n - not need to be initialized to -1, and e.g. UNTRACKED_CACHE_UNSET and\n - FETCH_NEGOTIATION_UNSET existed purely to reflect the previous\n - internal state of the prepare_repo_settings() function.\n -\n - Much of the \"are we -1, then read xyz\" can simply be removed by\n - re-arranging what we read first. E.g. we should read\n - feature.experimental first, set some values, and then e.g. an explicit\n - index.version setting should override that. We don't need to read\n - index.version first, and then check when reading feature.experimental\n - if it's still -1.\n -\n - Instead of the global ignore_untracked_cache_config variable added in\n - dae6c322fa1 (test-dump-untracked-cache: don't modify the untracked\n - cache, 2016-01-27) we can make use of the new facility to set config\n - via environment variables added in d8d77153eaf (config: allow\n - specifying config entries via envvar pairs, 2021-01-12).\n -\n - It's arguably a bit hacky to use setenv() and getenv() to pass\n - messages between the same program, but since the test helpers are not\n - the main intended audience of repo-settings.c I think it's better than\n - hardcoding the test-only special-case in prepare_repo_settings().\n -\n - In ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\n - the \"unset\" and \"keep\" handling for core.untrackedCache was\n - consolidated. But it apparently wasn't noticed that while we\n - understand the \"keep\" value, we actually don't handle it differently\n - than the case of any other unknown value.\n -\n - So we can remove UNTRACKED_CACHE_KEEP from the codebase. It's not\n - handled any differently than UNTRACKED_CACHE_UNSET once we get past\n - the config parsing step.\n -\n - The UPDATE_DEFAULT_BOOL() wrapper added in 31b1de6a09b (commit-graph:\n - turn on commit-graph by default, 2019-08-13) is redundant to simply\n - using the return value from repo_config_get_bool(), which is non-zero\n - if the provided key exists in the config.\n -\n - This also fixes an (admittedly obscure) logic error in the previous\n - code where we'd conflate an explicit \"-1\" value in the config with our\n - own earlier memset() -1.\n -\n - Since the two enum fields added in aaf633c2ad1 (repo-settings: create\n - feature.experimental setting, 2019-08-13) and\n - ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\n - don't rely on the memzero() setting them to \"-1\" anymore we don't have\n - to provide them with explicit values. Let's also explicitly use the\n - enum type in read-cache.c and fetch-negotiator.c for\n - self-documentation. Since the FETCH_NEGOTIATION_UNSET is gone we can\n - remove the \"default\" case in fetch-negotiator.c, and rely on the\n - compiler to complain about missing enum values instead.\n + 2019-08-13) we have memset() the whole \"settings\" structure to -1 in\n + prepare_repo_settings(), and subsequently relied on the -1 value.\n +\n + Most of the fields did not need to be initialized to -1, and because\n + we were doing that we had the enum labels \"UNTRACKED_CACHE_UNSET\" and\n + \"FETCH_NEGOTIATION_UNSET\" purely to reflect the resulting state\n + created this memset() in prepare_repo_settings(). No other code used\n + or relied on them, more on that below.\n +\n + For the rest most of the subsequent \"are we -1, then read xyz\" can\n + simply be removed by re-arranging what we read first. E.g. when\n + setting the \"index.version\" setting we should have first read\n + \"feature.experimental\", so that it (and \"feature.manyfiles\") can\n + provide a default for our \"index.version\".\n +\n + Instead the code setting it, added when \"feature.manyFiles\"[1] was\n + created, was using the UPDATE_DEFAULT_BOOL() macro added in an earlier\n + commit[2]. That macro is now gone, since it was only needed for this\n + pattern of reading things in the wrong order.\n +\n + This also fixes an (admittedly obscure) logic error where we'd\n + conflate an explicit \"-1\" value in the config with our own earlier\n + memset() -1.\n +\n + We can also remove the UPDATE_DEFAULT_BOOL() wrapper added in\n + [3]. Using it is redundant to simply using the return value from\n + repo_config_get_bool(), which is non-zero if the provided key exists\n + in the config.\n +\n + Details on edge cases relating to the memset() to -1, continued from\n + \"more on that below\" above:\n +\n + * UNTRACKED_CACHE_KEEP:\n +\n + In [4] the \"unset\" and \"keep\" handling for core.untrackedCache was\n + consolidated. But it while we understand the \"keep\" value, we don't\n + handle it differently than the case of any other unknown value. So\n + we can remove UNTRACKED_CACHE_KEEP. It's not handled any\n + differently than UNTRACKED_CACHE_UNSET once we get past the config\n + parsing step.\n +\n + * FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:\n +\n + Since these two two enum fields added in [5] don't rely on the\n + memzero() setting them to \"-1\" anymore we don't have to provide\n + them with explicit values.\n +\n + 1. c6cc4c5afd2 (repo-settings: create feature.manyFiles setting,\n + 2019-08-13)\n + 2. 31b1de6a09b (commit-graph: turn on commit-graph by default,\n + 2019-08-13)\n + 3. 31b1de6a09b (commit-graph: turn on commit-graph by default,\n + 2019-08-13)\n + 4. ad0fb659993 (repo-settings: parse core.untrackedCache,\n + 2019-08-13)\n + 5. aaf633c2ad1 (repo-settings: create feature.experimental setting,\n + 2019-08-13)\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## cache.h ##\n -@@ cache.h: int update_server_info(int);\n - const char *get_log_output_encoding(void);\n - const char *get_commit_output_encoding(void);\n - \n --/*\n -- * This is a hack for test programs like test-dump-untracked-cache to\n -- * ensure that they do not modify the untracked cache when reading it.\n -- * Do not use it otherwise!\n -- */\n --extern int ignore_untracked_cache_config;\n --\n - int committer_ident_sufficiently_given(void);\n - int author_ident_sufficiently_given(void);\n - \n -\n - ## environment.c ##\n -@@ environment.c: int auto_comment_line_char;\n - /* Parallel index stat data preload? */\n - int core_preload_index = 1;\n - \n --/*\n -- * This is a hack for test programs like test-dump-untracked-cache to\n -- * ensure that they do not modify the untracked cache when reading it.\n -- * Do not use it otherwise!\n -- */\n --int ignore_untracked_cache_config;\n --\n - /* This is set by setup_git_dir_gently() and/or git_default_config() */\n - char *git_work_tree_cfg;\n - \n -\n ## fetch-negotiator.c ##\n -@@\n - void fetch_negotiator_init(struct repository *r,\n - \t\t\t struct fetch_negotiator *negotiator)\n - {\n -+\tenum fetch_negotiation_setting setting;\n - \tprepare_repo_settings(r);\n --\tswitch(r->settings.fetch_negotiation_algorithm) {\n -+\tsetting = r->settings.fetch_negotiation_algorithm;\n -+\n -+\tswitch (setting) {\n - \tcase FETCH_NEGOTIATION_SKIPPING:\n - \t\tskipping_negotiator_init(negotiator);\n - \t\treturn;\n @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \n \tcase FETCH_NEGOTIATION_DEFAULT:\n --\tdefault:\n +-\tcase FETCH_NEGOTIATION_UNSET:\n +-\tcase FETCH_NEGOTIATION_NONE:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n \t}\n \n ## read-cache.c ##\n -@@ read-cache.c: static void check_ce_order(struct index_state *istate)\n - static void tweak_untracked_cache(struct index_state *istate)\n - {\n - \tstruct repository *r = the_repository;\n -+\tenum untracked_cache_setting setting;\n - \n - \tprepare_repo_settings(r);\n -+\tsetting = r->settings.core_untracked_cache;\n - \n --\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_REMOVE) {\n -+\tswitch (setting) {\n -+\tcase UNTRACKED_CACHE_REMOVE:\n - \t\tremove_untracked_cache(istate);\n --\t\treturn;\n --\t}\n --\n --\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE)\n -+\t\tbreak;\n -+\tcase UNTRACKED_CACHE_WRITE:\n +@@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)\n \t\tadd_untracked_cache(istate);\n -+\t\tbreak;\n -+\tcase UNTRACKED_CACHE_UNSET:\n + \t\tbreak;\n + \tcase UNTRACKED_CACHE_UNSET:\n +-\tcase UNTRACKED_CACHE_KEEP:\n +\t\t/* This includes core.untrackedCache=keep */\n -+\t\tbreak;\n -+\t}\n -+\treturn;\n - }\n - \n - static void tweak_split_index(struct index_state *istate)\n + \t\tbreak;\n + \t}\n + \treturn;\n \n ## repo-settings.c ##\n @@\n @@ repo-settings.c\n #include \"midx.h\"\n \n -#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)\n -+static void repo_config_get_bool_or(struct repository *r, const char *key,\n -+\t\t\t\t int *dest, int def)\n ++static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n ++\t\t\t int def)\n +{\n +\tif (repo_config_get_bool(r, key, dest))\n +\t\t*dest = def;\n @@ repo-settings.c\n \n void prepare_repo_settings(struct repository *r)\n {\n --\tint value;\n +\tint experimental;\n -+\tint intval;\n + \tint value;\n \tchar *strval;\n +\tint manyfiles;\n \n - \tif (r->settings.initialized)\n +-\tif (r->settings.initialized)\n ++\tif (r->settings.initialized++)\n \t\treturn;\n \n \t/* Defaults */\n @@ repo-settings.c\n +\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n +\n +\t/* Booleans config or default, cascades to other settings */\n -+\trepo_config_get_bool_or(r, \"feature.manyfiles\", &manyfiles, 0);\n -+\trepo_config_get_bool_or(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 ++\trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n ++\trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n \n -\tif (!repo_config_get_bool(r, \"core.commitgraph\", &value))\n -\t\tr->settings.core_commit_graph = value;\n @@ repo-settings.c\n -\tUPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);\n -\tUPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);\n -\tUPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);\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 ++\n +\t/* Boolean config or default, does not cascade (simple) */\n -+\trepo_config_get_bool_or(r, \"core.commitgraph\",\n -+\t\t\t\t&r->settings.core_commit_graph, 1);\n -+\trepo_config_get_bool_or(r, \"commitgraph.readchangedpaths\",\n -+\t\t\t\t&r->settings.commit_graph_read_changed_paths, 1);\n -+\trepo_config_get_bool_or(r, \"gc.writecommitgraph\",\n -+\t\t\t\t&r->settings.gc_write_commit_graph, 1);\n -+\trepo_config_get_bool_or(r, \"fetch.writecommitgraph\",\n -+\t\t\t\t&r->settings.fetch_write_commit_graph, 0);\n -+\trepo_config_get_bool_or(r, \"pack.usesparse\",\n -+\t\t\t\t&r->settings.pack_use_sparse, 1);\n -+\trepo_config_get_bool_or(r, \"core.multipackindex\",\n -+\t\t\t\t&r->settings.core_multi_pack_index, 1);\n - \n --\tif (!repo_config_get_int(r, \"index.version\", &value))\n --\t\tr->settings.index_version = value;\n --\tif (!repo_config_get_maybe_bool(r, \"core.untrackedcache\", &value)) {\n --\t\tif (value == 0)\n --\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_REMOVE;\n --\t\telse\n --\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n --\t} else if (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n --\t\tif (!strcasecmp(strval, \"keep\"))\n --\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\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 ++\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 other cases.\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 @@ repo-settings.c\n +\t/*\n +\t * Non-boolean config\n +\t */\n -+\tif (!repo_config_get_int(r, \"index.version\", &intval))\n -+\t\tr->settings.index_version = intval;\n -+\n + \tif (!repo_config_get_int(r, \"index.version\", &value))\n + \t\tr->settings.index_version = value;\n +-\tif (!repo_config_get_maybe_bool(r, \"core.untrackedcache\", &value)) {\n +-\t\tif (value == 0)\n +-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_REMOVE;\n +-\t\telse\n +-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n +-\t} else if (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n +-\t\tif (!strcasecmp(strval, \"keep\"))\n +-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n + \n +\tif (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n -+\t\tint maybe_bool = git_parse_maybe_bool(strval);\n -+\t\tif (maybe_bool == -1) {\n -+\t\t\t/*\n -+\t\t\t * Set to \"keep\", or some other non-boolean\n -+\t\t\t * value. In either case we do nothing but\n -+\t\t\t * keep UNTRACKED_CACHE_UNSET.\n -+\t\t\t */\n -+\t\t} else {\n -+\t\t\tr->settings.core_untracked_cache = maybe_bool\n -+\t\t\t\t? UNTRACKED_CACHE_WRITE\n -+\t\t\t\t: UNTRACKED_CACHE_REMOVE;\n -+\t\t}\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 UNTRACKED_CACHE_UNSET.\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 @@ repo-settings.c: void prepare_repo_settings(struct repository *r)\n -\t\telse\n -\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n --\n + \n -\tif (!repo_config_get_bool(r, \"pack.usesparse\", &value))\n -\t\tr->settings.pack_use_sparse = value;\n -\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n @@ repo-settings.c: void prepare_repo_settings(struct repository *r)\n -\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n -\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n -\n --\t/* Hack for test programs like test-dump-untracked-cache */\n --\tif (ignore_untracked_cache_config)\n --\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n --\telse\n --\t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n --\n +-\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n -\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);\n - }\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 ## repository.h ##\n -@@ repository.h: struct raw_object_store;\n - struct submodule_cache;\n +@@ repository.h: struct submodule_cache;\n + struct promisor_remote_config;\n \n enum untracked_cache_setting {\n -\tUNTRACKED_CACHE_UNSET = -1,\n @@ repository.h: struct raw_object_store;\n };\n \n struct repo_settings {\n -\n - ## t/helper/test-dump-untracked-cache.c ##\n -@@ t/helper/test-dump-untracked-cache.c: int cmd__dump_untracked_cache(int ac, const char **av)\n - \tstruct untracked_cache *uc;\n - \tstruct strbuf base = STRBUF_INIT;\n - \n --\t/* Hack to avoid modifying the untracked cache when we read it */\n --\tignore_untracked_cache_config = 1;\n -+\t/* Set core.untrackedCache=keep before setup_git_directory() */\n -+\tsetenv(\"GIT_CONFIG_COUNT\", \"1\", 1);\n -+\tsetenv(\"GIT_CONFIG_KEY_0\", \"core.untrackedCache\", 1);\n -+\tsetenv(\"GIT_CONFIG_VALUE_0\", \"keep\", 1);\n - \n - \tsetup_git_directory();\n - \tif (read_cache() < 0)\n-: ----------- > 5: 0b5f213a639 repository.h: don't use a mix of int and bitfields\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436139","messageId":"patch-v2-2.5-57290842f0f-20210916T182918Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com","subject":"[PATCH v2 2/5] environment.c: remove test-specific \"ignore_untracked...\" variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T18:30:33Z","receivedAt":"2021-09-16T18:59:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Instead of the global ignore_untracked_cache_config variable added in\ndae6c322fa1 (test-dump-untracked-cache: don't modify the untracked\ncache, 2016-01-27) we can make use of the new facility to set config\nvia environment variables added in d8d77153eaf (config: allow\nspecifying config entries via envvar pairs, 2021-01-12).\n\nIt's arguably a bit hacky to use setenv() and getenv() to pass\nmessages between the same program, but since the test helpers are not\nthe main intended audience of repo-settings.c I think it's better than\nhardcoding the test-only special-case in prepare_repo_settings().\n\nThis uses the xsetenv() wrapper added in the preceding commit, if we\ndon't set these in the environment we'll fail in\nt7063-status-untracked-cache.sh, but let's fail earlier anyway if that\nwere to happen.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n cache.h | 7 -------\n environment.c | 7 -------\n repo-settings.c | 7 +------\n t/helper/test-dump-untracked-cache.c | 6 ++++--\n 4 files changed, 5 insertions(+), 22 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex d23de693680..8e60fdd2a12 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1719,13 +1719,6 @@ int update_server_info(int);\n const char *get_log_output_encoding(void);\n const char *get_commit_output_encoding(void);\n \n-/*\n- * This is a hack for test programs like test-dump-untracked-cache to\n- * ensure that they do not modify the untracked cache when reading it.\n- * Do not use it otherwise!\n- */\n-extern int ignore_untracked_cache_config;\n-\n int committer_ident_sufficiently_given(void);\n int author_ident_sufficiently_given(void);\n \ndiff --git a/environment.c b/environment.c\nindex 7d8a949285c..d73dd0c42f7 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -96,13 +96,6 @@ int auto_comment_line_char;\n /* Parallel index stat data preload? */\n int core_preload_index = 1;\n \n-/*\n- * This is a hack for test programs like test-dump-untracked-cache to\n- * ensure that they do not modify the untracked cache when reading it.\n- * Do not use it otherwise!\n- */\n-int ignore_untracked_cache_config;\n-\n /* This is set by setup_git_dir_gently() and/or git_default_config() */\n char *git_work_tree_cfg;\n \ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0cfe8b787db..b0df8b93b86 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -70,12 +70,7 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n \n-\t/* Hack for test programs like test-dump-untracked-cache */\n-\tif (ignore_untracked_cache_config)\n-\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n-\telse\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n-\n+\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n \tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);\n \n \t/*\ndiff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c\nindex cf0f2c7228e..99010614f6d 100644\n--- a/t/helper/test-dump-untracked-cache.c\n+++ b/t/helper/test-dump-untracked-cache.c\n@@ -45,8 +45,10 @@ int cmd__dump_untracked_cache(int ac, const char **av)\n \tstruct untracked_cache *uc;\n \tstruct strbuf base = STRBUF_INIT;\n \n-\t/* Hack to avoid modifying the untracked cache when we read it */\n-\tignore_untracked_cache_config = 1;\n+\t/* Set core.untrackedCache=keep before setup_git_directory() */\n+\txsetenv(\"GIT_CONFIG_COUNT\", \"1\", 1);\n+\txsetenv(\"GIT_CONFIG_KEY_0\", \"core.untrackedCache\", 1);\n+\txsetenv(\"GIT_CONFIG_VALUE_0\", \"keep\", 1);\n \n \tsetup_git_directory();\n \tif (read_cache() < 0)\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436140","messageId":"patch-v2-3.5-9f1bb0496ed-20210916T182918Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com","subject":"[PATCH v2 3/5] read-cache & fetch-negotiator: check \"enum\" values in switch()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T18:30:34Z","receivedAt":"2021-09-16T19:00:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change tweak_untracked_cache() in \"read-cache.c\" to use a switch() to\nhave the compiler assert that we checked all possible values in the\n\"enum untracked_cache_setting\" type, and likewise remove the \"default\"\ncase in fetch_negotiator_init() in favor of checking for\n\"FETCH_NEGOTIATION_UNSET\" and \"FETCH_NEGOTIATION_NONE\".\n\nSee ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\nfor when the \"unset\" and \"keep\" handling for core.untrackedCache was\nconsolidated, and aaf633c2ad1 (repo-settings: create\nfeature.experimental setting, 2019-08-13) for the addition of the\n\"default\" pattern in \"fetch-negotiator.c\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fetch-negotiator.c | 3 ++-\n read-cache.c | 15 ++++++++++-----\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 57ed5784e14..e7b5878be7c 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -19,7 +19,8 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \n \tcase FETCH_NEGOTIATION_DEFAULT:\n-\tdefault:\n+\tcase FETCH_NEGOTIATION_UNSET:\n+\tcase FETCH_NEGOTIATION_NONE:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n \t}\ndiff --git a/read-cache.c b/read-cache.c\nindex 9048ef9e905..9dd84d69f00 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1944,13 +1944,18 @@ static void tweak_untracked_cache(struct index_state *istate)\n \n \tprepare_repo_settings(r);\n \n-\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_REMOVE) {\n+\tswitch (r->settings.core_untracked_cache) {\n+\tcase UNTRACKED_CACHE_REMOVE:\n \t\tremove_untracked_cache(istate);\n-\t\treturn;\n-\t}\n-\n-\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE)\n+\t\tbreak;\n+\tcase UNTRACKED_CACHE_WRITE:\n \t\tadd_untracked_cache(istate);\n+\t\tbreak;\n+\tcase UNTRACKED_CACHE_UNSET:\n+\tcase UNTRACKED_CACHE_KEEP:\n+\t\tbreak;\n+\t}\n+\treturn;\n }\n \n static void tweak_split_index(struct index_state *istate)\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436141","messageId":"patch-v2-5.5-0b5f213a639-20210916T182918Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com","subject":"[PATCH v2 5/5] repository.h: don't use a mix of int and bitfields","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T18:30:36Z","receivedAt":"2021-09-16T19:00:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the bitfield added in 58300f47432 (sparse-index: add\nindex.sparse config option, 2021-03-30) and 3964fc2aae7 (sparse-index:\nadd guard to ensure full index, 2021-03-30) to just use an \"int\"\nboolean instead.\n\nIt might be smart to optimize the space here in the future, but by\nconsistently using an \"int\" we can take its address and pass it to\nrepo_cfg_bool(), and therefore don't need to handle \"sparse_index\" as\na special-case when reading the \"index.sparse\" setting.\n\nThere's no corresponding config for \"command_requires_full_index\", but\nlet's change it too for consistency and to prevent future bugs\ncreeping in due to one of these being \"unsigned\".\n\nUsing \"int\" consistently also prevents subtle bugs or undesired\ncontrol flow creeping in here. Before the preceding commit the\ninitialization of \"command_requires_full_index\" in\nprepare_repo_settings() did nothing, i.e. this:\n\n r->settings.command_requires_full_index = 1\n\nWas redundant to the earlier memset() to -1. Likewise for\n\"sparse_index\" added in 58300f47432 (sparse-index: add index.sparse\nconfig option, 2021-03-30) the code and comment added there was\nmisleading, we weren't initializing it to off, but re-initializing it\nfrom \"1\" to \"0\", and then finally checking the config, and perhaps\nsetting it to \"1\" again. I.e. we could have applied this patch before\nthe preceding commit:\n\n\t+\tassert(r->settings.command_requires_full_index == 1);\n\t \tr->settings.command_requires_full_index = 1;\n\n\t \t/*\n\t \t * Initialize this as off.\n\t \t */\n\t+\tassert(r->settings.sparse_index == 1);\n\t \tr->settings.sparse_index = 0;\n\t \tif (!repo_config_get_bool(r, \"index.sparse\", &value) && value)\n\t \t\tr->settings.sparse_index = 1;\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n repo-settings.c | 8 +-------\n repository.h | 5 ++---\n 2 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 6355bc67517..5f5bdddb1b2 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -45,6 +45,7 @@ void prepare_repo_settings(struct repository *r)\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 \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\n@@ -90,11 +91,4 @@ void prepare_repo_settings(struct repository *r)\n \t * removed.\n \t */\n \tr->settings.command_requires_full_index = 1;\n-\n-\t/*\n-\t * Initialize this as off.\n-\t */\n-\tr->settings.sparse_index = 0;\n-\tif (!repo_config_get_bool(r, \"index.sparse\", &value) && value)\n-\t\tr->settings.sparse_index = 1;\n }\ndiff --git a/repository.h b/repository.h\nindex c03fe8da8d0..5f8a48b8a5c 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -31,6 +31,8 @@ struct repo_settings {\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n \tint fetch_write_commit_graph;\n+\tint command_requires_full_index;\n+\tint sparse_index;\n \n \tint index_version;\n \tenum untracked_cache_setting core_untracked_cache;\n@@ -39,9 +41,6 @@ struct repo_settings {\n \tenum fetch_negotiation_setting fetch_negotiation_algorithm;\n \n \tint core_multi_pack_index;\n-\n-\tunsigned command_requires_full_index:1,\n-\t\t sparse_index:1;\n };\n \n struct repository {\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436142","messageId":"patch-v2-4.5-b28ad2b2607-20210916T182918Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com","subject":"[PATCH v2 4/5] repo-settings.c: simplify the setup","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T18:30:35Z","receivedAt":"2021-09-16T19:00:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Simplify the setup code in repo-settings.c in various ways, making the\ncode shorter, easier to read, and requiring fewer hacks to do the same\nthing as it did before:\n\nSince 7211b9e7534 (repo-settings: consolidate some config settings,\n2019-08-13) we have memset() the whole \"settings\" structure to -1 in\nprepare_repo_settings(), and subsequently relied on the -1 value.\n\nMost of the fields did not need to be initialized to -1, and because\nwe were doing that we had the enum labels \"UNTRACKED_CACHE_UNSET\" and\n\"FETCH_NEGOTIATION_UNSET\" purely to reflect the resulting state\ncreated this memset() in prepare_repo_settings(). No other code used\nor relied on them, more on that below.\n\nFor the rest most of the subsequent \"are we -1, then read xyz\" can\nsimply be removed by re-arranging what we read first. E.g. when\nsetting the \"index.version\" setting we should have first read\n\"feature.experimental\", so that it (and \"feature.manyfiles\") can\nprovide a default for our \"index.version\".\n\nInstead the code setting it, added when \"feature.manyFiles\"[1] was\ncreated, was using the UPDATE_DEFAULT_BOOL() macro added in an earlier\ncommit[2]. That macro is now gone, since it was only needed for this\npattern of reading things in the wrong order.\n\nThis also fixes an (admittedly obscure) logic error where we'd\nconflate an explicit \"-1\" value in the config with our own earlier\nmemset() -1.\n\nWe can also remove the UPDATE_DEFAULT_BOOL() wrapper added in\n[3]. Using it is redundant to simply using the return value from\nrepo_config_get_bool(), which is non-zero if the provided key exists\nin the config.\n\nDetails on edge cases relating to the memset() to -1, continued from\n\"more on that below\" above:\n\n * UNTRACKED_CACHE_KEEP:\n\n In [4] the \"unset\" and \"keep\" handling for core.untrackedCache was\n consolidated. But it while we understand the \"keep\" value, we don't\n handle it differently than the case of any other unknown value. So\n we can remove UNTRACKED_CACHE_KEEP. It's not handled any\n differently than UNTRACKED_CACHE_UNSET once we get past the config\n parsing step.\n\n * FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:\n\n Since these two two enum fields added in [5] don't rely on the\n memzero() setting them to \"-1\" anymore we don't have to provide\n them with explicit values.\n\n1. c6cc4c5afd2 (repo-settings: create feature.manyFiles setting,\n 2019-08-13)\n2. 31b1de6a09b (commit-graph: turn on commit-graph by default,\n 2019-08-13)\n3. 31b1de6a09b (commit-graph: turn on commit-graph by default,\n 2019-08-13)\n4. ad0fb659993 (repo-settings: parse core.untrackedCache,\n 2019-08-13)\n5. aaf633c2ad1 (repo-settings: create feature.experimental setting,\n 2019-08-13)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fetch-negotiator.c | 2 -\n read-cache.c | 2 +-\n repo-settings.c | 102 +++++++++++++++++++++++++--------------------\n repository.h | 15 +++----\n 4 files changed, 63 insertions(+), 58 deletions(-)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex e7b5878be7c..273390229fe 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -19,8 +19,6 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \n \tcase FETCH_NEGOTIATION_DEFAULT:\n-\tcase FETCH_NEGOTIATION_UNSET:\n-\tcase FETCH_NEGOTIATION_NONE:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n \t}\ndiff --git a/read-cache.c b/read-cache.c\nindex 9dd84d69f00..9bbbbbacabe 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1952,7 +1952,7 @@ static void tweak_untracked_cache(struct index_state *istate)\n \t\tadd_untracked_cache(istate);\n \t\tbreak;\n \tcase UNTRACKED_CACHE_UNSET:\n-\tcase UNTRACKED_CACHE_KEEP:\n+\t\t/* This includes core.untrackedCache=keep */\n \t\tbreak;\n \t}\n \treturn;\ndiff --git a/repo-settings.c b/repo-settings.c\nindex b0df8b93b86..6355bc67517 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -3,40 +3,76 @@\n #include \"repository.h\"\n #include \"midx.h\"\n \n-#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)\n+static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n+\t\t\t int def)\n+{\n+\tif (repo_config_get_bool(r, key, dest))\n+\t\t*dest = def;\n+}\n \n void prepare_repo_settings(struct repository *r)\n {\n+\tint experimental;\n \tint value;\n \tchar *strval;\n+\tint manyfiles;\n \n-\tif (r->settings.initialized)\n+\tif (r->settings.initialized++)\n \t\treturn;\n \n \t/* Defaults */\n-\tmemset(&r->settings, -1, sizeof(r->settings));\n+\tr->settings.index_version = -1;\n+\tr->settings.core_untracked_cache = UNTRACKED_CACHE_UNSET;\n+\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\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 \n-\tif (!repo_config_get_bool(r, \"core.commitgraph\", &value))\n-\t\tr->settings.core_commit_graph = value;\n-\tif (!repo_config_get_bool(r, \"commitgraph.readchangedpaths\", &value))\n-\t\tr->settings.commit_graph_read_changed_paths = value;\n-\tif (!repo_config_get_bool(r, \"gc.writecommitgraph\", &value))\n-\t\tr->settings.gc_write_commit_graph = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);\n-\tUPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);\n-\tUPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);\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+\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+\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/*\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-\tif (!repo_config_get_maybe_bool(r, \"core.untrackedcache\", &value)) {\n-\t\tif (value == 0)\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_REMOVE;\n-\t\telse\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n-\t} else if (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n-\t\tif (!strcasecmp(strval, \"keep\"))\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\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 UNTRACKED_CACHE_UNSET.\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@@ -45,34 +81,8 @@ void prepare_repo_settings(struct repository *r)\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\n-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n \n-\tif (!repo_config_get_bool(r, \"pack.usesparse\", &value))\n-\t\tr->settings.pack_use_sparse = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n-\n-\tvalue = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);\n-\tif (value || !repo_config_get_bool(r, \"core.multipackindex\", &value))\n-\t\tr->settings.core_multi_pack_index = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);\n-\n-\tif (!repo_config_get_bool(r, \"feature.manyfiles\", &value) && value) {\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n-\t}\n-\n-\tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n-\t\tr->settings.fetch_write_commit_graph = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0);\n-\n-\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n-\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n-\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);\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\ndiff --git a/repository.h b/repository.h\nindex 3740c93bc0f..c03fe8da8d0 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -13,18 +13,15 @@ struct submodule_cache;\n struct promisor_remote_config;\n \n enum untracked_cache_setting {\n-\tUNTRACKED_CACHE_UNSET = -1,\n-\tUNTRACKED_CACHE_REMOVE = 0,\n-\tUNTRACKED_CACHE_KEEP = 1,\n-\tUNTRACKED_CACHE_WRITE = 2\n+\tUNTRACKED_CACHE_UNSET,\n+\tUNTRACKED_CACHE_REMOVE,\n+\tUNTRACKED_CACHE_WRITE,\n };\n \n enum fetch_negotiation_setting {\n-\tFETCH_NEGOTIATION_UNSET = -1,\n-\tFETCH_NEGOTIATION_NONE = 0,\n-\tFETCH_NEGOTIATION_DEFAULT = 1,\n-\tFETCH_NEGOTIATION_SKIPPING = 2,\n-\tFETCH_NEGOTIATION_NOOP = 3,\n+\tFETCH_NEGOTIATION_DEFAULT,\n+\tFETCH_NEGOTIATION_SKIPPING,\n+\tFETCH_NEGOTIATION_NOOP,\n };\n \n struct repo_settings {\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436235","messageId":"xmqq35q3i1hi.fsf@gitster.g","threadId":"56517","inReplyTo":"patch-v2-1.5-49706b26642-20210916T182918Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-17T16:57:29Z","receivedAt":"2021-09-17T16:57:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> -\tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n> -\t\tdie(_(\"could not set GIT_DIR to '%s'\"), path);\n> +\txsetenv(GIT_DIR_ENVIRONMENT, path, 1);\n> ...\n> +int xsetenv(const char *name, const char *value, int overwrite)\n> +{\n> +\tif (!name)\n> +\t\tdie(\"xsetenv() got a NULL name, setenv() would return EINVAL\");\n> +\tif (setenv(name, value, overwrite))\n> +\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name, value, overwrite);\n> +\treturn 0;\n> +}\n> +\n> +int xunsetenv(const char *name)\n> +{\n> +\tif (!name)\n> +\t\tdie(\"xunsetenv() got a NULL name, xunsetenv() would return EINVAL\");\n> +\tif (!unsetenv(name))\n> +\t\tdie_errno(\"unsetenv(%s) failed\", name);\n> +\treturn 0;\n> +}\n\nNone of the existing callers have the \"NULL name gets shown a\nspecial error\". If we would get EINVAL and die anyway, there is any\nneed to add such an extra check that is always performed, no?\n\nAs there seems no justification for it in the proposed log message,\nI'd have to say this is another \"I'd do so while we are at it even\nthough it has no reason to be there to support this topic\" change.\n\nWith explanation, perhaps these addtions would make sense. If you\nwanted to protect the printf-like die_errno() from name==NULL, the\ncost of the check should be borne by the error codepath.\n\nIOW,\n\n if (!unsetenv(name))\n\tdie_errno(_(\"unsetenv(%s) failed\"), name ? name : \"\");\n\nor something along that line, perhaps? That won't need extra\njustification, as we are not adding a mysterious feature that gives\na NULL name any special error status.\n\n\n"},{"id":"436238","messageId":"xmqqy27vglwx.fsf@gitster.g","threadId":"56517","inReplyTo":"patch-v2-2.5-57290842f0f-20210916T182918Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] environment.c: remove test-specific \"ignore_untracked...\" variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-17T17:19:10Z","receivedAt":"2021-09-17T17:19:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c\n> index cf0f2c7228e..99010614f6d 100644\n> --- a/t/helper/test-dump-untracked-cache.c\n> +++ b/t/helper/test-dump-untracked-cache.c\n> @@ -45,8 +45,10 @@ int cmd__dump_untracked_cache(int ac, const char **av)\n> \tstruct untracked_cache *uc;\n> \tstruct strbuf base = STRBUF_INIT;\n> \n> -\t/* Hack to avoid modifying the untracked cache when we read it */\n> -\tignore_untracked_cache_config = 1;\n> +\t/* Set core.untrackedCache=keep before setup_git_directory() */\n> +\txsetenv(\"GIT_CONFIG_COUNT\", \"1\", 1);\n> +\txsetenv(\"GIT_CONFIG_KEY_0\", \"core.untrackedCache\", 1);\n> +\txsetenv(\"GIT_CONFIG_VALUE_0\", \"keep\", 1);\n> \n> \tsetup_git_directory();\n> \tif (read_cache() < 0)\n\nIf any parent process were using the GIT_CONFIG_* and the\nGIT_CONFIG_PARAMETERS mechanism to pass one-shot config setting down\nto a git subprocess, this obviously will break, but I would assume\nthat:\n\n (1) this test helper itself is not interested in being told to\n pretend any configuration other than what is found in the files\n are in effect, and\n\n (2) this test helper will not spawn git subprocesses that should\n honor the one-shot config setting whoever started the test\n helper wanted them to use.\n\nSo this should be an OK price to pay for being able to remove the\ntest-only code from the core part.\n\nLooking good.\n"},{"id":"436248","messageId":"87czp7dn2u.fsf@evledraar.gmail.com","threadId":"56517","inReplyTo":"xmqq35q3i1hi.fsf@gitster.g","subject":"Re: [PATCH v2 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-17T19:18:50Z","receivedAt":"2021-09-17T19:22:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 17 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> -\tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n>> -\t\tdie(_(\"could not set GIT_DIR to '%s'\"), path);\n>> +\txsetenv(GIT_DIR_ENVIRONMENT, path, 1);\n>> ...\n>> +int xsetenv(const char *name, const char *value, int overwrite)\n>> +{\n>> +\tif (!name)\n>> +\t\tdie(\"xsetenv() got a NULL name, setenv() would return EINVAL\");\n>> +\tif (setenv(name, value, overwrite))\n>> +\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name, value, overwrite);\n>> +\treturn 0;\n>> +}\n>> +\n>> +int xunsetenv(const char *name)\n>> +{\n>> +\tif (!name)\n>> +\t\tdie(\"xunsetenv() got a NULL name, xunsetenv() would return EINVAL\");\n>> +\tif (!unsetenv(name))\n>> +\t\tdie_errno(\"unsetenv(%s) failed\", name);\n>> +\treturn 0;\n>> +}\n>\n> None of the existing callers have the \"NULL name gets shown a\n> special error\". If we would get EINVAL and die anyway, there is any\n> need to add such an extra check that is always performed, no?\n>\n> As there seems no justification for it in the proposed log message,\n> I'd have to say this is another \"I'd do so while we are at it even\n> though it has no reason to be there to support this topic\" change.\n>\n> With explanation, perhaps these addtions would make sense. If you\n> wanted to protect the printf-like die_errno() from name==NULL, the\n> cost of the check should be borne by the error codepath.\n>\n> IOW,\n>\n> if (!unsetenv(name))\n> \tdie_errno(_(\"unsetenv(%s) failed\"), name ? name : \"\");\n>\n> or something along that line, perhaps? That won't need extra\n> justification, as we are not adding a mysterious feature that gives\n> a NULL name any special error status.\n\nSure, I didn't think much about it when writing it.\n\nI'd think skipping the translation would be fine here, but sure, will\ninclude it. On second thought just a:\n\n die_errno(_(\"unsetenv(%s) failed\"), name);\n\nShould be fine. I.e. this is an internal-only function, we're\nexceedingly unlikely to end up with a xsetenv(NULL, ...).\n\nEven if we did I'd think the undefined behavior is OK here. In practice\nmodern C libraries are forgiving about it (e.g. glibc formatting it as\n\"(null)\"), and if not we were about to die anyway...\n\nBut unless you explicitly Ack that undefined behavior bit I'll use your\nversion in a re-roll. Thanks.\n"},{"id":"436252","messageId":"xmqqsfy3gfua.fsf@gitster.g","threadId":"56517","inReplyTo":"patch-v2-3.5-9f1bb0496ed-20210916T182918Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/5] read-cache & fetch-negotiator: check \"enum\" values in switch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-17T19:30:21Z","receivedAt":"2021-09-17T19:30:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change tweak_untracked_cache() in \"read-cache.c\" to use a switch() to\n> have the compiler assert that we checked all possible values in the\n> \"enum untracked_cache_setting\" type, and likewise remove the \"default\"\n> case in fetch_negotiator_init() in favor of checking for\n> \"FETCH_NEGOTIATION_UNSET\" and \"FETCH_NEGOTIATION_NONE\".\n>\n> See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\n> for when the \"unset\" and \"keep\" handling for core.untrackedCache was\n> consolidated, and aaf633c2ad1 (repo-settings: create\n> feature.experimental setting, 2019-08-13) for the addition of the\n> \"default\" pattern in \"fetch-negotiator.c\".\n\nCovering all possibility is good, but ...\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> fetch-negotiator.c | 3 ++-\n> read-cache.c | 15 ++++++++++-----\n> 2 files changed, 12 insertions(+), 6 deletions(-)\n>\n> diff --git a/fetch-negotiator.c b/fetch-negotiator.c\n> index 57ed5784e14..e7b5878be7c 100644\n> --- a/fetch-negotiator.c\n> +++ b/fetch-negotiator.c\n> @@ -19,7 +19,8 @@ void fetch_negotiator_init(struct repository *r,\n> \t\treturn;\n> \n> \tcase FETCH_NEGOTIATION_DEFAULT:\n> -\tdefault:\n> +\tcase FETCH_NEGOTIATION_UNSET:\n> +\tcase FETCH_NEGOTIATION_NONE:\n> \t\tdefault_negotiator_init(negotiator);\n> \t\treturn;\n> \t}\n> diff --git a/read-cache.c b/read-cache.c\n> index 9048ef9e905..9dd84d69f00 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1944,13 +1944,18 @@ static void tweak_untracked_cache(struct index_state *istate)\n> \n> \tprepare_repo_settings(r);\n> \n> -\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_REMOVE) {\n> +\tswitch (r->settings.core_untracked_cache) {\n> +\tcase UNTRACKED_CACHE_REMOVE:\n> \t\tremove_untracked_cache(istate);\n> -\t\treturn;\n> -\t}\n> -\n> -\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE)\n> +\t\tbreak;\n> +\tcase UNTRACKED_CACHE_WRITE:\n> \t\tadd_untracked_cache(istate);\n> +\t\tbreak;\n> +\tcase UNTRACKED_CACHE_UNSET:\n> +\tcase UNTRACKED_CACHE_KEEP:\n> +\t\tbreak;\n> +\t}\n\n... this change makes me wonder if\n\n\tdefault:\n\t\tBUG(...);\n\nmight have been more appropriate? Are we sure these the flow will\nreach here with these two values?\n\n> +\treturn;\n> }\n\nI do not see why we want to add a no-op return that wasn't there in\nthe original. Perhaps later, but definitely not as a part of this\nchange.\n\n> \n> static void tweak_split_index(struct index_state *istate)\n"},{"id":"436357","messageId":"cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com","subject":"[PATCH v3 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T08:47:14Z","receivedAt":"2021-09-19T08:47:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This simplification of repo-settings.c addresses Junio's feedback,\nchanges:\n\n * Get rid of thename=NULL checkfrom the main codepath of\n xsetenv()/xunsetenv() (but don't feed NULL to %s).\n * Expanded commit message of 2/5 to discuss the safety/desire of\n overriding GIT_CONFIG_COUNT.\n * Marked unreachable code in 3/5 (later removed in 4/5) with a BUG().\n * I kept the UNTRACKED_CACHE_KEEP value instead of\n UNTRACKED_CACHE_UNSET for indicating the default.\n * Don't add a \"return\" to a void function.\n\nFor v2 see: https://lore.kernel.org/git/cover-v2-0.5-00000000000-20210916T182918Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (5):\n wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c\n environment.c: remove test-specific \"ignore_untracked...\" variable\n read-cache & fetch-negotiator: check \"enum\" values in switch()\n repo-settings.c: simplify the setup\n repository.h: don't use a mix of int and bitfields\n\n cache.h | 7 --\n environment.c | 10 +--\n fetch-negotiator.c | 1 -\n git-compat-util.h | 2 +\n read-cache.c | 19 +++--\n repo-settings.c | 115 +++++++++++++--------------\n repository.h | 20 ++---\n t/helper/test-dump-untracked-cache.c | 6 +-\n wrapper.c | 15 ++++\n 9 files changed, 101 insertions(+), 94 deletions(-)\n\nRange-diff against v2:\n1: 49706b26642 ! 1: 4b320edc933 wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c\n @@ Commit message\n the C library function signatures, including for any future code that\n expects a pointer to a setenv()-like function.\n \n + I think it would be OK skip the NULL check of the \"name\" here for the\n + calls to die_errno(). Almost all of our setenv() callers are taking a\n + constant string hardcoded in the source as the first argument, and for\n + the rest we can probably assume they've done the NULL check\n + themselves. Even if they didn't, modern C libraries are forgiving\n + about it (e.g. glibc formatting it as \"(null)\"), on those that aren't,\n + well, we were about to die anyway. But let's include the check anyway\n + for good measure.\n +\n 1. https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n @@ wrapper.c: void *xcalloc(size_t nmemb, size_t size)\n \n +int xsetenv(const char *name, const char *value, int overwrite)\n +{\n -+\tif (!name)\n -+\t\tdie(\"xsetenv() got a NULL name, setenv() would return EINVAL\");\n +\tif (setenv(name, value, overwrite))\n -+\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name, value, overwrite);\n ++\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name ? name : \"(null)\",\n ++\t\t\t value, overwrite);\n +\treturn 0;\n +}\n +\n +int xunsetenv(const char *name)\n +{\n -+\tif (!name)\n -+\t\tdie(\"xunsetenv() got a NULL name, xunsetenv() would return EINVAL\");\n +\tif (!unsetenv(name))\n -+\t\tdie_errno(\"unsetenv(%s) failed\", name);\n ++\t\tdie_errno(\"unsetenv(%s) failed\", name ? name : \"(null)\");\n +\treturn 0;\n +}\n +\n2: 57290842f0f ! 2: ece340af764 environment.c: remove test-specific \"ignore_untracked...\" variable\n @@ Commit message\n t7063-status-untracked-cache.sh, but let's fail earlier anyway if that\n were to happen.\n \n + This breaks any parent process that's potentially using the\n + GIT_CONFIG_* and GIT_CONFIG_PARAMETERS mechanism to pass one-shot\n + config setting down to a git subprocess, but in this case we don't\n + care about the general case of such potential parents. This process\n + neither spawns other \"git\" processes, nor is it interested in other\n + configuration. We might want to pick up other test modes here, but\n + those will be passed via GIT_TEST_* environment variables.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## cache.h ##\n3: 9f1bb0496ed ! 3: d837d905825 read-cache & fetch-negotiator: check \"enum\" values in switch()\n @@ Commit message\n case in fetch_negotiator_init() in favor of checking for\n \"FETCH_NEGOTIATION_UNSET\" and \"FETCH_NEGOTIATION_NONE\".\n \n + As will be discussed in a subsequent we'll only ever have either of\n + these set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and\n + UNTRACKED_CACHE_UNSET within the prepare_repo_settings() function\n + itself. In preparation for fixing that code let's add a BUG() here to\n + mark this as unreachable code.\n +\n See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\n for when the \"unset\" and \"keep\" handling for core.untrackedCache was\n consolidated, and aaf633c2ad1 (repo-settings: create\n @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,\n \n \tcase FETCH_NEGOTIATION_DEFAULT:\n -\tdefault:\n -+\tcase FETCH_NEGOTIATION_UNSET:\n -+\tcase FETCH_NEGOTIATION_NONE:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n ++\tcase FETCH_NEGOTIATION_NONE:\n ++\tcase FETCH_NEGOTIATION_UNSET:\n ++\t\tBUG(\"FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()\");\n \t}\n + }\n \n ## read-cache.c ##\n @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)\n @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)\n +\tcase UNTRACKED_CACHE_WRITE:\n \t\tadd_untracked_cache(istate);\n +\t\tbreak;\n -+\tcase UNTRACKED_CACHE_UNSET:\n +\tcase UNTRACKED_CACHE_KEEP:\n +\t\tbreak;\n ++\tcase UNTRACKED_CACHE_UNSET:\n ++\t\tBUG(\"UNTRACKED_CACHE_UNSET only in prepare_repo_settings()\");\n +\t}\n -+\treturn;\n }\n \n static void tweak_split_index(struct index_state *istate)\n4: b28ad2b2607 ! 4: 28286a61162 repo-settings.c: simplify the setup\n @@ Commit message\n \n In [4] the \"unset\" and \"keep\" handling for core.untrackedCache was\n consolidated. But it while we understand the \"keep\" value, we don't\n - handle it differently than the case of any other unknown value. So\n - we can remove UNTRACKED_CACHE_KEEP. It's not handled any\n - differently than UNTRACKED_CACHE_UNSET once we get past the config\n - parsing step.\n + handle it differently than the case of any other unknown value.\n +\n + So let's retain UNTRACKED_CACHE_KEEP and remove the\n + UNTRACKED_CACHE_UNSET setting (which was always implicitly\n + UNTRACKED_CACHE_KEEP before). We don't need to inform any code\n + after prepare_repo_settings() that the setting was \"unset\", as far\n + as anyone else is concerned it's core.untrackedCache=keep. if\n + \"core.untrackedcache\" isn't present in the config.\n \n * FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:\n \n @@ Commit message\n \n ## fetch-negotiator.c ##\n @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,\n - \t\treturn;\n - \n \tcase FETCH_NEGOTIATION_DEFAULT:\n --\tcase FETCH_NEGOTIATION_UNSET:\n --\tcase FETCH_NEGOTIATION_NONE:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n +-\tcase FETCH_NEGOTIATION_NONE:\n +-\tcase FETCH_NEGOTIATION_UNSET:\n +-\t\tBUG(\"FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()\");\n \t}\n + }\n \n ## read-cache.c ##\n @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)\n \t\tadd_untracked_cache(istate);\n \t\tbreak;\n - \tcase UNTRACKED_CACHE_UNSET:\n --\tcase UNTRACKED_CACHE_KEEP:\n -+\t\t/* This includes core.untrackedCache=keep */\n + \tcase UNTRACKED_CACHE_KEEP:\n ++\t\t/*\n ++\t\t * Either an explicit \"core.untrackedCache=keep\", the\n ++\t\t * default if \"core.untrackedCache\" isn't configured,\n ++\t\t * or a fallback on an unknown \"core.untrackedCache\"\n ++\t\t * value.\n ++\t\t */\n \t\tbreak;\n +-\tcase UNTRACKED_CACHE_UNSET:\n +-\t\tBUG(\"UNTRACKED_CACHE_UNSET only in prepare_repo_settings()\");\n \t}\n - \treturn;\n + }\n + \n \n ## repo-settings.c ##\n @@\n @@ repo-settings.c\n \t/* Defaults */\n -\tmemset(&r->settings, -1, sizeof(r->settings));\n +\tr->settings.index_version = -1;\n -+\tr->settings.core_untracked_cache = UNTRACKED_CACHE_UNSET;\n ++\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n +\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n +\n +\t/* Booleans config or default, cascades to other settings */\n @@ repo-settings.c\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 UNTRACKED_CACHE_UNSET.\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 @@ repository.h: struct submodule_cache;\n -\tUNTRACKED_CACHE_REMOVE = 0,\n -\tUNTRACKED_CACHE_KEEP = 1,\n -\tUNTRACKED_CACHE_WRITE = 2\n -+\tUNTRACKED_CACHE_UNSET,\n ++\tUNTRACKED_CACHE_KEEP,\n +\tUNTRACKED_CACHE_REMOVE,\n +\tUNTRACKED_CACHE_WRITE,\n };\n5: 0b5f213a639 = 5: 3cc033b8864 repository.h: don't use a mix of int and bitfields\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436358","messageId":"patch-v3-2.5-ece340af764-20210919T084703Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com","subject":"[PATCH v3 2/5] environment.c: remove test-specific \"ignore_untracked...\" variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T08:47:16Z","receivedAt":"2021-09-19T08:47:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Instead of the global ignore_untracked_cache_config variable added in\ndae6c322fa1 (test-dump-untracked-cache: don't modify the untracked\ncache, 2016-01-27) we can make use of the new facility to set config\nvia environment variables added in d8d77153eaf (config: allow\nspecifying config entries via envvar pairs, 2021-01-12).\n\nIt's arguably a bit hacky to use setenv() and getenv() to pass\nmessages between the same program, but since the test helpers are not\nthe main intended audience of repo-settings.c I think it's better than\nhardcoding the test-only special-case in prepare_repo_settings().\n\nThis uses the xsetenv() wrapper added in the preceding commit, if we\ndon't set these in the environment we'll fail in\nt7063-status-untracked-cache.sh, but let's fail earlier anyway if that\nwere to happen.\n\nThis breaks any parent process that's potentially using the\nGIT_CONFIG_* and GIT_CONFIG_PARAMETERS mechanism to pass one-shot\nconfig setting down to a git subprocess, but in this case we don't\ncare about the general case of such potential parents. This process\nneither spawns other \"git\" processes, nor is it interested in other\nconfiguration. We might want to pick up other test modes here, but\nthose will be passed via GIT_TEST_* environment variables.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n cache.h | 7 -------\n environment.c | 7 -------\n repo-settings.c | 7 +------\n t/helper/test-dump-untracked-cache.c | 6 ++++--\n 4 files changed, 5 insertions(+), 22 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex d23de693680..8e60fdd2a12 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1719,13 +1719,6 @@ int update_server_info(int);\n const char *get_log_output_encoding(void);\n const char *get_commit_output_encoding(void);\n \n-/*\n- * This is a hack for test programs like test-dump-untracked-cache to\n- * ensure that they do not modify the untracked cache when reading it.\n- * Do not use it otherwise!\n- */\n-extern int ignore_untracked_cache_config;\n-\n int committer_ident_sufficiently_given(void);\n int author_ident_sufficiently_given(void);\n \ndiff --git a/environment.c b/environment.c\nindex 7d8a949285c..d73dd0c42f7 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -96,13 +96,6 @@ int auto_comment_line_char;\n /* Parallel index stat data preload? */\n int core_preload_index = 1;\n \n-/*\n- * This is a hack for test programs like test-dump-untracked-cache to\n- * ensure that they do not modify the untracked cache when reading it.\n- * Do not use it otherwise!\n- */\n-int ignore_untracked_cache_config;\n-\n /* This is set by setup_git_dir_gently() and/or git_default_config() */\n char *git_work_tree_cfg;\n \ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0cfe8b787db..b0df8b93b86 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -70,12 +70,7 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n \n-\t/* Hack for test programs like test-dump-untracked-cache */\n-\tif (ignore_untracked_cache_config)\n-\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n-\telse\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n-\n+\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n \tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);\n \n \t/*\ndiff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c\nindex cf0f2c7228e..99010614f6d 100644\n--- a/t/helper/test-dump-untracked-cache.c\n+++ b/t/helper/test-dump-untracked-cache.c\n@@ -45,8 +45,10 @@ int cmd__dump_untracked_cache(int ac, const char **av)\n \tstruct untracked_cache *uc;\n \tstruct strbuf base = STRBUF_INIT;\n \n-\t/* Hack to avoid modifying the untracked cache when we read it */\n-\tignore_untracked_cache_config = 1;\n+\t/* Set core.untrackedCache=keep before setup_git_directory() */\n+\txsetenv(\"GIT_CONFIG_COUNT\", \"1\", 1);\n+\txsetenv(\"GIT_CONFIG_KEY_0\", \"core.untrackedCache\", 1);\n+\txsetenv(\"GIT_CONFIG_VALUE_0\", \"keep\", 1);\n \n \tsetup_git_directory();\n \tif (read_cache() < 0)\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436359","messageId":"patch-v3-1.5-4b320edc933-20210919T084703Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com","subject":"[PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T08:47:15Z","receivedAt":"2021-09-19T08:47:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add\nset_git_dir() function, 2007-08-01) we started checking its return\nvalue, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,\n2018-03-30) we've had set_git_dir_1() die if we couldn't set it.\n\nLet's provide a wrapper for both, this will be useful in many other\nplaces, a subsequent patch will make another use of xsetenv().\n\nThe checking of the return value here is over-eager according to\nsetenv(3) and POSIX. It's documented as returning just -1 or 0, so\nperhaps we should be checking -1 explicitly.\n\nLet's just instead die on any non-zero, if our C library is so broken\nas to return something else than -1 on error (and perhaps not set\nerrno?) the worst we'll do is die with a nonsensical errno value, but\nwe'll want to die in either case.\n\nWe could make these return \"void\" (as far as I can tell there's no\nother x*() wrappers that needed to make that decision before),\ni.e. our \"return 0\" is only to indicate that we didn't error, which we\nwould have died on. Let's return \"int\" instead to be consistent with\nthe C library function signatures, including for any future code that\nexpects a pointer to a setenv()-like function.\n\nI think it would be OK skip the NULL check of the \"name\" here for the\ncalls to die_errno(). Almost all of our setenv() callers are taking a\nconstant string hardcoded in the source as the first argument, and for\nthe rest we can probably assume they've done the NULL check\nthemselves. Even if they didn't, modern C libraries are forgiving\nabout it (e.g. glibc formatting it as \"(null)\"), on those that aren't,\nwell, we were about to die anyway. But let's include the check anyway\nfor good measure.\n\n1. https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n environment.c | 3 +--\n git-compat-util.h | 2 ++\n wrapper.c | 15 +++++++++++++++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex d6b22ede7ea..7d8a949285c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -330,8 +330,7 @@ char *get_graft_file(struct repository *r)\n \n static void set_git_dir_1(const char *path)\n {\n-\tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n-\t\tdie(_(\"could not set GIT_DIR to '%s'\"), path);\n+\txsetenv(GIT_DIR_ENVIRONMENT, path, 1);\n \tsetup_git_env(path);\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b46605300ab..0b0c0305165 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -875,6 +875,8 @@ void *xmemdupz(const void *data, size_t len);\n char *xstrndup(const char *str, size_t len);\n void *xrealloc(void *ptr, size_t size);\n void *xcalloc(size_t nmemb, size_t size);\n+int xsetenv(const char *name, const char *value, int overwrite);\n+int xunsetenv(const char *name);\n void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n const char *mmap_os_err(void);\n void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\ndiff --git a/wrapper.c b/wrapper.c\nindex 7c6586af321..95f989260cd 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -145,6 +145,21 @@ void *xcalloc(size_t nmemb, size_t size)\n \treturn ret;\n }\n \n+int xsetenv(const char *name, const char *value, int overwrite)\n+{\n+\tif (setenv(name, value, overwrite))\n+\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name ? name : \"(null)\",\n+\t\t\t value, overwrite);\n+\treturn 0;\n+}\n+\n+int xunsetenv(const char *name)\n+{\n+\tif (!unsetenv(name))\n+\t\tdie_errno(\"unsetenv(%s) failed\", name ? name : \"(null)\");\n+\treturn 0;\n+}\n+\n /*\n * Limit size of IO chunks, because huge chunks only cause pain. OS X\n * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436360","messageId":"patch-v3-3.5-d837d905825-20210919T084703Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com","subject":"[PATCH v3 3/5] read-cache & fetch-negotiator: check \"enum\" values in switch()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T08:47:17Z","receivedAt":"2021-09-19T08:47:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change tweak_untracked_cache() in \"read-cache.c\" to use a switch() to\nhave the compiler assert that we checked all possible values in the\n\"enum untracked_cache_setting\" type, and likewise remove the \"default\"\ncase in fetch_negotiator_init() in favor of checking for\n\"FETCH_NEGOTIATION_UNSET\" and \"FETCH_NEGOTIATION_NONE\".\n\nAs will be discussed in a subsequent we'll only ever have either of\nthese set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and\nUNTRACKED_CACHE_UNSET within the prepare_repo_settings() function\nitself. In preparation for fixing that code let's add a BUG() here to\nmark this as unreachable code.\n\nSee ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\nfor when the \"unset\" and \"keep\" handling for core.untrackedCache was\nconsolidated, and aaf633c2ad1 (repo-settings: create\nfeature.experimental setting, 2019-08-13) for the addition of the\n\"default\" pattern in \"fetch-negotiator.c\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fetch-negotiator.c | 4 +++-\n read-cache.c | 15 ++++++++++-----\n 2 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 57ed5784e14..237f92b8696 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -19,8 +19,10 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \n \tcase FETCH_NEGOTIATION_DEFAULT:\n-\tdefault:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n+\tcase FETCH_NEGOTIATION_NONE:\n+\tcase FETCH_NEGOTIATION_UNSET:\n+\t\tBUG(\"FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()\");\n \t}\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 9048ef9e905..6918dc3d8b2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1944,13 +1944,18 @@ static void tweak_untracked_cache(struct index_state *istate)\n \n \tprepare_repo_settings(r);\n \n-\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_REMOVE) {\n+\tswitch (r->settings.core_untracked_cache) {\n+\tcase UNTRACKED_CACHE_REMOVE:\n \t\tremove_untracked_cache(istate);\n-\t\treturn;\n-\t}\n-\n-\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE)\n+\t\tbreak;\n+\tcase UNTRACKED_CACHE_WRITE:\n \t\tadd_untracked_cache(istate);\n+\t\tbreak;\n+\tcase UNTRACKED_CACHE_KEEP:\n+\t\tbreak;\n+\tcase UNTRACKED_CACHE_UNSET:\n+\t\tBUG(\"UNTRACKED_CACHE_UNSET only in prepare_repo_settings()\");\n+\t}\n }\n \n static void tweak_split_index(struct index_state *istate)\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436361","messageId":"patch-v3-5.5-3cc033b8864-20210919T084703Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com","subject":"[PATCH v3 5/5] repository.h: don't use a mix of int and bitfields","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T08:47:19Z","receivedAt":"2021-09-19T08:47:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the bitfield added in 58300f47432 (sparse-index: add\nindex.sparse config option, 2021-03-30) and 3964fc2aae7 (sparse-index:\nadd guard to ensure full index, 2021-03-30) to just use an \"int\"\nboolean instead.\n\nIt might be smart to optimize the space here in the future, but by\nconsistently using an \"int\" we can take its address and pass it to\nrepo_cfg_bool(), and therefore don't need to handle \"sparse_index\" as\na special-case when reading the \"index.sparse\" setting.\n\nThere's no corresponding config for \"command_requires_full_index\", but\nlet's change it too for consistency and to prevent future bugs\ncreeping in due to one of these being \"unsigned\".\n\nUsing \"int\" consistently also prevents subtle bugs or undesired\ncontrol flow creeping in here. Before the preceding commit the\ninitialization of \"command_requires_full_index\" in\nprepare_repo_settings() did nothing, i.e. this:\n\n r->settings.command_requires_full_index = 1\n\nWas redundant to the earlier memset() to -1. Likewise for\n\"sparse_index\" added in 58300f47432 (sparse-index: add index.sparse\nconfig option, 2021-03-30) the code and comment added there was\nmisleading, we weren't initializing it to off, but re-initializing it\nfrom \"1\" to \"0\", and then finally checking the config, and perhaps\nsetting it to \"1\" again. I.e. we could have applied this patch before\nthe preceding commit:\n\n\t+\tassert(r->settings.command_requires_full_index == 1);\n\t \tr->settings.command_requires_full_index = 1;\n\n\t \t/*\n\t \t * Initialize this as off.\n\t \t */\n\t+\tassert(r->settings.sparse_index == 1);\n\t \tr->settings.sparse_index = 0;\n\t \tif (!repo_config_get_bool(r, \"index.sparse\", &value) && value)\n\t \t\tr->settings.sparse_index = 1;\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n repo-settings.c | 8 +-------\n repository.h | 5 ++---\n 2 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 46b9d56aeac..b93e91a212e 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -45,6 +45,7 @@ void prepare_repo_settings(struct repository *r)\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 \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\n@@ -90,11 +91,4 @@ void prepare_repo_settings(struct repository *r)\n \t * removed.\n \t */\n \tr->settings.command_requires_full_index = 1;\n-\n-\t/*\n-\t * Initialize this as off.\n-\t */\n-\tr->settings.sparse_index = 0;\n-\tif (!repo_config_get_bool(r, \"index.sparse\", &value) && value)\n-\t\tr->settings.sparse_index = 1;\n }\ndiff --git a/repository.h b/repository.h\nindex bf36744e0a7..02599ae2c98 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -31,6 +31,8 @@ struct repo_settings {\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n \tint fetch_write_commit_graph;\n+\tint command_requires_full_index;\n+\tint sparse_index;\n \n \tint index_version;\n \tenum untracked_cache_setting core_untracked_cache;\n@@ -39,9 +41,6 @@ struct repo_settings {\n \tenum fetch_negotiation_setting fetch_negotiation_algorithm;\n \n \tint core_multi_pack_index;\n-\n-\tunsigned command_requires_full_index:1,\n-\t\t sparse_index:1;\n };\n \n struct repository {\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436362","messageId":"patch-v3-4.5-28286a61162-20210919T084703Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com","subject":"[PATCH v3 4/5] repo-settings.c: simplify the setup","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T08:47:18Z","receivedAt":"2021-09-19T08:47:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Simplify the setup code in repo-settings.c in various ways, making the\ncode shorter, easier to read, and requiring fewer hacks to do the same\nthing as it did before:\n\nSince 7211b9e7534 (repo-settings: consolidate some config settings,\n2019-08-13) we have memset() the whole \"settings\" structure to -1 in\nprepare_repo_settings(), and subsequently relied on the -1 value.\n\nMost of the fields did not need to be initialized to -1, and because\nwe were doing that we had the enum labels \"UNTRACKED_CACHE_UNSET\" and\n\"FETCH_NEGOTIATION_UNSET\" purely to reflect the resulting state\ncreated this memset() in prepare_repo_settings(). No other code used\nor relied on them, more on that below.\n\nFor the rest most of the subsequent \"are we -1, then read xyz\" can\nsimply be removed by re-arranging what we read first. E.g. when\nsetting the \"index.version\" setting we should have first read\n\"feature.experimental\", so that it (and \"feature.manyfiles\") can\nprovide a default for our \"index.version\".\n\nInstead the code setting it, added when \"feature.manyFiles\"[1] was\ncreated, was using the UPDATE_DEFAULT_BOOL() macro added in an earlier\ncommit[2]. That macro is now gone, since it was only needed for this\npattern of reading things in the wrong order.\n\nThis also fixes an (admittedly obscure) logic error where we'd\nconflate an explicit \"-1\" value in the config with our own earlier\nmemset() -1.\n\nWe can also remove the UPDATE_DEFAULT_BOOL() wrapper added in\n[3]. Using it is redundant to simply using the return value from\nrepo_config_get_bool(), which is non-zero if the provided key exists\nin the config.\n\nDetails on edge cases relating to the memset() to -1, continued from\n\"more on that below\" above:\n\n * UNTRACKED_CACHE_KEEP:\n\n In [4] the \"unset\" and \"keep\" handling for core.untrackedCache was\n consolidated. But it while we understand the \"keep\" value, we don't\n handle it differently than the case of any other unknown value.\n\n So let's retain UNTRACKED_CACHE_KEEP and remove the\n UNTRACKED_CACHE_UNSET setting (which was always implicitly\n UNTRACKED_CACHE_KEEP before). We don't need to inform any code\n after prepare_repo_settings() that the setting was \"unset\", as far\n as anyone else is concerned it's core.untrackedCache=keep. if\n \"core.untrackedcache\" isn't present in the config.\n\n * FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:\n\n Since these two two enum fields added in [5] don't rely on the\n memzero() setting them to \"-1\" anymore we don't have to provide\n them with explicit values.\n\n1. c6cc4c5afd2 (repo-settings: create feature.manyFiles setting,\n 2019-08-13)\n2. 31b1de6a09b (commit-graph: turn on commit-graph by default,\n 2019-08-13)\n3. 31b1de6a09b (commit-graph: turn on commit-graph by default,\n 2019-08-13)\n4. ad0fb659993 (repo-settings: parse core.untrackedCache,\n 2019-08-13)\n5. aaf633c2ad1 (repo-settings: create feature.experimental setting,\n 2019-08-13)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fetch-negotiator.c | 3 --\n read-cache.c | 8 +++-\n repo-settings.c | 102 +++++++++++++++++++++++++--------------------\n repository.h | 15 +++----\n 4 files changed, 68 insertions(+), 60 deletions(-)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 237f92b8696..273390229fe 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -21,8 +21,5 @@ void fetch_negotiator_init(struct repository *r,\n \tcase FETCH_NEGOTIATION_DEFAULT:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n-\tcase FETCH_NEGOTIATION_NONE:\n-\tcase FETCH_NEGOTIATION_UNSET:\n-\t\tBUG(\"FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()\");\n \t}\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 6918dc3d8b2..147660c3090 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1952,9 +1952,13 @@ static void tweak_untracked_cache(struct index_state *istate)\n \t\tadd_untracked_cache(istate);\n \t\tbreak;\n \tcase UNTRACKED_CACHE_KEEP:\n+\t\t/*\n+\t\t * Either an explicit \"core.untrackedCache=keep\", the\n+\t\t * default if \"core.untrackedCache\" isn't configured,\n+\t\t * or a fallback on an unknown \"core.untrackedCache\"\n+\t\t * value.\n+\t\t */\n \t\tbreak;\n-\tcase UNTRACKED_CACHE_UNSET:\n-\t\tBUG(\"UNTRACKED_CACHE_UNSET only in prepare_repo_settings()\");\n \t}\n }\n \ndiff --git a/repo-settings.c b/repo-settings.c\nindex b0df8b93b86..46b9d56aeac 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -3,40 +3,76 @@\n #include \"repository.h\"\n #include \"midx.h\"\n \n-#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)\n+static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n+\t\t\t int def)\n+{\n+\tif (repo_config_get_bool(r, key, dest))\n+\t\t*dest = def;\n+}\n \n void prepare_repo_settings(struct repository *r)\n {\n+\tint experimental;\n \tint value;\n \tchar *strval;\n+\tint manyfiles;\n \n-\tif (r->settings.initialized)\n+\tif (r->settings.initialized++)\n \t\treturn;\n \n \t/* Defaults */\n-\tmemset(&r->settings, -1, sizeof(r->settings));\n+\tr->settings.index_version = -1;\n+\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n+\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\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 \n-\tif (!repo_config_get_bool(r, \"core.commitgraph\", &value))\n-\t\tr->settings.core_commit_graph = value;\n-\tif (!repo_config_get_bool(r, \"commitgraph.readchangedpaths\", &value))\n-\t\tr->settings.commit_graph_read_changed_paths = value;\n-\tif (!repo_config_get_bool(r, \"gc.writecommitgraph\", &value))\n-\t\tr->settings.gc_write_commit_graph = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);\n-\tUPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);\n-\tUPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);\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+\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+\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/*\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-\tif (!repo_config_get_maybe_bool(r, \"core.untrackedcache\", &value)) {\n-\t\tif (value == 0)\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_REMOVE;\n-\t\telse\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n-\t} else if (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n-\t\tif (!strcasecmp(strval, \"keep\"))\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\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@@ -45,34 +81,8 @@ void prepare_repo_settings(struct repository *r)\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\n-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n \n-\tif (!repo_config_get_bool(r, \"pack.usesparse\", &value))\n-\t\tr->settings.pack_use_sparse = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n-\n-\tvalue = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);\n-\tif (value || !repo_config_get_bool(r, \"core.multipackindex\", &value))\n-\t\tr->settings.core_multi_pack_index = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);\n-\n-\tif (!repo_config_get_bool(r, \"feature.manyfiles\", &value) && value) {\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n-\t}\n-\n-\tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n-\t\tr->settings.fetch_write_commit_graph = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0);\n-\n-\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n-\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n-\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);\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\ndiff --git a/repository.h b/repository.h\nindex 3740c93bc0f..bf36744e0a7 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -13,18 +13,15 @@ struct submodule_cache;\n struct promisor_remote_config;\n \n enum untracked_cache_setting {\n-\tUNTRACKED_CACHE_UNSET = -1,\n-\tUNTRACKED_CACHE_REMOVE = 0,\n-\tUNTRACKED_CACHE_KEEP = 1,\n-\tUNTRACKED_CACHE_WRITE = 2\n+\tUNTRACKED_CACHE_KEEP,\n+\tUNTRACKED_CACHE_REMOVE,\n+\tUNTRACKED_CACHE_WRITE,\n };\n \n enum fetch_negotiation_setting {\n-\tFETCH_NEGOTIATION_UNSET = -1,\n-\tFETCH_NEGOTIATION_NONE = 0,\n-\tFETCH_NEGOTIATION_DEFAULT = 1,\n-\tFETCH_NEGOTIATION_SKIPPING = 2,\n-\tFETCH_NEGOTIATION_NOOP = 3,\n+\tFETCH_NEGOTIATION_DEFAULT,\n+\tFETCH_NEGOTIATION_SKIPPING,\n+\tFETCH_NEGOTIATION_NOOP,\n };\n \n struct repo_settings {\n-- \n2.33.0.1092.g44c994ea1be\n\n"},{"id":"436412","messageId":"ddf328f2-30cf-2881-c61f-0698d499ac39@gmail.com","threadId":"56517","inReplyTo":"patch-v3-4.5-28286a61162-20210919T084703Z-avarab@gmail.com","subject":"Re: [PATCH v3 4/5] repo-settings.c: simplify the setup","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-20T12:42:37Z","receivedAt":"2021-09-20T12:42:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/19/2021 4:47 AM, Ævar Arnfjörð Bjarmason wrote:\n> Simplify the setup code in repo-settings.c in various ways, making the\n> code shorter, easier to read, and requiring fewer hacks to do the same\n> thing as it did before:\n\nPopping in here to say that I agree this new arrangement is cleaner.\n\nThe previous patterns were confusing, and probably led to at least one bug,\nincluding one in the builtin FS Monitor shipped with git-for-windows/git [1]\nwhere enabling features.experimental overrides core.useBuiltinFSMonitor=false.\nThe newer organization should make it harder for mistakes like that to happen.\n\nThanks,\n-Stolee\n\n[1] https://github.com/git-for-windows/git/discussions/3251#discussioncomment-1296896\n"},{"id":"436498","messageId":"YUkC7RIDR7pQWsPh@nand.local","threadId":"56517","inReplyTo":"patch-v3-1.5-4b320edc933-20210919T084703Z-avarab@gmail.com","subject":"Re: [PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T21:53:49Z","receivedAt":"2021-09-21T02:20:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Sep 19, 2021 at 10:47:15AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add\n> set_git_dir() function, 2007-08-01) we started checking its return\n> value, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,\n> 2018-03-30) we've had set_git_dir_1() die if we couldn't set it.\n>\n> Let's provide a wrapper for both, this will be useful in many other\n> places, a subsequent patch will make another use of xsetenv().\n\nMakes sense.\n\n> We could make these return \"void\" (as far as I can tell there's no\n> other x*() wrappers that needed to make that decision before),\n> i.e. our \"return 0\" is only to indicate that we didn't error, which we\n> would have died on. Let's return \"int\" instead to be consistent with\n> the C library function signatures, including for any future code that\n> expects a pointer to a setenv()-like function.\n\nThis may be a little over-clever ;). It is cute, but returning an int\nmakes xsetenv a drop-in replacement for setenv. Which is nice, but it\nmakes it all too-easy to take code like:\n\n if (setenv(...) < 0)\n die(_(\"...\"));\n\nand replace it with\n\n if (xsetenv(...) < 0)\n\nwhich makes the whole conditional redundant, since the wrappers are\nguaranteed not to return an error.\n\nIn other words, I like the idea that s/setenv/x&/ causes a compile-time\nerror, and returning an int from these wrappers prevents that from\nhappening.\n\nThis may be a little too-theoretical, and you're certainly free to\ndisagree, just my $0.02.\n\n> I think it would be OK skip the NULL check of the \"name\" here for the\n> calls to die_errno(). Almost all of our setenv() callers are taking a\n> constant string hardcoded in the source as the first argument, and for\n> the rest we can probably assume they've done the NULL check\n> themselves. Even if they didn't, modern C libraries are forgiving\n> about it (e.g. glibc formatting it as \"(null)\"), on those that aren't,\n> well, we were about to die anyway. But let's include the check anyway\n> for good measure.\n\nThis I think is a good call. I agree in practice that most times we'd be\njust fine to pass null to printf() (as we have seen from 88617d11f9\n(multi-pack-index: fix potential segfault without sub-command,\n2021-07-19) ;)). But there's no reason to rely on risky assumptions when\nit's easy to avoid doing so.\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 7c6586af321..95f989260cd 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -145,6 +145,21 @@ void *xcalloc(size_t nmemb, size_t size)\n> \treturn ret;\n> }\n>\n> +int xsetenv(const char *name, const char *value, int overwrite)\n> +{\n> +\tif (setenv(name, value, overwrite))\n> +\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name ? name : \"(null)\",\n> +\t\t\t value, overwrite);\n> +\treturn 0;\n> +}\n> +\n> +int xunsetenv(const char *name)\n> +{\n> +\tif (!unsetenv(name))\n> +\t\tdie_errno(\"unsetenv(%s) failed\", name ? name : \"(null)\");\n> +\treturn 0;\n> +}\n> +\n\nFor what it's worth, I find these new messages a little wordy. Maybe\nwe should just sticky \"could not (un)set %s\"?\n\nThanks,\nTaylor\n"},{"id":"436500","messageId":"YUkG1u7utvi6+VgS@nand.local","threadId":"56517","inReplyTo":"patch-v3-2.5-ece340af764-20210919T084703Z-avarab@gmail.com","subject":"Re: [PATCH v3 2/5] environment.c: remove test-specific \"ignore_untracked...\" variable","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T22:10:30Z","receivedAt":"2021-09-21T02:20:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Sep 19, 2021 at 10:47:16AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Instead of the global ignore_untracked_cache_config variable added in\n> dae6c322fa1 (test-dump-untracked-cache: don't modify the untracked\n> cache, 2016-01-27) we can make use of the new facility to set config\n> via environment variables added in d8d77153eaf (config: allow\n> specifying config entries via envvar pairs, 2021-01-12).\n>\n> It's arguably a bit hacky to use setenv() and getenv() to pass\n> messages between the same program, but since the test helpers are not\n> the main intended audience of repo-settings.c I think it's better than\n> hardcoding the test-only special-case in prepare_repo_settings().\n\nHmm. I tend to agree that using (a wrapper over) setenv() to pass\nmessages between the test helper and the rest of Git is a little bit of\na hack.\n\nEverything you wrote should work based on my understanding of the\nconfig-over-environment-variable stuff added recently. But I wish that\nit didn't involve losing some grep-ability between the test-helper and\nlibrary code.\n\nSo I wouldn't be sad to see this patch get dropped, and I also wouldn't\nbe overly sad to see it get picked up, either. But I don't think it's a\nnecessary step, and we may be slightly better without it.\n\nThanks,\nTaylor\n"},{"id":"436502","messageId":"YUkH1LSa1WcFDmvr@nand.local","threadId":"56517","inReplyTo":"patch-v3-3.5-d837d905825-20210919T084703Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/5] read-cache & fetch-negotiator: check \"enum\" values in switch()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T22:14:44Z","receivedAt":"2021-09-21T02:20:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Sep 19, 2021 at 10:47:17AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Change tweak_untracked_cache() in \"read-cache.c\" to use a switch() to\n> have the compiler assert that we checked all possible values in the\n> \"enum untracked_cache_setting\" type, and likewise remove the \"default\"\n> case in fetch_negotiator_init() in favor of checking for\n> \"FETCH_NEGOTIATION_UNSET\" and \"FETCH_NEGOTIATION_NONE\".\n>\n> As will be discussed in a subsequent we'll only ever have either of\n\ns/subsequent/& patch/ ?\n\n> these set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and\n> UNTRACKED_CACHE_UNSET within the prepare_repo_settings() function\n> itself. In preparation for fixing that code let's add a BUG() here to\n> mark this as unreachable code.\n>\n> See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\n> for when the \"unset\" and \"keep\" handling for core.untrackedCache was\n> consolidated, and aaf633c2ad1 (repo-settings: create\n> feature.experimental setting, 2019-08-13) for the addition of the\n> \"default\" pattern in \"fetch-negotiator.c\".\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> fetch-negotiator.c | 4 +++-\n> read-cache.c | 15 ++++++++++-----\n> 2 files changed, 13 insertions(+), 6 deletions(-)\n>\n> diff --git a/fetch-negotiator.c b/fetch-negotiator.c\n> index 57ed5784e14..237f92b8696 100644\n> --- a/fetch-negotiator.c\n> +++ b/fetch-negotiator.c\n> @@ -19,8 +19,10 @@ void fetch_negotiator_init(struct repository *r,\n> \t\treturn;\n>\n> \tcase FETCH_NEGOTIATION_DEFAULT:\n> -\tdefault:\n> \t\tdefault_negotiator_init(negotiator);\n> \t\treturn;\n> +\tcase FETCH_NEGOTIATION_NONE:\n> +\tcase FETCH_NEGOTIATION_UNSET:\n> +\t\tBUG(\"FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()\");\n\nI was briefly confused why this BUG message mentioned\nFETCH_NEGOTIATION_UNSET, since we only support FETCH_NEGOTIATION_DEFAULT\nhere.\n\nBut then I realized that it said \"only in prepare_repo_settings()\", and\nwe're in fetch_negotiator_init(). So this makes sense to me.\n\nOther than the small typo in the patch message, this looks good to me.\n\nThanks,\nTaylor\n"},{"id":"436508","messageId":"YUkI0XieMJbzqlTT@nand.local","threadId":"56517","inReplyTo":"ddf328f2-30cf-2881-c61f-0698d499ac39@gmail.com","subject":"Re: [PATCH v3 4/5] repo-settings.c: simplify the setup","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T22:18:57Z","receivedAt":"2021-09-21T02:21:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 08:42:37AM -0400, Derrick Stolee wrote:\n> On 9/19/2021 4:47 AM, Ævar Arnfjörð Bjarmason wrote:\n> > Simplify the setup code in repo-settings.c in various ways, making the\n> > code shorter, easier to read, and requiring fewer hacks to do the same\n> > thing as it did before:\n>\n> Popping in here to say that I agree this new arrangement is cleaner.\n\nAgreed; I couldn't spot anything in the post-image that wasn't an\nimprovement in one way or another.\n\nThanks,\nTaylor\n"},{"id":"436509","messageId":"YUkKYWEc9vXFUsDu@nand.local","threadId":"56517","inReplyTo":"patch-v3-5.5-3cc033b8864-20210919T084703Z-avarab@gmail.com","subject":"Re: [PATCH v3 5/5] repository.h: don't use a mix of int and bitfields","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T22:25:37Z","receivedAt":"2021-09-21T02:21:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Sep 19, 2021 at 10:47:19AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> There's no corresponding config for \"command_requires_full_index\", but\n> let's change it too for consistency and to prevent future bugs\n> creeping in due to one of these being \"unsigned\".\n\nThis was the only thing that I thought of while reading through the\ndiff, so I appreciate that you explicitly called out why\ncommand_requires_full_index was changed in this patch.\n\nThis looks good to me.\n\nThanks,\nTaylor\n"},{"id":"436520","messageId":"87pmt2aki2.fsf@evledraar.gmail.com","threadId":"56517","inReplyTo":"YUkH1LSa1WcFDmvr@nand.local","subject":"Re: [PATCH v3 3/5] read-cache & fetch-negotiator: check \"enum\" values in switch()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-20T23:33:20Z","receivedAt":"2021-09-21T02:22:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Taylor Blau wrote:\n\n> On Sun, Sep 19, 2021 at 10:47:17AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Change tweak_untracked_cache() in \"read-cache.c\" to use a switch() to\n>> have the compiler assert that we checked all possible values in the\n>> \"enum untracked_cache_setting\" type, and likewise remove the \"default\"\n>> case in fetch_negotiator_init() in favor of checking for\n>> \"FETCH_NEGOTIATION_UNSET\" and \"FETCH_NEGOTIATION_NONE\".\n>>\n>> As will be discussed in a subsequent we'll only ever have either of\n>\n> s/subsequent/& patch/ ?\n\nThanks.\n\n>> these set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and\n>> UNTRACKED_CACHE_UNSET within the prepare_repo_settings() function\n>> itself. In preparation for fixing that code let's add a BUG() here to\n>> mark this as unreachable code.\n>>\n>> See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\n>> for when the \"unset\" and \"keep\" handling for core.untrackedCache was\n>> consolidated, and aaf633c2ad1 (repo-settings: create\n>> feature.experimental setting, 2019-08-13) for the addition of the\n>> \"default\" pattern in \"fetch-negotiator.c\".\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> fetch-negotiator.c | 4 +++-\n>> read-cache.c | 15 ++++++++++-----\n>> 2 files changed, 13 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/fetch-negotiator.c b/fetch-negotiator.c\n>> index 57ed5784e14..237f92b8696 100644\n>> --- a/fetch-negotiator.c\n>> +++ b/fetch-negotiator.c\n>> @@ -19,8 +19,10 @@ void fetch_negotiator_init(struct repository *r,\n>> \t\treturn;\n>>\n>> \tcase FETCH_NEGOTIATION_DEFAULT:\n>> -\tdefault:\n>> \t\tdefault_negotiator_init(negotiator);\n>> \t\treturn;\n>> +\tcase FETCH_NEGOTIATION_NONE:\n>> +\tcase FETCH_NEGOTIATION_UNSET:\n>> +\t\tBUG(\"FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()\");\n>\n> I was briefly confused why this BUG message mentioned\n> FETCH_NEGOTIATION_UNSET, since we only support FETCH_NEGOTIATION_DEFAULT\n> here.\n>\n> But then I realized that it said \"only in prepare_repo_settings()\", and\n> we're in fetch_negotiator_init(). So this makes sense to me.\n>\n> Other than the small typo in the patch message, this looks good to me.\n\nI guess I'll also mention NONE here, so:\n\n BUG(\"FETCH_NEGOTIATION_{NONE,UNSET} only in prepare_repo_settings()\");\n\nOr elaborate a bit:\n\n BUG(\"FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!\");\n\nIn any case this lives for just one commit, and is just here to\ndemonstrate the transition we're in.\n\n"},{"id":"436521","messageId":"87tuieakms.fsf@evledraar.gmail.com","threadId":"56517","inReplyTo":"YUkG1u7utvi6+VgS@nand.local","subject":"Re: [PATCH v3 2/5] environment.c: remove test-specific \"ignore_untracked...\" variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-20T23:27:29Z","receivedAt":"2021-09-21T02:22:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Taylor Blau wrote:\n\n> On Sun, Sep 19, 2021 at 10:47:16AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Instead of the global ignore_untracked_cache_config variable added in\n>> dae6c322fa1 (test-dump-untracked-cache: don't modify the untracked\n>> cache, 2016-01-27) we can make use of the new facility to set config\n>> via environment variables added in d8d77153eaf (config: allow\n>> specifying config entries via envvar pairs, 2021-01-12).\n>>\n>> It's arguably a bit hacky to use setenv() and getenv() to pass\n>> messages between the same program, but since the test helpers are not\n>> the main intended audience of repo-settings.c I think it's better than\n>> hardcoding the test-only special-case in prepare_repo_settings().\n>\n> Hmm. I tend to agree that using (a wrapper over) setenv() to pass\n> messages between the test helper and the rest of Git is a little bit of\n> a hack.\n>\n> Everything you wrote should work based on my understanding of the\n> config-over-environment-variable stuff added recently. But I wish that\n> it didn't involve losing some grep-ability between the test-helper and\n> library code.\n\nDoes that grep-ability between the two have any reason to exist? The\nonly reason we need this special-case in the test helper is because it's\nnot setting up \"normal\" config.\n\nIt could also be made to do so, that's a bigger behavior change than\nthis narrow change, but likewise we'd just end up with a \"git config\ncore.untrackedCache keep\" in some test *.sh somewhere, and no\ngrep-ability between the test helper and library code.\n\nBut now that we have GIT_CONFIG_COUNT etc. using the environment has\nbecome a perfectly fine way to pass this data along, we could also do\nthat in the *.sh setup, but this was easier, and also easier to\nguarantee correctness with the new x*() wrapper.\n\nIOW just because it's called t/helper/test-dump-untracked-cache.c it\nreally doesn't have any business reaching into the guts of\nrepo-settings.c to tweak how we set up core.untrackedCache. The only\nreason it did was because the code pre-dated the\nGIT_CONFIG_{COUNT,KEY,VALUE} implementation.\n\n> So I wouldn't be sad to see this patch get dropped, and I also wouldn't\n> be overly sad to see it get picked up, either. But I don't think it's a\n> necessary step, and we may be slightly better without it.\n>\n> Thanks,\n> Taylor\n\n"},{"id":"436528","messageId":"87y27qakzx.fsf@evledraar.gmail.com","threadId":"56517","inReplyTo":"YUkC7RIDR7pQWsPh@nand.local","subject":"Re: [PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-20T23:17:35Z","receivedAt":"2021-09-21T02:22:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Taylor Blau wrote:\n\n> On Sun, Sep 19, 2021 at 10:47:15AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add\n>> set_git_dir() function, 2007-08-01) we started checking its return\n>> value, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,\n>> 2018-03-30) we've had set_git_dir_1() die if we couldn't set it.\n>>\n>> Let's provide a wrapper for both, this will be useful in many other\n>> places, a subsequent patch will make another use of xsetenv().\n>\n> Makes sense.\n>\n>> We could make these return \"void\" (as far as I can tell there's no\n>> other x*() wrappers that needed to make that decision before),\n>> i.e. our \"return 0\" is only to indicate that we didn't error, which we\n>> would have died on. Let's return \"int\" instead to be consistent with\n>> the C library function signatures, including for any future code that\n>> expects a pointer to a setenv()-like function.\n>\n> This may be a little over-clever ;). It is cute, but returning an int\n> makes xsetenv a drop-in replacement for setenv. Which is nice, but it\n> makes it all too-easy to take code like:\n>\n> if (setenv(...) < 0)\n> die(_(\"...\"));\n>\n> and replace it with\n>\n> if (xsetenv(...) < 0)\n>\n> which makes the whole conditional redundant, since the wrappers are\n> guaranteed not to return an error.\n>\n> In other words, I like the idea that s/setenv/x&/ causes a compile-time\n> error, and returning an int from these wrappers prevents that from\n> happening.\n>\n> This may be a little too-theoretical, and you're certainly free to\n> disagree, just my $0.02.\n\nI'm fine with doing that in principle, I couldn't find any \"x*()\"\nwrappers with different signatures. Yes, having the compiler complain\nbecause you used a \"void\" return value would be nice.\n\nI did have some vague notion that this might interact badly with\nsomething in compat, i.e. if setenv() or unsetenv() was a fallback, but\nI can't think now of why that wouldn't work...\n\n>> I think it would be OK skip the NULL check of the \"name\" here for the\n>> calls to die_errno(). Almost all of our setenv() callers are taking a\n>> constant string hardcoded in the source as the first argument, and for\n>> the rest we can probably assume they've done the NULL check\n>> themselves. Even if they didn't, modern C libraries are forgiving\n>> about it (e.g. glibc formatting it as \"(null)\"), on those that aren't,\n>> well, we were about to die anyway. But let's include the check anyway\n>> for good measure.\n>\n> This I think is a good call. I agree in practice that most times we'd be\n> just fine to pass null to printf() (as we have seen from 88617d11f9\n> (multi-pack-index: fix potential segfault without sub-command,\n> 2021-07-19) ;)). But there's no reason to rely on risky assumptions when\n> it's easy to avoid doing so.\n\n*nod*\n\n>> diff --git a/wrapper.c b/wrapper.c\n>> index 7c6586af321..95f989260cd 100644\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -145,6 +145,21 @@ void *xcalloc(size_t nmemb, size_t size)\n>> \treturn ret;\n>> }\n>>\n>> +int xsetenv(const char *name, const char *value, int overwrite)\n>> +{\n>> +\tif (setenv(name, value, overwrite))\n>> +\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name ? name : \"(null)\",\n>> +\t\t\t value, overwrite);\n>> +\treturn 0;\n>> +}\n>> +\n>> +int xunsetenv(const char *name)\n>> +{\n>> +\tif (!unsetenv(name))\n>> +\t\tdie_errno(\"unsetenv(%s) failed\", name ? name : \"(null)\");\n>> +\treturn 0;\n>> +}\n>> +\n>\n> For what it's worth, I find these new messages a little wordy. Maybe\n> we should just sticky \"could not (un)set %s\"?\n\nSure, will change it.\n\nI think I may have wanted to include the \"overwrite\" in the setenv() in\nsome way though, so you'd know if it failed because the libc call\n\"really\" failed, v.s. we just had an existing value in the environment\nand didn't set \"overwrite\".\n\nBut reading the docs again that won't work, since it'll succeed if it\ncan't write that key, i.e. the caller with overwrite=0 needs to\nfollow-up with a getenv() to see if their value or someone else's was\nset (or more probably, they don't care, which is why they used\noverwrite=0).\n\nSo it doesn't matter either way for xsetenv(), will make it less chatty.\n"},{"id":"436602","messageId":"cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com","subject":"[PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:12:58Z","receivedAt":"2021-09-21T13:13:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A hopefully final re-roll addressing Taylor's v3 review, except for\nthe suggestion (that I read as) perhaps retaining the test-only code,\nwhich I've decided not to do per\nhttp://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com\n\nThe x(un)setenv() now returns void, and the error messages are less\nchatty, I also improved a BUG() message in 4/5 that we end up deleting\nin 5/5 anyway, so it doesn't matter for the end-state, just for\nunderstanding the patches.\n\nÆvar Arnfjörð Bjarmason (5):\n wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c\n environment.c: remove test-specific \"ignore_untracked...\" variable\n read-cache & fetch-negotiator: check \"enum\" values in switch()\n repo-settings.c: simplify the setup\n repository.h: don't use a mix of int and bitfields\n\n cache.h | 7 --\n environment.c | 10 +--\n fetch-negotiator.c | 1 -\n git-compat-util.h | 2 +\n read-cache.c | 19 +++--\n repo-settings.c | 115 +++++++++++++--------------\n repository.h | 20 ++---\n t/helper/test-dump-untracked-cache.c | 6 +-\n wrapper.c | 12 +++\n 9 files changed, 98 insertions(+), 94 deletions(-)\n\nRange-diff against v3:\n1: 4b320edc933 ! 1: 4dd317ab65e wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c\n @@ Commit message\n errno?) the worst we'll do is die with a nonsensical errno value, but\n we'll want to die in either case.\n \n - We could make these return \"void\" (as far as I can tell there's no\n - other x*() wrappers that needed to make that decision before),\n - i.e. our \"return 0\" is only to indicate that we didn't error, which we\n - would have died on. Let's return \"int\" instead to be consistent with\n - the C library function signatures, including for any future code that\n - expects a pointer to a setenv()-like function.\n + Let's make these return \"void\" instead of \"int\". As far as I can tell\n + there's no other x*() wrappers that needed to make the decision of\n + deviating from the signature in the C library, but since their return\n + value is only used to indicate errors (so we'd die here), we can catch\n + unreachable code such as\n +\n + if (xsetenv(...) < 0)\n + [...];\n \n I think it would be OK skip the NULL check of the \"name\" here for the\n calls to die_errno(). Almost all of our setenv() callers are taking a\n @@ git-compat-util.h: void *xmemdupz(const void *data, size_t len);\n char *xstrndup(const char *str, size_t len);\n void *xrealloc(void *ptr, size_t size);\n void *xcalloc(size_t nmemb, size_t size);\n -+int xsetenv(const char *name, const char *value, int overwrite);\n -+int xunsetenv(const char *name);\n ++void xsetenv(const char *name, const char *value, int overwrite);\n ++void xunsetenv(const char *name);\n void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n const char *mmap_os_err(void);\n void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n @@ wrapper.c: void *xcalloc(size_t nmemb, size_t size)\n \treturn ret;\n }\n \n -+int xsetenv(const char *name, const char *value, int overwrite)\n ++void xsetenv(const char *name, const char *value, int overwrite)\n +{\n +\tif (setenv(name, value, overwrite))\n -+\t\tdie_errno(\"setenv(%s, '%s', %d) failed\", name ? name : \"(null)\",\n -+\t\t\t value, overwrite);\n -+\treturn 0;\n ++\t\tdie_errno(_(\"could not setenv '%s'\"), name ? name : \"(null)\");\n +}\n +\n -+int xunsetenv(const char *name)\n ++void xunsetenv(const char *name)\n +{\n +\tif (!unsetenv(name))\n -+\t\tdie_errno(\"unsetenv(%s) failed\", name ? name : \"(null)\");\n -+\treturn 0;\n ++\t\tdie_errno(_(\"could not unsetenv '%s'\"), name ? name : \"(null)\");\n +}\n +\n /*\n2: ece340af764 = 2: 3dc37521184 environment.c: remove test-specific \"ignore_untracked...\" variable\n3: d837d905825 ! 3: b36b23ee173 read-cache & fetch-negotiator: check \"enum\" values in switch()\n @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n +\tcase FETCH_NEGOTIATION_NONE:\n +\tcase FETCH_NEGOTIATION_UNSET:\n -+\t\tBUG(\"FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()\");\n ++\t\tBUG(\"FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!\");\n \t}\n }\n \n @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)\n +\tcase UNTRACKED_CACHE_KEEP:\n +\t\tbreak;\n +\tcase UNTRACKED_CACHE_UNSET:\n -+\t\tBUG(\"UNTRACKED_CACHE_UNSET only in prepare_repo_settings()\");\n ++\t\tBUG(\"UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!\");\n +\t}\n }\n \n4: 28286a61162 ! 4: c9f143b26f1 repo-settings.c: simplify the setup\n @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n -\tcase FETCH_NEGOTIATION_NONE:\n -\tcase FETCH_NEGOTIATION_UNSET:\n --\t\tBUG(\"FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()\");\n +-\t\tBUG(\"FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!\");\n \t}\n }\n \n @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)\n +\t\t */\n \t\tbreak;\n -\tcase UNTRACKED_CACHE_UNSET:\n --\t\tBUG(\"UNTRACKED_CACHE_UNSET only in prepare_repo_settings()\");\n +-\t\tBUG(\"UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!\");\n \t}\n }\n \n5: 3cc033b8864 = 5: aadd4c42923 repository.h: don't use a mix of int and bitfields\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436603","messageId":"patch-v4-1.5-4dd317ab65e-20210921T131003Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com","subject":"[PATCH v4 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:12:59Z","receivedAt":"2021-09-21T13:13:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add\nset_git_dir() function, 2007-08-01) we started checking its return\nvalue, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,\n2018-03-30) we've had set_git_dir_1() die if we couldn't set it.\n\nLet's provide a wrapper for both, this will be useful in many other\nplaces, a subsequent patch will make another use of xsetenv().\n\nThe checking of the return value here is over-eager according to\nsetenv(3) and POSIX. It's documented as returning just -1 or 0, so\nperhaps we should be checking -1 explicitly.\n\nLet's just instead die on any non-zero, if our C library is so broken\nas to return something else than -1 on error (and perhaps not set\nerrno?) the worst we'll do is die with a nonsensical errno value, but\nwe'll want to die in either case.\n\nLet's make these return \"void\" instead of \"int\". As far as I can tell\nthere's no other x*() wrappers that needed to make the decision of\ndeviating from the signature in the C library, but since their return\nvalue is only used to indicate errors (so we'd die here), we can catch\nunreachable code such as\n\n if (xsetenv(...) < 0)\n [...];\n\nI think it would be OK skip the NULL check of the \"name\" here for the\ncalls to die_errno(). Almost all of our setenv() callers are taking a\nconstant string hardcoded in the source as the first argument, and for\nthe rest we can probably assume they've done the NULL check\nthemselves. Even if they didn't, modern C libraries are forgiving\nabout it (e.g. glibc formatting it as \"(null)\"), on those that aren't,\nwell, we were about to die anyway. But let's include the check anyway\nfor good measure.\n\n1. https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n environment.c | 3 +--\n git-compat-util.h | 2 ++\n wrapper.c | 12 ++++++++++++\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex d6b22ede7ea..7d8a949285c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -330,8 +330,7 @@ char *get_graft_file(struct repository *r)\n \n static void set_git_dir_1(const char *path)\n {\n-\tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n-\t\tdie(_(\"could not set GIT_DIR to '%s'\"), path);\n+\txsetenv(GIT_DIR_ENVIRONMENT, path, 1);\n \tsetup_git_env(path);\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ddc65ff61d9..94d8250832d 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -875,6 +875,8 @@ void *xmemdupz(const void *data, size_t len);\n char *xstrndup(const char *str, size_t len);\n void *xrealloc(void *ptr, size_t size);\n void *xcalloc(size_t nmemb, size_t size);\n+void xsetenv(const char *name, const char *value, int overwrite);\n+void xunsetenv(const char *name);\n void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n const char *mmap_os_err(void);\n void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\ndiff --git a/wrapper.c b/wrapper.c\nindex 7c6586af321..1460d4e27b0 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -145,6 +145,18 @@ void *xcalloc(size_t nmemb, size_t size)\n \treturn ret;\n }\n \n+void xsetenv(const char *name, const char *value, int overwrite)\n+{\n+\tif (setenv(name, value, overwrite))\n+\t\tdie_errno(_(\"could not setenv '%s'\"), name ? name : \"(null)\");\n+}\n+\n+void xunsetenv(const char *name)\n+{\n+\tif (!unsetenv(name))\n+\t\tdie_errno(_(\"could not unsetenv '%s'\"), name ? name : \"(null)\");\n+}\n+\n /*\n * Limit size of IO chunks, because huge chunks only cause pain. OS X\n * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436604","messageId":"patch-v4-2.5-3dc37521184-20210921T131003Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com","subject":"[PATCH v4 2/5] environment.c: remove test-specific \"ignore_untracked...\" variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:13:00Z","receivedAt":"2021-09-21T13:13:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Instead of the global ignore_untracked_cache_config variable added in\ndae6c322fa1 (test-dump-untracked-cache: don't modify the untracked\ncache, 2016-01-27) we can make use of the new facility to set config\nvia environment variables added in d8d77153eaf (config: allow\nspecifying config entries via envvar pairs, 2021-01-12).\n\nIt's arguably a bit hacky to use setenv() and getenv() to pass\nmessages between the same program, but since the test helpers are not\nthe main intended audience of repo-settings.c I think it's better than\nhardcoding the test-only special-case in prepare_repo_settings().\n\nThis uses the xsetenv() wrapper added in the preceding commit, if we\ndon't set these in the environment we'll fail in\nt7063-status-untracked-cache.sh, but let's fail earlier anyway if that\nwere to happen.\n\nThis breaks any parent process that's potentially using the\nGIT_CONFIG_* and GIT_CONFIG_PARAMETERS mechanism to pass one-shot\nconfig setting down to a git subprocess, but in this case we don't\ncare about the general case of such potential parents. This process\nneither spawns other \"git\" processes, nor is it interested in other\nconfiguration. We might want to pick up other test modes here, but\nthose will be passed via GIT_TEST_* environment variables.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n cache.h | 7 -------\n environment.c | 7 -------\n repo-settings.c | 7 +------\n t/helper/test-dump-untracked-cache.c | 6 ++++--\n 4 files changed, 5 insertions(+), 22 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0c245d4f105..8941cc62c59 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1726,13 +1726,6 @@ int update_server_info(int);\n const char *get_log_output_encoding(void);\n const char *get_commit_output_encoding(void);\n \n-/*\n- * This is a hack for test programs like test-dump-untracked-cache to\n- * ensure that they do not modify the untracked cache when reading it.\n- * Do not use it otherwise!\n- */\n-extern int ignore_untracked_cache_config;\n-\n int committer_ident_sufficiently_given(void);\n int author_ident_sufficiently_given(void);\n \ndiff --git a/environment.c b/environment.c\nindex 7d8a949285c..d73dd0c42f7 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -96,13 +96,6 @@ int auto_comment_line_char;\n /* Parallel index stat data preload? */\n int core_preload_index = 1;\n \n-/*\n- * This is a hack for test programs like test-dump-untracked-cache to\n- * ensure that they do not modify the untracked cache when reading it.\n- * Do not use it otherwise!\n- */\n-int ignore_untracked_cache_config;\n-\n /* This is set by setup_git_dir_gently() and/or git_default_config() */\n char *git_work_tree_cfg;\n \ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0cfe8b787db..b0df8b93b86 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -70,12 +70,7 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n \n-\t/* Hack for test programs like test-dump-untracked-cache */\n-\tif (ignore_untracked_cache_config)\n-\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n-\telse\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n-\n+\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n \tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);\n \n \t/*\ndiff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c\nindex cf0f2c7228e..99010614f6d 100644\n--- a/t/helper/test-dump-untracked-cache.c\n+++ b/t/helper/test-dump-untracked-cache.c\n@@ -45,8 +45,10 @@ int cmd__dump_untracked_cache(int ac, const char **av)\n \tstruct untracked_cache *uc;\n \tstruct strbuf base = STRBUF_INIT;\n \n-\t/* Hack to avoid modifying the untracked cache when we read it */\n-\tignore_untracked_cache_config = 1;\n+\t/* Set core.untrackedCache=keep before setup_git_directory() */\n+\txsetenv(\"GIT_CONFIG_COUNT\", \"1\", 1);\n+\txsetenv(\"GIT_CONFIG_KEY_0\", \"core.untrackedCache\", 1);\n+\txsetenv(\"GIT_CONFIG_VALUE_0\", \"keep\", 1);\n \n \tsetup_git_directory();\n \tif (read_cache() < 0)\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436605","messageId":"patch-v4-3.5-b36b23ee173-20210921T131003Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com","subject":"[PATCH v4 3/5] read-cache & fetch-negotiator: check \"enum\" values in switch()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:13:01Z","receivedAt":"2021-09-21T13:13:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change tweak_untracked_cache() in \"read-cache.c\" to use a switch() to\nhave the compiler assert that we checked all possible values in the\n\"enum untracked_cache_setting\" type, and likewise remove the \"default\"\ncase in fetch_negotiator_init() in favor of checking for\n\"FETCH_NEGOTIATION_UNSET\" and \"FETCH_NEGOTIATION_NONE\".\n\nAs will be discussed in a subsequent we'll only ever have either of\nthese set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and\nUNTRACKED_CACHE_UNSET within the prepare_repo_settings() function\nitself. In preparation for fixing that code let's add a BUG() here to\nmark this as unreachable code.\n\nSee ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)\nfor when the \"unset\" and \"keep\" handling for core.untrackedCache was\nconsolidated, and aaf633c2ad1 (repo-settings: create\nfeature.experimental setting, 2019-08-13) for the addition of the\n\"default\" pattern in \"fetch-negotiator.c\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fetch-negotiator.c | 4 +++-\n read-cache.c | 15 ++++++++++-----\n 2 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 57ed5784e14..e61e40cb299 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -19,8 +19,10 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \n \tcase FETCH_NEGOTIATION_DEFAULT:\n-\tdefault:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n+\tcase FETCH_NEGOTIATION_NONE:\n+\tcase FETCH_NEGOTIATION_UNSET:\n+\t\tBUG(\"FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!\");\n \t}\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex f5d4385c408..e4ae35ccdb2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1944,13 +1944,18 @@ static void tweak_untracked_cache(struct index_state *istate)\n \n \tprepare_repo_settings(r);\n \n-\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_REMOVE) {\n+\tswitch (r->settings.core_untracked_cache) {\n+\tcase UNTRACKED_CACHE_REMOVE:\n \t\tremove_untracked_cache(istate);\n-\t\treturn;\n-\t}\n-\n-\tif (r->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE)\n+\t\tbreak;\n+\tcase UNTRACKED_CACHE_WRITE:\n \t\tadd_untracked_cache(istate);\n+\t\tbreak;\n+\tcase UNTRACKED_CACHE_KEEP:\n+\t\tbreak;\n+\tcase UNTRACKED_CACHE_UNSET:\n+\t\tBUG(\"UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!\");\n+\t}\n }\n \n static void tweak_split_index(struct index_state *istate)\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436606","messageId":"patch-v4-4.5-c9f143b26f1-20210921T131003Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com","subject":"[PATCH v4 4/5] repo-settings.c: simplify the setup","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:13:02Z","receivedAt":"2021-09-21T13:13:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Simplify the setup code in repo-settings.c in various ways, making the\ncode shorter, easier to read, and requiring fewer hacks to do the same\nthing as it did before:\n\nSince 7211b9e7534 (repo-settings: consolidate some config settings,\n2019-08-13) we have memset() the whole \"settings\" structure to -1 in\nprepare_repo_settings(), and subsequently relied on the -1 value.\n\nMost of the fields did not need to be initialized to -1, and because\nwe were doing that we had the enum labels \"UNTRACKED_CACHE_UNSET\" and\n\"FETCH_NEGOTIATION_UNSET\" purely to reflect the resulting state\ncreated this memset() in prepare_repo_settings(). No other code used\nor relied on them, more on that below.\n\nFor the rest most of the subsequent \"are we -1, then read xyz\" can\nsimply be removed by re-arranging what we read first. E.g. when\nsetting the \"index.version\" setting we should have first read\n\"feature.experimental\", so that it (and \"feature.manyfiles\") can\nprovide a default for our \"index.version\".\n\nInstead the code setting it, added when \"feature.manyFiles\"[1] was\ncreated, was using the UPDATE_DEFAULT_BOOL() macro added in an earlier\ncommit[2]. That macro is now gone, since it was only needed for this\npattern of reading things in the wrong order.\n\nThis also fixes an (admittedly obscure) logic error where we'd\nconflate an explicit \"-1\" value in the config with our own earlier\nmemset() -1.\n\nWe can also remove the UPDATE_DEFAULT_BOOL() wrapper added in\n[3]. Using it is redundant to simply using the return value from\nrepo_config_get_bool(), which is non-zero if the provided key exists\nin the config.\n\nDetails on edge cases relating to the memset() to -1, continued from\n\"more on that below\" above:\n\n * UNTRACKED_CACHE_KEEP:\n\n In [4] the \"unset\" and \"keep\" handling for core.untrackedCache was\n consolidated. But it while we understand the \"keep\" value, we don't\n handle it differently than the case of any other unknown value.\n\n So let's retain UNTRACKED_CACHE_KEEP and remove the\n UNTRACKED_CACHE_UNSET setting (which was always implicitly\n UNTRACKED_CACHE_KEEP before). We don't need to inform any code\n after prepare_repo_settings() that the setting was \"unset\", as far\n as anyone else is concerned it's core.untrackedCache=keep. if\n \"core.untrackedcache\" isn't present in the config.\n\n * FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:\n\n Since these two two enum fields added in [5] don't rely on the\n memzero() setting them to \"-1\" anymore we don't have to provide\n them with explicit values.\n\n1. c6cc4c5afd2 (repo-settings: create feature.manyFiles setting,\n 2019-08-13)\n2. 31b1de6a09b (commit-graph: turn on commit-graph by default,\n 2019-08-13)\n3. 31b1de6a09b (commit-graph: turn on commit-graph by default,\n 2019-08-13)\n4. ad0fb659993 (repo-settings: parse core.untrackedCache,\n 2019-08-13)\n5. aaf633c2ad1 (repo-settings: create feature.experimental setting,\n 2019-08-13)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fetch-negotiator.c | 3 --\n read-cache.c | 8 +++-\n repo-settings.c | 102 +++++++++++++++++++++++++--------------------\n repository.h | 15 +++----\n 4 files changed, 68 insertions(+), 60 deletions(-)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex e61e40cb299..273390229fe 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -21,8 +21,5 @@ void fetch_negotiator_init(struct repository *r,\n \tcase FETCH_NEGOTIATION_DEFAULT:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n-\tcase FETCH_NEGOTIATION_NONE:\n-\tcase FETCH_NEGOTIATION_UNSET:\n-\t\tBUG(\"FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!\");\n \t}\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex e4ae35ccdb2..b0a06db5c55 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1952,9 +1952,13 @@ static void tweak_untracked_cache(struct index_state *istate)\n \t\tadd_untracked_cache(istate);\n \t\tbreak;\n \tcase UNTRACKED_CACHE_KEEP:\n+\t\t/*\n+\t\t * Either an explicit \"core.untrackedCache=keep\", the\n+\t\t * default if \"core.untrackedCache\" isn't configured,\n+\t\t * or a fallback on an unknown \"core.untrackedCache\"\n+\t\t * value.\n+\t\t */\n \t\tbreak;\n-\tcase UNTRACKED_CACHE_UNSET:\n-\t\tBUG(\"UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!\");\n \t}\n }\n \ndiff --git a/repo-settings.c b/repo-settings.c\nindex b0df8b93b86..46b9d56aeac 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -3,40 +3,76 @@\n #include \"repository.h\"\n #include \"midx.h\"\n \n-#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)\n+static void repo_cfg_bool(struct repository *r, const char *key, int *dest,\n+\t\t\t int def)\n+{\n+\tif (repo_config_get_bool(r, key, dest))\n+\t\t*dest = def;\n+}\n \n void prepare_repo_settings(struct repository *r)\n {\n+\tint experimental;\n \tint value;\n \tchar *strval;\n+\tint manyfiles;\n \n-\tif (r->settings.initialized)\n+\tif (r->settings.initialized++)\n \t\treturn;\n \n \t/* Defaults */\n-\tmemset(&r->settings, -1, sizeof(r->settings));\n+\tr->settings.index_version = -1;\n+\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n+\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\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 \n-\tif (!repo_config_get_bool(r, \"core.commitgraph\", &value))\n-\t\tr->settings.core_commit_graph = value;\n-\tif (!repo_config_get_bool(r, \"commitgraph.readchangedpaths\", &value))\n-\t\tr->settings.commit_graph_read_changed_paths = value;\n-\tif (!repo_config_get_bool(r, \"gc.writecommitgraph\", &value))\n-\t\tr->settings.gc_write_commit_graph = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);\n-\tUPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);\n-\tUPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);\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+\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+\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/*\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-\tif (!repo_config_get_maybe_bool(r, \"core.untrackedcache\", &value)) {\n-\t\tif (value == 0)\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_REMOVE;\n-\t\telse\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;\n-\t} else if (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n-\t\tif (!strcasecmp(strval, \"keep\"))\n-\t\t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\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@@ -45,34 +81,8 @@ void prepare_repo_settings(struct repository *r)\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\n-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n \n-\tif (!repo_config_get_bool(r, \"pack.usesparse\", &value))\n-\t\tr->settings.pack_use_sparse = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n-\n-\tvalue = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);\n-\tif (value || !repo_config_get_bool(r, \"core.multipackindex\", &value))\n-\t\tr->settings.core_multi_pack_index = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);\n-\n-\tif (!repo_config_get_bool(r, \"feature.manyfiles\", &value) && value) {\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n-\t}\n-\n-\tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n-\t\tr->settings.fetch_write_commit_graph = value;\n-\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0);\n-\n-\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n-\n-\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);\n-\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);\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\ndiff --git a/repository.h b/repository.h\nindex 3740c93bc0f..bf36744e0a7 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -13,18 +13,15 @@ struct submodule_cache;\n struct promisor_remote_config;\n \n enum untracked_cache_setting {\n-\tUNTRACKED_CACHE_UNSET = -1,\n-\tUNTRACKED_CACHE_REMOVE = 0,\n-\tUNTRACKED_CACHE_KEEP = 1,\n-\tUNTRACKED_CACHE_WRITE = 2\n+\tUNTRACKED_CACHE_KEEP,\n+\tUNTRACKED_CACHE_REMOVE,\n+\tUNTRACKED_CACHE_WRITE,\n };\n \n enum fetch_negotiation_setting {\n-\tFETCH_NEGOTIATION_UNSET = -1,\n-\tFETCH_NEGOTIATION_NONE = 0,\n-\tFETCH_NEGOTIATION_DEFAULT = 1,\n-\tFETCH_NEGOTIATION_SKIPPING = 2,\n-\tFETCH_NEGOTIATION_NOOP = 3,\n+\tFETCH_NEGOTIATION_DEFAULT,\n+\tFETCH_NEGOTIATION_SKIPPING,\n+\tFETCH_NEGOTIATION_NOOP,\n };\n \n struct repo_settings {\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436607","messageId":"patch-v4-5.5-aadd4c42923-20210921T131003Z-avarab@gmail.com","threadId":"56517","inReplyTo":"cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com","subject":"[PATCH v4 5/5] repository.h: don't use a mix of int and bitfields","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:13:03Z","receivedAt":"2021-09-21T13:13:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the bitfield added in 58300f47432 (sparse-index: add\nindex.sparse config option, 2021-03-30) and 3964fc2aae7 (sparse-index:\nadd guard to ensure full index, 2021-03-30) to just use an \"int\"\nboolean instead.\n\nIt might be smart to optimize the space here in the future, but by\nconsistently using an \"int\" we can take its address and pass it to\nrepo_cfg_bool(), and therefore don't need to handle \"sparse_index\" as\na special-case when reading the \"index.sparse\" setting.\n\nThere's no corresponding config for \"command_requires_full_index\", but\nlet's change it too for consistency and to prevent future bugs\ncreeping in due to one of these being \"unsigned\".\n\nUsing \"int\" consistently also prevents subtle bugs or undesired\ncontrol flow creeping in here. Before the preceding commit the\ninitialization of \"command_requires_full_index\" in\nprepare_repo_settings() did nothing, i.e. this:\n\n r->settings.command_requires_full_index = 1\n\nWas redundant to the earlier memset() to -1. Likewise for\n\"sparse_index\" added in 58300f47432 (sparse-index: add index.sparse\nconfig option, 2021-03-30) the code and comment added there was\nmisleading, we weren't initializing it to off, but re-initializing it\nfrom \"1\" to \"0\", and then finally checking the config, and perhaps\nsetting it to \"1\" again. I.e. we could have applied this patch before\nthe preceding commit:\n\n\t+\tassert(r->settings.command_requires_full_index == 1);\n\t \tr->settings.command_requires_full_index = 1;\n\n\t \t/*\n\t \t * Initialize this as off.\n\t \t */\n\t+\tassert(r->settings.sparse_index == 1);\n\t \tr->settings.sparse_index = 0;\n\t \tif (!repo_config_get_bool(r, \"index.sparse\", &value) && value)\n\t \t\tr->settings.sparse_index = 1;\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n repo-settings.c | 8 +-------\n repository.h | 5 ++---\n 2 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 46b9d56aeac..b93e91a212e 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -45,6 +45,7 @@ void prepare_repo_settings(struct repository *r)\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 \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\n@@ -90,11 +91,4 @@ void prepare_repo_settings(struct repository *r)\n \t * removed.\n \t */\n \tr->settings.command_requires_full_index = 1;\n-\n-\t/*\n-\t * Initialize this as off.\n-\t */\n-\tr->settings.sparse_index = 0;\n-\tif (!repo_config_get_bool(r, \"index.sparse\", &value) && value)\n-\t\tr->settings.sparse_index = 1;\n }\ndiff --git a/repository.h b/repository.h\nindex bf36744e0a7..02599ae2c98 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -31,6 +31,8 @@ struct repo_settings {\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n \tint fetch_write_commit_graph;\n+\tint command_requires_full_index;\n+\tint sparse_index;\n \n \tint index_version;\n \tenum untracked_cache_setting core_untracked_cache;\n@@ -39,9 +41,6 @@ struct repo_settings {\n \tenum fetch_negotiation_setting fetch_negotiation_algorithm;\n \n \tint core_multi_pack_index;\n-\n-\tunsigned command_requires_full_index:1,\n-\t\t sparse_index:1;\n };\n \n struct repository {\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436630","messageId":"d852d412-4d6a-505f-2c4c-52e81ab4e3f4@gmail.com","threadId":"56517","inReplyTo":"cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com","subject":"Re: [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-21T15:58:34Z","receivedAt":"2021-09-21T15:58:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:\n> A hopefully final re-roll addressing Taylor's v3 review, except for\n> the suggestion (that I read as) perhaps retaining the test-only code,\n> which I've decided not to do per\n> http://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com\n> \n> The x(un)setenv() now returns void, and the error messages are less\n> chatty, I also improved a BUG() message in 4/5 that we end up deleting\n> in 5/5 anyway, so it doesn't matter for the end-state, just for\n> understanding the patches.\n> \n> Ævar Arnfjörð Bjarmason (5):\n> wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c\n> environment.c: remove test-specific \"ignore_untracked...\" variable\n> read-cache & fetch-negotiator: check \"enum\" values in switch()\n> repo-settings.c: simplify the setup\n> repository.h: don't use a mix of int and bitfields\n\nThis version looks good to me.\n\nThanks,\n-Stolee\n"},{"id":"436665","messageId":"YUpEubyYwD+Qe+oE@nand.local","threadId":"56517","inReplyTo":"d852d412-4d6a-505f-2c4c-52e81ab4e3f4@gmail.com","subject":"Re: [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T20:46:49Z","receivedAt":"2021-09-21T20:46:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 21, 2021 at 11:58:34AM -0400, Derrick Stolee wrote:\n> On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:\n> > A hopefully final re-roll addressing Taylor's v3 review, except for\n> > the suggestion (that I read as) perhaps retaining the test-only code,\n> > which I've decided not to do per\n> > http://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com\n> >\n> > [...]\n>\n> This version looks good to me.\n\nLikewise. Thanks Ævar for addressing my review.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"436776","messageId":"xmqqtuic5pgn.fsf@gitster.g","threadId":"56517","inReplyTo":"YUpEubyYwD+Qe+oE@nand.local","subject":"Re: [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T20:23:52Z","receivedAt":"2021-09-22T20:23:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Tue, Sep 21, 2021 at 11:58:34AM -0400, Derrick Stolee wrote:\n>> On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:\n>> > A hopefully final re-roll addressing Taylor's v3 review, except for\n>> > the suggestion (that I read as) perhaps retaining the test-only code,\n>> > which I've decided not to do per\n>> > http://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com\n>> >\n>> > [...]\n>>\n>> This version looks good to me.\n>\n> Likewise. Thanks Ævar for addressing my review.\n\nYeah, I think this one is good enough for 'next'.\n\nThanks, all.\n"}]}