ReceiveCommits: Add support for approvals

Add -l option to magic branch to pass approvals through git push.

One use case is to support Work In Progress (WIP) Workflow backed by
WIP label. In this scenario new label is introduced with (-1,0) range
to mark a change as WIP. This change allows to optionally mark a new
patch set as WIP through `git push`.

With configuration of push macro in .git/config:
  git config remote.wip.url ssh://review.example.com:29418/project
  git config remote.wip.push HEAD:refs/for/master%l=wip-1

WIP patch set can be uploaded with:
  git push wip

Another use case is to do %l=Verified+1 if a change was compiled and
tested locally, e.g.:

  git config alias r '! buck build gerrit && buck test --all && \\
  git push gerrit-review HEAD:refs/for/master%l=Verified+1'

Then all these can be done in one step - compile, run tests and push to
review:

  git r

Change-Id: I52a67993952f32f8a04d9c0226f1c456a6f522b4
This commit is contained in:
David Ostrovsky 2014-05-01 19:33:36 +02:00 committed by Shawn Pearce
parent 438e99402e
commit 913cc07299
5 changed files with 145 additions and 4 deletions

View File

@ -15,10 +15,14 @@
package com.google.gerrit.acceptance.git; package com.google.gerrit.acceptance.git;
import static com.google.gerrit.acceptance.GitUtil.cloneProject; import static com.google.gerrit.acceptance.GitUtil.cloneProject;
import static org.junit.Assert.assertEquals;
import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@ -143,6 +147,43 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
r.assertChange(Change.Status.DRAFT, null); r.assertChange(Change.Status.DRAFT, null);
} }
@Test
public void testPushForMasterWithApprovals() throws GitAPIException,
IOException, RestApiException {
PushOneCommit.Result r = pushTo("refs/for/master/%l=Code-Review");
r.assertOkStatus();
ChangeInfo ci = get(r.getChangeId());
LabelInfo cr = ci.labels.get("Code-Review");
assertEquals(1, cr.all.size());
assertEquals("Administrator", cr.all.get(0).name);
assertEquals(1, cr.all.get(0).value.intValue());
PushOneCommit push =
pushFactory.create(db, admin.getIdent(), PushOneCommit.SUBJECT,
"b.txt", "anotherContent", r.getChangeId());
r = push.to(git, "refs/for/master/%l=Code-Review+2");
ci = get(r.getChangeId());
cr = ci.labels.get("Code-Review");
assertEquals(1, cr.all.size());
assertEquals("Administrator", cr.all.get(0).name);
assertEquals(2, cr.all.get(0).value.intValue());
}
@Test
public void testPushForMasterWithApprovals_MissingLabel() throws GitAPIException,
IOException {
PushOneCommit.Result r = pushTo("refs/for/master/%l=Verify");
r.assertErrorStatus("label \"Verify\" is not a configured label");
}
@Test
public void testPushForMasterWithApprovals_ValueOutOfRange() throws GitAPIException,
IOException, RestApiException {
PushOneCommit.Result r = pushTo("refs/for/master/%l=Code-Review-3");
r.assertErrorStatus("label \"Code-Review\": -3 is not a valid value");
}
@Test @Test
public void testPushForNonExistingBranch() throws GitAPIException, public void testPushForNonExistingBranch() throws GitAPIException,
OrmException, IOException { OrmException, IOException {

View File

@ -32,6 +32,8 @@ import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets; import com.google.common.collect.Sets;
import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSet;
@ -49,9 +51,11 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import java.sql.Timestamp; import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection; import java.util.Collection;
import java.util.Collections; import java.util.Collections;
import java.util.List; import java.util.List;
import java.util.Map;
import java.util.Set; import java.util.Set;
/** /**
@ -215,6 +219,52 @@ public class ApprovalsUtil {
return Collections.unmodifiableList(cells); return Collections.unmodifiableList(cells);
} }
public void addApprovals(ReviewDb db, ChangeUpdate update, LabelTypes labelTypes,
PatchSet ps, PatchSetInfo info, Change change, ChangeControl changeCtl,
Map<String, Short> approvals) throws OrmException {
if (!approvals.isEmpty()) {
checkApprovals(approvals, labelTypes, change, changeCtl);
List<PatchSetApproval> cells = new ArrayList<>(approvals.size());
Timestamp ts = TimeUtil.nowTs();
for (Map.Entry<String, Short> vote : approvals.entrySet()) {
LabelType lt = labelTypes.byLabel(vote.getKey());
cells.add(new PatchSetApproval(new PatchSetApproval.Key(
ps.getId(),
info.getCommitter().getAccount(),
lt.getLabelId()),
vote.getValue(),
ts));
update.putApproval(vote.getKey(), vote.getValue());
}
db.patchSetApprovals().insert(cells);
}
}
public static void checkLabel(LabelTypes labelTypes, String name, Short value) {
LabelType label = labelTypes.byLabel(name);
if (label == null) {
throw new IllegalArgumentException(String.format(
"label \"%s\" is not a configured label", name));
}
if (label.getValue(value) == null) {
throw new IllegalArgumentException(String.format(
"label \"%s\": %d is not a valid value", name, value));
}
}
private static void checkApprovals(Map<String, Short> approvals, LabelTypes labelTypes,
Change change, ChangeControl changeCtl) {
for (Map.Entry<String, Short> vote : approvals.entrySet()) {
String name = vote.getKey();
Short value = vote.getValue();
PermissionRange range = changeCtl.getRange(Permission.forLabel(name));
if (range == null || !range.contains(value)) {
throw new IllegalArgumentException(String.format(
"applying label \"%s\": %d is restricted", name, value));
}
}
}
public ListMultimap<PatchSet.Id, PatchSetApproval> byChange(ReviewDb db, public ListMultimap<PatchSet.Id, PatchSetApproval> byChange(ReviewDb db,
ChangeNotes notes) throws OrmException { ChangeNotes notes) throws OrmException {
if (!migration.readPatchSetApprovals()) { if (!migration.readPatchSetApprovals()) {

View File

@ -32,6 +32,7 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.mail.CreateChangeSender; import com.google.gerrit.server.mail.CreateChangeSender;
import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.project.RefControl;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
@ -45,6 +46,7 @@ import org.slf4j.LoggerFactory;
import java.io.IOException; import java.io.IOException;
import java.util.Collections; import java.util.Collections;
import java.util.Map;
import java.util.Set; import java.util.Set;
public class ChangeInserter { public class ChangeInserter {
@ -72,6 +74,7 @@ public class ChangeInserter {
private ChangeMessage changeMessage; private ChangeMessage changeMessage;
private Set<Account.Id> reviewers; private Set<Account.Id> reviewers;
private Set<Account.Id> extraCC; private Set<Account.Id> extraCC;
private Map<String, Short> approvals;
private boolean runHooks; private boolean runHooks;
private boolean sendMail; private boolean sendMail;
@ -100,6 +103,7 @@ public class ChangeInserter {
this.commit = commit; this.commit = commit;
this.reviewers = Collections.emptySet(); this.reviewers = Collections.emptySet();
this.extraCC = Collections.emptySet(); this.extraCC = Collections.emptySet();
this.approvals = Collections.emptyMap();
this.runHooks = true; this.runHooks = true;
this.sendMail = true; this.sendMail = true;
@ -152,14 +156,20 @@ public class ChangeInserter {
return patchSet; return patchSet;
} }
public ChangeInserter setApprovals(Map<String, Short> approvals) {
this.approvals = approvals;
return this;
}
public PatchSetInfo getPatchSetInfo() { public PatchSetInfo getPatchSetInfo() {
return patchSetInfo; return patchSetInfo;
} }
public Change insert() throws OrmException, IOException { public Change insert() throws OrmException, IOException {
ReviewDb db = dbProvider.get(); ReviewDb db = dbProvider.get();
ChangeControl ctl = refControl.getProjectControl().controlFor(change);
ChangeUpdate update = updateFactory.create( ChangeUpdate update = updateFactory.create(
refControl.getProjectControl().controlFor(change), ctl,
change.getCreatedOn()); change.getCreatedOn());
db.changes().beginTransaction(change.getId()); db.changes().beginTransaction(change.getId());
try { try {
@ -169,6 +179,8 @@ public class ChangeInserter {
LabelTypes labelTypes = refControl.getProjectControl().getLabelTypes(); LabelTypes labelTypes = refControl.getProjectControl().getLabelTypes();
approvalsUtil.addReviewers(db, update, labelTypes, change, approvalsUtil.addReviewers(db, update, labelTypes, change,
patchSet, patchSetInfo, reviewers, Collections.<Account.Id> emptySet()); patchSet, patchSetInfo, reviewers, Collections.<Account.Id> emptySet());
approvalsUtil.addApprovals(db, update, labelTypes, patchSet, patchSetInfo,
change, ctl, approvals);
if (messageIsForChange()) { if (messageIsForChange()) {
insertMessage(db); insertMessage(db);
} }

View File

@ -50,6 +50,7 @@ import com.google.gerrit.common.ChangeHookRunner.HookResult;
import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.Nullable; import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.Capable; import com.google.gerrit.common.data.Capable;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule;
@ -102,6 +103,7 @@ import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.ssh.SshInfo; import com.google.gerrit.server.ssh.SshInfo;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.MagicBranch; import com.google.gerrit.server.util.MagicBranch;
import com.google.gerrit.server.util.RequestScopePropagator; import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.util.TimeUtil; import com.google.gerrit.server.util.TimeUtil;
@ -1070,7 +1072,10 @@ public class ReceiveCommits {
RefControl ctl; RefControl ctl;
Set<Account.Id> reviewer = Sets.newLinkedHashSet(); Set<Account.Id> reviewer = Sets.newLinkedHashSet();
Set<Account.Id> cc = Sets.newLinkedHashSet(); Set<Account.Id> cc = Sets.newLinkedHashSet();
Map<String, Short> labels = new HashMap<>();
List<RevCommit> baseCommit; List<RevCommit> baseCommit;
LabelTypes labelTypes;
CmdLineParser clp;
@Option(name = "--base", metaVar = "BASE", usage = "merge base of changes") @Option(name = "--base", metaVar = "BASE", usage = "merge base of changes")
List<ObjectId> base; List<ObjectId> base;
@ -1099,9 +1104,23 @@ public class ReceiveCommits {
draft = !publish; draft = !publish;
} }
MagicBranchInput(ReceiveCommand cmd) { @Option(name = "-l", metaVar = "LABEL+VALUE",
usage = "label(s) to assign (defaults to +1 if no value provided")
void addLabel(final String token) throws CmdLineException {
LabelVote v = LabelVote.parse(token);
try {
LabelType.checkName(v.getLabel());
ApprovalsUtil.checkLabel(labelTypes, v.getLabel(), v.getValue());
} catch (IllegalArgumentException e) {
throw clp.reject(e.getMessage());
}
labels.put(v.getLabel(), v.getValue());
}
MagicBranchInput(ReceiveCommand cmd, LabelTypes labelTypes) {
this.cmd = cmd; this.cmd = cmd;
this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE); this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE);
this.labelTypes = labelTypes;
} }
boolean isDraft() { boolean isDraft() {
@ -1116,6 +1135,10 @@ public class ReceiveCommits {
return new MailRecipients(reviewer, cc); return new MailRecipients(reviewer, cc);
} }
Map<String, Short> getLabels() {
return labels;
}
String parse(CmdLineParser clp, Repository repo, Set<String> refs) String parse(CmdLineParser clp, Repository repo, Set<String> refs)
throws CmdLineException { throws CmdLineException {
String ref = MagicBranch.getDestBranchName(cmd.getRefName()); String ref = MagicBranch.getDestBranchName(cmd.getRefName());
@ -1158,6 +1181,10 @@ public class ReceiveCommits {
} }
return ref.substring(0, split); return ref.substring(0, split);
} }
void setCmdLineParser(CmdLineParser clp) {
this.clp = clp;
}
} }
private void parseMagicBranch(final ReceiveCommand cmd) { private void parseMagicBranch(final ReceiveCommand cmd) {
@ -1167,12 +1194,13 @@ public class ReceiveCommits {
return; return;
} }
magicBranch = new MagicBranchInput(cmd); magicBranch = new MagicBranchInput(cmd, labelTypes);
magicBranch.reviewer.addAll(reviewersFromCommandLine); magicBranch.reviewer.addAll(reviewersFromCommandLine);
magicBranch.cc.addAll(ccFromCommandLine); magicBranch.cc.addAll(ccFromCommandLine);
String ref; String ref;
CmdLineParser clp = optionParserFactory.create(magicBranch); CmdLineParser clp = optionParserFactory.create(magicBranch);
magicBranch.setCmdLineParser(clp);
try { try {
ref = magicBranch.parse(clp, repo, rp.getAdvertisedRefs().keySet()); ref = magicBranch.parse(clp, repo, rp.getAdvertisedRefs().keySet());
} catch (CmdLineException e) { } catch (CmdLineException e) {
@ -1593,8 +1621,10 @@ public class ReceiveCommits {
final Account.Id me = currentUser.getAccountId(); final Account.Id me = currentUser.getAccountId();
final List<FooterLine> footerLines = commit.getFooterLines(); final List<FooterLine> footerLines = commit.getFooterLines();
final MailRecipients recipients = new MailRecipients(); final MailRecipients recipients = new MailRecipients();
Map<String, Short> approvals = new HashMap<>();
if (magicBranch != null) { if (magicBranch != null) {
recipients.add(magicBranch.getMailRecipients()); recipients.add(magicBranch.getMailRecipients());
approvals = magicBranch.getLabels();
} }
recipients.add(getRecipientsFromFooters(accountResolver, ps, footerLines)); recipients.add(getRecipientsFromFooters(accountResolver, ps, footerLines));
recipients.remove(me); recipients.remove(me);
@ -1606,6 +1636,7 @@ public class ReceiveCommits {
ins ins
.setReviewers(recipients.getReviewers()) .setReviewers(recipients.getReviewers())
.setApprovals(approvals)
.setMessage(msg) .setMessage(msg)
.setSendMail(false) .setSendMail(false)
.insert(); .insert();
@ -1923,8 +1954,10 @@ public class ReceiveCommits {
final Account.Id me = currentUser.getAccountId(); final Account.Id me = currentUser.getAccountId();
final List<FooterLine> footerLines = newCommit.getFooterLines(); final List<FooterLine> footerLines = newCommit.getFooterLines();
final MailRecipients recipients = new MailRecipients(); final MailRecipients recipients = new MailRecipients();
Map<String, Short> approvals = new HashMap<>();
if (magicBranch != null) { if (magicBranch != null) {
recipients.add(magicBranch.getMailRecipients()); recipients.add(magicBranch.getMailRecipients());
approvals = magicBranch.getLabels();
} }
recipients.add(getRecipientsFromFooters(accountResolver, newPatchSet, footerLines)); recipients.add(getRecipientsFromFooters(accountResolver, newPatchSet, footerLines));
recipients.remove(me); recipients.remove(me);
@ -1952,6 +1985,8 @@ public class ReceiveCommits {
approvalCopier.copy(db, changeCtl, newPatchSet); approvalCopier.copy(db, changeCtl, newPatchSet);
approvalsUtil.addReviewers(db, update, labelTypes, change, newPatchSet, approvalsUtil.addReviewers(db, update, labelTypes, change, newPatchSet,
info, recipients.getReviewers(), oldRecipients.getAll()); info, recipients.getReviewers(), oldRecipients.getAll());
approvalsUtil.addApprovals(db, update, labelTypes, newPatchSet, info,
change, changeCtl, approvals);
recipients.add(oldRecipients); recipients.add(oldRecipients);
msg = msg =

View File

@ -54,7 +54,6 @@ import org.kohsuke.args4j.spi.OptionHandler;
import org.kohsuke.args4j.spi.Setter; import org.kohsuke.args4j.spi.Setter;
import org.kohsuke.args4j.spi.FieldSetter; import org.kohsuke.args4j.spi.FieldSetter;
import java.io.StringWriter; import java.io.StringWriter;
import java.io.Writer; import java.io.Writer;
import java.lang.annotation.Annotation; import java.lang.annotation.Annotation;
@ -443,4 +442,8 @@ public class CmdLineParser {
return false; return false;
} }
} }
public CmdLineException reject(String message) {
return new CmdLineException(parser, message);
}
} }