Provide a way to lock patch sets

Add a new function type that provides a way to prevent new patch sets
from being added to a change. Defining a label with this function and
applying a +1 vote with that label onto a change will stop: 1) new
patch upload, 2) rebase, and 3) abandon.

Like NoBlock/NoOp, its value will never affect submittability.

Change-Id: Ib0d81d85e797f522d80da8c474a711ec5021f4b6
This commit is contained in:
Nasser Grainawi
2015-11-09 09:56:56 -08:00
parent c767f0a26a
commit 240ea29e21
10 changed files with 128 additions and 22 deletions

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 The label is purely informational and values are not considered when
determining whether a change is submittable. 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_copyMinScore]]
=== `label.Label-Name.copyMinScore` === `label.Label-Name.copyMinScore`

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.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume; import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.GitUtil.pushHead; 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.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS; 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.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount; 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.client.InheritableBoolean;
import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.LabelInfo; 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.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.eclipse.jgit.revwalk.RevCommit;
import org.joda.time.DateTime; import org.joda.time.DateTime;
@@ -51,6 +60,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
} }
private String sshUrl; private String sshUrl;
private LabelType patchSetLock;
@BeforeClass @BeforeClass
public static void setTimeForTesting() { public static void setTimeForTesting() {
@@ -73,6 +83,24 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
sshUrl = sshSession.getUrl(); 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 { protected void selectProtocol(Protocol p) throws Exception {
@@ -275,6 +303,18 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
r.assertOkStatus(); 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 @Test
public void testPushForMasterWithApprovals_MissingLabel() throws Exception { public void testPushForMasterWithApprovals_MissingLabel() throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master/%l=Verify"); PushOneCommit.Result r = pushTo("refs/for/master/%l=Verify");

View File

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

View File

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

View File

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

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_CAN_OVERRIDE = "canOverride";
private static final String KEY_Branch = "branch"; private static final String KEY_Branch = "branch";
private static final Set<String> LABEL_FUNCTIONS = ImmutableSet.of( 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"; private static final String PLUGIN = "plugin";

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

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.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData;
@@ -103,23 +104,27 @@ public class ChangeControl {
private final ChangeData.Factory changeDataFactory; private final ChangeData.Factory changeDataFactory;
private final RefControl refControl; private final RefControl refControl;
private final ChangeNotes notes; private final ChangeNotes notes;
private final ApprovalsUtil approvalsUtil;
@AssistedInject @AssistedInject
ChangeControl( ChangeControl(
ChangeData.Factory changeDataFactory, ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory, ChangeNotes.Factory notesFactory,
ApprovalsUtil approvalsUtil,
@Assisted RefControl refControl, @Assisted RefControl refControl,
@Assisted Change change) { @Assisted Change change) {
this(changeDataFactory, refControl, this(changeDataFactory, approvalsUtil, refControl,
notesFactory.create(change)); notesFactory.create(change));
} }
@AssistedInject @AssistedInject
ChangeControl( ChangeControl(
ChangeData.Factory changeDataFactory, ChangeData.Factory changeDataFactory,
ApprovalsUtil approvalsUtil,
@Assisted RefControl refControl, @Assisted RefControl refControl,
@Assisted ChangeNotes notes) { @Assisted ChangeNotes notes) {
this.changeDataFactory = changeDataFactory; this.changeDataFactory = changeDataFactory;
this.approvalsUtil = approvalsUtil;
this.refControl = refControl; this.refControl = refControl;
this.notes = notes; this.notes = notes;
} }
@@ -128,7 +133,7 @@ public class ChangeControl {
if (getUser().equals(who)) { if (getUser().equals(who)) {
return this; return this;
} }
return new ChangeControl(changeDataFactory, return new ChangeControl(changeDataFactory, approvalsUtil,
getRefControl().forUser(who), notes); getRefControl().forUser(who), notes);
} }
@@ -190,13 +195,13 @@ public class ChangeControl {
} }
/** Can this user abandon this change? */ /** Can this user abandon this change? */
public boolean canAbandon() { public boolean canAbandon(ReviewDb db) throws OrmException {
return isOwner() // owner (aka creator) of the change can abandon return (isOwner() // owner (aka creator) of the change can abandon
|| getRefControl().isOwner() // branch owner can abandon || getRefControl().isOwner() // branch owner can abandon
|| getProjectControl().isOwner() // project owner can abandon || getProjectControl().isOwner() // project owner can abandon
|| getUser().getCapabilities().canAdministrateServer() // site administers are god || getUser().getCapabilities().canAdministrateServer() // site administers are god
|| getRefControl().canAbandon() // user can abandon a specific ref || 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? */ /** 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? */ /** Can this user rebase this change? */
public boolean canRebase() { public boolean canRebase(ReviewDb db) throws OrmException {
return isOwner() || getRefControl().canSubmit() return (isOwner() || getRefControl().canSubmit()
|| getRefControl().canRebase(); || getRefControl().canRebase()) && !isPatchSetLocked(db);
} }
/** Can this user restore this change? */ /** Can this user restore this change? */
public boolean canRestore() { public boolean canRestore(ReviewDb db) throws OrmException {
return canAbandon() // Anyone who can abandon the change can restore it back return canAbandon(db) // Anyone who can abandon the change can restore it back
&& getRefControl().canUpload(); // as long as you can upload too && 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? */ /** Can this user add a patch set to this change? */
public boolean canAddPatchSet() { public boolean canAddPatchSet(ReviewDb db) throws OrmException {
return getRefControl().canUpload(); 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? */ /** Is this user the owner of the change? */

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('MaxNoBlock', Label, Min, Max, T) :- !, max_no_block(Label, Max, T).
legacy_submit_rule('NoBlock', Label, Min, Max, T) :- !, T = may(_). legacy_submit_rule('NoBlock', Label, Min, Max, T) :- !, T = may(_).
legacy_submit_rule('NoOp', 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)). legacy_submit_rule(Fun, Label, Min, Max, T) :- T = impossible(unsupported(Fun)).
%% max_with_block: %% max_with_block:

View File

@@ -96,6 +96,14 @@ public class Util {
value(-2, "This shall not be merged")); 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) { public static LabelValue value(int value, String text) {
return new LabelValue((short) value, text); return new LabelValue((short) value, text);
} }