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

Don't return incorrect files when there's a file whose name matches a dir #524

Closed
wants to merge 1 commit into from
Closed

Don't return incorrect files when there's a file whose name matches a dir #524

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
14 changes: 7 additions & 7 deletions lib/listen/record.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ def dir_entries(rel_path)

def _sub_tree(rel_path)
@tree.each_with_object({}) do |(path, meta), result|
next unless path.start_with?(rel_path)

if path == rel_path
result.merge!(meta)
else
sub_path = path.sub(%r{\A#{rel_path}/?}, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I had to stare at it for a long time, but the bug is the ? after the /. That means rel_path could match just a prefix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also it was a bug that the rel_path wasn't escaped before interpolating it into a regexp. All sorts of weird things could happen if rel_path had characters in it that are special in regexps, like ^ $ [ \ ? | . * etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I generally find regexes super hard to grok, so I'm a fan of removing it in favour of more explicit code. That's a matter of personal preference though - I can go back to a regex-based solution if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with you! It is best to avoid regexps in code. My experience with them is that the likelihood of being 100% correct drops quadratically with every punctuation character used. :)

result[sub_path] = meta
parts = path.split(::File::SEPARATOR)
if parts.shift == rel_path
if parts.empty?
result.merge!(meta)
else
result[parts.join(::File::SEPARATOR)] = meta
end
end
end
end
Expand Down
23 changes: 23 additions & 0 deletions spec/lib/listen/record_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,29 @@ def record_tree(record)
end
end

context 'when there is a file with the same name as a dir' do
subject { record.dir_entries('cypress') }

before do
record.update_file('cypress.json', mtime: 1.1)
record.update_file('cypress/README.md', mtime: 1.2)
record.update_file('a/b/cypress/d', mtime: 1.3)
record.update_file('a/b/c/cypress', mtime: 1.3)
end
it { should eq('README.md' => { mtime: 1.2 }) }
end

context 'when there is a file with a similar name to a dir' do
subject { record.dir_entries('app') }

before do
record.update_file('appspec.yml', mtime: 1.1)
record.update_file('app/README.md', mtime: 1.2)
record.update_file('spec/app/foo', mtime: 1.3)
end
it { should eq('README.md' => { mtime: 1.2 }) }
end

context 'in subdir /path' do
subject { record.dir_entries('path') }

Expand Down