MR on fork: Some cleanup, test updates

-The forked merge request test now tests it's componenets again, and seems to work every time (did this by reordering the branch updates so their is more time for update_branches to run) -- this could still technically fail, but after several runs it doesn't seem to.
-Removed todo in merge_request, pushed wrapping of grit down to the satellite
-updated action test to check flock status, made it nolonger pending
-moved all logging on failure to helper method in satellite
GITLAB-592

Change-Id: If0554ca35eedc3d3e8461f7d93d4b3939fa2cd75

app/assets/stylesheets/common.scss

@@ -415,6 +415,7 @@ img.emoji {
   @extend .light-well;
   @extend .light;
   margin-bottom: 10px;
+}
 
 .label-project {
   @include border-radius(4px);

app/models/merge_request.rb

@@ -26,12 +26,10 @@ class MergeRequest < ActiveRecord::Base
 
   include Issuable
 
-  belongs_to :target_project,:foreign_key => :target_project_id, class_name: "Project"
-  belongs_to :source_project, :foreign_key => :source_project_id,class_name: "Project"
+  belongs_to :target_project, :foreign_key => :target_project_id, class_name: "Project"
+  belongs_to :source_project, :foreign_key => :source_project_id, class_name: "Project"
 
-  BROKEN_DIFF = "--broken-diff"
-
-  attr_accessible :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch,  :milestone_id,:author_id_of_changes, :state_event
+  attr_accessible :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id, :author_id_of_changes, :state_event
 
 
   attr_accessor :should_remove_source_branch
@@ -87,8 +85,8 @@ class MergeRequest < ActiveRecord::Base
   validates :target_branch, presence: true
   validate :validate_branches
 
-  scope :of_group, ->(group) { where("source_project_id in (:group_project_ids) OR target_project_id in (:group_project_ids)",group_project_ids:group.project_ids) }
-  scope :of_user_team, ->(team) { where("(source_project_id in (:team_project_ids) OR target_project_id in (:team_project_ids) AND assignee_id in (:team_member_ids))",team_project_ids:team.project_ids,team_member_ids:team.member_ids) }
+  scope :of_group, ->(group) { where("source_project_id in (:group_project_ids) OR target_project_id in (:group_project_ids)", group_project_ids: group.project_ids) }
+  scope :of_user_team, ->(team) { where("(source_project_id in (:team_project_ids) OR target_project_id in (:team_project_ids) AND assignee_id in (:team_member_ids))", team_project_ids: team.project_ids, team_member_ids: team.member_ids) }
   scope :opened, -> { with_state(:opened) }
   scope :closed, -> { with_state(:closed) }
   scope :merged, -> { with_state(:merged) }
@@ -151,13 +149,8 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def unmerged_diffs
-    #TODO:[IA-8] this needs to be handled better -- logged etc
     diffs = Gitlab::Satellite::MergeAction.new(author, self).diffs_between_satellite
-     if diffs
-      diffs = diffs.map { |diff| Gitlab::Git::Diff.new(diff) }
-     else
-       diffs = []
-     end
+    diffs ||= []
     diffs
   end
 
@@ -192,8 +185,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def unmerged_commits
-    commits = Gitlab::Satellite::MergeAction.new(self.author,self).commits_between
-    commits = commits.map{ |commit| Gitlab::Git::Commit.new(commit, nil) }
+    commits = Gitlab::Satellite::MergeAction.new(self.author, self).commits_between
     if commits.present?
       commits = Commit.decorate(commits).
           sort_by(&:created_at).
@@ -222,6 +214,7 @@ class MergeRequest < ActiveRecord::Base
     commit_ids = commits.map(&:id)
     Note.where("(noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR (noteable_type = 'Commit' AND commit_id IN (:commit_ids))", mr_id: id, commit_ids: commit_ids)
   end
+
   # Returns the raw diff for this merge request
   #
   # see "git diff"

features/steps/project/project_forked_merge_requests.rb

@@ -5,6 +5,7 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps
   include SharedPaths
 
 
+
   Given 'I am a member of project "Shop"' do
     @project = Project.find_by_name "Shop"
     @project ||= create(:project_with_code, name: "Shop")
@@ -34,22 +35,42 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps
     current_path.should == project_merge_request_path(@project, @merge_request)
     @merge_request.title.should == "Merge Request On Forked Project"
     @merge_request.source_project.should == @forked_project
+    @merge_request.source_branch.should == "master"
+    @merge_request.target_branch.should == "stable"
+    page.should have_content @forked_project.path_with_namespace
+    page.should have_content @project.path_with_namespace
+    page.should have_content @merge_request.source_branch
+    page.should have_content @merge_request.target_branch
   end
 
   And 'I fill out a "Merge Request On Forked Project" merge request' do
+    #The ordering here is a bit whacky on purpose:
+    #Select the target right away, to give update_branches time to run and clean up the target_branches
+    find(:select, "merge_request_target_project_id", {}).value.should == @forked_project.id.to_s
+    select @project.path_with_namespace, from: "merge_request_target_project_id"
+
+
     fill_in "merge_request_title", with: "Merge Request On Forked Project"
     find(:select, "merge_request_source_project_id", {}).value.should == @forked_project.id.to_s
-    find(:select, "merge_request_target_project_id", {}).value.should == @forked_project.id.to_s
 
-    select @project.path_with_namespace, from: "merge_request_target_project_id"
     find(:select, "merge_request_target_project_id", {}).value.should == @project.id.to_s
+
+    #Ensure the option exists in the select
+    find(:select, "merge_request_source_branch", {}).should have_content "master"
     select "master", from: "merge_request_source_branch"
+    #Ensure the option is selected
     find(:select, "merge_request_source_branch", {}).value.should have_content "master"
-    #Force the page to wait until the javascript finishes, so the stable option shows up
-    select "stable", from: "merge_request_target_branch"
+    verify_commit_link(".mr_source_commit",@forked_project)
+
+
+    #This could fail if the javascript hasn't run yet, there is a timing issue here -- this is why we do the select at the top
+    #Ensure the option exists in the select
     find(:select, "merge_request_target_branch", {}).should have_content "stable"
+    #We must give apparently lots of time for update branches to finish
 
-    verify_commit_link(".mr_source_commit",@forked_project)
+    (find(:select, "merge_request_target_branch", {}).find(:option, "stable",{}).select_option).should be_true
+    #Ensure the option is selected
+    find(:select, "merge_request_target_branch", {}).value.should have_content "stable"
     verify_commit_link(".mr_target_commit",@project)
   end
 
@@ -126,14 +147,8 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps
     find("#merge_request_target_project_id").value.should == @project.id.to_s
     find("#merge_request_source_branch").value.should have_content "master"
     verify_commit_link(".mr_source_commit",@forked_project)
-    #TODO[IA-08] reienstate these -- not sure why they started repeatedly breaking
-    #sleep 3
-    #puts "Source text:#{find("#merge_request_source_branch").text}"
-    #puts "Source value:#{find("#merge_request_source_branch").value}"
-    #puts "Target text:#{find("#merge_request_target_branch").text}"
-    #puts "Target value:#{find("#merge_request_target_branch").value}"
-    #find("#merge_request_target_branch").value.should have_content "stable"
-    #verify_commit_link(".mr_target_commit",@project)
+    find("#merge_request_target_branch").value.should have_content "stable"
+    verify_commit_link(".mr_target_commit",@project)
   end
 
 
@@ -144,16 +159,8 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps
   #Verify a link is generated against the correct project
   def verify_commit_link(container_div, container_project)
     #This should force a wait for the javascript to execute
-    #puts "text: #{find(:div,container_div).text}"
     find(:div,container_div).should have_css ".browse_code_link_holder"
     find(:div,container_div).find(".commit_short_id")['href'].should have_content "#{container_project.path_with_namespace}/commit"
-    #commit_links = commit.all("a").map(&:text)
-    #links = commit_links.collect {|e|commit.find(:link,e)['href']}
-    #links.size.should == 4
-    #links[0].should have_content "#{container_project.path_with_namespace}/tree"
-    #links[1].should have_content "#{container_project.path_with_namespace}/commit"
-    #links[3].should have_content "#{container_project.path_with_namespace}/commit"
-
   end
 
 end

lib/gitlab/satellite/action.rb

 module Gitlab
   module Satellite
     class Action
-      DEFAULT_OPTIONS = { git_timeout: 30.seconds}
+      DEFAULT_OPTIONS = {git_timeout: 30.seconds}
 
       attr_accessor :options, :project, :user
 
@@ -25,11 +25,9 @@ module Gitlab
           end
         end
       rescue Errno::ENOMEM => ex
-        Gitlab::GitLogger.error(ex.message)
-        return false
+        return handle_exception(ex)
       rescue Grit::Git::GitTimeout => ex
-        Gitlab::GitLogger.error(ex.message)
-        return false
+        return handle_exception(ex)
       ensure
         Gitlab::ShellEnv.reset_env
       end
@@ -48,6 +46,11 @@ module Gitlab
       def default_options(options = {})
         {raise: true, timeout: true}.merge(options)
       end
+
+      def handle_exception(exception)
+        Gitlab::GitLogger.error(exception.message)
+        false
+      end
     end
   end
 end

lib/gitlab/satellite/merge_action.rb

@@ -41,8 +41,7 @@ module Gitlab
           end
         end
       rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
+        handle_exception(ex)
       end
 
 
@@ -61,8 +60,7 @@ module Gitlab
           return diff
         end
       rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
+        handle_exception(ex)
       end
 
       # Only show what is new in the source branch compared to the target branch, not the other way around.
@@ -81,11 +79,11 @@ module Gitlab
             #this method doesn't take default options
             diffs = merge_repo.diff(common_commit, "#{merge_request.source_branch}")
           end
+          diffs = diffs.map { |diff| Gitlab::Git::Diff.new(diff) }
           return diffs
         end
       rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
+        handle_exception(ex)
       end
 
       # Get commit as an email patch
@@ -101,8 +99,7 @@ module Gitlab
           return patch
         end
       rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
+        handle_exception(ex)
       end
 
       # Retrieve an array of commits between the source and the target
@@ -115,12 +112,12 @@ module Gitlab
           else
             commits = merge_repo.commits_between("#{merge_request.target_branch}", "#{merge_request.source_branch}")
           end
+          commits = commits.map { |commit| Gitlab::Git::Commit.new(commit, nil) }
           return commits
 
         end
       rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
+        handle_exception(ex)
       end
 
       private
@@ -142,8 +139,7 @@ module Gitlab
           repo.git.pull(default_options({no_ff: true}), 'origin', merge_request.source_branch)
         end
       rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
+        handle_exception(ex)
       end
 
       # Assumes a satellite exists that is a fresh clone of the projects repo, prepares satellite for merges, diffs etc
@@ -159,11 +155,9 @@ module Gitlab
           repo.git.checkout(default_options, "#{merge_request.target_branch}")
         end
       rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
+        handle_exception(ex)
       end
 
-
     end
   end
 end

spec/lib/gitlab/satellite/action_spec.rb

@@ -78,52 +78,43 @@ describe 'Gitlab::Satellite::Action' do
     end
 
     it 'should be able to use the satellite after locking' do
-      pending "can't test this, doesn't seem to be a way to the the flock status on a file, throwing piles of processes at it seems lousy too"
       repo = project.satellite.repo
-      first_call = false
-
-      (File.exists? project.satellite.lock_file).should be_false
-
-      test_file = ->(called) {
-        File.exists?(project.satellite.lock_file).should be_true
-        called.should be_true
-        File.readlines.should == "some test code"
-        File.truncate(project.satellite.lock, 0)
-        File.readlines.should == ""
-      }
+      called = false
 
-      write_file = ->(called, checker) {
-        if (File.exists?(project.satellite.lock_file))
-          file = File.open(project.satellite.lock, '+w')
-          file.write("some test code")
-          file.close
-          checker.call(called)
+      # Set base assumptions
+      if File.exists? project.satellite.lock_file
+        FileLockStatusChecker.new(project.satellite.lock_file).flocked?.should be_false
       end
-      }
-
 
       satellite_action = Gitlab::Satellite::Action.new(user, project)
       satellite_action.send(:in_locked_and_timed_satellite) do |sat_repo|
-        write_file.call(first_call, test_file)
-        first_call = true
+        called = true
         repo.should == sat_repo
         (File.exists? project.satellite.lock_file).should be_true
-
+        FileLockStatusChecker.new(project.satellite.lock_file).flocked?.should be_true
       end
 
-      first_call.should be_true
-      puts File.stat(project.satellite.lock_file).inspect
+      called.should be_true
+      FileLockStatusChecker.new(project.satellite.lock_file).flocked?.should be_false
 
-      second_call = false
-      satellite_action.send(:in_locked_and_timed_satellite) do |sat_repo|
-        write_file.call(second_call, test_file)
-        second_call = true
-        repo.should == sat_repo
-        (File.exists? project.satellite.lock_file).should be_true
     end
 
-      second_call.should be_true
-      (File.exists? project.satellite.lock_file).should be_true
+    class FileLockStatusChecker < File
+      def flocked? &block
+        status = flock LOCK_EX|LOCK_NB
+        case status
+          when false
+            return true
+          when 0
+            begin
+              block ? block.call : false
+            ensure
+              flock LOCK_UN
+            end
+          else
+            raise SystemCallError, status
+        end
+      end
     end
 
   end

spec/lib/gitlab/satellite/merge_action_spec.rb

@@ -18,19 +18,23 @@ describe 'Gitlab::Satellite::MergeAction' do
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
   let(:merge_request_fork) { create(:merge_request) }
   describe '#commits_between' do
+    def verify_commits(commits, first_commit_sha, last_commit_sha)
+      commits.each { |commit| commit.class.should == Gitlab::Git::Commit }
+      commits.first.id.should == first_commit_sha
+      commits.last.id.should == last_commit_sha
+    end
+
     context 'on fork' do
       it 'should get proper commits between' do
         merge_request_fork.target_branch = @one_after_stable[0]
         merge_request_fork.source_branch = @master[0]
         commits = Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request_fork).commits_between
