-
Notifications
You must be signed in to change notification settings - Fork 1.4k
FileTarget - Strict wildcard check only possible when single wildcard #5981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdjusts wildcard index calculation and propagation for archive file operations so EndIndex is only non-negative for a single '*' wildcard; adds a guard to skip sequence scanning when no wildcard exists; simplifies a character check; and changes Rolling handler to use endIndex = -1 when no wildcard. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Logger
participant BaseHandler as BaseFileArchiveHandler
participant Exclude as ExcludeFileName
participant Cleanup as CleanupFiles
Logger->>BaseHandler: DeleteOldFilesBeforeArchive(fileWildcard)
Note over BaseHandler: Determine archiveCleanupReason\nThen compute fileWildcardStartIndex\nCompute fileWildcardEndIndex only if exactly one '*'
alt Single "*" wildcard
BaseHandler->>Exclude: call with startIndex and EndIndex
BaseHandler->>Cleanup: call with startIndex and EndIndex
else No or multiple "*" wildcards
BaseHandler->>Exclude: call with startIndex and endIndex = -1
BaseHandler->>Cleanup: call with startIndex and endIndex = -1
end
sequenceDiagram
autonumber
participant Rolling as RollingArchiveFileHandler
participant Seq as GetMaxArchiveSequenceNo
Rolling->>Rolling: RollToInitialSequenceNumber(archivePattern)
Note over Rolling: compute wildcard indices after path/dir checks
alt Wildcard present (single '*')
Rolling->>Seq: startIndex, endIndex (positive)
else No wildcard or multiple '*'
Note over Rolling: endIndex = -1
Rolling->>Seq: startIndex, endIndex = -1
end
Seq-->>Rolling: maxSequenceNo (or null)
Rolling->>Rolling: increment if ArchiveOldFileOnStartup
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
3b6ec8f to
c7d7f67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/NLog/Targets/FileArchiveHandlers/RollingArchiveFileHandler.cs (1)
130-134: Align wildcard index semantics with Base handler and avoid path-level Replace.For consistency with BaseFileArchiveHandler (single '' strictness) and to avoid accidental Replace in directory segments, compute indices from the file name and use LastIndexOf('').
- var archivePathWildCard = _fileTarget.BuildFullFilePath(newFilePath, int.MaxValue, DateTime.MinValue).Replace(int.MaxValue.ToString(), "*"); - var archiveFileName = Path.GetFileName(archivePathWildCard); - var fileWildcardStartIndex = archiveFileName.IndexOf('*'); - var fileWildcardEndIndex = fileWildcardStartIndex >= 0 ? archiveFileName.Length - fileWildcardStartIndex : -1; + var archiveTestPath = _fileTarget.BuildFullFilePath(newFilePath, int.MaxValue, DateTime.MinValue); + var archiveFileName = Path.GetFileName(archiveTestPath).Replace(int.MaxValue.ToString(), "*"); + var fileWildcardStartIndex = archiveFileName.IndexOf('*'); + var lastStarIndex = archiveFileName.LastIndexOf('*'); + var fileWildcardEndIndex = (fileWildcardStartIndex >= 0 && fileWildcardStartIndex == lastStarIndex) + ? archiveFileName.Length - fileWildcardStartIndex + : -1;src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
94-96: Single-wildcard strictness: good guard, consider centralizing helper.This matches the “strict only with single '*'” intent. To keep semantics consistent across callers (e.g., RollingArchiveFileHandler), consider extracting a small helper to compute (startIndex, endIndex) to avoid divergence.
Example helper (internal):
internal static (int Start, int End) ComputeSingleStarIndices(string pattern) { var s = pattern.IndexOf('*'); return (s >= 0 && s == pattern.LastIndexOf('*')) ? (s, pattern.Length - s) : (s, -1); }
📜 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 (3)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(2 hunks)src/NLog/Targets/FileArchiveHandlers/LegacyArchiveFileNameHandler.cs(1 hunks)src/NLog/Targets/FileArchiveHandlers/RollingArchiveFileHandler.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NLog/Targets/FileArchiveHandlers/LegacyArchiveFileNameHandler.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Targets/FileArchiveHandlers/RollingArchiveFileHandler.cs (2)
src/NLog/Targets/FileTarget.cs (3)
BuildFullFilePath(1090-1116)DateTime(837-855)DateTime(865-873)src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
GetMaxArchiveSequenceNo(116-119)
🔇 Additional comments (3)
src/NLog/Targets/FileArchiveHandlers/RollingArchiveFileHandler.cs (2)
106-110: Good early-exit with debug when directory is missing.Prevents needless work and noisy warnings.
138-145: Sequence increment on ArchiveOldFileOnStartup looks correct.The new unified flow is clearer and avoids nested returns.
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
159-164: Early return when no wildcard: correct and cheap.Prevents unnecessary scanning; aligns with new EndIndex semantics.
|



No description provided.