D11038: balooctl: Add pruneDb option to remove stale file index entries.

[James Smith]

smithjd updated this revision to Diff 28987.
smithjd added a comment.


  - balooctl: Clarify the prune option description.

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11038?vs=28747=28987

BRANCH
  master-purgeDb (branched from master)

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

AFFECTED FILES
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/tools/balooctl/main.cpp

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

D11038: balooctl: Add pruneDb option to remove stale file index entries.

[James Smith]

smithjd added a comment.


  In D11038#220486 , @michaelh wrote:
  
  > In D11038#219353 , @smithjd 
wrote:
  >
  > > All mounts must be manually made available by the user before running 
this option, or all files on a previously available mount will be removed from 
the index.
  >
  >
  > It is much too easy for users to accidentally ruin their database with 
this. There should at least be a warning message including the advice to mount 
all external items. And users must confirm execution. 
  >  This command also should have a --dry-run option to show users what would 
happen to their database.
  
  
  Adding paths to index on top of the user's home directory requires manual 
modification of the config file, or symlinks into the user's home directory. It 
can be assumed that the user is reasonably competent about destructive options 
if he/she does this. Adding a --dry-run modifier is overkill for regeneratable 
data. Requiring that the user confirm execution is bad practice.
  
  I've changed the option description to more clearly convey that it will 
remove any entry that doesn't have a resolveable file path.

REPOSITORY
  R293 Baloo

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

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

D11038: balooctl: Add pruneDb option to remove stale file index entries.

[Michael Heidelbach]

michaelh requested changes to this revision.
michaelh added a comment.
This revision now requires changes to proceed.


  In D11038#219353 , @smithjd wrote:
  
  > All mounts must be manually made available by the user before running this 
option, or all files on a previously available mount will be removed from the 
index.
  
  
  It is much too easy for users to accidentally ruin their database with this. 
There should at least be a warning message including the advice to mount all 
external items. And users must confirm execution. 
  This command also should have a --dry-run option to show users what would 
happen to their database.

REPOSITORY
  R293 Baloo

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

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

D11038: balooctl: Add pruneDb option to remove stale file index entries.

[Michael Heidelbach]

michaelh added a reviewer: michaelh.

REPOSITORY
  R293 Baloo

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

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

D11038: balooctl: Add pruneDb option to remove stale file index entries.

[James Smith]

smithjd marked 7 inline comments as done.
smithjd added a comment.


  All mounts must be manually made available by the user before running this 
option, or all files on a previously available mount will be removed from the 
index.

INLINE COMMENTS

> michaelh wrote in transaction.cpp:271
> Can we or should we check the result of the operation here?

removeDocument() is void.

REPOSITORY
  R293 Baloo

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

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

D11038: balooctl: Add pruneDb option to remove stale file index entries.

[James Smith]

smithjd updated this revision to Diff 28747.
smithjd added a comment.


  Review changes.

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11038?vs=28656=28747

BRANCH
  master-purgeDb (branched from master)

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

AFFECTED FILES
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/tools/balooctl/main.cpp

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

D11038: balooctl: Add pruneDb option to remove stale file index entries.

[Michael Heidelbach]

michaelh added a comment.


  Baloo is in bad need for something like this. Unfortunately in cannot be done 
so easily.
  We have to account for indexed net shares and removable drives which are only 
temporarily unavailable. Files on those should not be removed. And most likely 
some user interaction is needed here.
  
  #Baloo  has it own Project page now. 
Please have a look and file your plans for Baloo as tasks there. Also I'm very 
much interested in your opinion about the tasks filed there specially about  
T8054  and .

INLINE COMMENTS

> transaction.cpp:264
> +QTextStream out(stdout);
> +out << "Total Document IDs: " << map.size() << endl;
> +

This should go to stderr so only pruned files go to stdout

> transaction.cpp:270
> +if (!QFileInfo::exists(url)) {
> +out << "Purging " << id << endl;
> +m_writeTrans->removeDocument(id);

Place this at the end of the loop and also print url, please. E.g. `out << 
"Removed" << id << ":" << url;` would be easily parseable. So one could catch 
stdout with

  balooctl pruneDb | tr '-d:' -f2 >removed_files.lst

> transaction.cpp:271
> +out << "Purging " << id << endl;
> +m_writeTrans->removeDocument(id);
> +count++;

Can we or should we check the result of the operation here?

> transaction.cpp:276
> +
> +out << "Removed Entries: " << count << " (" << count * 100.0 / 
> map.size() << "%)" << endl;
> +}

Same as above

> main.cpp:90
>  parser.addPositionalArgument(QStringLiteral("monitor"), i18n("Monitor 
> the file indexer"));
> +parser.addPositionalArgument(QStringLiteral("pruneDb"), i18n("Purge 
> invalid index entries"));
>  parser.addPositionalArgument(QStringLiteral("indexSize"), i18n("Display 
> the disk space used by index"));

Just 'prune'?

> main.cpp:326
>  
> +if (command == QStringLiteral("pruneDb")) {
> +Database *db = globalDatabaseInstance();

see above

> main.cpp:329
> +if (!db->open(Database::ReadOnlyDatabase)) {
> +out << "Baloo Index could not be opened\n";
> +return 1;

stderr

> main.cpp:334
> +Transaction tr(db, Transaction::ReadOnly);
> +out << "Checking file paths .. " << endl;
> +tr.pruneFsTree();

stderr

REPOSITORY
  R293 Baloo

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

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

D11038: balooctl: Add pruneDb option to remove stale file index entries.

[James Smith]

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

REPOSITORY
  R293 Baloo

BRANCH
  master-purgeDb (branched from master)

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

AFFECTED FILES
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/tools/balooctl/main.cpp

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