From 38075d486a79fc6a2915115a0761bf731b62cf53 Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Tue, 14 Aug 2018 11:55:37 -0400
Subject: [PATCH 01/15] Add commit_email column to users table

---
 ...0180814153625_add_commit_email_to_users.rb | 33 +++++++++++++++++++
 db/schema.rb                                  |  3 +-
 2 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 db/migrate/20180814153625_add_commit_email_to_users.rb

diff --git a/db/migrate/20180814153625_add_commit_email_to_users.rb b/db/migrate/20180814153625_add_commit_email_to_users.rb
new file mode 100644
index 00000000000..5c87d73688e
--- /dev/null
+++ b/db/migrate/20180814153625_add_commit_email_to_users.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddCommitEmailToUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
+  # "add_column_with_default" you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    add_column :users, :commit_email, :string
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index f1d8f4df3b7..a2b85edca67 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180807153545) do
+ActiveRecord::Schema.define(version: 20180814153625) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -2174,6 +2174,7 @@
     t.integer "accepted_term_id"
     t.string "feed_token"
     t.boolean "private_profile"
+    t.string "commit_email"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
-- 
GitLab


From a5b29ad8fd3106b3d7c66ad555cb6b39a4fdbcec Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Tue, 14 Aug 2018 12:18:36 -0400
Subject: [PATCH 02/15] Show badge next to commit email in list

---
 app/views/profiles/emails/index.html.haml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index 04a19ab14dd..3144d3703e7 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -22,7 +22,9 @@
     .account-well.append-bottom-default
       %ul
         %li
-          Your Primary Email will be used for avatar detection and web based operations, such as edits and merges.
+          Your Primary Email will be used for avatar detection.
+        %li
+          Your Commit Email will be used for web based operations, such as edits and merges.
         %li
           Your Notification Email will be used for account notifications.
         %li
@@ -34,6 +36,8 @@
         = render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? }
         %span.float-right
           %span.badge.badge-success Primary email
+          - if !current_user.commit_email? || @primary_email === current_user.commit_email
+            %span.badge.badge-info Commit email
           - if @primary_email === current_user.public_email
             %span.badge.badge-info Public email
           - if @primary_email === current_user.notification_email
@@ -42,6 +46,8 @@
         %li
           = render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? }
           %span.float-right
+            - if email.email === current_user.commit_email
+              %span.badge.badge-info Commit email
             - if email.email === current_user.public_email
               %span.badge.badge-info Public email
             - if email.email === current_user.notification_email
-- 
GitLab


From 91f44c3028a18afdb01bc9c5cc806378e3a9acbe Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Tue, 14 Aug 2018 12:38:45 -0400
Subject: [PATCH 03/15] Let user choose commit email

---
 app/controllers/profiles_controller.rb |  1 +
 app/models/user.rb                     | 19 ++++++++++++++++++-
 app/views/profiles/show.html.haml      |  3 +++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index 6f50cbb4a36..03e67f746d7 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -94,6 +94,7 @@ def user_params
       :location,
       :name,
       :public_email,
+      :commit_email,
       :skype,
       :twitter,
       :username,
diff --git a/app/models/user.rb b/app/models/user.rb
index fb19de4b980..fedc6a47c72 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -158,6 +158,7 @@ def update_tracked_fields!(request)
   validates :notification_email, presence: true
   validates :notification_email, email: true, if: ->(user) { user.notification_email != user.email }
   validates :public_email, presence: true, uniqueness: true, email: true, allow_blank: true
+  validates :commit_email, email: true, allow_nil: true, if: ->(user) { user.commit_email != user.email }
   validates :bio, length: { maximum: 255 }, allow_blank: true
   validates :projects_limit,
     presence: true,
@@ -170,12 +171,15 @@ def update_tracked_fields!(request)
   validate :unique_email, if: :email_changed?
   validate :owns_notification_email, if: :notification_email_changed?
   validate :owns_public_email, if: :public_email_changed?
+  validate :owns_commit_email, if: :commit_email_changed?
   validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
 
   before_validation :sanitize_attrs
   before_validation :set_notification_email, if: :new_record?
   before_validation :set_public_email, if: :public_email_changed?
+  before_validation :set_commit_email, if: :commit_email_changed?
   before_save :set_public_email, if: :public_email_changed? # in case validation is skipped
+  before_save :set_commit_email, if: :commit_email_changed? # in case validation is skipped
   before_save :ensure_incoming_email_token
   before_save :ensure_user_rights_and_limits, if: ->(user) { user.new_record? || user.external_changed? }
   before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
@@ -560,6 +564,12 @@ def owns_public_email
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   end
 
+  def owns_commit_email
+    return if commit_email.blank?
+
+    errors.add(:commit_email, "is not an email you own") unless all_emails.include?(commit_email)
+  end
+
   # see if the new email is already a verified secondary email
   def check_for_verified_email
     skip_reconfirmation! if emails.confirmed.where(email: self.email).any?
@@ -806,10 +816,17 @@ def set_public_email
     end
   end
 
+  def set_commit_email
+    if commit_email.blank? || all_emails.exclude?(commit_email)
+        self.commit_email = nil
+    end
+  end
+
   def update_secondary_emails!
     set_notification_email
     set_public_email
-    save if notification_email_changed? || public_email_changed?
+    set_commit_email
+    save if notification_email_changed? || public_email_changed? || commit_email_changed?
   end
 
   def set_projects_limit
diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml
index 6f08a294c5d..c1d6daee47e 100644
--- a/app/views/profiles/show.html.haml
+++ b/app/views/profiles/show.html.haml
@@ -88,6 +88,9 @@
       = f.select :public_email, options_for_select(@user.all_emails, selected: @user.public_email),
         { help: 'This email will be displayed on your public profile.', include_blank: 'Do not show on profile' },
         control_class: 'select2'
+      = f.select :commit_email, options_for_select(@user.all_emails, selected: @user.commit_email),
+        { help: 'This email will be used for web based operations, such as edits and merges.', include_blank: 'Use my primary email' },
+        control_class: 'select2'
       = f.select :preferred_language, Gitlab::I18n::AVAILABLE_LANGUAGES.map { |value, label| [label, value] },
         { help: 'This feature is experimental and translations are not complete yet.' },
         control_class: 'select2'
-- 
GitLab


From 3fe4754a5921604b74f96dfebdf6ed8e72802031 Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Tue, 14 Aug 2018 12:44:33 -0400
Subject: [PATCH 04/15] Prefer commit_email over email when making commits

---
 lib/gitlab/git/user.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/git/user.rb b/lib/gitlab/git/user.rb
index e573cd0e143..894cc284a4a 100644
--- a/lib/gitlab/git/user.rb
+++ b/lib/gitlab/git/user.rb
@@ -4,7 +4,7 @@ class User
       attr_reader :username, :name, :email, :gl_id
 
       def self.from_gitlab(gitlab_user)
-        new(gitlab_user.username, gitlab_user.name, gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user))
+        new(gitlab_user.username, gitlab_user.name, gitlab_user.commit_email || gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user))
       end
 
       def self.from_gitaly(gitaly_user)
-- 
GitLab


From d5420214f4d561cc6fb0b844d12c9adb1ca214dd Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Tue, 14 Aug 2018 14:26:51 -0400
Subject: [PATCH 05/15] Update tests for new commit_email field

---
 spec/lib/gitlab/git/user_spec.rb | 15 ++++++++++++---
 spec/models/user_spec.rb         |  2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/spec/lib/gitlab/git/user_spec.rb b/spec/lib/gitlab/git/user_spec.rb
index 99d850e1df9..61918eba4d8 100644
--- a/spec/lib/gitlab/git/user_spec.rb
+++ b/spec/lib/gitlab/git/user_spec.rb
@@ -22,10 +22,19 @@
   end
 
   describe '.from_gitlab' do
-    let(:user) { build(:user) }
-    subject { described_class.from_gitlab(user) }
+    context 'when no commit_email has been set' do
+      let(:user) { build(:user, email: 'alice@example.com', commit_email: nil) }
+      subject { described_class.from_gitlab(user) }
 
