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 efd0a4e020..55b855638a 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.client.Project; 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.errors.ConfigInvalidException; 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 @@ public abstract class AbstractChangeNotes { @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 @@ public abstract class AbstractChangeNotes { 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 @@ public abstract class AbstractChangeNotes { } 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 24256bf2b3..b68548535b 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.errors.ConfigInvalidException; 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 @@ public class ChangeNotes extends AbstractChangeNotes { 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 0000000000..5d2845415b --- /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. + *

+ *

    + *
  • Caches the result of {@link #getFooterLines()}, which is + * otherwise very wasteful with allocations.
    • + *
+ */ +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 footerLines; + + public ChangeNotesCommit(AnyObjectId id) { + super(id); + } + + public List getFooterLineValues(FooterKey key) { + if (footerLines == null) { + List 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 69209b9f2b..604c866caf 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.RefNames; 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.PersonIdent; 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 @@ class ChangeNotesParser implements AutoCloseable { 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, Optional>> approvals; @@ -131,7 +130,7 @@ class ChangeNotesParser implements AutoCloseable { private final Multimap 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 @@ class ChangeNotesParser implements AutoCloseable { 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 @@ class ChangeNotesParser implements AutoCloseable { 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 @@ class ChangeNotesParser implements AutoCloseable { 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 @@ class ChangeNotesParser implements AutoCloseable { } } - 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 footerLines = commit.getFooterLines(footerKey); + List footerLines = commit.getFooterLineValues(footerKey); if (footerLines.isEmpty()) { return null; } else if (footerLines.size() > 1) { @@ -332,8 +336,8 @@ class ChangeNotesParser implements AutoCloseable { 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. emptyList()); @@ -341,7 +345,7 @@ class ChangeNotesParser implements AutoCloseable { 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 @@ class ChangeNotesParser implements AutoCloseable { 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 @@ class ChangeNotesParser implements AutoCloseable { 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 hashtagsLines = commit.getFooterLines(FOOTER_HASHTAGS); + List hashtagsLines = commit.getFooterLineValues(FOOTER_HASHTAGS); if (hashtagsLines.isEmpty()) { return; } else if (hashtagsLines.size() > 1) { @@ -411,9 +417,10 @@ class ChangeNotesParser implements AutoCloseable { } } - private void parseTag(RevCommit commit) throws ConfigInvalidException { + private void parseTag(ChangeNotesCommit commit) + throws ConfigInvalidException { tag = null; - List tagLines = commit.getFooterLines(FOOTER_TAG); + List tagLines = commit.getFooterLineValues(FOOTER_TAG); if (tagLines.isEmpty()) { return; } else if (tagLines.size() == 1) { @@ -423,9 +430,9 @@ class ChangeNotesParser implements AutoCloseable { } } - private Change.Status parseStatus(RevCommit commit) + private Change.Status parseStatus(ChangeNotesCommit commit) throws ConfigInvalidException { - List statusLines = commit.getFooterLines(FOOTER_STATUS); + List statusLines = commit.getFooterLineValues(FOOTER_STATUS); if (statusLines.isEmpty()) { return null; } else if (statusLines.size() > 1) { @@ -439,7 +446,7 @@ class ChangeNotesParser implements AutoCloseable { 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 @@ class ChangeNotesParser implements AutoCloseable { 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 @@ class ChangeNotesParser implements AutoCloseable { } 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 @@ class ChangeNotesParser implements AutoCloseable { 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 rns = revisionNoteMap.revisionNotes; @@ -705,7 +712,7 @@ class ChangeNotesParser implements AutoCloseable { } } - 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 ba824a020e..43f58a53e2 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.ObjectReader; 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 @@ public class DraftCommentNotes extends AbstractChangeNotes { } 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 dc00eaa547..d4d7d190ac 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 static org.junit.Assert.fail; 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.ObjectId; 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 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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "\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 class ChangeNotesParserTest extends AbstractChangeNotesTest { 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 class ChangeNotesParserTest extends AbstractChangeNotesTest { 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 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "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 44fa6f54bb..4d80866275 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.PatchLineComment.Status; 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 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { 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 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { } } - 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)) {