diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index f84615105f..b88278eabc 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -14,19 +14,22 @@ package com.google.gerrit.acceptance.server.change; -import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; -import static com.google.gerrit.testutil.TestChanges.newChange; +import static com.google.gerrit.extensions.common.ProblemInfo.Status.FIXED; +import static com.google.gerrit.extensions.common.ProblemInfo.Status.FIX_FAILED; import static com.google.gerrit.testutil.TestChanges.newPatchSet; import static java.util.Collections.singleton; -import com.google.common.base.Function; -import com.google.common.collect.Lists; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.common.FooterConstants; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.api.changes.FixInput; +import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ProblemInfo; @@ -34,22 +37,39 @@ 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.reviewdb.client.RefNames; +import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.Sequences; +import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ConsistencyChecker; -import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.change.PatchSetInserter; +import com.google.gerrit.server.config.AnonymousCowardName; +import com.google.gerrit.server.git.BatchUpdate; +import com.google.gerrit.server.git.BatchUpdate.ChangeContext; +import com.google.gerrit.server.git.BatchUpdate.RepoContext; +import com.google.gerrit.server.git.validators.CommitValidators; +import com.google.gerrit.server.notedb.ChangeNoteUtil; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.testutil.InMemoryRepositoryManager; import com.google.gerrit.testutil.TestChanges; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.ReceiveCommand; import org.junit.Before; import org.junit.Test; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; import java.util.List; @NoHttpd @@ -57,26 +77,37 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { @Inject private ChangeControl.GenericFactory changeControlFactory; - @Inject - private ChangeUpdate.Factory changeUpdateFactory; - @Inject private Provider checkerProvider; @Inject private IdentifiedUser.GenericFactory userFactory; + @Inject + private BatchUpdate.Factory updateFactory; + + @Inject + private ChangeInserter.Factory changeInserterFactory; + + @Inject + private PatchSetInserter.Factory patchSetInserterFactory; + + @Inject + private ChangeNoteUtil noteUtil; + + @Inject + @AnonymousCowardName + private String anonymousCowardName; + + @Inject + private Sequences sequences; + private RevCommit tip; private Account.Id adminId; private ConsistencyChecker checker; @Before public void setUp() throws Exception { - // TODO(dborowitz): Re-enable when ConsistencyChecker supports NoteDb. - // Note that we *do* want to enable these tests with GERRIT_CHECK_NOTEDB, as - // we need to be able to convert old, corrupt changes. However, those tests - // don't necessarily need to pass. - assume().that(notesMigration.enabled()).isFalse(); // Ignore client clone of project; repurpose as server-side TestRepository. testRepo = new TestRepository<>( (InMemoryRepository) repoManager.openRepository(project)); @@ -88,62 +119,56 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { @Test public void validNewChange() throws Exception { - Change c = insertChange(); - insertPatchSet(c); - incrementPatchSet(c); - insertPatchSet(c); - assertProblems(c); + assertNoProblems(insertChange(), null); } @Test public void validMergedChange() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - insertPatchSet(c); - incrementPatchSet(c); - - incrementPatchSet(c); - RevCommit commit2 = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, adminId); - db.patchSets().insert(singleton(ps2)); - - testRepo.branch(c.getDest().get()).update(commit2); - assertProblems(c); + ChangeControl ctl = mergeChange(incrementPatchSet(insertChange())); + assertNoProblems(ctl, null); } @Test public void missingOwner() throws Exception { - Change c = newChange(project, new Account.Id(2)); - db.changes().insert(singleton(c)); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - db.patchSets().insert(singleton(ps)); + TestAccount owner = accounts.create("missing"); + ChangeControl ctl = insertChange(owner); + db.accounts().deleteKeys(singleton(owner.getId())); - assertProblems(c, "Missing change owner: 2"); + assertProblems(ctl, null, + problem("Missing change owner: " + owner.getId())); } @Test public void missingRepo() throws Exception { - Change c = newChange(new Project.NameKey("otherproject"), adminId); - db.changes().insert(singleton(c)); - insertMissingPatchSet(c, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - assertProblems(c, "Destination repository not found: otherproject"); + // NoteDb can't have a change without a repo. + assume().that(notesMigration.enabled()).isFalse(); + + ChangeControl ctl = insertChange(); + Project.NameKey name = ctl.getProject().getNameKey(); + ((InMemoryRepositoryManager) repoManager).deleteRepository(name); + + assertProblems( + ctl, null, + problem("Destination repository not found: " + name)); } @Test public void invalidRevision() throws Exception { - Change c = insertChange(); + // NoteDb always parses the revision when inserting a patch set, so we can't + // create an invalid patch set. + assume().that(notesMigration.enabled()).isFalse(); - db.patchSets().insert(singleton(newPatchSet(c.currentPatchSetId(), - "fooooooooooooooooooooooooooooooooooooooo", adminId))); - incrementPatchSet(c); - insertPatchSet(c); + ChangeControl ctl = insertChange(); + PatchSet ps = newPatchSet( + ctl.getChange().currentPatchSetId(), + "fooooooooooooooooooooooooooooooooooooooo", + adminId); + db.patchSets().update(singleton(ps)); - assertProblems(c, - "Invalid revision on patch set 1:" - + " fooooooooooooooooooooooooooooooooooooooo"); + assertProblems( + ctl, null, + problem("Invalid revision on patch set 1:" + + " fooooooooooooooooooooooooooooooooooooooo")); } // No test for ref existing but object missing; InMemoryRepository won't let @@ -151,414 +176,429 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { @Test public void patchSetObjectAndRefMissing() throws Exception { - Change c = insertChange(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), - ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), adminId); - db.patchSets().insert(singleton(ps)); - - assertProblems(c, - "Ref missing: " + ps.getId().toRefName(), - "Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + ChangeControl ctl = insertChange(); + PatchSet ps = insertMissingPatchSet(ctl, rev); + ctl = reload(ctl); + assertProblems( + ctl, null, + problem("Ref missing: " + ps.getId().toRefName()), + problem( + "Object missing: patch set 2:" + + " deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); } @Test public void patchSetObjectAndRefMissingWithFix() throws Exception { - Change c = insertChange(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), - ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), adminId); - db.patchSets().insert(singleton(ps)); + String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + ChangeControl ctl = insertChange(); + PatchSet ps = insertMissingPatchSet(ctl, rev); + ctl = reload(ctl); String refName = ps.getId().toRefName(); - List problems = checker.check(c, new FixInput()).problems(); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + refName); - assertThat(p.status).isNull(); + assertProblems( + ctl, new FixInput(), + problem("Ref missing: " + refName), + problem("Object missing: patch set 2: " + rev)); } @Test public void patchSetRefMissing() throws Exception { - Change c = insertChange(); - PatchSet ps = insertPatchSet(c); - String refName = ps.getId().toRefName(); - testRepo.update("refs/other/foo", ObjectId.fromString(ps.getRevision().get())); + ChangeControl ctl = insertChange(); + testRepo.update( + "refs/other/foo", + ObjectId.fromString( + psUtil.current(db, ctl.getNotes()).getRevision().get())); + String refName = ctl.getChange().currentPatchSetId().toRefName(); deleteRef(refName); - assertProblems(c, "Ref missing: " + refName); + assertProblems(ctl, null, problem("Ref missing: " + refName)); } @Test public void patchSetRefMissingWithFix() throws Exception { - Change c = insertChange(); - PatchSet ps = insertPatchSet(c); - String refName = ps.getId().toRefName(); - testRepo.update("refs/other/foo", ObjectId.fromString(ps.getRevision().get())); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.update("refs/other/foo", ObjectId.fromString(rev)); + String refName = ctl.getChange().currentPatchSetId().toRefName(); deleteRef(refName); - List problems = checker.check(c, new FixInput()).problems(); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + refName); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Repaired patch set ref"); - + assertProblems( + ctl, new FixInput(), + problem("Ref missing: " + refName, FIXED, "Repaired patch set ref")); assertThat(testRepo.getRepository().exactRef(refName).getObjectId().name()) - .isEqualTo(ps.getRevision().get()); + .isEqualTo(rev); } @Test public void patchSetObjectAndRefMissingWithDeletingPatchSet() throws Exception { - Change c = insertChange(); - PatchSet ps1 = insertPatchSet(c); - incrementPatchSet(c); - PatchSet ps2 = insertMissingPatchSet(c, - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + ChangeControl ctl = insertChange(); + PatchSet ps1 = psUtil.current(db, ctl.getNotes()); + + String rev2 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + PatchSet ps2 = insertMissingPatchSet(ctl, rev2); + ctl = reload(ctl); FixInput fix = new FixInput(); fix.deletePatchSetIfCommitMissing = true; - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(2); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + ps2.getId().toRefName()); - assertThat(p.status).isNull(); - p = problems.get(1); - assertThat(p.message).isEqualTo( - "Object missing: patch set 2: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Deleted patch set"); + assertProblems( + ctl, fix, + problem("Ref missing: " + ps2.getId().toRefName()), + problem("Object missing: patch set 2: " + rev2, + FIXED, "Deleted patch set")); - c = notesFactory.createChecked(db, c).getChange(); - assertThat(c.currentPatchSetId().get()).isEqualTo(1); - assertThat(getPatchSet(ps1.getId())).isNotNull(); - assertThat(getPatchSet(ps2.getId())).isNull(); + ctl = reload(ctl); + assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(1); + assertThat(psUtil.get(db, ctl.getNotes(), ps1.getId())).isNotNull(); + assertThat(psUtil.get(db, ctl.getNotes(), ps2.getId())).isNull(); } @Test public void patchSetMultipleObjectsMissingWithDeletingPatchSets() throws Exception { - Change c = insertChange(); - PatchSet ps1 = insertPatchSet(c); + ChangeControl ctl = insertChange(); + PatchSet ps1 = psUtil.current(db, ctl.getNotes()); - incrementPatchSet(c); - PatchSet ps2 = insertMissingPatchSet(c, - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + String rev2 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + PatchSet ps2 = insertMissingPatchSet(ctl, rev2); - incrementPatchSet(c); - PatchSet ps3 = insertPatchSet(c); + ctl = incrementPatchSet(reload(ctl)); + PatchSet ps3 = psUtil.current(db, ctl.getNotes()); - incrementPatchSet(c); - PatchSet ps4 = insertMissingPatchSet(c, - "c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee"); + String rev4 = "c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee"; + PatchSet ps4 = insertMissingPatchSet(ctl, rev4); + ctl = reload(ctl); FixInput fix = new FixInput(); fix.deletePatchSetIfCommitMissing = true; - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(4); + assertProblems( + ctl, fix, + problem("Ref missing: " + ps2.getId().toRefName()), + problem("Object missing: patch set 2: " + rev2, + FIXED, "Deleted patch set"), + problem("Ref missing: " + ps4.getId().toRefName()), + problem("Object missing: patch set 4: " + rev4, + FIXED, "Deleted patch set")); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + ps4.getId().toRefName()); - assertThat(p.status).isNull(); - - p = problems.get(1); - assertThat(p.message).isEqualTo( - "Object missing: patch set 4: c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Deleted patch set"); - - p = problems.get(2); - assertThat(p.message).isEqualTo("Ref missing: " + ps2.getId().toRefName()); - assertThat(p.status).isNull(); - - p = problems.get(3); - assertThat(p.message).isEqualTo( - "Object missing: patch set 2: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Deleted patch set"); - - c = notesFactory.createChecked(db, c).getChange(); - assertThat(c.currentPatchSetId().get()).isEqualTo(3); - assertThat(getPatchSet(ps1.getId())).isNotNull(); - assertThat(getPatchSet(ps2.getId())).isNull(); - assertThat(getPatchSet(ps3.getId())).isNotNull(); - assertThat(getPatchSet(ps4.getId())).isNull(); + ctl = reload(ctl); + assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(3); + assertThat(psUtil.get(db, ctl.getNotes(), ps1.getId())).isNotNull(); + assertThat(psUtil.get(db, ctl.getNotes(), ps2.getId())).isNull(); + assertThat(psUtil.get(db, ctl.getNotes(), ps3.getId())).isNotNull(); + assertThat(psUtil.get(db, ctl.getNotes(), ps4.getId())).isNull(); } @Test public void onlyPatchSetObjectMissingWithFix() throws Exception { - Change c = insertChange(); - PatchSet ps1 = insertMissingPatchSet(c, - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + Change c = TestChanges.newChange( + project, admin.getId(), sequences.nextChangeId()); + PatchSet.Id psId = c.currentPatchSetId(); + String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + PatchSet ps = newPatchSet(psId, rev, adminId); + + db.changes().insert(singleton(c)); + db.patchSets().insert(singleton(ps)); + addNoteDbCommit( + c.getId(), + "Create change\n" + + "\n" + + "Patch-set: 1\n" + + "Branch: " + c.getDest().get() + "\n" + + "Change-id: " + c.getKey().get() + "\n" + + "Subject: Bogus subject\n" + + "Commit: " + rev + "\n" + + "Groups: " + rev + "\n"); + indexer.index(db, c.getProject(), c.getId()); + IdentifiedUser user = userFactory.create(admin.getId()); + ChangeControl ctl = changeControlFactory.controlFor( + db, c.getProject(), c.getId(), user); FixInput fix = new FixInput(); fix.deletePatchSetIfCommitMissing = true; - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(2); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + ps1.getId().toRefName()); - assertThat(p.status).isNull(); - p = problems.get(1); - assertThat(p.message).isEqualTo( - "Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIX_FAILED); - assertThat(p.outcome) - .isEqualTo("Cannot delete patch set; no patch sets would remain"); + assertProblems( + ctl, fix, + problem("Ref missing: " + ps.getId().toRefName()), + problem("Object missing: patch set 1: " + rev, + FIX_FAILED, "Cannot delete patch set; no patch sets would remain")); - c = notesFactory.createChecked(db, c).getChange(); - assertThat(c.currentPatchSetId().get()).isEqualTo(1); - assertThat(getPatchSet(ps1.getId())).isNotNull(); + ctl = reload(ctl); + assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(1); + assertThat(psUtil.current(db, ctl.getNotes())).isNotNull(); } @Test public void currentPatchSetMissing() throws Exception { - Change c = insertChange(); - assertProblems(c, "Current patch set 1 not found"); + // NoteDb can't create a change without a patch set. + assume().that(notesMigration.enabled()).isFalse(); + + ChangeControl ctl = insertChange(); + db.patchSets().deleteKeys(singleton(ctl.getChange().currentPatchSetId())); + assertProblems(ctl, null, problem("Current patch set 1 not found")); } @Test public void duplicatePatchSetRevisions() throws Exception { - Change c = insertChange(); - PatchSet ps1 = insertPatchSet(c); + ChangeControl ctl = insertChange(); + PatchSet ps1 = psUtil.current(db, ctl.getNotes()); String rev = ps1.getRevision().get(); - incrementPatchSet(c); - PatchSet ps2 = insertMissingPatchSet(c, rev); - updatePatchSetRef(ps2); - assertProblems(c, - "Multiple patch sets pointing to " + rev + ": [1, 2]"); + ctl = incrementPatchSet( + ctl, testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); + + assertProblems( + ctl, null, + problem("Multiple patch sets pointing to " + rev + ": [1, 2]")); } @Test public void missingDestRef() throws Exception { + ChangeControl ctl = insertChange(); + String ref = "refs/heads/master"; // Detach head so we're allowed to delete ref. testRepo.reset(testRepo.getRepository().exactRef(ref).getObjectId()); RefUpdate ru = testRepo.getRepository().updateRef(ref); ru.setForceUpdate(true); assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED); - Change c = insertChange(); - RevCommit commit = testRepo.commit().create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - updatePatchSetRef(ps); - db.patchSets().insert(singleton(ps)); - assertProblems(c, "Destination ref not found (may be new branch): " + ref); + assertProblems( + ctl, null, + problem("Destination ref not found (may be new branch): " + ref)); } @Test public void mergedChangeIsNotMerged() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - PatchSet ps = insertPatchSet(c); - String rev = ps.getRevision().get(); + ChangeControl ctl = insertChange(); - assertProblems(c, - "Patch set 1 (" + rev + ") is not merged into destination ref" - + " refs/heads/master (" + tip.name() - + "), but change status is MERGED"); + try (BatchUpdate bu = newUpdate(adminId)) { + bu.addOp(ctl.getId(), new BatchUpdate.Op() { + @Override + public boolean updateChange(ChangeContext ctx) throws OrmException { + ctx.getChange().setStatus(Change.Status.MERGED); + ctx.getUpdate(ctx.getChange().currentPatchSetId()) + .fixStatus(Change.Status.MERGED); + return true; + } + }); + bu.execute(); + } + ctl = reload(ctl); + + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + ObjectId tip = getDestRef(ctl); + assertProblems( + ctl, null, + problem( + "Patch set 1 (" + rev + ") is not merged into destination ref" + + " refs/heads/master (" + tip.name() + + "), but change status is MERGED")); } @Test public void newChangeIsMerged() throws Exception { - Change c = insertChange(); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - db.patchSets().insert(singleton(ps)); - testRepo.branch(c.getDest().get()).update(commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); - assertProblems(c, - "Patch set 1 (" + commit.name() + ") is merged into destination ref" - + " refs/heads/master (" + commit.name() - + "), but change status is NEW"); + assertProblems( + ctl, null, + problem( + "Patch set 1 (" + rev + ") is merged into destination ref" + + " refs/heads/master (" + rev + + "), but change status is NEW")); } @Test public void newChangeIsMergedWithFix() throws Exception { - Change c = insertChange(); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - db.patchSets().insert(singleton(ps)); - testRepo.branch(c.getDest().get()).update(commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); - List problems = checker.check(c, new FixInput()).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Patch set 1 (" + commit.name() + ") is merged into destination ref" - + " refs/heads/master (" + commit.name() - + "), but change status is NEW"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Marked change as merged"); + assertProblems( + ctl, new FixInput(), + problem( + "Patch set 1 (" + rev + ") is merged into destination ref" + + " refs/heads/master (" + rev + + "), but change status is NEW", + FIXED, "Marked change as merged")); - c = notesFactory.createChecked(db, c).getChange(); - assertThat(c.getStatus()).isEqualTo(Change.Status.MERGED); - assertProblems(c); + ctl = reload(ctl); + assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertNoProblems(ctl, null); } @Test public void extensionApiReturnsUpdatedValueAfterFix() throws Exception { - Change c = insertChange(); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - db.patchSets().insert(singleton(ps)); - testRepo.branch(c.getDest().get()).update(commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); ChangeInfo info = gApi.changes() - .id(c.getChangeId()) + .id(ctl.getId().get()) .info(); assertThat(info.status).isEqualTo(ChangeStatus.NEW); info = gApi.changes() - .id(c.getChangeId()) + .id(ctl.getId().get()) .check(new FixInput()); assertThat(info.status).isEqualTo(ChangeStatus.MERGED); } @Test public void expectedMergedCommitIsLatestPatchSet() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); - testRepo.update(c.getDest().get(), commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); FixInput fix = new FixInput(); - fix.expectMergedAs = commit.name(); - assertThat(checker.check(c, fix).problems()).isEmpty(); + fix.expectMergedAs = rev; + assertProblems( + ctl, fix, + problem( + "Patch set 1 (" + rev + ") is merged into destination ref" + + " refs/heads/master (" + rev + "), but change status is NEW", + FIXED, "Marked change as merged")); + + ctl = reload(ctl); + assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertNoProblems(ctl, null); } @Test public void expectedMergedCommitNotMergedIntoDestination() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); - testRepo.update(c.getDest().get(), commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); + testRepo.branch(ctl.getChange().getDest().get()).update(commit); FixInput fix = new FixInput(); RevCommit other = testRepo.commit().message(commit.getFullMessage()).create(); fix.expectMergedAs = other.name(); - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Expected merged commit " + other.name() - + " is not merged into destination ref refs/heads/master" - + " (" + commit.name() + ")"); + assertProblems( + ctl, fix, + problem( + "Expected merged commit " + other.name() + + " is not merged into destination ref refs/heads/master" + + " (" + commit.name() + ")")); } @Test public void createNewPatchSetForExpectedMergeCommitWithNoChangeId() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - RevCommit parent = - testRepo.branch(c.getDest().get()).commit().message("parent").create(); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); + ChangeControl ctl = insertChange(); + String dest = ctl.getChange().getDest().get(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); - RevCommit mergedAs = testRepo.commit().parent(parent) + RevCommit mergedAs = testRepo.commit().parent(commit.getParent(0)) .message(commit.getShortMessage()).create(); testRepo.getRevWalk().parseBody(mergedAs); assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)).isEmpty(); - testRepo.update(c.getDest().get(), mergedAs); + testRepo.update(dest, mergedAs); - assertProblems(c, "Patch set 1 (" + commit.name() + ") is not merged into" - + " destination ref refs/heads/master (" + mergedAs.name() - + "), but change status is MERGED"); + assertNoProblems(ctl, null); FixInput fix = new FixInput(); fix.expectMergedAs = mergedAs.name(); - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "No patch set found for merged commit " + mergedAs.name()); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Inserted as patch set 2"); + assertProblems( + ctl, fix, + problem( + "No patch set found for merged commit " + mergedAs.name(), + FIXED, "Inserted as patch set 2"), + problem( + "Expected merged commit " + mergedAs.name() + + " has no associated patch set", + FIXED, "Marked change as merged")); - c = notesFactory.createChecked(db, c).getChange(); - PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2); - assertThat(c.currentPatchSetId()).isEqualTo(psId2); - assertThat(getPatchSet(psId2).getRevision().get()) + ctl = reload(ctl); + PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2); + assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId2); + assertThat(psUtil.get(db, ctl.getNotes(), psId2).getRevision().get()) .isEqualTo(mergedAs.name()); - assertProblems(c); + assertNoProblems(ctl, null); } @Test public void createNewPatchSetForExpectedMergeCommitWithChangeId() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - RevCommit parent = - testRepo.branch(c.getDest().get()).commit().message("parent").create(); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); + ChangeControl ctl = insertChange(); + String dest = ctl.getChange().getDest().get(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); - RevCommit mergedAs = testRepo.commit().parent(parent) + RevCommit mergedAs = testRepo.commit().parent(commit.getParent(0)) .message(commit.getShortMessage() + "\n" + "\n" - + "Change-Id: " + c.getKey().get() + "\n").create(); + + "Change-Id: " + ctl.getChange().getKey().get() + "\n").create(); testRepo.getRevWalk().parseBody(mergedAs); assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)) - .containsExactly(c.getKey().get()); - testRepo.update(c.getDest().get(), mergedAs); + .containsExactly(ctl.getChange().getKey().get()); + testRepo.update(dest, mergedAs); - assertProblems(c, "Patch set 1 (" + commit.name() + ") is not merged into" - + " destination ref refs/heads/master (" + mergedAs.name() - + "), but change status is MERGED"); + assertNoProblems(ctl, null); FixInput fix = new FixInput(); fix.expectMergedAs = mergedAs.name(); - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "No patch set found for merged commit " + mergedAs.name()); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Inserted as patch set 2"); + assertProblems( + ctl, fix, + problem( + "No patch set found for merged commit " + mergedAs.name(), + FIXED, "Inserted as patch set 2"), + problem( + "Expected merged commit " + mergedAs.name() + + " has no associated patch set", + FIXED, "Marked change as merged")); - c = notesFactory.createChecked(db, c).getChange(); - PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2); - assertThat(c.currentPatchSetId()).isEqualTo(psId2); - assertThat(getPatchSet(psId2).getRevision().get()) + ctl = reload(ctl); + PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2); + assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId2); + assertThat(psUtil.get(db, ctl.getNotes(), psId2).getRevision().get()) .isEqualTo(mergedAs.name()); - assertProblems(c); + assertNoProblems(ctl, null); } @Test public void expectedMergedCommitIsOldPatchSetOfSameChange() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - PatchSet ps1 = insertPatchSet(c); + ChangeControl ctl = insertChange(); + PatchSet ps1 = psUtil.current(db, ctl.getNotes()); String rev1 = ps1.getRevision().get(); - incrementPatchSet(c); - PatchSet ps2 = insertPatchSet(c); - testRepo.branch(c.getDest().get()).update(parseCommit(ps1)); + ctl = incrementPatchSet(ctl); + PatchSet ps2 = psUtil.current(db, ctl.getNotes()); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev1))); FixInput fix = new FixInput(); fix.expectMergedAs = rev1; - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Expected merged commit " + rev1 + " corresponds to patch set " - + ps1.getId() + ", which is not the current patch set " + ps2.getId()); + assertProblems( + ctl, fix, + problem( + "Expected merged commit " + rev1 + " corresponds to patch set " + + ps1.getId() + ", which is not the current patch set " + + ps2.getId())); } @Test public void expectedMergedCommitWithMismatchedChangeId() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); + ChangeControl ctl = insertChange(); + String dest = ctl.getChange().getDest().get(); RevCommit parent = - testRepo.branch(c.getDest().get()).commit().message("parent").create(); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); + testRepo.branch(dest).commit().message("parent").create(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); + testRepo.branch(dest).update(commit); String badId = "I0000000000000000000000000000000000000000"; RevCommit mergedAs = testRepo.commit().parent(parent) @@ -567,85 +607,140 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { + "Change-Id: " + badId + "\n") .create(); testRepo.getRevWalk().parseBody(mergedAs); - testRepo.update(c.getDest().get(), mergedAs); + assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)) + .containsExactly(badId); + testRepo.update(dest, mergedAs); + + assertNoProblems(ctl, null); FixInput fix = new FixInput(); fix.expectMergedAs = mergedAs.name(); - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Expected merged commit " + mergedAs.name() + " has Change-Id: " - + badId + ", but expected " + c.getKey().get()); + assertProblems( + ctl, fix, + problem( + "Expected merged commit " + mergedAs.name() + " has Change-Id: " + + badId + ", but expected " + ctl.getChange().getKey().get())); } @Test public void expectedMergedCommitMatchesMultiplePatchSets() throws Exception { - Change c1 = insertChange(); - c1.setStatus(Change.Status.MERGED); - insertPatchSet(c1); + ChangeControl ctl1 = insertChange(); + PatchSet.Id psId1 = psUtil.current(db, ctl1.getNotes()).getId(); + String dest = ctl1.getChange().getDest().get(); + String rev = psUtil.current(db, ctl1.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); + testRepo.branch(dest).update(commit); - RevCommit commit = testRepo.branch(c1.getDest().get()).commit().create(); - Change c2 = insertChange(); - PatchSet ps2 = newPatchSet(c2.currentPatchSetId(), commit, adminId); - updatePatchSetRef(ps2); - db.patchSets().insert(singleton(ps2)); + ChangeControl ctl2 = insertChange(); + ctl2 = incrementPatchSet(ctl2, commit); + PatchSet.Id psId2 = psUtil.current(db, ctl2.getNotes()).getId(); - Change c3 = insertChange(); - PatchSet ps3 = newPatchSet(c3.currentPatchSetId(), commit, adminId); - updatePatchSetRef(ps3); - db.patchSets().insert(singleton(ps3)); + ChangeControl ctl3 = insertChange(); + ctl3 = incrementPatchSet(ctl3, commit); + PatchSet.Id psId3 = psUtil.current(db, ctl3.getNotes()).getId(); FixInput fix = new FixInput(); fix.expectMergedAs = commit.name(); - List problems = checker.check(c1, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Multiple patch sets for expected merged commit " + commit.name() - + ": [" + ps2.getId() + ", " + ps3.getId() + "]"); + assertProblems( + ctl1, fix, + problem( + "Multiple patch sets for expected merged commit " + commit.name() + + ": [" + psId1 + ", " + psId2 + ", " + psId3 + "]")); } - private Change insertChange() throws Exception { - Change c = newChange(project, adminId); - db.changes().insert(singleton(c)); - indexer.index(db, c); - - ChangeUpdate u = changeUpdateFactory.create( - changeControlFactory.controlFor(db, c, userFactory.create(adminId))); - u.setBranch(c.getDest().get()); - u.setChangeId(c.getKey().get()); - u.commit(); - - return c; + private BatchUpdate newUpdate(Account.Id owner) { + return updateFactory.create( + db, project, userFactory.create(owner), TimeUtil.nowTs()); } - private void incrementPatchSet(Change c) throws Exception { - TestChanges.incrementPatchSet(c); - db.changes().upsert(singleton(c)); + private ChangeControl insertChange() throws Exception { + return insertChange(admin); } - private PatchSet insertPatchSet(Change c) throws Exception { - db.changes().upsert(singleton(c)); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).message("Change " + c.getId().get()).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - updatePatchSetRef(ps); + + private ChangeControl insertChange(TestAccount owner) throws Exception { + return insertChange(owner, "refs/heads/master"); + } + + private ChangeControl insertChange(TestAccount owner, String dest) + throws Exception { + Change.Id id = new Change.Id(sequences.nextChangeId()); + ChangeInserter ins; + try (BatchUpdate bu = newUpdate(owner.getId())) { + RevCommit commit = patchSetCommit(new PatchSet.Id(id, 1)); + ins = changeInserterFactory + .create(id, commit, dest) + .setValidatePolicy(CommitValidators.Policy.NONE) + .setNotify(NotifyHandling.NONE) + .setFireRevisionCreated(false) + .setSendMail(false); + bu.insertChange(ins).execute(); + } + // Return control for admin regardless of owner. + return changeControlFactory.controlFor( + db, ins.getChange(), userFactory.create(adminId)); + } + + private PatchSet.Id nextPatchSetId(ChangeControl ctl) { + return ChangeUtil.nextPatchSetId(ctl.getChange().currentPatchSetId()); + } + + private ChangeControl incrementPatchSet(ChangeControl ctl) throws Exception { + return incrementPatchSet(ctl, patchSetCommit(nextPatchSetId(ctl))); + } + + private ChangeControl incrementPatchSet(ChangeControl ctl, + RevCommit commit) throws Exception { + PatchSetInserter ins; + try (BatchUpdate bu = newUpdate(ctl.getChange().getOwner())) { + ins = patchSetInserterFactory.create(ctl, nextPatchSetId(ctl), commit) + .setValidatePolicy(CommitValidators.Policy.NONE) + .setFireRevisionCreated(false) + .setSendMail(false); + bu.addOp(ctl.getId(), ins).execute(); + } + return reload(ctl); + } + + private ChangeControl reload(ChangeControl ctl) throws Exception { + return changeControlFactory.controlFor( + db, ctl.getChange().getProject(), ctl.getId(), ctl.getUser()); + } + + private RevCommit patchSetCommit(PatchSet.Id psId) throws Exception { + RevCommit c = testRepo + .commit() + .parent(tip) + .message("Change " + psId) + .create(); + return testRepo.parseBody(c); + } + + private PatchSet insertMissingPatchSet(ChangeControl ctl, String rev) + throws Exception { + // Don't use BatchUpdate since we're manually updating the meta ref rather + // than using ChangeUpdate. + String subject = "Subject for missing commit"; + Change c = new Change(ctl.getChange()); + PatchSet.Id psId = nextPatchSetId(ctl); + c.setCurrentPatchSet(psId, subject, c.getOriginalSubject()); + + PatchSet ps = newPatchSet(psId, rev, adminId); db.patchSets().insert(singleton(ps)); - return ps; - } + db.changes().update(singleton(c)); - private PatchSet insertMissingPatchSet(Change c, String id) throws Exception { - PatchSet ps = newPatchSet(c.currentPatchSetId(), - ObjectId.fromString(id), adminId); - db.patchSets().insert(singleton(ps)); - return ps; - } + addNoteDbCommit( + c.getId(), + "Update patch set " + psId.get() + "\n" + + "\n" + + "Patch-set: " + psId.get() + "\n" + + "Commit: " + rev + "\n" + + "Subject: " + subject + "\n"); + indexer.index(db, c.getProject(), c.getId()); - private void updatePatchSetRef(PatchSet ps) throws Exception { - testRepo.update(ps.getId().toRefName(), - ObjectId.fromString(ps.getRevision().get())); + return ps; } private void deleteRef(String refName) throws Exception { @@ -654,22 +749,82 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED); } - private RevCommit parseCommit(PatchSet ps) throws Exception { - RevCommit commit = testRepo.getRevWalk() - .parseCommit(ObjectId.fromString(ps.getRevision().get())); - testRepo.getRevWalk().parseBody(commit); - return commit; + private void addNoteDbCommit(Change.Id id, String commitMessage) + throws Exception { + if (!notesMigration.commitChangeWrites()) { + return; + } + PersonIdent committer = serverIdent.get(); + PersonIdent author = noteUtil.newIdent( + accountCache.get(admin.getId()).getAccount(), + committer.getWhen(), + committer, + anonymousCowardName); + testRepo.branch(RefNames.changeMetaRef(id)) + .commit() + .author(author) + .committer(committer) + .message(commitMessage) + .create(); } - private void assertProblems(Change c, String... expected) { - assertThat(Lists.transform(checker.check(c).problems(), - new Function() { - @Override - public String apply(ProblemInfo in) { - checkArgument(in.status == null, - "Status is not null: " + in.message); - return in.message; - } - })).containsExactly((Object[]) expected); + private ObjectId getDestRef(ChangeControl ctl) throws Exception { + return testRepo.getRepository() + .exactRef(ctl.getChange().getDest().get()) + .getObjectId(); + } + + private ChangeControl mergeChange(ChangeControl ctl) throws Exception { + final ObjectId oldId = getDestRef(ctl); + final ObjectId newId = ObjectId.fromString( + psUtil.current(db, ctl.getNotes()).getRevision().get()); + final String dest = ctl.getChange().getDest().get(); + + try (BatchUpdate bu = newUpdate(adminId)) { + bu.addOp(ctl.getId(), new BatchUpdate.Op() { + @Override + public void updateRepo(RepoContext ctx) throws IOException { + ctx.addRefUpdate(new ReceiveCommand(oldId, newId, dest)); + } + + @Override + public boolean updateChange(ChangeContext ctx) throws OrmException { + ctx.getChange().setStatus(Change.Status.MERGED); + ctx.getUpdate(ctx.getChange().currentPatchSetId()) + .fixStatus(Change.Status.MERGED); + return true; + } + }); + bu.execute(); + } + return reload(ctl); + } + + private static ProblemInfo problem(String message) { + ProblemInfo p = new ProblemInfo(); + p.message = message; + return p; + } + + private static ProblemInfo problem(String message, + ProblemInfo.Status status, String outcome) { + ProblemInfo p = problem(message); + p.status = checkNotNull(status); + p.outcome = checkNotNull(outcome); + return p; + } + + private void assertProblems(ChangeControl ctl, @Nullable FixInput fix, + ProblemInfo first, ProblemInfo... rest) { + List expected = new ArrayList<>(1 + rest.length); + expected.add(first); + expected.addAll(Arrays.asList(rest)); + assertThat(checker.check(ctl, fix).problems()) + .containsExactlyElementsIn(expected) + .inOrder(); + } + + private void assertNoProblems(ChangeControl ctl, @Nullable FixInput fix) { + assertThat(checker.check(ctl, fix).problems()).isEmpty(); } } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java index 4dd910d147..ff04fdcf80 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java @@ -14,6 +14,8 @@ package com.google.gerrit.extensions.common; +import java.util.Objects; + public class ProblemInfo { public enum Status { FIXED, FIX_FAILED @@ -23,6 +25,22 @@ public class ProblemInfo { public Status status; public String outcome; + @Override + public int hashCode() { + return Objects.hash(message, status, outcome); + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof ProblemInfo)) { + return false; + } + ProblemInfo p = (ProblemInfo) o; + return Objects.equals(message, p.message) + && Objects.equals(status, p.status) + && Objects.equals(outcome, p.outcome); + } + @Override public String toString() { StringBuilder sb = new StringBuilder(getClass().getSimpleName()) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index ce381867a4..158758b3b2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -355,7 +355,21 @@ public class ChangeJson { } private ChangeInfo checkOnly(ChangeData cd) { - ConsistencyChecker.Result result = checkerProvider.get().check(cd, fix); + ChangeControl ctl; + try { + ctl = cd.changeControl().forUser(userProvider.get()); + } catch (OrmException e) { + String msg = "Error loading change"; + log.warn(msg + " " + cd.getId(), e); + ChangeInfo info = new ChangeInfo(); + info._number = cd.getId().get(); + ProblemInfo p = new ProblemInfo(); + p.message = msg; + info.problems = Lists.newArrayList(p); + return info; + } + + ConsistencyChecker.Result result = checkerProvider.get().check(ctl, fix); ChangeInfo info; Change c = result.change(); if (c != null) { @@ -384,9 +398,11 @@ public class ChangeJson { Optional limitToPsId) throws PatchListNotAvailableException, GpgException, OrmException, IOException { ChangeInfo out = new ChangeInfo(); + CurrentUser user = userProvider.get(); + ChangeControl ctl = cd.changeControl().forUser(user); if (has(CHECK)) { - out.problems = checkerProvider.get().check(cd.change(), fix).problems(); + out.problems = checkerProvider.get().check(ctl, fix).problems(); // If any problems were fixed, the ChangeData needs to be reloaded. for (ProblemInfo p : out.problems) { if (p.status == ProblemInfo.Status.FIXED) { @@ -397,8 +413,6 @@ public class ChangeJson { } Change in = cd.change(); - CurrentUser user = userProvider.get(); - ChangeControl ctl = cd.changeControl().forUser(user); out.project = in.getProject().get(); out.branch = in.getDest().getShortName(); out.topic = in.getTopic(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java index 465ce95418..f4869bedb1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java @@ -18,12 +18,10 @@ import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.restapi.AuthException; -import com.google.gerrit.extensions.restapi.NotImplementedException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.RestReadView; -import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -32,27 +30,22 @@ import java.util.EnumSet; public class Check implements RestReadView, RestModifyView { - private final NotesMigration notesMigration; private final ChangeJson.Factory jsonFactory; @Inject - Check(NotesMigration notesMigration, - ChangeJson.Factory json) { - this.notesMigration = notesMigration; + Check(ChangeJson.Factory json) { this.jsonFactory = json; } @Override public Response apply(ChangeResource rsrc) throws RestApiException, OrmException { - checkEnabled(); return Response.withMustRevalidate(newChangeJson().format(rsrc)); } @Override public Response apply(ChangeResource rsrc, FixInput input) throws RestApiException, OrmException { - checkEnabled(); ChangeControl ctl = rsrc.getControl(); if (!ctl.isOwner() && !ctl.getProjectControl().isOwner() @@ -65,10 +58,4 @@ public class Check implements RestReadView, private ChangeJson newChangeJson() { return jsonFactory.create(EnumSet.of(ListChangesOption.CHECK)); } - - private void checkEnabled() throws NotImplementedException { - if (notesMigration.readChanges()) { - throw new NotImplementedException("check not implemented for NoteDb"); - } - } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index c0a92ba11a..b16d8b8555 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -14,7 +14,8 @@ package com.google.gerrit.server.change; -import static com.google.common.base.Preconditions.checkState; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES; import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering; import static com.google.gerrit.server.ChangeUtil.PS_ID_ORDER; @@ -39,23 +40,22 @@ 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.reviewdb.server.ReviewDb; +import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.git.BatchUpdate; +import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.git.validators.CommitValidators; -import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; -import com.google.gerrit.server.notedb.ChangeUpdate; -import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.notedb.PatchSetState; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; -import com.google.gerrit.server.query.change.ChangeData; -import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -78,9 +78,10 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Objects; +import java.util.Set; /** * Checks changes for various kinds of inconsistency and corruption. @@ -94,12 +95,10 @@ public class ConsistencyChecker { @AutoValue public abstract static class Result { - private static Result create(Change.Id id, List problems) { - return new AutoValue_ConsistencyChecker_Result(id, null, problems); - } - - private static Result create(Change c, List problems) { - return new AutoValue_ConsistencyChecker_Result(c.getId(), c, problems); + private static Result create(ChangeControl ctl, + List problems) { + return new AutoValue_ConsistencyChecker_Result( + ctl.getId(), ctl.getChange(), problems); } public abstract Change.Id id(); @@ -110,22 +109,20 @@ public class ConsistencyChecker { public abstract List problems(); } - private final Provider db; - private final GitRepositoryManager repoManager; - private final NotesMigration notesMigration; - private final Provider user; - private final Provider serverIdent; - private final PatchSetInfoFactory patchSetInfoFactory; - private final PatchSetInserter.Factory patchSetInserterFactory; private final BatchUpdate.Factory updateFactory; - private final ChangeIndexer indexer; private final ChangeControl.GenericFactory changeControlFactory; private final ChangeNotes.Factory notesFactory; - private final ChangeUpdate.Factory changeUpdateFactory; private final DynamicItem accountPatchReviewStore; + private final GitRepositoryManager repoManager; + private final PatchSetInfoFactory patchSetInfoFactory; + private final PatchSetInserter.Factory patchSetInserterFactory; + private final PatchSetUtil psUtil; + private final Provider user; + private final Provider serverIdent; + private final Provider db; private FixInput fix; - private Change change; + private ChangeControl ctl; private Repository repo; private RevWalk rw; @@ -137,67 +134,51 @@ public class ConsistencyChecker { private List problems; @Inject - ConsistencyChecker(Provider db, - GitRepositoryManager repoManager, - NotesMigration notesMigration, - Provider user, + ConsistencyChecker( @GerritPersonIdent Provider serverIdent, - PatchSetInfoFactory patchSetInfoFactory, - PatchSetInserter.Factory patchSetInserterFactory, BatchUpdate.Factory updateFactory, - ChangeIndexer indexer, ChangeControl.GenericFactory changeControlFactory, ChangeNotes.Factory notesFactory, - ChangeUpdate.Factory changeUpdateFactory, - DynamicItem accountPatchReviewStore) { + DynamicItem accountPatchReviewStore, + GitRepositoryManager repoManager, + PatchSetInfoFactory patchSetInfoFactory, + PatchSetInserter.Factory patchSetInserterFactory, + PatchSetUtil psUtil, + Provider user, + Provider db) { + this.accountPatchReviewStore = accountPatchReviewStore; + this.changeControlFactory = changeControlFactory; this.db = db; - this.notesMigration = notesMigration; - this.repoManager = repoManager; - this.user = user; - this.serverIdent = serverIdent; + this.notesFactory = notesFactory; this.patchSetInfoFactory = patchSetInfoFactory; this.patchSetInserterFactory = patchSetInserterFactory; + this.psUtil = psUtil; + this.repoManager = repoManager; + this.serverIdent = serverIdent; this.updateFactory = updateFactory; - this.indexer = indexer; - this.changeControlFactory = changeControlFactory; - this.notesFactory = notesFactory; - this.changeUpdateFactory = changeUpdateFactory; - this.accountPatchReviewStore = accountPatchReviewStore; + this.user = user; reset(); } private void reset() { - change = null; + ctl = null; repo = null; rw = null; problems = new ArrayList<>(); } - public Result check(ChangeData cd) { - return check(cd, null); + private Change change() { + return ctl.getChange(); } - public Result check(ChangeData cd, @Nullable FixInput f) { - reset(); - try { - return check(cd.change(), f); - } catch (OrmException e) { - error("Error looking up change", e); - return Result.create(cd.getId(), problems); - } - } - - public Result check(Change c) { - return check(c, null); - } - - public Result check(Change c, @Nullable FixInput f) { - reset(); - fix = f; - change = c; + public Result check(ChangeControl cc, @Nullable FixInput f) { + checkNotNull(cc); try { + reset(); + ctl = cc; + fix = f; checkImpl(); - return Result.create(c, problems); + return result(); } finally { if (rw != null) { rw.close(); @@ -209,8 +190,6 @@ public class ConsistencyChecker { } private void checkImpl() { - checkState(!notesMigration.readChanges(), - "ConsistencyChecker for NoteDb not yet implemented"); checkOwner(); checkCurrentPatchSetEntity(); @@ -226,8 +205,8 @@ public class ConsistencyChecker { private void checkOwner() { try { - if (db.get().accounts().get(change.getOwner()) == null) { - problem("Missing change owner: " + change.getOwner()); + if (db.get().accounts().get(change().getOwner()) == null) { + problem("Missing change owner: " + change().getOwner()); } } catch (OrmException e) { error("Failed to look up owner", e); @@ -236,10 +215,10 @@ public class ConsistencyChecker { private void checkCurrentPatchSetEntity() { try { - PatchSet.Id psId = change.currentPatchSetId(); - currPs = db.get().patchSets().get(psId); + currPs = psUtil.current(db.get(), ctl.getNotes()); if (currPs == null) { - problem(String.format("Current patch set %d not found", psId.get())); + problem(String.format("Current patch set %d not found", + change().currentPatchSetId().get())); } } catch (OrmException e) { error("Failed to look up current patch set", e); @@ -247,7 +226,7 @@ public class ConsistencyChecker { } private boolean openRepo() { - Project.NameKey project = change.getDest().getParentKey(); + Project.NameKey project = change().getDest().getParentKey(); try { repo = repoManager.openRepository(project); rw = new RevWalk(repo); @@ -262,13 +241,11 @@ public class ConsistencyChecker { private boolean checkPatchSets() { List all; try { - all = Lists.newArrayList(db.get().patchSets().byChange(change.getId())); + // Iterate in descending order. + all = PS_ID_ORDER.sortedCopy(psUtil.byChange(db.get(), ctl.getNotes())); } catch (OrmException e) { return error("Failed to look up patch sets", e); } - // Iterate in descending order so deletePatchSet can assume the latest patch - // set exists. - Collections.sort(all, PS_ID_ORDER.reverse()); patchSetsBySha = MultimapBuilder.hashKeys(all.size()) .treeSetValues(PS_ID_ORDER) .build(); @@ -287,6 +264,7 @@ public class ConsistencyChecker { refs = Collections.emptyMap(); } + List deletePatchSetOps = new ArrayList<>(); for (PatchSet ps : all) { // Check revision format. int psNum = ps.getId().get(); @@ -317,17 +295,21 @@ public class ConsistencyChecker { objId, String.format("patch set %d", psNum)); if (psCommit == null) { if (fix != null && fix.deletePatchSetIfCommitMissing) { - deletePatchSet(lastProblem(), change.getProject(), ps.getId()); + deletePatchSetOps.add( + new DeletePatchSetFromDbOp(lastProblem(), ps.getId())); } continue; } else if (refProblem != null && fix != null) { fixPatchSetRef(refProblem, ps); } - if (ps.getId().equals(change.currentPatchSetId())) { + if (ps.getId().equals(change().currentPatchSetId())) { currPsCommit = psCommit; } } + // Delete any bad patch sets found above, in a single update. + deletePatchSets(deletePatchSetOps); + // Check for duplicates. for (Map.Entry> e : patchSetsBySha.asMap().entrySet()) { @@ -342,7 +324,7 @@ public class ConsistencyChecker { } private void checkMerged() { - String refName = change.getDest().get(); + String refName = change().getDest().get(); Ref dest; try { dest = repo.getRefDatabase().exactRef(refName); @@ -375,22 +357,27 @@ public class ConsistencyChecker { } } + private ProblemInfo wrongChangeStatus(PatchSet.Id psId, RevCommit commit) { + String refName = change().getDest().get(); + return problem(String.format( + "Patch set %d (%s) is merged into destination ref %s (%s), but change" + + " status is %s", psId.get(), commit.name(), + refName, tip.name(), change().getStatus())); + } + private void checkMergedBitMatchesStatus(PatchSet.Id psId, RevCommit commit, boolean merged) { - String refName = change.getDest().get(); - if (merged && change.getStatus() != Change.Status.MERGED) { - ProblemInfo p = problem(String.format( - "Patch set %d (%s) is merged into destination ref %s (%s), but change" - + " status is %s", psId.get(), commit.name(), - refName, tip.name(), change.getStatus())); + String refName = change().getDest().get(); + if (merged && change().getStatus() != Change.Status.MERGED) { + ProblemInfo p = wrongChangeStatus(psId, commit); if (fix != null) { fixMerged(p); } - } else if (!merged && change.getStatus() == Change.Status.MERGED) { + } else if (!merged && change().getStatus() == Change.Status.MERGED) { problem(String.format("Patch set %d (%s) is not merged into" + " destination ref %s (%s), but change status is %s", currPs.getId().get(), commit.name(), refName, tip.name(), - change.getStatus())); + change().getStatus())); } } @@ -401,16 +388,12 @@ public class ConsistencyChecker { if (commit == null) { return; } - if (Objects.equals(commit, currPsCommit)) { - // Caller gave us latest patch set SHA-1; verified in checkPatchSets. - return; - } try { if (!rw.isMergedInto(commit, tip)) { problem(String.format("Expected merged commit %s is not merged into" + " destination ref %s (%s)", - commit.name(), change.getDest().get(), tip.name())); + commit.name(), change().getDest().get(), tip.name())); return; } @@ -425,8 +408,8 @@ public class ConsistencyChecker { } try { Change c = notesFactory.createChecked( - db.get(), change.getProject(), psId.getParentKey()).getChange(); - if (!c.getDest().equals(change.getDest())) { + db.get(), change().getProject(), psId.getParentKey()).getChange(); + if (!c.getDest().equals(change().getDest())) { continue; } } catch (OrmException | NoSuchChangeException e) { @@ -442,16 +425,17 @@ public class ConsistencyChecker { String changeId = Iterables.getFirst( commit.getFooterLines(FooterConstants.CHANGE_ID), null); // Missing Change-Id footer is ok, but mismatched is not. - if (changeId != null && !changeId.equals(change.getKey().get())) { + if (changeId != null && !changeId.equals(change().getKey().get())) { problem(String.format("Expected merged commit %s has Change-Id: %s," + " but expected %s", - commit.name(), changeId, change.getKey().get())); + commit.name(), changeId, change().getKey().get())); return; } - PatchSet.Id psId = insertPatchSet(commit); - if (psId != null) { - checkMergedBitMatchesStatus(psId, commit, true); - } + // TODO(dborowitz): Combine into one op. + insertPatchSet(commit); + fixMerged(problem(String.format( + "Expected merged commit %s has no associated patch set", + commit.name()))); break; case 1: @@ -460,10 +444,12 @@ public class ConsistencyChecker { // TODO(dborowitz): This could be fixed if it's an older patch set of // the current change. PatchSet.Id id = psIds.get(0); - if (!id.equals(change.currentPatchSetId())) { + if (id.equals(change().currentPatchSetId())) { + fixMerged(wrongChangeStatus(id, commit)); + } else { problem(String.format("Expected merged commit %s corresponds to" + " patch set %s, which is not the current patch set %s", - commit.name(), id, change.currentPatchSetId())); + commit.name(), id, change().currentPatchSetId())); } break; @@ -490,17 +476,14 @@ public class ConsistencyChecker { } try { - ChangeControl ctl = changeControlFactory - .controlFor(db.get(), change, user.get()); PatchSet.Id psId = - ChangeUtil.nextPatchSetId(repo, change.currentPatchSetId()); + ChangeUtil.nextPatchSetId(repo, change().currentPatchSetId()); PatchSetInserter inserter = patchSetInserterFactory.create(ctl, psId, commit); - try (BatchUpdate bu = updateFactory.create( - db.get(), change.getProject(), ctl.getUser(), TimeUtil.nowTs()); + try (BatchUpdate bu = newBatchUpdate(); ObjectInserter oi = repo.newObjectInserter()) { bu.setRepository(repo, rw, oi); - bu.addOp(change.getId(), inserter + bu.addOp(ctl.getId(), inserter .setValidatePolicy(CommitValidators.Policy.NONE) .setFireRevisionCreated(false) .setSendMail(false) @@ -509,7 +492,8 @@ public class ConsistencyChecker { "Patch set for merged commit inserted by consistency checker")); bu.execute(); } - change = inserter.getChange(); + ctl = changeControlFactory.controlFor( + db.get(), inserter.getChange(), ctl.getUser()); p.status = Status.FIXED; p.outcome = "Inserted as patch set " + psId.get(); return psId; @@ -523,30 +507,33 @@ public class ConsistencyChecker { } private void fixMerged(ProblemInfo p) { - try { - change = db.get().changes().atomicUpdate(change.getId(), - new AtomicUpdate() { - @Override - public Change update(Change c) { - c.setStatus(Change.Status.MERGED); - return c; - } - }); - ChangeUpdate changeUpdate = - changeUpdateFactory.create( - changeControlFactory.controlFor(db.get(), change, user.get())); - changeUpdate.fixStatus(Change.Status.MERGED); - changeUpdate.commit(); - indexer.index(db.get(), change); + try (BatchUpdate bu = newBatchUpdate(); + ObjectInserter oi = repo.newObjectInserter()) { + bu.setRepository(repo, rw, oi); + bu.addOp(ctl.getId(), new BatchUpdate.Op() { + @Override + public boolean updateChange(ChangeContext ctx) throws OrmException { + ctx.getChange().setStatus(Change.Status.MERGED); + ctx.getUpdate(ctx.getChange().currentPatchSetId()) + .fixStatus(Change.Status.MERGED); + return true; + } + }); + bu.execute(); p.status = Status.FIXED; p.outcome = "Marked change as merged"; - } catch (OrmException | IOException | NoSuchChangeException e) { - log.warn("Error marking " + change.getId() + "as merged", e); + } catch (UpdateException | RestApiException e) { + log.warn("Error marking " + ctl.getId() + "as merged", e); p.status = Status.FIX_FAILED; p.outcome = "Error updating status to merged"; } } + private BatchUpdate newBatchUpdate() { + return updateFactory.create( + db.get(), change().getProject(), ctl.getUser(), TimeUtil.nowTs()); + } + private void fixPatchSetRef(ProblemInfo p, PatchSet ps) { try { RefUpdate ru = repo.updateRef(ps.getId().toRefName()); @@ -582,59 +569,108 @@ public class ConsistencyChecker { } } - private void deletePatchSet(ProblemInfo p, Project.NameKey project, - PatchSet.Id psId) { - ReviewDb db = this.db.get(); - Change.Id cid = psId.getParentKey(); - try { - db.changes().beginTransaction(cid); - try { - ChangeNotes notes = notesFactory.createChecked(db, project, cid); - Change c = notes.getChange(); - if (psId.equals(c.currentPatchSetId())) { - List all = Lists.newArrayList(db.patchSets().byChange(cid)); - if (all.size() == 1 && all.get(0).getId().equals(psId)) { - p.status = Status.FIX_FAILED; - p.outcome = "Cannot delete patch set; no patch sets would remain"; - return; - } - // If there were multiple missing patch sets, assumes deletePatchSet - // has been called in decreasing order, so the max remaining PatchSet - // is the effective current patch set. - Collections.sort(all, PS_ID_ORDER.reverse()); - PatchSet.Id latest = null; - for (PatchSet ps : all) { - latest = ps.getId(); - if (!ps.getId().equals(psId)) { - break; - } - } - c.setCurrentPatchSet(patchSetInfoFactory.get(db, notes, latest)); - db.changes().update(Collections.singleton(c)); - } - - // Delete dangling primary key references. Don't delete ChangeMessages, - // which don't use patch sets as a primary key, and may provide useful - // historical information. - accountPatchReviewStore.get().clearReviewed(psId); - db.patchSetApprovals().delete( - db.patchSetApprovals().byPatchSet(psId)); - db.patchComments().delete( - db.patchComments().byPatchSet(psId)); - db.patchSets().deleteKeys(Collections.singleton(psId)); - db.commit(); - - p.status = Status.FIXED; - p.outcome = "Deleted patch set"; - } finally { - db.rollback(); + private void deletePatchSets(List ops) { + try (BatchUpdate bu = newBatchUpdate(); + ObjectInserter oi = repo.newObjectInserter()) { + bu.setRepository(repo, rw, oi); + for (DeletePatchSetFromDbOp op : ops) { + checkArgument(op.psId.getParentKey().equals(ctl.getId())); + bu.addOp(ctl.getId(), op); } - } catch (PatchSetInfoNotAvailableException | OrmException - | NoSuchChangeException e) { + bu.addOp(ctl.getId(), new UpdateCurrentPatchSetOp(ops)); + bu.execute(); + } catch (NoPatchSetsWouldRemainException e) { + for (DeletePatchSetFromDbOp op : ops) { + op.p.status = Status.FIX_FAILED; + op.p.outcome = e.getMessage(); + } + } catch (UpdateException | RestApiException e) { String msg = "Error deleting patch set"; - log.warn(msg + ' ' + psId, e); - p.status = Status.FIX_FAILED; - p.outcome = msg; + log.warn(msg + " of change " + ops.get(0).psId.getParentKey(), e); + for (DeletePatchSetFromDbOp op : ops) { + // Overwrite existing statuses that were set before the transaction was + // rolled back. + op.p.status = Status.FIX_FAILED; + op.p.outcome = msg; + } + } + } + + private class DeletePatchSetFromDbOp extends BatchUpdate.Op { + private final ProblemInfo p; + private final PatchSet.Id psId; + + private DeletePatchSetFromDbOp(ProblemInfo p, PatchSet.Id psId) { + this.p = p; + this.psId = psId; + } + + @Override + public boolean updateChange(ChangeContext ctx) + throws OrmException, PatchSetInfoNotAvailableException { + // Delete dangling key references. + ReviewDb db = DeleteDraftChangeOp.unwrap(ctx.getDb()); + accountPatchReviewStore.get().clearReviewed(psId); + db.changeMessages().delete( + db.changeMessages().byChange(psId.getParentKey())); + db.patchSetApprovals().delete( + db.patchSetApprovals().byPatchSet(psId)); + db.patchComments().delete( + db.patchComments().byPatchSet(psId)); + db.patchSets().deleteKeys(Collections.singleton(psId)); + + // NoteDb requires no additional fiddling; setting the state to deleted is + // sufficient to filter everything else out. + ctx.getUpdate(psId).setPatchSetState(PatchSetState.DELETED); + + p.status = Status.FIXED; + p.outcome = "Deleted patch set"; + return true; + } + } + + private static class NoPatchSetsWouldRemainException + extends RestApiException { + private static final long serialVersionUID = 1L; + + private NoPatchSetsWouldRemainException() { + super("Cannot delete patch set; no patch sets would remain"); + } + } + + private class UpdateCurrentPatchSetOp extends BatchUpdate.Op { + private final Set toDelete; + + private UpdateCurrentPatchSetOp(List deleteOps) { + toDelete = new HashSet<>(); + for (DeletePatchSetFromDbOp op : deleteOps) { + toDelete.add(op.psId); + } + } + + @Override + public boolean updateChange(ChangeContext ctx) + throws OrmException, PatchSetInfoNotAvailableException, + NoPatchSetsWouldRemainException { + if (!toDelete.contains(ctx.getChange().currentPatchSetId())) { + return false; + } + Set all = new HashSet<>(); + // Doesn't make any assumptions about the order in which deletes happen + // and whether they are seen by this op; we are already given the full set + // of patch sets that will eventually be deleted in this update. + for (PatchSet ps : psUtil.byChange(ctx.getDb(), ctx.getNotes())) { + if (!toDelete.contains(ps.getId())) { + all.add(ps.getId()); + } + } + if (all.isEmpty()) { + throw new NoPatchSetsWouldRemainException(); + } + PatchSet.Id latest = ReviewDbUtil.intKeyOrdering().max(all); + ctx.getChange().setCurrentPatchSet( + patchSetInfoFactory.get(ctx.getDb(), ctx.getNotes(), latest)); + return true; } } @@ -687,6 +723,10 @@ public class ConsistencyChecker { } private void warn(Throwable t) { - log.warn("Error in consistency check of change " + change.getId(), t); + log.warn("Error in consistency check of change " + ctl.getId(), t); + } + + private Result result() { + return Result.create(ctl, problems); } }