[JGIT PATCH 1/5] Remove dead/unused reset method from CountingOutputStream

[JGIT PATCH 1/5] Remove dead/unused reset method from CountingOutputStream

[Shawn O. Pearce]

We never reset the stream, so just remove that as a valid option.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/util/CountingOutputStream.java    |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java b/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
index b0b5f7d..5f333f5 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
@@ -72,17 +72,9 @@ public void write(byte[] b, int off, int len) throws IOException {
 	/**
 	 * Return number of already written bytes.
 	 *
-	 * @return number of written bytes since last reset (object is reset upon
-	 *         creation)
+	 * @return number of written bytes since stream start.
 	 */
 	public long getCount() {
 		return count;
 	}
-
-	/**
-	 * Reset counter to zero value.
-	 */
-	public void reset() {
-		count = 0;
-	}
 }
-- 
1.6.2.1.471.g682837

[JGIT PATCH 2/5] Implement CRC32 computation during PackWriter

[Shawn O. Pearce]

To correctly create a v2 index file for a pack we must compute the
CRC32 code for each object entry as they are written out, otherwise
the CRC32 table will be full of 0's, and fail verification if the
resulting pack were to be reused.

Reported-by: Daniel Cheng (aka SDiZ) <j16sdiz+freenet@gmail.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../tst/org/spearce/jgit/lib/PackWriterTest.java   |   17 +++---
 .../PackOutputStream.java}                         |   65 +++++++++++++-------
 .../src/org/spearce/jgit/lib/PackWriter.java       |   16 ++---
 3 files changed, 57 insertions(+), 41 deletions(-)
 rename org.spearce.jgit/src/org/spearce/jgit/{util/CountingOutputStream.java => lib/PackOutputStream.java} (61%)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
index f7139fc..46616e3 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
@@ -56,7 +56,6 @@
 import org.spearce.jgit.revwalk.RevObject;
 import org.spearce.jgit.revwalk.RevWalk;
 import org.spearce.jgit.transport.IndexPack;
