[PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()

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 <redacted>
---
 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

Dana How [off-list ref] 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?

On Sun, 8 Apr 2007, Junio C Hamano wrote:

Dana How [off-list ref] writes:

quoted

+/*
+ * 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

Nicolas Pitre [off-list ref] wrote:

On Sun, 8 Apr 2007, Junio C Hamano wrote:

quoted

Dana How [off-list ref] writes:

quoted

+/*
+ * 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.

On Mon, 9 Apr 2007, Shawn O. Pearce wrote:

Nicolas Pitre [off-list ref] wrote:

quoted

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

Nicolas Pitre [off-list ref] 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.

On 4/9/07, Shawn O. Pearce [off-list ref] wrote:

Nicolas Pitre [off-list ref] wrote:

quoted

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

On 4/9/07, Nicolas Pitre [off-list ref] wrote:

On Mon, 9 Apr 2007, Dana How wrote:

quoted

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".)

quoted

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

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

On Mon, 9 Apr 2007, Dana How wrote:

On 4/9/07, Nicolas Pitre [off-list ref] wrote:

quoted

On Mon, 9 Apr 2007, Dana How wrote:

quoted

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