Don't reset approvals after rebase from UI

Rebasing from the UI should be considered a 'safe' action, so this
should ignore the 'reset approvals on push' project
setting.

Unfortunately, as rebase is implemented by working directly on
a copy of the repository and pushing (as it's not supported fully by
libgit2), we can't detect this purely with information available to the
PostReceive job. If we used commit metadata, the MR author could also
add the same metadata and push without resetting approvals.

To work around this, add a new column to the MergeRequest model to store
the SHA from the rebase action. (Ensure that this is set before pushing,
to avoid a race condition!) Then, in PostReceive, don't reset approvals
if the pushed SHA matches the SHA stored in the database.

CHANGELOG-EE

@@ -13,6 +13,7 @@ v 8.9.0 (unreleased)
   - [Elastic] Move ES settings to application settings
   - Disable mirror flag for projects without import_url
   - UpdateMirror service return an error status when no mirror
+  - Don't reset approvals when rebasing an MR from the UI
   - Show flash notice when Git Hooks are updated successfully
   - Remove explicit Gitlab::Metrics.action assignments, are already automatic.
   - [Elastic] Project members with guest role can't access confidential issues

app/services/merge_requests/rebase_service.rb

@@ -52,6 +52,19 @@ module MergeRequests
         return false
       end
 
+      output, status = popen(
+        %W(git rev-parse #{merge_request.source_branch}),
+        tree_path,
+        git_env
+      )
+
+      unless status.zero?
+        log('Failed to get SHA of rebased branch:')
+        log(output)
+        return false
+      end
+
+      merge_request.update_attributes(rebase_commit_sha: output.chomp)
       # Push
       output, status = popen(
         %W(git push -f origin #{merge_request.source_branch}),

app/services/merge_requests/refresh_service.rb

@@ -83,7 +83,10 @@ module MergeRequests
       merge_requests_for_source_branch.each do |merge_request|
         target_project = merge_request.target_project
 
-        if target_project.approvals_before_merge.nonzero? && target_project.reset_approvals_on_push
+        if target_project.approvals_before_merge.nonzero? &&
+           target_project.reset_approvals_on_push &&
+           merge_request.rebase_commit_sha != @newrev
+
           merge_request.approvals.destroy_all
         end
       end

db/migrate/20160621123729_add_rebase_commit_sha_to_merge_requests.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRebaseCommitShaToMergeRequests < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # When using the methods "add_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" make sure that this
+  # method 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 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 :merge_requests, :rebase_commit_sha, :string
+  end
+end

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: 20160616084004) do
+ActiveRecord::Schema.define(version: 20160621123729) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -708,6 +708,7 @@ ActiveRecord::Schema.define(version: 20160616084004) do
     t.string   "merge_commit_sha"
     t.datetime "deleted_at"
     t.integer  "approvals_before_merge"
+    t.string   "rebase_commit_sha"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree

spec/services/merge_requests/rebase_service_spec.rb

@@ -26,6 +26,11 @@ describe MergeRequests::RebaseService do
         target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha
         expect(parent_sha).to eq(target_branch_sha)
       end
+
+      it 'records the new SHA on the merge request' do
+        head_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha
+        expect(merge_request.reload.rebase_commit_sha).to eq(head_sha)
+      end
     end
   end
 end

spec/services/merge_requests/refresh_service_spec.rb

@@ -173,27 +173,62 @@ describe MergeRequests::RefreshService, services: true do
     end
 
     context 'resetting approvals if they are enabled' do
-      it "does not reset approvals if approvals_before_merge is disabled" do
+      context 'when approvals_before_merge is disabled' do
+        before do
           @project.update(approvals_before_merge: 0)
           refresh_service = service.new(@project, @user)
           allow(refresh_service).to receive(:execute_hooks)
           refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
           reload_mrs
+        end
 
+        it 'does not reset approvals' do
           expect(@merge_request.approvals).not_to be_empty
         end
+      end
 
-      it "does not reset approvals if reset_approvals_on_push is disabled" do
+      context 'when approvals_before_merge is disabled' do
+        before do
           @project.update(reset_approvals_on_push: false)
           refresh_service = service.new(@project, @user)
           allow(refresh_service).to receive(:execute_hooks)
           refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
           reload_mrs
+        end
 
+        it 'does not reset approvals' do
           expect(@merge_request.approvals).not_to be_empty
         end
       end
 
+      context 'when the rebase_commit_sha on the MR matches the pushed SHA' do
+        before do
+          @merge_request.update(rebase_commit_sha: @newrev)
+          refresh_service = service.new(@project, @user)
+          allow(refresh_service).to receive(:execute_hooks)
+          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+          reload_mrs
+        end
+
+        it 'does not reset approvals' do
+          expect(@merge_request.approvals).not_to be_empty
+        end
+      end
+
+      context 'when there are approvals to be reset' do
+        before do
+          refresh_service = service.new(@project, @user)
+          allow(refresh_service).to receive(:execute_hooks)
+          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+          reload_mrs
+        end
+
+        it 'resets the approvals' do
+          expect(@merge_request.approvals).to be_empty
+        end
+      end
+    end
+
     context 'push new branch that exists in a merge request' do
       let(:refresh_service) { service.new(@fork_project, @user) }