From 3fcab51ebb0f3156b5d732d050b292cd3e081262 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Fri, 6 Oct 2017 22:41:23 -0300 Subject: Refactor conflict resolution to contain git ops within Gitlab::Git This prepares the codebase for a Gitaly migration. See https://gitlab.com/gitlab-org/gitaly/issues/553 --- .../merge_requests/conflicts_controller.rb | 2 +- app/models/repository.rb | 8 - .../merge_requests/conflicts/list_service.rb | 4 +- .../merge_requests/conflicts/resolve_service.rb | 2 +- lib/gitlab/conflict/file.rb | 84 ++------ lib/gitlab/conflict/file_collection.rb | 102 ++-------- lib/gitlab/conflict/parser.rb | 74 ------- lib/gitlab/conflict/resolution_error.rb | 5 - lib/gitlab/git/conflict_file.rb | 84 ++++++++ lib/gitlab/git/conflict_parser.rb | 89 ++++++++ lib/gitlab/git/merge.rb | 89 ++++++++ lib/gitlab/git/repository.rb | 14 ++ .../merge_requests/conflicts_controller_spec.rb | 8 +- spec/lib/gitlab/conflict/file_collection_spec.rb | 2 +- spec/lib/gitlab/conflict/file_spec.rb | 18 +- spec/lib/gitlab/conflict/parser_spec.rb | 222 -------------------- spec/lib/gitlab/git/conflict_parser_spec.rb | 224 +++++++++++++++++++++ .../merge_requests/conflicts/list_service_spec.rb | 2 +- .../conflicts/resolve_service_spec.rb | 33 +-- 19 files changed, 577 insertions(+), 489 deletions(-) delete mode 100644 lib/gitlab/conflict/parser.rb delete mode 100644 lib/gitlab/conflict/resolution_error.rb create mode 100644 lib/gitlab/git/conflict_file.rb create mode 100644 lib/gitlab/git/conflict_parser.rb create mode 100644 lib/gitlab/git/merge.rb delete mode 100644 spec/lib/gitlab/conflict/parser_spec.rb create mode 100644 spec/lib/gitlab/git/conflict_parser_spec.rb diff --git a/app/controllers/projects/merge_requests/conflicts_controller.rb b/app/controllers/projects/merge_requests/conflicts_controller.rb index 28afef101a9..39814c1795c 100644 --- a/app/controllers/projects/merge_requests/conflicts_controller.rb +++ b/app/controllers/projects/merge_requests/conflicts_controller.rb @@ -53,7 +53,7 @@ class Projects::MergeRequests::ConflictsController < Projects::MergeRequests::Ap flash[:notice] = 'All merge conflicts were resolved. The merge request can now be merged.' render json: { redirect_to: project_merge_request_url(@project, @merge_request, resolved_conflicts: true) } - rescue Gitlab::Conflict::ResolutionError => e + rescue Gitlab::Git::Merge::ResolutionError => e render status: :bad_request, json: { message: e.message } end end diff --git a/app/models/repository.rb b/app/models/repository.rb index 579aa2b2f05..2adf51df9d6 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -912,14 +912,6 @@ class Repository end end - def resolve_conflicts(user, branch_name, params) - with_branch(user, branch_name) do - committer = user_to_committer(user) - - create_commit(params.merge(author: committer, committer: committer)) - end - end - def merged_to_root_ref?(branch_name) branch_commit = commit(branch_name) root_ref_commit = commit(root_ref) diff --git a/app/services/merge_requests/conflicts/list_service.rb b/app/services/merge_requests/conflicts/list_service.rb index 9835606812c..86e71e5aec8 100644 --- a/app/services/merge_requests/conflicts/list_service.rb +++ b/app/services/merge_requests/conflicts/list_service.rb @@ -23,13 +23,13 @@ module MergeRequests # when there are no conflict files. conflicts.files.each(&:lines) @conflicts_can_be_resolved_in_ui = conflicts.files.length > 0 - rescue Rugged::OdbError, Gitlab::Conflict::Parser::UnresolvableError, Gitlab::Conflict::FileCollection::ConflictSideMissing + rescue Rugged::OdbError, Gitlab::Git::ConflictParser::UnresolvableError, Gitlab::Git::Merge::ConflictSideMissing @conflicts_can_be_resolved_in_ui = false end end def conflicts - @conflicts ||= Gitlab::Conflict::FileCollection.read_only(merge_request) + @conflicts ||= Gitlab::Conflict::FileCollection.new(merge_request) end end end diff --git a/app/services/merge_requests/conflicts/resolve_service.rb b/app/services/merge_requests/conflicts/resolve_service.rb index a7900860c7e..27cafd2d7d9 100644 --- a/app/services/merge_requests/conflicts/resolve_service.rb +++ b/app/services/merge_requests/conflicts/resolve_service.rb @@ -2,7 +2,7 @@ module MergeRequests module Conflicts class ResolveService < MergeRequests::Conflicts::BaseService def execute(current_user, params) - conflicts = Gitlab::Conflict::FileCollection.for_resolution(merge_request) + conflicts = Gitlab::Conflict::FileCollection.new(merge_request) conflicts.resolve(current_user, params[:commit_message], params[:files]) end diff --git a/lib/gitlab/conflict/file.rb b/lib/gitlab/conflict/file.rb index 52f8fe27f3b..2d8a21f4905 100644 --- a/lib/gitlab/conflict/file.rb +++ b/lib/gitlab/conflict/file.rb @@ -4,82 +4,26 @@ module Gitlab include Gitlab::Routing include IconsHelper - MissingResolution = Class.new(ResolutionError) - CONTEXT_LINES = 3 - attr_reader :merge_file_result, :their_path, :our_path, :our_mode, :merge_request, :repository + attr_reader :merge_request, :raw + + delegate :type, :content, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw - def initialize(merge_file_result, conflict, merge_request:) - @merge_file_result = merge_file_result - @their_path = conflict[:theirs][:path] - @our_path = conflict[:ours][:path] - @our_mode = conflict[:ours][:mode] + def initialize(raw, merge_request:) + @raw = raw @merge_request = merge_request - @repository = merge_request.project.repository @match_line_headers = {} end - def content - merge_file_result[:data] - end - - def our_blob - @our_blob ||= repository.blob_at(merge_request.diff_refs.head_sha, our_path) - end - - def type - lines unless @type - - @type.inquiry - end - - # Array of Gitlab::Diff::Line objects def lines return @lines if defined?(@lines) - begin - @type = 'text' - @lines = Gitlab::Conflict::Parser.new.parse(content, - our_path: our_path, - their_path: their_path, - parent_file: self) - rescue Gitlab::Conflict::Parser::ParserError - @type = 'text-editor' - @lines = nil - end + @lines = raw.lines.nil? ? nil : map_raw_lines(raw.lines) end def resolve_lines(resolution) - section_id = nil - - lines.map do |line| - unless line.type - section_id = nil - next line - end - - section_id ||= line_code(line) - - case resolution[section_id] - when 'head' - next unless line.type == 'new' - when 'origin' - next unless line.type == 'old' - else - raise MissingResolution, "Missing resolution for section ID: #{section_id}" - end - - line - end.compact - end - - def resolve_content(resolution) - if resolution == content - raise MissingResolution, "Resolved content has no changes for file #{our_path}" - end - - resolution + map_raw_lines(raw.resolve_lines(resolution)) end def highlight_lines! @@ -227,9 +171,9 @@ module Gitlab new_path: our_path) end - # Don't try to print merge_request or repository. + # Don't try to print merge_request. def inspect - instance_variables = [:merge_file_result, :their_path, :our_path, :our_mode, :type].map do |instance_variable| + instance_variables = [:content, :their_path, :our_path, :our_mode, :type].map do |instance_variable| value = instance_variable_get("@#{instance_variable}") "#{instance_variable}=\"#{value}\"" @@ -237,6 +181,16 @@ module Gitlab "#<#{self.class} #{instance_variables.join(' ')}>" end + + private + + def map_raw_lines(raw_lines) + raw_lines.map do |raw_line| + Gitlab::Diff::Line.new(raw_line[:full_line], raw_line[:type], + raw_line[:line_obj_index], raw_line[:line_old], + raw_line[:line_new], parent_file: self) + end + end end end end diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb index 4fedfe8691a..573a953b2aa 100644 --- a/lib/gitlab/conflict/file_collection.rb +++ b/lib/gitlab/conflict/file_collection.rb @@ -1,68 +1,29 @@ module Gitlab module Conflict class FileCollection - ConflictSideMissing = Class.new(StandardError) - MissingFiles = Class.new(ResolutionError) - - attr_reader :merge_request, :our_commit, :their_commit, :project, :read_only - - delegate :repository, to: :project - - class << self - # We can only write when getting the merge index from the source - # project, because we will write to that project. We don't use this all - # the time because this fetches a ref into the source project, which - # isn't needed for reading. - def for_resolution(merge_request) - new(merge_request, merge_request.source_project, false) - end - - # We don't need to do `with_repo_branch_commit` here, because the target - # project always fetches source refs when creating merge request diffs. - def read_only(merge_request) - new(merge_request, merge_request.target_project, true) - end + attr_reader :merge_request, :merge + + def initialize(merge_request) + source_repo = merge_request.source_project.repository.raw + our_commit = merge_request.source_branch_head.raw + their_commit = merge_request.target_branch_head.raw + target_repo = merge_request.target_project.repository.raw + @merge = Gitlab::Git::Merge.new(source_repo, our_commit, target_repo, their_commit) + @merge_request = merge_request end def resolve(user, commit_message, files) - raise "can't resolve a read-only Conflict File Collection" if read_only - - repository.with_repo_branch_commit(merge_request.target_project.repository.raw, merge_request.target_branch) do - rugged = repository.rugged - - files.each do |file_params| - conflict_file = file_for_path(file_params[:old_path], file_params[:new_path]) - - write_resolved_file_to_index(merge_index, rugged, conflict_file, file_params) - end - - unless merge_index.conflicts.empty? - missing_files = merge_index.conflicts.map { |file| file[:ours][:path] } - - raise MissingFiles, "Missing resolutions for the following files: #{missing_files.join(', ')}" - end - - commit_params = { - message: commit_message || default_commit_message, - parents: [our_commit, their_commit].map(&:oid), - tree: merge_index.write_tree(rugged) - } - - repository.resolve_conflicts(user, merge_request.source_branch, commit_params) - end - end - - def merge_index - @merge_index ||= repository.rugged.merge_commits(our_commit, their_commit) + args = { + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch, + commit_message: commit_message || default_commit_message + } + merge.resolve_conflicts(user, files, args) end def files - @files ||= merge_index.conflicts.map do |conflict| - raise ConflictSideMissing unless conflict[:theirs] && conflict[:ours] - - Gitlab::Conflict::File.new(merge_index.merge_file(conflict[:ours][:path]), - conflict, - merge_request: merge_request) + @files ||= merge.conflicts.map do |conflict_file| + Gitlab::Conflict::File.new(conflict_file, merge_request: merge_request) end end @@ -81,8 +42,8 @@ module Gitlab end def default_commit_message - conflict_filenames = merge_index.conflicts.map do |conflict| - "# #{conflict[:ours][:path]}" + conflict_filenames = files.map do |conflict| + "# #{conflict.our_path}" end < 200.kilobytes - - text.force_encoding('UTF-8') - - raise UnsupportedEncoding unless text.valid_encoding? - end - - def validate_delimiter!(condition) - raise UnexpectedDelimiter unless condition - end - end - end -end diff --git a/lib/gitlab/conflict/resolution_error.rb b/lib/gitlab/conflict/resolution_error.rb deleted file mode 100644 index 0b61256b35a..00000000000 --- a/lib/gitlab/conflict/resolution_error.rb +++ /dev/null @@ -1,5 +0,0 @@ -module Gitlab - module Conflict - ResolutionError = Class.new(StandardError) - end -end diff --git a/lib/gitlab/git/conflict_file.rb b/lib/gitlab/git/conflict_file.rb new file mode 100644 index 00000000000..95a1a9bbe5b --- /dev/null +++ b/lib/gitlab/git/conflict_file.rb @@ -0,0 +1,84 @@ +module Gitlab + module Git + class ConflictFile + attr_reader :content, :their_path, :our_path, :our_mode, :repository + + def initialize(repository, commit_oid, conflict, content) + @repository = repository + @commit_oid = commit_oid + @their_path = conflict[:theirs][:path] + @our_path = conflict[:ours][:path] + @our_mode = conflict[:ours][:mode] + @content = content + end + + def lines + return @lines if defined?(@lines) + + begin + @type = 'text' + @lines = Gitlab::Git::ConflictParser.parse(content, + our_path: our_path, + their_path: their_path) + rescue Gitlab::Git::ConflictParser::ParserError + @type = 'text-editor' + @lines = nil + end + end + + def type + lines unless @type + + @type.inquiry + end + + def our_blob + # REFACTOR NOTE: the source of `commit_oid` used to be + # `merge_request.diff_refs.head_sha`. Instead of passing this value + # around the new lib structure, I decided to use `@commit_oid` which is + # equivalent to `merge_request.source_branch_head.raw.rugged_commit.oid`. + # That is what `merge_request.diff_refs.head_sha` is equivalent to when + # `merge_request` is not persisted (see `MergeRequest#diff_head_commit`). + # I think using the same oid is more consistent anyways, but if Conflicts + # start breaking, the change described above is a good place to look at. + @our_blob ||= repository.blob_at(@commit_oid, our_path) + end + + def line_code(line) + Gitlab::Git::DiffLineCode.generate(our_path, line[:line_new], line[:line_old]) + end + + def resolve_lines(resolution) + section_id = nil + + lines.map do |line| + unless line[:type] + section_id = nil + next line + end + + section_id ||= line_code(line) + + case resolution[section_id] + when 'head' + next unless line[:type] == 'new' + when 'origin' + next unless line[:type] == 'old' + else + raise Gitlab::Git::Merge::ResolutionError, "Missing resolution for section ID: #{section_id}" + end + + line + end.compact + end + + def resolve_content(resolution) + if resolution == content + raise Gitlab::Git::Merge::ResolutionError, "Resolved content has no changes for file #{our_path}" + end + + resolution + end + end + end +end diff --git a/lib/gitlab/git/conflict_parser.rb b/lib/gitlab/git/conflict_parser.rb new file mode 100644 index 00000000000..90b03848aee --- /dev/null +++ b/lib/gitlab/git/conflict_parser.rb @@ -0,0 +1,89 @@ +module Gitlab + module Git + class ConflictParser + UnresolvableError = Class.new(StandardError) + UnmergeableFile = Class.new(UnresolvableError) + UnsupportedEncoding = Class.new(UnresolvableError) + + # Recoverable errors - the conflict can be resolved in an editor, but not with + # sections. + ParserError = Class.new(StandardError) + UnexpectedDelimiter = Class.new(ParserError) + MissingEndDelimiter = Class.new(ParserError) + + class << self + def parse(text, our_path:, their_path:, parent_file: nil) + validate_text!(text) + + line_obj_index = 0 + line_old = 1 + line_new = 1 + type = nil + lines = [] + conflict_start = "<<<<<<< #{our_path}" + conflict_middle = '=======' + conflict_end = ">>>>>>> #{their_path}" + + text.each_line.map do |line| + full_line = line.delete("\n") + + if full_line == conflict_start + validate_delimiter!(type.nil?) + + type = 'new' + elsif full_line == conflict_middle + validate_delimiter!(type == 'new') + + type = 'old' + elsif full_line == conflict_end + validate_delimiter!(type == 'old') + + type = nil + elsif line[0] == '\\' + type = 'nonewline' + lines << { + full_line: full_line, + type: type, + line_obj_index: line_obj_index, + line_old: line_old, + line_new: line_new + } + else + lines << { + full_line: full_line, + type: type, + line_obj_index: line_obj_index, + line_old: line_old, + line_new: line_new + } + + line_old += 1 if type != 'new' + line_new += 1 if type != 'old' + + line_obj_index += 1 + end + end + + raise MissingEndDelimiter unless type.nil? + + lines + end + + private + + def validate_text!(text) + raise UnmergeableFile if text.blank? # Typically a binary file + raise UnmergeableFile if text.length > 200.kilobytes + + text.force_encoding('UTF-8') + + raise UnsupportedEncoding unless text.valid_encoding? + end + + def validate_delimiter!(condition) + raise UnexpectedDelimiter unless condition + end + end + end + end +end diff --git a/lib/gitlab/git/merge.rb b/lib/gitlab/git/merge.rb new file mode 100644 index 00000000000..3a0205c17f4 --- /dev/null +++ b/lib/gitlab/git/merge.rb @@ -0,0 +1,89 @@ +module Gitlab + module Git + class Merge + ConflictSideMissing = Class.new(StandardError) + ResolutionError = Class.new(StandardError) + + def initialize(repository, our_commit, target_repository, their_commit) + @repository = repository + @our_commit = our_commit.rugged_commit + @target_repository = target_repository + @their_commit = their_commit.rugged_commit + end + + def conflicts + @conflicts ||= begin + target_index = @target_repository.rugged.merge_commits(@our_commit, @their_commit) + + # We don't need to do `with_repo_branch_commit` here, because the target + # project always fetches source refs when creating merge request diffs. + target_index.conflicts.map do |conflict| + raise ConflictSideMissing unless conflict[:theirs] && conflict[:ours] + + Gitlab::Git::ConflictFile.new( + @target_repository, + @our_commit.oid, + conflict, + target_index.merge_file(conflict[:ours][:path])[:data] + ) + end + end + end + + def resolve_conflicts(user, files, source_branch:, target_branch:, commit_message:) + @repository.with_repo_branch_commit(@target_repository, target_branch) do + files.each do |file_params| + conflict_file = conflict_for_path(file_params[:old_path], file_params[:new_path]) + + write_resolved_file_to_index(conflict_file, file_params) + end + + unless index.conflicts.empty? + missing_files = index.conflicts.map { |file| file[:ours][:path] } + + raise ResolutionError, "Missing resolutions for the following files: #{missing_files.join(', ')}" + end + + commit_params = { + message: commit_message, + parents: [@our_commit, @their_commit].map(&:oid) + } + + @repository.commit_index(user, source_branch, index, commit_params) + end + end + + def conflict_for_path(old_path, new_path) + conflicts.find do |conflict| + conflict.their_path == old_path && conflict.our_path == new_path + end + end + + private + + # We can only write when getting the merge index from the source + # project, because we will write to that project. We don't use this all + # the time because this fetches a ref into the source project, which + # isn't needed for reading. + def index + @index ||= @repository.rugged.merge_commits(@our_commit, @their_commit) + end + + def write_resolved_file_to_index(file, params) + if params[:sections] + resolved_lines = file.resolve_lines(params[:sections]) + new_file = resolved_lines.map { |line| line[:full_line] }.join("\n") + + new_file << "\n" if file.our_blob.data.ends_with?("\n") + elsif params[:content] + new_file = file.resolve_content(params[:content]) + end + + our_path = file.our_path + + index.add(path: our_path, oid: @repository.rugged.write(new_file, :blob), mode: file.our_mode) + index.conflict_remove(our_path) + end + end + end +end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b705c92d686..0eae8913c49 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1096,6 +1096,20 @@ module Gitlab Gitlab::Git::Blob.find(self, sha, path) unless Gitlab::Git.blank_ref?(sha) end + def commit_index(user, branch_name, index, options) + committer = user_to_committer(user) + + OperationService.new(user, self).with_branch(branch_name) do + commit_params = options.merge( + tree: index.write_tree(rugged), + author: committer, + committer: committer + ) + + create_commit(commit_params) + end + end + def gitaly_repository Gitlab::GitalyClient::Util.repository(@storage, @relative_path, @gl_repository) end diff --git a/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb b/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb index 393d38c6e6b..7047f14c758 100644 --- a/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb @@ -17,8 +17,8 @@ describe Projects::MergeRequests::ConflictsController do describe 'GET show' do context 'when the conflicts cannot be resolved in the UI' do before do - allow_any_instance_of(Gitlab::Conflict::Parser) - .to receive(:parse).and_raise(Gitlab::Conflict::Parser::UnmergeableFile) + allow(Gitlab::Git::ConflictParser).to receive(:parse) + .and_raise(Gitlab::Git::ConflictParser::UnmergeableFile) get :show, namespace_id: merge_request_with_conflicts.project.namespace.to_param, @@ -109,8 +109,8 @@ describe Projects::MergeRequests::ConflictsController do context 'when the conflicts cannot be resolved in the UI' do before do - allow_any_instance_of(Gitlab::Conflict::Parser) - .to receive(:parse).and_raise(Gitlab::Conflict::Parser::UnmergeableFile) + allow(Gitlab::Git::ConflictParser).to receive(:parse) + .and_raise(Gitlab::Git::ConflictParser::UnmergeableFile) conflict_for_path('files/ruby/regex.rb') end diff --git a/spec/lib/gitlab/conflict/file_collection_spec.rb b/spec/lib/gitlab/conflict/file_collection_spec.rb index a4d7628b03a..5944ce8049a 100644 --- a/spec/lib/gitlab/conflict/file_collection_spec.rb +++ b/spec/lib/gitlab/conflict/file_collection_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::Conflict::FileCollection do let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start') } - let(:file_collection) { described_class.read_only(merge_request) } + let(:file_collection) { described_class.new(merge_request) } describe '#files' do it 'returns an array of Conflict::Files' do diff --git a/spec/lib/gitlab/conflict/file_spec.rb b/spec/lib/gitlab/conflict/file_spec.rb index 5356e9742b4..89dd545829b 100644 --- a/spec/lib/gitlab/conflict/file_spec.rb +++ b/spec/lib/gitlab/conflict/file_spec.rb @@ -8,9 +8,10 @@ describe Gitlab::Conflict::File do let(:our_commit) { rugged.branches['conflict-resolvable'].target } let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) } let(:index) { rugged.merge_commits(our_commit, their_commit) } - let(:conflict) { index.conflicts.last } - let(:merge_file_result) { index.merge_file('files/ruby/regex.rb') } - let(:conflict_file) { described_class.new(merge_file_result, conflict, merge_request: merge_request) } + let(:rugged_conflict) { index.conflicts.last } + let(:raw_conflict_content) { index.merge_file('files/ruby/regex.rb')[:data] } + let(:raw_conflict_file) { Gitlab::Git::ConflictFile.new(repository, our_commit.oid, rugged_conflict, raw_conflict_content) } + let(:conflict_file) { described_class.new(raw_conflict_file, merge_request: merge_request) } describe '#resolve_lines' do let(:section_keys) { conflict_file.sections.map { |section| section[:id] }.compact } @@ -48,18 +49,18 @@ describe Gitlab::Conflict::File do end end - it 'raises MissingResolution when passed a hash without resolutions for all sections' do + it 'raises ResolutionError when passed a hash without resolutions for all sections' do empty_hash = section_keys.map { |key| [key, nil] }.to_h invalid_hash = section_keys.map { |key| [key, 'invalid'] }.to_h expect { conflict_file.resolve_lines({}) } - .to raise_error(Gitlab::Conflict::File::MissingResolution) + .to raise_error(Gitlab::Git::Merge::ResolutionError) expect { conflict_file.resolve_lines(empty_hash) } - .to raise_error(Gitlab::Conflict::File::MissingResolution) + .to raise_error(Gitlab::Git::Merge::ResolutionError) expect { conflict_file.resolve_lines(invalid_hash) } - .to raise_error(Gitlab::Conflict::File::MissingResolution) + .to raise_error(Gitlab::Git::Merge::ResolutionError) end end @@ -144,7 +145,7 @@ describe Gitlab::Conflict::File do end context 'with an example file' do - let(:file) do + let(:raw_conflict_content) do <>>>>>> files/ruby/regex.rb - end - - def path_regexp - default_regexp - end - -<<<<<<< files/ruby/regex.rb - def archive_formats_regexp - /(zip|tar|7z|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)/ -======= - def archive_formats_regex - %r{(zip|tar|7z|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)} ->>>>>>> files/ruby/regex.rb - end - - def git_reference_regexp - # Valid git ref regexp, see: - # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html - %r{ - (?! - (?# doesn't begins with) - \/| (?# rule #6) - (?# doesn't contain) - .*(?: - [\/.]\.| (?# rule #1,3) - \/\/| (?# rule #6) - @\{| (?# rule #8) - \\ (?# rule #9) - ) - ) - [^\000-\040\177~^:?*\[]+ (?# rule #4-5) - (?# doesn't end with) - (?>>>>>> files/ruby/regex.rb - end - end -end -CONFLICT - end - - let(:lines) do - parser.parse(text, our_path: 'files/ruby/regex.rb', their_path: 'files/ruby/regex.rb') - end - - it 'sets our lines as new lines' do - expect(lines[8..13]).to all(have_attributes(type: 'new')) - expect(lines[26..27]).to all(have_attributes(type: 'new')) - expect(lines[56..57]).to all(have_attributes(type: 'new')) - end - - it 'sets their lines as old lines' do - expect(lines[14..19]).to all(have_attributes(type: 'old')) - expect(lines[28..29]).to all(have_attributes(type: 'old')) - expect(lines[58..59]).to all(have_attributes(type: 'old')) - end - - it 'sets non-conflicted lines as both' do - expect(lines[0..7]).to all(have_attributes(type: nil)) - expect(lines[20..25]).to all(have_attributes(type: nil)) - expect(lines[30..55]).to all(have_attributes(type: nil)) - expect(lines[60..62]).to all(have_attributes(type: nil)) - end - - it 'sets consecutive line numbers for index, old_pos, and new_pos' do - old_line_numbers = lines.select { |line| line.type != 'new' }.map(&:old_pos) - new_line_numbers = lines.select { |line| line.type != 'old' }.map(&:new_pos) - - expect(lines.map(&:index)).to eq(0.upto(62).to_a) - expect(old_line_numbers).to eq(1.upto(53).to_a) - expect(new_line_numbers).to eq(1.upto(53).to_a) - end - end - - context 'when the file contents include conflict delimiters' do - context 'when there is a non-start delimiter first' do - it 'raises UnexpectedDelimiter when there is a middle delimiter first' do - expect { parse_text('=======') } - .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter) - end - - it 'raises UnexpectedDelimiter when there is an end delimiter first' do - expect { parse_text('>>>>>>> README.md') } - .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter) - end - - it 'does not raise when there is an end delimiter for a different path first' do - expect { parse_text('>>>>>>> some-other-path.md') } - .not_to raise_error - end - end - - context 'when a start delimiter is followed by a non-middle delimiter' do - let(:start_text) { "<<<<<<< README.md\n" } - let(:end_text) { "\n=======\n>>>>>>> README.md" } - - it 'raises UnexpectedDelimiter when it is followed by an end delimiter' do - expect { parse_text(start_text + '>>>>>>> README.md' + end_text) } - .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter) - end - - it 'raises UnexpectedDelimiter when it is followed by another start delimiter' do - expect { parse_text(start_text + start_text + end_text) } - .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter) - end - - it 'does not raise when it is followed by a start delimiter for a different path' do - expect { parse_text(start_text + '>>>>>>> some-other-path.md' + end_text) } - .not_to raise_error - end - end - - context 'when a middle delimiter is followed by a non-end delimiter' do - let(:start_text) { "<<<<<<< README.md\n=======\n" } - let(:end_text) { "\n>>>>>>> README.md" } - - it 'raises UnexpectedDelimiter when it is followed by another middle delimiter' do - expect { parse_text(start_text + '=======' + end_text) } - .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter) - end - - it 'raises UnexpectedDelimiter when it is followed by a start delimiter' do - expect { parse_text(start_text + start_text + end_text) } - .to raise_error(Gitlab::Conflict::Parser::UnexpectedDelimiter) - end - - it 'does not raise when it is followed by a start delimiter for another path' do - expect { parse_text(start_text + '<<<<<<< some-other-path.md' + end_text) } - .not_to raise_error - end - end - - it 'raises MissingEndDelimiter when there is no end delimiter at the end' do - start_text = "<<<<<<< README.md\n=======\n" - - expect { parse_text(start_text) } - .to raise_error(Gitlab::Conflict::Parser::MissingEndDelimiter) - - expect { parse_text(start_text + '>>>>>>> some-other-path.md') } - .to raise_error(Gitlab::Conflict::Parser::MissingEndDelimiter) - end - end - - context 'other file types' do - it 'raises UnmergeableFile when lines is blank, indicating a binary file' do - expect { parse_text('') } - .to raise_error(Gitlab::Conflict::Parser::UnmergeableFile) - - expect { parse_text(nil) } - .to raise_error(Gitlab::Conflict::Parser::UnmergeableFile) - end - - it 'raises UnmergeableFile when the file is over 200 KB' do - expect { parse_text('a' * 204801) } - .to raise_error(Gitlab::Conflict::Parser::UnmergeableFile) - end - - # All text from Rugged has an encoding of ASCII_8BIT, so force that in - # these strings. - context 'when the file contains UTF-8 characters' do - it 'does not raise' do - expect { parse_text("Espa\xC3\xB1a".force_encoding(Encoding::ASCII_8BIT)) } - .not_to raise_error - end - end - - context 'when the file contains non-UTF-8 characters' do - it 'raises UnsupportedEncoding' do - expect { parse_text("a\xC4\xFC".force_encoding(Encoding::ASCII_8BIT)) } - .to raise_error(Gitlab::Conflict::Parser::UnsupportedEncoding) - end - end - end - end -end diff --git a/spec/lib/gitlab/git/conflict_parser_spec.rb b/spec/lib/gitlab/git/conflict_parser_spec.rb new file mode 100644 index 00000000000..42eccb8cd4d --- /dev/null +++ b/spec/lib/gitlab/git/conflict_parser_spec.rb @@ -0,0 +1,224 @@ +require 'spec_helper' + +describe Gitlab::Git::ConflictParser do + describe '.parse' do + def parse_text(text) + described_class.parse(text, our_path: 'README.md', their_path: 'README.md') + end + + context 'when the file has valid conflicts' do + let(:text) do + <>>>>>> files/ruby/regex.rb + end + + def path_regexp + default_regexp + end + +<<<<<<< files/ruby/regex.rb + def archive_formats_regexp + /(zip|tar|7z|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)/ +======= + def archive_formats_regex + %r{(zip|tar|7z|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)} +>>>>>>> files/ruby/regex.rb + end + + def git_reference_regexp + # Valid git ref regexp, see: + # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html + %r{ + (?! + (?# doesn't begins with) + \/| (?# rule #6) + (?# doesn't contain) + .*(?: + [\/.]\.| (?# rule #1,3) + \/\/| (?# rule #6) + @\{| (?# rule #8) + \\ (?# rule #9) + ) + ) + [^\000-\040\177~^:?*\[]+ (?# rule #4-5) + (?# doesn't end with) + (?>>>>>> files/ruby/regex.rb + end + end +end +CONFLICT + end + + let(:lines) do + described_class.parse(text, our_path: 'files/ruby/regex.rb', their_path: 'files/ruby/regex.rb') + end + let(:old_line_numbers) do + lines.select { |line| line[:type] != 'new' }.map { |line| line[:line_old] } + end + let(:new_line_numbers) do + lines.select { |line| line[:type] != 'old' }.map { |line| line[:line_new] } + end + let(:line_indexes) { lines.map { |line| line[:line_obj_index] } } + + it 'sets our lines as new lines' do + expect(lines[8..13]).to all(include(type: 'new')) + expect(lines[26..27]).to all(include(type: 'new')) + expect(lines[56..57]).to all(include(type: 'new')) + end + + it 'sets their lines as old lines' do + expect(lines[14..19]).to all(include(type: 'old')) + expect(lines[28..29]).to all(include(type: 'old')) + expect(lines[58..59]).to all(include(type: 'old')) + end + + it 'sets non-conflicted lines as both' do + expect(lines[0..7]).to all(include(type: nil)) + expect(lines[20..25]).to all(include(type: nil)) + expect(lines[30..55]).to all(include(type: nil)) + expect(lines[60..62]).to all(include(type: nil)) + end + + it 'sets consecutive line numbers for line_obj_index, line_old, and line_new' do + expect(line_indexes).to eq(0.upto(62).to_a) + expect(old_line_numbers).to eq(1.upto(53).to_a) + expect(new_line_numbers).to eq(1.upto(53).to_a) + end + end + + context 'when the file contents include conflict delimiters' do + context 'when there is a non-start delimiter first' do + it 'raises UnexpectedDelimiter when there is a middle delimiter first' do + expect { parse_text('=======') } + .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter) + end + + it 'raises UnexpectedDelimiter when there is an end delimiter first' do + expect { parse_text('>>>>>>> README.md') } + .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter) + end + + it 'does not raise when there is an end delimiter for a different path first' do + expect { parse_text('>>>>>>> some-other-path.md') } + .not_to raise_error + end + end + + context 'when a start delimiter is followed by a non-middle delimiter' do + let(:start_text) { "<<<<<<< README.md\n" } + let(:end_text) { "\n=======\n>>>>>>> README.md" } + + it 'raises UnexpectedDelimiter when it is followed by an end delimiter' do + expect { parse_text(start_text + '>>>>>>> README.md' + end_text) } + .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter) + end + + it 'raises UnexpectedDelimiter when it is followed by another start delimiter' do + expect { parse_text(start_text + start_text + end_text) } + .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter) + end + + it 'does not raise when it is followed by a start delimiter for a different path' do + expect { parse_text(start_text + '>>>>>>> some-other-path.md' + end_text) } + .not_to raise_error + end + end + + context 'when a middle delimiter is followed by a non-end delimiter' do + let(:start_text) { "<<<<<<< README.md\n=======\n" } + let(:end_text) { "\n>>>>>>> README.md" } + + it 'raises UnexpectedDelimiter when it is followed by another middle delimiter' do + expect { parse_text(start_text + '=======' + end_text) } + .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter) + end + + it 'raises UnexpectedDelimiter when it is followed by a start delimiter' do + expect { parse_text(start_text + start_text + end_text) } + .to raise_error(Gitlab::Git::ConflictParser::UnexpectedDelimiter) + end + + it 'does not raise when it is followed by a start delimiter for another path' do + expect { parse_text(start_text + '<<<<<<< some-other-path.md' + end_text) } + .not_to raise_error + end + end + + it 'raises MissingEndDelimiter when there is no end delimiter at the end' do + start_text = "<<<<<<< README.md\n=======\n" + + expect { parse_text(start_text) } + .to raise_error(Gitlab::Git::ConflictParser::MissingEndDelimiter) + + expect { parse_text(start_text + '>>>>>>> some-other-path.md') } + .to raise_error(Gitlab::Git::ConflictParser::MissingEndDelimiter) + end + end + + context 'other file types' do + it 'raises UnmergeableFile when lines is blank, indicating a binary file' do + expect { parse_text('') } + .to raise_error(Gitlab::Git::ConflictParser::UnmergeableFile) + + expect { parse_text(nil) } + .to raise_error(Gitlab::Git::ConflictParser::UnmergeableFile) + end + + it 'raises UnmergeableFile when the file is over 200 KB' do + expect { parse_text('a' * 204801) } + .to raise_error(Gitlab::Git::ConflictParser::UnmergeableFile) + end + + # All text from Rugged has an encoding of ASCII_8BIT, so force that in + # these strings. + context 'when the file contains UTF-8 characters' do + it 'does not raise' do + expect { parse_text("Espa\xC3\xB1a".force_encoding(Encoding::ASCII_8BIT)) } + .not_to raise_error + end + end + + context 'when the file contains non-UTF-8 characters' do + it 'raises UnsupportedEncoding' do + expect { parse_text("a\xC4\xFC".force_encoding(Encoding::ASCII_8BIT)) } + .to raise_error(Gitlab::Git::ConflictParser::UnsupportedEncoding) + end + end + end + end +end diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb index 23982b9e6e1..0b32c51a16f 100644 --- a/spec/services/merge_requests/conflicts/list_service_spec.rb +++ b/spec/services/merge_requests/conflicts/list_service_spec.rb @@ -35,7 +35,7 @@ describe MergeRequests::Conflicts::ListService do it 'returns a falsey value when the MR has a missing ref after a force push' do merge_request = create_merge_request('conflict-resolvable') service = conflicts_service(merge_request) - allow(service.conflicts).to receive(:merge_index).and_raise(Rugged::OdbError) + allow_any_instance_of(Rugged::Repository).to receive(:merge_commits).and_raise(Rugged::OdbError) expect(service.can_be_resolved_in_ui?).to be_falsey end diff --git a/spec/services/merge_requests/conflicts/resolve_service_spec.rb b/spec/services/merge_requests/conflicts/resolve_service_spec.rb index 9dc6e3c3494..4ecd318b5b6 100644 --- a/spec/services/merge_requests/conflicts/resolve_service_spec.rb +++ b/spec/services/merge_requests/conflicts/resolve_service_spec.rb @@ -107,25 +107,27 @@ describe MergeRequests::Conflicts::ResolveService do branch_name: 'conflict-start') end - def resolve_conflicts + subject do described_class.new(merge_request_from_fork).execute(user, params) end it 'gets conflicts from the source project' do + # REFACTOR NOTE: We used to test that `project.repository.rugged` wasn't + # used in this case, but since the refactor, for simplification, + # we always use that repository for read only operations. expect(forked_project.repository.rugged).to receive(:merge_commits).and_call_original - expect(project.repository.rugged).not_to receive(:merge_commits) - resolve_conflicts + subject end it 'creates a commit with the message' do - resolve_conflicts + subject expect(merge_request_from_fork.source_branch_head.message).to eq(params[:commit_message]) end it 'creates a commit with the correct parents' do - resolve_conflicts + subject expect(merge_request_from_fork.source_branch_head.parents.map(&:id)) .to eq(['404fa3fc7c2c9b5dacff102f353bdf55b1be2813', target_head]) @@ -200,14 +202,19 @@ describe MergeRequests::Conflicts::ResolveService do } end - it 'raises a MissingResolution error' do + it 'raises a ResolutionError error' do expect { service.execute(user, invalid_params) } - .to raise_error(Gitlab::Conflict::File::MissingResolution) + .to raise_error(Gitlab::Git::Merge::ResolutionError) end end context 'when the content of a file is unchanged' do - let(:list_service) { MergeRequests::Conflicts::ListService.new(merge_request) } + let(:merge) do + MergeRequests::Conflicts::ListService.new(merge_request).conflicts.merge + end + let(:regex_conflict) do + merge.conflict_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb') + end let(:invalid_params) do { @@ -219,16 +226,16 @@ describe MergeRequests::Conflicts::ResolveService do }, { old_path: 'files/ruby/regex.rb', new_path: 'files/ruby/regex.rb', - content: list_service.conflicts.file_for_path('files/ruby/regex.rb', 'files/ruby/regex.rb').content + content: regex_conflict.content } ], commit_message: 'This is a commit message!' } end - it 'raises a MissingResolution error' do + it 'raises a ResolutionError error' do expect { service.execute(user, invalid_params) } - .to raise_error(Gitlab::Conflict::File::MissingResolution) + .to raise_error(Gitlab::Git::Merge::ResolutionError) end end @@ -246,9 +253,9 @@ describe MergeRequests::Conflicts::ResolveService do } end - it 'raises a MissingFiles error' do + it 'raises a ResolutionError error' do expect { service.execute(user, invalid_params) } - .to raise_error(Gitlab::Conflict::FileCollection::MissingFiles) + .to raise_error(Gitlab::Git::Merge::ResolutionError) end end end -- cgit v1.2.1