strmap: introduce `git_strmap_get` and use it throughout the tree

The current way of looking up an entry from a map is tightly coupled with the
map implementation, as one first has to look up the index of the key and then
retrieve the associated value by using the index. As a caller, you usually do
not care about any indices at all, though, so this is more complicated than
really necessary. Furthermore, it invites for errors to happen if the correct
error checking sequence is not being followed.

Introduce a new high-level function `git_strmap_get` that takes a map and a key
and returns a pointer to the associated value if such a key exists. Otherwise,
a `NULL` pointer is returned. Adjust all callers that can trivially be
converted.

11 files changed, 79 insertions, 96 deletions

diff --git a/src/attrcache.c b/src/attrcache.c
index 6d241e4a5..f1bc0df5f 100644
--- a/src/attrcache.c
+++ b/src/attrcache.c
@@ -33,12 +33,7 @@ GIT_INLINE(void) attr_cache_unlock(git_attr_cache *cache)
 GIT_INLINE(git_attr_file_entry *) attr_cache_lookup_entry(
 	git_attr_cache *cache, const char *path)
 {
-	size_t pos = git_strmap_lookup_index(cache->files, path);
-
-	if (git_strmap_valid_index(cache->files, pos))
-		return git_strmap_value_at(cache->files, pos);
-	else
-		return NULL;
+	return git_strmap_get(cache->files, path);
 }
 
 int git_attr_cache__alloc_file_entry(
@@ -265,19 +260,15 @@ bool git_attr_cache__is_cached(
 	const char *filename)
 {
 	git_attr_cache *cache = git_repository_attr_cache(repo);
-	git_strmap *files;
-	size_t pos;
 	git_attr_file_entry *entry;
+	git_strmap *files;
 
 	if (!cache || !(files = cache->files))
 		return false;
 
-	pos = git_strmap_lookup_index(files, filename);
-	if (!git_strmap_valid_index(files, pos))
+	if ((entry = git_strmap_get(files, filename)) == NULL)
 		return false;
 
-	entry = git_strmap_value_at(files, pos);
-
 	return entry && (entry->file[source] != NULL);
 }
 
@@ -457,13 +448,6 @@ git_attr_rule *git_attr_cache__lookup_macro(
 	git_repository *repo, const char *name)
 {
 	git_strmap *macros = git_repository_attr_cache(repo)->macros;
-	size_t pos;
 
-	pos = git_strmap_lookup_index(macros, name);
-
-	if (!git_strmap_valid_index(macros, pos))
-		return NULL;
-
-	return (git_attr_rule *)git_strmap_value_at(macros, pos);
+	return git_strmap_get(macros, name);
 }
-
diff --git a/src/config_entries.c b/src/config_entries.c
index c4551fb67..f0f5bc8d4 100644
--- a/src/config_entries.c
+++ b/src/config_entries.c
@@ -133,14 +133,12 @@ int git_config_entries_append(git_config_entries *entries, git_config_entry *ent
 {
 	config_entry_list *existing, *var;
 	int error = 0;
-	size_t pos;
 
 	var = git__calloc(1, sizeof(config_entry_list));
 	GIT_ERROR_CHECK_ALLOC(var);
 	var->entry = entry;
 
-	pos = git_strmap_lookup_index(entries->map, entry->name);
-	if (!git_strmap_valid_index(entries->map, pos)) {
+	if ((existing = git_strmap_get(entries->map, entry->name)) == NULL) {
 		/*
 		 * We only ever inspect `last` from the first config
 		 * entry in a multivar. In case where this new entry is
@@ -157,7 +155,6 @@ int git_config_entries_append(git_config_entries *entries, git_config_entry *ent
 		if (error > 0)
 			error = 0;
 	} else {
-		existing = git_strmap_value_at(entries->map, pos);
 		config_entry_list_append(&existing, var);
 	}
 
@@ -171,15 +168,12 @@ int git_config_entries_append(git_config_entries *entries, git_config_entry *ent
 
 int config_entry_get(config_entry_list **out, git_config_entries *entries, const char *key)
 {
-	size_t pos;
-
-	pos = git_strmap_lookup_index(entries->map, key);
+	config_entry_list *list;
 
-	/* no error message; the config system will write one */
-	if (!git_strmap_valid_index(entries->map, pos))
+	if ((list = git_strmap_get(entries->map, key)) == NULL)
 		return GIT_ENOTFOUND;
 
-	*out = git_strmap_value_at(entries->map, pos);
+	*out = list;
 
 	return 0;
 }
diff --git a/src/diff_driver.c b/src/diff_driver.c
index 9bc266e02..342ac247a 100644
--- a/src/diff_driver.c
+++ b/src/diff_driver.c
@@ -233,8 +233,8 @@ static int git_diff_driver_load(
 {
 	int error = 0;
 	git_diff_driver_registry *reg;
-	git_diff_driver *drv = NULL;
-	size_t namelen, pos;
+	git_diff_driver *drv;
+	size_t namelen;
 	git_config *cfg = NULL;
 	git_buf name = GIT_BUF_INIT;
 	git_config_entry *ce = NULL;
@@ -243,9 +243,8 @@ static int git_diff_driver_load(
 	if ((reg = git_repository_driver_registry(repo)) == NULL)
 		return -1;
 
-	pos = git_strmap_lookup_index(reg->drivers, driver_name);
-	if (git_strmap_valid_index(reg->drivers, pos)) {
-		*out = git_strmap_value_at(reg->drivers, pos);
+	if ((drv = git_strmap_get(reg->drivers, driver_name)) != NULL) {
+		*out = drv;
 		return 0;
 	}
 
diff --git a/src/mwindow.c b/src/mwindow.c
index 07d3b15ed..3f45445d9 100644
--- a/src/mwindow.c
+++ b/src/mwindow.c
@@ -49,10 +49,9 @@ int git_mwindow_global_init(void)
 
 int git_mwindow_get_pack(struct git_pack_file **out, const char *path)
 {
-	int error;
-	char *packname;
-	size_t pos;
 	struct git_pack_file *pack;
+	char *packname;
+	int error;
 
 	if ((error = git_packfile__name(&packname, path)) < 0)
 		return error;
@@ -62,13 +61,11 @@ int git_mwindow_get_pack(struct git_pack_file **out, const char *path)
 		return -1;
 	}
 
-	pos = git_strmap_lookup_index(git__pack_cache, packname);
+	pack = git_strmap_get(git__pack_cache, packname);
 	git__free(packname);
 
-	if (git_strmap_valid_index(git__pack_cache, pos)) {
-		pack = git_strmap_value_at(git__pack_cache, pos);
+	if (pack != NULL) {
 		git_atomic_inc(&pack->refcount);
-
 		git_mutex_unlock(&git__mwindow_mutex);
 		*out = pack;
 		return 0;
diff --git a/src/sortedcache.c b/src/sortedcache.c
index 3967bce15..f58764479 100644
--- a/src/sortedcache.c
+++ b/src/sortedcache.c
@@ -276,11 +276,8 @@ int git_sortedcache_upsert(void **out, git_sortedcache *sc, const char *key)
 	size_t keylen, itemlen;
 	char *item_key;
 
-	pos = git_strmap_lookup_index(sc->map, key);
-	if (git_strmap_valid_index(sc->map, pos)) {
-		item = git_strmap_value_at(sc->map, pos);
+	if ((item = git_strmap_get(sc->map, key)) != NULL)
 		goto done;
-	}
 
 	keylen  = strlen(key);
 	itemlen = sc->item_path_offset + keylen + 1;
@@ -320,10 +317,7 @@ done:
 /* lookup item by key */
 void *git_sortedcache_lookup(const git_sortedcache *sc, const char *key)
 {
-	size_t pos = git_strmap_lookup_index(sc->map, key);
-	if (git_strmap_valid_index(sc->map, pos))
-		return git_strmap_value_at(sc->map, pos);
-	return NULL;
+	return git_strmap_get(sc->map, key);
 }
 
 /* find out how many items are in the cache */
diff --git a/src/strmap.c b/src/strmap.c
index a2132285e..bf8e7f34d 100644
--- a/src/strmap.c
+++ b/src/strmap.c
@@ -41,6 +41,15 @@ size_t git_strmap_size(git_strmap *map)
 	return kh_size(map);
 }
 
+void *git_strmap_get(git_strmap *map, const char *key)
+{
+	size_t idx = git_strmap_lookup_index(map, key);
+	if (!git_strmap_valid_index(map, idx) ||
+	    !git_strmap_has_data(map, idx))
+		return NULL;
+	return kh_val(map, idx);
+}
+
 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 1b7f1a4b7..dfcf9eef9 100644
--- a/src/strmap.h
+++ b/src/strmap.h
@@ -50,6 +50,15 @@ void git_strmap_clear(git_strmap *map);
  */
 size_t git_strmap_size(git_strmap *map);
 
+/**
+ * Return value associated with the given key.
+ *
+ * @param map map to search key in
+ * @param key key to search for
+ * @return value associated with the given key or NULL if the key was not found
+ */
+void *git_strmap_get(git_strmap *map, const char *key);
+
 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 72921fa3e..971da11b2 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -267,10 +267,9 @@ int git_submodule_lookup(
 	}
 
 	if (repo->submodule_cache != NULL) {
-		size_t pos = git_strmap_lookup_index(repo->submodule_cache, name);
-		if (git_strmap_valid_index(repo->submodule_cache, pos)) {
+		if ((sm = git_strmap_get(repo->submodule_cache, name)) != NULL) {
 			if (out) {
-				*out = git_strmap_value_at(repo->submodule_cache, pos);
+				*out = sm;
 				GIT_REFCOUNT_INC(*out);
 			}
 			return 0;
@@ -399,11 +398,8 @@ static int submodule_get_or_create(git_submodule **out, git_repository *repo, gi
 	size_t pos;
 	git_submodule *sm = NULL;
 
-	pos = git_strmap_lookup_index(map, name);
-	if (git_strmap_valid_index(map, pos)) {
-		sm = git_strmap_value_at(map, pos);
+	if ((sm = git_strmap_get(map, name)) != NULL)
 		goto done;
-	}
 
 	/* if the submodule doesn't exist yet in the map, create it */
 	if ((error = submodule_alloc(&sm, repo, name)) < 0)
@@ -439,26 +435,18 @@ static int submodules_from_index(git_strmap *map, git_index *idx, git_config *cf
 		goto done;
 
 	while (!(error = git_iterator_advance(&entry, i))) {
-		size_t pos = git_strmap_lookup_index(map, entry->path);
 		git_submodule *sm;
 
-		if (git_strmap_valid_index(map, pos)) {
-			sm = git_strmap_value_at(map, pos);
-
+		if ((sm = git_strmap_get(map, entry->path)) != NULL) {
 			if (S_ISGITLINK(entry->mode))
 				submodule_update_from_index_entry(sm, entry);
 			else
 				sm->flags |= GIT_SUBMODULE_STATUS__INDEX_NOT_SUBMODULE;
 		} else if (S_ISGITLINK(entry->mode)) {
-			size_t name_pos;
 			const char *name;
 
-			name_pos = git_strmap_lookup_index(names, entry->path);
-			if (git_strmap_valid_index(names, name_pos)) {
-				name = git_strmap_value_at(names, name_pos);
-			} else {
+			if ((name = git_strmap_get(names, entry->path)) == NULL)
 				name = entry->path;
-			}
 
 			if (!submodule_get_or_create(&sm, git_index_owner(idx), map, name)) {
 				submodule_update_from_index_entry(sm, entry);
@@ -491,26 +479,18 @@ static int submodules_from_head(git_strmap *map, git_tree *head, git_config *cfg
 		goto done;
 
 	while (!(error = git_iterator_advance(&entry, i))) {
-		size_t pos = git_strmap_lookup_index(map, entry->path);
 		git_submodule *sm;
 
-		if (git_strmap_valid_index(map, pos)) {
-			sm = git_strmap_value_at(map, pos);
-
+		if ((sm = git_strmap_get(map, entry->path)) != NULL) {
 			if (S_ISGITLINK(entry->mode))
 				submodule_update_from_head_data(sm, entry->mode, &entry->id);
 			else
 				sm->flags |= GIT_SUBMODULE_STATUS__HEAD_NOT_SUBMODULE;
 		} else if (S_ISGITLINK(entry->mode)) {
-			size_t name_pos;
 			const char *name;
 
-			name_pos = git_strmap_lookup_index(names, entry->path);
-			if (git_strmap_valid_index(names, name_pos)) {
-				name = git_strmap_value_at(names, name_pos);
-			} else {
+			if ((name = git_strmap_get(names, entry->path)) == NULL)
 				name = entry->path;
-			}
 
 			if (!submodule_get_or_create(&sm, git_tree_owner(head), map, name)) {
 				submodule_update_from_head_data(
diff --git a/src/transaction.c b/src/transaction.c
index a09b2eb46..8e65783f1 100644
--- a/src/transaction.c
+++ b/src/transaction.c
@@ -134,16 +134,12 @@ cleanup:
 static int find_locked(transaction_node **out, git_transaction *tx, const char *refname)
 {
 	transaction_node *node;
-	size_t pos;
 
-	pos = git_strmap_lookup_index(tx->locks, refname);
-	if (!git_strmap_valid_index(tx->locks, pos)) {
+	if ((node = git_strmap_get(tx->locks, refname)) == NULL) {
 		git_error_set(GIT_ERROR_REFERENCE, "the specified reference is not locked");
 		return GIT_ENOTFOUND;
 	}
 
-	node = git_strmap_value_at(tx->locks, pos);
-
 	*out = node;
 	return 0;
 }
diff --git a/src/tree.c b/src/tree.c
index cc6f93db5..b1665cee3 100644
--- a/src/tree.c
+++ b/src/tree.c
@@ -723,16 +723,13 @@ int git_treebuilder_insert(
 {
 	git_tree_entry *entry;
 	int error;
-	size_t pos;
 
 	assert(bld && id && filename);
 
 	if ((error = check_entry(bld->repo, filename, id, filemode)) < 0)
 		return error;
 
-	pos = git_strmap_lookup_index(bld->map, filename);
-	if (git_strmap_valid_index(bld->map, pos)) {
-		entry = git_strmap_value_at(bld->map, pos);
+	if ((entry = git_strmap_get(bld->map, filename)) != NULL) {
 		git_oid_cpy((git_oid *) entry->oid, id);
 	} else {
 		entry = alloc_entry(filename, strlen(filename), id);
@@ -757,16 +754,8 @@ int git_treebuilder_insert(
 
 static git_tree_entry *treebuilder_get(git_treebuilder *bld, const char *filename)
 {
-	git_tree_entry *entry = NULL;
-	size_t pos;
-
 	assert(bld && filename);
-
-	pos = git_strmap_lookup_index(bld->map, filename);
-	if (git_strmap_valid_index(bld->map, pos))
-		entry = git_strmap_value_at(bld->map, pos);
-
-	return entry;
+	return git_strmap_get(bld->map, filename);
 }
 
 const git_tree_entry *git_treebuilder_get(git_treebuilder *bld, const char *filename)
diff --git a/tests/core/strmap.c b/tests/core/strmap.c
index d6af9773d..72885c8b1 100644
--- a/tests/core/strmap.c
+++ b/tests/core/strmap.c
@@ -1,7 +1,7 @@
 #include "clar_libgit2.h"
 #include "strmap.h"
 
-git_strmap *g_table;
+static git_strmap *g_table;
 
 void test_core_strmap__initialize(void)
 {
@@ -97,3 +97,35 @@ void test_core_strmap__3(void)
 	git_strmap_foreach_value(g_table, str, { i++; free(str); });
 	cl_assert(i == 10000);
 }
+
+void test_core_strmap__get_succeeds_with_existing_entries(void)
+{
+	const char *keys[] = { "foo", "bar", "gobble" };
+	char *values[] = { "oof", "rab", "elbbog" };
+	int error;
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(keys); i++) {
+		git_strmap_insert(g_table, keys[i], values[i], &error);
+		cl_assert_equal_i(error, 1);
+	}
+
+	cl_assert_equal_s(git_strmap_get(g_table, "foo"), "oof");
+	cl_assert_equal_s(git_strmap_get(g_table, "bar"), "rab");
+	cl_assert_equal_s(git_strmap_get(g_table, "gobble"), "elbbog");
+}
+
+void test_core_strmap__get_returns_null_on_nonexisting_key(void)
+{
+	const char *keys[] = { "foo", "bar", "gobble" };
+	char *values[] = { "oof", "rab", "elbbog" };
+	int error;
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(keys); i++) {
+		git_strmap_insert(g_table, keys[i], values[i], &error);
+		cl_assert_equal_i(error, 1);
+	}
+
+	cl_assert_equal_p(git_strmap_get(g_table, "other"), NULL);
+}