| Message ID | cover-v8-0.9-00000000000-20230328T140126Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | config API: make "multi" safe, fix segfaults, propagate "ret" | expand |
Æ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). > > [..] > > * A trivial documentation change to 3/9, to clarify which doc in > config.h refer to what. As noted in the v7 discussion I think that > config.h could use some larger cleanups in this area, but let's > leave that for some future topic. > > [...] > > Range-diff against v7: > 1: 9f297a35e14 = 1: b600354c0f6 config tests: cover blind spots in git_die_config() tests > 2: 45d483066ef = 2: 49908f0bcf3 config tests: add "NULL" tests for *_get_value_multi() > 3: a977b7b188f ! 3: d163b3d04ff config API: add and use a "git_config_get()" family of functions > @@ config.h: void git_configset_clear(struct config_set *cs); > * value in the 'dest' pointer. > */ > > ++/** > ++ * git_configset_get() returns negative values on error, see > ++ * repo_config_get() below. > ++ */ > +RESULT_MUST_BE_USED > +int git_configset_get(struct config_set *cs, const char *key); > + Thanks! I read through config.h to be sure, and the result looks pretty clear to me. > 4: 3a5a323cd91 = 4: d7dfedb7225 versioncmp.c: refactor config reading next commit > 5: dced12a40d2 = 5: 840fb9d5c74 config API: have *_multi() return an "int" and take a "dest" > 6: d910f7e3a27 = 6: 75a68b14217 for-each-repo: error on bad --config > 7: 57db0fcd91f = 7: a78056e2748 config API users: test for *_get_value_multi() segfaults > 8: b374a716555 = 8: 686b512c3df config API: add "string" version of *_value_multi(), fix segfaults > 9: 6791e1f6f85 = 9: 6fce633493b for-each-repo: with bad config, don't conflate <path> and <cmd> Reviewed-by: Glen Choo <chooglen@google.com>
Glen Choo <chooglen@google.com> writes: > Thanks! I read through config.h to be sure, and the result looks pretty > clear to me. Thanks, both. Replaced and will mark the topic for 'next' soonish.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > A larger general overview at v1[1], but note the API changes in > v2[2]. Changes since v7[3]: > > * A trivial documentation change to 3/9, to clarify which doc in > config.h refer to what. As noted in the v7 discussion I think that > config.h could use some larger cleanups in this area, but let's > leave that for some future topic. Thanks. Will replace.
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 v7[3]: * A trivial documentation change to 3/9, to clarify which doc in config.h refer to what. As noted in the v7 discussion I think that config.h could use some larger cleanups in this area, but let's leave that for some future topic. Branch & passing CI for this at: https://github.com/avar/git/tree/avar/have-git_configset_get_value-use-dest-and-int-pattern-8 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/cover-v7-0.9-00000000000-20230308T090513Z-avarab@gmail.com/ Ævar Arnfjörð Bjarmason (9): config tests: cover blind spots in git_die_config() tests config tests: add "NULL" tests for *_get_value_multi() config API: add and use a "git_config_get()" family of functions 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 | 15 ++-- builtin/log.c | 6 +- builtin/submodule--helper.c | 7 +- builtin/worktree.c | 3 +- config.c | 109 ++++++++++++++++++++++----- config.h | 72 +++++++++++++++--- pack-bitmap.c | 6 +- submodule.c | 3 +- t/helper/test-config.c | 28 ++++++- t/t0068-for-each-repo.sh | 19 +++++ t/t1308-config-set.sh | 108 +++++++++++++++++++++++++- 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 ++++-- 21 files changed, 481 insertions(+), 72 deletions(-) Range-diff against v7: 1: 9f297a35e14 = 1: b600354c0f6 config tests: cover blind spots in git_die_config() tests 2: 45d483066ef = 2: 49908f0bcf3 config tests: add "NULL" tests for *_get_value_multi() 3: a977b7b188f ! 3: d163b3d04ff config API: add and use a "git_config_get()" family of functions @@ config.h: void git_configset_clear(struct config_set *cs); * value in the 'dest' pointer. */ ++/** ++ * git_configset_get() returns negative values on error, see ++ * repo_config_get() below. ++ */ +RESULT_MUST_BE_USED +int git_configset_get(struct config_set *cs, const char *key); + 4: 3a5a323cd91 = 4: d7dfedb7225 versioncmp.c: refactor config reading next commit 5: dced12a40d2 = 5: 840fb9d5c74 config API: have *_multi() return an "int" and take a "dest" 6: d910f7e3a27 = 6: 75a68b14217 for-each-repo: error on bad --config 7: 57db0fcd91f = 7: a78056e2748 config API users: test for *_get_value_multi() segfaults 8: b374a716555 = 8: 686b512c3df config API: add "string" version of *_value_multi(), fix segfaults 9: 6791e1f6f85 = 9: 6fce633493b for-each-repo: with bad config, don't conflate <path> and <cmd>