From af62404c5a96462b0910f1e55e4defd4e45416cb Mon Sep 17 00:00:00 2001 From: Elton Gao Date: Thu, 31 Aug 2023 16:54:52 -0400 Subject: [PATCH 1/8] Replace two places that can call chdir in thread with `cd dir` While cd will change where the current dir is, because it happens in a thread it should not impact the subsequent operations. This is also what used be before https://github.com/square/git-fastclone/pull/53 is landed --- Gemfile.lock | 2 +- lib/git-fastclone.rb | 16 +++++++--------- lib/git-fastclone/version.rb | 2 +- spec/git_fastclone_runner_spec.rb | 2 +- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index b325642..08d8e89 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - git-fastclone (1.4.2) + git-fastclone (1.4.3) colorize GEM diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb index d68403b..f27aa40 100644 --- a/lib/git-fastclone.rb +++ b/lib/git-fastclone.rb @@ -270,11 +270,10 @@ def update_submodules(pwd, url) def thread_update_submodule(submodule_url, submodule_path, threads, pwd) threads << Thread.new do with_git_mirror(submodule_url) do |mirror, _| - Dir.chdir(File.join(abs_clone_path, pwd).to_s) do - cmd = ['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, - submodule_path.to_s].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) - end + fail_on_error(['cd', File.join(abs_clone_path, pwd).to_s], quiet: !verbose) + cmd = ['cd', File.join(abs_clone_path, pwd).to_s, '&&', 'git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, + submodule_path.to_s].compact + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) end update_submodules(File.join(pwd, submodule_path), submodule_url) @@ -350,10 +349,9 @@ def store_updated_repo(url, mirror, repo_name, fail_hard) quiet: !verbose, print_on_failure: print_git_errors) end - Dir.chdir(mirror) do - cmd = ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) - end + cmd = ['cd', mirror, '&&', 'git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) + reference_updated[repo_name] = true rescue RunnerExecutionRuntimeError => e # To avoid corruption of the cache, if we failed to update or check out we remove diff --git a/lib/git-fastclone/version.rb b/lib/git-fastclone/version.rb index d7bf002..3b2bd80 100644 --- a/lib/git-fastclone/version.rb +++ b/lib/git-fastclone/version.rb @@ -2,5 +2,5 @@ # Version string for git-fastclone module GitFastCloneVersion - VERSION = '1.4.2' + VERSION = '1.4.3' end diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb index 8148502..a3e4f1b 100644 --- a/spec/git_fastclone_runner_spec.rb +++ b/spec/git_fastclone_runner_spec.rb @@ -404,7 +404,7 @@ def clone_cmds(verbose: false) [ ['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', test_url_valid, test_reference_repo_dir], - ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact + ['cd', test_reference_repo_dir, '&&', 'git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact ] end From 89f0afe473d25c6048152a10de9505c3be57ddf1 Mon Sep 17 00:00:00 2001 From: Elton Gao Date: Thu, 31 Aug 2023 16:59:50 -0400 Subject: [PATCH 2/8] rubocop --- lib/git-fastclone.rb | 4 ++-- spec/git_fastclone_runner_spec.rb | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb index f27aa40..f26018e 100644 --- a/lib/git-fastclone.rb +++ b/lib/git-fastclone.rb @@ -271,8 +271,8 @@ def thread_update_submodule(submodule_url, submodule_path, threads, pwd) threads << Thread.new do with_git_mirror(submodule_url) do |mirror, _| fail_on_error(['cd', File.join(abs_clone_path, pwd).to_s], quiet: !verbose) - cmd = ['cd', File.join(abs_clone_path, pwd).to_s, '&&', 'git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, - submodule_path.to_s].compact + cmd = ['cd', File.join(abs_clone_path, pwd).to_s, '&&', 'git', 'submodule', + verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) end diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb index a3e4f1b..ba583be 100644 --- a/spec/git_fastclone_runner_spec.rb +++ b/spec/git_fastclone_runner_spec.rb @@ -404,7 +404,8 @@ def clone_cmds(verbose: false) [ ['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', test_url_valid, test_reference_repo_dir], - ['cd', test_reference_repo_dir, '&&', 'git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact + ['cd', test_reference_repo_dir, '&&', 'git', 'remote', verbose ? '--verbose' : nil, 'update', + '--prune'].compact ] end From 565a0b3894b597303e44c0701a97470bad4eace2 Mon Sep 17 00:00:00 2001 From: Elton Gao Date: Thu, 31 Aug 2023 17:30:14 -0400 Subject: [PATCH 3/8] fixup --- lib/git-fastclone.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb index f26018e..29cf689 100644 --- a/lib/git-fastclone.rb +++ b/lib/git-fastclone.rb @@ -270,7 +270,6 @@ def update_submodules(pwd, url) def thread_update_submodule(submodule_url, submodule_path, threads, pwd) threads << Thread.new do with_git_mirror(submodule_url) do |mirror, _| - fail_on_error(['cd', File.join(abs_clone_path, pwd).to_s], quiet: !verbose) cmd = ['cd', File.join(abs_clone_path, pwd).to_s, '&&', 'git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) From 9e7c8bd25f6e27cb7bd240d136f372cfa0e06f7e Mon Sep 17 00:00:00 2001 From: Elton Gao Date: Thu, 31 Aug 2023 17:41:31 -0400 Subject: [PATCH 4/8] trial --- lib/git-fastclone.rb | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb index 29cf689..ac0d7d4 100644 --- a/lib/git-fastclone.rb +++ b/lib/git-fastclone.rb @@ -248,7 +248,6 @@ def update_submodules(pwd, url) puts 'Updating submodules...' if verbose - threads = [] submodule_url_list = [] output = '' Dir.chdir(File.join(abs_clone_path, pwd).to_s) do @@ -260,23 +259,19 @@ def update_submodules(pwd, url) submodule_path, submodule_url = parse_update_info(line) submodule_url_list << submodule_url - thread_update_submodule(submodule_url, submodule_path, threads, pwd) - end - - update_submodule_reference(url, submodule_url_list) - threads.each(&:join) - end - def thread_update_submodule(submodule_url, submodule_path, threads, pwd) - threads << Thread.new do with_git_mirror(submodule_url) do |mirror, _| - cmd = ['cd', File.join(abs_clone_path, pwd).to_s, '&&', 'git', 'submodule', - verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) + Dir.chdir(File.join(abs_clone_path, pwd).to_s) do + cmd = ['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, + submodule_path.to_s].compact + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) + end end update_submodules(File.join(pwd, submodule_path), submodule_url) end + + update_submodule_reference(url, submodule_url_list) end def with_reference_repo_lock(url, &block) @@ -335,8 +330,7 @@ def update_reference_repo(url, fail_hard) # Grab the children in the event of a prefetch def prefetch(submodule_file) File.readlines(submodule_file).each do |line| - # We don't join these threads explicitly - Thread.new { update_reference_repo(line.strip, false) } + update_reference_repo(line.strip, false) end end @@ -347,9 +341,10 @@ def store_updated_repo(url, mirror, repo_name, fail_hard) fail_on_error('git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', url.to_s, mirror.to_s, quiet: !verbose, print_on_failure: print_git_errors) end - - cmd = ['cd', mirror, '&&', 'git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) + Dir.chdir(mirror) do + cmd = ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) + end reference_updated[repo_name] = true rescue RunnerExecutionRuntimeError => e From 3e6a0a3c9686c69978ca9fffa4d241ff40356d6f Mon Sep 17 00:00:00 2001 From: Elton Gao Date: Thu, 31 Aug 2023 23:53:29 -0400 Subject: [PATCH 5/8] Revert "trial" This reverts commit 9e7c8bd25f6e27cb7bd240d136f372cfa0e06f7e. --- lib/git-fastclone.rb | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb index ac0d7d4..29cf689 100644 --- a/lib/git-fastclone.rb +++ b/lib/git-fastclone.rb @@ -248,6 +248,7 @@ def update_submodules(pwd, url) puts 'Updating submodules...' if verbose + threads = [] submodule_url_list = [] output = '' Dir.chdir(File.join(abs_clone_path, pwd).to_s) do @@ -259,19 +260,23 @@ def update_submodules(pwd, url) submodule_path, submodule_url = parse_update_info(line) submodule_url_list << submodule_url + thread_update_submodule(submodule_url, submodule_path, threads, pwd) + end + + update_submodule_reference(url, submodule_url_list) + threads.each(&:join) + end + def thread_update_submodule(submodule_url, submodule_path, threads, pwd) + threads << Thread.new do with_git_mirror(submodule_url) do |mirror, _| - Dir.chdir(File.join(abs_clone_path, pwd).to_s) do - cmd = ['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, - submodule_path.to_s].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) - end + cmd = ['cd', File.join(abs_clone_path, pwd).to_s, '&&', 'git', 'submodule', + verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) end update_submodules(File.join(pwd, submodule_path), submodule_url) end - - update_submodule_reference(url, submodule_url_list) end def with_reference_repo_lock(url, &block) @@ -330,7 +335,8 @@ def update_reference_repo(url, fail_hard) # Grab the children in the event of a prefetch def prefetch(submodule_file) File.readlines(submodule_file).each do |line| - update_reference_repo(line.strip, false) + # We don't join these threads explicitly + Thread.new { update_reference_repo(line.strip, false) } end end @@ -341,10 +347,9 @@ def store_updated_repo(url, mirror, repo_name, fail_hard) fail_on_error('git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', url.to_s, mirror.to_s, quiet: !verbose, print_on_failure: print_git_errors) end - Dir.chdir(mirror) do - cmd = ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) - end + + cmd = ['cd', mirror, '&&', 'git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) reference_updated[repo_name] = true rescue RunnerExecutionRuntimeError => e From 52164f4a36d99a57e3b6ab88db5aef1663b65748 Mon Sep 17 00:00:00 2001 From: Elton Gao Date: Thu, 31 Aug 2023 23:56:22 -0400 Subject: [PATCH 6/8] try --- lib/git-fastclone.rb | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb index 29cf689..a8b60fb 100644 --- a/lib/git-fastclone.rb +++ b/lib/git-fastclone.rb @@ -225,10 +225,9 @@ def clone(url, rev, src_dir, config) # Only checkout if we're changing branches to a non-default branch if rev - Dir.chdir(File.join(abs_clone_path, src_dir)) do fail_on_error('git', 'checkout', '--quiet', rev.to_s, quiet: !verbose, - print_on_failure: print_git_errors) - end + print_on_failure: print_git_errors, + :chdir => File.join(abs_clone_path, src_dir)) end update_submodules(src_dir, url) @@ -251,10 +250,9 @@ def update_submodules(pwd, url) threads = [] submodule_url_list = [] output = '' - Dir.chdir(File.join(abs_clone_path, pwd).to_s) do - output = fail_on_error('git', 'submodule', 'init', quiet: !verbose, - print_on_failure: print_git_errors) - end + output = fail_on_error('git', 'submodule', 'init', quiet: !verbose, + print_on_failure: print_git_errors, + :chdir => File.join(abs_clone_path, pwd)) output.split("\n").each do |line| submodule_path, submodule_url = parse_update_info(line) @@ -270,9 +268,9 @@ def update_submodules(pwd, url) def thread_update_submodule(submodule_url, submodule_path, threads, pwd) threads << Thread.new do with_git_mirror(submodule_url) do |mirror, _| - cmd = ['cd', File.join(abs_clone_path, pwd).to_s, '&&', 'git', 'submodule', + cmd = ['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors, :chdir => File.join(abs_clone_path, pwd)) end update_submodules(File.join(pwd, submodule_path), submodule_url) @@ -348,8 +346,8 @@ def store_updated_repo(url, mirror, repo_name, fail_hard) quiet: !verbose, print_on_failure: print_git_errors) end - cmd = ['cd', mirror, '&&', 'git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors) + cmd = ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors, :chdir => mirror) reference_updated[repo_name] = true rescue RunnerExecutionRuntimeError => e From 87a300e71da0c4fe6efc7b62710ad6a68e123524 Mon Sep 17 00:00:00 2001 From: Elton Gao Date: Fri, 1 Sep 2023 00:11:39 -0400 Subject: [PATCH 7/8] rubocop --- lib/git-fastclone.rb | 15 ++++++++------- spec/git_fastclone_runner_spec.rb | 9 +++------ 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb index a8b60fb..c8ccd02 100644 --- a/lib/git-fastclone.rb +++ b/lib/git-fastclone.rb @@ -225,9 +225,9 @@ def clone(url, rev, src_dir, config) # Only checkout if we're changing branches to a non-default branch if rev - fail_on_error('git', 'checkout', '--quiet', rev.to_s, quiet: !verbose, - print_on_failure: print_git_errors, - :chdir => File.join(abs_clone_path, src_dir)) + fail_on_error('git', 'checkout', '--quiet', rev.to_s, quiet: !verbose, + print_on_failure: print_git_errors, + chdir: File.join(abs_clone_path, src_dir)) end update_submodules(src_dir, url) @@ -251,8 +251,8 @@ def update_submodules(pwd, url) submodule_url_list = [] output = '' output = fail_on_error('git', 'submodule', 'init', quiet: !verbose, - print_on_failure: print_git_errors, - :chdir => File.join(abs_clone_path, pwd)) + print_on_failure: print_git_errors, + chdir: File.join(abs_clone_path, pwd)) output.split("\n").each do |line| submodule_path, submodule_url = parse_update_info(line) @@ -270,7 +270,8 @@ def thread_update_submodule(submodule_url, submodule_path, threads, pwd) with_git_mirror(submodule_url) do |mirror, _| cmd = ['git', 'submodule', verbose ? nil : '--quiet', 'update', '--reference', mirror.to_s, submodule_path.to_s].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors, :chdir => File.join(abs_clone_path, pwd)) + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors, + chdir: File.join(abs_clone_path, pwd)) end update_submodules(File.join(pwd, submodule_path), submodule_url) @@ -347,7 +348,7 @@ def store_updated_repo(url, mirror, repo_name, fail_hard) end cmd = ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact - fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors, :chdir => mirror) + fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors, chdir: mirror) reference_updated[repo_name] = true rescue RunnerExecutionRuntimeError => e diff --git a/spec/git_fastclone_runner_spec.rb b/spec/git_fastclone_runner_spec.rb index ba583be..b90d4fd 100644 --- a/spec/git_fastclone_runner_spec.rb +++ b/spec/git_fastclone_runner_spec.rb @@ -91,7 +91,6 @@ def create_lockfile_double before(:each) do allow(runner_execution_double).to receive(:fail_on_error) {} allow(Dir).to receive(:pwd) { '/pwd' } - allow(Dir).to receive(:chdir).and_yield allow(subject).to receive(:with_git_mirror).and_yield('/cache', 0) expect(subject).to receive(:clear_clone_dest_if_needed).once {} end @@ -99,7 +98,7 @@ def create_lockfile_double it 'should clone correctly' do expect(subject).to receive(:fail_on_error).with( 'git', 'checkout', '--quiet', 'PH', - { quiet: true, print_on_failure: false } + { chdir: '/pwd/.', print_on_failure: false, quiet: true } ) { runner_execution_double } expect(subject).to receive(:fail_on_error).with( 'git', 'clone', '--quiet', '--reference', '/cache', 'PH', '/pwd/.', @@ -113,7 +112,7 @@ def create_lockfile_double subject.verbose = true expect(subject).to receive(:fail_on_error).with( 'git', 'checkout', '--quiet', 'PH', - { quiet: false, print_on_failure: false } + { chdir: '/pwd/.', print_on_failure: false, quiet: false } ) { runner_execution_double } expect(subject).to receive(:fail_on_error).with( 'git', 'clone', '--verbose', '--reference', '/cache', 'PH', '/pwd/.', @@ -338,7 +337,6 @@ def create_lockfile_double it 'should correctly update the hash' do allow(subject).to receive(:fail_on_error) - allow(Dir).to receive(:chdir) {} subject.reference_updated = placeholder_hash subject.store_updated_repo(placeholder_arg, placeholder_arg, placeholder_arg, false) @@ -389,7 +387,6 @@ def try_with_git_mirror(responses, results) expected_command = expected_commands.shift expect(command).to eq(expected_command) } - allow(Dir).to receive(:chdir).and_yield allow(subject).to receive(:print_formatted_error) {} allow(subject).to receive(:reference_repo_dir).and_return(test_reference_repo_dir) @@ -404,7 +401,7 @@ def clone_cmds(verbose: false) [ ['git', 'clone', verbose ? '--verbose' : '--quiet', '--mirror', test_url_valid, test_reference_repo_dir], - ['cd', test_reference_repo_dir, '&&', 'git', 'remote', verbose ? '--verbose' : nil, 'update', + ['git', 'remote', verbose ? '--verbose' : nil, 'update', '--prune'].compact ] end From 1f57f591522f8077eda428f59a511def039c5d10 Mon Sep 17 00:00:00 2001 From: Elton Gao Date: Fri, 1 Sep 2023 09:41:52 -0400 Subject: [PATCH 8/8] fixup --- lib/git-fastclone.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb index c8ccd02..56f8c51 100644 --- a/lib/git-fastclone.rb +++ b/lib/git-fastclone.rb @@ -249,7 +249,6 @@ def update_submodules(pwd, url) threads = [] submodule_url_list = [] - output = '' output = fail_on_error('git', 'submodule', 'init', quiet: !verbose, print_on_failure: print_git_errors, chdir: File.join(abs_clone_path, pwd))