fetch fails with a short read of received pack

Somehow I got my gnulib mirror to a strange state where I can't fetch
anything from it:

    $ cd gnulib
    $ git fetch -f ../gnulib.git 'refs/heads/*:refs/remotes/origin/*'
    remote: Counting objects: 2202, done.
    remote: Compressing objects: 100% (633/633), done.
    remote: Total 1769 (delta 1448), reused 1455 (delta 1134)
    Receiving objects: 100% (1769/1769), 404.11 KiB, done.
    fatal: premature end of pack file, 1745 bytes missing
    fatal: index-pack failed

This is with current Shawn's master (Junio's master erroneously gives
an error). Bisect points at ac0463ed44e859c84e354cd0ae47d9b5b124e112

    pack-objects: use fixup_pack_header_footer()'s validation mode

    When limiting the pack size, a new header has to be written to the
    pack and a new SHA1 computed.  Make sure that the SHA1 of what is being
    read back matches the SHA1 of what was written.

This and the diff make no sense to me (as to what could be broken).
I made both repos available at:

    The source, it unpacks in gitlib.git (as in example above)
	http://vin-soft.org/~ftp-tmp/gnulib-broken-src.tar.bz2

    The target, where the fetch in the example is run
	http://vin-soft.org/~ftp-tmp/gnulib-broken-tgt.tar.bz2

Could someone who actually understands the code please have a look?

Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for 
its ability to compute a SHA1 on the written data.  This also provides 
data buffering which, along with commit 92392b4a45, will confuse pread() 
whenever an appended object is 1) freed due to memory pressure because 
of the depth-first delta processing, and 2) needed again because it has 
many delta children, and 3) its data is still buffered by sha1write().

Let's fix the issue by simply forcing cached data out when such an 
object is written so it can be pread()'d at leisure.

Signed-off-by: Nicolas Pitre <redacted>

---

On Thu, 9 Oct 2008, Alex Riesen wrote:

Somehow I got my gnulib mirror to a strange state where I can't fetch
anything from it:

    $ cd gnulib
    $ git fetch -f ../gnulib.git 'refs/heads/*:refs/remotes/origin/*'
    remote: Counting objects: 2202, done.
    remote: Compressing objects: 100% (633/633), done.
    remote: Total 1769 (delta 1448), reused 1455 (delta 1134)
    Receiving objects: 100% (1769/1769), 404.11 KiB, done.
    fatal: premature end of pack file, 1745 bytes missing
    fatal: index-pack failed

This is with current Shawn's master (Junio's master erroneously gives
an error). Bisect points at ac0463ed44e859c84e354cd0ae47d9b5b124e112

Thanks for providing a good test data set.  As you can see above, this 
is something that is tricky to reproduce otherwise.  This patch is meant 
for the maint branch but should be included in master as well.

diff --git a/csum-file.c b/csum-file.c
index bb70c75..3b3e090 100644
--- a/csum-file.c
+++ b/csum-file.c

@@ -11,7 +11,7 @@
 #include "progress.h"
 #include "csum-file.h"
 
-static void sha1flush(struct sha1file *f, void *buf, unsigned int count)
+static void flush(struct sha1file *f, void *buf, unsigned int count)
 {
 	for (;;) {
 		int ret = xwrite(f->fd, buf, count);

@@ -30,22 +30,28 @@ static void sha1flush(struct sha1file *f, void *buf, unsigned int count)
 	}
 }
 
