Merge "Compute only as many owned paths as needed, not all"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 8361d9b..1c5e87f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -122,30 +122,39 @@
* @param changeNotes the change notes for which the owned files should be returned
* @param patchSet the patch set for which the owned files should be returned
* @param accountId account ID of the code owner for which the owned files should be returned
+ * @param limit the max number of owned paths that should be returned (0 = unlimited)
* @return the paths of the files in the given patch set that are owned by the specified account
* @throws ResourceConflictException if the destination branch of the change no longer exists
*/
public ImmutableList<Path> getOwnedPaths(
- ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
+ ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int limit)
throws ResourceConflictException {
try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
logger.atFine().log(
- "compute owned paths for account %d (project = %s, change = %d, patch set = %d)",
+ "compute owned paths for account %d (project = %s, change = %d, patch set = %d,"
+ + " limit = %d)",
accountId.get(),
changeNotes.getProjectName(),
changeNotes.getChangeId().get(),
- patchSet.id().get());
- return getFileStatusesForAccount(changeNotes, patchSet, accountId)
- .flatMap(
- fileCodeOwnerStatus ->
- Stream.of(
- fileCodeOwnerStatus.newPathStatus(), fileCodeOwnerStatus.oldPathStatus())
- .filter(Optional::isPresent)
- .map(Optional::get))
- .filter(pathCodeOwnerStatus -> pathCodeOwnerStatus.status() == CodeOwnerStatus.APPROVED)
- .map(PathCodeOwnerStatus::path)
- .sorted(comparing(Path::toString))
- .collect(toImmutableList());
+ patchSet.id().get(),
+ limit);
+ Stream<Path> ownedPaths =
+ getFileStatusesForAccount(changeNotes, patchSet, accountId)
+ .flatMap(
+ fileCodeOwnerStatus ->
+ Stream.of(
+ fileCodeOwnerStatus.newPathStatus(),
+ fileCodeOwnerStatus.oldPathStatus())
+ .filter(Optional::isPresent)
+ .map(Optional::get))
+ .filter(
+ pathCodeOwnerStatus -> pathCodeOwnerStatus.status() == CodeOwnerStatus.APPROVED)
+ .map(PathCodeOwnerStatus::path)
+ .sorted(comparing(Path::toString));
+ if (limit > 0) {
+ ownedPaths = ownedPaths.limit(limit);
+ }
+ return ownedPaths.collect(toImmutableList());
} catch (IOException | PatchListNotAvailableException e) {
throw new CodeOwnersInternalServerErrorException(
String.format(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 9352e97..e6a55ee 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -96,8 +96,8 @@
CodeOwnersPluginConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(projectName);
- if (codeOwnersConfig.isDisabled(event.getChange().branch)
- || codeOwnersConfig.getMaxPathsInChangeMessages() <= 0) {
+ int maxPathsInChangeMessages = codeOwnersConfig.getMaxPathsInChangeMessages();
+ if (codeOwnersConfig.isDisabled(event.getChange().branch) || maxPathsInChangeMessages <= 0) {
return;
}
@@ -108,7 +108,8 @@
updateFactory -> {
try (BatchUpdate batchUpdate =
updateFactory.create(projectName, userProvider.get(), TimeUtil.nowTs())) {
- batchUpdate.addOp(changeId, new Op(event.getReviewers()));
+ batchUpdate.addOp(
+ changeId, new Op(event.getReviewers(), maxPathsInChangeMessages));
batchUpdate.execute();
}
return null;
@@ -124,9 +125,11 @@
private class Op implements BatchUpdateOp {
private final List<AccountInfo> reviewers;
+ private final int limit;
- Op(List<AccountInfo> reviewers) {
+ Op(List<AccountInfo> reviewers, int limit) {
this.reviewers = reviewers;
+ this.limit = limit;
}
@Override
@@ -158,9 +161,10 @@
ImmutableList<Path> ownedPaths;
try {
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes, changeNotes.getCurrentPatchSet(), reviewerAccountId);
+ changeNotes, changeNotes.getCurrentPatchSet(), reviewerAccountId, limit + 1);
} catch (RestApiException e) {
logger.atFine().withCause(e).log(
"Couldn't compute owned paths of change %s for account %s",
@@ -181,15 +185,11 @@
"%s who was added as reviewer owns the following files:\n",
reviewerAccount.getName()));
- int maxPathsInChangeMessage =
- codeOwnersPluginConfiguration.getProjectConfig(projectName).getMaxPathsInChangeMessages();
- if (ownedPaths.size() <= maxPathsInChangeMessage) {
+ if (ownedPaths.size() <= limit) {
appendPaths(message, ownedPaths.stream());
} else {
- // -1 so that we never show "(1 more files)"
- int limit = maxPathsInChangeMessage - 1;
appendPaths(message, ownedPaths.stream().limit(limit));
- message.append(String.format("(%s more files)\n", ownedPaths.size() - limit));
+ message.append("(more files)\n");
}
return Optional.of(message.toString());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index 101cedd..e9e2c39 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -77,7 +77,9 @@
Map<String, Short> approvals) {
CodeOwnersPluginConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
- if (codeOwnersConfig.isDisabled(changeNotes.getChange().getDest().branch())) {
+ int maxPathsInChangeMessage = codeOwnersConfig.getMaxPathsInChangeMessages();
+ if (codeOwnersConfig.isDisabled(changeNotes.getChange().getDest().branch())
+ || maxPathsInChangeMessage <= 0) {
return Optional.empty();
}
@@ -97,7 +99,13 @@
try (Timer0.Context ctx = codeOwnerMetrics.extendChangeMessageOnPostReview.start()) {
return buildMessageForCodeOwnerApproval(
- user, changeNotes, patchSet, oldApprovals, approvals, requiredApproval);
+ user,
+ changeNotes,
+ patchSet,
+ oldApprovals,
+ approvals,
+ requiredApproval,
+ maxPathsInChangeMessage);
}
}
@@ -107,21 +115,16 @@
PatchSet patchSet,
Map<String, Short> oldApprovals,
Map<String, Short> approvals,
- RequiredApproval requiredApproval) {
- CodeOwnersPluginConfigSnapshot codeOwnersConfig =
- codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
- int maxPathsInChangeMessage = codeOwnersConfig.getMaxPathsInChangeMessages();
- if (maxPathsInChangeMessage <= 0) {
- return Optional.empty();
- }
-
+ RequiredApproval requiredApproval,
+ int limit) {
LabelVote newVote = getNewVote(requiredApproval, approvals);
ImmutableList<Path> ownedPaths;
try {
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes, changeNotes.getCurrentPatchSet(), user.getAccountId());
+ changeNotes, changeNotes.getCurrentPatchSet(), user.getAccountId(), limit + 1);
} catch (RestApiException e) {
logger.atFine().withCause(e).log(
"Couldn't compute owned paths of change %s for account %s",
@@ -147,7 +150,9 @@
}
boolean hasImplicitApprovalByUser =
- codeOwnersConfig.areImplicitApprovalsEnabled()
+ codeOwnersPluginConfiguration
+ .getProjectConfig(changeNotes.getProjectName())
+ .areImplicitApprovalsEnabled()
&& patchSet.uploader().equals(user.getAccountId());
boolean noLongerExplicitlyApproved = false;
@@ -213,13 +218,11 @@
return Optional.empty();
}
- if (ownedPaths.size() <= maxPathsInChangeMessage) {
+ if (ownedPaths.size() <= limit) {
appendPaths(message, ownedPaths.stream());
} else {
- // -1 so that we never show "(1 more files)"
- int limit = maxPathsInChangeMessage - 1;
appendPaths(message, ownedPaths.stream().limit(limit));
- message.append(String.format("(%s more files)\n", ownedPaths.size() - limit));
+ message.append("(more files)\n");
}
if (hasImplicitApprovalByUser && noLongerExplicitlyApproved) {
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
index 305b31a..0430f94 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
@@ -66,7 +66,7 @@
ImmutableList<Path> ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- revisionResource.getNotes(), revisionResource.getPatchSet(), accountId);
+ revisionResource.getNotes(), revisionResource.getPatchSet(), accountId, /* limit= */ 0);
OwnedPathsInfo ownedPathsInfo = new OwnedPathsInfo();
ownedPathsInfo.ownedPaths = ownedPaths.stream().map(Path::toString).collect(toImmutableList());
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
index 576d2fe..91c40db 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
@@ -210,6 +210,7 @@
String path2 = "foo/baz.bar";
String path3 = "bar/foo.baz";
String path4 = "bar/baz.foo";
+ String path5 = "baz/foo.bar";
String changeId =
createChange(
"Test Change",
@@ -221,6 +222,8 @@
path3,
"file content",
path4,
+ "file content",
+ path5,
"file content"))
.getChangeId();
@@ -233,8 +236,9 @@
"%s who was added as reviewer owns the following files:\n"
+ "* %s\n"
+ "* %s\n"
- + "(2 more files)\n",
- user.fullName(), path4, path3));
+ + "* %s\n"
+ + "(more files)\n",
+ user.fullName(), path4, path3, path5));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index 971f8e9..8a8b9c4 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -808,6 +808,7 @@
String path2 = "foo/baz.bar";
String path3 = "bar/foo.baz";
String path4 = "bar/baz.foo";
+ String path5 = "baz/foo.bar";
String changeId =
createChange(
"Test Change",
@@ -819,6 +820,8 @@
path3,
"file content",
path4,
+ "file content",
+ path5,
"file content"))
.getChangeId();
@@ -833,8 +836,9 @@
+ " %s:\n"
+ "* %s\n"
+ "* %s\n"
- + "(2 more files)\n",
- admin.fullName(), path4, path3));
+ + "* %s\n"
+ + "(more files)\n",
+ admin.fullName(), path4, path3, path5));
}
@Test
