FileTarget - Avoid parsing archive sequence number when other digits #5988
Conversation
WalkthroughIntroduces a new archive-suffix heuristic in RollingArchiveFileHandler: computes whether the archive suffix lacks digits and passes this as a new parameter to DeleteOldFilesBeforeArchive, which now conditions old-file deletion on this flag. Adds System.Linq for Any, and updates the method signature accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant Logger as Logger
participant Handler as RollingArchiveFileHandler
participant FS as FileSystem
App->>Logger: Write triggers file roll
Logger->>Handler: ArchiveBeforeOpenFile(newFilePath, initialOpen)
Note over Handler: Determine if archive suffix lacks digits
Handler->>Handler: archiveSuffixWithSeqNo = !GetFileNameWithoutExt(newFilePath).Any(IsDigit)
Handler->>Handler: DeleteOldFilesBeforeArchive(newFilePath, initialOpen, archiveSuffixWithSeqNo)
alt archiveSuffixWithSeqNo = true
Note over Handler,FS: Use sequence-numbered suffix matching
Handler->>FS: Enumerate and delete old files matching seq pattern
else archiveSuffixWithSeqNo = false
Note over Handler,FS: Use timestamp/other suffix matching
Handler->>FS: Enumerate and delete old files via default pattern
end
Handler-->>Logger: Archive pruning complete
Logger-->>App: Continue with new file open
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Targets/FileArchiveHandlers/RollingArchiveFileHandler.cs (1)
38-38: Clarify intent and avoid LINQ allocation here (minor).The logic is correct and aligns with the PR goal. Two small polish items:
- Rename the local for readability (true means “base name has no digits; safe to parse a seq-no suffix”).
- Drop LINQ’s
Anyto avoid the extra import and iterator allocation.Apply:
- using System.Linq; @@ - var archiveSuffixWithSeqNo = !Path.GetFileNameWithoutExtension(newFilePath).Any(c => char.IsDigit(c)); - bool deletedOldFiles = DeleteOldFilesBeforeArchive(newFilePath, initialFileOpen, archiveSuffixWithSeqNo: archiveSuffixWithSeqNo); + // True when base filename has no digits; safe to assume archive suffix contains sequence-number + var baseName = Path.GetFileNameWithoutExtension(newFilePath); + bool assumeArchiveSuffixHasSeqNo = true; + for (int i = 0; i < baseName.Length; i++) + { + char ch = baseName[i]; + if (ch >= '0' && ch <= '9') { assumeArchiveSuffixHasSeqNo = false; break; } + } + bool deletedOldFiles = DeleteOldFilesBeforeArchive(newFilePath, initialFileOpen, archiveSuffixWithSeqNo: assumeArchiveSuffixHasSeqNo);Also applies to: 59-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/NLog/Targets/FileArchiveHandlers/RollingArchiveFileHandler.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Targets/FileArchiveHandlers/RollingArchiveFileHandler.cs (1)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (2)
DeleteOldFilesBeforeArchive(52-60)DeleteOldFilesBeforeArchive(62-114)
|
Followup to #5987 by making the parsing of archive sequence number more strict (Avoid parsing ${processid} or ${shortdate} as archive sequence number)