From c1d16336ba17aae4efa4aef97512c182d648ca74 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 28 Nov 2019 15:52:04 +0900 Subject: [PATCH 01/15] Upgrade gitiles-servlet and blame-cache to 0.3-7 Includes the following changes: 2e5f39b - Bump version to 0.3-7 2ab0f9f - Bump bazel version to 1.2.0 219a919 - Format Java files with google-java-format e5a8b41 - Correct license for VisibilityChecker c16518b - Correct license header for VisibilityCacheTest 9ca395d - VisibilityChecker: Remove topoSort argument f0b1806 - VisibilityCache: Use jgit's ReachabilityChecker 284a35a - Accept first-parent parameter in LogServlet 11af49e - Upgrade JGit to 5.5.1.201910021850-r beb12ac - Check BLOB content size before trying to render it Notably this includes beb12ac which is the fix for [1], which is a JVM crash when a large XML document is rendered. [1] https://github.com/google/gitiles/issues/192 Change-Id: If837e84500d00b05659c1bbbbe39ce9c5879a4d8 --- WORKSPACE | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 7fd8964685..3e948c10e3 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -723,7 +723,7 @@ maven_jar( sha1 = "f7be08ec23c21485b9b5a1cf1654c2ec8c58168d", ) -GITILES_VERS = "0.3-6" +GITILES_VERS = "0.3-7" GITILES_REPO = GERRIT @@ -732,14 +732,14 @@ maven_jar( artifact = "com.google.gitiles:blame-cache:" + GITILES_VERS, attach_source = False, repository = GITILES_REPO, - sha1 = "bd1ec86570b8a6e4b68c5af6311c8cd10aa3f295", + sha1 = "af6212a62363906c63d367f8276ae1645f83bf93", ) maven_jar( name = "gitiles-servlet", artifact = "com.google.gitiles:gitiles-servlet:" + GITILES_VERS, repository = GITILES_REPO, - sha1 = "98bf06ca9abc871beb3d6c01e6f053243d4e911a", + sha1 = "6a53f722f8572a2f1bcb7d86e5692168844bab76", ) # prettify must match the version used in Gitiles From 8083ea4fc653e665ec2ebfe0013e8234c1d3748e Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 2 Dec 2019 08:16:40 +0900 Subject: [PATCH 02/15] Fix typos: Aggreements -> Agreements Change-Id: I49cb451cbc509f673ac5d596d14ca8deb4a06d30 --- .../settings/gr-cla-view/gr-cla-view.html | 4 ++-- .../elements/settings/gr-cla-view/gr-cla-view.js | 6 +++--- .../settings/gr-cla-view/gr-cla-view_test.html | 16 ++++++++-------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.html b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.html index d5f1dc3a8a..4a939e6433 100644 --- a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.html +++ b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.html @@ -77,10 +77,10 @@ limitations under the License. data-name$="[[item.name]]" data-url$="[[item.url]]" on-tap="_handleShowAgreement" - disabled$="[[_disableAggreements(item, _groups, _signedAgreements)]]"> + disabled$="[[_disableAgreements(item, _groups, _signedAgreements)]]"> -
+
Agreement already submitted.
diff --git a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js index c7713322fa..c9b404d2ca 100644 --- a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js +++ b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js @@ -107,7 +107,7 @@ return agreements ? 'show' : ''; }, - _disableAggreements(item, groups, signedAgreements) { + _disableAgreements(item, groups, signedAgreements) { for (const group of groups) { if ((item && item.auto_verify_group && item.auto_verify_group.id === group.id) || @@ -118,8 +118,8 @@ return false; }, - _hideAggreements(item, groups, signedAgreements) { - return this._disableAggreements(item, groups, signedAgreements) ? + _hideAgreements(item, groups, signedAgreements) { + return this._disableAgreements(item, groups, signedAgreements) ? '' : 'hide'; }, diff --git a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view_test.html b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view_test.html index 2304d15205..2bc0b10d65 100644 --- a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view_test.html +++ b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view_test.html @@ -140,28 +140,28 @@ limitations under the License. 'none'); }); - test('_disableAggreements', () => { + test('_disableAgreements', () => { // In the auto verify group and have not yet signed agreement assert.isTrue( - element._disableAggreements(auth, groups, signedAgreements)); + element._disableAgreements(auth, groups, signedAgreements)); // Not in the auto verify group and have not yet signed agreement assert.isFalse( - element._disableAggreements(auth2, groups, signedAgreements)); + element._disableAgreements(auth2, groups, signedAgreements)); // Not in the auto verify group, have signed agreement assert.isTrue( - element._disableAggreements(auth3, groups, signedAgreements)); + element._disableAgreements(auth3, groups, signedAgreements)); }); - test('_hideAggreements', () => { + test('_hideAgreements', () => { // Not in the auto verify group and have not yet signed agreement assert.equal( - element._hideAggreements(auth, groups, signedAgreements), ''); + element._hideAgreements(auth, groups, signedAgreements), ''); // In the auto verify group assert.equal( - element._hideAggreements(auth2, groups, signedAgreements), 'hide'); + element._hideAgreements(auth2, groups, signedAgreements), 'hide'); // Not in the auto verify group, have signed agreement assert.equal( - element._hideAggreements(auth3, groups, signedAgreements), ''); + element._hideAgreements(auth3, groups, signedAgreements), ''); }); test('_disableAgreementsText', () => { From fa1e1f1a5d75eac65eb7a398c0ed6cba60648e17 Mon Sep 17 00:00:00 2001 From: Paladox none Date: Sat, 30 Nov 2019 23:46:02 +0000 Subject: [PATCH 03/15] Fix "TypeError: groups is not iterable" in _disableAgreements This fixes an issue by adding a undefined check to _disableAgreements in addition to that we also add the check to _computeHideAgreementClass too. Bug: Issue 12020 Change-Id: Icf444aa315d24b34b5bf1cd62173ae62f6b84931 --- polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js | 2 ++ .../app/elements/settings/gr-cla-view/gr-cla-view_test.html | 3 +++ 2 files changed, 5 insertions(+) diff --git a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js index c9b404d2ca..493b1794cf 100644 --- a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js +++ b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view.js @@ -108,6 +108,7 @@ }, _disableAgreements(item, groups, signedAgreements) { + if (!groups) return false; for (const group of groups) { if ((item && item.auto_verify_group && item.auto_verify_group.id === group.id) || @@ -131,6 +132,7 @@ // if specified it returns 'hideAgreementsTextBox' which // then hides the text box and submit button. _computeHideAgreementClass(name, config) { + if (!config) return ''; for (const key in config) { if (!config.hasOwnProperty(key)) { continue; } for (const prop in config[key]) { diff --git a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view_test.html b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view_test.html index 2bc0b10d65..aec52cb8f2 100644 --- a/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view_test.html +++ b/polygerrit-ui/app/elements/settings/gr-cla-view/gr-cla-view_test.html @@ -150,6 +150,9 @@ limitations under the License. // Not in the auto verify group, have signed agreement assert.isTrue( element._disableAgreements(auth3, groups, signedAgreements)); + // Make sure the undefined check works + assert.isFalse( + element._disableAgreements(auth, undefined, signedAgreements)); }); test('_hideAgreements', () => { From a41d4db58abfec96ccc4837a34a7cf15cc0d9dda Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 2 Dec 2019 16:12:49 +0900 Subject: [PATCH 04/15] Update git submodules * Update plugins/hooks from branch 'stable-2.16' to 57c8e6261016e1cc233ea41f19fa5bdf187df8a7 - Fix and clarify documentation of ref-update and commit-received hooks - Clarify behavior of ref-update when multiple commits are pushed, i.e. that it is only called once for the entire ref update. - Fix the documentation of commit-received which states that it is called for changes pushed for review. In fact it is also called for direct pushes, but unlike ref-update it is called once for each commit. Change-Id: Ie3e2dcab37eeb5d0ed06e05322e72d3fc73925fd - Correct documentation of ref-update hook regarding output The output of the hook is only returned when the update is rejected, not regardless of the return code as stated in the doc. Change-Id: I93483f44e31e7055c302c0758084de868d1c4f93 --- plugins/hooks | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hooks b/plugins/hooks index e9127800c4..57c8e62610 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit e9127800c4efe8ad9601c7b1152783cd12a55bc2 +Subproject commit 57c8e6261016e1cc233ea41f19fa5bdf187df8a7 From be294ac2e18629a4827f6c229b184cdcab97495a Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 2 Dec 2019 17:04:52 +0900 Subject: [PATCH 05/15] Update git submodules * Update plugins/hooks from branch 'stable-3.0' to 072ee210f38f4c1bdc55330e88dc844b4daad6ca - Merge branch 'stable-2.16' into stable-3.0 * stable-2.16: Fix and clarify documentation of ref-update and commit-received hooks Correct documentation of ref-update hook regarding output Change-Id: Ia446c95e1f6ca12e1cbd198d3a7e4f6a16b09fc6 - Fix and clarify documentation of ref-update and commit-received hooks - Clarify behavior of ref-update when multiple commits are pushed, i.e. that it is only called once for the entire ref update. - Fix the documentation of commit-received which states that it is called for changes pushed for review. In fact it is also called for direct pushes, but unlike ref-update it is called once for each commit. Change-Id: Ie3e2dcab37eeb5d0ed06e05322e72d3fc73925fd - Correct documentation of ref-update hook regarding output The output of the hook is only returned when the update is rejected, not regardless of the return code as stated in the doc. Change-Id: I93483f44e31e7055c302c0758084de868d1c4f93 --- plugins/hooks | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hooks b/plugins/hooks index d0976393a5..072ee210f3 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit d0976393a5d8eea9384631bfa189b1f6518e9128 +Subproject commit 072ee210f38f4c1bdc55330e88dc844b4daad6ca From e7d69c9cceba5aaeee5c14e3fc3d4f444257175d Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 2 Dec 2019 17:36:19 +0900 Subject: [PATCH 06/15] Update git submodules * Update plugins/hooks from branch 'stable-3.1' to 22d1dbbbd2b34dc066e20e6fb26a8623f1ae47fc - Merge branch 'stable-3.0' into stable-3.1 * stable-3.0: Fix and clarify documentation of ref-update and commit-received hooks Correct documentation of ref-update hook regarding output Change-Id: Ifa4fefac518fc170f3248ebfcbe5e1a338e46c0f - Merge branch 'stable-2.16' into stable-3.0 * stable-2.16: Fix and clarify documentation of ref-update and commit-received hooks Correct documentation of ref-update hook regarding output Change-Id: Ia446c95e1f6ca12e1cbd198d3a7e4f6a16b09fc6 - Fix and clarify documentation of ref-update and commit-received hooks - Clarify behavior of ref-update when multiple commits are pushed, i.e. that it is only called once for the entire ref update. - Fix the documentation of commit-received which states that it is called for changes pushed for review. In fact it is also called for direct pushes, but unlike ref-update it is called once for each commit. Change-Id: Ie3e2dcab37eeb5d0ed06e05322e72d3fc73925fd - Correct documentation of ref-update hook regarding output The output of the hook is only returned when the update is rejected, not regardless of the return code as stated in the doc. Change-Id: I93483f44e31e7055c302c0758084de868d1c4f93 --- plugins/hooks | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hooks b/plugins/hooks index 56a1275644..22d1dbbbd2 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit 56a1275644ca69c8203354c3f5de5e99324a313f +Subproject commit 22d1dbbbd2b34dc066e20e6fb26a8623f1ae47fc From d90465732fabbe6981f038e6af84579ae804797c Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Mon, 2 Dec 2019 11:56:28 +0100 Subject: [PATCH 07/15] NotificationEmail: use MailHeader for 'Gerrit-Branch' footer name MailHeader.BRANCH defines footer name that can be re-used in NotificationEmail. Change-Id: If8c6e2585f5b4ec566a91615aacea8e70378622f --- java/com/google/gerrit/server/mail/send/NotificationEmail.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/mail/send/NotificationEmail.java b/java/com/google/gerrit/server/mail/send/NotificationEmail.java index a60dfea097..be593ff9f2 100644 --- a/java/com/google/gerrit/server/mail/send/NotificationEmail.java +++ b/java/com/google/gerrit/server/mail/send/NotificationEmail.java @@ -123,7 +123,7 @@ public abstract class NotificationEmail extends OutgoingEmail { soyContext.put("branch", branchData); footers.add(MailHeader.PROJECT.withDelimiter() + branch.getParentKey().get()); - footers.add("Gerrit-Branch: " + branch.getShortName()); + footers.add(MailHeader.BRANCH.withDelimiter() + branch.getShortName()); } @VisibleForTesting From 418c8364aa5c633a0fbec3d45666ec7246ebc458 Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Mon, 2 Dec 2019 12:06:26 +0100 Subject: [PATCH 08/15] OutgoingEmail: use consistently va.smtpRcptTo reference All remaining parameters that are passed to validation are used consistently but not this one. Change-Id: Ib7ae35b3f0afd3aa9e3f06e231ee498c34f398b7 --- java/com/google/gerrit/server/mail/send/OutgoingEmail.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index c1bef07e72..400bc4f300 100644 --- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -204,12 +204,12 @@ public abstract class OutgoingEmail { } } - Set
intersection = Sets.intersection(smtpRcptTo, smtpRcptToPlaintextOnly); + Set
intersection = Sets.intersection(va.smtpRcptTo, smtpRcptToPlaintextOnly); if (!intersection.isEmpty()) { logger.atSevere().log("Email '%s' will be sent twice to %s", messageClass, intersection); } - if (!smtpRcptTo.isEmpty()) { + if (!va.smtpRcptTo.isEmpty()) { // Send multipart message logger.atFine().log("Sending multipart '%s'", messageClass); args.emailSender.send(va.smtpFromAddress, va.smtpRcptTo, va.headers, va.body, va.htmlBody); From b6935b7f7f0be827f2e51ac3ec3880b454ea0254 Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Mon, 2 Dec 2019 13:38:36 +0100 Subject: [PATCH 09/15] ChangeEmail: add project to email headers It would simplify the email filtering: email body wouldn't have to be checked against project name. This would be also useful for project specific validation rules. Change-Id: I795c80d21d6768c14be7726f4450f6b1b5cb9be5 --- java/com/google/gerrit/server/mail/send/ChangeEmail.java | 1 + 1 file changed, 1 insertion(+) diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 8b8f6f9fb4..a1b20afc5c 100644 --- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java @@ -181,6 +181,7 @@ public abstract class ChangeEmail extends NotificationEmail { setChangeSubjectHeader(); setHeader(MailHeader.CHANGE_ID.fieldName(), "" + change.getKey().get()); setHeader(MailHeader.CHANGE_NUMBER.fieldName(), "" + change.getChangeId()); + setHeader(MailHeader.PROJECT.fieldName(), "" + change.getProject()); setChangeUrlHeader(); setCommitIdHeader(); From 5a8cc6a7c3ac852d74d98ce6a94338596acaf390 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Thu, 28 Nov 2019 14:35:06 +0100 Subject: [PATCH 10/15] ExternalIdCacheLoader: suppress warning if cache is not persisted. This generated some questions repo-discuss@ . This strategy probably doesn't work on googlesource.com, but we are less concerned about log spam. Change-Id: I3746edb79db67408fa66f7e4bb906b23d67acc80 (cherry picked from commit 596da9d839d510b35da15e87f09f02e6e2ad1518) --- .../account/externalids/ExternalIdCacheLoader.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java b/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java index 25420ee5d3..8887e064cd 100644 --- a/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java +++ b/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java @@ -71,6 +71,7 @@ public class ExternalIdCacheLoader extends CacheLoader private final Counter1 reloadCounter; private final Timer0 reloadDifferential; private final boolean enablePartialReloads; + private final boolean isPersistentCache; @Inject ExternalIdCacheLoader( @@ -101,6 +102,8 @@ public class ExternalIdCacheLoader extends CacheLoader .setUnit(Units.MILLISECONDS)); this.enablePartialReloads = config.getBoolean("cache", ExternalIdCacheImpl.CACHE_NAME, "enablePartialReloads", true); + this.isPersistentCache = + config.getInt("cache", ExternalIdCacheImpl.CACHE_NAME, "diskLimit", 0) > 0; } @Override @@ -156,8 +159,11 @@ public class ExternalIdCacheLoader extends CacheLoader } } if (oldExternalIds == null) { - logger.atWarning().log( - "Unable to find an old ExternalId cache state, falling back to full reload"); + if (isPersistentCache) { + // If there is no persistence, this is normal. Don't upset admins reading the logs. + logger.atWarning().log( + "Unable to find an old ExternalId cache state, falling back to full reload"); + } return reloadAllExternalIds(notesRev); } From 3ec1221b0dce213504c363686e414e5a38236dcd Mon Sep 17 00:00:00 2001 From: Paladox none Date: Fri, 29 Nov 2019 20:03:29 +0000 Subject: [PATCH 11/15] Fix loading font-roboto-local in the ui In I731e54ca872 paper-styles were switched to consuming the fonts from local gerrit site, instead of from external location. However, the fonts were missed to be added to the final artifact. Bug: Issue 11993 Change-Id: Iabdb392ae643ddf01c47d0ea99369cba257a9658 --- polygerrit-ui/app/rules.bzl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/polygerrit-ui/app/rules.bzl b/polygerrit-ui/app/rules.bzl index 3012f7fc8b..ca8c402227 100644 --- a/polygerrit-ui/app/rules.bzl +++ b/polygerrit-ui/app/rules.bzl @@ -90,6 +90,8 @@ def polygerrit_bundle(name, srcs, outs, app): # we extract from the zip, but depend on the component for license checking. "@webcomponentsjs//:zipfile", "//lib/js:webcomponentsjs", + "@font-roboto-local//:zipfile", + "//lib/js:font-roboto-local", ], outs = outs, cmd = " && ".join([ @@ -101,6 +103,7 @@ def polygerrit_bundle(name, srcs, outs, app): "for f in $(locations " + name + "_theme_sources); do cp $$f $$TMP/polygerrit_ui/styles/themes; done", "for f in $(locations //lib/js:highlightjs_files); do cp $$f $$TMP/polygerrit_ui/bower_components/highlightjs/ ; done", "unzip -qd $$TMP/polygerrit_ui/bower_components $(location @webcomponentsjs//:zipfile) webcomponentsjs/webcomponents-lite.js", + "unzip -qd $$TMP/polygerrit_ui/bower_components $(location @font-roboto-local//:zipfile) font-roboto-local/fonts/\*/\*.ttf", "cd $$TMP", "find . -exec touch -t 198001010000 '{}' ';'", "zip -qr $$ROOT/$@ *", From 15dceb8bae34ded804175494d1f3b6bba0f47e8f Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Mon, 2 Dec 2019 17:51:04 -0500 Subject: [PATCH 12/15] Upgrade testcontainers to 1.12.4 Change-Id: I8e9c62c7d22871ab03e0c5e6c6bcd5420c4c81f9 --- tools/nongoogle.bzl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl index 48671e3b39..d0a683ca9d 100644 --- a/tools/nongoogle.bzl +++ b/tools/nongoogle.bzl @@ -162,18 +162,18 @@ def declare_nongoogle_deps(): sha1 = "3e83394258ae2089be7219b971ec21a8288528ad", ) - TESTCONTAINERS_VERSION = "1.12.3" + TESTCONTAINERS_VERSION = "1.12.4" maven_jar( name = "testcontainers", artifact = "org.testcontainers:testcontainers:" + TESTCONTAINERS_VERSION, - sha1 = "e424a4549640e120acceac641ac909fcda58bf62", + sha1 = "456b6facac12c4b67130d9056a43c011679e9f0c", ) maven_jar( name = "testcontainers-elasticsearch", artifact = "org.testcontainers:elasticsearch:" + TESTCONTAINERS_VERSION, - sha1 = "c0796de5032070b8768ce78c78949b48f13c30db", + sha1 = "9e210c277a35a95a76d03a79e2812575bd07391c", ) maven_jar( From a47258be4ea1cb95505b68f373206fe483e92ff1 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 2 Dec 2019 18:33:03 +0100 Subject: [PATCH 13/15] Fix deletion of draft comment refs In NoteDb, we store draft comments in the All-Users repository. This improves performance in the googlesource.com deployment, as high-traffic repositories can be configured to bypass Gerrit ACL checks for the ref advertisement. This makes publishing comments non-atomic: after publishing comments successfully, the update of the draft branch may fail. For this reason, any NoteDb update that touches the draft ref in All-Users will schedule a deletion of all published comments for the change. If there were multiple patchsets, scheduling deletions of items that were not in the draft ref triggered a logic error in ChangeDraftUpdate, which caused the branch to become empty rather than deleted. This had the following consequences: * unused draft refs persisted in the All-Users repository, polluting the namespace. * published draft comments as well as deleted draft comments were kept in the history of the draft ref, keeping them alive for GC, and causing a steady increase of repository size. In other words, for this to trigger, a change would have to publish draft comments on a change with multiple patchsets, and a previously published comment. The problem was not visible to users of Gerrit. This changes fixes the problem by simply looking at the notemap to be serialized, and deleting the draft ref if the new notemap is empty. Existing refs must still be cleaned up. We could introduce code, perhaps in the ProjectConsistencyChecker, to remove the stale refs from All-Users. Change-Id: I6f7b65828bb047bbff041b4d78f6b40190e76219 --- .../server/notedb/ChangeDraftUpdate.java | 5 +-- .../acceptance/server/change/CommentsIT.java | 43 +++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index 169fd2e4a0..bb6476660f 100644 --- a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -198,10 +198,9 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { return NO_OP_UPDATE; } - // If we touched every revision and there are no comments left, tell the + // If there are no comments left, tell the // caller to delete the entire ref. - boolean touchedAllRevs = updatedRevs.equals(rnm.revisionNotes.keySet()); - if (touchedAllRevs && !hasComments) { + if (!rnm.noteMap.iterator().hasNext()) { return null; } diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index 7455419c26..0a9f41c66c 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -47,6 +47,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.notedb.ChangeNoteUtil; @@ -69,6 +70,7 @@ import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.RevCommit; @@ -287,6 +289,47 @@ public class CommentsIT extends AbstractDaemonTest { revision(r).review(input); } + @Test + public void postCommentsUnreachableData() throws Exception { + setApiUser(admin); + + String file = "file"; + PushOneCommit push = + pushFactory.create(db, admin.getIdent(), testRepo, "first subject", file, "l1\nl2\n"); + + String dest = "refs/for/master"; + PushOneCommit.Result r1 = push.to(dest); + r1.assertOkStatus(); + String changeId = r1.getChangeId(); + String revId = r1.getCommit().getName(); + + PushOneCommit.Result r2 = amendChange(r1.getChangeId()); + r2.assertOkStatus(); + + String draftRefName = RefNames.refsDraftComments(r1.getChange().getId(), admin.getId()); + + DraftInput draft = newDraft(file, Side.REVISION, 1, "comment"); + addDraft(changeId, "1", draft); + ReviewInput reviewInput = new ReviewInput(); + reviewInput.drafts = DraftHandling.PUBLISH; + reviewInput.message = "foo"; + gApi.changes().id(r1.getChangeId()).revision(1).review(reviewInput); + + addDraft(changeId, "2", newDraft(file, Side.REVISION, 2, "comment2")); + reviewInput = new ReviewInput(); + reviewInput.drafts = DraftHandling.PUBLISH_ALL_REVISIONS; + reviewInput.message = "bar"; + gApi.changes().id(r1.getChangeId()).revision(2).review(reviewInput); + + Map> drafts = getDraftComments(changeId, revId); + assertThat(drafts.isEmpty()).isTrue(); + + try (Repository repo = repoManager.openRepository(allUsers)) { + Ref ref = repo.exactRef(draftRefName); + assertThat(ref).isNull(); + } + } + @Test public void listComments() throws Exception { String file = "file"; From 1b2e0a7994b88db31bdacc17b36935b9339bc118 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 3 Dec 2019 09:32:34 +0900 Subject: [PATCH 14/15] ChangeDraftUpdate: Remove unused local variable Change-Id: I3c878ac283e92ea89d8dd92c906e97bd57a08682 --- java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index bb6476660f..d8390b754d 100644 --- a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -174,7 +174,6 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { Map builders = cache.getBuilders(); boolean touchedAnyRevs = false; - boolean hasComments = false; for (Map.Entry e : builders.entrySet()) { updatedRevs.add(e.getKey()); ObjectId id = ObjectId.fromString(e.getKey().get()); @@ -185,7 +184,6 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { if (data.length == 0) { rnm.noteMap.remove(id); } else { - hasComments = true; ObjectId dataBlob = ins.insert(OBJ_BLOB, data); rnm.noteMap.set(id, dataBlob); } From b81a9aa509675d3cdfab5ff4701af9ee5eeb15a7 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 3 Dec 2019 09:33:42 +0900 Subject: [PATCH 15/15] CommentsIT#postCommentsUnreachableData: Remove unnecessary setApiUser call Change-Id: Icfb41af5e35e3674a85e188918c972d623f41be1 --- .../com/google/gerrit/acceptance/server/change/CommentsIT.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index 0a9f41c66c..fb2441b6dc 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -291,8 +291,6 @@ public class CommentsIT extends AbstractDaemonTest { @Test public void postCommentsUnreachableData() throws Exception { - setApiUser(admin); - String file = "file"; PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo, "first subject", file, "l1\nl2\n");