-
Notifications
You must be signed in to change notification settings - Fork 184
Fix/MultiFile pattern (Issue #696) #697
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
base: Development
Are you sure you want to change the base?
Changes from all commits
54b2891
3989ae2
15c3d96
905c979
1fcb262
568bb04
2a5e1ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,15 +116,23 @@ public string BuildFileName () | |
|
|
||
| if (_indexGroup != null && _indexGroup.Success) | ||
| { | ||
| fileName = fileName.Remove(_indexGroup.Index, _indexGroup.Length); | ||
| var indexPosition = _indexGroup.Index; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hidden invariant / Feature Envy — indexPosition = _condGroup.Index; indexLength += _condGroup.Length; silently assumes cond is contiguous and immediately precedes index — an invariant set 60 lines away in ParseFormatString. Uncommented; changing either regex breaks the other. |
||
| var indexLength = _indexGroup.Length; | ||
| if (_condGroup != null && _condGroup.Success) | ||
| { | ||
| indexPosition = _condGroup.Index; | ||
| indexLength += _condGroup.Length; | ||
| } | ||
|
|
||
| fileName = fileName.Remove(indexPosition, indexLength); | ||
|
|
||
| if (!_hideZeroIndex || Index > 0) | ||
| { | ||
| var format = "D" + _indexGroup.Length; | ||
| fileName = fileName.Insert(_indexGroup.Index, Index.ToString(format)); | ||
| fileName = fileName.Insert(indexPosition, Index.ToString(format)); | ||
| if (_hideZeroIndex && _condContent != null) | ||
| { | ||
| fileName = fileName.Insert(_indexGroup.Index, _condContent); | ||
| fileName = fileName.Insert(indexPosition, _condContent); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -141,14 +149,14 @@ public string BuildFileName () | |
| private void ParseFormatString (string formatString) | ||
| { | ||
| var fmt = EscapeNonvarRegions(formatString); | ||
| var datePos = formatString.IndexOf("$D(", StringComparison.Ordinal); | ||
| var datePos = fmt.IndexOf("$D(", StringComparison.Ordinal); | ||
|
|
||
| if (datePos != -1) | ||
| { | ||
| var endPos = formatString.IndexOf(')', datePos); | ||
| var endPos = fmt.IndexOf(')', datePos); | ||
| if (endPos != -1) | ||
| { | ||
| _dateTimeFormat = formatString.Substring(datePos + 3, endPos - datePos - 3) | ||
| _dateTimeFormat = fmt.Substring(datePos + 3, endPos - datePos - 3) | ||
| .ToUpperInvariant() | ||
| .Replace('D', 'd') | ||
| .Replace('Y', 'y'); | ||
|
|
@@ -176,12 +184,15 @@ private void ParseFormatString (string formatString) | |
| } | ||
| } | ||
|
|
||
| fmt = fmt.Replace("*", ".*", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("*", ".*?", StringComparison.Ordinal); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .→.? and :194 \A…\z anchoring change every mask, not just ones with trailing literals. It holds because RolloverFilenameHandler.cs:39 passes a bare filename, not a path — but that invariant is undocumented and untested; a full path with any non-*-prefixed mask now fails where it previously matched. |
||
| _hideZeroIndex = fmt.Contains("$J", StringComparison.Ordinal); | ||
| fmt = fmt.Replace("$I", "(?'index'[\\d]+)", StringComparison.Ordinal); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Allow index tags ($J, $I) to appear before a fixed file extension suffix." RolloverFilenameBuilder.cs:189 still maps $I → (?'index'[\d]+); only $J gets the prefix-aware alternation (:190-193). So *$I.log cannot match the active file app.log — SetFileName fails, IsIndexPattern stays false, no chain is built. The one new $I test (RollingNameTest.cs:36, engine1.log→engine2.log) starts from a numbered file and never exercises the failing case. |
||
| fmt = fmt.Replace("$J", "(?'index'[\\d]*)", StringComparison.Ordinal); | ||
| var optionalIndexPattern = _condContent != null | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. optionalIndexPattern reads as "the pattern is optional"; |
||
| ? $"(?:(?'cond'{Regex.Escape(_condContent)})(?'index'[\\d]+)|(?'index'))" | ||
| : "(?'index'[\\d]*)"; | ||
| fmt = fmt.Replace("$J", optionalIndexPattern, StringComparison.Ordinal); | ||
|
|
||
| _regex = new Regex(fmt); | ||
| _regex = new Regex(@"\A" + fmt + @"\z"); | ||
| } | ||
|
|
||
| private string EscapeNonvarRegions (string formatString) | ||
|
|
@@ -192,15 +203,20 @@ private string EscapeNonvarRegions (string formatString) | |
| StringBuilder result = new(); | ||
| StringBuilder segment = new(); | ||
|
|
||
| void FlushEscaped () | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FlushEscaped() covers only case 0, while case 1 and case 3 still inline _ = result.Append(segment); segment = new StringBuilder();. |
||
| { | ||
| _ = result.Append(Regex.Escape(segment.ToString())); | ||
| segment = new StringBuilder(); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extract it to a real function not a function in a function, this only leads to unreadable code |
||
|
|
||
| for (var i = 0; i < fmt.Length; ++i) | ||
| { | ||
| switch (state) | ||
| { | ||
| case 0: // looking for $ | ||
| if (fmt[i] == '$') | ||
| { | ||
| _ = result.Append(Regex.Escape(segment.ToString())); | ||
| segment = new StringBuilder(); | ||
| FlushEscaped(); | ||
| state = 1; | ||
| } | ||
|
|
||
|
|
@@ -238,9 +254,10 @@ private string EscapeNonvarRegions (string formatString) | |
| } | ||
| } | ||
|
|
||
| FlushEscaped(); | ||
| fmt = result.ToString().Replace('\xFFFD', '*'); | ||
| return fmt; | ||
| } | ||
|
|
||
| #endregion | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1680,12 +1680,16 @@ Ein ausgewähltes Tool erscheint in der Iconbar. Alle anderen verfügbaren Tools | |
| <value>Muster syntax: | ||
|
|
||
| * = alle Zeichen (wildcard) | ||
| $D(&lt;date&gt;) = Datumsmuster | ||
| $D(<date>) = Datumsmuster | ||
| $I = Dateiindexnummer | ||
| $J = Dateiindexnummer, versteckt wenn 09 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still reads $J = Dateiindexnummer, versteckt wenn 09 — should be 0. Correct in the English and Chinese files. |
||
| $J(&lt;prefix&gt;) = Wie $J, jedoch wird ein &lt;prefix&gt; hinzugefügt when es nicht 0 ist | ||
| $J(<prefix>) = Wie $J, jedoch wird ein <prefix> hinzugefügt wenn es nicht 0 ist | ||
|
|
||
| &lt;date&gt;: | ||
| Beispiele: | ||
| *$J(.) → app.log, app.log.1, app.log.2 | ||
| *$J(.).log → app.log, app.1.log, app.2.log | ||
|
|
||
| <date>: | ||
| DD = Tag | ||
| MM = Monat | ||
| YY[YY] = Jahr | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,11 @@ public void TestFilename1(string expectedResult, string formatString) | |
| [TestCase("engine.log", "engine1.log","engine$J.log")] | ||
| [TestCase("engine1.log", "engine2.log","engine$J.log")] | ||
| [TestCase("engine.log", "engine.log.1","*$J(.)")] | ||
| [TestCase("engine.log", "engine.1.log", "*$J(.).log")] | ||
| [TestCase("engine.log.1", "engine.log.2", "*$J(.)")] | ||
| [TestCase("engine.1.log", "engine.2.log", "*$J(.).log")] | ||
| [TestCase("engine1.log", "engine2.log", "*$I.log")] | ||
| [TestCase("engine_2010-06-12.1.log", "engine_2010-06-12.2.log", "*$D(yyyy-MM-dd)$J(.).log")] | ||
| [TestCase("engine_2010-06-12.log", "engine_2010-06-12.log.1", "*$D(yyyy-MM-dd).log$J(.)")] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. $I untested. For "$J, $I" support, the fix itself generalizes, but no test covers $I or $D followed by a trailing literal (e.g. *$D(yyyy-MM-dd)$J(.).log).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added cases which cover:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ParseFormatString computes datePos from the original string but edits the escaped one — a literal with regex metachars before $D (e.g. app.$D(yyyy-MM-dd)) corrupts the regex. Not introduced here, but the new trailing-literal support makes such patterns likelier.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed by locating and parsing $D(...) in the escaped format string. Offsets remain correct when literal regex metacharacters appear before the date placeholder. |
||
| public void TestFilenameAnd1(string fileName, string expectedResult, string formatString) | ||
| { | ||
|
|
@@ -45,6 +50,7 @@ public void TestFilenameAnd1(string fileName, string expectedResult, string form | |
| [Test] | ||
| [TestCase("engine.log", "engine.log.2","*$J(.)")] | ||
| [TestCase("engine.log", "engine.log.2","*.log$J(.)")] | ||
| [TestCase("engine.log", "engine.2.log", "*$J(.).log")] | ||
| public void TestFilenameAnd2(string fileName, string expectedResult, string formatString) | ||
| { | ||
| RolloverFilenameBuilder fnb = new(formatString); | ||
|
|
@@ -54,6 +60,16 @@ public void TestFilenameAnd2(string fileName, string expectedResult, string form | |
| Assert.That(name, Is.EqualTo(expectedResult)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void BuildFileName_DatePatternAfterRegexMetacharacter_IncrementsDate () | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BuildFileName_X_Y while the file's existing methods are TestFilenameAnd1/2. |
||
| { | ||
| RolloverFilenameBuilder fnb = new("app.$D(yyyy-MM-dd).log"); | ||
| fnb.SetFileName("app.2010-06-12.log"); | ||
|
|
||
| fnb.IncrementDate(); | ||
|
|
||
| Assert.That(fnb.BuildFileName(), Is.EqualTo("app.2010-06-13.log")); | ||
| } | ||
|
|
||
| [Test] | ||
| [TestCase("engine1.log", "engine.log","engine$J.log")] | ||
|
|
@@ -65,4 +81,4 @@ public void TestFilenameMinus1(string fileName, string expectedResult, string fo | |
| var name = fnb.BuildFileName(); | ||
| Assert.That(name, Is.EqualTo("engine.log")); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ public void SetUp () | |
|
|
||
| File.Copy(Path.Combine(_testDataDirectory, "app.log"), _logFile); | ||
| File.Copy(Path.Combine(_testDataDirectory, "app.log.1"), _logFile + ".1"); | ||
| File.Copy(Path.Combine(_testDataDirectory, "app.1.log"), Path.Combine(_testDirectory, "app.1.log")); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SetUp copies app.1.log/app.2.log unconditionally, so all four tests now see two extra files though only the new one needs them. |
||
| File.Copy(Path.Combine(_testDataDirectory, "app.2.log"), Path.Combine(_testDirectory, "app.2.log")); | ||
|
|
||
| _ = PluginRegistry.PluginRegistry.Create(_testDirectory, 500); | ||
| } | ||
|
|
@@ -68,6 +70,22 @@ public void SingleFileCtor_MultiFileTrue_ExpandsRollover () | |
| }); | ||
| } | ||
|
|
||
| [Test] | ||
| public void SingleFileCtor_MultiFileTrue_LoadsIndexBeforeExtension () | ||
| { | ||
| var options = new MultiFileOptions { FormatPattern = "*$J(.).log" }; | ||
| using var reader = CreateSingleFileReader(multiFile: true, options); | ||
|
|
||
| reader.ReadFiles(); | ||
|
|
||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(reader.IsMultiFile, Is.True); | ||
| Assert.That(reader.GetLogFileInfoList().Select(file => Path.GetFileName(file.FullName)), | ||
| Is.EqualTo(new[] { "app.2.log", "app.1.log", "app.log" })); | ||
| }); | ||
| } | ||
|
|
||
| [Test] | ||
| public void MultiFileCtor_AlwaysMultiFile () | ||
| { | ||
|
|
@@ -91,15 +109,15 @@ public void MultiFileCtor_AlwaysMultiFile () | |
| }); | ||
| } | ||
|
|
||
| private LogfileReader CreateSingleFileReader (bool multiFile) | ||
| private LogfileReader CreateSingleFileReader (bool multiFile, MultiFileOptions? options = null) | ||
| { | ||
| return new LogfileReader( | ||
| _logFile, | ||
| new EncodingOptions { Encoding = Encoding.UTF8 }, | ||
| multiFile, | ||
| bufferCount: 40, | ||
| linesPerBuffer: 50, | ||
| new MultiFileOptions(), | ||
| options ?? new MultiFileOptions(), | ||
| ReaderType.System, | ||
| PluginRegistry.PluginRegistry.Instance, | ||
| maximumLineLength: 500, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| app.1.log |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| app.2.log |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
A saved mask *$J(.).log previously behaved as *$J(.) and matched app.log.1; it now means app.1.log. That's what the spec wants, but existing user settings change