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