Re: Rebase max-pack-size?

Re: Rebase max-pack-size?

[Junio C Hamano]

"Dana How" <danahow@gmail.com> writes:

> Since max-pack-size has appeared, and disappeard, in "pu" twice
> and never made it into "next",  I'm guessing I should rebase/merge it on
> top of the other recent pack-objects work.  I may get to this tonight;
> perhaps I'll send the patchset in one email with multiple attachments?

I am still holding onto the series on its own topic branch, but
merging it to 'pu' and resolving conflicts seem pointless while
I am (and hopefully the rest of the list ought to be ;-)
primarily concentrating on last-minute bugfixes in preparation
for v1.5.2.

A rebase to be queued in 'next' would be good, though.

Thanks.

Re: Rebase max-pack-size?

[Dana How]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

"Junio C Hamano" <junkio@cox.net> writes:
> "Dana How" <danahow@gmail.com> writes:
> > Since max-pack-size has appeared, and disappeard, in "pu" twice
> > and never made it into "next",  I'm guessing I should rebase/merge it on
> > top of the other recent pack-objects work.  I may get to this tonight;
> > perhaps I'll send the patchset in one email with multiple attachments?
> I am still holding onto the series on its own topic branch, but
> merging it to 'pu' and resolving conflicts seem pointless while
> I am (and hopefully the rest of the list ought to be ;-)
> primarily concentrating on last-minute bugfixes in preparation
> for v1.5.2.
> 
> A rebase to be queued in 'next' would be good, though.

Yes,  the conflicts were extensive.
The rebased patches are attached.
Let me know if you want them re-sent inline and/or separately.

Thanks

Dana

[-- Attachment #2: 0001-Alter-sha1close-3rd-argument-to-request-flush-only.patch --]
[-- Type: text/plain, Size: 1345 bytes --]

>From 781820f147bcb12dae576734585b27f42faca3ea Mon Sep 17 00:00:00 2001
From: Dana L. How <danahow@gmail.com>
Date: Sun, 13 May 2007 11:28:19 -0700
Subject: [PATCH] Alter sha1close() 3rd argument to request flush only

update=0 suppressed writing the final SHA-1 but was not used.
Now final=0 suppresses SHA-1 finalization, SHA-1 writing,
and closing -- in other words,  only flush the buffer.

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 csum-file.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/csum-file.c b/csum-file.c
index 7088f6e..5109342 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -29,18 +29,20 @@ static void sha1flush(struct sha1file *f, unsigned int count)
 	}
 }
 
