{"thread":{"id":"25516","subject":"[PATCH] make pack-objects a bit more resilient to repo corruption","startedAt":"2010-10-22T04:53:32Z","lastAt":"2010-10-24T02:47:52Z","messageCount":11,"participants":["Nicolas Pitre","Jeff King","Drew Northup","Sverre Rabbelier","Junio C Hamano","Geert Bosch"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"154096","messageId":"alpine.LFD.2.00.1010220037250.2764@xanadu.home","threadId":"25516","inReplyTo":null,"subject":"[PATCH] make pack-objects a bit more resilient to repo corruption","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-10-22T04:53:32Z","receivedAt":"2010-10-22T04:53:32Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Right now, packing valid objects could fail when creating a thin pack \nsimply because a pack edge object used as a preferred base is corrupted.\nSince preferred base objects are not strictly needed to produce a valid\npack, let's not consider the inability to read them as a fatal error.\nDelta compression may well be attempted against other objects in the\nsearch window.\n\nSigned-off-by: Nicolas Pitre \n---\n\nI wrote this patch while helping Uwe with his repo corruption. While \nhis problem turned out to be different from the one this patch is \naddressing (see previous patch I posted) I think that since I did the \npatch already then this wouldn't hurt to have this one merged too.\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f8eba53..674247e 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1298,9 +1298,19 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \t\tread_lock();\n \t\tsrc->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);\n \t\tread_unlock();\n-\t\tif (!src->data)\n+\t\tif (!src->data) {\n+\t\t\tif (src_entry->preferred_base) {\n+\t\t\t\t/* \n+\t\t\t\t * Those objects are not included in the\n+\t\t\t\t * resulting pack. Be resilient and ignore\n+\t\t\t\t * them if they can't be read, in case the\n+\t\t\t\t * pack could be created nevertheless.\n+\t\t\t\t */\n+\t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tdie(\"object %s cannot be read\",\n \t\t\t sha1_to_hex(src_entry->idx.sha1));\n+\t\t}\n \t\tif (sz != src_size)\n \t\t\tdie(\"object %s inconsistent object length (%lu vs %lu)\",\n \t\t\t sha1_to_hex(src_entry->idx.sha1), sz, src_size);\n"},{"id":"154149","messageId":"20101022144600.GA5554@sigill.intra.peff.net","threadId":"25516","inReplyTo":"alpine.LFD.2.00.1010220037250.2764@xanadu.home","subject":"Re: [PATCH] make pack-objects a bit more resilient to repo corruption","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-22T14:46:01Z","receivedAt":"2010-10-22T14:46:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:\n\n> -\t\tif (!src->data)\n> +\t\tif (!src->data) {\n> +\t\t\tif (src_entry->preferred_base) {\n> +\t\t\t\t/* \n> +\t\t\t\t * Those objects are not included in the\n> +\t\t\t\t * resulting pack. Be resilient and ignore\n> +\t\t\t\t * them if they can't be read, in case the\n> +\t\t\t\t * pack could be created nevertheless.\n> +\t\t\t\t */\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> \t\t\tdie(\"object %s cannot be read\",\n> \t\t\t sha1_to_hex(src_entry->idx.sha1));\n> +\t\t}\n\nBy converting this die() into a silent return, are we losing a place\nwhere git might previously have alerted a user to corruption? In this\ncase, we can continue the operation without the object, but if we have\ndetected corruption, letting the user know as soon as possible is\nprobably a good idea.\n\nIn other words, should this instead be:\n\n warning(\"unable to read preferred base object: %s\", ...);\n return 0;\n\nOr will some other part of the code already complained to stderr?\n\n-Peff\n"},{"id":"154158","messageId":"1287761064.31218.37.camel@drew-northup.unet.maine.edu","threadId":"25516","inReplyTo":"20101022144600.GA5554@sigill.intra.peff.net","subject":"Re: [PATCH] make pack-objects a bit more resilient to repo corruption","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2010-10-22T15:24:24Z","receivedAt":"2010-10-22T15:24:24Z","isPatch":true,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Fri, 2010-10-22 at 10:46 -0400, Jeff King wrote:\n> On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:\n> \n> > -\t\tif (!src->data)\n> > +\t\tif (!src->data) {\n> > +\t\t\tif (src_entry->preferred_base) {\n> > +\t\t\t\t/* \n> > +\t\t\t\t * Those objects are not included in the\n> > +\t\t\t\t * resulting pack. Be resilient and ignore\n> > +\t\t\t\t * them if they can't be read, in case the\n> > +\t\t\t\t * pack could be created nevertheless.\n> > +\t\t\t\t */\n> > +\t\t\t\treturn 0;\n> > +\t\t\t}\n> > \t\t\tdie(\"object %s cannot be read\",\n> > \t\t\t sha1_to_hex(src_entry->idx.sha1));\n> > +\t\t}\n> \n> By converting this die() into a silent return, are we losing a place\n> where git might previously have alerted a user to corruption? In this\n> case, we can continue the operation without the object, but if we have\n> detected corruption, letting the user know as soon as possible is\n> probably a good idea.\n> \n> In other words, should this instead be:\n> \n> warning(\"unable to read preferred base object: %s\", ...);\n> return 0;\n> \n> Or will some other part of the code already complained to stderr?\n> \n> -Peff\n\nAgreed. If it broke we should probably tell the user--even if we can't\ndo much useful about it other than attempt to recover by continuing.\n\n-- \n-Drew Northup N1XIM\n AKA RvnPhnx on OPN\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"154186","messageId":"alpine.LFD.2.00.1010221427390.2764@xanadu.home","threadId":"25516","inReplyTo":"20101022144600.GA5554@sigill.intra.peff.net","subject":"Re: [PATCH] make pack-objects a bit more resilient to repo corruption","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-10-22T18:42:09Z","receivedAt":"2010-10-22T18:42:09Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 22 Oct 2010, Jeff King wrote:\n\n> On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:\n> \n> > -\t\tif (!src->data)\n> > +\t\tif (!src->data) {\n> > +\t\t\tif (src_entry->preferred_base) {\n> > +\t\t\t\t/* \n> > +\t\t\t\t * Those objects are not included in the\n> > +\t\t\t\t * resulting pack. Be resilient and ignore\n> > +\t\t\t\t * them if they can't be read, in case the\n> > +\t\t\t\t * pack could be created nevertheless.\n> > +\t\t\t\t */\n> > +\t\t\t\treturn 0;\n> > +\t\t\t}\n> > \t\t\tdie(\"object %s cannot be read\",\n> > \t\t\t sha1_to_hex(src_entry->idx.sha1));\n> > +\t\t}\n> \n> By converting this die() into a silent return, are we losing a place\n> where git might previously have alerted a user to corruption? In this\n> case, we can continue the operation without the object, but if we have\n> detected corruption, letting the user know as soon as possible is\n> probably a good idea.\n> \n> In other words, should this instead be:\n> \n> warning(\"unable to read preferred base object: %s\", ...);\n> return 0;\n\nWell, this get called repeatedly, being within the inner part of the \ndelta search loop. So you might get that warning as many times as the \ndelta window which is not that nice. If anything a static flag to \ndisplay the warning only once would be needed. But you're pretty likely \nto have met that warning/error already from other operations, which is \nwhy I didn't bother.\n\n> Or will some other part of the code already complained to stderr?\n\nSome other part is likely to already have complained, through \ncheck_object() -> sha1_object_info(). But not necessarily in all cases.\n\n\nNicolas\n"},{"id":"154189","messageId":"alpine.LFD.2.00.1010221451340.2764@xanadu.home","threadId":"25516","inReplyTo":"1287761064.31218.37.camel@drew-northup.unet.maine.edu","subject":"Re: [PATCH] make pack-objects a bit more resilient to repo corruption","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-10-22T18:54:40Z","receivedAt":"2010-10-22T18:54:40Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 22 Oct 2010, Drew Northup wrote:\n\n> \n> On Fri, 2010-10-22 at 10:46 -0400, Jeff King wrote:\n> > On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:\n> > \n> > > -\t\tif (!src->data)\n> > > +\t\tif (!src->data) {\n> > > +\t\t\tif (src_entry->preferred_base) {\n> > > +\t\t\t\t/* \n> > > +\t\t\t\t * Those objects are not included in the\n> > > +\t\t\t\t * resulting pack. Be resilient and ignore\n> > > +\t\t\t\t * them if they can't be read, in case the\n> > > +\t\t\t\t * pack could be created nevertheless.\n> > > +\t\t\t\t */\n> > > +\t\t\t\treturn 0;\n> > > +\t\t\t}\n> > > \t\t\tdie(\"object %s cannot be read\",\n> > > \t\t\t sha1_to_hex(src_entry->idx.sha1));\n> > > +\t\t}\n> > \n> > By converting this die() into a silent return, are we losing a place\n> > where git might previously have alerted a user to corruption? In this\n> > case, we can continue the operation without the object, but if we have\n> > detected corruption, letting the user know as soon as possible is\n> > probably a good idea.\n> > \n> > In other words, should this instead be:\n> > \n> > warning(\"unable to read preferred base object: %s\", ...);\n> > return 0;\n> > \n> > Or will some other part of the code already complained to stderr?\n> > \n> > -Peff\n> \n> Agreed. If it broke we should probably tell the user--even if we can't\n> do much useful about it other than attempt to recover by continuing.\n\nPlease don't misinterpret this case. As far as this change is \nconcerned, nothing is actually \"broken\". The operation _will_ still \nsucceed. The repository may be broken, but in this case we can do \nwithout the broken object. In those cases where the object is really \nneeded the original die() is still in place.\n\n\nNicolas\n"},{"id":"154202","messageId":"alpine.LFD.2.00.1010221606550.2764@xanadu.home","threadId":"25516","inReplyTo":"alpine.LFD.2.00.1010221427390.2764@xanadu.home","subject":"[PATCH v2] make pack-objects a bit more resilient to repo corruption","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-10-22T20:26:23Z","receivedAt":"2010-10-22T20:26:23Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Right now, packing valid objects could fail when creating a thin pack\nsimply because a pack edge object used as a preferred base is corrupted.\nSince preferred base objects are not strictly needed to produce a valid\npack, let's not consider the inability to read them as a fatal error.\nDelta compression may well be attempted against other objects in the\nsearch window. To avoid warning storms (we are in the inner loop of\nthe delta search window) a warning is emitted only on the first \noccurrence.\n\nSigned-off-by: Nicolas Pitre \n---\n\nOn Fri, 22 Oct 2010, Nicolas Pitre wrote:\n\n> On Fri, 22 Oct 2010, Jeff King wrote:\n> \n> > By converting this die() into a silent return, are we losing a place\n> > where git might previously have alerted a user to corruption? In this\n> > case, we can continue the operation without the object, but if we have\n> > detected corruption, letting the user know as soon as possible is\n> > probably a good idea.\n> > \n> > In other words, should this instead be:\n> > \n> > warning(\"unable to read preferred base object: %s\", ...);\n> > return 0;\n> \n> Well, this get called repeatedly, being within the inner part of the \n> delta search loop. So you might get that warning as many times as the \n> delta window which is not that nice. If anything a static flag to \n> display the warning only once would be needed. But you're pretty likely \n> to have met that warning/error already from other operations, which is \n> why I didn't bother.\n> \n> > Or will some other part of the code already complained to stderr?\n> \n> Some other part is likely to already have complained, through \n> check_object() -> sha1_object_info(). But not necessarily in all cases.\n\nOK... After further analysis, it seems that the cases when problem \nobjects are already warned about through sha1_object_info(), those \nobjects will never end up in the delta search window, as their type ends \nup being a negative error code. We already support that possibility on \npurpose even.\n\nSo let's add a warning for when check_object() was able to bypass\nthe more expensive sha1_object_info() call and therefore object \ncorruptions remain undetected until that point.\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f8eba53..81155b4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1298,9 +1298,23 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \t\tread_lock();\n \t\tsrc->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);\n \t\tread_unlock();\n-\t\tif (!src->data)\n+\t\tif (!src->data) {\n+\t\t\tif (src_entry->preferred_base) {\n+\t\t\t\tstatic int warned = 0;\n+\t\t\t\tif (!warned++)\n+\t\t\t\t\twarning(\"object %s cannot be read\",\n+\t\t\t\t\t\tsha1_to_hex(src_entry->idx.sha1));\n+\t\t\t\t/* \n+\t\t\t\t * Those objects are not included in the\n+\t\t\t\t * resulting pack. Be resilient and ignore\n+\t\t\t\t * them if they can't be read, in case the\n+\t\t\t\t * pack could be created nevertheless.\n+\t\t\t\t */\n+\t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tdie(\"object %s cannot be read\",\n \t\t\t sha1_to_hex(src_entry->idx.sha1));\n+\t\t}\n \t\tif (sz != src_size)\n \t\t\tdie(\"object %s inconsistent object length (%lu vs %lu)\",\n \t\t\t sha1_to_hex(src_entry->idx.sha1), sz, src_size);\n"},{"id":"154204","messageId":"AANLkTimy-ihrF1syWYe3T4W6-UHzCaj5Jud5rdFmv3D5@mail.gmail.com","threadId":"25516","inReplyTo":"alpine.LFD.2.00.1010221606550.2764@xanadu.home","subject":"Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-10-22T20:50:01Z","receivedAt":"2010-10-22T20:50:01Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Fri, Oct 22, 2010 at 13:26, Nicolas Pitre wrote:\n> +                               static int warned = 0;\n> +                               if (!warned++)\n> +                                       warning(\"object %s cannot be read\",\n> +                                               sha1_to_hex(src_entry->idx.sha1));\n\nHow does this handle multiple missing objects? Will it only warn for\nthe first one?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"154209","messageId":"alpine.LFD.2.00.1010221714450.2764@xanadu.home","threadId":"25516","inReplyTo":"AANLkTimy-ihrF1syWYe3T4W6-UHzCaj5Jud5rdFmv3D5@mail.gmail.com","subject":"Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-10-22T21:19:29Z","receivedAt":"2010-10-22T21:19:29Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 22 Oct 2010, Sverre Rabbelier wrote:\n\n> Heya,\n> \n> On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre wrote:\n> > +                               static int warned = 0;\n> > +                               if (!warned++)\n> > +                                       warning(\"object %s cannot be read\",\n> > +                                               sha1_to_hex(src_entry->idx.sha1));\n> \n> How does this handle multiple missing objects? Will it only warn for\n> the first one?\n\nYes, only the first one, so you have a bone to chase if that ever \nhappens to you. And that's good enough IMHO. Trying to warn for every \nmissing object would require extra storage per object to remember if any \nparticular object was warned for already, which is I think overkill for \nan extremely unlikely event. Comprehensive reporting is the job of \nfsck.\n\n\nNicolas\n"},{"id":"154215","messageId":"7v4ocdlwhm.fsf@alter.siamese.dyndns.org","threadId":"25516","inReplyTo":"alpine.LFD.2.00.1010221714450.2764@xanadu.home","subject":"Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-22T21:59:33Z","receivedAt":"2010-10-22T21:59:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> Yes, only the first one, ...\n> ... Comprehensive reporting is the job of \n> fsck.\n\nI had the same knee-jerk reaction, and about to suggest using one bit from\nflags, but I agree the design balance you struck here is a good one.\n\nThanks.\n"},{"id":"154292","messageId":"30FC97D9-D9F2-4A08-8E69-4556DE204AA6@adacore.com","threadId":"25516","inReplyTo":"alpine.LFD.2.00.1010221714450.2764@xanadu.home","subject":"Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption","fromName":"Geert Bosch","fromEmail":"bosch@adacore.com","sentAt":"2010-10-24T02:26:46Z","receivedAt":"2010-10-24T02:26:46Z","isPatch":true,"sender":{"key":"bosch@adacore.com","avatar":null},"body":"\nOn Oct 22, 2010, at 17:19, Nicolas Pitre wrote:\n\n>> On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre wrote:\n>>> + static int warned = 0;\n>>> + if (!warned++)\n>>> + warning(\"object %s cannot be read\",\n>>> + sha1_to_hex(src_entry->idx.sha1));\n>> \n>> How does this handle multiple missing objects? Will it only warn for\n>> the first one?\n> \n> Yes, only the first one, so you have a bone to chase if that ever \n> happens to you. And that's good enough IMHO. Trying to warn for every \n> missing object would require extra storage per object to remember if any \n> particular object was warned for already, which is I think overkill for \n> an extremely unlikely event. Comprehensive reporting is the job of \n> fsck.\n\nMaybe add a \", run git fsck\" to the message. Will still comfortably fit a line.\n\n -Geert"},{"id":"154293","messageId":"alpine.LFD.2.00.1010232237150.2764@xanadu.home","threadId":"25516","inReplyTo":"30FC97D9-D9F2-4A08-8E69-4556DE204AA6@adacore.com","subject":"Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-10-24T02:47:52Z","receivedAt":"2010-10-24T02:47:52Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sat, 23 Oct 2010, Geert Bosch wrote:\n\n> \n> On Oct 22, 2010, at 17:19, Nicolas Pitre wrote:\n> \n> >> On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre wrote:\n> >>> + static int warned = 0;\n> >>> + if (!warned++)\n> >>> + warning(\"object %s cannot be read\",\n> >>> + sha1_to_hex(src_entry->idx.sha1));\n> >> \n> >> How does this handle multiple missing objects? Will it only warn for\n> >> the first one?\n> > \n> > Yes, only the first one, so you have a bone to chase if that ever \n> > happens to you. And that's good enough IMHO. Trying to warn for every \n> > missing object would require extra storage per object to remember if any \n> > particular object was warned for already, which is I think overkill for \n> > an extremely unlikely event. Comprehensive reporting is the job of \n> > fsck.\n> \n> Maybe add a \", run git fsck\" to the message. Will still comfortably fit a line.\n\nMaybe if this message ever gets printed often enough. Let's see if \nsomeone will even report it before 2012, and be clueless about it. And \nto be consistent, you'd have to do the same throughout the code where \nthis could be relevant.\n\nFurthermore, if someone really need the additional clue, then I'm afraid \nthat the current fsck output won't help at all except to confuse that \nperson even more.\n\nBetter for people to ask for help on this list when things break due to \ncorruptions if they can't figure it out on their own.\n\n\nNicolas\n"}]}