{"thread":{"id":"7916","subject":"[PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","startedAt":"2007-04-30T23:21:15Z","lastAt":"2007-05-01T18:39:54Z","messageCount":8,"participants":["Dana How","Shawn O. Pearce","Johannes Schindelin","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"40772","messageId":"463679EB.2010301@gmail.com","threadId":"7916","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-30T23:21:15Z","receivedAt":"2007-04-30T23:21:15Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"\nAdd our own version of the one in fast-import.c here.\nNeeded later to correct bad object count in header for split pack.\n\nSigned-off-by: Dana L. How \n---\n builtin-pack-objects.c | 36 ++++++++++++++++++++++++++++++++++++\n 1 files changed, 36 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex bc45ca6..98066bf 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f,\n \treturn offset + size;\n }\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 /* forward declarations for write_pack_file */\n static void write_index_file(off_t last_obj_offset, unsigned char *sha1);\n static int adjust_perm(const char *path, mode_t mode);\n-- \n1.5.2.rc0.766.gba60-dirty\n"},{"id":"40789","messageId":"20070501050633.GZ5942@spearce.org","threadId":"7916","inReplyTo":"463679EB.2010301@gmail.com","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-05-01T05:06:33Z","receivedAt":"2007-05-01T05:06:33Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Dana How wrote:\n> Add our own version of the one in fast-import.c here.\n> Needed later to correct bad object count in header for split pack.\n...\n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index bc45ca6..98066bf 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f,\n> \treturn offset + size;\n> }\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\nThis looks a *lot* like the code in fast-import.c. Why not\nrefactor both to use the same implementation and stuff it away in\nsay pack-check.c (for lack of a better place), or start a new file\n(pack-write.c)?\n\nThere is a *lot* of code in fast-import.c (over 2,000 lines) that\nwas half-copied from other core code, and that was half created\non its own. This is also true in index-pack.c and pack-objects.c;\nI'd like to see these implementations unify more rather than copy\ncode from each other.\n\nI know git-blame will identify the original author quite well,\nbut I'd really like to avoid adding lots more code to maintain\nif we can avoid it.\n\n-- \nShawn.\n"},{"id":"40793","messageId":"56b7f5510704302241n79601619kda8251a9f7776884@mail.gmail.com","threadId":"7916","inReplyTo":"20070501050633.GZ5942@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-05-01T05:41:33Z","receivedAt":"2007-05-01T05:41:33Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/30/07, Shawn O. Pearce wrote:\n> Dana How wrote:\n> > Add our own version of the one in fast-import.c here.\n> > Needed later to correct bad object count in header for split pack.\n> ...\n> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> > --- a/builtin-pack-objects.c\n> > +++ b/builtin-pack-objects.c\n> > @@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f,\n> > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,\n> > + char *pack_name, uint32_t object_count)\n> > +{\n>\n> This looks a *lot* like the code in fast-import.c.\nI hope so! That's where I copied it from.\n\n> Why not\n> refactor both to use the same implementation and stuff it away in\n> say pack-check.c (for lack of a better place), or start a new file\n> (pack-write.c)?\nActually I didn't just copy it, I tried to rewrite it for my use\nas well as the fast-import.c use (note there is a 3rd copy\nin some *index*.c file which I didn't try to merge in yet).\nHowever I didn't yet put it in a new file or change fast-import.c\nto call it since I wanted to change as little as possible.\n\n> There is a *lot* of code in fast-import.c (over 2,000 lines) that\n> was half-copied from other core code, and that was half created\n> on its own. This is also true in index-pack.c and pack-objects.c;\n> I'd like to see these implementations unify more rather than copy\n> code from each other.\n>\n> I know git-blame will identify the original author quite well,\n> but I'd really like to avoid adding lots more code to maintain\n> if we can avoid it.\n\nI agree with all your arguments. I had several reasons\nto avoid extra rearrangements/refactorings:\n(a) First patch to git, not previously known to me;\n(b) I prefer to separate new functionality from \"clean-up\" work;\n(c) I didn't really view myself as the person to make the *writing*\n code in git as well organized/minimized as the *reading* code\n [e.g. the sliding mmap stuff -- nice!];\n(d) Apparently you and Nicolas Pitre have a lot of pending changes\n affecting the packing code.\n\nI'd have no problem submitting a follow-on patch later containing\nsome clean-up work if you & NP clear it, so I know I won't have\nproblems from (d). Note I had to completely rewrite this patch\nwhen NP submitted some of his pending stuff.\n\nNP wrote that you posted a summary of v3/v4 pack ideas,\nbut I couldn't find it in the list archives. Could you either\nemail it to me, or (re-)post it to the list?\n\nThanks,\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"},{"id":"40796","messageId":"20070501060340.GD5942@spearce.org","threadId":"7916","inReplyTo":"56b7f5510704302241n79601619kda8251a9f7776884@mail.gmail.com","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-05-01T06:03:40Z","receivedAt":"2007-05-01T06:03:40Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Dana How wrote:\n> On 4/30/07, Shawn O. Pearce wrote:\n> >Why not\n> >refactor both to use the same implementation and stuff it away in\n> >say pack-check.c (for lack of a better place), or start a new file\n> >(pack-write.c)?\n> Actually I didn't just copy it, I tried to rewrite it for my use\n> as well as the fast-import.c use (note there is a 3rd copy\n> in some *index*.c file which I didn't try to merge in yet).\n> However I didn't yet put it in a new file or change fast-import.c\n> to call it since I wanted to change as little as possible.\n...\n> I agree with all your arguments. I had several reasons\n> to avoid extra rearrangements/refactorings:\n> (a) First patch to git, not previously known to me;\n> (b) I prefer to separate new functionality from \"clean-up\" work;\n\nA really good reason. ;-)\n\nBut I'd still rather see it done right the first time, then done\npartially (copied) and wait for someone to clean it up later.\nSometimes that cleanup doesn't happen. Anyway, I'm not against\nyou copying it, I just think there's a better way (refactor at\nleast fast-import's use of it).\n \n> (c) I didn't really view myself as the person to make the *writing*\n> code in git as well organized/minimized as the *reading* code\n> [e.g. the sliding mmap stuff -- nice!];\n\nNot sure I follow that argument, but thanks for the compliment on\nthe sliding mmap work. I think I was trying to point out that that\none particular function is actually quite simple, and you did a\ndirect copy of it from fast-import.c. Instead a simple refactoring\nthat allows both pack-objects.c and fast-import.c share the same\nimplementation of those 30 or so lines of code would be a good\nstart towards cleaning up the writing code. Yes it doesn't help\nthe index-pack.c usage of same logic, but baby steps will help us\nto cleanup the mess we have already made!\n\nWe like baby steps around here... ;-)\n\n> (d) Apparently you and Nicolas Pitre have a lot of pending changes\n> affecting the packing code.\n\nYes, but Nico's work has also destroyed in pack v4 topic. Nico has\npromised to start working on porting some of that work over, but I\ndon't know if he has been able to start doing so yet. I personally\nhave been too busy this past month and a half to really work on\npackv4, but I'm hoping to circle back to it before the end of May\n(if Nico doesn't beat me to it!).\n\n> I'd have no problem submitting a follow-on patch later containing\n> some clean-up work if you & NP clear it, so I know I won't have\n> problems from (d). Note I had to completely rewrite this patch\n> when NP submitted some of his pending stuff.\n\nYea, hazard of working in this part of the code when Nico is\nalso active. My own sliding mmap stuff was written twice too,\nfor the same reason - Nico doing much needed improvements right in\nthe same spot as I was working, at the same time.\n \n> NP wrote that you posted a summary of v3/v4 pack ideas,\n> but I couldn't find it in the list archives. Could you either\n> email it to me, or (re-)post it to the list?\n\nYou can start here:\n\n http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=40799\n http://article.gmane.org/gmane.comp.version-control.git/42423\n http://article.gmane.org/gmane.comp.version-control.git/45406\n http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=43046\n\nanything by me or Nico in those threads is good reading on pack v4.\nThe last link is probably the best thread available on the subject.\n\n-- \nShawn.\n"},{"id":"40801","messageId":"Pine.LNX.4.64.0705010927240.29859@racer.site","threadId":"7916","inReplyTo":"20070501060340.GD5942@spearce.org","subject":"Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-05-01T07:32:50Z","receivedAt":"2007-05-01T07:32:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 1 May 2007, Shawn O. Pearce wrote:\n\n> Dana How wrote:\n>\n> > On 4/30/07, Shawn O. Pearce wrote:\n> >\n> > >Why not refactor both to use the same implementation and stuff it \n> > >away in say pack-check.c (for lack of a better place), or start a new \n> > >file (pack-write.c)?\n> >\n> > Actually I didn't just copy it, I tried to rewrite it for my use as \n> > well as the fast-import.c use (note there is a 3rd copy in some \n> > *index*.c file which I didn't try to merge in yet). However I didn't \n> > yet put it in a new file or change fast-import.c to call it since I \n> > wanted to change as little as possible.\n> ...\n> > I agree with all your arguments. I had several reasons\n> > to avoid extra rearrangements/refactorings:\n> > (a) First patch to git, not previously known to me;\n> > (b) I prefer to separate new functionality from \"clean-up\" work;\n> \n> A really good reason. ;-)\n> \n> But I'd still rather see it done right the first time, then done\n> partially (copied) and wait for someone to clean it up later.\n\nFWIW it is not just a clean up in the sense of making the code more \nelegant. It is about catching bugs. The smaller the code base, and the \nmore code is reused for common tasks, the easier it is to\n\n- get at bugs,\n- fix bugs, and\n- prevent bugs\n\nSome might even argue that the elegance of the code is a direct indicator \nof its quality.\n\nObviously, I am suggesting to go the slightly slower route. In effect, \nthis will turn out to be the faster route, though, since bug fixing \ntraditionally makes up for 90% of development time.\n\nCiao,\nDscho\n"},{"id":"40839","messageId":"alpine.LFD.0.98.0705011318000.6574@xanadu.home","threadId":"7916","inReplyTo":"20070501060340.GD5942@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-05-01T17:48:08Z","receivedAt":"2007-05-01T17:48:08Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 1 May 2007, Shawn O. Pearce wrote:\n\n> Dana How wrote:\n> > On 4/30/07, Shawn O. Pearce wrote:\n> > >Why not\n> > >refactor both to use the same implementation and stuff it away in\n> > >say pack-check.c (for lack of a better place), or start a new file\n> > >(pack-write.c)?\n> > Actually I didn't just copy it, I tried to rewrite it for my use\n> > as well as the fast-import.c use (note there is a 3rd copy\n> > in some *index*.c file which I didn't try to merge in yet).\n> > However I didn't yet put it in a new file or change fast-import.c\n> > to call it since I wanted to change as little as possible.\n> ...\n> > I agree with all your arguments. I had several reasons\n> > to avoid extra rearrangements/refactorings:\n> > (a) First patch to git, not previously known to me;\n> > (b) I prefer to separate new functionality from \"clean-up\" work;\n> \n> A really good reason. ;-)\n> \n> But I'd still rather see it done right the first time, then done\n> partially (copied) and wait for someone to clean it up later.\n> Sometimes that cleanup doesn't happen.\n\nWell I intended to do more cleanups in the pack code eventually. That \nincluded the index writing and pack header fixing. But I was expecting \nfor the pack splitting changes to go in first as it is likely to impose \nsome requirements of its own. It is then easier to have a proper \ninterface common to all users after everything is in place.\n\n> > (d) Apparently you and Nicolas Pitre have a lot of pending changes\n> > affecting the packing code.\n> \n> Yes, but Nico's work has also destroyed in pack v4 topic. Nico has\n> promised to start working on porting some of that work over, but I\n> don't know if he has been able to start doing so yet.\n\nIt shouldn't be too hard to port the existing code. Most of it is new \ncode that hooks into the existing code in a limited way.\n\n> I personally have been too busy this past month and a half to really \n> work on packv4, but I'm hoping to circle back to it before the end of \n> May (if Nico doesn't beat me to it!).\n\nWhile the current pack v4 branch is certainly valuable, I consider it \nmore as a proof of concept and a test bench. In practice it isn't \nreally efficient and it won't be able to show its full potential until \ncore code like tree walking is better abstracted seamless processing of \nparallel tree representations.\n\nBut we're getting there slowly. My work on index v2 will make the pack \nv4 changes much smaller in that area. My progress display rework was a \ndirect \"huh!\" reaction to the extra progress reporting the current pack \nv4 code added. And the general pack-objects.c refactoring was made with \na look on better and easier pack v4 integration in the future.\n\nSo in my mind pack v4 already started to make its way in the main code \nin subtle ways. ;-) But since I may do Git work only when I'm bored \nprogress doesn't happen as fast as one would have expected.\n\n> > I'd have no problem submitting a follow-on patch later containing\n> > some clean-up work if you & NP clear it, so I know I won't have\n> > problems from (d). Note I had to completely rewrite this patch\n> > when NP submitted some of his pending stuff.\n> \n> Yea, hazard of working in this part of the code when Nico is\n> also active. My own sliding mmap stuff was written twice too,\n> for the same reason - Nico doing much needed improvements right in\n> the same spot as I was working, at the same time.\n\nWell well. OK I'm used to be considered as the bad guy anyway. ;-)\n\n\nNicolas\n"},{"id":"40841","messageId":"56b7f5510705011058y89e4aa4p8f8b7eccde30af78@mail.gmail.com","threadId":"7916","inReplyTo":"alpine.LFD.0.98.0705011318000.6574@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-05-01T17:58:52Z","receivedAt":"2007-05-01T17:58:52Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 5/1/07, Nicolas Pitre wrote:\n> On Tue, 1 May 2007, Shawn O. Pearce wrote:\n> > Dana How wrote:\n> > > On 4/30/07, Shawn O. Pearce wrote:\n> > > > Why not\n> > > > refactor both to use the same implementation and stuff it away in\n> > > > say pack-check.c (for lack of a better place), or start a new file\n> > > > (pack-write.c)?\n> > > Actually I didn't just copy it, I tried to rewrite it for my use\n> > > as well as the fast-import.c use (note there is a 3rd copy\n> > > in some *index*.c file which I didn't try to merge in yet).\n> > > However I didn't yet put it in a new file or change fast-import.c\n> > > to call it since I wanted to change as little as possible.\n> > ...\n> > > I agree with all your arguments. I had several reasons\n> > > to avoid extra rearrangements/refactorings:\n> > > (a) First patch to git, not previously known to me;\n> > > (b) I prefer to separate new functionality from \"clean-up\" work;\n> >\n> > A really good reason. ;-)\n> >\n> > But I'd still rather see it done right the first time, then done\n> > partially (copied) and wait for someone to clean it up later.\n> > Sometimes that cleanup doesn't happen.\n>\n> Well I intended to do more cleanups in the pack code eventually. That\n> included the index writing and pack header fixing. But I was expecting\n> for the pack splitting changes to go in first as it is likely to impose\n> some requirements of its own. It is then easier to have a proper\n> interface common to all users after everything is in place.\nI was in the middle of creating pack-write.c at Shawn's suggestion.\nIt will only contain fixup_header_footer(), to be called by fast-import.c\nand builtin-pack-object.c. index-pack.c also has\nreadjust_pack_header_and_sha1(),\nwhich is compatible except it doesn't close the file. I was going to leave it\nalone for now. This new file should be the logical place to put other common\npack-writing-related things. Please barf now if you don't think I\nshould do this\ntiny refactoring at this point.\n\n> > > I'd have no problem submitting a follow-on patch later containing\n> > > some clean-up work if you & NP clear it, so I know I won't have\n> > > problems from (d). Note I had to completely rewrite this patch\n> > > when NP submitted some of his pending stuff.\n> >\n> > Yea, hazard of working in this part of the code when Nico is\n> > also active. My own sliding mmap stuff was written twice too,\n> > for the same reason - Nico doing much needed improvements right in\n> > the same spot as I was working, at the same time.\n>\n> Well well. OK I'm used to be considered as the bad guy anyway. ;-)\nYou *did* tell me about your upcoming patches as I recall ;-)\n\nThanks,\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"},{"id":"40846","messageId":"alpine.LFD.0.98.0705011438490.6574@xanadu.home","threadId":"7916","inReplyTo":"56b7f5510705011058y89e4aa4p8f8b7eccde30af78@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-05-01T18:39:54Z","receivedAt":"2007-05-01T18:39:54Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 1 May 2007, Dana How wrote:\n\n> I was in the middle of creating pack-write.c at Shawn's suggestion. It \n> will only contain fixup_header_footer(), to be called by fast-import.c \n> and builtin-pack-object.c. index-pack.c also has \n> readjust_pack_header_and_sha1(), which is compatible except it doesn't \n> close the file. I was going to leave it alone for now. This new file \n> should be the logical place to put other common pack-writing-related \n> things. Please barf now if you don't think I should do this tiny \n> refactoring at this point.\n\nThat's fine. Please go ahead.\n\n\nNicolas\n"}]}