opendev/gerrit
Code Issues Proposed changes

Add patch set description

Browse Source 
With this change, the description field is properly populated by the %m
flag and set in ReviewDB and NoteDB, and is set in the ChangeJSON
received in a GetChange call. The test for pushing a change with a
message was also updated to check for the description field being set in
RevisionInfo.

To be included in descendant changes:
 - Handling cases where PSUtil.insert is called that aren't change
   creations or updates (cherrypick, etc)
 - Implementation of an API endpoint for setting/deleting the value
 - Docs

Feature: Issue 4544
Change-Id: I1c4d817d1d5e263b0cd42bf9ea874808aac00582
This commit is contained in:
Kasper Nilsson 2016-10-25 10:56:16 -07:00
parent aedb7fbce0
commit 1987154564
21 changed files with 182 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@ -47,6 +47,7 @@ import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@ -353,6 +354,45 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
assertThat(cm.message).isEqualTo(
"Uploaded patch set 1.\nmy test message");
}
Collection<RevisionInfo> revisions = ci.revisions.values();
assertThat(revisions).hasSize(1);
for (RevisionInfo ri : revisions) {
assertThat(ri.description).isEqualTo("my test message");
}
}
@Test
public void pushForMasterWithMessageTwiceWithDifferentMessages()
throws Exception {
ProjectConfig config = projectCache.checkedGet(project).getConfig();
config.getProject()
.setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE);
saveProjectConfig(project, config);
PushOneCommit push =
pushFactory
.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT,
"a.txt", "content");
PushOneCommit.Result r = push.to("refs/for/master/%m=my_test_message");
r.assertOkStatus();
push =
pushFactory
.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT,
"b.txt", "anotherContent", r.getChangeId());
r = push.to("refs/for/master/%m=new_test_message");
r.assertOkStatus();
ChangeInfo ci = get(r.getChangeId());
Collection<RevisionInfo> revisions = ci.revisions.values();
assertThat(revisions).hasSize(2);
for (RevisionInfo ri: revisions) {
if (ri.isCurrent) {
assertThat(ri.description).isEqualTo("new test message");
} else {
assertThat(ri.description).isEqualTo("my test message");
}
}
}
@Test

1
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RevisionInfo.java
View File

@ -33,4 +33,5 @@ public class RevisionInfo {
public Map<String, ActionInfo> actions;
public String commitWithFooters;
public PushCertificateInfo pushCertificate;
public String description;
}

19
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java
View File

