multi-file commit

add docs and tests - add additional validation
allow move without content
updated response

CHANGELOG

@@ -15,6 +15,7 @@ v 8.13.0 (unreleased)
   - Avoid database queries on Banzai::ReferenceParser::BaseParser for nodes without references
   - Simplify Mentionable concern instance methods
   - Fix permission for setting an issue's due date
+  - API: Multi-file commit !6096 (mahcsig)
   - Expose expires_at field when sharing project on API
   - Fix VueJS template tags being rendered in code comments
   - Fix issue with page scrolling to top when closing or pinning sidebar (lukehowell)

app/models/repository.rb

@@ -838,6 +838,52 @@ 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|
+      index = rugged.index
+      parents = []
+      branch = find_branch(ref)
+
+      if branch
+        last_commit = branch.target
+        index.read_tree(last_commit.raw_commit.tree)
+        parents = [last_commit.sha]
+      end
+
+      actions.each do |action|
+        case action[:action]
+        when :create, :update, :move
+          mode =
+            case action[:action]
+            when :update
+              index.get(action[:file_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: action[:file_path], oid: oid, mode: mode)
+        when :delete
+          index.remove(action[:file_path])
+        end
+      end
+
+      options = {
+        tree: index.write_tree(rugged),
+        message: message,
+        parents: parents
+      }
+      options.merge!(get_committer_and_author(user, email: author_email, name: author_name))
+
+      Rugged::Commit.create(rugged, options)
+    end
+  end
+
   def get_committer_and_author(user, email: nil, name: nil)
     committer = user_to_committer(user)
     author = Gitlab::Git::committer_hash(email: email, name: name) || committer

app/services/base_service.rb

@@ -56,9 +56,8 @@ class BaseService
     result
   end
 
-  def success
-    {
-      status: :success
-    }
+  def success(pass_back = {})
+    pass_back[:status] = :success
+    pass_back
   end
 end

app/services/files/base_service.rb

@@ -27,8 +27,9 @@ module Files
         create_target_branch
       end
 
-      if commit
-        success
+      result = commit
+      if result
+        success(result: result)
       else
         error('Something went wrong. Your changes were not committed')
       end
@@ -42,6 +43,12 @@ module Files
       @source_branch != @target_branch || @source_project != @project
     end
 
+    def file_has_changed?
+      return false unless @last_commit_sha && last_commit
+
+      @last_commit_sha != last_commit.sha
+    end
+
     def raise_error(message)
       raise ValidationError.new(message)
     end

app/services/files/multi_service.rb

+require_relative "base_service"
+
+module Files
+  class MultiService < Files::BaseService
+    class FileChangedError < StandardError; end
+
+    def commit
+      repository.multi_action(
+        user: current_user,
+        branch: @target_branch,
+        message: @commit_message,
+        actions: params[:actions],
+        author_email: @author_email,
+        author_name: @author_name
+      )
+    end
+
+    private
+
+    def validate
+      super
+
+      params[:actions].each_with_index do |action, index|
+        unless action[:file_path].present?
+          raise_error("You must specify a file_path.")
+        end
+
+        regex_check(action[:file_path])
+        regex_check(action[:previous_path]) if action[:previous_path]
+
+        if project.empty_repo? && action[:action] != :create
+          raise_error("No files to #{action[:action]}.")
+        end
+
+        validate_file_exists(action)
+
+        case action[:action]
+        when :create
+          validate_create(action)
+        when :update
+          validate_update(action)
+        when :delete
+          validate_delete(action)
+        when :move
+          validate_move(action, index)
+        else
+          raise_error("Unknown action type `#{action[:action]}`.")
+        end
+      end
+    end
+
+    def validate_file_exists(action)
+      return if action[:action] == :create
+
+      file_path = action[:file_path]
+      file_path = action[:previous_path] if action[:action] == :move
+
+      blob = repository.blob_at_branch(params[:branch_name], file_path)
+
+      unless blob
+        raise_error("File to be #{action[:action]}d `#{file_path}` does not exist.")
+      end
+    end
+
+    def last_commit
+      Gitlab::Git::Commit.last_for_path(repository, @source_branch, @file_path)
+    end
+
+    def regex_check(file)
+      if file =~ Gitlab::Regex.directory_traversal_regex
+        raise_error(
+          'Your changes could not be committed, because the file name, `' +
+          file +
+          '` ' +
+          Gitlab::Regex.directory_traversal_regex_message
+        )
+      end
+
+      unless file =~ Gitlab::Regex.file_path_regex
+        raise_error(
+          'Your changes could not be committed, because the file name, `' +
+          file +
+          '` ' +
+          Gitlab::Regex.file_path_regex_message
+        )
+      end
+    end
+
+    def validate_create(action)
+      return if project.empty_repo?
+
+      if repository.blob_at_branch(params[:branch_name], action[:file_path])
+        raise_error("Your changes could not be committed because a file with the name `#{action[:file_path]}` already exists.")
+      end
+    end
+
+    def validate_delete(action)
+    end
+
+    def validate_move(action, index)
+      if action[:previous_path].nil?
+        raise_error("You must supply the original file path when moving file `#{action[:file_path]}`.")
+      end
+
+      blob = repository.blob_at_branch(params[:branch_name], action[:file_path])
+
+      if blob
+        raise_error("Move destination `#{action[:file_path]}` already exists.")
+      end
+
+      if action[:content].nil?
+        blob = repository.blob_at_branch(params[:branch_name], action[:previous_path])
+        blob.load_all_data!(repository) if blob.truncated?
+        params[:actions][index][:content] = blob.data
+      end
+    end
+
+    def validate_update(action)
+      if file_has_changed?
+        raise FileChangedError.new("You are attempting to update a file `#{action[:file_path]}` that has changed since you started editing it.")
+      end
+    end
+  end
+end

app/services/files/update_service.rb

@@ -23,12 +23,6 @@ module Files
       end
     end
 
-    def file_has_changed?
-      return false unless @last_commit_sha && last_commit
-
-      @last_commit_sha != last_commit.sha
-    end
-
     def last_commit
       @last_commit ||= Gitlab::Git::Commit.
         last_for_path(@source_project.repository, @source_branch, @file_path)

doc/api/commits.md

@@ -46,6 +46,91 @@ Example response:
 ]
 ```
 
+## Create a commit with multiple files and actions
+
+> [Introduced][ce-6096] in GitLab 8.13.
+
+Create a commit by posting a JSON payload
+
+```
+POST /projects/:id/repository/commits
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer/string | yes | The ID of a project or NAMESPACE/PROJECT_NAME |
+| `branch_name` | string | yes | The name of a branch |
+| `commit_message` | string | yes | Commit message |
+| `actions[]` | array | yes | An array of action hashes to commit as a batch. See the next table for what attributes it can take. |
+| `author_email` | string | no | Specify the commit author's email address |
+| `author_name` | string | no | Specify the commit author's name |
+
+
+| `actions[]` Attribute | Type | Required | Description |
+| --------------------- | ---- | -------- | ----------- |
+| `action` | string | yes | The action to perform, `create`, `delete`, `move`, `update` |
+| `file_path` | string | yes | Full path to the file. Ex. `lib/class.rb` |
+| `previous_path` | string | no | Original full path to the file being moved. Ex. `lib/class1.rb` |
+| `content` | string | no | File content, required for all except `delete`. Optional for `move` |
+| `encoding` | string | no | `text` or `base64`. `text` is default. |
+
+```bash
+PAYLOAD=$(cat << 'JSON'
+{
+  "branch_name": "master",
+  "commit_message": "some commit message",
+  "actions": [
+    {
+      "action": "create",
+      "file_path": "foo/bar",
+      "content": "some content"
+    },
+    {
+      "action": "delete",
+      "file_path": "foo/bar2",
+    },
+    {
+      "action": "move",
+      "file_path": "foo/bar3",
+      "previous_path": "foo/bar4",
+      "content": "some content"
+    },
+    {
+      "action": "update",
+      "file_path": "foo/bar5",
+      "content": "new content"
+    }
+  ]
+}
+JSON
+)
+curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --header "Content-Type: application/json" --data "$PAYLOAD" https://gitlab.example.com/api/v3/projects/1/repository/commits
+```
+
+Example response:
+```json
+{
+  "id": "ed899a2f4b50b4370feeea94676502b42383c746",
+  "short_id": "ed899a2f4b5",
+  "title": "some commit message",
+  "author_name": "Dmitriy Zaporozhets",
+  "author_email": "dzaporozhets@sphereconsultinginc.com",
+  "created_at": "2016-09-20T09:26:24.000-07:00",
+  "message": "some commit message",
+  "parent_ids": [
+    "ae1d9fb46aa2b07ee9836d49862ec4e2c46fbbba"
+  ],
+  "committed_date": "2016-09-20T09:26:24.000-07:00",
+  "authored_date": "2016-09-20T09:26:24.000-07:00",
+  "stats": {
+    "additions": 2,
+    "deletions": 2,
+    "total": 4
+  },
+  "status": null
+}
+```
+
 ## Get a single commit
 
 Get a specific commit identified by the commit hash or name of a branch or tag.
