Merge branch 'git-archive-golang' into 'master'

Let gitlab-git-http-server handle archive downloads

This change relies on changes in gitlab_git and gitlab-git-http-server.

fixes #2429

See merge request !1548

13 files changed, 62 insertions, 253 deletions

diff --git a/Gemfile b/Gemfile
index 39371c0a642..29d9db05f42 100644
--- a/Gemfile
+++ b/Gemfile
@@ -39,7 +39,7 @@ gem "browser", '~> 1.0.0'
 
 # Extracting information from a git repository
 # Provide access to Gitlab::Git library
-gem "gitlab_git", '~> 7.2.18'
+gem "gitlab_git", '~> 7.2.19'
 
 # LDAP Auth
 # GitLab fork with several improvements to original library. For full list of changes
diff --git a/Gemfile.lock b/Gemfile.lock
index 403d7095197..ee545f366f4 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -278,7 +278,7 @@ GEM
       mime-types (~> 1.19)
     gitlab_emoji (0.1.1)
       gemojione (~> 2.0)
-    gitlab_git (7.2.18)
+    gitlab_git (7.2.19)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.6)
       gitlab-linguist (~> 3.0)
@@ -815,7 +815,7 @@ DEPENDENCIES
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-linguist (~> 3.0.1)
   gitlab_emoji (~> 0.1)
-  gitlab_git (~> 7.2.18)
+  gitlab_git (~> 7.2.19)
   gitlab_meta (= 7.0)
   gitlab_omniauth-ldap (~> 1.2.1)
   gollum-lib (~> 4.0.2)
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 2b2ea3dff16..f0124c6bd60 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -150,7 +150,7 @@ class ApplicationController < ActionController::Base
   end
 
   def git_not_found!
-    render "errors/git_not_found", layout: "errors", status: 404
+    render html: "errors/git_not_found", layout: "errors", status: 404
   end
 
   def method_missing(method_sym, *arguments, &block)
diff --git a/app/controllers/projects/repositories_controller.rb b/app/controllers/projects/repositories_controller.rb
index c4a5e2d6359..ba9aea1c165 100644
--- a/app/controllers/projects/repositories_controller.rb
+++ b/app/controllers/projects/repositories_controller.rb
@@ -11,18 +11,9 @@ class Projects::RepositoriesController < Projects::ApplicationController
   end
 
   def archive
-    begin
-      file_path = ArchiveRepositoryService.new(@project, params[:ref], params[:format]).execute
-    rescue
-      return head :not_found
-    end
-
-    if file_path
-      # Send file to user
-      response.headers["Content-Length"] = File.open(file_path).size.to_s
-      send_file file_path
-    else
-      redirect_to request.fullpath
-    end
+    render json: ArchiveRepositoryService.new(@project, params[:ref], params[:format]).execute
+  rescue => ex
+    logger.error("#{self.class.name}: #{ex}")
+    return git_not_found!
   end
 end
diff --git a/app/services/archive_repository_service.rb b/app/services/archive_repository_service.rb
index e1b41527d8d..6414b5a0184 100644
--- a/app/services/archive_repository_service.rb
+++ b/app/services/archive_repository_service.rb
@@ -9,17 +9,10 @@ class ArchiveRepositoryService
   def execute(options = {})
     project.repository.clean_old_archives
 
-    raise "No archive file path" unless file_path
+    metadata = project.repository.archive_metadata(ref, storage_path, format)
+    raise "Repository or ref not found" if metadata.empty?
 
-    return file_path if archived?
-
-    unless archiving?
-      RepositoryArchiveWorker.perform_async(project.id, ref, format)
-    end
-
-    archived = wait_until_archived(options[:timeout] || 5.0)
-
-    file_path if archived
+    metadata
   end
 
   private
@@ -27,36 +20,4 @@ class ArchiveRepositoryService
   def storage_path
     Gitlab.config.gitlab.repository_downloads_path
   end
-
-  def file_path
-    @file_path ||= project.repository.archive_file_path(ref, storage_path, format)
-  end
-
-  def pid_file_path
-    @pid_file_path ||= project.repository.archive_pid_file_path(ref, storage_path, format)
-  end
-
-  def archived?
-    File.exist?(file_path)
-  end
-
-  def archiving?
-    File.exist?(pid_file_path)
-  end
-
-  def wait_until_archived(timeout = 5.0)
-    return archived? if timeout == 0.0
-    
-    t1 = Time.now
-
-    begin
-      sleep 0.1
-
-      success = archived?
-
-      t2 = Time.now
-    end until success || t2 - t1 >= timeout
-
-    success
-  end
 end
diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb
index 2d96c9666d2..20d568cf462 100644
--- a/lib/api/repositories.rb
+++ b/lib/api/repositories.rb
@@ -133,7 +133,7 @@ module API
         authorize! :download_code, user_project
 
         begin
-          file_path = ArchiveRepositoryService.new(
+          ArchiveRepositoryService.new(
             user_project,
             params[:sha],
             params[:format]
@@ -141,17 +141,6 @@ module API
         rescue
           not_found!('File')
         end
-
-        if file_path && File.exists?(file_path)
-          data = File.open(file_path, 'rb').read
-          basename = File.basename(file_path)
-          header['Content-Disposition'] = "attachment; filename=\"#{basename}\""
-          content_type MIME::Types.type_for(file_path).first.content_type
-          env['api.format'] = :binary
-          present data
-        else
-          redirect request.fullpath
-        end
       end
 
       # Compare two branches, tags or commits
diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index 0353b3b7ed3..6830a916bcb 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -193,7 +193,14 @@ module Grack
     end
 
     def render_grack_auth_ok
-      [200, { "Content-Type" => "application/json" }, [JSON.dump({ 'GL_ID' => Gitlab::ShellEnv.gl_id(@user) })]]
+      [
+        200,
+        { "Content-Type" => "application/json" },
+        [JSON.dump({
+          'GL_ID' => Gitlab::ShellEnv.gl_id(@user),
+          'RepoPath' => project.repository.path_to_repo,
+        })]
+      ]
     end
 
     def render_not_found
diff --git a/lib/support/nginx/gitlab b/lib/support/nginx/gitlab
index 7218a4d2f20..1e55c5a0486 100644
--- a/lib/support/nginx/gitlab
+++ b/lib/support/nginx/gitlab
@@ -113,7 +113,25 @@ server {
     proxy_pass http://gitlab;
   }
 
-  location ~ [-\/\w\.]+\.git\/ {
+  location ~ ^/[\w\.-]+/[\w\.-]+/(info/refs|git-upload-pack|git-receive-pack)$ {
+    # 'Error' 418 is a hack to re-use the @gitlab-git-http-server block
+    error_page 418 = @gitlab-git-http-server;
+    return 418;
+  }
+
+  location ~ ^/[\w\.-]+/[\w\.-]+/repository/archive {
+    # 'Error' 418 is a hack to re-use the @gitlab-git-http-server block
+    error_page 418 = @gitlab-git-http-server;
+    return 418;
+  }
+
+  location ~ ^/api/v3/projects/.*/repository/archive {
+    # 'Error' 418 is a hack to re-use the @gitlab-git-http-server block
+    error_page 418 = @gitlab-git-http-server;
+    return 418;
+  }
+
+  location @gitlab-git-http-server {
     ## If you use HTTPS make sure you disable gzip compression
     ## to be safe against BREACH attack.
     # gzip off;
diff --git a/lib/support/nginx/gitlab-ssl b/lib/support/nginx/gitlab-ssl
index 7dabfba87e2..08641bbcc17 100644
--- a/lib/support/nginx/gitlab-ssl
+++ b/lib/support/nginx/gitlab-ssl
@@ -160,7 +160,25 @@ server {
     proxy_pass http://gitlab;
   }
 
-  location ~ [-\/\w\.]+\.git\/ {
+  location ~ ^/[\w\.-]+/[\w\.-]+/(info/refs|git-upload-pack|git-receive-pack)$ {
+    # 'Error' 418 is a hack to re-use the @gitlab-git-http-server block
+    error_page 418 = @gitlab-git-http-server;
+    return 418;
+  }
+
+  location ~ ^/[\w\.-]+/[\w\.-]+/repository/archive {
+    # 'Error' 418 is a hack to re-use the @gitlab-git-http-server block
+    error_page 418 = @gitlab-git-http-server;
+    return 418;
+  }
+
+  location ~ ^/api/v3/projects/.*/repository/archive {
+    # 'Error' 418 is a hack to re-use the @gitlab-git-http-server block
+    error_page 418 = @gitlab-git-http-server;
+    return 418;
+  }
+
+  location @gitlab-git-http-server {
     ## If you use HTTPS make sure you disable gzip compression
     ## to be safe against BREACH attack.
     gzip off;
diff --git a/spec/controllers/projects/repositories_controller_spec.rb b/spec/controllers/projects/repositories_controller_spec.rb
index 91856ed0cc0..18a30033ed8 100644
--- a/spec/controllers/projects/repositories_controller_spec.rb
+++ b/spec/controllers/projects/repositories_controller_spec.rb
@@ -33,33 +33,5 @@ describe Projects::RepositoriesController do
         expect(response.status).to eq(404)
       end
     end
-
-    context "when the service doesn't return a path" do
-
-      before do
-        allow(service).to receive(:execute).and_return(nil)
-      end
-
-      it "reloads the page" do
-        get :archive, namespace_id: project.namespace.path, project_id: project.path, ref: "master", format: "zip"
-
-        expect(response).to redirect_to(archive_namespace_project_repository_path(project.namespace, project, ref: "master", format: "zip"))
-      end
-    end
-
-    context "when the service returns a path" do
-
-      let(:path) { Rails.root.join("spec/fixtures/dk.png").to_s }
-
-      before do
-        allow(service).to receive(:execute).and_return(path)
-      end
-
-      it "sends the file" do
-        get :archive, namespace_id: project.namespace.path, project_id: project.path, ref: "master", format: "zip"
-
-        expect(response.body).to eq(File.binread(path))
-      end
-    end
   end
 end
diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb
index 09a79553f72..1149f7e7989 100644
--- a/spec/requests/api/repositories_spec.rb
+++ b/spec/requests/api/repositories_spec.rb
@@ -166,24 +166,21 @@ describe API::API, api: true  do
       get api("/projects/#{project.id}/repository/archive", user)
       repo_name = project.repository.name.gsub("\.git", "")
       expect(response.status).to eq(200)
-      expect(response.headers['Content-Disposition']).to match(/filename\=\"#{repo_name}\-[^\.]+\.tar.gz\"/)
-      expect(response.content_type).to eq(MIME::Types.type_for('file.tar.gz').first.content_type)
+      expect(json_response['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.tar.gz/)
     end
 
     it "should get the archive.zip" do
       get api("/projects/#{project.id}/repository/archive.zip", user)
       repo_name = project.repository.name.gsub("\.git", "")
       expect(response.status).to eq(200)
-      expect(response.headers['Content-Disposition']).to match(/filename\=\"#{repo_name}\-[^\.]+\.zip\"/)
-      expect(response.content_type).to eq(MIME::Types.type_for('file.zip').first.content_type)
+      expect(json_response['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.zip/)
     end
 
     it "should get the archive.tar.bz2" do
       get api("/projects/#{project.id}/repository/archive.tar.bz2", user)
       repo_name = project.repository.name.gsub("\.git", "")
       expect(response.status).to eq(200)
-      expect(response.headers['Content-Disposition']).to match(/filename\=\"#{repo_name}\-[^\.]+\.tar.bz2\"/)
-      expect(response.content_type).to eq(MIME::Types.type_for('file.tar.bz2').first.content_type)
+      expect(json_response['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.tar.bz2/)
     end
 
     it "should return 404 for invalid sha" do
diff --git a/spec/services/archive_repository_service_spec.rb b/spec/services/archive_repository_service_spec.rb
index 0ec70c51b3a..1cc7b240216 100644
--- a/spec/services/archive_repository_service_spec.rb
+++ b/spec/services/archive_repository_service_spec.rb
@@ -13,7 +13,7 @@ describe ArchiveRepositoryService do
 
     context "when the repository doesn't have an archive file path" do
       before do
-        allow(project.repository).to receive(:archive_file_path).and_return(nil)
+        allow(project.repository).to receive(:archive_metadata).and_return(Hash.new)
       end
 
       it "raises an error" do
@@ -21,70 +21,5 @@ describe ArchiveRepositoryService do
       end
     end
 
-    context "when the repository has an archive file path" do
-      let(:file_path)     { "/archive.zip" }
-      let(:pid_file_path) { "/archive.zip.pid" }
-
-      before do
-        allow(project.repository).to receive(:archive_file_path).and_return(file_path)
-        allow(project.repository).to receive(:archive_pid_file_path).and_return(pid_file_path)
-      end
-
-      context "when the archive file already exists" do
-        before do
-          allow(File).to receive(:exist?).with(file_path).and_return(true)
-        end
-
-        it "returns the file path" do
-          expect(subject.execute(timeout: 0.0)).to eq(file_path)
-        end
-      end
-
-      context "when the archive file doesn't exist yet" do
-        before do
-          allow(File).to receive(:exist?).with(file_path).and_return(false)
-          allow(File).to receive(:exist?).with(pid_file_path).and_return(true)
-        end
-
-        context "when the archive pid file doesn't exist yet" do
-          before do
-            allow(File).to receive(:exist?).with(pid_file_path).and_return(false)
-          end
-
-          it "queues the RepositoryArchiveWorker" do
-            expect(RepositoryArchiveWorker).to receive(:perform_async)
-
-            subject.execute(timeout: 0.0)
-          end
-        end
-
-        context "when the archive pid file already exists" do
-          it "doesn't queue the RepositoryArchiveWorker" do
-            expect(RepositoryArchiveWorker).not_to receive(:perform_async)
-
-            subject.execute(timeout: 0.0)
-          end
-        end
-
-        context "when the archive file exists after a little while" do
-          before do
-            Thread.new do
-              sleep 0.1
-              allow(File).to receive(:exist?).with(file_path).and_return(true)
-            end
-          end
-
-          it "returns the file path" do
-            expect(subject.execute(timeout: 0.2)).to eq(file_path)
-          end
-        end
-
-        context "when the archive file doesn't exist after the timeout" do
-          it "returns nil" do
-            expect(subject.execute(timeout: 0.0)).to eq(nil)
-          end
-        end
-      end
-    end
   end
 end
diff --git a/spec/workers/repository_archive_worker_spec.rb b/spec/workers/repository_archive_worker_spec.rb
deleted file mode 100644
index a914d0ac8dc..00000000000
--- a/spec/workers/repository_archive_worker_spec.rb
+++ /dev/null
@@ -1,79 +0,0 @@
-require 'spec_helper'
-
-describe RepositoryArchiveWorker do
-  let(:project) { create(:project) }
-  subject { RepositoryArchiveWorker.new }
-
-  before do
-    allow(Project).to receive(:find).and_return(project)
-  end
-
-  describe "#perform" do
-    it "cleans old archives" do
-      expect(project.repository).to receive(:clean_old_archives)
-
-      subject.perform(project.id, "master", "zip")
-    end
-
-    context "when the repository doesn't have an archive file path" do
-      before do
-        allow(project.repository).to receive(:archive_file_path).and_return(nil)
-      end
-
-      it "doesn't archive the repo" do
-        expect(project.repository).not_to receive(:archive_repo)
-
-        subject.perform(project.id, "master", "zip")
-      end
-    end
-
-    context "when the repository has an archive file path" do
-      let(:file_path)     { "/archive.zip" }
-      let(:pid_file_path) { "/archive.zip.pid" }
-
-      before do
-        allow(project.repository).to receive(:archive_file_path).and_return(file_path)
-        allow(project.repository).to receive(:archive_pid_file_path).and_return(pid_file_path)
-      end
-
-      context "when the archive file already exists" do
-        before do
-          allow(File).to receive(:exist?).with(file_path).and_return(true)
-        end
-
-        it "doesn't archive the repo" do
-          expect(project.repository).not_to receive(:archive_repo)
-
-          subject.perform(project.id, "master", "zip")
-        end
-      end
-
-      context "when the archive file doesn't exist yet" do
-        before do
-          allow(File).to receive(:exist?).with(file_path).and_return(false)
-          allow(File).to receive(:exist?).with(pid_file_path).and_return(true)
-        end
-
-        context "when the archive pid file doesn't exist yet" do
-          before do
-            allow(File).to receive(:exist?).with(pid_file_path).and_return(false)
-          end
-
-          it "archives the repo" do
-            expect(project.repository).to receive(:archive_repo)
-
-            subject.perform(project.id, "master", "zip")
-          end
-        end
-
-        context "when the archive pid file already exists" do
-          it "doesn't archive the repo" do
-            expect(project.repository).not_to receive(:archive_repo)
-
-            subject.perform(project.id, "master", "zip")
-          end
-        end
-      end
-    end
-  end
-end