Fix bug about persisting copied votes on submit
This bug is happening because when submitting a change, on some submit
strategies, we create a new patch-set which causes us to persist copied
votes. When persisting copied votes with ListOfFilesUnchanged copy
condition, we need to fetch the commit. This fails for non-existent
patch-sets (we are just now creating it).
Instead of fixing the above, we do the following:
We ensure we don't call this code at all for patch-sets created during
submit. This is unnecessary since during submit we anyway store
approvals (and they don't need to be "copied approvals"). It's best to
make sure we don't copy approvals twice, even though that bug never
surfaced (luck).
Change-Id: Ibc61b390814ec12ed2bdd1b8dd199187c14cbea4
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index f093958..209901d 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -104,6 +104,7 @@
private boolean allowClosed;
private boolean sendEmail = true;
private String topic;
+ private boolean storeCopiedVotes = true;
// Fields set during some phase of BatchUpdate.Op.
private Change change;
@@ -203,6 +204,17 @@
return this;
}
+ /**
+ * We always want to store copied votes except when the change is getting submitted and a new
+ * patch-set is created on submit (using submit strategies such as "REBASE_ALWAYS"). In such
+ * cases, we already store the votes of the new patch-sets in SubmitStrategyOp#saveApprovals. We
+ * should not also store the copied votes.
+ */
+ public PatchSetInserter setStoreCopiedVotes(boolean storeCopiedVotes) {
+ this.storeCopiedVotes = storeCopiedVotes;
+ return this;
+ }
+
public Change getChange() {
checkState(change != null, "getChange() only valid after executing update");
return change;
@@ -288,7 +300,9 @@
// Approvals that are being set in the new patch-set during this operation are not available yet
// outside of the scope of this method. Only copied approvals are set here.
- approvalsUtil.byPatchSet(ctx.getNotes(), patchSet).forEach(a -> update.putCopiedApproval(a));
+ if (storeCopiedVotes) {
+ approvalsUtil.byPatchSet(ctx.getNotes(), patchSet).forEach(a -> update.putCopiedApproval(a));
+ }
return true;
}
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 4952464..3e67cca 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -92,6 +92,7 @@
private boolean detailedCommitMessage;
private boolean postMessage = true;
private boolean sendEmail = true;
+ private boolean storeCopiedVotes = true;
private boolean matchAuthorToCommitterDate = false;
private CodeReviewCommit rebasedCommit;
@@ -169,6 +170,17 @@
return this;
}
+ /**
+ * We always want to store copied votes except when the change is getting submitted and a new
+ * patch-set is created on submit (using submit strategies such as "REBASE_ALWAYS"). In such
+ * cases, we already store the votes of the new patch-sets in SubmitStrategyOp#saveApprovals. We
+ * should not also store the copied votes.
+ */
+ public RebaseChangeOp setStoreCopiedVotes(boolean storeCopiedVotes) {
+ this.storeCopiedVotes = storeCopiedVotes;
+ return this;
+ }
+
public RebaseChangeOp setSendEmail(boolean sendEmail) {
this.sendEmail = sendEmail;
return this;
@@ -219,7 +231,10 @@
.setFireRevisionCreated(fireRevisionCreated)
.setCheckAddPatchSetPermission(checkAddPatchSetPermission)
.setValidate(validate)
- .setSendEmail(sendEmail);
+ .setSendEmail(sendEmail)
+ // The votes are automatically copied and they don't count as copied votes. See
+ // method's javadoc.
+ .setStoreCopiedVotes(storeCopiedVotes);
if (!rebasedCommit.getFilesWithGitConflicts().isEmpty()
&& !notes.getChange().isWorkInProgress()) {
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index cc3b75d..355d25f 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -211,7 +211,10 @@
.setPostMessage(false)
.setSendEmail(false)
.setMatchAuthorToCommitterDate(
- args.project.is(BooleanProjectConfig.MATCH_AUTHOR_TO_COMMITTER_DATE));
+ args.project.is(BooleanProjectConfig.MATCH_AUTHOR_TO_COMMITTER_DATE))
+ // The votes are automatically copied and they don't count as copied votes. See
+ // method's javadoc.
+ .setStoreCopiedVotes(/* storeCopiedVotes = */ false);
try {
rebaseOp.updateRepo(ctx);
} catch (MergeConflictException | NoSuchChangeException e) {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
index fff3cb6..b3592e3 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
@@ -14,9 +14,11 @@
package com.google.gerrit.acceptance.rest.change;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
+import static java.util.Comparator.comparing;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.ExtensionRegistry;
@@ -26,6 +28,10 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project.NameKey;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -403,4 +409,54 @@
assertSubmittable(change2Result.getChangeId());
}
+
+ @Test
+ public void stickyVoteStoredOnSubmitOnNewPatchset_withoutCopyCondition() throws Exception {
+ try (ProjectConfigUpdate u = updateProject(NameKey.parse("All-Projects"))) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+ stickyVoteStoredOnSubmitOnNewPatchset();
+ }
+
+ @Test
+ public void stickyVoteStoredOnSubmitOnNewPatchset_withCopyCondition() throws Exception {
+ // Code-Review will be sticky.
+ try (ProjectConfigUpdate u = updateProject(NameKey.parse("All-Projects"))) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ stickyVoteStoredOnSubmitOnNewPatchset();
+ }
+
+ private void stickyVoteStoredOnSubmitOnNewPatchset() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ // Add a new vote.
+ ReviewInput input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+
+ // Submit, also keeping the Code-Review +2 vote.
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // The last approval is stored on the submitted patch-set which was created by cherry-pick
+ // during submit.
+ PatchSetApproval patchSetApprovals =
+ Iterables.getLast(
+ r.getChange().notes().getApprovalsWithCopied().values().stream()
+ .filter(a -> a.labelId().equals(LabelId.create(LabelId.CODE_REVIEW)))
+ .sorted(comparing(a -> a.patchSetId().get()))
+ .collect(toImmutableList()));
+
+ assertThat(patchSetApprovals.patchSetId().get()).isEqualTo(2);
+ assertThat(patchSetApprovals.label()).isEqualTo(LabelId.CODE_REVIEW);
+ assertThat(patchSetApprovals.value()).isEqualTo((short) 2);
+
+ // The approval is not copied since we don't need to persist copied votes on submit, only
+ // persist votes normally.
+ assertThat(patchSetApprovals.copied()).isFalse();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
index eeeac2a..aa93815 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
@@ -14,20 +14,27 @@
package com.google.gerrit.acceptance.rest.change;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.util.Comparator.comparing;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.PushOneCommit;
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.LabelId;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.exceptions.InternalServerWithUserMessageException;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -159,6 +166,56 @@
}
}
+ @Test
+ public void stickyVoteStoredOnSubmitOnNewPatchset_withoutCopyCondition() throws Exception {
+ try (ProjectConfigUpdate u = updateProject(NameKey.parse("All-Projects"))) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+ stickyVoteStoredOnSubmitOnNewPatchset();
+ }
+
+ @Test
+ public void stickyVoteStoredOnSubmitOnNewPatchset_withCopyCondition() throws Exception {
+ // Code-Review will be sticky.
+ try (ProjectConfigUpdate u = updateProject(NameKey.parse("All-Projects"))) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ stickyVoteStoredOnSubmitOnNewPatchset();
+ }
+
+ private void stickyVoteStoredOnSubmitOnNewPatchset() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ // Add a new vote.
+ ReviewInput input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+
+ // Submit, also keeping the Code-Review +2 vote.
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // The last approval is stored on the submitted patch-set which was created by rebase during
+ // submit.
+ PatchSetApproval patchSetApprovals =
+ Iterables.getLast(
+ r.getChange().notes().getApprovalsWithCopied().values().stream()
+ .filter(a -> a.labelId().equals(LabelId.create(LabelId.CODE_REVIEW)))
+ .sorted(comparing(a -> a.patchSetId().get()))
+ .collect(toImmutableList()));
+
+ assertThat(patchSetApprovals.patchSetId().get()).isEqualTo(2);
+ assertThat(patchSetApprovals.label()).isEqualTo(LabelId.CODE_REVIEW);
+ assertThat(patchSetApprovals.value()).isEqualTo((short) 2);
+
+ // The approval is not copied since we don't need to persist copied votes on submit, only
+ // persist votes normally.
+ assertThat(patchSetApprovals.copied()).isFalse();
+ }
+
private void assertLatestRevisionHasFooters(PushOneCommit.Result change) throws Throwable {
RevCommit c = getCurrentCommit(change);
assertThat(c.getFooterLines(FooterConstants.CHANGE_ID)).isNotEmpty();
