From: Marek Zawirski <marek.zawirski@gmail.com>
Date: Wed, 25 Mar 2009 13:31:04 GMT
Subject: Re: [PATCH JGIT 1/2] Calculate CRC32 on Pack Index v2
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:

(...)

> 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