# [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() 10 messages from 2007-04-08 to 2007-04-09. Participants: Dana How, Junio C Hamano, Nicolas Pitre, Shawn O. Pearce. Thread: https://gitlist.dev/t/7573 ## Dana How, 2007-04-08 23:22 Subject: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <46197950.3080804@gmail.com> URL: https://gitlist.dev/e/46197950.3080804%40gmail.com ``` Add our own version of the one in fast-import.c here. Will need this later to correct an incorrect object count in the pack header. Signed-off-by: Dana How --- builtin-pack-objects.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 8415549..7ab0712 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -518,6 +518,46 @@ static off_t write_one(struct sha1file *f, return offset + write_object(f, e); } +/* + * Move this, the version in fast-import.c, + * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ? + */ +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); +} + typedef int (*entry_sort_t)(const struct object_entry *, const struct object_entry *); static entry_sort_t current_sort; -- 1.5.1.89.g8abf0 ``` ## Junio C Hamano, 2007-04-09 00:04 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <7vwt0m76rv.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vwt0m76rv.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <46197950.3080804@gmail.com> ``` Dana How writes: > +/* > + * Move this, the version in fast-import.c, > + * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ? > + */ > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1, > + char *pack_name, uint32_t object_count) > +{ Indeed that is a very good point. I admit I did not notice we already had the duplication between fast-import.c and index-pack.c Shawn, Nico, what do you think? Wouldn't it be better to refactor them first, independent of Dana's series? ``` ## Nicolas Pitre, 2007-04-09 00:18 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.0704082012220.28181%40xanadu.home In-Reply-To: <7vwt0m76rv.fsf@assigned-by-dhcp.cox.net> ``` On Sun, 8 Apr 2007, Junio C Hamano wrote: > Dana How writes: > > > +/* > > + * Move this, the version in fast-import.c, > > + * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ? > > + */ > > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1, > > + char *pack_name, uint32_t object_count) > > +{ > > Indeed that is a very good point. > > I admit I did not notice we already had the duplication between > fast-import.c and index-pack.c > > Shawn, Nico, what do you think? Wouldn't it be better to > refactor them first, independent of Dana's series? Probably, yes. But probably not in sha1_file.c though. This file is getting a bit large already, and it deals with pack reading only not pack writing. I think another file with common pack writing functions could be created. Pack index writing is another item that is currently duplicated in pack-objects and index-pack for example. Nicolas ``` ## Shawn O. Pearce, 2007-04-09 17:38 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <20070409173858.GT5436@spearce.org> URL: https://gitlist.dev/e/20070409173858.GT5436%40spearce.org In-Reply-To: ``` Nicolas Pitre wrote: > On Sun, 8 Apr 2007, Junio C Hamano wrote: > > > Dana How writes: > > > > > +/* > > > + * Move this, the version in fast-import.c, > > > + * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ? > > > + */ > > > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1, > > > + char *pack_name, uint32_t object_count) > > > +{ > > > > Indeed that is a very good point. > > > > I admit I did not notice we already had the duplication between > > fast-import.c and index-pack.c > > > > Shawn, Nico, what do you think? Wouldn't it be better to > > refactor them first, independent of Dana's series? > > Probably, yes. But probably not in sha1_file.c though. This file is > getting a bit large already, and it deals with pack reading only not > pack writing. > > I think another file with common pack writing functions could be > created. Pack index writing is another item that is currently > duplicated in pack-objects and index-pack for example. I agree entirely. And I'd like to see that refactoring occur before this series, or as part of it. At least for the nr_objects correction routine. To be honest we should have done that when fast-import.c and index-pack.c both needed that logic, but we didn't. I don't remember whose version showed up first in Junio's tree (I think it was index-pack.c) but the other one (probably me with fast-import.c) should have done the refactoring then. We already have *waaay* too many functions that know packfile structure. I'd like to see that decline, but unfortunately a number of them are using rather specialized data structures so it makes things somewhat difficult. For example, writing objects to a packfile: we have 3 implementations. fast-import.c doesn't use sha1write_compressed because that was a waste of time to compute the SHA_CTX when we know we have to go back and fixup nr_objects. It also doesn't use it because fast-import.c's pack-splitting logic is based on the final object size, not the starting offset. It does the deflate itself, decides if the end of the object will overflow, and if so, jumps to a new packfile. -- Shawn. ``` ## Nicolas Pitre, 2007-04-09 18:30 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.0704091404530.28181%40xanadu.home In-Reply-To: <20070409173858.GT5436@spearce.org> ``` On Mon, 9 Apr 2007, Shawn O. Pearce wrote: > Nicolas Pitre wrote: > > I think another file with common pack writing functions could be > > created. Pack index writing is another item that is currently > > duplicated in pack-objects and index-pack for example. > > I agree entirely. And I'd like to see that refactoring occur > before this series, or as part of it. At least for the nr_objects > correction routine. That's easy enough for the nr_objects correction case. The next obvious candidate would be index generation, but I'd wait a bit for all features in progress to be merged and stabilize first. > For example, writing objects to a packfile: we have 3 > implementations. fast-import.c doesn't use sha1write_compressed > because that was a waste of time to compute the SHA_CTX when we > know we have to go back and fixup nr_objects. It also doesn't use > it because fast-import.c's pack-splitting logic is based on the > final object size, not the starting offset. It does the deflate > itself, decides if the end of the object will overflow, and if so, > jumps to a new packfile. I'd be really tempted to create a pack v4 which only change is to still have the pack header at the beginning of the pack like we do today, but include the header in the pack SHA1 computation at the end of the stream only. This way the pack SHA1 could be computed as the pack is generated, and the header fixed up without having to read the entire pack back. I think it was Geert Bosch who proposed this and it makes tons of sense IMHO. Nicolas ``` ## Shawn O. Pearce, 2007-04-09 18:40 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <20070409184039.GZ5436@spearce.org> URL: https://gitlist.dev/e/20070409184039.GZ5436%40spearce.org In-Reply-To: ``` Nicolas Pitre wrote: > I'd be really tempted to create a pack v4 which only change is to still > have the pack header at the beginning of the pack like we do today, but > include the header in the pack SHA1 computation at the end of the stream > only. This way the pack SHA1 could be computed as the pack is > generated, and the header fixed up without having to read the entire > pack back. I think it was Geert Bosch who proposed this and it makes > tons of sense IMHO. Yes. If we really are heading in this direction of needing to correct object counts, we should make that change. Its trivial to hang onto that header for the duration of the rest of the data processing, and tack it onto the end for final SHA-1 computation. Since pack v4 looks like it will be a dev cycle longer than these index format changes and pack-splitting changes, I have to say I agree with you. Lets move "pack v4" back to "pack v5" and make v4 just a shift of where the header is included in the SHA-1. We're heading where I said I didn't want to go, which is two file format changes in 2007. But I think that ship has already sailed... Folks need support for larger repositories now, and pack v4/v5 (whatever you call our dictionary work) just isn't ready. Nor does it solve the big packfile problems that this current work is addressing. -- Shawn. ``` ## Dana How, 2007-04-09 19:11 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <56b7f5510704091211i3d101cafq371fbb96ca27238b@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704091211i3d101cafq371fbb96ca27238b%40mail.gmail.com In-Reply-To: <20070409184039.GZ5436@spearce.org> ``` On 4/9/07, Shawn O. Pearce wrote: > Nicolas Pitre wrote: > > I'd be really tempted to create a pack v4 which only change is to still > > have the pack header at the beginning of the pack like we do today, but > > include the header in the pack SHA1 computation at the end of the stream > > only. This way the pack SHA1 could be computed as the pack is > > generated, and the header fixed up without having to read the entire > > pack back. I think it was Geert Bosch who proposed this and it makes > > tons of sense IMHO. > > Yes. If we really are heading in this direction of needing to > correct object counts, we should make that change. Its trivial > to hang onto that header for the duration of the rest of the data > processing, and tack it onto the end for final SHA-1 computation. I like the property that when an SHA-1 appears at the end of a file, it is a checksum of every byte before it. The ideas above are a departure from that. Do we want this rule to be different for each file type? Wouldn't the following address the "object count unknown at the start of sequential pack writing" problem: Write 0 for object count in the header. This is a flag to look for another header of same format just before the final SHA-1 which has the correct count. The SHA-1 is still a checksum of everything before it and no seeking/rewriting is needed on generation. When reading the object count from a .pack file, you might need to add xread(pack_fd, &header, sizeof(header)); + if (!header.object_count) { + lseek(pack_fd, -20-sizeof(header), SEEK_END); + xread(pack_fd, &header, sizeof(header); + } Or maybe you want this before the object_list_sha1 instead (20->40). Finally, when I generate several 2GB split packfiles, I do notice the slight delay for fixup_header_footer(), and I do think it's a bit ugly, but in quantitative terms it's an insignificant part of a long operation that's infrequently performed. Does this need to be optimized at all? Thanks, -- Dana L. How danahow@gmail.com +1 650 804 5991 cell ``` ## Nicolas Pitre, 2007-04-09 19:33 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.0704091525260.28181%40xanadu.home In-Reply-To: <56b7f5510704091211i3d101cafq371fbb96ca27238b@mail.gmail.com> ``` On Mon, 9 Apr 2007, Dana How wrote: > Wouldn't the following address the "object count unknown > at the start of sequential pack writing" problem: > Write 0 for object count in the header. This is a flag to look for > another header of same format just before the final SHA-1 which > has the correct count. The SHA-1 is still a checksum of everything > before it and no seeking/rewriting is needed on generation. No. You really wants to know up front how many objects a pack contains when streaming it. And this is not only for packs written to stdout. > Finally, when I generate several 2GB split packfiles, I do notice > the slight delay for fixup_header_footer(), and I do think it's a bit > ugly, but in quantitative terms it's an insignificant part of a long > operation that's infrequently performed. Does this need to be > optimized at all? Maybe, maybe not. That depends how much data we think GIT could be used to manage in the future. With a 1TB pack file you definitely want to optimize that case. OTOH this could wait for the real pack v4 too. Nicolas ``` ## Dana How, 2007-04-09 21:38 Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer() Message-ID: <56b7f5510704091438v4d42a7efq9b241bdc31affd08@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704091438v4d42a7efq9b241bdc31affd08%40mail.gmail.com In-Reply-To: ``` On 4/9/07, Nicolas Pitre wrote: > On Mon, 9 Apr 2007, Dana How wrote: > > Wouldn't the following address the "object count unknown > > at the start of sequential pack writing" problem: > > Write 0 for object count in the header. This is a flag to look for > > another header of same format just before the final SHA-1 which > > has the correct count. The SHA-1 is still a checksum of everything > > before it and no seeking/rewriting is needed on generation. > > No. You really wants to know up front how many objects a pack contains > when streaming it. And this is not only for packs written to stdout. OK, let me ask a dumb question and flog one last additional obvious idea. Does your wanting to know stem from more than wanting to stick to one malloc of all the object info at once? My suggestion quoted above is actually a change to the .pack format. With all the other ideas for .pack format changes floating around, let me withdraw that and suggest a simpler one: write a "0" in the header, and terminate the pack with a sentinel in object format before the final SHA-1s. The sentinel would be type=OBJ_NONE/length=0, i.e. a null byte. "Not much" would need to be updated to tolerate it and you could count objects while looking for it (if header has 0) during normal processing. (I'm reacting to your word "streaming".) > > Finally, when I generate several 2GB split packfiles, I do notice > > the slight delay for fixup_header_footer(), and I do think it's a bit > > ugly, but in quantitative terms it's an insignificant part of a long > > operation that's infrequently performed. Does this need to be > > optimized at all? > Maybe, maybe not. That depends how much data we think GIT could be used > to manage in the future. With a 1TB pack file you definitely want to > optimize that case. OK. Just FYI, we have a perforce repository near 200GB and this is not what would concern me right now if we converted all or part of it to git. Of course that would depend on the packing schedule. > OTOH this could wait for the real pack v4 too. Makes sense to me. The fewer format changes the better. BTW, I've caught up on reading the mailing list archives, but I don't recall seeing any overview of the objectives of pack v2/v3/v4. Does that exist any where? I didn't see it in Documentation or Documentation/technical. It would probably reduce uninformed questions like the above. I've deduced rationales for what miscellaneous details I have seen, except moving the SHA-1s from .idx to .pack (?). Thanks, -- Dana L. How danahow@gmail.com +1 650 804 5991 cell ``` ## Nicolas Pitre, 2007-04-09 23:22 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.0704091905291.28181%40xanadu.home In-Reply-To: <56b7f5510704091438v4d42a7efq9b241bdc31affd08@mail.gmail.com> ``` On Mon, 9 Apr 2007, Dana How wrote: > On 4/9/07, Nicolas Pitre wrote: > > On Mon, 9 Apr 2007, Dana How wrote: > > > Wouldn't the following address the "object count unknown > > > at the start of sequential pack writing" problem: > > > Write 0 for object count in the header. This is a flag to look for > > > another header of same format just before the final SHA-1 which > > > has the correct count. The SHA-1 is still a checksum of everything > > > before it and no seeking/rewriting is needed on generation. > > > > No. You really wants to know up front how many objects a pack contains > > when streaming it. And this is not only for packs written to stdout. > OK, let me ask a dumb question and flog one last additional obvious idea. > > Does your wanting to know stem from more than wanting > to stick to one malloc of all the object info at once? That, plus progress reporting when fetching. And progress reporting is probably the most important thing for the user experience. > My suggestion quoted above is actually a change to the .pack format. > With all the other ideas for .pack format changes floating around, > let me withdraw that and suggest a simpler one: write a "0" in the header, > and terminate the pack with a sentinel in object format before the final > SHA-1s. > The sentinel would be type=OBJ_NONE/length=0, i.e. a null byte. > "Not much" would need to be updated to tolerate it and > you could count objects while looking for it (if header has 0) > during normal processing. (I'm reacting to your word "streaming".) When streaming you really want to know up front how much to expect / wait for. > BTW, I've caught up on reading the mailing list archives, > but I don't recall seeing any overview of the objectives of pack v2/v3/v4. > Does that exist any where? I didn't see it in Documentation or > Documentation/technical. It would probably reduce uninformed > questions like the above. I've deduced rationales for what > miscellaneous details I have seen, > except moving the SHA-1s from .idx to .pack (?). I'm sure this exists in the archive as I recall sending a summary about that a while ago. In short: - Pack v1 was the initial implementation from Linus. It had some flaws and didn't exist for more than 2 or 3 days. It was never used in any official release. - Pack v2 is what GIT still produces today. - Pack v3 was created when a bit in the delta encoding was redefined. See commit d60fc1c8649f8 for the details. Because it caused too much compatibility issues when we attempted to enable pack v3 at the time, we reverted pack generation to v2. - Pack v4 has a much larger scope. This is the WIP from Shawn Pearce and I and I know for sure Shawn already posted a detailed design overview to the list already. Nicolas ```