{"thread":{"id":"16531","subject":"two questions about the format of loose object","startedAt":"2008-12-01T08:00:55Z","lastAt":"2008-12-04T00:54:26Z","messageCount":27,"participants":["Liu Yubao","Junio C Hamano","Jakub Narebski","Nick Andrew","Shawn O. Pearce","Nicolas Pitre"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"96810","messageId":"493399B7.5000505@gmail.com","threadId":"16531","inReplyTo":null,"subject":"two questions about the format of loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-01T08:00:55Z","receivedAt":"2008-12-01T08:00:55Z","isPatch":false,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Hi,\n\nIn current implementation the loose objects are compressed:\n\n     loose object = deflate(typename + <space> + size + '\\0' + data)\n\nIn sha1_file.c:unpack_sha1_file():\n\t1) unpack_sha1_header() inflates first 8KB\n        2) parse_sha1_header() gets object's size\n        3) unpack_sha1_reset() allocates a (1+size) bytes buffer and\n           copy the first 8KB without header to it.\n\n* Question 1:\n\nWhy not use the format below for loose object?\n    loose object = typename + <space> + size + '\\0' + deflate(data)\n\nSo the size of loose object can be known before inflating it, in\nstep 3 above the 8KB memcpy isn't required.\n\nIn general, deflate() can decrease file size by 70% for text file, \nI checked the git source and linux-2.6 source and got the statistical\ndata below:\n\n.------------------+--------------+--------.\n|                  | <= (8/0.3)KB | <= 8KB |\n|------------------+--------------+--------|\n| git-1.6.03       |          97% |    84% |\n| linux-2.6.27-rc6 |          90% |    66% |\n`------------------+--------------+--------'\n\n\n* Question 2:\n\nWhy not use uncompressed loose object? That's to say:\n   loose object = typename + <space> + size + '\\0' + data\n\nI did a simple benchmark on my notebook and a server in my company,\nwriting a big file to disk is faster than compressing it first and\nwriting the result out. The former's performance for reading should\nalso be better because of file cache.\n\nThe current implementation caches objects in one process, the objects\ncan't be shared by many processes because they are uncompressed\nto heap memory area of each process.\n\nUncompressed loose objects are better for sharing objects among\nmultiple git processes because they can be used directly after being\nmmap-ed.\n\nAnd I guess the most frequently used objects are loose objects\nwhen you do some coding(git add, git diff, git diff --cached, git merge),\nusing uncompressed loose objects avoids uncompressing loose objects again\nand again.\n\n\nBelow is the result of my simple benchmark:\n\n########################################\n# on my notebook\n$ perl b.pl git-1.5.6/Makefile 1000\n               Rate   compressed uncompressed\ncompressed    198/s           --         -92%\nuncompressed 2463/s        1147%           --\n\n\n$ perl b.pl git-1.5.6/parse-options.c 2000\n               Rate   compressed uncompressed\ncompressed    341/s           --         -88%\nuncompressed 2845/s         734%           --\n\n\n$ find git-1.5.6/ -name \"*.[ch]\" -exec cat {} + > all.c\n$ perl b.pl all.c 1000\n               Rate   compressed uncompressed\ncompressed   3.39/s           --         -97%\nuncompressed  111/s        3182%           --\n\n#######################################\n# on a server\n$ perl b.pl Makefile 6000\n            (warning: too few iterations for a reliable count)\n                Rate   compressed uncompressed\ncompressed     447/s           --         -98%\nuncompressed 18750/s        4094%           --\n\n$ perl b.pl parse-options.c 8000\n            (warning: too few iterations for a reliable count)\n                Rate   compressed uncompressed\ncompressed    1130/s           --         -97%\nuncompressed 33333/s        2850%           --\n\n$ perl b.pl all.c 1000\n               Rate   compressed uncompressed\ncompressed   5.48/s           --         -95%\nuncompressed  115/s        1997%           \n\n#####################################################\n# b.pl\n#!/usr/bin/perl\nuse strict;\nuse warnings;\nuse Benchmark qw(:hireswallclock cmpthese);\nuse File::Slurp;\nuse IO::Compress::Deflate qw(deflate $DeflateError);\n\nmy $text = read_file($ARGV[0], binmode => ':raw');\n\ncmpthese($ARGV[1], {'compressed' => \\&zip, 'uncompressed' => \\&output});\n\nsub zip {\n    deflate \\$text => 'all.c.z' || die \"$!\\n\";\n}\n\nsub output {\n    write_file(\"all2.c\", {binmode => ':raw'}, $text);\n}\n"},{"id":"96811","messageId":"7voczws3np.fsf@gitster.siamese.dyndns.org","threadId":"16531","inReplyTo":"493399B7.5000505@gmail.com","subject":"Re: two questions about the format of loose object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-01T08:25:30Z","receivedAt":"2008-12-01T08:25:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Liu Yubao <yubao.liu@gmail.com> writes:\n\n> In current implementation the loose objects are compressed:\n>\n>      loose object = deflate(typename + <space> + size + '\\0' + data)\n>\n> In sha1_file.c:unpack_sha1_file():\n> \t1) unpack_sha1_header() inflates first 8KB\n>         2) parse_sha1_header() gets object's size\n>         3) unpack_sha1_reset() allocates a (1+size) bytes buffer and\n>            copy the first 8KB without header to it.\n>\n> * Question 1:\n> Why not ...\n> * Question 2:\n> Why not ...\n\nA hint for understanding why loose objects are compressed is that\npackfiles were invented much later in the history of git.\n\nThese are both good questions, and it might have made a difference if they\nwere posed in early April 2005.\n\nAt this point, the plain and clear answer to both of these \"Why not\"\nquestions is \"because that is the way it is and it is costly to change\nthem now in thousands of repositories people use every day.\"\n\nIn other words, it is not interesting anymore to raise these questions\nnow, especially as a suggestion to change the system, unless they are\naccompanied by arguments that convinces everybody that the cost of such a\nchange outweighs the benefits, and a clear transition plans how to upgrade\neverybody's existing repositories without any pain.\n"},{"id":"96818","messageId":"4933AE55.2090007@gmail.com","threadId":"16531","inReplyTo":"7voczws3np.fsf@gitster.siamese.dyndns.org","subject":"Re: two questions about the format of loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-01T09:28:53Z","receivedAt":"2008-12-01T09:28:53Z","isPatch":false,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> Liu Yubao <yubao.liu@gmail.com> writes:\n> \n> \n> A hint for understanding why loose objects are compressed is that\n> packfiles were invented much later in the history of git.\n> \n> These are both good questions, and it might have made a difference if they\n> were posed in early April 2005.\n> \n> At this point, the plain and clear answer to both of these \"Why not\"\n> questions is \"because that is the way it is and it is costly to change\n> them now in thousands of repositories people use every day.\"\n> \n> In other words, it is not interesting anymore to raise these questions\n> now, especially as a suggestion to change the system, unless they are\n> accompanied by arguments that convinces everybody that the cost of such a\n> change outweighs the benefits, and a clear transition plans how to upgrade\n> everybody's existing repositories without any pain.\n> \n\nThanks for your explanation, but I doubt if it's too costly to change the\nformat of loose object, after all this doesn't change the format of pack\nfile and affect git-pull/fetch of old git client. \n\nI ask the \"why not\" questions because I doubt if I miss some technical points\nthat the change isn't worth at all in fact.\n\nIf no severe technical problem will occur, I think it's worth breaking\n*forward* compatibility for better performance and I'm willing to implement\nit.\n\nSome cons and pros.\n\ncons:\n\n* old git client can't read loose objects in new format\n  (People degrade git rarely and old git can read pack files\n   generated by new git, so it's not a big problem)\n\npros:\n\n* avoid compressing and uncompressing loose objects that are likely\n  frequently used when you are coding/merging\n* share loose objects among multipe git processes\n* the new code path is simpler although we will have more code paths for\n  compatibility\n\n\nBest regards,\n\nLiu Yubao\n"},{"id":"96827","messageId":"m33ah8jfm2.fsf@localhost.localdomain","threadId":"16531","inReplyTo":"4933AE55.2090007@gmail.com","subject":"Re: two questions about the format of loose object","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-01T11:32:52Z","receivedAt":"2008-12-01T11:32:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Liu Yubao <yubao.liu@gmail.com> writes:\n\n> Junio C Hamano wrote:\n> > Liu Yubao <yubao.liu@gmail.com> writes:\n> > \n> > A hint for understanding why loose objects are compressed is that\n> > packfiles were invented much later in the history of git.\n> > \n> > These are both good questions, and it might have made a difference if they\n> > were posed in early April 2005.\n> > \n> > At this point, the plain and clear answer to both of these \"Why not\"\n> > questions is \"because that is the way it is and it is costly to change\n> > them now in thousands of repositories people use every day.\"\n[...] \n> \n> Thanks for your explanation, but I doubt if it's too costly to change the\n> format of loose object, after all this doesn't change the format of pack\n> file and affect git-pull/fetch of old git client. \n[...]\n\n> cons:\n> \n> * old git client can't read loose objects in new format\n>   (People degrade git rarely and old git can read pack files\n>    generated by new git, so it's not a big problem)\n\nYou forgot about \"dumb\" protocols, namely HTTP and (deprecated) rsync\n(and IIRC also FTP), which doesn't generate packfiles, and would get\nloose object in format intelligible for old clients.\n\nIIRC this was main reason why core.legacyHeaders = false was abandoned.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"96837","messageId":"20081201121611.GC32415@mail.local.tull.net","threadId":"16531","inReplyTo":"493399B7.5000505@gmail.com","subject":"Re: two questions about the format of loose object","fromName":"Nick Andrew","fromEmail":"nick@nick-andrew.net","sentAt":"2008-12-01T12:16:11Z","receivedAt":"2008-12-01T12:16:11Z","isPatch":false,"sender":{"key":"nick@nick-andrew.net","avatar":"https://gravatar.com/avatar/85f25a67ca6eaa4016ed374f6d07f3cd853c886aeb7e1507eb7dbc47b00082fe?d=mp&s=160"},"body":"On Mon, Dec 01, 2008 at 04:00:55PM +0800, Liu Yubao wrote:\n> I did a simple benchmark on my notebook and a server in my company,\n> writing a big file to disk is faster than compressing it first and\n> writing the result out. The former's performance for reading should\n> also be better because of file cache.\n\nIn a corporate environment (and not related to git) I found the\nopposite. The disk was fairly slow (over NFS) and it was in fact\nquicker to read and write compressed files.\n\nNick.\n"},{"id":"96856","messageId":"20081201152148.GG23984@spearce.org","threadId":"16531","inReplyTo":"4933AE55.2090007@gmail.com","subject":"Re: two questions about the format of loose object","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-01T15:21:48Z","receivedAt":"2008-12-01T15:21:48Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Liu Yubao <yubao.liu@gmail.com> wrote:\n> Thanks for your explanation, but I doubt if it's too costly to change the\n> format of loose object, after all this doesn't change the format of pack\n> file and affect git-pull/fetch of old git client. \n\nIt is too costly; Jakub pointed out the dumb protocol clients\nwould have issues with the new format.  Anyone copying a repository\nbetween machines using scp or a USB memory stick may also run into\na problem.  Etc.\n \n> Some cons and pros.\n> \n> cons:\n> \n> * old git client can't read loose objects in new format\n>   (People degrade git rarely and old git can read pack files\n>    generated by new git, so it's not a big problem)\n\nThat's a pretty big con.  We can also add slower performance on NFS,\nas has been reported already by others.\n \n> pros:\n> \n> * avoid compressing and uncompressing loose objects that are likely\n>   frequently used when you are coding/merging\n\nTrue, loose objects are among the more frequently accessed items.\n\n> * share loose objects among multipe git processes\n\nProbably not a huge issue.  How many concurrent git processes are\nyou running on the same object store at once?  During development?\nIts probably not more than 1.  So sharing the objects doesn't make\na very compelling argument.\n\n> * the new code path is simpler although we will have more code paths for\n>   compatibility\n\nThe new code path is more complex, because although one branch is\nvery simple (mmap and use) the other code paths have to stay for\nbackwards compatibility.  Every time you add a branch point the\ncode gets more complex.  It works well enough now, and is at least\none branch point simpler than what you are proposing.  So I'm not\nreally interested in seeing the change made.\n\n-- \nShawn.\n"},{"id":"96859","messageId":"20081201153211.GH23984@spearce.org","threadId":"16531","inReplyTo":"493399B7.5000505@gmail.com","subject":"Re: two questions about the format of loose object","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-01T15:32:11Z","receivedAt":"2008-12-01T15:32:11Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Liu Yubao <yubao.liu@gmail.com> wrote:\n> \n> In current implementation the loose objects are compressed:\n> \n>      loose object = deflate(typename + <space> + size + '\\0' + data)\n...\n> * Question 1:\n> \n> Why not use the format below for loose object?\n>     loose object = typename + <space> + size + '\\0' + deflate(data)\n\nHistorical accident.  We really should have used a format more\nlike what you are asking here, because it makes inflation easier.\nThe pack file format uses a header structure sort of like this,\nfor exactly that reason.  IOW we did learn our mistakes and fix them.\n\nIf you look up the new style loose object code you'll see that it\nhas a format like this (sort of), the header is actually the same\nformat that is used in the pack files, making it smaller than what\nyou propose but also easier to unpack as the code can be reused\nwith the pack reading code.\n\nUnfortunately the new style loose object was phased out; it never\nreally took off and it made the code much more complex.  So it was\npulled in commit 726f852b0ed7e03e88c419a9996c3815911c9db1:\n\n Author: Nicolas Pitre <nico@cam.org>:\n >  deprecate the new loose object header format\n >\n >  Now that we encourage and actively preserve objects in a packed form\n >  more agressively than we did at the time the new loose object format and\n >  core.legacyheaders were introduced, that extra loose object format\n >  doesn't appear to be worth it anymore.\n >\n >  Because the packing of loose objects has to go through the delta match\n >  loop anyway, and since most of them should end up being deltified in\n >  most cases, there is really little advantage to have this parallel loose\n >  object format as the CPU savings it might provide is rather lost in the\n >  noise in the end.\n >\n >  This patch gets rid of core.legacyheaders, preserve the legacy format as\n >  the only writable loose object format and deprecate the other one to\n >  keep things simpler.\n\n-- \nShawn.\n"},{"id":"96902","messageId":"493493ED.8090903@gmail.com","threadId":"16531","inReplyTo":"7voczws3np.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 0/5] support reading and writing uncompressed loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T01:48:29Z","receivedAt":"2008-12-02T01:48:29Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Hi,\n\nIn original implementation, git stores loose object like this:\n    loose object = deflate(typename + <space> + size + data)\n\nThe patches below add support to read and write uncompressed loose\nobject:\n    loose object = typename + <space> + size + data\n\nThe cons and pros to use uncompressed loose object:\n\ncons\n    * old git can't read these uncompressed loose objects\n      (I think it's not a big problem because old git can read\n       pack files generated by new git)\n\n    * uncompressed loose objects occupy more disk space\n      (I also think it's not a big problem because loose objects\n       aren't too many in general)\n\npros\n    * avoid compressing and uncompressing loose objects that are likely\n      frequently used when coding/merging with git add/diff/diff --cached/\n      merge/rebase/log.\n\n    * the code to read and write uncompressed loose objects is\n      simpler, although there are now more code paths for compatibility.\n\n    * better to share loose objects among multiple git processes because\n      sha1 files can be used directly after mmapped. The original git\n      uncompresses loose objects into heap memory area so that they\n      can't be shared by other processes.\n     (NOTICE: The patches below doesn't use mmapped sha1 files directly\n      because I find parse_object() requires a buffer terminated with\n      zero.)\n\n    * easy to grep objects in .git/objects  (...stupid use case :-)\n\n\nIf these patches are worth being included into upstream branch,\nI will add a new config variable core.uncompressedLooseObject.\n\n\nExplanation to the patches:\n\n1) avoid parse_sha1_header() accessing memory out of bound\n  Just for more safety, no inflateInit() to detect errors for\n  uncompressed loose objects.\n\n2) don't die immediately when convert an invalid type name\n  So we can fall back to compressed loose objects.\n\n3) optimize parse_sha1_header() a little by detecting object type\n  To quickly detect whether it seems an uncompressed loose object.\n\n4) support reading uncompressed loose object\n  The new feature.\n\n5) support writing uncompressed loose object\n  The new feature, need a git-config variable yet.\n\n\nThe patches are generated against git-1.6.1-rc, I have run the test cases\nand it seems ok.\n\n\n object.c    |   14 +++++++++++++-\n object.h    |    1 +\n sha1_file.c |   58 +++++++++++++++++++++++++++++++++++++++++++++-------------\n 3 files changed, 59 insertions(+), 14 deletions(-)\n"},{"id":"96903","messageId":"4934949B.70307@gmail.com","threadId":"16531","inReplyTo":"7voczws3np.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/5] avoid parse_sha1_header() accessing memory out of bound","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T01:51:23Z","receivedAt":"2008-12-02T01:51:23Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"\nSigned-off-by: Liu Yubao <yubao.liu@gmail.com>\n---\n sha1_file.c |   15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6c0e251..efe6967 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1245,8 +1245,9 @@ static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size\n  * too permissive for what we want to check. So do an anal\n  * object header parse by hand.\n  */\n-static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n+static int parse_sha1_header(const char *hdr, unsigned long length, unsigned long *sizep)\n {\n+\tconst char *hdr_end = hdr + length;\n \tchar type[10];\n \tint i;\n \tunsigned long size;\n@@ -1254,10 +1255,10 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n \t/*\n \t * The type can be at most ten bytes (including the\n \t * terminating '\\0' that we add), and is followed by\n-\t * a space.\n+\t * a space, at least one byte for size, and a '\\0'.\n \t */\n \ti = 0;\n-\tfor (;;) {\n+\twhile (hdr < hdr_end - 2) {\n \t\tchar c = *hdr++;\n \t\tif (c == ' ')\n \t\t\tbreak;\n@@ -1265,6 +1266,8 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n \t\tif (i >= sizeof(type))\n \t\t\treturn -1;\n \t}\n+\tif (' ' != *(hdr - 1))\n+\t\treturn -1;\n \ttype[i] = 0;\n \n \t/*\n@@ -1275,7 +1278,7 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n \tif (size > 9)\n \t\treturn -1;\n \tif (size) {\n-\t\tfor (;;) {\n+\t\twhile (hdr < hdr_end - 1) {\n \t\t\tunsigned long c = *hdr - '0';\n \t\t\tif (c > 9)\n \t\t\t\tbreak;\n@@ -1298,7 +1301,7 @@ static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type\n \tchar hdr[8192];\n \n \tret = unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr));\n-\tif (ret < Z_OK || (*type = parse_sha1_header(hdr, size)) < 0)\n+\tif (ret < Z_OK || (*type = parse_sha1_header(hdr, stream.total_out, size)) < 0)\n \t\treturn NULL;\n \n \treturn unpack_sha1_rest(&stream, hdr, *size, sha1);\n@@ -1982,7 +1985,7 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n \tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n \t\tstatus = error(\"unable to unpack %s header\",\n \t\t\t       sha1_to_hex(sha1));\n-\telse if ((status = parse_sha1_header(hdr, &size)) < 0)\n+\telse if ((status = parse_sha1_header(hdr, stream.total_out, &size)) < 0)\n \t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n \telse if (sizep)\n \t\t*sizep = size;\n-- \n1.6.1.rc1.5.gde86c\n"},{"id":"96905","messageId":"493494FF.5030001@gmail.com","threadId":"16531","inReplyTo":"7voczws3np.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 2/5] don't die immediately when convert an invalid type name","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T01:53:03Z","receivedAt":"2008-12-02T01:53:03Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"\n\nSigned-off-by: Liu Yubao <yubao.liu@gmail.com>\n---\n object.c    |   14 +++++++++++++-\n object.h    |    1 +\n sha1_file.c |    2 +-\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 50b6528..0a18db6 100644\n--- a/object.c\n+++ b/object.c\n@@ -33,13 +33,25 @@ const char *typename(unsigned int type)\n \treturn object_type_strings[type];\n }\n \n-int type_from_string(const char *str)\n+int type_from_string_gently(const char *str)\n {\n \tint i;\n \n \tfor (i = 1; i < ARRAY_SIZE(object_type_strings); i++)\n \t\tif (!strcmp(str, object_type_strings[i]))\n \t\t\treturn i;\n+\n+\treturn -1;\n+}\n+\n+int type_from_string(const char *str)\n+{\n+\tint i;\n+\n+\ti = type_from_string_gently(str);\n+\tif (i > 0)\n+\t\treturn i;\n+\n \tdie(\"invalid object type \\\"%s\\\"\", str);\n }\n \ndiff --git a/object.h b/object.h\nindex d962ff1..88baf2b 100644\n--- a/object.h\n+++ b/object.h\n@@ -36,6 +36,7 @@ struct object {\n };\n \n extern const char *typename(unsigned int type);\n+extern int type_from_string_gently(const char *str);\n extern int type_from_string(const char *str);\n \n extern unsigned int get_max_object_index(void);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex efe6967..dccc455 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1291,7 +1291,7 @@ static int parse_sha1_header(const char *hdr, unsigned long length, unsigned lon\n \t/*\n \t * The length must be followed by a zero byte\n \t */\n-\treturn *hdr ? -1 : type_from_string(type);\n+\treturn *hdr ? -1 : type_from_string_gently(type);\n }\n \n static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type *type, unsigned long *size, const unsigned char *sha1)\n-- \n1.6.1.rc1.5.gde86c\n"},{"id":"96906","messageId":"49349579.2030506@gmail.com","threadId":"16531","inReplyTo":"7voczws3np.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 3/5] optimize parse_sha1_header() a little by detecting object type","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T01:55:05Z","receivedAt":"2008-12-02T01:55:05Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"\n\nSigned-off-by: Liu Yubao <yubao.liu@gmail.com>\n---\n sha1_file.c |    5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex dccc455..79062f0 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1099,7 +1099,8 @@ static void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n \n \t\tif (!fstat(fd, &st)) {\n \t\t\t*size = xsize_t(st.st_size);\n-\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\t\tif (*size > 0)\n+\t\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n \t\t}\n \t\tclose(fd);\n \t}\n@@ -1257,6 +1258,8 @@ static int parse_sha1_header(const char *hdr, unsigned long length, unsigned lon\n \t * terminating '\\0' that we add), and is followed by\n \t * a space, at least one byte for size, and a '\\0'.\n \t */\n+\tif ('b' != *hdr && 'c' != *hdr && 't' != *hdr)\t/* blob/commit/tag/tree */\n+\t\treturn -1;\n \ti = 0;\n \twhile (hdr < hdr_end - 2) {\n \t\tchar c = *hdr++;\n-- \n1.6.1.rc1.5.gde86c\n"},{"id":"96907","messageId":"493495B4.5070304@gmail.com","threadId":"16531","inReplyTo":"7voczws3np.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 4/5] support reading uncompressed loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T01:56:04Z","receivedAt":"2008-12-02T01:56:04Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"\n\nSigned-off-by: Liu Yubao <yubao.liu@gmail.com>\n---\n sha1_file.c |   20 +++++++++++++++++++-\n 1 files changed, 19 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 79062f0..05a9fa3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1985,6 +1985,16 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n \tmap = map_sha1_file(sha1, &mapsize);\n \tif (!map)\n \t\treturn error(\"unable to find %s\", sha1_to_hex(sha1));\n+\n+\t/*\n+\t * Is it an uncompressed loose objects?\n+\t */\n+\tif ((status = parse_sha1_header(map, mapsize, &size)) >= 0) {\n+\t\tif (sizep)\n+\t\t\t*sizep = size;\n+\t\tgoto L_leave;\n+\t}\n+\n \tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n \t\tstatus = error(\"unable to unpack %s header\",\n \t\t\t       sha1_to_hex(sha1));\n@@ -1993,6 +2003,8 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n \telse if (sizep)\n \t\t*sizep = size;\n \tinflateEnd(&stream);\n+\n+L_leave:\n \tmunmap(map, mapsize);\n \treturn status;\n }\n@@ -2124,7 +2136,13 @@ void *read_object(const unsigned char *sha1, enum object_type *type,\n \t\treturn buf;\n \tmap = map_sha1_file(sha1, &mapsize);\n \tif (map) {\n-\t\tbuf = unpack_sha1_file(map, mapsize, type, size, sha1);\n+\t\t/*\n+\t\t * Is it an uncompressed loose object?\n+\t\t */\n+\t\tif ((*type = parse_sha1_header(map, mapsize, size)) >= 0)\n+\t\t\tbuf = xmemdupz(map + strlen(map) + 1, *size);\n+\t\telse\n+\t\t\tbuf = unpack_sha1_file(map, mapsize, type, size, sha1);\n \t\tmunmap(map, mapsize);\n \t\treturn buf;\n \t}\n-- \n1.6.1.rc1.5.gde86c\n"},{"id":"96909","messageId":"4934975E.2010601@gmail.com","threadId":"16531","inReplyTo":"7voczws3np.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 5/5] support writing uncompressed loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T02:03:10Z","receivedAt":"2008-12-02T02:03:10Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"\n\nSigned-off-by: Liu Yubao <yubao.liu@gmail.com>\n---\n sha1_file.c |   16 ++++++++++++----\n 1 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 05a9fa3..053b564 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2328,7 +2328,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n }\n \n static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n-\t\t\t      void *buf, unsigned long len, time_t mtime)\n+\t\t\t      void *buf, unsigned long len, time_t mtime, int dont_deflate)\n {\n \tint fd, size, ret;\n \tunsigned char *compressed;\n@@ -2345,6 +2345,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \t\t\treturn error(\"unable to create temporary sha1 filename %s: %s\\n\", tmpfile, strerror(errno));\n \t}\n \n+\tif (dont_deflate) {\n+\t\tif (write_buffer(fd, hdr, hdrlen) < 0 || write_buffer(fd, buf, len) < 0)\n+\t\t\tdie(\"unable to write sha1 file\");\n+\t\tgoto L_close_file;\n+\t}\n+\n \t/* Set it up */\n \tmemset(&stream, 0, sizeof(stream));\n \tdeflateInit(&stream, zlib_compression_level);\n@@ -2376,9 +2382,11 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \n \tif (write_buffer(fd, compressed, size) < 0)\n \t\tdie(\"unable to write sha1 file\");\n-\tclose_sha1_file(fd);\n \tfree(compressed);\n \n+L_close_file:\n+\tclose_sha1_file(fd);\n+\n \tif (mtime) {\n \t\tstruct utimbuf utb;\n \t\tutb.actime = mtime;\n@@ -2405,7 +2413,7 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n \t\thashcpy(returnsha1, sha1);\n \tif (has_sha1_file(sha1))\n \t\treturn 0;\n-\treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n+\treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0, 1);\n }\n \n int force_object_loose(const unsigned char *sha1, time_t mtime)\n@@ -2423,7 +2431,7 @@ int force_object_loose(const unsigned char *sha1, time_t mtime)\n \tif (!buf)\n \t\treturn error(\"cannot read sha1_file for %s\", sha1_to_hex(sha1));\n \thdrlen = sprintf(hdr, \"%s %lu\", typename(type), len) + 1;\n-\tret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime);\n+\tret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime, 1);\n \tfree(buf);\n \n \treturn ret;\n-- \n1.6.1.rc1.5.gde86c\n"},{"id":"96910","messageId":"49349B48.9050603@gmail.com","threadId":"16531","inReplyTo":"m33ah8jfm2.fsf@localhost.localdomain","subject":"Re: two questions about the format of loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T02:19:52Z","receivedAt":"2008-12-02T02:19:52Z","isPatch":false,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> Liu Yubao <yubao.liu@gmail.com> writes:\n> \n>> cons:\n>>\n>> * old git client can't read loose objects in new format\n>>   (People degrade git rarely and old git can read pack files\n>>    generated by new git, so it's not a big problem)\n> \n> You forgot about \"dumb\" protocols, namely HTTP and (deprecated) rsync\n> (and IIRC also FTP), which doesn't generate packfiles, and would get\n> loose object in format intelligible for old clients.\n> \n> IIRC this was main reason why core.legacyHeaders = false was abandoned.\n> \n\nThe server can keep an old git or set core.uncompressedLooseObject = false.\n\nEven the pack file format can be changed so that forward compatability will\nbe broken, I think it's unavoidable.\n\nI'm not clear about the story of core.legacyHeaders, I'll dig the mail\nlist archive.\n\nThanks for reminding me of the dumb protocols.\n\n\nBest regards,\n\nLiu Yubao\n"},{"id":"96913","messageId":"49349CE2.9080205@gmail.com","threadId":"16531","inReplyTo":"20081201121611.GC32415@mail.local.tull.net","subject":"Re: two questions about the format of loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T02:26:42Z","receivedAt":"2008-12-02T02:26:42Z","isPatch":false,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Nick Andrew wrote:\n> On Mon, Dec 01, 2008 at 04:00:55PM +0800, Liu Yubao wrote:\n>> I did a simple benchmark on my notebook and a server in my company,\n>> writing a big file to disk is faster than compressing it first and\n>> writing the result out. The former's performance for reading should\n>> also be better because of file cache.\n> \n> In a corporate environment (and not related to git) I found the\n> opposite. The disk was fairly slow (over NFS) and it was in fact\n> quicker to read and write compressed files.\n> \n> Nick.\n> \nOk, there must be exceptional cases, for these cases you can turn\noff core.uncompressedLooseObject (if there will be this config).\n\n\nBest regards,\n\nLiu Yubao\n"},{"id":"96920","messageId":"4934A0EC.5010600@gmail.com","threadId":"16531","inReplyTo":"20081201152148.GG23984@spearce.org","subject":"Re: two questions about the format of loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T02:43:56Z","receivedAt":"2008-12-02T02:43:56Z","isPatch":false,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Shawn O. Pearce wrote:\n> Liu Yubao <yubao.liu@gmail.com> wrote:\n>> Thanks for your explanation, but I doubt if it's too costly to change the\n>> format of loose object, after all this doesn't change the format of pack\n>> file and affect git-pull/fetch of old git client. \n> \n> It is too costly; Jakub pointed out the dumb protocol clients\n> would have issues with the new format.  Anyone copying a repository\n> between machines using scp or a USB memory stick may also run into\n> a problem.  Etc.\n>  \n\nYes, exceptional case, is it acceptable that core.uncompressedLooseObject\nis set to false by default especially for NFS file system?\n\n>> Some cons and pros.\n>>\n>> cons:\n>>\n>> * old git client can't read loose objects in new format\n>>   (People degrade git rarely and old git can read pack files\n>>    generated by new git, so it's not a big problem)\n> \n> That's a pretty big con.  We can also add slower performance on NFS,\n> as has been reported already by others.\n>  \n\nI mean to add a format, not to replace the current format of loose object.\n\n>> pros:\n>>\n>> * avoid compressing and uncompressing loose objects that are likely\n>>   frequently used when you are coding/merging\n> \n> True, loose objects are among the more frequently accessed items.\n> \n>> * share loose objects among multipe git processes\n> \n> Probably not a huge issue.  How many concurrent git processes are\n> you running on the same object store at once?  During development?\n> Its probably not more than 1.  So sharing the objects doesn't make\n> a very compelling argument.\n> \n\nIn my company we have a central server to host source code repository\nmanaged by git+ssh. Some collegues also work on the same machine (maybe\nnot a good practice) and set alternates to the central repository, so\nthere can be multiple git processes operating same git object database.\n\nIn fact we have a wrapper script of git to make git fit our development\nprocess better because git's submodule support isn't good enough. One\ncommand in the wrapper script can execute many git commands in a short\ntime. \n\n>> * the new code path is simpler although we will have more code paths for\n>>   compatibility\n> \n> The new code path is more complex, because although one branch is\n> very simple (mmap and use) the other code paths have to stay for\n> backwards compatibility.  Every time you add a branch point the\n> code gets more complex.  It works well enough now, and is at least\n> one branch point simpler than what you are proposing.  So I'm not\n> really interested in seeing the change made.\n> \n\nCould you review my patches sent just a moment ago? The key changes are\nrather small.\n\n\nBest regards,\n\nLiu Yubao\n"},{"id":"96923","messageId":"4934A5EC.2090708@gmail.com","threadId":"16531","inReplyTo":"20081201153211.GH23984@spearce.org","subject":"Re: two questions about the format of loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T03:05:16Z","receivedAt":"2008-12-02T03:05:16Z","isPatch":false,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Shawn O. Pearce wrote:\n> Liu Yubao <yubao.liu@gmail.com> wrote:\n>> In current implementation the loose objects are compressed:\n>>\n>>      loose object = deflate(typename + <space> + size + '\\0' + data)\n> ...\n>> * Question 1:\n>>\n>> Why not use the format below for loose object?\n>>     loose object = typename + <space> + size + '\\0' + deflate(data)\n> \n> Historical accident.  We really should have used a format more\n> like what you are asking here, because it makes inflation easier.\n> The pack file format uses a header structure sort of like this,\n> for exactly that reason.  IOW we did learn our mistakes and fix them.\n> \n> If you look up the new style loose object code you'll see that it\n> has a format like this (sort of), the header is actually the same\n> format that is used in the pack files, making it smaller than what\n> you propose but also easier to unpack as the code can be reused\n> with the pack reading code.\n> \n> Unfortunately the new style loose object was phased out; it never\n> really took off and it made the code much more complex.  So it was\n> pulled in commit 726f852b0ed7e03e88c419a9996c3815911c9db1:\n> \n\nIn fact the format I proposed in my patches is uncompressed loose\nobject, not uncompressed loose object header, that's to say I\nproposed format 2 in my question 2, I am just curious why the\nloose object header is compressed in question 1.\n\nI did a test to add all files of git-1.6.1-rc1 with git-add, the\ntime spent decreased by half. Other commands like git diff,\ngit diff --cached, git diff HEAD~ HEAD should be faster now\nalthough the change may be not noticable for small and medium project.\n\n\n>  Author: Nicolas Pitre <nico@cam.org>:\n>  >  deprecate the new loose object header format\n>  >\n>  >  Now that we encourage and actively preserve objects in a packed form\n>  >  more agressively than we did at the time the new loose object format and\n>  >  core.legacyheaders were introduced, that extra loose object format\n>  >  doesn't appear to be worth it anymore.\n>  >\n>  >  Because the packing of loose objects has to go through the delta match\n>  >  loop anyway, and since most of them should end up being deltified in\n>  >  most cases, there is really little advantage to have this parallel loose\n>  >  object format as the CPU savings it might provide is rather lost in the\n>  >  noise in the end.\n>  >\n>  >  This patch gets rid of core.legacyheaders, preserve the legacy format as\n>  >  the only writable loose object format and deprecate the other one to\n>  >  keep things simpler.\n> \n\nThank you for dig it out for me!\n\n\nBest regards,\n\nLiu Yubao\n"},{"id":"96925","messageId":"4934A77F.80900@gmail.com","threadId":"16531","inReplyTo":"7voczws3np.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 0/5] support reading and writing uncompressed loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-02T03:11:59Z","receivedAt":"2008-12-02T03:11:59Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Hi all,\n\nIt seems my patch series are not in one mail thread, I'm very sorry\nfor that, I replied this thread and paste my patches into Thunderbird\nwith external editor plugin, don't know why they become separate topics.\n\n\nBest regards,\n\nLiu Yubao\n"},{"id":"96955","messageId":"20081202154257.GK23984@spearce.org","threadId":"16531","inReplyTo":"4934949B.70307@gmail.com","subject":"Re: [PATCH 1/5] avoid parse_sha1_header() accessing memory out of bound","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-02T15:42:57Z","receivedAt":"2008-12-02T15:42:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Liu Yubao <yubao.liu@gmail.com> wrote:\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 6c0e251..efe6967 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1254,10 +1255,10 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>  \t/*\n>  \t * The type can be at most ten bytes (including the\n>  \t * terminating '\\0' that we add), and is followed by\n> -\t * a space.\n> +\t * a space, at least one byte for size, and a '\\0'.\n>  \t */\n>  \ti = 0;\n> -\tfor (;;) {\n> +\twhile (hdr < hdr_end - 2) {\n>  \t\tchar c = *hdr++;\n>  \t\tif (c == ' ')\n>  \t\t\tbreak;\n> @@ -1265,6 +1266,8 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>  \t\tif (i >= sizeof(type))\n>  \t\t\treturn -1;\n\nThat first hunk I am citing is unnecessary, because of the lines\nright above.  All of the callers of this function pass in a buffer\nthat is at least 32 bytes in size; this loop aborts if it does not\nfind a ' ' within the first 10 bytes of the buffer.  We'll never\naccess memory outside of the buffer during this loop.\n\nSo IMHO your first three hunks here aren't necessary.\n\n> @@ -1275,7 +1278,7 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>  \tif (size > 9)\n>  \t\treturn -1;\n>  \tif (size) {\n> -\t\tfor (;;) {\n> +\t\twhile (hdr < hdr_end - 1) {\n>  \t\t\tunsigned long c = *hdr - '0';\n>  \t\t\tif (c > 9)\n>  \t\t\t\tbreak;\n\nOK, there's no promise here that we don't roll off the buffer.\n\nThis can be fixed in the caller, ensuring we always have the '\\0'\nat some point in the initial header buffer we were asked to parse:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6c0e251..26c6ffb 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1162,9 +1162,10 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon\n \tstream->next_in = map;\n \tstream->avail_in = mapsize;\n \tstream->next_out = buffer;\n-\tstream->avail_out = bufsiz;\n \n \tif (legacy_loose_object(map)) {\n+\t\tstream->avail_out = bufsiz - 1;\n+\t\tbuffer[bufsiz - 1] = '\\0';\n \t\tinflateInit(stream);\n \t\treturn inflate(stream, 0);\n \t}\n@@ -1186,6 +1187,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon\n \t/* Set up the stream for the rest.. */\n \tstream->next_in = map;\n \tstream->avail_in = mapsize;\n+\tstream->avail_out = bufsiz;\n \tinflateInit(stream);\n \n \t/* And generate the fake traditional header */\n\n-- \nShawn.\n"},{"id":"96956","messageId":"20081202155300.GL23984@spearce.org","threadId":"16531","inReplyTo":"49349579.2030506@gmail.com","subject":"Re: [PATCH 3/5] optimize parse_sha1_header() a little by detecting object type","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-02T15:53:00Z","receivedAt":"2008-12-02T15:53:00Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Liu Yubao <yubao.liu@gmail.com> wrote:\n> diff --git a/sha1_file.c b/sha1_file.c\n> index dccc455..79062f0 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1099,7 +1099,8 @@ static void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n>  \n>  \t\tif (!fstat(fd, &st)) {\n>  \t\t\t*size = xsize_t(st.st_size);\n> -\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\t\t\tif (*size > 0)\n> +\t\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n>  \t\t}\n>  \t\tclose(fd);\n>  \t}\n\nThis has nothing to do with this change description.  Why are we\nreturning NULL from map_sha1_file when the file length is 0 bytes?\nNo loose object should ever be an empty file, there must always be\nsome sort of type header present.  So it probably is an error to\nhave a 0 length file here.  But that bug is a different change.\n\n> @@ -1257,6 +1258,8 @@ static int parse_sha1_header(const char *hdr, unsigned long length, unsigned lon\n>  \t * terminating '\\0' that we add), and is followed by\n>  \t * a space, at least one byte for size, and a '\\0'.\n>  \t */\n> +\tif ('b' != *hdr && 'c' != *hdr && 't' != *hdr)\t/* blob/commit/tag/tree */\n> +\t\treturn -1;\n>  \ti = 0;\n>  \twhile (hdr < hdr_end - 2) {\n>  \t\tchar c = *hdr++;\n\nOh.  I wouldn't do that.  Its a cute trick and it works to quickly\ndetermine if the header is an uncompressed header vs. a zlib header\nvs. a new-style loose object header (which git cannot write anymore,\nbut it still can read).  But its just asking for trouble when/if a\nnew object type was ever added to the type table.\n\nGiven that we know that no type name can be more than 10 bytes and\nif you use my patch from earlier today you can be certain hdr has a\n'\\0' terminator, so you could write a function to test for the type\nagainst the hdr, stopping on either ' ' or '\\0'.  Or find the first\n' ' in the first 10 bytes (which is what this loop does anyway) and\nthen test that against the type name table.\n\n-- \nShawn.\n"},{"id":"96957","messageId":"20081202155806.GM23984@spearce.org","threadId":"16531","inReplyTo":"493495B4.5070304@gmail.com","subject":"Re: [PATCH 4/5] support reading uncompressed loose object","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-02T15:58:06Z","receivedAt":"2008-12-02T15:58:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Liu Yubao <yubao.liu@gmail.com> wrote:\n> \n> Signed-off-by: Liu Yubao <yubao.liu@gmail.com>\n\nI'd like to see a bit more of an explanation of the new loose\nobject format you are reading in the commit message.  We have a\nlong history of explaining *why* the code behaves the way it does\nin our commits, so we can look at it in blame/log and understand\nwhat the heck went on.\n \n> ---\n>  sha1_file.c |   20 +++++++++++++++++++-\n>  1 files changed, 19 insertions(+), 1 deletions(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 79062f0..05a9fa3 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1985,6 +1985,16 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n>  \tmap = map_sha1_file(sha1, &mapsize);\n>  \tif (!map)\n>  \t\treturn error(\"unable to find %s\", sha1_to_hex(sha1));\n> +\n> +\t/*\n> +\t * Is it an uncompressed loose objects?\n> +\t */\n> +\tif ((status = parse_sha1_header(map, mapsize, &size)) >= 0) {\n> +\t\tif (sizep)\n> +\t\t\t*sizep = size;\n> +\t\tgoto L_leave;\n> +\t}\n> +\n>  \tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n>  \t\tstatus = error(\"unable to unpack %s header\",\n>  \t\t\t       sha1_to_hex(sha1));\n> @@ -1993,6 +2003,8 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n>  \telse if (sizep)\n>  \t\t*sizep = size;\n>  \tinflateEnd(&stream);\n> +\n> +L_leave:\n>  \tmunmap(map, mapsize);\n>  \treturn status;\n>  }\n> @@ -2124,7 +2136,13 @@ void *read_object(const unsigned char *sha1, enum object_type *type,\n>  \t\treturn buf;\n>  \tmap = map_sha1_file(sha1, &mapsize);\n>  \tif (map) {\n> -\t\tbuf = unpack_sha1_file(map, mapsize, type, size, sha1);\n> +\t\t/*\n> +\t\t * Is it an uncompressed loose object?\n> +\t\t */\n> +\t\tif ((*type = parse_sha1_header(map, mapsize, size)) >= 0)\n> +\t\t\tbuf = xmemdupz(map + strlen(map) + 1, *size);\n> +\t\telse\n> +\t\t\tbuf = unpack_sha1_file(map, mapsize, type, size, sha1);\n>  \t\tmunmap(map, mapsize);\n>  \t\treturn buf;\n>  \t}\n\n-- \nShawn.\n"},{"id":"96958","messageId":"20081202160706.GN23984@spearce.org","threadId":"16531","inReplyTo":"4934975E.2010601@gmail.com","subject":"Re: [PATCH 5/5] support writing uncompressed loose object","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-02T16:07:06Z","receivedAt":"2008-12-02T16:07:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Liu Yubao <yubao.liu@gmail.com> wrote:\n> \n> Signed-off-by: Liu Yubao <yubao.liu@gmail.com>\n\nIMHO, this needs more description in the commit message.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 05a9fa3..053b564 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2328,7 +2328,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n>  }\n>  \n>  static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n> -\t\t\t      void *buf, unsigned long len, time_t mtime)\n> +\t\t\t      void *buf, unsigned long len, time_t mtime, int dont_deflate)\n\nPassing this as an argument is pointless.  It should be a repository\nwide configuration option in core, so you can declare it a static and\nallow git_config to populate it.  Defaulting to 1 (no compression)\nlike you do elsewhere in the patch isn't good.\n\nI'm still against this file format change.  The series itself isn't\nthat bad, and the buffer overflow catch in parse_sha1_header()\nmay be something worthwhile fixing.  But I'm still not sold that\nintroducing a new loose object format is worth it.\n\nI'd rather use a binary header encoding like the new-style/in-pack\nformat rather than the older style text headers.  Its faster to\nparse for one thing.\n\nYour changes in the reading code cause a copy of the buffer we\nmmap()'d.  That sort of ruins your argument that this change is\nworthwhile because concurrent processes on the same host can mmap the\nsame buffer and save memory.  We're still copying the buffer anyway.\nI probably should have commented on that in patch 4/5, but I just\nrealized it, so I'm saying it here.\n\n-- \nShawn.\n"},{"id":"97014","messageId":"493601DE.3050107@gmail.com","threadId":"16531","inReplyTo":"20081202154257.GK23984@spearce.org","subject":"Re: [PATCH 1/5] avoid parse_sha1_header() accessing memory out of bound","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-03T03:49:50Z","receivedAt":"2008-12-03T03:49:50Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Shawn O. Pearce wrote:\n> Liu Yubao <yubao.liu@gmail.com> wrote:\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 6c0e251..efe6967 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -1254,10 +1255,10 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>>  \t/*\n>>  \t * The type can be at most ten bytes (including the\n>>  \t * terminating '\\0' that we add), and is followed by\n>> -\t * a space.\n>> +\t * a space, at least one byte for size, and a '\\0'.\n>>  \t */\n>>  \ti = 0;\n>> -\tfor (;;) {\n>> +\twhile (hdr < hdr_end - 2) {\n>>  \t\tchar c = *hdr++;\n>>  \t\tif (c == ' ')\n>>  \t\t\tbreak;\n>> @@ -1265,6 +1266,8 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>>  \t\tif (i >= sizeof(type))\n>>  \t\t\treturn -1;\n> \n> That first hunk I am citing is unnecessary, because of the lines\n> right above.  All of the callers of this function pass in a buffer\n> that is at least 32 bytes in size; this loop aborts if it does not\n> find a ' ' within the first 10 bytes of the buffer.  We'll never\n> access memory outside of the buffer during this loop.\n> \n> So IMHO your first three hunks here aren't necessary.\n> \n\nSeems you missed the cover letter sent as patch 0/5, all patches are explained\nin the cover letter, sorry I sent them as separate topics by mistake.\n\nThis bound check is mainly for uncompressed loose object, a loose object\nthat just are uncompressed:\n\nuncompressed loose object = inflate(loose object)\nloose object = deflate(typename + <space> + size + '\\0' + data)\n\nI'm doing a defensive programming, for uncompressed loose object the mmapped\nmemory is passed to parse_sha1_header without being checked by inflateInit() first,\nso there may be a SIGSEGV crash for a corrupted uncompressed loose object.\n\n\n>> @@ -1275,7 +1278,7 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>>  \tif (size > 9)\n>>  \t\treturn -1;\n>>  \tif (size) {\n>> -\t\tfor (;;) {\n>> +\t\twhile (hdr < hdr_end - 1) {\n>>  \t\t\tunsigned long c = *hdr - '0';\n>>  \t\t\tif (c > 9)\n>>  \t\t\t\tbreak;\n> \n> OK, there's no promise here that we don't roll off the buffer.\n> \n> This can be fixed in the caller, ensuring we always have the '\\0'\n> at some point in the initial header buffer we were asked to parse:\n> \nIsn't it easier to solve this problem in one place and maintain it? Maybe someday\nsomeone forgets parse_sha1_header requires a null terminated buffer, and a corrupted\nuncompressed loose object even doesn't have to be null terminated (if there will be\nthis kind of loose object).\n"},{"id":"97017","messageId":"493605BB.8020705@gmail.com","threadId":"16531","inReplyTo":"20081202155300.GL23984@spearce.org","subject":"Re: [PATCH 3/5] optimize parse_sha1_header() a little by detecting object type","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-03T04:06:19Z","receivedAt":"2008-12-03T04:06:19Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Shawn O. Pearce wrote:\n> Liu Yubao <yubao.liu@gmail.com> wrote:\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index dccc455..79062f0 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -1099,7 +1099,8 @@ static void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n>>  \n>>  \t\tif (!fstat(fd, &st)) {\n>>  \t\t\t*size = xsize_t(st.st_size);\n>> -\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n>> +\t\t\tif (*size > 0)\n>> +\t\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n>>  \t\t}\n>>  \t\tclose(fd);\n>>  \t}\n> \n> This has nothing to do with this change description.  Why are we\n> returning NULL from map_sha1_file when the file length is 0 bytes?\n> No loose object should ever be an empty file, there must always be\n> some sort of type header present.  So it probably is an error to\n> have a 0 length file here.  But that bug is a different change.\n> \n\nAlso a defensive programming for uncompressed loose object because the mapped memory\nwill be passed to parse_sha1_header() directly without being checked by inflateInit().\n\nIn fact unpack_sha1_header() in current code calls legacy_loose_object() without checking\nmapsize first. If it encounters (very very unlikely) a corrupted empty loose object, it\nwill crash.\n\n>> @@ -1257,6 +1258,8 @@ static int parse_sha1_header(const char *hdr, unsigned long length, unsigned lon\n>>  \t * terminating '\\0' that we add), and is followed by\n>>  \t * a space, at least one byte for size, and a '\\0'.\n>>  \t */\n>> +\tif ('b' != *hdr && 'c' != *hdr && 't' != *hdr)\t/* blob/commit/tag/tree */\n>> +\t\treturn -1;\n>>  \ti = 0;\n>>  \twhile (hdr < hdr_end - 2) {\n>>  \t\tchar c = *hdr++;\n> \n> Oh.  I wouldn't do that.  Its a cute trick and it works to quickly\n> determine if the header is an uncompressed header vs. a zlib header\n> vs. a new-style loose object header (which git cannot write anymore,\n> but it still can read).  But its just asking for trouble when/if a\n> new object type was ever added to the type table.\n> \nI can't agree any more, it's just a trick. I considered adding\na function seems_likely_uncompressed_loose_object(), but I didn't\nbecause this patch series are just my first try, I don't know whether\nthe idea to support uncompressed loose object is attractive enough.\n\n> Given that we know that no type name can be more than 10 bytes and\n> if you use my patch from earlier today you can be certain hdr has a\n> '\\0' terminator, so you could write a function to test for the type\n> against the hdr, stopping on either ' ' or '\\0'.  Or find the first\n> ' ' in the first 10 bytes (which is what this loop does anyway) and\n> then test that against the type name table.\n> \n"},{"id":"97020","messageId":"49360687.4060701@gmail.com","threadId":"16531","inReplyTo":"20081202155806.GM23984@spearce.org","subject":"Re: [PATCH 4/5] support reading uncompressed loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-03T04:09:43Z","receivedAt":"2008-12-03T04:09:43Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Shawn O. Pearce wrote:\n> Liu Yubao <yubao.liu@gmail.com> wrote:\n>> Signed-off-by: Liu Yubao <yubao.liu@gmail.com>\n> \n> I'd like to see a bit more of an explanation of the new loose\n> object format you are reading in the commit message.  We have a\n> long history of explaining *why* the code behaves the way it does\n> in our commits, so we can look at it in blame/log and understand\n> what the heck went on.\n>  \nI mentioned in the cover letter sent as patch 0/5, indeed I should\nhave mentioned it in the commit message again.\n\nAn uncompressed loose object is just an uncompressed loose object:\n\nloose object = deflate(typename + <space> + size + '\\0' + data)\nuncompressed object = inflate(loose object)\n"},{"id":"97021","messageId":"49360996.40106@gmail.com","threadId":"16531","inReplyTo":"20081202160706.GN23984@spearce.org","subject":"Re: [PATCH 5/5] support writing uncompressed loose object","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2008-12-03T04:22:46Z","receivedAt":"2008-12-03T04:22:46Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Shawn O. Pearce wrote:\n> Liu Yubao <yubao.liu@gmail.com> wrote:\n>> Signed-off-by: Liu Yubao <yubao.liu@gmail.com>\n> \n> IMHO, this needs more description in the commit message.\n> \n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 05a9fa3..053b564 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -2328,7 +2328,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n>>  }\n>>  \n>>  static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n>> -\t\t\t      void *buf, unsigned long len, time_t mtime)\n>> +\t\t\t      void *buf, unsigned long len, time_t mtime, int dont_deflate)\n> \n> Passing this as an argument is pointless.  It should be a repository\n> wide configuration option in core, so you can declare it a static and\n> allow git_config to populate it.  Defaulting to 1 (no compression)\n> like you do elsewhere in the patch isn't good.\n> \nAha, sorry again, I sent the patch series as separate topics by mistake.\n\nI considered adding a configuration variable, the patch series are sent\njust to see whether the idea is worth.\n\n> I'm still against this file format change.  The series itself isn't\n> that bad, and the buffer overflow catch in parse_sha1_header()\n> may be something worthwhile fixing.  But I'm still not sold that\n> introducing a new loose object format is worth it.\n> \n> I'd rather use a binary header encoding like the new-style/in-pack\n> format rather than the older style text headers.  Its faster to\n> parse for one thing.\n> \nThe key point I suggest is to use *uncompressed* loose object, I didn't\nchange the format of uncompressed loose object because I don't want\nto distract your attention and keep the patches small.\n\n> Your changes in the reading code cause a copy of the buffer we\n> mmap()'d.  That sort of ruins your argument that this change is\n> worthwhile because concurrent processes on the same host can mmap the\n> same buffer and save memory.  We're still copying the buffer anyway.\n> I probably should have commented on that in patch 4/5, but I just\n> realized it, so I'm saying it here.\n> \nYes, I mentioned it in the cover letter(sigh, sorry!)\n\nI didn't use the mapped buffer directly because other functions required\na null terminated buffer to parse data part of loose object. It can be\nfixed but I don't want to make the patches too big.\n\nThe two big pros of uncompressed loose object are:\n\n*) avoid compressing and uncompressing loose objects    (I have implemented it)\n*) use memory mapped loose object directly              (I havn't implemented it)\n\n\nThank you for reviewing my patches, seems the idea to use uncompressed loose\nobject isn't attractive enough, I will keep the patches locally.\n\n\nBest regards,\n\nLiu Yubao\n"},{"id":"97123","messageId":"alpine.LFD.2.00.0812031949060.14328@xanadu.home","threadId":"16531","inReplyTo":"4934A5EC.2090708@gmail.com","subject":"Re: two questions about the format of loose object","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-12-04T00:54:26Z","receivedAt":"2008-12-04T00:54:26Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 2 Dec 2008, Liu Yubao wrote:\n\n> In fact the format I proposed in my patches is uncompressed loose\n> object, not uncompressed loose object header, that's to say I\n> proposed format 2 in my question 2, I am just curious why the\n> loose object header is compressed in question 1.\n> \n> I did a test to add all files of git-1.6.1-rc1 with git-add, the\n> time spent decreased by half. Other commands like git diff,\n> git diff --cached, git diff HEAD~ HEAD should be faster now\n> although the change may be not noticable for small and medium project.\n\nPlease try this with an unmodified git version:\n\n\tgit config --global core.loosecompression 0\n\nand redo your tests please.\n\nOne thing that a purely uncompressed loose object format is missing is \nquick data integrity protection. With the above, you'll have all your \nloose objects uncompressed but they'll still have a CRC32 done over \nthem.\n\n\nNicolas\n"}]}