opendev/gerrit
Code Issues Proposed changes

Added copyAllScoresIfNoChange

Browse Source 
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
This commit is contained in:
Zalan Blenessy 2015-02-13 14:06:57 +01:00 committed by David Pursehouse
parent f62a233cc4
commit ae4768654e
9 changed files with 68 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
Documentation/config-hooks.txt
View File

@ -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

18
Documentation/config-labels.txt
View File

@ -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`

4
Documentation/json.txt
View File

@ -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 <<approval,approval attribute>> granted.

9
gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
View File

@ -94,6 +94,7 @@ public class LabelType {
protected boolean copyMaxScore;
protected boolean copyAllScoresOnTrivialRebase;
protected boolean copyAllScoresIfNoCodeChange;
protected boolean copyAllScoresIfNoChange;
protected short defaultValue;
protected List<LabelValue> 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();
}

4
gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
View File

@ -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) {

7
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java
View File

@ -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;
}

27
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
View File

@ -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;
}
}

12
gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
View File

@ -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);

1
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -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: