On Thu, Feb 02 2023, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

We already have the basic "git_config_get_value()" function and its
"repo_*" and "configset" siblings to get a given "key" and assign the
last key found to a provided "value".

But some callers don't care about that value, but just want to use the
return value of the "get_value()" function to check whether the key
exist (or another non-zero return value).

The immediate motivation for this is that a subsequent commit will
need to change all callers of the "*_get_value_multi()" family of
functions. In two cases here we (ab)used it to check whether we had
any values for the given key, but didn't care about the return value.

So, the idea is that 

	if (!git_config_get_string(key, &discard))
		free(discard);
	else
		... the key is missing ...

becomes

	if (git_config_get(key))
		... the key is missing ...

In other words, git_config_get() returns 0 only when the key is
used, and non-zero return signals that the key is not used?

Similarly, get_value_multi() was an interface to get to the
value_list associated to the given key, and was abused like

	if (git_config_get_value_multi(key))
		... the key exists ...

which will become

	if (!git_config_get(key))
		... the key exists ...

right?

Yes, I've amended this in a re-roll to add tests to the configset tests,
so how the new API should be used becomes obvious.

quoted

@@ -3185,7 +3184,7 @@ static void configure_added_submodule(struct add_data *add_data)
 	 * is_submodule_active(), since that function needs to find
 	 * out the value of "submodule.active" again anyway.
 	 */
-	if (!git_config_get_string_tmp("submodule.active", &val)) {
+	if (!git_config_get("submodule.active")) {

string_tmp() variant is to retrieve borrowed value, and it returns 0
when there is a value.  If it is a valueless true, we get -1 back
with an error message.  What does the updated version do in the
valueless true case?

No, we'll get back 0, as a value-less key exists. 

This sort of code would be correct in general, as if we really want to
check if the key exists a value-less is a valid boolean true.

In this case we happen to segfault later on if we encounter such a key,
but that's unrelated to this being correct.

The segfault is then fixed later in this topic.

quoted

@@ -2425,8 +2433,25 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *
 
 const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
 {
-	struct config_set_element *e = configset_find_element(cs, key);
-	return e ? &e->value_list : NULL;
+	struct config_set_element *e;
+
+	if (configset_find_element(cs, key, &e))
+		return NULL;
+	else if (!e)
+		return NULL;
+	return &e->value_list;
+}

OK.  !e means "we got a healthy key and peeking into the hash table,
there wasn't any entry for the key", and that is reported with NULL.
Do we evern return a string list with .nr == 0, I wonder.  Having to
deal with such a list would make the caller's job more complex, but
perhaps we are not allowing the code to shrink value_list.nr to
avoid such a situation?

No, we never return a list with .nr == 0. That's why Stolee's earlier
RFC tried to solve a subset of the problem this topic addresse by
returning such a list as a way to indicate "does not exist".

That would work to an extent, but would leave the main problem (which
Stolee wasn't aware of at the time) of having a ".nr > 0" list with
"NULL" elements in it.

It also wouldn't be idiomatic with the rest of the API, as this topic
shows, and means you can't distinguish non-existence from other errors.

quoted

+int git_configset_get(struct config_set *cs, const char *key)
+{
+	struct config_set_element *e;
+	int ret;
+
+	if ((ret = configset_find_element(cs, key, &e)))
+		return ret;
+	else if (!e)
+		return 1;
+	return 0;
 }

OK.  So 0 return from the function means there is a value (or more)
for a given key.  Good.

quoted

diff --git a/config.h b/config.h
index ef9eade6414..04c5e594015 100644
--- a/config.h
+++ b/config.h

@@ -471,9 +471,12 @@ void git_configset_clear(struct config_set *cs);
 
 /*
  * These functions return 1 if not found, and 0 if found, leaving the found
- * value in the 'dest' pointer.
+ * value in the 'dest' pointer (if any).
  */

Now the returned non-zero values are no longer 1 alone, no?
Whatever lower-level functions use to signal an error is propagated
up with the

	if ((ret = func())
		return ret;

That's still mostly true, but I should have added the new
git_configset_get() below this comment, and split it up, i.e. it's still
true for the functions that populate "dest".

I have a topic-on-top to fix those (to propagate them up), and it was
hard to know where to draw the line, in this case I got it wrong. Will
fix it.