[v7,0/9] config API: make "multi" safe, fix segfaults, propagate "ret"

Message

Ævar Arnfjörð Bjarmason March 8, 2023, 9:06 a.m. UTC

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 v6[3]:

 * Glen pointed out that ejecting a commit in v6 orphaned a
   corresponding forward-reference in a commit message, fix that.

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/
3. https://lore.kernel.org/git/[e-mail]/
4. https://lore.kernel.org/git/[e-mail]/

Æ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                             |  68 ++++++++++++++---
 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, 477 insertions(+), 72 deletions(-)

Range-diff against v6:
 1:  43fdb0cf50c =  1:  9f297a35e14 config tests: cover blind spots in git_die_config() tests
 2:  4b0799090c9 =  2:  45d483066ef config tests: add "NULL" tests for *_get_value_multi()
 3:  62fe2f04e71 !  3:  a977b7b188f config API: add and use a "git_config_get()" family of functions
    @@ Commit message
         "int" instead of "void". Let's leave that for now, and focus on
         the *_get_*() functions.
     
    -    In a subsequent commit we'll fix the other *_get_*() functions to so
    -    that they'll ferry our underlying "ret" along, rather than normalizing
    -    it to a "return 1". But as an intermediate step to that we'll need to
    -    fix git_configset_get_value_multi() to return "int", and that change
    -    itself is smaller because of this change to migrate some callers away
    -    from the *_value_multi() API.
    -
         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()
 4:  e36303f4d3d =  4:  3a5a323cd91 versioncmp.c: refactor config reading next commit
 5:  e38523267e7 =  5:  dced12a40d2 config API: have *_multi() return an "int" and take a "dest"
 6:  3a87b35e114 =  6:  d910f7e3a27 for-each-repo: error on bad --config
 7:  66b7060f66f =  7:  57db0fcd91f config API users: test for *_get_value_multi() segfaults
 8:  0da4cdb3f6a =  8:  b374a716555 config API: add "string" version of *_value_multi(), fix segfaults
 9:  627eb15a319 =  9:  6791e1f6f85 for-each-repo: with bad config, don't conflate <path> and <cmd>

Comments

Glen Choo March 9, 2023, 7:08 p.m. UTC | #1

Ævar Arnfjörð Bjarmason <[e-mail]> 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 larger general overview at v1[1], but note the API changes in
> v2[2]. Changes since v6[3]:
>
>  * Glen pointed out that ejecting a commit in v6 orphaned a
>    corresponding forward-reference in a commit message, fix that.

Thanks for your patience with the rerolls :) I only spotted a minor
comment issue [1] (which I think was what originally motivated v5?).
IMO this will be mergeable once we reorder that comment.

1. https://lore.kernel.org/git/[e-mail]/

Junio C Hamano March 9, 2023, 8:46 p.m. UTC | #2

Glen Choo <[e-mail]> writes:

> Ævar Arnfjörð Bjarmason <[e-mail]> 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 larger general overview at v1[1], but note the API changes in
>> v2[2]. Changes since v6[3]:
>>
>>  * Glen pointed out that ejecting a commit in v6 orphaned a
>>    corresponding forward-reference in a commit message, fix that.
>
> Thanks for your patience with the rerolls :) I only spotted a minor
> comment issue [1] (which I think was what originally motivated v5?).
> IMO this will be mergeable once we reorder that comment.
>
> 1. https://lore.kernel.org/git/[e-mail]/

Thanks for carefully reading these patches.  I agree that this round
is in quite a good shape.