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.

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

Recursive monitoring request #36

Open
Vizor opened this issue Mar 30, 2016 · 12 comments
Open

Recursive monitoring request #36

Vizor opened this issue Mar 30, 2016 · 12 comments

Comments

@Vizor
Copy link

Vizor commented Mar 30, 2016

Please, it's possible to add support for recursive monitoring? On Windows there is argument bWatchSubtree (see here), which is set to FALSE for now.

@sbelsky
Copy link
Contributor

sbelsky commented Mar 30, 2016

short answer: yes :) but this requires a changes in the dir_monitor interface.

long answer: i have got a working copy with functional requested by you. and i want to merge it in the nearest future. but I can't do it right now because my changes have been made onto base Boris's implementation of dir_monitor. so i need some additional job for adoptation.

@Vizor
Copy link
Author

Vizor commented Mar 30, 2016

Ok, thank you for answer :).

@sbelsky
Copy link
Contributor

sbelsky commented Mar 30, 2016

how much time you can endure without this feature?)))

@Vizor
Copy link
Author

Vizor commented Mar 30, 2016

I have own fork with only setted this argument, so there's no hurry :).

@akapelrud
Copy link
Contributor

It would be nice if this was consistent between the different implementations.

  • The inotify one has recursive support, but it is impossible to turn it off atm (relatively easy to fix, as Inotify is not recursive by default).
  • The OSX one (fsevents) is recursive by default, but It would be nice to have a non-recursive implementation (filter) here as well.
  • Windows has native support for both recursive and nonrecursive watching, but as you noticed, it's turned off in the code.

@berkus
Copy link
Owner

berkus commented Apr 2, 2016

Agree with @akapelrud lets add some flag in the interface (or another method set like add_directory_recursive()) for the recursive case and make default non-recursive.

Opinions?

@sbelsky
Copy link
Contributor

sbelsky commented Apr 2, 2016

agree in common but need to clarify details in my realization.. on monday.

@akapelrud
Copy link
Contributor

@sbelsky I implemented adding of non-recursively monitored directories for inotify on my own master branch before easter. Have a look at commit 51275f6. Feel free to take that code if you need it. (I pushed my local changes to my master branch, but since then you have started using a develop branch, so my fork is a bit of a mess atm.)

Making the add_directory() functions work in both recursive and non-recursive mode isn't that hard, the hassle lies in implementing this so that directories can be removed afterwards. For instance, a test case could be

  1. Add one directory foo/bar/foo2 non-recursively.
  2. Add the directory foo recursively.
  3. Remove the directory foo.
  4. First directory should still be watched by dir_monitor.
  • On OSX (fsevents) you can just add directories and check paths against the stored directory paths as the events happen.
  • For inotify you can be a bit more clever, as it's non-recursive by default.
  • On windows you can choose either strategies above, as the API supports both.

@sbelsky
Copy link
Contributor

sbelsky commented Apr 4, 2016

@akapelrud thank you for analyze and code sharing. I fully agree with your arguments but want to add my view.
recursive directory monitoring is more complex task than even you describe because performance questions come into play. just imagine a folder with a big big subtree. i don't think real user want to watch over all leafs, just a subset.
my proposal:

  1. yes, this must be consistent between the different implementations. each of us specializes in its own operating system and it should be used.
  2. library's interface should stay backward compatible
  3. we can introduce new overloaded add_directory method with second unsigned integer argument. the meaning of this argument is a recursion depth. 0 means no recursion. i did it in my company's fork but as the wrapper of dir_monitor and only for windows.

@akapelrud
Copy link
Contributor

  1. Yes, and it currently isn't. That can give some surprises if portable code is needed.
  2. Yes, or simply alter the current add_directory function to have a second default-valued argument. I would prefer your suggestion though.
  3. I like the recursion depth argument, maybe simply a negative value can be interpreted as infinite recursion (the current default for inotify).

@GamePad64
Copy link
Contributor

Oh, I have tested dir_monitor only on the inotify backend.
I thought it already has recursive directory scanning for all platforms (inotify backend is recursive).

@BurningEnlightenment
Copy link

From my point of view one can't keep the interface backwards compatible and fix the inconsistent behaviour at the same time, because by fixing the inconsistent behaviour the semantics of add_directory are changed on at least one platform.
My suggestion is to introduce two new methods with well defined semantics like add_directory_recursive and add_directory_shallow. While we leave add_directory untouched and deprecate it (with c++14 the [[deprecated]] attribute could be used).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants