diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/BUCK b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/BUCK new file mode 100644 index 0000000000..f46a8e55da --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/BUCK @@ -0,0 +1,7 @@ +include_defs('//gerrit-acceptance-tests/tests.defs') + +acceptance_tests( + group = 'server-notedb', + srcs = glob(['*IT.java']), + labels = ['notedb', 'server'], +) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java new file mode 100644 index 0000000000..3e269515b6 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java @@ -0,0 +1,126 @@ +// Copyright (C) 2016 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.acceptance.server.notedb; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; +import static com.google.gerrit.common.TimeUtil.roundToSecond; +import static com.google.gerrit.testutil.GerritServerTests.isNoteDbTestEnabled; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.ChangeRebuilder; +import com.google.inject.Inject; + +import org.junit.Before; +import org.junit.Test; + +import java.util.Map; + +public class ChangeRebuilderIT extends AbstractDaemonTest { + @Inject + private ChangeRebuilder rebuilder; + + @Before + public void setUp() { + assume().that(isNoteDbTestEnabled()).isFalse(); + notesMigration.setAllEnabled(false); + } + + @Test + public void changeFields() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getPatchSetId().getParentKey(); + gApi.changes().id(id.get()).topic(name("a-topic")); + Change old = db.changes().get(id); + + rebuild(id); + + assertChangeEqual(old, notesFactory.create(db, project, id).getChange()); + } + + @Test + public void patchSets() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getPatchSetId().getParentKey(); + r = amendChange(r.getChangeId()); + + PatchSet ps1 = db.patchSets().get(new PatchSet.Id(id, 1)); + PatchSet ps2 = db.patchSets().get(new PatchSet.Id(id, 2)); + + rebuild(id); + + ChangeNotes notes = notesFactory.create(db, project, id); + Map patchSets = notes.getPatchSets(); + assertThat(patchSets.keySet()).containsExactly(ps1.getId(), ps2.getId()) + .inOrder(); + + assertPatchSetEqual(ps1, patchSets.get(ps1.getId())); + assertPatchSetEqual(ps2, patchSets.get(ps2.getId())); + } + + private void rebuild(Change.Id... changeIds) throws Exception { + notesMigration.setWriteChanges(true); + for (Change.Id id : changeIds) { + rebuilder.rebuild(db, id); + } + notesMigration.setReadChanges(true); + } + + private static void assertChangeEqual(Change expectedReviewDb, + Change actualNoteDb) { + assertThat(actualNoteDb.getId()).isEqualTo(expectedReviewDb.getId()); + assertThat(actualNoteDb.getKey()).isEqualTo(expectedReviewDb.getKey()); + + // TODO(dborowitz): actualNoteDb's timestamps should come from notedb, currently + // they're read from reviewdb. + assertThat(roundToSecond(actualNoteDb.getCreatedOn())) + .isEqualTo(roundToSecond(expectedReviewDb.getCreatedOn())); + assertThat(roundToSecond(actualNoteDb.getLastUpdatedOn())) + .isEqualTo(roundToSecond(expectedReviewDb.getLastUpdatedOn())); + assertThat(actualNoteDb.getOwner()).isEqualTo(expectedReviewDb.getOwner()); + assertThat(actualNoteDb.getDest()).isEqualTo(expectedReviewDb.getDest()); + assertThat(actualNoteDb.getStatus()) + .isEqualTo(expectedReviewDb.getStatus()); + assertThat(actualNoteDb.currentPatchSetId()) + .isEqualTo(expectedReviewDb.currentPatchSetId()); + assertThat(actualNoteDb.getSubject()) + .isEqualTo(expectedReviewDb.getSubject()); + assertThat(actualNoteDb.getTopic()).isEqualTo(expectedReviewDb.getTopic()); + assertThat(actualNoteDb.getOriginalSubject()) + .isEqualTo(expectedReviewDb.getOriginalSubject()); + assertThat(actualNoteDb.getSubmissionId()) + .isEqualTo(expectedReviewDb.getSubmissionId()); + } + + private static void assertPatchSetEqual(PatchSet expectedReviewDb, + PatchSet actualNoteDb) { + assertThat(actualNoteDb.getId()).isEqualTo(expectedReviewDb.getId()); + assertThat(actualNoteDb.getRevision()) + .isEqualTo(expectedReviewDb.getRevision()); + assertThat(actualNoteDb.getUploader()) + .isEqualTo(expectedReviewDb.getUploader()); + assertThat(actualNoteDb.getCreatedOn()) + .isEqualTo(roundToSecond(expectedReviewDb.getCreatedOn())); + assertThat(actualNoteDb.isDraft()).isEqualTo(expectedReviewDb.isDraft()); + assertThat(actualNoteDb.getGroups()) + .isEqualTo(expectedReviewDb.getGroups()); + assertThat(actualNoteDb.getPushCertificate()) + .isEqualTo(expectedReviewDb.getPushCertificate()); + } +} diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java index 1a43c38bef..190de6d59f 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java @@ -108,49 +108,43 @@ public class RebuildNotedb extends SiteProgram { System.out.println("Rebuilding the notedb"); ChangeRebuilder rebuilder = sysInjector.getInstance(ChangeRebuilder.class); - Multimap changesByProject = getChangesByProject(); - final AtomicBoolean ok = new AtomicBoolean(true); + Multimap changesByProject = + getChangesByProject(); + AtomicBoolean ok = new AtomicBoolean(true); Stopwatch sw = Stopwatch.createStarted(); GitRepositoryManager repoManager = sysInjector.getInstance(GitRepositoryManager.class); - final Project.NameKey allUsersName = + Project.NameKey allUsersName = sysInjector.getInstance(AllUsersName.class); try (Repository allUsersRepo = repoManager.openMetadataRepository(allUsersName)) { deleteRefs(RefNames.REFS_DRAFT_COMMENTS, allUsersRepo); deleteRefs(RefNames.REFS_STARRED_CHANGES, allUsersRepo); - for (final Project.NameKey project : changesByProject.keySet()) { - try (Repository repo = repoManager.openMetadataRepository(project)) { - final BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate(); - final BatchRefUpdate bruAllUsers = - allUsersRepo.getRefDatabase().newBatchUpdate(); + for (Project.NameKey project : changesByProject.keySet()) { + try { List> futures = Lists.newArrayList(); // Here, we elide the project name to 50 characters to ensure that // the whole monitor line for a project fits on one line (<80 chars). final MultiProgressMonitor mpm = new MultiProgressMonitor(System.out, FormatUtil.elide(project.get(), 50)); - final Task doneTask = + Task doneTask = mpm.beginSubTask("done", changesByProject.get(project).size()); - final Task failedTask = + Task failedTask = mpm.beginSubTask("failed", MultiProgressMonitor.UNKNOWN); - for (final Change c : changesByProject.get(project)) { - final ListenableFuture future = rebuilder.rebuildAsync(c, - executor, bru, bruAllUsers, repo, allUsersRepo); + for (Change.Id id : changesByProject.get(project)) { + ListenableFuture future = rebuilder.rebuildAsync(id, executor); futures.add(future); future.addListener( - new RebuildListener(c.getId(), future, ok, doneTask, failedTask), + new RebuildListener(id, future, ok, doneTask, failedTask), MoreExecutors.directExecutor()); } mpm.waitFor(Futures.transformAsync(Futures.successfulAsList(futures), new AsyncFunction, Void>() { - @Override - public ListenableFuture apply(List input) - throws Exception { - execute(bru, repo); - execute(bruAllUsers, allUsersRepo); + @Override + public ListenableFuture apply(List input) { mpm.end(); return Futures.immediateFuture(null); } @@ -218,17 +212,17 @@ public class RebuildNotedb extends SiteProgram { } } - private Multimap getChangesByProject() + private Multimap getChangesByProject() throws OrmException { // Memorize all changes so we can close the db connection and allow // rebuilder threads to use the full connection pool. SchemaFactory schemaFactory = sysInjector.getInstance(Key.get( new TypeLiteral>() {})); - Multimap changesByProject = + Multimap changesByProject = ArrayListMultimap.create(); try (ReviewDb db = schemaFactory.open()) { for (Change c : db.changes().all()) { - changesByProject.put(c.getProject(), c); + changesByProject.put(c.getProject(), c.getId()); } return changesByProject; } 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 index 94973f2833..07d4326ad0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeMessagesUtil.java @@ -14,6 +14,8 @@ package com.google.gerrit.server; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.annotations.VisibleForTesting; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; @@ -27,6 +29,7 @@ import com.google.inject.Singleton; import java.util.Collections; import java.util.List; +import java.util.Objects; /** * Utility functions to manipulate ChangeMessages. @@ -68,6 +71,11 @@ public class ChangeMessagesUtil { public void addChangeMessage(ReviewDb db, ChangeUpdate update, ChangeMessage changeMessage) throws OrmException { + checkState( + Objects.equals(changeMessage.getAuthor(), + update.getUser().getAccountId()), + "cannot store change message of %s in update of %s", + changeMessage.getAuthor(), update.getUser().getAccountId()); update.setChangeMessage(changeMessage.getMessage()); db.changeMessages().insert(Collections.singleton(changeMessage)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java index c5440aaf7f..cb7e65b4f3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java @@ -288,30 +288,14 @@ public class PatchLineCommentsUtil { return sort(comments); } - public void insertComments(ReviewDb db, ChangeUpdate update, + public void putComments(ReviewDb db, ChangeUpdate update, Iterable comments) throws OrmException { for (PatchLineComment c : comments) { - update.insertComment(c); - } - db.patchComments().insert(comments); - } - - public void upsertComments(ReviewDb db, ChangeUpdate update, - Iterable comments) throws OrmException { - for (PatchLineComment c : comments) { - update.upsertComment(c); + update.putComment(c); } db.patchComments().upsert(comments); } - public void updateComments(ReviewDb db, ChangeUpdate update, - Iterable comments) throws OrmException { - for (PatchLineComment c : comments) { - update.updateComment(c); - } - db.patchComments().update(comments); - } - public void deleteComments(ReviewDb db, ChangeUpdate update, Iterable comments) throws OrmException { for (PatchLineComment c : comments) { 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 2c93ae38b8..d8fbd3bc49 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 @@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; @@ -85,18 +86,19 @@ public class Abandon implements RestModifyView, if (!control.canAbandon(dbProvider.get())) { throw new AuthException("abandon not permitted"); } - Change change = abandon(control, input.message, - control.getUser().asIdentifiedUser().getAccount()); + Change change = abandon(control, input.message); return json.create(ChangeJson.NO_OPTIONS).format(change); } - public Change abandon(ChangeControl control, - final String msgTxt, final Account account) + public Change abandon(ChangeControl control, String msgTxt) throws RestApiException, UpdateException { + CurrentUser user = control.getUser(); + Account account = user.isIdentifiedUser() + ? user.asIdentifiedUser().getAccount() + : null; Op op = new Op(msgTxt, account); try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(), - control.getProject().getNameKey(), control.getUser(), - TimeUtil.nowTs())) { + control.getProject().getNameKey(), user, TimeUtil.nowTs())) { u.addOp(control.getId(), op).execute(); } return op.change; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java index 65d250ab44..efb2d4d44d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java @@ -15,7 +15,7 @@ package com.google.gerrit.server.change; import com.google.gerrit.extensions.restapi.ResourceConflictException; -import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.config.ChangeCleanupConfig; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.query.QueryParseException; @@ -37,7 +37,7 @@ public class AbandonUtil { private static final Logger log = LoggerFactory.getLogger(AbandonUtil.class); private final ChangeCleanupConfig cfg; - private final IdentifiedUser.GenericFactory identifiedUserFactory; + private final InternalUser.Factory internalUserFactory; private final QueryProcessor queryProcessor; private final ChangeQueryBuilder queryBuilder; private final Abandon abandon; @@ -45,12 +45,12 @@ public class AbandonUtil { @Inject AbandonUtil( ChangeCleanupConfig cfg, - IdentifiedUser.GenericFactory identifiedUserFactory, + InternalUser.Factory internalUserFactory, QueryProcessor queryProcessor, ChangeQueryBuilder queryBuilder, Abandon abandon) { this.cfg = cfg; - this.identifiedUserFactory = identifiedUserFactory; + this.internalUserFactory = internalUserFactory; this.queryProcessor = queryProcessor; this.queryBuilder = queryBuilder; this.abandon = abandon; @@ -73,7 +73,7 @@ public class AbandonUtil { int count = 0; for (ChangeData cd : changesToAbandon) { try { - abandon.abandon(changeControl(cd), cfg.getAbandonMessage(), null); + abandon.abandon(changeControl(cd), cfg.getAbandonMessage()); count++; } catch (ResourceConflictException e) { // Change was already merged or abandoned. @@ -91,7 +91,6 @@ public class AbandonUtil { } private ChangeControl changeControl(ChangeData cd) throws OrmException { - return cd.changeControl( - identifiedUserFactory.create(cd.change().getOwner())); + return cd.changeControl(internalUserFactory.create()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java index 8e124b20ce..9ae9496a8f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java @@ -124,7 +124,7 @@ public class CreateDraftComment implements RestModifyView // TODO(dborowitz): Currently doesn't work for PUBLISH_ALL_REVISIONS with // notedb. plcUtil.deleteComments(ctx.getDb(), u, del); - plcUtil.upsertComments(ctx.getDb(), u, ups); + plcUtil.putComments(ctx.getDb(), u, ups); comments.addAll(ups); return !del.isEmpty() || !ups.isEmpty(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java index 00246c6f26..0707dc7584 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java @@ -142,14 +142,14 @@ public class PutDraftComment implements RestModifyView commands = new LinkedHashMap<>(); + private final Map oldIds = new HashMap<>(); - /** @return true if no commands have been added. */ public boolean isEmpty() { return commands.isEmpty(); } @@ -64,6 +69,43 @@ public class ChainedReceiveCommands { old.getOldId(), cmd.getNewId(), cmd.getRefName())); } + /** + * Get the latest value of a ref according to this sequence of commands. + *

