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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2016, 20:16 UTC
Message-ID: <xmqqa8o6ha3e.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1452841192-620-11-git-send-email-chriscool@tuxfamily.org>

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

Show 10 quoted lines

> 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.

Show 44 quoted lines

> 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");