From 375e5f3a7dfecf766d2f4910d778a98574bc12e2 Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Mon, 13 Aug 2018 12:11:58 -0700 Subject: [PATCH] Reduce and isolate usage of Dir.chdir Instead of using chdir blocks, we should use absolute paths so that we can enable threaded usage of the library If Process.fork() is available, we'll use that for git.chdir actions, otherwise, we'll use a global mutex to prevent threads from calling git.chdir before the previous execution has completed. - fork() when using Dir.chdir, otherwise, use a mutex - wrap calls to in a semaphore - replace Dir.chdir with paths - fixes #355 Note: if you do async work in git.chdir all bets are off. Signed-off-by: Joshua Liebowitz --- lib/git.rb | 2 +- lib/git/base.rb | 41 ++++++++++++++++++++++++++++++++--------- lib/git/lib.rb | 35 ++++++++++++++++++++--------------- lib/git/status.rb | 16 ++++++++-------- 4 files changed, 61 insertions(+), 33 deletions(-) diff --git a/lib/git.rb b/lib/git.rb index 1992dc1d..2300922a 100644 --- a/lib/git.rb +++ b/lib/git.rb @@ -108,7 +108,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 068d7931..703e2069 100644 --- a/lib/git/base.rb +++ b/lib/git/base.rb @@ -3,6 +3,12 @@ module Git class Base + # Adding a mutex to the class because each repo should be sharing the same mutex + # in case we need to Dir.chdir and we don't have fork() support to isolate that + @chdir_semaphore = Mutex.new + class << self + attr_accessor :chdir_semaphore + end include Git::Base::Factory @@ -92,8 +98,11 @@ def initialize(options = {}) @index = options[:index] ? Git::Index.new(options[:index], false) : nil end - # changes current working directory for a block - # to the git working directory + # changes current working directory for a block to the git working directory. + # + # Note: If we can fork() or spawn(), Dir.chdir will happen in a new process + # otherwise, we will use a mutex to prevent threading errors + # See https://github.com/ruby-git/ruby-git/issues/355 for more info # # example # @git.chdir do @@ -101,9 +110,25 @@ def initialize(options = {}) # @git.add # @git.commit('message') # end - def chdir # :yields: the Git::Path - Dir.chdir(dir.path) do - yield dir.path + def chdir(&block) # :yields: the Git::Path + chdir_block = Proc.new do + Dir.chdir(dir.path) do + block.call(dir.path) + end + end + + if Process.respond_to?(:fork) + # Forking this process so that we can be threadsafe + pid = Process.fork do + chdir_block.call + end + Process.wait(pid) + else + # JRuby and Windows don't support fork() + # let's use a mutex to prevent race conditions with threads + Git::Base.chdir_semaphore.synchronize do + chdir_block.call + end end end @@ -144,9 +169,7 @@ def repo # returns the repository size in bytes def repo_size - Dir.chdir(repo.path) do - return `du -s`.chomp.split.first.to_i - end + return `du -s #{repo.path}`.chomp.split.first.to_i end def set_index(index_file, check = true) @@ -348,7 +371,7 @@ def each_conflict(&block) # :yields: file, your_version, their_version # @git.pull('upstream', 'develope') # pulls from upstream/develop # def pull(remote='origin', branch='master') - self.lib.pull(remote, branch) + self.lib.pull(remote, branch) end # returns an array of Git:Remote objects diff --git a/lib/git/lib.rb b/lib/git/lib.rb index fc390af5..307b7061 100644 --- a/lib/git/lib.rb +++ b/lib/git/lib.rb @@ -8,6 +8,7 @@ class GitExecuteError < StandardError class Lib @@semaphore = Mutex.new + @@config_semaphore = Mutex.new def initialize(base = nil, logger = nil) @git_dir = nil @@ -310,7 +311,7 @@ def branches_all 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 + files = Dir.glob(File.join(dir, '**/*')).select { |f| File.file?(f) } rescue nil files end @@ -441,14 +442,16 @@ def config_remote(name) end def config_get(name) - do_get = lambda do |path| - command('config', ['--get', name]) - end + @@config_semaphore.synchronize do + do_get = lambda do |path| + command('config', ['--get', name]) + end - if @git_dir - Dir.chdir(@git_dir, &do_get) - else - do_get.call + if @git_dir + Dir.chdir(@git_dir, &do_get) + else + do_get.call + end end end @@ -457,14 +460,16 @@ def global_config_get(name) end def config_list - build_list = lambda do |path| - parse_config_list command_lines('config', ['--list']) - end + @@config_semaphore.synchronize do + build_list = lambda do |path| + parse_config_list command_lines('config', ['--list']) + end - if @git_dir - Dir.chdir(@git_dir, &build_list) - else - build_list.call + if @git_dir + Dir.chdir(@git_dir, &build_list) + else + build_list.call + end end end diff --git a/lib/git/status.rb b/lib/git/status.rb index 23050e08..93100543 100644 --- a/lib/git/status.rb +++ b/lib/git/status.rb @@ -171,14 +171,14 @@ 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\/.+} - - @files[file] = { path: file, untracked: true } - end + root_base_dir_path_length = File.join(@base.dir.path, '/').length + Dir.glob(File.join(@base.dir.path, '**/*'), File::FNM_DOTMATCH) do |file| + # Strip off the `base.dir.path` to make these relative paths + relative_file_name = file.slice(root_base_dir_path_length..file.length) + next if @files[relative_file_name] || File.directory?(file) || + ignore.include?(relative_file_name) || relative_file_name =~ %r{^.git\/.+} + + @files[relative_file_name] = { path: relative_file_name, untracked: true } end end