From dfba96d081091e1e17c4f2b9970e8f79fca1dce6 Mon Sep 17 00:00:00 2001
From: Googler <jhorvitz@google.com>
Date: Fri, 27 Sep 2024 18:17:03 -0700
Subject: [PATCH] Remove unnecessary tolerance for duplicate fileset entries.

Fileset symlinks are already deduplicated by `SkyframeFilesetManifestAction`, so we can use a preconditions check instead of tolerating them.

PiperOrigin-RevId: 679795852
Change-Id: I3031de779731254e069d1e801010bcf595520b20
---
 .../build/lib/actions/FilesetManifest.java    | 30 +++++++++++--------
 .../build/lib/exec/FilesetManifestTest.java   | 14 ---------
 2 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java
index 7e2e84ac31b33a..8cabc82ccaa695 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java
@@ -13,7 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
-import com.google.common.base.Preconditions;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.common.base.Strings;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
@@ -109,8 +111,12 @@ public static FilesetManifest constructFilesetManifest(
       String artifact = Strings.emptyToNull(outputSymlink.getTargetPath().getPathString());
       if (isRelativeSymlink(outputSymlink)) {
         addRelativeSymlinkEntry(artifact, fullLocation, relSymlinkBehavior, relativeLinks);
-      } else if (!entries.containsKey(fullLocation)) { // Keep consistent behavior: no overwriting.
-        entries.put(fullLocation, artifact);
+      } else {
+        // Symlinks are already deduplicated by name in SkyframeFilesetManifestAction.
+        checkState(
+            entries.put(fullLocation, artifact) == null,
+            "Duplicate fileset entry at %s",
+            fullLocation);
       }
       if (outputSymlink.getMetadata() instanceof FileArtifactValue) {
         artifactValues.put(artifact, (FileArtifactValue) outputSymlink.getMetadata());
@@ -135,11 +141,11 @@ private static void addRelativeSymlinkEntry(
       throws ForbiddenRelativeSymlinkException {
     switch (relSymlinkBehavior) {
       case ERROR -> throw new ForbiddenRelativeSymlinkException(artifact);
-      case RESOLVE, RESOLVE_FULLY -> {
-        if (!relativeLinks.containsKey(fullLocation)) { // Keep consistent behavior: no overwriting.
-          relativeLinks.put(fullLocation, artifact);
-        }
-      }
+      case RESOLVE, RESOLVE_FULLY ->
+          checkState(
+              relativeLinks.put(fullLocation, artifact) == null,
+              "Duplicate fileset entry at %s",
+              fullLocation);
       case IGNORE -> {
         // Do nothing.
       }
@@ -171,8 +177,8 @@ private static void fullyResolveRelativeSymlinks(
       for (Map.Entry<PathFragment, String> e : relativeLinks.entrySet()) {
         PathFragment location = e.getKey();
         Path locationPath = fs.getPath("/").getRelative(location);
-        PathFragment value = PathFragment.create(Preconditions.checkNotNull(e.getValue(), e));
-        Preconditions.checkState(!value.isAbsolute(), e);
+        PathFragment value = PathFragment.create(checkNotNull(e.getValue(), e));
+        checkState(!value.isAbsolute(), e);
 
         locationPath.getParentDirectory().createDirectoryAndParents();
         locationPath.createSymbolicLink(value);
@@ -219,8 +225,8 @@ private static void resolveRelativeSymlinks(
       for (Map.Entry<PathFragment, String> e : relativeLinks.entrySet()) {
         PathFragment location = e.getKey();
         String value = e.getValue();
-        String actual = Preconditions.checkNotNull(value, e);
-        Preconditions.checkState(!actual.startsWith("/"), e);
+        String actual = checkNotNull(value, e);
+        checkState(!actual.startsWith("/"), e);
         PathFragment actualLocation = location;
 
         // Recursively resolve relative symlinks.
diff --git a/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java b/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java
index 782395fc167b4e..6682c9e176ab76 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java
@@ -144,20 +144,6 @@ public void testManifestWithExecRootRelativePath() throws Exception {
       assertThat(manifest.getEntries())
           .containsExactly(PathFragment.create("out/foo/bar"), "foo/bar");
     }
-
-    /** Current behavior is first one wins. */
-    @Test
-    public void testDefactoBehaviorWithDuplicateEntries() throws Exception {
-      List<FilesetOutputSymlink> symlinks =
-          ImmutableList.of(filesetSymlink("bar", "/foo/bar"), filesetSymlink("bar", "/baz"));
-
-      FilesetManifest manifest =
-          FilesetManifest.constructFilesetManifest(
-              symlinks, PathFragment.create("out/foo"), behavior);
-
-      assertThat(manifest.getEntries())
-          .containsExactly(PathFragment.create("out/foo/bar"), "/foo/bar");
-    }
   }
 
   /** Manifest tests that apply to a specific relative symlink behavior. */