Add ListOfFilesUnchangedPredicate approval copy predicate
This predicate is used to copy over approvals when the list of files
hasn't changed. This is the same as the label config
copyAllScoresIfListOfFilesDidNotChange, except that we implement it for
the new copyCondition done in Ib755f3aea.
We use the old diff cache since the new one (DiffOperations) is not
ready to be used yet.
Change-Id: I83d805ae711f32a4b664615eeb6ae649a4f9cc58
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index 4737460..f5346c1 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -296,6 +296,12 @@
Matches votes where the new patch set was uploaded by a member of `groupUUID`.
+==== has:unchanged-files
+
+Matches when the new patch-set includes the same files as the old patch-set.
+
+Only 'unchanged-files' is supported for 'has'.
+
==== Example
----
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java
index 5aa32b8..1ae5ea5 100644
--- a/java/com/google/gerrit/server/approval/ApprovalInference.java
+++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -26,7 +26,6 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelTypes;
-import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.exceptions.StorageException;
@@ -47,6 +46,7 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.approval.ApprovalContext;
import com.google.gerrit.server.query.approval.ApprovalQueryBuilder;
+import com.google.gerrit.server.query.approval.ListOfFilesUnchangedPredicate;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
@@ -75,6 +75,7 @@
private final PatchListCache patchListCache;
private final ApprovalQueryBuilder approvalQueryBuilder;
private final OneOffRequestContext requestContext;
+ private final ListOfFilesUnchangedPredicate listOfFilesUnchangedPredicate;
@Inject
ApprovalInference(
@@ -83,13 +84,15 @@
LabelNormalizer labelNormalizer,
PatchListCache patchListCache,
ApprovalQueryBuilder approvalQueryBuilder,
- OneOffRequestContext requestContext) {
+ OneOffRequestContext requestContext,
+ ListOfFilesUnchangedPredicate listOfFilesUnchangedPredicate) {
this.projectCache = projectCache;
this.changeKindCache = changeKindCache;
this.labelNormalizer = labelNormalizer;
this.patchListCache = patchListCache;
this.approvalQueryBuilder = approvalQueryBuilder;
this.requestContext = requestContext;
+ this.listOfFilesUnchangedPredicate = listOfFilesUnchangedPredicate;
}
/**
@@ -116,7 +119,7 @@
}
}
- private static boolean canCopyBasedOnBooleanLabelConfigs(
+ private boolean canCopyBasedOnBooleanLabelConfigs(
ProjectState project,
PatchSetApproval psa,
PatchSet.Id psId,
@@ -183,12 +186,7 @@
project.getName());
return true;
} else if (type.isCopyAllScoresIfListOfFilesDidNotChange()
- && patchList.getPatches().stream()
- .noneMatch(
- p ->
- p.getChangeType() == ChangeType.ADDED
- || p.getChangeType() == ChangeType.DELETED
- || p.getChangeType() == ChangeType.RENAMED)) {
+ && listOfFilesUnchangedPredicate.match(patchList)) {
logger.atFine().log(
"approval %d on label %s of patch set %d of change %d can be copied"
+ " to patch set %d because the label has set "
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java b/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java
index cd79cb2..c3594f5 100644
--- a/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java
@@ -33,18 +33,21 @@
private final UserInPredicate.Factory userInPredicate;
private final GroupResolver groupResolver;
private final GroupControl.Factory groupControl;
+ private final ListOfFilesUnchangedPredicate listOfFilesUnchangedPredicate;
@Inject
protected ApprovalQueryBuilder(
MagicValuePredicate.Factory magicValuePredicate,
UserInPredicate.Factory userInPredicate,
GroupResolver groupResolver,
- GroupControl.Factory groupControl) {
+ GroupControl.Factory groupControl,
+ ListOfFilesUnchangedPredicate listOfFilesUnchangedPredicate) {
super(mydef, null);
this.magicValuePredicate = magicValuePredicate;
this.userInPredicate = userInPredicate;
this.groupResolver = groupResolver;
this.groupControl = groupControl;
+ this.listOfFilesUnchangedPredicate = listOfFilesUnchangedPredicate;
}
@Operator
@@ -67,6 +70,17 @@
return userInPredicate.create(UserInPredicate.Field.UPLOADER, parseGroupOrThrow(group));
}
+ @Operator
+ public Predicate<ApprovalContext> has(String value) throws QueryParseException {
+ if (value.equals("unchanged-files")) {
+ return listOfFilesUnchangedPredicate;
+ }
+ throw error(
+ String.format(
+ "'%s' is not a supported argument for has. only 'unchanged-files' is supported",
+ value));
+ }
+
private static <T extends Enum<T>> T toEnumValue(Class<T> clazz, String term)
throws QueryParseException {
try {
diff --git a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
new file mode 100644
index 0000000..30097d8
--- /dev/null
+++ b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
@@ -0,0 +1,91 @@
+// 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.google.gerrit.server.query.approval;
+
+import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.client.DiffPreferencesInfo;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.server.patch.PatchList;
+import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.patch.PatchListKey;
+import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Objects;
+
+/** Predicate that matches when the new patch-set includes the same files as the old patch-set. */
+@Singleton
+public class ListOfFilesUnchangedPredicate extends ApprovalPredicate {
+ private final PatchListCache patchListCache;
+
+ @Inject
+ public ListOfFilesUnchangedPredicate(PatchListCache patchListCache) {
+ this.patchListCache = patchListCache;
+ }
+
+ @Override
+ public boolean match(ApprovalContext ctx) {
+ PatchSet currentPatchset = ctx.changeNotes().getCurrentPatchSet();
+ Map.Entry<PatchSet.Id, PatchSet> priorPatchSet =
+ ctx.changeNotes().getPatchSets().lowerEntry(currentPatchset.id());
+ PatchListKey key =
+ PatchListKey.againstCommit(
+ priorPatchSet.getValue().commitId(),
+ currentPatchset.commitId(),
+ DiffPreferencesInfo.Whitespace.IGNORE_NONE);
+ try {
+ return match(patchListCache.get(key, ctx.changeNotes().getProjectName()));
+ } catch (PatchListNotAvailableException ex) {
+ throw new StorageException(
+ "failed to compute difference in files, so won't copy"
+ + " votes on labels even if list of files is the same and "
+ + "copyAllIfListOfFilesDidNotChange",
+ ex);
+ }
+ }
+
+ public boolean match(PatchList patchList) {
+ return patchList.getPatches().stream()
+ .noneMatch(
+ p ->
+ p.getChangeType() == ChangeType.ADDED
+ || p.getChangeType() == ChangeType.DELETED
+ || p.getChangeType() == ChangeType.RENAMED);
+ }
+
+ @Override
+ public Predicate<ApprovalContext> copy(
+ Collection<? extends Predicate<ApprovalContext>> children) {
+ return new ListOfFilesUnchangedPredicate(patchListCache);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(patchListCache);
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (!(other instanceof ListOfFilesUnchangedPredicate)) {
+ return false;
+ }
+ ListOfFilesUnchangedPredicate o = (ListOfFilesUnchangedPredicate) other;
+ return Objects.equals(o.patchListCache, patchListCache);
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
index 3bb9b35..9392219 100644
--- a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.testsuite.change.ChangeKindCreator;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelId;
@@ -42,6 +43,7 @@
@Inject private ChangeKindCreator changeKindCreator;
@Inject private ChangeNotes.Factory changeNotesFactory;
@Inject private ChangeKindCache changeKindCache;
+ @Inject private ChangeOperations changeOperations;
@Test
public void magicValuePredicate() throws Exception {
@@ -66,60 +68,66 @@
public void changeKindPredicate_noCodeChange() throws Exception {
String change = changeKindCreator.createChange(ChangeKind.NO_CODE_CHANGE, testRepo, admin);
changeKindCreator.updateChange(change, ChangeKind.NO_CODE_CHANGE, testRepo, admin, project);
- PatchSet.Id ps1 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 1 /* psId */);
+ PatchSet.Id ps1 =
+ PatchSet.id(Change.id(gApi.changes().id(change).get()._number), /* psId= */ 1);
assertTrue(
queryBuilder
.parse("changekind:no-code-change")
.asMatchable()
- .match(contextForCodeReviewLabel(-2 /* value */, ps1, admin.id())));
+ .match(contextForCodeReviewLabel(/* value= */ -2, ps1, admin.id())));
changeKindCreator.updateChange(change, ChangeKind.TRIVIAL_REBASE, testRepo, admin, project);
- PatchSet.Id ps2 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 2 /* psId */);
+ PatchSet.Id ps2 =
+ PatchSet.id(Change.id(gApi.changes().id(change).get()._number), /* psId= */ 2);
assertFalse(
queryBuilder
.parse("changekind:no-code-change")
.asMatchable()
- .match(contextForCodeReviewLabel(-2 /* value */, ps2, admin.id())));
+ .match(contextForCodeReviewLabel(/* value= */ -2, ps2, admin.id())));
}
@Test
public void changeKindPredicate_trivialRebase() throws Exception {
String change = changeKindCreator.createChange(ChangeKind.TRIVIAL_REBASE, testRepo, admin);
changeKindCreator.updateChange(change, ChangeKind.TRIVIAL_REBASE, testRepo, admin, project);
- PatchSet.Id ps1 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 1 /* psId */);
+ PatchSet.Id ps1 =
+ PatchSet.id(Change.id(gApi.changes().id(change).get()._number), /* psId= */ 1);
assertTrue(
queryBuilder
.parse("changekind:trivial-rebase")
.asMatchable()
- .match(contextForCodeReviewLabel(-2 /* value */, ps1, admin.id())));
+ .match(contextForCodeReviewLabel(/* value= */ -2, ps1, admin.id())));
changeKindCreator.updateChange(change, ChangeKind.REWORK, testRepo, admin, project);
- PatchSet.Id ps2 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 2 /* psId */);
+ PatchSet.Id ps2 =
+ PatchSet.id(Change.id(gApi.changes().id(change).get()._number), /* psId= */ 2);
assertFalse(
queryBuilder
.parse("changekind:trivial-rebase")
.asMatchable()
- .match(contextForCodeReviewLabel(-2 /* value */, ps2, admin.id())));
+ .match(contextForCodeReviewLabel(/* value= */ -2, ps2, admin.id())));
}
@Test
public void changeKindPredicate_reworkAndNotRework() throws Exception {
String change = changeKindCreator.createChange(ChangeKind.REWORK, testRepo, admin);
changeKindCreator.updateChange(change, ChangeKind.REWORK, testRepo, admin, project);
- PatchSet.Id ps1 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 1 /* psId */);
+ PatchSet.Id ps1 =
+ PatchSet.id(Change.id(gApi.changes().id(change).get()._number), /* psId= */ 1);
assertTrue(
queryBuilder
.parse("changekind:rework")
.asMatchable()
- .match(contextForCodeReviewLabel(-2 /* value */, ps1, admin.id())));
+ .match(contextForCodeReviewLabel(/* value= */ -2, ps1, admin.id())));
changeKindCreator.updateChange(change, ChangeKind.REWORK, testRepo, admin, project);
- PatchSet.Id ps2 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 2 /* psId */);
+ PatchSet.Id ps2 =
+ PatchSet.id(Change.id(gApi.changes().id(change).get()._number), /* psId= */ 2);
assertFalse(
queryBuilder
.parse("-changekind:rework")
.asMatchable()
- .match(contextForCodeReviewLabel(-2 /* value */, ps2, admin.id())));
+ .match(contextForCodeReviewLabel(/* value= */ -2, ps2, admin.id())));
}
@Test
@@ -141,8 +149,8 @@
.asMatchable()
.match(
contextForCodeReviewLabel(
- 2 /* value */,
- PatchSet.id(pushResult.getChange().getId(), 1 /* psId */),
+ /* value= */ 2,
+ PatchSet.id(pushResult.getChange().getId(), /* psId= */ 1),
admin.id())));
// can not copy approval from patchset 2 -> 3
assertFalse(
@@ -151,8 +159,8 @@
.asMatchable()
.match(
contextForCodeReviewLabel(
- 2 /* value */,
- PatchSet.id(pushResult.getChange().getId(), 2 /* psId */),
+ /* value= */ 2,
+ PatchSet.id(pushResult.getChange().getId(), /* psId= */ 2),
admin.id())));
}
@@ -170,8 +178,8 @@
.asMatchable()
.match(
contextForCodeReviewLabel(
- 2 /* value */,
- PatchSet.id(pushResult.getChange().getId(), 1 /* psId */),
+ /* value= */ 2,
+ PatchSet.id(pushResult.getChange().getId(), /* psId= */ 1),
admin.id())));
// can not copy approval from patchset 2 -> 3
assertFalse(
@@ -180,8 +188,8 @@
.asMatchable()
.match(
contextForCodeReviewLabel(
- 2 /* value */,
- PatchSet.id(pushResult.getChange().getId(), 2 /* psId */),
+ /* value= */ 2,
+ PatchSet.id(pushResult.getChange().getId(), /* psId= */ 2),
user.id())));
}
@@ -194,10 +202,52 @@
queryBuilder
.parse("uploaderin:foobar")
.asMatchable()
- .match(contextForCodeReviewLabel(2)));
+ .match(contextForCodeReviewLabel(/* value= */ 2)));
assertThat(thrown).hasMessageThat().contains("Group foobar not found");
}
+ @Test
+ public void hasChangedFilesPredicate() throws Exception {
+ Change.Id changeId =
+ changeOperations.newChange().project(project).file("file").content("content").create();
+ changeOperations.change(changeId).newPatchset().file("file").content("new content").create();
+
+ // can copy approval from patch-set 1 -> 2
+ assertTrue(
+ queryBuilder
+ .parse("has:unchanged-files")
+ .asMatchable()
+ .match(
+ contextForCodeReviewLabel(
+ /* value= */ 2, PatchSet.id(changeId, /* psId= */ 1), admin.id())));
+ changeOperations.change(changeId).newPatchset().file("file").delete().create();
+
+ // can not copy approval from patch-set 2 -> 3
+ assertFalse(
+ queryBuilder
+ .parse("has:unchanged-files")
+ .asMatchable()
+ .match(
+ contextForCodeReviewLabel(
+ /* value= */ 2, PatchSet.id(changeId, /* psId= */ 2), admin.id())));
+ }
+
+ @Test
+ public void hasChangedFilesPredicate_unsupportedOperator() {
+ QueryParseException thrown =
+ assertThrows(
+ QueryParseException.class,
+ () ->
+ queryBuilder
+ .parse("has:invalid")
+ .asMatchable()
+ .match(contextForCodeReviewLabel(/* value= */ 2)));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(
+ "'invalid' is not a supported argument for has. only 'unchanged-files' is supported");
+ }
+
private ApprovalContext contextForCodeReviewLabel(int value) throws Exception {
PushOneCommit.Result result = createChange();
amendChange(result.getChangeId());
