{"thread":{"id":"7573","subject":"[PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","startedAt":"2007-04-08T23:22:56Z","lastAt":"2007-04-09T23:22:30Z","messageCount":10,"participants":["Dana How","Junio C Hamano","Nicolas Pitre","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"38892","messageId":"46197950.3080804@gmail.com","threadId":"7573","inReplyTo":null,"subject":"[PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-08T23:22:56Z","receivedAt":"2007-04-08T23:22:56Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"\nAdd our own version of the one in fast-import.c here.\nWill need this later to correct an incorrect object\ncount in the pack header.\n\nSigned-off-by: Dana How \n---\n builtin-pack-objects.c | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 40 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 8415549..7ab0712 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -518,6 +518,46 @@ static off_t write_one(struct sha1file *f,\n \treturn offset + write_object(f, e);\n }\n \n+/*\n+ * Move this, the version in fast-import.c,\n+ * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ?\n+ */\n+static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,\n+\t\t\t\tchar *pack_name, uint32_t object_count)\n+{\n+\tstatic const int buf_sz = 128 * 1024;\n+\tSHA_CTX c;\n+\tstruct pack_header hdr;\n+\tchar *buf;\n+\n+\tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n+\t\tdie(\"Failed seeking to start: %s\", strerror(errno));\n+\tif (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n+\t\tdie(\"Unable to reread header of %s\", pack_name);\n+\tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n+\t\tdie(\"Failed seeking to start: %s\", strerror(errno));\n+\thdr.hdr_entries = htonl(object_count);\n+\twrite_or_die(pack_fd, &hdr, sizeof(hdr));\n+\n+\tSHA1_Init(&c);\n+\tSHA1_Update(&c, &hdr, sizeof(hdr));\n+\n+\tbuf = xmalloc(buf_sz);\n+\tfor (;;) {\n+\t\tsize_t n = xread(pack_fd, buf, buf_sz);\n+\t\tif (!n)\n+\t\t\tbreak;\n+\t\tif (n < 0)\n+\t\t\tdie(\"Failed to checksum %s\", pack_name);\n+\t\tSHA1_Update(&c, buf, n);\n+\t}\n+\tfree(buf);\n+\n+\tSHA1_Final(pack_file_sha1, &c);\n+\twrite_or_die(pack_fd, pack_file_sha1, 20);\n+\tclose(pack_fd);\n+}\n+\n typedef int (*entry_sort_t)(const struct object_entry *, const struct object_entry *);\n \n static entry_sort_t current_sort;\n-- \n1.5.1.89.g8abf0\n"},{"id":"38898","messageId":"7vwt0m76rv.fsf@assigned-by-dhcp.cox.net","threadId":"7573","inReplyTo":"46197950.3080804@gmail.com","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-09T00:04:04Z","receivedAt":"2007-04-09T00:04:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dana How writes:\n\n> +/*\n> + * Move this, the version in fast-import.c,\n> + * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ?\n> + */\n> +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,\n> +\t\t\t\tchar *pack_name, uint32_t object_count)\n> +{\n\nIndeed that is a very good point.\n\nI admit I did not notice we already had the duplication between\nfast-import.c and index-pack.c\n\nShawn, Nico, what do you think? Wouldn't it be better to\nrefactor them first, independent of Dana's series?\n"},{"id":"38901","messageId":"alpine.LFD.0.98.0704082012220.28181@xanadu.home","threadId":"7573","inReplyTo":"7vwt0m76rv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-09T00:18:23Z","receivedAt":"2007-04-09T00:18:23Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 8 Apr 2007, Junio C Hamano wrote:\n\n> Dana How writes:\n> \n> > +/*\n> > + * Move this, the version in fast-import.c,\n> > + * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ?\n> > + */\n> > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,\n> > +\t\t\t\tchar *pack_name, uint32_t object_count)\n> > +{\n> \n> Indeed that is a very good point.\n> \n> I admit I did not notice we already had the duplication between\n> fast-import.c and index-pack.c\n> \n> Shawn, Nico, what do you think? Wouldn't it be better to\n> refactor them first, independent of Dana's series?\n\nProbably, yes. But probably not in sha1_file.c though. This file is \ngetting a bit large already, and it deals with pack reading only not \npack writing.\n\nI think another file with common pack writing functions could be \ncreated. Pack index writing is another item that is currently \nduplicated in pack-objects and index-pack for example.\n\n\nNicolas\n"},{"id":"38940","messageId":"20070409173858.GT5436@spearce.org","threadId":"7573","inReplyTo":"alpine.LFD.0.98.0704082012220.28181@xanadu.home","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-09T17:38:58Z","receivedAt":"2007-04-09T17:38:58Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Sun, 8 Apr 2007, Junio C Hamano wrote:\n> \n> > Dana How writes:\n> > \n> > > +/*\n> > > + * Move this, the version in fast-import.c,\n> > > + * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ?\n> > > + */\n> > > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,\n> > > +\t\t\t\tchar *pack_name, uint32_t object_count)\n> > > +{\n> > \n> > Indeed that is a very good point.\n> > \n> > I admit I did not notice we already had the duplication between\n> > fast-import.c and index-pack.c\n> > \n> > Shawn, Nico, what do you think? Wouldn't it be better to\n> > refactor them first, independent of Dana's series?\n> \n> Probably, yes. But probably not in sha1_file.c though. This file is \n> getting a bit large already, and it deals with pack reading only not \n> pack writing.\n> \n> I think another file with common pack writing functions could be \n> created. Pack index writing is another item that is currently \n> duplicated in pack-objects and index-pack for example.\n\nI agree entirely. And I'd like to see that refactoring occur\nbefore this series, or as part of it. At least for the nr_objects\ncorrection routine. To be honest we should have done that when\nfast-import.c and index-pack.c both needed that logic, but we didn't.\nI don't remember whose version showed up first in Junio's tree\n(I think it was index-pack.c) but the other one (probably me with\nfast-import.c) should have done the refactoring then.\n\nWe already have *waaay* too many functions that know packfile\nstructure. I'd like to see that decline, but unfortunately a number\nof them are using rather specialized data structures so it makes\nthings somewhat difficult.\n\nFor example, writing objects to a packfile: we have 3\nimplementations. fast-import.c doesn't use sha1write_compressed\nbecause that was a waste of time to compute the SHA_CTX when we\nknow we have to go back and fixup nr_objects. It also doesn't use\nit because fast-import.c's pack-splitting logic is based on the\nfinal object size, not the starting offset. It does the deflate\nitself, decides if the end of the object will overflow, and if so,\njumps to a new packfile.\n\n-- \nShawn.\n"},{"id":"38942","messageId":"alpine.LFD.0.98.0704091404530.28181@xanadu.home","threadId":"7573","inReplyTo":"20070409173858.GT5436@spearce.org","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-09T18:30:49Z","receivedAt":"2007-04-09T18:30:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 9 Apr 2007, Shawn O. Pearce wrote:\n\n> Nicolas Pitre wrote:\n> > I think another file with common pack writing functions could be \n> > created. Pack index writing is another item that is currently \n> > duplicated in pack-objects and index-pack for example.\n> \n> I agree entirely. And I'd like to see that refactoring occur\n> before this series, or as part of it. At least for the nr_objects\n> correction routine.\n\nThat's easy enough for the nr_objects correction case.\n\nThe next obvious candidate would be index generation, but I'd wait a bit \nfor all features in progress to be merged and stabilize first.\n\n> For example, writing objects to a packfile: we have 3\n> implementations. fast-import.c doesn't use sha1write_compressed\n> because that was a waste of time to compute the SHA_CTX when we\n> know we have to go back and fixup nr_objects. It also doesn't use\n> it because fast-import.c's pack-splitting logic is based on the\n> final object size, not the starting offset. It does the deflate\n> itself, decides if the end of the object will overflow, and if so,\n> jumps to a new packfile.\n\nI'd be really tempted to create a pack v4 which only change is to still \nhave the pack header at the beginning of the pack like we do today, but \ninclude the header in the pack SHA1 computation at the end of the stream \nonly. This way the pack SHA1 could be computed as the pack is \ngenerated, and the header fixed up without having to read the entire \npack back. I think it was Geert Bosch who proposed this and it makes \ntons of sense IMHO.\n\n\nNicolas\n"},{"id":"38955","messageId":"20070409184039.GZ5436@spearce.org","threadId":"7573","inReplyTo":"alpine.LFD.0.98.0704091404530.28181@xanadu.home","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-09T18:40:39Z","receivedAt":"2007-04-09T18:40:39Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> I'd be really tempted to create a pack v4 which only change is to still \n> have the pack header at the beginning of the pack like we do today, but \n> include the header in the pack SHA1 computation at the end of the stream \n> only. This way the pack SHA1 could be computed as the pack is \n> generated, and the header fixed up without having to read the entire \n> pack back. I think it was Geert Bosch who proposed this and it makes \n> tons of sense IMHO.\n\nYes. If we really are heading in this direction of needing to\ncorrect object counts, we should make that change. Its trivial\nto hang onto that header for the duration of the rest of the data\nprocessing, and tack it onto the end for final SHA-1 computation.\n\nSince pack v4 looks like it will be a dev cycle longer than these\nindex format changes and pack-splitting changes, I have to say I\nagree with you. Lets move \"pack v4\" back to \"pack v5\" and make v4\njust a shift of where the header is included in the SHA-1.\n\nWe're heading where I said I didn't want to go, which is two\nfile format changes in 2007. But I think that ship has already\nsailed... Folks need support for larger repositories now, and pack\nv4/v5 (whatever you call our dictionary work) just isn't ready.\nNor does it solve the big packfile problems that this current work\nis addressing.\n\n-- \nShawn.\n"},{"id":"38963","messageId":"56b7f5510704091211i3d101cafq371fbb96ca27238b@mail.gmail.com","threadId":"7573","inReplyTo":"20070409184039.GZ5436@spearce.org","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-09T19:11:26Z","receivedAt":"2007-04-09T19:11:26Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/9/07, Shawn O. Pearce wrote:\n> Nicolas Pitre wrote:\n> > I'd be really tempted to create a pack v4 which only change is to still\n> > have the pack header at the beginning of the pack like we do today, but\n> > include the header in the pack SHA1 computation at the end of the stream\n> > only. This way the pack SHA1 could be computed as the pack is\n> > generated, and the header fixed up without having to read the entire\n> > pack back. I think it was Geert Bosch who proposed this and it makes\n> > tons of sense IMHO.\n>\n> Yes. If we really are heading in this direction of needing to\n> correct object counts, we should make that change. Its trivial\n> to hang onto that header for the duration of the rest of the data\n> processing, and tack it onto the end for final SHA-1 computation.\n\nI like the property that when an SHA-1 appears at the end of a file,\nit is a checksum of every byte before it. The ideas above are a\ndeparture from that. Do we want this rule to be different for each file type?\n\nWouldn't the following address the \"object count unknown\nat the start of sequential pack writing\" problem:\n Write 0 for object count in the header. This is a flag to look for\n another header of same format just before the final SHA-1 which\n has the correct count. The SHA-1 is still a checksum of everything\n before it and no seeking/rewriting is needed on generation. When\n reading the object count from a .pack file, you might need to add\n xread(pack_fd, &header, sizeof(header));\n+ if (!header.object_count) {\n+ lseek(pack_fd, -20-sizeof(header), SEEK_END);\n+ xread(pack_fd, &header, sizeof(header);\n+ }\n Or maybe you want this before the object_list_sha1 instead (20->40).\n\nFinally, when I generate several 2GB split packfiles, I do notice\nthe slight delay for fixup_header_footer(), and I do think it's a bit\nugly, but in quantitative terms it's an insignificant part of a long\noperation that's infrequently performed. Does this need to be\noptimized at all?\n\nThanks,\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"},{"id":"38967","messageId":"alpine.LFD.0.98.0704091525260.28181@xanadu.home","threadId":"7573","inReplyTo":"56b7f5510704091211i3d101cafq371fbb96ca27238b@mail.gmail.com","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-09T19:33:50Z","receivedAt":"2007-04-09T19:33:50Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 9 Apr 2007, Dana How wrote:\n\n> Wouldn't the following address the \"object count unknown\n> at the start of sequential pack writing\" problem:\n> Write 0 for object count in the header. This is a flag to look for\n> another header of same format just before the final SHA-1 which\n> has the correct count. The SHA-1 is still a checksum of everything\n> before it and no seeking/rewriting is needed on generation.\n\nNo. You really wants to know up front how many objects a pack contains \nwhen streaming it. And this is not only for packs written to stdout.\n\n> Finally, when I generate several 2GB split packfiles, I do notice\n> the slight delay for fixup_header_footer(), and I do think it's a bit\n> ugly, but in quantitative terms it's an insignificant part of a long\n> operation that's infrequently performed. Does this need to be\n> optimized at all?\n\nMaybe, maybe not. That depends how much data we think GIT could be used \nto manage in the future. With a 1TB pack file you definitely want to \noptimize that case.\n\nOTOH this could wait for the real pack v4 too.\n\n\nNicolas\n"},{"id":"38966","messageId":"56b7f5510704091438v4d42a7efq9b241bdc31affd08@mail.gmail.com","threadId":"7573","inReplyTo":"alpine.LFD.0.98.0704091525260.28181@xanadu.home","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-09T21:38:57Z","receivedAt":"2007-04-09T21:38:57Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/9/07, Nicolas Pitre wrote:\n> On Mon, 9 Apr 2007, Dana How wrote:\n> > Wouldn't the following address the \"object count unknown\n> > at the start of sequential pack writing\" problem:\n> > Write 0 for object count in the header. This is a flag to look for\n> > another header of same format just before the final SHA-1 which\n> > has the correct count. The SHA-1 is still a checksum of everything\n> > before it and no seeking/rewriting is needed on generation.\n>\n> No. You really wants to know up front how many objects a pack contains\n> when streaming it. And this is not only for packs written to stdout.\nOK, let me ask a dumb question and flog one last additional obvious idea.\n\nDoes your wanting to know stem from more than wanting\nto stick to one malloc of all the object info at once?\n\nMy suggestion quoted above is actually a change to the .pack format.\nWith all the other ideas for .pack format changes floating around,\nlet me withdraw that and suggest a simpler one: write a \"0\" in the header,\nand terminate the pack with a sentinel in object format before the final SHA-1s.\nThe sentinel would be type=OBJ_NONE/length=0, i.e. a null byte.\n\"Not much\" would need to be updated to tolerate it and\nyou could count objects while looking for it (if header has 0)\nduring normal processing. (I'm reacting to your word \"streaming\".)\n\n> > Finally, when I generate several 2GB split packfiles, I do notice\n> > the slight delay for fixup_header_footer(), and I do think it's a bit\n> > ugly, but in quantitative terms it's an insignificant part of a long\n> > operation that's infrequently performed. Does this need to be\n> > optimized at all?\n> Maybe, maybe not. That depends how much data we think GIT could be used\n> to manage in the future. With a 1TB pack file you definitely want to\n> optimize that case.\nOK. Just FYI, we have a perforce repository near 200GB and\nthis is not what would concern me right now if we converted all\nor part of it to git. Of course that would depend on the packing schedule.\n\n> OTOH this could wait for the real pack v4 too.\nMakes sense to me. The fewer format changes the better.\n\nBTW, I've caught up on reading the mailing list archives,\nbut I don't recall seeing any overview of the objectives of pack v2/v3/v4.\nDoes that exist any where? I didn't see it in Documentation or\nDocumentation/technical. It would probably reduce uninformed\nquestions like the above. I've deduced rationales for what\nmiscellaneous details I have seen,\nexcept moving the SHA-1s from .idx to .pack (?).\n\nThanks,\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"},{"id":"38978","messageId":"alpine.LFD.0.98.0704091905291.28181@xanadu.home","threadId":"7573","inReplyTo":"56b7f5510704091438v4d42a7efq9b241bdc31affd08@mail.gmail.com","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-09T23:22:30Z","receivedAt":"2007-04-09T23:22:30Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 9 Apr 2007, Dana How wrote:\n\n> On 4/9/07, Nicolas Pitre wrote:\n> > On Mon, 9 Apr 2007, Dana How wrote:\n> > > Wouldn't the following address the \"object count unknown\n> > > at the start of sequential pack writing\" problem:\n> > > Write 0 for object count in the header. This is a flag to look for\n> > > another header of same format just before the final SHA-1 which\n> > > has the correct count. The SHA-1 is still a checksum of everything\n> > > before it and no seeking/rewriting is needed on generation.\n> > \n> > No. You really wants to know up front how many objects a pack contains\n> > when streaming it. And this is not only for packs written to stdout.\n> OK, let me ask a dumb question and flog one last additional obvious idea.\n> \n> Does your wanting to know stem from more than wanting\n> to stick to one malloc of all the object info at once?\n\nThat, plus progress reporting when fetching. And progress reporting is \nprobably the most important thing for the user experience.\n\n> My suggestion quoted above is actually a change to the .pack format.\n> With all the other ideas for .pack format changes floating around,\n> let me withdraw that and suggest a simpler one: write a \"0\" in the header,\n> and terminate the pack with a sentinel in object format before the final\n> SHA-1s.\n> The sentinel would be type=OBJ_NONE/length=0, i.e. a null byte.\n> \"Not much\" would need to be updated to tolerate it and\n> you could count objects while looking for it (if header has 0)\n> during normal processing. (I'm reacting to your word \"streaming\".)\n\nWhen streaming you really want to know up front how much to expect / \nwait for.\n\n> BTW, I've caught up on reading the mailing list archives,\n> but I don't recall seeing any overview of the objectives of pack v2/v3/v4.\n> Does that exist any where? I didn't see it in Documentation or\n> Documentation/technical. It would probably reduce uninformed\n> questions like the above. I've deduced rationales for what\n> miscellaneous details I have seen,\n> except moving the SHA-1s from .idx to .pack (?).\n\nI'm sure this exists in the archive as I recall sending a summary about \nthat a while ago.\n\nIn short:\n\n - Pack v1 was the initial implementation from Linus. It had some flaws\n and didn't exist for more than 2 or 3 days. It was never used in any \n official release.\n\n - Pack v2 is what GIT still produces today.\n\n - Pack v3 was created when a bit in the delta encoding was redefined.\n See commit d60fc1c8649f8 for the details. Because it caused too much\n compatibility issues when we attempted to enable pack v3 at the time, \n we reverted pack generation to v2.\n\n - Pack v4 has a much larger scope. This is the WIP from Shawn Pearce \n and I and I know for sure Shawn already posted a detailed design \n overview to the list already.\n\n\nNicolas\n"}]}