From 3c5c658a27d1dfe4abf6469f35776b78f2169d81 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 17 Sep 2018 14:14:58 +0100
Subject: Use the correct email address when committing via a file service

---
 app/services/files/base_service.rb         |  6 ++++--
 spec/factories/users.rb                    |  8 ++++++++
 spec/services/files/create_service_spec.rb | 15 ++++++++++++++-
 spec/services/files/delete_service_spec.rb |  8 ++++++++
 spec/services/files/update_service_spec.rb |  8 ++++++++
 5 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb
index fc7b236f7da..39e614d6569 100644
--- a/app/services/files/base_service.rb
+++ b/app/services/files/base_service.rb
@@ -7,8 +7,10 @@ module Files
     def initialize(*args)
       super
 
-      @author_email = params[:author_email] || current_user&.email
-      @author_name = params[:author_name] || current_user&.name
+      git_user = Gitlab::Git::User.from_gitlab(current_user) if current_user.present?
+
+      @author_email = params[:author_email] || git_user&.email
+      @author_name = params[:author_name] || git_user&.name
       @commit_message = params[:commit_message]
       @last_commit_sha = params[:last_commit_sha]
 
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index 59db8cdc34b..a47bd7cafca 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -58,6 +58,14 @@ FactoryBot.define do
       project_view :readme
     end
 
+    trait :commit_email do
+      after(:create) do |user, evaluator|
+        additional = create(:email, :confirmed, user: user, email: "commit-#{user.email}")
+
+        user.update!(commit_email: additional.email)
+      end
+    end
+
     factory :omniauth_user do
       transient do
         extern_uid '123456'
diff --git a/spec/services/files/create_service_spec.rb b/spec/services/files/create_service_spec.rb
index 30d94e4318d..c8f899e6dff 100644
--- a/spec/services/files/create_service_spec.rb
+++ b/spec/services/files/create_service_spec.rb
@@ -3,7 +3,7 @@ require "spec_helper"
 describe Files::CreateService do
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
-  let(:user) { create(:user) }
+  let(:user) { create(:user, :commit_email) }
   let(:file_content) { 'Test file content' }
   let(:branch_name) { project.default_branch }
   let(:start_branch) { branch_name }
@@ -20,6 +20,8 @@ describe Files::CreateService do
     }
   end
 
+  let(:commit) { repository.head_commit }
+
   subject { described_class.new(project, user, commit_params) }
 
   before do
@@ -75,4 +77,15 @@ describe Files::CreateService do
       end
     end
   end
+
+  context 'commit attribute' do
+    let(:file_path) { 'test-commit-attributes.txt' }
+
+    it 'uses the commit email' do
+      subject.execute
+
+      expect(commit.author_email).to eq(user.commit_email)
+      expect(commit.committer_email).to eq(user.commit_email)
+    end
+  end
 end
diff --git a/spec/services/files/delete_service_spec.rb b/spec/services/files/delete_service_spec.rb
index 73566afe8c8..ba4a0594666 100644
--- a/spec/services/files/delete_service_spec.rb
+++ b/spec/services/files/delete_service_spec.rb
@@ -8,6 +8,7 @@ describe Files::DeleteService do
   let(:file_path) { 'files/ruby/popen.rb' }
   let(:branch_name) { project.default_branch }
   let(:last_commit_sha) { nil }
+  let(:commit) { project.repository.head_commit }
 
   let(:commit_params) do
     {
@@ -34,6 +35,13 @@ describe Files::DeleteService do
 
       expect(blob).to be_nil
     end
+
+    it 'uses the commit email' do
+      subject.execute
+
+      expect(commit.author_email).to eq(user.commit_email)
+      expect(commit.committer_email).to eq(user.commit_email)
+    end
   end
 
   before do
diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb
index e01fe487ffa..3bfa2717960 100644
--- a/spec/services/files/update_service_spec.rb
+++ b/spec/services/files/update_service_spec.rb
@@ -9,6 +9,7 @@ describe Files::UpdateService do
   let(:new_contents) { 'New Content' }
   let(:branch_name) { project.default_branch }
   let(:last_commit_sha) { nil }
+  let(:commit) { project.repository.commit }
 
   let(:commit_params) do
     {
@@ -54,6 +55,13 @@ describe Files::UpdateService do
 
         expect(results.data).to eq(new_contents)
       end
+
+      it 'uses the commit email' do
+        subject.execute
+
+        expect(commit.author_email).to eq(user.commit_email)
+        expect(commit.committer_email).to eq(user.commit_email)
+      end
     end
 
     context "when the last_commit_sha is not supplied" do
-- 
cgit v1.2.1


From e35fc8b9299e7bd257e00601f206e2670c559651 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 17 Sep 2018 14:36:22 +0100
Subject: Use commit email for wiki actions

---
 app/models/project_wiki.rb       |  2 +-
 spec/models/project_wiki_spec.rb | 26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb
index f4b3421f04b..d3c6312baf2 100644
--- a/app/models/project_wiki.rb
+++ b/app/models/project_wiki.rb
@@ -188,7 +188,7 @@ class ProjectWiki
     Gitlab::Git::Wiki::CommitDetails.new(@user.id,
                                          @user.username,
                                          @user.name,
-                                         @user.email,
+                                         @user.commit_email,
                                          commit_message)
   end
 
diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb
index 528f5b610d7..f607bb7a917 100644
--- a/spec/models/project_wiki_spec.rb
+++ b/spec/models/project_wiki_spec.rb
@@ -2,12 +2,13 @@
 require "spec_helper"
 
 describe ProjectWiki do
-  let(:project) { create(:project, :wiki_repo) }
+  let(:user) { create(:user, :commit_email) }
+  let(:project) { create(:project, :wiki_repo, namespace: user.namespace) }
   let(:repository) { project.repository }
-  let(:user) { project.owner }
   let(:gitlab_shell) { Gitlab::Shell.new }
   let(:project_wiki) { described_class.new(project, user) }
   let(:raw_repository) { Gitlab::Git::Repository.new(project.repository_storage, subject.disk_path + '.git', 'foo') }
+  let(:commit) { project_wiki.repository.head_commit }
 
   subject { project_wiki }
 
@@ -276,6 +277,13 @@ describe ProjectWiki do
         expect(subject.pages.first.page.version.message).to eq("commit message")
       end
 
+      it 'sets the correct commit email' do
+        subject.create_page('test page', 'content')
+
+        expect(commit.author_email).to eq(user.commit_email)
+        expect(commit.committer_email).to eq(user.commit_email)
+      end
+
       it 'updates project activity' do
         subject.create_page('Test Page', 'This is content')
 
@@ -320,6 +328,11 @@ describe ProjectWiki do
       expect(@page.version.message).to eq("updated page")
     end
 
+    it 'sets the correct commit email' do
+      expect(commit.author_email).to eq(user.commit_email)
+      expect(commit.committer_email).to eq(user.commit_email)
+    end
+
     it 'updates project activity' do
       subject.update_page(
         @gitlab_git_wiki_page,
@@ -347,6 +360,13 @@ describe ProjectWiki do
         expect(subject.pages.count).to eq(0)
       end
 
+      it 'sets the correct commit email' do
+        subject.delete_page(@page)
+
+        expect(commit.author_email).to eq(user.commit_email)
+        expect(commit.committer_email).to eq(user.commit_email)
+      end
+
       it 'updates project activity' do
         subject.delete_page(@page)
 
@@ -420,7 +440,7 @@ describe ProjectWiki do
   end
 
   def commit_details
-    Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.email, "test commit")
+    Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.commit_email, "test commit")
   end
 
   def create_page(name, content)
-- 
cgit v1.2.1


From f54cada7b7e574f0dcd4997ee4d9256f6e5d8529 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 17 Sep 2018 14:44:24 +0100
Subject: Remove Gitlab::Git::Repository#user_to_committer

---
 lib/gitlab/git/repository.rb | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 1b8d320ff3b..61786f1d896 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -581,10 +581,6 @@ module Gitlab
         end
       end
 
-      def user_to_committer(user)
-        Gitlab::Git.committer_hash(email: user.email, name: user.name)
-      end
-
       # Delete the specified branch from the repository
       def delete_branch(branch_name)
         wrapped_gitaly_errors do
-- 
cgit v1.2.1


From 12f9879295ac9abdd0d6aff47cc3828c9ca36dc7 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 17 Sep 2018 15:38:10 +0100
Subject: Add changelog

---
 changelogs/unreleased/51564-fix-commit-email-usage.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/51564-fix-commit-email-usage.yml

diff --git a/changelogs/unreleased/51564-fix-commit-email-usage.yml b/changelogs/unreleased/51564-fix-commit-email-usage.yml
new file mode 100644
index 00000000000..2f1b042ae8a
--- /dev/null
+++ b/changelogs/unreleased/51564-fix-commit-email-usage.yml
@@ -0,0 +1,5 @@
+---
+title: Respect the user commit email in more places
+merge_request: 21773
+author:
+type: fixed
-- 
cgit v1.2.1


From 5a883915793f0f89f850d5393dce497ff9c5f893 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 18 Sep 2018 10:04:15 +0100
Subject: Guard against regressions in commit email specs

---
 app/models/project_wiki.rb                 | 7 ++++---
 spec/models/project_wiki_spec.rb           | 3 +++
 spec/services/files/create_service_spec.rb | 1 +
 spec/services/files/delete_service_spec.rb | 3 ++-
 spec/services/files/update_service_spec.rb | 3 ++-
 5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb
index d3c6312baf2..761359b3c9f 100644
--- a/app/models/project_wiki.rb
+++ b/app/models/project_wiki.rb
@@ -184,11 +184,12 @@ class ProjectWiki
 
   def commit_details(action, message = nil, title = nil)
     commit_message = message || default_message(action, title)
+    git_user = Gitlab::Git::User.from_gitlab(@user)
 
     Gitlab::Git::Wiki::CommitDetails.new(@user.id,
-                                         @user.username,
-                                         @user.name,
-                                         @user.commit_email,
+                                         git_user.username,
+                                         git_user.name,
+                                         git_user.email,
                                          commit_message)
   end
 
diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb
index f607bb7a917..f38fc191943 100644
--- a/spec/models/project_wiki_spec.rb
+++ b/spec/models/project_wiki_spec.rb
@@ -280,6 +280,7 @@ describe ProjectWiki do
       it 'sets the correct commit email' do
         subject.create_page('test page', 'content')
 
+        expect(user.commit_email).not_to eq(user.email)
         expect(commit.author_email).to eq(user.commit_email)
         expect(commit.committer_email).to eq(user.commit_email)
       end
@@ -329,6 +330,7 @@ describe ProjectWiki do
     end
 
     it 'sets the correct commit email' do
+      expect(user.commit_email).not_to eq(user.email)
       expect(commit.author_email).to eq(user.commit_email)
       expect(commit.committer_email).to eq(user.commit_email)
     end
@@ -363,6 +365,7 @@ describe ProjectWiki do
       it 'sets the correct commit email' do
         subject.delete_page(@page)
 
+        expect(user.commit_email).not_to eq(user.email)
         expect(commit.author_email).to eq(user.commit_email)
         expect(commit.committer_email).to eq(user.commit_email)
       end
diff --git a/spec/services/files/create_service_spec.rb b/spec/services/files/create_service_spec.rb
index c8f899e6dff..751b7160276 100644
--- a/spec/services/files/create_service_spec.rb
+++ b/spec/services/files/create_service_spec.rb
@@ -84,6 +84,7 @@ describe Files::CreateService do
     it 'uses the commit email' do
       subject.execute
 
+      expect(user.commit_email).not_to eq(user.email)
       expect(commit.author_email).to eq(user.commit_email)
       expect(commit.committer_email).to eq(user.commit_email)
     end
diff --git a/spec/services/files/delete_service_spec.rb b/spec/services/files/delete_service_spec.rb
index ba4a0594666..309802ce733 100644
--- a/spec/services/files/delete_service_spec.rb
+++ b/spec/services/files/delete_service_spec.rb
@@ -4,7 +4,7 @@ describe Files::DeleteService do
   subject { described_class.new(project, user, commit_params) }
 
   let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
+  let(:user) { create(:user, :commit_email) }
   let(:file_path) { 'files/ruby/popen.rb' }
   let(:branch_name) { project.default_branch }
   let(:last_commit_sha) { nil }
@@ -39,6 +39,7 @@ describe Files::DeleteService do
     it 'uses the commit email' do
       subject.execute
 
+      expect(user.commit_email).not_to eq(user.email)
       expect(commit.author_email).to eq(user.commit_email)
       expect(commit.committer_email).to eq(user.commit_email)
     end
diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb
index 3bfa2717960..23db35c2418 100644
--- a/spec/services/files/update_service_spec.rb
+++ b/spec/services/files/update_service_spec.rb
@@ -4,7 +4,7 @@ describe Files::UpdateService do
   subject { described_class.new(project, user, commit_params) }
 
   let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
+  let(:user) { create(:user, :commit_email) }
   let(:file_path) { 'files/ruby/popen.rb' }
   let(:new_contents) { 'New Content' }
   let(:branch_name) { project.default_branch }
@@ -59,6 +59,7 @@ describe Files::UpdateService do
       it 'uses the commit email' do
         subject.execute
 
+        expect(user.commit_email).not_to eq(user.email)
         expect(commit.author_email).to eq(user.commit_email)
         expect(commit.committer_email).to eq(user.commit_email)
       end
-- 
cgit v1.2.1