Cleanup parameters, easier to understand and

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?
 

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

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

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

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

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
 

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

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

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