-    it { expect(subject).to eq(described_class.new(user.username, user.name, user.email, 'user-')) }
+      it { expect(subject).to eq(described_class.new(user.username, user.name, user.email, 'user-')) }
+    end
+
+    context 'when commit_email has been set' do
+      let(:user) { user = build(:user, email: 'alice@example.com', commit_email: 'bob@example.com') }
+      subject { described_class.from_gitlab(user) }
+
+      it { expect(subject).to eq(described_class.new(user.username, user.name, user.commit_email, 'user-')) }
+    end
   end
 
   describe '#==' do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index f5e2c977104..ecaad7b930f 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -163,7 +163,7 @@
       subject { build(:user) }
     end
 
-    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email do
+    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email, :commit_email do
       subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
     end
 
-- 
GitLab


From 48eb5492a47ed43a71ad0930a796e62aa022c879 Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Tue, 28 Aug 2018 14:23:28 -0400
Subject: [PATCH 06/15] Guard against commit_email column missing

---
 app/models/user.rb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/app/models/user.rb b/app/models/user.rb
index fedc6a47c72..1992178fdf4 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -570,6 +570,22 @@ def owns_commit_email
     errors.add(:commit_email, "is not an email you own") unless all_emails.include?(commit_email)
   end
 
+  # Define commit_email-related attribute methods explicitly instead of relying on ActiveRecord to
+  # provide them. Some of the specs use the current state of the model code but an older database
+  # schema, so we need to guard against the possibility of the commit_email column not existing.
+
+  def commit_email
+    self[:commit_email] if has_attribute?(:commit_email)
+  end
+
+  def commit_email=(email)
+    self[:commit_email] = email if has_attribute?(:commit_email)
+  end
+
+  def commit_email_changed?
+    has_attribute?(:commit_email) && attribute_changed?(:commit_email)
+  end
+
   # see if the new email is already a verified secondary email
   def check_for_verified_email
     skip_reconfirmation! if emails.confirmed.where(email: self.email).any?
-- 
GitLab


From 7cf0c97537d2bf3cb1541cd96e868064e1bc1e02 Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Tue, 28 Aug 2018 16:31:52 -0400
Subject: [PATCH 07/15] Fix up a couple static analysis issues

---
 app/models/user.rb               | 2 +-
 spec/lib/gitlab/git/user_spec.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index d321520f999..d280e1a00d6 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -880,7 +880,7 @@ def set_public_email
 
   def set_commit_email
     if commit_email.blank? || all_emails.exclude?(commit_email)
-        self.commit_email = nil
+      self.commit_email = nil
     end
   end
 
diff --git a/spec/lib/gitlab/git/user_spec.rb b/spec/lib/gitlab/git/user_spec.rb
index 61918eba4d8..d9d338206f8 100644
--- a/spec/lib/gitlab/git/user_spec.rb
+++ b/spec/lib/gitlab/git/user_spec.rb
@@ -30,7 +30,7 @@
     end
 
     context 'when commit_email has been set' do
-      let(:user) { user = build(:user, email: 'alice@example.com', commit_email: 'bob@example.com') }
+      let(:user) { build(:user, email: 'alice@example.com', commit_email: 'bob@example.com') }
       subject { described_class.from_gitlab(user) }
 
       it { expect(subject).to eq(described_class.new(user.username, user.name, user.commit_email, 'user-')) }
-- 
GitLab


From c25fc106adc300ecaa819f97cbfbbfd85bec9185 Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Mon, 3 Sep 2018 12:04:48 -0400
Subject: [PATCH 08/15] Add changelog for commit_email feature

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

diff --git a/changelogs/unreleased/23986-choose-commit-email.yml b/changelogs/unreleased/23986-choose-commit-email.yml
new file mode 100644
index 00000000000..1ebd62cd5b1
--- /dev/null
+++ b/changelogs/unreleased/23986-choose-commit-email.yml
@@ -0,0 +1,5 @@
+---
+title: Allow user to choose the email used for commits made through GitLab's UI.
+merge_request: 21213
+author: Joshua Campbell
+type: added
-- 
GitLab


From e38e64970242bf4ccb200b8e7dafa199284b7171 Mon Sep 17 00:00:00 2001
From: Joshua Campbell <jcamp0x2a@gmail.com>
Date: Mon, 3 Sep 2018 12:27:38 -0400
Subject: [PATCH 09/15] Update docs with commit_email feature

