From 506b25bb98a6f7a4610ba05f059a0c9a61c62f0c Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Tue, 7 Feb 2023 20:27:20 -0800 Subject: [PATCH 1/5] valid paths are not commented out paths --- .../CodeOwnersParser/MatchedCodeownersEntry.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs index de5f69117f8..5e635062d58 100644 --- a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs +++ b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs @@ -59,9 +59,13 @@ public class MatchedCodeownersEntry /// See the class comment to understand this method purpose. /// public static bool IsCodeownersPathValid(string codeownersPathExpression) - => !ContainsUnsupportedCharacters(codeownersPathExpression) + => !IsCommentedOutPath(codeownersPathExpression) + && !ContainsUnsupportedCharacters(codeownersPathExpression) && !ContainsUnsupportedSequences(codeownersPathExpression); + private static bool IsCommentedOutPath(string codeownersPathExpression) + => codeownersPathExpression.Trim().StartsWith("#"); + /// /// Any CODEOWNERS path with these characters will be skipped. /// Note these are valid parts of file paths, but we are not supporting From c986e4f5112cfad14218b077f9fa813306e2584a Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Tue, 7 Feb 2023 20:39:05 -0800 Subject: [PATCH 2/5] add comment --- .../CodeOwnersParser/MatchedCodeownersEntry.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs index 5e635062d58..c42f729b6a8 100644 --- a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs +++ b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs @@ -63,9 +63,6 @@ public static bool IsCodeownersPathValid(string codeownersPathExpression) && !ContainsUnsupportedCharacters(codeownersPathExpression) && !ContainsUnsupportedSequences(codeownersPathExpression); - private static bool IsCommentedOutPath(string codeownersPathExpression) - => codeownersPathExpression.Trim().StartsWith("#"); - /// /// Any CODEOWNERS path with these characters will be skipped. /// Note these are valid parts of file paths, but we are not supporting @@ -132,6 +129,16 @@ private CodeownersEntry GetMatchingCodeownersEntry( private CodeownersEntry NoMatchCodeownersEntry { get; } = new CodeownersEntry(); + /// + /// We do not output any error message in case the path is commented out, + /// as a) such paths are expected to be processed and discarded by this logic + /// and b) outputting error messages would possibly result in output + /// truncation, truncating the resulting json, and thus making the output of the + /// calling tool malformed. + /// + private static bool IsCommentedOutPath(string codeownersPathExpression) + => codeownersPathExpression.Trim().StartsWith("#"); + /// /// See the comment on unsupportedChars. /// From b190d4fc30e82917beb8847d27d79a602894c863 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Thu, 9 Feb 2023 18:43:34 -0800 Subject: [PATCH 3/5] ongoing --- .../CodeownersManualAnalysisTests.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs index b734cf0fa13..aa7fe47953f 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs @@ -37,7 +37,7 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests; /// Enable the new, regex-based, wildcard-supporting CODEOWNERS matcher /// https://github.com/Azure/azure-sdk-tools/pull/5088 /// -[TestFixture(Ignore = "Tools to be used manually")] +[TestFixture]//(Ignore = "Tools to be used manually")] public class CodeownersManualAnalysisTests { private const string OwnersDiffOutputPathSuffix = "_owners_diff.csv"; @@ -331,7 +331,7 @@ private static void WriteOwnersToCsv( } // Possible future work: - // instead of returning lines with issues, consider returning the modified & fixed CODEOWNERS file. + // instead of returning lines with issues, consider returning the modified & fixed CODEOWNERS file. // It could work by reading all the lines, then replacing the wrong // lines by using dict replacement. Need to be careful about retaining spaces to not misalign, // e.g. @@ -352,6 +352,7 @@ private static List PathsWithIssues( outputLines.AddRange(PathsWithMissingPrefixSlash(entries)); outputLines.AddRange(PathsWithMissingSuffixSlash(targetDir, entries, paths)); outputLines.AddRange(InvalidPaths(entries)); + // TODO: add a check here for CODEOWNERS paths that do not match any dir or file. return outputLines; } @@ -414,9 +415,13 @@ private static List PathsWithMissingSuffixSlash( private static bool MatchesNamePrefix(string[] paths, string trimmedPathExpression) => paths.Any( path => - path.TrimStart('/').StartsWith(trimmedPathExpression) - && path.TrimStart('/').Length != trimmedPathExpression.Length - && !path.TrimStart('/').Substring(trimmedPathExpression.Length).StartsWith('/')); + { + string trimmedPath = path.TrimStart('/'); + bool pathIsChildDir = trimmedPath.Substring(trimmedPathExpression.Length).StartsWith('/'); + return trimmedPath.StartsWith(trimmedPathExpression) + && trimmedPath.Length != trimmedPathExpression.Length + && !pathIsChildDir; + }); private static bool MatchesDirExactly(string targetDir, string trimmedPathExpression) { From e134db145eedc1a5671bd82c9c3e72ecdef4eb56 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Thu, 9 Feb 2023 19:37:40 -0800 Subject: [PATCH 4/5] fix bug --- .../CodeownersManualAnalysisTests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs index aa7fe47953f..c472844d416 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs @@ -417,7 +417,9 @@ private static bool MatchesNamePrefix(string[] paths, string trimmedPathExpressi path => { string trimmedPath = path.TrimStart('/'); - bool pathIsChildDir = trimmedPath.Substring(trimmedPathExpression.Length).StartsWith('/'); + bool pathIsChildDir = trimmedPath.Contains("/") + && trimmedPath.Length > trimmedPathExpression.Length + && trimmedPath.Substring(trimmedPathExpression.Length).StartsWith('/'); return trimmedPath.StartsWith(trimmedPathExpression) && trimmedPath.Length != trimmedPathExpression.Length && !pathIsChildDir; From 2087fab3e78bada9d55bf59241c923db7b54e9b6 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Fri, 10 Feb 2023 11:31:07 -0800 Subject: [PATCH 5/5] Update tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs --- .../CodeownersManualAnalysisTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs index c472844d416..b206011cfb2 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs @@ -37,7 +37,7 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests; /// Enable the new, regex-based, wildcard-supporting CODEOWNERS matcher /// https://github.com/Azure/azure-sdk-tools/pull/5088 /// -[TestFixture]//(Ignore = "Tools to be used manually")] +[TestFixture(Ignore = "Tools to be used manually")] public class CodeownersManualAnalysisTests { private const string OwnersDiffOutputPathSuffix = "_owners_diff.csv";