From fe24d396e1b8d2eedbc07f626af3dcd2b14e6012 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 13 Jul 2014 02:41:55 -0400 Subject: move setting of object->type to alloc_* functions The "struct object" type implements basic object polymorphism. Individual instances are allocated as concrete types (or as a union type that can store any object), and a "struct object *" can be cast into its real type after examining its "type" enum. This means it is dangerous to have a type field that does not match the allocation (e.g., setting the type field of a "struct blob" to "OBJ_COMMIT" would mean that a reader might read past the allocated memory). In most of the current code this is not a problem; the first thing we do after allocating an object is usually to set its type field by passing it to create_object. However, the virtual commits we create in merge-recursive.c do not ever get their type set. This does not seem to have caused problems in practice, though (presumably because we always pass around a "struct commit" pointer and never even look at the type). We can fix this oversight and also make it harder for future code to get it wrong by setting the type directly in the object allocation functions. This will also make it easier to fix problems with commit index allocation, as we know that any object allocated by alloc_commit_node will meet the invariant that an object with an OBJ_COMMIT type field will have a unique index number. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'commit.c') diff --git a/commit.c b/commit.c index 4ff8077dbf..eb24add866 100644 --- a/commit.c +++ b/commit.c @@ -61,10 +61,8 @@ struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_n struct commit *lookup_commit(const unsigned char *sha1) { struct object *obj = lookup_object(sha1); - if (!obj) { - struct commit *c = alloc_commit_node(); - return create_object(sha1, OBJ_COMMIT, c); - } + if (!obj) + return create_object(sha1, alloc_commit_node()); if (!obj->type) obj->type = OBJ_COMMIT; return check_commit(obj, sha1, 0); -- cgit v1.2.3 From c4ad00f8ccb59a0ae0735e8e32b203d4bd835616 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 13 Jul 2014 02:42:03 -0400 Subject: add object_as_type helper for casting objects When we call lookup_commit, lookup_tree, etc, the logic goes something like: 1. Look for an existing object struct. If we don't have one, allocate and return a new one. 2. Double check that any object we have is the expected type (and complain and return NULL otherwise). 3. Convert an object with type OBJ_NONE (from a prior call to lookup_unknown_object) to the expected type. We can encapsulate steps 2 and 3 in a helper function which checks whether we have the expected object type, converts OBJ_NONE as appropriate, and returns the object. Not only does this shorten the code, but it also provides one central location for converting OBJ_NONE objects into objects of other types. Future patches will use that to enforce type-specific invariants. Since this is a refactoring, we would want it to behave exactly as the current code. It takes a little reasoning to see that this is the case: - for lookup_{commit,tree,etc} functions, we are just pulling steps 2 and 3 into a function that does the same thing. - for the call in peel_object, we currently only do step 3 (but we want to consolidate it with the others, as mentioned above). However, step 2 is a noop here, as the surrounding conditional makes sure we have OBJ_NONE (which we want to keep to avoid an extraneous call to sha1_object_info). - for the call in lookup_commit_reference_gently, we are currently doing step 2 but not step 3. However, step 3 is a noop here. The object we got will have just come from deref_tag, which must have figured out the type for each object in order to know when to stop peeling. Therefore the type will never be OBJ_NONE. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- blob.c | 9 +-------- commit.c | 19 ++----------------- object.c | 17 +++++++++++++++++ object.h | 2 ++ refs.c | 3 +-- tag.c | 9 +-------- tree.c | 9 +-------- 7 files changed, 25 insertions(+), 43 deletions(-) (limited to 'commit.c') diff --git a/blob.c b/blob.c index 5720a38ca2..1fcb8e44b0 100644 --- a/blob.c +++ b/blob.c @@ -8,14 +8,7 @@ struct blob *lookup_blob(const unsigned char *sha1) struct object *obj = lookup_object(sha1); if (!obj) return create_object(sha1, alloc_blob_node()); - if (!obj->type) - obj->type = OBJ_BLOB; - if (obj->type != OBJ_BLOB) { - error("Object %s is a %s, not a blob", - sha1_to_hex(sha1), typename(obj->type)); - return NULL; - } - return (struct blob *) obj; + return object_as_type(obj, OBJ_BLOB, 0); } int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size) diff --git a/commit.c b/commit.c index eb24add866..65179f96ee 100644 --- a/commit.c +++ b/commit.c @@ -18,19 +18,6 @@ int save_commit_buffer = 1; const char *commit_type = "commit"; -static struct commit *check_commit(struct object *obj, - const unsigned char *sha1, - int quiet) -{ - if (obj->type != OBJ_COMMIT) { - if (!quiet) - error("Object %s is a %s, not a commit", - sha1_to_hex(sha1), typename(obj->type)); - return NULL; - } - return (struct commit *) obj; -} - struct commit *lookup_commit_reference_gently(const unsigned char *sha1, int quiet) { @@ -38,7 +25,7 @@ struct commit *lookup_commit_reference_gently(const unsigned char *sha1, if (!obj) return NULL; - return check_commit(obj, sha1, quiet); + return object_as_type(obj, OBJ_COMMIT, quiet); } struct commit *lookup_commit_reference(const unsigned char *sha1) @@ -63,9 +50,7 @@ struct commit *lookup_commit(const unsigned char *sha1) struct object *obj = lookup_object(sha1); if (!obj) return create_object(sha1, alloc_commit_node()); - if (!obj->type) - obj->type = OBJ_COMMIT; - return check_commit(obj, sha1, 0); + return object_as_type(obj, OBJ_COMMIT, 0); } struct commit *lookup_commit_reference_by_name(const char *name) diff --git a/object.c b/object.c index 472aa8d5be..b2319f6246 100644 --- a/object.c +++ b/object.c @@ -158,6 +158,23 @@ void *create_object(const unsigned char *sha1, void *o) return obj; } +void *object_as_type(struct object *obj, enum object_type type, int quiet) +{ + if (obj->type == type) + return obj; + else if (obj->type == OBJ_NONE) { + obj->type = type; + return obj; + } + else { + if (!quiet) + error("object %s is a %s, not a %s", + sha1_to_hex(obj->sha1), + typename(obj->type), typename(type)); + return NULL; + } +} + struct object *lookup_unknown_object(const unsigned char *sha1) { struct object *obj = lookup_object(sha1); diff --git a/object.h b/object.h index 8020ace566..5e8d8ee548 100644 --- a/object.h +++ b/object.h @@ -81,6 +81,8 @@ struct object *lookup_object(const unsigned char *sha1); extern void *create_object(const unsigned char *sha1, void *obj); +void *object_as_type(struct object *obj, enum object_type type, int quiet); + /* * Returns the object, having parsed it to find out what it is. * diff --git a/refs.c b/refs.c index 59fb70087a..f0bd7ac0e9 100644 --- a/refs.c +++ b/refs.c @@ -1520,9 +1520,8 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh if (o->type == OBJ_NONE) { int type = sha1_object_info(name, NULL); - if (type < 0) + if (type < 0 || !object_as_type(o, type, 0)) return PEEL_INVALID; - o->type = type; } if (o->type != OBJ_TAG) diff --git a/tag.c b/tag.c index 79552c716c..82d841bf2d 100644 --- a/tag.c +++ b/tag.c @@ -41,14 +41,7 @@ struct tag *lookup_tag(const unsigned char *sha1) struct object *obj = lookup_object(sha1); if (!obj) return create_object(sha1, alloc_tag_node()); - if (!obj->type) - obj->type = OBJ_TAG; - if (obj->type != OBJ_TAG) { - error("Object %s is a %s, not a tag", - sha1_to_hex(sha1), typename(obj->type)); - return NULL; - } - return (struct tag *) obj; + return object_as_type(obj, OBJ_TAG, 0); } static unsigned long parse_tag_date(const char *buf, const char *tail) diff --git a/tree.c b/tree.c index ed66575079..bb02c1caa4 100644 --- a/tree.c +++ b/tree.c @@ -184,14 +184,7 @@ struct tree *lookup_tree(const unsigned char *sha1) struct object *obj = lookup_object(sha1); if (!obj) return create_object(sha1, alloc_tree_node()); - if (!obj->type) - obj->type = OBJ_TREE; - if (obj->type != OBJ_TREE) { - error("Object %s is a %s, not a tree", - sha1_to_hex(sha1), typename(obj->type)); - return NULL; - } - return (struct tree *) obj; + return object_as_type(obj, OBJ_TREE, 0); } int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size) -- cgit v1.2.3