From e8618b8e55eeaa55482abf6a8c53dbb2de0744ef Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 20 Jan 2016 14:32:27 +0100 Subject: [PATCH] Store destination branch in notedb 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 --- .../server/change/ConsistencyCheckerIT.java | 18 +++++++++ .../google/gerrit/reviewdb/client/Change.java | 4 ++ .../gerrit/server/change/ChangeInserter.java | 1 + .../gerrit/server/notedb/ChangeNoteUtil.java | 1 + .../gerrit/server/notedb/ChangeNotes.java | 2 + .../server/notedb/ChangeNotesParser.java | 38 ++++++++++++++++++- .../gerrit/server/notedb/ChangeRebuilder.java | 7 +++- .../gerrit/server/notedb/ChangeUpdate.java | 12 ++++++ .../gerrit/server/change/CommentsTest.java | 8 +++- .../notedb/AbstractChangeNotesTest.java | 4 +- .../server/notedb/ChangeNotesParserTest.java | 29 ++++++++++++++ .../gerrit/server/notedb/ChangeNotesTest.java | 25 ++++++++++++ 12 files changed, 143 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index b22c3a4283..5839ae7ea0 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -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 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; } 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 9b6581a0ba..1482b11f4d 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 @@ -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(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 80fe622d2c..ad7d607959 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java @@ -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; 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 afba8adf55..672b204004 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 @@ -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"); 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 e93b09f4d9..aa5545d515 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 @@ -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 { 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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 30432a48f3..8cb5618a20 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -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 submitRecords; final Multimap comments; NoteMap commentNoteMap; + String branch; Change.Status status; String topic; Set hashtags; @@ -125,6 +131,7 @@ class ChangeNotesParser implements AutoCloseable { parseComments(); allPastReviewers.addAll(reviewers.keySet()); pruneReviewers(); + checkMandatoryFooters(); } ImmutableListMultimap @@ -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 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 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 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() { + @Override + public String apply(FooterKey input) { + return input.getName(); + } + }))); + } + } + private ConfigInvalidException expectedOneFooter(FooterKey footer, List actual) { return parseException("missing or multiple %s: %s", diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java index 22ccac72ec..15602ee4ef 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java @@ -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()); } 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 f52b279587..449c3ba682 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 @@ -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> approvals; private final Map reviewers; + private String branch; private Change.Status status; private String subject; private List 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)"); @@ -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 diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java index 052b126ec0..699dc6e0e1 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java @@ -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 { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java index 857f13a5dd..0d19ba9b7e 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java @@ -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; } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java index 14e38f14a0..1dce91c695 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java @@ -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, 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 34cc2ee729..20d55bd7e2 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 @@ -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();