[JGit] Mismatch CRC in packed objects from `jgit push`

[JGit] Mismatch CRC in packed objects from `jgit push`

[Daniel Cheng]

Hi list,

When I working with jgit-over-freenet, I found the pack files as
generated by `jgit push` give CRC error in `git fsck` and `git clone`,
but they are perfectly okay if I do a `git unpack-objects` manually.

You can download the pack file here:
 http://sdiz.net/temp/pack-fcedfaa7130866c884e208769661360563a3081f.idx
 http://sdiz.net/temp/pack-fcedfaa7130866c884e208769661360563a3081f.pack


Here is the diagnose session:

sdiz@sp2:/tmp$ git clone --verbose http://........
[...]
Getting index for pack fcedfaa7130866c884e208769661360563a3081f
Getting pack fcedfaa7130866c884e208769661360563a3081f
 which contains f197d4578a1b8ed195981d1e1ad4c390875c353a
error: index CRC mismatch for object
f197d4578a1b8ed195981d1e1ad4c390875c353a from
/tmp/egit-freenet/.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack
at offset 12
error: index CRC mismatch for object
0b2cb180fef969e0da259765564f9bf8bcd8cf25 from
/tmp/egit-freenet/.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack
at offset 183
error: index CRC mismatch for object
d88f5a430841925629c30199e666473d201bdf5a from
/tmp/egit-freenet/.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack
at offset 379
error: index CRC mismatch for object
40d3204c679fc5d25281331b981d968016030930 from
/tmp/egit-freenet/.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack
at offset 558
[...]
sdiz@sp2:/tmp$ git --version
git version 1.6.2


// Using the pack file directly seems okay, but fsck give the CRC error :

sdiz@sp2:/tmp/z$ git init
Initialized empty Git repository in /tmp/z/.git/
sdiz@sp2:/tmp/z$ cp ../pack-* .git/objects/pack/
sdiz@sp2:/tmp/z$ git checkout f197d4578a1b8ed195981d1e1ad4c390875c353a
warning: You appear to be on a branch yet to be born.
warning: Forcing checkout of f197d4578a1b8ed195981d1e1ad4c390875c353a.
Note: moving to "f197d4578a1b8ed195981d1e1ad4c390875c353a" which isn't
a local branch
If you want to create a new branch from this checkout, you may do so
(now or later) by using -b with the checkout command again. Example:
  git checkout -b <new_branch_name>
HEAD is now at f197d45... Instruction for cloning with git+fproxy
sdiz@sp2:/tmp/z$ ls
0x7494252.asc  activelink.png  freenet-bunny.svg  index.html  jgit
jgit.jar  jgit_src.zip
sdiz@sp2:/tmp/z$
sdiz@sp2:/tmp/z$ git fsck f197d4578a1b8ed195981d1e1ad4c390875c353a
error: index CRC mismatch for object
f197d4578a1b8ed195981d1e1ad4c390875c353a from
.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack
at offset 12
error: index CRC mismatch for object
0b2cb180fef969e0da259765564f9bf8bcd8cf25 from
.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack
at offset 183
error: index CRC mismatch for object
d88f5a430841925629c30199e666473d201bdf5a from
.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack
at offset 379
[...]

