Fix blame view line groupings

Closes #2305

3 files changed, 86 insertions, 19 deletions

diff --git a/app/controllers/projects/blame_controller.rb b/app/controllers/projects/blame_controller.rb
index 45e157c90cb..9ea518e6c85 100644
--- a/app/controllers/projects/blame_controller.rb
+++ b/app/controllers/projects/blame_controller.rb
@@ -8,6 +8,28 @@ class Projects::BlameController < Projects::ApplicationController
 
   def show
     @blob = @repository.blob_at(@commit.id, @path)
-    @blame = Gitlab::Git::Blame.new(@repository, @commit.id, @path)
+    @blame = group_blame_lines
+  end
+
+  def group_blame_lines
+    blame = Gitlab::Git::Blame.new(@repository, @commit.id, @path)
+
+    prev_sha = nil
+    groups = []
+    current_group = nil
+
+    blame.each do |commit, line|
+      if prev_sha && prev_sha == commit.sha
+        current_group[:lines] << line
+      else
+        groups << current_group if current_group.present?
+        current_group = { commit: commit, lines: [line] }
+      end
+
+      prev_sha = commit.sha
+    end
+
+    groups << current_group if current_group.present?
+    groups
   end
 end
diff --git a/app/views/projects/blame/show.html.haml b/app/views/projects/blame/show.html.haml
index 05d5db5d3fe..a3ff7ce2f1f 100644
--- a/app/views/projects/blame/show.html.haml
+++ b/app/views/projects/blame/show.html.haml
@@ -13,30 +13,32 @@
     .file-content.blame.highlight
       %table
         - current_line = 1
-        - @blame.each do |raw_commit, line|
+        - @blame.each do |blame_group|
           %tr
             %td.blame-commit
               .commit
-                - unless @prev_commit && @prev_commit.sha == raw_commit.sha
-                  - commit = Commit.new(raw_commit, @project)
-                  .commit-row-title
-                    %strong
-                      = link_to_gfm truncate(commit.title, length: 35), namespace_project_commit_path(@project.namespace, @project, commit.id), class: "cdark"
-                    .pull-right
-                      = link_to commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit), class: "monospace"
-                    &nbsp;
-                  .light
-                    = commit_author_link(commit, avatar: false)
-                    authored
-                    #{time_ago_with_tooltip(commit.committed_date)}
-                - @prev_commit = raw_commit
+                - commit = Commit.new(blame_group[:commit], @project)
+                .commit-row-title
+                  %strong
+                    = link_to_gfm truncate(commit.title, length: 35), namespace_project_commit_path(@project.namespace, @project, commit.id), class: "cdark"
+                  .pull-right
+                    = link_to commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit), class: "monospace"
+                  &nbsp;
+                .light
+                  = commit_author_link(commit, avatar: false)
+                  authored
+                  #{time_ago_with_tooltip(commit.committed_date)}
             %td.lines.blame-numbers
               %pre
-                = current_line
-                - current_line += 1
+                - line_count = blame_group[:lines].count
+                - (current_line...(current_line + line_count)).each do |i|
+                  = i
+                  \
+                - current_line += line_count
             %td.lines
               %pre{class: 'code highlight white'}
                 %code
-                  :erb
-                    <%= highlight(@blob.name, line, nowrap: true, continue: true).html_safe %>
+                  - blame_group[:lines].each do |line|
+                    :erb
+                      <%= highlight(@blob.name, line, nowrap: true, continue: true).html_safe %>
 
diff --git a/spec/controllers/blame_controller_spec.rb b/spec/controllers/blame_controller_spec.rb
new file mode 100644
index 00000000000..3ad4d5fc0a8
--- /dev/null
+++ b/spec/controllers/blame_controller_spec.rb
@@ -0,0 +1,43 @@
+require 'spec_helper'
+
+describe Projects::BlameController do
+  let(:project) { create(:project) }
+  let(:user)    { create(:user) }
+
+  before do
+    sign_in(user)
+
+    project.team << [user, :master]
+    controller.instance_variable_set(:@project, project)
+  end
+
+  describe "GET show" do
+    render_views
+
+    before do
+      get(:show,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          id: id)
+    end
+
+    context "valid file" do
+      let(:id) { 'master/files/ruby/popen.rb' }
+      it { is_expected.to respond_with(:success) }
+
+      it 'groups blames properly' do
+        blame = assigns(:blame)
+        # Sanity check a few items
+        expect(blame.count).to eq(18)
+        expect(blame[0][:commit].sha).to eq('913c66a37b4a45b9769037c55c2d238bd0942d2e')
+        expect(blame[0][:lines]).to eq(["require 'fileutils'", "require 'open3'", ""])
+
+        expect(blame[1][:commit].sha).to eq('874797c3a73b60d2187ed6e2fcabd289ff75171e')
+        expect(blame[1][:lines]).to eq(["module Popen", "  extend self"])
+
+        expect(blame[-1][:commit].sha).to eq('913c66a37b4a45b9769037c55c2d238bd0942d2e')
+        expect(blame[-1][:lines]).to eq(["  end", "end"])
+      end
+    end
+  end
+end