Differential D11753 Diff 32105 src/engine/experimental/databasesanitizer.cpp

Changeset View

Standalone View

View Options

src/engine/experimental/databasesanitizer.cpp

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Line(s)
43 43​
44public: 44​public:
45 /** 45​ /**
46 * \brief Basic info about database items 46​ * \brief Basic info about database items
47 */ 47​ */
48 struct FileInfo { 48​ struct FileInfo {
49 quint32 deviceId = 0; 49​ quint32 deviceId = 0;
50 quint32 inode = 0; 50​ quint32 inode = 0;
51 QString url = QString(); 51​ quint64 id = 0;
52​ bool isSymLink = false;
52 bool accessible = true; 53​ bool accessible = true;
54​ QString url = QString();
53 }; 55​ };
54 56​
55 void printProgress(QTextStream& out, uint& cur, const uint max, const uint step) const 57​ void printProgress(QTextStream& out, uint& cur, const uint max, const uint step) const
56 { 58​ {
57 if (cur % step == 0) { 59​ if (cur % step == 0) {
58 out << QStringLiteral("%1%2\r").arg(100 * cur / max, 6).arg("%", -16); 60​ out << QStringLiteral("%1%2\r").arg(100 * cur / max, 6).arg("%", -16);
59 out.flush(); 61​ out.flush();
60 } 62​ }
▲ Show 20 Lines • Show All 62 Lines • ▼ Show 20 Line(s)117 for (auto it = map.constBegin(), end = map.constEnd(); it != end; it++) {
123 } else if (urlFilter && !urlFilter->match(it.value()).hasMatch()) { 125​ } else if (urlFilter && !urlFilter->match(it.value()).hasMatch()) {
124 continue; 126​ continue;
125 } 127​ }
126 128​
127 FileInfo info; 129​ FileInfo info;
128 info.deviceId = deviceId; 130​ info.deviceId = deviceId;
129 info.inode = idToInode(id); 131​ info.inode = idToInode(id);
130 info.url = QFile::decodeName(it.value()); 132​ info.url = QFile::decodeName(it.value());
131 info.accessible = !info.url.isEmpty() && QFileInfo::exists(info.url); 133​ info.id = id;
134​ QFileInfo fileInfo(info.url);
135​ info.accessible = !info.url.isEmpty() && fileInfo.exists();
132 136​
133 if (info.accessible && (accessFilter & DatabaseSanitizer::IgnoreAvailable)) { 137​ if (info.accessible && (accessFilter & DatabaseSanitizer::IgnoreAvailable)) {
134 continue; 138​ continue;
135 } else if (!info.accessible && (accessFilter & DatabaseSanitizer::IgnoreUnavailable)) { 139​ } else if (!info.accessible && (accessFilter & DatabaseSanitizer::IgnoreUnavailable)) {
136 continue; 140​ continue;
137 } 141​ }
138 142​
143​ info.isSymLink = fileInfo.isSymLink();
144​
139 result.append(info); 145​ result.append(info);
140 summary.ignored--; 146​ summary.ignored--;
141 if (info.accessible) { 147​ if (info.accessible) {
142 summary.accessible++; 148​ summary.accessible++;
143 } 149​ }
144 } 150​ }
145 return {result, summary}; 151​ return {result, summary};
146 } 152​ }
Show All 11 Lines155 static QMap<quint32, QStorageInfo> storageInfos = []() {
158 } 164​ }
159 return result; 165​ return result;
160 }(); 166​ }();
161 167​
162 QStorageInfo info = storageInfos.value(id); 168​ QStorageInfo info = storageInfos.value(id);
163 return info; 169​ return info;
164 } 170​ }
165 171​
166private: 172​ struct IdInfo {
brunsUnsubmitted
Done

Why do you remove the private: here?

bruns: Why do you remove the private: here?
michaelhAuthorUnsubmitted
Done

340: m_pimpl->m_transaction->removeDocument(info.id);
and more

michaelh: 340: m_pimpl->m_transaction->removeDocument(info.id); and more
173​ quint32 deviceId = 0;
brunsUnsubmitted
Not Done

Pass in a List/Set of deviceIds, and return the same type.

bruns: Pass in a List/Set of deviceIds, and return the same type.
michaelhAuthorUnsubmitted
Not Done

Your suggestion in D12222 can be reused. I'll do it there.

michaelh: Your suggestion in D12222 can be reused. I'll do it there.
174​ quint32 inode = 0;
175​ quint64 id = 0;
176​ };
brunsUnsubmitted
Done

Is this struct still used?

