opendev/gerrit
Code Issues Proposed changes

Don't pass RevCommits to BatchUpdateOp factories

Browse Source 
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
This commit is contained in:
Dave Borowitz
2017-03-23 15:35:46 -07:00
parent aad79ac07e
commit 6e5adaef69
7 changed files with 56 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -75,6 +75,7 @@ import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future; import java.util.concurrent.Future;
import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.transport.ReceiveCommand;
import org.eclipse.jgit.util.ChangeIdUtil; import org.eclipse.jgit.util.ChangeIdUtil;
import org.slf4j.Logger; import org.slf4j.Logger;
@@ -82,7 +83,7 @@ import org.slf4j.LoggerFactory;
public class ChangeInserter implements InsertChangeOp { public class ChangeInserter implements InsertChangeOp {
public interface Factory { 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); private static final Logger log = LoggerFactory.getLogger(ChangeInserter.class);
@@ -102,7 +103,7 @@ public class ChangeInserter implements InsertChangeOp {
private final Change.Id changeId; private final Change.Id changeId;
private final PatchSet.Id psId; private final PatchSet.Id psId;
private final RevCommit commit; private final ObjectId commitId;
private final String refName; private final String refName;
// Fields exposed as setters. // Fields exposed as setters.
@@ -146,7 +147,7 @@ public class ChangeInserter implements InsertChangeOp {
CommentAdded commentAdded, CommentAdded commentAdded,
RevisionCreated revisionCreated, RevisionCreated revisionCreated,
@Assisted Change.Id changeId, @Assisted Change.Id changeId,
@Assisted RevCommit commit, @Assisted ObjectId commitId,
@Assisted String refName) { @Assisted String refName) {
this.projectControlFactory = projectControlFactory; this.projectControlFactory = projectControlFactory;
this.userFactory = userFactory; this.userFactory = userFactory;
@@ -163,7 +164,7 @@ public class ChangeInserter implements InsertChangeOp {
this.changeId = changeId; this.changeId = changeId;
this.psId = new PatchSet.Id(changeId, INITIAL_PATCH_SET_ID); this.psId = new PatchSet.Id(changeId, INITIAL_PATCH_SET_ID);
this.commit = commit; this.commitId = commitId.copy();
this.refName = refName; this.refName = refName;
this.reviewers = Collections.emptySet(); this.reviewers = Collections.emptySet();
this.extraCC = Collections.emptySet(); this.extraCC = Collections.emptySet();
@@ -175,10 +176,10 @@ public class ChangeInserter implements InsertChangeOp {
} }
@Override @Override
public Change createChange(Context ctx) { public Change createChange(Context ctx) throws IOException {
change = change =
new Change( new Change(
getChangeKey(commit), getChangeKey(ctx.getRevWalk(), commitId),
changeId, changeId,
ctx.getAccountId(), ctx.getAccountId(),
new Branch.NameKey(ctx.getProject(), refName), new Branch.NameKey(ctx.getProject(), refName),
@@ -189,29 +190,31 @@ public class ChangeInserter implements InsertChangeOp {
return change; 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); List<String> idList = commit.getFooterLines(FooterConstants.CHANGE_ID);
if (!idList.isEmpty()) { if (!idList.isEmpty()) {
return new Change.Key(idList.get(idList.size() - 1).trim()); return new Change.Key(idList.get(idList.size() - 1).trim());
} }
ObjectId id = ObjectId changeId =
ChangeIdUtil.computeChangeId( ChangeIdUtil.computeChangeId(
commit.getTree(), commit.getTree(),
commit, commit,
commit.getAuthorIdent(), commit.getAuthorIdent(),
commit.getCommitterIdent(), commit.getCommitterIdent(),
commit.getShortMessage()); commit.getShortMessage());
StringBuilder changeId = new StringBuilder(); StringBuilder changeIdStr = new StringBuilder();
changeId.append("I").append(ObjectId.toString(id)); changeIdStr.append("I").append(ObjectId.toString(changeId));
return new Change.Key(changeId.toString()); return new Change.Key(changeIdStr.toString());
} }
public PatchSet.Id getPatchSetId() { public PatchSet.Id getPatchSetId() {
return psId; return psId;
} }
public RevCommit getCommit() { public ObjectId getCommitId() {
return commit; return commitId;
} }
public Change getChange() { public Change getChange() {
@@ -338,7 +341,7 @@ public class ChangeInserter implements InsertChangeOp {
return; return;
} }
if (updateRefCommand == null) { if (updateRefCommand == null) {
ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), commit, psId.toRefName())); ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), commitId, psId.toRefName()));
} else { } else {
ctx.addRefUpdate(updateRefCommand); ctx.addRefUpdate(updateRefCommand);
} }
@@ -350,7 +353,8 @@ public class ChangeInserter implements InsertChangeOp {
change = ctx.getChange(); // Use defensive copy created by ChangeControl. change = ctx.getChange(); // Use defensive copy created by ChangeControl.
ReviewDb db = ctx.getDb(); ReviewDb db = ctx.getDb();
ChangeControl ctl = ctx.getControl(); 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); ctx.getChange().setCurrentPatchSet(patchSetInfo);
ChangeUpdate update = ctx.getUpdate(psId); ChangeUpdate update = ctx.getUpdate(psId);
@@ -366,7 +370,7 @@ public class ChangeInserter implements InsertChangeOp {
boolean draft = status == Change.Status.DRAFT; boolean draft = status == Change.Status.DRAFT;
List<String> newGroups = groups; List<String> newGroups = groups;
if (newGroups.isEmpty()) { if (newGroups.isEmpty()) {
newGroups = GroupCollector.getDefaultGroups(commit); newGroups = GroupCollector.getDefaultGroups(commitId);
} }
patchSet = patchSet =
psUtil.insert( psUtil.insert(
@@ -374,7 +378,7 @@ public class ChangeInserter implements InsertChangeOp {
ctx.getRevWalk(), ctx.getRevWalk(),
update, update,
psId, psId,
commit, commitId,
draft, draft,
newGroups, newGroups,
pushCert, pushCert,
@@ -518,11 +522,11 @@ public class ChangeInserter implements InsertChangeOp {
String refName = psId.toRefName(); String refName = psId.toRefName();
try (CommitReceivedEvent event = try (CommitReceivedEvent event =
new CommitReceivedEvent( new CommitReceivedEvent(
new ReceiveCommand(ObjectId.zeroId(), commit.getId(), refName), new ReceiveCommand(ObjectId.zeroId(), commitId, refName),
refControl.getProjectControl().getProject(), refControl.getProjectControl().getProject(),
change.getDest().get(), change.getDest().get(),
ctx.getRevWalk().getObjectReader(), ctx.getRevWalk().getObjectReader(),
commit, commitId,
ctx.getIdentifiedUser())) { ctx.getIdentifiedUser())) {
commitValidatorsFactory commitValidatorsFactory
.create(validatePolicy, refControl, new NoSshInfo(), ctx.getRepository()) .create(validatePolicy, refControl, new NoSshInfo(), ctx.getRepository())

2
gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
View File

@@ -288,7 +288,7 @@ public class CherryPickChange {
String topic, String topic,
Branch.NameKey sourceBranch, Branch.NameKey sourceBranch,
ObjectId sourceCommit) ObjectId sourceCommit)
throws OrmException { throws OrmException, IOException {
Change.Id changeId = new Change.Id(seq.nextChangeId()); Change.Id changeId = new Change.Id(seq.nextChangeId());
ChangeInserter ins = ChangeInserter ins =
changeInserterFactory changeInserterFactory

20
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -56,7 +56,6 @@ import java.io.IOException;
import java.util.Collections; import java.util.Collections;
import java.util.List; import java.util.List;
import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.transport.ReceiveCommand;
import org.slf4j.Logger; import org.slf4j.Logger;
import org.slf4j.LoggerFactory; import org.slf4j.LoggerFactory;
@@ -65,7 +64,7 @@ public class PatchSetInserter implements BatchUpdateOp {
private static final Logger log = LoggerFactory.getLogger(PatchSetInserter.class); private static final Logger log = LoggerFactory.getLogger(PatchSetInserter.class);
public interface Factory { public interface Factory {
PatchSetInserter create(ChangeControl ctl, PatchSet.Id psId, RevCommit commit); PatchSetInserter create(ChangeControl ctl, PatchSet.Id psId, ObjectId commitId);
} }
// Injected fields. // Injected fields.
@@ -80,7 +79,7 @@ public class PatchSetInserter implements BatchUpdateOp {
// Assisted-injected fields. // Assisted-injected fields.
private final PatchSet.Id psId; 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 // Read prior to running the batch update, so must only be used during
// updateRepo; updateChange and later must use the control from the // updateRepo; updateChange and later must use the control from the
// ChangeContext. // ChangeContext.
@@ -118,7 +117,7 @@ public class PatchSetInserter implements BatchUpdateOp {
RevisionCreated revisionCreated, RevisionCreated revisionCreated,
@Assisted ChangeControl ctl, @Assisted ChangeControl ctl,
@Assisted PatchSet.Id psId, @Assisted PatchSet.Id psId,
@Assisted RevCommit commit) { @Assisted ObjectId commitId) {
this.approvalsUtil = approvalsUtil; this.approvalsUtil = approvalsUtil;
this.approvalCopier = approvalCopier; this.approvalCopier = approvalCopier;
this.cmUtil = cmUtil; this.cmUtil = cmUtil;
@@ -130,7 +129,7 @@ public class PatchSetInserter implements BatchUpdateOp {
this.origCtl = ctl; this.origCtl = ctl;
this.psId = psId; this.psId = psId;
this.commit = commit; this.commitId = commitId.copy();
} }
public PatchSet.Id getPatchSetId() { public PatchSet.Id getPatchSetId() {
@@ -210,7 +209,7 @@ public class PatchSetInserter implements BatchUpdateOp {
validate(ctx); validate(ctx);
ctx.addRefUpdate( ctx.addRefUpdate(
new ReceiveCommand( new ReceiveCommand(
ObjectId.zeroId(), commit, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE)); ObjectId.zeroId(), commitId, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE));
} }
@Override @Override
@@ -243,7 +242,7 @@ public class PatchSetInserter implements BatchUpdateOp {
ctx.getRevWalk(), ctx.getRevWalk(),
ctx.getUpdate(psId), ctx.getUpdate(psId),
psId, psId,
commit, commitId,
draft, draft,
newGroups, newGroups,
null, null,
@@ -264,7 +263,8 @@ public class PatchSetInserter implements BatchUpdateOp {
changeMessage.setMessage(message); 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) { if (change.getStatus() != Change.Status.DRAFT && !allowClosed) {
change.setStatus(Change.Status.NEW); change.setStatus(Change.Status.NEW);
} }
@@ -315,12 +315,12 @@ public class PatchSetInserter implements BatchUpdateOp {
new CommitReceivedEvent( new CommitReceivedEvent(
new ReceiveCommand( new ReceiveCommand(
ObjectId.zeroId(), ObjectId.zeroId(),
commit.getId(), commitId,
refName.substring(0, refName.lastIndexOf('/') + 1) + "new"), refName.substring(0, refName.lastIndexOf('/') + 1) + "new"),
origCtl.getProjectControl().getProject(), origCtl.getProjectControl().getProject(),
origCtl.getRefControl().getRefName(), origCtl.getRefControl().getRefName(),
ctx.getRevWalk().getObjectReader(), ctx.getRevWalk().getObjectReader(),
commit, commitId,
ctx.getIdentifiedUser())) { ctx.getIdentifiedUser())) {
commitValidatorsFactory commitValidatorsFactory
.create(validatePolicy, origCtl.getRefControl(), new NoSshInfo(), ctx.getRepository()) .create(validatePolicy, origCtl.getRefControl(), new NoSshInfo(), ctx.getRepository())

31
gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
View File

@@ -83,9 +83,9 @@ public class ReplaceOp implements BatchUpdateOp {
Branch.NameKey dest, Branch.NameKey dest,
boolean checkMergedInto, boolean checkMergedInto,
@Assisted("priorPatchSetId") PatchSet.Id priorPatchSetId, @Assisted("priorPatchSetId") PatchSet.Id priorPatchSetId,
@Assisted("priorCommit") RevCommit priorCommit, @Assisted("priorCommitId") ObjectId priorCommit,
@Assisted("patchSetId") PatchSet.Id patchSetId, @Assisted("patchSetId") PatchSet.Id patchSetId,
@Assisted("commit") RevCommit commit, @Assisted("commitId") ObjectId commitId,
PatchSetInfo info, PatchSetInfo info,
List<String> groups, List<String> groups,
@Nullable MagicBranchInput magicBranch, @Nullable MagicBranchInput magicBranch,
@@ -115,9 +115,9 @@ public class ReplaceOp implements BatchUpdateOp {
private final Branch.NameKey dest; private final Branch.NameKey dest;
private final boolean checkMergedInto; private final boolean checkMergedInto;
private final PatchSet.Id priorPatchSetId; private final PatchSet.Id priorPatchSetId;
private final RevCommit priorCommit; private final ObjectId priorCommitId;
private final PatchSet.Id patchSetId; private final PatchSet.Id patchSetId;
private final RevCommit commit; private final ObjectId commitId;
private final PatchSetInfo info; private final PatchSetInfo info;
private final MagicBranchInput magicBranch; private final MagicBranchInput magicBranch;
private final PushCertificate pushCertificate; private final PushCertificate pushCertificate;
@@ -125,6 +125,7 @@ public class ReplaceOp implements BatchUpdateOp {
private final Map<String, Short> approvals = new HashMap<>(); private final Map<String, Short> approvals = new HashMap<>();
private final MailRecipients recipients = new MailRecipients(); private final MailRecipients recipients = new MailRecipients();
private RevCommit commit;
private Change change; private Change change;
private PatchSet newPatchSet; private PatchSet newPatchSet;
private ChangeKind changeKind; private ChangeKind changeKind;
@@ -154,9 +155,9 @@ public class ReplaceOp implements BatchUpdateOp {
@Assisted Branch.NameKey dest, @Assisted Branch.NameKey dest,
@Assisted boolean checkMergedInto, @Assisted boolean checkMergedInto,
@Assisted("priorPatchSetId") PatchSet.Id priorPatchSetId, @Assisted("priorPatchSetId") PatchSet.Id priorPatchSetId,
@Assisted("priorCommit") RevCommit priorCommit, @Assisted("priorCommitId") ObjectId priorCommitId,
@Assisted("patchSetId") PatchSet.Id patchSetId, @Assisted("patchSetId") PatchSet.Id patchSetId,
@Assisted("commit") RevCommit commit, @Assisted("commitId") ObjectId commitId,
@Assisted PatchSetInfo info, @Assisted PatchSetInfo info,
@Assisted List<String> groups, @Assisted List<String> groups,
@Assisted @Nullable MagicBranchInput magicBranch, @Assisted @Nullable MagicBranchInput magicBranch,
@@ -180,9 +181,9 @@ public class ReplaceOp implements BatchUpdateOp {
this.dest = dest; this.dest = dest;
this.checkMergedInto = checkMergedInto; this.checkMergedInto = checkMergedInto;
this.priorPatchSetId = priorPatchSetId; this.priorPatchSetId = priorPatchSetId;
this.priorCommit = priorCommit; this.priorCommitId = priorCommitId.copy();
this.patchSetId = patchSetId; this.patchSetId = patchSetId;
this.commit = commit; this.commitId = commitId.copy();
this.info = info; this.info = info;
this.groups = groups; this.groups = groups;
this.magicBranch = magicBranch; this.magicBranch = magicBranch;
@@ -192,9 +193,11 @@ public class ReplaceOp implements BatchUpdateOp {
@Override @Override
public void updateRepo(RepoContext ctx) throws Exception { public void updateRepo(RepoContext ctx) throws Exception {
commit = ctx.getRevWalk().parseCommit(commitId);
ctx.getRevWalk().parseBody(commit);
changeKind = changeKind =
changeKindCache.getChangeKind( changeKindCache.getChangeKind(
projectControl.getProject().getNameKey(), ctx.getRepository(), priorCommit, commit); projectControl.getProject().getNameKey(), ctx.getRepository(), priorCommitId, commitId);
if (checkMergedInto) { if (checkMergedInto) {
Ref mergedInto = findMergedInto(ctx, dest.get(), commit); Ref mergedInto = findMergedInto(ctx, dest.get(), commit);
@@ -205,7 +208,7 @@ public class ReplaceOp implements BatchUpdateOp {
} }
if (updateRef) { if (updateRef) {
ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), commit, patchSetId.toRefName())); ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), commitId, patchSetId.toRefName()));
} }
} }
@@ -259,7 +262,7 @@ public class ReplaceOp implements BatchUpdateOp {
ctx.getRevWalk(), ctx.getRevWalk(),
update, update,
patchSetId, patchSetId,
commit, commitId,
draft, draft,
groups, groups,
pushCertificate != null ? pushCertificate.toTextWithSignature() : null, pushCertificate != null ? pushCertificate.toTextWithSignature() : null,
@@ -383,7 +386,7 @@ public class ReplaceOp implements BatchUpdateOp {
List<String> idList = commit.getFooterLines(CHANGE_ID); List<String> idList = commit.getFooterLines(CHANGE_ID);
if (idList.isEmpty()) { if (idList.isEmpty()) {
change.setKey(new Change.Key("I" + commit.name())); change.setKey(new Change.Key("I" + commitId.name()));
} else { } else {
change.setKey(new Change.Key(idList.get(idList.size() - 1).trim())); change.setKey(new Change.Key(idList.get(idList.size() - 1).trim()));
} }
@@ -398,7 +401,7 @@ public class ReplaceOp implements BatchUpdateOp {
final Account account = ctx.getAccount(); final Account account = ctx.getAccount();
if (!updateRef) { if (!updateRef) {
gitRefUpdated.fire( gitRefUpdated.fire(
ctx.getProject(), newPatchSet.getRefName(), ObjectId.zeroId(), commit, account); ctx.getProject(), newPatchSet.getRefName(), ObjectId.zeroId(), commitId, account);
} }
if (changeKind != ChangeKind.TRIVIAL_REBASE) { if (changeKind != ChangeKind.TRIVIAL_REBASE) {
@@ -505,7 +508,7 @@ public class ReplaceOp implements BatchUpdateOp {
return this; return this;
} }
private Ref findMergedInto(Context ctx, String first, RevCommit commit) { private static Ref findMergedInto(Context ctx, String first, RevCommit commit) {
try { try {
RefDatabase refDatabase = ctx.getRepository().getRefDatabase(); RefDatabase refDatabase = ctx.getRepository().getRefDatabase();

2
gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
View File

@@ -337,7 +337,7 @@ public abstract class BatchUpdate implements AutoCloseable {
return this; return this;
} }
public BatchUpdate insertChange(InsertChangeOp op) { public BatchUpdate insertChange(InsertChangeOp op) throws IOException {
Context ctx = newContext(); Context ctx = newContext();
Change c = op.createChange(ctx); Change c = op.createChange(ctx);
checkArgument( checkArgument(

3
gerrit-server/src/main/java/com/google/gerrit/server/update/InsertChangeOp.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.update; package com.google.gerrit.server.update;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import java.io.IOException;
/** /**
* Specialization of {@link BatchUpdateOp} for creating changes. * Specialization of {@link BatchUpdateOp} for creating changes.
@@ -27,5 +28,5 @@ import com.google.gerrit.reviewdb.client.Change;
* first. * first.
*/ */
public interface InsertChangeOp extends BatchUpdateOp { public interface InsertChangeOp extends BatchUpdateOp {
Change createChange(Context ctx); Change createChange(Context ctx) throws IOException;
} }

4
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -414,7 +414,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
TestRepository<Repo> repo = createProject("repo"); TestRepository<Repo> repo = createProject("repo");
ChangeInserter ins = newChange(repo); ChangeInserter ins = newChange(repo);
insert(repo, ins); insert(repo, ins);
String sha = ins.getCommit().name(); String sha = ins.getCommitId().name();
assertQuery("0000000000000000000000000000000000000000"); assertQuery("0000000000000000000000000000000000000000");
for (int i = 0; i <= 36; i++) { for (int i = 0; i <= 36; i++) {
@@ -1741,7 +1741,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
if (dest == null) { if (dest == null) {
dest = ins.getChange().getDest(); dest = ins.getChange().getDest();
} }
shas.add(ins.getCommit().name()); shas.add(ins.getCommitId().name());
expectedIds.add(ins.getChange().getId().get()); expectedIds.add(ins.getChange().getId().get());
} }