Add only canonical paths to dirWatcher
ClosedPublic

Authored by hoffmannrobert on Dec 4 2019, 10:33 AM.

Details

Summary

KDirWatch only works correctly with canonical paths, i.e. symbolic links resolved.

Test Plan
  1. Create dirs:

$ mkdir test1
$ mkdir test1/subdir
$ ln -s test1 test2

  1. Start dolphin, navigate to test2/subdir, then in terminal:

$ echo test > test2/subdir/test

Without the patch, test2/subdir/test won't be shown automatically, only after reload (F5).
With the patch applied, test2/subdir/test will be shown automatically.

  1. Restart dolphin, navigate to test2/subdir, then in terminal:

$ echo test >> test2/subdir/test

Without the patch, the increased size of test2/subdir/test won't be shown automatically,
only after reload (F5). With the patch applied, it will be shown automatically.

  1. Restart dolphin, navigate to test2/subdir, then in terminal:

$ rm test2/subdir/test

Without the patch, test2/subdir/test stays visible, will only disappear after reload (F5).
With the patch applied, it will disappear automatically.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
hoffmannrobert created this revision.Dec 4 2019, 10:33 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptDec 4 2019, 10:33 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
hoffmannrobert requested review of this revision.Dec 4 2019, 10:33 AM
dfaure added a comment.Dec 6 2019, 8:06 AM

This should go into KDirWatch itself then, to avoid the risk of other callers falling into that trap.

dfaure requested changes to this revision.Dec 6 2019, 8:11 AM

See also commit 6be886bafae463ba in kcoreaddons (which was wrong, it would seem -- there are better ways to prevent infinite loops than to say "let's not follow symlinks at all").

Doing this in kcoreaddons is also an opportunity to add a unittest for this :)

What I wonder, however, is whether we also want to be notified if someone does rm test2, because then test2/subdir/test no longer exists (under that name)...

This revision now requires changes to proceed.Dec 6 2019, 8:11 AM

This should go into KDirWatch itself then, to avoid the risk of other callers falling into that trap.

I'm not sure, if that's a good idea.

  1. It's legal to add non-existing directories to KDirWatch. Calling QFileInfo::canonicalFilePath() on such a dir would return an empty string for it.
  1. Ok, you could check if it exists before canonicalizing it, but it still could be a path containing symlinks, so you need to know what you are doing anyway, when adding something to KDirWatch.
  1. In other places (e.g. KIO KCoreDirLister), paths added to KDirWatch are already canonicalized. Adding the QFileInfo::canonicalFilePath() call to KDirWatch would result in doing this operation twice.

    But initializing a QFileInfo and especially calling canonicalFilePath() are quite expensive operations. On locally mounted network paths these operations can take several seconds if the network is slow, for example when using a laptop with a VPN connection. Clicking through a directory tree can be a tedious task if you have to wait ages until the next dir level is shown.

    Because of this, at our LiMux project in Munich we don't use QFileInfo::canonicalFilePath() on locally mounted netshares paths (cifs, no symlinks), but an own fast method which resolves the symlinks in the users' netshares directories to the mounted netshares (below /mnt) and caches them. This symlink construction is the reason why this bug showed up, by the way.
dfaure accepted this revision.Dec 14 2019, 11:39 AM

Thanks for your well thought-out reply.
The usual solution to 1. and 2. is rather "call canonicalFilePath, and if the return value is empty, use the original path". We do this in many places. But indeed this means, if a non-existing dir gets created later, it won't be canonicalized. But that's not worse than the current situation, and not an issue for the problem at hand (different use case, could be fixed in KDirWatch if needed one day).

You are very correct about 3, resolving in two places is slow. This is the usual problem with shifting responsibilities around...

I did some archeology, and KDirLister needs to keep a mapping because kdirwatch needs canonical paths while the model (upon receiving notifications of changes) only understands the paths as initially given (with symlinks). On hindsight, maybe I should have moved that one level down. It would be convenient for all KDirWatch users if KDirWatch itself would do this mapping, making things transparent for users of KDirWatch. You ask to watch X, you get notifications about X.

So I consider the "cleanest solution, in an ideal world" to implement such a mapping in KDirWatch, and remove it from KCoreDirLister (to avoid doing it twice).
However I can see how this is a much more involved change than what is proposed here, which continues in the same direction as what was already done, resolving in the callers of KDirWatch. So I'll accept it.
It would be nice if someone did the right thing in KDirWatch though, so we can avoid N applications hitting this problem separately and all of them having to implement workarounds :-)

This revision is now accepted and ready to land.Dec 14 2019, 11:39 AM

(forgot to paste more details about the archeology: kdelibs commit d3a9f1037d1410, bugs 213799 and 219547)

elvisangelaccio requested changes to this revision.Mon, Jan 6, 3:36 PM
elvisangelaccio added a subscriber: elvisangelaccio.

Just minor nitpicks, looks good otherwise.

src/kitemviews/private/kdirectorycontentscounter.cpp
89–93

Missing const. I'd also call this variable resolvedPath or canonicalPath.

113–117

Same here

This revision now requires changes to proceed.Mon, Jan 6, 3:36 PM
hoffmannrobert marked 2 inline comments as done.
  • Add const, rename variable

Added const, renamed variables.

dfaure accepted this revision.Sat, Jan 11, 9:16 AM
elvisangelaccio accepted this revision.Sat, Jan 11, 12:45 PM

Do you have commit access?

This revision is now accepted and ready to land.Sat, Jan 11, 12:45 PM

Yes, I do, but this will be my first push and I don't want to break something. So all I have to do is

arc land

in my feature branch to merge these two commits and push (push remote is git://git.kde.org/dolphin.git). Correct?

Yep, that's correct!

This revision was automatically updated to reflect the committed changes.