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.

Sign up for GitHub

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

Jump to bottom

Fixes for repository detection on Windows #136

Merged
merged 16 commits into from
Nov 7, 2018
Merged

Fixes for repository detection on Windows #136

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
552add8
Respect executable file extensions on windows
GICodeWarrior Oct 21, 2018
72b977a
Fix git execution on Windows
GICodeWarrior Oct 21, 2018
d30a531
Drop support for 2.2
GICodeWarrior Oct 21, 2018
e744b52
Include blank extension for non-Windows systems
GICodeWarrior Oct 21, 2018
5310d03
Fixup style
GICodeWarrior Oct 21, 2018
1dcd78e
Merge branch 'master' into master
ashmaroli Oct 23, 2018
9ceab82
Switch to Jekyll::Utils::Exec
GICodeWarrior Oct 27, 2018
1f99e1d
Remove reference to removed method
GICodeWarrior Oct 27, 2018
147d9fc
Clear PATH instead of deleting
GICodeWarrior Oct 27, 2018
5341d34
Remove unused `require`
GICodeWarrior Oct 27, 2018
598bc39
Update lib/jekyll-github-metadata/repository_finder.rb
ashmaroli Oct 27, 2018
f369e51
Update lib/jekyll-github-metadata/repository_finder.rb
ashmaroli Oct 27, 2018
27cc2d5
Drop support for versions < 3.4
GICodeWarrior Oct 27, 2018
939614d
Rescue method instead of block
GICodeWarrior Nov 7, 2018
66e5be6
Merge branch 'master' into master
GICodeWarrior Nov 7, 2018
3f3379b
Return empty array, not string
GICodeWarrior Nov 7, 2018
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
16 changes: 7 additions & 9 deletions lib/jekyll-github-metadata/repository_finder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,14 @@ def nwo_from_config
repo if repo && repo.is_a?(String) && repo.include?("/")
end

def git_exe_path
ENV["PATH"].to_s
.split(File::PATH_SEPARATOR)
.map { |path| File.join(path, "git") }
.find { |path| File.exist?(path) }
end

def git_remotes
return [] if git_exe_path.nil?
`#{git_exe_path} remote --verbose`.to_s.strip.split("\n")
output = ""
begin
_p, output = Jekyll::Utils::Exec.run("git", "remote", "--verbose")
GICodeWarrior marked this conversation as resolved.
Show resolved Hide resolved
rescue Errno::ENOENT => e
Jekyll.logger.warn "Git is not installed: ", e.message
GICodeWarrior marked this conversation as resolved.
Show resolved Hide resolved
end
output.to_s.strip.split("\n")
DirtyF marked this conversation as resolved.
Show resolved Hide resolved
end

def git_remote_url
Expand Down
8 changes: 5 additions & 3 deletions spec/repository_finder_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,14 @@
end

context "when git doesn't exist" do
before(:each) { @old_path = ENV.delete("PATH").to_s.split(File::PATH_SEPARATOR) }
after(:each) { ENV["PATH"] = @old_path.join(File::PATH_SEPARATOR) }
before(:each) do
@old_path = ENV["PATH"]
ENV["PATH"] = ""
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member

Choose a reason for hiding this comment

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

The PATH is "reset" to an empty string to simulate the scenario where git is not installed.. (especially in CI environments)

end
after(:each) { ENV["PATH"] = @old_path }

it "fails with a nice error message" do
allow(subject).to receive(:git_remote_url).and_call_original
expect(subject.send(:git_exe_path)).to eql(nil)
Copy link
Member

Choose a reason for hiding this comment

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

I think we still need to be able to run git? If we're relying on Jekyll::Utils::Exec, then we'll definitely need to make sure that the exe can be found, right?

Copy link
Member

Choose a reason for hiding this comment

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

Relying on Jekyll::Utils::Exec is like running system "git" in a begin..rescue block..
If the utility method doesn't find the executable, an Errno::ENOENT is raised which is rescued to output a warning to the terminal..

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so we can remove git_exe_path?

Copy link
Member

Choose a reason for hiding this comment

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

Yep. In fact, this PR has already removed it..

expect(subject.send(:git_remote_url)).to be_empty
end
end
Expand Down