<br><br><div class="gmail_quote">On Sun, Jun 28, 2009 at 22:47, Xueming Shen <span dir="ltr"><<a href="mailto:Xueming.Shen@sun.com">Xueming.Shen@sun.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Looks fine.<br>
<br>
(1) "import java.nio.file.Paths" is not used by anyone.</blockquote><div><br>Let's make sure to start using nio2 then...<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
(2)nio2 APIs are good, but now I can't not just copy/paste the jar Main to 6.x:-)<br>
</blockquote><div><br>That *is* a problem.  Fortunately, it's a small change.<br><br>Is there anything else stopping the jdk6 version from being identical to jdk7?<br><br>Martin<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
sherman<br>
<br>
<br>
Martin Buchholz wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div class="h5">
<br>
<br>
On Fri, Jun 26, 2009 at 10:33, Xueming Shen <<a href="mailto:Xueming.Shen@sun.com" target="_blank">Xueming.Shen@sun.com</a> <mailto:<a href="mailto:Xueming.Shen@sun.com" target="_blank">Xueming.Shen@sun.com</a>>> wrote:<br>

<br>
    The latest version looks good. 2 nit comments<br>
<br>
    (1) it's reasonable to have createTempFileInSamDirectoryAs<br>
    separate out, but I would<br>
    keep the directoryOf within it. Yes, it's  "clearer":-)<br>
<br>
<br>
Done.<br>
<br>
     }<br>
      /**<br>
-     * A variant of File.getParentFile that always returns a valid<br>
-     * directory (i.e. returns new File(".") where File.getParentFile<br>
-     * would return null).<br>
-     */<br>
-    private static File directoryOf(File file) {<br>
-        File dir = file.getParentFile();<br>
-        return (dir != null) ? dir : new File(".");<br>
-    }<br>
-<br>
-    /**<br>
      * Creates a new empty temporary file in the same directory as the<br>
      * specified file.  A variant of File.createTempFile.<br>
      */<br>
     private static File createTempFileInSameDirectoryAs(File file)<br>
         throws IOException {<br>
-        return File.createTempFile("jartmp", null, directoryOf(file));<br>
+        File dir = file.getParentFile();<br>
+        if (dir == null)<br>
+            dir = new File(".");<br>
+        return File.createTempFile("jartmp", null, dir);<br>
     }<br>
      private boolean ok;<br>
<br>
 <br>
<br>
    (2)the "updateOK" in dumpIndex really serves nobody but "clearer":-)<br>
<br>
<br>
I tried to improve dumpIndex by<br>
- removing updateOk variable.<br>
- using Path.moveTo is more likely to be atomic, so that<br>
  a concurrent process is less likely to find the jar being updated missing.<br>
(Alan: is there a better way to do the common task of replacing a file<br>
with its transformed output?)<br>
<br>
      */<br>
     void dumpIndex(String rootjar, JarIndex index) throws IOException {<br>
         File jarFile = new File(rootjar);<br>
-        File tmpFile = createTempFileInSameDirectoryAs(jarFile);<br>
+        Path jarPath = jarFile.toPath();<br>
+        Path tmpPath = createTempFileInSameDirectoryAs(jarFile).toPath();<br>
         try {<br>
-            boolean updateOk = update(new FileInputStream(jarFile),<br>
-                                      new FileOutputStream(tmpFile),<br>
-                                      null, index);<br>
-            if (updateOk) {<br>
-                jarFile.delete();<br>
-                if (!tmpFile.renameTo(jarFile)) {<br>
-                    throw new IOException(getMsg("error.write.file"));<br>
+            if (update(jarPath.newInputStream(),<br>
+                       tmpPath.newOutputStream(),<br>
+                       null, index)) {<br>
+                try {<br>
+                    tmpPath.moveTo(jarPath, REPLACE_EXISTING);<br>
+                } catch (IOException e) {<br>
+                    throw new IOException(getMsg("error.write.file"), e);<br>
                 }<br>
             }<br>
         } finally {<br>
-            tmpFile.delete();<br>
+            tmpPath.delete(false);<br>
         }<br>
     }<br>
<br>
webrev updated.<br>
 Martin<br>
<br>
<br>
 <br>
<br>
    sherman<br>
<br>
    btw, while you are here:-) do you have time to make the update()<br>
    NOT to put jarIndex the<br>
    first one when there is a manifest present? The JarInputStream<br>
    assumes the manifest is always<br>
    the first entry (or the second if the manifest dir is the first),<br>
    which causes problem...I'm not saying<br>
    to mix with this one:-) just in case you have time and interested<br>
    while you are here:-)<br>
