diff options
-rw-r--r-- | changelogs/unreleased/gitlab-git-unit-tests.yml | 5 | ||||
-rw-r--r-- | internal/git/command.go | 9 | ||||
-rw-r--r-- | internal/rubyserver/rubyserver.go | 4 | ||||
-rw-r--r-- | ruby/lib/gitlab/git/popen.rb | 3 | ||||
-rw-r--r-- | ruby/spec/lib/gitlab/git/popen_spec.rb | 163 |
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 |