Merge changes from topic "notedb-pack-inserter" into stable-2.15
* changes:
NoteDbMigrator: Use new pack-based inserter implementation
Upgrade JGit to 4.9.0.201710071750-r.8-g678c99c05
NoteDbMigrator: Use one NoteDbUpdateManager per project
LockFailureException: Expose ref names
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
index 0f921f3..689c5b7 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
@@ -244,7 +244,8 @@
"Update git submodules\n\n"
+ "* Update "
+ name("subscribed-to-project")
- + " from branch 'master'\n to " + subHEAD.getName());
+ + " from branch 'master'\n to "
+ + subHEAD.getName());
// The next commit should generate only its commit message,
// omitting previous commit logs
@@ -256,7 +257,8 @@
"Update git submodules\n\n"
+ "* Update "
+ name("subscribed-to-project")
- + " from branch 'master'\n to " + subHEAD.getName()
+ + " from branch 'master'\n to "
+ + subHEAD.getName()
+ "\n - "
+ subCommitMsg.getShortMessage());
}
@@ -280,7 +282,8 @@
"Update git submodules\n\n"
+ "* Update "
+ name("subscribed-to-project")
- + " from branch 'master'\n to " + subHEAD.getName());
+ + " from branch 'master'\n to "
+ + subHEAD.getName());
// The next commit should generate only its commit message,
// omitting previous commit logs
@@ -292,7 +295,8 @@
"Update git submodules\n\n"
+ "* Update "
+ name("subscribed-to-project")
- + " from branch 'master'\n to " + subHEAD.getName()
+ + " from branch 'master'\n to "
+ + subHEAD.getName()
+ "\n - "
+ subCommitMsg.getFullMessage().replace("\n", "\n "));
}
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 0324ffa..7e71cd7 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
@@ -868,6 +868,45 @@
}
@Test
+ public void allTimestampsExceptUpdatedAreEqualDueToBadMigration() throws Exception {
+ // https://bugs.chromium.org/p/gerrit/issues/detail?id=7397
+ PushOneCommit.Result r = createChange();
+ Change c = r.getChange().change();
+ Change.Id id = c.getId();
+ Timestamp ts = TimeUtil.nowTs();
+ Timestamp origUpdated = c.getLastUpdatedOn();
+
+ c.setCreatedOn(ts);
+ assertThat(c.getCreatedOn()).isGreaterThan(c.getLastUpdatedOn());
+ db.changes().update(Collections.singleton(c));
+
+ List<ChangeMessage> cm = db.changeMessages().byChange(id).toList();
+ cm.forEach(m -> m.setWrittenOn(ts));
+ db.changeMessages().update(cm);
+
+ List<PatchSet> ps = db.patchSets().byChange(id).toList();
+ ps.forEach(p -> p.setCreatedOn(ts));
+ db.patchSets().update(ps);
+
+ List<PatchSetApproval> psa = db.patchSetApprovals().byChange(id).toList();
+ psa.forEach(p -> p.setGranted(ts));
+ db.patchSetApprovals().update(psa);
+
+ List<PatchLineComment> plc = db.patchComments().byChange(id).toList();
+ plc.forEach(p -> p.setWrittenOn(ts));
+ db.patchComments().update(plc);
+
+ checker.rebuildAndCheckChanges(id);
+
+ setNotesMigration(true, true);
+ ChangeNotes notes = notesFactory.create(db, project, id);
+ assertThat(notes.getChange().getCreatedOn()).isEqualTo(origUpdated);
+ assertThat(notes.getChange().getLastUpdatedOn()).isAtLeast(origUpdated);
+ assertThat(notes.getPatchSets().get(new PatchSet.Id(id, 1)).getCreatedOn())
+ .isEqualTo(origUpdated);
+ }
+
+ @Test
public void createWithAutoRebuildingDisabled() throws Exception {
ReviewDb oldDb = db;
setNotesMigration(true, true);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetReadyForReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetReadyForReview.java
index eb61d3c..3d258c3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetReadyForReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetReadyForReview.java
@@ -75,7 +75,7 @@
@Override
public Description getDescription(ChangeResource rsrc) {
return new Description()
- .setLabel("Ready")
+ .setLabel("Start Review")
.setTitle("Set Ready For Review")
.setVisible(
rsrc.isUserOwner()
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 200e0d6..a9663c7 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
@@ -68,6 +68,7 @@
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 java.util.TreeSet;
@@ -387,6 +388,8 @@
boolean excludeCreatedOn = false;
boolean excludeCurrentPatchSetId = false;
boolean excludeTopic = false;
+ Timestamp aCreated = a.getCreatedOn();
+ Timestamp bCreated = b.getCreatedOn();
Timestamp aUpdated = a.getLastUpdatedOn();
Timestamp bUpdated = b.getLastUpdatedOn();
@@ -397,8 +400,10 @@
String aSubj = Strings.nullToEmpty(a.getSubject());
String bSubj = Strings.nullToEmpty(b.getSubject());
- // Allow created timestamp in NoteDb to be either the created timestamp of
- // the change, or the timestamp of the first remaining patch set.
+ // Allow created timestamp in NoteDb to be any of:
+ // - The created timestamp of the change.
+ // - The timestamp of the first remaining patch set.
+ // - The last updated timestamp, if it is less than the created timestamp.
//
// Ignore subject if the NoteDb subject starts with the ReviewDb subject.
// The NoteDb subject is read directly from the commit, whereas the ReviewDb
@@ -434,8 +439,14 @@
//
// Use max timestamp of all ReviewDb entities when comparing with NoteDb.
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
+ boolean createdOnMatchesFirstPs =
+ !timestampsDiffer(bundleA, bundleA.getFirstPatchSetTime(), bundleB, bCreated);
+ boolean createdOnMatchesLastUpdatedOn =
+ !timestampsDiffer(bundleA, aUpdated, bundleB, bCreated);
+ boolean createdAfterUpdated = aCreated.compareTo(aUpdated) > 0;
excludeCreatedOn =
- !timestampsDiffer(bundleA, bundleA.getFirstPatchSetTime(), bundleB, b.getCreatedOn());
+ createdOnMatchesFirstPs || (createdAfterUpdated && createdOnMatchesLastUpdatedOn);
+
aSubj = cleanReviewDbSubject(aSubj);
bSubj = cleanNoteDbSubject(bSubj);
excludeCurrentPatchSetId = !bundleA.validPatchSetPredicate().apply(a.currentPatchSetId());
@@ -446,8 +457,14 @@
Objects.equals(aTopic, b.getTopic()) || "".equals(aTopic) && b.getTopic() == null;
aUpdated = bundleA.getLatestTimestamp();
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
+ boolean createdOnMatchesFirstPs =
+ !timestampsDiffer(bundleA, aCreated, bundleB, bundleB.getFirstPatchSetTime());
+ boolean createdOnMatchesLastUpdatedOn =
+ !timestampsDiffer(bundleA, aCreated, bundleB, bUpdated);
+ boolean createdAfterUpdated = bCreated.compareTo(bUpdated) > 0;
excludeCreatedOn =
- !timestampsDiffer(bundleA, a.getCreatedOn(), bundleB, bundleB.getFirstPatchSetTime());
+ createdOnMatchesFirstPs || (createdAfterUpdated && createdOnMatchesLastUpdatedOn);
+
aSubj = cleanNoteDbSubject(aSubj);
bSubj = cleanReviewDbSubject(bSubj);
excludeCurrentPatchSetId = !bundleB.validPatchSetPredicate().apply(b.currentPatchSetId());
@@ -651,6 +668,8 @@
List<String> diffs, ChangeBundle bundleA, ChangeBundle bundleB) {
Map<PatchSet.Id, PatchSet> as = bundleA.patchSets;
Map<PatchSet.Id, PatchSet> bs = bundleB.patchSets;
+ Optional<PatchSet.Id> minA = as.keySet().stream().min(intKeyOrdering());
+ Optional<PatchSet.Id> minB = bs.keySet().stream().min(intKeyOrdering());
Set<PatchSet.Id> ids = diffKeySets(diffs, as, bs);
// Old versions of Gerrit had a bug that created patch sets during
@@ -663,11 +682,14 @@
// ignore the createdOn timestamps if both:
// * ReviewDb timestamps are non-monotonic.
// * NoteDb timestamps are monotonic.
- boolean excludeCreatedOn = false;
+ //
+ // Allow the timestamp of the first patch set to match the creation time of
+ // the change.
+ boolean excludeAllCreatedOn = false;
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
- excludeCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids);
+ excludeAllCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids);
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
- excludeCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids);
+ excludeAllCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids);
}
for (PatchSet.Id id : ids) {
@@ -676,11 +698,16 @@
String desc = describe(id);
String pushCertField = "pushCertificate";
+ boolean excludeCreatedOn = excludeAllCreatedOn;
boolean excludeDesc = false;
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
excludeDesc = Objects.equals(trimOrNull(a.getDescription()), b.getDescription());
+ excludeCreatedOn |=
+ Optional.of(id).equals(minB) && b.getCreatedOn().equals(bundleB.change.getCreatedOn());
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
excludeDesc = Objects.equals(a.getDescription(), trimOrNull(b.getDescription()));
+ excludeCreatedOn |=
+ Optional.of(id).equals(minA) && a.getCreatedOn().equals(bundleA.change.getCreatedOn());
}
List<String> exclude = Lists.newArrayList(pushCertField);
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 a28d9c5..f96e96c 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
@@ -305,6 +305,13 @@
if (bundle.getPatchSets().isEmpty()) {
throw new NoPatchSetsException(change.getId());
}
+ if (change.getLastUpdatedOn().compareTo(change.getCreatedOn()) < 0) {
+ // A bug in data migration might set created_on to the time of the migration. The
+ // correct timestamps were lost, but we can at least set it so created_on is not after
+ // last_updated_on.
+ // See https://bugs.chromium.org/p/gerrit/issues/detail?id=7397
+ change.setCreatedOn(change.getLastUpdatedOn());
+ }
// We will rebuild all events, except for draft comments, in buckets based on author and
// timestamp.
@@ -434,9 +441,11 @@
new EventSorter(events).sort();
// Ensure the first event in the list creates the change, setting the author and any required
- // footers.
+ // footers. Also force the creation time of the first patch set to match the creation time of
+ // the change.
Event first = events.get(0);
if (first instanceof PatchSetEvent && change.getOwner().equals(first.user)) {
+ first.when = change.getCreatedOn();
((PatchSetEvent) first).createChange = true;
} else {
events.add(0, new CreateChangeEvent(change, minPsNum));
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 90e6800..80a8ab9 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
@@ -667,6 +667,39 @@
}
@Test
+ public void diffChangesAllowsCreatedToMatchLastUpdated() throws Exception {
+ Change c1 = TestChanges.newChange(new Project.NameKey("project"), new Account.Id(100));
+ c1.setCreatedOn(TimeUtil.nowTs());
+ assertThat(c1.getCreatedOn()).isGreaterThan(c1.getLastUpdatedOn());
+ Change c2 = clone(c1);
+ c2.setCreatedOn(c2.getLastUpdatedOn());
+
+ // Both ReviewDb.
+ 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,
+ "createdOn differs for Change.Id "
+ + c1.getId()
+ + ": {2009-09-30 17:00:06.0} != {2009-09-30 17:00:00.0}");
+
+ // One NoteDb.
+ 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 diffChangeMessageKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);
int id = c.getId().get();
@@ -1393,6 +1426,90 @@
}
@Test
+ public void diffPatchSetsAllowsFirstPatchSetCreatedOnToMatchChangeCreatedOn() {
+ Change c = TestChanges.newChange(project, accountId);
+ c.setLastUpdatedOn(TimeUtil.nowTs());
+
+ PatchSet goodPs1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
+ goodPs1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+ goodPs1.setUploader(accountId);
+ goodPs1.setCreatedOn(TimeUtil.nowTs());
+ assertThat(goodPs1.getCreatedOn()).isGreaterThan(c.getCreatedOn());
+
+ PatchSet ps1AtCreatedOn = clone(goodPs1);
+ ps1AtCreatedOn.setCreatedOn(c.getCreatedOn());
+
+ PatchSet goodPs2 = new PatchSet(new PatchSet.Id(c.getId(), 2));
+ goodPs2.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+ goodPs2.setUploader(accountId);
+ goodPs2.setCreatedOn(TimeUtil.nowTs());
+
+ PatchSet ps2AtCreatedOn = clone(goodPs2);
+ ps2AtCreatedOn.setCreatedOn(c.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(ps1AtCreatedOn, ps2AtCreatedOn),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "createdOn differs for PatchSet.Id "
+ + c.getId()
+ + ",1: {2009-09-30 17:00:12.0} != {2009-09-30 17:00:00.0}",
+ "createdOn differs for PatchSet.Id "
+ + c.getId()
+ + ",2: {2009-09-30 17:00:18.0} != {2009-09-30 17:00:00.0}");
+
+ // One ReviewDb, PS1 is allowed to match change createdOn, but PS2 isn't.
+ b1 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(goodPs1, goodPs2),
+ approvals(),
+ comments(),
+ reviewers(),
+ REVIEW_DB);
+ b2 =
+ new ChangeBundle(
+ c,
+ messages(),
+ patchSets(ps1AtCreatedOn, ps2AtCreatedOn),
+ approvals(),
+ comments(),
+ reviewers(),
+ NOTE_DB);
+ assertDiffs(
+ b1,
+ b2,
+ "createdOn differs for PatchSet.Id "
+ + c.getId()
+ + ",2 in NoteDb vs. ReviewDb: {2009-09-30 17:00:00.0} != {2009-09-30 17:00:18.0}");
+ assertDiffs(
+ b2,
+ b1,
+ "createdOn differs for PatchSet.Id "
+ + c.getId()
+ + ",2 in NoteDb vs. ReviewDb: {2009-09-30 17:00:00.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();
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.html b/polygerrit-ui/app/elements/change/gr-message/gr-message.html
index 845bdaf..e04eeb7 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.html
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.html
@@ -126,13 +126,13 @@
top: 10px;
}
.replyContainer {
- padding: .5em 0 1em;
+ padding: .5em 0 0 0;
}
.positiveVote {
- box-shadow: inset 0 4.4em #d4ffd4;
+ box-shadow: inset 0 3.8em #d4ffd4;
}
.negativeVote {
- box-shadow: inset 0 4.4em #ffd4d4;
+ box-shadow: inset 0 3.8em #ffd4d4;
}
gr-account-label {
--gr-account-label-text-style: {
@@ -156,9 +156,13 @@
<div class="content">
<div class="message hideOnOpen">[[message.message]]</div>
<gr-formatted-text
+ no-trailing-margin
class="message hideOnCollapsed"
content="[[message.message]]"
config="[[_commentLinks]]"></gr-formatted-text>
+ <div class="replyContainer" hidden$="[[!showReplyButton]]" hidden>
+ <gr-button link small on-tap="_handleReplyTap">Reply</gr-button>
+ </div>
<gr-comment-list
comments="[[comments]]"
change-num="[[changeNum]]"
@@ -196,9 +200,6 @@
</a>
</template>
</div>
- <div class="replyContainer" hidden$="[[!showReplyButton]]" hidden>
- <gr-button small on-tap="_handleReplyTap">Reply</gr-button>
- </div>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
</template>
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.js b/polygerrit-ui/app/elements/change/gr-message/gr-message.js
index 5c49c1c..d907f3b 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.js
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.js
@@ -131,7 +131,8 @@
},
_computeShowReplyButton(message, loggedIn) {
- return !!message.message && loggedIn;
+ return !!message.message && loggedIn &&
+ !this._computeIsAutomated(message);
},
_computeExpanded(expanded) {
@@ -163,7 +164,7 @@
_computeIsAutomated(message) {
return !!(message.reviewer ||
- message.type === 'REVIEWER_UPDATE' ||
+ this._computeIsReviewerUpdate(message) ||
(message.tag && message.tag.startsWith('autogenerated')));
},
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.html b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.html
index 28a9e0e..1ef5a0c 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.html
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.html
@@ -28,7 +28,7 @@
ul,
blockquote,
gr-linked-text.pre {
- margin: 0 0 1.4em 0;
+ margin: 0 0 .8em 0;
}
p,
ul,
