D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh added a task: T8250: Sanitize the database.

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, bruns, 
alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

This revision was automatically updated to reflect the committed changes.
michaelh marked 2 inline comments as done.
Closed by commit R293:b050c0ab0ef7: Introduce baloodb CLI tool (authored by 
michaelh).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11287?vs=29969=29976

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

AFFECTED FILES
  src/tools/CMakeLists.txt
  src/tools/baloodb/CMakeLists.txt
  src/tools/baloodb/Messages.sh
  src/tools/baloodb/main.cpp

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Laurent Montel]

mlaurent accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R293 Baloo

BRANCH
  sanitize-cmd (branched from master)

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Laurent Montel]

mlaurent added inline comments.

INLINE COMMENTS

> michaelh wrote in CMakeLists.txt:16
> @mlaurent  Is it worth the effort to change this for the other cli tools 
> accordingly?

It's the default variable.
But don't chnage it in this patch.
Perhaps in the future as you are the baloo maintainer :)

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh marked 7 inline comments as done.
michaelh added inline comments.

INLINE COMMENTS

> mlaurent wrote in CMakeLists.txt:16
> install(TARGETS baloodb ${KDE_INSTALL_TARGETS_DEFAULT_ARGS})

@mlaurent  Is it worth the effort to change this for the other cli tools 
accordingly?

> mlaurent wrote in main.cpp:151
> const QString allCommandStr

More const than suggested

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh updated this revision to Diff 29969.
michaelh added a comment.


  - Apply suggested changes

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11287?vs=29914=29969

BRANCH
  sanitize-cmd (branched from master)

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

AFFECTED FILES
  src/tools/CMakeLists.txt
  src/tools/baloodb/CMakeLists.txt
  src/tools/baloodb/Messages.sh
  src/tools/baloodb/main.cpp

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Laurent Montel]

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

INLINE COMMENTS

> CMakeLists.txt:16
> +
> +install(TARGETS baloodb DESTINATION ${BIN_INSTALL_DIR})

install(TARGETS baloodb ${KDE_INSTALL_TARGETS_DEFAULT_ARGS})

> main.cpp:151
> +}
> +QString allCommandsStr = allowedcommands.join(QStringLiteral("\n"));
> +return i18n("\n\nCommands:\n%1", allCommandsStr);

const QString allCommandStr

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh added inline comments.

INLINE COMMENTS

> ngraham wrote in main.cpp:65
> Any reason not to use a std::vector instead?

No reason. Will change to std::vector.

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> michaelh wrote in main.cpp:161
> To point this out. I'm **not** the maintainer of baloo. Maybe better remove 
> this?

I think you sort of are Baloo's de facto maintainer (which is a good thing, and 
you've earned the position)!

> main.cpp:65
> +
> +const auto commands = QVector{
> +Command{

Any reason not to use a std::vector instead?

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh marked 4 inline comments as done.

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh added inline comments.

INLINE COMMENTS

> mlaurent wrote in main.cpp:135
> very hard to read it.
> Could you split in several QString so it will more easy to check it's if ok :)

I had trouble myself :)

> main.cpp:161
> + i18n("(c) 2018, Michael Heidelbach"));
> +aboutData.addAuthor(i18n("Michael Heidelbach"), i18n("Maintainer"), 
> QStringLiteral("ottw...@gmail.com"));
> +KAboutData::setApplicationData(aboutData);

To point this out. I'm **not** the maintainer of baloo. Maybe better remove 
this?

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh updated this revision to Diff 29914.
michaelh added a comment.


  - Improve readability and apply other suggested changes

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11287?vs=29694=29914

BRANCH
  sanitize-cmd (branched from master)

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

AFFECTED FILES
  src/tools/CMakeLists.txt
  src/tools/baloodb/CMakeLists.txt
  src/tools/baloodb/Messages.sh
  src/tools/baloodb/main.cpp

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Laurent Montel]

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

INLINE COMMENTS

> main.cpp:103
> +QStringLiteral("devices"), 
> +i18n("List devices "),
> +QStringList{},

extra space at the end of i18n

> main.cpp:112
> +QStringList names;
> +for (const auto c : commands) {
> +names.append(c.name);

const auto  ?

> main.cpp:124
> +}
> +return QStringList() ;
> +}

coding style: remove space after()

> main.cpp:131
> +auto options = getOptions(c.name);
> +QString optionStr = options.count() == 0
> +? QString()

options.count() == 0 => isEmpty()