-        commits.first.id.should == @one_after_stable[1]
-        commits.last.id.should == @master[1]
+        verify_commits(commits, @one_after_stable[1], @master[1])
 
         merge_request_fork.target_branch = @wiki_branch[0]
         merge_request_fork.source_branch = @master[0]
         commits = Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request_fork).commits_between
-        commits.first.id.should == @wiki_branch[1]
-        commits.last.id.should == @master[1]
+        verify_commits(commits, @wiki_branch[1], @master[1])
       end
     end
 
@@ -39,14 +43,12 @@ describe 'Gitlab::Satellite::MergeAction' do
         merge_request.target_branch = @one_after_stable[0]
         merge_request.source_branch = @master[0]
         commits = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).commits_between
-        commits.first.id.should == @one_after_stable[1]
-        commits.last.id.should == @master[1]
+        verify_commits(commits, @one_after_stable[1], @master[1])
 
         merge_request.target_branch = @wiki_branch[0]
         merge_request.source_branch = @master[0]
         commits = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).commits_between
-        commits.first.id.should == @wiki_branch[1]
-        commits.last.id.should == @master[1]
+        verify_commits(commits, @wiki_branch[1], @master[1])
       end
     end
   end
@@ -81,8 +83,12 @@ describe 'Gitlab::Satellite::MergeAction' do
       diff_count = diff.scan('diff --git').size
       diff_count.should >= 1
       diffs.size.should == diff_count
-      diffs.each {|a_diff| (diff.include? a_diff.diff).should be_true}
+      diffs.each do |a_diff|
+        a_diff.class.should == Gitlab::Git::Diff
+        (diff.include? a_diff.diff).should be_true
       end
+    end
+
     context 'on fork' do
       it 'should get proper diffs' do
         merge_request_fork.target_branch = @close_commit1[0]
@@ -93,7 +99,7 @@ describe 'Gitlab::Satellite::MergeAction' do
         merge_request_fork.source_branch = @master[0]
         diff = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request_fork).diff_in_satellite
 
-        is_a_matching_diff(diff,diffs)
+        is_a_matching_diff(diff, diffs)
       end
     end
 
@@ -108,7 +114,7 @@ describe 'Gitlab::Satellite::MergeAction' do
         merge_request.source_branch = @master[0]
         diff = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).diff_in_satellite
 
-        is_a_matching_diff(diff,diffs)
+        is_a_matching_diff(diff, diffs)
       end
     end
   end