D15583: [Balooctl] fix directory parent check

[Nathaniel Graham]

ngraham updated this revision to Diff 42152.
ngraham edited the test plan for this revision.
ngraham added a comment.


  Remove parent directory check entirely; it's not necessary

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15583?vs=42018=42152

BRANCH
  arcpatch-D15583

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

AFFECTED FILES
  src/tools/balooctl/configcommand.cpp

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

D15583: [Balooctl] fix directory parent check

[Nathaniel Graham]

ngraham added a comment.


  Oh wow, you're right. Let me fix `balooctl` to support that, then.

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Stefan Brüns]

bruns added a comment.


  In D15583#330119 , @ngraham wrote:
  
  > In D15583#330085 , @bruns wrote:
  >
  > > I think the whole `startsWith` is flawed - it should be possible to have 
a e.g. "/home/user/foo/bar" include when "/home/user/foo" has been excluded.
  >
  >
  > Hmm, I'm not sure how much that matches the user expectation. If a folder 
is explicitly marked as excluded, I think it's most commonly understood that 
all its sub-folders would also be excluded.
  >
  > I could see the case for allowing this behavior to be explicitly overridden 
by an advanced user who marks `~/foo/` as excluded and then later marks 
`~foo/bar/` as included, but that would be material for another patch I think.
  
  
  If I read the unit tests correctly 
(https://phabricator.kde.org/source/baloo/browse/master/autotests/unit/file/fileindexerconfigtest.cpp),
 this is already supported. You just can not do it via balooctl.

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Nathaniel Graham]

ngraham added a comment.


  In D15583#330085 , @bruns wrote:
  
  > I think the whole `startsWith` is flawed - it should be possible to have a 
e.g. "/home/user/foo/bar" include when "/home/user/foo" has been excluded.
  
  
  Hmm, I'm not sure how much that matches the user expectation. If a folder is 
explicitly marked as excluded, I think it's most commonly understood that all 
its sub-folders would also be excluded.
  
  I could see the case for allowing this behavior to be explicitly overridden 
by an advanced user who marks `~/foo/` as excluded and then later marks 
`~foo/bar/` as included, but that would be material for another patch I think.

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Stefan Brüns]

bruns added a comment.


  I think the whole `startsWith` is flawed - it should be possible to have a 
e.g. "/home/user/foo/bar" include when "/home/user/foo" has been excluded.

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Nathaniel Graham]

ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Nathaniel Graham]

ngraham updated this revision to Diff 42018.
ngraham added a comment.


  - Address review comments and do it better

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15583?vs=41873=42018

BRANCH
  fix-hyphen-in-name (branched from master)

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

AFFECTED FILES
  src/tools/balooctl/configcommand.cpp

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

D15583: [Balooctl] fix directory parent check

[Nathaniel Graham]

ngraham planned changes to this revision.
ngraham added a comment.


  Actually it looks like we shouldn't even use it anyway for other reasons per 
the documentation: https://doc.qt.io/qt-5/qdir.html#separator
  
  They recommend always using `/` directly like you indicate, so I'll do that.

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Anthony Fieroni]

anthonyfieroni added a comment.


  In Qt 5.9 (if i remember correct) was introduced to not have trailing '/' so, 
QDir::separator should not be used, so better get folder as copy
  
for (QString folder : folders) {
if (!folder.endsWith(QLatin1Char('/')) {
folder += QLaitin1Char('/');
}
if (path.startsWith(folder)) {

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Christoph Feck]

cfeck resigned from this revision.

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Nathaniel Graham]

ngraham added reviewers: Frameworks, cfeck.

REPOSITORY
  R293 Baloo

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

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

D15583: [Balooctl] fix directory parent check

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Baloo, Dolphin, bruns.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
ngraham requested review of this revision.

REVISION SUMMARY
  [Submitting this patch on behalf of James Ausmus, who attached it to 
https://bugs.kde.org/show_bug.cgi?id=396535 and did not respond to a request to 
submit it here]
  
  balooctl add includeFolders/excludeFolders was checking for parent
  directories using a startsWith string match, so directories with the
  same initial character would match - for example:
  
$ balooctl add excludeFolders t
$ balooctl add excludeFolders test
Parent folder /test/kde/baloo/t is already in the list of exclude folders
  
  Fix this by appending a path separator to the path used for parent
  checking.
  
  BUG: 396535
  FIXED-IN: 5.51

TEST PLAN
  Executed the test plan from https://bugs.kde.org/show_bug.cgi?id=396535; it 
works now.

REPOSITORY
  R293 Baloo

BRANCH
  fix-hyphen-in-name (branched from master)

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

AFFECTED FILES
  src/tools/balooctl/configcommand.cpp

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