opendev/gerrit
Code Issues Proposed changes

Record submit records in notedb at submit time

Browse Source 
Record a new footer, Submitted-with, to indicate the results of the
submit rule evaluator. Record additional normalized labels during
Submit by recording multiple commits in a single batch update, prior
to the update that actually submits the change.

Change-Id: Ia87913ad9d710fe0dea59ea56537e4ec1dd9f648
This commit is contained in:
Dave Borowitz
2014-01-10 10:01:04 -08:00
parent aebe37efa9
commit a57ac7e0f5
10 changed files with 448 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
View File

@@ -400,6 +400,23 @@ public final class Change {
setLastSha1MergeTested(null);
}
public Change(Change other) {
changeId = other.changeId;
changeKey = other.changeKey;
rowVersion = other.rowVersion;
createdOn = other.createdOn;
lastUpdatedOn = other.lastUpdatedOn;
sortKey = other.sortKey;
owner = other.owner;
dest = other.dest;
open = other.open;
status = other.status;
currentPatchSetId = other.currentPatchSetId;
subject = other.subject;
topic = other.topic;
mergeable = other.mergeable;
}
/** Legacy 32 bit integer identity for a change. */
public Change.Id getId() {
return changeId;

98
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@@ -18,15 +18,19 @@ import static com.google.gerrit.common.data.SubmitRecord.Status.OK;
import com.google.common.base.Optional;
import com.google.common.base.Predicate;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Table;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -35,12 +39,14 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ProjectUtil;
import com.google.gerrit.server.change.ChangeJson.ChangeInfo;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.git.MergeQueue;
import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ChangeControl;
@@ -51,6 +57,8 @@ import com.google.inject.Inject;
import com.google.inject.Provider;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.PersonIdent;
import java.io.IOException;
import java.sql.Timestamp;
@@ -78,8 +86,10 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
}
}
private final PersonIdent serverIdent;
private final Provider<ReviewDb> dbProvider;
private final GitRepositoryManager repoManager;
private final IdentifiedUser.GenericFactory userFactory;
private final ChangeUpdate.Factory updateFactory;
private final ApprovalsUtil approvalsUtil;
private final MergeQueue mergeQueue;
@@ -87,15 +97,19 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
private final LabelNormalizer labelNormalizer;
@Inject
Submit(Provider<ReviewDb> dbProvider,
Submit(@GerritPersonIdent PersonIdent serverIdent,
Provider<ReviewDb> dbProvider,
GitRepositoryManager repoManager,
IdentifiedUser.GenericFactory userFactory,
ChangeUpdate.Factory updateFactory,
ApprovalsUtil approvalsUtil,
MergeQueue mergeQueue,
ChangeIndexer indexer,
LabelNormalizer labelNormalizer) {
this.serverIdent = serverIdent;
this.dbProvider = dbProvider;
this.repoManager = repoManager;
this.userFactory = userFactory;
this.updateFactory = updateFactory;
this.approvalsUtil = approvalsUtil;
this.mergeQueue = mergeQueue;
@@ -125,8 +139,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
rsrc.getPatchSet().getRevision().get()));
}
checkSubmitRule(rsrc);
change = submit(rsrc, caller);
change = submit(rsrc, caller, false);
if (change == null) {
throw new ResourceConflictException("change is "
+ status(dbProvider.get().changes().get(rsrc.getChange().getId())));
@@ -189,15 +202,22 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
}), null);
}
public Change submit(RevisionResource rsrc, IdentifiedUser caller)
throws OrmException, IOException {
public Change submit(RevisionResource rsrc, IdentifiedUser caller,
boolean force) throws ResourceConflictException, OrmException,
IOException {
List<SubmitRecord> submitRecords = checkSubmitRule(rsrc, force);
final Timestamp timestamp = TimeUtil.nowTs();
Change change = rsrc.getChange();
ChangeUpdate update = updateFactory.create(rsrc.getControl(), timestamp);
update.submit(submitRecords);
ReviewDb db = dbProvider.get();
db.changes().beginTransaction(change.getId());
try {
approve(rsrc, update, caller, timestamp);
BatchMetaDataUpdate batch = approve(rsrc, update, caller, timestamp);
// Write update commit after all normalized label commits.
batch.write(update, new CommitBuilder());
change = db.changes().atomicUpdate(
change.getId(),
new AtomicUpdate<Change>() {
@@ -223,8 +243,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
return change;
}
private void approve(RevisionResource rsrc, ChangeUpdate update,
IdentifiedUser caller, Timestamp timestamp) throws OrmException {
private BatchMetaDataUpdate approve(RevisionResource rsrc,
ChangeUpdate update, IdentifiedUser caller, Timestamp timestamp)
throws OrmException {
PatchSet.Id psId = rsrc.getPatchSet().getId();
List<PatchSetApproval> approvals =
approvalsUtil.byPatchSet(dbProvider.get(), rsrc.getNotes(), psId);
@@ -261,19 +282,71 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
// TODO(dborowitz): Don't use a label in notedb; just check when status
// change happened.
update.putApproval(submit.getLabel(), submit.getValue());
dbProvider.get().patchSetApprovals().upsert(normalized.getNormalized());
dbProvider.get().patchSetApprovals().delete(normalized.getDeleted());
try {
return saveToBatch(rsrc, update, normalized, timestamp);
} catch (IOException e) {
throw new OrmException(e);
}
}
private void checkSubmitRule(RevisionResource rsrc)
throws ResourceConflictException {
List<SubmitRecord> results = rsrc.getControl().canSubmit(
private BatchMetaDataUpdate saveToBatch(RevisionResource rsrc,
ChangeUpdate callerUpdate, LabelNormalizer.Result normalized,
Timestamp timestamp) throws IOException {
Table<Account.Id, String, Optional<Short>> byUser = HashBasedTable.create();
for (PatchSetApproval psa : normalized.getUpdated()) {
byUser.put(psa.getAccountId(), psa.getLabel(),
Optional.of(psa.getValue()));
}
for (PatchSetApproval psa : normalized.getDeleted()) {
byUser.put(psa.getAccountId(), psa.getLabel(), Optional.<Short> absent());
}
ChangeControl ctl = rsrc.getControl();
BatchMetaDataUpdate batch = callerUpdate.openUpdate();
for (Account.Id accountId : byUser.rowKeySet()) {
if (!accountId.equals(callerUpdate.getUser().getAccountId())) {
ChangeUpdate update = updateFactory.create(
ctl.forUser(userFactory.create(dbProvider, accountId)), timestamp);
update.setSubject("Finalize approvals at submit");
putApprovals(update, byUser.row(accountId));
CommitBuilder commit = new CommitBuilder();
commit.setCommitter(new PersonIdent(serverIdent, timestamp));
batch.write(update, commit);
}
}
putApprovals(callerUpdate,
byUser.row(callerUpdate.getUser().getAccountId()));
return batch;
}
private static void putApprovals(ChangeUpdate update,
Map<String, Optional<Short>> approvals) {
for (Map.Entry<String, Optional<Short>> e : approvals.entrySet()) {
if (e.getValue().isPresent()) {
update.putApproval(e.getKey(), e.getValue().get());
} else {
update.removeApproval(e.getKey());
}
}
}
private List<SubmitRecord> checkSubmitRule(RevisionResource rsrc,
boolean force) throws ResourceConflictException {
List<SubmitRecord> results = rsrc.getControl().canSubmit(
dbProvider.get(),
rsrc.getPatchSet());
Optional<SubmitRecord> ok = findOkRecord(results);
if (ok.isPresent()) {
// Rules supplied a valid solution.
return;
return ImmutableList.of(ok.get());
} else if (force) {
return results;
} else if (results.isEmpty()) {
throw new IllegalStateException(String.format(
"ChangeControl.canSubmit returned empty list for %s in %s",
@@ -333,6 +406,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
rsrc.getChange().getProject().get()));
}
}
throw new IllegalStateException();
}
private static Optional<SubmitRecord> findOkRecord(Collection<SubmitRecord> in) {

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

@@ -54,6 +54,7 @@ import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicMap.Entry;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
@@ -1618,7 +1619,13 @@ public class ReceiveCommits {
throws OrmException, IOException {
Submit submit = submitProvider.get();
RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps);
Change c = submit.submit(rsrc, currentUser);
Change c;
try {
// Force submit even if submit rule evaluation fails.
c = submit.submit(rsrc, currentUser, true);
} catch (ResourceConflictException e) {
throw new IOException(e);
}
if (c == null) {
addError("Submitting change " + changeCtl.getChange().getChangeId()
+ " failed.");

3
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
View File

@@ -183,9 +183,6 @@ public class CherryPick extends SubmitStrategy {
: args.approvalsUtil.byPatchSet(args.db, n.notes(), n.getPatchsetId())) {
approvals.add(new PatchSetApproval(ps.getId(), a));
}
// TODO(dborowitz): This doesn't copy labels in the notedb. We should
// stamp those in atomically with the same commit that describes the
// change being submitted.
args.db.patchSetApprovals().insert(approvals);
ru = args.repo.updateRef(ps.getRefName());

3
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
View File

@@ -94,9 +94,6 @@ public class RebaseIfNecessary extends SubmitStrategy {
newMergeTip, args.mergeUtil, committerIdent,
false, false, ValidatePolicy.NONE);
// TODO(dborowitz): This doesn't copy labels in the notedb. We
// should stamp those in atomically with the same commit that
// describes the change being submitted.
List<PatchSetApproval> approvals = Lists.newArrayList();
for (PatchSetApproval a : args.approvalsUtil.byPatchSet(
args.db, n.notes(), n.getPatchsetId())) {

3
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
View File

@@ -24,6 +24,9 @@ public class ChangeNoteUtil {
static final FooterKey FOOTER_LABEL = new FooterKey("Label");
static final FooterKey FOOTER_PATCH_SET = new FooterKey("Patch-set");
static final FooterKey FOOTER_STATUS = new FooterKey("Status");
static final FooterKey FOOTER_SUBMITTED_WITH =
new FooterKey("Submitted-with");
public static String changeRefName(Change.Id id) {
StringBuilder r = new StringBuilder();

124
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -16,21 +16,27 @@ package com.google.gerrit.server.notedb;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WITH;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Enums;
import com.google.common.base.Function;
import com.google.common.base.Optional;
import com.google.common.base.Supplier;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Ordering;
import com.google.common.collect.Table;
import com.google.common.collect.Tables;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -48,6 +54,7 @@ import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.FooterKey;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.util.RawParseUtils;
@@ -93,6 +100,8 @@ public class ChangeNotes extends VersionedMetaData {
private final Map<PatchSet.Id,
Table<Account.Id, String, Optional<PatchSetApproval>>> approvals;
private final Map<Account.Id, ReviewerState> reviewers;
private final List<SubmitRecord> submitRecords;
private Change.Status status;
private Parser(Change.Id changeId, ObjectId tip, RevWalk walk) {
this.changeId = changeId;
@@ -100,6 +109,7 @@ public class ChangeNotes extends VersionedMetaData {
this.walk = walk;
approvals = Maps.newHashMap();
reviewers = Maps.newLinkedHashMap();
submitRecords = Lists.newArrayListWithExpectedSize(1);
}
private void parseAll() throws ConfigInvalidException, IOException {
@@ -127,12 +137,22 @@ public class ChangeNotes extends VersionedMetaData {
}
private void parse(RevCommit commit) throws ConfigInvalidException {
if (status == null) {
status = parseStatus(commit);
}
PatchSet.Id psId = parsePatchSetId(commit);
Account.Id accountId = parseIdent(commit);
if (submitRecords.isEmpty()) {
// Only parse the most recent set of submit records; any older ones are
// still there, but not currently used.
parseSubmitRecords(commit.getFooterLines(FOOTER_SUBMITTED_WITH));
}
for (String line : commit.getFooterLines(FOOTER_LABEL)) {
parseApproval(psId, accountId, commit, line);
}
for (ReviewerState state : ReviewerState.values()) {
for (String line : commit.getFooterLines(state.getFooterKey())) {
parseReviewer(state, line);
@@ -140,17 +160,31 @@ public class ChangeNotes extends VersionedMetaData {
}
}
private Change.Status parseStatus(RevCommit commit)
throws ConfigInvalidException {
List<String> statusLines = commit.getFooterLines(FOOTER_STATUS);
if (statusLines.isEmpty()) {
return null;
} else if (statusLines.size() > 1) {
throw expectedOneFooter(FOOTER_STATUS, statusLines);
}
Optional<Change.Status> status = Enums.getIfPresent(
Change.Status.class, statusLines.get(0).toUpperCase());
if (!status.isPresent()) {
throw invalidFooter(FOOTER_STATUS, statusLines.get(0));
}
return status.get();
}
private PatchSet.Id parsePatchSetId(RevCommit commit)
throws ConfigInvalidException {
List<String> psIdLines = commit.getFooterLines(FOOTER_PATCH_SET);
if (psIdLines.size() != 1) {
throw parseException("missing or multiple %s: %s",
FOOTER_PATCH_SET, psIdLines);
throw expectedOneFooter(FOOTER_PATCH_SET, psIdLines);
}
Integer psId = Ints.tryParse(psIdLines.get(0));
if (psId == null) {
throw parseException("invalid %s: %s",
FOOTER_PATCH_SET, psIdLines.get(0));
throw invalidFooter(FOOTER_PATCH_SET, psIdLines.get(0));
}
return new PatchSet.Id(changeId, psId);
}
@@ -199,6 +233,50 @@ public class ChangeNotes extends VersionedMetaData {
}
}
private void parseSubmitRecords(List<String> lines)
throws ConfigInvalidException {
SubmitRecord rec = null;
for (String line : lines) {
int c = line.indexOf(": ");
if (c < 0) {
rec = new SubmitRecord();
submitRecords.add(rec);
int s = line.indexOf(' ');
String statusStr = s >= 0 ? line.substring(0, s) : line;
Optional<SubmitRecord.Status> status =
Enums.getIfPresent(SubmitRecord.Status.class, statusStr);
checkFooter(status.isPresent(), FOOTER_SUBMITTED_WITH, line);
rec.status = status.get();
if (s >= 0) {
rec.errorMessage = line.substring(s);
}
} else {
checkFooter(rec != null, FOOTER_SUBMITTED_WITH, line);
SubmitRecord.Label label = new SubmitRecord.Label();
if (rec.labels == null) {
rec.labels = Lists.newArrayList();
}
rec.labels.add(label);
Optional<SubmitRecord.Label.Status> status = Enums.getIfPresent(
SubmitRecord.Label.Status.class, line.substring(0, c));
checkFooter(status.isPresent(), FOOTER_SUBMITTED_WITH, line);
label.status = status.get();
int c2 = line.indexOf(": ", c + 2);
if (c2 >= 0) {
label.label = line.substring(c + 2, c2);
PersonIdent ident =
RawParseUtils.parsePersonIdent(line.substring(c2 + 2));
checkFooter(ident != null, FOOTER_SUBMITTED_WITH, line);
label.appliedBy = parseIdent(ident);
} else {
label.label = line.substring(c + 2);
}
}
}
}
private Account.Id parseIdent(RevCommit commit)
throws ConfigInvalidException {
return parseIdent(commit.getAuthorIdent());
@@ -223,8 +301,7 @@ public class ChangeNotes extends VersionedMetaData {
throws ConfigInvalidException {
PersonIdent ident = RawParseUtils.parsePersonIdent(line);
if (ident == null) {
throw parseException(
"invalid %s: %s", state.getFooterKey().getName(), line);
throw invalidFooter(state.getFooterKey(), line);
}
Account.Id accountId = parseIdent(ident);
if (!reviewers.containsKey(accountId)) {
@@ -250,6 +327,24 @@ public class ChangeNotes extends VersionedMetaData {
return new ConfigInvalidException("Change " + changeId + ": "
+ String.format(fmt, args));
}
private ConfigInvalidException expectedOneFooter(FooterKey footer,
List<String> actual) {
return parseException("missing or multiple %s: %s",
footer.getName(), actual);
}
private ConfigInvalidException invalidFooter(FooterKey footer,
String actual) {
return parseException("invalid %s: %s", footer.getName(), actual);
}
private void checkFooter(boolean expr, FooterKey footer, String actual)
throws ConfigInvalidException {
if (!expr) {
throw invalidFooter(footer, actual);
}
}
}
private final GitRepositoryManager repoManager;
@@ -257,11 +352,12 @@ public class ChangeNotes extends VersionedMetaData {
private boolean loaded;
private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
private ImmutableSetMultimap<ReviewerState, Account.Id> reviewers;
private ImmutableList<SubmitRecord> submitRecords;
@VisibleForTesting
ChangeNotes(GitRepositoryManager repoManager, Change change) {
this.repoManager = repoManager;
this.change = change;
this.change = new Change(change);
}
// TODO(dborowitz): Wrap fewer exceptions if/when we kill gwtorm.
@@ -301,6 +397,14 @@ public class ChangeNotes extends VersionedMetaData {
return reviewers;
}
/**
* @return submit records stored during the most recent submit; only for
* changes that were actually submitted.
*/
public ImmutableList<SubmitRecord> getSubmitRecords() {
return submitRecords;
}
@Override
protected String getRefName() {
return ChangeNoteUtil.changeRefName(change.getId());
@@ -316,7 +420,12 @@ public class ChangeNotes extends VersionedMetaData {
try {
Parser parser = new Parser(change.getId(), rev, walk);
parser.parseAll();
if (parser.status != null) {
change.setStatus(parser.status);
}
approvals = parser.buildApprovals();
ImmutableSetMultimap.Builder<ReviewerState, Account.Id> reviewers =
ImmutableSetMultimap.builder();
for (Map.Entry<Account.Id, ReviewerState> e
@@ -324,6 +433,7 @@ public class ChangeNotes extends VersionedMetaData {
reviewers.put(e.getValue(), e.getKey());
}
this.reviewers = reviewers.build();
submitRecords = ImmutableList.copyOf(parser.submitRecords);
} finally {
walk.release();
}

70
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@@ -17,11 +17,15 @@ package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WITH;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Maps;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -49,6 +53,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
import java.io.IOException;
import java.util.Comparator;
import java.util.Date;
import java.util.List;
import java.util.Map;
/**
@@ -79,8 +84,10 @@ public class ChangeUpdate extends VersionedMetaData {
private final Date when;
private final Map<String, Optional<Short>> approvals;
private final Map<Account.Id, ReviewerState> reviewers;
private Change.Status status;
private String subject;
private PatchSet.Id psId;
private List<SubmitRecord> submitRecords;
@AssistedInject
private ChangeUpdate(
@@ -90,6 +97,7 @@ public class ChangeUpdate extends VersionedMetaData {
AccountCache accountCache,
MetaDataUpdate.User updateFactory,
ProjectCache projectCache,
IdentifiedUser user,
@Assisted ChangeControl ctl) {
this(serverIdent, repoManager, migration, accountCache, updateFactory,
projectCache, ctl, serverIdent.getWhen());
@@ -147,6 +155,12 @@ public class ChangeUpdate extends VersionedMetaData {
return when;
}
public void setStatus(Change.Status status) {
checkArgument(status != Change.Status.SUBMITTED,
"use submit(Iterable<PatchSetApproval>)");
this.status = status;
}
public void putApproval(String label, short value) {
approvals.put(label, Optional.of(value));
}
@@ -155,6 +169,13 @@ public class ChangeUpdate extends VersionedMetaData {
approvals.put(label, Optional.<Short> absent());
}
public void submit(Iterable<SubmitRecord> submitRecords) {
status = Change.Status.SUBMITTED;
this.submitRecords = ImmutableList.copyOf(submitRecords);
checkArgument(!this.submitRecords.isEmpty(),
"no submit records specified at submit time");
}
public void setSubject(String subject) {
this.subject = subject;
}
@@ -202,7 +223,7 @@ public class ChangeUpdate extends VersionedMetaData {
}
}
public PersonIdent newIdent(Account author) {
private PersonIdent newIdent(Account author, Date when) {
return new PersonIdent(
author.getFullName(),
author.getId().get() + "@" + GERRIT_PLACEHOLDER_HOST,
@@ -262,10 +283,10 @@ public class ChangeUpdate extends VersionedMetaData {
@Override
protected boolean onSave(CommitBuilder commit) {
if (approvals.isEmpty() && reviewers.isEmpty()) {
if (isEmpty()) {
return false;
}
commit.setAuthor(newIdent(getUser().getAccount()));
commit.setAuthor(newIdent(getUser().getAccount(), when));
commit.setCommitter(new PersonIdent(serverIdent, when));
int ps = psId != null ? psId.get() : getChange().currentPatchSetId().get();
@@ -277,9 +298,13 @@ public class ChangeUpdate extends VersionedMetaData {
}
msg.append("\n\n");
addFooter(msg, FOOTER_PATCH_SET, ps);
if (status != null) {
addFooter(msg, FOOTER_STATUS, status.name().toLowerCase());
}
for (Map.Entry<Account.Id, ReviewerState> e : reviewers.entrySet()) {
Account account = accountCache.get(e.getKey()).getAccount();
PersonIdent ident = newIdent(account);
PersonIdent ident = newIdent(account, when);
addFooter(msg, e.getValue().getFooterKey())
.append(ident.getName())
.append(" <").append(ident.getEmailAddress()).append(">\n");
@@ -293,10 +318,43 @@ public class ChangeUpdate extends VersionedMetaData {
new LabelVote(e.getKey(), e.getValue().get()).formatWithEquals());
}
}
if (submitRecords != null) {
for (SubmitRecord rec : submitRecords) {
addFooter(msg, FOOTER_SUBMITTED_WITH)
.append(rec.status);
if (rec.errorMessage != null) {
msg.append(' ').append(sanitizeFooter(rec.errorMessage));
}
msg.append('\n');
if (rec.labels != null) {
for (SubmitRecord.Label label : rec.labels) {
addFooter(msg, FOOTER_SUBMITTED_WITH)
.append(label.status).append(": ").append(label.label);
if (label.appliedBy != null) {
PersonIdent ident =
newIdent(accountCache.get(label.appliedBy).getAccount(), when);
msg.append(": ").append(ident.getName())
.append(" <").append(ident.getEmailAddress()).append('>');
}
msg.append('\n');
}
}
}
}
commit.setMessage(msg.toString());
return true;
}
private boolean isEmpty() {
return approvals.isEmpty()
&& reviewers.isEmpty()
&& status == null
&& submitRecords == null;
}
private static StringBuilder addFooter(StringBuilder sb, FooterKey footer) {
return sb.append(footer.getName()).append(": ");
}
@@ -310,6 +368,10 @@ public class ChangeUpdate extends VersionedMetaData {
sb.append('\n');
}
private static String sanitizeFooter(String value) {
return value.replace('\n', ' ').replace('\0', ' ');
}
@Override
protected void onLoad() throws IOException, ConfigInvalidException {
// Do nothing; just reads current revision.

5
gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java
View File

@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.base.Objects;
import com.google.common.base.Strings;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
/** A single vote on a label, consisting of a label name and a value. */
public class LabelVote {
@@ -64,6 +65,10 @@ public class LabelVote {
this.value = value;
}
public LabelVote(PatchSetApproval psa) {
this(psa.getLabel(), psa.getValue());
}
public String getLabel() {
return name;
}

147
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -23,10 +23,12 @@ import static org.easymock.EasyMock.expect;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Ordering;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
@@ -170,7 +172,7 @@ public class ChangeNotesTest {
}
@Test
public void approvalsCommitFormat() throws Exception {
public void approvalsCommitFormatSimple() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
update.putApproval("Verified", (short) 1);
@@ -231,6 +233,79 @@ public class ChangeNotesTest {
}
}
@Test
public void submitCommitFormat() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubject("Submit patch set 1");
update.submit(ImmutableList.of(
submitRecord("NOT_READY", null,
submitLabel("Verified", "OK", changeOwner.getAccountId()),
submitLabel("Code-Review", "NEED", null)),
submitRecord("NOT_READY", null,
submitLabel("Verified", "OK", changeOwner.getAccountId()),
submitLabel("Alternative-Code-Review", "NEED", null))));
update.commit();
RevWalk walk = new RevWalk(repo);
try {
RevCommit commit = walk.parseCommit(update.getRevision());
walk.parseBody(commit);
assertEquals("Submit patch set 1\n"
+ "\n"
+ "Patch-set: 1\n"
+ "Status: submitted\n"
+ "Submitted-with: NOT_READY\n"
+ "Submitted-with: OK: Verified: Change Owner <1@gerrit>\n"
+ "Submitted-with: NEED: Code-Review\n"
+ "Submitted-with: NOT_READY\n"
+ "Submitted-with: OK: Verified: Change Owner <1@gerrit>\n"
+ "Submitted-with: NEED: Alternative-Code-Review\n",
commit.getFullMessage());
PersonIdent author = commit.getAuthorIdent();
assertEquals("Change Owner", author.getName());
assertEquals("1@gerrit", author.getEmailAddress());
assertEquals(new Date(c.getCreatedOn().getTime() + 1000),
author.getWhen());
assertEquals(TimeZone.getTimeZone("GMT-7:00"), author.getTimeZone());
PersonIdent committer = commit.getCommitterIdent();
assertEquals("Gerrit Server", committer.getName());
assertEquals("noreply@gerrit.com", committer.getEmailAddress());
assertEquals(author.getWhen(), committer.getWhen());
assertEquals(author.getTimeZone(), committer.getTimeZone());
} finally {
walk.release();
}
}
@Test
public void submitWithErrorMessage() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubject("Submit patch set 1");
update.submit(ImmutableList.of(
submitRecord("RULE_ERROR", "Problem with patch set:\n1")));
update.commit();
RevWalk walk = new RevWalk(repo);
try {
RevCommit commit = walk.parseCommit(update.getRevision());
walk.parseBody(commit);
assertEquals("Submit patch set 1\n"
+ "\n"
+ "Patch-set: 1\n"
+ "Status: submitted\n"
+ "Submitted-with: RULE_ERROR Problem with patch set: 1\n",
commit.getFullMessage());
} finally {
walk.release();
}
}
@Test
public void approvalsOnePatchSet() throws Exception {
Change c = newChange();
@@ -451,6 +526,56 @@ public class ChangeNotesTest {
assertEquals(changeOwner.getAccount().getId(), psas.get(0).getAccountId());
}
@Test
public void submitRecords() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubject("Submit patch set 1");
update.submit(ImmutableList.of(
submitRecord("NOT_READY", null,
submitLabel("Verified", "OK", changeOwner.getAccountId()),
submitLabel("Code-Review", "NEED", null)),
submitRecord("NOT_READY", null,
submitLabel("Verified", "OK", changeOwner.getAccountId()),
submitLabel("Alternative-Code-Review", "NEED", null))));
update.commit();
ChangeNotes notes = newNotes(c);
List<SubmitRecord> recs = notes.getSubmitRecords();
assertEquals(2, recs.size());
assertEquals(submitRecord("NOT_READY", null,
submitLabel("Verified", "OK", changeOwner.getAccountId()),
submitLabel("Code-Review", "NEED", null)), recs.get(0));
assertEquals(submitRecord("NOT_READY", null,
submitLabel("Verified", "OK", changeOwner.getAccountId()),
submitLabel("Alternative-Code-Review", "NEED", null)), recs.get(1));
}
@Test
public void latestSubmitRecordsOnly() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubject("Submit patch set 1");
update.submit(ImmutableList.of(
submitRecord("OK", null,
submitLabel("Code-Review", "OK", otherUser.getAccountId()))));
update.commit();
incrementPatchSet(c);
update = newUpdate(c, changeOwner);
update.setSubject("Submit patch set 2");
update.submit(ImmutableList.of(
submitRecord("OK", null,
submitLabel("Code-Review", "OK", changeOwner.getAccountId()))));
update.commit();
ChangeNotes notes = newNotes(c);
assertEquals(submitRecord("OK", null,
submitLabel("Code-Review", "OK", changeOwner.getAccountId())),
Iterables.getOnlyElement(notes.getSubmitRecords()));
}
@Test
public void multipleUpdatesInBatch() throws Exception {
Change c = newChange();
@@ -535,4 +660,24 @@ public class ChangeNotesTest {
EasyMock.replay(ctl);
return ctl;
}
private static SubmitRecord submitRecord(String status,
String errorMessage, SubmitRecord.Label... labels) {
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.valueOf(status);
rec.errorMessage = errorMessage;
if (labels.length > 0) {
rec.labels = ImmutableList.copyOf(labels);
}
return rec;
}
private static SubmitRecord.Label submitLabel(String name, String status,
Account.Id appliedBy) {
SubmitRecord.Label label = new SubmitRecord.Label();
label.label = name;
label.status = SubmitRecord.Label.Status.valueOf(status);
label.appliedBy = appliedBy;
return label;
}
}