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

(CODEMGMT-1299) Do not recurse into recursive globbed exclusions when purging #1178

Merged
merged 1 commit into from
Jul 7, 2021
Merged

(CODEMGMT-1299) Do not recurse into recursive globbed exclusions when purging #1178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 70 additions & 8 deletions lib/r10k/util/purgeable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ module Util
# {#desired_contents}
module Purgeable

HIDDEN_FILE = /\.[^.]+/

FN_MATCH_OPTS = File::FNM_PATHNAME | File::FNM_DOTMATCH

# @deprecated
#
# @!method logger
# @abstract Including classes must provide a logger method
# @return [Log4r::Logger]
Expand Down Expand Up @@ -38,23 +44,79 @@ def current_contents(recurse)
end
end

# @deprecated Unused helper function
#
# @return [Array<String>] Directory contents that are expected but not present
def pending_contents(recurse)
desired_contents - current_contents(recurse)
end

def matches?(test, path)
if test == path
true
elsif File.fnmatch?(test, path, FN_MATCH_OPTS)
true
else
false
end
end

# A method to collect potentially purgeable content without searching into
# ignored directories when recursively searching.
#
# @param dir [String, Pathname] The directory to search for purgeable content
# @param exclusion_gobs [Array<String>] A list of file paths or File globs
# to exclude from recursion (These are generated by the classes that
# mix this module into them and are typically programatically generated)
# @param allowed_gobs [Array<String>] A list of file paths or File globs to exclude
# from recursion (These are passed in by the caller of purge! and typically
# are user supplied configuration values)
# @param desireds_not_to_recurse_into [Array<String>] A list of file paths not to
# recurse into. These are programatically generated, these exist to maintain
# backwards compatibility with previous implementations that used File.globs
# for "recursion", ie "**/{*,.[^.]*}" which would not recurse into dot directories.
# @param recurse [Boolean] Whether or not to recurse into child directories that do
# not match other filters.
#
# @return [Array<String>] Contents which may be purged.
def potentially_purgeable(dir, exclusion_globs, allowed_globs, desireds_not_to_recurse_into, recurse)
children = Pathname.new(dir).children.reject do |path|
path = path.to_s

if exclusion_match = exclusion_globs.find { |exclusion| matches?(exclusion, path) }
logger.debug2 _("Not purging %{path} due to internal exclusion match: %{exclusion_match}") % {path: path, exclusion_match: exclusion_match}
elsif allowlist_match = allowed_globs.find { |allowed| matches?(allowed, path) }
logger.debug _("Not purging %{path} due to whitelist match: %{allowlist_match}") % {path: path, allowlist_match: allowlist_match}
else
desired_match = desireds_not_to_recurse_into.grep(path).first
end

!!exclusion_match || !!allowlist_match || !!desired_match
end

children.flat_map do |child|
if File.directory?(child) && recurse
potentially_purgeable(child, exclusion_globs, allowed_globs, desireds_not_to_recurse_into, recurse)
else
child.to_s
end
end
end

# @return [Array<String>] Directory contents that are present but not expected
def stale_contents(recurse, exclusions, whitelist)
fn_match_opts = File::FNM_PATHNAME | File::FNM_DOTMATCH
dirs = self.managed_directories
desireds = self.desired_contents
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't love "desireds" 😛 But I don't have an alternative that isn't really wordy.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, it should be "desires" but I didn't want to write a bunch of code about my hidden_desires.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds weird to me too, anyway. Oh well, adjectives can be nouns whenever we want, it's fine :P

hidden_desireds, regular_desireds = desireds.partition do |desired|
HIDDEN_FILE.match(File.basename(desired))
end

(current_contents(recurse) - desired_contents).reject do |item|
if exclusion_match = exclusions.find { |ex_item| (ex_item == item) || File.fnmatch?(ex_item, item, fn_match_opts) }
logger.debug2 _("Not purging %{item} due to internal exclusion match: %{exclusion_match}") % {item: item, exclusion_match: exclusion_match}
elsif whitelist_match = whitelist.find { |wl_item| (wl_item == item) || File.fnmatch?(wl_item, item, fn_match_opts) }
logger.debug _("Not purging %{item} due to whitelist match: %{whitelist_match}") % {item: item, whitelist_match: whitelist_match}
end
initial_purgelist = dirs.flat_map do |dir|
potentially_purgeable(dir, exclusions, whitelist, hidden_desireds, recurse)
end

!!exclusion_match || !!whitelist_match
initial_purgelist.reject do |path|
regular_desireds.any? { |desired| matches?(desired, path) }
end
end

