From 65c0cc6ccd78efa62e1c2aba0e5fda3c5ab9dee7 Mon Sep 17 00:00:00 2001 From: Pavel Forkert Date: Fri, 20 Oct 2023 18:38:59 +0300 Subject: [PATCH] Remove calls to Dir.chdir In multithreaded environment Dir.chdir changes current directory of all process' threads, so other threads might misbehave. Base#chdir, Base#with_working and Base#with_temp_working were left intact, cause they are not used internally, so it's an explicit user decision to use them. Signed-off-by: Pavel Forkert --- lib/git.rb | 2 +- lib/git/base.rb | 11 ++++--- lib/git/lib.rb | 48 +++++++++++++---------------- lib/git/status.rb | 11 +++---- tests/units/test_commit_with_gpg.rb | 12 ++++---- tests/units/test_lib.rb | 2 +- 6 files changed, 40 insertions(+), 46 deletions(-) diff --git a/lib/git.rb b/lib/git.rb index 63e1f3b1..e75ff189 100644 --- a/lib/git.rb +++ b/lib/git.rb @@ -244,7 +244,7 @@ def self.export(repository, name, options = {}) options.delete(:remote) repo = clone(repository, name, {:depth => 1}.merge(options)) repo.checkout("origin/#{options[:branch]}") if options[:branch] - Dir.chdir(repo.dir.to_s) { FileUtils.rm_r '.git' } + FileUtils.rm_r File.join(repo.dir.to_s, '.git') end # Same as g.config, but forces it to be at the global level diff --git a/lib/git/base.rb b/lib/git/base.rb index 6b468d07..93dcf16e 100644 --- a/lib/git/base.rb +++ b/lib/git/base.rb @@ -1,5 +1,6 @@ require 'git/base/factory' require 'logger' +require 'open3' module Git # Git::Base is the main public interface for interacting with Git commands. @@ -66,11 +67,11 @@ def self.init(directory = '.', options = {}) def self.root_of_worktree(working_dir) result = working_dir status = nil - Dir.chdir(working_dir) do - git_cmd = "#{Git::Base.config.binary_path} -c core.quotePath=true -c color.ui=false rev-parse --show-toplevel 2>&1" - result = `#{git_cmd}`.chomp - status = $? - end + + git_cmd = "#{Git::Base.config.binary_path} -c core.quotePath=true -c color.ui=false rev-parse --show-toplevel 2>&1" + result, status = Open3.capture2(git_cmd, chdir: File.expand_path(working_dir)) + result = result.chomp + raise ArgumentError, "'#{working_dir}' is not in a git working tree" unless status.success? result end diff --git a/lib/git/lib.rb b/lib/git/lib.rb index 335b45b6..86c34a85 100644 --- a/lib/git/lib.rb +++ b/lib/git/lib.rb @@ -2,6 +2,7 @@ require 'logger' require 'tempfile' require 'zlib' +require 'open3' module Git class Lib @@ -441,7 +442,10 @@ def worktree_prune def list_files(ref_dir) dir = File.join(@git_dir, 'refs', ref_dir) files = [] - Dir.chdir(dir) { files = Dir.glob('**/*').select { |f| File.file?(f) } } rescue nil + begin + files = Dir.glob('**/*', base: dir).select { |f| File.file?(File.join(dir, f)) } + rescue + end files end @@ -579,15 +583,7 @@ def config_remote(name) end def config_get(name) - do_get = Proc.new do |path| - command('config', '--get', name) - end - - if @git_dir - Dir.chdir(@git_dir, &do_get) - else - do_get.call - end + command('config', '--get', name, chdir: @git_dir) end def global_config_get(name) @@ -595,15 +591,7 @@ def global_config_get(name) end def config_list - build_list = Proc.new do |path| - parse_config_list command_lines('config', '--list') - end - - if @git_dir - Dir.chdir(@git_dir, &build_list) - else - build_list.call - end + parse_config_list command_lines('config', '--list', chdir: @git_dir) end def global_config_list @@ -1148,8 +1136,8 @@ def self.warn_if_old_command(lib) # @return [] the names of the EVN variables involved in the git commands ENV_VARIABLE_NAMES = ['GIT_DIR', 'GIT_WORK_TREE', 'GIT_INDEX_FILE', 'GIT_SSH'] - def command_lines(cmd, *opts) - cmd_op = command(cmd, *opts) + def command_lines(cmd, *opts, chdir: nil) + cmd_op = command(cmd, *opts, chdir: chdir) if cmd_op.encoding.name != "UTF-8" op = cmd_op.encode("UTF-8", "binary", :invalid => :replace, :undef => :replace) else @@ -1195,7 +1183,7 @@ def with_custom_env_variables(&block) restore_git_system_env_variables() end - def command(*cmd, redirect: '', chomp: true, &block) + def command(*cmd, redirect: '', chomp: true, chdir: nil, &block) Git::Lib.warn_if_old_command(self) raise 'cmd can not include a nested array' if cmd.any? { |o| o.is_a? Array } @@ -1220,8 +1208,7 @@ def command(*cmd, redirect: '', chomp: true, &block) with_custom_env_variables do command_thread = Thread.new do - output = run_command(git_cmd, &block) - status = $? + output, status = run_command(git_cmd, chdir, &block) end command_thread.join end @@ -1303,10 +1290,17 @@ def log_path_options(opts) arr_opts end - def run_command(git_cmd, &block) - return IO.popen(git_cmd, &block) if block_given? + def run_command(git_cmd, chdir=nil, &block) + block ||= Proc.new do |io| + io.readlines.map { |l| Git::EncodingUtils.normalize_encoding(l) }.join + end + + opts = {} + opts[:chdir] = File.expand_path(chdir) if chdir - `#{git_cmd}`.lines.map { |l| Git::EncodingUtils.normalize_encoding(l) }.join + Open3.popen2(git_cmd, opts) do |stdin, stdout, wait_thr| + [block.call(stdout), wait_thr.value] + end end def escape(s) diff --git a/lib/git/status.rb b/lib/git/status.rb index fff67868..3f741bfd 100644 --- a/lib/git/status.rb +++ b/lib/git/status.rb @@ -172,13 +172,12 @@ def construct_status def fetch_untracked ignore = @base.lib.ignored_files - Dir.chdir(@base.dir.path) do - Dir.glob('**/*', File::FNM_DOTMATCH) do |file| - next if @files[file] || File.directory?(file) || - ignore.include?(file) || file =~ %r{^.git\/.+} + root_dir = @base.dir.path + Dir.glob('**/*', File::FNM_DOTMATCH, base: root_dir) do |file| + next if @files[file] || File.directory?(File.join(root_dir, file)) || + ignore.include?(file) || file =~ %r{^.git\/.+} - @files[file] = { path: file, untracked: true } - end + @files[file] = { path: file, untracked: true } end end diff --git a/tests/units/test_commit_with_gpg.rb b/tests/units/test_commit_with_gpg.rb index f9e8bb28..10eae678 100644 --- a/tests/units/test_commit_with_gpg.rb +++ b/tests/units/test_commit_with_gpg.rb @@ -11,9 +11,9 @@ def test_with_configured_gpg_keyid Dir.mktmpdir do |dir| git = Git.init(dir) actual_cmd = nil - git.lib.define_singleton_method(:run_command) do |git_cmd, &block| + git.lib.define_singleton_method(:run_command) do |git_cmd, chdir, &block| actual_cmd = git_cmd - `true` + [`true`, $?] end message = 'My commit message' git.commit(message, gpg_sign: true) @@ -25,9 +25,9 @@ def test_with_specific_gpg_keyid Dir.mktmpdir do |dir| git = Git.init(dir) actual_cmd = nil - git.lib.define_singleton_method(:run_command) do |git_cmd, &block| + git.lib.define_singleton_method(:run_command) do |git_cmd, chdir, &block| actual_cmd = git_cmd - `true` + [`true`, $?] end message = 'My commit message' git.commit(message, gpg_sign: 'keykeykey') @@ -39,9 +39,9 @@ def test_disabling_gpg_sign Dir.mktmpdir do |dir| git = Git.init(dir) actual_cmd = nil - git.lib.define_singleton_method(:run_command) do |git_cmd, &block| + git.lib.define_singleton_method(:run_command) do |git_cmd, chdir, &block| actual_cmd = git_cmd - `true` + [`true`, $?] end message = 'My commit message' git.commit(message, no_gpg_sign: true) diff --git a/tests/units/test_lib.rb b/tests/units/test_lib.rb index c7283d4e..b5502efd 100644 --- a/tests/units/test_lib.rb +++ b/tests/units/test_lib.rb @@ -91,7 +91,7 @@ def test_checkout_with_start_point assert(@lib.reset(nil, hard: true)) # to get around worktree status on windows actual_cmd = nil - @lib.define_singleton_method(:run_command) do |git_cmd, &block| + @lib.define_singleton_method(:run_command) do |git_cmd, chdir, &block| actual_cmd = git_cmd super(git_cmd, &block) end