RFR 8229888: Zip FS does not retain permissions when updating a Zip file

[Lance Andersen]

Hi Brian 

Thank you for the review 

I can add an entry to the data provider as you suggest before I push the patch 

Best 
Lance 

--

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

Sent from my iPhone

> On Dec 17, 2019, at 8:12 PM, Brian Burkhalter <Brian.Burkhalter at oracle.com> wrote:
> 
> Hi Lance,
> 
> No further comments beyond what Christoph wrote aside from noticing that the GROUP_EXECUTE permission is not tested so perhaps you might want to add it somewhere in the DataProvider in the test.
> 
> Brian
> 
>> On Dec 17, 2019, at 12:24 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
>> 
>> I checked your patch. I think it is correct, I only have a few style nits which you may or may not address (no need for further webrev, though):
>>  
>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java:
>> Line 1793: Maybe the comment should rather be: // Set the POSIX permissions of the original Zip File if available
>> Line 1804: The path to the file (need not necessarily be a zip file)
>> Line 1806: I would indent the comment to the same column as “The” in the row above – makes it more readable. Also, there is a space too much between “the” and “POSIX”
>> Line 1808: Indention as above
>> Line 1811: Remove
>> Line 1816: Insert blank between if and (
>>  
>> test/jdk/jdk/nio/zipfs/ZipFSPermissionsTest.java:
>> Line 68: Remove
>> Line 85: Remove
>> Method teardown, line 97: I think this method is not needed. zipFile is already deleted before each test in method “before()” and in the end you clean up via “suiteCleanUp()”. So maybe tearDown can/should be removed?
>> Line 113: Maybe you should move “Zip file” to the line before while keeping this line as a blank line
>> Line 115: Same as with 113
>> Line 120: Space between if and (
>> Line 181: Same as in 120
>> Method getPosixAttributes, line 199: I think you should remove the try/catch UnsupportedOperationException. If the test is run on a non-posix fs, the whole test is skipped in setup() anyways.
>>  
>> Cheers
>> Christoph
>>  
>>  
>> From: nio-dev <nio-dev-bounces at openjdk.java.net> On Behalf Of Lance Andersen
>> Sent: Donnerstag, 12. Dezember 2019 17:48
>> Cc: nio-dev <nio-dev at openjdk.java.net>
>> Subject: Re: RFR 8229888: Zip FS does not retain permissions when updating a Zip file
>>  
>> Attached is the updated patch for addressing the permissions based on feedback from Alan:  http://cr.openjdk.java.net/~lancea/8229888/webrev.02/index.html
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20191217/58024ac6/attachment-0001.htm>