Accept per-file with multiple globs and emails.
* Enhance Parser regular expressions, and reuse them in OwnersValidator.
* Add more unit test cases with multiple globs and emails.
* Reject not yet implemented "per-file glob = set noparent" syntax.
Change-Id: I289926b2516196e84651a83f7cfa2f6f461ca9e4
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
index 866a2e7..8f2b262 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
@@ -51,8 +51,6 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
import org.eclipse.jgit.diff.RawText;
import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.lib.ObjectId;
@@ -230,17 +228,6 @@
}
}
- // Line patterns accepted by Parser.java in the find-owners plugin.
- static final Pattern patComment = Pattern.compile("^ *(#.*)?$");
- static final Pattern patEmail = // email address or a "*"
- Pattern.compile("^ *([^ <>@]+@[^ <>@#]+|\\*) *(#.*)?$");
- static final Pattern patFile = Pattern.compile("^ *file:.*$");
- static final Pattern patNoParent = Pattern.compile("^ *set +noparent *(#.*)?$");
- static final Pattern patPerFileNoParent =
- Pattern.compile("^ *per-file +([^= ]+) *= *set +noparent *(#.*)?$");
- static final Pattern patPerFileEmail =
- Pattern.compile("^ *per-file +([^= ]+) *= *([^ <>@]+@[^ <>@#]+|\\*) *(#.*)?$");
-
private static void collectEmail(
Map<String, Set<String>> map, String email, String file, int lineNumber) {
if (!email.equals("*")) {
@@ -268,17 +255,18 @@
String path,
int lineNumber,
String line) {
- Matcher m;
- if (patComment.matcher(line).find()
- || patNoParent.matcher(line).find()
- || patPerFileNoParent.matcher(line).find()) {
- return;
- } else if ((m = patEmail.matcher(line)).find()) {
- collectEmail(email2lines, m.group(1), path, lineNumber);
- } else if ((m = patPerFileEmail.matcher(line)).find()) {
- collectEmail(email2lines, m.group(2).trim(), path, lineNumber);
+ String email;
+ String[] emails;
+ if (Parser.isComment(line) || Parser.isNoParent(line)) {
+ // no email address to check
+ } else if ((email = Parser.parseEmail(line)) != null) {
+ collectEmail(email2lines, email, path, lineNumber);
+ } else if ((emails = Parser.parsePerFileEmails(line)) != null) {
+ for (String e : emails) {
+ collectEmail(email2lines, e, path, lineNumber);
+ }
} else {
- String prefix = patFile.matcher(line).find() ? "ignored" : "syntax";
+ String prefix = Parser.isFile(line) ? "ignored" : "syntax";
add(messages, prefix + ": " + path + ":" + lineNumber + ": " + line, true);
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java
index 57f137d..1264e78 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java
@@ -28,31 +28,90 @@
* <p>OWNERS file syntax:
*
* <pre>
- * lines := (\s* line? \s* "\n")*
- * line := "set noparent"
- * | "per-file" \s+ glob \s* "=" \s* directive
- * | "file:" glob
- * | comment
- * | directive
- * directive := email_address
- * | "*"
- * glob := [a-zA-Z0-9_-*?]+
- * comment := "#" [^"\n"]*
+ * lines := (\s* line? \s* "\n")*
+ * line := "set" \s+ "noparent"
+ * | "per-file" \s+ globs \s* "=" \s* directives
+ * | "file:" \s* glob
+ * | comment
+ * | directive
+ * directives := directive (comma directive)*
+ * directive := email_address
+ * | "*"
+ * globs := glob (comma glob)*
+ * glob := [a-zA-Z0-9_-*?.]+
+ * comma := \s* "," \s*
+ * comment := "#" [^"\n"]*
* </pre>
*
* <p>The "file:" directive is not implemented yet.
*
- * <p>"per-file glob = directive" applies directive only to files matching glob. glob does not
- * contain directory path.
+ * <p>"per-file globs = directives" applies each directive to files matching any of the globs.
+ * A glob does not contain directory path.
*/
class Parser {
- static final Pattern patComment = Pattern.compile("^ *(#.*)?$");
+ // Globs and emails are separated by commas with optional spaces around a comma.
+ protected static final String COMMA = "[\\s]*,[\\s]*"; // used in unit tests
+
+ private static final String BOL = "^[\\s]*"; // begin-of-line
+ private static final String EOL = "[\\s]*(#.*)?$"; // end-of-line
+ private static final String GLOB = "[^\\s,=]+"; // a file glob
+
// TODO: have a more precise email address pattern.
- static final Pattern patEmail = // email address or a "*"
- Pattern.compile("^ *([^ <>@]+@[^ <>@#]+|\\*) *(#.*)?$");
- static final Pattern patFile = Pattern.compile("^ *file:.*$");
- static final Pattern patNoParent = Pattern.compile("^ *set +noparent(( |#).*)?$");
- static final Pattern patPerFile = Pattern.compile("^ *per-file +([^= ]+) *= *([^#]+).*$");
+ private static final String EMAIL_OR_STAR = "([^\\s<>@,]+@[^\\s<>@#,]+|\\*)";
+
+ // Simple input lines with 0 or 1 sub-pattern.
+ private static final Pattern patComment = Pattern.compile(BOL + EOL);
+ private static final Pattern patEmail = Pattern.compile(BOL + EMAIL_OR_STAR + EOL);
+ private static final Pattern patFile = Pattern.compile(BOL + "file:.*" + EOL);
+ private static final Pattern patNoParent = Pattern.compile(BOL + "set[\\s]+noparent" + EOL);
+
+ // Patterns to match trimmed globs and emails in per-file lines.
+ private static final Pattern patEmailList =
+ Pattern.compile("^(" + EMAIL_OR_STAR + "(" + COMMA + EMAIL_OR_STAR + ")*)$");
+ private static final Pattern patGlobs =
+ Pattern.compile("^(" + GLOB + "(" + COMMA + GLOB + ")*)$");
+ // patPerFile matches a line to two groups: (1) globs, (2) emails
+ // Trimmed 1st group should match patGlobs; trimmed 2nd group should match patEmailList.
+ private static final Pattern patPerFile =
+ Pattern.compile(BOL + "per-file[\\s]+([^=#]+)=[\\s]*([^#]+)" + EOL);
+
+ static boolean isComment(String line) {
+ return patComment.matcher(line).matches();
+ }
+
+ static boolean isFile(String line) {
+ return patFile.matcher(line).matches();
+ }
+
+ static boolean isGlobs(String line) {
+ return patGlobs.matcher(line).matches();
+ }
+
+ static boolean isNoParent(String line) {
+ return patNoParent.matcher(line).matches();
+ }
+
+ static String parseEmail(String line) {
+ Matcher m = Parser.patEmail.matcher(line);
+ return m.matches() ? m.group(1).trim() : null;
+ }
+
+ static String[] parsePerFile(String line) {
+ Matcher m = patPerFile.matcher(line);
+ if (!m.matches() || !isGlobs(m.group(1).trim())
+ || !patEmailList.matcher(m.group(2).trim()).matches()) {
+ return null;
+ }
+ String[] parts = new String[2];
+ parts[0] = m.group(1).trim();
+ parts[1] = m.group(2).trim();
+ return parts;
+ }
+
+ static String[] parsePerFileEmails(String line) {
+ String[] globsAndEmails = parsePerFile(line);
+ return (globsAndEmails != null) ? globsAndEmails[1].split(COMMA) : null;
+ }
static class Result {
boolean stopLooking; // if this file contains set noparent
@@ -88,28 +147,23 @@
*/
static void parseLine(Result result, String dir, String file, String line, int num) {
// comment and file: directive are parsed but ignored.
- if (patNoParent.matcher(line).find()) {
+ String email;
+ String[] globsAndEmails;
+ if (isNoParent(line)) {
result.stopLooking = true;
- } else if (patPerFile.matcher(line).find()) {
- Matcher m = patPerFile.matcher(line);
- m.find();
- parseDirective(result, dir + m.group(1), file, m.group(2).trim(), num);
- } else if (patFile.matcher(line).find()) {
- result.warnings.add(warningMsg(file, num, "ignored", line));
- } else if (patComment.matcher(line).find()) {
+ } else if (isComment(line)) {
// ignore comment and empty lines.
- } else {
- parseDirective(result, dir, file, line, num);
- }
- }
-
- private static void parseDirective(
- Result result, String pathGlob, String file, String line, int num) {
- // A directive is an email address or "*".
- if (patEmail.matcher(line).find()) {
- Matcher m = patEmail.matcher(line);
- m.find();
- Util.addToMap(result.owner2paths, m.group(1), pathGlob);
+ } else if ((email = parseEmail(line)) != null) {
+ Util.addToMap(result.owner2paths, email, dir);
+ } else if ((globsAndEmails = parsePerFile(line)) != null) {
+ String[] emails = globsAndEmails[1].split(COMMA);
+ for (String glob : globsAndEmails[0].split(COMMA)) {
+ for (String e : emails) {
+ Util.addToMap(result.owner2paths, e, dir + glob);
+ }
+ }
+ } else if (isFile(line)) {
+ result.warnings.add(warningMsg(file, num, "ignored", line));
} else {
result.errors.add(errorMsg(file, num, "ignored unknown line", line));
}
diff --git a/src/main/resources/Documentation/syntax.md b/src/main/resources/Documentation/syntax.md
index 54db99f..0cf3ae7 100644
--- a/src/main/resources/Documentation/syntax.md
+++ b/src/main/resources/Documentation/syntax.md
@@ -7,23 +7,27 @@
The syntax is:
```java
-lines := (SPACE* line? SPACE* EOL)*
-line := "set noparent"
- | "per-file" SPACE+ glob SPACE* "=" SPACE* directive
- | comment
- | directive
-directive := email
- | "*"
-glob := [a-zA-Z0-9_-*?]+
-comment := "#" ANYCHAR*
-email := [^ @]+@[^ #]+
-ANYCHAR := any character but EOL
-EOL := end of line characters
-SPACE := any white space character
+lines := (SPACE* line? SPACE* EOL)*
+line := "set" SPACE+ "noparent"
+ | "per-file" SPACE+ globs SPACE* "=" SPACE* directives
+ | comment
+ | directive
+directives := directive (SPACE* "," SPACE* directive)*
+directive := email
+ | "*"
+globs := glob (SPACE* "," SPACE* glob)*
+glob := [a-zA-Z0-9_-*?.]+
+comment := "#" ANYCHAR*
+email := [^ @]+@[^ #]+
+ANYCHAR := any character but EOL
+EOL := end of line characters
+SPACE := any white space character
```
-* `per-file glob = directive` applies `directive` only to files
- matching `glob`, which does not contain directory path.
+* `per-file globs = directives` applies each `directive` only to files
+ matching any of the `glob`.
+
+* A 'glob' does not contain directory path.
* The email addresses should belong to registered Gerrit users.
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
index b8d445a..3403bcc 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
@@ -135,17 +135,18 @@
+ " user1@google.com # comment\n"
+ "u1+review@g.com###\n"
+ " * # everyone can approve\n"
- + "per-file *.py = set noparent###\n"
- + "per-file *.py=u2.m@g.com\n"
- + "per-file *.txt = * # everyone can approve\n"
- + "set noparent # comment\n");
+ + "per-file *.py=u2.m@g.com \n"
+ + "per-file *.c, *.java ,A*bp = u1@g.com, u1+review@g.com ,u2.m@g.com#comment\n"
+ + " per-file *.txt = * # everyone can approve # \n"
+ + " set noparent # comment#\n");
private static final ImmutableSet<String> EXPECTED_VERBOSE_OUTPUT =
ImmutableSet.of(
"MSG: validate: " + OWNERS,
- "MSG: owner: user1@google.com",
"MSG: owner: u1+review@g.com",
- "MSG: owner: u2.m@g.com");
+ "MSG: owner: u1@g.com",
+ "MSG: owner: u2.m@g.com",
+ "MSG: owner: user1@google.com");
@Test
public void testGoodInput() throws Exception {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java
index 6f0ca72..bd50f0a 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
+import java.util.Arrays;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -119,27 +120,40 @@
@Test
public void perFileGoodDirectiveTest() {
- String[] directives = {"abc@google.com#comment", " *# comment", " xyz@gmail.com # comment"};
- String[] emails = {"abc@google.com", "*", "xyz@gmail.com"};
+ String[] directives = {
+ "abc@google.com#comment", " *# comment", " xyz@gmail.com # comment",
+ "a@g.com , xyz@gmail.com , * # comment", "*,*#comment", " a@b,c@d "
+ };
+ String[] globsList = {"*", "*,*.c", " *test*.java , *.cc, *.cpp ", "*.bp,*.mk ,A* "};
for (int i = 0; i < directives.length; i++) {
- String line = "per-file *test*.java=" + directives[i];
- Parser.Result result = testLine(line);
- String[] paths = result.owner2paths.get(emails[i]).toArray(new String[1]);
- assertThat(paths.length).isEqualTo(1);
- assertThat(paths[0]).isEqualTo(mockedTestDir() + "*test*.java");
+ for (String globs : globsList) {
+ String line = "per-file " + globs + "=" + directives[i];
+ Parser.Result result = testLine(line);
+ String[] emailList = directives[i].replaceAll("#.*$", "").trim().split(Parser.COMMA);
+ String[] globList = globs.trim().split(Parser.COMMA);
+ Arrays.sort(globList);
+ for (String email : emailList) {
+ String[] paths = result.owner2paths.get(email).toArray(new String[1]);
+ assertThat(paths.length).isEqualTo(globList.length);
+ Arrays.sort(paths);
+ for (int g = 0; g < globList.length; g++) {
+ assertThat(paths[g]).isEqualTo(mockedTestDir() + globList[g]);
+ }
+ }
+ }
}
}
@Test
public void perFileBadDirectiveTest() {
String[] directives = {
- "file://OWNERS", " ** ", "a b@c .co", "a@b@c #com", "a.<b>@zc#", " set noparent "
+ "file://OWNERS", " ** ", "a b@c .co", "a@b@c #com", "a.<b>@zc#", " set noparent ",
+ " , a@b ", "a@b, , c@d #"
};
- String[] errors = {"file://OWNERS", "**", "a b@c .co", "a@b@c", "a.<b>@zc", "set noparent"};
for (int i = 0; i < directives.length; i++) {
String line = "per-file *test*.c=" + directives[i];
Parser.Result result = testLine(line);
- String expected = testLineErrorMsg(errors[i]);
+ String expected = testLineErrorMsg(line);
assertThat(result.warnings).isEmpty();
assertThat(result.errors).hasSize(1);
assertThat(result.errors.get(0)).isEqualTo(expected);
