From 081a584a35eb44dafd77d212acd9317a7f1414ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 10 Jan 2018 13:34:31 -0300 Subject: [PATCH 1/2] Ensure hooks are deleted regardless of the project forking method --- spec/lib/gitlab/git/gitlab_projects_spec.rb | 3 +++ spec/spec_helper.rb | 10 ++++++++++ spec/support/project_forks_helper.rb | 4 ---- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/spec/lib/gitlab/git/gitlab_projects_spec.rb b/spec/lib/gitlab/git/gitlab_projects_spec.rb index 78e4fbca28e..f4b964e1ee9 100644 --- a/spec/lib/gitlab/git/gitlab_projects_spec.rb +++ b/spec/lib/gitlab/git/gitlab_projects_spec.rb @@ -219,6 +219,9 @@ describe Gitlab::Git::GitlabProjects do before do FileUtils.mkdir_p(dest_repos_path) + + # Undo spec_helper stub that deletes hooks + allow_any_instance_of(described_class).to receive(:fork_repository).and_call_original end after do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6186fb92bad..85de0a14631 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -100,6 +100,16 @@ RSpec.configure do |config| config.before(:example) do # Skip pre-receive hook check so we can use the web editor and merge. allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil]) + + allow_any_instance_of(Gitlab::Git::GitlabProjects).to receive(:fork_repository).and_wrap_original do |m, *args| + m.call(*args) + + shard_path, repository_relative_path = args + # We can't leave the hooks in place after a fork, as those would fail in tests + # The "internal" API is not available + FileUtils.rm_rf(File.join(shard_path, repository_relative_path, 'hooks')) + end + # Enable all features by default for testing allow(Feature).to receive(:enabled?) { true } end diff --git a/spec/support/project_forks_helper.rb b/spec/support/project_forks_helper.rb index d6680735aa1..2c501a2a27c 100644 --- a/spec/support/project_forks_helper.rb +++ b/spec/support/project_forks_helper.rb @@ -38,10 +38,6 @@ module ProjectForksHelper # so we have to explicitely call this method to clear the @exists variable. # of the instance we're returning here. forked_project.repository.after_import - - # We can't leave the hooks in place after a fork, as those would fail in tests - # The "internal" API is not available - FileUtils.rm_rf("#{forked_project.repository.path}/hooks") end forked_project From 2e0951e93f15ab3437cb13a1a2be36f45f777c3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 9 Jan 2018 00:51:05 -0300 Subject: [PATCH 2/2] Incorporate Gitaly's OperationService.UserCommitFiles RPC --- GITALY_SERVER_VERSION | 2 +- Gemfile | 2 +- Gemfile.lock | 4 +- lib/gitlab/git/repository.rb | 66 ++++++++++------- lib/gitlab/gitaly_client/operation_service.rb | 72 +++++++++++++++++++ spec/services/files/update_service_spec.rb | 12 ++-- spec/support/cycle_analytics_helpers.rb | 31 +++++++- 7 files changed, 152 insertions(+), 37 deletions(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 534b316aef6..7375dee5f49 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.70.0 +0.72.0 diff --git a/Gemfile b/Gemfile index 6f82890d0d1..a4c3db4bf5f 100644 --- a/Gemfile +++ b/Gemfile @@ -406,7 +406,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.73.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.74.0', require: 'gitaly' gem 'toml-rb', '~> 0.3.15', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 284e1e7654d..d69c532b309 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -285,7 +285,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly-proto (0.73.0) + gitaly-proto (0.74.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -1056,7 +1056,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.2.0) - gitaly-proto (~> 0.73.0) + gitaly-proto (~> 0.74.0) github-linguist (~> 4.7.0) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-markup (~> 1.6.2) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b89a38d187e..25dc34a2c3a 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1287,32 +1287,15 @@ module Gitlab author_email: nil, author_name: nil, start_branch_name: nil, start_repository: self) - OperationService.new(user, self).with_branch( - branch_name, - start_branch_name: start_branch_name, - start_repository: start_repository - ) do |start_commit| - index = Gitlab::Git::Index.new(self) - parents = [] - - if start_commit - index.read_tree(start_commit.rugged_commit.tree) - parents = [start_commit.sha] + gitaly_migrate(:operation_user_commit_files) do |is_enabled| + if is_enabled + gitaly_operation_client.user_commit_files(user, branch_name, + message, actions, author_email, author_name, + start_branch_name, start_repository) + else + rugged_multi_action(user, branch_name, message, actions, + author_email, author_name, start_branch_name, start_repository) end - - actions.each { |opts| index.apply(opts.delete(:action), opts) } - - committer = user_to_committer(user) - author = Gitlab::Git.committer_hash(email: author_email, name: author_name) || committer - options = { - tree: index.write_tree, - message: message, - parents: parents, - author: author, - committer: committer - } - - create_commit(options) end end # rubocop:enable Metrics/ParameterLists @@ -2075,6 +2058,39 @@ module Gitlab remove_remote(remote_name) end + def rugged_multi_action( + user, branch_name, message, actions, author_email, author_name, + start_branch_name, start_repository) + + OperationService.new(user, self).with_branch( + branch_name, + start_branch_name: start_branch_name, + start_repository: start_repository + ) do |start_commit| + index = Gitlab::Git::Index.new(self) + parents = [] + + if start_commit + index.read_tree(start_commit.rugged_commit.tree) + parents = [start_commit.sha] + end + + actions.each { |opts| index.apply(opts.delete(:action), opts) } + + committer = user_to_committer(user) + author = Gitlab::Git.committer_hash(email: author_email, name: author_name) || committer + options = { + tree: index.write_tree, + message: message, + parents: parents, + author: author, + committer: committer + } + + create_commit(options) + end + end + def fetch_remote(remote_name = 'origin', env: nil) run_git(['fetch', remote_name], env: env).last.zero? end diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 7319de69d13..c2b4155e6a5 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -3,6 +3,8 @@ module Gitlab class OperationService include Gitlab::EncodingHelper + MAX_MSG_SIZE = 128.kilobytes.freeze + def initialize(repository) @gitaly_repo = repository.gitaly_repository @repository = repository @@ -175,6 +177,49 @@ module Gitlab end end + def user_commit_files( + user, branch_name, commit_message, actions, author_email, author_name, + start_branch_name, start_repository) + + req_enum = Enumerator.new do |y| + header = user_commit_files_request_header(user, branch_name, + commit_message, actions, author_email, author_name, + start_branch_name, start_repository) + + y.yield Gitaly::UserCommitFilesRequest.new(header: header) + + actions.each do |action| + action_header = user_commit_files_action_header(action) + y.yield Gitaly::UserCommitFilesRequest.new( + action: Gitaly::UserCommitFilesAction.new(header: action_header) + ) + + reader = binary_stringio(action[:content]) + + until reader.eof? + chunk = reader.read(MAX_MSG_SIZE) + + y.yield Gitaly::UserCommitFilesRequest.new( + action: Gitaly::UserCommitFilesAction.new(content: chunk) + ) + end + end + end + + response = GitalyClient.call(@repository.storage, :operation_service, + :user_commit_files, req_enum, remote_storage: start_repository.storage) + + if (pre_receive_error = response.pre_receive_error.presence) + raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error + end + + if (index_error = response.index_error.presence) + raise Gitlab::Git::Index::IndexError, index_error + end + + Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update) + end + private def call_cherry_pick_or_revert(rpc, user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) @@ -212,6 +257,33 @@ module Gitlab Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update) end end + + def user_commit_files_request_header( + user, branch_name, commit_message, actions, author_email, author_name, + start_branch_name, start_repository) + + Gitaly::UserCommitFilesRequestHeader.new( + repository: @gitaly_repo, + user: Gitlab::Git::User.from_gitlab(user).to_gitaly, + branch_name: encode_binary(branch_name), + commit_message: encode_binary(commit_message), + commit_author_name: encode_binary(author_name), + commit_author_email: encode_binary(author_email), + start_branch_name: encode_binary(start_branch_name), + start_repository: start_repository.gitaly_repository + ) + end + + def user_commit_files_action_header(action) + Gitaly::UserCommitFilesActionHeader.new( + action: action[:action].upcase.to_sym, + file_path: encode_binary(action[:file_path]), + previous_path: encode_binary(action[:previous_path]), + base64_content: action[:encoding] == 'base64' + ) + rescue RangeError + raise ArgumentError, "Unknown action '#{action[:action]}'" + end end end end diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb index 43b0c9a63a9..16bfbdf3089 100644 --- a/spec/services/files/update_service_spec.rb +++ b/spec/services/files/update_service_spec.rb @@ -72,13 +72,15 @@ describe Files::UpdateService do end end - context 'when target branch is different than source branch' do - let(:branch_name) { "#{project.default_branch}-new" } + context 'with gitaly disabled', :skip_gitaly_mock do + context 'when target branch is different than source branch' do + let(:branch_name) { "#{project.default_branch}-new" } - it 'fires hooks only once' do - expect(Gitlab::Git::HooksService).to receive(:new).once.and_call_original + it 'fires hooks only once' do + expect(Gitlab::Git::HooksService).to receive(:new).once.and_call_original - subject.execute + subject.execute + end end end end diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb index 26fd271ce31..d5ef80cfab2 100644 --- a/spec/support/cycle_analytics_helpers.rb +++ b/spec/support/cycle_analytics_helpers.rb @@ -5,10 +5,16 @@ module CycleAnalyticsHelpers end def create_commit(message, project, user, branch_name, count: 1) - oldrev = project.repository.commit(branch_name).sha + repository = project.repository + oldrev = repository.commit(branch_name).sha + + if Timecop.frozen? && Gitlab::GitalyClient.feature_enabled?(:operation_user_commit_files) + mock_gitaly_multi_action_dates(repository.raw) + end + commit_shas = Array.new(count) do |index| - commit_sha = project.repository.create_file(user, generate(:branch), "content", message: message, branch_name: branch_name) - project.repository.commit(commit_sha) + commit_sha = repository.create_file(user, generate(:branch), "content", message: message, branch_name: branch_name) + repository.commit(commit_sha) commit_sha end @@ -98,6 +104,25 @@ module CycleAnalyticsHelpers pipeline: dummy_pipeline, protected: false) end + + def mock_gitaly_multi_action_dates(raw_repository) + allow(raw_repository).to receive(:multi_action).and_wrap_original do |m, *args| + new_date = Time.now + branch_update = m.call(*args) + + if branch_update.newrev + _, opts = args + commit = raw_repository.commit(branch_update.newrev).rugged_commit + branch_update.newrev = commit.amend( + update_ref: "#{Gitlab::Git::BRANCH_REF_PREFIX}#{opts[:branch_name]}", + author: commit.author.merge(time: new_date), + committer: commit.committer.merge(time: new_date) + ) + end + + branch_update + end + end end RSpec.configure do |config|