diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index 0e5c27a7df..c08d48462f 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -248,6 +248,18 @@ 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. +[[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 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. +Defaults to false. + [[label_canOverride]] `label.Label-Name.canOverride` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 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 83d3514058..7fd8864247 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 @@ -98,6 +98,7 @@ public class LabelType { protected boolean copyMinScore; protected boolean copyMaxScore; protected boolean copyAllScoresOnTrivialRebase; + protected boolean copyAllScoresIfNoCodeChange; protected List values; protected short maxNegative; @@ -214,6 +215,14 @@ public class LabelType { this.copyAllScoresOnTrivialRebase = copyAllScoresOnTrivialRebase; } + public boolean isCopyAllScoresIfNoCodeChange() { + return copyAllScoresIfNoCodeChange; + } + + public void setCopyAllScoresIfNoCodeChange(boolean copyAllScoresIfNoCodeChange) { + this.copyAllScoresIfNoCodeChange = copyAllScoresIfNoCodeChange; + } + public boolean isMaxNegative(PatchSetApproval ca) { return maxNegative == ca.getValue(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java index 7ec26b02cb..da2f98f6a5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java @@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId; import com.google.gerrit.reviewdb.client.PatchSetInfo; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.change.PatchSetInserter.ChangeKind; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -69,10 +70,10 @@ public class ApprovalsUtil { * @throws OrmException */ public static void copyLabels(ReviewDb db, LabelTypes labelTypes, - PatchSet.Id source, PatchSet dest, boolean trivialRebase) throws OrmException { + PatchSet.Id source, PatchSet dest, ChangeKind changeKind) throws OrmException { Iterable sourceApprovals = db.patchSetApprovals().byPatchSet(source); - copyLabels(db, labelTypes, sourceApprovals, source, dest, trivialRebase); + copyLabels(db, labelTypes, sourceApprovals, source, dest, changeKind); } /** @@ -82,7 +83,7 @@ public class ApprovalsUtil { */ public static void copyLabels(ReviewDb db, LabelTypes labelTypes, Iterable sourceApprovals, PatchSet.Id source, - PatchSet dest, boolean trivialRebase) throws OrmException { + PatchSet dest, ChangeKind changeKind) throws OrmException { List copied = Lists.newArrayList(); for (PatchSetApproval a : sourceApprovals) { if (source.equals(a.getPatchSetId())) { @@ -93,7 +94,11 @@ public class ApprovalsUtil { copied.add(new PatchSetApproval(dest.getId(), a)); } else if (type.isCopyMaxScore() && type.isMaxPositive(a)) { copied.add(new PatchSetApproval(dest.getId(), a)); - } else if (type.isCopyAllScoresOnTrivialRebase() && trivialRebase) { + } else if (type.isCopyAllScoresOnTrivialRebase() + && ChangeKind.TRIVIAL_REBASE.equals(changeKind)) { + copied.add(new PatchSetApproval(dest.getId(), a)); + } else if (type.isCopyAllScoresIfNoCodeChange() + && ChangeKind.NO_CODE_CHANGE.equals(changeKind)) { copied.add(new PatchSetApproval(dest.getId(), a)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java index 2df016fb91..3d14babb34 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java @@ -25,7 +25,6 @@ import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.PatchSetInfo; -import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; @@ -53,7 +52,6 @@ import com.google.inject.assistedinject.Assisted; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ThreeWayMerger; import org.eclipse.jgit.revwalk.FooterLine; import org.eclipse.jgit.revwalk.RevCommit; @@ -86,6 +84,10 @@ public class PatchSetInserter { GERRIT, RECEIVE_COMMITS, NONE; } + public static enum ChangeKind { + REWORK, TRIVIAL_REBASE, NO_CODE_CHANGE; + } + private final ChangeHooks hooks; private final TrackingFooters trackingFooters; private final PatchSetInfoFactory patchSetInfoFactory; @@ -278,11 +280,11 @@ public class PatchSetInserter { RevCommit priorCommit = revWalk.parseCommit(priorCommitId); ProjectState projectState = refControl.getProjectControl().getProjectState(); - boolean trivialRebase = - isTrivialRebase(mergeUtilFactory, projectState, git, priorCommit, commit); + ChangeKind changeKind = + getChangeKind(mergeUtilFactory, projectState, git, priorCommit, commit); ApprovalsUtil.copyLabels(db, refControl.getProjectControl() - .getLabelTypes(), currentPatchSetId, patchSet, trivialRebase); + .getLabelTypes(), currentPatchSetId, patchSet, changeKind); } final List footerLines = commit.getFooterLines(); @@ -362,20 +364,25 @@ public class PatchSetInserter { } } - public static boolean isTrivialRebase(MergeUtil.Factory mergeUtilFactory, ProjectState project, + public static ChangeKind getChangeKind(MergeUtil.Factory mergeUtilFactory, ProjectState project, Repository git, RevCommit prior, RevCommit next) { if (!next.getFullMessage().equals(prior.getFullMessage())) { - return false; + if (next.getTree() == prior.getTree() + && prior.getParent(0).equals(next.getParent(0))) { + return ChangeKind.NO_CODE_CHANGE; + } else { + return ChangeKind.REWORK; + } } if (prior.getParentCount() != 1 || next.getParentCount() != 1) { // Trivial rebases done by machine only work well on 1 parent. - return false; + return ChangeKind.REWORK; } if (next.getTree() == prior.getTree() && prior.getParent(0).equals(next.getParent(0))) { - return true; + return ChangeKind.TRIVIAL_REBASE; } // A trivial rebase can be detected by looking for the next commit @@ -386,12 +393,16 @@ public class PatchSetInserter { ThreeWayMerger merger = mergeUtil.newThreeWayMerger(git, mergeUtil.createDryRunInserter()); merger.setBase(prior.getParent(0)); - return merger.merge(next.getParent(0), prior) - && merger.getResultTreeId().equals(next.getTree()); + if (merger.merge(next.getParent(0), prior) + && merger.getResultTreeId().equals(next.getTree())) { + return ChangeKind.TRIVIAL_REBASE; + } else { + return ChangeKind.REWORK; + } } catch (IOException err) { log.warn("Cannot check trivial rebase of new patch set " + next.name() + " in " + project.getProject().getName(), err); - return false; + return ChangeKind.REWORK; } } 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 43cc17ae05..0c960eb7b8 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 @@ -127,6 +127,7 @@ public class ProjectConfig extends VersionedMetaData { 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_VALUE = "value"; private static final String KEY_CAN_OVERRIDE = "canOverride"; private static final String KEY_Branch = "branch"; @@ -654,6 +655,8 @@ public class ProjectConfig extends VersionedMetaData { rc.getBoolean(LABEL, name, KEY_COPY_MAX_SCORE, false)); 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)); label.setCanOverride( rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, true)); label.setRefPatterns(getStringListOrNull(rc, LABEL, name, KEY_Branch)); @@ -986,6 +989,11 @@ public class ProjectConfig extends VersionedMetaData { } else { rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE); } + if (label.isCopyAllScoresIfNoCodeChange()) { + rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, true); + } else { + rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE); + } if (!label.canOverride()) { rc.setBoolean(LABEL, name, KEY_CAN_OVERRIDE, false); } else { 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 8485db7144..a98dbeeb16 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 @@ -66,6 +66,7 @@ import com.google.gerrit.server.account.AccountResolver; import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.PatchSetInserter; +import com.google.gerrit.server.change.PatchSetInserter.ChangeKind; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Submit; import com.google.gerrit.server.config.AllProjectsName; @@ -1601,7 +1602,7 @@ public class ReceiveCommits { ChangeMessage msg; String mergedIntoRef; boolean skip; - boolean trivialRebase; + ChangeKind changeKind; private PatchSet.Id priorPatchSet; ReplaceRequest(final Change.Id toChange, final RevCommit newCommit, @@ -1610,7 +1611,7 @@ public class ReceiveCommits { this.newCommit = newCommit; this.inputCommand = cmd; this.checkMergedInto = checkMergedInto; - this.trivialRebase = false; + this.changeKind = ChangeKind.REWORK; revisions = HashBiMap.create(); for (Ref ref : refs(toChange)) { @@ -1711,8 +1712,8 @@ public class ReceiveCommits { } } - trivialRebase = - PatchSetInserter.isTrivialRebase(mergeUtilFactory, + changeKind = + PatchSetInserter.getChangeKind(mergeUtilFactory, projectControl.getProjectState(), repo, priorCommit, newCommit); PatchSet.Id id = @@ -1793,7 +1794,7 @@ public class ReceiveCommits { final MailRecipients oldRecipients = getRecipientsFromApprovals( oldChangeApprovals); ApprovalsUtil.copyLabels(db, labelTypes, oldChangeApprovals, - priorPatchSet, newPatchSet, trivialRebase); + priorPatchSet, newPatchSet, changeKind); approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info, recipients.getReviewers(), oldRecipients.getAll()); recipients.add(oldRecipients);