Merge branch 'gitlab-git-popen' into 'master'

Use Gitlab::Git's Popen on that module's code

Closes gitaly#597

See merge request gitlab-org/gitlab-ce!14237

7 files changed, 19 insertions, 13 deletions

diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb
index 6f054ed3c6c..786e2e7e8dc 100644
--- a/lib/gitlab/git/operation_service.rb
+++ b/lib/gitlab/git/operation_service.rb
@@ -1,6 +1,8 @@
 module Gitlab
   module Git
     class OperationService
+      include Gitlab::Git::Popen
+
       WithBranchResult = Struct.new(:newrev, :repo_created, :branch_created) do
         alias_method :repo_created?, :repo_created
         alias_method :branch_created?, :branch_created
@@ -150,7 +152,7 @@ module Gitlab
         # (and have!) accidentally reset the ref to an earlier state, clobbering
         # commits. See also https://github.com/libgit2/libgit2/issues/1534.
         command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z]
-        _, status = Gitlab::Popen.popen(
+        _, status = popen(
           command,
           repository.path) do |stdin|
           stdin.write("update #{ref}\x00#{newrev}\x00#{oldrev}\x00")
diff --git a/lib/gitlab/git/popen.rb b/lib/gitlab/git/popen.rb
index 25fa62ce4bd..3d2fc471d28 100644
--- a/lib/gitlab/git/popen.rb
+++ b/lib/gitlab/git/popen.rb
@@ -5,17 +5,21 @@ require 'open3'
 module Gitlab
   module Git
     module Popen
-      def popen(cmd, path)
+      def popen(cmd, path, vars = {})
         unless cmd.is_a?(Array)
           raise "System commands must be given as an array of strings"
         end
 
-        vars = { "PWD" => path }
+        path ||= Dir.pwd
+        vars['PWD'] = path
         options = { chdir: path }
 
         @cmd_output = ""
         @cmd_status = 0
         Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+          yield(stdin) if block_given?
+          stdin.close
+
           @cmd_output << stdout.read
           @cmd_output << stderr.read
           @cmd_status = wait_thr.value.exitstatus
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 4b000bd31e2..0be35034d24 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -490,7 +490,7 @@ module Gitlab
 
             # Not found -> ["", 0]
             # Found -> ["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0]
-            Gitlab::Popen.popen(args, @path).first.split.last
+            popen(args, @path).first.split.last
           end
         end
       end
@@ -792,9 +792,7 @@ module Gitlab
         end
 
         command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z]
-        message, status = Gitlab::Popen.popen(
-          command,
-          path) do |stdin|
+        message, status = popen(command, path) do |stdin|
           stdin.write(instructions.join)
         end
 
diff --git a/lib/gitlab/git/rev_list.rb b/lib/gitlab/git/rev_list.rb
index 2b5785a1f08..e0943d3a3eb 100644
--- a/lib/gitlab/git/rev_list.rb
+++ b/lib/gitlab/git/rev_list.rb
@@ -3,6 +3,8 @@
 module Gitlab
   module Git
     class RevList
+      include Gitlab::Git::Popen
+
       attr_reader :oldrev, :newrev, :path_to_repo
 
       def initialize(path_to_repo:, newrev:, oldrev: nil)
@@ -26,7 +28,7 @@ module Gitlab
       private
 
       def execute(args)
-        output, status = Gitlab::Popen.popen(args, nil, Gitlab::Git::Env.all.stringify_keys)
+        output, status = popen(args, nil, Gitlab::Git::Env.all.stringify_keys)
 
         unless status.zero?
           raise "Got a non-zero exit code while calling out `#{args.join(' ')}`."
diff --git a/spec/lib/gitlab/checks/force_push_spec.rb b/spec/lib/gitlab/checks/force_push_spec.rb
index f8c8b83a3ac..2c7ef622c51 100644
--- a/spec/lib/gitlab/checks/force_push_spec.rb
+++ b/spec/lib/gitlab/checks/force_push_spec.rb
@@ -5,13 +5,13 @@ describe Gitlab::Checks::ForcePush do
 
   context "exit code checking", skip_gitaly_mock: true do
     it "does not raise a runtime error if the `popen` call to git returns a zero exit code" do
-      allow(Gitlab::Popen).to receive(:popen).and_return(['normal output', 0])
+      allow_any_instance_of(Gitlab::Git::RevList).to receive(:popen).and_return(['normal output', 0])
 
       expect { described_class.force_push?(project, 'oldrev', 'newrev') }.not_to raise_error
     end
 
     it "raises a runtime error if the `popen` call to git returns a non-zero exit code" do
-      allow(Gitlab::Popen).to receive(:popen).and_return(['error', 1])
+      allow_any_instance_of(Gitlab::Git::RevList).to receive(:popen).and_return(['error', 1])
 
       expect { described_class.force_push?(project, 'oldrev', 'newrev') }.to raise_error(RuntimeError)
     end
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 556a148c3bc..4fc26c625a5 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -481,7 +481,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     it 'raises an error if it failed' do
-      expect(Gitlab::Popen).to receive(:popen).and_return(['Error', 1])
+      expect(@repo).to receive(:popen).and_return(['Error', 1])
 
       expect do
         @repo.delete_refs('refs/heads/fix')
diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb
index b051a088171..c0eac98d718 100644
--- a/spec/lib/gitlab/git/rev_list_spec.rb
+++ b/spec/lib/gitlab/git/rev_list_spec.rb
@@ -14,7 +14,7 @@ describe Gitlab::Git::RevList do
     let(:rev_list) { described_class.new(newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
 
     it 'calls out to `popen`' do
-      expect(Gitlab::Popen).to receive(:popen).with([
+      expect(rev_list).to receive(:popen).with([
         Gitlab.config.git.bin_path,
         "--git-dir=#{project.repository.path_to_repo}",
         'rev-list',
@@ -36,7 +36,7 @@ describe Gitlab::Git::RevList do
     let(:rev_list) { described_class.new(oldrev: 'oldrev', newrev: 'newrev', path_to_repo: project.repository.path_to_repo) }
 
     it 'calls out to `popen`' do
-      expect(Gitlab::Popen).to receive(:popen).with([
+      expect(rev_list).to receive(:popen).with([
         Gitlab.config.git.bin_path,
         "--git-dir=#{project.repository.path_to_repo}",
         'rev-list',