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.

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);
 }
-

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;
 }

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;
 	}
 

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;

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 */

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);

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);
 

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(

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;
 }

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)

tests/core/strmap.c

 #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);
+}