Merge branch 'jramsay-38830-tarball' into 'master'

Add alternative archive route

Closes #38830

See merge request gitlab-org/gitlab-ce!17225

app/controllers/projects/repositories_controller.rb

 class Projects::RepositoriesController < Projects::ApplicationController
+  include ExtractsPath
+
   # Authorize
   before_action :require_non_empty_project, except: :create
+  before_action :assign_archive_vars, only: :archive
   before_action :authorize_download_code!
   before_action :authorize_admin_project!, only: :create
 
@@ -11,9 +14,21 @@ class Projects::RepositoriesController < Projects::ApplicationController
   end
 
   def archive
-    send_git_archive @repository, ref: params[:ref], format: params[:format]
+    append_sha = params[:append_sha]
+
+    shortname = "#{@project.path}-#{@ref.tr('/', '-')}"
+    append_sha = false if @filename == shortname
+
+    send_git_archive @repository, ref: @ref, format: params[:format], append_sha: append_sha
   rescue => ex
     logger.error("#{self.class.name}: #{ex}")
     return git_not_found!
   end
+
+  def assign_archive_vars
+    @id = params[:id]
+    @ref, @filename = extract_ref(@id)
+  rescue InvalidPathError
+    render_404
+  end
 end

app/helpers/workhorse_helper.rb

@@ -24,8 +24,8 @@ module WorkhorseHelper
   end
 
   # Archive a Git repository and send it through Workhorse
-  def send_git_archive(repository, ref:, format:)
-    headers.store(*Gitlab::Workhorse.send_git_archive(repository, ref: ref, format: format))
+  def send_git_archive(repository, **kwargs)
+    headers.store(*Gitlab::Workhorse.send_git_archive(repository, **kwargs))
     head :ok
   end
 

app/views/projects/buttons/_download.html.haml

 - pipeline = local_assigns.fetch(:pipeline) { project.latest_successful_pipeline_for(ref) }
 
 - if !project.empty_repo? && can?(current_user, :download_code, project)
+  - archive_prefix = "#{project.path}-#{ref.tr('/', '-')}"
   .project-action-button.dropdown.inline>
     %button.btn.has-tooltip{ title: s_('DownloadSource|Download'), 'data-toggle' => 'dropdown', 'aria-label' => s_('DownloadSource|Download') }
       = sprite_icon('download')
@@ -10,16 +11,16 @@
       %li.dropdown-header
         #{ _('Source code') }
       %li
-        = link_to archive_project_repository_path(project, ref: ref, format: 'zip'), rel: 'nofollow', download: '' do
+        = link_to project_archive_path(project, id: tree_join(ref, archive_prefix), format: 'zip'), rel: 'nofollow', download: '' do
           %span=  _('Download zip')
       %li
-        = link_to archive_project_repository_path(project, ref: ref, format: 'tar.gz'), rel: 'nofollow', download: '' do
+        = link_to project_archive_path(project, id: tree_join(ref, archive_prefix), format: 'tar.gz'), rel: 'nofollow', download: '' do
           %span= _('Download tar.gz')
       %li
-        = link_to archive_project_repository_path(project, ref: ref, format: 'tar.bz2'), rel: 'nofollow', download: '' do
+        = link_to project_archive_path(project, id: tree_join(ref, archive_prefix), format: 'tar.bz2'), rel: 'nofollow', download: '' do
           %span= _('Download tar.bz2')
       %li
-        = link_to archive_project_repository_path(project, ref: ref, format: 'tar'), rel: 'nofollow', download: '' do
+        = link_to project_archive_path(project, id: tree_join(ref, archive_prefix), format: 'tar'), rel: 'nofollow', download: '' do
           %span= _('Download tar')
 
       - if pipeline && pipeline.latest_builds_with_artifacts.any?

changelogs/unreleased/jramsay-38830-tarball.yml

+---
+title: Add alternate archive route for simplified packaging
+merge_request: 17225
+author:
+type: added

config/routes/project.rb

@@ -249,6 +249,8 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       end
 
       scope '-' do
+        get 'archive/*id', constraints: { format: Gitlab::PathRegex.archive_formats_regex, id: /.+?/ }, to: 'repositories#archive', as: 'archive'
+
         resources :jobs, only: [:index, :show], constraints: { id: /\d+/ } do
           collection do
             post :cancel_all

config/routes/repository.rb

@@ -2,10 +2,11 @@
 
 resource :repository, only: [:create] do
   member do
-    get ':ref/archive', constraints: { format: Gitlab::PathRegex.archive_formats_regex, ref: /.+/ }, action: 'archive', as: 'archive'
-
     # deprecated since GitLab 9.5
-    get 'archive', constraints: { format: Gitlab::PathRegex.archive_formats_regex }, as: 'archive_alternative'
+    get 'archive', constraints: { format: Gitlab::PathRegex.archive_formats_regex }, as: 'archive_alternative', defaults: { append_sha: true }
+
+    # deprecated since GitLab 10.7
+    get ':id/archive', constraints: { format: Gitlab::PathRegex.archive_formats_regex, id: /.+/ }, action: 'archive', as: 'archive_deprecated', defaults: { append_sha: true }
   end
 end
 

