Commit 5ec1ad8b2375bdce7a820df1be3dc67b18ad2bd0
Exists in
master
and in
4 other branches
Merge pull request #1743 from riyad/fix-merging-bugs
Fix merging bugs
Showing
2 changed files
with
74 additions
and
34 deletions
Show diff stats
app/models/merge_request.rb
... | ... | @@ -135,7 +135,8 @@ class MergeRequest < ActiveRecord::Base |
135 | 135 | end |
136 | 136 | |
137 | 137 | def mark_as_unmergable |
138 | - self.update_attributes state: CANNOT_BE_MERGED | |
138 | + self.state = CANNOT_BE_MERGED | |
139 | + self.save | |
139 | 140 | end |
140 | 141 | |
141 | 142 | def reloaded_commits |
... | ... | @@ -166,7 +167,7 @@ class MergeRequest < ActiveRecord::Base |
166 | 167 | end |
167 | 168 | |
168 | 169 | def automerge!(current_user) |
169 | - if Gitlab::Merge.new(self, current_user).merge && self.unmerged_commits.empty? | |
170 | + if Gitlab::Merge.new(self, current_user).merge! && self.unmerged_commits.empty? | |
170 | 171 | self.merge!(current_user.id) |
171 | 172 | true |
172 | 173 | end | ... | ... |
lib/gitlab/merge.rb
1 | 1 | module Gitlab |
2 | 2 | class Merge |
3 | - attr_accessor :project, :merge_request, :user | |
3 | + attr_accessor :merge_request, :project, :user | |
4 | 4 | |
5 | 5 | def initialize(merge_request, user) |
6 | - self.user = user | |
7 | - self.merge_request = merge_request | |
8 | - self.project = merge_request.project | |
6 | + @merge_request = merge_request | |
7 | + @project = merge_request.project | |
8 | + @user = user | |
9 | 9 | end |
10 | 10 | |
11 | 11 | def can_be_merged? |
12 | - result = false | |
13 | - process do |repo, output| | |
14 | - result = !(output =~ /CONFLICT/) | |
12 | + in_locked_and_timed_satellite do |merge_repo| | |
13 | + merge_in_satellite!(merge_repo) | |
15 | 14 | end |
16 | - result | |
17 | 15 | end |
18 | 16 | |
19 | - def merge | |
20 | - process do |repo, output| | |
21 | - if output =~ /CONFLICT/ | |
22 | - false | |
23 | - else | |
24 | - !!repo.git.push({}, "origin", merge_request.target_branch) | |
17 | + # Merges the source branch into the target branch in the satellite and | |
18 | + # pushes it back to Gitolite. | |
19 | + # It also removes the source branch if requested in the merge request. | |
20 | + # | |
21 | + # Returns false if the merge produced conflicts | |
22 | + # Returns false if pushing from the satallite to Gitolite failed or was rejected | |
23 | + # Returns true otherwise | |
24 | + def merge! | |
25 | + in_locked_and_timed_satellite do |merge_repo| | |
26 | + if merge_in_satellite!(merge_repo) | |
27 | + # push merge back to Gitolite | |
28 | + # will raise CommandFailed when push fails | |
29 | + merge_repo.git.push({raise: true}, :origin, merge_request.target_branch) | |
30 | + | |
31 | + # remove source branch | |
32 | + if merge_request.should_remove_source_branch && !project.root_ref?(merge_request.source_branch) | |
33 | + # will raise CommandFailed when push fails | |
34 | + merge_repo.git.push({raise: true}, :origin, ":#{merge_request.source_branch}") | |
35 | + end | |
36 | + | |
37 | + # merge, push and branch removal successful | |
38 | + true | |
25 | 39 | end |
26 | 40 | end |
41 | + rescue Grit::Git::CommandFailed | |
42 | + false | |
27 | 43 | end |
28 | 44 | |
29 | - def process | |
45 | + private | |
46 | + | |
47 | + # * Sets a 30s timeout for Git | |
48 | + # * Locks the satellite repo | |
49 | + # * Yields the prepared satallite repo | |
50 | + def in_locked_and_timed_satellite | |
30 | 51 | Grit::Git.with_timeout(30.seconds) do |
31 | 52 | lock_file = Rails.root.join("tmp", "#{project.path}.lock") |
32 | 53 | |
... | ... | @@ -37,29 +58,47 @@ module Gitlab |
37 | 58 | raise "Satellite doesn't exist" |
38 | 59 | end |
39 | 60 | |
40 | - project.satellite.clear | |
41 | - | |
42 | 61 | Dir.chdir(project.satellite.path) do |
43 | - merge_repo = Grit::Repo.new('.') | |
44 | - merge_repo.git.sh "git reset --hard" | |
45 | - merge_repo.git.sh "git fetch origin" | |
46 | - merge_repo.git.sh "git config user.name \"#{user.name}\"" | |
47 | - merge_repo.git.sh "git config user.email \"#{user.email}\"" | |
48 | - merge_repo.git.sh "git checkout -b #{merge_request.target_branch} origin/#{merge_request.target_branch}" | |
49 | - output = merge_repo.git.pull({}, "--no-ff", "origin", merge_request.source_branch) | |
50 | - | |
51 | - #remove source-branch | |
52 | - if merge_request.should_remove_source_branch && !project.root_ref?(merge_request.source_branch) | |
53 | - merge_repo.git.sh "git push origin :#{merge_request.source_branch}" | |
54 | - end | |
55 | - | |
56 | - yield(merge_repo, output) | |
62 | + repo = Grit::Repo.new('.') | |
63 | + | |
64 | + return yield repo | |
57 | 65 | end |
58 | 66 | end |
59 | 67 | end |
60 | - | |
61 | 68 | rescue Grit::Git::GitTimeout |
62 | 69 | return false |
63 | 70 | end |
71 | + | |
72 | + # Merges the source_branch into the target_branch in the satellite. | |
73 | + # | |
74 | + # Note: it will clear out the satellite before doing anything | |
75 | + # | |
76 | + # Returns false if the merge produced conflicts | |
77 | + # Returns true otherwise | |
78 | + def merge_in_satellite!(repo) | |
79 | + prepare_satelite!(repo) | |
80 | + | |
81 | + # create target branch in satellite at the corresponding commit from Gitolite | |
82 | + repo.git.checkout({b: true}, merge_request.target_branch, "origin/#{merge_request.target_branch}") | |
83 | + | |
84 | + # merge the source branch from Gitolite into the satellite | |
85 | + # will raise CommandFailed when merge fails | |
86 | + repo.git.pull({no_ff: true, raise: true}, :origin, merge_request.source_branch) | |
87 | + rescue Grit::Git::CommandFailed | |
88 | + false | |
89 | + end | |
90 | + | |
91 | + # * Clears the satellite | |
92 | + # * Updates the satellite from Gitolite | |
93 | + # * Sets up Git variables for the user | |
94 | + def prepare_satelite!(repo) | |
95 | + project.satellite.clear | |
96 | + | |
97 | + repo.git.reset(hard: true) | |
98 | + repo.git.fetch({}, :origin) | |
99 | + | |
100 | + repo.git.config({}, "user.name", user.name) | |
101 | + repo.git.config({}, "user.email", user.email) | |
102 | + end | |
64 | 103 | end |
65 | 104 | end | ... | ... |