Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
*:lock
*.gem
Gemfile.lock
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated but we no longer need to track changes to this file since we removed it from Git

4 changes: 3 additions & 1 deletion lib/git-fastclone.rb
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,11 @@ def clone(url, rev, src_dir, config)

clone_commands = ['git', 'clone', verbose ? '--verbose' : '--quiet']
# For sparse checkouts, clone directly from the local mirror and skip the actual checkout process
# --shared is included so that the checkout remains fast even if the reference and destination directories
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc says this is a dangerous option which totally makes sense: https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---shared

just to be suare that we have tested cases like deleting the checkout in the machine? yeah this is def only for CI usage only since we have clear idea on when and how to use these clones

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. That's why initially I removed it in between the 1.6.0.pre1 -> 1.6.0 release because I wasn't going to keep it if it wasn't improving the checkout speed. But yeah, that's where when I tested locally on my Mac it wasn't the same situation as the disk/volume setup on the CI workers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's potentially room to support more flags to augment or change the behavior of the sparse checkout, but I figure we should only add those if we've got a good reason

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100%, we should not allow caller to pass in any flags

# live on different filesystem volumes.
# For normal clones, use --reference and clone from the remote URL
if sparse_paths
clone_commands.push('--no-checkout')
clone_commands.push('--no-checkout', '--shared')
clone_commands << mirror.to_s << clone_dest
else
clone_commands << '--reference' << mirror.to_s << url.to_s << clone_dest
Expand Down
2 changes: 1 addition & 1 deletion lib/git-fastclone/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@

# Version string for git-fastclone
module GitFastCloneVersion
VERSION = '1.6.0'
VERSION = '1.6.1'
end
41 changes: 41 additions & 0 deletions spec/git_fastclone_runner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,47 @@ def create_lockfile_double
end
end

context 'with sparse checkout' do
before(:each) do
subject.sparse_paths = %w[path1 path2]
end

it 'should clone with --no-checkout and --shared flags' do
expect(subject).to receive(:fail_on_error).with(
'git', 'clone', '--quiet', '--no-checkout', '--shared', '/cache', '/pwd/.',
{ quiet: true, print_on_failure: false }
) { runner_execution_double }
expect(subject).to receive(:perform_sparse_checkout).with('/pwd/.', 'PH')

subject.clone(placeholder_arg, 'PH', '.', nil)
end

it 'should clone with verbose mode and --shared flag' do
subject.verbose = true
expect(subject).to receive(:fail_on_error).with(
'git', 'clone', '--verbose', '--no-checkout', '--shared', '/cache', '/pwd/.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder with these new flags if the specs should include inspecting the cloned repo... for example we expect .git/objects/info/alternates to exists with --shared and not the case without --shared?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah – this work made me think that this repo could benefit from some simple integration tests which actually clones some simple repos and asserts for the expected state of the cloned repos.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be easy to write with AI

{ quiet: false, print_on_failure: false }
) { runner_execution_double }
expect(subject).to receive(:perform_sparse_checkout).with('/pwd/.', 'PH')

subject.clone(placeholder_arg, 'PH', '.', nil)
end

it 'should not perform regular checkout when sparse checkout is enabled' do
expect(subject).to receive(:fail_on_error).with(
'git', 'clone', '--quiet', '--no-checkout', '--shared', '/cache', '/pwd/.',
{ quiet: true, print_on_failure: false }
) { runner_execution_double }
expect(subject).to receive(:perform_sparse_checkout).with('/pwd/.', 'PH')
expect(subject).not_to receive(:fail_on_error).with(
'git', 'checkout', '--quiet', 'PH',
anything
)

subject.clone(placeholder_arg, 'PH', '.', nil)
end
end

context 'with pre-clone-hook' do
let(:pre_clone_hook) { '/some/command' }
before(:each) do
Expand Down