lib/api/helpers.rb

@@ -468,8 +468,8 @@ module API
       header(*Gitlab::Workhorse.send_git_blob(repository, blob))
     end
 
-    def send_git_archive(repository, ref:, format:)
-      header(*Gitlab::Workhorse.send_git_archive(repository, ref: ref, format: format))
+    def send_git_archive(repository, **kwargs)
+      header(*Gitlab::Workhorse.send_git_archive(repository, **kwargs))
     end
 
     def send_artifacts_entry(build, entry)

lib/api/repositories.rb

@@ -88,7 +88,7 @@ module API
       end
       get ':id/repository/archive', requirements: { format: Gitlab::PathRegex.archive_formats_regex } do
         begin
-          send_git_archive user_project.repository, ref: params[:sha], format: params[:format]
+          send_git_archive user_project.repository, ref: params[:sha], format: params[:format], append_sha: true
         rescue
           not_found!('File')
         end

lib/api/v3/repositories.rb

@@ -75,7 +75,7 @@ module API
         end
         get ':id/repository/archive', requirements: { format: Gitlab::PathRegex.archive_formats_regex } do
           begin
-            send_git_archive user_project.repository, ref: params[:sha], format: params[:format]
+            send_git_archive user_project.repository, ref: params[:sha], format: params[:format], append_sha: true
           rescue
             not_found!('File')
           end

lib/gitlab/git/repository.rb

@@ -394,17 +394,24 @@ module Gitlab
         nil
       end
 
-      def archive_prefix(ref, sha)
+      def archive_prefix(ref, sha, append_sha:)
+        append_sha = (ref != sha) if append_sha.nil?
+
         project_name = self.name.chomp('.git')
-        "#{project_name}-#{ref.tr('/', '-')}-#{sha}"
+        formatted_ref = ref.tr('/', '-')
+
+        prefix_segments = [project_name, formatted_ref]
+        prefix_segments << sha if append_sha
+
+        prefix_segments.join('-')
       end
 
-      def archive_metadata(ref, storage_path, format = "tar.gz")
+      def archive_metadata(ref, storage_path, format = "tar.gz", append_sha:)
         ref ||= root_ref
         commit = Gitlab::Git::Commit.find(self, ref)
         return {} if commit.nil?
 
-        prefix = archive_prefix(ref, commit.id)
+        prefix = archive_prefix(ref, commit.id, append_sha: append_sha)
 
         {
           'RepoPath' => path,

lib/gitlab/workhorse.rb

@@ -59,10 +59,10 @@ module Gitlab
         ]
       end
 
-      def send_git_archive(repository, ref:, format:)
+      def send_git_archive(repository, ref:, format:, append_sha:)
         format ||= 'tar.gz'
         format.downcase!
-        params = repository.archive_metadata(ref, Gitlab.config.gitlab.repository_downloads_path, format)
+        params = repository.archive_metadata(ref, Gitlab.config.gitlab.repository_downloads_path, format, append_sha: append_sha)
         raise "Repository or ref not found" if params.empty?
 
         if Gitlab::GitalyClient.feature_enabled?(:workhorse_archive, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT)

spec/controllers/projects/repositories_controller_spec.rb

@@ -6,7 +6,7 @@ describe Projects::RepositoriesController do
   describe "GET archive" do
     context 'as a guest' do
       it 'responds with redirect in correct format' do
-        get :archive, namespace_id: project.namespace, project_id: project, format: "zip", ref: 'master'
+        get :archive, namespace_id: project.namespace, project_id: project, id: "master", format: "zip"
 
         expect(response.header["Content-Type"]).to start_with('text/html')
         expect(response).to be_redirect
@@ -22,18 +22,25 @@ describe Projects::RepositoriesController do
       end
 
       it "uses Gitlab::Workhorse" do
-        get :archive, namespace_id: project.namespace, project_id: project, ref: "master", format: "zip"
+        get :archive, namespace_id: project.namespace, project_id: project, id: "master", format: "zip"
 
         expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-archive:")
       end
 
+      it 'responds with redirect to the short name archive if fully qualified' do
+        get :archive, namespace_id: project.namespace, project_id: project, id: "master/#{project.path}-master", format: "zip"
+
+        expect(assigns(:ref)).to eq("master")
+        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-archive:")
+      end
+
       context "when the service raises an error" do
         before do
           allow(Gitlab::Workhorse).to receive(:send_git_archive).and_raise("Archive failed")
         end
 
         it "renders Not Found" do
-          get :archive, namespace_id: project.namespace, project_id: project, ref: "master", format: "zip"
+          get :archive, namespace_id: project.namespace, project_id: project, id: "master", format: "zip"
 
           expect(response).to have_gitlab_http_status(404)
         end

spec/lib/gitlab/git/repository_spec.rb

@@ -247,38 +247,44 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     it 'returns parameterised string for a ref containing slashes' do
-      prefix = repository.archive_prefix('test/branch', 'SHA')
+      prefix = repository.archive_prefix('test/branch', 'SHA', append_sha: nil)
 
       expect(prefix).to eq("#{project_name}-test-branch-SHA")
     end
 
     it 'returns correct string for a ref containing dots' do
