Re: [PATCH 3/5] repo-settings: parse core.untrackedCache

[Derrick Stolee <stolee@gmail.com>]

On 7/23/2019 11:04 AM, Johannes Schindelin wrote:
> Hi Stolee,
> 
> On Mon, 22 Jul 2019, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The core.untrackedCache config setting is slightly complicated,
>> so clarify its use and centralize its parsing into the repo
>> settings.
>>
>> The default value is "keep" (returned as -1), which persists the
>> untracked cache if it exists.
>>
>> If the value is set as "false" (returned as 0), then remove the
>> untracked cache if it exists.
>>
>> If the value is set as "true" (returned as 1), then write the
>> untracked cache and persist it.
>>
>> Instead of relying on magic values of -1, 0, and 1, split these
>> options into bitflags CORE_UNTRACKED_CACHE_KEEP and
>> CORE_UNTRACKED_CACHE_WRITE. This allows the use of "-1" as a
>> default value. After parsing the config options, if the value is
>> unset we can initialize it to CORE_UNTRACKED_CACHE_KEEP.
> 
> Nice!
> 
>> [...]
>> diff --git a/read-cache.c b/read-cache.c
>> index ee1aaa8917..e67e6f6e3e 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1846,18 +1846,17 @@ static void check_ce_order(struct index_state *istate)
>>
>>  static void tweak_untracked_cache(struct index_state *istate)
>>  {
>> -	switch (git_config_get_untracked_cache()) {
>> -	case -1: /* keep: do nothing */
>> -		break;
>> -	case 0: /* false */
>> +	struct repository *r = the_repository;
>> +
>> +	prepare_repo_settings(r);
>> +
>> +	if (!(r->settings->core_untracked_cache & CORE_UNTRACKED_CACHE_KEEP)) {
>>  		remove_untracked_cache(istate);
>> -		break;
>> -	case 1: /* true */
>> -		add_untracked_cache(istate);
>> -		break;
>> -	default: /* unknown value: do nothing */
>> -		break;
>> +		return;
>>  	}
>> +
>> +	if (r->settings->core_untracked_cache & CORE_UNTRACKED_CACHE_WRITE)
>> +		add_untracked_cache(istate);
> 
> This changes the flow in a subtle way: in the
> `CORE_UNTRACKED_CACHE_WRITE` case, we used to _not_ remove the untracked
> cache, but now we do.
> 
> I _think_ what you would want to do is replace the `!(..._KEEP)`
> condition by `..._REMOVE`.

I believe the code as written is correct, but confusing. The value is not an enum, but instead a bitflag. When the config setting is given as "true", then both _KEEP and _WRITE are set, so the flow is identical.

However, you already suggested switching to an enum, in which case using _REMOVE would be clearer.

> 
>>  }
>>
>>  static void tweak_split_index(struct index_state *istate)
>> diff --git a/repo-settings.c b/repo-settings.c
>> index f328602fd7..807c5a29d6 100644
>> --- a/repo-settings.c
>> +++ b/repo-settings.c
>> @@ -30,6 +30,20 @@ static int git_repo_config(const char *key, const char *value, void *cb)
>>  		rs->index_version = git_config_int(key, value);
>>  		return 0;
>>  	}
>> +	if (!strcmp(key, "core.untrackedcache")) {
>> +		int bool_value = git_parse_maybe_bool(value);
>> +		if (bool_value == 0)
>> +			rs->core_untracked_cache = 0;
>> +		else if (bool_value == 1)
>> +			rs->core_untracked_cache = CORE_UNTRACKED_CACHE_KEEP |
>> +						   CORE_UNTRACKED_CACHE_WRITE;
>> +		else if (!strcasecmp(value, "keep"))
>> +			rs->core_untracked_cache = CORE_UNTRACKED_CACHE_KEEP;
>> +		else
>> +			error(_("unknown core.untrackedCache value '%s'; "
>> +				"using 'keep' default value"), value);
>> +		return 0;
>> +	}
>>
>>  	return 1;
>>  }
>> @@ -46,6 +60,13 @@ void prepare_repo_settings(struct repository *r)
>>  	r->settings->gc_write_commit_graph = -1;
>>  	r->settings->pack_use_sparse = -1;
>>  	r->settings->index_version = -1;
>> +	r->settings->core_untracked_cache = -1;
> 
> At this point at the latest, I am starting to wonder whether it would
> not make more sense to use `memset(..., -1, sizeof(struct
> repo_settings)` instead.
> 
>>
>>  	repo_config(r, git_repo_config, r->settings);
>> +
>> +	/* Hack for test programs like test-dump-untracked-cache */
>> +	if (ignore_untracked_cache_config)
>> +		r->settings->core_untracked_cache = CORE_UNTRACKED_CACHE_KEEP;
>> +	else
>> +		UPDATE_DEFAULT(r->settings->core_untracked_cache, CORE_UNTRACKED_CACHE_KEEP);
>>  }
>> diff --git a/repo-settings.h b/repo-settings.h
>> index 1151c2193a..bac9b87d49 100644
>> --- a/repo-settings.h
>> +++ b/repo-settings.h
>> @@ -1,11 +1,15 @@
>>  #ifndef REPO_SETTINGS_H
>>  #define REPO_SETTINGS_H
>>
>> +#define CORE_UNTRACKED_CACHE_WRITE (1 << 0)
>> +#define CORE_UNTRACKED_CACHE_KEEP (1 << 1)
> 
> I think it would read even nicer as an enum. In any case, using `1<<1`
> suggests that this is a bit field, but I don't think that is what we
> actually want here. Instead, what `core_untracked_cache` seems to be (at
> least from my point of view) is a mode, where any two modes are mutually
> exclusive.
> 
> For example, what is the difference between `(_KEEP | _WRITE)` and
> `(_WRITE)` supposed to be? That the latter writes a fresh untracked
> cache without looking at the previous one? That's not how the existing
> code behaves, though...

Yes, there is no reason to have _WRITE without also _KEEP. An enum is better. Thanks!

> 
> Ciao,
> Dscho
> 
>> +
>>  struct repo_settings {
>>  	int core_commit_graph;
>>  	int gc_write_commit_graph;
>>  	int pack_use_sparse;
>>  	int index_version;
>> +	int core_untracked_cache;
>>  };
>>
>>  struct repository;
>> --
>> gitgitgadget
>>
>>