Resend RFR 8231766: Files.copy/move do not honor requested compression method when copying/moving within the same zip file

[Langer, Christoph]

Hi Lance,

thanks for doing the updates. Looks fine, except that you should have a look at the comment in line 389/390 of CopyMoveTests.java again – this seems to be a little bit edgy still �� But no need for updated webrev…

Best regards
Christoph

From: Lance Andersen <lance.andersen at oracle.com>
Sent: Freitag, 25. Oktober 2019 19:49
To: Langer, Christoph <christoph.langer at sap.com>
Cc: Brian Burkhalter <brian.burkhalter at oracle.com>; nio-dev <nio-dev at openjdk.java.net>
Subject: Re: Resend RFR 8231766: Files.copy/move do not honor requested compression method when copying/moving within the same zip file

Hi Christoph,

Thank you for the review, please see below.

On Oct 25, 2019, at 4:00 AM, Langer, Christoph <christoph.langer at sap.com<mailto:christoph.langer at sap.com>> wrote:

Hi Lance,

I’ve had a look as well now. Thanks for doing this fix and providing extensive testing ��
:-)


The change in ZipFileSystem looks correct.

thank you

I have a few remarks for the test(s):

UpdateEntryTest:

testReplaceStoredEntry

maybe add some comment that we expect the updated entry to have the default compression method of the zip file system (which is DEFLATED for this test case)
done


Maybe you can fix some minor thing that was already in the test, while touching it:
line 121: We don't use rc, so either remove it or add @SuppressWarnings("unused") if you think it's important to have for debugging purposes. I'd rather vote for removing :)

OK, done.  I added this  originally  because Intellij complained via the inspect option and  then complained after the change that it was an unused variable.  so I  just removed it per your request.


lines 163ff: Maybe you can initialize the properties map in one line: Map.of("noCompression", e1.method != ZipEntry.DEFLATED) and use this in place in line 177
The only drawback is that you wouldn't have a check whether e1.method is in (ZipEntry.DEFLATED, ZipEntry.STORED) but is that needed? 10 fewer lines of code to read/understand ;-)

OK done in a similar way to your suggestion.



CopyMoveTests

line 110: use "expectedCompression == ZipEntry.STORED" instead of targetCompression (remove line 102). I think that makes the code more readable
This also applies to line 144, 181, 247 and 280/282

OK,  while I think this is a personal style choice, I made the suggested change.


line 329: grammar: Validate that a FileAlreadyExistsException is thrown
done


line 338: formatMap printout: I guess this is for debugging and it does only exist in this one place although something like that could be as useful everywhere. I suggest to either remove it here or insert it in each test method, maybe hidden behind some debug switch

Added a DEBUG flag and  also updated the System.out.printf in the createZip method


line 387: comment should be:
Create a Zip file containing the given entries, using Zip File System and the specified properties map

Updated the comment

The updated webrev can be found at: http://cr.openjdk.java.net/~lancea/8231766/webrev.02/index.html

Currently re-running mach5 jdk-tier1 through jdk-tier3

Best
Lance



Thanks
Christoph

From: nio-dev <nio-dev-bounces at openjdk.java.net<mailto:nio-dev-bounces at openjdk.java.net>> On Behalf Of Brian Burkhalter
Sent: Donnerstag, 24. Oktober 2019 19:50
To: Lance Andersen <lance.andersen at oracle.com<mailto:lance.andersen at oracle.com>>
Cc: nio-dev <nio-dev at openjdk.java.net<mailto:nio-dev at openjdk.java.net>>
Subject: Re: Resend RFR 8231766: Files.copy/move do not honor requested compression method when copying/moving within the same zip file

Hi Lance,

OK that all sounds good. No need to update the webrev.

Thanks,

Brian



On Oct 24, 2019, at 5:21 AM, Lance Andersen <lance.andersen at oracle.com<mailto:lance.andersen at oracle.com>> wrote:

Thank you for the review.
On Oct 23, 2019, at 8:03 PM, Brian Burkhalter <brian.burkhalter at oracle.com<mailto:brian.burkhalter at oracle.com>> wrote:

Hi Lance,

Please see comments on the test below listed by line numbers.

[cid:image001.gif at 01D58D79.70CB2B60]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com<mailto:Lance.Andersen at oracle.com>




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20191028/de3afa3b/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 658 bytes
Desc: image001.gif
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20191028/de3afa3b/image001-0001.gif>