Merge changes Ibc413d7d,Idd5168ff into stable-2.15
* changes:
Remove handling of change.allowDrafts
Remove documentation on change.allowDrafts
diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt
index 3f6acf6..59ed6ff 100644
--- a/Documentation/cmd-review.txt
+++ b/Documentation/cmd-review.txt
@@ -120,15 +120,10 @@
Votes that are not permitted for the user are silently ignored.
--label::
- Set a label by name to the value 'N'. Invalid votes (invalid label
- or invalid value) and votes that are not permitted for the user are
- silently ignored.
-
---strict-labels::
- Require ability to vote on all specified labels before reviewing change.
- If the vote is invalid (invalid label or invalid name), the vote is not
- permitted for the user, or the vote is on an outdated or closed patch set,
- return an error instead of silently discarding the vote.
+ Set a label by name to the value 'N'. The ability to vote on all specified
+ labels is required. If the vote is invalid (invalid label or invalid name),
+ the vote is not permitted for the user, or the vote is on an outdated or
+ closed patch set, return an error instead of silently discarding the vote.
--tag::
-t::
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index ea577f3..618ad3f 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6695,13 +6695,6 @@
|`robot_comments` |optional|
The robot comments that should be added as a map that maps a file path
to a list of link:#robot-comment-input[RobotCommentInput] entities.
-|`strict_labels` |`true` if not set|
-Whether all labels are required to be within the user's permitted ranges
-based on access controls. +
-If `true`, attempting to use a label not granted to the user will fail
-the entire modify operation early. +
-If `false`, the operation will execute anyway, but the proposed labels
-will be modified to be the "best" value allowed by the access controls.
|`drafts` |optional|
Draft handling that defines how draft comments are handled that are
already in the database but that were not also described in this
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index abbecd0..a801669 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -21,6 +21,8 @@
import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.project.Util.category;
+import static com.google.gerrit.server.project.Util.value;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.HEAD;
@@ -37,6 +39,9 @@
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.LabelFunction;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.api.GerritApi;
@@ -1398,4 +1403,19 @@
config.getProject().setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE);
saveProjectConfig(project, config);
}
+
+ protected void configLabel(String label, LabelFunction func) throws Exception {
+ configLabel(
+ project, label, func, value(1, "Passes"), value(0, "No score"), value(-1, "Failed"));
+ }
+
+ protected void configLabel(
+ Project.NameKey project, String label, LabelFunction func, LabelValue... value)
+ throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ LabelType labelType = category(label, value);
+ labelType.setFunction(func);
+ cfg.getLabelSections().put(labelType.getName(), labelType);
+ saveProjectConfig(project, cfg);
+ }
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 780c2d7..d3389c5 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -70,6 +70,7 @@
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
@@ -133,6 +134,7 @@
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.server.update.BatchUpdate;
@@ -1582,7 +1584,6 @@
// Exact request format made by GWT UI at ddc6b7160fe416fed9e7e3180489d44c82fd64f8.
ReviewInput in = new ReviewInput();
in.labels = ImmutableMap.of("Code-Review", (short) 0);
- in.strictLabels = true;
in.drafts = DraftHandling.PUBLISH_ALL_REVISIONS;
in.message = "comment";
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in);
@@ -3189,6 +3190,68 @@
gApi.changes().id(changeId).topic(topic);
}
+ @Test
+ public void submittableAfterLosingPermissions_MaxWithBlock() throws Exception {
+ configLabel("Label", LabelFunction.MAX_WITH_BLOCK);
+ submittableAfterLosingPermissions("Label");
+ }
+
+ @Test
+ public void submittableAfterLosingPermissions_AnyWithBlock() throws Exception {
+ configLabel("Label", LabelFunction.ANY_WITH_BLOCK);
+ submittableAfterLosingPermissions("Label");
+ }
+
+ public void submittableAfterLosingPermissions(String label) throws Exception {
+ String codeReviewLabel = "Code-Review";
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ AccountGroup.UUID registered = SystemGroupBackend.REGISTERED_USERS;
+ Util.allow(cfg, Permission.forLabel(label), -1, +1, registered, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(codeReviewLabel), -2, +2, registered, "refs/heads/*");
+ saveProjectConfig(cfg);
+
+ setApiUser(user);
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Verify user's permitted range.
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertPermitted(change, label, -1, 0, 1);
+ assertPermitted(change, codeReviewLabel, -2, -1, 0, 1, 2);
+
+ ReviewInput input = new ReviewInput();
+ input.label(codeReviewLabel, 2);
+ input.label(label, 1);
+ gApi.changes().id(changeId).current().review(input);
+
+ assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().keySet())
+ .containsExactly(codeReviewLabel, label);
+ assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().values())
+ .containsExactly((short) 2, (short) 1);
+ assertThat(gApi.changes().id(changeId).get().submittable).isTrue();
+
+ setApiUser(admin);
+ // Remove user's permission for 'Label'.
+ Util.remove(cfg, Permission.forLabel(label), registered, "refs/heads/*");
+ // Update user's permitted range for 'Code-Review' to be -1...+1.
+ Util.remove(cfg, Permission.forLabel(codeReviewLabel), registered, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(codeReviewLabel), -1, +1, registered, "refs/heads/*");
+ saveProjectConfig(cfg);
+
+ // Verify user's new permitted range.
+ setApiUser(user);
+ change = gApi.changes().id(changeId).get();
+ assertPermitted(change, label);
+ assertPermitted(change, codeReviewLabel, -1, 0, 1);
+
+ assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().values())
+ .containsExactly((short) 2, (short) 1);
+ assertThat(gApi.changes().id(changeId).get().submittable).isTrue();
+
+ setApiUser(admin);
+ gApi.changes().id(changeId).current().submit();
+ }
+
private String getCommitMessage(String changeId) throws RestApiException, IOException {
return gApi.changes().id(changeId).current().file("/COMMIT_MSG").content().asString();
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index 36843a5..9b88e0d 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -146,7 +146,6 @@
ReviewInput in = new ReviewInput();
in.onBehalfOf = user.id.toString();
- in.strictLabels = true;
in.label("Not-A-Label", 5);
exception.expect(BadRequestException.class);
@@ -155,23 +154,6 @@
}
@Test
- public void voteOnBehalfOfInvalidLabelIgnoredWithoutStrictLabels() throws Exception {
- allowCodeReviewOnBehalfOf();
- PushOneCommit.Result r = createChange();
- RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
-
- ReviewInput in = new ReviewInput();
- in.onBehalfOf = user.id.toString();
- in.strictLabels = false;
- in.label("Code-Review", 1);
- in.label("Not-A-Label", 5);
-
- revision.review(in);
-
- assertThat(gApi.changes().id(r.getChangeId()).get().labels).doesNotContainKey("Not-A-Label");
- }
-
- @Test
public void voteOnBehalfOfLabelNotPermitted() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
LabelType verified = Util.verified();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
index 8388ed0..37d3e1e 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.MoveInput;
@@ -33,6 +34,7 @@
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.Util;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.PersonIdent;
@@ -227,6 +229,59 @@
move(r.getChangeId(), newBranch.get());
}
+ @Test
+ public void moveChangeOnlyKeepVetoVotes() throws Exception {
+ // A vote for a label will be kept after moving if the label's function is *WithBlock and the
+ // vote holds the minimum value.
+ createBranch(new Branch.NameKey(project, "foo"));
+
+ String codeReviewLabel = "Code-Review"; // 'Code-Review' uses 'MaxWithBlock' function.
+ String testLabelA = "Label-A";
+ String testLabelB = "Label-B";
+ String testLabelC = "Label-C";
+ configLabel(testLabelA, LabelFunction.ANY_WITH_BLOCK);
+ configLabel(testLabelB, LabelFunction.MAX_NO_BLOCK);
+ configLabel(testLabelC, LabelFunction.NO_BLOCK);
+
+ AccountGroup.UUID registered = SystemGroupBackend.REGISTERED_USERS;
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ Util.allow(cfg, Permission.forLabel(testLabelA), -1, +1, registered, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(testLabelB), -1, +1, registered, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(testLabelC), -1, +1, registered, "refs/heads/*");
+ saveProjectConfig(cfg);
+
+ String changeId = createChange().getChangeId();
+ gApi.changes().id(changeId).current().review(ReviewInput.reject());
+
+ amendChange(changeId);
+
+ ReviewInput input = new ReviewInput();
+ input.label(testLabelA, -1);
+ input.label(testLabelB, -1);
+ input.label(testLabelC, -1);
+ gApi.changes().id(changeId).current().review(input);
+
+ assertThat(gApi.changes().id(changeId).current().reviewer(admin.email).votes().keySet())
+ .containsExactly(codeReviewLabel, testLabelA, testLabelB, testLabelC);
+ assertThat(gApi.changes().id(changeId).current().reviewer(admin.email).votes().values())
+ .containsExactly((short) -2, (short) -1, (short) -1, (short) -1);
+
+ // Move the change to the 'foo' branch.
+ assertThat(gApi.changes().id(changeId).get().branch).isEqualTo("master");
+ move(changeId, "foo");
+ assertThat(gApi.changes().id(changeId).get().branch).isEqualTo("foo");
+
+ // 'Code-Review -2' and 'Label-A -1' will be kept.
+ assertThat(gApi.changes().id(changeId).current().reviewer(admin.email).votes().values())
+ .containsExactly((short) -2, (short) -1, (short) 0, (short) 0);
+
+ // Move the change back to 'master'.
+ move(changeId, "master");
+ assertThat(gApi.changes().id(changeId).get().branch).isEqualTo("master");
+ assertThat(gApi.changes().id(changeId).current().reviewer(admin.email).votes().values())
+ .containsExactly((short) -2, (short) -1, (short) 0, (short) 0);
+ }
+
private void move(int changeNum, String destination) throws RestApiException {
gApi.changes().id(changeNum).move(destination);
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
index 38ff3c7..b02ae7b 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
@@ -15,6 +15,10 @@
package com.google.gerrit.acceptance.server.project;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.common.data.LabelFunction.ANY_WITH_BLOCK;
+import static com.google.gerrit.common.data.LabelFunction.MAX_NO_BLOCK;
+import static com.google.gerrit.common.data.LabelFunction.NO_BLOCK;
+import static com.google.gerrit.common.data.LabelFunction.NO_OP;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.project.Util.category;
import static com.google.gerrit.server.project.Util.value;
@@ -80,7 +84,7 @@
@Test
public void customLabelNoOp_NegativeVoteNotBlock() throws Exception {
- label.setFunctionName("NoOp");
+ label.setFunction(NO_OP);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@@ -93,7 +97,7 @@
@Test
public void customLabelNoBlock_NegativeVoteNotBlock() throws Exception {
- label.setFunctionName("NoBlock");
+ label.setFunction(NO_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@@ -106,7 +110,7 @@
@Test
public void customLabelMaxNoBlock_NegativeVoteNotBlock() throws Exception {
- label.setFunctionName("MaxNoBlock");
+ label.setFunction(MAX_NO_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@@ -119,7 +123,7 @@
@Test
public void customLabelAnyWithBlock_NegativeVoteBlock() throws Exception {
- label.setFunctionName("AnyWithBlock");
+ label.setFunction(ANY_WITH_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@@ -133,7 +137,7 @@
@Test
public void customLabelAnyWithBlock_Addreviewer_ZeroVote() throws Exception {
- P.setFunctionName("AnyWithBlock");
+ P.setFunction(ANY_WITH_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
AddReviewerInput in = new AddReviewerInput();
@@ -168,9 +172,9 @@
@Test
public void customLabel_DisallowPostSubmit() throws Exception {
- label.setFunctionName("NoOp");
+ label.setFunction(NO_OP);
label.setAllowPostSubmit(false);
- P.setFunctionName("NoOp");
+ P.setFunction(NO_OP);
saveLabelConfig();
PushOneCommit.Result r = createChange();
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelFunction.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelFunction.java
new file mode 100644
index 0000000..0ce2c29
--- /dev/null
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelFunction.java
@@ -0,0 +1,71 @@
+// 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.google.gerrit.common.data;
+
+import com.google.gerrit.common.Nullable;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * Functions for determining submittability based on label votes.
+ *
+ * <p>Only describes built-in label functions. Admins can extend the logic arbitrarily using Prolog
+ * rules, in which case the choice of function in the project config is ignored.
+ *
+ * <p>Function semantics are documented in {@code config-labels.txt}, and actual behavior is
+ * implemented in Prolog in {@code gerrit_common.pl}.
+ */
+public enum LabelFunction {
+ MAX_WITH_BLOCK("MaxWithBlock", true),
+ ANY_WITH_BLOCK("AnyWithBlock", true),
+ MAX_NO_BLOCK("MaxNoBlock", false),
+ NO_BLOCK("NoBlock", false),
+ NO_OP("NoOp", false),
+ PATCH_SET_LOCK("PatchSetLock", false);
+
+ public static final Map<String, LabelFunction> ALL;
+
+ static {
+ Map<String, LabelFunction> all = new LinkedHashMap<>();
+ for (LabelFunction f : values()) {
+ all.put(f.getFunctionName(), f);
+ }
+ ALL = Collections.unmodifiableMap(all);
+ }
+
+ public static Optional<LabelFunction> parse(@Nullable String str) {
+ return Optional.ofNullable(ALL.get(str));
+ }
+
+ private final String name;
+ private final boolean isBlock;
+
+ private LabelFunction(String name, boolean isBlock) {
+ this.name = name;
+ this.isBlock = isBlock;
+ }
+
+ /** The function name as defined in documentation and {@code project.config}. */
+ public String getFunctionName() {
+ return name;
+ }
+
+ /** Whether the label is a "block" label, meaning a minimum vote will prevent submission. */
+ public boolean isBlock() {
+ return isBlock;
+ }
+}
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
index c90e1fd..7bfd22e 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
@@ -14,6 +14,7 @@
package com.google.gerrit.common.data;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.LabelId;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import java.util.ArrayList;
@@ -22,6 +23,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
public class LabelType {
public static final boolean DEF_ALLOW_POST_SUBMIT = true;
@@ -97,7 +99,9 @@
protected String name;
+ // String rather than LabelFunction for backwards compatibility with GWT JSON interface.
protected String functionName;
+
protected boolean copyMinScore;
protected boolean copyMaxScore;
protected boolean copyAllScoresOnMergeFirstParentUpdate;
@@ -124,7 +128,7 @@
values = sortValues(valueList);
defaultValue = 0;
- functionName = "MaxWithBlock";
+ functionName = LabelFunction.MAX_WITH_BLOCK.getFunctionName();
maxNegative = Short.MIN_VALUE;
maxPositive = Short.MAX_VALUE;
@@ -154,12 +158,19 @@
return psa.getLabelId().get().equalsIgnoreCase(name);
}
- public String getFunctionName() {
- return functionName;
+ public LabelFunction getFunction() {
+ if (functionName == null) {
+ return null;
+ }
+ Optional<LabelFunction> f = LabelFunction.parse(functionName);
+ if (!f.isPresent()) {
+ throw new IllegalStateException("Unsupported functionName: " + functionName);
+ }
+ return f.get();
}
- public void setFunctionName(String functionName) {
- this.functionName = functionName;
+ public void setFunction(@Nullable LabelFunction function) {
+ this.functionName = function != null ? function.getFunctionName() : null;
}
public boolean canOverride() {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
index 113651b..f851d5e 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
@@ -60,7 +60,6 @@
private native void init() /*-{
this.labels = {};
- this.strict_labels = true;
}-*/;
public final native void prePost() /*-{
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java
index 60fd60f..c7309f8 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java
@@ -14,6 +14,8 @@
package com.google.gerrit.pgm.init;
+import static com.google.gerrit.common.data.LabelFunction.MAX_WITH_BLOCK;
+
import com.google.gerrit.pgm.init.api.AllProjectsConfig;
import com.google.gerrit.pgm.init.api.ConsoleUI;
import com.google.gerrit.pgm.init.api.InitStep;
@@ -54,7 +56,7 @@
public void postRun() throws Exception {
Config cfg = allProjectsConfig.load().getConfig();
if (installVerified) {
- cfg.setString(KEY_LABEL, LABEL_VERIFIED, KEY_FUNCTION, "MaxWithBlock");
+ cfg.setString(KEY_LABEL, LABEL_VERIFIED, KEY_FUNCTION, MAX_WITH_BLOCK.getFunctionName());
cfg.setStringList(
KEY_LABEL,
LABEL_VERIFIED,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
index 6971b48..c1f89e2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
@@ -31,7 +31,6 @@
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
@@ -213,7 +212,7 @@
}
}
return labelNormalizer.normalize(notes, user, byUser.values()).getNormalized();
- } catch (IOException | PermissionBackendException e) {
+ } catch (IOException e) {
throw new OrmException(e);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
index 2f3855c..27d4eb1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
@@ -21,6 +21,7 @@
import com.google.common.base.Strings;
import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.extensions.api.changes.MoveInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -31,18 +32,24 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.LabelId;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -55,7 +62,8 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import java.util.ArrayList;
+import java.util.List;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -71,6 +79,9 @@
private final Provider<InternalChangeQuery> queryProvider;
private final ChangeMessagesUtil cmUtil;
private final PatchSetUtil psUtil;
+ private final ApprovalsUtil approvalsUtil;
+ private final ProjectCache projectCache;
+ private final Provider<CurrentUser> userProvider;
@Inject
Move(
@@ -81,7 +92,10 @@
Provider<InternalChangeQuery> queryProvider,
ChangeMessagesUtil cmUtil,
RetryHelper retryHelper,
- PatchSetUtil psUtil) {
+ PatchSetUtil psUtil,
+ ApprovalsUtil approvalsUtil,
+ ProjectCache projectCache,
+ Provider<CurrentUser> userProvider) {
super(retryHelper);
this.permissionBackend = permissionBackend;
this.dbProvider = dbProvider;
@@ -90,6 +104,9 @@
this.queryProvider = queryProvider;
this.cmUtil = cmUtil;
this.psUtil = psUtil;
+ this.approvalsUtil = approvalsUtil;
+ this.projectCache = projectCache;
+ this.userProvider = userProvider;
}
@Override
@@ -138,7 +155,7 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws OrmException, ResourceConflictException, RepositoryNotFoundException, IOException {
+ throws OrmException, ResourceConflictException, IOException {
change = ctx.getChange();
if (change.getStatus() != Status.NEW) {
throw new ResourceConflictException("Change is " + ChangeUtil.status(change));
@@ -188,10 +205,13 @@
throw new ResourceConflictException("Patch set is not current");
}
- ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
+ PatchSet.Id psId = change.currentPatchSetId();
+ ChangeUpdate update = ctx.getUpdate(psId);
update.setBranch(newDestKey.get());
change.setDest(newDestKey);
+ updateApprovals(ctx, update, psId, projectKey);
+
StringBuilder msgBuf = new StringBuilder();
msgBuf.append("Change destination moved from ");
msgBuf.append(changePrevDest.getShortName());
@@ -207,6 +227,46 @@
return true;
}
+
+ /**
+ * We have a long discussion about how to deal with its votes after moving a change from one
+ * branch to another. In the end, we think only keeping the veto votes is the best way since
+ * it's simple for us and less confusing for our users. See the discussion in the following
+ * proposal: https://gerrit-review.googlesource.com/c/gerrit/+/129171
+ */
+ private void updateApprovals(
+ ChangeContext ctx, ChangeUpdate update, PatchSet.Id psId, Project.NameKey project)
+ throws IOException, OrmException {
+ List<PatchSetApproval> approvals = new ArrayList<>();
+ for (PatchSetApproval psa :
+ approvalsUtil.byPatchSet(
+ ctx.getDb(),
+ ctx.getNotes(),
+ userProvider.get(),
+ psId,
+ ctx.getRevWalk(),
+ ctx.getRepoView().getConfig())) {
+ ProjectState projectState = projectCache.checkedGet(project);
+ LabelType type =
+ projectState.getLabelTypes(ctx.getNotes(), ctx.getUser()).byLabel(psa.getLabelId());
+ // Only keep veto votes, defined as votes where:
+ // 1- the label function allows minimum values to block submission.
+ // 2- the vote holds the minimum value.
+ if (type.isMaxNegative(psa) && type.getFunction().isBlock()) {
+ continue;
+ }
+
+ // Remove votes from NoteDb.
+ update.removeApprovalFor(psa.getAccountId(), psa.getLabel());
+ approvals.add(
+ new PatchSetApproval(
+ new PatchSetApproval.Key(psId, psa.getAccountId(), new LabelId(psa.getLabel())),
+ (short) 0,
+ ctx.getWhen()));
+ }
+ // Remove votes from ReviewDb.
+ ctx.getDb().patchSetApprovals().upsert(approvals);
+ }
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 58634a5..0022656 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -233,7 +233,7 @@
input.drafts = DraftHandling.DELETE;
}
if (input.labels != null) {
- checkLabels(revision, labelTypes, input.strictLabels, input.labels);
+ checkLabels(revision, labelTypes, input.labels);
}
if (input.comments != null) {
cleanUpComments(input.comments);
@@ -443,12 +443,9 @@
while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next();
LabelType type = labelTypes.byLabel(ent.getKey());
- if (type == null && in.strictLabels) {
+ if (type == null) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
- } else if (type == null) {
- itr.remove();
- continue;
}
if (!caller.isInternalUser()) {
@@ -479,8 +476,7 @@
changeResourceFactory.create(rev.getNotes(), reviewer), rev.getPatchSet());
}
- private void checkLabels(
- RevisionResource rsrc, LabelTypes labelTypes, boolean strict, Map<String, Short> labels)
+ private void checkLabels(RevisionResource rsrc, LabelTypes labelTypes, Map<String, Short> labels)
throws BadRequestException, AuthException, PermissionBackendException {
PermissionBackend.ForChange perm = rsrc.permissions();
Iterator<Map.Entry<String, Short>> itr = labels.entrySet().iterator();
@@ -488,12 +484,8 @@
Map.Entry<String, Short> ent = itr.next();
LabelType lt = labelTypes.byLabel(ent.getKey());
if (lt == null) {
- if (strict) {
- throw new BadRequestException(
- String.format("label \"%s\" is not a configured label", ent.getKey()));
- }
- itr.remove();
- continue;
+ throw new BadRequestException(
+ String.format("label \"%s\" is not a configured label", ent.getKey()));
}
if (ent.getValue() == null || ent.getValue() == 0) {
@@ -503,23 +495,16 @@
}
if (lt.getValue(ent.getValue()) == null) {
- if (strict) {
- throw new BadRequestException(
- String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue()));
- }
- itr.remove();
- continue;
+ throw new BadRequestException(
+ String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue()));
}
short val = ent.getValue();
try {
perm.check(new LabelPermission.WithValue(lt, val));
} catch (AuthException e) {
- if (strict) {
- throw new AuthException(
- String.format("Applying label \"%s\": %d is restricted", lt.getName(), val));
- }
- ent.setValue(perm.squashThenCheck(lt, val));
+ throw new AuthException(
+ String.format("Applying label \"%s\": %d is restricted", lt.getName(), val));
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
index 6a332cd..73cda7f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
@@ -24,32 +24,26 @@
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.LabelValue;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.permissions.LabelPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Collection;
import java.util.List;
/**
- * Normalizes votes on labels according to project config and permissions.
+ * Normalizes votes on labels according to project config.
*
* <p>Votes are recorded in the database for a user based on the state of the project at that time:
- * what labels are defined for the project, and what the user is allowed to vote on. Both of those
- * can change between the time a vote is originally made and a later point, for example when a
- * change is submitted. This class normalizes old votes against current project configuration.
+ * what labels are defined for the project. The label definition can change between the time a vote
+ * is originally made and a later point, for example when a change is submitted. This class
+ * normalizes old votes against current project configuration.
*/
@Singleton
public class LabelNormalizer {
@@ -77,33 +71,24 @@
}
}
- private final Provider<ReviewDb> db;
private final IdentifiedUser.GenericFactory userFactory;
- private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
@Inject
- LabelNormalizer(
- Provider<ReviewDb> db,
- IdentifiedUser.GenericFactory userFactory,
- PermissionBackend permissionBackend,
- ProjectCache projectCache) {
- this.db = db;
+ LabelNormalizer(IdentifiedUser.GenericFactory userFactory, ProjectCache projectCache) {
this.userFactory = userFactory;
- this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
}
/**
* @param notes change containing the given approvals.
* @param approvals list of approvals.
- * @return copies of approvals normalized to the defined ranges for the label type and permissions
- * for the user. Approvals for unknown labels are not included in the output, nor are
- * approvals where the user has no permissions for that label.
+ * @return copies of approvals normalized to the defined ranges for the label type. Approvals for
+ * unknown labels are not included in the output.
* @throws OrmException
*/
public Result normalize(ChangeNotes notes, Collection<PatchSetApproval> approvals)
- throws OrmException, PermissionBackendException, IOException {
+ throws OrmException, IOException {
IdentifiedUser user = userFactory.create(notes.getChange().getOwner());
return normalize(notes, user, approvals);
}
@@ -112,13 +97,12 @@
* @param notes change notes containing the given approvals.
* @param user current user.
* @param approvals list of approvals.
- * @return copies of approvals normalized to the defined ranges for the label type and permissions
- * for the user. Approvals for unknown labels are not included in the output, nor are
- * approvals where the user has no permissions for that label.
+ * @return copies of approvals normalized to the defined ranges for the label type. Approvals for
+ * unknown labels are not included in the output.
*/
public Result normalize(
ChangeNotes notes, CurrentUser user, Collection<PatchSetApproval> approvals)
- throws PermissionBackendException, IOException {
+ throws IOException {
List<PatchSetApproval> unchanged = Lists.newArrayListWithCapacity(approvals.size());
List<PatchSetApproval> updated = Lists.newArrayListWithCapacity(approvals.size());
List<PatchSetApproval> deleted = Lists.newArrayListWithCapacity(approvals.size());
@@ -142,9 +126,7 @@
}
PatchSetApproval copy = copy(psa);
applyTypeFloor(label, copy);
- if (!applyRightFloor(notes, label, copy)) {
- deleted.add(psa);
- } else if (copy.getValue() != psa.getValue()) {
+ if (copy.getValue() != psa.getValue()) {
updated.add(copy);
} else {
unchanged.add(psa);
@@ -157,26 +139,6 @@
return new PatchSetApproval(src.getPatchSetId(), src);
}
- private boolean applyRightFloor(ChangeNotes notes, LabelType lt, PatchSetApproval a)
- throws PermissionBackendException {
- PermissionBackend.ForChange forChange =
- permissionBackend.user(userFactory.create(a.getAccountId())).database(db).change(notes);
- // Check if the user is allowed to vote on the label at all
- try {
- forChange.check(new LabelPermission(lt.getName()));
- } catch (AuthException e) {
- return false;
- }
- // Squash vote to nearest allowed value
- try {
- forChange.check(new LabelPermission.WithValue(lt.getName(), a.getValue()));
- return true;
- } catch (AuthException e) {
- a.setValue(forChange.squashThenCheck(lt, a.getValue()));
- return true;
- }
- }
-
private void applyTypeFloor(LabelType lt, PatchSetApproval a) {
LabelValue atMin = lt.getMin();
if (atMin != null && a.getValue() < atMin.getValue()) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
index 33f9b7d..ed5695c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
@@ -19,11 +19,9 @@
import com.google.common.base.CharMatcher;
import com.google.common.base.Joiner;
-import com.google.common.base.MoreObjects;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
@@ -33,6 +31,7 @@
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
@@ -67,6 +66,7 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
+import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@@ -156,9 +156,6 @@
private static final String KEY_VALUE = "value";
private static final String KEY_CAN_OVERRIDE = "canOverride";
private static final String KEY_BRANCH = "branch";
- private static final ImmutableSet<String> LABEL_FUNCTIONS =
- ImmutableSet.of(
- "MaxWithBlock", "AnyWithBlock", "MaxNoBlock", "NoBlock", "NoOp", "PatchSetLock");
private static final String REVIEWER = "reviewer";
private static final String KEY_ENABLE_REVIEWER_BY_EMAIL = "enableByEmail";
@@ -868,19 +865,20 @@
continue;
}
- String functionName =
- MoreObjects.firstNonNull(rc.getString(LABEL, name, KEY_FUNCTION), "MaxWithBlock");
- if (LABEL_FUNCTIONS.contains(functionName)) {
- label.setFunctionName(functionName);
- } else {
+ String functionName = rc.getString(LABEL, name, KEY_FUNCTION);
+ Optional<LabelFunction> function =
+ functionName != null
+ ? LabelFunction.parse(functionName)
+ : Optional.of(LabelFunction.MAX_WITH_BLOCK);
+ if (!function.isPresent()) {
error(
new ValidationError(
PROJECT_CONFIG,
String.format(
"Invalid %s for label \"%s\". Valid names are: %s",
- KEY_FUNCTION, name, Joiner.on(", ").join(LABEL_FUNCTIONS))));
- label.setFunctionName(null);
+ KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet()))));
}
+ label.setFunction(function.orElse(null));
if (!values.isEmpty()) {
short dv = (short) rc.getInt(LABEL, name, KEY_DEFAULT_VALUE, 0);
@@ -1367,7 +1365,7 @@
String name = e.getKey();
LabelType label = e.getValue();
toUnset.remove(name);
- rc.setString(LABEL, name, KEY_FUNCTION, label.getFunctionName());
+ rc.setString(LABEL, name, KEY_FUNCTION, label.getFunction().getFunctionName());
rc.setInt(LABEL, name, KEY_DEFAULT_VALUE, label.getDefaultValue());
setBooleanConfigKey(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
index 152d398..9a362d4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
@@ -45,7 +45,6 @@
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.git.SubmoduleException;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -330,7 +329,7 @@
}
private void setApproval(ChangeContext ctx, IdentifiedUser user)
- throws OrmException, IOException, PermissionBackendException {
+ throws OrmException, IOException {
Change.Id id = ctx.getChange().getId();
List<SubmitRecord> records = args.commitStatus.getSubmitRecords(id);
PatchSet.Id oldPsId = toMerge.getPatchsetId();
@@ -352,7 +351,7 @@
}
private LabelNormalizer.Result approve(ChangeContext ctx, ChangeUpdate update)
- throws OrmException, IOException, PermissionBackendException {
+ throws OrmException, IOException {
PatchSet.Id psId = update.getPatchSetId();
Map<PatchSetApproval.Key, PatchSetApproval> byKey = new HashMap<>();
for (PatchSetApproval psa :
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
index 6db9357..c87e8e4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -429,65 +429,5 @@
.map((v) -> new LabelPermission.WithValue(label, v))
.collect(toSet());
}
-
- /**
- * Squash a label value to the nearest allowed value.
- *
- * <p>For multi-valued labels like Code-Review with values -2..+2 a user may try to use +2, but
- * only have permission for the -1..+1 range. The caller should have already tried:
- *
- * <pre>
- * check(new LabelPermission.WithValue("Code-Review", 2));
- * </pre>
- *
- * and caught {@link AuthException}. {@code squashThenCheck} will use {@link #test(LabelType)}
- * to determine potential values of Code-Review the user can use, and select the nearest value
- * along the same sign, e.g. -1 for -2 and +1 for +2.
- *
- * @param label definition of the label to test values of.
- * @param val previously denied value the user attempted.
- * @return nearest allowed value, or {@code 0} if no value was allowed.
- * @throws PermissionBackendException backend cannot run test or check.
- */
- public short squashThenCheck(LabelType label, short val) throws PermissionBackendException {
- short s = squashByTest(label, val);
- if (s == 0 || s == val) {
- return 0;
- }
- try {
- check(new LabelPermission.WithValue(label, s));
- return s;
- } catch (AuthException e) {
- return 0;
- }
- }
-
- /**
- * Squash a label value to the nearest allowed value using only test methods.
- *
- * <p>Tests all possible values and selects the closet available to {@code val} while matching
- * the sign of {@code val}. Unlike {@code #squashThenCheck(LabelType, short)} this method only
- * uses {@code test} methods and should not be used in contexts like a review handler without
- * checking the resulting score.
- *
- * @param label definition of the label to test values of.
- * @param val previously denied value the user attempted.
- * @return nearest likely allowed value, or {@code 0} if no value was identified.
- * @throws PermissionBackendException backend cannot run test.
- */
- public short squashByTest(LabelType label, short val) throws PermissionBackendException {
- return nearest(test(label), val);
- }
-
- private static short nearest(Iterable<LabelPermission.WithValue> possible, short wanted) {
- short s = 0;
- for (LabelPermission.WithValue v : possible) {
- if ((wanted < 0 && v.value() < 0 && wanted <= v.value() && v.value() < s)
- || (wanted > 0 && v.value() > 0 && wanted >= v.value() && v.value() > s)) {
- s = v.value();
- }
- }
- return s;
- }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index 2203f77..484cd0c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -22,6 +22,7 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -300,7 +301,7 @@
.byLabel(ap.getLabel());
if (type != null
&& ap.getValue() == 1
- && type.getFunctionName().equalsIgnoreCase("PatchSetLock")) {
+ && type.getFunction() == LabelFunction.PATCH_SET_LOCK) {
return true;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
index 1917d6f..785ae38 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -79,7 +79,7 @@
for (PatchSetApproval p : object.currentApprovals()) {
if (labelType.matches(p)) {
hasVote = true;
- if (match(object, p.getValue(), p.getAccountId(), labelType)) {
+ if (match(object, p.getValue(), p.getAccountId())) {
return true;
}
}
@@ -105,7 +105,7 @@
return null;
}
- protected boolean match(ChangeData cd, short value, Account.Id approver, LabelType type) {
+ protected boolean match(ChangeData cd, short value, Account.Id approver) {
if (value != expVal) {
return false;
}
@@ -119,11 +119,11 @@
return false;
}
- // Double check the value is still permitted for the user.
+ // Check the user has 'READ' permission.
try {
PermissionBackend.ForChange perm =
permissionBackend.user(reviewer).database(dbProvider).change(cd);
- return perm.test(ChangePermission.READ) && expVal == perm.squashByTest(type, value);
+ return perm.test(ChangePermission.READ);
} catch (PermissionBackendException e) {
return false;
}
diff --git a/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java b/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java
index 9bfcc61..2c76999 100644
--- a/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java
+++ b/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java
@@ -78,7 +78,7 @@
return new StructureTerm(
symLabelType,
SymbolTerm.intern(type.getName()),
- SymbolTerm.intern(type.getFunctionName()),
+ SymbolTerm.intern(type.getFunction().getFunctionName()),
min != null ? new IntegerTerm(min.getValue()) : NONE,
max != null ? new IntegerTerm(max.getValue()) : NONE);
}
diff --git a/gerrit-server/src/main/prolog/gerrit_common.pl b/gerrit-server/src/main/prolog/gerrit_common.pl
index 4671e0d..8fd0657 100644
--- a/gerrit-server/src/main/prolog/gerrit_common.pl
+++ b/gerrit-server/src/main/prolog/gerrit_common.pl
@@ -279,12 +279,12 @@
!,
max_with_block(Label, Min, Max, S).
max_with_block(Label, Min, Max, reject(Who)) :-
- check_label_range_permission(Label, Min, ok(Who)),
+ commit_label(label(Label, Min), Who),
!
.
max_with_block(Label, Min, Max, ok(Who)) :-
- \+ check_label_range_permission(Label, Min, ok(_)),
- check_label_range_permission(Label, Max, ok(Who)),
+ \+ commit_label(label(Label, Min), _),
+ commit_label(label(Label, Max), Who),
!
.
max_with_block(Label, Min, Max, need(Max)) :-
@@ -306,7 +306,7 @@
%%
any_with_block(Label, Min, reject(Who)) :-
Min < 0,
- check_label_range_permission(Label, Min, ok(Who)),
+ commit_label(label(Label, Min), Who),
!
.
any_with_block(Label, Min, may(_)).
@@ -321,7 +321,7 @@
!,
max_no_block(Label, Max, S).
max_no_block(Label, Max, ok(Who)) :-
- check_label_range_permission(Label, Max, ok(Who)),
+ commit_label(label(Label, Max), Who),
!
.
max_no_block(Label, Max, need(Max)) :-
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
index 75b00fd..5453fad 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
@@ -150,7 +150,7 @@
}
@Test
- public void normalizeByPermission() throws Exception {
+ public void noNormalizeByPermission() throws Exception {
ProjectConfig pc = loadAllProjects();
allow(pc, forLabel("Code-Review"), -1, 1, REGISTERED_USERS, "refs/heads/*");
allow(pc, forLabel("Verified"), -1, 1, REGISTERED_USERS, "refs/heads/*");
@@ -158,8 +158,7 @@
PatchSetApproval cr = psa(userId, "Code-Review", 2);
PatchSetApproval v = psa(userId, "Verified", 1);
- assertEquals(
- Result.create(list(v), list(copy(cr, 1)), list()), norm.normalize(notes, list(cr, v)));
+ assertEquals(Result.create(list(cr, v), list(), list()), norm.normalize(notes, list(cr, v)));
}
@Test
@@ -177,10 +176,10 @@
}
@Test
- public void emptyPermissionRangeOmitsResult() throws Exception {
+ public void emptyPermissionRangeKeepsResult() throws Exception {
PatchSetApproval cr = psa(userId, "Code-Review", 1);
PatchSetApproval v = psa(userId, "Verified", 1);
- assertEquals(Result.create(list(), list(), list(cr, v)), norm.normalize(notes, list(cr, v)));
+ assertEquals(Result.create(list(cr, v), list(), list()), norm.normalize(notes, list(cr, v)));
}
@Test
@@ -191,7 +190,7 @@
PatchSetApproval cr = psa(userId, "Code-Review", 0);
PatchSetApproval v = psa(userId, "Verified", 0);
- assertEquals(Result.create(list(cr), list(), list(v)), norm.normalize(notes, list(cr, v)));
+ assertEquals(Result.create(list(cr, v), list(), list()), norm.normalize(notes, list(cr, v)));
}
private ProjectConfig loadAllProjects() throws Exception {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
index 5a72d5c..6604641 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
@@ -17,6 +17,7 @@
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
@@ -47,7 +48,7 @@
public static final LabelType patchSetLock() {
LabelType label =
category("Patch-Set-Lock", value(1, "Patch Set Locked"), value(0, "Patch Set Unlocked"));
- label.setFunctionName("PatchSetLock");
+ label.setFunction(LabelFunction.PATCH_SET_LOCK);
return label;
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java
index 7eda3cc..3cd1696 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java
@@ -18,6 +18,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.LabelValue;
@@ -105,7 +106,7 @@
assertThat(codeReview).isNotNull();
assertThat(codeReview.getName()).isEqualTo("Code-Review");
assertThat(codeReview.getDefaultValue()).isEqualTo(0);
- assertThat(codeReview.getFunctionName()).isEqualTo("MaxWithBlock");
+ assertThat(codeReview.getFunction()).isEqualTo(LabelFunction.MAX_WITH_BLOCK);
assertThat(codeReview.isCopyMinScore()).isTrue();
assertValueRange(codeReview, 2, 1, 0, -1, -2);
}
diff --git a/gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl b/gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl
index c993394..a7df2b9 100644
--- a/gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl
+++ b/gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl
@@ -65,7 +65,7 @@
test(default_submit_fails) :-
findall(P, default_submit(P), All),
All = [submit(C, V)],
- C = label('Code-Review', ok(test_user(alice))),
+ C = label('Code-Review', ok(_)),
V = label('Verified', need(1)).
@@ -84,7 +84,7 @@
test(can_submit_not_ready) :-
can_submit(gerrit:default_submit, S),
S = not_ready(submit(C, V)),
- C = label('Code-Review', ok(test_user(alice))),
+ C = label('Code-Review', ok(_)),
V = label('Verified', need(1)).
test(can_submit_only_verified_not_ready) :-
@@ -99,7 +99,7 @@
can_submit(gerrit:default_submit, R),
filter_submit_results(filter_out_v, [R], S),
S = [ok(submit(C))],
- C = label('Code-Review', ok(test_user(alice))).
+ C = label('Code-Review', ok(_)).
test(filter_submit_add_code_review) :-
set_commit_labels([
@@ -119,7 +119,7 @@
can_submit(gerrit:default_submit, R),
arg(1, R, S),
find_label(S, 'Code-Review', L),
- L = label('Code-Review', ok(test_user(alice))).
+ L = label('Code-Review', ok(_)).
test(find_default_verified) :-
can_submit(gerrit:default_submit, R),
@@ -133,7 +133,7 @@
test(remove_default_code_review) :-
can_submit(gerrit:default_submit, R),
arg(1, R, S),
- C = label('Code-Review', ok(test_user(alice))),
+ C = label('Code-Review', ok(_)),
remove_label(S, C, Out),
Out = submit(V),
V = label('Verified', need(1)).
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java
index 634c47c..1a123bf 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java
@@ -16,6 +16,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.base.Joiner;
import com.google.common.util.concurrent.Atomics;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
@@ -110,6 +111,9 @@
/** Unparsed command line options. */
private String[] argv;
+ /** trimmed command line arguments. */
+ private String[] trimmedArgv;
+
public BaseCommand() {
task = Atomics.newReference();
}
@@ -155,6 +159,26 @@
this.argv = argv;
}
+ /**
+ * Trim the argument if it is spanning multiple lines.
+ *
+ * @return the arguments where all the multiple-line fields are trimmed.
+ */
+ protected String[] getTrimmedArguments() {
+ if (trimmedArgv == null && argv != null) {
+ trimmedArgv = new String[argv.length];
+ for (int i = 0; i < argv.length; i++) {
+ String arg = argv[i];
+ int indexOfMultiLine = arg.indexOf("\n");
+ if (indexOfMultiLine > -1) {
+ arg = arg.substring(0, indexOfMultiLine).concat(" [trimmed]");
+ }
+ trimmedArgv[i] = arg;
+ }
+ }
+ return trimmedArgv;
+ }
+
@Override
public void destroy() {
Future<?> future = task.getAndSet(null);
@@ -370,8 +394,11 @@
}
protected String getTaskDescription() {
- StringBuilder m = new StringBuilder();
- m.append(context.getCommandLine());
+ StringBuilder m = new StringBuilder(commandName);
+ String[] ta = getTrimmedArguments();
+ if (ta != null) {
+ m.append(Joiner.on(" ").join(ta));
+ }
return m.toString();
}
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java
index 12064c8..612f0eb 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java
@@ -14,6 +14,7 @@
package com.google.gerrit.sshd;
+import com.google.common.base.Joiner;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
import com.google.gerrit.audit.AuditService;
@@ -282,9 +283,9 @@
return "Command was already destroyed";
}
StringBuilder commandName = new StringBuilder(dcmd.getCommandName());
- String[] args = dcmd.getArguments();
- for (int i = 1; i < args.length; i++) {
- commandName.append(".").append(args[i]);
+ String[] trimmedArgs = dcmd.getTrimmedArguments();
+ if (trimmedArgs != null) {
+ commandName.append(Joiner.on(".").join(trimmedArgs));
}
return commandName.toString();
}
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
index ff444e6..46d6dca 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -135,12 +135,6 @@
private boolean json;
@Option(
- name = "--strict-labels",
- usage = "Strictly check if the labels specified can be applied to the given patch set(s)"
- )
- private boolean strictLabels;
-
- @Option(
name = "--tag",
aliases = "-t",
usage = "applies a tag to the given review",
@@ -247,11 +241,6 @@
}
}
- @Override
- protected String getTaskDescription() {
- return "gerrit review";
- }
-
private void applyReview(PatchSet patchSet, ReviewInput review) throws RestApiException {
gApi.changes()
.id(patchSet.getId().getParentKey().get())
@@ -279,7 +268,6 @@
review.notify = notify;
review.labels = new TreeMap<>();
review.drafts = ReviewInput.DraftHandling.PUBLISH;
- review.strictLabels = strictLabels;
for (ApproveOption ao : optionList) {
Short v = ao.value();
if (v != null) {
