From 725380c866d9efb2a8e0555aad02d1ff00b200de Mon Sep 17 00:00:00 2001
From: Elton Gao <gyfelton@gmail.com>
Date: Wed, 8 Mar 2023 10:17:33 -0500
Subject: [PATCH 01/20] Fix stuck at handling extra long stderr during clone or
 remote update

Also add log when cleaning cache
---
 lib/git-fastclone.rb | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index 1fd0f7c..cdfeced 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -73,6 +73,8 @@ class Runner
 
     DEFAULT_GIT_ALLOW_PROTOCOL = 'file:git:http:https:ssh'
 
+    NO_STDOUT_ERR = ' > /dev/null 2>&1 '
+
     attr_accessor :reference_dir, :prefetch_submodules, :reference_updated, :reference_mutex,
                   :options, :logger, :abs_clone_path, :using_local_repo, :verbose, :color,
                   :flock_timeout_secs
@@ -343,17 +345,18 @@ def prefetch(submodule_file)
     # that this repo has been updated on this run of fastclone
     def store_updated_repo(url, mirror, repo_name, fail_hard)
       unless Dir.exist?(mirror)
-        Terrapin::CommandLine.new('git clone', '--mirror :url :mirror')
+        Terrapin::CommandLine.new('git clone', '--mirror :url :mirror' + NO_STDOUT_ERR)
                              .run(url: url.to_s, mirror: mirror.to_s)
       end
 
-      Terrapin::CommandLine.new('cd', ':path; git remote update --prune').run(path: mirror)
+      Terrapin::CommandLine.new('cd', ':path; git remote update --prune' + NO_STDOUT_ERR).run(path: mirror)
 
       reference_updated[repo_name] = true
     rescue Terrapin::ExitStatusError => e
       # To avoid corruption of the cache, if we failed to update or check out we remove
       # the cache directory entirely. This may cause the current clone to fail, but if the
       # underlying error from git is transient it will not affect future clones.
+      puts "Removing the fastclone cache."
       FileUtils.remove_entry_secure(mirror, force: true)
       raise e if fail_hard
     end

From f0ada43acdb5f14b0021326826d03e1f2a3f277f Mon Sep 17 00:00:00 2001
From: Elton Gao <gyfelton@gmail.com>
Date: Wed, 8 Mar 2023 10:23:28 -0500
Subject: [PATCH 02/20] Update git_fastclone_runner_spec.rb

---
 spec/git_fastclone_runner_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb
index 11dc884..6d764bc 100644
--- a/spec/git_fastclone_runner_spec.rb
+++ b/spec/git_fastclone_runner_spec.rb
@@ -391,8 +391,8 @@ def try_with_git_mirror(responses, results)
 
     def clone_cmds
       [
-        ['git clone', '--mirror :url :mirror'],
-        ['cd', ':path; git remote update --prune']
+        ['git clone', '--mirror :url :mirror > /dev/null 2>&1 '],
+        ['cd', ':path; git remote update --prune > /dev/null 2>&1 ']
       ]
     end
 

From cbaa7db7637a01bbf714b98c527e9dbb01c69a94 Mon Sep 17 00:00:00 2001
From: Elton Gao <gyfelton@gmail.com>
Date: Wed, 8 Mar 2023 10:23:59 -0500
Subject: [PATCH 03/20] Update version.rb

---
 lib/git-fastclone/version.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/git-fastclone/version.rb b/lib/git-fastclone/version.rb
index bec154a..83dbd7c 100644
--- a/lib/git-fastclone/version.rb
+++ b/lib/git-fastclone/version.rb
@@ -2,5 +2,5 @@
 
 # Version string for git-fastclone
 module GitFastCloneVersion
-  VERSION = '1.3.3'
+  VERSION = '1.3.4'
 end

From f307043848e5e841baca9dcaadb2b763fa2f0d8d Mon Sep 17 00:00:00 2001
From: Elton Gao <gyfelton@gmail.com>
Date: Wed, 8 Mar 2023 10:24:07 -0500
Subject: [PATCH 04/20] Update Gemfile.lock

---
 Gemfile.lock | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index 7c584e0..34cdf59 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    git-fastclone (1.3.3)
+    git-fastclone (1.3.4)
       colorize
       terrapin (~> 0.6.0)
 

From 3fdc53db87f676a0967570ee0af75c9977ca9ed8 Mon Sep 17 00:00:00 2001
From: Elton Gao <gyfelton@gmail.com>
Date: Wed, 8 Mar 2023 11:13:47 -0500
Subject: [PATCH 05/20] Update git-fastclone.rb

---
 lib/git-fastclone.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index cdfeced..23fba73 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -356,7 +356,7 @@ def store_updated_repo(url, mirror, repo_name, fail_hard)
       # To avoid corruption of the cache, if we failed to update or check out we remove
       # the cache directory entirely. This may cause the current clone to fail, but if the
       # underlying error from git is transient it will not affect future clones.
-      puts "Removing the fastclone cache."
+      puts '[WARN] Removing the fastclone cache.'
       FileUtils.remove_entry_secure(mirror, force: true)
       raise e if fail_hard
     end

From 49a2be693e1169d57a3a57f87b2597bd51a22c96 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Wed, 8 Mar 2023 11:27:32 -0500
Subject: [PATCH 06/20] Update git-fastclone.rb

---
 lib/git-fastclone.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index 23fba73..c6c200b 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -345,11 +345,11 @@ def prefetch(submodule_file)
     # that this repo has been updated on this run of fastclone
     def store_updated_repo(url, mirror, repo_name, fail_hard)
       unless Dir.exist?(mirror)
-        Terrapin::CommandLine.new('git clone', '--mirror :url :mirror' + NO_STDOUT_ERR)
+        Terrapin::CommandLine.new('git clone', "--mirror :url :mirror#{NO_STDOUT_ERR}")
                              .run(url: url.to_s, mirror: mirror.to_s)
       end
 
-      Terrapin::CommandLine.new('cd', ':path; git remote update --prune' + NO_STDOUT_ERR).run(path: mirror)
+      Terrapin::CommandLine.new('cd', ":path; git remote update --prune#{NO_STDOUT_ERR}").run(path: mirror)
 
       reference_updated[repo_name] = true
     rescue Terrapin::ExitStatusError => e

From 0a09e2509d1ea92a497dd46bef141f6c33978a71 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Thu, 9 Mar 2023 16:18:46 -0500
Subject: [PATCH 07/20] Use build_execution wrapper for Open3

Also use the same retriable logic around another git clone operation
---
 Gemfile.lock                 |   2 +-
 lib/git-fastclone.rb         |  79 ++++++-------
 lib/git-fastclone/version.rb |   2 +-
 lib/runner_execution.rb      | 217 +++++++++++++++++++++++++++++++++++
 4 files changed, 257 insertions(+), 43 deletions(-)
 create mode 100644 lib/runner_execution.rb

diff --git a/Gemfile.lock b/Gemfile.lock
index 34cdf59..b4103be 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    git-fastclone (1.3.4)
+    git-fastclone (1.4.0)
       colorize
       terrapin (~> 0.6.0)
 
diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index c6c200b..a01a9d9 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -16,9 +16,8 @@
 
 require 'optparse'
 require 'fileutils'
