| Message ID | cover-v3-0.9-00000000000-20221125T093158Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | config API: make "multi" safe, fix numerous segfaults | expand |
We covered this at Review Club this week (thanks for coming, Ævar!). You can find the notes at: https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit The overall sentiment from the meeting was that this is a positive direction for the config API to go in. My personal opinion is that this series is close to mergeable and I had mostly minor comments. Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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). As you mentioned in Review Club, this series also fixes a wart in config.h where *_get_value_multi() returned a "struct string_list" instead of an error code like all other getters. So this series is technically doing two sort-of-different things... > Æ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 Fix the wart.. > 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> and introduce the better API that won't segfault, but I think it's okay to have the series do both since they're closely related enough and the latter is quite small anyway.
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@gmail.com/ 2. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20221101T225822Z-avarab@gmail.com/ 3. https://lore.kernel.org/git/221107.86leomx2dg.gmgdl@evledraar.gmail.com/ 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>