{"thread":{"id":"35662","subject":"[PATCH] do not discard revindex when re-preparing packfiles","startedAt":"2014-01-15T11:17:48Z","lastAt":"2014-01-15T11:17:48Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"233149","messageId":"20140115111746.GA8872@sigill.intra.peff.net","threadId":"35662","inReplyTo":null,"subject":"[PATCH] do not discard revindex when re-preparing packfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T11:17:48Z","receivedAt":"2014-01-15T11:17:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When an object lookup fails, we re-read the objects/pack\ndirectory to pick up any new packfiles that may have been\ncreated since our last read. We also discard any pack\nrevindex structs we've allocated.\n\nThe discarding is a problem for the pack-bitmap code, which keeps\na pointer to the revindex for the bitmapped pack. After the\ndiscard, the pointer is invalid, and we may read free()d\nmemory.\n\nOther revindex users do not keep a bare pointer to the\nrevindex; instead, they always access it through\nrevindex_for_pack(), which lazily builds the revindex. So\none solution is to teach the pack-bitmap code a similar\ntrick. It would be slightly less efficient, but probably not\nall that noticeable.\n\nHowever, it turns out this discarding is not actually\nnecessary. When we call reprepare_packed_git, we do not\nthrow away our old pack list. We keep the existing entries,\nand only add in new ones. So there is no safety problem; we\nwill still have the pack struct that matches each revindex.\nThe packfile itself may go away, of course, but we are\nalready prepared to handle that, and it may happen outside\nof reprepare_packed_git anyway.\n\nThrowing away the revindex may save some RAM if the pack\nnever gets reused (about 12 bytes per object). But it also\nwastes some CPU time (to regenerate the index) if the pack\ndoes get reused. It's hard to say which is more valuable,\nbut in either case, it happens very rarely (only when we\nrace with a simultaneous repack). Just leaving the revindex\nin place is simple and safe both for current and future\ncode.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn top of jk/pack-bitmap.\n\n pack-revindex.c | 11 -----------\n pack-revindex.h |  1 -\n sha1_file.c     |  1 -\n 3 files changed, 13 deletions(-)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 0bb13b1..5bd7c61 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -245,14 +245,3 @@ struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs)\n \n \treturn pridx->revindex + pos;\n }\n-\n-void discard_revindex(void)\n-{\n-\tif (pack_revindex_hashsz) {\n-\t\tint i;\n-\t\tfor (i = 0; i < pack_revindex_hashsz; i++)\n-\t\t\tfree(pack_revindex[i].revindex);\n-\t\tfree(pack_revindex);\n-\t\tpack_revindex_hashsz = 0;\n-\t}\n-}\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex 866ca9c..d737f98 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -15,6 +15,5 @@ struct pack_revindex *revindex_for_pack(struct packed_git *p);\n int find_revindex_position(struct pack_revindex *pridx, off_t ofs);\n \n struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs);\n-void discard_revindex(void);\n \n #endif\ndiff --git a/sha1_file.c b/sha1_file.c\nindex df89b57..45f9bb4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1279,7 +1279,6 @@ void prepare_packed_git(void)\n \n void reprepare_packed_git(void)\n {\n-\tdiscard_revindex();\n \tprepare_packed_git_run_once = 0;\n \tprepare_packed_git();\n }\n-- \n1.8.5.2.500.g8060133\n"}]}