From a431ca0f8b7f8967e89a35caddf1e41e53eee290 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Wed, 2 Nov 2016 11:39:12 +0100
Subject: [PATCH 01/78] Don't execute git hooks if you create branch as part of
other change
Currently, our procedure for adding a commit requires us to execute `CreateBranchService` before file creation.
It's OK, but also we do execute `git hooks` (the `PostReceive` sidekiq job) as part of this process.
However, this hook is execute before the file is actually committed, so the ref is updated.
Secondly, we do execute a `git hooks` after committing file and updating ref.
This results in duplicate `PostReceive` jobs, where the first one is completely invalid.
This change makes the branch creation, something that is intermediate step of bigger process (file creation or update, commit cherry pick or revert) to not execute git hooks.
---
app/models/repository.rb | 8 ++++++--
app/services/commits/change_service.rb | 2 +-
app/services/create_branch_service.rb | 4 ++--
app/services/files/base_service.rb | 2 +-
4 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 30be7262438..0776c7ccc5d 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -160,14 +160,18 @@ class Repository
tags.find { |tag| tag.name == name }
end
- def add_branch(user, branch_name, target)
+ def add_branch(user, branch_name, target, with_hooks: true)
oldrev = Gitlab::Git::BLANK_SHA
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
target = commit(target).try(:id)
return false unless target
- GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do
+ if with_hooks
+ GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do
+ update_ref!(ref, target, oldrev)
+ end
+ else
update_ref!(ref, target, oldrev)
end
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 1c82599c579..2d4c9788d02 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -55,7 +55,7 @@ module Commits
return success if repository.find_branch(new_branch)
result = CreateBranchService.new(@project, current_user)
- .execute(new_branch, @target_branch, source_project: @source_project)
+ .execute(new_branch, @target_branch, source_project: @source_project, with_hooks: false)
if result[:status] == :error
raise ChangeError, "There was an error creating the source branch: #{result[:message]}"
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index 757fc35a78f..a6a3461e17b 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -1,7 +1,7 @@
require_relative 'base_service'
class CreateBranchService < BaseService
- def execute(branch_name, ref, source_project: @project)
+ def execute(branch_name, ref, source_project: @project, with_hooks: true)
valid_branch = Gitlab::GitRefValidator.validate(branch_name)
unless valid_branch
@@ -26,7 +26,7 @@ class CreateBranchService < BaseService
repository.find_branch(branch_name)
else
- repository.add_branch(current_user, branch_name, ref)
+ repository.add_branch(current_user, branch_name, ref, with_hooks: with_hooks)
end
if new_branch
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 9bd4bd464f7..1802b932e03 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -74,7 +74,7 @@ module Files
end
def create_target_branch
- result = CreateBranchService.new(project, current_user).execute(@target_branch, @source_branch, source_project: @source_project)
+ result = CreateBranchService.new(project, current_user).execute(@target_branch, @source_branch, source_project: @source_project, with_hooks: false)
unless result[:status] == :success
raise_error("Something went wrong when we tried to create #{@target_branch} for you: #{result[:message]}")
--
GitLab
From 92aa402882cb9e2badc8213dd88913ad21b49857 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 5 Nov 2016 02:48:58 +0800
Subject: [PATCH 02/78] Add a test to make sure hooks are fire only once
when updating a file to a different branch.
---
spec/services/files/update_service_spec.rb | 29 +++++++++++-----------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb
index d3c37c7820f..6fadee9304b 100644
--- a/spec/services/files/update_service_spec.rb
+++ b/spec/services/files/update_service_spec.rb
@@ -6,7 +6,10 @@ describe Files::UpdateService do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:file_path) { 'files/ruby/popen.rb' }
- let(:new_contents) { "New Content" }
+ let(:new_contents) { 'New Content' }
+ let(:target_branch) { project.default_branch }
+ let(:last_commit_sha) { nil }
+
let(:commit_params) do
{
file_path: file_path,
@@ -16,7 +19,7 @@ describe Files::UpdateService do
last_commit_sha: last_commit_sha,
source_project: project,
source_branch: project.default_branch,
- target_branch: project.default_branch,
+ target_branch: target_branch
}
end
@@ -54,18 +57,6 @@ describe Files::UpdateService do
end
context "when the last_commit_sha is not supplied" do
- let(:commit_params) do
- {
- file_path: file_path,
- commit_message: "Update File",
- file_content: new_contents,
- file_content_encoding: "text",
- source_project: project,
- source_branch: project.default_branch,
- target_branch: project.default_branch,
- }
- end
-
it "returns a hash with the :success status " do
results = subject.execute
@@ -80,5 +71,15 @@ describe Files::UpdateService do
expect(results.data).to eq(new_contents)
end
end
+
+ context 'when target branch is different than source branch' do
+ let(:target_branch) { "#{project.default_branch}-new" }
+
+ it 'fires hooks only once' do
+ expect(GitHooksService).to receive(:new).once.and_call_original
+
+ subject.execute
+ end
+ end
end
end
--
GitLab
From 3128641f7eb93fec0930ebfb83a93dfa5e0b343a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 01:41:14 +0800
Subject: [PATCH 03/78] Revert "Don't execute git hooks if you create branch as
part of other change"
This reverts commit a431ca0f8b7f8967e89a35caddf1e41e53eee290.
---
app/models/repository.rb | 8 ++------
app/services/commits/change_service.rb | 2 +-
app/services/create_branch_service.rb | 4 ++--
app/services/files/base_service.rb | 2 +-
4 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index feaaacd02a9..063dc74021d 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -162,18 +162,14 @@ class Repository
tags.find { |tag| tag.name == name }
end
- def add_branch(user, branch_name, target, with_hooks: true)
+ def add_branch(user, branch_name, target)
oldrev = Gitlab::Git::BLANK_SHA
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
target = commit(target).try(:id)
return false unless target
- if with_hooks
- GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do
- update_ref!(ref, target, oldrev)
- end
- else
+ GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do
update_ref!(ref, target, oldrev)
end
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 2d4c9788d02..1c82599c579 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -55,7 +55,7 @@ module Commits
return success if repository.find_branch(new_branch)
result = CreateBranchService.new(@project, current_user)
- .execute(new_branch, @target_branch, source_project: @source_project, with_hooks: false)
+ .execute(new_branch, @target_branch, source_project: @source_project)
if result[:status] == :error
raise ChangeError, "There was an error creating the source branch: #{result[:message]}"
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index a6a3461e17b..757fc35a78f 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -1,7 +1,7 @@
require_relative 'base_service'
class CreateBranchService < BaseService
- def execute(branch_name, ref, source_project: @project, with_hooks: true)
+ def execute(branch_name, ref, source_project: @project)
valid_branch = Gitlab::GitRefValidator.validate(branch_name)
unless valid_branch
@@ -26,7 +26,7 @@ class CreateBranchService < BaseService
repository.find_branch(branch_name)
else
- repository.add_branch(current_user, branch_name, ref, with_hooks: with_hooks)
+ repository.add_branch(current_user, branch_name, ref)
end
if new_branch
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 1802b932e03..9bd4bd464f7 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -74,7 +74,7 @@ module Files
end
def create_target_branch
- result = CreateBranchService.new(project, current_user).execute(@target_branch, @source_branch, source_project: @source_project, with_hooks: false)
+ result = CreateBranchService.new(project, current_user).execute(@target_branch, @source_branch, source_project: @source_project)
unless result[:status] == :success
raise_error("Something went wrong when we tried to create #{@target_branch} for you: #{result[:message]}")
--
GitLab
From 0b5a2eef8fa5ff4976f97883b631ec28f0553f6a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 04:02:10 +0800
Subject: [PATCH 04/78] Add `source_branch` option for various git operations
If `source_branch` option is passed, and target branch cannot be found,
`Repository#update_branch_with_hooks` would try to create a new branch
from `source_branch`.
This way, we could make changes in the new branch while only firing
the hooks once for the changes. Previously, we can only create a new
branch first then make changes to the new branch, firing hooks twice.
This behaviour is bad for CI.
Fixes #7237
---
app/models/repository.rb | 98 +++++++++++++++------
app/services/commits/change_service.rb | 10 +--
app/services/create_branch_service.rb | 22 ++---
app/services/files/base_service.rb | 11 ++-
app/services/files/create_dir_service.rb | 9 +-
app/services/files/create_service.rb | 11 ++-
app/services/files/delete_service.rb | 9 +-
app/services/files/multi_service.rb | 3 +-
app/services/files/update_service.rb | 3 +-
app/services/validate_new_branch_service.rb | 22 +++++
10 files changed, 145 insertions(+), 53 deletions(-)
create mode 100644 app/services/validate_new_branch_service.rb
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 063dc74021d..b6581486983 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -786,8 +786,12 @@ class Repository
@root_ref ||= cache.fetch(:root_ref) { raw_repository.root_ref }
end
- def commit_dir(user, path, message, branch, author_email: nil, author_name: nil)
- update_branch_with_hooks(user, branch) do |ref|
+ def commit_dir(user, path, message, branch,
+ author_email: nil, author_name: nil, source_branch: nil)
+ update_branch_with_hooks(
+ user,
+ branch,
+ source_branch: source_branch) do |ref|
options = {
commit: {
branch: ref,
@@ -802,8 +806,12 @@ class Repository
end
end
- def commit_file(user, path, content, message, branch, update, author_email: nil, author_name: nil)
- update_branch_with_hooks(user, branch) do |ref|
+ def commit_file(user, path, content, message, branch, update,
+ author_email: nil, author_name: nil, source_branch: nil)
+ update_branch_with_hooks(
+ user,
+ branch,
+ source_branch: source_branch) do |ref|
options = {
commit: {
branch: ref,
@@ -823,8 +831,13 @@ class Repository
end
end
- def update_file(user, path, content, branch:, previous_path:, message:, author_email: nil, author_name: nil)
- update_branch_with_hooks(user, branch) do |ref|
+ def update_file(user, path, content,
+ branch:, previous_path:, message:,
+ author_email: nil, author_name: nil, source_branch: nil)
+ update_branch_with_hooks(
+ user,
+ branch,
+ source_branch: source_branch) do |ref|
options = {
commit: {
branch: ref,
@@ -849,8 +862,12 @@ class Repository
end
end
- def remove_file(user, path, message, branch, author_email: nil, author_name: nil)
- update_branch_with_hooks(user, branch) do |ref|
+ def remove_file(user, path, message, branch,
+ author_email: nil, author_name: nil, source_branch: nil)
+ update_branch_with_hooks(
+ user,
+ branch,
+ source_branch: source_branch) do |ref|
options = {
commit: {
branch: ref,
@@ -868,17 +885,18 @@ class Repository
end
end
- def multi_action(user:, branch:, message:, actions:, author_email: nil, author_name: nil)
- update_branch_with_hooks(user, branch) do |ref|
+ def multi_action(user:, branch:, message:, actions:,
+ author_email: nil, author_name: nil, source_branch: nil)
+ update_branch_with_hooks(
+ user,
+ branch,
+ source_branch: source_branch) do |ref|
index = rugged.index
parents = []
- branch = find_branch(ref)
- if branch
- last_commit = branch.dereferenced_target
- index.read_tree(last_commit.raw_commit.tree)
- parents = [last_commit.sha]
- end
+ last_commit = find_branch(ref).dereferenced_target
+ index.read_tree(last_commit.raw_commit.tree)
+ parents = [last_commit.sha]
actions.each do |action|
case action[:action]
@@ -967,7 +985,10 @@ class Repository
return false unless revert_tree_id
- update_branch_with_hooks(user, base_branch) do
+ update_branch_with_hooks(
+ user,
+ base_branch,
+ source_branch: revert_tree_id) do
committer = user_to_committer(user)
source_sha = Rugged::Commit.create(rugged,
message: commit.revert_message,
@@ -984,7 +1005,10 @@ class Repository
return false unless cherry_pick_tree_id
- update_branch_with_hooks(user, base_branch) do
+ update_branch_with_hooks(
+ user,
+ base_branch,
+ source_branch: cherry_pick_tree_id) do
committer = user_to_committer(user)
source_sha = Rugged::Commit.create(rugged,
message: commit.message,
@@ -1082,11 +1106,11 @@ class Repository
fetch_ref(path_to_repo, ref, ref_path)
end
- def update_branch_with_hooks(current_user, branch)
+ def update_branch_with_hooks(current_user, branch, source_branch: nil)
update_autocrlf_option
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
- target_branch = find_branch(branch)
+ target_branch, new_branch_added = raw_ensure_branch(branch, source_branch)
was_empty = empty?
# Make commit
@@ -1096,7 +1120,7 @@ class Repository
raise CommitError.new('Failed to create commit')
end
- if rugged.lookup(newrev).parent_ids.empty? || target_branch.nil?
+ if rugged.lookup(newrev).parent_ids.empty?
oldrev = Gitlab::Git::BLANK_SHA
else
oldrev = rugged.merge_base(newrev, target_branch.dereferenced_target.sha)
@@ -1105,11 +1129,9 @@ class Repository
GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do
update_ref!(ref, newrev, oldrev)
- if was_empty || !target_branch
- # If repo was empty expire cache
- after_create if was_empty
- after_create_branch
- end
+ # If repo was empty expire cache
+ after_create if was_empty
+ after_create_branch if was_empty || new_branch_added
end
newrev
@@ -1169,4 +1191,28 @@ class Repository
def repository_event(event, tags = {})
Gitlab::Metrics.add_event(event, { path: path_with_namespace }.merge(tags))
end
+
+ def raw_ensure_branch(branch_name, source_branch)
+ old_branch = find_branch(branch_name)
+
+ if old_branch
+ [old_branch, false]
+ elsif source_branch
+ oldrev = Gitlab::Git::BLANK_SHA
+ ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
+ target = commit(source_branch).try(:id)
+
+ unless target
+ raise CommitError.new(
+ "Cannot find branch #{branch_name} nor #{source_branch}")
+ end
+
+ update_ref!(ref, target, oldrev)
+
+ [find_branch(branch_name), true]
+ else
+ raise CommitError.new(
+ "Cannot find branch #{branch_name} and source_branch is not set")
+ end
+ end
end
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 1c82599c579..04b28cfaed8 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -29,7 +29,7 @@ module Commits
tree_id = repository.public_send("check_#{action}_content", @commit, @target_branch)
if tree_id
- create_target_branch(into) if @create_merge_request
+ validate_target_branch(into) if @create_merge_request
repository.public_send(action, current_user, @commit, into, tree_id)
success
@@ -50,12 +50,12 @@ module Commits
true
end
- def create_target_branch(new_branch)
+ def validate_target_branch(new_branch)
# Temporary branch exists and contains the change commit
- return success if repository.find_branch(new_branch)
+ return if repository.find_branch(new_branch)
- result = CreateBranchService.new(@project, current_user)
- .execute(new_branch, @target_branch, source_project: @source_project)
+ result = ValidateNewBranchService.new(@project, current_user).
+ execute(new_branch)
if result[:status] == :error
raise ChangeError, "There was an error creating the source branch: #{result[:message]}"
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index 757fc35a78f..f4270a09928 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -2,18 +2,9 @@ require_relative 'base_service'
class CreateBranchService < BaseService
def execute(branch_name, ref, source_project: @project)
- valid_branch = Gitlab::GitRefValidator.validate(branch_name)
+ failure = validate_new_branch(branch_name)
- unless valid_branch
- return error('Branch name is invalid')
- end
-
- repository = project.repository
- existing_branch = repository.find_branch(branch_name)
-
- if existing_branch
- return error('Branch already exists')
- end
+ return failure if failure
new_branch = if source_project != @project
repository.fetch_ref(
@@ -41,4 +32,13 @@ class CreateBranchService < BaseService
def success(branch)
super().merge(branch: branch)
end
+
+ private
+
+ def validate_new_branch(branch_name)
+ result = ValidateNewBranchService.new(project, current_user).
+ execute(branch_name)
+
+ error(result[:message]) if result[:status] == :error
+ end
end
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 9bd4bd464f7..6779bd2818a 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -23,9 +23,7 @@ module Files
validate
# Create new branch if it different from source_branch
- if different_branch?
- create_target_branch
- end
+ validate_target_branch if different_branch?
result = commit
if result
@@ -73,10 +71,11 @@ module Files
end
end
- def create_target_branch
- result = CreateBranchService.new(project, current_user).execute(@target_branch, @source_branch, source_project: @source_project)
+ def validate_target_branch
+ result = ValidateNewBranchService.new(project, current_user).
+ execute(@target_branch)
- unless result[:status] == :success
+ if result[:status] == :error
raise_error("Something went wrong when we tried to create #{@target_branch} for you: #{result[:message]}")
end
end
diff --git a/app/services/files/create_dir_service.rb b/app/services/files/create_dir_service.rb
index d00d78cee7e..c59b3f8c70c 100644
--- a/app/services/files/create_dir_service.rb
+++ b/app/services/files/create_dir_service.rb
@@ -3,7 +3,14 @@ require_relative "base_service"
module Files
class CreateDirService < Files::BaseService
def commit
- repository.commit_dir(current_user, @file_path, @commit_message, @target_branch, author_email: @author_email, author_name: @author_name)
+ repository.commit_dir(
+ current_user,
+ @file_path,
+ @commit_message,
+ @target_branch,
+ author_email: @author_email,
+ author_name: @author_name,
+ source_branch: @source_branch)
end
def validate
diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index bf127843d55..6d0a0f2629d 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -3,7 +3,16 @@ require_relative "base_service"
module Files
class CreateService < Files::BaseService
def commit
- repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch, false, author_email: @author_email, author_name: @author_name)
+ repository.commit_file(
+ current_user,
+ @file_path,
+ @file_content,
+ @commit_message,
+ @target_branch,
+ false,
+ author_email: @author_email,
+ author_name: @author_name,
+ source_branch: @source_branch)
end
def validate
diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb
index 8b27ad51789..79d592731e9 100644
--- a/app/services/files/delete_service.rb
+++ b/app/services/files/delete_service.rb
@@ -3,7 +3,14 @@ require_relative "base_service"
module Files
class DeleteService < Files::BaseService
def commit
- repository.remove_file(current_user, @file_path, @commit_message, @target_branch, author_email: @author_email, author_name: @author_name)
+ repository.remove_file(
+ current_user,
+ @file_path,
+ @commit_message,
+ @target_branch,
+ author_email: @author_email,
+ author_name: @author_name,
+ source_branch: @source_branch)
end
end
end
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index d28912e1301..0550dec15a6 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -11,7 +11,8 @@ module Files
message: @commit_message,
actions: params[:actions],
author_email: @author_email,
- author_name: @author_name
+ author_name: @author_name,
+ source_branch: @source_branch
)
end
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index c17fdb8d1f1..f3a766ed9fd 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -10,7 +10,8 @@ module Files
previous_path: @previous_path,
message: @commit_message,
author_email: @author_email,
- author_name: @author_name)
+ author_name: @author_name,
+ source_branch: @source_branch)
end
private
diff --git a/app/services/validate_new_branch_service.rb b/app/services/validate_new_branch_service.rb
new file mode 100644
index 00000000000..2f61be184ce
--- /dev/null
+++ b/app/services/validate_new_branch_service.rb
@@ -0,0 +1,22 @@
+require_relative 'base_service'
+
+class ValidateNewBranchService < BaseService
+ def execute(branch_name)
+ valid_branch = Gitlab::GitRefValidator.validate(branch_name)
+
+ unless valid_branch
+ return error('Branch name is invalid')
+ end
+
+ repository = project.repository
+ existing_branch = repository.find_branch(branch_name)
+
+ if existing_branch
+ return error('Branch already exists')
+ end
+
+ success
+ rescue GitHooksService::PreReceiveError => ex
+ error(ex.message)
+ end
+end
--
GitLab
From 92a438263fafdd7c4163f09e63815dc805cb8d12 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 04:27:21 +0800
Subject: [PATCH 05/78] Fix issues found by rubocop
---
app/models/repository.rb | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index b6581486983..beafe9060bf 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -786,7 +786,8 @@ class Repository
@root_ref ||= cache.fetch(:root_ref) { raw_repository.root_ref }
end
- def commit_dir(user, path, message, branch,
+ def commit_dir(
+ user, path, message, branch,
author_email: nil, author_name: nil, source_branch: nil)
update_branch_with_hooks(
user,
@@ -806,7 +807,9 @@ class Repository
end
end
- def commit_file(user, path, content, message, branch, update,
+ # rubocop:disable Metrics/ParameterLists
+ def commit_file(
+ user, path, content, message, branch, update,
author_email: nil, author_name: nil, source_branch: nil)
update_branch_with_hooks(
user,
@@ -830,8 +833,11 @@ class Repository
Gitlab::Git::Blob.commit(raw_repository, options)
end
end
+ # rubocop:enable Metrics/ParameterLists
- def update_file(user, path, content,
+ # rubocop:disable Metrics/ParameterLists
+ def update_file(
+ user, path, content,
branch:, previous_path:, message:,
author_email: nil, author_name: nil, source_branch: nil)
update_branch_with_hooks(
@@ -861,8 +867,10 @@ class Repository
end
end
end
+ # rubocop:enable Metrics/ParameterLists
- def remove_file(user, path, message, branch,
+ def remove_file(
+ user, path, message, branch,
author_email: nil, author_name: nil, source_branch: nil)
update_branch_with_hooks(
user,
@@ -885,14 +893,14 @@ class Repository
end
end
- def multi_action(user:, branch:, message:, actions:,
+ def multi_action(
+ user:, branch:, message:, actions:,
author_email: nil, author_name: nil, source_branch: nil)
update_branch_with_hooks(
user,
branch,
source_branch: source_branch) do |ref|
index = rugged.index
- parents = []
last_commit = find_branch(ref).dereferenced_target
index.read_tree(last_commit.raw_commit.tree)
--
GitLab
From 8fca786bdee6fa23a5e000bf23e65b153fc1bf73 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 04:46:54 +0800
Subject: [PATCH 06/78] They're never referred
---
app/models/repository.rb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index beafe9060bf..b2b5d528840 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -998,7 +998,7 @@ class Repository
base_branch,
source_branch: revert_tree_id) do
committer = user_to_committer(user)
- source_sha = Rugged::Commit.create(rugged,
+ Rugged::Commit.create(rugged,
message: commit.revert_message,
author: committer,
committer: committer,
@@ -1018,7 +1018,7 @@ class Repository
base_branch,
source_branch: cherry_pick_tree_id) do
committer = user_to_committer(user)
- source_sha = Rugged::Commit.create(rugged,
+ Rugged::Commit.create(rugged,
message: commit.message,
author: {
email: commit.author_email,
--
GitLab
From 30d7b5c32cfb97d7c1e57fb8874069077097c89d Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 05:29:24 +0800
Subject: [PATCH 07/78] Fix the case when it's a whole new branch
---
app/models/repository.rb | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index b2b5d528840..5e7bb309967 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1128,7 +1128,7 @@ class Repository
raise CommitError.new('Failed to create commit')
end
- if rugged.lookup(newrev).parent_ids.empty?
+ if rugged.lookup(newrev).parent_ids.empty? || target_branch.nil?
oldrev = Gitlab::Git::BLANK_SHA
else
oldrev = rugged.merge_base(newrev, target_branch.dereferenced_target.sha)
@@ -1219,8 +1219,7 @@ class Repository
[find_branch(branch_name), true]
else
- raise CommitError.new(
- "Cannot find branch #{branch_name} and source_branch is not set")
+ [nil, true] # Empty branch
end
end
end
--
GitLab
From eddee5fe8770d79c80fdb0d91731f866c14c9b8d Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 06:01:54 +0800
Subject: [PATCH 08/78] Make sure we create target branch for cherry/revert
---
app/models/repository.rb | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 5e7bb309967..0f3e98db420 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -988,7 +988,8 @@ class Repository
end
def revert(user, commit, base_branch, revert_tree_id = nil)
- source_sha = find_branch(base_branch).dereferenced_target.sha
+ source_sha = raw_ensure_branch(base_branch, source_commit: commit).
+ first.dereferenced_target.sha
revert_tree_id ||= check_revert_content(commit, base_branch)
return false unless revert_tree_id
@@ -1008,7 +1009,8 @@ class Repository
end
def cherry_pick(user, commit, base_branch, cherry_pick_tree_id = nil)
- source_sha = find_branch(base_branch).dereferenced_target.sha
+ source_sha = raw_ensure_branch(base_branch, source_commit: commit).
+ first.dereferenced_target.sha
cherry_pick_tree_id ||= check_cherry_pick_content(commit, base_branch)
return false unless cherry_pick_tree_id
@@ -1118,7 +1120,8 @@ class Repository
update_autocrlf_option
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
- target_branch, new_branch_added = raw_ensure_branch(branch, source_branch)
+ target_branch, new_branch_added =
+ raw_ensure_branch(branch, source_branch: source_branch)
was_empty = empty?
# Make commit
@@ -1200,19 +1203,19 @@ class Repository
Gitlab::Metrics.add_event(event, { path: path_with_namespace }.merge(tags))
end
- def raw_ensure_branch(branch_name, source_branch)
+ def raw_ensure_branch(branch_name, source_commit: nil, source_branch: nil)
old_branch = find_branch(branch_name)
if old_branch
[old_branch, false]
- elsif source_branch
+ elsif source_commit || source_branch
oldrev = Gitlab::Git::BLANK_SHA
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
- target = commit(source_branch).try(:id)
+ target = (source_commit || commit(source_branch)).try(:sha)
unless target
raise CommitError.new(
- "Cannot find branch #{branch_name} nor #{source_branch}")
+ "Cannot find branch #{branch_name} nor #{source_commit.try(:sha) ||source_branch}")
end
update_ref!(ref, target, oldrev)
--
GitLab
From 30bcc3de41d86e02c27940e0e8e4a1a82183520e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 06:18:24 +0800
Subject: [PATCH 09/78] Add missing space due to Sublime Text
It's Sublime Text's fault. Its word wrapping is not code friendly
---
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 0f3e98db420..89293fa8b4d 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1215,7 +1215,7 @@ class Repository
unless target
raise CommitError.new(
- "Cannot find branch #{branch_name} nor #{source_commit.try(:sha) ||source_branch}")
+ "Cannot find branch #{branch_name} nor #{source_commit.try(:sha) || source_branch}")
end
update_ref!(ref, target, oldrev)
--
GitLab
From 5de74551ace7b6df9fdb2a3c8aa30c836d693728 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 06:55:54 +0800
Subject: [PATCH 10/78] Branch could be nil if it's an empty repo
---
app/models/repository.rb | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 89293fa8b4d..c4bdc84348e 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -901,10 +901,15 @@ class Repository
branch,
source_branch: source_branch) do |ref|
index = rugged.index
-
- last_commit = find_branch(ref).dereferenced_target
- index.read_tree(last_commit.raw_commit.tree)
- parents = [last_commit.sha]
+ branch_commit = find_branch(ref)
+
+ parents = if branch_commit
+ last_commit = branch_commit.dereferenced_target
+ index.read_tree(last_commit.raw_commit.tree)
+ [last_commit.sha]
+ else
+ []
+ end
actions.each do |action|
case action[:action]
--
GitLab
From d8fe2fac7e681ddbff3c7a5338f939eb2d540e38 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 07:22:50 +0800
Subject: [PATCH 11/78] Make sure we have the branch on the other project
---
app/models/project.rb | 11 +++++++++++
app/services/create_branch_service.rb | 10 +---------
app/services/files/base_service.rb | 10 +++++++++-
3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/app/models/project.rb b/app/models/project.rb
index 94aabafce20..1208e5da6fa 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -988,6 +988,17 @@ class Project < ActiveRecord::Base
Gitlab::UploadsTransfer.new.rename_project(path_was, path, namespace.path)
end
+ def fetch_ref(source_project, branch_name, ref)
+ repository.fetch_ref(
+ source_project.repository.path_to_repo,
+ "refs/heads/#{ref}",
+ "refs/heads/#{branch_name}"
+ )
+
+ repository.after_create_branch
+ repository.find_branch(branch_name)
+ end
+
# Expires various caches before a project is renamed.
def expire_caches_before_rename(old_path)
repo = Repository.new(old_path, self)
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index f4270a09928..ecb5994fab8 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -7,15 +7,7 @@ class CreateBranchService < BaseService
return failure if failure
new_branch = if source_project != @project
- repository.fetch_ref(
- source_project.repository.path_to_repo,
- "refs/heads/#{ref}",
- "refs/heads/#{branch_name}"
- )
-
- repository.after_create_branch
-
- repository.find_branch(branch_name)
+ @project.fetch_ref(source_project, branch_name, ref)
else
repository.add_branch(current_user, branch_name, ref)
end
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 6779bd2818a..fd62246eddb 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -23,7 +23,7 @@ module Files
validate
# Create new branch if it different from source_branch
- validate_target_branch if different_branch?
+ ensure_target_branch if different_branch?
result = commit
if result
@@ -71,6 +71,14 @@ module Files
end
end
+ def ensure_target_branch
+ validate_target_branch
+
+ if @source_project != project
+ @project.fetch_ref(@source_project, @target_branch, @source_branch)
+ end
+ end
+
def validate_target_branch
result = ValidateNewBranchService.new(project, current_user).
execute(@target_branch)
--
GitLab
From a68a62011d03c15d6116dc1e6dcb9514040a51f5 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 07:53:36 +0800
Subject: [PATCH 12/78] Don't pass source_branch if it doesn't exist
---
app/controllers/concerns/creates_commit.rb | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index dacb5679dd3..643b61af1b2 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,9 +4,10 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
+ source_branch = @ref if @repository.find_branch(@ref)
commit_params = @commit_params.merge(
source_project: @project,
- source_branch: @ref,
+ source_branch: source_branch,
target_branch: @target_branch
)
--
GitLab
From 39d83f72e7af78d503ef278e22eda25f90322f4b Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 08:10:32 +0800
Subject: [PATCH 13/78] Add a few comments to explain implementation detail
---
app/models/repository.rb | 2 ++
app/services/files/base_service.rb | 3 +++
2 files changed, 5 insertions(+)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index c4bdc84348e..4e3e70192ab 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1121,6 +1121,8 @@ class Repository
fetch_ref(path_to_repo, ref, ref_path)
end
+ # Whenever `source_branch` is passed, if `branch` doesn't exist, it would
+ # be created from `source_branch`.
def update_branch_with_hooks(current_user, branch, source_branch: nil)
update_autocrlf_option
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index fd62246eddb..8689c83d40e 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -75,6 +75,9 @@ module Files
validate_target_branch
if @source_project != project
+ # Make sure we have the source_branch in target project,
+ # and use source_branch as target_branch directly to avoid
+ # unnecessary source branch in target project.
@project.fetch_ref(@source_project, @target_branch, @source_branch)
end
end
--
GitLab
From cd22fdd531c151cf1bbc307e52c565e86070fe3b Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 08:21:16 +0800
Subject: [PATCH 14/78] @ref might not exist
---
app/controllers/concerns/creates_commit.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 643b61af1b2..5d11f286e9a 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,7 +4,7 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
- source_branch = @ref if @repository.find_branch(@ref)
+ source_branch = @ref if @ref && @repository.find_branch(@ref)
commit_params = @commit_params.merge(
source_project: @project,
source_branch: source_branch,
--
GitLab
From 3e69c716f0b81b2c3b863d3c8a5e6346b9978fc1 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 Nov 2016 20:03:18 +0800
Subject: [PATCH 15/78] Try if branch_exists? would work, feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_18424135
---
app/controllers/concerns/creates_commit.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 5d11f286e9a..e5c40446314 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,7 +4,7 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
- source_branch = @ref if @ref && @repository.find_branch(@ref)
+ source_branch = @ref if @ref && @repository.branch_exists?(@ref)
commit_params = @commit_params.merge(
source_project: @project,
source_branch: source_branch,
--
GitLab
From b82f415f09dd67da010a8c08397a13499e70efeb Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 22 Nov 2016 18:14:41 +0800
Subject: [PATCH 16/78] Move all branch creation to raw_ensure_branch,
and keep it only called in update_branch_with_hooks.
---
app/models/project.rb | 11 ----
app/models/repository.rb | 85 ++++++++++++++++++++-------
app/services/create_branch_service.rb | 8 +--
app/services/files/base_service.rb | 13 +---
app/services/files/update_service.rb | 1 +
5 files changed, 69 insertions(+), 49 deletions(-)
diff --git a/app/models/project.rb b/app/models/project.rb
index 5346e18b051..f8a54324341 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -998,17 +998,6 @@ class Project < ActiveRecord::Base
Gitlab::UploadsTransfer.new.rename_project(path_was, path, namespace.path)
end
- def fetch_ref(source_project, branch_name, ref)
- repository.fetch_ref(
- source_project.repository.path_to_repo,
- "refs/heads/#{ref}",
- "refs/heads/#{branch_name}"
- )
-
- repository.after_create_branch
- repository.find_branch(branch_name)
- end
-
# Expires various caches before a project is renamed.
def expire_caches_before_rename(old_path)
repo = Repository.new(old_path, self)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index a029993db7f..9162f494a60 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -784,13 +784,16 @@ class Repository
@tags ||= raw_repository.tags
end
+ # rubocop:disable Metrics/ParameterLists
def commit_dir(
user, path, message, branch,
- author_email: nil, author_name: nil, source_branch: nil)
+ author_email: nil, author_name: nil,
+ source_branch: nil, source_project: project)
update_branch_with_hooks(
user,
branch,
- source_branch: source_branch) do |ref|
+ source_branch: source_branch,
+ source_project: source_project) do |ref|
options = {
commit: {
branch: ref,
@@ -804,15 +807,18 @@ class Repository
raw_repository.mkdir(path, options)
end
end
+ # rubocop:enable Metrics/ParameterLists
# rubocop:disable Metrics/ParameterLists
def commit_file(
user, path, content, message, branch, update,
- author_email: nil, author_name: nil, source_branch: nil)
+ author_email: nil, author_name: nil,
+ source_branch: nil, source_project: project)
update_branch_with_hooks(
user,
branch,
- source_branch: source_branch) do |ref|
+ source_branch: source_branch,
+ source_project: source_project) do |ref|
options = {
commit: {
branch: ref,
@@ -837,11 +843,13 @@ class Repository
def update_file(
user, path, content,
branch:, previous_path:, message:,
- author_email: nil, author_name: nil, source_branch: nil)
+ author_email: nil, author_name: nil,
+ source_branch: nil, source_project: project)
update_branch_with_hooks(
user,
branch,
- source_branch: source_branch) do |ref|
+ source_branch: source_branch,
+ source_project: source_project) do |ref|
options = {
commit: {
branch: ref,
@@ -867,13 +875,16 @@ class Repository
end
# rubocop:enable Metrics/ParameterLists
+ # rubocop:disable Metrics/ParameterLists
def remove_file(
user, path, message, branch,
- author_email: nil, author_name: nil, source_branch: nil)
+ author_email: nil, author_name: nil,
+ source_branch: nil, source_project: project)
update_branch_with_hooks(
user,
branch,
- source_branch: source_branch) do |ref|
+ source_branch: source_branch,
+ source_project: source_project) do |ref|
options = {
commit: {
branch: ref,
@@ -890,14 +901,18 @@ class Repository
Gitlab::Git::Blob.remove(raw_repository, options)
end
end
+ # rubocop:enable Metrics/ParameterLists
+ # rubocop:disable Metrics/ParameterLists
def multi_action(
user:, branch:, message:, actions:,
- author_email: nil, author_name: nil, source_branch: nil)
+ author_email: nil, author_name: nil,
+ source_branch: nil, source_project: project)
update_branch_with_hooks(
user,
branch,
- source_branch: source_branch) do |ref|
+ source_branch: source_branch,
+ source_project: source_project) do |ref|
index = rugged.index
branch_commit = find_branch(ref)
@@ -942,6 +957,7 @@ class Repository
Rugged::Commit.create(rugged, options)
end
end
+ # rubocop:enable Metrics/ParameterLists
def get_committer_and_author(user, email: nil, name: nil)
committer = user_to_committer(user)
@@ -991,8 +1007,6 @@ class Repository
end
def revert(user, commit, base_branch, revert_tree_id = nil)
- source_sha = raw_ensure_branch(base_branch, source_commit: commit).
- first.dereferenced_target.sha
revert_tree_id ||= check_revert_content(commit, base_branch)
return false unless revert_tree_id
@@ -1000,8 +1014,11 @@ class Repository
update_branch_with_hooks(
user,
base_branch,
- source_branch: revert_tree_id) do
+ source_commit: commit) do
+
+ source_sha = find_branch(base_branch).dereferenced_target.sha
committer = user_to_committer(user)
+
Rugged::Commit.create(rugged,
message: commit.revert_message,
author: committer,
@@ -1012,8 +1029,6 @@ class Repository
end
def cherry_pick(user, commit, base_branch, cherry_pick_tree_id = nil)
- source_sha = raw_ensure_branch(base_branch, source_commit: commit).
- first.dereferenced_target.sha
cherry_pick_tree_id ||= check_cherry_pick_content(commit, base_branch)
return false unless cherry_pick_tree_id
@@ -1021,8 +1036,11 @@ class Repository
update_branch_with_hooks(
user,
base_branch,
- source_branch: cherry_pick_tree_id) do
+ source_commit: commit) do
+
+ source_sha = find_branch(base_branch).dereferenced_target.sha
committer = user_to_committer(user)
+
Rugged::Commit.create(rugged,
message: commit.message,
author: {
@@ -1130,12 +1148,19 @@ class Repository
# Whenever `source_branch` is passed, if `branch` doesn't exist, it would
# be created from `source_branch`.
- def update_branch_with_hooks(current_user, branch, source_branch: nil)
+ def update_branch_with_hooks(
+ current_user, branch,
+ source_branch: nil, source_commit: nil, source_project: project)
update_autocrlf_option
- ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
target_branch, new_branch_added =
- raw_ensure_branch(branch, source_branch: source_branch)
+ raw_ensure_branch(
+ branch,
+ source_branch: source_branch,
+ source_project: source_project
+ )
+
+ ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
was_empty = empty?
# Make commit
@@ -1244,11 +1269,31 @@ class Repository
Gitlab::Metrics.add_event(event, { path: path_with_namespace }.merge(tags))
end
- def raw_ensure_branch(branch_name, source_commit: nil, source_branch: nil)
+ def raw_ensure_branch(
+ branch_name, source_commit: nil, source_branch: nil, source_project: nil)
old_branch = find_branch(branch_name)
+ if source_commit && source_branch
+ raise ArgumentError,
+ 'Should only pass either :source_branch or :source_commit, not both'
+ end
+
if old_branch
[old_branch, false]
+ elsif project != source_project
+ unless source_branch
+ raise ArgumentError,
+ 'Should also pass :source_branch if' +
+ ' :source_project is different from current project'
+ end
+
+ fetch_ref(
+ source_project.repository.path_to_repo,
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch}",
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch}"
+ )
+
+ [find_branch(branch_name), true]
elsif source_commit || source_branch
oldrev = Gitlab::Git::BLANK_SHA
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index ecb5994fab8..b2bc3626c0f 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -1,16 +1,12 @@
require_relative 'base_service'
class CreateBranchService < BaseService
- def execute(branch_name, ref, source_project: @project)
+ def execute(branch_name, ref)
failure = validate_new_branch(branch_name)
return failure if failure
- new_branch = if source_project != @project
- @project.fetch_ref(source_project, branch_name, ref)
- else
- repository.add_branch(current_user, branch_name, ref)
- end
+ new_branch = repository.add_branch(current_user, branch_name, ref)
if new_branch
success(new_branch)
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 8689c83d40e..6779bd2818a 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -23,7 +23,7 @@ module Files
validate
# Create new branch if it different from source_branch
- ensure_target_branch if different_branch?
+ validate_target_branch if different_branch?
result = commit
if result
@@ -71,17 +71,6 @@ module Files
end
end
- def ensure_target_branch
- validate_target_branch
-
- if @source_project != project
- # Make sure we have the source_branch in target project,
- # and use source_branch as target_branch directly to avoid
- # unnecessary source branch in target project.
- @project.fetch_ref(@source_project, @target_branch, @source_branch)
- end
- end
-
def validate_target_branch
result = ValidateNewBranchService.new(project, current_user).
execute(@target_branch)
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index f3a766ed9fd..14e5af4d8c6 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -11,6 +11,7 @@ module Files
message: @commit_message,
author_email: @author_email,
author_name: @author_name,
+ source_project: @source_project,
source_branch: @source_branch)
end
--
GitLab
From 6cd2256c84c9a785a6b15864ab170084fdebf45f Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 24 Nov 2016 17:56:26 +0800
Subject: [PATCH 17/78] Should also pass source_commit to raw_ensure_branch
---
app/models/repository.rb | 1 +
1 file changed, 1 insertion(+)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 9162f494a60..276d8829873 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1157,6 +1157,7 @@ class Repository
raw_ensure_branch(
branch,
source_branch: source_branch,
+ source_commit: source_commit,
source_project: source_project
)
--
GitLab
From e866985be81f54002667117e03dae6680829a462 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 24 Nov 2016 23:35:28 +0800
Subject: [PATCH 18/78] Fix local name from branch to branch_name
---
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 276d8829873..6abe498ebde 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1291,7 +1291,7 @@ class Repository
fetch_ref(
source_project.repository.path_to_repo,
"#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch}",
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch}"
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch_name}"
)
[find_branch(branch_name), true]
--
GitLab
From c916f293cecf3d112f0338aec0a9dc9e3577f17e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 25 Nov 2016 15:07:27 +0800
Subject: [PATCH 19/78] Add some explanation to
Repository#update_branch_with_hooks
---
app/models/repository.rb | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6abe498ebde..09e8cc060c7 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1146,8 +1146,11 @@ class Repository
fetch_ref(path_to_repo, ref, ref_path)
end
- # Whenever `source_branch` is passed, if `branch` doesn't exist, it would
- # be created from `source_branch`.
+ # Whenever `source_branch` or `source_commit` is passed, if `branch`
+ # doesn't exist, it would be created from `source_branch` or
+ # `source_commit`. Should only pass one of them, not both.
+ # If `source_project` is passed, and the branch doesn't exist,
+ # it would try to find the source from it instead of current repository.
def update_branch_with_hooks(
current_user, branch,
source_branch: nil, source_commit: nil, source_project: project)
--
GitLab
From a52dc7cec70ef97b2755fb9cef7d6b489062310c Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Dec 2016 03:13:15 +0800
Subject: [PATCH 20/78] Introduce GitOperationService and wrap every
git operation inside GitHooksService. Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19210942
TODO: Fix tests for update_branch_with_hooks
---
app/models/repository.rb | 174 ++++----------------------
app/services/git_operation_service.rb | 168 +++++++++++++++++++++++++
spec/models/repository_spec.rb | 12 +-
3 files changed, 197 insertions(+), 157 deletions(-)
create mode 100644 app/services/git_operation_service.rb
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6bfa24f6329..491d2ab69b2 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -5,7 +5,7 @@ class Repository
attr_accessor :path_with_namespace, :project
- class CommitError < StandardError; end
+ CommitError = Class.new(StandardError)
# Methods that cache data from the Git repository.
#
@@ -64,10 +64,6 @@ class Repository
@raw_repository ||= Gitlab::Git::Repository.new(path_to_repo)
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(
@@ -172,54 +168,39 @@ class Repository
tags.find { |tag| tag.name == name }
end
- def add_branch(user, branch_name, target)
- oldrev = Gitlab::Git::BLANK_SHA
- ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
- target = commit(target).try(:id)
+ def add_branch(user, branch_name, ref)
+ newrev = commit(ref).try(:sha)
- return false unless target
+ return false unless newrev
- GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do
- update_ref!(ref, target, oldrev)
- end
+ GitOperationService.new(user, self).add_branch(branch_name, newrev)
after_create_branch
find_branch(branch_name)
end
def add_tag(user, tag_name, target, message = nil)
- oldrev = Gitlab::Git::BLANK_SHA
- ref = Gitlab::Git::TAG_REF_PREFIX + tag_name
- target = commit(target).try(:id)
-
- return false unless target
-
+ newrev = commit(target).try(:id)
options = { message: message, tagger: user_to_committer(user) } if message
- GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do |service|
- raw_tag = rugged.tags.create(tag_name, target, options)
- service.newrev = raw_tag.target_id
- end
+ return false unless newrev
+
+ GitOperationService.new(user, self).add_tag(tag_name, newrev, options)
find_tag(tag_name)
end
def rm_branch(user, branch_name)
before_remove_branch
-
branch = find_branch(branch_name)
- oldrev = branch.try(:dereferenced_target).try(:id)
- newrev = Gitlab::Git::BLANK_SHA
- ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
- GitHooksService.new.execute(user, path_to_repo, oldrev, newrev, ref) do
- update_ref!(ref, newrev, oldrev)
- end
+ GitOperationService.new(user, self).rm_branch(branch)
after_remove_branch
true
end
+ # TODO: why we don't pass user here?
def rm_tag(tag_name)
before_remove_tag
@@ -245,21 +226,6 @@ class Repository
false
end
- def update_ref!(name, newrev, oldrev)
- # We use 'git update-ref' because libgit2/rugged currently does not
- # offer 'compare and swap' ref updates. Without compare-and-swap we can
- # (and have!) accidentally reset the ref to an earlier state, clobbering
- # commits. See also https://github.com/libgit2/libgit2/issues/1534.
- command = %W(#{Gitlab.config.git.bin_path} update-ref --stdin -z)
- _, status = Gitlab::Popen.popen(command, path_to_repo) do |stdin|
- stdin.write("update #{name}\x00#{newrev}\x00#{oldrev}\x00")
- end
-
- return if status.zero?
-
- raise CommitError.new("Could not update branch #{name.sub('refs/heads/', '')}. Please refresh and try again.")
- end
-
# Makes sure a commit is kept around when Git garbage collection runs.
# Git GC will delete commits from the repository that are no longer in any
# branches or tags, but we want to keep some of these commits around, for
@@ -783,8 +749,7 @@ class Repository
user, path, message, branch,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- update_branch_with_hooks(
- user,
+ GitOperationService.new(user, self).with_branch(
branch,
source_branch: source_branch,
source_project: source_project) do |ref|
@@ -808,8 +773,7 @@ class Repository
user, path, content, message, branch, update,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- update_branch_with_hooks(
- user,
+ GitOperationService.new(user, self).with_branch(
branch,
source_branch: source_branch,
source_project: source_project) do |ref|
@@ -839,8 +803,7 @@ class Repository
branch:, previous_path:, message:,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- update_branch_with_hooks(
- user,
+ GitOperationService.new(user, self).with_branch(
branch,
source_branch: source_branch,
source_project: source_project) do |ref|
@@ -874,8 +837,7 @@ class Repository
user, path, message, branch,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- update_branch_with_hooks(
- user,
+ GitOperationService.new(user, self).with_branch(
branch,
source_branch: source_branch,
source_project: source_project) do |ref|
@@ -902,8 +864,7 @@ class Repository
user:, branch:, message:, actions:,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- update_branch_with_hooks(
- user,
+ GitOperationService.new(user, self).with_branch(
branch,
source_branch: source_branch,
source_project: source_project) do |ref|
@@ -964,7 +925,7 @@ class Repository
end
def user_to_committer(user)
- Gitlab::Git::committer_hash(email: user.email, name: user.name)
+ Gitlab::Git.committer_hash(email: user.email, name: user.name)
end
def can_be_merged?(source_sha, target_branch)
@@ -988,7 +949,8 @@ class Repository
merge_index = rugged.merge_commits(our_commit, their_commit)
return false if merge_index.conflicts?
- update_branch_with_hooks(user, merge_request.target_branch) do
+ GitOperationService.new(user, self).with_branch(
+ merge_request.target_branch) do
actual_options = options.merge(
parents: [our_commit, their_commit],
tree: merge_index.write_tree(rugged),
@@ -1005,8 +967,7 @@ class Repository
return false unless revert_tree_id
- update_branch_with_hooks(
- user,
+ GitOperationService.new(user, self).with_branch(
base_branch,
source_commit: commit) do
@@ -1027,8 +988,7 @@ class Repository
return false unless cherry_pick_tree_id
- update_branch_with_hooks(
- user,
+ GitOperationService.new(user, self).with_branch(
base_branch,
source_commit: commit) do
@@ -1048,8 +1008,8 @@ class Repository
end
end
- def resolve_conflicts(user, branch, params)
- update_branch_with_hooks(user, branch) do
+ def resolve_conflicts(user, branch_name, params)
+ GitOperationService.new(user, self).with_branch(branch_name) do
committer = user_to_committer(user)
Rugged::Commit.create(rugged, params.merge(author: committer, committer: committer))
@@ -1140,51 +1100,6 @@ class Repository
fetch_ref(path_to_repo, ref, ref_path)
end
- # Whenever `source_branch` or `source_commit` is passed, if `branch`
- # doesn't exist, it would be created from `source_branch` or
- # `source_commit`. Should only pass one of them, not both.
- # If `source_project` is passed, and the branch doesn't exist,
- # it would try to find the source from it instead of current repository.
- def update_branch_with_hooks(
- current_user, branch,
- source_branch: nil, source_commit: nil, source_project: project)
- update_autocrlf_option
-
- target_branch, new_branch_added =
- raw_ensure_branch(
- branch,
- source_branch: source_branch,
- source_commit: source_commit,
- source_project: source_project
- )
-
- ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
- was_empty = empty?
-
- # Make commit
- newrev = yield(ref)
-
- unless newrev
- raise CommitError.new('Failed to create commit')
- end
-
- if rugged.lookup(newrev).parent_ids.empty? || target_branch.nil?
- oldrev = Gitlab::Git::BLANK_SHA
- else
- oldrev = rugged.merge_base(newrev, target_branch.dereferenced_target.sha)
- end
-
- GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do
- update_ref!(ref, newrev, oldrev)
-
- # If repo was empty expire cache
- after_create if was_empty
- after_create_branch if was_empty || new_branch_added
- end
-
- newrev
- end
-
def ls_files(ref)
actual_ref = ref || root_ref
raw_repository.ls_files(actual_ref)
@@ -1266,47 +1181,4 @@ class Repository
def repository_event(event, tags = {})
Gitlab::Metrics.add_event(event, { path: path_with_namespace }.merge(tags))
end
-
- def raw_ensure_branch(
- branch_name, source_commit: nil, source_branch: nil, source_project: nil)
- old_branch = find_branch(branch_name)
-
- if source_commit && source_branch
- raise ArgumentError,
- 'Should only pass either :source_branch or :source_commit, not both'
- end
-
- if old_branch
- [old_branch, false]
- elsif project != source_project
- unless source_branch
- raise ArgumentError,
- 'Should also pass :source_branch if' +
- ' :source_project is different from current project'
- end
-
- fetch_ref(
- source_project.repository.path_to_repo,
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch}",
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch_name}"
- )
-
- [find_branch(branch_name), true]
- elsif source_commit || source_branch
- oldrev = Gitlab::Git::BLANK_SHA
- ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
- target = (source_commit || commit(source_branch)).try(:sha)
-
- unless target
- raise CommitError.new(
- "Cannot find branch #{branch_name} nor #{source_commit.try(:sha) || source_branch}")
- end
-
- update_ref!(ref, target, oldrev)
-
- [find_branch(branch_name), true]
- else
- [nil, true] # Empty branch
- end
- end
end
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
new file mode 100644
index 00000000000..88175c6931d
--- /dev/null
+++ b/app/services/git_operation_service.rb
@@ -0,0 +1,168 @@
+
+GitOperationService = Struct.new(:user, :repository) do
+ def add_branch(branch_name, newrev)
+ ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
+ oldrev = Gitlab::Git::BLANK_SHA
+
+ with_hooks_and_update_ref(ref, oldrev, newrev)
+ end
+
+ def rm_branch(branch)
+ ref = Gitlab::Git::BRANCH_REF_PREFIX + branch.name
+ oldrev = branch.dereferenced_target.id
+ newrev = Gitlab::Git::BLANK_SHA
+
+ with_hooks_and_update_ref(ref, oldrev, newrev)
+ end
+
+ def add_tag(tag_name, newrev, options = {})
+ ref = Gitlab::Git::TAG_REF_PREFIX + tag_name
+ oldrev = Gitlab::Git::BLANK_SHA
+
+ with_hooks(ref, oldrev, newrev) do |service|
+ raw_tag = repository.rugged.tags.create(tag_name, newrev, options)
+ service.newrev = raw_tag.target_id
+ end
+ end
+
+ # Whenever `source_branch` or `source_commit` is passed, if `branch`
+ # doesn't exist, it would be created from `source_branch` or
+ # `source_commit`. Should only pass one of them, not both.
+ # If `source_project` is passed, and the branch doesn't exist,
+ # it would try to find the source from it instead of current repository.
+ def with_branch(
+ branch_name,
+ source_branch: nil,
+ source_commit: nil,
+ source_project: repository.project)
+
+ if source_commit && source_branch
+ raise ArgumentError, 'Should pass only :source_branch or :source_commit'
+ end
+
+ ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
+ oldrev = Gitlab::Git::BLANK_SHA
+
+ if repository.branch_exists?(branch_name)
+ oldrev = newrev = repository.commit(branch_name).sha
+
+ elsif repository.project != source_project
+ unless source_branch
+ raise ArgumentError,
+ 'Should also pass :source_branch if' +
+ ' :source_project is different from current project'
+ end
+
+ newrev = source_project.repository.commit(source_branch).try(:sha)
+
+ unless newrev
+ raise Repository::CommitError.new(
+ "Cannot find branch #{branch_name} nor" \
+ " #{source_branch} from" \
+ " #{source_project.path_with_namespace}")
+ end
+
+ elsif source_commit || source_branch
+ newrev = (source_commit || repository.commit(source_branch)).try(:sha)
+
+ unless newrev
+ raise Repository::CommitError.new(
+ "Cannot find branch #{branch_name} nor" \
+ " #{source_commit.try(:sha) || source_branch} from" \
+ " #{repository.project.path_with_namespace}")
+ end
+
+ else # we want an orphan empty branch
+ newrev = Gitlab::Git::BLANK_SHA
+ end
+
+ commit_with_hooks(ref, oldrev, newrev) do
+ if repository.project != source_project
+ repository.fetch_ref(
+ source_project.repository.path_to_repo,
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch}",
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch_name}"
+ )
+ end
+
+ yield(ref)
+ end
+ end
+
+ private
+
+ def commit_with_hooks(ref, oldrev, newrev)
+ with_hooks_and_update_ref(ref, oldrev, newrev) do |service|
+ was_empty = repository.empty?
+
+ # Make commit
+ nextrev = yield(ref)
+
+ unless nextrev
+ raise Repository::CommitError.new('Failed to create commit')
+ end
+
+ service.newrev = nextrev
+
+ update_ref!(ref, nextrev, newrev)
+
+ # If repo was empty expire cache
+ repository.after_create if was_empty
+ repository.after_create_branch if was_empty ||
+ oldrev == Gitlab::Git::BLANK_SHA
+
+ nextrev
+ end
+ end
+
+ def with_hooks_and_update_ref(ref, oldrev, newrev)
+ with_hooks(ref, oldrev, newrev) do |service|
+ update_ref!(ref, newrev, oldrev)
+
+ yield(service) if block_given?
+ end
+ end
+
+ def with_hooks(ref, oldrev, newrev)
+ update_autocrlf_option
+
+ result = nil
+
+ GitHooksService.new.execute(
+ user,
+ repository.path_to_repo,
+ oldrev,
+ newrev,
+ ref) do |service|
+
+ result = yield(service) if block_given?
+ end
+
+ result
+ end
+
+ def update_ref!(name, newrev, oldrev)
+ # We use 'git update-ref' because libgit2/rugged currently does not
+ # offer 'compare and swap' ref updates. Without compare-and-swap we can
+ # (and have!) accidentally reset the ref to an earlier state, clobbering
+ # commits. See also https://github.com/libgit2/libgit2/issues/1534.
+ command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z]
+ _, status = Gitlab::Popen.popen(
+ command,
+ repository.path_to_repo) do |stdin|
+ stdin.write("update #{name}\x00#{newrev}\x00#{oldrev}\x00")
+ end
+
+ unless status.zero?
+ raise Repository::CommitError.new(
+ "Could not update branch #{name.sub('refs/heads/', '')}." \
+ " Please refresh and try again.")
+ end
+ end
+
+ def update_autocrlf_option
+ if repository.raw_repository.autocrlf != :input
+ repository.raw_repository.autocrlf = :input
+ end
+ end
+end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index b797d19161d..3ce3c4dec2a 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -667,7 +667,7 @@ describe Repository, models: true do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
expect do
- repository.rm_branch(user, 'new_feature')
+ repository.rm_branch(user, 'feature')
end.to raise_error(GitHooksService::PreReceiveError)
end
@@ -682,7 +682,7 @@ describe Repository, models: true do
end
end
- describe '#update_branch_with_hooks' do
+ xdescribe '#update_branch_with_hooks' do
let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
let(:new_rev) { 'a74ae73c1ccde9b974a70e82b901588071dc142a' } # commit whose parent is old_rev
@@ -848,7 +848,7 @@ describe Repository, models: true do
end
it 'sets autocrlf to :input' do
- repository.update_autocrlf_option
+ GitOperationService.new(nil, repository).send(:update_autocrlf_option)
expect(repository.raw_repository.autocrlf).to eq(:input)
end
@@ -863,7 +863,7 @@ describe Repository, models: true do
expect(repository.raw_repository).not_to receive(:autocrlf=).
with(:input)
- repository.update_autocrlf_option
+ GitOperationService.new(nil, repository).send(:update_autocrlf_option)
end
end
end
@@ -1429,14 +1429,14 @@ describe Repository, models: true do
describe '#update_ref!' do
it 'can create a ref' do
- repository.update_ref!('refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+ GitOperationService.new(nil, repository).send(:update_ref!, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
expect(repository.find_branch('foobar')).not_to be_nil
end
it 'raises CommitError when the ref update fails' do
expect do
- repository.update_ref!('refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+ GitOperationService.new(nil, repository).send(:update_ref!, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
end.to raise_error(Repository::CommitError)
end
end
--
GitLab
From 444da6f47ed77172471a27386b969e6401d7cf84 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Dec 2016 15:20:50 +0800
Subject: [PATCH 21/78] Fix update_ref! call in the test
---
spec/workers/git_garbage_collect_worker_spec.rb | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb
index e471a68a49a..2b31efbf631 100644
--- a/spec/workers/git_garbage_collect_worker_spec.rb
+++ b/spec/workers/git_garbage_collect_worker_spec.rb
@@ -107,7 +107,8 @@ describe GitGarbageCollectWorker do
tree: old_commit.tree,
parents: [old_commit],
)
- project.repository.update_ref!(
+ GitOperationService.new(nil, project.repository).send(
+ :update_ref!,
"refs/heads/#{SecureRandom.hex(6)}",
new_commit_sha,
Gitlab::Git::BLANK_SHA
--
GitLab
From 65806ec632f2ea1e2087b7cdc64f13e6db49c88a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 6 Dec 2016 18:47:24 +0800
Subject: [PATCH 22/78] Re-enable tests for update_branch_with_hooks and
Add back check if we're losing commits in a merge.
---
app/services/git_operation_service.rb | 15 ++++++--
spec/models/repository_spec.rb | 50 ++++++++++++++++++++-------
2 files changed, 51 insertions(+), 14 deletions(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 88175c6931d..c34d4bde150 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -102,9 +102,20 @@ GitOperationService = Struct.new(:user, :repository) do
raise Repository::CommitError.new('Failed to create commit')
end
- service.newrev = nextrev
+ branch =
+ repository.find_branch(ref[Gitlab::Git::BRANCH_REF_PREFIX.size..-1])
+
+ prevrev = if branch &&
+ !repository.rugged.lookup(nextrev).parent_ids.empty?
+ repository.rugged.merge_base(
+ nextrev, branch.dereferenced_target.sha)
+ else
+ newrev
+ end
- update_ref!(ref, nextrev, newrev)
+ update_ref!(ref, nextrev, prevrev)
+
+ service.newrev = nextrev
# If repo was empty expire cache
repository.after_create if was_empty
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 3ce3c4dec2a..e3ec4c85a74 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -682,33 +682,48 @@ describe Repository, models: true do
end
end
- xdescribe '#update_branch_with_hooks' do
+ describe '#update_branch_with_hooks' do
let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
let(:new_rev) { 'a74ae73c1ccde9b974a70e82b901588071dc142a' } # commit whose parent is old_rev
context 'when pre hooks were successful' do
before do
- expect_any_instance_of(GitHooksService).to receive(:execute).
- with(user, repository.path_to_repo, old_rev, new_rev, 'refs/heads/feature').
- and_yield.and_return(true)
+ service = GitHooksService.new
+ expect(GitHooksService).to receive(:new).and_return(service)
+ expect(service).to receive(:execute).
+ with(
+ user,
+ repository.path_to_repo,
+ old_rev,
+ old_rev,
+ 'refs/heads/feature').
+ and_yield(service).and_return(true)
end
it 'runs without errors' do
expect do
- repository.update_branch_with_hooks(user, 'feature') { new_rev }
+ GitOperationService.new(user, repository).with_branch('feature') do
+ new_rev
+ end
end.not_to raise_error
end
it 'ensures the autocrlf Git option is set to :input' do
- expect(repository).to receive(:update_autocrlf_option)
+ service = GitOperationService.new(user, repository)
+
+ expect(service).to receive(:update_autocrlf_option)
- repository.update_branch_with_hooks(user, 'feature') { new_rev }
+ service.with_branch('feature') { new_rev }
end
context "when the branch wasn't empty" do
it 'updates the head' do
expect(repository.find_branch('feature').dereferenced_target.id).to eq(old_rev)
- repository.update_branch_with_hooks(user, 'feature') { new_rev }
+
+ GitOperationService.new(user, repository).with_branch('feature') do
+ new_rev
+ end
+
expect(repository.find_branch('feature').dereferenced_target.id).to eq(new_rev)
end
end
@@ -727,7 +742,11 @@ describe Repository, models: true do
branch = 'feature-ff-target'
repository.add_branch(user, branch, old_rev)
- expect { repository.update_branch_with_hooks(user, branch) { new_rev } }.not_to raise_error
+ expect do
+ GitOperationService.new(user, repository).with_branch(branch) do
+ new_rev
+ end
+ end.not_to raise_error
end
end
@@ -742,7 +761,9 @@ describe Repository, models: true do
# Updating 'master' to new_rev would lose the commits on 'master' that
# are not contained in new_rev. This should not be allowed.
expect do
- repository.update_branch_with_hooks(user, branch) { new_rev }
+ GitOperationService.new(user, repository).with_branch(branch) do
+ new_rev
+ end
end.to raise_error(Repository::CommitError)
end
end
@@ -752,7 +773,9 @@ describe Repository, models: true do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
expect do
- repository.update_branch_with_hooks(user, 'feature') { new_rev }
+ GitOperationService.new(user, repository).with_branch('feature') do
+ new_rev
+ end
end.to raise_error(GitHooksService::PreReceiveError)
end
end
@@ -770,7 +793,10 @@ describe Repository, models: true do
expect(repository).to receive(:expire_branches_cache)
expect(repository).to receive(:expire_has_visible_content_cache)
- repository.update_branch_with_hooks(user, 'new-feature') { new_rev }
+ GitOperationService.new(user, repository).
+ with_branch('new-feature') do
+ new_rev
+ end
end
end
--
GitLab
From 6ae1a73cfdad4b98176bb99846042d4378119de2 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 7 Dec 2016 19:50:08 +0800
Subject: [PATCH 23/78] Pass source_branch properly for cherry-pick/revert
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237/diffs#note_19210818
---
app/models/repository.rb | 12 ++++++++----
app/services/commits/change_service.rb | 9 ++++++++-
app/services/git_operation_service.rb | 16 +++++-----------
3 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 491d2ab69b2..36cbb0d051e 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -962,14 +962,16 @@ class Repository
end
end
- def revert(user, commit, base_branch, revert_tree_id = nil)
+ def revert(
+ user, commit, base_branch, revert_tree_id = nil,
+ source_branch: nil, source_project: project)
revert_tree_id ||= check_revert_content(commit, base_branch)
return false unless revert_tree_id
GitOperationService.new(user, self).with_branch(
base_branch,
- source_commit: commit) do
+ source_branch: source_branch, source_project: source_project) do
source_sha = find_branch(base_branch).dereferenced_target.sha
committer = user_to_committer(user)
@@ -983,14 +985,16 @@ class Repository
end
end
- def cherry_pick(user, commit, base_branch, cherry_pick_tree_id = nil)
+ def cherry_pick(
+ user, commit, base_branch, cherry_pick_tree_id = nil,
+ source_branch: nil, source_project: project)
cherry_pick_tree_id ||= check_cherry_pick_content(commit, base_branch)
return false unless cherry_pick_tree_id
GitOperationService.new(user, self).with_branch(
base_branch,
- source_commit: commit) do
+ source_branch: source_branch, source_project: source_project) do
source_sha = find_branch(base_branch).dereferenced_target.sha
committer = user_to_committer(user)
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 04b28cfaed8..5458f7a6790 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -31,7 +31,14 @@ module Commits
if tree_id
validate_target_branch(into) if @create_merge_request
- repository.public_send(action, current_user, @commit, into, tree_id)
+ repository.public_send(
+ action,
+ current_user,
+ @commit,
+ into,
+ tree_id,
+ source_branch: @target_branch)
+
success
else
error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title} automatically.
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index c34d4bde150..c9e2c21737a 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -25,21 +25,15 @@ GitOperationService = Struct.new(:user, :repository) do
end
end
- # Whenever `source_branch` or `source_commit` is passed, if `branch`
- # doesn't exist, it would be created from `source_branch` or
- # `source_commit`. Should only pass one of them, not both.
+ # Whenever `source_branch` is passed, if `branch` doesn't exist,
+ # it would be created from `source_branch`.
# If `source_project` is passed, and the branch doesn't exist,
# it would try to find the source from it instead of current repository.
def with_branch(
branch_name,
source_branch: nil,
- source_commit: nil,
source_project: repository.project)
- if source_commit && source_branch
- raise ArgumentError, 'Should pass only :source_branch or :source_commit'
- end
-
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
oldrev = Gitlab::Git::BLANK_SHA
@@ -62,13 +56,13 @@ GitOperationService = Struct.new(:user, :repository) do
" #{source_project.path_with_namespace}")
end
- elsif source_commit || source_branch
- newrev = (source_commit || repository.commit(source_branch)).try(:sha)
+ elsif source_branch
+ newrev = repository.commit(source_branch).try(:sha)
unless newrev
raise Repository::CommitError.new(
"Cannot find branch #{branch_name} nor" \
- " #{source_commit.try(:sha) || source_branch} from" \
+ " #{source_branch} from" \
" #{repository.project.path_with_namespace}")
end
--
GitLab
From 5ecd0c81af85476c2328d3836cc68b17ebd5a8a6 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 7 Dec 2016 22:37:43 +0800
Subject: [PATCH 24/78] Commit outside the hooks if possible:
So we still commit outside the hooks, and only
update ref inside the hooks. There are only two
exceptions:
* Whenever it's adding a tag. We can't add a tag
without committing, unfortunately. See !7700
* Whenever source project is in another repository.
We'll need to fetch ref otherwise commits can't be made.
See the whole discussion starting from:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19210942
---
app/models/repository.rb | 16 +++-
app/services/git_operation_service.rb | 111 ++++++++++++--------------
spec/models/repository_spec.rb | 2 +-
3 files changed, 65 insertions(+), 64 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 36cbb0d051e..9393d6b461e 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -973,7 +973,8 @@ class Repository
base_branch,
source_branch: source_branch, source_project: source_project) do
- source_sha = find_branch(base_branch).dereferenced_target.sha
+ source_sha = source_project.repository.find_source_sha(
+ source_branch || base_branch)
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
@@ -996,7 +997,8 @@ class Repository
base_branch,
source_branch: source_branch, source_project: source_project) do
- source_sha = find_branch(base_branch).dereferenced_target.sha
+ source_sha = source_project.repository.find_source_sha(
+ source_branch || base_branch)
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
@@ -1162,6 +1164,16 @@ class Repository
end
end
+ protected
+
+ def find_source_sha(branch_name)
+ if branch_exists?(branch_name)
+ find_branch(branch_name).dereferenced_target.sha
+ else
+ Gitlab::Git::BLANK_SHA
+ end
+ end
+
private
def refs_directory_exists?
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index c9e2c21737a..3a102a9276b 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -34,43 +34,10 @@ GitOperationService = Struct.new(:user, :repository) do
source_branch: nil,
source_project: repository.project)
- ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
- oldrev = Gitlab::Git::BLANK_SHA
+ check_with_branch_arguments!(branch_name, source_branch, source_project)
- if repository.branch_exists?(branch_name)
- oldrev = newrev = repository.commit(branch_name).sha
-
- elsif repository.project != source_project
- unless source_branch
- raise ArgumentError,
- 'Should also pass :source_branch if' +
- ' :source_project is different from current project'
- end
-
- newrev = source_project.repository.commit(source_branch).try(:sha)
-
- unless newrev
- raise Repository::CommitError.new(
- "Cannot find branch #{branch_name} nor" \
- " #{source_branch} from" \
- " #{source_project.path_with_namespace}")
- end
-
- elsif source_branch
- newrev = repository.commit(source_branch).try(:sha)
-
- unless newrev
- raise Repository::CommitError.new(
- "Cannot find branch #{branch_name} nor" \
- " #{source_branch} from" \
- " #{repository.project.path_with_namespace}")
- end
-
- else # we want an orphan empty branch
- newrev = Gitlab::Git::BLANK_SHA
- end
-
- commit_with_hooks(ref, oldrev, newrev) do
+ update_branch_with_hooks(
+ branch_name, source_branch, source_project) do |ref|
if repository.project != source_project
repository.fetch_ref(
source_project.repository.path_to_repo,
@@ -85,39 +52,37 @@ GitOperationService = Struct.new(:user, :repository) do
private
- def commit_with_hooks(ref, oldrev, newrev)
- with_hooks_and_update_ref(ref, oldrev, newrev) do |service|
- was_empty = repository.empty?
-
- # Make commit
- nextrev = yield(ref)
-
- unless nextrev
- raise Repository::CommitError.new('Failed to create commit')
- end
+ def update_branch_with_hooks(branch_name, source_branch, source_project)
+ update_autocrlf_option
- branch =
- repository.find_branch(ref[Gitlab::Git::BRANCH_REF_PREFIX.size..-1])
+ ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
+ oldrev = Gitlab::Git::BLANK_SHA
+ was_empty = repository.empty?
- prevrev = if branch &&
- !repository.rugged.lookup(nextrev).parent_ids.empty?
- repository.rugged.merge_base(
- nextrev, branch.dereferenced_target.sha)
- else
- newrev
- end
+ # Make commit
+ newrev = yield(ref)
- update_ref!(ref, nextrev, prevrev)
+ unless newrev
+ raise Repository::CommitError.new('Failed to create commit')
+ end
- service.newrev = nextrev
+ branch = repository.find_branch(branch_name)
+ oldrev = if repository.rugged.lookup(newrev).parent_ids.empty? ||
+ branch.nil?
+ Gitlab::Git::BLANK_SHA
+ else
+ repository.rugged.merge_base(
+ newrev, branch.dereferenced_target.sha)
+ end
+ with_hooks_and_update_ref(ref, oldrev, newrev) do
# If repo was empty expire cache
repository.after_create if was_empty
repository.after_create_branch if was_empty ||
oldrev == Gitlab::Git::BLANK_SHA
-
- nextrev
end
+
+ newrev
end
def with_hooks_and_update_ref(ref, oldrev, newrev)
@@ -129,8 +94,6 @@ GitOperationService = Struct.new(:user, :repository) do
end
def with_hooks(ref, oldrev, newrev)
- update_autocrlf_option
-
result = nil
GitHooksService.new.execute(
@@ -170,4 +133,30 @@ GitOperationService = Struct.new(:user, :repository) do
repository.raw_repository.autocrlf = :input
end
end
+
+ def check_with_branch_arguments!(branch_name, source_branch, source_project)
+ return if repository.branch_exists?(branch_name)
+
+ if repository.project != source_project
+ unless source_branch
+ raise ArgumentError,
+ 'Should also pass :source_branch if' +
+ ' :source_project is different from current project'
+ end
+
+ unless source_project.repository.commit(source_branch).try(:sha)
+ raise Repository::CommitError.new(
+ "Cannot find branch #{branch_name} nor" \
+ " #{source_branch} from" \
+ " #{source_project.path_with_namespace}")
+ end
+ elsif source_branch
+ unless repository.commit(source_branch).try(:sha)
+ raise Repository::CommitError.new(
+ "Cannot find branch #{branch_name} nor" \
+ " #{source_branch} from" \
+ " #{repository.project.path_with_namespace}")
+ end
+ end
+ end
end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index e3ec4c85a74..ebd05c946cc 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -695,7 +695,7 @@ describe Repository, models: true do
user,
repository.path_to_repo,
old_rev,
- old_rev,
+ new_rev,
'refs/heads/feature').
and_yield(service).and_return(true)
end
--
GitLab
From 5ba468efde94ed823aaccabf2405e63cecfbf9d6 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 7 Dec 2016 23:03:58 +0800
Subject: [PATCH 25/78] Not sure why rubocop prefers this but anyway
---
app/models/repository.rb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 9393d6b461e..038ab5e104a 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -974,7 +974,7 @@ class Repository
source_branch: source_branch, source_project: source_project) do
source_sha = source_project.repository.find_source_sha(
- source_branch || base_branch)
+ source_branch || base_branch)
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
@@ -998,7 +998,7 @@ class Repository
source_branch: source_branch, source_project: source_project) do
source_sha = source_project.repository.find_source_sha(
- source_branch || base_branch)
+ source_branch || base_branch)
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
--
GitLab
From fff3c5262857ee8c8dbf4ba7159032f836eba1bd Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 7 Dec 2016 23:33:33 +0800
Subject: [PATCH 26/78] Use multi_action to commit which doesn't need to
have the branch existed upfront. That is,
`Rugged::Commit.create` rather than `Gitlab::Git::Blob.commit`
which the former doesn't need to have the branch but the latter
needs.
---
app/models/repository.rb | 99 +++++++++++++++-------------------------
1 file changed, 36 insertions(+), 63 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 038ab5e104a..c05cfb271c7 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -773,27 +773,17 @@ class Repository
user, path, content, message, branch, update,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- GitOperationService.new(user, self).with_branch(
- branch,
+ multi_action(
+ user: user,
+ branch: branch,
+ message: message,
+ author_email: author_email,
+ author_name: author_name,
source_branch: source_branch,
- source_project: source_project) do |ref|
- options = {
- commit: {
- branch: ref,
- message: message,
- update_ref: false
- },
- file: {
- content: content,
- path: path,
- update: update
- }
- }
-
- options.merge!(get_committer_and_author(user, email: author_email, name: author_name))
-
- Gitlab::Git::Blob.commit(raw_repository, options)
- end
+ source_project: source_project,
+ actions: [{action: :create,
+ file_path: path,
+ content: content}])
end
# rubocop:enable Metrics/ParameterLists
@@ -803,32 +793,24 @@ class Repository
branch:, previous_path:, message:,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- GitOperationService.new(user, self).with_branch(
- branch,
+ action = if previous_path && previous_path != path
+ :move
+ else
+ :update
+ end
+
+ multi_action(
+ user: user,
+ branch: branch,
+ message: message,
+ author_email: author_email,
+ author_name: author_name,
source_branch: source_branch,
- source_project: source_project) do |ref|
- options = {
- commit: {
- branch: ref,
- message: message,
- update_ref: false
- },
- file: {
- content: content,
- path: path,
- update: true
- }
- }
-
- options.merge!(get_committer_and_author(user, email: author_email, name: author_name))
-
- if previous_path && previous_path != path
- options[:file][:previous_path] = previous_path
- Gitlab::Git::Blob.rename(raw_repository, options)
- else
- Gitlab::Git::Blob.commit(raw_repository, options)
- end
- end
+ source_project: source_project,
+ actions: [{action: action,
+ file_path: path,
+ content: content,
+ previous_path: previous_path}])
end
# rubocop:enable Metrics/ParameterLists
@@ -837,25 +819,16 @@ class Repository
user, path, message, branch,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- GitOperationService.new(user, self).with_branch(
- branch,
+ multi_action(
+ user: user,
+ branch: branch,
+ message: message,
+ author_email: author_email,
+ author_name: author_name,
source_branch: source_branch,
- source_project: source_project) do |ref|
- options = {
- commit: {
- branch: ref,
- message: message,
- update_ref: false
- },
- file: {
- path: path
- }
- }
-
- options.merge!(get_committer_and_author(user, email: author_email, name: author_name))
-
- Gitlab::Git::Blob.remove(raw_repository, options)
- end
+ source_project: source_project,
+ actions: [{action: :delete,
+ file_path: path}])
end
# rubocop:enable Metrics/ParameterLists
--
GitLab
From a51f26e5142d6c5f40984cd374f0dea7580a4235 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 01:12:49 +0800
Subject: [PATCH 27/78] Use commit_file for commit_dir
---
app/models/repository.rb | 38 ++++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index c05cfb271c7..6246630300c 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -749,22 +749,32 @@ class Repository
user, path, message, branch,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
- GitOperationService.new(user, self).with_branch(
+ if branch_exists?(branch)
+ # tree_entry is private
+ entry = raw_repository.send(:tree_entry, commit(branch), path)
+
+ if entry
+ if entry[:type] == :blob
+ raise Gitlab::Git::Repository::InvalidBlobName.new(
+ "Directory already exists as a file")
+ else
+ raise Gitlab::Git::Repository::InvalidBlobName.new(
+ "Directory already exists")
+ end
+ end
+ end
+
+ commit_file(
+ user,
+ "#{Gitlab::Git::PathHelper.normalize_path(path)}/.gitkeep",
+ '',
+ message,
branch,
+ false,
+ author_email: author_email,
+ author_name: author_name,
source_branch: source_branch,
- source_project: source_project) do |ref|
- options = {
- commit: {
- branch: ref,
- message: message,
- update_ref: false
- }
- }
-
- options.merge!(get_committer_and_author(user, email: author_email, name: author_name))
-
- raw_repository.mkdir(path, options)
- end
+ source_project: source_project)
end
# rubocop:enable Metrics/ParameterLists
--
GitLab
From e76173038b03c660a498d9f38b07797b453f1e7f Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 01:13:16 +0800
Subject: [PATCH 28/78] Restore the check for update in commit_file
---
app/models/repository.rb | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6246630300c..8a94fbf3ecc 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -783,6 +783,14 @@ class Repository
user, path, content, message, branch, update,
author_email: nil, author_name: nil,
source_branch: nil, source_project: project)
+ if branch_exists?(branch) && update == false
+ # tree_entry is private
+ if raw_repository.send(:tree_entry, commit(branch), path)
+ raise Gitlab::Git::Repository::InvalidBlobName.new(
+ "Filename already exists; update not allowed")
+ end
+ end
+
multi_action(
user: user,
branch: branch,
--
GitLab
From 4b3c18ce5cbd88156423d89f26b0869f45e2225e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 01:13:33 +0800
Subject: [PATCH 29/78] Use branch_name to find the branch rather than ref
---
app/models/repository.rb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 8a94fbf3ecc..4d74ac6f585 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -858,9 +858,9 @@ class Repository
GitOperationService.new(user, self).with_branch(
branch,
source_branch: source_branch,
- source_project: source_project) do |ref|
+ source_project: source_project) do
index = rugged.index
- branch_commit = find_branch(ref)
+ branch_commit = find_branch(branch)
parents = if branch_commit
last_commit = branch_commit.dereferenced_target
--
GitLab
From e36088dd98c1c00a8884684158d47f479940346e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 01:30:10 +0800
Subject: [PATCH 30/78] We need to normalize the path for all actions
---
app/models/repository.rb | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 4d74ac6f585..cf2c08f618e 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -766,7 +766,7 @@ class Repository
commit_file(
user,
- "#{Gitlab::Git::PathHelper.normalize_path(path)}/.gitkeep",
+ "#{path}/.gitkeep",
'',
message,
branch,
@@ -871,12 +871,14 @@ class Repository
end
actions.each do |action|
+ path = Gitlab::Git::PathHelper.normalize_path(action[:file_path]).to_s
+
case action[:action]
when :create, :update, :move
mode =
case action[:action]
when :update
- index.get(action[:file_path])[:mode]
+ index.get(path)[:mode]
when :move
index.get(action[:previous_path])[:mode]
end
@@ -887,9 +889,9 @@ class Repository
content = action[:encoding] == 'base64' ? Base64.decode64(action[:content]) : action[:content]
oid = rugged.write(content, :blob)
- index.add(path: action[:file_path], oid: oid, mode: mode)
+ index.add(path: path, oid: oid, mode: mode)
when :delete
- index.remove(action[:file_path])
+ index.remove(path)
end
end
--
GitLab
From 56e0dcab97f43e14ae88a66031e3cd71cd062b23 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 02:15:09 +0800
Subject: [PATCH 31/78] Spaces around hash braces
---
app/models/repository.rb | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index cf2c08f618e..4d350f937a6 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -799,9 +799,9 @@ class Repository
author_name: author_name,
source_branch: source_branch,
source_project: source_project,
- actions: [{action: :create,
- file_path: path,
- content: content}])
+ actions: [{ action: :create,
+ file_path: path,
+ content: content }])
end
# rubocop:enable Metrics/ParameterLists
@@ -825,10 +825,10 @@ class Repository
author_name: author_name,
source_branch: source_branch,
source_project: source_project,
- actions: [{action: action,
- file_path: path,
- content: content,
- previous_path: previous_path}])
+ actions: [{ action: action,
+ file_path: path,
+ content: content,
+ previous_path: previous_path }])
end
# rubocop:enable Metrics/ParameterLists
@@ -845,8 +845,8 @@ class Repository
author_name: author_name,
source_branch: source_branch,
source_project: source_project,
- actions: [{action: :delete,
- file_path: path}])
+ actions: [{ action: :delete,
+ file_path: path }])
end
# rubocop:enable Metrics/ParameterLists
--
GitLab
From cdb598f3973bb7643d8d7f85f6109d84aea759ec Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 14:15:50 +0800
Subject: [PATCH 32/78] We probably don't need this anymore, not sure why
---
app/controllers/concerns/creates_commit.rb | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index e5c40446314..dacb5679dd3 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,10 +4,9 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
- source_branch = @ref if @ref && @repository.branch_exists?(@ref)
commit_params = @commit_params.merge(
source_project: @project,
- source_branch: source_branch,
+ source_branch: @ref,
target_branch: @target_branch
)
--
GitLab
From ae5b935b2888f7721e424cf41e2963e1483d8bb5 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 14:16:06 +0800
Subject: [PATCH 33/78] find the commit properly and replicate gitlab_git
by checking filename as well
---
app/models/repository.rb | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 4d350f937a6..50f347b58c8 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -860,7 +860,8 @@ class Repository
source_branch: source_branch,
source_project: source_project) do
index = rugged.index
- branch_commit = find_branch(branch)
+ branch_commit = source_project.repository.find_branch(
+ source_branch || branch)
parents = if branch_commit
last_commit = branch_commit.dereferenced_target
@@ -873,6 +874,9 @@ class Repository
actions.each do |action|
path = Gitlab::Git::PathHelper.normalize_path(action[:file_path]).to_s
+ raise Gitlab::Git::Repository::InvalidBlobName.new("Invalid path") if
+ path.split('/').include?('..')
+
case action[:action]
when :create, :update, :move
mode =
--
GitLab
From 4f94053c21196b3445c117130e3556463e4ca1d0 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 15:22:32 +0800
Subject: [PATCH 34/78] We still need it for empty repo for web UI!
We shouldn't pass a non-existing branch to source_branch.
Checkout test for:
* spec/features/tags/master_views_tags_spec.rb:24
* spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb:13
---
app/controllers/concerns/creates_commit.rb | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index dacb5679dd3..5d11f286e9a 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,9 +4,10 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
+ source_branch = @ref if @ref && @repository.find_branch(@ref)
commit_params = @commit_params.merge(
source_project: @project,
- source_branch: @ref,
+ source_branch: source_branch,
target_branch: @target_branch
)
--
GitLab
From cf677378ee8104e7505cf0670ad45a51613af575 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 15:23:41 +0800
Subject: [PATCH 35/78] Prefer repository.branch_exists?
---
app/services/files/base_service.rb | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 6779bd2818a..80e1d1d60f2 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -5,7 +5,7 @@ module Files
def execute
@source_project = params[:source_project] || @project
@source_branch = params[:source_branch]
- @target_branch = params[:target_branch]
+ @target_branch = params[:target_branch]
@commit_message = params[:commit_message]
@file_path = params[:file_path]
@@ -59,12 +59,12 @@ module Files
end
unless project.empty_repo?
- unless @source_project.repository.branch_names.include?(@source_branch)
+ unless @source_project.repository.branch_exists?(@source_branch)
raise_error('You can only create or edit files when you are on a branch')
end
if different_branch?
- if repository.branch_names.include?(@target_branch)
+ if repository.branch_exists?(@target_branch)
raise_error('Branch with such name already exists. You need to switch to this branch in order to make changes')
end
end
--
GitLab
From 691f1c496834078ba41209597558259d20790a0b Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 15:31:42 +0800
Subject: [PATCH 36/78] Simply give result if result[:status] == :error
---
app/services/create_branch_service.rb | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index 076f976ed06..1b5e504573a 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -1,8 +1,9 @@
class CreateBranchService < BaseService
def execute(branch_name, ref)
- failure = validate_new_branch(branch_name)
+ result = ValidateNewBranchService.new(project, current_user).
+ execute(branch_name)
- return failure if failure
+ return result if result[:status] == :error
new_branch = repository.add_branch(current_user, branch_name, ref)
@@ -18,13 +19,4 @@ class CreateBranchService < BaseService
def success(branch)
super().merge(branch: branch)
end
-
- private
-
- def validate_new_branch(branch_name)
- result = ValidateNewBranchService.new(project, current_user).
- execute(branch_name)
-
- error(result[:message]) if result[:status] == :error
- end
end
--
GitLab
From 3fa3fcd7876262bb63966debd04d16ea219fad73 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 17:08:25 +0800
Subject: [PATCH 37/78] Cleanup parameters, easier to understand and
more consistent across different methodst
---
app/models/repository.rb | 91 +++++++++++++-----------
app/services/commits/change_service.rb | 2 +-
app/services/files/create_dir_service.rb | 6 +-
app/services/files/create_service.rb | 8 +--
app/services/files/delete_service.rb | 6 +-
app/services/files/multi_service.rb | 4 +-
app/services/files/update_service.rb | 6 +-
app/services/git_operation_service.rb | 16 ++---
spec/models/repository_spec.rb | 86 ++++++++++++++--------
9 files changed, 130 insertions(+), 95 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 50f347b58c8..73a9e269a65 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -746,12 +746,13 @@ class Repository
# rubocop:disable Metrics/ParameterLists
def commit_dir(
- user, path, message, branch,
+ user, path,
+ message:, branch_name:,
author_email: nil, author_name: nil,
- source_branch: nil, source_project: project)
- if branch_exists?(branch)
+ source_branch_name: nil, source_project: project)
+ if branch_exists?(branch_name)
# tree_entry is private
- entry = raw_repository.send(:tree_entry, commit(branch), path)
+ entry = raw_repository.send(:tree_entry, commit(branch_name), path)
if entry
if entry[:type] == :blob
@@ -768,24 +769,25 @@ class Repository
user,
"#{path}/.gitkeep",
'',
- message,
- branch,
- false,
+ message: message,
+ branch_name: branch_name,
+ update: false,
author_email: author_email,
author_name: author_name,
- source_branch: source_branch,
+ source_branch_name: source_branch_name,
source_project: source_project)
end
# rubocop:enable Metrics/ParameterLists
# rubocop:disable Metrics/ParameterLists
def commit_file(
- user, path, content, message, branch, update,
+ user, path, content,
+ message:, branch_name:, update: true,
author_email: nil, author_name: nil,
- source_branch: nil, source_project: project)
- if branch_exists?(branch) && update == false
+ source_branch_name: nil, source_project: project)
+ if branch_exists?(branch_name) && update == false
# tree_entry is private
- if raw_repository.send(:tree_entry, commit(branch), path)
+ if raw_repository.send(:tree_entry, commit(branch_name), path)
raise Gitlab::Git::Repository::InvalidBlobName.new(
"Filename already exists; update not allowed")
end
@@ -793,11 +795,11 @@ class Repository
multi_action(
user: user,
- branch: branch,
message: message,
+ branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- source_branch: source_branch,
+ source_branch_name: source_branch_name,
source_project: source_project,
actions: [{ action: :create,
file_path: path,
@@ -808,9 +810,9 @@ class Repository
# rubocop:disable Metrics/ParameterLists
def update_file(
user, path, content,
- branch:, previous_path:, message:,
+ message:, branch_name:, previous_path:,
author_email: nil, author_name: nil,
- source_branch: nil, source_project: project)
+ source_branch_name: nil, source_project: project)
action = if previous_path && previous_path != path
:move
else
@@ -819,11 +821,11 @@ class Repository
multi_action(
user: user,
- branch: branch,
message: message,
+ branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- source_branch: source_branch,
+ source_branch_name: source_branch_name,
source_project: source_project,
actions: [{ action: action,
file_path: path,
@@ -834,16 +836,17 @@ class Repository
# rubocop:disable Metrics/ParameterLists
def remove_file(
- user, path, message, branch,
+ user, path,
+ message:, branch_name:,
author_email: nil, author_name: nil,
- source_branch: nil, source_project: project)
+ source_branch_name: nil, source_project: project)
multi_action(
user: user,
- branch: branch,
message: message,
+ branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- source_branch: source_branch,
+ source_branch_name: source_branch_name,
source_project: source_project,
actions: [{ action: :delete,
file_path: path }])
@@ -852,16 +855,16 @@ class Repository
# rubocop:disable Metrics/ParameterLists
def multi_action(
- user:, branch:, message:, actions:,
+ user:, branch_name:, message:, actions:,
author_email: nil, author_name: nil,
- source_branch: nil, source_project: project)
+ source_branch_name: nil, source_project: project)
GitOperationService.new(user, self).with_branch(
- branch,
- source_branch: source_branch,
+ branch_name,
+ source_branch_name: source_branch_name,
source_project: source_project) do
index = rugged.index
branch_commit = source_project.repository.find_branch(
- source_branch || branch)
+ source_branch_name || branch_name)
parents = if branch_commit
last_commit = branch_commit.dereferenced_target
@@ -960,18 +963,19 @@ class Repository
end
def revert(
- user, commit, base_branch, revert_tree_id = nil,
- source_branch: nil, source_project: project)
- revert_tree_id ||= check_revert_content(commit, base_branch)
+ user, commit, branch_name, revert_tree_id = nil,
+ source_branch_name: nil, source_project: project)
+ revert_tree_id ||= check_revert_content(commit, branch_name)
return false unless revert_tree_id
GitOperationService.new(user, self).with_branch(
- base_branch,
- source_branch: source_branch, source_project: source_project) do
+ branch_name,
+ source_branch_name: source_branch_name,
+ source_project: source_project) do
source_sha = source_project.repository.find_source_sha(
- source_branch || base_branch)
+ source_branch_name || branch_name)
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
@@ -984,18 +988,19 @@ class Repository
end
def cherry_pick(
- user, commit, base_branch, cherry_pick_tree_id = nil,
- source_branch: nil, source_project: project)
- cherry_pick_tree_id ||= check_cherry_pick_content(commit, base_branch)
+ user, commit, branch_name, cherry_pick_tree_id = nil,
+ source_branch_name: nil, source_project: project)
+ cherry_pick_tree_id ||= check_cherry_pick_content(commit, branch_name)
return false unless cherry_pick_tree_id
GitOperationService.new(user, self).with_branch(
- base_branch,
- source_branch: source_branch, source_project: source_project) do
+ branch_name,
+ source_branch_name: source_branch_name,
+ source_project: source_project) do
source_sha = source_project.repository.find_source_sha(
- source_branch || base_branch)
+ source_branch_name || branch_name)
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
@@ -1019,8 +1024,8 @@ class Repository
end
end
- def check_revert_content(commit, base_branch)
- source_sha = find_branch(base_branch).dereferenced_target.sha
+ def check_revert_content(commit, branch_name)
+ source_sha = find_branch(branch_name).dereferenced_target.sha
args = [commit.id, source_sha]
args << { mainline: 1 } if commit.merge_commit?
@@ -1033,8 +1038,8 @@ class Repository
tree_id
end
- def check_cherry_pick_content(commit, base_branch)
- source_sha = find_branch(base_branch).dereferenced_target.sha
+ def check_cherry_pick_content(commit, branch_name)
+ source_sha = find_branch(branch_name).dereferenced_target.sha
args = [commit.id, source_sha]
args << 1 if commit.merge_commit?
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 5458f7a6790..d49fcd42a08 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -37,7 +37,7 @@ module Commits
@commit,
into,
tree_id,
- source_branch: @target_branch)
+ source_branch_name: @target_branch)
success
else
diff --git a/app/services/files/create_dir_service.rb b/app/services/files/create_dir_service.rb
index f0bb3333db8..4a2b2e8fcaf 100644
--- a/app/services/files/create_dir_service.rb
+++ b/app/services/files/create_dir_service.rb
@@ -4,11 +4,11 @@ module Files
repository.commit_dir(
current_user,
@file_path,
- @commit_message,
- @target_branch,
+ message: @commit_message,
+ branch_name: @target_branch,
author_email: @author_email,
author_name: @author_name,
- source_branch: @source_branch)
+ source_branch_name: @source_branch)
end
def validate
diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index 65f7baf56fd..c95cb75f7cb 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -5,12 +5,12 @@ module Files
current_user,
@file_path,
@file_content,
- @commit_message,
- @target_branch,
- false,
+ message: @commit_message,
+ branch_name: @target_branch,
+ update: false,
author_email: @author_email,
author_name: @author_name,
- source_branch: @source_branch)
+ source_branch_name: @source_branch)
end
def validate
diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb
index b3f323d0173..45a9a559469 100644
--- a/app/services/files/delete_service.rb
+++ b/app/services/files/delete_service.rb
@@ -4,11 +4,11 @@ module Files
repository.remove_file(
current_user,
@file_path,
- @commit_message,
- @target_branch,
+ message: @commit_message,
+ branch_name: @target_branch,
author_email: @author_email,
author_name: @author_name,
- source_branch: @source_branch)
+ source_branch_name: @source_branch)
end
end
end
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index 6f5f25f88fd..42ed97ca3c0 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -5,12 +5,12 @@ module Files
def commit
repository.multi_action(
user: current_user,
- branch: @target_branch,
message: @commit_message,
+ branch_name: @target_branch,
actions: params[:actions],
author_email: @author_email,
author_name: @author_name,
- source_branch: @source_branch
+ source_branch_name: @source_branch
)
end
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index 67d473d4978..5f671817cdb 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -4,13 +4,13 @@ module Files
def commit
repository.update_file(current_user, @file_path, @file_content,
- branch: @target_branch,
- previous_path: @previous_path,
message: @commit_message,
+ branch_name: @target_branch,
+ previous_path: @previous_path,
author_email: @author_email,
author_name: @author_name,
source_project: @source_project,
- source_branch: @source_branch)
+ source_branch_name: @source_branch)
end
private
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 3a102a9276b..c8504ecf3cd 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -25,23 +25,23 @@ GitOperationService = Struct.new(:user, :repository) do
end
end
- # Whenever `source_branch` is passed, if `branch` doesn't exist,
- # it would be created from `source_branch`.
+ # Whenever `source_branch_name` is passed, if `branch_name` doesn't exist,
+ # it would be created from `source_branch_name`.
# If `source_project` is passed, and the branch doesn't exist,
# it would try to find the source from it instead of current repository.
def with_branch(
branch_name,
- source_branch: nil,
+ source_branch_name: nil,
source_project: repository.project)
- check_with_branch_arguments!(branch_name, source_branch, source_project)
+ check_with_branch_arguments!(
+ branch_name, source_branch_name, source_project)
- update_branch_with_hooks(
- branch_name, source_branch, source_project) do |ref|
+ update_branch_with_hooks(branch_name) do |ref|
if repository.project != source_project
repository.fetch_ref(
source_project.repository.path_to_repo,
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch}",
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
"#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch_name}"
)
end
@@ -52,7 +52,7 @@ GitOperationService = Struct.new(:user, :repository) do
private
- def update_branch_with_hooks(branch_name, source_branch, source_project)
+ def update_branch_with_hooks(branch_name)
update_autocrlf_option
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index ebd05c946cc..78596346b91 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -249,7 +249,8 @@ describe Repository, models: true do
describe "#commit_dir" do
it "commits a change that creates a new directory" do
expect do
- repository.commit_dir(user, 'newdir', 'Create newdir', 'master')
+ repository.commit_dir(user, 'newdir',
+ message: 'Create newdir', branch_name: 'master')
end.to change { repository.commits('master').count }.by(1)
newdir = repository.tree('master', 'newdir')
@@ -259,7 +260,10 @@ describe Repository, models: true do
context "when an author is specified" do
it "uses the given email/name to set the commit's author" do
expect do
- repository.commit_dir(user, "newdir", "Add newdir", 'master', author_email: author_email, author_name: author_name)
+ repository.commit_dir(user, 'newdir',
+ message: 'Add newdir',
+ branch_name: 'master',
+ author_email: author_email, author_name: author_name)
end.to change { repository.commits('master').count }.by(1)
last_commit = repository.commit
@@ -274,8 +278,9 @@ describe Repository, models: true do
it 'commits change to a file successfully' do
expect do
repository.commit_file(user, 'CHANGELOG', 'Changelog!',
- 'Updates file content',
- 'master', true)
+ message: 'Updates file content',
+ branch_name: 'master',
+ update: true)
end.to change { repository.commits('master').count }.by(1)
blob = repository.blob_at('master', 'CHANGELOG')
@@ -286,8 +291,12 @@ describe Repository, models: true do
context "when an author is specified" do
it "uses the given email/name to set the commit's author" do
expect do
- repository.commit_file(user, "README", 'README!', 'Add README',
- 'master', true, author_email: author_email, author_name: author_name)
+ repository.commit_file(user, 'README', 'README!',
+ message: 'Add README',
+ branch_name: 'master',
+ update: true,
+ author_email: author_email,
+ author_name: author_name)
end.to change { repository.commits('master').count }.by(1)
last_commit = repository.commit
@@ -302,7 +311,7 @@ describe Repository, models: true do
it 'updates filename successfully' do
expect do
repository.update_file(user, 'NEWLICENSE', 'Copyright!',
- branch: 'master',
+ branch_name: 'master',
previous_path: 'LICENSE',
message: 'Changes filename')
end.to change { repository.commits('master').count }.by(1)
@@ -315,15 +324,16 @@ describe Repository, models: true do
context "when an author is specified" do
it "uses the given email/name to set the commit's author" do
- repository.commit_file(user, "README", 'README!', 'Add README', 'master', true)
+ repository.commit_file(user, 'README', 'README!',
+ message: 'Add README', branch_name: 'master', update: true)
expect do
- repository.update_file(user, 'README', "Updated README!",
- branch: 'master',
- previous_path: 'README',
- message: 'Update README',
- author_email: author_email,
- author_name: author_name)
+ repository.update_file(user, 'README', 'Updated README!',
+ branch_name: 'master',
+ previous_path: 'README',
+ message: 'Update README',
+ author_email: author_email,
+ author_name: author_name)
end.to change { repository.commits('master').count }.by(1)
last_commit = repository.commit
@@ -336,10 +346,12 @@ describe Repository, models: true do
describe "#remove_file" do
it 'removes file successfully' do
- repository.commit_file(user, "README", 'README!', 'Add README', 'master', true)
+ repository.commit_file(user, 'README', 'README!',
+ message: 'Add README', branch_name: 'master', update: true)
expect do
- repository.remove_file(user, "README", "Remove README", 'master')
+ repository.remove_file(user, 'README',
+ message: 'Remove README', branch_name: 'master')
end.to change { repository.commits('master').count }.by(1)
expect(repository.blob_at('master', 'README')).to be_nil
@@ -347,10 +359,13 @@ describe Repository, models: true do
context "when an author is specified" do
it "uses the given email/name to set the commit's author" do
- repository.commit_file(user, "README", 'README!', 'Add README', 'master', true)
+ repository.commit_file(user, 'README', 'README!',
+ message: 'Add README', branch_name: 'master', update: true)
expect do
- repository.remove_file(user, "README", "Remove README", 'master', author_email: author_email, author_name: author_name)
+ repository.remove_file(user, 'README',
+ message: 'Remove README', branch_name: 'master',
+ author_email: author_email, author_name: author_name)
end.to change { repository.commits('master').count }.by(1)
last_commit = repository.commit
@@ -498,11 +513,14 @@ describe Repository, models: true do
describe "#license_blob", caching: true do
before do
- repository.remove_file(user, 'LICENSE', 'Remove LICENSE', 'master')
+ repository.remove_file(
+ user, 'LICENSE', message: 'Remove LICENSE', branch_name: 'master')
end
it 'handles when HEAD points to non-existent ref' do
- repository.commit_file(user, 'LICENSE', 'Copyright!', 'Add LICENSE', 'master', false)
+ repository.commit_file(
+ user, 'LICENSE', 'Copyright!',
+ message: 'Add LICENSE', branch_name: 'master', update: false)
allow(repository).to receive(:file_on_head).
and_raise(Rugged::ReferenceError)
@@ -511,21 +529,27 @@ describe Repository, models: true do
end
it 'looks in the root_ref only' do
- repository.remove_file(user, 'LICENSE', 'Remove LICENSE', 'markdown')
- repository.commit_file(user, 'LICENSE', Licensee::License.new('mit').content, 'Add LICENSE', 'markdown', false)
+ repository.remove_file(user, 'LICENSE',
+ message: 'Remove LICENSE', branch_name: 'markdown')
+ repository.commit_file(user, 'LICENSE',
+ Licensee::License.new('mit').content,
+ message: 'Add LICENSE', branch_name: 'markdown', update: false)
expect(repository.license_blob).to be_nil
end
it 'detects license file with no recognizable open-source license content' do
- repository.commit_file(user, 'LICENSE', 'Copyright!', 'Add LICENSE', 'master', false)
+ repository.commit_file(user, 'LICENSE', 'Copyright!',
+ message: 'Add LICENSE', branch_name: 'master', update: false)
expect(repository.license_blob.name).to eq('LICENSE')
end
%w[LICENSE LICENCE LiCensE LICENSE.md LICENSE.foo COPYING COPYING.md].each do |filename|
it "detects '#{filename}'" do
- repository.commit_file(user, filename, Licensee::License.new('mit').content, "Add #{filename}", 'master', false)
+ repository.commit_file(user, filename,
+ Licensee::License.new('mit').content,
+ message: "Add #{filename}", branch_name: 'master', update: false)
expect(repository.license_blob.name).to eq(filename)
end
@@ -534,7 +558,8 @@ describe Repository, models: true do
describe '#license_key', caching: true do
before do
- repository.remove_file(user, 'LICENSE', 'Remove LICENSE', 'master')
+ repository.remove_file(user, 'LICENSE',
+ message: 'Remove LICENSE', branch_name: 'master')
end
it 'returns nil when no license is detected' do
@@ -548,13 +573,16 @@ describe Repository, models: true do
end
it 'detects license file with no recognizable open-source license content' do
- repository.commit_file(user, 'LICENSE', 'Copyright!', 'Add LICENSE', 'master', false)
+ repository.commit_file(user, 'LICENSE', 'Copyright!',
+ message: 'Add LICENSE', branch_name: 'master', update: false)
expect(repository.license_key).to be_nil
end
it 'returns the license key' do
- repository.commit_file(user, 'LICENSE', Licensee::License.new('mit').content, 'Add LICENSE', 'master', false)
+ repository.commit_file(user, 'LICENSE',
+ Licensee::License.new('mit').content,
+ message: 'Add LICENSE', branch_name: 'master', update: false)
expect(repository.license_key).to eq('mit')
end
@@ -815,7 +843,9 @@ describe Repository, models: true do
expect(empty_repository).to receive(:expire_has_visible_content_cache)
empty_repository.commit_file(user, 'CHANGELOG', 'Changelog!',
- 'Updates file content', 'master', false)
+ message: 'Updates file content',
+ branch_name: 'master',
+ update: false)
end
end
end
--
GitLab
From 23032467d4a1282f69e76bba921bd71c0083f7a8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 17:27:50 +0800
Subject: [PATCH 38/78] source_branch -> source_branch_name
---
app/services/git_operation_service.rb | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index c8504ecf3cd..36c8b8ff575 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -134,27 +134,28 @@ GitOperationService = Struct.new(:user, :repository) do
end
end
- def check_with_branch_arguments!(branch_name, source_branch, source_project)
+ def check_with_branch_arguments!(
+ branch_name, source_branch_name, source_project)
return if repository.branch_exists?(branch_name)
if repository.project != source_project
- unless source_branch
+ unless source_branch_name
raise ArgumentError,
- 'Should also pass :source_branch if' +
+ 'Should also pass :source_branch_name if' +
' :source_project is different from current project'
end
- unless source_project.repository.commit(source_branch).try(:sha)
+ unless source_project.repository.commit(source_branch_name).try(:sha)
raise Repository::CommitError.new(
"Cannot find branch #{branch_name} nor" \
- " #{source_branch} from" \
+ " #{source_branch_name} from" \
" #{source_project.path_with_namespace}")
end
- elsif source_branch
- unless repository.commit(source_branch).try(:sha)
+ elsif source_branch_name
+ unless repository.commit(source_branch_name).try(:sha)
raise Repository::CommitError.new(
"Cannot find branch #{branch_name} nor" \
- " #{source_branch} from" \
+ " #{source_branch_name} from" \
" #{repository.project.path_with_namespace}")
end
end
--
GitLab
From 8384d0d8d528ffdd60c9ba9e3c0c9f688cb560ef Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 17:57:52 +0800
Subject: [PATCH 39/78] Introduce Repository#with_tmp_ref which we need
commits from the other repository. We'll cleanup
the tmp ref after we're done with our business.
---
.../projects/compare_controller.rb | 3 +-
app/models/merge_request_diff.rb | 3 +-
app/models/repository.rb | 15 ++++++++++
app/services/compare_service.rb | 30 +++++++++++--------
app/services/git_operation_service.rb | 15 +++++-----
app/services/merge_requests/build_service.rb | 5 ++--
app/workers/emails_on_push_worker.rb | 6 ++--
spec/services/compare_service_spec.rb | 6 ++--
8 files changed, 54 insertions(+), 29 deletions(-)
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index bee3d56076c..e2b178314c0 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -37,7 +37,8 @@ class Projects::CompareController < Projects::ApplicationController
end
def define_diff_vars
- @compare = CompareService.new.execute(@project, @head_ref, @project, @start_ref)
+ @compare = CompareService.new(@project, @head_ref).
+ execute(@project, @start_ref)
if @compare
@commits = @compare.commits
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index b8f36a2c958..7946d8e123e 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -169,7 +169,8 @@ class MergeRequestDiff < ActiveRecord::Base
# When compare merge request versions we want diff A..B instead of A...B
# so we handle cases when user does squash and rebase of the commits between versions.
# For this reason we set straight to true by default.
- CompareService.new.execute(project, head_commit_sha, project, sha, straight: straight)
+ CompareService.new(project, head_commit_sha).
+ execute(project, sha, straight: straight)
end
def commits_count
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 73a9e269a65..ced68b9d274 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1099,6 +1099,21 @@ class Repository
Gitlab::Popen.popen(args, path_to_repo).first.lines.map(&:strip)
end
+ def with_tmp_ref(source_repository, source_branch_name)
+ random_string = SecureRandom.hex
+
+ fetch_ref(
+ source_repository.path_to_repo,
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
+ "refs/tmp/#{random_string}/head"
+ )
+
+ yield
+
+ ensure
+ FileUtils.rm_rf("#{path_to_repo}/refs/tmp/#{random_string}")
+ end
+
def fetch_ref(source_path, source_ref, target_ref)
args = %W(#{Gitlab.config.git.bin_path} fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref})
Gitlab::Popen.popen(args, path_to_repo)
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index 5e8fafca98c..4367cb5f615 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -3,23 +3,29 @@ require 'securerandom'
# Compare 2 branches for one repo or between repositories
# and return Gitlab::Git::Compare object that responds to commits and diffs
class CompareService
- def execute(source_project, source_branch, target_project, target_branch, straight: false)
- source_commit = source_project.commit(source_branch)
- return unless source_commit
+ attr_reader :source_project, :source_sha
- source_sha = source_commit.sha
+ def initialize(new_source_project, source_branch)
+ @source_project = new_source_project
+ @source_sha = new_source_project.commit(source_branch).try(:sha)
+ end
- # If compare with other project we need to fetch ref first
- unless target_project == source_project
- random_string = SecureRandom.hex
+ def execute(target_project, target_branch, straight: false)
+ return unless source_sha
- target_project.repository.fetch_ref(
- source_project.repository.path_to_repo,
- "refs/heads/#{source_branch}",
- "refs/tmp/#{random_string}/head"
- )
+ # If compare with other project we need to fetch ref first
+ if target_project == source_project
+ compare(target_project, target_branch, straight)
+ else
+ target_project.repository.with_tmp_ref(source_project, source_branch) do
+ compare(target_project, target_branch, straight)
+ end
end
+ end
+
+ private
+ def compare(target_project, target_branch, straight)
raw_compare = Gitlab::Git::Compare.new(
target_project.repository.raw_repository,
target_branch,
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 36c8b8ff575..a7d267cd6b4 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -38,15 +38,14 @@ GitOperationService = Struct.new(:user, :repository) do
branch_name, source_branch_name, source_project)
update_branch_with_hooks(branch_name) do |ref|
- if repository.project != source_project
- repository.fetch_ref(
- source_project.repository.path_to_repo,
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch_name}"
- )
+ if repository.project == source_project
+ yield(ref)
+ else
+ repository.with_tmp_ref(
+ source_project.repository, source_branch_name) do
+ yield(ref)
+ end
end
-
- yield(ref)
end
end
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index bebfca7537b..a52a94c5ffa 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -16,9 +16,10 @@ module MergeRequests
messages = validate_branches(merge_request)
return build_failed(merge_request, messages) unless messages.empty?
- compare = CompareService.new.execute(
+ compare = CompareService.new(
merge_request.source_project,
- merge_request.source_branch,
+ merge_request.source_branch
+ ).execute(
merge_request.target_project,
merge_request.target_branch,
)
diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb
index b9cd49985dc..d4c3f14ec06 100644
--- a/app/workers/emails_on_push_worker.rb
+++ b/app/workers/emails_on_push_worker.rb
@@ -33,13 +33,15 @@ class EmailsOnPushWorker
reverse_compare = false
if action == :push
- compare = CompareService.new.execute(project, after_sha, project, before_sha)
+ compare = CompareService.new(project, after_sha).
+ execute(project, before_sha)
diff_refs = compare.diff_refs
return false if compare.same
if compare.commits.empty?
- compare = CompareService.new.execute(project, before_sha, project, after_sha)
+ compare = CompareService.new(project, before_sha).
+ execute(project, after_sha)
diff_refs = compare.diff_refs
reverse_compare = true
diff --git a/spec/services/compare_service_spec.rb b/spec/services/compare_service_spec.rb
index 3760f19aaa2..0a7fc58523f 100644
--- a/spec/services/compare_service_spec.rb
+++ b/spec/services/compare_service_spec.rb
@@ -3,17 +3,17 @@ require 'spec_helper'
describe CompareService, services: true do
let(:project) { create(:project) }
let(:user) { create(:user) }
- let(:service) { described_class.new }
+ let(:service) { described_class.new(project, 'feature') }
describe '#execute' do
context 'compare with base, like feature...fix' do
- subject { service.execute(project, 'feature', project, 'fix', straight: false) }
+ subject { service.execute(project, 'fix', straight: false) }
it { expect(subject.diffs.size).to eq(1) }
end
context 'straight compare, like feature..fix' do
- subject { service.execute(project, 'feature', project, 'fix', straight: true) }
+ subject { service.execute(project, 'fix', straight: true) }
it { expect(subject.diffs.size).to eq(3) }
end
--
GitLab
From 07b9b80a8833cf44ba804c9b8dfdf1550785fe83 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 19:11:52 +0800
Subject: [PATCH 40/78] Fix tests to use the new API
---
app/services/compare_service.rb | 14 ++++---
.../projects/templates_controller_spec.rb | 3 +-
spec/factories/projects.rb | 34 +++++++++++++++-
...project_owner_creates_license_file_spec.rb | 3 +-
.../projects/issuable_templates_spec.rb | 40 ++++++++++++++++---
spec/lib/gitlab/git_access_spec.rb | 8 +++-
.../gitlab/template/issue_template_spec.rb | 19 +++++----
.../template/merge_request_template_spec.rb | 19 +++++----
.../models/cycle_analytics/production_spec.rb | 8 +++-
spec/models/cycle_analytics/staging_spec.rb | 8 ++--
.../merge_requests/resolve_service_spec.rb | 8 +++-
spec/support/cycle_analytics_helpers.rb | 8 +++-
12 files changed, 130 insertions(+), 42 deletions(-)
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index 4367cb5f615..199a015622e 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -3,29 +3,31 @@ require 'securerandom'
# Compare 2 branches for one repo or between repositories
# and return Gitlab::Git::Compare object that responds to commits and diffs
class CompareService
- attr_reader :source_project, :source_sha
+ attr_reader :source_project, :source_branch
- def initialize(new_source_project, source_branch)
+ def initialize(new_source_project, source_branch_name)
@source_project = new_source_project
- @source_sha = new_source_project.commit(source_branch).try(:sha)
+ @source_branch = new_source_project.commit(source_branch_name)
end
def execute(target_project, target_branch, straight: false)
+ source_sha = source_branch.try(:sha)
+
return unless source_sha
# If compare with other project we need to fetch ref first
if target_project == source_project
- compare(target_project, target_branch, straight)
+ compare(source_sha, target_project, target_branch, straight)
else
target_project.repository.with_tmp_ref(source_project, source_branch) do
- compare(target_project, target_branch, straight)
+ compare(source_sha, target_project, target_branch, straight)
end
end
end
private
- def compare(target_project, target_branch, straight)
+ def compare(source_sha, target_project, target_branch, straight)
raw_compare = Gitlab::Git::Compare.new(
target_project.repository.raw_repository,
target_branch,
diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb
index 19a152bcb05..78e54e92a56 100644
--- a/spec/controllers/projects/templates_controller_spec.rb
+++ b/spec/controllers/projects/templates_controller_spec.rb
@@ -14,7 +14,8 @@ describe Projects::TemplatesController do
before do
project.add_user(user, Gitlab::Access::MASTER)
- project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
+ project.repository.commit_file(user, file_path_1, 'something valid',
+ message: 'test 3', branch_name: 'master', update: false)
end
describe '#show' do
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index 1166498ddff..aa971e12a2e 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -91,8 +91,40 @@ FactoryGirl.define do
factory :project, parent: :empty_project do
path { 'gitlabhq' }
- after :create do |project|
+ transient do
+ create_template nil
+ end
+
+ after :create do |project, evaluator|
TestEnv.copy_repo(project)
+
+ if evaluator.create_template
+ args = evaluator.create_template
+
+ project.add_user(args[:user], args[:access])
+
+ project.repository.commit_file(
+ args[:user],
+ ".gitlab/#{args[:path]}/bug.md",
+ 'something valid',
+ message: 'test 3',
+ branch_name: 'master',
+ update: false)
+ project.repository.commit_file(
+ args[:user],
+ ".gitlab/#{args[:path]}/template_test.md",
+ 'template_test',
+ message: 'test 1',
+ branch_name: 'master',
+ update: false)
+ project.repository.commit_file(
+ args[:user],
+ ".gitlab/#{args[:path]}/feature_proposal.md",
+ 'feature_proposal',
+ message: 'test 2',
+ branch_name: 'master',
+ update: false)
+ end
end
end
diff --git a/spec/features/projects/files/project_owner_creates_license_file_spec.rb b/spec/features/projects/files/project_owner_creates_license_file_spec.rb
index a521ce50f35..64094af29c0 100644
--- a/spec/features/projects/files/project_owner_creates_license_file_spec.rb
+++ b/spec/features/projects/files/project_owner_creates_license_file_spec.rb
@@ -6,7 +6,8 @@ feature 'project owner creates a license file', feature: true, js: true do
let(:project_master) { create(:user) }
let(:project) { create(:project) }
background do
- project.repository.remove_file(project_master, 'LICENSE', 'Remove LICENSE', 'master')
+ project.repository.remove_file(project_master, 'LICENSE',
+ message: 'Remove LICENSE', branch_name: 'master')
project.team << [project_master, :master]
login_as(project_master)
visit namespace_project_path(project.namespace, project)
diff --git a/spec/features/projects/issuable_templates_spec.rb b/spec/features/projects/issuable_templates_spec.rb
index 2f377312ea5..daf08067ed1 100644
--- a/spec/features/projects/issuable_templates_spec.rb
+++ b/spec/features/projects/issuable_templates_spec.rb
@@ -18,8 +18,20 @@ feature 'issuable templates', feature: true, js: true do
let(:description_addition) { ' appending to description' }
background do
- project.repository.commit_file(user, '.gitlab/issue_templates/bug.md', template_content, 'added issue template', 'master', false)
- project.repository.commit_file(user, '.gitlab/issue_templates/test.md', longtemplate_content, 'added issue template', 'master', false)
+ project.repository.commit_file(
+ user,
+ '.gitlab/issue_templates/bug.md',
+ template_content,
+ message: 'added issue template',
+ branch_name: 'master',
+ update: false)
+ project.repository.commit_file(
+ user,
+ '.gitlab/issue_templates/test.md',
+ longtemplate_content,
+ message: 'added issue template',
+ branch_name: 'master',
+ update: false)
visit edit_namespace_project_issue_path project.namespace, project, issue
fill_in :'issue[title]', with: 'test issue title'
end
@@ -68,7 +80,13 @@ feature 'issuable templates', feature: true, js: true do
let(:issue) { create(:issue, author: user, assignee: user, project: project) }
background do
- project.repository.commit_file(user, '.gitlab/issue_templates/bug.md', template_content, 'added issue template', 'master', false)
+ project.repository.commit_file(
+ user,
+ '.gitlab/issue_templates/bug.md',
+ template_content,
+ message: 'added issue template',
+ branch_name: 'master',
+ update: false)
visit edit_namespace_project_issue_path project.namespace, project, issue
fill_in :'issue[title]', with: 'test issue title'
fill_in :'issue[description]', with: prior_description
@@ -87,7 +105,13 @@ feature 'issuable templates', feature: true, js: true do
let(:merge_request) { create(:merge_request, :with_diffs, source_project: project) }
background do
- project.repository.commit_file(user, '.gitlab/merge_request_templates/feature-proposal.md', template_content, 'added merge request template', 'master', false)
+ project.repository.commit_file(
+ user,
+ '.gitlab/merge_request_templates/feature-proposal.md',
+ template_content,
+ message: 'added merge request template',
+ branch_name: 'master',
+ update: false)
visit edit_namespace_project_merge_request_path project.namespace, project, merge_request
fill_in :'merge_request[title]', with: 'test merge request title'
end
@@ -112,7 +136,13 @@ feature 'issuable templates', feature: true, js: true do
fork_project.team << [fork_user, :master]
create(:forked_project_link, forked_to_project: fork_project, forked_from_project: project)
login_as fork_user
- project.repository.commit_file(fork_user, '.gitlab/merge_request_templates/feature-proposal.md', template_content, 'added merge request template', 'master', false)
+ project.repository.commit_file(
+ fork_user,
+ '.gitlab/merge_request_templates/feature-proposal.md',
+ template_content,
+ message: 'added merge request template',
+ branch_name: 'master',
+ update: false)
visit edit_namespace_project_merge_request_path project.namespace, project, merge_request
fill_in :'merge_request[title]', with: 'test merge request title'
end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index f1d0a190002..a48287b7a75 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -209,7 +209,13 @@ describe Gitlab::GitAccess, lib: true do
stub_git_hooks
project.repository.add_branch(user, unprotected_branch, 'feature')
target_branch = project.repository.lookup('feature')
- source_branch = project.repository.commit_file(user, FFaker::InternetSE.login_user_name, FFaker::HipsterIpsum.paragraph, FFaker::HipsterIpsum.sentence, unprotected_branch, false)
+ source_branch = project.repository.commit_file(
+ user,
+ FFaker::InternetSE.login_user_name,
+ FFaker::HipsterIpsum.paragraph,
+ message: FFaker::HipsterIpsum.sentence,
+ branch_name: unprotected_branch,
+ update: false)
rugged = project.repository.rugged
author = { email: "email@example.com", time: Time.now, name: "Example Git User" }
diff --git a/spec/lib/gitlab/template/issue_template_spec.rb b/spec/lib/gitlab/template/issue_template_spec.rb
index d2d334e6413..984236b74ce 100644
--- a/spec/lib/gitlab/template/issue_template_spec.rb
+++ b/spec/lib/gitlab/template/issue_template_spec.rb
@@ -4,16 +4,15 @@ describe Gitlab::Template::IssueTemplate do
subject { described_class }
let(:user) { create(:user) }
- let(:project) { create(:project) }
- let(:file_path_1) { '.gitlab/issue_templates/bug.md' }
- let(:file_path_2) { '.gitlab/issue_templates/template_test.md' }
- let(:file_path_3) { '.gitlab/issue_templates/feature_proposal.md' }
-
- before do
- project.add_user(user, Gitlab::Access::MASTER)
- project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
- project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false)
- project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false)
+
+ let(:project) do
+ create(:project,
+ create_template: {
+ user: user,
+ access: Gitlab::Access::MASTER,
+ path: 'issue_templates'
+ }
+ )
end
describe '.all' do
diff --git a/spec/lib/gitlab/template/merge_request_template_spec.rb b/spec/lib/gitlab/template/merge_request_template_spec.rb
index ddf68c4cf78..e98a8beccdd 100644
--- a/spec/lib/gitlab/template/merge_request_template_spec.rb
+++ b/spec/lib/gitlab/template/merge_request_template_spec.rb
@@ -4,16 +4,15 @@ describe Gitlab::Template::MergeRequestTemplate do
subject { described_class }
let(:user) { create(:user) }
- let(:project) { create(:project) }
- let(:file_path_1) { '.gitlab/merge_request_templates/bug.md' }
- let(:file_path_2) { '.gitlab/merge_request_templates/template_test.md' }
- let(:file_path_3) { '.gitlab/merge_request_templates/feature_proposal.md' }
-
- before do
- project.add_user(user, Gitlab::Access::MASTER)
- project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
- project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false)
- project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false)
+
+ let(:project) do
+ create(:project,
+ create_template: {
+ user: user,
+ access: Gitlab::Access::MASTER,
+ path: 'merge_request_templates'
+ }
+ )
end
describe '.all' do
diff --git a/spec/models/cycle_analytics/production_spec.rb b/spec/models/cycle_analytics/production_spec.rb
index 21b9c6e7150..97568c47903 100644
--- a/spec/models/cycle_analytics/production_spec.rb
+++ b/spec/models/cycle_analytics/production_spec.rb
@@ -21,7 +21,13 @@ describe 'CycleAnalytics#production', feature: true do
["production deploy happens after merge request is merged (along with other changes)",
lambda do |context, data|
# Make other changes on master
- sha = context.project.repository.commit_file(context.user, context.random_git_name, "content", "commit message", 'master', false)
+ sha = context.project.repository.commit_file(
+ context.user,
+ context.random_git_name,
+ 'content',
+ message: 'commit message',
+ branch_name: 'master',
+ update: false)
context.project.repository.commit(sha)
context.deploy_master
diff --git a/spec/models/cycle_analytics/staging_spec.rb b/spec/models/cycle_analytics/staging_spec.rb
index dad653964b7..27c826110fb 100644
--- a/spec/models/cycle_analytics/staging_spec.rb
+++ b/spec/models/cycle_analytics/staging_spec.rb
@@ -28,10 +28,10 @@ describe 'CycleAnalytics#staging', feature: true do
sha = context.project.repository.commit_file(
context.user,
context.random_git_name,
- "content",
- "commit message",
- 'master',
- false)
+ 'content',
+ message: 'commit message',
+ branch_name: 'master',
+ update: false)
context.project.repository.commit(sha)
context.deploy_master
diff --git a/spec/services/merge_requests/resolve_service_spec.rb b/spec/services/merge_requests/resolve_service_spec.rb
index 388abb6a0df..a0e51681725 100644
--- a/spec/services/merge_requests/resolve_service_spec.rb
+++ b/spec/services/merge_requests/resolve_service_spec.rb
@@ -66,7 +66,13 @@ describe MergeRequests::ResolveService do
context 'when the source project is a fork and does not contain the HEAD of the target branch' do
let!(:target_head) do
- project.repository.commit_file(user, 'new-file-in-target', '', 'Add new file in target', 'conflict-start', false)
+ project.repository.commit_file(
+ user,
+ 'new-file-in-target',
+ '',
+ message: 'Add new file in target',
+ branch_name: 'conflict-start',
+ update: false)
end
before do
diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb
index 75c95d70951..6ed55289ed9 100644
--- a/spec/support/cycle_analytics_helpers.rb
+++ b/spec/support/cycle_analytics_helpers.rb
@@ -35,7 +35,13 @@ module CycleAnalyticsHelpers
project.repository.add_branch(user, source_branch, 'master')
end
- sha = project.repository.commit_file(user, random_git_name, "content", "commit message", source_branch, false)
+ sha = project.repository.commit_file(
+ user,
+ random_git_name,
+ 'content',
+ message: 'commit message',
+ branch_name: source_branch,
+ update: false)
project.repository.commit(sha)
opts = {
--
GitLab
From 9c6563f64a3a770cccb9fcf3eb609416c2466080 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 19:46:51 +0800
Subject: [PATCH 41/78] rubocop prefers lisp style
---
spec/lib/gitlab/template/issue_template_spec.rb | 4 +---
spec/lib/gitlab/template/merge_request_template_spec.rb | 4 +---
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/spec/lib/gitlab/template/issue_template_spec.rb b/spec/lib/gitlab/template/issue_template_spec.rb
index 984236b74ce..4275fda5c74 100644
--- a/spec/lib/gitlab/template/issue_template_spec.rb
+++ b/spec/lib/gitlab/template/issue_template_spec.rb
@@ -10,9 +10,7 @@ describe Gitlab::Template::IssueTemplate do
create_template: {
user: user,
access: Gitlab::Access::MASTER,
- path: 'issue_templates'
- }
- )
+ path: 'issue_templates' })
end
describe '.all' do
diff --git a/spec/lib/gitlab/template/merge_request_template_spec.rb b/spec/lib/gitlab/template/merge_request_template_spec.rb
index e98a8beccdd..708bb084eef 100644
--- a/spec/lib/gitlab/template/merge_request_template_spec.rb
+++ b/spec/lib/gitlab/template/merge_request_template_spec.rb
@@ -10,9 +10,7 @@ describe Gitlab::Template::MergeRequestTemplate do
create_template: {
user: user,
access: Gitlab::Access::MASTER,
- path: 'merge_request_templates'
- }
- )
+ path: 'merge_request_templates' })
end
describe '.all' do
--
GitLab
From e7599eb0b76f7ef199e8719377a212f53aaa17f8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 8 Dec 2016 19:49:20 +0800
Subject: [PATCH 42/78] Should pass repository rather than project
---
app/services/compare_service.rb | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index 199a015622e..fcbfe68f1a3 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -19,7 +19,8 @@ class CompareService
if target_project == source_project
compare(source_sha, target_project, target_branch, straight)
else
- target_project.repository.with_tmp_ref(source_project, source_branch) do
+ target_project.repository.with_tmp_ref(
+ source_project.repository, source_branch) do
compare(source_sha, target_project, target_branch, straight)
end
end
--
GitLab
From 56f697466d288c3bff16cc270e5cb2c50c9c14c8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 9 Dec 2016 20:05:35 +0800
Subject: [PATCH 43/78] Introduce git_action and normalize previous_path
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747456
---
app/models/repository.rb | 72 +++++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 26 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 1684aa97567..9d554991ab4 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -862,32 +862,8 @@ class Repository
[]
end
- actions.each do |action|
- path = Gitlab::Git::PathHelper.normalize_path(action[:file_path]).to_s
-
- raise Gitlab::Git::Repository::InvalidBlobName.new("Invalid path") if
- path.split('/').include?('..')
-
- case action[:action]
- when :create, :update, :move
- mode =
- case action[:action]
- when :update
- index.get(path)[:mode]
- when :move
- index.get(action[:previous_path])[:mode]
- end
- mode ||= 0o100644
-
- index.remove(action[:previous_path]) if action[:action] == :move
-
- content = action[:encoding] == 'base64' ? Base64.decode64(action[:content]) : action[:content]
- oid = rugged.write(content, :blob)
-
- index.add(path: path, oid: oid, mode: mode)
- when :delete
- index.remove(path)
- end
+ actions.each do |act|
+ git_action(index, act)
end
options = {
@@ -1181,6 +1157,50 @@ class Repository
private
+ def git_action(index, action)
+ path = normalize_path(action[:file_path])
+
+ if action[:action] == :move
+ previous_path = normalize_path(action[:previous_path])
+ end
+
+ case action[:action]
+ when :create, :update, :move
+ mode =
+ case action[:action]
+ when :update
+ index.get(path)[:mode]
+ when :move
+ index.get(previous_path)[:mode]
+ end
+ mode ||= 0o100644
+
+ index.remove(previous_path) if action[:action] == :move
+
+ content = if action[:encoding] == 'base64'
+ Base64.decode64(action[:content])
+ else
+ action[:content]
+ end
+
+ oid = rugged.write(content, :blob)
+
+ index.add(path: path, oid: oid, mode: mode)
+ when :delete
+ index.remove(path)
+ end
+ end
+
+ def normalize_path(path)
+ pathname = Gitlab::Git::PathHelper.normalize_path(path)
+
+ if pathname.each_filename.include?('..')
+ raise Gitlab::Git::Repository::InvalidBlobName.new('Invalid path')
+ end
+
+ pathname.to_s
+ end
+
def refs_directory_exists?
return false unless path_with_namespace
--
GitLab
From 5a115671b9d7c22daf8160d7284786d0f8b216cb Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 9 Dec 2016 20:06:19 +0800
Subject: [PATCH 44/78] Use rugged.references.delete to delete reference
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19746468
---
app/models/repository.rb | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 9d554991ab4..389d52f8c0f 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1064,18 +1064,18 @@ class Repository
end
def with_tmp_ref(source_repository, source_branch_name)
- random_string = SecureRandom.hex
+ tmp_ref = "refs/tmp/#{SecureRandom.hex}/head"
fetch_ref(
source_repository.path_to_repo,
"#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
- "refs/tmp/#{random_string}/head"
+ tmp_ref
)
yield
ensure
- FileUtils.rm_rf("#{path_to_repo}/refs/tmp/#{random_string}")
+ rugged.references.delete(tmp_ref)
end
def fetch_ref(source_path, source_ref, target_ref)
--
GitLab
From bb9d30590d4ca5b25d5020234916ce961acf15b6 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 10 Dec 2016 00:40:23 +0800
Subject: [PATCH 45/78] Pass source_commit so that we save a few lookups
---
app/models/repository.rb | 51 ++++++++++-----------------
app/services/git_operation_service.rb | 25 ++++++-------
spec/models/repository_spec.rb | 28 ++++++++++++---
3 files changed, 54 insertions(+), 50 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 389d52f8c0f..4034a49ae63 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -849,15 +849,12 @@ class Repository
GitOperationService.new(user, self).with_branch(
branch_name,
source_branch_name: source_branch_name,
- source_project: source_project) do
+ source_project: source_project) do |source_commit|
index = rugged.index
- branch_commit = source_project.repository.find_branch(
- source_branch_name || branch_name)
- parents = if branch_commit
- last_commit = branch_commit.dereferenced_target
- index.read_tree(last_commit.raw_commit.tree)
- [last_commit.sha]
+ parents = if source_commit
+ index.read_tree(source_commit.raw_commit.tree)
+ [source_commit.sha]
else
[]
end
@@ -904,17 +901,17 @@ class Repository
end
def merge(user, merge_request, options = {})
- our_commit = rugged.branches[merge_request.target_branch].target
- their_commit = rugged.lookup(merge_request.diff_head_sha)
+ GitOperationService.new(user, self).with_branch(
+ merge_request.target_branch) do |source_commit|
+ our_commit = source_commit.raw_commit
+ their_commit = rugged.lookup(merge_request.diff_head_sha)
- raise "Invalid merge target" if our_commit.nil?
- raise "Invalid merge source" if their_commit.nil?
+ raise 'Invalid merge target' unless our_commit
+ raise 'Invalid merge source' unless their_commit
- merge_index = rugged.merge_commits(our_commit, their_commit)
- return false if merge_index.conflicts?
+ merge_index = rugged.merge_commits(our_commit, their_commit)
+ break if merge_index.conflicts?
- GitOperationService.new(user, self).with_branch(
- merge_request.target_branch) do
actual_options = options.merge(
parents: [our_commit, their_commit],
tree: merge_index.write_tree(rugged),
@@ -924,6 +921,8 @@ class Repository
merge_request.update(in_progress_merge_commit_sha: commit_id)
commit_id
end
+ rescue Repository::CommitError # when merge_index.conflicts?
+ false
end
def revert(
@@ -936,10 +935,8 @@ class Repository
GitOperationService.new(user, self).with_branch(
branch_name,
source_branch_name: source_branch_name,
- source_project: source_project) do
+ source_project: source_project) do |source_commit|
- source_sha = source_project.repository.find_source_sha(
- source_branch_name || branch_name)
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
@@ -947,7 +944,7 @@ class Repository
author: committer,
committer: committer,
tree: revert_tree_id,
- parents: [rugged.lookup(source_sha)])
+ parents: [source_commit.sha])
end
end
@@ -961,10 +958,8 @@ class Repository
GitOperationService.new(user, self).with_branch(
branch_name,
source_branch_name: source_branch_name,
- source_project: source_project) do
+ source_project: source_project) do |source_commit|
- source_sha = source_project.repository.find_source_sha(
- source_branch_name || branch_name)
committer = user_to_committer(user)
Rugged::Commit.create(rugged,
@@ -976,7 +971,7 @@ class Repository
},
committer: committer,
tree: cherry_pick_tree_id,
- parents: [rugged.lookup(source_sha)])
+ parents: [source_commit.sha])
end
end
@@ -1145,16 +1140,6 @@ class Repository
end
end
- protected
-
- def find_source_sha(branch_name)
- if branch_exists?(branch_name)
- find_branch(branch_name).dereferenced_target.sha
- else
- Gitlab::Git::BLANK_SHA
- end
- end
-
private
def git_action(index, action)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index a7d267cd6b4..62a9eda3eba 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -37,13 +37,16 @@ GitOperationService = Struct.new(:user, :repository) do
check_with_branch_arguments!(
branch_name, source_branch_name, source_project)
- update_branch_with_hooks(branch_name) do |ref|
+ source_commit = source_project.repository.find_branch(
+ source_branch_name || branch_name).try(:dereferenced_target)
+
+ update_branch_with_hooks(branch_name) do
if repository.project == source_project
- yield(ref)
+ yield(source_commit)
else
repository.with_tmp_ref(
source_project.repository, source_branch_name) do
- yield(ref)
+ yield(source_commit)
end
end
end
@@ -54,31 +57,29 @@ GitOperationService = Struct.new(:user, :repository) do
def update_branch_with_hooks(branch_name)
update_autocrlf_option
- ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
- oldrev = Gitlab::Git::BLANK_SHA
was_empty = repository.empty?
# Make commit
- newrev = yield(ref)
+ newrev = yield
unless newrev
raise Repository::CommitError.new('Failed to create commit')
end
branch = repository.find_branch(branch_name)
- oldrev = if repository.rugged.lookup(newrev).parent_ids.empty? ||
- branch.nil?
- Gitlab::Git::BLANK_SHA
+ oldrev = if branch
+ # This could verify we're not losing commits
+ repository.rugged.merge_base(newrev, branch.target)
else
- repository.rugged.merge_base(
- newrev, branch.dereferenced_target.sha)
+ Gitlab::Git::BLANK_SHA
end
+ ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
with_hooks_and_update_ref(ref, oldrev, newrev) do
# If repo was empty expire cache
repository.after_create if was_empty
repository.after_create_branch if was_empty ||
- oldrev == Gitlab::Git::BLANK_SHA
+ Gitlab::Git.blank_ref?(oldrev)
end
newrev
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 9cc13a9a25b..a61d7f0c76d 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -257,6 +257,24 @@ describe Repository, models: true do
expect(newdir.path).to eq('newdir')
end
+ context "when committing to another project" do
+ let(:forked_project) { create(:project) }
+
+ it "creates a fork and commit to the forked project" do
+ expect do
+ repository.commit_dir(user, 'newdir',
+ message: 'Create newdir', branch_name: 'patch',
+ source_branch_name: 'master', source_project: forked_project)
+ end.to change { repository.commits('master').count }.by(0)
+
+ expect(repository.branch_exists?('patch')).to be_truthy
+ expect(forked_project.repository.branch_exists?('patch')).to be_falsy
+
+ newdir = repository.tree('patch', 'newdir')
+ expect(newdir.path).to eq('newdir')
+ end
+ end
+
context "when an author is specified" do
it "uses the given email/name to set the commit's author" do
expect do
@@ -758,9 +776,9 @@ describe Repository, models: true do
end
context 'when the update adds more than one commit' do
- it 'runs without errors' do
- old_rev = '33f3729a45c02fc67d00adb1b8bca394b0e761d9'
+ let(:old_rev) { '33f3729a45c02fc67d00adb1b8bca394b0e761d9' }
+ it 'runs without errors' do
# old_rev is an ancestor of new_rev
expect(repository.rugged.merge_base(old_rev, new_rev)).to eq(old_rev)
@@ -779,10 +797,10 @@ describe Repository, models: true do
end
context 'when the update would remove commits from the target branch' do
- it 'raises an exception' do
- branch = 'master'
- old_rev = repository.find_branch(branch).dereferenced_target.sha
+ let(:branch) { 'master' }
+ let(:old_rev) { repository.find_branch(branch).dereferenced_target.sha }
+ it 'raises an exception' do
# The 'master' branch is NOT an ancestor of new_rev.
expect(repository.rugged.merge_base(old_rev, new_rev)).not_to eq(old_rev)
--
GitLab
From 3e01385bca92dc8c0df3aa4032cc58d708dc0ff5 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 10 Dec 2016 01:23:49 +0800
Subject: [PATCH 46/78] Should pass branch name, not commit object!
---
app/services/compare_service.rb | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index fcbfe68f1a3..31c371c4b34 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -3,15 +3,16 @@ require 'securerandom'
# Compare 2 branches for one repo or between repositories
# and return Gitlab::Git::Compare object that responds to commits and diffs
class CompareService
- attr_reader :source_project, :source_branch
+ attr_reader :source_project, :source_branch_name
- def initialize(new_source_project, source_branch_name)
+ def initialize(new_source_project, new_source_branch_name)
@source_project = new_source_project
- @source_branch = new_source_project.commit(source_branch_name)
+ @source_branch_name = new_source_branch_name
end
def execute(target_project, target_branch, straight: false)
- source_sha = source_branch.try(:sha)
+ source_sha = source_project.repository.
+ commit(source_branch_name).try(:sha)
return unless source_sha
@@ -20,7 +21,7 @@ class CompareService
compare(source_sha, target_project, target_branch, straight)
else
target_project.repository.with_tmp_ref(
- source_project.repository, source_branch) do
+ source_project.repository, source_branch_name) do
compare(source_sha, target_project, target_branch, straight)
end
end
--
GitLab
From c0dfa0c609805558b30f11046eedadfb8a14886d Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 12 Dec 2016 23:07:52 +0800
Subject: [PATCH 47/78] Not sure why, but apparently SHA works better
It's very weird that source_commit.raw_commit and
rugged.branches[merge_request.target_branch].target
should be completely the same. I checked with ==
and other values which proved that both should be
the same, but still tests cannot pass for:
spec/services/merge_requests/refresh_service_spec.rb
I decided to give it up. We could just use SHA and
that works fine anyway.
---
app/models/repository.rb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 44f66b89600..7a7236993a8 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -903,8 +903,8 @@ class Repository
def merge(user, merge_request, options = {})
GitOperationService.new(user, self).with_branch(
merge_request.target_branch) do |source_commit|
- our_commit = source_commit.raw_commit
- their_commit = rugged.lookup(merge_request.diff_head_sha)
+ our_commit = source_commit.sha
+ their_commit = merge_request.diff_head_sha
raise 'Invalid merge target' unless our_commit
raise 'Invalid merge source' unless their_commit
--
GitLab
From 26af4b5a61d5cdaffa7769336f40cd0861f6b1d4 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Dec 2016 01:21:48 +0800
Subject: [PATCH 48/78] Also check blob path from source branch
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747244
---
app/models/repository.rb | 57 +++++++++++++++++++++++++++-------------
1 file changed, 39 insertions(+), 18 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 7a7236993a8..2e706b770b2 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -738,19 +738,11 @@ class Repository
message:, branch_name:,
author_email: nil, author_name: nil,
source_branch_name: nil, source_project: project)
- if branch_exists?(branch_name)
- # tree_entry is private
- entry = raw_repository.send(:tree_entry, commit(branch_name), path)
-
- if entry
- if entry[:type] == :blob
- raise Gitlab::Git::Repository::InvalidBlobName.new(
- "Directory already exists as a file")
- else
- raise Gitlab::Git::Repository::InvalidBlobName.new(
- "Directory already exists")
- end
- end
+ check_tree_entry_for_dir(branch_name, path)
+
+ if source_branch_name
+ source_project.repository.
+ check_tree_entry_for_dir(source_branch_name, path)
end
commit_file(
@@ -773,11 +765,16 @@ class Repository
message:, branch_name:, update: true,
author_email: nil, author_name: nil,
source_branch_name: nil, source_project: project)
- if branch_exists?(branch_name) && update == false
- # tree_entry is private
- if raw_repository.send(:tree_entry, commit(branch_name), path)
- raise Gitlab::Git::Repository::InvalidBlobName.new(
- "Filename already exists; update not allowed")
+ unless update
+ error_message = "Filename already exists; update not allowed"
+
+ if tree_entry_at(branch_name, path)
+ raise Gitlab::Git::Repository::InvalidBlobName.new(error_message)
+ end
+
+ if source_branch_name &&
+ source_project.repository.tree_entry_at(source_branch_name, path)
+ raise Gitlab::Git::Repository::InvalidBlobName.new(error_message)
end
end
@@ -1140,6 +1137,30 @@ class Repository
end
end
+ protected
+
+ def tree_entry_at(branch_name, path)
+ branch_exists?(branch_name) &&
+ # tree_entry is private
+ raw_repository.send(:tree_entry, commit(branch_name), path)
+ end
+
+ def check_tree_entry_for_dir(branch_name, path)
+ return unless branch_exists?(branch_name)
+
+ entry = tree_entry_at(branch_name, path)
+
+ return unless entry
+
+ if entry[:type] == :blob
+ raise Gitlab::Git::Repository::InvalidBlobName.new(
+ "Directory already exists as a file")
+ else
+ raise Gitlab::Git::Repository::InvalidBlobName.new(
+ "Directory already exists")
+ end
+ end
+
private
def git_action(index, action)
--
GitLab
From dc4b3dd0ae5d6e0b55ba6723e5deff6eee127409 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Dec 2016 01:47:17 +0800
Subject: [PATCH 49/78] Fix source_project and also pass source_project
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747556
---
app/controllers/concerns/creates_commit.rb | 4 ++--
app/services/commits/change_service.rb | 1 +
app/services/files/create_dir_service.rb | 1 +
app/services/files/create_service.rb | 1 +
app/services/files/delete_service.rb | 1 +
app/services/files/multi_service.rb | 1 +
6 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index d2fcb2efd6a..a94077c2bd4 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -6,12 +6,12 @@ module CreatesCommit
source_branch = @ref if @ref && @repository.find_branch(@ref)
commit_params = @commit_params.merge(
- source_project: @project,
+ source_project: @tree_edit_project,
source_branch: source_branch,
target_branch: @target_branch
)
- result = service.new(@tree_edit_project, current_user, commit_params).execute
+ result = service.new(@project, current_user, commit_params).execute
if result[:status] == :success
update_flash_notice(success_notice)
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 99d459908e7..9c630f5bbf1 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -37,6 +37,7 @@ module Commits
@commit,
into,
tree_id,
+ source_project: @source_project,
source_branch_name: @target_branch)
success
diff --git a/app/services/files/create_dir_service.rb b/app/services/files/create_dir_service.rb
index 4a2b2e8fcaf..ee4e130a38f 100644
--- a/app/services/files/create_dir_service.rb
+++ b/app/services/files/create_dir_service.rb
@@ -8,6 +8,7 @@ module Files
branch_name: @target_branch,
author_email: @author_email,
author_name: @author_name,
+ source_project: @source_project,
source_branch_name: @source_branch)
end
diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index c95cb75f7cb..853c471666d 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -10,6 +10,7 @@ module Files
update: false,
author_email: @author_email,
author_name: @author_name,
+ source_project: @source_project,
source_branch_name: @source_branch)
end
diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb
index 45a9a559469..cfe532d49b3 100644
--- a/app/services/files/delete_service.rb
+++ b/app/services/files/delete_service.rb
@@ -8,6 +8,7 @@ module Files
branch_name: @target_branch,
author_email: @author_email,
author_name: @author_name,
+ source_project: @source_project,
source_branch_name: @source_branch)
end
end
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index 42ed97ca3c0..f77e5d91103 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -10,6 +10,7 @@ module Files
actions: params[:actions],
author_email: @author_email,
author_name: @author_name,
+ source_project: @source_project,
source_branch_name: @source_branch
)
end
--
GitLab
From 46d752ce218d833ff947bd4503de56300471a8cb Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Dec 2016 02:03:04 +0800
Subject: [PATCH 50/78] Use a regular class for GitOperationService
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747793
---
app/services/git_operation_service.rb | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 62a9eda3eba..9a052f952cf 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -1,5 +1,12 @@
-GitOperationService = Struct.new(:user, :repository) do
+class GitOperationService
+ attr_reader :user, :repository
+
+ def initialize(new_user, new_repository)
+ @user = new_user
+ @repository = new_repository
+ end
+
def add_branch(branch_name, newrev)
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
oldrev = Gitlab::Git::BLANK_SHA
--
GitLab
From d03c605bd4a128d45179dd05f117a78aab7af6be Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Dec 2016 02:29:35 +0800
Subject: [PATCH 51/78] Unify parameters and callback after hooks
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747856
---
app/services/git_operation_service.rb | 34 +++++++++----------
lib/gitlab/git.rb | 2 +-
spec/models/repository_spec.rb | 7 ++--
.../git_garbage_collect_worker_spec.rb | 2 +-
4 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 9a052f952cf..68b28231595 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -11,7 +11,7 @@ class GitOperationService
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
oldrev = Gitlab::Git::BLANK_SHA
- with_hooks_and_update_ref(ref, oldrev, newrev)
+ update_ref_in_hooks(ref, newrev, oldrev)
end
def rm_branch(branch)
@@ -19,14 +19,14 @@ class GitOperationService
oldrev = branch.dereferenced_target.id
newrev = Gitlab::Git::BLANK_SHA
- with_hooks_and_update_ref(ref, oldrev, newrev)
+ update_ref_in_hooks(ref, newrev, oldrev)
end
def add_tag(tag_name, newrev, options = {})
ref = Gitlab::Git::TAG_REF_PREFIX + tag_name
oldrev = Gitlab::Git::BLANK_SHA
- with_hooks(ref, oldrev, newrev) do |service|
+ with_hooks(ref, newrev, oldrev) do |service|
raw_tag = repository.rugged.tags.create(tag_name, newrev, options)
service.newrev = raw_tag.target_id
end
@@ -82,25 +82,23 @@ class GitOperationService
end
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
- with_hooks_and_update_ref(ref, oldrev, newrev) do
- # If repo was empty expire cache
- repository.after_create if was_empty
- repository.after_create_branch if was_empty ||
- Gitlab::Git.blank_ref?(oldrev)
- end
+ update_ref_in_hooks(ref, newrev, oldrev)
+
+ # If repo was empty expire cache
+ repository.after_create if was_empty
+ repository.after_create_branch if was_empty ||
+ Gitlab::Git.blank_ref?(oldrev)
newrev
end
- def with_hooks_and_update_ref(ref, oldrev, newrev)
- with_hooks(ref, oldrev, newrev) do |service|
- update_ref!(ref, newrev, oldrev)
-
- yield(service) if block_given?
+ def update_ref_in_hooks(ref, newrev, oldrev)
+ with_hooks(ref, newrev, oldrev) do
+ update_ref(ref, newrev, oldrev)
end
end
- def with_hooks(ref, oldrev, newrev)
+ def with_hooks(ref, newrev, oldrev)
result = nil
GitHooksService.new.execute(
@@ -116,7 +114,7 @@ class GitOperationService
result
end
- def update_ref!(name, newrev, oldrev)
+ def update_ref(ref, newrev, oldrev)
# We use 'git update-ref' because libgit2/rugged currently does not
# offer 'compare and swap' ref updates. Without compare-and-swap we can
# (and have!) accidentally reset the ref to an earlier state, clobbering
@@ -125,12 +123,12 @@ class GitOperationService
_, status = Gitlab::Popen.popen(
command,
repository.path_to_repo) do |stdin|
- stdin.write("update #{name}\x00#{newrev}\x00#{oldrev}\x00")
+ stdin.write("update #{ref}\x00#{newrev}\x00#{oldrev}\x00")
end
unless status.zero?
raise Repository::CommitError.new(
- "Could not update branch #{name.sub('refs/heads/', '')}." \
+ "Could not update branch #{Gitlab::Git.branch_name(ref)}." \
" Please refresh and try again.")
end
end
diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb
index 3cd515e4a3a..d3df3f1bca1 100644
--- a/lib/gitlab/git.rb
+++ b/lib/gitlab/git.rb
@@ -6,7 +6,7 @@ module Gitlab
class << self
def ref_name(ref)
- ref.gsub(/\Arefs\/(tags|heads)\//, '')
+ ref.sub(/\Arefs\/(tags|heads)\//, '')
end
def branch_name(ref)
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index a61d7f0c76d..65e96351033 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -829,7 +829,6 @@ describe Repository, models: true do
context 'when target branch is different from source branch' do
before do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, ''])
- allow(repository).to receive(:update_ref!)
end
it 'expires branch cache' do
@@ -1474,16 +1473,16 @@ describe Repository, models: true do
end
end
- describe '#update_ref!' do
+ describe '#update_ref' do
it 'can create a ref' do
- GitOperationService.new(nil, repository).send(:update_ref!, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+ GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
expect(repository.find_branch('foobar')).not_to be_nil
end
it 'raises CommitError when the ref update fails' do
expect do
- GitOperationService.new(nil, repository).send(:update_ref!, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+ GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
end.to raise_error(Repository::CommitError)
end
end
diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb
index 2b31efbf631..5ef8cf1105b 100644
--- a/spec/workers/git_garbage_collect_worker_spec.rb
+++ b/spec/workers/git_garbage_collect_worker_spec.rb
@@ -108,7 +108,7 @@ describe GitGarbageCollectWorker do
parents: [old_commit],
)
GitOperationService.new(nil, project.repository).send(
- :update_ref!,
+ :update_ref,
"refs/heads/#{SecureRandom.hex(6)}",
new_commit_sha,
Gitlab::Git::BLANK_SHA
--
GitLab
From 944a8fa4d204ce7e9967f372a61657e75b4e88a0 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Dec 2016 03:00:16 +0800
Subject: [PATCH 52/78] Use branch_exists? to check branches
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747922
---
app/services/git_operation_service.rb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 68b28231595..b00fbcf9a79 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -150,14 +150,14 @@ class GitOperationService
' :source_project is different from current project'
end
- unless source_project.repository.commit(source_branch_name).try(:sha)
+ unless source_project.repository.branch_exists?(source_branch_name)
raise Repository::CommitError.new(
"Cannot find branch #{branch_name} nor" \
" #{source_branch_name} from" \
" #{source_project.path_with_namespace}")
end
elsif source_branch_name
- unless repository.commit(source_branch_name).try(:sha)
+ unless repository.branch_exists?(source_branch_name)
raise Repository::CommitError.new(
"Cannot find branch #{branch_name} nor" \
" #{source_branch_name} from" \
--
GitLab
From 56d131dcd52cba98e0eee253cab8bbf0b3b706df Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Dec 2016 03:03:20 +0800
Subject: [PATCH 53/78] Use ArgumentError error instead because it's a bug
if it happens. Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_19747933
---
app/services/git_operation_service.rb | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index b00fbcf9a79..0d1bd05e552 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -151,17 +151,17 @@ class GitOperationService
end
unless source_project.repository.branch_exists?(source_branch_name)
- raise Repository::CommitError.new(
+ raise ArgumentError,
"Cannot find branch #{branch_name} nor" \
" #{source_branch_name} from" \
- " #{source_project.path_with_namespace}")
+ " #{source_project.path_with_namespace}"
end
elsif source_branch_name
unless repository.branch_exists?(source_branch_name)
- raise Repository::CommitError.new(
+ raise ArgumentError,
"Cannot find branch #{branch_name} nor" \
" #{source_branch_name} from" \
- " #{repository.project.path_with_namespace}")
+ " #{repository.project.path_with_namespace}"
end
end
end
--
GitLab
From 99b556976370bfe0c052d15b6a8f0642256173fd Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Dec 2016 03:33:43 +0800
Subject: [PATCH 54/78] Try to use those @mr variables for full correctness
---
app/controllers/concerns/creates_commit.rb | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index a94077c2bd4..f0e6fc4b3e8 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,14 +4,16 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
- source_branch = @ref if @ref && @repository.find_branch(@ref)
+ source_branch = @ref if @ref &&
+ @mr_source_project.repository.branch_exists?(@ref)
commit_params = @commit_params.merge(
- source_project: @tree_edit_project,
+ source_project: @mr_source_project,
source_branch: source_branch,
- target_branch: @target_branch
+ target_branch: @mr_target_branch
)
- result = service.new(@project, current_user, commit_params).execute
+ result = service.new(
+ @mr_target_project, current_user, commit_params).execute
if result[:status] == :success
update_flash_notice(success_notice)
--
GitLab
From 14c4db2ae4efa1187476f11569df1b77c9c055fa Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 4 Jan 2017 22:31:06 +0800
Subject: [PATCH 55/78] Add a comment to explain why newrev should be updated
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_20301332
---
app/services/git_operation_service.rb | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 0d1bd05e552..5acfdb0f9e2 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -1,4 +1,3 @@
-
class GitOperationService
attr_reader :user, :repository
@@ -28,6 +27,9 @@ class GitOperationService
with_hooks(ref, newrev, oldrev) do |service|
raw_tag = repository.rugged.tags.create(tag_name, newrev, options)
+
+ # If raw_tag is an annotated tag, we'll need to update newrev to point
+ # to the new revision.
service.newrev = raw_tag.target_id
end
end
--
GitLab
From c1a75c3c0b59af7a9e6af3ff834adf56256aa2ce Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 4 Jan 2017 22:32:36 +0800
Subject: [PATCH 56/78] Prefer leading dots over trailing dots
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_20601323
---
app/controllers/projects/compare_controller.rb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 325987199fa..d359920c91e 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -37,8 +37,8 @@ class Projects::CompareController < Projects::ApplicationController
end
def define_diff_vars
- @compare = CompareService.new(@project, @head_ref).
- execute(@project, @start_ref)
+ @compare = CompareService.new(@project, @head_ref)
+ .execute(@project, @start_ref)
if @compare
@commits = @compare.commits
--
GitLab
From ecac2f1122f093455e7b9e5d054157241dcd5cff Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 4 Jan 2017 22:50:01 +0800
Subject: [PATCH 57/78] Update the comment:
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_20876648
---
app/services/git_operation_service.rb | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 5acfdb0f9e2..f36c0b082e4 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -26,10 +26,12 @@ class GitOperationService
oldrev = Gitlab::Git::BLANK_SHA
with_hooks(ref, newrev, oldrev) do |service|
+ # We want to pass the OID of the tag object to the hooks. For an
+ # annotated tag we don't know that OID until after the tag object
+ # (raw_tag) is created in the repository. That is why we have to
+ # update the value after creating the tag object. Only the
+ # "post-receive" hook will receive the correct value in this case.
raw_tag = repository.rugged.tags.create(tag_name, newrev, options)
-
- # If raw_tag is an annotated tag, we'll need to update newrev to point
- # to the new revision.
service.newrev = raw_tag.target_id
end
end
--
GitLab
From 05d742a047cca3ded10e6e3a545e211a3592c89c Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 5 Jan 2017 01:10:35 +0800
Subject: [PATCH 58/78] Indent the way rubocop likes
---
app/controllers/concerns/creates_commit.rb | 4 ++--
app/models/repository.rb | 2 +-
app/services/git_operation_service.rb | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 58de0917049..9d41c559b0b 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,8 +4,8 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
- source_branch = @ref if @ref &&
- @mr_source_project.repository.branch_exists?(@ref)
+ source_branch = @ref if
+ @ref && @mr_source_project.repository.branch_exists?(@ref)
commit_params = @commit_params.merge(
source_project: @mr_source_project,
source_branch: source_branch,
diff --git a/app/models/repository.rb b/app/models/repository.rb
index b01437acd8e..46995bdcb33 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -781,7 +781,7 @@ class Repository
end
if source_branch_name &&
- source_project.repository.tree_entry_at(source_branch_name, path)
+ source_project.repository.tree_entry_at(source_branch_name, path)
raise Gitlab::Git::Repository::InvalidBlobName.new(error_message)
end
end
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index f36c0b082e4..00c85112873 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -90,8 +90,8 @@ class GitOperationService
# If repo was empty expire cache
repository.after_create if was_empty
- repository.after_create_branch if was_empty ||
- Gitlab::Git.blank_ref?(oldrev)
+ repository.after_create_branch if
+ was_empty || Gitlab::Git.blank_ref?(oldrev)
newrev
end
--
GitLab
From 99ac0935271b1e99f4512e496104219045f1018e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 5 Jan 2017 01:52:21 +0800
Subject: [PATCH 59/78] Introduce Repository#with_repo_branch_commit
We merge repository checks inside it so we don't have to
check it on the call site, and we could also load the commit
for the caller. This greatly reduce code duplication.
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_20572919
---
app/models/repository.rb | 25 ++++++++++++++++---------
app/services/compare_service.rb | 18 ++++++------------
app/services/git_operation_service.rb | 18 ++++++------------
3 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 46995bdcb33..b1a789492d3 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -1063,19 +1063,26 @@ class Repository
Gitlab::Popen.popen(args, path_to_repo).first.lines.map(&:strip)
end
- def with_tmp_ref(source_repository, source_branch_name)
- tmp_ref = "refs/tmp/#{SecureRandom.hex}/head"
+ def with_repo_branch_commit(source_repository, source_branch_name)
+ branch_name_or_sha =
+ if source_repository == self
+ source_branch_name
+ else
+ tmp_ref = "refs/tmp/#{SecureRandom.hex}/head"
- fetch_ref(
- source_repository.path_to_repo,
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
- tmp_ref
- )
+ fetch_ref(
+ source_repository.path_to_repo,
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
+ tmp_ref
+ )
+
+ source_repository.commit(source_branch_name).sha
+ end
- yield
+ yield(commit(branch_name_or_sha))
ensure
- rugged.references.delete(tmp_ref)
+ rugged.references.delete(tmp_ref) if tmp_ref
end
def fetch_ref(source_path, source_ref, target_ref)
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index 31c371c4b34..d3d613661a6 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -11,19 +11,13 @@ class CompareService
end
def execute(target_project, target_branch, straight: false)
- source_sha = source_project.repository.
- commit(source_branch_name).try(:sha)
-
- return unless source_sha
-
# If compare with other project we need to fetch ref first
- if target_project == source_project
- compare(source_sha, target_project, target_branch, straight)
- else
- target_project.repository.with_tmp_ref(
- source_project.repository, source_branch_name) do
- compare(source_sha, target_project, target_branch, straight)
- end
+ target_project.repository.with_repo_branch_commit(
+ source_project.repository,
+ source_branch_name) do |commit|
+ break unless commit
+
+ compare(commit.sha, target_project, target_branch, straight)
end
end
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 00c85112873..ed9822cfee6 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -43,23 +43,17 @@ class GitOperationService
def with_branch(
branch_name,
source_branch_name: nil,
- source_project: repository.project)
+ source_project: repository.project,
+ &block)
check_with_branch_arguments!(
branch_name, source_branch_name, source_project)
- source_commit = source_project.repository.find_branch(
- source_branch_name || branch_name).try(:dereferenced_target)
-
update_branch_with_hooks(branch_name) do
- if repository.project == source_project
- yield(source_commit)
- else
- repository.with_tmp_ref(
- source_project.repository, source_branch_name) do
- yield(source_commit)
- end
- end
+ repository.with_repo_branch_commit(
+ source_project.repository,
+ source_branch_name || branch_name,
+ &block)
end
end
--
GitLab
From e01c692a35d817c09416356b549f473f63d78dc8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 5 Jan 2017 02:53:45 +0800
Subject: [PATCH 60/78] Remove tag with git hooks
---
app/models/repository.rb | 19 +++++++++++--------
app/services/delete_tag_service.rb | 2 +-
app/services/git_operation_service.rb | 12 +++++++++++-
spec/models/repository_spec.rb | 5 +++--
4 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index b1a789492d3..e834936aa93 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -196,16 +196,14 @@ class Repository
true
end
- # TODO: why we don't pass user here?
- def rm_tag(tag_name)
+ def rm_tag(user, tag_name)
before_remove_tag
+ tag = find_tag(tag_name)
- begin
- rugged.tags.delete(tag_name)
- true
- rescue Rugged::ReferenceError
- false
- end
+ GitOperationService.new(user, self).rm_tag(tag)
+
+ after_remove_tag
+ true
end
def ref_names
@@ -401,6 +399,11 @@ class Repository
repository_event(:remove_tag)
end
+ # Runs code after removing a tag.
+ def after_remove_tag
+ expire_tags_cache
+ end
+
def before_import
expire_content_cache
end
diff --git a/app/services/delete_tag_service.rb b/app/services/delete_tag_service.rb
index a44dee14a0f..9d4bffb93e9 100644
--- a/app/services/delete_tag_service.rb
+++ b/app/services/delete_tag_service.rb
@@ -7,7 +7,7 @@ class DeleteTagService < BaseService
return error('No such tag', 404)
end
- if repository.rm_tag(tag_name)
+ if repository.rm_tag(current_user, tag_name)
release = project.releases.find_by(tag: tag_name)
release.destroy if release
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index ed9822cfee6..3b7f702e3ab 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -15,7 +15,7 @@ class GitOperationService
def rm_branch(branch)
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch.name
- oldrev = branch.dereferenced_target.id
+ oldrev = branch.target
newrev = Gitlab::Git::BLANK_SHA
update_ref_in_hooks(ref, newrev, oldrev)
@@ -36,6 +36,16 @@ class GitOperationService
end
end
+ def rm_tag(tag)
+ ref = Gitlab::Git::TAG_REF_PREFIX + tag.name
+ oldrev = tag.target
+ newrev = Gitlab::Git::BLANK_SHA
+
+ update_ref_in_hooks(ref, newrev, oldrev) do
+ repository.rugged.tags.delete(tag_name)
+ end
+ end
+
# Whenever `source_branch_name` is passed, if `branch_name` doesn't exist,
# it would be created from `source_branch_name`.
# If `source_project` is passed, and the branch doesn't exist,
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index b1fee342b57..0f43c5c019a 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1419,9 +1419,10 @@ describe Repository, models: true do
describe '#rm_tag' do
it 'removes a tag' do
expect(repository).to receive(:before_remove_tag)
- expect(repository.rugged.tags).to receive(:delete).with('v1.1.0')
- repository.rm_tag('v1.1.0')
+ repository.rm_tag(create(:user), 'v1.1.0')
+
+ expect(repository.find_tag('v1.1.0')).to be_nil
end
end
--
GitLab
From 9244c81bc5f1c74966cb3ecb7b099fe7fd33689f Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 5 Jan 2017 03:01:16 +0800
Subject: [PATCH 61/78] I think I am really confused, should be
@tree_edit_project
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_20571990
---
app/controllers/concerns/creates_commit.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 9d41c559b0b..a2e1d7c4653 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -13,7 +13,7 @@ module CreatesCommit
)
result = service.new(
- @mr_target_project, current_user, commit_params).execute
+ @tree_edit_project, current_user, commit_params).execute
if result[:status] == :success
update_flash_notice(success_notice)
--
GitLab
From 0b3b56b34d959322cced8a317138945c685015b8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 5 Jan 2017 22:58:04 +0800
Subject: [PATCH 62/78] Merge request terms are reversed for
GitOperationService
This is seriously confusing but a target branch in merge request,
is actually the source branch for GitOperationService, which is
the base branch. The source branch in a merge request, is the
target branch for GitOperationService, which is where we want to
make commits.
Perhaps we should rename source branch in GitOperationService to
base branch, and target branch to committing branch.
---
app/controllers/concerns/creates_commit.rb | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index a2e1d7c4653..025fca088bf 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -5,15 +5,15 @@ module CreatesCommit
set_commit_variables
source_branch = @ref if
- @ref && @mr_source_project.repository.branch_exists?(@ref)
+ @ref && @mr_target_project.repository.branch_exists?(@ref)
commit_params = @commit_params.merge(
- source_project: @mr_source_project,
+ source_project: @mr_target_project,
source_branch: source_branch,
- target_branch: @mr_target_branch
+ target_branch: @mr_source_branch
)
result = service.new(
- @tree_edit_project, current_user, commit_params).execute
+ @mr_source_project, current_user, commit_params).execute
if result[:status] == :success
update_flash_notice(success_notice)
--
GitLab
From 5e12b3d841b0da1a2c6047de53a033107bbb5c32 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 5 Jan 2017 23:49:11 +0800
Subject: [PATCH 63/78] Prefer leading dots over trailing dots
---
app/services/create_branch_service.rb | 4 ++--
app/workers/emails_on_push_worker.rb | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index 1b5e504573a..77459d8779d 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -1,7 +1,7 @@
class CreateBranchService < BaseService
def execute(branch_name, ref)
- result = ValidateNewBranchService.new(project, current_user).
- execute(branch_name)
+ result = ValidateNewBranchService.new(project, current_user)
+ .execute(branch_name)
return result if result[:status] == :error
diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb
index d4c3f14ec06..f5ccc84c160 100644
--- a/app/workers/emails_on_push_worker.rb
+++ b/app/workers/emails_on_push_worker.rb
@@ -33,15 +33,15 @@ class EmailsOnPushWorker
reverse_compare = false
if action == :push
- compare = CompareService.new(project, after_sha).
- execute(project, before_sha)
+ compare = CompareService.new(project, after_sha)
+ .execute(project, before_sha)
diff_refs = compare.diff_refs
return false if compare.same
if compare.commits.empty?
- compare = CompareService.new(project, before_sha).
- execute(project, after_sha)
+ compare = CompareService.new(project, before_sha)
+ .execute(project, after_sha)
diff_refs = compare.diff_refs
reverse_compare = true
--
GitLab
From ae86a1b9d3c9ca4ce592fa89085acd059ffc09a0 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 02:11:27 +0800
Subject: [PATCH 64/78] Just trust set_commit_variables to set everything!
Removing those weird setup in assign_change_commit_vars
fixed all the failures in the tests. I still cannot say
why but clearly we need to have better names. It's so
confusing right now. We should seriously stop fiddling
those instance variables.
---
app/controllers/concerns/creates_commit.rb | 4 +---
app/controllers/projects/commit_controller.rb | 8 +++-----
app/services/commits/change_service.rb | 3 ++-
3 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 025fca088bf..eafaed8a3d0 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,11 +4,9 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
- source_branch = @ref if
- @ref && @mr_target_project.repository.branch_exists?(@ref)
commit_params = @commit_params.merge(
source_project: @mr_target_project,
- source_branch: source_branch,
+ source_branch: @mr_target_branch,
target_branch: @mr_source_branch
)
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 791ed88db30..44f34006049 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -40,7 +40,7 @@ class Projects::CommitController < Projects::ApplicationController
end
def revert
- assign_change_commit_vars(@commit.revert_branch_name)
+ assign_change_commit_vars
return render_404 if @target_branch.blank?
@@ -49,7 +49,7 @@ class Projects::CommitController < Projects::ApplicationController
end
def cherry_pick
- assign_change_commit_vars(@commit.cherry_pick_branch_name)
+ assign_change_commit_vars
return render_404 if @target_branch.blank?
@@ -110,11 +110,9 @@ class Projects::CommitController < Projects::ApplicationController
@ci_pipelines = project.pipelines.where(sha: commit.sha)
end
- def assign_change_commit_vars(mr_source_branch)
+ def assign_change_commit_vars
@commit = project.commit(params[:id])
@target_branch = params[:target_branch]
- @mr_source_branch = mr_source_branch
- @mr_target_branch = @target_branch
@commit_params = {
commit: @commit,
create_merge_request: params[:create_merge_request].present? || different_project?
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 9b241aa8b04..60bd59a5d9f 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -5,6 +5,7 @@ module Commits
def execute
@source_project = params[:source_project] || @project
+ @source_branch = params[:source_branch]
@target_branch = params[:target_branch]
@commit = params[:commit]
@create_merge_request = params[:create_merge_request].present?
@@ -38,7 +39,7 @@ module Commits
into,
tree_id,
source_project: @source_project,
- source_branch_name: @target_branch)
+ source_branch_name: @source_branch)
success
else
--
GitLab
From a30f278bdee399346f199ada0e33f5c2d233d861 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 04:18:51 +0800
Subject: [PATCH 65/78] Fix for initial commit and remove unneeded args
---
app/controllers/concerns/creates_commit.rb | 12 +++++++++---
app/services/commits/change_service.rb | 11 +++++++++--
lib/api/commits.rb | 2 --
3 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index eafaed8a3d0..f5f9cdeaec5 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,9 +4,10 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
+ source_branch = @mr_target_branch unless initial_commit?
commit_params = @commit_params.merge(
source_project: @mr_target_project,
- source_branch: @mr_target_branch,
+ source_branch: source_branch,
target_branch: @mr_source_branch
)
@@ -113,7 +114,7 @@ module CreatesCommit
else
# Merge request to this project
@mr_target_project = @project
- @mr_target_branch ||= @ref
+ @mr_target_branch = @ref || @target_branch
end
else
# Edit file in fork
@@ -121,7 +122,12 @@ module CreatesCommit
# Merge request from fork to this project
@mr_source_project = @tree_edit_project
@mr_target_project = @project
- @mr_target_branch ||= @ref
+ @mr_target_branch = @ref || @target_branch
end
end
+
+ def initial_commit?
+ @mr_target_branch.nil? ||
+ !@mr_target_project.repository.branch_exists?(@mr_target_branch)
+ end
end
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 60bd59a5d9f..4a5d8029413 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -26,8 +26,15 @@ module Commits
def commit_change(action)
raise NotImplementedError unless repository.respond_to?(action)
- into = @create_merge_request ? @commit.public_send("#{action}_branch_name") : @target_branch
- tree_id = repository.public_send("check_#{action}_content", @commit, @target_branch)
+ if @create_merge_request
+ into = @commit.public_send("#{action}_branch_name")
+ tree_branch = @source_branch
+ else
+ into = tree_branch = @target_branch
+ end
+
+ tree_id = repository.public_send(
+ "check_#{action}_content", @commit, tree_branch)
if tree_id
validate_target_branch(into) if @create_merge_request
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index cf2489dbb67..2c1da0902c9 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -140,8 +140,6 @@ module API
commit_params = {
commit: commit,
create_merge_request: false,
- source_project: user_project,
- source_branch: commit.cherry_pick_branch_name,
target_branch: params[:branch]
}
--
GitLab
From dea589d635d4c41fbf0db721b132ea466c34cb4a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 04:41:17 +0800
Subject: [PATCH 66/78] Prefer leading dots over trailing dots
---
app/models/merge_request_diff.rb | 4 ++--
app/services/commits/change_service.rb | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 7946d8e123e..00c2a3695af 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -169,8 +169,8 @@ class MergeRequestDiff < ActiveRecord::Base
# When compare merge request versions we want diff A..B instead of A...B
# so we handle cases when user does squash and rebase of the commits between versions.
# For this reason we set straight to true by default.
- CompareService.new(project, head_commit_sha).
- execute(project, sha, straight: straight)
+ CompareService.new(project, head_commit_sha)
+ .execute(project, sha, straight: straight)
end
def commits_count
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 4a5d8029413..8d1dfbcea7d 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -70,8 +70,8 @@ module Commits
# Temporary branch exists and contains the change commit
return if repository.find_branch(new_branch)
- result = ValidateNewBranchService.new(@project, current_user).
- execute(new_branch)
+ result = ValidateNewBranchService.new(@project, current_user)
+ .execute(new_branch)
if result[:status] == :error
raise ChangeError, "There was an error creating the source branch: #{result[:message]}"
--
GitLab
From 593228ffe3b2e4ff82c4d63e5d5c59b835f70085 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 20:59:38 +0800
Subject: [PATCH 67/78] Don't set invalid @mr_source_branch when
create_merge_request?
---
app/controllers/concerns/creates_commit.rb | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index f5f9cdeaec5..258791bb5cd 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -91,16 +91,13 @@ module CreatesCommit
@mr_source_project != @mr_target_project
end
- def different_branch?
- @mr_source_branch != @mr_target_branch || different_project?
- end
-
def create_merge_request?
- params[:create_merge_request].present? && different_branch?
+ params[:create_merge_request].present?
end
+ # TODO: We should really clean this up
def set_commit_variables
- @mr_source_branch ||= @target_branch
+ @mr_source_branch = @target_branch unless create_merge_request?
if can?(current_user, :push_code, @project)
# Edit file in this project
--
GitLab
From a4b97b2cb61c03d08e25cf2cd7fcbb3f21611350 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 22:05:30 +0800
Subject: [PATCH 68/78] Rename source to base to avoid confusion from MR
---
app/controllers/concerns/creates_commit.rb | 6 +-
app/models/repository.rb | 78 +++++++++++-----------
app/services/commits/change_service.rb | 10 +--
app/services/compare_service.rb | 12 ++--
app/services/files/base_service.rb | 10 +--
app/services/files/create_dir_service.rb | 4 +-
app/services/files/create_service.rb | 6 +-
app/services/files/delete_service.rb | 4 +-
app/services/files/multi_service.rb | 6 +-
app/services/files/update_service.rb | 6 +-
app/services/git_operation_service.rb | 40 +++++------
11 files changed, 91 insertions(+), 91 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 258791bb5cd..c503f8bf696 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,10 +4,10 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
- source_branch = @mr_target_branch unless initial_commit?
+ base_branch = @mr_target_branch unless initial_commit?
commit_params = @commit_params.merge(
- source_project: @mr_target_project,
- source_branch: source_branch,
+ base_project: @mr_target_project,
+ base_branch: base_branch,
target_branch: @mr_source_branch
)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index e834936aa93..a335c629a78 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -748,12 +748,12 @@ class Repository
user, path,
message:, branch_name:,
author_email: nil, author_name: nil,
- source_branch_name: nil, source_project: project)
+ base_branch_name: nil, base_project: project)
check_tree_entry_for_dir(branch_name, path)
- if source_branch_name
- source_project.repository.
- check_tree_entry_for_dir(source_branch_name, path)
+ if base_branch_name
+ base_project.repository.
+ check_tree_entry_for_dir(base_branch_name, path)
end
commit_file(
@@ -765,8 +765,8 @@ class Repository
update: false,
author_email: author_email,
author_name: author_name,
- source_branch_name: source_branch_name,
- source_project: source_project)
+ base_branch_name: base_branch_name,
+ base_project: base_project)
end
# rubocop:enable Metrics/ParameterLists
@@ -775,7 +775,7 @@ class Repository
user, path, content,
message:, branch_name:, update: true,
author_email: nil, author_name: nil,
- source_branch_name: nil, source_project: project)
+ base_branch_name: nil, base_project: project)
unless update
error_message = "Filename already exists; update not allowed"
@@ -783,8 +783,8 @@ class Repository
raise Gitlab::Git::Repository::InvalidBlobName.new(error_message)
end
- if source_branch_name &&
- source_project.repository.tree_entry_at(source_branch_name, path)
+ if base_branch_name &&
+ base_project.repository.tree_entry_at(base_branch_name, path)
raise Gitlab::Git::Repository::InvalidBlobName.new(error_message)
end
end
@@ -795,8 +795,8 @@ class Repository
branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- source_branch_name: source_branch_name,
- source_project: source_project,
+ base_branch_name: base_branch_name,
+ base_project: base_project,
actions: [{ action: :create,
file_path: path,
content: content }])
@@ -808,7 +808,7 @@ class Repository
user, path, content,
message:, branch_name:, previous_path:,
author_email: nil, author_name: nil,
- source_branch_name: nil, source_project: project)
+ base_branch_name: nil, base_project: project)
action = if previous_path && previous_path != path
:move
else
@@ -821,8 +821,8 @@ class Repository
branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- source_branch_name: source_branch_name,
- source_project: source_project,
+ base_branch_name: base_branch_name,
+ base_project: base_project,
actions: [{ action: action,
file_path: path,
content: content,
@@ -835,15 +835,15 @@ class Repository
user, path,
message:, branch_name:,
author_email: nil, author_name: nil,
- source_branch_name: nil, source_project: project)
+ base_branch_name: nil, base_project: project)
multi_action(
user: user,
message: message,
branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- source_branch_name: source_branch_name,
- source_project: source_project,
+ base_branch_name: base_branch_name,
+ base_project: base_project,
actions: [{ action: :delete,
file_path: path }])
end
@@ -853,16 +853,16 @@ class Repository
def multi_action(
user:, branch_name:, message:, actions:,
author_email: nil, author_name: nil,
- source_branch_name: nil, source_project: project)
+ base_branch_name: nil, base_project: project)
GitOperationService.new(user, self).with_branch(
branch_name,
- source_branch_name: source_branch_name,
- source_project: source_project) do |source_commit|
+ base_branch_name: base_branch_name,
+ base_project: base_project) do |base_commit|
index = rugged.index
- parents = if source_commit
- index.read_tree(source_commit.raw_commit.tree)
- [source_commit.sha]
+ parents = if base_commit
+ index.read_tree(base_commit.raw_commit.tree)
+ [base_commit.sha]
else
[]
end
@@ -910,8 +910,8 @@ class Repository
def merge(user, merge_request, options = {})
GitOperationService.new(user, self).with_branch(
- merge_request.target_branch) do |source_commit|
- our_commit = source_commit.sha
+ merge_request.target_branch) do |base_commit|
+ our_commit = base_commit.sha
their_commit = merge_request.diff_head_sha
raise 'Invalid merge target' unless our_commit
@@ -935,15 +935,15 @@ class Repository
def revert(
user, commit, branch_name, revert_tree_id = nil,
- source_branch_name: nil, source_project: project)
+ base_branch_name: nil, base_project: project)
revert_tree_id ||= check_revert_content(commit, branch_name)
return false unless revert_tree_id
GitOperationService.new(user, self).with_branch(
branch_name,
- source_branch_name: source_branch_name,
- source_project: source_project) do |source_commit|
+ base_branch_name: base_branch_name,
+ base_project: base_project) do |base_commit|
committer = user_to_committer(user)
@@ -952,21 +952,21 @@ class Repository
author: committer,
committer: committer,
tree: revert_tree_id,
- parents: [source_commit.sha])
+ parents: [base_commit.sha])
end
end
def cherry_pick(
user, commit, branch_name, cherry_pick_tree_id = nil,
- source_branch_name: nil, source_project: project)
+ base_branch_name: nil, base_project: project)
cherry_pick_tree_id ||= check_cherry_pick_content(commit, branch_name)
return false unless cherry_pick_tree_id
GitOperationService.new(user, self).with_branch(
branch_name,
- source_branch_name: source_branch_name,
- source_project: source_project) do |source_commit|
+ base_branch_name: base_branch_name,
+ base_project: base_project) do |base_commit|
committer = user_to_committer(user)
@@ -979,7 +979,7 @@ class Repository
},
committer: committer,
tree: cherry_pick_tree_id,
- parents: [source_commit.sha])
+ parents: [base_commit.sha])
end
end
@@ -1066,20 +1066,20 @@ class Repository
Gitlab::Popen.popen(args, path_to_repo).first.lines.map(&:strip)
end
- def with_repo_branch_commit(source_repository, source_branch_name)
+ def with_repo_branch_commit(base_repository, base_branch_name)
branch_name_or_sha =
- if source_repository == self
- source_branch_name
+ if base_repository == self
+ base_branch_name
else
tmp_ref = "refs/tmp/#{SecureRandom.hex}/head"
fetch_ref(
- source_repository.path_to_repo,
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_branch_name}",
+ base_repository.path_to_repo,
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{base_branch_name}",
tmp_ref
)
- source_repository.commit(source_branch_name).sha
+ base_repository.commit(base_branch_name).sha
end
yield(commit(branch_name_or_sha))
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 8d1dfbcea7d..1faa052e0ca 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -4,8 +4,8 @@ module Commits
class ChangeError < StandardError; end
def execute
- @source_project = params[:source_project] || @project
- @source_branch = params[:source_branch]
+ @base_project = params[:base_project] || @project
+ @base_branch = params[:base_branch]
@target_branch = params[:target_branch]
@commit = params[:commit]
@create_merge_request = params[:create_merge_request].present?
@@ -28,7 +28,7 @@ module Commits
if @create_merge_request
into = @commit.public_send("#{action}_branch_name")
- tree_branch = @source_branch
+ tree_branch = @base_branch
else
into = tree_branch = @target_branch
end
@@ -45,8 +45,8 @@ module Commits
@commit,
into,
tree_id,
- source_project: @source_project,
- source_branch_name: @source_branch)
+ base_project: @base_project,
+ base_branch_name: @base_branch)
success
else
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index d3d613661a6..2e4f8ee9dc8 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -3,18 +3,18 @@ require 'securerandom'
# Compare 2 branches for one repo or between repositories
# and return Gitlab::Git::Compare object that responds to commits and diffs
class CompareService
- attr_reader :source_project, :source_branch_name
+ attr_reader :base_project, :base_branch_name
- def initialize(new_source_project, new_source_branch_name)
- @source_project = new_source_project
- @source_branch_name = new_source_branch_name
+ def initialize(new_base_project, new_base_branch_name)
+ @base_project = new_base_project
+ @base_branch_name = new_base_branch_name
end
def execute(target_project, target_branch, straight: false)
# If compare with other project we need to fetch ref first
target_project.repository.with_repo_branch_commit(
- source_project.repository,
- source_branch_name) do |commit|
+ base_project.repository,
+ base_branch_name) do |commit|
break unless commit
compare(commit.sha, target_project, target_branch, straight)
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 80e1d1d60f2..89f7dcbaa87 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -3,8 +3,8 @@ module Files
class ValidationError < StandardError; end
def execute
- @source_project = params[:source_project] || @project
- @source_branch = params[:source_branch]
+ @base_project = params[:base_project] || @project
+ @base_branch = params[:base_branch]
@target_branch = params[:target_branch]
@commit_message = params[:commit_message]
@@ -22,7 +22,7 @@ module Files
# Validate parameters
validate
- # Create new branch if it different from source_branch
+ # Create new branch if it different from base_branch
validate_target_branch if different_branch?
result = commit
@@ -38,7 +38,7 @@ module Files
private
def different_branch?
- @source_branch != @target_branch || @source_project != @project
+ @base_branch != @target_branch || @base_project != @project
end
def file_has_changed?
@@ -59,7 +59,7 @@ module Files
end
unless project.empty_repo?
- unless @source_project.repository.branch_exists?(@source_branch)
+ unless @base_project.repository.branch_exists?(@base_branch)
raise_error('You can only create or edit files when you are on a branch')
end
diff --git a/app/services/files/create_dir_service.rb b/app/services/files/create_dir_service.rb
index ee4e130a38f..53b6d456e0d 100644
--- a/app/services/files/create_dir_service.rb
+++ b/app/services/files/create_dir_service.rb
@@ -8,8 +8,8 @@ module Files
branch_name: @target_branch,
author_email: @author_email,
author_name: @author_name,
- source_project: @source_project,
- source_branch_name: @source_branch)
+ base_project: @base_project,
+ base_branch_name: @base_branch)
end
def validate
diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index 853c471666d..270dc6471aa 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -10,8 +10,8 @@ module Files
update: false,
author_email: @author_email,
author_name: @author_name,
- source_project: @source_project,
- source_branch_name: @source_branch)
+ base_project: @base_project,
+ base_branch_name: @base_branch)
end
def validate
@@ -34,7 +34,7 @@ module Files
unless project.empty_repo?
@file_path.slice!(0) if @file_path.start_with?('/')
- blob = repository.blob_at_branch(@source_branch, @file_path)
+ blob = repository.blob_at_branch(@base_branch, @file_path)
if blob
raise_error('Your changes could not be committed because a file with the same name already exists')
diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb
index cfe532d49b3..d5341b9e197 100644
--- a/app/services/files/delete_service.rb
+++ b/app/services/files/delete_service.rb
@@ -8,8 +8,8 @@ module Files
branch_name: @target_branch,
author_email: @author_email,
author_name: @author_name,
- source_project: @source_project,
- source_branch_name: @source_branch)
+ base_project: @base_project,
+ base_branch_name: @base_branch)
end
end
end
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index f77e5d91103..ca13b887e06 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -10,8 +10,8 @@ module Files
actions: params[:actions],
author_email: @author_email,
author_name: @author_name,
- source_project: @source_project,
- source_branch_name: @source_branch
+ base_project: @base_project,
+ base_branch_name: @base_branch
)
end
@@ -63,7 +63,7 @@ module Files
end
def last_commit
- Gitlab::Git::Commit.last_for_path(repository, @source_branch, @file_path)
+ Gitlab::Git::Commit.last_for_path(repository, @base_branch, @file_path)
end
def regex_check(file)
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index 5f671817cdb..f546b169550 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -9,8 +9,8 @@ module Files
previous_path: @previous_path,
author_email: @author_email,
author_name: @author_name,
- source_project: @source_project,
- source_branch_name: @source_branch)
+ base_project: @base_project,
+ base_branch_name: @base_branch)
end
private
@@ -25,7 +25,7 @@ module Files
def last_commit
@last_commit ||= Gitlab::Git::Commit.
- last_for_path(@source_project.repository, @source_branch, @file_path)
+ last_for_path(@base_project.repository, @base_branch, @file_path)
end
end
end
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 3b7f702e3ab..ec23407544c 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -46,23 +46,23 @@ class GitOperationService
end
end
- # Whenever `source_branch_name` is passed, if `branch_name` doesn't exist,
- # it would be created from `source_branch_name`.
- # If `source_project` is passed, and the branch doesn't exist,
- # it would try to find the source from it instead of current repository.
+ # Whenever `base_branch_name` is passed, if `branch_name` doesn't exist,
+ # it would be created from `base_branch_name`.
+ # If `base_project` is passed, and the branch doesn't exist,
+ # it would try to find the base from it instead of current repository.
def with_branch(
branch_name,
- source_branch_name: nil,
- source_project: repository.project,
+ base_branch_name: nil,
+ base_project: repository.project,
&block)
check_with_branch_arguments!(
- branch_name, source_branch_name, source_project)
+ branch_name, base_branch_name, base_project)
update_branch_with_hooks(branch_name) do
repository.with_repo_branch_commit(
- source_project.repository,
- source_branch_name || branch_name,
+ base_project.repository,
+ base_branch_name || branch_name,
&block)
end
end
@@ -148,27 +148,27 @@ class GitOperationService
end
def check_with_branch_arguments!(
- branch_name, source_branch_name, source_project)
+ branch_name, base_branch_name, base_project)
return if repository.branch_exists?(branch_name)
- if repository.project != source_project
- unless source_branch_name
+ if repository.project != base_project
+ unless base_branch_name
raise ArgumentError,
- 'Should also pass :source_branch_name if' +
- ' :source_project is different from current project'
+ 'Should also pass :base_branch_name if' +
+ ' :base_project is different from current project'
end
- unless source_project.repository.branch_exists?(source_branch_name)
+ unless base_project.repository.branch_exists?(base_branch_name)
raise ArgumentError,
"Cannot find branch #{branch_name} nor" \
- " #{source_branch_name} from" \
- " #{source_project.path_with_namespace}"
+ " #{base_branch_name} from" \
+ " #{base_project.path_with_namespace}"
end
- elsif source_branch_name
- unless repository.branch_exists?(source_branch_name)
+ elsif base_branch_name
+ unless repository.branch_exists?(base_branch_name)
raise ArgumentError,
"Cannot find branch #{branch_name} nor" \
- " #{source_branch_name} from" \
+ " #{base_branch_name} from" \
" #{repository.project.path_with_namespace}"
end
end
--
GitLab
From 358501df2d3229f68be700d2fc57cd3c3e7e5042 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 22:20:02 +0800
Subject: [PATCH 69/78] Properly fix the edge case!
---
app/controllers/concerns/creates_commit.rb | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index c503f8bf696..516b1cac6ef 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -97,8 +97,6 @@ module CreatesCommit
# TODO: We should really clean this up
def set_commit_variables
- @mr_source_branch = @target_branch unless create_merge_request?
-
if can?(current_user, :push_code, @project)
# Edit file in this project
@tree_edit_project = @project
@@ -121,10 +119,25 @@ module CreatesCommit
@mr_target_project = @project
@mr_target_branch = @ref || @target_branch
end
+
+ @mr_source_branch = guess_mr_source_branch
end
def initial_commit?
@mr_target_branch.nil? ||
!@mr_target_project.repository.branch_exists?(@mr_target_branch)
end
+
+ def guess_mr_source_branch
+ # XXX: Happens when viewing a commit without a branch. In this case,
+ # @target_branch would be the default branch for @mr_source_project,
+ # however we want a generated new branch here. Thus we can't use
+ # @target_branch, but should pass nil to indicate that we want a new
+ # branch instead of @target_branch.
+ return if
+ create_merge_request? &&
+ @mr_source_project.repository.branch_exists?(@target_branch)
+
+ @target_branch
+ end
end
--
GitLab
From e3c36850a618ee2f7f9087b681e62d8a50e7b1b1 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 22:49:23 +0800
Subject: [PATCH 70/78] Detect if we really want a new merge request properly
---
app/controllers/concerns/creates_commit.rb | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 516b1cac6ef..646d922cb24 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -92,7 +92,9 @@ module CreatesCommit
end
def create_merge_request?
- params[:create_merge_request].present?
+ # XXX: Even if the field is set, if we're checking the same branch
+ # as the target branch, we don't want to create a merge request.
+ params[:create_merge_request].present? && @ref != @target_branch
end
# TODO: We should really clean this up
@@ -136,7 +138,8 @@ module CreatesCommit
# branch instead of @target_branch.
return if
create_merge_request? &&
- @mr_source_project.repository.branch_exists?(@target_branch)
+ # XXX: Don't understand why rubocop prefers this indention
+ @mr_source_project.repository.branch_exists?(@target_branch)
@target_branch
end
--
GitLab
From ccc73c455ba0b95b531c69414a6a1f47667f16b5 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 23:29:13 +0800
Subject: [PATCH 71/78] Rename from base to start because base could mean merge
base
---
app/controllers/concerns/creates_commit.rb | 6 +-
app/models/repository.rb | 78 +++++++++++-----------
app/services/commits/change_service.rb | 10 +--
app/services/compare_service.rb | 12 ++--
app/services/files/base_service.rb | 10 +--
app/services/files/create_dir_service.rb | 4 +-
app/services/files/create_service.rb | 6 +-
app/services/files/delete_service.rb | 4 +-
app/services/files/multi_service.rb | 6 +-
app/services/files/update_service.rb | 6 +-
app/services/git_operation_service.rb | 40 +++++------
11 files changed, 91 insertions(+), 91 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 646d922cb24..2ece99aebc0 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -4,10 +4,10 @@ module CreatesCommit
def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
set_commit_variables
- base_branch = @mr_target_branch unless initial_commit?
+ start_branch = @mr_target_branch unless initial_commit?
commit_params = @commit_params.merge(
- base_project: @mr_target_project,
- base_branch: base_branch,
+ start_project: @mr_target_project,
+ start_branch: start_branch,
target_branch: @mr_source_branch
)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index a335c629a78..f8bdfb602a9 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -748,12 +748,12 @@ class Repository
user, path,
message:, branch_name:,
author_email: nil, author_name: nil,
- base_branch_name: nil, base_project: project)
+ start_branch_name: nil, start_project: project)
check_tree_entry_for_dir(branch_name, path)
- if base_branch_name
- base_project.repository.
- check_tree_entry_for_dir(base_branch_name, path)
+ if start_branch_name
+ start_project.repository.
+ check_tree_entry_for_dir(start_branch_name, path)
end
commit_file(
@@ -765,8 +765,8 @@ class Repository
update: false,
author_email: author_email,
author_name: author_name,
- base_branch_name: base_branch_name,
- base_project: base_project)
+ start_branch_name: start_branch_name,
+ start_project: start_project)
end
# rubocop:enable Metrics/ParameterLists
@@ -775,7 +775,7 @@ class Repository
user, path, content,
message:, branch_name:, update: true,
author_email: nil, author_name: nil,
- base_branch_name: nil, base_project: project)
+ start_branch_name: nil, start_project: project)
unless update
error_message = "Filename already exists; update not allowed"
@@ -783,8 +783,8 @@ class Repository
raise Gitlab::Git::Repository::InvalidBlobName.new(error_message)
end
- if base_branch_name &&
- base_project.repository.tree_entry_at(base_branch_name, path)
+ if start_branch_name &&
+ start_project.repository.tree_entry_at(start_branch_name, path)
raise Gitlab::Git::Repository::InvalidBlobName.new(error_message)
end
end
@@ -795,8 +795,8 @@ class Repository
branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- base_branch_name: base_branch_name,
- base_project: base_project,
+ start_branch_name: start_branch_name,
+ start_project: start_project,
actions: [{ action: :create,
file_path: path,
content: content }])
@@ -808,7 +808,7 @@ class Repository
user, path, content,
message:, branch_name:, previous_path:,
author_email: nil, author_name: nil,
- base_branch_name: nil, base_project: project)
+ start_branch_name: nil, start_project: project)
action = if previous_path && previous_path != path
:move
else
@@ -821,8 +821,8 @@ class Repository
branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- base_branch_name: base_branch_name,
- base_project: base_project,
+ start_branch_name: start_branch_name,
+ start_project: start_project,
actions: [{ action: action,
file_path: path,
content: content,
@@ -835,15 +835,15 @@ class Repository
user, path,
message:, branch_name:,
author_email: nil, author_name: nil,
- base_branch_name: nil, base_project: project)
+ start_branch_name: nil, start_project: project)
multi_action(
user: user,
message: message,
branch_name: branch_name,
author_email: author_email,
author_name: author_name,
- base_branch_name: base_branch_name,
- base_project: base_project,
+ start_branch_name: start_branch_name,
+ start_project: start_project,
actions: [{ action: :delete,
file_path: path }])
end
@@ -853,16 +853,16 @@ class Repository
def multi_action(
user:, branch_name:, message:, actions:,
author_email: nil, author_name: nil,
- base_branch_name: nil, base_project: project)
+ start_branch_name: nil, start_project: project)
GitOperationService.new(user, self).with_branch(
branch_name,
- base_branch_name: base_branch_name,
- base_project: base_project) do |base_commit|
+ start_branch_name: start_branch_name,
+ start_project: start_project) do |start_commit|
index = rugged.index
- parents = if base_commit
- index.read_tree(base_commit.raw_commit.tree)
- [base_commit.sha]
+ parents = if start_commit
+ index.read_tree(start_commit.raw_commit.tree)
+ [start_commit.sha]
else
[]
end
@@ -910,8 +910,8 @@ class Repository
def merge(user, merge_request, options = {})
GitOperationService.new(user, self).with_branch(
- merge_request.target_branch) do |base_commit|
- our_commit = base_commit.sha
+ merge_request.target_branch) do |start_commit|
+ our_commit = start_commit.sha
their_commit = merge_request.diff_head_sha
raise 'Invalid merge target' unless our_commit
@@ -935,15 +935,15 @@ class Repository
def revert(
user, commit, branch_name, revert_tree_id = nil,
- base_branch_name: nil, base_project: project)
+ start_branch_name: nil, start_project: project)
revert_tree_id ||= check_revert_content(commit, branch_name)
return false unless revert_tree_id
GitOperationService.new(user, self).with_branch(
branch_name,
- base_branch_name: base_branch_name,
- base_project: base_project) do |base_commit|
+ start_branch_name: start_branch_name,
+ start_project: start_project) do |start_commit|
committer = user_to_committer(user)
@@ -952,21 +952,21 @@ class Repository
author: committer,
committer: committer,
tree: revert_tree_id,
- parents: [base_commit.sha])
+ parents: [start_commit.sha])
end
end
def cherry_pick(
user, commit, branch_name, cherry_pick_tree_id = nil,
- base_branch_name: nil, base_project: project)
+ start_branch_name: nil, start_project: project)
cherry_pick_tree_id ||= check_cherry_pick_content(commit, branch_name)
return false unless cherry_pick_tree_id
GitOperationService.new(user, self).with_branch(
branch_name,
- base_branch_name: base_branch_name,
- base_project: base_project) do |base_commit|
+ start_branch_name: start_branch_name,
+ start_project: start_project) do |start_commit|
committer = user_to_committer(user)
@@ -979,7 +979,7 @@ class Repository
},
committer: committer,
tree: cherry_pick_tree_id,
- parents: [base_commit.sha])
+ parents: [start_commit.sha])
end
end
@@ -1066,20 +1066,20 @@ class Repository
Gitlab::Popen.popen(args, path_to_repo).first.lines.map(&:strip)
end
- def with_repo_branch_commit(base_repository, base_branch_name)
+ def with_repo_branch_commit(start_repository, start_branch_name)
branch_name_or_sha =
- if base_repository == self
- base_branch_name
+ if start_repository == self
+ start_branch_name
else
tmp_ref = "refs/tmp/#{SecureRandom.hex}/head"
fetch_ref(
- base_repository.path_to_repo,
- "#{Gitlab::Git::BRANCH_REF_PREFIX}#{base_branch_name}",
+ start_repository.path_to_repo,
+ "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}",
tmp_ref
)
- base_repository.commit(base_branch_name).sha
+ start_repository.commit(start_branch_name).sha
end
yield(commit(branch_name_or_sha))
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index 1faa052e0ca..25e22f14e60 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -4,8 +4,8 @@ module Commits
class ChangeError < StandardError; end
def execute
- @base_project = params[:base_project] || @project
- @base_branch = params[:base_branch]
+ @start_project = params[:start_project] || @project
+ @start_branch = params[:start_branch]
@target_branch = params[:target_branch]
@commit = params[:commit]
@create_merge_request = params[:create_merge_request].present?
@@ -28,7 +28,7 @@ module Commits
if @create_merge_request
into = @commit.public_send("#{action}_branch_name")
- tree_branch = @base_branch
+ tree_branch = @start_branch
else
into = tree_branch = @target_branch
end
@@ -45,8 +45,8 @@ module Commits
@commit,
into,
tree_id,
- base_project: @base_project,
- base_branch_name: @base_branch)
+ start_project: @start_project,
+ start_branch_name: @start_branch)
success
else
diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb
index 2e4f8ee9dc8..ab4c02a97a0 100644
--- a/app/services/compare_service.rb
+++ b/app/services/compare_service.rb
@@ -3,18 +3,18 @@ require 'securerandom'
# Compare 2 branches for one repo or between repositories
# and return Gitlab::Git::Compare object that responds to commits and diffs
class CompareService
- attr_reader :base_project, :base_branch_name
+ attr_reader :start_project, :start_branch_name
- def initialize(new_base_project, new_base_branch_name)
- @base_project = new_base_project
- @base_branch_name = new_base_branch_name
+ def initialize(new_start_project, new_start_branch_name)
+ @start_project = new_start_project
+ @start_branch_name = new_start_branch_name
end
def execute(target_project, target_branch, straight: false)
# If compare with other project we need to fetch ref first
target_project.repository.with_repo_branch_commit(
- base_project.repository,
- base_branch_name) do |commit|
+ start_project.repository,
+ start_branch_name) do |commit|
break unless commit
compare(commit.sha, target_project, target_branch, straight)
diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index 89f7dcbaa87..0a25f56d24c 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -3,8 +3,8 @@ module Files
class ValidationError < StandardError; end
def execute
- @base_project = params[:base_project] || @project
- @base_branch = params[:base_branch]
+ @start_project = params[:start_project] || @project
+ @start_branch = params[:start_branch]
@target_branch = params[:target_branch]
@commit_message = params[:commit_message]
@@ -22,7 +22,7 @@ module Files
# Validate parameters
validate
- # Create new branch if it different from base_branch
+ # Create new branch if it different from start_branch
validate_target_branch if different_branch?
result = commit
@@ -38,7 +38,7 @@ module Files
private
def different_branch?
- @base_branch != @target_branch || @base_project != @project
+ @start_branch != @target_branch || @start_project != @project
end
def file_has_changed?
@@ -59,7 +59,7 @@ module Files
end
unless project.empty_repo?
- unless @base_project.repository.branch_exists?(@base_branch)
+ unless @start_project.repository.branch_exists?(@start_branch)
raise_error('You can only create or edit files when you are on a branch')
end
diff --git a/app/services/files/create_dir_service.rb b/app/services/files/create_dir_service.rb
index 53b6d456e0d..858de5f0538 100644
--- a/app/services/files/create_dir_service.rb
+++ b/app/services/files/create_dir_service.rb
@@ -8,8 +8,8 @@ module Files
branch_name: @target_branch,
author_email: @author_email,
author_name: @author_name,
- base_project: @base_project,
- base_branch_name: @base_branch)
+ start_project: @start_project,
+ start_branch_name: @start_branch)
end
def validate
diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index 270dc6471aa..88dd7bbaedb 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -10,8 +10,8 @@ module Files
update: false,
author_email: @author_email,
author_name: @author_name,
- base_project: @base_project,
- base_branch_name: @base_branch)
+ start_project: @start_project,
+ start_branch_name: @start_branch)
end
def validate
@@ -34,7 +34,7 @@ module Files
unless project.empty_repo?
@file_path.slice!(0) if @file_path.start_with?('/')
- blob = repository.blob_at_branch(@base_branch, @file_path)
+ blob = repository.blob_at_branch(@start_branch, @file_path)
if blob
raise_error('Your changes could not be committed because a file with the same name already exists')
diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb
index d5341b9e197..50f0ffcac9f 100644
--- a/app/services/files/delete_service.rb
+++ b/app/services/files/delete_service.rb
@@ -8,8 +8,8 @@ module Files
branch_name: @target_branch,
author_email: @author_email,
author_name: @author_name,
- base_project: @base_project,
- base_branch_name: @base_branch)
+ start_project: @start_project,
+ start_branch_name: @start_branch)
end
end
end
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index ca13b887e06..6ba868df04d 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -10,8 +10,8 @@ module Files
actions: params[:actions],
author_email: @author_email,
author_name: @author_name,
- base_project: @base_project,
- base_branch_name: @base_branch
+ start_project: @start_project,
+ start_branch_name: @start_branch
)
end
@@ -63,7 +63,7 @@ module Files
end
def last_commit
- Gitlab::Git::Commit.last_for_path(repository, @base_branch, @file_path)
+ Gitlab::Git::Commit.last_for_path(repository, @start_branch, @file_path)
end
def regex_check(file)
diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb
index f546b169550..a71fe61a4b6 100644
--- a/app/services/files/update_service.rb
+++ b/app/services/files/update_service.rb
@@ -9,8 +9,8 @@ module Files
previous_path: @previous_path,
author_email: @author_email,
author_name: @author_name,
- base_project: @base_project,
- base_branch_name: @base_branch)
+ start_project: @start_project,
+ start_branch_name: @start_branch)
end
private
@@ -25,7 +25,7 @@ module Files
def last_commit
@last_commit ||= Gitlab::Git::Commit.
- last_for_path(@base_project.repository, @base_branch, @file_path)
+ last_for_path(@start_project.repository, @start_branch, @file_path)
end
end
end
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index ec23407544c..2b2ba0870a4 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -46,23 +46,23 @@ class GitOperationService
end
end
- # Whenever `base_branch_name` is passed, if `branch_name` doesn't exist,
- # it would be created from `base_branch_name`.
- # If `base_project` is passed, and the branch doesn't exist,
- # it would try to find the base from it instead of current repository.
+ # Whenever `start_branch_name` is passed, if `branch_name` doesn't exist,
+ # it would be created from `start_branch_name`.
+ # If `start_project` is passed, and the branch doesn't exist,
+ # it would try to find the commits from it instead of current repository.
def with_branch(
branch_name,
- base_branch_name: nil,
- base_project: repository.project,
+ start_branch_name: nil,
+ start_project: repository.project,
&block)
check_with_branch_arguments!(
- branch_name, base_branch_name, base_project)
+ branch_name, start_branch_name, start_project)
update_branch_with_hooks(branch_name) do
repository.with_repo_branch_commit(
- base_project.repository,
- base_branch_name || branch_name,
+ start_project.repository,
+ start_branch_name || branch_name,
&block)
end
end
@@ -148,27 +148,27 @@ class GitOperationService
end
def check_with_branch_arguments!(
- branch_name, base_branch_name, base_project)
+ branch_name, start_branch_name, start_project)
return if repository.branch_exists?(branch_name)
- if repository.project != base_project
- unless base_branch_name
+ if repository.project != start_project
+ unless start_branch_name
raise ArgumentError,
- 'Should also pass :base_branch_name if' +
- ' :base_project is different from current project'
+ 'Should also pass :start_branch_name if' +
+ ' :start_project is different from current project'
end
- unless base_project.repository.branch_exists?(base_branch_name)
+ unless start_project.repository.branch_exists?(start_branch_name)
raise ArgumentError,
"Cannot find branch #{branch_name} nor" \
- " #{base_branch_name} from" \
- " #{base_project.path_with_namespace}"
+ " #{start_branch_name} from" \
+ " #{start_project.path_with_namespace}"
end
- elsif base_branch_name
- unless repository.branch_exists?(base_branch_name)
+ elsif start_branch_name
+ unless repository.branch_exists?(start_branch_name)
raise ArgumentError,
"Cannot find branch #{branch_name} nor" \
- " #{base_branch_name} from" \
+ " #{start_branch_name} from" \
" #{repository.project.path_with_namespace}"
end
end
--
GitLab
From a6394540327cd3919e5189a35a21b57800a104fc Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 6 Jan 2017 23:50:08 +0800
Subject: [PATCH 72/78] Fix renaming
---
lib/api/commits.rb | 2 +-
lib/api/files.rb | 2 +-
spec/features/projects/files/editing_a_file_spec.rb | 2 +-
spec/lib/gitlab/diff/position_tracer_spec.rb | 6 +++---
spec/models/repository_spec.rb | 2 +-
spec/services/files/update_service_spec.rb | 4 ++--
6 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index 2c1da0902c9..031759cdcdf 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -55,7 +55,7 @@ module API
authorize! :push_code, user_project
attrs = declared_params
- attrs[:source_branch] = attrs[:branch_name]
+ attrs[:start_branch] = attrs[:branch_name]
attrs[:target_branch] = attrs[:branch_name]
attrs[:actions].map! do |action|
action[:action] = action[:action].to_sym
diff --git a/lib/api/files.rb b/lib/api/files.rb
index 2e79e22e649..c58472de578 100644
--- a/lib/api/files.rb
+++ b/lib/api/files.rb
@@ -5,7 +5,7 @@ module API
def commit_params(attrs)
{
file_path: attrs[:file_path],
- source_branch: attrs[:branch_name],
+ start_branch: attrs[:branch_name],
target_branch: attrs[:branch_name],
commit_message: attrs[:commit_message],
file_content: attrs[:content],
diff --git a/spec/features/projects/files/editing_a_file_spec.rb b/spec/features/projects/files/editing_a_file_spec.rb
index fe047e00409..36a80d7575d 100644
--- a/spec/features/projects/files/editing_a_file_spec.rb
+++ b/spec/features/projects/files/editing_a_file_spec.rb
@@ -7,7 +7,7 @@ feature 'User wants to edit a file', feature: true do
let(:user) { create(:user) }
let(:commit_params) do
{
- source_branch: project.default_branch,
+ start_branch: project.default_branch,
target_branch: project.default_branch,
commit_message: "Committing First Update",
file_path: ".gitignore",
diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb
index c268f84c759..f77ab016e9b 100644
--- a/spec/lib/gitlab/diff/position_tracer_spec.rb
+++ b/spec/lib/gitlab/diff/position_tracer_spec.rb
@@ -99,7 +99,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
Files::CreateService.new(
project,
current_user,
- source_branch: branch_name,
+ start_branch: branch_name,
target_branch: branch_name,
commit_message: "Create file",
file_path: file_name,
@@ -112,7 +112,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
Files::UpdateService.new(
project,
current_user,
- source_branch: branch_name,
+ start_branch: branch_name,
target_branch: branch_name,
commit_message: "Update file",
file_path: file_name,
@@ -125,7 +125,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
Files::DeleteService.new(
project,
current_user,
- source_branch: branch_name,
+ start_branch: branch_name,
target_branch: branch_name,
commit_message: "Delete file",
file_path: file_name
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 0f43c5c019a..36564a3f9e0 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -280,7 +280,7 @@ describe Repository, models: true do
expect do
repository.commit_dir(user, 'newdir',
message: 'Create newdir', branch_name: 'patch',
- source_branch_name: 'master', source_project: forked_project)
+ start_branch_name: 'master', start_project: forked_project)
end.to change { repository.commits('master').count }.by(0)
expect(repository.branch_exists?('patch')).to be_truthy
diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb
index 6fadee9304b..35e6e139238 100644
--- a/spec/services/files/update_service_spec.rb
+++ b/spec/services/files/update_service_spec.rb
@@ -17,8 +17,8 @@ describe Files::UpdateService do
file_content: new_contents,
file_content_encoding: "text",
last_commit_sha: last_commit_sha,
- source_project: project,
- source_branch: project.default_branch,
+ start_project: project,
+ start_branch: project.default_branch,
target_branch: target_branch
}
end
--
GitLab
From acda0cd48d69dbd98ec9df8339f15139cd098726 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 26 Jan 2017 19:13:02 +0800
Subject: [PATCH 73/78] @tree_edit_project is no longer used
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_21626434
---
app/controllers/concerns/creates_commit.rb | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 2ece99aebc0..fa7c22b5388 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -101,7 +101,6 @@ module CreatesCommit
def set_commit_variables
if can?(current_user, :push_code, @project)
# Edit file in this project
- @tree_edit_project = @project
@mr_source_project = @project
if @project.forked?
@@ -114,10 +113,8 @@ module CreatesCommit
@mr_target_branch = @ref || @target_branch
end
else
- # Edit file in fork
- @tree_edit_project = current_user.fork_of(@project)
# Merge request from fork to this project
- @mr_source_project = @tree_edit_project
+ @mr_source_project = current_user.fork_of(@project)
@mr_target_project = @project
@mr_target_branch = @ref || @target_branch
end
--
GitLab
From 9bb4cd75ad51f61a53a2bef205be2b4b24acc513 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 26 Jan 2017 19:22:44 +0800
Subject: [PATCH 74/78] Use commit rather than branch, and rename to avoid
confusion
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_21626953
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_21626952
---
app/models/repository.rb | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index f3a0148c423..6c847e07c00 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -996,10 +996,10 @@ class Repository
end
end
- def check_revert_content(commit, branch_name)
- source_sha = find_branch(branch_name).dereferenced_target.sha
- args = [commit.id, source_sha]
- args << { mainline: 1 } if commit.merge_commit?
+ def check_revert_content(target_commit, branch_name)
+ source_sha = commit(branch_name).sha
+ args = [target_commit.sha, source_sha]
+ args << { mainline: 1 } if target_commit.merge_commit?
revert_index = rugged.revert_commit(*args)
return false if revert_index.conflicts?
@@ -1010,10 +1010,10 @@ class Repository
tree_id
end
- def check_cherry_pick_content(commit, branch_name)
- source_sha = find_branch(branch_name).dereferenced_target.sha
- args = [commit.id, source_sha]
- args << 1 if commit.merge_commit?
+ def check_cherry_pick_content(target_commit, branch_name)
+ source_sha = commit(branch_name).sha
+ args = [target_commit.sha, source_sha]
+ args << 1 if target_commit.merge_commit?
cherry_pick_index = rugged.cherrypick_commit(*args)
return false if cherry_pick_index.conflicts?
--
GitLab
From 05f4e48a4c761e13faf080e2d33fb8cc9886f723 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 26 Jan 2017 19:35:19 +0800
Subject: [PATCH 75/78] Make GitHooksService#execute return block value
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_21627207
---
app/services/git_hooks_service.rb | 6 +++---
app/services/git_operation_service.rb | 6 +-----
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/app/services/git_hooks_service.rb b/app/services/git_hooks_service.rb
index 6cd3908d43a..d222d1e63aa 100644
--- a/app/services/git_hooks_service.rb
+++ b/app/services/git_hooks_service.rb
@@ -18,9 +18,9 @@ class GitHooksService
end
end
- yield self
-
- run_hook('post-receive')
+ yield(self).tap do
+ run_hook('post-receive')
+ end
end
private
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index 2b2ba0870a4..df9c393844d 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -107,8 +107,6 @@ class GitOperationService
end
def with_hooks(ref, newrev, oldrev)
- result = nil
-
GitHooksService.new.execute(
user,
repository.path_to_repo,
@@ -116,10 +114,8 @@ class GitOperationService
newrev,
ref) do |service|
- result = yield(service) if block_given?
+ yield(service)
end
-
- result
end
def update_ref(ref, newrev, oldrev)
--
GitLab
From d475fa094689a6319fa60f2532898234979e30d3 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 26 Jan 2017 21:18:04 +0800
Subject: [PATCH 76/78] We don't care about the return value now
---
spec/services/git_hooks_service_spec.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/spec/services/git_hooks_service_spec.rb b/spec/services/git_hooks_service_spec.rb
index 41b0968b8b4..3318dfb22b6 100644
--- a/spec/services/git_hooks_service_spec.rb
+++ b/spec/services/git_hooks_service_spec.rb
@@ -21,7 +21,7 @@ describe GitHooksService, services: true do
hook = double(trigger: [true, nil])
expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook)
- expect(service.execute(user, @repo_path, @blankrev, @newrev, @ref) { }).to eq([true, nil])
+ service.execute(user, @repo_path, @blankrev, @newrev, @ref) { }
end
end
--
GitLab
From 8f3aa6ac338ee3595909fea9938611fb03187e6a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 26 Jan 2017 21:59:12 +0800
Subject: [PATCH 77/78] Try to check if branch diverged explicitly
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_21627134
---
app/services/git_operation_service.rb | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb
index df9c393844d..27bcc047601 100644
--- a/app/services/git_operation_service.rb
+++ b/app/services/git_operation_service.rb
@@ -82,12 +82,7 @@ class GitOperationService
end
branch = repository.find_branch(branch_name)
- oldrev = if branch
- # This could verify we're not losing commits
- repository.rugged.merge_base(newrev, branch.target)
- else
- Gitlab::Git::BLANK_SHA
- end
+ oldrev = find_oldrev_from_branch(newrev, branch)
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
update_ref_in_hooks(ref, newrev, oldrev)
@@ -100,6 +95,18 @@ class GitOperationService
newrev
end
+ def find_oldrev_from_branch(newrev, branch)
+ return Gitlab::Git::BLANK_SHA unless branch
+
+ oldrev = branch.target
+
+ if oldrev == repository.rugged.merge_base(newrev, branch.target)
+ oldrev
+ else
+ raise Repository::CommitError.new('Branch diverged')
+ end
+ end
+
def update_ref_in_hooks(ref, newrev, oldrev)
with_hooks(ref, newrev, oldrev) do
update_ref(ref, newrev, oldrev)
--
GitLab
From eb242fc865c032f6408f3b68700da9b840b416dd Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 26 Jan 2017 22:37:22 +0800
Subject: [PATCH 78/78] Make sure different project gets a merge request
Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7237#note_21626479
---
app/controllers/concerns/creates_commit.rb | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index fa7c22b5388..6286d67d30c 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -93,8 +93,10 @@ module CreatesCommit
def create_merge_request?
# XXX: Even if the field is set, if we're checking the same branch
- # as the target branch, we don't want to create a merge request.
- params[:create_merge_request].present? && @ref != @target_branch
+ # as the target branch in the same project,
+ # we don't want to create a merge request.
+ params[:create_merge_request].present? &&
+ (different_project? || @ref != @target_branch)
end
# TODO: We should really clean this up
--
GitLab