{"thread":{"id":"642","subject":"[PATCH] Make object contents optionally available","startedAt":"2005-05-17T04:56:36Z","lastAt":"2005-05-17T19:59:53Z","messageCount":7,"participants":["Daniel Barkalow","Linus Torvalds","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"3441","messageId":"Pine.LNX.4.21.0505170049480.30848-100000@iabervon.org","threadId":"642","inReplyTo":null,"subject":"[PATCH] Make object contents optionally available","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-05-17T04:56:36Z","receivedAt":"2005-05-17T04:56:36Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"This adds contents and size fields to struct object. If unpack_object is\ncalled on an object, it will fill in the contents field with the complete\nraw contents of the object. If free_object_contents is called on an\nobject, the contents will be freed. If contents is filled when an object\nis parsed, it is not unpacked an extra time, but the contents are not\nretained if they were not unpacked before parsing.\n\nThis patch also centralizes the code to unpack and check the type of\nobjects, making it worthwhile even without callers for the new functions.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\nIndex: blob.c\n===================================================================\n--- 4fdd3c0825943d0d489ce9ca46cb853b41db3807/blob.c  (mode:100644 sha1:280f5241577ac029e9d5a7eb5bf895642b342fc8)\n+++ e63ecfdf25ec8d61fe16b6c22152a2e45e60efd2/blob.c  (mode:100644 sha1:68c1adc2b1009fbc5525207b0ddaac34e9c4413b)\n@@ -30,21 +30,15 @@\n \n int parse_blob(struct blob *item)\n {\n-        char type[20];\n-        void *buffer;\n-        unsigned long size;\n+\tint unpacked = !!item->object.contents;\n \tint ret;\n \n         if (item->object.parsed)\n                 return 0;\n-        buffer = read_sha1_file(item->object.sha1, type, &size);\n-        if (!buffer)\n-                return error(\"Could not read %s\",\n-                             sha1_to_hex(item->object.sha1));\n-        if (strcmp(type, blob_type))\n-                return error(\"Object %s not a blob\",\n-                             sha1_to_hex(item->object.sha1));\n-\tret = parse_blob_buffer(item, buffer, size);\n-\tfree(buffer);\n+\tunpack_object(&item->object);\n+\tret = parse_blob_buffer(item, item->object.contents, \n+\t\t\t       item->object.size);\n+\tif (!unpacked)\n+\t\tfree_object_contents(&item->object);\n \treturn ret;\n }\nIndex: commit.c\n===================================================================\n--- 4fdd3c0825943d0d489ce9ca46cb853b41db3807/commit.c  (mode:100644 sha1:706c7cba08ebc2100c2dbf63ed1238f39324f750)\n+++ e63ecfdf25ec8d61fe16b6c22152a2e45e60efd2/commit.c  (mode:100644 sha1:38324dcc16785caa4baafb260c2fa9c0af1248a9)\n@@ -69,24 +69,16 @@\n \n int parse_commit(struct commit *item)\n {\n-\tchar type[20];\n-\tvoid *buffer;\n-\tunsigned long size;\n+\tint unpacked = !!item->object.contents;\n \tint ret;\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\tbuffer = read_sha1_file(item->object.sha1, type, &size);\n-\tif (!buffer)\n-\t\treturn error(\"Could not read %s\",\n-\t\t\t     sha1_to_hex(item->object.sha1));\n-\tif (strcmp(type, commit_type)) {\n-\t\tfree(buffer);\n-\t\treturn error(\"Object %s not a commit\",\n-\t\t\t     sha1_to_hex(item->object.sha1));\n-\t}\n-\tret = parse_commit_buffer(item, buffer, size);\n-\tfree(buffer);\n+\tunpack_object(&item->object);\n+\tret = parse_commit_buffer(item, item->object.contents, \n+\t\t\t\t  item->object.size);\n+\tif (!unpacked)\n+\t\tfree_object_contents(&item->object);\n \treturn ret;\n }\n \nIndex: object.c\n===================================================================\n--- 4fdd3c0825943d0d489ce9ca46cb853b41db3807/object.c  (mode:100644 sha1:b5a62e7f87f24c2ab0ea83f3c445d81bcbff027a)\n+++ e63ecfdf25ec8d61fe16b6c22152a2e45e60efd2/object.c  (mode:100644 sha1:3e712d5f665d22e06e310932447629b0badc6b48)\n@@ -99,6 +99,34 @@\n \t}\n }\n \n+int unpack_object(struct object *obj)\n+{\n+\tunsigned long mapsize;\n+\tvoid *map;\n+\tchar type[20];\n+\tif (obj->contents)\n+\t\treturn 0;\n+\tmap = map_sha1_file(obj->sha1, &mapsize);\n+\tif (!map)\n+\t\treturn error(\"Could not map %s\",\n+\t\t\t     sha1_to_hex(obj->sha1));\n+\tobj->contents = unpack_sha1_file(map, mapsize, type, &obj->size);\n+\tmunmap(map, mapsize);\n+\tif (!obj->contents)\n+\t\treturn error(\"Could not unpack %s\",\n+\t\t\t     sha1_to_hex(obj->sha1));\n+\tif (strcmp(type, obj->type))\n+\t\treturn error(\"Object %s not a %s\",\n+\t\t\t     sha1_to_hex(obj->sha1), obj->type);\n+\treturn 0;\n+}\n+\n+void free_object_contents(struct object *obj)\n+{\n+\tfree(obj->contents);\n+\tobj->contents = NULL;\n+}\n+\n struct object *parse_object(unsigned char *sha1)\n {\n \tunsigned long mapsize;\nIndex: object.h\n===================================================================\n--- 4fdd3c0825943d0d489ce9ca46cb853b41db3807/object.h  (mode:100644 sha1:09700d376077b2d6136620faf6efb77ee679deeb)\n+++ e63ecfdf25ec8d61fe16b6c22152a2e45e60efd2/object.h  (mode:100644 sha1:848aee6a1b9138618d5e44abf8567d2c2d3c35c1)\n@@ -13,6 +13,8 @@\n \tunsigned char sha1[20];\n \tconst char *type;\n \tstruct object_list *refs;\n+\tchar *contents;\n+\tunsigned long size;\n };\n \n extern int nr_objs;\n@@ -25,6 +27,12 @@\n /** Returns the object, having parsed it to find out what it is. **/\n struct object *parse_object(unsigned char *sha1);\n \n+/** Unpacks the object into the contents field. **/\n+int unpack_object(struct object *obj);\n+\n+/** Deallocates the unpacked contents of the object. **/\n+void free_object_contents(struct object *obj);\n+\n void add_ref(struct object *refer, struct object *target);\n \n void mark_reachable(struct object *obj, unsigned int mask);\nIndex: tag.c\n===================================================================\n--- 4fdd3c0825943d0d489ce9ca46cb853b41db3807/tag.c  (mode:100644 sha1:22deb243ad58b2c57fb8652fe2d08c571ee3e781)\n+++ e63ecfdf25ec8d61fe16b6c22152a2e45e60efd2/tag.c  (mode:100644 sha1:7fc7960c866a2704ec8f230d24259bc331f7f209)\n@@ -66,23 +66,15 @@\n \n int parse_tag(struct tag *item)\n {\n-\tchar type[20];\n-\tvoid *data;\n-\tunsigned long size;\n+\tint unpacked = !!item->object.contents;\n \tint ret;\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\tdata = read_sha1_file(item->object.sha1, type, &size);\n-\tif (!data)\n-\t\treturn error(\"Could not read %s\",\n-\t\t\t     sha1_to_hex(item->object.sha1));\n-\tif (strcmp(type, tag_type)) {\n-\t\tfree(data);\n-\t\treturn error(\"Object %s not a tag\",\n-\t\t\t     sha1_to_hex(item->object.sha1));\n-\t}\n-\tret = parse_tag_buffer(item, data, size);\n-\tfree(data);\n+\tunpack_object(&item->object);\n+\tret = parse_tag_buffer(item, item->object.contents, \n+\t\t\t       item->object.size);\n+\tif (!unpacked)\n+\t\tfree_object_contents(&item->object);\n \treturn ret;\n }\nIndex: tree.c\n===================================================================\n--- 4fdd3c0825943d0d489ce9ca46cb853b41db3807/tree.c  (mode:100644 sha1:ca800a85f771be1bd10d6575d93ca05bd3fc381c)\n+++ e63ecfdf25ec8d61fe16b6c22152a2e45e60efd2/tree.c  (mode:100644 sha1:3b10428aa0a8b9da32c0deaa16edea38c74da0c0)\n@@ -140,23 +140,15 @@\n \n int parse_tree(struct tree *item)\n {\n-\t char type[20];\n-\t void *buffer;\n-\t unsigned long size;\n-\t int ret;\n+\tint unpacked = !!item->object.contents;\n+\tint ret;\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\tbuffer = read_sha1_file(item->object.sha1, type, &size);\n-\tif (!buffer)\n-\t\treturn error(\"Could not read %s\",\n-\t\t\t     sha1_to_hex(item->object.sha1));\n-\tif (strcmp(type, tree_type)) {\n-\t\tfree(buffer);\n-\t\treturn error(\"Object %s not a tree\",\n-\t\t\t     sha1_to_hex(item->object.sha1));\n-\t}\n-\tret = parse_tree_buffer(item, buffer, size);\n-\tfree(buffer);\n+\tunpack_object(&item->object);\n+\tret = parse_tree_buffer(item, item->object.contents, \n+\t\t\t\titem->object.size);\n+\tif (!unpacked)\n+\t\tfree_object_contents(&item->object);\n \treturn ret;\n }\n\n"},{"id":"3447","messageId":"Pine.LNX.4.58.0505170826061.18337@ppc970.osdl.org","threadId":"642","inReplyTo":"Pine.LNX.4.21.0505170049480.30848-100000@iabervon.org","subject":"Re: [PATCH] Make object contents optionally available","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-17T15:29:37Z","receivedAt":"2005-05-17T15:29:37Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 17 May 2005, Daniel Barkalow wrote:\n>\n> This adds contents and size fields to struct object. If unpack_object is\n> called on an object, it will fill in the contents field with the complete\n> raw contents of the object. If free_object_contents is called on an\n> object, the contents will be freed. If contents is filled when an object\n> is parsed, it is not unpacked an extra time, but the contents are not\n> retained if they were not unpacked before parsing.\n\nI really hate magic interfaces like that. It's just a bug waiting to \nhappen.\n\nI'd actually prefer it if \"parse_object()\" just always unpacks it, and \nleaves the unpacked thing in memory. \n\nThen, the _few_ programs that really care about memory use because they\nparse potentially millions of objects (especially blobs, which obviously\ncan be very large) can be updated to just do a manual\n\"free_object_contents()\". That's primarily things like fsck and\nconvert-cache, I suspect.\n\nThat would leave a lot less special cases and strange rules..\n\n\t\tLinus\n"},{"id":"3449","messageId":"Pine.LNX.4.21.0505171130460.30848-100000@iabervon.org","threadId":"642","inReplyTo":"Pine.LNX.4.58.0505170826061.18337@ppc970.osdl.org","subject":"Re: [PATCH] Make object contents optionally available","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-05-17T15:52:17Z","receivedAt":"2005-05-17T15:52:17Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 17 May 2005, Linus Torvalds wrote:\n\n> \n> \n> On Tue, 17 May 2005, Daniel Barkalow wrote:\n> >\n> > This adds contents and size fields to struct object. If unpack_object is\n> > called on an object, it will fill in the contents field with the complete\n> > raw contents of the object. If free_object_contents is called on an\n> > object, the contents will be freed. If contents is filled when an object\n> > is parsed, it is not unpacked an extra time, but the contents are not\n> > retained if they were not unpacked before parsing.\n> \n> I really hate magic interfaces like that. It's just a bug waiting to \n> happen.\n\nI think I described it with too many cases above. parse_*() doesn't change\nwhether the contents are unpacked. The obvious optimization is performed \nif possible.\n\nI'm already going to add a per-type global to have the parse functions\nalso unpack the object contents user-visibly, for the case that Junio\npointed out. (Making it: parse_* doesn't change whether the contents are\nunpacked, unless you tell it to unpack objects.)\n\nI think the only likely bug would be unpacking objects after parsing\nthem, instead of before, which is correct but inefficient. It should be\nclear to a user whether the raw contents are available at any point in the\nuser code.\n\n\t-Daniel\n*This .sig left intentionally blank*\n\n"},{"id":"3451","messageId":"7vfywlhj3h.fsf@assigned-by-dhcp.cox.net","threadId":"642","inReplyTo":"Pine.LNX.4.21.0505171130460.30848-100000@iabervon.org","subject":"Re: [PATCH] Make object contents optionally available","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-17T17:12:50Z","receivedAt":"2005-05-17T17:12:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"DB\" == Daniel Barkalow <barkalow@iabervon.org> writes:\n\nDB> I'm already going to add a per-type global to have the parse functions\nDB> also unpack the object contents user-visibly, for the case that Junio\nDB> pointed out. (Making it: parse_* doesn't change whether the contents are\nDB> unpacked, unless you tell it to unpack objects.)\n\nThat sounds better than the patch you sent last night, but are\nwe sure that callers would be satisfied if you just make some\n_types_ more special than others?\n\nParse functions need to unpack anyway, so it might make more\nsense to add an optional callback just after where unpacking\nhappens to ask the main program if it wants the unpacked raw\ndata to be kept.  So you would do something like:\n\n    struct object *parse_object(unsigned char *sha1)\n    {\n   ...\n                    unsigned long size;\n                    void *buffer = unpack_sha1_file(map, mapsize, type, &size);\n                    munmap(map, mapsize);\n                    if (!buffer)\n                            return NULL;\n   ...             \n                    } else {\n                            obj = NULL;\n                    }\n+\t\t    if (obj && keep_object_raw_data(sha1, type, size, buffer)) {\n+                           obj.raw_data = buffer;\n+                           obj.raw_size = size;\n+                   } else \n                            free(buffer);\n                    return obj;\n            }\n            return NULL;\n    }\n\nAnd put a dummy implementation of keep_object_raw_data() that\nsays \"I do not want anything to be kept\" in the libgit.a.  Main\nprograms that _care_ about raw data can implement their own\nkeep_object_raw_data() callback that is more intelligent.\nIn addition you give them the interface to free raw data you\nalready have.\n\nDB> I think the only likely bug would be unpacking objects after parsing\nDB> them, instead of before, which is correct but inefficient. It should be\nDB> clear to a user whether the raw contents are available at any point in the\nDB> user code.\n\nAnother possibility is not to make double unpacking too costly\nby having an LRU of unpack_sha1_file(), but I am not sure how\neffective that would be.\n\n"},{"id":"3454","messageId":"Pine.LNX.4.21.0505171325150.30848-100000@iabervon.org","threadId":"642","inReplyTo":"7vfywlhj3h.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Make object contents optionally available","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-05-17T17:55:51Z","receivedAt":"2005-05-17T17:55:51Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 17 May 2005, Junio C Hamano wrote:\n\n> >>>>> \"DB\" == Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> DB> I'm already going to add a per-type global to have the parse functions\n> DB> also unpack the object contents user-visibly, for the case that Junio\n> DB> pointed out. (Making it: parse_* doesn't change whether the contents are\n> DB> unpacked, unless you tell it to unpack objects.)\n> \n> That sounds better than the patch you sent last night, but are\n> we sure that callers would be satisfied if you just make some\n> _types_ more special than others?\n\nIt wouldn't make any types special; the caller can just control each type\nindividually, so that code that only cares about commits could get commits\nunpacked, but not get trees unpacked even if it has them parsed.\n\n> Parse functions need to unpack anyway, so it might make more\n> sense to add an optional callback just after where unpacking\n> happens to ask the main program if it wants the unpacked raw\n> data to be kept.\n\nI think that a callback is a bit excessive, and it wouldn't get the useful\ninformation anyway, which is really: \"what is supposed to happen to this\nobject, which you haven't seen before, after it's parsed?\" \n\n> So you would do something like:\n>     struct object *parse_object(unsigned char *sha1)\n>     {\n\nThat reminds me that I should also fix the parse_object path, which can\nnow be simplified substantially.\n\nNote that parse_object calls parse_<type>, rather than the reverse,\nbecause parse_<type> can skip the step of figuring out what routine to\nuse. The parse_<type> version also has lookup_<type>, which can return the\nstruct which will be filled out later; this isn't possible for an object\nof unknown type, since it can't tell what struct type to allocate.\n\n> Another possibility is not to make double unpacking too costly\n> by having an LRU of unpack_sha1_file(), but I am not sure how\n> effective that would be.\n\nIt would work for pop_most_recent_commit, which parses the thing it's\nabout to return, but I don't think it's a particularly good solution in\ngeneral.\n\n\t-Daniel\n*This .sig left intentionally blank*\n\n"},{"id":"3462","messageId":"7vbr79fy22.fsf@assigned-by-dhcp.cox.net","threadId":"642","inReplyTo":"Pine.LNX.4.21.0505171325150.30848-100000@iabervon.org","subject":"Re: [PATCH] Make object contents optionally available","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-17T19:32:37Z","receivedAt":"2005-05-17T19:32:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"DB\" == Daniel Barkalow <barkalow@iabervon.org> writes:\n\nDB> It wouldn't make any types special; the caller can just control each type\nDB> individually, so that code that only cares about commits could get commits\nDB> unpacked, but not get trees unpacked even if it has them parsed.\n\nThat is exactly the behaviour I am questioning about.  You are\ngiving the caller the ability to discriminate objects based on\ntheir _type_ (which is fine and better than not having that\ncontrol at all).  Why can't the caller discriminate objects\nbased on their, say, size for example [*1*]?  That's what I\nmeant to say by \"types are special\" but I phrased it so badly.\n \nAnd the callback would solve that naturally.  Or you can\nadditionally give the callback the result of the parse_xxx(),\nwhich would be even more useful.  The callback can then throw\ncommit raw-data away based on the date, for example.\n\n[Footnote]\n\n*1* So this hypothethical program uses fast cached raw data for\nsmall things but is willing to pay penalty for big things if it\nlater finds those big things turn out to be needed.\n\n"},{"id":"3463","messageId":"Pine.LNX.4.21.0505171535510.30848-100000@iabervon.org","threadId":"642","inReplyTo":"7vbr79fy22.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Make object contents optionally available","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-05-17T19:59:53Z","receivedAt":"2005-05-17T19:59:53Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 17 May 2005, Junio C Hamano wrote:\n\n> That's what I meant to say by \"types are special\" but I phrased it so\n> badly.\n\nAh, okay. I'm not convinced that discriminating at all at the point where\nparse is deciding whether to free the contents is that useful; doing it\nbased on type is just sufficiently easy that it could be worthwhile.\n\n> And the callback would solve that naturally.  Or you can\n> additionally give the callback the result of the parse_xxx(),\n> which would be even more useful.  The callback can then throw\n> commit raw-data away based on the date, for example.\n\nI don't think a callback has any advantage over turning on a flag to\nprovide all unpacked objects, and having the program free them when it\nwants to, either right after whatever caused them to be parsed returns\nor later. I suspect that the main relevant thing is going to be the stack,\nnot the data.\n\nI have another idea, which is almost certainly too magical, but which\nwould work: have library functions that parse objects return them unpacked\nif the objects they are given are unpacked. So, for example,\npop_most_recent_commit() would unpack parents of a commit if the commit\nwas unpacked. This would make everything work the way you'd expect if you\ndidn't think about it, although it would probably be really surprising if\nyou were thinking about it. I'm just mentioning this because it might make\nsomeone think of something similar but sane.\n\n\t-Daniel\n*This .sig left intentionally blank*\n\n"}]}