---
 doc/user/profile/index.md                 | 1 +
 doc/user/project/repository/web_editor.md | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/doc/user/profile/index.md b/doc/user/profile/index.md
index b1b822f25bd..7709e4c9844 100644
--- a/doc/user/profile/index.md
+++ b/doc/user/profile/index.md
@@ -37,6 +37,7 @@ From there, you can:
 [use GitLab as an OAuth provider](../../integration/oauth_provider.md#introduction-to-oauth)
 - Manage [personal access tokens](personal_access_tokens.md) to access your account via API and authorized applications
 - Add and delete emails linked to your account
+- Choose which email to use for notifications, web-based commits, and display on your public profile
 - Manage [SSH keys](../../ssh/README.md#ssh) to access your account via SSH
 - Manage your [preferences](preferences.md#syntax-highlighting-theme)
 to customize your own GitLab experience
diff --git a/doc/user/project/repository/web_editor.md b/doc/user/project/repository/web_editor.md
index 33c9a1a4d6b..035028c9266 100644
--- a/doc/user/project/repository/web_editor.md
+++ b/doc/user/project/repository/web_editor.md
@@ -177,5 +177,9 @@ you commit the changes you will be taken to a new merge request form.
 
 ![Start a new merge request with these changes](img/web_editor_start_new_merge_request.png)
 
+If you'd prefer _not_ to use your primary email address for commits created
+through the web editor, you can choose to use another of your linked email
+addresses from the **User Settings > Edit Profile** page.
+
 [ce-2808]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2808
 [issue closing pattern]: ../issues/automatic_issue_closing.md
-- 
GitLab


From 4b22f496343ba5f1e25ba61f075b129a2b360fd0 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Fri, 7 Sep 2018 15:54:56 +0100
Subject: [PATCH 10/15] Only show/accept appropriate emails for
 User#commit_email

---
 app/models/user.rb                |  4 ++--
 app/views/profiles/show.html.haml |  5 ++--
 spec/factories/emails.rb          |  1 +
 spec/models/user_spec.rb          | 39 +++++++++++++++++++++++++++++--
 4 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index d280e1a00d6..383de150708 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -613,7 +613,7 @@ def owns_public_email
   def owns_commit_email
     return if commit_email.blank?
 
-    errors.add(:commit_email, "is not an email you own") unless all_emails.include?(commit_email)
+    errors.add(:commit_email, "is not an email you own") unless verified_emails.include?(commit_email)
   end
 
   # Define commit_email-related attribute methods explicitly instead of relying on ActiveRecord to
@@ -879,7 +879,7 @@ def set_public_email
   end
 
   def set_commit_email
-    if commit_email.blank? || all_emails.exclude?(commit_email)
+    if commit_email.blank? || verified_emails.exclude?(commit_email)
       self.commit_email = nil
     end
   end
diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml
index c1d6daee47e..be2e7bdd92e 100644
--- a/app/views/profiles/show.html.haml
+++ b/app/views/profiles/show.html.haml
@@ -88,8 +88,9 @@
       = f.select :public_email, options_for_select(@user.all_emails, selected: @user.public_email),
         { help: 'This email will be displayed on your public profile.', include_blank: 'Do not show on profile' },
         control_class: 'select2'
-      = f.select :commit_email, options_for_select(@user.all_emails, selected: @user.commit_email),
-        { help: 'This email will be used for web based operations, such as edits and merges.', include_blank: 'Use my primary email' },
+      - selected_commit_email = @user.commit_email.presence || (@user.email if @user.primary_email_verified?)
+      = f.select :commit_email, options_for_select(@user.verified_emails, selected: selected_commit_email),
+        { help: 'This email will be used for web based operations, such as edits and merges.' },
         control_class: 'select2'
       = f.select :preferred_language, Gitlab::I18n::AVAILABLE_LANGUAGES.map { |value, label| [label, value] },
         { help: 'This feature is experimental and translations are not complete yet.' },
diff --git a/spec/factories/emails.rb b/spec/factories/emails.rb
index 4dc7961060a..d23ddf9d79b 100644
--- a/spec/factories/emails.rb
+++ b/spec/factories/emails.rb
@@ -4,5 +4,6 @@
     email { generate(:email_alias) }
 
     trait(:confirmed) { confirmed_at Time.now }
+    trait(:skip_validate) { to_create {|instance| instance.save(validate: false) } }
   end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 79f90bc60ab..a24fe04ee7b 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -163,10 +163,46 @@
       subject { build(:user) }
     end
 
-    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email, :commit_email do
+    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email do
       subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
     end
 
+    describe '#commit_email' do
+      subject(:user) { create(:user) }
+
+      it 'can be set to a confirmed email' do
+        confirmed = create(:email, :confirmed, user: user)
+        user.commit_email = confirmed.email
+
+        expect(user).to be_valid
+        expect(user.commit_email).to eq(confirmed.email)
+      end
+
+      it 'can not be set to an unconfirmed email' do
+        unconfirmed = create(:email, user: user)
+        user.commit_email = unconfirmed.email
+
+        # This should clear the commit_email attribute
+        expect(user).to be_valid
+        expect(user.commit_email).to be_nil
+      end
+
+      it 'can not be set to a non-existent email' do
+        user.commit_email = 'non-existent-email@nonexistent.nonexistent'
+
+        # This should clear the commit_email attribute
+        expect(user).to be_valid
+        expect(user.commit_email).to be_nil
+      end
+
+      it 'can not be set to an invalid email, even if confirmed' do
+        confirmed = create(:email, :confirmed, :skip_validate, user: user, email: 'invalid')
+        user.commit_email = confirmed.email
+
+        expect(user).not_to be_valid
+      end
+    end
+
     describe 'email' do
       context 'when no signup domains whitelisted' do
         before do
@@ -1373,7 +1409,6 @@
     it 'returns only confirmed emails' do
       email_confirmed = create :email, user: user, confirmed_at: Time.now
       create :email, user: user
-      user.reload
 
       expect(user.verified_emails).to match_array([user.email, email_confirmed.email])
     end
-- 
GitLab


From 04a4837c2e0dc3e61fe9c478a647817ee8c74eb7 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Fri, 7 Sep 2018 17:01:17 +0100
Subject: [PATCH 11/15] Commit email falls back to primary email if
 unspecified. Internalise this logic in the model

---
 app/models/user.rb                        |  5 +++++
 app/views/profiles/emails/index.html.haml |  2 +-
 lib/gitlab/git/user.rb                    |  2 +-
 spec/models/user_spec.rb                  | 18 ++++++++++++++++++
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 383de150708..8425d46db43 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -632,6 +632,11 @@ def commit_email_changed?
     has_attribute?(:commit_email) && attribute_changed?(:commit_email)
   end
 
+  # If commit_email is nil, we should fall back to the primary email.
+  def computed_commit_email
+    commit_email.presence || email
+  end
+
   # see if the new email is already a verified secondary email
   def check_for_verified_email
     skip_reconfirmation! if emails.confirmed.where(email: self.email).any?
diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index 3144d3703e7..37fcb5f5ab2 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -36,7 +36,7 @@
         = render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? }
         %span.float-right
           %span.badge.badge-success Primary email
-          - if !current_user.commit_email? || @primary_email === current_user.commit_email
+          - if @primary_email === current_user.computed_commit_email
             %span.badge.badge-info Commit email
           - if @primary_email === current_user.public_email
             %span.badge.badge-info Public email
diff --git a/lib/gitlab/git/user.rb b/lib/gitlab/git/user.rb
index 894cc284a4a..674826bcb60 100644
--- a/lib/gitlab/git/user.rb
+++ b/lib/gitlab/git/user.rb
@@ -4,7 +4,7 @@ class User
       attr_reader :username, :name, :email, :gl_id
 
       def self.from_gitlab(gitlab_user)
-        new(gitlab_user.username, gitlab_user.name, gitlab_user.commit_email || gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user))
+        new(gitlab_user.username, gitlab_user.name, gitlab_user.computed_commit_email, Gitlab::GlId.gl_id(gitlab_user))
       end
 
       def self.from_gitaly(gitaly_user)
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a24fe04ee7b..cb4a164ea20 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -203,6 +203,24 @@
       end
     end
 
+    describe '#computed_commit_email' do
+      let(:user) { create(:user) }
+      subject { user.computed_commit_email }
+
+      it 'returns #commit_email' do
+        user.commit_email = 'foo'
+
+        is_expected.to eq('foo')
+      end
+
+      it 'falls back to #email if #commit_email is blank' do
+        user.commit_email = ''
+        user.email = 'foo'
+
+        is_expected.to eq('foo')
+      end
+    end
+
     describe 'email' do
       context 'when no signup domains whitelisted' do
         before do
-- 
GitLab


From 0cea43e376dea277f1c6476b67d12f1d7992dbad Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Fri, 7 Sep 2018 17:27:30 +0100
Subject: [PATCH 12/15] Switch from computed_commit_email to an
 after_initialize hook

---
 app/models/user.rb                        | 27 ++++++++++++++---------
 app/views/profiles/emails/index.html.haml |  2 +-
 app/views/profiles/show.html.haml         |  3 +--
 lib/gitlab/git/user.rb                    |  2 +-
 spec/models/user_spec.rb                  | 27 ++++++++---------------
 5 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index de637cd0d75..7fcab97284a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -208,6 +208,7 @@ def update_tracked_fields!(request)
   end
 
   after_initialize :set_projects_limit
+  after_initialize :initialize_commit_email
 
   # User's Layout preference
   enum layout: [:fixed, :fluid]
@@ -627,25 +628,21 @@ def owns_commit_email
     errors.add(:commit_email, "is not an email you own") unless verified_emails.include?(commit_email)
   end
 
-  # Define commit_email-related attribute methods explicitly instead of relying on ActiveRecord to
-  # provide them. Some of the specs use the current state of the model code but an older database
-  # schema, so we need to guard against the possibility of the commit_email column not existing.
+  # Define commit_email-related attribute methods explicitly instead of relying
+  # on ActiveRecord to provide them. Some of the specs use the current state of
+  # the model code but an older database schema, so we need to guard against the
+  # possibility of the commit_email column not existing.
 
   def commit_email
-    self[:commit_email] if has_attribute?(:commit_email)
+    super if has_attribute?(:commit_email)
   end
 
   def commit_email=(email)
-    self[:commit_email] = email if has_attribute?(:commit_email)
+    super if has_attribute?(:commit_email)
   end
 
   def commit_email_changed?
-    has_attribute?(:commit_email) && attribute_changed?(:commit_email)
-  end
-
-  # If commit_email is nil, we should fall back to the primary email.
-  def computed_commit_email
-    commit_email.presence || email
+    has_attribute?(:commit_email) && super
   end
 
   # see if the new email is already a verified secondary email
@@ -894,6 +891,14 @@ def set_public_email
     end
   end
 
+  # The commit email defaults to the primary email
+  def initialize_commit_email
+    return unless has_attribute?(:commit_email)
+    return if commit_email.present?
+
+    self.commit_email = self.email
+  end
+
   def set_commit_email
     if commit_email.blank? || verified_emails.exclude?(commit_email)
       self.commit_email = nil
diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index 37fcb5f5ab2..c8faf2b3af3 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -36,7 +36,7 @@
         = render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? }
         %span.float-right
           %span.badge.badge-success Primary email
