Make intra-package wildcards work for remote repositories and clean up target pattern parsing just a tiny little bit.

This wounds #389 dealing 4d6 fire damage (recursive wildcards, e.g. /... and friends still don't work)

--
MOS_MIGRATED_REVID=103822319

6 files changed, 98 insertions, 74 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
index 6dc23857ad..d456059b23 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
@@ -22,6 +22,8 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.LabelValidator.BadLabelException;
 import com.google.devtools.build.lib.cmdline.LabelValidator.PackageAndTarget;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier.RepositoryName;
+import com.google.devtools.build.lib.util.StringUtilities;
 
 import java.io.Serializable;
 import java.util.ArrayList;
@@ -197,7 +199,7 @@ public abstract class TargetPattern implements Serializable {
       Preconditions.checkArgument(excludedSubdirectories.isEmpty(),
           "Target pattern \"%s\" of type %s cannot be evaluated with excluded subdirectories: %s.",
           getOriginalPattern(), getType(), excludedSubdirectories);
-      return resolver.getExplicitTarget(targetName);
+      return resolver.getExplicitTarget(label(targetName));
     }
 
     @Override
@@ -251,7 +253,7 @@ public abstract class TargetPattern implements Serializable {
           getOriginalPattern(), getType(), excludedSubdirectories);
       if (resolver.isPackage(PackageIdentifier.createInDefaultRepo(path))) {
         // User has specified a package name. lookout for default target.
-        return resolver.getExplicitTarget("//" + path);
+        return resolver.getExplicitTarget(label("//" + path));
       }
 
       List<String> pieces = SLASH_SPLITTER.splitToList(path);
@@ -262,7 +264,7 @@ public abstract class TargetPattern implements Serializable {
         String packageName = SLASH_JOINER.join(pieces.subList(0, i));
         if (resolver.isPackage(PackageIdentifier.createInDefaultRepo(packageName))) {
           String targetName = SLASH_JOINER.join(pieces.subList(i, pieces.size()));
-          return resolver.getExplicitTarget("//" + packageName + ":" + targetName);
+          return resolver.getExplicitTarget(label("//" + packageName + ":" + targetName));
         }
       }
 
