From 4c58e1031f0ba7cf7c455b36ef2db6d4dc27fe21 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 14 Feb 2019 13:15:41 +0900 Subject: [PATCH] Change: Add helper methods to check change state To check a change's state the caller currently has to call getState() and then do a comparison on the enum value. To make this easier, introduce new methods on Change: - isNew - isMerged - isAbandoned - isClosed Note that we don't add "isOpen" since there are no longer multiple states that could be considered "open". Previously there were "new" and "draft", but the draft workflow was removed. Update callers to use the new methods. Change-Id: Ic047f74b114e2277e66c878dace73ea8e0babab6 --- .../elasticsearch/ElasticChangeIndex.java | 2 +- .../gerrit/lucene/LuceneChangeIndex.java | 2 +- .../google/gerrit/reviewdb/client/Change.java | 16 ++++++++++++++++ .../google/gerrit/server/PatchSetUtil.java | 2 +- .../gerrit/server/change/AbandonOp.java | 2 +- .../gerrit/server/change/ActionJson.java | 4 +--- .../gerrit/server/change/ChangeJson.java | 6 +++--- .../server/change/ConsistencyChecker.java | 4 ++-- .../gerrit/server/change/LabelsJson.java | 10 +++++----- .../server/change/PatchSetInserter.java | 2 +- .../gerrit/server/change/RebaseChangeOp.java | 5 ++--- .../gerrit/server/change/RebaseUtil.java | 5 ++--- .../server/edit/ChangeEditModifier.java | 2 +- .../gerrit/server/events/EventFactory.java | 2 +- .../gerrit/server/git/MergedByPushOp.java | 3 +-- .../server/git/receive/ReceiveCommits.java | 4 ++-- .../gerrit/server/git/receive/ReplaceOp.java | 2 +- .../server/permissions/ChangeControl.java | 4 ++-- .../server/project/RemoveReviewerControl.java | 2 +- .../server/project/SubmitRuleEvaluator.java | 2 +- .../server/query/change/ChangeData.java | 6 +++--- .../query/change/InternalChangeQuery.java | 2 +- .../gerrit/server/restapi/change/Abandon.java | 2 +- .../restapi/change/CherryPickChange.java | 7 +++---- .../server/restapi/change/DeleteChange.java | 10 +++++----- .../server/restapi/change/DeleteChangeOp.java | 3 +-- .../server/restapi/change/Mergeable.java | 2 +- .../gerrit/server/restapi/change/Move.java | 7 +++---- .../server/restapi/change/PostPrivate.java | 2 +- .../server/restapi/change/PostReview.java | 6 +++--- .../server/restapi/change/PreviewSubmit.java | 2 +- .../gerrit/server/restapi/change/Rebase.java | 7 +++---- .../gerrit/server/restapi/change/Restore.java | 4 ++-- .../gerrit/server/restapi/change/Revert.java | 4 ++-- .../restapi/change/SetReadyForReview.java | 5 ++--- .../restapi/change/SetWorkInProgress.java | 5 ++--- .../gerrit/server/restapi/change/Submit.java | 19 ++++++++----------- .../restapi/change/SubmittedTogether.java | 5 ++--- .../server/rules/PrologRuleEvaluator.java | 2 +- .../google/gerrit/server/submit/MergeOp.java | 7 +++---- .../gerrit/server/submit/RebaseSorter.java | 4 +--- .../server/submit/SubmitStrategyOp.java | 2 +- .../acceptance/git/AbstractPushForReview.java | 6 +++--- .../gerrit/acceptance/git/SubmitOnPushIT.java | 14 +++++++------- .../rest/account/ImpersonationIT.java | 3 +-- .../server/change/ConsistencyCheckerIT.java | 10 +++++----- .../converter/ChangeProtoConverterTest.java | 2 +- plugins/reviewnotes | 2 +- 48 files changed, 114 insertions(+), 117 deletions(-) diff --git a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java index 69dde395f5..344e10c6d0 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java +++ b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java @@ -112,7 +112,7 @@ class ElasticChangeIndex extends AbstractElasticIndex String insertIndex; try { - if (cd.change().getStatus().isOpen()) { + if (cd.change().isNew()) { insertIndex = OPEN_CHANGES; deleteIndex = CLOSED_CHANGES; } else { diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index a3394c3fa6..381adcbf07 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -207,7 +207,7 @@ public class LuceneChangeIndex implements ChangeIndex { // sub-index, so just pick one. Document doc = openIndex.toDocument(cd); try { - if (cd.change().getStatus().isOpen()) { + if (cd.change().isNew()) { Futures.allAsList(closedIndex.delete(id), openIndex.replace(id, doc)).get(); } else { Futures.allAsList(openIndex.delete(id), closedIndex.replace(id, doc)).get(); diff --git a/java/com/google/gerrit/reviewdb/client/Change.java b/java/com/google/gerrit/reviewdb/client/Change.java index a8a3304fd1..c1443f2ef2 100644 --- a/java/com/google/gerrit/reviewdb/client/Change.java +++ b/java/com/google/gerrit/reviewdb/client/Change.java @@ -671,6 +671,22 @@ public final class Change { status = newStatus.getCode(); } + public boolean isNew() { + return getStatus().equals(Status.NEW); + } + + public boolean isMerged() { + return getStatus().equals(Status.MERGED); + } + + public boolean isAbandoned() { + return getStatus().equals(Status.ABANDONED); + } + + public boolean isClosed() { + return isAbandoned() || isMerged(); + } + public String getTopic() { return topic; } diff --git a/java/com/google/gerrit/server/PatchSetUtil.java b/java/com/google/gerrit/server/PatchSetUtil.java index 744d199c04..e7f1d2a3eb 100644 --- a/java/com/google/gerrit/server/PatchSetUtil.java +++ b/java/com/google/gerrit/server/PatchSetUtil.java @@ -145,7 +145,7 @@ public class PatchSetUtil { /** Is the current patch set locked against state changes? */ public boolean isPatchSetLocked(ChangeNotes notes) throws OrmException, IOException { Change change = notes.getChange(); - if (change.getStatus() == Change.Status.MERGED) { + if (change.isMerged()) { return false; } diff --git a/java/com/google/gerrit/server/change/AbandonOp.java b/java/com/google/gerrit/server/change/AbandonOp.java index a43690c25d..458a680a92 100644 --- a/java/com/google/gerrit/server/change/AbandonOp.java +++ b/java/com/google/gerrit/server/change/AbandonOp.java @@ -83,7 +83,7 @@ public class AbandonOp implements BatchUpdateOp { change = ctx.getChange(); PatchSet.Id psId = change.currentPatchSetId(); ChangeUpdate update = ctx.getUpdate(psId); - if (!change.getStatus().isOpen()) { + if (!change.isNew()) { throw new ResourceConflictException("change is " + ChangeUtil.status(change)); } patchSet = psUtil.get(ctx.getNotes(), psId); diff --git a/java/com/google/gerrit/server/change/ActionJson.java b/java/com/google/gerrit/server/change/ActionJson.java index 0a9fe81107..52c0451bcd 100644 --- a/java/com/google/gerrit/server/change/ActionJson.java +++ b/java/com/google/gerrit/server/change/ActionJson.java @@ -29,7 +29,6 @@ import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.extensions.webui.PrivateInternals_UiActionDescription; import com.google.gerrit.extensions.webui.UiAction; -import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.extensions.webui.UiActions; import com.google.gerrit.server.notedb.ChangeNotes; @@ -180,8 +179,7 @@ public class ActionJson { // The followup action is a client-side only operation that does not // have a server side handler. It must be manually registered into the // resulting action map. - Status status = notes.getChange().getStatus(); - if (status.isOpen() || status.equals(Status.MERGED)) { + if (!notes.getChange().isAbandoned()) { UiAction.Description descr = new UiAction.Description(); PrivateInternals_UiActionDescription.setId(descr, "followup"); PrivateInternals_UiActionDescription.setMethod(descr, "POST"); diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index fa38139492..f9e015e64a 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -510,7 +510,7 @@ public class ChangeJson { out.assignee = in.getAssignee() != null ? accountLoader.get(in.getAssignee()) : null; out.hashtags = cd.hashtags(); out.changeId = in.getKey().get(); - if (in.getStatus().isOpen()) { + if (in.isNew()) { SubmitTypeRecord str = cd.submitTypeRecord(); if (str.isOk()) { out.submitType = str.type; @@ -547,7 +547,7 @@ public class ChangeJson { } } - if (in.getStatus().isOpen() && has(REVIEWED) && user.isIdentifiedUser()) { + if (in.isNew() && has(REVIEWED) && user.isIdentifiedUser()) { out.reviewed = cd.isReviewedBy(user.getAccountId()) ? true : null; } @@ -560,7 +560,7 @@ public class ChangeJson { if (user.isIdentifiedUser() && (!limitToPsId.isPresent() || limitToPsId.get().equals(in.currentPatchSetId()))) { out.permittedLabels = - cd.change().getStatus() != Change.Status.ABANDONED + !cd.change().isAbandoned() ? labelsJson.permittedLabels(user.getAccountId(), cd) : ImmutableMap.of(); } diff --git a/java/com/google/gerrit/server/change/ConsistencyChecker.java b/java/com/google/gerrit/server/change/ConsistencyChecker.java index 7b911c4c67..ded791206c 100644 --- a/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -368,12 +368,12 @@ public class ConsistencyChecker { private void checkMergedBitMatchesStatus(PatchSet.Id psId, RevCommit commit, boolean merged) { String refName = change().getDest().get(); - if (merged && change().getStatus() != Change.Status.MERGED) { + if (merged && !change().isMerged()) { ProblemInfo p = wrongChangeStatus(psId, commit); if (fix != null) { fixMerged(p); } - } else if (!merged && change().getStatus() == Change.Status.MERGED) { + } else if (!merged && change().isMerged()) { problem( String.format( "Patch set %d (%s) is not merged into" diff --git a/java/com/google/gerrit/server/change/LabelsJson.java b/java/com/google/gerrit/server/change/LabelsJson.java index d76cadc49b..16d3ad1e51 100644 --- a/java/com/google/gerrit/server/change/LabelsJson.java +++ b/java/com/google/gerrit/server/change/LabelsJson.java @@ -41,7 +41,6 @@ import com.google.gerrit.extensions.common.ApprovalInfo; import com.google.gerrit.extensions.common.LabelInfo; import com.google.gerrit.extensions.common.VotingRangeInfo; 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.server.ApprovalsUtil; import com.google.gerrit.server.account.AccountLoader; @@ -107,7 +106,7 @@ public class LabelsJson { LabelTypes labelTypes = cd.getLabelTypes(); Map withStatus = - cd.change().getStatus() == Change.Status.MERGED + cd.change().isMerged() ? labelsForSubmittedChange(accountLoader, cd, labelTypes, standard, detailed) : labelsForUnsubmittedChange(accountLoader, cd, labelTypes, standard, detailed); return ImmutableMap.copyOf(Maps.transformValues(withStatus, LabelWithStatus::label)); @@ -116,7 +115,7 @@ public class LabelsJson { /** Returns all labels that the provided user has permission to vote on. */ Map> permittedLabels(Account.Id filterApprovalsBy, ChangeData cd) throws OrmException, PermissionBackendException { - boolean isMerged = cd.change().getStatus() == Change.Status.MERGED; + boolean isMerged = cd.change().isMerged(); LabelTypes labelTypes = cd.getLabelTypes(); Map toCheck = new HashMap<>(); for (SubmitRecord rec : submitRecords(cd)) { @@ -434,9 +433,10 @@ public class LabelsJson { private void setAllApprovals( AccountLoader accountLoader, ChangeData cd, Map labels) throws OrmException, PermissionBackendException { - Change.Status status = cd.change().getStatus(); checkState( - status != Change.Status.MERGED, "should not call setAllApprovals on %s change", status); + !cd.change().isMerged(), + "should not call setAllApprovals on %s change", + cd.change().getStatus()); // Include a user in the output for this label if either: // - They are an explicit reviewer. diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java index f62d943af1..be32616a25 100644 --- a/java/com/google/gerrit/server/change/PatchSetInserter.java +++ b/java/com/google/gerrit/server/change/PatchSetInserter.java @@ -194,7 +194,7 @@ public class PatchSetInserter implements BatchUpdateOp { ChangeUpdate update = ctx.getUpdate(psId); update.setSubjectForCommit("Create patch set " + psId.get()); - if (!change.getStatus().isOpen() && !allowClosed) { + if (!change.isNew() && !allowClosed) { throw new ResourceConflictException( String.format( "Cannot create new patch set of change %s because it is %s", diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java index 61bdc76a4e..51aa5880b8 100644 --- a/java/com/google/gerrit/server/change/RebaseChangeOp.java +++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java @@ -19,7 +19,6 @@ import static com.google.common.base.Preconditions.checkState; import com.google.gerrit.extensions.restapi.MergeConflictException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; -import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.ChangeUtil; @@ -194,8 +193,8 @@ public class RebaseChangeOp implements BatchUpdateOp { + " was rebased"); } - if (base != null && base.notes().getChange().getStatus() != Change.Status.MERGED) { - if (base.notes().getChange().getStatus() != Change.Status.MERGED) { + if (base != null && !base.notes().getChange().isMerged()) { + if (!base.notes().getChange().isMerged()) { // Add to end of relation chain for open base change. patchSetInserter.setGroups(base.patchSet().getGroups()); } else { diff --git a/java/com/google/gerrit/server/change/RebaseUtil.java b/java/com/google/gerrit/server/change/RebaseUtil.java index 6cb61c1e45..db68898d51 100644 --- a/java/com/google/gerrit/server/change/RebaseUtil.java +++ b/java/com/google/gerrit/server/change/RebaseUtil.java @@ -22,7 +22,6 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.PatchSetUtil; @@ -164,12 +163,12 @@ public class RebaseUtil { continue; } Change depChange = cd.change(); - if (depChange.getStatus() == Status.ABANDONED) { + if (depChange.isAbandoned()) { throw new ResourceConflictException( "Cannot rebase a change with an abandoned parent: " + depChange.getKey()); } - if (depChange.getStatus().isOpen()) { + if (depChange.isNew()) { if (depPatchSet.getId().equals(depChange.currentPatchSetId())) { throw new ResourceConflictException( "Change is already based on the latest patch set of the dependent change."); diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java index 4d1811de2b..15ebdd1479 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java @@ -397,7 +397,7 @@ public class ChangeEditModifier { } Change c = notes.getChange(); - if (!c.getStatus().isOpen()) { + if (!c.isNew()) { throw new ResourceConflictException( String.format( "change %s is %s", c.getChangeId(), c.getStatus().toString().toLowerCase())); diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java index fc803c8d89..4103ce2a4d 100644 --- a/java/com/google/gerrit/server/events/EventFactory.java +++ b/java/com/google/gerrit/server/events/EventFactory.java @@ -190,7 +190,7 @@ public class EventFactory { */ public void extend(ChangeAttribute a, Change change) { a.lastUpdated = change.getLastUpdatedOn().getTime() / 1000L; - a.open = change.getStatus().isOpen(); + a.open = change.isNew(); } /** diff --git a/java/com/google/gerrit/server/git/MergedByPushOp.java b/java/com/google/gerrit/server/git/MergedByPushOp.java index e63c646555..55bb222a1b 100644 --- a/java/com/google/gerrit/server/git/MergedByPushOp.java +++ b/java/com/google/gerrit/server/git/MergedByPushOp.java @@ -125,8 +125,7 @@ public class MergedByPushOp implements BatchUpdateOp { info = getPatchSetInfo(ctx); ChangeUpdate update = ctx.getUpdate(psId); - Change.Status status = change.getStatus(); - if (status == Change.Status.MERGED) { + if (change.isMerged()) { return true; } change.setCurrentPatchSet(info); diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 39b34df226..8413b6dbae 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -1975,7 +1975,7 @@ class ReceiveCommits { */ private boolean requestReplace( ReceiveCommand cmd, boolean checkMergedInto, Change change, RevCommit newCommit) { - if (change.getStatus().isClosed()) { + if (change.isClosed()) { reject( cmd, changeFormatter.changeClosed( @@ -2712,7 +2712,7 @@ class ReceiveCommits { return false; } - if (change.getStatus().isClosed()) { + if (change.isClosed()) { reject(inputCommand, "change " + ontoChange + " closed"); return false; } else if (revisions.containsKey(newCommit)) { diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java index 84bab4a462..4acde286af 100644 --- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java +++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java @@ -246,7 +246,7 @@ public class ReplaceOp implements BatchUpdateOp { ConfigInvalidException { notes = ctx.getNotes(); Change change = notes.getChange(); - if (change == null || change.getStatus().isClosed()) { + if (change == null || change.isClosed()) { rejectMessage = CHANGE_IS_CLOSED; return false; } diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java index 5e9501c5fe..250ca03ff9 100644 --- a/java/com/google/gerrit/server/permissions/ChangeControl.java +++ b/java/com/google/gerrit/server/permissions/ChangeControl.java @@ -165,7 +165,7 @@ class ChangeControl { /** Can this user edit the topic name? */ private boolean canEditTopicName() { - if (getChange().getStatus().isOpen()) { + if (getChange().isNew()) { return isOwner() // owner (aka creator) of the change can edit topic || refControl.isOwner() // branch owner can edit topic || getProjectControl().isOwner() // project owner can edit topic @@ -178,7 +178,7 @@ class ChangeControl { /** Can this user edit the description? */ private boolean canEditDescription() { - if (getChange().getStatus().isOpen()) { + if (getChange().isNew()) { return isOwner() // owner (aka creator) of the change can edit desc || refControl.isOwner() // branch owner can edit desc || getProjectControl().isOwner() // project owner can edit desc diff --git a/java/com/google/gerrit/server/project/RemoveReviewerControl.java b/java/com/google/gerrit/server/project/RemoveReviewerControl.java index f7cf8b69eb..7353ce8dfa 100644 --- a/java/com/google/gerrit/server/project/RemoveReviewerControl.java +++ b/java/com/google/gerrit/server/project/RemoveReviewerControl.java @@ -92,7 +92,7 @@ public class RemoveReviewerControl { Account.Id reviewer, int value) throws PermissionBackendException { - if (change.getStatus().equals(Change.Status.MERGED)) { + if (change.isMerged()) { return false; } diff --git a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java index 7150fae3c0..1afa426a11 100644 --- a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java +++ b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java @@ -102,7 +102,7 @@ public class SubmitRuleEvaluator { return ruleError("Error looking up change " + cd.getId(), e); } - if (!opts.allowClosed() && change.getStatus().isClosed()) { + if (!opts.allowClosed() && change.isClosed()) { SubmitRecord rec = new SubmitRecord(); rec.status = SubmitRecord.Status.CLOSED; return Collections.singletonList(rec); diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java index 787980c93d..b058166040 100644 --- a/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/java/com/google/gerrit/server/query/change/ChangeData.java @@ -173,7 +173,7 @@ public class ChangeData { throws OrmException { List pending = new ArrayList<>(); for (ChangeData cd : changes) { - if (cd.reviewedBy == null && cd.change().getStatus().isOpen()) { + if (cd.reviewedBy == null && cd.change().isNew()) { pending.add(cd); } } @@ -886,9 +886,9 @@ public class ChangeData { if (c == null) { return null; } - if (c.getStatus() == Change.Status.MERGED) { + if (c.isMerged()) { mergeable = true; - } else if (c.getStatus() == Change.Status.ABANDONED) { + } else if (c.isAbandoned()) { return null; } else if (c.isWorkInProgress()) { return null; diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java index 973c451e12..6bd9025b31 100644 --- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java +++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java @@ -188,7 +188,7 @@ public class InternalChangeQuery extends InternalQuery { Change c = cn.getChange(); - return c.getDest().equals(branch) && c.getStatus() != Change.Status.MERGED; + return c.getDest().equals(branch) && !c.isMerged(); }); return Lists.transform(notes, n -> changeDataFactory.create(n)); } diff --git a/java/com/google/gerrit/server/restapi/change/Abandon.java b/java/com/google/gerrit/server/restapi/change/Abandon.java index 05de9e420e..15dc9e5edf 100644 --- a/java/com/google/gerrit/server/restapi/change/Abandon.java +++ b/java/com/google/gerrit/server/restapi/change/Abandon.java @@ -137,7 +137,7 @@ public class Abandon extends RetryingRestModifyView applyImpl( BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input) throws RestApiException, UpdateException, PermissionBackendException { - if (!isChangeDeletable(rsrc.getChange().getStatus())) { + if (!isChangeDeletable(rsrc)) { throw new MethodNotAllowedException("delete not permitted"); } rsrc.permissions().check(ChangePermission.DELETE); @@ -66,16 +66,16 @@ public class DeleteChange extends RetryingRestModifyView patchSets) throws ResourceConflictException, MethodNotAllowedException, IOException { - Change.Status status = ctx.getChange().getStatus(); - if (status == Change.Status.MERGED) { + if (ctx.getChange().isMerged()) { throw new MethodNotAllowedException("Deleting merged change " + id + " is not allowed"); } for (PatchSet patchSet : patchSets) { diff --git a/java/com/google/gerrit/server/restapi/change/Mergeable.java b/java/com/google/gerrit/server/restapi/change/Mergeable.java index f88773743e..44413978ee 100644 --- a/java/com/google/gerrit/server/restapi/change/Mergeable.java +++ b/java/com/google/gerrit/server/restapi/change/Mergeable.java @@ -96,7 +96,7 @@ public class Mergeable implements RestReadView { PatchSet ps = resource.getPatchSet(); MergeableInfo result = new MergeableInfo(); - if (!change.getStatus().isOpen()) { + if (!change.isNew()) { throw new ResourceConflictException("change is " + ChangeUtil.status(change)); } else if (!ps.getId().equals(change.currentPatchSetId())) { // Only the current revision is mergeable. Others always fail. diff --git a/java/com/google/gerrit/server/restapi/change/Move.java b/java/com/google/gerrit/server/restapi/change/Move.java index 2932e5ca19..36f91bbb63 100644 --- a/java/com/google/gerrit/server/restapi/change/Move.java +++ b/java/com/google/gerrit/server/restapi/change/Move.java @@ -33,7 +33,6 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Branch; 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; @@ -133,7 +132,7 @@ public class Move extends RetryingRestModifyView ups, List del) throws ResourceConflictException { - if (ctx.getChange().getStatus().isOpen()) { + if (ctx.getChange().isNew()) { return; // Not closed, nothing to validate. } else if (del.isEmpty() && ups.isEmpty()) { return; // No new votes. - } else if (ctx.getChange().getStatus() != Change.Status.MERGED) { + } else if (!ctx.getChange().isMerged()) { throw new ResourceConflictException("change is closed"); } diff --git a/java/com/google/gerrit/server/restapi/change/PreviewSubmit.java b/java/com/google/gerrit/server/restapi/change/PreviewSubmit.java index 794cf6c1c6..08d86da097 100644 --- a/java/com/google/gerrit/server/restapi/change/PreviewSubmit.java +++ b/java/com/google/gerrit/server/restapi/change/PreviewSubmit.java @@ -99,7 +99,7 @@ public class PreviewSubmit implements RestReadView { } Change change = rsrc.getChange(); - if (!change.getStatus().isOpen()) { + if (!change.isNew()) { throw new PreconditionFailedException("change is " + ChangeUtil.status(change)); } if (!rsrc.getUser().isIdentifiedUser()) { diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java index fe319bf311..0c3f6c2fe5 100644 --- a/java/com/google/gerrit/server/restapi/change/Rebase.java +++ b/java/com/google/gerrit/server/restapi/change/Rebase.java @@ -27,7 +27,6 @@ import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.PatchSetUtil; @@ -115,7 +114,7 @@ public class Rebase extends RetryingRestModifyView { List cds; int hidden; - if (c.getStatus().isOpen()) { + if (c.isNew()) { ChangeSet cs = mergeSuperSet.get().completeChangeSet(c, resource.getUser()); cds = ensureRequiredDataIsLoaded(cs.changes().asList()); hidden = cs.nonVisibleChanges().size(); - } else if (c.getStatus().asChangeStatus() == ChangeStatus.MERGED) { + } else if (c.isMerged()) { cds = queryProvider.get().bySubmissionId(c.getSubmissionId()); hidden = 0; } else { diff --git a/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java b/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java index eb188dbd7f..edc26740fe 100644 --- a/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java +++ b/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java @@ -159,7 +159,7 @@ public class PrologRuleEvaluator { return ruleError("Error looking up change " + cd.getId(), e); } - if (!opts.allowClosed() && change.getStatus().isClosed()) { + if (!opts.allowClosed() && change.isClosed()) { SubmitRecord rec = new SubmitRecord(); rec.status = SubmitRecord.Status.CLOSED; return Collections.singletonList(rec); diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 393609604e..d9b882ddab 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -383,9 +383,8 @@ public class MergeOp implements AutoCloseable { !cs.furtherHiddenChanges(), "checkSubmitRulesAndState called for topic with hidden change"); for (ChangeData cd : cs.changes()) { try { - Change.Status status = cd.change().getStatus(); - if (status != Change.Status.NEW) { - if (!(status == Change.Status.MERGED && allowMerged)) { + if (!cd.change().isNew()) { + if (!(cd.change().isMerged() && allowMerged)) { commitStatus.problem( cd.getId(), "Change " + cd.getId() + " is " + cd.change().getStatus().toString().toLowerCase()); @@ -906,7 +905,7 @@ public class MergeOp implements AutoCloseable { @Override public boolean updateChange(ChangeContext ctx) { Change change = ctx.getChange(); - if (!change.getStatus().isOpen()) { + if (!change.isNew()) { return false; } diff --git a/java/com/google/gerrit/server/submit/RebaseSorter.java b/java/com/google/gerrit/server/submit/RebaseSorter.java index 1fca330ed3..ccb08daefd 100644 --- a/java/com/google/gerrit/server/submit/RebaseSorter.java +++ b/java/com/google/gerrit/server/submit/RebaseSorter.java @@ -16,7 +16,6 @@ package com.google.gerrit.server.submit; import com.google.common.flogger.FluentLogger; import com.google.gerrit.reviewdb.client.Branch; -import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; @@ -126,8 +125,7 @@ public class RebaseSorter { // check if the commit associated change is merged in the same branch List changes = queryProvider.get().byCommit(commit); for (ChangeData change : changes) { - if (change.change().getStatus() == Status.MERGED - && change.change().getDest().equals(dest)) { + if (change.change().isMerged() && change.change().getDest().equals(dest)) { logger.atFine().log( "Dependency %s associated with merged change %s.", commit.getName(), change.getId()); return true; diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java index a49ddff78d..1cc0db53e4 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java @@ -215,7 +215,7 @@ abstract class SubmitStrategyOp implements BatchUpdateOp { "%s#updateChange for change %s", getClass().getSimpleName(), toMerge.change().getId()); toMerge.setNotes(ctx.getNotes()); // Update change and notes from ctx. - if (ctx.getChange().getStatus() == Change.Status.MERGED) { + if (ctx.getChange().isMerged()) { // Either another thread won a race, or we are retrying a whole topic submission after one // repo failed with lock failure. if (alreadyMergedCommit == null) { diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 3e26cab001..93aaf1fe2a 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -1783,7 +1783,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { // Added a new patch set and auto-closed the change. cd = byChangeId(id); - assertThat(cd.change().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(cd.change().isMerged()).isTrue(); assertThat(getPatchSetRevisions(cd)) .containsExactlyEntriesIn( ImmutableMap.of(1, ps1Rev, 2, testRepo.getRepository().resolve("HEAD").name())); @@ -1801,7 +1801,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { // Change not updated. cd = byChangeId(id); - assertThat(cd.change().getStatus()).isEqualTo(Change.Status.NEW); + assertThat(cd.change().isNew()).isTrue(); assertThat(getPatchSetRevisions(cd)).containsExactlyEntriesIn(ImmutableMap.of(1, ps1Rev)); } @@ -1851,7 +1851,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { testRepo.reset(ps2Commit); ChangeData cd = byCommit(ps1Commit); - assertThat(cd.change().getStatus()).isEqualTo(Change.Status.NEW); + assertThat(cd.change().isNew()).isTrue(); assertThat(getPatchSetRevisions(cd)) .containsExactlyEntriesIn(ImmutableMap.of(1, ps1Commit.name())); return c.getId(); diff --git a/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java b/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java index 882c0ff84b..f5832745e3 100644 --- a/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java +++ b/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java @@ -160,7 +160,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { RevCommit c = r.getCommit(); PatchSet.Id psId = cd.currentPatchSet().getId(); assertThat(psId.get()).isEqualTo(1); - assertThat(cd.change().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(cd.change().isMerged()).isTrue(); assertSubmitApproval(psId); assertThat(cd.patchSets()).hasSize(1); @@ -184,7 +184,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { assertCommit(project, master); ChangeData cd = Iterables.getOnlyElement(queryProvider.get().byKey(new Change.Key(r.getChangeId()))); - assertThat(cd.change().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(cd.change().isMerged()).isTrue(); RemoteRefUpdate.Status status = pushCommitTo(commit, "refs/for/other"); assertThat(status).isEqualTo(RemoteRefUpdate.Status.OK); @@ -194,7 +194,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { for (ChangeData c : queryProvider.get().byKey(new Change.Key(r.getChangeId()))) { if (c.change().getDest().get().equals(other)) { - assertThat(c.change().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(c.change().isMerged()).isTrue(); } } } @@ -230,7 +230,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { ChangeData cd = r.getChange(); RevCommit c2 = r.getCommit(); - assertThat(cd.change().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(cd.change().isMerged()).isTrue(); PatchSet.Id psId2 = cd.change().currentPatchSetId(); assertThat(psId2.get()).isEqualTo(2); assertCommit(project, "refs/heads/master"); @@ -262,7 +262,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { cd = changeDataFactory.create(project, psId1.getParentKey()); Change c = cd.change(); - assertThat(c.getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(c.isMerged()).isTrue(); assertThat(c.currentPatchSetId()).isEqualTo(psId1); assertThat(cd.patchSets().stream().map(PatchSet::getId).collect(toList())) .containsExactly(psId1, psId2); @@ -301,14 +301,14 @@ public class SubmitOnPushIT extends AbstractDaemonTest { assertPushOk(pushHead(testRepo, "refs/heads/master", false), "refs/heads/master"); ChangeData cd2 = r2.getChange(); - assertThat(cd2.change().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(cd2.change().isMerged()).isTrue(); PatchSet.Id psId2_2 = cd2.change().currentPatchSetId(); assertThat(psId2_2.get()).isEqualTo(2); assertThat(cd2.patchSet(psId2_1).getRevision().get()).isEqualTo(c2_1.name()); assertThat(cd2.patchSet(psId2_2).getRevision().get()).isEqualTo(c2_2.name()); ChangeData cd1 = r1.getChange(); - assertThat(cd1.change().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(cd1.change().isMerged()).isTrue(); PatchSet.Id psId1_2 = cd1.change().currentPatchSetId(); assertThat(psId1_2.get()).isEqualTo(2); assertThat(cd1.patchSet(psId1_1).getRevision().get()).isEqualTo(c1_1.name()); diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java index 2ae706a475..b001d14514 100644 --- a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java @@ -50,7 +50,6 @@ import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.Comment; import com.google.gerrit.reviewdb.client.Patch; @@ -336,7 +335,7 @@ public class ImpersonationIT extends AbstractDaemonTest { gApi.changes().id(changeId).current().submit(in); ChangeData cd = r.getChange(); - assertThat(cd.change().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(cd.change().isMerged()).isTrue(); PatchSetApproval submitter = approvalsUtil.getSubmitter(cd.notes(), cd.change().currentPatchSetId()); assertThat(submitter.getAccountId()).isEqualTo(admin2.id); diff --git a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index 2ee2d46535..c80cd217c5 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -379,7 +379,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { "Marked change as merged")); notes = reload(notes); - assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(notes.getChange().isMerged()).isTrue(); assertNoProblems(notes, null); } @@ -422,7 +422,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { "Marked change as merged")); notes = reload(notes); - assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(notes.getChange().isMerged()).isTrue(); assertNoProblems(notes, null); } @@ -572,7 +572,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { notes = reload(notes); PatchSet.Id psId3 = new PatchSet.Id(notes.getChangeId(), 3); assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId3); - assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(notes.getChange().isMerged()).isTrue(); assertThat(psUtil.byChangeAsMap(notes).keySet()).containsExactly(ps2.getId(), psId3); assertThat(psUtil.get(notes, psId3).getRevision().get()).isEqualTo(rev1); } @@ -620,7 +620,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { notes = reload(notes); PatchSet.Id psId4 = new PatchSet.Id(notes.getChangeId(), 4); assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId4); - assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(notes.getChange().isMerged()).isTrue(); assertThat(psUtil.byChangeAsMap(notes).keySet()) .containsExactly(ps1.getId(), ps3.getId(), psId4); assertThat(psUtil.get(notes, psId4).getRevision().get()).isEqualTo(rev2); @@ -657,7 +657,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { notes = reload(notes); assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId2); - assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertThat(notes.getChange().isMerged()).isTrue(); assertThat(psUtil.byChangeAsMap(notes).keySet()).containsExactly(ps1.getId(), psId2); assertThat(psUtil.get(notes, psId2).getRevision().get()).isEqualTo(rev2); } diff --git a/javatests/com/google/gerrit/reviewdb/converter/ChangeProtoConverterTest.java b/javatests/com/google/gerrit/reviewdb/converter/ChangeProtoConverterTest.java index 30487a658d..61bf105d8b 100644 --- a/javatests/com/google/gerrit/reviewdb/converter/ChangeProtoConverterTest.java +++ b/javatests/com/google/gerrit/reviewdb/converter/ChangeProtoConverterTest.java @@ -253,7 +253,7 @@ public class ChangeProtoConverterTest { assertThat(change.currentPatchSetId()).isNull(); // Default values for unset protobuf fields which can't be unset in the entity object. assertThat(change.getRowVersion()).isEqualTo(0); - assertThat(change.getStatus()).isEqualTo(Change.Status.NEW); + assertThat(change.isNew()).isTrue(); assertThat(change.isPrivate()).isFalse(); assertThat(change.isWorkInProgress()).isFalse(); assertThat(change.hasReviewStarted()).isFalse(); diff --git a/plugins/reviewnotes b/plugins/reviewnotes index a9456bfdb8..d7ba4997b5 160000 --- a/plugins/reviewnotes +++ b/plugins/reviewnotes @@ -1 +1 @@ -Subproject commit a9456bfdb862dfa7197583decac3c22149ae8109 +Subproject commit d7ba4997b5c0038c85f6393b73a9ca16ca4fb927