Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pushing changes in progress for git-lfs support adding to entire ecosystem. #9766

Closed
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
56 changes: 38 additions & 18 deletions common/lib/dependabot/file_fetchers/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -441,12 +441,8 @@ def codecommit_client
# INTERNAL METHODS (not for use by sub-classes) #
#################################################

sig do
params(path: String, fetch_submodules: T::Boolean, raise_errors: T::Boolean)
.returns(T::Array[OpenStruct])
end
def _fetch_repo_contents(path, fetch_submodules: false,
raise_errors: true)
sig { params(path: String, fetch_submodules: T::Boolean, raise_errors: T::Boolean).returns(T::Array[OpenStruct]) }
def _fetch_repo_contents(path, fetch_submodules: false, raise_errors: true)
path = path.gsub(" ", "%20")
provider, repo, tmp_path, commit =
_full_specification_for(path, fetch_submodules: fetch_submodules)
Expand Down Expand Up @@ -476,10 +472,7 @@ def _fetch_repo_contents(path, fetch_submodules: false,
retry
end

sig do
params(provider: String, repo: String, path: String, commit: String)
.returns(T::Array[OpenStruct])
end
sig { params(provider: String, repo: String, path: String, commit: String).returns(T::Array[OpenStruct]) }
def _fetch_repo_contents_fully_specified(provider, repo, path, commit)
case provider
when "github"
Expand Down Expand Up @@ -831,8 +824,12 @@ def _clone_repo_contents(target_directory:)
" --recurse-submodules=on-demand"
end
# Need to fetch the commit due to the --depth 1 above.
SharedHelpers.run_shell_command("git fetch #{fetch_options.string} origin #{source.commit}")

if lfs_enabled?(path.to_s)
SharedHelpers.run_shell_command("git lfs install")
SharedHelpers.run_shell_command("git-lfs-fetch #{fetch_options.string} origin #{source.commit}")
else
SharedHelpers.run_shell_command("git fetch #{fetch_options.string} origin #{source.commit}")
end
reset_options = StringIO.new
reset_options << "--hard"
reset_options << if submodule_cloning_failed
Expand Down Expand Up @@ -861,18 +858,41 @@ def decode_binary_string(str)

sig { params(path: String).returns(T::Array[String]) }
def find_submodules(path)
SharedHelpers.run_shell_command(
<<~CMD
git -C #{path} ls-files --stage
CMD
).split("\n").filter_map do |line|
lfs_enabled = lfs_enabled?(path) if lfs_enabled.nil?
SharedHelpers.run_shell_command("git-lfs-checkout") if lfs_enabled
command_string = get_command_string(path, lfs_enabled)
# eep command_string
SharedHelpers.run_shell_command(command_string).split("\n").filter_map do |line|
info = line.split

type = info.first
path = T.must(info.last)

next path if type == DependencyFile::Mode::SUBMODULE
end
rescue SharedHelpers::HelperSubprocessFailed => e
Dependabot.logger.warn("LFS is enabled in this repo. Please use an LFS enabled client") if lfs_enabled
Dependabot.logger.error(e.message)
raise e.exception("Message: #{e.message}")
end

sig { params(path: String).returns(T.nilable(T::Boolean)) }
def lfs_enabled?(path)
filepath = File.join(path, ".gitattributes")
T.let(true, T::Boolean) if File.exist?(filepath) && File.readable?(filepath) &&
SharedHelpers.run_shell_command("cat #{filepath} | grep \"filter=lfs\"")
.include?("filter=lfs")
rescue StandardError => e
Dependabot.logger.warn("An error has occurred: #{e.message}")
# this should not be needed, but I don't trust 'should'
T.let(false, T::Boolean)
end

sig { params(path: String, lfs_enabled: T.nilable(T::Boolean)).returns(String) }
def get_command_string(path, lfs_enabled)
return "git -C #{path} ls-files --stage" unless lfs_enabled

Dependabot.logger.warn("LFS is enabled in this repo. Please use an LFS enabled client")
return "cd #{path};git-lfs ls-files --stage"
end
end
end
Expand Down
29 changes: 26 additions & 3 deletions common/lib/dependabot/shared_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -374,12 +374,35 @@ def self.configure_git_to_use_https(host)

sig { params(path: String).void }
def self.reset_git_repo(path)
Dir.chdir(path) do
run_shell_command("git reset HEAD --hard")
run_shell_command("git clean -fx")
if lfs_enabled?(path)
Dir.chdir(path) do
begin
run_shell_command("git-lfs-reset HEAD --hard")
rescue SharedHelpers::HelperSubprocessFailed
Dependabot.logger.warn("LFS is enabled in this repo. Please use an LFS enabled client")
end
run_shell_command("git clean -fx")
end
else
Dir.chdir(path) do
run_shell_command("git reset HEAD --hard")
run_shell_command("git clean -fx")
end
end
end

sig { params(path: String).returns(T.nilable(T::Boolean)) }
def self.lfs_enabled?(path)
filepath = File.join(path, ".gitattributes")
T.let(true, T::Boolean) if File.exist?(filepath) && File.readable?(filepath) &&
SharedHelpers.run_shell_command("cat #{filepath} | grep \"filter=lfs\"")
.include?("filter=lfs")
rescue StandardError => e
Copy link
Member

@bdragon bdragon May 30, 2024

Choose a reason for hiding this comment

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

Jurre's suggestion is more idiomatic and won't incur the cost of shelling out, so I think you should incorporate that.

Just to call attention to it, shelling out to grep like this will return a non-zero exit if there's no match. SharedHelpers.run_shell_command will raise a HelperSubprocessFailed exception on a non-zero exit, but here we're swallowing virtually any error that could occur and carrying on. The problem with this is it assumes the only thing that could go wrong is grep not finding a match, when there are other legitimate exceptions that could occur that we would want to propagate.

If you find yourself writing rescue StandardError, it might be a good idea to ask if it's really necessary for what you're trying to do. Doing so will match any subclass of StandardError, which is the vast majority of errors you'll encounter (the Ruby docs on Exception are good resource). If you truly want to check for the HelperSubprocessFailed exception, rescue that class specifically so that other, unexpected exceptions are propagated correctly.

Dependabot.logger.warn("An error occurred: #{e.message}")
# this should not be needed, but I don't trust 'should'
T.let(false, T::Boolean)
end

sig { returns(T::Array[String]) }
def self.find_safe_directories
# to preserve safe directories from global .gitconfig
Expand Down
13 changes: 13 additions & 0 deletions common/lib/dependabot/workspace/git.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ def initialize(path)
super(path)
@initial_head_sha = T.let(head_sha, String)
configure_git
run_shell_command("git lfs install") if lfs_enabled?(path.to_s)
end

sig { returns(T::Boolean) }
Expand Down Expand Up @@ -168,6 +169,18 @@ def run_shell_command(*args, **kwargs)
def debug(message)
Dependabot.logger.debug("[workspace] #{message}")
end

sig { params(path: String).returns(T.nilable(T::Boolean)) }
def lfs_enabled?(path)
filepath = File.join(path, ".gitattributes")
T.let(true, T::Boolean) if File.exist?(filepath) && File.readable?(filepath) &&
SharedHelpers.run_shell_command("cat #{filepath} | grep \"filter=lfs\"")
.include?("filter=lfs")
rescue StandardError => e
Dependabot.logger.warn("An error has occurred: #{e.message}")
# this should not be needed, but I don't trust 'should'
T.let(false, T::Boolean)
end
end
end
end
Loading