Record submit records in notedb at submit time
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
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
index 8adba94..86f77db 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
@@ -400,6 +400,23 @@
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;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
index b2265a7..e09dfc0 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
@@ -18,15 +18,19 @@
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.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.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 @@
}
}
+ 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 @@
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 @@
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 @@
}), 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 @@
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 @@
// 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 @@
rsrc.getChange().getProject().get()));
}
}
+ throw new IllegalStateException();
}
private static Optional<SubmitRecord> findOkRecord(Collection<SubmitRecord> in) {
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 e602ccb..ed4f9ab 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
@@ -54,6 +54,7 @@
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 @@
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.");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
index 83119de..e8012f2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
@@ -183,9 +183,6 @@
: 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());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
index c31c92e1..7a6ce7a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
@@ -94,9 +94,6 @@
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())) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
index 7051b76..7204735 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
@@ -24,6 +24,9 @@
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();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 90cbea6..286bf4ad 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -16,21 +16,27 @@
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.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 @@
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 @@
this.walk = walk;
approvals = Maps.newHashMap();
reviewers = Maps.newLinkedHashMap();
+ submitRecords = Lists.newArrayListWithExpectedSize(1);
}
private void parseAll() throws ConfigInvalidException, IOException {
@@ -127,12 +137,22 @@
}
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 @@
}
}
+ 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 @@
}
}
+ 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
reviewers.put(e.getValue(), e.getKey());
}
this.reviewers = reviewers.build();
+ submitRecords = ImmutableList.copyOf(parser.submitRecords);
} finally {
walk.release();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index 8a6f128..74ca198 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -17,11 +17,15 @@
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 java.io.IOException;
import java.util.Comparator;
import java.util.Date;
+import java.util.List;
import java.util.Map;
/**
@@ -79,8 +84,10 @@
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 @@
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 @@
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 @@
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 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 @@
@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 @@
}
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 @@
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 @@
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.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java
index 52f528f..13bc766 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java
@@ -19,6 +19,7 @@
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 @@
this.value = value;
}
+ public LabelVote(PatchSetApproval psa) {
+ this(psa.getLabel(), psa.getValue());
+ }
+
public String getLabel() {
return name;
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 8d268f9..f2ad057 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -23,10 +23,12 @@
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 @@
}
@Test
- public void approvalsCommitFormat() throws Exception {
+ public void approvalsCommitFormatSimple() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
update.putApproval("Verified", (short) 1);
@@ -232,6 +234,79 @@
}
@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();
ChangeUpdate update = newUpdate(c, changeOwner);
@@ -452,6 +527,56 @@
}
@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();
ChangeUpdate update1 = newUpdate(c, changeOwner);
@@ -535,4 +660,24 @@
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;
+ }
}
