Merge branch '225174-paginated-git-blame' into 'master'

Plumbing to allow blames to be limited or paginated

See merge request gitlab-org/gitlab!84388

Gemfile

@@ -482,7 +482,7 @@ gem 'ssh_data', '~> 1.2'
 gem 'spamcheck', '~> 0.1.0'
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 14.9.0-rc5'
+gem 'gitaly', '~> 14.10.0-rc1'
 
 # KAS GRPC protocol definitions
 gem 'kas-grpc', '~> 0.0.2'

Gemfile.lock

@@ -455,7 +455,7 @@ GEM
       rails (>= 3.2.0)
     git (1.7.0)
       rchardet (~> 1.8)
-    gitaly (14.9.0.pre.rc5)
+    gitaly (14.10.0.pre.rc1)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab (4.16.1)
@@ -1494,7 +1494,7 @@ DEPENDENCIES
   gettext (~> 3.3)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 14.9.0.pre.rc5)
+  gitaly (~> 14.10.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-dangerfiles (~> 3.0)

app/presenters/gitlab/blame_presenter.rb

@@ -28,6 +28,10 @@ module Gitlab
       precalculate_data_by_commit!
     end
 
+    def first_line
+      blame.first_line
+    end
+
     def groups
       @groups ||= blame.groups
     end

app/views/projects/blame/show.html.haml

@@ -22,7 +22,7 @@
 
     .table-responsive.file-content.blame.code{ class: user_color_scheme }
       %table
-        - current_line = 1
+        - current_line = @blame.first_line
         - @blame.groups.each do |blame_group|
           - commit_data = @blame.commit_data(blame_group[:commit])
           - line_count = blame_group[:lines].count

lib/gitlab/blame.rb

@@ -2,11 +2,16 @@
 
 module Gitlab
   class Blame
-    attr_accessor :blob, :commit
+    attr_accessor :blob, :commit, :range
 
-    def initialize(blob, commit)
+    def initialize(blob, commit, range: nil)
       @blob = blob
       @commit = commit
+      @range = range
+    end
+
+    def first_line
+      range&.first || 1
     end
 
     def groups(highlight: true)
@@ -14,7 +19,7 @@ module Gitlab
       groups = []
       current_group = nil
 
-      i = 0
+      i = first_line - 1
       blame.each do |commit, line, previous_path|
         commit = Commit.new(commit, project)
         commit.lazy_author # preload author
@@ -37,7 +42,7 @@ module Gitlab
     private
 
     def blame
-      @blame ||= Gitlab::Git::Blame.new(repository, @commit.id, @blob.path)
+      @blame ||= Gitlab::Git::Blame.new(repository, @commit.id, @blob.path, range: range)
     end
 
     def highlighted_lines

lib/gitlab/git/blame.rb

@@ -5,12 +5,13 @@ module Gitlab
     class Blame
       include Gitlab::EncodingHelper
 
-      attr_reader :lines, :blames
+      attr_reader :lines, :blames, :range
 
-      def initialize(repository, sha, path)
+      def initialize(repository, sha, path, range: nil)
         @repo = repository
         @sha = sha
         @path = path
+        @range = range
         @lines = []
         @blames = load_blame
       end
@@ -23,13 +24,20 @@ module Gitlab
 
       private
 
+      def range_spec
+        "#{range.first},#{range.last}" if range
+      end
+
       def load_blame
-        output = encode_utf8(@repo.gitaly_commit_client.raw_blame(@sha, @path))
+        output = encode_utf8(
+          @repo.gitaly_commit_client.raw_blame(@sha, @path, range: range_spec)
+        )
 
         process_raw_blame(output)
       end
 
       def process_raw_blame(output)
+        start_line = nil
         lines = []
         final = []
         info = {}
@@ -47,6 +55,10 @@ module Gitlab
             commit_id = m[1]
             commits[commit_id] = nil unless commits.key?(commit_id)
             info[m[3].to_i] = [commit_id, m[2].to_i]
+
+            # Assumption: the first line returned by git blame is lowest-numbered
+            # This is true unless we start passing it `--incremental`.
+            start_line = m[3].to_i if start_line.nil?
           elsif line.start_with?("previous ")
             # previous 1485b69e7b839a21436e81be6d3aa70def5ed341 initial-commit
             # previous 9521e52704ee6100e7d2a76896a4ef0eb53ff1b8 "\303\2511\\\303\251\\303\\251\n"
@@ -61,7 +73,13 @@ module Gitlab
 
         # get it together
         info.sort.each do |lineno, (commit_id, old_lineno)|
-          final << BlameLine.new(lineno, old_lineno, commits[commit_id], lines[lineno - 1], previous_paths[commit_id])
+          final << BlameLine.new(
+            lineno,
+            old_lineno,
+            commits[commit_id],
+            lines[lineno - start_line],
+            previous_paths[commit_id]
+          )
         end
 
         @lines = final

lib/gitlab/gitaly_client/commit_service.rb

@@ -315,11 +315,12 @@ module Gitlab
         response.languages.map { |l| { value: l.share.round(2), label: l.name, color: l.color, highlight: l.color } }
       end
 
-      def raw_blame(revision, path)
+      def raw_blame(revision, path, range:)
         request = Gitaly::RawBlameRequest.new(
           repository: @gitaly_repo,
           revision: encode_binary(revision),
-          path: encode_binary(path)
+          path: encode_binary(path),
+          range: (encode_binary(range) if range)
         )
 
         response = GitalyClient.call(@repository.storage, :commit_service, :raw_blame, request, timeout: GitalyClient.medium_timeout)

spec/lib/gitlab/blame_spec.rb

@@ -3,13 +3,31 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Blame do
-  let(:project) { create(:project, :repository) }
+  let_it_be(:project) { create(:project, :repository) }
+
   let(:path) { 'files/ruby/popen.rb' }
   let(:commit) { project.commit('master') }
   let(:blob) { project.repository.blob_at(commit.id, path) }
+  let(:range) { nil }
+
+  subject(:blame) { described_class.new(blob, commit, range: range) }
+
+  describe '#first_line' do
+    subject { blame.first_line }
+
+    it { is_expected.to eq(1) }
+
+    context 'with a range' do
+      let(:range) { 2..3 }
+
+      it { is_expected.to eq(range.first) }
+    end
+  end
 
   describe "#groups" do
-    let(:subject) { described_class.new(blob, commit).groups(highlight: false) }
+    let(:highlighted) { false }
+
+    subject(:groups) { blame.groups(highlight: highlighted) }
 
     it 'groups lines properly' do
       expect(subject.count).to eq(18)
@@ -23,6 +41,50 @@ RSpec.describe Gitlab::Blame do
       expect(subject[-1][:lines]).to eq(["  end", "end"])
     end
 
+    context 'with a range 1..5' do
+      let(:range) { 1..5 }
+
+      it 'returns the correct lines' do
+        expect(groups.count).to eq(2)
+        expect(groups[0][:lines]).to eq(["require 'fileutils'", "require 'open3'", ""])
+        expect(groups[1][:lines]).to eq(['module Popen', '  extend self'])
+      end
+
+      context 'with highlighted lines' do
+        let(:highlighted) { true }
+
+        it 'returns the correct lines' do
+          expect(groups.count).to eq(2)
+          expect(groups[0][:lines][0]).to match(/LC1.*fileutils/)
+          expect(groups[0][:lines][1]).to match(/LC2.*open3/)
+          expect(groups[0][:lines][2]).to eq("<span id=\"LC3\" class=\"line\" lang=\"ruby\"></span>\n")
+          expect(groups[1][:lines][0]).to match(/LC4.*Popen/)
+          expect(groups[1][:lines][1]).to match(/LC5.*extend/)
+        end
+      end
+    end
+
+    context 'with a range 2..4' do
+      let(:range) { 2..4 }
+
+      it 'returns the correct lines' do
+        expect(groups.count).to eq(2)
+        expect(groups[0][:lines]).to eq(["require 'open3'", ""])
+        expect(groups[1][:lines]).to eq(['module Popen'])
+      end
+
+      context 'with highlighted lines' do
+        let(:highlighted) { true }
+
+        it 'returns the correct lines' do
+          expect(groups.count).to eq(2)
+          expect(groups[0][:lines][0]).to match(/LC2.*open3/)
+          expect(groups[0][:lines][1]).to eq("<span id=\"LC3\" class=\"line\" lang=\"ruby\"></span>\n")
+          expect(groups[1][:lines][0]).to match(/LC4.*Popen/)
+        end
+      end
+    end
+
     context 'renamed file' do
       let(:path) { 'files/plain_text/renamed' }
       let(:commit) { project.commit('blame-on-renamed') }

spec/lib/gitlab/git/blame_spec.rb

@@ -4,106 +4,81 @@ require "spec_helper"
 
 RSpec.describe Gitlab::Git::Blame, :seed_helper do
   let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '', 'group/project') }
