Merge branch 'osw-fix-quick-suggestion-application-being-reverted' into 'master'

Adjust applied suggestion reverting previous changes

Closes #56017

See merge request gitlab-org/gitlab-ce!24250

5 files changed, 226 insertions, 20 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index a9d1ece0d7e..7206d858dae 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -560,15 +560,19 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def diff_refs
-    if persisted?
-      merge_request_diff.diff_refs
-    else
-      Gitlab::Diff::DiffRefs.new(
-        base_sha:  diff_base_sha,
-        start_sha: diff_start_sha,
-        head_sha:  diff_head_sha
-      )
-    end
+    persisted? ? merge_request_diff.diff_refs : repository_diff_refs
+  end
+
+  # Instead trying to fetch the
+  # persisted diff_refs, this method goes
+  # straight to the repository to get the
+  # most recent data possible.
+  def repository_diff_refs
+    Gitlab::Diff::DiffRefs.new(
+      base_sha:  branch_merge_base_sha,
+      start_sha: target_branch_sha,
+      head_sha:  source_branch_sha
+    )
   end
 
   def branch_merge_base_sha
diff --git a/app/models/suggestion.rb b/app/models/suggestion.rb
index c76b8e71507..7eee4fbbe5f 100644
--- a/app/models/suggestion.rb
+++ b/app/models/suggestion.rb
@@ -5,8 +5,7 @@ class Suggestion < ApplicationRecord
   validates :note, presence: true
   validates :commit_id, presence: true, if: :applied?
 
-  delegate :original_position, :position, :diff_file,
-    :noteable, to: :note
+  delegate :original_position, :position, :noteable, to: :note
 
   def project
     noteable.source_project
@@ -16,6 +15,15 @@ class Suggestion < ApplicationRecord
     noteable.source_branch
   end
 
+  def file_path
+    position.file_path
+  end
+
+  def diff_file
+    repository = project.repository
+    position.diff_file(repository)
+  end
+
   # For now, suggestions only serve as a way to send patches that
   # will change a single line (being able to apply multiple in the same place),
   # which explains `from_line` and `to_line` being the same line.
diff --git a/app/services/suggestions/apply_service.rb b/app/services/suggestions/apply_service.rb
index d931d528c86..cc47b46b527 100644
--- a/app/services/suggestions/apply_service.rb
+++ b/app/services/suggestions/apply_service.rb
@@ -11,6 +11,10 @@ module Suggestions
         return error('Suggestion is not appliable')
       end
 
+      unless latest_diff_refs?(suggestion)
+        return error('The file has been changed')
+      end
+
       params = file_update_params(suggestion)
       result = ::Files::UpdateService.new(suggestion.project, @current_user, params).execute
 
@@ -19,30 +23,44 @@ module Suggestions
       end
 
       result
+    rescue Files::UpdateService::FileChangedError
+      error('The file has been changed')
     end
 
     private
 
-    def file_update_params(suggestion)
-      diff_file = suggestion.diff_file
+    # Checks whether the latest diff refs for the branch matches with
+    # the position refs we're using to update the file content. Since
+    # the persisted refs are updated async (for MergeRequest),
+    # it's more consistent to fetch this data directly from the repository.
+    def latest_diff_refs?(suggestion)
+      suggestion.position.diff_refs == suggestion.noteable.repository_diff_refs
+    end
 
-      file_path = diff_file.file_path
-      branch_name = suggestion.noteable.source_branch
-      file_content = new_file_content(suggestion)
+    def file_update_params(suggestion)
+      blob = suggestion.diff_file.new_blob
+      file_path = suggestion.file_path
+      branch_name = suggestion.branch
+      file_content = new_file_content(suggestion, blob)
       commit_message = "Apply suggestion to #{file_path}"
 
+      file_last_commit =
+        Gitlab::Git::Commit.last_for_path(suggestion.project.repository,
+                                          blob.commit_id,
+                                          blob.path)
+
       {
         file_path: file_path,
         branch_name: branch_name,
         start_branch: branch_name,
         commit_message: commit_message,
-        file_content: file_content
+        file_content: file_content,
+        last_commit_sha: file_last_commit&.id
       }
     end
 
-    def new_file_content(suggestion)
+    def new_file_content(suggestion, blob)
       range = suggestion.from_line_index..suggestion.to_line_index
-      blob = suggestion.diff_file.new_blob
 
       blob.load_all_data!
       content = blob.data.lines
diff --git a/changelogs/unreleased/osw-fix-quick-suggestion-application-being-reverted.yml b/changelogs/unreleased/osw-fix-quick-suggestion-application-being-reverted.yml
new file mode 100644
index 00000000000..1f80d7535a5
--- /dev/null
+++ b/changelogs/unreleased/osw-fix-quick-suggestion-application-being-reverted.yml
@@ -0,0 +1,5 @@
+---
+title: Adjust applied suggestion reverting previous changes
+merge_request: 24250
+author:
+type: fixed
diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb
index 3a483717756..e5ca1c155ed 100644
--- a/spec/services/suggestions/apply_service_spec.rb
+++ b/spec/services/suggestions/apply_service_spec.rb
@@ -117,13 +117,184 @@ describe Suggestions::ApplyService do
         expect(commit.committer_email).to eq(user.commit_email)
         expect(commit.author_name).to eq(user.name)
       end