+ * Once the value for a ref is read once, it is cached in this instance, so + * that multiple callers using this instance for lookups see a single + * consistent snapshot. + * + * @param repo repository to read from, if result is not cached. + * @param refName name of the ref. + * @return value of the ref, taking into account commands that have already + * been added to this instance. Null if the ref is deleted, matching the + * behavior of {@link Repository#exactRef(String)}. + */ + public ObjectId getObjectId(Repository repo, String refName) + throws IOException { + ReceiveCommand cmd = commands.get(refName); + if (cmd != null) { + return zeroToNull(cmd.getNewId()); + } + ObjectId old = oldIds.get(refName); + if (old != null) { + return zeroToNull(old); + } + Ref ref = repo.exactRef(refName); + ObjectId id = ref != null ? ref.getObjectId() : null; + // Cache missing ref as zeroId to match value in commands map. + oldIds.put(refName, firstNonNull(id, ObjectId.zeroId())); + return id; + } + + private static ObjectId zeroToNull(ObjectId id) { + if (id == null || id.equals(ObjectId.zeroId())) { + return null; + } + return id; + } + /** * Add commands from this instance to a native JGit batch update. *

@@ -74,7 +116,6 @@ public class ChainedReceiveCommands { * @param bru batch update */ public void addTo(BatchRefUpdate bru) { - checkState(!isEmpty(), "no commands to add"); for (ReceiveCommand cmd : commands.values()) { bru.addCommand(cmd); } 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 21db483027..a79d8626ef 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 @@ -52,6 +52,7 @@ 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.InternalUser; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; import com.google.gerrit.server.git.strategy.SubmitStrategy; @@ -309,7 +310,7 @@ public class MergeOp implements AutoCloseable { private final ChangeMessagesUtil cmUtil; private final BatchUpdate.Factory batchUpdateFactory; private final GitRepositoryManager repoManager; - private final IdentifiedUser.GenericFactory identifiedUserFactory; + private final InternalUser.Factory internalUserFactory; private final MergeSuperSet mergeSuperSet; private final MergeValidators.Factory mergeValidatorsFactory; private final ProjectCache projectCache; @@ -341,7 +342,7 @@ public class MergeOp implements AutoCloseable { MergeOp(ChangeMessagesUtil cmUtil, BatchUpdate.Factory batchUpdateFactory, GitRepositoryManager repoManager, - IdentifiedUser.GenericFactory identifiedUserFactory, + InternalUser.Factory internalUserFactory, MergeSuperSet mergeSuperSet, MergeValidators.Factory mergeValidatorsFactory, ProjectCache projectCache, @@ -351,7 +352,7 @@ public class MergeOp implements AutoCloseable { this.cmUtil = cmUtil; this.batchUpdateFactory = batchUpdateFactory; this.repoManager = repoManager; - this.identifiedUserFactory = identifiedUserFactory; + this.internalUserFactory = internalUserFactory; this.mergeSuperSet = mergeSuperSet; this.mergeValidatorsFactory = mergeValidatorsFactory; this.projectCache = projectCache; @@ -898,10 +899,8 @@ public class MergeOp implements AutoCloseable { Project.NameKey destProject) { try { for (ChangeData cd : internalChangeQuery.byProjectOpen(destProject)) { - //TODO: Use InternalUser instead of change owner try (BatchUpdate bu = batchUpdateFactory.create(db, destProject, - identifiedUserFactory.create(cd.change().getOwner()), - TimeUtil.nowTs())) { + internalUserFactory.create(), TimeUtil.nowTs())) { bu.addOp(cd.getId(), new BatchUpdate.Op() { @Override public boolean updateChange(ChangeContext ctx) throws OrmException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index 296a093fb3..e1d309c4a8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java @@ -20,49 +20,52 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.MetaDataUpdate; -import com.google.gerrit.server.git.VersionedMetaData; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; -import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; import java.util.Date; /** A single delta related to a specific patch-set of a change. */ -public abstract class AbstractChangeUpdate extends VersionedMetaData { +public abstract class AbstractChangeUpdate { protected final NotesMigration migration; protected final GitRepositoryManager repoManager; - protected final MetaDataUpdate.User updateFactory; protected final ChangeControl ctl; protected final String anonymousCowardName; - protected final PersonIdent serverIdent; protected final Date when; - protected PatchSet.Id psId; + private final PersonIdent serverIdent; - AbstractChangeUpdate(NotesMigration migration, + protected PatchSet.Id psId; + private ObjectId result; + + protected AbstractChangeUpdate(NotesMigration migration, GitRepositoryManager repoManager, - MetaDataUpdate.User updateFactory, ChangeControl ctl, + ChangeControl ctl, PersonIdent serverIdent, String anonymousCowardName, Date when) { this.migration = migration; this.repoManager = repoManager; - this.updateFactory = updateFactory; this.ctl = ctl; this.serverIdent = serverIdent; this.anonymousCowardName = anonymousCowardName; this.when = when; + checkArgument( + (ctl.getUser() instanceof IdentifiedUser) + || (ctl.getUser() instanceof InternalUser), + "user must be IdentifiedUser or InternalUser: %s", ctl.getUser()); } public ChangeNotes getChangeNotes() { @@ -77,8 +80,8 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData { return when; } - public IdentifiedUser getUser() { - return ctl.getUser().asIdentifiedUser(); + public CurrentUser getUser() { + return ctl.getUser(); } public PatchSet.Id getPatchSetId() { @@ -90,85 +93,15 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData { this.psId = psId; } - private void load() throws IOException { - if (migration.writeChanges() && getRevision() == null) { - try (Repository repo = repoManager.openMetadataRepository(getProjectName())) { - load(repo); - } catch (ConfigInvalidException e) { - throw new IOException(e); - } + private PersonIdent newAuthorIdent() { + CurrentUser u = getUser(); + if (u instanceof IdentifiedUser) { + return ChangeNoteUtil.newIdent(u.asIdentifiedUser().getAccount(), when, + serverIdent, anonymousCowardName); + } else if (u instanceof InternalUser) { + return serverIdent; } - } - - public void setInserter(ObjectInserter inserter) { - this.inserter = inserter; - } - - @Override - public BatchMetaDataUpdate openUpdate(MetaDataUpdate update) throws IOException { - throw new UnsupportedOperationException("use openUpdate()"); - } - - public BatchMetaDataUpdate openUpdate() throws IOException { - return openUpdateInBatch(null); - } - - public BatchMetaDataUpdate openUpdateInBatch(BatchRefUpdate bru) - throws IOException { - if (migration.writeChanges()) { - load(); - Project.NameKey p = getProjectName(); - MetaDataUpdate md = updateFactory.create( - p, repoManager.openMetadataRepository(p), getUser(), bru); - md.setAllowEmpty(true); - return super.openUpdate(md); - } - return new BatchMetaDataUpdate() { - @Override - public void write(CommitBuilder commit) { - // Do nothing. - } - - @Override - public void write(VersionedMetaData config, CommitBuilder commit) { - // Do nothing. - } - - @Override - public RevCommit createRef(String refName) { - return null; - } - - @Override - public void removeRef(String refName) { - // Do nothing. - } - - @Override - public RevCommit commit() { - return null; - } - - @Override - public RevCommit commitAt(ObjectId revision) { - return null; - } - - @Override - public void close() { - // Do nothing. - } - }; - } - - @Override - public RevCommit commit(MetaDataUpdate md) throws IOException { - throw new UnsupportedOperationException("use commit()"); - } - - @Override - protected void onLoad() throws IOException, ConfigInvalidException { - //Do nothing; just reads the current revision. + throw new IllegalStateException(); } protected PersonIdent newIdent(Account author, Date when) { @@ -176,10 +109,6 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData { anonymousCowardName); } - /** Writes commit to a BatchMetaDataUpdate without committing the batch. */ - public abstract void writeCommit(BatchMetaDataUpdate batch) - throws OrmException, IOException; - /** Whether no updates have been done. */ public abstract boolean isEmpty(); @@ -188,4 +117,71 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData { * which is not necessarily the same as the change's project. */ protected abstract Project.NameKey getProjectName(); + + protected abstract String getRefName(); + + /** + * Apply this update to the given inserter. + * + * @param rw walk for reading back any objects needed for the update. + * @param ins inserter to write to; callers should not flush. + * @param curr the current tip of the branch prior to this update. + * @return commit ID produced by inserting this update's commit, or null if + * this update is a no-op and should be skipped. The zero ID is a valid + * return value, and indicates the ref should be deleted. + * @throws OrmException if a Gerrit-level error occurred. + * @throws IOException if a lower-level error occurred. + */ + final ObjectId apply(RevWalk rw, ObjectInserter ins, ObjectId curr) + throws OrmException, IOException { + if (isEmpty()) { + return null; + } + ObjectId z = ObjectId.zeroId(); + CommitBuilder cb = applyImpl(rw, ins, curr); + if (cb == null) { + result = z; + return z; // Impl intends to delete the ref. + } + cb.setAuthor(newAuthorIdent()); + cb.setCommitter(new PersonIdent(serverIdent, when)); + if (!curr.equals(z)) { + cb.setParentId(curr); + } else { + cb.setParentIds(); // Ref is currently nonexistent, commit has no parents. + } + if (cb.getTreeId() == null) { + if (curr.equals(z)) { + cb.setTreeId(emptyTree(ins)); // No parent, assume empty tree. + } else { + RevCommit p = rw.parseCommit(curr); + cb.setTreeId(p.getTree()); // Copy tree from parent. + } + } + result = ins.insert(cb); + return result; + } + + /** + * Create a commit containing the contents of this update. + * + * @param ins inserter to write to; callers should not flush. + * @return a new commit builder representing this commit, or null to indicate + * the meta ref should be deleted as a result of this update. The parent, + * author, and committer fields in the return value are always + * overwritten. The tree ID may be unset by this method, which indicates + * to the caller that it should be copied from the parent commit. + * @throws OrmException if a Gerrit-level error occurred. + * @throws IOException if a lower-level error occurred. + */ + protected abstract CommitBuilder applyImpl(RevWalk rw, ObjectInserter ins, + ObjectId curr) throws OrmException, IOException; + + ObjectId getResult() { + return result; + } + + private static ObjectId emptyTree(ObjectInserter ins) throws IOException { + return ins.insert(Constants.OBJ_TREE, new byte[] {}); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index bf0ab8134e..06650920cc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -14,16 +14,16 @@ package com.google.gerrit.server.notedb; +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 static com.google.gerrit.server.notedb.CommentsInNotesUtil.addCommentToMap; +import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; -import com.google.common.collect.ListMultimap; -import com.google.common.collect.Lists; +import com.google.auto.value.AutoValue; import com.google.common.collect.Sets; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.PatchLineComment; -import com.google.gerrit.reviewdb.client.PatchLineComment.Status; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RevId; @@ -32,7 +32,6 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import com.google.inject.assistedinject.Assisted; @@ -41,18 +40,17 @@ import com.google.inject.assistedinject.AssistedInject; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.notes.NoteMap; -import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; -import java.util.ArrayList; import java.util.Date; import java.util.HashMap; -import java.util.List; +import java.util.HashSet; import java.util.Map; import java.util.Set; -import java.util.concurrent.atomic.AtomicBoolean; /** * A single delta to apply atomically to a change. @@ -68,14 +66,23 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { ChangeDraftUpdate create(ChangeControl ctl, Date when); } + @AutoValue + static abstract class Key { + abstract RevId revId(); + abstract PatchLineComment.Key key(); + } + + private static Key key(PatchLineComment c) { + return new AutoValue_ChangeDraftUpdate_Key(c.getRevId(), c.getKey()); + } + private final AllUsersName draftsProject; private final Account.Id accountId; private final CommentsInNotesUtil commentsUtil; - private final ChangeNotes changeNotes; - private final DraftCommentNotes draftNotes; - private List upsertComments; - private List deleteComments; + // TODO: can go back to a list? + private Map put; + private Set delete; @AssistedInject private ChangeDraftUpdate( @@ -83,187 +90,129 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { @AnonymousCowardName String anonymousCowardName, GitRepositoryManager repoManager, NotesMigration migration, - MetaDataUpdate.User updateFactory, - DraftCommentNotes.Factory draftNotesFactory, AllUsersName allUsers, CommentsInNotesUtil commentsUtil, @Assisted ChangeControl ctl, - @Assisted Date when) throws OrmException { - super(migration, repoManager, updateFactory, ctl, serverIdent, - anonymousCowardName, when); + @Assisted Date when) { + super(migration, repoManager, ctl, serverIdent, anonymousCowardName, when); this.draftsProject = allUsers; this.commentsUtil = commentsUtil; checkState(ctl.getUser().isIdentifiedUser(), "Current user must be identified"); IdentifiedUser user = ctl.getUser().asIdentifiedUser(); this.accountId = user.getAccountId(); - this.changeNotes = getChangeNotes().load(); - this.draftNotes = draftNotesFactory.create(ctl.getId(), - user.getAccountId()); - this.upsertComments = Lists.newArrayList(); - this.deleteComments = Lists.newArrayList(); + this.put = new HashMap<>(); + this.delete = new HashSet<>(); } - public void insertComment(PatchLineComment c) throws OrmException { + public void putComment(PatchLineComment c) { verifyComment(c); - checkArgument(c.getStatus() == Status.DRAFT, + checkArgument(c.getStatus() == PatchLineComment.Status.DRAFT, "Cannot insert a published comment into a ChangeDraftUpdate"); - if (migration.readChanges()) { - checkArgument(!changeNotes.containsComment(c), - "A comment already exists with the same key," - + " so the following comment cannot be inserted: %s", c); - } - upsertComments.add(c); + put.put(key(c), c); } - public void upsertComment(PatchLineComment c) { + public void deleteComment(PatchLineComment c) { verifyComment(c); - checkArgument(c.getStatus() == Status.DRAFT, - "Cannot upsert a published comment into a ChangeDraftUpdate"); - upsertComments.add(c); + delete.add(key(c)); } - public void updateComment(PatchLineComment c) throws OrmException { - verifyComment(c); - checkArgument(c.getStatus() == Status.DRAFT, - "Cannot update a published comment into a ChangeDraftUpdate"); - // Here, we check to see if this comment existed previously as a draft. - // However, this could cause a race condition if there is a delete and an - // update operation happening concurrently (or two deletes) and they both - // believe that the comment exists. If a delete happens first, then - // the update will fail. However, this is an acceptable risk since the - // caller wanted the comment deleted anyways, so the end result will be the - // same either way. - if (migration.readChanges()) { - checkArgument(draftNotes.load().containsComment(c), - "Cannot update this comment because it didn't exist previously"); - } - upsertComments.add(c); - } - - public void deleteComment(PatchLineComment c) throws OrmException { - verifyComment(c); - // See the comment above about potential race condition. - if (migration.readChanges()) { - checkArgument(draftNotes.load().containsComment(c), - "Cannot delete this comment because it didn't previously exist as a" - + " draft"); - } - if (migration.writeChanges()) { - if (draftNotes.load().containsComment(c)) { - deleteComments.add(c); - } - } - } - - /** - * Deletes a PatchLineComment from the list of drafts only if it existed - * previously as a draft. If it wasn't a draft previously, this is a no-op. - */ - public void deleteCommentIfPresent(PatchLineComment c) throws OrmException { - if (draftNotes.load().containsComment(c)) { - verifyComment(c); - deleteComments.add(c); - } + public void deleteComment(RevId revId, PatchLineComment.Key key) { + delete.add(new AutoValue_ChangeDraftUpdate_Key(revId, key)); } private void verifyComment(PatchLineComment comment) { - if (migration.writeChanges()) { - checkArgument(comment.getRevId() != null); - } checkArgument(comment.getAuthor().equals(accountId), "The author for the following comment does not match the author of" + " this ChangeDraftUpdate (%s): %s", accountId, comment); } /** @return the tree id for the updated tree */ - private ObjectId storeCommentsInNotes(AtomicBoolean removedAllComments) - throws OrmException, IOException { - if (isEmpty()) { - return null; + private ObjectId storeCommentsInNotes(RevWalk rw, ObjectInserter ins, + ObjectId curr) throws ConfigInvalidException, OrmException, IOException { + RevisionNoteMap rnm = getRevisionNoteMap(rw, curr); + Set updatedRevs = + Sets.newHashSetWithExpectedSize(rnm.revisionNotes.size()); + RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(rnm); + + for (PatchLineComment c : put.values()) { + if (!delete.contains(key(c))) { + cache.get(c.getRevId()).putComment(c); + } + } + for (Key k : delete) { + cache.get(k.revId()).deleteComment(k.key()); } - NoteMap noteMap = draftNotes.load().getNoteMap(); - if (noteMap == null) { - noteMap = NoteMap.newEmptyMap(); - } - - Map> allComments = new HashMap<>(); - + Map builders = cache.getBuilders(); boolean hasComments = false; - int n = deleteComments.size() + upsertComments.size(); - Set updatedRevs = Sets.newHashSetWithExpectedSize(n); - Set updatedKeys = Sets.newHashSetWithExpectedSize(n); - for (PatchLineComment c : deleteComments) { - allComments.put(c.getRevId(), new ArrayList()); - updatedRevs.add(c.getRevId()); - updatedKeys.add(c.getKey()); - } - - for (PatchLineComment c : upsertComments) { - hasComments = true; - addCommentToMap(allComments, c); - updatedRevs.add(c.getRevId()); - updatedKeys.add(c.getKey()); - } - - // Re-add old comments for updated revisions so the new note contents - // includes both old and new comments merged in the right order. - // - // writeCommentsToNoteMap doesn't touch notes for SHA-1s that are not - // mentioned in the input map, so by omitting comments for those revisions, - // we avoid the work of having to re-serialize identical comment data for - // those revisions. - ListMultimap existing = - draftNotes.getComments(); - for (Map.Entry e : existing.entries()) { - PatchLineComment c = e.getValue(); - if (updatedRevs.contains(c.getRevId()) - && !updatedKeys.contains(c.getKey())) { + for (Map.Entry e : builders.entrySet()) { + updatedRevs.add(e.getKey()); + ObjectId id = ObjectId.fromString(e.getKey().get()); + byte[] data = e.getValue().build(commentsUtil); + if (data.length == 0) { + rnm.noteMap.remove(id); + } else { hasComments = true; - addCommentToMap(allComments, e.getValue()); + ObjectId dataBlob = ins.insert(OBJ_BLOB, data); + rnm.noteMap.set(id, dataBlob); } } - // If we touched every revision and there are no comments left, set the flag - // for the caller to delete the entire ref. - boolean touchedAllRevs = updatedRevs.equals(existing.keySet()); + // If we touched every revision and there are no comments left, tell the + // caller to delete the entire ref. + boolean touchedAllRevs = updatedRevs.equals(rnm.revisionNotes.keySet()); if (touchedAllRevs && !hasComments) { - removedAllComments.set(touchedAllRevs && !hasComments); return null; } - commentsUtil.writeCommentsToNoteMap(noteMap, allComments, inserter); - return noteMap.writeTree(inserter); + return rnm.noteMap.writeTree(ins); } - public RevCommit commit() throws IOException { - BatchMetaDataUpdate batch = openUpdate(); - try { - writeCommit(batch); - return batch.commit(); - } catch (OrmException e) { - throw new IOException(e); - } finally { - batch.close(); + private RevisionNoteMap getRevisionNoteMap(RevWalk rw, ObjectId curr) + throws ConfigInvalidException, OrmException, IOException { + if (migration.readChanges()) { + // If reading from changes is enabled, then the old DraftCommentNotes + // already parsed the revision notes. We can reuse them as long as the ref + // hasn't advanced. + DraftCommentNotes draftNotes = + ctl.getNotes().load().getDraftCommentNotes(); + if (draftNotes != null) { + ObjectId idFromNotes = + firstNonNull(draftNotes.getRevision(), ObjectId.zeroId()); + if (idFromNotes.equals(curr)) { + return checkNotNull(ctl.getNotes().revisionNoteMap); + } + } } + NoteMap noteMap; + if (!curr.equals(ObjectId.zeroId())) { + noteMap = NoteMap.read(rw.getObjectReader(), rw.parseCommit(curr)); + } else { + noteMap = NoteMap.newEmptyMap(); + } + // Even though reading from changes might not be enabled, we need to + // parse any existing revision notes so we can merge them. + return RevisionNoteMap.parse( + ctl.getId(), rw.getObjectReader(), noteMap, true); } @Override - public void writeCommit(BatchMetaDataUpdate batch) - throws OrmException, IOException { - CommitBuilder builder = new CommitBuilder(); - if (migration.writeChanges()) { - AtomicBoolean removedAllComments = new AtomicBoolean(); - ObjectId treeId = storeCommentsInNotes(removedAllComments); - if (removedAllComments.get()) { - batch.removeRef(getRefName()); - } else if (treeId != null) { - builder.setTreeId(treeId); - batch.write(builder); + protected CommitBuilder applyImpl(RevWalk rw, ObjectInserter ins, + ObjectId curr) throws OrmException, IOException { + CommitBuilder cb = new CommitBuilder(); + cb.setMessage("Update draft comments"); + try { + ObjectId treeId = storeCommentsInNotes(rw, ins, curr); + if (treeId == null) { + return null; // Delete ref. } + cb.setTreeId(checkNotNull(treeId)); + } catch (ConfigInvalidException e) { + throw new OrmException(e); } + return cb; } @Override @@ -276,21 +225,9 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { return RefNames.refsDraftComments(accountId, ctl.getId()); } - @Override - protected boolean onSave(CommitBuilder commit) throws IOException, - ConfigInvalidException { - if (isEmpty()) { - return false; - } - commit.setAuthor(newIdent(getUser().getAccount(), when)); - commit.setCommitter(new PersonIdent(serverIdent, when)); - commit.setMessage("Update draft comments"); - return true; - } - @Override public boolean isEmpty() { - return deleteComments.isEmpty() - && upsertComments.isEmpty(); + return delete.isEmpty() + && put.isEmpty(); } } 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 149d3ff6ed..b2e765cf98 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 @@ -33,6 +33,8 @@ import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Multimap; +import com.google.common.collect.Multimaps; import com.google.common.collect.Ordering; import com.google.common.primitives.Ints; import com.google.common.util.concurrent.AsyncFunction; @@ -70,7 +72,6 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.RevWalk; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -398,10 +399,9 @@ public class ChangeNotes extends AbstractChangeNotes { private ImmutableListMultimap comments; private ImmutableSet hashtags; - // Mutable note map state, only used by ChangeUpdate to make in-place editing - // of notes easier. - NoteMap noteMap; - Map revisionNotes; + // Parsed note map state, used by ChangeUpdate to make in-place editing of + // notes easier. + RevisionNoteMap revisionNoteMap; private final AllUsersName allUsers; private DraftCommentNotes draftCommentNotes; @@ -477,7 +477,25 @@ public class ChangeNotes extends AbstractChangeNotes { public ImmutableListMultimap getDraftComments( Account.Id author) throws OrmException { loadDraftComments(author); - return draftCommentNotes.getComments(); + final Multimap published = comments; + // Filter out any draft comments that also exist in the published map, in + // case the update to All-Users to delete them during the publish operation + // failed. + Multimap filtered = Multimaps.filterEntries( + draftCommentNotes.getComments(), + new Predicate>() { + @Override + public boolean apply(Map.Entry in) { + for (PatchLineComment c : published.get(in.getKey())) { + if (c.getKey().equals(in.getValue().getKey())) { + return false; + } + } + return true; + } + }); + return ImmutableListMultimap.copyOf( + filtered); } /** @@ -518,15 +536,6 @@ public class ChangeNotes extends AbstractChangeNotes { return false; } - /** @return the NoteMap */ - NoteMap getNoteMap() { - return noteMap; - } - - Map getRevisionNotes() { - return revisionNotes; - } - @Override protected String getRefName() { return ChangeNoteUtil.changeRefName(getChangeId()); @@ -557,8 +566,7 @@ public class ChangeNotes extends AbstractChangeNotes { changeMessagesByPatchSet = parser.buildMessagesByPatchSet(); allChangeMessages = parser.buildAllMessages(); comments = ImmutableListMultimap.copyOf(parser.comments); - noteMap = parser.noteMap; - revisionNotes = parser.revisionNotes; + revisionNoteMap = parser.revisionNoteMap; change.setKey(new Change.Key(parser.changeId)); change.setDest(new Branch.NameKey(project, parser.branch)); change.setTopic(Strings.emptyToNull(parser.topic)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 2eb19686da..64b4b62587 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -69,7 +69,6 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.notes.Note; import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.RevCommit; @@ -101,7 +100,6 @@ class ChangeNotesParser implements AutoCloseable { final Multimap comments; final TreeMap patchSets; final Map patchSetStates; - final Map revisionNotes; String branch; Change.Status status; @@ -115,7 +113,7 @@ class ChangeNotesParser implements AutoCloseable { String originalSubject; String submissionId; PatchSet.Id currentPatchSetId; - NoteMap noteMap; + RevisionNoteMap revisionNoteMap; private final Change.Id id; private final ObjectId tip; @@ -142,7 +140,6 @@ class ChangeNotesParser implements AutoCloseable { comments = ArrayListMultimap.create(); patchSets = Maps.newTreeMap(ReviewDbUtil.intKeyOrdering()); patchSetStates = Maps.newHashMap(); - revisionNotes = Maps.newHashMap(); } @Override @@ -217,7 +214,9 @@ class ChangeNotesParser implements AutoCloseable { } Account.Id accountId = parseIdent(commit); - ownerId = accountId; + if (accountId != null) { + ownerId = accountId; + } if (changeId == null) { changeId = parseChangeId(commit); @@ -339,6 +338,10 @@ class ChangeNotesParser implements AutoCloseable { private void parsePatchSet(PatchSet.Id psId, ObjectId rev, Account.Id accountId, Timestamp ts) throws ConfigInvalidException { + if (accountId == null) { + throw parseException( + "patch set %s requires an identified user as uploader", psId.get()); + } PatchSet ps = patchSets.get(psId); if (ps == null) { ps = new PatchSet(psId); @@ -497,19 +500,18 @@ class ChangeNotesParser implements AutoCloseable { throws IOException, ConfigInvalidException { ObjectReader reader = walk.getObjectReader(); RevCommit tipCommit = walk.parseCommit(tip); - noteMap = NoteMap.read(reader, tipCommit); + revisionNoteMap = RevisionNoteMap.parse( + id, reader, NoteMap.read(reader, tipCommit), false); + Map rns = revisionNoteMap.revisionNotes; - for (Note note : noteMap) { - RevisionNote rn = new RevisionNote(id, reader, note.getData()); - RevId revId = new RevId(note.name()); - revisionNotes.put(revId, rn); - for (PatchLineComment plc : rn.comments) { - comments.put(revId, plc); + for (Map.Entry e : rns.entrySet()) { + for (PatchLineComment plc : e.getValue().comments) { + comments.put(e.getKey(), plc); } } for (PatchSet ps : patchSets.values()) { - RevisionNote rn = revisionNotes.get(ps.getRevision()); + RevisionNote rn = rns.get(ps.getRevision()); if (rn != null && rn.pushCert != null) { ps.setPushCertificate(rn.pushCert); } @@ -518,6 +520,10 @@ class ChangeNotesParser implements AutoCloseable { private void parseApproval(PatchSet.Id psId, Account.Id accountId, Timestamp ts, String line) throws ConfigInvalidException { + if (accountId == null) { + throw parseException( + "patch set %s requires an identified user as uploader", psId.get()); + } if (line.startsWith("-")) { parseRemoveApproval(psId, accountId, line); } else { @@ -665,6 +671,14 @@ class ChangeNotesParser implements AutoCloseable { private Account.Id parseIdent(RevCommit commit) throws ConfigInvalidException { + // Check if the author name/email is the same as the committer name/email, + // i.e. was the server ident at the time this commit was made. + PersonIdent a = commit.getAuthorIdent(); + PersonIdent c = commit.getCommitterIdent(); + if (a.getName().equals(c.getName()) + && a.getEmailAddress().equals(c.getEmailAddress())) { + return null; + } return parseIdent(commit.getAuthorIdent()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java index 74ef67f718..49eb185a5b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java @@ -40,22 +40,23 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.StarredChange; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.PatchLineCommentsUtil; -import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate; +import com.google.gerrit.server.git.ChainedReceiveCommands; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; -import com.google.inject.Provider; +import com.google.inject.util.Providers; -import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; -import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; @@ -78,61 +79,75 @@ public class ChangeRebuilder { private static final long TS_WINDOW_MS = TimeUnit.MILLISECONDS.convert(1, TimeUnit.SECONDS); - private final Provider dbProvider; + private final SchemaFactory schemaFactory; + private final GitRepositoryManager repoManager; private final ChangeControl.GenericFactory controlFactory; private final IdentifiedUser.GenericFactory userFactory; + private final InternalUser.Factory internalUserFactory; private final PatchListCache patchListCache; private final ChangeUpdate.Factory updateFactory; private final ChangeDraftUpdate.Factory draftUpdateFactory; + private final NoteDbUpdateManager.Factory updateManagerFactory; @Inject - ChangeRebuilder(Provider dbProvider, + ChangeRebuilder(SchemaFactory schemaFactory, + GitRepositoryManager repoManager, ChangeControl.GenericFactory controlFactory, IdentifiedUser.GenericFactory userFactory, + InternalUser.Factory internalUserFactory, PatchListCache patchListCache, ChangeUpdate.Factory updateFactory, - ChangeDraftUpdate.Factory draftUpdateFactory) { - this.dbProvider = dbProvider; + ChangeDraftUpdate.Factory draftUpdateFactory, + NoteDbUpdateManager.Factory updateManagerFactory) { + this.schemaFactory = schemaFactory; + this.repoManager = repoManager; this.controlFactory = controlFactory; this.userFactory = userFactory; + this.internalUserFactory = internalUserFactory; this.patchListCache = patchListCache; this.updateFactory = updateFactory; this.draftUpdateFactory = draftUpdateFactory; + this.updateManagerFactory = updateManagerFactory; } - public ListenableFuture rebuildAsync(final Change change, - ListeningExecutorService executor, final BatchRefUpdate bru, - final BatchRefUpdate bruForDrafts, final Repository changeRepo, - final Repository allUsersRepo) { + public ListenableFuture rebuildAsync(final Change.Id id, + ListeningExecutorService executor) { return executor.submit(new Callable() { @Override public Void call() throws Exception { - rebuild(change, bru, bruForDrafts, changeRepo, allUsersRepo); + try (ReviewDb db = schemaFactory.open()) { + rebuild(db, id); + } return null; } }); } - public void rebuild(Change change, BatchRefUpdate bru, - BatchRefUpdate bruAllUsers, Repository changeRepo, - Repository allUsersRepo) throws NoSuchChangeException, IOException, - OrmException { - ReviewDb db = dbProvider.get(); - Change.Id changeId = change.getId(); + public void rebuild(ReviewDb db, Change.Id changeId) + throws NoSuchChangeException, IOException, OrmException { + Change change = db.changes().get(changeId); + if (change == null) { + return; + } + NoteDbUpdateManager manager = + updateManagerFactory.create(change.getProject()); + // We will rebuild all events, except for draft comments, in buckets based - // on author and timestamp. However, all draft comments for a given change - // and author will be written as one commit in the notedb. + // on author and timestamp. List events = Lists.newArrayList(); Multimap draftCommentEvents = ArrayListMultimap.create(); - try (RevWalk rw = new RevWalk(changeRepo)) { - events.addAll(getHashtagsEvents(change, changeRepo, rw)); + Repository changeMetaRepo = manager.getChangeRepo(); + events.addAll(getHashtagsEvents(change, manager)); - deleteRef(change, changeRepo); + // Delete ref only after hashtags have been read + deleteRef(change, changeMetaRepo, manager.getChangeCommands()); + try (Repository codeRepo = repoManager.openRepository(change.getProject()); + RevWalk codeRw = new RevWalk(codeRepo)) { for (PatchSet ps : db.patchSets().byChange(changeId)) { - events.add(new PatchSetEvent(change, ps, rw)); + events.add(new PatchSetEvent(change, ps, codeRw)); for (PatchLineComment c : db.patchComments().byPatchSet(ps.getId())) { PatchLineCommentEvent e = new PatchLineCommentEvent(c, change, ps, patchListCache); @@ -156,63 +171,51 @@ public class ChangeRebuilder { Collections.sort(events); events.add(new FinalUpdatesEvent(change, notedbChange)); - BatchMetaDataUpdate batch = null; ChangeUpdate update = null; for (Event e : events) { if (!sameUpdate(e, update)) { - writeToBatch(batch, update, changeRepo); - IdentifiedUser user = userFactory.create(dbProvider, e.who); + if (update != null) { + manager.add(update); + } + CurrentUser user = e.who != null + ? userFactory.create(Providers.of(db), e.who) + : internalUserFactory.create(); update = updateFactory.create( controlFactory.controlFor(db, change, user), e.when); update.setPatchSetId(e.psId); - if (batch == null) { - batch = update.openUpdateInBatch(bru); - } } e.apply(update); } - if (batch != null) { - writeToBatch(batch, update, changeRepo); - - // Since the BatchMetaDataUpdates generated by all ChangeRebuilders on a - // given project are backed by the same BatchRefUpdate, we need to - // synchronize on the BatchRefUpdate. Therefore, since commit on a - // BatchMetaDataUpdate is the only method that modifies a BatchRefUpdate, - // we can just synchronize this call. - synchronized (bru) { - batch.commit(); - } - } + manager.add(update); for (Account.Id author : draftCommentEvents.keys()) { - IdentifiedUser user = userFactory.create(dbProvider, author); + IdentifiedUser user = userFactory.create(Providers.of(db), author); ChangeDraftUpdate draftUpdate = null; - BatchMetaDataUpdate batchForDrafts = null; for (PatchLineCommentEvent e : draftCommentEvents.get(author)) { - if (draftUpdate == null) { + if (!sameUpdate(e, draftUpdate)) { + if (draftUpdate != null) { + manager.add(draftUpdate); + } draftUpdate = draftUpdateFactory.create( controlFactory.controlFor(db, change, user), e.when); draftUpdate.setPatchSetId(e.psId); - batchForDrafts = draftUpdate.openUpdateInBatch(bruAllUsers); } e.applyDraft(draftUpdate); } - writeToBatch(batchForDrafts, draftUpdate, allUsersRepo); - synchronized(bruAllUsers) { - batchForDrafts.commit(); - } + manager.add(draftUpdate); } - createStarredChangesRefs(changeId, bruAllUsers, allUsersRepo); + createStarredChangesRefs(db, changeId, manager.getAllUsersCommands(), + manager.getAllUsersRepo()); + manager.execute(); } - private void createStarredChangesRefs(Change.Id changeId, - BatchRefUpdate bruAllUsers, Repository allUsersRepo) + private void createStarredChangesRefs(ReviewDb db, Change.Id changeId, + ChainedReceiveCommands allUsersCmds, Repository allUsersRepo) throws IOException, OrmException { ObjectId emptyTree = emptyTree(allUsersRepo); - for (StarredChange starred : dbProvider.get().starredChanges() - .byChange(changeId)) { - bruAllUsers.addCommand(new ReceiveCommand(ObjectId.zeroId(), emptyTree, + for (StarredChange starred : db.starredChanges().byChange(changeId)) { + allUsersCmds.add(new ReceiveCommand(ObjectId.zeroId(), emptyTree, RefNames.refsStarredChanges(starred.getAccountId(), changeId))); } } @@ -226,16 +229,18 @@ public class ChangeRebuilder { } private List getHashtagsEvents(Change change, - Repository changeRepo, RevWalk rw) throws IOException { + NoteDbUpdateManager manager) throws IOException { String refName = ChangeNoteUtil.changeRefName(change.getId()); - Ref ref = changeRepo.exactRef(refName); - if (ref == null) { + ObjectId old = manager.getChangeCommands() + .getObjectId(manager.getChangeRepo(), refName); + if (old == null) { return Collections.emptyList(); } + RevWalk rw = manager.getChangeRevWalk(); List events = new ArrayList<>(); rw.reset(); - rw.markStart(rw.parseCommit(ref.getObjectId())); + rw.markStart(rw.parseCommit(old)); for (RevCommit commit : rw) { Account.Id authorId = ChangeNoteUtil.parseIdent(commit.getAuthorIdent()); @@ -277,40 +282,12 @@ public class ChangeRebuilder { return new PatchSet.Id(change.getId(), psId); } - private void deleteRef(Change change, Repository changeRepo) - throws IOException { + private void deleteRef(Change change, Repository repo, + ChainedReceiveCommands cmds) throws IOException { String refName = ChangeNoteUtil.changeRefName(change.getId()); - RefUpdate ru = changeRepo.updateRef(refName, true); - ru.setForceUpdate(true); - RefUpdate.Result result = ru.delete(); - switch (result) { - case FORCED: - case NEW: - case NO_CHANGE: - break; - case FAST_FORWARD: - case IO_FAILURE: - case LOCK_FAILURE: - case NOT_ATTEMPTED: - case REJECTED: - case REJECTED_CURRENT_BRANCH: - case RENAMED: - default: - throw new IOException( - String.format("Failed to delete ref %s: %s", refName, result)); - } - } - - private void writeToBatch(BatchMetaDataUpdate batch, - AbstractChangeUpdate update, Repository repo) throws IOException, - OrmException { - if (update == null || update.isEmpty()) { - return; - } - - try (ObjectInserter inserter = repo.newObjectInserter()) { - update.setInserter(inserter); - update.writeCommit(batch); + ObjectId old = cmds.getObjectId(repo, refName); + if (old != null) { + cmds.add(new ReceiveCommand(old, ObjectId.zeroId(), refName)); } } @@ -318,7 +295,7 @@ public class ChangeRebuilder { return when.getTime() / TS_WINDOW_MS; } - private static boolean sameUpdate(Event event, ChangeUpdate update) { + private static boolean sameUpdate(Event event, AbstractChangeUpdate update) { return update != null && round(event.when) == round(update.getWhen()) && event.who.equals(update.getUser().getAccountId()) @@ -441,14 +418,14 @@ public class ChangeRebuilder { if (c.getRevId() == null) { setCommentRevId(c, cache, change, ps); } - update.insertComment(c); + update.putComment(c); } void applyDraft(ChangeDraftUpdate draftUpdate) throws OrmException { if (c.getRevId() == null) { setCommentRevId(c, cache, change, ps); } - draftUpdate.insertComment(c); + draftUpdate.putComment(c); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 219f8c88da..5b7ce685b5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.notedb; +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; @@ -46,14 +47,12 @@ import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchLineComment; -import com.google.gerrit.reviewdb.client.PatchLineComment.Status; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.LabelVote; @@ -61,8 +60,10 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.FooterKey; @@ -72,7 +73,7 @@ import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; import java.util.Comparator; import java.util.Date; -import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -99,27 +100,29 @@ public class ChangeUpdate extends AbstractChangeUpdate { } private final AccountCache accountCache; - private String commitSubject; - private String subject; + private final CommentsInNotesUtil commentsUtil; + private final ChangeDraftUpdate.Factory draftUpdateFactory; + private final NoteDbUpdateManager.Factory updateManagerFactory; + private final Table> approvals; private final Map reviewers; + + private String commitSubject; + private String subject; private String changeId; private String branch; private Change.Status status; private List submitRecords; private String submissionId; - private final CommentsInNotesUtil commentsUtil; private List comments; private String topic; private ObjectId commit; private Set hashtags; private String changeMessage; - private ChangeNotes notes; private PatchSetState psState; private Iterable groups; private String pushCert; - private final ChangeDraftUpdate.Factory draftUpdateFactory; private ChangeDraftUpdate draftUpdate; @AssistedInject @@ -129,13 +132,13 @@ public class ChangeUpdate extends AbstractChangeUpdate { GitRepositoryManager repoManager, NotesMigration migration, AccountCache accountCache, - MetaDataUpdate.User updateFactory, + NoteDbUpdateManager.Factory updateManagerFactory, ChangeDraftUpdate.Factory draftUpdateFactory, ProjectCache projectCache, @Assisted ChangeControl ctl, CommentsInNotesUtil commentsUtil) { this(serverIdent, anonymousCowardName, repoManager, migration, accountCache, - updateFactory, draftUpdateFactory, + updateManagerFactory, draftUpdateFactory, projectCache, ctl, serverIdent.getWhen(), commentsUtil); } @@ -146,14 +149,14 @@ public class ChangeUpdate extends AbstractChangeUpdate { GitRepositoryManager repoManager, NotesMigration migration, AccountCache accountCache, - MetaDataUpdate.User updateFactory, + NoteDbUpdateManager.Factory updateManagerFactory, ChangeDraftUpdate.Factory draftUpdateFactory, ProjectCache projectCache, @Assisted ChangeControl ctl, @Assisted Date when, CommentsInNotesUtil commentsUtil) { this(serverIdent, anonymousCowardName, repoManager, migration, accountCache, - updateFactory, draftUpdateFactory, ctl, + updateManagerFactory, draftUpdateFactory, ctl, when, projectCache.get(getProjectName(ctl)).getLabelTypes().nameComparator(), commentsUtil); @@ -170,17 +173,19 @@ public class ChangeUpdate extends AbstractChangeUpdate { GitRepositoryManager repoManager, NotesMigration migration, AccountCache accountCache, - MetaDataUpdate.User updateFactory, + NoteDbUpdateManager.Factory updateManagerFactory, ChangeDraftUpdate.Factory draftUpdateFactory, @Assisted ChangeControl ctl, @Assisted Date when, @Assisted Comparator labelNameComparator, CommentsInNotesUtil commentsUtil) { - super(migration, repoManager, updateFactory, ctl, serverIdent, + super(migration, repoManager, ctl, serverIdent, anonymousCowardName, when); - this.draftUpdateFactory = draftUpdateFactory; this.accountCache = accountCache; this.commentsUtil = commentsUtil; + this.draftUpdateFactory = draftUpdateFactory; + this.updateManagerFactory = updateManagerFactory; + this.approvals = TreeBasedTable.create( labelNameComparator, Ordering.natural().onResultOf(new Function() { @@ -193,13 +198,19 @@ public class ChangeUpdate extends AbstractChangeUpdate { this.comments = Lists.newArrayList(); } - public void setChangeId(String changeId) throws OrmException { - if (notes == null) { - notes = getChangeNotes().load(); - } - checkArgument(notes.getChange().getKey().get().equals(changeId), + public ObjectId commit() throws IOException, OrmException { + NoteDbUpdateManager updateManager = + updateManagerFactory.create(getProjectName()); + updateManager.add(this); + updateManager.execute(); + return getResult(); + } + + public void setChangeId(String changeId) { + String old = ctl.getChange().getKey().get(); + checkArgument(old.equals(changeId), "The Change-Id was already set to %s, so we cannot set this Change-Id: %s", - notes.getChange().getKey(), changeId); + old, changeId); this.changeId = changeId; } @@ -258,120 +269,41 @@ public class ChangeUpdate extends AbstractChangeUpdate { this.changeMessage = changeMessage; } - public void insertComment(PatchLineComment comment) throws OrmException { - if (comment.getStatus() == Status.DRAFT) { - insertDraftComment(comment); - } else { - insertPublishedComment(comment); - } - } - - public void upsertComment(PatchLineComment comment) throws OrmException { - if (comment.getStatus() == Status.DRAFT) { - upsertDraftComment(comment); - } else { - deleteDraftCommentIfPresent(comment); - upsertPublishedComment(comment); - } - } - - public void updateComment(PatchLineComment comment) throws OrmException { - if (comment.getStatus() == Status.DRAFT) { - updateDraftComment(comment); - } else { - deleteDraftCommentIfPresent(comment); - updatePublishedComment(comment); - } - } - - public void deleteComment(PatchLineComment comment) throws OrmException { - if (comment.getStatus() == Status.DRAFT) { - deleteDraftComment(comment); - } else { - throw new IllegalArgumentException("Cannot delete a published comment."); - } - } - - private void insertPublishedComment(PatchLineComment c) throws OrmException { + public void putComment(PatchLineComment c) { verifyComment(c); - if (notes == null) { - notes = getChangeNotes().load(); - } - if (migration.readChanges()) { - checkArgument(!notes.containsComment(c), - "A comment already exists with the same key as the following comment," - + " so we cannot insert this comment: %s", c); - } - comments.add(c); - } - - private void insertDraftComment(PatchLineComment c) throws OrmException { createDraftUpdateIfNull(); - draftUpdate.insertComment(c); + if (c.getStatus() == PatchLineComment.Status.DRAFT) { + draftUpdate.putComment(c); + } else { + comments.add(c); + // Always delete the corresponding comment from drafts. Published comments + // are immutable, meaning in normal operation we only hit this path when + // publishing a comment. It's exactly in that case that we have to delete + // the draft. + draftUpdate.deleteComment(c); + } } - private void upsertPublishedComment(PatchLineComment c) throws OrmException { + public void deleteComment(PatchLineComment c) { verifyComment(c); - if (notes == null) { - notes = getChangeNotes().load(); + if (c.getStatus() == PatchLineComment.Status.DRAFT) { + createDraftUpdateIfNull().deleteComment(c); + } else { + throw new IllegalArgumentException( + "Cannot delete published comment " + c); } - // This could allow callers to update a published comment if migration.write - // is on and migration.readComments is off because we will not be able to - // verify that the comment didn't already exist as a published comment - // since we don't have a ReviewDb. - if (migration.readChanges()) { - checkArgument(!notes.containsCommentPublished(c), - "Cannot update a comment that has already been published and saved"); - } - comments.add(c); } - private void upsertDraftComment(PatchLineComment c) { - createDraftUpdateIfNull(); - draftUpdate.upsertComment(c); - } - - private void updatePublishedComment(PatchLineComment c) throws OrmException { - verifyComment(c); - if (notes == null) { - notes = getChangeNotes().load(); - } - // See comment above in upsertPublishedComment() about potential risk with - // this check. - if (migration.readChanges()) { - checkArgument(!notes.containsCommentPublished(c), - "Cannot update a comment that has already been published and saved"); - } - comments.add(c); - } - - private void updateDraftComment(PatchLineComment c) throws OrmException { - createDraftUpdateIfNull(); - draftUpdate.updateComment(c); - } - - private void deleteDraftComment(PatchLineComment c) throws OrmException { - createDraftUpdateIfNull(); - draftUpdate.deleteComment(c); - } - - private void deleteDraftCommentIfPresent(PatchLineComment c) - throws OrmException { - createDraftUpdateIfNull(); - draftUpdate.deleteCommentIfPresent(c); - } - - private void createDraftUpdateIfNull() { + @VisibleForTesting + ChangeDraftUpdate createDraftUpdateIfNull() { if (draftUpdate == null) { draftUpdate = draftUpdateFactory.create(ctl, when); } + return draftUpdate; } private void verifyComment(PatchLineComment c) { checkArgument(c.getRevId() != null); - checkArgument(c.getStatus() == Status.PUBLISHED, - "Cannot add a draft comment to a ChangeUpdate. Use a ChangeDraftUpdate" - + " for draft comments"); checkArgument(c.getAuthor().equals(getUser().getAccountId()), "The author for the following comment does not match the author of" + " this ChangeDraftUpdate (%s): %s", getUser().getAccountId(), c); @@ -418,71 +350,92 @@ public class ChangeUpdate extends AbstractChangeUpdate { } /** @return the tree id for the updated tree */ - private ObjectId storeRevisionNotes() throws OrmException, IOException { - ChangeNotes notes = ctl.getNotes().load(); - NoteMap noteMap = notes.getNoteMap(); - if (noteMap == null) { - noteMap = NoteMap.newEmptyMap(); - } + private ObjectId storeRevisionNotes(RevWalk rw, ObjectInserter inserter, + ObjectId curr) throws ConfigInvalidException, OrmException, IOException { if (comments.isEmpty() && pushCert == null) { return null; } + RevisionNoteMap rnm = getRevisionNoteMap(rw, curr); - Map builders = new HashMap<>(); + RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(rnm); for (PatchLineComment c : comments) { - builder(builders, c.getRevId()).addComment(c); + cache.get(c.getRevId()).putComment(c); } if (pushCert != null) { checkState(commit != null); - builder(builders, new RevId(commit.name())).setPushCertificate(pushCert); + cache.get(new RevId(commit.name())).setPushCertificate(pushCert); } + Map builders = cache.getBuilders(); + checkComments(rnm.revisionNotes, builders); for (Map.Entry e : builders.entrySet()) { ObjectId data = inserter.insert( OBJ_BLOB, e.getValue().build(commentsUtil)); - noteMap.set(ObjectId.fromString(e.getKey().get()), data); + rnm.noteMap.set(ObjectId.fromString(e.getKey().get()), data); } - return noteMap.writeTree(inserter); + return rnm.noteMap.writeTree(inserter); } - private RevisionNoteBuilder builder(Map builders, - RevId revId) { - RevisionNoteBuilder b = builders.get(revId); - if (b == null) { - b = new RevisionNoteBuilder( - getChangeNotes().getRevisionNotes().get(revId)); - builders.put(revId, b); - } - return b; - } - - public RevCommit commit() throws IOException { - BatchMetaDataUpdate batch = openUpdate(); - try { - writeCommit(batch); - RevCommit c = batch.commit(); - return c; - } catch (OrmException e) { - throw new IOException(e); - } finally { - batch.close(); - } - } - - @Override - public void writeCommit(BatchMetaDataUpdate batch) throws OrmException, - IOException { - CommitBuilder builder = new CommitBuilder(); - if (migration.writeChanges()) { - ObjectId treeId = storeRevisionNotes(); - if (treeId != null) { - builder.setTreeId(treeId); + private RevisionNoteMap getRevisionNoteMap(RevWalk rw, ObjectId curr) + throws ConfigInvalidException, OrmException, IOException { + if (migration.readChanges()) { + // If reading from changes is enabled, then the old ChangeNotes already + // parsed the revision notes. We can reuse them as long as the ref hasn't + // advanced. + ObjectId idFromNotes = + firstNonNull(ctl.getNotes().load().getRevision(), ObjectId.zeroId()); + if (idFromNotes.equals(curr)) { + return checkNotNull(ctl.getNotes().revisionNoteMap); } } - batch.write(this, builder); - if (draftUpdate != null) { - draftUpdate.commit(); + NoteMap noteMap; + if (!curr.equals(ObjectId.zeroId())) { + noteMap = NoteMap.read(rw.getObjectReader(), rw.parseCommit(curr)); + } else { + noteMap = NoteMap.newEmptyMap(); + } + // Even though reading from changes might not be enabled, we need to + // parse any existing revision notes so we can merge them. + return RevisionNoteMap.parse( + ctl.getId(), rw.getObjectReader(), noteMap, false); + } + + private void checkComments(Map existingNotes, + Map toUpdate) throws OrmException { + // Prohibit various kinds of illegal operations on comments. + Set existing = new HashSet<>(); + for (RevisionNote rn : existingNotes.values()) { + for (PatchLineComment c : rn.comments) { + existing.add(c.getKey()); + if (draftUpdate != null) { + // Take advantage of an existing update on All-Users to prune any + // published comments from drafts. NoteDbUpdateManager takes care of + // ensuring that this update is applied before its dependent draft + // update. + // + // Deleting aggressively in this way, combined with filtering out + // duplicate published/draft comments in ChangeNotes#getDraftComments, + // makes up for the fact that updates between the change repo and + // All-Users are not atomic. + // + // TODO(dborowitz): We might want to distinguish between deleted + // drafts that we're fixing up after the fact by putting them in a + // separate commit. But note that we don't care much about the commit + // graph of the draft ref, particularly because the ref is completely + // deleted when all drafts are gone. + draftUpdate.deleteComment(c.getRevId(), c.getKey()); + } + } + } + + for (RevisionNoteBuilder b : toUpdate.values()) { + for (PatchLineComment c : b.put.values()) { + if (existing.contains(c.getKey())) { + throw new OrmException( + "Cannot update existing published comment: " + c); + } + } } } @@ -492,12 +445,9 @@ public class ChangeUpdate extends AbstractChangeUpdate { } @Override - protected boolean onSave(CommitBuilder cb) { - if (getRevision() != null && isEmpty()) { - return false; - } - cb.setAuthor(newIdent(getUser().getAccount(), when)); - cb.setCommitter(new PersonIdent(serverIdent, when)); + protected CommitBuilder applyImpl(RevWalk rw, ObjectInserter ins, + ObjectId curr) throws OrmException, IOException { + CommitBuilder cb = new CommitBuilder(); int ps = psId != null ? psId.get() : getChange().currentPatchSetId().get(); StringBuilder msg = new StringBuilder(); @@ -599,7 +549,15 @@ public class ChangeUpdate extends AbstractChangeUpdate { } cb.setMessage(msg.toString()); - return true; + try { + ObjectId treeId = storeRevisionNotes(rw, ins, curr); + if (treeId != null) { + cb.setTreeId(treeId); + } + } catch (ConfigInvalidException e) { + throw new OrmException(e); + } + return cb; } private void addPatchSetFooter(StringBuilder sb, int ps) { @@ -634,6 +592,10 @@ public class ChangeUpdate extends AbstractChangeUpdate { && groups == null; } + ChangeDraftUpdate getDraftUpdate() { + return draftUpdate; + } + private static StringBuilder addFooter(StringBuilder sb, FooterKey footer) { return sb.append(footer.getName()).append(": "); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java index b59386bbef..e660147085 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java @@ -15,16 +15,12 @@ package com.google.gerrit.server.notedb; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER; import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST; import static com.google.gerrit.server.notedb.ChangeNotes.parseException; -import static com.google.gerrit.server.notedb.RevisionNote.MAX_NOTE_SZ; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; -import com.google.common.collect.Multimap; import com.google.common.primitives.Ints; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; @@ -42,16 +38,7 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.ObjectInserter; -import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.PersonIdent; -import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.notes.Note; -import org.eclipse.jgit.notes.NoteMap; -import org.eclipse.jgit.revwalk.RevCommit; -import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.util.GitDateFormatter; import org.eclipse.jgit.util.GitDateFormatter.Format; import org.eclipse.jgit.util.GitDateParser; @@ -60,18 +47,14 @@ import org.eclipse.jgit.util.QuotedString; import org.eclipse.jgit.util.RawParseUtils; import java.io.ByteArrayOutputStream; -import java.io.IOException; import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.sql.Timestamp; import java.text.ParseException; -import java.util.ArrayList; -import java.util.Collections; import java.util.Date; import java.util.List; import java.util.Locale; -import java.util.Map; /** * Utility functions to parse PatchLineComments out of a note byte array and @@ -89,33 +72,6 @@ public class CommentsInNotesUtil { private static final String REVISION = "Revision"; private static final String UUID = "UUID"; - public static NoteMap parseCommentsFromNotes(Repository repo, String refName, - RevWalk walk, Change.Id changeId, - Multimap comments, - Status status) - throws IOException, ConfigInvalidException { - Ref ref = repo.getRefDatabase().exactRef(refName); - if (ref == null) { - return null; - } - - ObjectReader reader = walk.getObjectReader(); - RevCommit commit = walk.parseCommit(ref.getObjectId()); - NoteMap noteMap = NoteMap.read(reader, commit); - - for (Note note : noteMap) { - byte[] bytes = - reader.open(note.getData(), OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ); - List result = - parseNote(bytes, new MutableInteger(), changeId, status); - if (result == null || result.isEmpty()) { - continue; - } - comments.putAll(new RevId(note.name()), result); - } - return noteMap; - } - public static List parseNote(byte[] note, MutableInteger p, Change.Id changeId, Status status) throws ConfigInvalidException { if (p.value >= note.length) { @@ -432,7 +388,7 @@ public class CommentsInNotesUtil { * same side and must share the same patch set ID. * @param out output stream to write to. */ - public void buildNote(List comments, OutputStream out) { + void buildNote(List comments, OutputStream out) { if (comments.isEmpty()) { return; } @@ -514,51 +470,4 @@ public class CommentsInNotesUtil { } } } - - /** - * Write comments for multiple revisions to a note map. - *

- * Mutates the map in-place. only notes for SHA-1s found as keys in the map - * are modified; all other notes are left untouched. - * - * @param noteMap note map to modify. - * @param allComments map of revision to all comments for that revision; - * callers are responsible for reading the original comments and applying - * any changes. Differs from a multimap in that present-but-empty values - * are significant, and indicate the note for that SHA-1 should be - * deleted. - * @param inserter object inserter for writing notes. - * @throws IOException if an error occurred. - */ - public void writeCommentsToNoteMap(NoteMap noteMap, - Map> allComments, ObjectInserter inserter) - throws IOException { - for (Map.Entry> e : allComments.entrySet()) { - List comments = e.getValue(); - ObjectId commit = ObjectId.fromString(e.getKey().get()); - if (comments.isEmpty()) { - noteMap.remove(commit); - continue; - } - Collections.sort(comments, PLC_ORDER); - // We allow comments for multiple commits to be written in the same - // update, even though the rest of the metadata update is associated with - // a single patch set. - byte[] note = buildNote(comments); - if (note != null && note.length > 0) { - noteMap.set(commit, inserter.insert(OBJ_BLOB, note)); - } - } - } - - static void addCommentToMap(Map> map, - PatchLineComment c) { - List list = map.get(c.getRevId()); - if (list == null) { - list = new ArrayList<>(); - map.put(c.getRevId(), list); - } - list.add(c); - } - } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java index c349321f6a..13312dc10e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java @@ -15,7 +15,9 @@ package com.google.gerrit.server.notedb; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.Multimap; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchLineComment; @@ -31,6 +33,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.notes.NoteMap; +import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; @@ -66,7 +69,7 @@ public class DraftCommentNotes extends AbstractChangeNotes { private final Account.Id author; private ImmutableListMultimap comments; - private NoteMap noteMap; + private RevisionNoteMap revisionNoteMap; DraftCommentNotes(GitRepositoryManager repoManager, NotesMigration migration, AllUsersName draftsProject, Change.Id changeId, Account.Id author) { @@ -75,8 +78,8 @@ public class DraftCommentNotes extends AbstractChangeNotes { this.author = author; } - public NoteMap getNoteMap() { - return noteMap; + RevisionNoteMap getRevisionNoteMap() { + return revisionNoteMap; } public Account.Id getAuthor() { @@ -110,13 +113,17 @@ public class DraftCommentNotes extends AbstractChangeNotes { return; } - try (RevWalk walk = new RevWalk(reader); - DraftCommentNotesParser parser = new DraftCommentNotesParser( - getChangeId(), walk, rev, repoManager, draftsProject, author)) { - parser.parseDraftComments(); - - comments = ImmutableListMultimap.copyOf(parser.comments); - noteMap = parser.noteMap; + try (RevWalk walk = new RevWalk(reader)) { + RevCommit tipCommit = walk.parseCommit(rev); + revisionNoteMap = RevisionNoteMap.parse( + getChangeId(), reader, NoteMap.read(reader, tipCommit), true); + Multimap cs = ArrayListMultimap.create(); + for (RevisionNote rn : revisionNoteMap.revisionNotes.values()) { + for (PatchLineComment c : rn.comments) { + cs.put(c.getRevId(), c); + } + } + comments = ImmutableListMultimap.copyOf(cs); } } @@ -136,4 +143,9 @@ public class DraftCommentNotes extends AbstractChangeNotes { public Project.NameKey getProjectName() { return draftsProject; } + + @VisibleForTesting + NoteMap getNoteMap() { + return revisionNoteMap != null ? revisionNoteMap.noteMap : null; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotesParser.java deleted file mode 100644 index ef8683fa83..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotesParser.java +++ /dev/null @@ -1,69 +0,0 @@ -// 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.notedb; - -import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.Multimap; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.PatchLineComment; -import com.google.gerrit.reviewdb.client.RefNames; -import com.google.gerrit.reviewdb.client.RevId; -import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.git.GitRepositoryManager; - -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.errors.RepositoryNotFoundException; -import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.notes.NoteMap; -import org.eclipse.jgit.revwalk.RevWalk; - -import java.io.IOException; - -class DraftCommentNotesParser implements AutoCloseable { - final Multimap comments; - NoteMap noteMap; - - private final Change.Id changeId; - private final ObjectId tip; - private final RevWalk walk; - private final Repository repo; - private final Account.Id author; - - DraftCommentNotesParser(Change.Id changeId, RevWalk walk, ObjectId tip, - GitRepositoryManager repoManager, AllUsersName draftsProject, - Account.Id author) throws RepositoryNotFoundException, IOException { - this.changeId = changeId; - this.walk = walk; - this.tip = tip; - this.repo = repoManager.openMetadataRepository(draftsProject); - this.author = author; - - comments = ArrayListMultimap.create(); - } - - @Override - public void close() { - repo.close(); - } - - void parseDraftComments() throws IOException, ConfigInvalidException { - walk.markStart(walk.parseCommit(tip)); - noteMap = CommentsInNotesUtil.parseCommentsFromNotes(repo, - RefNames.refsDraftComments(author, changeId), - walk, changeId, comments, PatchLineComment.Status.DRAFT); - } -} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java index 75d926b987..6d84b41cfc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java @@ -21,5 +21,6 @@ public class NoteDbModule extends FactoryModule { public void configure() { factory(ChangeUpdate.Factory.class); factory(ChangeDraftUpdate.Factory.class); + factory(NoteDbUpdateManager.Factory.class); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java new file mode 100644 index 0000000000..173000d2d1 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java @@ -0,0 +1,265 @@ +// Copyright (C) 2016 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.notedb; + +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.ArrayListMultimap; +import com.google.common.collect.ListMultimap; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.git.ChainedReceiveCommands; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gwtorm.server.OrmException; +import com.google.inject.assistedinject.Assisted; +import com.google.inject.assistedinject.AssistedInject; + +import org.eclipse.jgit.lib.BatchRefUpdate; +import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectInserter; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.ReceiveCommand; + +import java.io.IOException; + +public class NoteDbUpdateManager { + public interface Factory { + NoteDbUpdateManager create(Project.NameKey projectName); + } + + private static class OpenRepo implements AutoCloseable { + final Repository repo; + final RevWalk rw; + final ObjectInserter ins; + final ChainedReceiveCommands cmds; + final boolean close; + + OpenRepo(Repository repo, RevWalk rw, ObjectInserter ins, + ChainedReceiveCommands cmds, boolean close) { + this.repo = checkNotNull(repo); + this.rw = checkNotNull(rw); + this.ins = checkNotNull(ins); + this.cmds = checkNotNull(cmds); + this.close = close; + } + + @Override + public void close() { + if (close) { + ins.close(); + rw.close(); + repo.close(); + } + } + } + + private final GitRepositoryManager repoManager; + private final NotesMigration migration; + private final AllUsersName allUsersName; + private final Project.NameKey projectName; + private final ListMultimap changeUpdates; + private final ListMultimap draftUpdates; + + private OpenRepo changeRepo; + private OpenRepo allUsersRepo; + + @AssistedInject + NoteDbUpdateManager(GitRepositoryManager repoManager, + NotesMigration migration, + AllUsersName allUsersName, + @Assisted Project.NameKey projectName) { + this.repoManager = repoManager; + this.migration = migration; + this.allUsersName = allUsersName; + this.projectName = projectName; + changeUpdates = ArrayListMultimap.create(); + draftUpdates = ArrayListMultimap.create(); + } + + public NoteDbUpdateManager setChangeRepo(Repository repo, RevWalk rw, + ObjectInserter ins, ChainedReceiveCommands cmds) { + checkState(changeRepo == null, "change repo already initialized"); + changeRepo = new OpenRepo(repo, rw, ins, cmds, false); + return this; + } + + Repository getChangeRepo() throws IOException { + initChangeRepo(); + return changeRepo.repo; + } + + RevWalk getChangeRevWalk() throws IOException { + initChangeRepo(); + return changeRepo.rw; + } + + ChainedReceiveCommands getChangeCommands() throws IOException { + initChangeRepo(); + return changeRepo.cmds; + } + + public NoteDbUpdateManager setAllUsersRepo(Repository repo, RevWalk rw, + ObjectInserter ins, ChainedReceiveCommands cmds) { + checkState(allUsersRepo == null, "allUsers repo already initialized"); + allUsersRepo = new OpenRepo(repo, rw, ins, cmds, false); + return this; + } + + Repository getAllUsersRepo() throws IOException { + initAllUsersRepo(); + return allUsersRepo.repo; + } + + ChainedReceiveCommands getAllUsersCommands() throws IOException { + initAllUsersRepo(); + return allUsersRepo.cmds; + } + + private void initChangeRepo() throws IOException { + if (changeRepo == null) { + changeRepo = openRepo(projectName); + } + } + + private void initAllUsersRepo() throws IOException { + if (allUsersRepo == null) { + allUsersRepo = openRepo(allUsersName); + } + } + + private OpenRepo openRepo(Project.NameKey p) throws IOException { + Repository repo = repoManager.openMetadataRepository(p); + ObjectInserter ins = repo.newObjectInserter(); + return new OpenRepo(repo, new RevWalk(ins.newReader()), ins, + new ChainedReceiveCommands(), true); + } + + private boolean isEmpty() { + if (!migration.writeChanges()) { + return true; + } + return changeUpdates.isEmpty() + && draftUpdates.isEmpty(); + } + + /** + * Add an update to the list of updates to execute. + *

+ * Updates should only be added to the manager after all mutations have been + * made, as this method may eagerly access the update. + * + * @param update the update to add. + */ + public void add(ChangeUpdate update) { + checkArgument(update.getProjectName().equals(projectName), + "update for project %s cannot be added to manager for project %s", + update.getProjectName(), projectName); + changeUpdates.put(update.getRefName(), update); + ChangeDraftUpdate du = update.getDraftUpdate(); + if (du != null) { + draftUpdates.put(du.getRefName(), du); + } + } + + public void add(ChangeDraftUpdate draftUpdate) { + draftUpdates.put(draftUpdate.getRefName(), draftUpdate); + } + + public void execute() throws OrmException, IOException { + if (isEmpty()) { + return; + } + try { + initChangeRepo(); + if (!draftUpdates.isEmpty()) { + initAllUsersRepo(); + } + addCommands(); + + // ChangeUpdates must execute before ChangeDraftUpdates. + // + // ChangeUpdate will automatically delete draft comments for any published + // comments, but the updates to the two repos don't happen atomically. + // Thus if the change meta update succeeds and the All-Users update fails, + // we may have stale draft comments. Doing it in this order allows stale + // comments to be filtered out by ChangeNotes, reflecting the fact that + // comments can only go from DRAFT to PUBLISHED, not vice versa. + execute(changeRepo); + execute(allUsersRepo); + } finally { + if (allUsersRepo != null) { + allUsersRepo.close(); + } + if (changeRepo != null) { + changeRepo.close(); + } + } + } + + private static void execute(OpenRepo or) throws IOException { + if (or == null || or.cmds.isEmpty()) { + return; + } + or.ins.flush(); + BatchRefUpdate bru = or.repo.getRefDatabase().newBatchUpdate(); + or.cmds.addTo(bru); + bru.execute(or.rw, NullProgressMonitor.INSTANCE); + for (ReceiveCommand cmd : bru.getCommands()) { + if (cmd.getResult() != ReceiveCommand.Result.OK) { + throw new IOException("Update failed: " + bru); + } + } + } + + private void addCommands() throws OrmException, IOException { + if (isEmpty()) { + return; + } + checkState(changeRepo != null, "must set change repo"); + if (!draftUpdates.isEmpty()) { + checkState(allUsersRepo != null, "must set all users repo"); + } + addUpdates(changeUpdates, changeRepo); + if (!draftUpdates.isEmpty()) { + addUpdates(draftUpdates, allUsersRepo); + } + } + + private static void addUpdates( + ListMultimap updates, OpenRepo or) + throws OrmException, IOException { + for (String refName : updates.keySet()) { + ObjectId old = firstNonNull( + or.cmds.getObjectId(or.repo, refName), ObjectId.zeroId()); + ObjectId curr = old; + + for (AbstractChangeUpdate u : updates.get(refName)) { + ObjectId next = u.apply(or.rw, or.ins, curr); + if (next == null) { + continue; + } + curr = next; + } + if (!old.equals(curr)) { + or.cmds.add(new ReceiveCommand(old, curr, refName)); + } + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java index dc43a5303a..b14b5f0a60 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java @@ -63,14 +63,21 @@ class RevisionNote { final ImmutableList comments; final String pushCert; - RevisionNote(Change.Id changeId, ObjectReader reader, ObjectId noteId) - throws ConfigInvalidException, IOException { + RevisionNote(Change.Id changeId, ObjectReader reader, ObjectId noteId, + boolean draftsOnly) throws ConfigInvalidException, IOException { byte[] bytes = reader.open(noteId, OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ); MutableInteger p = new MutableInteger(); trimLeadingEmptyLines(bytes, p); - pushCert = parsePushCert(changeId, bytes, p); - trimLeadingEmptyLines(bytes, p); - comments = ImmutableList.copyOf(CommentsInNotesUtil.parseNote( - bytes, p, changeId, PatchLineComment.Status.PUBLISHED)); + if (!draftsOnly) { + pushCert = parsePushCert(changeId, bytes, p); + trimLeadingEmptyLines(bytes, p); + } else { + pushCert = null; + } + PatchLineComment.Status status = draftsOnly + ? PatchLineComment.Status.DRAFT + : PatchLineComment.Status.PUBLISHED; + comments = ImmutableList.copyOf( + CommentsInNotesUtil.parseNote(bytes, p, changeId, status)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java index 031da1be22..3d79795dae 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java @@ -14,35 +14,77 @@ package com.google.gerrit.server.notedb; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.gerrit.reviewdb.client.PatchLineComment; +import com.google.gerrit.reviewdb.client.RevId; import java.io.ByteArrayOutputStream; +import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; +import java.util.List; import java.util.Map; +import java.util.Set; class RevisionNoteBuilder { - private final Map comments; + static class Cache { + private final RevisionNoteMap revisionNoteMap; + private final Map builders; + + Cache(RevisionNoteMap revisionNoteMap) { + this.revisionNoteMap = revisionNoteMap; + this.builders = new HashMap<>(); + } + + RevisionNoteBuilder get(RevId revId) { + RevisionNoteBuilder b = builders.get(revId); + if (b == null) { + b = new RevisionNoteBuilder( + revisionNoteMap.revisionNotes.get(revId)); + builders.put(revId, b); + } + return b; + } + + Map getBuilders() { + return Collections.unmodifiableMap(builders); + } + } + + final List baseComments; + final Map put; + final Set delete; + private String pushCert; RevisionNoteBuilder(RevisionNote base) { if (base != null) { - comments = Maps.newHashMapWithExpectedSize(base.comments.size()); - for (PatchLineComment c : base.comments) { - addComment(c); - } + baseComments = base.comments; + put = Maps.newHashMapWithExpectedSize(base.comments.size()); pushCert = base.pushCert; } else { - comments = new HashMap<>(); + baseComments = Collections.emptyList(); + put = new HashMap<>(); pushCert = null; } + delete = new HashSet<>(); } - void addComment(PatchLineComment comment) { - comments.put(comment.getKey(), comment); + void putComment(PatchLineComment comment) { + checkArgument(!delete.contains(comment.getKey()), + "cannot both delete and put %s", comment.getKey()); + put.put(comment.getKey(), comment); + } + + void deleteComment(PatchLineComment.Key key) { + checkArgument(!put.containsKey(key), "cannot both delete and put %s", key); + delete.add(key); } void setPushCertificate(String pushCert) { @@ -56,7 +98,16 @@ class RevisionNoteBuilder { out.write(certBytes, 0, trimTrailingNewlines(certBytes)); out.write('\n'); } - commentsUtil.buildNote(PLC_ORDER.sortedCopy(comments.values()), out); + + List all = + new ArrayList<>(baseComments.size() + put.size()); + for (PatchLineComment c : Iterables.concat(baseComments, put.values())) { + if (!delete.contains(c.getKey())) { + all.add(c); + } + } + Collections.sort(all, PLC_ORDER); + commentsUtil.buildNote(all, out); return out.toByteArray(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteMap.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteMap.java new file mode 100644 index 0000000000..17f33b0d9a --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteMap.java @@ -0,0 +1,51 @@ +// Copyright (C) 2016 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.notedb; + +import com.google.common.collect.ImmutableMap; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.RevId; + +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.notes.Note; +import org.eclipse.jgit.notes.NoteMap; + +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +class RevisionNoteMap { + final NoteMap noteMap; + final ImmutableMap revisionNotes; + + static RevisionNoteMap parse(Change.Id changeId, ObjectReader reader, + NoteMap noteMap, boolean draftsOnly) + throws ConfigInvalidException, IOException { + Map result = new HashMap<>(); + for (Note note : noteMap) { + RevisionNote rn = + new RevisionNote(changeId, reader, note.getData(), draftsOnly); + result.put(new RevId(note.name()), rn); + } + return new RevisionNoteMap(noteMap, ImmutableMap.copyOf(result)); + } + + private RevisionNoteMap(NoteMap noteMap, + ImmutableMap revisionNotes) { + this.noteMap = noteMap; + this.revisionNotes = revisionNotes; + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java index 30f747aa24..4d4c698eef 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java @@ -29,8 +29,10 @@ import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.CapabilityControl; @@ -67,7 +69,6 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.PersonIdent; -import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevWalk; import org.junit.After; import org.junit.Before; @@ -91,17 +92,23 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests { protected InMemoryRepository repo; protected InMemoryRepositoryManager repoManager; protected PersonIdent serverIdent; + protected InternalUser internalUser; protected Project.NameKey project; protected RevWalk rw; protected TestRepository tr; - @Inject protected IdentifiedUser.GenericFactory userFactory; + @Inject + protected IdentifiedUser.GenericFactory userFactory; + + @Inject + protected NoteDbUpdateManager.Factory updateManagerFactory; + + @Inject + protected AllUsersName allUsers; private Injector injector; private String systemTimeZone; - @Inject private AllUsersName allUsers; - @Before public void setUp() throws Exception { setTimeForTesting(); @@ -128,6 +135,7 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests { @Override public void configure() { install(new GitModule()); + factory(NoteDbUpdateManager.Factory.class); bind(AllUsersName.class).toProvider(AllUsersNameProvider.class); bind(NotesMigration.class).toInstance(MIGRATION); bind(GitRepositoryManager.class).toInstance(repoManager); @@ -159,6 +167,7 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests { changeOwner = userFactory.create(co.getId()); otherUser = userFactory.create(ou.getId()); otherUserId = otherUser.getAccountId(); + internalUser = new InternalUser(null); } private void setTimeForTesting() { @@ -181,13 +190,10 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests { return c; } - protected ChangeUpdate newUpdate(Change c, IdentifiedUser user) + protected ChangeUpdate newUpdate(Change c, CurrentUser user) throws Exception { ChangeUpdate update = TestChanges.newUpdate( injector, repoManager, MIGRATION, c, allUsers, user); - try (Repository repo = repoManager.openMetadataRepository(c.getProject())) { - update.load(repo); - } return update; } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java index ff8f0ddfeb..acabfc7c1e 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java @@ -381,6 +381,39 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "Groups: d,e,f\n"); } + @Test + public void parseServerIdent() throws Exception { + String msg = "Update change\n" + + "\n" + + "Patch-set: 1\n" + + "Branch: refs/heads/master\n" + + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n" + + "Subject: Change subject\n"; + assertParseSucceeds(msg); + assertParseSucceeds(writeCommit(msg, serverIdent)); + + msg = "Update change\n" + + "\n" + + "With a message." + + "\n" + + "Patch-set: 1\n" + + "Branch: refs/heads/master\n" + + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n" + + "Subject: Change subject\n"; + assertParseSucceeds(msg); + assertParseSucceeds(writeCommit(msg, serverIdent)); + + msg = "Update change\n" + + "\n" + + "Patch-set: 1\n" + + "Branch: refs/heads/master\n" + + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n" + + "Subject: Change subject\n" + + "Label: Label1=+1\n"; + assertParseSucceeds(msg); + assertParseFails(writeCommit(msg, serverIdent)); + } + private RevCommit writeCommit(String body) throws Exception { return writeCommit(body, ChangeNoteUtil.newIdent( changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent, @@ -404,7 +437,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { } private void assertParseSucceeds(String body) throws Exception { - try (ChangeNotesParser parser = newParser(writeCommit(body))) { + assertParseSucceeds(writeCommit(body)); + } + + private void assertParseSucceeds(RevCommit commit) throws Exception { + try (ChangeNotesParser parser = newParser(commit)) { parser.parseAll(); } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index f13af17122..32a9c9f191 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -43,19 +43,19 @@ import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchLineComment.Status; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RevId; -import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate; import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; -import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.Constants; -import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.notes.Note; import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; -import org.eclipse.jgit.transport.ReceiveCommand; import org.junit.Test; import java.sql.Timestamp; @@ -65,6 +65,9 @@ import java.util.List; import java.util.Map; public class ChangeNotesTest extends AbstractChangeNotesTest { + @Inject + private DraftCommentNotes.Factory draftNotesFactory; + @Test public void approvalsOnePatchSet() throws Exception { Change c = newChange(); @@ -410,17 +413,16 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { @Test public void emptyChangeUpdate() throws Exception { Change c = newChange(); + Ref initial = repo.exactRef(ChangeNoteUtil.changeRefName(c.getId())); + assertThat(initial).isNotNull(); - // The initial empty update creates a commit which is needed to track the - // creation time of the change. + // Empty update doesn't create a new commit. ChangeUpdate update = newUpdate(c, changeOwner); - ObjectId revision = update.getRevision(); - assertThat(revision).isNotNull(); - - // Any further empty update doesn't create a new commit. - update = newUpdate(c, changeOwner); update.commit(); - assertThat(update.getRevision()).isEqualTo(revision); + assertThat(update.getResult()).isNull(); + + Ref updated = repo.exactRef(ChangeNoteUtil.changeRefName(c.getId())); + assertThat(updated.getObjectId()).isEqualTo(initial.getObjectId()); } @Test @@ -433,7 +435,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { update.setHashtags(hashtags); update.commit(); try (RevWalk walk = new RevWalk(repo)) { - RevCommit commit = walk.parseCommit(update.getRevision()); + RevCommit commit = walk.parseCommit(update.getResult()); walk.parseBody(commit); assertThat(commit.getFullMessage()).endsWith("Hashtags: tag1,tag2\n"); } @@ -712,7 +714,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { update.setPatchSetState(PatchSetState.DRAFT); update.putApproval("Code-Review", (short) 1); update.setChangeMessage("This is a message"); - update.insertComment(newPublishedComment(c.currentPatchSetId(), "a.txt", + update.putComment(newPublishedComment(c.currentPatchSetId(), "a.txt", "uuid1", new CommentRange(1, 2, 3, 4), 1, changeOwner, null, TimeUtil.nowTs(), "Comment", (short) 1, commit.name())); update.commit(); @@ -810,7 +812,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { update = newUpdate(c, changeOwner); update.setPatchSetId(psId2); Timestamp ts = TimeUtil.nowTs(); - update.insertComment(newPublishedComment(psId2, "a.txt", + update.putComment(newPublishedComment(psId2, "a.txt", "uuid1", new CommentRange(1, 2, 3, 4), 1, changeOwner, null, ts, "Comment", (short) 1, commit.name())); update.commit(); @@ -840,12 +842,11 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { public void emptyExceptSubject() throws Exception { ChangeUpdate update = newUpdate(newChange(), changeOwner); update.setSubjectForCommit("Create change"); - update.commit(); - assertThat(update.getRevision()).isNotNull(); + assertThat(update.commit()).isNotNull(); } @Test - public void multipleUpdatesInBatch() throws Exception { + public void multipleUpdatesInManager() throws Exception { Change c = newChange(); ChangeUpdate update1 = newUpdate(c, changeOwner); update1.putApproval("Verified", (short) 1); @@ -853,14 +854,10 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { ChangeUpdate update2 = newUpdate(c, otherUser); update2.putApproval("Code-Review", (short) 2); - BatchMetaDataUpdate batch = update1.openUpdate(); - try { - update1.writeCommit(batch); - update2.writeCommit(batch); - batch.commit(); - } finally { - batch.close(); - } + NoteDbUpdateManager updateManager = updateManagerFactory.create(project); + updateManager.add(update1); + updateManager.add(update2); + updateManager.execute(); ChangeNotes notes = newNotes(c); List psas = @@ -887,29 +884,24 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { CommentRange range1 = new CommentRange(1, 1, 2, 1); Timestamp time1 = TimeUtil.nowTs(); PatchSet.Id psId = c.currentPatchSetId(); - BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate(); - BatchMetaDataUpdate batch = update1.openUpdateInBatch(bru); + NoteDbUpdateManager updateManager = updateManagerFactory.create(project); PatchLineComment comment1 = newPublishedComment(psId, "file1", uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1, (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update1.setPatchSetId(psId); - update1.upsertComment(comment1); - update1.writeCommit(batch); + update1.putComment(comment1); + updateManager.add(update1); + ChangeUpdate update2 = newUpdate(c, otherUser); update2.putApproval("Code-Review", (short) 2); - update2.writeCommit(batch); + updateManager.add(update2); RevCommit tipCommit; - try (RevWalk rw = new RevWalk(repo)) { - batch.commit(); - bru.execute(rw, NullProgressMonitor.INSTANCE); + updateManager.execute(); - ChangeNotes notes = newNotes(c); - ObjectId tip = notes.getRevision(); - tipCommit = rw.parseCommit(tip); - } finally { - batch.close(); - } + ChangeNotes notes = newNotes(c); + ObjectId tip = notes.getRevision(); + tipCommit = rw.parseCommit(tip); RevCommit commitWithApprovals = tipCommit; assertThat(commitWithApprovals).isNotNull(); @@ -949,43 +941,30 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { ChangeUpdate update2 = newUpdate(c2, otherUser); update2.putApproval("Code-Review", (short) 2); - BatchMetaDataUpdate batch1 = null; - BatchMetaDataUpdate batch2 = null; + Ref initial1 = repo.exactRef(update1.getRefName()); + assertThat(initial1).isNotNull(); + Ref initial2 = repo.exactRef(update2.getRefName()); + assertThat(initial2).isNotNull(); - BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate(); - try { - batch1 = update1.openUpdateInBatch(bru); - update1.writeCommit(batch1); - batch1.commit(); - assertThat(repo.exactRef(update1.getRefName())).isNotNull(); + NoteDbUpdateManager updateManager = updateManagerFactory.create(project); + updateManager.add(update1); + updateManager.add(update2); + updateManager.execute(); - batch2 = update2.openUpdateInBatch(bru); - update2.writeCommit(batch2); - batch2.commit(); - assertThat(repo.exactRef(update2.getRefName())).isNotNull(); - } finally { - if (batch1 != null) { - batch1.close(); - } - if (batch2 != null) { - batch2.close(); - } - } + Ref ref1 = repo.exactRef(update1.getRefName()); + assertThat(ref1.getObjectId()).isEqualTo(update1.getResult()); + assertThat(ref1.getObjectId()).isNotEqualTo(initial1.getObjectId()); + Ref ref2 = repo.exactRef(update2.getRefName()); + assertThat(ref2.getObjectId()).isEqualTo(update2.getResult()); + assertThat(ref2.getObjectId()).isNotEqualTo(initial2.getObjectId()); - List cmds = bru.getCommands(); - assertThat(cmds).hasSize(2); - assertThat(cmds.get(0).getRefName()).isEqualTo(update1.getRefName()); - assertThat(cmds.get(1).getRefName()).isEqualTo(update2.getRefName()); + PatchSetApproval approval1 = newNotes(c1).getApprovals() + .get(c1.currentPatchSetId()).iterator().next(); + assertThat(approval1.getLabel()).isEqualTo("Verified"); - try (RevWalk rw = new RevWalk(repo)) { - bru.execute(rw, NullProgressMonitor.INSTANCE); - } - - assertThat(cmds.get(0).getResult()).isEqualTo(ReceiveCommand.Result.OK); - assertThat(cmds.get(1).getResult()).isEqualTo(ReceiveCommand.Result.OK); - - assertThat(repo.exactRef(update1.getRefName())).isNotNull(); - assertThat(repo.exactRef(update2.getRefName())).isNotNull(); + PatchSetApproval approval2 = newNotes(c2).getApprovals() + .get(c2.currentPatchSetId()).iterator().next(); + assertThat(approval2.getLabel()).isEqualTo("Code-Review"); } @Test @@ -1150,7 +1129,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1, (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); - update.upsertComment(comment1); + update.putComment(comment1); update.commit(); update = newUpdate(c, otherUser); @@ -1159,7 +1138,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid2, range2, range2.getEndLine(), otherUser, null, time2, message2, (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); - update.upsertComment(comment2); + update.putComment(comment2); update.commit(); update = newUpdate(c, otherUser); @@ -1168,14 +1147,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid3, range3, range3.getEndLine(), otherUser, null, time3, message3, (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); - update.upsertComment(comment3); + update.putComment(comment3); update.commit(); ChangeNotes notes = newNotes(c); try (RevWalk walk = new RevWalk(repo)) { ArrayList notesInTree = - Lists.newArrayList(notes.getNoteMap().iterator()); + Lists.newArrayList(notes.revisionNoteMap.noteMap.iterator()); Note note = Iterables.getOnlyElement(notesInTree); byte[] bytes = @@ -1229,7 +1208,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1, (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); - update.upsertComment(comment1); + update.putComment(comment1); update.commit(); update = newUpdate(c, otherUser); @@ -1238,14 +1217,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid2, range2, range2.getEndLine(), otherUser, null, time2, message2, (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); - update.upsertComment(comment2); + update.putComment(comment2); update.commit(); ChangeNotes notes = newNotes(c); try (RevWalk walk = new RevWalk(repo)) { ArrayList notesInTree = - Lists.newArrayList(notes.getNoteMap().iterator()); + Lists.newArrayList(notes.revisionNoteMap.noteMap.iterator()); Note note = Iterables.getOnlyElement(notesInTree); byte[] bytes = @@ -1293,7 +1272,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { range, range.getEndLine(), otherUser, null, now, messageForBase, (short) 0, rev1); update.setPatchSetId(psId); - update.upsertComment(commentForBase); + update.putComment(commentForBase); update.commit(); update = newUpdate(c, otherUser); @@ -1302,7 +1281,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { range, range.getEndLine(), otherUser, null, now, messageForPS, (short) 1, rev2); update.setPatchSetId(psId); - update.upsertComment(commentForPS); + update.putComment(commentForPS); update.commit(); assertThat(newNotes(c).getComments()).containsExactlyEntriesIn( @@ -1329,7 +1308,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid1, range, range.getEndLine(), otherUser, null, timeForComment1, "comment 1", side, rev); update.setPatchSetId(psId); - update.upsertComment(comment1); + update.putComment(comment1); update.commit(); update = newUpdate(c, otherUser); @@ -1337,7 +1316,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid2, range, range.getEndLine(), otherUser, null, timeForComment2, "comment 2", side, rev); update.setPatchSetId(psId); - update.upsertComment(comment2); + update.putComment(comment2); update.commit(); assertThat(newNotes(c).getComments()).containsExactlyEntriesIn( @@ -1364,7 +1343,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid, range, range.getEndLine(), otherUser, null, now, "comment 1", side, rev); update.setPatchSetId(psId); - update.upsertComment(comment1); + update.putComment(comment1); update.commit(); update = newUpdate(c, otherUser); @@ -1372,7 +1351,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid, range, range.getEndLine(), otherUser, null, now, "comment 2", side, rev); update.setPatchSetId(psId); - update.upsertComment(comment2); + update.putComment(comment2); update.commit(); assertThat(newNotes(c).getComments()).containsExactlyEntriesIn( @@ -1398,7 +1377,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps1", side, rev1); update.setPatchSetId(ps1); - update.upsertComment(comment1); + update.putComment(comment1); update.commit(); incrementPatchSet(c); @@ -1410,7 +1389,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps2", side, rev2); update.setPatchSetId(ps2); - update.upsertComment(comment2); + update.putComment(comment2); update.commit(); assertThat(newNotes(c).getComments()).containsExactlyEntriesIn( @@ -1435,7 +1414,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { range.getEndLine(), otherUser, null, now, "comment on ps1", side, rev, Status.DRAFT); update.setPatchSetId(ps1); - update.insertComment(comment1); + update.putComment(comment1); update.commit(); ChangeNotes notes = newNotes(c); @@ -1446,7 +1425,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { comment1.setStatus(Status.PUBLISHED); update = newUpdate(c, otherUser); update.setPatchSetId(ps1); - update.updateComment(comment1); + update.putComment(comment1); update.commit(); notes = newNotes(c); @@ -1478,8 +1457,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { PatchLineComment comment2 = newComment(psId, filename, uuid2, range2, range2.getEndLine(), otherUser, null, now, "other on ps1", side, rev, Status.DRAFT); - update.insertComment(comment1); - update.insertComment(comment2); + update.putComment(comment1); + update.putComment(comment2); update.commit(); ChangeNotes notes = newNotes(c); @@ -1493,7 +1472,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { update = newUpdate(c, otherUser); update.setPatchSetId(psId); comment1.setStatus(Status.PUBLISHED); - update.updateComment(comment1); + update.putComment(comment1); update.commit(); notes = newNotes(c); @@ -1527,8 +1506,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { range2, range2.getEndLine(), otherUser, null, now, "comment on ps", (short) 1, rev2, Status.DRAFT); - update.insertComment(baseComment); - update.insertComment(psComment); + update.putComment(baseComment); + update.putComment(psComment); update.commit(); ChangeNotes notes = newNotes(c); @@ -1544,8 +1523,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { baseComment.setStatus(Status.PUBLISHED); psComment.setStatus(Status.PUBLISHED); - update.updateComment(baseComment); - update.updateComment(psComment); + update.putComment(baseComment); + update.putComment(psComment); update.commit(); notes = newNotes(c); @@ -1573,7 +1552,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { range.getEndLine(), otherUser, null, now, "comment on ps1", side, rev, Status.DRAFT); update.setPatchSetId(psId); - update.upsertComment(comment); + update.putComment(comment); update.commit(); ChangeNotes notes = newNotes(c); @@ -1612,7 +1591,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps1", side, rev1, Status.DRAFT); update.setPatchSetId(ps1); - update.upsertComment(comment1); + update.putComment(comment1); update.commit(); incrementPatchSet(c); @@ -1624,7 +1603,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps2", side, rev2, Status.DRAFT); update.setPatchSetId(ps2); - update.upsertComment(comment2); + update.putComment(comment2); update.commit(); ChangeNotes notes = newNotes(c); @@ -1657,7 +1636,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { psId, "filename", uuid, null, 0, otherUser, null, now, messageForBase, (short) 0, rev); update.setPatchSetId(psId); - update.upsertComment(comment); + update.putComment(comment); update.commit(); assertThat(newNotes(c).getComments()).containsExactlyEntriesIn( @@ -1678,7 +1657,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { psId, "filename", uuid, null, 1, otherUser, null, now, messageForBase, (short) 0, rev); update.setPatchSetId(psId); - update.upsertComment(comment); + update.putComment(comment); update.commit(); assertThat(newNotes(c).getComments()).containsExactlyEntriesIn( @@ -1686,7 +1665,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { } @Test - public void updateCommentsForMultipleRevisions() throws Exception { + public void putCommentsForMultipleRevisions() throws Exception { Change c = newChange(); String uuid = "uuid"; String rev1 = "abcd1234abcd1234abcd1234abcd1234abcd1234"; @@ -1708,8 +1687,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { PatchLineComment comment2 = newComment(ps2, filename, uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps2", side, rev2, Status.DRAFT); - update.upsertComment(comment1); - update.upsertComment(comment2); + update.putComment(comment1); + update.putComment(comment2); update.commit(); ChangeNotes notes = newNotes(c); @@ -1720,8 +1699,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { update.setPatchSetId(ps2); comment1.setStatus(Status.PUBLISHED); comment2.setStatus(Status.PUBLISHED); - update.upsertComment(comment1); - update.upsertComment(comment2); + update.putComment(comment1); + update.putComment(comment2); update.commit(); notes = newNotes(c); @@ -1729,9 +1708,170 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(notes.getComments()).hasSize(2); } + @Test + public void publishSubsetOfCommentsOnRevision() throws Exception { + Change c = newChange(); + RevId rev1 = new RevId("abcd1234abcd1234abcd1234abcd1234abcd1234"); + CommentRange range = new CommentRange(1, 1, 2, 1); + PatchSet.Id ps1 = c.currentPatchSetId(); + short side = (short) 1; + + ChangeUpdate update = newUpdate(c, otherUser); + update.setPatchSetId(ps1); + Timestamp now = TimeUtil.nowTs(); + PatchLineComment comment1 = newComment(ps1, "file1", + "uuid1", range, range.getEndLine(), otherUser, null, now, "comment1", + side, rev1.get(), Status.DRAFT); + PatchLineComment comment2 = newComment(ps1, "file2", + "uuid2", range, range.getEndLine(), otherUser, null, now, "comment2", + side, rev1.get(), Status.DRAFT); + update.putComment(comment1); + update.putComment(comment2); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getDraftComments(otherUserId).get(rev1)) + .containsExactly(comment1, comment2); + assertThat(notes.getComments()).isEmpty(); + + update = newUpdate(c, otherUser); + update.setPatchSetId(ps1); + comment2.setStatus(Status.PUBLISHED); + update.putComment(comment2); + update.commit(); + + notes = newNotes(c); + assertThat(notes.getDraftComments(otherUserId).get(rev1)) + .containsExactly(comment1); + assertThat(notes.getComments().get(rev1)).containsExactly(comment2); + } + + @Test + public void updateWithServerIdent() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, internalUser); + update.setChangeMessage("A message."); + update.commit(); + + ChangeMessage msg = Iterables.getLast(newNotes(c).getChangeMessages()); + assertThat(msg.getMessage()).isEqualTo("A message."); + assertThat(msg.getAuthor()).isNull(); + + update = newUpdate(c, internalUser); + exception.expect(UnsupportedOperationException.class); + update.putApproval("Code-Review", (short) 1); + } + + @Test + public void filterOutAndFixUpZombieDraftComments() throws Exception { + Change c = newChange(); + RevId rev1 = new RevId("abcd1234abcd1234abcd1234abcd1234abcd1234"); + CommentRange range = new CommentRange(1, 1, 2, 1); + PatchSet.Id ps1 = c.currentPatchSetId(); + short side = (short) 1; + + ChangeUpdate update = newUpdate(c, otherUser); + Timestamp now = TimeUtil.nowTs(); + PatchLineComment comment1 = newComment(ps1, "file1", + "uuid1", range, range.getEndLine(), otherUser, null, now, "comment on ps1", + side, rev1.get(), Status.DRAFT); + PatchLineComment comment2 = newComment(ps1, "file2", + "uuid2", range, range.getEndLine(), otherUser, null, now, "another comment", + side, rev1.get(), Status.DRAFT); + update.putComment(comment1); + update.putComment(comment2); + update.commit(); + + + String refName = RefNames.refsDraftComments(otherUserId, c.getId()); + ObjectId oldDraftId = exactRefAllUsers(refName); + + update = newUpdate(c, otherUser); + update.setPatchSetId(ps1); + comment2.setStatus(Status.PUBLISHED); + update.putComment(comment2); + update.commit(); + assertThat(exactRefAllUsers(refName)).isNotNull(); + assertThat(exactRefAllUsers(refName)).isNotEqualTo(oldDraftId); + + // Re-add draft version of comment2 back to draft ref without updating + // change ref. Simulates the case where deleting the draft failed + // non-atomically after adding the published comment succeeded. + ChangeDraftUpdate draftUpdate = + newUpdate(c, otherUser).createDraftUpdateIfNull(); + comment2.setStatus(Status.DRAFT); + draftUpdate.putComment(comment2); + NoteDbUpdateManager manager = updateManagerFactory.create(c.getProject()); + manager.add(draftUpdate); + manager.execute(); + + // Looking at drafts directly shows the zombie comment. + DraftCommentNotes draftNotes = + draftNotesFactory.create(c.getId(), otherUserId); + assertThat(draftNotes.load().getComments().get(rev1)) + .containsExactly(comment1, comment2); + + comment2.setStatus(Status.PUBLISHED); // Reset for later assertions. + + // Zombie comment is filtered out of drafts via ChangeNotes. + ChangeNotes notes = newNotes(c); + assertThat(notes.getDraftComments(otherUserId).get(rev1)) + .containsExactly(comment1); + assertThat(notes.getComments().get(rev1)) + .containsExactly(comment2); + + update = newUpdate(c, otherUser); + update.setPatchSetId(ps1); + comment1.setStatus(Status.PUBLISHED); + update.putComment(comment1); + update.commit(); + + // Updating an unrelated comment causes the zombie comment to get fixed up. + assertThat(exactRefAllUsers(refName)).isNull(); + } + + @Test + public void updateCommentsInSequentialUpdates() throws Exception { + Change c = newChange(); + CommentRange range = new CommentRange(1, 1, 2, 1); + String rev = "abcd1234abcd1234abcd1234abcd1234abcd1234"; + + ChangeUpdate update1 = newUpdate(c, otherUser); + PatchLineComment comment1 = newComment(c.currentPatchSetId(), "filename", + "uuid1", range, range.getEndLine(), otherUser, null, + new Timestamp(update1.getWhen().getTime()), "comment 1", (short) 1, rev, + Status.PUBLISHED); + update1.putComment(comment1); + + ChangeUpdate update2 = newUpdate(c, otherUser); + PatchLineComment comment2 = newComment(c.currentPatchSetId(), "filename", + "uuid2", range, range.getEndLine(), otherUser, null, + new Timestamp(update2.getWhen().getTime()), "comment 2", (short) 1, rev, + Status.PUBLISHED); + update2.putComment(comment2); + + NoteDbUpdateManager manager = updateManagerFactory.create(project); + manager.add(update1); + manager.add(update2); + manager.execute(); + + ChangeNotes notes = newNotes(c); + List comments = notes.getComments().get(new RevId(rev)); + assertThat(comments).hasSize(2); + assertThat(comments.get(0).getMessage()).isEqualTo("comment 1"); + assertThat(comments.get(1).getMessage()).isEqualTo("comment 2"); + } + private String readNote(ChangeNotes notes, ObjectId noteId) throws Exception { - ObjectId dataId = notes.getNoteMap().getNote(noteId).getData(); + ObjectId dataId = notes.revisionNoteMap.noteMap.getNote(noteId).getData(); return new String( rw.getObjectReader().open(dataId, OBJ_BLOB).getCachedBytes(), UTF_8); } + + private ObjectId exactRefAllUsers(String refName) throws Exception { + try (Repository allUsersRepo = repoManager.openRepository(allUsers)) { + Ref ref = allUsersRepo.exactRef(refName); + return ref != null ? ref.getObjectId() : null; + } + } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java index 943499b523..907db6b229 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java @@ -45,7 +45,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { update.commit(); assertThat(update.getRefName()).isEqualTo("refs/changes/01/1/meta"); - RevCommit commit = parseCommit(update.getRevision()); + RevCommit commit = parseCommit(update.getResult()); assertBodyEquals("Update patch set 1\n" + "\n" + "Patch-set: 1\n" @@ -93,7 +93,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "Subject: Change subject\n" + "Branch: refs/heads/master\n" + "Commit: " + update.getCommit().name() + "\n", - update.getRevision()); + update.getResult()); } @Test @@ -115,7 +115,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "Subject: Subject\n" + "Branch: refs/heads/master\n" + "Commit: " + commit.name() + "\n", - update.getRevision()); + update.getResult()); } @Test @@ -129,7 +129,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "\n" + "Patch-set: 1\n" + "Label: -Code-Review\n", - update.getRevision()); + update.getResult()); } @Test @@ -147,7 +147,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { submitLabel("Alternative-Code-Review", "NEED", null)))); update.commit(); - RevCommit commit = parseCommit(update.getRevision()); + RevCommit commit = parseCommit(update.getResult()); assertBodyEquals("Submit patch set 1\n" + "\n" + "Patch-set: 1\n" @@ -185,7 +185,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { update.setChangeMessage("Comment on the change."); update.commit(); - RevCommit commit = parseCommit(update.getRevision()); + RevCommit commit = parseCommit(update.getResult()); assertBodyEquals("Update patch set 1\n" + "\n" + "Comment on the change.\n" @@ -214,7 +214,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "Status: merged\n" + "Submission-id: 1-1453387607626-96fabc25\n" + "Submitted-with: RULE_ERROR Problem with patch set: 1\n", - update.getRevision()); + update.getResult()); } @Test @@ -228,7 +228,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "\n" + "Patch-set: 1\n" + "Reviewer: Change Owner <1@gerrit>\n", - update.getRevision()); + update.getResult()); } @Test @@ -246,7 +246,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "\n" + "\n" + "Patch-set: 1\n", - update.getRevision()); + update.getResult()); } @Test @@ -269,7 +269,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { + "Testing paragraph 3\n" + "\n" + "Patch-set: 1\n", - update.getRevision()); + update.getResult()); } private RevCommit parseCommit(ObjectId id) throws Exception { diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java index 1f7ea7a94e..88ab5514f3 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java @@ -27,7 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetInfo; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; -import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeDraftUpdate; @@ -88,14 +88,14 @@ public class TestChanges { public static ChangeUpdate newUpdate(Injector injector, GitRepositoryManager repoManager, NotesMigration migration, Change c, - final AllUsersName allUsers, final IdentifiedUser user) + final AllUsersName allUsers, final CurrentUser user) throws Exception { ChangeUpdate update = injector.createChildInjector(new FactoryModule() { @Override public void configure() { factory(ChangeUpdate.Factory.class); factory(ChangeDraftUpdate.Factory.class); - bind(IdentifiedUser.class).toInstance(user); + bind(CurrentUser.class).toInstance(user); } }).getInstance(ChangeUpdate.Factory.class).create( stubChangeControl(repoManager, migration, c, allUsers, user), @@ -112,8 +112,8 @@ public class TestChanges { // first patch set, so create one. try (Repository repo = repoManager.openRepository(c.getProject())) { TestRepository tr = new TestRepository<>(repo); - PersonIdent ident = - user.newCommitterIdent(update.getWhen(), TimeZone.getDefault()); + PersonIdent ident = user.asIdentifiedUser() + .newCommitterIdent(update.getWhen(), TimeZone.getDefault()); TestRepository.CommitBuilder cb = tr.commit() .author(ident) .committer(ident) @@ -132,7 +132,7 @@ public class TestChanges { private static ChangeControl stubChangeControl( GitRepositoryManager repoManager, NotesMigration migration, Change c, AllUsersName allUsers, - IdentifiedUser user) throws OrmException { + CurrentUser user) throws OrmException { ChangeControl ctl = EasyMock.createMock(ChangeControl.class); expect(ctl.getChange()).andStubReturn(c); expect(ctl.getProject()).andStubReturn(new Project(c.getProject()));