> main.cpp:135
> +
> +auto str = QStringLiteral("%1 %2")
> +.arg(QStringLiteral("%1%2%3")

very hard to read it.
Could you split in several QString so it will more easy to check it's if ok :)

> main.cpp:203
> +const bool missingOnly = parser.isSet(QStringLiteral("missing-only"));
> +const QString pattern = args.size() > 0
> +? args.at(0)

!isEmpty()

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks, mlaurent
Cc: mlaurent, ngraham, ashaposhnikov, michaelh, astippich, spoorun, 
nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh added a dependent revision: D11452: sanitizer: Improve device listing.

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks
Cc: ngraham, ashaposhnikov, michaelh, astippich, spoorun, nicolasfella, 
alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh added a comment.


  Should there be a note in `--help` message like "This is an experimental 
tool. The command like interface is subject to change."?

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks
Cc: ngraham, ashaposhnikov, michaelh, astippich, spoorun, nicolasfella, 
alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh edited the test plan for this revision.

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks
Cc: ngraham, ashaposhnikov, michaelh, astippich, spoorun, nicolasfella, 
alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh updated this revision to Diff 29694.
michaelh added a comment.


  - Rebase on master
  - Improve help message

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11287?vs=29525=29694

BRANCH
  sanitize-cmd (branched from master)

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

AFFECTED FILES
  src/tools/CMakeLists.txt
  src/tools/baloodb/CMakeLists.txt
  src/tools/baloodb/Messages.sh
  src/tools/baloodb/main.cpp

To: michaelh, #baloo, #frameworks
Cc: ngraham, ashaposhnikov, michaelh, astippich, spoorun, nicolasfella, 
alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh added a comment.


  In D11287#225860 , @ngraham wrote:
  
  > This is really nice, I like it! Should make debugging a lot easier. Noticed 
a couple of things:
  
  
  This is how the output now does and should look.
  
$ baloodb devices
Listing database contents...
Device:22 3 items 
Device:43 76 items
Device:42 3627 items
Device:41 40 items
Device:2053 6438 items
Device:2049 22 items
Device:2069 1533 items
Device:2066 11643 items
Device:2064 99 items
Device:2086 220 items
Device:2098 1450 items
Found 25151 matching items
Elapsed: 4.42067 secs
  
  or
  
$ baloodb devices 2>/dev/null
Device:22 3 items
Device:41 40 items
Device:42 3627 items
Device:43 76 items
Device:2069 1533 items
Device:2064 99 items
Device:2066 11643 items
Device:2053 6438 items
Device:2049 22 items
Device:2098 1450 items
Device:2086 220 items
  
  Are there more things?

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks
Cc: ngraham, ashaposhnikov, michaelh, spoorun, nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh updated this revision to Diff 29525.
michaelh added a comment.


  - Remove obsolete include

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11287?vs=29524=29525

BRANCH
  sanitize-cmd (branched from master)

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

AFFECTED FILES
  src/tools/CMakeLists.txt
  src/tools/baloodb/CMakeLists.txt
  src/tools/baloodb/Messages.sh
  src/tools/baloodb/main.cpp

To: michaelh, #baloo, #frameworks
Cc: ngraham, ashaposhnikov, michaelh, spoorun, nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh updated this revision to Diff 29524.
michaelh edited the test plan for this revision.
michaelh added a comment.


  - Add KAboutData
  - Remove debug statement

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11287?vs=29406=29524

BRANCH
  sanitize-cmd (branched from master)

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

AFFECTED FILES
  src/tools/CMakeLists.txt
  src/tools/baloodb/CMakeLists.txt
  src/tools/baloodb/Messages.sh
  src/tools/baloodb/main.cpp

To: michaelh, #baloo, #frameworks
Cc: ngraham, ashaposhnikov, michaelh, spoorun, nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Nathaniel Graham]

ngraham added a comment.


  This is really nice, I like it! Should make debugging a lot easier. Noticed a 
couple of things:
  
$ baloodb -v
baloodb

$ baloodb devices
command "devices"
Listing database contents...
Device:2049 14855 items
Found 14855 matching items
Elapsed: 0.326553 secs

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks
Cc: ngraham, ashaposhnikov, michaelh, spoorun, nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh created this revision.
michaelh added reviewers: Baloo, Frameworks.
Restricted Application added projects: Frameworks, Baloo.
michaelh requested review of this revision.

REVISION SUMMARY
  This is the command line interface of database sanitizer

TEST PLAN
  Run on command line
  Example:
  
$ baloodb list --missing-only --device-id 2049 '/otto/'
command "list"
Listing database contents...
Missing: device: 2049 inode: 5053 url: /tmp/otto/A
Missing: device: 2049 inode: 9441 url: /tmp/otto/B
Missing: device: 2049 inode: 9464 url: /tmp/otto/B/B.txt
Found 3 matching items
Elapsed: 1.50824 secs

REPOSITORY
  R293 Baloo

BRANCH
  sanitize-cmd (branched from master)

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

AFFECTED FILES
  src/tools/CMakeLists.txt
  src/tools/baloodb/CMakeLists.txt
  src/tools/baloodb/Messages.sh
  src/tools/baloodb/main.cpp

To: michaelh, #baloo, #frameworks
Cc: ashaposhnikov, michaelh, spoorun, nicolasfella, alexeymin

D11287: Introduce baloodb CLI tool

[Michael Heidelbach]

michaelh added a dependency: D11285: Introduce sanitizer class.

REPOSITORY
  R293 Baloo

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

To: michaelh, #baloo, #frameworks
Cc: ashaposhnikov, michaelh, spoorun, nicolasfella, alexeymin