git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 4/5] config: return an empty list, not NULL

From
Derrick Stolee <derrickstolee@github.com>
Date
Sep 27, 2022, 16:50 UTC
Message-ID
<b85d8a43-6640-aa9c-3103-0c3d43c2a479@github.com>
In-Reply-To
<220927.86sfkcyebf.gmgdl@evledraar.gmail.com>
On 9/27/2022 12:21 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:
Show 22 quoted lines
>>  /**
>>   * Finds and returns the value list, sorted in order of increasing priority
>>   * for the configuration variable `key`. When the configuration variable
>> - * `key` is not found, returns NULL. The caller should not free or modify
>> - * the returned pointer, as it is owned by the cache.
>> + * `key` is not found, returns an empty list. The caller should not free or
>> + * modify the returned pointer, as it is owned by the cache.
>>   */
>>  const struct string_list *git_config_get_value_multi(const char *key);
> 
> Aside from the "DWIM API" aspect of this (which I don't mind) I think
> this is really taking the low-level function in the wrong direction, and
> that we should just add a new simple wrapper instead.
> 
> I.e. both the pre-image API docs & this series gloss over the fact that
> we'd not just return NULL here if the config wasn't there, but also if
> git_config_parse_key() failed.
> 
> So it seems to me that a better direction would be starting with
> something like the WIP below (which doesn't compile the whole code, I
> stopped at config.[ch] and pack-bitmap.c). I.e. the same "int" return
> and "dest" pattern that most other things in the config API have.

Do you have an example where a caller would benefit from this distinction? Without such an example, I don't think it is worth creating such a huge change for purity's sake alone.

I'm pretty happy that the diff for this series is an overall reduction in code, while also not being too large in the interim:

 12 files changed, 39 insertions(+), 57 deletions(-)

If all callers that use the *_multi() methods would only use the wrapper, then what is the point of doing the low-level manipulations?

Thanks, -Stolee

Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 10 of 21 in “[RFC] config API: return empty list, not NULL”
  1. 0/5 [RFC] config API: return empty list, not NULLDerrick Stolee via GitGitGadget, Sep 27, 2022
  2. 1/5 config: relax requirements on multi-value returnDerrick Stolee via GitGitGadget, Sep 27, 2022
  3. Junio C HamanoSep 27, 2022
  4. 3/5 config: add BUG() statement instead of possible segfaultDerrick Stolee via GitGitGadget, Sep 27, 2022
  5. Ævar Arnfjörð BjarmasonSep 27, 2022
  6. Derrick StoleeSep 27, 2022
  7. Ævar Arnfjörð BjarmasonSep 27, 2022
  8. 4/5 config: return an empty list, not NULLDerrick Stolee via GitGitGadget, Sep 27, 2022
  9. Ævar Arnfjörð BjarmasonSep 27, 2022
  10. Derrick StoleeSep 27, 2022
  11. Ævar Arnfjörð BjarmasonSep 27, 2022
  12. Derrick StoleeSep 28, 2022
  13. Ævar Arnfjörð BjarmasonSep 28, 2022
  14. Derrick StoleeSep 28, 2022
  15. Ævar Arnfjörð BjarmasonSep 28, 2022
  16. 2/5 *: relax git_configset_get_value_multi resultDerrick Stolee via GitGitGadget, Sep 27, 2022
  17. Taylor BlauSep 28, 2022
  18. 5/5 *: expect a non-NULL list of config valuesDerrick Stolee via GitGitGadget, Sep 27, 2022
  19. Junio C HamanoSep 28, 2022
  20. Derrick StoleeSep 28, 2022
  21. Ævar Arnfjörð BjarmasonSep 28, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.