Merge changes I409485ea,Ia330dbbd * changes: Ignore invalid globs in OWNERS files Fix parsing of globs that contain commas
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java index e42501d..4e381d7 100644 --- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java +++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersIT.java
@@ -78,7 +78,7 @@ protected ChangeCodeOwnersFactory changeCodeOwnersApiFactory; protected ProjectCodeOwnersFactory projectCodeOwnersApiFactory; - private BackendConfig backendConfig; + protected BackendConfig backendConfig; @Before public void baseSetup() throws Exception {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/GlobMatcher.java b/java/com/google/gerrit/plugins/codeowners/backend/GlobMatcher.java index 23a1132..4d4f6a7 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/GlobMatcher.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/GlobMatcher.java
@@ -17,6 +17,7 @@ import com.google.common.flogger.FluentLogger; import java.nio.file.FileSystems; import java.nio.file.Path; +import java.util.regex.PatternSyntaxException; /** * Matcher that checks for a given path expression as Java NIO glob if it matches a given path. @@ -47,9 +48,15 @@ @Override public boolean matches(String glob, Path relativePath) { - boolean isMatching = - FileSystems.getDefault().getPathMatcher("glob:" + glob).matches(relativePath); - logger.atFine().log("path %s %s matching %s", relativePath, isMatching ? "is" : "is not", glob); - return isMatching; + try { + boolean isMatching = + FileSystems.getDefault().getPathMatcher("glob:" + glob).matches(relativePath); + logger.atFine().log( + "path %s %s matching %s", relativePath, isMatching ? "is" : "is not", glob); + return isMatching; + } catch (PatternSyntaxException e) { + logger.atFine().log("glob %s is invalid: %s", glob, e.getMessage()); + return false; + } } }
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java index 5ddbb9a..788e921 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
@@ -126,7 +126,8 @@ return Joiner.on("\n").join(updatedLines); } - private static class Parser implements ValidationError.Sink { + @VisibleForTesting + static class Parser implements ValidationError.Sink { private static final String COMMA = "[\\s]*,[\\s]*"; // Separator for project and file paths in an include line. @@ -241,7 +242,7 @@ String[] globsAndOwners = new String[] {removeExtraSpaces(m.group(1)), removeExtraSpaces(m.group(2))}; - String[] dirGlobs = globsAndOwners[0].split(COMMA, -1); + String[] dirGlobs = splitGlobs(globsAndOwners[0]); String directive = globsAndOwners[1]; if (directive.equals(TOK_SET_NOPARENT)) { return CodeOwnerSet.builder() @@ -266,6 +267,58 @@ .build(); } + /** + * Splits the given glob string by the commas that separate the globs. + * + * <p>Commas that appear within a glob do not cause the string to be split at this position: + * + * <ul> + * <li>commas that are used as separator when matching choices via {@code {choice1,choice2}} + * <li>commas that appears as part of a character class via {@code + * [<any-chars-including-comma>]} + * </ul> + * + * @param commaSeparatedGlobs globs as comma-separated list + * @return the globs as array + */ + @VisibleForTesting + static String[] splitGlobs(String commaSeparatedGlobs) { + ArrayList<String> globList = new ArrayList<>(); + StringBuilder nextGlob = new StringBuilder(); + int curlyBracesIndentionLevel = 0; + int squareBracesIndentionLevel = 0; + for (int i = 0; i < commaSeparatedGlobs.length(); i++) { + char c = commaSeparatedGlobs.charAt(i); + if (c == ',') { + if (curlyBracesIndentionLevel == 0 && squareBracesIndentionLevel == 0) { + globList.add(nextGlob.toString()); + nextGlob = new StringBuilder(); + } else { + nextGlob.append(c); + } + } else { + nextGlob.append(c); + if (c == '{') { + curlyBracesIndentionLevel++; + } else if (c == '}') { + if (curlyBracesIndentionLevel > 0) { + curlyBracesIndentionLevel--; + } + } else if (c == '[') { + squareBracesIndentionLevel++; + } else if (c == ']') { + if (squareBracesIndentionLevel > 0) { + squareBracesIndentionLevel--; + } + } + } + } + if (nextGlob.length() > 0) { + globList.add(nextGlob.toString()); + } + return globList.toArray(new String[globList.size()]); + } + private static boolean isComment(String line) { return PAT_COMMENT.matcher(line).matches(); }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java index 5d63b73..fef1205 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -15,6 +15,7 @@ package com.google.gerrit.plugins.codeowners.acceptance.api; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerStatusInfoSubject.assertThat; import static com.google.gerrit.plugins.codeowners.testing.LegacySubmitRequirementInfoSubject.assertThatCollection; import static com.google.gerrit.testing.GerritJUnit.assertThrows; @@ -38,6 +39,8 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT; import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatusInfo; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet; +import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend; import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus; import com.google.gerrit.plugins.codeowners.testing.LegacySubmitRequirementInfoSubject; import com.google.gerrit.plugins.codeowners.util.JgitPath; @@ -488,6 +491,50 @@ } @Test + public void changeIsSubmittableIfOwnersFileContainsInvalidPathExpression() throws Exception { + assume().that(backendConfig.getDefaultBackend()).isInstanceOf(FindOwnersBackend.class); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/") + .addCodeOwnerEmail(admin.email()) + .addCodeOwnerSet( + CodeOwnerSet.builder() + .addPathExpression("{foo") // invalid because '{' is not closed + .addCodeOwnerEmail(user.email()) + .build()) + .create(); + + PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content"); + String changeId = r.getChangeId(); + + // Apply Code-Review+2 to satisfy the MaxWithBlock function of the Code-Review label. + approve(changeId); + + ChangeInfo changeInfo = + gApi.changes() + .id(changeId) + .get( + ListChangesOption.SUBMITTABLE, + ListChangesOption.ALL_REVISIONS, + ListChangesOption.CURRENT_ACTIONS); + assertThat(changeInfo.submittable).isTrue(); + + // Check the submit requirement. + LegacySubmitRequirementInfoSubject submitRequirementInfoSubject = + assertThatCollection(changeInfo.requirements).onlyElement(); + submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK"); + submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners"); + submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners"); + + // Submit the change. + gApi.changes().id(changeId).current().submit(); + assertThat(gApi.changes().id(changeId).get().status).isEqualTo(ChangeStatus.MERGED); + } + + @Test @GerritConfig( name = "plugin.code-owners.mergeCommitStrategy", value = "FILES_WITH_CONFLICT_RESOLUTION")
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractCodeOwnerConfigParserTest.java index 59a2050..3a44ed7 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractCodeOwnerConfigParserTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractCodeOwnerConfigParserTest.java
@@ -366,6 +366,25 @@ } @Test + public void setCodeOwnerSetsWithGlobPathExpression() throws Exception { + CodeOwnerSet codeOwnerSet1 = CodeOwnerSet.createWithoutPathExpressions(EMAIL_1, EMAIL_3); + CodeOwnerSet codeOwnerSet2 = + CodeOwnerSet.builder() + .addPathExpression("{foo,bar}/**/baz[1-5]/a[,]b*.md") + .addCodeOwnerEmail(EMAIL_2) + .build(); + assertParseAndFormat( + getCodeOwnerConfig(false, codeOwnerSet1, codeOwnerSet2), + codeOwnerConfig -> { + assertThat(codeOwnerConfig) + .hasCodeOwnerSetsThat() + .containsExactly(codeOwnerSet1, codeOwnerSet2) + .inOrder(); + }, + getCodeOwnerConfig(false, codeOwnerSet1, codeOwnerSet2)); + } + + @Test public void setMultipleCodeOwnerSetsWithPathExpressions() throws Exception { CodeOwnerSet codeOwnerSet1 = CodeOwnerSet.builder()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/GlobMatcherTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/GlobMatcherTest.java index 5291bb7..73f525d 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/GlobMatcherTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/GlobMatcherTest.java
@@ -82,4 +82,10 @@ assertMatch(pathExpression2, "foo-1.txt", "foo-2.txt"); assertNoMatch(pathExpression2, "foo-5.txt", "foo-11.txt", "sub/foo-3.txt", "sub/sub/foo-4.txt"); } + + @Test + public void invalidPattern() throws Exception { + // the path expressions is invalid because '{' is not closed + assertNoMatch("{foo-[1-4].txt", "foo-1.txt", "foo-2.txt", "foo-5.txt", "foo-11.txt"); + } }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java index 6499fa7..53c986d 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
@@ -753,4 +753,40 @@ assertThat(FindOwnersCodeOwnerConfigParser.replaceEmail(content + "\n", oldEmail, newEmail)) .isEqualTo(expectedContent + "\n"); } + + @Test + public void splitGlobs() throws Exception { + // empty globs + assertSplitGlobs(""); + assertSplitGlobs(",", ""); + + // single globs + assertSplitGlobs("BUILD", "BUILD"); + assertSplitGlobs("*.md", "*.md"); + assertSplitGlobs("foo/*", "foo/*"); + assertSplitGlobs("{foo,bar}", "{foo,bar}"); + assertSplitGlobs("{foo,bar}/**", "{foo,bar}/**"); + assertSplitGlobs("{{foo,bar}}", "{{foo,bar}}"); + assertSplitGlobs("foo[1-5]", "foo[1-5]"); + assertSplitGlobs("a[,]b", "a[,]b"); + assertSplitGlobs("a[[,]]b", "a[[,]]b"); + + // multiple globs + assertSplitGlobs("BUILD,*.md,foo/*", "BUILD", "*.md", "foo/*"); + assertSplitGlobs( + "{foo,bar},{foo,bar}/**,{{foo,bar}}", "{foo,bar}", "{foo,bar}/**", "{{foo,bar}}"); + assertSplitGlobs("foo[1-5],a[,]b,a[[,]]b", "foo[1-5]", "a[,]b", "a[[,]]b"); + assertSplitGlobs("a[,]b,{foo,bar}", "a[,]b", "{foo,bar}"); + + // invalid globs + assertSplitGlobs("{foo,bar", "{foo,bar"); + assertSplitGlobs("[abc,", "[abc,"); + assertSplitGlobs("{foo,bar,a[,]b", "{foo,bar,a[,]b"); + } + + private static void assertSplitGlobs(String commaSeparatedGlobs, String... expectedGlobs) { + assertThat(FindOwnersCodeOwnerConfigParser.Parser.splitGlobs(commaSeparatedGlobs)) + .asList() + .containsExactlyElementsIn(expectedGlobs); + } }