opendev/gerrit
Code Issues Proposed changes

Merge changes Ib0d81d85,I319914ca

Browse Source 
* changes:
  Provide a way to lock patch sets
  Fix logic for revert permissions
This commit is contained in:
Shawn Pearce
2015-11-09 21:44:04 +00:00
committed by Gerrit Code Review
parent 7837cc0ca5 240ea29e21
commit 92bc6b42e5
11 changed files with 132 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
Documentation/config-labels.txt
View File

@@ -217,6 +217,19 @@ the lowest possible negative value will not block the change.
The label is purely informational and values are not considered when
determining whether a change is submittable.
* `PatchSetLock`
+
The `PatchSetLock` function provides a locking mechanism for patch
sets. This function's values are not considered when determining
whether a change is submittable. When set, no new patchsets can be
created and rebase and abandon are blocked.
+
This function is designed to allow overlapping locks, so several lock
accounts could lock the same change.
+
Allowed range of values are 0 (Patch Set Unlocked) to 1 (Patch Set
Locked).
[[label_copyMinScore]]
=== `label.Label-Name.copyMinScore`

40
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.git;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
@@ -26,11 +27,19 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.git.MetaDataUpdate;
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.revwalk.RevCommit;
import org.joda.time.DateTime;
@@ -51,6 +60,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
}
private String sshUrl;
private LabelType patchSetLock;
@BeforeClass
public static void setTimeForTesting() {
@@ -73,6 +83,24 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
@Before
public void setUp() throws Exception {
sshUrl = sshSession.getUrl();
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
patchSetLock = Util.patchSetLock();
cfg.getLabelSections().put(patchSetLock.getName(), patchSetLock);
AccountGroup.UUID anonymousUsers =
SystemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID();
Util.allow(cfg, Permission.forLabel(patchSetLock.getName()), 0, 1, anonymousUsers,
"refs/heads/*");
saveProjectConfig(cfg);
grant(Permission.LABEL + "Patch-Set-Lock", project, "refs/heads/*");
}
private void saveProjectConfig(ProjectConfig cfg) throws Exception {
MetaDataUpdate md = metaDataUpdateFactory.create(project);
try {
cfg.commit(md);
} finally {
md.close();
}
}
protected void selectProtocol(Protocol p) throws Exception {
@@ -275,6 +303,18 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
r.assertOkStatus();
}
@Test
public void testPushNewPatchsetToPatchSetLockedChange() throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master");
r.assertOkStatus();
PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo,
PushOneCommit.SUBJECT, "b.txt", "anotherContent", r.getChangeId());
revision(r).review(new ReviewInput().label("Patch-Set-Lock", 1));
r = push.to("refs/for/master");
r.assertErrorStatus("cannot replace " + r.getChange().change().getChangeId()
+ ". Change is patch set locked.");
}
@Test
public void testPushForMasterWithApprovals_MissingLabel() throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master/%l=Verify");

10
gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java
View File

@@ -82,7 +82,7 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
throws RestApiException, UpdateException, OrmException {
ChangeControl control = req.getControl();
IdentifiedUser caller = control.getUser().asIdentifiedUser();
if (!control.canAbandon()) {
if (!control.canAbandon(dbProvider.get())) {
throw new AuthException("abandon not permitted");
}
Change change = abandon(control, input.message, caller.getAccount());
@@ -174,12 +174,18 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
@Override
public UiAction.Description getDescription(ChangeResource resource) {
boolean canAbandon = false;
try {
canAbandon = resource.getControl().canAbandon(dbProvider.get());
} catch (OrmException e) {
log.error("Cannot check canAbandon status. Assuming false.", e);
}
return new UiAction.Description()
.setLabel("Abandon")
.setTitle("Abandon the change")
.setVisible(resource.getChange().getStatus().isOpen()
&& resource.getChange().getStatus() != Change.Status.DRAFT
&& resource.getControl().canAbandon());
&& canAbandon);
}
private static String status(Change change) {

10
gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java
View File

@@ -90,7 +90,7 @@ public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
ObjectInserter oi = repo.newObjectInserter();
BatchUpdate bu = updateFactory.create(dbProvider.get(),
change.getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
if (!control.canRebase()) {
if (!control.canRebase(dbProvider.get())) {
throw new AuthException("rebase not permitted");
} else if (!change.getStatus().isOpen()) {
throw new ResourceConflictException("change is "
@@ -209,9 +209,15 @@ public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
public UiAction.Description getDescription(RevisionResource resource) {
PatchSet patchSet = resource.getPatchSet();
Branch.NameKey dest = resource.getChange().getDest();
boolean canRebase = false;
try {
canRebase = resource.getControl().canRebase(dbProvider.get());
} catch (OrmException e) {
log.error("Cannot check canRebase status. Assuming false.", e);
}
boolean visible = resource.getChange().getStatus().isOpen()
&& resource.isCurrent()
&& resource.getControl().canRebase();
&& canRebase;
boolean enabled = true;
if (visible) {

10
gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java
View File

@@ -80,7 +80,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
public ChangeInfo apply(ChangeResource req, RestoreInput input)
throws RestApiException, UpdateException, OrmException {
ChangeControl ctl = req.getControl();
if (!ctl.canRestore()) {
if (!ctl.canRestore(dbProvider.get())) {
throw new AuthException("restore not permitted");
}
@@ -160,11 +160,17 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
@Override
public UiAction.Description getDescription(ChangeResource resource) {
boolean canRestore = false;
try {
canRestore = resource.getControl().canRestore(dbProvider.get());
} catch (OrmException e) {
log.error("Cannot check canRestore status. Assuming false.", e);
}
return new UiAction.Description()
.setLabel("Restore")
.setTitle("Restore the change")
.setVisible(resource.getChange().getStatus() == Status.ABANDONED
&& resource.getControl().canRestore());
&& canRestore);
}
private static String status(Change change) {

8
gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java
View File

@@ -29,8 +29,8 @@ import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.RefControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -59,9 +59,9 @@ public class Revert implements RestModifyView<ChangeResource, RevertInput>,
public ChangeInfo apply(ChangeResource req, RevertInput input)
throws IOException, OrmException, RestApiException,
UpdateException {
ChangeControl control = req.getControl();
RefControl refControl = req.getControl().getRefControl();
Change change = req.getChange();
if (!control.canAddPatchSet()) {
if (!refControl.canUpload()) {
throw new AuthException("revert not permitted");
} else if (change.getStatus() != Status.MERGED) {
throw new ResourceConflictException("change is " + status(change));
@@ -69,7 +69,7 @@ public class Revert implements RestModifyView<ChangeResource, RevertInput>,
Change.Id revertedChangeId;
try {
revertedChangeId = changeUtil.revert(control,
revertedChangeId = changeUtil.revert(req.getControl(),
change.currentPatchSetId(),
Strings.emptyToNull(input.message),
new PersonIdent(myIdent, TimeUtil.nowTs()));

2
gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
View File

@@ -141,7 +141,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
private static final String KEY_CAN_OVERRIDE = "canOverride";
private static final String KEY_Branch = "branch";
private static final Set<String> LABEL_FUNCTIONS = ImmutableSet.of(
"MaxWithBlock", "AnyWithBlock", "MaxNoBlock", "NoBlock", "NoOp");
"MaxWithBlock", "AnyWithBlock", "MaxNoBlock", "NoBlock", "NoOp", "PatchSetLock");
private static final String PLUGIN = "plugin";

10
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -1966,7 +1966,7 @@ public class ReceiveCommits {
}
}
boolean validate(boolean autoClose) throws IOException {
boolean validate(boolean autoClose) throws IOException, OrmException {
if (!autoClose && inputCommand.getResult() != NOT_ATTEMPTED) {
return false;
} else if (change == null) {
@@ -1989,8 +1989,12 @@ public class ReceiveCommits {
}
changeCtl = projectControl.controlFor(change);
if (!changeCtl.canAddPatchSet()) {
reject(inputCommand, "cannot replace " + ontoChange);
if (!changeCtl.canAddPatchSet(db)) {
String locked = ".";
if (changeCtl.isPatchSetLocked(db)) {
locked = ". Change is patch set locked.";
}
reject(inputCommand, "cannot replace " + ontoChange + locked);
return false;
} else if (change.getStatus().isClosed()) {
reject(inputCommand, "change " + ontoChange + " closed");

46
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -28,6 +28,7 @@ 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.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.query.change.ChangeData;
@@ -103,23 +104,27 @@ public class ChangeControl {
private final ChangeData.Factory changeDataFactory;
private final RefControl refControl;
private final ChangeNotes notes;
private final ApprovalsUtil approvalsUtil;
@AssistedInject
ChangeControl(
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory,
ApprovalsUtil approvalsUtil,
@Assisted RefControl refControl,
@Assisted Change change) {
this(changeDataFactory, refControl,
this(changeDataFactory, approvalsUtil, refControl,
notesFactory.create(change));
}
@AssistedInject
ChangeControl(
ChangeData.Factory changeDataFactory,
ApprovalsUtil approvalsUtil,
@Assisted RefControl refControl,
@Assisted ChangeNotes notes) {
this.changeDataFactory = changeDataFactory;
this.approvalsUtil = approvalsUtil;
this.refControl = refControl;
this.notes = notes;
}
@@ -128,7 +133,7 @@ public class ChangeControl {
if (getUser().equals(who)) {
return this;
}
return new ChangeControl(changeDataFactory,
return new ChangeControl(changeDataFactory, approvalsUtil,
getRefControl().forUser(who), notes);
}
@@ -190,13 +195,13 @@ public class ChangeControl {
}
/** Can this user abandon this change? */
public boolean canAbandon() {
return isOwner() // owner (aka creator) of the change can abandon
public boolean canAbandon(ReviewDb db) throws OrmException {
return (isOwner() // owner (aka creator) of the change can abandon
|| getRefControl().isOwner() // branch owner can abandon
|| getProjectControl().isOwner() // project owner can abandon
|| getUser().getCapabilities().canAdministrateServer() // site administers are god
|| getRefControl().canAbandon() // user can abandon a specific ref
;
) && !isPatchSetLocked(db);
}
/** Can this user publish this draft change or any draft patch set of this change? */
@@ -212,14 +217,14 @@ public class ChangeControl {
}
/** Can this user rebase this change? */
public boolean canRebase() {
return isOwner() || getRefControl().canSubmit()
|| getRefControl().canRebase();
public boolean canRebase(ReviewDb db) throws OrmException {
return (isOwner() || getRefControl().canSubmit()
|| getRefControl().canRebase()) && !isPatchSetLocked(db);
}
/** Can this user restore this change? */
public boolean canRestore() {
return canAbandon() // Anyone who can abandon the change can restore it back
public boolean canRestore(ReviewDb db) throws OrmException {
return canAbandon(db) // Anyone who can abandon the change can restore it back
&& getRefControl().canUpload(); // as long as you can upload too
}
@@ -258,8 +263,25 @@ public class ChangeControl {
}
/** Can this user add a patch set to this change? */
public boolean canAddPatchSet() {
return getRefControl().canUpload();
public boolean canAddPatchSet(ReviewDb db) throws OrmException {
return getRefControl().canUpload() && !isPatchSetLocked(db);
}
/** Is the current patch set locked against state changes? */
public boolean isPatchSetLocked(ReviewDb db) throws OrmException {
if (getChange().getStatus() == Change.Status.MERGED) {
return false;
}
for (PatchSetApproval ap : approvalsUtil.byPatchSet(db, this,
getChange().currentPatchSetId())) {
LabelType type = getLabelTypes().byLabel(ap.getLabel());
if (type != null && ap.getValue() == 1
&& type.getFunctionName().equalsIgnoreCase("PatchSetLock")) {
return true;
}
}
return false;
}
/** Is this user the owner of the change? */

1
gerrit-server/src/main/prolog/gerrit_common.pl
View File

@@ -243,6 +243,7 @@ legacy_submit_rule('AnyWithBlock', Label, Min, Max, T) :- !, any_with_block(Labe
legacy_submit_rule('MaxNoBlock', Label, Min, Max, T) :- !, max_no_block(Label, Max, T).
legacy_submit_rule('NoBlock', Label, Min, Max, T) :- !, T = may(_).
legacy_submit_rule('NoOp', Label, Min, Max, T) :- !, T = may(_).
legacy_submit_rule('PatchSetLock', Label, Min, Max, T) :- !, T = may(_).
legacy_submit_rule(Fun, Label, Min, Max, T) :- T = impossible(unsupported(Fun)).
%% max_with_block:

8
gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
View File

@@ -96,6 +96,14 @@ public class Util {
value(-2, "This shall not be merged"));
}
public static final LabelType patchSetLock() {
LabelType label = category("Patch-Set-Lock",
value(1, "Patch Set Locked"),
value(0, "Patch Set Unlocked"));
label.setFunctionName("PatchSetLock");
return label;
}
public static LabelValue value(int value, String text) {
return new LabelValue((short) value, text);
}