opendev/gerrit
Code Issues Proposed changes

Store destination branch in notedb

Browse Source 
This adds a 'Branch' footer to the commit message of the
refs/changes/XX/YYYY/meta notes branch when the change is created.

Tests which manually insert new changes must be adapted to make an
initial commit to the notes branch which contains the branch info.

Change-Id: If6ffadf722ca5c48e159889aedbc1ffba27a8d5a
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-01-20 14:32:27 +01:00
parent 631c8009d7
commit e8618b8e55
12 changed files with 143 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
View File

@@ -31,7 +31,10 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.ConsistencyChecker;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.testutil.TestChanges;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -48,9 +51,18 @@ import java.util.List;
@NoHttpd
public class ConsistencyCheckerIT extends AbstractDaemonTest {
@Inject
private ChangeControl.GenericFactory changeControlFactory;
@Inject
private ChangeUpdate.Factory changeUpdateFactory;
@Inject
private Provider<ConsistencyChecker> checkerProvider;
@Inject
private IdentifiedUser.GenericFactory userFactory;
private RevCommit tip;
private Account.Id adminId;
private ConsistencyChecker checker;
@@ -570,6 +582,12 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
private Change insertChange() throws Exception {
Change c = newChange(project, adminId);
db.changes().insert(singleton(c));
ChangeUpdate u = changeUpdateFactory.create(
changeControlFactory.controlFor(c, userFactory.create(adminId)));
u.setBranch(c.getDest().get());
u.commit();
return c;
}

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

@@ -551,6 +551,10 @@ public final class Change {
return dest;
}
public void setDest(Branch.NameKey dest) {
this.dest = dest;
}
public Project.NameKey getProject() {
return dest.getParentKey();
}

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

@@ -297,6 +297,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
ChangeUpdate update = ctx.getUpdate(psId);
update.setSubject("Create change");
update.setBranch(change.getDest().get());
update.setTopic(change.getTopic());
boolean draft = status == Change.Status.DRAFT;

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

@@ -28,6 +28,7 @@ import java.util.Date;
public class ChangeNoteUtil {
static final String GERRIT_PLACEHOLDER_HOST = "gerrit";
static final FooterKey FOOTER_BRANCH = new FooterKey("Branch");
static final FooterKey FOOTER_HASHTAGS = new FooterKey("Hashtags");
static final FooterKey FOOTER_LABEL = new FooterKey("Label");
static final FooterKey FOOTER_PATCH_SET = new FooterKey("Patch-set");

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

@@ -28,6 +28,7 @@ import com.google.common.collect.Ordering;
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.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchLineComment;
@@ -265,6 +266,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
allChangeMessages = parser.buildAllMessages();
comments = ImmutableListMultimap.copyOf(parser.comments);
noteMap = parser.commentNoteMap;
change.setDest(new Branch.NameKey(getProjectName(), parser.branch));
change.setTopic(Strings.emptyToNull(parser.topic));
change.setCreatedOn(parser.createdOn);
change.setLastUpdatedOn(parser.lastUpdatedOn);

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

@@ -14,6 +14,7 @@
package com.google.gerrit.server.notedb;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
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;
@@ -24,6 +25,8 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
import com.google.common.base.Enums;
import com.google.common.base.Function;
import com.google.common.base.Joiner;
import com.google.common.base.Optional;
import com.google.common.base.Splitter;
import com.google.common.base.Supplier;
@@ -48,6 +51,7 @@ import com.google.gerrit.reviewdb.client.LabelId;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.util.LabelVote;
@@ -66,6 +70,7 @@ import org.eclipse.jgit.util.RawParseUtils;
import java.io.IOException;
import java.nio.charset.Charset;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
@@ -79,6 +84,7 @@ class ChangeNotesParser implements AutoCloseable {
final List<SubmitRecord> submitRecords;
final Multimap<RevId, PatchLineComment> comments;
NoteMap commentNoteMap;
String branch;
Change.Status status;
String topic;
Set<String> hashtags;
@@ -125,6 +131,7 @@ class ChangeNotesParser implements AutoCloseable {
parseComments();
allPastReviewers.addAll(reviewers.keySet());
pruneReviewers();
checkMandatoryFooters();
}
ImmutableListMultimap<PatchSet.Id, PatchSetApproval>
@@ -161,6 +168,9 @@ class ChangeNotesParser implements AutoCloseable {
if (lastUpdatedOn == null) {
lastUpdatedOn = getCommitTime(commit);
}
if (branch == null) {
branch = parseBranch(commit);
}
if (status == null) {
status = parseStatus(commit);
}
@@ -204,6 +214,17 @@ class ChangeNotesParser implements AutoCloseable {
return submissionIdLines.get(0);
}
private String parseBranch(RevCommit commit)
throws ConfigInvalidException {
List<String> branchLines = commit.getFooterLines(FOOTER_BRANCH);
if (branchLines.isEmpty()) {
return null;
} else if (branchLines.size() > 1) {
throw expectedOneFooter(FOOTER_BRANCH, branchLines);
}
return RefNames.fullName(branchLines.get(0));
}
private String parseTopic(RevCommit commit)
throws ConfigInvalidException {
List<String> topicLines = commit.getFooterLines(FOOTER_TOPIC);
@@ -215,7 +236,6 @@ class ChangeNotesParser implements AutoCloseable {
return topicLines.get(0);
}
private void parseHashtags(RevCommit commit) throws ConfigInvalidException {
// Commits are parsed in reverse order and only the last set of hashtags should be used.
if (hashtags != null) {
@@ -520,6 +540,22 @@ class ChangeNotesParser implements AutoCloseable {
}
}
private void checkMandatoryFooters() throws ConfigInvalidException {
List<FooterKey> missing = new ArrayList<>();
if (branch == null) {
missing.add(FOOTER_BRANCH);
}
if (!missing.isEmpty()) {
throw parseException("Missing footers: " + Joiner.on(", ")
.join(Lists.transform(missing, new Function<FooterKey, String>() {
@Override
public String apply(FooterKey input) {
return input.getName();
}
})));
}
}
private ConfigInvalidException expectedOneFooter(FooterKey footer,
List<String> actual) {
return parseException("missing or multiple %s: %s",

7
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java
View File

@@ -115,7 +115,7 @@ public class ChangeRebuilder {
ArrayListMultimap.create();
for (PatchSet ps : db.patchSets().byChange(changeId)) {
events.add(new PatchSetEvent(ps));
events.add(new PatchSetEvent(change, ps));
for (PatchLineComment c : db.patchComments().byPatchSet(ps.getId())) {
PatchLineCommentEvent e =
new PatchLineCommentEvent(c, change, ps, patchListCache);
@@ -306,10 +306,12 @@ public class ChangeRebuilder {
}
private static class PatchSetEvent extends Event {
private final Change change;
private final PatchSet ps;
PatchSetEvent(PatchSet ps) {
PatchSetEvent(Change change, PatchSet ps) {
super(ps.getId(), ps.getUploader(), ps.getCreatedOn());
this.change = change;
this.ps = ps;
}
@@ -318,6 +320,7 @@ public class ChangeRebuilder {
checkUpdate(update);
if (ps.getPatchSetId() == 1) {
update.setSubject("Create change");
update.setBranch(change.getDest().get());
} else {
update.setSubject("Create patch set " + ps.getPatchSetId());
}

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

@@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
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;
@@ -94,6 +95,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private final AccountCache accountCache;
private final Table<String, Account.Id, Optional<Short>> approvals;
private final Map<Account.Id, ReviewerStateInternal> reviewers;
private String branch;
private Change.Status status;
private String subject;
private List<SubmitRecord> submitRecords;
@@ -179,6 +181,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
this.comments = Lists.newArrayList();
}
public void setBranch(String branch) {
this.branch = branch;
}
public void setStatus(Change.Status status) {
checkArgument(status != Change.Status.MERGED,
"use merge(Iterable<SubmitRecord>)");
@@ -443,6 +449,11 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addFooter(msg, FOOTER_PATCH_SET, ps);
if (branch != null) {
addFooter(msg, FOOTER_BRANCH, branch);
}
if (status != null) {
addFooter(msg, FOOTER_STATUS, status.name().toLowerCase());
}
@@ -519,6 +530,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
&& changeMessage == null
&& comments.isEmpty()
&& reviewers.isEmpty()
&& branch == null
&& status == null
&& subject == null
&& submissionId == null

8
gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java
View File

@@ -319,8 +319,12 @@ public class CommentsTest extends GerritServerTests {
repoManager, migration, c, allUsers, changeOwner);
}
private Change newChange() {
return TestChanges.newChange(project, changeOwner.getAccountId());
private Change newChange() throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
ChangeUpdate u = newUpdate(c, changeOwner);
u.setBranch(c.getDest().get());
u.commit();
return c;
}
private ChangeUpdate newUpdate(Change c, final IdentifiedUser user) throws Exception {

4
gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
View File

@@ -165,7 +165,9 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
protected Change newChange()
throws IOException, OrmException, ConfigInvalidException {
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
newUpdate(c, changeOwner).commit();
ChangeUpdate u = newUpdate(c, changeOwner);
u.setBranch(c.getDest().get());
u.commit();
return c;
}

29
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
View File

@@ -50,6 +50,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
public void parseAuthor() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n");
assertParseFails(writeCommit("Update change\n"
+ "\n"
@@ -67,10 +68,12 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
public void parseStatus() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Status: NEW\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Status: new\n");
assertParseFails("Update change\n"
@@ -88,6 +91,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
public void parsePatchSetId() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n");
assertParseFails("Update change\n"
+ "\n");
@@ -97,6 +101,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "Patch-Set: 1\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n");
assertParseFails("Update change\n"
+ "\n"
@@ -107,6 +112,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
public void parseApproval() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Label: Label1=+1\n"
+ "Label: Label2=1\n"
@@ -114,6 +120,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "Label: Label4=-1\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Label: -Label1\n"
+ "Label: -Label1 Other Account <2@gerrit>\n");
@@ -147,6 +154,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
public void parseSubmitRecords() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Submitted-with: NOT_READY\n"
+ "Submitted-with: OK: Verified: Change Owner <1@gerrit>\n"
@@ -176,6 +184,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
public void parseSubmissionId() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Submission-id: 1-1453387607626-96fabc25");
assertParseFails("Update change\n"
@@ -189,6 +198,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
public void parseReviewer() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Reviewer: Change Owner <1@gerrit>\n"
+ "CC: Other Account <2@gerrit>\n");
@@ -202,10 +212,12 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
public void parseTopic() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Topic: Some Topic");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1\n"
+ "Topic:");
assertParseFails("Update change\n"
@@ -215,6 +227,23 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "Topic: Other Topic");
}
@Test
public void parseBranch() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Patch-Set: 1");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: master\n"
+ "Patch-Set: 1");
assertParseFails("Update change\n"
+ "\n"
+ "Patch-Set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Branch: refs/heads/stable");
}
private RevCommit writeCommit(String body) throws Exception {
return writeCommit(body, ChangeNoteUtil.newIdent(
changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent,

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

@@ -32,6 +32,7 @@ import com.google.common.collect.Ordering;
import com.google.gerrit.common.TimeUtil;
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;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.CommentRange;
@@ -486,6 +487,30 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getChange().getTopic()).isNull();
}
@Test
public void branchChangeNotes() throws Exception {
Change c = newChange();
ChangeNotes notes = newNotes(c);
Branch.NameKey expectedBranch =
new Branch.NameKey(project, "refs/heads/master");
assertThat(notes.getChange().getDest()).isEqualTo(expectedBranch);
// An update doesn't affect the branch
ChangeUpdate update = newUpdate(c, changeOwner);
update.setTopic("topic"); // Change something to get a new commit.
update.commit();
assertThat(newNotes(c).getChange().getDest()).isEqualTo(expectedBranch);
// Set another branch
String otherBranch = "refs/heads/stable";
update = newUpdate(c, changeOwner);
update.setBranch(otherBranch);
update.commit();
assertThat(newNotes(c).getChange().getDest()).isEqualTo(
new Branch.NameKey(project, otherBranch));
}
@Test
public void ownerChangeNotes() throws Exception {
Change c = newChange();