-int sha1close(struct sha1file *f, unsigned char *result, int update)
+int sha1close(struct sha1file *f, unsigned char *result, int final)
 {
 	unsigned offset = f->offset;
 	if (offset) {
 		SHA1_Update(&f->ctx, f->buffer, offset);
 		sha1flush(f, offset);
+		f->offset = 0;
 	}
+	if (!final)
+		return 0;	/* only want to flush (no checksum write, no close) */
 	SHA1_Final(f->buffer, &f->ctx);
 	if (result)
 		hashcpy(result, f->buffer);
-	if (update)
-		sha1flush(f, 20);
+	sha1flush(f, 20);
 	if (close(f->fd))
 		die("%s: sha1 file error on close (%s)", f->name, strerror(errno));
 	free(f);
-- 
1.5.2.rc3.726.g279d


[-- Attachment #3: 0001-git-repack-max-pack-size-new-file-statics-and-cod.patch --]
[-- Type: text/plain, Size: 4900 bytes --]

>From 0c75fd66aa6a758e245d85b9304e85a38e977f94 Mon Sep 17 00:00:00 2001
From: Dana L. How <danahow@gmail.com>
Date: Sun, 13 May 2007 11:34:56 -0700
Subject: [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring

Add "pack_size_limit", the limit specified by --max-pack-size,
"written_list", the list of objects written to the current pack,
and "nr_written", the number of objects in written_list.
Put "base_name" at file scope again and add forward declarations.
Move write_index_file() call from cnd_pack_objects() to
write_pack_file() since only the latter will know how
many times to call write_index_file().

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 builtin-pack-objects.c |   68 +++++++++++++++++++++++++----------------------
 1 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index d165f10..7c17ea0 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -52,7 +52,8 @@ struct object_entry {
  * nice "minimum seek" order.
  */
 static struct object_entry *objects;
-static uint32_t nr_objects, nr_alloc, nr_result;
+static struct object_entry **written_list;
+static uint32_t nr_objects, nr_alloc, nr_result, nr_written;
 
 static int non_empty;
 static int no_reuse_delta, no_reuse_object;
@@ -61,9 +62,11 @@ static int incremental;
 static int allow_ofs_delta;
 static const char *pack_tmp_name, *idx_tmp_name;
 static char tmpname[PATH_MAX];
+static const char *base_name;
 static unsigned char pack_file_sha1[20];
 static int progress = 1;
 static int window = 10;
+static uint32_t pack_size_limit;
 static int depth = 50;
 static int pack_to_stdout;
 static int num_preferred_base;
@@ -501,7 +504,11 @@ static int open_object_dir_tmp(const char *path)
     return mkstemp(tmpname);
 }
 
-static off_t write_pack_file(void)
+/* 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);
+
+static void write_pack_file(void)
 {
 	uint32_t i;
 	struct sha1file *f;
@@ -543,7 +550,32 @@ static off_t write_pack_file(void)
 		die("wrote %u objects while expecting %u", written, nr_result);
 	sha1close(f, pack_file_sha1, 1);
 
-	return last_obj_offset;
+	if (!pack_to_stdout) {
+			unsigned char object_list_sha1[20];
+			mode_t mode = umask(0);
+
+			umask(mode);
+			mode = 0444 & ~mode;
+
+			write_index_file(last_obj_offset, object_list_sha1);
+			snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
+				 base_name, sha1_to_hex(object_list_sha1));
+			if (adjust_perm(pack_tmp_name, mode))
+				die("unable to make temporary pack file readable: %s",
+				    strerror(errno));
+			if (rename(pack_tmp_name, tmpname))
+				die("unable to rename temporary pack file: %s",
+				    strerror(errno));
+			snprintf(tmpname, sizeof(tmpname), "%s-%s.idx",
+				 base_name, sha1_to_hex(object_list_sha1));
+			if (adjust_perm(idx_tmp_name, mode))
+				die("unable to make temporary index file readable: %s",
+				    strerror(errno));
+			if (rename(idx_tmp_name, tmpname))
+				die("unable to rename temporary index file: %s",
+				    strerror(errno));
+			puts(sha1_to_hex(object_list_sha1));
+	}
 }
 
 static int sha1_sort(const void *_a, const void *_b)
@@ -1537,8 +1569,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	int use_internal_rev_list = 0;
 	int thin = 0;
 	uint32_t i;
-	off_t last_obj_offset;
-	const char *base_name = NULL;
 	const char **rp_av;
 	int rp_ac_alloc = 64;
 	int rp_ac;
@@ -1707,33 +1737,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, "Result has %u objects.\n", nr_result);
 	if (nr_result)
 		prepare_pack(window, depth);
-	last_obj_offset = write_pack_file();
-	if (!pack_to_stdout) {
-		unsigned char object_list_sha1[20];
-		mode_t mode = umask(0);
-
-		umask(mode);
-		mode = 0444 & ~mode;
-
-		write_index_file(last_obj_offset, object_list_sha1);
-		snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
-			 base_name, sha1_to_hex(object_list_sha1));
-		if (adjust_perm(pack_tmp_name, mode))
-			die("unable to make temporary pack file readable: %s",
-			    strerror(errno));
-		if (rename(pack_tmp_name, tmpname))
-			die("unable to rename temporary pack file: %s",
-			    strerror(errno));
-		snprintf(tmpname, sizeof(tmpname), "%s-%s.idx",
-			 base_name, sha1_to_hex(object_list_sha1));
-		if (adjust_perm(idx_tmp_name, mode))
-			die("unable to make temporary index file readable: %s",
-			    strerror(errno));
-		if (rename(idx_tmp_name, tmpname))
-			die("unable to rename temporary index file: %s",
-			    strerror(errno));
-		puts(sha1_to_hex(object_list_sha1));
-	}
+	write_pack_file();
 	if (progress)
 		fprintf(stderr, "Total %u (delta %u), reused %u (delta %u)\n",
 			written, written_delta, reused, reused_delta);
-- 
1.5.2.rc3.726.g279d


[-- Attachment #4: 0002-git-repack-max-pack-size-write_-object-one-res.patch --]
[-- Type: text/plain, Size: 8068 bytes --]

>From 40ac2f294ec68cae27f8a9262db4cab84f1d2257 Mon Sep 17 00:00:00 2001
From: Dana L. How <danahow@gmail.com>
Date: Sun, 13 May 2007 12:06:18 -0700
Subject: [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit

With --max-pack-size,  generate the appropriate write limit
for each object and check against it before each group of writes.
Update delta usability rules to handle base being in a previously-
written pack.  Inline sha1write_compress() so we know the
exact size of the written data when it needs to be compressed.
Detect and return write "failure".

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 builtin-pack-objects.c |  131 ++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 104 insertions(+), 27 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 7c17ea0..3023aac 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -352,16 +352,31 @@ static void copy_pack_data(struct sha1file *f,
 }
 
 static unsigned long write_object(struct sha1file *f,
-				  struct object_entry *entry)
+				  struct object_entry *entry,
+				  off_t write_offset)
 {
 	unsigned long size;
 	enum object_type type;
 	void *buf;
 	unsigned char header[10];
+	unsigned char dheader[10];
 	unsigned hdrlen;
 	off_t datalen;
 	enum object_type obj_type;
 	int to_reuse = 0;
+	/* write limit if limited packsize and not first object */
+	unsigned long limit = pack_size_limit && nr_written ?
+				pack_size_limit - write_offset : 0;
+				/* no if no delta */
+	int usable_delta =	!entry->delta ? 0 :
+				/* yes if unlimited packfile */
+				!pack_size_limit ? 1 :
+				/* no if base written to previous pack */
+				entry->delta->offset == (off_t)-1 ? 0 :
+				/* otherwise double-check written to this
+				 * pack,  like we do below
+				 */
+				entry->delta->offset ? 1 : 0;
 
 	if (!pack_to_stdout)
 		crc32_begin(f);
@@ -372,7 +387,9 @@ static unsigned long write_object(struct sha1file *f,
 	else if (!entry->in_pack)
 		to_reuse = 0;	/* can't reuse what we don't have */
 	else if (obj_type == OBJ_REF_DELTA || obj_type == OBJ_OFS_DELTA)
-		to_reuse = 1;	/* check_object() decided it for us */
+				/* check_object() decided it for us ... */
+		to_reuse = usable_delta;
+				/* ... but pack split may override that */
 	else if (obj_type != entry->in_pack_type)
 		to_reuse = 0;	/* pack has delta which is unusable */
 	else if (entry->delta)
@@ -383,24 +400,48 @@ static unsigned long write_object(struct sha1file *f,
 				 */
 
 	if (!to_reuse) {
+		z_stream stream;
+		unsigned long maxsize;
+		void *out;
 		buf = read_sha1_file(entry->sha1, &type, &size);
 		if (!buf)
 			die("unable to read %s", sha1_to_hex(entry->sha1));
 		if (size != entry->size)
 			die("object %s size inconsistency (%lu vs %lu)",
 			    sha1_to_hex(entry->sha1), size, entry->size);
-		if (entry->delta) {
+		if (usable_delta) {
 			buf = delta_against(buf, size, entry);
 			size = entry->delta_size;
 			obj_type = (allow_ofs_delta && entry->delta->offset) ?
 				OBJ_OFS_DELTA : OBJ_REF_DELTA;
+		} else {
+			/*
+			 * recover real object type in case
+			 * check_object() wanted to re-use a delta,
+			 * but we couldn't since base was in previous split pack
+			 */
+			obj_type = type;
 		}
+		/* compress the data to store and put compressed length in datalen */
+		memset(&stream, 0, sizeof(stream));
+		deflateInit(&stream, pack_compression_level);
+		maxsize = deflateBound(&stream, size);
+		out = xmalloc(maxsize);
+		/* Compress it */
+		stream.next_in = buf;
+		stream.avail_in = size;
+		stream.next_out = out;
+		stream.avail_out = maxsize;
+		while (deflate(&stream, Z_FINISH) == Z_OK)
+			/* nothing */;
+		deflateEnd(&stream);
+		datalen = stream.total_out;
+		deflateEnd(&stream);
 		/*
 		 * The object header is a byte of 'type' followed by zero or
 		 * more bytes of length.
 		 */
 		hdrlen = encode_header(obj_type, size, header);
-		sha1write(f, header, hdrlen);
 
 		if (obj_type == OBJ_OFS_DELTA) {
 			/*
@@ -409,21 +450,41 @@ static unsigned long write_object(struct sha1file *f,
 			 * base from this object's position in the pack.
 			 */
 			off_t ofs = entry->offset - entry->delta->offset;
-			unsigned pos = sizeof(header) - 1;
-			header[pos] = ofs & 127;
+			unsigned pos = sizeof(dheader) - 1;
+			dheader[pos] = ofs & 127;
 			while (ofs >>= 7)
-				header[--pos] = 128 | (--ofs & 127);
-			sha1write(f, header + pos, sizeof(header) - pos);
-			hdrlen += sizeof(header) - pos;
+				dheader[--pos] = 128 | (--ofs & 127);
+			if (limit && hdrlen + sizeof(dheader) - pos + datalen + 20 >= limit) {
+				free(out);
+				free(buf);
+				return 0;
+			}
+			sha1write(f, header, hdrlen);
+			sha1write(f, dheader + pos, sizeof(dheader) - pos);
+			hdrlen += sizeof(dheader) - pos;
 		} else if (obj_type == OBJ_REF_DELTA) {
 			/*
 			 * Deltas with a base reference contain
 			 * an additional 20 bytes for the base sha1.
 			 */
+			if (limit && hdrlen + 20 + datalen + 20 >= limit) {
+				free(out);
+				free(buf);
+				return 0;
+			}
+			sha1write(f, header, hdrlen);
 			sha1write(f, entry->delta->sha1, 20);
 			hdrlen += 20;
+		} else {
+			if (limit && hdrlen + datalen + 20 >= limit) {
+				free(out);
+				free(buf);
+				return 0;
+			}
+			sha1write(f, header, hdrlen);
 		}
-		datalen = sha1write_compressed(f, buf, size, pack_compression_level);
+		sha1write(f, out, datalen);
+		free(out);
 		free(buf);
 	}
 	else {
@@ -438,20 +499,6 @@ static unsigned long write_object(struct sha1file *f,
 			reused_delta++;
 		}
 		hdrlen = encode_header(obj_type, entry->size, header);
-		sha1write(f, header, hdrlen);
-		if (obj_type == OBJ_OFS_DELTA) {
-			off_t ofs = entry->offset - entry->delta->offset;
-			unsigned pos = sizeof(header) - 1;
-			header[pos] = ofs & 127;
-			while (ofs >>= 7)
-				header[--pos] = 128 | (--ofs & 127);
-			sha1write(f, header + pos, sizeof(header) - pos);
-			hdrlen += sizeof(header) - pos;
-		} else if (obj_type == OBJ_REF_DELTA) {
-			sha1write(f, entry->delta->sha1, 20);
-			hdrlen += 20;
-		}
-
 		offset = entry->in_pack_offset;
 		revidx = find_packed_object(p, offset);
 		datalen = revidx[1].offset - offset;
@@ -460,6 +507,29 @@ static unsigned long write_object(struct sha1file *f,
 			die("bad packed object CRC for %s", sha1_to_hex(entry->sha1));
 		offset += entry->in_pack_header_size;
 		datalen -= entry->in_pack_header_size;
+		if (obj_type == OBJ_OFS_DELTA) {
+			off_t ofs = entry->offset - entry->delta->offset;
+			unsigned pos = sizeof(dheader) - 1;
+			dheader[pos] = ofs & 127;
+			while (ofs >>= 7)
+				dheader[--pos] = 128 | (--ofs & 127);
+			if (limit && hdrlen + sizeof(dheader) - pos + datalen + 20 >= limit)
+				return 0;
+			sha1write(f, header, hdrlen);
+			sha1write(f, dheader + pos, sizeof(dheader) - pos);
+			hdrlen += sizeof(dheader) - pos;
+		} else if (obj_type == OBJ_REF_DELTA) {
+			if (limit && hdrlen + 20 + datalen + 20 >= limit)
+				return 0;
+			sha1write(f, header, hdrlen);
+			sha1write(f, entry->delta->sha1, 20);
+			hdrlen += 20;
+		} else {
+			if (limit && hdrlen + datalen + 20 >= limit)
+				return 0;
+			sha1write(f, header, hdrlen);
+		}
+
 		if (!pack_to_stdout && p->index_version == 1 &&
 		    check_pack_inflate(p, &w_curs, offset, datalen, entry->size))
 			die("corrupt packed object for %s", sha1_to_hex(entry->sha1));
@@ -467,7 +537,7 @@ static unsigned long write_object(struct sha1file *f,
 		unuse_pack(&w_curs);
 		reused++;
 	}
-	if (entry->delta)
+	if (usable_delta)
 		written_delta++;
 	written++;
 	if (!pack_to_stdout)
@@ -486,11 +556,18 @@ static off_t write_one(struct sha1file *f,
 		return offset;
 
 	/* if we are deltified, write out base object first. */
-	if (e->delta)
+	if (e->delta) {
 		offset = write_one(f, e->delta, offset);
+		if (!offset)
+			return 0;
+	}
 
 	e->offset = offset;
-	size = write_object(f, e);
+	size = write_object(f, e, offset);
+	if (!size) {
+		e->offset = 0;
+		return 0;
+	}
 
 	/* make sure off_t is sufficiently large not to wrap */
 	if (offset > offset + size)
-- 
1.5.2.rc3.726.g279d


[-- Attachment #5: 0003-git-repack-max-pack-size-split-packs-as-asked-by.patch --]
[-- Type: text/plain, Size: 6712 bytes --]

>From 9ec3af1f52f41d0737b6271fded5e94527eb0466 Mon Sep 17 00:00:00 2001
From: Dana L. How <danahow@gmail.com>
Date: Sun, 13 May 2007 12:09:16 -0700
Subject: [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()

Rewrite write_pack_file() to break to a new packfile
whenever write_object/write_one request it,  and
correct the header's object count in the previous packfile.
Change write_index_file() to write an index
for just the objects in the most recent packfile.

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 builtin-pack-objects.c |  120 ++++++++++++++++++++++++++++-------------------
 1 files changed, 71 insertions(+), 49 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 3023aac..ce9eb2d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -568,6 +568,7 @@ static off_t write_one(struct sha1file *f,
 		e->offset = 0;
 		return 0;
 	}
+	written_list[nr_written++] = e;
 
 	/* make sure off_t is sufficiently large not to wrap */
 	if (offset > offset + size)
@@ -587,47 +588,57 @@ static int adjust_perm(const char *path, mode_t mode);
 
 static void write_pack_file(void)
 {
-	uint32_t i;
+	uint32_t i = 0, j;
 	struct sha1file *f;
-	off_t offset, last_obj_offset = 0;
+	off_t offset, offset_one, last_obj_offset = 0;
 	struct pack_header hdr;
-	int do_progress = progress;
-
-	if (pack_to_stdout) {
-		f = sha1fd(1, "<stdout>");
-		do_progress >>= 1;
-	} else {
-		int fd = open_object_dir_tmp("tmp_pack_XXXXXX");
-		if (fd < 0)
-			die("unable to create %s: %s\n", tmpname, strerror(errno));
-		pack_tmp_name = xstrdup(tmpname);
-		f = sha1fd(fd, pack_tmp_name);
-	}
+	int do_progress = progress >> pack_to_stdout;
+	uint32_t nr_remaining = nr_result;
 
 	if (do_progress)
 		start_progress(&progress_state, "Writing %u objects...", "", nr_result);
+	written_list = xmalloc(nr_objects * sizeof(struct object_entry *));
 
-	hdr.hdr_signature = htonl(PACK_SIGNATURE);
-	hdr.hdr_version = htonl(PACK_VERSION);
-	hdr.hdr_entries = htonl(nr_result);
-	sha1write(f, &hdr, sizeof(hdr));
-	offset = sizeof(hdr);
-	if (!nr_result)
-		goto done;
-	for (i = 0; i < nr_objects; i++) {
-		last_obj_offset = offset;
-		offset = write_one(f, objects + i, offset);
-		if (do_progress)
-			display_progress(&progress_state, written);
-	}
-	if (do_progress)
-		stop_progress(&progress_state);
- done:
-	if (written != nr_result)
-		die("wrote %u objects while expecting %u", written, nr_result);
-	sha1close(f, pack_file_sha1, 1);
+	do {
+		if (pack_to_stdout) {
+			f = sha1fd(1, "<stdout>");
+		} else {
+			int fd = open_object_dir_tmp("tmp_pack_XXXXXX");
+			if (fd < 0)
+				die("unable to create %s: %s\n", tmpname, strerror(errno));
+			pack_tmp_name = xstrdup(tmpname);
+			f = sha1fd(fd, pack_tmp_name);
+		}
 
-	if (!pack_to_stdout) {
+		hdr.hdr_signature = htonl(PACK_SIGNATURE);
+		hdr.hdr_version = htonl(PACK_VERSION);
+		hdr.hdr_entries = htonl(nr_remaining);
+		sha1write(f, &hdr, sizeof(hdr));
+		offset = sizeof(hdr);
+		nr_written = 0;
+		for (; i < nr_objects; i++) {
+			last_obj_offset = offset;
+			offset_one = write_one(f, objects + i, offset);
+			if (!offset_one)
+				break;
+			offset = offset_one;
+			if (do_progress)
+				display_progress(&progress_state, written);
+		}
+
+		/*
+		 * Did we write the wrong # entries in the header?
+		 * If so, rewrite it like in fast-import
+		 */
+		if (pack_to_stdout || nr_written == nr_remaining) {
+			sha1close(f, pack_file_sha1, 1);
+		} else {
+			sha1close(f, pack_file_sha1, 0);
+			fixup_pack_header_footer(f->fd, pack_file_sha1, pack_tmp_name, nr_written);
+			close(f->fd);
+		}
+
+		if (!pack_to_stdout) {
 			unsigned char object_list_sha1[20];
 			mode_t mode = umask(0);
 
@@ -652,7 +663,26 @@ static void write_pack_file(void)
 				die("unable to rename temporary index file: %s",
 				    strerror(errno));
 			puts(sha1_to_hex(object_list_sha1));
+		}
+
+		/* mark written objects as written to previous pack */
+		for (j = 0; j < nr_written; j++) {
+			written_list[j]->offset = (off_t)-1;
+		}
+		nr_remaining -= nr_written;
+	} while (nr_remaining && i < nr_objects);
+
+	free(written_list);
+	if (do_progress)
+		stop_progress(&progress_state);
+	if (written != nr_result)
+		die("wrote %u objects while expecting %u", written, nr_result);
+	for (j = 0; i < nr_objects; i++) {
+		struct object_entry *e = objects + i;
+		j += !e->offset && !e->preferred_base;
 	}
+	if (j)
+		die("wrote %u objects as expected but %u unwritten", written, j);
 }
 
 static int sha1_sort(const void *_a, const void *_b)
@@ -679,18 +709,11 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 	idx_tmp_name = xstrdup(tmpname);
 	f = sha1fd(fd, idx_tmp_name);
 
-	if (nr_result) {
-		uint32_t j = 0;
-		sorted_by_sha =
-			xcalloc(nr_result, sizeof(struct object_entry *));
-		for (i = 0; i < nr_objects; i++)
-			if (!objects[i].preferred_base)
-				sorted_by_sha[j++] = objects + i;
-		if (j != nr_result)
-			die("listed %u objects while expecting %u", j, nr_result);
-		qsort(sorted_by_sha, nr_result, sizeof(*sorted_by_sha), sha1_sort);
+	if (nr_written) {
+		sorted_by_sha = written_list;
+		qsort(sorted_by_sha, nr_written, sizeof(*sorted_by_sha), sha1_sort);
 		list = sorted_by_sha;
-		last = sorted_by_sha + nr_result;
+		last = sorted_by_sha + nr_written;
 	} else
 		sorted_by_sha = list = last = NULL;
 
@@ -728,7 +751,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 
 	/* Write the actual SHA1 entries. */
 	list = sorted_by_sha;
-	for (i = 0; i < nr_result; i++) {
+	for (i = 0; i < nr_written; i++) {
 		struct object_entry *entry = *list++;
 		if (index_version < 2) {
 			uint32_t offset = htonl(entry->offset);
@@ -743,7 +766,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 
 		/* write the crc32 table */
 		list = sorted_by_sha;
-		for (i = 0; i < nr_objects; i++) {
+		for (i = 0; i < nr_written; i++) {
 			struct object_entry *entry = *list++;
 			uint32_t crc32_val = htonl(entry->crc32);
 			sha1write(f, &crc32_val, 4);
@@ -751,7 +774,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 
 		/* write the 32-bit offset table */
 		list = sorted_by_sha;
-		for (i = 0; i < nr_objects; i++) {
+		for (i = 0; i < nr_written; i++) {
 			struct object_entry *entry = *list++;
 			uint32_t offset = (entry->offset <= index_off32_limit) ?
 				entry->offset : (0x80000000 | nr_large_offset++);
@@ -776,7 +799,6 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 
 	sha1write(f, pack_file_sha1, 20);
 	sha1close(f, NULL, 1);
-	free(sorted_by_sha);
 	SHA1_Final(sha1, &ctx);
 }
 
-- 
1.5.2.rc3.726.g279d


[-- Attachment #6: 0004-git-repack-max-pack-size-add-option-parsing-to-en.patch --]
[-- Type: text/plain, Size: 4311 bytes --]

>From 279d1f9e0e7265c6ccf8759bbe8c5539bf3089fa Mon Sep 17 00:00:00 2001
From: Dana L. How <danahow@gmail.com>
Date: Sun, 13 May 2007 12:47:09 -0700
Subject: [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature

Add --max-pack-size parsing and usage messages.
Upgrade git-repack.sh to handle multiple packfile names,
and build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR.
Update documentation.

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 Documentation/git-pack-objects.txt |    5 +++++
 Documentation/git-repack.txt       |    5 +++++
 builtin-pack-objects.c             |    9 ++++++++-
 git-repack.sh                      |   14 ++++++++------
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 2531238..cfe127a 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -85,6 +85,11 @@ base-name::
 	times to get to the necessary object.
 	The default value for --window is 10 and --depth is 50.
 
+--max-pack-size=<n>::
+	Maximum size of each output packfile, expressed in MiB.
+	If specified,  multiple packfiles may be created.
+	The default is unlimited.
+
 --incremental::
 	This flag causes an object already in a pack ignored
 	even if it appears in the standard input.
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index cc3b0b2..2847c9b 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -65,6 +65,11 @@ OPTIONS
 	to be applied that many times to get to the necessary object.
 	The default value for --window is 10 and --depth is 50.
 
+--max-pack-size=<n>::
+	Maximum size of each output packfile, expressed in MiB.
+	If specified,  multiple packfiles may be created.
+	The default is unlimited.
+
 
 Configuration
 -------------
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index ce9eb2d..930b57a 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -15,7 +15,7 @@
 #include "progress.h"
 
 static const char pack_usage[] = "\
-git-pack-objects [{ -q | --progress | --all-progress }] \n\
+git-pack-objects [{ -q | --progress | --all-progress }] [--max-pack-size=N] \n\
 	[--local] [--incremental] [--window=N] [--depth=N] \n\
 	[--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \n\
 	[--non-empty] [--revs [--unpacked | --all]*] [--reflog] \n\
@@ -1713,6 +1713,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			pack_compression_level = level;
 			continue;
 		}
+		if (!prefixcmp(arg, "--max-pack-size=")) {
+			char *end;
+			pack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;
+			if (!arg[16] || *end)
+				usage(pack_usage);
+			continue;
+		}
 		if (!prefixcmp(arg, "--window=")) {
 			char *end;
 			window = strtoul(arg+9, &end, 0);
diff --git a/git-repack.sh b/git-repack.sh
index 8bf66a4..4ea6e5b 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2005 Linus Torvalds
 #
 
-USAGE='[-a] [-d] [-f] [-l] [-n] [-q] [--window=N] [--depth=N]'
+USAGE='[-a] [-d] [-f] [-l] [-n] [-q] [--max-pack-size=N] [--window=N] [--depth=N]'
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
 
@@ -18,6 +18,7 @@ do
 	-q)	quiet=-q ;;
 	-f)	no_reuse=--no-reuse-object ;;
 	-l)	local=--local ;;
+	--max-pack-size=*) extra="$extra $1" ;;
 	--window=*) extra="$extra $1" ;;
 	--depth=*) extra="$extra $1" ;;
 	*)	usage ;;
@@ -35,7 +36,7 @@ true)
 esac
 
 PACKDIR="$GIT_OBJECT_DIRECTORY/pack"
-PACKTMP="$GIT_DIR/.tmp-$$-pack"
+PACKTMP="$GIT_OBJECT_DIRECTORY/.tmp-$$-pack"
 rm -f "$PACKTMP"-*
 trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
 
@@ -62,11 +63,12 @@ case ",$all_into_one," in
 esac
 
 args="$args $local $quiet $no_reuse$extra"
-name=$(git-pack-objects --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
+names=$(git-pack-objects --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
 	exit 1
-if [ -z "$name" ]; then
+if [ -z "$names" ]; then
 	echo Nothing new to pack.
-else
+fi
+for name in $names ; do
 	chmod a-w "$PACKTMP-$name.pack"
 	chmod a-w "$PACKTMP-$name.idx"
 	if test "$quiet" != '-q'; then
@@ -92,7 +94,7 @@ else
 		exit 1
 	}
 	rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
-fi
+done
 
 if test "$remove_redundant" = t
 then
-- 
1.5.2.rc3.726.g279d

Re: Rebase max-pack-size?

[Junio C Hamano]

Dana How <danahow@gmail.com> writes:

> Let me know if you want them re-sent inline and/or separately.

Recently Linus was very unhappy on the kernel list when somebody
forwarded bunch of patches in a single message, saying we do
reviews one patch per message (and we do not do attachments).

I haven't been very strict about that kind of formality, as I
haven't felt the need too much so far.  Let's see how well we
can do this with this message, which has 5 patches.

----------------------------------------------------------------
>>From 781820f147bcb12dae576734585b27f42faca3ea Mon Sep 17 00:00:00 2001
> From: Dana L. How <danahow@gmail.com>
> Date: Sun, 13 May 2007 11:28:19 -0700
> Subject: [PATCH] Alter sha1close() 3rd argument to request flush only
>
> update=0 suppressed writing the final SHA-1 but was not used.
> Now final=0 suppresses SHA-1 finalization, SHA-1 writing,
> and closing -- in other words,  only flush the buffer.
>
> Signed-off-by: Dana L. How <danahow@gmail.com>

What it does is understandable but it somehow feels funny that
"sha1close(file, hashresult, 0)" does _not_ close it (and does
not hash either for obvious reasons ;-).  I would say we should
let it pass this round, but might want to separate the first
part out into a separate "update hash and flush" function if we
get more callers.

----------------------------------------------------------------
>>From 0c75fd66aa6a758e245d85b9304e85a38e977f94 Mon Sep 17 00:00:00 2001
> From: Dana L. How <danahow@gmail.com>
> Date: Sun, 13 May 2007 11:34:56 -0700
> Subject: [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring
>
> Add "pack_size_limit", the limit specified by --max-pack-size,
> "written_list", the list of objects written to the current pack,
> and "nr_written", the number of objects in written_list.
> Put "base_name" at file scope again and add forward declarations.
> Move write_index_file() call from cnd_pack_objects() to
> write_pack_file() since only the latter will know how
> many times to call write_index_file().
>
> Signed-off-by: Dana L. How <danahow@gmail.com>

I would have split this part a bit differently.

This is mostly about restructuring the code so that
write_index_file() is called from write_pack_file(), which by
itself is a very good change (but then we might have been better
off passing basename as a parameter).

You are not using written_list nor limit yet but are introducing
them in this step, which feels not quite right.  I usually compile stuff
with -Werror, and if I ever have to bisect the series, this would bomb out
for these unused variables.  Not nice.

----------------------------------------------------------------
>>From 40ac2f294ec68cae27f8a9262db4cab84f1d2257 Mon Sep 17 00:00:00 2001
> From: Dana L. How <danahow@gmail.com>
> Date: Sun, 13 May 2007 12:06:18 -0700
> Subject: [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit
>
> With --max-pack-size,  generate the appropriate write limit
> for each object and check against it before each group of writes.
> Update delta usability rules to handle base being in a previously-
> written pack.  Inline sha1write_compress() so we know the
> exact size of the written data when it needs to be compressed.
> Detect and return write "failure".
>
> Signed-off-by: Dana L. How <danahow@gmail.com>
> ---
>  builtin-pack-objects.c |  131 ++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 104 insertions(+), 27 deletions(-)

Again, this is split somewhat wrongly, as you do not have a way
to set max size from the caller, but more importantly, even
though write_one and write_object knows to obey the limit
(perhaps somebody bisecting this series later may set the limit
under the debugger, to work around the lack of option parser),
write_pack_file() does not notice zero return from write_one();
I would have added a check there to do:

	die("sorry, limit reached and we do not have code to split the pack yet.")

which obviously can be updated in the next patch.

----------------------------------------------------------------
>>From 9ec3af1f52f41d0737b6271fded5e94527eb0466 Mon Sep 17 00:00:00 2001
> From: Dana L. How <danahow@gmail.com>
> Date: Sun, 13 May 2007 12:09:16 -0700
> Subject: [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()
>
> Rewrite write_pack_file() to break to a new packfile
> whenever write_object/write_one request it,  and
> correct the header's object count in the previous packfile.
> Change write_index_file() to write an index
> for just the objects in the most recent packfile.
>
> Signed-off-by: Dana L. How <danahow@gmail.com>
> ---
>  builtin-pack-objects.c |  120 ++++++++++++++++++++++++++++-------------------
>  1 files changed, 71 insertions(+), 49 deletions(-)
>
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 3023aac..ce9eb2d 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> ...
> @@ -652,7 +663,26 @@ static void write_pack_file(void)
>  				die("unable to rename temporary index file: %s",
>  				    strerror(errno));
>  			puts(sha1_to_hex(object_list_sha1));
> +		}
> +
> +		/* mark written objects as written to previous pack */
> +		for (j = 0; j < nr_written; j++) {
> +			written_list[j]->offset = (off_t)-1;
> +		}
> +		nr_remaining -= nr_written;
> +	} while (nr_remaining && i < nr_objects);
> +
> +	free(written_list);
> +	if (do_progress)
> +		stop_progress(&progress_state);
> +	if (written != nr_result)
> +		die("wrote %u objects while expecting %u", written, nr_result);
> +	for (j = 0; i < nr_objects; i++) {
> +		struct object_entry *e = objects + i;
> +		j += !e->offset && !e->preferred_base;
>  	}

I am a bit confused by this loop.  Don't you have to start with i=0
for this check to be meaningful?

> +	if (j)
> +		die("wrote %u objects as expected but %u unwritten", written, j);
>  }


----------------------------------------------------------------
>>From 279d1f9e0e7265c6ccf8759bbe8c5539bf3089fa Mon Sep 17 00:00:00 2001
> From: Dana L. How <danahow@gmail.com>
> Date: Sun, 13 May 2007 12:47:09 -0700
> Subject: [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature
>
> Add --max-pack-size parsing and usage messages.
> Upgrade git-repack.sh to handle multiple packfile names,
> and build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR.
> Update documentation.
>
> Signed-off-by: Dana L. How <danahow@gmail.com>
> ---
>  Documentation/git-pack-objects.txt |    5 +++++
>  Documentation/git-repack.txt       |    5 +++++
>  builtin-pack-objects.c             |    9 ++++++++-
>  git-repack.sh                      |   14 ++++++++------
>  4 files changed, 26 insertions(+), 7 deletions(-)
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index ce9eb2d..930b57a 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> ...
> @@ -1713,6 +1713,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  			pack_compression_level = level;
>  			continue;
>  		}
> +		if (!prefixcmp(arg, "--max-pack-size=")) {
> +			char *end;
> +			pack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;
> +			if (!arg[16] || *end)
> +				usage(pack_usage);
> +			continue;
> +		}
>  		if (!prefixcmp(arg, "--window=")) {
>  			char *end;
>  			window = strtoul(arg+9, &end, 0);

Hmmm.  I was almost going to suggest to have this spelled in
bytes, with suffixes like k/m/g.  However, because wanting to
limit a pack under 1.4MB does not make much sense these days,
and because having to spell "up to 2GB" as 2047 is not too much
trouble, I think this is Ok.

Shouldn't we have a safety to error out when --stdout and
--max-pack-size are both given?  Currently it silently ignores
the limit, doesn't it?

> diff --git a/git-repack.sh b/git-repack.sh
> index 8bf66a4..4ea6e5b 100755
> --- a/git-repack.sh
> +++ b/git-repack.sh
> ...
> @@ -35,7 +36,7 @@ true)
>  esac
>  
>  PACKDIR="$GIT_OBJECT_DIRECTORY/pack"
> -PACKTMP="$GIT_DIR/.tmp-$$-pack"
> +PACKTMP="$GIT_OBJECT_DIRECTORY/.tmp-$$-pack"
>  rm -f "$PACKTMP"-*
>  trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
>  

Although this is a good change, this hunk does not belong to
this.

----------------------------------------------------------------

Overall everything looks good, except some minor details noted
above.  Separation of the commits into logical steps does not
need to be fixed up (they are already in 'next'), but follow-up
patches might be needed.

And I have to agree with Linus; responding this way was more
cumbersome than it should have been.

Re: Rebase max-pack-size?

[Dana How]

On 5/22/07, Junio C Hamano <junkio@cox.net> wrote:
> Dana How <danahow@gmail.com> writes:
> ----------------------------------------------------------------
> > Subject: [PATCH] Alter sha1close() 3rd argument to request flush only
> >
> > update=0 suppressed writing the final SHA-1 but was not used.
> > Now final=0 suppresses SHA-1 finalization, SHA-1 writing,
> > and closing -- in other words,  only flush the buffer.
>
> What it does is understandable but it somehow feels funny that
> "sha1close(file, hashresult, 0)" does _not_ close it (and does
> not hash either for obvious reasons ;-).  I would say we should
> let it pass this round, but might want to separate the first
> part out into a separate "update hash and flush" function if we
> get more callers.
OK,  see the list at the end.

> ----------------------------------------------------------------
> > Subject: [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring
> >
> > Add "pack_size_limit", the limit specified by --max-pack-size,
> > "written_list", the list of objects written to the current pack,
> > and "nr_written", the number of objects in written_list.
> > Put "base_name" at file scope again and add forward declarations.
> > Move write_index_file() call from cnd_pack_objects() to
> > write_pack_file() since only the latter will know how
> > many times to call write_index_file().
>
> I would have split this part a bit differently.
>
> This is mostly about restructuring the code so that
> write_index_file() is called from write_pack_file(), which by
> itself is a very good change (but then we might have been better
> off passing basename as a parameter).
That would have worked too.  I made base_name file scope
b/c that's how it was before NP's immediatley previous patches.

> You are not using written_list nor limit yet but are introducing
> them in this step, which feels not quite right.  I usually compile stuff
> with -Werror, and if I ever have to bisect the series, this would bomb
> out for these unused variables.  Not nice.
Very good point -- I did not think of that.
I will make sure in the future
each patch in a patchset has no warnings.

> ----------------------------------------------------------------
> > Subject: [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit
> >
> > With --max-pack-size,  generate the appropriate write limit
> > for each object and check against it before each group of writes.
> > Update delta usability rules to handle base being in a previously-
> > written pack.  Inline sha1write_compress() so we know the
> > exact size of the written data when it needs to be compressed.
> > Detect and return write "failure".
>
> Again, this is split somewhat wrongly, as you do not have a way
> to set max size from the caller, but more importantly, even
> though write_one and write_object knows to obey the limit
> (perhaps somebody bisecting this series later may set the limit
> under the debugger, to work around the lack of option parser),
> write_pack_file() does not notice zero return from write_one();
> I would have added a check there to do:
>
>         die("sorry, limit reached and we do not have code to split the pack yet.")
>
> which obviously can be updated in the next patch.
This makes sense to me.  In a previous version of the patchset,
I had some temporary code like you say (just some extra
arguments, not any checking or die() calls) which was
immediately changed/replaced in the next patch.
Shawn or Nicolas didn't like that,
so I migrated to the split I used here.

I didn't really intend bisecting this patchset to do
anything useful *with respect to max-pack-size functionality*,
but bisecting it would be useful to detect if these patches
had broken something else.  But I can rethink that in
the future.

> ----------------------------------------------------------------
> > Subject: [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()
> >
> > Rewrite write_pack_file() to break to a new packfile
> > whenever write_object/write_one request it,  and
> > correct the header's object count in the previous packfile.
> > Change write_index_file() to write an index
> > for just the objects in the most recent packfile.
> >
> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> > --- a/builtin-pack-objects.c
> > +++ b/builtin-pack-objects.c
> > @@ -652,7 +663,26 @@ static void write_pack_file(void)
> > +     } while (nr_remaining && i < nr_objects);
> > +
> > +     for (j = 0; i < nr_objects; i++) {
> > +             struct object_entry *e = objects + i;
> > +             j += !e->offset && !e->preferred_base;
> > +     }
> > +     if (j)
> > +             die("wrote %u objects as expected but %u unwritten", written, j);
> I am a bit confused by this loop.  Don't you have to start with i=0
> for this check to be meaningful?
The previous do-while loop whose last line is shown
could end with i < nr_objects due to nr_remaining becoming
0.  This means the objects [i ... nr_objects) have not been
inspected.  They should all be either already written
or non-writable,  which is what the two terms in the && expression
are testing.  See list at end.

> ----------------------------------------------------------------
> > Subject: [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature
> >
> > Add --max-pack-size parsing and usage messages.
> > Upgrade git-repack.sh to handle multiple packfile names,
> > and build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR.
> > Update documentation.
> >
> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> > --- a/builtin-pack-objects.c
> > +++ b/builtin-pack-objects.c
> > @@ -1713,6 +1713,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> > +             if (!prefixcmp(arg, "--max-pack-size=")) {
> > +                     char *end;
> > +                     pack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;
> > +                     if (!arg[16] || *end)
> > +                             usage(pack_usage);
> > +                     continue;
> > +             }
>
> Hmmm.  I was almost going to suggest to have this spelled in
> bytes, with suffixes like k/m/g.  However, because wanting to
> limit a pack under 1.4MB does not make much sense these days,
> and because having to spell "up to 2GB" as 2047 is not too much
> trouble, I think this is Ok.
I thought about suffixes too but I chose this way
b/c that's how git fast-import does it.

> Shouldn't we have a safety to error out when --stdout and
> --max-pack-size are both given?  Currently it silently ignores
> the limit, doesn't it?
Yes & yes.  Previously there was disagreement on this point.
One preference was to disallow the combination,
another was that it was useful and should be supported.
So I left the code in a state where a small follow-on patch
could make the decision.  The current behavior is not
to disallow the combination,  and it is well-defined
(you get a sequence of packs on stdout,  concatenated,
whose headers indicate how many objects there are
in the current pack and all following).

I do not think --stdout && --max-pack-size is currently
useful,  and a follow-on patch should complain.
See list at end.

> > diff --git a/git-repack.sh b/git-repack.sh
> > --- a/git-repack.sh
> > +++ b/git-repack.sh
> > @@ -35,7 +36,7 @@ true)
> > -PACKTMP="$GIT_DIR/.tmp-$$-pack"
> > +PACKTMP="$GIT_OBJECT_DIRECTORY/.tmp-$$-pack"
>
> Although this is a good change, this hunk does not belong to
> this.
OK.
If you decide not to keep this change here for historical clarity,
I can add it back later.

> Overall everything looks good, except some minor details noted
> above.  Separation of the commits into logical steps does not
> need to be fixed up (they are already in 'next'), but follow-up
> patches might be needed.
OK,  I will not edit and re-submit these patches.
But I will submit two follow-on patches:

Patch 1:
* Pull "sha1flush()" or similar out of sha1close() inside csum-file.c.
  This will require some edits to callers.

Patch 2:
* Add a comment to "confusing loop" to explain what it's checking.
* Complain about --stdout && --max-pack-size combination.

This will have to happen tomorrow.

> And I have to agree with Linus; responding this way was more
> cumbersome than it should have been.
Understood.

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell

Re: Rebase max-pack-size?

[Junio C Hamano]

"Dana How" <danahow@gmail.com> writes:

> Patch 1:
> * Pull "sha1flush()" or similar out of sha1close() inside csum-file.c.
>  This will require some edits to callers.
>
> Patch 2:
> * Add a comment to "confusing loop" to explain what it's checking.
> * Complain about --stdout && --max-pack-size combination.
>
> This will have to happen tomorrow.
>
>> And I have to agree with Linus; responding this way was more
>> cumbersome than it should have been.
> Understood.
>
> Thanks,

Although I mentioned it, as we have only one caller that does
not want to close the file, we probably do not need the first
patch right now.

Also thanks for the clarification on the (nr_result != nr_objects)
stuff -- it should have been obvious, as I was the guilty party
who originally introduced that condition (i.e. "thin" packs).