<br>
<br>
    Martin Buchholz wrote:<br>
<br>
        I did some benchmarking,<br>
        and found that my changes "only" make jar 10-20% faster.<br>
        Disappointing - we expect an order of magnitude for every commit!<br>
<br>
        While benchmarking, I discovered to my horror that the simple<br>
<br>
        jar cf /tmp/t1 ...<br>
        jar i /tmp/t1<br>
<br>
        fails, because it tries to create the replacement jar in "."<br>
        and then<br>
        rename() it into place.  Oops... Better refactor out the code that<br>
        puts the replacement temp file in the same directory.<br>
        Better write some tests for that, too.<br>
<br>
           /**<br>
            * A variant of File.getParentFile that always returns a valid<br>
            * directory (i.e. returns new File(".") where<br>
        File.getParentFile<br>
            * would return null).<br>
            */<br>
           private static File directoryOf(File file) {<br>
               File dir = file.getParentFile();<br>
               return (dir != null) ? dir : new File(".");<br>
           }<br>
<br>
           /**<br>
            * Creates a new empty temporary file in the same directory<br>
        as the<br>
            * specified file.  A variant of File.createTempFile.<br>
            */<br>
           private static File createTempFileInSameDirectoryAs(File<br>
        file) throws IOException {<br>
               return File.createTempFile("jartmp", null,<br>
        directoryOf(file));<br>
           }<br>
<br>
        ---<br>
<br>
        While we're here,  let's remove the double buffering on file<br>
        copy operations.<br>
        And the repeated allocations of big buffers.<br>
<br>
           /**<br>
            * A buffer for use only by copy(InputStream, OutputStream).<br>
            * Not as clean as allocating a new buffer as needed by copy,<br>
            * but significantly more efficient.<br>
            */<br>
           private byte[] copyBuf = new byte[8192];<br>
<br>
           /**<br>
            * Copies all bytes from the input stream to the output stream.<br>
            * Does not close or flush either stream.<br>
            *<br>
            * @param from the input stream to read from<br>
            * @param to the output stream to write to<br>
            * @throws IOException if an I/O error occurs<br>
            */<br>
           private void copy(InputStream from, OutputStream to) throws<br>
        IOException {<br>
               int n;<br>
               while ((n = from.read(copyBuf)) != -1)<br>
                   to.write(copyBuf, 0, n);<br>
           }<br>
<br>
           /**<br>
            * Copies all bytes from the input file to the output stream.<br>
            * Does not close or flush the output stream.<br>
            *<br>
            * @param from the input file to read from<br>
            * @param to the output stream to write to<br>
            * @throws IOException if an I/O error occurs<br>
            */<br>
           private void copy(File from, OutputStream to) throws<br>
        IOException {<br>
               InputStream in = new FileInputStream(from);<br>
               try {<br>
                   copy(in, to);<br>
               } finally {<br>
                   in.close();<br>
               }<br>
           }<br>
<br>
           /**<br>
            * Copies all bytes from the input stream to the output file.<br>
            * Does not close the input stream.<br>
            *<br>
            * @param from the input stream to read from<br>
            * @param to the output file to write to<br>
            * @throws IOException if an I/O error occurs<br>
            */<br>
           private void copy(InputStream from, File to) throws<br>
        IOException {<br>
               OutputStream out = new FileOutputStream(to);<br>
               try {<br>
                   copy(from, out);<br>
               } finally {<br>
                   out.close();<br>
               }<br>
           }<br>
<br>
        ----<br>
