GitHub repository archive download

Addresses #3661 and fixes #3770 bug

Changes:
- Properly escape the download URL. `GitCloner` used to incorrectly escape `/` delimiters as `%2F`. This change was originally introduced in #3770 but [rolled back](https://github.com/akira-baruah/bazel/commit/58c003cf98b81d433603eeeb3874c7dbba33e1f0) due to a bug that is now fixed below.
- Omit an initial "v" from a version tag (e.g. `v1.2.3`) in the archive prefix. GitHub seems to do this.

Closes #4002.

PiperOrigin-RevId: 175047685

2 files changed, 25 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/GitCloner.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/GitCloner.java
index 2ee5ba8834..a1c98fb928 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/GitCloner.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/GitCloner.java
@@ -62,6 +62,8 @@ public class GitCloner {
   private static final Pattern GITHUB_URL = Pattern.compile(
       "(?:git@|https?://)github\\.com[:/](\\w+)/(\\w+)\\.git");
 
+  private static final Pattern GITHUB_VERSION_FORMAT = Pattern.compile("v(\\d+\\.)*\\d+");
+
   private GitCloner() {
     // Only static methods in this class
   }
@@ -291,18 +293,23 @@ public class GitCloner {
     String repositoryName = matcher.group(2);
     String downloadUrl =
         "https://github.com/"
-            + UrlEscapers.urlPathSegmentEscaper().escape(
-                user + "/" + repositoryName + "/archive/" + descriptor.ref + ".tar.gz");
+            + UrlEscapers.urlFragmentEscaper()
+                .escape(user + "/" + repositoryName + "/archive/" + descriptor.ref + ".tar.gz");
     try {
       FileSystemUtils.createDirectoryAndParents(descriptor.directory);
       Path tgz = downloader.download(ImmutableList.of(new URL(downloadUrl)), uncheckedSha256,
           Optional.of("tar.gz"), descriptor.directory, eventHandler, clientEnvironment);
-      DecompressorValue.decompress(DecompressorDescriptor.builder()
-          .setArchivePath(tgz)
-          // GitHub puts the contents under a directory called <repo>-<commit>.
-          .setPrefix(repositoryName + "-" + descriptor.ref)
-          .setRepositoryPath(descriptor.directory)
-          .build());
+      String githubRef = descriptor.ref;
+      if (githubRef.startsWith("v") && GITHUB_VERSION_FORMAT.matcher(githubRef).matches()) {
+        githubRef = githubRef.substring(1);
+      }
+      DecompressorValue.decompress(
+          DecompressorDescriptor.builder()
+              .setArchivePath(tgz)
+              // GitHub puts the contents under a directory called <repo>-<commit>.
+              .setPrefix(repositoryName + "-" + githubRef)
+              .setRepositoryPath(descriptor.directory)
+              .build());
     } catch (InterruptedException | IOException e) {
       try {
         FileSystemUtils.deleteTree(descriptor.directory);
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/GitClonerTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/GitClonerTest.java
index 10b668b171..738058c421 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/GitClonerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/GitClonerTest.java
@@ -19,9 +19,11 @@ import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyListOf;
 import static org.mockito.Matchers.anyMapOf;
 import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache;
@@ -82,6 +84,14 @@ public class GitClonerTest extends BuildViewTestCase {
 
     HttpDownloadValue value = GitCloner.clone(
         rule, outputDirectory, eventHandler, clientEnvironment, downloader);
+    verify(downloader)
+        .download(
+            eq(ImmutableList.of(new URL("https://github.com/foo/bar/archive/1.2.3.tar.gz"))),
+            any(String.class),
+            eq(Optional.of("tar.gz")),
+            eq(outputDirectory),
+            any(ExtendedEventHandler.class),
+            anyMapOf(String.class, String.class));
     assertThat(value).isNotNull();
     assertThat(value.getPath()).isEqualTo(outputDirectory);
   }