Google Git
Sign in
gerrit/plugins/code-owners/728efba819f4f89b317193bcf15a6c45c3656afe^1..728efba819f4f89b317193bcf15a6c45c3656afe/.
commit
728efba819f4f89b317193bcf15a6c45c3656afe
[log]
author
Edwin Kempin <ekempin@google.com>
Fri Jan 29 14:06:32 2021 +0000
committer
Gerrit Code Review <noreply-gerritcodereview@google.com>
Fri Jan 29 14:06:32 2021 +0000
tree
bda5697bcd29ff9ee2b8765362fef972486c668e
parent
9f36254628ea172c648ee4ed14045aebde406cd4 [diff]
parent
1e8d992caa6d8b6c181c65e33f5492db9bc747f9 [diff]
Merge changes I5552c999,I04931606

* changes:
  config.md: Fix link to config guide
  FindOwnersBackend: Fix incompatibility of handling globs

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/config.md b/resources/Documentation/config.md
index aa9c064..9c99988 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md

@@ -4,7 +4,7 @@
 file in the `plugin.@PLUGIN@` subsection.
 
 This page describes all available configuration parameters. For configuration
-recommendations please consult the [config guide](#config-guide.html).
+recommendations please consult the [config guide](config-guide.html).
 
 ## <a id="projectLevelConfigFile">
 In addition some configuration can be done on the project level in

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
 
 ---