# [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() 8 messages from 2007-04-30 to 2007-05-01. Participants: Dana How, Shawn O. Pearce, Johannes Schindelin, Nicolas Pitre. Thread: https://gitlist.dev/t/7916 ## Dana How, 2007-04-30 23:21 Subject: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <463679EB.2010301@gmail.com> URL: https://gitlist.dev/e/463679EB.2010301%40gmail.com ``` Add our own version of the one in fast-import.c here. Needed later to correct bad object count in header for split pack. Signed-off-by: Dana L. How --- builtin-pack-objects.c | 36 ++++++++++++++++++++++++++++++++++++ 1 files changed, 36 insertions(+), 0 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index bc45ca6..98066bf 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f, return offset + size; } +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1, + char *pack_name, uint32_t object_count) +{ + static const int buf_sz = 128 * 1024; + SHA_CTX c; + struct pack_header hdr; + char *buf; + + if (lseek(pack_fd, 0, SEEK_SET) != 0) + die("Failed seeking to start: %s", strerror(errno)); + if (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr)) + die("Unable to reread header of %s", pack_name); + if (lseek(pack_fd, 0, SEEK_SET) != 0) + die("Failed seeking to start: %s", strerror(errno)); + hdr.hdr_entries = htonl(object_count); + write_or_die(pack_fd, &hdr, sizeof(hdr)); + + SHA1_Init(&c); + SHA1_Update(&c, &hdr, sizeof(hdr)); + + buf = xmalloc(buf_sz); + for (;;) { + size_t n = xread(pack_fd, buf, buf_sz); + if (!n) + break; + if (n < 0) + die("Failed to checksum %s", pack_name); + SHA1_Update(&c, buf, n); + } + free(buf); + + SHA1_Final(pack_file_sha1, &c); + write_or_die(pack_fd, pack_file_sha1, 20); + close(pack_fd); +} + /* forward declarations for write_pack_file */ static void write_index_file(off_t last_obj_offset, unsigned char *sha1); static int adjust_perm(const char *path, mode_t mode); -- 1.5.2.rc0.766.gba60-dirty ``` ## Shawn O. Pearce, 2007-05-01 05:06 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <20070501050633.GZ5942@spearce.org> URL: https://gitlist.dev/e/20070501050633.GZ5942%40spearce.org In-Reply-To: <463679EB.2010301@gmail.com> ``` Dana How wrote: > Add our own version of the one in fast-import.c here. > Needed later to correct bad object count in header for split pack. ... > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c > index bc45ca6..98066bf 100644 > --- a/builtin-pack-objects.c > +++ b/builtin-pack-objects.c > @@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f, > return offset + size; > } > > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1, > + char *pack_name, uint32_t object_count) > +{ This looks a *lot* like the code in fast-import.c. Why not refactor both to use the same implementation and stuff it away in say pack-check.c (for lack of a better place), or start a new file (pack-write.c)? There is a *lot* of code in fast-import.c (over 2,000 lines) that was half-copied from other core code, and that was half created on its own. This is also true in index-pack.c and pack-objects.c; I'd like to see these implementations unify more rather than copy code from each other. I know git-blame will identify the original author quite well, but I'd really like to avoid adding lots more code to maintain if we can avoid it. -- Shawn. ``` ## Dana How, 2007-05-01 05:41 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <56b7f5510704302241n79601619kda8251a9f7776884@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704302241n79601619kda8251a9f7776884%40mail.gmail.com In-Reply-To: <20070501050633.GZ5942@spearce.org> ``` On 4/30/07, Shawn O. Pearce wrote: > Dana How wrote: > > Add our own version of the one in fast-import.c here. > > Needed later to correct bad object count in header for split pack. > ... > > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c > > --- a/builtin-pack-objects.c > > +++ b/builtin-pack-objects.c > > @@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f, > > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1, > > + char *pack_name, uint32_t object_count) > > +{ > > This looks a *lot* like the code in fast-import.c. I hope so! That's where I copied it from. > Why not > refactor both to use the same implementation and stuff it away in > say pack-check.c (for lack of a better place), or start a new file > (pack-write.c)? Actually I didn't just copy it, I tried to rewrite it for my use as well as the fast-import.c use (note there is a 3rd copy in some *index*.c file which I didn't try to merge in yet). However I didn't yet put it in a new file or change fast-import.c to call it since I wanted to change as little as possible. > There is a *lot* of code in fast-import.c (over 2,000 lines) that > was half-copied from other core code, and that was half created > on its own. This is also true in index-pack.c and pack-objects.c; > I'd like to see these implementations unify more rather than copy > code from each other. > > I know git-blame will identify the original author quite well, > but I'd really like to avoid adding lots more code to maintain > if we can avoid it. I agree with all your arguments. I had several reasons to avoid extra rearrangements/refactorings: (a) First patch to git, not previously known to me; (b) I prefer to separate new functionality from "clean-up" work; (c) I didn't really view myself as the person to make the *writing* code in git as well organized/minimized as the *reading* code [e.g. the sliding mmap stuff -- nice!]; (d) Apparently you and Nicolas Pitre have a lot of pending changes affecting the packing code. I'd have no problem submitting a follow-on patch later containing some clean-up work if you & NP clear it, so I know I won't have problems from (d). Note I had to completely rewrite this patch when NP submitted some of his pending stuff. NP wrote that you posted a summary of v3/v4 pack ideas, but I couldn't find it in the list archives. Could you either email it to me, or (re-)post it to the list? Thanks, -- Dana L. How danahow@gmail.com +1 650 804 5991 cell ``` ## Shawn O. Pearce, 2007-05-01 06:03 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <20070501060340.GD5942@spearce.org> URL: https://gitlist.dev/e/20070501060340.GD5942%40spearce.org In-Reply-To: <56b7f5510704302241n79601619kda8251a9f7776884@mail.gmail.com> ``` Dana How wrote: > On 4/30/07, Shawn O. Pearce wrote: > >Why not > >refactor both to use the same implementation and stuff it away in > >say pack-check.c (for lack of a better place), or start a new file > >(pack-write.c)? > Actually I didn't just copy it, I tried to rewrite it for my use > as well as the fast-import.c use (note there is a 3rd copy > in some *index*.c file which I didn't try to merge in yet). > However I didn't yet put it in a new file or change fast-import.c > to call it since I wanted to change as little as possible. ... > I agree with all your arguments. I had several reasons > to avoid extra rearrangements/refactorings: > (a) First patch to git, not previously known to me; > (b) I prefer to separate new functionality from "clean-up" work; A really good reason. ;-) But I'd still rather see it done right the first time, then done partially (copied) and wait for someone to clean it up later. Sometimes that cleanup doesn't happen. Anyway, I'm not against you copying it, I just think there's a better way (refactor at least fast-import's use of it). > (c) I didn't really view myself as the person to make the *writing* > code in git as well organized/minimized as the *reading* code > [e.g. the sliding mmap stuff -- nice!]; Not sure I follow that argument, but thanks for the compliment on the sliding mmap work. I think I was trying to point out that that one particular function is actually quite simple, and you did a direct copy of it from fast-import.c. Instead a simple refactoring that allows both pack-objects.c and fast-import.c share the same implementation of those 30 or so lines of code would be a good start towards cleaning up the writing code. Yes it doesn't help the index-pack.c usage of same logic, but baby steps will help us to cleanup the mess we have already made! We like baby steps around here... ;-) > (d) Apparently you and Nicolas Pitre have a lot of pending changes > affecting the packing code. Yes, but Nico's work has also destroyed in pack v4 topic. Nico has promised to start working on porting some of that work over, but I don't know if he has been able to start doing so yet. I personally have been too busy this past month and a half to really work on packv4, but I'm hoping to circle back to it before the end of May (if Nico doesn't beat me to it!). > I'd have no problem submitting a follow-on patch later containing > some clean-up work if you & NP clear it, so I know I won't have > problems from (d). Note I had to completely rewrite this patch > when NP submitted some of his pending stuff. Yea, hazard of working in this part of the code when Nico is also active. My own sliding mmap stuff was written twice too, for the same reason - Nico doing much needed improvements right in the same spot as I was working, at the same time. > NP wrote that you posted a summary of v3/v4 pack ideas, > but I couldn't find it in the list archives. Could you either > email it to me, or (re-)post it to the list? You can start here: http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=40799 http://article.gmane.org/gmane.comp.version-control.git/42423 http://article.gmane.org/gmane.comp.version-control.git/45406 http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=43046 anything by me or Nico in those threads is good reading on pack v4. The last link is probably the best thread available on the subject. -- Shawn. ``` ## Johannes Schindelin, 2007-05-01 07:32 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0705010927240.29859%40racer.site In-Reply-To: <20070501060340.GD5942@spearce.org> ``` Hi, On Tue, 1 May 2007, Shawn O. Pearce wrote: > Dana How wrote: > > > On 4/30/07, Shawn O. Pearce wrote: > > > > >Why not refactor both to use the same implementation and stuff it > > >away in say pack-check.c (for lack of a better place), or start a new > > >file (pack-write.c)? > > > > Actually I didn't just copy it, I tried to rewrite it for my use as > > well as the fast-import.c use (note there is a 3rd copy in some > > *index*.c file which I didn't try to merge in yet). However I didn't > > yet put it in a new file or change fast-import.c to call it since I > > wanted to change as little as possible. > ... > > I agree with all your arguments. I had several reasons > > to avoid extra rearrangements/refactorings: > > (a) First patch to git, not previously known to me; > > (b) I prefer to separate new functionality from "clean-up" work; > > A really good reason. ;-) > > But I'd still rather see it done right the first time, then done > partially (copied) and wait for someone to clean it up later. FWIW it is not just a clean up in the sense of making the code more elegant. It is about catching bugs. The smaller the code base, and the more code is reused for common tasks, the easier it is to - get at bugs, - fix bugs, and - prevent bugs Some might even argue that the elegance of the code is a direct indicator of its quality. Obviously, I am suggesting to go the slightly slower route. In effect, this will turn out to be the faster route, though, since bug fixing traditionally makes up for 90% of development time. Ciao, Dscho ``` ## Nicolas Pitre, 2007-05-01 17:48 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0705011318000.6574%40xanadu.home In-Reply-To: <20070501060340.GD5942@spearce.org> ``` On Tue, 1 May 2007, Shawn O. Pearce wrote: > Dana How wrote: > > On 4/30/07, Shawn O. Pearce wrote: > > >Why not > > >refactor both to use the same implementation and stuff it away in > > >say pack-check.c (for lack of a better place), or start a new file > > >(pack-write.c)? > > Actually I didn't just copy it, I tried to rewrite it for my use > > as well as the fast-import.c use (note there is a 3rd copy > > in some *index*.c file which I didn't try to merge in yet). > > However I didn't yet put it in a new file or change fast-import.c > > to call it since I wanted to change as little as possible. > ... > > I agree with all your arguments. I had several reasons > > to avoid extra rearrangements/refactorings: > > (a) First patch to git, not previously known to me; > > (b) I prefer to separate new functionality from "clean-up" work; > > A really good reason. ;-) > > But I'd still rather see it done right the first time, then done > partially (copied) and wait for someone to clean it up later. > Sometimes that cleanup doesn't happen. Well I intended to do more cleanups in the pack code eventually. That included the index writing and pack header fixing. But I was expecting for the pack splitting changes to go in first as it is likely to impose some requirements of its own. It is then easier to have a proper interface common to all users after everything is in place. > > (d) Apparently you and Nicolas Pitre have a lot of pending changes > > affecting the packing code. > > Yes, but Nico's work has also destroyed in pack v4 topic. Nico has > promised to start working on porting some of that work over, but I > don't know if he has been able to start doing so yet. It shouldn't be too hard to port the existing code. Most of it is new code that hooks into the existing code in a limited way. > I personally have been too busy this past month and a half to really > work on packv4, but I'm hoping to circle back to it before the end of > May (if Nico doesn't beat me to it!). While the current pack v4 branch is certainly valuable, I consider it more as a proof of concept and a test bench. In practice it isn't really efficient and it won't be able to show its full potential until core code like tree walking is better abstracted seamless processing of parallel tree representations. But we're getting there slowly. My work on index v2 will make the pack v4 changes much smaller in that area. My progress display rework was a direct "huh!" reaction to the extra progress reporting the current pack v4 code added. And the general pack-objects.c refactoring was made with a look on better and easier pack v4 integration in the future. So in my mind pack v4 already started to make its way in the main code in subtle ways. ;-) But since I may do Git work only when I'm bored progress doesn't happen as fast as one would have expected. > > I'd have no problem submitting a follow-on patch later containing > > some clean-up work if you & NP clear it, so I know I won't have > > problems from (d). Note I had to completely rewrite this patch > > when NP submitted some of his pending stuff. > > Yea, hazard of working in this part of the code when Nico is > also active. My own sliding mmap stuff was written twice too, > for the same reason - Nico doing much needed improvements right in > the same spot as I was working, at the same time. Well well. OK I'm used to be considered as the bad guy anyway. ;-) Nicolas ``` ## Dana How, 2007-05-01 17:58 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <56b7f5510705011058y89e4aa4p8f8b7eccde30af78@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510705011058y89e4aa4p8f8b7eccde30af78%40mail.gmail.com In-Reply-To: ``` On 5/1/07, Nicolas Pitre wrote: > On Tue, 1 May 2007, Shawn O. Pearce wrote: > > Dana How wrote: > > > On 4/30/07, Shawn O. Pearce wrote: > > > > Why not > > > > refactor both to use the same implementation and stuff it away in > > > > say pack-check.c (for lack of a better place), or start a new file > > > > (pack-write.c)? > > > Actually I didn't just copy it, I tried to rewrite it for my use > > > as well as the fast-import.c use (note there is a 3rd copy > > > in some *index*.c file which I didn't try to merge in yet). > > > However I didn't yet put it in a new file or change fast-import.c > > > to call it since I wanted to change as little as possible. > > ... > > > I agree with all your arguments. I had several reasons > > > to avoid extra rearrangements/refactorings: > > > (a) First patch to git, not previously known to me; > > > (b) I prefer to separate new functionality from "clean-up" work; > > > > A really good reason. ;-) > > > > But I'd still rather see it done right the first time, then done > > partially (copied) and wait for someone to clean it up later. > > Sometimes that cleanup doesn't happen. > > Well I intended to do more cleanups in the pack code eventually. That > included the index writing and pack header fixing. But I was expecting > for the pack splitting changes to go in first as it is likely to impose > some requirements of its own. It is then easier to have a proper > interface common to all users after everything is in place. I was in the middle of creating pack-write.c at Shawn's suggestion. It will only contain fixup_header_footer(), to be called by fast-import.c and builtin-pack-object.c. index-pack.c also has readjust_pack_header_and_sha1(), which is compatible except it doesn't close the file. I was going to leave it alone for now. This new file should be the logical place to put other common pack-writing-related things. Please barf now if you don't think I should do this tiny refactoring at this point. > > > I'd have no problem submitting a follow-on patch later containing > > > some clean-up work if you & NP clear it, so I know I won't have > > > problems from (d). Note I had to completely rewrite this patch > > > when NP submitted some of his pending stuff. > > > > Yea, hazard of working in this part of the code when Nico is > > also active. My own sliding mmap stuff was written twice too, > > for the same reason - Nico doing much needed improvements right in > > the same spot as I was working, at the same time. > > Well well. OK I'm used to be considered as the bad guy anyway. ;-) You *did* tell me about your upcoming patches as I recall ;-) Thanks, -- Dana L. How danahow@gmail.com +1 650 804 5991 cell ``` ## Nicolas Pitre, 2007-05-01 18:39 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0705011438490.6574%40xanadu.home In-Reply-To: <56b7f5510705011058y89e4aa4p8f8b7eccde30af78@mail.gmail.com> ``` On Tue, 1 May 2007, Dana How wrote: > I was in the middle of creating pack-write.c at Shawn's suggestion. It > will only contain fixup_header_footer(), to be called by fast-import.c > and builtin-pack-object.c. index-pack.c also has > readjust_pack_header_and_sha1(), which is compatible except it doesn't > close the file. I was going to leave it alone for now. This new file > should be the logical place to put other common pack-writing-related > things. Please barf now if you don't think I should do this tiny > refactoring at this point. That's fine. Please go ahead. Nicolas ```