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

Changes from 6 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
12 changes: 9 additions & 3 deletions lib/jekyll-github-metadata/repository_finder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# frozen_string_literal: true

require "open3"

module Jekyll
module GitHubMetadata
class RepositoryFinder
Expand Down Expand Up @@ -46,15 +48,19 @@ def nwo_from_config
end

def git_exe_path
exts = (ENV["PATHEXT"] || "").split(File::PATH_SEPARATOR).push("")
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PATHEXT on Windows is a list of file extensions respected by cmd.exe.
http://environmentvariables.org/PathExt

This code splits PATHEXT into an array of extensions and adds a blank extension to the end (for odd Windows setups and normal non-Windows setups).

irb(main):001:0> (ENV["PATHEXT"] || "").split(File::PATH_SEPARATOR).push("")
=> [".COM", ".EXE", ".BAT", ".CMD", ".VBS", ".VBE", ".JS", ".JSE", ".WSF", ".WSH", ".MSC", ".RB", ".RBW", ""]

On non-Windows platforms exts is only [""].

cmds = exts.map { |ext| "git#{ext}" }
Copy link
Member

Choose a reason for hiding this comment

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

Is there any known scenario where git.exe is not the Git Executable on Windows? If not, lets not waste time searching for those paths and zero in on ~/git.exe straight away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really have visibility into how people have their machines setup, but having a git.bat file to wrap Git is one edge case I can think of.

Random example: https://gist.github.com/duebbert/c4f26c9d0691b7d4a0e932e7d9f36271

Technically all of the extensions in PATH_EXT can be used plainly as git from the command line on Windows if someone added a wrapper or alternate implementation to their PATH.

If you want to simplify it, I recommend skipping the PATH search altogether and allowing the Open3/Exec.run call to outsource that to the OS.

Do you know why this PATH code exists in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

Delegating to Windows can be bothersome IMO.
Some machines respond to where while some responds to where.exe while the others do not have either of the two programs in their PATH..

ENV["PATH"].to_s
.split(File::PATH_SEPARATOR)
.map { |path| File.join(path, "git") }
.find { |path| File.exist?(path) }
.map { |path| cmds.map { |cmd| File.join(path, cmd) } }
.flatten
.find { |path| File.executable?(path) && !File.directory?(path) }
Copy link
Member

Choose a reason for hiding this comment

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

Is this branching really necessary?
When is an executable ever a directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directories are always executable. So if there is a git directory in PATH, we don't want to select it.

Windows:

irb(main):006:0> File.directory?('/Users')
=> true
irb(main):007:0> File.executable?('/Users')
=> true

Linux:

irb(main):002:0> File.directory?('/home')
=> true
irb(main):003:0> File.executable?('/home')
=> true

Copy link
Member

Choose a reason for hiding this comment

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

Today I Learned TM 👍

end

def git_remotes
return [] if git_exe_path.nil?
`#{git_exe_path} remote --verbose`.to_s.strip.split("\n")
output, _status = Open3.capture2(git_exe_path, "remote", "--verbose")
Copy link
Member

Choose a reason for hiding this comment

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

@parkr @benbalter Can we drop support for Jekyll versions < 3.4.0 and use Jekyll::Utils::Exec.run instead of using Open3 directly..?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, as GitHub pages uses currently 3.7.4 it shouldn't be a problem.

output.to_s.strip.split("\n")
DirtyF marked this conversation as resolved.
Show resolved Hide resolved
end

def git_remote_url
Expand Down