-          - if @primary_email === current_user.computed_commit_email
+          - if @primary_email === current_user.commit_email
             %span.badge.badge-info Commit email
           - if @primary_email === current_user.public_email
             %span.badge.badge-info Public email
diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml
index c56867bcbfb..51f5ecf2166 100644
--- a/app/views/profiles/show.html.haml
+++ b/app/views/profiles/show.html.haml
@@ -91,8 +91,7 @@
       = f.select :public_email, options_for_select(@user.all_emails, selected: @user.public_email),
         { help: s_("Profiles|This email will be displayed on your public profile."), include_blank: s_("Profiles|Do not show on profile") },
         control_class: 'select2'
-      - selected_commit_email = @user.commit_email.presence || (@user.email if @user.primary_email_verified?)
-      = f.select :commit_email, options_for_select(@user.verified_emails, selected: selected_commit_email),
+      = f.select :commit_email, options_for_select(@user.verified_emails, selected: @user.commit_email),
         { help: 'This email will be used for web based operations, such as edits and merges.' },
         control_class: 'select2'
       = f.select :preferred_language, Gitlab::I18n::AVAILABLE_LANGUAGES.map { |value, label| [label, value] },
diff --git a/lib/gitlab/git/user.rb b/lib/gitlab/git/user.rb
index 674826bcb60..338e1a30c45 100644
--- a/lib/gitlab/git/user.rb
+++ b/lib/gitlab/git/user.rb
@@ -4,7 +4,7 @@ class User
       attr_reader :username, :name, :email, :gl_id
 
       def self.from_gitlab(gitlab_user)
