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
