{"thread":{"id":"4375","subject":"[PATCH] handle concurrent pruning of packed objects","startedAt":"2006-06-02T15:32:23Z","lastAt":"2006-06-02T17:47:07Z","messageCount":7,"participants":["Jeff King","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"21103","messageId":"20060602153223.GA4223@coredump.intra.peff.net","threadId":"4375","inReplyTo":null,"subject":"[PATCH] handle concurrent pruning of packed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-06-02T15:32:23Z","receivedAt":"2006-06-02T15:32:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch causes read_sha1_file and sha1_object_info to re-examine the\nlist of packs if an object cannot be found. It works by re-running\nprepare_packed_git() after an object fails to be found.\n\nIt does not attempt to clean up the old pack list. Old packs which are in\nuse can continue to be used (until unused by lru selection). New packs\nare placed at the front of the list and will thus be examined before old\npacks.\n\nSigned-off-by: Jeff King \n---\n\nThis is a repost, since there was no response last time. Linus\nindicated this approach was reasonable; see:\n \n\nI tested this by making a simple repo with three commits: a, b, and c.\nI ran git diff-tree --stdin, and then did the following:\n 1. fed 'a b' to diff-tree\n 2. ran git repack -a -d\n 3. fed 'b c' to diff-tree\nVanilla git complains about the lack of object, whereas this version\nprovides the correct diff-tree output.\n\n sha1_file.c | 24 ++++++++++++++++++------\n 1 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f77c189..696e53f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -626,12 +626,12 @@ static void prepare_packed_git_one(char \n \tclosedir(dir);\n }\n \n+static int prepare_packed_git_run_once = 0;\n void prepare_packed_git(void)\n {\n-\tstatic int run_once = 0;\n \tstruct alternate_object_database *alt;\n \n-\tif (run_once)\n+\tif (prepare_packed_git_run_once)\n \t\treturn;\n \tprepare_packed_git_one(get_object_directory(), 1);\n \tprepare_alt_odb();\n@@ -640,7 +640,13 @@ void prepare_packed_git(void)\n \t\tprepare_packed_git_one(alt->base, 0);\n \t\talt->name[-1] = '/';\n \t}\n-\trun_once = 1;\n+\tprepare_packed_git_run_once = 1;\n+}\n+\n+static void reprepare_packed_git(void)\n+{\n+\tprepare_packed_git_run_once = 0;\n+\tprepare_packed_git();\n }\n \n int check_sha1_signature(const unsigned char *sha1, void *map, unsigned long size, const char *type)\n@@ -1212,9 +1218,12 @@ int sha1_object_info(const unsigned char\n \tif (!map) {\n \t\tstruct pack_entry e;\n \n-\t\tif (!find_pack_entry(sha1, &e))\n-\t\t\treturn error(\"unable to find %s\", sha1_to_hex(sha1));\n-\t\treturn packed_object_info(&e, type, sizep);\n+\t\tif (find_pack_entry(sha1, &e))\n+\t\t\treturn packed_object_info(&e, type, sizep);\n+\t\treprepare_packed_git();\n+\t\tif (find_pack_entry(sha1, &e))\n+\t\t\treturn packed_object_info(&e, type, sizep);\n+\t\treturn error(\"unable to find %s\", sha1_to_hex(sha1));\n \t}\n \tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n \t\tstatus = error(\"unable to unpack %s header\",\n@@ -1256,6 +1265,9 @@ void * read_sha1_file(const unsigned cha\n \t\tmunmap(map, mapsize);\n \t\treturn buf;\n \t}\n+\treprepare_packed_git();\n+\tif (find_pack_entry(sha1, &e))\n+\t\treturn read_packed_sha1(sha1, type, size);\n \treturn NULL;\n }\n \n-- \n1.3.3.g331f\n\n-\nTo unsubscribe from this list: send the line \"unsubscribe git\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html\n\n----- End forwarded message -----\n"},{"id":"21106","messageId":"7vwtbzblkf.fsf@assigned-by-dhcp.cox.net","threadId":"4375","inReplyTo":"20060602153223.GA4223@coredump.intra.peff.net","subject":"Re: [PATCH] handle concurrent pruning of packed objects","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-02T15:53:52Z","receivedAt":"2006-06-02T15:53:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is a repost, since there was no response last time. Linus\n> indicated this approach was reasonable; see:\n> \n\nI haven't forgotten about it, but I have been sick.\n\nI am uncertain about not re-examining the packs it originally\nthought it had. By prepending the new ones (and the same old\nsurviving ones) at the beginning you are effectively hiding the\nold packs, which sounds reasonable in the usual case.\n\nAlso I suspect this might have funny interaction with the case\nwhere there are hand-added packs (see how verify-pack does it).\nWe do not silently \"fix\" missing object problems we discover\nthere.\n"},{"id":"21108","messageId":"7vodxbbl4p.fsf@assigned-by-dhcp.cox.net","threadId":"4375","inReplyTo":"7vwtbzblkf.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] handle concurrent pruning of packed objects","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-02T16:03:18Z","receivedAt":"2006-06-02T16:03:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Also I suspect this might have funny interaction with the case\n> where there are hand-added packs (see how verify-pack does it).\n> We do not silently \"fix\" missing object problems we discover\n> there.\n\nThe last sentence should read 'We do not want to silently...'.\nSorry.\n"},{"id":"21109","messageId":"20060602160456.GA8957@coredump.intra.peff.net","threadId":"4375","inReplyTo":"7vwtbzblkf.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] handle concurrent pruning of packed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-06-02T16:04:56Z","receivedAt":"2006-06-02T16:04:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2006 at 08:53:52AM -0700, Junio C Hamano wrote:\n\n> I am uncertain about not re-examining the packs it originally\n> thought it had. By prepending the new ones (and the same old\n> surviving ones) at the beginning you are effectively hiding the\n> old packs, which sounds reasonable in the usual case.\n\nThat shouldn't make a difference for correctness, even if the old packs\nare still there. If you have an object in two packs, then it doesn't\nmatter which one you pull it from. The main impacts I can think of are:\n 1. The old pack may already be mapped, and it would be more efficient\n to use it. However, the new pack will be mapped on first use, so it\n will be used from then on.\n 2. The pack list can grow without bound. However, for this to matter,\n you'd have to do many prunes during the course of a single git\n command.\n\n> Also I suspect this might have funny interaction with the case\n> where there are hand-added packs (see how verify-pack does it).\n> We do not silently \"fix\" missing object problems we discover\n> there.\n\nI will take a look at this.\n\n-Peff\n"},{"id":"21110","messageId":"7vk67zbksv.fsf@assigned-by-dhcp.cox.net","threadId":"4375","inReplyTo":"20060602160456.GA8957@coredump.intra.peff.net","subject":"Re: [PATCH] handle concurrent pruning of packed objects","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-02T16:10:24Z","receivedAt":"2006-06-02T16:10:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That shouldn't make a difference for correctness, even if the old packs\n> are still there. If you have an object in two packs, then it doesn't\n> matter which one you pull it from. The main impacts I can think of are:\n> 1. The old pack may already be mapped, and it would be more efficient\n> to use it. However, the new pack will be mapped on first use, so it\n> will be used from then on.\n> 2. The pack list can grow without bound. However, for this to matter,\n> you'd have to do many prunes during the course of a single git\n> command.\n\nI agree 100% on \"shouldn't\" part. What I wonder is if everybody\nworks correctly if we mmap the same file (all available .idx are\nmapped all the time, and we map .pack LRU) twice. But I realize\nwe have NO_MMAP configuration for unfortunate platforms to work\nit around so that wouldn't be a big deal.\n"},{"id":"21111","messageId":"20060602164932.GA10216@coredump.intra.peff.net","threadId":"4375","inReplyTo":"7vk67zbksv.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] sha1_file: avoid re-preparing duplicate packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-06-02T16:49:32Z","receivedAt":"2006-06-02T16:49:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When adding packs, skip the pack if we already have it in the packed_git\nlist. This might happen if we are re-preparing our packs because of a\nmissing object.\n---\n\nOn Fri, Jun 02, 2006 at 09:10:24AM -0700, Junio C Hamano wrote:\n\n> I agree 100% on \"shouldn't\" part. What I wonder is if everybody\n> works correctly if we mmap the same file (all available .idx are\n\nThis patch avoids duplicates in the packed_git list. It's not necessary\nunder Linux, at least, but it just seems cleaner, and it's simple to do.\nThe list might still have packs that are now gone. I didn't want to\npurge anything from the packed_git list since I'm not clear on whether\nother code might have pointers into the mmap'd portion.\n\n sha1_file.c | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 696e53f..aea0f40 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -617,6 +617,12 @@ static void prepare_packed_git_one(char \n \n \t\t/* we have .idx. Is it a file we can map? */\n \t\tstrcpy(path + len, de->d_name);\n+\t\tfor (p = packed_git; p; p = p->next) {\n+\t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n+\t\t\t\tbreak;\n+\t\t}\n+\t\tif (p)\n+\t\t\tcontinue;\n \t\tp = add_packed_git(path, len + namelen, local);\n \t\tif (!p)\n \t\t\tcontinue;\n-- \n1.3.3.gfb825\n"},{"id":"21113","messageId":"Pine.LNX.4.64.0606021045580.5498@g5.osdl.org","threadId":"4375","inReplyTo":"20060602164932.GA10216@coredump.intra.peff.net","subject":"Re: [PATCH] sha1_file: avoid re-preparing duplicate packs","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-06-02T17:47:07Z","receivedAt":"2006-06-02T17:47:07Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 2 Jun 2006, Jeff King wrote:\n> \n> This patch avoids duplicates in the packed_git list. It's not necessary\n> under Linux, at least, but it just seems cleaner, and it's simple to do.\n\nI think it's a good idea even under Linux, since on 32-bit machines, one \nof the constraints we have may simply be virtual memory space to map \nthings into. With big packs, avoiding mapping them twice is a good idea.\n\n\t\tLinus\n"}]}