-int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)
+void sha1flush(struct sha1file *f)
 {
-	int fd;
 	unsigned offset = f->offset;
 
 	if (offset) {
 		SHA1_Update(&f->ctx, f->buffer, offset);
-		sha1flush(f, f->buffer, offset);
+		flush(f, f->buffer, offset);
 		f->offset = 0;
 	}
+}
+
+int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)
+{
+	int fd;
+
+	sha1flush(f);
 	SHA1_Final(f->buffer, &f->ctx);
 	if (result)
 		hashcpy(result, f->buffer);
 	if (flags & (CSUM_CLOSE | CSUM_FSYNC)) {
 		/* write checksum and close fd */
-		sha1flush(f, f->buffer, 20);
+		flush(f, f->buffer, 20);
 		if (flags & CSUM_FSYNC)
 			fsync_or_die(f->fd, f->name);
 		if (close(f->fd))

@@ -83,7 +89,7 @@ int sha1write(struct sha1file *f, void *buf, unsigned int count)
 		left -= nr;
 		if (!left) {
 			SHA1_Update(&f->ctx, data, offset);
-			sha1flush(f, data, offset);
+			flush(f, data, offset);
 			offset = 0;
 		}
 		f->offset = offset;

diff --git a/csum-file.h b/csum-file.h
index 72c9487..01f13b5 100644
--- a/csum-file.h
+++ b/csum-file.h

@@ -24,6 +24,7 @@ extern struct sha1file *sha1fd(int fd, const char *name);
 extern struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp);
 extern int sha1close(struct sha1file *, unsigned char *, unsigned int);
 extern int sha1write(struct sha1file *, void *, unsigned int);
+extern void sha1flush(struct sha1file *f);
 extern void crc32_begin(struct sha1file *);
 extern uint32_t crc32_end(struct sha1file *);
 

diff --git a/index-pack.c b/index-pack.c
index 530d820..ca72329 100644
--- a/index-pack.c
+++ b/index-pack.c

@@ -704,6 +704,7 @@ static struct object_entry *append_obj_to_pack(struct sha1file *f,
 	obj[1].idx.offset = obj[0].idx.offset + n;
 	obj[1].idx.offset += write_compressed(f, buf, size);
 	obj[0].idx.crc32 = crc32_end(f);
+	sha1flush(f);
 	hashcpy(obj->idx.sha1, sha1);
 	return obj;
 }

2008/10/10 Nicolas Pitre [off-list ref]:

Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for
its ability to compute a SHA1 on the written data.  This also provides
data buffering which, along with commit 92392b4a45, will confuse pread()
whenever an appended object is 1) freed due to memory pressure because
of the depth-first delta processing, and 2) needed again because it has
many delta children, and 3) its data is still buffered by sha1write().

So I seem to have had luck not doing gc in either of source or target repos
(I usually repack, than fetch. Especially this one repo, it's very active).

BTW, I run into complications trying to test the fix on Shawn's
master: conflicts,
and my trivial resolution wasn't good enough.

Aside from that, I confirm the case fixed.

Alex Riesen [off-list ref] wrote:

2008/10/10 Nicolas Pitre [off-list ref]:

quoted

Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for
its ability to compute a SHA1 on the written data.  This also provides
data buffering which, along with commit 92392b4a45, will confuse pread()
whenever an appended object is 1) freed due to memory pressure because
of the depth-first delta processing, and 2) needed again because it has
many delta children, and 3) its data is still buffered by sha1write().

BTW, I run into complications trying to test the fix on Shawn's
master: conflicts,
and my trivial resolution wasn't good enough.

Huh.

Color me confused.  What the heck was the base version?

Near as I can tell Nico wrote this fix against a8032d12 (sha1write:
don't copy full sized buffers).  This is in Junio's master.

But the changes conflict with what is in Junio's maint.  So the
patch doesn't apply cleanly due to the sha1flush function's
signature changing.

And the changes conflict with 9126f009 (fix openssl headers
conflicting with custom SHA1 implementations), which is in my
master, and was written by Nico.

;-)

Anyway, its merged now.

-- 
Shawn.

On Fri, 10 Oct 2008, Shawn O. Pearce wrote:

Alex Riesen [off-list ref] wrote:

quoted

2008/10/10 Nicolas Pitre [off-list ref]:

quoted

Since v1.6.0.2~13^2~ the completion of a thin pack uses sha1write() for
its ability to compute a SHA1 on the written data.  This also provides
data buffering which, along with commit 92392b4a45, will confuse pread()
whenever an appended object is 1) freed due to memory pressure because
of the depth-first delta processing, and 2) needed again because it has
many delta children, and 3) its data is still buffered by sha1write().

BTW, I run into complications trying to test the fix on Shawn's
master: conflicts,
and my trivial resolution wasn't good enough.

Huh.

Color me confused.  What the heck was the base version?

Near as I can tell Nico wrote this fix against a8032d12 (sha1write:
don't copy full sized buffers).  This is in Junio's master.

But the changes conflict with what is in Junio's maint.  So the
patch doesn't apply cleanly due to the sha1flush function's
signature changing.

And the changes conflict with 9126f009 (fix openssl headers
conflicting with custom SHA1 implementations), which is in my
master, and was written by Nico.

;-)

Whatever.

Your truly is still recovering from a knee surgery and therefore is 
literally on drugs.

Anyway, its merged now.

Thanks.  The important thing is that it must be included in what would 
be v1.6.0.3.


Nicolas