-require 'logger'
-require 'terrapin'
 require 'timeout'
+require_relative 'runner_execution'
 
 # Contains helper module UrlHelper and execution class GitFastClone::Runner
 module GitFastClone
@@ -68,6 +67,7 @@ class Runner
     require 'colorize'
 
     include GitFastClone::UrlHelper
+    include RunnerExecution
 
     DEFAULT_REFERENCE_REPO_DIR = '/var/tmp/git-fastclone/reference'
 
@@ -76,7 +76,7 @@ class Runner
     NO_STDOUT_ERR = ' > /dev/null 2>&1 '
 
     attr_accessor :reference_dir, :prefetch_submodules, :reference_updated, :reference_mutex,
-                  :options, :logger, :abs_clone_path, :using_local_repo, :verbose, :color,
+                  :options, :abs_clone_path, :using_local_repo, :verbose, :color,
                   :flock_timeout_secs
 
     def initialize
@@ -96,8 +96,6 @@ def initialize
 
       self.options = {}
 
-      self.logger = nil # Only set in verbose mode
-
       self.abs_clone_path = Dir.pwd
 
       self.using_local_repo = false
@@ -121,8 +119,7 @@ def run
       end
 
       puts "Cloning #{path_from_git_url(url)} to #{File.join(abs_clone_path, path)}"
-      Terrapin::CommandLine.environment['GIT_ALLOW_PROTOCOL'] =
-        ENV['GIT_ALLOW_PROTOCOL'] || DEFAULT_GIT_ALLOW_PROTOCOL
+      ENV['GIT_ALLOW_PROTOCOL'] ||= DEFAULT_GIT_ALLOW_PROTOCOL
       clone(url, options[:branch], path, options[:config])
     end
 
@@ -139,11 +136,6 @@ def parse_options
 
         opts.on('-v', '--verbose', 'Verbose mode') do
           self.verbose = true
-          self.logger = Logger.new($stdout)
-          logger.formatter = proc do |_severity, _datetime, _progname, msg|
-            "#{msg}\n"
-          end
-          Terrapin::CommandLine.logger = logger
         end
 
         opts.on('-c', '--color', 'Display colored output') do
@@ -219,19 +211,16 @@ def clone(url, rev, src_dir, config)
       with_git_mirror(url) do |mirror, attempt_number|
         clear_clone_dest_if_needed(attempt_number, clone_dest)
 
-        clone_command = '--quiet --reference :mirror :url :path'
-        clone_command += ' --config :config' unless config.nil?
-        Terrapin::CommandLine.new('git clone', clone_command)
-                             .run(mirror: mirror.to_s,
-                                  url: url.to_s,
-                                  path: clone_dest,
-                                  config: config.to_s)
+        clone_commands = ['git', 'clone', verbose ? '--verbose' : '--quiet']
+        clone_commands << "--reference" << mirror.to_s << url.to_s << clone_dest
+        clone_commands << "--config" << config.to_s unless config.nil?
+        fail_pipe_on_error(clone_commands, quiet: !verbose)
       end
 
       # Only checkout if we're changing branches to a non-default branch
       if rev
         Dir.chdir(File.join(abs_clone_path, src_dir)) do
-          Terrapin::CommandLine.new('git checkout', '--quiet :rev').run(rev: rev.to_s)
+          fail_pipe_on_error(['git', 'checkout', '--quiet', rev.to_s], quiet: !verbose)
         end
       end
 
@@ -254,9 +243,12 @@ def update_submodules(pwd, url)
 
       threads = []
       submodule_url_list = []
+      output = ''
+      Dir.chdir(File.join(abs_clone_path, pwd).to_s) {
+        output = fail_on_error('git', 'submodule', 'init', quiet: !verbose)
+      }
 
-      Terrapin::CommandLine.new('cd', ':path; git submodule init 2>&1')
-                           .run(path: File.join(abs_clone_path, pwd)).split("\n").each do |line|
+      output.split("\n").each do |line|
         submodule_path, submodule_url = parse_update_info(line)
         submodule_url_list << submodule_url
 
@@ -270,10 +262,10 @@ def update_submodules(pwd, url)
     def thread_update_submodule(submodule_url, submodule_path, threads, pwd)
       threads << Thread.new do
         with_git_mirror(submodule_url) do |mirror, _|
