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
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 100e642..5b89110 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.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 @@
@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 @@
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 @@
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<ChangeControl> 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<RevisionResource> 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 f81c128..c3c99d0 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 @@
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 321b8fc..b383042 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 @@
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 @@
List<String> 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 @@
}
if (!hashes.isEmpty()) {
- // Merged changes are ok to exclude
Iterable<ChangeData> destChanges = queryProvider.get()
.byCommitsOnBranchNotMerged(
repo, db, cd.change().getDest(), hashes);
