Merge branch 'loc-fixes' into 'master'

Force english output on git commands

See merge request gitlab-org/gitaly!898

5 files changed, 182 insertions, 2 deletions

diff --git a/changelogs/unreleased/gitlab-git-unit-tests.yml b/changelogs/unreleased/gitlab-git-unit-tests.yml
new file mode 100644
index 000000000..6bfca1c85
--- /dev/null
+++ b/changelogs/unreleased/gitlab-git-unit-tests.yml
@@ -0,0 +1,5 @@
+---
+title: Force english output on git commands
+merge_request: 898
+author:
+type: other
diff --git a/internal/git/command.go b/internal/git/command.go
index 45f569c37..408221010 100644
--- a/internal/git/command.go
+++ b/internal/git/command.go
@@ -9,9 +9,18 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
 )
 
+// GitEnv contains the ENV variables for git commands
+var GitEnv = []string{
+	// Force english locale for consistency on the output messages
+	"LANG=en_US.UTF-8",
+}
+
 // Command creates a git.Command with the given args
 func Command(ctx context.Context, repo *gitalypb.Repository, args ...string) (*command.Command, error) {
 	repoPath, env, err := alternates.PathAndEnv(repo)
+
+	env = append(env, GitEnv...)
+
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index 7b7cedbbd..fa4d9db9b 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -14,6 +14,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver/balancer"
 	"gitlab.com/gitlab-org/gitaly/internal/supervisor"
@@ -109,6 +110,9 @@ func Start() (*Server, error) {
 		"GITALY_RUBY_GITALY_BIN_DIR="+cfg.BinDir,
 		"GITALY_VERSION="+version.GetVersion(),
 	)
+
+	env = append(env, git.GitEnv...)
+
 	if dsn := cfg.Logging.RubySentryDSN; dsn != "" {
 		env = append(env, "SENTRY_DSN="+dsn)
 	}
diff --git a/ruby/lib/gitlab/git/popen.rb b/ruby/lib/gitlab/git/popen.rb
index 7426688fc..53996b682 100644
--- a/ruby/lib/gitlab/git/popen.rb
+++ b/ruby/lib/gitlab/git/popen.rb
@@ -1,5 +1,3 @@
-# Gitaly note: JV: no RPC's here.
-
 require 'open3'
 
 module Gitlab
@@ -20,6 +18,7 @@ module Gitlab
         cmd_status = 0
         Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
           stdout.set_encoding(Encoding::ASCII_8BIT)
+          stderr.set_encoding(Encoding::ASCII_8BIT)
 
           # stderr and stdout pipes can block if stderr/stdout aren't drained: https://bugs.ruby-lang.org/issues/9082
           # Mimic what Ruby does with capture3: https://github.com/ruby/ruby/blob/1ec544695fa02d714180ef9c34e755027b6a2103/lib/open3.rb#L257-L273
diff --git a/ruby/spec/lib/gitlab/git/popen_spec.rb b/ruby/spec/lib/gitlab/git/popen_spec.rb
new file mode 100644
index 000000000..72321b01d
--- /dev/null
+++ b/ruby/spec/lib/gitlab/git/popen_spec.rb
@@ -0,0 +1,163 @@
+require 'spec_helper'
+
+describe 'Gitlab::Git::Popen' do
+  let(:path) { Dir.mktmpdir }
+
+  let(:klass) do
+    Class.new(Object) do
+      include Gitlab::Git::Popen
+    end
+  end
+
+  after do
+    FileUtils.remove_entry path
+  end
+
+  context 'popen' do
+    context 'zero status' do
+      let(:result) { klass.new.popen(%w(ls), path) }
+      let(:status) { result.last }
+
+      it { expect(status).to be_zero }
+    end
+
+    context 'non-zero status' do
+      let(:result) { klass.new.popen(%w(cat NOTHING), path) }
+      let(:output) { result.first }
+      let(:status) { result.last }
+
+      it { expect(status).to eq(1) }
+      it { expect(output).to include('No such file or directory') }
+    end
+
+    context 'unsafe string command' do
+      it 'raises an error when it gets called with a string argument' do
+        expect { klass.new.popen('ls', path) }.to raise_error(RuntimeError)
+      end
+    end
+
+    context 'with custom options' do
+      let(:vars) { { 'foobar' => 123, 'PWD' => path } }
+      let(:options) { { chdir: path } }
+
+      it 'calls popen3 with the provided environment variables' do
+        expect(Open3).to receive(:popen3).with(vars, 'ls', options)
+
+        klass.new.popen(%w(ls), path, { 'foobar' => 123 })
+      end
+    end
+
+    context 'use stdin' do
+      let(:result) { klass.new.popen(%w[cat], path) { |stdin| stdin.write 'hello' } }
+      let(:output) { result.first }
+      let(:status) { result.last }
+
+      it { expect(status).to be_zero }
+      it { expect(output).to eq('hello') }
+    end
+
+    context 'with lazy block' do
+      it 'yields a lazy io' do
+        expect_lazy_io = lambda do |io|
+          expect(io).to be_a Enumerator::Lazy
+          expect(io.inspect).to include('#<IO:fd')
+        end
+
+        klass.new.popen(%w[ls], path, lazy_block: expect_lazy_io)
+      end
+
+      it "doesn't wait for process exit" do
+        Timeout.timeout(2) do
+          klass.new.popen(%w[yes], path, lazy_block: ->(io) {})
+        end
+      end
+    end
+
+    context 'with non ASCII output' do
+      let(:stdin) { StringIO.new }
+      let(:stdout) { StringIO.new("Preparando \xC3\xA1rbol de trabajo") }
+      let(:stderr) { StringIO.new("UTF-8 error é").set_encoding('UTF-8') }
+      let(:process_status) { double('Process::Status', exitstatus: 0) }
+      let(:wait_thr) { double('Process::Waiter', value: process_status) }
+
+      it "handles the output correctly" do
+        expect(Open3).to receive(:popen3).and_yield(stdin, stdout, stderr, wait_thr)
+
+        klass.new.popen(%w[ls], path)
+      end
+    end
+  end
+
+  context 'popen_with_timeout' do
+    let(:timeout) { 1.second }
+
+    context 'no timeout' do
+      context 'zero status' do
+        let(:result) { klass.new.popen_with_timeout(%w(ls), timeout, path) }
+        let(:status) { result.last }
+
+        it { expect(status).to be_zero }
+      end
+
+      context 'non-zero status' do
+        let(:result) { klass.new.popen_with_timeout(%w(cat NOTHING), timeout, path) }
+        let(:output) { result.first }
+        let(:status) { result.last }
+
+        it { expect(status).to eq(1) }
+        it { expect(output).to include('No such file or directory') }
+      end
+
+      context 'unsafe string command' do
+        it 'raises an error when it gets called with a string argument' do
+          expect { klass.new.popen_with_timeout('ls', timeout, path) }.to raise_error(RuntimeError)
+        end
+      end
+    end
+
+    context 'timeout' do
+      context 'timeout' do
+        it "raises a Timeout::Error" do
+          expect { klass.new.popen_with_timeout(%w(sleep 1000), timeout, path) }.to raise_error(Timeout::Error)
+        end
+
+        it "handles processes that do not shutdown correctly" do
+          expect { klass.new.popen_with_timeout(['bash', '-c', "trap -- '' SIGTERM; sleep 1000"], timeout, path) }.to raise_error(Timeout::Error)
+        end
+      end
+
+      context 'timeout period' do
+        let(:time_taken) do
+          begin
+            start = Time.now
+            klass.new.popen_with_timeout(%w(sleep 1000), timeout, path)
+          rescue
+            Time.now - start
+          end
+        end
+
+        it { expect(time_taken).to be >= timeout }
+      end
+
+      context 'clean up' do
+        let(:instance) { klass.new }
+
+        it 'kills the child process' do
+          expect(instance).to receive(:kill_process_group_for_pid).and_wrap_original do |m, *args|
+            # is the PID, and it should not be running at this point
+            m.call(*args)
+
+            pid = args.first
+            begin
+              Process.getpgid(pid)
+              raise "The child process should have been killed"
+            rescue Errno::ESRCH
+            end
+          end
+
+          expect { instance.popen_with_timeout(['bash', '-c', "trap -- '' SIGTERM; sleep 1000"], timeout, path) }.to raise_error(Timeout::Error)
+        end
+      end
+    end
+  end
+end