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

Issue #548: fix error when renaming folder #552

Merged
merged 5 commits into from Jan 13, 2022
Merged

Issue #548: fix error when renaming folder #552

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ed3896
issue #548: refactor to add () around include? args
ColinDKelley Dec 30, 2021
6f0f0ec
issue #548: dir_entries now skips non-dir entries entirely
ColinDKelley Dec 30, 2021
af73ed4
issue #548: remove _auto_hash in favor of a reset_tree method
ColinDKelley Jan 9, 2022
e46ce2b
issue #548: use empty_dirname? method
ColinDKelley Jan 9, 2022
62fee1b
issue #548: remove superstitious ||= {} since that is implied in the …
ColinDKelley Jan 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/listen/directory.rb
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ def self.scan(snapshot, rel_path, options)
end

# TODO: this is not tested properly
previous = previous.reject { |entry, _| current.include? path + entry }
previous = previous.reject { |entry, _| current.include?(path + entry) }

_async_changes(snapshot, Pathname.new(rel_path), previous, options)
rescue Errno::ENOENT, Errno::EHOSTDOWN
Expand Down
16 changes: 9 additions & 7 deletions lib/listen/record.rb
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@ def unset_path(rel_path)

def file_data(rel_path)
dirname, basename = Pathname(rel_path).split.map(&:to_s)
if [nil, '', '.'].include? dirname
if [nil, '', '.'].include?(dirname)
@tree[basename] ||= {}
@tree[basename].dup
else
Expand All @@ -46,16 +46,18 @@ def file_data(rel_path)
end

def dir_entries(rel_path)
subtree = if ['', '.'].include? rel_path.to_s
rel_path_s = rel_path.to_s
subtree = if ['', '.'].include?(rel_path_s)
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving the assignment into the expression for readability sake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which assignment? rel_path_s =?

Copy link
Member

Choose a reason for hiding this comment

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

I would personally prefer:

if ...
  subtree = ... 
else
  subtree = ...
end

I think it's easier to read and less likely to cause bugs in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. To me, that would be a surprising step away from DRY code. I've found the common assignment can be less bug-prone, for example if an elsif is added. And it often identifies a good refactor point where the entire if/else cascade can move into a separate method. I'd rather leave this for now.

@tree
else
@tree[rel_path.to_s] ||= _auto_hash
@tree[rel_path.to_s]
@tree[rel_path_s] ||= _auto_hash
end

subtree.transform_values do |values|
# only get data for file entries
values.key?(:mtime) ? values : {}
subtree.each_with_object({}) do |(key, values), result|
# only return data for file entries
if values.respond_to?(:has_key?)
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this extra check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The root of this bug is that for folders the subtree has an array (edit: hash) of hashes (that each respond to has_key?). But for files, subtree is a hash with key-values. So if a folder gets renamed into a file, we were crashing here trying to treat a float (mtime) as if it were a hash.
So my hack here is to just treat the symptoms and skip over entries like that, if they happen. I'm sure there's a better answer, and it probably involves the comment higher up about deleting this entire class. But I wasn't able to take on a scope like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I think I could address the above more cleanly a few lines up. Maybe check if subtree is an Array at all? I think in the file case it will itself be a Hash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out my check for Array wouldn't work, because in fact the top-level object is a hash with the filename as key. The values in turn are hashes with symbols as keys (like :mtime) and scalar values.

Copy link
Member

Choose a reason for hiding this comment

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

I personally think respond_to? is a hack in this case. Not sure if there is a better alternative without spending some time debugging the data. Maybe you can pp the top level out so we can see exactly what it looks like?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed--it's definitely a hack!

I certainly could replace respond_to? with is_a?(Hash). Would that seem any less hacky to you? Over the years I've gone back and forth between asking objects is_a? vs respond_to?. The latter is a bit more "duck-typey" but sometimes less clear.

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing transform_values is more efficient too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, transform_values is probably a bit more efficient. I'd be shocked if there were a case where it mattered here, though. The real cost of this gem is the multiple threads it creates and all the event reprocessing it does.

result[key] = values.has_key?(:mtime) ? values : {}
end
end
end

Expand Down
10 changes: 9 additions & 1 deletion spec/lib/listen/record_spec.rb
View file
Open in desktop
Expand Up @@ -66,7 +66,7 @@ def record_tree(record)
end

context 'with subdir path' do
it 'sets path by spliting dirname and basename' do
it 'sets path by splitting dirname and basename' do
record.update_file('path/file.rb', mtime: 1.1)
expect(record_tree(record)['path']).to eq('file.rb' => { mtime: 1.1 })
end
Expand Down Expand Up @@ -257,6 +257,14 @@ def record_tree(record)
end
it { should be_empty }
end

context 'with path renamed to file' do
before do
record.add_dir('path/subdir')
record.update_file('path', mtime: 1.1)
end
it { should be_empty }
end
end
end

Expand Down