{"thread":{"id":"37105","subject":"[PATCH v3 2/2] alloc.c: remove the redundant commit_count variable","startedAt":"2014-07-10T23:59:48Z","lastAt":"2014-07-15T20:12:48Z","messageCount":35,"participants":["Ramsay Jones","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"245743","messageId":"53BF28F4.8050704@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":null,"subject":"[PATCH v3 2/2] alloc.c: remove the redundant commit_count variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-10T23:59:48Z","receivedAt":"2014-07-10T23:59:48Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe 'commit_count' static variable is used in alloc_commit_node()\nto set the 'index' field of the commit structure to a unique value.\nThis variable assumes the same value as the 'count' field of the\n'commit_state' allocator state structure, which may be used in its\nplace.\n\nSigned-off-by: Ramsay Jones \n---\n alloc.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex d7c3605..c6687f9 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -64,9 +64,8 @@ static struct alloc_state commit_state;\n \n void *alloc_commit_node(void)\n {\n-\tstatic int commit_count;\n \tstruct commit *c = alloc_node(&commit_state, sizeof(struct commit));\n-\tc->index = commit_count++;\n+\tc->index = commit_state.count - 1;\n \treturn c;\n }\n \n-- \n2.0.0\n"},{"id":"245745","messageId":"20140711003053.GB11360@sigill.intra.peff.net","threadId":"37105","inReplyTo":"53BF28F4.8050704@ramsay1.demon.co.uk","subject":"Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T00:30:53Z","receivedAt":"2014-07-11T00:30:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 12:59:48AM +0100, Ramsay Jones wrote:\n\n> The 'commit_count' static variable is used in alloc_commit_node()\n> to set the 'index' field of the commit structure to a unique value.\n> This variable assumes the same value as the 'count' field of the\n> 'commit_state' allocator state structure, which may be used in its\n> place.\n\nI don't think we want to do this, because there is a bug in the current\ncode that I have not reported yet. :)\n\nThe code you're touching here was trying to make sure that each commit\ngets a unique index, under the assumption that commits only get\nallocated via alloc_commit_node. But I think that assumption is wrong.\nWe can also get commit objects by allocating an OBJ_NONE (e.g., via\nlookup_unknown_object) and then converting it into an OBJ_COMMIT when we\nfind out what it is.\n\nWe should be allocating a commit index to it at that point (but we don't\ncurrently), at which point the commit_count and commit_state.count\nindices will no longer be in sync.\n\nThis only happens in a few places. Most calls will probably be via\nlookup_commit (which gets called when we parse_object such an unknown\nobject), but there are others (e.g., peel_object may fill in the type).\nSo we could fix it with something like:\n\ndiff --git a/commit.c b/commit.c\nindex fb7897c..f4f4278 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -65,8 +65,11 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \t\tstruct commit *c = alloc_commit_node();\n \t\treturn create_object(sha1, OBJ_COMMIT, c);\n \t}\n-\tif (!obj->type)\n+\tif (!obj->type) {\n+\t\tstruct commit *c = (struct commit *)obj;\n \t\tobj->type = OBJ_COMMIT;\n+\t\tc->index = alloc_commit_index();\n+\t}\n \treturn check_commit(obj, sha1, 0);\n }\n \n\nwhere alloc_commit_index() would be a thin wrapper around \"return\ncommit_count++\". And then find and update any other callsites that need\nit.\n\nThe downside is that it's hard to find those callsites. A safer\nalternative would be to speculatively allocate a commit index for all\nunknown objects. Then if any other code switches the type to commit,\nthey already have an index. But it also means we'll have holes in our\ncommit index space, which makes commit-slabs less efficient.\n\nWe do not call lookup_unknown_object all that often, though, so it might\nbe an OK tradeoff (and in at least one case, in diff_tree_stdin, I think\nthe code can be converted not to use lookup_unknown_object in the first\nplace). So worrying about the performance may not be worth it.\n\n-Peff\n"},{"id":"245751","messageId":"53BF3709.6000307@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":"20140711003053.GB11360@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-11T00:59:53Z","receivedAt":"2014-07-11T00:59:53Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 11/07/14 01:30, Jeff King wrote:\n> On Fri, Jul 11, 2014 at 12:59:48AM +0100, Ramsay Jones wrote:\n> \n>> The 'commit_count' static variable is used in alloc_commit_node()\n>> to set the 'index' field of the commit structure to a unique value.\n>> This variable assumes the same value as the 'count' field of the\n>> 'commit_state' allocator state structure, which may be used in its\n>> place.\n> \n> I don't think we want to do this, because there is a bug in the current\n> code that I have not reported yet. :)\n\n:P OK, I will simply drop this one then.\n\n> \n> The code you're touching here was trying to make sure that each commit\n> gets a unique index, under the assumption that commits only get\n> allocated via alloc_commit_node. But I think that assumption is wrong.\n> We can also get commit objects by allocating an OBJ_NONE (e.g., via\n> lookup_unknown_object) and then converting it into an OBJ_COMMIT when we\n> find out what it is.\n\nHmm, I don't know how the object is converted, but the object allocator\nis actually allocating an 'union any_object', so it's allocating more\nspace than for a struct object anyway. If you add an 'index' field to\nstruct object, (and remove it from struct commit) it could be set in\nalloc_object_node(). ie _all_ node types get an index field.\n\nHmm, that was just off the top of my head, so take with a pinch of salt.\n\nATB,\nRamsay Jones\n"},{"id":"245758","messageId":"20140711083220.GA5407@sigill.intra.peff.net","threadId":"37105","inReplyTo":"53BF3709.6000307@ramsay1.demon.co.uk","subject":"Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:32:20Z","receivedAt":"2014-07-11T08:32:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 01:59:53AM +0100, Ramsay Jones wrote:\n\n> > The code you're touching here was trying to make sure that each commit\n> > gets a unique index, under the assumption that commits only get\n> > allocated via alloc_commit_node. But I think that assumption is wrong.\n> > We can also get commit objects by allocating an OBJ_NONE (e.g., via\n> > lookup_unknown_object) and then converting it into an OBJ_COMMIT when we\n> > find out what it is.\n> \n> Hmm, I don't know how the object is converted, but the object allocator\n> is actually allocating an 'union any_object', so it's allocating more\n> space than for a struct object anyway.\n\nRight, we would generally want to avoid lookup_unknown_object where we\ncan for that reason.\n\n> If you add an 'index' field to struct object, (and remove it from\n> struct commit) it could be set in alloc_object_node(). ie _all_ node\n> types get an index field.\n\nThat was something I considered when we did the original commit-slab\nwork, as it would let you do similar tricks for any set of objects, not\njust commits. The reasons against it are:\n\n 1. It would bloat the size of blob and tree structs by at least 4\n bytes (probably 8 for alignment). In most repos, commits make up\n only 10-20% of the total objects (so for linux.git, we're talking\n about 25MB extra in the working set).\n\n 2. It makes single types sparse in the index space. In cases where you\n do just want to keep data on commits (and that is the main use),\n you end up allocating a slab entry per object, rather than per\n commit. That wastes memory (much worse than 25MB if your slab items\n are large), and reduces cache locality.\n\nYou could probably get around (2) by splitting the index space by type\nand allocating them in pools, but that complicates things considerably,\nas you have to guess ahead of time at reasonable maximums for each type.\n\n-Peff\n"},{"id":"245759","messageId":"20140711084141.GA5521@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140711003053.GB11360@sigill.intra.peff.net","subject":"[PATCH 0/7] ensure index is set for all OBJ_COMMIT objects variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:41:41Z","receivedAt":"2014-07-11T08:41:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a series to address the bug I mentioned earlier by catching the\nconversion of OBJ_NONE to OBJ_COMMIT in a central location and setting\nthe index there.\n\nI've included your patch 1/2 unchanged in the beginning, as I build on\ntop of it (and your patch 2/2 is no longer applicable). The rest is\nrefactoring leading up to patch 6 to fix the bug. Patch 7 is a bonus\ncleanup.\n\nI'd hoped to cap off the series by converting the \"type\" field of\n\"struct commit\" to a \"const unsigned type : 3\", which would avoid any\nnew callers being added that would touch it without going through the\nproper procedure. However, it's a bitfield, which makes it hard to cast\nthe constness away in the actual setter function. My best attempt was to\nuse a union with matching const and non-const members, but that would\nmean changing all of the sites which read the field (and there are many)\nto use \"object->type.read\".\n\nThere may be a clever solution hiding in a dark corner of C, but I\nsuspect we are entering a realm of portability problems with older\ncompilers (I even saw one compiler's documentation claim that \"const\"\nwas forbidden on bitfields, even though C99 has an example which does\nit).\n\n [1/7]: alloc.c: remove the alloc_raw_commit_node() function\n [2/7]: move setting of object->type to alloc_* functions\n [3/7]: parse_object_buffer: do not set object type\n [4/7]: add object_as_type helper for casting objects\n [5/7]: alloc: factor out commit index\n [6/7]: object_as_type: set commit index\n [7/7]: diff-tree: avoid lookup_unknown_object\n\n-Peff\n"},{"id":"245760","messageId":"20140711084219.GA5625@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140711084141.GA5521@sigill.intra.peff.net","subject":"[PATCH 1/7] alloc.c: remove the alloc_raw_commit_node() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:42:19Z","receivedAt":"2014-07-11T08:42:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Ramsay Jones \n\nIn order to encapsulate the setting of the unique commit index, commit\n969eba63 (\"commit: push commit_index update into alloc_commit_node\",\n10-06-2014) introduced a (logically private) intermediary allocator\nfunction. However, this function (alloc_raw_commit_node()) was declared\nas a public function, which undermines its entire purpose.\n\nIntroduce an inline function, alloc_node(), which implements the main\nlogic of the allocator used by DEFINE_ALLOCATOR, and redefine the macro\nin terms of the new function. In addition, use the new function in the\nimplementation of the alloc_commit_node() allocator, rather than the\nintermediary allocator, which can now be removed.\n\nNoticed by sparse (\"symbol 'alloc_raw_commit_node' was not declared.\nShould it be static?\").\n\nSigned-off-by: Ramsay Jones \nSigned-off-by: Jeff King \n---\n alloc.c | 47 +++++++++++++++++++++++++++++------------------\n 1 file changed, 29 insertions(+), 18 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex eb22a45..d7c3605 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -19,22 +19,10 @@\n #define BLOCKING 1024\n \n #define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n-static unsigned int name##_allocs;\t\t\t\t\\\n+static struct alloc_state name##_state;\t\t\t\t\\\n void *alloc_##name##_node(void)\t\t\t\t\t\\\n {\t\t\t\t\t\t\t\t\\\n-\tstatic int nr;\t\t\t\t\t\t\\\n-\tstatic type *block;\t\t\t\t\t\\\n-\tvoid *ret;\t\t\t\t\t\t\\\n-\t\t\t\t\t\t\t\t\\\n-\tif (!nr) {\t\t\t\t\t\t\\\n-\t\tnr = BLOCKING;\t\t\t\t\t\\\n-\t\tblock = xmalloc(BLOCKING * sizeof(type));\t\\\n-\t}\t\t\t\t\t\t\t\\\n-\tnr--;\t\t\t\t\t\t\t\\\n-\tname##_allocs++;\t\t\t\t\t\\\n-\tret = block++;\t\t\t\t\t\t\\\n-\tmemset(ret, 0, sizeof(type));\t\t\t\t\\\n-\treturn ret;\t\t\t\t\t\t\\\n+\treturn alloc_node(&name##_state, sizeof(type));\t\t\\\n }\n \n union any_object {\n@@ -45,16 +33,39 @@ union any_object {\n \tstruct tag tag;\n };\n \n+struct alloc_state {\n+\tint count; /* total number of nodes allocated */\n+\tint nr; /* number of nodes left in current allocation */\n+\tvoid *p; /* first free node in current allocation */\n+};\n+\n+static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n+{\n+\tvoid *ret;\n+\n+\tif (!s->nr) {\n+\t\ts->nr = BLOCKING;\n+\t\ts->p = xmalloc(BLOCKING * node_size);\n+\t}\n+\ts->nr--;\n+\ts->count++;\n+\tret = s->p;\n+\ts->p = (char *)s->p + node_size;\n+\tmemset(ret, 0, node_size);\n+\treturn ret;\n+}\n+\n DEFINE_ALLOCATOR(blob, struct blob)\n DEFINE_ALLOCATOR(tree, struct tree)\n-DEFINE_ALLOCATOR(raw_commit, struct commit)\n DEFINE_ALLOCATOR(tag, struct tag)\n DEFINE_ALLOCATOR(object, union any_object)\n \n+static struct alloc_state commit_state;\n+\n void *alloc_commit_node(void)\n {\n \tstatic int commit_count;\n-\tstruct commit *c = alloc_raw_commit_node();\n+\tstruct commit *c = alloc_node(&commit_state, sizeof(struct commit));\n \tc->index = commit_count++;\n \treturn c;\n }\n@@ -66,13 +77,13 @@ static void report(const char *name, unsigned int count, size_t size)\n }\n \n #define REPORT(name, type)\t\\\n- report(#name, name##_allocs, name##_allocs * sizeof(type) >> 10)\n+ report(#name, name##_state.count, name##_state.count * sizeof(type) >> 10)\n \n void alloc_report(void)\n {\n \tREPORT(blob, struct blob);\n \tREPORT(tree, struct tree);\n-\tREPORT(raw_commit, struct commit);\n+\tREPORT(commit, struct commit);\n \tREPORT(tag, struct tag);\n \tREPORT(object, union any_object);\n }\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245761","messageId":"20140711084611.GB5625@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140711084141.GA5521@sigill.intra.peff.net","subject":"[PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:46:11Z","receivedAt":"2014-07-11T08:46:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"struct object\" type implements basic object\npolymorphism. Individual instances are allocated as\nconcrete types (or as a union type that can store any\nobject), and a \"struct object *\" can be cast into its real\ntype after examining its \"type\" enum. This means it is\ndangerous to have a type field that does not match the\nallocation (e.g., setting the type field of a \"struct blob\"\nto \"OBJ_COMMIT\" would mean that a reader might read past the\nallocated memory).\n\nIn most of the current code this is not a problem; the first\nthing we do after allocating an object is usually to set its\ntype field by passing it to create_object. However, the\nvirtual commits we create in merge-recursive.c do not ever\nget their type set. This does not seem to have caused\nproblems in practice, though (presumably because we always\npass around a \"struct commit\" pointer and never even look at\nthe type).\n\nWe can fix this oversight and also make it harder for future\ncode to get it wrong by setting the type directly in the\nobject allocation functions.\n\nThis will also make it easier to fix problems with commit\nindex allocation, as we know that any object allocated by\nalloc_commit_node will meet the invariant that an object\nwith an OBJ_COMMIT type field will have a unique index\nnumber.\n\nSigned-off-by: Jeff King \n---\n alloc.c | 18 ++++++++++--------\n blob.c | 2 +-\n builtin/blame.c | 1 -\n commit.c | 2 +-\n object.c | 5 ++---\n object.h | 2 +-\n tag.c | 2 +-\n tree.c | 2 +-\n 8 files changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex d7c3605..fd5fcb7 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -18,11 +18,11 @@\n \n #define BLOCKING 1024\n \n-#define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n+#define DEFINE_ALLOCATOR(name, flag, type)\t\t\t\\\n static struct alloc_state name##_state;\t\t\t\t\\\n void *alloc_##name##_node(void)\t\t\t\t\t\\\n {\t\t\t\t\t\t\t\t\\\n-\treturn alloc_node(&name##_state, sizeof(type));\t\t\\\n+\treturn alloc_node(&name##_state, flag, sizeof(type));\t\\\n }\n \n union any_object {\n@@ -39,7 +39,8 @@ struct alloc_state {\n \tvoid *p; /* first free node in current allocation */\n };\n \n-static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n+static inline void *alloc_node(struct alloc_state *s, enum object_type type,\n+\t\t\t size_t node_size)\n {\n \tvoid *ret;\n \n@@ -52,20 +53,21 @@ static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n \tret = s->p;\n \ts->p = (char *)s->p + node_size;\n \tmemset(ret, 0, node_size);\n+\t((struct object *)ret)->type = type;\n \treturn ret;\n }\n \n-DEFINE_ALLOCATOR(blob, struct blob)\n-DEFINE_ALLOCATOR(tree, struct tree)\n-DEFINE_ALLOCATOR(tag, struct tag)\n-DEFINE_ALLOCATOR(object, union any_object)\n+DEFINE_ALLOCATOR(blob, OBJ_BLOB, struct blob)\n+DEFINE_ALLOCATOR(tree, OBJ_TREE, struct tree)\n+DEFINE_ALLOCATOR(tag, OBJ_TAG, struct tag)\n+DEFINE_ALLOCATOR(object, OBJ_NONE, union any_object)\n \n static struct alloc_state commit_state;\n \n void *alloc_commit_node(void)\n {\n \tstatic int commit_count;\n-\tstruct commit *c = alloc_node(&commit_state, sizeof(struct commit));\n+\tstruct commit *c = alloc_node(&commit_state, OBJ_COMMIT, sizeof(struct commit));\n \tc->index = commit_count++;\n \treturn c;\n }\ndiff --git a/blob.c b/blob.c\nindex ae320bd..5720a38 100644\n--- a/blob.c\n+++ b/blob.c\n@@ -7,7 +7,7 @@ struct blob *lookup_blob(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n-\t\treturn create_object(sha1, OBJ_BLOB, alloc_blob_node());\n+\t\treturn create_object(sha1, alloc_blob_node());\n \tif (!obj->type)\n \t\tobj->type = OBJ_BLOB;\n \tif (obj->type != OBJ_BLOB) {\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex d3b256e..8f3e311 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2287,7 +2287,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tcommit = alloc_commit_node();\n \tcommit->object.parsed = 1;\n \tcommit->date = now;\n-\tcommit->object.type = OBJ_COMMIT;\n \tparent_tail = &commit->parents;\n \n \tif (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\ndiff --git a/commit.c b/commit.c\nindex fb7897c..21ed310 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -63,7 +63,7 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj) {\n \t\tstruct commit *c = alloc_commit_node();\n-\t\treturn create_object(sha1, OBJ_COMMIT, c);\n+\t\treturn create_object(sha1, c);\n \t}\n \tif (!obj->type)\n \t\tobj->type = OBJ_COMMIT;\ndiff --git a/object.c b/object.c\nindex 9c31e9a..a950b85 100644\n--- a/object.c\n+++ b/object.c\n@@ -141,13 +141,12 @@ static void grow_object_hash(void)\n \tobj_hash_size = new_hash_size;\n }\n \n-void *create_object(const unsigned char *sha1, int type, void *o)\n+void *create_object(const unsigned char *sha1, void *o)\n {\n \tstruct object *obj = o;\n \n \tobj->parsed = 0;\n \tobj->used = 0;\n-\tobj->type = type;\n \tobj->flags = 0;\n \thashcpy(obj->sha1, sha1);\n \n@@ -163,7 +162,7 @@ struct object *lookup_unknown_object(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n-\t\tobj = create_object(sha1, OBJ_NONE, alloc_object_node());\n+\t\tobj = create_object(sha1, alloc_object_node());\n \treturn obj;\n }\n \ndiff --git a/object.h b/object.h\nindex 6e12f2c..8020ace 100644\n--- a/object.h\n+++ b/object.h\n@@ -79,7 +79,7 @@ extern struct object *get_indexed_object(unsigned int);\n */\n struct object *lookup_object(const unsigned char *sha1);\n \n-extern void *create_object(const unsigned char *sha1, int type, void *obj);\n+extern void *create_object(const unsigned char *sha1, void *obj);\n \n /*\n * Returns the object, having parsed it to find out what it is.\ndiff --git a/tag.c b/tag.c\nindex 7b07921..79552c7 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -40,7 +40,7 @@ struct tag *lookup_tag(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n-\t\treturn create_object(sha1, OBJ_TAG, alloc_tag_node());\n+\t\treturn create_object(sha1, alloc_tag_node());\n \tif (!obj->type)\n \t\tobj->type = OBJ_TAG;\n \tif (obj->type != OBJ_TAG) {\ndiff --git a/tree.c b/tree.c\nindex c8c49d7..ed66575 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -183,7 +183,7 @@ struct tree *lookup_tree(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n-\t\treturn create_object(sha1, OBJ_TREE, alloc_tree_node());\n+\t\treturn create_object(sha1, alloc_tree_node());\n \tif (!obj->type)\n \t\tobj->type = OBJ_TREE;\n \tif (obj->type != OBJ_TREE) {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245762","messageId":"20140711084625.GC5625@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140711084141.GA5521@sigill.intra.peff.net","subject":"[PATCH 3/7] parse_object_buffer: do not set object type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:46:25Z","receivedAt":"2014-07-11T08:46:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The only way that \"obj\" can be non-NULL is if it came from\none of the lookup_* functions. These functions always ensure\nthat the object has the expected type (and return NULL\notherwise), so there is no need for us to set the type.\n\nSigned-off-by: Jeff King \n---\n object.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex a950b85..472aa8d 100644\n--- a/object.c\n+++ b/object.c\n@@ -213,8 +213,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\twarning(\"object %s has unknown type id %d\", sha1_to_hex(sha1), type);\n \t\tobj = NULL;\n \t}\n-\tif (obj && obj->type == OBJ_NONE)\n-\t\tobj->type = type;\n \treturn obj;\n }\n \n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245763","messageId":"20140711084823.GD5625@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140711084141.GA5521@sigill.intra.peff.net","subject":"[PATCH 4/7] add object_as_type helper for casting objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:48:24Z","receivedAt":"2014-07-11T08:48:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we call lookup_commit, lookup_tree, etc, the logic goes\nsomething like:\n\n 1. Look for an existing object struct. If we don't have\n one, allocate and return a new one.\n\n 2. Double check that any object we have is the expected\n type (and complain and return NULL otherwise).\n\n 3. Convert an object with type OBJ_NONE (from a prior\n call to lookup_unknown_object) to the expected type.\n\nWe can encapsulate steps 2 and 3 in a helper function which\nchecks whether we have the expected object type, converts\nOBJ_NONE as appropriate, and returns the object.\n\nNot only does this shorten the code, but it also provides\none central location for converting OBJ_NONE objects into\nobjects of other types. Future patches will use that to\nenforce type-specific invariants.\n\nSince this is a refactoring, we would want it to behave\nexactly as the current code. It takes a little reasoning to\nsee that this is the case:\n\n - for lookup_{commit,tree,etc} functions, we are just\n pulling steps 2 and 3 into a function that does the same\n thing.\n\n - for the call in peel_object, we currently only do step 3\n (but we want to consolidate it with the others, as\n mentioned above). However, step 2 is a noop here, as the\n surrounding conditional makes sure we have OBJ_NONE\n (which we want to keep to avoid an extraneous call to\n sha1_object_info).\n\n - for the call in lookup_commit_reference_gently, we are\n currently doing step 2 but not step 3. However, step 3\n is a noop here. The object we got will have just come\n from deref_tag, which must have figured out the type for\n each object in order to know when to stop peeling.\n Therefore the type will never be OBJ_NONE.\n\nSigned-off-by: Jeff King \n---\n blob.c | 9 +--------\n commit.c | 19 ++-----------------\n object.c | 17 +++++++++++++++++\n object.h | 2 ++\n refs.c | 3 +--\n tag.c | 9 +--------\n tree.c | 9 +--------\n 7 files changed, 25 insertions(+), 43 deletions(-)\n\ndiff --git a/blob.c b/blob.c\nindex 5720a38..1fcb8e4 100644\n--- a/blob.c\n+++ b/blob.c\n@@ -8,14 +8,7 @@ struct blob *lookup_blob(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n \t\treturn create_object(sha1, alloc_blob_node());\n-\tif (!obj->type)\n-\t\tobj->type = OBJ_BLOB;\n-\tif (obj->type != OBJ_BLOB) {\n-\t\terror(\"Object %s is a %s, not a blob\",\n-\t\t sha1_to_hex(sha1), typename(obj->type));\n-\t\treturn NULL;\n-\t}\n-\treturn (struct blob *) obj;\n+\treturn object_as_type(obj, OBJ_BLOB, 0);\n }\n \n int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size)\ndiff --git a/commit.c b/commit.c\nindex 21ed310..dd638de 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -18,19 +18,6 @@ int save_commit_buffer = 1;\n \n const char *commit_type = \"commit\";\n \n-static struct commit *check_commit(struct object *obj,\n-\t\t\t\t const unsigned char *sha1,\n-\t\t\t\t int quiet)\n-{\n-\tif (obj->type != OBJ_COMMIT) {\n-\t\tif (!quiet)\n-\t\t\terror(\"Object %s is a %s, not a commit\",\n-\t\t\t sha1_to_hex(sha1), typename(obj->type));\n-\t\treturn NULL;\n-\t}\n-\treturn (struct commit *) obj;\n-}\n-\n struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\n \t\t\t\t\t int quiet)\n {\n@@ -38,7 +25,7 @@ struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\n \n \tif (!obj)\n \t\treturn NULL;\n-\treturn check_commit(obj, sha1, quiet);\n+\treturn object_as_type(obj, OBJ_COMMIT, quiet);\n }\n \n struct commit *lookup_commit_reference(const unsigned char *sha1)\n@@ -65,9 +52,7 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \t\tstruct commit *c = alloc_commit_node();\n \t\treturn create_object(sha1, c);\n \t}\n-\tif (!obj->type)\n-\t\tobj->type = OBJ_COMMIT;\n-\treturn check_commit(obj, sha1, 0);\n+\treturn object_as_type(obj, OBJ_COMMIT, 0);\n }\n \n struct commit *lookup_commit_reference_by_name(const char *name)\ndiff --git a/object.c b/object.c\nindex 472aa8d..b2319f6 100644\n--- a/object.c\n+++ b/object.c\n@@ -158,6 +158,23 @@ void *create_object(const unsigned char *sha1, void *o)\n \treturn obj;\n }\n \n+void *object_as_type(struct object *obj, enum object_type type, int quiet)\n+{\n+\tif (obj->type == type)\n+\t\treturn obj;\n+\telse if (obj->type == OBJ_NONE) {\n+\t\tobj->type = type;\n+\t\treturn obj;\n+\t}\n+\telse {\n+\t\tif (!quiet)\n+\t\t\terror(\"object %s is a %s, not a %s\",\n+\t\t\t sha1_to_hex(obj->sha1),\n+\t\t\t typename(obj->type), typename(type));\n+\t\treturn NULL;\n+\t}\n+}\n+\n struct object *lookup_unknown_object(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\ndiff --git a/object.h b/object.h\nindex 8020ace..5e8d8ee 100644\n--- a/object.h\n+++ b/object.h\n@@ -81,6 +81,8 @@ struct object *lookup_object(const unsigned char *sha1);\n \n extern void *create_object(const unsigned char *sha1, void *obj);\n \n+void *object_as_type(struct object *obj, enum object_type type, int quiet);\n+\n /*\n * Returns the object, having parsed it to find out what it is.\n *\ndiff --git a/refs.c b/refs.c\nindex 20e2bf1..5a18e2d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1729,9 +1729,8 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh\n \n \tif (o->type == OBJ_NONE) {\n \t\tint type = sha1_object_info(name, NULL);\n-\t\tif (type < 0)\n+\t\tif (type < 0 || !object_as_type(o, type, 0))\n \t\t\treturn PEEL_INVALID;\n-\t\to->type = type;\n \t}\n \n \tif (o->type != OBJ_TAG)\ndiff --git a/tag.c b/tag.c\nindex 79552c7..82d841b 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -41,14 +41,7 @@ struct tag *lookup_tag(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n \t\treturn create_object(sha1, alloc_tag_node());\n-\tif (!obj->type)\n-\t\tobj->type = OBJ_TAG;\n-\tif (obj->type != OBJ_TAG) {\n-\t\terror(\"Object %s is a %s, not a tag\",\n-\t\t sha1_to_hex(sha1), typename(obj->type));\n-\t\treturn NULL;\n-\t}\n-\treturn (struct tag *) obj;\n+\treturn object_as_type(obj, OBJ_TAG, 0);\n }\n \n static unsigned long parse_tag_date(const char *buf, const char *tail)\ndiff --git a/tree.c b/tree.c\nindex ed66575..bb02c1c 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -184,14 +184,7 @@ struct tree *lookup_tree(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n \t\treturn create_object(sha1, alloc_tree_node());\n-\tif (!obj->type)\n-\t\tobj->type = OBJ_TREE;\n-\tif (obj->type != OBJ_TREE) {\n-\t\terror(\"Object %s is a %s, not a tree\",\n-\t\t sha1_to_hex(sha1), typename(obj->type));\n-\t\treturn NULL;\n-\t}\n-\treturn (struct tree *) obj;\n+\treturn object_as_type(obj, OBJ_TREE, 0);\n }\n \n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245764","messageId":"20140711084847.GE5625@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140711084141.GA5521@sigill.intra.peff.net","subject":"[PATCH 5/7] alloc: factor out commit index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:48:47Z","receivedAt":"2014-07-11T08:48:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep a static counter to set the commit index on newly\nallocated objects. However, since we also need to set the\nindex on any_objects which are converted to commits, let's\nmake the counter available as a public function.\n\nWhile we're moving it, let's make sure the counter is\nallocated as an unsigned integer to match the index field in\n\"struct commit\".\n\nSigned-off-by: Jeff King \n---\n alloc.c | 9 +++++++--\n cache.h | 1 +\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex fd5fcb7..21f3d81 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -64,11 +64,16 @@ DEFINE_ALLOCATOR(object, OBJ_NONE, union any_object)\n \n static struct alloc_state commit_state;\n \n+unsigned int alloc_commit_index(void)\n+{\n+\tstatic unsigned int count;\n+\treturn count++;\n+}\n+\n void *alloc_commit_node(void)\n {\n-\tstatic int commit_count;\n \tstruct commit *c = alloc_node(&commit_state, OBJ_COMMIT, sizeof(struct commit));\n-\tc->index = commit_count++;\n+\tc->index = alloc_commit_index();\n \treturn c;\n }\n \ndiff --git a/cache.h b/cache.h\nindex df65231..42a5e86 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1376,6 +1376,7 @@ extern void *alloc_commit_node(void);\n extern void *alloc_tag_node(void);\n extern void *alloc_object_node(void);\n extern void alloc_report(void);\n+extern unsigned int alloc_commit_index(void);\n \n /* trace.c */\n __attribute__((format (printf, 1, 2)))\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245765","messageId":"20140711084942.GF5625@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140711084141.GA5521@sigill.intra.peff.net","subject":"[PATCH 6/7] object_as_type: set commit index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:49:42Z","receivedAt":"2014-07-11T08:49:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The point of the \"index\" field of struct commit is that\nevery allocated commit would have a uniquely allocated\nvalue. It is supposed to be an invariant that whenever\nobject->type is set to OBJ_COMMIT, we have a unique index.\n\nCommit 969eba6 (commit: push commit_index update into\nalloc_commit_node, 2014-06-10) covered this case for\nnewly-allocated commits. However, we may also allocate an\nOBJ_NONE object via lookup_unknown_object, and only later\nconvert it to a commit. We must make sure that we set the\ncommit index when we switch the type field.\n\nSigned-off-by: Jeff King \n---\n object.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/object.c b/object.c\nindex b2319f6..69fbbbf 100644\n--- a/object.c\n+++ b/object.c\n@@ -163,6 +163,8 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)\n \tif (obj->type == type)\n \t\treturn obj;\n \telse if (obj->type == OBJ_NONE) {\n+\t\tif (type == OBJ_COMMIT)\n+\t\t\t((struct commit *)obj)->index = alloc_commit_index();\n \t\tobj->type = type;\n \t\treturn obj;\n \t}\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245766","messageId":"20140711085041.GG5625@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140711084141.GA5521@sigill.intra.peff.net","subject":"[PATCH 7/7] diff-tree: avoid lookup_unknown_object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T08:50:41Z","receivedAt":"2014-07-11T08:50:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generally want to avoid lookup_unknown_object, because it\nresults in allocating more memory for the object than may be\nstrictly necessary.\n\nIn this case, it is used to check whether we have an\nalready-parsed object before calling parse_object, to save\nus from reading the object from disk. Using lookup_object\nwould be fine for that purpose, but we can take it a step\nfurther. Since this code was written, parse_object already\nlearned the \"check lookup_object\" optimization; we can\nsimply call parse_object directly.\n\nSigned-off-by: Jeff King \n---\n builtin/diff-tree.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex ce0e019..1c4ad62 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -68,9 +68,7 @@ static int diff_tree_stdin(char *line)\n \tline[len-1] = 0;\n \tif (get_sha1_hex(line, sha1))\n \t\treturn -1;\n-\tobj = lookup_unknown_object(sha1);\n-\tif (!obj || !obj->parsed)\n-\t\tobj = parse_object(sha1);\n+\tobj = parse_object(sha1);\n \tif (!obj)\n \t\treturn -1;\n \tif (obj->type == OBJ_COMMIT)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245770","messageId":"53BFB14C.10000@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":"20140711083220.GA5407@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-11T09:41:32Z","receivedAt":"2014-07-11T09:41:32Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 11/07/14 09:32, Jeff King wrote:\n> On Fri, Jul 11, 2014 at 01:59:53AM +0100, Ramsay Jones wrote:\n> \n>>> The code you're touching here was trying to make sure that each commit\n>>> gets a unique index, under the assumption that commits only get\n>>> allocated via alloc_commit_node. But I think that assumption is wrong.\n>>> We can also get commit objects by allocating an OBJ_NONE (e.g., via\n>>> lookup_unknown_object) and then converting it into an OBJ_COMMIT when we\n>>> find out what it is.\n>>\n>> Hmm, I don't know how the object is converted, but the object allocator\n>> is actually allocating an 'union any_object', so it's allocating more\n>> space than for a struct object anyway.\n> \n> Right, we would generally want to avoid lookup_unknown_object where we\n> can for that reason.\n> \n>> If you add an 'index' field to struct object, (and remove it from\n>> struct commit) it could be set in alloc_object_node(). ie _all_ node\n>> types get an index field.\n> \n> That was something I considered when we did the original commit-slab\n> work, as it would let you do similar tricks for any set of objects, not\n> just commits. The reasons against it are:\n> \n> 1. It would bloat the size of blob and tree structs by at least 4\n> bytes (probably 8 for alignment). In most repos, commits make up\n> only 10-20% of the total objects (so for linux.git, we're talking\n> about 25MB extra in the working set).\n> \n> 2. It makes single types sparse in the index space. In cases where you\n> do just want to keep data on commits (and that is the main use),\n> you end up allocating a slab entry per object, rather than per\n> commit. That wastes memory (much worse than 25MB if your slab items\n> are large), and reduces cache locality.\n> \n\nAhem, yeah I keep telling myself not to post at 2am ... ;-)\n\nAlthough I haven't given this too much extra thought, I'm beginning\nto think that your best course would be to revert commit 969eba63\nand add an convert_object_to_commit() function to commit.c which\nwould set c->index. Then track down each place an OBJ_NONE gets\nconverted to an OBJ_COMMIT and insert a call to\nconvert_object_to_commit(). (which may do more than just set the\nindex field ...)\n\nHmm, I've just noticed a new series in my in-box. I guess I will\ndiscover what you decided to do shortly ... ;-P\n\nATB,\nRamsay Jones\n"},{"id":"245774","messageId":"53BFBD14.3070607@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":"20140711084141.GA5521@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] ensure index is set for all OBJ_COMMIT objects variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-11T10:31:48Z","receivedAt":"2014-07-11T10:31:48Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 11/07/14 09:41, Jeff King wrote:\n> Here's a series to address the bug I mentioned earlier by catching the\n> conversion of OBJ_NONE to OBJ_COMMIT in a central location and setting\n> the index there.\n> \n> I've included your patch 1/2 unchanged in the beginning, as I build on\n> top of it (and your patch 2/2 is no longer applicable). The rest is\n> refactoring leading up to patch 6 to fix the bug. Patch 7 is a bonus\n> cleanup.\n\nI have just read this series in my email client (I will apply and test\nthem later), but this looks very good to me. :)\n\nOnly one patch gave me slight pause; see later.\n\n> \n> I'd hoped to cap off the series by converting the \"type\" field of\n> \"struct commit\" to a \"const unsigned type : 3\", which would avoid any\n> new callers being added that would touch it without going through the\n> proper procedure. However, it's a bitfield, which makes it hard to cast\n> the constness away in the actual setter function. My best attempt was to\n> use a union with matching const and non-const members, but that would\n> mean changing all of the sites which read the field (and there are many)\n> to use \"object->type.read\".\n> \n> There may be a clever solution hiding in a dark corner of C, but I\n> suspect we are entering a realm of portability problems with older\n> compilers (I even saw one compiler's documentation claim that \"const\"\n> was forbidden on bitfields, even though C99 has an example which does\n> it).\n\nYes, I've come across such compilers too; I wouldn't go there! ;-P\n\nATB,\nRamsay Jones\n"},{"id":"245775","messageId":"53BFC066.2090102@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":"20140711084823.GD5625@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] add object_as_type helper for casting objects","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-11T10:45:58Z","receivedAt":"2014-07-11T10:45:58Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 11/07/14 09:48, Jeff King wrote:\n\n[snip]\n\n> diff --git a/object.c b/object.c\n> index 472aa8d..b2319f6 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -158,6 +158,23 @@ void *create_object(const unsigned char *sha1, void *o)\n> \treturn obj;\n> }\n> \n> +void *object_as_type(struct object *obj, enum object_type type, int quiet)\n> +{\n> +\tif (obj->type == type)\n> +\t\treturn obj;\n> +\telse if (obj->type == OBJ_NONE) {\n> +\t\tobj->type = type;\n> +\t\treturn obj;\n> +\t}\n> +\telse {\n> +\t\tif (!quiet)\n> +\t\t\terror(\"object %s is a %s, not a %s\",\n> +\t\t\t sha1_to_hex(obj->sha1),\n> +\t\t\t typename(obj->type), typename(type));\n> +\t\treturn NULL;\n> +\t}\n> +}\n> +\n> struct object *lookup_unknown_object(const unsigned char *sha1)\n> {\n> \tstruct object *obj = lookup_object(sha1);\n> diff --git a/object.h b/object.h\n> index 8020ace..5e8d8ee 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -81,6 +81,8 @@ struct object *lookup_object(const unsigned char *sha1);\n> \n> extern void *create_object(const unsigned char *sha1, void *obj);\n> \n> +void *object_as_type(struct object *obj, enum object_type type, int quiet);\n> +\n> /*\n> * Returns the object, having parsed it to find out what it is.\n> *\n> diff --git a/refs.c b/refs.c\n> index 20e2bf1..5a18e2d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1729,9 +1729,8 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh\n> \n> \tif (o->type == OBJ_NONE) {\n> \t\tint type = sha1_object_info(name, NULL);\n> -\t\tif (type < 0)\n> +\t\tif (type < 0 || !object_as_type(o, type, 0))\n--------------------------------------------------------^^^\n\nIt is not possible here for object_as_type() to issue an error()\nreport, but I had to go look at the code to check. (It would have\nbeen a change in behaviour, otherwise). So, it doesn't really matter\nwhat you pass to the quiet argument, but setting it to 1 _may_ help\nthe next reader. (No, I'm not convinced either; the only reason I\nknew it had anything to do with error messages was because I had\njust read the code ...) Hmm, dunno.\n\n> \t\t\treturn PEEL_INVALID;\n> -\t\to->type = type;\n> \t}\n> \n> \tif (o->type != OBJ_TAG)\n> diff --git a/tag.c b/tag.c\n> index 79552c7..82d841b 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -41,14 +41,7 @@ struct tag *lookup_tag(const unsigned char *sha1)\n> \tstruct object *obj = lookup_object(sha1);\n> \tif (!obj)\n> \t\treturn create_object(sha1, alloc_tag_node());\n> -\tif (!obj->type)\n> -\t\tobj->type = OBJ_TAG;\n> -\tif (obj->type != OBJ_TAG) {\n> -\t\terror(\"Object %s is a %s, not a tag\",\n> -\t\t sha1_to_hex(sha1), typename(obj->type));\n> -\t\treturn NULL;\n> -\t}\n> -\treturn (struct tag *) obj;\n> +\treturn object_as_type(obj, OBJ_TAG, 0);\n> }\n> \n> static unsigned long parse_tag_date(const char *buf, const char *tail)\n> diff --git a/tree.c b/tree.c\n> index ed66575..bb02c1c 100644\n> --- a/tree.c\n> +++ b/tree.c\n> @@ -184,14 +184,7 @@ struct tree *lookup_tree(const unsigned char *sha1)\n> \tstruct object *obj = lookup_object(sha1);\n> \tif (!obj)\n> \t\treturn create_object(sha1, alloc_tree_node());\n> -\tif (!obj->type)\n> -\t\tobj->type = OBJ_TREE;\n> -\tif (obj->type != OBJ_TREE) {\n> -\t\terror(\"Object %s is a %s, not a tree\",\n> -\t\t sha1_to_hex(sha1), typename(obj->type));\n> -\t\treturn NULL;\n> -\t}\n> -\treturn (struct tree *) obj;\n> +\treturn object_as_type(obj, OBJ_TREE, 0);\n> }\n> \n> int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\n> \n\nATB,\nRamsay Jones\n"},{"id":"245797","messageId":"20140711165956.GA7856@sigill.intra.peff.net","threadId":"37105","inReplyTo":"53BFC066.2090102@ramsay1.demon.co.uk","subject":"Re: [PATCH 4/7] add object_as_type helper for casting objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T16:59:56Z","receivedAt":"2014-07-11T16:59:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 11:45:58AM +0100, Ramsay Jones wrote:\n\n> > @@ -1729,9 +1729,8 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh\n> > \n> > \tif (o->type == OBJ_NONE) {\n> > \t\tint type = sha1_object_info(name, NULL);\n> > -\t\tif (type < 0)\n> > +\t\tif (type < 0 || !object_as_type(o, type, 0))\n> --------------------------------------------------------^^^\n> \n> It is not possible here for object_as_type() to issue an error()\n> report, but I had to go look at the code to check. (It would have\n> been a change in behaviour, otherwise). So, it doesn't really matter\n> what you pass to the quiet argument, but setting it to 1 _may_ help\n> the next reader. (No, I'm not convinced either; the only reason I\n> knew it had anything to do with error messages was because I had\n> just read the code ...) Hmm, dunno.\n\nRight, as I mentioned in the commit message, the type-check part of\nobject_type is a noop here. In that sense you could write this as just:\n\n object_as_type(o, type. 1);\n\nand ignore the return value. However, I'd rather err on the side of\nchecking for and reporting the error, even if we expect it to do nothing\nright now. That decouples the two functions, and if object_as_type ever\nlearns to report other errors, then we automatically do the right\nthing here.\n\n-Peff\n"},{"id":"245893","messageId":"53C149B6.7010705@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":"20140711084611.GB5625@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-12T14:44:06Z","receivedAt":"2014-07-12T14:44:06Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 11/07/14 09:46, Jeff King wrote:\n> The \"struct object\" type implements basic object\n> polymorphism. Individual instances are allocated as\n> concrete types (or as a union type that can store any\n> object), and a \"struct object *\" can be cast into its real\n> type after examining its \"type\" enum. This means it is\n> dangerous to have a type field that does not match the\n> allocation (e.g., setting the type field of a \"struct blob\"\n> to \"OBJ_COMMIT\" would mean that a reader might read past the\n> allocated memory).\n> \n> In most of the current code this is not a problem; the first\n> thing we do after allocating an object is usually to set its\n> type field by passing it to create_object. However, the\n> virtual commits we create in merge-recursive.c do not ever\n> get their type set. This does not seem to have caused\n> problems in practice, though (presumably because we always\n> pass around a \"struct commit\" pointer and never even look at\n> the type).\n> \n> We can fix this oversight and also make it harder for future\n> code to get it wrong by setting the type directly in the\n> object allocation functions.\n> \n> This will also make it easier to fix problems with commit\n> index allocation, as we know that any object allocated by\n> alloc_commit_node will meet the invariant that an object\n> with an OBJ_COMMIT type field will have a unique index\n> number.\n> \n> Signed-off-by: Jeff King \n> ---\n> alloc.c | 18 ++++++++++--------\n> blob.c | 2 +-\n> builtin/blame.c | 1 -\n> commit.c | 2 +-\n> object.c | 5 ++---\n> object.h | 2 +-\n> tag.c | 2 +-\n> tree.c | 2 +-\n> 8 files changed, 17 insertions(+), 17 deletions(-)\n> \n> diff --git a/alloc.c b/alloc.c\n> index d7c3605..fd5fcb7 100644\n> --- a/alloc.c\n> +++ b/alloc.c\n> @@ -18,11 +18,11 @@\n> \n> #define BLOCKING 1024\n> \n> -#define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n> +#define DEFINE_ALLOCATOR(name, flag, type)\t\t\t\\\n> static struct alloc_state name##_state;\t\t\t\t\\\n> void *alloc_##name##_node(void)\t\t\t\t\t\\\n> {\t\t\t\t\t\t\t\t\\\n> -\treturn alloc_node(&name##_state, sizeof(type));\t\t\\\n> +\treturn alloc_node(&name##_state, flag, sizeof(type));\t\\\n> }\n\nI don't particularly like 'flag' here. (not a massive dislike, mind you:)\n\nPerhaps: flag->object_type, type->node_type?\nOr, if that's too verbose, maybe just: flag->type, type->node?\n\nATB,\nRamsay Jones\n"},{"id":"245894","messageId":"53C14C67.60300@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":"20140711084611.GB5625@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-12T14:55:35Z","receivedAt":"2014-07-12T14:55:35Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 11/07/14 09:46, Jeff King wrote:\n\n[snip]\n\nSorry, hit send too early ...\n\n> diff --git a/blob.c b/blob.c\n> index ae320bd..5720a38 100644\n> --- a/blob.c\n> +++ b/blob.c\n> @@ -7,7 +7,7 @@ struct blob *lookup_blob(const unsigned char *sha1)\n> {\n> \tstruct object *obj = lookup_object(sha1);\n> \tif (!obj)\n> -\t\treturn create_object(sha1, OBJ_BLOB, alloc_blob_node());\n> +\t\treturn create_object(sha1, alloc_blob_node());\n> \tif (!obj->type)\n> \t\tobj->type = OBJ_BLOB;\n> \tif (obj->type != OBJ_BLOB) {\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index d3b256e..8f3e311 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2287,7 +2287,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> \tcommit = alloc_commit_node();\n> \tcommit->object.parsed = 1;\n> \tcommit->date = now;\n> -\tcommit->object.type = OBJ_COMMIT;\n> \tparent_tail = &commit->parents;\n> \n> \tif (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\n> diff --git a/commit.c b/commit.c\n> index fb7897c..21ed310 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -63,7 +63,7 @@ struct commit *lookup_commit(const unsigned char *sha1)\n> \tstruct object *obj = lookup_object(sha1);\n> \tif (!obj) {\n> \t\tstruct commit *c = alloc_commit_node();\n> -\t\treturn create_object(sha1, OBJ_COMMIT, c);\n> +\t\treturn create_object(sha1, c);\n> \t}\n\nperhaps:\n\tif (!obj)\n\t\treturn create_object(sha1, alloc_commit_node());\n\n(increasing similarity with other calls here ...)\n\n> \tif (!obj->type)\n> \t\tobj->type = OBJ_COMMIT;\n> diff --git a/object.c b/object.c\n> index 9c31e9a..a950b85 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -141,13 +141,12 @@ static void grow_object_hash(void)\n> \tobj_hash_size = new_hash_size;\n> }\n> \n> -void *create_object(const unsigned char *sha1, int type, void *o)\n> +void *create_object(const unsigned char *sha1, void *o)\n> {\n> \tstruct object *obj = o;\n> \n> \tobj->parsed = 0;\n> \tobj->used = 0;\n> -\tobj->type = type;\n> \tobj->flags = 0;\n> \thashcpy(obj->sha1, sha1);\n> \n> @@ -163,7 +162,7 @@ struct object *lookup_unknown_object(const unsigned char *sha1)\n> {\n> \tstruct object *obj = lookup_object(sha1);\n> \tif (!obj)\n> -\t\tobj = create_object(sha1, OBJ_NONE, alloc_object_node());\n> +\t\tobj = create_object(sha1, alloc_object_node());\n> \treturn obj;\n> }\n> \n> diff --git a/object.h b/object.h\n> index 6e12f2c..8020ace 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -79,7 +79,7 @@ extern struct object *get_indexed_object(unsigned int);\n> */\n> struct object *lookup_object(const unsigned char *sha1);\n> \n> -extern void *create_object(const unsigned char *sha1, int type, void *obj);\n> +extern void *create_object(const unsigned char *sha1, void *obj);\n> \n> /*\n> * Returns the object, having parsed it to find out what it is.\n> diff --git a/tag.c b/tag.c\n> index 7b07921..79552c7 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -40,7 +40,7 @@ struct tag *lookup_tag(const unsigned char *sha1)\n> {\n> \tstruct object *obj = lookup_object(sha1);\n> \tif (!obj)\n> -\t\treturn create_object(sha1, OBJ_TAG, alloc_tag_node());\n> +\t\treturn create_object(sha1, alloc_tag_node());\n> \tif (!obj->type)\n> \t\tobj->type = OBJ_TAG;\n> \tif (obj->type != OBJ_TAG) {\n> diff --git a/tree.c b/tree.c\n> index c8c49d7..ed66575 100644\n> --- a/tree.c\n> +++ b/tree.c\n> @@ -183,7 +183,7 @@ struct tree *lookup_tree(const unsigned char *sha1)\n> {\n> \tstruct object *obj = lookup_object(sha1);\n> \tif (!obj)\n> -\t\treturn create_object(sha1, OBJ_TREE, alloc_tree_node());\n> +\t\treturn create_object(sha1, alloc_tree_node());\n> \tif (!obj->type)\n> \t\tobj->type = OBJ_TREE;\n> \tif (obj->type != OBJ_TREE) {\n> \n\nATB,\nRamsay Jones\n"},{"id":"245896","messageId":"20140712180539.GA13806@sigill.intra.peff.net","threadId":"37105","inReplyTo":"53C149B6.7010705@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-12T18:05:39Z","receivedAt":"2014-07-12T18:05:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 12, 2014 at 03:44:06PM +0100, Ramsay Jones wrote:\n\n> > -\treturn alloc_node(&name##_state, sizeof(type));\t\t\\\n> > +\treturn alloc_node(&name##_state, flag, sizeof(type));\t\\\n> > }\n> \n> I don't particularly like 'flag' here. (not a massive dislike, mind you:)\n> \n> Perhaps: flag->object_type, type->node_type?\n> Or, if that's too verbose, maybe just: flag->type, type->node?\n\nMe either, but as you noticed, type was taken. Your suggestions seem\nfine. We could also just do away with the macro as discussed earlier (we\nalready do in the commit_node case, anyway...).\n\n-Peff\n"},{"id":"245897","messageId":"20140712180719.GB13806@sigill.intra.peff.net","threadId":"37105","inReplyTo":"53C14C67.60300@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-12T18:07:19Z","receivedAt":"2014-07-12T18:07:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 12, 2014 at 03:55:35PM +0100, Ramsay Jones wrote:\n\n> > \tif (!obj) {\n> > \t\tstruct commit *c = alloc_commit_node();\n> > -\t\treturn create_object(sha1, OBJ_COMMIT, c);\n> > +\t\treturn create_object(sha1, c);\n> > \t}\n> \n> perhaps:\n> \tif (!obj)\n> \t\treturn create_object(sha1, alloc_commit_node());\n> \n> (increasing similarity with other calls here ...)\n\nYeah, I noticed that but didn't change it to keep the diff small. The\none that should have is 969eba6, but it is not a big deal to do it here.\n\n-Peff\n"},{"id":"245937","messageId":"20140713064116.GA4768@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140712180539.GA13806@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:41:16Z","receivedAt":"2014-07-13T06:41:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 12, 2014 at 02:05:39PM -0400, Jeff King wrote:\n\n> > I don't particularly like 'flag' here. (not a massive dislike, mind you:)\n> > \n> > Perhaps: flag->object_type, type->node_type?\n> > Or, if that's too verbose, maybe just: flag->type, type->node?\n> \n> Me either, but as you noticed, type was taken. Your suggestions seem\n> fine. We could also just do away with the macro as discussed earlier (we\n> already do in the commit_node case, anyway...).\n\nThinking on this more, writing out the definitions is the only sane\nthing to do here, now that alloc_commit_node does not use the macro.\nOtherwise you are inviting people to modify the macro, but fail to\nnotice that the commit allocator also needs updating.\n\nHere's a re-roll. The interesting bit is the addition of the second\npatch (but the rest needed to be rebased on top).\n\n [1/8]: alloc.c: remove the alloc_raw_commit_node() function\n [2/8]: alloc: write out allocator definitions\n [3/8]: move setting of object->type to alloc_* functions\n [4/8]: parse_object_buffer: do not set object type\n [5/8]: add object_as_type helper for casting objects\n [6/8]: alloc: factor out commit index\n [7/8]: object_as_type: set commit index\n [8/8]: diff-tree: avoid lookup_unknown_object\n\n-Peff\n"},{"id":"245938","messageId":"20140713064140.GA4852@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"[PATCH v2 1/8] alloc.c: remove the alloc_raw_commit_node() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:41:41Z","receivedAt":"2014-07-13T06:41:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Ramsay Jones \n\nIn order to encapsulate the setting of the unique commit index, commit\n969eba63 (\"commit: push commit_index update into alloc_commit_node\",\n10-06-2014) introduced a (logically private) intermediary allocator\nfunction. However, this function (alloc_raw_commit_node()) was declared\nas a public function, which undermines its entire purpose.\n\nIntroduce an inline function, alloc_node(), which implements the main\nlogic of the allocator used by DEFINE_ALLOCATOR, and redefine the macro\nin terms of the new function. In addition, use the new function in the\nimplementation of the alloc_commit_node() allocator, rather than the\nintermediary allocator, which can now be removed.\n\nNoticed by sparse (\"symbol 'alloc_raw_commit_node' was not declared.\nShould it be static?\").\n\nSigned-off-by: Ramsay Jones \nSigned-off-by: Jeff King \n---\n alloc.c | 47 +++++++++++++++++++++++++++++------------------\n 1 file changed, 29 insertions(+), 18 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex eb22a45..d7c3605 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -19,22 +19,10 @@\n #define BLOCKING 1024\n \n #define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n-static unsigned int name##_allocs;\t\t\t\t\\\n+static struct alloc_state name##_state;\t\t\t\t\\\n void *alloc_##name##_node(void)\t\t\t\t\t\\\n {\t\t\t\t\t\t\t\t\\\n-\tstatic int nr;\t\t\t\t\t\t\\\n-\tstatic type *block;\t\t\t\t\t\\\n-\tvoid *ret;\t\t\t\t\t\t\\\n-\t\t\t\t\t\t\t\t\\\n-\tif (!nr) {\t\t\t\t\t\t\\\n-\t\tnr = BLOCKING;\t\t\t\t\t\\\n-\t\tblock = xmalloc(BLOCKING * sizeof(type));\t\\\n-\t}\t\t\t\t\t\t\t\\\n-\tnr--;\t\t\t\t\t\t\t\\\n-\tname##_allocs++;\t\t\t\t\t\\\n-\tret = block++;\t\t\t\t\t\t\\\n-\tmemset(ret, 0, sizeof(type));\t\t\t\t\\\n-\treturn ret;\t\t\t\t\t\t\\\n+\treturn alloc_node(&name##_state, sizeof(type));\t\t\\\n }\n \n union any_object {\n@@ -45,16 +33,39 @@ union any_object {\n \tstruct tag tag;\n };\n \n+struct alloc_state {\n+\tint count; /* total number of nodes allocated */\n+\tint nr; /* number of nodes left in current allocation */\n+\tvoid *p; /* first free node in current allocation */\n+};\n+\n+static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n+{\n+\tvoid *ret;\n+\n+\tif (!s->nr) {\n+\t\ts->nr = BLOCKING;\n+\t\ts->p = xmalloc(BLOCKING * node_size);\n+\t}\n+\ts->nr--;\n+\ts->count++;\n+\tret = s->p;\n+\ts->p = (char *)s->p + node_size;\n+\tmemset(ret, 0, node_size);\n+\treturn ret;\n+}\n+\n DEFINE_ALLOCATOR(blob, struct blob)\n DEFINE_ALLOCATOR(tree, struct tree)\n-DEFINE_ALLOCATOR(raw_commit, struct commit)\n DEFINE_ALLOCATOR(tag, struct tag)\n DEFINE_ALLOCATOR(object, union any_object)\n \n+static struct alloc_state commit_state;\n+\n void *alloc_commit_node(void)\n {\n \tstatic int commit_count;\n-\tstruct commit *c = alloc_raw_commit_node();\n+\tstruct commit *c = alloc_node(&commit_state, sizeof(struct commit));\n \tc->index = commit_count++;\n \treturn c;\n }\n@@ -66,13 +77,13 @@ static void report(const char *name, unsigned int count, size_t size)\n }\n \n #define REPORT(name, type)\t\\\n- report(#name, name##_allocs, name##_allocs * sizeof(type) >> 10)\n+ report(#name, name##_state.count, name##_state.count * sizeof(type) >> 10)\n \n void alloc_report(void)\n {\n \tREPORT(blob, struct blob);\n \tREPORT(tree, struct tree);\n-\tREPORT(raw_commit, struct commit);\n+\tREPORT(commit, struct commit);\n \tREPORT(tag, struct tag);\n \tREPORT(object, union any_object);\n }\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245939","messageId":"20140713064150.GB4852@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"[PATCH v2 2/8] alloc: write out allocator definitions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:41:51Z","receivedAt":"2014-07-13T06:41:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because the allocator functions for tree, blobs, etc are all\nvery similar, we originally used a macro to avoid repeating\nourselves. Since the prior commit, though, the heavy lifting\nis done by an inline helper function. The macro does still\nsave us a few lines, but at some readability cost. It\nobfuscates the function definitions (and makes them hard to\nfind via grep).\n\nMuch worse, though, is the fact that it isn't used\nconsistently for all allocators. Somebody coming later may\nbe tempted to modify DEFINE_ALLOCATOR, but they would miss\nalloc_commit_node, which is treated specially.\n\nLet's just drop the macro and write everything out\nexplicitly.\n\nSigned-off-by: Jeff King \n---\n alloc.c | 38 +++++++++++++++++++++++++++-----------\n 1 file changed, 27 insertions(+), 11 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex d7c3605..03e458b 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -18,13 +18,6 @@\n \n #define BLOCKING 1024\n \n-#define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n-static struct alloc_state name##_state;\t\t\t\t\\\n-void *alloc_##name##_node(void)\t\t\t\t\t\\\n-{\t\t\t\t\t\t\t\t\\\n-\treturn alloc_node(&name##_state, sizeof(type));\t\t\\\n-}\n-\n union any_object {\n \tstruct object object;\n \tstruct blob blob;\n@@ -55,10 +48,33 @@ static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n \treturn ret;\n }\n \n-DEFINE_ALLOCATOR(blob, struct blob)\n-DEFINE_ALLOCATOR(tree, struct tree)\n-DEFINE_ALLOCATOR(tag, struct tag)\n-DEFINE_ALLOCATOR(object, union any_object)\n+static struct alloc_state blob_state;\n+void *alloc_blob_node(void)\n+{\n+\tstruct blob *b = alloc_node(&blob_state, sizeof(struct blob));\n+\treturn b;\n+}\n+\n+static struct alloc_state tree_state;\n+void *alloc_tree_node(void)\n+{\n+\tstruct tree *t = alloc_node(&tree_state, sizeof(struct tree));\n+\treturn t;\n+}\n+\n+static struct alloc_state tag_state;\n+void *alloc_tag_node(void)\n+{\n+\tstruct tag *t = alloc_node(&tag_state, sizeof(struct tag));\n+\treturn t;\n+}\n+\n+static struct alloc_state object_state;\n+void *alloc_object_node(void)\n+{\n+\tstruct object *obj = alloc_node(&object_state, sizeof(union any_object));\n+\treturn obj;\n+}\n \n static struct alloc_state commit_state;\n \n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245940","messageId":"20140713064155.GC4852@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"[PATCH v2 3/8] move setting of object->type to alloc_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:41:55Z","receivedAt":"2014-07-13T06:41:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"struct object\" type implements basic object\npolymorphism. Individual instances are allocated as\nconcrete types (or as a union type that can store any\nobject), and a \"struct object *\" can be cast into its real\ntype after examining its \"type\" enum. This means it is\ndangerous to have a type field that does not match the\nallocation (e.g., setting the type field of a \"struct blob\"\nto \"OBJ_COMMIT\" would mean that a reader might read past the\nallocated memory).\n\nIn most of the current code this is not a problem; the first\nthing we do after allocating an object is usually to set its\ntype field by passing it to create_object. However, the\nvirtual commits we create in merge-recursive.c do not ever\nget their type set. This does not seem to have caused\nproblems in practice, though (presumably because we always\npass around a \"struct commit\" pointer and never even look at\nthe type).\n\nWe can fix this oversight and also make it harder for future\ncode to get it wrong by setting the type directly in the\nobject allocation functions.\n\nThis will also make it easier to fix problems with commit\nindex allocation, as we know that any object allocated by\nalloc_commit_node will meet the invariant that an object\nwith an OBJ_COMMIT type field will have a unique index\nnumber.\n\nSigned-off-by: Jeff King \n---\n alloc.c | 5 +++++\n blob.c | 2 +-\n builtin/blame.c | 1 -\n commit.c | 6 ++----\n object.c | 5 ++---\n object.h | 2 +-\n tag.c | 2 +-\n tree.c | 2 +-\n 8 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 03e458b..fd2e32d 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -52,6 +52,7 @@ static struct alloc_state blob_state;\n void *alloc_blob_node(void)\n {\n \tstruct blob *b = alloc_node(&blob_state, sizeof(struct blob));\n+\tb->object.type = OBJ_BLOB;\n \treturn b;\n }\n \n@@ -59,6 +60,7 @@ static struct alloc_state tree_state;\n void *alloc_tree_node(void)\n {\n \tstruct tree *t = alloc_node(&tree_state, sizeof(struct tree));\n+\tt->object.type = OBJ_TREE;\n \treturn t;\n }\n \n@@ -66,6 +68,7 @@ static struct alloc_state tag_state;\n void *alloc_tag_node(void)\n {\n \tstruct tag *t = alloc_node(&tag_state, sizeof(struct tag));\n+\tt->object.type = OBJ_TAG;\n \treturn t;\n }\n \n@@ -73,6 +76,7 @@ static struct alloc_state object_state;\n void *alloc_object_node(void)\n {\n \tstruct object *obj = alloc_node(&object_state, sizeof(union any_object));\n+\tobj->type = OBJ_NONE;\n \treturn obj;\n }\n \n@@ -82,6 +86,7 @@ void *alloc_commit_node(void)\n {\n \tstatic int commit_count;\n \tstruct commit *c = alloc_node(&commit_state, sizeof(struct commit));\n+\tc->object.type = OBJ_COMMIT;\n \tc->index = commit_count++;\n \treturn c;\n }\ndiff --git a/blob.c b/blob.c\nindex ae320bd..5720a38 100644\n--- a/blob.c\n+++ b/blob.c\n@@ -7,7 +7,7 @@ struct blob *lookup_blob(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n-\t\treturn create_object(sha1, OBJ_BLOB, alloc_blob_node());\n+\t\treturn create_object(sha1, alloc_blob_node());\n \tif (!obj->type)\n \t\tobj->type = OBJ_BLOB;\n \tif (obj->type != OBJ_BLOB) {\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex d3b256e..8f3e311 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2287,7 +2287,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tcommit = alloc_commit_node();\n \tcommit->object.parsed = 1;\n \tcommit->date = now;\n-\tcommit->object.type = OBJ_COMMIT;\n \tparent_tail = &commit->parents;\n \n \tif (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\ndiff --git a/commit.c b/commit.c\nindex acb74b5..c1815c8 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -61,10 +61,8 @@ struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_n\n struct commit *lookup_commit(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n-\tif (!obj) {\n-\t\tstruct commit *c = alloc_commit_node();\n-\t\treturn create_object(sha1, OBJ_COMMIT, c);\n-\t}\n+\tif (!obj)\n+\t\treturn create_object(sha1, alloc_commit_node());\n \tif (!obj->type)\n \t\tobj->type = OBJ_COMMIT;\n \treturn check_commit(obj, sha1, 0);\ndiff --git a/object.c b/object.c\nindex 9c31e9a..a950b85 100644\n--- a/object.c\n+++ b/object.c\n@@ -141,13 +141,12 @@ static void grow_object_hash(void)\n \tobj_hash_size = new_hash_size;\n }\n \n-void *create_object(const unsigned char *sha1, int type, void *o)\n+void *create_object(const unsigned char *sha1, void *o)\n {\n \tstruct object *obj = o;\n \n \tobj->parsed = 0;\n \tobj->used = 0;\n-\tobj->type = type;\n \tobj->flags = 0;\n \thashcpy(obj->sha1, sha1);\n \n@@ -163,7 +162,7 @@ struct object *lookup_unknown_object(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n-\t\tobj = create_object(sha1, OBJ_NONE, alloc_object_node());\n+\t\tobj = create_object(sha1, alloc_object_node());\n \treturn obj;\n }\n \ndiff --git a/object.h b/object.h\nindex 6e12f2c..8020ace 100644\n--- a/object.h\n+++ b/object.h\n@@ -79,7 +79,7 @@ extern struct object *get_indexed_object(unsigned int);\n */\n struct object *lookup_object(const unsigned char *sha1);\n \n-extern void *create_object(const unsigned char *sha1, int type, void *obj);\n+extern void *create_object(const unsigned char *sha1, void *obj);\n \n /*\n * Returns the object, having parsed it to find out what it is.\ndiff --git a/tag.c b/tag.c\nindex 7b07921..79552c7 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -40,7 +40,7 @@ struct tag *lookup_tag(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n-\t\treturn create_object(sha1, OBJ_TAG, alloc_tag_node());\n+\t\treturn create_object(sha1, alloc_tag_node());\n \tif (!obj->type)\n \t\tobj->type = OBJ_TAG;\n \tif (obj->type != OBJ_TAG) {\ndiff --git a/tree.c b/tree.c\nindex c8c49d7..ed66575 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -183,7 +183,7 @@ struct tree *lookup_tree(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n-\t\treturn create_object(sha1, OBJ_TREE, alloc_tree_node());\n+\t\treturn create_object(sha1, alloc_tree_node());\n \tif (!obj->type)\n \t\tobj->type = OBJ_TREE;\n \tif (obj->type != OBJ_TREE) {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245941","messageId":"20140713064159.GD4852@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"[PATCH v2 4/8] parse_object_buffer: do not set object type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:42:00Z","receivedAt":"2014-07-13T06:42:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The only way that \"obj\" can be non-NULL is if it came from\none of the lookup_* functions. These functions always ensure\nthat the object has the expected type (and return NULL\notherwise), so there is no need for us to set the type.\n\nSigned-off-by: Jeff King \n---\n object.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex a950b85..472aa8d 100644\n--- a/object.c\n+++ b/object.c\n@@ -213,8 +213,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\twarning(\"object %s has unknown type id %d\", sha1_to_hex(sha1), type);\n \t\tobj = NULL;\n \t}\n-\tif (obj && obj->type == OBJ_NONE)\n-\t\tobj->type = type;\n \treturn obj;\n }\n \n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245943","messageId":"20140713064203.GE4852@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"[PATCH v2 5/8] add object_as_type helper for casting objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:42:03Z","receivedAt":"2014-07-13T06:42:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we call lookup_commit, lookup_tree, etc, the logic goes\nsomething like:\n\n 1. Look for an existing object struct. If we don't have\n one, allocate and return a new one.\n\n 2. Double check that any object we have is the expected\n type (and complain and return NULL otherwise).\n\n 3. Convert an object with type OBJ_NONE (from a prior\n call to lookup_unknown_object) to the expected type.\n\nWe can encapsulate steps 2 and 3 in a helper function which\nchecks whether we have the expected object type, converts\nOBJ_NONE as appropriate, and returns the object.\n\nNot only does this shorten the code, but it also provides\none central location for converting OBJ_NONE objects into\nobjects of other types. Future patches will use that to\nenforce type-specific invariants.\n\nSince this is a refactoring, we would want it to behave\nexactly as the current code. It takes a little reasoning to\nsee that this is the case:\n\n - for lookup_{commit,tree,etc} functions, we are just\n pulling steps 2 and 3 into a function that does the same\n thing.\n\n - for the call in peel_object, we currently only do step 3\n (but we want to consolidate it with the others, as\n mentioned above). However, step 2 is a noop here, as the\n surrounding conditional makes sure we have OBJ_NONE\n (which we want to keep to avoid an extraneous call to\n sha1_object_info).\n\n - for the call in lookup_commit_reference_gently, we are\n currently doing step 2 but not step 3. However, step 3\n is a noop here. The object we got will have just come\n from deref_tag, which must have figured out the type for\n each object in order to know when to stop peeling.\n Therefore the type will never be OBJ_NONE.\n\nSigned-off-by: Jeff King \n---\n blob.c | 9 +--------\n commit.c | 19 ++-----------------\n object.c | 17 +++++++++++++++++\n object.h | 2 ++\n refs.c | 3 +--\n tag.c | 9 +--------\n tree.c | 9 +--------\n 7 files changed, 25 insertions(+), 43 deletions(-)\n\ndiff --git a/blob.c b/blob.c\nindex 5720a38..1fcb8e4 100644\n--- a/blob.c\n+++ b/blob.c\n@@ -8,14 +8,7 @@ struct blob *lookup_blob(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n \t\treturn create_object(sha1, alloc_blob_node());\n-\tif (!obj->type)\n-\t\tobj->type = OBJ_BLOB;\n-\tif (obj->type != OBJ_BLOB) {\n-\t\terror(\"Object %s is a %s, not a blob\",\n-\t\t sha1_to_hex(sha1), typename(obj->type));\n-\t\treturn NULL;\n-\t}\n-\treturn (struct blob *) obj;\n+\treturn object_as_type(obj, OBJ_BLOB, 0);\n }\n \n int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size)\ndiff --git a/commit.c b/commit.c\nindex c1815c8..08816d3 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -18,19 +18,6 @@ int save_commit_buffer = 1;\n \n const char *commit_type = \"commit\";\n \n-static struct commit *check_commit(struct object *obj,\n-\t\t\t\t const unsigned char *sha1,\n-\t\t\t\t int quiet)\n-{\n-\tif (obj->type != OBJ_COMMIT) {\n-\t\tif (!quiet)\n-\t\t\terror(\"Object %s is a %s, not a commit\",\n-\t\t\t sha1_to_hex(sha1), typename(obj->type));\n-\t\treturn NULL;\n-\t}\n-\treturn (struct commit *) obj;\n-}\n-\n struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\n \t\t\t\t\t int quiet)\n {\n@@ -38,7 +25,7 @@ struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\n \n \tif (!obj)\n \t\treturn NULL;\n-\treturn check_commit(obj, sha1, quiet);\n+\treturn object_as_type(obj, OBJ_COMMIT, quiet);\n }\n \n struct commit *lookup_commit_reference(const unsigned char *sha1)\n@@ -63,9 +50,7 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n \t\treturn create_object(sha1, alloc_commit_node());\n-\tif (!obj->type)\n-\t\tobj->type = OBJ_COMMIT;\n-\treturn check_commit(obj, sha1, 0);\n+\treturn object_as_type(obj, OBJ_COMMIT, 0);\n }\n \n struct commit *lookup_commit_reference_by_name(const char *name)\ndiff --git a/object.c b/object.c\nindex 472aa8d..b2319f6 100644\n--- a/object.c\n+++ b/object.c\n@@ -158,6 +158,23 @@ void *create_object(const unsigned char *sha1, void *o)\n \treturn obj;\n }\n \n+void *object_as_type(struct object *obj, enum object_type type, int quiet)\n+{\n+\tif (obj->type == type)\n+\t\treturn obj;\n+\telse if (obj->type == OBJ_NONE) {\n+\t\tobj->type = type;\n+\t\treturn obj;\n+\t}\n+\telse {\n+\t\tif (!quiet)\n+\t\t\terror(\"object %s is a %s, not a %s\",\n+\t\t\t sha1_to_hex(obj->sha1),\n+\t\t\t typename(obj->type), typename(type));\n+\t\treturn NULL;\n+\t}\n+}\n+\n struct object *lookup_unknown_object(const unsigned char *sha1)\n {\n \tstruct object *obj = lookup_object(sha1);\ndiff --git a/object.h b/object.h\nindex 8020ace..5e8d8ee 100644\n--- a/object.h\n+++ b/object.h\n@@ -81,6 +81,8 @@ struct object *lookup_object(const unsigned char *sha1);\n \n extern void *create_object(const unsigned char *sha1, void *obj);\n \n+void *object_as_type(struct object *obj, enum object_type type, int quiet);\n+\n /*\n * Returns the object, having parsed it to find out what it is.\n *\ndiff --git a/refs.c b/refs.c\nindex 82e4842..7f8960d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1730,9 +1730,8 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh\n \n \tif (o->type == OBJ_NONE) {\n \t\tint type = sha1_object_info(name, NULL);\n-\t\tif (type < 0)\n+\t\tif (type < 0 || !object_as_type(o, type, 0))\n \t\t\treturn PEEL_INVALID;\n-\t\to->type = type;\n \t}\n \n \tif (o->type != OBJ_TAG)\ndiff --git a/tag.c b/tag.c\nindex 79552c7..82d841b 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -41,14 +41,7 @@ struct tag *lookup_tag(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n \t\treturn create_object(sha1, alloc_tag_node());\n-\tif (!obj->type)\n-\t\tobj->type = OBJ_TAG;\n-\tif (obj->type != OBJ_TAG) {\n-\t\terror(\"Object %s is a %s, not a tag\",\n-\t\t sha1_to_hex(sha1), typename(obj->type));\n-\t\treturn NULL;\n-\t}\n-\treturn (struct tag *) obj;\n+\treturn object_as_type(obj, OBJ_TAG, 0);\n }\n \n static unsigned long parse_tag_date(const char *buf, const char *tail)\ndiff --git a/tree.c b/tree.c\nindex ed66575..bb02c1c 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -184,14 +184,7 @@ struct tree *lookup_tree(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj)\n \t\treturn create_object(sha1, alloc_tree_node());\n-\tif (!obj->type)\n-\t\tobj->type = OBJ_TREE;\n-\tif (obj->type != OBJ_TREE) {\n-\t\terror(\"Object %s is a %s, not a tree\",\n-\t\t sha1_to_hex(sha1), typename(obj->type));\n-\t\treturn NULL;\n-\t}\n-\treturn (struct tree *) obj;\n+\treturn object_as_type(obj, OBJ_TREE, 0);\n }\n \n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245942","messageId":"20140713064208.GF4852@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"[PATCH v2 6/8] alloc: factor out commit index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:42:08Z","receivedAt":"2014-07-13T06:42:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep a static counter to set the commit index on newly\nallocated objects. However, since we also need to set the\nindex on any_objects which are converted to commits, let's\nmake the counter available as a public function.\n\nWhile we're moving it, let's make sure the counter is\nallocated as an unsigned integer to match the index field in\n\"struct commit\".\n\nSigned-off-by: Jeff King \n---\n alloc.c | 9 +++++++--\n cache.h | 1 +\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex fd2e32d..12afadf 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -82,12 +82,17 @@ void *alloc_object_node(void)\n \n static struct alloc_state commit_state;\n \n+unsigned int alloc_commit_index(void)\n+{\n+\tstatic unsigned int count;\n+\treturn count++;\n+}\n+\n void *alloc_commit_node(void)\n {\n-\tstatic int commit_count;\n \tstruct commit *c = alloc_node(&commit_state, sizeof(struct commit));\n \tc->object.type = OBJ_COMMIT;\n-\tc->index = commit_count++;\n+\tc->index = alloc_commit_index();\n \treturn c;\n }\n \ndiff --git a/cache.h b/cache.h\nindex 44aa439..ba68e11 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1380,6 +1380,7 @@ extern void *alloc_commit_node(void);\n extern void *alloc_tag_node(void);\n extern void *alloc_object_node(void);\n extern void alloc_report(void);\n+extern unsigned int alloc_commit_index(void);\n \n /* trace.c */\n __attribute__((format (printf, 1, 2)))\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245945","messageId":"20140713064212.GG4852@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"[PATCH v2 7/8] object_as_type: set commit index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:42:12Z","receivedAt":"2014-07-13T06:42:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The point of the \"index\" field of struct commit is that\nevery allocated commit would have one. It is supposed to be\nan invariant that whenever object->type is set to\nOBJ_COMMIT, we have a unique index.\n\nCommit 969eba6 (commit: push commit_index update into\nalloc_commit_node, 2014-06-10) covered this case for\nnewly-allocated commits. However, we may also allocate an\n\"unknown\" object via lookup_unknown_object, and only later\nconvert it to a commit. We must make sure that we set the\ncommit index when we switch the type field.\n\nSigned-off-by: Jeff King \n---\n object.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/object.c b/object.c\nindex b2319f6..69fbbbf 100644\n--- a/object.c\n+++ b/object.c\n@@ -163,6 +163,8 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)\n \tif (obj->type == type)\n \t\treturn obj;\n \telse if (obj->type == OBJ_NONE) {\n+\t\tif (type == OBJ_COMMIT)\n+\t\t\t((struct commit *)obj)->index = alloc_commit_index();\n \t\tobj->type = type;\n \t\treturn obj;\n \t}\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245944","messageId":"20140713064217.GH4852@sigill.intra.peff.net","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"[PATCH v2 8/8] diff-tree: avoid lookup_unknown_object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-13T06:42:17Z","receivedAt":"2014-07-13T06:42:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generally want to avoid lookup_unknown_object, because it\nresults in allocating more memory for the object than may be\nstrictly necessary.\n\nIn this case, it is used to check whether we have an\nalready-parsed object before calling parse_object, to save\nus from reading the object from disk. Using lookup_object\nwould be fine for that purpose, but we can take it a step\nfurther. Since this code was written, parse_object already\nlearned the \"check lookup_object\" optimization, so we can\nsimply call parse_object directly.\n\nSigned-off-by: Jeff King \n---\n builtin/diff-tree.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex ce0e019..1c4ad62 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -68,9 +68,7 @@ static int diff_tree_stdin(char *line)\n \tline[len-1] = 0;\n \tif (get_sha1_hex(line, sha1))\n \t\treturn -1;\n-\tobj = lookup_unknown_object(sha1);\n-\tif (!obj || !obj->parsed)\n-\t\tobj = parse_object(sha1);\n+\tobj = parse_object(sha1);\n \tif (!obj)\n \t\treturn -1;\n \tif (obj->type == OBJ_COMMIT)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245957","messageId":"53C2DDB7.2070708@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":"20140713064116.GA4768@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-13T19:27:51Z","receivedAt":"2014-07-13T19:27:51Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 13/07/14 07:41, Jeff King wrote:\n> On Sat, Jul 12, 2014 at 02:05:39PM -0400, Jeff King wrote:\n> \n>>> I don't particularly like 'flag' here. (not a massive dislike, mind you:)\n>>>\n>>> Perhaps: flag->object_type, type->node_type?\n>>> Or, if that's too verbose, maybe just: flag->type, type->node?\n>>\n>> Me either, but as you noticed, type was taken. Your suggestions seem\n>> fine. We could also just do away with the macro as discussed earlier (we\n>> already do in the commit_node case, anyway...).\n> \n> Thinking on this more, writing out the definitions is the only sane\n> thing to do here, now that alloc_commit_node does not use the macro.\n> Otherwise you are inviting people to modify the macro, but fail to\n> notice that the commit allocator also needs updating.\n\nHmm, well I could argue that using the macro for all allocators, apart\nfrom alloc_commit_node(), clearly shows which allocator is the odd-man\nout (and conversely, that all others are the same)! :-P\n\nNo, I don't think this is a telling advantage; I don't think it makes\nthat much difference. (six of one, half-a-dozen of the other).\n\nBTW, I tested the previous series on Linux 32-bit, Cygwin 32-bit, MinGW\n32-bit and Cygwin 64-bit. (I can't test on Linux 64-bit, since I can't\nget Linux installed on my new laptop :( ). Admittedly, the testing on\nMinGW and Cygwin was only fairly light (it takes *hours* to run the full\ntestsuite, and I just don't have the time).\n\nI was slightly concerned, when reading through this new series, that the\nalloc_node() function may no longer be inlined in the new allocators.\nHowever, I have just tested on Linux (only using gcc this time), and it\nwas just fine. I will test the new series on the above systems later\n(probably tomorrow) but don't expect to find any problems.\n\n> \n> Here's a re-roll. The interesting bit is the addition of the second\n> patch (but the rest needed to be rebased on top).\n\nYep, this looks good. Thanks!\n\n> \n> [1/8]: alloc.c: remove the alloc_raw_commit_node() function\n> [2/8]: alloc: write out allocator definitions\n> [3/8]: move setting of object->type to alloc_* functions\n> [4/8]: parse_object_buffer: do not set object type\n> [5/8]: add object_as_type helper for casting objects\n> [6/8]: alloc: factor out commit index\n> [7/8]: object_as_type: set commit index\n> [8/8]: diff-tree: avoid lookup_unknown_object\n\nATB,\nRamsay Jones\n"},{"id":"245972","messageId":"20140714055727.GA5593@sigill.intra.peff.net","threadId":"37105","inReplyTo":"53C2DDB7.2070708@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-14T05:57:27Z","receivedAt":"2014-07-14T05:57:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 13, 2014 at 08:27:51PM +0100, Ramsay Jones wrote:\n\n> > Thinking on this more, writing out the definitions is the only sane\n> > thing to do here, now that alloc_commit_node does not use the macro.\n> > Otherwise you are inviting people to modify the macro, but fail to\n> > notice that the commit allocator also needs updating.\n> \n> Hmm, well I could argue that using the macro for all allocators, apart\n> from alloc_commit_node(), clearly shows which allocator is the odd-man\n> out (and conversely, that all others are the same)! :-P\n> \n> No, I don't think this is a telling advantage; I don't think it makes\n> that much difference. (six of one, half-a-dozen of the other).\n\nYeah, I agree with your final statement in parentheses. I am OK with it\neither way (but I have a slight preference for what I posted).\n\n> I was slightly concerned, when reading through this new series, that the\n> alloc_node() function may no longer be inlined in the new allocators.\n> However, I have just tested on Linux (only using gcc this time), and it\n> was just fine. I will test the new series on the above systems later\n> (probably tomorrow) but don't expect to find any problems.\n\nThat should not be due to my patches (which are just expanding macros),\nbut rather to your 1/8, right?\n\nI do not know that it matters that much anyway. Yes, we allocate a lot\nof objects in some workloads. But I think it is not so tight a loop that\nthe extra function call is going to kill us (and we tend to _read_ the\nallocated objects much more than we allocate them).\n\n> > Here's a re-roll. The interesting bit is the addition of the second\n> > patch (but the rest needed to be rebased on top).\n> \n> Yep, this looks good. Thanks!\n\nThanks for reviewing, as usual.\n\n-Peff\n"},{"id":"245983","messageId":"53C3B8F5.5000702@ramsay1.demon.co.uk","threadId":"37105","inReplyTo":"20140714055727.GA5593@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] move setting of object->type to alloc_* functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-14T11:03:17Z","receivedAt":"2014-07-14T11:03:17Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 14/07/14 06:57, Jeff King wrote:\n> On Sun, Jul 13, 2014 at 08:27:51PM +0100, Ramsay Jones wrote:\n> \n>>> Thinking on this more, writing out the definitions is the only sane\n>>> thing to do here, now that alloc_commit_node does not use the macro.\n>>> Otherwise you are inviting people to modify the macro, but fail to\n>>> notice that the commit allocator also needs updating.\n>>\n>> Hmm, well I could argue that using the macro for all allocators, apart\n>> from alloc_commit_node(), clearly shows which allocator is the odd-man\n>> out (and conversely, that all others are the same)! :-P\n>>\n>> No, I don't think this is a telling advantage; I don't think it makes\n>> that much difference. (six of one, half-a-dozen of the other).\n> \n> Yeah, I agree with your final statement in parentheses. I am OK with it\n> either way (but I have a slight preference for what I posted).\n\nYep, once again, this looks good to me. :-)\n\n>> I was slightly concerned, when reading through this new series, that the\n>> alloc_node() function may no longer be inlined in the new allocators.\n>> However, I have just tested on Linux (only using gcc this time), and it\n>> was just fine. I will test the new series on the above systems later\n>> (probably tomorrow) but don't expect to find any problems.\n> \n> That should not be due to my patches (which are just expanding macros),\n> but rather to your 1/8, right?\n\nAh, no. Over the years, I've used many compilers which would happily\ninline this:\n\n void *alloc_blob_node(void)\n {\n \treturn alloc_node(&blob_state, sizeof(struct blob));\n }\n\nbut not this:\n\n void *alloc_blob_node(void)\n {\n \tstruct blob *b = alloc_node(&blob_state, sizeof(struct blob));\n \treturn b;\n }\n\nAdmittedly, it has been many years since I've had to use such a compiler,\nbut old habits die hard and I always check! (The compiler is not obliged\nto inline the code anyway).\n\n> I do not know that it matters that much anyway. Yes, we allocate a lot\n> of objects in some workloads. But I think it is not so tight a loop that\n> the extra function call is going to kill us (and we tend to _read_ the\n> allocated objects much more than we allocate them).\n\nYes, my testing indicates that it wouldn't be noticeable if it wasn't\ninlined, at least on my git.git repo. (The largest repo I have is an\nffmpeg repo, which is only about twice the size of git.git; but I used\ngit.git for the timing/profiling tests). So, I'm always slightly nervous\nabout affecting users of really large repos.\n\nI don't think it will be an issue either way (famous last words!). ;-)\n\nATB,\nRamsay Jones\n"},{"id":"246091","messageId":"xmqq4myiv12f.fsf@gitster.dls.corp.google.com","threadId":"37105","inReplyTo":"20140713064140.GA4852@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/8] alloc.c: remove the alloc_raw_commit_node() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T20:06:48Z","receivedAt":"2014-07-15T20:06:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> #define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n> +static struct alloc_state name##_state;\t\t\t\t\\\n> void *alloc_##name##_node(void)\t\t\t\t\t\\\n> {\t\t\t\t\t\t\t\t\\\n> +\treturn alloc_node(&name##_state, sizeof(type));\t\t\\\n> }\n\nThis is really nice. Thanks.\n\n> +static struct alloc_state commit_state;\n> +\n> void *alloc_commit_node(void)\n> {\n> \tstatic int commit_count;\n> -\tstruct commit *c = alloc_raw_commit_node();\n> +\tstruct commit *c = alloc_node(&commit_state, sizeof(struct commit));\n> \tc->index = commit_count++;\n> \treturn c;\n> }\n"},{"id":"246092","messageId":"xmqqzjgatm9u.fsf@gitster.dls.corp.google.com","threadId":"37105","inReplyTo":"20140713064150.GB4852@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/8] alloc: write out allocator definitions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T20:11:41Z","receivedAt":"2014-07-15T20:11:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Because the allocator functions for tree, blobs, etc are all\n> very similar, we originally used a macro to avoid repeating\n> ourselves. Since the prior commit, though, the heavy lifting\n> is done by an inline helper function. The macro does still\n> save us a few lines, but at some readability cost. It\n> obfuscates the function definitions (and makes them hard to\n> find via grep).\n>\n> Much worse, though, is the fact that it isn't used\n> consistently for all allocators. Somebody coming later may\n> be tempted to modify DEFINE_ALLOCATOR, but they would miss\n> alloc_commit_node, which is treated specially.\n>\n> Let's just drop the macro and write everything out\n> explicitly.\n>\n> Signed-off-by: Jeff King \n> ---\n> alloc.c | 38 +++++++++++++++++++++++++++-----------\n> 1 file changed, 27 insertions(+), 11 deletions(-)\n> ...\n> +static struct alloc_state blob_state;\n> +void *alloc_blob_node(void)\n> +{\n> +\tstruct blob *b = alloc_node(&blob_state, sizeof(struct blob));\n> +\treturn b;\n> +}\n\nI think the change makes the code nicer overall, but it looks\nstrange to see a (void *) that was returned by alloc_node()\nimplicitly casted to (struct blob *) by assignment to b and then\nagain implicitly casted to (void *) by it being the return type of\nthe function.\n\nIs there a reason why it is not like so?\n\n\tvoid *alloc_blob_node(void)\n {\n return alloc_node(&blob_state, sizeof(struct blob));\n\t}\n\nI may have missed previous discussion on it, in which case I'd\napologize in advance.\n"},{"id":"246093","messageId":"xmqqvbqytm7z.fsf@gitster.dls.corp.google.com","threadId":"37105","inReplyTo":"20140713064155.GC4852@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/8] move setting of object->type to alloc_* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T20:12:48Z","receivedAt":"2014-07-15T20:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/alloc.c b/alloc.c\n> index 03e458b..fd2e32d 100644\n> --- a/alloc.c\n> +++ b/alloc.c\n> @@ -52,6 +52,7 @@ static struct alloc_state blob_state;\n> void *alloc_blob_node(void)\n> {\n> \tstruct blob *b = alloc_node(&blob_state, sizeof(struct blob));\n> +\tb->object.type = OBJ_BLOB;\n> \treturn b;\n> }\n\nAhh, please disregard my question on 2/8 ;-)\n"}]}