Merge "Move the 'isPatchSetLocked' check out from the ChangeControl"
diff --git a/java/com/google/gerrit/server/PatchSetUtil.java b/java/com/google/gerrit/server/PatchSetUtil.java
index 82ca8d2..e9ed51b 100644
--- a/java/com/google/gerrit/server/PatchSetUtil.java
+++ b/java/com/google/gerrit/server/PatchSetUtil.java
@@ -26,15 +26,22 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
+import com.google.gerrit.common.data.LabelFunction;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
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.sql.Timestamp;
@@ -48,10 +55,20 @@
@Singleton
public class PatchSetUtil {
private final NotesMigration migration;
+ private final Provider<ApprovalsUtil> approvalsUtilProvider;
+ private final ProjectCache projectCache;
+ private final Provider<ReviewDb> dbProvider;
@Inject
- PatchSetUtil(NotesMigration migration) {
+ PatchSetUtil(
+ NotesMigration migration,
+ Provider<ApprovalsUtil> approvalsUtilProvider,
+ ProjectCache projectCache,
+ Provider<ReviewDb> dbProvider) {
this.migration = migration;
+ this.approvalsUtilProvider = approvalsUtilProvider;
+ this.projectCache = projectCache;
+ this.dbProvider = dbProvider;
}
public PatchSet current(ReviewDb db, ChangeNotes notes) throws OrmException {
@@ -155,4 +172,38 @@
update.setGroups(groups);
db.patchSets().update(Collections.singleton(ps));
}
+
+ /** Check if the current patch set of the change is locked. */
+ public void checkPatchSetNotLocked(ChangeNotes notes, CurrentUser user)
+ throws OrmException, IOException, ResourceConflictException {
+ if (isPatchSetLocked(notes, user)) {
+ throw new ResourceConflictException(
+ String.format("The current patch set of change %s is locked", notes.getChangeId()));
+ }
+ }
+
+ /** Is the current patch set locked against state changes? */
+ public boolean isPatchSetLocked(ChangeNotes notes, CurrentUser user)
+ throws OrmException, IOException {
+ Change change = notes.getChange();
+ if (change.getStatus() == Change.Status.MERGED) {
+ return false;
+ }
+
+ ProjectState projectState = projectCache.checkedGet(notes.getProjectName());
+ checkNotNull(projectState, "Failed to load project %s", notes.getProjectName());
+
+ ApprovalsUtil approvalsUtil = approvalsUtilProvider.get();
+ for (PatchSetApproval ap :
+ approvalsUtil.byPatchSet(
+ dbProvider.get(), notes, user, change.currentPatchSetId(), null, null)) {
+ LabelType type = projectState.getLabelTypes(notes, user).byLabel(ap.getLabel());
+ if (type != null
+ && ap.getValue() == 1
+ && type.getFunction() == LabelFunction.PATCH_SET_LOCK) {
+ return true;
+ }
+ }
+ return false;
+ }
}
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index 3a32f8f..d05d133 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -311,7 +311,11 @@
}
private void validate(RepoContext ctx)
- throws AuthException, ResourceConflictException, IOException, PermissionBackendException {
+ throws AuthException, ResourceConflictException, IOException, PermissionBackendException,
+ OrmException {
+ // Not allowed to create a new patch set if the current patch set is locked.
+ psUtil.checkPatchSetNotLocked(origNotes, ctx.getUser());
+
if (checkAddPatchSetPermission) {
permissionBackend
.user(ctx.getUser())
diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
index 83f0120..80d1cd1 100644
--- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -394,7 +394,8 @@
}
private void assertCanEdit(ChangeNotes notes)
- throws AuthException, PermissionBackendException, IOException, ResourceConflictException {
+ throws AuthException, PermissionBackendException, IOException, ResourceConflictException,
+ OrmException {
if (!currentUser.get().isIdentifiedUser()) {
throw new AuthException("Authentication required");
}
@@ -406,6 +407,8 @@
"change %s is %s", c.getChangeId(), c.getStatus().toString().toLowerCase()));
}
+ // Not allowed to edit if the current patch set is locked.
+ patchSetUtil.checkPatchSetNotLocked(notes, currentUser.get());
try {
permissionBackend
.currentUser()
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 5f65814..dbb6bcf 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -2452,6 +2452,13 @@
RevCommit newCommit = rp.getRevWalk().parseCommit(newCommitId);
RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
+
+ // Not allowed to create a new patch set if the current patch set is locked.
+ if (psUtil.isPatchSetLocked(notes, user)) {
+ reject(inputCommand, "cannot add patch set to " + ontoChange + ".");
+ return false;
+ }
+
try {
permissions.change(notes).database(db).check(ChangePermission.ADD_PATCH_SET);
} catch (AuthException no) {
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 990c318..b13d921 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -21,19 +21,14 @@
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.Permission;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
-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.PatchSetUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
import com.google.gerrit.server.query.change.ChangeData;
@@ -52,19 +47,11 @@
static class Factory {
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory notesFactory;
- private final ApprovalsUtil approvalsUtil;
- private final PatchSetUtil patchSetUtil;
@Inject
- Factory(
- ChangeData.Factory changeDataFactory,
- ChangeNotes.Factory notesFactory,
- ApprovalsUtil approvalsUtil,
- PatchSetUtil patchSetUtil) {
+ Factory(ChangeData.Factory changeDataFactory, ChangeNotes.Factory notesFactory) {
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
- this.approvalsUtil = approvalsUtil;
- this.patchSetUtil = patchSetUtil;
}
ChangeControl create(
@@ -74,27 +61,19 @@
}
ChangeControl create(RefControl refControl, ChangeNotes notes) {
- return new ChangeControl(changeDataFactory, approvalsUtil, refControl, notes, patchSetUtil);
+ return new ChangeControl(changeDataFactory, refControl, notes);
}
}
private final ChangeData.Factory changeDataFactory;
- private final ApprovalsUtil approvalsUtil;
private final RefControl refControl;
private final ChangeNotes notes;
- private final PatchSetUtil patchSetUtil;
private ChangeControl(
- ChangeData.Factory changeDataFactory,
- ApprovalsUtil approvalsUtil,
- RefControl refControl,
- ChangeNotes notes,
- PatchSetUtil patchSetUtil) {
+ ChangeData.Factory changeDataFactory, RefControl refControl, ChangeNotes notes) {
this.changeDataFactory = changeDataFactory;
- this.approvalsUtil = approvalsUtil;
this.refControl = refControl;
this.notes = notes;
- this.patchSetUtil = patchSetUtil;
}
ForChange asForChange(@Nullable ChangeData cd, @Nullable Provider<ReviewDb> db) {
@@ -105,8 +84,7 @@
if (getUser().equals(who)) {
return this;
}
- return new ChangeControl(
- changeDataFactory, approvalsUtil, refControl.forUser(who), notes, patchSetUtil);
+ return new ChangeControl(changeDataFactory, refControl.forUser(who), notes);
}
private CurrentUser getUser() {
@@ -130,26 +108,24 @@
}
/** Can this user abandon this change? */
- private boolean canAbandon(ReviewDb db) throws OrmException {
- return (isOwner() // owner (aka creator) of the change can abandon
- || refControl.isOwner() // branch owner can abandon
- || getProjectControl().isOwner() // project owner can abandon
- || refControl.canPerform(Permission.ABANDON) // user can abandon a specific ref
- || getProjectControl().isAdmin())
- && !isPatchSetLocked(db);
+ private boolean canAbandon() {
+ return isOwner() // owner (aka creator) of the change can abandon
+ || refControl.isOwner() // branch owner can abandon
+ || getProjectControl().isOwner() // project owner can abandon
+ || refControl.canPerform(Permission.ABANDON) // user can abandon a specific ref
+ || getProjectControl().isAdmin();
}
/** Can this user rebase this change? */
- private boolean canRebase(ReviewDb db) throws OrmException {
+ private boolean canRebase() {
return (isOwner() || refControl.canSubmit(isOwner()) || refControl.canRebase())
- && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE)
- && !isPatchSetLocked(db);
+ && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
}
/** Can this user restore this change? */
- private boolean canRestore(ReviewDb db) throws OrmException {
+ private boolean canRestore() {
// Anyone who can abandon the change can restore it, as long as they can create changes.
- return canAbandon(db) && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
+ return canAbandon() && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
}
/** The range of permitted values associated with a label permission. */
@@ -158,8 +134,8 @@
}
/** Can this user add a patch set to this change? */
- private boolean canAddPatchSet(ReviewDb db) throws OrmException {
- if (!(refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE)) || isPatchSetLocked(db)) {
+ private boolean canAddPatchSet() {
+ if (!refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE)) {
return false;
}
if (isOwner()) {
@@ -168,29 +144,6 @@
return refControl.canAddPatchSet();
}
- /** Is the current patch set locked against state changes? */
- private boolean isPatchSetLocked(ReviewDb db) throws OrmException {
- if (getChange().getStatus() == Change.Status.MERGED) {
- return false;
- }
-
- for (PatchSetApproval ap :
- approvalsUtil.byPatchSet(
- db, notes, getUser(), getChange().currentPatchSetId(), null, null)) {
- LabelType type =
- getProjectControl()
- .getProjectState()
- .getLabelTypes(notes, getUser())
- .byLabel(ap.getLabel());
- if (type != null
- && ap.getValue() == 1
- && type.getFunction() == LabelFunction.PATCH_SET_LOCK) {
- return true;
- }
- }
- return false;
- }
-
/** Is this user the owner of the change? */
private boolean isOwner() {
if (getUser().isIdentifiedUser()) {
@@ -355,12 +308,12 @@
case READ:
return isVisible(db(), changeData());
case ABANDON:
- return canAbandon(db());
+ return canAbandon();
case DELETE:
return (isOwner() && refControl.canPerform(Permission.DELETE_OWN_CHANGES))
|| getProjectControl().isAdmin();
case ADD_PATCH_SET:
- return canAddPatchSet(db());
+ return canAddPatchSet();
case EDIT_ASSIGNEE:
return canEditAssignee();
case EDIT_DESCRIPTION:
@@ -370,9 +323,9 @@
case EDIT_TOPIC_NAME:
return canEditTopicName();
case REBASE:
- return canRebase(db());
+ return canRebase();
case RESTORE:
- return canRestore(db());
+ return canRestore();
case SUBMIT:
return refControl.canSubmit(isOwner());
diff --git a/java/com/google/gerrit/server/permissions/ChangePermission.java b/java/com/google/gerrit/server/permissions/ChangePermission.java
index 6a23cdd..ba1785d 100644
--- a/java/com/google/gerrit/server/permissions/ChangePermission.java
+++ b/java/com/google/gerrit/server/permissions/ChangePermission.java
@@ -20,15 +20,39 @@
public enum ChangePermission implements ChangePermissionOrLabel {
READ,
+ /**
+ * The change can't be restored if its current patch set is locked.
+ *
+ * <p>Before checking this permission, the caller should first verify the current patch set of the
+ * change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}.
+ */
RESTORE,
DELETE,
+ /**
+ * The change can't be abandoned if its current patch set is locked.
+ *
+ * <p>Before checking this permission, the caller should first verify the current patch set of the
+ * change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}.
+ */
ABANDON,
EDIT_ASSIGNEE,
EDIT_DESCRIPTION,
EDIT_HASHTAGS,
EDIT_TOPIC_NAME,
REMOVE_REVIEWER,
+ /**
+ * A new patch set can't be added if the patch set is locked for the change.
+ *
+ * <p>Before checking this permission, the caller should first verify the current patch set of the
+ * change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}.
+ */
ADD_PATCH_SET,
+ /**
+ * The change can't be rebased if its current patch set is locked.
+ *
+ * <p>Before checking this permission, the caller should first verify the current patch set of the
+ * change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}.
+ */
REBASE,
SUBMIT,
SUBMIT_AS("submit on behalf of other users");
diff --git a/java/com/google/gerrit/server/restapi/change/Abandon.java b/java/com/google/gerrit/server/restapi/change/Abandon.java
index 5485cf6..116cbdb 100644
--- a/java/com/google/gerrit/server/restapi/change/Abandon.java
+++ b/java/com/google/gerrit/server/restapi/change/Abandon.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.restapi.change;
-import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
-
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.gerrit.common.TimeUtil;
@@ -29,6 +27,7 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.change.AbandonOp;
import com.google.gerrit.server.change.ChangeJson;
@@ -47,14 +46,19 @@
import com.google.inject.Singleton;
import java.io.IOException;
import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@Singleton
public class Abandon extends RetryingRestModifyView<ChangeResource, AbandonInput, ChangeInfo>
implements UiAction<ChangeResource> {
+ private static final Logger log = LoggerFactory.getLogger(Abandon.class);
+
private final Provider<ReviewDb> dbProvider;
private final ChangeJson.Factory json;
private final AbandonOp.Factory abandonOpFactory;
private final NotifyUtil notifyUtil;
+ private final PatchSetUtil patchSetUtil;
@Inject
Abandon(
@@ -62,27 +66,32 @@
ChangeJson.Factory json,
RetryHelper retryHelper,
AbandonOp.Factory abandonOpFactory,
- NotifyUtil notifyUtil) {
+ NotifyUtil notifyUtil,
+ PatchSetUtil patchSetUtil) {
super(retryHelper);
this.dbProvider = dbProvider;
this.json = json;
this.abandonOpFactory = abandonOpFactory;
this.notifyUtil = notifyUtil;
+ this.patchSetUtil = patchSetUtil;
}
@Override
protected ChangeInfo applyImpl(
- BatchUpdate.Factory updateFactory, ChangeResource req, AbandonInput input)
+ BatchUpdate.Factory updateFactory, ChangeResource rsrc, AbandonInput input)
throws RestApiException, UpdateException, OrmException, PermissionBackendException,
IOException, ConfigInvalidException {
- req.permissions().database(dbProvider).check(ChangePermission.ABANDON);
+ // Not allowed to abandon if the current patch set is locked.
+ patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
- NotifyHandling notify = input.notify == null ? defaultNotify(req.getChange()) : input.notify;
+ rsrc.permissions().database(dbProvider).check(ChangePermission.ABANDON);
+
+ NotifyHandling notify = input.notify == null ? defaultNotify(rsrc.getChange()) : input.notify;
Change change =
abandon(
updateFactory,
- req.getNotes(),
- req.getUser(),
+ rsrc.getNotes(),
+ rsrc.getUser(),
input.message,
notify,
notifyUtil.resolveAccounts(input.notifyDetails));
@@ -135,13 +144,29 @@
@Override
public UiAction.Description getDescription(ChangeResource rsrc) {
+ UiAction.Description description =
+ new UiAction.Description()
+ .setLabel("Abandon")
+ .setTitle("Abandon the change")
+ .setVisible(false);
+
Change change = rsrc.getChange();
- return new UiAction.Description()
- .setLabel("Abandon")
- .setTitle("Abandon the change")
- .setVisible(
- and(
- change.getStatus().isOpen(),
- rsrc.permissions().database(dbProvider).testCond(ChangePermission.ABANDON)));
+ if (!change.getStatus().isOpen()) {
+ return description;
+ }
+
+ try {
+ if (patchSetUtil.isPatchSetLocked(rsrc.getNotes(), rsrc.getUser())) {
+ return description;
+ }
+ } catch (OrmException | IOException e) {
+ log.error(
+ String.format(
+ "Failed to check if the current patch set of change %s is locked", change.getId()),
+ e);
+ return description;
+ }
+
+ return description.setVisible(rsrc.permissions().testOrFalse(ChangePermission.ABANDON));
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java b/java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java
index 374aaec..ff85880 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java
@@ -50,7 +50,6 @@
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.restapi.project.CommitsCollection;
@@ -126,8 +125,11 @@
@Override
protected Response<ChangeInfo> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, MergePatchSetInput in)
- throws OrmException, IOException, InvalidChangeOperationException, RestApiException,
- UpdateException, PermissionBackendException {
+ throws OrmException, IOException, RestApiException, UpdateException,
+ PermissionBackendException {
+ // Not allowed to create a new patch set if the current patch set is locked.
+ psUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
+
rsrc.permissions().database(db).check(ChangePermission.ADD_PATCH_SET);
ProjectState projectState = projectCache.checkedGet(rsrc.getProject());
diff --git a/java/com/google/gerrit/server/restapi/change/Move.java b/java/com/google/gerrit/server/restapi/change/Move.java
index 2ad954d..6e049438 100644
--- a/java/com/google/gerrit/server/restapi/change/Move.java
+++ b/java/com/google/gerrit/server/restapi/change/Move.java
@@ -134,6 +134,9 @@
throw new ResourceConflictException("Change is already destined for the specified branch");
}
+ // Not allowed to move if the current patch set is locked.
+ psUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
+
// Move requires abandoning this change, and creating a new change.
try {
rsrc.permissions().database(dbProvider).check(ABANDON);
@@ -279,24 +282,41 @@
@Override
public UiAction.Description getDescription(ChangeResource rsrc) {
+ UiAction.Description description =
+ new UiAction.Description()
+ .setLabel("Move Change")
+ .setTitle("Move change to a different branch")
+ .setVisible(false);
+
Change change = rsrc.getChange();
- boolean projectStatePermitsWrite = false;
+ if (!change.getStatus().isOpen()) {
+ return description;
+ }
+
try {
- projectStatePermitsWrite = projectCache.checkedGet(rsrc.getProject()).statePermitsWrite();
+ if (!projectCache.checkedGet(rsrc.getProject()).statePermitsWrite()) {
+ return description;
+ }
} catch (IOException e) {
log.error("Failed to check if project state permits write: " + rsrc.getProject(), e);
+ return description;
}
- return new UiAction.Description()
- .setLabel("Move Change")
- .setTitle("Move change to a different branch")
- .setVisible(
- and(
- change.getStatus().isOpen() && projectStatePermitsWrite,
- and(
- permissionBackend
- .user(rsrc.getUser())
- .ref(change.getDest())
- .testCond(CREATE_CHANGE),
- rsrc.permissions().database(dbProvider).testCond(ABANDON))));
+
+ try {
+ if (psUtil.isPatchSetLocked(rsrc.getNotes(), rsrc.getUser())) {
+ return description;
+ }
+ } catch (OrmException | IOException e) {
+ log.error(
+ String.format(
+ "Failed to check if the current patch set of change %s is locked", change.getId()),
+ e);
+ return description;
+ }
+
+ return description.setVisible(
+ and(
+ permissionBackend.user(rsrc.getUser()).ref(change.getDest()).testCond(CREATE_CHANGE),
+ rsrc.permissions().database(dbProvider).testCond(ABANDON)));
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PutMessage.java b/java/com/google/gerrit/server/restapi/change/PutMessage.java
index c9c43cb..eb46521 100644
--- a/java/com/google/gerrit/server/restapi/change/PutMessage.java
+++ b/java/com/google/gerrit/server/restapi/change/PutMessage.java
@@ -33,7 +33,6 @@
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyUtil;
import com.google.gerrit.server.change.PatchSetInserter;
-import com.google.gerrit.server.edit.UnchangedCommitMessageException;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
@@ -68,7 +67,7 @@
extends RetryingRestModifyView<ChangeResource, CommitMessageInput, Response<?>> {
private final GitRepositoryManager repositoryManager;
- private final Provider<CurrentUser> currentUserProvider;
+ private final Provider<CurrentUser> userProvider;
private final Provider<ReviewDb> db;
private final TimeZone tz;
private final PatchSetInserter.Factory psInserterFactory;
@@ -81,7 +80,7 @@
PutMessage(
RetryHelper retryHelper,
GitRepositoryManager repositoryManager,
- Provider<CurrentUser> currentUserProvider,
+ Provider<CurrentUser> userProvider,
Provider<ReviewDb> db,
PatchSetInserter.Factory psInserterFactory,
PermissionBackend permissionBackend,
@@ -91,7 +90,7 @@
ProjectCache projectCache) {
super(retryHelper);
this.repositoryManager = repositoryManager;
- this.currentUserProvider = currentUserProvider;
+ this.userProvider = userProvider;
this.db = db;
this.psInserterFactory = psInserterFactory;
this.tz = gerritIdent.getTimeZone();
@@ -104,8 +103,8 @@
@Override
protected Response<String> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource resource, CommitMessageInput input)
- throws IOException, UnchangedCommitMessageException, RestApiException, UpdateException,
- PermissionBackendException, OrmException, ConfigInvalidException {
+ throws IOException, RestApiException, UpdateException, PermissionBackendException,
+ OrmException, ConfigInvalidException {
PatchSet ps = psUtil.current(db.get(), resource.getNotes());
if (ps == null) {
throw new ResourceConflictException("current revision is missing");
@@ -140,7 +139,7 @@
Timestamp ts = TimeUtil.nowTs();
try (BatchUpdate bu =
updateFactory.create(
- db.get(), resource.getChange().getProject(), currentUserProvider.get(), ts)) {
+ db.get(), resource.getChange().getProject(), userProvider.get(), ts)) {
// Ensure that BatchUpdate will update the same repo
bu.setRepository(repository, new RevWalk(objectInserter.newReader()), objectInserter);
@@ -170,8 +169,7 @@
builder.setTreeId(basePatchSetCommit.getTree());
builder.setParentIds(basePatchSetCommit.getParents());
builder.setAuthor(basePatchSetCommit.getAuthorIdent());
- builder.setCommitter(
- currentUserProvider.get().asIdentifiedUser().newCommitterIdent(timestamp, tz));
+ builder.setCommitter(userProvider.get().asIdentifiedUser().newCommitterIdent(timestamp, tz));
builder.setMessage(commitMessage);
ObjectId newCommitId = objectInserter.insert(builder);
objectInserter.flush();
@@ -179,13 +177,17 @@
}
private void ensureCanEditCommitMessage(ChangeNotes changeNotes)
- throws AuthException, PermissionBackendException, IOException, ResourceConflictException {
- if (!currentUserProvider.get().isIdentifiedUser()) {
+ throws AuthException, PermissionBackendException, IOException, ResourceConflictException,
+ OrmException {
+ if (!userProvider.get().isIdentifiedUser()) {
throw new AuthException("Authentication required");
}
+
+ // Not allowed to put message if the current patch set is locked.
+ psUtil.checkPatchSetNotLocked(changeNotes, userProvider.get());
try {
permissionBackend
- .user(currentUserProvider.get())
+ .user(userProvider.get())
.database(db.get())
.change(changeNotes)
.check(ChangePermission.ADD_PATCH_SET);
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 767ef7b..02e7c18 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -14,16 +14,12 @@
package com.google.gerrit.server.restapi.change;
-import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
-
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -81,6 +77,7 @@
private final Provider<ReviewDb> dbProvider;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
+ private final PatchSetUtil patchSetUtil;
@Inject
public Rebase(
@@ -91,7 +88,8 @@
ChangeJson.Factory json,
Provider<ReviewDb> dbProvider,
PermissionBackend permissionBackend,
- ProjectCache projectCache) {
+ ProjectCache projectCache,
+ PatchSetUtil patchSetUtil) {
super(retryHelper);
this.repoManager = repoManager;
this.rebaseFactory = rebaseFactory;
@@ -100,13 +98,17 @@
this.dbProvider = dbProvider;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
+ this.patchSetUtil = patchSetUtil;
}
@Override
protected ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, RevisionResource rsrc, RebaseInput input)
- throws EmailException, OrmException, UpdateException, RestApiException, IOException,
- NoSuchChangeException, PermissionBackendException {
+ throws OrmException, UpdateException, RestApiException, IOException,
+ PermissionBackendException {
+ // Not allowed to rebase if the current patch set is locked.
+ patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
+
rsrc.permissions().database(dbProvider).check(ChangePermission.REBASE);
projectCache.checkedGet(rsrc.getProject()).checkStatePermitsWrite();
@@ -203,40 +205,54 @@
}
@Override
- public UiAction.Description getDescription(RevisionResource resource) {
- PatchSet patchSet = resource.getPatchSet();
- Change change = resource.getChange();
- Branch.NameKey dest = change.getDest();
- boolean visible = change.getStatus().isOpen() && resource.isCurrent();
- boolean enabled = false;
+ public UiAction.Description getDescription(RevisionResource rsrc) {
+ UiAction.Description description =
+ new UiAction.Description()
+ .setLabel("Rebase")
+ .setTitle("Rebase onto tip of branch or parent change")
+ .setVisible(false);
+
+ Change change = rsrc.getChange();
+ if (!(change.getStatus().isOpen() && rsrc.isCurrent())) {
+ return description;
+ }
try {
- visible &= projectCache.checkedGet(resource.getProject()).statePermitsWrite();
- } catch (IOException e) {
- log.error("Failed to check if project state permits write: " + resource.getProject(), e);
- visible = false;
- }
-
- if (visible) {
- try (Repository repo = repoManager.openRepository(dest.getParentKey());
- RevWalk rw = new RevWalk(repo)) {
- visible = hasOneParent(rw, resource.getPatchSet());
- if (visible) {
- enabled = rebaseUtil.canRebase(patchSet, dest, repo, rw);
- }
- } catch (IOException e) {
- log.error("Failed to check if patch set can be rebased: " + resource.getPatchSet(), e);
- visible = false;
+ if (!projectCache.checkedGet(rsrc.getProject()).statePermitsWrite()) {
+ return description;
}
+ } catch (IOException e) {
+ log.error("Failed to check if project state permits write: " + rsrc.getProject(), e);
+ return description;
}
- BooleanCondition permissionCond =
- resource.permissions().database(dbProvider).testCond(ChangePermission.REBASE);
- return new UiAction.Description()
- .setLabel("Rebase")
- .setTitle("Rebase onto tip of branch or parent change")
- .setVisible(and(visible, permissionCond))
- .setEnabled(and(enabled, permissionCond));
+ try {
+ if (patchSetUtil.isPatchSetLocked(rsrc.getNotes(), rsrc.getUser())) {
+ return description;
+ }
+ } catch (OrmException | IOException e) {
+ log.error(
+ String.format(
+ "Failed to check if the current patch set of change %s is locked", change.getId()),
+ e);
+ return description;
+ }
+
+ boolean enabled = false;
+ try (Repository repo = repoManager.openRepository(change.getDest().getParentKey());
+ RevWalk rw = new RevWalk(repo)) {
+ if (hasOneParent(rw, rsrc.getPatchSet())) {
+ enabled = rebaseUtil.canRebase(rsrc.getPatchSet(), change.getDest(), repo, rw);
+ }
+ } catch (IOException e) {
+ log.error("Failed to check if patch set can be rebased: " + rsrc.getPatchSet(), e);
+ return description;
+ }
+
+ if (rsrc.permissions().database(dbProvider).testOrFalse(ChangePermission.REBASE)) {
+ return description.setVisible(true).setEnabled(enabled);
+ }
+ return description;
}
public static class CurrentRevision
@@ -254,7 +270,7 @@
@Override
protected ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, RebaseInput input)
- throws EmailException, OrmException, UpdateException, RestApiException, IOException,
+ throws OrmException, UpdateException, RestApiException, IOException,
PermissionBackendException {
PatchSet ps = psUtil.current(rebase.dbProvider.get(), rsrc.getNotes());
if (ps == null) {
diff --git a/java/com/google/gerrit/server/restapi/change/Restore.java b/java/com/google/gerrit/server/restapi/change/Restore.java
index 642c35a..d5bfea1 100644
--- a/java/com/google/gerrit/server/restapi/change/Restore.java
+++ b/java/com/google/gerrit/server/restapi/change/Restore.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.restapi.change;
-import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
-
import com.google.common.base.Strings;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.api.changes.RestoreInput;
@@ -90,17 +88,20 @@
@Override
protected ChangeInfo applyImpl(
- BatchUpdate.Factory updateFactory, ChangeResource req, RestoreInput input)
+ BatchUpdate.Factory updateFactory, ChangeResource rsrc, RestoreInput input)
throws RestApiException, UpdateException, OrmException, PermissionBackendException,
IOException {
- req.permissions().database(dbProvider).check(ChangePermission.RESTORE);
- projectCache.checkedGet(req.getProject()).checkStatePermitsWrite();
+ // Not allowed to restore if the current patch set is locked.
+ psUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
+
+ rsrc.permissions().database(dbProvider).check(ChangePermission.RESTORE);
+ projectCache.checkedGet(rsrc.getProject()).checkStatePermitsWrite();
Op op = new Op(input);
try (BatchUpdate u =
updateFactory.create(
- dbProvider.get(), req.getChange().getProject(), req.getUser(), TimeUtil.nowTs())) {
- u.addOp(req.getId(), op).execute();
+ dbProvider.get(), rsrc.getChange().getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
+ u.addOp(rsrc.getId(), op).execute();
}
return json.noOptions().format(op.change);
}
@@ -161,18 +162,39 @@
@Override
public UiAction.Description getDescription(ChangeResource rsrc) {
- boolean projectStatePermitsWrite = false;
+ UiAction.Description description =
+ new UiAction.Description()
+ .setLabel("Restore")
+ .setTitle("Restore the change")
+ .setVisible(false);
+
+ Change change = rsrc.getChange();
+ if (change.getStatus() != Status.ABANDONED) {
+ return description;
+ }
+
try {
- projectStatePermitsWrite = projectCache.checkedGet(rsrc.getProject()).statePermitsWrite();
+ if (!projectCache.checkedGet(rsrc.getProject()).statePermitsWrite()) {
+ return description;
+ }
} catch (IOException e) {
log.error("Failed to check if project state permits write: " + rsrc.getProject(), e);
+ return description;
}
- return new UiAction.Description()
- .setLabel("Restore")
- .setTitle("Restore the change")
- .setVisible(
- and(
- rsrc.getChange().getStatus() == Status.ABANDONED && projectStatePermitsWrite,
- rsrc.permissions().database(dbProvider).testCond(ChangePermission.RESTORE)));
+
+ try {
+ if (psUtil.isPatchSetLocked(rsrc.getNotes(), rsrc.getUser())) {
+ return description;
+ }
+ } catch (OrmException | IOException e) {
+ log.error(
+ String.format(
+ "Failed to check if the current patch set of change %s is locked", change.getId()),
+ e);
+ return description;
+ }
+
+ boolean visible = rsrc.permissions().database(dbProvider).testOrFalse(ChangePermission.RESTORE);
+ return description.setVisible(visible);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
index 93ad2fe..be0879a 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
@@ -229,8 +229,9 @@
grant(project, "refs/heads/*", Permission.LABEL + "Patch-Set-Lock");
revision(r).review(new ReviewInput().label("Patch-Set-Lock", 1));
- exception.expect(AuthException.class);
- exception.expectMessage("move not permitted");
+ exception.expect(ResourceConflictException.class);
+ exception.expectMessage(
+ String.format("The current patch set of change %s is locked", r.getChange().getId()));
move(r.getChangeId(), newBranch.get());
}
