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 globs differs.
The find-owners plugin ignores subdirectories when checking if a glob
matches a file.
Examples:
1. the glob "*.md" in "/" matches "foo.md", "bar/foo.md" and
"baz/bar/foo.md"
2. the glob "BUILD" in "/" matches "BUILD", "bar/BUILD" and
"baz/bar/BUILD"
3. the glob "foo-[1-4].txt" in "/" matches "foo-1.txt", "bar/foo-1.txt"
and "baz/bar/foo-1.txt"
This is because the find-owners plugin iterates over all parent
directories [1,2] to see if there is a glob that matches the
filename [3]. E.g. if the glob "*.md" is defined at '/' and the file
path is "baz/bar/foo.md" then it does the following checks:
a. Is there a glob at "./baz/bar" that matches file "./baz/bar/foo.md" (no)
b. Is there a glob at "./baz" that matches file "./baz/foo.md" (no)
c. Is there a glob at "./" that matches file "./foo.md" (yes)
Effectively this means that subdirectories are ignored (although the
actual file is "./baz/bar/foo.md" the find-owners plugin checks the glob
against "./foo.md").
To be compatible with this behaviour the code-owners plugin needs to
ignore subdirectories too when checking if a file matches a glob.
Instead of iterating over the parent directories for each file as it's
done in the find-owners plugin, we simply prefix all globs with "{**/,}"
("{**/,}" matches "**/" or "", which matches any subdirectory and no
subdirectory).
Change I1ddc8bf32 was a first attempt to fix this by replacing "*" with
"**" in globs, but this only fixed case 1 (and not the cases 2 and 3).
[1] https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/master/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java#367
[2] https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/master/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java#396
[3] https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/master/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java#375
Bug: Issue 14008
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I04931606858511fb1c36c449a4ce3f2300eabecf
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcher.java b/java/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcher.java
index 2db7b6d..650ae50 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcher.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcher.java
@@ -14,8 +14,6 @@
package com.google.gerrit.plugins.codeowners.backend;
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.flogger.FluentLogger;
import java.nio.file.Path;
/**
@@ -28,8 +26,6 @@
* </ul>
*/
public class FindOwnersGlobMatcher implements PathExpressionMatcher {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
/** Singleton instance. */
public static FindOwnersGlobMatcher INSTANCE = new FindOwnersGlobMatcher();
@@ -38,43 +34,7 @@
@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();
+ // always match files in all subdirectories
+ return GlobMatcher.INSTANCE.matches("{**/,}" + glob, relativePath);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcherTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcherTest.java
index 7e0c03e..8696f19 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcherTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/FindOwnersGlobMatcherTest.java
@@ -14,8 +14,6 @@
package com.google.gerrit.plugins.codeowners.backend;
-import static com.google.common.truth.Truth.assertThat;
-
import org.junit.Test;
/** Tests for {@link FindOwnersGlobMatcher}. */
@@ -25,67 +23,68 @@
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 *}.
+ * This test differs from the base class ({@link GlobMatcherTest}), since {@link
+ * FindOwnersGlobMatcher} matches globs against any subdirectory and the test in the base class
+ * checks that subdirectories are not matched.
*/
@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 {
+ public void matchFileType() throws Exception {
String pathExpression = "*.md";
assertMatch(pathExpression, "README.md", "config.md", "foo/README.md", "foo/bar/README.md");
assertNoMatch(pathExpression, "README", "README.md5");
}
+ /**
+ * This test differs from the base class ({@link GlobMatcherTest}), since {@link
+ * FindOwnersGlobMatcher} matches globs against any subdirectory and the test in the base class
+ * checks that subdirectories are not matched.
+ */
@Test
- public void matchAllFilesInSubfolderBySingleStar() throws Exception {
- String pathExpression = "foo/*";
+ @Override
+ public void matchConcreteFile() throws Exception {
+ String pathExpression = "BUILD";
+ assertMatch(pathExpression, "BUILD", "foo/BUILD", "foo/bar/BUILD");
+ assertNoMatch(pathExpression, "README", "BUILD2");
+ }
+
+ /**
+ * This test differs from the base class ({@link GlobMatcherTest}), since {@link
+ * FindOwnersGlobMatcher} matches globs against any subdirectory and the test in the base class
+ * checks that subdirectories are not matched.
+ */
+ @Test
+ @Override
+ public void matchAllFilesInSubfolder() 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");
+ "foo/bar/baz/README.md",
+ "bar/foo/README.md",
+ "bar/foo/config.txt",
+ "bar/foo/bar/README.md",
+ "bar/foo/bar/baz/README.md");
+ assertNoMatch(pathExpression, "README", "foo2/README", "bar/README", "bar/foo2/README");
+ }
+
+ /**
+ * This test differs from the base class ({@link GlobMatcherTest}), since {@link
+ * FindOwnersGlobMatcher} matches globs against any subdirectory and the test in the base class
+ * checks that subdirectories are not matched.
+ */
+ @Test
+ @Override
+ public void matchPattern() throws Exception {
+ String pathExpression = "{**/,}foo-[1-4].txt";
+ assertMatch(pathExpression, "foo-1.txt", "foo-2.txt", "sub/foo-3.txt", "sub/sub/foo-4.txt");
+ assertNoMatch(pathExpression, "foo-5.txt", "foo-11.txt");
+
+ String pathExpression2 = "foo-[1-4].txt";
+ assertMatch(pathExpression2, "foo-1.txt", "foo-2.txt", "sub/foo-3.txt", "sub/sub/foo-4.txt");
+ assertNoMatch(pathExpression2, "foo-5.txt", "foo-11.txt");
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/GlobMatcherTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/GlobMatcherTest.java
index 75affd9..5291bb7 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/GlobMatcherTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/GlobMatcherTest.java
@@ -24,17 +24,17 @@
}
@Test
- public void matchConcreteFileInCurrentFolder() throws Exception {
+ public void matchConcreteFile() throws Exception {
String pathExpression = "BUILD";
assertMatch(pathExpression, "BUILD");
- assertNoMatch(pathExpression, "README", "BUILD2", "foo/BUILD");
+ assertNoMatch(pathExpression, "README", "BUILD2", "foo/BUILD", "foo/bar/BUILD");
}
@Test
- public void matchFileTypeInCurrentFolder() throws Exception {
+ public void matchFileType() throws Exception {
String pathExpression = "*.md";
assertMatch(pathExpression, "README.md", "config.md");
- assertNoMatch(pathExpression, "README", "README.md5", "foo/README.md");
+ assertNoMatch(pathExpression, "README", "README.md5", "foo/README.md", "foo/bar/README.md");
}
@Test
@@ -60,7 +60,16 @@
"foo/config.txt",
"foo/bar/README.md",
"foo/bar/baz/README.md");
- assertNoMatch(pathExpression, "README", "foo2/README");
+ assertNoMatch(
+ pathExpression,
+ "README",
+ "foo2/README",
+ "bar/README",
+ "bar/foo2/README",
+ "bar/foo/README.md",
+ "bar/foo/config.txt",
+ "bar/foo/bar/README.md",
+ "bar/foo/bar/baz/README.md");
}
@Test
@@ -68,5 +77,9 @@
String pathExpression = "{**/,}foo-[1-4].txt";
assertMatch(pathExpression, "foo-1.txt", "foo-2.txt", "sub/foo-3.txt", "sub/sub/foo-4.txt");
assertNoMatch(pathExpression, "foo-5.txt", "foo-11.txt");
+
+ String pathExpression2 = "foo-[1-4].txt";
+ 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");
}
}
diff --git a/resources/Documentation/path-expressions.md b/resources/Documentation/path-expressions.md
index 322d757..e5c2dd8 100644
--- a/resources/Documentation/path-expressions.md
+++ b/resources/Documentation/path-expressions.md
@@ -7,15 +7,18 @@
Which syntax is used depends on the used code owner backend:
-* [find-owners](backend-find-owners.html) backend: uses [globs](#globs)
-* [proto](backend-proto.html) backend: uses
- [simple path expressions](#simplePathExpressions)
+* [find-owners](backend-find-owners.html) backend:
+ uses [globs](#globs), but each glob is automatically prefixed with `{**/,}`
+ so that subfolders are always matched (e.g. `*.md` matches all md files in all
+ subfolders, rather then only md files in the current folder)
+* [proto](backend-proto.html) backend:
+ uses [simple path expressions](#simplePathExpressions)
## <a id="globs">Globs
Globs support the following wildcards:
-* `*`/`**`: 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
@@ -34,15 +37,15 @@
## <a id="examples">Examples
-| To Match | Glob | Simple Path Expression |
-| -------- | ---- | ---------------------- |
-| Concrete file in current folder | `BUILD` | `BUILD` |
-| File type in current folder | not possible | `*.md` |
-| Concrete file in the current folder and in all subfolders | `{**/,}BUILD` | needs 2 expressions: `BUILD` + `.../BUILD` |
-| File type in the current folder and in all subfolders | `**.md` or `*.md` | `....md` |
-| All files in a subfolder | `my-folder/**` | `my-folder/...` |
-| All “foo-<1-digit-number>.txt” files in all subfolders | `{**/,}foo-[0-9].txt` | not possible |
-| All “foo-<n-digit-number>.txt” files in all subfolders | not possible | not possible |
+| To Match | Glob | find-owners | Simple Path Expression |
+| -------- | ---- | ----------- | ---------------------- |
+| Concrete file in current folder | `BUILD` | not possible | `BUILD` |
+| File type in current folder | `*.md` | not possible | `*.md` |
+| Concrete file in the current folder and in all subfolders | `{**/,}BUILD` | `BUILD` | needs 2 expressions: `BUILD` + `.../BUILD` |
+| File type in the current folder and in all subfolders | `**.md` | `*.md` or `**.md` | `....md` |
+| All files in a subfolder | `my-folder/**` | not possible, but you can add a `my-folder/OWNERS` file instead of using a glob | `my-folder/...` |
+| All “foo-<1-digit-number>.txt” files in all subfolders | `{**/,}foo-[0-9].txt` | `foo-[0-9].txt` |not possible |
+| All “foo-<n-digit-number>.txt” files in all subfolders | not possible | not possible | not possible
---