-        new(gitlab_user.username, gitlab_user.name, gitlab_user.computed_commit_email, Gitlab::GlId.gl_id(gitlab_user))
+        new(gitlab_user.username, gitlab_user.name, gitlab_user.commit_email, Gitlab::GlId.gl_id(gitlab_user))
       end
 
       def self.from_gitaly(gitaly_user)
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 9fa25f9fa10..68a1c393703 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -170,6 +170,15 @@
     describe '#commit_email' do
       subject(:user) { create(:user) }
 
+      it 'defaults to the primary email' do
+        # The default commit email is filled from an after_initialize hook that
+        # isn't triggered by the factory creation process
+        found_user = described_class.find_by(id: user.id)
+
+        expect(found_user.email).to be_present
+        expect(found_user.commit_email).to eq(user.email)
+      end
+
       it 'can be set to a confirmed email' do
         confirmed = create(:email, :confirmed, user: user)
         user.commit_email = confirmed.email
@@ -203,24 +212,6 @@
       end
     end
 
-    describe '#computed_commit_email' do
-      let(:user) { create(:user) }
-      subject { user.computed_commit_email }
-
-      it 'returns #commit_email' do
-        user.commit_email = 'foo'
-
-        is_expected.to eq('foo')
-      end
-
-      it 'falls back to #email if #commit_email is blank' do
-        user.commit_email = ''
-        user.email = 'foo'
-
-        is_expected.to eq('foo')
-      end
-    end
-
     describe 'email' do
       context 'when no signup domains whitelisted' do
         before do
