{"thread":{"id":"8904","subject":"git-gc \"--aggressive\" somewhat broken","startedAt":"2007-07-06T20:19:41Z","lastAt":"2007-07-09T03:46:44Z","messageCount":4,"participants":["Linus Torvalds","Nicolas Pitre"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"46658","messageId":"alpine.LFD.0.999.0707061310390.8278@woody.linux-foundation.org","threadId":"8904","inReplyTo":null,"subject":"git-gc \"--aggressive\" somewhat broken","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-06T20:19:41Z","receivedAt":"2007-07-06T20:19:41Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis flag seems misdesigned as-is.\n\nIt makes the window bigger (good, and aggressive), but it also enabled \n\"-f\".\n\nWhich sometimes causes _worse_ packing.\n\nIn particular, there's a gcc import at\n\n\tgit://git.infradead.org/toolchain/gcc.git\n\nwhich apparently packs down quite nicely to a 450MB pack, but when it gets \nrepacked \"aggressively\", it blows up to 1.4GB. The reason? That pack was \ngenerated using --depth=100 --window=100 (I think).\n\nSo under certain circumstances, \"--aggressive\" is anything _but_ \naggressive, and actually causes much worse packing.\n\nIf we want to be really aggressive, we migth decide to pass a new flag to \npack-objects that does something closer to what \"aggressive\" was meant to \ndo: it would use existing delta's if they exist, but _despite_ existing it \ncould look if there are even better choices.\n\nSo right now we have:\n\n - default behaviour:\n\n   always re-use existing deltas, don't look at alternatives at all.\n\n   This is optimal for CPU/memory/IO usage, and is generally a good idea\n\n - \"-f\" (and as a result, the current bad \"git gc --aggressive\"):\n\n   never re-use existing deltas, always look for new ones.\n\n   This is good if you have reason to believe that the old choices are \n   bad, or you need to force re-generation of deltas (because you want to \n   force a new pack-file format, for example)\n\nand the missing piece might be\n\n - \"git pack-objects --aggressive\":\n\n   re-use existing deltas _and_ look for even better ones.\n\n   This is good if all you're looking for is better packing.\n\nHmm?\n\n\t\tLinus\n"},{"id":"46664","messageId":"alpine.LFD.0.999.0707061512550.8278@woody.linux-foundation.org","threadId":"8904","inReplyTo":"alpine.LFD.0.999.0707061310390.8278@woody.linux-foundation.org","subject":"Re: git-gc \"--aggressive\" somewhat broken","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-06T22:17:25Z","receivedAt":"2007-07-06T22:17:25Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 6 Jul 2007, Linus Torvalds wrote:\n> \n> If we want to be really aggressive, we migth decide to pass a new flag to \n> pack-objects that does something closer to what \"aggressive\" was meant to \n> do: it would use existing delta's if they exist, but _despite_ existing it \n> could look if there are even better choices.\n\nThis is a totally untested patch that may or may not work.\n\nThe reason I say \"may not work\" is not just that I haven't really tested \nit, it's also because I haven't thought it through very well.\n\nIn particular, does this possibly cause infinite loops of delta chains? \nProbably. It would need code to explicitly make sure that we don't do \nthat, but I couldn't even convince myself as to why we might not hit that \ncase _already_ with delta re-use, so maybe there's something going that \nprotects us against it.\n\nThe patch itself is trivial, except for hunk #2, which fixes up the fact \nthat we didn't fill in the \"entry->size\" correctly for a delta entry (we \nleft it at the *delta* size). It didn't use to matter, since the entry \nsize wasn't ever used for anything, I think (with the possible exception \nof the object sorting).\n\nAnyway, consider this a starting point for somebody else who wants to \nreally try to look into this. But I do think that \"git gc --aggressive\" is \nbroken as it stands now.\n\n\t\tLinus\n\n---\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 3d396ca..89e9900 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -57,6 +57,7 @@ static struct object_entry *objects;\n static struct object_entry **written_list;\n static uint32_t nr_objects, nr_alloc, nr_result, nr_written;\n \n+static int aggressive;\n static int non_empty;\n static int no_reuse_delta, no_reuse_object;\n static int local;\n@@ -1179,6 +1180,8 @@ static void check_object(struct object_entry *entry)\n \t\t\tentry->delta = base_entry;\n \t\t\tentry->delta_sibling = base_entry->delta_child;\n \t\t\tbase_entry->delta_child = entry;\n+\t\t\tentry->size = get_size_from_delta(p, &w_curs,\n+\t\t\t\tentry->in_pack_offset + entry->in_pack_header_size);\n \t\t\tunuse_pack(&w_curs);\n \t\t\treturn;\n \t\t}\n@@ -1425,7 +1428,7 @@ static void find_deltas(struct object_entry **list, int window, int depth)\n \t\tif (progress)\n \t\t\tdisplay_progress(&progress_state, processed);\n \n-\t\tif (entry->delta)\n+\t\tif (entry->delta && !aggressive)\n \t\t\t/* This happens if we decided to reuse existing\n \t\t\t * delta from a pack.  \"!no_reuse_delta &&\" is implied.\n \t\t\t */\n@@ -1760,6 +1763,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tdie(\"bad %s\", arg);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"--aggressive\", arg)) {\n+\t\t\taggressive = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(pack_usage);\n \t}\n \n"},{"id":"46667","messageId":"alpine.LFD.0.999.0707061812410.8278@woody.linux-foundation.org","threadId":"8904","inReplyTo":"alpine.LFD.0.999.0707061512550.8278@woody.linux-foundation.org","subject":"Re: git-gc \"--aggressive\" somewhat broken","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-07T01:16:15Z","receivedAt":"2007-07-07T01:16:15Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 6 Jul 2007, Linus Torvalds wrote:\n> \n> This is a totally untested patch that may or may not work.\n\nAnd by that I obviously mean \"does not work\".\n\nI only tested on the gcc thing, and it causes an assertion error when \ntrying to actually write out the deltas (\"corrupt packed object\").\n\nI'm sure somebody can figure it out, \n\n\t\tLinus\n"},{"id":"46819","messageId":"alpine.LFD.0.999.0707082341460.26459@xanadu.home","threadId":"8904","inReplyTo":"alpine.LFD.0.999.0707061512550.8278@woody.linux-foundation.org","subject":"Re: git-gc \"--aggressive\" somewhat broken","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-07-09T03:46:44Z","receivedAt":"2007-07-09T03:46:44Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 6 Jul 2007, Linus Torvalds wrote:\n\n> \n> \n> On Fri, 6 Jul 2007, Linus Torvalds wrote:\n> > \n> > If we want to be really aggressive, we migth decide to pass a new flag to \n> > pack-objects that does something closer to what \"aggressive\" was meant to \n> > do: it would use existing delta's if they exist, but _despite_ existing it \n> > could look if there are even better choices.\n> \n> This is a totally untested patch that may or may not work.\n> \n> The reason I say \"may not work\" is not just that I haven't really tested \n> it, it's also because I haven't thought it through very well.\n> \n> In particular, does this possibly cause infinite loops of delta chains? \n> Probably. It would need code to explicitly make sure that we don't do \n> that, but I couldn't even convince myself as to why we might not hit that \n> case _already_ with delta re-use, so maybe there's something going that \n> protects us against it.\n\nThere is.\n\n> Anyway, consider this a starting point for somebody else who wants to \n> really try to look into this.\n\nThis is a real teaser.  But I have real work to do if I want to leave on \nvacation this summer, and therefore I'll then be on vacation.  So if I \nend up looking at it myself it will be in September.\n\n\nNicolas\n"}]}