@@ -305,16 +307,16 @@ public abstract class TargetPattern implements Serializable {
 
   private static final class TargetsInPackage extends TargetPattern {
 
-    private final String pattern;
+    private final PackageIdentifier packageIdentifier;
     private final String suffix;
     private final boolean isAbsolute;
     private final boolean rulesOnly;
     private final boolean checkWildcardConflict;
 
-    private TargetsInPackage(String originalPattern, String pattern, String suffix,
-        boolean isAbsolute, boolean rulesOnly, boolean checkWildcardConflict) {
+    private TargetsInPackage(String originalPattern, PackageIdentifier packageIdentifier,
+        String suffix, boolean isAbsolute, boolean rulesOnly, boolean checkWildcardConflict) {
       super(Type.TARGETS_IN_PACKAGE, originalPattern);
-      this.pattern = Preconditions.checkNotNull(pattern);
+      this.packageIdentifier = packageIdentifier;
       this.suffix = Preconditions.checkNotNull(suffix);
       this.isAbsolute = isAbsolute;
       this.rulesOnly = rulesOnly;
@@ -335,17 +337,7 @@ public abstract class TargetPattern implements Serializable {
         }
       }
 
-      String packageName = removeSuffix(pattern, suffix);
-
-      try {
-        PackageIdentifier.parse(packageName);
-      } catch (LabelSyntaxException e) {
-        throw new TargetParsingException(
-            "Invalid package name '" + packageName + "': " + e.getMessage());
-      }
-
-      return resolver.getTargetsInPackage(getOriginalPattern(),
-          PackageIdentifier.createInDefaultRepo(packageName), rulesOnly);
+      return resolver.getTargetsInPackage(getOriginalPattern(), packageIdentifier, rulesOnly);
     }
 
     @Override
@@ -355,7 +347,7 @@ public abstract class TargetPattern implements Serializable {
 
     @Override
     public String getDirectory() {
-      return removeSuffix(pattern, suffix);
+      return packageIdentifier.getPackageFragment().getPathString();
     }
 
     @Override
@@ -375,13 +367,13 @@ public abstract class TargetPattern implements Serializable {
       return isAbsolute == that.isAbsolute && rulesOnly == that.rulesOnly
           && checkWildcardConflict == that.checkWildcardConflict
           && getOriginalPattern().equals(that.getOriginalPattern())
-          && pattern.equals(that.pattern) && suffix.equals(that.suffix);
+          && packageIdentifier.equals(that.packageIdentifier) && suffix.equals(that.suffix);
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(getType(), getOriginalPattern(), pattern, suffix, isAbsolute, rulesOnly,
-          checkWildcardConflict);
+      return Objects.hash(getType(), getOriginalPattern(), packageIdentifier, suffix, isAbsolute,
+          rulesOnly, checkWildcardConflict);
     }
 
     /**
@@ -397,18 +389,23 @@ public abstract class TargetPattern implements Serializable {
         return null;
       }
 
-      T target = resolver.getTargetOrNull("//" + pattern);
+      T target;
+      Label label;
+      try {
+        label = Label.create(packageIdentifier, suffix);
+        target = resolver.getTargetOrNull(label);
+      } catch (LabelSyntaxException e) {
+        return null;
+      }
+
       if (target != null) {
-        String name = pattern.lastIndexOf(':') != -1
-            ? pattern.substring(pattern.lastIndexOf(':') + 1)
-            : pattern.substring(pattern.lastIndexOf('/') + 1);
-        resolver.warn(String.format("The Blaze target pattern '%s' is ambiguous: '%s' is " +
+        resolver.warn(String.format("The target pattern '%s' is ambiguous: '%s' is " +
                                     "both a wildcard, and the name of an existing %s; " +
                                     "using the latter interpretation",
-                                    "//" + pattern, ":" + name,
+                                    getOriginalPattern(), ":" + suffix,
                                     resolver.getTargetKind(target)));
         try {
-          return resolver.getExplicitTarget("//" + pattern);
+          return resolver.getExplicitTarget(label);
         } catch (TargetParsingException e) {
           throw new IllegalStateException(
               "getTargetOrNull() returned non-null, so target should exist", e);
@@ -548,13 +545,18 @@ public abstract class TargetPattern implements Serializable {
 
       String originalPattern = pattern;
       final boolean includesRepo = pattern.startsWith("@");
-      String repoName = "";
+      RepositoryName repository = PackageIdentifier.DEFAULT_REPOSITORY_NAME;
       if (includesRepo) {
         int pkgStart = pattern.indexOf("//");
         if (pkgStart < 0) {
           throw new TargetParsingException("Couldn't find package in target " + pattern);
         }
-        repoName = pattern.substring(0, pkgStart);
+        try {
+          repository = RepositoryName.create(pattern.substring(0, pkgStart));
+        } catch (LabelSyntaxException e) {
+          throw new TargetParsingException(e.getMessage());
+        }
+
         pattern = pattern.substring(pkgStart);
       }
       final boolean isAbsolute = pattern.startsWith("//");
@@ -598,19 +600,32 @@ public abstract class TargetPattern implements Serializable {
       }
 
       if (ALL_RULES_IN_SUFFIXES.contains(targetPart)) {
+        PackageIdentifier packageIdentifier;
+        try {
+          packageIdentifier = PackageIdentifier.parse(repository.getName() + "//" + packagePart);
+        } catch (LabelSyntaxException e) {
+          throw new TargetParsingException(
+              "Invalid package name '" + packagePart + "': " + e.getMessage());
+        }
         return new TargetsInPackage(
-            originalPattern, pattern, ":" + targetPart, isAbsolute, true, true);
+            originalPattern, packageIdentifier, targetPart, isAbsolute, true, true);
       }
 
       if (ALL_TARGETS_IN_SUFFIXES.contains(targetPart)) {
+        PackageIdentifier packageIdentifier;
+        try {
+          packageIdentifier = PackageIdentifier.parse(repository.getName() + "//" + packagePart);
+        } catch (LabelSyntaxException e) {
+          throw new TargetParsingException(
+              "Invalid package name '" + packagePart + "': " + e.getMessage());
+        }
         return new TargetsInPackage(
-            originalPattern, pattern, ":" + targetPart, isAbsolute, false, true);
+            originalPattern, packageIdentifier, targetPart, isAbsolute, false, true);
       }
 
-
       if (includesRepo || isAbsolute || pattern.contains(":")) {
         PackageAndTarget packageAndTarget;
-        String fullLabel = repoName + "//" + pattern;
+        String fullLabel = repository.getName() + "//" + pattern;
         try {
           packageAndTarget = LabelValidator.validateAbsoluteLabel(fullLabel);
         } catch (BadLabelException e) {
@@ -666,6 +681,18 @@ public abstract class TargetPattern implements Serializable {
     }
   }
 
+  // Parse 'label' as a Label, mapping LabelSyntaxException into
+  // TargetParsingException.
+  private static Label label(String label) throws TargetParsingException {
+    try {
+      return Label.parseAbsolute(label);
+    } catch (LabelSyntaxException e) {
+      throw new TargetParsingException("invalid target format: '"
+          + StringUtilities.sanitizeControlChars(label) + "'; "
+          + StringUtilities.sanitizeControlChars(e.getMessage()));
+    }
+  }
+
   /**
    * The target pattern type (targets below package, in package, explicit target, etc.)
    */
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
index d106692d9f..abcd516f37 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
@@ -32,15 +32,15 @@ public interface TargetPatternResolver<T> {
   void warn(String msg);
 
   /**
-   * Returns a single target corresponding to the given name, or null. This method may only throw an
-   * exception if the current thread was interrupted.
+   * Returns a single target corresponding to the given label, or null. This method may only throw
+   * an exception if the current thread was interrupted.
    */
-  T getTargetOrNull(String targetName) throws InterruptedException;
+  T getTargetOrNull(Label label) throws InterruptedException;
 
   /**
-   * Returns a single target corresponding to the given name, or an empty or failed result.
+   * Returns a single target corresponding to the given label, or an empty or failed result.
    */
-  ResolvedTargets<T> getExplicitTarget(String targetName)
+  ResolvedTargets<T> getExplicitTarget(Label label)
       throws TargetParsingException, InterruptedException;
 
   /**
@@ -83,8 +83,8 @@ public interface TargetPatternResolver<T> {
       throws TargetParsingException, InterruptedException;
 
   /**
-   * Returns true, if and only if the given name corresponds to a package, i.e., a file with the
-   * name {@code packageName/BUILD} exists.
+   * Returns true, if and only if the given package identifier corresponds to a package, i.e., a
+   * file with the name {@code packageName/BUILD} exists in the appropriat repository.
    */
   boolean isPackage(PackageIdentifier packageIdentifier);
 
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternResolverUtil.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternResolverUtil.java
index 60a0d64f8a..0e86f7f2d1 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternResolverUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternResolverUtil.java
@@ -14,8 +14,6 @@
 package com.google.devtools.build.lib.pkgcache;
 
 import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.LabelValidator;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
@@ -23,7 +21,6 @@ import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.cmdline.TargetPatternResolver;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.util.StringUtilities;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 /**
@@ -34,26 +31,6 @@ public final class TargetPatternResolverUtil {
     // Utility class.
   }
 
-  // Parse 'label' as a Label, mapping LabelSyntaxException into
-  // TargetParsingException.
-  public static Label label(String label) throws TargetParsingException {
-    try {
-      return Label.parseAbsolute(label);
-    } catch (LabelSyntaxException e) {
-      throw invalidTarget(label, e.getMessage());
-    }
-  }
-
-  /**
-   * Returns a new exception indicating that a command-line target is invalid.
-   */
-  private static TargetParsingException invalidTarget(String packageName,
-                                                      String additionalMessage) {
-    return new TargetParsingException("invalid target format: '" +
-        StringUtilities.sanitizeControlChars(packageName) + "'; " +
-        StringUtilities.sanitizeControlChars(additionalMessage));
-  }
-
   public static String getParsingErrorMessage(String message, String originalPattern) {
     if (originalPattern == null) {
       return message;
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 a48117f757..34f5f0474a 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
@@ -131,7 +131,7 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
     }
 
     @Override
-    public Void getTargetOrNull(String targetName) throws InterruptedException {
+    public Void getTargetOrNull(Label label) throws InterruptedException {
       // Note:
       // This method is used in just one place, TargetPattern.TargetsInPackage#getWildcardConflict.
       // Returning null tells #getWildcardConflict that there is not a target with a name like
@@ -142,9 +142,8 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
     }
 
     @Override
-    public ResolvedTargets<Void> getExplicitTarget(String targetName)
+    public ResolvedTargets<Void> getExplicitTarget(Label label)
         throws TargetParsingException, InterruptedException {
-      Label label = TargetPatternResolverUtil.label(targetName);
       try {
         Target target = packageProvider.getTarget(env.getListener(), label);
         SkyKey key = TransitiveTraversalValue.key(target.getLabel());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
index 147a4e7046..25af04ec7f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier.RepositoryName;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
@@ -73,22 +72,20 @@ public class RecursivePackageProviderBackedTargetPatternResolver
   }
 
   @Override
-  public Target getTargetOrNull(String targetName) throws InterruptedException {
+  public Target getTargetOrNull(Label label) throws InterruptedException {
     try {
-      Label label = Label.parseAbsolute(targetName);
       if (!isPackage(label.getPackageIdentifier())) {
         return null;
       }
       return recursivePackageProvider.getTarget(eventHandler, label);
-    } catch (LabelSyntaxException | NoSuchThingException e) {
+    } catch (NoSuchThingException e) {
       return null;
     }
   }
 
   @Override
-  public ResolvedTargets<Target> getExplicitTarget(String targetName)
+  public ResolvedTargets<Target> getExplicitTarget(Label label)
       throws TargetParsingException, InterruptedException {
-    Label label = TargetPatternResolverUtil.label(targetName);
     try {
       Target target = recursivePackageProvider.getTarget(eventHandler, label);
       return policy.shouldRetain(target, true)
diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh
index 9fc7a76e7a..e24149a706 100755
--- a/src/test/shell/bazel/local_repository_test.sh
+++ b/src/test/shell/bazel/local_repository_test.sh
@@ -824,4 +824,28 @@ EOF
   expect_log "Hello User"
 }
 
+function test_package_wildcard_in_remote_repository() {
+  local r=$TEST_TMPDIR/r
+  rm -fr $r
+  mkdir -p $r/a
+  touch $r/{x,y,a/g,a/h}
+  cat > $r/BUILD <<EOF
+exports_files(["x", "y"])
+EOF
+
+  cat > $r/a/BUILD <<EOF
+exports_files(["g", "h"])
+EOF
+
+  cat > WORKSPACE <<EOF
+local_repository(name="r", path="$r")
+EOF
+
+  bazel query @r//:all-targets + @r//a:all-targets >& $TEST_log || fail "query failed"
+  expect_log "@r//:x"
+  expect_log "@r//:y"
+  expect_log "@r//a:g"
+  expect_log "@r//a:h"
+}
+
 run_suite "local repository tests"