Hi Christoph, Thank you for taking this on.
Overall this looks pretty good. A few comments ZipFileSystem initCEN() I think I understand why you made some of the changes here, but for initializing for example cen, and end, starting on line 137, was there a reason you are doing this outside of the method now? FindEND() I know that endsub and comlen fields are not currently used by Zip FS but given they are valid fields in the header we should probably add a comment that they are not currently used which is why they are commented out? line 1245, comment to Sync can you fix the typo udpate to update? line 1928 IndexNode(IndexNode other) , is there a reason you left this code? line 2061 version(boolean) Could you add a comment to describe the method as the changes could be confusing to someone looking at this code for the 1st time lines 2362 and 2462 are commented out but were not previously. Can you clarify why? HTH Lance > On May 9, 2019, at 10:47 AM, Langer, Christoph <christoph.lan...@sap.com> > wrote: > > Hi, > > after exchanging a few mails with Lance, I have a new version ready: > http://cr.openjdk.java.net/~clanger/webrevs/8222276.2/ > <http://cr.openjdk.java.net/~clanger/webrevs/8222276.2/> > > I stepped back from changing the class hierarchy for ZipFileSystem.IndexNode > and ZipFileSystem.Entry. It’s not a prerequisite for my current proposal for > JDK-8213031 anymore. I’ve added further cleanups and implemented feedback I > got from Lance. > > Please get me some reviews 😊 > > Thanks > Christoph > > > From: Langer, Christoph > Sent: Dienstag, 30. April 2019 11:54 > To: core-libs-dev <core-libs-dev@openjdk.java.net>; nio-dev > <nio-...@openjdk.java.net>; Alan Bateman <alan.bate...@oracle.com>; Lance > Andersen <lance.ander...@oracle.com> > Subject: RE: RFR: 8222276: (zipfs) Refactoring and cleanups to prepare for > JDK-8213031 > > Hi, > > it turned out that there was a regression in my last version. I needed to fix > some initializations in the constructor of Entry. I also added some other > fixes found by the IDE’s code analysis to ZipFileSystem.java. > > By accident, I updated the last webrev in-place. So, please find my most > current version here:http://cr.openjdk.java.net/~clanger/webrevs/8222276.1/ > <http://cr.openjdk.java.net/~clanger/webrevs/8222276.1/> > > Now it passes all testing. > > Thank you > Christoph > > From: Langer, Christoph > Sent: Freitag, 26. April 2019 17:37 > To: core-libs-dev <core-libs-dev@openjdk.java.net > <mailto:core-libs-dev@openjdk.java.net>>; nio-dev <nio-...@openjdk.java.net > <mailto:nio-...@openjdk.java.net>>; Alan Bateman <alan.bate...@oracle.com > <mailto:alan.bate...@oracle.com>>; Lance Andersen <lance.ander...@oracle.com > <mailto:lance.ander...@oracle.com>> > Subject: RE: RFR: 8222276: (zipfs) Refactoring and cleanups to prepare for > JDK-8213031 > > Hi, > > ping 😊 > > I’ve rebased this change after the latest zipfs updates. I also made some > further modifications. Apart from minor things, I modified the constructors > of the ZipFileSystem.Entry class and I also made some post JDK-8222440 > cleanup. > > Please find the new webrev here: > http://cr.openjdk.java.net/~clanger/webrevs/8222276.1/ > <http://cr.openjdk.java.net/~clanger/webrevs/8222276.1/> > > The jtreg tests for zipfs are all passing. > > Thanks > Christoph > > From: Langer, Christoph > Sent: Donnerstag, 11. April 2019 16:06 > To: core-libs-dev <core-libs-dev@openjdk.java.net > <mailto:core-libs-dev@openjdk.java.net>>; nio-dev <nio-...@openjdk.java.net > <mailto:nio-...@openjdk.java.net>>; Alan Bateman <alan.bate...@oracle.com > <mailto:alan.bate...@oracle.com>>; Lance Andersen <lance.ander...@oracle.com > <mailto:lance.ander...@oracle.com>> > Subject: RFR: 8222276: (zipfs) Refactoring and cleanups to prepare for > JDK-8213031 > > Hi, > > working on JDK-8213031 to add posix file permission support for the zipfs, I > thought it makes sense to factor out some of the changes into a separate > change. Those changes reorganize and clean up some parts of the code which I > think makes sense in any case. > > Please review: > Bug: https://bugs.openjdk.java.net/browse/JDK-8222276 > <https://bugs.openjdk.java.net/browse/JDK-8222276> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8222276.0/ > <http://cr.openjdk.java.net/~clanger/webrevs/8222276.0/> > > In detail: > The main change is to the hierarchy of inner classes ZipFileSystem.IndexNode > and ZipFileSystem.Entry (the latter extends the former). Both classes are > static classes currently. It makes sense, though, to have those associated > with the ZipFileSystem they belong to. > To do that, I need to add a static superclass named ZFSNode which only holds > a node name and its hashcode. This class needs to exist because of the lookup > implementation to find specific nodes of tze ZipFileSystem in its node map. > Then the classes IndexNode extends ZFSNode and Entry extends IndexNode can be > made non-static. > > Further changes are: > Improve error handling for ZipFileAttributeView::get methods > improve handling for zip64 attribute/version > minor clenaups such as adding @Override annotations, whitespace fixes, > private method visibility etc. > > Furthermore, there is some suspicious coding in JarFileSystem:: > createVersionedLinks where a temporary key node is inserted into the alias > map. However, this is a singleton instance which always gets new values. I’ll > open a separate bug for that and I’ll try to create a test case which > demonstrates an issue with the current code. > > I ran the zipfs jtreg tests successfully and will put it into our internal > system as well as run it through jdk-submit. > > Thanks > Christoph <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <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.ander...@oracle.com <mailto:lance.ander...@oracle.com>