-import org.spearce.jgit.util.CountingOutputStream;
 import org.spearce.jgit.util.JGitTestUtil;
 
 public class PackWriterTest extends RepositoryTestCase {
@@ -71,7 +70,7 @@
 
 	private ByteArrayOutputStream os;
 
-	private CountingOutputStream cos;
+	private PackOutputStream cos;
 
 	private File packBase;
 
@@ -84,7 +83,7 @@
 	public void setUp() throws Exception {
 		super.setUp();
 		os = new ByteArrayOutputStream();
-		cos = new CountingOutputStream(os);
+		cos = new PackOutputStream(os);
 		packBase = new File(trash, "tmp_pack");
 		packFile = new File(trash, "tmp_pack.pack");
 		indexFile = new File(trash, "tmp_pack.idx");
@@ -308,11 +307,11 @@ public void testWritePack4ThinPack() throws IOException {
 	 */
 	public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {
 		testWritePack2();
-		final long sizePack2NoDeltas = cos.getCount();
+		final long sizePack2NoDeltas = cos.length();
 		tearDown();
 		setUp();
 		testWritePack2DeltasReuseRefs();
-		final long sizePack2DeltasRefs = cos.getCount();
+		final long sizePack2DeltasRefs = cos.length();
 
 		assertTrue(sizePack2NoDeltas > sizePack2DeltasRefs);
 	}
@@ -327,11 +326,11 @@ public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {
 	 */
 	public void testWritePack2SizeOffsetsVsRefs() throws Exception {
 		testWritePack2DeltasReuseRefs();
-		final long sizePack2DeltasRefs = cos.getCount();
+		final long sizePack2DeltasRefs = cos.length();
 		tearDown();
 		setUp();
 		testWritePack2DeltasReuseOffsets();
-		final long sizePack2DeltasOffsets = cos.getCount();
+		final long sizePack2DeltasOffsets = cos.length();
 
 		assertTrue(sizePack2DeltasRefs > sizePack2DeltasOffsets);
 	}
@@ -345,11 +344,11 @@ public void testWritePack2SizeOffsetsVsRefs() throws Exception {
 	 */
 	public void testWritePack4SizeThinVsNoThin() throws Exception {
 		testWritePack4();
-		final long sizePack4 = cos.getCount();
+		final long sizePack4 = cos.length();
 		tearDown();
 		setUp();
 		testWritePack4ThinPack();
-		final long sizePack4Thin = cos.getCount();
+		final long sizePack4Thin = cos.length();
 
 		assertTrue(sizePack4 > sizePack4Thin);
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackOutputStream.java
similarity index 61%
rename from org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
rename to org.spearce.jgit/src/org/spearce/jgit/lib/PackOutputStream.java
index 5f333f5..403b892 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackOutputStream.java
@@ -1,4 +1,5 @@
 /*
+ * Copyright (C) 2009, Google Inc.
  * Copyright (C) 2008, Marek Zawirski <marek.zawirski@gmail.com>
  *
  * All rights reserved.
@@ -35,46 +36,66 @@
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-package org.spearce.jgit.util;
+package org.spearce.jgit.lib;
 
-import java.io.FilterOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.security.MessageDigest;
+import java.util.zip.CRC32;
+
+/** Custom output stream to support {@link PackWriter}. */
+final class PackOutputStream extends OutputStream {
+	private final OutputStream out;
+
+	private final CRC32 crc = new CRC32();
+
+	private final MessageDigest md = Constants.newMessageDigest();
 
-/**
- * Counting output stream decoration. Counts bytes written to stream.
- */
-public class CountingOutputStream extends FilterOutputStream {
 	private long count;
 
-	/**
-	 * Create counting stream being decorated to provided real output stream.
-	 *
-	 * @param out
-	 *            output stream where data should be written
-	 */
-	public CountingOutputStream(OutputStream out) {
-		super(out);
+	PackOutputStream(final OutputStream out) {
+		this.out = out;
 	}
 
 	@Override
-	public void write(int b) throws IOException {
+	public void write(final int b) throws IOException {
 		out.write(b);
+		crc.update(b);
+		md.update((byte) b);
 		count++;
 	}
 
 	@Override
-	public void write(byte[] b, int off, int len) throws IOException {
+	public void write(final byte[] b, final int off, final int len)
+			throws IOException {
 		out.write(b, off, len);
+		crc.update(b, off, len);
+		md.update(b, off, len);
 		count += len;
 	}
 
-	/**
-	 * Return number of already written bytes.
-	 *
-	 * @return number of written bytes since stream start.
-	 */
-	public long getCount() {
+	@Override
+	public void flush() throws IOException {
+		out.flush();
+	}
+
+	/** @return total number of bytes written since stream start. */
+	long length() {
 		return count;
 	}
+
+	/** @return obtain the current CRC32 register. */
+	int getCRC32() {
+		return (int) crc.getValue();
+	}
+
+	/** Reinitialize the CRC32 register for a new region. */
+	void resetCRC32() {
+		crc.reset();
+	}
+
+	/** @return obtain the current SHA-1 digest. */
+	byte[] getDigest() {
+		return md.digest();
+	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
index 601ce71..cfec35c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
@@ -40,7 +40,6 @@
 import java.io.BufferedOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
-import java.security.DigestOutputStream;
 import java.security.MessageDigest;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -57,7 +56,6 @@
 import org.spearce.jgit.revwalk.RevObject;
 import org.spearce.jgit.revwalk.RevSort;
 import org.spearce.jgit.transport.PackedObjectInfo;
-import org.spearce.jgit.util.CountingOutputStream;
 import org.spearce.jgit.util.NB;
 
 /**
@@ -166,9 +164,7 @@
 
 	private final Repository db;
 
-	private DigestOutputStream out;
-
-	private CountingOutputStream countingOut;
+	private PackOutputStream out;
 
 	private final Deflater deflater;
 
@@ -563,8 +559,7 @@ public void writePack(OutputStream packStream) throws IOException {
 
 		if (!(packStream instanceof BufferedOutputStream))
 			packStream = new BufferedOutputStream(packStream);
-		countingOut = new CountingOutputStream(packStream);
-		out = new DigestOutputStream(countingOut, Constants.newMessageDigest());
+		out = new PackOutputStream(packStream);
 
 		writeMonitor.beginTask(WRITING_OBJECTS_PROGRESS, getObjectsNumber());
 		writeHeader();
@@ -687,11 +682,13 @@ private void writeObject(final ObjectToPack otp) throws IOException {
 
 		assert !otp.isWritten();
 
-		otp.setOffset(countingOut.getCount());
+		out.resetCRC32();
+		otp.setOffset(out.length());
 		if (otp.isDeltaRepresentation())
 			writeDeltaObject(otp);
 		else
 			writeWholeObject(otp);
+		otp.setCRC(out.getCRC32());
 
 		writeMonitor.update(1);
 	}
@@ -753,8 +750,7 @@ private void writeObjectHeader(final int objectType, long dataLength)
 	}
 
 	private void writeChecksum() throws IOException {
-		out.on(false);
-		packcsum = out.getMessageDigest().digest();
+		packcsum = out.getDigest();
 		out.write(packcsum);
 	}
 
-- 
1.6.2.1.471.g682837

[JGIT PATCH 3/5] Test case for pack index CRC32 when written by PackWriter

[Shawn O. Pearce]

Suggested-by: Daniel Cheng (aka SDiZ) <j16sdiz+freenet@gmail.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../tst/org/spearce/jgit/lib/PackWriterTest.java   |   33 ++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
index 46616e3..636059f 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
@@ -40,6 +40,7 @@
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
+import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
@@ -353,6 +354,37 @@ public void testWritePack4SizeThinVsNoThin() throws Exception {
 		assertTrue(sizePack4 > sizePack4Thin);
 	}
 
+	public void testWriteIndex() throws Exception {
+		writer.setIndexVersion(2);
+		writeVerifyPack4(false);
+
+		// Validate that IndexPack came up with the right CRC32 value.
+		final PackIndex idx1 = PackIndex.open(indexFile);
+		assertTrue(idx1 instanceof PackIndexV2);
+		assertEquals(0x4743F1E4L, idx1.findCRC32(ObjectId
+				.fromString("82c6b885ff600be425b4ea96dee75dca255b69e7")));
+
+		// Validate that an index written by PackWriter is the same.
+		final File idx2File = new File(indexFile.getAbsolutePath() + ".2");
+		final FileOutputStream is = new FileOutputStream(idx2File);
+		try {
+			writer.writeIndex(is);
+		} finally {
+			is.close();
+		}
+		final PackIndex idx2 = PackIndex.open(idx2File);
+		assertTrue(idx2 instanceof PackIndexV2);
+		assertEquals(idx1.getObjectCount(), idx2.getObjectCount());
+		assertEquals(idx1.getOffset64Count(), idx2.getOffset64Count());
+
+		for (int i = 0; i < idx1.getObjectCount(); i++) {
+			final ObjectId id = idx1.getObjectId(i);
+			assertEquals(id, idx2.getObjectId(i));
+			assertEquals(idx1.findOffset(id), idx2.findOffset(id));
+			assertEquals(idx1.findCRC32(id), idx2.findCRC32(id));
+		}
+	}
+
 	// TODO: testWritePackDeltasCycle()
 	// TODO: testWritePackDeltasDepth()
 
@@ -469,6 +501,7 @@ private void verifyOpenPack(final boolean thin) throws IOException {
 		final IndexPack indexer = new IndexPack(db, is, packBase);
 		indexer.setKeepEmpty(true);
 		indexer.setFixThin(thin);
+		indexer.setIndexVersion(2);
 		indexer.index(new TextProgressMonitor());
 		pack = new PackFile(indexFile, packFile);
 	}
-- 
1.6.2.1.471.g682837

[JGIT PATCH 4/5] Write the pack header in one shot

[Shawn O. Pearce]

Instead of making 3 individual write calls of 4 bytes each, write
the entire header as a single 12 byte buffer.  Its also a little
bit easier to follow the final code, that these items appear at
the 0/4/8 offsets within the 12 byte long header.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/PackWriter.java       |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
index cfec35c..2f34255 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
@@ -644,13 +644,10 @@ private void selectObjectReuseForObject(final ObjectToPack otp,
 	}
 
 	private void writeHeader() throws IOException {
-		out.write(Constants.PACK_SIGNATURE);
-
-		NB.encodeInt32(buf, 0, PACK_VERSION_GENERATED);
-		out.write(buf, 0, 4);
-
-		NB.encodeInt32(buf, 0, getObjectsNumber());
-		out.write(buf, 0, 4);
+		System.arraycopy(Constants.PACK_SIGNATURE, 0, buf, 0, 4);
+		NB.encodeInt32(buf, 4, PACK_VERSION_GENERATED);
+		NB.encodeInt32(buf, 8, getObjectsNumber());
+		out.write(buf, 0, 12);
 	}
 
 	private void writeObjects() throws IOException {
-- 
1.6.2.1.471.g682837

[JGIT PATCH 5/5] Use Deflater directly in PackWriter

[Shawn O. Pearce]

Rather than going through all of the indirection that makes up the
DeflaterOutputStream, including the new object construction for it
and the temporary buffer it allocates internally, we can pump data
directly through our Deflater instance and use our existing 16 KB
temporary "buf" for the transient storage as we compress data to
the pack output stream.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/PackWriter.java       |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
index 2f34255..2d05c4e 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
@@ -47,7 +47,6 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.zip.Deflater;
-import java.util.zip.DeflaterOutputStream;
 
 import org.spearce.jgit.errors.IncorrectObjectTypeException;
 import org.spearce.jgit.errors.MissingObjectException;
@@ -699,12 +698,15 @@ private void writeWholeObject(final ObjectToPack otp) throws IOException {
 		} else {
 			final ObjectLoader loader = db.openObject(windowCursor, otp);
 			final byte[] data = loader.getCachedBytes();
-			final DeflaterOutputStream deflaterOut = new DeflaterOutputStream(
-					out, deflater);
 			writeObjectHeader(otp.getType(), data.length);
-			deflaterOut.write(data);
-			deflaterOut.finish();
 			deflater.reset();
+			deflater.setInput(data, 0, data.length);
+			deflater.finish();
+			do {
+				final int n = deflater.deflate(buf, 0, buf.length);
+				if (n > 0)
+					out.write(buf, 0, n);
+			} while (!deflater.finished());
 		}
 	}
 
-- 
1.6.2.1.471.g682837

Re: [JGIT PATCH 3/5] Test case for pack index CRC32 when written by PackWriter

[Daniel Cheng]

Shawn O. Pearce wrote:
> Suggested-by: Daniel Cheng (aka SDiZ) <j16sdiz+freenet@gmail.com>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  .../tst/org/spearce/jgit/lib/PackWriterTest.java   |   33 ++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
> 

Thanks.


-- 

Re: [JGIT PATCH 3/5] Test case for pack index CRC32 when written by PackWriter

[Daniel Cheng]

Shawn O. Pearce wrote:
> Suggested-by: Daniel Cheng (aka SDiZ) <j16sdiz+freenet@gmail.com>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  .../tst/org/spearce/jgit/lib/PackWriterTest.java   |   33 ++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
> 

Thanks.

Re: patch series starting with [JGIT PATCH 1/5] Remove dead/unused reset method from CountingOutputStream

[Robin Rosenberg]

Seems fine. While verifying this I got this error.

jgit push sftp://me@calhost/home/me/tmp/cb/.git master:master

Counting objects:       20464
Compressing objects:    100% (20464/20464)
Writing objects:        100% (20464/20464)
fatal: Can't write /home/me/tmp/cb/.git/objects/../HEAD: Failure
fatal: sftp://me@localhost/home/me/tmp/cb/.git: cannot create HEAD

It seems the remote repo is OK and actually has handsome HEAD despite this.

More related, I also noted that the git:// ssh or file: clone calculates the CRC's separately
and wasn't subject to this bug.

-- robin

Re: patch series starting with [JGIT PATCH 1/5] Remove dead/unused reset method from CountingOutputStream

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> 
> More related, I also noted that the git:// ssh or file: clone calculates the CRC's separately
> and wasn't subject to this bug.

clone (or fetch) is going through IndexPack, which has its own logic
to tear apart a pack file and build up the table of contents of
that pack in the most efficient method possible.  Its an entirely
different code base than writing a pack, because its going the
other direction.  :-)

The new test case at least validates that both IndexPack and
PackWriter produce the same information in the .idx file.

-- 
Shawn.

Re: patch series starting with [JGIT PATCH 1/5] Remove dead/unused reset method from CountingOutputStream

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> Seems fine. While verifying this I got this error.
> 
> jgit push sftp://me@calhost/home/me/tmp/cb/.git master:master
> 
> Counting objects:       20464
> Compressing objects:    100% (20464/20464)
> Writing objects:        100% (20464/20464)
> fatal: Can't write /home/me/tmp/cb/.git/objects/../HEAD: Failure
> fatal: sftp://me@localhost/home/me/tmp/cb/.git: cannot create HEAD

I can't reproduce this error with sftp://.

At first, it failed to push because my target directory didn't exist.

After I made the target directory, I also had to create below it
"refs", "objects", "objects/pack" in order to get the push to
succeed, and then it went through clean.
 
-- 
Shawn.

Re: patch series starting with [JGIT PATCH 1/5] Remove dead/unused reset method from CountingOutputStream

[Robin Rosenberg]

fredag 27 mars 2009 15:55:50 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> > Seems fine. While verifying this I got this error.
> > 
> > jgit push sftp://me@calhost/home/me/tmp/cb/.git master:master
> > 
> > Counting objects:       20464
> > Compressing objects:    100% (20464/20464)
> > Writing objects:        100% (20464/20464)
> > fatal: Can't write /home/me/tmp/cb/.git/objects/../HEAD: Failure
> > fatal: sftp://me@localhost/home/me/tmp/cb/.git: cannot create HEAD
> 
> I can't reproduce this error with sftp://.
> 
> At first, it failed to push because my target directory didn't exist.
Same here.

> After I made the target directory, I also had to create below it
> "refs", "objects", "objects/pack" in order to get the push to
> succeed, and then it went through clean.

hm, I git init'd the repo on the server, that's cheating I suppose.

-- robin