| Message ID | patch-v3-2.5-ece340af764-20210919T084703Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | repo-settings.c: refactor for clarity, get rid of hacks etc. | expand |
On Sun, Sep 19, 2021 at 10:47:16AM +0200, Ævar Arnfjörð Bjarmason wrote: > Instead of the global ignore_untracked_cache_config variable added in > dae6c322fa1 (test-dump-untracked-cache: don't modify the untracked > cache, 2016-01-27) we can make use of the new facility to set config > via environment variables added in d8d77153eaf (config: allow > specifying config entries via envvar pairs, 2021-01-12). > > It's arguably a bit hacky to use setenv() and getenv() to pass > messages between the same program, but since the test helpers are not > the main intended audience of repo-settings.c I think it's better than > hardcoding the test-only special-case in prepare_repo_settings(). Hmm. I tend to agree that using (a wrapper over) setenv() to pass messages between the test helper and the rest of Git is a little bit of a hack. Everything you wrote should work based on my understanding of the config-over-environment-variable stuff added recently. But I wish that it didn't involve losing some grep-ability between the test-helper and library code. So I wouldn't be sad to see this patch get dropped, and I also wouldn't be overly sad to see it get picked up, either. But I don't think it's a necessary step, and we may be slightly better without it. Thanks, Taylor
On Mon, Sep 20 2021, Taylor Blau wrote: > On Sun, Sep 19, 2021 at 10:47:16AM +0200, Ævar Arnfjörð Bjarmason wrote: >> Instead of the global ignore_untracked_cache_config variable added in >> dae6c322fa1 (test-dump-untracked-cache: don't modify the untracked >> cache, 2016-01-27) we can make use of the new facility to set config >> via environment variables added in d8d77153eaf (config: allow >> specifying config entries via envvar pairs, 2021-01-12). >> >> It's arguably a bit hacky to use setenv() and getenv() to pass >> messages between the same program, but since the test helpers are not >> the main intended audience of repo-settings.c I think it's better than >> hardcoding the test-only special-case in prepare_repo_settings(). > > Hmm. I tend to agree that using (a wrapper over) setenv() to pass > messages between the test helper and the rest of Git is a little bit of > a hack. > > Everything you wrote should work based on my understanding of the > config-over-environment-variable stuff added recently. But I wish that > it didn't involve losing some grep-ability between the test-helper and > library code. Does that grep-ability between the two have any reason to exist? The only reason we need this special-case in the test helper is because it's not setting up "normal" config. It could also be made to do so, that's a bigger behavior change than this narrow change, but likewise we'd just end up with a "git config core.untrackedCache keep" in some test *.sh somewhere, and no grep-ability between the test helper and library code. But now that we have GIT_CONFIG_COUNT etc. using the environment has become a perfectly fine way to pass this data along, we could also do that in the *.sh setup, but this was easier, and also easier to guarantee correctness with the new x*() wrapper. IOW just because it's called t/helper/test-dump-untracked-cache.c it really doesn't have any business reaching into the guts of repo-settings.c to tweak how we set up core.untrackedCache. The only reason it did was because the code pre-dated the GIT_CONFIG_{COUNT,KEY,VALUE} implementation. > So I wouldn't be sad to see this patch get dropped, and I also wouldn't > be overly sad to see it get picked up, either. But I don't think it's a > necessary step, and we may be slightly better without it. > > Thanks, > Taylor
diff --git a/cache.h b/cache.h index d23de693680..8e60fdd2a12 100644 --- a/cache.h +++ b/cache.h @@ -1719,13 +1719,6 @@ int update_server_info(int); const char *get_log_output_encoding(void); const char *get_commit_output_encoding(void); -/* - * This is a hack for test programs like test-dump-untracked-cache to - * ensure that they do not modify the untracked cache when reading it. - * Do not use it otherwise! - */ -extern int ignore_untracked_cache_config; - int committer_ident_sufficiently_given(void); int author_ident_sufficiently_given(void); diff --git a/environment.c b/environment.c index 7d8a949285c..d73dd0c42f7 100644 --- a/environment.c +++ b/environment.c @@ -96,13 +96,6 @@ int auto_comment_line_char; /* Parallel index stat data preload? */ int core_preload_index = 1; -/* - * This is a hack for test programs like test-dump-untracked-cache to - * ensure that they do not modify the untracked cache when reading it. - * Do not use it otherwise! - */ -int ignore_untracked_cache_config; - /* This is set by setup_git_dir_gently() and/or git_default_config() */ char *git_work_tree_cfg; diff --git a/repo-settings.c b/repo-settings.c index 0cfe8b787db..b0df8b93b86 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -70,12 +70,7 @@ void prepare_repo_settings(struct repository *r) if (!repo_config_get_bool(r, "feature.experimental", &value) && value) UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING); - /* Hack for test programs like test-dump-untracked-cache */ - if (ignore_untracked_cache_config) - r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP; - else - UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP); - + UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP); UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT); /* diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c index cf0f2c7228e..99010614f6d 100644 --- a/t/helper/test-dump-untracked-cache.c +++ b/t/helper/test-dump-untracked-cache.c @@ -45,8 +45,10 @@ int cmd__dump_untracked_cache(int ac, const char **av) struct untracked_cache *uc; struct strbuf base = STRBUF_INIT; - /* Hack to avoid modifying the untracked cache when we read it */ - ignore_untracked_cache_config = 1; + /* Set core.untrackedCache=keep before setup_git_directory() */ + xsetenv("GIT_CONFIG_COUNT", "1", 1); + xsetenv("GIT_CONFIG_KEY_0", "core.untrackedCache", 1); + xsetenv("GIT_CONFIG_VALUE_0", "keep", 1); setup_git_directory(); if (read_cache() < 0)
Instead of the global ignore_untracked_cache_config variable added in dae6c322fa1 (test-dump-untracked-cache: don't modify the untracked cache, 2016-01-27) we can make use of the new facility to set config via environment variables added in d8d77153eaf (config: allow specifying config entries via envvar pairs, 2021-01-12). It's arguably a bit hacky to use setenv() and getenv() to pass messages between the same program, but since the test helpers are not the main intended audience of repo-settings.c I think it's better than hardcoding the test-only special-case in prepare_repo_settings(). This uses the xsetenv() wrapper added in the preceding commit, if we don't set these in the environment we'll fail in t7063-status-untracked-cache.sh, but let's fail earlier anyway if that were to happen. This breaks any parent process that's potentially using the GIT_CONFIG_* and GIT_CONFIG_PARAMETERS mechanism to pass one-shot config setting down to a git subprocess, but in this case we don't care about the general case of such potential parents. This process neither spawns other "git" processes, nor is it interested in other configuration. We might want to pick up other test modes here, but those will be passed via GIT_TEST_* environment variables. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- cache.h | 7 ------- environment.c | 7 ------- repo-settings.c | 7 +------ t/helper/test-dump-untracked-cache.c | 6 ++++-- 4 files changed, 5 insertions(+), 22 deletions(-)