D12222: baloodb: Use complete access filtering for all outputs
astippich added a comment. Herald added a subscriber: kde-frameworks-devel. @bruns this still applies cleanly to current master, do you think this and the dependent revision are still worth it to merge? REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: kde-frameworks-devel, astippich, bruns, gennad, domson, ashaposhnikov, michaelh, spoorun, ngraham, abrahams
D12222: baloodb: Use complete access filtering for all outputs
michaelh marked an inline comment as done. michaelh added a comment. I'm sorry for making so much noise with all those stupid mistakes. It's probably best for me to let this rest for a few days, relax a little and gain some distance. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
michaelh updated this revision to Diff 32580. michaelh added a comment. - That was no typo REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1?vs=32579=32580 BRANCH sanitize-dry (branched from master) REVISION DETAIL https://phabricator.kde.org/D1 AFFECTED FILES src/engine/experimental/databasesanitizer.cpp src/engine/experimental/databasesanitizer.h src/tools/experimental/baloodb/main.cpp To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
michaelh updated this revision to Diff 32579. michaelh added a comment. - Correct typo REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1?vs=32578=32579 BRANCH sanitize-dry (branched from master) REVISION DETAIL https://phabricator.kde.org/D1 AFFECTED FILES src/engine/experimental/databasesanitizer.cpp src/engine/experimental/databasesanitizer.h src/tools/experimental/baloodb/main.cpp To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
michaelh updated this revision to Diff 32578. michaelh marked 5 inline comments as done. michaelh added a comment. - Revert most previous changes - Use `isMounted()` and `isObscured()` - Remove documents recursively REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1?vs=32426=32578 BRANCH sanitize-dry (branched from master) REVISION DETAIL https://phabricator.kde.org/D1 AFFECTED FILES src/engine/experimental/databasesanitizer.cpp src/engine/experimental/databasesanitizer.h src/tools/experimental/baloodb/main.cpp To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
bruns added inline comments.
INLINE COMMENTS
> databasesanitizer.cpp:179
> +
> +bool isMounted(const quint32 id) {
> +return m_devices.mounted.contains(id);
`isMounted(id)` can be implemented as `getStorageInfo(id).isValid()`
> databasesanitizer.cpp:183
> +
> +bool isUnMounted(const quint32 id) {
> +return m_devices.unmounted.contains(id);
unused, please remove
> databasesanitizer.cpp:189
> {
> -QMap result;
> +createDeviceStates(infos, accessFilter);
> +auto tail = infos.end();
I can not see what this function is useful for. Just inline the FileInfo ->
DeviceIds reduction here, and ...
> databasesanitizer.cpp:192
> +Summary summary;
> +for (const quint32 deviceId : m_devices.ignored) {
> +tail = std::remove_if(infos.begin(), tail,
... skip the deviceId if it should be ignored:
const auto storageInfo = getStorageInfo(id);
if (isDeviceIgnored(storageInfo, accessFilter)) {
continue;
}
> databasesanitizer.cpp:195
> + [deviceId, ] (const FileInfo& info) {
> + summary.accessible += (info.deviceId ==
> deviceId && info.accessible ? 1 : 0);
> + return info.deviceId == deviceId;
if (info.deviceId != deviceId) {
return false;
}
summary.accessible += info.accessible ? 1 : 0;
return true;
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D1
To: michaelh, #baloo, #frameworks
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
michaelh marked 6 inline comments as done. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
michaelh marked an inline comment as done. michaelh added inline comments. INLINE COMMENTS > databasesanitizer.cpp:196 > + summary.accessible += (info.deviceId == > deviceId && info.accessible ? 1 : 0); > + return info.deviceId == deviceId; > + }); ~3 hrs for this, phew. > michaelh wrote in databasesanitizer.cpp:160 > $ baloodb list --mounted-only Einstein > Listing database contents... > ! device: 43 inode: 319 url: /media/circulans/(tv-analog)/L-Z/Was Einstein > noch nicht wusste.mkv > + device: 46 inode: 319 url: /media/circulans/(doku)/Was Einstein noch > nicht wusste.mkv > > 43 is the correct id for a share. The path is wrong. > 46 is a wrong id for the same share. It should not be listed. The path > however is correct. > > `QStorageInfo` is not enough, I'm afraid. For proper matching mtab must be > read. On the wrong track REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
michaelh updated this revision to Diff 32426. michaelh added a comment. - Filter by device id REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1?vs=32422=32426 BRANCH sanitize-dry (branched from master) REVISION DETAIL https://phabricator.kde.org/D1 AFFECTED FILES src/engine/experimental/databasesanitizer.cpp src/engine/experimental/databasesanitizer.h src/tools/experimental/baloodb/main.cpp To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
michaelh added a comment. I went I little overboard with `remove_if` and had to repair. Device filtering is also broken see inline comment. INLINE COMMENTS > databasesanitizer.cpp:160 > const QByteArray rootPath = > QFile::encodeName(vol.rootPath()); > const auto fsinfo = filePathToStat(rootPath); > const quint32 id = static_cast(fsinfo.st_dev); $ baloodb list --mounted-only Einstein Listing database contents... ! device: 43 inode: 319 url: /media/circulans/(tv-analog)/L-Z/Was Einstein noch nicht wusste.mkv + device: 46 inode: 319 url: /media/circulans/(doku)/Was Einstein noch nicht wusste.mkv 43 is the correct id for a share. The path is wrong. 46 is a wrong id for the same share. It should not be listed. The path however is correct. `QStorageInfo` is not enough, I'm afraid. For proper matching mtab must be read. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
michaelh updated this revision to Diff 32422. michaelh added a comment. - Apply suggested changes - Correct summary error - Repair cleaning - Comment some decisions REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1?vs=32321=32422 BRANCH sanitize-dry (branched from master) REVISION DETAIL https://phabricator.kde.org/D1 AFFECTED FILES src/engine/experimental/databasesanitizer.cpp src/engine/experimental/databasesanitizer.h src/tools/experimental/baloodb/main.cpp To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun
D12222: baloodb: Use complete access filtering for all outputs
bruns added inline comments.
INLINE COMMENTS
> databasesanitizer.cpp:179
> +tail = std::remove_if(infos.begin(), tail,
> + [deviceId,] (const FileInfo& info) {
> + summary.accessible += info.accessible;
missing space
> databasesanitizer.cpp:180
> + [deviceId,] (const FileInfo& info) {
> + summary.accessible += info.accessible;
> + return info.id == deviceId;
bool is automatically cast to int, false -> 0, true -> 1.
Although probably better to do it explicitly:
`summary.accessible += info.accessible ? 1 : 0;`
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D1
To: michaelh, #baloo, #frameworks
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun, ngraham
D12222: baloodb: Use complete access filtering for all outputs
michaelh added inline comments.
INLINE COMMENTS
> databasesanitizer.cpp:180
> + [deviceId,] (const FileInfo& info) {
> + summary.accessible += info.accessible;
> + return info.id == deviceId;
How can this work? `quint64 + bool`
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D1
To: michaelh, #baloo, #frameworks
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun, ngraham
D12222: baloodb: Use complete access filtering for all outputs
michaelh updated this revision to Diff 32321. michaelh added a comment. - Use remove_if REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1?vs=32176=32321 BRANCH sanitize-dry (branched from master) REVISION DETAIL https://phabricator.kde.org/D1 AFFECTED FILES src/engine/experimental/databasesanitizer.cpp src/tools/experimental/baloodb/main.cpp To: michaelh, #baloo, #frameworks Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun, ngraham
D12222: baloodb: Use complete access filtering for all outputs
michaelh added inline comments.
INLINE COMMENTS
> bruns wrote in databasesanitizer.cpp:252
> If ignoredDevices is a Set/List, you can do a filter pass over the fileList
> first.
>
> auto& fileList = listResult.first;
> auto tail = fileList.end();
> for (auto deviceId : ignoredDevices) {
> tail = std::remove_if(fileList.begin(), tail,
> [deviceId] (const FileInfo& info) {
> return info.id == deviceId;
> });
> }
> summary.ignored += fileList.end() - tail;
> std::erase(tail, fileList.end());
Cool!
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D1
To: michaelh, #baloo, #frameworks
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun, ngraham
D12222: baloodb: Use complete access filtering for all outputs
bruns added inline comments.
INLINE COMMENTS
> databasesanitizer.cpp:252
> +auto& summary = listResult.second;
> for (const auto& info: listResult.first) {
> +if (ignoredDevices[info.id] == false) {
If ignoredDevices is a Set/List, you can do a filter pass over the fileList
first.
auto& fileList = listResult.first;
auto tail = fileList.end();
for (auto deviceId : ignoredDevices) {
tail = std::remove_if(fileList.begin(), tail,
[deviceId] (const FileInfo& info) {
return info.id == deviceId;
});
}
summary.ignored += fileList.end() - tail;
std::erase(tail, fileList.end());
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D1
To: michaelh, #baloo, #frameworks
Cc: bruns, ashaposhnikov, michaelh, astippich, spoorun, ngraham
D12222: baloodb: Use complete access filtering for all outputs
michaelh added a dependent revision: D12044: baloodb: Improve interface. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, alexeymin
D12222: baloodb: Use complete access filtering for all outputs
michaelh added inline comments.
INLINE COMMENTS
> main.cpp:161
> return i18n("\n\nCommands:\n%1", allCommandsStr);
> }
>
Ideas for better formatting this are welcome.
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D1
To: michaelh, #baloo, #frameworks
Cc: ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, alexeymin
D12222: baloodb: Use complete access filtering for all outputs
michaelh added a task: T8250: Sanitize the database. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, alexeymin
D12222: baloodb: Use complete access filtering for all outputs
michaelh edited the summary of this revision. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, alexeymin
D12222: baloodb: Use complete access filtering for all outputs
michaelh added a dependency: D11753: baloodb: Add clean command. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D1 To: michaelh, #baloo, #frameworks Cc: ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, alexeymin
D12222: baloodb: Use complete access filtering for all outputs
michaelh created this revision. michaelh added reviewers: Baloo, Frameworks. Restricted Application added projects: Frameworks, Baloo. michaelh requested review of this revision. REVISION SUMMARY Add `mounted-only` option to 'list' and 'devices' command Use a common function to determine if a device should be ignored REPOSITORY R293 Baloo BRANCH sanitize-dry (branched from master) REVISION DETAIL https://phabricator.kde.org/D1 AFFECTED FILES src/engine/experimental/databasesanitizer.cpp src/tools/experimental/baloodb/main.cpp To: michaelh, #baloo, #frameworks Cc: ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, alexeymin
