This series fixes numerous segfaults in config API users, because they didn't expect *_get_multi() to hand them a string_list with a NULL in it given config like "[a] key" (note, no "="'s). A larger general overview at v1[1], but note the API changes in v2[2]. Changes since v2: * Rebased on the now-landed "rp/maintenance-qol", which had conflicts in builtin/gc.c. * Re-wrap some of the code properly at 79 characters. * Drop the stray submodule--helper from "for-each-repo", it was from a mistaken earlier rebase in v2. * Avoid minor whitespace changes in a test. * Don't change the BUG() message in config.c later in the series (another earlier rebase change when slimming down the v2). Junio: I'm sending this re-roll because I see "ab/config-multi-and-nonbool" got ejected (presumably due to the conflicts). Per [3] I think the "mixed bag" note in "What's Cooking" refers to the state of v1, which I tried to address in v2. 1. https://lore.kernel.org/git/cover-00.10-00000000000-20221026T151328Z-avarab@xxxxxxxxx/ 2. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20221101T225822Z-avarab@xxxxxxxxx/ 3. https://lore.kernel.org/git/221107.86leomx2dg.gmgdl@xxxxxxxxxxxxxxxxxxx/ CI & branch at: https://github.com/avar/git/tree/avar/have-git_configset_get_value-use-dest-and-int-pattern-3 Ævar Arnfjörð Bjarmason (9): for-each-repo tests: test bad --config keys config tests: cover blind spots in git_die_config() tests config tests: add "NULL" tests for *_get_value_multi() versioncmp.c: refactor config reading next commit config API: have *_multi() return an "int" and take a "dest" for-each-repo: error on bad --config config API users: test for *_get_value_multi() segfaults config API: add "string" version of *_value_multi(), fix segfaults for-each-repo: with bad config, don't conflate <path> and <cmd> builtin/for-each-repo.c | 14 ++--- builtin/gc.c | 10 ++-- builtin/log.c | 6 +- builtin/submodule--helper.c | 7 ++- config.c | 87 +++++++++++++++++++++++----- config.h | 50 +++++++++++++--- pack-bitmap.c | 6 +- submodule.c | 3 +- t/helper/test-config.c | 6 +- t/t0068-for-each-repo.sh | 19 ++++++ t/t1308-config-set.sh | 30 ++++++++++ t/t3309-notes-merge-auto-resolve.sh | 7 ++- t/t4202-log.sh | 15 +++++ t/t5304-prune.sh | 12 +++- t/t5310-pack-bitmaps.sh | 20 +++++++ t/t5552-skipping-fetch-negotiator.sh | 16 +++++ t/t7004-tag.sh | 17 ++++++ t/t7413-submodule-is-active.sh | 16 +++++ t/t7900-maintenance.sh | 38 ++++++++++++ versioncmp.c | 22 ++++--- 20 files changed, 339 insertions(+), 62 deletions(-) Range-diff against v2: 1: b8fd3bea4d1 = 1: 5c8819ff388 for-each-repo tests: test bad --config keys 2: 6cd0d6faf3c = 2: 3eb8da6086d config tests: cover blind spots in git_die_config() tests 3: f2a8766a802 = 3: 14b08dfc162 config tests: add "NULL" tests for *_get_value_multi() 4: 42cfc61202d = 4: cb802b30cd8 versioncmp.c: refactor config reading next commit 5: 48fb7cbf585 ! 5: e0e6ade3f38 config API: have *_multi() return an "int" and take a "dest" @@ builtin/gc.c: static int maintenance_register(int argc, const char **argv, const if (!strcmp(maintpath, item->string)) { found = 1; @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, const char *prefi - usage_with_options(builtin_maintenance_unregister_usage, - options); - -- list = git_config_get_value_multi(key); + if (config_file) { + git_configset_init(&cs); + git_configset_add_file(&cs, config_file); +- list = git_configset_get_value_multi(&cs, key); +- } else { +- list = git_config_get_value_multi(key); + } - if (list) { -+ if (!git_config_get_value_multi(key, &list)) { ++ if (!(config_file ++ ? git_configset_get_value_multi(&cs, key, &list) ++ : git_config_get_value_multi(key, &list))) { for_each_string_list_item(item, list) { if (!strcmp(maintpath, item->string)) { found = 1; @@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg * by default, only initialize 'active' modules. */ - if (!argc && git_config_get_value_multi("submodule.active")) -+ if (!argc && !git_config_get_value_multi("submodule.active", &values)) ++ if (!argc && !git_config_get_value_multi("submodule.active", ++ &values)) module_list_active(&list); info.prefix = opt.prefix; 6: a0c29d46556 ! 6: 06d502bc577 for-each-repo: error on bad --config @@ builtin/for-each-repo.c: int cmd_for_each_repo(int argc, const char **argv, cons return 0; - ## builtin/submodule--helper.c ## -@@ builtin/submodule--helper.c: static int module_init(int argc, const char **argv, const char *prefix) - NULL - }; - int ret = 1; -- const struct string_list *values; -+ const struct string_list *unused; - - argc = parse_options(argc, argv, prefix, module_init_options, - git_submodule_helper_usage, 0); -@@ builtin/submodule--helper.c: static int module_init(int argc, const char **argv, const char *prefix) - * If there are no path args and submodule.active is set then, - * by default, only initialize 'active' modules. - */ -- if (!argc && !git_config_get_value_multi("submodule.active", &values)) -+ if (!argc && !git_config_get_value_multi("submodule.active", &unused)) - module_list_active(&list); - - info.prefix = prefix; -@@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix) - if (opt.init) { - struct module_list list = MODULE_LIST_INIT; - struct init_cb info = INIT_CB_INIT; -- const struct string_list *values; -+ const struct string_list *unused; - - if (module_list_compute(argv, opt.prefix, - &pathspec2, &list) < 0) { -@@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix) - * If there are no path args and submodule.active is set then, - * by default, only initialize 'active' modules. - */ -- if (!argc && !git_config_get_value_multi("submodule.active", &values)) -+ if (!argc && !git_config_get_value_multi("submodule.active", &unused)) - module_list_active(&list); - - info.prefix = opt.prefix; - ## t/t0068-for-each-repo.sh ## @@ t/t0068-for-each-repo.sh: test_expect_success 'do nothing on empty config' ' git for-each-repo --config=bogus.config -- help --no-such-option 7: c12805f3d55 ! 7: f35aacef4ca config API users: test for *_get_value_multi() segfaults @@ t/t7413-submodule-is-active.sh: test_expect_success 'is-active works with submod ## t/t7900-maintenance.sh ## @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' ' - git maintenance unregister --force + git maintenance unregister --config-file ./other --force ' +test_expect_failure 'register with no value for maintenance.repo' ' 8: 6b76f9eac90 ! 8: b45189b4624 config API: add "string" version of *_value_multi(), fix segfaults @@ builtin/gc.c: static int maintenance_register(int argc, const char **argv, const if (!strcmp(maintpath, item->string)) { found = 1; @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, const char *prefi - usage_with_options(builtin_maintenance_unregister_usage, - options); - -- if (!git_config_get_value_multi(key, &list)) { -+ if (!git_config_get_string_multi(key, &list)) { + git_configset_add_file(&cs, config_file); + } + if (!(config_file +- ? git_configset_get_value_multi(&cs, key, &list) +- : git_config_get_value_multi(key, &list))) { ++ ? git_configset_get_string_multi(&cs, key, &list) ++ : git_config_get_string_multi(key, &list))) { for_each_string_list_item(item, list) { if (!strcmp(maintpath, item->string)) { found = 1; @@ builtin/log.c: static void set_default_decoration_filter(struct decoration_filte - if (!git_config_get_value_multi("log.excludeDecoration", - &config_exclude)) { -+ if (!git_config_get_string_multi("log.excludeDecoration", &config_exclude)) { ++ if (!git_config_get_string_multi("log.excludeDecoration", ++ &config_exclude)) { struct string_list_item *item; for_each_string_list_item(item, config_exclude) string_list_append(decoration_filter->exclude_ref_config_pattern, @@ config.c: int git_config_get_value_multi(const char *key, const struct string_li int git_config_get_string(const char *key, char **dest) { return repo_config_get_string(the_repository, key, dest); -@@ config.c: void git_die_config(const char *key, const char *err, ...) - va_end(params); - } - if (git_config_get_value_multi(key, &values)) -- BUG("for key '%s' we must have a value to report on", key); -+ BUG("key '%s' does not exist, should not be given to git_die_config()", -+ key); - kv_info = values->items[values->nr - 1].util; - git_die_config_linenr(key, kv_info->filename, kv_info->linenr); - } ## config.h ## @@ config.h: RESULT_MUST_BE_USED @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () { preferBitmapTips EOF - git repack -adb -+ + cat >expect <<-\EOF && + error: missing value for '\''pack.preferbitmaptips'\'' + EOF @@ t/t7413-submodule-is-active.sh: test_expect_failure 'is-active handles submodule ## t/t7900-maintenance.sh ## @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' ' - git maintenance unregister --force + git maintenance unregister --config-file ./other --force ' -test_expect_failure 'register with no value for maintenance.repo' ' 9: e2f8f7c52e3 = 9: 58ead3ca555 for-each-repo: with bad config, don't conflate <path> and <cmd> -- 2.39.0.rc0.955.ge9b241be664
