diff options
-rw-r--r-- | app/models/repository.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/23062-allow-git-log-to-accept-follow-and-skip.yml | 4 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 42 | ||||
-rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 124 |
4 files changed, 123 insertions, 51 deletions
diff --git a/app/models/repository.rb b/app/models/repository.rb index 1762118278a..0dbf246c3a4 100644 --- a/app/models/repository.rb +++ b/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 } diff --git a/changelogs/unreleased/23062-allow-git-log-to-accept-follow-and-skip.yml b/changelogs/unreleased/23062-allow-git-log-to-accept-follow-and-skip.yml new file mode 100644 index 00000000000..f7c856040e0 --- /dev/null +++ b/changelogs/unreleased/23062-allow-git-log-to-accept-follow-and-skip.yml @@ -0,0 +1,4 @@ +--- +title: Make Git history follow renames again by performing the --skip in Ruby +merge_request: +author: diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b8354d7bf04..8ec90885231 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -324,24 +324,30 @@ module Gitlab end def log_by_shell(sha, options) - cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{path} log) - cmd += %W(-n #{options[:limit].to_i}) - cmd += %w(--format=%H) - cmd += %W(--skip=#{options[:offset].to_i}) - cmd += %w(--follow) if options[:follow] - 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 - - log.is_a?(Array) ? log : [] + 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 << "--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) } 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 766f16bb824..1919542ca25 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,49 +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 + log_commits = repository.log(options.merge(path: "encoding")) - it "should 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) + 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 - let(:log_commits) do - repository.log(options.merge(path: "encoding/CHANGELOG")) + context 'without offset' do + it "follows renames" do + 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 - it "should follow 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) + context 'with offset=1' do + it "follows renames and skip the latest commit" do + 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 + it "follows renames, skip the latest commit and return only one commit" do + 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 + it "follows renames, skip the latest commit and return only two commits" do + 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 + it "follows renames and skip the latest commit" do + 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 + it "follows renames, skip the two latest commit and return only one commit" do + 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 + it "follows renames, skip the two latest commit and return only one commit" do + 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 + log_commits = repository.log(options.merge(path: "CHANGELOG")) - it "should not follow renames" do - expect(log_commits).to include(commit_with_old_name) - expect(log_commits).to include(rename_commit) - expect(log_commits).not_to include(commit_with_new_name) + 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 "should return empty" do expect(log_commits).to eq([]) end end @@ -699,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 |