Re: [PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2
- From
- Marek Zawirski <marek.zawirski@gmail.com>
- Date
- Mar 25, 2009, 13:31 UTC
- Message-ID
- <49CA3218.9090202@gmail.com>
- In-Reply-To
- <1237962115-22709-1-git-send-email-j16sdiz+freenet@gmail.com>
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