Expand Down
Empty file added spec/fixtures/unit/util/purgeable/managed_one/managed_subdir_1/managed_subdir_2/ignored_1
View file
Open in desktop
Empty file.
Empty file added spec/fixtures/unit/util/purgeable/managed_two/.hidden/unmanaged_3
View file
Open in desktop
Empty file.
44 changes: 38 additions & 6 deletions spec/unit/util/purgeable_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
'spec/fixtures/unit/util/purgeable/managed_one/managed_subdir_1/subdir_new_1',
'spec/fixtures/unit/util/purgeable/managed_two/expected_2',
'spec/fixtures/unit/util/purgeable/managed_two/new_2',
'spec/fixtures/unit/util/purgeable/managed_two/.hidden',
]
end

Expand Down Expand Up @@ -98,7 +99,13 @@

describe '#current_contents' do
it 'collects contents of all managed directories recursively' do
expect(subject.current_contents(recurse)).to contain_exactly(/\/expected_1/, /\/expected_2/, /\/unmanaged_1/, /\/unmanaged_2/, /\/managed_subdir_1/, /\/subdir_expected_1/, /\/subdir_unmanaged_1/)
expect(subject.current_contents(recurse)).
to contain_exactly(/\/expected_1/, /\/expected_2/,
/\/unmanaged_1/, /\/unmanaged_2/,
/\/managed_subdir_1/,
/\/subdir_expected_1/, /\/subdir_unmanaged_1/,
/\/managed_subdir_2/, /\/ignored_1/,
/\/\.hidden/)
end
end

Expand All @@ -114,7 +121,8 @@
let(:whitelist) { [] }

it 'collects current_contents that should not exist recursively' do
expect(subject.stale_contents(recurse, exclusions, whitelist)).to contain_exactly(/\/unmanaged_1/, /\/unmanaged_2/, /\/subdir_unmanaged_1/)
expect(subject.stale_contents(recurse, exclusions, whitelist)).
to contain_exactly(/\/unmanaged_1/, /\/unmanaged_2/, /\/subdir_unmanaged_1/, /\/ignored_1/)
end
end

Expand All @@ -124,7 +132,17 @@

it 'collects current_contents that should not exist except whitelisted items' do
expect(subject.logger).to receive(:debug).with(/unmanaged_1.*whitelist match/i)
expect(subject.stale_contents(recurse, exclusions, whitelist)).to contain_exactly(/\/unmanaged_2/, /\/subdir_unmanaged_1/)

expect(subject.stale_contents(recurse, exclusions, whitelist)).
to contain_exactly(/\/unmanaged_2/, /\/subdir_unmanaged_1/, /\/ignored_1/)
end

it 'does not collect contents that match recursive globbed whitelist items as intermediate values' do
recursive_whitelist = ['**/managed_subdir_1/**/*']
expect(subject.logger).not_to receive(:debug).with(/ignored_1/)

expect(subject.stale_contents(recurse, exclusions, recursive_whitelist)).
to contain_exactly(/\/unmanaged_2/, /\/managed_one\/unmanaged_1/)
end
end

Expand All @@ -134,7 +152,17 @@

it 'collects current_contents that should not exist except excluded items' do
expect(subject.logger).to receive(:debug2).with(/unmanaged_2.*internal exclusion match/i)
expect(subject.stale_contents(recurse, exclusions, whitelist)).to contain_exactly(/\/unmanaged_1/, /\/subdir_unmanaged_1/)

expect(subject.stale_contents(recurse, exclusions, whitelist)).
to contain_exactly(/\/unmanaged_1/, /\/subdir_unmanaged_1/, /\/ignored_1/)
end

it 'does not collect contents that match recursive globbed exclusion items as intermediate values' do
recursive_exclusions = ['**/managed_subdir_1/**/*']
expect(subject.logger).not_to receive(:debug).with(/ignored_1/)

expect(subject.stale_contents(recurse, recursive_exclusions, whitelist)).
to contain_exactly(/\/unmanaged_2/, /\/managed_one\/unmanaged_1/)
end
end
end
Expand Down Expand Up @@ -179,7 +207,11 @@
end

context "recursive whitelist glob" do
let(:whitelist) { managed_directories.collect { |dir| File.join(dir, "**", "*unmanaged*") } }
let(:whitelist) do
managed_directories.flat_map do |dir|
[File.join(dir, "**", "*unmanaged*"), File.join(dir, "**", "managed_subdir_2")]
end
end
let(:purge_opts) { { recurse: true, whitelist: whitelist } }

describe '#purge!' do
Expand Down Expand Up @@ -214,7 +246,7 @@
context "when class does not implement #purge_exclusions" do
describe '#purge!' do
it 'purges normally' do
expect(FileUtils).to receive(:rm_r).at_least(3).times
expect(FileUtils).to receive(:rm_r).at_least(4).times

subject.purge!(purge_opts)
end
Expand Down