| Message ID | 47ae3e7d4d765a00d14e8892db88a8936d56591b.1563818059.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Create 'feature.*' config area and some centralized config parsing | expand |
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 > >
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 >> >>
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;