From bb5f17b876ca2c6febd37363bbadffd94682adbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 13 Oct 2026 08:36:08 +0200 Subject: [PATCH] [Installer][Client] Fix stale backup resurrection, batch session backups, extend coverage NSIS: clear stale %TEMP% backup and only arm restore when something was copied, directory-existence checks, back up loose .dec decks, drop downloaded pics but keep pics\CUSTOM. Helper: single session dir per run (ms timestamps), same-basename disambiguation, partial copies still report their location. --- cmake/NSIS.template.in | 36 ++++- cockatrice/src/interface/window_main.cpp | 37 +++-- .../utility/user_data_backup.cpp | 119 +++++++++++++--- .../libcockatrice/utility/user_data_backup.h | 32 ++++- tests/user_data_backup_test.cpp | 130 ++++++++++++++++++ 5 files changed, 312 insertions(+), 42 deletions(-) diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index baf0cfe09..b4893445f 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -126,6 +126,9 @@ FunctionEnd Function .onInit StrCpy $InstallLogPath "$TEMP\Cockatrice-installer.log" +; No backup has been taken yet; RestoreUserDataAfterUninstall must be a no-op +; until BackupUserDataBeforeUninstall sets this after actually copying something. +StrCpy $DataBackupDir "" ; Start each run from a clean file so the log that gets sent back is the last ; run, not every past run glued together. FileOpen $LogHandle "$InstallLogPath" w @@ -350,28 +353,49 @@ FunctionEnd ; destructive one is already on disk on affected machines, so stash a copy ; outside $INSTDIR before invoking it and put it back afterwards. Best effort ; only: a failed backup must never abort the install. +; +; The backup is intentionally left in %TEMP% afterwards as a second safety copy. +; Non-portable data paths beyond loose decks are covered by the client's +; startup safety backup (warnAboutDataPathsInProgramDir). Function BackupUserDataBeforeUninstall -StrCpy $DataBackupDir "$TEMP\CockatriceDataBackup" +; Always start from a clean slate: %TEMP% survives across installer runs, and a +; leftover backup must never be mistaken for this run's - a fresh install would +; otherwise resurrect ancient decks via RestoreUserDataAfterUninstall. +RMDir /r "$TEMP\CockatriceDataBackup" +StrCpy $DataBackupDir "" ${If} ${FileExists} "$INSTDIR\portable.dat" -${AndIf} ${FileExists} "$INSTDIR\data\*.*" - !insertmacro CockLog "pre-uninstall backup: copying $INSTDIR\data" +${AndIf} ${FileExists} "$INSTDIR\data" + StrCpy $DataBackupDir "$TEMP\CockatriceDataBackup" + !insertmacro CockLog "pre-uninstall backup: copying $INSTDIR\data (downloaded pics excluded)" DetailPrint "Backing up portable user data before removing previous version..." CreateDirectory "$DataBackupDir\data" CopyFiles /SILENT "$INSTDIR\data\*.*" "$DataBackupDir\data\" + ; Downloaded pictures are large and re-downloadable, so drop them from the + ; safety backup - but keep user-made custom pics (data\pics\CUSTOM). + RMDir /r "$DataBackupDir\data\pics" + ${If} ${FileExists} "$INSTDIR\data\pics\CUSTOM" + CreateDirectory "$DataBackupDir\data\pics\CUSTOM" + CopyFiles /SILENT "$INSTDIR\data\pics\CUSTOM\*.*" "$DataBackupDir\data\pics\CUSTOM\" + ${EndIf} DetailPrint "Portable user data backup complete." ${EndIf} ${If} ${FileExists} "$INSTDIR\*.cod" +${OrIf} ${FileExists} "$INSTDIR\*.dec" + ${If} $DataBackupDir == "" + StrCpy $DataBackupDir "$TEMP\CockatriceDataBackup" + ${EndIf} !insertmacro CockLog "pre-uninstall backup: copying loose deck files" DetailPrint "Backing up deck files stored in the install directory..." CreateDirectory "$DataBackupDir\loose_decks" CopyFiles /SILENT "$INSTDIR\*.cod" "$DataBackupDir\loose_decks\" + CopyFiles /SILENT "$INSTDIR\*.dec" "$DataBackupDir\loose_decks\" ${EndIf} FunctionEnd Function RestoreUserDataAfterUninstall StrCmp $DataBackupDir "" done -${If} ${FileExists} "$DataBackupDir\data\*.*" - ${If} ${FileExists} "$INSTDIR\data\*.*" +${If} ${FileExists} "$DataBackupDir\data" + ${If} ${FileExists} "$INSTDIR\data" !insertmacro CockLog "post-uninstall restore: user data survived, backup kept" DetailPrint "Previous user data survived the uninstall; backup kept at $DataBackupDir\data." ${Else} @@ -382,9 +406,11 @@ ${If} ${FileExists} "$DataBackupDir\data\*.*" ${EndIf} ${EndIf} ${If} ${FileExists} "$DataBackupDir\loose_decks\*.cod" +${OrIf} ${FileExists} "$DataBackupDir\loose_decks\*.dec" !insertmacro CockLog "post-uninstall restore: restoring loose deck files" CreateDirectory "$INSTDIR" CopyFiles /SILENT "$DataBackupDir\loose_decks\*.cod" "$INSTDIR\" + CopyFiles /SILENT "$DataBackupDir\loose_decks\*.dec" "$INSTDIR\" DetailPrint "Restored deck files stored in the install directory." ${IfNot} ${Silent} MessageBox MB_OK|MB_ICONINFORMATION "Your decks were stored inside the Cockatrice install folder, which the previous version's uninstaller deletes.$\n$\nThey have been restored from a backup. Please move them out of the install folder (Settings -> General -> Paths) so a future uninstall cannot delete them again." diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index dfeac5793..db54801f6 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -626,11 +626,15 @@ void MainWindow::warnAboutDataPathsInProgramDir() QStringList offenders; if (isPortable) { // Portable data lives inside the install directory by design, so there is - // nothing to reconfigure. Back up the small irreplaceable data (decks, custom - // sets, settings) instead. Pictures are deliberately excluded: they are large - // and can always be re-downloaded. + // nothing to reconfigure. Back up the small irreplaceable data instead. + // Pictures are deliberately excluded: they are large and can always be + // re-downloaded (the installer's pre-uninstall backup keeps custom pics + // but drops downloaded ones for the same reason). Replays are excluded + // here for size; the installer backup covers them. const QString dataDir = SettingsCache::instance().getDataPath(); - for (const QString &sub : {QStringLiteral("decks"), QStringLiteral("customsets"), QStringLiteral("settings")}) { + for (const QString &sub : + QStringList{QStringLiteral("decks"), QStringLiteral("customsets"), QStringLiteral("settings"), + QStringLiteral("filters"), QStringLiteral("themes")}) { const QString candidate = QDir(dataDir).absoluteFilePath(sub); if (QFileInfo::exists(candidate)) { backupSources.append(candidate); @@ -651,22 +655,26 @@ void MainWindow::warnAboutDataPathsInProgramDir() if (PathsSettings::isInsideDir(backupRoot, appDir)) { backupRoot = QDir::temp().absoluteFilePath(QStringLiteral("CockatriceBackups")); } + // One timestamped session directory for all sources, so a second boundary + // never splits a single startup check across two backup folders and + // same-named sources cannot overwrite each other. QStringList backupLocations; - for (const QString &source : backupSources) { - QString backupLocation; - if (UserDataBackup::backupAtRiskPath(source, backupRoot, &backupLocation)) { - qCInfo(MainWindowLog) << "Backed up at-risk user data" << source << "to" << backupLocation; - backupLocations.append(backupLocation); - } else { - qCWarning(MainWindowLog) << "Failed to back up at-risk user data:" << source; - } + QString sessionDir; + const bool backupComplete = + UserDataBackup::backupAtRiskPaths(backupSources, backupRoot, &backupLocations, &sessionDir); + for (const QString &backupLocation : backupLocations) { + qCInfo(MainWindowLog) << "Backed up at-risk user data to" << backupLocation; + } + if (!backupComplete) { + qCWarning(MainWindowLog) << "At-risk user data backup incomplete, session dir:" << sessionDir; } QString message; if (isPortable) { message = tr("This portable install keeps its data inside the program directory, where the " "uninstallers shipped in Cockatrice 3.1.0/3.1.1 delete everything on uninstall or " - "update.\n\nA safety backup of your decks, custom sets and settings has been created " + "update.\n\nA safety backup of your decks, custom sets, settings, filters and themes has " + "been created " "at:\n%1\n\nUninstalling with the old uninstaller still deletes the program " "directory, so keep your own copy of that backup until you have updated once.") .arg(backupLocations.isEmpty() ? backupRoot : backupLocations.join(QLatin1Char('\n'))); @@ -679,6 +687,9 @@ void MainWindow::warnAboutDataPathsInProgramDir() .arg(offenders.join(QLatin1Char('\n'))); if (!backupLocations.isEmpty()) { message += tr("\n\nA safety backup has been created at:\n%1").arg(backupLocations.join(QLatin1Char('\n'))); + if (!backupComplete) { + message += tr("\n\nThe backup may be incomplete; please back up these folders manually as well."); + } } else { message += tr("\n\nThe automatic safety backup failed; please back up these folders manually."); } diff --git a/libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp b/libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp index 554872b0a..a6c287923 100644 --- a/libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp +++ b/libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp @@ -10,6 +10,31 @@ namespace UserDataBackup { +namespace +{ + +QString uniqueTargetPath(const QString &sessionDir, const QString &fileName) +{ + QDir dir(sessionDir); + QString candidate = dir.absoluteFilePath(fileName); + if (!QFileInfo::exists(candidate)) { + return candidate; + } + const QFileInfo info(fileName); + const QString base = info.completeBaseName(); + const QString suffix = info.suffix(); + for (int i = 2;; ++i) { + QString numbered = suffix.isEmpty() ? QStringLiteral("%1 (%2)").arg(base).arg(i) + : QStringLiteral("%1 (%2).%3").arg(base).arg(i).arg(suffix); + candidate = dir.absoluteFilePath(numbered); + if (!QFileInfo::exists(candidate)) { + return candidate; + } + } +} + +} // namespace + bool copyDirectoryRecursively(const QString &sourceDir, const QString &destDir) { QDir source(sourceDir); @@ -32,6 +57,9 @@ bool copyDirectoryRecursively(const QString &sourceDir, const QString &destDir) ok = false; } } else { + // Ensure the parent exists even if the iterator yields a file + // before its directory entry. + QDir().mkpath(QFileInfo(target).absolutePath()); if (QFile::exists(target) && !QFile::remove(target)) { ok = false; continue; @@ -44,37 +72,84 @@ bool copyDirectoryRecursively(const QString &sourceDir, const QString &destDir) return ok; } -bool backupAtRiskPath(const QString &sourcePath, const QString &backupRoot, QString *backupLocationOut) +bool createBackupSessionDir(const QString &backupRoot, QString *sessionDirOut) { - QFileInfo source(sourcePath); - if (!source.exists()) { + if (!QDir().mkpath(backupRoot)) { return false; } - QString timestamp = QDateTime::currentDateTime().toString("yyyyMMdd-hhmmss"); - QString backupDir = QDir(backupRoot).absoluteFilePath("CockatriceDataBackup-" + timestamp); - if (!QDir().mkpath(backupDir)) { + const QString timestamp = QDateTime::currentDateTime().toString(QStringLiteral("yyyyMMdd-hhmmss-zzz")); + const QDir root(backupRoot); + QString sessionDir = root.absoluteFilePath(QStringLiteral("CockatriceDataBackup-") + timestamp); + for (int i = 1; QFileInfo::exists(sessionDir); ++i) { + sessionDir = root.absoluteFilePath(QStringLiteral("CockatriceDataBackup-%1-%2").arg(timestamp).arg(i)); + } + if (!QDir().mkpath(sessionDir)) { + return false; + } + if (sessionDirOut) { + *sessionDirOut = sessionDir; + } + return true; +} + +bool backupAtRiskPath(const QString &sourcePath, const QString &backupRoot, QString *backupLocationOut) +{ + QStringList locations; + const bool ok = backupAtRiskPaths({sourcePath}, backupRoot, &locations, nullptr); + if (!locations.isEmpty() && backupLocationOut) { + *backupLocationOut = locations.first(); + } + return ok; +} + +bool backupAtRiskPaths(const QStringList &sourcePaths, + const QString &backupRoot, + QStringList *backupLocationsOut, + QString *sessionDirOut) +{ + QStringList existing; + for (const QString &sourcePath : sourcePaths) { + if (QFileInfo::exists(sourcePath)) { + existing.append(sourcePath); + } + } + if (existing.isEmpty()) { return false; } - bool ok; - QString backupLocation; - if (source.isDir()) { - backupLocation = QDir(backupDir).absoluteFilePath(source.fileName()); - ok = copyDirectoryRecursively(sourcePath, backupLocation); - } else { - backupLocation = QDir(backupDir).absoluteFilePath(source.fileName()); - if (QFile::exists(backupLocation)) { - QFile::remove(backupLocation); - } - ok = QFile::copy(sourcePath, backupLocation); - } - if (!ok) { + QString sessionDir; + if (!createBackupSessionDir(backupRoot, &sessionDir)) { return false; } - if (backupLocationOut) { - *backupLocationOut = backupLocation; + if (sessionDirOut) { + *sessionDirOut = sessionDir; } - return true; + + bool allOk = true; + QStringList locations; + for (const QString &sourcePath : existing) { + const QFileInfo source(sourcePath); + const QString backupLocation = uniqueTargetPath(sessionDir, source.fileName()); + bool ok; + if (source.isDir()) { + ok = copyDirectoryRecursively(sourcePath, backupLocation); + } else { + ok = QFile::copy(sourcePath, backupLocation); + } + // Report the location whenever a target was created, even for partial + // copies, so callers can point the user at what was salvaged. + if (QFileInfo::exists(backupLocation)) { + locations.append(backupLocation); + } else { + // Total failure with no target created (e.g. QFile::copy failed). + ok = false; + } + allOk = allOk && ok; + } + if (backupLocationsOut) { + *backupLocationsOut = locations; + } + return allOk && !locations.isEmpty(); } QString defaultBackupRoot() diff --git a/libcockatrice_utility/libcockatrice/utility/user_data_backup.h b/libcockatrice_utility/libcockatrice/utility/user_data_backup.h index 57656f0c5..db391da90 100644 --- a/libcockatrice_utility/libcockatrice/utility/user_data_backup.h +++ b/libcockatrice_utility/libcockatrice/utility/user_data_backup.h @@ -17,6 +17,7 @@ #define USER_DATA_BACKUP_H #include +#include namespace UserDataBackup { @@ -24,18 +25,45 @@ namespace UserDataBackup /** * @brief Recursively copies @p sourceDir into @p destDir, creating it as needed. * @returns true when every file was copied (or there was nothing to copy). + * Symlinks are copied best-effort via QFile::copy; a dangling link fails that entry only. */ bool copyDirectoryRecursively(const QString &sourceDir, const QString &destDir); +/** + * @brief Creates a fresh timestamped session directory under @p backupRoot. + * @param backupRoot directory OUTSIDE the install tree that will hold timestamped backups. + * @param sessionDirOut receives the created directory on success. + * @returns true on success. Millisecond timestamps plus a numeric suffix keep rapid + * successive calls (e.g. one per installer phase) from sharing a directory. + */ +bool createBackupSessionDir(const QString &backupRoot, QString *sessionDirOut); + /** * @brief Copies an at-risk path (file or directory) to a timestamped folder under @p backupRoot. * @param sourcePath file or directory holding user data (must exist). * @param backupRoot directory OUTSIDE the install tree that will hold timestamped backups. - * @param backupLocationOut receives the created backup file or directory on success. - * @returns true on success, false when the source is missing or nothing could be copied. + * @param backupLocationOut receives the created backup file or directory whenever a target + * was created, even when the copy was only partial (i.e. also on failure with a partial copy). + * @returns true when the whole source was copied, false when the source is missing or + * any entry failed. A false return with a non-empty @p backupLocationOut means a partial + * backup exists at that location. */ bool backupAtRiskPath(const QString &sourcePath, const QString &backupRoot, QString *backupLocationOut = nullptr); +/** + * @brief Copies several at-risk paths into a single timestamped session directory. + * @param sourcePaths files or directories holding user data; missing entries are skipped. + * @param backupRoot directory OUTSIDE the install tree that will hold timestamped backups. + * @param backupLocationsOut receives one entry per source that produced a backup target + * (including partial copies). + * @param sessionDirOut optionally receives the shared session directory. + * @returns true when every existing source was copied completely. + */ +bool backupAtRiskPaths(const QStringList &sourcePaths, + const QString &backupRoot, + QStringList *backupLocationsOut, + QString *sessionDirOut = nullptr); + /** * @brief Default backup root guaranteed to live outside the install directory. */ diff --git a/tests/user_data_backup_test.cpp b/tests/user_data_backup_test.cpp index c48b103de..829ceb90c 100644 --- a/tests/user_data_backup_test.cpp +++ b/tests/user_data_backup_test.cpp @@ -25,6 +25,60 @@ TEST(UserDataBackup, CopyDirectoryKeepsNestedStructure) EXPECT_TRUE(QFile::exists(dest + "/decks/sub/mydeck.cod")); } +TEST(UserDataBackup, CopyDirectoryKeepsHiddenFiles) +{ + QTemporaryDir sourceDir; + QTemporaryDir destParent; + ASSERT_TRUE(sourceDir.isValid()); + ASSERT_TRUE(destParent.isValid()); + QFile hidden(QDir(sourceDir.path()).absoluteFilePath(".hidden.ini")); + ASSERT_TRUE(hidden.open(QIODevice::WriteOnly | QIODevice::Text)); + hidden.write("[general]\n"); + hidden.close(); + + QString dest = QDir(destParent.path()).absoluteFilePath("copy"); + EXPECT_TRUE(UserDataBackup::copyDirectoryRecursively(sourceDir.path(), dest)); + EXPECT_TRUE(QFile::exists(dest + "/.hidden.ini")); +} + +TEST(UserDataBackup, CopyDirectoryOverwritesExistingTarget) +{ + QTemporaryDir sourceDir; + QTemporaryDir destParent; + ASSERT_TRUE(sourceDir.isValid()); + ASSERT_TRUE(destParent.isValid()); + const QString sourceFile = QDir(sourceDir.path()).absoluteFilePath("settings.ini"); + QFile first(sourceFile); + ASSERT_TRUE(first.open(QIODevice::WriteOnly | QIODevice::Text)); + first.write("version=1\n"); + first.close(); + + QString dest = QDir(destParent.path()).absoluteFilePath("copy"); + ASSERT_TRUE(UserDataBackup::copyDirectoryRecursively(sourceDir.path(), dest)); + + QFile second(sourceFile); + ASSERT_TRUE(second.open(QIODevice::WriteOnly | QIODevice::Text)); + second.write("version=2\n"); + second.close(); + + EXPECT_TRUE(UserDataBackup::copyDirectoryRecursively(sourceDir.path(), dest)); + QFile result(dest + "/settings.ini"); + ASSERT_TRUE(result.open(QIODevice::ReadOnly | QIODevice::Text)); + EXPECT_EQ(result.readAll(), QByteArray("version=2\n")); +} + +TEST(UserDataBackup, CopyEmptyDirectorySucceeds) +{ + QTemporaryDir sourceDir; + QTemporaryDir destParent; + ASSERT_TRUE(sourceDir.isValid()); + ASSERT_TRUE(destParent.isValid()); + ASSERT_TRUE(QDir(sourceDir.path()).mkpath("empty")); + + QString dest = QDir(destParent.path()).absoluteFilePath("copy"); + EXPECT_TRUE(UserDataBackup::copyDirectoryRecursively(sourceDir.path(), dest)); + EXPECT_TRUE(QDir(dest + "/empty").exists()); +} TEST(UserDataBackup, CopyMissingDirectoryFails) { QTemporaryDir destParent; @@ -75,6 +129,82 @@ TEST(UserDataBackup, BackupMissingSourceFails) EXPECT_FALSE(UserDataBackup::backupAtRiskPath("/nonexistent-cockatrice-path-xyz", backupRoot.path(), nullptr)); } +TEST(UserDataBackup, BackupPathsShareSingleSessionDir) +{ + QTemporaryDir sourceDir; + QTemporaryDir backupRoot; + ASSERT_TRUE(sourceDir.isValid()); + ASSERT_TRUE(backupRoot.isValid()); + QDir source(sourceDir.path()); + ASSERT_TRUE(source.mkpath("decks")); + ASSERT_TRUE(source.mkpath("settings")); + QFile deck(source.absoluteFilePath("decks/mydeck.cod")); + ASSERT_TRUE(deck.open(QIODevice::WriteOnly | QIODevice::Text)); + deck.write("\n"); + deck.close(); + QFile settings(source.absoluteFilePath("settings/settings.ini")); + ASSERT_TRUE(settings.open(QIODevice::WriteOnly | QIODevice::Text)); + settings.write("[general]\n"); + settings.close(); + + QStringList locations; + QString sessionDir; + EXPECT_TRUE( + UserDataBackup::backupAtRiskPaths({source.absoluteFilePath("decks"), source.absoluteFilePath("settings")}, + backupRoot.path(), &locations, &sessionDir)); + ASSERT_EQ(locations.size(), 2); + // Both backups live in the same timestamped session, even across a second boundary. + for (const QString &location : locations) { + EXPECT_TRUE(location.startsWith(QDir(sessionDir).absolutePath())); + } + EXPECT_TRUE(QFile::exists(QDir(sessionDir).absoluteFilePath("decks/mydeck.cod"))); + EXPECT_TRUE(QFile::exists(QDir(sessionDir).absoluteFilePath("settings/settings.ini"))); +} + +TEST(UserDataBackup, BackupPathsWithSameNameDoNotCollide) +{ + QTemporaryDir firstDir; + QTemporaryDir secondDir; + QTemporaryDir backupRoot; + ASSERT_TRUE(firstDir.isValid()); + ASSERT_TRUE(secondDir.isValid()); + ASSERT_TRUE(backupRoot.isValid()); + for (const QString &dir : {firstDir.path(), secondDir.path()}) { + QFile deck(QDir(dir).absoluteFilePath("cards.xml")); + ASSERT_TRUE(deck.open(QIODevice::WriteOnly | QIODevice::Text)); + deck.write("\n"); + deck.close(); + } + + QStringList locations; + EXPECT_TRUE(UserDataBackup::backupAtRiskPaths( + {QDir(firstDir.path()).absoluteFilePath("cards.xml"), QDir(secondDir.path()).absoluteFilePath("cards.xml")}, + backupRoot.path(), &locations, nullptr)); + ASSERT_EQ(locations.size(), 2); + EXPECT_NE(locations.at(0), locations.at(1)); + EXPECT_TRUE(QFile::exists(locations.at(0))); + EXPECT_TRUE(QFile::exists(locations.at(1))); +} + +TEST(UserDataBackup, RapidBackupsGetUniqueSessionDirs) +{ + QTemporaryDir sourceDir; + QTemporaryDir backupRoot; + ASSERT_TRUE(sourceDir.isValid()); + ASSERT_TRUE(backupRoot.isValid()); + QFile deck(QDir(sourceDir.path()).absoluteFilePath("mydeck.cod")); + ASSERT_TRUE(deck.open(QIODevice::WriteOnly | QIODevice::Text)); + deck.write("\n"); + deck.close(); + + QString firstSession; + QString secondSession; + QStringList locations; + EXPECT_TRUE(UserDataBackup::backupAtRiskPaths({sourceDir.path()}, backupRoot.path(), &locations, &firstSession)); + EXPECT_TRUE(UserDataBackup::backupAtRiskPaths({sourceDir.path()}, backupRoot.path(), &locations, &secondSession)); + EXPECT_NE(firstSession, secondSession); +} + TEST(UserDataBackup, DefaultBackupRootIsUsable) { QString root = UserDataBackup::defaultBackupRoot();