FreeAndNil commented on PR #336:
URL: https://github.com/apache/logging-log4net/pull/336#issuecomment-6112864326
Implementation plan for #335, as approved before the work started, kept here
for the record. The merged change took a different route than step 1 of the
plan: it switches `StaticLogFileName` off instead of making no numbered backups
(see the PR description).
<details>
<summary>Plan</summary>
# Plan for #335: short date pattern collides with size backup names
## Context
Issue #335 (split out of #334, reported by @Snotface): with
`StaticLogFileName` true, a
`DatePattern` that formats as a short number (`.dd`, `.MM`, `.HH`, `.d`)
names dated files like
numbered size backups: `log.txt.10` is both the file for the 10th and backup
10.
Why files get destroyed (`src/log4net/Appender/RollingFileAppender.cs`, at
`d0efa655`):
- `RollFile` deletes its target before `File.Move`, so a size roll onto
`log.txt.10` destroys the
dated file, and a time roll onto `log.txt.10` destroys backup 10.
- `DetermineCurSizeRollBackups` counts the dated files as backups, so
`RollOverRenameFiles`
deletes the "oldest backup" `log.txt.10` and `RollOverTime` moves
`log.txt.1..Cur` as backups.
- Affected: `Composite`, and `Date` with `AppendToFile` false (the startup
roll-out in
`ExistingInit` makes numbered backups when `MaxSizeRollBackups` is not 0).
Decision (user, 2026-10-10): **report at activation and make no numbered
backups** for such an
appender. Date rolling keeps working, the file grows within the period.
Accepted cost from the
plan review: with `AppendToFile` false the file of the same period is
overwritten on restart, as
with `MaxSizeRollBackups` 0. The changelog states that.
Not a collision, and must not be reported (plan review): the default
count-down setup with a finite
limit. With `CountDirection` -1 and `MaxSizeRollBackups` N only `.1` to `.N`
exist, and the dated
names of a k-digit sample start at `10^(k-1)` (`.dd` gives `.01`, which is
not `.1`). So `.dd`
with 5 backups is fine, `.dd` with 10 collides, `.d` always collides,
`CountDirection` 0 or 1 and
`MaxSizeRollBackups` -1 always collide.
## Steps
### 0. Branch
`git switch -c Feature/335-short-date-pattern-backup-collision master`
(done, at `d0efa655`).
### 1. `RollingFileAppender`: detect, report, behave as `MaxSizeRollBackups` 0
No new field. In `ActivateOptions`, after the `ComputeCheckPeriod` block and
before
`ExistingInit()`:
```csharp
if (DoesDatePatternCollideWithBackupNames())
{
LogLog.Warn(_declaringType, $"""
DatePattern [{DatePattern}] of [{Name}] names dated files like numbered
backups, so none are
made. Use a DatePattern with a separator or more digits, such as
.yyyyMMdd, or set
StaticLogFileName to false.
""");
MaxSizeRollBackups = 0;
_rollSize = false;
}
```
`MaxSizeRollBackups = 0` makes every existing path do the right thing:
`InitializeFromOneFile`
stays at zero, so the time roll moves nothing; `ExistingInit` takes its
overwrite branch instead
of `RollOverRenameFiles`; `OpenFile` raises no INTERNAL ERROR. `_rollSize =
false` keeps a size
roll from truncating the file, which is what 0 backups would otherwise do at
`MaxFileSize`. Neither
needs undoing: reconfiguration creates a new appender. `LogLog.Warn`, not
`ErrorHandler.Error`: the default
`OnlyOnceErrorHandler` reports only its first error, so the activation
report would mute a later
write failure; `LogLog` reaches the `ConfigureAndWatch` message list just
the same.
Detection, a private helper next to `ComputeCheckPeriod`, which already
formats `_sDate1970`:
```csharp
/// <summary>
/// With a static name, a date that formats as a dot and up to four digits
(<c>.dd</c> writes
/// <c>.10</c>) is also the name of backup 10. Counting down to a limit
below the smallest such
/// name never reaches it. Five or more digits never count as a backup, see
<see cref="IsDateSuffix"/>.
/// </summary>
private bool DoesDatePatternCollideWithBackupNames()
```
True when `_rollDate && StaticLogFileName && DatePattern is not null &&
MaxSizeRollBackups != 0 &&
(_rollSize || !AppendToFile)`, the sample `_sDate1970.ToString(DatePattern,
InvariantInfo)` is a `.`
followed by one to four digits (a `for` loop with `c is < '0' or > '9'`, no
`Regex`), and
`CountDirection >= 0 || MaxSizeRollBackups < 0 || MaxSizeRollBackups >=
(int)Math.Pow(10, digits - 1)`.
Docs: one sentence on the `DatePattern` property remarks, saying that such
an appender is
reported when activated, makes no numbered backups and then reads
`MaxSizeRollBackups` as 0. That
makes the changed public property visible; a second `ActivateOptions` on the
same instance sees 0
and skips the warning, which is acceptable for a minimal fix.
### 2. Tests, `src/log4net.Tests/Appender/RollingFileAppenderTest.cs`
Shape: `RemoteSyslogAppenderTest.IdentityWithControlCharactersIsReported`
(`src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs:256`):
`List<LogLog> messages`,
`LogLog.ExecuteWithoutEmittingInternalMessages` around `using
LogLog.LogReceivedAdapter _ =
new(messages);` plus the activation, assert on `messages.ConvertAll(m =>
m.Message)`. The adapter
hooks a static event, so `[NonParallelizable]`. Folder from `using
AutoTempFolder folder = new();`,
close the appender before the scope ends. Doc comment on each test.
1. `ShortDatePatternMakesNoBackups`: `Composite`, `.dd`, static,
`MaxSizeRollBackups` 10,
`MaxFileSize` 100, `File` in the temp folder, `DateTimeStrategy` a
`MockDateTime`. Assert one
warning containing `DatePattern [.dd]`, log past the size a few times,
assert the folder holds
only `log.txt` and that its length is above `MaxFileSize`. Fails at
`d0efa655` (a `log.txt.1`
appears).
2. `ShortDatePatternIsReportedOnlyWhenBackupsCollide`, `[TestCase]`s
shortest to longest source
line, arguments (pattern, static, maxBackups, countDirection, expected):
`.dd` 10 -1 true;
`.dd` 5 -1 false; `.d` 5 -1 true; `.dd` 5 1 true; `.dd` -1 -1 true;
`.yyyy-MM-dd` 10 -1 false;
`.dd` with `StaticLogFileName` false → false. `Composite`, `AppendToFile`
true, so no file is
touched beyond creation. `.yyyy` is not a case: `ComputeCheckPeriod`
already throws for it.
3. `ShortDatePatternRestartWithoutAppendMakesNoBackups`, the second affected
path: `Date`, `.d`,
static, `AppendToFile` false, `MaxSizeRollBackups` 3, an existing
`log.txt` in the folder with
its last-write time set to the mocked date, so
`RollOverIfDateBoundaryCrossing` sees the same
period and does not time roll. After `ActivateOptions` the folder holds
no `log.txt.1`. Fails
at `d0efa655`, where `ExistingInit` rolls the file to `log.txt.1`.
### 3. Changelog
`src/changelog/3.5.1/335-short-date-pattern-collides-with-backup-names.xml`,
`type="fixed"`,
`<issue id="335"
link="https://github.com/apache/logging-log4net/issues/335"/>`. Prose, no
bullets: what collided, that such an appender is now reported at activation
and makes no numbered
backups (with `AppendToFile` false the file of the same period is
overwritten on restart, as with
`MaxSizeRollBackups` 0), and the opt-out (a `DatePattern` with a separator
or at least five digits,
or `StaticLogFileName` false). Close with `(reported by @Snotface, fixed by
@FreeAndNil)`.
### 4. Manual
`src/site/antora/modules/ROOT/pages/manual/configuration/appenders/rollingfileappender.adoc`:
one
NOTE paragraph after the Composite example (its `yyyyMMdd` has no dot and is
fine). No ` -- `.
### 5. Drafts into `.git/plans/335/`
`commit.txt` in the style of the #334 one (subject with `#335`, bullets, no
attribution line) and
a short `pr.md`.
## Verification
```
S=/tmp/claude-1002/-apache-logging-log4net/bae2ed16-61bf-4ff2-908d-7be0718e13b1/scratchpad
dotnet build src/log4net.sln > $S/build.txt 2>&1 # read the whole
file, 0 warnings
dotnet test src/log4net.Tests/log4net.Tests.csproj --filter
"FullyQualifiedName~RollingFileAppenderTest" > $S/test.txt 2>&1
```
Revert to fail: copy the appender source to `$S/RFA.fixed.cs`, `git checkout
d0efa655 --
src/log4net/Appender/RollingFileAppender.cs`, rerun with `--filter
"FullyQualifiedName~ShortDatePattern"` and confirm tests 1 and 3 fail, copy
the saved file back. Then grep
the touched files for `[—–]` and ` -- `.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
