diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java new file mode 100644 index 0000000000..6ef04fb919 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java @@ -0,0 +1,76 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server; + +import com.google.common.annotations.VisibleForTesting; +import com.google.gerrit.reviewdb.client.ChangeMessage; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +import java.util.Collections; +import java.util.List; + +/** + * Utility functions to manipulate ChangeMessages. + *

+ * These methods either query for and update ChangeMessages in the NoteDb or + * ReviewDb, depending on the state of the NotesMigration. + */ +@Singleton +public class ChangeMessagesUtil { + private static List sortChangeMessages( + Iterable changeMessage) { + return ChangeNotes.MESSAGE_BY_TIME.sortedCopy(changeMessage); + } + + private final NotesMigration migration; + + @VisibleForTesting + @Inject + public ChangeMessagesUtil(NotesMigration migration) { + this.migration = migration; + } + + public List byChange(ReviewDb db, ChangeNotes notes) throws OrmException { + if (!migration.readChangeMessages()) { + return + sortChangeMessages(db.changeMessages().byChange(notes.getChangeId())); + } else { + return sortChangeMessages(notes.load().getChangeMessages().values()); + } + } + + public List byPatchSet(ReviewDb db, ChangeNotes notes, + PatchSet.Id psId) throws OrmException { + if (!migration.readChangeMessages()) { + return sortChangeMessages(db.changeMessages().byPatchSet(psId)); + } + return notes.load().getChangeMessages().get(psId); + } + + public void addChangeMessage(ReviewDb db, ChangeUpdate update, + ChangeMessage changeMessage) throws OrmException { + if (changeMessage != null) { + update.setChangeMessage(changeMessage.getMessage()); + db.changeMessages().insert(Collections.singleton(changeMessage)); + } + } +} \ No newline at end of file diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java index b083850fd1..388e1cf83f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java @@ -25,12 +25,14 @@ import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.ChangeJson.ChangeInfo; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.mail.AbandonedSender; import com.google.gerrit.server.mail.ReplyToChangeSender; +import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; @@ -42,7 +44,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; -import java.util.Collections; @Singleton public class Abandon implements RestModifyView, @@ -54,18 +55,24 @@ public class Abandon implements RestModifyView, private final Provider dbProvider; private final ChangeJson json; private final ChangeIndexer indexer; + private final ChangeUpdate.Factory updateFactory; + private final ChangeMessagesUtil cmUtil; @Inject Abandon(ChangeHooks hooks, AbandonedSender.Factory abandonedSenderFactory, Provider dbProvider, ChangeJson json, - ChangeIndexer indexer) { + ChangeIndexer indexer, + ChangeUpdate.Factory updateFactory, + ChangeMessagesUtil cmUtil) { this.hooks = hooks; this.abandonedSenderFactory = abandonedSenderFactory; this.dbProvider = dbProvider; this.json = json; this.indexer = indexer; + this.updateFactory = updateFactory; + this.cmUtil = cmUtil; } @Override @@ -84,6 +91,7 @@ public class Abandon implements RestModifyView, } ChangeMessage message; + ChangeUpdate update; ReviewDb db = dbProvider.get(); db.changes().beginTransaction(change.getId()); try { @@ -104,12 +112,16 @@ public class Abandon implements RestModifyView, throw new ResourceConflictException("change is " + status(db.changes().get(req.getChange().getId()))); } + + //TODO(yyonas): atomic update was not propagated + update = updateFactory.create(control, change.getLastUpdatedOn()); message = newMessage(input, caller, change); - db.changeMessages().insert(Collections.singleton(message)); + cmUtil.addChangeMessage(db, update, message); db.commit(); } finally { db.rollback(); } + update.commit(); CheckedFuture indexFuture = indexer.indexAsync(change.getId()); 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 d8344f8eae..3c210d44dd 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 @@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.mail.CreateChangeSender; @@ -62,6 +63,7 @@ public class ChangeInserter { private final GitReferenceUpdated gitRefUpdated; private final ChangeHooks hooks; private final ApprovalsUtil approvalsUtil; + private final ChangeMessagesUtil cmUtil; private final MergeabilityChecker mergeabilityChecker; private final CreateChangeSender.Factory createChangeSenderFactory; @@ -85,6 +87,7 @@ public class ChangeInserter { GitReferenceUpdated gitRefUpdated, ChangeHooks hooks, ApprovalsUtil approvalsUtil, + ChangeMessagesUtil cmUtil, MergeabilityChecker mergeabilityChecker, CreateChangeSender.Factory createChangeSenderFactory, @Assisted RefControl refControl, @@ -95,6 +98,7 @@ public class ChangeInserter { this.gitRefUpdated = gitRefUpdated; this.hooks = hooks; this.approvalsUtil = approvalsUtil; + this.cmUtil = cmUtil; this.mergeabilityChecker = mergeabilityChecker; this.createChangeSenderFactory = createChangeSenderFactory; this.refControl = refControl; @@ -181,20 +185,22 @@ public class ChangeInserter { approvalsUtil.addApprovals(db, update, labelTypes, patchSet, patchSetInfo, change, ctl, approvals); if (messageIsForChange()) { - insertMessage(db); + cmUtil.addChangeMessage(db, update, changeMessage); } db.commit(); } finally { db.rollback(); } - - update.commit(); + if (messageIsForChange()) { + update.commit(); + } CheckedFuture f = mergeabilityChecker.newCheck() .addChange(change) .reindex() .runAsync(); - if (!messageIsForChange()) { - insertMessage(db); + + if(!messageIsForChange()) { + commitMessageNotForChange(); } gitRefUpdated.fire(change.getProject(), patchSet.getRefName(), ObjectId.zeroId(), commit); @@ -220,14 +226,23 @@ public class ChangeInserter { return change; } + private void commitMessageNotForChange() throws OrmException, + IOException { + ReviewDb db = dbProvider.get(); + if (changeMessage != null) { + Change otherChange = + db.changes().get(changeMessage.getPatchSetId().getParentKey()); + ChangeControl otherControl = + refControl.getProjectControl().controlFor(otherChange); + ChangeUpdate updateForOtherChange = + updateFactory.create(otherControl, change.getLastUpdatedOn()); + cmUtil.addChangeMessage(db, updateForOtherChange, changeMessage); + updateForOtherChange.commit(); + } + } + private boolean messageIsForChange() { return changeMessage != null && changeMessage.getKey().getParentKey().equals(change.getKey()); } - - private void insertMessage(ReviewDb db) throws OrmException { - if (changeMessage != null) { - db.changeMessages().insert(Collections.singleton(changeMessage)); - } - } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 067affa3f8..80ddfe960b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -74,12 +74,14 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo.ParentInfo; import com.google.gerrit.reviewdb.client.UserIdentity; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.AnonymousUser; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.WebLinks; import com.google.gerrit.server.account.AccountInfo; import com.google.gerrit.server.extensions.webui.UiActions; import com.google.gerrit.server.git.LabelNormalizer; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; @@ -88,7 +90,6 @@ import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData.ChangedLines; import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; import com.google.inject.Provider; @@ -100,7 +101,6 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.EnumSet; -import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -108,22 +108,8 @@ import java.util.TreeMap; public class ChangeJson { private static final Logger log = LoggerFactory.getLogger(ChangeJson.class); - private static final ResultSet NO_MESSAGES = - new ResultSet() { - @Override - public Iterator iterator() { - return toList().iterator(); - } - - @Override - public List toList() { - return Collections.emptyList(); - } - - @Override - public void close() { - } - }; + private static final List NO_MESSAGES = + ImmutableList.of(); private final Provider db; private final LabelNormalizer labelNormalizer; @@ -140,6 +126,7 @@ public class ChangeJson { private final Revisions revisions; private final Provider webLinks; private final EnumSet options; + private final ChangeMessagesUtil cmUtil; private AccountInfo.Loader accountLoader; @@ -159,7 +146,8 @@ public class ChangeJson { DynamicMap downloadCommands, DynamicMap> changeViews, Revisions revisions, - Provider webLinks) { + Provider webLinks, + ChangeMessagesUtil cmUtil) { this.db = db; this.labelNormalizer = ln; this.userProvider = user; @@ -174,6 +162,7 @@ public class ChangeJson { this.changeViews = changeViews; this.revisions = revisions; this.webLinks = webLinks; + this.cmUtil = cmUtil; options = EnumSet.noneOf(ListChangesOption.class); } @@ -642,8 +631,7 @@ public class ChangeJson { private Collection messages(ChangeControl ctl, ChangeData cd, Map map) throws OrmException { - List messages = - db.get().changeMessages().byChange(cd.getId()).toList(); + List messages = cmUtil.byChange(db.get(), cd.notes()); if (messages.isEmpty()) { return Collections.emptyList(); } @@ -705,18 +693,18 @@ public class ChangeJson { if (userProvider.get().isIdentifiedUser()) { Account.Id self = ((IdentifiedUser) userProvider.get()).getAccountId(); for (List batch : Iterables.partition(all, 50)) { - List> m = + List> m = Lists.newArrayListWithCapacity(batch.size()); for (ChangeData cd : batch) { PatchSet.Id ps = cd.change().currentPatchSetId(); if (ps != null && cd.change().getStatus().isOpen()) { - m.add(db.get().changeMessages().byPatchSet(ps)); + m.add(cmUtil.byPatchSet(db.get(), cd.notes(), ps)); } else { m.add(NO_MESSAGES); } } for (int i = 0; i < m.size(); i++) { - if (isChangeReviewed(self, batch.get(i), m.get(i).toList())) { + if (isChangeReviewed(self, batch.get(i), m.get(i))) { reviewed.add(batch.get(i).getId()); } } @@ -728,12 +716,7 @@ public class ChangeJson { private boolean isChangeReviewed(Account.Id self, ChangeData cd, List msgs) throws OrmException { // Sort messages to keep the most recent ones at the beginning. - Collections.sort(msgs, new Comparator() { - @Override - public int compare(ChangeMessage a, ChangeMessage b) { - return b.getWrittenOn().compareTo(a.getWrittenOn()); - } - }); + msgs = ChangeNotes.MESSAGE_BY_TIME.sortedCopy(msgs); Account.Id changeOwnerId = cd.change().getOwner(); for (ChangeMessage cm : msgs) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java index a1245fb154..3c53f7b0ed 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java @@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.DeleteReviewer.Input; @@ -40,7 +41,6 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; -import java.util.Collections; import java.util.List; @Singleton @@ -51,6 +51,7 @@ public class DeleteReviewer implements RestModifyView { private final Provider dbProvider; private final ChangeUpdate.Factory updateFactory; private final ApprovalsUtil approvalsUtil; + private final ChangeMessagesUtil cmUtil; private final ChangeIndexer indexer; private final IdentifiedUser.GenericFactory userFactory; @@ -58,11 +59,13 @@ public class DeleteReviewer implements RestModifyView { DeleteReviewer(Provider dbProvider, ChangeUpdate.Factory updateFactory, ApprovalsUtil approvalsUtil, + ChangeMessagesUtil cmUtil, ChangeIndexer indexer, IdentifiedUser.GenericFactory userFactory) { this.dbProvider = dbProvider; this.updateFactory = updateFactory; this.approvalsUtil = approvalsUtil; + this.cmUtil = cmUtil; this.indexer = indexer; this.userFactory = userFactory; } @@ -111,7 +114,7 @@ public class DeleteReviewer implements RestModifyView { ((IdentifiedUser) control.getCurrentUser()).getAccountId(), TimeUtil.nowTs(), rsrc.getChange().currentPatchSetId()); changeMessage.setMessage(msg.toString()); - db.changeMessages().insert(Collections.singleton(changeMessage)); + cmUtil.addChangeMessage(db, update, changeMessage); } db.commit(); 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 c059bf4ac0..044e8990b9 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 @@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalCopier; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.events.CommitReceivedEvent; @@ -41,6 +42,7 @@ import com.google.gerrit.server.notedb.ReviewerState; import com.google.gerrit.server.patch.PatchSetInfoFactory; 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.ssh.NoSshInfo; import com.google.gerrit.server.ssh.SshInfo; import com.google.gerrit.server.util.TimeUtil; @@ -83,12 +85,14 @@ public class PatchSetInserter { private final PatchSetInfoFactory patchSetInfoFactory; private final ReviewDb db; private final ChangeUpdate.Factory updateFactory; + private final ChangeControl.GenericFactory ctlFactory; private final GitReferenceUpdated gitRefUpdated; private final CommitValidators.Factory commitValidatorsFactory; private final MergeabilityChecker mergeabilityChecker; private final ReplacePatchSetSender.Factory replacePatchSetFactory; private final ApprovalsUtil approvalsUtil; private final ApprovalCopier approvalCopier; + private final ChangeMessagesUtil cmUtil; private final Repository git; private final RevWalk revWalk; @@ -110,8 +114,10 @@ public class PatchSetInserter { public PatchSetInserter(ChangeHooks hooks, ReviewDb db, ChangeUpdate.Factory updateFactory, + ChangeControl.GenericFactory ctlFactory, ApprovalsUtil approvalsUtil, ApprovalCopier approvalCopier, + ChangeMessagesUtil cmUtil, PatchSetInfoFactory patchSetInfoFactory, GitReferenceUpdated gitRefUpdated, CommitValidators.Factory commitValidatorsFactory, @@ -127,8 +133,10 @@ public class PatchSetInserter { this.hooks = hooks; this.db = db; this.updateFactory = updateFactory; + this.ctlFactory = ctlFactory; this.approvalsUtil = approvalsUtil; this.approvalCopier = approvalCopier; + this.cmUtil = cmUtil; this.patchSetInfoFactory = patchSetInfoFactory; this.gitRefUpdated = gitRefUpdated; this.commitValidatorsFactory = commitValidatorsFactory; @@ -212,7 +220,7 @@ public class PatchSetInserter { } public Change insert() throws InvalidChangeOperationException, OrmException, - IOException { + IOException, NoSuchChangeException { init(); validate(); @@ -273,17 +281,19 @@ public class PatchSetInserter { } if (messageIsForChange()) { - insertMessage(db); + cmUtil.addChangeMessage(db, update, changeMessage); } if (copyLabels) { approvalCopier.copy(db, ctl, patchSet); } db.commit(); - update.commit(); + if (messageIsForChange()) { + update.commit(); + } if (!messageIsForChange()) { - insertMessage(db); + commitMessageNotForChange(updatedChange); } if (sendMail) { @@ -317,6 +327,20 @@ public class PatchSetInserter { return updatedChange; } + private void commitMessageNotForChange(Change updatedChange) + throws OrmException, NoSuchChangeException, IOException { + if (changeMessage != null) { + Change otherChange = + db.changes().get(changeMessage.getPatchSetId().getParentKey()); + ChangeControl otherControl = + ctlFactory.controlFor(otherChange, user); + ChangeUpdate updateForOtherChange = + updateFactory.create(otherControl, updatedChange.getLastUpdatedOn()); + cmUtil.addChangeMessage(db, updateForOtherChange, changeMessage); + updateForOtherChange.commit(); + } + } + private void init() throws IOException { if (sshInfo == null) { sshInfo = new NoSshInfo(); @@ -368,12 +392,6 @@ public class PatchSetInserter { .equals(patchSet.getId().getParentKey()); } - private void insertMessage(ReviewDb db) throws OrmException { - if (changeMessage != null) { - db.changeMessages().insert(Collections.singleton(changeMessage)); - } - } - public class ChangeModifiedException extends InvalidChangeOperationException { private static final long serialVersionUID = 1L; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index d5047438a9..288172e627 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -46,6 +46,7 @@ import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountsCollection; @@ -82,6 +83,7 @@ public class PostReview implements RestModifyView private final ChangeData.Factory changeDataFactory; private final ChangeUpdate.Factory updateFactory; private final ApprovalsUtil approvalsUtil; + private final ChangeMessagesUtil cmUtil; private final ChangeIndexer indexer; private final AccountsCollection accounts; private final EmailReviewComments.Factory email; @@ -100,6 +102,7 @@ public class PostReview implements RestModifyView ChangeData.Factory changeDataFactory, ChangeUpdate.Factory updateFactory, ApprovalsUtil approvalsUtil, + ChangeMessagesUtil cmUtil, ChangeIndexer indexer, AccountsCollection accounts, EmailReviewComments.Factory email, @@ -109,6 +112,7 @@ public class PostReview implements RestModifyView this.changeDataFactory = changeDataFactory; this.updateFactory = updateFactory; this.approvalsUtil = approvalsUtil; + this.cmUtil = cmUtil; this.indexer = indexer; this.accounts = accounts; this.email = email; @@ -144,7 +148,7 @@ public class PostReview implements RestModifyView update = updateFactory.create(revision.getControl(), timestamp); dirty |= insertComments(revision, input.comments, input.drafts); dirty |= updateLabels(revision, update, input.labels); - dirty |= insertMessage(revision, input.message); + dirty |= insertMessage(revision, input.message, update); if (dirty) { db.get().changes().update(Collections.singleton(change)); db.get().commit(); @@ -505,8 +509,8 @@ public class PostReview implements RestModifyView labelDelta.add(new LabelVote(name, value).format()); } - private boolean insertMessage(RevisionResource rsrc, String msg) - throws OrmException { + private boolean insertMessage(RevisionResource rsrc, String msg, + ChangeUpdate update) throws OrmException { msg = Strings.nullToEmpty(msg).trim(); StringBuilder buf = new StringBuilder(); @@ -534,7 +538,7 @@ public class PostReview implements RestModifyView "Patch Set %d:%s", rsrc.getPatchSet().getPatchSetId(), buf.toString())); - db.get().changeMessages().insert(Collections.singleton(message)); + cmUtil.addChangeMessage(db.get(), update, message); return true; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java index 8d96f2dc56..9676762ee4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java @@ -25,10 +25,12 @@ import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.PutTopic.Input; import com.google.gerrit.server.index.ChangeIndexer; +import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.util.TimeUtil; import com.google.gwtorm.server.AtomicUpdate; @@ -38,7 +40,6 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; -import java.util.Collections; @Singleton class PutTopic implements RestModifyView, @@ -46,6 +47,8 @@ class PutTopic implements RestModifyView, private final Provider dbProvider; private final ChangeIndexer indexer; private final ChangeHooks hooks; + private final ChangeUpdate.Factory updateFactory; + private final ChangeMessagesUtil cmUtil; static class Input { @DefaultInput @@ -54,10 +57,13 @@ class PutTopic implements RestModifyView, @Inject PutTopic(Provider dbProvider, ChangeIndexer indexer, - ChangeHooks hooks) { + ChangeHooks hooks, ChangeUpdate.Factory updateFactory, + ChangeMessagesUtil cmUtil) { this.dbProvider = dbProvider; this.indexer = indexer; this.hooks = hooks; + this.updateFactory = updateFactory; + this.cmUtil = cmUtil; } @Override @@ -94,6 +100,7 @@ class PutTopic implements RestModifyView, currentUser.getAccountId(), TimeUtil.nowTs(), change.currentPatchSetId()); cmsg.setMessage(summary); + ChangeUpdate update; db.changes().beginTransaction(change.getId()); try { @@ -106,11 +113,16 @@ class PutTopic implements RestModifyView, return change; } }); - db.changeMessages().insert(Collections.singleton(cmsg)); + + //TODO(yyonas): atomic update was not propagated + update = updateFactory.create(control); + cmUtil.addChangeMessage(db, update, cmsg); + db.commit(); } finally { db.rollback(); } + update.commit(); CheckedFuture indexFuture = indexer.indexAsync(change.getId()); hooks.doTopicChangedHook(change, currentUser.getAccount(), diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java index 083de08af1..7a31ebec86 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java @@ -26,11 +26,13 @@ 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.server.ReviewDb; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.ChangeJson.ChangeInfo; import com.google.gerrit.server.mail.ReplyToChangeSender; import com.google.gerrit.server.mail.RestoredSender; +import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; @@ -42,7 +44,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; -import java.util.Collections; @Singleton public class Restore implements RestModifyView, @@ -54,18 +55,24 @@ public class Restore implements RestModifyView, private final Provider dbProvider; private final ChangeJson json; private final MergeabilityChecker mergeabilityChecker; + private final ChangeMessagesUtil cmUtil; + private final ChangeUpdate.Factory updateFactory; @Inject Restore(ChangeHooks hooks, RestoredSender.Factory restoredSenderFactory, Provider dbProvider, ChangeJson json, - MergeabilityChecker mergeabilityChecker) { + MergeabilityChecker mergeabilityChecker, + ChangeMessagesUtil cmUtil, + ChangeUpdate.Factory updateFactory) { this.hooks = hooks; this.restoredSenderFactory = restoredSenderFactory; this.dbProvider = dbProvider; this.json = json; this.mergeabilityChecker = mergeabilityChecker; + this.cmUtil = cmUtil; + this.updateFactory = updateFactory; } @Override @@ -82,6 +89,7 @@ public class Restore implements RestModifyView, } ChangeMessage message; + ChangeUpdate update; ReviewDb db = dbProvider.get(); db.changes().beginTransaction(change.getId()); try { @@ -102,12 +110,16 @@ public class Restore implements RestModifyView, throw new ResourceConflictException("change is " + status(db.changes().get(req.getChange().getId()))); } + + //TODO(yyonas): atomic update was not propagated + update = updateFactory.create(control); message = newMessage(input, caller, change); - db.changeMessages().insert(Collections.singleton(message)); + cmUtil.addChangeMessage(db, update, message); db.commit(); } finally { db.rollback(); } + update.commit(); CheckedFuture f = mergeabilityChecker.newCheck() .addChange(change) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index be2788a11c..371902871a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -20,11 +20,11 @@ import com.google.common.base.Objects; import com.google.common.base.Optional; import com.google.common.base.Predicate; import com.google.common.base.Strings; +import com.google.common.collect.FluentIterable; import com.google.common.collect.HashBasedTable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Table; import com.google.gerrit.common.data.ParameterizedString; @@ -44,6 +44,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; @@ -103,6 +104,7 @@ public class Submit implements RestModifyView, private final IdentifiedUser.GenericFactory userFactory; private final ChangeUpdate.Factory updateFactory; private final ApprovalsUtil approvalsUtil; + private final ChangeMessagesUtil cmUtil; private final MergeQueue mergeQueue; private final ChangeIndexer indexer; private final LabelNormalizer labelNormalizer; @@ -118,6 +120,7 @@ public class Submit implements RestModifyView, IdentifiedUser.GenericFactory userFactory, ChangeUpdate.Factory updateFactory, ApprovalsUtil approvalsUtil, + ChangeMessagesUtil cmUtil, MergeQueue mergeQueue, AccountsCollection accounts, ChangesCollection changes, @@ -130,6 +133,7 @@ public class Submit implements RestModifyView, this.userFactory = userFactory; this.updateFactory = updateFactory; this.approvalsUtil = approvalsUtil; + this.cmUtil = cmUtil; this.mergeQueue = mergeQueue; this.accounts = accounts; this.changes = changes; @@ -226,16 +230,16 @@ public class Submit implements RestModifyView, */ public ChangeMessage getConflictMessage(RevisionResource rsrc) throws OrmException { - return Iterables.getFirst(Iterables.filter( - Lists.reverse(dbProvider.get().changeMessages() - .byChange(rsrc.getChange().getId()) - .toList()), - new Predicate() { - @Override - public boolean apply(ChangeMessage input) { - return input.getAuthor() == null; - } - }), null); + return FluentIterable.from(cmUtil.byPatchSet(dbProvider.get(), rsrc.getNotes(), + rsrc.getPatchSet().getId())) + .filter(new Predicate() { + @Override + public boolean apply(ChangeMessage input) { + return input.getAuthor() == null; + } + }) + .last() + .orNull(); } public Change submit(RevisionResource rsrc, IdentifiedUser caller, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/AsyncReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/AsyncReceiveCommits.java index aaf0273187..85eba4255c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/AsyncReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/AsyncReceiveCommits.java @@ -23,7 +23,6 @@ import com.google.gerrit.server.util.RequestScopePropagator; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.FactoryModuleBuilder; import com.google.inject.Inject; - import com.google.inject.name.Named; import com.google.inject.PrivateModule; import com.google.inject.Provides; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 74056ab271..06e17ceddd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -42,6 +42,7 @@ 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.ApprovalsUtil; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; @@ -54,6 +55,7 @@ import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.mail.MergeFailSender; import com.google.gerrit.server.mail.MergedSender; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.ChangeControl; @@ -88,7 +90,6 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -137,9 +138,11 @@ public class MergeOp { private final PatchSetInfoFactory patchSetInfoFactory; private final IdentifiedUser.GenericFactory identifiedUserFactory; private final ChangeControl.GenericFactory changeControlFactory; + private final ChangeUpdate.Factory updateFactory; private final MergeQueue mergeQueue; private final MergeValidators.Factory mergeValidatorsFactory; private final ApprovalsUtil approvalsUtil; + private final ChangeMessagesUtil cmUtil; private final Branch.NameKey destBranch; private ProjectState destProject; @@ -173,6 +176,7 @@ public class MergeOp { final MergeFailSender.Factory mfsf, final PatchSetInfoFactory psif, final IdentifiedUser.GenericFactory iuf, final ChangeControl.GenericFactory changeControlFactory, + final ChangeUpdate.Factory updateFactory, final MergeQueue mergeQueue, @Assisted final Branch.NameKey branch, final ChangeHooks hooks, final AccountCache accountCache, final TagCache tagCache, @@ -182,7 +186,8 @@ public class MergeOp { final RequestScopePropagator requestScopePropagator, final ChangeIndexer indexer, final MergeValidators.Factory mergeValidatorsFactory, - final ApprovalsUtil approvalsUtil) { + final ApprovalsUtil approvalsUtil, + final ChangeMessagesUtil cmUtil) { repoManager = grm; schemaFactory = sf; notesFactory = nf; @@ -193,6 +198,7 @@ public class MergeOp { patchSetInfoFactory = psif; identifiedUserFactory = iuf; this.changeControlFactory = changeControlFactory; + this.updateFactory = updateFactory; this.mergeQueue = mergeQueue; this.hooks = hooks; this.accountCache = accountCache; @@ -204,6 +210,7 @@ public class MergeOp { this.indexer = indexer; this.mergeValidatorsFactory = mergeValidatorsFactory; this.approvalsUtil = approvalsUtil; + this.cmUtil = cmUtil; destBranch = branch; toMerge = ArrayListMultimap.create(); potentiallyStillSubmittable = new ArrayList<>(); @@ -224,7 +231,7 @@ public class MergeOp { } } - public void merge() throws MergeException { + public void merge() throws MergeException, NoSuchChangeException, IOException { setDestProject(); try { openSchema(); @@ -395,7 +402,7 @@ public class MergeOp { inserter = repo.newObjectInserter(); } - private RefUpdate openBranch() throws MergeException, OrmException { + private RefUpdate openBranch() throws MergeException, OrmException, NoSuchChangeException { try { final RefUpdate branchUpdate = repo.updateRef(destBranch.get()); if (branchUpdate.getOldObjectId() != null) { @@ -442,7 +449,7 @@ public class MergeOp { } private ListMultimap validateChangeList( - final List submitted) throws MergeException { + final List submitted) throws MergeException, NoSuchChangeException { final ListMultimap toSubmit = ArrayListMultimap.create(); @@ -658,7 +665,7 @@ public class MergeOp { hooks.doRefUpdatedHook(destBranch, branchUpdate, account); } - private void updateChangeStatus(final List submitted) { + private void updateChangeStatus(final List submitted) throws NoSuchChangeException { for (final Change c : submitted) { final CodeReviewCommit commit = commits.get(c.getId()); final CommitMergeStatus s = commit != null ? commit.getStatusCode() : null; @@ -824,7 +831,8 @@ public class MergeOp { } private void setMerged(Change c, ChangeMessage msg) - throws OrmException, IOException { + throws OrmException, IOException, NoSuchChangeException { + ChangeUpdate update = null; try { db.changes().beginTransaction(c.getId()); @@ -835,8 +843,13 @@ public class MergeOp { c = setMergedPatchSet(c.getId(), merged); PatchSetApproval submitter = approvalsUtil.getSubmitter(db, commit.notes(), merged); - addMergedMessage(submitter, msg); + ChangeControl control = commit.getControl(); + update = updateFactory.create(control, c.getLastUpdatedOn()); + // TODO(yyonas): we need to be able to change the author of the message + // is not the person for whom the change was made. addMergedMessage + // did this in the past. + cmUtil.addChangeMessage(db, update, msg); db.commit(); sendMergedEmail(c, submitter); @@ -853,6 +866,7 @@ public class MergeOp { db.rollback(); } indexer.index(db, c); + update.commit(); } private Change setMergedPatchSet(Change.Id changeId, final PatchSet.Id merged) @@ -881,16 +895,6 @@ public class MergeOp { }); } - private void addMergedMessage(PatchSetApproval submitter, ChangeMessage msg) - throws OrmException { - if (msg != null) { - if (submitter != null && msg.getAuthor() == null) { - msg.setAuthor(submitter.getAccountId()); - } - db.changeMessages().insert(Collections.singleton(msg)); - } - } - private void sendMergedEmail(final Change c, final PatchSetApproval from) { workQueue.getDefaultQueue() .submit(requestScopePropagator.wrap(new Runnable() { @@ -933,11 +937,11 @@ public class MergeOp { c, identifiedUserFactory.create(c.getOwner())); } - private void setNew(CodeReviewCommit c, ChangeMessage msg) { + private void setNew(CodeReviewCommit c, ChangeMessage msg) throws NoSuchChangeException, IOException { sendMergeFail(c.notes(), msg, true); } - private void setNew(Change c, ChangeMessage msg) throws OrmException { + private void setNew(Change c, ChangeMessage msg) throws OrmException, NoSuchChangeException, IOException { sendMergeFail(notesFactory.create(c), msg, true); } @@ -947,7 +951,8 @@ public class MergeOp { private RetryStatus getRetryStatus( @Nullable PatchSetApproval submitter, - ChangeMessage msg) { + ChangeMessage msg, + ChangeNotes notes) { if (submitter != null && TimeUtil.nowMs() - submitter.getGranted().getTime() > MAX_SUBMIT_WINDOW) { @@ -955,8 +960,7 @@ public class MergeOp { } try { - ChangeMessage last = Iterables.getLast(db.changeMessages().byChange( - msg.getPatchSetId().getParentKey()), null); + ChangeMessage last = Iterables.getLast(cmUtil.byChange(db, notes)); if (last != null) { if (Objects.equal(last.getAuthor(), msg.getAuthor()) && Objects.equal(last.getMessage(), msg.getMessage())) { @@ -975,7 +979,7 @@ public class MergeOp { } private void sendMergeFail(ChangeNotes notes, final ChangeMessage msg, - boolean makeNew) { + boolean makeNew) throws NoSuchChangeException, IOException { PatchSetApproval submitter = null; try { submitter = approvalsUtil.getSubmitter( @@ -985,7 +989,7 @@ public class MergeOp { } if (!makeNew) { - RetryStatus retryStatus = getRetryStatus(submitter, msg); + RetryStatus retryStatus = getRetryStatus(submitter, msg, notes); if (retryStatus == RetryStatus.RETRY_NO_MESSAGE) { return; } else if (retryStatus == RetryStatus.UNSUBMIT) { @@ -996,6 +1000,7 @@ public class MergeOp { final boolean setStatusNew = makeNew; final Change c = notes.getChange(); Change change = null; + ChangeUpdate update = null; try { db.changes().beginTransaction(c.getId()); try { @@ -1013,7 +1018,11 @@ public class MergeOp { return c; } }); - db.changeMessages().insert(Collections.singleton(msg)); + ChangeControl control = changeControl(change); + + //TODO(yyonas): atomic change is not propagated. + update = updateFactory.create(control, c.getLastUpdatedOn()); + cmUtil.addChangeMessage(db, update, msg); db.commit(); } finally { db.rollback(); @@ -1021,6 +1030,9 @@ public class MergeOp { } catch (OrmException err) { log.warn("Cannot record merge failure message", err); } + if (update != null) { + update.commit(); + } CheckedFuture indexFuture; if (change != null) { @@ -1083,7 +1095,7 @@ public class MergeOp { } } - private void abandonAllOpenChanges() { + private void abandonAllOpenChanges() throws NoSuchChangeException { Exception err = null; try { openSchema(); @@ -1105,8 +1117,13 @@ public class MergeOp { } private void abandonOneChange(Change change) throws OrmException, - IOException { + NoSuchChangeException, IOException { db.changes().beginTransaction(change.getId()); + + //TODO(dborowitz): support InternalUser in ChangeUpdate + ChangeControl control = changeControlFactory.controlFor(change, + identifiedUserFactory.create(change.getOwner())); + ChangeUpdate update = updateFactory.create(control); try { change = db.changes().atomicUpdate( change.getId(), @@ -1120,6 +1137,7 @@ public class MergeOp { return null; } }); + if (change != null) { ChangeMessage msg = new ChangeMessage( new ChangeMessage.Key( @@ -1129,12 +1147,15 @@ public class MergeOp { change.getLastUpdatedOn(), change.currentPatchSetId()); msg.setMessage("Project was deleted."); - db.changeMessages().insert(Collections.singleton(msg)); + + //TODO(yyonas): atomic change is not propagated. + cmUtil.addChangeMessage(db, update, msg); db.commit(); indexer.index(db, change); } } finally { db.rollback(); } + update.commit(); } -} +} \ No newline at end of file diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index ac0a95ed78..3267e5febc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -69,6 +69,7 @@ import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalCopier; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; @@ -97,6 +98,7 @@ import com.google.gerrit.server.mail.ReplacePatchSetSender; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectState; @@ -279,6 +281,7 @@ public class ReceiveCommits { private final ChangeHooks hooks; private final ApprovalsUtil approvalsUtil; private final ApprovalCopier approvalCopier; + private final ChangeMessagesUtil cmUtil; private final GitRepositoryManager repoManager; private final ProjectCache projectCache; private final String canonicalWebUrl; @@ -344,6 +347,7 @@ public class ReceiveCommits { final ChangeHooks hooks, final ApprovalsUtil approvalsUtil, final ApprovalCopier approvalCopier, + final ChangeMessagesUtil cmUtil, final ProjectCache projectCache, final GitRepositoryManager repoManager, final TagCache tagCache, @@ -383,6 +387,7 @@ public class ReceiveCommits { this.hooks = hooks; this.approvalsUtil = approvalsUtil; this.approvalCopier = approvalCopier; + this.cmUtil = cmUtil; this.projectCache = projectCache; this.repoManager = repoManager; this.canonicalWebUrl = canonicalWebUrl; @@ -576,54 +581,59 @@ public class ReceiveCommits { } for (final ReceiveCommand c : commands) { - if (c.getResult() == OK) { - switch (c.getType()) { - case CREATE: - if (isHead(c) || isConfig(c)) { - autoCloseChanges(c); + if (c.getResult() == OK) { + try { + switch (c.getType()) { + case CREATE: + if (isHead(c) || isConfig(c)) { + autoCloseChanges(c); + } + break; + + case UPDATE: // otherwise known as a fast-forward + tagCache.updateFastForward(project.getNameKey(), + c.getRefName(), + c.getOldId(), + c.getNewId()); + if (isHead(c) || isConfig(c)) { + autoCloseChanges(c); + } + break; + + case UPDATE_NONFASTFORWARD: + if (isHead(c) || isConfig(c)) { + autoCloseChanges(c); + } + break; + + case DELETE: + break; } - break; - case UPDATE: // otherwise known as a fast-forward - tagCache.updateFastForward(project.getNameKey(), - c.getRefName(), - c.getOldId(), - c.getNewId()); - if (isHead(c) || isConfig(c)) { - autoCloseChanges(c); + if (isConfig(c)) { + projectCache.evict(project); + ProjectState ps = projectCache.get(project.getNameKey()); + repoManager.setProjectDescription(project.getNameKey(), // + ps.getProject().getDescription()); } - break; - case UPDATE_NONFASTFORWARD: - if (isHead(c) || isConfig(c)) { - autoCloseChanges(c); + if (!MagicBranch.isMagicBranch(c.getRefName())) { + // We only fire gitRefUpdated for direct refs updates. + // Events for change refs are fired when they are created. + // + gitRefUpdated.fire(project.getNameKey(), c.getRefName(), + c.getOldId(), c.getNewId()); + hooks.doRefUpdatedHook( + new Branch.NameKey(project.getNameKey(), c.getRefName()), + c.getOldId(), + c.getNewId(), + currentUser.getAccount()); } - break; - - case DELETE: - break; + } catch (NoSuchChangeException e) { + c.setResult(REJECTED_OTHER_REASON, + "No such change: " + e.getMessage()); + } } - - if (isConfig(c)) { - projectCache.evict(project); - ProjectState ps = projectCache.get(project.getNameKey()); - repoManager.setProjectDescription(project.getNameKey(), // - ps.getProject().getDescription()); - } - - if (!MagicBranch.isMagicBranch(c.getRefName())) { - // We only fire gitRefUpdated for direct refs updates. - // Events for change refs are fired when they are created. - // - gitRefUpdated.fire(project.getNameKey(), c.getRefName(), - c.getOldId(), c.getNewId()); - hooks.doRefUpdatedHook( - new Branch.NameKey(project.getNameKey(), c.getRefName()), - c.getOldId(), - c.getNewId(), - currentUser.getAccount()); - } - } } closeProgress.end(); commandProgress.end(); @@ -1928,7 +1938,7 @@ public class ReceiveCommits { ListenableFuture future = changeUpdateExector.submit( requestScopePropagator.wrap(new Callable() { @Override - public PatchSet.Id call() throws OrmException, IOException { + public PatchSet.Id call() throws OrmException, IOException, NoSuchChangeException { try { if (caller == Thread.currentThread()) { return insertPatchSet(db); @@ -1993,7 +2003,7 @@ public class ReceiveCommits { new ChangeMessage(new ChangeMessage.Key(change.getId(), ChangeUtil .messageUUID(db)), me, newPatchSet.getCreatedOn(), newPatchSet.getId()); msg.setMessage("Uploaded patch set " + newPatchSet.getPatchSetId() + "."); - db.changeMessages().insert(Collections.singleton(msg)); + cmUtil.addChangeMessage(db, update, msg); if (mergedIntoRef == null) { // Change should be new, so it can go through review again. @@ -2049,7 +2059,7 @@ public class ReceiveCommits { if (mergedIntoRef != null) { // Change was already submitted to a branch, close it. // - markChangeMergedByPush(db, this); + markChangeMergedByPush(db, this, changeCtl); } if (cmd.getResult() == NOT_ATTEMPTED) { @@ -2256,7 +2266,7 @@ public class ReceiveCommits { return true; } - private void autoCloseChanges(final ReceiveCommand cmd) { + private void autoCloseChanges(final ReceiveCommand cmd) throws NoSuchChangeException { final RevWalk rw = rp.getRevWalk(); try { rw.reset(); @@ -2355,7 +2365,7 @@ public class ReceiveCommits { result.newPatchSet = ps; result.info = patchSetInfoFactory.get(commit, psi); result.mergedIntoRef = refName; - markChangeMergedByPush(db, result); + markChangeMergedByPush(db, result, result.changeCtl); hooks.doChangeMergedHook( change, currentUser.getAccount(), result.newPatchSet, db); sendMergedEmail(result); @@ -2383,11 +2393,13 @@ public class ReceiveCommits { return r; } - private void markChangeMergedByPush(ReviewDb db, final ReplaceRequest result) - throws OrmException, IOException { + private void markChangeMergedByPush(ReviewDb db, final ReplaceRequest result, + ChangeControl control) throws OrmException, IOException { Change.Id id = result.change.getId(); db.changes().beginTransaction(id); Change change; + + ChangeUpdate update; try { change = db.changes().atomicUpdate(id, new AtomicUpdate() { @Override @@ -2420,12 +2432,15 @@ public class ReceiveCommits { result.info.getKey()); msg.setMessage(msgBuf.toString()); - db.changeMessages().insert(Collections.singleton(msg)); + update = updateFactory.create(control, change.getLastUpdatedOn()); + + cmUtil.addChangeMessage(db, update, msg); db.commit(); } finally { db.rollback(); } indexer.index(db, change); + update.commit(); } private void sendMergedEmail(final ReplaceRequest result) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index e2f56d1f69..f8bfd94140 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -81,7 +81,7 @@ public class ChangeNotes extends VersionedMetaData { } }); - private static final Ordering MESSAGE_BY_TIME = + public static final Ordering MESSAGE_BY_TIME = Ordering.natural().onResultOf( new Function() { @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java index 53210463fb..db72def0c1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java @@ -35,17 +35,21 @@ public class NotesMigration { Config cfg = new Config(); cfg.setBoolean("notedb", null, "write", true); cfg.setBoolean("notedb", "patchSetApprovals", "read", true); + cfg.setBoolean("notedb", "changeMessages", "read", true); return new NotesMigration(cfg); } private final boolean write; private final boolean readPatchSetApprovals; + private final boolean readChangeMessages; @Inject NotesMigration(@GerritServerConfig Config cfg) { write = cfg.getBoolean("notedb", null, "write", false); readPatchSetApprovals = cfg.getBoolean("notedb", "patchSetApprovals", "read", false); + readChangeMessages = + cfg.getBoolean("notedb", "changeMessages", "read", false); } public boolean write() { @@ -55,4 +59,8 @@ public class NotesMigration { public boolean readPatchSetApprovals() { return readPatchSetApprovals; } + + public boolean readChangeMessages() { + return readChangeMessages; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index da19d19818..ed640154b8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -32,6 +32,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.git.GitRepositoryManager; @@ -152,7 +153,8 @@ public class ChangeData { * @return instance for testing. */ static ChangeData createForTest(Change.Id id, int currentPatchSetId) { - ChangeData cd = new ChangeData(null, null, null, null, null, null, null, null, id); + ChangeData cd = new ChangeData(null, null, null, null, null, + null, null, null, null, id); cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId)); return cd; } @@ -163,6 +165,7 @@ public class ChangeData { private final IdentifiedUser.GenericFactory userFactory; private final ChangeNotes.Factory notesFactory; private final ApprovalsUtil approvalsUtil; + private final ChangeMessagesUtil cmUtil; private final PatchListCache patchListCache; private final NotesMigration notesMigration; private final Change.Id legacyId; @@ -190,6 +193,7 @@ public class ChangeData { IdentifiedUser.GenericFactory userFactory, ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, + ChangeMessagesUtil cmUtil, PatchListCache patchListCache, NotesMigration notesMigration, @Assisted ReviewDb db, @@ -200,6 +204,7 @@ public class ChangeData { this.userFactory = userFactory; this.notesFactory = notesFactory; this.approvalsUtil = approvalsUtil; + this.cmUtil = cmUtil; this.patchListCache = patchListCache; this.notesMigration = notesMigration; legacyId = id; @@ -212,6 +217,7 @@ public class ChangeData { IdentifiedUser.GenericFactory userFactory, ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, + ChangeMessagesUtil cmUtil, PatchListCache patchListCache, NotesMigration notesMigration, @Assisted ReviewDb db, @@ -222,6 +228,7 @@ public class ChangeData { this.userFactory = userFactory; this.notesFactory = notesFactory; this.approvalsUtil = approvalsUtil; + this.cmUtil = cmUtil; this.patchListCache = patchListCache; this.notesMigration = notesMigration; legacyId = c.getId(); @@ -235,6 +242,7 @@ public class ChangeData { IdentifiedUser.GenericFactory userFactory, ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, + ChangeMessagesUtil cmUtil, PatchListCache patchListCache, NotesMigration notesMigration, @Assisted ReviewDb db, @@ -245,6 +253,7 @@ public class ChangeData { this.userFactory = userFactory; this.notesFactory = notesFactory; this.approvalsUtil = approvalsUtil; + this.cmUtil = cmUtil; this.patchListCache = patchListCache; this.notesMigration = notesMigration; legacyId = c.getChange().getId(); @@ -521,7 +530,7 @@ public class ChangeData { public List messages() throws OrmException { if (messages == null) { - messages = db.changeMessages().byChange(legacyId).toList(); + messages = cmUtil.byChange(db, notes); } return messages; }