opendev/gerrit
Code Issues Proposed changes

ChangeBundle: Handle leading/trailing whitespace in topic/desc

Browse Source 
PatchSet descriptions should use the same logic as Change topics to
ignore whitespace that JGit won't accurately parse out of a NoteDb
footer. While writing this, I found that topics should actually be
trimming leading as well as trailing whitespace. Use a single
trimming implementation for both of these.

Change-Id: I564fe674d9b5970717cb3e923b0aa28522c6347a
This commit is contained in:
Dave Borowitz
2017-02-06 10:58:47 -05:00
committed by Edwin Kempin
parent 4e1f02db91
commit e96bd74cf7
2 changed files with 69 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
View File

@@ -438,7 +438,7 @@ public class ChangeBundle {
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();
@@ -450,7 +450,7 @@ public class ChangeBundle {
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();
@@ -486,8 +486,8 @@ public class ChangeBundle {
}
}
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) {
@@ -653,7 +653,20 @@ public class ChangeBundle {
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 (excludeDesc) {
exclude.add("description");
}
diffColumnsExcluding(diffs, PatchSet.class, desc, bundleA, a, bundleB, b, exclude);
diffValues(diffs, desc, trimPushCert(a), trimPushCert(b), pushCertField);
}
}

56
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
View File

@@ -355,9 +355,9 @@ public class ChangeBundleTest extends GerritBaseTests {
}
@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");
@@ -368,7 +368,7 @@ public class ChangeBundleTest extends GerritBaseTests {
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 =
@@ -380,7 +380,7 @@ public class ChangeBundleTest extends GerritBaseTests {
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 =
@@ -389,7 +389,7 @@ public class ChangeBundleTest extends GerritBaseTests {
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
@@ -1244,6 +1244,52 @@ public class ChangeBundleTest extends GerritBaseTests {
"PatchSetApproval.Key sets differ:" + " [] only in A; [" + a2.getKey() + "] only in B");
}
@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 diffPatchSetApprovalKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);