diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index c3e09c4d3e..c08d48462f 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -236,6 +236,30 @@ forward when a new patch set is uploaded. This can be used to enable sticky approvals, reducing turn-around for trivial cleanups prior to submitting a change. +[[label_copyAllScoresOnTrivialRebase]] +`label.Label-Name.copyAllScoresOnTrivialRebase` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If true, all scores for the label are copied forward when a new patch +set is uploaded that is a trivial rebase. A new patch set is considered +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. + +[[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 caf914d47d..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 @@ -97,6 +97,8 @@ public class LabelType { protected String functionName; protected boolean copyMinScore; protected boolean copyMaxScore; + protected boolean copyAllScoresOnTrivialRebase; + protected boolean copyAllScoresIfNoCodeChange; protected List values; protected short maxNegative; @@ -205,6 +207,22 @@ public class LabelType { this.copyMaxScore = copyMaxScore; } + public boolean isCopyAllScoresOnTrivialRebase() { + return copyAllScoresOnTrivialRebase; + } + + public void setCopyAllScoresOnTrivialRebase(boolean copyAllScoresOnTrivialRebase) { + 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 3fd24f1988..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.Id dest) throws OrmException { + PatchSet.Id source, PatchSet dest, ChangeKind changeKind) throws OrmException { Iterable sourceApprovals = db.patchSetApprovals().byPatchSet(source); - copyLabels(db, labelTypes, sourceApprovals, source, dest); + 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.Id dest) throws OrmException { + PatchSet dest, ChangeKind changeKind) throws OrmException { List copied = Lists.newArrayList(); for (PatchSetApproval a : sourceApprovals) { if (source.equals(a.getPatchSetId())) { @@ -90,9 +91,15 @@ public class ApprovalsUtil { if (type == null) { continue; } else if (type.isCopyMinScore() && type.isMaxNegative(a)) { - copied.add(new PatchSetApproval(dest, a)); + copied.add(new PatchSetApproval(dest.getId(), a)); } else if (type.isCopyMaxScore() && type.isMaxPositive(a)) { - copied.add(new PatchSetApproval(dest, a)); + copied.add(new PatchSetApproval(dest.getId(), a)); + } 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 5e2c797aaa..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 @@ -33,12 +33,14 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.config.TrackingFooters; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.validators.CommitValidationException; import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.mail.ReplacePatchSetSender; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.InvalidChangeOperationException; +import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.ssh.NoSshInfo; import com.google.gerrit.server.ssh.SshInfo; @@ -50,6 +52,7 @@ 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.ThreeWayMerger; import org.eclipse.jgit.revwalk.FooterLine; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; @@ -81,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; @@ -90,6 +97,7 @@ public class PatchSetInserter { private final CommitValidators.Factory commitValidatorsFactory; private final ChangeIndexer indexer; private final ReplacePatchSetSender.Factory replacePatchSetFactory; + private final MergeUtil.Factory mergeUtilFactory; private final Repository git; private final RevWalk revWalk; @@ -115,6 +123,7 @@ public class PatchSetInserter { CommitValidators.Factory commitValidatorsFactory, ChangeIndexer indexer, ReplacePatchSetSender.Factory replacePatchSetFactory, + MergeUtil.Factory mergeUtilFactory, @Assisted Repository git, @Assisted RevWalk revWalk, @Assisted RefControl refControl, @@ -130,6 +139,7 @@ public class PatchSetInserter { this.commitValidatorsFactory = commitValidatorsFactory; this.indexer = indexer; this.replacePatchSetFactory = replacePatchSetFactory; + this.mergeUtilFactory = mergeUtilFactory; this.git = git; this.revWalk = revWalk; @@ -265,8 +275,16 @@ public class PatchSetInserter { } if (copyLabels) { + PatchSet priorPatchSet = db.patchSets().get(currentPatchSetId); + ObjectId priorCommitId = ObjectId.fromString(priorPatchSet.getRevision().get()); + RevCommit priorCommit = revWalk.parseCommit(priorCommitId); + ProjectState projectState = + refControl.getProjectControl().getProjectState(); + ChangeKind changeKind = + getChangeKind(mergeUtilFactory, projectState, git, priorCommit, commit); + ApprovalsUtil.copyLabels(db, refControl.getProjectControl() - .getLabelTypes(), currentPatchSetId, updatedChange.currentPatchSetId()); + .getLabelTypes(), currentPatchSetId, patchSet, changeKind); } final List footerLines = commit.getFooterLines(); @@ -346,6 +364,48 @@ public class PatchSetInserter { } } + public static ChangeKind getChangeKind(MergeUtil.Factory mergeUtilFactory, ProjectState project, + Repository git, RevCommit prior, RevCommit next) { + if (!next.getFullMessage().equals(prior.getFullMessage())) { + 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 ChangeKind.REWORK; + } + + if (next.getTree() == prior.getTree() && + prior.getParent(0).equals(next.getParent(0))) { + 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. + try { + MergeUtil mergeUtil = mergeUtilFactory.create(project); + ThreeWayMerger merger = + mergeUtil.newThreeWayMerger(git, mergeUtil.createDryRunInserter()); + merger.setBase(prior.getParent(0)); + 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 ChangeKind.REWORK; + } + } + public class ChangeModifiedException extends InvalidChangeOperationException { private static final long serialVersionUID = 1L; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java index d89568e445..07c0c5dfa5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java @@ -486,15 +486,10 @@ public class MergeUtil { public ObjectInserter createDryRunInserter() { return new ObjectInserter() { - private final MutableObjectId buf = new MutableObjectId(); - private static final int LAST_BYTE = Constants.OBJECT_ID_LENGTH - 1; - @Override public ObjectId insert(int objectType, long length, InputStream in) throws IOException { - // create non-existing dummy ID - buf.setByte(LAST_BYTE, buf.getByte(LAST_BYTE) + 1); - return buf.copy(); + return idFor(objectType, length, in); } @Override 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 ec61a57d14..e4ee1bff73 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 @@ -128,6 +128,8 @@ public class ProjectConfig extends VersionedMetaData { private static final String KEY_FUNCTION = "function"; 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"; @@ -657,6 +659,10 @@ public class ProjectConfig extends VersionedMetaData { rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, false)); label.setCopyMaxScore( 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)); @@ -1006,6 +1012,16 @@ public class ProjectConfig extends VersionedMetaData { } else { rc.unset(LABEL, name, KEY_COPY_MAX_SCORE); } + if (label.isCopyAllScoresOnTrivialRebase()) { + rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, true); + } 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 4a7624fd39..95f06165e2 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,8 @@ import com.google.gerrit.server.account.AccountCache; 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; @@ -293,6 +295,7 @@ public class ReceiveCommits { private final SubmoduleOp.Factory subOpFactory; private final Provider submitProvider; private final MergeQueue mergeQueue; + private final MergeUtil.Factory mergeUtilFactory; private final List messages = new ArrayList(); private ListMultimap errors = LinkedListMultimap.create(); @@ -336,7 +339,8 @@ public class ReceiveCommits { @Assisted final Repository repo, final SubmoduleOp.Factory subOpFactory, final Provider submitProvider, - final MergeQueue mergeQueue) throws IOException { + final MergeQueue mergeQueue, + final MergeUtil.Factory mergeUtilFactory) throws IOException { this.currentUser = (IdentifiedUser) projectControl.getCurrentUser(); this.db = db; this.schemaFactory = schemaFactory; @@ -375,6 +379,7 @@ public class ReceiveCommits { this.subOpFactory = subOpFactory; this.submitProvider = submitProvider; this.mergeQueue = mergeQueue; + this.mergeUtilFactory = mergeUtilFactory; this.messageSender = new ReceivePackMessageSender(); @@ -1607,6 +1612,7 @@ public class ReceiveCommits { ChangeMessage msg; String mergedIntoRef; boolean skip; + ChangeKind changeKind; private PatchSet.Id priorPatchSet; ReplaceRequest(final Change.Id toChange, final RevCommit newCommit, @@ -1615,6 +1621,7 @@ public class ReceiveCommits { this.newCommit = newCommit; this.inputCommand = cmd; this.checkMergedInto = checkMergedInto; + this.changeKind = ChangeKind.REWORK; revisions = HashBiMap.create(); for (Ref ref : refs(toChange)) { @@ -1715,6 +1722,10 @@ public class ReceiveCommits { } } + changeKind = + PatchSetInserter.getChangeKind(mergeUtilFactory, + projectControl.getProjectState(), repo, priorCommit, newCommit); + PatchSet.Id id = ChangeUtil.nextPatchSetId(allRefs, change.currentPatchSetId()); newPatchSet = new PatchSet(id); @@ -1793,7 +1804,7 @@ public class ReceiveCommits { final MailRecipients oldRecipients = getRecipientsFromApprovals( oldChangeApprovals); ApprovalsUtil.copyLabels(db, labelTypes, oldChangeApprovals, - priorPatchSet, newPatchSet.getId()); + priorPatchSet, newPatchSet, changeKind); approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info, recipients.getReviewers(), oldRecipients.getAll()); recipients.add(oldRecipients);