Re: [PATCH v5 10/11] test-dump-untracked-cache: don't modify the untracked cache

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> diff --git a/cache.h b/cache.h
> index 59a15fd..89c7e10 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1605,6 +1605,8 @@ extern int git_config_get_maybe_bool(const char *key, int *dest);
>  extern int git_config_get_pathname(const char *key, const char **dest);
>  extern int git_config_get_untracked_cache(void);
>  
> +extern int ignore_untracked_cache_config;
> +

I know you said this is a hack to support the test, but I really do
not like to have a test-only global variable exposed to everybody
like this, as I do not think "ignore_untracked_cache_config" should
be necessary outside the context of testing [*1*].

If the config cache layer that is used by the implementation of
git_config_get_untracked_cache() had a way to be told to pretend
that the value of a particular configuration variable is a given
value, then we could do

	git_config_pretend_value("core.untrackedcache", "keep");

at the beginning of the test program without harming anybody else.

The above is just me "thinking aloud", without assessing if the
damage to the entire codebase with that approach to extend the
config layer would be larger than the damabe by this patch, and it
is certainly not a suggestion to redo this patch along that line.
But I am saying it aloud because it may turn out to be a good
direction to go in the larger picture once this series is done--it
may be a solution that is applicable to a class of similar problems
in a more general way.

Inside the scope of this series, can we at least add a comment next
to this variable telling everybody to never use it in normal
programs, or something?

Thanks.


[Footnote]

*1* Otherwise, we have a larger problem; it would mean that "we
trust the configuration variable better than the fact that the user
said the feature must (or must not) be used with this index file" is
flawed, which would contradict with the whole premise of this
series.


> diff --git a/config.c b/config.c
> index 647a15e..a4f70f7 100644
> --- a/config.c
> +++ b/config.c
> @@ -1599,6 +1599,9 @@ int git_config_get_untracked_cache(void)
>  	int val = -1;
>  	const char *v;
>  
> +	if (ignore_untracked_cache_config)
> +		return -1;
> +
>  	if (!git_config_get_maybe_bool("core.untrackedcache", &val))
>  		return val;
>  
> diff --git a/environment.c b/environment.c
> index 1cc4aab..74294ee 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -87,6 +87,12 @@ int auto_comment_line_char;
>  /* Parallel index stat data preload? */
>  int core_preload_index = 1;
>  
> +/*
> + * This is to ensure that the untracked cache is not modified, for
> + * example in test programs like test-dump-untracked-cache.
> + */
> +int ignore_untracked_cache_config;
> +
>  /* This is set by setup_git_dir_gently() and/or git_default_config() */
>  char *git_work_tree_cfg;
>  static char *work_tree;
> diff --git a/test-dump-untracked-cache.c b/test-dump-untracked-cache.c
> index 25d855d..8d1293c 100644
> --- a/test-dump-untracked-cache.c
> +++ b/test-dump-untracked-cache.c
> @@ -44,6 +44,8 @@ int main(int ac, char **av)
>  {
>  	struct untracked_cache *uc;
>  	struct strbuf base = STRBUF_INIT;
> +
> +	ignore_untracked_cache_config = 1;
>  	setup_git_directory();
>  	if (read_cache() < 0)
>  		die("unable to read index file");