I want to update or create a comment to zip file.

For instance, I have test.zip file.
I'm using these statement to create a comment :

from zipfile import ZipFile

z=ZipFile('test.zip','a')
z.comment='Create a new comment'
z.close()

After to ran this script, the zip file test.zip doesn't including the new comment !

I can have the expected behavior when I add a new file inner zip archive.

I'm marking this as easy hoping someone in the mentoring group will pick it up and take a look.  The relevant code is in Python, and I'm guessing there is some logic bug when only the comment is set (and nothing is added to the zipfile), but I haven't looked at the code.  I'm also adding 3.2 and 3.3; it fails on 3.3 as well.

Tanks à lot. To complete Information about this bug.

it up and take a look.  The relevant code is in Python, and I'm guessing
there is some logic bug when only the comment is set (and nothing is added
to the zipfile), but I haven't looked at the code.  I'm also adding 3.2 and
3.3; it fails on 3.3 as well.
>
> ----------
> keywords: +easy
> nosy: +r.david.murray
> stage:  -> needs patch
> versions: +Python 3.2, Python 3.3
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue14399>
> _______________________________________
>

I'm unable to reproduce on either 2.7 or 3.3. Running the following:

from zipfile import ZipFile

z=ZipFile('test.zip','a')
z.comment='Create a new comment'
z.close()

produces the output:

Archive:  test.zip
Create a new comment

with the comment changed to b'...' for 3.3. Note that in 3.3 if you set the comment to a string and try to close, an exception will be raised and the test.zip file will not have a comment. 

Odd that I don't see the bug on either branch. Can someone else confirm?

Hmm.  When I did the same (changed to b'...') and ran it, I got an archive test.zip.  When I then opened that archive using ZipFile, its comment attribute was b''.  Thus I concluded that the bug exists in Python3.  I don't remember seeing any output from running the script, and indeed I would not expect to.  How did you run it?

Also, I used the uniz unzip command on the produced archive, and got a corrupted archive error message and a warning about an incorrect comment length.

The bug only occurs for non-empty archives.

There are two ways of fixing this bug -- very simple and no so simple. Patches are attached.

Thanks for the patch.

The advantage of the more complicated way being that you get an immediate TypeError instead of a delayed one?  Is there any other advantage? (That's probably enough reason, though :)

Now we just need a unit test for this.

This is the second advantage. The first one is that nothing is to be written when zipfile opened in a mode "a", if we don't change anything.

Got you.  We'll definitely go with the more complete fix, then.

Fixed patch with tests attached.

The tests don't seem to be included in the new patch.  Also, you could modernize the property definition (@property, @comment.setter).

My fault.

I see a comment around line 240 that says:

    # It is assumed that the "end of central directory" magic
    # number does not appear in the comment.

Any reason not to add that check to comment.setter?

Serhiy: this looks good.  I get some test errors when I apply it on 2.7 though.  Would you be interested in doing a 2.7 version as well?

(Minor comment: the test method would be better as two test methods, and it would be nice to have a third test method that checked for the TypeError for non-binary comments...that won't apply to 2.7, obviously).

Thank you, the TypeError test helped me find the error. Here is the
corrected patch.

For 2.7 it was necessary to turn the ZipFile in the new-style class.

Thanks.

We've had trouble in the past with a conversion to new style class breaking people's code.  People are less likely to be subclassing ZipFile, though, so it is probably OK.

New changeset ac0ec1f31b0a by R David Murray in branch '2.7':
#14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/ac0ec1f31b0a

New changeset 4186f20d9fa4 by R David Murray in branch '3.2':
#14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/4186f20d9fa4

New changeset e5b30d4b0647 by R David Murray in branch 'default':
Merge #14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/e5b30d4b0647

Thanks, Serhiy.

I made one small change, using 'with self.assertEqual' in the TypeError test.  You might want to check that out, it is a useful technique.

Oh, and I removed the type check from the 2.7 patch.  You can use a unicode string as long as it doesn't contain non-ASCII (the reason we created python3!), so it would be a backward incompatible change to raise a TypeError there.

I mean "with self.assertRaises(TypeError):".

> I mean "with self.assertRaises(TypeError):".

Thank you, I did not know that the context managers may catch exceptions
from the nested block.

> #14399: zipfile now correctly handles comments added to empty zipfiles.

This is a wrong description of the issue.  On the contrary, the error
occurred when adding or changing the comment in a *non-empty* zipfile
without adding or changing files.

Description in the Misc/NEWS also wrong.

Ah.  I based that on the fact that the third test passed without the change.  I thought you were adding that test of changing the comment just as a double check.  I should have asked instead of assuming.

New changeset b3b7f9dd7ce4 by R David Murray in branch '3.2':
#14399: corrected news item
http://hg.python.org/cpython/rev/b3b7f9dd7ce4

New changeset 225126c9d4b5 by R David Murray in branch '2.7':
#14399: corrected news item
http://hg.python.org/cpython/rev/225126c9d4b5

New changeset 160245735299 by R David Murray in branch 'default':
Merge #14399: corrected news item
http://hg.python.org/cpython/rev/160245735299

I must have been seeing what I expected to see.  The test that failed was the non-empty test.

News item fixed, thanks for the correction.

Just for the record, David's comment at msg158136 is apposite:

> We've had trouble in the past with a conversion to new style class
> breaking people's code.  People are less likely to be subclassing
> ZipFile, though, so it is probably OK.

We just spent some time this morning fixing a bug in some internal software; the cause was precisely this unexpected change from old-style class to new-style class in a bugfix release. And indeed, we were subclassing ZipFile. :-(

:( :(

OK, next time this comes up I won't say "it will probably be OK".