{"thread":{"id":"6628","subject":"[PATCH 2/2] Flag and skip over packfiles known to be invalid.","startedAt":"2007-02-02T08:00:10Z","lastAt":"2007-02-04T05:02:37Z","messageCount":5,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"33354","messageId":"20070202080010.GB21094@spearce.org","threadId":"6628","inReplyTo":"dda240a4adf0511b3e1ab1eb74abdd28821358b0.1170403175.git.spearce@spearce.org","subject":"[PATCH 2/2] Flag and skip over packfiles known to be invalid.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-02-02T08:00:10Z","receivedAt":"2007-02-02T08:00:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we've already looked at a packfile and determined it isn't\nvalid/usable as a pack, we shouldn't try to use it again in\nthe future either. This avoids multiple error messages from\nthe same packfile.\n\nSigned-off-by: Shawn O. Pearce \n---\n cache.h | 1 +\n sha1_file.c | 5 +++++\n 2 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 9873ee9..64c2d3f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -357,6 +357,7 @@ extern struct packed_git {\n \toff_t pack_size;\n \tint pack_fd;\n \tint pack_local;\n+\tunsigned invalid:1;\n \tunsigned char sha1[20];\n \t/* something like \".git/objects/pack/xxxxx.pack\" */\n \tchar pack_name[FLEX_ARRAY]; /* more */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ba1c799..9f6e94e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -714,6 +714,7 @@ struct packed_git *add_packed_git(char *path, int path_len, int local)\n \tp->next = NULL;\n \tp->windows = NULL;\n \tp->pack_fd = -1;\n+\tp->invalid = 0;\n \tp->pack_local = local;\n \tif ((path_len > 44) && !get_sha1_hex(path + path_len - 44, sha1))\n \t\thashcpy(p->sha1, sha1);\n@@ -746,6 +747,7 @@ struct packed_git *parse_pack_index_file(const unsigned char *sha1, char *idx_pa\n \tp->next = NULL;\n \tp->windows = NULL;\n \tp->pack_fd = -1;\n+\tp->invalid = 0;\n \thashcpy(p->sha1, sha1);\n \treturn p;\n }\n@@ -1395,6 +1397,8 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons\n \tprepare_packed_git();\n \n \tfor (p = packed_git; p; p = p->next) {\n+\t\tif (p->invalid)\n+\t\t\tcontinue;\n \t\tif (ignore_packed) {\n \t\t\tconst char **ig;\n \t\t\tfor (ig = ignore_packed; *ig; ig++)\n@@ -1418,6 +1422,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons\n \t\t\t\t\tclose(p->pack_fd);\n \t\t\t\t\tp->pack_fd = -1;\n \t\t\t\t}\n+\t\t\t\tp->invalid = 1;\n \t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n \t\t\t\tcontinue;\n \t\t\t}\n-- \n1.5.0.rc3.1.ge4b0e\n"},{"id":"33356","messageId":"7virelhsae.fsf@assigned-by-dhcp.cox.net","threadId":"6628","inReplyTo":"20070202080010.GB21094@spearce.org","subject":"Re: [PATCH 2/2] Flag and skip over packfiles known to be invalid.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-02T08:34:17Z","receivedAt":"2007-02-02T08:34:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> If we've already looked at a packfile and determined it isn't\n> valid/usable as a pack, we shouldn't try to use it again in\n> the future either. This avoids multiple error messages from\n> the same packfile.\n\nI haven't looked your patch very closely, but two successive\n\"git-repack -a -d\" would produce the packfile under the same\nname, but with different parameters (--window or --depth), the\nold .idx and new .pack may not match.\n\nCan this be possible (time flows from top to bottom):\n\n\tgit-repack -a -d\t\tsomebody else\n\n\tproduces a new .pack/.idx\n\n mv new .pack to objects/pack\n\n\t\t\t\t\tmmap .idx (old)\n\t\t\t\t\tfinds .pack does not match\n\n\tmv new .idx to objects/pack\n\t\nIs this something we care about?\n"},{"id":"33358","messageId":"20070202085108.GC20832@spearce.org","threadId":"6628","inReplyTo":"7virelhsae.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/2] Flag and skip over packfiles known to be invalid.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-02-02T08:51:08Z","receivedAt":"2007-02-02T08:51:08Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> I haven't looked your patch very closely, but two successive\n> \"git-repack -a -d\" would produce the packfile under the same\n> name, but with different parameters (--window or --depth), the\n> old .idx and new .pack may not match.\n> \n> Can this be possible (time flows from top to bottom):\n> \n> \tgit-repack -a -d\t\tsomebody else\n> \n> \tproduces a new .pack/.idx\n> \n> mv new .pack to objects/pack\n> \n> \t\t\t\t\tmmap .idx (old)\n> \t\t\t\t\tfinds .pack does not match\n> \n> \tmv new .idx to objects/pack\n\nGah. That's sick. And *so* totally possible.\n \t\n> Is this something we care about?\n \nYes and no.\n\nI've pushed the idea that `git repack -a -d` is totally safe to\nrun in a live repository. Its clearly not. *Especially* if you\nrepack with the same set of objects (as you do above) or if you\nrepack in rapid succession faster than readers can make progress.\nI think both of these are just outside of the realm of \"good ideas\nto do to live things\".\n\nIts like taking your cat to the zoo play with a lion. Maybe not\nthe best thing for the cat...\n\n\nIn this particular case that you highlight above this patch would\nmark the pack as invalid and never permit it to be accessed again\nby the 'somebody else'.\n\nBut Git has always been broken for this case. prepare_packed_git_one\nwould refuse to open the new index, as it already has a packed_git.\nPrior to this patch we'd try to access the packfile and die(),\nas it did not match the index that we have. Now we'd at least\ntry to locate some other source, or die() over a missing object,\nbut only after listing the idx/pack mismatch error too.\n\nThe only fix for the case above is to unlink the invalid packed_git\nfrom packed_git, so that if we fail to find the object and wind up\ncalling reprepare_packed_git() we can reload the new index. I did\nconsider doing that over the invalid flag, but flipped a coin and\nadded invalid. FWIW, maybe invalid is the wrong way to go.\n\n\nWe still have a problem though, as the above case could still\nhappen to us during the fallback search in read_sha1_file().\nIn other words two searches ain't enough. As Dscho pointed out in\nthe original thread, we probably need to loop until no new packs\nare identified before giving up.\n\nI almost submitted a patch to do that tonight, but I couldn't decide\non behavior: should we scan known packs, then try for loose, then\nscan packs again until no object or no new pack is found? Probably.\n\n-- \nShawn.\n"},{"id":"33433","messageId":"7vr6t7bxsn.fsf@assigned-by-dhcp.cox.net","threadId":"6628","inReplyTo":"20070202085108.GC20832@spearce.org","subject":"Re: [PATCH 2/2] Flag and skip over packfiles known to be invalid.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-03T05:44:08Z","receivedAt":"2007-02-03T05:44:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> I almost submitted a patch to do that tonight, but I couldn't decide\n> on behavior: should we scan known packs, then try for loose, then\n> scan packs again until no object or no new pack is found? Probably.\n\nHmmm. Probably. \n\nBut I tend to think that this particular failure scenario is\nprobably rare enough that plugging this in \"the right way\" is\nnot a high priority. We should definitely revisit it post\n1.5.0.\n\nAlso if we are adding a bitfield, I think pack_local should also\nbecome one, as it currently wastes a whole word to hold one bit\n(on the other hand if we do not want to add a field I think a\ndifferent negative value in pack_fd could mean \"do not bother to\nlook at it again\").\n"},{"id":"33529","messageId":"20070204050237.GD16766@spearce.org","threadId":"6628","inReplyTo":"7vr6t7bxsn.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/2] Flag and skip over packfiles known to be invalid.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-02-04T05:02:37Z","receivedAt":"2007-02-04T05:02:37Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > I almost submitted a patch to do that tonight, but I couldn't decide\n> > on behavior: should we scan known packs, then try for loose, then\n> > scan packs again until no object or no new pack is found? Probably.\n> \n> Hmmm. Probably. \n> \n> But I tend to think that this particular failure scenario is\n> probably rare enough that plugging this in \"the right way\" is\n> not a high priority. We should definitely revisit it post\n> 1.5.0.\n\nIndeed. I'll come back to it after 1.5.0 is out.\n \n> Also if we are adding a bitfield, I think pack_local should also\n> become one, as it currently wastes a whole word to hold one bit\n> (on the other hand if we do not want to add a field I think a\n> different negative value in pack_fd could mean \"do not bother to\n> look at it again\").\n\nGood point. I forgot about that ~4 byte boolean hanging around.\nAs a comment on the TDWTF might say, \"Yes, No, FileNotFound, 42,\n192, 1088, ... these are all valid values for pack_local!\" :-)\n\n-- \nShawn.\n"}]}