Merge branch 'stable-3.3' into stable-3.4
* stable-3.3:
Trigger owners-autoassign processing for change readyForReview
Fix matcher inheritance between OWNERS files
Allow adding reviewers in the OWNERS file
Don't use local disk in integration tests
Fix test module package name for owners-autoassign integration-tests
Change-Id: I6aa63e7de579a04a505e6f0e849e7f171f928666
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
index 81559f7..7792583 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
@@ -18,10 +18,12 @@
import static com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace.IGNORE_NONE;
+import com.google.common.base.Predicates;
import com.google.common.collect.Sets;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.annotations.Listen;
@@ -32,8 +34,10 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListKey;
@@ -46,6 +50,7 @@
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -64,6 +69,8 @@
private Provider<CurrentUser> currentUserProvider;
+ private ChangeNotes.Factory notesFactory;
+
@Inject
public GitRefListener(
GerritApi api,
@@ -72,7 +79,8 @@
Accounts accounts,
ReviewerManager reviewerManager,
OneOffRequestContext oneOffReqCtx,
- Provider<CurrentUser> currentUserProvider) {
+ Provider<CurrentUser> currentUserProvider,
+ ChangeNotes.Factory notesFactory) {
this.api = api;
this.patchListCache = patchListCache;
this.repositoryManager = repositoryManager;
@@ -80,6 +88,7 @@
this.reviewerManager = reviewerManager;
this.oneOffReqCtx = oneOffReqCtx;
this.currentUserProvider = currentUserProvider;
+ this.notesFactory = notesFactory;
}
@Override
@@ -120,11 +129,15 @@
String projectName = event.getProjectName();
Repository repository;
try {
- repository = repositoryManager.openRepository(Project.NameKey.parse(projectName));
+ NameKey projectNameKey = Project.NameKey.parse(projectName);
+ repository = repositoryManager.openRepository(projectNameKey);
try {
String refName = event.getRefName();
- if (refName.startsWith(RefNames.REFS_CHANGES) && !RefNames.isNoteDbMetaRef(refName)) {
- processEvent(repository, event);
+ Change.Id changeId = Change.Id.fromRef(refName);
+ if (changeId != null
+ && (!RefNames.isNoteDbMetaRef(refName)
+ || isChangeSetReadyForReview(projectNameKey, changeId, event.getNewObjectId()))) {
+ processEvent(repository, event, changeId);
}
} finally {
repository.close();
@@ -134,21 +147,33 @@
}
}
- public void processEvent(Repository repository, Event event) {
- Change.Id cId = Change.Id.fromRef(event.getRefName());
+ private boolean isChangeSetReadyForReview(
+ NameKey project, Change.Id changeId, String metaObjectId) {
+ ChangeNotes changeNotes = notesFactory.createChecked(project, changeId);
+ return !changeNotes.getChange().isWorkInProgress()
+ && changeNotes.getChangeMessages().stream()
+ .filter(message -> message.getKey().uuid().equals(metaObjectId))
+ .map(message -> message.getTag())
+ .filter(Predicates.notNull())
+ .anyMatch(tag -> tag.contains(ChangeMessagesUtil.TAG_SET_READY));
+ }
+
+ public void processEvent(Repository repository, Event event, Change.Id cId) {
Changes changes = api.changes();
// The provider injected by Gerrit is shared with other workers on the
// same local thread and thus cannot be closed in this event listener.
try {
ChangeApi cApi = changes.id(cId.get());
ChangeInfo change = cApi.get();
- PatchList patchList = getPatchList(event, change);
+ PatchList patchList = getPatchList(repository, event, change);
if (patchList != null) {
PathOwners owners = new PathOwners(accounts, repository, change.branch, patchList);
Set<Account.Id> allReviewers = Sets.newHashSet();
allReviewers.addAll(owners.get().values());
+ allReviewers.addAll(owners.getReviewers().values());
for (Matcher matcher : owners.getMatchers().values()) {
allReviewers.addAll(matcher.getOwners());
+ allReviewers.addAll(matcher.getReviewers());
}
logger.debug("Autoassigned reviewers are: {}", allReviewers.toString());
reviewerManager.addReviewers(cApi, allReviewers);
@@ -160,17 +185,23 @@
}
}
- private PatchList getPatchList(Event event, ChangeInfo change) {
+ private PatchList getPatchList(Repository repository, Event event, ChangeInfo change) {
ObjectId newId = null;
- if (event.getNewObjectId() != null) {
- newId = ObjectId.fromString(event.getNewObjectId());
- }
-
- PatchListKey plKey = PatchListKey.againstCommit(null, newId, IGNORE_NONE);
+ PatchListKey plKey;
try {
+ if (RefNames.isNoteDbMetaRef(event.getRefName())) {
+ newId = ObjectId.fromString(change.currentRevision);
+ RevCommit revCommit = repository.parseCommit(newId);
+ plKey = PatchListKey.againstBase(newId, revCommit.getParentCount());
+ } else {
+ if (event.getNewObjectId() != null) {
+ newId = ObjectId.fromString(event.getNewObjectId());
+ }
+ plKey = PatchListKey.againstCommit(null, newId, IGNORE_NONE);
+ }
return patchListCache.get(plKey, Project.nameKey(change.project));
- } catch (PatchListNotAvailableException e) {
- logger.warn("Could not load patch list: {}", plKey, e);
+ } catch (PatchListNotAvailableException | IOException e) {
+ logger.warn("Could not load patch list for change {}", change.id, e);
}
return null;
}
diff --git a/owners-autoassign/src/main/resources/Documentation/config.md b/owners-autoassign/src/main/resources/Documentation/config.md
index b294ae8..6db4b1d 100644
--- a/owners-autoassign/src/main/resources/Documentation/config.md
+++ b/owners-autoassign/src/main/resources/Documentation/config.md
@@ -84,6 +84,27 @@
That means that in the absence of any OWNERS file in the target branch, the refs/meta/config
OWNERS is used as global default.
+## Additional non-owners added as reviewers
+
+The OWNERS file can also contain a section called `reviewers` that allows
+to add extra people as reviewers to a change without having to make them
+owners and therefore without having any impact on the underlying validation
+rules.
+
+See for instance the example below, where `john@example.com` is added as an additional
+reviewer in addition to the owners.
+
+```yaml
+inherited: true
+owners:
+- some.email@example.com
+- User Name
+reviewers:
+- john@example.com
+```
+
+The `reviewers` optional section can be added in any place where `owners` is specified
+and can be also associated with matchers exactly in the same way that `owners` do.
## Example 1 - OWNERS file without matchers and default Gerrit submit rules
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java
new file mode 100644
index 0000000..e8fa11b
--- /dev/null
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java
@@ -0,0 +1,222 @@
+// Copyright (C) 2021 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.owners.common;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.inject.AbstractModule;
+import java.util.Collection;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.junit.Ignore;
+import org.junit.Test;
+
+@Ignore
+public abstract class AbstractAutoassignIT extends LightweightPluginDaemonTest {
+ private final String section;
+ private final boolean INHERITED = true;
+ private final boolean NOT_INHERITED = false;
+
+ AbstractAutoassignIT(String section) {
+ this.section = section;
+ }
+
+ public static class TestModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ install(new AutoassignModule());
+ }
+ }
+
+ @Test
+ public void shouldAutoassignOneOwner() throws Exception {
+ String ownerEmail = user.email();
+
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ "Set OWNERS",
+ "OWNERS",
+ "inherited: false\n" + "owners:\n" + "- " + ownerEmail)
+ .to("refs/heads/master")
+ .assertOkStatus();
+
+ ChangeApi changeApi = change(createChange());
+ Collection<AccountInfo> reviewers = changeApi.get().reviewers.get(ReviewerState.REVIEWER);
+ }
+
+ @Test
+ public void shouldAutoassignUserInPath() throws Exception {
+ String ownerEmail = user.email();
+
+ addOwnersToRepo("", ownerEmail, NOT_INHERITED);
+
+ Collection<AccountInfo> reviewers = getChangeReviewers(change(createChange()));
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewers).hasSize(1);
+ assertThat(reviewersEmail(reviewers).get(0)).isEqualTo(ownerEmail);
+ }
+
+ @Test
+ public void shouldAutoassignUserInPathWithInheritance() throws Exception {
+ String childOwnersEmail = accountCreator.user2().email();
+ String parentOwnersEmail = user.email();
+ String childpath = "childpath/";
+
+ addOwnersToRepo("", parentOwnersEmail, NOT_INHERITED);
+ addOwnersToRepo(childpath, childOwnersEmail, INHERITED);
+
+ Collection<AccountInfo> reviewers =
+ getChangeReviewers(change(createChange("test change", childpath + "foo.txt", "foo")));
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewersEmail(reviewers)).containsExactly(parentOwnersEmail, childOwnersEmail);
+ }
+
+ @Test
+ public void shouldAutoassignUserInPathWithoutInheritance() throws Exception {
+ String childOwnersEmail = accountCreator.user2().email();
+ String parentOwnersEmail = user.email();
+ String childpath = "childpath/";
+
+ addOwnersToRepo("", parentOwnersEmail, NOT_INHERITED);
+ addOwnersToRepo(childpath, childOwnersEmail, NOT_INHERITED);
+
+ Collection<AccountInfo> reviewers =
+ getChangeReviewers(change(createChange("test change", childpath + "foo.txt", "foo")));
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewersEmail(reviewers)).containsExactly(childOwnersEmail);
+ }
+
+ @Test
+ public void shouldAutoassignUserMatchingPath() throws Exception {
+ String ownerEmail = user.email();
+
+ addOwnersToRepo("", "suffix", ".java", ownerEmail, NOT_INHERITED);
+
+ Collection<AccountInfo> reviewers =
+ getChangeReviewers(change(createChange("test change", "foo.java", "foo")));
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewersEmail(reviewers)).containsExactly(ownerEmail);
+ }
+
+ @Test
+ public void shouldNotAutoassignUserNotMatchingPath() throws Exception {
+ String ownerEmail = user.email();
+
+ addOwnersToRepo("", "suffix", ".java", ownerEmail, NOT_INHERITED);
+
+ ChangeApi changeApi = change(createChange("test change", "foo.bar", "foo"));
+ Collection<AccountInfo> reviewers = getChangeReviewers(changeApi);
+
+ assertThat(reviewers).isNull();
+ }
+
+ @Test
+ public void shouldAutoassignUserMatchingPathWithInheritance() throws Exception {
+ String childOwnersEmail = accountCreator.user2().email();
+ String parentOwnersEmail = user.email();
+ String childpath = "childpath/";
+
+ addOwnersToRepo("", "suffix", ".java", parentOwnersEmail, NOT_INHERITED);
+ addOwnersToRepo(childpath, "suffix", ".java", childOwnersEmail, INHERITED);
+
+ ChangeApi changeApi = change(createChange("test change", childpath + "foo.java", "foo"));
+ Collection<AccountInfo> reviewers = getChangeReviewers(changeApi);
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewersEmail(reviewers)).containsExactly(parentOwnersEmail, childOwnersEmail);
+ }
+
+ @Test
+ public void shouldAutoassignUserMatchingPathWithoutInheritance() throws Exception {
+ String childOwnersEmail = accountCreator.user2().email();
+ String parentOwnersEmail = user.email();
+ String childpath = "childpath/";
+
+ addOwnersToRepo("", parentOwnersEmail, NOT_INHERITED);
+ addOwnersToRepo(childpath, "suffix", ".java", childOwnersEmail, NOT_INHERITED);
+
+ ChangeApi changeApi = change(createChange("test change", childpath + "foo.java", "foo"));
+ Collection<AccountInfo> reviewers = getChangeReviewers(changeApi);
+
+ assertThat(reviewers).isNotNull();
+ assertThat(reviewersEmail(reviewers)).containsExactly(childOwnersEmail);
+ }
+
+ private Collection<AccountInfo> getChangeReviewers(ChangeApi changeApi) throws RestApiException {
+ Collection<AccountInfo> reviewers = changeApi.get().reviewers.get(ReviewerState.REVIEWER);
+ return reviewers;
+ }
+
+ private List<String> reviewersEmail(Collection<AccountInfo> reviewers) {
+ List<String> reviewersEmail = reviewers.stream().map(a -> a.email).collect(Collectors.toList());
+ return reviewersEmail;
+ }
+
+ private void addOwnersToRepo(String parentPath, String ownerEmail, boolean inherited)
+ throws Exception {
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ "Set OWNERS",
+ parentPath + "OWNERS",
+ "inherited: " + inherited + "\n" + section + ":\n" + "- " + ownerEmail)
+ .to("refs/heads/master")
+ .assertOkStatus();
+ }
+
+ private void addOwnersToRepo(
+ String parentPath,
+ String matchingType,
+ String patternMatch,
+ String ownerEmail,
+ boolean inherited)
+ throws Exception {
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ "Set OWNERS",
+ parentPath + "OWNERS",
+ "inherited: "
+ + inherited
+ + "\n"
+ + "matchers:\n"
+ + "- "
+ + matchingType
+ + ": "
+ + patternMatch
+ + "\n"
+ + " "
+ + section
+ + ":\n"
+ + " - "
+ + ownerEmail)
+ .to("refs/heads/master")
+ .assertOkStatus();
+ }
+}
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignIT.java
index 374657c..dbe6d41 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignIT.java
@@ -15,50 +15,14 @@
package com.googlesource.gerrit.owners.common;
-import static com.google.common.truth.Truth.assertThat;
-
-import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.TestPlugin;
-import com.google.gerrit.acceptance.UseLocalDisk;
-import com.google.gerrit.extensions.api.changes.ChangeApi;
-import com.google.gerrit.extensions.client.ReviewerState;
-import com.google.gerrit.extensions.common.AccountInfo;
-import com.google.inject.AbstractModule;
-import java.util.Collection;
-import org.junit.Test;
@TestPlugin(
name = "owners-api",
- sysModule = "com.googlesource.gerrit.owners.common.OwnersAutoassignIT$TestModule")
-public class OwnersAutoassignIT extends LightweightPluginDaemonTest {
+ sysModule = "com.googlesource.gerrit.owners.common.AbstractAutoassignIT$TestModule")
+public class OwnersAutoassignIT extends AbstractAutoassignIT {
- public static class TestModule extends AbstractModule {
- @Override
- protected void configure() {
- install(new AutoassignModule());
- }
- }
-
- @UseLocalDisk
- @Test
- public void shouldAutoassignOneOwner() throws Exception {
- String ownerEmail = user.email();
-
- pushFactory
- .create(
- admin.newIdent(),
- testRepo,
- "Set OWNERS",
- "OWNERS",
- "inherited: false\n" + "owners:\n" + "- " + ownerEmail)
- .to("refs/heads/master")
- .assertOkStatus();
-
- ChangeApi changeApi = change(createChange());
- Collection<AccountInfo> reviewers = changeApi.get().reviewers.get(ReviewerState.REVIEWER);
-
- assertThat(reviewers).hasSize(1);
- AccountInfo reviewer = reviewers.iterator().next();
- assertThat(reviewer.email).isEqualTo(ownerEmail);
+ public OwnersAutoassignIT() {
+ super("owners");
}
}
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
index 5495240..b26b2f9 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
@@ -20,7 +20,6 @@
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.TestPlugin;
-import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.entities.Account.Id;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -35,8 +34,7 @@
@TestPlugin(
name = "owners-autoassign",
- sysModule =
- "com.googlesource.gerrit.owners.common.OwnersAutoassignWithAttentionSetIT$TestModule")
+ sysModule = "com.googlesource.gerrit.owners.common.OwnersAutoassignWithAttentionSetIT$TestModule")
public class OwnersAutoassignWithAttentionSetIT extends LightweightPluginDaemonTest {
@Override
@@ -63,7 +61,6 @@
}
}
- @UseLocalDisk
@Test
public void shouldAutoassignTwoOwnersWithOneAttentionSet() throws Exception {
String ownerEmail1 = user.email();
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/ReviewersAutoassignIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/ReviewersAutoassignIT.java
new file mode 100644
index 0000000..c88edfb
--- /dev/null
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/ReviewersAutoassignIT.java
@@ -0,0 +1,28 @@
+// Copyright (C) 2021 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.owners.common;
+
+import com.google.gerrit.acceptance.TestPlugin;
+
+@TestPlugin(
+ name = "owners-api",
+ sysModule = "com.googlesource.gerrit.owners.common.AbstractAutoassignIT$TestModule")
+public class ReviewersAutoassignIT extends AbstractAutoassignIT {
+
+ public ReviewersAutoassignIT() {
+ super("reviewers");
+ }
+}
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
index 5c9f1a7..1e0f8ec 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerIT.java
@@ -21,11 +21,14 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.registration.Extension;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
+import java.util.stream.StreamSupport;
import org.eclipse.jgit.transport.ReceiveCommand.Type;
import org.junit.Test;
@@ -34,62 +37,97 @@
sysModule = "com.vmware.gerrit.owners.common.GitRefListenerIT$TestModule")
public class GitRefListenerIT extends LightweightPluginDaemonTest {
- @Inject GitRefListenerTest gitRefListener;
+ @Inject DynamicSet<GitReferenceUpdatedListener> allRefUpdateListeners;
@Inject ThreadLocalRequestContext requestContext;
- String aRefChange = RefNames.REFS_CHANGES + "01/01/01";
String anOldObjectId = "anOldRef";
String aNewObjectId = "aNewRef";
public static class TestModule extends AbstractModule {
@Override
protected void configure() {
- bind(GitReferenceUpdatedListener.class).to(GitRefListenerTest.class);
+ DynamicSet.bind(binder(), GitReferenceUpdatedListener.class).to(GitRefListenerTest.class);
}
}
@Test
- public void shouldNotProcessNoteDbOnlyRefs() {
+ public void shouldNotProcessNoteDbOnlyRefs() throws Exception {
+ String changeRefPrefix = createChange().getChange().getId().toRefPrefix();
+ int baselineProcessedEvents = gitRefListener().getProcessedEvents();
+
ReferenceUpdatedEventTest refUpdatedEvent =
new ReferenceUpdatedEventTest(
project,
- RefNames.REFS_CHANGES + "01/01" + RefNames.META_SUFFIX,
+ changeRefPrefix + RefNames.META_SUFFIX.substring(1),
anOldObjectId,
aNewObjectId,
Type.CREATE,
admin.id());
- gitRefListener.onGitReferenceUpdated(refUpdatedEvent);
- assertEquals(0, gitRefListener.getProcessedEvents());
+ gitRefListener().onGitReferenceUpdated(refUpdatedEvent);
+ assertEquals(baselineProcessedEvents, gitRefListener().getProcessedEvents());
}
@Test
- public void shouldProcessRefChanges() {
- gitRefListener.onGitReferenceUpdated(newRefUpdateEvent());
- assertEquals(1, gitRefListener.getProcessedEvents());
+ public void shoulProcessSetReadyForReviewOnNoteDb() throws Exception {
+ int wipChangeNum = createChange().getChange().getId().get();
+ gApi.changes().id(wipChangeNum).setWorkInProgress();
+
+ int baselineProcessedEvents = gitRefListener().getProcessedEvents();
+
+ gApi.changes().id(wipChangeNum).setReadyForReview();
+ assertEquals(1, gitRefListener().getProcessedEvents() - baselineProcessedEvents);
+ }
+
+ @Test
+ public void shoulNotProcessEmptyMetaRefUpdatesAfterSetReadyForReviewOnNoteDb() throws Exception {
+ int wipChangeNum = createChange().getChange().getId().get();
+ gApi.changes().id(wipChangeNum).setWorkInProgress();
+ gApi.changes().id(wipChangeNum).setReadyForReview();
+ int baselineProcessedEvents = gitRefListener().getProcessedEvents();
+
+ gApi.changes().id(wipChangeNum).addReviewer("user");
+ assertEquals(baselineProcessedEvents, gitRefListener().getProcessedEvents());
+ }
+
+ @Test
+ public void shouldProcessRefChanges() throws Exception {
+ gitRefListener().onGitReferenceUpdated(newRefUpdateEvent());
+ assertEquals(1, gitRefListener().getProcessedEvents());
}
@Test
public void shouldRetrieveChangeFromAnonymousContext() throws Exception {
try (ManualRequestContext ctx = new ManualRequestContext(new AnonymousUser(), requestContext)) {
- gitRefListener.onGitReferenceUpdated(newRefUpdateEvent());
- assertEquals(1, gitRefListener.getProcessedEvents());
+ gitRefListener().onGitReferenceUpdated(newRefUpdateEvent());
+ assertEquals(1, gitRefListener().getProcessedEvents());
}
}
@Test
public void shouldRetrieveChangeFromAnonymousContextWithoutAccountId() throws Exception {
+ String refPrefix = createChange().getChange().getId().toRefPrefix();
ReferenceUpdatedEventTest refUpdateWithoutAccountId =
new ReferenceUpdatedEventTest(
- project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE, null);
+ project, refPrefix, anOldObjectId, aNewObjectId, Type.CREATE, null);
try (ManualRequestContext ctx = new ManualRequestContext(new AnonymousUser(), requestContext)) {
- gitRefListener.onGitReferenceUpdated(refUpdateWithoutAccountId);
- assertEquals(1, gitRefListener.getProcessedEvents());
+ gitRefListener().onGitReferenceUpdated(refUpdateWithoutAccountId);
+ assertEquals(1, gitRefListener().getProcessedEvents());
}
}
- private ReferenceUpdatedEventTest newRefUpdateEvent() {
+ private GitRefListenerTest gitRefListener() {
+ return (GitRefListenerTest)
+ StreamSupport.stream(allRefUpdateListeners.entries().spliterator(), false)
+ .map(Extension::get)
+ .filter(listener -> GitRefListenerTest.class.isAssignableFrom(listener.getClass()))
+ .findFirst()
+ .get();
+ }
+
+ private ReferenceUpdatedEventTest newRefUpdateEvent() throws Exception {
+ String refPrefix = createChange().getChange().getId().toRefPrefix();
return new ReferenceUpdatedEventTest(
- project, aRefChange, anOldObjectId, aNewObjectId, Type.CREATE, admin.id());
+ project, refPrefix, anOldObjectId, aNewObjectId, Type.CREATE, admin.id());
}
}
diff --git a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
index e3d4be4..75a9189 100644
--- a/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
+++ b/owners-autoassign/src/test/java/com/vmware/gerrit/owners/common/GitRefListenerTest.java
@@ -15,13 +15,17 @@
package com.vmware.gerrit.owners.common;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener.Event;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import com.google.inject.Singleton;
import com.googlesource.gerrit.owners.common.Accounts;
import com.googlesource.gerrit.owners.common.GitRefListener;
import com.googlesource.gerrit.owners.common.ReviewerManager;
@@ -29,6 +33,7 @@
import org.junit.Ignore;
@Ignore
+@Singleton
public class GitRefListenerTest extends GitRefListener {
int processedEvents = 0;
@@ -40,7 +45,8 @@
Accounts accounts,
ReviewerManager reviewerManager,
OneOffRequestContext oneOffReqCtx,
- Provider<CurrentUser> currentUserProvider) {
+ Provider<CurrentUser> currentUserProvider,
+ ChangeNotes.Factory notesFactory) {
super(
api,
patchListCache,
@@ -48,11 +54,12 @@
accounts,
reviewerManager,
oneOffReqCtx,
- currentUserProvider);
+ currentUserProvider,
+ notesFactory);
}
@Override
- public void processEvent(Repository repository, Event event) {
+ public void processEvent(Repository repository, Event event, Change.Id cId) {
processedEvents++;
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
index 5b1049d..2fa2b16 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
@@ -30,7 +30,6 @@
import org.slf4j.LoggerFactory;
public class ConfigurationParser {
-
private static final Logger log = LoggerFactory.getLogger(OwnersConfig.class);
private Accounts accounts;
@@ -55,21 +54,14 @@
}
private void addClassicMatcher(JsonNode jsonNode, OwnersConfig ret) {
- Optional<Stream<String>> owners =
- Optional.ofNullable(jsonNode.get("owners")).map(ConfigurationParser::extractOwners);
- ret.setOwners(flattenSet(owners));
- }
-
- private static <T> Set<T> flattenSet(Optional<Stream<T>> optionalStream) {
- return flatten(optionalStream).collect(Collectors.toSet());
- }
-
- private static <T> Stream<T> flatten(Optional<Stream<T>> optionalStream) {
- return optionalStream.orElse(Stream.empty());
+ ret.setOwners(toClassicOwnersList(jsonNode, "owners").collect(Collectors.toSet()));
+ ret.setReviewers(toClassicOwnersList(jsonNode, "reviewers").collect(Collectors.toSet()));
}
private void addMatchers(JsonNode jsonNode, OwnersConfig ret) {
- getNode(jsonNode, "matchers").map(this::getMatchers).ifPresent(m -> m.forEach(ret::addMatcher));
+ getNode(jsonNode, "matchers")
+ .map(m -> getMatchers(m))
+ .ifPresent(m -> m.forEach(ret::addMatcher));
}
private Stream<Matcher> getMatchers(JsonNode node) {
@@ -79,29 +71,43 @@
.map(m -> m.get());
}
- private static Stream<String> extractOwners(JsonNode node) {
+ private static Stream<String> extractAsText(JsonNode node) {
if (node.isTextual()) {
return Stream.of(node.asText());
}
return iteratorStream(node.iterator()).map(JsonNode::asText);
}
+ private Stream<String> toClassicOwnersList(JsonNode jsonNode, String sectionName) {
+ Stream<String> ownersStream =
+ Optional.ofNullable(jsonNode.get(sectionName))
+ .map(ConfigurationParser::extractAsText)
+ .orElse(Stream.empty());
+ return ownersStream;
+ }
+
private Optional<Matcher> toMatcher(JsonNode node) {
Set<Id> owners =
- flatten(getNode(node, "owners").map(ConfigurationParser::extractOwners))
+ getNode(node, "owners")
+ .map(ConfigurationParser::extractAsText)
+ .orElse(Stream.empty())
.flatMap(o -> accounts.find(o).stream())
.collect(Collectors.toSet());
- if (owners.isEmpty()) {
- log.warn("Matchers must contain a list of owners");
- return Optional.empty();
- }
+ Set<Id> reviewers =
+ getNode(node, "reviewers")
+ .map(ConfigurationParser::extractAsText)
+ .orElse(Stream.empty())
+ .flatMap(o -> accounts.find(o).stream())
+ .collect(Collectors.toSet());
Optional<Matcher> suffixMatcher =
- getText(node, "suffix").map(el -> new SuffixMatcher(el, owners));
- Optional<Matcher> regexMatcher = getText(node, "regex").map(el -> new RegExMatcher(el, owners));
+ getText(node, "suffix").map(el -> new SuffixMatcher(el, owners, reviewers));
+ Optional<Matcher> regexMatcher =
+ getText(node, "regex").map(el -> new RegExMatcher(el, owners, reviewers));
Optional<Matcher> partialRegexMatcher =
- getText(node, "partial_regex").map(el -> new PartialRegExMatcher(el, owners));
- Optional<Matcher> exactMatcher = getText(node, "exact").map(el -> new ExactMatcher(el, owners));
+ getText(node, "partial_regex").map(el -> new PartialRegExMatcher(el, owners, reviewers));
+ Optional<Matcher> exactMatcher =
+ getText(node, "exact").map(el -> new ExactMatcher(el, owners, reviewers));
return Optional.ofNullable(
suffixMatcher.orElseGet(
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ExactMatcher.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ExactMatcher.java
index 07dadbe..d97b01e 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ExactMatcher.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ExactMatcher.java
@@ -16,15 +16,21 @@
package com.googlesource.gerrit.owners.common;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Account.Id;
import java.util.Set;
public class ExactMatcher extends Matcher {
- public ExactMatcher(String path, Set<Account.Id> owners) {
- super(path, owners);
+ public ExactMatcher(String path, Set<Account.Id> owners, Set<Account.Id> reviewers) {
+ super(path, owners, reviewers);
}
@Override
public boolean matches(String pathToMatch) {
return pathToMatch.equals(path);
}
+
+ @Override
+ protected Matcher clone(Set<Id> owners, Set<Id> reviewers) {
+ return new ExactMatcher(path, owners, reviewers);
+ }
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/Matcher.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/Matcher.java
index 412fa79..7c1565f 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/Matcher.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/Matcher.java
@@ -14,21 +14,25 @@
package com.googlesource.gerrit.owners.common;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Account.Id;
import java.util.Set;
public abstract class Matcher {
private Set<Account.Id> owners;
+ private Set<Account.Id> reviewers;
protected String path;
- public Matcher(String key, Set<Account.Id> owners) {
+ public Matcher(String key, Set<Account.Id> owners, Set<Account.Id> reviewers) {
this.path = key;
this.owners = owners;
+ this.reviewers = reviewers;
}
@Override
public String toString() {
- return "Matcher [path=" + path + ", owners=" + owners + "]";
+ return "Matcher [path=" + path + ", owners=" + owners + ", reviewers=" + reviewers + "]";
}
public Set<Account.Id> getOwners() {
@@ -39,6 +43,14 @@
this.owners = owners;
}
+ public Set<Account.Id> getReviewers() {
+ return reviewers;
+ }
+
+ public void setReviewers(Set<Account.Id> reviewers) {
+ this.reviewers = reviewers;
+ }
+
public void setPath(String path) {
this.path = path;
}
@@ -48,4 +60,19 @@
}
public abstract boolean matches(String pathToMatch);
+
+ public Matcher merge(Matcher other) {
+ if (other == null) {
+ return this;
+ }
+
+ return clone(mergeSet(owners, other.owners), mergeSet(reviewers, other.reviewers));
+ }
+
+ protected abstract Matcher clone(Set<Id> owners, Set<Id> reviewers);
+
+ private Set<Id> mergeSet(Set<Id> set1, Set<Id> set2) {
+ ImmutableSet.Builder<Id> setBuilder = ImmutableSet.builder();
+ return setBuilder.addAll(set1).addAll(set2).build();
+ }
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
index 0c29516..d6e40eb 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
@@ -30,8 +30,8 @@
/** Flag for marking that this OWNERS file inherits from the parent OWNERS. */
private boolean inherited = true;
- /** Set of OWNER email addresses. */
private Set<String> owners = Sets.newHashSet();
+ private Set<String> reviewers = Sets.newHashSet();
/** Map name of matcher and Matcher (value + Set Owners) */
private Map<String, Matcher> matchers = Maps.newHashMap();
@@ -63,6 +63,14 @@
this.owners = owners;
}
+ public Set<String> getReviewers() {
+ return reviewers;
+ }
+
+ public void setReviewers(Set<String> reviewers) {
+ this.reviewers = reviewers;
+ }
+
public Map<String, Matcher> getMatchers() {
return matchers;
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
index 06d9d12..111b88e 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
@@ -25,8 +25,10 @@
public class OwnersMap {
private SetMultimap<String, Account.Id> pathOwners = HashMultimap.create();
+ private SetMultimap<String, Account.Id> pathReviewers = HashMultimap.create();
private Map<String, Matcher> matchers = Maps.newHashMap();
private Map<String, Set<Account.Id>> fileOwners = Maps.newHashMap();
+ private Map<String, Set<Account.Id>> fileReviewers = Maps.newHashMap();
@Override
public String toString() {
@@ -45,6 +47,14 @@
this.pathOwners = pathOwners;
}
+ public SetMultimap<String, Account.Id> getPathReviewers() {
+ return pathReviewers;
+ }
+
+ public void setPathReviewers(SetMultimap<String, Account.Id> pathReviewers) {
+ this.pathReviewers = pathReviewers;
+ }
+
public Map<String, Matcher> getMatchers() {
return matchers;
}
@@ -57,10 +67,18 @@
pathOwners.putAll(ownersPath, owners);
}
+ public void addPathReviewers(String ownersPath, Set<Id> reviewers) {
+ pathOwners.putAll(ownersPath, reviewers);
+ }
+
public Map<String, Set<Id>> getFileOwners() {
return fileOwners;
}
+ public Map<String, Set<Id>> getFileReviewers() {
+ return fileReviewers;
+ }
+
public void addFileOwners(String file, Set<Id> owners) {
if (owners.isEmpty()) {
return;
@@ -74,4 +92,18 @@
fileOwners.put(file, Sets.newHashSet(owners));
}
}
+
+ public void addFileReviewers(String file, Set<Id> reviewers) {
+ if (reviewers.isEmpty()) {
+ return;
+ }
+
+ Set<Id> set = fileReviewers.get(file);
+ if (set != null) {
+ // add new owners removing duplicates
+ set.addAll(reviewers);
+ } else {
+ fileReviewers.put(file, Sets.newHashSet(reviewers));
+ }
+ }
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PartialRegExMatcher.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PartialRegExMatcher.java
index c046c13..18f04f5 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PartialRegExMatcher.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PartialRegExMatcher.java
@@ -16,6 +16,7 @@
package com.googlesource.gerrit.owners.common;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Account.Id;
import java.util.Set;
import java.util.regex.Pattern;
@@ -23,8 +24,8 @@
public class PartialRegExMatcher extends Matcher {
Pattern pattern;
- public PartialRegExMatcher(String path, Set<Account.Id> owners) {
- super(path, owners);
+ public PartialRegExMatcher(String path, Set<Account.Id> owners, Set<Account.Id> reviewers) {
+ super(path, owners, reviewers);
pattern = Pattern.compile(".*" + path + ".*");
}
@@ -32,4 +33,9 @@
public boolean matches(String pathToMatch) {
return pattern.matcher(pathToMatch).matches();
}
+
+ @Override
+ protected Matcher clone(Set<Id> owners, Set<Id> reviewers) {
+ return new PartialRegExMatcher(path, owners, reviewers);
+ }
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
index 77289a3..6bf288b 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -31,6 +31,7 @@
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListEntry;
import java.io.IOException;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
@@ -49,6 +50,8 @@
private final SetMultimap<String, Account.Id> owners;
+ private final SetMultimap<String, Account.Id> reviewers;
+
private final Repository repository;
private final PatchList patchList;
@@ -69,6 +72,7 @@
OwnersMap map = fetchOwners(branch);
owners = Multimaps.unmodifiableSetMultimap(map.getPathOwners());
+ reviewers = Multimaps.unmodifiableSetMultimap(map.getPathReviewers());
matchers = map.getMatchers();
fileOwners = map.getFileOwners();
}
@@ -82,6 +86,15 @@
return owners;
}
+ /**
+ * Returns a read only view of the paths to reviewers mapping.
+ *
+ * @return multimap of paths to reviewers
+ */
+ public SetMultimap<String, Account.Id> getReviewers() {
+ return reviewers;
+ }
+
public Map<String, Matcher> getMatchers() {
return matchers;
}
@@ -102,12 +115,28 @@
PathOwnersEntry projectEntry =
getOwnersConfig(rootPath, RefNames.REFS_CONFIG)
- .map(conf -> new PathOwnersEntry(rootPath, conf, accounts, Collections.emptySet()))
+ .map(
+ conf ->
+ new PathOwnersEntry(
+ rootPath,
+ conf,
+ accounts,
+ Collections.emptySet(),
+ Collections.emptySet(),
+ Collections.emptySet()))
.orElse(new PathOwnersEntry());
PathOwnersEntry rootEntry =
getOwnersConfig(rootPath, branch)
- .map(conf -> new PathOwnersEntry(rootPath, conf, accounts, Collections.emptySet()))
+ .map(
+ conf ->
+ new PathOwnersEntry(
+ rootPath,
+ conf,
+ accounts,
+ Collections.emptySet(),
+ Collections.emptySet(),
+ Collections.emptySet()))
.orElse(new PathOwnersEntry());
Set<String> modifiedPaths = getModifiedPaths();
@@ -116,13 +145,15 @@
for (String path : modifiedPaths) {
currentEntry = resolvePathEntry(path, branch, projectEntry, rootEntry, entries);
- // add owners to file for matcher predicates
+ // add owners and reviewers to file for matcher predicates
ownersMap.addFileOwners(path, currentEntry.getOwners());
+ ownersMap.addFileReviewers(path, currentEntry.getReviewers());
// Only add the path to the OWNERS file to reduce the number of
// entries in the result
if (currentEntry.getOwnersPath() != null) {
ownersMap.addPathOwners(currentEntry.getOwnersPath(), currentEntry.getOwners());
+ ownersMap.addPathReviewers(currentEntry.getOwnersPath(), currentEntry.getReviewers());
}
ownersMap.addMatchers(currentEntry.getMatchers());
}
@@ -157,6 +188,7 @@
if (matcher.matches(path)) {
newMatchers.put(matcher.getPath(), matcher);
ownersMap.addFileOwners(path, matcher.getOwners());
+ ownersMap.addFileReviewers(path, matcher.getReviewers());
}
}
}
@@ -200,14 +232,14 @@
String ownersPath = partial + "OWNERS";
Optional<OwnersConfig> conf = getOwnersConfig(ownersPath, branch);
final Set<Id> owners = currentEntry.getOwners();
+ final Set<Id> reviewers = currentEntry.getReviewers();
+ Collection<Matcher> inheritedMatchers = currentEntry.getMatchers().values();
currentEntry =
- conf.map(c -> new PathOwnersEntry(ownersPath, c, accounts, owners))
+ conf.map(
+ c ->
+ new PathOwnersEntry(
+ ownersPath, c, accounts, owners, reviewers, inheritedMatchers))
.orElse(currentEntry);
- if (conf.map(OwnersConfig::isInherited).orElse(false)) {
- for (Matcher m : currentEntry.getMatchers().values()) {
- currentEntry.addMatcher(m);
- }
- }
entries.put(partial, currentEntry);
}
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
index d2c73ec..6c498d6 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
@@ -31,22 +31,40 @@
*/
class PathOwnersEntry {
private final boolean inherited;
+ private Set<Account.Id> owners = Sets.newHashSet();
+ private Set<Account.Id> reviewers = Sets.newHashSet();
+ private String ownersPath;
+ private Map<String, Matcher> matchers = Maps.newHashMap();
public PathOwnersEntry() {
inherited = true;
}
public PathOwnersEntry(
- String path, OwnersConfig config, Accounts accounts, Set<Account.Id> inheritedOwners) {
+ String path,
+ OwnersConfig config,
+ Accounts accounts,
+ Set<Account.Id> inheritedOwners,
+ Set<Account.Id> inheritedReviewers,
+ Collection<Matcher> inheritedMatchers) {
this.ownersPath = path;
this.owners =
config.getOwners().stream()
.flatMap(o -> accounts.find(o).stream())
.collect(Collectors.toSet());
+ this.reviewers =
+ config.getReviewers().stream()
+ .flatMap(o -> accounts.find(o).stream())
+ .collect(Collectors.toSet());
+ this.matchers = config.getMatchers();
+
if (config.isInherited()) {
this.owners.addAll(inheritedOwners);
+ this.reviewers.addAll(inheritedReviewers);
+ for (Matcher matcher : inheritedMatchers) {
+ addMatcher(matcher);
+ }
}
- this.matchers = config.getMatchers();
this.inherited = config.isInherited();
}
@@ -61,13 +79,9 @@
+ "]";
}
- private String ownersPath;
- private Set<Account.Id> owners = Sets.newHashSet();
-
- private Map<String, Matcher> matchers = Maps.newHashMap();
-
public void addMatcher(Matcher matcher) {
- this.matchers.put(matcher.getPath(), matcher);
+ Matcher currMatchers = this.matchers.get(matcher.getPath());
+ this.matchers.put(matcher.getPath(), matcher.merge(currMatchers));
}
public Map<String, Matcher> getMatchers() {
@@ -82,6 +96,14 @@
this.owners = owners;
}
+ public Set<Account.Id> getReviewers() {
+ return reviewers;
+ }
+
+ public void setReviewers(Set<Account.Id> reviewers) {
+ this.reviewers = reviewers;
+ }
+
public String getOwnersPath() {
return ownersPath;
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/RegExMatcher.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/RegExMatcher.java
index 45fd615..13f3636 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/RegExMatcher.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/RegExMatcher.java
@@ -16,14 +16,15 @@
package com.googlesource.gerrit.owners.common;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Account.Id;
import java.util.Set;
import java.util.regex.Pattern;
public class RegExMatcher extends Matcher {
Pattern pattern;
- public RegExMatcher(String path, Set<Account.Id> owners) {
- super(path, owners);
+ public RegExMatcher(String path, Set<Account.Id> owners, Set<Account.Id> reviewers) {
+ super(path, owners, reviewers);
pattern = Pattern.compile(path);
}
@@ -31,4 +32,9 @@
public boolean matches(String pathToMatch) {
return pattern.matcher(pathToMatch).matches();
}
+
+ @Override
+ protected Matcher clone(Set<Id> owners, Set<Id> reviewers) {
+ return new RegExMatcher(path, owners, reviewers);
+ }
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/SuffixMatcher.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/SuffixMatcher.java
index 39e8b32..6b56cc4 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/SuffixMatcher.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/SuffixMatcher.java
@@ -16,15 +16,21 @@
package com.googlesource.gerrit.owners.common;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Account.Id;
import java.util.Set;
public class SuffixMatcher extends Matcher {
- public SuffixMatcher(String path, Set<Account.Id> owners) {
- super(path, owners);
+ public SuffixMatcher(String path, Set<Account.Id> owners, Set<Account.Id> reviewers) {
+ super(path, owners, reviewers);
}
@Override
public boolean matches(String pathToMatch) {
return pathToMatch.endsWith(path);
}
+
+ @Override
+ protected Matcher clone(Set<Id> owners, Set<Id> reviewers) {
+ return new SuffixMatcher(path, owners, reviewers);
+ }
}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexMatcherTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexMatcherTest.java
index 1fd557b..c61827e 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexMatcherTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexMatcherTest.java
@@ -23,18 +23,18 @@
public class RegexMatcherTest {
@Test
public void testRegex() {
- RegExMatcher matcher = new RegExMatcher(".*/a.*", null);
+ RegExMatcher matcher = new RegExMatcher(".*/a.*", null, null);
assertTrue(matcher.matches("xxxxxx/axxxx"));
assertFalse(matcher.matches("axxxx"));
assertFalse(matcher.matches("xxxxx/bxxxx"));
- RegExMatcher matcher2 = new RegExMatcher("a.*.sql", null);
+ RegExMatcher matcher2 = new RegExMatcher("a.*.sql", null, null);
assertFalse(matcher2.matches("xxxxxx/alfa.sql"));
}
@Test
public void testFloatingRegex() {
- PartialRegExMatcher matcher = new PartialRegExMatcher("a.*.sql", null);
+ PartialRegExMatcher matcher = new PartialRegExMatcher("a.*.sql", null, null);
assertTrue(matcher.matches("xxxxxxx/alfa.sql"));
assertTrue(matcher.matches("alfa.sqlxxxxx"));
assertFalse(matcher.matches("alfa.bar"));
