Fix Revert Submission related changes The endpoint "Get Related Changes" searches for related changes based on groups predicate, rather than by relation. We need to insert the same group for related changes when cherry-picking one change on top of another. This change unfortunately doesn't actually solve the full problem of cherry-picks, since it's not always straightforward to infer the group of the parent when performing a cherry-pick. However, this change does fix the problem for Revert Submission: We ensure here that all changes that are cherry-picked in the same project+ branch will have the same group. Change-Id: Iabd8eaae7840aaa810822f0418ee8ad0fa22506b
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java index 3820877..a7a7998 100644 --- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java +++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -18,6 +18,7 @@ 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; @@ -172,6 +173,7 @@ null, null, null, + null, null); } @@ -216,6 +218,7 @@ null, null, null, + null, null); } @@ -241,6 +244,8 @@ * @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 @@ -263,7 +268,8 @@ @Nullable String topic, @Nullable Change.Id revertedChange, @Nullable ObjectId changeIdForNewChange, - @Nullable Change.Id idForNewChange) + @Nullable Change.Id idForNewChange, + @Nullable String groupName) throws IOException, InvalidChangeOperationException, IntegrationException, UpdateException, RestApiException, ConfigInvalidException, NoSuchProjectException { @@ -388,7 +394,8 @@ sourceCommit, input, revertedChange, - idForNewChange); + idForNewChange, + groupName); } bu.execute(); return Result.create(changeId, cherryPickCommit.getFilesWithGitConflicts()); @@ -469,7 +476,8 @@ @Nullable ObjectId sourceCommit, CherryPickInput input, @Nullable Change.Id revertOf, - @Nullable Change.Id idForNewChange) + @Nullable Change.Id idForNewChange, + @Nullable String groupName) throws IOException { Change.Id changeId = idForNewChange != null ? idForNewChange : Change.id(seq.nextChangeId()); ChangeInserter ins = changeInserterFactory.create(changeId, cherryPickCommit, refName); @@ -495,6 +503,9 @@ 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)); + } } bu.insertChange(ins); return changeId;
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java index b75a5e5..e5e01fe 100644 --- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java +++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -230,6 +230,7 @@ cherryPickInput.keepReviewers = true; for (BranchNameKey projectAndBranch : changesPerProjectAndBranch.keySet()) { + String groupName = null; Project.NameKey project = projectAndBranch.project(); cherryPickInput.destination = projectAndBranch.branch(); Collection<ChangeData> changesInProjectAndBranch = @@ -249,7 +250,6 @@ if (cherryPickInput.base == null) { cherryPickInput.base = getBase(changeNotes, commitIdsInProjectAndBranch).name(); } - // 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. if (cherryPickInput.base.equals(changeNotes.getCurrentPatchSet().commitId().getName())) { @@ -264,6 +264,7 @@ .getCurrentPatchSet() .commitId() .getName(); + groupName = cherryPickInput.base; } else { // This is the code in case this is the second revert (or more) of this project + branch. ObjectId revCommitId = @@ -279,6 +280,9 @@ changeNotes.getCurrentPatchSet().commitId().name()); ObjectId generatedChangeId = Change.generateChangeId(); Change.Id cherryPickRevertChangeId = Change.id(seq.nextChangeId()); + if (groupName == null) { + groupName = cherryPickInput.base; + } // TODO (paiking): In the the future, the timestamp should be the same for all the revert // changes. try (BatchUpdate bu = updateFactory.create(project, user.get(), TimeUtil.nowTs())) { @@ -289,7 +293,11 @@ bu.addOp( changeNotes.getChange().getId(), new CreateCherryPickOp( - revCommitId, revertInput.topic, generatedChangeId, cherryPickRevertChangeId)); + revCommitId, + revertInput.topic, + generatedChangeId, + cherryPickRevertChangeId, + groupName)); bu.addOp(changeNotes.getChange().getId(), new PostRevertedMessageOp(generatedChangeId)); bu.addOp( cherryPickRevertChangeId, @@ -479,16 +487,19 @@ private final String topic; private final ObjectId computedChangeId; private final Change.Id cherryPickRevertChangeId; + private final String groupName; CreateCherryPickOp( ObjectId revCommitId, String topic, ObjectId computedChangeId, - Change.Id cherryPickRevertChangeId) { + Change.Id cherryPickRevertChangeId, + String groupName) { this.revCommitId = revCommitId; this.topic = topic; this.computedChangeId = computedChangeId; this.cherryPickRevertChangeId = cherryPickRevertChangeId; + this.groupName = groupName; } @Override @@ -507,7 +518,8 @@ topic, change.getId(), computedChangeId, - cherryPickRevertChangeId); + cherryPickRevertChangeId, + groupName); // save the commit as base for next cherryPick of that branch cherryPickInput.base = changeNotesFactory
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java index ab30bef..3087988 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -756,6 +756,8 @@ assertThat(revertChanges.get(i).get().topic) .startsWith("revert-" + resultCommits.get(0).getChange().change().getSubmissionId()); } + + assertThat(gApi.changes().id(revertChanges.get(1).id()).current().related().changes).hasSize(2); } @Test @@ -792,6 +794,7 @@ assertThat(revertChanges.get(1).current().files().get("a.txt").linesDeleted).isEqualTo(1); assertThat(revertChanges).hasSize(2); + assertThat(gApi.changes().id(revertChanges.get(0).id()).current().related().changes).hasSize(2); } @Test @@ -820,6 +823,7 @@ assertThat(revertChanges.get(1).current().files().get("a.txt").linesDeleted).isEqualTo(1); assertThat(revertChanges).hasSize(2); + assertThat(gApi.changes().id(revertChanges.get(0).id()).current().related().changes).hasSize(2); } @Test @@ -892,6 +896,7 @@ .isEqualTo(sha1SecondRevert); assertThat(revertChanges).hasSize(3); + assertThat(gApi.changes().id(revertChanges.get(1).id()).current().related().changes).hasSize(2); } @Test @@ -940,6 +945,7 @@ .isEqualTo(sha1SecondRevert); assertThat(revertChanges).hasSize(3); + assertThat(gApi.changes().id(revertChanges.get(1).id()).current().related().changes).hasSize(3); } @Test @@ -989,6 +995,7 @@ .isEqualTo(sha1SecondRevert); assertThat(revertChanges).hasSize(3); + assertThat(gApi.changes().id(revertChanges.get(1).id()).current().related().changes).hasSize(3); } @Override