Don't reuse ALREADY_MERGED for an identical tree
ALREADY_MERGED now means the change is already reachable from the
branch tip. This is different from the case where we catch
MergeIdenticalTreeException, which indicates the change was marked as
merged despite not actually appearing in the commit history.
Distinguish between these cases by using a newer, more descriptive
CommitMergeStatus. Add a test for this case, fixing an
IllegalStateException found in the process.
Change-Id: I842e579d12ce745b2bc84dfd93a9f5b7213c71a8
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
index 28742158..a59afc5 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
@@ -30,6 +30,7 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.change.Submit.TestSubmitInput;
+import com.google.gerrit.server.git.strategy.CommitMergeStatus;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -269,6 +270,31 @@
}
@Test
+ @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
+ public void submitIdenticalTree() throws Exception {
+ RevCommit initialHead = getRemoteHead();
+
+ PushOneCommit.Result change1 = createChange("Change 1", "a.txt", "a");
+
+ testRepo.reset(initialHead);
+ PushOneCommit.Result change2 = createChange("Change 2", "a.txt", "a");
+
+ submit(change1.getChangeId());
+ RevCommit oldHead = getRemoteHead();
+ assertThat(oldHead.getShortMessage()).isEqualTo("Change 1");
+
+ // Don't check merge result, since ref isn't updated.
+ submit(change2.getChangeId(), new SubmitInput(), null, null, false);
+
+ assertThat(getRemoteHead()).isEqualTo(oldHead);
+
+ ChangeInfo info2 = get(change2.getChangeId());
+ assertThat(info2.status).isEqualTo(ChangeStatus.MERGED);
+ assertThat(Iterables.getLast(info2.messages).message)
+ .isEqualTo(CommitMergeStatus.SKIPPED_IDENTICAL_TREE.getMessage());
+ }
+
+ @Test
public void repairChangeStateAfterFailure() throws Exception {
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change =
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
index b5ee57c..04625ed 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.git.strategy;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.server.git.strategy.CommitMergeStatus.SKIPPED_IDENTICAL_TREE;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.restapi.MergeConflictException;
@@ -126,7 +127,7 @@
toMerge.setStatusCode(CommitMergeStatus.PATH_CONFLICT);
return;
} catch (MergeIdenticalTreeException mie) {
- toMerge.setStatusCode(CommitMergeStatus.ALREADY_MERGED);
+ toMerge.setStatusCode(SKIPPED_IDENTICAL_TREE);
return;
}
// Initial copy doesn't have new patch set ID since change hasn't been
@@ -146,6 +147,10 @@
@Override
public PatchSet updateChangeImpl(ChangeContext ctx) throws OrmException,
NoSuchChangeException, IOException {
+ if (newCommit == null
+ && toMerge.getStatusCode() == SKIPPED_IDENTICAL_TREE) {
+ return null;
+ }
checkState(newCommit != null,
"no new commit produced by CherryPick of %s, expected to fail fast",
toMerge.change().getId());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CommitMergeStatus.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CommitMergeStatus.java
index cbce1bb..1102357 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CommitMergeStatus.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CommitMergeStatus.java
@@ -36,6 +36,9 @@
+ "\n"
+ "Please rebase the change locally and upload the rebased commit for review."),
+ SKIPPED_IDENTICAL_TREE(
+ "Marking change merged without cherry-picking to branch, as the resulting commit would be empty."),
+
MISSING_DEPENDENCY(""),
MANUAL_RECURSIVE_MERGE("The change requires a local merge to resolve.\n"
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java
index 1b3d4d8..8ed1b63 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java
@@ -106,6 +106,7 @@
case CLEAN_MERGE:
case CLEAN_REBASE:
case CLEAN_PICK:
+ case SKIPPED_IDENTICAL_TREE:
break; // Merge strategy accepted this change.
case ALREADY_MERGED:
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
index e5bc614..24daab6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
@@ -274,6 +274,8 @@
|| s == CommitMergeStatus.CLEAN_PICK) {
msg = message(ctx, commit.getPatchsetId(),
txt + " as " + commit.name() + getByAccountName());
+ } else if (s == CommitMergeStatus.SKIPPED_IDENTICAL_TREE) {
+ msg = message(ctx, commit.getPatchsetId(), txt);
} else if (s == CommitMergeStatus.ALREADY_MERGED) {
msg = null;
} else {
