Fix bug where all three implementations of RecursivePackageProvider#getPackagesUnderDirectory assumed that the given directory needed to be strictly underneath all directories in the given blacklistedSubdirectories set, yet the callsites were calling this method such that the given directory was not-necessarily-strictly underneath the blacklistedSubdirectories.

N.B. There is no end-to-end bug in EnvironmentBackedRecursivePackageProvider because TargetPatternFunction's usage of TargetPattern#eval is always with blacklistedSubdirectories=ImmutableSet.of(). Also note that the existing fast-path in GraphBackedRecursivePackageProvider is dead code because the Preconditions check would fail first :(

RELNOTES: None
PiperOrigin-RevId: 173924216

5 files changed, 20 insertions, 10 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
index 1a16b6c84c..998c36dd2b 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
@@ -38,9 +38,10 @@ public interface RecursivePackageProvider extends PackageProvider {
    *     and non-excluded directories beneath it will be reported here
    * @param directory a {@link RootedPath} specifying the directory to search
    * @param blacklistedSubdirectories a set of {@link PathFragment}s, all of which are beneath
-   *     {@code directory}, specifying transitive subdirectories to that have been blacklisted
+   *     {@code directory} (not necessarily strictly), specifying transitive subdirectories that
+   *     have been blacklisted
    * @param excludedSubdirectories a set of {@link PathFragment}s, all of which are beneath {@code
-   *     directory}, specifying transitive subdirectories to exclude
+   *     directory} (not necessarily strictly), specifying transitive subdirectories to exclude
    */
   Iterable<PathFragment> getPackagesUnderDirectory(
       ExtendedEventHandler eventHandler,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index e070b659b4..82cbe381f9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -141,9 +142,13 @@ public final class EnvironmentBackedRecursivePackageProvider implements Recursiv
       roots.add(repositoryValue.getPath());
     }
 
+    if (blacklistedSubdirectories.contains(directory)) {
+      return ImmutableList.of();
+    }
+    PathFragment.checkAllPathsAreUnder(blacklistedSubdirectories, directory);
+
     LinkedHashSet<PathFragment> packageNames = new LinkedHashSet<>();
     for (Path root : roots) {
-      PathFragment.checkAllPathsAreUnder(blacklistedSubdirectories, directory);
       RecursivePkgValue lookup = (RecursivePkgValue) env.getValue(RecursivePkgValue.key(
           repository, RootedPath.toRootedPath(root, directory), blacklistedSubdirectories));
       if (lookup == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index 0dd1b4d301..5cc487fe62 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -169,13 +169,14 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
       ImmutableSet<PathFragment> blacklistedSubdirectories,
       ImmutableSet<PathFragment> excludedSubdirectories)
       throws InterruptedException {
-    PathFragment.checkAllPathsAreUnder(blacklistedSubdirectories, directory);
-    PathFragment.checkAllPathsAreUnder(excludedSubdirectories, directory);
-
-    if (excludedSubdirectories.contains(directory)) {
+    if (blacklistedSubdirectories.contains(directory)
+        || excludedSubdirectories.contains(directory)) {
       return ImmutableList.of();
     }
 
+    PathFragment.checkAllPathsAreUnder(blacklistedSubdirectories, directory);
+    PathFragment.checkAllPathsAreUnder(excludedSubdirectories, directory);
+
     // Check that this package is covered by at least one of our universe patterns.
     boolean inUniverse = false;
     for (TargetPatternKey patternKey : universeTargetPatternKeys) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
index b2ce5b6469..6e650a5db6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
@@ -239,10 +239,13 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
         BatchCallback<Void, E> callback,
         Class<E> exceptionClass)
         throws TargetParsingException, E, InterruptedException {
+      PathFragment directoryPathFragment = TargetPatternResolverUtil.getPathFragment(directory);
+      if (blacklistedSubdirectories.contains(directoryPathFragment)) {
+        return;
+      }
       Preconditions.checkArgument(excludedSubdirectories.isEmpty(), excludedSubdirectories);
       FilteringPolicy policy =
           rulesOnly ? FilteringPolicies.RULES_ONLY : FilteringPolicies.NO_FILTER;
-      PathFragment pathFragment = TargetPatternResolverUtil.getPathFragment(directory);
       List<Path> roots = new ArrayList<>();
       if (repository.isMain()) {
         roots.addAll(pkgPath.getPathEntries());
@@ -262,7 +265,7 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
       }
 
       for (Path root : roots) {
-        RootedPath rootedPath = RootedPath.toRootedPath(root, pathFragment);
+        RootedPath rootedPath = RootedPath.toRootedPath(root, directoryPathFragment);
         env.getValues(
             ImmutableList.of(
                 PrepareDepsOfTargetsUnderDirectoryValue.key(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
index 2d3025b612..3fee688970 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
@@ -70,7 +70,7 @@ public class RecursivePkgValue implements SkyValue {
    * A RecursivePkgKey is a tuple of a {@link RootedPath}, {@code rootedPath}, defining the
    * directory to recurse beneath in search of packages, and an {@link ImmutableSet} of {@link
    * PathFragment}s, {@code excludedPaths}, relative to {@code rootedPath.getRoot}, defining the
-   * set of subdirectories beneath {@code rootedPath} to skip.
+   * set of subdirectories strictly beneath {@code rootedPath} to skip.
    *
    * <p>Throws {@link IllegalArgumentException} if {@code excludedPaths} contains any paths that
    * are equal to {@code rootedPath} or that are not beneath {@code rootedPath}.