{"thread":{"id":"11181","subject":"[RFC/PATCH] Add a --nosort option to pack-objects","startedAt":"2007-12-07T21:10:32Z","lastAt":"2007-12-08T08:54:29Z","messageCount":8,"participants":["Mike Hommey","Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"62318","messageId":"1197061832-8489-1-git-send-email-mh@glandium.org","threadId":"11181","inReplyTo":null,"subject":"[RFC/PATCH] Add a --nosort option to pack-objects","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-12-07T21:10:32Z","receivedAt":"2007-12-07T21:10:32Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"While most of the time the heuristics used by pack-objects to sort the\ngiven object list are satisfying enough, there are cases where it can be\nuseful for the user to sort the list with heuristics that would be better\nsuited.\n\nThe --nosort option disabled the internal sorting used by pack-objects,\nand runs the sliding window along the object list litterally as given on\nstdin.\n\nSigned-off-by: Mike Hommey \n---\n\n I would obviously add the appropriate documentation for this flag if this\n is accepted. I'll also try to send another documentation patch for\n pack-objects with some information compiled from Linus's explanation to my\n last message about pack-objects.\n\n builtin-pack-objects.c | 11 +++++++++--\n 1 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 4f44658..8bc2d5f 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -21,7 +21,7 @@\n \n static const char pack_usage[] = \"\\\n git-pack-objects [{ -q | --progress | --all-progress }] \\n\\\n-\t[--max-pack-size=N] [--local] [--incremental] \\n\\\n+\t[--max-pack-size=N] [--local] [--incremental] [--nosort]\\n\\\n \t[--window=N] [--window-memory=N] [--depth=N] \\n\\\n \t[--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \\n\\\n \t[--threads=N] [--non-empty] [--revs [--unpacked | --all]*] [--reflog] \\n\\\n@@ -64,6 +64,7 @@ static int non_empty;\n static int no_reuse_delta, no_reuse_object, keep_unreachable;\n static int local;\n static int incremental;\n+static int nosort;\n static int allow_ofs_delta;\n static const char *base_name;\n static int progress = 1;\n@@ -1715,7 +1716,9 @@ static void prepare_pack(int window, int depth)\n \t\tif (progress)\n \t\t\tprogress_state = start_progress(\"Compressing objects\",\n \t\t\t\t\t\t\tnr_deltas);\n-\t\tqsort(delta_list, n, sizeof(*delta_list), type_size_sort);\n+\t\tif (! nosort)\n+\t\t\tqsort(delta_list, n, sizeof(*delta_list),\n+\t\t\t\ttype_size_sort);\n \t\tll_find_deltas(delta_list, n, window+1, depth, &nr_done);\n \t\tstop_progress(&progress_state);\n \t\tif (nr_done != nr_deltas)\n@@ -1988,6 +1991,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tincremental = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"--nosort\", arg)) {\n+\t\t\tnosort = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!prefixcmp(arg, \"--compression=\")) {\n \t\t\tchar *end;\n \t\t\tint level = strtoul(arg+14, &end, 0);\n-- \n1.5.3.7\n"},{"id":"62320","messageId":"alpine.LFD.0.99999.0712071622150.555@xanadu.home","threadId":"11181","inReplyTo":"1197061832-8489-1-git-send-email-mh@glandium.org","subject":"Re: [RFC/PATCH] Add a --nosort option to pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-12-07T21:24:17Z","receivedAt":"2007-12-07T21:24:17Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 7 Dec 2007, Mike Hommey wrote:\n\n> While most of the time the heuristics used by pack-objects to sort the\n> given object list are satisfying enough, there are cases where it can be\n> useful for the user to sort the list with heuristics that would be better\n> suited.\n\nCould you please elaborate on those cases where the current heuristic \nwould be unsatisfactory?\n\n\nNicolas\n"},{"id":"62321","messageId":"7v4peu19kr.fsf@gitster.siamese.dyndns.org","threadId":"11181","inReplyTo":"1197061832-8489-1-git-send-email-mh@glandium.org","subject":"Re: [RFC/PATCH] Add a --nosort option to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-07T21:25:24Z","receivedAt":"2007-12-07T21:25:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> The --nosort option disabled the internal sorting used by pack-objects,\n> and runs the sliding window along the object list litterally as given on\n> stdin.\n\nI think this is a good way to give people an easier way to experiment.\n\nBut it makes me wonder if this is disabling too much, and if the list\nshould be sorted at least by type, as we won't delta different types of\nobjects against each other.\n\nAt the beginning of try_delta(), when we see that the next candidate is\nof a different type, we return -1 telling the caller that \"No object in\nthe window will ever be a good delta base for the current object, please\nabort\". This relies on the fact that we sort by type first, so I think\none of the following is necessary:\n \n (1) you weaken this check (return 0, saying \"This did not delta well but\n do not give up yet\"),\n\n (2) you document this well so that --nosort user will know, or\n\n (3) you sort --nosort input by type.\n\n> I would obviously add the appropriate documentation for this flag if this\n> is accepted. I'll also try to send another documentation patch for\n> pack-objects with some information compiled from Linus's explanation to my\n> last message about pack-objects.\n\nI need to rant here a bit.\n\nSometimes people say \"Here is my patch. If this is accepted, I'll add\ndocumentation and tests\". My reaction is, \"Don't you, as the person who\nproposes that change, believe in your patch deeply enough yourself to be\nwilling to perfect it, to make it suitable for consumption by the\ngeneral public, whether it is included in my tree or not? A change that\neven you do not believe in deeply enough probably to perfect would not\nbenefit the general public, so thanks but no thanks, I'll pass.\"\n\nFortunately we haven't had this problem too many times on this list.\n\nI would not have minded at all if you said:\n\n\tObviously, appropriate documentation and tests are needed before\n\tinclusion, but I am sending this out primarily to seek opinions\n\tfrom the list to make sure this is going in the right direction,\n\tiow, this is an RFC.\n\nWhat bugged me was the phrase \"if this is accepted\".\n"},{"id":"62329","messageId":"7vzlwmyyms.fsf@gitster.siamese.dyndns.org","threadId":"11181","inReplyTo":"7v4peu19kr.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC/PATCH] Add a --nosort option to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-07T21:37:47Z","receivedAt":"2007-12-07T21:37:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Mike Hommey writes:\n>\n>> The --nosort option disabled the internal sorting used by pack-objects,\n>> and runs the sliding window along the object list litterally as given on\n>> stdin.\n>\n> I think this is a good way to give people an easier way to experiment.\n\nActually, I take the half of this back.\n\nWe need a pair of two good sort orders. The order of objects fed to\npack-objects determines the final layout of the resulting pack, and\nusing something like the \"recency order\" we currently have is to\noptimize the layout in the resulting pack for typical access patterns.\n\nBy using your --nosort, you may be able to influence the deltification\nprocess, but the order you use will most likely not match the access\npattern of the resulting pack. So it will be an easy, quick-and-dirty\nway to _experiment_ how the deltification sort order affects the final\npack size, but I suspect that the resulting \"small\" pack won't be useful\nin the real life.\n"},{"id":"62340","messageId":"20071207214426.GA13170@glandium.org","threadId":"11181","inReplyTo":"alpine.LFD.0.99999.0712071622150.555@xanadu.home","subject":"Re: [RFC/PATCH] Add a --nosort option to pack-objects","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-12-07T21:44:26Z","receivedAt":"2007-12-07T21:44:26Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Dec 07, 2007 at 04:24:17PM -0500, Nicolas Pitre wrote:\n> On Fri, 7 Dec 2007, Mike Hommey wrote:\n> \n> > While most of the time the heuristics used by pack-objects to sort the\n> > given object list are satisfying enough, there are cases where it can be\n> > useful for the user to sort the list with heuristics that would be better\n> > suited.\n> \n> Could you please elaborate on those cases where the current heuristic \n> would be unsatisfactory?\n\nI imagine it could be useful when importing a huge tree in the first commit,\nwhen some data in the tree is redundant with (or similar to) others in the\nsame tree. I guess there could be some other VCS use-cases.\n\nThe real case where I've been using this is that I use git to store my debian\nbuild logs in an efficient manner, and having a custom-sorted list of objects\nends up being much faster and less memory consuming than using a huge\nwindow (and 1GB of logs became less than 10MB).\n\nMike\n"},{"id":"62335","messageId":"20071207214627.GB13170@glandium.org","threadId":"11181","inReplyTo":"7v4peu19kr.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC/PATCH] Add a --nosort option to pack-objects","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-12-07T21:46:27Z","receivedAt":"2007-12-07T21:46:27Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Dec 07, 2007 at 01:25:24PM -0800, Junio C Hamano wrote:\n> I need to rant here a bit.\n> \n> Sometimes people say \"Here is my patch. If this is accepted, I'll add\n> documentation and tests\". My reaction is, \"Don't you, as the person who\n> proposes that change, believe in your patch deeply enough yourself to be\n> willing to perfect it, to make it suitable for consumption by the\n> general public, whether it is included in my tree or not? A change that\n> even you do not believe in deeply enough probably to perfect would not\n> benefit the general public, so thanks but no thanks, I'll pass.\"\n\nAs you can seen from my other message, I'm *actually* not sure this is\nreally material for git as a VCS. I will add documentation unrelated to\n--nosort to pack-objects anyways.\n\nMike\n"},{"id":"62344","messageId":"alpine.LFD.0.99999.0712071707250.555@xanadu.home","threadId":"11181","inReplyTo":"20071207214627.GB13170@glandium.org","subject":"Re: [RFC/PATCH] Add a --nosort option to pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-12-07T22:20:59Z","receivedAt":"2007-12-07T22:20:59Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 7 Dec 2007, Mike Hommey wrote:\n\n> As you can seen from my other message, I'm *actually* not sure this is\n> really material for git as a VCS. I will add documentation unrelated to\n> --nosort to pack-objects anyways.\n\nWell, I have serious doubts about this patch in the first place.\n\nI think it is simply unneeded.\n\nIf you want pack-objects not to change the sort order because you have \nsome sorting of your own, externally implemented, then you simply have \nto run git-pack-objects feeding it the list of object SHA1s along with a \ntag of your own which will effectively impose the sorting you want, \nbased on that tag.\n\nObjects with the same tag will still be sorted amongst themselves which \nis still a good thing.\n\nfor example, you may have something like:\n\n\tgit rev-list --all --objects |\n\tsed -e 's|foo/logs/.*|LOGS|' |\n\tgit pack-objects ...\n\nThis will effectively cluster all foo/logs/* files together for delta \ncompression regardless of their actual name. Maybe that's what you \nreally want?\n\n\nNicolas\n"},{"id":"62393","messageId":"20071208085429.GB13432@glandium.org","threadId":"11181","inReplyTo":"alpine.LFD.0.99999.0712071707250.555@xanadu.home","subject":"Re: [RFC/PATCH] Add a --nosort option to pack-objects","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2007-12-08T08:54:29Z","receivedAt":"2007-12-08T08:54:29Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Dec 07, 2007 at 05:20:59PM -0500, Nicolas Pitre wrote:\n> On Fri, 7 Dec 2007, Mike Hommey wrote:\n> \n> > As you can seen from my other message, I'm *actually* not sure this is\n> > really material for git as a VCS. I will add documentation unrelated to\n> > --nosort to pack-objects anyways.\n> \n> Well, I have serious doubts about this patch in the first place.\n> \n> I think it is simply unneeded.\n> \n> If you want pack-objects not to change the sort order because you have \n> some sorting of your own, externally implemented, then you simply have \n> to run git-pack-objects feeding it the list of object SHA1s along with a \n> tag of your own which will effectively impose the sorting you want, \n> based on that tag.\n> \n> Objects with the same tag will still be sorted amongst themselves which \n> is still a good thing.\n> \n> for example, you may have something like:\n> \n> \tgit rev-list --all --objects |\n> \tsed -e 's|foo/logs/.*|LOGS|' |\n> \tgit pack-objects ...\n> \n> This will effectively cluster all foo/logs/* files together for delta \n> compression regardless of their actual name. Maybe that's what you \n> really want?\n\nI've been thinking about this, but I'm not sure the list won't be\nmixed up with the rest of the sort... I'll just try to see what the\nsorted list look like...\n\nMike\n"}]}