D28472: [DirectorySizeJob] Fix sub-dirs count when resolving symlinks to dirs

[David Faure]

dfaure added a comment.


  Suggested fix in D28601 

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28472: [DirectorySizeJob] Fix sub-dirs count when resolving symlinks to dirs

[David Faure]

dfaure added a comment.


  Does the test pass for you now?
  
  For me it fails (and it fails on FreeBSD CI too 
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.14/37/testReport/junit/projectroot/autotests/kiocore_jobtest/)
  
  FAIL!  : JobTest::directorySize() Compared values are not the same
  
Actual   (job->totalSubdirs()): 3
Expected (4ULL)   : 4
Loc: [/d/kde/src/5/frameworks/kio/autotests/jobtest.cpp(1133)]
  
  What's happening is
  
  QDEBUG : JobTest::directorySize() KIO::DirectorySizeJobPrivate::slotEntries 
subdir "dirFromHome" => 1
  QDEBUG : JobTest::directorySize() KIO::DirectorySizeJobPrivate::slotEntries 
Skip visited inode "dirFromHome_link"
  QDEBUG : JobTest::directorySize() KIO::DirectorySizeJobPrivate::slotEntries 
subdir "dirFromHome_copied" => 2
  QDEBUG : JobTest::directorySize() KIO::DirectorySizeJobPrivate::slotEntries 
subdir "dirFromHome_copied/dirFromHome" => 3
  
  http://www.davidfaure.fr/2020/debug.diff
  
  I think the problem is that order is undefined. If the symlink is seen first, 
both will be counted (your if() is only around the insert). If it's seen 
second, it'll be skipped.
  
  I think we should skip the whole visitedInodes thing for symlinks, not just 
the insert.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28472: [DirectorySizeJob] Fix sub-dirs count when resolving symlinks to dirs

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:e247985be433: [DirectorySizeJob] Fix sub-dirs count when 
resolving symlinks to dirs (authored by ahmadsamir).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28472?vs=79014=79024

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

AFFECTED FILES
  src/core/directorysizejob.cpp

To: ahmadsamir, #frameworks, dfaure, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28472: [DirectorySizeJob] Fix sub-dirs count when resolving symlinks to dirs

[David Faure]

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


  Yay for unittests (and my bad for not running them when making that change). 
Thanks for the fix!

REPOSITORY
  R241 KIO

BRANCH
  l-dirsizejob (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28472: [DirectorySizeJob] Fix sub-dirs count when resolving symlinks to dirs

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, meven.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  After 730a6ddd828fb1fbdf0f3 
, 
the behaviour changed and the code sets
  KIO::UDSEntry::UDS_DEVICE_ID after symlinks are resovled (if
  KIO::StatResolveSymlink is set); this meant that if we have dir A and
  a symlink to it B, the loop that detects hard-links will skip A if B
  was processed before, as both will have the same inode.
  
  This fixes the directorySize() unit test from kiocore-jobtest; the
  totalSubdirs() count didn't match the expected value.

TEST PLAN
  make && ctest

REPOSITORY
  R241 KIO

BRANCH
  l-dirsizejob (branched from master)

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

AFFECTED FILES
  src/core/directorysizejob.cpp

To: ahmadsamir, #frameworks, dfaure, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns