From 3eb0354061b6c2ad39c90c9a3e9a5afa62e7685f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 16 Feb 2017 14:35:48 +0100 Subject: Make Git history follow renames again by performing the --skip in Ruby MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/23062 Signed-off-by: Rémy Coutable --- app/models/repository.rb | 4 +- ...062-allow-git-log-to-accept-follow-and-skip.yml | 4 + lib/gitlab/git/repository.rb | 24 ++++-- spec/lib/gitlab/git/repository_spec.rb | 96 +++++++++++++++++++--- 4 files changed, 105 insertions(+), 23 deletions(-) create mode 100644 changelogs/unreleased/23062-allow-git-log-to-accept-follow-and-skip.yml diff --git a/app/models/repository.rb b/app/models/repository.rb index cd2568ad445..d7668cbd131 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 4b6ad8037ce..9c68060064a 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) + 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) diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 2a915bf426f..05594089db3 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/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,14 +554,88 @@ describe Gitlab::Git::Repository, seed_helper: true do 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 + 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) + 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 + 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) + 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 @@ -570,17 +644,17 @@ describe Gitlab::Git::Repository, seed_helper: true do repository.log(options.merge(path: "CHANGELOG")) end - it "should not follow renames" do - expect(log_commits).to include(commit_with_old_name) - expect(log_commits).to include(rename_commit) + 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 -- cgit v1.2.3 From 9720bcd83df29b4dc8da241d4d632993cd3f2895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 23 Feb 2017 19:05:13 +0100 Subject: Optimize a bit Gitlab::Git::Repository#log_by_shell and its specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/gitlab/git/repository.rb | 22 +++--- spec/lib/gitlab/git/repository_spec.rb | 136 +++++++++++++++------------------ 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 -- cgit v1.2.3