config API: add and use a "git_config_get()" family of functions

We already have the basic "git_config_get_value()" function and its "repo_*" and "configset" siblings to get a given "key" and assign the last key found to a provided "value". But some callers don't care about that value, but just want to use the return value of the "get_value()" function to check whether the key exist (or another non-zero return value). The immediate motivation for this is that a subsequent commit will need to change all callers of the "*_get_value_multi()" family of functions. In two cases here we (ab)used it to check whether we had any values for the given key, but didn't care about the return value. The rest of the callers here used various other config API functions to do the same, all of which resolved to the same underlying functions to provide the answer. Some of these were using either git_config_get_string() or git_config_get_string_tmp(), see fe4c750fb13 (submodule--helper: fix a configure_added_submodule() leak, 2022-09-01) for a recent example. We can now use a helper function that doesn't require a throwaway variable. We could have changed git_configset_get_value_multi() (and then git_config_get_value() etc.) to accept a "NULL" as a "dest" for all callers, but let's avoid changing the behavior of existing API users. Having an "unused" value that we throw away internal to config.c is cheap. A "NULL as optional dest" pattern is also more fragile, as the intent of the caller might be misinterpreted if he were to accidentally pass "NULL", e.g. when "dest" is passed in from another function. Another name for this function could have been "*_config_key_exists()", as suggested in [1]. That would work for all of these callers, and would currently be equivalent to this function, as the git_configset_get_value() API normalizes all non-zero return values to a "1". But adding that API would set us up to lose information, as e.g. if git_config_parse_key() in the underlying configset_find_element() fails we'd like to return -1, not 1. Let's change the underlying configset_find_element() function to support this use-case, we'll make further use of it in a subsequent commit where the git_configset_get_value_multi() function itself will expose this new return value. This still leaves various inconsistencies and clobbering or ignoring of the return value in place. E.g here we're modifying configset_add_value(), but ever since it was added in [2] we've been ignoring its "int" return value, but as we're changing the configset_find_element() it uses, let's have it faithfully ferry that "ret" along. Let's also use the "RESULT_MUST_BE_USED" macro introduced in [3] to assert that we're checking the return value of configset_find_element(). We're leaving the same change to configset_add_value() for some future series. Once we start paying attention to its return value we'd need to ferry it up as deep as do_config_from(), and would need to make least read_{,very_}early_config() and git_protected_config() return an "int" instead of "void". Let's leave that for now, and focus on the *_get_*() functions. 1. 3c8687a73ee (add `config_set` API for caching config-like files, 2014-07-28) 2. https://lore.kernel.org/git/xmqqczadkq9f.fsf@gitster.g/ 3. 1e8697b5c4e (submodule--helper: check repo{_submodule,}_init() return values, 2022-09-01), Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Mar 28, 2023 at 16:04 UTC b83efcecaf12884d2bbb72b8ef2e4528205a0b02
7 files changed +135 -18
builtin/gc.c
+1 -4
@@ -1493,7 +1493,6 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
1493 };
1494 int found = 0;
1495 const char *key = "maintenance.repo";
1496 - char *config_value;
1496 char *maintpath = get_maintpath();
1497 struct string_list_item *item;
1498 const struct string_list *list;
@@ -1508,9 +1507,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
1507 git_config_set("maintenance.auto", "false");
1508
1509 /* Set maintenance strategy, if unset */
1511 - if (!git_config_get_string("maintenance.strategy", &config_value))
1512 - free(config_value);
1513 - else
1510 + if (git_config_get("maintenance.strategy"))
1511 git_config_set("maintenance.strategy", "incremental");
1512
1513 list = git_config_get_value_multi(key);
builtin/submodule--helper.c
+3 -4
@@ -552,7 +552,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
552 * If there are no path args and submodule.active is set then,
553 * by default, only initialize 'active' modules.
554 */
555 - if (!argc && git_config_get_value_multi("submodule.active"))
555 + if (!argc && !git_config_get("submodule.active"))
556 module_list_active(&list);
557
558 info.prefix = prefix;
@@ -2726,7 +2726,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
2726 * If there are no path args and submodule.active is set then,
2727 * by default, only initialize 'active' modules.
2728 */
2729 - if (!argc && git_config_get_value_multi("submodule.active"))
2729 + if (!argc && !git_config_get("submodule.active"))
2730 module_list_active(&list);
2731
2732 info.prefix = opt.prefix;
@@ -3119,7 +3119,6 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con
3119 static void configure_added_submodule(struct add_data *add_data)
3120 {
3121 char *key;
3122 - const char *val;
3122 struct child_process add_submod = CHILD_PROCESS_INIT;
3123 struct child_process add_gitmodules = CHILD_PROCESS_INIT;
3124
@@ -3164,7 +3163,7 @@ static void configure_added_submodule(struct add_data *add_data)
3163 * is_submodule_active(), since that function needs to find
3164 * out the value of "submodule.active" again anyway.
3165 */
3167 - if (!git_config_get_string_tmp("submodule.active", &val)) {
3166 + if (!git_config_get("submodule.active")) {
3167 /*
3168 * If the submodule being added isn't already covered by the
3169 * current configured pathspec, set the submodule's active flag
builtin/worktree.c
+1 -2
@@ -319,7 +319,6 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
319
320 if (file_exists(from_file)) {
321 struct config_set cs = { { 0 } };
322 - const char *core_worktree;
322 int bare;
323
324 if (safe_create_leading_directories(to_file) ||
@@ -338,7 +337,7 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
337 to_file, "core.bare", NULL, "true", 0))
338 error(_("failed to unset '%s' in '%s'"),
339 "core.bare", to_file);
341 - if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) &&
340 + if (!git_configset_get(&cs, "core.worktree") &&
341 git_config_set_in_file_gently(to_file,
342 "core.worktree", NULL))
343 error(_("failed to unset '%s' in '%s'"),
config.c
+44 -7
@@ -2275,23 +2275,29 @@ void read_very_early_config(config_fn_t cb, void *data)
2275 config_with_options(cb, data, NULL, &opts);
2276 }
2277
2278 -static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
2278 +RESULT_MUST_BE_USED
2279 +static int configset_find_element(struct config_set *cs, const char *key,
2280 + struct config_set_element **dest)
2281 {
2282 struct config_set_element k;
2283 struct config_set_element *found_entry;
2284 char *normalized_key;
2285 + int ret;
2286 +
2287 /*
2288 * `key` may come from the user, so normalize it before using it
2289 * for querying entries from the hashmap.
2290 */
2287 - if (git_config_parse_key(key, &normalized_key, NULL))
2288 - return NULL;
2291 + ret = git_config_parse_key(key, &normalized_key, NULL);
2292 + if (ret)
2293 + return ret;
2294
2295 hashmap_entry_init(&k.ent, strhash(normalized_key));
2296 k.key = normalized_key;
2297 found_entry = hashmap_get_entry(&cs->config_hash, &k, ent, NULL);
2298 free(normalized_key);
2294 - return found_entry;
2299 + *dest = found_entry;
2300 + return 0;
2301 }
2302
2303 static int configset_add_value(struct config_set *cs, const char *key, const char *value)
@@ -2300,8 +2306,11 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
2306 struct string_list_item *si;
2307 struct configset_list_item *l_item;
2308 struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
2309 + int ret;
2310
2304 - e = configset_find_element(cs, key);
2311 + ret = configset_find_element(cs, key, &e);
2312 + if (ret)
2313 + return ret;
2314 /*
2315 * Since the keys are being fed by git_config*() callback mechanism, they
2316 * are already normalized. So simply add them without any further munging.
@@ -2411,8 +2420,25 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *
2420
2421 const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
2422 {
2414 - struct config_set_element *e = configset_find_element(cs, key);
2415 - return e ? &e->value_list : NULL;
2423 + struct config_set_element *e;
2424 +
2425 + if (configset_find_element(cs, key, &e))
2426 + return NULL;
2427 + else if (!e)
2428 + return NULL;
2429 + return &e->value_list;
2430 +}
2431 +
2432 +int git_configset_get(struct config_set *cs, const char *key)
2433 +{
2434 + struct config_set_element *e;
2435 + int ret;
2436 +
2437 + if ((ret = configset_find_element(cs, key, &e)))
2438 + return ret;
2439 + else if (!e)
2440 + return 1;
2441 + return 0;
2442 }
2443
2444 int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
@@ -2551,6 +2577,12 @@ void repo_config(struct repository *repo, config_fn_t fn, void *data)
2577 configset_iter(repo->config, fn, data);
2578 }
2579
2580 +int repo_config_get(struct repository *repo, const char *key)
2581 +{
2582 + git_config_check_init(repo);
2583 + return git_configset_get(repo->config, key);
2584 +}
2585 +
2586 int repo_config_get_value(struct repository *repo,
2587 const char *key, const char **value)
2588 {
@@ -2665,6 +2697,11 @@ void git_config_clear(void)
2697 repo_config_clear(the_repository);
2698 }
2699
2700 +int git_config_get(const char *key)
2701 +{
2702 + return repo_config_get(the_repository, key);
2703 +}
2704 +
2705 int git_config_get_value(const char *key, const char **value)
2706 {
2707 return repo_config_get_value(the_repository, key, value);
config.h
+22
@@ -474,6 +474,13 @@ void git_configset_clear(struct config_set *cs);
474 * value in the 'dest' pointer.
475 */
476
477 +/**
478 + * git_configset_get() returns negative values on error, see
479 + * repo_config_get() below.
480 + */
481 +RESULT_MUST_BE_USED
482 +int git_configset_get(struct config_set *cs, const char *key);
483 +
484 /*
485 * Finds the highest-priority value for the configuration variable `key`
486 * and config set `cs`, stores the pointer to it in `value` and returns 0.
@@ -494,6 +501,14 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
501 /* Functions for reading a repository's config */
502 struct repository;
503 void repo_config(struct repository *repo, config_fn_t fn, void *data);
504 +
505 +/**
506 + * Run only the discover part of the repo_config_get_*() functions
507 + * below, in addition to 1 if not found, returns negative values on
508 + * error (e.g. if the key itself is invalid).
509 + */
510 +RESULT_MUST_BE_USED
511 +int repo_config_get(struct repository *repo, const char *key);
512 int repo_config_get_value(struct repository *repo,
513 const char *key, const char **value);
514 const struct string_list *repo_config_get_value_multi(struct repository *repo,
@@ -530,8 +545,15 @@ void git_protected_config(config_fn_t fn, void *data);
545 * manner, the config API provides two functions `git_config_get_value`
546 * and `git_config_get_value_multi`. They both read values from an internal
547 * cache generated previously from reading the config files.
548 + *
549 + * For those git_config_get*() functions that aren't documented,
550 + * consult the corresponding repo_config_get*() function's
551 + * documentation.
552 */
553
554 +RESULT_MUST_BE_USED
555 +int git_config_get(const char *key);
556 +
557 /**
558 * Finds the highest-priority value for the configuration variable `key`,
559 * stores the pointer to it in `value` and returns 0. When the
t/helper/test-config.c
+22
@@ -14,6 +14,8 @@
14 * get_value_multi -> prints all values for the entered key in increasing order
15 * of priority
16 *
17 + * get -> print return value for the entered key
18 + *
19 * get_int -> print integer value for the entered key or die
20 *
21 * get_bool -> print bool value for the entered key or die
@@ -109,6 +111,26 @@ int cmd__config(int argc, const char **argv)
111 printf("Value not found for \"%s\"\n", argv[2]);
112 goto exit1;
113 }
114 + } else if (argc == 3 && !strcmp(argv[1], "get")) {
115 + int ret;
116 +
117 + if (!(ret = git_config_get(argv[2])))
118 + goto exit0;
119 + else if (ret == 1)
120 + printf("Value not found for \"%s\"\n", argv[2]);
121 + else if (ret == -CONFIG_INVALID_KEY)
122 + printf("Key \"%s\" is invalid\n", argv[2]);
123 + else if (ret == -CONFIG_NO_SECTION_OR_NAME)
124 + printf("Key \"%s\" has no section\n", argv[2]);
125 + else
126 + /*
127 + * A normal caller should just check "ret <
128 + * 0", but for our own tests let's BUG() if
129 + * our whitelist of git_config_parse_key()
130 + * return values isn't exhaustive.
131 + */
132 + BUG("Key \"%s\" has unknown return %d", argv[2], ret);
133 + goto exit1;
134 } else if (argc == 3 && !strcmp(argv[1], "get_int")) {
135 if (!git_config_get_int(argv[2], &val)) {
136 printf("%d\n", val);
t/t1308-config-set.sh
+42 -1
@@ -58,6 +58,8 @@ test_expect_success 'setup default config' '
58 skin = false
59 nose = 1
60 horns
61 + [value]
62 + less
63 EOF
64 '
65
@@ -116,6 +118,45 @@ test_expect_success 'find value with the highest priority' '
118 check_config get_value case.baz "hask"
119 '
120
121 +test_expect_success 'return value for an existing key' '
122 + test-tool config get lamb.chop >out 2>err &&
123 + test_must_be_empty out &&
124 + test_must_be_empty err
125 +'
126 +
127 +test_expect_success 'return value for value-less key' '
128 + test-tool config get value.less >out 2>err &&
129 + test_must_be_empty out &&
130 + test_must_be_empty err
131 +'
132 +
133 +test_expect_success 'return value for a missing key' '
134 + cat >expect <<-\EOF &&
135 + Value not found for "missing.key"
136 + EOF
137 + test_expect_code 1 test-tool config get missing.key >actual 2>err &&
138 + test_cmp actual expect &&
139 + test_must_be_empty err
140 +'
141 +
142 +test_expect_success 'return value for a bad key: CONFIG_INVALID_KEY' '
143 + cat >expect <<-\EOF &&
144 + Key "fails.iskeychar.-" is invalid
145 + EOF
146 + test_expect_code 1 test-tool config get fails.iskeychar.- >actual 2>err &&
147 + test_cmp actual expect &&
148 + test_must_be_empty out
149 +'
150 +
151 +test_expect_success 'return value for a bad key: CONFIG_NO_SECTION_OR_NAME' '
152 + cat >expect <<-\EOF &&
153 + Key "keynosection" has no section
154 + EOF
155 + test_expect_code 1 test-tool config get keynosection >actual 2>err &&
156 + test_cmp actual expect &&
157 + test_must_be_empty out
158 +'
159 +
160 test_expect_success 'find integer value for a key' '
161 check_config get_int lamb.chop 65
162 '
@@ -272,7 +313,7 @@ test_expect_success 'proper error on error in default config files' '
313 cp .git/config .git/config.old &&
314 test_when_finished "mv .git/config.old .git/config" &&
315 echo "[" >>.git/config &&
275 - echo "fatal: bad config line 34 in file .git/config" >expect &&
316 + echo "fatal: bad config line 36 in file .git/config" >expect &&
317 test_expect_code 128 test-tool config get_value foo.bar 2>actual &&
318 test_cmp expect actual
319 '