From 6d1221a154aafe18eeb1da5ea971fc4fa398e57f Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 3 Feb 2016 14:07:22 -0500 Subject: [PATCH] MergeSuperSet: Don't guarantee change is present in output This sanity check was added I6922b597 to make SubmitStrategyOp able to fix changes that appeared merged in the repo but not the db. However, that's not the only case where MergeSuperSet is used: when checking the ETag of revisions for a merged change, we do want to use the empty ChangeSet, since once a change is merged we no longer need to consider dependent changes. Move the sanity check to MergeOp where it belongs, avoiding an ISE in GetRevisionActions. Add test coverage for this case. Change-Id: I2a57553b30eab85355f161914f79b6f30fff4e45 --- .../acceptance/api/revision/RevisionIT.java | 73 +++++++++++++++++++ .../com/google/gerrit/server/git/MergeOp.java | 2 + .../gerrit/server/git/MergeSuperSet.java | 14 +--- 3 files changed, 79 insertions(+), 10 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java index 100e64232f..5b891107c1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java @@ -47,8 +47,18 @@ import com.google.gerrit.extensions.common.MergeableInfo; import com.google.gerrit.extensions.common.RevisionInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BinaryResult; +import com.google.gerrit.extensions.restapi.ETagView; +import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.change.ChangeResource; +import com.google.gerrit.server.change.GetRevisionActions; +import com.google.gerrit.server.change.RevisionResource; +import com.google.gerrit.server.change.Revisions; +import com.google.gerrit.server.project.ChangeControl; +import com.google.inject.Inject; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.RefUpdate; @@ -69,6 +79,15 @@ import java.util.Map; @NoHttpd public class RevisionIT extends AbstractDaemonTest { + @Inject + private ChangeUtil changeUtil; + + @Inject + private GetRevisionActions getRevisionActions; + + @Inject + private Revisions revisions; + private TestAccount admin2; @Before @@ -618,6 +637,38 @@ public class RevisionIT extends AbstractDaemonTest { String.format(PATCH, r.getCommitId().name(), date, r.getChangeId())); } + @Test + public void actions() throws Exception { + PushOneCommit.Result r = createChange(); + assertThat(current(r).actions().keySet()) + .containsExactly("cherrypick", "rebase"); + + current(r).review(ReviewInput.approve()); + assertThat(current(r).actions().keySet()) + .containsExactly("submit", "cherrypick", "rebase"); + + current(r).submit(); + assertThat(current(r).actions().keySet()) + .containsExactly("cherrypick"); + } + + @Test + public void actionsETag() throws Exception { + PushOneCommit.Result r1 = createChange(); + PushOneCommit.Result r2 = createChange(); + + String oldETag = checkETag(getRevisionActions, r2, null); + current(r2).review(ReviewInput.approve()); + oldETag = checkETag(getRevisionActions, r2, oldETag); + + // Dependent change is included in ETag. + current(r1).review(ReviewInput.approve()); + oldETag = checkETag(getRevisionActions, r2, oldETag); + + current(r2).submit(); + oldETag = checkETag(getRevisionActions, r2, oldETag); + } + private void merge(PushOneCommit.Result r) throws Exception { revision(r).review(ReviewInput.approve()); revision(r).submit(); @@ -634,4 +685,26 @@ public class RevisionIT extends AbstractDaemonTest { PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo); return push.to("refs/drafts/master"); } + + private RevisionApi current(PushOneCommit.Result r) throws Exception { + return gApi.changes().id(r.getChangeId()).current(); + } + + private RevisionResource parseRevisionResource(PushOneCommit.Result r) + throws Exception { + PatchSet.Id psId = r.getPatchSetId(); + List ctls = changeUtil.findChanges( + Integer.toString(psId.getParentKey().get()), atrScope.get().getUser()); + assertThat(ctls).hasSize(1); + return revisions.parse( + new ChangeResource(ctls.get(0)), + IdString.fromDecoded(Integer.toString(psId.get()))); + } + + private String checkETag(ETagView view, + PushOneCommit.Result r, String oldETag) throws Exception { + String eTag = view.getETag(parseRevisionResource(r)); + assertThat(eTag).isNotEqualTo(oldETag); + return eTag; + } } 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 f81c128bf1..c3c99d078e 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 @@ -555,6 +555,8 @@ public class MergeOp implements AutoCloseable { logDebug("Beginning integration of {}", change); try { ChangeSet cs = mergeSuperSet.completeChangeSet(db, change); + checkState(cs.ids().contains(change.getId()), + "change %s missing from %s", change.getId(), cs); this.commits = new CommitStatus(cs); reloadChanges(cs); logDebug("Calculated to merge {}", cs); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java index 321b8fc649..b38304227f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java @@ -14,8 +14,6 @@ package com.google.gerrit.server.git; -import static com.google.common.base.Preconditions.checkState; - import com.google.common.base.Strings; import com.google.common.collect.Multimap; import com.google.gerrit.common.data.SubmitTypeRecord; @@ -86,15 +84,11 @@ public class MergeSuperSet { throws MissingObjectException, IncorrectObjectTypeException, IOException, OrmException { ChangeData cd = changeDataFactory.create(db, change.getId()); - ChangeSet result; if (Submit.wholeTopicEnabled(cfg)) { - result = completeChangeSetIncludingTopics(db, new ChangeSet(cd)); + return completeChangeSetIncludingTopics(db, new ChangeSet(cd)); } else { - result = completeChangeSetWithoutTopic(db, new ChangeSet(cd)); + return completeChangeSetWithoutTopic(db, new ChangeSet(cd)); } - checkState(result.ids().contains(change.getId()), - "change %s missing from result %s", change.getId(), result); - return result; } private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes) @@ -139,7 +133,8 @@ public class MergeSuperSet { List hashes = new ArrayList<>(); // Always include the input, even if merged. This allows - // SubmitStrategyOp to correct the situation later. + // SubmitStrategyOp to correct the situation later, assuming it gets + // returned by byCommitsOnBranchNotMerged below. hashes.add(objIdStr); for (RevCommit c : rw) { if (!c.equals(commit)) { @@ -148,7 +143,6 @@ public class MergeSuperSet { } if (!hashes.isEmpty()) { - // Merged changes are ok to exclude Iterable destChanges = queryProvider.get() .byCommitsOnBranchNotMerged( repo, db, cd.change().getDest(), hashes);