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();