Merge branch 'stable-2.14' into stable-2.15
* stable-2.14:
Fix test dependencies
Change-Id: I2e02db7984cb105ae950e8ed8342d52a8e09acf9
diff --git a/BUILD b/BUILD
index 8cb3515..0371641 100644
--- a/BUILD
+++ b/BUILD
@@ -13,7 +13,7 @@
srcs = glob(["src/main/prolog/*.pl"]),
deps = [
":find-owners-lib",
- "//gerrit-server/src/main/prolog:common",
+ "//gerrit-server:prolog-common",
],
)
@@ -40,6 +40,7 @@
# resources = glob(['src/test/resources/**/*']),
tags = ["findowners"],
deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [
+ "@commons-io//jar",
":find-owners-lib",
":find-owners-prolog-rules",
],
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 13a1cc9..d232382 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -27,6 +27,7 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.PluginConfigFactory;
@@ -54,6 +55,7 @@
private static final Logger log = LoggerFactory.getLogger(Action.class);
private AccountCache accountCache;
+ private Emails emails;
private ChangeData.Factory changeDataFactory;
private GitRepositoryManager repoManager;
private Provider<CurrentUser> userProvider;
@@ -72,11 +74,13 @@
SchemaFactory<ReviewDb> reviewDbProvider,
ChangeData.Factory changeDataFactory,
AccountCache accountCache,
+ Emails emails,
GitRepositoryManager repoManager) {
this.userProvider = userProvider;
this.reviewDbProvider = reviewDbProvider;
this.changeDataFactory = changeDataFactory;
this.accountCache = accountCache;
+ this.emails = emails;
this.repoManager = repoManager;
Config.setVariables(pluginName, configFactory);
Cache.getInstance(); // Create a single Cache.
@@ -139,7 +143,7 @@
result.add(account.getPreferredEmail());
}
} catch (OrmException e) {
- log.error("Exception", e);
+ log.error("Exception for " + Config.getChangeId(changeData), e);
result = new ArrayList<>();
}
return result;
@@ -165,9 +169,9 @@
/** REST API to return owners info of a change. */
public Response<RestResult> getChangeData(
Repository repository, Parameters params, ChangeData changeData)
- throws OrmException, BadRequestException {
+ throws OrmException, BadRequestException, IOException {
int patchset = getValidPatchsetNum(changeData, params.patchset);
- OwnersDb db = Cache.getInstance().get(repository, changeData, patchset);
+ OwnersDb db = Cache.getInstance().get(accountCache, emails, repository, changeData, patchset);
Collection<String> changedFiles = changeData.currentFilePaths();
Map<String, Set<String>> file2Owners = db.findOwners(changedFiles);
@@ -180,6 +184,7 @@
obj.dbgmsgs.user = getUserName();
obj.dbgmsgs.project = changeData.change().getProject().get();
obj.dbgmsgs.branch = changeData.change().getDest().get();
+ obj.dbgmsgs.errors = db.errors;
obj.dbgmsgs.path2owners = Util.makeSortedMap(db.path2Owners);
obj.dbgmsgs.owner2paths = Util.makeSortedMap(db.owner2Paths);
}
@@ -194,10 +199,13 @@
@Override
public Description getDescription(RevisionResource resource) {
Change change = resource.getChangeResource().getChange();
+ ChangeData changeData = null;
try (ReviewDb reviewDb = reviewDbProvider.open()) {
- ChangeData changeData = changeDataFactory.create(reviewDb, change);
+ changeData = changeDataFactory.create(reviewDb, change);
if (changeData.change().getDest().get() == null) {
- log.error("Cannot get branch of change: " + changeData.getId().get());
+ if (!Checker.isExemptFromOwnerApproval(changeData)) {
+ log.error("Cannot get branch of change: " + changeData.getId().get());
+ }
return null; // no "Find Owners" button
}
Status status = resource.getChange().getStatus();
@@ -211,7 +219,7 @@
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(repo, changeData);
+ OwnersDb db = Cache.getInstance().get(accountCache, emails, repo, changeData);
log.trace("getDescription db key = " + db.key);
needFindOwners = db.getNumOwners() > 0;
}
@@ -221,7 +229,7 @@
.setTitle("Find owners to add to Reviewers list")
.setVisible(needFindOwners);
} catch (IOException | OrmException e) {
- log.error("Exception", e);
+ log.error("Exception for " + Config.getChangeId(changeData), e);
throw new IllegalStateException(e);
}
}
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 48f86e6..aa43896 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
@@ -18,8 +18,11 @@
import com.google.common.cache.CacheBuilder;
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.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
+import java.io.IOException;
import java.util.Collection;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
@@ -85,29 +88,48 @@
}
/** Returns a cached or new OwnersDb, for the current patchset. */
- OwnersDb get(Repository repo, ChangeData changeData) throws OrmException {
- return get(repo, changeData, changeData.currentPatchSet().getId().get());
+ OwnersDb get(AccountCache accountCache, Emails emails, Repository repo, ChangeData changeData)
+ throws OrmException, IOException {
+ return get(accountCache, emails, repo, changeData, changeData.currentPatchSet().getId().get());
}
/** Returns a cached or new OwnersDb, for the specified patchset. */
- OwnersDb get(Repository repository, ChangeData changeData, int patchset) throws OrmException {
+ OwnersDb get(
+ AccountCache accountCache,
+ Emails emails,
+ Repository repository,
+ ChangeData changeData,
+ int patchset)
+ throws OrmException, IOException {
Project.NameKey project = changeData.change().getProject();
String branch = changeData.change().getDest().get();
String dbKey = Cache.makeKey(changeData.getId().get(), patchset, branch);
// TODO: get changed files of the given patchset?
- return get(dbKey, repository, project, branch, changeData.currentFilePaths());
+ return get(
+ accountCache,
+ emails,
+ dbKey,
+ repository,
+ changeData,
+ project,
+ branch,
+ changeData.currentFilePaths());
}
/** Returns a cached or new OwnersDb, for the specified branch and changed files. */
OwnersDb get(
+ AccountCache accountCache,
+ Emails emails,
String key,
Repository repository,
+ ChangeData changeData,
Project.NameKey project,
String branch,
Collection<String> files) {
if (dbCache == null) { // Do not cache OwnersDb
log.trace("Create new OwnersDb, key=" + key);
- return new OwnersDb(key, repository, project, branch, files);
+ return new OwnersDb(
+ accountCache, emails, key, repository, changeData, project, branch, files);
}
try {
log.trace("Get from cash " + dbCache + ", key=" + key + ", cache size=" + dbCache.size());
@@ -117,12 +139,14 @@
@Override
public OwnersDb call() {
log.trace("Create new OwnersDb, key=" + key);
- return new OwnersDb(key, repository, project, branch, files);
+ return new OwnersDb(
+ accountCache, emails, key, repository, changeData, project, branch, files);
}
});
} catch (ExecutionException e) {
- log.error("Cache.get has exception: " + e);
- return new OwnersDb(key, repository, project, branch, files);
+ log.error("Cache.get has exception for " + Config.getChangeId(changeData), e);
+ return new OwnersDb(
+ accountCache, emails, key, repository, changeData, project, 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 8cc92ac..ae163d5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -14,12 +14,11 @@
package com.googlesource.gerrit.plugins.findowners;
-import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.server.AccountAccess;
-import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.rules.StoredValues;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.googlecode.prolog_cafe.lang.Prolog;
@@ -50,20 +49,22 @@
}
/** Returns a map from reviewer email to vote value. */
- static Map<String, Integer> getVotes(ChangeData changeData) throws OrmException {
- ReviewDb db = changeData.db();
+ Map<String, Integer> getVotes(AccountCache accountCache, ChangeData changeData)
+ throws OrmException {
Map<String, Integer> map = new HashMap<>();
- AccountAccess ac = db.accounts();
for (PatchSetApproval p : changeData.currentApprovals()) {
if (p.getValue() != 0) {
- Account.Id id = p.getAccountId();
- try {
- map.put(ac.get(id).getPreferredEmail(), Integer.valueOf(p.getValue()));
- } catch (OrmException e) {
- log.error("Cannot get email address of account id: " + id.get(), e);
- }
+ map.put(
+ accountCache.get(p.getAccountId()).getAccount().getPreferredEmail(),
+ Integer.valueOf(p.getValue()));
}
}
+ // Give CL author a default minVoteLevel vote.
+ String author =
+ accountCache.get(changeData.change().getOwner()).getAccount().getPreferredEmail();
+ if (!map.containsKey(author) || map.get(author) == 0) {
+ map.put(author, minVoteLevel);
+ }
return map;
}
@@ -74,8 +75,6 @@
for (String owner : owners) {
if (votes.containsKey(owner)) {
int v = votes.get(owner);
- // TODO: Maybe add a configurable feature in the next version
- // to exclude the committer's vote from the "foundApproval".
foundApproval |= (v >= minVoteLevel);
foundVeto |= (v < 0); // an owner's -1 vote is a veto
} else if (owner.equals("*")) {
@@ -86,12 +85,12 @@
}
/** Returns 1 if owner approval is found, -1 if missing, 0 if unneeded. */
- int findApproval(OwnersDb db) throws OrmException {
+ int findApproval(AccountCache accountCache, OwnersDb db) throws OrmException, IOException {
Map<String, Set<String>> file2Owners = db.findOwners(changeData.currentFilePaths());
if (file2Owners.size() == 0) { // do not need owner approval
return 0;
}
- Map<String, Integer> votes = getVotes(changeData);
+ Map<String, Integer> votes = getVotes(accountCache, changeData);
for (Set<String> owners : file2Owners.values()) {
if (!findOwnersInVotes(owners, votes)) {
return -1;
@@ -102,12 +101,15 @@
/** Returns 1 if owner approval is found, -1 if missing, 0 if unneeded. */
public static int findApproval(Prolog engine, int minVoteLevel) {
+ ChangeData changeData = null;
try {
- ChangeData changeData = StoredValues.CHANGE_DATA.get(engine);
+ changeData = StoredValues.CHANGE_DATA.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).findApproval();
- } catch (OrmException e) {
- log.error("Exception", e);
+ return new Checker(repository, changeData, minVoteLevel).findApproval(accountCache, emails);
+ } catch (OrmException | IOException e) {
+ log.error("Exception for " + Config.getChangeId(changeData), e);
return 0; // owner approval may or may not be required.
}
}
@@ -120,7 +122,7 @@
return true;
}
} catch (IOException | OrmException e) {
- log.error("Cannot get commit message", e);
+ log.error("Cannot get commit message for " + Config.getChangeId(changeData), e);
return true; // exempt from owner approval due to lack of data
}
// Abandoned and merged changes do not need approval again.
@@ -128,13 +130,13 @@
return (status == Status.ABANDONED || status == Status.MERGED);
}
- int findApproval() throws OrmException {
+ int findApproval(AccountCache accountCache, Emails emails) throws OrmException, IOException {
if (isExemptFromOwnerApproval(changeData)) {
return 0;
}
// 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(repository, changeData);
+ OwnersDb db = Cache.getInstance().get(accountCache, emails, repository, changeData);
if (db.getNumOwners() <= 0) {
return 0;
}
@@ -142,6 +144,6 @@
minVoteLevel = Config.getMinOwnerVoteLevel(changeData);
}
log.trace("findApproval db key = " + db.key);
- return findApproval(db);
+ return findApproval(accountCache, db);
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
index d7008b3..e450dfb 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Config.java
@@ -32,16 +32,15 @@
static final String MAX_CACHE_AGE = "maxCacheAge"; // seconds to stay in cache
static final String MAX_CACHE_SIZE = "maxCacheSize"; // number of OwnersDb in cache
static final String MIN_OWNER_VOTE_LEVEL = "minOwnerVoteLevel"; // default +1
- static final String OWNERS_FILE_NAME = "ownersFileName"; // default "OWNERS"
+ static final String OWNERS = "OWNERS"; // Default file name
+ static final String OWNERS_FILE_NAME = "ownersFileName"; // config key for file name
+ static final String REJECT_ERROR_IN_OWNERS = "rejectErrorInOwners"; // config key for validator
static final String REPORT_SYNTAX_ERROR = "reportSyntaxError";
// Name of plugin and namespace.
static final String PLUGIN_NAME = "find-owners";
static final String PROLOG_NAMESPACE = "find_owners";
- // Default values.
- private static final String DEFAULT_OWNERS_FILE_NAME = "OWNERS";
-
// Global/plugin config parameters.
private static PluginConfigFactory config = null;
private static boolean addDebugMsg = false;
@@ -88,24 +87,35 @@
return alwaysShowButton;
}
- static String getOwnersFileName(Project.NameKey project) {
+ static String getChangeId(ChangeData data) {
+ return data == null ? "(unknown change)" : ("change c/" + data.getId().get());
+ }
+
+ static String getOwnersFileName(Project.NameKey project, ChangeData c) {
if (config != null && project != null) {
try {
String name =
config
.getFromProjectConfigWithInheritance(project, PLUGIN_NAME)
- .getString(OWNERS_FILE_NAME, DEFAULT_OWNERS_FILE_NAME);
+ .getString(OWNERS_FILE_NAME, OWNERS);
if (name.trim().equals("")) {
log.error(
- "Project " + project.get() + " has wrong " + OWNERS_FILE_NAME + ": \"" + name + "\"");
- return DEFAULT_OWNERS_FILE_NAME;
+ "Project "
+ + project
+ + " has wrong "
+ + OWNERS_FILE_NAME
+ + ": \""
+ + name
+ + "\" for "
+ + getChangeId(c));
+ return OWNERS;
}
return name;
} catch (NoSuchProjectException e) {
- log.error("Cannot find project: " + project);
+ log.error("Cannot find project " + project + " for " + getChangeId(c), e);
}
}
- return DEFAULT_OWNERS_FILE_NAME;
+ return OWNERS;
}
@VisibleForTesting
@@ -122,7 +132,7 @@
.getFromProjectConfigWithInheritance(project, PLUGIN_NAME)
.getInt(MIN_OWNER_VOTE_LEVEL, minOwnerVoteLevel);
} catch (NoSuchProjectException e) {
- log.error("Cannot find project: " + project);
+ log.error("Cannot find project " + project + " for " + getChangeId(changeData), e);
return minOwnerVoteLevel;
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
index d4e45ef..fd60850 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
@@ -21,6 +21,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -56,6 +57,7 @@
SchemaFactory<ReviewDb> reviewDbProvider,
ChangeData.Factory dataFactory,
AccountCache accountCache,
+ Emails emails,
GitRepositoryManager repoManager) {
this.action =
new Action(
@@ -65,6 +67,7 @@
reviewDbProvider,
dataFactory,
accountCache,
+ emails,
repoManager);
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java
index 9fc2709..e57bd51 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Module.java
@@ -47,6 +47,7 @@
@Override
protected void configure() {
+ install(OwnersValidator.module());
install(
new RestApiModule() {
@Override
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 b2ce1f9..2c595b2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -16,8 +16,12 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.collect.Multimap;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
-import java.io.IOException;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Emails;
+import com.google.gerrit.server.query.change.ChangeData;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.PathMatcher;
@@ -26,8 +30,10 @@
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Set;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevTree;
@@ -40,6 +46,8 @@
class OwnersDb {
private static final Logger log = LoggerFactory.getLogger(OwnersDb.class);
+ private AccountCache accountCache;
+ private Emails emails;
private int numOwners = -1; // # of owners of all given files.
String key = ""; // key to find this OwnersDb in a cache.
@@ -49,42 +57,54 @@
Map<String, Set<String>> path2Owners = new HashMap<>(); // dir or file glob to owner emails
Set<String> readDirs = new HashSet<>(); // directories in which we have checked OWNERS
Set<String> stopLooking = new HashSet<>(); // directories where OWNERS has "set noparent"
+ Map<String, String> preferredEmails = new HashMap<>(); // owner email to preferred email
+ List<String> errors = new ArrayList<>(); // error messages
OwnersDb() {}
OwnersDb(
+ AccountCache accountCache,
+ Emails emails,
String key,
Repository repository,
+ ChangeData changeData,
Project.NameKey project,
String branch,
Collection<String> files) {
+ this.accountCache = accountCache;
+ this.emails = emails;
this.key = key;
- String ownersFileName = Config.getOwnersFileName(project);
- 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
- while (!readDirs.contains(dir)) {
- readDirs.add(dir);
- String filePath = (dir + "/" + ownersFileName).substring(2); // remove "./"
- String content = getRepositoryFile(repository, branch, filePath);
- if (content != null && !content.equals("")) {
- addFile(dir + "/", dir + "/" + ownersFileName, content.split("\\R+"));
+ preferredEmails.put("*", "*");
+ String ownersFileName = Config.getOwnersFileName(project, changeData);
+ // Some hacked CL could have a target branch that is not created yet.
+ ObjectId id = getBranchId(repository, branch, changeData);
+ 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
+ while (!readDirs.contains(dir)) {
+ readDirs.add(dir);
+ String filePath = (dir + "/" + ownersFileName).substring(2); // remove "./"
+ String content = getRepositoryFile(repository, id, filePath);
+ if (content != null && !content.equals("")) {
+ addFile(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 = repository.getRef(branch).getObjectId().getName();
+ } catch (Exception e) {
+ log.error("Fail to get branch revision for " + Config.getChangeId(changeData), e);
}
}
countNumOwners(files);
- try {
- revision = repository.getRef(branch).getObjectId().getName();
- } catch (IOException e) {
- log.error("Fail to get branch revision", e);
- revision = "";
- }
}
int getNumOwners() {
@@ -110,14 +130,50 @@
}
}
+ void addPreferredEmails(Set<String> ownerEmails) {
+ List<String> owners = new ArrayList<>(ownerEmails);
+ owners.removeIf(o -> preferredEmails.get(o) != null);
+ if (!owners.isEmpty()) {
+ String[] ownerEmailsAsArray = new String[owners.size()];
+ owners.toArray(ownerEmailsAsArray);
+ Multimap<String, Account.Id> email2ids = null;
+ try {
+ email2ids = emails.getAccountsFor(ownerEmailsAsArray);
+ } catch (Exception e) {
+ log.error("accounts.byEmails failed with exception: ", e);
+ }
+ for (String owner : ownerEmailsAsArray) {
+ String email = owner;
+ try {
+ if (email2ids == null) {
+ errors.add(owner);
+ } else {
+ Collection<Account.Id> ids = email2ids.get(owner);
+ if (ids == null || ids.size() != 1) {
+ errors.add(owner);
+ } else {
+ email = accountCache.get(ids.iterator().next()).getAccount().getPreferredEmail();
+ }
+ }
+ } catch (Exception e) {
+ log.error("Fail to find preferred email of " + owner, e);
+ errors.add(owner);
+ }
+ preferredEmails.put(owner, email);
+ }
+ }
+ }
+
void addFile(String dirPath, String filePath, String[] lines) {
Parser.Result result = Parser.parseFile(dirPath, filePath, lines);
if (result.stopLooking) {
stopLooking.add(dirPath);
}
+ addPreferredEmails(result.owner2paths.keySet());
for (String owner : result.owner2paths.keySet()) {
+ String email = preferredEmails.get(owner);
for (String path : result.owner2paths.get(owner)) {
- addOwnerPathPair(owner, path);
+ addOwnerPathPair(email, path);
}
}
if (Config.getReportSyntaxError()) {
@@ -232,16 +288,30 @@
return file2Owners;
}
+ /** Returns ObjectId of the given branch, or null. */
+ private static ObjectId getBranchId(Repository repo, String branch, ChangeData changeData) {
+ try {
+ ObjectId id = repo.resolve(branch);
+ if (id == null && changeData != null && !Checker.isExemptFromOwnerApproval(changeData)) {
+ log.error("cannot find branch " + branch + " for " + Config.getChangeId(changeData));
+ }
+ return id;
+ } catch (Exception e) {
+ log.error("cannot find branch " + branch + " for " + Config.getChangeId(changeData), e);
+ }
+ return null;
+ }
+
/** Returns file content or empty string; uses Repository. */
- static String getRepositoryFile(Repository repo, String branch, String file) {
+ private static String getRepositoryFile(Repository repo, ObjectId id, String file) {
try (RevWalk revWalk = new RevWalk(repo)) {
- RevTree tree = revWalk.parseCommit(repo.resolve(branch)).getTree();
+ RevTree tree = revWalk.parseCommit(id).getTree();
ObjectReader reader = revWalk.getObjectReader();
TreeWalk treeWalk = TreeWalk.forPath(reader, file, tree);
if (treeWalk != null) {
return new String(reader.open(treeWalk.getObjectId(0)).getBytes(), UTF_8);
}
- } catch (IOException e) {
+ } catch (Exception e) {
log.error("get file " + file, 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
new file mode 100644
index 0000000..8d81574
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
@@ -0,0 +1,354 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.findowners;
+
+import static com.googlesource.gerrit.plugins.findowners.Config.OWNERS;
+import static com.googlesource.gerrit.plugins.findowners.Config.OWNERS_FILE_NAME;
+import static com.googlesource.gerrit.plugins.findowners.Config.REJECT_ERROR_IN_OWNERS;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.Multimap;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.extensions.api.projects.ProjectConfigEntryType;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.account.Emails;
+import com.google.gerrit.server.config.PluginConfig;
+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;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ExecutionException;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.eclipse.jgit.diff.RawText;
+import org.eclipse.jgit.lib.FileMode;
+import org.eclipse.jgit.lib.ObjectId;
+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;
+import org.eclipse.jgit.treewalk.filter.TreeFilter;
+
+/** Check syntax of changed OWNERS files. */
+public class OwnersValidator implements CommitValidationListener {
+ private interface TreeWalkVisitor {
+ void onVisit(TreeWalk tw);
+ }
+
+ public static AbstractModule module() {
+ return new AbstractModule() {
+ @Override
+ protected void configure() {
+ DynamicSet.bind(binder(), CommitValidationListener.class).to(OwnersValidator.class);
+ bind(ProjectConfigEntry.class)
+ .annotatedWith(Exports.named(REJECT_ERROR_IN_OWNERS))
+ .toInstance(
+ new ProjectConfigEntry(
+ "Reject OWNERS Files With Errors",
+ null,
+ ProjectConfigEntryType.BOOLEAN,
+ null,
+ false,
+ "Pushes of commits with errors in OWNERS files will be rejected."));
+ }
+ };
+ }
+
+ 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;
+ }
+
+ public static String getOwnersFileName(PluginConfig cfg) {
+ return getOwnersFileName(cfg, OWNERS);
+ }
+
+ public static String getOwnersFileName(PluginConfig cfg, String defaultName) {
+ return cfg.getString(OWNERS_FILE_NAME, defaultName);
+ }
+
+ public String getOwnersFileName(Project.NameKey project) {
+ String name = getOwnersFileName(cfgFactory.getFromGerritConfig(pluginName, true));
+ try {
+ return getOwnersFileName(
+ cfgFactory.getFromProjectConfigWithInheritance(project, pluginName), name);
+ } catch (NoSuchProjectException e) {
+ return name;
+ }
+ }
+
+ @VisibleForTesting
+ static boolean isActive(PluginConfig cfg) {
+ return cfg.getBoolean(REJECT_ERROR_IN_OWNERS, false);
+ }
+
+ @Override
+ public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
+ throws CommitValidationException {
+ List<CommitValidationMessage> messages = new LinkedList<>();
+ try {
+ 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);
+ }
+ }
+ } catch (NoSuchProjectException | IOException | ExecutionException e) {
+ throw new CommitValidationException("failed to check owners files", e);
+ }
+ if (hasError(messages)) {
+ throw new CommitValidationException("found invalid owners file", messages);
+ }
+ return messages;
+ }
+
+ @VisibleForTesting
+ List<CommitValidationMessage> performValidation(
+ RevCommit c, RevWalk revWalk, String ownersFileName, boolean verbose)
+ throws IOException, ExecutionException {
+ // Collect all messages from all files.
+ List<CommitValidationMessage> messages = new LinkedList<>();
+ // Collect all email addresses from all files and check each address only once.
+ Map<String, Set<String>> email2lines = new HashMap<>();
+ Map<String, ObjectId> content = getChangedOwners(c, revWalk, ownersFileName);
+ for (String path : content.keySet()) {
+ ObjectLoader ol = revWalk.getObjectReader().open(content.get(path));
+ try (InputStream in = ol.openStream()) {
+ if (RawText.isBinary(in)) {
+ add(messages, path + " is a binary file", true); // OWNERS files cannot be binary
+ continue;
+ }
+ }
+ checkFile(messages, email2lines, path, ol, verbose);
+ }
+ checkEmails(messages, emails, email2lines, verbose);
+ return messages;
+ }
+
+ private static void checkEmails(
+ List<CommitValidationMessage> messages,
+ Emails emails,
+ Map<String, Set<String>> email2lines,
+ boolean verbose) {
+ List<String> owners = new ArrayList<>(email2lines.keySet());
+ if (verbose) {
+ for (String owner : owners) {
+ add(messages, "owner: " + owner, false);
+ }
+ }
+ if (emails == null || owners.isEmpty()) {
+ return;
+ }
+ String[] ownerEmailsAsArray = new String[owners.size()];
+ owners.toArray(ownerEmailsAsArray);
+ try {
+ Multimap<String, Account.Id> email2ids = emails.getAccountsFor(ownerEmailsAsArray);
+ for (String owner : ownerEmailsAsArray) {
+ boolean wrongEmail = (email2ids == null);
+ if (!wrongEmail) {
+ try {
+ Collection<Account.Id> ids = email2ids.get(owner);
+ wrongEmail = (ids == null || ids.size() != 1);
+ } catch (Exception e) {
+ wrongEmail = true;
+ }
+ }
+ if (wrongEmail) {
+ String locations = String.join(" ", email2lines.get(owner));
+ add(messages, "unknown: " + owner + " at " + locations, true);
+ }
+ }
+ } catch (Exception e) {
+ add(messages, "checkEmails failed.", true);
+ }
+ }
+
+ private static void checkFile(
+ List<CommitValidationMessage> messages,
+ Map<String, Set<String>> email2lines,
+ String path,
+ ObjectLoader ol,
+ boolean verbose)
+ throws IOException {
+ if (verbose) {
+ add(messages, "validate: " + path, false);
+ }
+ try (BufferedReader br =
+ new BufferedReader(new InputStreamReader(ol.openStream(), StandardCharsets.UTF_8))) {
+ int line = 0;
+ for (String l = br.readLine(); l != null; l = br.readLine()) {
+ line++;
+ checkLine(messages, email2lines, path, line, l);
+ }
+ }
+ }
+
+ // Line patterns accepted by Parser.java in the find-owners plugin.
+ static final Pattern patComment = Pattern.compile("^ *(#.*)?$");
+ static final Pattern patEmail = // email address or a "*"
+ Pattern.compile("^ *([^ <>@]+@[^ <>@#]+|\\*) *(#.*)?$");
+ static final Pattern patFile = Pattern.compile("^ *file:.*$");
+ static final Pattern patNoParent = Pattern.compile("^ *set +noparent *(#.*)?$");
+ static final Pattern patPerFileNoParent =
+ Pattern.compile("^ *per-file +([^= ]+) *= *set +noparent *(#.*)?$");
+ static final Pattern patPerFileEmail =
+ Pattern.compile("^ *per-file +([^= ]+) *= *([^ <>@]+@[^ <>@#]+|\\*) *(#.*)?$");
+
+ private static void collectEmail(
+ Map<String, Set<String>> map, String email, String file, int lineNumber) {
+ if (!email.equals("*")) {
+ map.computeIfAbsent(email, (String k) -> new HashSet<>());
+ map.get(email).add(file + ":" + lineNumber);
+ }
+ }
+
+ private static boolean hasError(List<CommitValidationMessage> messages) {
+ for (CommitValidationMessage m : messages) {
+ if (m.isError()) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private static void add(List<CommitValidationMessage> messages, String msg, boolean error) {
+ messages.add(new CommitValidationMessage(msg, error));
+ }
+
+ private static void checkLine(
+ List<CommitValidationMessage> messages,
+ Map<String, Set<String>> email2lines,
+ String path,
+ int lineNumber,
+ String line) {
+ Matcher m;
+ if (patComment.matcher(line).find()
+ || patNoParent.matcher(line).find()
+ || patPerFileNoParent.matcher(line).find()) {
+ return;
+ } else if ((m = patEmail.matcher(line)).find()) {
+ collectEmail(email2lines, m.group(1), path, lineNumber);
+ } else if ((m = patPerFileEmail.matcher(line)).find()) {
+ collectEmail(email2lines, m.group(2).trim(), path, lineNumber);
+ } else {
+ String prefix = patFile.matcher(line).find() ? "ignored" : "syntax";
+ add(messages, prefix + ": " + path + ":" + lineNumber + ": " + line, true);
+ }
+ }
+
+ /**
+ * Find all changed OWNERS files which differ between the commit and its parents. Return a map
+ * from "Path to the changed file" to "ObjectId of the file".
+ */
+ private static Map<String, ObjectId> getChangedOwners(
+ RevCommit c, RevWalk revWalk, String ownersFileName) throws IOException {
+ final Map<String, ObjectId> content = new HashMap<>();
+ visitChangedEntries(
+ c,
+ revWalk,
+ new TreeWalkVisitor() {
+ @Override
+ public void onVisit(TreeWalk tw) {
+ if (isFile(tw) && ownersFileName.equals(tw.getNameString())) {
+ content.put(tw.getPathString(), tw.getObjectId(0));
+ }
+ }
+ });
+ return content;
+ }
+
+ private static boolean isFile(TreeWalk tw) {
+ return FileMode.EXECUTABLE_FILE.equals(tw.getRawMode(0))
+ || FileMode.REGULAR_FILE.equals(tw.getRawMode(0));
+ }
+
+ /**
+ * Find all TreeWalk entries which differ between the commit and its parents. If a TreeWalk entry
+ * is found this method calls the onVisit() method of the class TreeWalkVisitor.
+ */
+ private static void visitChangedEntries(RevCommit c, RevWalk revWalk, TreeWalkVisitor visitor)
+ throws IOException {
+ try (TreeWalk tw = new TreeWalk(revWalk.getObjectReader())) {
+ tw.setRecursive(true);
+ tw.setFilter(TreeFilter.ANY_DIFF);
+ tw.addTree(c.getTree());
+ if (c.getParentCount() > 0) {
+ for (RevCommit p : c.getParents()) {
+ if (p.getTree() == null) {
+ revWalk.parseHeaders(p);
+ }
+ tw.addTree(p.getTree());
+ }
+ while (tw.next()) {
+ if (isDifferentToAllParents(c, tw)) {
+ visitor.onVisit(tw);
+ }
+ }
+ } else {
+ while (tw.next()) {
+ visitor.onVisit(tw);
+ }
+ }
+ }
+ }
+
+ private static boolean isDifferentToAllParents(RevCommit c, TreeWalk tw) {
+ if (c.getParentCount() > 1) {
+ for (int p = 1; p <= c.getParentCount(); p++) {
+ if (tw.getObjectId(0).equals(tw.getObjectId(p))) {
+ return false;
+ }
+ }
+ }
+ return true;
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
index f91ecbe..296f408 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java
@@ -54,6 +54,7 @@
String user;
String project;
String branch;
+ List<String> errors;
SortedMap<String, List<String>> path2owners;
SortedMap<String, List<String>> owner2paths;
};
diff --git a/src/main/prolog/find_owners.pl b/src/main/prolog/find_owners.pl
index 6afd829..306339b 100644
--- a/src/main/prolog/find_owners.pl
+++ b/src/main/prolog/find_owners.pl
@@ -50,7 +50,12 @@
owner_approved(X), !.
create_owner_approval_label(N, label(X, ok(user(1)))) :-
N > 0, owner_approved(X), !.
-create_owner_approval_label(_, label(X, need(_))) :-
+% If owner approval is required and missing,
+% use the owner_approval_missing(X) label and may(_) state to
+% enable the Submit button. Front-end JavaScript should check
+% the label and then block the submit or suggest users to
+% add "Exempt-From-Owner-Approval:" to the commit message.
+create_owner_approval_label(_, label(X, may(_))) :-
owner_approval_missing(X).
has_owner_approval_label([label(X, _)|_]) :- owner_approval_label(X).
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index d754912..fa046cc 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -36,10 +36,13 @@
`label('Owner-Approved', ok(user(1)))` is added.
* If a change needs owner approval, but some changed file has no *owner*
+1 vote or has negative *owner* vote,
- `label('Owner-Review-Vote', need(_))` is added.
- This will make a change *not-submittable*.
- The change author should add missing *owners* to the
- reviewers list and/or ask for those owner's +1 Code-Review votes.
+ `label('Owner-Review-Vote', may(_))` is added.
+ This will show the label but not disable the Submit button.
+ 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
+ (2) add `Exempt-From-Owner-Approval:` to the commit message.
The **`Find Owners`** button is useful in this situation to find
the missing *owners* or +1 votes of any changed files.
@@ -67,6 +70,16 @@
If a project has already used OWNERS files for other purpose,
the "ownersFileName" parameter can be used to change the default.
+## Validate OWNERS files before upload
+
+To check syntax of OWNERS files before they are uploaded,
+set the following variable in project.config files.
+
+```bash
+[plugin "find-owners"]
+ rejectErrorInOwners = true
+```
+
## Example 0, call `submit_filter/2`
The simplest configuration adds to `rules.pl` of the root
diff --git a/src/main/resources/Documentation/rest-api.md b/src/main/resources/Documentation/rest-api.md
index 7676059..10b1565 100644
--- a/src/main/resources/Documentation/rest-api.md
+++ b/src/main/resources/Documentation/rest-api.md
@@ -54,6 +54,8 @@
* **branch**: the change's destination branch name.
+ * **errors**: error messages from OWNERS files.
+
* **path2owners**:
a map from directory path or file glob to an array of owner emails.
Note that `*` is a special owner email address.
diff --git a/src/main/resources/static/find-owners.js b/src/main/resources/static/find-owners.js
index 8122474..0abdabe 100644
--- a/src/main/resources/static/find-owners.js
+++ b/src/main/resources/static/find-owners.js
@@ -13,23 +13,63 @@
// limitations under the License.
Gerrit.install(function(self) {
- function onFindOwners(c) {
- const HTML_ALL_HAVE_OWNER_APPROVAL =
- '<b>All files have owner approval.</b><br>';
+ if (window.Polymer) {
+ // Install deprecated APIs to mimic GWT UI API.
+ self.deprecated.install();
+ }
+
+ // If context.popup API exists and popup content is small,
+ // use the API and set useContextPopup,
+ // otherwise, use pageDiv and set its visibility.
+ var useContextPopup = false;
+ var pageDiv = document.createElement('div');
+ document.body.appendChild(pageDiv);
+ function hideFindOwnersPage() {
+ pageDiv.style.visibility = 'hidden';
+ }
+ function popupFindOwnersPage(context, change, revision, onSubmit) {
+ const PADDING = 5;
+ const LARGE_PAGE_STYLE = Gerrit.css(
+ 'visibility: hidden;' +
+ 'background: rgba(200, 200, 200, 0.95);' +
+ 'border: 3px solid;' +
+ 'border-color: #c8c8c8;' +
+ 'border-radius: 3px;' +
+ 'position: fixed;' +
+ 'z-index: 100;' +
+ 'overflow: auto;' +
+ 'padding: ' + PADDING + 'px;'
+ );
+ const BUTTON_STYLE = Gerrit.css(
+ 'background-color: #4d90fe;' +
+ 'border: 2px solid;' +
+ 'border-color: #4d90fe;' +
+ 'margin: 2px 10px 2px 10px;' +
+ 'text-align: center;' +
+ 'font-size: 8pt;' +
+ 'font-weight: bold;' +
+ 'color: #fff;' +
+ '-webkit-border-radius: 2px;' +
+ 'cursor: pointer;'
+ );
const HTML_BULLET = '<small>★</small>'; // a Black Star
+ const HTML_HAS_APPROVAL_HEADER =
+ '<hr><b>Files with +1 or +2 Code-Review vote from owners:</b><br>';
const HTML_IS_EXEMPTED =
- '<b>This commit is exempted from owner approval.</b><br>';
+ '<b>This change is Exempt-From-Owner-Approval.</b>';
const HTML_NEED_REVIEWER_HEADER =
'<hr><b>Files without owner reviewer:</b><br>';
const HTML_NEED_APPROVAL_HEADER =
'<hr><b>Files without Code-Review vote from an owner:</b><br>';
const HTML_NO_OWNER =
- '<b>No owner was found for changed files.</b><br>';
+ '<b>No owner was found for changed files.</b>';
+ const HTML_ONSUBMIT_HEADER =
+ '<b>WARNING: Need owner approval vote before submit.</b><hr>';
const HTML_OWNERS_HEADER = '<hr><b>Owners in alphabetical order:</b><br>';
const HTML_SELECT_REVIEWERS =
'<b>Check the box before owner names to select reviewers, ' +
'then click the "Apply" button.' +
- '</b><br><small>Each file needs at least one owner. ' +
+ '</b><br><small>Each file needs at least one owner code review vote. ' +
'Owners listed after a file are ordered by their importance. ' +
'(Or declare "<b><span style="font-size:80%;">' +
'Exempt-From-Owner-Approval:</span></b> ' +
@@ -39,15 +79,18 @@
const CHECKBOX_ID = 'FindOwners:CheckBox';
const HEADER_DIV_ID = 'FindOwners:Header';
const OWNERS_DIV_ID = 'FindOwners:Owners';
+ const HAS_APPROVAL_DIV_ID = 'FindOwners:HasApproval';
const NEED_APPROVAL_DIV_ID = 'FindOwners:NeedApproval';
const NEED_REVIEWER_DIV_ID = 'FindOwners:NeedReviewer';
// Aliases to values in the context.
- const branch = c.change.branch;
- const changeId = c.change._number;
- const message = c.revision.commit.message;
- const project = c.change.project;
+ const branch = change.branch;
+ const changeId = change._number;
+ const changeOwner = change.owner;
+ const message = revision.commit.message;
+ const project = change.project;
+ var minVoteLevel = 1; // could be changed by server returned results.
var reviewerId = {}; // map from a reviewer's email to account id.
var reviewerVote = {}; // map from a reviewer's email to Code-Review vote.
@@ -59,8 +102,39 @@
function getElement(id) {
return document.getElementById(id);
}
+ function httpGet(url, callback) {
+ var xhr = new XMLHttpRequest();
+ xhr.open('GET', url);
+ xhr.onreadystatechange = function() {
+ if (xhr.readyState == XMLHttpRequest.DONE) {
+ // skip special characters ")]}'\n"
+ callback(!!xhr.responseText ?
+ JSON.parse(xhr.responseText.substring(5)) : {});
+ }
+ };
+ xhr.send();
+ }
+ function httpError(msg, callback) {
+ console.log('UNIMPLEMENTED: ' + msg);
+ callback();
+ }
+ function gerritGet(url, callback) {
+ (!!Gerrit.get) ? Gerrit.get(url, callback) :
+ ((!!self.get) ? self.get('/../..' + url, callback) :
+ httpGet(url, callback));
+ }
+ function gerritPost(url, data, callback) {
+ (!!Gerrit.post) ? Gerrit.post(url, data, callback) :
+ ((!!self.post) ? self.post('/../..' + url, data, callback) :
+ httpError('POST ' + url, callback));
+ }
+ function gerritDelete(url, callback) {
+ (!!Gerrit.delete) ? Gerrit.delete(url, callback) :
+ ((!!self.delete) ? self.delete('/../..' + url, callback) :
+ httpError('DELETE ' + url, callback));
+ }
function getReviewers(change, callBack) {
- Gerrit.get('changes/' + change + '/reviewers', callBack);
+ gerritGet('/changes/' + change + '/reviewers', callBack);
}
function setupReviewersMap(reviewerList) {
reviewerId = {};
@@ -76,6 +150,14 @@
}
}
});
+ // Give CL author a default minVoteLevel vote.
+ if (changeOwner != null &&
+ 'email' in changeOwner && '_account_id' in changeOwner &&
+ (!(changeOwner.email in reviewerId) ||
+ reviewerVote[changeOwner.email] == 0)) {
+ reviewerId[changeOwner.email] = changeOwner._account_id;
+ reviewerVote[changeOwner.email] = minVoteLevel;
+ }
}
function checkAddRemoveLists() {
// Gerrit.post and delete are asynchronous.
@@ -88,9 +170,9 @@
// Gerrit core UI shows the error dialog and does not provide
// a way for plugins to handle the error yet.
needRefresh = true;
- Gerrit.post('changes/' + changeId + '/reviewers',
- {'reviewer': email},
- checkAddRemoveLists);
+ gerritPost('/changes/' + changeId + '/reviewers',
+ {'reviewer': email},
+ checkAddRemoveLists);
return;
}
}
@@ -99,16 +181,16 @@
if (email in reviewerId) {
removeList = removeList.slice(i + 1, removeList.length);
needRefresh = true;
- Gerrit.delete('changes/' + changeId +
- '/reviewers/' + reviewerId[email],
- checkAddRemoveLists);
+ gerritDelete('/changes/' + changeId +
+ '/reviewers/' + reviewerId[email],
+ checkAddRemoveLists);
return;
}
}
- c.hide();
+ hideFindOwnersPage();
if (needRefresh) {
needRefresh = false;
- Gerrit.refresh();
+ (!!Gerrit.refresh) ? Gerrit.refresh() : location.reload();
}
callServer(showFindOwnersResults);
}
@@ -121,7 +203,7 @@
return (owner in reviewers || owner == '*');
});
}
- function hasOwnerApproval(votes, minVoteLevel, owners) {
+ function hasOwnerApproval(votes, owners) {
var foundApproval = false;
for (var j = 0; j < owners.length; j++) {
if (owners[j] in votes) {
@@ -129,7 +211,6 @@
if (v < 0) {
return false; // cannot have any negative vote
}
- // TODO: do not count if owners[j] is the patch committer.
foundApproval |= v >= minVoteLevel;
}
}
@@ -147,26 +228,40 @@
e.innerHTML = s;
return e;
}
+ function br() {
+ return document.createElement('br');
+ }
+ function hr() {
+ return document.createElement('hr');
+ }
+ function newButton(name, action) {
+ var b = document.createElement('button');
+ b.appendChild(document.createTextNode(name));
+ b.className = BUTTON_STYLE;
+ b.onclick = action;
+ return b;
+ }
function showJsonLines(args, key, obj) {
showBoldKeyValueLines(args, key, JSON.stringify(obj, null, 2));
}
function showBoldKeyValueLines(args, key, value) {
- args.push(c.hr(), strElement('<b>' + key + '</b>:'), c.br());
+ args.push(hr(), strElement('<b>' + key + '</b>:'), br());
value.split('\n').forEach(function(line) {
- args.push(c.msg(line), c.br());
+ args.push(strElement(line), br());
});
}
function showDebugMessages(result, args) {
function addKeyValue(key, value) {
args.push(strElement('<b>' + key + '</b>: ' + value + '<br>'));
}
- args.push(c.hr());
+ args.push(hr());
addKeyValue('changeId', changeId);
addKeyValue('project', project);
addKeyValue('branch', branch);
+ addKeyValue('changeOwner.email', changeOwner.email);
addKeyValue('Gerrit.url', Gerrit.url());
addKeyValue('self.url', self.url());
- showJsonLines(args, 'changeOwner', c.change.owner);
+ showJsonLines(args, 'changeOwner', change.owner);
showBoldKeyValueLines(args, 'commit.message', message);
showJsonLines(args, 'Client reviewers Ids', reviewerId);
showJsonLines(args, 'Client reviewers Votes', reviewerVote);
@@ -184,14 +279,18 @@
var header = emptyDiv(HEADER_DIV_ID);
var needReviewerDiv = emptyDiv(NEED_REVIEWER_DIV_ID);
var needApprovalDiv = emptyDiv(NEED_APPROVAL_DIV_ID);
+ var hasApprovalDiv = emptyDiv(HAS_APPROVAL_DIV_ID);
addApplyButton();
+ args.push(newButton('Cancel', hideFindOwnersPage));
var ownersDiv = emptyDiv(OWNERS_DIV_ID);
var numCheckBoxes = 0;
var owner2boxes = {}; // owner name ==> array of checkbox id
var owner2email = {}; // owner name ==> email address
+ minVoteLevel =
+ ('minOwnerVoteLevel' in result ? result.minOwnerVoteLevel : 1);
function addApplyButton() {
- var apply = c.button('Apply', {onclick: doApplyButton});
+ var apply = newButton('Apply', doApplyButton);
apply.id = APPLY_BUTTON_ID;
apply.style.display = 'none';
args.push(apply);
@@ -237,7 +336,8 @@
owner2boxes[name] = [];
}
owner2boxes[name].push(id);
- var box = c.checkbox();
+ var box = document.createElement('input');
+ box.type = 'checkbox';
box.checked = (ownerEmail in reviewerId);
box.id = id;
box.value = name;
@@ -262,7 +362,7 @@
}
div.appendChild(strElement(item));
sortedOwners.reduce(add2list, []).forEach(addOwner);
- div.appendChild(c.br());
+ div.appendChild(br());
});
}
function addOwnersDiv(div, title) {
@@ -285,6 +385,7 @@
function updateDivContent() {
var groupNeedReviewer = [];
var groupNeedApproval = [];
+ var groupHasApproval = [];
numCheckBoxes = 0;
owner2boxes = {};
Object.keys(groups).sort().forEach(function(key) {
@@ -293,24 +394,22 @@
groupNeedReviewer.push(key);
} else if (g.needApproval) {
groupNeedApproval.push(key);
+ } else {
+ groupHasApproval.push(key);
}
});
- if (0 == groupNeedReviewer.length && 0 == groupNeedApproval.length) {
- showDiv(header, HTML_ALL_HAVE_OWNER_APPROVAL);
- } else {
- showDiv(header, HTML_SELECT_REVIEWERS);
- addGroupsToDiv(needReviewerDiv, groupNeedReviewer,
- HTML_NEED_REVIEWER_HEADER);
- addGroupsToDiv(needApprovalDiv, groupNeedApproval,
- HTML_NEED_APPROVAL_HEADER);
- addOwnersDiv(ownersDiv, HTML_OWNERS_HEADER);
- }
+ showDiv(header,
+ (onSubmit ? HTML_ONSUBMIT_HEADER : '') + HTML_SELECT_REVIEWERS);
+ addGroupsToDiv(needReviewerDiv, groupNeedReviewer,
+ HTML_NEED_REVIEWER_HEADER);
+ addGroupsToDiv(needApprovalDiv, groupNeedApproval,
+ HTML_NEED_APPROVAL_HEADER);
+ addGroupsToDiv(hasApprovalDiv, groupHasApproval,
+ HTML_HAS_APPROVAL_HEADER);
+ addOwnersDiv(ownersDiv, HTML_OWNERS_HEADER);
}
function createGroups() {
var owners2group = {}; // owner list to group name
- var minVoteLevel =
- ('minOwnerVoteLevel' in result ?
- result['minOwnerVoteLevel'] : 1);
Object.keys(result.file2owners).sort().forEach(function(name) {
var splitOwners = result.file2owners[name];
var owners = splitOwners.join(' ');
@@ -321,7 +420,7 @@
groupSize[name] = 1;
var needReviewer = !hasOwnerReviewer(reviewerId, splitOwners);
var needApproval = !needReviewer &&
- !hasOwnerApproval(reviewerVote, minVoteLevel, splitOwners);
+ !hasOwnerApproval(reviewerVote, splitOwners);
groups[name] = {
'needReviewer': needReviewer,
'needApproval': needApproval,
@@ -333,29 +432,98 @@
updateDivContent();
}
function showFindOwnersResults(result) {
+ function prepareElements() {
+ var elems = [];
+ var text = isExemptedFromOwnerApproval() ? HTML_IS_EXEMPTED :
+ (Object.keys(result.file2owners).length <= 0 ?
+ HTML_NO_OWNER : null);
+ useContextPopup = !!context && !!text && !!context.popup;
+ if (!!text) {
+ if (useContextPopup) {
+ elems.push(hr(), strElement(text), hr());
+ var onClick = function() { context.hide(); };
+ elems.push(context.button('OK', {onclick: onClick}), hr());
+ } else {
+ elems.push(strElement(text), newButton('OK', hideFindOwnersPage));
+ }
+ } else {
+ showFilesAndOwners(result, elems);
+ if (result.addDebugMsg) {
+ showDebugMessages(result, elems);
+ }
+ }
+ return elems;
+ }
function popupWindow(reviewerList) {
setupReviewersMap(reviewerList);
- var args = [];
- if (isExemptedFromOwnerApproval()) {
- args.push(strElement(HTML_IS_EXEMPTED));
- } else if (Object.keys(result.file2owners).length <= 0) {
- args.push(strElement(HTML_NO_OWNER));
+ var elems = prepareElements();
+ if (useContextPopup) {
+ context.popup(context.div.apply(this, elems));
} else {
- showFilesAndOwners(result, args);
+ while (pageDiv.firstChild) {
+ pageDiv.removeChild(pageDiv.firstChild);
+ }
+ elems.forEach(function(e) { pageDiv.appendChild(e); });
+ pageDiv.className = LARGE_PAGE_STYLE;
+ // Calculate required height, limited to 85% of window height,
+ // and required width, limited to 75% of window width.
+ pageDiv.style.top = '5%';
+ pageDiv.style.height = 'auto';
+ pageDiv.style.left = '10%';
+ pageDiv.style.width = 'auto';
+ var rect = pageDiv.getBoundingClientRect();
+ var h = Math.round(Math.min(rect.height - 2 * PADDING,
+ window.innerHeight * 0.85));
+ pageDiv.style.height = h + 'px';
+ pageDiv.style.top = Math.round((window.innerHeight - h) / 2) + 'px';
+ var w = Math.round(Math.min(rect.width - 2 * PADDING,
+ window.innerWidth * 0.75));
+ pageDiv.style.width = w + 'px';
+ pageDiv.style.left = Math.round((window.innerWidth - w) / 2) + 'px';
+ pageDiv.style.visibility = 'visible';
}
- if (result.addDebugMsg) {
- showDebugMessages(result, args);
- }
- c.popup(c.div.apply(this, args));
}
getReviewers(changeId, popupWindow);
}
function callServer(callBack) {
// Use the plugin REST API; pass only changeId;
// let server get current patch set, project and branch info.
- Gerrit.get('changes/' + changeId + '/owners', showFindOwnersResults);
+ gerritGet('/changes/' + changeId + '/owners', showFindOwnersResults);
}
+ event.stopPropagation();
callServer(showFindOwnersResults);
}
- self.onAction('revision', 'find-owners', onFindOwners);
+ function onFindOwners(context) {
+ popupFindOwnersPage(context, context.change, context.revision, false);
+ }
+ function onSubmit(change, revision) {
+ const OWNER_REVIEW_LABEL = 'Owner-Review-Vote';
+ if (change.labels.hasOwnProperty(OWNER_REVIEW_LABEL)) {
+ // Pop up Find Owners page; do not submit.
+ popupFindOwnersPage(null, change, revision, true);
+ return false;
+ }
+ return true; // Okay to submit.
+ }
+ function onClick(e) {
+ if (pageDiv.style.visibility != 'hidden' && !useContextPopup) {
+ var x = event.clientX;
+ var y = event.clientY;
+ var rect = pageDiv.getBoundingClientRect();
+ if (x < rect.left || x >= rect.left + rect.width ||
+ y < rect.top || y >= rect.top + rect.height) {
+ hideFindOwnersPage();
+ }
+ }
+ }
+ // When the "Find Owners" button is clicked, call onFindOwners.
+ if (!!self.onAction) { // PolyGerrit does not have self.onAction
+ self.onAction('revision', 'find-owners', onFindOwners);
+ } else {
+ console.log('WARNING, no handler for the Find Owners button');
+ }
+ // When the "Submit" button is clicked, call onSubmit.
+ self.on('submitchange', onSubmit);
+ // Clicks outside the pop up window should close the window.
+ document.body.addEventListener('click', onClick);
});
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 2e841e8..160d5fa 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -18,10 +18,12 @@
import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.collect.Multimap;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.extensions.api.accounts.EmailInput;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -30,10 +32,13 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.inject.Inject;
+import java.util.Collection;
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;
@@ -44,6 +49,7 @@
@TestPlugin(name = "find-owners", sysModule = "com.googlesource.gerrit.plugins.findowners.Module")
public class FindOwnersIT extends LightweightPluginDaemonTest {
+ @Inject private Emails emails;
@Inject private PluginConfigFactory configFactory;
@Test
@@ -170,13 +176,46 @@
@Test
public void accountTest() throws Exception {
- String[] myTestEmails = {"abc@g.com", "abc+xyz@g.com", "xyz@g.com"};
- for (String email : myTestEmails) {
- Account.Id id = accounts.create("User" + email, email, "FullName" + email).getId();
- // Action.getReviewers uses accountCache to get email address.
- assertThat(accountCache.get(id).getAccount().getPreferredEmail()).isEqualTo(email);
- // Checker.getVotes uses AccountAccess to get email address.
- assertThat(db.accounts().get(id).getPreferredEmail()).isEqualTo(email);
+ String[] users = {"user1", "user2", "user3"};
+ String[] emails1 = {"abc@g.com", "abc+xyz@g.com", "xyz-team+review@g.com"};
+ String[] emails2 = {"abc@goog.com", "abc+xyz2@g.com", "xyz-team@goog.com"};
+ // Create accounts with given user name, first and second email addresses.
+ for (int i = 0; i < users.length; i++) {
+ accountCreator.create(users[i], emails1[i], "FullName " + users[i]).getId();
+ EmailInput input = new EmailInput();
+ input.email = emails2[i];
+ input.noConfirmation = true;
+ gApi.accounts().id(users[i]).addEmail(input);
+ }
+ // Find accounts with given first and second email addresses.
+ // OwnersDb uses either emails.getAccountFor or getAccountsFor to get preferred email addresses.
+ Multimap<String, Account.Id> map1 = emails.getAccountsFor(emails1);
+ Multimap<String, Account.Id> map2 = emails.getAccountsFor(emails2);
+ for (int i = 0; i < users.length; i++) {
+ Collection<Account.Id> ids1 = emails.getAccountFor(emails1[i]);
+ Collection<Account.Id> ids2 = emails.getAccountFor(emails2[i]);
+ Collection<Account.Id> ids3 = map1.get(emails1[i]);
+ Collection<Account.Id> ids4 = map2.get(emails2[i]);
+ assertThat(ids1).hasSize(1);
+ assertThat(ids2).hasSize(1);
+ assertThat(ids3).hasSize(1);
+ assertThat(ids4).hasSize(1);
+ Account.Id id1 = ids1.iterator().next();
+ Account.Id id2 = ids2.iterator().next();
+ Account.Id id3 = ids3.iterator().next();
+ Account.Id id4 = ids4.iterator().next();
+ assertThat(id1).isEqualTo(id2); // Both emails should find the same account.
+ assertThat(id1).isEqualTo(id3);
+ assertThat(id1).isEqualTo(id4);
+ // Action.getReviewers and Checker.getVotes use accountCache to get email address.
+ assertThat(accountCache.get(id1).getAccount().getPreferredEmail()).isEqualTo(emails1[i]);
+ }
+ // Wrong or non-existing email address.
+ String[] wrongEmails = {"nobody", "@g.com", "nobody@g.com", "*"};
+ Multimap<String, Account.Id> email2ids = emails.getAccountsFor(wrongEmails);
+ for (String email : wrongEmails) {
+ assertThat(emails.getAccountFor(email)).isEmpty();
+ assertThat(email2ids.get(email)).isEmpty();
}
}
@@ -220,9 +259,9 @@
PushOneCommit.Result cB = createChange("add tB.c", "tB.c", "Hello B!");
// Default owners file name is "OWNERS".
- assertThat(Config.getOwnersFileName(null)).isEqualTo("OWNERS");
- assertThat(Config.getOwnersFileName(pA)).isEqualTo("OWNERS");
- assertThat(Config.getOwnersFileName(pB)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(null, null)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(pA, null)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS");
String ownerX = oneOwnerList("x@x");
String ownerY = oneOwnerList("y@y");
@@ -234,8 +273,8 @@
setProjectConfig("ownersFileName", "OWNERS.alpha");
switchProject(pB);
setProjectConfig("ownersFileName", "OWNERS.beta");
- assertThat(Config.getOwnersFileName(pA)).isEqualTo("OWNERS.alpha");
- assertThat(Config.getOwnersFileName(pB)).isEqualTo("OWNERS.beta");
+ assertThat(Config.getOwnersFileName(pA, null)).isEqualTo("OWNERS.alpha");
+ assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS.beta");
String ownerA = oneOwnerList("a@a");
String ownerB = oneOwnerList("b@b");
assertThat(getOwnersResponse(cA)).contains(ownerA + ", files:[ tA.c ]");
@@ -244,24 +283,38 @@
// Change back to OWNERS in Project_A
switchProject(pA);
setProjectConfig("ownersFileName", "OWNERS");
- assertThat(Config.getOwnersFileName(pA)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(pA, null)).isEqualTo("OWNERS");
assertThat(getOwnersResponse(cA)).contains(ownerX + ", files:[ tA.c ]");
assertThat(getOwnersResponse(cB)).contains(ownerB + ", files:[ tB.c ]");
// Change back to OWNERS.alpha in Project_B, but there is no OWNERS.alpha
switchProject(pB);
setProjectConfig("ownersFileName", "OWNERS.alpha");
- assertThat(Config.getOwnersFileName(pB)).isEqualTo("OWNERS.alpha");
+ assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS.alpha");
assertThat(getOwnersResponse(cA)).contains(ownerX + ", files:[ tA.c ]");
assertThat(getOwnersResponse(cB)).contains("owners:[], files:[ tB.c ]");
// Do not accept empty string or all-white-spaces for ownersFileName.
setProjectConfig("ownersFileName", " ");
- assertThat(Config.getOwnersFileName(pB)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS");
setProjectConfig("ownersFileName", " \t ");
- assertThat(Config.getOwnersFileName(pB)).isEqualTo("OWNERS");
+ assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("OWNERS");
setProjectConfig("ownersFileName", "O");
- assertThat(Config.getOwnersFileName(pB)).isEqualTo("O");
+ assertThat(Config.getOwnersFileName(pB, null)).isEqualTo("O");
+ }
+
+ @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
+ // 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");
+ 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
}
@Test
@@ -273,7 +326,8 @@
ChangeResource cr = parseChangeResource(changeInfo.changeId);
Action.Parameters param = new Action.Parameters();
Action action =
- new Action("find-owners", null, null, null, changeDataFactory, accountCache, repoManager);
+ new Action(
+ "find-owners", null, null, null, changeDataFactory, accountCache, emails, repoManager);
Response<RestResult> response = action.apply(db, cr, param);
RestResult result = response.value();
verifyRestResult(result, 1, 1, changeInfo._number, false);
@@ -381,6 +435,14 @@
approveSubmit(commit);
}
+ private int checkApproval(PushOneCommit.Result r) throws Exception {
+ Repository repo = repoManager.openRepository(r.getChange().project());
+ Cache cache = Cache.getInstance().init(0, 0);
+ OwnersDb db = cache.get(accountCache, emails, repo, r.getChange(), 1);
+ Checker c = new Checker(repo, r.getChange(), 1);
+ return c.findApproval(accountCache, db);
+ }
+
// Remove '"' and space; replace '\n' with ' '; ignore unpredictable "owner_revision".
private static String filteredJson(String json) {
return json.replaceAll("[\" ]*", "").replace('\n', ' ').replaceAll("owner_revision:[^ ]* ", "");
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
new file mode 100644
index 0000000..68e8b3d
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorTest.java
@@ -0,0 +1,317 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.findowners;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.googlesource.gerrit.plugins.findowners.Config.OWNERS;
+import static com.googlesource.gerrit.plugins.findowners.Config.OWNERS_FILE_NAME;
+import static com.googlesource.gerrit.plugins.findowners.Config.REJECT_ERROR_IN_OWNERS;
+
+import com.google.common.base.Function;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
+import com.google.common.collect.Lists;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.account.Emails;
+import com.google.gerrit.server.config.PluginConfig;
+import com.google.gerrit.server.git.validators.CommitValidationMessage;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.commons.io.FileUtils;
+import org.eclipse.jgit.api.AddCommand;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.storage.file.FileRepositoryBuilder;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Test OwnersValidator, which checks syntax of changed OWNERS files. */
+@RunWith(JUnit4.class)
+public class OwnersValidatorTest {
+
+ private static class MockedEmails extends Emails {
+ Set<String> registered;
+
+ MockedEmails() {
+ super(null, null);
+ registered =
+ ImmutableSet.of(
+ "u1@g.com", "u2@g.com", "u2.m@g.com", "user1@google.com", "u1+review@g.com");
+ }
+
+ @Override
+ public ImmutableSetMultimap<String, Account.Id> getAccountsFor(String... emails) {
+ // Used by checkEmails; each email should have exactly one Account.Id
+ ImmutableSetMultimap.Builder<String, Account.Id> builder = ImmutableSetMultimap.builder();
+ int id = 1000000;
+ for (String s : registered) {
+ builder.put(s, new Account.Id(++id));
+ }
+ return builder.build();
+ }
+ }
+
+ private File repoFolder;
+ private Repository repo;
+
+ @Before
+ public void init() throws IOException {
+ repoFolder = File.createTempFile("Git", "");
+ repoFolder.delete();
+ repo = FileRepositoryBuilder.create(new File(repoFolder, ".git"));
+ repo.create();
+ }
+
+ @After
+ public void cleanup() throws IOException {
+ repo.close();
+ if (repoFolder.exists()) {
+ FileUtils.deleteDirectory(repoFolder);
+ }
+ }
+
+ private static final String OWNERS_ANDROID = "OWNERS.android"; // alternative OWNERS file name
+ private static final PluginConfig ANDROID_CONFIG = createAndroidConfig(); // use OWNERS_ANDROID
+ private static final PluginConfig EMPTY_CONFIG = new PluginConfig("", new Config());
+ private static final PluginConfig ENABLED_CONFIG = createEnabledConfig(); // use OWNERS
+ private static final PluginConfig DISABLED_CONFIG = createDisabledConfig();
+
+ @Test
+ public void chekIsActiveAndFileName() throws Exception {
+ // This check should be enabled in project.config, default is not active.
+ assertThat(OwnersValidator.isActive(EMPTY_CONFIG)).isFalse();
+ assertThat(OwnersValidator.isActive(ENABLED_CONFIG)).isTrue();
+ assertThat(OwnersValidator.isActive(ANDROID_CONFIG)).isTrue();
+ assertThat(OwnersValidator.isActive(DISABLED_CONFIG)).isFalse();
+ // Default file name is "OWNERS".
+ assertThat(OwnersValidator.getOwnersFileName(EMPTY_CONFIG)).isEqualTo(OWNERS);
+ assertThat(OwnersValidator.getOwnersFileName(ENABLED_CONFIG)).isEqualTo(OWNERS);
+ assertThat(OwnersValidator.getOwnersFileName(DISABLED_CONFIG)).isEqualTo(OWNERS);
+ assertThat(OwnersValidator.getOwnersFileName(ANDROID_CONFIG)).isEqualTo(OWNERS_ANDROID);
+ }
+
+ private static final ImmutableMap<String, String> FILES_WITHOUT_OWNERS =
+ ImmutableMap.of("README", "any\n", "d1/test.c", "int x;\n");
+
+ @Test
+ public void testNoOwners() throws Exception {
+ try (RevWalk rw = new RevWalk(repo)) {
+ RevCommit c = makeCommit(rw, "Commit no OWNERS.", FILES_WITHOUT_OWNERS);
+ assertThat(validate(rw, c, false, ENABLED_CONFIG)).isEmpty();
+ assertThat(validate(rw, c, true, ENABLED_CONFIG)).isEmpty();
+ }
+ }
+
+ private static final ImmutableMap<String, String> FILES_WITH_NO_ERROR =
+ ImmutableMap.of(
+ OWNERS,
+ "\n\n#comments ...\n ### more comments\n"
+ + " user1@google.com # comment\n"
+ + "u1+review@g.com###\n"
+ + " * # everyone can approve\n"
+ + "per-file *.py = set noparent###\n"
+ + "per-file *.py=u2.m@g.com\n"
+ + "per-file *.txt = * # everyone can approve\n"
+ + "set noparent # comment\n");
+
+ private static final ImmutableSet<String> EXPECTED_VERBOSE_OUTPUT =
+ ImmutableSet.of(
+ "MSG: validate: " + OWNERS,
+ "MSG: owner: user1@google.com",
+ "MSG: owner: u1+review@g.com",
+ "MSG: owner: u2.m@g.com");
+
+ @Test
+ public void testGoodInput() throws Exception {
+ try (RevWalk rw = new RevWalk(repo)) {
+ RevCommit c = makeCommit(rw, "Commit good files", FILES_WITH_NO_ERROR);
+ assertThat(validate(rw, c, false, ENABLED_CONFIG)).isEmpty();
+ assertThat(validate(rw, c, true, ENABLED_CONFIG))
+ .containsExactlyElementsIn(EXPECTED_VERBOSE_OUTPUT);
+ }
+ }
+
+ private static final ImmutableMap<String, String> FILES_WITH_WRONG_SYNTAX =
+ ImmutableMap.of(
+ "README",
+ "# some content\nu2@g.com\n",
+ OWNERS,
+ "\nwrong syntax\n#comment\nuser1@google.com\n",
+ "d2/" + OWNERS,
+ "u1@g.com\nu3@g.com\n*\n",
+ "d3/" + OWNERS,
+ "\nfile: common/Owners\n");
+
+ private static final ImmutableSet<String> EXPECTED_WRONG_SYNTAX =
+ ImmutableSet.of(
+ "ERROR: syntax: " + OWNERS + ":2: wrong syntax",
+ "ERROR: unknown: u3@g.com at d2/" + OWNERS + ":2",
+ "ERROR: ignored: d3/" + OWNERS + ":2: file: common/Owners");
+
+ 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 + ":2: wrong syntax",
+ "ERROR: unknown: u3@g.com at d2/" + OWNERS + ":2",
+ "ERROR: ignored: d3/" + OWNERS + ":2: file: common/Owners");
+
+ @Test
+ public void testWrongSyntax() throws Exception {
+ try (RevWalk rw = new RevWalk(repo)) {
+ RevCommit c = makeCommit(rw, "Commit wrong syntax", FILES_WITH_WRONG_SYNTAX);
+ assertThat(validate(rw, c, false, ENABLED_CONFIG))
+ .containsExactlyElementsIn(EXPECTED_WRONG_SYNTAX);
+ assertThat(validate(rw, c, true, ENABLED_CONFIG))
+ .containsExactlyElementsIn(EXPECTED_VERBOSE_WRONG_SYNTAX);
+ }
+ }
+
+ private static final ImmutableMap<String, String> FILES_WITH_WRONG_EMAILS =
+ ImmutableMap.of("d1/" + OWNERS, "u1@g.com\n", "d2/" + OWNERS_ANDROID, "u2@g.com\n");
+
+ private static final ImmutableSet<String> EXPECTED_VERBOSE_DEFAULT =
+ ImmutableSet.of("MSG: validate: d1/" + OWNERS, "MSG: owner: u1@g.com");
+
+ private static final ImmutableSet<String> EXPECTED_VERBOSE_ANDROID =
+ ImmutableSet.of("MSG: validate: d2/" + OWNERS_ANDROID, "MSG: owner: u2@g.com");
+
+ @Test
+ public void checkWrongEmails() throws Exception {
+ try (RevWalk rw = new RevWalk(repo)) {
+ RevCommit c = makeCommit(rw, "Commit Default", FILES_WITH_WRONG_EMAILS);
+ assertThat(validate(rw, c, true, ENABLED_CONFIG))
+ .containsExactlyElementsIn(EXPECTED_VERBOSE_DEFAULT);
+ }
+ }
+
+ @Test
+ public void checkAndroidOwners() throws Exception {
+ try (RevWalk rw = new RevWalk(repo)) {
+ RevCommit c = makeCommit(rw, "Commit Android", FILES_WITH_WRONG_EMAILS);
+ assertThat(validate(rw, c, true, ANDROID_CONFIG))
+ .containsExactlyElementsIn(EXPECTED_VERBOSE_ANDROID);
+ }
+ }
+
+ private static PluginConfig createEnabledConfig() {
+ PluginConfig c = new PluginConfig("", new Config());
+ c.setBoolean(REJECT_ERROR_IN_OWNERS, true);
+ return c;
+ }
+
+ private static PluginConfig createDisabledConfig() {
+ PluginConfig c = new PluginConfig("", new Config());
+ c.setBoolean(REJECT_ERROR_IN_OWNERS, false);
+ return c;
+ }
+
+ private static PluginConfig createAndroidConfig() {
+ PluginConfig c = createEnabledConfig();
+ c.setString(OWNERS_FILE_NAME, OWNERS_ANDROID);
+ return c;
+ }
+
+ private RevCommit makeCommit(RevWalk rw, String message, Map<String, String> fileStrings)
+ throws IOException, GitAPIException {
+ Map<File, byte[]> fileBytes = new HashMap<>();
+ for (String path : fileStrings.keySet()) {
+ fileBytes.put(
+ new File(repo.getDirectory().getParent(), path),
+ fileStrings.get(path).getBytes(StandardCharsets.UTF_8));
+ }
+ return makeCommit(rw, repo, message, fileBytes);
+ }
+
+ 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);
+ String ownersFileName = OwnersValidator.getOwnersFileName(cfg);
+ List<CommitValidationMessage> m = validator.performValidation(c, rw, ownersFileName, verbose);
+ return transformMessages(m);
+ }
+
+ private static String generateFilePattern(File f, Git git) {
+ return f.getAbsolutePath()
+ .replace(git.getRepository().getWorkTree().getAbsolutePath(), "")
+ .substring(1);
+ }
+
+ private static void addFiles(Git git, Map<File, byte[]> files)
+ throws IOException, GitAPIException {
+ AddCommand ac = git.add();
+ for (File f : files.keySet()) {
+ if (!f.exists()) {
+ FileUtils.touch(f);
+ }
+ if (files.get(f) != null) {
+ FileUtils.writeByteArrayToFile(f, files.get(f));
+ }
+ ac = ac.addFilepattern(generateFilePattern(f, git));
+ }
+ ac.call();
+ }
+
+ private static RevCommit makeCommit(
+ RevWalk rw, Repository repo, String message, Map<File, byte[]> files)
+ throws IOException, GitAPIException {
+ try (Git git = new Git(repo)) {
+ if (files != null) {
+ addFiles(git, files);
+ }
+ return rw.parseCommit(git.commit().setMessage(message).call());
+ }
+ }
+
+ private static List<String> transformMessages(List<CommitValidationMessage> messages) {
+ return Lists.transform(
+ messages,
+ new Function<CommitValidationMessage, String>() {
+ @Override
+ public String apply(CommitValidationMessage input) {
+ String pre = (input.isError()) ? "ERROR: " : "MSG: ";
+ return pre + input.getMessage();
+ }
+ });
+ }
+
+ @Test
+ public void testTransformer() {
+ List<CommitValidationMessage> messages = new LinkedList<>();
+ messages.add(new CommitValidationMessage("a message", false));
+ messages.add(new CommitValidationMessage("an error", true));
+ Set<String> expected = ImmutableSet.of("ERROR: an error", "MSG: a message");
+ assertThat(transformMessages(messages)).containsExactlyElementsIn(expected);
+ }
+}
