From ae4768654e3510652e25517f735ee3c2a35c4540 Mon Sep 17 00:00:00 2001 From: Zalan Blenessy Date: Fri, 13 Feb 2015 14:06:57 +0100 Subject: [PATCH] Added copyAllScoresIfNoChange Covers the case when nothing has changed except the commit of the rebased patch set. This is possible when a patch set is rebased on target branch. With this patch, when PS1 is rebased (without modification) to PS2 the Verified+1 and Code-Review+2 will be copied since PS1 and PS2 are effectively the same change with different commits. ----- (B) ----- (B+M) ----- (B+M-M) \ \ ----- (PS1) ----- (PS2) Change-Id: I6a8ebd3c83ca6167cfc826267a057a871c752ff6 --- Documentation/config-hooks.txt | 3 ++- Documentation/config-labels.txt | 18 +++++++++++-- Documentation/json.txt | 4 ++- .../google/gerrit/common/data/LabelType.java | 9 +++++++ .../google/gerrit/server/ApprovalCopier.java | 4 ++- .../gerrit/server/change/ChangeKind.java | 7 +++-- .../server/change/ChangeKindCacheImpl.java | 27 +++++++++++++------ .../gerrit/server/git/ProjectConfig.java | 12 +++++++-- .../gerrit/server/git/ReceiveCommits.java | 1 + 9 files changed, 68 insertions(+), 17 deletions(-) diff --git a/Documentation/config-hooks.txt b/Documentation/config-hooks.txt index 96dbff5afa..5666dffff3 100644 --- a/Documentation/config-hooks.txt +++ b/Documentation/config-hooks.txt @@ -50,8 +50,9 @@ kind:: change kind represents the kind of change uploaded, also represented in l TRIVIAL_REBASE;; Conflict-free merge between the new parent and the prior patch set. - NO_CODE_CHANGE;; No code changed; same tree and same parents. + NO_CODE_CHANGE;; No code changed; same tree and same parent tree. + NO_CHANGE;; No changes; same commit message, same tree and same parent tree. === draft-published diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index aaeb834af8..b20cd74131 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -241,19 +241,33 @@ as trivial rebase if the commit message is the same as in the previous patch set and if it has the same code delta as the previous patch set. This is the case if the change was rebased onto a different parent. This can be used to enable sticky approvals, reducing turn-around for -trivial rebases prior to submitting a change. Defaults to false. +trivial rebases prior to submitting a change. +It is recommended to enable this for the Code-Review label. +Defaults to false. [[label_copyAllScoresIfNoCodeChange]] === `label.Label-Name.copyAllScoresIfNoCodeChange` If true, all scores for the label are copied forward when a new patch -set is uploaded that has the same parent commit as the previous patch +set is uploaded that has the same parent tree as the previous patch set and the same code delta as the previous patch set. This means only the commit message is different. This can be used to enable sticky approvals on labels that only depend on the code, reducing turn-around if only the commit message is changed prior to submitting a change. +It is recommended to enable this for the Verified label if enabled. Defaults to false. +[[label_copyAllScoresIfNoChange]] +=== `label.Label-Name.copyAllScoresIfNoChange` + +If true, all scores for the label are copied forward when a new patch +set is uploaded that has the same parent tree, code delta, and commit +message as the previous patch set. This means that only the patch +set SHA1 is different. This can be used to enable sticky +approvals, reducing turn-around for this special case. +It is recommended to leave this enabled for both Verified and +Code-Review labels. Defaults to true. + [[label_canOverride]] === `label.Label-Name.canOverride` diff --git a/Documentation/json.txt b/Documentation/json.txt index af942a2af7..feef1a1f74 100644 --- a/Documentation/json.txt +++ b/Documentation/json.txt @@ -119,7 +119,9 @@ kind:: Kind of change uploaded. TRIVIAL_REBASE;; Conflict-free merge between the new parent and the prior patch set. - NO_CODE_CHANGE;; No code changed; same tree and same parents. + NO_CODE_CHANGE;; No code changed; same tree and same parent tree. + + NO_CHANGE;; No changes; same commit message, same tree and same parent tree. approvals:: The <> granted. diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java index 8f921d080a..315be845bd 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java @@ -94,6 +94,7 @@ public class LabelType { protected boolean copyMaxScore; protected boolean copyAllScoresOnTrivialRebase; protected boolean copyAllScoresIfNoCodeChange; + protected boolean copyAllScoresIfNoChange; protected short defaultValue; protected List values; @@ -219,6 +220,14 @@ public class LabelType { this.copyAllScoresIfNoCodeChange = copyAllScoresIfNoCodeChange; } + public boolean isCopyAllScoresIfNoChange() { + return copyAllScoresIfNoChange; + } + + public void setCopyAllScoresIfNoChange(boolean copyAllScoresIfNoChange) { + this.copyAllScoresIfNoChange = copyAllScoresIfNoChange; + } + public boolean isMaxNegative(PatchSetApproval ca) { return maxNegative == ca.getValue(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java index 81ffe1c200..2a477c2d7c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java @@ -16,6 +16,7 @@ package com.google.gerrit.server; import static com.google.common.base.Preconditions.checkArgument; import static com.google.gerrit.server.change.ChangeKind.NO_CODE_CHANGE; +import static com.google.gerrit.server.change.ChangeKind.NO_CHANGE; import static com.google.gerrit.server.change.ChangeKind.TRIVIAL_REBASE; import com.google.common.collect.HashBasedTable; @@ -164,7 +165,8 @@ public class ApprovalCopier { return true; } return (type.isCopyAllScoresOnTrivialRebase() && kind == TRIVIAL_REBASE) - || (type.isCopyAllScoresIfNoCodeChange() && kind == NO_CODE_CHANGE); + || (type.isCopyAllScoresIfNoCodeChange() && kind == NO_CODE_CHANGE) + || (type.isCopyAllScoresIfNoChange() && kind == NO_CHANGE); } private static PatchSetApproval copy(PatchSetApproval src, PatchSet.Id psId) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java index c6e088f8ea..d22d6ff344 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java @@ -22,6 +22,9 @@ public enum ChangeKind { /** Conflict-free merge between the new parent and the prior patch set. */ TRIVIAL_REBASE, - /** Same tree and same parents. */ - NO_CODE_CHANGE; + /** Same tree and same parent tree. */ + NO_CODE_CHANGE, + + /** Same tree, parent tree, same commit message. */ + NO_CHANGE; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java index a5e7d12f17..76cf49cb72 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java @@ -197,23 +197,22 @@ public class ChangeKindCacheImpl implements ChangeKindCache { walk.parseBody(next); if (!next.getFullMessage().equals(prior.getFullMessage())) { - if (next.getTree() == prior.getTree() && isSameParents(prior, next)) { + if (isSameDeltaAndTree(prior, next)) { return ChangeKind.NO_CODE_CHANGE; } else { return ChangeKind.REWORK; } } + if (isSameDeltaAndTree(prior, next)) { + return ChangeKind.NO_CHANGE; + } + if (prior.getParentCount() != 1 || next.getParentCount() != 1) { // Trivial rebases done by machine only work well on 1 parent. return ChangeKind.REWORK; } - if (next.getTree() == prior.getTree() && - isSameParents(prior, next)) { - return ChangeKind.TRIVIAL_REBASE; - } - // A trivial rebase can be detected by looking for the next commit // having the same tree as would exist when the prior commit is // cherry-picked onto the next commit's new first parent. @@ -232,13 +231,25 @@ public class ChangeKindCacheImpl implements ChangeKindCache { } } - private static boolean isSameParents(RevCommit prior, RevCommit next) { + private static boolean isSameDeltaAndTree(RevCommit prior, RevCommit next) { + if (next.getTree() != prior.getTree()) { + return false; + } + if (prior.getParentCount() != next.getParentCount()) { return false; } else if (prior.getParentCount() == 0) { return true; } - return prior.getParent(0).equals(next.getParent(0)); + + // Make sure that the prior/next delta is the same - not just the tree. + // This is done by making sure that the parent trees are equal. + for (int i = 0; i < prior.getParentCount(); i++) { + if (next.getParent(i).getTree() != prior.getParent(i).getTree()) { + return false; + } + } + return true; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index e23916d51e..58b41cd5eb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java @@ -134,7 +134,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. private static final String KEY_COPY_MIN_SCORE = "copyMinScore"; private static final String KEY_COPY_MAX_SCORE = "copyMaxScore"; private static final String KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE = "copyAllScoresOnTrivialRebase"; - private static final String KEY_COPY_ALL_SCORES_IF_NO_CHANGE = "copyAllScoresIfNoCodeChange"; + private static final String KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE = "copyAllScoresIfNoCodeChange"; + private static final String KEY_COPY_ALL_SCORES_IF_NO_CHANGE = "copyAllScoresIfNoChange"; private static final String KEY_VALUE = "value"; private static final String KEY_CAN_OVERRIDE = "canOverride"; private static final String KEY_Branch = "branch"; @@ -696,7 +697,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. label.setCopyAllScoresOnTrivialRebase( rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, false)); label.setCopyAllScoresIfNoCodeChange( - rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, false)); + rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE, false)); + label.setCopyAllScoresIfNoChange( + rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, true)); label.setCanOverride( rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, true)); label.setRefPatterns(getStringListOrNull(rc, LABEL, name, KEY_Branch)); @@ -1047,6 +1050,11 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE); } if (label.isCopyAllScoresIfNoCodeChange()) { + rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE, true); + } else { + rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE); + } + if (label.isCopyAllScoresIfNoChange()) { rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, true); } else { rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 99d0542ab5..3465a3d013 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -2072,6 +2072,7 @@ public class ReceiveCommits { String message = "Uploaded patch set " + newPatchSet.getPatchSetId(); switch (changeKind) { case TRIVIAL_REBASE: + case NO_CHANGE: message += ": Patch Set " + priorPatchSet.get() + " was rebased"; break; case NO_CODE_CHANGE: