opendev/gerrit
Code Issues Proposed changes

Store Change-Id in notedb

Browse Source 
This adds a 'Change-id' 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 so that the
initial commit to the notes branch contains the Change-Id info.

Change-Id: I4a8fde3da5458afab69b39230a6db117fbef8ab9
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-01-27 13:59:42 +01:00
parent dc1c9efd84
commit f2693acdaa
13 changed files with 104 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -589,6 +589,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
ChangeUpdate u = changeUpdateFactory.create(
changeControlFactory.controlFor(c, userFactory.create(adminId)));
u.setBranch(c.getDest().get());
u.setChangeId(c.getKey().get());
u.commit();
return c;

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

@@ -296,6 +296,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
ctx.getChange().setCurrentPatchSet(patchSetInfo);
ChangeUpdate update = ctx.getUpdate(psId);
update.setChangeId(change.getKey().get());
update.setSubjectForCommit("Create change");
update.setBranch(change.getDest().get());
update.setTopic(change.getTopic());

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

@@ -30,6 +30,7 @@ public class ChangeNoteUtil {
static final String GERRIT_PLACEHOLDER_HOST = "gerrit";
static final FooterKey FOOTER_BRANCH = new FooterKey("Branch");
static final FooterKey FOOTER_CHANGE_ID = new FooterKey("Change-id");
static final FooterKey FOOTER_COMMIT = new FooterKey("Commit");
static final FooterKey FOOTER_GROUPS = new FooterKey("Groups");
static final FooterKey FOOTER_HASHTAGS = new FooterKey("Hashtags");

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

@@ -290,6 +290,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
comments = ImmutableListMultimap.copyOf(parser.comments);
noteMap = parser.noteMap;
revisionNotes = parser.revisionNotes;
change.setKey(new Change.Key(parser.changeId));
change.setDest(new Branch.NameKey(getProjectName(), parser.branch));
change.setTopic(Strings.emptyToNull(parser.topic));
change.setCreatedOn(parser.createdOn);

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

@@ -15,6 +15,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_CHANGE_ID;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_COMMIT;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_GROUPS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
@@ -108,13 +109,14 @@ class ChangeNotesParser implements AutoCloseable {
Timestamp createdOn;
Timestamp lastUpdatedOn;
Account.Id ownerId;
String changeId;
String subject;
String originalSubject;
String submissionId;
PatchSet.Id currentPatchSetId;
NoteMap noteMap;
private final Change.Id changeId;
private final Change.Id id;
private final ObjectId tip;
private final RevWalk walk;
private final Repository repo;
@@ -126,7 +128,7 @@ class ChangeNotesParser implements AutoCloseable {
ChangeNotesParser(Change change, ObjectId tip, RevWalk walk,
GitRepositoryManager repoManager)
throws RepositoryNotFoundException, IOException {
this.changeId = change.getId();
this.id = change.getId();
this.tip = tip;
this.walk = walk;
this.repo = repoManager.openMetadataRepository(change.getProject());
@@ -216,6 +218,10 @@ class ChangeNotesParser implements AutoCloseable {
Account.Id accountId = parseIdent(commit);
ownerId = accountId;
if (changeId == null) {
changeId = parseChangeId(commit);
}
String currSubject = parseSubject(commit);
if (currSubject != null) {
if (subject == null) {
@@ -267,6 +273,10 @@ class ChangeNotesParser implements AutoCloseable {
return branch != null ? RefNames.fullName(branch) : null;
}
private String parseChangeId(RevCommit commit) throws ConfigInvalidException {
return parseOneFooter(commit, FOOTER_CHANGE_ID);
}
private String parseSubject(RevCommit commit) throws ConfigInvalidException {
return parseOneFooter(commit, FOOTER_SUBJECT);
}
@@ -386,7 +396,7 @@ class ChangeNotesParser implements AutoCloseable {
if (psId == null) {
throw invalidFooter(FOOTER_PATCH_SET, psIdStr);
}
return new PatchSet.Id(changeId, psId);
return new PatchSet.Id(id, psId);
}
private PatchSetState parsePatchSetState(RevCommit commit)
@@ -472,7 +482,7 @@ class ChangeNotesParser implements AutoCloseable {
noteMap = NoteMap.read(reader, tipCommit);
for (Note note : noteMap) {
RevisionNote rn = new RevisionNote(changeId, reader, note.getData());
RevisionNote rn = new RevisionNote(id, reader, note.getData());
RevId revId = new RevId(note.name());
revisionNotes.put(revId, rn);
for (PatchLineComment plc : rn.comments) {
@@ -743,6 +753,9 @@ class ChangeNotesParser implements AutoCloseable {
if (branch == null) {
missing.add(FOOTER_BRANCH);
}
if (changeId == null) {
missing.add(FOOTER_CHANGE_ID);
}
if (originalSubject == null || subject == null) {
missing.add(FOOTER_SUBJECT);
}
@@ -776,6 +789,6 @@ class ChangeNotesParser implements AutoCloseable {
}
private ConfigInvalidException parseException(String fmt, Object... args) {
return ChangeNotes.parseException(changeId, fmt, args);
return ChangeNotes.parseException(id, fmt, args);
}
}

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

@@ -401,11 +401,12 @@ public class ChangeRebuilder {
}
@Override
void apply(ChangeUpdate update) throws IOException {
void apply(ChangeUpdate update) throws IOException, OrmException {
checkUpdate(update);
update.setSubject(change.getSubject());
if (ps.getPatchSetId() == 1) {
update.setSubjectForCommit("Create change");
update.setChangeId(change.getKey().get());
update.setBranch(change.getDest().get());
} else {
update.setSubjectForCommit("Create patch set " + ps.getPatchSetId());

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

@@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_CHANGE_ID;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_COMMIT;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_GROUPS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
@@ -101,6 +102,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private String subject;
private final Table<String, Account.Id, Optional<Short>> approvals;
private final Map<Account.Id, ReviewerStateInternal> reviewers;
private String changeId;
private String branch;
private Change.Status status;
private List<SubmitRecord> submitRecords;
@@ -190,6 +192,16 @@ public class ChangeUpdate extends AbstractChangeUpdate {
this.comments = Lists.newArrayList();
}
public void setChangeId(String changeId) throws OrmException {
if (notes == null) {
notes = getChangeNotes().load();
}
checkArgument(notes.getChange().getKey().get().equals(changeId),
"The Change-Id was already set to %s, so we cannot set this Change-Id: %s",
notes.getChange().getKey(), changeId);
this.changeId = changeId;
}
public void setBranch(String branch) {
this.branch = branch;
}
@@ -501,6 +513,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addPatchSetFooter(msg, ps);
if (changeId != null) {
addFooter(msg, FOOTER_CHANGE_ID, changeId);
}
if (subject != null) {
addFooter(msg, FOOTER_SUBJECT, subject);
}
@@ -604,6 +620,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
&& changeMessage == null
&& comments.isEmpty()
&& reviewers.isEmpty()
&& changeId == null
&& branch == null
&& status == null
&& submissionId == null

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

@@ -322,6 +322,7 @@ public class CommentsTest extends GerritServerTests {
private Change newChange() throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
ChangeUpdate u = newUpdate(c, changeOwner);
u.setChangeId(c.getKey().get());
u.setBranch(c.getDest().get());
u.commit();
return c;

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

@@ -169,6 +169,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
protected Change newChange() throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
ChangeUpdate u = newUpdate(c, changeOwner);
u.setChangeId(c.getKey().get());
u.setBranch(c.getDest().get());
u.commit();
return c;

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

@@ -51,6 +51,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails(writeCommit("Update change\n"
@@ -70,12 +71,14 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Status: NEW\n"
+ "Subject: This is a test change\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Status: new\n"
+ "Subject: This is a test change\n");
@@ -95,6 +98,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
@@ -106,6 +110,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
@@ -118,6 +123,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Label: Label1=+1\n"
+ "Label: Label2=1\n"
@@ -127,6 +133,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Label: -Label1\n"
+ "Label: -Label1 Other Account <2@gerrit>\n"
@@ -162,6 +169,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Subject: This is a test change\n"
+ "Submitted-with: NOT_READY\n"
@@ -193,6 +201,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Subject: This is a test change\n"
+ "Submission-id: 1-1453387607626-96fabc25");
@@ -208,6 +217,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Reviewer: Change Owner <1@gerrit>\n"
+ "CC: Other Account <2@gerrit>\n"
@@ -223,12 +233,14 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Topic: Some Topic\n"
+ "Subject: This is a test change\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Topic:\n"
+ "Subject: This is a test change\n");
@@ -244,11 +256,13 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Subject: This is a test change\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
@@ -258,12 +272,28 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "Branch: refs/heads/stable");
}
@Test
public void parseChangeId() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-Set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n"
+ "Patch-Set: 1\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Change-id: I159532ef4844d7c18f7f3fd37a0b275590d41b1b");
}
@Test
public void parseSubject() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
+ "Patch-Set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Some subject of a change\n");
assertParseFails("Update change\n"
+ "\n"
@@ -278,6 +308,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "\n"
+ "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Some subject of a change\n"
+ "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234");
assertParseFails("Update change\n"
@@ -301,16 +332,19 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "\n"
+ "Patch-set: 1 (PUBLISHED)\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Some subject of a change\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Patch-set: 1 (DRAFT)\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Some subject of a change\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Patch-set: 1 (DELETED)\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Some subject of a change\n");
assertParseFails("Update change\n"
+ "\n"
@@ -325,6 +359,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "\n"
+ "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+ "Subject: Change subject\n"
+ "Groups: a,b,c\n");

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

@@ -491,6 +491,28 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getChange().getTopic()).isNull();
}
@Test
public void changeIdChangeNotes() throws Exception {
Change c = newChange();
ChangeNotes notes = newNotes(c);
assertThat(notes.getChange().getKey()).isEqualTo(c.getKey());
// An update doesn't affect the Change-Id
ChangeUpdate update = newUpdate(c, changeOwner);
update.setTopic("topic"); // Change something to get a new commit.
update.commit();
assertThat(notes.getChange().getKey()).isEqualTo(c.getKey());
// Trying to set another Change-Id fails
String otherChangeId = "I577fb248e474018276351785930358ec0450e9f7";
update = newUpdate(c, changeOwner);
exception.expect(IllegalArgumentException.class);
exception.expectMessage("The Change-Id was already set to " + c.getKey()
+ ", so we cannot set this Change-Id: " + otherChangeId);
update.setChangeId(otherChangeId);
}
@Test
public void branchChangeNotes() throws Exception {
Change c = newChange();

3
gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
View File

@@ -49,6 +49,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
assertBodyEquals("Update patch set 1\n"
+ "\n"
+ "Patch-set: 1\n"
+ "Change-id: " + c.getKey().get() + "\n"
+ "Subject: Change subject\n"
+ "Branch: refs/heads/master\n"
+ "Commit: " + update.getCommit().name() + "\n"
@@ -88,6 +89,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
+ "How about a new line\n"
+ "\n"
+ "Patch-set: 1\n"
+ "Change-id: " + c.getKey().get() + "\n"
+ "Subject: Change subject\n"
+ "Branch: refs/heads/master\n"
+ "Commit: " + update.getCommit().name() + "\n",
@@ -109,6 +111,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
+ "Foo\n"
+ "\n"
+ "Patch-set: 1\n"
+ "Change-id: " + c.getKey().get() + "\n"
+ "Subject: Subject\n"
+ "Branch: refs/heads/master\n"
+ "Commit: " + commit.name() + "\n",

1
gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java
View File

@@ -125,6 +125,7 @@ public class TestChanges {
cb.parent(tr.getRevWalk().parseCommit(parent.getObjectId()));
}
update.setBranch(c.getDest().get());
update.setChangeId(c.getKey().get());
update.setCommit(tr.getRevWalk(), cb.create());
return update;
}