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

From: Derrick Stolee <derrickstolee@github.com>
Date: Sep 28, 2022, 18:10 UTC
Message-ID: <31ceb36b-3c60-c4c2-c6fb-3f602eb9d4ea@github.com>
In-Reply-To: <220928.86y1u3wnaz.gmgdl@evledraar.gmail.com>

On 9/28/2022 10:37 AM, Ævar Arnfjörð Bjarmason wrote:

> 
> On Wed, Sep 28 2022, Derrick Stolee wrote:

Show 10 quoted lines

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