Fix bug: cherry-picks were not "related"
Currently, when creating a cherry-pick and specifying a base,
if the base is an open change, we should show the created
cherry-pick and the base as related changes. However, this doesn't
happen.
This change ensures that those changes will have the same "groups"
thus the GetRelated endpoint will identify those changes as related.
Also removed legacy workaround for RevertSubmission that allowed
those changes to be related.
Change-Id: Id712918b2fc8e25ea997a668c1137fd59fd687b6
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index 0ba4905..44dc6e1 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -19,7 +19,6 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.Nullable;
@@ -47,6 +46,7 @@
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
@@ -173,7 +173,6 @@
TimeUtil.nowTs(),
null,
null,
- null,
null);
}
@@ -205,7 +204,7 @@
throws IOException, InvalidChangeOperationException, UpdateException, RestApiException,
ConfigInvalidException, NoSuchProjectException {
return cherryPick(
- sourceChange, project, sourceCommit, input, dest, TimeUtil.nowTs(), null, null, null, null);
+ sourceChange, project, sourceCommit, input, dest, TimeUtil.nowTs(), null, null, null);
}
/**
@@ -227,8 +226,6 @@
* @param idForNewChange The ID that the new change of the cherry pick will have. If provided and
* the cherry-pick doesn't result in creating a new change, then
* InvalidChangeOperationException is thrown.
- * @param groupName The name of the group for grouping related changes (used by GetRelated
- * endpoint).
* @return Result object that describes the cherry pick.
* @throws IOException Unable to open repository or read from the database.
* @throws InvalidChangeOperationException Parent or branch don't exist, or two changes with same
@@ -248,8 +245,7 @@
Timestamp timestamp,
@Nullable Change.Id revertedChange,
@Nullable ObjectId changeIdForNewChange,
- @Nullable Change.Id idForNewChange,
- @Nullable String groupName)
+ @Nullable Change.Id idForNewChange)
throws IOException, InvalidChangeOperationException, UpdateException, RestApiException,
ConfigInvalidException, NoSuchProjectException {
@@ -379,12 +375,12 @@
cherryPickCommit,
dest.branch(),
newTopic,
+ project,
sourceChange,
sourceCommit,
input,
revertedChange,
- idForNewChange,
- groupName);
+ idForNewChange);
}
bu.execute();
return Result.create(changeId, cherryPickCommit.getFilesWithGitConflicts());
@@ -473,13 +469,13 @@
CodeReviewCommit cherryPickCommit,
String refName,
String topic,
+ Project.NameKey project,
@Nullable Change sourceChange,
@Nullable ObjectId sourceCommit,
CherryPickInput input,
@Nullable Change.Id revertOf,
- @Nullable Change.Id idForNewChange,
- @Nullable String groupName)
- throws IOException {
+ @Nullable Change.Id idForNewChange)
+ throws IOException, InvalidChangeOperationException {
Change.Id changeId = idForNewChange != null ? idForNewChange : Change.id(seq.nextChangeId());
ChangeInserter ins = changeInserterFactory.create(changeId, cherryPickCommit, refName);
ins.setRevertOf(revertOf);
@@ -506,8 +502,23 @@
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.get().getAccountId());
ins.setReviewersAndCcs(reviewers, ccs);
- if (groupName != null) {
- ins.setGroups(ImmutableList.of(groupName));
+ }
+ // If there is a base, and the base is not merged, the groups will be overridden by the base's
+ // groups.
+ ins.setGroups(GroupCollector.getDefaultGroups(cherryPickCommit.getId()));
+ if (input.base != null) {
+ List<ChangeData> changes =
+ queryProvider.get().setLimit(2).byBranchCommitOpen(project.get(), refName, input.base);
+ if (changes.size() > 1) {
+ throw new InvalidChangeOperationException(
+ "Several changes with key "
+ + input.base
+ + " reside on the same branch. "
+ + "Cannot cherry-pick on target branch.");
+ }
+ if (changes.size() == 1) {
+ Change change = changes.get(0).change();
+ ins.setGroups(changeNotesFactory.createChecked(change).getCurrentPatchSet().groups());
}
}
bu.insertChange(ins);
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index 7a04f05..846b80c 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -286,7 +286,6 @@
throws IOException, RestApiException, UpdateException, ConfigInvalidException,
PermissionBackendException {
- String groupName = null;
String initialMessage = revertInput.message;
while (sortedChangesInProjectAndBranch.hasNext()) {
ChangeNotes changeNotes = sortedChangesInProjectAndBranch.next().data().notes();
@@ -300,13 +299,8 @@
// This is the code in case this is the first revert of this project + branch, and the
// revert would be on top of the change being reverted.
craeteNormalRevert(revertInput, changeNotes, timestamp);
- groupName = cherryPickInput.base;
} else {
- // This is the code in case this is the second revert (or more) of this project + branch.
- if (groupName == null) {
- groupName = cherryPickInput.base;
- }
- createCherryPickedRevert(revertInput, project, groupName, changeNotes, timestamp);
+ createCherryPickedRevert(revertInput, project, changeNotes, timestamp);
}
}
}
@@ -314,7 +308,6 @@
private void createCherryPickedRevert(
RevertInput revertInput,
Project.NameKey project,
- String groupName,
ChangeNotes changeNotes,
Timestamp timestamp)
throws IOException, ConfigInvalidException, UpdateException, RestApiException {
@@ -333,7 +326,7 @@
bu.addOp(
changeNotes.getChange().getId(),
new CreateCherryPickOp(
- revCommitId, generatedChangeId, cherryPickRevertChangeId, groupName, timestamp));
+ revCommitId, generatedChangeId, cherryPickRevertChangeId, timestamp));
bu.addOp(changeNotes.getChange().getId(), new PostRevertedMessageOp(generatedChangeId));
bu.addOp(
cherryPickRevertChangeId,
@@ -547,19 +540,16 @@
private final ObjectId revCommitId;
private final ObjectId computedChangeId;
private final Change.Id cherryPickRevertChangeId;
- private final String groupName;
private final Timestamp timestamp;
CreateCherryPickOp(
ObjectId revCommitId,
ObjectId computedChangeId,
Change.Id cherryPickRevertChangeId,
- String groupName,
Timestamp timestamp) {
this.revCommitId = revCommitId;
this.computedChangeId = computedChangeId;
this.cherryPickRevertChangeId = cherryPickRevertChangeId;
- this.groupName = groupName;
this.timestamp = timestamp;
}
@@ -577,8 +567,7 @@
timestamp,
change.getId(),
computedChangeId,
- cherryPickRevertChangeId,
- groupName);
+ cherryPickRevertChangeId);
// save the commit as base for next cherryPick of that branch
cherryPickInput.base =
changeNotesFactory
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 17391de..74f9134 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -1097,6 +1097,57 @@
}
@Test
+ public void getRelatedCherryPicks() throws Exception {
+ PushOneCommit.Result r1 = createChange(SUBJECT, "a.txt", "a");
+ PushOneCommit.Result r2 = createChange(SUBJECT, "b.txt", "b");
+
+ String branch = "foo";
+ // Create target branch to cherry-pick to.
+ gApi.projects().name(project.get()).branch(branch).create(new BranchInput());
+
+ CherryPickInput input = new CherryPickInput();
+ input.message = "message";
+ input.destination = branch;
+ ChangeInfo firstCherryPickResult =
+ gApi.changes().id(r1.getChangeId()).current().cherryPickAsInfo(input);
+
+ input.base = gApi.changes().id(firstCherryPickResult.changeId).current().commit(false).commit;
+ ChangeInfo secondCherryPickResult =
+ gApi.changes().id(r2.getChangeId()).current().cherryPickAsInfo(input);
+ assertThat(gApi.changes().id(firstCherryPickResult.changeId).current().related().changes)
+ .hasSize(2);
+ assertThat(gApi.changes().id(secondCherryPickResult.changeId).current().related().changes)
+ .hasSize(2);
+ }
+
+ @Test
+ public void cherryPickOnMergedChangeIsNotRelated() throws Exception {
+ PushOneCommit.Result r1 = createChange(SUBJECT, "a.txt", "a");
+ PushOneCommit.Result r2 = createChange(SUBJECT, "b.txt", "b");
+
+ String branch = "foo";
+ // Create target branch to cherry-pick to.
+ gApi.projects().name(project.get()).branch(branch).create(new BranchInput());
+
+ CherryPickInput input = new CherryPickInput();
+ input.message = "message";
+ input.destination = branch;
+ ChangeInfo firstCherryPickResult =
+ gApi.changes().id(r1.getChangeId()).current().cherryPickAsInfo(input);
+
+ gApi.changes().id(firstCherryPickResult.id).current().review(ReviewInput.approve());
+ gApi.changes().id(firstCherryPickResult.id).current().submit();
+
+ input.base = gApi.changes().id(firstCherryPickResult.changeId).current().commit(false).commit;
+ ChangeInfo secondCherryPickResult =
+ gApi.changes().id(r2.getChangeId()).current().cherryPickAsInfo(input);
+ assertThat(gApi.changes().id(firstCherryPickResult.changeId).current().related().changes)
+ .hasSize(0);
+ assertThat(gApi.changes().id(secondCherryPickResult.changeId).current().related().changes)
+ .hasSize(0);
+ }
+
+ @Test
public void canRebase() throws Exception {
PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
PushOneCommit.Result r1 = push.to("refs/for/master");
