<div dir="ltr"><div dir="ltr"><div>Hi Christoph,</div><div><br></div><div>thanks for updating the change. I think it is in a good state now and ready to go!</div><div><br></div><div>Also the documentation in the CSR for this issue (<a href="https://bugs.openjdk.java.net/browse/JDK-8213082">https://bugs.openjdk.java.net/browse/JDK-8213082</a>) is greatly appreciated and answers all the questions which have been raised so far. So if there are still any open questions I'd recommend that any potential reviewer consults the CSR at <a href="https://bugs.openjdk.java.net/browse/JDK-8213082">https://bugs.openjdk.java.net/browse/JDK-8213082</a> first.</div><div><br></div><div>Thank you and best regards,</div><div>Volker</div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Dec 21, 2018 at 3:31 PM Langer, Christoph <<a href="mailto:christoph.langer@sap.com">christoph.langer@sap.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi all,<br>
<br>
here comes the updated webrev: <a href="http://cr.openjdk.java.net/~clanger/webrevs/8213031.3/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~clanger/webrevs/8213031.3/</a><br>
<br>
I've rebased the change to the current state of the JDK depot. Thanks to Volker, the test has been enhanced and now also tests more copy operations (from zip file system to zip file system and from zip file system to default file system and back).<br>
<br>
The points below were also addressed:<br>
<br>
> ZipFileAttributeView.java<br>
>  - can you please throw an AssertionError() for the default branch in<br>
> the switch expression in the "ZipFileAttributeView.name()".<br>
<br>
The default branch will return "basic". Looking at the code it should not be jumped to anyway.<br>
<br>
> ZipFileSystem.java<br>
>  - please also put @Override annotations on the method implementations<br>
> from ZipFileAttributes (e.g. "compressedSize()", "crc()", etc...) and<br>
> a comment at the place where the implementation of the<br>
> "PosixFileAttributes" methods starts.<br>
<br>
Done, I also reordered the methods - first "basic" then "posix" then "zip".<br>
<br>
> ZipUtils.java<br>
> - just a question: isn't it possible to reuse<br>
> sun.nio.fs.UnixFileModeAttribute.toUnixMode() and the corresponding<br>
> constants from sun.nio.fs.UnixConstants instead of redefining them<br>
> here? You could export them from java.base to jdk.zipfs but the<br>
> problem is probably that at least sun.nio.fs.UnixConstants is a<br>
> generated class which only gets generated on Unix systems, right ?<br>
<br>
You've already answered yourself 😊 These classes only exist on Unix type JDKs.<br>
<br>
> ZipFileAttributes.java<br>
> - it seems a little odd that you've added "setPermissions()" to<br>
> ZipFileAttributes. All the attribute classes (i.e.<br>
> BasicFileAttributes, PosixFileAttributes) have no setters. Isn't it<br>
> possible to implement "ZipFileAttributeView.setPermissions()" by<br>
> calling "path.setPermissions()" (as this is done in<br>
> "ZipFileAttributeView.setTimes()") and handle everything in<br>
> ZipFileSyste (as this is done with "setTimes()").<br>
<br>
Good catch & thanks for providing the better implementation.<br>
<br>
<br>
I think this version is quite final now and thoroughly tested. I hope to get some valid reviews soon...<br>
<br>
Thanks<br>
Christoph<br>
<br>
</blockquote></div>