Commit 2e0a304839764236654e73d38fa380b317a3fac1

Patrick Steinhardt 2019-01-23T10:48:55

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.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
diff --git a/src/cache.c b/src/cache.c
index ec3d9d1..a790783 100644
--- a/src/cache.c
+++ b/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 */
 		}
diff --git a/src/describe.c b/src/describe.c
index 62d8860..943fa55 100644
--- a/src/describe.c
+++ b/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);
diff --git a/src/indexer.c b/src/indexer.c
index f898d20..9ad78e8 100644
--- a/src/indexer.c
+++ b/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,9 +376,11 @@ 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;
 		}
@@ -385,7 +388,8 @@ static int check_object_connectivity(git_indexer *idx, const git_rawobj *obj)
 		{
 			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 @@ out:
 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;
diff --git a/src/merge.c b/src/merge.c
index b8bb194..fbae704 100644
--- a/src/merge.c
+++ b/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);
diff --git a/src/odb_mempack.c b/src/odb_mempack.c
index e66f2e9..fc23023 100644
--- a/src/odb_mempack.c
+++ b/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);
diff --git a/src/oidmap.c b/src/oidmap.c
index 1e2b812..2890fdb 100644
--- a/src/oidmap.c
+++ b/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);
diff --git a/src/oidmap.h b/src/oidmap.h
index 799fec4..39987d3 100644
--- a/src/oidmap.h
+++ b/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);
 
diff --git a/src/pack-objects.c b/src/pack-objects.c
index 094317a..b2e2457 100644
--- a/src/pack-objects.c
+++ b/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;
diff --git a/src/revwalk.c b/src/revwalk.c
index a052a64..6d12d34 100644
--- a/src/revwalk.c
+++ b/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;
 }
diff --git a/tests/core/oidmap.c b/tests/core/oidmap.c
index 0e66b72..38b4c7d 100644
--- a/tests/core/oidmap.c
+++ b/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);
+}