Make Git history follow renames again by performing the --skip in Ruby

This hack is needed because of an issue when --follow and --skip
are passed to git log at the same time.

See https://gitlab.com/gitlab-org/gitlab-ce/issues/23062Signed-off-by: Rémy Coutable <remy@rymai.me>

app/models/repository.rb

@@ -109,9 +109,7 @@ class Repository
       offset: offset,
       after: after,
       before: before,
-      # --follow doesn't play well with --skip. See:
-      # https://gitlab.com/gitlab-org/gitlab-ce/issues/3574#note_3040520
-      follow: false,
+      follow: path.present?,
       skip_merges: skip_merges
     }
 

changelogs/unreleased/23062-allow-git-log-to-accept-follow-and-skip.yml

+---
+title: Make Git history follow renames again by performing the --skip in Ruby
+merge_request:
+author:

lib/gitlab/git/repository.rb

@@ -324,24 +324,30 @@ module Gitlab
       end
 
       def log_by_shell(sha, options)
+        limit = options[:limit].to_i
+        offset = options[:offset].to_i
+        use_follow_flag = options[:follow] && options[:path].present?
+
+        # We will perform the offset in Ruby because --follow doesn't play well with --skip.
+        # See: https://gitlab.com/gitlab-org/gitlab-ce/issues/3574#note_3040520
+        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 #{options[:limit].to_i})
+        cmd += %W(-n #{limit})
         cmd += %w(--format=%H)
-        cmd += %W(--skip=#{options[:offset].to_i})
-        cmd += %w(--follow) if options[:follow]
+        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?
 
-        raw_output = IO.popen(cmd) {|io| io.read }
-
-        log = raw_output.lines.map do |c|
-          Rugged::Commit.new(rugged, c.strip)
-        end
+        raw_output = IO.popen(cmd) { |io| io.read }
+        lines = offset_in_ruby ? raw_output.lines.drop(offset) : raw_output.lines
 
-        log.is_a?(Array) ? log : []
+        lines.map { |c| Rugged::Commit.new(rugged, c.strip) }
       end
 
       def sha_from_ref(ref)

spec/lib/gitlab/git/repository_spec.rb

@@ -546,7 +546,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
           repository.log(options.merge(path: "encoding"))
         end
 
-        it "should not follow renames" do
+        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)
@@ -554,33 +554,107 @@ describe Gitlab::Git::Repository, seed_helper: true do
       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 "should follow renames" do
+          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)
           end
         end
 
-      context "and 'path' is a file that matches the old filename" do
+        context 'with offset=1' do
           let(:log_commits) do
-          repository.log(options.merge(path: "CHANGELOG"))
+            repository.log(options.merge(path: "encoding/CHANGELOG", offset: 1))
           end
 
-        it "should not follow renames" do
+          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)
+          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)
+          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)
+          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)
+          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)
+          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)
+          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)
         end
       end
 
       context "unknown ref" do
         let(:log_commits) { repository.log(options.merge(ref: 'unknown')) }
 
-        it "should return empty" do
+        it "return an empty array" do
           expect(log_commits).to eq([])
         end
       end