Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API

[Tanay Abhra <tanayabh@gmail.com>]

On 8/8/2014 2:01 AM, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
>>>> Why is this needed? Are you now using key_value_info outside config.c?
>>>> Or is it a leftover from a previous experiment?
>>>
>>> Has this been resolved in the new round?
>>
>> Tanay explained in another subthread why this was needed. For callers
>> iterating over the string_list who want to get the file/line info, they
>> need to be able to cast the void * pointer to struct key_value_info *.
> 
> For callers that want to see all the multi-values, it would be
> preferrable for the iterator to pass the filename and the linenumber
> to the callback function, instead of exposing its implementation
> detail as a single string list and telling them to pick it apart,
> no?
> 
> Not a very convincing argument, but OK for now in the sense that we
> can fix it later if we wanted to before it gets too late.
>

(cc to Ramsay)

The discussion in both threads (v8 and v9), boils down to this,
is the `key_value_info` struct really required to be declared public or should be
just an implementation detail. I will give you the context,

The usage of the above mentioned struct is only required for
git_config_get_value_multi(). With the public struct, the code flow would look
like,


-- 8< --
diff --git a/notes.c b/notes.c
index 5fe691d..b7ab115 100644
--- a/notes.c
+++ b/notes.c
@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 	free(globs_copy);
 }

-static int notes_display_config(const char *k, const char *v, void *cb)
-{
-	int *load_refs = cb;
-
-	if (*load_refs && !strcmp(k, "notes.displayref")) {
-		if (!v)
-			config_error_nonbool(k);
-		string_list_add_refs_by_glob(&display_notes_refs, v);
-	}
-
-	return 0;
-}
-
 const char *default_notes_ref(void)
 {
 	const char *notes_ref = NULL;
@@ -1041,7 +1028,9 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 void init_display_notes(struct display_notes_opt *opt)
 {
 	char *display_ref_env;
-	int load_config_refs = 0;
+	const struct string_list *values;
+	struct key_value_info *kv_info;
+	int load_config_refs = 0, i;
 	display_notes_refs.strdup_strings = 1;

 	assert(!display_notes_trees);
@@ -1058,7 +1047,21 @@ void init_display_notes(struct display_notes_opt *opt)
 			load_config_refs = 1;
 	}

-	git_config(notes_display_config, &load_config_refs);
+	if (load_config_refs) {
+		values = git_config_get_value_multi("notes.displayref");
+		if (values) {
+			for (i = 0; i < values->nr; i++) {
+				if (!values->items[i].string) {
+					kv_info = values->items[i].util;
+					config_error_nonbool("notes.displayref");
+					git_die_config_linenr("notes.displayref", kv_info->filename, kv_info->linenr);
+				}
+				else
+					string_list_add_refs_by_glob(&display_notes_refs,
+								     values->items[i].string);
+			}
+		}
+	}

 	if (opt) {
 		struct string_list_item *item;
-- 8< --

We cannot use git_die_config() here because it is applicable to the last
value for a given variable.

Alternative solution to the problem can be a helper function like this,

git_die_config_index(key, value_index, err_msg, ...) which needs the value_index for a multi valued one,

+		values = git_config_get_value_multi("notes.displayref");
+		if (values) {
+			for (i = 0; i < values->nr; i++) {
+				if (!values->items[i].string)
+					git_die_config_linenr("notes.displayref", i, "no null values allowed for :'%s'", "notes.displayref");
+				else ; /* do stuff */
+			}

A callback iterator which supplies the linenr and filename to the callback
function is not helpful, because there are many variable checks in a
git_config() call where multi valued and single valued both reside, so we cannot
use a callback iterator without adding more code cruft.

What do you think, which way seems least obtrusive, or is there an another way out?