Merge "Ensure changes with outdated patchsets have a disabled submit button"
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index e77bfe7..790b2db 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -30,8 +30,10 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.SubmitTypeRecord;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.api.changes.SubmitInput;
+import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -66,12 +68,14 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.Arrays;
import java.util.Collection;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config;
@@ -373,8 +377,15 @@
public Collection<ChangeData> unmergeableChanges(ChangeSet cs) throws IOException {
Set<ChangeData> mergeabilityMap = new HashSet<>();
+ Set<ObjectId> outDatedPatchsets = new HashSet<>();
for (ChangeData change : cs.changes()) {
mergeabilityMap.add(change);
+ // Add all the patchsets commit ids except the current patchset.
+ outDatedPatchsets.addAll(
+ change.notes().getPatchSets().values().stream()
+ .map(p -> p.commitId())
+ .collect(Collectors.toSet()));
+ outDatedPatchsets.remove(change.currentPatchSet().commitId());
}
ListMultimap<BranchNameKey, ChangeData> cbb = cs.changesByBranch();
@@ -388,12 +399,17 @@
allParents.add(parent.getId());
}
}
-
for (ChangeData change : targetBranch) {
+
RevCommit commit = commits.get(change.getId());
boolean isMergeCommit = commit.getParentCount() > 1;
boolean isLastInChain = !allParents.contains(commit.getId());
-
+ if (Arrays.stream(commit.getParents()).anyMatch(c -> outDatedPatchsets.contains(c.getId()))
+ && !isCherryPickSubmit(change)) {
+ // Found a parent that depends on an outdated patchset and the submit strategy is not
+ // cherry-pick.
+ continue;
+ }
// Recheck mergeability rather than using value stored in the index,
// which may be stale.
// TODO(dborowitz): This is ugly; consider providing a way to not read
@@ -419,6 +435,11 @@
return mergeabilityMap;
}
+ private boolean isCherryPickSubmit(ChangeData changeData) {
+ SubmitTypeRecord submitTypeRecord = changeData.submitTypeRecord();
+ return submitTypeRecord.isOk() && submitTypeRecord.type == SubmitType.CHERRY_PICK;
+ }
+
private HashMap<Change.Id, RevCommit> findCommits(
Collection<ChangeData> changes, Project.NameKey project) throws IOException {
HashMap<Change.Id, RevCommit> commits = new HashMap<>();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index faef5aa..085d23d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -1439,6 +1439,12 @@
assertWithMessage("enabled bit on submit action").that(desc.isEnabled()).isTrue();
}
+ protected void assertSubmitDisabled(String changeId) throws Throwable {
+ RevisionResource rsrc = parseCurrentRevisionResource(changeId);
+ UiAction.Description desc = submitHandler.getDescription(rsrc);
+ assertWithMessage("enabled bit on submit action").that(desc.isEnabled()).isFalse();
+ }
+
protected void assertChangeMergedEvents(String... expected) throws Throwable {
eventRecorder.assertChangeMergedEvents(project.get(), "refs/heads/master", expected);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
index f77552d..9c496fa 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
@@ -20,6 +20,7 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.inject.Inject;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -117,4 +118,49 @@
assertThat(head.getParent(0)).isEqualTo(change1.getCommit());
assertThat(head.getParent(1)).isEqualTo(change2.getCommit());
}
+
+ @Test
+ public void dependencyOnOutdatedPatchSetPreventsMerge() throws Throwable {
+ // Create a change
+ PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
+ PushOneCommit.Result changeResult = change.to("refs/for/master");
+ PatchSet.Id patchSetId = changeResult.getPatchSetId();
+
+ // Create a successor change.
+ PushOneCommit change2 =
+ pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
+ PushOneCommit.Result change2Result = change2.to("refs/for/master");
+
+ // Create new patch set for first change.
+ testRepo.reset(changeResult.getCommit().name());
+ amendChange(changeResult.getChangeId());
+
+ // Approve both changes
+ approve(changeResult.getChangeId());
+ approve(change2Result.getChangeId());
+
+ // submit button is disabled.
+ assertSubmitDisabled(change2Result.getChangeId());
+
+ submitWithConflict(
+ change2Result.getChangeId(),
+ "Failed to submit 2 changes due to the following problems:\n"
+ + "Change "
+ + change2Result.getChange().getId()
+ + ": Depends on change that was not submitted."
+ + " Commit "
+ + change2Result.getCommit().name()
+ + " depends on commit "
+ + changeResult.getCommit().name()
+ + ", which is outdated patch set "
+ + patchSetId.get()
+ + " of change "
+ + changeResult.getChange().getId()
+ + ". The latest patch set is "
+ + changeResult.getPatchSetId().get()
+ + ".");
+
+ assertRefUpdatedEvents();
+ assertChangeMergedEvents();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
index 955dd7a..5eb19df 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
@@ -29,6 +29,7 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.ChangeStatus;
@@ -384,4 +385,49 @@
gApi.changes().id(change2.getChangeId()).current().rebase();
submit(change2.getChangeId());
}
+
+ @Test
+ public void dependencyOnOutdatedPatchSetPreventsRebase() throws Throwable {
+ // Create a change
+ PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
+ PushOneCommit.Result changeResult = change.to("refs/for/master");
+ PatchSet.Id patchSetId = changeResult.getPatchSetId();
+
+ // Create a successor change.
+ PushOneCommit change2 =
+ pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
+ PushOneCommit.Result change2Result = change2.to("refs/for/master");
+
+ // Create new patch set for first change.
+ testRepo.reset(changeResult.getCommit().name());
+ amendChange(changeResult.getChangeId());
+
+ // Approve both changes
+ approve(changeResult.getChangeId());
+ approve(change2Result.getChangeId());
+
+ // submit button is disabled.
+ assertSubmitDisabled(change2Result.getChangeId());
+
+ submitWithConflict(
+ change2Result.getChangeId(),
+ "Failed to submit 2 changes due to the following problems:\n"
+ + "Change "
+ + change2Result.getChange().getId()
+ + ": Depends on change that was not submitted."
+ + " Commit "
+ + change2Result.getCommit().name()
+ + " depends on commit "
+ + changeResult.getCommit().name()
+ + ", which is outdated patch set "
+ + patchSetId.get()
+ + " of change "
+ + changeResult.getChange().getId()
+ + ". The latest patch set is "
+ + changeResult.getPatchSetId().get()
+ + ".");
+
+ assertRefUpdatedEvents();
+ assertChangeMergedEvents();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
index 6f519f1..5bcf995 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
@@ -25,6 +25,7 @@
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.FooterConstants;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -373,4 +374,27 @@
change2.getChangeId(),
headAfterFirstSubmit.name());
}
+
+ @Test
+ public void dependencyOnOutdatedPatchSetNotPreventingCherryPick() throws Throwable {
+ // Create a change
+ PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
+ PushOneCommit.Result changeResult = change.to("refs/for/master");
+ PatchSet.Id patchSetId = changeResult.getPatchSetId();
+
+ // Create a successor change.
+ PushOneCommit change2 =
+ pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
+ PushOneCommit.Result change2Result = change2.to("refs/for/master");
+
+ // Create new patch set for first change.
+ testRepo.reset(changeResult.getCommit().name());
+ amendChange(changeResult.getChangeId());
+
+ // Approve both changes
+ approve(changeResult.getChangeId());
+ approve(change2Result.getChangeId());
+
+ assertSubmittable(change2Result.getChangeId());
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
index 1912697..66eb48c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ActionInfo;
@@ -170,4 +171,49 @@
assertRefUpdatedEvents(initialHead, headAfterSubmit);
assertChangeMergedEvents(id1, headAfterSubmit.name());
}
+
+ @Test
+ public void dependencyOnOutdatedPatchSetPreventsFastForward() throws Throwable {
+ // Create a change
+ PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
+ PushOneCommit.Result changeResult = change.to("refs/for/master");
+ PatchSet.Id patchSetId = changeResult.getPatchSetId();
+
+ // Create a successor change.
+ PushOneCommit change2 =
+ pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
+ PushOneCommit.Result change2Result = change2.to("refs/for/master");
+
+ // Create new patch set for first change.
+ testRepo.reset(changeResult.getCommit().name());
+ amendChange(changeResult.getChangeId());
+
+ // Approve both changes
+ approve(changeResult.getChangeId());
+ approve(change2Result.getChangeId());
+
+ // submit button is disabled.
+ assertSubmitDisabled(change2Result.getChangeId());
+
+ submitWithConflict(
+ change2Result.getChangeId(),
+ "Failed to submit 2 changes due to the following problems:\n"
+ + "Change "
+ + change2Result.getChange().getId()
+ + ": Depends on change that was not submitted."
+ + " Commit "
+ + change2Result.getCommit().name()
+ + " depends on commit "
+ + changeResult.getCommit().name()
+ + ", which is outdated patch set "
+ + patchSetId.get()
+ + " of change "
+ + changeResult.getChange().getId()
+ + ". The latest patch set is "
+ + changeResult.getPatchSetId().get()
+ + ".");
+
+ assertRefUpdatedEvents();
+ assertChangeMergedEvents();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
index 5fe741d..995de0d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
@@ -29,7 +29,6 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
-import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.changes.ChangeApi;
@@ -534,48 +533,6 @@
}
@Test
- public void dependencyOnOutdatedPatchSetPreventsMerge() throws Throwable {
- // Create a change
- PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
- PushOneCommit.Result changeResult = change.to("refs/for/master");
- PatchSet.Id patchSetId = changeResult.getPatchSetId();
-
- // Create a successor change.
- PushOneCommit change2 =
- pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
- PushOneCommit.Result change2Result = change2.to("refs/for/master");
-
- // Create new patch set for first change.
- testRepo.reset(changeResult.getCommit().name());
- amendChange(changeResult.getChangeId());
-
- // Approve both changes
- approve(changeResult.getChangeId());
- approve(change2Result.getChangeId());
-
- submitWithConflict(
- change2Result.getChangeId(),
- "Failed to submit 2 changes due to the following problems:\n"
- + "Change "
- + change2Result.getChange().getId()
- + ": Depends on change that was not submitted."
- + " Commit "
- + change2Result.getCommit().name()
- + " depends on commit "
- + changeResult.getCommit().name()
- + ", which is outdated patch set "
- + patchSetId.get()
- + " of change "
- + changeResult.getChange().getId()
- + ". The latest patch set is "
- + changeResult.getPatchSetId().get()
- + ".");
-
- assertRefUpdatedEvents();
- assertChangeMergedEvents();
- }
-
- @Test
public void dependencyOnDeletedChangePreventsMerge() throws Throwable {
// Create a change
PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