// unpack the objects manually seems to fix the issue
sdiz@sp2:/tmp/z$ rm -f .git/objects/*/*
sdiz@sp2:/tmp/z$ git unpack-objects --strict -r <
../pack-fcedfaa7130866c884e208769661360563a3081f.pack
Unpacking objects: 100% (26/26), done.
sdiz@sp2:/tmp/z$ git fsck --full f197d4578a1b8ed195981d1e1a
sdiz@sp2:/tmp/z$

// diff'ing the checkout with the original repository is also okay
sdiz@sp2:/tmp/z$ diff -Nau ~/build/egit/ .
Common subdirectories: /home/sdiz/build/egit/.git and ./.git
sdiz@sp2:/tmp/z$

// Fsck'ing in original repository is perfectly okay:

sdiz@sp2:~/build/egit$ git fsck --full f197d4
dangling commit 9900ac0df33e046c2f3f77ad8e084d535d3c023d
dangling commit 2e12ce6571923190124e86cc6b877ccb3ace9219
dangling commit f8150b71a352176f672270ffced6958682b215f3
dangling commit 101ae6bff9ca647c7c8297556314757162fbc2f2
[..]

[JGIT Test Case] This (incomplete) test case demo the index wrong CRC bug.

[Daniel Cheng (aka SDiZ)]

The PackIndex always give 0 for CRC.
I know this patch is ugly, but I am not familiar with the code to make a good test code.

Signed-off-by: Daniel Cheng (aka SDiZ) <j16sdiz+freenet@gmail.com>
---
 .../tst/org/spearce/jgit/lib/PackWriterTest.java   |   14 ++++++++++++++
 1 files changed, 14 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 f7139fc..0279c6b 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;
@@ -354,6 +355,19 @@ public void testWritePack4SizeThinVsNoThin() throws Exception {
 		assertTrue(sizePack4 > sizePack4Thin);
 	}
 
+	public void testWriteIndex() throws Exception {
+		testWritePack4();
+
+		File idxFile = File.createTempFile("temp", ".idx");
+		FileOutputStream ios = new FileOutputStream(idxFile);
+		writer.writeIndex(ios);
+		ios.close();
+
+		PackIndex idx = PackIndex.open(idxFile);
+		assertFalse(0 == idx.findCRC32(ObjectId
+				.fromString("82c6b885ff600be425b4ea96dee75dca255b69e7")));
+	}
+
 	// TODO: testWritePackDeltasCycle()
 	// TODO: testWritePackDeltasDepth()
 
-- 
1.6.2

[PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2

[Daniel Cheng (aka SDiZ)]

Signed-off-by: Daniel Cheng (aka SDiZ) <j16sdiz+freenet@gmail.com>
---
 .../src/org/spearce/jgit/lib/PackWriter.java       |    2 +
 .../spearce/jgit/util/CountingOutputStream.java    |   32 +++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 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 601ce71..d8b50e6 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
@@ -687,11 +687,13 @@ public class PackWriter {
 
 		assert !otp.isWritten();
 
+		countingOut.resetCRC32();
 		otp.setOffset(countingOut.getCount());
 		if (otp.isDeltaRepresentation())
 			writeDeltaObject(otp);
 		else
 			writeWholeObject(otp);
+		otp.setCRC((int) countingOut.getCRC32());
 
 		writeMonitor.update(1);
 	}
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..b4ae915 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
@@ -40,12 +40,16 @@ package org.spearce.jgit.util;
 import java.io.FilterOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.util.zip.CRC32;
 
 /**
- * Counting output stream decoration. Counts bytes written to stream.
+ * Counting output stream decoration. Counts bytes written to stream and 
+ * calculate CRC32 checksum.
  */
 public class CountingOutputStream extends FilterOutputStream {
 	private long count;
+	
+	private CRC32 crc;
 
 	/**
 	 * Create counting stream being decorated to provided real output stream.
@@ -55,6 +59,7 @@ public class CountingOutputStream extends FilterOutputStream {
 	 */
 	public CountingOutputStream(OutputStream out) {
 		super(out);
+		crc = new CRC32();
 	}
 
 	@Override
@@ -79,10 +84,35 @@ public class CountingOutputStream extends FilterOutputStream {
 		return count;
 	}
 
+    /**
+     * Resets CRC-32 to initial value.
+     */
+	public void resetCRC32() {
+		crc.reset();
+	}
+
+    /**
+     * Returns CRC-32 value.
+     * @return CRC32
+     */
+	public long getCRC32() {
+		return crc.getValue();
+	}
+
+
 	/**
 	 * Reset counter to zero value.
 	 */
 	public void reset() {
 		count = 0;
+		crc.reset();
+	}
+	
+	/**
+	 * {@inheritDoc}
+	 */
+	public void close() throws IOException {
+		crc = null;
+		super.close();
 	}
 }
-- 
1.6.2

[PATCH JGIT 2/2] Test case for pack index CRC

[Daniel Cheng (aka SDiZ)]

Signed-off-by: Daniel Cheng (aka SDiZ) <j16sdiz+freenet@gmail.com>
---
 .../tst/org/spearce/jgit/lib/PackWriterTest.java   |   10 ++++++++++
 1 files changed, 10 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 f7139fc..9a5513f 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
@@ -354,6 +354,15 @@ public class PackWriterTest extends RepositoryTestCase {
 		assertTrue(sizePack4 > sizePack4Thin);
 	}
 
+	public void testWriteIndex() throws Exception {
+		writer.setIndexVersion(2);
+		writeVerifyPack4(true);
+		
+		PackIndex idx = PackIndex.open(indexFile);
+		assertEquals(0x4743F1E4L, idx.findCRC32(ObjectId
+				.fromString("82c6b885ff600be425b4ea96dee75dca255b69e7")));
+	}
+
 	// TODO: testWritePackDeltasCycle()
 	// TODO: testWritePackDeltasDepth()
 
