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::File
is 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::Parser
throws 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 AcetRenames. libgit2 doesn't seem to detect these as conflicts.
Laaame. Can we detect them ourselves?
ancestor...ours
andancestor...theirs
would both have aDiff::File
with 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_status
tocannot_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
ours
andtheirs
is 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_text
orrichText
introduced 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
%a
doesn'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_result
doesn'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 MaanI 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!
- lib/gitlab/conflict/file.rb 0 → 100644
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('', 57 'match', 58 first_tail_line.index, 59 first_tail_line.old_pos, 60 first_tail_line.new_pos) 61 62 section = [ 63 { conflict: false, lines: lines.first(CONTEXT_LINES) }, 64 { conflict: false, lines: tail_lines.unshift(match_line) } 65 ] 66 end 67 elsif conflict_after 68 lines = lines.last(CONTEXT_LINES) - lib/gitlab/conflict/file.rb 0 → 100644
60 first_tail_line.new_pos) 61 62 section = [ 63 { conflict: false, lines: lines.first(CONTEXT_LINES) }, 64 { conflict: false, lines: tail_lines.unshift(match_line) } 65 ] 66 end 67 elsif conflict_after 68 lines = lines.last(CONTEXT_LINES) 69 elsif conflict_before 70 lines = lines.first(CONTEXT_LINES) 71 end 72 end 73 74 if match_line && !section 75 match_line.text = "@@ -#{match_line.old_pos},#{lines.last.old_pos} +#{match_line.new_pos},#{lines.last.new_pos} @@" Yes, I still have a note to check and add the section headers. There's https://github.com/libgit2/libgit2/blob/master/src/xdiff/xemit.c#L148 but I'm not sure where
find_func
comes from, exactly.
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}} 30 .file-actions 31 %a.btn.btn-sm View file @{{conflictsData.shortCommitShab}} 32 33 .diff-content.diff-wrap-lines 34 .diff-wrap-lines.code.file-content.js-syntax-highlight.white Always in
white
? I likedark
:DEdited by Douwe Maan@fatihacet If we're going to support all highlighting themes (like I think we should), we should tweak the colors for ours/theirs as well
- lib/gitlab/conflict/file.rb 0 → 100644
16 17 # Array of Gitlab::Diff::Line objects 18 def lines 19 @lines ||= Gitlab::Conflict::Parser.new.parse(merge_file[:data], their_path, our_path) 20 end 21 22 def highlighted_lines 23 return @highlighted_lines if @highlighted_lines 24 25 their_highlight = Gitlab::Highlight.highlight_lines(repository, their_ref, their_path) 26 our_highlight = Gitlab::Highlight.highlight_lines(repository, our_ref, our_path) 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") @smcgivern Hmm. Do our other
diff_lines
not have a\n
at the end too?We strip it out: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/diff/parser.rb#L21
I guess there's a bunch of ways to resolve this, just not sure what the best one is. I'm going to move this to the spec for now.
3 3 class Line 4 4 attr_reader :type, :index, :old_pos, :new_pos 5 5 attr_accessor :text 6 attr_accessor :rich_text Yes, I think so. I'd like something like https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5401#note_13298134 (quoting as it doesn't auto-expand):
@DouweM just a thought: if we stored the highlighted content in
Diff::Line#rich_text
, and kept the un-highlighted content inDiff::Line#text
, then we could just have ahighlight!
method. We also wouldn't need to duplicate all the lines like we do now.What would potentially be even nicer is have that called on the first call to
#rich_text
on any of the diff lines. That might be a bit too magical, though.@smcgivern I like that suggestion.
- lib/gitlab/conflict/parser.rb 0 → 100644
1 module Gitlab 2 module Conflict 3 class Parser 4 class UnexpectedDelimiter < StandardError 5 end 6 7 class MissingEndDelimiter < StandardError 8 end 9 10 def parse(text, their_path, our_path) @smcgivern Not if we ever do renames :D
- lib/gitlab/conflict/parser.rb 0 → 100644
40 else 41 lines << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) 42 line_old += 1 if type != 'new' 43 line_new += 1 if type != 'old' 44 45 line_obj_index += 1 46 end 47 end 48 49 raise MissingEndDelimiter unless type == nil 50 51 lines 52 end 53 54 def empty? 55 @lines.empty? - lib/gitlab/conflict/parser.rb 0 → 100644
34 raise UnexpectedDelimiter unless type == 'old' 35 36 type = nil 37 elsif line[0] == '\\' 38 type = 'nonewline' 39 lines << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) 40 else 41 lines << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) 42 line_old += 1 if type != 'new' 43 line_new += 1 if type != 'old' 44 45 line_obj_index += 1 46 end 47 end 48 49 raise MissingEndDelimiter unless type == nil - lib/gitlab/conflict/parser.rb 0 → 100644
3 class Parser 4 class UnexpectedDelimiter < StandardError 5 end 6 7 class MissingEndDelimiter < StandardError 8 end 9 10 def parse(text, their_path, our_path) 11 return [] if text.blank? 12 13 line_obj_index = 0 14 line_old = 1 15 line_new = 1 16 type = nil 17 lines = [] 18 conflict_start = "<<<<<<< #{our_path}" As I know you've realized, this is very brittle :/ Using https://github.com/GoBoundless/dyph may indeed be a better idea, although we want to leverage rugged's upcoming knowledge of merge strategies as defined in
.gitattributes
, which https://github.com/GoBoundless/dyph won't...@smcgivern Yep sounds good. I'm happy as long as things like this are documented somewhere!
Reassigned to @smcgivern
mentioned in issue #3963 (closed)
- lib/gitlab/conflict/parser.rb 0 → 100644
3 class Parser 4 class UnexpectedDelimiter < StandardError 5 end 6 7 class MissingEndDelimiter < StandardError 8 end 9 10 def parse(text, their_path, our_path) 11 return [] if text.blank? 12 13 line_obj_index = 0 14 line_old = 1 15 line_new = 1 16 type = nil 17 lines = [] 18 conflict_start = "<<<<<<< #{our_path}" Totally agree. Originally I had the section splitting in here too, to avoid doing too many passes over the file. But splitting the responsibilities like this means that we can either pass the result of this, or the result of a 'native' three-way diff, to
Gitlab::Conflict::File
without too many issues.So my plan for the moment is to carry on with the resolution step (which is independent of how we get the conflict), and then go back and review whether or not Dyph (or something else!) is worth it. I see we have a
Repository#gitattribute
method, so we can handle that case ... with work😄
Laaame. Can we detect them ourselves?
ancestor...ours
andancestor...theirs
would both have aDiff::File
with the sameold_path
, but differentnew_path
.That's a really good idea! The only problem I can see is that we'd have to do that on every push to set whether the MR has conflicts or not, because Rugged will return no conflicts if that's the only conflict. But that might not be a big deal. I'm also not sure on what the API would look like for that, so I'm tempted to create a separate issue for it as it will also need frontend work too.
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.
That's mean! The git CLI does this:
- Create two (untracked) files:
$newname~HEAD
and$newname-$theirref
. - Add the deletion of the file in their ref to the index.
- Mark
$newname
as 'both added'.
With Rugged, I get a standard conflict file, with the old names both removed, as if renames were just removals and additions.
- Create two (untracked) files:
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.
I'm not sure if this is needed if we're not showing this in a tab. Do we need the number of conflicts on the main MR page @fatihacet @cperessini?
Also, @fatihacet, what do you want the backend to return when the conflict isn't resolvable in this way? Originally I was going to do this by file, but that's silly, because we're doing all-or-nothing here, so if we can't resolve one file, we can't resolve the conflict in the UI at all.
@smcgivern since we agreed to remove the tab and if resolving conflicts isn't possible, I think say it and ask for manual resolving.
⚠ This merge request contains merge conflictsIt's not possible to resolve conflicts in GitLab, please resolve these conflicts or merge this request manually.
@fatihacet oh yeah, I could just check that a step earlier (on push)! Good point, thanks!
@smcgivern As discussed, let's look at renames later.
@smcgivern We could show a message like
This merge request contains 6 merge conflicts
, but I don't think it provides too much value so if it makes the backend simpler we can do without the total count in the MR page.@DouweM @fatihacet I just pushed, there's now a resolution endpoint. Still to do for me in this MR:
- Auto-highlight diff lines when
rich_text
is accessed: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5479#note_13316079 - Add context to match line: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5479#note_13316064
- Investigate using an alternative for three-way diff: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5479#note_13316112
- Handle the exceptions that the parsing and resolving throw, and add specs for these cases
- Move to its own section, show only when resolvable
- Add more detailed specs for conflict resolving
Edited by Sean McGivern- Auto-highlight diff lines when
428 let(:json_response) { JSON.parse(response.body) } 429 430 it 'includes meta info about the MR' do 431 expect(json_response['commit_message']).to include('Merge branch') 432 expect(json_response['commit_sha']).to match(/\h{40}/) 433 expect(json_response['source_branch']).to eq(merge_request_with_conflicts.source_branch) 434 expect(json_response['target_branch']).to eq(merge_request_with_conflicts.target_branch) 435 end 436 437 it 'includes each file that has conflicts' do 438 filenames = json_response['files'].map { |file| file['new_path'] } 439 440 expect(filenames).to contain_exactly('files/ruby/popen.rb', 'files/ruby/regex.rb') 441 end 442 443 it 'splits files into sections with lines' do mentioned in issue #20344 (closed)
mentioned in issue #20345 (moved)
mentioned in issue #3567 (closed)
Also, @fatihacet, which of us wants to rebase / merge master? We could screenhero it at some point if we want to pair on it.
Added 1 commit:
- de1a6059 - Allow resolving conflicts in MR controller
Added 1 commit:
- 44250a4c - Allow resolving conflicts in MR controller
Added 1 commit:
- 2ed536c9 - Allow resolving conflicts in MR controller
Added 1308 commits:
-
2ed536c9...7b015fd8 - 1292 commits from branch
master
- 5b57d5d3 - Added vue.js as a lib.
- 72c49f72 - Added routes for conflicts in merge requests page.
- 8e08dabf - Added dummy merge conflicts data.
- 5a9640c9 - Added new Conflicts tab and conflicts placeholder to MRs page.
- 2188e3d1 - Make resolve these conflicts text a link to conflicts tab.
- c53db615 - Added new UI components for Merge Conflicts UI.
- 26d32d7d - Improvements to simplify vue template.
- 1e910122 - Update dummy data.
- 41212200 - Implemented conflict side selection.
- 34e39483 - Remove icon and get conflicts count from data.
- f07417c3 - Styling for selected/unselected versions.
- cf7f0056 - Implement conflict selection with highlighting.
- b7b99bf4 - Add backend for merge conflicts reading
- 9150f3fc - Handle multiple merge conflict files in collection
- 277690fc - Allow resolving conflicts in MR controller
- e9a7179f - CS to JS.
Toggle commit list-
2ed536c9...7b015fd8 - 1292 commits from branch
Added 15 commits:
- 97c0e43e - Added vue.js as a lib.
- f848af09 - Added routes for conflicts in merge requests page.
- 91f30ff5 - Added dummy merge conflicts data.
- 360d5d2d - Added new Conflicts tab and conflicts placeholder to MRs page.
- fc193e50 - Make resolve these conflicts text a link to conflicts tab.
- 2a6544de - Added new UI components for Merge Conflicts UI.
- ee626609 - Improvements to simplify vue template.
- 85c6fd18 - Update dummy data.
- b8b2c288 - Implemented conflict side selection.
- 8f6e7029 - Remove icon and get conflicts count from data.
- a23a250f - Styling for selected/unselected versions.
- 61c4d128 - Implement conflict selection with highlighting.
- 8bbe3b2e - Add backend for merge conflicts reading
- d90a5e04 - Handle multiple merge conflict files in collection
- 2ed536c9 - Allow resolving conflicts in MR controller
Toggle commit list