Merge pull request #106 from cunei/wip-milliBis

Revert *ModifiedTime() calls to *lastModified*() calls

io/src/main/scala/sbt/internal/io/Milli.scala

@@ -237,22 +237,17 @@ private trait Mac extends Library with Posix[Long] {
                   options: Int): Int
 }
 private object MacMilli extends PosixMilliLong[Mac] {
-  private final val ATTR_BIT_MAP_COUNT: Short = 5
-  private final val ATTR_CMN_MODTIME: Int = 0x00000400
+  private val attr = new Attrlist
+  attr.bitmapcount = 5 // ATTR_BIT_MAP_COUNT
+  attr.commonattr = 0x00000400 // ATTR_CMN_MODTIME
 
   protected def getModifiedTimeNative(filePath: String) = {
-    val attr = new Attrlist
-    attr.bitmapcount = ATTR_BIT_MAP_COUNT
-    attr.commonattr = ATTR_CMN_MODTIME
     val buf = new TimeBuf
     checkedIO(filePath) { libc.getattrlist(filePath, attr, buf, 20 /* buf size */, 0) }
     (buf.tv_sec, buf.tv_nsec)
   }
 
   protected def setModifiedTimeNative(filePath: String, mtimeNative: (Long, Long)): Unit = {
-    val attr = new Attrlist
-    attr.bitmapcount = ATTR_BIT_MAP_COUNT
-    attr.commonattr = ATTR_CMN_MODTIME
     val buf = new Timespec
     val (sec, nsec) = mtimeNative
     buf.tv_sec = sec

io/src/main/scala/sbt/io/IO.scala

@@ -15,7 +15,7 @@ import java.io.{
   OutputStream,
   PrintWriter
 }
-import java.io.{ ObjectInputStream, ObjectStreamClass }
+import java.io.{ ObjectInputStream, ObjectStreamClass, FileNotFoundException }
 import java.net.{ URI, URISyntaxException, URL }
 import java.nio.charset.Charset
 import java.nio.file.FileSystems
@@ -306,7 +306,7 @@ object IO {
             }
           }
           if (preserveLastModified)
-            setModifiedTime(target, entry.getTime)
+            setModifiedTimeOrFalse(target, entry.getTime)
         } else {
           //log.debug("Ignoring zip entry '" + name + "'")
         }