-  let(:blame) do
-    Gitlab::Git::Blame.new(repository, SeedRepo::Commit::ID, "CONTRIBUTING.md")
+
+  let(:sha) { SeedRepo::Commit::ID }
+  let(:path) { 'CONTRIBUTING.md' }
+  let(:range) { nil }
+
+  subject(:blame) { Gitlab::Git::Blame.new(repository, sha, path, range: range) }
+
+  let(:result) do
+    [].tap do |data|
+      blame.each do |commit, line, previous_path|
+        data << { commit: commit, line: line, previous_path: previous_path }
+      end
+    end
   end
 
   describe 'blaming a file' do
-    context "each count" do
-      it do
-        data = []
-        blame.each do |commit, line|
-          data << {
-            commit: commit,
-            line: line
-          }
-        end
+    it 'has the right number of lines' do
+      expect(result.size).to eq(95)
+      expect(result.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
+      expect(result.first[:line]).to eq("# Contribute to GitLab")
+      expect(result.first[:line]).to be_utf8
+    end
+
+    context 'blaming a range' do
+      let(:range) { 2..4 }
 
-        expect(data.size).to eq(95)
-        expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
-        expect(data.first[:line]).to eq("# Contribute to GitLab")
-        expect(data.first[:line]).to be_utf8
+      it 'only returns the range' do
+        expect(result.size).to eq(range.size)
+        expect(result.map {|r| r[:line] }).to eq(['', 'This guide details how contribute to GitLab.', ''])
       end
     end
 
     context "ISO-8859 encoding" do
-      let(:blame) do
-        Gitlab::Git::Blame.new(repository, SeedRepo::EncodingCommit::ID, "encoding/iso8859.txt")
-      end
+      let(:sha) { SeedRepo::EncodingCommit::ID }
+      let(:path) { 'encoding/iso8859.txt' }
 
       it 'converts to UTF-8' do
-        data = []
-        blame.each do |commit, line|
-          data << {
-            commit: commit,
-            line: line
-          }
-        end
-
-        expect(data.size).to eq(1)
-        expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
-        expect(data.first[:line]).to eq("Ä ü")
-        expect(data.first[:line]).to be_utf8
+        expect(result.size).to eq(1)
+        expect(result.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
+        expect(result.first[:line]).to eq("Ä ü")
+        expect(result.first[:line]).to be_utf8
       end
     end
 
     context "unknown encoding" do
-      let(:blame) do
-        Gitlab::Git::Blame.new(repository, SeedRepo::EncodingCommit::ID, "encoding/iso8859.txt")
-      end
+      let(:sha) { SeedRepo::EncodingCommit::ID }
+      let(:path) { 'encoding/iso8859.txt' }
 
       it 'converts to UTF-8' do
         expect_next_instance_of(CharlockHolmes::EncodingDetector) do |detector|
           expect(detector).to receive(:detect).and_return(nil)
         end
 
-        data = []
-        blame.each do |commit, line|
-          data << {
-            commit: commit,
-            line: line
-          }
-        end
-
-        expect(data.size).to eq(1)
-        expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
-        expect(data.first[:line]).to eq(" ")
-        expect(data.first[:line]).to be_utf8
+        expect(result.size).to eq(1)
+        expect(result.first[:commit]).to be_kind_of(Gitlab::Git::Commit)
+        expect(result.first[:line]).to eq(" ")
+        expect(result.first[:line]).to be_utf8
       end
     end
 
     context "renamed file" do
       let(:project) { create(:project, :repository) }
+      let(:repository) { project.repository.raw_repository }
       let(:commit) { project.commit('blame-on-renamed') }
+      let(:sha) { commit.id }
       let(:path) { 'files/plain_text/renamed' }
 
-      let(:blame) { described_class.new(project.repository, commit.id, path) }
-
-      it do
-        data = []
-        blame.each do |commit, line, previous_path|
-          data << {
-            commit: commit,
-            line: line,
-            previous_path: previous_path
-          }
-        end
-
-        expect(data.size).to eq(5)
-
-        expect(data[0][:line]).to eq('Initial commit')
-        expect(data[0][:previous_path]).to be nil
-        expect(data[1][:line]).to eq('Initial commit')
-        expect(data[1][:previous_path]).to be nil
-
-        expect(data[2][:line]).to eq('Renamed as "filename"')
-        expect(data[2][:previous_path]).to eq('files/plain_text/initial-commit')
-
-        expect(data[3][:line]).to eq('Renamed as renamed')
-        expect(data[3][:previous_path]).to eq('files/plain_text/"filename"')
+      it 'includes the previous path' do
+        expect(result.size).to eq(5)
 
-        expect(data[4][:line]).to eq('Last edit, no rename')
-        expect(data[4][:previous_path]).to eq('files/plain_text/renamed')
+        expect(result[0]).to include(line: 'Initial commit', previous_path: nil)
+        expect(result[1]).to include(line: 'Initial commit', previous_path: nil)
+        expect(result[2]).to include(line: 'Renamed as "filename"', previous_path: 'files/plain_text/initial-commit')
+        expect(result[3]).to include(line: 'Renamed as renamed', previous_path: 'files/plain_text/"filename"')
+        expect(result[4]).to include(line: 'Last edit, no rename', previous_path: path)
       end
     end
   end

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -563,4 +563,39 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
       expect(response).not_to have_key 'nonexistent'
     end
   end
+
+  describe '#raw_blame' do
+    let(:project) { create(:project, :test_repo) }
+    let(:revision) { 'blame-on-renamed' }
+    let(:path) { 'files/plain_text/renamed' }
+
+    let(:blame_headers) do
+      [
+        '405a45736a75e439bb059e638afaa9a3c2eeda79 1 1 2',
+        '405a45736a75e439bb059e638afaa9a3c2eeda79 2 2',
+        'bed1d1610ebab382830ee888288bf939c43873bb 3 3 1',
+        '3685515c40444faf92774e72835e1f9c0e809672 4 4 1',
+        '32c33da59f8a1a9f90bdeda570337888b00b244d 5 5 1'
+      ]
+    end
+
+    subject(:blame) { client.raw_blame(revision, path, range: range).split("\n") }
+
+    context 'without a range' do
+      let(:range) { nil }
+
+      it 'blames a whole file' do
+        is_expected.to include(*blame_headers)
+      end
+    end
+
+    context 'with a range' do
+      let(:range) { '3,4' }
+
+      it 'blames part of a file' do
+        is_expected.to include(blame_headers[2], blame_headers[3])
+        is_expected.not_to include(blame_headers[0], blame_headers[1], blame_headers[4])
+      end
+    end
+  end
 end

spec/presenters/gitlab/blame_presenter_spec.rb

@@ -27,6 +27,14 @@ RSpec.describe Gitlab::BlamePresenter do
     end
   end
 
+  describe '#first_line' do
+    it 'delegates #first_line call to the blame' do
+      expect(blame).to receive(:first_line).at_least(:once).and_call_original
+
+      subject.first_line
+    end
+  end
+
   describe '#commit_data' do
     it 'has the data necessary to render the view' do
       commit = blame.groups.first[:commit]