RFR 8211919: ZipDirectoryStream should provide a stream of paths that are relative to the directory

[Lance Andersen]

Hi Christoph

Thank you for the feedback, please see below 
> On Jan 14, 2019, at 9:13 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
> 
> Hi Lance,
> 
> as I'm currently active in that area, it's an easy one for me to review this ��
> 
> Overall the change looks good, thanks for doing it. Here are some few nits I discovered:
> 
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java:
> in the comments:
> line 426: Maybe you could take the chance to improve the text of the comment ?
> // (1) assume all path from zip file itself is "normalized" -> (1) assume each path from zip file is "normalized"
> line 432: I think the word "also" can be removed
> 
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipPath.java:
> line 95: Could you please also fix the indentation of @Override (4 instead of 3 spaces)
> 
done
> test/jdk/jdk/nio/zipfs/Basic.java:
> line 45: I think you should remove 8211385 from the @bug tag, since you removed the DirectoryStream tests from this file

done
> 
> test/jdk/jdk/nio/zipfs/ZipFsDirectoryStreamTests.java
> I think you should add a @bug tag for both, 8211385 and 8211919

done
> line 53: UNZIPFS_MAP: not needed

I removed it though I had added it as I have some additional tests to add later which will need it.
> line 70: ; not needed in try statement
done (thought I got all of these earlier, guess not)
> line 72: jarFile should be createad outside of try block
Utils.createJarFile throws an IOException which is why it is there vs introducing a 2nd try block.
> line 97: spelling: “filter”
fixed though was “DirectoryStream.Filter” which I changed to DirectoryStream filter
> line 106: space before + missing
fixed
> line 144: ";" is not needed and bracket of try could be closed on this line
fixed
> line 170: Spelling: use "a" instead of "an" ?
> line 187: "an" instead of "a" ?
> line 205: same ("an" instead of "a”)

“a”  vs "an" is always fun, should be addressed.
> line 215: the assert for IllegalStateException could be dropped here, as it is a superclass of ClosedDirectoryStreamException which is asserted in line 219? Unless you want to use this as a test for ClosedDirectoryStreamException class hierarchy…

Well the spec expects IllegalStateException.  ClosedDirectoryStreamException is thrown by ZipFS so I also included this for now as a sanity check in case someone was already catching this.  I am happy to remove it though
> line 238: I think this commented debug code should be removed.
done, thought I did that earlier
> line 306: Looks like the matcher instance is not needed
true and removed
> line 309: you could use an import java.util.zip.ZipException?
done, still getting used to Intellij vs Netbeans which I like better for import handling 
> line 314: The check should rather be: if (ds.iterator().hasNext()) throw…

The check is fine as it is but I changed it  to make it clearer similar to DirectoryStream/Basic.java

Again, thank you for your feedback.

Updated webrev is here http://cr.openjdk.java.net/~lancea/8211919/webrev.01/index.html

Best
Lance
> 
> Best regards
> Christoph
> 
>> -----Original Message-----
>> From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> On Behalf
>> Of Lance Andersen
>> Sent: Samstag, 12. Januar 2019 21:13
>> To: core-libs-dev <core-libs-dev at openjdk.java.net>; nio-dev <nio-
>> dev at openjdk.java.net>
>> Subject: RFR 8211919: ZipDirectoryStream should provide a stream of paths
>> that are relative to the directory
>> 
>> Hi all,
>> 
>> The following patch addresses the issue where the stream of paths where
>> not relative to the specified directory as needed.
>> 
>> As part of the fix, I added additional DirectoryStream tests similar to the tests
>> in nio/file/DirectoryStream/Basic.java.
>> 
>> Webrev can be found:
>> http://cr.openjdk.java.net/~lancea/8211919/webrev.00/
>> 
>> Best
>> Lance
>> <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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>> 
>> 
> 

 <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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>