Fix leftover "size" check when deciding whether to use digest or mtime. Since we no longer stored mtime for empty files, this bug meant that we always compared empty files equal (which is good). But we shouldn't be using Metadata based on mtime for them.

A follow-up change will do a refactor to make this impossible.

--
MOS_MIGRATED_REVID=128742054

2 files changed, 7 insertions, 12 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index f6a02249d9..0b10ed416c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -36,7 +36,6 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.lib.vfs.Symlinks;
-
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.Arrays;
@@ -46,7 +45,6 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
-
 import javax.annotation.Nullable;
 
 /**
@@ -137,14 +135,10 @@ public class ActionMetadataHandler implements MetadataHandler {
         || value == FileArtifactValue.OMITTED_FILE_MARKER) {
       throw new FileNotFoundException();
     }
-    // If the file is empty or a directory, we need to return the mtime because the action cache
-    // uses mtime to determine if this artifact has changed.  We do not optimize for this code
-    // path (by storing the mtime somewhere) because we eventually may be switching to use digests
-    // for empty files. We want this code path to go away somehow too for directories (maybe by
-    // implementing FileSet in Skyframe).
-    return value.getSize() > 0
-        ? new Metadata(value.getDigest())
-        : new Metadata(value.getModifiedTime());
+    // If the file is a directory, we need to return the mtime because the action cache uses mtime
+    // to determine if this artifact has changed. We want this code path to go away somehow
+    // for directories (maybe by implementing FileSet in Skyframe).
+    return value.isFile() ? new Metadata(value.getDigest()) : new Metadata(value.getModifiedTime());
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
index 5125764b63..2c1dc5a887 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
@@ -76,10 +76,11 @@ public class FileArtifactValue extends ArtifactValue {
     this.mtime = -1;
   }
 
-  // Only used by empty files (non-null digest) and directories (null digest).
+  // Only used by directories (null digest).
   private FileArtifactValue(byte[] digest, long mtime, long size) {
     Preconditions.checkState(mtime >= 0, "mtime must be non-negative: %s %s", mtime, size);
     Preconditions.checkState(size == 0, "size must be zero: %s %s", mtime, size);
+    Preconditions.checkState(digest == null, "digest must be null:");
     this.digest = digest;
     this.size = size;
     this.mtime = mtime;
@@ -154,7 +155,7 @@ public class FileArtifactValue extends ArtifactValue {
 
   /** Gets last modified time of file. Should only be called if this is a directory. */
   long getModifiedTime() {
-    Preconditions.checkState(size == 0, "%s %s %s", digest, mtime, size);
+    Preconditions.checkState(size == 0 && digest == null, "%s %s %s", digest, mtime, size);
     return mtime;
   }