Merge "Merge branch 'stable-2.13'"
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 22a1f5d..50cef79 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -47,6 +47,7 @@
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ApprovalInfo;
@@ -215,9 +216,22 @@
setApiUser(user);
ReviewInput in = new ReviewInput();
- in.label("Code-Review", 0);
+ in.label("Code-Review", 1);
in.message = "Still LGTM";
revision(r).review(in);
+
+ ApprovalInfo cr =
+ gApi.changes()
+ .id(changeId)
+ .get(EnumSet.of(ListChangesOption.DETAILED_LABELS))
+ .labels
+ .get("Code-Review")
+ .all
+ .stream()
+ .filter(a -> a._accountId == user.getId().get())
+ .findFirst()
+ .get();
+ assertThat(cr.postSubmit).isTrue();
}
@Test
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
index c62839e..7db5b9e 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -285,7 +285,7 @@
+ "Gerrit-MessageType: comment\n"
+ "Gerrit-Comment-Date: "
+ timestamp
- + "\n";
+ + " \n";
}
private MailMessage.Builder messageBuilderWithDefaultFields() {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
index 04080af..3d2c3d40 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
@@ -790,7 +790,7 @@
PushOneCommit.FILE_NAME,
"new contents",
r.getChangeId())
- .to("refs/heads/master");
+ .to("refs/for/master");
r.assertOkStatus();
PatchSet.Id psId = r.getPatchSetId();
@@ -1216,6 +1216,64 @@
rebuilderWrapper.rebuild(db, id);
}
+ @Test
+ public void commitWithCrLineEndings() throws Exception {
+ PushOneCommit.Result r =
+ createChange("Subject\r\rBody\r", PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT);
+ Change c = r.getChange().change();
+
+ // This assertion demonstrates an arguable bug in JGit's commit subject
+ // parsing, and shows how this kind of data might have gotten into
+ // ReviewDb. If that bug ever gets fixed upstream, this assert may start
+ // failing. If that happens, this test can be rewritten to directly set the
+ // subject field in ReviewDb.
+ assertThat(c.getSubject()).isEqualTo("Subject\r\rBody");
+
+ checker.rebuildAndCheckChanges(c.getId());
+ }
+
+ @Test
+ public void patchSetsOutOfOrder() throws Exception {
+ String id = createChange().getChangeId();
+ amendChange(id);
+ PushOneCommit.Result r = amendChange(id);
+
+ ChangeData cd = r.getChange();
+ PatchSet.Id psId3 = cd.change().currentPatchSetId();
+ assertThat(psId3.get()).isEqualTo(3);
+
+ PatchSet ps1 = db.patchSets().get(new PatchSet.Id(cd.getId(), 1));
+ PatchSet ps3 = db.patchSets().get(psId3);
+ assertThat(ps1.getCreatedOn()).isLessThan(ps3.getCreatedOn());
+
+ // Simulate an old Gerrit bug by setting the created timestamp of the latest
+ // patch set ID to the timestamp of PS1.
+ ps3.setCreatedOn(ps1.getCreatedOn());
+ db.patchSets().update(Collections.singleton(ps3));
+
+ checker.rebuildAndCheckChanges(cd.getId());
+
+ setNotesMigration(true, true);
+ cd = changeDataFactory.create(db, project, cd.getId());
+ assertThat(cd.change().currentPatchSetId()).isEqualTo(psId3);
+
+ List<PatchSet> patchSets = ImmutableList.copyOf(cd.patchSets());
+ assertThat(patchSets).hasSize(3);
+
+ PatchSet newPs1 = patchSets.get(0);
+ assertThat(newPs1.getId()).isEqualTo(ps1.getId());
+ assertThat(newPs1.getCreatedOn()).isEqualTo(ps1.getCreatedOn());
+
+ PatchSet newPs2 = patchSets.get(1);
+ assertThat(newPs2.getCreatedOn()).isGreaterThan(newPs1.getCreatedOn());
+
+ PatchSet newPs3 = patchSets.get(2);
+ assertThat(newPs3.getId()).isEqualTo(ps3.getId());
+ // Migrated with a newer timestamp than the original, to preserve ordering.
+ assertThat(newPs3.getCreatedOn()).isAtLeast(newPs2.getCreatedOn());
+ assertThat(newPs3.getCreatedOn()).isGreaterThan(ps1.getCreatedOn());
+ }
+
private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java
index c812568..f703ed7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MetadataParser.java
@@ -23,9 +23,14 @@
import com.google.gerrit.server.mail.MetadataName;
import java.sql.Timestamp;
import java.time.Instant;
+import java.time.format.DateTimeParseException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/** Parse metadata from inbound email */
public class MetadataParser {
+ private static final Logger log = LoggerFactory.getLogger(MailProcessor.class.getName());
+
public static MailMetadata parse(MailMessage m) {
MailMetadata metadata = new MailMetadata();
// Find author
@@ -40,8 +45,12 @@
String ps = header.substring(toHeaderWithDelimiter(MetadataName.PATCH_SET).length());
metadata.patchSet = Ints.tryParse(ps);
} else if (header.startsWith(toHeaderWithDelimiter(MetadataName.TIMESTAMP))) {
- String ts = header.substring(toHeaderWithDelimiter(MetadataName.TIMESTAMP).length());
- metadata.timestamp = Timestamp.from(MailUtil.rfcDateformatter.parse(ts, Instant::from));
+ String ts = header.substring(toHeaderWithDelimiter(MetadataName.TIMESTAMP).length()).trim();
+ try {
+ metadata.timestamp = Timestamp.from(MailUtil.rfcDateformatter.parse(ts, Instant::from));
+ } catch (DateTimeParseException e) {
+ log.error("Mail: Error while parsing timestamp from header of message " + m.id(), e);
+ }
} else if (header.startsWith(toHeaderWithDelimiter(MetadataName.MESSAGE_TYPE))) {
metadata.messageType =
header.substring(toHeaderWithDelimiter(MetadataName.MESSAGE_TYPE).length());
@@ -53,18 +62,18 @@
// If the required fields were not yet found, continue to parse the text
if (!Strings.isNullOrEmpty(m.textContent())) {
- String[] lines = m.textContent().split("\n");
- extractFooters(lines, metadata);
+ String[] lines = m.textContent().replace("\r\n", "\n").split("\n");
+ extractFooters(lines, metadata, m);
if (metadata.hasRequiredFields()) {
return metadata;
}
}
// If the required fields were not yet found, continue to parse the HTML
- // HTML footer are contained inside a <p> tag
+ // HTML footer are contained inside a <div> tag
if (!Strings.isNullOrEmpty(m.htmlContent())) {
- String[] lines = m.htmlContent().split("</p>");
- extractFooters(lines, metadata);
+ String[] lines = m.htmlContent().replace("\r\n", "\n").split("</div>");
+ extractFooters(lines, metadata, m);
if (metadata.hasRequiredFields()) {
return metadata;
}
@@ -73,7 +82,7 @@
return metadata;
}
- private static void extractFooters(String[] lines, MailMetadata metadata) {
+ private static void extractFooters(String[] lines, MailMetadata metadata, MailMessage m) {
for (String line : lines) {
if (metadata.changeId == null && line.contains(MetadataName.CHANGE_ID)) {
metadata.changeId = extractFooter(toFooterWithDelimiter(MetadataName.CHANGE_ID), line);
@@ -82,7 +91,11 @@
Ints.tryParse(extractFooter(toFooterWithDelimiter(MetadataName.PATCH_SET), line));
} else if (metadata.timestamp == null && line.contains(MetadataName.TIMESTAMP)) {
String ts = extractFooter(toFooterWithDelimiter(MetadataName.TIMESTAMP), line);
- metadata.timestamp = Timestamp.from(MailUtil.rfcDateformatter.parse(ts, Instant::from));
+ try {
+ metadata.timestamp = Timestamp.from(MailUtil.rfcDateformatter.parse(ts, Instant::from));
+ } catch (DateTimeParseException e) {
+ log.error("Mail: Error while parsing timestamp from footer of message " + m.id(), e);
+ }
} else if (metadata.messageType == null && line.contains(MetadataName.MESSAGE_TYPE)) {
metadata.messageType =
extractFooter(toFooterWithDelimiter(MetadataName.MESSAGE_TYPE), line);
@@ -91,6 +104,6 @@
}
private static String extractFooter(String key, String line) {
- return line.substring(line.indexOf(key) + key.length(), line.length());
+ return line.substring(line.indexOf(key) + key.length(), line.length()).trim();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
index eaa0335..d5b1b3d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
@@ -22,6 +22,7 @@
import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering;
import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB;
import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB;
+import static java.util.stream.Collectors.toList;
import com.google.auto.value.AutoValue;
import com.google.common.base.CharMatcher;
@@ -50,6 +51,7 @@
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.server.ChangeUtil;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl;
@@ -434,21 +436,23 @@
excludeCreatedOn =
!timestampsDiffer(bundleA, bundleA.getFirstPatchSetTime(), bundleB, b.getCreatedOn());
aSubj = cleanReviewDbSubject(aSubj);
+ bSubj = cleanNoteDbSubject(bSubj);
excludeCurrentPatchSetId = !bundleA.validPatchSetPredicate().apply(a.currentPatchSetId());
excludeSubject = bSubj.startsWith(aSubj) || excludeCurrentPatchSetId;
excludeOrigSubj = true;
- String aTopic = trimLeadingOrNull(a.getTopic());
+ String aTopic = trimOrNull(a.getTopic());
excludeTopic =
Objects.equals(aTopic, b.getTopic()) || "".equals(aTopic) && b.getTopic() == null;
aUpdated = bundleA.getLatestTimestamp();
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
excludeCreatedOn =
!timestampsDiffer(bundleA, a.getCreatedOn(), bundleB, bundleB.getFirstPatchSetTime());
+ aSubj = cleanNoteDbSubject(aSubj);
bSubj = cleanReviewDbSubject(bSubj);
excludeCurrentPatchSetId = !bundleB.validPatchSetPredicate().apply(b.currentPatchSetId());
excludeSubject = aSubj.startsWith(bSubj) || excludeCurrentPatchSetId;
excludeOrigSubj = true;
- String bTopic = trimLeadingOrNull(b.getTopic());
+ String bTopic = trimOrNull(b.getTopic());
excludeTopic =
Objects.equals(bTopic, a.getTopic()) || a.getTopic() == null && "".equals(bTopic);
bUpdated = bundleB.getLatestTimestamp();
@@ -484,8 +488,8 @@
}
}
- private static String trimLeadingOrNull(String s) {
- return s != null ? CharMatcher.whitespace().trimLeadingFrom(s) : null;
+ private static String trimOrNull(String s) {
+ return s != null ? CharMatcher.whitespace().trimFrom(s) : null;
}
private static String cleanReviewDbSubject(String s) {
@@ -501,7 +505,11 @@
if (rn >= 0) {
s = s.substring(0, rn);
}
- return s;
+ return ChangeNoteUtil.sanitizeFooter(s);
+ }
+
+ private static String cleanNoteDbSubject(String s) {
+ return ChangeNoteUtil.sanitizeFooter(s);
}
/**
@@ -642,12 +650,47 @@
List<String> diffs, ChangeBundle bundleA, ChangeBundle bundleB) {
Map<PatchSet.Id, PatchSet> as = bundleA.patchSets;
Map<PatchSet.Id, PatchSet> bs = bundleB.patchSets;
- for (PatchSet.Id id : diffKeySets(diffs, as, bs)) {
+ Set<PatchSet.Id> ids = diffKeySets(diffs, as, bs);
+
+ // Old versions of Gerrit had a bug that created patch sets during
+ // rebase or submission with a createdOn timestamp earlier than the patch
+ // set it was replacing. (In the cases I examined, it was equal to createdOn
+ // for the change, but we're not counting on this exact behavior.)
+ //
+ // ChangeRebuilder ensures patch set events come out in order, but it's hard
+ // to predict what the resulting timestamps would look like. So, completely
+ // ignore the createdOn timestamps if both:
+ // * ReviewDb timestamps are non-monotonic.
+ // * NoteDb timestamps are monotonic.
+ boolean excludeCreatedOn = false;
+ if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
+ excludeCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids);
+ } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
+ excludeCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids);
+ }
+
+ for (PatchSet.Id id : ids) {
PatchSet a = as.get(id);
PatchSet b = bs.get(id);
String desc = describe(id);
String pushCertField = "pushCertificate";
- diffColumnsExcluding(diffs, PatchSet.class, desc, bundleA, a, bundleB, b, pushCertField);
+
+ boolean excludeDesc = false;
+ if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
+ excludeDesc = Objects.equals(trimOrNull(a.getDescription()), b.getDescription());
+ } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
+ excludeDesc = Objects.equals(a.getDescription(), trimOrNull(b.getDescription()));
+ }
+
+ List<String> exclude = Lists.newArrayList(pushCertField);
+ if (excludeCreatedOn) {
+ exclude.add("createdOn");
+ }
+ if (excludeDesc) {
+ exclude.add("description");
+ }
+
+ diffColumnsExcluding(diffs, PatchSet.class, desc, bundleA, a, bundleB, b, exclude);
diffValues(diffs, desc, trimPushCert(a), trimPushCert(b), pushCertField);
}
}
@@ -659,6 +702,18 @@
return CharMatcher.is('\n').trimTrailingFrom(ps.getPushCertificate());
}
+ private static boolean createdOnIsMonotonic(
+ Map<?, PatchSet> patchSets, Set<PatchSet.Id> limitToIds) {
+ List<PatchSet> orderedById =
+ patchSets
+ .values()
+ .stream()
+ .filter(ps -> limitToIds.contains(ps.getId()))
+ .sorted(ChangeUtil.PS_ID_ORDER)
+ .collect(toList());
+ return Ordering.natural().onResultOf(PatchSet::getCreatedOn).isOrdered(orderedById);
+ }
+
private static void diffPatchSetApprovals(
List<String> diffs, ChangeBundle bundleA, ChangeBundle bundleB) {
Map<PatchSetApproval.Key, PatchSetApproval> as = bundleA.filterPatchSetApprovals();
@@ -675,23 +730,43 @@
// actual submit, so the timestamps may not line up. This shouldn't really
// happen, because postSubmit shouldn't be set in ReviewDb until after the
// change is submitted in ReviewDb, but you never know.
+ //
+ // Due to a quirk of PostReview, post-submit 0 votes might not have the
+ // postSubmit bit set in ReviewDb. As these are only used for tombstone
+ // purposes, ignore the postSubmit bit in NoteDb in this case.
Timestamp ta = a.getGranted();
Timestamp tb = b.getGranted();
PatchSet psa = checkNotNull(bundleA.patchSets.get(a.getPatchSetId()));
PatchSet psb = checkNotNull(bundleB.patchSets.get(b.getPatchSetId()));
boolean excludeGranted = false;
+ boolean excludePostSubmit = false;
List<String> exclude = new ArrayList<>(1);
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
excludeGranted =
(ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn()))
|| ta.compareTo(tb) < 0;
+ excludePostSubmit = a.getValue() == 0 && b.isPostSubmit();
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
excludeGranted =
tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn()) || tb.compareTo(ta) < 0;
+ excludePostSubmit = b.getValue() == 0 && a.isPostSubmit();
}
+
+ // Legacy submit approvals may or may not have tags associated with them,
+ // depending on whether ChangeRebuilder happened to group them with the
+ // status change.
+ boolean excludeTag =
+ bundleA.source != bundleB.source && a.isLegacySubmit() && b.isLegacySubmit();
+
if (excludeGranted) {
exclude.add("granted");
}
+ if (excludePostSubmit) {
+ exclude.add("postSubmit");
+ }
+ if (excludeTag) {
+ exclude.add("tag");
+ }
diffColumnsExcluding(diffs, PatchSetApproval.class, desc, bundleA, a, bundleB, b, exclude);
}
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 425575e..86aebf1 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
@@ -20,6 +20,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.CharMatcher;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
import com.google.common.primitives.Ints;
@@ -619,4 +620,17 @@
name.append('>');
appendHeaderField(writer, header, name.toString());
}
+
+ private static final CharMatcher INVALID_FOOTER_CHARS = CharMatcher.anyOf("\r\n\0");
+
+ static String sanitizeFooter(String value) {
+ // Remove characters that would confuse JGit's footer parser if they were
+ // included in footer values, for example by splitting the footer block into
+ // multiple paragraphs.
+ //
+ // One painful example: RevCommit#getShorMessage() might return a message
+ // containing "\r\r", which RevCommit#getFooterLines() will treat as an
+ // empty paragraph for the purposes of footer parsing.
+ return INVALID_FOOTER_CHARS.trimAndCollapseFrom(value, ' ');
+ }
}
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 068f368..c241965 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
@@ -36,6 +36,7 @@
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WITH;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TAG;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.sanitizeFooter;
import static java.util.Comparator.comparing;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
@@ -647,6 +648,7 @@
for (Table.Cell<String, Account.Id, Optional<Short>> c : approvals.cellSet()) {
addFooter(msg, FOOTER_LABEL);
+ // Label names/values are safe to append without sanitizing.
if (!c.getValue().isPresent()) {
msg.append('-').append(c.getRowKey());
} else {
@@ -673,6 +675,7 @@
if (rec.labels != null) {
for (SubmitRecord.Label label : rec.labels) {
+ // Label names/values are safe to append without sanitizing.
addFooter(msg, FOOTER_SUBMITTED_WITH)
.append(label.status)
.append(": ")
@@ -764,7 +767,7 @@
private static void addFooter(StringBuilder sb, FooterKey footer, Object... values) {
addFooter(sb, footer);
for (Object value : values) {
- sb.append(value);
+ sb.append(sanitizeFooter(Objects.toString(value)));
}
sb.append('\n');
}
@@ -779,8 +782,4 @@
sb.append('>');
return sb;
}
-
- private static String sanitizeFooter(String value) {
- return value.replace('\n', ' ').replace('\0', ' ');
- }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ApprovalEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ApprovalEvent.java
index b369281..9ecf476 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ApprovalEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ApprovalEvent.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.notedb.rebuild;
+import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.server.notedb.ChangeUpdate;
import java.sql.Timestamp;
@@ -38,6 +39,14 @@
}
@Override
+ protected boolean canHaveTag() {
+ // Legacy SUBM approvals don't have a tag field set, but the corresponding
+ // ChangeMessage for merging the change does. We need to let these be in the
+ // same meta commit so the SUBM approval isn't counted as post-submit.
+ return !psa.isLegacySubmit();
+ }
+
+ @Override
void apply(ChangeUpdate update) {
checkUpdate(update);
update.putApproval(psa.getLabel(), psa.getValue());
@@ -47,4 +56,9 @@
protected boolean isPostSubmitApproval() {
return psa.isPostSubmit();
}
+
+ @Override
+ protected void addToString(ToStringHelper helper) {
+ helper.add("approval", psa);
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java
index 84858b4e..54303fc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java
@@ -14,24 +14,39 @@
package com.google.gerrit.server.notedb.rebuild;
+import com.google.common.base.MoreObjects.ToStringHelper;
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gwtorm.server.OrmException;
import java.sql.Timestamp;
+import java.util.Map;
+import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
class ChangeMessageEvent extends Event {
+ private static final ImmutableMap<Change.Status, Pattern> STATUS_PATTERNS =
+ ImmutableMap.of(
+ Change.Status.ABANDONED, Pattern.compile("^Abandoned(\n.*)*$"),
+ Change.Status.MERGED,
+ Pattern.compile(
+ "^Change has been successfully" + " (merged|cherry-picked|rebased|pushed).*$"),
+ Change.Status.NEW, Pattern.compile("^Restored(\n.*)*$"));
+
private static final Pattern TOPIC_SET_REGEXP = Pattern.compile("^Topic set to (.+)$");
private static final Pattern TOPIC_CHANGED_REGEXP =
Pattern.compile("^Topic changed from (.+) to (.+)$");
private static final Pattern TOPIC_REMOVED_REGEXP = Pattern.compile("^Topic (.+) removed$");
- private final ChangeMessage message;
+ private final Change change;
private final Change noteDbChange;
+ private final Optional<Change.Status> status;
+ private final ChangeMessage message;
- ChangeMessageEvent(ChangeMessage message, Change noteDbChange, Timestamp changeCreatedOn) {
+ ChangeMessageEvent(
+ Change change, Change noteDbChange, ChangeMessage message, Timestamp changeCreatedOn) {
super(
message.getPatchSetId(),
message.getAuthor(),
@@ -39,8 +54,10 @@
message.getWrittenOn(),
changeCreatedOn,
message.getTag());
- this.message = message;
+ this.change = change;
this.noteDbChange = noteDbChange;
+ this.message = message;
+ this.status = parseStatus(message);
}
@Override
@@ -49,10 +66,44 @@
}
@Override
+ protected boolean isSubmit() {
+ return status.isPresent() && status.get() == Change.Status.MERGED;
+ }
+
+ @Override
+ protected boolean canHaveTag() {
+ return true;
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
void apply(ChangeUpdate update) throws OrmException {
checkUpdate(update);
update.setChangeMessage(message.getMessage());
setTopic(update);
+
+ if (status.isPresent()) {
+ Change.Status s = status.get();
+ update.fixStatus(s);
+ noteDbChange.setStatus(s);
+ if (s == Change.Status.MERGED) {
+ update.setSubmissionId(change.getSubmissionId());
+ noteDbChange.setSubmissionId(change.getSubmissionId());
+ }
+ }
+ }
+
+ private static Optional<Change.Status> parseStatus(ChangeMessage message) {
+ String msg = message.getMessage();
+ if (msg == null) {
+ return Optional.empty();
+ }
+ for (Map.Entry<Change.Status, Pattern> e : STATUS_PATTERNS.entrySet()) {
+ if (e.getValue().matcher(msg).matches()) {
+ return Optional.of(e.getKey());
+ }
+ }
+ return Optional.empty();
}
private void setTopic(ChangeUpdate update) {
@@ -81,4 +132,9 @@
noteDbChange.setTopic(null);
}
}
+
+ @Override
+ protected void addToString(ToStringHelper helper) {
+ helper.add("message", message);
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
index 326ceb3..10f4dd8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
@@ -27,7 +27,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
@@ -77,11 +76,12 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
+import java.util.Iterator;
import java.util.List;
-import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
+import java.util.TreeMap;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -304,8 +304,8 @@
deleteDraftRefs(change, manager.getAllUsersRepo());
Integer minPsNum = getMinPatchSetNum(bundle);
- Map<PatchSet.Id, PatchSetEvent> patchSetEvents =
- Maps.newHashMapWithExpectedSize(bundle.getPatchSets().size());
+ TreeMap<PatchSet.Id, PatchSetEvent> patchSetEvents =
+ new TreeMap<>(ReviewDbUtil.intKeyOrdering());
for (PatchSet ps : bundle.getPatchSets()) {
PatchSetEvent pse = new PatchSetEvent(change, ps, manager.getChangeRepo().rw);
@@ -320,6 +320,7 @@
draftCommentEvents.put(c.author.getId(), e);
}
}
+ ensurePatchSetOrder(patchSetEvents);
for (PatchSetApproval psa : bundle.getPatchSetApprovals()) {
PatchSetEvent pse = patchSetEvents.get(psa.getPatchSetId());
@@ -335,17 +336,15 @@
Change noteDbChange = new Change(null, null, null, null, null);
for (ChangeMessage msg : bundle.getChangeMessages()) {
- List<Event> msgEvents = parseChangeMessage(msg, change, noteDbChange);
+ Event msgEvent = new ChangeMessageEvent(change, noteDbChange, msg, change.getCreatedOn());
if (msg.getPatchSetId() != null) {
PatchSetEvent pse = patchSetEvents.get(msg.getPatchSetId());
if (pse == null) {
continue; // Ignore events for missing patch sets.
}
- for (Event e : msgEvents) {
- e.addDep(pse);
- }
+ msgEvent.addDep(pse);
}
- events.addAll(msgEvents);
+ events.add(msgEvent);
}
sortAndFillEvents(change, noteDbChange, bundle.getPatchSets(), events, minPsNum);
@@ -373,16 +372,6 @@
}
}
- private List<Event> parseChangeMessage(ChangeMessage msg, Change change, Change noteDbChange) {
- List<Event> events = new ArrayList<>(2);
- events.add(new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()));
- Optional<StatusChangeEvent> sce = StatusChangeEvent.parseFromMessage(msg, change, noteDbChange);
- if (sce.isPresent()) {
- events.add(sce.get());
- }
- return events;
- }
-
private static Integer getMinPatchSetNum(ChangeBundle bundle) {
Integer minPsNum = null;
for (PatchSet ps : bundle.getPatchSets()) {
@@ -394,6 +383,19 @@
return minPsNum;
}
+ private static void ensurePatchSetOrder(TreeMap<PatchSet.Id, PatchSetEvent> events) {
+ if (events.isEmpty()) {
+ return;
+ }
+ Iterator<PatchSetEvent> it = events.values().iterator();
+ PatchSetEvent curr = it.next();
+ while (it.hasNext()) {
+ PatchSetEvent next = it.next();
+ next.addDep(curr);
+ curr = next;
+ }
+ }
+
private static List<Comment> getComments(
ChangeBundle bundle, String serverId, PatchLineComment.Status status, PatchSet ps) {
return bundle
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java
index 1be5598..c8a649e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.server.CommentsUtil.setCommentRevId;
+import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.PatchLineComment;
@@ -51,6 +52,11 @@
}
@Override
+ protected boolean canHaveTag() {
+ return true;
+ }
+
+ @Override
void apply(ChangeUpdate update) throws OrmException {
checkUpdate(update);
if (c.revId == null) {
@@ -58,4 +64,9 @@
}
update.putComment(PatchLineComment.Status.PUBLISHED, c);
}
+
+ @Override
+ protected void addToString(ToStringHelper helper) {
+ helper.add("message", c.message);
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java
index 3cb3eb5..914930c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.server.CommentsUtil.setCommentRevId;
+import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -61,4 +62,9 @@
}
draftUpdate.putComment(c);
}
+
+ @Override
+ protected void addToString(ToStringHelper helper) {
+ helper.add("message", c.message);
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java
index 10adbab..3957c5c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl.MAX_WINDOW_MS;
import com.google.common.base.MoreObjects;
+import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.common.collect.ComparisonChain;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -102,16 +103,26 @@
return false;
}
+ protected boolean canHaveTag() {
+ return false;
+ }
+
@Override
public String toString() {
- return MoreObjects.toStringHelper(this)
- .add("psId", psId)
- .add("effectiveUser", user)
- .add("realUser", realUser)
- .add("when", when)
- .toString();
+ ToStringHelper helper =
+ MoreObjects.toStringHelper(this)
+ .add("psId", psId)
+ .add("effectiveUser", user)
+ .add("realUser", realUser)
+ .add("when", when)
+ .add("tag", tag);
+ addToString(helper);
+ return helper.toString();
}
+ /** @param helper toString helper to add fields to */
+ protected void addToString(ToStringHelper helper) {}
+
@Override
public int compareTo(Event other) {
return ComparisonChain.start()
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventList.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventList.java
index f0faef1..773215e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventList.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventList.java
@@ -18,6 +18,7 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
+import com.google.common.collect.Lists;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
import java.sql.Timestamp;
@@ -61,9 +62,12 @@
Event last = getLast();
if (!Objects.equals(e.user, last.user)
|| !Objects.equals(e.realUser, last.realUser)
- || !e.psId.equals(last.psId)
- || !Objects.equals(e.tag, last.tag)) {
- return false; // Different patch set, author, or tag.
+ || !e.psId.equals(last.psId)) {
+ return false; // Different patch set or author.
+ }
+ if (e.canHaveTag() && canHaveTag() && !Objects.equals(e.tag, getTag())) {
+ // We should trust the tag field, and it doesn't match.
+ return false;
}
if (e.isPostSubmitApproval() && isSubmit) {
// Post-submit approvals must come after the update that submits.
@@ -132,7 +136,16 @@
}
String getTag() {
- return getLast().tag;
+ for (E e : Lists.reverse(list)) {
+ if (e.tag != null) {
+ return e.tag;
+ }
+ }
+ return null;
+ }
+
+ private boolean canHaveTag() {
+ return list.stream().anyMatch(Event::canHaveTag);
}
private E get(int i) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java
index 474a7e0..b1bd6ec 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering;
+import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.common.collect.ImmutableCollection;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -52,7 +53,7 @@
if (!Objects.equals(change.getTopic(), noteDbChange.getTopic())) {
update.setTopic(change.getTopic());
}
- if (!Objects.equals(change.getStatus(), noteDbChange.getStatus())) {
+ if (!statusMatches()) {
// TODO(dborowitz): Stamp approximate approvals at this time.
update.fixStatus(change.getStatus());
}
@@ -71,6 +72,10 @@
}
}
+ private boolean statusMatches() {
+ return Objects.equals(change.getStatus(), noteDbChange.getStatus());
+ }
+
private boolean highestNumberedPatchSetIsCurrent() {
PatchSet.Id max = patchSets.stream().map(PatchSet::getId).max(intKeyOrdering()).get();
return max.equals(change.currentPatchSetId());
@@ -80,4 +85,11 @@
protected boolean isSubmit() {
return change.getStatus() == Change.Status.MERGED;
}
+
+ @Override
+ protected void addToString(ToStringHelper helper) {
+ if (!statusMatches()) {
+ helper.add("status", change.getStatus());
+ }
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/HashtagsEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/HashtagsEvent.java
index 028abfe..4f6f6ad 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/HashtagsEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/HashtagsEvent.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.notedb.rebuild;
+import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -53,4 +54,9 @@
void apply(ChangeUpdate update) throws OrmException {
update.setHashtags(hashtags);
}
+
+ @Override
+ protected void addToString(ToStringHelper helper) {
+ helper.add("hashtags", hashtags);
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ReviewerEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ReviewerEvent.java
index 1d8fbfb..2ecf969 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ReviewerEvent.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ReviewerEvent.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.notedb.rebuild;
+import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.common.collect.Table;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -54,4 +55,9 @@
checkUpdate(update);
update.putReviewer(reviewer.getColumnKey(), reviewer.getRowKey());
}
+
+ @Override
+ protected void addToString(ToStringHelper helper) {
+ helper.add("account", reviewer.getColumnKey()).add("state", reviewer.getRowKey());
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java
deleted file mode 100644
index 025b6c8..0000000
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java
+++ /dev/null
@@ -1,103 +0,0 @@
-// 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.rebuild;
-
-import com.google.common.collect.ImmutableMap;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.ChangeMessage;
-import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gwtorm.server.OrmException;
-import java.sql.Timestamp;
-import java.util.Map;
-import java.util.Optional;
-import java.util.regex.Pattern;
-
-class StatusChangeEvent extends Event {
- private static final ImmutableMap<Change.Status, Pattern> PATTERNS =
- ImmutableMap.of(
- Change.Status.ABANDONED, Pattern.compile("^Abandoned(\n.*)*$"),
- Change.Status.MERGED,
- Pattern.compile(
- "^Change has been successfully" + " (merged|cherry-picked|rebased|pushed).*$"),
- Change.Status.NEW, Pattern.compile("^Restored(\n.*)*$"));
-
- static Optional<StatusChangeEvent> parseFromMessage(
- ChangeMessage message, Change change, Change noteDbChange) {
- String msg = message.getMessage();
- if (msg == null) {
- return Optional.empty();
- }
- for (Map.Entry<Change.Status, Pattern> e : PATTERNS.entrySet()) {
- if (e.getValue().matcher(msg).matches()) {
- return Optional.of(new StatusChangeEvent(message, change, noteDbChange, e.getKey()));
- }
- }
- return Optional.empty();
- }
-
- private final Change.Status status;
- private final Change change;
- private final Change noteDbChange;
-
- private StatusChangeEvent(
- ChangeMessage message, Change change, Change noteDbChange, Change.Status status) {
- this(
- message.getPatchSetId(),
- message.getAuthor(),
- message.getWrittenOn(),
- change,
- noteDbChange,
- message.getTag(),
- status);
- }
-
- private StatusChangeEvent(
- PatchSet.Id psId,
- Account.Id author,
- Timestamp when,
- Change change,
- Change noteDbChange,
- String tag,
- Change.Status status) {
- super(psId, author, author, when, change.getCreatedOn(), tag);
- this.change = change;
- this.noteDbChange = noteDbChange;
- this.status = status;
- }
-
- @Override
- boolean uniquePerUpdate() {
- return true;
- }
-
- @SuppressWarnings("deprecation")
- @Override
- void apply(ChangeUpdate update) throws OrmException {
- checkUpdate(update);
- update.fixStatus(status);
- noteDbChange.setStatus(status);
- if (status == Change.Status.MERGED) {
- update.setSubmissionId(change.getSubmissionId());
- noteDbChange.setSubmissionId(change.getSubmissionId());
- }
- }
-
- @Override
- protected boolean isSubmit() {
- return status == Change.Status.MERGED;
- }
-}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java
index 2c60b5d..76995d5 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/mail/receive/MetadataParserTest.java
@@ -62,11 +62,11 @@
b.subject("");
StringBuilder stringBuilder = new StringBuilder();
- stringBuilder.append(toFooterWithDelimiter(MetadataName.CHANGE_ID) + "cid" + "\n");
- stringBuilder.append(toFooterWithDelimiter(MetadataName.PATCH_SET) + "1" + "\n");
+ stringBuilder.append(toFooterWithDelimiter(MetadataName.CHANGE_ID) + "cid" + "\r\n");
+ stringBuilder.append("> " + toFooterWithDelimiter(MetadataName.PATCH_SET) + "1" + "\n");
stringBuilder.append(toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment" + "\n");
stringBuilder.append(
- toFooterWithDelimiter(MetadataName.TIMESTAMP) + "Tue, 25 Oct 2016 02:11:35 -0700" + "\n");
+ toFooterWithDelimiter(MetadataName.TIMESTAMP) + "Tue, 25 Oct 2016 02:11:35 -0700" + "\r\n");
b.textContent(stringBuilder.toString());
Address author = new Address("Diffy", "test@gerritcodereview.com");
@@ -91,15 +91,16 @@
b.subject("");
StringBuilder stringBuilder = new StringBuilder();
- stringBuilder.append("<p>" + toFooterWithDelimiter(MetadataName.CHANGE_ID) + "cid" + "</p>");
- stringBuilder.append("<p>" + toFooterWithDelimiter(MetadataName.PATCH_SET) + "1" + "</p>");
stringBuilder.append(
- "<p>" + toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment" + "</p>");
+ "<div id\"someid\">" + toFooterWithDelimiter(MetadataName.CHANGE_ID) + "cid" + "</div>");
+ stringBuilder.append("<div>" + toFooterWithDelimiter(MetadataName.PATCH_SET) + "1" + "</div>");
stringBuilder.append(
- "<p>"
+ "<div>" + toFooterWithDelimiter(MetadataName.MESSAGE_TYPE) + "comment" + "</div>");
+ stringBuilder.append(
+ "<div>"
+ toFooterWithDelimiter(MetadataName.TIMESTAMP)
+ "Tue, 25 Oct 2016 02:11:35 -0700"
- + "</p>");
+ + "</div>");
b.htmlContent(stringBuilder.toString());
Address author = new Address("Diffy", "test@gerritcodereview.com");
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
index a3f8a76..916ba9fc 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
@@ -248,6 +248,55 @@
}
@Test
+ public void diffChangesSanitizesSubjectsBeforeComparison() throws Exception {
+ Change c1 = TestChanges.newChange(new Project.NameKey("project"), new Account.Id(100));
+ c1.setCurrentPatchSet(c1.currentPatchSetId(), "Subject\r\rbody", "Original");
+ Change c2 = clone(c1);
+ c2.setCurrentPatchSet(c2.currentPatchSetId(), "Subject body", "Original");
+
+ // Both ReviewDb, exact match required
+ ChangeBundle b1 =
+ new ChangeBundle(
+ c1, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
+ ChangeBundle b2 =
+ new ChangeBundle(
+ c2, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "subject differs for Change.Id "
+ + c1.getId()
+ + ":"
+ + " {Subject\r\rbody} != {Subject body}");
+
+ // Both NoteDb, exact match required (although it should be impossible to
+ // create a NoteDb change with '\r' in the subject).
+ b1 =
+ new ChangeBundle(
+ c1, messages(), patchSets(), approvals(), comments(), reviewers(), NOTE_DB);
+ b2 =
+ new ChangeBundle(
+ c2, messages(), patchSets(), approvals(), comments(), reviewers(), NOTE_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "subject differs for Change.Id "
+ + c1.getId()
+ + ":"
+ + " {Subject\r\rbody} != {Subject body}");
+
+ // One ReviewDb, one NoteDb, '\r' is normalized to ' '.
+ b1 =
+ new ChangeBundle(
+ c1, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
+ b2 =
+ new ChangeBundle(
+ c2, messages(), patchSets(), approvals(), comments(), reviewers(), NOTE_DB);
+ assertNoDiffs(b1, b2);
+ assertNoDiffs(b2, b1);
+ }
+
+ @Test
public void diffChangesConsidersEmptyReviewDbTopicEquivalentToNullInNoteDb() throws Exception {
Change c1 = TestChanges.newChange(new Project.NameKey("project"), new Account.Id(100));
c1.setTopic("");
@@ -306,9 +355,9 @@
}
@Test
- public void diffChangesIgnoresLeadingWhitespaceInReviewDbTopics() throws Exception {
+ public void diffChangesIgnoresLeadingAndTrailingWhitespaceInReviewDbTopics() throws Exception {
Change c1 = TestChanges.newChange(new Project.NameKey("project"), new Account.Id(100));
- c1.setTopic(" abc");
+ c1.setTopic(" abc ");
Change c2 = clone(c1);
c2.setTopic("abc");
@@ -319,7 +368,7 @@
ChangeBundle b2 =
new ChangeBundle(
c2, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
- assertDiffs(b1, b2, "topic differs for Change.Id " + c1.getId() + ":" + " { abc} != {abc}");
+ assertDiffs(b1, b2, "topic differs for Change.Id " + c1.getId() + ":" + " { abc } != {abc}");
// Leading whitespace in ReviewDb topic is ignored.
b1 =
@@ -331,7 +380,7 @@
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
- // Must match except for the leading whitespace.
+ // Must match except for the leading/trailing whitespace.
Change c3 = clone(c1);
c3.setTopic("cba");
b1 =
@@ -340,7 +389,7 @@
b2 =
new ChangeBundle(
c3, messages(), patchSets(), approvals(), comments(), reviewers(), NOTE_DB);
- assertDiffs(b1, b2, "topic differs for Change.Id " + c1.getId() + ":" + " { abc} != {cba}");
+ assertDiffs(b1, b2, "topic differs for Change.Id " + c1.getId() + ":" + " { abc } != {cba}");
}
@Test
@@ -1196,6 +1245,157 @@
}
@Test
+ public void diffPatchSetsIgnoresLeadingAndTrailingWhitespaceInReviewDbDescriptions()
+ throws Exception {
+ Change c = TestChanges.newChange(project, accountId);
+
+ PatchSet ps1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
+ ps1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+ ps1.setUploader(accountId);
+ ps1.setCreatedOn(TimeUtil.nowTs());
+ ps1.setDescription(" abc ");
+ PatchSet ps2 = clone(ps1);
+ ps2.setDescription("abc");
+
+ // Both ReviewDb, exact match required.
+ ChangeBundle b1 =
+ new ChangeBundle(
+ c, messages(), patchSets(ps1), approvals(), comments(), reviewers(), REVIEW_DB);
+ ChangeBundle b2 =
+ new ChangeBundle(
+ c, messages(), patchSets(ps2), approvals(), comments(), reviewers(), REVIEW_DB);
+ assertDiffs(
+ b1, b2, "description differs for PatchSet.Id " + ps1.getId() + ":" + " { abc } != {abc}");
+
+ // Whitespace in ReviewDb description is ignored.
+ b1 =
+ new ChangeBundle(
+ c, messages(), patchSets(ps1), approvals(), comments(), reviewers(), REVIEW_DB);
+ b2 =
+ new ChangeBundle(
+ c, messages(), patchSets(ps2), approvals(), comments(), reviewers(), NOTE_DB);
+ assertNoDiffs(b1, b2);
+ assertNoDiffs(b2, b1);
+
+ // Must match except for the leading/trailing whitespace.
+ PatchSet ps3 = clone(ps1);
+ ps3.setDescription("cba");
+ b1 =
+ new ChangeBundle(
+ c, messages(), patchSets(ps1), approvals(), comments(), reviewers(), REVIEW_DB);
+ b2 =
+ new ChangeBundle(
+ c, messages(), patchSets(ps3), approvals(), comments(), reviewers(), NOTE_DB);
+ assertDiffs(
+ b1, b2, "description differs for PatchSet.Id " + ps1.getId() + ":" + " { abc } != {cba}");
+ }
+
+ @Test
+ public void diffPatchSetsIgnoresCreatedOnWhenReviewDbIsNonMonotonic() throws Exception {
+ Change c = TestChanges.newChange(project, accountId);
+
+ Timestamp beforePs1 = TimeUtil.nowTs();
+
+ PatchSet goodPs1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
+ goodPs1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+ goodPs1.setUploader(accountId);
+ goodPs1.setCreatedOn(TimeUtil.nowTs());
+ PatchSet goodPs2 = new PatchSet(new PatchSet.Id(c.getId(), 2));
+ goodPs2.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+ goodPs2.setUploader(accountId);
+ goodPs2.setCreatedOn(TimeUtil.nowTs());
+ assertThat(goodPs2.getCreatedOn()).isGreaterThan(goodPs1.getCreatedOn());
+
+ PatchSet badPs2 = clone(goodPs2);
+ badPs2.setCreatedOn(beforePs1);
+ assertThat(badPs2.getCreatedOn()).isLessThan(goodPs1.getCreatedOn());
+
+ // Both ReviewDb, exact match required.
+ ChangeBundle b1 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(goodPs1, goodPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ ChangeBundle b2 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(goodPs1, badPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "createdOn differs for PatchSet.Id "
+ + badPs2.getId()
+ + ":"
+ + " {2009-09-30 17:00:18.0} != {2009-09-30 17:00:06.0}");
+
+ // Non-monotonic in ReviewDb but monotonic in NoteDb, timestamps are
+ // ignored, including for ps1.
+ PatchSet badPs1 = clone(goodPs1);
+ badPs1.setCreatedOn(TimeUtil.nowTs());
+ b1 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(badPs1, badPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ b2 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(goodPs1, goodPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ NOTE_DB);
+ assertNoDiffs(b1, b2);
+ assertNoDiffs(b2, b1);
+
+ // Non-monotonic in NoteDb but monotonic in ReviewDb, timestamps are not
+ // ignored.
+ b1 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(goodPs1, goodPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ b2 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(badPs1, badPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ NOTE_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "createdOn differs for PatchSet.Id "
+ + badPs1.getId()
+ + " in NoteDb vs. ReviewDb:"
+ + " {2009-09-30 17:00:24.0} != {2009-09-30 17:00:12.0}",
+ "createdOn differs for PatchSet.Id "
+ + badPs2.getId()
+ + " in NoteDb vs. ReviewDb:"
+ + " {2009-09-30 17:00:06.0} != {2009-09-30 17:00:18.0}");
+ }
+
+ @Test
public void diffPatchSetApprovalKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);
int id = c.getId().get();
@@ -1349,6 +1549,62 @@
}
@Test
+ public void diffPatchSetApprovalsIgnoresPostSubmitBitOnZeroVote() throws Exception {
+ Change c = TestChanges.newChange(project, accountId);
+ c.setStatus(Change.Status.MERGED);
+ PatchSetApproval a1 =
+ new PatchSetApproval(
+ new PatchSetApproval.Key(c.currentPatchSetId(), accountId, new LabelId("Code-Review")),
+ (short) 0,
+ TimeUtil.nowTs());
+ a1.setPostSubmit(false);
+ PatchSetApproval a2 = clone(a1);
+ a2.setPostSubmit(true);
+
+ // Both are ReviewDb, exact match is required.
+ ChangeBundle b1 =
+ new ChangeBundle(
+ c, messages(), latest(c), approvals(a1), comments(), reviewers(), REVIEW_DB);
+ ChangeBundle b2 =
+ new ChangeBundle(
+ c, messages(), latest(c), approvals(a2), comments(), reviewers(), REVIEW_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "postSubmit differs for PatchSetApproval.Key "
+ + c.getId()
+ + "%2C1,100,Code-Review:"
+ + " {false} != {true}");
+
+ // One NoteDb, postSubmit is ignored.
+ b1 =
+ new ChangeBundle(
+ c, messages(), latest(c), approvals(a1), comments(), reviewers(), REVIEW_DB);
+ b2 =
+ new ChangeBundle(c, messages(), latest(c), approvals(a2), comments(), reviewers(), NOTE_DB);
+ assertNoDiffs(b1, b2);
+ assertNoDiffs(b2, b1);
+
+ // postSubmit is not ignored if vote isn't 0.
+ a1.setValue((short) 1);
+ a2.setValue((short) 1);
+ assertDiffs(
+ b1,
+ b2,
+ "postSubmit differs for PatchSetApproval.Key "
+ + c.getId()
+ + "%2C1,100,Code-Review:"
+ + " {false} != {true}");
+ assertDiffs(
+ b2,
+ b1,
+ "postSubmit differs for PatchSetApproval.Key "
+ + c.getId()
+ + "%2C1,100,Code-Review:"
+ + " {true} != {false}");
+ }
+
+ @Test
public void diffReviewers() throws Exception {
Change c = TestChanges.newChange(project, accountId);
Timestamp now = TimeUtil.nowTs();
