Commit 0c2df939 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'notify-mr-upon-branch-presence-change' into 'master'

Add a system note and update relevant merge requests when a branch is deleted or re-added

If a branch is deleted with an open merge request, amended offline, and then pushed again,
GitLab doesn't bother to update the merge request even though the last commit ID and/or
code may have changed. This MR ensures that each push will update any relevant merge
requests and adds a system note if this happens as well.

The new messages look like:

![image](https://gitlab.com/gitlab-org/gitlab-ce/uploads/6581eea069cd8e485b7fa4187ed4c043/image.png)

Closes #2926 

See merge request !1601
parents bd3689e9 f726df26
...@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
v 8.1.0 (unreleased) v 8.1.0 (unreleased)
- Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu) - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu)
- Speed up load times of issue detail pages by roughly 1.5x - Speed up load times of issue detail pages by roughly 1.5x
- Add a system note and update relevant merge requests when a branch is deleted or re-added (Stan Hu)
- Make diff file view easier to use on mobile screens (Stan Hu) - Make diff file view easier to use on mobile screens (Stan Hu)
- Improved performance of finding users by username or Email address - Improved performance of finding users by username or Email address
- Fix bug where merge request comments created by API would not trigger notifications (Stan Hu) - Fix bug where merge request comments created by API would not trigger notifications (Stan Hu)
......
...@@ -163,7 +163,8 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -163,7 +163,8 @@ class MergeRequestDiff < ActiveRecord::Base
merge_request.fetch_ref merge_request.fetch_ref
# Get latest sha of branch from source project # Get latest sha of branch from source project
source_sha = merge_request.source_project.commit(source_branch).sha source_commit = merge_request.source_project.commit(source_branch)
source_sha = source_commit.try(:sha)
Gitlab::CompareResult.new( Gitlab::CompareResult.new(
Gitlab::Git::Compare.new( Gitlab::Git::Compare.new(
......
...@@ -480,6 +480,10 @@ class Repository ...@@ -480,6 +480,10 @@ class Repository
end end
end end
def merge_base(first_commit_id, second_commit_id)
rugged.merge_base(first_commit_id, second_commit_id)
end
def search_files(query, ref) def search_files(query, ref)
offset = 2 offset = 2
args = %W(git grep -i -n --before-context #{offset} --after-context #{offset} #{query} #{ref || root_ref}) args = %W(git grep -i -n --before-context #{offset} --after-context #{offset} #{query} #{ref || root_ref})
......
...@@ -49,10 +49,13 @@ class GitPushService ...@@ -49,10 +49,13 @@ class GitPushService
elsif push_to_existing_branch?(ref, oldrev) elsif push_to_existing_branch?(ref, oldrev)
# Collect data for this git push # Collect data for this git push
@push_commits = project.repository.commits_between(oldrev, newrev) @push_commits = project.repository.commits_between(oldrev, newrev)
project.update_merge_requests(oldrev, newrev, ref, @user)
process_commit_messages(ref) process_commit_messages(ref)
end end
# Update merge requests that may be affected by this push. A new branch
# could cause the last commit of a merge request to change.
project.update_merge_requests(oldrev, newrev, ref, @user)
@push_data = build_push_data(oldrev, newrev, ref) @push_data = build_push_data(oldrev, newrev, ref)
# If CI was disabled but .gitlab-ci.yml file was pushed # If CI was disabled but .gitlab-ci.yml file was pushed
......
...@@ -6,12 +6,20 @@ module MergeRequests ...@@ -6,12 +6,20 @@ module MergeRequests
@oldrev, @newrev = oldrev, newrev @oldrev, @newrev = oldrev, newrev
@branch_name = Gitlab::Git.ref_name(ref) @branch_name = Gitlab::Git.ref_name(ref)
@fork_merge_requests = @project.fork_merge_requests.opened @fork_merge_requests = @project.fork_merge_requests.opened
@commits = @project.repository.commits_between(oldrev, newrev) @commits = []
# Leave a system note if a branch were deleted/added
if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
comment_mr_branch_presence_changed
comment_mr_with_commits if @commits.present?
else
@commits = @project.repository.commits_between(oldrev, newrev)
comment_mr_with_commits
close_merge_requests close_merge_requests
end
reload_merge_requests reload_merge_requests
execute_mr_web_hooks execute_mr_web_hooks
comment_mr_with_commits
true true
end end
...@@ -31,7 +39,6 @@ module MergeRequests ...@@ -31,7 +39,6 @@ module MergeRequests
commit_ids.include?(merge_request.last_commit.id) commit_ids.include?(merge_request.last_commit.id)
end end
merge_requests.uniq.select(&:source_project).each do |merge_request| merge_requests.uniq.select(&:source_project).each do |merge_request|
MergeRequests::PostMergeService. MergeRequests::PostMergeService.
new(merge_request.target_project, @current_user). new(merge_request.target_project, @current_user).
...@@ -70,13 +77,38 @@ module MergeRequests ...@@ -70,13 +77,38 @@ module MergeRequests
end end
end end
# Add comment about branches being deleted or added to merge requests
def comment_mr_branch_presence_changed
presence = Gitlab::Git.blank_ref?(@oldrev) ? :add : :delete
merge_requests_for_source_branch.each do |merge_request|
last_commit = merge_request.last_commit
# Only look at changed commits in restore branch case
unless Gitlab::Git.blank_ref?(@newrev)
begin
# Since any number of commits could have been made to the restored branch,
# find the common root to see what has been added.
common_ref = @project.repository.merge_base(last_commit.id, @newrev)
# If the a commit no longer exists in this repo, gitlab_git throws
# a Rugged::OdbError. This is fixed in https://gitlab.com/gitlab-org/gitlab_git/merge_requests/52
@commits = @project.repository.commits_between(common_ref, @newrev) if common_ref
rescue
end
# Prevent system notes from seeing a blank SHA
@oldrev = nil
end
SystemNoteService.change_branch_presence(
merge_request, merge_request.project, @current_user,
:source, @branch_name, presence)
end
end
# Add comment about pushing new commits to merge requests # Add comment about pushing new commits to merge requests
def comment_mr_with_commits def comment_mr_with_commits
merge_requests = @project.origin_merge_requests.opened.where(source_branch: @branch_name).to_a merge_requests_for_source_branch.each do |merge_request|
merge_requests += @fork_merge_requests.where(source_branch: @branch_name).to_a
merge_requests = filter_merge_requests(merge_requests)
merge_requests.each do |merge_request|
mr_commit_ids = Set.new(merge_request.commits.map(&:id)) mr_commit_ids = Set.new(merge_request.commits.map(&:id))
new_commits, existing_commits = @commits.partition do |commit| new_commits, existing_commits = @commits.partition do |commit|
...@@ -91,14 +123,7 @@ module MergeRequests ...@@ -91,14 +123,7 @@ module MergeRequests
# Call merge request webhook with update branches # Call merge request webhook with update branches
def execute_mr_web_hooks def execute_mr_web_hooks
merge_requests = @project.origin_merge_requests.opened merge_requests_for_source_branch.each do |merge_request|
.where(source_branch: @branch_name)
.to_a
merge_requests += @fork_merge_requests.where(source_branch: @branch_name)
.to_a
merge_requests = filter_merge_requests(merge_requests)
merge_requests.each do |merge_request|
execute_hooks(merge_request, 'update') execute_hooks(merge_request, 'update')
end end
end end
...@@ -106,5 +131,13 @@ module MergeRequests ...@@ -106,5 +131,13 @@ module MergeRequests
def filter_merge_requests(merge_requests) def filter_merge_requests(merge_requests)
merge_requests.uniq.select(&:source_project) merge_requests.uniq.select(&:source_project)
end end
def merge_requests_for_source_branch
@source_merge_requests ||= begin
merge_requests = @project.origin_merge_requests.opened.where(source_branch: @branch_name).to_a
merge_requests += @fork_merge_requests.where(source_branch: @branch_name).to_a
filter_merge_requests(merge_requests)
end
end
end end
end end
...@@ -168,6 +168,31 @@ class SystemNoteService ...@@ -168,6 +168,31 @@ class SystemNoteService
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
# Called when a branch in Noteable is added or deleted
#
# noteable - Noteable object
# project - Project owning noteable
# author - User performing the change
# branch_type - :source or :target
# branch - branch name
# presence - :add or :delete
#
# Example Note text:
#
# "Restored target branch `feature`"
#
# Returns the created Note object
def self.change_branch_presence(noteable, project, author, branch_type, branch, presence)
verb =
if presence == :add
'restored'
else
'deleted'
end
body = "#{verb} #{branch_type.to_s} branch `#{branch}`".capitalize
create_note(noteable: noteable, project: project, author: author, note: body)
end
# Called when a Mentionable references a Noteable # Called when a Mentionable references a Noteable
# #
# noteable - Noteable object being referenced # noteable - Noteable object being referenced
......
...@@ -112,6 +112,14 @@ describe GitPushService do ...@@ -112,6 +112,14 @@ describe GitPushService do
it { expect(@event.project).to eq(project) } it { expect(@event.project).to eq(project) }
it { expect(@event.action).to eq(Event::PUSHED) } it { expect(@event.action).to eq(Event::PUSHED) }
it { expect(@event.data).to eq(service.push_data) } it { expect(@event.data).to eq(service.push_data) }
context "Updates merge requests" do
it "when pushing a new branch for the first time" do
expect(project).to receive(:update_merge_requests).
with(@blankrev, 'newrev', 'refs/heads/master', user)
service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master')
end
end
end end
describe "Web Hooks" do describe "Web Hooks" do
......
...@@ -106,6 +106,27 @@ describe MergeRequests::RefreshService do ...@@ -106,6 +106,27 @@ describe MergeRequests::RefreshService do
it { expect(@fork_merge_request.notes).to be_empty } it { expect(@fork_merge_request.notes).to be_empty }
end end
context 'push new branch that exists in a merge request' do
let(:refresh_service) { service.new(@fork_project, @user) }
it 'refreshes the merge request' do
expect(refresh_service).to receive(:execute_hooks).
with(@fork_merge_request, 'update')
allow_any_instance_of(Repository).to receive(:merge_base).and_return(@oldrev)
refresh_service.execute(Gitlab::Git::BLANK_SHA, @newrev, 'refs/heads/master')
reload_mrs
expect(@merge_request.notes).to be_empty
expect(@merge_request).to be_open
notes = @fork_merge_request.notes.reorder(:created_at).map(&:note)
expect(notes[0]).to include('Restored source branch `master`')
expect(notes[1]).to include('Added 4 commits')
expect(@fork_merge_request).to be_open
end
end
def reload_mrs def reload_mrs
@merge_request.reload @merge_request.reload
@fork_merge_request.reload @fork_merge_request.reload
......
...@@ -242,6 +242,18 @@ describe SystemNoteService do ...@@ -242,6 +242,18 @@ describe SystemNoteService do
end end
end end
describe '.change_branch_presence' do
subject { described_class.change_branch_presence(noteable, project, author, :source, 'feature', :delete) }
it_behaves_like 'a system note'
context 'when source branch deleted' do
it 'sets the note text' do
expect(subject.note).to eq "Deleted source branch `feature`"
end
end
end
describe '.cross_reference' do describe '.cross_reference' do
subject { described_class.cross_reference(noteable, mentioner, author) } subject { described_class.cross_reference(noteable, mentioner, author) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment