strmap: introduce high-level setter for key/value pairs

Currently, one would use the function `git_strmap_insert` to insert key/value
pairs into a map. This function has historically been a macro, which is why its
syntax is kind of weird: instead of returning an error code directly, it instead
has to be passed a pointer to where the return value shall be stored. This does
not match libgit2's common idiom of directly returning error codes.

Introduce a new function `git_strmap_set`, which takes as parameters the map,
key and value and directly returns an error code. Convert all callers of
`git_strmap_insert` to make use of it.

14 files changed, 89 insertions, 61 deletions

diff --git a/src/apply.c b/src/apply.c
index e47b6d492..4b7eedd90 100644
--- a/src/apply.c
+++ b/src/apply.c
@@ -549,7 +549,7 @@ static int apply_one(
 
 	if (delta->status == GIT_DELTA_RENAMED ||
 	    delta->status == GIT_DELTA_DELETED)
-		git_strmap_insert(removed_paths, delta->old_file.path, (char *)delta->old_file.path, &error);
+		error = git_strmap_set(removed_paths, delta->old_file.path, (char *) delta->old_file.path);
 
 	if (delta->status == GIT_DELTA_RENAMED ||
 	    delta->status == GIT_DELTA_ADDED)
diff --git a/src/attr.c b/src/attr.c
index 21ef1ba24..c6867e161 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -227,8 +227,7 @@ int git_attr_foreach(
 				if (git_strmap_exists(seen, assign->name))
 					continue;
 
-				git_strmap_insert(seen, assign->name, assign, &error);
-				if (error < 0)
+				if ((error = git_strmap_set(seen, assign->name, assign)) < 0)
 					goto cleanup;
 
 				error = callback(assign->name, assign->value, payload);
diff --git a/src/attrcache.c b/src/attrcache.c
index f1bc0df5f..00e45966c 100644
--- a/src/attrcache.c
+++ b/src/attrcache.c
@@ -75,18 +75,16 @@ int git_attr_cache__alloc_file_entry(
 static int attr_cache_make_entry(
 	git_attr_file_entry **out, git_repository *repo, const char *path)
 {
-	int error = 0;
 	git_attr_cache *cache = git_repository_attr_cache(repo);
 	git_attr_file_entry *entry = NULL;
+	int error;
 
-	error = git_attr_cache__alloc_file_entry(
-		&entry, git_repository_workdir(repo), path, &cache->pool);
+	if ((error = git_attr_cache__alloc_file_entry(&entry, git_repository_workdir(repo),
+						      path, &cache->pool)) < 0)
+		return error;
 
-	if (!error) {
-		git_strmap_insert(cache->files, entry->path, entry, &error);
-		if (error > 0)
-			error = 0;
-	}
+	if ((error = git_strmap_set(cache->files, entry->path, entry)) < 0)
+		return error;
 
 	*out = entry;
 	return error;
@@ -437,11 +435,11 @@ int git_attr_cache__insert_macro(git_repository *repo, git_attr_rule *macro)
 		git_error_set(GIT_ERROR_OS, "unable to get attr cache lock");
 		error = -1;
 	} else {
-		git_strmap_insert(macros, macro->match.pattern, macro, &error);
+		error = git_strmap_set(macros, macro->match.pattern, macro);
 		git_mutex_unlock(&cache->lock);
 	}
 
-	return (error < 0) ? -1 : 0;
+	return error;
 }
 
 git_attr_rule *git_attr_cache__lookup_macro(
diff --git a/src/config_entries.c b/src/config_entries.c
index f0f5bc8d4..18f8b21ff 100644
--- a/src/config_entries.c
+++ b/src/config_entries.c
@@ -150,10 +150,7 @@ int git_config_entries_append(git_config_entries *entries, git_config_entry *ent
 		 */
 		var->last = var;
 
-		git_strmap_insert(entries->map, entry->name, var, &error);
-
-		if (error > 0)
-			error = 0;
+		error = git_strmap_set(entries->map, entry->name, var);
 	} else {
 		config_entry_list_append(&existing, var);
 	}
diff --git a/src/diff_driver.c b/src/diff_driver.c
index 342ac247a..959cf6258 100644
--- a/src/diff_driver.c
+++ b/src/diff_driver.c
@@ -183,9 +183,9 @@ static int git_diff_driver_builtin(
 	git_diff_driver_registry *reg,
 	const char *driver_name)
 {
-	int error = 0;
 	git_diff_driver_definition *ddef = NULL;
 	git_diff_driver *drv = NULL;
+	int error = 0;
 	size_t idx;
 
 	for (idx = 0; idx < ARRAY_SIZE(builtin_defs); ++idx) {
@@ -215,9 +215,8 @@ static int git_diff_driver_builtin(
 		goto done;
 	}
 
-	git_strmap_insert(reg->drivers, drv->name, drv, &error);
-	if (error > 0)
-		error = 0;
+	if ((error = git_strmap_set(reg->drivers, drv->name, drv)) < 0)
+		goto done;
 
 done:
 	if (error && drv)
@@ -327,10 +326,8 @@ static int git_diff_driver_load(
 		goto done;
 
 	/* store driver in registry */
-	git_strmap_insert(reg->drivers, drv->name, drv, &error);
-	if (error < 0)
+	if ((error = git_strmap_set(reg->drivers, drv->name, drv)) < 0)
 		goto done;
-	error = 0;
 
 	*out = drv;
 
diff --git a/src/fileops.c b/src/fileops.c
index 988ea0f08..61906ed7f 100644
--- a/src/fileops.c
+++ b/src/fileops.c
@@ -640,8 +640,7 @@ retry_lstat:
 
 			memcpy(cache_path, make_path.ptr, make_path.size + 1);
 
-			git_strmap_insert(opts->dir_map, cache_path, cache_path, &error);
-			if (error < 0)
+			if ((error = git_strmap_set(opts->dir_map, cache_path, cache_path)) < 0)
 				goto done;
 		}
 	}
diff --git a/src/mwindow.c b/src/mwindow.c
index 3f45445d9..09e219d7c 100644
--- a/src/mwindow.c
+++ b/src/mwindow.c
@@ -79,7 +79,7 @@ int git_mwindow_get_pack(struct git_pack_file **out, const char *path)
 
 	git_atomic_inc(&pack->refcount);
 
-	git_strmap_insert(git__pack_cache, pack->pack_name, pack, &error);
+	error = git_strmap_set(git__pack_cache, pack->pack_name, pack);
 	git_mutex_unlock(&git__mwindow_mutex);
 
 	if (error < 0) {
diff --git a/src/sortedcache.c b/src/sortedcache.c
index f58764479..326aacb53 100644
--- a/src/sortedcache.c
+++ b/src/sortedcache.c
@@ -270,11 +270,10 @@ int git_sortedcache_clear(git_sortedcache *sc, bool wlock)
 /* find and/or insert item, returning pointer to item data */
 int git_sortedcache_upsert(void **out, git_sortedcache *sc, const char *key)
 {
-	size_t pos;
-	int error = 0;
-	void *item;
 	size_t keylen, itemlen;
+	int error = 0;
 	char *item_key;
+	void *item;
 
 	if ((item = git_strmap_get(sc->map, key)) != NULL)
 		goto done;
@@ -296,17 +295,11 @@ int git_sortedcache_upsert(void **out, git_sortedcache *sc, const char *key)
 	item_key = ((char *)item) + sc->item_path_offset;
 	memcpy(item_key, key, keylen);
 
-	pos = git_strmap_put(sc->map, item_key, &error);
-	if (error < 0)
+	if ((error = git_strmap_set(sc->map, item_key, item)) < 0)
 		goto done;
 
-	if (!error)
-		git_strmap_set_key_at(sc->map, pos, item_key);
-	git_strmap_set_value_at(sc->map, pos, item);
-
-	error = git_vector_insert(&sc->items, item);
-	if (error < 0)
-		git_strmap_delete_at(sc->map, pos);
+	if ((error = git_vector_insert(&sc->items, item)) < 0)
+		git_strmap_delete(sc->map, item_key);
 
 done:
 	if (out)
diff --git a/src/strmap.c b/src/strmap.c
index bf8e7f34d..4c1818a0e 100644
--- a/src/strmap.c
+++ b/src/strmap.c
@@ -50,6 +50,23 @@ void *git_strmap_get(git_strmap *map, const char *key)
 	return kh_val(map, idx);
 }
 
+int git_strmap_set(git_strmap *map, const char *key, void *value)
+{
+	size_t idx;
+	int rval;
+
+	idx = kh_put(str, map, key, &rval);
+	if (rval < 0)
+		return -1;
+
+	if (rval == 0)
+		kh_key(map, idx) = key;
+
+	kh_val(map, idx) = value;
+
+	return 0;
+}
+
 size_t git_strmap_lookup_index(git_strmap *map, const char *key)
 {
 	return kh_get(str, map, key);
diff --git a/src/strmap.h b/src/strmap.h
index dfcf9eef9..efd875fc7 100644
--- a/src/strmap.h
+++ b/src/strmap.h
@@ -59,6 +59,21 @@ size_t git_strmap_size(git_strmap *map);
  */
 void *git_strmap_get(git_strmap *map, const char *key);
 
+/**
+ * Set the entry for key to value.
+ *
+ * If the map has no corresponding entry for the given key, a new
+ * entry will be created with the given value. If an entry exists
+ * already, its value will be updated to match the given value.
+ *
+ * @param map map to create new entry in
+ * @param key key to set
+ * @param value value to associate the key with; may be NULL
+ * @return zero if the key was successfully set, a negative error
+ *         code otherwise
+ */
+int git_strmap_set(git_strmap *map, const char *key, void *value);
+
 size_t git_strmap_lookup_index(git_strmap *map, const char *key);
 int git_strmap_valid_index(git_strmap *map, size_t idx);
 
diff --git a/src/submodule.c b/src/submodule.c
index 971da11b2..aa4731ef4 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -197,8 +197,7 @@ static int load_submodule_names(git_strmap **out, git_repository *repo, git_conf
 	git_config_entry *entry;
 	git_buf buf = GIT_BUF_INIT;
 	git_strmap *names;
-	int rval, isvalid;
-	int error = 0;
+	int isvalid, error;
 
 	*out = NULL;
 
@@ -230,8 +229,7 @@ static int load_submodule_names(git_strmap **out, git_repository *repo, git_conf
 		if (!isvalid)
 			continue;
 
-		git_strmap_insert(names, git__strdup(entry->value), git_buf_detach(&buf), &rval);
-		if (rval < 0) {
+		if ((error = git_strmap_set(names, git__strdup(entry->value), git_buf_detach(&buf))) < 0) {
 			git_error_set(GIT_ERROR_NOMEMORY, "error inserting submodule into hash table");
 			error = -1;
 			goto out;
@@ -394,9 +392,8 @@ static void submodule_free_dup(void *sm)
 
 static int submodule_get_or_create(git_submodule **out, git_repository *repo, git_strmap *map, const char *name)
 {
-	int error = 0;
-	size_t pos;
 	git_submodule *sm = NULL;
+	int error;
 
 	if ((sm = git_strmap_get(map, name)) != NULL)
 		goto done;
@@ -405,16 +402,11 @@ static int submodule_get_or_create(git_submodule **out, git_repository *repo, gi
 	if ((error = submodule_alloc(&sm, repo, name)) < 0)
 		return error;
 
-	pos = git_strmap_put(map, sm->name, &error);
-	/* nobody can beat us to adding it */
-	assert(error != 0);
-	if (error < 0) {
+	if ((error = git_strmap_set(map, sm->name, sm)) < 0) {
 		git_submodule_free(sm);
 		return error;
 	}
 
-	git_strmap_set_value_at(map, pos, sm);
-
 done:
 	GIT_REFCOUNT_INC(sm);
 	*out = sm;
@@ -1961,9 +1953,7 @@ static int submodule_load_each(const git_config_entry *entry, void *payload)
 		goto done;
 	}
 
-	git_strmap_insert(map, sm->name, sm, &error);
-	assert(error != 0);
-	if (error < 0)
+	if ((error = git_strmap_set(map, sm->name, sm)) < 0)
 		goto done;
 
 	error = 0;
diff --git a/src/transaction.c b/src/transaction.c
index 8e65783f1..5dd7f42db 100644
--- a/src/transaction.c
+++ b/src/transaction.c
@@ -119,8 +119,7 @@ int git_transaction_lock_ref(git_transaction *tx, const char *refname)
 	if ((error = git_refdb_lock(&node->payload, tx->db, refname)) < 0)
 		return error;
 
-	git_strmap_insert(tx->locks, node->name, node, &error);
-	if (error < 0)
+	if ((error = git_strmap_set(tx->locks, node->name, node)) < 0)
 		goto cleanup;
 
 	return 0;
diff --git a/src/tree.c b/src/tree.c
index b1665cee3..12317540c 100644
--- a/src/tree.c
+++ b/src/tree.c
@@ -507,8 +507,7 @@ static int append_entry(
 
 	entry->attr = (uint16_t)filemode;
 
-	git_strmap_insert(bld->map, entry->filename, entry, &error);
-	if (error < 0) {
+	if ((error = git_strmap_set(bld->map, entry->filename, entry)) < 0) {
 		git_tree_entry_free(entry);
 		git_error_set(GIT_ERROR_TREE, "failed to append entry %s to the tree builder", filename);
 		return -1;
@@ -735,9 +734,7 @@ int git_treebuilder_insert(
 		entry = alloc_entry(filename, strlen(filename), id);
 		GIT_ERROR_CHECK_ALLOC(entry);
 
-		git_strmap_insert(bld->map, entry->filename, entry, &error);
-
-		if (error < 0) {
+		if ((error = git_strmap_set(bld->map, entry->filename, entry)) < 0) {
 			git_tree_entry_free(entry);
 			git_error_set(GIT_ERROR_TREE, "failed to insert %s", filename);
 			return -1;
diff --git a/tests/core/strmap.c b/tests/core/strmap.c
index 72885c8b1..4d36b744e 100644
--- a/tests/core/strmap.c
+++ b/tests/core/strmap.c
@@ -129,3 +129,30 @@ void test_core_strmap__get_returns_null_on_nonexisting_key(void)
 
 	cl_assert_equal_p(git_strmap_get(g_table, "other"), NULL);
 }
+
+void test_core_strmap__set_persists_key(void)
+{
+	cl_git_pass(git_strmap_set(g_table, "foo", "oof"));
+	cl_assert_equal_s(git_strmap_get(g_table, "foo"), "oof");
+}
+
+void test_core_strmap__set_persists_multpile_keys(void)
+{
+	cl_git_pass(git_strmap_set(g_table, "foo", "oof"));
+	cl_git_pass(git_strmap_set(g_table, "bar", "rab"));
+	cl_assert_equal_s(git_strmap_get(g_table, "foo"), "oof");
+	cl_assert_equal_s(git_strmap_get(g_table, "bar"), "rab");
+}
+
+void test_core_strmap__set_updates_existing_key(void)
+{
+	cl_git_pass(git_strmap_set(g_table, "foo", "oof"));
+	cl_git_pass(git_strmap_set(g_table, "bar", "rab"));
+	cl_git_pass(git_strmap_set(g_table, "gobble", "elbbog"));
+	cl_assert_equal_i(git_strmap_size(g_table), 3);
+
+	cl_git_pass(git_strmap_set(g_table, "foo", "other"));
+	cl_assert_equal_i(git_strmap_size(g_table), 3);
+
+	cl_assert_equal_s(git_strmap_get(g_table, "foo"), "other");
+}