{"thread":{"id":"6338","subject":"tree corrupted on disk quota full","startedAt":"2007-01-11T12:57:26Z","lastAt":"2007-01-13T00:53:26Z","messageCount":24,"participants":["Michael S. Tsirkin","Andreas Ericsson","Andy Whitcroft","Linus Torvalds","Junio C Hamano","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"31470","messageId":"20070111125726.GJ1759@mellanox.co.il","threadId":"6338","inReplyTo":null,"subject":"tree corrupted on disk quota full","fromName":"Michael S. Tsirkin","fromEmail":"mst@mellanox.co.il","sentAt":"2007-01-11T12:57:26Z","receivedAt":"2007-01-11T12:57:26Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"I hit a quota limit on a disk my tree was on, but did not notice.\nDoing git pull on a tree seems to have corrupted it.\nNow I have:\n\n$ git-fsck-objects\nerror: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\nerror: c03590b581d51d5fa43adbef9415e935d0229412: object not found\nmissing tree 10147d79b2418168d9433067b6439971bd4f1261\nbroken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n              to    tree 10147d79b2418168d9433067b6439971bd4f1261\ndangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n\nThe tree can not be pulled into, or from.\n\n-- \nMST\n"},{"id":"31471","messageId":"45A63E59.7030009@op5.se","threadId":"6338","inReplyTo":"20070111125726.GJ1759@mellanox.co.il","subject":"Re: tree corrupted on disk quota full","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-01-11T13:40:41Z","receivedAt":"2007-01-11T13:40:41Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Michael S. Tsirkin wrote:\n> I hit a quota limit on a disk my tree was on, but did not notice.\n> Doing git pull on a tree seems to have corrupted it.\n> Now I have:\n> \n> $ git-fsck-objects\n> error: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n> error: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n> missing tree 10147d79b2418168d9433067b6439971bd4f1261\n> broken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n>               to    tree 10147d79b2418168d9433067b6439971bd4f1261\n> dangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n> \n> The tree can not be pulled into, or from.\n> \n\nCan you do a \"git rev-list\" on the commit pointing to this tree? If so, \nyou should be able to do \"git reset HEAD~1\" and re-do the fetch.\n\nOtoh, this is curious. Aren't tree- and blob-objects written to disk \nbefore the commit object pointing to them? If not, how can we claim to \nsupport atomic commits? I imagined things were written in this order\n\n\tblob -> tree -> commit\n\nseeing as the dependency chain goes the other way.\n\n\"git repair\" could easily be cooked up by finding the first complete \ncommit and resetting current HEAD and all branches pointing to it to \nthat first complete commit. Issuing a fresh \"fetch\" after that should \nautomagically fix everything for you.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"31472","messageId":"45A63F02.5010600@shadowen.org","threadId":"6338","inReplyTo":"45A63E59.7030009@op5.se","subject":"Re: tree corrupted on disk quota full","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2007-01-11T13:43:30Z","receivedAt":"2007-01-11T13:43:30Z","isPatch":false,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"Andreas Ericsson wrote:\n> Michael S. Tsirkin wrote:\n>> I hit a quota limit on a disk my tree was on, but did not notice.\n>> Doing git pull on a tree seems to have corrupted it.\n>> Now I have:\n>>\n>> $ git-fsck-objects\n>> error: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n>> error: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n>> missing tree 10147d79b2418168d9433067b6439971bd4f1261\n>> broken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n>>               to    tree 10147d79b2418168d9433067b6439971bd4f1261\n>> dangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n>>\n>> The tree can not be pulled into, or from.\n>>\n> \n> Can you do a \"git rev-list\" on the commit pointing to this tree? If so,\n> you should be able to do \"git reset HEAD~1\" and re-do the fetch.\n> \n> Otoh, this is curious. Aren't tree- and blob-objects written to disk\n> before the commit object pointing to them? If not, how can we claim to\n> support atomic commits? I imagined things were written in this order\n> \n>     blob -> tree -> commit\n> \n> seeing as the dependency chain goes the other way.\n> \n> \"git repair\" could easily be cooked up by finding the first complete\n> commit and resetting current HEAD and all branches pointing to it to\n> that first complete commit. Issuing a fresh \"fetch\" after that should\n> automagically fix everything for you.\n\nWell we do have some dubious code which can fail in the face of short\nwrites when the disk is full.  Those I believe are now plugged following\nmy short i/o series which is in master as far as I know.\n\nI wonder if we can cook up a test with a loop back mount on a tiny ramfs\nthat will fail in this way.\n\n-apw\n"},{"id":"31474","messageId":"20070111135918.GO1759@mellanox.co.il","threadId":"6338","inReplyTo":"45A63E59.7030009@op5.se","subject":"Re: tree corrupted on disk quota full","fromName":"Michael S. Tsirkin","fromEmail":"mst@mellanox.co.il","sentAt":"2007-01-11T13:59:18Z","receivedAt":"2007-01-11T13:59:18Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> > I hit a quota limit on a disk my tree was on, but did not notice.\n> > Doing git pull on a tree seems to have corrupted it.\n> > Now I have:\n> > \n> > $ git-fsck-objects\n> > error: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n> > error: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n> > missing tree 10147d79b2418168d9433067b6439971bd4f1261\n> > broken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n> >               to    tree 10147d79b2418168d9433067b6439971bd4f1261\n> > dangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n> > \n> > The tree can not be pulled into, or from.\n> > \n> \n> Can you do a \"git rev-list\" on the commit pointing to this tree?\n\nOn which commit?\n\n> If so, you should be able to do \"git reset HEAD~1\" and re-do the fetch.\n\n$ git reset --hard HEAD~1\nHEAD is now at 2d41bf8... Remove svn keywords\n$ git-fsck-objects\nerror: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\nerror: c03590b581d51d5fa43adbef9415e935d0229412: object not found\nmissing tree 10147d79b2418168d9433067b6439971bd4f1261\nbroken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n              to    tree 10147d79b2418168d9433067b6439971bd4f1261\ndangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n\n-- \nMST\n"},{"id":"31476","messageId":"45A64987.60200@op5.se","threadId":"6338","inReplyTo":"20070111135918.GO1759@mellanox.co.il","subject":"Re: tree corrupted on disk quota full","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-01-11T14:28:23Z","receivedAt":"2007-01-11T14:28:23Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Michael S. Tsirkin wrote:\n>>> I hit a quota limit on a disk my tree was on, but did not notice.\n>>> Doing git pull on a tree seems to have corrupted it.\n>>> Now I have:\n>>>\n>>> $ git-fsck-objects\n>>> error: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n>>> error: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n>>> missing tree 10147d79b2418168d9433067b6439971bd4f1261\n>>> broken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n>>>               to    tree 10147d79b2418168d9433067b6439971bd4f1261\n>>> dangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n>>>\n>>> The tree can not be pulled into, or from.\n>>>\n>> Can you do a \"git rev-list\" on the commit pointing to this tree?\n> \n> On which commit?\n> \n\n322a6c93ad86d2a151dd97a4c6b0e014a4893437\n\n>> If so, you should be able to do \"git reset HEAD~1\" and re-do the fetch.\n> \n> $ git reset --hard HEAD~1\n> HEAD is now at 2d41bf8... Remove svn keywords\n> $ git-fsck-objects\n> error: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n> error: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n> missing tree 10147d79b2418168d9433067b6439971bd4f1261\n> broken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n>               to    tree 10147d79b2418168d9433067b6439971bd4f1261\n> dangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n> \n\nHumm... Seems there are other problems here.\n\nTry\n\n$ git reset --hard 322a6c93ad86d2a151dd97a4c6b0e014a4893437~1\n$ git fsck-objects HEAD\n\nIf this tells you your repo is clean, you should be able to redo \nwhatever fetches you did on top of that to get a working repository \nagain. If you have objects that got half-written due to end-of-disk \nerrors, you'd have to do \"git prune\" after resetting.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"31484","messageId":"Pine.LNX.4.64.0701111023090.3594@woody.osdl.org","threadId":"6338","inReplyTo":"20070111125726.GJ1759@mellanox.co.il","subject":"Re: tree corrupted on disk quota full","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T18:31:05Z","receivedAt":"2007-01-11T18:31:05Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Michael S. Tsirkin wrote:\n>\n> I hit a quota limit on a disk my tree was on, but did not notice.\n> Doing git pull on a tree seems to have corrupted it.\n> Now I have:\n> \n> $ git-fsck-objects\n> error: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n> error: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n> missing tree 10147d79b2418168d9433067b6439971bd4f1261\n> broken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n>               to    tree 10147d79b2418168d9433067b6439971bd4f1261\n> dangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n> \n> The tree can not be pulled into, or from.\n\nThat's nasty, but something as simple a \"git reset --hard ORIG_HEAD\" \nshould fix it (of course, if the disk-full happened earlier, or you've \ndone some other reset or something else that over-write your ORIG_HEAD, \nyou'd need to find the most recent commit that wasn't broken).\n\nThe good news is that there's no way your old data got corrupted. You just \nneed to _find_ it (and normally ORIG_HEAD points to it, so it's trivial to \nfind).\n\nThe suggestion to use \"HEAD~1\" is *not* a good one, simply because the \npull (if it was a fast-forward) will not have HEAD~1 as your previous \nhead: it will depend on how many commits you pulled. If you had reflog \nenabled, a \"HEAD@{1}\" might have worked (or \"HEAD@{2}\" as you already did \nthe \"git reset\").\n\nThat said, clearly something didn't check the error return of a write() \ncall. Some of that got fixed up recently, so it might even be fixed in \ncurrent git already.\n\nThe most likely case (for a pull) is \"git-unpack-objects\", I guess.\n\n\t\tLinus\n"},{"id":"31485","messageId":"Pine.LNX.4.64.0701111109070.3594@woody.osdl.org","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111023090.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T19:19:36Z","receivedAt":"2007-01-11T19:19:36Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Linus Torvalds wrote:\n> \n> That said, clearly something didn't check the error return of a write() \n> call. Some of that got fixed up recently, so it might even be fixed in \n> current git already.\n\nI'm not convinced.\n\nThe \"write_in_full()\" logic is supposed to help people avoid problems, but \nit *still* returns a success for a partial write.\n\nExample of extreme breakage:\n\n\tstatic int write_buffer(int fd, const void *buf, size_t len)\n\t{\n\t        ssize_t size;\n\t\n\t        size = write_in_full(fd, buf, len);\n\t        if (!size)\n\t                return error(\"file write: disk full\");\n\t        if (size < 0)\n\t                return error(\"file write error (%s)\", strerror(errno));\n\t        return 0;\n\t}\n\nwhich is TOTAL GARBAGE, because the disk-full event might have happened in \nthe middle of the write, so \"write_in_full()\" might have returned 4096, \nfor example, even though the buffer length was much bigger.\n\nI personally think write_in_full() is totally mis-designed. If you are \nready to handle partial writes, you should use \"xwrite()\". If you're not \nready to handle partial writes, you should either use \"write_or_die()\", \n_or_ you should expect a partial write to at least return an error code \n(which is how \"write_buffer()\" works).\n\nBut that's not how write_in_full() actually works. Write-in-full does not \nreturn an error for a partial write, it returns the partial size.\n\nWhich is idiotic. It makes the function pointless. Just use xwrite() for \nthat.\n\nI would suggest perhaps a patch like this..  And then you _really_ can \njust do\n\n\tif (write_in_full(fd, buf, len) < 0)\n\t\tdie(\"Not going to work: %s\", strerror(errno));\n\nand be happy.\n\n\t\tLinus\n\n---\ndiff --git a/write_or_die.c b/write_or_die.c\nindex a119e1d..f95299a 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -37,15 +37,14 @@ int write_in_full(int fd, const void *buf, size_t count)\n {\n \tconst char *p = buf;\n \tssize_t total = 0;\n-\tssize_t written = 0;\n \n \twhile (count > 0) {\n-\t\twritten = xwrite(fd, p, count);\n-\t\tif (written <= 0) {\n-\t\t\tif (total)\n-\t\t\t\treturn total;\n-\t\t\telse\n-\t\t\t\treturn written;\n+\t\tsize_t written = xwrite(fd, p, count);\n+\t\tif (written < 0)\n+\t\t\treturn -1;\n+\t\tif (!written) {\n+\t\t\terrno = ENOSPC;\n+\t\t\treturn -1\n \t\t}\n \t\tcount -= written;\n \t\tp += written;\n"},{"id":"31486","messageId":"Pine.LNX.4.64.0701111202250.3594@woody.osdl.org","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111109070.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T20:03:27Z","receivedAt":"2007-01-11T20:03:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Linus Torvalds wrote:\n> \n> I would suggest perhaps a patch like this..\n\nExcept you need to add a semicolon after the return:\n\n> +\t\tif (!written) {\n> +\t\t\terrno = ENOSPC;\n> +\t\t\treturn -1\n\notherwise it looks fine. But I obviously didn't even compile-test the \npatch I sent out, so caveat emptor.\n\n\t\tLinus\n"},{"id":"31491","messageId":"7vtzyx2s6i.fsf@assigned-by-dhcp.cox.net","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111109070.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-11T21:02:13Z","receivedAt":"2007-01-11T21:02:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> I personally think write_in_full() is totally mis-designed. If you are \n> ready to handle partial writes, you should use \"xwrite()\". If you're not \n> ready to handle partial writes, you should either use \"write_or_die()\", \n> _or_ you should expect a partial write to at least return an error code \n> (which is how \"write_buffer()\" works).\n\n> But that's not how write_in_full() actually works. Write-in-full does not \n> return an error for a partial write, it returns the partial size.\n>\n> Which is idiotic. It makes the function pointless. Just use xwrite() for \n> that.\n\nI agree 100% with the above reasoning.\n"},{"id":"31492","messageId":"20070111211143.GG17999@mellanox.co.il","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111023090.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Michael S. Tsirkin","fromEmail":"mst@mellanox.co.il","sentAt":"2007-01-11T21:11:43Z","receivedAt":"2007-01-11T21:11:43Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> >\n> > I hit a quota limit on a disk my tree was on, but did not notice.\n> > Doing git pull on a tree seems to have corrupted it.\n> > Now I have:\n> > \n> > $ git-fsck-objects\n> > error: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n> > error: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n> > missing tree 10147d79b2418168d9433067b6439971bd4f1261\n> > broken link from  commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n> >               to    tree 10147d79b2418168d9433067b6439971bd4f1261\n> > dangling commit 322a6c93ad86d2a151dd97a4c6b0e014a4893437\n> > \n> > The tree can not be pulled into, or from.\n> \n> That's nasty, but something as simple a \"git reset --hard ORIG_HEAD\" \n> should fix it (of course, if the disk-full happened earlier, or you've \n> done some other reset or something else that over-write your ORIG_HEAD, \n> you'd need to find the most recent commit that wasn't broken).\n> \n> The good news is that there's no way your old data got corrupted. You just \n> need to _find_ it (and normally ORIG_HEAD points to it, so it's trivial to \n> find).\n\nI did lots of resets and they did not seems to help.\n\nThen, on a hunch, I just did git prune and\nit cleaned the tree so I can pull/push fine.\n\nHow come?\n\nmst@mst-lt:~/tmp/libmthca_bad$ git --version\ngit version 1.5.0.rc0.g244a7\nmst@mst-lt:~/tmp/libmthca_bad$ git pull ~/scm/libmthca/\nremote: Generating pack...\nremote: Done counting 18 objects.\nResult has 11 objects.\nremote: Deltifying 11 objects.\nremote:  100% (11/11) done\nremote: Total 11 (delta 8), reused 1 (delta 1)\nUnpacking 11 objects\nerror: failed to read delta-pack base object 4d4d30be967d3284cbf59afd4fba6ab536e295f5\nfatal: unpack-objects died with error code 1\nFetch failure: /home/mst/scm/libmthca/\nmst@mst-lt:~/tmp/libmthca_bad$ git prune\nmst@mst-lt:~/tmp/libmthca_bad$ git pull ~/scm/libmthca/\nremote: Generating pack...\nremote: Done counting 18 objects.\nResult has 11 objects.\nDeltifying 11 objects.\n 10remote: 0% (11/11) done\nTotal 11 (delta 8), reused 1 (delta 1)\nUnpacking 11 objects\n 100% (11/11) done\nUpdating 732a507..322a6c9\nFast forward\n Makefile.am      |   25 +++++++++++++++++--------\n configure.in     |   16 ++++++++++++++--\n debian/changelog |    4 +---\n mthca.driver     |    1 +\n src/mthca.c      |   23 ++++++++++++-----------\n 5 files changed, 45 insertions(+), 24 deletions(-)\n create mode 100644 mthca.driver\n\nSo it seems the tree was more or less fine (it just has some useless objects\nbecause of the disk full failure), the bug seems to be that pull/push fail.\n\nI made a backup of the bad tree so that if someone wants to play with it, here it is:\nhttp://www.openfabrics.org/~mst/libmthca_bad.tgz\n\n-- \nMST\n"},{"id":"31493","messageId":"45A6A97F.5080008@shadowen.org","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111109070.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2007-01-11T21:17:51Z","receivedAt":"2007-01-11T21:17:51Z","isPatch":false,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"Linus Torvalds wrote:\n> \n> On Thu, 11 Jan 2007, Linus Torvalds wrote:\n>> That said, clearly something didn't check the error return of a write() \n>> call. Some of that got fixed up recently, so it might even be fixed in \n>> current git already.\n> \n> I'm not convinced.\n> \n> The \"write_in_full()\" logic is supposed to help people avoid problems, but \n> it *still* returns a success for a partial write.\n> \n> Example of extreme breakage:\n> \n> \tstatic int write_buffer(int fd, const void *buf, size_t len)\n> \t{\n> \t        ssize_t size;\n> \t\n> \t        size = write_in_full(fd, buf, len);\n> \t        if (!size)\n> \t                return error(\"file write: disk full\");\n> \t        if (size < 0)\n> \t                return error(\"file write error (%s)\", strerror(errno));\n> \t        return 0;\n> \t}\n> \n> which is TOTAL GARBAGE, because the disk-full event might have happened in \n> the middle of the write, so \"write_in_full()\" might have returned 4096, \n> for example, even though the buffer length was much bigger.\n> \n> I personally think write_in_full() is totally mis-designed. If you are \n> ready to handle partial writes, you should use \"xwrite()\". If you're not \n> ready to handle partial writes, you should either use \"write_or_die()\", \n> _or_ you should expect a partial write to at least return an error code \n> (which is how \"write_buffer()\" works).\n> \n> But that's not how write_in_full() actually works. Write-in-full does not \n> return an error for a partial write, it returns the partial size.\n> \n> Which is idiotic. It makes the function pointless. Just use xwrite() for \n> that.\n\nThe call was intended to replace a common idiom:\n\nif (xwrite(fd, buf, size) != size)\n\terror\n\nwrite_in_full() is intended as a drop in replacement for that.  On a\nshort write we will return a short count and that will fail such a test.\n The call basically implements the standard write() call semantic with\nmaximum attempts to complete.\n\nThat said returning -1 would have the same effect.\n\n-apw\n"},{"id":"31494","messageId":"7vodp52qzz.fsf@assigned-by-dhcp.cox.net","threadId":"6338","inReplyTo":"45A6A97F.5080008@shadowen.org","subject":"Re: tree corrupted on disk quota full","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-11T21:27:44Z","receivedAt":"2007-01-11T21:27:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Whitcroft <apw@shadowen.org> writes:\n\n> The call was intended to replace a common idiom:\n>\n> if (xwrite(fd, buf, size) != size)\n> \terror\n>\n> write_in_full() is intended as a drop in replacement for that.  On a\n> short write we will return a short count and that will fail such a test.\n> The call basically implements the standard write() call semantic with\n> maximum attempts to complete.\n\nYou are both right ;-).  I would have these fixups on top of\nLinus's.\n\n---\n\ndiff --git a/config.c b/config.c\nindex 2cd0263..733fb1a 100644\n--- a/config.c\n+++ b/config.c\n@@ -694,11 +694,9 @@ int git_config_set_multivar(const char* key, const char* value,\n \n \t\tstore.key = (char*)key;\n \t\tif (!store_write_section(fd, key) ||\n-\t\t    !store_write_pair(fd, key, value)) {\n-\t\t\tret = write_error();\n-\t\t\tgoto out_free;\n-\t\t}\n-\t} else{\n+\t\t    !store_write_pair(fd, key, value))\n+\t\t\tgoto write_err_out;\n+\t} else {\n \t\tstruct stat st;\n \t\tchar* contents;\n \t\tint i, copy_begin, copy_end, new_line = 0;\n@@ -777,31 +775,33 @@ int git_config_set_multivar(const char* key, const char* value,\n \n \t\t\t/* write the first part of the config */\n \t\t\tif (copy_end > copy_begin) {\n-\t\t\t\twrite_in_full(fd, contents + copy_begin,\n-\t\t\t\tcopy_end - copy_begin);\n-\t\t\t\tif (new_line)\n-\t\t\t\t\twrite_in_full(fd, \"\\n\", 1);\n+\t\t\t\tif (write_in_full(fd, contents + copy_begin,\n+\t\t\t\t\t\t  copy_end - copy_begin) <\n+\t\t\t\t    copy_end - copy_begin)\n+\t\t\t\t\tgoto write_err_out;\n+\t\t\t\tif (new_line &&\n+\t\t\t\t    write_in_full(fd, \"\\n\", 1) != 1)\n+\t\t\t\t\tgoto write_err_out;\n \t\t\t}\n \t\t\tcopy_begin = store.offset[i];\n \t\t}\n \n \t\t/* write the pair (value == NULL means unset) */\n \t\tif (value != NULL) {\n-\t\t\tif (store.state == START)\n-\t\t\t\tif (!store_write_section(fd, key)) {\n-\t\t\t\t\tret = write_error();\n-\t\t\t\t\tgoto out_free;\n-\t\t\t\t}\n-\t\t\tif (!store_write_pair(fd, key, value)) {\n-\t\t\t\tret = write_error();\n-\t\t\t\tgoto out_free;\n+\t\t\tif (store.state == START) {\n+\t\t\t\tif (!store_write_section(fd, key))\n+\t\t\t\t\tgoto write_err_out;\n \t\t\t}\n+\t\t\tif (!store_write_pair(fd, key, value))\n+\t\t\t\tgoto write_err_out;\n \t\t}\n \n \t\t/* write the rest of the config */\n \t\tif (copy_begin < st.st_size)\n-\t\t\twrite_in_full(fd, contents + copy_begin,\n-\t\t\t\tst.st_size - copy_begin);\n+\t\t\tif (write_in_full(fd, contents + copy_begin,\n+\t\t\t\t\t  st.st_size - copy_begin) <\n+\t\t\t    st.st_size - copy_begin)\n+\t\t\t\tgoto write_err_out;\n \n \t\tmunmap(contents, st.st_size);\n \t\tunlink(config_filename);\n@@ -824,6 +824,11 @@ out_free:\n \t\tfree(lock_file);\n \t}\n \treturn ret;\n+\n+write_err_out:\n+\tret = write_error();\n+\tgoto out_free;\n+\n }\n \n int git_config_rename_section(const char *old_name, const char *new_name)\ndiff --git a/index-pack.c b/index-pack.c\nindex 8d10d6b..72e0962 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -814,7 +814,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\tchar buf[48];\n \t\tint len = snprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n \t\t\t\t   report, sha1_to_hex(sha1));\n-\t\twrite_in_full(1, buf, len);\n+\t\twrite_or_die(1, buf, len);\n \n \t\t/*\n \t\t * Let's just mimic git-unpack-objects here and write\n"},{"id":"31499","messageId":"Pine.LNX.4.64.0701111324350.3594@woody.osdl.org","threadId":"6338","inReplyTo":"20070111211143.GG17999@mellanox.co.il","subject":"Re: tree corrupted on disk quota full","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T21:50:13Z","receivedAt":"2007-01-11T21:50:13Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Michael S. Tsirkin wrote:\n> \n> I did lots of resets and they did not seems to help.\n\nNOTE! If \"nothing seems to help\" means \"git-fsck-objects\" still complains, \nthen that is very much expected.\n\n\"git reset\" does NOT fix a corrupted database. Never has, never will. It \nshould only fix the case where a branch head points to a corrupt or \nincomplete tree.\n\nTo actually fix the database, you need to remove the broken objects. Which \n\"git prune\" did for you (but you could have just done it by hand too).\n\n> Then, on a hunch, I just did git prune and\n> it cleaned the tree so I can pull/push fine.\n\nIf so, your branch heads had _already_ been fixed (by one of the resets), \nand \"git prune\" just got rid of the (unreachable) objects that were \nbroken.\n\n> How come?\n\nWhat happens is:\n\n - you had gotten some corrupt objects due to the disk filling up \n   (probably during the pull that thus populated the object database with\n   partially written objects)\n\n   In particular, the 4d4d30be967d3284cbf59afd4fba6ab536e295f5 object was \n   corrupt. fsck gave a confusing error message:\n\n\terror: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n\terror: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n\n   which is really because the _file_ for that object does exist, but the \n   file doesn't actually contain the object it expects (due to \n   corruption), so the object wasn't \"found\". \n\n - git-fsck-objects complained about them, but it doesn't actually do \n   anything about it (which you maybe expected it to do - it doesn't \n   really act as a filesystem fsck which tries to _fix_ things, it really \n   just does the \"check\" part, since \"fixing\" things is almost always a \n   manual operation)\n\n - \"git prune\" actually removed all the unreachable objects, and since \n   none of the _reachable_ objects were broken, that makes \n   git-fsck-objects shut up too.\n\n - your \"git pull\" failed, because it was fetching objects that were \n   corrupt in your local database - and the rule is that local objects \n   ALWAYS override remote objects. That's an important security thing (a \n   \"pull\" is _never_ allowed to overwrite something you already have, and \n   it doesn't matter if it's corrupt or not, you're not ever going to have \n   a \"git pull\" overwriting old data)\n\n> So it seems the tree was more or less fine (it just has some useless objects\n> because of the disk full failure), the bug seems to be that pull/push fail.\n\nNot a bug. See above. It's really a very important safety feature: we \nnever EVER overwrite any old objects. Even if they are corrupt. But it \nmeans that you need to _fix_ any corruption in order to use the \nrepository. \n\nThat's a security feature. It's a feature because git absolutely ALWAYS \nconsiders the \"local repository\" to be the trustworthy one. It matters not \na whit if git thinks a local object may be corrupt - the local copy is \n_still_ considered to always be the definite copy. It's part of what makes \ngit stable.\n\nYou may not think that sounds \"stable\" to you right now, in that it made \nit harder to \"fix\" the problem, but it indirectly means that even a \n_buggy_ git binary (say, you by mistake use an older git binary that \ndoesn't understand some new object format) will never overwrite data that \nyou already had. That's true safety. \n\nIt does mean that if you have a corrupt database, you really do need to \nfix it first, but it's _still_ a safety feature: git won't fix the \ndatabase for you \"by mistake\". You need to fix it consciously.\n\nThat's true robustness.\n\n\t\t\tLinus\n"},{"id":"31501","messageId":"20070111215812.GK17999@mellanox.co.il","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111324350.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Michael S. Tsirkin","fromEmail":"mst@mellanox.co.il","sentAt":"2007-01-11T21:58:12Z","receivedAt":"2007-01-11T21:58:12Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":">  - your \"git pull\" failed, because it was fetching objects that were \n>    corrupt in your local database - and the rule is that local objects \n>    ALWAYS override remote objects. That's an important security thing (a \n>    \"pull\" is _never_ allowed to overwrite something you already have, and \n>    it doesn't matter if it's corrupt or not, you're not ever going to have \n>    a \"git pull\" overwriting old data)\n\n...\n\n> That's true robustness.\n> \n> \t\t\tLinus\n\nOK, now that I know what happened, this makes sense to me.\nBut I'd like to understand whether what created this in the first place:\n\n> \n>  - you had gotten some corrupt objects due to the disk filling up \n>    (probably during the pull that thus populated the object database with\n>    partially written objects)\n> \n>    In particular, the 4d4d30be967d3284cbf59afd4fba6ab536e295f5 object was \n>    corrupt. fsck gave a confusing error message:\n> \n> \terror: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n> \terror: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n> \n>    which is really because the _file_ for that object does exist, but the \n>    file doesn't actually contain the object it expects (due to \n>    corruption), so the object wasn't \"found\". \n\nis something expected?\n\n-- \nMST\n"},{"id":"31502","messageId":"Pine.LNX.4.64.0701111353030.3594@woody.osdl.org","threadId":"6338","inReplyTo":"45A6A97F.5080008@shadowen.org","subject":"Re: tree corrupted on disk quota full","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T21:59:39Z","receivedAt":"2007-01-11T21:59:39Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Andy Whitcroft wrote:\n> \n> The call was intended to replace a common idiom:\n> \n> if (xwrite(fd, buf, size) != size)\n> \terror\n\nI really don't agree.\n\nYou should use \"write_or_die()\" for this idiom.\n\nThere really is two cases:\n - the complex and robust one: \"we will keep writing even in the presense \n   of partial writes\".\n\n   This is \"xwrite()\". \n\n - the simple case: \"write everything\", anything else is an error. This is \n   \"write_or_die()\", or \"write_in_full()\".\n\nAnd I claim, that for the \"write_in_full()\" case, if you EVER get anything \nbut the full length back, that's an error.\n\nAnd it should be treated as such. There is NO POINT in write_in_full() \never returning a partial result. It's unrecoverable by design - and if it \nwasn't, you wouldn't use \"write_in_full()\" in the first place, you'd just \nuse \"xwrite()\".\n\nAnd returning a partial result in that case is just a recipe for disaster. \nI already pointed to one real bug due to this: write_buffer() was (and \ncurrently is) simply buggy. And it's buggy EXACTLY becaue \n\"write_in_full()\" was doing the wrong thing, and just made it easy to \nwrite buggy code - it was no easier to use than xwrite(), and thus there \nwas no point to it. Might as well have just used xwrite() in the first \nplace.\n\nSo I repeat: either you use \"xwrite()\" (and handle the partial case), or \nyou use \"write_in_full()\" (and the partial case is an *ERROR*). There is \nno sane middle ground in between those cases.\n\n\t\t\tLinus\n"},{"id":"31505","messageId":"Pine.LNX.4.64.0701111400031.3594@woody.osdl.org","threadId":"6338","inReplyTo":"7vodp52qzz.fsf@assigned-by-dhcp.cox.net","subject":"Better error messages for corrupt databases","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T22:09:31Z","receivedAt":"2007-01-11T22:09:31Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nThis fixes another problem that Andy's case showed: git-fsck-objects \nreports nonsensical results for corrupt objects.\n\nThere were actually two independent and confusing problems:\n\n - when we had a zero-sized file and used map_sha1_file, mmap() would \n   return EINVAL, and git-fsck-objects would report that as an insane and \n   confusing error. I don't know when this was introduced, it might have \n   been there forever.\n\n - when \"parse_object()\" returned NULL, fsck would say \"object not found\", \n   which can be very confusing, since obviously the object might \"exist\", \n   it's just unparseable because it's totally corrupt.\n\nSo this just makes \"xmmap()\" return NULL for a zero-sized object (which is \na valid thing pointer, exactly the same way \"malloc()\" can return NULL for \na zero-sized allocation). That fixes the first problem (but we could have \nfixed it in the caller too - I don't personally much care whichever way it \ngoes, but maybe somebody should check that the NO_MMAP case does \nsomething sane in this case too?).\n\nAnd the second problem is solved by just making the error message slightly \nclearer - the failure to parse an object may be because it's missing or \ncorrupt, not necessarily because it's not \"found\".\n\nSigned-off-by: Linus Torvalds <torvalds@osdl.org>\n---\n\nThis would hopefully have made the error case Andy had a bit more readable \nand understandable.\n\ndiff --git a/fsck-objects.c b/fsck-objects.c\nindex 0d8a8eb..81f00db 100644\n--- a/fsck-objects.c\n+++ b/fsck-objects.c\n@@ -290,7 +290,7 @@ static int fsck_sha1(unsigned char *sha1)\n {\n \tstruct object *obj = parse_object(sha1);\n \tif (!obj)\n-\t\treturn error(\"%s: object not found\", sha1_to_hex(sha1));\n+\t\treturn error(\"%s: object corrupt or missing\", sha1_to_hex(sha1));\n \tif (obj->flags & SEEN)\n \t\treturn 0;\n \tobj->flags |= SEEN;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f8d46d5..8781e8e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -202,6 +202,8 @@ static inline void *xmmap(void *start, size_t length,\n {\n \tvoid *ret = mmap(start, length, prot, flags, fd, offset);\n \tif (ret == MAP_FAILED) {\n+\t\tif (!length)\n+\t\t\treturn NULL;\n \t\trelease_pack_memory(length);\n \t\tret = mmap(start, length, prot, flags, fd, offset);\n \t\tif (ret == MAP_FAILED)\n"},{"id":"31504","messageId":"45A6B5E4.3050007@shadowen.org","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111353030.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2007-01-11T22:10:44Z","receivedAt":"2007-01-11T22:10:44Z","isPatch":false,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"Linus Torvalds wrote:\n> \n> On Thu, 11 Jan 2007, Andy Whitcroft wrote:\n>> The call was intended to replace a common idiom:\n>>\n>> if (xwrite(fd, buf, size) != size)\n>> \terror\n> \n> I really don't agree.\n> \n> You should use \"write_or_die()\" for this idiom.\n> \n> There really is two cases:\n>  - the complex and robust one: \"we will keep writing even in the presense \n>    of partial writes\".\n> \n>    This is \"xwrite()\". \n> \n>  - the simple case: \"write everything\", anything else is an error. This is \n>    \"write_or_die()\", or \"write_in_full()\".\n> \n> And I claim, that for the \"write_in_full()\" case, if you EVER get anything \n> but the full length back, that's an error.\n> \n> And it should be treated as such. There is NO POINT in write_in_full() \n> ever returning a partial result. It's unrecoverable by design - and if it \n> wasn't, you wouldn't use \"write_in_full()\" in the first place, you'd just \n> use \"xwrite()\".\n> \n> And returning a partial result in that case is just a recipe for disaster. \n> I already pointed to one real bug due to this: write_buffer() was (and \n> currently is) simply buggy. And it's buggy EXACTLY becaue \n> \"write_in_full()\" was doing the wrong thing, and just made it easy to \n> write buggy code - it was no easier to use than xwrite(), and thus there \n> was no point to it. Might as well have just used xwrite() in the first \n> place.\n> \n> So I repeat: either you use \"xwrite()\" (and handle the partial case), or \n> you use \"write_in_full()\" (and the partial case is an *ERROR*). There is \n> no sane middle ground in between those cases.\n\nThings should be safe in general with the code as it is as we are\nchecking the write length.  But it is clear that converting to an error\nwould simplify and clean up the code.  Should have done that the first\ntime.\n\nIf its not done in the morning, I'll fix it up.\n\n-apw\n"},{"id":"31507","messageId":"Pine.LNX.4.64.0701111410420.3594@woody.osdl.org","threadId":"6338","inReplyTo":"20070111215812.GK17999@mellanox.co.il","subject":"Re: tree corrupted on disk quota full","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T22:24:03Z","receivedAt":"2007-01-11T22:24:03Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Michael S. Tsirkin wrote:\n> \n> OK, now that I know what happened, this makes sense to me.\n> But I'd like to understand whether what created this in the first place:\n> \n> > \n> >  - you had gotten some corrupt objects due to the disk filling up \n> >    (probably during the pull that thus populated the object database with\n> >    partially written objects)\n> > \n> >    In particular, the 4d4d30be967d3284cbf59afd4fba6ab536e295f5 object was \n> >    corrupt. fsck gave a confusing error message:\n> > \n> > \terror: 4d4d30be967d3284cbf59afd4fba6ab536e295f5: object not found\n> > \terror: c03590b581d51d5fa43adbef9415e935d0229412: object not found\n> > \n> >    which is really because the _file_ for that object does exist, but the \n> >    file doesn't actually contain the object it expects (due to \n> >    corruption), so the object wasn't \"found\". \n> \n> is something expected?\n\nWell, it's a bug, and it's not \"expected\", but I think it's understood. \n\nWhat happens is that you pulled objects that were perfectly fine, and when \nyou ran out of diskspace git could create the _filenames_ and inodes for \nthem, but when it actually wrote the data itself, it failed.\n\nAnd it failed in a very unfortunate place: when unpacking the pack-file \n(which you got when doing the \"git pull\"), it would call \n\"write_sha1_file()\" to actually write the unpacked object. That one does \neverything correctly and is actually very careful, but then it does\n\n\tif (write_buffer(fd, compressed, size) < 0)\n\t\tdie(\"unable to write sha1 file\");\n\nwhich _should_ have caught the case that the buffer couldn't be written, \nand died cleanly with an error message, and it would neve rhave moved the \ntemporary file that it wrote to into the real database.\n\nIn other words, it was doing everything right. A partial file would never \nhave actually been allowed to be moved into the database at all, and you'd \nhave gotten a nice error message, and the \"git pull\" would have failed \ngracefully.\n\nHowever, \"write_buffer()\" itself was buggy. It did:\n\n\tstatic int write_buffer(int fd, const void *buf, size_t len)\n\t{\n\t        ssize_t size;\n\t\n\t        size = write_in_full(fd, buf, len);\n\t        if (!size)\n\t                return error(\"file write: disk full\");\n\t        if (size < 0)\n\t                return error(\"file write error (%s)\", strerror(errno));\n\t        return 0;\n\t}\n\nand the problem here is that if the write was _partial_ (not totally \nempty), it would see a positive size, but not a full write, and it would \nincorrectly return zero for \"everything is fine\". Even though it wasn't.\n\nSo my patch to make \"write_in_full()\" have better semantics (and make it \nsimpler too) should have fixed the fundamental problem that caused your \nfailure in the first place. The other patch I just sent out should just \nmake the error messages be a bit nicer, and isn't important in itself.\n\nBtw, that \"write_buffer()\" bug was introduced pretty recently. It used to \nbe that write_buffer() did the right thing: it used to do:\n\n\twhile (len) {\n\t\tssize_t size;\n\n\t\tsize = write(fd, buf, len);\n\t\tif (!size)\n\t\t\treturn error(\"file write: disk full\");\n\t\tif (size < 0) {\n\t\t\tif (errno == EINTR || errno == EAGAIN)\n\t\t\t\tcontinue;\n\t\t\treturn error(\"file write error (%s)\", strerror(errno));\n\t\t}\n\t\tlen -= size;\n\t\tbuf = (char *) buf + size;\n\t}\n\nwhich is safe and does the right thing for partial writes and disk full \nerrors (of course it is: I wrote that piece of code).\n\nSo this bug was _literally_ introduced three days ago, and it was \nintroduced by a set of patches that tried to _avoid_ the problem.\n\nSad. The old code was perfectly good. The new code was unsafe, but thought \nit was better. See commit 93822c2239a336e5cb583549071c59202ef6c5b2 for \ndetails.\n\nJunio, pls apply my \"fix write_in_full\" patch asap if you haven't already. \nI find it very irritating when people \"clean up\" my code, and introduce \nbugs when they do so.\n\n\t\tLinus\n"},{"id":"31510","messageId":"Pine.LNX.4.64.0701111428420.3594@woody.osdl.org","threadId":"6338","inReplyTo":"45A6B5E4.3050007@shadowen.org","subject":"Re: tree corrupted on disk quota full","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T22:32:56Z","receivedAt":"2007-01-11T22:32:56Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Andy Whitcroft wrote:\n>\n> > So I repeat: either you use \"xwrite()\" (and handle the partial case), or \n> > you use \"write_in_full()\" (and the partial case is an *ERROR*). There is \n> > no sane middle ground in between those cases.\n> \n> Things should be safe in general with the code as it is as we are\n> checking the write length.\n\nNO WE ARE NOT.\n\nI already pointed you to write_buffer(). It used to do the right thing. It \ndoesn't any more. And it doesn't, exactly because you converted it away \nfrom a loop that did it right, to doing \"write_in_full()\" and NOT checking \nthe return value properly.\n\nThe thing is, if you support partial writes (ie xwrite()), you need to do \nit in a loop, and then the correct thing to check for is \"zero or error\".\n\nOnce you don't do a loop (ie \"write_in_full()\" - the whole _point_ is to \nnot do the loop, after all), you need to either expand that check to \"zero \nor error or partial\" (which just makes the code _more_ complex), or you \nneed to make \"write_in_full()\" just return an error for the partial case.\n\nWhich is why I'm harping on this issue: either we use \"xwrite()\", or we \nfix \"write_in_full()\" to return errors on partials. Because the \"middle \nground\", where write_in_full() emulates the partial case of \"xwrite()\" is \nactually MORE complex than \"xwrite()\" itself.\n\n\t\tLinus\n"},{"id":"31511","messageId":"20070111223905.GP17999@mellanox.co.il","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111410420.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Michael S. Tsirkin","fromEmail":"mst@mellanox.co.il","sentAt":"2007-01-11T22:39:05Z","receivedAt":"2007-01-11T22:39:05Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> So this bug was _literally_ introduced three days ago, and it was \n> introduced by a set of patches that tried to _avoid_ the problem.\n> \n> Sad. The old code was perfectly good. The new code was unsafe, but thought \n> it was better. See commit 93822c2239a336e5cb583549071c59202ef6c5b2 for \n> details.\n\nWeird. I think the system where the tree got corrupted had git 1.4.4.4.\n\n-- \nMST\n"},{"id":"31525","messageId":"20070112004044.GA23864@spearce.org","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111400031.3594@woody.osdl.org","subject":"Re: Better error messages for corrupt databases","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-12T00:40:44Z","receivedAt":"2007-01-12T00:40:44Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> wrote:\n> So this just makes \"xmmap()\" return NULL for a zero-sized object (which is \n> a valid thing pointer, exactly the same way \"malloc()\" can return NULL for \n> a zero-sized allocation). That fixes the first problem (but we could have \n> fixed it in the caller too - I don't personally much care whichever way it \n> goes, but maybe somebody should check that the NO_MMAP case does \n> something sane in this case too?).\n\nAll calls to perform any sort of mmap go through xmmap().  If NO_MMAP\nwas defined at compile time than the mmap() call within xmmap() is\nredefined to invoke git_mmap() via a preprocessor define slightly\nearlier in git-compat-util.h.\n\nSo your patch will also automatically do the right thing in the\nNO_MMAP case.\n\nBTW, I like your solution of just fixing it here in xmmap().  Good idea.\n\n-- \nShawn.\n"},{"id":"31526","messageId":"Pine.LNX.4.64.0701111452340.3594@woody.osdl.org","threadId":"6338","inReplyTo":"20070111223905.GP17999@mellanox.co.il","subject":"Re: tree corrupted on disk quota full","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-12T00:42:04Z","receivedAt":"2007-01-12T00:42:04Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 12 Jan 2007, Michael S. Tsirkin wrote:\n> \n> Weird. I think the system where the tree got corrupted had git 1.4.4.4.\n\nOk, you had an email with \"git version\" that said 1.5.0-rc0-g<something>, \nbut that may have been after you already upgraded for other reasons.\n\nIt would definitely be good to reproduce this - there are multiple places \nwhere we could screw up in the presense of a short write. It's also \nentirely possible that the core code doesn't screw up, but if some of the \nhigher-level scripts don't always check the error code, an error at \nunpacking time (or some other time) could just go unnoticed.\n\nThat said, even though I complain about the \"write_in_full()\" bug, I think \nthat with the \"return error even for partial writes\" change it actually \n_does_ end up being a safer way to do things, so it's entirely possible \nthat while it introduced one bug, it could have fixed another one. The \ncall chain I looked at was just one particular one (it's the most \nfundamental one for writing out individual objects, so in that sense it's \nthe most likely one, but it's certainly possible that something else \nhappened)\n\n\t\tLinus\n"},{"id":"31528","messageId":"20070112005124.GC23864@spearce.org","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111452340.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-12T00:51:24Z","receivedAt":"2007-01-12T00:51:24Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, 12 Jan 2007, Michael S. Tsirkin wrote:\n> \n> Weird. I think the system where the tree got corrupted had git 1.4.4.4.\n\nOr maybe it was 1.5.0-rc0g<something>.  git-describe has a bug (but\nis patched in pu) that caused it to pick up 1.4.4.4 on master/next/pu\nright now due to the 1.4.4.4 maint release being merged back into\nmaster/next/pu after the 1.5.0rc-0 tag was created.\n\n-- \nShawn.\n"},{"id":"31616","messageId":"7vzm8noigp.fsf@assigned-by-dhcp.cox.net","threadId":"6338","inReplyTo":"Pine.LNX.4.64.0701111410420.3594@woody.osdl.org","subject":"Re: tree corrupted on disk quota full","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-13T00:53:26Z","receivedAt":"2007-01-13T00:53:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> Sad. The old code was perfectly good. The new code was unsafe, but thought \n> it was better. See commit 93822c2239a336e5cb583549071c59202ef6c5b2 for \n> details.\n\nSorry about not catching it earlier.\n"}]}