-      prefix = repository.archive_prefix('test.branch', 'SHA')
+      prefix = repository.archive_prefix('test.branch', 'SHA', append_sha: nil)
 
       expect(prefix).to eq("#{project_name}-test.branch-SHA")
     end
+
+    it 'returns string with sha when append_sha is false' do
+      prefix = repository.archive_prefix('test.branch', 'SHA', append_sha: false)
+
+      expect(prefix).to eq("#{project_name}-test.branch")
+    end
   end
 
   describe '#archive' do
-    let(:metadata) { repository.archive_metadata('master', '/tmp') }
+    let(:metadata) { repository.archive_metadata('master', '/tmp', append_sha: true) }
 
     it_should_behave_like 'archive check', '.tar.gz'
   end
 
   describe '#archive_zip' do
-    let(:metadata) { repository.archive_metadata('master', '/tmp', 'zip') }
+    let(:metadata) { repository.archive_metadata('master', '/tmp', 'zip', append_sha: true) }
 
     it_should_behave_like 'archive check', '.zip'
   end
 
   describe '#archive_bz2' do
-    let(:metadata) { repository.archive_metadata('master', '/tmp', 'tbz2') }
+    let(:metadata) { repository.archive_metadata('master', '/tmp', 'tbz2', append_sha: true) }
 
     it_should_behave_like 'archive check', '.tar.bz2'
   end
 
   describe '#archive_fallback' do
-    let(:metadata) { repository.archive_metadata('master', '/tmp', 'madeup') }
+    let(:metadata) { repository.archive_metadata('master', '/tmp', 'madeup', append_sha: true) }
 
     it_should_behave_like 'archive check', '.tar.gz'
   end

spec/lib/gitlab/workhorse_spec.rb

@@ -16,7 +16,7 @@ describe Gitlab::Workhorse do
     let(:ref) { 'master' }
     let(:format) { 'zip' }
     let(:storage_path) { Gitlab.config.gitlab.repository_downloads_path }
-    let(:base_params) { repository.archive_metadata(ref, storage_path, format) }
+    let(:base_params) { repository.archive_metadata(ref, storage_path, format, append_sha: nil) }
     let(:gitaly_params) do
       base_params.merge(
         'GitalyServer' => {
@@ -29,7 +29,7 @@ describe Gitlab::Workhorse do
     let(:cache_disabled) { false }
 
     subject do
-      described_class.send_git_archive(repository, ref: ref, format: format)
+      described_class.send_git_archive(repository, ref: ref, format: format, append_sha: nil)
     end
 
     before do

spec/routing/project_routing_spec.rb

@@ -164,20 +164,36 @@ describe 'project routing' do
   #  archive_project_repository GET    /:project_id/repository/archive(.:format)  projects/repositories#archive
   #     edit_project_repository GET    /:project_id/repository/edit(.:format)     projects/repositories#edit
   describe Projects::RepositoriesController, 'routing' do
-    it 'to #archive' do
-      expect(get('/gitlab/gitlabhq/repository/master/archive')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', ref: 'master')
-    end
-
     it 'to #archive format:zip' do
-      expect(get('/gitlab/gitlabhq/repository/master/archive.zip')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'zip', ref: 'master')
+      expect(get('/gitlab/gitlabhq/-/archive/master/archive.zip')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'zip', id: 'master/archive')
     end
 
     it 'to #archive format:tar.bz2' do
-      expect(get('/gitlab/gitlabhq/repository/master/archive.tar.bz2')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'tar.bz2', ref: 'master')
+      expect(get('/gitlab/gitlabhq/-/archive/master/archive.tar.bz2')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'tar.bz2', id: 'master/archive')
     end
 
     it 'to #archive with "/" in route' do
-      expect(get('/gitlab/gitlabhq/repository/improve/awesome/archive')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', ref: 'improve/awesome')
+      expect(get('/gitlab/gitlabhq/-/archive/improve/awesome/gitlabhq-improve-awesome.tar.gz')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'tar.gz', id: 'improve/awesome/gitlabhq-improve-awesome')
+    end
+
+    it 'to #archive_alternative' do
+      expect(get('/gitlab/gitlabhq/repository/archive')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', append_sha: true)
+    end
+
+    it 'to #archive_deprecated' do
+      expect(get('/gitlab/gitlabhq/repository/master/archive')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master', append_sha: true)
+    end
+
+    it 'to #archive_deprecated format:zip' do
+      expect(get('/gitlab/gitlabhq/repository/master/archive.zip')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'zip', id: 'master', append_sha: true)
+    end
+
+    it 'to #archive_deprecated format:tar.bz2' do
+      expect(get('/gitlab/gitlabhq/repository/master/archive.tar.bz2')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'tar.bz2', id: 'master', append_sha: true)
+    end
+
+    it 'to #archive_deprecated with "/" in route' do
+      expect(get('/gitlab/gitlabhq/repository/improve/awesome/archive')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'improve/awesome', append_sha: true)
     end
   end