Skip to content

Commit

Permalink
Fix projects in a subdir of HOME erroneously load ~/.rubocop.yml
Browse files Browse the repository at this point in the history
It is common for projects to be in a subdirectory of $HOME, causing
a problem in the ConfigLoader where add_excludes_from_files searches
for the highest file matching .rubocop.yml and finds ~/.rubocop.yml.

This violates the documentation stating that ~/.rubocop.yml will not be
loaded when a project specific file is found first.

Adds tests for config loading behavior and extends behavior for finding
ancestor configs to make it possible to grab the highest config before
the home dir.
  • Loading branch information
martinemde committed Aug 24, 2023
1 parent 4c83f86 commit 7498a15
Show file tree
Hide file tree
Showing 11 changed files with 237 additions and 49 deletions.
1 change: 1 addition & 0 deletions changelog/fix_specifically_exclude_rubocop_yml_from.md
@@ -0,0 +1 @@
* [#12147](https://github.com/rubocop/rubocop/pull/12147): Specifically exclude ~/.rubocop.yml when loading parent excludes when project is a subdir of HOME. ([@martinemde][])
15 changes: 11 additions & 4 deletions lib/rubocop/config_finder.rb
Expand Up @@ -21,6 +21,17 @@ def find_config_path(target_dir)
DEFAULT_FILE
end

def find_project_dotfile(target_dir)
if project_root
find_last_file_upwards(DOTFILE, target_dir, project_root)
else
files = files_upwards(DOTFILE, target_dir, Dir.home).to_a
# skip configs found in the user's home directory
files.pop if files.last && File.dirname(files.last) == Dir.home
files.last
end
end

# Returns the path RuboCop inferred as the root of the project. No file
# searches will go past this directory.
def project_root
Expand All @@ -37,10 +48,6 @@ def find_project_root
File.dirname(gems_file)
end

def find_project_dotfile(target_dir)
find_file_upwards(DOTFILE, target_dir, project_root)
end

def find_user_dotfile
return unless ENV.key?('HOME')

Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/config_loader.rb
Expand Up @@ -136,7 +136,7 @@ def possible_new_cops?(config)
end

def add_excludes_from_files(config, config_file)
exclusion_file = find_last_file_upwards(DOTFILE, config_file, ConfigFinder.project_root)
exclusion_file = ConfigFinder.find_project_dotfile(config_file)

return unless exclusion_file
return if PathUtil.relative_path(exclusion_file) == PathUtil.relative_path(config_file)
Expand Down
32 changes: 16 additions & 16 deletions lib/rubocop/file_finder.rb
Expand Up @@ -6,35 +6,35 @@ module RuboCop
# Common methods for finding files.
# @api private
module FileFinder
def self.root_level=(level)
@root_level = level
end

def self.root_level?(path, stop_dir)
(@root_level || stop_dir) == path.to_s
class << self
# Root level defines the absolute path of the top level directory
# that can be searched for files.
#
# If the start_dir is not a subdirectory of root_level, root_level is ignored.
#
# root_level is only used in tests to avoid finding files in directories
# outside of the isolated test directory, especially on Windows where
# the temporary directory is under the user's home directory.
attr_accessor :root_level
end

def find_file_upwards(filename, start_dir, stop_dir = nil)
traverse_files_upwards(filename, start_dir, stop_dir) do |file|
# minimize iteration for performance
return file if file
end
files_upwards(filename, start_dir, stop_dir).first
end

def find_last_file_upwards(filename, start_dir, stop_dir = nil)
last_file = nil
traverse_files_upwards(filename, start_dir, stop_dir) { |file| last_file = file }
last_file
files_upwards(filename, start_dir, stop_dir).to_a.last
end

private
def files_upwards(filename, start_dir, stop_dir = nil)
return enum_for(:files_upwards, filename, start_dir, stop_dir) unless block_given?

def traverse_files_upwards(filename, start_dir, stop_dir)
stop_dirs = [stop_dir, FileFinder.root_level].compact.map { |dir| File.expand_path(dir) }
Pathname.new(start_dir).expand_path.ascend do |dir|
file = dir + filename
yield(file.to_s) if file.exist?

break if FileFinder.root_level?(dir, stop_dir)
break if stop_dirs.include?(dir.to_s)
end
end
end
Expand Down
12 changes: 6 additions & 6 deletions lib/rubocop/rspec/shared_contexts.rb
Expand Up @@ -11,18 +11,18 @@
# Make sure to expand all symlinks in the path first. Otherwise we may
# get mismatched pathnames when loading config files later on.
tmpdir = File.realpath(tmpdir)
root = File.join(tmpdir, 'root')
Dir.mkdir(root)
RuboCop::FileFinder.root_level = root

virtual_home = File.expand_path(File.join(tmpdir, 'home'))
virtual_home = File.expand_path('home', root)
Dir.mkdir(virtual_home)
ENV['HOME'] = virtual_home
ENV.delete('XDG_CONFIG_HOME')

base_dir = example.metadata[:project_inside_home] ? virtual_home : tmpdir
root = example.metadata[:root]
working_dir = root ? File.join(base_dir, 'work', root) : File.join(base_dir, 'work')

# Make upwards search for .rubocop.yml files stop at this directory.
RuboCop::FileFinder.root_level = working_dir
base_dir = example.metadata[:project_inside_home] ? virtual_home : root
working_dir = File.join(base_dir, 'work')

begin
FileUtils.mkdir_p(working_dir)
Expand Down
4 changes: 3 additions & 1 deletion spec/isolated_environment_spec.rb
Expand Up @@ -10,8 +10,10 @@
# directory is under the user's home directory. On any platform we don't want
# a .rubocop.yml file in the temporary directory to affect the outcome of
# rspec.
# This is the only test that should place a file above the working directory.
it 'is not affected by a config file above the work directory' do
create_file('../.rubocop.yml', ['inherit_from: missing_file.yml'])
erroneous_file = File.expand_path('../.rubocop.yml', RuboCop::FileFinder.root_level)
File.write(erroneous_file, 'inherit_from: missing_file.yml')
create_file('ex.rb', ['# frozen_string_literal: true'])
# A return value of 0 means that the erroneous config file was not read.
expect(cli.run([])).to eq(0)
Expand Down
61 changes: 57 additions & 4 deletions spec/rubocop/config_loader_spec.rb
Expand Up @@ -9,12 +9,14 @@
described_class.debug = true
# Force reload of default configuration
described_class.default_configuration = nil
RuboCop::ConfigFinder.project_root = nil
end

after do
described_class.debug = false
# Remove custom configuration
described_class.default_configuration = nil
RuboCop::ConfigFinder.project_root = nil
end

let(:default_config) { described_class.default_configuration }
Expand Down Expand Up @@ -80,14 +82,14 @@
end
end

context 'when there is a spurious rubocop config outside of the project', root: 'dir' do
context 'when there is a spurious rubocop config outside of the project' do
let(:dir_path) { 'dir' }

before do
# Force reload of project root
RuboCop::ConfigFinder.project_root = nil
create_empty_file('Gemfile')
create_empty_file('../.rubocop.yml')
create_empty_file('dir/Gemfile')
create_empty_file('.rubocop.yml')
end

after do
Expand All @@ -96,7 +98,9 @@
end

it 'ignores the spurious config and falls back to the provided default file if run from the project' do
expect(configuration_file_for).to end_with('config/default.yml')
Dir.chdir('dir') do
expect(configuration_file_for).to end_with('config/default.yml')
end
end
end

Expand Down Expand Up @@ -149,6 +153,55 @@
end
end

context 'when the project is in a subdirectory of $HOME', :project_inside_home do
let(:file_path) { File.expand_path('~/work/vendor/.rubocop.yml') }

before do
create_file(file_path, <<~YAML)
AllCops:
Exclude:
- ./**
YAML
end

it 'gets AllCops/Exclude from the highest directory level' do
excludes = configuration_from_file['AllCops']['Exclude']
expect(excludes).to eq([File.expand_path('~/work/vendor/**')])
end

context 'and there is a personal config file in the home folder' do
before do
create_file('~/.rubocop.yml', <<~YAML)
AllCops:
Exclude:
- tmp/**
YAML
end

it 'ignores personal AllCops/Exclude' do
excludes = configuration_from_file['AllCops']['Exclude']
expect(excludes).to eq([File.expand_path('~/work/vendor/**')])
end

context 'and there is a project config higher up but before the home dir' do
before do
create_file(File.expand_path('~/work/.rubocop.yml'), <<~YAML)
AllCops:
Exclude:
- docs/**
YAML
end

it 'gets AllCops/Exclude from the highest directory level before $HOME' do
excludes = configuration_from_file['AllCops']['Exclude']
expect(excludes).to eq(
[File.expand_path('~/work/vendor/**'), File.expand_path('~/work/docs/**')]
)
end
end
end
end

context 'when multiple config files exist in ancestor directories' do
let(:file_path) { 'dir/.rubocop.yml' }

Expand Down
8 changes: 4 additions & 4 deletions spec/rubocop/cop/team_spec.rb
Expand Up @@ -88,7 +88,7 @@ def a
describe '#inspect_file', :isolated_environment do
include FileHelper

let(:file_path) { '/tmp/example.rb' }
let(:file_path) { 'example.rb' }
let(:source) do
source = RuboCop::ProcessedSource.from_file(file_path, ruby_version)
source.config = config
Expand Down Expand Up @@ -175,7 +175,7 @@ def a

let(:error_message) do
'An error occurred while Style/NumericLiterals cop was inspecting ' \
'/tmp/example.rb:1:0.'
'example.rb:1:0.'
end

it 'records Team#errors' do
Expand All @@ -198,7 +198,7 @@ def a
RUBY
end

let(:file_path) { '/tmp/Gemfile' }
let(:file_path) { 'Gemfile' }

let(:buggy_correction) { ->(_corrector) do raise cause end }
let(:options) { { autocorrect: true } }
Expand All @@ -207,7 +207,7 @@ def a

let(:error_message) do
'An error occurred while Bundler/OrderedGems cop was inspecting ' \
'/tmp/Gemfile.'
'Gemfile.'
end

it 'records Team#errors' do
Expand Down

0 comments on commit 7498a15

Please sign in to comment.