Fix rebasing chain when a checks refs exists for one of the changes
PatchSetUtil#getCurrentRevCommitIncludingPending had the wrong
assumption that a change only has:
* a change meta ref (ref/changes/<sharded-change-id>/meta)
* patch set refs (ref/changes/<sharded-change-id>/<patch-set-num>)
from which it's possible to parse a patch set number
If there are other change refs, e.g. a checks ref
(ref/changes/<sharded-change-id>/checks),
PatchSetUtil#getCurrentRevCommitIncludingPending failed with a
NullPointerException. That's because for all non-meta change refs it
tries to parse a patch set number from the ref, but it didn't expect
that the patch set number can be null if it's not a patch set ref (see
PatchSet.Id#fromRef).
Fix this by filtering out null patch set numbers.
Bug: Google b/276709753
Release-Notes: Fixed rebasing chain when a checks refs exists for one of the changes
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I47287d898112e9e2680c8738040ef1f05e173b8e
diff --git a/java/com/google/gerrit/server/PatchSetUtil.java b/java/com/google/gerrit/server/PatchSetUtil.java
index b34a56f..68d2314 100644
--- a/java/com/google/gerrit/server/PatchSetUtil.java
+++ b/java/com/google/gerrit/server/PatchSetUtil.java
@@ -42,6 +42,7 @@
import java.io.IOException;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
@@ -193,6 +194,7 @@
Optional<PatchSet.Id> latestPendingPatchSet =
refUpdates.keySet().stream()
.map(r -> PatchSet.Id.fromRef(changeId.toRefPrefix() + r))
+ .filter(Objects::nonNull)
.max(PatchSet.Id::compareTo);
if (latestPendingPatchSet.isPresent()) {
return ctx.getRevWalk().parseCommit(refUpdates.get(latestPendingPatchSet.get().getId()));
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
index 4e95032..fd37fb0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
@@ -44,6 +44,7 @@
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.AttentionSetInput;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
@@ -74,7 +75,9 @@
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -517,6 +520,36 @@
}
}
+ @Test
+ public void rebaseChangeWhenChecksRefExists() throws Exception {
+ // Create two changes both with the same parent
+ PushOneCommit.Result r = createChange();
+ testRepo.reset("HEAD~1");
+ PushOneCommit.Result r2 = createChange();
+
+ // Approve and submit the first change
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+ revision.review(ReviewInput.approve());
+ revision.submit();
+
+ // Create checks ref
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(project))) {
+ testRepo.update(
+ RefNames.changeRefPrefix(r2.getChange().getId()) + "checks",
+ testRepo.commit().message("Empty commit"));
+ }
+
+ // Rebase the second change
+ rebaseCall.call(r2.getChangeId());
+
+ verifyRebaseForChange(
+ r2.getChange().getId(),
+ r.getCommit().name(),
+ /* shouldHaveApproval= */ false,
+ /* expectedNumRevisions= */ 2);
+ }
+
protected void verifyRebaseForChange(
Change.Id changeId, Change.Id baseChangeId, boolean shouldHaveApproval)
throws RestApiException {
@@ -1233,6 +1266,39 @@
}
@Test
+ public void rebaseChainWhenChecksRefExists() throws Exception {
+ // Create changes with the following hierarchy:
+ // * HEAD
+ // * r1
+ // * r2
+ // * r3
+ PushOneCommit.Result r = createChange();
+ testRepo.reset("HEAD~1");
+ PushOneCommit.Result r2 = createChange();
+ PushOneCommit.Result r3 = createChange();
+
+ // Create checks ref
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(project))) {
+ testRepo.update(
+ RefNames.changeRefPrefix(r2.getChange().getId()) + "checks",
+ testRepo.commit().message("Empty commit"));
+ }
+
+ // Approve and submit the first change
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+ revision.review(ReviewInput.approve());
+ revision.submit();
+
+ // Add an approval whose score should be copied on trivial rebase
+ gApi.changes().id(r2.getChangeId()).current().review(ReviewInput.recommend());
+ gApi.changes().id(r3.getChangeId()).current().review(ReviewInput.recommend());
+
+ // Rebase the chain through r3.
+ verifyRebaseChainResponse(gApi.changes().id(r3.getChangeId()).rebaseChain(), false, r2, r3);
+ }
+
+ @Test
public void testCountRebasesMetric() throws Exception {
// Create changes with the following hierarchy:
// * HEAD
