Optimize a bit Gitlab::Git::Repository#log_by_shell and its specs23062-allow-git-log-to-accept-follow-and-skip

Signed-off-by: Rémy Coutable <remy@rymai.me>

2 files changed, 74 insertions, 84 deletions

diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 9c68060064a..8afbd818bbd 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -333,21 +333,21 @@ module Gitlab
         offset_in_ruby = use_follow_flag && options[:offset].present?
         limit += offset if offset_in_ruby
 
-        cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{path} log)
-        cmd += %W(-n #{limit})
-        cmd += %w(--format=%H)
-        cmd += %W(--skip=#{offset}) unless offset_in_ruby
-        cmd += %w(--follow) if use_follow_flag
-        cmd += %w(--no-merges) if options[:skip_merges]
-        cmd += %W(--after=#{options[:after].iso8601}) if options[:after]
-        cmd += %W(--before=#{options[:before].iso8601}) if options[:before]
-        cmd += [sha]
-        cmd += %W(-- #{options[:path]}) if options[:path].present?
+        cmd = %W[#{Gitlab.config.git.bin_path} --git-dir=#{path} log]
+        cmd << "--max-count=#{limit}"
+        cmd << '--format=%H'
+        cmd << "--skip=#{offset}" unless offset_in_ruby
+        cmd << '--follow' if use_follow_flag
+        cmd << '--no-merges' if options[:skip_merges]
+        cmd << "--after=#{options[:after].iso8601}" if options[:after]
+        cmd << "--before=#{options[:before].iso8601}" if options[:before]
+        cmd << sha
+        cmd += %W[-- #{options[:path]}] if options[:path].present?
 
         raw_output = IO.popen(cmd) { |io| io.read }
         lines = offset_in_ruby ? raw_output.lines.drop(offset) : raw_output.lines
 
-        lines.map { |c| Rugged::Commit.new(rugged, c.strip) }
+        lines.map! { |c| Rugged::Commit.new(rugged, c.strip) }
       end
 
       def sha_from_ref(ref)
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 05594089db3..2ea5f8f68be 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -529,7 +529,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
     commit_with_new_name = nil
     rename_commit = nil
 
-    before(:all) do
+    before(:context) do
       # Add new commits so that there's a renamed file in the commit history
       repo = Gitlab::Git::Repository.new(TEST_REPO_PATH).rugged
 
@@ -538,123 +538,119 @@ describe Gitlab::Git::Repository, seed_helper: true do
       commit_with_new_name = new_commit_edit_new_file(repo)
     end
 
+    after(:context) do
+      # Erase our commits so other tests get the original repo
+      repo = Gitlab::Git::Repository.new(TEST_REPO_PATH).rugged
+      repo.references.update("refs/heads/master", SeedRepo::LastCommit::ID)
+    end
+
     context "where 'follow' == true" do
-      options = { ref: "master", follow: true }
+      let(:options) { { ref: "master", follow: true } }
 
       context "and 'path' is a directory" do
-        let(:log_commits) do
-          repository.log(options.merge(path: "encoding"))
-        end
-
         it "does not follow renames" do
-          expect(log_commits).to include(commit_with_new_name)
-          expect(log_commits).to include(rename_commit)
-          expect(log_commits).not_to include(commit_with_old_name)
+          log_commits = repository.log(options.merge(path: "encoding"))
+
+          aggregate_failures do
+            expect(log_commits).to include(commit_with_new_name)
+            expect(log_commits).to include(rename_commit)
+            expect(log_commits).not_to include(commit_with_old_name)
+          end
         end
       end
 
       context "and 'path' is a file that matches the new filename" do
         context 'without offset' do
-          let(:log_commits) do
-            repository.log(options.merge(path: "encoding/CHANGELOG"))
-          end
-
           it "follows renames" do
-            expect(log_commits).to include(commit_with_new_name)
-            expect(log_commits).to include(rename_commit)
-            expect(log_commits).to include(commit_with_old_name)
+            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG"))
+
+            aggregate_failures do
+              expect(log_commits).to include(commit_with_new_name)
+              expect(log_commits).to include(rename_commit)
+              expect(log_commits).to include(commit_with_old_name)
+            end
           end
         end
 
         context 'with offset=1' do
-          let(:log_commits) do
-            repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1))
-          end
-
           it "follows renames and skip the latest commit" do
-            expect(log_commits).not_to include(commit_with_new_name)
-            expect(log_commits).to include(rename_commit)
-            expect(log_commits).to include(commit_with_old_name)
+            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1))
+
+            aggregate_failures do
+              expect(log_commits).not_to include(commit_with_new_name)
+              expect(log_commits).to include(rename_commit)
+              expect(log_commits).to include(commit_with_old_name)
+            end
           end
         end
 
         context 'with offset=1', 'and limit=1' do
-          let(:log_commits) do
-            repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1, limit: 1))
-          end
-
           it "follows renames, skip the latest commit and return only one commit" do
-            expect(log_commits).not_to include(commit_with_new_name)
-            expect(log_commits).to include(rename_commit)
-            expect(log_commits).not_to include(commit_with_old_name)
+            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1, limit: 1))
+
+            expect(log_commits).to contain_exactly(rename_commit)
           end
         end
 
         context 'with offset=1', 'and limit=2' do
