bruns added inline comments.
INLINE COMMENTS
> databasesanitizer.cpp:113
> info.url = url;
> - info.accessible = !url.isEmpty() && QFileInfo::exists(url);
> + info.accessible = !url.isEmpty() && fileInfo.exists(url);
> + info.symlink = fileInfo.isSymLink()
I think if you initialize fileinfo anyway, you should use fileinfo.exists() ...
> databasesanitizer.cpp:316
> + if (!info.symlink.isEmpty()) {
> + const auto id =
> m_pimpl->m_transaction->documentId(info.symlink.toLocal8Bit());
> + const auto idInfo = m_pimpl->toIdInfo(id);
I think it is better to use
QT_FSTAT(info.symlink.toLocal8Bit().constData(), ...) here, avoids lots of
calls to the database, and guarantees more consistent results - symlinkTarget()
works on the filesystem, so should the lookup here
> databasesanitizer.cpp:317
> + const auto id =
> m_pimpl->m_transaction->documentId(info.symlink.toLocal8Bit());
> + const auto idInfo = m_pimpl->toIdInfo(id);
> + if (!deviceIdFilter.contains(idInfo.deviceId)) {
missing check id != 0, or the equivalent if the above code is changed to
QT_FSTAT
> databasesanitizer.cpp:333
> + if (dryRun) {
> + m_pimpl->m_transaction->abort();
> + } else {
why not just make the removeDocument transaction above depend on dryRun?
removeDocument is expensive ...
> main.cpp:251
> } else if (command == QStringLiteral("clean")) {
> - /* TODO: add prune command */
> - parser.showHelp(1);
> + auto dbMode = Database::ReadWriteDatabase;
> + if (!db->open(dbMode)) {
make it depend on dry-run?
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D11753
To: michaelh, #baloo, #frameworks
Cc: bruns, cfeck, smithjd, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, alexeymin