<br>
        On the other hand, allocating a CRC32 is very cheap, so let's<br>
        make that<br>
        private to CRC32OutputStream.<br>
        +    private static class CRC32OutputStream extends<br>
        java.io.OutputStream {<br>
        +        final CRC32 crc = new CRC32();<br>
<br>
        ----<br>
<br>
        We should add a try { finally } to delete the tempfile for jar i<br>
        +        try {<br>
                boolean updateOk = update(new FileInputStream(jarFile),<br>
        -                                  new<br>
        FileOutputStream(scratchFile),<br>
<br>
        +                                      new<br>
        FileOutputStream(tmpFile),<br>
                                          null, index);<br>
        +            if (updateOk) {<br>
                jarFile.delete();<br>
<br>
        -        if (!scratchFile.renameTo(jarFile)) {<br>
        -            scratchFile.delete();<br>
        +                if (!tmpFile.renameTo(jarFile)) {<br>
<br>
                    throw new IOException(getMsg("error.write.file"));<br>
                }<br>
        -        scratchFile.delete();<br>
        +            }<br>
        +        } finally {<br>
<br>
        +            tmpFile.delete();<br>
        +        }<br>
            }<br>
                 ----<br>
<br>
        Webrev updated.<br>
        <a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/~martin/jar-misc/</a><br>
        <<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
        <<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<br>
<br>
        There are now many small changes combined in this fix.  Sorry<br>
        about that.<br>
        I'd be more inclined to separate them if creating new bugs was<br>
        easier.<br>
<br>
        I'm not planning on making any more changes.  Really.<br>
<br>
        Martin<br>
<br>
        On Thu, Jun 25, 2009 at 12:00, Martin Buchholz<br>
        <<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a> <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>><br></div></div><div><div></div><div class="h5">

        <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a> <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>>>> wrote:<br>
<br>
           I have an updated version of this fix, with these changes:<br>
<br>
           - Documented the turkish i problem<br>
<br>
               /**<br>
                * Compares two strings for equality, ignoring case.<br>
         The second<br>
                * argument must contain only upper-case ASCII characters.<br>
                * We don't want case comparison to be locale-dependent<br>
        (else we<br>
                * have the notorious "turkish i bug").<br>
                */<br>
               private boolean equalsIgnoreCase(String s, String upper) {<br>
<br>
           - Refactored code so that updateEntry now also sets the<br>
        method to<br>
           STORED.<br>
<br>
                   /**<br>
                    * Updates a ZipEntry which describes the data read<br>
        by this<br>
                    * output stream, in STORED mode.<br>
                    */<br>
                   public void updateEntry(ZipEntry e) {<br>
                       e.setMethod(ZipEntry.STORED);<br>
                       e.setSize(n);<br>
                       e.setCrc(crc.getValue());<br>
                   }<br>
<br>
           - addIndex was never updating the size in the ZipEntry (as<br>
        required),<br>
             which was not previously noticed because closeEntry was never<br>
           called.<br>
<br>
               private void addIndex(JarIndex index, ZipOutputStream zos)<br>
                   throws IOException<br>
               {<br>
                   ZipEntry e = new ZipEntry(INDEX_NAME);<br>
                   e.setTime(System.currentTimeMillis());<br>
                   if (flag0) {<br>
                       CRC32OutputStream os = new<br>
        CRC32OutputStream(crc32);<br>
                       index.write(os);<br>
                       os.updateEntry(e);<br>
                   }<br>
                   zos.putNextEntry(e);<br>
                   index.write(zos);<br>
                   zos.closeEntry();<br>
<br>
               }<br>
<br>
           <a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/~martin/jar-misc/</a><br>
        <<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
           <<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<br>
           Previous webrev:<br>
           <a href="http://cr.openjdk.java.net/%7Emartin/jar-misc.0/" target="_blank">http://cr.openjdk.java.net/~martin/jar-misc.0/</a><br>
        <<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc.0/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc.0/</a>><br>
           <<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc.0/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc.0/</a>><br>
<br>
<br>
           Martin<br>
<br>
<br>
<br>
           On Wed, Jun 24, 2009 at 19:34, Martin Buchholz<br>
           <<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a> <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>><br></div></div><div><div></div><div class="h5">

        <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a> <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>>>> wrote:<br>
<br>
               Hi jar team,<br>
<br>
               I have a bunch of minor improvements to<br>
                src/share/classes/sun/tools/jar/Main.java<br>
<br>
               Toby and Xueming, please review.<br>
<br>
               Warning: the index code has not been maintained for<br>
        many years.<br>
<br>
               Xueming, please file a bug.<br>
<br>
               Synopsis: Miscellaneous improvements to "jar".<br>
               Description:<br>
               - Use standard jdk coding style for javadoc<br>
               - Don't create a temp file for jar index in STORED mode.<br>
               - Don't use synchronized collections.<br>
               - Fix javac warnings.<br>
               - Don't define new names for things like INDEX_NAME;<br>
                 use static import instead.<br>
               - more efficiently compare special file names in update<br>
        mode.<br>
                 Update mode should be measurably faster.<br>
               - make CRC32OutputStream a nested class.<br>
                 refactor crc32.reset and updating entry into<br>
        CRC32OutputStream.<br>
               - Fix apparently benign bug updating n in<br>
               CRC32OutputStream.write(byte[], int, int)<br>
<br>
               Evaluation: Yep.<br>
<br>
               <a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/~martin/jar-misc/</a><br>
        <<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
               <<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<br>
               Thanks,<br>
<br>
               Martin<br>
<br>
<br>
<br>
<br>
<br>
</div></div></blockquote>
<br>
</blockquote></div><br>