+
+      context 'when it fails to apply because the file was changed' do
+        it 'returns error message' do
+          service = instance_double(Files::UpdateService)
+
+          expect(Files::UpdateService).to receive(:new)
+            .and_return(service)
+
+          allow(service).to receive(:execute)
+            .and_raise(Files::UpdateService::FileChangedError)
+
+          result = subject.execute(suggestion)
+
+          expect(result).to eq(message: 'The file has been changed', status: :error)
+        end
+      end
+
+      context 'when diff ref from position is different from repo diff refs' do
+        it 'returns error message' do
+          outdated_refs = Gitlab::Diff::DiffRefs.new(base_sha: 'foo', start_sha: 'bar', head_sha: 'outdated')
+
+          allow(suggestion).to receive(:appliable?) { true }
+          allow(suggestion.position).to receive(:diff_refs) { outdated_refs }
+
+          result = subject.execute(suggestion)
+
+          expect(result).to eq(message: 'The file has been changed', status: :error)
+        end
+      end
+
+      context 'multiple suggestions applied' do
+        let(:expected_content) do
+          <<-CONTENT.strip_heredoc
+              require 'fileutils'
+              require 'open3'
+
+              module Popen
+                extend self
+
+
+                def popen(cmd, path=nil)
+                  unless cmd.is_a?(Array)
+                    # v1 change
+                  end
+
+                  path ||= Dir.pwd
+                  # v1 change
+                  vars = {
+                    "PWD" => path
+                  }
+
+                  options = {
+                    chdir: path
+                  }
+                  # v2 change
+                  unless File.directory?(path)
+                    FileUtils.mkdir_p(path)
+                  end
+
+                  @cmd_output = ""
+                  # v2 change
+
+                  Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+                    @cmd_output << stdout.read
+                    @cmd_output << stderr.read
+                    @cmd_status = wait_thr.value.exitstatus
+                  end
+
+                  return @cmd_output, @cmd_status
+                end
+              end
+          CONTENT
+        end
+
+        let(:merge_request) do
+          create(:merge_request, source_project: project,
+                                 target_project: project)
+        end
+
+        def create_suggestion(diff, old_line: nil, new_line: nil, from_content:, to_content:, path:)
+          position = Gitlab::Diff::Position.new(old_path: path,
+                                                new_path: path,
+                                                old_line: old_line,
+                                                new_line: new_line,
+                                                diff_refs: diff.diff_refs)
+
+          suggestion_note = create(:diff_note_on_merge_request, noteable: merge_request,
+                                                                original_position: position,
+                                                                position: position,
+                                                                project: project)
+          create(:suggestion, note: suggestion_note,
+                              from_content: from_content,
+                              to_content: to_content)
+        end
+
+        def apply_suggestion(suggestion)
+          suggestion.note.reload
+          merge_request.reload
+          merge_request.clear_memoized_shas
+
+          result = subject.execute(suggestion)
+          refresh = MergeRequests::RefreshService.new(project, user)
+          refresh.execute(merge_request.diff_head_sha,
+                          suggestion.commit_id,
+                          merge_request.source_branch_ref)
+
+          result
+        end
+
+        def fetch_raw_diff(suggestion)
+          project.reload.commit(suggestion.commit_id).diffs.diff_files.first.diff.diff
+        end
+
+        it 'applies multiple suggestions in subsequent versions correctly' do
+          diff = merge_request.merge_request_diff
+          path = 'files/ruby/popen.rb'
+
+          suggestion_1_changes = { old_line: nil,
+                                   new_line: 13,
+                                   from_content: "\n",
+                                   to_content: "# v1 change\n",
+                                   path: path }
+
+          suggestion_2_changes = { old_line: 24,
+                                   new_line: 31,
+                                   from_content: "       @cmd_output << stderr.read\n",
+                                   to_content: "# v2 change\n",
+                                   path: path }
+
+          suggestion_1 = create_suggestion(diff, suggestion_1_changes)
+          suggestion_2 = create_suggestion(diff, suggestion_2_changes)
+
+          apply_suggestion(suggestion_1)
+
+          suggestion_1_diff = fetch_raw_diff(suggestion_1)
+
+          # rubocop: disable Layout/TrailingWhitespace
+          expected_suggestion_1_diff = <<-CONTENT.strip_heredoc
+            @@ -10,7 +10,7 @@ module Popen
+                 end
+             
+                 path ||= Dir.pwd
+            -
+            +# v1 change
+                 vars = {
+                   "PWD" => path
+                 }
+          CONTENT
+          # rubocop: enable Layout/TrailingWhitespace
+
+          apply_suggestion(suggestion_2)
+
+          suggestion_2_diff = fetch_raw_diff(suggestion_2)
+
+          # rubocop: disable Layout/TrailingWhitespace
+          expected_suggestion_2_diff = <<-CONTENT.strip_heredoc
+            @@ -28,7 +28,7 @@ module Popen
+             
+                 Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+                   @cmd_output << stdout.read
+            -      @cmd_output << stderr.read
+            +# v2 change
+                   @cmd_status = wait_thr.value.exitstatus
+                 end
+          CONTENT
+          # rubocop: enable Layout/TrailingWhitespace
+
+          expect(suggestion_1_diff.strip).to eq(expected_suggestion_1_diff.strip)
+          expect(suggestion_2_diff.strip).to eq(expected_suggestion_2_diff.strip)
+        end
+      end
     end
 
     context 'fork-project' do
       let(:project) { create(:project, :public, :repository) }
 
       let(:forked_project) do
-        fork_project_with_submodules(project, user)
+        fork_project_with_submodules(project, user, repository: project.repository)
       end
 
       let(:merge_request) do