On 9/28/2022 10:37 AM, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Sep 28 2022, Derrick Stolee wrote: >> If we return a negative value on an error and the number of matches on >> success, then this change could instead be "if (repo_config....() > 0)". > > Hrm, I think you're confusing the worldview your series here is > advocating for, and what I'm suggesting as an alternative. > > There isn't any way on "master" to have "an empty list", that's a > worldview you're proposing. In particular your 1/5 here removes: > > assert(values->nr > 0); Yes, I think the lack of a key should be the same to an empty list because it is logically equivalent and an empty list is safer to use than a possibly-NULL list. That's what started this whole investigation. By no longer returning NULL, we can eliminate an entire class of bugs from happening. > More generally the config format has no notion of "an empty list", if > you have a valid key-value pair at all you have a list of ".nr >= 1". The critical part is that you have a return code that has three modes: 1. Negative: some kind of parsing error happened, such as a malformed 'key' parameter. 2. Zero: The 'key' was found with multiple values. 3. Positive: The 'key' was not found. (Are there other innocuous errors that fit in this case?) This "positive means not found" is very non-obvious. Even with your goal of exposing those parsing errors when the 'key' is user-supplied, I still think it would be better to provide a non-NULL empty string_list and only care about these return values: 1. Negative: some kind of parsing error happened. 2. Nonnegative: parsing succeeded. The string_list has all found values (might be empty) and the return value is equal to the string_list's 'nr' member. In these cases, I see two modes of use. First, check for an error and exit early (empty list no-ops naturally): if (git_config_get_const_value_multi(key, &list) < 0) return -1; for_each_string_list_item(item, list) { ... } Second, ignore errors. Care about non-empty list: if (git_config_get_const_value_multi("known.key", &list) > 0) { ... } But this is just a matter of taste at this point, and I'm getting the feeling that my taste for reducing the chances of NULL references is not very popular. Thanks, -Stolee