Commit 8c1991b9 authored by Douwe Maan's avatar Douwe Maan

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
parents 3e9c9f97 6e293681
...@@ -560,16 +560,20 @@ class MergeRequest < ActiveRecord::Base ...@@ -560,16 +560,20 @@ class MergeRequest < ActiveRecord::Base
end end
def diff_refs def diff_refs
if persisted? persisted? ? merge_request_diff.diff_refs : repository_diff_refs
merge_request_diff.diff_refs end
else
# 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( Gitlab::Diff::DiffRefs.new(
base_sha: diff_base_sha, base_sha: branch_merge_base_sha,
start_sha: diff_start_sha, start_sha: target_branch_sha,
head_sha: diff_head_sha head_sha: source_branch_sha
) )
end end
end
def branch_merge_base_sha def branch_merge_base_sha
branch_merge_base_commit.try(:sha) branch_merge_base_commit.try(:sha)
......
...@@ -5,8 +5,7 @@ class Suggestion < ApplicationRecord ...@@ -5,8 +5,7 @@ class Suggestion < ApplicationRecord
validates :note, presence: true validates :note, presence: true
validates :commit_id, presence: true, if: :applied? validates :commit_id, presence: true, if: :applied?
delegate :original_position, :position, :diff_file, delegate :original_position, :position, :noteable, to: :note
:noteable, to: :note
def project def project
noteable.source_project noteable.source_project
...@@ -16,6 +15,15 @@ class Suggestion < ApplicationRecord ...@@ -16,6 +15,15 @@ class Suggestion < ApplicationRecord
noteable.source_branch noteable.source_branch
end 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 # 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), # 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. # which explains `from_line` and `to_line` being the same line.
......
...@@ -11,6 +11,10 @@ module Suggestions ...@@ -11,6 +11,10 @@ module Suggestions
return error('Suggestion is not appliable') return error('Suggestion is not appliable')
end end
unless latest_diff_refs?(suggestion)
return error('The file has been changed')
end
params = file_update_params(suggestion) params = file_update_params(suggestion)
result = ::Files::UpdateService.new(suggestion.project, @current_user, params).execute result = ::Files::UpdateService.new(suggestion.project, @current_user, params).execute
...@@ -19,30 +23,44 @@ module Suggestions ...@@ -19,30 +23,44 @@ module Suggestions
end end
result result
rescue Files::UpdateService::FileChangedError
error('The file has been changed')
end end
private private
def file_update_params(suggestion) # Checks whether the latest diff refs for the branch matches with
diff_file = suggestion.diff_file # 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 def file_update_params(suggestion)
branch_name = suggestion.noteable.source_branch blob = suggestion.diff_file.new_blob
file_content = new_file_content(suggestion) file_path = suggestion.file_path
branch_name = suggestion.branch
file_content = new_file_content(suggestion, blob)
commit_message = "Apply suggestion to #{file_path}" 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, file_path: file_path,
branch_name: branch_name, branch_name: branch_name,
start_branch: branch_name, start_branch: branch_name,
commit_message: commit_message, commit_message: commit_message,
file_content: file_content file_content: file_content,
last_commit_sha: file_last_commit&.id
} }
end end
def new_file_content(suggestion) def new_file_content(suggestion, blob)
range = suggestion.from_line_index..suggestion.to_line_index range = suggestion.from_line_index..suggestion.to_line_index
blob = suggestion.diff_file.new_blob
blob.load_all_data! blob.load_all_data!
content = blob.data.lines content = blob.data.lines
......
---
title: Adjust applied suggestion reverting previous changes
merge_request: 24250
author:
type: fixed
...@@ -117,13 +117,184 @@ describe Suggestions::ApplyService do ...@@ -117,13 +117,184 @@ describe Suggestions::ApplyService do
expect(commit.committer_email).to eq(user.commit_email) expect(commit.committer_email).to eq(user.commit_email)
expect(commit.author_name).to eq(user.name) expect(commit.author_name).to eq(user.name)
end 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 end
context 'fork-project' do context 'fork-project' do
let(:project) { create(:project, :public, :repository) } let(:project) { create(:project, :public, :repository) }
let(:forked_project) do let(:forked_project) do
fork_project_with_submodules(project, user) fork_project_with_submodules(project, user, repository: project.repository)
end end
let(:merge_request) do let(:merge_request) do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment