From c475b171114e9bd49b2f1779eb91d392328928d8 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 17 Feb 2016 16:49:16 +0100 Subject: [PATCH 1/5] Only set autocrlf when creating/updating files Setting the "autocrlf" Git option is an overkill since it's rarely actually needed. More importantly, it has quite the impact on performance (see gitlab-org/gitlab-ce#13457 for more information). By setting "autocrlf" when creating or updating files we guarantee the option is always set properly when we actually need it _without_ introducing overhead for requests that have nothing to do with this option. Fixes gitlab-org/gitlab-ce#13457 --- app/models/repository.rb | 10 ++++++--- spec/models/repository_spec.rb | 40 ++++++++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 5a25ccb1dd6..381b1db3758 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -24,14 +24,16 @@ class Repository return nil unless path_with_namespace @raw_repository ||= begin - repo = Gitlab::Git::Repository.new(path_to_repo) - repo.autocrlf = :input - repo + Gitlab::Git::Repository.new(path_to_repo) rescue Gitlab::Git::Repository::NoRepository nil end end + def update_autocrlf_option + raw_repository.autocrlf = :input if raw_repository.autocrlf != :input + end + # Return absolute path to repository def path_to_repo @path_to_repo ||= File.expand_path( @@ -693,6 +695,8 @@ class Repository end def commit_with_hooks(current_user, branch) + update_autocrlf_option + oldrev = Gitlab::Git::BLANK_SHA ref = Gitlab::Git::BRANCH_REF_PREFIX + branch was_empty = empty? diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 2cd0606a61d..8ff198f572d 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -200,13 +200,22 @@ describe Repository, models: true do describe :commit_with_hooks do context 'when pre hooks were successful' do - it 'should run without errors' do - expect_any_instance_of(GitHooksService).to receive(:execute).and_return(true) + before do + expect_any_instance_of(GitHooksService).to receive(:execute). + and_return(true) + end + it 'should run without errors' do expect do repository.commit_with_hooks(user, 'feature') { sample_commit.id } end.not_to raise_error end + + it 'should ensure the autocrlf Git option is set to :input' do + expect(repository).to receive(:update_autocrlf_option) + + repository.commit_with_hooks(user, 'feature') { sample_commit.id } + end end context 'when pre hooks failed' do @@ -249,6 +258,33 @@ describe Repository, models: true do end end + describe '#update_autocrlf_option' do + describe 'when autocrlf is not already set to :input' do + before do + repository.raw_repository.autocrlf = true + end + + it 'sets autocrlf to :input' do + repository.update_autocrlf_option + + expect(repository.raw_repository.autocrlf).to eq(:input) + end + end + + describe 'when autocrlf is already set to :input' do + before do + repository.raw_repository.autocrlf = :input + end + + it 'does nothing' do + expect(repository.raw_repository).to_not receive(:autocrlf=). + with(:input) + + repository.update_autocrlf_option + end + end + end + describe '#empty?' do let(:empty_repository) { create(:project_empty_repo).repository } From 54aa0969d40549014bee6e4e500276405fb055ee Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 18 Feb 2016 12:19:49 +0100 Subject: [PATCH 2/5] Fixed Repository#exists? to handle errors Now that Repository#raw_repository no longer sets the autocrlf option it will also no longer raise any NoRepository errors since it doesn't access Rugged any more. This also means that Repository#exists? can't simply return the raw repository as this is no indication of whether or not the repository actually exists (besides returning a non boolean is weird in the first place). To solve this problem Repository#exists? now properly checks if the repository exists and returns true/false instead of a Gitlab::Git::Repository or nil object. --- app/models/repository.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 381b1db3758..631a248258f 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -23,11 +23,7 @@ class Repository def raw_repository return nil unless path_with_namespace - @raw_repository ||= begin - Gitlab::Git::Repository.new(path_to_repo) - rescue Gitlab::Git::Repository::NoRepository - nil - end + @raw_repository ||= Gitlab::Git::Repository.new(path_to_repo) end def update_autocrlf_option @@ -42,7 +38,10 @@ class Repository end def exists? - raw_repository + raw_repository.rugged + true + rescue Gitlab::Git::Repository::NoRepository + false end def empty? From 8a7aad770cbf2fad1e6b7d5a0a1fd233a0e5ac9b Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 18 Feb 2016 12:28:47 +0100 Subject: [PATCH 3/5] Added specs for Repository#exists? --- spec/models/repository_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 8ff198f572d..b97e3cbc70a 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -229,6 +229,19 @@ describe Repository, models: true do end end + describe '#exists?' do + it 'returns true when a repository exists' do + expect(repository.exists?).to eq(true) + end + + it 'returns false when a repository does not exist' do + expect(repository.raw_repository).to receive(:rugged). + and_raise(Gitlab::Git::Repository::NoRepository) + + expect(repository.exists?).to eq(false) + end + end + describe '#has_visible_content?' do subject { repository.has_visible_content? } From 19b21c2e0ce1f6288172bd6fadc316f169048071 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 18 Feb 2016 13:22:33 +0100 Subject: [PATCH 4/5] Use Repository#exists? in Repository#commit? Just checking raw_repository is no longer accurate to determine if a repository exists or not. --- app/models/repository.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 631a248258f..67155ac4f80 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -68,7 +68,7 @@ class Repository end def commit(id = 'HEAD') - return nil unless raw_repository + return nil unless exists? commit = Gitlab::Git::Commit.find(raw_repository, id) commit = Commit.new(commit, @project) if commit commit From 5b6d347fcdb915a977369721bbc2545f17467cbb Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 18 Feb 2016 14:19:35 +0100 Subject: [PATCH 5/5] Handle raw_repository returning nil in exists? If path_with_namespace is nil Repository#raw_repository will also return nil. Apparently code out there creates a Repository instance without a namespace path. Right. --- app/models/repository.rb | 2 ++ spec/models/repository_spec.rb | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/app/models/repository.rb b/app/models/repository.rb index 67155ac4f80..73aa67d46ec 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -38,6 +38,8 @@ class Repository end def exists? + return false unless raw_repository + raw_repository.rugged true rescue Gitlab::Git::Repository::NoRepository diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index b97e3cbc70a..ed91b62c534 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -240,6 +240,12 @@ describe Repository, models: true do expect(repository.exists?).to eq(false) end + + it 'returns false when there is no namespace' do + allow(repository).to receive(:path_with_namespace).and_return(nil) + + expect(repository.exists?).to eq(false) + end end describe '#has_visible_content?' do