Merge pull request Shopify#1169 from powerhome/fix-corrupted-git-cach…

…es-exit-status-128

Fix corrupt git cache causes "exit status 128"

app/models/shipit/stack.rb

@@ -380,15 +380,15 @@ def merge_method
     delegate :git_url, to: :repository, prefix: :repo
 
     def base_path
-      Rails.root.join('data', 'stacks', repo_owner, repo_name, environment)
+      @base_path ||= Rails.root.join('data', 'stacks', repo_owner, repo_name, environment)
     end
 
     def deploys_path
-      base_path.join("deploys")
+      @deploys_path ||= base_path.join("deploys")
     end
 
     def git_path
-      base_path.join("git")
+      @git_path ||= base_path.join("git")
     end
 
     def acquire_git_cache_lock(timeout: 15, &block)

lib/shipit/stack_commands.rb

@@ -15,19 +15,25 @@ def env
 
     def fetch
       create_directories
-      if Dir.exist?(@stack.git_path)
+      if valid_git_repository?(@stack.git_path)
         git('fetch', 'origin', '--tags', @stack.branch, env: env, chdir: @stack.git_path)
       else
+        @stack.clear_git_cache!
         git_clone(@stack.repo_git_url, @stack.git_path, branch: @stack.branch, env: env, chdir: @stack.deploys_path)
       end
     end
 
     def fetched?(commit)
-      git_dir = File.join(@stack.git_path, '.git')
-      if Dir.exist?(git_dir)
+      if valid_git_repository?(@stack.git_path)
         git('rev-parse', '--quiet', '--verify', "#{commit.sha}^{commit}", env: env, chdir: @stack.git_path)
       else
-        Command.new('test', '-d', git_dir, env: env, chdir: @stack.deploys_path)
+        # When the stack's git cache is not valid, the commit is
+        # NOT fetched. To keep the interface of this method
+        # consistent, we must return a Shipit::Command whose #success?
+        # method returns false - has a non-zero exit status. We utilize
+        # the POSIX 'test' command with no arguments which should
+        # always have an exit status of 1.
+        Command.new('test', env: env, chdir: @stack.deploys_path)
       end
     end
 
@@ -71,6 +77,18 @@ def with_temporary_working_directory(commit: nil)
       end
     end
 
+    def valid_git_repository?(path)
+      path.exist? &&
+        !path.empty? &&
+        git_cmd_succeeds?(path)
+    end
+
+    def git_cmd_succeeds?(path)
+      git("rev-parse", "--git-dir", chdir: path)
+        .tap(&:run)
+        .success?
+    end
+
     def git_clone(url, path, branch: 'master', **kwargs)
       git('clone', *modern_git_args, '--recursive', '--branch', branch, url, path, **kwargs)
     end

test/unit/deploy_commands_test.rb

@@ -4,8 +4,9 @@
 module Shipit
   class DeployCommandsTest < ActiveSupport::TestCase
     def setup
-      @stack = shipit_stacks(:shipit)
       @deploy = shipit_deploys(:shipit_pending)
+      @stack = @deploy.stack
+      @stack.stubs(:clear_git_cache!)
       @commands = DeployCommands.new(@deploy)
       @deploy_spec = stub(
         dependencies_steps!: ['bundle install --some-args'],
@@ -21,35 +22,84 @@ def setup
     end
 
     test "#fetch calls git fetch if repository cache already exist" do
-      Dir.expects(:exist?).with(@stack.git_path).returns(true)
+      @stack.git_path.stubs(:exist?).returns(true)
+      @stack.git_path.stubs(:empty?).returns(false)
+
       command = @commands.fetch
+
       assert_equal %w(git fetch origin --tags master), command.args
     end
 
     test "#fetch calls git fetch in git_path directory if repository cache already exist" do
-      Dir.expects(:exist?).with(@stack.git_path).returns(true)
+      @stack.git_path.stubs(:exist?).returns(true)
+      @stack.git_path.stubs(:empty?).returns(false)
+
       command = @commands.fetch
+
       assert_equal @stack.git_path.to_s, command.chdir
     end
 
     test "#fetch calls git clone if repository cache do not exist" do
-      Dir.expects(:exist?).with(@stack.git_path).returns(false)
+      @stack.git_path.stubs(:exist?).returns(false)
+
       command = @commands.fetch
+
       expected = %W(git clone --single-branch --recursive --branch master #{@stack.repo_git_url} #{@stack.git_path})
       assert_equal expected, command.args.map(&:to_s)
     end
 
+    test "#fetch calls git clone if repository cache is empty" do
+      @stack.git_path.stubs(:exist?).returns(true)
+      @stack.git_path.stubs(:empty?).returns(true)
+
+      command = @commands.fetch
+
+      expected = %W(git clone --single-branch --recursive --branch master #{@stack.repo_git_url} #{@stack.git_path})
+      assert_equal expected, command.args
+    end
+
+    test "#fetch calls git clone if repository cache corrupt" do
+      @stack.git_path.stubs(:exist?).returns(true)
+      @stack.git_path.stubs(:empty?).returns(false)
+      StackCommands.any_instance.expects(:git_cmd_succeeds?)
+        .with(@stack.git_path)
+        .returns(false)
+
+      command = @commands.fetch
+
+      expected = %W(git clone --single-branch --recursive --branch master #{@stack.repo_git_url} #{@stack.git_path})
+      assert_equal expected, command.args
+    end
+
+    test "#fetch clears a corrupted git stash before cloning" do
+      @stack.expects(:clear_git_cache!)
+      @stack.git_path.stubs(:exist?).returns(true)
+      @stack.git_path.stubs(:empty?).returns(false)
+      StackCommands.any_instance.expects(:git_cmd_succeeds?)
+        .with(@stack.git_path)
+        .returns(false)
+
+      command = @commands.fetch
+
+      expected = %W(git clone --single-branch --recursive --branch master #{@stack.repo_git_url} #{@stack.git_path})
+      assert_equal expected, command.args
+    end
+
     test "#fetch does not use --single-branch if git is outdated" do
-      Dir.expects(:exist?).with(@stack.git_path).returns(false)
+      @stack.git_path.stubs(:exist?).returns(false)
       StackCommands.stubs(git_version: Gem::Version.new('1.7.2.30'))
+
       command = @commands.fetch
+
       expected = %W(git clone --recursive --branch master #{@stack.repo_git_url} #{@stack.git_path})
       assert_equal expected, command.args.map(&:to_s)
     end
 
     test "#fetch calls git fetch in base_path directory if repository cache do not exist" do
-      Dir.expects(:exist?).with(@stack.git_path).returns(false)
+      @stack.git_path.stubs(:exist?).returns(false)
+
       command = @commands.fetch
+
       assert_equal @stack.deploys_path.to_s, command.chdir
     end