Add callback to git_objects_table

This adds create and free callback to the git_objects_table so
that more of the creation and destruction of objects can be table
driven instead of using switch statements.  This also makes the
semantics of certain object creation functions consistent so that
we can make better use of function pointers.  This also fixes a
theoretical error case where an object allocation fails and we
end up storing NULL into the cache.

src/blob.c

@@ -35,17 +35,17 @@ int git_blob__getbuf(git_buf *buffer, git_blob *blob)
 		git_odb_object_size(blob->odb_object));
 }
 
-void git_blob__free(git_blob *blob)
+void git_blob__free(void *blob)
 {
-	git_odb_object_free(blob->odb_object);
+	git_odb_object_free(((git_blob *)blob)->odb_object);
 	git__free(blob);
 }
 
-int git_blob__parse(git_blob *blob, git_odb_object *odb_obj)
+int git_blob__from_odb_object(void *blob, git_odb_object *odb_obj)
 {
 	assert(blob);
 	git_cached_obj_incref((git_cached_obj *)odb_obj);
-	blob->odb_object = odb_obj;
+	((git_blob *)blob)->odb_object = odb_obj;
 	return 0;
 }
 

src/blob.h

@@ -17,8 +17,8 @@ struct git_blob {
 	git_odb_object *odb_object;
 };
 
-void git_blob__free(git_blob *blob);
-int git_blob__parse(git_blob *blob, git_odb_object *obj);
+void git_blob__free(void *blob);
+int git_blob__from_odb_object(void *blob, git_odb_object *obj);
 int git_blob__getbuf(git_buf *buffer, git_blob *blob);
 
 #endif

src/cache.c

@@ -70,12 +70,6 @@ int git_cache_init(git_cache *cache)
 	return 0;
 }
 
-void git_cache_free(git_cache *cache)
-{
-	git_oidmap_free(cache->map);
-	git_mutex_free(&cache->lock);
-}
-
 void git_cache_clear(git_cache *cache)
 {
 	git_cached_obj *evict = NULL;
@@ -93,6 +87,14 @@ void git_cache_clear(git_cache *cache)
 	git_mutex_unlock(&cache->lock);
 }
 
+void git_cache_free(git_cache *cache)
+{
+	git_cache_clear(cache);
+
+	git_oidmap_free(cache->map);
+	git_mutex_free(&cache->lock);
+}
+
 /* Call with lock, yo */
 static void cache_evict_entries(git_cache *cache, size_t evict_count)
 {

src/cache.h

@@ -21,17 +21,17 @@ enum {
 };
 
 typedef struct {
-	git_oid oid;
-	int16_t type;
-	uint16_t flags;
-	size_t size;
+	git_oid    oid;
+	int16_t    type;  /* git_otype value */
+	uint16_t   flags; /* GIT_CACHE_STORE value */
+	size_t     size;
 	git_atomic refcount;
 } git_cached_obj;
 
 typedef struct {
 	git_oidmap *map;
-	git_mutex lock;
-	size_t used_memory;
+	git_mutex   lock;
+	size_t      used_memory;
 } git_cache;
 
 extern bool git_cache__enabled;

src/commit.c

@@ -31,8 +31,10 @@ static void clear_parents(git_commit *commit)
 	git_vector_clear(&commit->parent_ids);
 }
 
-void git_commit__free(git_commit *commit)
+void git_commit__free(void *_commit)
 {
+	git_commit *commit = _commit;
+
 	clear_parents(commit);
 	git_vector_free(&commit->parent_ids);
 
@@ -166,10 +168,9 @@ int git_commit_create(
 	return retval;
 }
 
-int git_commit__parse_buffer(git_commit *commit, const void *data, size_t len)
+int git_commit__parse(void *_commit, const char *buffer, const char *buffer_end)
 {
-	const char *buffer = data;
-	const char *buffer_end = (const char *)data + len;
+	git_commit *commit = _commit;
 	git_oid parent_id;
 
 	if (git_vector_init(&commit->parent_ids, 4, NULL) < 0)
@@ -241,13 +242,6 @@ bad_buffer:
 	return -1;
 }
 
-int git_commit__parse(git_commit *commit, git_odb_object *obj)
-{
-	assert(commit);
-	return git_commit__parse_buffer(
-		commit, git_odb_object_data(obj), git_odb_object_size(obj));
-}
-
 #define GIT_COMMIT_GETTER(_rvalue, _name, _return) \
 	_rvalue git_commit_##_name(const git_commit *commit) \
 	{\

src/commit.h

@@ -27,8 +27,7 @@ struct git_commit {
 	char *message;
 };
 
-void git_commit__free(git_commit *c);
-int git_commit__parse(git_commit *commit, git_odb_object *obj);
+void git_commit__free(void *commit);
+int git_commit__parse(void *commit, const char *buf, const char *bufend);
 
-int git_commit__parse_buffer(git_commit *commit, const void *data, size_t len);
 #endif

src/object.c

@@ -18,63 +18,39 @@
 
 static const int OBJECT_BASE_SIZE = 4096;
 
-static struct {
+typedef struct {
 	const char	*str;	/* type name string */
-	int			loose; /* valid loose object type flag */
 	size_t		size;	/* size in bytes of the object structure */
-} git_objects_table[] = {
+
+	int  (*from_odb)(void *self, git_odb_object *obj);
+	int  (*parse)(void *self, const char *buf, const char *buf_end);
+	void (*free)(void *self);
+} git_object_def;
+
+static git_object_def git_objects_table[] = {
 	/* 0 = GIT_OBJ__EXT1 */
-	{ "", 0, 0},
+	{ "", 0, NULL, NULL, NULL },
 
 	/* 1 = GIT_OBJ_COMMIT */
-	{ "commit", 1, sizeof(struct git_commit)},
+	{ "commit", sizeof(git_commit), NULL, git_commit__parse, git_commit__free },
 
 	/* 2 = GIT_OBJ_TREE */
-	{ "tree", 1, sizeof(struct git_tree) },
+	{ "tree", sizeof(git_tree), NULL, git_tree__parse, git_tree__free },
 
 	/* 3 = GIT_OBJ_BLOB */
-	{ "blob", 1, sizeof(struct git_blob) },
+	{ "blob", sizeof(git_blob), git_blob__from_odb_object, NULL, git_blob__free },
 
 	/* 4 = GIT_OBJ_TAG */
-	{ "tag", 1, sizeof(struct git_tag) },
+	{ "tag", sizeof(git_tag), NULL, git_tag__parse, git_tag__free },
 
 	/* 5 = GIT_OBJ__EXT2 */
-	{ "", 0, 0 },
-
+	{ "", 0, NULL, NULL, NULL },
 	/* 6 = GIT_OBJ_OFS_DELTA */
-	{ "OFS_DELTA", 0, 0 },
-
+	{ "OFS_DELTA", 0, NULL, NULL, NULL },
 	/* 7 = GIT_OBJ_REF_DELTA */
-	{ "REF_DELTA", 0, 0	}
+	{ "REF_DELTA", 0, NULL, NULL, NULL },
 };
 
-static int create_object(git_object **object_out, git_otype type)
-{
-	git_object *object = NULL;
-
-	assert(object_out);
-
-	*object_out = NULL;
-
-	switch (type) {
-	case GIT_OBJ_COMMIT:
-	case GIT_OBJ_TAG:
-	case GIT_OBJ_BLOB:
-	case GIT_OBJ_TREE:
-		object = git__malloc(git_object__size(type));
-		GITERR_CHECK_ALLOC(object);
-		memset(object, 0x0, git_object__size(type));
-		break;
-
-	default:
-		giterr_set(GITERR_INVALID, "The given type is invalid");
-		return -1;
-	}
-
-	*object_out = object;
-	return 0;
-}
-
 int git_object__from_odb_object(
 	git_object **object_out,
 	git_repository *repo,
@@ -82,46 +58,47 @@ int git_object__from_odb_object(
 	git_otype type)
 {
 	int error;
+	size_t object_size;
+	git_object_def *def;
 	git_object *object = NULL;
 
+	assert(object_out);
+	*object_out = NULL;
+
+	/* Validate type match */
 	if (type != GIT_OBJ_ANY && type != odb_obj->cached.type) {
 		giterr_set(GITERR_INVALID,
 			"The requested type does not match the type in the ODB");
 		return GIT_ENOTFOUND;
 	}
 
-	if ((error = create_object(&object, odb_obj->cached.type)) < 0)
-		return error;
+	if ((object_size = git_object__size(odb_obj->cached.type)) == 0) {
+		giterr_set(GITERR_INVALID, "The requested type is invalid");
+		return GIT_ENOTFOUND;
+	}
+
+	/* Allocate and initialize base object */
+	object = git__calloc(1, object_size);
+	GITERR_CHECK_ALLOC(object);
 
-	/* Initialize parent object */
 	git_oid_cpy(&object->cached.oid, &odb_obj->cached.oid);
-	object->cached.size = odb_obj->cached.size;
 	object->cached.type = odb_obj->cached.type;
+	object->cached.size = odb_obj->cached.size;
 	object->repo = repo;
 
-	switch (object->cached.type) {
-	case GIT_OBJ_COMMIT:
-		error = git_commit__parse((git_commit *)object, odb_obj);
-		break;
-
-	case GIT_OBJ_TREE:
-		error = git_tree__parse((git_tree *)object, odb_obj);
-		break;
-
-	case GIT_OBJ_TAG:
-		error = git_tag__parse((git_tag *)object, odb_obj);
-		break;
+	/* Parse raw object data */
+	def = &git_objects_table[odb_obj->cached.type];
+	assert(def->free && (def->from_odb || def->parse));
 
-	case GIT_OBJ_BLOB:
-		error = git_blob__parse((git_blob *)object, odb_obj);
-		break;
-
-	default:
-		break;
+	if (def->from_odb) {
+		error = def->from_odb(object, odb_obj);
+	} else {
+		const char *data = (const char *)git_odb_object_data(odb_obj);
+		error = def->parse(object, data, data + git_odb_object_size(odb_obj));
 	}
 
 	if (error < 0) {
-		git_object__free(object);
+		def->free(object);
 		return error;
 	}
 
@@ -129,6 +106,17 @@ int git_object__from_odb_object(
 	return 0;
 }
 
+void git_object__free(void *obj)
+{
+	git_otype type = ((git_object *)obj)->cached.type;
+
+	if (type < 0 || ((size_t)type) >= ARRAY_SIZE(git_objects_table) ||
+		!git_objects_table[type].free)
+		git__free(obj);
+	else
+		git_objects_table[type].free(obj);
+}
+
 int git_object_lookup_prefix(
 	git_object **object_out,
 	git_repository *repo,
@@ -222,35 +210,6 @@ int git_object_lookup(git_object **object_out, git_repository *repo, const git_o
 	return git_object_lookup_prefix(object_out, repo, id, GIT_OID_HEXSZ, type);
 }
 
-void git_object__free(void *_obj)
-{
-	git_object *object = (git_object *)_obj;
-
-	assert(object);
-
-	switch (object->cached.type) {
-	case GIT_OBJ_COMMIT:
-		git_commit__free((git_commit *)object);
-		break;
-
-	case GIT_OBJ_TREE:
-		git_tree__free((git_tree *)object);
-		break;
-
-	case GIT_OBJ_TAG:
-		git_tag__free((git_tag *)object);
-		break;
-
-	case GIT_OBJ_BLOB:
-		git_blob__free((git_blob *)object);
-		break;
-
-	default:
-		git__free(object);
-		break;
-	}
-}
-
 void git_object_free(git_object *object)
 {
 	if (object == NULL)
@@ -304,7 +263,7 @@ int git_object_typeisloose(git_otype type)
 	if (type < 0 || ((size_t) type) >= ARRAY_SIZE(git_objects_table))
 		return 0;
 
-	return git_objects_table[type].loose;
+	return (git_objects_table[type].size > 0) ? 1 : 0;
 }
 
 size_t git_object__size(git_otype type)

src/odb.c

@@ -82,23 +82,24 @@ int git_odb__hashobj(git_oid *id, git_rawobj *obj)
 }
 
 
-static git_odb_object *new_odb_object(const git_oid *oid, git_rawobj *source)
+static git_odb_object *odb_object__alloc(const git_oid *oid, git_rawobj *source)
 {
-	git_odb_object *object = git__malloc(sizeof(git_odb_object));
-	memset(object, 0x0, sizeof(git_odb_object));
+	git_odb_object *object = git__calloc(1, sizeof(git_odb_object));
 
-	git_oid_cpy(&object->cached.oid, oid);
-	object->cached.size = source->len;
-	object->cached.type = source->type;
-	object->buffer = source->data;
+	if (object != NULL) {
+		git_oid_cpy(&object->cached.oid, oid);
+		object->cached.type = source->type;
+		object->cached.size = source->len;
+		object->buffer      = source->data;
+	}
 
 	return object;
 }
 
-void git_odb_object__free(git_odb_object *object)
+void git_odb_object__free(void *object)
 {
 	if (object != NULL) {
-		git__free(object->buffer);
+		git__free(((git_odb_object *)object)->buffer);
 		git__free(object);
 	}
 }
@@ -679,6 +680,7 @@ int git_odb_read(git_odb_object **out, git_odb *db, const git_oid *id)
 	int error;
 	bool refreshed = false;
 	git_rawobj raw;
+	git_odb_object *object;
 
 	assert(out && db && id);
 
@@ -713,7 +715,10 @@ attempt_lookup:
 	if (error && error != GIT_PASSTHROUGH)
 		return error;
 
-	*out = git_cache_store_raw(odb_cache(db), new_odb_object(id, &raw));
+	if ((object = odb_object__alloc(id, &raw)) == NULL)
+		return -1;
+
+	*out = git_cache_store_raw(odb_cache(db), object);
 	return 0;
 }
 
@@ -726,6 +731,7 @@ int git_odb_read_prefix(
 	git_rawobj raw;
 	void *data = NULL;
 	bool found = false, refreshed = false;
+	git_odb_object *object;
 
 	assert(out && db);
 
@@ -777,7 +783,10 @@ attempt_lookup:
 	if (!found)
 		return git_odb__error_notfound("no match for prefix", short_id);
 
-	*out = git_cache_store_raw(odb_cache(db), new_odb_object(&found_full_oid, &raw));
+	if ((object = odb_object__alloc(&found_full_oid, &raw)) == NULL)
+		return -1;
+
+	*out = git_cache_store_raw(odb_cache(db), object);
 	return 0;
 }
 

src/odb.h

@@ -39,8 +39,6 @@ struct git_odb {
 	git_cache own_cache;
 };
 
-void git_odb_object__free(git_odb_object *object);
-
 /*
  * Hash a git_rawobj internally.
  * The `git_rawobj` is supposed to be previously initialized
@@ -98,4 +96,7 @@ int git_odb__read_header_or_object(
 	git_odb_object **out, size_t *len_p, git_otype *type_p,
 	git_odb *db, const git_oid *id);
 
+/* fully free the object; internal method, DO NOT EXPORT */
+void git_odb_object__free(void *object);
+
 #endif

src/oidmap.h

@@ -19,7 +19,7 @@
 __KHASH_TYPE(oid, const git_oid *, void *);
 typedef khash_t(oid) git_oidmap;
 
-GIT_INLINE(khint_t) hash_git_oid(const git_oid *oid)
+GIT_INLINE(khint_t) git_oidmap_hash(const git_oid *oid)
 {
 	khint_t h;
 	memcpy(&h, oid, sizeof(khint_t));
@@ -27,7 +27,7 @@ GIT_INLINE(khint_t) hash_git_oid(const git_oid *oid)
 }
 
 #define GIT__USE_OIDMAP \
-	__KHASH_IMPL(oid, static kh_inline, const git_oid *, void *, 1, hash_git_oid, git_oid_equal)
+	__KHASH_IMPL(oid, static kh_inline, const git_oid *, void *, 1, git_oidmap_hash, git_oid_equal)
 
 #define git_oidmap_alloc() kh_init(oid)
 #define git_oidmap_free(h) kh_destroy(oid,h), h = NULL

src/tag.c

@@ -15,8 +15,9 @@
 #include "git2/signature.h"
 #include "git2/odb_backend.h"
 
-void git_tag__free(git_tag *tag)
+void git_tag__free(void *_tag)
 {
+	git_tag *tag = _tag;
 	git_signature_free(tag->tagger);
 	git__free(tag->message);
 	git__free(tag->tag_name);
@@ -69,18 +70,17 @@ static int tag_error(const char *str)
 	return -1;
 }
 
-int git_tag__parse_buffer(git_tag *tag, const char *buffer, size_t length)
+int git_tag__parse(void *_tag, const char *buffer, const char *buffer_end)
 {
 	static const char *tag_types[] = {
 		NULL, "commit\n", "tree\n", "blob\n", "tag\n"
 	};
 
+	git_tag *tag = _tag;
 	unsigned int i;
 	size_t text_len;
 	char *search;
 
-	const char *buffer_end = buffer + length;
-
 	if (git_oid__parse(&tag->target, &buffer, buffer_end, "object ") < 0)
 		return tag_error("Object field invalid");
 
@@ -317,7 +317,7 @@ int git_tag_create_frombuffer(git_oid *oid, git_repository *repo, const char *bu
 		return -1;
 
 	/* validate the buffer */
-	if (git_tag__parse_buffer(&tag, buffer, strlen(buffer)) < 0)
+	if (git_tag__parse(&tag, buffer, buffer + strlen(buffer)) < 0)
 		return -1;
 
 	/* validate the target */
@@ -390,15 +390,8 @@ int git_tag_delete(git_repository *repo, const char *tag_name)
 
 	if ((error = git_reference_delete(tag_ref)) == 0)
 		git_reference_free(tag_ref);
-	
-	return error;
-}
 
-int git_tag__parse(git_tag *tag, git_odb_object *obj)
-{
-	assert(tag);
-	return git_tag__parse_buffer(
-		tag, git_odb_object_data(obj), git_odb_object_size(obj));
+	return error;
 }
 
 typedef struct {

src/tag.h

@@ -22,8 +22,7 @@ struct git_tag {
 	char *message;
 };
 
-void git_tag__free(git_tag *tag);
-int git_tag__parse(git_tag *tag, git_odb_object *obj);
-int git_tag__parse_buffer(git_tag *tag, const char *data, size_t len);
+void git_tag__free(void *tag);
+int git_tag__parse(void *tag, const char *buf, const char *buf_end);
 
 #endif

src/tree.c

@@ -219,15 +219,16 @@ git_tree_entry *git_tree_entry_dup(const git_tree_entry *entry)
 	return copy;
 }
 
-void git_tree__free(git_tree *tree)
+void git_tree__free(void *tree)
 {
+	git_vector *entries = &((git_tree *)tree)->entries;
 	size_t i;
 	git_tree_entry *e;
 
-	git_vector_foreach(&tree->entries, i, e)
+	git_vector_foreach(entries, i, e)
 		git_tree_entry_free(e);
 
-	git_vector_free(&tree->entries);
+	git_vector_free(entries);
 	git__free(tree);
 }
 
@@ -371,10 +372,11 @@ static int tree_error(const char *str, const char *path)
 	return -1;
 }
 
-static int tree_parse_buffer(
-	git_tree *tree, const char *buffer, const char *buffer_end)
+int git_tree__parse(void *tree, const char *buffer, const char *buffer_end)
 {
-	if (git_vector_init(&tree->entries, DEFAULT_TREE_SIZE, entry_sort_cmp) < 0)
+	git_vector *tree_entries = &((git_tree *)tree)->entries;
+
+	if (git_vector_init(tree_entries, DEFAULT_TREE_SIZE, entry_sort_cmp) < 0)
 		return -1;
 
 	while (buffer < buffer_end) {
@@ -397,7 +399,7 @@ static int tree_parse_buffer(
 			entry = alloc_entry(buffer);
 			GITERR_CHECK_ALLOC(entry);
 
-			if (git_vector_insert(&tree->entries, entry) < 0) {
+			if (git_vector_insert(tree_entries, entry) < 0) {
 				git__free(entry);
 				return -1;
 			}
@@ -417,17 +419,6 @@ static int tree_parse_buffer(
 	return 0;
 }
 
-int git_tree__parse(git_tree *tree, git_odb_object *obj)
-{
-	const char *buf;
-
-	assert(tree && obj);
-
-	buf = (const char *)git_odb_object_data(obj);
-
-	return tree_parse_buffer(tree, buf, buf + git_odb_object_size(obj));
-}
-
 static size_t find_next_dir(const char *dirname, git_index *index, size_t start)
 {
 	size_t dirlen, i, entries = git_index_entrycount(index);

src/tree.h

@@ -37,8 +37,8 @@ GIT_INLINE(bool) git_tree_entry__is_tree(const struct git_tree_entry *e)
 
 extern int git_tree_entry_icmp(const git_tree_entry *e1, const git_tree_entry *e2);
 
-void git_tree__free(git_tree *tree);
-int git_tree__parse(git_tree *tree, git_odb_object *obj);
+void git_tree__free(void *tree);
+int git_tree__parse(void *tree, const char *buf, const char *buf_end);
 
 /**
  * Lookup the first position in the tree with a given prefix.

tests-clar/commit/parse.c

@@ -269,6 +269,7 @@ void test_commit_parse__entire_commit(void)
 	const int broken_commit_count = sizeof(failing_commit_cases) / sizeof(*failing_commit_cases);
 	const int working_commit_count = sizeof(passing_commit_cases) / sizeof(*passing_commit_cases);
 	int i;
+	const char *buf;
 
 	for (i = 0; i < broken_commit_count; ++i) {
 		git_commit *commit;
@@ -276,9 +277,8 @@ void test_commit_parse__entire_commit(void)
 		memset(commit, 0x0, sizeof(git_commit));
 		commit->object.repo = g_repo;
 
-		cl_git_fail(git_commit__parse_buffer(
-			commit, failing_commit_cases[i], strlen(failing_commit_cases[i]))
-		);
+		buf = failing_commit_cases[i];
+		cl_git_fail(git_commit__parse(commit, buf, buf + strlen(buf)));
 
 		git_commit__free(commit);
 	}
@@ -290,11 +290,8 @@ void test_commit_parse__entire_commit(void)
 		memset(commit, 0x0, sizeof(git_commit));
 		commit->object.repo = g_repo;
 
-		cl_git_pass(git_commit__parse_buffer(
-                     commit,
-                     passing_commit_cases[i],
-                     strlen(passing_commit_cases[i]))
-         );
+		buf = passing_commit_cases[i];
+		cl_git_pass(git_commit__parse(commit, buf, buf + strlen(buf)));
 
 		if (!i)
 			cl_assert_equal_s("", git_commit_message(commit));