From 3d789f7c5595dca359d14cd94d9ed1610f1d7588 Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Mon, 25 Jun 2018 11:07:21 -0700 Subject: [PATCH 1/7] [WIP]Start replacing Dir.chdir with paths Instead of using chdir blocks, we should use absolute paths so that we can enable threaded usage of the library Fixes #355 --- lib/git.rb | 2 +- lib/git/base.rb | 4 +--- lib/git/lib.rb | 14 +++----------- lib/git/status.rb | 16 ++++++++-------- 4 files changed, 13 insertions(+), 23 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..41e3a546 100644 --- a/lib/git/base.rb +++ b/lib/git/base.rb @@ -144,9 +144,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) diff --git a/lib/git/lib.rb b/lib/git/lib.rb index 17fe8f15..8cbffce3 100644 --- a/lib/git/lib.rb +++ b/lib/git/lib.rb @@ -310,7 +310,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) } files end @@ -442,11 +442,7 @@ def config_get(name) command('config', ['--get', name]) end - if @git_dir - Dir.chdir(@git_dir, &do_get) - else - build_list.call - end + do_get.call(@git_dir) end def global_config_get(name) @@ -458,11 +454,7 @@ def config_list parse_config_list command_lines('config', ['--list']) end - if @git_dir - Dir.chdir(@git_dir, &build_list) - else - build_list.call - end + build_list.call(@git_dir) end def global_config_list 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 From f3f75188233c0b40ec8a5ade8283de198d8554ce Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Mon, 25 Jun 2018 14:37:13 -0700 Subject: [PATCH 2/7] fork() when using Dir.chdir, otherwise, use a mutex Signed-off-by: Joshua Liebowitz --- lib/git/base.rb | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/lib/git/base.rb b/lib/git/base.rb index 41e3a546..552d1c4b 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 + class << self + attr_accessor :chdir_semaphore + end + Git::Base.chdir_semaphore = Mutex.new 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 + # Windows and NetBSD 4 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 From df31b573da5806b10d4fa0c243bf865d155b831c Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Mon, 25 Jun 2018 14:37:13 -0700 Subject: [PATCH 3/7] fork() when using Dir.chdir, otherwise, use a mutex Signed-off-by: Joshua Liebowitz --- lib/git/base.rb | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/lib/git/base.rb b/lib/git/base.rb index 41e3a546..552d1c4b 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 + class << self + attr_accessor :chdir_semaphore + end + Git::Base.chdir_semaphore = Mutex.new 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 + # Windows and NetBSD 4 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 From 22ae6097c928ec579cbe28be4c8bd9165c4a7ed4 Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Mon, 25 Jun 2018 16:24:32 -0700 Subject: [PATCH 4/7] wrap calls to in a semaphore Signed-off-by: Joshua Liebowitz --- lib/git/lib.rb | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/lib/git/lib.rb b/lib/git/lib.rb index 87bd85b2..bf44fa62 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 @@ -438,11 +439,17 @@ 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 - do_get.call(@git_dir) + if @git_dir + Dir.chdir(@git_dir, &do_get) + else + do_get.call + end + end end def global_config_get(name) @@ -450,11 +457,17 @@ 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 - build_list.call(@git_dir) + if @git_dir + Dir.chdir(@git_dir, &build_list) + else + build_list.call + end + end end def global_config_list From 0d325760c5c3699777b070cecd7144f401047245 Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Mon, 13 Aug 2018 10:53:45 -0700 Subject: [PATCH 5/7] Revert "355" --- lib/git/lib.rb | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/lib/git/lib.rb b/lib/git/lib.rb index 68e00374..fed051c9 100644 --- a/lib/git/lib.rb +++ b/lib/git/lib.rb @@ -8,7 +8,6 @@ class GitExecuteError < StandardError class Lib @@semaphore = Mutex.new - @@config_semaphore = Mutex.new def initialize(base = nil, logger = nil) @git_dir = nil @@ -442,17 +441,11 @@ def config_remote(name) end def config_get(name) - @@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 - end + do_get = lambda do |path| + command('config', ['--get', name]) end + + do_get.call(@git_dir) end def global_config_get(name) @@ -460,17 +453,11 @@ def global_config_get(name) end def config_list - @@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 - end + build_list = lambda do |path| + parse_config_list command_lines('config', ['--list']) end + + build_list.call(@git_dir) end def global_config_list From 1cdc5fd2cfaf476cdeb576722f8fb7cef679feca Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Mon, 13 Aug 2018 10:54:13 -0700 Subject: [PATCH 6/7] Revert "355" --- lib/git.rb | 2 +- lib/git/base.rb | 39 ++++++++------------------------------- lib/git/lib.rb | 14 +++++++++++--- lib/git/status.rb | 16 ++++++++-------- 4 files changed, 28 insertions(+), 43 deletions(-) diff --git a/lib/git.rb b/lib/git.rb index 2300922a..1992dc1d 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] - FileUtils.rm_r(File.join(repo.dir.to_s, '.git')) + Dir.chdir(repo.dir.to_s) { FileUtils.rm_r '.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 552d1c4b..068d7931 100644 --- a/lib/git/base.rb +++ b/lib/git/base.rb @@ -3,12 +3,6 @@ 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 - class << self - attr_accessor :chdir_semaphore - end - Git::Base.chdir_semaphore = Mutex.new include Git::Base::Factory @@ -98,11 +92,8 @@ 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. - # - # 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 + # changes current working directory for a block + # to the git working directory # # example # @git.chdir do @@ -110,25 +101,9 @@ def initialize(options = {}) # @git.add # @git.commit('message') # end - 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 - # Windows and NetBSD 4 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 + def chdir # :yields: the Git::Path + Dir.chdir(dir.path) do + yield dir.path end end @@ -169,7 +144,9 @@ def repo # returns the repository size in bytes def repo_size - return `du -s #{repo.path}`.chomp.split.first.to_i + Dir.chdir(repo.path) do + return `du -s`.chomp.split.first.to_i + end end def set_index(index_file, check = true) diff --git a/lib/git/lib.rb b/lib/git/lib.rb index fed051c9..fc390af5 100644 --- a/lib/git/lib.rb +++ b/lib/git/lib.rb @@ -310,7 +310,7 @@ def branches_all def list_files(ref_dir) dir = File.join(@git_dir, 'refs', ref_dir) files = [] - files = Dir.glob(File.join(dir, '**/*')).select { |f| File.file?(f) } + Dir.chdir(dir) { files = Dir.glob('**/*').select { |f| File.file?(f) } } rescue nil files end @@ -445,7 +445,11 @@ def config_get(name) command('config', ['--get', name]) end - do_get.call(@git_dir) + if @git_dir + Dir.chdir(@git_dir, &do_get) + else + do_get.call + end end def global_config_get(name) @@ -457,7 +461,11 @@ def config_list parse_config_list command_lines('config', ['--list']) end - build_list.call(@git_dir) + if @git_dir + Dir.chdir(@git_dir, &build_list) + else + build_list.call + end end def global_config_list diff --git a/lib/git/status.rb b/lib/git/status.rb index 93100543..23050e08 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 - 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 } + + 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 end end From 0fb979861dbaf9c9e42d815b8dfe02cf9cdfdac9 Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Mon, 13 Aug 2018 11:42:41 -0700 Subject: [PATCH 7/7] Isolate Dir.chdir to a new process, or mutex 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. Fixes #355 Note: if you do async work in git.chdir all bets are off. - Reduce and isolate usage of Dir.chdir - fork() when using Dir.chdir, otherwise, use a mutex - wrap calls to in a semaphore - replace Dir.chdir with paths 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