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

Currently, one would use either `git_oidmap_insert` to insert key/value pairs
into a map or `git_oidmap_put` to insert a key only. These function have
historically been macros, which is why their syntax is kind of weird: instead of
returning an error code directly, they instead have 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.Furthermore, `git_oidmap_put` is tightly
coupled with implementation details of the map as it exposes the index of
inserted entries.

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

Author: pks-t

src/cache.c

@@ -192,10 +192,7 @@ static void *cache_store(git_cache *cache, git_cached_obj *entry)
 
 	/* not found */
 	if ((stored_entry = git_oidmap_get(cache->map, &entry->oid)) == NULL) {
-		int rval;
-
-		git_oidmap_insert(cache->map, &entry->oid, entry, &rval);
-		if (rval >= 0) {
+		if (git_oidmap_set(cache->map, &entry->oid, entry) == 0) {
 			git_cached_obj_incref(entry);
 			cache->used_memory += entry->size;
 			git_atomic_ssize_add(&git_cache__current_storage, (ssize_t)entry->size);
@@ -209,12 +206,10 @@ static void *cache_store(git_cache *cache, git_cached_obj *entry)
 			entry = stored_entry;
 		} else if (stored_entry->flags == GIT_CACHE_STORE_RAW &&
 			   entry->flags == GIT_CACHE_STORE_PARSED) {
-			int rval;
-
 			git_cached_obj_decref(stored_entry);
 			git_cached_obj_incref(entry);
 
-			git_oidmap_insert(cache->map, &entry->oid, entry, &rval);
+			git_oidmap_set(cache->map, &entry->oid, entry);
 		} else {
 			/* NO OP */
 		}

src/describe.c

@@ -119,13 +119,8 @@ static int add_to_known_names(
 		e->path = git__strdup(path);
 		git_oid_cpy(&e->peeled, peeled);
 
-		if (!found) {
-			int ret;
-
-			git_oidmap_insert(names, &e->peeled, e, &ret);
-			if (ret < 0)
-				return -1;
-		}
+		if (!found && git_oidmap_set(names, &e->peeled, e) < 0)
+			return -1;
 	}
 	else
 		git_tag_free(tag);

src/indexer.c

@@ -309,10 +309,8 @@ static int crc_object(uint32_t *crc_out, git_mwindow_file *mwf, git_off_t start,
 	return 0;
 }
 
-static void add_expected_oid(git_indexer *idx, const git_oid *oid)
+static int add_expected_oid(git_indexer *idx, const git_oid *oid)
 {
-	int ret;
-
 	/*
 	 * If we know about that object because it is stored in our ODB or
 	 * because we have already processed it as part of our pack file, we do
@@ -323,8 +321,10 @@ static void add_expected_oid(git_indexer *idx, const git_oid *oid)
 	    !git_oidmap_exists(idx->expected_oids, oid)) {
 		    git_oid *dup = git__malloc(sizeof(*oid));
 		    git_oid_cpy(dup, oid);
-		    git_oidmap_put(idx->expected_oids, dup, &ret);
+		    return git_oidmap_set(idx->expected_oids, dup, NULL);
 	}
+
+	return 0;
 }
 
 static int check_object_connectivity(git_indexer *idx, const git_rawobj *obj)
@@ -364,7 +364,8 @@ static int check_object_connectivity(git_indexer *idx, const git_rawobj *obj)
 			size_t i;
 
 			git_array_foreach(tree->entries, i, entry)
-				add_expected_oid(idx, entry->oid);
+				if (add_expected_oid(idx, entry->oid) < 0)
+					goto out;
 
 			break;
 		}
@@ -375,17 +376,20 @@ static int check_object_connectivity(git_indexer *idx, const git_rawobj *obj)
 			size_t i;
 
 			git_array_foreach(commit->parent_ids, i, parent_oid)
-				add_expected_oid(idx, parent_oid);
+				if (add_expected_oid(idx, parent_oid) < 0)
+					goto out;
 
-			add_expected_oid(idx, &commit->tree_id);
+			if (add_expected_oid(idx, &commit->tree_id) < 0)
+				goto out;
 
 			break;
 		}
 		case GIT_OBJECT_TAG:
 		{
 			git_tag *tag = (git_tag *) object;
 
-			add_expected_oid(idx, &tag->target);
+			if (add_expected_oid(idx, &tag->target) < 0)
+				goto out;
 
 			break;
 		}
@@ -403,7 +407,6 @@ static int check_object_connectivity(git_indexer *idx, const git_rawobj *obj)
 static int store_object(git_indexer *idx)
 {
 	int i, error;
-	size_t k;
 	git_oid oid;
 	struct entry *entry;
 	git_off_t entry_size;
@@ -439,22 +442,18 @@ static int store_object(git_indexer *idx)
 	git_oid_cpy(&pentry->sha1, &oid);
 	pentry->offset = entry_start;
 
-	k = git_oidmap_put(idx->pack->idx_cache, &pentry->sha1, &error);
-	if (error == -1) {
+	if (git_oidmap_exists(idx->pack->idx_cache, &pentry->sha1)) {
+		git_error_set(GIT_ERROR_INDEXER, "duplicate object %s found in pack", git_oid_tostr_s(&pentry->sha1));
 		git__free(pentry);
-		git_error_set_oom();
 		goto on_error;
 	}
 
-	if (error == 0) {
-		git_error_set(GIT_ERROR_INDEXER, "duplicate object %s found in pack", git_oid_tostr_s(&pentry->sha1));
+	if ((error = git_oidmap_set(idx->pack->idx_cache, &pentry->sha1, pentry)) < 0) {
 		git__free(pentry);
+		git_error_set_oom();
 		goto on_error;
 	}
 
-
-	git_oidmap_set_value_at(idx->pack->idx_cache, k, pentry);
-
 	git_oid_cpy(&entry->oid, &oid);
 
 	if (crc_object(&entry->crc, &idx->pack->mwf, entry_start, entry_size) < 0)
@@ -483,8 +482,7 @@ GIT_INLINE(bool) has_entry(git_indexer *idx, git_oid *id)
 
 static int save_entry(git_indexer *idx, struct entry *entry, struct git_pack_entry *pentry, git_off_t entry_start)
 {
-	int i, error;
-	size_t k;
+	int i;
 
 	if (entry_start > UINT31_MAX) {
 		entry->offset = UINT32_MAX;
@@ -494,15 +492,13 @@ static int save_entry(git_indexer *idx, struct entry *entry, struct git_pack_ent
 	}
 
 	pentry->offset = entry_start;
-	k = git_oidmap_put(idx->pack->idx_cache, &pentry->sha1, &error);
 
-	if (error <= 0) {
+	if (git_oidmap_exists(idx->pack->idx_cache, &pentry->sha1) ||
+	    git_oidmap_set(idx->pack->idx_cache, &pentry->sha1, pentry) < 0) {
 		git_error_set(GIT_ERROR_INDEXER, "cannot insert object into pack");
 		return -1;
 	}
 
-	git_oidmap_set_value_at(idx->pack->idx_cache, k, pentry);
-
 	/* Add the object to the list */
 	if (git_vector_insert(&idx->objects, entry) < 0)
 		return -1;

src/merge.c

@@ -1108,7 +1108,6 @@ static int deletes_by_oid_enqueue(git_oidmap *map, git_pool* pool, const git_oid
 {
 	deletes_by_oid_queue *queue;
 	size_t *array_entry;
-	int error;
 
 	if ((queue = git_oidmap_get(map, id)) == NULL) {
 		queue = git_pool_malloc(pool, sizeof(deletes_by_oid_queue));
@@ -1118,8 +1117,7 @@ static int deletes_by_oid_enqueue(git_oidmap *map, git_pool* pool, const git_oid
 		queue->next_pos = 0;
 		queue->first_entry = idx;
 
-		git_oidmap_insert(map, id, queue, &error);
-		if (error < 0)
+		if (git_oidmap_set(map, id, queue) < 0)
 			return -1;
 	} else {
 		array_entry = git_array_alloc(queue->arr);

src/odb_mempack.c

@@ -37,15 +37,9 @@ static int impl__write(git_odb_backend *_backend, const git_oid *oid, const void
 {
 	struct memory_packer_db *db = (struct memory_packer_db *)_backend;
 	struct memobject *obj = NULL;
-	size_t pos;
 	size_t alloc_len;
-	int rval;
 
-	pos = git_oidmap_put(db->objects, oid, &rval);
-	if (rval < 0)
-		return -1;
-
-	if (rval == 0)
+	if (git_oidmap_exists(db->objects, oid))
 		return 0;
 
 	GIT_ERROR_CHECK_ALLOC_ADD(&alloc_len, sizeof(struct memobject), len);
@@ -57,8 +51,8 @@ static int impl__write(git_odb_backend *_backend, const git_oid *oid, const void
 	obj->len = len;
 	obj->type = type;
 
-	git_oidmap_set_key_at(db->objects, pos, &obj->oid);
-	git_oidmap_set_value_at(db->objects, pos, obj);
+	if (git_oidmap_set(db->objects, &obj->oid, obj) < 0)
+		return -1;
 
 	if (type == GIT_OBJECT_COMMIT) {
 		struct memobject **store = git_array_alloc(db->commits);

src/oidmap.c

@@ -57,6 +57,23 @@ void *git_oidmap_get(git_oidmap *map, const git_oid *key)
 	return kh_val(map, idx);
 }
 
+int git_oidmap_set(git_oidmap *map, const git_oid *key, void *value)
+{
+	size_t idx;
+	int rval;
+
+	idx = kh_put(oid, 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_oidmap_lookup_index(git_oidmap *map, const git_oid *key)
 {
 	return kh_get(oid, map, key);

src/oidmap.h

@@ -61,6 +61,21 @@ size_t git_oidmap_size(git_oidmap *map);
  */
 void *git_oidmap_get(git_oidmap *map, const git_oid *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_oidmap_set(git_oidmap *map, const git_oid *key, void *value);
+
 size_t git_oidmap_lookup_index(git_oidmap *map, const git_oid *key);
 int git_oidmap_valid_index(git_oidmap *map, size_t idx);
 

src/pack-objects.c

@@ -192,24 +192,26 @@ unsigned int git_packbuilder_set_threads(git_packbuilder *pb, unsigned int n)
 	return pb->nr_threads;
 }
 
-static void rehash(git_packbuilder *pb)
+static int rehash(git_packbuilder *pb)
 {
 	git_pobject *po;
-	size_t pos, i;
-	int ret;
+	size_t i;
 
 	git_oidmap_clear(pb->object_ix);
+
 	for (i = 0, po = pb->object_list; i < pb->nr_objects; i++, po++) {
-		pos = git_oidmap_put(pb->object_ix, &po->id, &ret);
-		git_oidmap_set_value_at(pb->object_ix, pos, po);
+		if (git_oidmap_set(pb->object_ix, &po->id, po) < 0)
+			return -1;
 	}
+
+	return 0;
 }
 
 int git_packbuilder_insert(git_packbuilder *pb, const git_oid *oid,
 			   const char *name)
 {
 	git_pobject *po;
-	size_t newsize, pos;
+	size_t newsize;
 	int ret;
 
 	assert(pb && oid);
@@ -233,7 +235,9 @@ int git_packbuilder_insert(git_packbuilder *pb, const git_oid *oid,
 		pb->object_list = git__reallocarray(pb->object_list,
 			pb->nr_alloc, sizeof(*po));
 		GIT_ERROR_CHECK_ALLOC(pb->object_list);
-		rehash(pb);
+
+		if (rehash(pb) < 0)
+			return -1;
 	}
 
 	po = pb->object_list + pb->nr_objects;
@@ -246,13 +250,10 @@ int git_packbuilder_insert(git_packbuilder *pb, const git_oid *oid,
 	git_oid_cpy(&po->id, oid);
 	po->hash = name_hash(name);
 
-	pos = git_oidmap_put(pb->object_ix, &po->id, &ret);
-	if (ret < 0) {
+	if (git_oidmap_set(pb->object_ix, &po->id, po) < 0) {
 		git_error_set_oom();
-		return ret;
+		return -1;
 	}
-	assert(ret != 0);
-	git_oidmap_set_value_at(pb->object_ix, pos, po);
 
 	pb->done = false;
 
@@ -1541,7 +1542,8 @@ static int retrieve_object(struct walk_object **out, git_packbuilder *pb, const
 		if ((error = lookup_walk_object(&obj, pb, id)) < 0)
 			return error;
 
-		git_oidmap_insert(pb->walk_objects, &obj->id, obj, &error);
+		if ((error = git_oidmap_set(pb->walk_objects, &obj->id, obj)) < 0)
+			return error;
 	}
 
 	*out = obj;

src/revwalk.c

@@ -21,8 +21,6 @@ git_commit_list_node *git_revwalk__commit_lookup(
 	git_revwalk *walk, const git_oid *oid)
 {
 	git_commit_list_node *commit;
-	size_t pos;
-	int ret;
 
 	/* lookup and reserve space if not already present */
 	if ((commit = git_oidmap_get(walk->commits, oid)) != NULL)
@@ -34,9 +32,8 @@ git_commit_list_node *git_revwalk__commit_lookup(
 
 	git_oid_cpy(&commit->oid, oid);
 
-	pos = git_oidmap_put(walk->commits, &commit->oid, &ret);
-	assert(ret != 0);
-	git_oidmap_set_value_at(walk->commits, pos, commit);
+	if ((git_oidmap_set(walk->commits, &commit->oid, commit)) < 0)
+		return NULL;
 
 	return commit;
 }

tests/core/oidmap.c

@@ -182,3 +182,47 @@ void test_core_oidmap__get_fails_with_nonexisting_key(void)
 
 	git_oidmap_free(map);
 }
+
+void test_core_oidmap__setting_oid_persists(void)
+{
+	git_oid oids[] = {
+	    {{ 0x01 }},
+	    {{ 0x02 }},
+	    {{ 0x03 }}
+	};
+	git_oidmap *map;
+
+	cl_git_pass(git_oidmap_new(&map));
+	cl_git_pass(git_oidmap_set(map, &oids[0], "one"));
+	cl_git_pass(git_oidmap_set(map, &oids[1], "two"));
+	cl_git_pass(git_oidmap_set(map, &oids[2], "three"));
+
+	cl_assert_equal_s(git_oidmap_get(map, &oids[0]), "one");
+	cl_assert_equal_s(git_oidmap_get(map, &oids[1]), "two");
+	cl_assert_equal_s(git_oidmap_get(map, &oids[2]), "three");
+
+	git_oidmap_free(map);
+}
+
+void test_core_oidmap__setting_existing_key_updates(void)
+{
+	git_oid oids[] = {
+	    {{ 0x01 }},
+	    {{ 0x02 }},
+	    {{ 0x03 }}
+	};
+	git_oidmap *map;
+
+	cl_git_pass(git_oidmap_new(&map));
+	cl_git_pass(git_oidmap_set(map, &oids[0], "one"));
+	cl_git_pass(git_oidmap_set(map, &oids[1], "two"));
+	cl_git_pass(git_oidmap_set(map, &oids[2], "three"));
+	cl_assert_equal_i(git_oidmap_size(map), 3);
+
+	cl_git_pass(git_oidmap_set(map, &oids[1], "other"));
+	cl_assert_equal_i(git_oidmap_size(map), 3);
+
+	cl_assert_equal_s(git_oidmap_get(map, &oids[1]), "other");
+
+	git_oidmap_free(map);
+}