Create pack-write.c for common pack writing code

PATCH In review Dana How 3 participants 5 messages Jun 15, 2016 Kbuild

Share & tools

Dana How

author

2016-06-15 22:43 UTC #0 lore raw

PATCH 1/1 Create pack-write.c for common pack writing code

Include a generalized fixup_header_footer() in this new file. Needed by git-repack --max-pack-size feature in a later patchset.

Signed-off-by Dana L. How <[email protected]>

---
 Makefile      |    4 ++--
 fast-import.c |   39 ++-------------------------------------
 pack-write.c  |   38 ++++++++++++++++++++++++++++++++++++++
 pack.h        |    3 +++
 4 files changed, 45 insertions(+), 39 deletions(-)

Makefile +2 -2

index 2fea115..e0a1308 100644
@@ -301,8 +301,8 @@ LIB_OBJS = \
 	interpolate.o \
 	lockfile.o \
 	patch-ids.o \
-	object.o pack-check.o patch-delta.o path.o pkt-line.o sideband.o \
-	reachable.o reflog-walk.o \
+	object.o pack-check.o pack-write.o patch-delta.o path.o pkt-line.o \
+	sideband.o reachable.o reflog-walk.o \
 	quote.o read-cache.o refs.o run-command.o dir.o object-refs.o \
 	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \
 	tag.o tree.o usage.o config.o environment.o ctype.o copy.o \

fast-import.c +2 ~~-37~~

index b4cbcd9..276e0e0 100644
@@ -651,42 +651,6 @@ static void start_packfile(void)
 	all_packs[pack_id] = p;
 }
 
-static void fixup_header_footer(void)
-{
-	static const int buf_sz = 128 * 1024;
-	int pack_fd = pack_data->pack_fd;
-	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_data->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 (;;) {
-		ssize_t n = xread(pack_fd, buf, buf_sz);
-		if (!n)
-			break;
-		if (n < 0)
-			die("Failed to checksum %s", pack_data->pack_name);
-		SHA1_Update(&c, buf, n);
-	}
-	free(buf);
-
-	SHA1_Final(pack_data->sha1, &c);
-	write_or_die(pack_fd, pack_data->sha1, sizeof(pack_data->sha1));
-	close(pack_fd);
-}
-
 static int oecmp (const void *a_, const void *b_)
 {
 	struct object_entry *a = *((struct object_entry**)a_);

@@ -802,7 +766,8 @@ static void end_packfile(void)
 		struct branch *b;
 		struct tag *t;
 
-		fixup_header_footer();
+		fixup_header_footer(pack_data->pack_fd, pack_data->sha1,
+				    pack_data->pack_name, object_count);
 		idx_name = keep_pack(create_index());
 
 		/* Register the packfile with core git's machinary. */

pack-write.c new file +38 -0

new file mode 100644
index 0000000..e4c1408
@@ -0,0 +1,38 @@
+#include "cache.h"
+#include "pack.h"
+
+void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,
+			 const 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);
+}

pack.h +3 -0

index d4d412c..dc296cc 100644
@@ -45,6 +45,9 @@ struct pack_idx_header {
 
 extern int verify_pack(struct packed_git *, int);
 
+void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,
+			 const char *pack_name, uint32_t object_count);
+
 #define PH_ERROR_EOF		(-1)
 #define PH_ERROR_PACK_SIGNATURE	(-2)
 #define PH_ERROR_PROTOCOL	(-3)

Signature · 1 line

1.5.2.rc0.789.gd951

Shawn O. Pearce

2016-06-15 22:43 UTC #1 lore raw

Quoted 3 lines · Dana How

Include a generalized fixup_header_footer() in this new file. Needed by git-repack --max-pack-size feature in a later patchset.

Thanks. I'm applying this to my fastimport.git tree, but I changed the name to fixup_pack_header_footer(). I'm also refactoring the same code out of index-pack, to call your version.

I'll ask Junio to pull your patch, and my index-pack cleanup soon. As soon as I'm sure everything still passes the tests. ;-)

Signature · 1 line

Shawn.

Dana How

author

2016-06-15 22:43 UTC #2 lore raw

Quoted 9 lines · Shawn O. Pearce

Quoted 2 lines · Dana How
Include a generalized fixup_header_footer() in this new file. Needed by git-repack --max-pack-size feature in a later patchset.
Thanks. I'm applying this to my fastimport.git tree, but I changed the name to fixup_pack_header_footer(). I'm also refactoring the same code out of index-pack, to call your version.
I'll ask Junio to pull your patch, and my index-pack cleanup soon. As soon as I'm sure everything still passes the tests. ;-)

Sounds good. I didn't refactor index-pack.c since that wasn't what we discussed; pulling out the close() as you or Nicolas later write is the only thing needed to include it too.

At this point, I guess there's not much left to comment on in the --max-pack-size=N patchset? Should I update it for your _pack name change/moving out the close()? I' feel like I've really spammed the list with the versions of this patchset (5 so far).

Also, any reaction to the pack.compression/--compression=N patch? It was the smallest code delta/most significant low-hanging fruit I could identify for my day-job workflow's performance.

Thanks,

Signature · 1 line

Dana L. How  [email protected]  +1 650 804 5991 cell

Nicolas Pitre

2016-06-15 22:43 UTC #3 lore raw

In reply to Shawn O. Pearce

Quoted 11 lines · Shawn O. Pearce

Quoted 3 lines · Dana How
Include a generalized fixup_header_footer() in this new file. Needed by git-repack --max-pack-size feature in a later patchset.
Thanks. I'm applying this to my fastimport.git tree, but I changed the name to fixup_pack_header_footer(). I'm also refactoring the same code out of index-pack, to call your version.
I'll ask Junio to pull your patch, and my index-pack cleanup soon. As soon as I'm sure everything still passes the tests. ;-)

BTW I think the common function should _not_ close the file descriptor
it is being handed. It is more flexible to let the caller close the
file, or possibly do whatever other operations like fchmod().

Nicolas

Shawn O. Pearce

2016-06-15 22:43 UTC #4 lore raw

Quoted 3 lines · Nicolas Pitre

BTW I think the common function should _not_ close the file descriptor
it is being handed. It is more flexible to let the caller close the
file, or possibly do whatever other operations like fchmod().

I didn't mention it, but I agree, and fixed it in Dana's patch.

Signature · 1 line

Shawn.