{"thread":{"id":"15842","subject":"fetch fails with a short read of received pack","startedAt":"2008-10-09T19:55:18Z","lastAt":"2008-10-10T14:54:26Z","messageCount":7,"participants":["Alex Riesen","Samuel Tardieu","Nicolas Pitre","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"92698","messageId":"20081009195518.GA1497@blimp.localhost","threadId":"15842","inReplyTo":null,"subject":"fetch fails with a short read of received pack","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-09T19:55:18Z","receivedAt":"2008-10-09T19:55:18Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Somehow I got my gnulib mirror to a strange state where I can't fetch\nanything from it:\n\n    $ cd gnulib\n    $ git fetch -f ../gnulib.git 'refs/heads/*:refs/remotes/origin/*'\n    remote: Counting objects: 2202, done.\n    remote: Compressing objects: 100% (633/633), done.\n    remote: Total 1769 (delta 1448), reused 1455 (delta 1134)\n    Receiving objects: 100% (1769/1769), 404.11 KiB, done.\n    fatal: premature end of pack file, 1745 bytes missing\n    fatal: index-pack failed\n\nThis is with current Shawn's master (Junio's master erroneously gives\nan error). Bisect points at ac0463ed44e859c84e354cd0ae47d9b5b124e112\n\n    pack-objects: use fixup_pack_header_footer()'s validation mode\n\n    When limiting the pack size, a new header has to be written to the\n    pack and a new SHA1 computed.  Make sure that the SHA1 of what is being\n    read back matches the SHA1 of what was written.\n\nThis and the diff make no sense to me (as to what could be broken).\nI made both repos available at:\n\n    The source, it unpacks in gitlib.git (as in example above)\n\thttp://vin-soft.org/~ftp-tmp/gnulib-broken-src.tar.bz2\n\n    The target, where the fetch in the example is run\n\thttp://vin-soft.org/~ftp-tmp/gnulib-broken-tgt.tar.bz2\n\nCould someone who actually understands the code please have a look?\n"},{"id":"92700","messageId":"2008-10-09-22-24-15+trackit+sam@rfc1149.net","threadId":"15842","inReplyTo":"20081009195518.GA1497@blimp.localhost","subject":"Re: fetch fails with a short read of received pack","fromName":"Samuel Tardieu","fromEmail":"sam@rfc1149.net","sentAt":"2008-10-09T20:24:15Z","receivedAt":"2008-10-09T20:24:15Z","isPatch":false,"sender":{"key":"sam@rfc1149.net","avatar":"https://avatars.githubusercontent.com/u/44656?v=4"},"body":">>>>> \"Alex\" == Alex Riesen <raa.lkml@gmail.com> writes:\n\nAlex> Somehow I got my gnulib mirror to a strange state where I can't\nAlex> fetch anything from it:\n\nYou're the third person to report this problem in three days (I was\nthe second one). I thought my repository or the upstream one (GCC) was\ncorrupted in some ways, but it looks like a general problem.\n\nAlex> fatal: premature end of pack file, 1745 bytes missing\nAlex> fatal: index-pack failed\n\nAlex> This is with current Shawn's master (Junio's master erroneously\nAlex> gives an error).\n\nThe error message has been fixed three days ago, it hasn't reached\nJunio's repository yet.\n\nAlex> Bisect points at ac0463ed44e859c84e354cd0ae47d9b5b124e112\n\nIncidentally, my first \"git fetch\" problem was around September 8 if I\nremember correctly, so this looks plausible to have had the bug around\nthe date of the commit you mention (Aug 29).\n\n Sam\n-- \nSamuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/\n"},{"id":"92702","messageId":"alpine.LFD.2.00.0810091629520.26244@xanadu.home","threadId":"15842","inReplyTo":"2008-10-09-22-24-15+trackit+sam@rfc1149.net","subject":"Re: fetch fails with a short read of received pack","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-09T20:31:06Z","receivedAt":"2008-10-09T20:31:06Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 9 Oct 2008, Samuel Tardieu wrote:\n\n> >>>>> \"Alex\" == Alex Riesen <raa.lkml@gmail.com> writes:\n> \n> Alex> Somehow I got my gnulib mirror to a strange state where I can't\n> Alex> fetch anything from it:\n> \n> You're the third person to report this problem in three days (I was\n> the second one). I thought my repository or the upstream one (GCC) was\n> corrupted in some ways, but it looks like a general problem.\n\nAlex has a good test case and his bissection points at me.\nSo I'm on it.\n\n\nNicolas\n"},{"id":"92726","messageId":"alpine.LFD.2.00.0810092140500.26244@xanadu.home","threadId":"15842","inReplyTo":"20081009195518.GA1497@blimp.localhost","subject":"[PATCH] fix pread()'s short read in index-pack","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-10T02:08:51Z","receivedAt":"2008-10-10T02:08:51Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"\nSince v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for \nits ability to compute a SHA1 on the written data.  This also provides \ndata buffering which, along with commit 92392b4a45, will confuse pread() \nwhenever an appended object is 1) freed due to memory pressure because \nof the depth-first delta processing, and 2) needed again because it has \nmany delta children, and 3) its data is still buffered by sha1write().\n\nLet's fix the issue by simply forcing cached data out when such an \nobject is written so it can be pread()'d at leisure.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n\n---\n\nOn Thu, 9 Oct 2008, Alex Riesen wrote:\n\n> Somehow I got my gnulib mirror to a strange state where I can't fetch\n> anything from it:\n> \n>     $ cd gnulib\n>     $ git fetch -f ../gnulib.git 'refs/heads/*:refs/remotes/origin/*'\n>     remote: Counting objects: 2202, done.\n>     remote: Compressing objects: 100% (633/633), done.\n>     remote: Total 1769 (delta 1448), reused 1455 (delta 1134)\n>     Receiving objects: 100% (1769/1769), 404.11 KiB, done.\n>     fatal: premature end of pack file, 1745 bytes missing\n>     fatal: index-pack failed\n> \n> This is with current Shawn's master (Junio's master erroneously gives\n> an error). Bisect points at ac0463ed44e859c84e354cd0ae47d9b5b124e112\n\nThanks for providing a good test data set.  As you can see above, this \nis something that is tricky to reproduce otherwise.  This patch is meant \nfor the maint branch but should be included in master as well.\n\ndiff --git a/csum-file.c b/csum-file.c\nindex bb70c75..3b3e090 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -11,7 +11,7 @@\n #include \"progress.h\"\n #include \"csum-file.h\"\n \n-static void sha1flush(struct sha1file *f, void *buf, unsigned int count)\n+static void flush(struct sha1file *f, void *buf, unsigned int count)\n {\n \tfor (;;) {\n \t\tint ret = xwrite(f->fd, buf, count);\n@@ -30,22 +30,28 @@ static void sha1flush(struct sha1file *f, void *buf, unsigned int count)\n \t}\n }\n \n-int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)\n+void sha1flush(struct sha1file *f)\n {\n-\tint fd;\n \tunsigned offset = f->offset;\n \n \tif (offset) {\n \t\tSHA1_Update(&f->ctx, f->buffer, offset);\n-\t\tsha1flush(f, f->buffer, offset);\n+\t\tflush(f, f->buffer, offset);\n \t\tf->offset = 0;\n \t}\n+}\n+\n+int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)\n+{\n+\tint fd;\n+\n+\tsha1flush(f);\n \tSHA1_Final(f->buffer, &f->ctx);\n \tif (result)\n \t\thashcpy(result, f->buffer);\n \tif (flags & (CSUM_CLOSE | CSUM_FSYNC)) {\n \t\t/* write checksum and close fd */\n-\t\tsha1flush(f, f->buffer, 20);\n+\t\tflush(f, f->buffer, 20);\n \t\tif (flags & CSUM_FSYNC)\n \t\t\tfsync_or_die(f->fd, f->name);\n \t\tif (close(f->fd))\n@@ -83,7 +89,7 @@ int sha1write(struct sha1file *f, void *buf, unsigned int count)\n \t\tleft -= nr;\n \t\tif (!left) {\n \t\t\tSHA1_Update(&f->ctx, data, offset);\n-\t\t\tsha1flush(f, data, offset);\n+\t\t\tflush(f, data, offset);\n \t\t\toffset = 0;\n \t\t}\n \t\tf->offset = offset;\ndiff --git a/csum-file.h b/csum-file.h\nindex 72c9487..01f13b5 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -24,6 +24,7 @@ extern struct sha1file *sha1fd(int fd, const char *name);\n extern struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp);\n extern int sha1close(struct sha1file *, unsigned char *, unsigned int);\n extern int sha1write(struct sha1file *, void *, unsigned int);\n+extern void sha1flush(struct sha1file *f);\n extern void crc32_begin(struct sha1file *);\n extern uint32_t crc32_end(struct sha1file *);\n \ndiff --git a/index-pack.c b/index-pack.c\nindex 530d820..ca72329 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -704,6 +704,7 @@ static struct object_entry *append_obj_to_pack(struct sha1file *f,\n \tobj[1].idx.offset = obj[0].idx.offset + n;\n \tobj[1].idx.offset += write_compressed(f, buf, size);\n \tobj[0].idx.crc32 = crc32_end(f);\n+\tsha1flush(f);\n \thashcpy(obj->idx.sha1, sha1);\n \treturn obj;\n }\n"},{"id":"92730","messageId":"81b0412b0810092352n2a9cb5d2m6a0550354af0b397@mail.gmail.com","threadId":"15842","inReplyTo":"alpine.LFD.2.00.0810092140500.26244@xanadu.home","subject":"Re: [PATCH] fix pread()'s short read in index-pack","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-10T06:52:40Z","receivedAt":"2008-10-10T06:52:40Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/10 Nicolas Pitre <nico@cam.org>:\n>\n> Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for\n> its ability to compute a SHA1 on the written data.  This also provides\n> data buffering which, along with commit 92392b4a45, will confuse pread()\n> whenever an appended object is 1) freed due to memory pressure because\n> of the depth-first delta processing, and 2) needed again because it has\n> many delta children, and 3) its data is still buffered by sha1write().\n\nSo I seem to have had luck not doing gc in either of source or target repos\n(I usually repack, than fetch. Especially this one repo, it's very active).\n\nBTW, I run into complications trying to test the fix on Shawn's\nmaster: conflicts,\nand my trivial resolution wasn't good enough.\n\nAside from that, I confirm the case fixed.\n"},{"id":"92753","messageId":"20081010141756.GB8203@spearce.org","threadId":"15842","inReplyTo":"81b0412b0810092352n2a9cb5d2m6a0550354af0b397@mail.gmail.com","subject":"Re: [PATCH] fix pread()'s short read in index-pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-10T14:17:56Z","receivedAt":"2008-10-10T14:17:56Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> wrote:\n> 2008/10/10 Nicolas Pitre <nico@cam.org>:\n> >\n> > Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for\n> > its ability to compute a SHA1 on the written data.  This also provides\n> > data buffering which, along with commit 92392b4a45, will confuse pread()\n> > whenever an appended object is 1) freed due to memory pressure because\n> > of the depth-first delta processing, and 2) needed again because it has\n> > many delta children, and 3) its data is still buffered by sha1write().\n> \n> BTW, I run into complications trying to test the fix on Shawn's\n> master: conflicts,\n> and my trivial resolution wasn't good enough.\n\nHuh.\n\nColor me confused.  What the heck was the base version?\n\nNear as I can tell Nico wrote this fix against a8032d12 (sha1write:\ndon't copy full sized buffers).  This is in Junio's master.\n\nBut the changes conflict with what is in Junio's maint.  So the\npatch doesn't apply cleanly due to the sha1flush function's\nsignature changing.\n\nAnd the changes conflict with 9126f009 (fix openssl headers\nconflicting with custom SHA1 implementations), which is in my\nmaster, and was written by Nico.\n\n;-)\n\nAnyway, its merged now.\n\n-- \nShawn.\n"},{"id":"92758","messageId":"alpine.LFD.2.00.0810101051280.26244@xanadu.home","threadId":"15842","inReplyTo":"20081010141756.GB8203@spearce.org","subject":"Re: [PATCH] fix pread()'s short read in index-pack","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-10T14:54:26Z","receivedAt":"2008-10-10T14:54:26Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 10 Oct 2008, Shawn O. Pearce wrote:\n\n> Alex Riesen <raa.lkml@gmail.com> wrote:\n> > 2008/10/10 Nicolas Pitre <nico@cam.org>:\n> > >\n> > > Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for\n> > > its ability to compute a SHA1 on the written data.  This also provides\n> > > data buffering which, along with commit 92392b4a45, will confuse pread()\n> > > whenever an appended object is 1) freed due to memory pressure because\n> > > of the depth-first delta processing, and 2) needed again because it has\n> > > many delta children, and 3) its data is still buffered by sha1write().\n> > \n> > BTW, I run into complications trying to test the fix on Shawn's\n> > master: conflicts,\n> > and my trivial resolution wasn't good enough.\n> \n> Huh.\n> \n> Color me confused.  What the heck was the base version?\n> \n> Near as I can tell Nico wrote this fix against a8032d12 (sha1write:\n> don't copy full sized buffers).  This is in Junio's master.\n> \n> But the changes conflict with what is in Junio's maint.  So the\n> patch doesn't apply cleanly due to the sha1flush function's\n> signature changing.\n> \n> And the changes conflict with 9126f009 (fix openssl headers\n> conflicting with custom SHA1 implementations), which is in my\n> master, and was written by Nico.\n> \n> ;-)\n\nWhatever.\n\nYour truly is still recovering from a knee surgery and therefore is \nliterally on drugs.\n\n> Anyway, its merged now.\n\nThanks.  The important thing is that it must be included in what would \nbe v1.6.0.3.\n\n\nNicolas\n"}]}