From f5c08415e0b9e71d260de3b32a2031ae85fd8136 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 18 Sep 2013 09:41:01 +0200 Subject: [PATCH 1/2] Support copying of approvals on trivial rebase It can now be configured that all scores for a 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. This makes it also easier for authors to make trivial edits in the middle of a patch series, as approvals on later changes in the same series will be carried forward automatically. This won't work where there are conflicts in the rebase. This may allow a bad rebase to be submitted, such as when the author inverts the order of two commits and messes up setup order, such as by using a method before it is declared. We can't catch everything in every change using automated tools. This change is based on a change from Shawn Pearce that was abandoned some time ago: https://gerrit-review.googlesource.com/34801 All the credits for detecting trivial rebases go to him. Change-Id: I4768e35b3fcb432e0489bc6e10a7f18a51aafd8d Signed-off-by: Edwin Kempin --- Documentation/config-labels.txt | 12 +++++ .../google/gerrit/common/data/LabelType.java | 9 ++++ .../google/gerrit/server/ApprovalsUtil.java | 12 +++-- .../server/change/PatchSetInserter.java | 51 ++++++++++++++++++- .../google/gerrit/server/git/MergeUtil.java | 7 +-- .../gerrit/server/git/ProjectConfig.java | 8 +++ .../gerrit/server/git/ReceiveCommits.java | 14 ++++- 7 files changed, 99 insertions(+), 14 deletions(-) diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index c3e09c4d3e..0e5c27a7df 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -236,6 +236,18 @@ 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_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..83d3514058 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,7 @@ public class LabelType { protected String functionName; protected boolean copyMinScore; protected boolean copyMaxScore; + protected boolean copyAllScoresOnTrivialRebase; protected List values; protected short maxNegative; @@ -205,6 +206,14 @@ public class LabelType { this.copyMaxScore = copyMaxScore; } + public boolean isCopyAllScoresOnTrivialRebase() { + return copyAllScoresOnTrivialRebase; + } + + public void setCopyAllScoresOnTrivialRebase(boolean copyAllScoresOnTrivialRebase) { + this.copyAllScoresOnTrivialRebase = copyAllScoresOnTrivialRebase; + } + 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..7ec26b02cb 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 @@ -69,10 +69,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, boolean trivialRebase) throws OrmException { Iterable sourceApprovals = db.patchSetApprovals().byPatchSet(source); - copyLabels(db, labelTypes, sourceApprovals, source, dest); + copyLabels(db, labelTypes, sourceApprovals, source, dest, trivialRebase); } /** @@ -82,7 +82,7 @@ public class ApprovalsUtil { */ public static void copyLabels(ReviewDb db, LabelTypes labelTypes, Iterable sourceApprovals, PatchSet.Id source, - PatchSet.Id dest) throws OrmException { + PatchSet dest, boolean trivialRebase) throws OrmException { List copied = Lists.newArrayList(); for (PatchSetApproval a : sourceApprovals) { if (source.equals(a.getPatchSetId())) { @@ -90,9 +90,11 @@ 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() && trivialRebase) { + 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..2df016fb91 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,6 +25,7 @@ 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; @@ -33,12 +34,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 +53,8 @@ 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; import org.eclipse.jgit.revwalk.RevWalk; @@ -90,6 +95,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 +121,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 +137,7 @@ public class PatchSetInserter { this.commitValidatorsFactory = commitValidatorsFactory; this.indexer = indexer; this.replacePatchSetFactory = replacePatchSetFactory; + this.mergeUtilFactory = mergeUtilFactory; this.git = git; this.revWalk = revWalk; @@ -265,8 +273,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(); + boolean trivialRebase = + isTrivialRebase(mergeUtilFactory, projectState, git, priorCommit, commit); + ApprovalsUtil.copyLabels(db, refControl.getProjectControl() - .getLabelTypes(), currentPatchSetId, updatedChange.currentPatchSetId()); + .getLabelTypes(), currentPatchSetId, patchSet, trivialRebase); } final List footerLines = commit.getFooterLines(); @@ -346,6 +362,39 @@ public class PatchSetInserter { } } + public static boolean isTrivialRebase(MergeUtil.Factory mergeUtilFactory, ProjectState project, + Repository git, RevCommit prior, RevCommit next) { + if (!next.getFullMessage().equals(prior.getFullMessage())) { + return false; + } + + if (prior.getParentCount() != 1 || next.getParentCount() != 1) { + // Trivial rebases done by machine only work well on 1 parent. + return false; + } + + if (next.getTree() == prior.getTree() && + prior.getParent(0).equals(next.getParent(0))) { + return true; + } + + // 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)); + return merger.merge(next.getParent(0), prior) + && merger.getResultTreeId().equals(next.getTree()); + } catch (IOException err) { + log.warn("Cannot check trivial rebase of new patch set " + next.name() + + " in " + project.getProject().getName(), err); + return false; + } + } + 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 e9704e0293..19c2468a8c 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 @@ -487,15 +487,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 3df559db5c..43cc17ae05 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 @@ -126,6 +126,7 @@ 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_VALUE = "value"; private static final String KEY_CAN_OVERRIDE = "canOverride"; private static final String KEY_Branch = "branch"; @@ -651,6 +652,8 @@ 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.setCanOverride( rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, true)); label.setRefPatterns(getStringListOrNull(rc, LABEL, name, KEY_Branch)); @@ -978,6 +981,11 @@ 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.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 52459576d7..8485db7144 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 @@ -65,6 +65,7 @@ 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.RevisionResource; import com.google.gerrit.server.change.Submit; import com.google.gerrit.server.config.AllProjectsName; @@ -294,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(); @@ -337,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; @@ -376,6 +379,7 @@ public class ReceiveCommits { this.subOpFactory = subOpFactory; this.submitProvider = submitProvider; this.mergeQueue = mergeQueue; + this.mergeUtilFactory = mergeUtilFactory; this.messageSender = new ReceivePackMessageSender(); @@ -1597,6 +1601,7 @@ public class ReceiveCommits { ChangeMessage msg; String mergedIntoRef; boolean skip; + boolean trivialRebase; private PatchSet.Id priorPatchSet; ReplaceRequest(final Change.Id toChange, final RevCommit newCommit, @@ -1605,6 +1610,7 @@ public class ReceiveCommits { this.newCommit = newCommit; this.inputCommand = cmd; this.checkMergedInto = checkMergedInto; + this.trivialRebase = false; revisions = HashBiMap.create(); for (Ref ref : refs(toChange)) { @@ -1705,6 +1711,10 @@ public class ReceiveCommits { } } + trivialRebase = + PatchSetInserter.isTrivialRebase(mergeUtilFactory, + projectControl.getProjectState(), repo, priorCommit, newCommit); + PatchSet.Id id = ChangeUtil.nextPatchSetId(allRefs, change.currentPatchSetId()); newPatchSet = new PatchSet(id); @@ -1783,7 +1793,7 @@ public class ReceiveCommits { final MailRecipients oldRecipients = getRecipientsFromApprovals( oldChangeApprovals); ApprovalsUtil.copyLabels(db, labelTypes, oldChangeApprovals, - priorPatchSet, newPatchSet.getId()); + priorPatchSet, newPatchSet, trivialRebase); approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info, recipients.getReviewers(), oldRecipients.getAll()); recipients.add(oldRecipients); From 54122d372511fb3a7835f51ab946da7abc1865a9 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 18 Sep 2013 13:46:14 +0200 Subject: [PATCH 2/2] Support copying of approvals if new patch set has no code changes It can now be configured that all scores for a 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. E.g. this configuration could be used for the Verified category if it is only used by an automated tool to express that build and tests succeed. Change-Id: I105f85bd28d0bea39a4e16aaebf2630ef1860fbe Signed-off-by: Edwin Kempin --- Documentation/config-labels.txt | 12 +++++++ .../google/gerrit/common/data/LabelType.java | 9 +++++ .../google/gerrit/server/ApprovalsUtil.java | 13 ++++--- .../server/change/PatchSetInserter.java | 35 ++++++++++++------- .../gerrit/server/git/ProjectConfig.java | 8 +++++ .../gerrit/server/git/ReceiveCommits.java | 11 +++--- 6 files changed, 67 insertions(+), 21 deletions(-) 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);