This series is a follow-up to an earlier RFC Stolee sent about making
the *_multi() config API return non-NULL, and instead give you an
empty string list[1].
I also think that part of the config API is a wart, but that we should
go for a different solution. It's the only config function that
doesn't return an "int" indicating whether we found the key.
Code that wants to use the values should then check that return
value. I.e. Stolee's version allows you to do:
const struct string_list *list = git_config_get_value_multi(key);
for_each_string_list_item(item, list) { ... found = 1 ... }
Whereas in this proposal we instead do (same as for non-multi):
if (!git_config_get_const_value_multi(key, &list))
for_each_string_list_item(item, list) { ... found = 1 ... }
Mid-series that's made nicer by adjusting the string_list API to have
sensible "const"'s (so we don't need catsing), and using utility
functions from there. I.e. the recently added code in builtin/gc.c
becomes (Stolee's at [3]):
if (!git_config_get_knownkey_value_multi(key, &list))
found = unsorted_string_list_has_string(list, maintpath);
But anyway.
Once I started poking at this approach I discovered that we have a
much larger issue here than whether the top-level value is NULL or an empty list.
As noted I don't think it's an issue that the *_multi() returns NULL
if we have no key, that's easy to handle.
But *_multi() doesn't have the equivalent of a wrapper that coerces
the values on the list into one of our types (as in "git config
--type=<type>").
A not so well known edge case in our config format (see 8/10) is that
value-less keys are represented as NULL's, and a "struct string_list"
is perfectly happy to have a "char *string" member that's NULL.
I.e.:
[a]key=x
[a]key
[a]key=y
Is represented as:
{ "x", NULL, "y" }
Not, as existing code apparently expected:
{ "x", "", "y" }
As a result existing code using *_multi() would segfault when reading
config like that. See 9/10, which fixes segfaults in 6 commits as old
as from 2015, and a couple of recent ones: One in the last release,
and one on "master" but not released yet.
The fix is thoroughly boring, we just start doing for *_multi() what
we've been doing for other config since Junio's 2008 fix (see 9/10) to
fix the same issue for non-multi config variables.
I.e. we provide a safer "I want strings, please" variant of the API,
just for *_multi(). At the culmination of this topic only the
test-helper uses the underlying unsafe API, and only because it needs
to check that we're still parsing the config correctly.
1. https://lore.kernel.org/git/pull.1369.git.1664287711.gitgitgadget@xxxxxxxxx/
2. https://lore.kernel.org/git/220928.868rm3w9d4.gmgdl@xxxxxxxxxxxxxxxxxxx
3. https://lore.kernel.org/git/e06cb4df081bc2222731f9185a22ed7ad67e3814.1664287711.git.gitgitgadget@xxxxxxxxx/
Ævar Arnfjörð Bjarmason (10):
config API: have *_multi() return an "int" and take a "dest"
for-each-repo: error on bad --config
config API: mark *_multi() with RESULT_MUST_BE_USED
string-list API: mark "struct_string_list" to "for_each_string_list"
const
string-list API: make has_string() and list_lookup() "const"
builtin/gc.c: use "unsorted_string_list_has_string()" where
appropriate
config API: add and use "lookup_value" functions
config tests: add "NULL" tests for *_get_value_multi()
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 | 29 +-----
builtin/log.c | 6 +-
builtin/submodule--helper.c | 7 +-
builtin/worktree.c | 3 +-
config.c | 164 +++++++++++++++++++++++++++++----
config.h | 110 ++++++++++++++++++++--
pack-bitmap.c | 7 +-
string-list.c | 6 +-
string-list.h | 6 +-
submodule.c | 3 +-
t/helper/test-config.c | 6 +-
t/t0068-for-each-repo.sh | 19 ++++
t/t1308-config-set.sh | 30 ++++++
t/t4202-log.sh | 15 +++
t/t5310-pack-bitmaps.sh | 21 +++++
t/t7004-tag.sh | 17 ++++
t/t7413-submodule-is-active.sh | 16 ++++
t/t7900-maintenance.sh | 38 ++++++++
versioncmp.c | 18 +++-
20 files changed, 451 insertions(+), 84 deletions(-)
--
2.38.0.1251.g3eefdfb5e7a
