Allow browsing branches that end with '.atom'

CHANGELOG

@@ -14,6 +14,7 @@ v 8.13.0 (unreleased)
   - Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs)
   - Add tag shortcut from the Commit page. !6543
   - Keep refs for each deployment
+  - Allow browsing branches that end with '.atom'
   - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller)
   - Add more tests for calendar contribution (ClemMakesApps)
   - Cache rendered markdown in the database, rather than Redis

config/routes/project.rb

@@ -159,7 +159,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only:
         get(
           '/commits/*id',
           to: 'commits#show',
-          constraints: { id: /(?:[^.]|\.(?!atom$))+/, format: /atom/ },
+          constraints: { id: /.+/, format: false },
           as: :commits
         )
       end

lib/extracts_path.rb

@@ -52,8 +52,7 @@ module ExtractsPath
       # Append a trailing slash if we only get a ref and no file path
       id += '/' unless id.ends_with?('/')
 
-      valid_refs = @project.repository.ref_names
-      valid_refs.select! { |v| id.start_with?("#{v}/") }
+      valid_refs = ref_names.select { |v| id.start_with?("#{v}/") }
 
       if valid_refs.length == 0
         # No exact ref match, so just try our best
@@ -74,6 +73,19 @@ module ExtractsPath
     pair
   end
 
+  # If we have an ID of 'foo.atom', and the controller provides Atom and HTML
+  # formats, then we have to check if the request was for the Atom version of
+  # the ID without the '.atom' suffix, or the HTML version of the ID including
+  # the suffix. We only check this if the version including the suffix doesn't
+  # match, so it is possible to create a branch which has an unroutable Atom
+  # feed.
+  def extract_ref_without_atom(id)
+    id_without_atom = id.sub(/\.atom$/, '')
+    valid_refs = ref_names.select { |v| "#{id_without_atom}/".start_with?("#{v}/") }
+
+    valid_refs.max_by(&:length)
+  end
+
   # Assigns common instance variables for views working with Git tree-ish objects
   #
   # Assignments are:
@@ -86,6 +98,10 @@ module ExtractsPath
   # If the :id parameter appears to be requesting a specific response format,
   # that will be handled as well.
   #
+  # If there is no path and the ref doesn't exist in the repo, try to resolve
+  # the ref without an '.atom' suffix. If _that_ ref is found, set the request's
+  # format to Atom manually.
+  #
   # Automatically renders `not_found!` if a valid tree path could not be
   # resolved (e.g., when a user inserts an invalid path or ref).
   def assign_ref_vars
@@ -103,6 +119,13 @@ module ExtractsPath
       @commit = @repo.commit(@options[:extended_sha1])
     end
 
+    if @path.empty? && !@commit
+      @id = @ref = extract_ref_without_atom(@id)
+      @commit = @repo.commit(@ref)
+
+      request.format = :atom if @commit
+    end
+
     raise InvalidPathError unless @commit
 
     @hex_path = Digest::SHA1.hexdigest(@path)
@@ -125,4 +148,10 @@ module ExtractsPath
     id += "/" + params[:path] unless params[:path].blank?
     id
   end
+
+  def ref_names
+    return [] unless @project
+
+    @ref_names ||= @project.repository.ref_names
+  end
 end

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

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

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