Skip to content

Commit b2ae167

Browse files
JusterZhuclaude
andauthored
fix: Copilot 审查后修复 — 备份排序改用创建时间、修正注释、补全测试断言 (#503)
* fix: address Copilot review feedback on backup PR (#502) 1. GetLatestBackup / CleanDirectories: sort by directory creation time instead of name. Legacy 'app-{version}' names are not reliably lexicographically sortable (e.g. app-10.0.0 vs app-2.0.0), and mixing .backups/ paths with installPath/ dirs breaks path-string ordering. Fallback to name tiebreaker for deterministic results. 2. ClientStrategy XML doc: corrected to .backups/backup-{yyyyMMddHHmmss} format (was stale __backups/backup_{...}). 3. CleanBackup_KeepsOnlyRecentVersions test: added missing assertion to verify legacy app-* directory retention behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: GetLatestBackup test now creates dirs in creation-time order The test expected the alphabetically-last directory, but GetLatestBackup now sorts by CreationTime. The newest (last-created) directory must match the expected result. Added Thread.Sleep(10) between creates to ensure distinct timestamps on fast filesystems (e.g. Linux CI). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent d52c666 commit b2ae167

3 files changed

Lines changed: 30 additions & 22 deletions

File tree

src/c#/GeneralUpdate.Core/FileSystem/StorageManager.cs

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -225,41 +225,44 @@ public static string GetTempDirectory(string name)
225225
/// <param name="installPath">The application installation root directory.</param>
226226
/// <returns>The full path of the most recent backup directory, or <c>null</c> if none exists.</returns>
227227
/// <remarks>
228-
/// Directories are sorted by name in descending order. Since both "backup-{yyyyMMddHHmmss}"
229-
/// and "app-{version}" formats are lexicographically sortable, this works for both.
230-
/// Search patterns are derived from <see cref="BlackDefaults.DefaultDirectories"/> by
231-
/// appending "*" to each entry — e.g. "backup-" becomes "backup-*".
228+
/// Sorts candidates by directory creation time descending (most recent first), with
229+
/// a name-based tie-breaker for deterministic results. Unlike lexicographic name
230+
/// sorting, this correctly handles mixed paths (e.g. .backups/ vs installPath/)
231+
/// and legacy version-format names ("app-10.0.0" vs "app-2.0.0").
232232
/// </remarks>
233233
public static string? GetLatestBackup(string installPath)
234234
{
235235
if (!Directory.Exists(installPath)) return null;
236236

237-
var allBackups = new List<string>();
237+
var allBackups = new List<DirectoryInfo>();
238238

239239
// Scan BackupRootDirectory subdirectory (new-format backup container)
240240
var backupRoot = Path.Combine(installPath, BackupRootDirectory);
241241
if (Directory.Exists(backupRoot))
242242
{
243-
allBackups.AddRange(Directory.GetDirectories(backupRoot, "*", SearchOption.TopDirectoryOnly));
243+
allBackups.AddRange(new DirectoryInfo(backupRoot)
244+
.GetDirectories("*", SearchOption.TopDirectoryOnly));
244245
}
245246

246247
// Scan installPath directly for backup dirs matching patterns from defaults
247-
allBackups.AddRange(GetBackupDirectories(installPath));
248+
allBackups.AddRange(GetBackupDirectoryInfos(installPath));
248249

249250
return allBackups
250-
.OrderByDescending(d => d)
251-
.FirstOrDefault();
251+
.OrderByDescending(d => d.CreationTime)
252+
.ThenByDescending(d => d.Name)
253+
.FirstOrDefault()?.FullName;
252254
}
253255

254256
/// <summary>
255257
/// Enumerates all backup directories in the given path using patterns derived
256258
/// from <see cref="BackupNamePrefixes"/> (both new and legacy formats).
257259
/// </summary>
258-
private static IEnumerable<string> GetBackupDirectories(string path)
260+
private static IEnumerable<DirectoryInfo> GetBackupDirectoryInfos(string path)
259261
{
262+
var dirInfo = new DirectoryInfo(path);
260263
foreach (var prefix in BackupNamePrefixes)
261264
{
262-
foreach (var dir in Directory.GetDirectories(path, prefix + "*", SearchOption.TopDirectoryOnly))
265+
foreach (var dir in dirInfo.GetDirectories(prefix + "*", SearchOption.TopDirectoryOnly))
263266
{
264267
yield return dir;
265268
}
@@ -637,9 +640,10 @@ private static IEnumerable<string> GetBackupSearchPatterns()
637640
/// </summary>
638641
private static void CleanDirectories(string rootPath, int keepVersions, string searchPattern = "*")
639642
{
640-
var dirs = Directory.GetDirectories(rootPath, searchPattern, SearchOption.TopDirectoryOnly)
641-
.Select(d => new DirectoryInfo(d))
642-
.OrderByDescending(d => d.Name)
643+
var dirs = new DirectoryInfo(rootPath)
644+
.GetDirectories(searchPattern, SearchOption.TopDirectoryOnly)
645+
.OrderByDescending(d => d.CreationTime)
646+
.ThenByDescending(d => d.Name)
643647
.Skip(keepVersions);
644648

645649
foreach (var dir in dirs)

src/c#/GeneralUpdate.Core/Strategy/ClientStrategy.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -854,7 +854,7 @@ private void InitBlackPolicy()
854854
/// </summary>
855855
/// <remarks>
856856
/// The backup operation is performed via <c>StorageManager.Backup</c>, excluding directories configured in the blacklist.
857-
/// The backup directory path format is: {InstallPath}/__backups/backup_{yyyyMMddHHmmss}.
857+
/// The backup directory path format is: {InstallPath}/.backups/backup-{yyyyMMddHHmmss}.
858858
/// This step can be skipped by setting <c>UpdateContext.BackupEnabled</c> to <c>false</c>.
859859
/// After a successful backup, old backups are cleaned up, retaining only the most recent 3.
860860
/// </remarks>

tests/CoreTest/FileSystem/BackupRestoreTests.cs

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
22
using System.IO;
33
using System.Linq;
4+
using System.Threading;
45
using GeneralUpdate.Core.FileSystem;
56
using Xunit;
67

@@ -71,9 +72,10 @@ public void CleanBackup_KeepsOnlyRecentVersions()
7172
Assert.Contains("backup-20260606000004", names[1]);
7273
Assert.Contains("backup-20260606000005", names[2]);
7374

74-
// Legacy app-* should also be cleaned (keeps top 3, but there's only 1 legacy,
75-
// and since we're using CleanBackup with keepVersions=3, the one legacy dir
76-
// at installPath level would be kept unless there are 3+ newer app-* dirs)
75+
// Legacy app-* — single directory is retained (only 1 total, keepVersions=3)
76+
var legacyRemaining = Directory.GetDirectories(installPath, "app-*");
77+
Assert.Single(legacyRemaining);
78+
Assert.Equal("app-0.0.1", Path.GetFileName(legacyRemaining[0]));
7779
}
7880
finally
7981
{
@@ -148,17 +150,19 @@ public void GetLatestBackup_ReturnsMostRecent()
148150
{
149151
Directory.CreateDirectory(installPath);
150152

151-
// Create backup dirs with different timestamps
153+
// Create backup dirs in creation-time order: oldest first, newest last
152154
var dir1 = Path.Combine(installPath, "backup-20260601000000");
153-
var dir2 = Path.Combine(installPath, "backup-20260606235200"); // This is the latest
154-
var dir3 = Path.Combine(installPath, "backup-20260603000000");
155+
var dir2 = Path.Combine(installPath, "backup-20260603000000");
156+
var dir3 = Path.Combine(installPath, "backup-20260606235200"); // Created last = most recent
155157
Directory.CreateDirectory(dir1);
158+
Thread.Sleep(10); // Ensure distinct creation time on fast filesystems
156159
Directory.CreateDirectory(dir2);
160+
Thread.Sleep(10);
157161
Directory.CreateDirectory(dir3);
158162

159163
var latest = StorageManager.GetLatestBackup(installPath);
160164
Assert.NotNull(latest);
161-
Assert.Equal(dir2, latest); // backup-20260606235200 is alphabetically last
165+
Assert.Equal(dir3, latest); // Most recently created
162166
}
163167
finally
164168
{

0 commit comments

Comments
 (0)