Do not match NO_CHANGE for non-merge if changekind:MERGE_FIRST_PARENT_UPDATE
It's unexpected that setting changekind:MERGE_FIRST_PARENT_UPDATE as
copy condition or copyAllScoresOnMergeFirstParentUpdate=true has any
effect on changes for non merge commits (e.g. the documentation for
copyAllScoresOnMergeFirstParentUpdate says "It only applies if the patch
set is a merge commit."). Since changekind:MERGE_FIRST_PARENT_UPDATE and
copyAllScoresOnMergeFirstParentUpdate=true implicitly match approvals if
the change kind was NO_CHANGE this setting unintentionally caused
copying of approvals on non-merge changes if the change kind was
NO_CHANGE.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ida02e8870a77205c91a615d8ece7fe484949fbaf
Release-Notes: skip
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index 037e1ba..db0308f 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -303,7 +303,8 @@
`NO_CHANGE` is more trivial than a trivial rebase, no code change and
a first parent update, hence this change kind is also matched by
`changekind:TRIVIAL_REBASE`, `changekind:NO_CODE_CHANGE` and
-`changekind:MERGE_FIRST_PARENT_UPDATE`.
+`changekind:MERGE_FIRST_PARENT_UPDATE` (only if the change is for a
+merge commit).
==== is:{MIN,MAX,ANY}
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java
index fba2fcb..c3c066f 100644
--- a/java/com/google/gerrit/server/approval/ApprovalInference.java
+++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -28,6 +28,7 @@
import com.google.gerrit.entities.LabelTypes;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.index.query.QueryParseException;
@@ -50,6 +51,7 @@
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.io.IOException;
import java.util.Collection;
import java.util.Map;
import java.util.Optional;
@@ -125,6 +127,7 @@
PatchSetApproval psa,
PatchSet.Id psId,
ChangeKind kind,
+ boolean isMerge,
LabelType type,
@Nullable Map<String, ModifiedFile> baseVsCurrentDiff,
@Nullable Map<String, ModifiedFile> baseVsPriorDiff,
@@ -272,7 +275,7 @@
project.getName());
return true;
}
- if (type.isCopyAllScoresOnMergeFirstParentUpdate()) {
+ if (isMerge && type.isCopyAllScoresOnMergeFirstParentUpdate()) {
logger.atFine().log(
"approval %d on label %s of patch set %d of change %d can be copied"
+ " to patch set %d because change kind is %s and the label has set"
@@ -317,13 +320,15 @@
PatchSet patchSet,
LabelType type,
ChangeKind changeKind,
+ boolean isMerge,
RevWalk revWalk,
Config repoConfig) {
if (!type.getCopyCondition().isPresent()) {
return false;
}
ApprovalContext ctx =
- ApprovalContext.create(changeNotes, psa, patchSet, changeKind, revWalk, repoConfig);
+ ApprovalContext.create(
+ changeNotes, psa, patchSet, changeKind, isMerge, revWalk, repoConfig);
try {
// Use a request context to run checks as an internal user with expanded visibility. This is
// so that the output of the copy condition does not depend on who is running the current
@@ -377,6 +382,7 @@
repoConfig,
priorPatchSet.getValue().commitId(),
patchSet.commitId());
+ boolean isMerge = isMerge(project.getNameKey(), rw, patchSet);
logger.atFine().log(
"change kind for patch set %d of change %d against prior patch set %s is %s",
patchSet.id().get(),
@@ -420,12 +426,13 @@
psa,
patchSet.id(),
changeKind,
+ isMerge,
type.get(),
baseVsCurrent,
baseVsPrior,
priorVsCurrent)
&& !canCopyBasedOnCopyCondition(
- notes, psa, patchSet, type.get(), changeKind, rw, repoConfig)) {
+ notes, psa, patchSet, type.get(), changeKind, isMerge, rw, repoConfig)) {
continue;
}
resultByUser.put(psa.label(), psa.accountId(), psa.copyWithPatchSet(patchSet.id()));
@@ -433,6 +440,18 @@
return resultByUser.values();
}
+ private boolean isMerge(Project.NameKey project, RevWalk rw, PatchSet patchSet) {
+ try {
+ return rw.parseCommit(patchSet.commitId()).getParentCount() > 1;
+ } catch (IOException e) {
+ throw new StorageException(
+ String.format(
+ "failed to check if patch set %d of change %s in project %s is a merge commit",
+ patchSet.id().get(), patchSet.id().changeId(), project),
+ e);
+ }
+ }
+
/**
* Gets the modified files between the two latest patch-sets. Can be used to compute difference in
* files between those two patch-sets .
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalContext.java b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
index 2839eb7..6e433c5 100644
--- a/java/com/google/gerrit/server/query/approval/ApprovalContext.java
+++ b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
@@ -43,6 +43,9 @@
/** {@link ChangeKind} of the delta between the origin and target patch set. */
public abstract ChangeKind changeKind();
+ /** Whether the new patch set is a merge commit. */
+ public abstract boolean isMerge();
+
/** {@link RevWalk} of the repository for the current commit. */
public abstract RevWalk revWalk();
@@ -54,6 +57,7 @@
PatchSetApproval psa,
PatchSet patchSet,
ChangeKind changeKind,
+ boolean isMerge,
RevWalk revWalk,
Config repoConfig) {
checkState(
@@ -68,6 +72,6 @@
// are no changes with gaps in patch set numbers. Since it's planned to fix-up old changes with
// gaps in patch set numbers, this todo is a reminder to add back the check once this is done.
return new AutoValue_ApprovalContext(
- psa, patchSet, changeNotes, changeKind, revWalk, repoConfig);
+ psa, patchSet, changeNotes, changeKind, isMerge, revWalk, repoConfig);
}
}
diff --git a/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java b/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java
index 370f46c..6866983 100644
--- a/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java
+++ b/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java
@@ -42,8 +42,8 @@
// configured change kind (changeKind) is:
// * TRIVIAL_REBASE: since NO_CHANGE is a special kind of a trivial rebase
// * NO_CODE_CHANGE: if there is no change, there is also no code change
- // * MERGE_FIRST_PARENT_UPDATE: if votes should be copied on first parent update, they should
- // also be copied if there was no change
+ // * MERGE_FIRST_PARENT_UPDATE (only if the new patch set is a merge commit): if votes should be
+ // copied on first parent update, they should also be copied if there was no change
//
// Motivation:
// * https://gerrit-review.googlesource.com/c/gerrit/+/74690
@@ -58,7 +58,7 @@
return ctx.changeKind() == ChangeKind.NO_CHANGE
&& (changeKind == ChangeKind.TRIVIAL_REBASE
|| changeKind == ChangeKind.NO_CODE_CHANGE
- || changeKind == ChangeKind.MERGE_FIRST_PARENT_UPDATE);
+ || (ctx.isMerge() && changeKind == ChangeKind.MERGE_FIRST_PARENT_UPDATE));
}
@Override
diff --git a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
index 6a9dd37..0bf5518 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
@@ -359,6 +359,37 @@
}
@Test
+ public void
+ notStickyOnNoChangeForNonMergeIfCopyingIsConfiguredForMergeFirstParentUpdate_withoutCopyCondition()
+ throws Exception {
+ updateCodeReviewLabel(b -> b.setCopyAllScoresOnMergeFirstParentUpdate(true));
+ testNotStickyOnNoChangeForNonMergeIfCopyingIsConfiguredForMergeFirstParentUpdate();
+ }
+
+ @Test
+ public void
+ notStickyOnNoChangeForNonMergeIfCopyingIsConfiguredForMergeFirstParentUpdate_withCopyCondition()
+ throws Exception {
+ updateCodeReviewLabel(
+ b -> b.setCopyCondition("changekind:" + MERGE_FIRST_PARENT_UPDATE.name()));
+ testNotStickyOnNoChangeForNonMergeIfCopyingIsConfiguredForMergeFirstParentUpdate();
+ }
+
+ private void testNotStickyOnNoChangeForNonMergeIfCopyingIsConfiguredForMergeFirstParentUpdate()
+ throws Exception {
+ // Create a change with a non-merge commit
+ String changeId = changeKindCreator.createChange(REWORK, testRepo, admin);
+ vote(admin, changeId, 2, 1);
+ vote(user, changeId, -2, -1);
+
+ // Make a NO_CHANGE update (expect that votes are not copied since it's not a merge change).
+ changeKindCreator.updateChange(changeId, NO_CHANGE, testRepo, admin, project);
+ ChangeInfo c = detailedChange(changeId);
+ assertVotes(c, admin, 0, 0, NO_CHANGE);
+ assertVotes(c, user, -0, 0, NO_CHANGE);
+ }
+
+ @Test
public void notStickyWithCopyOnNoChangeWhenSecondParentIsUpdated() throws Exception {
updateCodeReviewLabel(b -> b.setCopyAllScoresIfNoChange(true));
diff --git a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
index 44b59bf..b019354 100644
--- a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
@@ -314,6 +314,7 @@
approval,
changeNotes.getPatchSets().get(newPsId),
changeKind,
+ /* isMerge= */ false,
rw,
repo.getConfig());
}
