D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko closed this revision.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko updated this revision to Diff 51762.
poboiko added a comment.


  Forgot to define `fname` inside `EventMoveTo`

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18698?vs=51739&id=51762

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

AFFECTED FILES
  autotests/unit/file/kinotifytest.cpp
  src/file/kinotify.cpp
  src/file/kinotify.h

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns accepted this revision.
bruns added a comment.
This revision is now accepted and ready to land.


  Thanks!

REPOSITORY
  R293 Baloo

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko updated this revision to Diff 51739.
poboiko added a comment.


  Updated comment, removed duplicated `QFile::decodeName`

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18698?vs=51062&id=51739

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

AFFECTED FILES
  autotests/unit/file/kinotifytest.cpp
  src/file/kinotify.cpp
  src/file/kinotify.h

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> poboiko wrote in kinotify.cpp:390
> Right, sorry, misprinted. 
> Actually, since there are a lot of `decodeName` calls around, probably it 
> would be better to decode it just once, right before `event->mask` matching 
> then, and then pass it everywhere?
> (less code duplication & mutliple calls if `mask` matches several events...)

Yes, but such a cleanup should go into a different review.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> bruns wrote in kinotify.cpp:390
> yes, but just `const QString fname = ...`

Right, sorry, misprinted. 
Actually, since there are a lot of `decodeName` calls around, probably it would 
be better to decode it just once, right before `event->mask` matching then, and 
then pass it everywhere?
(less code duplication & mutliple calls if `mask` matches several events...)

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> poboiko wrote in kinotify.cpp:390
> I'm a bit lost, why is it a problem? Or do you mean it's a costly operation 
> and suggest to do the following?
> 
>   const QString& fname = QFile::decodeName(path);
>   Q_EMIT created(fname);
>   [...]
>   handleDirCreated(fname);

yes, but just `const QString fname = ...`

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> bruns wrote in kinotify.cpp:390
> `QFile::decodeName(path)` twice ...

I'm a bit lost, why is it a problem? Or do you mean it's a costly operation and 
suggest to do the following?

  const QString& fname = QFile::decodeName(path);
  Q_EMIT created(fname);
  [...]
  handleDirCreated(fname);

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> kinotify.h:198
> + * and installs watches for all subdirectories (including @param path)
> + */
> +void handleDirCreated(const QString& path);

Can you add  a note created events are only emitted if the file/directory is 
not exclude by the config, likewise for installed watches?

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added a comment.


  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.

INLINE COMMENTS

> kinotify.cpp:390
> +// is installed. Ensure created events for all children are 
> issued at least once
> +handleDirCreated(QFile::decodeName(path));
>  }

`QFile::decodeName(path)` twice ...

> kinotify.cpp:441
> +if (event->mask & IN_ISDIR) {
> +handleDirCreated(QFile::decodeName(path));
> +}

dito ...

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko added a comment.


  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.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko updated this revision to Diff 51062.
poboiko edited the summary of this revision.
poboiko added a comment.


  Added recursive iteration over all contents for `Create` event as well

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18698?vs=50967&id=51062

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

AFFECTED FILES
  autotests/unit/file/kinotifytest.cpp
  src/file/kinotify.cpp
  src/file/kinotify.h

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> bruns wrote in kinotify.cpp:429
> This is broken now, as you fail to emit created for any child.

Ignore, looked at the wrong line ...

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> kinotify.cpp:429
> +if (!it.next().isEmpty()) {
> +d->addWatch(it.filePath());
> +}

This is broken now, as you fail to emit created for any child.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added a comment.


  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

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko updated this revision to Diff 50967.
poboiko added a comment.


  Cosmetics

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18698?vs=50932&id=50967

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

AFFECTED FILES
  autotests/unit/file/kinotifytest.cpp
  src/file/kinotify.cpp

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added a comment.


  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.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added a comment.


  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.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko marked an inline comment as done.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko updated this revision to Diff 50932.
poboiko added a comment.


  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.

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18698?vs=50832&id=50932

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

AFFECTED FILES
  autotests/unit/file/kinotifytest.cpp
  src/file/kinotify.cpp

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko added a comment.


  > 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!

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added a comment.


  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.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kinotify.cpp:430
