Details
- Reviewers
bruns - Group Reviewers
Baloo Frameworks - Maniphest Tasks
- T8250: Sanitize the database
- Commits
- R293:675d989823ec: baloodb: Add clean command
$ baloodb clean --mounted-only Removing stale database contents... Ignoring device 2086 Ignoring device 2098 Ignoring device 2069 Ignoring device 2064 Ignoring device 2066 Database has corrupted entries baloo may misbehave, please recreate the DB by running $ balooctl disable && balooctl enable Removing: device: 2049 inode: 4861 url: /tmp/otto ... Removing: device: 2053 inode: 39208001 url: //Little Witch Academia - 08.mkv Removing: device: 2053 inode: 39215245 url: Removed 14965 items Elapsed: 4.6921 secs
Second run:
$ baloodb clean --mounted-only Removing stale database contents... access filter QFlags(0x4) Ignoring device 2086 Ignoring device 2098 Ignoring device 2066 Ignoring device 2064 Ignoring device 2069 Removing: device: 22 inode: 225383 url: Removing: device: 2053 inode: 34612126 url: Removed 14943 items Elapsed: 3.47939 secs
Diff Detail
- Repository
- R293 Baloo
- Branch
- sanitize-clean (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
src/engine/databasesanitizer.cpp | ||
---|---|---|
113 | I think if you initialize fileinfo anyway, you should use fileinfo.exists() ... | |
316 | I think it is better to use | |
317 | missing check id != 0, or the equivalent if the above code is changed to QT_FSTAT | |
333 | why not just make the removeDocument transaction above depend on dryRun? removeDocument is expensive ... | |
src/tools/baloodb/main.cpp | ||
251 | make it depend on dry-run? |
src/engine/databasesanitizer.cpp | ||
---|---|---|
333 | I did that before, but decided on --dry-run to simulate the process as close as possible. | |
src/tools/baloodb/main.cpp | ||
251 | Transaction complains (Q_ASSERT) when trying to remove a document on a readonly db or readonly transaction. |
src/engine/databasesanitizer.cpp | ||
---|---|---|
316 | I've tried it. Sadly your suggestion does not work. With fi = filePathToStat(info.symlink.toLocal8Bit()) fi._st_dev is == 0 when the symlink target does not exist. Hence it does tell me why the link can't be followed. baloo's db otoh knows about this. As deviceIdFilter does never contain 0 symlinks would be removed when they should be ignored. With If(id != 0) truly "dead" symlinks will not be removed. filePathToStat returns QT_STATBUF. My guess is that is essentially the same as QT_FSTAT, at least man fstat.2 says so. |
src/engine/databasesanitizer.cpp | ||
---|---|---|
316 | What do you consider a "truly dead" symlink? If you really want to do it correctly, you have to walk the file system yourself, one symlink target path component at a time - each path component can be a symlink itself, or a mount point. You should check the device id for *every* path component. |
src/engine/databasesanitizer.cpp | ||
---|---|---|
316 | Damn, you're right! I was fooled by this: $ balooctl index /mnt/otto/test.mp4 $ ln -s /mnt/otto/test.mp4 ~/Videos/ baloo now also indexes ~/Videos/test.mp4 I did not notice because every indexed symbolic link on my system also has its target indexed. I never saw id == 0 |
- Ignore symbolic links
There seems to be no reliable way to map a symbolic link's target
to a device id. Instead of removing it print a parseable message
so users can remove it manually.
src/engine/databasesanitizer.cpp | ||
---|---|---|
333 | Fair reason, the comment below is also mood then ... |
src/engine/databasesanitizer.cpp | ||
---|---|---|
333 | 8 meanings of MOOD acronym or abbreviation. Magic of Ordinary Days Meet only Original Designs Michigan Out of Doors Movie Organizer Online Database Miata Owners on Delmarva Methodology for Object Oriented Design Metrics for Object Oriented Design Museum of Outstanding Design typo? :-) |
src/engine/databasesanitizer.cpp | ||
---|---|---|
333 | s/mood/moot/ |
src/engine/experimental/databasesanitizer.cpp | ||
---|---|---|
166 ↗ | (On Diff #32105) | Why do you remove the private: here? |
176 ↗ | (On Diff #32105) | Is this struct still used? |
184 ↗ | (On Diff #32105) | duplicates code from idutils.h |
326 ↗ | (On Diff #32105) | Restructure: |
343 ↗ | (On Diff #32105) | remove sep, use `QStringLiteral(" device: %1") instead |
344 ↗ | (On Diff #32105) | dito sep, indent to align << with line above |
src/engine/experimental/databasesanitizer.cpp | ||
---|---|---|
166 ↗ | (On Diff #32105) | 340: m_pimpl->m_transaction->removeDocument(info.id); |
343 ↗ | (On Diff #32105) | I'd like to keep it this way.
|
- Wrap transaction operations
src/engine/experimental/databasesanitizer.cpp | ||
---|---|---|
344 ↗ | (On Diff #32201) | I'm assuming you meant DatabaseSanitizerImpl::removeDocument(info.id). If you meant DatabaseSanitizer::removeDocument, please explain what the advantage is. |
src/engine/experimental/databasesanitizer.cpp | ||
---|---|---|
187 ↗ | (On Diff #32255) | Are you planning to use this one anywhere else? Otherwise, inside deviceFilters(...): auto isIgnored = [&accessFilter](const QStorageInfo& storageInfo) { if (storageInfo.isValid() && ...) { ... } } |
189 ↗ | (On Diff #32255) | bool mounted = storageInfo.isValid() |
191 ↗ | (On Diff #32255) | This else is ok, although not necessary, it emphasizes symmetry (mounted, not mounted) |
193 ↗ | (On Diff #32255) | Remove the else, and add a newline here ... } if (storageInfo.fileSystemType() == ...) { ... return true; } return false; |
210 ↗ | (On Diff #32255) | ->commit() |
373 ↗ | (On Diff #32255) | Nitpick - one more space (align " and summary) |
344 ↗ | (On Diff #32201) | Right. |
src/engine/experimental/databasesanitizer.cpp | ||
---|---|---|
173 ↗ | (On Diff #32255) | Pass in a List/Set of deviceIds, and return the same type. |
343 ↗ | (On Diff #32105) | My preference is:
|