@ -194,6 +194,16 @@ public final class PatchSet {
@Column(id = 8, notNull = false, length = Integer.MAX_VALUE)
protected String pushCertificate;
/**
* Optional user-supplied description for this patch set.
* <p>
* When this field is null, the description was never set on the patch set.
* When this field is an empty string, the description was set and later
* cleared.
*/
@Column(id = 9, notNull = false, length = Integer.MAX_VALUE)
protected String description;
protected PatchSet() {
}
@ -209,6 +219,7 @@ public final class PatchSet {
this.draft = src.draft;
this.groups = src.groups;
this.pushCertificate = src.pushCertificate;
this.description = src.description;
}
public PatchSet.Id getId() {
@ -277,6 +288,14 @@ public final class PatchSet {
pushCertificate = cert;
}
public String getDescription() {
return description;
}
public void setDescription(String description) {
this.description = description;
}
@Override
public String toString() {
return "[PatchSet " + getId().toString() + "]";

11
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSetInfo.java
View File

@ -54,6 +54,9 @@ public final class PatchSetInfo {
/** SHA-1 of commit */
protected String revId;
/** Optional user-supplied description for the patch set. */
protected String description;
protected PatchSetInfo() {
}
@ -116,4 +119,12 @@ public final class PatchSetInfo {
public String getRevId() {
return revId;
}
public void setDescription(String description) {
this.description = description;
}
public String getDescription() {
return description;
}
}

4
gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java
View File

@ -91,7 +91,7 @@ public class PatchSetUtil {
public PatchSet insert(ReviewDb db, RevWalk rw, ChangeUpdate update,
PatchSet.Id psId, ObjectId commit, boolean draft,
List<String> groups, String pushCertificate)
List<String> groups, String pushCertificate, String description)
throws OrmException, IOException {
checkNotNull(groups, "groups may not be null");
ensurePatchSetMatches(psId, update);
@ -103,9 +103,11 @@ public class PatchSetUtil {
ps.setDraft(draft);
ps.setGroups(groups);
ps.setPushCertificate(pushCertificate);
ps.setDescription(description);
db.patchSets().insert(Collections.singleton(ps));
update.setCommit(rw, commit, pushCertificate);
update.setPsDescription(description);
update.setGroups(groups);
if (draft) {
update.setPatchSetState(DRAFT);

9
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@ -107,6 +107,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
private Change.Status status;
private String topic;
private String message;
private String patchSetDescription;
private List<String> groups = Collections.emptyList();
private CommitValidators.Policy validatePolicy =
CommitValidators.Policy.GERRIT;
@ -219,6 +220,11 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
return this;
}
public ChangeInserter setPatchSetDescription(String patchSetDescription) {
this.patchSetDescription = patchSetDescription;
return this;
}
public ChangeInserter setValidatePolicy(CommitValidators.Policy validate) {
this.validatePolicy = checkNotNull(validate);
return this;
@ -336,6 +342,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
update.setSubjectForCommit("Create change");
update.setBranch(change.getDest().get());
update.setTopic(change.getTopic());
update.setPsDescription(patchSetDescription);
boolean draft = status == Change.Status.DRAFT;
List<String> newGroups = groups;
@ -343,7 +350,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
newGroups = GroupCollector.getDefaultGroups(commit);
}
patchSet = psUtil.insert(ctx.getDb(), ctx.getRevWalk(), update, psId,
commit, draft, newGroups, pushCert);
commit, draft, newGroups, pushCert, patchSetDescription);
/* TODO: fixStatus is used here because the tests
* (byStatusClosed() in AbstractQueryChangesTest)

1
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@ -1065,6 +1065,7 @@ public class ChangeJson {
out.draft = in.isDraft() ? true : null;
out.fetch = makeFetchMap(ctl, in);
out.kind = changeKindCache.getChangeKind(repo, cd, in);
out.description = in.getDescription();
boolean setCommit = has(ALL_COMMITS)
|| (out.isCurrent && has(CURRENT_COMMIT));

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

@ -217,7 +217,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
}
}
patchSet = psUtil.insert(db, ctx.getRevWalk(), ctx.getUpdate(psId),
psId, commit, draft, newGroups, null);
psId, commit, draft, newGroups, null, null);
if (notify != NotifyHandling.NONE) {
oldReviewers = approvalsUtil.getReviewers(db, ctl.getNotes());

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

@ -2082,7 +2082,8 @@ public class ReceiveCommits {
.setNotify(magicBranch.notify)
.setRequestScopePropagator(requestScopePropagator)
.setSendMail(true)
.setUpdateRef(false));
.setUpdateRef(false)
.setPatchSetDescription(magicBranch.message));
if (!magicBranch.hashtags.isEmpty()) {
bu.addOp(
changeId,

5
gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
View File

@ -229,9 +229,11 @@ public class ReplaceOp extends BatchUpdate.Op {
update.setSubjectForCommit("Create patch set " + patchSetId.get());
String reviewMessage = null;
String psDescription = null;
if (magicBranch != null) {
recipients.add(magicBranch.getMailRecipients());
reviewMessage = magicBranch.message;
psDescription = magicBranch.message;
approvals.putAll(magicBranch.labels);
Set<String> hashtags = magicBranch.hashtags;
if (hashtags != null && !hashtags.isEmpty()) {
@ -252,8 +254,9 @@ public class ReplaceOp extends BatchUpdate.Op {
ctx.getDb(), ctx.getRevWalk(), update, patchSetId, commit, draft, groups,
pushCertificate != null
? pushCertificate.toTextWithSignature()
: null);
: null, psDescription);
update.setPsDescription(psDescription);
recipients.add(getRecipientsFromFooters(
ctx.getDb(), accountResolver, draft, commit.getFooterLines()));
recipients.remove(ctx.getAccountId());

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

@ -146,7 +146,7 @@ public class CherryPick extends SubmitStrategy {
PatchSet newPs = args.psUtil.insert(ctx.getDb(), ctx.getRevWalk(),
ctx.getUpdate(psId), psId, newCommit, false,
prevPs != null ? prevPs.getGroups() : ImmutableList.<String> of(),
null);
null, null);
ctx.getChange().setCurrentPatchSet(patchSetInfo);
// Don't copy approvals, as this is already taken care of by

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
View File

@ -210,7 +210,7 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
newPs = args.psUtil.insert(ctx.getDb(), ctx.getRevWalk(),
ctx.getUpdate(newPatchSetId), newPatchSetId, newCommit, false,
prevPs != null ? prevPs.getGroups() : ImmutableList.<String> of(),
null);
null, null);
}
ctx.getChange().setCurrentPatchSet(args.patchSetInfoFactory
.get(ctx.getRevWalk(), newCommit, newPatchSetId));

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
View File

@ -296,7 +296,7 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
? prevPs.getGroups()
: GroupCollector.getDefaultGroups(alreadyMerged);
return args.psUtil.insert(ctx.getDb(), ctx.getRevWalk(),
ctx.getUpdate(psId), psId, alreadyMerged, false, groups, null);
ctx.getUpdate(psId), psId, alreadyMerged, false, groups, null, null);
}
private void setApproval(ChangeContext ctx, IdentifiedUser user)

2
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
View File

@ -228,7 +228,7 @@ public class ChangeBundle {
checkColumns(ChangeMessage.Key.class, 1, 2);
checkColumns(ChangeMessage.class, 1, 2, 3, 4, 5, 6, 7);
checkColumns(PatchSet.Id.class, 1, 2);
checkColumns(PatchSet.class, 1, 2, 3, 4, 5, 6, 8);
checkColumns(PatchSet.class, 1, 2, 3, 4, 5, 6, 8, 9);
checkColumns(PatchSetApproval.Key.class, 1, 2, 3);
checkColumns(PatchSetApproval.class, 1, 2, 3, 6, 7, 8);
checkColumns(PatchLineComment.Key.class, 1, 2);

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

@ -71,6 +71,8 @@ public class ChangeNoteUtil {
public static final FooterKey FOOTER_HASHTAGS = new FooterKey("Hashtags");
public static final FooterKey FOOTER_LABEL = new FooterKey("Label");
public static final FooterKey FOOTER_PATCH_SET = new FooterKey("Patch-set");
public static final FooterKey FOOTER_PATCH_SET_DESCRIPTION =
new FooterKey("Patch-set-description");
public static final FooterKey FOOTER_REAL_USER = new FooterKey("Real-user");
public static final FooterKey FOOTER_STATUS = new FooterKey("Status");
public static final FooterKey FOOTER_SUBJECT = new FooterKey("Subject");

26
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@ -22,6 +22,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_GROUPS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
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_PATCH_SET_DESCRIPTION;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REAL_USER;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBJECT;
@ -325,7 +326,6 @@ class ChangeNotesParser {
}
parseHashtags(commit);
parseAssignee(commit);
if (submissionId == null) {
@ -365,6 +365,8 @@ class ChangeNotesParser {
if (lastUpdatedOn == null || ts.after(lastUpdatedOn)) {
lastUpdatedOn = ts;
}
parseDescription(psId, commit);
}
private String parseSubmissionId(ChangeNotesCommit commit)
@ -593,6 +595,28 @@ class ChangeNotesParser {
throw invalidFooter(FOOTER_PATCH_SET, psIdLine);
}
private void parseDescription(PatchSet.Id psId, ChangeNotesCommit commit)
throws ConfigInvalidException {
List<String> descLines =
commit.getFooterLineValues(FOOTER_PATCH_SET_DESCRIPTION);
if (descLines.isEmpty()) {
return;
} else if (descLines.size() == 1) {
String desc = descLines.get(0).trim();
PatchSet ps = patchSets.get(psId);
if (ps == null) {
ps = new PatchSet(psId);
ps.setRevision(PARTIAL_PATCH_SET);
patchSets.put(psId, ps);
}
if (ps.getDescription() == null) {
ps.setDescription(desc);
}
} else {
throw expectedOneFooter(FOOTER_PATCH_SET_DESCRIPTION, descLines);
}
}
private void parseChangeMessage(PatchSet.Id psId,
Account.Id accountId, Account.Id realAccountId,
ChangeNotesCommit commit, Timestamp ts) {

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

@ -27,6 +27,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_GROUPS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
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_PATCH_SET_DESCRIPTION;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REAL_USER;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBJECT;
@ -142,6 +143,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private Iterable<String> groups;
private String pushCert;
private boolean isAllowWriteToNewtRef;
private String psDescription;
private ChangeDraftUpdate draftUpdate;
private RobotCommentUpdate robotCommentUpdate;
@ -322,6 +324,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
this.tag = tag;
}
public void setPsDescription(String psDescription) {
this.psDescription = psDescription;
}
public void putComment(PatchLineComment.Status status, Comment c) {
verifyComment(c);
createDraftUpdateIfNull();
@ -561,6 +567,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addPatchSetFooter(msg, ps);
if (psDescription != null) {
addFooter(msg, FOOTER_PATCH_SET_DESCRIPTION, psDescription);
}
if (changeId != null) {
addFooter(msg, FOOTER_CHANGE_ID, changeId);
}
@ -703,7 +713,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
&& commit == null
&& psState == null
&& groups == null
&& tag == null;
&& tag == null
&& psDescription == null;
}
ChangeDraftUpdate getDraftUpdate() {

1
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/PatchSetEvent.java
View File

@ -57,6 +57,7 @@ class PatchSetEvent extends Event {
update.setSubjectForCommit("Create patch set " + ps.getPatchSetId());
}
setRevision(update, ps);
update.setPsDescription(ps.getDescription());
List<String> groups = ps.getGroups();
if (!groups.isEmpty()) {
update.setGroups(ps.getGroups());

2
gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
View File

@ -36,7 +36,7 @@ import java.util.concurrent.TimeUnit;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_135> C = Schema_135.class;
public static final Class<Schema_136> C = Schema_136.class;
public static int getBinaryVersion() {
return guessVersion(C);

25
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_136.java Normal file
View File

@ -0,0 +1,25 @@
// Copyright (C) 2016 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.schema;
import com.google.inject.Inject;
import com.google.inject.Provider;
public class Schema_136 extends SchemaVersion {
@Inject
Schema_136(Provider<Schema_135> prior) {
super(prior);
}
}

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

@ -97,6 +97,28 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getChangeMessages().get(0).getTag()).isEqualTo(tag);
}
@Test
public void patchSetDescription() throws Exception {
String description = "descriptive";
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
update.setPsDescription(description);
update.commit();
ChangeNotes notes = newNotes(c);
assertThat(notes.getCurrentPatchSet().getDescription())
.isEqualTo(description);
description = "new, now more descriptive!";
update = newUpdate(c, changeOwner);
update.setPsDescription(description);
update.commit();
notes = newNotes(c);
assertThat(notes.getCurrentPatchSet().getDescription())
.isEqualTo(description);
}
@Test
public void tagInlineCommenrts() throws Exception {
String tag = "jenkins";