@@ -543,7 +543,7 @@ object IO {
     def makeFileEntry(file: File, name: String) = {
       //			log.debug("\tAdding " + file + " as " + name + " ...")
       val e = createEntry(name)
-      e setTime getModifiedTime(file)
+      e setTime getModifiedTimeOrZero(file)
       e
     }
     def addFileEntry(file: File, name: String) = {
@@ -643,7 +643,7 @@ object IO {
       preserveLastModified: Boolean,
       preserveExecutable: Boolean
   )(from: File, to: File): File = {
-    if (overwrite || !to.exists || getModifiedTime(from) > getModifiedTime(to)) {
+    if (overwrite || !to.exists || getModifiedTimeOrZero(from) > getModifiedTimeOrZero(to)) {
       if (from.isDirectory)
         createDirectory(to)
       else {
@@ -728,7 +728,7 @@ object IO {
       }
     }
     if (preserveLastModified) {
-      copyModifiedTime(sourceFile, targetFile)
+      copyLastModified(sourceFile, targetFile)
       ()
     }
     if (preserveExecutable) {
@@ -737,33 +737,6 @@ object IO {
     }
   }
 
-  /**
-   * Transfers the last modified time of `sourceFile` to `targetFile`.
-   *
-   * Note: this method has an underspecified, and generally inconsistent
-   * behavior. In particular, if the source file is missing, it will
-   * silently set the target modification time to 1st January 1970,
-   * returning success. Also, because it uses lastModified(), it may
-   * trim away the millisecond part of the timestamp without notice.
-   *
-   * After sbt/io v1.0.2 (for the 1.0.x series) and v1.1.1 (for
-   * the 1.1.x series) a new method copyModifiedTime() has been added
-   * to sbt.io.IO. That method will correctly throw a FileNotFoundException
-   * if any of the two files are missing, or an IOException in case an
-   * IO exception occurs.
-   *
-   * The current behavior of copyLastModified() is retained for compatibility,
-   * but client code should be migrated to copyModifiedTime(), instead.
-   * The copyLastModified() method may be removed in future versions of sbt.io.IO.
-   */
-  @deprecated("Use copyModifiedTime() instead.", "1.0.3")
-  def copyLastModified(sourceFile: File, targetFile: File): Boolean = {
-    val last = sourceFile.lastModified
-    // lastModified can return a negative number, but setLastModified doesn't accept it
-    // see Java bug #6791812
-    targetFile.setLastModified(math.max(last, 0L))
-  }
-
   /** Transfers the executable property of `sourceFile` to `targetFile`. */
   def copyExecutable(sourceFile: File, targetFile: File) = {
     val executable = sourceFile.canExecute
@@ -1126,7 +1099,7 @@ object IO {
 
   /**
    * Return the last modification timestamp of the specified file,
-   * in milliseconds from the Unix epoch (January 1, 1970 UTC).
+   * in milliseconds since the Unix epoch (January 1, 1970 UTC).
    * This method will use whenever possible native code in order to
    * return a timestamp with a 1-millisecond precision.
    *
@@ -1157,9 +1130,16 @@ object IO {
    * and Java's get/setLastModifiedTime() will be used instead. This
    * setting applies to setModifiedTime() and copyModifiedTime() as well.
    *
+   * This method was added in sbt/io v1.1.2, but it may be
+   * replaced in the future by a similar method that returns
+   * a Try, rather than throwing an exception.
+   * It is therefore marked as deprecated, since we cannot
+   * guarantee that it will remain available in future versions.
+   *
    * @see setModifiedTime
    * @see copyModifiedTime
    */
+  @deprecated("This method might be removed in the future, also see getModifiedOrZero()", "1.1.3")
   def getModifiedTime(file: File): Long = Milli.getModifiedTime(file)
 
   /**
@@ -1174,9 +1154,19 @@ object IO {
    * contrast, Java's traditional setLastModified() will return a
    * boolean false value if an error occurs.
    *
+   * This method may not work correctly if mtime is negative.
+   *
+   * This method was added in sbt/io v1.1.2, but it may be
+   * replaced in the future by a similar method that returns
+   * a Try, rather than throwing an exception.
+   * It is therefore marked as deprecated, since we cannot
+   * guarantee that it will remain available in future versions.
+   *
    * @see getModifiedTime
    * @see copyModifiedTime
    */
+  @deprecated("This method might be removed in the future, also see setModifiedTimeOrFalse()",
+              "1.1.3")
   def setModifiedTime(file: File, mtime: Long): Unit = Milli.setModifiedTime(file, mtime)
 
   /**
@@ -1192,9 +1182,82 @@ object IO {
    * If the timestamp cannot be copied, this method will throw
    * a FileNotFoundException or an IOException, as appropriate.
    *
+   * This method was added in sbt/io v1.1.2, but it may be
+   * replaced in the future by a similar method that returns
+   * a Try, rather than throwing an exception.
+   * It is therefore marked as deprecated, since we cannot
+   * guarantee that it will remain available in future versions.
+   *
    * @see getModifiedTime
    * @see setModifiedTime
    */
+  @deprecated("This method might be removed in the future, also see copyLastModified()", "1.1.3")
   def copyModifiedTime(fromFile: File, toFile: File): Unit =
     Milli.copyModifiedTime(fromFile, toFile)
+
+  /**
+   * Return the last modification timestamp of the specified file,
+   * in milliseconds since the Unix epoch (January 1, 1970 UTC).
+   *
+   * If the specified file does not exist, this method will return 0L.
+   *
+   * The deprecated method getModifiedTime() has similar semantics,
+   * but will throw an exception if the file does not exist.
+   * Please refer to its documentation for additional details.
+   *
+   * @see getModifiedTime
+   */
+  def getModifiedTimeOrZero(file: File): Long =
+    try {
+      getModifiedTime(file)
+    } catch {
+      case _: FileNotFoundException => 0L
+    }
+
+  /**
+   * Sets the modification time of the file argument, in milliseconds
+   * since the Unix epoch (January 1, 1970 UTC).
+   *
+   * If the specified file does not exist, this method will return false.
+   * It will return true if the file modification time was successfully changed.
+   *
+   * The deprecated method setModifiedTime() has similar semantics,
+   * but will throw an exception if the file does not exist.
+   * Please refer to its documentation for additional details.
+   *
+   * This method may not work correctly if mtime is negative.
+   *
+   * @see setModifiedTime
+   */
+  def setModifiedTimeOrFalse(file: File, mtime: Long): Boolean =
+    try {
+      Milli.setModifiedTime(file, mtime)
+      true
+    } catch {
+      case _: FileNotFoundException => false
+    }
+
+  /**
+   * Transfers the last modified time of `sourceFile` to `targetFile`.
+   *
+   * Note: this method has a special semantics if files are missing.
+   * In particular, if the source file is missing, it will silently
+   * set the target modification time to 1st January 1970, which
+   * corresponds to the Unix epoch.
+   *
+   * The method returns true if the target file modification time was
+   * successfully changed, false otherwise.
+   *
+   * The deprecated related method copyModifiedTime() has a somewhat different
+   * semantics, please refer to its documentation for additional details.
+   *
+   * @see copyModifiedTime
+   */
+  def copyLastModified(sourceFile: File, targetFile: File): Boolean = {
+    val last = getModifiedTimeOrZero(sourceFile)
+    // getModifiedTimeOrZero can return a negative number, but setLastModified
+    // (which may be used by setModifiedTimeOrFalse) doesn't accept it,
+    // see Java bug #6791812
+    setModifiedTimeOrFalse(targetFile, math.max(last, 0L))
+  }
 }

io/src/main/scala/sbt/io/Path.scala

@@ -20,7 +20,7 @@ final class RichFile(val asFile: File) extends AnyVal with RichNioPath {
   def isDirectory: Boolean = asFile.isDirectory
 
   /** The last modified time of the wrapped file.*/
-  def lastModified: Long = IO.getModifiedTime(asFile)
+  def lastModified: Long = IO.getModifiedTimeOrZero(asFile)
 
   /**
    * True if and only if the wrapped file `asFile` exists and the file 'other'
@@ -277,7 +277,7 @@ object Path extends Mapper {
     separated.mkString(sep)
   }
   def newerThan(a: File, b: File): Boolean =
-    a.exists && (!b.exists || IO.getModifiedTime(a) > IO.getModifiedTime(b))
+    a.exists && (!b.exists || IO.getModifiedTimeOrZero(a) > IO.getModifiedTimeOrZero(b))
 
   /** The separator character of the platform.*/
   val sep: Char = java.io.File.separatorChar

io/src/main/scala/sbt/io/PollingWatchService.scala

@@ -93,7 +93,7 @@ class PollingWatchService(delay: FiniteDuration) extends WatchService {
       watched.toSeq.sortBy(_._1)(pathLengthOrdering).foreach {
         case (p, _) =>
           if (!results.contains(p))
-            p.toFile.allPaths.get.foreach(f => results += f.toPath -> IO.getModifiedTime(f))
+            p.toFile.allPaths.get.foreach(f => results += f.toPath -> IO.getModifiedTimeOrZero(f))
       }
       results.toMap
     }

io/src/test/scala/sbt/internal/io/SourceModificationWatchSpec.scala

@@ -21,10 +21,9 @@ abstract class SourceModificationWatchSpec(
 
     IO.write(file, "foo")
 
-    // watchTest(parentDir) {
-    //   IO.write(file, "bar")
-    // }
-    pending // until fixed https://github.com/sbt/io/issues/82
+    watchTest(parentDir) {
+      IO.write(file, "bar")
+    }
   }
 
   it should "watch a directory for file creation" in IO.withTemporaryDirectory { dir =>