Avoid unnecessarily nesting FilesetTraversalParams if the nesting adds no information.

--
MOS_MIGRATED_REVID=129012839

3 files changed, 53 insertions, 24 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
index dc6c247fe7..6c59132601 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
@@ -19,7 +19,6 @@ import com.google.devtools.build.lib.util.Fingerprint;
 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 java.util.Set;
 
 /**
@@ -155,7 +154,7 @@ public interface FilesetTraversalParams {
   }
 
   /** Label of the Fileset rule that owns this traversal. */
-  Label getOwnerLabel();
+  Label getOwnerLabelForErrorMessages();
 
   /** Returns the directory under the output path where the files will be mapped. May be empty. */
   PathFragment getDestPath();
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java
index 6b64fc9fe1..992cfc1a64 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java
@@ -105,7 +105,10 @@ public final class FilesetTraversalParamsFactory {
   }
 
   /**
-   * Creates traversal request parameters for a FilesetEntry wrapping another Fileset.
+   * Creates traversal request parameters for a FilesetEntry wrapping another Fileset. If possible,
+   * the original {@code nested} is returned to avoid unnecessary object creation. In that case, the
+   * {@code ownerLabelForErrorMessages} may be ignored. Since the wrapping traversal could not have
+   * an error on its own, any error messages printed will still be correct.
    *
    * @param ownerLabel the rule that created this object
    * @param nested the traversal params that were used for the nested (inner) Fileset
@@ -114,22 +117,31 @@ public final class FilesetTraversalParamsFactory {
    * @param excludes optional; set of files directly below (not in a subdirectory of) the nested
    *     Fileset that should be excluded from the outer Fileset
    */
-  public static FilesetTraversalParams nestedTraversal(Label ownerLabel,
-      FilesetTraversalParams nested, PathFragment destDir, @Nullable Set<String> excludes) {
+  public static FilesetTraversalParams nestedTraversal(
+      Label ownerLabel,
+      FilesetTraversalParams nested,
+      PathFragment destDir,
+      @Nullable Set<String> excludes) {
+    if (destDir.segmentCount() == 0 && (excludes == null || excludes.isEmpty())) {
+      // Wrapping the traversal here would not lead to a different result: the output location is
+      // the same and there are no additional excludes.
+      return nested;
+    }
     // When srcdir is another Fileset, then files must be null so strip_prefix must also be null.
     return new NestedTraversalParams(ownerLabel, nested, destDir, excludes);
   }
 
   private abstract static class ParamsCommon implements FilesetTraversalParams {
-    private final Label ownerLabel;
+    private final Label ownerLabelForErrorMessages;
     private final PathFragment destDir;
     private final ImmutableSet<String> excludes;
 
-    ParamsCommon(Label ownerLabel, PathFragment destDir, @Nullable Set<String> excludes) {
-      this.ownerLabel = ownerLabel;
+    ParamsCommon(
+        Label ownerLabelForErrorMessages, PathFragment destDir, @Nullable Set<String> excludes) {
+      this.ownerLabelForErrorMessages = ownerLabelForErrorMessages;
       this.destDir = destDir;
       if (excludes == null) {
-        this.excludes = ImmutableSet.<String>of();
+        this.excludes = ImmutableSet.of();
       } else {
         // Order the set for the sake of deterministic fingerprinting.
         this.excludes = ImmutableSet.copyOf(Ordering.natural().immutableSortedCopy(excludes));
@@ -137,8 +149,8 @@ public final class FilesetTraversalParamsFactory {
     }
 
     @Override
-    public Label getOwnerLabel() {
-      return ownerLabel;
+    public Label getOwnerLabelForErrorMessages() {
+      return ownerLabelForErrorMessages;
     }
 
     @Override
@@ -160,17 +172,24 @@ public final class FilesetTraversalParamsFactory {
 
     @Override
     public String toString() {
-      return super.toString() + "[" + destDir + ", " + ownerLabel + ", " + excludes + "]";
+      return super.toString()
+          + "["
+          + destDir
+          + ", "
+          + ownerLabelForErrorMessages
+          + ", "
+          + excludes
+          + "]";
     }
 
     protected boolean internalEquals(ParamsCommon that) {
-      return Objects.equals(this.ownerLabel, that.ownerLabel)
+      return Objects.equals(this.ownerLabelForErrorMessages, that.ownerLabelForErrorMessages)
           && Objects.equals(this.destDir, that.destDir)
           && Objects.equals(this.excludes, that.excludes);
     }
 
     protected int internalHashCode() {
-      return Objects.hash(ownerLabel, destDir, excludes);
+      return Objects.hash(ownerLabelForErrorMessages, destDir, excludes);
     }
   }
 
@@ -287,8 +306,11 @@ public final class FilesetTraversalParamsFactory {
   private static final class NestedTraversalParams extends ParamsCommon {
     private final FilesetTraversalParams nested;
 
-    public NestedTraversalParams(Label ownerLabel, FilesetTraversalParams nested,
-        PathFragment destDir, @Nullable Set<String> excludes) {
+    NestedTraversalParams(
+        Label ownerLabel,
+        FilesetTraversalParams nested,
+        PathFragment destDir,
+        @Nullable Set<String> excludes) {
       super(ownerLabel, destDir, excludes);
       this.nested = nested;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java
index 80f1d419a7..777794654f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java
@@ -31,7 +31,6 @@ import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -190,15 +189,21 @@ public final class FilesetEntryFunction implements SkyFunction {
     return FilesetEntryValue.of(ImmutableSet.copyOf(outputSymlinks.values()));
   }
 
-  /** Stores an output symlink unless it's excluded or would overwrite an existing one. */
-  private static void maybeStoreSymlink(FilesetOutputSymlink nestedLink, PathFragment destPath,
+  /** Stores an output symlink unless it would overwrite an existing one. */
+  private static void maybeStoreSymlink(
+      FilesetOutputSymlink nestedLink,
+      PathFragment destPath,
       Map<PathFragment, FilesetOutputSymlink> result) {
     maybeStoreSymlink(nestedLink.name, nestedLink.target, nestedLink.metadata, destPath, result);
   }
 
-  /** Stores an output symlink unless it's excluded or would overwrite an existing one. */
-  private static void maybeStoreSymlink(PathFragment linkName, PathFragment linkTarget,
-      String metadata, PathFragment destPath, Map<PathFragment, FilesetOutputSymlink> result) {
+  /** Stores an output symlink unless it would overwrite an existing one. */
+  private static void maybeStoreSymlink(
+      PathFragment linkName,
+      PathFragment linkTarget,
+      String metadata,
+      PathFragment destPath,
+      Map<PathFragment, FilesetOutputSymlink> result) {
     linkName = destPath.getRelative(linkName);
     if (!result.containsKey(linkName)) {
       result.put(linkName, new FilesetOutputSymlink(linkName, linkTarget, metadata));
@@ -237,11 +242,14 @@ public final class FilesetEntryFunction implements SkyFunction {
   private static String createErrorInfo(FilesetTraversalParams t) {
     if (t.getDirectTraversal().isPresent()) {
       DirectTraversal direct = t.getDirectTraversal().get();
-      return String.format("Fileset '%s' traversing %s '%s'", t.getOwnerLabel(),
+      return String.format(
+          "Fileset '%s' traversing %s '%s'",
+          t.getOwnerLabelForErrorMessages(),
           direct.isPackage() ? "package" : "file (or directory)",
           direct.getRoot().getRelativePart().getPathString());
     } else {
-      return String.format("Fileset '%s' traversing another Fileset", t.getOwnerLabel());
+      return String.format(
+          "Fileset '%s' traversing another Fileset", t.getOwnerLabelForErrorMessages());
     }
   }