{"thread":{"id":"35368","subject":"corrupt object memory allocation error","startedAt":"2013-11-20T20:33:50Z","lastAt":"2013-11-27T19:03:19Z","messageCount":28,"participants":["Joey Hess","Jeff King","Duy Nguyen","Keshav Kini","Junio C Hamano","Christian Couder","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"230858","messageId":"20131120203350.GA31139@kitenet.net","threadId":"35368","inReplyTo":null,"subject":"corrupt object memory allocation error","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2013-11-20T20:33:50Z","receivedAt":"2013-11-20T20:33:50Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"I've got a git repository of < 2 mb, where git wants to\nallocate a rather insane amount of memory:\n\n>git fsck\nChecking object directories: 100% (256/256), done.\nfatal: Out of memory, malloc failed (tried to allocate 124865231165 bytes)\n\n> git show 11644b5a075dc1425e01fbba51c045cea2d0c408\nfatal: Out of memory, malloc failed (tried to allocate 124865231165 bytes)\n\nThe problem seems to be the attached object file, which has gotten\ncorrupted, presumably in the header that git reads to see how large it\nis. Thought I'd report this in case there is some easy way to\nadd a sanity check.\n\n-- \nsee shy jo\n"},{"id":"230861","messageId":"20131120213348.GA29004@sigill.intra.peff.net","threadId":"35368","inReplyTo":"20131120203350.GA31139@kitenet.net","subject":"Re: corrupt object memory allocation error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-20T21:33:48Z","receivedAt":"2013-11-20T21:33:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 20, 2013 at 04:33:50PM -0400, Joey Hess wrote:\n\n> I've got a git repository of < 2 mb, where git wants to\n> allocate a rather insane amount of memory:\n> \n> >git fsck\n> Checking object directories: 100% (256/256), done.\n> fatal: Out of memory, malloc failed (tried to allocate 124865231165 bytes)\n> \n> > git show 11644b5a075dc1425e01fbba51c045cea2d0c408\n> fatal: Out of memory, malloc failed (tried to allocate 124865231165 bytes)\n> \n> The problem seems to be the attached object file, which has gotten\n> corrupted, presumably in the header that git reads to see how large it\n> is. Thought I'd report this in case there is some easy way to\n> add a sanity check.\n\nDefinitely a corrupt object. The start is not a valid zlib header, so we\nguess that it is an \"experimental loose object\". This is a format that\ngit wrote for very short period as a performance experiment; it didn't\npan out and we no longer write it.\n\nThe loose object format contains the (purported) object size outside of\nthe checksum'd zlib data (whereas the normal format has a human-readable\nheader that gets zlib'd). Your corrupted bytes end up specifying a\nridiculously large size.\n\nI wonder if it is time to drop reading support for the experimental\nobjects. It was never widely used, and was deprecated in v1.5.2 by\n726f852 (deprecate the new loose object header format, 2007-05-09). That\nwould improve the case when the initial bytes of a loose object are\ncorrupted, because we would complain about the bogus zlib data before\ntrying to allocate the buffer.\n\nThe problem would still remain for packfiles, which use a similar\nencoding, but I suspect it is less common there. For a single-byte\ncorruption, it is unlikely to be right in the length header. But for\nabsolute junk that is not git data at all, the first bytes are very\nlikely to be corrupted. In the pack case, we would notice early that it\ndoes not look like a packfile; for the loose object, we have no such\nheader and proceed with the allocation.\n\nAs for your specific corruption, I can't make heads or tails of it. It\nis not a single-bit error. The first two bytes of a loose object should\nalways be <0x78, 0x01>, which is the standard zlib deflate header. Your\nbytes aren't even close, and decoding the rest with a corrupted zlib\nheader seems fruitless.\n\nYou don't happen to have another copy of the object (or of the data\ncontained in the object, such as the working tree file), do you? It\nmight be interesting to see a comparison of the bytes of the correct\ndata and your corruption.\n\n-Peff\n"},{"id":"230865","messageId":"20131120222805.GC26468@kitenet.net","threadId":"35368","inReplyTo":"20131120213348.GA29004@sigill.intra.peff.net","subject":"Re: corrupt object memory allocation error","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2013-11-20T22:28:06Z","receivedAt":"2013-11-20T22:28:06Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Jeff King wrote:\n> As for your specific corruption, I can't make heads or tails of it. It\n> is not a single-bit error.\n\nOh, I should have mentioned that I am generating corrupt git\nrepositories mechanically for testing. I think in this case it prepended\nsome garbage to an object.\n\n-- \nsee shy jo\n"},{"id":"230882","messageId":"20131121114157.GA7171@sigill.intra.peff.net","threadId":"35368","inReplyTo":"20131120222805.GC26468@kitenet.net","subject":"[PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-21T11:41:58Z","receivedAt":"2013-11-21T11:41:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 20, 2013 at 06:28:06PM -0400, Joey Hess wrote:\n\n> Jeff King wrote:\n> > As for your specific corruption, I can't make heads or tails of it. It\n> > is not a single-bit error.\n> \n> Oh, I should have mentioned that I am generating corrupt git\n> repositories mechanically for testing. I think in this case it prepended\n> some garbage to an object.\n\nAh. That explains a lot (yes, dropping the first 19 bytes of your\ncorrupted file recovers the object).\n\nI still think we should probably do this, though:\n\n-- >8 --\nSubject: drop support for \"experimental\" loose objects\n\nIn git v1.4.3, we introduced a new loose object format that\nencoded some object information outside of the zlib stream.\nUltimately the format was dropped in v1.5.3, but we kept the\nreading side around to help people migrate objects. Each\ntime we open a loose object, we use a heuristic to check\nwhether it is in the normal loose format or the\nexperimental one.\n\nThis heuristic is robust in the face of valid data, but it\ntends to treat corrupted or garbage data as an experimental\nobject. With the regular format, we would notice quickly\nthat zlib's crc does not check out and complain. With the\nexperimental object, we are likely to extract a nonsensical\nobject size and try to allocate a huge buffer, resulting in\nxmalloc calling \"die\".\n\nThis latter behavior is much worse for two reasons. One,\ngit reports an allocation error when the real error is\ncorruption. And two, the program dies unconditionally, so\nyou cannot even run fsck (which would otherwise ignore the\nbroken object and keep going).\n\nWe could try to improve the heuristic to err on the side of\nnormal objects in the face of corruption, but there is\nreally little point. The experimental format is long-dead,\nand was never enabled by default to begin with. We can\ninstead simply remove it. The only affected repository would\nbe one that explicitly set core.legacyheaders in 2007, and\nthen never repacked in the intervening 6 years.\n\nSigned-off-by: Jeff King \n---\nThe test objects removed are all binary. Git seems to guess a few as\nnon-binary, though, because they don't contain any NULs, and includes\ngross binary bytes in the patch below. In theory the mail's transfer\nencoding will take care of this. We'll see, I guess. :)\n\n sha1_file.c | 74 ---------------------\n t/t1013-loose-object-format.sh | 66 ------------------\n .../14/9cedb5c46929d18e0f118e9fa31927487af3b6 | Bin 117 -> 0 bytes\n .../16/56f9233d999f61ef23ef390b9c71d75399f435 | Bin 17 -> 0 bytes\n .../1e/72a6b2c4a577ab0338860fa9fe87f761fc9bbd | Bin 18 -> 0 bytes\n .../25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 | Bin 19 -> 0 bytes\n .../2e/65efe2a145dda7ee51d1741299f848e5bf752e | Bin 10 -> 0 bytes\n .../6b/aee0540ea990d9761a3eb9ab183003a71c3696 | Bin 181 -> 0 bytes\n .../70/e6a83d8dcb26fc8bc0cf702e2ddeb6adca18fd | Bin 26 -> 0 bytes\n .../76/e7fa9941f4d5f97f64fea65a2cba436bc79cbb | 2 -\n .../78/75c6237d3fcdd0ac2f0decc7d3fa6a50b66c09 | Bin 139 -> 0 bytes\n .../7a/37b887a73791d12d26c0d3e39568a8fb0fa6e8 | Bin 54 -> 0 bytes\n .../85/df50785d62d3b05ab03d9cbf7e4a0b49449730 | Bin 13 -> 0 bytes\n .../8d/4e360d6c70fbd72411991c02a09c442cf7a9fa | Bin 156 -> 0 bytes\n .../95/b1625de3ba8b2214d1e0d0591138aea733f64f | Bin 252 -> 0 bytes\n .../9a/e9e86b7bd6cb1472d9373702d8249973da0832 | Bin 11 -> 0 bytes\n .../bd/15045f6ce8ff75747562173640456a394412c8 | Bin 34 -> 0 bytes\n .../e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 | Bin 9 -> 0 bytes\n .../f8/16d5255855ac160652ee5253b06cd8ee14165a | 1 -\n 19 files changed, 143 deletions(-)\n delete mode 100755 t/t1013-loose-object-format.sh\n delete mode 100644 t/t1013/objects/14/9cedb5c46929d18e0f118e9fa31927487af3b6\n delete mode 100644 t/t1013/objects/16/56f9233d999f61ef23ef390b9c71d75399f435\n delete mode 100644 t/t1013/objects/1e/72a6b2c4a577ab0338860fa9fe87f761fc9bbd\n delete mode 100644 t/t1013/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99\n delete mode 100644 t/t1013/objects/2e/65efe2a145dda7ee51d1741299f848e5bf752e\n delete mode 100644 t/t1013/objects/6b/aee0540ea990d9761a3eb9ab183003a71c3696\n delete mode 100644 t/t1013/objects/70/e6a83d8dcb26fc8bc0cf702e2ddeb6adca18fd\n delete mode 100644 t/t1013/objects/76/e7fa9941f4d5f97f64fea65a2cba436bc79cbb\n delete mode 100644 t/t1013/objects/78/75c6237d3fcdd0ac2f0decc7d3fa6a50b66c09\n delete mode 100644 t/t1013/objects/7a/37b887a73791d12d26c0d3e39568a8fb0fa6e8\n delete mode 100644 t/t1013/objects/85/df50785d62d3b05ab03d9cbf7e4a0b49449730\n delete mode 100644 t/t1013/objects/8d/4e360d6c70fbd72411991c02a09c442cf7a9fa\n delete mode 100644 t/t1013/objects/95/b1625de3ba8b2214d1e0d0591138aea733f64f\n delete mode 100644 t/t1013/objects/9a/e9e86b7bd6cb1472d9373702d8249973da0832\n delete mode 100644 t/t1013/objects/bd/15045f6ce8ff75747562173640456a394412c8\n delete mode 100644 t/t1013/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391\n delete mode 100644 t/t1013/objects/f8/16d5255855ac160652ee5253b06cd8ee14165a\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7dadd04..a72fcb6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1442,51 +1442,6 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n \treturn map;\n }\n \n-/*\n- * There used to be a second loose object header format which\n- * was meant to mimic the in-pack format, allowing for direct\n- * copy of the object data. This format turned up not to be\n- * really worth it and we no longer write loose objects in that\n- * format.\n- */\n-static int experimental_loose_object(unsigned char *map)\n-{\n-\tunsigned int word;\n-\n-\t/*\n-\t * We must determine if the buffer contains the standard\n-\t * zlib-deflated stream or the experimental format based\n-\t * on the in-pack object format. Compare the header byte\n-\t * for each format:\n-\t *\n-\t * RFC1950 zlib w/ deflate : 0www1000 : 0 <= www <= 7\n-\t * Experimental pack-based : Stttssss : ttt = 1,2,3,4\n-\t *\n-\t * If bit 7 is clear and bits 0-3 equal 8, the buffer MUST be\n-\t * in standard loose-object format, UNLESS it is a Git-pack\n-\t * format object *exactly* 8 bytes in size when inflated.\n-\t *\n-\t * However, RFC1950 also specifies that the 1st 16-bit word\n-\t * must be divisible by 31 - this checksum tells us our buffer\n-\t * is in the standard format, giving a false positive only if\n-\t * the 1st word of the Git-pack format object happens to be\n-\t * divisible by 31, ie:\n-\t * ((byte0 * 256) + byte1) % 31 = 0\n-\t * => 0ttt10000www1000 % 31 = 0\n-\t *\n-\t * As it happens, this case can only arise for www=3 & ttt=1\n-\t * - ie, a Commit object, which would have to be 8 bytes in\n-\t * size. As no Commit can be that small, we find that the\n-\t * combination of these two criteria (bitmask & checksum)\n-\t * can always correctly determine the buffer format.\n-\t */\n-\tword = (map[0] << 8) + map[1];\n-\tif ((map[0] & 0x8F) == 0x08 && !(word % 31))\n-\t\treturn 0;\n-\telse\n-\t\treturn 1;\n-}\n-\n unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\tunsigned long len, enum object_type *type, unsigned long *sizep)\n {\n@@ -1514,14 +1469,6 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \n int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n {\n-\tunsigned long size, used;\n-\tstatic const char valid_loose_object_type[8] = {\n-\t\t0, /* OBJ_EXT */\n-\t\t1, 1, 1, 1, /* \"commit\", \"tree\", \"blob\", \"tag\" */\n-\t\t0, /* \"delta\" and others are invalid in a loose object */\n-\t};\n-\tenum object_type type;\n-\n \t/* Get the data stream */\n \tmemset(stream, 0, sizeof(*stream));\n \tstream->next_in = map;\n@@ -1529,27 +1476,6 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n \tstream->next_out = buffer;\n \tstream->avail_out = bufsiz;\n \n-\tif (experimental_loose_object(map)) {\n-\t\t/*\n-\t\t * The old experimental format we no longer produce;\n-\t\t * we can still read it.\n-\t\t */\n-\t\tused = unpack_object_header_buffer(map, mapsize, &type, &size);\n-\t\tif (!used || !valid_loose_object_type[type])\n-\t\t\treturn -1;\n-\t\tmap += used;\n-\t\tmapsize -= used;\n-\n-\t\t/* Set up the stream for the rest.. */\n-\t\tstream->next_in = map;\n-\t\tstream->avail_in = mapsize;\n-\t\tgit_inflate_init(stream);\n-\n-\t\t/* And generate the fake traditional header */\n-\t\tstream->total_out = 1 + snprintf(buffer, bufsiz, \"%s %lu\",\n-\t\t\t\t\t\t typename(type), size);\n-\t\treturn 0;\n-\t}\n \tgit_inflate_init(stream);\n \treturn git_inflate(stream, 0);\n }\ndiff --git a/t/t1013-loose-object-format.sh b/t/t1013-loose-object-format.sh\ndeleted file mode 100755\nindex fbf5f2f..0000000\n--- a/t/t1013-loose-object-format.sh\n+++ /dev/null\n@@ -1,66 +0,0 @@\n-#!/bin/sh\n-#\n-# Copyright (c) 2011 Roberto Tyley\n-#\n-\n-test_description='Correctly identify and parse loose object headers\n-\n-There are two file formats for loose objects - the original standard\n-format, and the experimental format introduced with Git v1.4.3, later\n-deprecated with v1.5.3. Although Git no longer writes the\n-experimental format, objects in both formats must be read, with the\n-format for a given file being determined by the header.\n-\n-Detecting file format based on header is not entirely trivial, not\n-least because the first byte of a zlib-deflated stream will vary\n-depending on how much memory was allocated for the deflation window\n-buffer when the object was written out (for example 4KB on Android,\n-rather that 32KB on a normal PC).\n-\n-The loose objects used as test vectors have been generated with the\n-following Git versions:\n-\n-standard format: Git v1.7.4.1\n-experimental format: Git v1.4.3 (legacyheaders=false)\n-standard format, deflated with 4KB window size: Agit/JGit on Android\n-'\n-\n-. ./test-lib.sh\n-\n-assert_blob_equals() {\n-\tprintf \"%s\" \"$2\" >expected &&\n-\tgit cat-file -p \"$1\" >actual &&\n-\ttest_cmp expected actual\n-}\n-\n-test_expect_success setup '\n-\tcp -R \"$TEST_DIRECTORY/t1013/objects\" .git/ &&\n-\tgit --version\n-'\n-\n-test_expect_success 'read standard-format loose objects' '\n-\tgit cat-file tag 8d4e360d6c70fbd72411991c02a09c442cf7a9fa &&\n-\tgit cat-file commit 6baee0540ea990d9761a3eb9ab183003a71c3696 &&\n-\tgit ls-tree 7a37b887a73791d12d26c0d3e39568a8fb0fa6e8 &&\n-\tassert_blob_equals \"257cc5642cb1a054f08cc83f2d943e56fd3ebe99\" \"foo$LF\"\n-'\n-\n-test_expect_success 'read experimental-format loose objects' '\n-\tgit cat-file tag 76e7fa9941f4d5f97f64fea65a2cba436bc79cbb &&\n-\tgit cat-file commit 7875c6237d3fcdd0ac2f0decc7d3fa6a50b66c09 &&\n-\tgit ls-tree 95b1625de3ba8b2214d1e0d0591138aea733f64f &&\n-\tassert_blob_equals \"2e65efe2a145dda7ee51d1741299f848e5bf752e\" \"a\" &&\n-\tassert_blob_equals \"9ae9e86b7bd6cb1472d9373702d8249973da0832\" \"ab\" &&\n-\tassert_blob_equals \"85df50785d62d3b05ab03d9cbf7e4a0b49449730\" \"abcd\" &&\n-\tassert_blob_equals \"1656f9233d999f61ef23ef390b9c71d75399f435\" \"abcdefgh\" &&\n-\tassert_blob_equals \"1e72a6b2c4a577ab0338860fa9fe87f761fc9bbd\" \"abcdefghi\" &&\n-\tassert_blob_equals \"70e6a83d8dcb26fc8bc0cf702e2ddeb6adca18fd\" \"abcdefghijklmnop\" &&\n-\tassert_blob_equals \"bd15045f6ce8ff75747562173640456a394412c8\" \"abcdefghijklmnopqrstuvwx\"\n-'\n-\n-test_expect_success 'read standard-format objects deflated with smaller window buffer' '\n-\tgit cat-file tag f816d5255855ac160652ee5253b06cd8ee14165a &&\n-\tgit cat-file tag 149cedb5c46929d18e0f118e9fa31927487af3b6\n-'\n-\n-test_done\ndiff --git a/t/t1013/objects/14/9cedb5c46929d18e0f118e9fa31927487af3b6 b/t/t1013/objects/14/9cedb5c46929d18e0f118e9fa31927487af3b6\ndeleted file mode 100644\nindex 472fd1458e03e47136416bce60d6e7c893a468ab..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 117\nzcmV-*0E+)ei51K-4#F@DKvCwL!aJ&DH^jjbLR`g3tWwmHs(9h{lzp+xy&xqlfV?lkEVv$I`1V\nX&;Ic{P^6Jl5+pbyA%^e+FOeitJb^w>\n\ndiff --git a/t/t1013/objects/16/56f9233d999f61ef23ef390b9c71d75399f435 b/t/t1013/objects/16/56f9233d999f61ef23ef390b9c71d75399f435\ndeleted file mode 100644\nindex c379d74ae2b40faae9c31cb7478bc7f42a6fb13c..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 17\nYcmcDhnB(o^<>%?^eV&1VkAYbg05K>8VgLXD\n\ndiff --git a/t/t1013/objects/1e/72a6b2c4a577ab0338860fa9fe87f761fc9bbd b/t/t1013/objects/1e/72a6b2c4a577ab0338860fa9fe87f761fc9bbd\ndeleted file mode 100644\nindex 93706305bcff060547181ee4b7d3a8583b691181..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 18\nZcmcDlnB(o^<>%?^ef|UsgJ2(X9{@cS1}Ojl\n\ndiff --git a/t/t1013/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 b/t/t1013/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99\ndeleted file mode 100644\nindex bdcf704c9e663f3a11b3146b1b455bc2581b4761..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 19\nacmbvg{1ON`X0$Bh6\n\ndiff --git a/t/t1013/objects/6b/aee0540ea990d9761a3eb9ab183003a71c3696 b/t/t1013/objects/6b/aee0540ea990d9761a3eb9ab183003a71c3696\ndeleted file mode 100644\nindex 3d2f0337dbb64c092b4a7e9bd324a66986cfe01a..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 181\nzcmV;m080OO0i}>jY6CG41+&&EdLIz_c+?s&#*jt!#uw=6?r{d&BO_}9zP*3BL6-Fv\nzMe(?t&r^etx{p>>0V(2;GZIGZz4#y@v6HB=?^32b4sN8R*<7gV+yFCnoI*s2Ba1lV\nzFgj7@=Rk=%H>8K4H?*{$QejsHt*yZRcG4TH>l9XD~D{Ff%bx2yqP#(RK6mab-}gIhvxgaY4w3o)h-EQ|!Y2\nM+U=VO05jJR92g)Lz5oCK\n\ndiff --git a/t/t1013/objects/85/df50785d62d3b05ab03d9cbf7e4a0b49449730 b/t/t1013/objects/85/df50785d62d3b05ab03d9cbf7e4a0b49449730\ndeleted file mode 100644\nindex 6dff746876aab1acb9b42c557cdda69164d68398..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 13\nUcmXr1nB(o^<;Tdte1owY030|2-~a#s\n\ndiff --git a/t/t1013/objects/8d/4e360d6c70fbd72411991c02a09c442cf7a9fa b/t/t1013/objects/8d/4e360d6c70fbd72411991c02a09c442cf7a9fa\ndeleted file mode 100644\nindex cb41e92d076c191245389bedfc0184ff78511c03..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 156\nzcmV;N0Av4n0VRw<3c@fD06pgwdly5tsfhs*Z{DRJ*tBb?+D6i?(BE72I0G|63DCPu\nzj(2VaTqI_*uMJZOrVHL7S&o4s9;`8zJhs*ar(}6Cw0RhMQL;WJp|PXV?QXdY^mB;|\nzTyx|i8JgwE3mnTIwS4iM<~8VP)NR)D;{<52a+SALfUQAelxip??qHt!F~Ox5c%$~Z\nK)~G%MG&zg-a!8y2\n\ndiff --git a/t/t1013/objects/95/b1625de3ba8b2214d1e0d0591138aea733f64f b/t/t1013/objects/95/b1625de3ba8b2214d1e0d0591138aea733f64f\ndeleted file mode 100644\nindex 7ac46b4f703aa00cd96c42971a237e3da5bbb5ca..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 252\nzcmVGFoC4YqUk*LiV!x=c5Ks>#dDO9iWuD_XYc$kOuF%oc6@EC02B\nzPy91`FH}uFREb{d`$r31?=F8Ac(Fui<`0jj`%CqpN{TZpN>Wqvz{(1qt+4Gqt@fw;\nz!1)3_-Fw^Co|5`fQ1IeT|&t}Bnaap*};@I(N)b$dQ6\nC6@X{}\n\ndiff --git a/t/t1013/objects/9a/e9e86b7bd6cb1472d9373702d8249973da0832 b/t/t1013/objects/9a/e9e86b7bd6cb1472d9373702d8249973da0832\ndeleted file mode 100644\nindex 9d8316d4e598e32ef17b3918cf00276c8c1bffba..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 11\nScmXr2nB(ok#K5S=a0CDn4+6^o\n\ndiff --git a/t/t1013/objects/bd/15045f6ce8ff75747562173640456a394412c8 b/t/t1013/objects/bd/15045f6ce8ff75747562173640456a394412c8\ndeleted file mode 100644\nindex eebf23956e3b8ac736fa04e682b4214ac75c716a..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 34\nqcmdnNSTV=j$IH*t*Zcg5GpEj-JbPMSLq|(bQ&)RE14GpTE?oczGYz%?\n\ndiff --git a/t/t1013/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 b/t/t1013/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391\ndeleted file mode 100644\nindex 134cf1937963c733d7affa6919f9835db864188a..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 9\nOcmXr0n8VBf1dIR)&;dyR\n\ndiff --git a/t/t1013/objects/f8/16d5255855ac160652ee5253b06cd8ee14165a b/t/t1013/objects/f8/16d5255855ac160652ee5253b06cd8ee14165a\ndeleted file mode 100644\nindex 26b75ae..0000000\n--- a/t/t1013/objects/f8/16d5255855ac160652ee5253b06cd8ee14165a\n+++ /dev/null\n@@ -1 +0,0 @@\n-H‰\u0015ÌÁ\u000e‚0\f€aÏ{ŠÞ\rI»e\u001d&Æø*¥\u001dˆ\u0001G°\u0017ß^¸ýù\u000e¿Ë\u0004Då҆wU‡Ò—¬\u001cS±4ª\u0019ŠÆ\u0011­ªž ,\u0019\u0007fÅ[ðßVAÛºÎ\u001eüxÈÇö6[wtG§Lu\u0007¸?—¦²¼Ú×\u001f@‰\"gì{†+\u0012b\by¾%M\n\\ No newline at end of file\n-- \n1.8.5.rc2.442.gbff39ff\n"},{"id":"230883","messageId":"20131121114837.GB7171@sigill.intra.peff.net","threadId":"35368","inReplyTo":"20131121114157.GA7171@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-21T11:48:37Z","receivedAt":"2013-11-21T11:48:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 21, 2013 at 06:41:58AM -0500, Jeff King wrote:\n\n> The test objects removed are all binary. Git seems to guess a few as\n> non-binary, though, because they don't contain any NULs, and includes\n> gross binary bytes in the patch below. In theory the mail's transfer\n> encoding will take care of this. We'll see, I guess. :)\n\nNope; something munged it along the way (I sent it as C-T-E: 8bit, but\nvger rewrites to QP before re-mailing, so that is the likely spot).\n\nHere's the same patch, but with this stuck into .git/info/attributes:\n\n /t/t1013/objects/*/* binary\n\nwhich should work.\n\n-- >8 --\nSubject: drop support for \"experimental\" loose objects\n\nIn git v1.4.3, we introduced a new loose object format that\nencoded some object information outside of the zlib stream.\nUltimately the format was dropped in v1.5.3, but we kept the\nreading side around to help people migrate objects. Each\ntime we open a loose object, we use a heuristic to check\nwhether it is in the normal loose format, or the\nexperimental one.\n\nThis heuristic is robust in the face of valid data, but it\ntends to treat corrupted or garbage data as an experimental\nobject. With the regular format, we would notice quickly\nthat zlib's crc does not check out and complain. With the\nexperimental object, we are likely to extract a nonsensical\nobject size and try to allocate a huge buffer, resulting in\nxmalloc calling \"die\".\n\nThis latter behavior is much worse, for two reasons. One,\ngit reports an allocation error when the real error is\ncorruption. And two, the program dies unconditionally, so\nyou cannot even run fsck (which would otherwise ignore the\nbroken object and keep going).\n\nWe could try to improve the heuristic to err on the side of\nnormal objects in the face of corruption, but there is\nreally little point. The experimental format is long-dead,\nand was never enabled by default to begin with. We can\ninstead simply remove it. The only affected repository would\nbe one that explicitly set core.legacyheaders in 2007, and\nthen never repacked in the intervening 6 years.\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 74 ---------------------\n t/t1013-loose-object-format.sh | 66 ------------------\n .../14/9cedb5c46929d18e0f118e9fa31927487af3b6 | Bin 117 -> 0 bytes\n .../16/56f9233d999f61ef23ef390b9c71d75399f435 | Bin 17 -> 0 bytes\n .../1e/72a6b2c4a577ab0338860fa9fe87f761fc9bbd | Bin 18 -> 0 bytes\n .../25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 | Bin 19 -> 0 bytes\n .../2e/65efe2a145dda7ee51d1741299f848e5bf752e | Bin 10 -> 0 bytes\n .../6b/aee0540ea990d9761a3eb9ab183003a71c3696 | Bin 181 -> 0 bytes\n .../70/e6a83d8dcb26fc8bc0cf702e2ddeb6adca18fd | Bin 26 -> 0 bytes\n .../76/e7fa9941f4d5f97f64fea65a2cba436bc79cbb | Bin 155 -> 0 bytes\n .../78/75c6237d3fcdd0ac2f0decc7d3fa6a50b66c09 | Bin 139 -> 0 bytes\n .../7a/37b887a73791d12d26c0d3e39568a8fb0fa6e8 | Bin 54 -> 0 bytes\n .../85/df50785d62d3b05ab03d9cbf7e4a0b49449730 | Bin 13 -> 0 bytes\n .../8d/4e360d6c70fbd72411991c02a09c442cf7a9fa | Bin 156 -> 0 bytes\n .../95/b1625de3ba8b2214d1e0d0591138aea733f64f | Bin 252 -> 0 bytes\n .../9a/e9e86b7bd6cb1472d9373702d8249973da0832 | Bin 11 -> 0 bytes\n .../bd/15045f6ce8ff75747562173640456a394412c8 | Bin 34 -> 0 bytes\n .../e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 | Bin 9 -> 0 bytes\n .../f8/16d5255855ac160652ee5253b06cd8ee14165a | Bin 116 -> 0 bytes\n 19 files changed, 140 deletions(-)\n delete mode 100755 t/t1013-loose-object-format.sh\n delete mode 100644 t/t1013/objects/14/9cedb5c46929d18e0f118e9fa31927487af3b6\n delete mode 100644 t/t1013/objects/16/56f9233d999f61ef23ef390b9c71d75399f435\n delete mode 100644 t/t1013/objects/1e/72a6b2c4a577ab0338860fa9fe87f761fc9bbd\n delete mode 100644 t/t1013/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99\n delete mode 100644 t/t1013/objects/2e/65efe2a145dda7ee51d1741299f848e5bf752e\n delete mode 100644 t/t1013/objects/6b/aee0540ea990d9761a3eb9ab183003a71c3696\n delete mode 100644 t/t1013/objects/70/e6a83d8dcb26fc8bc0cf702e2ddeb6adca18fd\n delete mode 100644 t/t1013/objects/76/e7fa9941f4d5f97f64fea65a2cba436bc79cbb\n delete mode 100644 t/t1013/objects/78/75c6237d3fcdd0ac2f0decc7d3fa6a50b66c09\n delete mode 100644 t/t1013/objects/7a/37b887a73791d12d26c0d3e39568a8fb0fa6e8\n delete mode 100644 t/t1013/objects/85/df50785d62d3b05ab03d9cbf7e4a0b49449730\n delete mode 100644 t/t1013/objects/8d/4e360d6c70fbd72411991c02a09c442cf7a9fa\n delete mode 100644 t/t1013/objects/95/b1625de3ba8b2214d1e0d0591138aea733f64f\n delete mode 100644 t/t1013/objects/9a/e9e86b7bd6cb1472d9373702d8249973da0832\n delete mode 100644 t/t1013/objects/bd/15045f6ce8ff75747562173640456a394412c8\n delete mode 100644 t/t1013/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391\n delete mode 100644 t/t1013/objects/f8/16d5255855ac160652ee5253b06cd8ee14165a\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7dadd04..a72fcb6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1442,51 +1442,6 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n \treturn map;\n }\n \n-/*\n- * There used to be a second loose object header format which\n- * was meant to mimic the in-pack format, allowing for direct\n- * copy of the object data. This format turned up not to be\n- * really worth it and we no longer write loose objects in that\n- * format.\n- */\n-static int experimental_loose_object(unsigned char *map)\n-{\n-\tunsigned int word;\n-\n-\t/*\n-\t * We must determine if the buffer contains the standard\n-\t * zlib-deflated stream or the experimental format based\n-\t * on the in-pack object format. Compare the header byte\n-\t * for each format:\n-\t *\n-\t * RFC1950 zlib w/ deflate : 0www1000 : 0 <= www <= 7\n-\t * Experimental pack-based : Stttssss : ttt = 1,2,3,4\n-\t *\n-\t * If bit 7 is clear and bits 0-3 equal 8, the buffer MUST be\n-\t * in standard loose-object format, UNLESS it is a Git-pack\n-\t * format object *exactly* 8 bytes in size when inflated.\n-\t *\n-\t * However, RFC1950 also specifies that the 1st 16-bit word\n-\t * must be divisible by 31 - this checksum tells us our buffer\n-\t * is in the standard format, giving a false positive only if\n-\t * the 1st word of the Git-pack format object happens to be\n-\t * divisible by 31, ie:\n-\t * ((byte0 * 256) + byte1) % 31 = 0\n-\t * => 0ttt10000www1000 % 31 = 0\n-\t *\n-\t * As it happens, this case can only arise for www=3 & ttt=1\n-\t * - ie, a Commit object, which would have to be 8 bytes in\n-\t * size. As no Commit can be that small, we find that the\n-\t * combination of these two criteria (bitmask & checksum)\n-\t * can always correctly determine the buffer format.\n-\t */\n-\tword = (map[0] << 8) + map[1];\n-\tif ((map[0] & 0x8F) == 0x08 && !(word % 31))\n-\t\treturn 0;\n-\telse\n-\t\treturn 1;\n-}\n-\n unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\tunsigned long len, enum object_type *type, unsigned long *sizep)\n {\n@@ -1514,14 +1469,6 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \n int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n {\n-\tunsigned long size, used;\n-\tstatic const char valid_loose_object_type[8] = {\n-\t\t0, /* OBJ_EXT */\n-\t\t1, 1, 1, 1, /* \"commit\", \"tree\", \"blob\", \"tag\" */\n-\t\t0, /* \"delta\" and others are invalid in a loose object */\n-\t};\n-\tenum object_type type;\n-\n \t/* Get the data stream */\n \tmemset(stream, 0, sizeof(*stream));\n \tstream->next_in = map;\n@@ -1529,27 +1476,6 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n \tstream->next_out = buffer;\n \tstream->avail_out = bufsiz;\n \n-\tif (experimental_loose_object(map)) {\n-\t\t/*\n-\t\t * The old experimental format we no longer produce;\n-\t\t * we can still read it.\n-\t\t */\n-\t\tused = unpack_object_header_buffer(map, mapsize, &type, &size);\n-\t\tif (!used || !valid_loose_object_type[type])\n-\t\t\treturn -1;\n-\t\tmap += used;\n-\t\tmapsize -= used;\n-\n-\t\t/* Set up the stream for the rest.. */\n-\t\tstream->next_in = map;\n-\t\tstream->avail_in = mapsize;\n-\t\tgit_inflate_init(stream);\n-\n-\t\t/* And generate the fake traditional header */\n-\t\tstream->total_out = 1 + snprintf(buffer, bufsiz, \"%s %lu\",\n-\t\t\t\t\t\t typename(type), size);\n-\t\treturn 0;\n-\t}\n \tgit_inflate_init(stream);\n \treturn git_inflate(stream, 0);\n }\ndiff --git a/t/t1013-loose-object-format.sh b/t/t1013-loose-object-format.sh\ndeleted file mode 100755\nindex fbf5f2f..0000000\n--- a/t/t1013-loose-object-format.sh\n+++ /dev/null\n@@ -1,66 +0,0 @@\n-#!/bin/sh\n-#\n-# Copyright (c) 2011 Roberto Tyley\n-#\n-\n-test_description='Correctly identify and parse loose object headers\n-\n-There are two file formats for loose objects - the original standard\n-format, and the experimental format introduced with Git v1.4.3, later\n-deprecated with v1.5.3. Although Git no longer writes the\n-experimental format, objects in both formats must be read, with the\n-format for a given file being determined by the header.\n-\n-Detecting file format based on header is not entirely trivial, not\n-least because the first byte of a zlib-deflated stream will vary\n-depending on how much memory was allocated for the deflation window\n-buffer when the object was written out (for example 4KB on Android,\n-rather that 32KB on a normal PC).\n-\n-The loose objects used as test vectors have been generated with the\n-following Git versions:\n-\n-standard format: Git v1.7.4.1\n-experimental format: Git v1.4.3 (legacyheaders=false)\n-standard format, deflated with 4KB window size: Agit/JGit on Android\n-'\n-\n-. ./test-lib.sh\n-\n-assert_blob_equals() {\n-\tprintf \"%s\" \"$2\" >expected &&\n-\tgit cat-file -p \"$1\" >actual &&\n-\ttest_cmp expected actual\n-}\n-\n-test_expect_success setup '\n-\tcp -R \"$TEST_DIRECTORY/t1013/objects\" .git/ &&\n-\tgit --version\n-'\n-\n-test_expect_success 'read standard-format loose objects' '\n-\tgit cat-file tag 8d4e360d6c70fbd72411991c02a09c442cf7a9fa &&\n-\tgit cat-file commit 6baee0540ea990d9761a3eb9ab183003a71c3696 &&\n-\tgit ls-tree 7a37b887a73791d12d26c0d3e39568a8fb0fa6e8 &&\n-\tassert_blob_equals \"257cc5642cb1a054f08cc83f2d943e56fd3ebe99\" \"foo$LF\"\n-'\n-\n-test_expect_success 'read experimental-format loose objects' '\n-\tgit cat-file tag 76e7fa9941f4d5f97f64fea65a2cba436bc79cbb &&\n-\tgit cat-file commit 7875c6237d3fcdd0ac2f0decc7d3fa6a50b66c09 &&\n-\tgit ls-tree 95b1625de3ba8b2214d1e0d0591138aea733f64f &&\n-\tassert_blob_equals \"2e65efe2a145dda7ee51d1741299f848e5bf752e\" \"a\" &&\n-\tassert_blob_equals \"9ae9e86b7bd6cb1472d9373702d8249973da0832\" \"ab\" &&\n-\tassert_blob_equals \"85df50785d62d3b05ab03d9cbf7e4a0b49449730\" \"abcd\" &&\n-\tassert_blob_equals \"1656f9233d999f61ef23ef390b9c71d75399f435\" \"abcdefgh\" &&\n-\tassert_blob_equals \"1e72a6b2c4a577ab0338860fa9fe87f761fc9bbd\" \"abcdefghi\" &&\n-\tassert_blob_equals \"70e6a83d8dcb26fc8bc0cf702e2ddeb6adca18fd\" \"abcdefghijklmnop\" &&\n-\tassert_blob_equals \"bd15045f6ce8ff75747562173640456a394412c8\" \"abcdefghijklmnopqrstuvwx\"\n-'\n-\n-test_expect_success 'read standard-format objects deflated with smaller window buffer' '\n-\tgit cat-file tag f816d5255855ac160652ee5253b06cd8ee14165a &&\n-\tgit cat-file tag 149cedb5c46929d18e0f118e9fa31927487af3b6\n-'\n-\n-test_done\ndiff --git a/t/t1013/objects/14/9cedb5c46929d18e0f118e9fa31927487af3b6 b/t/t1013/objects/14/9cedb5c46929d18e0f118e9fa31927487af3b6\ndeleted file mode 100644\nindex 472fd1458e03e47136416bce60d6e7c893a468ab..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 117\nzcmV-*0E+)ei51K-4#F@DKvCwL!aJ&DH^jjbLR`g3tWwmHs(9h{lzp+xy&xqlfV?lkEVv$I`1V\nX&;Ic{P^6Jl5+pbyA%^e+FOeitJb^w>\n\ndiff --git a/t/t1013/objects/16/56f9233d999f61ef23ef390b9c71d75399f435 b/t/t1013/objects/16/56f9233d999f61ef23ef390b9c71d75399f435\ndeleted file mode 100644\nindex c379d74ae2b40faae9c31cb7478bc7f42a6fb13c..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 17\nYcmcDhnB(o^<>%?^eV&1VkAYbg05K>8VgLXD\n\ndiff --git a/t/t1013/objects/1e/72a6b2c4a577ab0338860fa9fe87f761fc9bbd b/t/t1013/objects/1e/72a6b2c4a577ab0338860fa9fe87f761fc9bbd\ndeleted file mode 100644\nindex 93706305bcff060547181ee4b7d3a8583b691181..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 18\nZcmcDlnB(o^<>%?^ef|UsgJ2(X9{@cS1}Ojl\n\ndiff --git a/t/t1013/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 b/t/t1013/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99\ndeleted file mode 100644\nindex bdcf704c9e663f3a11b3146b1b455bc2581b4761..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 19\nacmbvg{1ON`X0$Bh6\n\ndiff --git a/t/t1013/objects/6b/aee0540ea990d9761a3eb9ab183003a71c3696 b/t/t1013/objects/6b/aee0540ea990d9761a3eb9ab183003a71c3696\ndeleted file mode 100644\nindex 3d2f0337dbb64c092b4a7e9bd324a66986cfe01a..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 181\nzcmV;m080OO0i}>jY6CG41+&&EdLIz_c+?s&#*jt!#uw=6?r{d&BO_}9zP*3BL6-Fv\nzMe(?t&r^etx{p>>0V(2;GZIGZz4#y@v6HB=?^32b4sN8R*<7gV+yFCnoI*s2Ba1lV\nzFgj7@=Rk=%H>8K4H?*{$QejsHt*yZRcG4TH>lA>Ez2O4GbZDxSl3I-1-k{6>7C#LS\nzrUGr(He|JFof*kFg``L2m}m#I*r>r;QYTTig@ICxp@@PW__J^hk>`TbaZEYl&pl_j\nzr-5@x&~FoOaL)gfWzVZ$F}r}Xq$Jnp1u9c%tLsj8a8Q*}LiGE^!TJibhg&G{u4FBZ\nJ_yWO9IpM14O;`W`\n\ndiff --git a/t/t1013/objects/78/75c6237d3fcdd0ac2f0decc7d3fa6a50b66c09 b/t/t1013/objects/78/75c6237d3fcdd0ac2f0decc7d3fa6a50b66c09\ndeleted file mode 100644\nindex 3dd28be5c61840c23486bc654213c493c5387d75..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 139\nzcmV;60CfM64S1ZT%u5QwFc1LHeNHiZA!PEi1rc|y+=v&LG*cUF4TP!E+lzPvmv8f=\nzF+(2`MjJA_L|w8LeMUCfgdWj##I$#AjDA$K%2XR%YvLvqZrjWo9NLdszC7JmYPrx;\nt4^^*^BcMYYt&hRN&Y&@BsLN7B_}@oeC^Ni^OmHp&FVtQ;^#LMbK`4x{Jre)`\n\ndiff --git a/t/t1013/objects/7a/37b887a73791d12d26c0d3e39568a8fb0fa6e8 b/t/t1013/objects/7a/37b887a73791d12d26c0d3e39568a8fb0fa6e8\ndeleted file mode 100644\nindex 2b97b264c33f78fe6c8230b5bbeacd6409d9f963..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 54\nzcmV-60LlM&0V^p=O;s>9XD~D{Ff%bx2yqP#(RK6mab-}gIhvxgaY4w3o)h-EQ|!Y2\nM+U=VO05jJR92g)Lz5oCK\n\ndiff --git a/t/t1013/objects/85/df50785d62d3b05ab03d9cbf7e4a0b49449730 b/t/t1013/objects/85/df50785d62d3b05ab03d9cbf7e4a0b49449730\ndeleted file mode 100644\nindex 6dff746876aab1acb9b42c557cdda69164d68398..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 13\nUcmXr1nB(o^<;Tdte1owY030|2-~a#s\n\ndiff --git a/t/t1013/objects/8d/4e360d6c70fbd72411991c02a09c442cf7a9fa b/t/t1013/objects/8d/4e360d6c70fbd72411991c02a09c442cf7a9fa\ndeleted file mode 100644\nindex cb41e92d076c191245389bedfc0184ff78511c03..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 156\nzcmV;N0Av4n0VRw<3c@fD06pgwdly5tsfhs*Z{DRJ*tBb?+D6i?(BE72I0G|63DCPu\nzj(2VaTqI_*uMJZOrVHL7S&o4s9;`8zJhs*ar(}6Cw0RhMQL;WJp|PXV?QXdY^mB;|\nzTyx|i8JgwE3mnTIwS4iM<~8VP)NR)D;{<52a+SALfUQAelxip??qHt!F~Ox5c%$~Z\nK)~G%MG&zg-a!8y2\n\ndiff --git a/t/t1013/objects/95/b1625de3ba8b2214d1e0d0591138aea733f64f b/t/t1013/objects/95/b1625de3ba8b2214d1e0d0591138aea733f64f\ndeleted file mode 100644\nindex 7ac46b4f703aa00cd96c42971a237e3da5bbb5ca..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 252\nzcmVGFoC4YqUk*LiV!x=c5Ks>#dDO9iWuD_XYc$kOuF%oc6@EC02B\nzPy91`FH}uFREb{d`$r31?=F8Ac(Fui<`0jj`%CqpN{TZpN>Wqvz{(1qt+4Gqt@fw;\nz!1)3_-Fw^Co|5`fQ1IeT|&t}Bnaap*};@I(N)b$dQ6\nC6@X{}\n\ndiff --git a/t/t1013/objects/9a/e9e86b7bd6cb1472d9373702d8249973da0832 b/t/t1013/objects/9a/e9e86b7bd6cb1472d9373702d8249973da0832\ndeleted file mode 100644\nindex 9d8316d4e598e32ef17b3918cf00276c8c1bffba..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 11\nScmXr2nB(ok#K5S=a0CDn4+6^o\n\ndiff --git a/t/t1013/objects/bd/15045f6ce8ff75747562173640456a394412c8 b/t/t1013/objects/bd/15045f6ce8ff75747562173640456a394412c8\ndeleted file mode 100644\nindex eebf23956e3b8ac736fa04e682b4214ac75c716a..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 34\nqcmdnNSTV=j$IH*t*Zcg5GpEj-JbPMSLq|(bQ&)RE14GpTE?oczGYz%?\n\ndiff --git a/t/t1013/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 b/t/t1013/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391\ndeleted file mode 100644\nindex 134cf1937963c733d7affa6919f9835db864188a..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 9\nOcmXr0n8VBf1dIR)&;dyR\n\ndiff --git a/t/t1013/objects/f8/16d5255855ac160652ee5253b06cd8ee14165a b/t/t1013/objects/f8/16d5255855ac160652ee5253b06cd8ee14165a\ndeleted file mode 100644\nindex 26b75aec56f8f178a9f001be3b630a4e48ceb6b9..0000000000000000000000000000000000000000\nGIT binary patch\nliteral 0\nHcmV?d00001\n\nliteral 116\nzcmV-)0E_=fi51Mj4uUWYfML&jirx)LyJa0F#`r3w9f$!(uovH6xc&JKzsm$f< wrote:\n> @@ -1514,14 +1469,6 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n>\n> int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n> {\n> - unsigned long size, used;\n> - static const char valid_loose_object_type[8] = {\n> - 0, /* OBJ_EXT */\n> - 1, 1, 1, 1, /* \"commit\", \"tree\", \"blob\", \"tag\" */\n> - 0, /* \"delta\" and others are invalid in a loose object */\n> - };\n> - enum object_type type;\n> -\n> /* Get the data stream */\n> memset(stream, 0, sizeof(*stream));\n> stream->next_in = map;\n> @@ -1529,27 +1476,6 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n> stream->next_out = buffer;\n> stream->avail_out = bufsiz;\n>\n> - if (experimental_loose_object(map)) {\n\nPerhaps keep this..\n\n> - /*\n> - * The old experimental format we no longer produce;\n> - * we can still read it.\n> - */\n> - used = unpack_object_header_buffer(map, mapsize, &type, &size);\n> - if (!used || !valid_loose_object_type[type])\n> - return -1;\n> - map += used;\n> - mapsize -= used;\n> -\n> - /* Set up the stream for the rest.. */\n> - stream->next_in = map;\n> - stream->avail_in = mapsize;\n> - git_inflate_init(stream);\n> -\n> - /* And generate the fake traditional header */\n> - stream->total_out = 1 + snprintf(buffer, bufsiz, \"%s %lu\",\n> - typename(type), size);\n> - return 0;\n\nand replace all this with\n\ndie(\"detected an object in obsolete format, please repack the\nrepository using a version before XXX\");\n\n?\n\n> - }\n> git_inflate_init(stream);\n> return git_inflate(stream, 0);\n> }\n-- \nDuy\n"},{"id":"230887","messageId":"87ppptolz2.fsf@gmail.com","threadId":"35368","inReplyTo":"CACsJy8B5xY1FZyhPdct8Nt6Gad2cveRvmOXTXJP=uCaG2_0KuA@mail.gmail.com","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Keshav Kini","fromEmail":"keshav.kini@gmail.com","sentAt":"2013-11-21T14:42:09Z","receivedAt":"2013-11-21T14:42:09Z","isPatch":true,"sender":{"key":"keshav.kini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/691290?v=4"},"body":"Duy Nguyen writes:\n\n> On Thu, Nov 21, 2013 at 6:48 PM, Jeff King wrote:\n>> @@ -1514,14 +1469,6 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n>>\n>> int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)\n>> {\n>> - unsigned long size, used;\n>> - static const char valid_loose_object_type[8] = {\n>> - 0, /* OBJ_EXT */\n>> - 1, 1, 1, 1, /* \"commit\", \"tree\", \"blob\", \"tag\" */\n>> - 0, /* \"delta\" and others are invalid in a loose object */\n>> - };\n>> - enum object_type type;\n>> -\n>> /* Get the data stream */\n>> memset(stream, 0, sizeof(*stream));\n>> stream->next_in = map;\n>> @@ -1529,27 +1476,6 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n>> stream->next_out = buffer;\n>> stream->avail_out = bufsiz;\n>>\n>> - if (experimental_loose_object(map)) {\n>\n> Perhaps keep this..\n>\n>> - /*\n>> - * The old experimental format we no longer produce;\n>> - * we can still read it.\n>> - */\n>> - used = unpack_object_header_buffer(map, mapsize, &type, &size);\n>> - if (!used || !valid_loose_object_type[type])\n>> - return -1;\n>> - map += used;\n>> - mapsize -= used;\n>> -\n>> - /* Set up the stream for the rest.. */\n>> - stream->next_in = map;\n>> - stream->avail_in = mapsize;\n>> - git_inflate_init(stream);\n>> -\n>> - /* And generate the fake traditional header */\n>> - stream->total_out = 1 + snprintf(buffer, bufsiz, \"%s %lu\",\n>> - typename(type), size);\n>> - return 0;\n>\n> and replace all this with\n>\n> die(\"detected an object in obsolete format, please repack the\n> repository using a version before XXX\");\n>\n> ?\n\nWouldn't that fail to solve the issue of `git fsck` dying on corrupt\ndata? experimental_loose_object() would need to be rewritten to be more\nconservative in deciding that an object was in the experimental loose\nobject format.\n\n-Keshav\n"},{"id":"230892","messageId":"20131121160426.GA21843@kitenet.net","threadId":"35368","inReplyTo":"20131121114157.GA7171@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2013-11-21T16:04:26Z","receivedAt":"2013-11-21T16:04:26Z","isPatch":true,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Jeff King wrote:\n> This latter behavior is much worse for two reasons. One,\n> git reports an allocation error when the real error is\n> corruption. And two, the program dies unconditionally, so\n> you cannot even run fsck (which would otherwise ignore the\n> broken object and keep going).\n\nBTW, I've also seen git cat-file --batch report wrong sizes for objects,\nsometimes without crashing. This is particularly problimatic because if\nthe object size is wrong, it's very hard to detect the actual end of the\nobject output in the batch mode stream.\n\n-- \nsee shy jo\n"},{"id":"230897","messageId":"xmqqhab54k0f.fsf@gitster.dls.corp.google.com","threadId":"35368","inReplyTo":"20131121114837.GB7171@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-21T19:44:48Z","receivedAt":"2013-11-21T19:44:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We could try to improve the heuristic to err on the side of\n> normal objects in the face of corruption, but there is\n> really little point. The experimental format is long-dead,\n> and was never enabled by default to begin with. We can\n> instead simply remove it. The only affected repository would\n> be one that explicitly set core.legacyheaders in 2007, and\n> then never repacked in the intervening 6 years.\n\nSounds sensible. Thanks.\n"},{"id":"230901","messageId":"CAP8UFD2S1HUDYLbmEGFqLcBFExuB0h7=gqwsQ0qjpMSc+YaXog@mail.gmail.com","threadId":"35368","inReplyTo":"20131121160426.GA21843@kitenet.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2013-11-21T20:19:25Z","receivedAt":"2013-11-21T20:19:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Nov 21, 2013 at 5:04 PM, Joey Hess wrote:\n>\n> BTW, I've also seen git cat-file --batch report wrong sizes for objects,\n> sometimes without crashing. This is particularly problimatic because if\n> the object size is wrong, it's very hard to detect the actual end of the\n> object output in the batch mode stream.\n\nYeah, I think it might report wrong size in case of replaced objects\nfor example.\nI looked at that following Junio's comment about the\nsha1_object_info() API, which,\nunlike read_sha1_file() API, does not interact with the \"replace\" mechanism:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/234023/\n\nI started to work on a patch about this but didn't take the time to\nfinish and post it.\n\nThanks,\nChristian.\n"},{"id":"230920","messageId":"20131121224151.GA11258@sigill.intra.peff.net","threadId":"35368","inReplyTo":"CACsJy8B5xY1FZyhPdct8Nt6Gad2cveRvmOXTXJP=uCaG2_0KuA@mail.gmail.com","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-21T22:41:51Z","receivedAt":"2013-11-21T22:41:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 21, 2013 at 07:43:03PM +0700, Duy Nguyen wrote:\n\n> > - if (experimental_loose_object(map)) {\n> \n> Perhaps keep this..\n> \n> > - /*\n> > - * The old experimental format we no longer produce;\n> > - * we can still read it.\n> > - */\n> > - used = unpack_object_header_buffer(map, mapsize, &type, &size);\n> > - if (!used || !valid_loose_object_type[type])\n> > - return -1;\n> > - map += used;\n> > - mapsize -= used;\n> > -\n> > - /* Set up the stream for the rest.. */\n> > - stream->next_in = map;\n> > - stream->avail_in = mapsize;\n> > - git_inflate_init(stream);\n> > -\n> > - /* And generate the fake traditional header */\n> > - stream->total_out = 1 + snprintf(buffer, bufsiz, \"%s %lu\",\n> > - typename(type), size);\n> > - return 0;\n> \n> and replace all this with\n> \n> die(\"detected an object in obsolete format, please repack the\n> repository using a version before XXX\");\n\nThat would eliminate the second part of my purpose, which is to not\ndie() on a corrupted object because we incorrectly guess that it is\nexperimental.\n\nIf we think these objects are in the wild, the right thing to do would\nbe to warn() and continue. But I really find it hard to believe any such\nobjects exist at this point.\n\n-Peff\n"},{"id":"230931","messageId":"20131122020911.GA12042@sigill.intra.peff.net","threadId":"35368","inReplyTo":"20131121160426.GA21843@kitenet.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-22T02:09:12Z","receivedAt":"2013-11-22T02:09:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 21, 2013 at 12:04:26PM -0400, Joey Hess wrote:\n\n> Jeff King wrote:\n> > This latter behavior is much worse for two reasons. One,\n> > git reports an allocation error when the real error is\n> > corruption. And two, the program dies unconditionally, so\n> > you cannot even run fsck (which would otherwise ignore the\n> > broken object and keep going).\n> \n> BTW, I've also seen git cat-file --batch report wrong sizes for objects,\n> sometimes without crashing. This is particularly problimatic because if\n> the object size is wrong, it's very hard to detect the actual end of the\n> object output in the batch mode stream.\n\nHrm. For --batch, I'd think we would open the whole object and notice\nthe corruption, even with the current code. But for --batch-check, we\nuse sha1_object_info, and for an \"experimental\" object, we do not need\nto de-zlib the object at all. So we end up reporting whatever crap we\ndecipher from the garbage bytes. My patch would fix that, as we would\nnot incorrectly guess an object is experimental anymore.\n\nIf you have specific cases that trigger even after my patch, I'd be\ninterested to see them.\n\n-Peff\n"},{"id":"230934","messageId":"20131122095801.GB12042@sigill.intra.peff.net","threadId":"35368","inReplyTo":"CAP8UFD2S1HUDYLbmEGFqLcBFExuB0h7=gqwsQ0qjpMSc+YaXog@mail.gmail.com","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-22T09:58:02Z","receivedAt":"2013-11-22T09:58:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 21, 2013 at 09:19:25PM +0100, Christian Couder wrote:\n\n> Yeah, I think it might report wrong size in case of replaced objects\n> for example.\n> I looked at that following Junio's comment about the\n> sha1_object_info() API, which,\n> unlike read_sha1_file() API, does not interact with the \"replace\" mechanism:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/234023/\n> \n> I started to work on a patch about this but didn't take the time to\n> finish and post it.\n\nThat seems kind of crazy. Would the fix be as simple as this:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 10676ba..a051d6c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2529,6 +2529,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \tstruct pack_entry e;\n \tint rtype;\n \n+\tsha1 = lookup_replace_object(sha1);\n+\n \tco = find_cached_object(sha1);\n \tif (co) {\n \t\tif (oi->typep)\n\nor do we need some way for callers to turn off replacement? I notice\nthat read_sha1_file has such a feature, but it is only used in one\nplace. I guess we would need to audit all the sha1_object_info callers.\n\n-Peff\n"},{"id":"230936","messageId":"CAP8UFD1fMTrJGo9Z4+jdWqc-=UmPG1jQjwTij4962WDoh_a1DA@mail.gmail.com","threadId":"35368","inReplyTo":"20131122095801.GB12042@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2013-11-22T11:04:01Z","receivedAt":"2013-11-22T11:04:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Nov 22, 2013 at 10:58 AM, Jeff King wrote:\n> On Thu, Nov 21, 2013 at 09:19:25PM +0100, Christian Couder wrote:\n>\n>> Yeah, I think it might report wrong size in case of replaced objects\n>> for example.\n>> I looked at that following Junio's comment about the\n>> sha1_object_info() API, which,\n>> unlike read_sha1_file() API, does not interact with the \"replace\" mechanism:\n>>\n>> http://thread.gmane.org/gmane.comp.version-control.git/234023/\n>>\n>> I started to work on a patch about this but didn't take the time to\n>> finish and post it.\n>\n> That seems kind of crazy. Would the fix be as simple as this:\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 10676ba..a051d6c 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2529,6 +2529,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n> struct pack_entry e;\n> int rtype;\n>\n> + sha1 = lookup_replace_object(sha1);\n> +\n> co = find_cached_object(sha1);\n> if (co) {\n> if (oi->typep)\n>\n> or do we need some way for callers to turn off replacement? I notice\n> that read_sha1_file has such a feature, but it is only used in one\n> place.\n\nYeah, indeed, I asked myself such a question and that's why it is not\nso simple unfortunately.\n\nIn \"sha1_file.c\", there is:\n\nvoid *read_sha1_file_extended(const unsigned char *sha1,\n enum object_type *type,\n unsigned long *size,\n unsigned flag)\n{\n void *data;\n char *path;\n const struct packed_git *p;\n const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)\n ? lookup_replace_object(sha1) : sha1;\n\n errno = 0;\n data = read_object(repl, type, size);\n...\n\nAnd in cache.h, there is:\n\n#define READ_SHA1_FILE_REPLACE 1\nstatic inline void *read_sha1_file(const unsigned char *sha1, enum\nobject_type *type, unsigned long *size)\n{\n return read_sha1_file_extended(sha1, type, size,\nREAD_SHA1_FILE_REPLACE);\n}\n\nSo the READ_SHA1_FILE_REPLACE is a way to disable replacement at compile time.\n\nBut in my opinion if we want such a knob, we should use it when we set\nthe \"read_replace_refs\" global variable.\nFor example with something like this:\n\ndiff --git a/environment.c b/environment.c\nindex 0a15349..7c99af8 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -44,7 +44,7 @@ const char *editor_program;\n const char *askpass_program;\n const char *excludes_file;\n enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;\n-int read_replace_refs = 1; /* NEEDSWORK: rename to use_replace_refs */\n+int read_replace_refs = READ_SHA1_FILE_REPLACE; /* NEEDSWORK: rename\nto use_replace_refs */\n enum eol core_eol = EOL_UNSET;\n enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n\n@Junio what would you think about such a change?\n\n> I guess we would need to audit all the sha1_object_info callers.\n\nYeah but when I looked at them, there were not many that looked dangerous.\n\nThanks,\nChristian.\n"},{"id":"230938","messageId":"20131122112429.GA16172@sigill.intra.peff.net","threadId":"35368","inReplyTo":"CAP8UFD1fMTrJGo9Z4+jdWqc-=UmPG1jQjwTij4962WDoh_a1DA@mail.gmail.com","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-22T11:24:29Z","receivedAt":"2013-11-22T11:24:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 22, 2013 at 12:04:01PM +0100, Christian Couder wrote:\n\n> In \"sha1_file.c\", there is:\n> \n> void *read_sha1_file_extended(const unsigned char *sha1,\n> enum object_type *type,\n> unsigned long *size,\n> unsigned flag)\n> {\n> void *data;\n> char *path;\n> const struct packed_git *p;\n> const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)\n> ? lookup_replace_object(sha1) : sha1;\n> \n> errno = 0;\n> data = read_object(repl, type, size);\n> ...\n> \n> And in cache.h, there is:\n> \n> #define READ_SHA1_FILE_REPLACE 1\n> static inline void *read_sha1_file(const unsigned char *sha1, enum\n> object_type *type, unsigned long *size)\n> {\n> return read_sha1_file_extended(sha1, type, size,\n> READ_SHA1_FILE_REPLACE);\n> }\n> \n> So the READ_SHA1_FILE_REPLACE is a way to disable replacement at compile time.\n\nIs it? I did not have the impression anyone would ever redefine\nREAD_SHA1_FILE_REPLACE at compile time, but that it was a flag that\nindividual callsites would pass to read_sha1_file_extended to tell them\nwhether they were interested in replacements. And the obvious reasons to\nnot be are:\n\n 1. You are doing some operation which needs real objects, like fsck or\n generating a packfile.\n\n 2. You have already resolved any replacements, and want to make sure\n you are getting the same object used elsewhere (e.g., because you\n already printed its size :) ).\n\nThe only site which calls read_sha1_file_extended directly and does not\npass the REPLACE flag is in streaming.c. And that looks to be a case of\n(2), since we resolve the replacement at the start in open_istream().\n\nI am kind of surprised we do not need to do so for (1) in places like\npack-objects.c. Most of that code does not use read_sha1_file,\npreferring instead to find the individual pack entries (for reuse). But\nthere are some calls to read_sha1_file, and I wonder if there is a bug\nlurking there.\n\n-Peff\n"},{"id":"230946","messageId":"CAP8UFD1z4NsmgzrnPmqHo7CkNRkgg24qT2SGnFjhjrzckdKoTQ@mail.gmail.com","threadId":"35368","inReplyTo":"20131122112429.GA16172@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2013-11-22T14:23:31Z","receivedAt":"2013-11-22T14:23:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Nov 22, 2013 at 12:24 PM, Jeff King wrote:\n> On Fri, Nov 22, 2013 at 12:04:01PM +0100, Christian Couder wrote:\n>\n>> In \"sha1_file.c\", there is:\n>>\n>> void *read_sha1_file_extended(const unsigned char *sha1,\n>> enum object_type *type,\n>> unsigned long *size,\n>> unsigned flag)\n>> {\n>> void *data;\n>> char *path;\n>> const struct packed_git *p;\n>> const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)\n>> ? lookup_replace_object(sha1) : sha1;\n>>\n>> errno = 0;\n>> data = read_object(repl, type, size);\n>> ...\n>>\n>> And in cache.h, there is:\n>>\n>> #define READ_SHA1_FILE_REPLACE 1\n>> static inline void *read_sha1_file(const unsigned char *sha1, enum\n>> object_type *type, unsigned long *size)\n>> {\n>> return read_sha1_file_extended(sha1, type, size,\n>> READ_SHA1_FILE_REPLACE);\n>> }\n>>\n>> So the READ_SHA1_FILE_REPLACE is a way to disable replacement at compile time.\n>\n> Is it? I did not have the impression anyone would ever redefine\n> READ_SHA1_FILE_REPLACE at compile time, but that it was a flag that\n> individual callsites would pass to read_sha1_file_extended to tell them\n> whether they were interested in replacements. And the obvious reasons to\n> not be are:\n>\n> 1. You are doing some operation which needs real objects, like fsck or\n> generating a packfile.\n>\n> 2. You have already resolved any replacements, and want to make sure\n> you are getting the same object used elsewhere (e.g., because you\n> already printed its size :) ).\n>\n> The only site which calls read_sha1_file_extended directly and does not\n> pass the REPLACE flag is in streaming.c. And that looks to be a case of\n> (2), since we resolve the replacement at the start in open_istream().\n\nYeah, you are right. Sorry for overlooking this.\n\nBut anyway it looks redundant to me to have both this REPLACE flag and\nthe read_replace_refs global variable, so I think a proper solution\nwould involve some significant refactoring.\n\nAnd if we decide to keep a REPLACE flag we might need to add one to\nsha1_object_info_extended() too.\n\nThanks,\nChristian.\n"},{"id":"230951","messageId":"20131122161558.GA4170@sigill.intra.peff.net","threadId":"35368","inReplyTo":"CAP8UFD1z4NsmgzrnPmqHo7CkNRkgg24qT2SGnFjhjrzckdKoTQ@mail.gmail.com","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-22T16:15:59Z","receivedAt":"2013-11-22T16:15:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 22, 2013 at 03:23:31PM +0100, Christian Couder wrote:\n\n> > The only site which calls read_sha1_file_extended directly and does not\n> > pass the REPLACE flag is in streaming.c. And that looks to be a case of\n> > (2), since we resolve the replacement at the start in open_istream().\n> \n> Yeah, you are right. Sorry for overlooking this.\n> \n> But anyway it looks redundant to me to have both this REPLACE flag and\n> the read_replace_refs global variable, so I think a proper solution\n> would involve some significant refactoring.\n\nI don't think it is redundant. The global variable is about \"does the\nwhole operation want the replace feature turned on\" and the flag is\nabout \"does this particular callsite want the replace featured turned\non\". We use the feature iff both are true.\n\nWe could implement the callsite flag by tweaking the global right before\nthe call to read_sha1_file, but then we would have to remember to turn\nit back on afterwards. If this were a language with dynamic scopes like\nPerl, that would be easy. But in C you have to remember to reset it in\nall code paths. :)\n\nIn some cases it does make sense to turn the feature off for a whole\ncommand (like pack-objects); using the global makes sense there. And\nindeed, we seem to do it already in things like fsck, index-pack, etc.\nSo that answers my question of why I did not see more of case (1) in my\nprevious email: they do not need per-callsite disabling, because they do\nit for the whole command.\n\n> And if we decide to keep a REPLACE flag we might need to add one to\n> sha1_object_info_extended() too.\n\nYes, but somebody needs to look at all of the callsites and decide which\nform they want. :)\n\nI did a brief skim, and the ones I noticed were:\n\n - several spots in index-pack, pack-objects, etc. But these are\n already covered by unsetting read_replace_refs.\n\n - replace_object looks at both the original and new object to compare\n their types (due to your recent patches); it would obviously want to\n get the true type of the original object\n\n - When creating tags and trees, we care about the type of the object\n (the former for the \"type\" line of the tag, the latter to set the\n mode). What should they do with replace objects? As above, it is\n probably insane to switch types, so it may not matter for practical\n purposes.\n\n - istream_source in streaming.c would probably want to turn it off for\n the same reason it uses read_sha1_file_extended\n\nSo I think most sites would be unaffected, but due to the second and\nfourth item in my list above, we would need a flag for\nsha1_object_info_extended.\n\n-Peff\n"},{"id":"230956","messageId":"xmqqli0g1hbf.fsf@gitster.dls.corp.google.com","threadId":"35368","inReplyTo":"20131122095801.GB12042@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-22T17:23:32Z","receivedAt":"2013-11-22T17:23:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I guess we would need to audit all the sha1_object_info callers.\n\nYup; I agree that was the conclusion of Christian's thread.\n"},{"id":"230958","messageId":"20131122172859.GA703@kitenet.net","threadId":"35368","inReplyTo":"20131122020911.GA12042@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2013-11-22T17:28:59Z","receivedAt":"2013-11-22T17:28:59Z","isPatch":true,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Jeff King wrote:\n> > BTW, I've also seen git cat-file --batch report wrong sizes for objects,\n> \n> Hrm. For --batch, I'd think we would open the whole object and notice\n> the corruption, even with the current code. But for --batch-check, we\n> use sha1_object_info, and for an \"experimental\" object, we do not need\n> to de-zlib the object at all. So we end up reporting whatever crap we\n> decipher from the garbage bytes. My patch would fix that, as we would\n> not incorrectly guess an object is experimental anymore.\n> \n> If you have specific cases that trigger even after my patch, I'd be\n> interested to see them.\n\nI was seeing it with --batch, not --batch-check. Probably only with the\nold experimental loose object format. In one case, --batch reported a\nsize of 20k, and only output 1k of data. With the object file I sent\nearlier, --batch reports a huge size, and fails trying to allocate the\nmemory for it before it can output anything.\n\nI also have seen at least once a corrupt pack file that caused git to try\nand allocate a absurd quantity of memory.\n\nUnfortunately I do not currently have exemplars for these, although I\nshould be able to run a less robust version of my code and find them\nagain. ;) Will try to find time to do that.\n\nBTW, the fuzzing code is here:\nhttp://source.git-repair.branchable.com/?p=source.git;a=blob;f=Git/Destroyer.hs\n\n-- \nsee shy jo\n"},{"id":"230981","messageId":"20131123002405.GK4212@google.com","threadId":"35368","inReplyTo":"20131121114837.GB7171@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-11-23T00:24:05Z","receivedAt":"2013-11-23T00:24:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> sha1_file.c | 74 ---------------------\n\nYay!\n\n> t/t1013-loose-object-format.sh | 66 ------------------\n\nHmm, not all of these tests are about the \"experimental\" format. Do\nwe really want to remove them all?\n\nThanks,\nJonathan\n"},{"id":"230982","messageId":"20131123003014.GA11012@sigill.intra.peff.net","threadId":"35368","inReplyTo":"20131123002405.GK4212@google.com","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-23T00:30:14Z","receivedAt":"2013-11-23T00:30:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 22, 2013 at 04:24:05PM -0800, Jonathan Nieder wrote:\n\n> > t/t1013-loose-object-format.sh | 66 ------------------\n> \n> Hmm, not all of these tests are about the \"experimental\" format. Do\n> we really want to remove them all?\n\nI think so. They were not all testing the experimental format, but they\nwere about making sure the is-it-experimental heuristic triggered\nproperly with various zlib settings.\n\nNow that we do not apply that heuristic, there is nothing (in git) to\ntest. We feed the contents straight to zlib. We could keep the objects\nwith small window size as a test, but we are not really testing git; we\nare testing zlib at that point.\n\n-Peff\n"},{"id":"230983","messageId":"20131123004754.GL4212@google.com","threadId":"35368","inReplyTo":"20131123003014.GA11012@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-11-23T00:47:54Z","receivedAt":"2013-11-23T00:47:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Fri, Nov 22, 2013 at 04:24:05PM -0800, Jonathan Nieder wrote:\n\n>>> t/t1013-loose-object-format.sh | 66 ------------------\n>>\n>> Hmm, not all of these tests are about the \"experimental\" format. Do\n>> we really want to remove them all?\n>\n> I think so. They were not all testing the experimental format, but they\n> were about making sure the is-it-experimental heuristic triggered\n> properly with various zlib settings.\n>\n> Now that we do not apply that heuristic, there is nothing (in git) to\n> test. We feed the contents straight to zlib.\n\nOk, makes sense.\n\nIn principle the tests are still useful as futureproofing in case git\nstarts to sanity-check the objects as a way to notice corruption\nearlier or something. But in practice, that kind of futureproofing is\nprobably not worth the extra tests to maintain.\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder \n"},{"id":"231009","messageId":"20131124084444.GA23238@sigill.intra.peff.net","threadId":"35368","inReplyTo":"20131122172859.GA703@kitenet.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-24T08:44:44Z","receivedAt":"2013-11-24T08:44:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 22, 2013 at 01:28:59PM -0400, Joey Hess wrote:\n\n> > Hrm. For --batch, I'd think we would open the whole object and notice\n> > the corruption, even with the current code. But for --batch-check, we\n> > use sha1_object_info, and for an \"experimental\" object, we do not need\n> > to de-zlib the object at all. So we end up reporting whatever crap we\n> > decipher from the garbage bytes. My patch would fix that, as we would\n> > not incorrectly guess an object is experimental anymore.\n> > \n> > If you have specific cases that trigger even after my patch, I'd be\n> > interested to see them.\n> \n> I was seeing it with --batch, not --batch-check. Probably only with the\n> old experimental loose object format. In one case, --batch reported a\n> size of 20k, and only output 1k of data. With the object file I sent\n> earlier, --batch reports a huge size, and fails trying to allocate the\n> memory for it before it can output anything.\n\nAh, yeah, that makes sense. We report the size via sha1_object_info,\nwhether we are going to output the object itself or not. So we might\nreport the bogus size, not noticing the corruption, and then hit an\nerror and bail when sending the object itself.\n\nMy patch makes that better in some cases, because we'll notice more\ncorruption when looking at the header of the object for\nsha1_object_info. But fundamentally, we may still hit an error while\noutputting the bytes. Reading the cat-file code, it looks like we should\nalways die if we hit an error, so at least a reader will get premature\nEOF (and not the beginning of another object).\n\nI can believe there is some specific corruption that yields a valid zlib\nstream that is a different size than the object advertises. Since\nv1.8.4, we double-check that the size we advertised matches what we are\nabout to write. But the streaming-blob code path does not include that\ncheck, so it might still be affected. It would be pretty easy and cheap\nto detect that case.\n\nIn any code path where we call parse_object, we double-check that the\nresult matches the sha1 we asked for. But low-level commands like\ncat-file just call read_sha1_file directly, and do not have such a\ncheck. We could add it, but I suspect the processing cost would be\nnoticeable.\n\n> I also have seen at least once a corrupt pack file that caused git to try\n> and allocate a absurd quantity of memory.\n\nI'm not surprised by that. The packfiles contain size information\noutside of the checksummed zlib data, and we pre-allocate the buffer\nbefore reading the zlib data. We could try to detect it, but then we are\nhard-coding the definition of \"absurd\". The current definition is \"we\nasked the OS for memory, and it did not give it to us\". :)\n\n-Peff\n"},{"id":"231011","messageId":"20131124090743.GA495@sigill.intra.peff.net","threadId":"35368","inReplyTo":"20131124084444.GA23238@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-24T09:07:43Z","receivedAt":"2013-11-24T09:07:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 24, 2013 at 03:44:44AM -0500, Jeff King wrote:\n\n> In any code path where we call parse_object, we double-check that the\n> result matches the sha1 we asked for. But low-level commands like\n> cat-file just call read_sha1_file directly, and do not have such a\n> check. We could add it, but I suspect the processing cost would be\n> noticeable.\n\nCurious, I tested this. It is noticeable. Here's the best-of-five\ntimings for the patch below when running a --batch cat-file on every\nobject in my git.git repo, using the patch below:\n\n [before]\n real 0m12.941s\n user 0m12.700s\n sys 0m0.244s\n\n [after]\n real 0m15.800s\n user 0m15.472s\n sys 0m0.344s\n\nSo it's about 20% slower. I don't know what the right tradeoff is. It's\ncool to check the data each time we look at it, but it does carry a\nperformance penalty. I notice elsewhere in git we are inconsistent. If\nyou call parse_object() on an object, you get the sha1 check. But if you\njust call parse_commit() on something you know to be a commit (e.g.,\nbecause you are traversing the history and looked it up as a parent\npointer), you do not. I don't know if that is oversight, or an\nintentional performance decision.\n\n-Peff\n\n---\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b2ca775..2b09773 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -199,6 +199,8 @@ static void print_object_or_die(int fd, const unsigned char *sha1,\n \tif (type == OBJ_BLOB) {\n \t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n+\t\tif (check_sha1_signature(sha1, NULL, 0, NULL) < 0)\n+\t\t\tdie(\"object %s sha1 mismatch\", sha1_to_hex(sha1));\n \t}\n \telse {\n \t\tenum object_type rtype;\n@@ -208,6 +210,8 @@ static void print_object_or_die(int fd, const unsigned char *sha1,\n \t\tcontents = read_sha1_file(sha1, &rtype, &rsize);\n \t\tif (!contents)\n \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n+\t\tif (check_sha1_signature(sha1, contents, rsize, typename(rtype)) < 0)\n+\t\t\tdie(\"object %s sha1 mismatch\", sha1_to_hex(sha1));\n \t\tif (rtype != type)\n \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n \t\tif (rsize != size)\n"},{"id":"231081","messageId":"xmqq7gbwz5w8.fsf@gitster.dls.corp.google.com","threadId":"35368","inReplyTo":"20131124090743.GA495@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-25T18:35:19Z","receivedAt":"2013-11-25T18:35:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Nov 24, 2013 at 03:44:44AM -0500, Jeff King wrote:\n>\n>> In any code path where we call parse_object, we double-check that the\n>> result matches the sha1 we asked for. But low-level commands like\n>> cat-file just call read_sha1_file directly, and do not have such a\n>> check. We could add it, but I suspect the processing cost would be\n>> noticeable.\n>\n> Curious, I tested this. It is noticeable. Here's the best-of-five\n> timings for the patch below when running a --batch cat-file on every\n> object in my git.git repo, using the patch below:\n>\n> [before]\n> real 0m12.941s\n> user 0m12.700s\n> sys 0m0.244s\n>\n> [after]\n> real 0m15.800s\n> user 0m15.472s\n> sys 0m0.344s\n>\n> So it's about 20% slower. I don't know what the right tradeoff is. It's\n> cool to check the data each time we look at it, but it does carry a\n> performance penalty.\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index b2ca775..2b09773 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -199,6 +199,8 @@ static void print_object_or_die(int fd, const unsigned char *sha1,\n> \tif (type == OBJ_BLOB) {\n> \t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n> \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n> +\t\tif (check_sha1_signature(sha1, NULL, 0, NULL) < 0)\n> +\t\t\tdie(\"object %s sha1 mismatch\", sha1_to_hex(sha1));\n\ncheck_sha1_signature() opens the object again and streams the data.\nEssentially the read side is doing twice the work with that patch,\nisn't it?\n\nI wonder if we want to extend the stream_blob_to_fd() API to\noptionally allow the caller to ask to validate that the returned\ndata is consistent with the object name the caller asked the data\nfor. Something along the lines of the attached weatherbaloon patch?\n\n builtin/fsck.c | 3 ++-\n entry.c | 2 +-\n streaming.c | 29 ++++++++++++++++++++++++++++-\n streaming.h | 4 +++-\n 4 files changed, 34 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 97ce678..f42ed9c 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -237,7 +237,8 @@ static void check_unreachable_object(struct object *obj)\n \t\t\tif (!(f = fopen(filename, \"w\")))\n \t\t\t\tdie_errno(\"Could not open '%s'\", filename);\n \t\t\tif (obj->type == OBJ_BLOB) {\n-\t\t\t\tif (stream_blob_to_fd(fileno(f), obj->sha1, NULL, 1))\n+\t\t\t\tif (stream_blob_to_fd(fileno(f), obj->sha1, NULL,\n+\t\t\t\t\t\t STREAMING_OUTPUT_CAN_SEEK))\n \t\t\t\t\tdie_errno(\"Could not write '%s'\", filename);\n \t\t\t} else\n \t\t\t\tfprintf(f, \"%s\\n\", sha1_to_hex(obj->sha1));\ndiff --git a/entry.c b/entry.c\nindex 7b7aa81..b3bc827 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -127,7 +127,7 @@ static int streaming_write_entry(const struct cache_entry *ce, char *path,\n \tif (fd < 0)\n \t\treturn -1;\n \n-\tresult |= stream_blob_to_fd(fd, ce->sha1, filter, 1);\n+\tresult |= stream_blob_to_fd(fd, ce->sha1, filter, STREAMING_OUTPUT_CAN_SEEK);\n \t*fstat_done = fstat_output(fd, state, statbuf);\n \tresult |= close(fd);\n \ndiff --git a/streaming.c b/streaming.c\nindex debe904..50599df 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -2,6 +2,7 @@\n * Copyright (c) 2011, Google Inc.\n */\n #include \"cache.h\"\n+#include \"object.h\"\n #include \"streaming.h\"\n \n enum input_source {\n@@ -496,19 +497,33 @@ static open_method_decl(incore)\n ****************************************************************/\n \n int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *filter,\n-\t\t int can_seek)\n+\t\t unsigned flags)\n {\n \tstruct git_istream *st;\n \tenum object_type type;\n \tunsigned long sz;\n \tssize_t kept = 0;\n \tint result = -1;\n+\tint can_seek = flags & STREAMING_OUTPUT_CAN_SEEK;\n+\n+\tint want_verify = flags & STREAMING_VERIFY_OBJECT_NAME;\n+\tgit_SHA_CTX ctx;\n \n \tst = open_istream(sha1, &type, &sz, filter);\n \tif (!st)\n \t\treturn result;\n \tif (type != OBJ_BLOB)\n \t\tgoto close_and_exit;\n+\n+\tif (want_verify) {\n+\t\tchar hdr[32];\n+\t\tint hdrlen;\n+\n+\t\tgit_SHA1_Init(&ctx);\n+\t\thdrlen = sprintf(hdr, \"%s %lu\", typename(type), sz) + 1;\n+\t\tgit_SHA1_Update(&ctx, hdr, hdrlen);\n+\t}\n+\n \tfor (;;) {\n \t\tchar buf[1024 * 16];\n \t\tssize_t wrote, holeto;\n@@ -518,6 +533,10 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f\n \t\t\tgoto close_and_exit;\n \t\tif (!readlen)\n \t\t\tbreak;\n+\n+\t\tif (want_verify)\n+\t\t\tgit_SHA1_Update(&ctx, buf, readlen);\n+\n \t\tif (can_seek && sizeof(buf) == readlen) {\n \t\t\tfor (holeto = 0; holeto < readlen; holeto++)\n \t\t\t\tif (buf[holeto])\n@@ -542,6 +561,14 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f\n \t\tgoto close_and_exit;\n \tresult = 0;\n \n+\tif (want_verify) {\n+\t\tunsigned char verify[20];\n+\n+\t\tgit_SHA1_Final(verify, &ctx);\n+\t\tif (hashcmp(verify, lookup_replace_object(sha1)))\n+\t\t\tresult = -1;\n+\t}\n+\n close_and_exit:\n \tclose_istream(st);\n \treturn result;\ndiff --git a/streaming.h b/streaming.h\nindex 1d05c2a..68fe3a4 100644\n--- a/streaming.h\n+++ b/streaming.h\n@@ -12,6 +12,8 @@ extern struct git_istream *open_istream(const unsigned char *, enum object_type\n extern int close_istream(struct git_istream *);\n extern ssize_t read_istream(struct git_istream *, void *, size_t);\n \n-extern int stream_blob_to_fd(int fd, const unsigned char *, struct stream_filter *, int can_seek);\n+#define STREAMING_OUTPUT_CAN_SEEK 01\n+#define STREAMING_VERIFY_OBJECT_NAME 02\n+extern int stream_blob_to_fd(int fd, const unsigned char *, struct stream_filter *, unsigned flags);\n \n #endif /* STREAMING_H */\n"},{"id":"231161","messageId":"20131127093043.GA23429@sigill.intra.peff.net","threadId":"35368","inReplyTo":"xmqq7gbwz5w8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-27T09:30:43Z","receivedAt":"2013-11-27T09:30:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 25, 2013 at 10:35:19AM -0800, Junio C Hamano wrote:\n\n> > \tif (type == OBJ_BLOB) {\n> > \t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n> > \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n> > +\t\tif (check_sha1_signature(sha1, NULL, 0, NULL) < 0)\n> > +\t\t\tdie(\"object %s sha1 mismatch\", sha1_to_hex(sha1));\n> \n> check_sha1_signature() opens the object again and streams the data.\n> Essentially the read side is doing twice the work with that patch,\n> isn't it?\n\nYes. I considered that, but I also got ~20% slow-down when just doing\ncommits/trees, which are in-core and can re-hash the same buffer. So\nsince my with-blobs numbers backed that up, I didn't think too much\nfurther on it.\n\nBut there is something curious about the numbers I posted. It takes 12s\nwithout the check, and 15s with the check. So the extra hashing adds 3s.\nBut if we are reading each blob twice, and we would expect blob reading\nto be a significant chunk of that 12s, then shouldn't we expect much\nmore than 3s increase?\n\nThe answer must be that either we are not streaming as much as I think,\nor re-reading the data is much cheaper than I expect. And I think it is\nthe latter.\n\nThe vast majority of blobs in git.git will be stored as packed deltas.\nThat means the streaming code will fall back to doing the regular\nin-core access. We _could_ therefore use that in-core copy to do our\nsha1 check rather than streaming; but of course we never get access to\nit outside of stream_blob_to_fd, and it is discarded. However, we do\nkeep a copy in the delta base cache. When we immediately ask to unpack\nthe exact same entry for check_sha1_signature, we can pull the copy\nstraight out of the cache without having to re-inflate the object.\n\nAfter applying the patch below on top of yours, my numbers remain the\nsame:\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b2ca775..e3ff677 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -197,7 +197,7 @@ static void print_object_or_die(int fd, const unsigned char *sha1,\n \t\t\t\tenum object_type type, unsigned long size)\n {\n \tif (type == OBJ_BLOB) {\n-\t\tif (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)\n+\t\tif (stream_blob_to_fd(fd, sha1, NULL, STREAMING_VERIFY_OBJECT_NAME) < 0)\n \t\t\tdie(\"unable to stream %s to stdout\", sha1_to_hex(sha1));\n \t}\n \telse {\n@@ -208,6 +208,8 @@ static void print_object_or_die(int fd, const unsigned char *sha1,\n \t\tcontents = read_sha1_file(sha1, &rtype, &rsize);\n \t\tif (!contents)\n \t\t\tdie(\"object %s disappeared\", sha1_to_hex(sha1));\n+\t\tif (check_sha1_signature(sha1, contents, rsize, typename(rtype)) < 0)\n+\t\t\tdie(\"object %s sha1 mismatch\", sha1_to_hex(sha1));\n \t\tif (rtype != type)\n \t\t\tdie(\"object %s changed type!?\", sha1_to_hex(sha1));\n \t\tif (rsize != size)\n\n> I wonder if we want to extend the stream_blob_to_fd() API to\n> optionally allow the caller to ask to validate that the returned\n> data is consistent with the object name the caller asked the data\n> for. Something along the lines of the attached weatherbaloon patch?\n\nYes, I think it is a reasonable addition to the streaming API. However,\nI do not think there are any callsites which would currently want it.\nAll of the current users of stream_blob_to_fd use read_sha1_file as\ntheir alternative, and not parse_object. So we are not verifying the\nsha1 in either case (we may want to change that, of course, but that is\na bigger decision than just trying to bring streaming and non-streaming\ncode-paths into parity).\n\nI also wondered if parse_object itself had problems with double-reading\nor failing to verify. But its use goes the opposite direction; it wants\nto verify the sha1 of the blob object, but it knows that it does not\nactually need the data. So it streams (as of 090ea12) to check the\nsignature, but then discards each buffer-full after hashing it.\n\n-Peff\n"},{"id":"231201","messageId":"xmqqeh61u0z9.fsf@gitster.dls.corp.google.com","threadId":"35368","inReplyTo":"20131127093043.GA23429@sigill.intra.peff.net","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-27T18:57:14Z","receivedAt":"2013-11-27T18:57:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The vast majority of blobs in git.git will be stored as packed deltas.\n> That means the streaming code will fall back to doing the regular\n> in-core access. We _could_ therefore use that in-core copy to do our\n> sha1 check rather than streaming; but of course we never get access to\n> it outside of stream_blob_to_fd, and it is discarded. However, we do\n> keep a copy in the delta base cache. When we immediately ask to unpack\n> the exact same entry for check_sha1_signature, we can pull the copy\n> straight out of the cache without having to re-inflate the object.\n\nOK, that explains the overhead of 20% that is lower than one would\nnaïvely expect. Thanks.\n\n> Yes, I think it is a reasonable addition to the streaming API. However,\n> I do not think there are any callsites which would currently want it.\n> All of the current users of stream_blob_to_fd use read_sha1_file as\n> their alternative, and not parse_object. So we are not verifying the\n> sha1 in either case (we may want to change that, of course, but that is\n> a bigger decision than just trying to bring streaming and non-streaming\n> code-paths into parity).\n\nTrue. I am not offhand sure if we want to make read_sha1_file() to\nrehash, but I agree that it is a question different from what we are\nasking in this discussion.\n\n> I also wondered if parse_object itself had problems with double-reading\n> or failing to verify. But its use goes the opposite direction; it wants\n> to verify the sha1 of the blob object, but it knows that it does not\n> actually need the data. So it streams (as of 090ea12) to check the\n> signature, but then discards each buffer-full after hashing it.\n>\n> -Peff\n"},{"id":"231202","messageId":"20131127190319.GA3540@sigill.intra.peff.net","threadId":"35368","inReplyTo":"xmqqeh61u0z9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] drop support for \"experimental\" loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-27T19:03:19Z","receivedAt":"2013-11-27T19:03:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 27, 2013 at 10:57:14AM -0800, Junio C Hamano wrote:\n\n> > Yes, I think it is a reasonable addition to the streaming API. However,\n> > I do not think there are any callsites which would currently want it.\n> > All of the current users of stream_blob_to_fd use read_sha1_file as\n> > their alternative, and not parse_object. So we are not verifying the\n> > sha1 in either case (we may want to change that, of course, but that is\n> > a bigger decision than just trying to bring streaming and non-streaming\n> > code-paths into parity).\n> \n> True. I am not offhand sure if we want to make read_sha1_file() to\n> rehash, but I agree that it is a question different from what we are\n> asking in this discussion.\n\nI'm torn on that. Having git verify everything all the time is kind of\ncool. But it _does_ have a performance impact, and the vast majority of\nthe time nothing got corrupted since the last time we looked at the\nobject. It seems like periodically running \"git fsck\" is a smarter way\nof doing periodic checks.\n\nWe already are careful when objects are coming into the repository, and\nI think that is a sensible boundary (and I am increasingly of the\nopinion that running with transfer.fsckobjects off is not a good idea).\n\nThe checks in parse_object seem hack-ish to me, because they catch some\nrandom subset of the times we access objects (e.g., calling parse_object\non a commit sha1 will check, but calling parse_commit on an unparsed\ncommit struct will not). If anything, I'd suggest moving the checking\ndown to read_sha1_file, which would add it fairly consistently\neverywhere, and then tying it to a config option (off for high\nperformance, on for slower-but-meticulous).\n\n-Peff\n"}]}