Re: [cp-patches] [Patch] ZipFile

[Mark Wielaard]

Hi,

On Thu, 2005-01-27 at 16:54 +0100, Michael Koch wrote:
> Originally this was meant for Mark and and not for the list. I should 
> shoot me into the head for making so much mistakes.

No, it is better on the list since as you can see there are much more
people reviewing your patch in that case. And they are in general faster
and smarter then me!

> I have attached a new version of the patch with Jeroen's issue fixed.
> 
> Everybody okay with it ?

No ChangeLog entry? 
Having a ChangeLog entry really helps when reviewing a patch.

> +  private void checkZipFile() throws IOException, ZipException
> +  {
> +    byte[] magic = new byte[2];
> +    long position = raf.getFilePointer();
> +    raf.read(magic);
> +    raf.seek(position);
> +
> +    if (((((int) magic[1]) << 8) + ((int) magic[0])) != ZIP_MAGIC)
> +      {
> +       raf.close();
> +       throw new ZipException("Not a valid zip file");
> +      }
> +  }

It is probably clearer to use the readLeInt(DataInput di, byte[] b)
method. I don't think you need to record and rewind the file pointer
here.

>    /**
>     * Returns an enumeration of all Zip entries in this Zip file.
> +   *
> +   * @exception IllegalStateException when the ZipFile has already been 
> closed
>     */
>    public Enumeration entries()
>    {
> +    checkClosed();
> +    
>      try
>        {
>         return new ZipEntryEnumeration(getEntries().values().iterator());
>        }
>      catch (IOException ioe)
>        {
> -       return null;
> +       return EmptyEnumeration.getInstance();
>        }
>    }
> 

Good catch.

> > Looks OK to me. Is there a Mauve test for these cases?
> 
> Not yet. I will write some before commiting.

Thanks!

Mark

signature.asc
Description: This is a digitally signed message part