Re: [PATCH v3 0/9] config API: make "multi" safe, fix numerous segfaults

From: Glen Choo <chooglen@google.com>
Date: Jan 19, 2023, 00:10 UTC
Message-ID: <kl6llelzo044.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <cover-v3-0.9-00000000000-20221125T093158Z-avarab@gmail.com>

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...

Show 7 quoted lines

> Æ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.