{"thread":{"id":"18500","subject":"[JGit] Mismatch CRC in packed objects from `jgit push`","startedAt":"2009-03-24T02:53:40Z","lastAt":"2009-03-26T00:49:18Z","messageCount":7,"participants":["Daniel Cheng","Daniel Cheng (aka SDiZ)","Marek Zawirski","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"109151","messageId":"ff6a9c820903231953q29a5ccbk8e5b54c9afdb8abd@mail.gmail.com","threadId":"18500","inReplyTo":null,"subject":"[JGit] Mismatch CRC in packed objects from `jgit push`","fromName":"Daniel Cheng","fromEmail":"j16sdiz+freenet@gmail.com","sentAt":"2009-03-24T02:53:40Z","receivedAt":"2009-03-24T02:53:40Z","isPatch":false,"sender":{"key":"j16sdiz+freenet@gmail.com","avatar":"https://gravatar.com/avatar/3e796e8a156ee86e305bfc1fbe01608302554ee3b5fa7eb8d1213b8877bfcabd?d=mp&s=160"},"body":"Hi list,\n\nWhen I working with jgit-over-freenet, I found the pack files as\ngenerated by `jgit push` give CRC error in `git fsck` and `git clone`,\nbut they are perfectly okay if I do a `git unpack-objects` manually.\n\nYou can download the pack file here:\n http://sdiz.net/temp/pack-fcedfaa7130866c884e208769661360563a3081f.idx\n http://sdiz.net/temp/pack-fcedfaa7130866c884e208769661360563a3081f.pack\n\n\nHere is the diagnose session:\n\nsdiz@sp2:/tmp$ git clone --verbose http://........\n[...]\nGetting index for pack fcedfaa7130866c884e208769661360563a3081f\nGetting pack fcedfaa7130866c884e208769661360563a3081f\n which contains f197d4578a1b8ed195981d1e1ad4c390875c353a\nerror: index CRC mismatch for object\nf197d4578a1b8ed195981d1e1ad4c390875c353a from\n/tmp/egit-freenet/.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack\nat offset 12\nerror: index CRC mismatch for object\n0b2cb180fef969e0da259765564f9bf8bcd8cf25 from\n/tmp/egit-freenet/.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack\nat offset 183\nerror: index CRC mismatch for object\nd88f5a430841925629c30199e666473d201bdf5a from\n/tmp/egit-freenet/.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack\nat offset 379\nerror: index CRC mismatch for object\n40d3204c679fc5d25281331b981d968016030930 from\n/tmp/egit-freenet/.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack\nat offset 558\n[...]\nsdiz@sp2:/tmp$ git --version\ngit version 1.6.2\n\n\n// Using the pack file directly seems okay, but fsck give the CRC error :\n\nsdiz@sp2:/tmp/z$ git init\nInitialized empty Git repository in /tmp/z/.git/\nsdiz@sp2:/tmp/z$ cp ../pack-* .git/objects/pack/\nsdiz@sp2:/tmp/z$ git checkout f197d4578a1b8ed195981d1e1ad4c390875c353a\nwarning: You appear to be on a branch yet to be born.\nwarning: Forcing checkout of f197d4578a1b8ed195981d1e1ad4c390875c353a.\nNote: moving to \"f197d4578a1b8ed195981d1e1ad4c390875c353a\" which isn't\na local branch\nIf you want to create a new branch from this checkout, you may do so\n(now or later) by using -b with the checkout command again. Example:\n git checkout -b \nHEAD is now at f197d45... Instruction for cloning with git+fproxy\nsdiz@sp2:/tmp/z$ ls\n0x7494252.asc activelink.png freenet-bunny.svg index.html jgit\njgit.jar jgit_src.zip\nsdiz@sp2:/tmp/z$\nsdiz@sp2:/tmp/z$ git fsck f197d4578a1b8ed195981d1e1ad4c390875c353a\nerror: index CRC mismatch for object\nf197d4578a1b8ed195981d1e1ad4c390875c353a from\n.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack\nat offset 12\nerror: index CRC mismatch for object\n0b2cb180fef969e0da259765564f9bf8bcd8cf25 from\n.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack\nat offset 183\nerror: index CRC mismatch for object\nd88f5a430841925629c30199e666473d201bdf5a from\n.git/objects/pack/pack-fcedfaa7130866c884e208769661360563a3081f.pack\nat offset 379\n[...]\n\n// unpack the objects manually seems to fix the issue\nsdiz@sp2:/tmp/z$ rm -f .git/objects/*/*\nsdiz@sp2:/tmp/z$ git unpack-objects --strict -r <\n../pack-fcedfaa7130866c884e208769661360563a3081f.pack\nUnpacking objects: 100% (26/26), done.\nsdiz@sp2:/tmp/z$ git fsck --full f197d4578a1b8ed195981d1e1a\nsdiz@sp2:/tmp/z$\n\n// diff'ing the checkout with the original repository is also okay\nsdiz@sp2:/tmp/z$ diff -Nau ~/build/egit/ .\nCommon subdirectories: /home/sdiz/build/egit/.git and ./.git\nsdiz@sp2:/tmp/z$\n\n// Fsck'ing in original repository is perfectly okay:\n\nsdiz@sp2:~/build/egit$ git fsck --full f197d4\ndangling commit 9900ac0df33e046c2f3f77ad8e084d535d3c023d\ndangling commit 2e12ce6571923190124e86cc6b877ccb3ace9219\ndangling commit f8150b71a352176f672270ffced6958682b215f3\ndangling commit 101ae6bff9ca647c7c8297556314757162fbc2f2\n[..]\n"},{"id":"109202","messageId":"1237893210-23073-1-git-send-email-j16sdiz+freenet@gmail.com","threadId":"18500","inReplyTo":"ff6a9c820903231953q29a5ccbk8e5b54c9afdb8abd@mail.gmail.com","subject":"[JGIT Test Case] This (incomplete) test case demo the index wrong CRC bug.","fromName":"Daniel Cheng (aka SDiZ)","fromEmail":"j16sdiz+freenet@gmail.com","sentAt":"2009-03-24T11:13:30Z","receivedAt":"2009-03-24T11:13:30Z","isPatch":false,"sender":{"key":"j16sdiz+freenet@gmail.com","avatar":"https://gravatar.com/avatar/3e796e8a156ee86e305bfc1fbe01608302554ee3b5fa7eb8d1213b8877bfcabd?d=mp&s=160"},"body":"The PackIndex always give 0 for CRC.\nI know this patch is ugly, but I am not familiar with the code to make a good test code.\n\nSigned-off-by: Daniel Cheng (aka SDiZ) \n---\n .../tst/org/spearce/jgit/lib/PackWriterTest.java | 14 ++++++++++++++\n 1 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --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\nindex f7139fc..0279c6b 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java\n@@ -40,6 +40,7 @@\n import java.io.ByteArrayInputStream;\n import java.io.ByteArrayOutputStream;\n import java.io.File;\n+import java.io.FileOutputStream;\n import java.io.IOException;\n import java.io.InputStream;\n import java.util.ArrayList;\n@@ -354,6 +355,19 @@ public void testWritePack4SizeThinVsNoThin() throws Exception {\n \t\tassertTrue(sizePack4 > sizePack4Thin);\n \t}\n \n+\tpublic void testWriteIndex() throws Exception {\n+\t\ttestWritePack4();\n+\n+\t\tFile idxFile = File.createTempFile(\"temp\", \".idx\");\n+\t\tFileOutputStream ios = new FileOutputStream(idxFile);\n+\t\twriter.writeIndex(ios);\n+\t\tios.close();\n+\n+\t\tPackIndex idx = PackIndex.open(idxFile);\n+\t\tassertFalse(0 == idx.findCRC32(ObjectId\n+\t\t\t\t.fromString(\"82c6b885ff600be425b4ea96dee75dca255b69e7\")));\n+\t}\n+\n \t// TODO: testWritePackDeltasCycle()\n \t// TODO: testWritePackDeltasDepth()\n \n-- \n1.6.2\n"},{"id":"109319","messageId":"1237962115-22709-1-git-send-email-j16sdiz+freenet@gmail.com","threadId":"18500","inReplyTo":"ff6a9c820903231953q29a5ccbk8e5b54c9afdb8abd@mail.gmail.com","subject":"[PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2","fromName":"Daniel Cheng (aka SDiZ)","fromEmail":"j16sdiz+freenet@gmail.com","sentAt":"2009-03-25T06:21:54Z","receivedAt":"2009-03-25T06:21:54Z","isPatch":true,"sender":{"key":"j16sdiz+freenet@gmail.com","avatar":"https://gravatar.com/avatar/3e796e8a156ee86e305bfc1fbe01608302554ee3b5fa7eb8d1213b8877bfcabd?d=mp&s=160"},"body":"\nSigned-off-by: Daniel Cheng (aka SDiZ) \n---\n .../src/org/spearce/jgit/lib/PackWriter.java | 2 +\n .../spearce/jgit/util/CountingOutputStream.java | 32 +++++++++++++++++++-\n 2 files changed, 33 insertions(+), 1 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java\nindex 601ce71..d8b50e6 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java\n@@ -687,11 +687,13 @@ public class PackWriter {\n \n \t\tassert !otp.isWritten();\n \n+\t\tcountingOut.resetCRC32();\n \t\totp.setOffset(countingOut.getCount());\n \t\tif (otp.isDeltaRepresentation())\n \t\t\twriteDeltaObject(otp);\n \t\telse\n \t\t\twriteWholeObject(otp);\n+\t\totp.setCRC((int) countingOut.getCRC32());\n \n \t\twriteMonitor.update(1);\n \t}\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java b/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java\nindex b0b5f7d..b4ae915 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java\n@@ -40,12 +40,16 @@ package org.spearce.jgit.util;\n import java.io.FilterOutputStream;\n import java.io.IOException;\n import java.io.OutputStream;\n+import java.util.zip.CRC32;\n \n /**\n- * Counting output stream decoration. Counts bytes written to stream.\n+ * Counting output stream decoration. Counts bytes written to stream and \n+ * calculate CRC32 checksum.\n */\n public class CountingOutputStream extends FilterOutputStream {\n \tprivate long count;\n+\t\n+\tprivate CRC32 crc;\n \n \t/**\n \t * Create counting stream being decorated to provided real output stream.\n@@ -55,6 +59,7 @@ public class CountingOutputStream extends FilterOutputStream {\n \t */\n \tpublic CountingOutputStream(OutputStream out) {\n \t\tsuper(out);\n+\t\tcrc = new CRC32();\n \t}\n \n \t@Override\n@@ -79,10 +84,35 @@ public class CountingOutputStream extends FilterOutputStream {\n \t\treturn count;\n \t}\n \n+ /**\n+ * Resets CRC-32 to initial value.\n+ */\n+\tpublic void resetCRC32() {\n+\t\tcrc.reset();\n+\t}\n+\n+ /**\n+ * Returns CRC-32 value.\n+ * @return CRC32\n+ */\n+\tpublic long getCRC32() {\n+\t\treturn crc.getValue();\n+\t}\n+\n+\n \t/**\n \t * Reset counter to zero value.\n \t */\n \tpublic void reset() {\n \t\tcount = 0;\n+\t\tcrc.reset();\n+\t}\n+\t\n+\t/**\n+\t * {@inheritDoc}\n+\t */\n+\tpublic void close() throws IOException {\n+\t\tcrc = null;\n+\t\tsuper.close();\n \t}\n }\n-- \n1.6.2\n"},{"id":"109320","messageId":"1237962115-22709-2-git-send-email-j16sdiz+freenet@gmail.com","threadId":"18500","inReplyTo":"1237962115-22709-1-git-send-email-j16sdiz+freenet@gmail.com","subject":"[PATCH JGIT 2/2] Test case for pack index CRC","fromName":"Daniel Cheng (aka SDiZ)","fromEmail":"j16sdiz+freenet@gmail.com","sentAt":"2009-03-25T06:21:55Z","receivedAt":"2009-03-25T06:21:55Z","isPatch":true,"sender":{"key":"j16sdiz+freenet@gmail.com","avatar":"https://gravatar.com/avatar/3e796e8a156ee86e305bfc1fbe01608302554ee3b5fa7eb8d1213b8877bfcabd?d=mp&s=160"},"body":"\nSigned-off-by: Daniel Cheng (aka SDiZ) \n---\n .../tst/org/spearce/jgit/lib/PackWriterTest.java | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --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\nindex f7139fc..9a5513f 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java\n@@ -354,6 +354,15 @@ public class PackWriterTest extends RepositoryTestCase {\n \t\tassertTrue(sizePack4 > sizePack4Thin);\n \t}\n \n+\tpublic void testWriteIndex() throws Exception {\n+\t\twriter.setIndexVersion(2);\n+\t\twriteVerifyPack4(true);\n+\t\t\n+\t\tPackIndex idx = PackIndex.open(indexFile);\n+\t\tassertEquals(0x4743F1E4L, idx.findCRC32(ObjectId\n+\t\t\t\t.fromString(\"82c6b885ff600be425b4ea96dee75dca255b69e7\")));\n+\t}\n+\n \t// TODO: testWritePackDeltasCycle()\n \t// TODO: testWritePackDeltasDepth()\n \n@@ -470,6 +479,7 @@ public class PackWriterTest extends RepositoryTestCase {\n \t\tfinal IndexPack indexer = new IndexPack(db, is, packBase);\n \t\tindexer.setKeepEmpty(true);\n \t\tindexer.setFixThin(thin);\n+\t\tindexer.setIndexVersion(2);\n \t\tindexer.index(new TextProgressMonitor());\n \t\tpack = new PackFile(indexFile, packFile);\n \t}\n-- \n1.6.2\n"},{"id":"109354","messageId":"49CA3218.9090202@gmail.com","threadId":"18500","inReplyTo":"1237962115-22709-1-git-send-email-j16sdiz+freenet@gmail.com","subject":"Re: [PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2","fromName":"Marek Zawirski","fromEmail":"marek.zawirski@gmail.com","sentAt":"2009-03-25T13:31:04Z","receivedAt":"2009-03-25T13:31:04Z","isPatch":true,"sender":{"key":"marek.zawirski@gmail.com","avatar":null},"body":"Hi,\n\nThanks for spotting this bug.\n\nDaniel Cheng (aka SDiZ) wrote:\n\n(...)\n\n> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java\n> index 601ce71..d8b50e6 100644\n> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java\n> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackWriter.java\n> @@ -687,11 +687,13 @@ public class PackWriter {\n> \n> \t\tassert !otp.isWritten();\n> \n> +\t\tcountingOut.resetCRC32();\n> \t\totp.setOffset(countingOut.getCount());\n> \t\tif (otp.isDeltaRepresentation())\n> \t\t\twriteDeltaObject(otp);\n> \t\telse\n> \t\t\twriteWholeObject(otp);\n> +\t\totp.setCRC((int) countingOut.getCRC32());\n>\n> \nHuh, now it appears that you made CRC32 really computed;)\nI just wonder if it is sensible to compute it always regardless of used \nindex version (outputVersion) - for index v1 we don't really need CRC32 \nto be computed. I don't have a good idea how can it be avoided in truly \nelegant way, as we cannot rely on the outputVersion checking in this \ncode - currently it may became changed after writing pack, but before \nwriting index. But maybe it's not so important issue, as AFAIR v2 is \nalready default version for index.\n\n> }\n> diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java b/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java\n> index b0b5f7d..b4ae915 100644\n> --- a/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java\n> +++ b/org.spearce.jgit/src/org/spearce/jgit/util/CountingOutputStream.java\n> @@ -40,12 +40,16 @@ package org.spearce.jgit.util;\n> import java.io.FilterOutputStream;\n> import java.io.IOException;\n> import java.io.OutputStream;\n> +import java.util.zip.CRC32;\n> \n> /**\n> - * Counting output stream decoration. Counts bytes written to stream.\n> + * Counting output stream decoration. Counts bytes written to stream and \n> + * calculate CRC32 checksum.\n> \nIMO it would be better to make CRC32 computation in another decorator \nclass, but that's just me.\n\n> */\n> public class CountingOutputStream extends FilterOutputStream {\n> \tprivate long count;\n> +\t\n> +\tprivate CRC32 crc;\n> \n> \t/**\n> \t * Create counting stream being decorated to provided real output stream.\n> @@ -55,6 +59,7 @@ public class CountingOutputStream extends FilterOutputStream {\n> \t */\n> \tpublic CountingOutputStream(OutputStream out) {\n> \t\tsuper(out);\n> +\t\tcrc = new CRC32();\n> \t}\n> \n> \t@Override\n> @@ -79,10 +84,35 @@ public class CountingOutputStream extends FilterOutputStream {\n> \t\treturn count;\n> \t}\n> \n> + /**\n> + * Resets CRC-32 to initial value.\n> + */\n> +\tpublic void resetCRC32() {\n> +\t\tcrc.reset();\n> +\t}\n> +\n> + /**\n> + * Returns CRC-32 value.\n> + * @return CRC32\n> + */\n> +\tpublic long getCRC32() {\n> +\t\treturn crc.getValue();\n> +\t}\n> +\n> +\n> \t/**\n> \t * Reset counter to zero value.\n> \t */\n> \tpublic void reset() {\n> \t\tcount = 0;\n> +\t\tcrc.reset();\n> +\t}\n> +\t\n> +\t/**\n> +\t * {@inheritDoc}\n> +\t */\n> +\tpublic void close() throws IOException {\n> +\t\tcrc = null;\n> +\t\tsuper.close();\n> \t}\n> }\n> \nHave you tested that code? It seems that CRC32 updates is missing in \nwrite() method... or did I slept too short this night?:)\n\nBest,\nMarek\n"},{"id":"109435","messageId":"20090325215931.GC23521@spearce.org","threadId":"18500","inReplyTo":"49CA3218.9090202@gmail.com","subject":"Re: [PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-25T21:59:31Z","receivedAt":"2009-03-25T21:59:31Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Marek Zawirski wrote:\n> I just wonder if it is sensible to compute it always regardless of used \n> index version (outputVersion) - for index v1 we don't really need CRC32 \n> to be computed. I don't have a good idea how can it be avoided in truly \n> elegant way, as we cannot rely on the outputVersion checking in this \n> code - currently it may became changed after writing pack, but before \n> writing index. But maybe it's not so important issue, as AFAIR v2 is \n> already default version for index.\n\nIf the index version is specifically set to 1, we may be forced to\nwrite a version 2 index if the pack file is huge, in which case we\nneed the CRC32 data on each object. Since version 2 is the default,\nwe probably hav to compute it no matter what.\n\n> Have you tested that code? It seems that CRC32 updates is missing in \n> write() method... or did I slept too short this night?:)\n\nYea, its missing the updates in the write method.\n\nI'm writing up an alternate series of patches.\n\n-- \nShawn.\n"},{"id":"109449","messageId":"ff6a9c820903251749q41977ee0wf776a43dc8e420fb@mail.gmail.com","threadId":"18500","inReplyTo":"49CA3218.9090202@gmail.com","subject":"Re: [PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2","fromName":"Daniel Cheng","fromEmail":"j16sdiz+freenet@gmail.com","sentAt":"2009-03-26T00:49:18Z","receivedAt":"2009-03-26T00:49:18Z","isPatch":true,"sender":{"key":"j16sdiz+freenet@gmail.com","avatar":"https://gravatar.com/avatar/3e796e8a156ee86e305bfc1fbe01608302554ee3b5fa7eb8d1213b8877bfcabd?d=mp&s=160"},"body":"On Wed, Mar 25, 2009 at 9:31 PM, Marek Zawirski\n wrote:\n> Hi,\n>\n> Thanks for spotting this bug.\n> (...)\n[...]\n>\n> Have you tested that code? It seems that CRC32 updates is  missing in\n> write() method... or did I slept too short this night?:)\n\nSure I have tested some code, the wrong one.\n\nThe buggy code is at PackWriter.writeIndex(), and\nthe test case I have tested is using IndexPack.index().\n\n.. hm...\n\nI am thinking if we can combine two of them.\n\n> Best,\n> Marek\n>\n"}]}