Merge branch 'atom-routes' into 'master'

Allow browsing branches that end with '.atom'

## What does this MR do?

1. Simplify the regex capture in the routing for the CommitsController
   to not exclude the '.atom' suffix. That's a perfectly valid git
   branch name, so we shouldn't blow up if we get it.
2. Because Rails now can't automatically detect the request format, add
   some code to do so in `ExtractPath` when there is no path. This means
   that, given branches 'foo' and 'foo.atom', the Atom feed for the
   former is unroutable. To fix this: don't do that! Give the branches
   different names!

## Why was this MR needed?

Creating a branch or tag name ending in '.atom' would cause some 500s on that repo.

## What are the relevant issue numbers?

Closes #21955. Related to !5994.

See merge request !6750

3 files changed, 119 insertions, 11 deletions

diff --git a/spec/controllers/projects/commits_controller_spec.rb b/spec/controllers/projects/commits_controller_spec.rb
index 2518a48e336..1ac7e03a2db 100644
--- a/spec/controllers/projects/commits_controller_spec.rb
+++ b/spec/controllers/projects/commits_controller_spec.rb
@@ -10,15 +10,38 @@ describe Projects::CommitsController do
   end
 
   describe "GET show" do
-    context "as atom feed" do
-      it "renders as atom" do
-        get(:show,
-            namespace_id: project.namespace.to_param,
-            project_id: project.to_param,
-            id: "master",
-            format: "atom")
-        expect(response).to be_success
-        expect(response.content_type).to eq('application/atom+xml')
+    context "when the ref name ends in .atom" do
+      render_views
+
+      context "when the ref does not exist with the suffix" do
+        it "renders as atom" do
+          get(:show,
+              namespace_id: project.namespace.to_param,
+              project_id: project.to_param,
+              id: "master.atom")
+
+          expect(response).to be_success
+          expect(response.content_type).to eq('application/atom+xml')
+        end
+      end
+
+      context "when the ref exists with the suffix" do
+        before do
+          commit = project.repository.commit('master')
+
+          allow_any_instance_of(Repository).to receive(:commit).and_call_original
+          allow_any_instance_of(Repository).to receive(:commit).with('master.atom').and_return(commit)
+
+          get(:show,
+              namespace_id: project.namespace.to_param,
+              project_id: project.to_param,
+              id: "master.atom")
+        end
+
+        it "renders as HTML" do
+          expect(response).to be_success
+          expect(response.content_type).to eq('text/html')
+        end
       end
     end
   end
diff --git a/spec/lib/extracts_path_spec.rb b/spec/lib/extracts_path_spec.rb
index e10c1f5c547..0e85e302f29 100644
--- a/spec/lib/extracts_path_spec.rb
+++ b/spec/lib/extracts_path_spec.rb
@@ -6,6 +6,7 @@ describe ExtractsPath, lib: true do
   include Gitlab::Routing.url_helpers
 
   let(:project) { double('project') }
+  let(:request) { double('request') }
 
   before do
     @project = project
@@ -15,9 +16,10 @@ describe ExtractsPath, lib: true do
     allow(project).to receive(:repository).and_return(repo)
     allow(project).to receive(:path_with_namespace).
       and_return('gitlab/gitlab-ci')
+    allow(request).to receive(:format=)
   end
 
-  describe '#assign_ref' do
+  describe '#assign_ref_vars' do
     let(:ref) { sample_commit[:id] }
     let(:params) { { path: sample_commit[:line_code_path], ref: ref } }
 
@@ -61,6 +63,75 @@ describe ExtractsPath, lib: true do
         expect(@id).to eq(get_id)
       end
     end
+
+    context 'ref only exists without .atom suffix' do
+      context 'with a path' do
+        let(:params) { { ref: 'v1.0.0.atom', path: 'README.md' } }
+
+        it 'renders a 404' do
+          expect(self).to receive(:render_404)
+
+          assign_ref_vars
+        end
+      end
+
+      context 'without a path' do
+        let(:params) { { ref: 'v1.0.0.atom' } }
+        before { assign_ref_vars }
+
+        it 'sets the un-suffixed version as @ref' do
+          expect(@ref).to eq('v1.0.0')
+        end
+
+        it 'sets the request format to Atom' do
+          expect(request).to have_received(:format=).with(:atom)
+        end
+      end
+    end
+
+    context 'ref exists with .atom suffix' do
+      context 'with a path' do
+        let(:params) { { ref: 'master.atom', path: 'README.md' } }
+
+        before do
+          repository = @project.repository
+          allow(repository).to receive(:commit).and_call_original
+          allow(repository).to receive(:commit).with('master.atom').and_return(repository.commit('master'))
+
+          assign_ref_vars
+        end
+
+        it 'sets the suffixed version as @ref' do
+          expect(@ref).to eq('master.atom')
+        end
+
+        it 'does not change the request format' do
+          expect(request).not_to have_received(:format=)
+        end
+      end
+
+      context 'without a path' do
+        let(:params) { { ref: 'master.atom' } }
+
+        before do
+          repository = @project.repository
+          allow(repository).to receive(:commit).and_call_original
+          allow(repository).to receive(:commit).with('master.atom').and_return(repository.commit('master'))
+        end
+
+        it 'sets the suffixed version as @ref' do
+          assign_ref_vars
+
+          expect(@ref).to eq('master.atom')
+        end
+
+        it 'does not change the request format' do
+          expect(request).not_to receive(:format=)
+
+          assign_ref_vars
+        end
+      end
+    end
   end
 
   describe '#extract_ref' do
@@ -115,4 +186,18 @@ describe ExtractsPath, lib: true do
       end
     end
   end
+
+  describe '#extract_ref_without_atom' do
+    it 'ignores any matching refs suffixed with atom' do
+      expect(extract_ref_without_atom('master.atom')).to eq('master')
+    end
+
+    it 'returns the longest matching ref' do
+      expect(extract_ref_without_atom('release/app/v1.0.0.atom')).to eq('release/app/v1.0.0')
+    end
+
+    it 'returns nil if there are no matching refs' do
+      expect(extract_ref_without_atom('foo.atom')).to eq(nil)
+    end
+  end
 end
diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb
index 77842057a10..2322430d212 100644
--- a/spec/routing/project_routing_spec.rb
+++ b/spec/routing/project_routing_spec.rb
@@ -337,7 +337,7 @@ describe Projects::CommitsController, 'routing' do
   end
 
   it 'to #show' do
-    expect(get('/gitlab/gitlabhq/commits/master.atom')).to route_to('projects/commits#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master', format: 'atom')
+    expect(get('/gitlab/gitlabhq/commits/master.atom')).to route_to('projects/commits#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master.atom')
   end
 end