search

ar- / incron

cron-like daemon which handles filesystem events
Other
229 stars 51 forks source link

New options (dotdirs, recursion, etc.) break backward compatibility #19

Open k0lter opened 9 years ago

k0lter commented 9 years ago

Hi,

The default values for these new options added recently break backward compatibility. For people using incron installed with their favorite package manager, it could lead to unexpected behaviour. Especially with the recursion option which seems to be highly experimental.

What do you think about using default options which preserve backward compatibility?

M.

ar- commented 9 years ago

Yes. Maybe it is a good idea to delay the next debian package for another week so I can fix this. RECUSIVE=false should be the default.

DOTDIRS was acutaully a bugfix. Including DOTDIRS leads to errors, when using certain text editors (vim ?!). I thought it might be a good idea, to fix this bug (and so changing the current behaviour) and make it available as an option (so existing users, who depend on dot dirs can reenable it). This is an interesting question, what the defautl should actually be in future.

k0lter commented 9 years ago

IMO, recursion should be disabled by default (to preserve backward compatibility), and as you said previously It would be good to exclude hidden files/dirs by default (very low impact).

danfruehauf commented 9 years ago

I also think that the current fix is rather dangerous and incomplete.

at https://github.com/ar-/incron/blob/master/usertable.cpp#L393 - you may actually miss quite a bit of events in that second just because a directory was created. That's like 1 full second (at least) that incron will not process any events.

While I don't have a solution for the mkdir -p situation, I did do some work on 0.5.10 which can work much better without introducing that incomplete fix but still have recursive directory watching.

ar- commented 9 years ago

Hi Dan,

you are commenting on an unrelated issue. This issue is about the default settings.

Anyways I've checked your code and also merged it into my current source. I couldn't make it running yet, because you were not developing on the newest development branch.

As far as I see it, you use the same thing that I have left commented in the code, which updates the watches on-the-fly, and which doesn't work.

Just to understand you last sentence correctly: are you saying yours is working, incomplete, but less incomplete than mine?

danfruehauf commented 9 years ago

@ar- I didn't mean for you to incorporate my code as it was not branched from your latest master code (Lukas told me about this repository only after I've implemented my solution for 0.5.10). Hopefully I'll get to rebase my patch against your master branch.

My concern is this, imagine the following scenario:

  1. incron starts running (with recursive watches)
  2. something creates a directory (mkdir or mkdir -p, it doesn't matter)
  3. incron picks the event
  4. incron disposes all watches
  5. sleep(1); 5.1. An event occurs - but it is never picked up!
  6. incron rebuilds the watches (so it can also watch the newly created directory/ies

So what I'm trying to demonstrate is that in step 5.1 - events may be discarded. My recommendation would be to add just the directory that was added. Even though it will not monitor new directories that are created with mkdir -p. Because obviously the alternative is much worse.