Thanks! Ship it. On Wed, Sep 6, 2017 at 9:25 AM, Xueming Shen <xueming.s...@oracle.com> wrote:
> Thanks Martin! > > webrev has been updated as suggested (the old webrev has been renamed to > webrev00) > > http://cr.openjdk.java.net/~sherman/8186464/webrev > > The only thing is not updated is the centot, we are using it as "int" > anyway at other place. > Let's keep it for couple more years till the 32-bit is totally gone. > > -Sherman > > > On 9/5/17, 5:51 PM, Martin Buchholz wrote: > > Thanks for doing this. > I'm also hoping to contribute some testing effort, but time ... time ... > This Looks Good To Me, but as always I have comments. > --- > I wouldn't create an echo process. Just create a zip subprocess and write > the bytes from java to its stdin. > > 181 ProcessBuilder echo = new ProcessBuilder("echo", "hello"); > > --- > You should check whether there's a usable zip program, e.g. I once wrote > code like this: > > if (! new File("/usr/bin/perl").canExecute() > > 182 ProcessBuilder zip = new ProcessBuilder("zip", > path.toString().toString(), "-"); > > oh wait, now I see > > 196 } catch (IOException x) { 197 // ignore, probably > from process.start() for "Cannot run program" > > well ... I would still add some checks for /usr/bin/zip and elide the > catch. > --- > > > """Always specify the charset""" (here "ASCII" is fine). > > 163 if (!"hello".equals(new String(zf.getInputStream(new > ZipEntry("hello")) 164 > .readAllBytes()))) > > > --- > Can we say something like > // We must always check for a zip64 EOCD record; it is always permitted to > be present > > 1125 // try if we have a zip64 end; > > --- > Maybe > // end64 candidate found > > 1139 // end64 found, > > --- > It's 2017. Time to just make centot a long? > > 1152 end.centot = (int)centot64; // assume total > < 2g > > --- > Here's a typo to fix: > > entires > > > On Fri, Sep 1, 2017 at 4:17 PM, Xueming Shen <xueming.s...@oracle.com> > wrote: > >> On 8/22/17, 5:54 PM, Martin Buchholz wrote: >> >> >> >> On Tue, Aug 22, 2017 at 3:27 PM, Xueming Shen <xueming.s...@oracle.com> >> wrote: >> >>> >>> How about to add an option to our zipfs to force the ZIP64 end record >>> when >>> enabled. Harmless if not specified. >>> >> >> I agree that adding more options for testability is good. Since our >> users are likely to need such things as well, I'd also like to see such >> features become public, analogous to zip's -fz- flag. >> >> OTOH, interoperability testing is very valuable, so despite the troubles >> involved writing tests that involve both jdk and zipinfo code, I think we >> should do it. >> >> >> Martin, >> >> I have added a test case that tests "echo hello | zip infozip.zip -" via >> pb.startPipeline() >> in ReadZip.java, which should add some coverage for the "interoperability >> testing". >> >> Would you please help take a look? >> >> http://cr.openjdk.java.net/~sherman/8186464/webrev/ >> >> Thanks, >> Sherman >> > > >
