[3/5] repo-settings: parse core.untrackedCache
diff mbox series

Message ID 47ae3e7d4d765a00d14e8892db88a8936d56591b.1563818059.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Create 'feature.*' config area and some centralized config parsing
Related show

Commit Message

Berislav Lopac via GitGitGadget July 22, 2019, 5:54 p.m. UTC
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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/update-index.c |  7 +++++--
 config.c               | 24 ------------------------
 read-cache.c           | 19 +++++++++----------
 repo-settings.c        | 21 +++++++++++++++++++++
 repo-settings.h        |  4 ++++
 5 files changed, 39 insertions(+), 36 deletions(-)

Comments

Johannes Schindelin July 23, 2019, 3:04 p.m. UTC | #1
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`.

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

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
>
>
Derrick Stolee July 24, 2019, 7:27 p.m. UTC | #2
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
>>
>>

Patch
diff mbox series

diff --git a/builtin/update-index.c b/builtin/update-index.c
index dff2f4b837..6c26bbae80 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -18,6 +18,7 @@ 
 #include "dir.h"
 #include "split-index.h"
 #include "fsmonitor.h"
+#include "repo-settings.h"
 
 /*
  * Default to not allowing changes to the list of files. The
@@ -966,6 +967,7 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 	struct parse_opt_ctx_t ctx;
 	strbuf_getline_fn getline_fn;
 	int parseopt_state = PARSE_OPT_UNKNOWN;
+	struct repository *r = the_repository;
 	struct option options[] = {
 		OPT_BIT('q', NULL, &refresh_args.flags,
 			N_("continue refresh even when index needs update"),
@@ -1180,11 +1182,12 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 		remove_split_index(&the_index);
 	}
 
+	prepare_repo_settings(r);
 	switch (untracked_cache) {
 	case UC_UNSPECIFIED:
 		break;
 	case UC_DISABLE:
-		if (git_config_get_untracked_cache() == 1)
+		if (r->settings->core_untracked_cache & CORE_UNTRACKED_CACHE_WRITE)
 			warning(_("core.untrackedCache is set to true; "
 				  "remove or change it, if you really want to "
 				  "disable the untracked cache"));
@@ -1196,7 +1199,7 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 		return !test_if_untracked_cache_is_supported();
 	case UC_ENABLE:
 	case UC_FORCE:
-		if (git_config_get_untracked_cache() == 0)
+		if (r->settings->core_untracked_cache == 0)
 			warning(_("core.untrackedCache is set to false; "
 				  "remove or change it, if you really want to "
 				  "enable the untracked cache"));
diff --git a/config.c b/config.c
index faa57e436c..3241dbc54d 100644
--- a/config.c
+++ b/config.c
@@ -2277,30 +2277,6 @@  int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestam
 	return -1; /* thing exists but cannot be parsed */
 }
 
-int git_config_get_untracked_cache(void)
-{
-	int val = -1;
-	const char *v;
-
-	/* Hack for test programs like test-dump-untracked-cache */
-	if (ignore_untracked_cache_config)
-		return -1;
-
-	if (!git_config_get_maybe_bool("core.untrackedcache", &val))
-		return val;
-
-	if (!git_config_get_value("core.untrackedcache", &v)) {
-		if (!strcasecmp(v, "keep"))
-			return -1;
-
-		error(_("unknown core.untrackedCache value '%s'; "
-			"using 'keep' default value"), v);
-		return -1;
-	}
-
-	return -1; /* default value */
-}
-
 int git_config_get_split_index(void)
 {
 	int val;
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);
 }
 
 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;
 
 	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)
+
 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;