config: drop entry payload; teach config_list about entries

The opaque `payload` on an entry is unnecessary and distracting; config
entries should follow the patterns of other objects and use space
elsewhere in the structure with a "base" config entry struct embedded.

include/git2/config.h

@@ -66,8 +66,12 @@ typedef struct git_config_entry {
 	const char *value; /**< String value of the entry */
 	unsigned int include_depth; /**< Depth of includes where this variable was found */
 	git_config_level_t level; /**< Which config file this was found in */
-	void GIT_CALLBACK(free)(struct git_config_entry *entry); /**< Free function for this entry */
-	void *payload; /**< Opaque value for the free function. Do not read or write */
+
+	/**
+	 * Free function for this entry; for internal purposes. Callers
+	 * should call `git_config_entry_free` to free data.
+	 */
+	void GIT_CALLBACK(free)(struct git_config_entry *entry);
 } git_config_entry;
 
 /**

src/libgit2/config_file.c

@@ -293,7 +293,7 @@ static int config_file_set(git_config_backend *cfg, const char *name, const char
 {
 	config_file_backend *b = GIT_CONTAINER_OF(cfg, config_file_backend, parent);
 	git_config_list *config_list;
-	git_config_entry *existing;
+	git_config_list_entry *existing;
 	char *key, *esc_value = NULL;
 	int error;
 
@@ -308,8 +308,8 @@ static int config_file_set(git_config_backend *cfg, const char *name, const char
 		if (error != GIT_ENOTFOUND)
 			goto out;
 		error = 0;
-	} else if ((!existing->value && !value) ||
-		   (existing->value && value && !strcmp(existing->value, value))) {
+	} else if ((!existing->base.value && !value) ||
+		   (existing->base.value && value && !strcmp(existing->base.value, value))) {
 		/* don't update if old and new values already match */
 		error = 0;
 		goto out;
@@ -331,13 +331,6 @@ out:
 	return error;
 }
 
-/* release the map containing the entry as an equivalent to freeing it */
-static void config_file_entry_free(git_config_entry *entry)
-{
-	git_config_list *config_list = (git_config_list *) entry->payload;
-	git_config_list_free(config_list);
-}
-
 /*
  * Internal function that actually gets the value in string form
  */
