To which "file1"/"file2" are you referring in the summary? The commit log should be self contained.

In D18698#404820, @poboiko wrote:

Explained the race condition in summary, expanded test to check if watches were installed correctly.

I am not sure if I understood your description correctly, but I am quite sure this race condition does not exist - the files/folders inside the moved folder are not created/moved one by one, but the containing folder ist just "renamed" - it is unlinked from the old parent and linked into the new one, atomically.

I am not sure if I understood your description correctly, but I am quite sure this race condition does not exist - the files/folders inside the moved folder are not created/moved one by one, but the containing folder ist just "renamed" - it is unlinked from the old parent and linked into the new one, atomically.

Sure. That concern corresponded only to the second note - if moving from another device, system has to do actual copy/move.

FilteredDirIterator is somewhat odd, as it returns the traversed folder itself as the first element (if not filtered/excluded by the config).
The folder itself has no fileInfo, as it is not backed by a QDirIterator item. Unfortunately, this is not really trivial to fix.

Damn. I've though it's behavior is inherited from QDirIterator. Didn't look into FilteredDirIterator enough. Thanks!

In D18698#405636, @poboiko wrote:

Added code to work with first entry that pops from FilteredDirIterator (that is the directory itself)
Test still works; but we should emit created() signal for it as well.

Thats the reason I moved the Q_EMIT created(...) out of the if(mask & IN_ISDIR) branch.

Thinking about it, it should be before the branch, otherwise baloo may receive "created" events for the children before getting a "created" for the parent.

In D18698#405504, @poboiko wrote:

I am not sure if I understood your description correctly, but I am quite sure this race condition does not exist - the files/folders inside the moved folder are not created/moved one by one, but the containing folder ist just "renamed" - it is unlinked from the old parent and linked into the new one, atomically.

Sure. That concern corresponded only to the second note - if moving from another device, system has to do actual copy/move.

But when "moving" across devices, we should get regular "created" events for each directory and file, as copying/moving across devices is done one by one. This is definitely not of concern for the "MovedTo" case.

For the "Created" case, we may be actually missing events, although it is possible the baloo core itself syncs its state when getting a directory create event. Haven't checked it though.

In general, inotify is racy, but the code as written here would not be, even for "created" events:

1. For each directory, we add a watch, while traversing the parent directory.
2. Each directory is also pushed to m_paths in FilteredDirIterator - https://phabricator.kde.org/source/baloo/browse/master/src/file/filtereddiriterator.cpp$80
3. After traversing the parent, we descend into the first found directory - https://phabricator.kde.org/source/baloo/browse/master/src/file/filtereddiriterator.cpp$67
4. The subdirectory is traversed - note, the watch has been installed already

Addendum:
I have just checked, https://phabricator.kde.org/source/baloo/browse/master/src/file/filewatch.cpp$144

filewatch relies on the code here to emit created events for all children, so we need the traversal code not only for "moved", but also for "created", as otherwise we may miss some "created" events.

Duplicate "created" events is completely fine, these are merged in the "PendingFileQueue".

So I would like you to do the following:

• take the newly added code and create a function from it, e.g. handleCreateRecursive(event, path)
• move the Q_EMIT created(...) to the beginning of the function
• call this function for the "EventMoveTo && noCookie)" case
• call this function also for "EventCreated"

Probably also add a comment to the function why the manual traversal is needed:

// Files/directories inside the new directory may be created before the watch
// is installed. Ensure created events for all children are issued at least once

Something like that? I've decided not to emit created signal from inside the function, just to have a bit less branching in the code (and documented this behavior, since it might be a bit confusing)

Actually, I think this race condition is now handled properly, i.e. I can't think even of the case when we got created signal twice for the same file.

In D18698#406755, @poboiko wrote:

Something like that? I've decided not to emit created signal from inside the function, just to have a bit less branching in the code (and documented this behavior, since it might be a bit confusing)

Actually, I think this race condition is now handled properly, i.e. I can't think even of the case when we got created signal twice for the same file.

Yes, looks almost fine now.

It is still possible to get two "created" signals for a new file, but as said, two signals are the race we handle fine, only zero signals would be an issue:

= a =
it->next()   -- adds "foobar" to it.m_paths
addWatch("foobar")
= b =
it.next(), it.next(), ... it.next()   -- siblings of "foobar"
= c =
it.next() -- take "foobar" from it.m_paths, instantiate QDirIterator("foobar")
it.next(), ... it.next()  -- contents of "foobar"
it.next() -- destruct "foobar" iterator
= d =

Any file created after =b= and before =c= will emit "created" twice, once from the already created watcher, and once during the traversal
Files created before =a= will be missed by the watch (thus we need the iteration), files created after =d= will only be picked up by the watcher.