Add include command to OWNERS file.
* New OwnersDB.getRepoFile function to read any included file.
* Inject GitRepositoryManager to plugin entry functions.
* Functions passing Repository parameter are now passing a
GitRepositoryManager parameter.
* Depend on new StoredValues.REPO_MANAGER in Checker.java for
Prolog engine.
* Enhanced parser:
* Rename/clean up dir/file name normalization functions in Util.java.
* Use a Parser object to store states and reduce number of parameters
passed through parser functions.
* Use an includeStack to keep track of current include file project
and file path.
* Use a set of "included" files to avoid cyclic/repeated included files.
* Rename some constants in Parse.java to match google3 Java coding style.
* OwnersValidator.java skips the include command for now.
More validation tests to be added later on the include path
and included files not named as OWNERS.
* New tests and document:
* Add new integration test cases to FindOwnersIT.java.
* Add new unit test cases to ParserTest.java.
* Add new syntax and examples to syntax.md,
and fix some other minor documentation errors.
Change-Id: Ifd8b4dd9742820939d94b765f35cd9d7410d2a23
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
index 493ae7e..8b33f73 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -51,7 +51,6 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
-import org.eclipse.jgit.lib.Repository;
/** Create and return OWNERS info when "Find Owners" button is clicked. */
class Action implements RestReadView<RevisionResource>, UiAction<RevisionResource> {
@@ -134,11 +133,8 @@
// Used by integration tests, because they do not have ReviewDb Provider.
public Response<RestResult> apply(ReviewDb reviewDb, ChangeResource rsrc, Parameters params)
throws IOException, OrmException, BadRequestException {
- Change c = rsrc.getChange();
- try (Repository repo = repoManager.openRepository(c.getProject())) {
- ChangeData changeData = changeDataFactory.create(reviewDb, c);
- return getChangeData(repo, params, changeData);
- }
+ ChangeData changeData = changeDataFactory.create(reviewDb, rsrc.getChange());
+ return getChangeData(params, changeData);
}
/** Returns reviewer emails got from ChangeData. */
@@ -178,14 +174,13 @@
}
/** REST API to return owners info of a change. */
- public Response<RestResult> getChangeData(
- Repository repository, Parameters params, ChangeData changeData)
+ public Response<RestResult> getChangeData(Parameters params, ChangeData changeData)
throws OrmException, BadRequestException, IOException {
int patchset = getValidPatchsetNum(changeData, params.patchset);
ProjectState projectState = projectCache.get(changeData.project());
Boolean useCache = params.nocache == null || !params.nocache;
OwnersDb db = Cache.getInstance().get(
- useCache, projectState, accountCache, emails, repository, changeData, patchset);
+ useCache, projectState, accountCache, emails, repoManager, changeData, patchset);
Collection<String> changedFiles = changeData.currentFilePaths();
Map<String, Set<String>> file2Owners = db.findOwners(changedFiles);
@@ -234,19 +229,17 @@
// If alwaysShowButton is true, skip expensive owner lookup.
if (needFindOwners && !Config.getAlwaysShowButton()) {
needFindOwners = false; // Show button only if some owner is found.
- try (Repository repo = repoManager.openRepository(change.getProject())) {
- OwnersDb db =
- Cache.getInstance()
- .get(
- true, // use cached OwnersDb
- projectCache.get(resource.getProject()),
- accountCache,
- emails,
- repo,
- changeData);
- logger.atFiner().log("getDescription db key = %s", db.key);
- needFindOwners = db.getNumOwners() > 0;
- }
+ OwnersDb db =
+ Cache.getInstance()
+ .get(
+ true, // use cached OwnersDb
+ projectCache.get(resource.getProject()),
+ accountCache,
+ emails,
+ repoManager,
+ changeData);
+ logger.atFiner().log("getDescription db key = %s", db.key);
+ needFindOwners = db.getNumOwners() > 0;
}
return new Description()
.setLabel("Find Owners")
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
index bf01666..df56fc7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
@@ -20,6 +20,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.Emails;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
@@ -27,7 +28,6 @@
import java.util.Collection;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
-import org.eclipse.jgit.lib.Repository;
/** Save OwnersDb in a cache for multiple calls to submit_filter. */
class Cache {
@@ -92,7 +92,7 @@
ProjectState projectState,
AccountCache accountCache,
Emails emails,
- Repository repo,
+ GitRepositoryManager repoManager,
ChangeData changeData)
throws OrmException, IOException {
return get(
@@ -100,7 +100,7 @@
projectState,
accountCache,
emails,
- repo,
+ repoManager,
changeData,
changeData.currentPatchSet().getId().get());
}
@@ -111,7 +111,7 @@
ProjectState projectState,
AccountCache accountCache,
Emails emails,
- Repository repository,
+ GitRepositoryManager repoManager,
ChangeData changeData,
int patchset)
throws OrmException, IOException {
@@ -124,7 +124,7 @@
accountCache,
emails,
dbKey,
- repository,
+ repoManager,
changeData,
branch,
changeData.currentFilePaths());
@@ -137,14 +137,14 @@
AccountCache accountCache,
Emails emails,
String key,
- Repository repository,
+ GitRepositoryManager repoManager,
ChangeData changeData,
String branch,
Collection<String> files) {
if (dbCache == null || !useCache) { // Do not cache OwnersDb
logger.atFiner().log("Create new OwnersDb, key=%s", key);
return new OwnersDb(
- projectState, accountCache, emails, key, repository, changeData, branch, files);
+ projectState, accountCache, emails, key, repoManager, changeData, branch, files);
}
try {
logger.atFiner().log(
@@ -156,14 +156,14 @@
public OwnersDb call() {
logger.atFiner().log("Create new OwnersDb, key=%s", key);
return new OwnersDb(
- projectState, accountCache, emails, key, repository, changeData, branch, files);
+ projectState, accountCache, emails, key, repoManager, changeData, branch, files);
}
});
} catch (ExecutionException e) {
logger.atSevere().withCause(e).log(
"Cache.get has exception for %s", Config.getChangeId(changeData));
return new OwnersDb(
- projectState, accountCache, emails, key, repository, changeData, branch, files);
+ projectState, accountCache, emails, key, repoManager, changeData, branch, files);
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
index e361f82..187e87b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -19,6 +19,7 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.Emails;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.StoredValues;
@@ -29,7 +30,6 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
-import org.eclipse.jgit.lib.Repository;
/** Check if a change needs owner approval. */
public class Checker {
@@ -39,12 +39,12 @@
private static final String EXEMPT_MESSAGE1 = "Exempt-From-Owner-Approval:";
private static final String EXEMPT_MESSAGE2 = "Exempted-From-Owner-Approval:";
- private Repository repository;
- private ChangeData changeData;
+ private final GitRepositoryManager repoManager;
+ private final ChangeData changeData;
private int minVoteLevel;
- Checker(Repository repository, ChangeData changeData, int v) {
- this.repository = repository;
+ Checker(GitRepositoryManager repoManager, ChangeData changeData, int v) {
+ this.repoManager = repoManager;
this.changeData = changeData;
minVoteLevel = v;
}
@@ -115,10 +115,10 @@
try {
changeData = StoredValues.CHANGE_DATA.get(engine);
ProjectState projectState = StoredValues.PROJECT_STATE.get(engine);
+ GitRepositoryManager repoManager = StoredValues.REPO_MANAGER.get(engine);
AccountCache accountCache = StoredValues.ACCOUNT_CACHE.get(engine);
Emails emails = StoredValues.EMAILS.get(engine);
- Repository repository = StoredValues.REPOSITORY.get(engine);
- return new Checker(repository, changeData, minVoteLevel)
+ return new Checker(repoManager, changeData, minVoteLevel)
.findApproval(projectState, accountCache, emails);
} catch (OrmException | IOException e) {
logger.atSevere().withCause(e).log("Exception for %s ", Config.getChangeId(changeData));
@@ -151,7 +151,7 @@
// One update to a Gerrit change can call submit_rule or submit_filter
// many times. So this function should use cached values.
OwnersDb db =
- Cache.getInstance().get(true, projectState, accountCache, emails, repository, changeData);
+ Cache.getInstance().get(true, projectState, accountCache, emails, repoManager, changeData);
if (db.getNumOwners() <= 0) {
return 0;
}
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 04f295d..ca6cae0 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -19,8 +19,10 @@
import com.google.common.collect.Multimap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.Emails;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import java.net.InetAddress;
@@ -49,6 +51,7 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private AccountCache accountCache;
+ private GitRepositoryManager repoManager;
private Emails emails;
private int numOwners = -1; // # of owners of all given files.
@@ -70,11 +73,12 @@
AccountCache accountCache,
Emails emails,
String key,
- Repository repository,
+ GitRepositoryManager repoManager,
ChangeData changeData,
String branch,
Collection<String> files) {
this.accountCache = accountCache;
+ this.repoManager = repoManager;
this.emails = emails;
this.key = key;
try {
@@ -85,39 +89,48 @@
}
logs.add("key:" + key);
preferredEmails.put("*", "*");
+ String projectName = projectState.getName();
+ logs.add("project:" + projectName);
String ownersFileName = Config.getOwnersFileName(projectState, changeData);
logs.add("ownersFileName:" + ownersFileName);
- // Some hacked CL could have a target branch that is not created yet.
- ObjectId id = getBranchId(repository, branch, changeData, logs);
- revision = "";
- if (id != null) {
- for (String fileName : files) {
- // Find OWNERS in fileName's directory and parent directories.
- // Stop looking for a parent directory if OWNERS has "set noparent".
- fileName = Util.normalizedFilePath(fileName);
- String dir = Util.normalizedDirPath(fileName); // e.g. dir = ./d1/d2
- logs.add("findOwnersFileFor:" + fileName);
- while (!readDirs.contains(dir)) {
- readDirs.add(dir);
- logs.add("findOwnersFileIn:" + dir);
- String filePath = (dir + "/" + ownersFileName).substring(2); // remove "./"
- String content = getRepositoryFile(repository, id, filePath, logs);
- if (content != null && !content.equals("")) {
- addFile(dir + "/", dir + "/" + ownersFileName, content.split("\\R+"));
+ try (Repository repo = repoManager.openRepository(projectState.getNameKey())) {
+ // Some hacked CL could have a target branch that is not created yet.
+ ObjectId id = getBranchId(repo, branch, changeData, logs);
+ revision = "";
+ if (id != null) {
+ for (String fileName : files) {
+ // Find OWNERS in fileName's directory and parent directories.
+ // Stop looking for a parent directory if OWNERS has "set noparent".
+ fileName = Util.addDotPrefix(fileName); // e.g. "./" "./d1/f1" "./d2/d3/"
+ String dir = Util.getParentDir(fileName); // e.g. "." "./d1" "./d2"
+ logs.add("findOwnersFileFor:" + fileName);
+ while (!readDirs.contains(dir)) {
+ readDirs.add(dir);
+ logs.add("findOwnersFileIn:" + dir);
+ String filePath = dir + "/" + ownersFileName;
+ String content = getFile(repo, id, filePath, logs);
+ if (content != null && !content.isEmpty()) {
+ addFile(projectName, branch, dir + "/", dir + "/" + ownersFileName,
+ content.split("\\R+"));
+ }
+ if (stopLooking.contains(dir + "/") || !dir.contains("/")) {
+ break; // stop looking through parent directory
+ }
+ dir = Util.getDirName(dir); // go up one level
}
- if (stopLooking.contains(dir + "/") || !dir.contains("/")) {
- break; // stop looking through parent directory
- }
- dir = Util.getDirName(dir); // go up one level
+ }
+ try {
+ revision = repo.exactRef(branch).getObjectId().getName();
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(
+ "Fail to get branch revision for %s", Config.getChangeId(changeData));
+ logException(logs, "OwnersDb get revision", e);
}
}
- try {
- revision = repository.exactRef(branch).getObjectId().getName();
- } catch (Exception e) {
- logger.atSevere().withCause(e).log(
- "Fail to get branch revision for %s", Config.getChangeId(changeData));
- logException(logs, "OwnersDb get revision", e);
- }
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(
+ "Fail to find repository of project %s", projectName);
+ logException(logs, "OwnersDb get repository", e);
}
countNumOwners(files);
}
@@ -186,8 +199,9 @@
}
}
- void addFile(String dirPath, String filePath, String[] lines) {
- Parser.Result result = Parser.parseFile(dirPath, filePath, lines);
+ void addFile(String project, String branch, String dirPath, String filePath, String[] lines) {
+ Parser parser = new Parser(repoManager, project, branch, filePath, logs);
+ Parser.Result result = parser.parseFile(dirPath, lines);
if (result.stopLooking) {
stopLooking.add(dirPath);
}
@@ -253,9 +267,9 @@
Arrays.sort(files); // Force an ordered search sequence.
Map<String, Set<String>> file2Owners = new HashMap<>();
for (String fileName : files) {
- fileName = Util.normalizedFilePath(fileName);
+ fileName = Util.addDotPrefix(fileName);
logs.add("checkFile:" + fileName);
- String dirPath = Util.normalizedDirPath(fileName);
+ String dirPath = Util.getParentDir(fileName);
String baseName = fileName.substring(dirPath.length() + 1);
int distance = 1;
FileSystem fileSystem = FileSystems.getDefault();
@@ -341,10 +355,30 @@
return null;
}
+ /** Returns file content or empty string; uses project+branch+file names. */
+ public static String getRepoFile(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);
+ 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 not found branch " + branch);
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(
+ "Fail to find repository of project %s", project);
+ logException(logs, "getRepoFile", e);
+ }
+ return "";
+ }
+
/** Returns file content or empty string; uses Repository. */
- private static String getRepositoryFile(
+ private static String getFile(
Repository repo, ObjectId id, String file, List<String> logs) {
- String header = "getRepositoryFile:" + file;
+ file = Util.gitRepoFilePath(file);
+ String header = "getFile:" + file;
try (RevWalk revWalk = new RevWalk(repo)) {
RevTree tree = revWalk.parseCommit(id).getTree();
ObjectReader reader = revWalk.getObjectReader();
@@ -357,7 +391,7 @@
logs.add(header + " (NOT FOUND)");
} catch (Exception e) {
logger.atSevere().withCause(e).log("get file %s", file);
- logException(logs, header, e);
+ logException(logs, "getFile", e);
}
return "";
}
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 8f2b262..cf3efb4 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
@@ -31,7 +31,6 @@
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.config.ProjectConfigEntry;
import com.google.gerrit.server.events.CommitReceivedEvent;
-import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
@@ -55,7 +54,6 @@
import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectLoader;
-import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.treewalk.TreeWalk;
@@ -88,18 +86,15 @@
private final String pluginName;
private final PluginConfigFactory cfgFactory;
- private final GitRepositoryManager repoManager;
private final Emails emails;
@Inject
OwnersValidator(
@PluginName String pluginName,
PluginConfigFactory cfgFactory,
- GitRepositoryManager repoManager,
Emails emails) {
this.pluginName = pluginName;
this.cfgFactory = cfgFactory;
- this.repoManager = repoManager;
this.emails = emails;
}
@@ -134,10 +129,8 @@
Project.NameKey project = receiveEvent.project.getNameKey();
PluginConfig cfg = cfgFactory.getFromProjectConfigWithInheritance(project, pluginName);
if (isActive(cfg)) {
- try (Repository repo = repoManager.openRepository(project)) {
- String name = getOwnersFileName(project);
- messages = performValidation(receiveEvent.commit, receiveEvent.revWalk, name, false);
- }
+ String name = getOwnersFileName(project);
+ messages = performValidation(receiveEvent.commit, receiveEvent.revWalk, name, false);
}
} catch (NoSuchProjectException | IOException e) {
throw new CommitValidationException("failed to check owners files", e);
@@ -265,9 +258,15 @@
for (String e : emails) {
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);
} else {
- String prefix = Parser.isFile(line) ? "ignored" : "syntax";
- add(messages, prefix + ": " + path + ":" + lineNumber + ": " + line, true);
+ 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 690e474..317bea7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java
@@ -14,8 +14,14 @@
package com.googlesource.gerrit.plugins.findowners;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import java.io.IOException;
+import java.util.ArrayDeque;
import java.util.ArrayList;
+import java.util.Deque;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -32,8 +38,11 @@
* line := "set" \s+ "noparent"
* | "per-file" \s+ globs \s* "=" \s* directives
* | "file:" \s* glob
+ * | "include" SPACE+ (project SPACE* ":" SPACE*)? filePath
* | comment
* | directive
+ * project := a Gerrit absolute project path name without space or column character
+ * filePath := a file path name without space or column character
* directives := directive (comma directive)*
* directive := email_address
* | "*"
@@ -49,9 +58,14 @@
* A glob does not contain directory path.
*/
class Parser {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
// Globs and emails are separated by commas with optional spaces around a comma.
protected static final String COMMA = "[\\s]*,[\\s]*"; // used in unit tests
+ // Separator for project and file paths in an include line.
+ private static final String COLUMN = "[\\s]*:[\\s]*"; // project:file
+
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
@@ -59,58 +73,110 @@
// TODO: have a more precise email address pattern.
private static final String EMAIL_OR_STAR = "([^\\s<>@,]+@[^\\s<>@#,]+|\\*)";
+ // A Gerrit project name followed by a column and optional spaces.
+ 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 PROJECT_AND_FILE = PROJECT_NAME + FILE_PATH;
+
// 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);
+ 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);
+ private static final Pattern PAT_NO_PARENT = Pattern.compile(BOL + "set[\\s]+noparent" + EOL);
// Patterns to match trimmed globs and emails in per-file lines.
- private static final Pattern patEmailList =
+ private static final Pattern PAT_EMAIL_LIST =
Pattern.compile("^(" + EMAIL_OR_STAR + "(" + COMMA + EMAIL_OR_STAR + ")*)$");
- private static final Pattern patGlobs =
+ private static final Pattern PAT_GLOBS =
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 =
+ // PAT_PER_FILE matches a line to two groups: (1) globs, (2) emails
+ // Trimmed 1st group should match PAT_GLOBS; trimmed 2nd group should match PAT_EMAIL_LIST.
+ private static final Pattern PAT_PER_FILE =
Pattern.compile(BOL + "per-file[\\s]+([^=#]+)=[\\s]*([^#]+)" + EOL);
+ // A parser keeps current repoManager, project, branch, included file path, and debug/trace logs.
+ 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 List<String> logs; // Keeps debug/trace messages.
+
+ Parser(GitRepositoryManager repoManager, String project, String branch, String file) {
+ init(repoManager, project, branch, file, new ArrayList<>());
+ }
+
+ Parser(GitRepositoryManager repoManager,
+ String project, String branch, String file, List<String> logs) {
+ init(repoManager, project, branch, file, logs);
+ }
+
+ private void init(GitRepositoryManager repoManager,
+ String project, String branch, String file, List<String> logs) {
+ this.repoManager = repoManager;
+ this.branch = branch;
+ this.logs = logs;
+ includeStack = new ArrayDeque<>();
+ included = new HashSet<>();
+ pushProjectFilePaths(project, normalizedRepoDirFilePath(".", file));
+ }
+
static boolean isComment(String line) {
- return patComment.matcher(line).matches();
+ return PAT_COMMENT.matcher(line).matches();
}
static boolean isFile(String line) {
- return patFile.matcher(line).matches();
+ return PAT_FILE.matcher(line).matches();
+ }
+
+ static boolean isInclude(String line) {
+ return PAT_INCLUDE.matcher(line).matches();
}
static boolean isGlobs(String line) {
- return patGlobs.matcher(line).matches();
+ return PAT_GLOBS.matcher(line).matches();
}
static boolean isNoParent(String line) {
- return patNoParent.matcher(line).matches();
+ return PAT_NO_PARENT.matcher(line).matches();
}
static String parseEmail(String line) {
- Matcher m = Parser.patEmail.matcher(line);
+ Matcher m = Parser.PAT_EMAIL.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()) {
+ static String[] parseInclude(String project, String line) {
+ Matcher m = Parser.PAT_INCLUDE.matcher(line);
+ if (!m.matches()) {
return null;
}
- String[] parts = new String[2];
- parts[0] = m.group(1).trim();
- parts[1] = m.group(2).trim();
+ 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
+ }
return parts;
}
+ static String[] parsePerFile(String line) {
+ Matcher m = PAT_PER_FILE.matcher(line);
+ if (!m.matches() || !isGlobs(m.group(1).trim())
+ || !PAT_EMAIL_LIST.matcher(m.group(2).trim()).matches()) {
+ return null;
+ }
+ return new String[]{m.group(1).trim(), m.group(2).trim()};
+ }
+
static String[] parsePerFileEmails(String line) {
String[] globsAndEmails = parsePerFile(line);
- return (globsAndEmails != null) ? globsAndEmails[1].split(COMMA) : null;
+ return (globsAndEmails != null) ? globsAndEmails[1].split(COMMA, -1) : null;
}
static class Result {
@@ -125,30 +191,77 @@
errors = new ArrayList<>();
owner2paths = new HashMap<>();
}
+
+ void append(Result r) {
+ warnings.addAll(r.warnings);
+ errors.addAll(r.warnings);
+ stopLooking |= r.stopLooking; // included file's "set noparent" applies to the including file
+ for (String key : r.owner2paths.keySet()) {
+ for (String dir : r.owner2paths.get(key)) {
+ Util.addToMap(owner2paths, key, dir);
+ }
+ }
+ }
}
- static Result parseFile(String dir, String file, String[] lines) {
+ // 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".
+ Result parseFile(String dir, String[] lines) {
Result result = new Result();
int n = 0;
for (String line : lines) {
- Parser.parseLine(result, dir, file, line, ++n);
+ parseLine(result, dir, line, ++n);
}
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();
+ }
+
+ private String normalizedRepoDirFilePath(String dir, String path) {
+ try {
+ return Util.normalizedRepoDirFilePath(dir, path);
+ } catch (IOException e) {
+ String msg = "Fail to normalized path " + dir + " / " + path;
+ logger.atSevere().withCause(e).log(msg);
+ logs.add(msg + ":" + e.getMessage());
+ return dir + "/" + path;
+ }
+ }
+
/**
* Parse a line in OWNERS file and add info to OwnersDb.
*
* @param result a Result object to keep parsed info.
* @param dir the path to OWNERS file directory.
- * @param file the OWNERS file path.
* @param line the source line.
* @param num the line number.
*/
- static void parseLine(Result result, String dir, String file, String line, int num) {
- // comment and file: directive are parsed but ignored.
+ void parseLine(Result result, String dir, String line, int num) {
String email;
String[] globsAndEmails;
+ String[] projectAndFile;
if (isNoParent(line)) {
result.stopLooking = true;
} else if (isComment(line)) {
@@ -163,9 +276,34 @@
}
}
} else if (isFile(line)) {
- result.warnings.add(warningMsg(file, num, "ignored", 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 {
- result.errors.add(errorMsg(file, num, "ignored unknown line", line));
+ result.errors.add(errorMsg(currentFilePath(), num, "ignored unknown line", line));
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Util.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Util.java
index 1888fa1..77eff7f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Util.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Util.java
@@ -16,6 +16,7 @@
import com.google.common.collect.Ordering;
import java.io.File;
+import java.io.IOException;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
@@ -35,12 +36,50 @@
return new File(path).getParent();
}
- static String normalizedFilePath(String path) {
+ // Assuming that path is a valid git file or directory path, add a "./" prefix,
+ // so that "." can be used as the "root" parent directory.
+ static String addDotPrefix(String path) {
return path.startsWith("./") ? path : ("./" + path);
}
- static String normalizedDirPath(String path) {
- return new File(normalizedFilePath(path)).getParent();
+ // Assuming that path is a valid git file or directory path,
+ // get the parent directory path of the given file or directory.
+ static String getParentDir(String path) {
+ return new File(addDotPrefix(path)).getParent();
+ }
+
+ // Given valid git 'dir' path and user-input relative or absolute file 'path',
+ // return a valid git file path with "./" prefix.
+ static String normalizedDirFilePath(String dir, String path) throws IOException {
+ // With an absolute path, starting with "/", current dir is ignored.
+ if (path.startsWith("/")) {
+ dir = ".";
+ }
+ return "." + new File("/" + dir + "/" + path).getCanonicalPath();
+ }
+
+ // Git repository file path cannot contain leading "/" or "./", or "/" at the end.
+ static String gitRepoFilePath(String file) {
+ if (file == null) {
+ return "";
+ }
+ int last = file.length() - 1;
+ while (last >= 0 && file.charAt(last) == '/') {
+ --last;
+ }
+ int first = 0;
+ while (first < last && file.charAt(first) == '/') {
+ ++first;
+ }
+ file = file.substring(first, last + 1);
+ if (file.startsWith("./")) {
+ return gitRepoFilePath(file.substring(2));
+ }
+ return file;
+ }
+
+ static String normalizedRepoDirFilePath(String dir, String path) throws IOException {
+ return gitRepoFilePath(normalizedDirFilePath(dir, path));
}
static boolean parseBoolean(String s) {
diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md
index 839f738..882249e 100644
--- a/src/main/resources/Documentation/about.md
+++ b/src/main/resources/Documentation/about.md
@@ -10,10 +10,10 @@
Projects with **OWNERS** file should be configured with
Prolog `submit_rule` or `submit_filter`, see [config.md](config.md).
-A **`Find Owners`** button is added to the Gerrit revision screen
-if a change needs owner approval. The button pops up a window that contains
-* current reviewers and owners of changed files for users to select, and
-* optionally changed files without required *owner* code review vote.
+A **[FIND OWNERS]** action button is added to the Gerrit revision screen.
+The button pops up a window that contains
+* owners of changed files for users to select to add to the reviewers list, and
+* changed files without required *owner* code review vote.
A REST API is added to get owners information of a change.
The API is described in [rest-api.md](rest-api.md).
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index 5974482..38b7d0d 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -43,14 +43,14 @@
When a user clicks on the Submit button,
a window will pop up and ask the user to
(1) add missing *owners* to the reviewers list and/or
- ask for owner's +1 Code-Review votes, or
+ ask for owner's +1 Code-Review votes, or
(2) add `Exempt-From-Owner-Approval:` to the commit message.
- The **`Find Owners`** button is useful in this situation to find
+ The **`[[FIND OWNERS]]`** button is useful in this situation to find
the missing *owners* or +1 votes of any changed files.
When `label('Owner-Approved', may(_))` is added to the submit rule output,
Gerrit displays a grey 'Owner-Approved' label. To avoid confusion,
-this 'may(_)' state label could be removed by the `submit_filter` of
+this `may(_)` state label could be removed by the `submit_filter` of
the root level `All-Projects`. Special automerge processes could
create changes that do not need either Code-Review vote or owner approval.
Such special conditions can also be handled in the `submit_filter`
diff --git a/src/main/resources/Documentation/syntax.md b/src/main/resources/Documentation/syntax.md
index 0cf3ae7..b263f4f 100644
--- a/src/main/resources/Documentation/syntax.md
+++ b/src/main/resources/Documentation/syntax.md
@@ -4,12 +4,13 @@
Owner approval is based on OWNERS files located in the same
repository and top of the _merge-to_ branch of a patch set.
-The syntax is:
+### Syntax
```java
lines := (SPACE* line? SPACE* EOL)*
line := "set" SPACE+ "noparent"
| "per-file" SPACE+ globs SPACE* "=" SPACE* directives
+ | "include" SPACE+ (project SPACE* ":" SPACE*)? filePath
| comment
| directive
directives := directive (SPACE* "," SPACE* directive)*
@@ -19,13 +20,25 @@
glob := [a-zA-Z0-9_-*?.]+
comment := "#" ANYCHAR*
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"
+ or "include project:filePath" line.
+ When the "project:" is not specified, the OWNERS file's project is used.
+ The included file is given with the "filePath".
+
+* If the filePath starts with "/", it is an absolute path starting from
+ the project root directory. Otherwise, the filePath is added a prefix
+ of the current including file directory and then searched from the
+ (given) project root directory.
+
* `per-file globs = directives` applies each `directive` only to files
- matching any of the `glob`.
+ matching any of the `globs`.
* A 'glob' does not contain directory path.
@@ -33,3 +46,18 @@
* The `*` directive means that no owner is specified for the directory
or file. Any user can approve that directory or file.
+
+### Examples
+
+```bash
+ # a comment starts with # to EOL; leading spaces are ignored
+set noparent # do not inherit owners defined in parent directories
+include P1/P2:/core/OWNERS # include file core/OWNERS of project P1/P2
+include ../base/OWNERS # include with relative path to the current directory
+per-file *.c,*.cpp = c@g.com,cpp@g.com # special owners of .c and .cpp files
+per-file *.c = x@g.com # .c file owners are c@g.com and x@g.com
+abc@g.com # default owner, not for *.c and *.cpp files
+xyz@g.com # another default owner, not for *.c and *.cpp files
+per-file README:* # no specific owner for the README file
+
+```
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 6198139..3b865bd 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -41,7 +41,6 @@
import java.util.Collection;
import java.util.Optional;
import org.eclipse.jgit.lib.ObjectLoader;
-import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -79,88 +78,363 @@
}
@Test
- public void ownersFile1Test() throws Exception {
- // Create 1st OWNERS file, this change does not have owner.
- PushOneCommit.Result c1 = createChange("add OWNERS", "OWNERS", "x@x\na@a\n");
- assertThat(getOwnersResponse(c1)).contains("owners:[], files:[ OWNERS ]");
- // Create another "t.c" file, which has no owner because c1 is not submitted yet.
- PushOneCommit.Result c2 = createChange("add t.c", "t.c", "##");
+ public void includeNotFoundTest() throws Exception {
+ // c2 and c1 are both submitted before existence of OWNERS.
+ PushOneCommit.Result c2 = addFile("1", "t.c", "##");
+ // Submitted c2 still finds no owners before c1 is submitted.
assertThat(getOwnersResponse(c2)).contains("owners:[], files:[ t.c ]");
- // Change of "t.c" file has owner after c1 is submitted.
- approveSubmit(c1);
- String ownerA = ownerJson("a@a");
- String ownerX = ownerJson("x@x");
- String ownersAX = "owners:[ " + ownerA + ", " + ownerX + " ]";
- assertThat(getOwnersResponse(c2)).contains(ownersAX + ", 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");
+ // 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 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, "
+ + "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:" + 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
+ + "countNumOwners, "
+ + "findOwners, "
+ + "checkFile:./t.c, "
+ + "checkDir:., "
+ + "addOwnerWeightsIn:./ "
+ + "] ";
+ String c2Response = getOwnersDebugResponse(c2);
+ assertThat(c2Response).contains(path2owners);
+ assertThat(c2Response).contains(owner2paths);
+ assertThat(c2Response).contains("file2owners:{ ./t.c:[ a@a, x@x ] }");
+ assertThat(c2Response).contains(ownersAX);
+ assertThat(c2Response).contains(expectedInLog);
// A submitted change gets owners info from current repository.
- assertThat(getOwnersResponse(c1)).contains(ownersAX + ", files:[ OWNERS ]");
- // Check all fields in response.
- String expectedTail =
- "path2owners:{ ./:[ a@a, x@x ] }, owner2paths:{ a@a:[ ./ ], x@x:[ ./ ] }, logs:[ "
- + "key:12:1:master, ownersFileName:OWNERS, getBranchId:refs/heads/master(FOUND), "
- + "findOwnersFileFor:./t.c, findOwnersFileIn:., getRepositoryFile:OWNERS:x@x\\na@a\\n, "
- + "countNumOwners, findOwners, checkFile:./t.c, checkDir:., addOwnerWeightsIn:./ ] "
- + "}, file2owners:{ ./t.c:[ a@a, x@x ] }, reviewers:[], "
- + ownersAX
- + ", files:[ t.c ] }";
- assertThat(getOwnersDebugResponse(c2)).contains(expectedTail);
+ String c1Response = getOwnersDebugResponse(c1);
+ assertThat(c1Response).contains(path2owners);
+ assertThat(c1Response).contains(owner2paths);
+ assertThat(c1Response).contains("file2owners:{ ./OWNERS:[ a@a, x@x ] }");
+ assertThat(c1Response).contains(ownersAX);
}
@Test
- public void ownersFile2Test() throws Exception {
- // Add OWNERS file, this test does not inherit files created in ownersFile1Test.
- addFile("add OWNERS", "OWNERS", "per-file *.c=x@x\na@a\nc@c\nb@b\n");
+ public void includeFoundTest() throws Exception {
+ // Compared with includeNotFoundTest, this one has file "f2" to include.
+ addFile("c0", "f2", "g1@g\ng2@g\n");
+ // 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");
+ String ownerA = ownerJson("a@a");
+ String ownerX = ownerJson("x@x");
+ String ownerG1 = ownerJson("g1@g");
+ String ownerG2 = ownerJson("g2@g");
+ String ownersAG1G2X =
+ "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 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, "
+ + "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:" + projectName + ":./d1/d2/../../f2, "
+ + "getRepoFile:" + projectName + ":refs/heads/master:f2, "
+ + "getFile:f2:g1@g\\ng2@g\\n, " // f2 is included
+ + "countNumOwners, "
+ + "findOwners, "
+ + "checkFile:./t.c, "
+ + "checkDir:., "
+ + "addOwnerWeightsIn:./ "
+ + "] ";
+ String c2Response = getOwnersDebugResponse(c2);
+ assertThat(c2Response).contains(path2owners);
+ assertThat(c2Response).contains(owner2paths);
+ assertThat(c2Response).contains("file2owners:{ ./t.c:[ a@a, g1@g, g2@g, x@x ] }");
+ assertThat(c2Response).contains(ownersAG1G2X);
+ assertThat(c2Response).contains(expectedInLog);
+ // A submitted change gets owners info from current repository.
+ String c1Response = getOwnersDebugResponse(c1);
+ assertThat(c1Response).contains(path2owners);
+ assertThat(c1Response).contains(owner2paths);
+ assertThat(c1Response).contains("file2owners:{ ./OWNERS:[ a@a, g1@g, g2@g, x@x ] }");
+ assertThat(c1Response).contains(ownersAG1G2X);
+ }
+
+ @Test
+ public void includeIndirectFileTest() throws Exception {
+ // Test indirectly included file and relative file path.
+ addFile("1", "d1/f2", "d1f2@g\n");
+ addFile("2", "d2/f2", "d2f2@g\n");
+ addFile("3", "d3/f2", "d3f2@g\n");
+ addFile("4", "d1/d2/owners", "d1d2@g\ninclude ../f2\n");
+ addFile("5", "d2/d2/owners", "d2d2@g\ninclude ../f2\n");
+ addFile("6", "d3/d2/owners", "d3d2@g\ninclude ../f2\n");
+ addFile("7", "d3/OWNERS", "d3@g\ninclude ../d2/d2/owners\n");
+ addFile("8", "OWNERS", "x@g\n");
+ PushOneCommit.Result c1 = createChange("c1", "d3/t.c", "Hello!");
+ // d3's owners are in d3/OWNERS, d2/d2/owners, d2/f2, OWNERS,
+ // If the include directories are based on original directory d3,
+ // then the included files will be d2/d2/owners and d3/f2.
+ String ownerD3 = ownerJson("d3@g");
+ String ownerD2 = ownerJson("d2d2@g");
+ String ownerF2 = ownerJson("d2f2@g");
+ String ownerX = ownerJson("x@g", 0, 1, 0);
+ assertThat(getOwnersResponse(c1)).contains("owners:[ " + ownerD2 + ", "
+ + ownerF2 + ", " + ownerD3 + ", " + ownerX + " ], files:[ d3/t.c ]");
+ }
+
+ @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.
+ addFile("1", "f1", "f1@g\ninclude ./f2\n");
+ addFile("2", "f2", "f2@g\ninclude d1/../f3\n");
+ addFile("3", "f3", "f3@g\ninclude /f4\n");
+ addFile("4", "f4", "f4@g\ninclude d2/../f2\n");
+ 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 expectedInLog = "project:" + projectName + ", "
+ + "ownersFileName:OWNERS, "
+ + "getBranchId:refs/heads/master(FOUND), "
+ + "findOwnersFileFor:./t.c, "
+ + "findOwnersFileIn:., "
+ + "getFile:OWNERS:x@g\\ninclude./d1/../f1\\n, "
+ + "parseLine:include:" + projectName + ":./d1/../f1, "
+ + "getRepoFile:" + projectName + ":refs/heads/master:f1, "
+ + "getFile:f1:f1@g\\ninclude./f2\\n, "
+ + "parseLine:include:" + projectName + ":./f2, "
+ + "getRepoFile:" + projectName + ":refs/heads/master:f2, "
+ + "getFile:f2:f2@g\\nincluded1/../f3\\n, "
+ + "parseLine:include:" + projectName + ":d1/../f3, "
+ + "getRepoFile:" + projectName + ":refs/heads/master:f3, "
+ + "getFile:f3:f3@g\\ninclude/f4\\n, "
+ + "parseLine:include:" + projectName + ":/f4, "
+ + "getRepoFile:" + projectName + ":refs/heads/master:f4, "
+ + "getFile:f4:f4@g\\nincluded2/../f2\\n, "
+ + "parseLine:skip:include:" + projectName + ":d2/../f2, "
+ + "countNumOwners, "
+ + "findOwners, "
+ + "checkFile:./t.c, "
+ + "checkDir:., "
+ + "addOwnerWeightsIn:./ "
+ + "] ";
+ assertThat(response).contains("path2owners:{ ./:[ f1@g, f2@g, f3@g, f4@g, x@g ] }");
+ assertThat(response).contains(
+ "owner2paths:{ f1@g:[ ./ ], f2@g:[ ./ ], f3@g:[ ./ ], f4@g:[ ./ ], x@g:[ ./ ] }");
+ assertThat(response).contains(expectedInLog);
+ }
+
+ @Test
+ public void includeDuplicationTest() throws Exception {
+ // f0 is included into f1, f2, f3,
+ // f2 is included into f4 and f5; f4 is included into f5.
+ // f0, f1, f2, f3, f5 are included into d6/OWNERS.
+ addFile("0", "d0/f0", "f0@g\n");
+ addFile("1", "d1/d2/f1", "f1@g\ninclude ../../d0/f0\n");
+ addFile("2", "d2/f2", "f2@g\ninclude ../d0/f0\n");
+ addFile("3", "d2/d3/f3", "f3@g\ninclude /d0/f0\n");
+ addFile("4", "d4/f4", "f4@g\ninclude ../d2/f2\n");
+ addFile("5", "d4/d5/f5", "f5@g\ninclude /d2/f2\ninclude ../f4\n");
+ PushOneCommit.Result c = addFile("6", "d6/OWNERS",
+ "f6@g\ninclude /d0/f0\ninclude ../d1/d2/f1\n"
+ + "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") + ":";
+ 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, "
+ + "parseLine:include:" + projectName + ":/d0/f0, "
+ + "getRepoFile:" + projectName + ":refs/heads/master:d0/f0, "
+ + "getFile:d0/f0:f0@g\\n, "
+ + "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, "
+ + "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, "
+ + "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, "
+ + "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, "
+ + "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, "
+ + "findOwnersFileIn:., "
+ + "getFile:OWNERS(NOTFOUND), "
+ + "countNumOwners, "
+ + "findOwners, "
+ + "checkFile:./d6/OWNERS, "
+ + "checkDir:./d6, "
+ + "checkDir:., "
+ + "addOwnerWeightsIn:./d6/ "
+ + "] ";
+ assertThat(result).contains(expectedInLog);
+ }
+
+ @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.
- PushOneCommit.Result c2 = createChange("add t.c", "t.c", "Hello!");
+ PushOneCommit.Result c2 = createChange("2", "t.c", "Hello!");
String ownerA = ownerJson("a@a");
String ownerB = ownerJson("b@b");
String ownerC = ownerJson("c@c");
String ownerX = ownerJson("x@x");
assertThat(getOwnersResponse(c2)).contains("owners:[ " + ownerX + " ], files:[ t.c ]");
// Add "t.txt" file, which has new owners.
- PushOneCommit.Result c3 = createChange("add t.txt", "t.txt", "Test!");
+ PushOneCommit.Result c3 = createChange("3", "t.txt", "Test!");
assertThat(getOwnersResponse(c3))
.contains("owners:[ " + ownerA + ", " + ownerB + ", " + ownerC + " ], files:[ t.txt ]");
}
@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");
+ PushOneCommit.Result c2 = addFile("2", "d1/OWNERS", "d1@g\nper-file OWNERS=d1o@g\n");
+ PushOneCommit.Result c3 = addFile("3", "d2/d1/OWNERS", "d2d1@g\ninclude ../../d1/d1/OWNERS\n");
+ PushOneCommit.Result c4 = addFile("4", "d2/OWNERS", "d2@g\nper-file OWNERS=d2o@g");
+ assertThat(getOwnersResponse(c1)).contains("{ ./d1/d1/OWNERS:[ d1d1o@g, d1o@g ] }");
+ assertThat(getOwnersResponse(c2)).contains("{ ./d1/OWNERS:[ d1o@g ] }");
+ assertThat(getOwnersResponse(c3)).contains("{ ./d2/d1/OWNERS:[ d1d1o@g, d2o@g ] }");
+ assertThat(getOwnersResponse(c4)).contains("{ ./d2/OWNERS:[ d2o@g ] }");
+ }
+
+ @Test
+ public void includeNoParentTest() throws Exception {
+ // Test included file with noparent, which affects the inheritance of including file.
+ PushOneCommit.Result c1 = addFile("1", "d1/d1/OWNERS", "d1d1@g\nset noparent\n");
+ PushOneCommit.Result c2 = addFile("2", "d1/d2/OWNERS", "d1d2@g\n");
+ PushOneCommit.Result c3 = addFile("3", "d1/OWNERS", "d1@g\n");
+ PushOneCommit.Result c4 = addFile("4", "d2/d1/OWNERS", "d2d1@g\ninclude ../../d1/d1/OWNERS\n");
+ PushOneCommit.Result c5 = addFile("5", "d2/d2/OWNERS", "d2d2@g\ninclude ../../d1/d2/OWNERS");
+ PushOneCommit.Result c6 = addFile("6", "d2/OWNERS", "d2@g\n");
+ // d1/d1/OWNERS sets noparent, does not inherit d1/OWNERS
+ assertThat(getOwnersResponse(c1)).contains("{ ./d1/d1/OWNERS:[ d1d1@g ] }");
+ // d1/d2/OWNERS inherits d1/OWNERS
+ assertThat(getOwnersResponse(c2)).contains("{ ./d1/d2/OWNERS:[ d1@g, d1d2@g ] }");
+ assertThat(getOwnersResponse(c3)).contains("{ ./d1/OWNERS:[ d1@g ] }");
+ // d2/d1/OWNERS includes d1/d1/OWNERS, does not inherit d1/OWNERS or d2/OWNERS
+ assertThat(getOwnersResponse(c4)).contains("{ ./d2/d1/OWNERS:[ d1d1@g, d2d1@g ] }");
+ // d2/d2/OWNERS includes d1/d1/OWNERS, inherit d2/OWNERS but not d1/OWNERS
+ assertThat(getOwnersResponse(c5)).contains("{ ./d2/d2/OWNERS:[ d1d2@g, d2@g, d2d2@g ] }");
+ assertThat(getOwnersResponse(c6)).contains("{ ./d2/OWNERS:[ d2@g ] }");
+ }
+
+ @Test
+ public void includeProjectOwnerTest() throws Exception {
+ // Test include directive with other project name.
+ String testName = "includeProjectOwnerTest";
+ String projectA = "A1/A2/A3";
+ String projectB = "B1/B2/B3";
+ Project.NameKey pA = createProject(projectA);
+ Project.NameKey pB = createProject(projectB);
+ String nameA = pA.get();
+ String nameB = pB.get();
+ assertThat(nameA).isEqualTo(myProjectName(testName, projectA));
+ assertThat(nameB).isEqualTo(myProjectName(testName, projectB));
+ switchProject(pA);
+ addFile("1", "f1", "pAf1@g\ninclude ./d1/f1\n");
+ addFile("2", "d1/f1", "pAd1f1@g\ninclude " + nameB + ":" + "/d2/f2\n");
+ addFile("3", "d2/OWNERS", "pAd2@g\n include " + nameA + " : " + "../f1\n");
+ addFile("4", "OWNERS", "pA@g\n");
+ switchProject(pB);
+ addFile("5", "f1", "pBf1@g\ninclude ./d1/f1\n");
+ addFile("6", "f2", "pBf2@g\n");
+ addFile("7", "d1/f1", "pBd1f1@g\n");
+ addFile("8", "d2/f2", "pBd2f2@g\ninclude ../f1\n");
+ switchProject(pA);
+ PushOneCommit.Result c1 = createChange("c1", "d2/t.c", "Hello!");
+ // included: pA:d2/OWNERS, pA:d2/../f1, pA:d1/f1, pB:d2/f2, pB:d2/../f1, pB:./d1/f1
+ // inherited: pA:OWNERS
+ String owners = "owners:[ " + ownerJson("pAd1f1@g") + ", " + ownerJson("pAd2@g") + ", "
+ + ownerJson("pAf1@g") + ", " + ownerJson("pBd1f1@g") + ", " + ownerJson("pBd2f2@g")
+ + ", " + ownerJson("pBf1@g") + ", " + ownerJson("pA@g", 0, 1, 0) + " ]";
+ assertThat(getOwnersResponse(c1)).contains(owners);
+ }
+
+ @Test
public void subOwnersFileTest() throws Exception {
// Add OWNERS file in root and subdirectories.
- addFile("add OWNERS", "OWNERS", "x@x\n");
- addFile("add d1/OWNERS", "d1/OWNERS", "a@a\n");
- addFile("add d2/OWNERS", "d2/OWNERS", "y@y\n");
- addFile("add d3/OWNERS", "d3/OWNERS", "b@b\nset noparent\n");
+ addFile("1", "OWNERS", "x@x\n");
+ addFile("2", "d1/OWNERS", "a@a\n");
+ addFile("3", "d2/OWNERS", "y@y\n");
+ addFile("4", "d3/OWNERS", "b@b\nset noparent\n");
+ addFile("5", "d4/OWNERS", "z@z\ninclude ../d2/OWNERS");
// Add "t.c" file, which is not owned by subdirectory owners.
- PushOneCommit.Result c2 = createChange("add t.c", "t.c", "Hello!");
+ PushOneCommit.Result c2 = createChange("c2", "t.c", "Hello!");
String ownerA = ownerJson("a@a");
String ownerX = ownerJson("x@x");
assertThat(getOwnersResponse(c2)).contains("owners:[ " + ownerX + " ], files:[ t.c ]");
// Add "d1/t.c" file, which is owned by ./d1 and root owners.
- PushOneCommit.Result c3 = createChange("add d1/t.c", "d1/t.c", "Hello!");
+ PushOneCommit.Result c3 = createChange("c3", "d1/t.c", "Hello!");
String ownerX010 = ownerJson("x@x", 0, 1, 0);
assertThat(getOwnersResponse(c3))
.contains("owners:[ " + ownerA + ", " + ownerX010 + " ], files:[ d1/t.c ]");
// Add "d2/t.c" file, which is owned by ./d2 and root owners.
- PushOneCommit.Result c4 = createChange("add d2/t.c", "d2/t.c", "Hello!");
+ PushOneCommit.Result c4 = createChange("c4", "d2/t.c", "Hello!");
String ownerY = ownerJson("y@y");
assertThat(getOwnersResponse(c4))
.contains("owners:[ " + ownerY + ", " + ownerX010 + " ], files:[ d2/t.c ]");
// Add "d2/d1/t.c" file, which is owned by ./d2 and root owners.
- PushOneCommit.Result c5 = createChange("add d2/d1/t.c", "d2/d1/t.c", "Hello!");
- assertThat(getOwnersResponse(c5))
- .contains("owners:[ " + ownerY + ", " + ownerX010 + " ], files:[ d2/d1/t.c ]");
+ PushOneCommit.Result c5 = createChange("c5", "d2/d1/t.c", "Hello!");
+ assertThat(getOwnersResponse(c5)).contains(
+ "owners:[ " + ownerY + ", " + ownerX010 + " ], files:[ d2/d1/t.c ]");
// Add "d3/t.c" file, which is owned only by ./d3 owners due to "set noparent".
- PushOneCommit.Result c6 = createChange("add d3/t.c", "d3/t.c", "Hello!");
+ PushOneCommit.Result c6 = createChange("c6", "d3/t.c", "Hello!");
String ownerB = ownerJson("b@b");
assertThat(getOwnersResponse(c6)).contains("owners:[ " + ownerB + " ], files:[ d3/t.c ]");
// Add "d3/d1/t.c" file, which is owned only by ./d3 owners due to "set noparent".
- PushOneCommit.Result c7 = createChange("add d3/d1/t.c", "d3/d1/t.c", "Hello!");
- assertThat(getOwnersResponse(c7)).contains("owners:[ " + ownerB + " ], files:[ d3/d1/t.c ]");
+ PushOneCommit.Result c7 = createChange("c7", "d3/d1/t.c", "Hello!");
+ assertThat(getOwnersResponse(c7)).contains(
+ "owners:[ " + ownerB + " ], files:[ d3/d1/t.c ]");
+ // Add "d4/t.c" file, which is owned by ./d4 and ./d2 owners, but not root owners.
+ PushOneCommit.Result c8 = createChange("c8", "d4/t.c", "Hello!");
+ String ownerZ = ownerJson("z@z");
+ assertThat(getOwnersResponse(c8)).contains(
+ "owners:[ " + ownerY + ", " + ownerZ + ", " + ownerX010 + " ], files:[ d4/t.c ]");
}
@Test
public void requestErrorTest() throws Exception {
- PushOneCommit.Result c1 = createChange("add t.c", "t.c", "##");
+ PushOneCommit.Result c1 = createChange("1", "t.c", "##");
assertThat(getOwnersResponse(c1)).contains("owners:[], files:[ t.c ]");
int id = c1.getChange().getId().get();
// Correct change id.
@@ -231,8 +505,7 @@
public void projectTest() throws Exception {
RestResponse response = adminRestSession.get("/projects/?d");
String content = response.getEntityContent();
- // Predefined projects: "All-Projects", "All-Users",
- // "com.googlesource.gerrit.plugins.findowners.FindOwnersIT_projectTest_project"
+ // Predefined projects: "All-Projects", "All-Users", myProjectName("projectTest")
assertThat(content).contains("\"id\": \"All-Projects\",");
assertThat(content).contains("\"id\": \"All-Users\",");
assertThat(content).contains(idProject("projectTest", "project"));
@@ -245,26 +518,19 @@
@Test
public void ownersFileNameTest() throws Exception {
Config.setVariables("find-owners", configFactory);
-
// Default project is something like ....FindOwnersIT..._project
Project.NameKey pA = createProject("Project_A");
Project.NameKey pB = createProject("Project_B");
- switchProject(pA);
- // Now project is ....FindOwnersIT..._Project_A
- switchProject(pB);
- // Now project is ....FindOwnersIT..._Project_B
-
// Add OWNERS and OWNERS.alpha file to Project_A.
switchProject(pA);
- addFile("add OWNERS", "OWNERS", "per-file *.c=x@x\n"); // default owner x@x
- addFile("add OWNERS.alpha", "OWNERS.alpha", "per-file *.c=a@a\n"); // alpha owner a@a
- PushOneCommit.Result cA = createChange("add tA.c", "tA.c", "Hello A!");
-
+ addFile("1", "OWNERS", "per-file *.c=x@x\n"); // default owner x@x
+ addFile("2", "OWNERS.alpha", "per-file *.c=a@a\n"); // alpha owner a@a
+ PushOneCommit.Result cA = createChange("cA", "tA.c", "Hello A!");
// Add OWNERS and OWNERS.beta file to Project_B.
switchProject(pB);
- addFile("add OWNERS", "OWNERS", "per-file *.c=y@y\n"); // default owner y@y
- addFile("add OWNERS.beta", "OWNERS.beta", "per-file *.c=b@b\n"); // beta owner b@b
- PushOneCommit.Result cB = createChange("add tB.c", "tB.c", "Hello B!");
+ addFile("3", "OWNERS", "per-file *.c=y@y\n"); // default owner y@y
+ addFile("4", "OWNERS.beta", "per-file *.c=b@b\n"); // beta owner b@b
+ PushOneCommit.Result cB = createChange("cB", "tB.c", "Hello B!");
// Default owners file name is "OWNERS".
assertThat(Config.getDefaultOwnersFileName()).isEqualTo("OWNERS");
@@ -315,12 +581,12 @@
@Test
public void authorDefaultVoteTest() throws Exception {
// CL author has default +1 owner vote.
- addFile("add d1/OWNERS", "d1/OWNERS", user.email + "\n"); // d1 owned by user
- addFile("add d2/OWNERS", "d2/OWNERS", admin.email + "\n"); // d2 owned by admin
+ addFile("1", "d1/OWNERS", user.email + "\n"); // d1 owned by user
+ addFile("2", "d2/OWNERS", admin.email + "\n"); // d2 owned by admin
// admin is the author of CLs created by createChange.
- PushOneCommit.Result r1 = createChange("add d1/t.c", "d1/t.c", "Hello1");
- PushOneCommit.Result r2 = createChange("add d2/t.c", "d2/t.c", "Hello2");
- PushOneCommit.Result r3 = createChange("add d3/t.c", "d3/t.c", "Hello3");
+ PushOneCommit.Result r1 = createChange("r1", "d1/t.c", "Hello1");
+ PushOneCommit.Result r2 = createChange("r2", "d2/t.c", "Hello2");
+ PushOneCommit.Result r3 = createChange("r3", "d3/t.c", "Hello3");
assertThat(checkApproval(r1)).isEqualTo(-1); // owner is not change author
assertThat(checkApproval(r2)).isEqualTo(1); // owner is change author, default +1
assertThat(checkApproval(r3)).isEqualTo(0); // no owner is found in d3
@@ -412,8 +678,11 @@
gApi.changes().id(change.getChangeId()).current().submit(new SubmitInput());
}
- private void addFile(String subject, String file, String content) throws Exception {
- approveSubmit(createChange(subject, file, content));
+ private PushOneCommit.Result addFile(
+ String subject, String file, String content) throws Exception {
+ PushOneCommit.Result c = createChange(subject, file, content);
+ approveSubmit(c);
+ return c;
}
private void switchProject(Project.NameKey p) throws Exception {
@@ -454,11 +723,10 @@
private int checkApproval(PushOneCommit.Result r) throws Exception {
Project.NameKey project = r.getChange().project();
- Repository repo = repoManager.openRepository(project);
Cache cache = Cache.getInstance().init(0, 0);
- OwnersDb db =
- cache.get(true, projectCache.get(project), accountCache, emails, repo, r.getChange(), 1);
- Checker c = new Checker(repo, r.getChange(), 1);
+ OwnersDb db = cache.get(true, projectCache.get(project), accountCache, emails,
+ repoManager, r.getChange(), 1);
+ Checker c = new Checker(repoManager, r.getChange(), 1);
return c.findApproval(accountCache, db);
}
@@ -472,11 +740,17 @@
return filteredJson(response.getEntityContent());
}
- private static String idProject(String test, String project) {
+ private String myProjectName(String test) {
+ return myProjectName(test, "project");
+ }
+
+ private String myProjectName(String test, String project) {
+ return this.getClass().getName() + "_" + test + "_" + project;
+ }
+
+ private String idProject(String test, String project) {
// Expected string of "id": "project_name",
- return String.format(
- "\"id\": \"com.googlesource.gerrit.plugins.findowners.FindOwnersIT_" + "%s_%s\",",
- test, project);
+ return "\"id\": \"" + myProjectName(test, project) + "\",";
}
private static void verifyRestResult(
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 3403bcc..bd6de95 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
@@ -257,7 +257,7 @@
private List<String> validate(RevWalk rw, RevCommit c, boolean verbose, PluginConfig cfg)
throws Exception {
MockedEmails myEmails = new MockedEmails();
- OwnersValidator validator = new OwnersValidator(null, null, null, myEmails);
+ OwnersValidator validator = new OwnersValidator(null, null, myEmails);
String ownersFileName = OwnersValidator.getOwnersFileName(cfg);
List<CommitValidationMessage> m = validator.performValidation(c, rw, ownersFileName, verbose);
return transformMessages(m);
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 2f50420..2020bd3 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java
@@ -34,9 +34,15 @@
return "./d1/d2/";
}
+ private static String mockedProject() {
+ return "myTestProject";
+ }
+
private static Parser.Result testLine(String line) {
Parser.Result result = new Parser.Result();
- Parser.parseLine(result, mockedTestDir(), "OWNERS", line, 3);
+ // Single line parser tests do not need repoManager.
+ Parser parser = new Parser(null, mockedProject(), "master", "OWNERS");
+ parser.parseLine(result, mockedTestDir(), line, 3);
return result;
}
@@ -160,6 +166,24 @@
}
}
+ private void testOneIncludeLine(
+ String project, String line, String parsedProject, String parsedFile) {
+ String[] results = Parser.parseInclude(project, line);
+ assertThat(results).hasLength(2);
+ assertThat(results[0]).isEqualTo(parsedProject);
+ assertThat(results[1]).isEqualTo(parsedFile);
+ }
+
+ @Test
+ public void includeLineTest() {
+ testOneIncludeLine("P0", " include /common/OWNERS #comment", "P0", "/common/OWNERS");
+ testOneIncludeLine("P1", "include Other : /Group ", "Other", "/Group");
+ testOneIncludeLine("P2", "include /Common/Project: OWNER", "/Common/Project", "OWNER");
+ testOneIncludeLine("P3", " include \tP2/D2:/D3/F.txt", "P2/D2", "/D3/F.txt");
+ testOneIncludeLine("P4", "\t include \t P2/D2:\t/D3/F2.txt\n", "P2/D2", "/D3/F2.txt");
+ testOneIncludeLine("P5", "include ../d1/d2/F \n", "P5", "../d1/d2/F");
+ }
+
@Test
public void errorMsgTest() {
String file = "./OWNERS";
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/UtilTest.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/UtilTest.java
index bf7c8a7..43451c4 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/UtilTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/UtilTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
+import java.io.IOException;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
@@ -122,20 +123,42 @@
}
@Test
- public void normalizeFilePathTest() {
+ public void addDotPrefixTest() {
String[] files = {"", "./d1/", "d1/d2/f1.c", "d2/f2/", "d1"};
String[] results = {"./", "./d1/", "./d1/d2/f1.c", "./d2/f2/", "./d1"};
for (int i = 0; i < files.length; i++) {
- assertThat(Util.normalizedFilePath(files[i])).isEqualTo(results[i]);
+ assertThat(Util.addDotPrefix(files[i])).isEqualTo(results[i]);
}
}
@Test
- public void normalizedDirPathTest() {
+ public void getParentDirTest() {
String[] files = {"", "./d1/", "d1/d2/f1.c", "./d2/f2.c", "./d1"};
String[] dirs = {null, ".", "./d1/d2", "./d2", "."};
for (int i = 0; i < files.length; i++) {
- assertThat(Util.normalizedDirPath(files[i])).isEqualTo(dirs[i]);
+ assertThat(Util.getParentDir(files[i])).isEqualTo(dirs[i]);
+ }
+ }
+
+ @Test
+ public void gitRepoFilePathTest() {
+ String[] inFiles = {null, "", "//", "./d1/", "d1/d2", "///d1/f//", "/././/f2", "/./f3"};
+ String[] outFiles = {"", "", "", "d1", "d1/d2", "d1/f", "f2", "f3"};
+ for (int i = 0; i < inFiles.length; i++) {
+ assertThat(Util.gitRepoFilePath(inFiles[i])).isEqualTo(outFiles[i]);
+ }
+ }
+
+ @Test
+ public void normalizedDirFilePathTest() throws IOException {
+ String[] dirs = {"", "", ".", "./d1/..", "/d1/d2/../..",
+ "d1", "d2/../d1", "/d1", "/d1", "/d1"};
+ String[] files = {"f0.c", "./f1.c", "./f2", "/f3", "f4",
+ "f5", "f6", "f7", "f8", "../d5/../d1/f9"};
+ String[] paths = {"./f0.c", "./f1.c", "./f2", "./f3", "./f4",
+ "./d1/f5", "./d1/f6", "./d1/f7", "./d1/f8", "./d1/f9"};
+ for (int i = 0; i < files.length; i++) {
+ assertThat(Util.normalizedDirFilePath(dirs[i], files[i])).isEqualTo(paths[i]);
}
}