FindOwnersBackend: Fix incompatibility of handling globs
The find-owners backend promises to be compatible with the syntax of the
find-owners plugin, but currently the handling of '*' in globs differs.
The find-owners backend in the code-owners plugin currently handles '*'s
in globs like this:
* '*': matches any string that does not include slashes
* '**': matches any string, including slashes
This matches with how globs work in Java [1], however it is incompatible
with the find-owners plugin that handles '*'s in globs like this:
* '*'/'**': matches any string, including slashes
Change the find-owners backend in the code-owners plugin to be
compatible with the find-owners plugin. For this we simply replace any
single '*' character that appears in globs with '**' before we check if
the glob matches.
[1] https://mincong.io/2019/04/16/glob-expression-understanding/
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I1ddc8bf326bbcb8867ffd09eebc7a6739ad7d827
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java b/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java
index ac9ee4d..b0ebb67 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java
@@ -15,6 +15,7 @@
package com.google.gerrit.plugins.codeowners.acceptance.testsuite;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
+import com.google.gerrit.plugins.codeowners.backend.FindOwnersGlobMatcher;
import com.google.gerrit.plugins.codeowners.backend.GlobMatcher;
import com.google.gerrit.plugins.codeowners.backend.PathExpressionMatcher;
import com.google.gerrit.plugins.codeowners.backend.SimplePathExpressionMatcher;
@@ -47,6 +48,7 @@
public String matchFileTypeInCurrentFolder(String fileType) {
PathExpressionMatcher pathExpressionMatcher = getPathExpressionMatcher();
if (pathExpressionMatcher instanceof GlobMatcher
+ || pathExpressionMatcher instanceof FindOwnersGlobMatcher
|| pathExpressionMatcher instanceof SimplePathExpressionMatcher) {
return "*." + fileType;
}
@@ -63,7 +65,8 @@
*/
public String matchAllFilesInSubfolder(String subfolder) {
PathExpressionMatcher pathExpressionMatcher = getPathExpressionMatcher();
- if (pathExpressionMatcher instanceof GlobMatcher) {
+ if (pathExpressionMatcher instanceof GlobMatcher
+ || pathExpressionMatcher instanceof FindOwnersGlobMatcher) {
return subfolder + "/**";
} else if (pathExpressionMatcher instanceof SimplePathExpressionMatcher) {
return subfolder + "/...";
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcher.java b/java/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcher.java
new file mode 100644
index 0000000..2db7b6d
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcher.java
@@ -0,0 +1,80 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.flogger.FluentLogger;
+import java.nio.file.Path;
+
+/**
+ * Glob matcher that is compatible with how globs are interpreted by the {@code find-owners} plugin.
+ *
+ * <p>This matcher has the same behaviour as the {@link GlobMatcher} except that:
+ *
+ * <ul>
+ * <li>'*': matches any string, including slashes (same as '**')
+ * </ul>
+ */
+public class FindOwnersGlobMatcher implements PathExpressionMatcher {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ /** Singleton instance. */
+ public static FindOwnersGlobMatcher INSTANCE = new FindOwnersGlobMatcher();
+
+ /** Private constructor to prevent creation of further instances. */
+ private FindOwnersGlobMatcher() {}
+
+ @Override
+ public boolean matches(String glob, Path relativePath) {
+ String adaptedGlob = replaceSingleStarWithDoubleStar(glob);
+ logger.atFine().log("adapted glob = %s", adaptedGlob);
+ return GlobMatcher.INSTANCE.matches(adaptedGlob, relativePath);
+ }
+
+ /**
+ * Replaces any single '*' in the given glob with '**'. Non-single '*'s, like '**' or '***', stay
+ * unchanged.
+ *
+ * @param glob glob in which any single '*' should be replaced by '**'
+ */
+ @VisibleForTesting
+ String replaceSingleStarWithDoubleStar(String glob) {
+ StringBuilder adaptedGlob = new StringBuilder();
+ Character previousChar = null;
+ boolean maybeSingleStar = false;
+ for (char nextCharacter : glob.toCharArray()) {
+ if (maybeSingleStar && nextCharacter != '*') {
+ // the previous character was a '*' that was not preceded by '*' (maybeSingleStar == true),
+ // since the next character is not '*', we are now sure that the previous character was a
+ // single '*' which should be replaced by '**',
+ // to do this append another '*'
+ adaptedGlob.append('*');
+ }
+ adaptedGlob.append(nextCharacter);
+
+ // the current character may be a single '*' if it's not preceded by '*'
+ maybeSingleStar =
+ nextCharacter == '*' && (previousChar == null || previousChar.charValue() != '*');
+ previousChar = nextCharacter;
+ }
+
+ if (maybeSingleStar) {
+ // the last character was a '*' that was not preceded by '*'
+ adaptedGlob.append('*');
+ }
+
+ return adaptedGlob.toString();
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackend.java b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackend.java
index ab08201..15b4385 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackend.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackend.java
@@ -17,7 +17,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
-import com.google.gerrit.plugins.codeowners.backend.GlobMatcher;
+import com.google.gerrit.plugins.codeowners.backend.FindOwnersGlobMatcher;
import com.google.gerrit.plugins.codeowners.backend.PathExpressionMatcher;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.GerritPersonIdent;
@@ -61,7 +61,7 @@
@Override
public Optional<PathExpressionMatcher> getPathExpressionMatcher() {
- return Optional.of(GlobMatcher.INSTANCE);
+ return Optional.of(FindOwnersGlobMatcher.INSTANCE);
}
@Override
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/BUILD b/javatests/com/google/gerrit/plugins/codeowners/backend/BUILD
index 0a2c144..df2542e 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/BUILD
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/BUILD
@@ -23,7 +23,10 @@
java_library(
name = "testbases",
- srcs = glob(["Abstract*.java"]),
+ srcs = glob([
+ "Abstract*.java",
+ "GlobMatcherTest.java",
+ ]),
deps = [
"//java/com/google/gerrit/acceptance/config",
"//java/com/google/gerrit/common:annotations",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcherTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcherTest.java
new file mode 100644
index 0000000..7e0c03e
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcherTest.java
@@ -0,0 +1,91 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import org.junit.Test;
+
+/** Tests for {@link FindOwnersGlobMatcher}. */
+public class FindOwnersGlobMatcherTest extends GlobMatcherTest {
+ @Override
+ protected PathExpressionMatcher getPathExpressionMatcher() {
+ return FindOwnersGlobMatcher.INSTANCE;
+ }
+
+ @Test
+ public void singleStarInGlobIsReplacedWithDoubleStar() throws Exception {
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("*.md"))
+ .isEqualTo("**.md");
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("foo/*.md"))
+ .isEqualTo("foo/**.md");
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("*/foo/*.md"))
+ .isEqualTo("**/foo/**.md");
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("foo/*"))
+ .isEqualTo("foo/**");
+ }
+
+ @Test
+ public void doubleStarInGlobIsNotReplaced() throws Exception {
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("**.md"))
+ .isEqualTo("**.md");
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("foo/**.md"))
+ .isEqualTo("foo/**.md");
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("**/foo/**.md"))
+ .isEqualTo("**/foo/**.md");
+ }
+
+ @Test
+ public void tripleStarInGlobIsNotReplaced() throws Exception {
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("***.md"))
+ .isEqualTo("***.md");
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("foo/***.md"))
+ .isEqualTo("foo/***.md");
+ assertThat(FindOwnersGlobMatcher.INSTANCE.replaceSingleStarWithDoubleStar("***/foo/***.md"))
+ .isEqualTo("***/foo/***.md");
+ }
+
+ /**
+ * This test differs from the base class ({@link GlobMatcherTest}), since for {@link
+ * FindOwnersGlobMatcher} {@code *} also matches slashes and the test in the base class has an
+ * assertion that checks that slashes are not matched by {@code *}.
+ */
+ @Test
+ @Override
+ public void matchFileTypeInCurrentFolder() throws Exception {
+ String pathExpression = "*.md";
+ assertMatch(pathExpression, "README.md", "config.md");
+ assertNoMatch(pathExpression, "README", "README.md5");
+ }
+
+ @Test
+ public void matchFileTypeInCurrentFolderAndAllSubfoldersBySingleStar() throws Exception {
+ String pathExpression = "*.md";
+ assertMatch(pathExpression, "README.md", "config.md", "foo/README.md", "foo/bar/README.md");
+ assertNoMatch(pathExpression, "README", "README.md5");
+ }
+
+ @Test
+ public void matchAllFilesInSubfolderBySingleStar() throws Exception {
+ String pathExpression = "foo/*";
+ assertMatch(
+ pathExpression,
+ "foo/README.md",
+ "foo/config.txt",
+ "foo/bar/README.md",
+ "foo/bar/baz/README.md");
+ assertNoMatch(pathExpression, "README", "foo2/README");
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackendTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackendTest.java
index 14bb3c3..ff6f184 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackendTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackendTest.java
@@ -19,7 +19,7 @@
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackendTest;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigParser;
-import com.google.gerrit.plugins.codeowners.backend.GlobMatcher;
+import com.google.gerrit.plugins.codeowners.backend.FindOwnersGlobMatcher;
import org.junit.Test;
/** Tests for {@link FindOwnersBackend}. */
@@ -41,6 +41,8 @@
@Test
public void getPathExpressionMatcher() throws Exception {
- assertThat(codeOwnerBackend.getPathExpressionMatcher()).value().isInstanceOf(GlobMatcher.class);
+ assertThat(codeOwnerBackend.getPathExpressionMatcher())
+ .value()
+ .isInstanceOf(FindOwnersGlobMatcher.class);
}
}
diff --git a/resources/Documentation/backend-find-owners-cookbook.md b/resources/Documentation/backend-find-owners-cookbook.md
index 7b49820..2af4e74 100644
--- a/resources/Documentation/backend-find-owners-cookbook.md
+++ b/resources/Documentation/backend-find-owners-cookbook.md
@@ -142,19 +142,18 @@
per-file *.md=tina.toe@example.com
```
\
-To match '*.md' in the current directory and all its subdirectories use:
+This matches all '*.md' in the current directory and all its subdirectories.
-```
- per-file **.md=tina.toe@example.com
-```
-\
+**NOTE**: Using '*.md' is the same as using '**.md' (both expressions match
+files in the current directory and in all subdirectories).
+
**NOTE:** The syntax for path expressions / globs is explained
[here](path-expressions.html#globs).
### <a id="defineCodeOwnersForAllFileInASubdirectory">Define code owners for all files in a subdirectory
It is discouraged to use path expressions that explicitly name subdirectories
-such as `my-subdir/**` as they will break when the subdirectory gets
+such as `my-subdir/*` as they will break when the subdirectory gets
renamed/moved. Instead prefer to define these code owners in `my-subdir/OWNERS`
so that the code owners for the subdirectory stay intact when the subdirectory
gets renamed/moved.
@@ -165,7 +164,7 @@
expression matches all files in the subdirectory:
```
- per-file my-subdir/**=tina.toe@example.com
+ per-file my-subdir/*=tina.toe@example.com
```
### <a id="defineAGroupAsCodeOwner">Define a group as code owner
@@ -322,7 +321,7 @@
`OWNERS` files that exempts all '*.md' files (in the current directory and all
subdirectories) from requiring code owner approvals:
```
- per-file **.md=*
+ per-file *.md=*
```
\
**NOTE:** Files that are not owned by anyone are **not** excluded from requiring
diff --git a/resources/Documentation/backend-find-owners.md b/resources/Documentation/backend-find-owners.md
index b0c0af6..2510e3a 100644
--- a/resources/Documentation/backend-find-owners.md
+++ b/resources/Documentation/backend-find-owners.md
@@ -227,7 +227,8 @@
In the example below, Jana Roe, John Doe and the code owners that are inherited
from parent `OWNERS` files are code owners of all files that are contained in
the directory that contains the `OWNERS` file. In addition Richard Roe is a code
-owner of the `docs.config` file and all `*.md` files in this directory.
+owner of the `docs.config` file in this directory and all `*.md` files in this
+directory and the subdirectories.
```
jane.roe@example.com
@@ -237,7 +238,7 @@
\
##### <a id="doNotUsePathExpressionsForSubdirectories">
**NOTE:** It is discouraged to use path expressions that explicitly name
-subdirectories such as `my-subdir/**` as they will break when the subdirectory
+subdirectories such as `my-subdir/*` as they will break when the subdirectory
gets renamed/moved. Instead prefer to define these code owners in
`my-subdir/OWNERS` so that the code owners for the subdirectory stay intact when
the subdirectory gets renamed/moved.
@@ -268,9 +269,10 @@
from parent directories.
In the example below, Richard Roe is the only code owner of the `docs.config`
-file and all `*.md` files in this directory. All other files in this directory
-and its subdirectories are owned by Jana Roe, John Doe and the code owners that
-are inherited from parent directories.
+file in this directory and all `*.md` files in this directory and the
+subdirectories. All other files in this directory and its subdirectories are
+owned by Jana Roe, John Doe and the code owners that are inherited from parent
+directories.
```
jane.roe@example.com
diff --git a/resources/Documentation/path-expressions.md b/resources/Documentation/path-expressions.md
index 7137d4c..bedbbfe 100644
--- a/resources/Documentation/path-expressions.md
+++ b/resources/Documentation/path-expressions.md
@@ -15,8 +15,7 @@
Globs support the following wildcards:
-* `*`: matches any string that does not include slashes
-* `**`: matches any string, including slashes
+* `*`/`**`: matches any string, including slashes
* `?`: matches any character
* `[abc]`: matches one character given in the bracket
* `[a-c]`: matches one character from the range given in the bracket