@@ -343,3 +428,5 @@ Example response:
    "finished_at" : "2016-01-19T09:05:50.365Z"
 }
 ```
+
+[ce-6096]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6096 "Multi-file commit"

lib/api/commits.rb

@@ -29,6 +29,42 @@ module API
         present commits, with: Entities::RepoCommit
       end
 
+      desc 'Commit multiple file changes as one commit' do
+        detail 'This feature was introduced in GitLab 8.13'
+      end
+
+      params do
+        requires :id, type: Integer, desc: 'The project ID'
+        requires :branch_name, type: String, desc: 'The name of branch'
+        requires :commit_message, type: String, desc: 'Commit message'
+        requires :actions, type: Array, desc: 'Actions to perform in commit'
+        optional :author_email, type: String, desc: 'Author email for commit'
+        optional :author_name, type: String, desc: 'Author name for commit'
+      end
+
+      post ":id/repository/commits" do
+        authorize! :push_code, user_project
+
+        attrs = declared(params)
+        attrs[:source_branch] = attrs[:branch_name]
+        attrs[:target_branch] = attrs[:branch_name]
+        attrs[:actions].map! do |action|
+          action[:action] = action[:action].to_sym
+          action[:file_path].slice!(0) if action[:file_path] && action[:file_path].start_with?('/')
+          action[:previous_path].slice!(0) if action[:previous_path] && action[:previous_path].start_with?('/')
+          action
+        end
+
+        result = ::Files::MultiService.new(user_project, current_user, attrs).execute
+
+        if result[:status] == :success
+          commit_detail = user_project.repository.commits(result[:result], limit: 1).first
+          present commit_detail, with: Entities::RepoCommitDetail
+        else
+          render_api_error!(result[:message], 400)
+        end
+      end
+
       # Get a specific commit of a project
       #
       # Parameters:

spec/requests/api/commits_spec.rb

@@ -5,7 +5,7 @@ describe API::API, api: true  do
   include ApiHelpers
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
-  let!(:project) { create(:project, creator_id: user.id) }
+  let!(:project) { create(:project, creator_id: user.id, namespace: user.namespace) }
   let!(:master) { create(:project_member, :master, user: user, project: project) }
   let!(:guest) { create(:project_member, :guest, user: user2, project: project) }
   let!(:note) { create(:note_on_commit, author: user, project: project, commit_id: project.repository.commit.id, note: 'a comment on a commit') }
@@ -13,7 +13,7 @@ describe API::API, api: true  do
 
   before { project.team << [user, :reporter] }
 
-  describe "GET /projects/:id/repository/commits" do
+  describe "List repository commits" do
     context "authorized user" do
       before { project.team << [user2, :reporter] }
 
@@ -69,7 +69,268 @@ describe API::API, api: true  do
     end
   end
 
-  describe "GET /projects:id/repository/commits/:sha" do
+  describe "Create a commit with multiple files and actions" do
+    let!(:url) { "/projects/#{project.id}/repository/commits" }
+
+    it 'returns a 403 unauthorized for user without permissions' do
+      post api(url, user2)
+
+      expect(response).to have_http_status(403)
+    end
+
+    it 'returns a 400 bad request if no params are given' do
+      post api(url, user)
+
+      expect(response).to have_http_status(400)
+    end
+
+    context :create do
+      let(:message) { 'Created file' }
+      let!(:invalid_c_params) do
+        {
+          branch_name: 'master',
+          commit_message: message,
+          actions: [
+            {
+              action: 'create',
+              file_path: 'files/ruby/popen.rb',
+              content: 'puts 8'
+            }
+          ]
+        }
+      end
+      let!(:valid_c_params) do
+        {
+          branch_name: 'master',
+          commit_message: message,
+          actions: [
+            {
+              action: 'create',
+              file_path: 'foo/bar/baz.txt',
+              content: 'puts 8'
+            }
+          ]
+        }
+      end
+
+      it 'a new file in project repo' do
+        post api(url, user), valid_c_params
+
+        expect(response).to have_http_status(201)
+        expect(json_response['title']).to eq(message)
+      end
+
+      it 'returns a 400 bad request if file exists' do
+        post api(url, user), invalid_c_params
+
+        expect(response).to have_http_status(400)
+      end
+    end
+
+    context :delete do
+      let(:message) { 'Deleted file' }
+      let!(:invalid_d_params) do
+        {
+          branch_name: 'markdown',
+          commit_message: message,
+          actions: [
+            {
+              action: 'delete',
+              file_path: 'doc/api/projects.md'
+            }
+          ]
+        }
+      end
+      let!(:valid_d_params) do
+        {
+          branch_name: 'markdown',
+          commit_message: message,
+          actions: [
+            {
+              action: 'delete',
+              file_path: 'doc/api/users.md'
+            }
+          ]
+        }
+      end
+
+      it 'an existing file in project repo' do
+        post api(url, user), valid_d_params
+
+        expect(response).to have_http_status(201)
+        expect(json_response['title']).to eq(message)
+      end
+
+      it 'returns a 400 bad request if file does not exist' do
+        post api(url, user), invalid_d_params
+
+        expect(response).to have_http_status(400)
+      end
+    end
+
+    context :move do
+      let(:message) { 'Moved file' }
+      let!(:invalid_m_params) do
+        {
+          branch_name: 'feature',
+          commit_message: message,
+          actions: [
+            {
+              action: 'move',
+              file_path: 'CHANGELOG',
+              previous_path: 'VERSION',
+              content: '6.7.0.pre'
+            }
+          ]
+        }
+      end
+      let!(:valid_m_params) do
+        {
+          branch_name: 'feature',
+          commit_message: message,
+          actions: [
+            {
+              action: 'move',
+              file_path: 'VERSION.txt',
+              previous_path: 'VERSION',
+              content: '6.7.0.pre'
+            }
+          ]
+        }
+      end
+
+      it 'an existing file in project repo' do
+        post api(url, user), valid_m_params
+
+        expect(response).to have_http_status(201)
+        expect(json_response['title']).to eq(message)
+      end
+
+      it 'returns a 400 bad request if file does not exist' do
+        post api(url, user), invalid_m_params
+
+        expect(response).to have_http_status(400)
+      end
+    end
+
+    context :update do
+      let(:message) { 'Updated file' }
+      let!(:invalid_u_params) do
+        {
+          branch_name: 'master',
+          commit_message: message,
+          actions: [
+            {
+              action: 'update',
+              file_path: 'foo/bar.baz',
+              content: 'puts 8'
+            }
+          ]
+        }
+      end
+      let!(:valid_u_params) do
+        {
+          branch_name: 'master',
+          commit_message: message,
+          actions: [
+            {
+              action: 'update',
+              file_path: 'files/ruby/popen.rb',
+              content: 'puts 8'
+            }
+          ]
+        }
+      end
+
+      it 'an existing file in project repo' do
+        post api(url, user), valid_u_params
+
+        expect(response).to have_http_status(201)
+        expect(json_response['title']).to eq(message)
+      end
+
+      it 'returns a 400 bad request if file does not exist' do
+        post api(url, user), invalid_u_params
+
+        expect(response).to have_http_status(400)
+      end
+    end
+
+    context "multiple operations" do
+      let(:message) { 'Multiple actions' }
+      let!(:invalid_mo_params) do
+        {
+          branch_name: 'master',
+          commit_message: message,
+          actions: [
+            {
+              action: 'create',
+              file_path: 'files/ruby/popen.rb',
+              content: 'puts 8'
+            },
+            {
+              action: 'delete',
+              file_path: 'doc/api/projects.md'
+            },
+            {
+              action: 'move',
+              file_path: 'CHANGELOG',
+              previous_path: 'VERSION',
+              content: '6.7.0.pre'
+            },
+            {
+              action: 'update',
+              file_path: 'foo/bar.baz',
+              content: 'puts 8'
+            }
+          ]
+        }
+      end
+      let!(:valid_mo_params) do
+        {
+          branch_name: 'master',
+          commit_message: message,
+          actions: [
+            {
+              action: 'create',
+              file_path: 'foo/bar/baz.txt',
+              content: 'puts 8'
+            },
+            {
+              action: 'delete',
+              file_path: 'Gemfile.zip'
+            },
+            {
+              action: 'move',
+              file_path: 'VERSION.txt',
+              previous_path: 'VERSION',
+              content: '6.7.0.pre'
+            },
+            {
+              action: 'update',
+              file_path: 'files/ruby/popen.rb',
+              content: 'puts 8'
+            }
+          ]
+        }
+      end
+
+      it 'are commited as one in project repo' do
+        post api(url, user), valid_mo_params
+
+        expect(response).to have_http_status(201)
+        expect(json_response['title']).to eq(message)
+      end
+
+      it 'return a 400 bad request if there are any issues' do
+        post api(url, user), invalid_mo_params
+
+        expect(response).to have_http_status(400)
+      end
+    end
+  end
+
+  describe "Get a single commit" do
     context "authorized user" do
       it "returns a commit by sha" do
         get api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user)
@@ -122,7 +383,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe "GET /projects:id/repository/commits/:sha/diff" do
+  describe "Get the diff of a commit" do
     context "authorized user" do
       before { project.team << [user2, :reporter] }
 
@@ -149,7 +410,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe 'GET /projects:id/repository/commits/:sha/comments' do
+  describe 'Get the comments of a commit' do
     context 'authorized user' do
       it 'returns merge_request comments' do
         get api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}/comments", user)
@@ -174,7 +435,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe 'POST /projects:id/repository/commits/:sha/comments' do
+  describe 'Post comment to commit' do
     context 'authorized user' do
       it 'returns comment' do
         post api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}/comments", user), note: 'My comment'

spec/services/files/update_service_spec.rb

@@ -41,7 +41,7 @@ describe Files::UpdateService do
       it "returns a hash with the :success status " do
         results = subject.execute
 
-        expect(results).to match({ status: :success })
+        expect(results[:status]).to match(:success)
       end
 
       it "updates the file with the new contents" do
@@ -69,7 +69,7 @@ describe Files::UpdateService do
       it "returns a hash with the :success status " do
         results = subject.execute
 
-        expect(results).to match({ status: :success })
+        expect(results[:status]).to match(:success)
       end
 
       it "updates the file with the new contents" do