Implement "file:..." statement and directive
* See semantics and examples in updated syntax.md.
* Add new feature:
* Accept new file: statement/directive in OwnersValidator.java
but still not checking the content of included files.
* Parser.FILE_DIRECTIVE defined as one pattern used in per-file.
* Add Parser.IncludeStack to keep track of included project:file.
* Factor out Parser.includeFile to handle all "include" and "file:"
statements or directives.
* Reduce repeated inclusion and parsing:
* Keep readFiles in OwnersDb to read one repository file only once.
* Add Parser.savedResults to avoid parsing a file multiple times.
* Improve log and error messages:
* Shorten log message: () for empty content, (...) for non-empty.
* Keep Parser warnings and errors as sets to avoid duplication.
Sort all unique warnings and errors before dump to logger.
* Rename parseLine:skip messages to
parseLine:errorRecursion and parseLine:useSaved
* Add documents and tests:
* Add more JavaDoc comments to Parser.java.
* Update syntax.md to include "file:" statement and directive.
* Add test cases in FindOwnersIT, ParserTest, OwnersValidatorTest.
* Reduce dependency on myProjectName
Change-Id: I235bbdd70ecaded4b20767eac0abbe27e6061b01
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
index bf5a2c7..2ae18de 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -16,6 +16,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.collect.Ordering;
import com.google.common.collect.Multimap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.reviewdb.client.Account;
@@ -100,13 +101,17 @@
// Some hacked CL could have a target branch that is not created yet.
ObjectId id = getBranchId(repo, branch, changeData, logs);
revision = "";
+ // For the same repo and branch id, keep content of all read files to avoid
+ // repeated read. This cache of files should be passed down to the Parser to
+ // avoid reading the same file through "include" or "file:" statements.
+ Map<String, String> readFiles = new HashMap<>();
if (id != null) {
if (!ownersFileName.equals(Config.OWNERS) && branch.equals("refs/heads/master")) {
// If ownersFileName is not the default "OWNERS", and current branch is master,
// this project should have a non-empty root file of that name.
// We added this requirement to detect errors in project config files
// and Gerrit server bugs that return wrong value of "ownersFileName".
- String content = getFile(repo, id, "/" + ownersFileName, logs);
+ String content = getFile(readFiles, repo, projectName, id, "/" + ownersFileName, logs);
String found = "Found";
if (content.isEmpty()) {
String changeId = Config.getChangeId(changeData);
@@ -122,13 +127,20 @@
fileName = Util.addDotPrefix(fileName); // e.g. "./" "./d1/f1" "./d2/d3/"
String dir = Util.getParentDir(fileName); // e.g. "." "./d1" "./d2"
logs.add("findOwnersFileFor:" + fileName);
+ // Multiple changed files can be in one directory, but each directory
+ // is only searched once for an OWNERS file.
+ // However any file (including another OWNERS file) can be included
+ // by OWNERS files in different directories. In that case, the included
+ // file could be parsed multiple times for different "dir".
+ // Since open/read a Gerrit repository file could be slow, getFile should keep
+ // a copy of all read files to avoid repeated accesses of the same file.
while (!readDirs.contains(dir)) {
readDirs.add(dir);
logs.add("findOwnersFileIn:" + dir);
String filePath = dir + "/" + ownersFileName;
- String content = getFile(repo, id, filePath, logs);
+ String content = getFile(readFiles, repo, projectName, id, filePath, logs);
if (content != null && !content.isEmpty()) {
- addFile(projectName, branch, dir + "/", dir + "/" + ownersFileName,
+ addFile(readFiles, projectName, branch, dir + "/", dir + "/" + ownersFileName,
content.split("\\R"));
}
if (stopLooking.contains(dir + "/") || !dir.contains("/")) {
@@ -220,8 +232,9 @@
}
}
- void addFile(String project, String branch, String dirPath, String filePath, String[] lines) {
- Parser parser = new Parser(repoManager, project, branch, filePath, logs);
+ void addFile(Map<String, String> readFiles, String project, String branch,
+ String dirPath, String filePath, String[] lines) {
+ Parser parser = new Parser(readFiles, repoManager, project, branch, filePath, logs);
Parser.Result result = parser.parseFile(dirPath, lines);
if (result.stopLooking) {
stopLooking.add(dirPath);
@@ -238,8 +251,8 @@
add2dir2Globs(Util.getDirName(glob) + "/", glob);
}
if (config.getReportSyntaxError()) {
- result.warnings.forEach(w -> logger.atWarning().log(w));
- result.errors.forEach(w -> logger.atSevere().log(w));
+ Ordering.natural().sortedCopy(result.errors).forEach(e -> logger.atSevere().log(e));
+ Ordering.natural().sortedCopy(result.warnings).forEach(w -> logger.atWarning().log(w));
}
}
@@ -378,46 +391,71 @@
return null;
}
+ private static String findReadFile(Map<String, String> readFiles, String project, String file) {
+ String key = Parser.getFileKey(project, file);
+ if (readFiles != null && readFiles.get(key) != null) {
+ return readFiles.get(key);
+ }
+ return null;
+ }
+
+ private static void saveReadFile(
+ Map<String, String> readFiles, String project, String file, String content) {
+ if (readFiles != null) {
+ readFiles.put(Parser.getFileKey(project, file), content);
+ }
+ }
+
/** Returns file content or empty string; uses project+branch+file names. */
- public static String getRepoFile(GitRepositoryManager repoManager,
+ public static String getRepoFile(Map<String, String> readFiles, GitRepositoryManager repoManager,
String project, String branch, String file, List<String> logs) {
// 'file' must be an absolute path from the root of 'project'.
- logs.add("getRepoFile:" + project + ":" + branch + ":" + file);
- if (repoManager != null) { // ParserTest can call with null repoManager
- try (Repository repo = repoManager.openRepository(new Project.NameKey(project))) {
- ObjectId id = repo.resolve(branch);
- if (id != null) {
- return getFile(repo, id, file, logs);
+ logs.add("getRepoFile:" + Parser.getFileKey(project, branch, file));
+ file = Util.gitRepoFilePath(file);
+ String content = findReadFile(readFiles, project, file);
+ if (content == null) {
+ content = "";
+ if (repoManager != null) { // ParserTest can call with null repoManager
+ try (Repository repo = repoManager.openRepository(new Project.NameKey(project))) {
+ ObjectId id = repo.resolve(branch);
+ if (id != null) {
+ return getFile(readFiles, repo, project, id, file, logs);
+ }
+ logs.add("getRepoFile not found branch " + branch);
+ } catch (Exception e) {
+ logger.atSevere().log("getRepoFile failed to find repository of project %s", project);
+ logException(logs, "getRepoFile", e);
}
- logs.add("getRepoFile not found branch " + branch);
- } catch (Exception e) {
- logger.atSevere().log("getRepoFile failed to find repository of project %s", project);
- logException(logs, "getRepoFile", e);
}
}
- return "";
+ return content;
}
/** Returns file content or empty string; uses Repository. */
- private static String getFile(
- Repository repo, ObjectId id, String file, List<String> logs) {
+ private static String getFile(Map<String, String> readFiles,
+ Repository repo, String project, ObjectId id, String file, List<String> logs) {
file = Util.gitRepoFilePath(file);
- String header = "getFile:" + file;
- try (RevWalk revWalk = new RevWalk(repo)) {
- RevTree tree = revWalk.parseCommit(id).getTree();
- ObjectReader reader = revWalk.getObjectReader();
- TreeWalk treeWalk = TreeWalk.forPath(reader, file, tree);
- if (treeWalk != null) {
- String content = new String(reader.open(treeWalk.getObjectId(0)).getBytes(), UTF_8);
- logs.add(header + ":" + content);
- return content;
+ String content = findReadFile(readFiles, project, file);
+ if (content == null) {
+ content = "";
+ try (RevWalk revWalk = new RevWalk(repo)) {
+ String header = "getFile:" + file;
+ RevTree tree = revWalk.parseCommit(id).getTree();
+ ObjectReader reader = revWalk.getObjectReader();
+ TreeWalk treeWalk = TreeWalk.forPath(reader, file, tree);
+ if (treeWalk != null) {
+ content = new String(reader.open(treeWalk.getObjectId(0)).getBytes(), UTF_8);
+ logs.add(header + ":(...)");
+ } else {
+ logs.add(header + " (NOT FOUND)");
+ }
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log("get file %s", file);
+ logException(logs, "getFile", e);
}
- logs.add(header + " (NOT FOUND)");
- } catch (Exception e) {
- logger.atSevere().withCause(e).log("get file %s", file);
- logException(logs, "getFile", e);
+ saveReadFile(readFiles, project, file, content);
}
- return "";
+ return content;
}
/** Adds a header + exception message to the logs. */
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 c58a9ef..c36afcb 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
@@ -244,6 +244,13 @@
messages.add(new CommitValidationMessage(msg, error));
}
+ private static void checkIncludeOrFile(
+ List<CommitValidationMessage> messages, String path, int num, String line) {
+ // TODO: Check if an included file exists and that it has valid syntax.
+ // An included file could be a new file added by a CL and not in the repository yet
+ add(messages, "unchecked: " + path + ":" + num + ": " + Parser.getIncludeOrFile(line), false);
+ }
+
private static void checkLine(
List<CommitValidationMessage> messages,
Map<String, Set<String>> email2lines,
@@ -258,17 +265,14 @@
collectEmail(email2lines, email, path, lineNumber);
} else if ((emails = Parser.parsePerFileOwners(line)) != null) {
for (String e : emails) {
- if (!e.equals(Parser.TOK_SET_NOPARENT)) {
+ if (e.startsWith("file:")) {
+ checkIncludeOrFile(messages, path, lineNumber, line);
+ } else if (!e.equals(Parser.TOK_SET_NOPARENT)) {
collectEmail(email2lines, e, path, lineNumber);
}
}
} else if (Parser.isInclude(line)) {
- // Included "OWNERS" files will be checked by themselves.
- // TODO: Check if the include file path is valid and existence of the included file.
- // TODO: Check an included file syntax if it is not named as the project ownersFileName.
- add(messages, "unchecked: " + path + ":" + lineNumber + ": " + line, false);
- } else if (Parser.isFile(line)) {
- add(messages, "ignored: " + path + ":" + lineNumber + ": " + line, true);
+ checkIncludeOrFile(messages, path, lineNumber, line);
} else {
add(messages, "syntax: " + 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 0bf1cca..b26d1a9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java
@@ -19,6 +19,7 @@
import java.io.IOException;
import java.util.ArrayDeque;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
@@ -30,8 +31,17 @@
/**
* Parse lines in an OWNERS file and put them into an OwnersDb.
+ * One Parser object should be created to parse only one OWNERS file.
+ * It keeps repoManager, project, branch, and filePath of the OWNERS
+ * file so it can find files that are included by OWNERS.
*
- * The syntax, semantics, and some examples are included in syntax.md.
+ * The usage pattern is:
+ * Parser parser = new Parser(repoManager, project, branch, repoFilePath);
+ * String content = OwnersDb.getRepoFile(readFiles, repoManager, project,
+ * branch, repoFilePath, logs);
+ * Parser.Result result = parser.parseFile(dirPath, content);
+ *
+ * OWNERS file syntax, semantics, and examples are included in syntax.md.
*/
class Parser {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -58,23 +68,25 @@
private static final String PROJECT_NAME = "([^\\s:]+" + COLUMN + ")?";
// A relative or absolute file path name without any column or space character.
- private static final String FILE_PATH = "([^\\s:]+)";
+ private static final String FILE_PATH = "([^\\s:#]+)";
private static final String PROJECT_AND_FILE = PROJECT_NAME + FILE_PATH;
private static final String SET_NOPARENT = "set[\\s]+noparent";
+ private static final String FILE_DIRECTIVE = "file:[\\s]*" + PROJECT_AND_FILE;
+ private static final String INCLUDE_OR_FILE = "(file:[\\s]*|include[\\s]+)";
+
// Simple input lines with 0 or 1 sub-pattern.
private static final Pattern PAT_COMMENT = Pattern.compile(BOL + EOL);
private static final Pattern PAT_EMAIL = Pattern.compile(BOL + EMAIL_OR_STAR + EOL);
- private static final Pattern PAT_FILE = Pattern.compile(BOL + "file:.*" + EOL);
private static final Pattern PAT_INCLUDE =
- Pattern.compile(BOL + "include[\\s]+" + PROJECT_AND_FILE + EOL);
+ Pattern.compile(BOL + INCLUDE_OR_FILE + PROJECT_AND_FILE + EOL);
private static final Pattern PAT_NO_PARENT = Pattern.compile(BOL + SET_NOPARENT + EOL);
// Patterns to match trimmed globs, emails, and set noparent in per-file lines.
private static final Pattern PAT_PER_FILE_OWNERS =
- Pattern.compile("^(" + EMAIL_LIST + "|(" + SET_NOPARENT + "))$");
+ Pattern.compile("^(" + EMAIL_LIST + "|" + SET_NOPARENT + "|" + FILE_DIRECTIVE + ")$");
private static final Pattern PAT_GLOBS =
Pattern.compile("^(" + GLOB + "(" + COMMA + GLOB + ")*)$");
// PAT_PER_FILE matches a line to two groups: (1) globs, (2) emails
@@ -82,41 +94,80 @@
// trimmed 2nd group should match PAT_PER_FILE_OWNERS.
private static final Pattern PAT_PER_FILE =
Pattern.compile(BOL + "per-file[\\s]+([^=#]+)=[\\s]*([^#]+)" + EOL);
+ // Fetch the include/file part of a line with correct syntax.
+ private static final Pattern PAT_INCLUDE_OR_FILE =
+ Pattern.compile("^.*(" + INCLUDE_OR_FILE + PROJECT_AND_FILE + ")" + EOL);
- // A parser keeps current repoManager, project, branch, included file path, and debug/trace logs.
+ // A parser keeps current readFiles, repoManager, project, branch,
+ // included file path, and debug/trace logs.
+ private Map<String, String> readFiles;
private GitRepositoryManager repoManager;
private String branch; // All owners files are read from the same branch.
- private Deque<String[]> includeStack; // Keeps include stack of [projectName,filePath].
- private Set<String> included; // Keeps included files of projectName:filePath
+ private IncludeStack stack; // a stack of including files.
private List<String> logs; // Keeps debug/trace messages.
+ private Map<String, Result> savedResults; // projectName:filePath => Parser.Result
- Parser(GitRepositoryManager repoManager, String project, String branch, String file) {
- init(repoManager, project, branch, file, new ArrayList<>());
+ static class IncludeStack {
+ Deque<String> projectName; // project/repository name of included file
+ Deque<String> filePath; // absolute or relative path of included file
+ Set<String> allFiles; // to detect recursive inclusion quickly
+
+ IncludeStack(String project, String file) {
+ projectName = new ArrayDeque<>();
+ filePath = new ArrayDeque<>();
+ allFiles = new HashSet<>();
+ push(project, file);
+ }
+
+ void push(String project, String file) {
+ projectName.push(project);
+ filePath.push(file);
+ allFiles.add(getFileKey(project, file));
+ }
+
+ void pop() {
+ allFiles.remove(getFileKey(currentProject(), currentFile()));
+ projectName.pop();
+ filePath.pop();
+ }
+
+ String currentProject() {
+ return projectName.peek();
+ }
+
+ String currentFile() {
+ return filePath.peek();
+ }
+
+ boolean contains(String project, String file) {
+ return allFiles.contains(getFileKey(project, file));
+ }
}
- Parser(GitRepositoryManager repoManager,
- String project, String branch, String file, List<String> logs) {
- init(repoManager, project, branch, file, logs);
+ Parser(Map<String, String> readFiles, GitRepositoryManager repoManager,
+ String project, String branch, String file) {
+ init(readFiles, repoManager, project, branch, file, new ArrayList<>());
}
- private void init(GitRepositoryManager repoManager,
+ Parser(Map<String, String> readFiles, GitRepositoryManager repoManager,
String project, String branch, String file, List<String> logs) {
+ init(readFiles, repoManager, project, branch, file, logs);
+ }
+
+ private void init(Map<String, String> readFiles, GitRepositoryManager repoManager,
+ String project, String branch, String file, List<String> logs) {
+ this.readFiles = readFiles;
this.repoManager = repoManager;
this.branch = branch;
this.logs = logs;
- includeStack = new ArrayDeque<>();
- included = new HashSet<>();
- pushProjectFilePaths(project, normalizedRepoDirFilePath(".", file));
+ stack = new IncludeStack(project, normalizedRepoDirFilePath(".", file));
+ savedResults = new HashMap<>();
}
static boolean isComment(String line) {
return PAT_COMMENT.matcher(line).matches();
}
- static boolean isFile(String line) {
- return PAT_FILE.matcher(line).matches();
- }
-
static boolean isInclude(String line) {
return PAT_INCLUDE.matcher(line).matches();
}
@@ -139,18 +190,22 @@
if (!m.matches()) {
return null;
}
- String[] parts = new String[]{m.group(1), m.group(2).trim()};
- if (parts[0] != null && parts[0].length() > 0) {
- // parts[0] has project name followed by ':'
- parts[0] = parts[0].split(COLUMN, -1)[0].trim();
- } else {
- parts[0] = project; // default project name
+ String keyword = m.group(1).trim();
+ if (keyword.equals("file:")) {
+ keyword = "file";
}
- return parts;
+ String projectName = m.group(2);
+ if (projectName != null && projectName.length() > 1) {
+ // PROJECT_NAME ends with ':'
+ projectName = projectName.split(COLUMN, -1)[0].trim();
+ } else {
+ projectName = project; // default project name
+ }
+ return new String[]{keyword, projectName, m.group(3).trim()};
}
static String removeExtraSpaces(String s) {
- return s.trim().replaceAll("[\\s]+", " ");
+ return s.trim().replaceAll("[\\s]+", " ").replaceAll("[\\s]*:[\\s]*", ":");
}
static String[] parsePerFile(String line) {
@@ -167,37 +222,60 @@
return (globsAndOwners != null) ? globsAndOwners[1].split(COMMA, -1) : null;
}
+ static String getIncludeOrFile(String line) {
+ Matcher m = PAT_INCLUDE_OR_FILE.matcher(line);
+ return m.matches() ? removeExtraSpaces(m.group(1)) : "";
+ }
+
static class Result {
boolean stopLooking; // if this file contains set noparent
- List<String> warnings; // warning messages
- List<String> errors; // error messages
+ Set<String> warnings; // unique warning messages
+ Set<String> errors; // unique error messages
Map<String, Set<String>> owner2paths; // maps from owner email to pathGlobs
Set<String> noParentGlobs; // per-file dirpath+glob with "set noparent"
Result() {
stopLooking = false;
- warnings = new ArrayList<>();
- errors = new ArrayList<>();
+ warnings = new HashSet<>();
+ errors = new HashSet<>();
owner2paths = new HashMap<>();
noParentGlobs = new HashSet<>();
}
- void append(Result r) {
+ void append(Result r, String dir, boolean addAll) {
+ // addAll is true when the Result is from an include statements.
+ // It is false for the included result of "file:" directive, which
+ // only collects owner emails, not per-file or set noparent statement.
warnings.addAll(r.warnings);
errors.addAll(r.errors);
- stopLooking |= r.stopLooking; // included file's "set noparent" applies to the including file
- for (String key : r.owner2paths.keySet()) {
- Util.addAllToMap(owner2paths, key, r.owner2paths.get(key));
+ if (addAll) {
+ stopLooking = stopLooking || r.stopLooking;
+ for (String glob : r.noParentGlobs) {
+ noParentGlobs.add(dir + glob);
+ }
}
- noParentGlobs.addAll(r.noParentGlobs);
+ for (String key : r.owner2paths.keySet()) {
+ for (String path : r.owner2paths.get(key)) {
+ // In an included file, top-level owener emails have empty dir path.
+ if (path.isEmpty() || addAll) {
+ Util.addToMap(owner2paths, key, dir + path);
+ }
+ }
+ }
}
}
- // Parse an OWNERS file or included file content in lines.
- // "dir" is the directory that contains "changed files" of a CL,
- // not necessarily the OWNERS or included file directory.
- // "owners" listed in lines control changed files in 'dir' not
- // necessrily the files in the directory containing "lines".
+ /**
+ * Parse given lines of an OWNERS files; return parsed Result.
+ * It can recursively call itself to parse included files.
+ *
+ * @param dir is the directory that contains "changed files" of a CL,
+ * not necessarily the OWNERS or included file directory.
+ * "owners" found in lines control changed files in 'dir'.
+ * 'dir' ends with '/' or is empty when parsing an included file.
+ * @param lines are the source lines of the file to be parsed.
+ * @return the parsed data
+ */
Result parseFile(String dir, String[] lines) {
Result result = new Result();
int n = 0;
@@ -207,25 +285,8 @@
return result;
}
- private String currentProject() {
- return includeStack.peek()[0];
- }
-
- private String currentFilePath() {
- return includeStack.peek()[1];
- }
-
- private String combineProjectAndFile(String project, String file) {
- return project + ":" + file;
- }
-
- private void pushProjectFilePaths(String project, String file) {
- includeStack.push(new String[]{project, file});
- included.add(combineProjectAndFile(project, file));
- }
-
- private void popProjectFilePaths() {
- includeStack.pop();
+ Result parseFile(String dir, String content) {
+ return parseFile(dir, content.split("\\R"));
}
private String normalizedRepoDirFilePath(String dir, String path) {
@@ -240,7 +301,8 @@
}
/**
- * Parse a line in OWNERS file and add info to OwnersDb.
+ * Parse a line in OWNERS file and add parsed info into result.
+ * This function should be called only by parseFile and Parser unit tests.
*
* @param result a Result object to keep parsed info.
* @param dir the path to OWNERS file directory.
@@ -250,56 +312,111 @@
void parseLine(Result result, String dir, String line, int num) {
String email;
String[] globsAndOwners;
- String[] projectAndFile;
+ String[] parsedKPF; // parsed keyword, projectName, filePath
if (isNoParent(line)) {
result.stopLooking = true;
} else if (isComment(line)) {
// ignore comment and empty lines.
} else if ((email = parseEmail(line)) != null) {
- Util.addToMap(result.owner2paths, email, dir);
+ Util.addToMap(result.owner2paths, email, dir); // here dir is not a glob
} else if ((globsAndOwners = parsePerFile(line)) != null) {
- String[] owners = globsAndOwners[1].split(COMMA, -1);
- for (String glob : globsAndOwners[0].split(COMMA, -1)) {
- for (String e : owners) {
- if (e.equals(Parser.TOK_SET_NOPARENT)) {
- result.noParentGlobs.add(dir + glob);
- } else {
- Util.addToMap(result.owner2paths, e, dir + glob);
+ String[] dirGlobs = globsAndOwners[0].split(COMMA, -1);
+ String directive = globsAndOwners[1];
+ if (directive.equals(Parser.TOK_SET_NOPARENT)) {
+ // per-file globs = set noparent
+ for (String glob : dirGlobs) {
+ result.noParentGlobs.add(dir + glob);
+ }
+ } else {
+ List<String> ownerEmails;
+ if ((parsedKPF = parseInclude(stack.currentProject(), directive)) == null) {
+ // per-file globs = ownerEmails
+ ownerEmails = Arrays.asList(directive.split(COMMA, -1));
+ } else {
+ // per-file globs = file: projectFile
+ ownerEmails = new ArrayList<>();
+ Result r = new Result();
+ includeFile(r, "", num, parsedKPF, false);
+ for (String key : r.owner2paths.keySet()) {
+ for (String path : r.owner2paths.get(key)) {
+ if (path.isEmpty()) {
+ ownerEmails.add(key);
+ break;
+ }
+ }
+ }
+ }
+ for (String glob : dirGlobs) {
+ for (String owner: ownerEmails) {
+ Util.addToMap(result.owner2paths, owner, dir + glob);
}
}
}
- } else if (isFile(line)) {
- // file: directive is parsed but ignored.
- result.warnings.add(warningMsg(currentFilePath(), num, "ignored", line));
- logs.add("parseLine:file");
- } else if ((projectAndFile = parseInclude(currentProject(), line)) != null) {
- String project = projectAndFile[0];
- String file = projectAndFile[1];
- String includePath = combineProjectAndFile(project, file);
- // Like C/C++ #include, when f1 includes f2 with a relative path projectAndFile[1],
- // use f1's directory, not 'dir', as the base for relative path.
- // 'dir' is the directory of OWNERS file, which might include f1 indirectly.
- String repoFile = normalizedRepoDirFilePath(Util.getParentDir(currentFilePath()), file);
- String repoProjectFile = combineProjectAndFile(project, repoFile);
- if (included.contains(repoProjectFile)) {
- logs.add("parseLine:skip:include:" + includePath);
- } else {
- pushProjectFilePaths(project, repoFile);
- logs.add("parseLine:include:" + includePath);
- String content =
- OwnersDb.getRepoFile(repoManager, project, branch, repoFile, logs);
- if (content != null && !content.isEmpty()) {
- result.append(parseFile(dir, content.split("\\R")));
- } else {
- logs.add("parseLine:include:(empty)");
- }
- popProjectFilePaths();
- }
+ } else if ((parsedKPF = parseInclude(stack.currentProject(), line)) != null) {
+ includeFile(result, dir, num, parsedKPF, parsedKPF[0].equals("include"));
} else {
- result.errors.add(errorMsg(currentFilePath(), num, "ignored unknown line", line));
+ result.errors.add(errorMsg(stack.currentFile(), num, "ignored unknown line", line));
}
}
+ /**
+ * Find and parse an included file and append data to the 'result'.
+ * For an 'include' statement, parsed data is all appended to the given result parameter.
+ * For a 'file:' statement or directive, only owner emails are appended.
+ * If the project+file name is found in the stored result set, the stored result is reused.
+ * The inclusion is skipped if the to be included file is already on the including file stack.
+ *
+ * @param result to where the included file data should be added.
+ * @param dir the including file's directory or glob.
+ * @param num source code line number
+ * @param parsedKPF the parsed line of include or file directive.
+ * @param addAll to add all parsed data into result or not.
+ */
+ private void includeFile(Result result, String dir, int num, String[] parsedKPF, boolean addAll) {
+ String keyword = parsedKPF[0];
+ String project = parsedKPF[1];
+ String file = parsedKPF[2];
+ String includeKPF = keyword + ":" + getFileKey(project, file);
+ // Like C/C++ #include, when f1 includes f2 with a relative file path,
+ // use f1's directory, not 'dir', as the base for relative path.
+ // 'dir' is the directory of OWNERS file, which might include f1 indirectly.
+ String repoFile = normalizedRepoDirFilePath(Util.getParentDir(stack.currentFile()), file);
+ if (stack.contains(project, repoFile)) {
+ logs.add("parseLine:errorRecursion:" + includeKPF);
+ result.errors.add(errorMsg(stack.currentFile(), num, "recursive include", includeKPF));
+ return;
+ }
+ String savedResultKey = getFileKey(project, repoFile);
+ Result includedFileResult = savedResults.get(savedResultKey);
+ if (null != includedFileResult) {
+ logs.add("parseLine:useSaved:" + includeKPF);
+ } else {
+ stack.push(project, repoFile);
+ logs.add("parseLine:" + includeKPF);
+ String content =
+ OwnersDb.getRepoFile(readFiles, repoManager, project, branch, repoFile, logs);
+ if (content != null && !content.isEmpty()) {
+ includedFileResult = parseFile("", content);
+ } else {
+ logs.add("parseLine:" + keyword + ":()");
+ includedFileResult = new Result();
+ }
+ stack.pop();
+ savedResults.put(savedResultKey, includedFileResult);
+ }
+ result.append(includedFileResult, dir, addAll);
+ }
+
+ // Build a readable key or output string for a (project, file) pair.
+ static String getFileKey(String project, String file) {
+ return project + ":" + file;
+ }
+
+ // Build a readable key or output string for a (project, branch, file) tuple.
+ static String getFileKey(String project, String branch, String file) {
+ return project + ":" + branch + ":" + file;
+ }
+
private static String createMsgLine(String prefix, String file, int n, String msg, String line) {
return prefix + file + ":" + n + ": " + msg + ": [" + line + "]";
}
diff --git a/src/main/resources/Documentation/syntax.md b/src/main/resources/Documentation/syntax.md
index 1470ed2..ac1637e 100644
--- a/src/main/resources/Documentation/syntax.md
+++ b/src/main/resources/Documentation/syntax.md
@@ -7,26 +7,30 @@
### Syntax
```java
-lines := (SPACE* line? SPACE* EOL)*
-line := comment
- | noparent
- | ownerEmail
- | "per-file" SPACE+ globs SPACE* "=" SPACE* owners
- | "include" SPACE+ (project SPACE* ":" SPACE*)? filePath
-comment := "#" ANYCHAR*
-noparent := "set" SPACE+ "noparent"
-ownerEmail := email
- | "*"
-owners := noparent
- | ownerEmail (SPACE* "," SPACE* ownerEmail)*
-globs := glob (SPACE* "," SPACE* glob)*
-glob := [a-zA-Z0-9_-*?.]+
-email := [^ @]+@[^ #]+
-project := a Gerrit project name without space or column character
-filePath := a file path name without space or column character
-ANYCHAR := any character but EOL
-EOL := end of line characters
-SPACE := any white space character
+lines := (SPACE* line? SPACE* EOL)*
+line := comment
+ | noparent
+ | ownerEmail
+ | "per-file" SPACE+ globs SPACE* "=" SPACE* owners
+ | include projectFile
+include := "include" SPACE+
+ | "file:" SPACE*
+projectFile := (project SPACE* ":" SPACE*)? filePath
+comment := "#" ANYCHAR*
+noparent := "set" SPACE+ "noparent"
+ownerEmail := email
+ | "*"
+owners := noparent
+ | "file:" SPACE* projectFile
+ | ownerEmail (SPACE* "," SPACE* ownerEmail)*
+globs := glob (SPACE* "," SPACE* glob)*
+glob := [a-zA-Z0-9_-*?.]+
+email := [^ @]+@[^ #]+
+project := a Gerrit project name without space or column character
+filePath := a file path name without space or column character
+ANYCHAR := any character but EOL
+EOL := end of line characters
+SPACE := any white space character
```
* An OWNERS file can include another file with the `include filePath`
@@ -39,8 +43,31 @@
of the current including file directory and then searched from the
(given) project root directory.
-* An OWNERS file inherit rules in OWNERS files of parent directories
+* The `file: ...` statement is similar to `include ...`
+ * To be compatible to Google's old OWNERS `file:` directive,
+ the `file: ...` statement ignores the `per-file ...` and
+ `set noparent` lines in the directly or indirectly included files.
+ The `include ...` statement takes all statements in the
+ directly included file.
+ * The `include ...` statement is more general and should be
+ used in new OWNERS files.
+ * Google's `file: <file>` directive only includes files
+ in the same repository. The syntax does not accept a
+ "project path" like the `include ...` statement.
+ * Google's `file: <file>` directive requires the include file
+ to have a name with the 'OWNERS' substring,
+ but `include ...` statement can include any file.
+
+* Recursive inclusion (loop) is an error, which is detected
+ and logged as a run-time error. The inclusion is then ignored.
+
+* Repeated inclusion (non-loop) is allowed but redundant.
+ The effect is similar to duplicated statements in an OWNERS file.
+
+* An OWNERS file inherits rules in OWNERS files of parent directories
by default, unless `set noparent` is specified.
+ Note that when a file is included into other files,
+ it does not inherit rules in its parent directory's OWNERS files.
* A "glob" is UNIX globbing pathname without the directory path.
@@ -50,11 +77,15 @@
* `per-file globs = set noparent` is like `set noparent` but applies only to
files matching any of the `globs`. OWNERS files in parent directories
- are not considrered for files matching those globs. Even default ownerEmails
+ are not considered for files matching those globs. Even default ownerEmails
specified in the current OWNERS file are not included.
-* Without the `per-file globs = set noparent`, all global ownerEmails also
- apply to files matching those globs.
+* Without the `per-file globs = set noparent`, all non-per-file
+ ownerEmails also apply to files matching those globs.
+
+* `per-file globs = file: <projectFile>` is equivalent to
+ `per-file globs = emails`, where the emails are collected
+ from top-level emails collected from `file: <projectFile>`.
* The email addresses should belong to registered Gerrit users.
A group mailing address can be used as long as it is associated to
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
index 3233024..e7fef5b 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -113,28 +113,26 @@
// Submitted c2 still finds no owners before c1 is submitted.
assertThat(getOwnersResponse(c2)).contains("owners:[], files:[ t.c ]");
PushOneCommit.Result c1 = addFile("2", "OWNERS",
- "x@x\na@a\nfile:f1.txt\ninclude P1/P2 : f1\ninclude ./d1/d2/../../f2\n");
+ "x@x\na@a\ninclude P1/P2 : f1\ninclude ./d1/d2/../../f2\n");
// Now c2 should find owners, but include directives find no repository or file.
String ownersAX = "owners:[ " + ownerJson("a@a") + ", " + ownerJson("x@x") + " ]";
String path2owners = "path2owners:{ ./:[ a@a, x@x ] }";
String owner2paths = "owner2paths:{ a@a:[ ./ ], x@x:[ ./ ] }";
- String projectName = myProjectName("includeNotFoundTest");
+ String projectName = project.get();
String expectedInLog = "project:" + projectName + ", "
+ "ownersFileName:OWNERS, "
+ "getBranchId:refs/heads/master(FOUND), "
+ "findOwnersFileFor:./t.c, "
+ "findOwnersFileIn:., "
- + "getFile:OWNERS:x@x\\na@a\\nfile:f1.txt\\n"
- + "includeP1/P2:f1\\ninclude./d1/d2/../../f2\\n, "
- + "parseLine:file, "
+ + "getFile:OWNERS:(...), "
+ "parseLine:include:P1/P2:f1, "
+ "getRepoFile:P1/P2:refs/heads/master:f1, "
+ "getRepoFileException:repositorynotfound:P1/P2, " // repository not found
- + "parseLine:include:(empty), " // missing file is treated as empty
+ + "parseLine:include:(), " // missing file is treated as empty
+ "parseLine:include:" + projectName + ":./d1/d2/../../f2, "
+ "getRepoFile:" + projectName + ":refs/heads/master:f2, "
+ "getFile:f2(NOTFOUND), " // same repository but f2 is missing
- + "parseLine:include:(empty), " // missing file is treated as empty
+ + "parseLine:include:(), " // missing file is treated as empty
+ "countNumOwners, "
+ "findOwners, "
+ "checkFile:./t.c, "
@@ -162,7 +160,7 @@
// c2 and c1 are both submitted before existence of OWNERS.
PushOneCommit.Result c2 = addFile("c2", "t.c", "##");
PushOneCommit.Result c1 = addFile("c1", "OWNERS",
- "x@x\na@a\nfile:f1.txt\ninclude P1/P2 : f1\ninclude ./d1/d2/../../f2\n");
+ "x@x\na@a\ninclude P1/P2 : f1\ninclude ./d1/d2/../../f2\n");
String ownerA = ownerJson("a@a");
String ownerX = ownerJson("x@x");
String ownerG1 = ownerJson("g1@g");
@@ -171,22 +169,20 @@
"owners:[ " + ownerA + ", " + ownerG1 + ", " + ownerG2 + ", " + ownerX + " ]";
String path2owners = "path2owners:{ ./:[ a@a, g1@g, g2@g, x@x ] }";
String owner2paths = "owner2paths:{ a@a:[ ./ ], g1@g:[ ./ ], g2@g:[ ./ ], x@x:[ ./ ] }";
- String projectName = myProjectName("includeFoundTest");
+ String projectName = project.get();
String expectedInLog = "project:" + projectName + ", "
+ "ownersFileName:OWNERS, "
+ "getBranchId:refs/heads/master(FOUND), "
+ "findOwnersFileFor:./t.c, "
+ "findOwnersFileIn:., "
- + "getFile:OWNERS:x@x\\na@a\\nfile:f1.txt\\n"
- + "includeP1/P2:f1\\ninclude./d1/d2/../../f2\\n, "
- + "parseLine:file, "
+ + "getFile:OWNERS:(...), "
+ "parseLine:include:P1/P2:f1, "
+ "getRepoFile:P1/P2:refs/heads/master:f1, "
+ "getRepoFileException:repositorynotfound:P1/P2, "
- + "parseLine:include:(empty), " // P1/P2 is still not found
+ + "parseLine:include:(), " // P1/P2 is still not found
+ "parseLine:include:" + projectName + ":./d1/d2/../../f2, "
+ "getRepoFile:" + projectName + ":refs/heads/master:f2, "
- + "getFile:f2:g1@g\\ng2@g\\n, " // f2 is included
+ + "getFile:f2:(...), " // f2 is included
+ "countNumOwners, "
+ "findOwners, "
+ "checkFile:./t.c, "
@@ -231,6 +227,78 @@
}
@Test
+ public void includeVsFileTest() throws Exception {
+ // Test difference between include and file statements.
+ // The file statement skips "set noparent" and "per-file" statements.
+ addFile("d1", "d1/OWNERS", "d1@g\n");
+ addFile("d1/d1", "d1/d1/OWNERS",
+ "per-file *.c=d1d1p@g\nd1d1@g\nfile: d1/OWNERS\n");
+ addFile("d1/d1/d1", "d1/d1/d1/OWNERS",
+ "set noparent\nper-file *.c=d1d1d1p@g\nd1d1d1@g\n");
+ addFile("d1/d2", "d1/d2/OWNERS",
+ "per-file *.c=d1d2p@g\nd1d2@g\ninclude d1/OWNERS\n");
+ addFile("d1/d2/d1", "d1/d2/d1/OWNERS",
+ "set noparent\nper-file *.c=d1d2d1p@g\nd1d2d1@g\n");
+
+ addFile("d2", "d2/OWNERS", "d2@g\n");
+ addFile("d2/d1", "d2/d1/OWNERS",
+ "per-file *.c=d2d1p@g\nd2d1@g\nfile: ./d1/OWNERS\n");
+ addFile("d2/d1/d1", "d2/d1/d1/OWNERS",
+ "set noparent\nper-file *.c=d2d1d1p@g\nd2d1d1@g\n");
+ addFile("d2/d2", "d2/d2/OWNERS",
+ "per-file *.c=d2d2p@g\nd2d2@g\ninclude ./d1/OWNERS\n");
+ addFile("d2/d2/d1", "d2/d2/d1/OWNERS",
+ "set noparent\nper-file *.c=d2d2d1p@g\nd2d2d1@g\n");
+
+ addFile("d3", "d3/OWNERS", "d3@g\n");
+ addFile("d3/d1/d1", "d3/d1/d1/OWNERS", "d3d1d1@g\nfile: ../../../d1/d1/OWNERS\n");
+ addFile("d3/d1/d2", "d3/d1/d2/OWNERS", "d3d1d2@g\nfile: //d1/d2/OWNERS\n");
+ addFile("d3/d2/d1", "d3/d2/d1/OWNERS", "d3d2d1@g\ninclude /d2/d1/OWNERS\n");
+ addFile("d3/d2/d2", "d3/d2/d2/OWNERS", "d3d2d2@g\ninclude //d2/d2/OWNERS\n");
+ PushOneCommit.Result c11 = createChange("c11", "d3/d1/d1/t.c", "test");
+ PushOneCommit.Result c12 = createChange("c12", "d3/d1/d2/t.c", "test");
+ PushOneCommit.Result c21 = createChange("c21", "d3/d2/d1/t.c", "test");
+ PushOneCommit.Result c22 = createChange("c22", "d3/d2/d2/t.c", "test");
+
+ // file and file
+ String owners11 = "file2owners:{ ./d3/d1/d1/t.c:"
+ + "[ d1d1@g, d1d1d1@g, d3@g, d3d1d1@g ] }";
+ // file and include
+ String owners12 = "file2owners:{ ./d3/d1/d2/t.c:"
+ + "[ d1d2@g, d1d2d1@g, d3@g, d3d1d2@g ] }";
+ // include and file
+ String owners21 = "file2owners:{ ./d3/d2/d1/t.c:"
+ + "[ d2d1@g, d2d1d1@g, d2d1p@g, d3@g, d3d2d1@g ] }";
+ // include and include
+ String owners22 = "file2owners:{ ./d3/d2/d2/t.c:"
+ + "[ d2d2@g, d2d2d1@g, d2d2d1p@g, d2d2p@g, d3d2d2@g ] }";
+
+ assertThat(getOwnersDebugResponse(c11)).contains(owners11);
+ assertThat(getOwnersDebugResponse(c12)).contains(owners12);
+ assertThat(getOwnersDebugResponse(c21)).contains(owners21);
+ assertThat(getOwnersDebugResponse(c22)).contains(owners22);
+ }
+
+ @Test
+ public void multipleIncludeTest() throws Exception {
+ // Now "include" and "file:" statements can share parsed results.
+ addFile("d1", "d1/OWNERS", "d1@g\n");
+ addFile("d2/d1", "d2/d1/OWNERS", "include /d1/OWNERS\nfile://d1/OWNERS\n");
+ addFile("d2/d2", "d2/d2/OWNERS", "file: //d1/OWNERS\ninclude /d1/OWNERS\n");
+ PushOneCommit.Result c1 = createChange("c1", "d2/d1/t.c", "test");
+ PushOneCommit.Result c2 = createChange("c2", "d2/d2/t.c", "test");
+ String projectName = project.get();
+ String log1 = "parseLine:useSaved:file:" + projectName + "://d1/OWNERS, ";
+ String log2 = "parseLine:useSaved:include:" + projectName + ":/d1/OWNERS, ";
+ String response1 = getOwnersDebugResponse(c1);
+ String response2 = getOwnersDebugResponse(c2);
+ assertThat(response1).contains(log1);
+ assertThat(response1).doesNotContain(log2);
+ assertThat(response2).doesNotContain(log1);
+ assertThat(response2).contains(log2);
+ }
+
+ @Test
public void includeCycleTest() throws Exception {
// f1 includes f2, f2 includes f3, f3 includes f4, f4 includes f2, OWNERS includes f1.
// All files are in the root directory, but could be referred with relative paths.
@@ -241,26 +309,26 @@
addFile("5", "OWNERS", "x@g\ninclude ./d1/../f1\n");
PushOneCommit.Result c = createChange("6", "t.c", "#\n");
String response = getOwnersDebugResponse(c);
- String projectName = myProjectName("includeCycleTest");
+ String projectName = project.get();
String expectedInLog = "project:" + projectName + ", "
+ "ownersFileName:OWNERS, "
+ "getBranchId:refs/heads/master(FOUND), "
+ "findOwnersFileFor:./t.c, "
+ "findOwnersFileIn:., "
- + "getFile:OWNERS:x@g\\ninclude./d1/../f1\\n, "
+ + "getFile:OWNERS:(...), "
+ "parseLine:include:" + projectName + ":./d1/../f1, "
+ "getRepoFile:" + projectName + ":refs/heads/master:f1, "
- + "getFile:f1:f1@g\\ninclude./f2\\n, "
+ + "getFile:f1:(...), "
+ "parseLine:include:" + projectName + ":./f2, "
+ "getRepoFile:" + projectName + ":refs/heads/master:f2, "
- + "getFile:f2:f2@g\\nincluded1/../f3\\n, "
+ + "getFile:f2:(...), "
+ "parseLine:include:" + projectName + ":d1/../f3, "
+ "getRepoFile:" + projectName + ":refs/heads/master:f3, "
- + "getFile:f3:f3@g\\ninclude/f4\\n, "
+ + "getFile:f3:(...), "
+ "parseLine:include:" + projectName + ":/f4, "
+ "getRepoFile:" + projectName + ":refs/heads/master:f4, "
- + "getFile:f4:f4@g\\nincluded2/../f2\\n, "
- + "parseLine:skip:include:" + projectName + ":d2/../f2, "
+ + "getFile:f4:(...), "
+ + "parseLine:errorRecursion:include:" + projectName + ":d2/../f2, "
+ "countNumOwners, "
+ "findOwners, "
+ "checkFile:./t.c, "
@@ -289,42 +357,41 @@
+ "include ../d2/f2\ninclude /d2/d3/f3\ninclude /d2/../d4/d5/f5\ninclude /d4/f4\n");
String result = getOwnersDebugResponse(c);
assertThat(result).contains("{ ./d6/OWNERS:[ f0@g, f1@g, f2@g, f3@g, f4@g, f5@g, f6@g ] }");
- String skipLog = "parseLine:skip:include:" + myProjectName("includeDuplicationTest") + ":";
+ String projectName = project.get();
+ String skipLog = "parseLine:useSaved:include:" + projectName + ":";
for (String path : new String[]{"../../d0/f0", "../d0/f0", "../d2/f2", "/d2/f2", "/d4/f4"}) {
assertThat(result).contains(skipLog + path);
}
- String projectName = myProjectName("includeDuplicationTest");
String expectedInLog = "project:" + projectName + ", "
+ "ownersFileName:OWNERS, "
+ "getBranchId:refs/heads/master(FOUND), "
+ "findOwnersFileFor:./d6/OWNERS, "
+ "findOwnersFileIn:./d6, "
- + "getFile:d6/OWNERS:f6@g\\ninclude/d0/f0\\ninclude../d1/d2/f1\\ninclude../d2/f2\\n"
- + "include/d2/d3/f3\\ninclude/d2/../d4/d5/f5\\ninclude/d4/f4\\n, "
+ + "getFile:d6/OWNERS:(...), "
+ "parseLine:include:" + projectName + ":/d0/f0, "
+ "getRepoFile:" + projectName + ":refs/heads/master:d0/f0, "
- + "getFile:d0/f0:f0@g\\n, "
+ + "getFile:d0/f0:(...), "
+ "parseLine:include:" + projectName + ":../d1/d2/f1, "
+ "getRepoFile:" + projectName + ":refs/heads/master:d1/d2/f1, "
- + "getFile:d1/d2/f1:f1@g\\ninclude../../d0/f0\\n, "
- + "parseLine:skip:include:" + projectName + ":../../d0/f0, "
+ + "getFile:d1/d2/f1:(...), "
+ + "parseLine:useSaved:include:" + projectName + ":../../d0/f0, "
+ "parseLine:include:" + projectName + ":../d2/f2, "
+ "getRepoFile:" + projectName + ":refs/heads/master:d2/f2, "
- + "getFile:d2/f2:f2@g\\ninclude../d0/f0\\n, "
- + "parseLine:skip:include:" + projectName + ":../d0/f0, "
+ + "getFile:d2/f2:(...), "
+ + "parseLine:useSaved:include:" + projectName + ":../d0/f0, "
+ "parseLine:include:" + projectName + ":/d2/d3/f3, "
+ "getRepoFile:" + projectName + ":refs/heads/master:d2/d3/f3, "
- + "getFile:d2/d3/f3:f3@g\\ninclude/d0/f0\\n, "
- + "parseLine:skip:include:" + projectName + ":/d0/f0, "
+ + "getFile:d2/d3/f3:(...), "
+ + "parseLine:useSaved:include:" + projectName + ":/d0/f0, "
+ "parseLine:include:" + projectName + ":/d2/../d4/d5/f5, "
+ "getRepoFile:" + projectName + ":refs/heads/master:d4/d5/f5, "
- + "getFile:d4/d5/f5:f5@g\\ninclude/d2/f2\\ninclude../f4\\n, "
- + "parseLine:skip:include:" + projectName + ":/d2/f2, "
+ + "getFile:d4/d5/f5:(...), "
+ + "parseLine:useSaved:include:" + projectName + ":/d2/f2, "
+ "parseLine:include:" + projectName + ":../f4, "
+ "getRepoFile:" + projectName + ":refs/heads/master:d4/f4, "
- + "getFile:d4/f4:f4@g\\ninclude../d2/f2\\n, "
- + "parseLine:skip:include:" + projectName + ":../d2/f2, "
- + "parseLine:skip:include:" + projectName + ":/d4/f4, "
+ + "getFile:d4/f4:(...), "
+ + "parseLine:useSaved:include:" + projectName + ":../d2/f2, "
+ + "parseLine:useSaved:include:" + projectName + ":/d4/f4, "
+ "findOwnersFileIn:., "
+ "getFile:OWNERS(NOTFOUND), "
+ "countNumOwners, "
@@ -340,7 +407,7 @@
@Test
public void ownersPerFileTest() throws Exception {
addFile("1", "OWNERS", "per-file *.c=x@x\na@a\nc@c\nb@b\n");
- // Add "t.c" file, which has per-file owner x@x, not a@a, b@b, c@c.
+ // Add "t.c" file, which has per-file owner x@x, and a@a, b@b, c@c.
PushOneCommit.Result c2 = createChange("2", "t.c", "Hello!");
String ownerA = ownerJson("a@a");
String ownerB = ownerJson("b@b");
@@ -354,6 +421,20 @@
}
@Test
+ public void perFileIncludeTest() throws Exception {
+ // A per-file with file: directive to include more owners.
+ addFile("1", "OWNERS", "per-file *.c=x@x\na@a\nper-file t.c=file: t_owner\n");
+ addFile("2", "t_owner", "t1@g\n*\nper-file *.c=y@y\ninclude more_owner\n");
+ addFile("3", "more_owner", "m@g\nm2@g\nper-file *.c=z@z\n");
+ PushOneCommit.Result c1 = createChange("c1", "x.c", "test");
+ PushOneCommit.Result c2 = createChange("c2", "t.c", "test");
+ String c1Response = getOwnersDebugResponse(c1);
+ String c2Response = getOwnersDebugResponse(c2);
+ assertThat(c1Response).contains("file2owners:{ ./x.c:[ a@a, x@x ] }");
+ assertThat(c2Response).contains("file2owners:{ ./t.c:[ *, a@a, m2@g, m@g, t1@g, x@x ] }");
+ }
+
+ @Test
public void includePerFileTest() throws Exception {
// Test included file with per-file, which affects the including file.
PushOneCommit.Result c1 = addFile("1", "d1/d1/OWNERS", "d1d1@g\nper-file OWNERS=d1d1o@g\n");
@@ -551,7 +632,7 @@
public void projectTest() throws Exception {
RestResponse response = adminRestSession.get("/projects/?d");
String content = response.getEntityContent();
- // Predefined projects: "All-Projects", "All-Users", myProjectName("projectTest")
+ // Predefined projects: "All-Projects", "All-Users", project
assertThat(content).contains("\"id\": \"All-Projects\",");
assertThat(content).contains("\"id\": \"All-Users\",");
assertThat(content).contains(idProject("projectTest", "project"));
@@ -887,10 +968,6 @@
return filteredJson(response.getEntityContent());
}
- private String myProjectName(String test) {
- return myProjectName(test, "project");
- }
-
private String myProjectName(String test, String project) {
return this.getClass().getName() + "_" + test + "_" + project;
}
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 b0444bc..a9292b5 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
@@ -34,6 +34,7 @@
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -146,16 +147,40 @@
}
}
- private static final String INCLUDE_STMT1 = " include p1/p2 : /d1/owners";
+ private static final String[] INCLUDE_STMTS = new String[]{
+ " include p1/p2 : /d1/owners",
+ "include p1/p2://d1/owners #comment",
+ "include ../d2/owners",
+ " per-file *.java = file: //d2/OWNERS.java #",
+ "per-file *.cpp,*cc=file:p1/p2:/c++owners",
+ " file: p1/p2 : /d1/owners ",
+ "file: p1/p2://d1/owners #comment",
+ "file: ../d2/owners",
+ "file: //OWNERS",
+ "file:///OWNERS.android"};
- private static final String INCLUDE_STMT2 = "include ../d2/owners ";
+ private static String allIncludeStatements() {
+ String statement = "";
+ for (String s: INCLUDE_STMTS) {
+ statement += s + "\n";
+ }
+ return statement;
+ };
+
+ private static Set<String> allIncludeMsgs() {
+ Set<String> msgs = new HashSet<>();
+ for (int i = 0; i < INCLUDE_STMTS.length; i++) {
+ msgs.add("MSG: unchecked: OWNERS:" + (i + 1)
+ + ": " + Parser.getIncludeOrFile(INCLUDE_STMTS[i]));
+ }
+ return msgs;
+ };
private static final ImmutableMap<String, String> FILES_WITH_NO_ERROR =
ImmutableMap.of(
OWNERS,
- "\n\n#comments ...\n ### more comments\n"
- + INCLUDE_STMT1 + "\n"
- + INCLUDE_STMT2 + "\n"
+ allIncludeStatements()
+ + "\n\n#comments ...\n ### more comments\n"
+ " user1@google.com # comment\n"
+ "u1+review@g.com###\n"
+ " * # everyone can approve\n"
@@ -165,20 +190,20 @@
+ "per-file *.java = set noparent # \n"
+ " set noparent # comment#\n");
- private static final ImmutableSet<String> EXPECTED_VERBOSE_OUTPUT =
- ImmutableSet.of(
- "MSG: validate: " + OWNERS,
- "MSG: unchecked: OWNERS:5: " + INCLUDE_STMT1,
- "MSG: unchecked: OWNERS:6: " + INCLUDE_STMT2,
- "MSG: owner: u1+review@g.com",
- "MSG: owner: u1@g.com",
- "MSG: owner: u2.m@g.com",
- "MSG: owner: user1@google.com");
+ private static Set<String> allVerboseMsgs() {
+ Set<String> msgs = allIncludeMsgs();
+ msgs.add("MSG: validate: " + OWNERS);
+ msgs.add("MSG: owner: u1+review@g.com");
+ msgs.add("MSG: owner: u1@g.com");
+ msgs.add("MSG: owner: u2.m@g.com");
+ msgs.add("MSG: owner: user1@google.com");
+ return msgs;
+ };
+ private static final ImmutableSet<String> EXPECTED_VERBOSE_OUTPUT =
+ ImmutableSet.copyOf(allVerboseMsgs());
private static final ImmutableSet<String> EXPECTED_NON_VERBOSE_OUTPUT =
- ImmutableSet.of(
- "MSG: unchecked: OWNERS:5: " + INCLUDE_STMT1,
- "MSG: unchecked: OWNERS:6: " + INCLUDE_STMT2);
+ ImmutableSet.copyOf(allIncludeMsgs());
@Test
public void testGoodInput() throws Exception {
@@ -198,27 +223,23 @@
OWNERS,
"\n\n\nwrong syntax\n#comment\nuser1@google.com\n",
"d2/" + OWNERS,
- "u1@g.com\nu3@g.com\n*\n",
- "d3/" + OWNERS,
- "\n\nfile: common/Owners\n");
+ "u1@g.com\n\nu3@g.com\n*\n");
+
private static final ImmutableSet<String> EXPECTED_WRONG_SYNTAX =
ImmutableSet.of(
"ERROR: syntax: " + OWNERS + ":4: wrong syntax",
- "ERROR: unknown: u3@g.com at d2/" + OWNERS + ":2",
- "ERROR: ignored: d3/" + OWNERS + ":3: file: common/Owners");
+ "ERROR: unknown: u3@g.com at d2/" + OWNERS + ":3");
private static final ImmutableSet<String> EXPECTED_VERBOSE_WRONG_SYNTAX =
ImmutableSet.of(
- "MSG: validate: d3/" + OWNERS,
"MSG: validate: d2/" + OWNERS,
"MSG: validate: " + OWNERS,
"MSG: owner: user1@google.com",
"MSG: owner: u1@g.com",
"MSG: owner: u3@g.com",
"ERROR: syntax: " + OWNERS + ":4: wrong syntax",
- "ERROR: unknown: u3@g.com at d2/" + OWNERS + ":2",
- "ERROR: ignored: d3/" + OWNERS + ":3: file: common/Owners");
+ "ERROR: unknown: u3@g.com at d2/" + OWNERS + ":3");
@Test
public void testWrongSyntax() 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 15431fe..c2de856 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java
@@ -54,7 +54,7 @@
private static Parser.Result testLine(String line) {
Parser.Result result = new Parser.Result();
// Single line parser tests do not need repoManager.
- Parser parser = new Parser(null, mockedProject(), "master", "OWNERS");
+ Parser parser = new Parser(null, null, mockedProject(), "master", "OWNERS");
parser.parseLine(result, mockedTestDir(), line, 3);
return result;
}
@@ -111,15 +111,25 @@
r2.warnings.add(w2);
r1.noParentGlobs.add(b1);
r2.noParentGlobs.add(b2);
- r2.append(r1);
+ assertThat(r1.owner2paths).isEmpty();
+ assertThat(r2.owner2paths).isEmpty();
+ r2.append(r1, "", true);
+ assertThat(r1.owner2paths).isEmpty();
+ assertThat(r2.owner2paths).isEmpty();
assertThat(r2.warnings).containsExactly(w2, w1);
assertThat(r2.noParentGlobs).containsExactly(b2, b1);
assertThat(r1.noParentGlobs).containsExactly(b1);
assertThat(r2.errors).containsExactly(e2, e1);
- r1.append(r2);
- assertThat(r1.warnings).containsExactly(w1, w2, w1);
- assertThat(r1.noParentGlobs).containsExactly(b2, b1); // set union
- assertThat(r1.errors).containsExactly(e1, e2, e1);
+ r1.append(r2, "", true);
+ assertThat(r1.owner2paths).isEmpty();
+ assertThat(r2.owner2paths).isEmpty();
+ // warnings, errors, and noParentGlobs are sets of strings.
+ // containsExactly does not check order of elements.
+ assertThat(r1.warnings).containsExactly(w1, w2);
+ assertThat(r1.warnings).containsExactly(w2, w1);
+ assertThat(r1.noParentGlobs).containsExactly(b2, b1);
+ assertThat(r1.errors).containsExactly(e1, e2);
+ assertThat(r1.errors).containsExactly(e2, e1);
}
@Test
@@ -149,13 +159,20 @@
@Test
public void fileLineTest() {
- // file: directive is not implemented yet.
+ // file: statement should work like include.
String[] lines = {"file://owners", " file: //d1/owner", "file:owner #"};
for (String s : lines) {
Parser.Result result = testLine(s);
- String expected = testLineWarningMsg(s);
- assertThat(result.warnings).containsExactly(expected);
+ assertThat(result.warnings).isEmpty();
+ assertThat(result.errors).isEmpty();
}
+ testOneFileLine("P0", " file: //common/OWNERS #comment", "P0", "//common/OWNERS");
+ testOneFileLine("P1", "file: Other : /Group ", "Other", "/Group");
+ testOneFileLine("P2", "file: /Common/Project: OWNER", "/Common/Project", "OWNER");
+ testOneFileLine("P3", " file: \tP2/D2:/D3/F.txt", "P2/D2", "/D3/F.txt");
+ testOneFileLine("P4", " file: \tP2/D2://D3/F.txt", "P2/D2", "//D3/F.txt");
+ testOneFileLine("P5", "\t file: \t P2/D2:\t/D3/F2.txt\n", "P2/D2", "/D3/F2.txt");
+ testOneFileLine("P6", "file: ../d1/d2/F \n", "P6", "../d1/d2/F");
}
@Test
@@ -173,7 +190,8 @@
String[] directives = {
"abc@google.com#comment", " *# comment", " xyz@gmail.com # comment",
"a@g.com , xyz@gmail.com , * # comment", "*,*#comment", " a@b,c@d ",
- " set noparent ", "\tset\t\tnoparent\t"
+ " set noparent ", "\tset\t\tnoparent\t",
+ "file://java.owners", " file: p1/p2 : /OWNERS "
};
String[] globsList = {"*", "*,*.c", " *test*.java , *.cc, *.cpp ", "*.bp,*.mk ,A* "};
for (String directive : directives) {
@@ -194,6 +212,10 @@
for (String glob : globList) {
assertThat(result.noParentGlobs).contains(mockedTestDir() + glob);
}
+ } else if (e.startsWith("file:")) {
+ // If per-file has file: directive, it cannot have any other directive.
+ assertThat(e).isEqualTo(Parser.removeExtraSpaces(directive));
+ assertThat(owners).hasLength(1);
} else {
String[] paths = result.owner2paths.get(e).toArray(new String[1]);
assertThat(paths).hasLength(globList.length); // should not work for "set noparent"
@@ -210,8 +232,9 @@
@Test
public void perFileBadDirectiveTest() {
String[] directives = {
- "file://OWNERS", " ** ", "a b@c .co", "a@b@c #com", "a.<b>@zc#",
- " , a@b ", "a@b, , c@d #", "a@b, set noparent"
+ " ** ", "a b@c .co", "a@b@c #com", "a.<b>@zc#",
+ " , a@b ", "a@b, , c@d #", "a@b, set noparent",
+ "a@b, file://java.owners", "*,file:OWNERS"
};
for (String directive : directives) {
String line = "per-file *test*.c=" + directive;
@@ -222,12 +245,21 @@
}
}
- private void testOneIncludeLine(
- String project, String line, String parsedProject, String parsedFile) {
+ private void testOneIncludeOrFileLine(
+ String project, String line, String keyword, String projectName, String filePath) {
String[] results = Parser.parseInclude(project, line);
- assertThat(results).hasLength(2);
- assertThat(results[0]).isEqualTo(parsedProject);
- assertThat(results[1]).isEqualTo(parsedFile);
+ assertThat(results).hasLength(3);
+ assertThat(results[0]).isEqualTo(keyword);
+ assertThat(results[1]).isEqualTo(projectName);
+ assertThat(results[2]).isEqualTo(filePath);
+ }
+
+ private void testOneFileLine(String project, String line, String projectName, String filePath) {
+ testOneIncludeOrFileLine(project, line, "file", projectName, filePath);
+ }
+
+ private void testOneIncludeLine(String project, String line, String projectName, String filePath) {
+ testOneIncludeOrFileLine(project, line, "include", projectName, filePath);
}
@Test
@@ -241,6 +273,31 @@
}
@Test
+ public void getIncludeOrFileTest() {
+ String[] line = new String[] {
+ "", "wrong input", "INCLUDE X",
+ "include //f2.txt # ",
+ " include P1/P2: ../f1 # ",
+ " file://f3 # ",
+ "file: P1:f3",
+ " per-file *.c,file.c = file: /OWNERS # ",
+ "per-file *=file:P1/P2: /O# ",
+ };
+ String[] expected = new String[] {
+ "", "", "",
+ "include //f2.txt",
+ "include P1/P2:../f1",
+ "file://f3",
+ "file:P1:f3",
+ "file:/OWNERS",
+ "file:P1/P2:/O",
+ };
+ for (int i = 0; i < line.length; i++) {
+ assertThat(Parser.getIncludeOrFile(line[i])).isEqualTo(expected[i]);
+ }
+ }
+
+ @Test
public void errorMsgTest() {
String file = "./OWNERS";
int n = 5;
