threads / discuss / 18500

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

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

## tl;dr

7 messages between Mar 24, 2009 and Mar 26, 2009.

replies: 6people: 3as markdown or json

Daniel Cheng· Mar 24, 2009, 02:53 UTC · lore
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 [..]

Daniel Cheng (aka SDiZ)· Mar 24, 2009, 11:13 UTC · re: Daniel Cheng · lore

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

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
Daniel Cheng (aka SDiZ)· Mar 25, 2009, 06:21 UTC · re: Daniel Cheng · lore

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

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
Daniel Cheng (aka SDiZ)· Mar 25, 2009, 06:21 UTC · re: Daniel Cheng (aka SDiZ) · lore

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

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
Marek Zawirski· Mar 25, 2009, 13:31 UTC · re: Daniel Cheng (aka SDiZ) · lore

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

Hi,
Thanks for spotting this bug.
Daniel Cheng (aka SDiZ) wrote:
(...)
Show 17 quoted lines
> 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.

Show 16 quoted lines
> }
> 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.

Show 53 quoted lines
>   */
>  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

Shawn O. Pearce· Mar 25, 2009, 21:59 UTC · re: Marek Zawirski · lore

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

Marek Zawirski <marek.zawirski@gmail.com> wrote:
Show 7 quoted lines
> 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.
Daniel Cheng· Mar 26, 2009, 00:49 UTC · re: Marek Zawirski · lore

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

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
>

← back to recent threads