@@ -345,7 +338,7 @@ static int config_file_get(git_config_backend *cfg, const char *key, git_config_
 {
 	config_file_backend *h = GIT_CONTAINER_OF(cfg, config_file_backend, parent);
 	git_config_list *config_list = NULL;
-	git_config_entry *entry;
+	git_config_list_entry *entry;
 	int error = 0;
 
 	if (!h->parent.readonly && ((error = config_file_refresh(cfg)) < 0))
@@ -359,7 +352,7 @@ static int config_file_get(git_config_backend *cfg, const char *key, git_config_
 		return error;
 	}
 
-	*out = entry;
+	*out = &entry->base;
 
 	return 0;
 }
@@ -395,7 +388,7 @@ static int config_file_delete(git_config_backend *cfg, const char *name)
 {
 	config_file_backend *b = GIT_CONTAINER_OF(cfg, config_file_backend, parent);
 	git_config_list *config_list = NULL;
-	git_config_entry *entry;
+	git_config_list_entry *entry;
 	char *key = NULL;
 	int error;
 
@@ -412,7 +405,7 @@ static int config_file_delete(git_config_backend *cfg, const char *name)
 		goto out;
 	}
 
-	if ((error = config_file_write(b, name, entry->name, NULL, NULL)) < 0)
+	if ((error = config_file_write(b, name, entry->base.name, NULL, NULL)) < 0)
 		goto out;
 
 out:
@@ -425,7 +418,7 @@ static int config_file_delete_multivar(git_config_backend *cfg, const char *name
 {
 	config_file_backend *b = GIT_CONTAINER_OF(cfg, config_file_backend, parent);
 	git_config_list *config_list = NULL;
-	git_config_entry *entry = NULL;
+	git_config_list_entry *entry = NULL;
 	git_regexp preg = GIT_REGEX_INIT;
 	char *key = NULL;
 	int result;
@@ -774,7 +767,7 @@ static int read_on_variable(
 {
 	config_file_parse_data *parse_data = (config_file_parse_data *)data;
 	git_str buf = GIT_STR_INIT;
-	git_config_entry *entry;
+	git_config_list_entry *entry;
 	const char *c;
 	int result = 0;
 
@@ -797,14 +790,21 @@ static int read_on_variable(
 	if (git_str_oom(&buf))
 		return -1;
 
-	entry = git__calloc(1, sizeof(git_config_entry));
+	entry = git__calloc(1, sizeof(git_config_list_entry));
 	GIT_ERROR_CHECK_ALLOC(entry);
-	entry->name = git_str_detach(&buf);
-	entry->value = var_value ? git__strdup(var_value) : NULL;
-	entry->level = parse_data->level;
-	entry->include_depth = parse_data->depth;
-	entry->free = config_file_entry_free;
-	entry->payload = parse_data->config_list;
+
+	entry->base.name = git_str_detach(&buf);
+	GIT_ERROR_CHECK_ALLOC(entry->base.name);
+
+	if (var_value) {
+		entry->base.value = git__strdup(var_value);
+		GIT_ERROR_CHECK_ALLOC(entry->base.value);
+	}
+
+	entry->base.level = parse_data->level;
+	entry->base.include_depth = parse_data->depth;
+	entry->base.free = git_config_list_entry_free;
+	entry->config_list = parse_data->config_list;
 
 	if ((result = git_config_list_append(parse_data->config_list, entry)) < 0)
 		return result;
@@ -812,11 +812,11 @@ static int read_on_variable(
 	result = 0;
 
 	/* Add or append the new config option */
-	if (!git__strcmp(entry->name, "include.path"))
-		result = parse_include(parse_data, entry->value);
-	else if (!git__prefixcmp(entry->name, "includeif.") &&
-	         !git__suffixcmp(entry->name, ".path"))
-		result = parse_conditional_include(parse_data, entry->name, entry->value);
+	if (!git__strcmp(entry->base.name, "include.path"))
+		result = parse_include(parse_data, entry->base.value);
+	else if (!git__prefixcmp(entry->base.name, "includeif.") &&
+	         !git__suffixcmp(entry->base.name, ".path"))
+		result = parse_conditional_include(parse_data, entry->base.name, entry->base.value);
 
 	return result;
 }

src/libgit2/config_list.c

@@ -10,11 +10,11 @@
 typedef struct config_entry_list {
 	struct config_entry_list *next;
 	struct config_entry_list *last;
-	git_config_entry *entry;
+	git_config_list_entry *entry;
 } config_entry_list;
 
 typedef struct {
-	git_config_entry *entry;
+	git_config_list_entry *entry;
 	bool multivar;
 } config_entry_map_head;
 
@@ -49,29 +49,32 @@ int git_config_list_new(git_config_list **out)
 
 int git_config_list_dup_entry(git_config_list *config_list, const git_config_entry *entry)
 {
-	git_config_entry *duplicated;
+	git_config_list_entry *duplicated;
 	int error;
 
-	duplicated = git__calloc(1, sizeof(git_config_entry));
+	duplicated = git__calloc(1, sizeof(git_config_list_entry));
 	GIT_ERROR_CHECK_ALLOC(duplicated);
 
-	duplicated->name = git__strdup(entry->name);
-	GIT_ERROR_CHECK_ALLOC(duplicated->name);
+	duplicated->base.name = git__strdup(entry->name);
+	GIT_ERROR_CHECK_ALLOC(duplicated->base.name);
 
 	if (entry->value) {
-		duplicated->value = git__strdup(entry->value);
-		GIT_ERROR_CHECK_ALLOC(duplicated->value);
+		duplicated->base.value = git__strdup(entry->value);
+		GIT_ERROR_CHECK_ALLOC(duplicated->base.value);
 	}
-	duplicated->level = entry->level;
-	duplicated->include_depth = entry->include_depth;
+
+	duplicated->base.level = entry->level;
+	duplicated->base.include_depth = entry->include_depth;
+	duplicated->base.free = git_config_list_entry_free;
+	duplicated->config_list = config_list;
 
 	if ((error = git_config_list_append(config_list, duplicated)) < 0)
 		goto out;
 
 out:
 	if (error && duplicated) {
-		git__free((char *) duplicated->name);
-		git__free((char *) duplicated->value);
+		git__free((char *) duplicated->base.name);
+		git__free((char *) duplicated->base.value);
 		git__free(duplicated);
 	}
 	return error;
@@ -87,7 +90,7 @@ int git_config_list_dup(git_config_list **out, git_config_list *config_list)
 		goto out;
 
 	for (head = config_list->entries; head; head = head->next)
-		if ((git_config_list_dup_entry(result, head->entry)) < 0)
+		if ((git_config_list_dup_entry(result, &head->entry->base)) < 0)
 			goto out;
 
 	*out = result;
@@ -109,7 +112,7 @@ static void config_list_free(git_config_list *config_list)
 	config_entry_map_head *head;
 
 	git_strmap_foreach_value(config_list->map, head, {
-		git__free((char *) head->entry->name);
+		git__free((char *) head->entry->base.name);
 		git__free(head);
 	});
 	git_strmap_free(config_list->map);
@@ -117,7 +120,7 @@ static void config_list_free(git_config_list *config_list)
 	entry_list = config_list->entries;
 	while (entry_list != NULL) {
 		next = entry_list->next;
-		git__free((char *) entry_list->entry->value);
+		git__free((char *) entry_list->entry->base.value);
 		git__free(entry_list->entry);
 		git__free(entry_list);
 		entry_list = next;
@@ -132,12 +135,12 @@ void git_config_list_free(git_config_list *config_list)
 		GIT_REFCOUNT_DEC(config_list, config_list_free);
 }
 
-int git_config_list_append(git_config_list *config_list, git_config_entry *entry)
+int git_config_list_append(git_config_list *config_list, git_config_list_entry *entry)
 {
 	config_entry_list *list_head;
 	config_entry_map_head *map_head;
 
-	if ((map_head = git_strmap_get(config_list->map, entry->name)) != NULL) {
+	if ((map_head = git_strmap_get(config_list->map, entry->base.name)) != NULL) {
 		map_head->multivar = true;
 		/*
 		 * This is a micro-optimization for configuration files
@@ -145,11 +148,11 @@ int git_config_list_append(git_config_list *config_list, git_config_entry *entry
 		 * key will be the same for all list, we can just free
 		 * all except the first entry's name and just re-use it.
 		 */
-		git__free((char *) entry->name);
-		entry->name = map_head->entry->name;
+		git__free((char *) entry->base.name);
+		entry->base.name = map_head->entry->base.name;
 	} else {
 		map_head = git__calloc(1, sizeof(*map_head));
-		if ((git_strmap_set(config_list->map, entry->name, map_head)) < 0)
+		if ((git_strmap_set(config_list->map, entry->base.name, map_head)) < 0)
 			return -1;
 	}
 	map_head->entry = entry;
@@ -167,16 +170,18 @@ int git_config_list_append(git_config_list *config_list, git_config_entry *entry
 	return 0;
 }
 
-int git_config_list_get(git_config_entry **out, git_config_list *config_list, const char *key)
+int git_config_list_get(git_config_list_entry **out, git_config_list *config_list, const char *key)
 {
 	config_entry_map_head *entry;
+
 	if ((entry = git_strmap_get(config_list->map, key)) == NULL)
 		return GIT_ENOTFOUND;
+
 	*out = entry->entry;
 	return 0;
 }
 
-int git_config_list_get_unique(git_config_entry **out, git_config_list *config_list, const char *key)
+int git_config_list_get_unique(git_config_list_entry **out, git_config_list *config_list, const char *key)
 {
 	config_entry_map_head *entry;
 
@@ -188,13 +193,12 @@ int git_config_list_get_unique(git_config_entry **out, git_config_list *config_l
 		return -1;
 	}
 
-	if (entry->entry->include_depth) {
+	if (entry->entry->base.include_depth) {
 		git_error_set(GIT_ERROR_CONFIG, "entry is not unique due to being included");
 		return -1;
 	}
 
 	*out = entry->entry;
-
 	return 0;
 }
 
@@ -214,7 +218,7 @@ static int config_iterator_next(
 	if (!it->head)
 		return GIT_ITEROVER;
 
-	*entry = it->head->entry;
+	*entry = &it->head->entry->base;
 	it->head = it->head->next;
 
 	return 0;
@@ -236,3 +240,10 @@ int git_config_list_iterator_new(git_config_iterator **out, git_config_list *con
 
 	return 0;
 }
+
+/* release the map containing the entry as an equivalent to freeing it */
+void git_config_list_entry_free(git_config_entry *e)
+{
+	git_config_list_entry *entry = (git_config_list_entry *)e;
+	git_config_list_free(entry->config_list);
+}

src/libgit2/config_list.h

@@ -12,13 +12,20 @@
 
 typedef struct git_config_list git_config_list;
 
+typedef struct {
+	git_config_entry base;
+	git_config_list *config_list;
+} git_config_list_entry;
+
 int git_config_list_new(git_config_list **out);
 int git_config_list_dup(git_config_list **out, git_config_list *list);
 int git_config_list_dup_entry(git_config_list *list, const git_config_entry *entry);
 void git_config_list_incref(git_config_list *list);
 void git_config_list_free(git_config_list *list);
 /* Add or append the new config option */
-int git_config_list_append(git_config_list *list, git_config_entry *entry);
-int git_config_list_get(git_config_entry **out, git_config_list *list, const char *key);
-int git_config_list_get_unique(git_config_entry **out, git_config_list *list, const char *key);
+int git_config_list_append(git_config_list *list, git_config_list_entry *entry);
+int git_config_list_get(git_config_list_entry **out, git_config_list *list, const char *key);
+int git_config_list_get_unique(git_config_list_entry **out, git_config_list *list, const char *key);
 int git_config_list_iterator_new(git_config_iterator **out, git_config_list *list);
+
+void git_config_list_entry_free(git_config_entry *entry);

src/libgit2/config_mem.c

@@ -39,7 +39,7 @@ static int read_variable_cb(
 {
 	config_memory_parse_data *parse_data = (config_memory_parse_data *) payload;
 	git_str buf = GIT_STR_INIT;
-	git_config_entry *entry;
+	git_config_list_entry *entry;
 	const char *c;
 	int result;
 
@@ -62,12 +62,14 @@ static int read_variable_cb(
 	if (git_str_oom(&buf))
 		return -1;
 
-	entry = git__calloc(1, sizeof(git_config_entry));
+	entry = git__calloc(1, sizeof(git_config_list_entry));
 	GIT_ERROR_CHECK_ALLOC(entry);
-	entry->name = git_str_detach(&buf);
-	entry->value = var_value ? git__strdup(var_value) : NULL;
-	entry->level = parse_data->level;
-	entry->include_depth = 0;
+	entry->base.name = git_str_detach(&buf);
+	entry->base.value = var_value ? git__strdup(var_value) : NULL;
+	entry->base.level = parse_data->level;
+	entry->base.include_depth = 0;
+	entry->base.free = git_config_list_entry_free;
+	entry->config_list = parse_data->config_list;
 
 	if ((result = git_config_list_append(parse_data->config_list, entry)) < 0)
 		return result;
@@ -101,7 +103,14 @@ out:
 static int config_memory_get(git_config_backend *backend, const char *key, git_config_entry **out)
 {
 	config_memory_backend *memory_backend = (config_memory_backend *) backend;
-	return git_config_list_get(out, memory_backend->config_list, key);
+	git_config_list_entry *entry;
+	int error;
+
+	if ((error = git_config_list_get(&entry, memory_backend->config_list, key)) != 0)
+		return error;
+
+	*out = &entry->base;
+	return 0;
 }
 
 static int config_memory_iterator(

src/libgit2/config_snapshot.c

@@ -41,18 +41,11 @@ out:
 	return error;
 }
 
-/* release the map containing the entry as an equivalent to freeing it */
-static void config_snapshot_entry_free(git_config_entry *entry)
-{
-	git_config_list *config_list = (git_config_list *) entry->payload;
-	git_config_list_free(config_list);
-}
-
 static int config_snapshot_get(git_config_backend *cfg, const char *key, git_config_entry **out)
 {
 	config_snapshot_backend *b = GIT_CONTAINER_OF(cfg, config_snapshot_backend, parent);
 	git_config_list *config_list = NULL;
-	git_config_entry *entry;
+	git_config_list_entry *entry;
 	int error = 0;
 
 	if (git_mutex_lock(&b->values_mutex) < 0) {
@@ -69,10 +62,7 @@ static int config_snapshot_get(git_config_backend *cfg, const char *key, git_con
 		return error;
 	}
 
-	entry->free = config_snapshot_entry_free;
-	entry->payload = config_list;
-	*out = entry;
-
+	*out = &entry->base;
 	return 0;
 }