Merge branch 'add_optional_author_for_commits' into 'master'

Add optional 'author' param when making commits

This fulfills https://gitlab.com/gitlab-org/gitlab-ce/issues/20789 by adding the ability to specify a user's ID to be used as the author of a commit.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/20789

See merge request !5822

CHANGELOG

@@ -100,6 +100,7 @@ v 8.12.0 (unreleased)
   - Fix missing flash messages on service edit page (airatshigapov)
   - Added project-specific enable/disable setting for LFS !5997
   - Added group-specific enable/disable setting for LFS !6164
+  - Add optional 'author' param when making commits. !5822 (dandunckelman)
   - Don't expose a user's token in the `/api/v3/user` API (!6047)
   - Remove redundant js-timeago-pending from user activity log (ClemMakesApps)
   - Ability to manage project issues, snippets, wiki, merge requests and builds access level

app/models/repository.rb

@@ -756,62 +756,59 @@ 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)
     update_branch_with_hooks(user, branch) do |ref|
-      committer = user_to_committer(user)
-      options = {}
-      options[:committer] = committer
-      options[:author] = committer
-
-      options[:commit] = {
-        message: message,
-        branch: ref,
-        update_ref: false,
+      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
   end
 
-  def commit_file(user, path, content, message, branch, update)
+  def commit_file(user, path, content, message, branch, update, author_email: nil, author_name: nil)
     update_branch_with_hooks(user, branch) do |ref|
-      committer = user_to_committer(user)
-      options = {}
-      options[:committer] = committer
-      options[:author] = committer
-      options[:commit] = {
-        message: message,
-        branch: ref,
-        update_ref: false,
+      options = {
+        commit: {
+          branch: ref,
+          message: message,
+          update_ref: false
+        },
+        file: {
+          content: content,
+          path: path,
+          update: update
+        }
       }
 
-      options[: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
   end
 
-  def update_file(user, path, content, branch:, previous_path:, message:)
+  def update_file(user, path, content, branch:, previous_path:, message:, author_email: nil, author_name: nil)
     update_branch_with_hooks(user, branch) do |ref|
-      committer = user_to_committer(user)
-      options = {}
-      options[:committer] = committer
-      options[:author] = committer
-      options[:commit] = {
-        message: message,
-        branch: ref,
-        update_ref: false
+      options = {
+        commit: {
+          branch: ref,
+          message: message,
+          update_ref: false
+        },
+        file: {
+          content: content,
+          path: path,
+          update: true
+        }
       }
 
-      options[: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
@@ -822,34 +819,39 @@ class Repository
     end
   end
 
-  def remove_file(user, path, message, branch)
+  def remove_file(user, path, message, branch, author_email: nil, author_name: nil)
     update_branch_with_hooks(user, branch) do |ref|
-      committer = user_to_committer(user)
-      options = {}
-      options[:committer] = committer
-      options[:author] = committer
-      options[:commit] = {
-        message: message,
-        branch: ref,
-        update_ref: false,
+      options = {
+        commit: {
+          branch: ref,
+          message: message,
+          update_ref: false
+        },
+        file: {
+          path: path
+        }
       }
 
-      options[:file] = {
-        path: path
-      }
+      options.merge!(get_committer_and_author(user, email: author_email, name: author_name))
 
       Gitlab::Git::Blob.remove(raw_repository, options)
     end
   end
 
-  def user_to_committer(user)
+  def get_committer_and_author(user, email: nil, name: nil)
+    committer = user_to_committer(user)
+    author = name && email ? Gitlab::Git::committer_hash(email: email, name: name) : committer
+
     {
-      email: user.email,
-      name: user.name,
-      time: Time.now
+      author: author,
+      committer: committer
     }
   end
 
+  def user_to_committer(user)
+    Gitlab::Git::committer_hash(email: user.email, name: user.name)
+  end
+
   def can_be_merged?(source_sha, target_branch)
     our_commit = rugged.branches[target_branch].target
     their_commit = rugged.lookup(source_sha)

app/services/files/base_service.rb

@@ -16,6 +16,8 @@ module Files
                           params[:file_content]
                         end
       @last_commit_sha = params[:last_commit_sha]
+      @author_email    = params[:author_email]
+      @author_name     = params[:author_name]
 
       # Validate parameters
       validate

app/services/files/create_dir_service.rb

@@ -3,7 +3,7 @@ require_relative "base_service"
 module Files
   class CreateDirService < Files::BaseService
     def commit
-      repository.commit_dir(current_user, @file_path, @commit_message, @target_branch)
+      repository.commit_dir(current_user, @file_path, @commit_message, @target_branch, author_email: @author_email, author_name: @author_name)
     end
 
     def validate

app/services/files/create_service.rb

@@ -3,7 +3,7 @@ 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)
+      repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch, false, author_email: @author_email, author_name: @author_name)
     end
 
     def validate

app/services/files/delete_service.rb

@@ -3,7 +3,7 @@ require_relative "base_service"
 module Files
   class DeleteService < Files::BaseService
     def commit
-      repository.remove_file(current_user, @file_path, @commit_message, @target_branch)
+      repository.remove_file(current_user, @file_path, @commit_message, @target_branch, author_email: @author_email, author_name: @author_name)
     end
   end
 end

app/services/files/update_service.rb

@@ -8,7 +8,9 @@ module Files
       repository.update_file(current_user, @file_path, @file_content,
                              branch: @target_branch,
                              previous_path: @previous_path,
-                             message: @commit_message)
+                             message: @commit_message,
+                             author_email: @author_email,
+                             author_name: @author_name)
     end
 
     private

doc/api/repository_files.md

@@ -44,7 +44,7 @@ POST /projects/:id/repository/files
 ```
 
 ```bash
-curl --request POST --header 'PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK' 'https://gitlab.example.com/api/v3/projects/13083/repository/files?file_path=app/project.rb&branch_name=master&content=some%20content&commit_message=create%20a%20new%20file'
+curl --request POST --header 'PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK' 'https://gitlab.example.com/api/v3/projects/13083/repository/files?file_path=app/project.rb&branch_name=master&author_email=author%40example.com&author_name=Firstname%20Lastname&content=some%20content&commit_message=create%20a%20new%20file'
 ```
 
 Example response:
@@ -61,6 +61,8 @@ Parameters:
 - `file_path` (required) - Full path to new file. Ex. lib/class.rb
 - `branch_name` (required) - The name of branch
 - `encoding` (optional) - 'text' or 'base64'. Text is default.
+- `author_email` (optional) - Specify the commit author's email address
+- `author_name` (optional) - Specify the commit author's name
 - `content` (required) - File content
 - `commit_message` (required) - Commit message
 
@@ -71,7 +73,7 @@ PUT /projects/:id/repository/files
 ```
 
 ```bash
-curl --request PUT --header 'PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK' 'https://gitlab.example.com/api/v3/projects/13083/repository/files?file_path=app/project.rb&branch_name=master&content=some%20other%20content&commit_message=update%20file'
+curl --request PUT --header 'PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK' 'https://gitlab.example.com/api/v3/projects/13083/repository/files?file_path=app/project.rb&branch_name=master&author_email=author%40example.com&author_name=Firstname%20Lastname&content=some%20other%20content&commit_message=update%20file'
 ```
 
 Example response:
@@ -88,6 +90,8 @@ Parameters:
 - `file_path` (required) - Full path to file. Ex. lib/class.rb
 - `branch_name` (required) - The name of branch
 - `encoding` (optional) - 'text' or 'base64'. Text is default.
+- `author_email` (optional) - Specify the commit author's email address
+- `author_name` (optional) - Specify the commit author's name
 - `content` (required) - New file content
 - `commit_message` (required) - Commit message
 
@@ -107,7 +111,7 @@ DELETE /projects/:id/repository/files
 ```
 
 ```bash
-curl --request PUT --header 'PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK' 'https://gitlab.example.com/api/v3/projects/13083/repository/files?file_path=app/project.rb&branch_name=master&commit_message=delete%20file'
+curl --request PUT --header 'PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK' 'https://gitlab.example.com/api/v3/projects/13083/repository/files?file_path=app/project.rb&branch_name=master&author_email=author%40example.com&author_name=Firstname%20Lastname&commit_message=delete%20file'
 ```
 
 Example response:
@@ -123,4 +127,6 @@ Parameters:
 
 - `file_path` (required) - Full path to file. Ex. lib/class.rb
 - `branch_name` (required) - The name of branch
+- `author_email` (optional) - Specify the commit author's email address
+- `author_name` (optional) - Specify the commit author's name
 - `commit_message` (required) - Commit message

lib/api/files.rb

@@ -11,14 +11,16 @@ module API
           target_branch: attrs[:branch_name],
           commit_message: attrs[:commit_message],
           file_content: attrs[:content],
-          file_content_encoding: attrs[:encoding]
+          file_content_encoding: attrs[:encoding],
+          author_email: attrs[:author_email],
+          author_name: attrs[:author_name]
         }
       end
 
       def commit_response(attrs)
         {
           file_path: attrs[:file_path],
-          branch_name: attrs[:branch_name],
+          branch_name: attrs[:branch_name]
         }
       end
     end
@@ -96,7 +98,7 @@ module API
         authorize! :push_code, user_project
 
         required_attributes! [:file_path, :branch_name, :content, :commit_message]
-        attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding]
+        attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding, :author_email, :author_name]
         result = ::Files::CreateService.new(user_project, current_user, commit_params(attrs)).execute
 
         if result[:status] == :success
@@ -122,7 +124,7 @@ module API
         authorize! :push_code, user_project
 
         required_attributes! [:file_path, :branch_name, :content, :commit_message]
-        attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding]
+        attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding, :author_email, :author_name]
         result = ::Files::UpdateService.new(user_project, current_user, commit_params(attrs)).execute
 
         if result[:status] == :success
@@ -149,7 +151,7 @@ module API
         authorize! :push_code, user_project
 
         required_attributes! [:file_path, :branch_name, :commit_message]
-        attrs = attributes_for_keys [:file_path, :branch_name, :commit_message]
+        attrs = attributes_for_keys [:file_path, :branch_name, :commit_message, :author_email, :author_name]
         result = ::Files::DeleteService.new(user_project, current_user, commit_params(attrs)).execute
 
         if result[:status] == :success

lib/gitlab/git.rb

@@ -18,6 +18,14 @@ module Gitlab
         end
       end
 
+      def committer_hash(email:, name:)
+        {
+          email: email,
+          name: name,
+          time: Time.now
+        }
+      end
+
       def tag_name(ref)
         ref = ref.to_s
         if self.tag_ref?(ref)

spec/models/repository_spec.rb

@@ -16,6 +16,21 @@ describe Repository, models: true do
     merge_commit_id = repository.merge(user, merge_request, commit_options)
     repository.commit(merge_commit_id)
   end
+  let(:author_email) { FFaker::Internet.email }
+
+  # I have to remove periods from the end of the name
+  # This happened when the user's name had a suffix (i.e. "Sr.")
+  # This seems to be what git does under the hood. For example, this commit:
+  #
+  # $ git commit --author='Foo Sr. <foo@example.com>' -m 'Where's my trailing period?'
+  #
+  # results in this:
+  #
+  # $ git show --pretty
+  # ...
+  # Author: Foo Sr <foo@example.com>
+  # ...
+  let(:author_name) { FFaker::Name.name.chomp("\.") }
 
   describe '#branch_names_contains' do
     subject { repository.branch_names_contains(sample_commit.id) }
@@ -132,7 +147,31 @@ describe Repository, models: true do
     end
   end
 
-  describe :commit_file 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')
+      end.to change { repository.commits('master').count }.by(1)
+
+      newdir = repository.tree('master', 'newdir')
+      expect(newdir.path).to eq('newdir')
+    end
+
+    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)
+        end.to change { repository.commits('master').count }.by(1)
+
+        last_commit = repository.commit
+
+        expect(last_commit.author_email).to eq(author_email)
+        expect(last_commit.author_name).to eq(author_name)
+      end
+    end
+  end
+
+  describe "#commit_file" do
     it 'commits change to a file successfully' do
       expect do
         repository.commit_file(user, 'CHANGELOG', 'Changelog!',
@@ -144,9 +183,23 @@ describe Repository, models: true do
 
       expect(blob.data).to eq('Changelog!')
     end
+
+    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)
+        end.to change { repository.commits('master').count }.by(1)
+
+        last_commit = repository.commit
+
+        expect(last_commit.author_email).to eq(author_email)
+        expect(last_commit.author_name).to eq(author_name)
+      end
+    end
   end
 
-  describe :update_file do
+  describe "#update_file" do
     it 'updates filename successfully' do
       expect do
         repository.update_file(user, 'NEWLICENSE', 'Copyright!',
@@ -160,6 +213,85 @@ describe Repository, models: true do
       expect(files).not_to include('LICENSE')
       expect(files).to include('NEWLICENSE')
     end
+
+    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)
+
+        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)
+        end.to change { repository.commits('master').count }.by(1)
+
+        last_commit = repository.commit
+
+        expect(last_commit.author_email).to eq(author_email)
+        expect(last_commit.author_name).to eq(author_name)
+      end
+    end
+  end
+
+  describe "#remove_file" do
+    it 'removes file successfully' do
+      repository.commit_file(user, "README", 'README!', 'Add README', 'master', true)
+
+      expect do
+        repository.remove_file(user, "README", "Remove README", 'master')
+      end.to change { repository.commits('master').count }.by(1)
+
+      expect(repository.blob_at('master', 'README')).to be_nil
+    end
+
+    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)
+
+        expect do
+          repository.remove_file(user, "README", "Remove README", 'master', author_email: author_email, author_name: author_name)
+        end.to change { repository.commits('master').count }.by(1)
+
+        last_commit = repository.commit
+
+        expect(last_commit.author_email).to eq(author_email)
+        expect(last_commit.author_name).to eq(author_name)
+      end
+    end
+  end
+
+  describe '#get_committer_and_author' do
+    it 'returns the committer and author data' do
+      options = repository.get_committer_and_author(user)
+      expect(options[:committer][:email]).to eq(user.email)
+      expect(options[:author][:email]).to eq(user.email)
+    end
+
+    context 'when the email/name are given' do
+      it 'returns an object containing the email/name' do
+        options = repository.get_committer_and_author(user, email: author_email, name: author_name)
+        expect(options[:author][:email]).to eq(author_email)
+        expect(options[:author][:name]).to eq(author_name)
+      end
+    end
+
+    context 'when the email is given but the name is not' do
+      it 'returns the committer as the author' do
+        options = repository.get_committer_and_author(user, email: author_email)
+        expect(options[:author][:email]).to eq(user.email)
+        expect(options[:author][:name]).to eq(user.name)
+      end
+    end
+
+    context 'when the name is given but the email is not' do
+      it 'returns nil' do
+        options = repository.get_committer_and_author(user, name: author_name)
+        expect(options[:author][:email]).to eq(user.email)
+        expect(options[:author][:name]).to eq(user.name)
+      end
+    end
   end
 
   describe "search_files" do

spec/requests/api/files_spec.rb

@@ -5,6 +5,21 @@ describe API::API, api: true  do
   let(:user) { create(:user) }
   let!(:project) { create(:project, namespace: user.namespace ) }
   let(:file_path) { 'files/ruby/popen.rb' }
+  let(:author_email) { FFaker::Internet.email }
+
+  # I have to remove periods from the end of the name
+  # This happened when the user's name had a suffix (i.e. "Sr.")
+  # This seems to be what git does under the hood. For example, this commit:
+  #
+  # $ git commit --author='Foo Sr. <foo@example.com>' -m 'Where's my trailing period?'
+  #
+  # results in this:
+  #
+  # $ git show --pretty
+  # ...
+  # Author: Foo Sr <foo@example.com>
+  # ...
+  let(:author_name) { FFaker::Name.name.chomp("\.") }
 
   before { project.team << [user, :developer] }
 
@@ -16,6 +31,7 @@ describe API::API, api: true  do
       }
 
       get api("/projects/#{project.id}/repository/files", user), params
+
       expect(response).to have_http_status(200)
       expect(json_response['file_path']).to eq(file_path)
       expect(json_response['file_name']).to eq('popen.rb')
@@ -25,6 +41,7 @@ describe API::API, api: true  do
 
     it "returns a 400 bad request if no params given" do
       get api("/projects/#{project.id}/repository/files", user)
+
       expect(response).to have_http_status(400)
     end
 
@@ -35,6 +52,7 @@ describe API::API, api: true  do
       }
 
       get api("/projects/#{project.id}/repository/files", user), params
+
       expect(response).to have_http_status(404)
     end
   end
@@ -51,12 +69,17 @@ describe API::API, api: true  do
 
     it "creates a new file in project repo" do
       post api("/projects/#{project.id}/repository/files", user), valid_params
+
       expect(response).to have_http_status(201)
       expect(json_response['file_path']).to eq('newfile.rb')
+      last_commit = project.repository.commit.raw
+      expect(last_commit.author_email).to eq(user.email)
+      expect(last_commit.author_name).to eq(user.name)
     end
 
     it "returns a 400 bad request if no params given" do
       post api("/projects/#{project.id}/repository/files", user)
+
       expect(response).to have_http_status(400)
     end
 
@@ -65,8 +88,22 @@ describe API::API, api: true  do
         and_return(false)
 
       post api("/projects/#{project.id}/repository/files", user), valid_params
+
       expect(response).to have_http_status(400)
     end
+
+    context "when specifying an author" do
+      it "creates a new file with the specified author" do
+        valid_params.merge!(author_email: author_email, author_name: author_name)
+
+        post api("/projects/#{project.id}/repository/files", user), valid_params
+
+        expect(response).to have_http_status(201)
+        last_commit = project.repository.commit.raw
+        expect(last_commit.author_email).to eq(author_email)
+        expect(last_commit.author_name).to eq(author_name)
+      end
+    end
   end
 
   describe "PUT /projects/:id/repository/files" do
@@ -81,14 +118,32 @@ describe API::API, api: true  do
 
     it "updates existing file in project repo" do
       put api("/projects/#{project.id}/repository/files", user), valid_params
+
       expect(response).to have_http_status(200)
       expect(json_response['file_path']).to eq(file_path)
+      last_commit = project.repository.commit.raw
+      expect(last_commit.author_email).to eq(user.email)
+      expect(last_commit.author_name).to eq(user.name)
     end
 
     it "returns a 400 bad request if no params given" do
       put api("/projects/#{project.id}/repository/files", user)
+
       expect(response).to have_http_status(400)
     end
+
+    context "when specifying an author" do
+      it "updates a file with the specified author" do
+        valid_params.merge!(author_email: author_email, author_name: author_name, content: "New content")
+
+        put api("/projects/#{project.id}/repository/files", user), valid_params
+
+        expect(response).to have_http_status(200)
+        last_commit = project.repository.commit.raw
+        expect(last_commit.author_email).to eq(author_email)
+        expect(last_commit.author_name).to eq(author_name)
+      end
+    end
   end
 
   describe "DELETE /projects/:id/repository/files" do
@@ -102,12 +157,17 @@ describe API::API, api: true  do
 
     it "deletes existing file in project repo" do
       delete api("/projects/#{project.id}/repository/files", user), valid_params
+
       expect(response).to have_http_status(200)
       expect(json_response['file_path']).to eq(file_path)
+      last_commit = project.repository.commit.raw
+      expect(last_commit.author_email).to eq(user.email)
+      expect(last_commit.author_name).to eq(user.name)
     end
 
     it "returns a 400 bad request if no params given" do
       delete api("/projects/#{project.id}/repository/files", user)
+
       expect(response).to have_http_status(400)
     end
 
@@ -115,8 +175,22 @@ describe API::API, api: true  do
       allow_any_instance_of(Repository).to receive(:remove_file).and_return(false)
 
       delete api("/projects/#{project.id}/repository/files", user), valid_params
+
       expect(response).to have_http_status(400)
     end
+
+    context "when specifying an author" do
+      it "removes a file with the specified author" do
+        valid_params.merge!(author_email: author_email, author_name: author_name)
+
+        delete api("/projects/#{project.id}/repository/files", user), valid_params
+
+        expect(response).to have_http_status(200)
+        last_commit = project.repository.commit.raw
+        expect(last_commit.author_email).to eq(author_email)
+        expect(last_commit.author_name).to eq(author_name)
+      end
+    end
   end
 
   describe "POST /projects/:id/repository/files with binary file" do
@@ -143,6 +217,7 @@ describe API::API, api: true  do
 
     it "remains unchanged" do
       get api("/projects/#{project.id}/repository/files", user), get_params
+
       expect(response).to have_http_status(200)
       expect(json_response['file_path']).to eq(file_path)
       expect(json_response['file_name']).to eq(file_path)