diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2016-12-08 12:08:25 +0300 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2016-12-08 12:08:25 +0300 |
commit | 3fa3fcd7876262bb63966debd04d16ea219fad73 (patch) | |
tree | 8ccb4823ba9d9a47e8ee7d6514c274c223288f80 | |
parent | 691f1c496834078ba41209597558259d20790a0b (diff) |
Cleanup parameters, easier to understand and
more consistent across different methodst
-rw-r--r-- | app/models/repository.rb | 91 | ||||
-rw-r--r-- | app/services/commits/change_service.rb | 2 | ||||
-rw-r--r-- | app/services/files/create_dir_service.rb | 6 | ||||
-rw-r--r-- | app/services/files/create_service.rb | 8 | ||||
-rw-r--r-- | app/services/files/delete_service.rb | 6 | ||||
-rw-r--r-- | app/services/files/multi_service.rb | 4 | ||||
-rw-r--r-- | app/services/files/update_service.rb | 6 | ||||
-rw-r--r-- | app/services/git_operation_service.rb | 16 | ||||
-rw-r--r-- | 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 |