Skip to content

Commit

Permalink
[Fix #595] Exclude files ignored by git
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexWayfer authored and bbatsov committed Dec 23, 2018
1 parent bf2bc4c commit 56c4cd0
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 8 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -20,6 +20,10 @@
* [#6570](https://github.com/rubocop-hq/rubocop/pull/6570): Fix a false positive for `Style/MethodCallWithArgsParentheses` `omit_parentheses` enforced style while splatting the result of a method invocation. ([@gsamokovarov][])
* [#6594](https://github.com/rubocop-hq/rubocop/pull/6594): Fix a false positive for `Rails/OutputSafety` when the receiver is a non-interpolated string literal. ([@amatsuda][])

### Changes

* [#595](https://github.com/rubocop-hq/rubocop/issues/595): Exclude files ignored by `git`. ([@AlexWayfer][])

## 0.61.1 (2018-12-06)

### Bug fixes
Expand Down
38 changes: 30 additions & 8 deletions lib/rubocop/target_finder.rb
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require 'set'
require 'open3'

module RuboCop
# This class finds target files to inspect by scanning the directory tree
Expand Down Expand Up @@ -59,10 +60,13 @@ def target_files_in_dir(base_dir = Dir.pwd)
end
all_files = find_files(base_dir, File::FNM_DOTMATCH)
hidden_files = Set.new(all_files - find_files(base_dir, 0))

git_files = ls_git_files(base_dir)

base_dir_config = @config_store.for(base_dir)

target_files = all_files.select do |file|
to_inspect?(file, hidden_files, base_dir_config)
to_inspect?(file, git_files, hidden_files, base_dir_config)
end

# Most recently modified file first.
Expand All @@ -71,13 +75,6 @@ def target_files_in_dir(base_dir = Dir.pwd)
target_files
end

def to_inspect?(file, hidden_files, base_dir_config)
return false if base_dir_config.file_to_exclude?(file)
return true if !hidden_files.include?(file) && ruby_file?(file)

base_dir_config.file_to_include?(file)
end

# Search for files recursively starting at the given base directory using
# the given flags that determine how the match is made. Excluded files will
# be removed later by the caller, but as an optimization find_files removes
Expand All @@ -100,6 +97,31 @@ def find_files(base_dir, flags)
Dir.glob(pattern, flags).select { |path| FileTest.file?(path) }
end

private

def ls_git_files(base_dir)
return if `sh -c 'command -v git'`.empty?

This comment has been minimized.

Copy link
@thomthom

thomthom Jan 9, 2019

Contributor

How is this working under Windows? Testing on my machine such a line would yield an error. Is this code not reached on Windows systems?

This comment has been minimized.

Copy link
@agross

agross Jan 9, 2019

It is reached, and it expects sh to be in your $PATH. If it isn't because you didn't install git it will raise an exception.

This comment has been minimized.

Copy link
@thomthom

thomthom Jan 9, 2019

Contributor

I have git installed and in my PATH, but I've always excluded the additional (and optional) bash tools. I've had issues with having the bash tools in PATH before. Conflicting with other tools.

From the looks of the rest of the code in this PR it's using only call to git itself.

I'm not that familiar with bash commands, what is this line doing? check for the existence of git?
If so, can this be tweaked to not require bash tools on Windows? Would simplify dependency requirements for rubocop.

This comment has been minimized.

Copy link
@agross

agross Jan 9, 2019

I've had issues with having the bash tools

Classic find and sort differences between Windows and GNU versions.

what is this line doing

It runs a shell script (command -v git) which returns the path to the git executable (if found, otherwise an empty string). If you're familiar with Windows' cmd: where git has a similar effect.

Would simplify dependency requirements for rubocop.

Since 0.62 rubocop depends on sh. I guess the developers were willing to take that dependency and did not think deeply about systems where sh is normally not installed.

I've raised other issues with the new git integration, you may track it here: #6646

This comment has been minimized.

Copy link
@thomthom

thomthom Jan 10, 2019

Contributor

Thanks for the insight. I'll follow up on #6646. The new bash dependencies is a troublesome barrier for me and the rubocop-sketchup extension I maintain - where the majority is using Windows.

This comment has been minimized.

Copy link
@abrthel

abrthel Jan 13, 2019

Just wanted to add that this caused problems for myself as well as I do the same as thomthom, I don't add the bash tools to my PATH

This comment has been minimized.

Copy link
@bbatsov

bbatsov via email Jan 13, 2019

Collaborator

This comment has been minimized.

Copy link
@abrthel

abrthel Jan 13, 2019

Oh? Has it released yet? On rubocop v0.62.0 I get error No such file or directory - sh -c 'command -v git' when trying to run rubocop. I assumed it was because of this feature.

This comment has been minimized.

Copy link
@bbatsov

bbatsov via email Jan 13, 2019

Collaborator

output, _error, status = Open3.capture3(
'git', 'ls-files', '-z', base_dir,
'--exclude-standard', '--others', '--cached', '--modified'
)

return unless status.success?

output.split("\0").map { |git_file| "#{base_dir}/#{git_file}" }
end

def to_inspect?(file, git_files, hidden_files, base_dir_config)
return false if base_dir_config.file_to_exclude?(file)

if !hidden_files.include?(file) && ruby_file?(file)
return git_files.nil? || git_files.include?(file)
end

base_dir_config.file_to_include?(file)
end

def toplevel_dirs(base_dir, flags)
Dir.glob(File.join(base_dir, '*'), flags).select do |dir|
File.directory?(dir) && !dir.end_with?('/.', '/..')
Expand Down
1 change: 1 addition & 0 deletions manual/configuration.md
Expand Up @@ -229,6 +229,7 @@ ruby interpreters listed under `AllCops`/`RubyInterpreters` are
inspected, unless the file also matches a pattern in
`AllCops`/`Exclude`. Hidden directories (i.e., directories whose names
start with a dot) are not searched by default.
Files ignored by Git are ignored by RuboCop by default.

Here is an example that might be used for a Rails project:

Expand Down
59 changes: 59 additions & 0 deletions spec/rubocop/target_finder_spec.rb
Expand Up @@ -398,6 +398,65 @@
expect(found_basenames).not_to include('ruby2.rb')
end

context 'in git repo directory' do
before do
system 'git init'

create_empty_file('git_regular.rb')
create_empty_file('git_group.rb')
create_empty_file('git_excluded.rb')
create_empty_file('git_glob.rb')
create_empty_file('git with spaces.rb')
create_empty_file('git_dir/thing.rb')
create_file('.gitignore', <<-'CONTENT'.strip_indent)
git_regular.rb
git_gr[ao]up.rb
!git_excluded.rb
git_glob*
git_dir/
CONTENT
end

it 'does not pick files ignored by git' do
expect(found_basenames).not_to include(
'git_regular.rb',
'git_group.rb',
'git_glob.rb',
'git_dir/thing.rb'
)
expect(found_basenames).to include(
'git_excluded.rb',
'git with spaces.rb'
)
end

context 'without `git` executable' do
before do
allow_any_instance_of(Kernel).to receive(:`)
.with(/command -v git/)
.and_return('')

allow_any_instance_of(Kernel).to receive(:`)
.with(start_with('git ls-files'))
.and_raise(Errno::ENOENT, 'git')
end

it 'does not fail' do
expect { found_basenames }.not_to raise_error
end
end

context 'with `*` in argument' do
let(:base_dir) { './git*' }

it 'does not fail' do
expect(found_basenames).not_to include(
'thing.rb'
)
end
end
end

context 'when an exception is raised while reading file' do
around do |example|
original_stderr = $stderr
Expand Down

0 comments on commit 56c4cd0

Please sign in to comment.