-          let(:log_commits) do
-            repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1, limit: 2))
-          end
-
           it "follows renames, skip the latest commit and return only two commits" do
-            expect(log_commits).not_to include(commit_with_new_name)
-            expect(log_commits).to include(rename_commit)
-            expect(log_commits).to include(commit_with_old_name)
+            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1, limit: 2))
+
+            aggregate_failures do
+              expect(log_commits).to contain_exactly(rename_commit, commit_with_old_name)
+            end
           end
         end
 
         context 'with offset=2' do
-          let(:log_commits) do
-            repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2))
-          end
-
           it "follows renames and skip the latest commit" do
-            expect(log_commits).not_to include(commit_with_new_name)
-            expect(log_commits).not_to include(rename_commit)
-            expect(log_commits).to include(commit_with_old_name)
+            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2))
+
+            aggregate_failures do
+              expect(log_commits).not_to include(commit_with_new_name)
+              expect(log_commits).not_to include(rename_commit)
+              expect(log_commits).to include(commit_with_old_name)
+            end
           end
         end
 
         context 'with offset=2', 'and limit=1' do
-          let(:log_commits) do
-            repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2, limit: 1))
-          end
-
           it "follows renames, skip the two latest commit and return only one commit" do
-            expect(log_commits).not_to include(commit_with_new_name)
-            expect(log_commits).not_to include(rename_commit)
-            expect(log_commits).to include(commit_with_old_name)
+            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2, limit: 1))
+
+            expect(log_commits).to contain_exactly(commit_with_old_name)
           end
         end
 
         context 'with offset=2', 'and limit=2' do
-          let(:log_commits) do
-            repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2, limit: 2))
-          end
-
           it "follows renames, skip the two latest commit and return only one commit" do
-            expect(log_commits).not_to include(commit_with_new_name)
-            expect(log_commits).not_to include(rename_commit)
-            expect(log_commits).to include(commit_with_old_name)
+            log_commits = repository.log(options.merge(path: "encoding/CHANGELOG", offset: 2, limit: 2))
+
+            aggregate_failures do
+              expect(log_commits).not_to include(commit_with_new_name)
+              expect(log_commits).not_to include(rename_commit)
+              expect(log_commits).to include(commit_with_old_name)
+            end
           end
         end
       end
 
       context "and 'path' is a file that matches the old filename" do
-        let(:log_commits) do
-          repository.log(options.merge(path: "CHANGELOG"))
-        end
-
         it "does not follow renames" do
-          expect(log_commits).not_to include(commit_with_new_name)
-          expect(log_commits).to include(rename_commit)
-          expect(log_commits).to include(commit_with_old_name)
+          log_commits = repository.log(options.merge(path: "CHANGELOG"))
+
+          aggregate_failures do
+            expect(log_commits).not_to include(commit_with_new_name)
+            expect(log_commits).to include(rename_commit)
+            expect(log_commits).to include(commit_with_old_name)
+          end
         end
       end
 
       context "unknown ref" do
-        let(:log_commits) { repository.log(options.merge(ref: 'unknown')) }
+        it "returns an empty array" do
+          log_commits = repository.log(options.merge(ref: 'unknown'))
 
-        it "return an empty array" do
           expect(log_commits).to eq([])
         end
       end
@@ -773,12 +769,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
         end
       end
     end
-
-    after(:all) do
-      # Erase our commits so other tests get the original repo
-      repo = Gitlab::Git::Repository.new(TEST_REPO_PATH).rugged
-      repo.references.update("refs/heads/master", SeedRepo::LastCommit::ID)
-    end
   end
 
   describe "#commits_between" do