From f201492b4dd633bcca7dda71cf4cf5bb81764e56 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 7 Oct 2015 16:48:09 -0400 Subject: [PATCH] Make PatchSetInserter a BatchUpdate.Op Callers that already have a BatchUpdate can just add the inserter directly, rather than attempting to use the alternate factory method removed a few commits ago. The code within PatchSetInserter is much cleaner this way. (There are no such callers yet.) Change-Id: If267dcf1236ee1cd41681f12f5aa53834eda4013 --- .../server/change/CherryPickChange.java | 29 ++- .../server/change/ConsistencyChecker.java | 33 +-- .../server/change/PatchSetInserter.java | 202 ++++++++---------- .../gerrit/server/change/RebaseChange.java | 19 +- .../gerrit/server/edit/ChangeEditUtil.java | 42 ++-- .../change/AbstractQueryChangesTest.java | 15 +- 6 files changed, 181 insertions(+), 159 deletions(-) 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 5b7c95de72..eb9e83ec66 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 @@ -31,6 +31,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.events.CommitReceivedEvent; +import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; import com.google.gerrit.server.git.GitRepositoryManager; @@ -86,6 +87,7 @@ public class CherryPickChange { private final MergeUtil.Factory mergeUtilFactory; private final ChangeMessagesUtil changeMessagesUtil; private final ChangeUpdate.Factory updateFactory; + private final BatchUpdate.Factory batchUpdateFactory; @Inject CherryPickChange(Provider db, @@ -98,7 +100,8 @@ public class CherryPickChange { PatchSetInserter.Factory patchSetInserterFactory, MergeUtil.Factory mergeUtilFactory, ChangeMessagesUtil changeMessagesUtil, - ChangeUpdate.Factory updateFactory) { + ChangeUpdate.Factory updateFactory, + BatchUpdate.Factory batchUpdateFactory) { this.db = db; this.queryProvider = queryProvider; this.gitManager = gitManager; @@ -110,6 +113,7 @@ public class CherryPickChange { this.mergeUtilFactory = mergeUtilFactory; this.changeMessagesUtil = changeMessagesUtil; this.updateFactory = updateFactory; + this.batchUpdateFactory = batchUpdateFactory; } public Change.Id cherryPick(Change change, PatchSet patch, @@ -210,23 +214,26 @@ public class CherryPickChange { } } - private Change.Id insertPatchSet(Repository git, RevWalk revWalk, Change change, - CodeReviewCommit cherryPickCommit, RefControl refControl, + private Change.Id insertPatchSet(Repository git, RevWalk revWalk, + Change change, CodeReviewCommit cherryPickCommit, RefControl refControl, IdentifiedUser identifiedUser) - throws InvalidChangeOperationException, IOException, OrmException, - UpdateException, RestApiException { + throws IOException, OrmException, UpdateException, RestApiException { final ChangeControl changeControl = refControl.getProjectControl().controlFor(change); final PatchSetInserter inserter = patchSetInserterFactory .create(git, revWalk, changeControl, cherryPickCommit); final PatchSet.Id newPatchSetId = inserter.getPatchSetId(); PatchSet current = db.get().patchSets().get(change.currentPatchSetId()); - inserter - .setMessage("Uploaded patch set " + newPatchSetId.get() + ".") - .setDraft(current.isDraft()) - .setUploader(identifiedUser.getAccountId()) - .setSendMail(false) - .insert(); + + try (BatchUpdate bu = batchUpdateFactory.create( + db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) { + bu.addOp(changeControl, inserter + .setMessage("Uploaded patch set " + newPatchSetId.get() + ".") + .setDraft(current.isDraft()) + .setUploader(identifiedUser.getAccountId()) + .setSendMail(false)); + bu.execute(); + } return change.getId(); } 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 934a0da127..bfa073a388 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 @@ -28,6 +28,7 @@ import com.google.common.collect.Multimap; import com.google.common.collect.MultimapBuilder; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.common.ProblemInfo; import com.google.gerrit.extensions.common.ProblemInfo.Status; @@ -42,12 +43,12 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy; +import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; 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.query.change.ChangeData; import com.google.gwtorm.server.AtomicUpdate; @@ -111,6 +112,7 @@ public class ConsistencyChecker { private final ChangeControl.GenericFactory changeControlFactory; private final PatchSetInfoFactory patchSetInfoFactory; private final PatchSetInserter.Factory patchSetInserterFactory; + private final BatchUpdate.Factory updateFactory; private FixInput fix; private Change change; @@ -131,7 +133,8 @@ public class ConsistencyChecker { @GerritPersonIdent Provider serverIdent, ChangeControl.GenericFactory changeControlFactory, PatchSetInfoFactory patchSetInfoFactory, - PatchSetInserter.Factory patchSetInserterFactory) { + PatchSetInserter.Factory patchSetInserterFactory, + BatchUpdate.Factory updateFactory) { this.db = db; this.repoManager = repoManager; this.user = user; @@ -139,6 +142,7 @@ public class ConsistencyChecker { this.changeControlFactory = changeControlFactory; this.patchSetInfoFactory = patchSetInfoFactory; this.patchSetInserterFactory = patchSetInserterFactory; + this.updateFactory = updateFactory; reset(); } @@ -461,21 +465,24 @@ public class ConsistencyChecker { ChangeControl ctl = changeControlFactory.controlFor(change, user.get()); PatchSetInserter inserter = patchSetInserterFactory.create(repo, rw, ctl, commit); - change = inserter.setValidatePolicy(ValidatePolicy.NONE) - .setRunHooks(false) - .setSendMail(false) - .setAllowClosed(true) - .setUploader(((IdentifiedUser) user.get()).getAccountId()) - // TODO: fix setMessage to work without init() - .setMessage( - "Patch set for merged commit inserted by consistency checker") - .insert(); + try (BatchUpdate bu = updateFactory.create( + db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) { + bu.addOp(ctl, inserter.setValidatePolicy(ValidatePolicy.NONE) + .setRunHooks(false) + .setSendMail(false) + .setAllowClosed(true) + .setUploader(((IdentifiedUser) user.get()).getAccountId()) + // TODO: fix setMessage to work without init() + .setMessage( + "Patch set for merged commit inserted by consistency checker")); + bu.execute(); + } + change = inserter.getChange(); PatchSet.Id psId = change.currentPatchSetId(); p.status = Status.FIXED; p.outcome = "Inserted as patch set " + psId.get(); return psId; - } catch (InvalidChangeOperationException | IOException - | NoSuchChangeException | UpdateException | RestApiException e) { + } catch (NoSuchChangeException | UpdateException | RestApiException e) { warn(e); p.status = Status.FIX_FAILED; p.outcome = "Error inserting new patch set"; 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 4711d3aa99..31016b12f6 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 @@ -17,11 +17,10 @@ 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; -import com.google.gerrit.common.TimeUtil; -import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; @@ -41,7 +40,6 @@ 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; import com.google.gerrit.server.mail.ReplacePatchSetSender; @@ -69,7 +67,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.Collections; -public class PatchSetInserter { +public class PatchSetInserter extends BatchUpdate.Op { private static final Logger log = LoggerFactory.getLogger(PatchSetInserter.class); @@ -91,7 +89,6 @@ public class PatchSetInserter { private final ChangeHooks hooks; private final PatchSetInfoFactory patchSetInfoFactory; private final ReviewDb db; - private final BatchUpdate.Factory batchUpdateFactory; private final CommitValidators.Factory commitValidatorsFactory; private final ReplacePatchSetSender.Factory replacePatchSetFactory; private final ApprovalsUtil approvalsUtil; @@ -119,10 +116,15 @@ public class PatchSetInserter { private Account.Id uploader; private boolean allowClosed; + // Fields set during some phase of BatchUpdate.Op. + private Change change; + private PatchSet patchSet; + private ChangeMessage changeMessage; + private SetMultimap oldReviewers; + @AssistedInject public PatchSetInserter(ChangeHooks hooks, ReviewDb db, - BatchUpdate.Factory batchUpdateFactory, ApprovalsUtil approvalsUtil, ApprovalCopier approvalCopier, ChangeMessagesUtil cmUtil, @@ -135,7 +137,6 @@ public class PatchSetInserter { @Assisted RevCommit commit) { this.hooks = hooks; this.db = db; - this.batchUpdateFactory = batchUpdateFactory; this.approvalsUtil = approvalsUtil; this.approvalCopier = approvalCopier; this.cmUtil = cmUtil; @@ -210,121 +211,108 @@ public class PatchSetInserter { return this; } - public Change insert() throws InvalidChangeOperationException, IOException, - UpdateException, RestApiException { - init(); - validate(); - - Op op = new Op(); - try (BatchUpdate bu = batchUpdateFactory.create( - db, ctl.getChange().getProject(), TimeUtil.nowTs())) { - bu.addOp(ctl, op); - bu.execute(); - } - return op.change; + public Change getChange() { + checkState(change != null, "getChange() only valid after executing update"); + return change; } - private class Op extends BatchUpdate.Op { - private Change change; - private PatchSet patchSet; - private ChangeMessage changeMessage; - private SetMultimap oldReviewers; + @Override + public void updateRepo(RepoContext ctx) + throws InvalidChangeOperationException, IOException { + init(); + validate(); + ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), + commit, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE)); + } - @Override - public void updateRepo(RepoContext ctx) throws IOException { - ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(), - commit, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE)); + @Override + public void updateChange(ChangeContext ctx) throws OrmException, + InvalidChangeOperationException { + change = ctx.readChange(); + Change.Id id = change.getId(); + final PatchSet.Id currentPatchSetId = change.currentPatchSetId(); + if (!change.getStatus().isOpen() && !allowClosed) { + throw new InvalidChangeOperationException(String.format( + "Change %s is closed", change.getId())); } - @Override - public void updateChange(ChangeContext ctx) throws OrmException, - InvalidChangeOperationException { - change = ctx.readChange(); - Change.Id id = change.getId(); - final PatchSet.Id currentPatchSetId = change.currentPatchSetId(); - if (!change.getStatus().isOpen() && !allowClosed) { - throw new InvalidChangeOperationException(String.format( - "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); - 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); + } else { + patchSet.setGroups(GroupCollector.getCurrentGroups(db, change)); + } + db.patchSets().insert(Collections.singleton(patchSet)); - ChangeUtil.insertAncestors(db, patchSet.getId(), commit); - if (groups != null) { - patchSet.setGroups(groups); - } else { - patchSet.setGroups(GroupCollector.getCurrentGroups(db, change)); - } - db.patchSets().insert(Collections.singleton(patchSet)); + if (sendMail) { + oldReviewers = approvalsUtil.getReviewers(db, ctl.getNotes()); + } - if (sendMail) { - oldReviewers = approvalsUtil.getReviewers(db, ctl.getNotes()); - } + if (message != null) { + changeMessage = new ChangeMessage( + new ChangeMessage.Key( + ctl.getChange().getId(), ChangeUtil.messageUUID(db)), + user.getAccountId(), ctx.getWhen(), patchSet.getId()); + changeMessage.setMessage(message); + } - if (message != null) { - changeMessage = new ChangeMessage( - new ChangeMessage.Key( - ctl.getChange().getId(), ChangeUtil.messageUUID(db)), - user.getAccountId(), ctx.getWhen(), patchSet.getId()); - changeMessage.setMessage(message); - } - - // TODO(dborowitz): Throw ResourceConflictException instead of using - // AtomicUpdate. - change = db.changes().atomicUpdate(id, new AtomicUpdate() { - @Override - public Change update(Change change) { - if (change.getStatus().isClosed() && !allowClosed) { - return null; - } - if (!change.currentPatchSetId().equals(currentPatchSetId)) { - return null; - } - if (change.getStatus() != Change.Status.DRAFT && !allowClosed) { - change.setStatus(Change.Status.NEW); - } - change.setCurrentPatchSet(patchSetInfoFactory.get(commit, psId)); - ChangeUtil.updated(change); - return change; + // TODO(dborowitz): Throw ResourceConflictException instead of using + // AtomicUpdate. + change = db.changes().atomicUpdate(id, new AtomicUpdate() { + @Override + public Change update(Change change) { + if (change.getStatus().isClosed() && !allowClosed) { + return null; } - }); - if (change == null) { - throw new ChangeModifiedException(String.format( - "Change %s was modified", id)); + if (!change.currentPatchSetId().equals(currentPatchSetId)) { + return null; + } + if (change.getStatus() != Change.Status.DRAFT && !allowClosed) { + change.setStatus(Change.Status.NEW); + } + change.setCurrentPatchSet(patchSetInfoFactory.get(commit, psId)); + ChangeUtil.updated(change); + return change; } + }); + if (change == null) { + throw new ChangeModifiedException(String.format( + "Change %s was modified", id)); + } - approvalCopier.copy(db, ctl, patchSet); - if (changeMessage != null) { - cmUtil.addChangeMessage(db, ctx.getChangeUpdate(), changeMessage); + approvalCopier.copy(db, ctl, patchSet); + if (changeMessage != null) { + cmUtil.addChangeMessage(db, ctx.getChangeUpdate(), changeMessage); + } + } + + @Override + public void postUpdate(Context ctx) throws OrmException { + if (sendMail) { + try { + PatchSetInfo info = patchSetInfoFactory.get(commit, psId); + ReplacePatchSetSender cm = replacePatchSetFactory.create( + change.getId()); + cm.setFrom(user.getAccountId()); + cm.setPatchSet(patchSet, info); + cm.setChangeMessage(changeMessage); + cm.addReviewers(oldReviewers.get(ReviewerState.REVIEWER)); + cm.addExtraCC(oldReviewers.get(ReviewerState.CC)); + cm.send(); + } catch (Exception err) { + log.error("Cannot send email for new patch set on change " + + change.getId(), err); } } - @Override - public void postUpdate(Context ctx) throws OrmException { - if (sendMail) { - try { - PatchSetInfo info = patchSetInfoFactory.get(commit, psId); - ReplacePatchSetSender cm = replacePatchSetFactory.create( - change.getId()); - cm.setFrom(user.getAccountId()); - cm.setPatchSet(patchSet, info); - cm.setChangeMessage(changeMessage); - cm.addReviewers(oldReviewers.get(ReviewerState.REVIEWER)); - cm.addExtraCC(oldReviewers.get(ReviewerState.CC)); - cm.send(); - } catch (Exception err) { - log.error("Cannot send email for new patch set on change " - + change.getId(), err); - } - } - - if (runHooks) { - hooks.doPatchsetCreatedHook(change, patchSet, ctx.getDb()); - } + if (runHooks) { + hooks.doPatchsetCreatedHook(change, patchSet, ctx.getDb()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java index 865ec39db3..834ab23e1b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java @@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy; +import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeConflictException; import com.google.gerrit.server.git.MergeUtil; @@ -64,6 +65,7 @@ public class RebaseChange { private final TimeZone serverTimeZone; private final MergeUtil.Factory mergeUtilFactory; private final PatchSetInserter.Factory patchSetInserterFactory; + private final BatchUpdate.Factory updateFactory; @Inject RebaseChange(ChangeControl.GenericFactory changeControlFactory, @@ -71,13 +73,15 @@ public class RebaseChange { @GerritPersonIdent PersonIdent myIdent, GitRepositoryManager gitManager, MergeUtil.Factory mergeUtilFactory, - PatchSetInserter.Factory patchSetInserterFactory) { + PatchSetInserter.Factory patchSetInserterFactory, + BatchUpdate.Factory updateFactory) { this.changeControlFactory = changeControlFactory; this.db = db; this.gitManager = gitManager; this.serverTimeZone = myIdent.getTimeZone(); this.mergeUtilFactory = mergeUtilFactory; this.patchSetInserterFactory = patchSetInserterFactory; + this.updateFactory = updateFactory; } /** @@ -282,12 +286,15 @@ public class RebaseChange { .setSendMail(false) .setRunHooks(runHooks); - Change newChange = patchSetInserter - .setMessage("Patch Set " + patchSetInserter.getPatchSetId().get() - + ": Patch Set " + patchSetId.get() + " was rebased") - .insert(); + try (BatchUpdate bu = updateFactory.create( + db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) { + bu.addOp(changeControl, patchSetInserter.setMessage( + "Patch Set " + patchSetInserter.getPatchSetId().get() + + ": Patch Set " + patchSetId.get() + " was rebased")); + bu.execute(); + } - return db.get().patchSets().get(newChange.currentPatchSetId()); + return db.get().patchSets().get(patchSetInserter.getPatchSetId()); } /** 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 94f64f7333..6b32e00b4f 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,6 +17,7 @@ package com.google.gerrit.server.edit; 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; @@ -30,11 +31,11 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.ChangeKind; import com.google.gerrit.server.change.ChangeKindCache; import com.google.gerrit.server.change.PatchSetInserter; +import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.index.ChangeIndexer; 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; @@ -70,6 +71,7 @@ public class ChangeEditUtil { private final Provider db; private final Provider user; private final ChangeKindCache changeKindCache; + private final BatchUpdate.Factory updateFactory; @Inject ChangeEditUtil(GitRepositoryManager gitManager, @@ -79,7 +81,8 @@ public class ChangeEditUtil { ProjectCache projectCache, Provider db, Provider user, - ChangeKindCache changeKindCache) { + ChangeKindCache changeKindCache, + BatchUpdate.Factory updateFactory) { this.gitManager = gitManager; this.patchSetInserterFactory = patchSetInserterFactory; this.changeControlFactory = changeControlFactory; @@ -88,6 +91,7 @@ public class ChangeEditUtil { this.db = db; this.user = user; this.changeKindCache = changeKindCache; + this.updateFactory = updateFactory; } /** @@ -161,16 +165,12 @@ public class ChangeEditUtil { "only edit for current patch set can be published"); } - try { - Change updatedChange = - insertPatchSet(edit, change, repo, rw, basePatchSet, - squashEdit(rw, inserter, edit.getEditCommit(), basePatchSet)); - // TODO(davido): This should happen in the same BatchRefUpdate. - deleteRef(repo, edit); - indexer.index(db.get(), updatedChange); - } catch (InvalidChangeOperationException e) { - throw new ResourceConflictException(e.getMessage()); - } + Change updatedChange = + insertPatchSet(edit, change, repo, rw, basePatchSet, + squashEdit(rw, inserter, edit.getEditCommit(), basePatchSet)); + // TODO(davido): This should happen in the same BatchRefUpdate. + deleteRef(repo, edit); + indexer.index(db.get(), updatedChange); } } @@ -217,11 +217,10 @@ public class ChangeEditUtil { private Change insertPatchSet(ChangeEdit edit, Change change, Repository repo, RevWalk rw, PatchSet basePatchSet, RevCommit squashed) throws NoSuchChangeException, RestApiException, UpdateException, - InvalidChangeOperationException, IOException { + IOException { + ChangeControl ctl = changeControlFactory.controlFor(change, edit.getUser()); PatchSetInserter inserter = - patchSetInserterFactory.create(repo, rw, - changeControlFactory.controlFor(change, edit.getUser()), - squashed); + patchSetInserterFactory.create(repo, rw, ctl, squashed); StringBuilder message = new StringBuilder("Patch set ") .append(inserter.getPatchSetId().get()) @@ -239,11 +238,16 @@ public class ChangeEditUtil { .append("."); } - return inserter + try (BatchUpdate bu = updateFactory.create( + db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) { + bu.addOp(ctl, inserter .setDraft(change.getStatus() == Status.DRAFT || basePatchSet.isDraft()) - .setMessage(message.toString()) - .insert(); + .setMessage(message.toString())); + bu.execute(); + } + + return inserter.getChange(); } private static void deleteRef(Repository repo, ChangeEdit edit) 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 499caa2728..3f6cb32a8e 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 @@ -54,6 +54,7 @@ import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ChangeTriplet; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy; +import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.index.IndexCollection; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; @@ -113,6 +114,7 @@ public abstract class AbstractQueryChangesTest { @ConfigSuite.Parameter public Config config; @Inject protected AccountManager accountManager; + @Inject protected BatchUpdate.Factory updateFactory; @Inject protected ChangeInserter.Factory changeFactory; @Inject protected PatchSetInserter.Factory patchSetFactory; @Inject protected ChangeControl.GenericFactory changeControlFactory; @@ -1326,12 +1328,19 @@ public abstract class AbstractQueryChangesTest { .add("file" + n, "contents " + n) .create()); ChangeControl ctl = changeControlFactory.controlFor(c.getId(), user); - return patchSetFactory.create( + + PatchSetInserter inserter = patchSetFactory.create( repo.getRepository(), repo.getRevWalk(), ctl, commit) .setSendMail(false) .setRunHooks(false) - .setValidatePolicy(ValidatePolicy.NONE) - .insert(); + .setValidatePolicy(ValidatePolicy.NONE); + try (BatchUpdate bu = updateFactory.create( + db, c.getDest().getParentKey(), TimeUtil.nowTs())) { + bu.addOp(ctl, inserter); + bu.execute(); + } + + return inserter.getChange(); } protected void assertBadQuery(Object query) throws Exception {