wxFileSystemWatcher: Fix watching directories that contain symlinks

[dghart]

This patch, together with the wxFileName and wxDir changes of #14542, makes it possible to add a watch on a dir that contains symlinks, without wxFileSystemWatcher asserting that items are being added twice. I've added a new flag, wxFSW_EVENT_DONT_FOLLOW, to the Add() filter. This is not part of wxFSW_EVENT_ALL, so the current default behaviour is unchanged.

I've added a menuitem to the 'fswatcher' sample optionally to set this, and a section to the FileSystemWatcherTestCase.

[PB]

(In #14544) Replying to dghart:

(In #14488) Replying to vadz:

I wonder if we shouldn't just remove the filespec argument. If it's really going to simplify the implementation a lot, I'd be ready to sacrifice it. After all, you always can easily filter out the files you're not interested in in your event handler and usually you'd want to watch all files anyhow, wouldn't you? If we want to keep support for this feature, we'd need to add individual watches for all files matching it under the given tree. And possibly also internal watches for all directories that would be handled to add watches for any newly created files. This doesn't seem easy to do...

Fortunately the latest patch, #14544, changes how a filespec is handled and solves these issues.

Vadim may have meant it generally: underlying native MSW API doesn't support watching individual files so there's still that... :( I believe that the documentation should mention that watching individual files and using filter is not supported on MSW.

[vadz]

(In #14544) I'll try to look at this series soon although to be honest I'm a bit intimidated. It might be better to redo a series of patches (0001-Something, 0002-SomethingElse, ...) if parts of the later patches change something done by the earlier ones as this should make reviewing them simpler.

I.e. perhaps it would be more convenient for both of us if you created a new branch, cherry pick all your commits there and used git rebase -i to reorganize them in the optimal way and then just used git format-patch?

If not, I'll try to look at this stuff starting from #14488 on Thursday.

[dghart]

(In #14544) Replying to vadz:

I'll try to look at this series soon although to be honest I'm a bit intimidated. It might be better to redo a series of patches (0001-Something, 0002-SomethingElse, ...) if parts of the later patches change something done by the earlier ones as this should make reviewing them simpler.

I.e. perhaps it would be more convenient for both of us if you created a new branch, cherry pick all your commits there and used git rebase -i to reorganize them in the optimal way and then just used git format-patch?

If not, I'll try to look at this stuff starting from #14488 on Thursday.

I tried to keep the patches clean and simple. With the exception of the unit-test, there are only minor 'internal' reversions in the series; by minor I mean just a couple of lines. So I think you'll find it easier that you expect.

If not, I'll redo them differently.

[vadz]

Sorry, I should have noticed it immediately and not 3 months later, but I don't like the approach of this patch. The whole idea of adding wxFileName::DontFollowLink() was to avoid adding other "no follow" flags like this so instead of adding this wxFSW_EVENT_DONT_FOLLOW (which really doesn't fit into "events" enum at all) I'd prefer to just be able to do

wxFileName fn(wxFileName::DirName("..."));
fn.DontFollowLink();
fsw.Add(fn);

Granted, this stretches the meaning of this flag a little for AddTree() but it makes so much sense for Add() that I think it's forgiveable to extend it to AddTree() too.

What do you think?

[vadz]

(In #14544) Thanks, I've applied everything here, I think, but I had to do it by combining some patches to avoid breaking unit tests.

The only change I intentionally did (as opposed to any number of bugs I could have introduced unintentionally...) was to change the filespec check to use the full file name instead of just its extension, I don't see any reason why we shouldn't allow watching for e.g. "syslog*".

Please let me know if I broke anything else and thanks again for all your work on this!

[VZ]

(In #14544) (In [72681]) Check for filespec when generating events in wxFileSystemWatcher.

Instead of setting watches on individual files when a non-empty filespec is
given, always watch all the files but just ignore the events from the ones not
matching the filespec. This makes the code simpler and fixes several bugs.

See #14544.

[VZ]

(In #14544) (In [72682]) Fix bug in Unix wxFileSystemWatcher implementation when watch is deleted.

Don't assert when removing a watch descriptor from the stale descriptors list.

See #14544.

[VZ]

(In #14544) (In [72683]) Handle deletion of watched directories in wxFileSystemWatcher sample.

Don't assert when trying to stop watching a directory that doesn't exist any
more later.

See #14544.

[dghart]

(In #14544) Replying to vadz:

Thanks, I've applied everything here, I think, but I had to do it by combining some patches to avoid breaking unit tests.

Sorry to reopen this, but unless I'm missing something (which is quite possible; the complex series of patches confuses me too) I don't think you applied CreateDelete.diff

[vadz]

(In #14544) Oops, sorry about this and thanks for noticing it. Hopefully nothing else got forgotten, so re-closing it, thanks again.

[VZ]

(In #14544) (In [72689]) Improve inotify()-based wxFileSystemWatcher to handle creation/deletion.

Handle creation and deletion of directories under the watched path better.

See #14544.

[dghart]

Replying to vadz:

Sorry, I should have noticed it immediately and not 3 months later, but I don't like the approach of this patch. The whole idea of adding wxFileName::DontFollowLink() was to avoid adding other "no follow" flags like this so instead of adding this wxFSW_EVENT_DONT_FOLLOW (which really doesn't fit into "events" enum at all)

The wxFSW_EVENT enum is currently unused except for wxFSW_EVENT_ALL so, if it's a naming issue, changing wouldn't be difficult (except for thinking of a better one).

I'd prefer to just be able to do
{{{
#!cpp
wxFileName fn(wxFileName::DirName("..."));
fn.DontFollowLink();
fsw.Add(fn);
}}}

Granted, this stretches the meaning of this flag a little for AddTree() but it makes so much sense for Add() that I think it's forgiveable to extend it to AddTree() too.

What do you think?

I'm happy with that. The main drawback is, as in #14542, the loss of the self-documenting nature of the extra flag.

Please let me know if you want me to rework the patch.

[vadz]

I'm not sure why do you think this enum is unused, I think it is used and does allow to specify which events you're interested in, doesn't it?

I think we should either use the suggestion above or maybe replace (well, augment) Add() and AddTree() by some new method (Watch()?) which would take

1. The path.
2. The flags specifying the kind of watched to add (wxFSW_FLAGS_RECURSIVE, for the current AddTree(), to follow the links or not (wxFSW_FLAGS_NO_FOLLOW) and anything else we can think about.
3. The event mask (wxFSW_EVENT_XXX combination).
4. Pattern.

Actually I think that the latter approach might make more sense than the current Add()/AddTree() but it clearly would require more work too. So for now I'd be happy with the former, wxFileName::DontFollowLink()-using one.

TIA!

[dghart]

Replying to vadz:

I'm not sure why do you think this enum is unused, I think it is used and does allow to specify which events you're interested in, doesn't it?

I was half-remembering the Watcher2NativeFlags() situation, which ignores the passed flags and returns the native events matching wxFSW_EVENT_ALL.

I think we should either use the suggestion above or maybe replace (well, augment) Add() and AddTree() by some new method (Watch()?) which would take

1. The path.
1. The flags specifying the kind of watched to add (wxFSW_FLAGS_RECURSIVE, for the current AddTree(), to follow the links or not (wxFSW_FLAGS_NO_FOLLOW) and anything else we can think about.
1. The event mask (wxFSW_EVENT_XXX combination).
1. Pattern.

Actually I think that the latter approach might make more sense than the current Add()/AddTree() but it clearly would require more work too.

That sounds good to me too. It should be easy to implement as (unless I misunderstand) the new Watch() is just a public wxFileSystemWatcherBase::AddAny with a different parameter order.

The new wxFSW_FLAGS_* sounds very similar to the current wxFSWPathType enum. Should they be merged? And what about Add() and AddTree()? Should they be removed or deprecated?

[vadz]

Replying to dghart:

I think we should either use the suggestion above or maybe replace (well, augment) Add() and AddTree() by some new method (Watch()?) which would take

1. The path.
1. The flags specifying the kind of watched to add (wxFSW_FLAGS_RECURSIVE, for the current AddTree(), to follow the links or not (wxFSW_FLAGS_NO_FOLLOW) and anything else we can think about.
1. The event mask (wxFSW_EVENT_XXX combination).
1. Pattern.

Actually I think that the latter approach might make more sense than the current Add()/AddTree() but it clearly would require more work too.

That sounds good to me too. It should be easy to implement as (unless I misunderstand) the new Watch() is just a public wxFileSystemWatcherBase::AddAny with a different parameter order.

Right. I called it Watch() just because I don't know what else to call it but maybe we could even continue calling it just Add() if there are no overloading conflicts here.

The new wxFSW_FLAGS_* sounds very similar to the current wxFSWPathType enum. Should they be merged?

I'm not sure about the similarity. I propose to have flags, i.e. values that can be combined with each other, while wxFSWPath_File and wxFSWPath_Dir can't be combined. And while we apparently (I don't actually remember why is it so, but we presumably wouldn't have done it like this otherwise) need to specify whether we're dealing with a file or a directory internally, I don't think we need to expose this in the public API, just giving the path should be enough.

And what about Add() and AddTree()? Should they be removed or deprecated?

I definitely don't think they should be removed, there is nothing really wrong with them. And I don't even think we need to deprecate them, it doesn't cost much to keep them as inline wrappers for the new function.

[dghart]

I implemented this; then on testing I couldn't make the current code fail when it should have done! I expect the ref-counting of watches helped, but it seems that the r72680 wxDir fix stopped symlink-to-dirs being passed to inotify, so adding the IN_DONT_FOLLOW native flag is no longer necessary. As that was the only reason for the wxFSW_EVENT_DONT_FOLLOW flag, I'm glad to say that this bug can be closed as invalid (I'll reopen it if real-world testing succeeds in failing).

I doubt if a Watch() method is worth having now, as its flags parameter would hold only wxFSW_FLAGS_RECURSIVE, but I still have the code if you feel it is.

[vadz]

Actually I think it's a bad thing that symlinks to dirs don't work any longer because it's (silently) incompatible with the old behaviour. So ideal would be to restore this by default but add DONT_FOLLOW as an optional flag.

In fact this probably means that the behaviour of wxDirTraverser itself changed so it affects even more code than just wxFSWatcher. And this makes it even more important to restore the old behaviour that people could be relying on.

[vadz]

P.S. IOW I think we should reverse the path changing src/unix/dir.cpp of r72680 and maybe provide some other way of not following symlinks during the traversal. I suppose that after this reversal the problem here would reappear, wouldn't it?

[dghart]

Replying to vadz:

Actually I think it's a bad thing that symlinks to dirs don't work any longer because it's (silently) incompatible with the old behaviour. So ideal would be to restore this by default but add DONT_FOLLOW as an optional flag.

Sadly, I have to agree with you. I'll rework my code so that it just affects the traversal (I'm fairly sure that adding an IN_DONT_FOLLOW native flag will still not be needed).

In fact this probably means that the behaviour of wxDirTraverser itself changed so it affects even more code than just wxFSWatcher. And this makes it even more important to restore the old behaviour that people could be relying on.

Yes, I'll reopen #14542 and provided a way to make the wxDir change optional, and off by default.

[dghart]

The new fswatcher.diff is a simplified version of the previous diffs, which it replaces. It makes AddTree() and RemoveTree() check wxFileName::ShouldFollowLink to determine whether to pass the new flag wxDIR_NO_FOLLOW to wxDir::Traverse. As before the bulk of the patch is to demonstrate this in the sample and test.

This patch depends on the new fix for #14542

[vadz]

Thanks, I'm going to apply this but ideally we should mention this behaviour in the docs as it's probably not obvious. Also, I'd need to undo s/Connect/Bind/ in the sample as the latter can't be used with all compilers so we still have to use Connect() inside wx itself unfortunately. If you can update the patch like this, it'd be great. TIA!

[dghart]

Replying to vadz:

Thanks, I'm going to apply this but ideally we should mention this behaviour in the docs as it's probably not obvious.

Oops, I forgot to merge in the original docs.diff. Done, and updated for the changes.

Also, I'd need to undo s/Connect/Bind/ in the sample as the latter can't be used with all compilers so we still have to use Connect() inside wx itself unfortunately.

I hadn't realised. I've updated fswatcher.diff

[VZ]

(In [72751]) Respect wxFileName::DontFollowLink() in wxFileSystemWatcher.

Watch the link itself and not its target if DontFollowLink() had been called.

Closes #14543.

[VZ]

(In [72753]) Don't iterate over files in wxFileSystemWatcherBase.

We ignore the files anyhow when recursively adding watches for the entire
tree, so don't include them in the iteration.

See #14543.