Add glob-based support for wildcards in CODEOWNERS file parser (disabled, behind a feature flag)

[konrad-jamrozik]

This is a first PR to address:

• Add support for wildcard handling of file matching in codeowners library #2770

Changes made

Changes in CodeOwnersFile.cs:

• In this PR I add a new implementation for matching entries in our CODEOWNERS file. This implementation is disabled, hidden behind a method feature flag parameter.
• The method targeted by this PR is CodeOwnersFile.FindOwnersForClosestMatch. It now either calls its previous, unchanged, legacy implementation, now located in CodeOwnersFile.FindOwnersForClosestMatchLegacyImpl, or the newly introduced implementation. The decision is made here and is based on newly introduced method feature flag parameter, useNewFindOwnersForClosestMatchImpl.
  • The useNewFindOwnersForClosestMatchImpl is by default false, meaning the code uses the legacy implementation. It is only ever set to true in the added tests.
• If useNewFindOwnersForClosestMatchImpl is true, then new implementation is used. That implementation's entry point is the newly-introduced MatchedCodeOwnerEntry ctor.

Newly added MatchedCodeOwnerEntry.cs class:

• This class contains the logic for the new matching logic, as explained above.
• This class leverages Microsoft.Extensions.FileSystemGlobbing (namespace) to reuse support for glob matching of * and ** during path matching. See the class implementation and comments for details.

Newly added Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeOwnersFileTests.cs:

• This is a new project and a test class for testing the implementations of FindOwnersForClosestMatch directly.
• It includes a battery of over 50 test cases that achieve 100% code coverage both on the legacy and new parsing logic.
• The test battery also compares the behavior of the legacy and new matching logic, to be discussed w.r.t. backward compatibility issues and pre-existing bugs in the legacy logic.

Changes to Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj:

• I updated and added some missing dependencies, based on the dependencies created when creating a new NUnit project in visual studio.
  • This includes linter for NUnit, which resulted in some warnings around assertions, which broke PR build. I fixed them.
  • I also enabled strict nullability checking.

Testing done

• Added the battery of tests with 100% coverage both of legacy and new implementation.
• There are also existing tests in Azure.Sdk.Tools.RetrieveCodeOwners.Tests/MainTests.cs.

Next steps

• We need to discuss the discrepancies between legacy and new matching logic, as captured by the battery of tests. Note that this is not a blocker to merging this PR, as the new matching logic is currently unused beyond this test suite.
  • Notably, when the input path is e.g. foo the parser doesn't know if it is a file or a directory. This is a fundamental limitation, because CODEOWNERS entry of form foo/ should match the path only if it is a directory, but not a file. We could work around this problem in two ways:
    • assume all targetPaths are files, always;
    • or assume that a targetPath is a directory if and only if it ends with /.
• As mentioned in the issue Add support for wildcard handling of file matching in codeowners library #2770, we also need to decide what to do with wildcard paths in our language repos that start to match once we enable the new matching logic. For example these paths in azure-sdk-for-net CODEOWNERS.
• Discovered a bug in the used library, have to find a workaround:
  • Microsoft.Extensions.FileSystemGlobbing.Matcher matches glob pattern /*/ against file path /a/b, but it shouldn't. dotnet/runtime#80076
• In a follow-up PR, I am going to flip the feature flag to enable the new matching logic.
  • Once we are confident it works as expected, I am going to make another PR deleting the legacy logic.

Reference

• https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-syntax
• https://git-scm.com/docs/gitignore#_pattern_format
• https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.filesystemglobbing.matcher?view=dotnet-plat-ext-7.0
• https://man7.org/linux/man-pages/man7/glob.7.html

[konrad-jamrozik]

@weshaggard while the new parser implementation in this PR requires a rewrite, I would like to merge this PR to keep it as a reference, e.g. for the bug I reported here:

• Microsoft.Extensions.FileSystemGlobbing.Matcher matches glob pattern /*/ against file path /a/b, but it shouldn't. dotnet/runtime#80076

Is it OK if we merge this? Note the new implementation is not used. I will make a follow-up PR that changes the implementation as we discussed in:

• Implement CODEOWNERS file changes validation (gating), with baselining #4859 (comment)

UPDATE 1/5/2023 8:12 AM PST: here is the new regex-based parser. The PR for it is based on this PR:

• Replace the glob-based CODEOWNERS parser with regex-based #5063

[weshaggard]

Given the new implementation is currently disabled and you want it for the glob reference I'm good with you merging this and then pulling in your change to switch to regex.