@@ -470,6 +479,7 @@ public class PackWriterTest extends RepositoryTestCase {
 		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

Re: [PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2

[Marek Zawirski]

Hi,

Thanks for spotting this bug.

Daniel Cheng (aka SDiZ) wrote:

(...)

> 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..d8b50e6 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java
> @@ -687,11 +687,13 @@ public class PackWriter {
>  
>  		assert !otp.isWritten();
>  
> +		countingOut.resetCRC32();
>  		otp.setOffset(countingOut.getCount());
>  		if (otp.isDeltaRepresentation())
>  			writeDeltaObject(otp);
>  		else
>  			writeWholeObject(otp);
> +		otp.setCRC((int) countingOut.getCRC32());
>
>   
Huh, now it appears that you made CRC32 really computed;)
I just wonder if it is sensible to compute it always regardless of used 
index version (outputVersion) - for index v1 we don't really need CRC32 
to be computed. I don't have a good idea how can it be avoided in truly 
elegant way, as we cannot rely on the outputVersion checking in this 
code - currently it may became changed after writing pack, but before 
writing index.  But maybe it's not so important issue, as AFAIR v2 is 
already default version for index.

> }
> 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..b4ae915 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java
> @@ -40,12 +40,16 @@ package org.spearce.jgit.util;
>  import java.io.FilterOutputStream;
>  import java.io.IOException;
>  import java.io.OutputStream;
> +import java.util.zip.CRC32;
>  
>  /**
> - * Counting output stream decoration. Counts bytes written to stream.
> + * Counting output stream decoration. Counts bytes written to stream and 
> + * calculate CRC32 checksum.
>   
IMO it would be better to make CRC32 computation in another decorator 
class, but that's just me.

>   */
>  public class CountingOutputStream extends FilterOutputStream {
>  	private long count;
> +	
> +	private CRC32 crc;
>  
>  	/**
>  	 * Create counting stream being decorated to provided real output stream.
> @@ -55,6 +59,7 @@ public class CountingOutputStream extends FilterOutputStream {
>  	 */
>  	public CountingOutputStream(OutputStream out) {
>  		super(out);
> +		crc = new CRC32();
>  	}
>  
>  	@Override
> @@ -79,10 +84,35 @@ public class CountingOutputStream extends FilterOutputStream {
>  		return count;
>  	}
>  
> +    /**
> +     * Resets CRC-32 to initial value.
> +     */
> +	public void resetCRC32() {
> +		crc.reset();
> +	}
> +
> +    /**
> +     * Returns CRC-32 value.
> +     * @return CRC32
> +     */
> +	public long getCRC32() {
> +		return crc.getValue();
> +	}
> +
> +
>  	/**
>  	 * Reset counter to zero value.
>  	 */
>  	public void reset() {
>  		count = 0;
> +		crc.reset();
> +	}
> +	
> +	/**
> +	 * {@inheritDoc}
> +	 */
> +	public void close() throws IOException {
> +		crc = null;
> +		super.close();
>  	}
>  }
>   
Have you tested that code? It seems that CRC32 updates is  missing in 
write() method... or did I slept too short this night?:)

Best,
Marek

Re: [PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2

[Shawn O. Pearce]

Marek Zawirski <marek.zawirski@gmail.com> wrote:
> I just wonder if it is sensible to compute it always regardless of used  
> index version (outputVersion) - for index v1 we don't really need CRC32  
> to be computed. I don't have a good idea how can it be avoided in truly  
> elegant way, as we cannot rely on the outputVersion checking in this  
> code - currently it may became changed after writing pack, but before  
> writing index.  But maybe it's not so important issue, as AFAIR v2 is  
> already default version for index.

If the index version is specifically set to 1, we may be forced to
write a version 2 index if the pack file is huge, in which case we
need the CRC32 data on each object.  Since version 2 is the default,
we probably hav to compute it no matter what.

> Have you tested that code? It seems that CRC32 updates is  missing in  
> write() method... or did I slept too short this night?:)

Yea, its missing the updates in the write method.

I'm writing up an alternate series of patches.

-- 
Shawn.

Re: [PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2

[Daniel Cheng]

On Wed, Mar 25, 2009 at 9:31 PM, Marek Zawirski
<marek.zawirski@gmail.com> wrote:
> Hi,
>
> Thanks for spotting this bug.
> (...)
[...]
>
> Have you tested that code? It seems that CRC32 updates is  missing in
> write() method... or did I slept too short this night?:)

Sure I have tested some code, the wrong one.

The buggy code is at PackWriter.writeIndex(), and
the test case I have tested is using IndexPack.index().

.. hm...

I am thinking if we can combine two of them.

> Best,
> Marek
>