> +// HACK: For some reason, it.fileInfo() returns an 
> empty QFileInfo for `path` here
> +// Creating new QFileInfo works fine though
> +QFileInfo fi(it.filePath());

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.

Instead of creating another QFileInfo for each item, you can just do the 
following:

  if (event->mask & IN_ISDIR) {
  Baloo::FilteredDirIterator it(d->config, QFile::decodeName(path));
  if (!it.next().isEmpty()) {
  // add folder itself, it not excluded
  d->addWatch(it.filePath());
  }
  while (!it.next().isEmpty()) {
  Q_EMIT created(it.filePath(), it.fileInfo().isDir());
  if (it.fileInfo().isDir()) {
  d->addWatch(it.filePath());
  }
  }
  }
  Q_EMIT created(QFile::decodeName(path), event->mask & IN_ISDIR);

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko updated this revision to Diff 50832.
poboiko edited the summary of this revision.
poboiko added a comment.


  Explained the race condition in summary, expanded test to check if watches 
were installed correctly.
  
  I've encountered some pretty weird issue: the watch for the moved folder is 
not installed; but it is installed for all its subfolders.
  The reason is that `it.fileInfo()` returns empty `QFileInfo()` for the moved 
folder. But it works fine for all the subentries.
  That is not the issue with the previous version, because `KInotify::addWatch` 
doesn't check for `fileInfo().isDir()` explicitly: it just relies on `DirsOnly` 
flag for the iterator.
  
  There is a workaround: if I create `QFileInfo` by hand, using `QFileInfo 
fi(it.filePath())` instead of `it.fileInfo()`, everything seems to be working 
fine.
  (the test fails if I use `it.fileInfo()`, but it passes if I use this 
workaround)

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18698?vs=50776&id=50832

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

AFFECTED FILES
  autotests/unit/file/kinotifytest.cpp
  src/file/kinotify.cpp

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko marked 2 inline comments as done.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns added a comment.


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

INLINE COMMENTS

> kinotify.cpp:431
> +}
> +addWatch(QFile::decodeName(path), d->mode, d->flags);
> +} else {

`QFile:decodeName(path)` called twice for the same path

> kinotify.cpp:431
> +}
> +addWatch(QFile::decodeName(path), d->mode, d->flags);
> +} else {

This also traverses the directory tree twice, once with all items (here), and 
once for  directories (in KInotify::addWatch(...)).
Calling `if (it.fileInfo().isDir()) { d->addWatch(it.filePath()) }` in the loop 
should have the same effect.

REPOSITORY
  R293 Baloo

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Stefan Brüns]

bruns requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham, bruns
Cc: bruns, ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  This makes sense to me. Thanks for the test, too.

REPOSITORY
  R293 Baloo

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

To: poboiko, #baloo, #frameworks, ngraham
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D18698: [baloo/KInotify] Notify if folder was moved from unwatched place

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, Frameworks.
Herald added projects: Frameworks, Baloo.
poboiko requested review of this revision.

REVISION SUMMARY
  If a folder was moved from an unwatched place, `KInotify` will receive an 
`EventMoveTo` event,
  which doesn't have an `EventMoveFrom` counterpart, and thus it will emit only 
`created` signal
  for the moved directory, but not its contents.
  Also, it won't install watches for the directory (as it does in 
`EventCreate`).
  
  Instead use FilteredDirIterator to emit created() signal for all the contents 
as well, and add inotify watch
  
  Few side notes:
  
  1. It's not really realted to symbolic links, we just need to move a folder 
from excluded place to included.
  2. If we move folder from another device, `Inotify` doesn't create `Move` 
signal, but `Create` instead, which seems to be handled properly
  
  (I'm not really sure why, because it smells like a race condition here. 
  If i.e. we got `Create` signal -> then `file1` gets moved -> then we add 
watches -> then `file2` gets moved, information about file1 should get lost.
  Am I missing something here?)
  
  BUG: 342224

TEST PLAN
  Added a test case for `KInotifyTest`, similar to one described in Bug 342224. 
  It passes now.

REPOSITORY
  R293 Baloo

BRANCH
  add-watch-moved (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D18698

AFFECTED FILES
  autotests/unit/file/kinotifytest.cpp
  src/file/kinotify.cpp

To: poboiko, #baloo, #frameworks
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams