Skip to content
Snippets Groups Projects

Merge conflict resolution

Merged Sean McGivern requested to merge mc-ui into master

Conflict_resolution

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Reassigned to @DouweM

  • Author Contributor

    @DouweM do you want to take a look please? Known problems:

    1. Renames. libgit2 doesn't seem to detect these as conflicts.
    2. The constructor for a Conflict::File is crazy, this should be made simpler.
    3. 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
    4. 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?
    5. 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 😃

    Screen_Shot_2016-07-25_at_23.27.28

  • @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
  • @smcgivern

    Renames. libgit2 doesn't seem to detect these as conflicts.

    Laaame. Can we detect them ourselves? ancestor...ours and ancestor...theirs would both have a Diff::File with the same old_path, but different new_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 to cannot_be_merged.

    Edited by Douwe Maan
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)
  • 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'
  • 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)
  • 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
  • 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}}
  • 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}}}
  • 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}}
  • 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
  • 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? }
  • 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)
      • What type of object is merge_file? Right now it sounds like it will be a reference to an actual file
      • Could we use DiffRefs to wrap their and our ref (theirs == start, ours == head, base == ancestor)
      • Can we use a keyword arg for repository?
    • Author Contributor
      • It's the output of Rugged::Index#merge_file, which is just a hash. libgit2 calls this a git_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).
    • @smcgivern

      • 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
    • Please register or sign in to reply
  • 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('',
    • No text for match lines?

    • Never mind, already done later! Hmm, not when !section?

      Edited by Douwe Maan
    • Should match_line be reset after this?

    • Author Contributor

      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!

    • Yes, makes sense!

    • Please register or sign in to reply
  • 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)
  • 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} @@"
  • 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
  • 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")
  • 3 3 class Line
    4 4 attr_reader :type, :index, :old_pos, :new_pos
    5 5 attr_accessor :text
    6 attr_accessor :rich_text
  • 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)
  • 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?
  • 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
  • 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}"
  • Reassigned to @smcgivern

  • mentioned in issue #3963 (closed)

  • 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}"
    • Author Contributor

      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 😄

    • Please register or sign in to reply
  • Author Contributor

    @DouweM:

    Laaame. Can we detect them ourselves? ancestor...ours and ancestor...theirs would both have a Diff::File with the same old_path, but different new_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.

  • Author Contributor

    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.

    Screen_Shot_2016-07-26_at_19.15.12

    This merge request contains merge conflicts

    It's not possible to resolve conflicts in GitLab, please resolve these conflicts or merge this request manually.

  • Author Contributor

    @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.

  • Sean McGivern Added 2 commits:

    Added 2 commits:

    • d90a5e04 - Handle multiple merge conflict files in collection
    • 6e7fe7d9 - Allow resolving conflicts in MR controller
  • Author Contributor

    @DouweM @fatihacet I just pushed, there's now a resolution endpoint. Still to do for me in this MR:

    Edited by Sean McGivern
  • 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)

  • Author Contributor

    Also, @fatihacet, which of us wants to rebase / merge master? We could screenhero it at some point if we want to pair on it.

  • Sean McGivern Added 1 commit:

    Added 1 commit:

    • de1a6059 - Allow resolving conflicts in MR controller
  • Sean McGivern Added 1 commit:

    Added 1 commit:

    • 44250a4c - Allow resolving conflicts in MR controller
  • Sean McGivern Added 1 commit:

    Added 1 commit:

    • 2ed536c9 - Allow resolving conflicts in MR controller
  • Fatih Acet Added 1308 commits:

    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.
  • Sean McGivern Added 15 commits:

    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
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading