Merge conflict resolution
Merge request reports
Activity
Reassigned to @DouweM
@DouweM do you want to take a look please? Known problems:
- Renames. libgit2 doesn't seem to detect these as conflicts.
- The constructor for a
Conflict::Fileis crazy, this should be made simpler. - We might not need to parse the conflict at all, we could maybe use a stand-alone implementation of three-way diff like: https://github.com/GoBoundless/dyph
- The exceptions that
Conflict::Parserthrows aren't handled; we need to decide what the front-end wants for those. Maybe just an empty array? - We probably want to store the number of conflicts on the MR model, so the tab can have the right number without us needing to create the index each time.
For resolving, we just need to parse the conflicts again, and parse all the lines that are either unchanged or match the lines in the request from the frontend.
@fatihacet my front-end changes are in https://gitlab.com/gitlab-org/gitlab-ce/commit/8bbe3b2ea12402e8425080f34a5bd7f942a0fe7d as well, mostly just making sure this works with the backend API and swapping some ours / theirs mixups
@smcgivern Your frontend changes LGTM. I will now update CS to ES6 in your branch.
I like the irony tho, hang in there GitLab, it's about time
-
@smcgivern Btw, line 12 in both sides should also have color of the related lines, right now they are white and empty but it's not related with you or your changes tho. I will take a look at it soon enough.
Edited by Fatih Acet Renames. libgit2 doesn't seem to detect these as conflicts.
Laaame. Can we detect them ourselves?
ancestor...oursandancestor...theirswould both have aDiff::Filewith the sameold_path, but differentnew_path. Also interesting: What happens if both diffs rename a different file, to the same filename? :D We don't need to be perfect about these in the first iteration though. If we could, that would be great.The constructor for a Conflict::File is crazy, this should be made simpler.
Let me respond in-code.
We might not need to parse the conflict at all, we could maybe use a stand-alone implementation of three-way diff like: https://github.com/GoBoundless/dyph
What are the benefits of this?
The exceptions that Conflict::Parser throws aren't handled; we need to decide what the front-end wants for those. Maybe just an empty array?
I'd guess just show an error page: "GitLab could not figure out the conflicts. Please resolve them locally", or whatever.
We probably want to store the number of conflicts on the MR model, so the tab can have the right number without us needing to create the index each time.
Yes! Store it when we transition
merge_statustocannot_be_merged.Edited by Douwe Maan119 119 end 120 120 end 121 121 122 def conflicts 123 return render_404 unless @merge_request.cannot_be_merged? 124 125 respond_to do |format| 126 format.html { render 'show' } 127 format.json do 128 rugged = @merge_request.project.repository.rugged 129 their_commit = rugged.branches[@merge_request.target_branch].target 130 our_commit = rugged.lookup(@merge_request.diff_head_sha) 119 119 end 120 120 end 121 121 122 def conflicts 123 return render_404 unless @merge_request.cannot_be_merged? 124 125 respond_to do |format| 126 format.html { render 'show' } 127 format.json do 128 rugged = @merge_request.project.repository.rugged 129 their_commit = rugged.branches[@merge_request.target_branch].target 119 119 end 120 120 end 121 121 122 def conflicts 123 return render_404 unless @merge_request.cannot_be_merged? 124 125 respond_to do |format| 126 format.html { render 'show' } 127 format.json do 128 rugged = @merge_request.project.repository.rugged 129 their_commit = rugged.branches[@merge_request.target_branch].target 130 our_commit = rugged.lookup(@merge_request.diff_head_sha) 131 merge = rugged.merge_commits(our_commit, their_commit) 123 return render_404 unless @merge_request.cannot_be_merged? 124 125 respond_to do |format| 126 format.html { render 'show' } 127 format.json do 128 rugged = @merge_request.project.repository.rugged 129 their_commit = rugged.branches[@merge_request.target_branch].target 130 our_commit = rugged.lookup(@merge_request.diff_head_sha) 131 merge = rugged.merge_commits(our_commit, their_commit) 132 commit_message = "Merge branch '#{@merge_request.target_branch}' into '#{@merge_request.source_branch}'\n\n# Conflicts:" 133 134 files = merge.conflicts.map do |conflict| 135 their_path = conflict[:theirs][:path] 136 our_path = conflict[:ours][:path] 137 138 raise 'path mismatch!' unless their_path == our_path 67 for line in lines 68 if line.id is sectionId and (line.lineType is 'conflict' or line.isHeader) 69 if selection is 'head' and line.isHead 70 line.isSelected = yes 71 line.isUnselected = no 72 else if selection is 'origin' and line.isOrigin 73 line.isSelected = yes 74 line.isUnselected = no 75 else 76 line.isUnselected = yes 77 line.isSelected = no 78 79 80 decorateData: (data) -> 81 82 headHeaderText = 'HEAD//our changes' @fatihacet What do you think about showing the branch name here? Most people will know what their target branch is, and what the source branch is.
@DouweM Yes, please! In a MR context everyone knows their source and target branch names, but
oursandtheirsis not nearly as obvious.
128 rugged = @merge_request.project.repository.rugged 129 their_commit = rugged.branches[@merge_request.target_branch].target 130 our_commit = rugged.lookup(@merge_request.diff_head_sha) 131 merge = rugged.merge_commits(our_commit, their_commit) 132 commit_message = "Merge branch '#{@merge_request.target_branch}' into '#{@merge_request.source_branch}'\n\n# Conflicts:" 133 134 files = merge.conflicts.map do |conflict| 135 their_path = conflict[:theirs][:path] 136 our_path = conflict[:ours][:path] 137 138 raise 'path mismatch!' unless their_path == our_path 139 140 commit_message << "\n# #{our_path}" 141 merge_file = merge.merge_file(our_path) 142 143 Gitlab::Conflict::File.new(merge_file, conflict, @merge_request.target_branch, @merge_request.source_branch, @merge_request.project.repository) @smcgivern Yep, sounds reasonable
61 61 = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do 62 62 Changes 63 63 %span.badge= @merge_request.diff_size 64 %li.conflicts-tab -
@DouweM yes please add everything you see. I will add a TODO for this.
78 82 - # This tab is always loaded via AJAX 79 83 #diffs.diffs.tab-pane 80 84 - # This tab is always loaded via AJAX 85 #conflicts.conflicts.tab-pane 86 - # This tab is always loaded via AJAX 87 = render 'projects/merge_requests/show/conflicts' 14 Side-by-side 15 16 .js-toggle-container 17 .commit-stat-summary 18 Showing 19 %strong.cred {{conflictsCount}} conflicts 20 for 21 %strong {{conflictsData.source_branch}} 22 into 23 %strong {{conflictsData.target_branch}} 24 25 .files-wrapper{ "v-if" => "!isLoading" } 26 .files{"v-if" => "isParallel"} 27 .diff-file.file-holder.conflict.parallel-view{"v-for" => "file in conflictsData.files"} 28 .file-title 29 %span {{file.new_path}} @fatihacet We should show an icon here, like we have in the normal file and diff views
63 .diff-wrap-lines.code.file-content.js-syntax-highlight.white 64 %table 65 %tr.line_holder{"v-for" => "line in file.inlineLines", | 66 ":class" => "{ | 67 'head': line.isHead, | 68 'origin': line.isOrigin, | 69 'selected': line.isSelected, | 70 'unselected': line.isUnselected }"} 71 72 %template{"v-if" => "!line.isHeader"} 73 %td.diff-line-num.old_line 74 %a {{line.old_line}} 75 %td.diff-line-num.new_line 76 %a {{line.new_line}} 77 %td.line_content 78 {{{line.rich_text}}} @fatihacet Not
richText?-
rich_textorrichTextintroduced by @smcgivern.@smcgivern can you check this please?
-
@fatihacet up to you, I was just hammering away to get it to show
59 .file-actions 60 %a.btn.btn-sm View file @{{conflictsData.shortCommitSha}} 61 62 .diff-content.diff-wrap-lines 63 .diff-wrap-lines.code.file-content.js-syntax-highlight.white 64 %table 65 %tr.line_holder{"v-for" => "line in file.inlineLines", | 66 ":class" => "{ | 67 'head': line.isHead, | 68 'origin': line.isOrigin, | 69 'selected': line.isSelected, | 70 'unselected': line.isUnselected }"} 71 72 %template{"v-if" => "!line.isHeader"} 73 %td.diff-line-num.old_line 74 %a {{line.old_line}} @fatihacet
%adoesn't link anywhere?
3 3 This merge request contains merge conflicts 4 4 5 5 %p 6 Please resolve these conflicts or 6 Please 7 = link_to conflicts_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) do - lib/gitlab/conflict/file.rb 0 → 100644
27 28 @highlighted_lines = lines.map do |line| 29 line = line.dup 30 if line.type == 'old' 31 line.rich_text = their_highlight[line.old_line - 1].delete("\n") 32 else 33 line.rich_text = our_highlight[line.new_line - 1].delete("\n") 34 end 35 line 36 end 37 end 38 39 def sections 40 return @sections if @sections 41 42 chunked_lines = highlighted_lines.chunk { |line| line.type.nil? } - lib/gitlab/conflict/file.rb 0 → 100644
1 module Gitlab 2 module Conflict 3 class File 4 CONTEXT_LINES = 3 5 6 attr_reader :merge_file, :their_path, :their_ref, :our_path, :our_ref, :repository 7 8 def initialize(merge_file, conflict, their_ref, our_ref, repository) -
- It's the output of
Rugged::Index#merge_file, which is just a hash. libgit2 calls this agit_merge_file_result, which isn't a huge amount more descriptive :-/ - Yes, definitely.
- Yes, but it can't be optional - we need a repository for reading (to highlight) and resolving (to add objects).
- It's the output of
- At least
merge_file_resultdoesn't make it sounds like it contains a file :) 
- Right, but I like to have keyword args after the first 2 or so "obvious" attributes ;)
Edited by Douwe Maan- At least
- lib/gitlab/conflict/file.rb 0 → 100644
41 42 chunked_lines = highlighted_lines.chunk { |line| line.type.nil? } 43 match_line = nil 44 45 @sections = chunked_lines.flat_map.with_index do |(no_conflict, lines), i| 46 section = nil 47 48 if no_conflict 49 conflict_before = i > 0 50 conflict_after = chunked_lines.peek 51 52 if conflict_before && conflict_after 53 if lines.length > CONTEXT_LINES * 2 54 tail_lines = lines.last(CONTEXT_LINES) 55 first_tail_line = tail_lines.first 56 match_line = Gitlab::Diff::Line.new('', Never mind, already done later!Hmm, not when!section?Edited by Douwe Maan-
I don't think so. Here's my reasoning:
- Every conflict has context before it, unless it's at the start of the file (in which case we don't need a match line anyway).
- We only have a match line in the case where a context block is non-contiguous with the lines before it.
- That only happens when the gap between two conflicts is greater than
CONTEXT_LINES * 2. - So in that case only, we add two lots of lines and create a match line.
- If we already have a match line, we need to keep updating it until we get to the end of the contiguous lines it represents - which is every iteration until the next time we have to split a context block, at which point match line gets reset anyway.
Basically, there aren't gaps until we create them, so we only need to set this when we create a gap. That might be totally wrong, though!
