PatchSetInserter: Don't set or expose PatchSet before insertion
Callers only really ever needed the patch set ID, so expose that
directly. There is no need to set the PatchSet externally, as we
already have setters for the appropriate subfields, like uploader.
Change-Id: Ib0f8182ccf520d399f12730568e3b4be59cc952c
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
index 39d2ff0..934a0da 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
@@ -345,17 +345,17 @@
+ " is merged");
return;
}
- checkMergedBitMatchesStatus(currPs, currPsCommit, merged);
+ checkMergedBitMatchesStatus(currPs.getId(), currPsCommit, merged);
}
}
- private void checkMergedBitMatchesStatus(PatchSet ps, RevCommit commit,
+ private void checkMergedBitMatchesStatus(PatchSet.Id psId, RevCommit commit,
boolean merged) {
String refName = change.getDest().get();
if (merged && change.getStatus() != Change.Status.MERGED) {
ProblemInfo p = problem(String.format(
"Patch set %d (%s) is merged into destination ref %s (%s), but change"
- + " status is %s", ps.getId().get(), commit.name(),
+ + " status is %s", psId.get(), commit.name(),
refName, tip.name(), change.getStatus()));
if (fix != null) {
fixMerged(p);
@@ -416,9 +416,9 @@
commit.name(), changeId, change.getKey().get()));
return;
}
- PatchSet ps = insertPatchSet(commit);
- if (ps != null) {
- checkMergedBitMatchesStatus(ps, commit, true);
+ PatchSet.Id psId = insertPatchSet(commit);
+ if (psId != null) {
+ checkMergedBitMatchesStatus(psId, commit, true);
}
break;
@@ -447,7 +447,7 @@
}
}
- private PatchSet insertPatchSet(RevCommit commit) {
+ private PatchSet.Id insertPatchSet(RevCommit commit) {
ProblemInfo p =
problem("No patch set found for merged commit " + commit.name());
if (!user.get().isIdentifiedUser()) {
@@ -470,9 +470,10 @@
.setMessage(
"Patch set for merged commit inserted by consistency checker")
.insert();
+ PatchSet.Id psId = change.currentPatchSetId();
p.status = Status.FIXED;
- p.outcome = "Inserted as patch set " + change.currentPatchSetId().get();
- return inserter.getPatchSet();
+ p.outcome = "Inserted as patch set " + psId.get();
+ return psId;
} catch (InvalidChangeOperationException | IOException
| NoSuchChangeException | UpdateException | RestApiException e) {
warn(e);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index e728237..cada143 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -14,9 +14,9 @@
package com.google.gerrit.server.change;
+import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.SetMultimap;
import com.google.gerrit.common.ChangeHooks;
@@ -37,10 +37,10 @@
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.BanCommit;
import com.google.gerrit.server.git.BatchUpdate;
-import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.BatchUpdate.Context;
import com.google.gerrit.server.git.BatchUpdate.RepoContext;
+import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
@@ -103,7 +103,7 @@
private final Repository git;
private final RevWalk revWalk;
- private PatchSet patchSet;
+ private PatchSet.Id psId;
private String message;
private SshInfo sshInfo;
private ValidatePolicy validatePolicy = ValidatePolicy.GERRIT;
@@ -152,27 +152,12 @@
return (IdentifiedUser) ctl.getCurrentUser();
}
- public PatchSetInserter setPatchSet(PatchSet patchSet) {
- Change c = ctl.getChange();
- PatchSet.Id psid = patchSet.getId();
- checkArgument(psid.getParentKey().equals(c.getId()),
- "patch set %s not for change %s", psid, c.getId());
- checkArgument(psid.get() > c.currentPatchSetId().get(),
- "new patch set ID %s is not greater than current patch set ID %s",
- psid.get(), c.currentPatchSetId().get());
- this.patchSet = patchSet;
- return this;
- }
-
public PatchSet.Id getPatchSetId() throws IOException {
- init();
- return patchSet.getId();
- }
-
- public PatchSet getPatchSet() {
- checkState(patchSet != null,
- "getPatchSet() only valid after patch set is created");
- return patchSet;
+ if (psId == null) {
+ psId =
+ ChangeUtil.nextPatchSetId(git, ctl.getChange().currentPatchSetId());
+ }
+ return psId;
}
public PatchSetInserter setMessage(String message) {
@@ -227,7 +212,7 @@
Op op = new Op();
try (BatchUpdate bu = batchUpdateFactory.create(
- db, ctl.getChange().getProject(), patchSet.getCreatedOn())) {
+ db, ctl.getChange().getProject(), TimeUtil.nowTs())) {
bu.addOp(ctl, op);
bu.execute();
}
@@ -236,13 +221,14 @@
private class Op extends BatchUpdate.Op {
private Change change;
+ private PatchSet patchSet;
private ChangeMessage changeMessage;
private SetMultimap<ReviewerState, Account.Id> oldReviewers;
@Override
public void updateRepo(RepoContext ctx) throws IOException {
ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(),
- commit, patchSet.getRefName(), ReceiveCommand.Type.CREATE));
+ commit, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE));
}
@Override
@@ -256,6 +242,12 @@
"Change %s is closed", change.getId()));
}
+ patchSet = new PatchSet(psId);
+ patchSet.setCreatedOn(ctx.getWhen());
+ patchSet.setUploader(firstNonNull(uploader, ctl.getChange().getOwner()));
+ patchSet.setRevision(new RevId(commit.name()));
+ patchSet.setDraft(draft);
+
ChangeUtil.insertAncestors(db, patchSet.getId(), commit);
if (groups != null) {
patchSet.setGroups(groups);
@@ -290,8 +282,7 @@
if (change.getStatus() != Change.Status.DRAFT && !allowClosed) {
change.setStatus(Change.Status.NEW);
}
- change.setCurrentPatchSet(patchSetInfoFactory.get(commit,
- patchSet.getId()));
+ change.setCurrentPatchSet(patchSetInfoFactory.get(commit, psId));
ChangeUtil.updated(change);
return change;
}
@@ -311,8 +302,7 @@
public void postUpdate(Context ctx) throws OrmException {
if (sendMail) {
try {
- PatchSetInfo info =
- patchSetInfoFactory.get(commit, patchSet.getId());
+ PatchSetInfo info = patchSetInfoFactory.get(commit, psId);
ReplacePatchSetSender cm = replacePatchSetFactory.create(
change.getId());
cm.setFrom(user.getAccountId());
@@ -333,28 +323,17 @@
}
}
- private void init() throws IOException {
+ private void init() {
if (sshInfo == null) {
sshInfo = new NoSshInfo();
}
- if (patchSet == null) {
- patchSet = new PatchSet(
- ChangeUtil.nextPatchSetId(git, ctl.getChange().currentPatchSetId()));
- patchSet.setCreatedOn(TimeUtil.nowTs());
- patchSet.setUploader(ctl.getChange().getOwner());
- patchSet.setRevision(new RevId(commit.name()));
- }
- patchSet.setDraft(draft);
- if (uploader != null) {
- patchSet.setUploader(uploader);
- }
}
private void validate() throws InvalidChangeOperationException, IOException {
CommitValidators cv =
commitValidatorsFactory.create(ctl.getRefControl(), sshInfo, git);
- String refName = patchSet.getRefName();
+ String refName = getPatchSetId().toRefName();
CommitReceivedEvent event = new CommitReceivedEvent(
new ReceiveCommand(
ObjectId.zeroId(),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
index c465aad..94f64f7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
@@ -17,7 +17,6 @@
import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.base.Optional;
-import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -25,9 +24,7 @@
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.ChangeKind;
@@ -39,6 +36,8 @@
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
+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;
@@ -67,6 +66,7 @@
private final PatchSetInserter.Factory patchSetInserterFactory;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeIndexer indexer;
+ private final ProjectCache projectCache;
private final Provider<ReviewDb> db;
private final Provider<CurrentUser> user;
private final ChangeKindCache changeKindCache;
@@ -76,6 +76,7 @@
PatchSetInserter.Factory patchSetInserterFactory,
ChangeControl.GenericFactory changeControlFactory,
ChangeIndexer indexer,
+ ProjectCache projectCache,
Provider<ReviewDb> db,
Provider<CurrentUser> user,
ChangeKindCache changeKindCache) {
@@ -83,6 +84,7 @@
this.patchSetInserterFactory = patchSetInserterFactory;
this.changeControlFactory = changeControlFactory;
this.indexer = indexer;
+ this.projectCache = projectCache;
this.db = db;
this.user = user;
this.changeKindCache = changeKindCache;
@@ -216,17 +218,19 @@
Repository repo, RevWalk rw, PatchSet basePatchSet, RevCommit squashed)
throws NoSuchChangeException, RestApiException, UpdateException,
InvalidChangeOperationException, IOException {
- PatchSet ps = new PatchSet(
- ChangeUtil.nextPatchSetId(change.currentPatchSetId()));
- ps.setRevision(new RevId(ObjectId.toString(squashed)));
- ps.setUploader(edit.getUser().getAccountId());
- ps.setCreatedOn(TimeUtil.nowTs());
+ PatchSetInserter inserter =
+ patchSetInserterFactory.create(repo, rw,
+ changeControlFactory.controlFor(change, edit.getUser()),
+ squashed);
StringBuilder message = new StringBuilder("Patch set ")
- .append(ps.getPatchSetId())
+ .append(inserter.getPatchSetId().get())
.append(": ");
- ChangeKind kind = changeKindCache.getChangeKind(db.get(), change, ps);
+ ProjectState project = projectCache.get(change.getDest().getParentKey());
+ // Previously checked that the base patch set is the current patch set.
+ ObjectId prior = ObjectId.fromString(basePatchSet.getRevision().get());
+ ChangeKind kind = changeKindCache.getChangeKind(project, repo, prior, squashed);
if (kind == ChangeKind.NO_CODE_CHANGE) {
message.append("Commit message was updated.");
} else {
@@ -235,11 +239,7 @@
.append(".");
}
- PatchSetInserter inserter =
- patchSetInserterFactory.create(repo, rw,
- changeControlFactory.controlFor(change, edit.getUser()),
- squashed);
- return inserter.setPatchSet(ps)
+ return inserter
.setDraft(change.getStatus() == Status.DRAFT ||
basePatchSet.isDraft())
.setMessage(message.toString())