-          Terrapin::CommandLine.new('cd', ':dir; git submodule update --quiet --reference :mirror :path')
-                               .run(dir: File.join(abs_clone_path, pwd).to_s,
-                                    mirror: mirror.to_s,
-                                    path: submodule_path.to_s)
+
+          Dir.chdir(File.join(abs_clone_path, pwd).to_s) {
+            fail_pipe_on_error(['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact, quiet: !verbose)
+          }
         end
 
         update_submodules(File.join(pwd, submodule_path), submodule_url)
@@ -345,19 +337,24 @@ def prefetch(submodule_file)
     # that this repo has been updated on this run of fastclone
     def store_updated_repo(url, mirror, repo_name, fail_hard)
       unless Dir.exist?(mirror)
-        Terrapin::CommandLine.new('git clone', "--mirror :url :mirror#{NO_STDOUT_ERR}")
-                             .run(url: url.to_s, mirror: mirror.to_s)
+        fail_pipe_on_error(['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', url.to_s, mirror.to_s], quiet: !verbose)
       end
 
-      Terrapin::CommandLine.new('cd', ":path; git remote update --prune#{NO_STDOUT_ERR}").run(path: mirror)
-
+      Dir.chdir(mirror) {
+        output = fail_on_error(*['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact, quiet: !verbose)
+        puts "Output from `git remote update`:\n#{output}" if verbose
+      }
       reference_updated[repo_name] = true
-    rescue Terrapin::ExitStatusError => e
-      # To avoid corruption of the cache, if we failed to update or check out we remove
-      # the cache directory entirely. This may cause the current clone to fail, but if the
-      # underlying error from git is transient it will not affect future clones.
-      puts '[WARN] Removing the fastclone cache.'
-      FileUtils.remove_entry_secure(mirror, force: true)
+    rescue RunnerExecutionRuntimeError => e
+      if retriable_error?(e.output)
+        print_formatted_error(e.output, mirror)
+        clear_cache(mirror, url)
+
+        if attempt_number < retries_allowed
+          attempt_number += 1
+          retry
+        end
+      end
       raise e if fail_hard
     end
 
@@ -374,9 +371,9 @@ def retriable_error?(error)
       error.to_s =~ /^STDERR:\n.*^#{Regexp.union(error_strings)}/m
     end
 
-    def print_formatted_error(error)
+    def print_formatted_error(error, location)
       indented_error = error.to_s.split("\n").map { |s| ">  #{s}\n" }.join
-      puts "Encountered a retriable error:\n#{indented_error}\n\nRemoving the fastclone cache."
+      puts "[INFO] Encountered a retriable error:\n#{indented_error}\n\n[WARN] Removing the fastclone cache at #{location}"
     end
 
     # To avoid corruption of the cache, if we failed to update or check out we remove
@@ -408,9 +405,9 @@ def with_git_mirror(url)
       with_reference_repo_lock(url) do
         yield dir, attempt_number
       end
-    rescue Terrapin::ExitStatusError => e
-      if retriable_error?(e)
-        print_formatted_error(e)
+    rescue RunnerExecutionRuntimeError => e
+      if retriable_error?(e.output)
+        print_formatted_error(e.output, dir)
         clear_cache(dir, url)
 
         if attempt_number < retries_allowed
diff --git a/lib/git-fastclone/version.rb b/lib/git-fastclone/version.rb
index 83dbd7c..b354043 100644
--- a/lib/git-fastclone/version.rb
+++ b/lib/git-fastclone/version.rb
@@ -2,5 +2,5 @@
 
 # Version string for git-fastclone
 module GitFastCloneVersion
-  VERSION = '1.3.4'
+  VERSION = '1.4.0'
 end
diff --git a/lib/runner_execution.rb b/lib/runner_execution.rb
new file mode 100644
index 0000000..89fd56b
--- /dev/null
+++ b/lib/runner_execution.rb
@@ -0,0 +1,217 @@
+require 'open3'
+require 'logger'
+
+# Execution primitives that force explicit error handling and never call the shell.
+# Cargo-culted from internal BuildExecution code on top of public version: https://github.com/square/build_execution
+module RunnerExecution
+  class RunnerExecutionRuntimeError < RuntimeError
+    attr_reader :status, :exitstatus, :command, :output
+
+    def initialize(status, command, output = nil)
+      @status           = status
+      @exitstatus       = status.exitstatus
+      @command          = command
+      @output           = output
+
+      super "#{status.inspect}\n#{command.inspect}"
+    end
+  end
+
+  # Wrapper around open3.pipeline_r which fails on error.
+  # and stops users from invoking the shell by accident.
+  def fail_pipe_on_error(*cmd_list, quiet: false, **opts)
+    print_command('Running Pipeline:', cmd_list) unless quiet
+
+    env = opts.delete(:env) { {} }
+    raise ArgumentError, "The :env option must be a hash, not #{env.inspect}" if !env.is_a?(Hash)
+
+    cmd_list.map! { |cmd| shell_safe(cmd).unshift(env) }
+
+    output, *status_list = Open3.pipeline_r(*cmd_list, opts) do |out, wait_threads|
+      out_reader = Thread.new do
+        if quiet
+          output = out.read
+        else
+          # Output from pipeline should go to stdout and also get returned for
+          # processing if necessary.
+          output = tee(out, STDOUT)
+        end
+        out.close
+        output
+      end
+      [out_reader.value] + wait_threads.map(&:value)
+    end
+    exit_on_status(output, cmd_list, status_list, quiet: quiet)
+  end
+  module_function :fail_pipe_on_error
+
+  # Runs a command that fails on error.
+  # Uses popen2e wrapper. Handles bad statuses with potential for retries.
+  def fail_on_error(*cmd, stdin_data: nil, binmode: false, quiet: false, **opts)
+    print_command('Running Shell Safe Command:', [cmd]) unless quiet
+    shell_safe_cmd = shell_safe(cmd)
+    retry_times = opts[:retry] || 0
+    opts.delete(:retry)
+
+    while retry_times >= 0
+      output, status = popen2e_wrapper(*shell_safe_cmd, stdin_data: stdin_data, binmode: binmode,
+                                       quiet: quiet, **opts)
+
+      break unless status.exitstatus != 0
+
+      logger.debug("Command failed with exit status #{status.exitstatus}, retrying #{retry_times} more time(s).") if retry_times > 0
+      retry_times -= 1
+    end
+
+    # Get out with the status, good or bad.
+    exit_on_status(output, [shell_safe_cmd], [status], quiet: quiet)
+  end
+  module_function :fail_on_error
+
+  # Wrapper around open3.popen2e
+  #
+  # We emulate open3.capture2e with the following changes in behavior:
+  # 1) The command is printed to stdout before execution.
+  # 2) Attempts to use the shell implicitly are blocked.
+  # 3) Nonzero return codes result in the process exiting.
+  # 4) Combined stdout/stderr goes to callers stdout
+  #    (continuously streamed) and is returned as a string
+  #
+  # If you're looking for more process/stream control read the spawn
+  # documentation, and pass options directly here
+  def popen2e_wrapper(*shell_safe_cmd, stdin_data: nil, binmode: false,
+                       quiet: false, **opts)
+
+    env = opts.delete(:env) { {} }
+    raise ArgumentError, "The :env option must be a hash, not #{env.inspect}" if !env.is_a?(Hash)
+
+    # Most of this is copied from Open3.capture2e in ruby/lib/open3.rb
+    _output, _status = Open3.popen2e(env, *shell_safe_cmd, opts) do |i, oe, t|
+      if binmode
+        i.binmode
+        oe.binmode
+      end
+
+      outerr_reader = Thread.new do
+        if quiet
+          oe.read
+        else
+          # Instead of oe.read, we redirect. Output from command goes to stdout
+          # and also is returned for processing if necessary.
+          tee(oe, STDOUT)
+        end
+      end
+
+      if stdin_data
+        begin
+          i.write stdin_data
+        rescue Errno::EPIPE
+        end
+      end
+
+      i.close
+      [outerr_reader.value, t.value]
+    end
+  end
+  module_function :popen2e_wrapper
+
+  # Look at a cmd list intended for spawn.
+  # determine if spawn will call the shell implicitly, fail in that case.
+  def shell_safe(cmd)
+    # Take the first string and change it to a list of [executable,argv0]
+    # This syntax for calling popen2e (and eventually spawn) avoids
+    # the shell in all cases
+    shell_safe_cmd = Array.new(cmd)
+    if shell_safe_cmd[0].class == String
+      shell_safe_cmd[0] = [shell_safe_cmd[0], shell_safe_cmd[0]]
+    end
+    shell_safe_cmd
+  end
+  module_function :shell_safe
+
+  def debug_print_cmd_list(cmd_list)
+    # Take a list of command argument lists like you'd sent to open3.pipeline or
+    # fail_on_error_pipe and print out a string that would do the same thing when
+    # entered at the shell.
+    #
+    # This is a converter from our internal representation of commands to a subset
+    # of bash that can be executed directly.
+    #
+    # Note this has problems if you specify env or opts
+    # TODO: make this remove those command parts
+    "\"" +
+      cmd_list.map do |cmd|
+        cmd.map do |arg|
+          arg.gsub("\"", "\\\"") # Escape all double quotes in command arguments
+        end.join("\" \"") # Fully quote all command parts, beginning and end.
+      end.join("\" | \"") + "\"" # Pipe commands to one another.
+  end
+  module_function :debug_print_cmd_list
+
+  # Prints a formatted string with command
+  def print_command(message, cmd)
+    logger.debug("#{message} #{debug_print_cmd_list(cmd)}\n")
+  end
+  module_function :print_command
+
+  # Takes in an input stream and an output stream
+  # Redirects data from one to the other until the input stream closes.
+  # Returns all data that passed through on return.
+  def tee(in_stream, out_stream)
+    alldata = ''
+    loop do
+      begin
+        data = in_stream.read_nonblock(4096)
+        alldata += data
+        out_stream.write(data)
+        out_stream.flush
+      rescue IO::WaitReadable
+        IO.select([in_stream])
+        retry
+      rescue IOError
+        break
+      end
+    end
+    alldata
+  end
+  module_function :tee
+
+  # If any of the statuses are bad, exits with the
+  # return code of the first one.
+  #
+  # Otherwise returns first argument (output)
+  def exit_on_status(output, cmd_list, status_list, quiet: false)
+    status_list.each_index do |index|
+      status = status_list[index]
+      cmd = cmd_list[index]
+      check_status(cmd, status, output: output, quiet: quiet)
+    end
+
+    output
+  end
+  module_function :exit_on_status
+
+  def check_status(cmd, status, output: nil, quiet: false)
+    return if status.exited? && status.exitstatus == 0
+
+    # If we exited nonzero or abnormally, print debugging info and explode.
+    if status.exited?
+      logger.debug("Process Exited normally. Exit status:#{status.exitstatus}") unless quiet
+    else
+      # This should only get executed if we're stopped or signaled
+      logger.debug("Process exited abnormally:\nProcessStatus: #{status.inspect}\n" \
+        "Raw POSIX Status: #{status.to_i}\n") unless quiet
+    end
+
+    raise RunnerExecutionRuntimeError.new(status, cmd, output)
+  end
+  module_function :check_status
+
+  DEFAULT_LOGGER = Logger.new(STDOUT)
+  private_constant :DEFAULT_LOGGER
+
+  def logger
+    DEFAULT_LOGGER
+  end
+  module_function :logger
+end

From 8958595815c592ab6bbecbb7739839018993b386 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Thu, 9 Mar 2023 16:23:38 -0500
Subject: [PATCH 08/20] fixup

---
 lib/git-fastclone.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index a01a9d9..44c70e6 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -73,8 +73,6 @@ class Runner
 
     DEFAULT_GIT_ALLOW_PROTOCOL = 'file:git:http:https:ssh'
 
-    NO_STDOUT_ERR = ' > /dev/null 2>&1 '
-
     attr_accessor :reference_dir, :prefetch_submodules, :reference_updated, :reference_mutex,
                   :options, :abs_clone_path, :using_local_repo, :verbose, :color,
                   :flock_timeout_secs

From f575e9a54c0f5ed021c03d38247925ffa60039e0 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Fri, 10 Mar 2023 15:14:47 -0500
Subject: [PATCH 09/20] Remove Terrapin

---
 Gemfile.lock          | 6 +-----
 git-fastclone.gemspec | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index b4103be..f79b23d 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -3,13 +3,11 @@ PATH
   specs:
     git-fastclone (1.4.0)
       colorize
-      terrapin (~> 0.6.0)
 
 GEM
   remote: https://rubygems.org/
   specs:
     ast (2.4.2)
-    climate_control (0.2.0)
     colorize (0.8.1)
     diff-lcs (1.5.0)
     json (2.6.3)
@@ -46,8 +44,6 @@ GEM
     rubocop-ast (1.24.0)
       parser (>= 3.1.1.0)
     ruby-progressbar (1.11.0)
-    terrapin (0.6.0)
-      climate_control (>= 0.0.3, < 1.0)
     unicode-display_width (2.3.0)
 
 PLATFORMS
@@ -61,4 +57,4 @@ DEPENDENCIES
   rubocop
 
 BUNDLED WITH
-   2.3.26
+   2.3.21
diff --git a/git-fastclone.gemspec b/git-fastclone.gemspec
index 233bda2..0ec90c2 100644
--- a/git-fastclone.gemspec
+++ b/git-fastclone.gemspec
@@ -37,6 +37,5 @@ Gem::Specification.new do |gem|
   gem.required_ruby_version = '>= 2.7'
 
   gem.add_runtime_dependency 'colorize'
-  gem.add_runtime_dependency 'terrapin', '~> 0.6.0'
   gem.metadata['rubygems_mfa_required'] = 'true'
 end

From ea6e7e571c3630ee4505765bdfe78c31190f5eb8 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Fri, 10 Mar 2023 15:27:17 -0500
Subject: [PATCH 10/20] address comments

---
 lib/git-fastclone.rb | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index 44c70e6..d60cb12 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -260,7 +260,6 @@ def update_submodules(pwd, url)
     def thread_update_submodule(submodule_url, submodule_path, threads, pwd)
       threads << Thread.new do
         with_git_mirror(submodule_url) do |mirror, _|
-
           Dir.chdir(File.join(abs_clone_path, pwd).to_s) {
             fail_pipe_on_error(['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact, quiet: !verbose)
           }
@@ -339,20 +338,22 @@ def store_updated_repo(url, mirror, repo_name, fail_hard)
       end
 
       Dir.chdir(mirror) {
-        output = fail_on_error(*['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact, quiet: !verbose)
-        puts "Output from `git remote update`:\n#{output}" if verbose
+        cmd = ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact
+        if verbose
+          fail_pipe_on_error(cmd, quiet: !verbose)
+        else
+          # Because above operation might spit out a lot to stderr, we use this to swallow them
+          # and only display if the operation return non 0 exit code
+          fail_on_error(*cmd, quiet: !verbose)
+        end
       }
       reference_updated[repo_name] = true
     rescue RunnerExecutionRuntimeError => e
-      if retriable_error?(e.output)
-        print_formatted_error(e.output, mirror)
-        clear_cache(mirror, url)
-
-        if attempt_number < retries_allowed
-          attempt_number += 1
-          retry
-        end
-      end
+      # To avoid corruption of the cache, if we failed to update or check out we remove
+      # the cache directory entirely. This may cause the current clone to fail, but if the
+      # underlying error from git is transient it will not affect future clones.
+      puts 'elton checkpoint1'
+      clear_cache(mirror, url)
       raise e if fail_hard
     end
 
@@ -371,13 +372,14 @@ def retriable_error?(error)
 
     def print_formatted_error(error, location)
       indented_error = error.to_s.split("\n").map { |s| ">  #{s}\n" }.join
-      puts "[INFO] Encountered a retriable error:\n#{indented_error}\n\n[WARN] Removing the fastclone cache at #{location}"
+      puts "[INFO] Encountered a retriable error:\n#{indented_error}\n"
     end
 
     # To avoid corruption of the cache, if we failed to update or check out we remove
     # the cache directory entirely. This may cause the current clone to fail, but if the
     # underlying error from git is transient it will not affect future clones.
     def clear_cache(dir, url)
+      puts "[WARN] Removing the fastclone cache at #{dir}"
       FileUtils.remove_entry_secure(dir, force: true)
       reference_updated.delete(reference_repo_name(url))
     end
@@ -414,7 +416,7 @@ def with_git_mirror(url)
         end
       end
 
-      raise
+      raise e
     end
 
     def usage

From 0dc8b43b5be5874412e7c2c8445bde89e5ac207b Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Fri, 10 Mar 2023 17:07:39 -0500
Subject: [PATCH 11/20] Fix specs

---
 spec/git_fastclone_runner_spec.rb | 120 ++++++++++++------------------
 1 file changed, 49 insertions(+), 71 deletions(-)

diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb
index 6d764bc..ad92a74 100644
--- a/spec/git_fastclone_runner_spec.rb
+++ b/spec/git_fastclone_runner_spec.rb
@@ -50,7 +50,6 @@ def create_lockfile_double
       expect(subject.reference_mutex).to eq({})
       expect(subject.reference_updated).to eq({})
       expect(subject.options).to eq({})
-      expect(subject.logger).to eq(nil)
     end
   end
 
@@ -87,10 +86,9 @@ def create_lockfile_double
   end
 
   describe '.clone' do
-    let(:terrapin_commandline_double) { double('new_terrapin_commandline') }
+    let(:runner_execution_double) { double('runner_execution') }
     before(:each) do
-      allow(terrapin_commandline_double).to receive(:run) {}
-      expect(Time).to receive(:now).twice { 0 }
+      allow(runner_execution_double).to receive(:fail_pipe_on_error) {}
       allow(Dir).to receive(:pwd) { '/pwd' }
       allow(Dir).to receive(:chdir).and_yield
       allow(subject).to receive(:with_git_mirror).and_yield('/cache', 0)
@@ -98,36 +96,23 @@ def create_lockfile_double
     end
 
     it 'should clone correctly' do
-      expect(Terrapin::CommandLine).to receive(:new).with(
-        'git clone',
-        '--quiet --reference :mirror :url :path'
-      ) { terrapin_commandline_double }
-      expect(Terrapin::CommandLine).to receive(:new).with(
-        'git checkout',
-        '--quiet :rev'
-      ) { terrapin_commandline_double }
-      expect(terrapin_commandline_double).to receive(:run).with(
-        mirror: '/cache',
-        url: placeholder_arg,
-        path: '/pwd/.',
-        config: ''
-      )
-      expect(terrapin_commandline_double).to receive(:run).with(rev: placeholder_arg)
+      expect(subject).to receive(:fail_pipe_on_error).with(
+        ["git", "checkout", "--quiet", "PH"],
+        {:quiet=>true}
+      ) { runner_execution_double }
+      expect(subject).to receive(:fail_pipe_on_error).with(
+        ["git", "clone", "--quiet", "--reference", "/cache", "PH", "/pwd/."],
+        {:quiet=>true}
+      ) { runner_execution_double }
 
       subject.clone(placeholder_arg, placeholder_arg, '.', nil)
     end
 
     it 'should clone correctly with custom configs' do
-      expect(Terrapin::CommandLine).to receive(:new).with(
-        'git clone',
-        '--quiet --reference :mirror :url :path --config :config'
-      ) { terrapin_commandline_double }
-      expect(terrapin_commandline_double).to receive(:run).with(
-        mirror: '/cache',
-        url: placeholder_arg,
-        path: '/pwd/.',
-        config: 'config'
-      )
+      expect(subject).to receive(:fail_pipe_on_error).with(
+        ["git", "clone", "--quiet", "--reference", "/cache", "PH", "/pwd/.", "--config", "config"],
+        {:quiet=>true}
+      ) { runner_execution_double }
 
       subject.clone(placeholder_arg, nil, '.', 'config')
     end
@@ -294,36 +279,35 @@ def create_lockfile_double
 
   describe '.store_updated_repo' do
     context 'when fail_hard is true' do
-      it 'should raise a Terrapin error' do
-        terrapin_commandline_double = double('new_terrapin_commandline')
-        allow(terrapin_commandline_double).to receive(:run) { raise Terrapin::ExitStatusError }
-        allow(Terrapin::CommandLine).to receive(:new) { terrapin_commandline_double }
+      it 'should raise a Runtime error and clear cache' do
+        status = double('status')
+        allow(status).to receive(:exitstatus).and_return(1)
+        ex = RunnerExecution::RunnerExecutionRuntimeError.new(status, 'cmd')
+        allow(subject).to receive(:fail_pipe_on_error) { raise ex }
         expect(FileUtils).to receive(:remove_entry_secure).with(placeholder_arg, force: true)
-        expect do
+        expect {
           subject.store_updated_repo(placeholder_arg, placeholder_arg, placeholder_arg, true)
-        end.to raise_error(Terrapin::ExitStatusError)
+        }.to raise_error(ex)
       end
     end
 
     context 'when fail_hard is false' do
-      it 'should not raise a terrapin error' do
-        terrapin_commandline_double = double('new_terrapin_commandline')
-        allow(terrapin_commandline_double).to receive(:run) { raise Terrapin::ExitStatusError }
-        allow(Terrapin::CommandLine).to receive(:new) { terrapin_commandline_double }
+      it 'should not raise a Runtime error but clear cache' do
+        status = double('status')
+        allow(status).to receive(:exitstatus).and_return(1)
+        ex = RunnerExecution::RunnerExecutionRuntimeError.new(status, 'cmd')
+        allow(subject).to receive(:fail_pipe_on_error) { raise ex }
         expect(FileUtils).to receive(:remove_entry_secure).with(placeholder_arg, force: true)
-
-        expect do
+        expect {
           subject.store_updated_repo(placeholder_arg, placeholder_arg, placeholder_arg, false)
-        end.not_to raise_error
+        }.to_not raise_error(ex)
       end
     end
 
     let(:placeholder_hash) { {} }
 
     it 'should correctly update the hash' do
-      terrapin_commandline_double = double('new_terrapin_commandline')
-      allow(terrapin_commandline_double).to receive(:run) {}
-      allow(Terrapin::CommandLine).to receive(:new) { terrapin_commandline_double }
+      allow(subject).to receive(:fail_pipe_on_error)
       allow(Dir).to receive(:chdir) {}
 
       subject.reference_updated = placeholder_hash
@@ -351,7 +335,11 @@ def try_with_git_mirror(responses, results)
           ->(url) { url }
         else
           # Simulate failed error response
-          ->(_url) { raise Terrapin::ExitStatusError, response }
+          ->(_url) {
+            status = double('status')
+            allow(status).to receive(:exitstatus).and_return(1)
+            raise RunnerExecution::RunnerExecutionRuntimeError.new(status, 'cmd', response)
+          }
         end
       end
 
@@ -366,19 +354,22 @@ def try_with_git_mirror(responses, results)
     end
 
     let(:expected_commands) { [] }
-    let(:expected_commands_args) { [] }
 
     before(:each) do
-      expect(expected_commands.length).to eq(expected_commands_args.length)
-      allow(Terrapin::CommandLine).to receive(:new) do |*command|
+      allow(subject).to receive(:fail_pipe_on_error) { |*params|
+        command = params[0]
         expect(expected_commands.length).to be > 0
         expected_command = expected_commands.shift
-        expected_args = expected_commands_args.shift
         expect(command).to eq(expected_command)
-        stub = double(Terrapin::CommandLine)
-        expect(stub).to receive(:run).with(expected_args)
-        stub
-      end
+      }
+      allow(subject).to receive(:fail_on_error) { |*params|
+        # last one is an argument `quiet:`
+        command = params.first(params.size-1)
+        expect(expected_commands.length).to be > 0
+        expected_command = expected_commands.shift
+        expect(command).to eq(expected_command)
+      }
+      allow(Dir).to receive(:chdir).and_yield
 
       allow(subject).to receive(:print_formatted_error) {}
       allow(subject).to receive(:reference_repo_dir).and_return(test_reference_repo_dir)
@@ -391,26 +382,13 @@ def try_with_git_mirror(responses, results)
 
     def clone_cmds
       [
-        ['git clone', '--mirror :url :mirror > /dev/null 2>&1 '],
-        ['cd', ':path; git remote update --prune > /dev/null 2>&1 ']
-      ]
-    end
-
-    def clone_args
-      [
-        {
-          mirror: test_reference_repo_dir,
-          url: test_url_valid
-        },
-        {
-          path: test_reference_repo_dir
-        }
+        ['git', 'clone', '--quiet', '--mirror', test_url_valid, test_reference_repo_dir],
+        ['git', 'remote', 'update', '--prune']
       ]
     end
 
     context 'expecting 1 clone attempt' do
       let(:expected_commands) { clone_cmds }
-      let(:expected_commands_args) { clone_args }
 
       it 'should succeed with a successful clone' do
         expect(subject).not_to receive(:clear_cache)
@@ -421,7 +399,7 @@ def clone_args
         expect(subject).not_to receive(:clear_cache)
         expect do
           try_with_git_mirror(['Some unexpected error message'], [])
-        end.to raise_error(Terrapin::ExitStatusError)
+        end.to raise_error(RunnerExecution::RunnerExecutionRuntimeError)
       end
     end
 
@@ -438,7 +416,7 @@ def clone_args
         expect(subject).to receive(:clear_cache).twice.and_call_original
         expect do
           try_with_git_mirror([retriable_error, retriable_error], [])
-        end.to raise_error(Terrapin::ExitStatusError)
+        end.to raise_error(RunnerExecution::RunnerExecutionRuntimeError)
       end
     end
   end

From 59e8e145db40eac2f36a45cdb6039018799521ad Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Fri, 10 Mar 2023 17:10:47 -0500
Subject: [PATCH 12/20] fixup

---
 Gemfile.lock | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index f79b23d..c6557c3 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -57,4 +57,4 @@ DEPENDENCIES
   rubocop
 
 BUNDLED WITH
-   2.3.21
+   2.3.26

From d4d01c92537b36d62d021dac0a60010128ac2921 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Fri, 10 Mar 2023 17:24:07 -0500
Subject: [PATCH 13/20] rubocop fix

---
 lib/git-fastclone.rb              | 28 +++++++++++++++++-----------
 lib/runner_execution.rb           |  4 +++-
 spec/git_fastclone_runner_spec.rb | 24 ++++++++++++------------
 3 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index d60cb12..b50cd0a 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -210,8 +210,8 @@ def clone(url, rev, src_dir, config)
         clear_clone_dest_if_needed(attempt_number, clone_dest)
 
         clone_commands = ['git', 'clone', verbose ? '--verbose' : '--quiet']
-        clone_commands << "--reference" << mirror.to_s << url.to_s << clone_dest
-        clone_commands << "--config" << config.to_s unless config.nil?
+        clone_commands << '--reference' << mirror.to_s << url.to_s << clone_dest
+        clone_commands << '--config' << config.to_s unless config.nil?
         fail_pipe_on_error(clone_commands, quiet: !verbose)
       end
 
@@ -242,9 +242,9 @@ def update_submodules(pwd, url)
       threads = []
       submodule_url_list = []
       output = ''
-      Dir.chdir(File.join(abs_clone_path, pwd).to_s) {
+      Dir.chdir(File.join(abs_clone_path, pwd).to_s) do
         output = fail_on_error('git', 'submodule', 'init', quiet: !verbose)
-      }
+      end
 
       output.split("\n").each do |line|
         submodule_path, submodule_url = parse_update_info(line)
@@ -260,9 +260,12 @@ def update_submodules(pwd, url)
     def thread_update_submodule(submodule_url, submodule_path, threads, pwd)
       threads << Thread.new do
         with_git_mirror(submodule_url) do |mirror, _|
-          Dir.chdir(File.join(abs_clone_path, pwd).to_s) {
-            fail_pipe_on_error(['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact, quiet: !verbose)
-          }
+          Dir.chdir(File.join(abs_clone_path, pwd).to_s) do
+            fail_pipe_on_error(
+              ['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s,
+               submodule_path.to_s].compact, quiet: !verbose
+            )
+          end
         end
 
         update_submodules(File.join(pwd, submodule_path), submodule_url)
@@ -334,10 +337,13 @@ def prefetch(submodule_file)
     # that this repo has been updated on this run of fastclone
     def store_updated_repo(url, mirror, repo_name, fail_hard)
       unless Dir.exist?(mirror)
-        fail_pipe_on_error(['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', url.to_s, mirror.to_s], quiet: !verbose)
+        fail_pipe_on_error(
+          ['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', url.to_s,
+           mirror.to_s], quiet: !verbose
+        )
       end
 
-      Dir.chdir(mirror) {
+      Dir.chdir(mirror) do
         cmd = ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact
         if verbose
           fail_pipe_on_error(cmd, quiet: !verbose)
@@ -346,7 +352,7 @@ def store_updated_repo(url, mirror, repo_name, fail_hard)
           # and only display if the operation return non 0 exit code
           fail_on_error(*cmd, quiet: !verbose)
         end
-      }
+      end
       reference_updated[repo_name] = true
     rescue RunnerExecutionRuntimeError => e
       # To avoid corruption of the cache, if we failed to update or check out we remove
@@ -370,7 +376,7 @@ def retriable_error?(error)
       error.to_s =~ /^STDERR:\n.*^#{Regexp.union(error_strings)}/m
     end
 
-    def print_formatted_error(error, location)
+    def print_formatted_error(error, _location)
       indented_error = error.to_s.split("\n").map { |s| ">  #{s}\n" }.join
       puts "[INFO] Encountered a retriable error:\n#{indented_error}\n"
     end
diff --git a/lib/runner_execution.rb b/lib/runner_execution.rb
index 89fd56b..b638aa3 100644
--- a/lib/runner_execution.rb
+++ b/lib/runner_execution.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'open3'
 require 'logger'
 
@@ -23,7 +25,7 @@ def fail_pipe_on_error(*cmd_list, quiet: false, **opts)
     print_command('Running Pipeline:', cmd_list) unless quiet
 
     env = opts.delete(:env) { {} }
-    raise ArgumentError, "The :env option must be a hash, not #{env.inspect}" if !env.is_a?(Hash)
+    raise ArgumentError, "The :env option must be a hash, not #{env.inspect}" unless env.is_a?(Hash)
 
     cmd_list.map! { |cmd| shell_safe(cmd).unshift(env) }
 
diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb
index ad92a74..154fb4f 100644
--- a/spec/git_fastclone_runner_spec.rb
+++ b/spec/git_fastclone_runner_spec.rb
@@ -97,12 +97,12 @@ def create_lockfile_double
 
     it 'should clone correctly' do
       expect(subject).to receive(:fail_pipe_on_error).with(
-        ["git", "checkout", "--quiet", "PH"],
-        {:quiet=>true}
+        ['git', 'checkout', '--quiet', 'PH'],
+        { quiet: true }
       ) { runner_execution_double }
       expect(subject).to receive(:fail_pipe_on_error).with(
-        ["git", "clone", "--quiet", "--reference", "/cache", "PH", "/pwd/."],
-        {:quiet=>true}
+        ['git', 'clone', '--quiet', '--reference', '/cache', 'PH', '/pwd/.'],
+        { quiet: true }
       ) { runner_execution_double }
 
       subject.clone(placeholder_arg, placeholder_arg, '.', nil)
@@ -110,8 +110,8 @@ def create_lockfile_double
 
     it 'should clone correctly with custom configs' do
       expect(subject).to receive(:fail_pipe_on_error).with(
-        ["git", "clone", "--quiet", "--reference", "/cache", "PH", "/pwd/.", "--config", "config"],
-        {:quiet=>true}
+        ['git', 'clone', '--quiet', '--reference', '/cache', 'PH', '/pwd/.', '--config', 'config'],
+        { quiet: true }
       ) { runner_execution_double }
 
       subject.clone(placeholder_arg, nil, '.', 'config')
@@ -285,9 +285,9 @@ def create_lockfile_double
         ex = RunnerExecution::RunnerExecutionRuntimeError.new(status, 'cmd')
         allow(subject).to receive(:fail_pipe_on_error) { raise ex }
         expect(FileUtils).to receive(:remove_entry_secure).with(placeholder_arg, force: true)
-        expect {
+        expect do
           subject.store_updated_repo(placeholder_arg, placeholder_arg, placeholder_arg, true)
-        }.to raise_error(ex)
+        end.to raise_error(ex)
       end
     end
 
@@ -298,9 +298,9 @@ def create_lockfile_double
         ex = RunnerExecution::RunnerExecutionRuntimeError.new(status, 'cmd')
         allow(subject).to receive(:fail_pipe_on_error) { raise ex }
         expect(FileUtils).to receive(:remove_entry_secure).with(placeholder_arg, force: true)
-        expect {
+        expect do
           subject.store_updated_repo(placeholder_arg, placeholder_arg, placeholder_arg, false)
-        }.to_not raise_error(ex)
+        end.to_not raise_error(ex)
       end
     end
 
@@ -335,7 +335,7 @@ def try_with_git_mirror(responses, results)
           ->(url) { url }
         else
           # Simulate failed error response
-          ->(_url) {
+          lambda { |_url|
             status = double('status')
             allow(status).to receive(:exitstatus).and_return(1)
             raise RunnerExecution::RunnerExecutionRuntimeError.new(status, 'cmd', response)
@@ -364,7 +364,7 @@ def try_with_git_mirror(responses, results)
       }
       allow(subject).to receive(:fail_on_error) { |*params|
         # last one is an argument `quiet:`
-        command = params.first(params.size-1)
+        command = params.first(params.size - 1)
         expect(expected_commands.length).to be > 0
         expected_command = expected_commands.shift
         expect(command).to eq(expected_command)

From fbff44e84b80ac804fb7001657afeba32c90abe6 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Fri, 10 Mar 2023 17:25:45 -0500
Subject: [PATCH 14/20] exclude runner_execution from rubocop

---
 .rubocop.yml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.rubocop.yml b/.rubocop.yml
index 0350c34..712ee13 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -3,6 +3,8 @@ inherit_from: .rubocop_todo.yml
 AllCops:
   TargetRubyVersion: 2.7
   NewCops: enable
+  Exclude:
+    - 'lib/runner_execution.rb'
 
 Naming/FileName:
   Exclude:

From 8f552aa5ef0f2dabde278be89f9e2ca994a61bcf Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Fri, 10 Mar 2023 17:42:45 -0500
Subject: [PATCH 15/20] address comments

---
 lib/git-fastclone.rb | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index b50cd0a..73ec8a0 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -358,7 +358,6 @@ def store_updated_repo(url, mirror, repo_name, fail_hard)
       # To avoid corruption of the cache, if we failed to update or check out we remove
       # the cache directory entirely. This may cause the current clone to fail, but if the
       # underlying error from git is transient it will not affect future clones.
-      puts 'elton checkpoint1'
       clear_cache(mirror, url)
       raise e if fail_hard
     end
@@ -376,7 +375,7 @@ def retriable_error?(error)
       error.to_s =~ /^STDERR:\n.*^#{Regexp.union(error_strings)}/m
     end
 
-    def print_formatted_error(error, _location)
+    def print_formatted_error(error)
       indented_error = error.to_s.split("\n").map { |s| ">  #{s}\n" }.join
       puts "[INFO] Encountered a retriable error:\n#{indented_error}\n"
     end
@@ -413,7 +412,7 @@ def with_git_mirror(url)
       end
     rescue RunnerExecutionRuntimeError => e
       if retriable_error?(e.output)
-        print_formatted_error(e.output, dir)
+        print_formatted_error(e.output)
         clear_cache(dir, url)
 
         if attempt_number < retries_allowed

From 0e69db4933eaa6c982915271756650c7afbc82fc Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Mon, 13 Mar 2023 10:23:00 -0400
Subject: [PATCH 16/20] Revert "exclude runner_execution from rubocop"

This reverts commit fbff44e84b80ac804fb7001657afeba32c90abe6.
---
 .rubocop.yml | 2 --
 1 file changed, 2 deletions(-)

diff --git a/.rubocop.yml b/.rubocop.yml
index 712ee13..0350c34 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -3,8 +3,6 @@ inherit_from: .rubocop_todo.yml
 AllCops:
   TargetRubyVersion: 2.7
   NewCops: enable
-  Exclude:
-    - 'lib/runner_execution.rb'
 
 Naming/FileName:
   Exclude:

From 280d3280200976e85d4712b0519dd1bc8b926994 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Mon, 13 Mar 2023 11:19:37 -0400
Subject: [PATCH 17/20] Fix wrong logic in retriable_error

---
 lib/git-fastclone.rb              | 14 +++++++-------
 spec/git_fastclone_runner_spec.rb |  6 +-----
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index 73ec8a0..ebef927 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -364,15 +364,15 @@ def store_updated_repo(url, mirror, repo_name, fail_hard)
 
     def retriable_error?(error)
       error_strings = [
-        'fatal: missing blob object',
-        'fatal: remote did not send all necessary objects',
-        /fatal: packed object [a-z0-9]+ \(stored in .*?\) is corrupt/,
-        /fatal: pack has \d+ unresolved delta/,
-        'error: unable to read sha1 file of ',
-        'fatal: did not receive expected object',
+        /^fatal: missing blob object/,
+        /^fatal: remote did not send all necessary objects/,
+        /^fatal: packed object [a-z0-9]+ \(stored in .*?\) is corrupt/,
+        /^fatal: pack has \d+ unresolved delta/,
+        /^error: unable to read sha1 file of /,
+        /^fatal: did not receive expected object/,
         /^fatal: unable to read tree [a-z0-9]+\n^warning: Clone succeeded, but checkout failed/
       ]
-      error.to_s =~ /^STDERR:\n.*^#{Regexp.union(error_strings)}/m
+      error.to_s =~ /.*#{Regexp.union(error_strings)}/m
     end
 
     def print_formatted_error(error)
diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb
index 154fb4f..ec5ff28 100644
--- a/spec/git_fastclone_runner_spec.rb
+++ b/spec/git_fastclone_runner_spec.rb
@@ -319,10 +319,6 @@ def create_lockfile_double
   describe '.with_git_mirror' do
     def retriable_error
       %(
-        STDOUT:
-
-        STDERR:
-
         fatal: bad object ee35b1e14e7c3a53dcc14d82606e5b872f6a05a7
         fatal: remote did not send all necessary objects
       ).strip.split("\n").map(&:strip).join("\n")
@@ -423,7 +419,7 @@ def clone_cmds
 
   describe '.retriable_error?' do
     def format_error(error)
-      error_wrapper = "STDOUT:\n\nSTDERR:\n#{error}"
+      error_wrapper = "#{error}"
       error_wrapper.strip.lines.map(&:strip).join("\n")
     end
 

From 69373645dd970cffee54f66dab8b881482e0a1cd Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Mon, 13 Mar 2023 11:20:01 -0400
Subject: [PATCH 18/20] Include coverage for verbose mode in rspec

---
 spec/git_fastclone_runner_spec.rb | 60 ++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb
index ec5ff28..1b7a9cb 100644
--- a/spec/git_fastclone_runner_spec.rb
+++ b/spec/git_fastclone_runner_spec.rb
@@ -36,6 +36,7 @@ def create_lockfile_double
 
   before do
     stub_const('ARGV', ['ssh://git@git.com/git-fastclone.git', 'test_reference_dir'])
+    allow(STDOUT).to receive(:puts)
   end
 
   let(:yielded) { [] }
@@ -108,6 +109,20 @@ def create_lockfile_double
       subject.clone(placeholder_arg, placeholder_arg, '.', nil)
     end
 
+    it 'should clone correctly with verbose mode on' do
+      subject.verbose = true
+      expect(subject).to receive(:fail_pipe_on_error).with(
+        ['git', 'checkout', '--quiet', 'PH'],
+        { quiet: false }
+      ) { runner_execution_double }
+      expect(subject).to receive(:fail_pipe_on_error).with(
+        ['git', 'clone', '--verbose', '--reference', '/cache', 'PH', '/pwd/.'],
+        { quiet: false }
+      ) { runner_execution_double }
+
+      subject.clone(placeholder_arg, placeholder_arg, '.', nil)
+    end
+
     it 'should clone correctly with custom configs' do
       expect(subject).to receive(:fail_pipe_on_error).with(
         ['git', 'clone', '--quiet', '--reference', '/cache', 'PH', '/pwd/.', '--config', 'config'],
@@ -300,7 +315,7 @@ def create_lockfile_double
         expect(FileUtils).to receive(:remove_entry_secure).with(placeholder_arg, force: true)
         expect do
           subject.store_updated_repo(placeholder_arg, placeholder_arg, placeholder_arg, false)
-        end.to_not raise_error(ex)
+        end.to_not raise_error
       end
     end
 
@@ -376,26 +391,45 @@ def try_with_git_mirror(responses, results)
       expect(expected_commands).to be_empty
     end
 
-    def clone_cmds
+    def clone_cmds(verbose: false)
       [
-        ['git', 'clone', '--quiet', '--mirror', test_url_valid, test_reference_repo_dir],
-        ['git', 'remote', 'update', '--prune']
+        ['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', test_url_valid, test_reference_repo_dir],
+        ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact
       ]
     end
 
     context 'expecting 1 clone attempt' do
-      let(:expected_commands) { clone_cmds }
+      context 'with verbose mode on' do
+        before { subject.verbose = true }
+        let(:expected_commands) { clone_cmds(verbose:true) }
 
-      it 'should succeed with a successful clone' do
-        expect(subject).not_to receive(:clear_cache)
-        try_with_git_mirror([true], [[test_reference_repo_dir, 0]])
+        it 'should succeed with a successful clone' do
+          expect(subject).not_to receive(:clear_cache)
+          try_with_git_mirror([true], [[test_reference_repo_dir, 0]])
+        end
+
+        it 'should fail after a non-retryable clone error' do
+          expect(subject).not_to receive(:clear_cache)
+          expect do
+            try_with_git_mirror(['Some unexpected error message'], [])
+          end.to raise_error(RunnerExecution::RunnerExecutionRuntimeError)
+        end
       end
 
-      it 'should fail after a non-retryable clone error' do
-        expect(subject).not_to receive(:clear_cache)
-        expect do
-          try_with_git_mirror(['Some unexpected error message'], [])
-        end.to raise_error(RunnerExecution::RunnerExecutionRuntimeError)
+      context 'with verbose mode off' do
+        let(:expected_commands) { clone_cmds }
+
+        it 'should succeed with a successful clone' do
+          expect(subject).not_to receive(:clear_cache)
+          try_with_git_mirror([true], [[test_reference_repo_dir, 0]])
+        end
+
+        it 'should fail after a non-retryable clone error' do
+          expect(subject).not_to receive(:clear_cache)
+          expect do
+            try_with_git_mirror(['Some unexpected error message'], [])
+          end.to raise_error(RunnerExecution::RunnerExecutionRuntimeError)
+        end
       end
     end
 

From 4e5cca5ea043503475dd11e6fc5aacf46fcfc92f Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Mon, 13 Mar 2023 11:21:57 -0400
Subject: [PATCH 19/20] Disable rubocop for runner_execution since it is cargo
 culted

---
 lib/runner_execution.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/runner_execution.rb b/lib/runner_execution.rb
index b638aa3..e50f208 100644
--- a/lib/runner_execution.rb
+++ b/lib/runner_execution.rb
@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+# rubocop:disable all
 
 require 'open3'
 require 'logger'
@@ -217,3 +218,4 @@ def logger
   end
   module_function :logger
 end
+# rubocop:enable all

From 6033b7d81ba9a09463be92bd73de1e4b307e91c6 Mon Sep 17 00:00:00 2001
From: Elton Gao <yuanfeng@squareup.com>
Date: Mon, 13 Mar 2023 12:07:42 -0400
Subject: [PATCH 20/20] fixup

---
 spec/git_fastclone_runner_spec.rb | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb
index 1b7a9cb..caf81d7 100644
--- a/spec/git_fastclone_runner_spec.rb
+++ b/spec/git_fastclone_runner_spec.rb
@@ -36,7 +36,7 @@ def create_lockfile_double
 
   before do
     stub_const('ARGV', ['ssh://git@git.com/git-fastclone.git', 'test_reference_dir'])
-    allow(STDOUT).to receive(:puts)
+    allow($stdout).to receive(:puts)
   end
 
   let(:yielded) { [] }
@@ -393,7 +393,8 @@ def try_with_git_mirror(responses, results)
 
     def clone_cmds(verbose: false)
       [
-        ['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', test_url_valid, test_reference_repo_dir],
+        ['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', test_url_valid,
+         test_reference_repo_dir],
         ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact
       ]
     end
@@ -401,7 +402,7 @@ def clone_cmds(verbose: false)
     context 'expecting 1 clone attempt' do
       context 'with verbose mode on' do
         before { subject.verbose = true }
-        let(:expected_commands) { clone_cmds(verbose:true) }
+        let(:expected_commands) { clone_cmds(verbose: true) }
 
         it 'should succeed with a successful clone' do
           expect(subject).not_to receive(:clear_cache)
@@ -453,7 +454,7 @@ def clone_cmds(verbose: false)
 
   describe '.retriable_error?' do
     def format_error(error)
-      error_wrapper = "#{error}"
+      error_wrapper = error.to_s
       error_wrapper.strip.lines.map(&:strip).join("\n")
     end