opendev/gerrit
Code Issues Proposed changes

Support copying of approvals if new patch set has no code changes

Browse Source 
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 <edwin.kempin@sap.com>
This commit is contained in:
Edwin Kempin 2013-09-18 13:46:14 +02:00
parent f5c08415e0
commit 54122d3725
6 changed files with 67 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
Documentation/config-labels.txt
View File

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

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

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

13
gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
View File

@ -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<PatchSetApproval> 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<PatchSetApproval> sourceApprovals, PatchSet.Id source,
PatchSet dest, boolean trivialRebase) throws OrmException {
PatchSet dest, ChangeKind changeKind) throws OrmException {
List<PatchSetApproval> 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));
}
}

35
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

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

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

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

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

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