-- 
GitLab


From 76da7ffd219a4abbc40f3f862790bc59b0d468d1 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 11 Sep 2018 17:01:53 +0100
Subject: [PATCH 13/15] Further reworking of User#commit_email initialization

---
 app/models/user.rb       | 16 +++++-----------
 spec/models/user_spec.rb | 20 ++++++++++++--------
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 7fcab97284a..515b0edd840 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -178,7 +178,7 @@ def update_tracked_fields!(request)
   before_validation :sanitize_attrs
   before_validation :set_notification_email, if: :new_record?
   before_validation :set_public_email, if: :public_email_changed?
-  before_validation :set_commit_email, if: :commit_email_changed?
+  before_validation :set_commit_email, if: ->(user) { user.new_record? || user.commit_email_changed? }
   before_save :set_public_email, if: :public_email_changed? # in case validation is skipped
   before_save :set_commit_email, if: :commit_email_changed? # in case validation is skipped
   before_save :ensure_incoming_email_token
@@ -208,7 +208,7 @@ def update_tracked_fields!(request)
   end
 
   after_initialize :set_projects_limit
-  after_initialize :initialize_commit_email
+  after_initialize :set_commit_email
 
   # User's Layout preference
   enum layout: [:fixed, :fluid]
@@ -891,17 +891,11 @@ def set_public_email
     end
   end
 
-  # The commit email defaults to the primary email
-  def initialize_commit_email
-    return unless has_attribute?(:commit_email)
-    return if commit_email.present?
-
-    self.commit_email = self.email
-  end
-
   def set_commit_email
+    return unless primary_email_verified?
+
     if commit_email.blank? || verified_emails.exclude?(commit_email)
-      self.commit_email = nil
+      self.commit_email = self.email
     end
   end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 68a1c393703..1ab04750ced 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -163,7 +163,7 @@
       subject { build(:user) }
     end
 
-    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email do
+    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email, :commit_email do
       subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
     end
 
@@ -171,11 +171,15 @@
       subject(:user) { create(:user) }
 
       it 'defaults to the primary email' do
-        # The default commit email is filled from an after_initialize hook that
-        # isn't triggered by the factory creation process
+        expect(user.email).to be_present
+        expect(user.commit_email).to eq(user.email)
+      end
+
+      it 'defaults to the primary email when the column in the database is null' do
+        user.update_column(:commit_email, nil)
+
         found_user = described_class.find_by(id: user.id)
 
-        expect(found_user.email).to be_present
         expect(found_user.commit_email).to eq(user.email)
       end
 
@@ -191,17 +195,17 @@
         unconfirmed = create(:email, user: user)
         user.commit_email = unconfirmed.email
 
-        # This should clear the commit_email attribute
+        # This should set the commit_email attribute to the primary email
         expect(user).to be_valid
-        expect(user.commit_email).to be_nil
+        expect(user.commit_email).to eq(user.email)
       end
 
       it 'can not be set to a non-existent email' do
         user.commit_email = 'non-existent-email@nonexistent.nonexistent'
 
-        # This should clear the commit_email attribute
+        # This should set the commit_email attribute to the primary email
         expect(user).to be_valid
-        expect(user.commit_email).to be_nil
+        expect(user.commit_email).to eq(user.email)
       end
 
       it 'can not be set to an invalid email, even if confirmed' do
-- 
GitLab


From 8af540c655eb4b0cb83b0e5b96493c4d5e4e2901 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Thu, 13 Sep 2018 15:20:22 +0100
Subject: [PATCH 14/15] Fix db/schema.rb

---
 db/schema.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/db/schema.rb b/db/schema.rb
index 8dd34c95864..40701a1b66f 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -2206,8 +2206,8 @@
     t.integer "accepted_term_id"
     t.string "feed_token"
     t.boolean "private_profile"
-    t.string "commit_email"
     t.boolean "include_private_contributions"
+    t.string "commit_email"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
-- 
GitLab


From c093d6c610329db6386ca2a6fd50f1392a62f8a1 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Thu, 13 Sep 2018 15:43:02 +0100
Subject: [PATCH 15/15] Move the commit_email fallback logic into the attribute
 reader

---
 app/models/user.rb       | 14 +++++++-------
 spec/models/user_spec.rb |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 515b0edd840..d2854df8710 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -178,7 +178,7 @@ def update_tracked_fields!(request)
   before_validation :sanitize_attrs
   before_validation :set_notification_email, if: :new_record?
   before_validation :set_public_email, if: :public_email_changed?
-  before_validation :set_commit_email, if: ->(user) { user.new_record? || user.commit_email_changed? }
+  before_validation :set_commit_email, if: :commit_email_changed?
   before_save :set_public_email, if: :public_email_changed? # in case validation is skipped
   before_save :set_commit_email, if: :commit_email_changed? # in case validation is skipped
   before_save :ensure_incoming_email_token
@@ -208,7 +208,6 @@ def update_tracked_fields!(request)
   end
 
   after_initialize :set_projects_limit
-  after_initialize :set_commit_email
 
   # User's Layout preference
   enum layout: [:fixed, :fluid]
@@ -623,7 +622,7 @@ def owns_public_email
   end
 
   def owns_commit_email
-    return if commit_email.blank?
+    return if read_attribute(:commit_email).blank?
 
     errors.add(:commit_email, "is not an email you own") unless verified_emails.include?(commit_email)
   end
@@ -634,7 +633,10 @@ def owns_commit_email
   # possibility of the commit_email column not existing.
 
   def commit_email
-    super if has_attribute?(:commit_email)
+    return unless has_attribute?(:commit_email)
+
+    # The commit email is the same as the primary email if undefined
+    super.presence || self.email
   end
 
   def commit_email=(email)
@@ -892,10 +894,8 @@ def set_public_email
   end
 
   def set_commit_email
-    return unless primary_email_verified?
-
     if commit_email.blank? || verified_emails.exclude?(commit_email)
-      self.commit_email = self.email
+      self.commit_email = nil
     end
   end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 1ab04750ced..27aec49e348 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -163,7 +163,7 @@
       subject { build(:user) }
     end
 
-    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email, :commit_email do
+    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email do
       subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
     end
 
-- 
GitLab