ChangeNotesParser: Avoid lots of FooterLine allocations
The implementation of RevCommit#getFooterLines(String) always calls
getFooterLines(), which creates a new list filled with newly-parsed
FooterLine objects. This seems wasteful, especially considering in
ChangeNotes we call getFooterLines 10 or so times per commit.
Add a subclass of RevWalk that caches the result of getFooterLines in
a multimap. I don't claim that this is the most efficient
implementation possible, but it does give us the abstraction we need
to optimize more later.
Some tests were failing in a previous iteration of this change when I
didn't implement case-insensitive behavior. Normalize
ChangeNotesParserTest to use exactly-matching case in labels, except
for a single test method that explicitly tests case-folding behavior.
Change-Id: I836afa43cde2c1d8df04286664a3c6cc0fac57a9
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
index efd0a4e..55b8556 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
@@ -26,6 +26,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -35,7 +36,6 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
@@ -76,7 +76,7 @@
@AutoValue
public abstract static class LoadHandle implements AutoCloseable {
- public static LoadHandle create(RevWalk walk, ObjectId id) {
+ public static LoadHandle create(ChangeNotesRevWalk walk, ObjectId id) {
return new AutoValue_AbstractChangeNotes_LoadHandle(
checkNotNull(walk), id != null ? id.copy() : null);
}
@@ -85,7 +85,7 @@
return new AutoValue_AbstractChangeNotes_LoadHandle(null, null);
}
- @Nullable public abstract RevWalk walk();
+ @Nullable public abstract ChangeNotesRevWalk walk();
@Nullable public abstract ObjectId id();
@Override
@@ -145,7 +145,7 @@
}
protected LoadHandle openHandle(Repository repo) throws IOException {
- return LoadHandle.create(new RevWalk(repo), readRef(repo));
+ return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), readRef(repo));
}
public T reload() throws OrmException {
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 24256bf..b685485 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
@@ -69,7 +69,6 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -642,7 +641,8 @@
return super.openHandle(repo); // May be null in tests.
}
repo.scanForRepoChanges();
- return LoadHandle.create(new RevWalk(repo), newState.getChangeMetaId());
+ return LoadHandle.create(
+ ChangeNotesCommit.newRevWalk(repo), newState.getChangeMetaId());
} catch (NoSuchChangeException e) {
return super.openHandle(repo);
} catch (OrmException | ConfigInvalidException e) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesCommit.java
new file mode 100644
index 0000000..5d28454
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesCommit.java
@@ -0,0 +1,106 @@
+// 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.notedb;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.ListMultimap;
+
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.lib.AnyObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.FooterKey;
+import org.eclipse.jgit.revwalk.FooterLine;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+import java.io.IOException;
+import java.util.List;
+
+/**
+ * Commit implementation with some optimizations for change notes parsing.
+ * <p>
+ * <ul>
+ * <li>Caches the result of {@link #getFooterLines()}, which is
+ * otherwise very wasteful with allocations.</li>
+ * </ul>
+ */
+public class ChangeNotesCommit extends RevCommit {
+ public static ChangeNotesRevWalk newRevWalk(Repository repo) {
+ return new ChangeNotesRevWalk(repo);
+ }
+
+ public static class ChangeNotesRevWalk extends RevWalk {
+ private ChangeNotesRevWalk(Repository repo) {
+ super(repo);
+ }
+
+ @Override
+ protected ChangeNotesCommit createCommit(AnyObjectId id) {
+ return new ChangeNotesCommit(id);
+ }
+
+ @Override
+ public ChangeNotesCommit next() throws MissingObjectException,
+ IncorrectObjectTypeException, IOException {
+ return (ChangeNotesCommit) super.next();
+ }
+
+ @Override
+ public void markStart(RevCommit c) throws MissingObjectException,
+ IncorrectObjectTypeException, IOException {
+ checkArgument(c instanceof ChangeNotesCommit);
+ super.markStart(c);
+ }
+
+ @Override
+ public void markUninteresting(RevCommit c) throws MissingObjectException,
+ IncorrectObjectTypeException, IOException {
+ checkArgument(c instanceof ChangeNotesCommit);
+ super.markUninteresting(c);
+ }
+
+ @Override
+ public ChangeNotesCommit lookupCommit(AnyObjectId id) {
+ return (ChangeNotesCommit) super.lookupCommit(id);
+ }
+
+ @Override
+ public ChangeNotesCommit parseCommit(AnyObjectId id)
+ throws MissingObjectException, IncorrectObjectTypeException,
+ IOException {
+ return (ChangeNotesCommit) super.parseCommit(id);
+ }
+ }
+
+ private ListMultimap<String, String> footerLines;
+
+ public ChangeNotesCommit(AnyObjectId id) {
+ super(id);
+ }
+
+ public List<String> getFooterLineValues(FooterKey key) {
+ if (footerLines == null) {
+ List<FooterLine> src = getFooterLines();
+ footerLines = ArrayListMultimap.create(src.size(), 1);
+ for (FooterLine fl : src) {
+ footerLines.put(fl.getKey().toLowerCase(), fl.getValue());
+ }
+ }
+ return footerLines.get(key.getName().toLowerCase());
+ }
+}
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 69209b9..604c866 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
@@ -62,6 +62,7 @@
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.util.LabelVote;
import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -73,8 +74,6 @@
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.FooterKey;
-import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.util.RawParseUtils;
import java.io.IOException;
@@ -123,7 +122,7 @@
private final NoteDbMetrics metrics;
private final Change.Id id;
private final ObjectId tip;
- private final RevWalk walk;
+ private final ChangeNotesRevWalk walk;
private final Repository repo;
private final Map<PatchSet.Id,
Table<Account.Id, Entry<String, String>, Optional<PatchSetApproval>>> approvals;
@@ -131,7 +130,7 @@
private final Multimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet;
ChangeNotesParser(Project.NameKey project, Change.Id changeId, ObjectId tip,
- RevWalk walk, GitRepositoryManager repoManager,
+ ChangeNotesRevWalk walk, GitRepositoryManager repoManager,
ChangeNoteUtil noteUtil, NoteDbMetrics metrics)
throws RepositoryNotFoundException, IOException {
this.id = changeId;
@@ -163,7 +162,8 @@
walk.markStart(walk.parseCommit(tip));
try (Timer1.Context timer = metrics.parseLatency.start(CHANGES)) {
- for (RevCommit commit : walk) {
+ ChangeNotesCommit commit;
+ while ((commit = walk.next()) != null) {
parse(commit);
}
parseNotes();
@@ -203,7 +203,7 @@
return ImmutableListMultimap.copyOf(changeMessagesByPatchSet);
}
- private void parse(RevCommit commit) throws ConfigInvalidException {
+ private void parse(ChangeNotesCommit commit) throws ConfigInvalidException {
Timestamp ts =
new Timestamp(commit.getCommitterIdent().getWhen().getTime());
@@ -275,17 +275,17 @@
if (submitRecords.isEmpty()) {
// Only parse the most recent set of submit records; any older ones are
// still there, but not currently used.
- parseSubmitRecords(commit.getFooterLines(FOOTER_SUBMITTED_WITH));
+ parseSubmitRecords(commit.getFooterLineValues(FOOTER_SUBMITTED_WITH));
updateTs |= !submitRecords.isEmpty();
}
- for (String line : commit.getFooterLines(FOOTER_LABEL)) {
+ for (String line : commit.getFooterLineValues(FOOTER_LABEL)) {
parseApproval(psId, accountId, ts, line);
updateTs = true;
}
for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
- for (String line : commit.getFooterLines(state.getFooterKey())) {
+ for (String line : commit.getFooterLineValues(state.getFooterKey())) {
parseReviewer(state, line);
}
// Don't update timestamp when a reviewer was added, matching RevewDb
@@ -299,31 +299,35 @@
}
}
- private String parseSubmissionId(RevCommit commit)
+ private String parseSubmissionId(ChangeNotesCommit commit)
throws ConfigInvalidException {
return parseOneFooter(commit, FOOTER_SUBMISSION_ID);
}
- private String parseBranch(RevCommit commit) throws ConfigInvalidException {
+ private String parseBranch(ChangeNotesCommit commit)
+ throws ConfigInvalidException {
String branch = parseOneFooter(commit, FOOTER_BRANCH);
return branch != null ? RefNames.fullName(branch) : null;
}
- private String parseChangeId(RevCommit commit) throws ConfigInvalidException {
+ private String parseChangeId(ChangeNotesCommit commit)
+ throws ConfigInvalidException {
return parseOneFooter(commit, FOOTER_CHANGE_ID);
}
- private String parseSubject(RevCommit commit) throws ConfigInvalidException {
+ private String parseSubject(ChangeNotesCommit commit)
+ throws ConfigInvalidException {
return parseOneFooter(commit, FOOTER_SUBJECT);
}
- private String parseTopic(RevCommit commit) throws ConfigInvalidException {
+ private String parseTopic(ChangeNotesCommit commit)
+ throws ConfigInvalidException {
return parseOneFooter(commit, FOOTER_TOPIC);
}
- private String parseOneFooter(RevCommit commit, FooterKey footerKey)
+ private String parseOneFooter(ChangeNotesCommit commit, FooterKey footerKey)
throws ConfigInvalidException {
- List<String> footerLines = commit.getFooterLines(footerKey);
+ List<String> footerLines = commit.getFooterLineValues(footerKey);
if (footerLines.isEmpty()) {
return null;
} else if (footerLines.size() > 1) {
@@ -332,8 +336,8 @@
return footerLines.get(0);
}
- private String parseExactlyOneFooter(RevCommit commit, FooterKey footerKey)
- throws ConfigInvalidException {
+ private String parseExactlyOneFooter(ChangeNotesCommit commit,
+ FooterKey footerKey) throws ConfigInvalidException {
String line = parseOneFooter(commit, footerKey);
if (line == null) {
throw expectedOneFooter(footerKey, Collections.<String> emptyList());
@@ -341,7 +345,7 @@
return line;
}
- private ObjectId parseRevision(RevCommit commit)
+ private ObjectId parseRevision(ChangeNotesCommit commit)
throws ConfigInvalidException {
String sha = parseOneFooter(commit, FOOTER_COMMIT);
if (sha == null) {
@@ -377,7 +381,7 @@
ps.setCreatedOn(ts);
}
- private void parseGroups(PatchSet.Id psId, RevCommit commit)
+ private void parseGroups(PatchSet.Id psId, ChangeNotesCommit commit)
throws ConfigInvalidException {
String groupsStr = parseOneFooter(commit, FOOTER_GROUPS);
if (groupsStr == null) {
@@ -394,12 +398,14 @@
ps.setGroups(PatchSet.splitGroups(groupsStr));
}
- private void parseHashtags(RevCommit commit) throws ConfigInvalidException {
- // Commits are parsed in reverse order and only the last set of hashtags should be used.
+ private void parseHashtags(ChangeNotesCommit commit)
+ throws ConfigInvalidException {
+ // Commits are parsed in reverse order and only the last set of hashtags
+ // should be used.
if (hashtags != null) {
return;
}
- List<String> hashtagsLines = commit.getFooterLines(FOOTER_HASHTAGS);
+ List<String> hashtagsLines = commit.getFooterLineValues(FOOTER_HASHTAGS);
if (hashtagsLines.isEmpty()) {
return;
} else if (hashtagsLines.size() > 1) {
@@ -411,9 +417,10 @@
}
}
- private void parseTag(RevCommit commit) throws ConfigInvalidException {
+ private void parseTag(ChangeNotesCommit commit)
+ throws ConfigInvalidException {
tag = null;
- List<String> tagLines = commit.getFooterLines(FOOTER_TAG);
+ List<String> tagLines = commit.getFooterLineValues(FOOTER_TAG);
if (tagLines.isEmpty()) {
return;
} else if (tagLines.size() == 1) {
@@ -423,9 +430,9 @@
}
}
- private Change.Status parseStatus(RevCommit commit)
+ private Change.Status parseStatus(ChangeNotesCommit commit)
throws ConfigInvalidException {
- List<String> statusLines = commit.getFooterLines(FOOTER_STATUS);
+ List<String> statusLines = commit.getFooterLineValues(FOOTER_STATUS);
if (statusLines.isEmpty()) {
return null;
} else if (statusLines.size() > 1) {
@@ -439,7 +446,7 @@
return status.get();
}
- private PatchSet.Id parsePatchSetId(RevCommit commit)
+ private PatchSet.Id parsePatchSetId(ChangeNotesCommit commit)
throws ConfigInvalidException {
String psIdLine = parseExactlyOneFooter(commit, FOOTER_PATCH_SET);
int s = psIdLine.indexOf(' ');
@@ -451,7 +458,7 @@
return new PatchSet.Id(id, psId);
}
- private PatchSetState parsePatchSetState(RevCommit commit)
+ private PatchSetState parsePatchSetState(ChangeNotesCommit commit)
throws ConfigInvalidException {
String psIdLine = parseExactlyOneFooter(commit, FOOTER_PATCH_SET);
int s = psIdLine.indexOf(' ');
@@ -470,7 +477,7 @@
}
private ChangeMessage parseChangeMessage(PatchSet.Id psId,
- Account.Id accountId, RevCommit commit, Timestamp ts) {
+ Account.Id accountId, ChangeNotesCommit commit, Timestamp ts) {
byte[] raw = commit.getRawBuffer();
int size = raw.length;
Charset enc = RawParseUtils.parseEncoding(raw);
@@ -532,7 +539,7 @@
private void parseNotes()
throws IOException, ConfigInvalidException {
ObjectReader reader = walk.getObjectReader();
- RevCommit tipCommit = walk.parseCommit(tip);
+ ChangeNotesCommit tipCommit = walk.parseCommit(tip);
revisionNoteMap = RevisionNoteMap.parse(
noteUtil, id, reader, NoteMap.read(reader, tipCommit), false);
Map<RevId, RevisionNote> rns = revisionNoteMap.revisionNotes;
@@ -705,7 +712,7 @@
}
}
- private Account.Id parseIdent(RevCommit commit)
+ private Account.Id parseIdent(ChangeNotesCommit commit)
throws ConfigInvalidException {
// Check if the author name/email is the same as the committer name/email,
// i.e. was the server ident at the time this commit was made.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
index ba824a0..43f58a5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
@@ -35,7 +35,6 @@
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
@@ -169,7 +168,7 @@
}
ObjectId draftsId = newState.getDraftIds().get(author);
repo.scanForRepoChanges();
- return LoadHandle.create(new RevWalk(repo), draftsId);
+ return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId);
} catch (NoSuchChangeException e) {
return super.openHandle(repo);
} catch (OrmException | ConfigInvalidException e) {
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 dc00eaa..d4d7d19 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
@@ -18,6 +18,7 @@
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
@@ -27,19 +28,18 @@
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
public class ChangeNotesParserTest extends AbstractChangeNotesTest {
private TestRepository<InMemoryRepository> testRepo;
- private RevWalk walk;
+ private ChangeNotesRevWalk walk;
@Before
public void setUpTestRepo() throws Exception {
testRepo = new TestRepository<>(repo);
- walk = new RevWalk(repo);
+ walk = ChangeNotesCommit.newRevWalk(repo);
}
@After
@@ -53,16 +53,16 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails(writeCommit("Update change\n"
+ "\n"
- + "Patch-Set: 1\n",
+ + "Patch-set: 1\n",
new PersonIdent("Change Owner", "owner@example.com",
serverIdent.getWhen(), serverIdent.getTimeZone())));
assertParseFails(writeCommit("Update change\n"
+ "\n"
- + "Patch-Set: 1\n",
+ + "Patch-set: 1\n",
new PersonIdent("Change Owner", "x@gerrit",
serverIdent.getWhen(), serverIdent.getTimeZone())));
}
@@ -73,23 +73,23 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\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"
+ + "Patch-set: 1\n"
+ "Status: new\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Status: OOPS\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Status: NEW\n"
+ "Status: NEW\n");
}
@@ -100,23 +100,23 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
- + "Patch-Set: 1\n");
+ + "Patch-set: 1\n"
+ + "Patch-set: 1\n");
assertParseSucceeds("Update change\n"
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: x\n");
+ + "Patch-set: x\n");
}
@Test
@@ -125,7 +125,7 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Label: Label1=+1\n"
+ "Label: Label2=1\n"
+ "Label: Label3=0\n"
@@ -135,33 +135,33 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Label: -Label1\n"
+ "Label: -Label1 Other Account <2@gerrit>\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Label: Label1=X\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Label: Label1 = 1\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Label: X+Y\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Label: Label1 Other Account <2@gerrit>\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Label: -Label!1\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Label: -Label!1 Other Account <2@gerrit>\n");
}
@@ -171,7 +171,7 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Subject: This is a test change\n"
+ "Submitted-with: NOT_READY\n"
+ "Submitted-with: OK: Verified: Change Owner <1@gerrit>\n"
@@ -181,19 +181,19 @@
+ "Submitted-with: NEED: Alternative-Code-Review\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Submitted-with: OOPS\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Submitted-with: NEED: X+Y\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Submitted-with: OK: X+Y: Change Owner <1@gerrit>\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Submitted-with: OK: Code-Review: 1@gerrit\n");
}
@@ -203,12 +203,12 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Subject: This is a test change\n"
+ "Submission-id: 1-1453387607626-96fabc25");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Submission-id: 1-1453387607626-96fabc25\n"
+ "Submission-id: 1-1453387901516-5d1e2450");
}
@@ -219,13 +219,13 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Reviewer: Change Owner <1@gerrit>\n"
+ "CC: Other Account <2@gerrit>\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Reviewer: 1@gerrit\n");
}
@@ -235,19 +235,19 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\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"
+ + "Patch-set: 1\n"
+ "Topic:\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Topic: Some Topic\n"
+ "Topic: Other Topic");
}
@@ -258,17 +258,17 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\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"
+ + "Patch-set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Branch: refs/heads/stable");
}
@@ -279,11 +279,11 @@
+ "\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Subject: This is a test change\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Change-id: I159532ef4844d7c18f7f3fd37a0b275590d41b1b");
}
@@ -292,13 +292,13 @@
public void parseSubject() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
- + "Patch-Set: 1\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"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Subject: Some subject of a change\n"
+ "Subject: Some other subject\n");
}
@@ -418,21 +418,21 @@
public void parseTag() throws Exception {
assertParseSucceeds("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Change subject\n"
+ "Tag:\n");
assertParseSucceeds("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Change subject\n"
+ "Tag: jenkins\n");
assertParseFails("Update change\n"
+ "\n"
- + "Patch-Set: 1\n"
+ + "Patch-set: 1\n"
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Subject: Change subject\n"
@@ -440,6 +440,16 @@
+ "Tag: jenkins\n");
}
+ @Test
+ public void caseInsensitiveFooters() 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");
+ }
+
private RevCommit writeCommit(String body) throws Exception {
return writeCommit(body, noteUtil.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 44fa6f5..4d80866 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
@@ -46,6 +46,7 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -1020,7 +1021,7 @@
RevCommit commitWithComments = commitWithApprovals.getParent(0);
assertThat(commitWithComments).isNotNull();
- try (RevWalk rw = new RevWalk(repo)) {
+ try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
try (ChangeNotesParser notesWithComments = new ChangeNotesParser(
project, c.getId(), commitWithComments.copy(), rw, repoManager,
noteUtil, args.metrics)) {
@@ -1032,7 +1033,7 @@
}
}
- try (RevWalk rw = new RevWalk(repo)) {
+ try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
try (ChangeNotesParser notesWithApprovals = new ChangeNotesParser(project,
c.getId(), commitWithApprovals.copy(), rw, repoManager,
noteUtil, args.metrics)) {
