guard / listen

The Listen gem listens to file modifications and notifies you about the changes.
https://rubygems.org/gems/listen
MIT License
1.93k stars 247 forks source link

do not treat non-existent paths as dirs to prevent further scanning [fix #354] #357

Closed unjello closed 8 years ago

e2 commented 8 years ago

I think I'd prefer a :none result here. (Especially if there's no test for it).

Even if it increases the branch complexity, at least it's more semantically correct and more intuitive to deal with later.

unjello commented 8 years ago

Agreed. Although to be honest it deserves a rewrite anyway. At least a minor one where you could infer the type from inspecting previous records. But that may be the next step. For starters - lets stop crashing :)

e2 commented 8 years ago

Although to be honest it deserves a rewrite anyway.

The problem is finding time to do so. Listen 3.x was already a major rewrite (which happened not long ago).

At least a minor one where you could infer the type from inspecting previous records.

No, that approach actually failed. It's best to never assume what's on the filesystem may match what was recorded previously. Moving trees would just cause all kinds of problems.

For starters - lets stop crashing :)

That's not a good approach. Every problem has to be well understood and fixed properly. Because a crash is easy to fix, but debugging a logical problem can take ages - and it's often impossible to reproduce.

Crashes are good. The solution to crashes is understanding and prevention. (Otherwise I'd be digging myself a grave...)

You use case seems strange. Let's look at the code here (simplified for clarity):

      current = Set.new(Pathname('foo').children)
      current.each do |full_path|
        ::File.lstat(full_path.to_s)
      end

An Errno::ENOENT here suggest that the list of files (children) is no longer valid.

To me the solution is to handle Errno::ENOENT by rescanning the directory (because the old list obviously no longer reflects what's on the filesystem).

So I'd suggest something like this:

begin
  current = Set.new(Pathname('foo').children)
  current.each do |full_path|
    ::File.lstat(full_path.to_s)
    # (...)
  end
rescue Errno::ENOENT
  retry
end

(And so self.detect_type shouldn't handle the exception).

What do you think?

e2 commented 8 years ago

Let me know if this solves the crashing problem: https://github.com/guard/listen/pull/358

unjello commented 8 years ago

Fixed by #358 and released in 3.0.5 with a different fix. Thanks :)