diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-10-16 13:36:06 +0300 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-10-16 13:36:06 +0300 |
commit | b2553840e88bed82bad45712e2696de83fac1230 (patch) | |
tree | 0218a658d703f7fe09a67e5d82058c608cb55073 /lib | |
parent | 3a7623fc010832c337e0ba0eb8650d7f6fca3562 (diff) | |
parent | 9fdde3693b3b49e929b7c80ccbec4abe412edb7f (diff) |
Merge branch 'conflict-resolution-refactor' into 'master'
Conflict resolution refactor
See merge request gitlab-org/gitlab-ce!14747
Diffstat (limited to 'lib')
-rw-r--r-- | lib/api/commits.rb | 2 | ||||
-rw-r--r-- | lib/api/v3/commits.rb | 2 | ||||
-rw-r--r-- | lib/github/representation/comment.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/bitbucket_import/importer.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/conflict/file.rb | 88 | ||||
-rw-r--r-- | lib/gitlab/conflict/file_collection.rb | 68 | ||||
-rw-r--r-- | lib/gitlab/conflict/parser.rb | 74 | ||||
-rw-r--r-- | lib/gitlab/conflict/resolution_error.rb | 5 | ||||
-rw-r--r-- | lib/gitlab/diff/file.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/diff/line_code.rb | 9 | ||||
-rw-r--r-- | lib/gitlab/git.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/git/conflict/file.rb | 86 | ||||
-rw-r--r-- | lib/gitlab/git/conflict/parser.rb | 91 | ||||
-rw-r--r-- | lib/gitlab/git/conflict/resolver.rb | 91 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 18 | ||||
-rw-r--r-- | lib/gitlab/github_import/comment_formatter.rb | 2 |
16 files changed, 333 insertions, 213 deletions
diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 4af37a2ad1d..62565e2d522 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -186,7 +186,7 @@ module API lines.each do |line| next unless line.new_pos == params[:line] && line.type == params[:line_type] - break opts[:line_code] = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos) + break opts[:line_code] = Gitlab::Git.diff_line_code(diff.new_path, line.new_pos, line.old_pos) end break if opts[:line_code] diff --git a/lib/api/v3/commits.rb b/lib/api/v3/commits.rb index 345cb7e7c11..bd5c6b34d12 100644 --- a/lib/api/v3/commits.rb +++ b/lib/api/v3/commits.rb @@ -173,7 +173,7 @@ module API lines.each do |line| next unless line.new_pos == params[:line] && line.type == params[:line_type] - break opts[:line_code] = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos) + break opts[:line_code] = Gitlab::Git.diff_line_code(diff.new_path, line.new_pos, line.old_pos) end break if opts[:line_code] diff --git a/lib/github/representation/comment.rb b/lib/github/representation/comment.rb index 1b5be91461b..83bf0b5310d 100644 --- a/lib/github/representation/comment.rb +++ b/lib/github/representation/comment.rb @@ -23,7 +23,7 @@ module Github private def generate_line_code(line) - Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) + Gitlab::Git.diff_line_code(file_path, line.new_pos, line.old_pos) end def on_diff? diff --git a/lib/gitlab/bitbucket_import/importer.rb b/lib/gitlab/bitbucket_import/importer.rb index d1979bb7ed3..033ecd15749 100644 --- a/lib/gitlab/bitbucket_import/importer.rb +++ b/lib/gitlab/bitbucket_import/importer.rb @@ -241,7 +241,7 @@ module Gitlab end def generate_line_code(pr_comment) - Gitlab::Diff::LineCode.generate(pr_comment.file_path, pr_comment.new_pos, pr_comment.old_pos) + Gitlab::Git.diff_line_code(pr_comment.file_path, pr_comment.new_pos, pr_comment.old_pos) end def pull_request_comment_attributes(comment) diff --git a/lib/gitlab/conflict/file.rb b/lib/gitlab/conflict/file.rb index 98dfe900044..2a0cb640a14 100644 --- a/lib/gitlab/conflict/file.rb +++ b/lib/gitlab/conflict/file.rb @@ -4,82 +4,29 @@ 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 - - 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] - @merge_request = merge_request - @repository = merge_request.project.repository - @match_line_headers = {} - end - - def content - merge_file_result[:data] - end + attr_reader :merge_request - def our_blob - @our_blob ||= repository.blob_at(merge_request.diff_refs.head_sha, our_path) - end + # 'raw' holds the Gitlab::Git::Conflict::File that this instance wraps + attr_reader :raw - def type - lines unless @type + delegate :type, :content, :their_path, :our_path, :our_mode, :our_blob, :repository, to: :raw - @type.inquiry + def initialize(raw, merge_request:) + @raw = raw + @merge_request = merge_request + @match_line_headers = {} 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! @@ -163,7 +110,7 @@ module Gitlab end def line_code(line) - Gitlab::Diff::LineCode.generate(our_path, line.new_pos, line.old_pos) + Gitlab::Git.diff_line_code(our_path, line.new_pos, line.old_pos) end def create_match_line(line) @@ -227,15 +174,14 @@ module Gitlab new_path: our_path) end - # Don't try to print merge_request or repository. - def inspect - instance_variables = [:merge_file_result, :their_path, :our_path, :our_mode, :type].map do |instance_variable| - value = instance_variable_get("@#{instance_variable}") + private - "#{instance_variable}=\"#{value}\"" + 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 - - "#<#{self.class} #{instance_variables.join(' ')}>" end end end diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb index 90f83e0f810..fb28e80ff73 100644 --- a/lib/gitlab/conflict/file_collection.rb +++ b/lib/gitlab/conflict/file_collection.rb @@ -1,48 +1,29 @@ module Gitlab module Conflict class FileCollection - ConflictSideMissing = Class.new(StandardError) - - attr_reader :merge_request, :our_commit, :their_commit, :project - - 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) - project = merge_request.source_project - - new(merge_request, project).tap do |file_collection| - project - .repository - .with_repo_branch_commit(merge_request.target_project.repository.raw_repository, merge_request.target_branch) do - - yield file_collection - end - end - 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) - end + attr_reader :merge_request, :resolver + + 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 + @resolver = Gitlab::Git::Conflict::Resolver.new(source_repo, our_commit, target_repo, their_commit) + @merge_request = merge_request end - def merge_index - @merge_index ||= repository.rugged.merge_commits(our_commit, their_commit) + def resolve(user, commit_message, files) + args = { + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch, + commit_message: commit_message || default_commit_message + } + resolver.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 ||= resolver.conflicts.map do |conflict_file| + Gitlab::Conflict::File.new(conflict_file, merge_request: merge_request) end end @@ -61,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 <<EOM.chomp @@ -72,15 +53,6 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc #{conflict_filenames.join("\n")} EOM end - - private - - def initialize(merge_request, project) - @merge_request = merge_request - @our_commit = merge_request.source_branch_head.raw.rugged_commit - @their_commit = merge_request.target_branch_head.raw.rugged_commit - @project = project - end end end end diff --git a/lib/gitlab/conflict/parser.rb b/lib/gitlab/conflict/parser.rb deleted file mode 100644 index e3678c914db..00000000000 --- a/lib/gitlab/conflict/parser.rb +++ /dev/null @@ -1,74 +0,0 @@ -module Gitlab - module Conflict - class Parser - 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) - - 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 << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new, parent_file: parent_file) - else - lines << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new, parent_file: parent_file) - 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 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/diff/file.rb b/lib/gitlab/diff/file.rb index 599c3c5deab..ea5891a028a 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -49,7 +49,7 @@ module Gitlab def line_code(line) return if line.meta? - Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) + Gitlab::Git.diff_line_code(file_path, line.new_pos, line.old_pos) end def line_for_line_code(code) diff --git a/lib/gitlab/diff/line_code.rb b/lib/gitlab/diff/line_code.rb deleted file mode 100644 index f3578ab3d35..00000000000 --- a/lib/gitlab/diff/line_code.rb +++ /dev/null @@ -1,9 +0,0 @@ -module Gitlab - module Diff - class LineCode - def self.generate(file_path, new_line_position, old_line_position) - "#{Digest::SHA1.hexdigest(file_path)}_#{old_line_position}_#{new_line_position}" - end - end - end -end diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index c78fe63f9b5..1f31cdbc96d 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -66,6 +66,10 @@ module Gitlab end end end + + def diff_line_code(file_path, new_line_position, old_line_position) + "#{Digest::SHA1.hexdigest(file_path)}_#{old_line_position}_#{new_line_position}" + end end end end diff --git a/lib/gitlab/git/conflict/file.rb b/lib/gitlab/git/conflict/file.rb new file mode 100644 index 00000000000..fc1595f1faf --- /dev/null +++ b/lib/gitlab/git/conflict/file.rb @@ -0,0 +1,86 @@ +module Gitlab + module Git + module Conflict + class File + 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::Conflict::Parser.parse(content, + our_path: our_path, + their_path: their_path) + rescue Gitlab::Git::Conflict::Parser::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.diff_line_code(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::Conflict::Resolver::ResolutionError, "Missing resolution for section ID: #{section_id}" + end + + line + end.compact + end + + def resolve_content(resolution) + if resolution == content + raise Gitlab::Git::Conflict::Resolver::ResolutionError, "Resolved content has no changes for file #{our_path}" + end + + resolution + end + 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..3effa9d2d31 --- /dev/null +++ b/lib/gitlab/git/conflict/parser.rb @@ -0,0 +1,91 @@ +module Gitlab + module Git + module Conflict + class Parser + 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 +end diff --git a/lib/gitlab/git/conflict/resolver.rb b/lib/gitlab/git/conflict/resolver.rb new file mode 100644 index 00000000000..df509c5f4ce --- /dev/null +++ b/lib/gitlab/git/conflict/resolver.rb @@ -0,0 +1,91 @@ +module Gitlab + module Git + module Conflict + class Resolver + 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::Conflict::File.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 +end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 54f66b0683a..59a54b48ed9 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1109,6 +1109,24 @@ module Gitlab popen(args, @path).last.zero? end + def blob_at(sha, path) + 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/lib/gitlab/github_import/comment_formatter.rb b/lib/gitlab/github_import/comment_formatter.rb index e21922070c1..8911b81ec9a 100644 --- a/lib/gitlab/github_import/comment_formatter.rb +++ b/lib/gitlab/github_import/comment_formatter.rb @@ -38,7 +38,7 @@ module Gitlab end def generate_line_code(line) - Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) + Gitlab::Git.diff_line_code(file_path, line.new_pos, line.old_pos) end def on_diff? |