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

On Wed, Sep 28 2022, Derrick Stolee wrote:

quoted

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