Don't pass RevCommits to BatchUpdateOp factories
Using a RevCommit with a RevWalk instance other than the exact one that
created it is a source of difficult-to-find bugs. Some of our
BatchUpdateOp implementations for various reasons took RevCommits in
their assisted constructors, even though the RevWalk that produced the
commit was not guaranteed to be the same RevWalk returned by
Context#getRevWalk(). It happens that all callers were already doing
the right thing, namely calling BatchUpdate#setRepository to set the
RevWalk to the same instance that produced the commits, but there was
no guarantee.
Fix the problem more generally by always passing in ObjectIds to
BatchUpdateOps, and parse the IDs to commits only when using the RevWalk
from the context.
Change-Id: I609bf83342101726755978c5e90f619e09cd3fc4
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
index 8321c4e..50a4167 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -75,6 +75,7 @@
import java.util.concurrent.Future;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.eclipse.jgit.util.ChangeIdUtil;
import org.slf4j.Logger;
@@ -82,7 +83,7 @@
public class ChangeInserter implements InsertChangeOp {
public interface Factory {
- ChangeInserter create(Change.Id cid, RevCommit rc, String refName);
+ ChangeInserter create(Change.Id cid, ObjectId commitId, String refName);
}
private static final Logger log = LoggerFactory.getLogger(ChangeInserter.class);
@@ -102,7 +103,7 @@
private final Change.Id changeId;
private final PatchSet.Id psId;
- private final RevCommit commit;
+ private final ObjectId commitId;
private final String refName;
// Fields exposed as setters.
@@ -146,7 +147,7 @@
CommentAdded commentAdded,
RevisionCreated revisionCreated,
@Assisted Change.Id changeId,
- @Assisted RevCommit commit,
+ @Assisted ObjectId commitId,
@Assisted String refName) {
this.projectControlFactory = projectControlFactory;
this.userFactory = userFactory;
@@ -163,7 +164,7 @@
this.changeId = changeId;
this.psId = new PatchSet.Id(changeId, INITIAL_PATCH_SET_ID);
- this.commit = commit;
+ this.commitId = commitId.copy();
this.refName = refName;
this.reviewers = Collections.emptySet();
this.extraCC = Collections.emptySet();
@@ -175,10 +176,10 @@
}
@Override
- public Change createChange(Context ctx) {
+ public Change createChange(Context ctx) throws IOException {
change =
new Change(
- getChangeKey(commit),
+ getChangeKey(ctx.getRevWalk(), commitId),
changeId,
ctx.getAccountId(),
new Branch.NameKey(ctx.getProject(), refName),
@@ -189,29 +190,31 @@
return change;
}
- private static Change.Key getChangeKey(RevCommit commit) {
+ private static Change.Key getChangeKey(RevWalk rw, ObjectId id) throws IOException {
+ RevCommit commit = rw.parseCommit(id);
+ rw.parseBody(commit);
List<String> idList = commit.getFooterLines(FooterConstants.CHANGE_ID);
if (!idList.isEmpty()) {
return new Change.Key(idList.get(idList.size() - 1).trim());
}
- ObjectId id =
+ ObjectId changeId =
ChangeIdUtil.computeChangeId(
commit.getTree(),
commit,
commit.getAuthorIdent(),
commit.getCommitterIdent(),
commit.getShortMessage());
- StringBuilder changeId = new StringBuilder();
- changeId.append("I").append(ObjectId.toString(id));
- return new Change.Key(changeId.toString());
+ StringBuilder changeIdStr = new StringBuilder();
+ changeIdStr.append("I").append(ObjectId.toString(changeId));
+ return new Change.Key(changeIdStr.toString());
}
public PatchSet.Id getPatchSetId() {
return psId;
}
- public RevCommit getCommit() {
- return commit;
+ public ObjectId getCommitId() {
+ return commitId;
}
public Change getChange() {
@@ -338,7 +341,7 @@
return;
}
if (updateRefCommand == null) {
- ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), commit, psId.toRefName()));
+ ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), commitId, psId.toRefName()));
} else {
ctx.addRefUpdate(updateRefCommand);
}
@@ -350,7 +353,8 @@
change = ctx.getChange(); // Use defensive copy created by ChangeControl.
ReviewDb db = ctx.getDb();
ChangeControl ctl = ctx.getControl();
- patchSetInfo = patchSetInfoFactory.get(ctx.getRevWalk(), commit, psId);
+ patchSetInfo =
+ patchSetInfoFactory.get(ctx.getRevWalk(), ctx.getRevWalk().parseCommit(commitId), psId);
ctx.getChange().setCurrentPatchSet(patchSetInfo);
ChangeUpdate update = ctx.getUpdate(psId);
@@ -366,7 +370,7 @@
boolean draft = status == Change.Status.DRAFT;
List<String> newGroups = groups;
if (newGroups.isEmpty()) {
- newGroups = GroupCollector.getDefaultGroups(commit);
+ newGroups = GroupCollector.getDefaultGroups(commitId);
}
patchSet =
psUtil.insert(
@@ -374,7 +378,7 @@
ctx.getRevWalk(),
update,
psId,
- commit,
+ commitId,
draft,
newGroups,
pushCert,
@@ -518,11 +522,11 @@
String refName = psId.toRefName();
try (CommitReceivedEvent event =
new CommitReceivedEvent(
- new ReceiveCommand(ObjectId.zeroId(), commit.getId(), refName),
+ new ReceiveCommand(ObjectId.zeroId(), commitId, refName),
refControl.getProjectControl().getProject(),
change.getDest().get(),
ctx.getRevWalk().getObjectReader(),
- commit,
+ commitId,
ctx.getIdentifiedUser())) {
commitValidatorsFactory
.create(validatePolicy, refControl, new NoSshInfo(), ctx.getRepository())
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
index 8f0d45f..8d0da36 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
@@ -288,7 +288,7 @@
String topic,
Branch.NameKey sourceBranch,
ObjectId sourceCommit)
- throws OrmException {
+ throws OrmException, IOException {
Change.Id changeId = new Change.Id(seq.nextChangeId());
ChangeInserter ins =
changeInserterFactory
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 b5089f6..88e9d42 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
@@ -56,7 +56,6 @@
import java.util.Collections;
import java.util.List;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -65,7 +64,7 @@
private static final Logger log = LoggerFactory.getLogger(PatchSetInserter.class);
public interface Factory {
- PatchSetInserter create(ChangeControl ctl, PatchSet.Id psId, RevCommit commit);
+ PatchSetInserter create(ChangeControl ctl, PatchSet.Id psId, ObjectId commitId);
}
// Injected fields.
@@ -80,7 +79,7 @@
// Assisted-injected fields.
private final PatchSet.Id psId;
- private final RevCommit commit;
+ private final ObjectId commitId;
// Read prior to running the batch update, so must only be used during
// updateRepo; updateChange and later must use the control from the
// ChangeContext.
@@ -118,7 +117,7 @@
RevisionCreated revisionCreated,
@Assisted ChangeControl ctl,
@Assisted PatchSet.Id psId,
- @Assisted RevCommit commit) {
+ @Assisted ObjectId commitId) {
this.approvalsUtil = approvalsUtil;
this.approvalCopier = approvalCopier;
this.cmUtil = cmUtil;
@@ -130,7 +129,7 @@
this.origCtl = ctl;
this.psId = psId;
- this.commit = commit;
+ this.commitId = commitId.copy();
}
public PatchSet.Id getPatchSetId() {
@@ -210,7 +209,7 @@
validate(ctx);
ctx.addRefUpdate(
new ReceiveCommand(
- ObjectId.zeroId(), commit, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE));
+ ObjectId.zeroId(), commitId, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE));
}
@Override
@@ -243,7 +242,7 @@
ctx.getRevWalk(),
ctx.getUpdate(psId),
psId,
- commit,
+ commitId,
draft,
newGroups,
null,
@@ -264,7 +263,8 @@
changeMessage.setMessage(message);
}
- patchSetInfo = patchSetInfoFactory.get(ctx.getRevWalk(), commit, psId);
+ patchSetInfo =
+ patchSetInfoFactory.get(ctx.getRevWalk(), ctx.getRevWalk().parseCommit(commitId), psId);
if (change.getStatus() != Change.Status.DRAFT && !allowClosed) {
change.setStatus(Change.Status.NEW);
}
@@ -315,12 +315,12 @@
new CommitReceivedEvent(
new ReceiveCommand(
ObjectId.zeroId(),
- commit.getId(),
+ commitId,
refName.substring(0, refName.lastIndexOf('/') + 1) + "new"),
origCtl.getProjectControl().getProject(),
origCtl.getRefControl().getRefName(),
ctx.getRevWalk().getObjectReader(),
- commit,
+ commitId,
ctx.getIdentifiedUser())) {
commitValidatorsFactory
.create(validatePolicy, origCtl.getRefControl(), new NoSshInfo(), ctx.getRepository())
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
index 27dea6b..8731dd5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
@@ -83,9 +83,9 @@
Branch.NameKey dest,
boolean checkMergedInto,
@Assisted("priorPatchSetId") PatchSet.Id priorPatchSetId,
- @Assisted("priorCommit") RevCommit priorCommit,
+ @Assisted("priorCommitId") ObjectId priorCommit,
@Assisted("patchSetId") PatchSet.Id patchSetId,
- @Assisted("commit") RevCommit commit,
+ @Assisted("commitId") ObjectId commitId,
PatchSetInfo info,
List<String> groups,
@Nullable MagicBranchInput magicBranch,
@@ -115,9 +115,9 @@
private final Branch.NameKey dest;
private final boolean checkMergedInto;
private final PatchSet.Id priorPatchSetId;
- private final RevCommit priorCommit;
+ private final ObjectId priorCommitId;
private final PatchSet.Id patchSetId;
- private final RevCommit commit;
+ private final ObjectId commitId;
private final PatchSetInfo info;
private final MagicBranchInput magicBranch;
private final PushCertificate pushCertificate;
@@ -125,6 +125,7 @@
private final Map<String, Short> approvals = new HashMap<>();
private final MailRecipients recipients = new MailRecipients();
+ private RevCommit commit;
private Change change;
private PatchSet newPatchSet;
private ChangeKind changeKind;
@@ -154,9 +155,9 @@
@Assisted Branch.NameKey dest,
@Assisted boolean checkMergedInto,
@Assisted("priorPatchSetId") PatchSet.Id priorPatchSetId,
- @Assisted("priorCommit") RevCommit priorCommit,
+ @Assisted("priorCommitId") ObjectId priorCommitId,
@Assisted("patchSetId") PatchSet.Id patchSetId,
- @Assisted("commit") RevCommit commit,
+ @Assisted("commitId") ObjectId commitId,
@Assisted PatchSetInfo info,
@Assisted List<String> groups,
@Assisted @Nullable MagicBranchInput magicBranch,
@@ -180,9 +181,9 @@
this.dest = dest;
this.checkMergedInto = checkMergedInto;
this.priorPatchSetId = priorPatchSetId;
- this.priorCommit = priorCommit;
+ this.priorCommitId = priorCommitId.copy();
this.patchSetId = patchSetId;
- this.commit = commit;
+ this.commitId = commitId.copy();
this.info = info;
this.groups = groups;
this.magicBranch = magicBranch;
@@ -192,9 +193,11 @@
@Override
public void updateRepo(RepoContext ctx) throws Exception {
+ commit = ctx.getRevWalk().parseCommit(commitId);
+ ctx.getRevWalk().parseBody(commit);
changeKind =
changeKindCache.getChangeKind(
- projectControl.getProject().getNameKey(), ctx.getRepository(), priorCommit, commit);
+ projectControl.getProject().getNameKey(), ctx.getRepository(), priorCommitId, commitId);
if (checkMergedInto) {
Ref mergedInto = findMergedInto(ctx, dest.get(), commit);
@@ -205,7 +208,7 @@
}
if (updateRef) {
- ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), commit, patchSetId.toRefName()));
+ ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), commitId, patchSetId.toRefName()));
}
}
@@ -259,7 +262,7 @@
ctx.getRevWalk(),
update,
patchSetId,
- commit,
+ commitId,
draft,
groups,
pushCertificate != null ? pushCertificate.toTextWithSignature() : null,
@@ -383,7 +386,7 @@
List<String> idList = commit.getFooterLines(CHANGE_ID);
if (idList.isEmpty()) {
- change.setKey(new Change.Key("I" + commit.name()));
+ change.setKey(new Change.Key("I" + commitId.name()));
} else {
change.setKey(new Change.Key(idList.get(idList.size() - 1).trim()));
}
@@ -398,7 +401,7 @@
final Account account = ctx.getAccount();
if (!updateRef) {
gitRefUpdated.fire(
- ctx.getProject(), newPatchSet.getRefName(), ObjectId.zeroId(), commit, account);
+ ctx.getProject(), newPatchSet.getRefName(), ObjectId.zeroId(), commitId, account);
}
if (changeKind != ChangeKind.TRIVIAL_REBASE) {
@@ -505,7 +508,7 @@
return this;
}
- private Ref findMergedInto(Context ctx, String first, RevCommit commit) {
+ private static Ref findMergedInto(Context ctx, String first, RevCommit commit) {
try {
RefDatabase refDatabase = ctx.getRepository().getRefDatabase();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
index 4d0939b6..8227684 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -337,7 +337,7 @@
return this;
}
- public BatchUpdate insertChange(InsertChangeOp op) {
+ public BatchUpdate insertChange(InsertChangeOp op) throws IOException {
Context ctx = newContext();
Change c = op.createChange(ctx);
checkArgument(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/InsertChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/InsertChangeOp.java
index 1a947e6..7060059 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/InsertChangeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/InsertChangeOp.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.update;
import com.google.gerrit.reviewdb.client.Change;
+import java.io.IOException;
/**
* Specialization of {@link BatchUpdateOp} for creating changes.
@@ -27,5 +28,5 @@
* first.
*/
public interface InsertChangeOp extends BatchUpdateOp {
- Change createChange(Context ctx);
+ Change createChange(Context ctx) throws IOException;
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index bcd5d39..a921916 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -414,7 +414,7 @@
TestRepository<Repo> repo = createProject("repo");
ChangeInserter ins = newChange(repo);
insert(repo, ins);
- String sha = ins.getCommit().name();
+ String sha = ins.getCommitId().name();
assertQuery("0000000000000000000000000000000000000000");
for (int i = 0; i <= 36; i++) {
@@ -1741,7 +1741,7 @@
if (dest == null) {
dest = ins.getChange().getDest();
}
- shas.add(ins.getCommit().name());
+ shas.add(ins.getCommitId().name());
expectedIds.add(ins.getChange().getId().get());
}