bruns: Is this struct still used?
177​ IdInfo toIdInfo(quint64 id) {
178​ IdInfo result;
179​ const quint32* arr = reinterpret_cast<quint32*>(&id);
180​ result.deviceId = arr[0];
181​ result.inode = arr[1];
182​ result.id = id;
183​ return result;
184​ }
brunsUnsubmitted
Done

duplicates code from idutils.h

bruns: duplicates code from idutils.h
185​
167 Transaction* m_transaction; 186​ Transaction* m_transaction;
168}; 187​};
brunsUnsubmitted
Done

Are you planning to use this one anywhere else?

Otherwise, inside deviceFilters(...):

auto isIgnored = [&accessFilter](const QStorageInfo& storageInfo) {
    if (storageInfo.isValid() && ...) {
        ...
    }
}
bruns: Are you planning to use this one anywhere else? Otherwise, inside `deviceFilters(...)`: ```…
michaelhAuthorUnsubmitted
Not Done

Are you planning to use this one anywhere else?

Yes. For all output: D12222

michaelh: > Are you planning to use this one anywhere else? Yes. For all output: D12222
169} 188​}
170 189​
brunsUnsubmitted
Done

bool mounted = storageInfo.isValid()

bruns: `bool mounted = storageInfo.isValid()`
171using namespace Baloo; 190​using namespace Baloo;
172 191​
brunsUnsubmitted
Done

This else is ok, although not necessary, it emphasizes symmetry (mounted, not mounted)

bruns: This `else` is ok, although not necessary, it emphasizes symmetry (mounted, not mounted)
173 192​
174DatabaseSanitizer::DatabaseSanitizer(const Database& db, Baloo::Transaction::TransactionType type) 193​DatabaseSanitizer::DatabaseSanitizer(const Database& db, Baloo::Transaction::TransactionType type)
brunsUnsubmitted
Done

Remove the else, and add a newline here

    ...
}

if (storageInfo.fileSystemType() == ...) {
    ...
    return true;
}

return false;
bruns: Remove the `else`, and add a newline here ``` ... } if (storageInfo.fileSystemType() == ...
175 : m_pimpl(new DatabaseSanitizerImpl(db, type)) 194​ : m_pimpl(new DatabaseSanitizerImpl(db, type))
176{ 195​{
177} 196​}
178 197​
179DatabaseSanitizer::DatabaseSanitizer(Database* db, Transaction::TransactionType type) 198​DatabaseSanitizer::DatabaseSanitizer(Database* db, Transaction::TransactionType type)
180 : DatabaseSanitizer(*db, type) 199​ : DatabaseSanitizer(*db, type)
181{ 200​{
182} 201​}
183 202​
184DatabaseSanitizer::~DatabaseSanitizer() 203​DatabaseSanitizer::~DatabaseSanitizer()
185{ 204​{
186 delete m_pimpl; 205​ delete m_pimpl;
187 m_pimpl = nullptr; 206​ m_pimpl = nullptr;
188} 207​}
189 208​
190/** 209​/**
191* Create a list of \a FileInfo items and print it to stdout. 210​* Create a list of \a FileInfo items and print it to stdout.
brunsUnsubmitted
Done

->commit()

bruns: ->commit()
192* 211​*
193* \p deviceIDs filter by device ids. If the vector is empty no filtering is done 212​* \p deviceIDs filter by device ids. If the vector is empty no filtering is done
194* and everything is printed. 213​* and everything is printed.
195* Positive numbers are including filters printing only the mentioned device ids. 214​* Positive numbers are including filters printing only the mentioned device ids.
196* Negative numbers are excluding filters printing everything but the mentioned device ids. 215​* Negative numbers are excluding filters printing everything but the mentioned device ids.
197* 216​*
198* \p missingOnly Simulate purging operation. Only inaccessible items are printed. 217​* \p missingOnly Simulate purging operation. Only inaccessible items are printed.
199* 218​*
▲ Show 20 Lines • Show All 74 Lines • ▼ Show 20 Line(s)266 for (auto it = useCount.cbegin(); it != useCount.cend(); it++) {
274 } 293​ }
275 // TODO: see above 294​ // TODO: see above
276 // out << QStringLiteral("\033[0m") << endl; 295​ // out << QStringLiteral("\033[0m") << endl;
277 out << endl; 296​ out << endl;
278 } 297​ }
279 298​
280 err << i18n("Found %1 matching in %2 devices", matchCount, useCount.size()) << endl; 299​ err << i18n("Found %1 matching in %2 devices", matchCount, useCount.size()) << endl;
281} 300​}
301​
302​void DatabaseSanitizer::removeStaleEntries(const QVector<qint64>& deviceIds,
303​ const DatabaseSanitizer::ItemAccessFilters accessFilter,
304​ const bool dryRun,
305​ const QSharedPointer<QRegularExpression>& urlFilter)
306​{
307​ auto listResult = m_pimpl->createList(deviceIds, IgnoreAvailable, urlFilter);
308​
309​ // Cache mounted info. Flags:
310​ // 0 = uninitialized
311​ // -1 = ignore
312​ // 1 = use
313​ const QMap<quint32, int> deviceIdFilter = [&]() {
314​ QMap<quint32, int> result;
315​ for (const auto& info : listResult.first) {
316​ if (result[info.deviceId] == 0) {
317​ const auto storageInfo = m_pimpl->getStorageInfo(info.deviceId);
318​ if (storageInfo.isValid() && (accessFilter & IgnoreMounted)) {
319​ result[info.deviceId] = -1;
320​ } else if (!storageInfo.isValid() && (accessFilter & IgnoreUnmounted)) {
321​ result[info.deviceId] = -1;
322​ } else {
323​ result[info.deviceId] = 1;
brunsUnsubmitted
Done

do the calls to m_transaction using a wrapper, e.g. DatabaseSanitizer::removeDocument(info.id)

bruns: do the calls to m_transaction using a wrapper, e.g. `DatabaseSanitizer::removeDocument(info.id)`
michaelhAuthorUnsubmitted
Done

I'm assuming you meant DatabaseSanitizerImpl::removeDocument(info.id). If you meant DatabaseSanitizer::removeDocument, please explain what the advantage is.

michaelh: I'm assuming you meant `DatabaseSanitizerImpl::removeDocument(info.id)`. If you meant…
brunsUnsubmitted
Done

Right.

bruns: Right.
324​ }
325​ }
326​ }
brunsUnsubmitted
Done

Restructure:
Iterate over listResult.first to populate with deviceIds
Iterate over result to set each entry to ignore/use.

bruns: Restructure: Iterate over listResult.first to populate with deviceIds Iterate over result to…
327​ return result;
328​ }();
329​
330​ const auto sep = QLatin1Char(' ');
331​ auto& summary = listResult.second;
332​ QTextStream out(stdout);
333​ QTextStream err(stderr);
brunsUnsubmitted
Done

dito

bruns: dito
334​ for (const auto& info: listResult.first) {
335​ if (deviceIdFilter[info.deviceId] == 1) {
336​ if (info.isSymLink) {
337​ out << i18n("IgnoredSymbolicLink:");
338​ summary.ignored++;
339​ } else {
brunsUnsubmitted
Done

Nitpick - one more space (align " and summary)

bruns: Nitpick - one more space (align " and __s__ummary)
340​ m_pimpl->m_transaction->removeDocument(info.id);
341​ out << i18n("Removing:");
342​ }
343​ out << sep << QStringLiteral("device: %1").arg(info.deviceId)
brunsUnsubmitted
Not Done

remove sep, use `QStringLiteral(" device: %1") instead

bruns: remove `sep`, use `QStringLiteral(" device: %1") instead
michaelhAuthorUnsubmitted
Not Done

I'd like to keep it this way.

  1. For consistency with printList and printDevices
  2. I'm thinking in the opposite direction: Make sep global to easily change it to another value for all output or even add a field-separator getter/setter ('--separator') should the need arise. (in a separate patch, of course)
michaelh: I'd like to keep it this way. # For consistency with `printList` and `printDevices` # I'm…
brunsUnsubmitted
Not Done

My preference is:

  • Use one fixed format string for human readable output, which is translatable
  • When the need arises to e.g. have something machine readable, use a different format string.
bruns: My preference is: - Use **one** fixed format string for human readable output, which is…
344​ << sep << QStringLiteral("inode: %1").arg(info.inode)
brunsUnsubmitted
Done

dito sep, indent to align << with line above

bruns: dito `sep`, indent to align `<<` with line above
345​ << sep << QStringLiteral("url: %1").arg(info.url)
346​ << endl;
347​ } else {
348​ summary.ignored++;
349​ }
350​ }
351​ if (dryRun) {
352​ m_pimpl->m_transaction->abort();
353​ } else {
354​ m_pimpl->m_transaction->commit();
355​ }
356​ Q_ASSERT(summary.accessible == 0);
357​ err << i18nc("numbers", "Removed: %1, Total: %2, Ignored: %3",
358​ summary.total - summary.ignored,
359​ summary.total,
360​ summary.ignored
361​ ) << endl;
362​}