mbox series

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

Message ID cover-v6-0.9-00000000000-20230307T180516Z-avarab@gmail.com (mailing list archive)
Headers show
Series config API: make "multi" safe, fix segfaults, propagate "ret" | expand

Message

Ævar Arnfjörð Bjarmason March 7, 2023, 6:09 p.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 v5[3]:

 * Drop the 6th commit, which made existing API functions return
   -1. We shouldn't coerce errors to "return 1", but making the API
   consistent can wait for a follow-up to this topic.

   This should address the reason for this topic being stalled for a
   while, see e.g. [4].

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-v5-00.10-00000000000-20230207T154000Z-avarab@gmail.com/
4. https://lore.kernel.org/git/xmqqcz5snyxz.fsf@gitster.g/

Æ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 v5:
 1:  cefc4188984 =  1:  43fdb0cf50c config tests: cover blind spots in git_die_config() tests
 2:  91a44456327 =  2:  4b0799090c9 config tests: add "NULL" tests for *_get_value_multi()
 3:  4a73151abde =  3:  62fe2f04e71 config API: add and use a "git_config_get()" family of functions
 4:  382a77ca69e =  4:  e36303f4d3d versioncmp.c: refactor config reading next commit
 5:  8f17bf8150c !  5:  e38523267e7 config API: have *_multi() return an "int" and take a "dest"
    @@ config.c: void git_die_config(const char *key, const char *err, ...)
      }
     
      ## config.h ##
    -@@ config.h: int git_configset_add_parameters(struct config_set *cs);
    +@@ config.h: int git_configset_add_file(struct config_set *cs, const char *filename);
      /**
       * Finds and returns the value list, sorted in order of increasing priority
       * for the configuration variable `key` and config set `cs`. When the
 6:  b515ff13f9b <  -:  ----------- config API: don't lose the git_*get*() return values
 7:  8a83c30ea78 =  6:  3a87b35e114 for-each-repo: error on bad --config
 8:  d9abc78c2be =  7:  66b7060f66f config API users: test for *_get_value_multi() segfaults
 9:  65fa91e7ce7 =  8:  0da4cdb3f6a config API: add "string" version of *_value_multi(), fix segfaults
10:  4db3c6d0ed9 =  9:  627eb15a319 for-each-repo: with bad config, don't conflate <path> and <cmd>

Comments

Glen Choo March 8, 2023, 12:48 a.m. UTC | #1
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Range-diff against v5:
>  1:  cefc4188984 =  1:  43fdb0cf50c config tests: cover blind spots in git_die_config() tests
>  2:  91a44456327 =  2:  4b0799090c9 config tests: add "NULL" tests for *_get_value_multi()
>  3:  4a73151abde =  3:  62fe2f04e71 config API: add and use a "git_config_get()" family of functions
>  4:  382a77ca69e =  4:  e36303f4d3d versioncmp.c: refactor config reading next commit
>  5:  8f17bf8150c !  5:  e38523267e7 config API: have *_multi() return an "int" and take a "dest"
>     @@ config.c: void git_die_config(const char *key, const char *err, ...)
>       }
>      
>       ## config.h ##
>     -@@ config.h: int git_configset_add_parameters(struct config_set *cs);
>     +@@ config.h: int git_configset_add_file(struct config_set *cs, const char *filename);
>       /**
>        * Finds and returns the value list, sorted in order of increasing priority
>        * for the configuration variable `key` and config set `cs`. When the
>  6:  b515ff13f9b <  -:  ----------- config API: don't lose the git_*get*() return values
>  7:  8a83c30ea78 =  6:  3a87b35e114 for-each-repo: error on bad --config
>  8:  d9abc78c2be =  7:  66b7060f66f config API users: test for *_get_value_multi() segfaults
>  9:  65fa91e7ce7 =  8:  0da4cdb3f6a config API: add "string" version of *_value_multi(), fix segfaults
> 10:  4db3c6d0ed9 =  9:  627eb15a319 for-each-repo: with bad config, don't conflate <path> and <cmd>

I haven't reread the series in its totality yet (I should get to it in
the next few days), but a small-ish thing that jumps out from
the range-diff is that this version doesn't revert the commit message
changes in the previous version (v5 CL [1]) that referred to the ejected
06/10. I.e. v5 said that we were changing the return values of the
*_get_*() functions so that the new function is not a special snowflake,
and those commit messages haven't been changed.

1. https://lore.kernel.org/git/cover-v5-00.10-00000000000-20230207T154000Z-avarab@gmail.com/