From c65a3bd3bec622ddc2ff8bae5c617fd503878699 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 16 Jan 2018 08:15:25 +0100 Subject: [PATCH 1/6] Bazel: Use rules_closure from HEAD The latest rules_closure changes updated closure compiler version and remove stale bazel version check, that broke Bazel CI for gerrit: [1]. It turns out that the current code cannot be compiled by the latest closure compiler, so that it must be adjusted first. We add two more suppressions to the closure compiler invocation: * JSC_UNNECESSARY_ESCAPE * JSC_JSDOC_MISSING_TYPE_WARNING Some of the warnings flagged by the closure compiler are related to the moment library. The PR to fix them is pending for review: [2]. * [1] https://github.com/bazelbuild/bazel/issues/4425 * [2] https://github.com/moment/moment/pull/4350 Change-Id: I0354ceddc7f615d93b5d23cf2652b8552e53cc91 --- WORKSPACE | 6 +++--- polygerrit-ui/app/BUILD | 6 +++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 013031d81d..c9bd66b041 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -6,9 +6,9 @@ load("//plugins:external_plugin_deps.bzl", "external_plugin_deps") http_archive( name = "io_bazel_rules_closure", - sha256 = "25f5399f18d8bf9ce435f85c6bbf671ec4820bc4396b3022cc5dc4bc66303609", - strip_prefix = "rules_closure-0.4.2", - url = "https://bazel-mirror.storage.googleapis.com/github.com/bazelbuild/rules_closure/archive/0.4.2.tar.gz", # 2017-08-29 + sha256 = "6691c58a2cd30a86776dd9bb34898b041e37136f2dc7e24cadaeaf599c95c657", + strip_prefix = "rules_closure-08039ba8ca59f64248bb3b6ae016460fe9c9914f", + url = "https://github.com/bazelbuild/rules_closure/archive/08039ba8ca59f64248bb3b6ae016460fe9c9914f.tar.gz", ) # File is specific to Polymer and copied from the Closure Github -- should be diff --git a/polygerrit-ui/app/BUILD b/polygerrit-ui/app/BUILD index abfb8f85e5..f422cdbd20 100644 --- a/polygerrit-ui/app/BUILD +++ b/polygerrit-ui/app/BUILD @@ -36,7 +36,11 @@ closure_js_library( convention = "GOOGLE", # TODO(davido): Clean up these issues: http://paste.openstack.org/show/608548 # and remove this supression - suppress = ["JSC_UNUSED_LOCAL_ASSIGNMENT"], + suppress = [ + "JSC_JSDOC_MISSING_TYPE_WARNING", + "JSC_UNNECESSARY_ESCAPE", + "JSC_UNUSED_LOCAL_ASSIGNMENT", + ], deps = [ "//lib/polymer_externs:polymer_closure", "@io_bazel_rules_closure//closure/library", From 885bdb7545cb7e65de1c6bf4d68653a9f0d1103a Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 5 Feb 2018 08:25:16 +0900 Subject: [PATCH 2/6] AbstractPushForReview: Factor repeated code to setRequireChangeId method Several tests in AbstractPushForReview repeat the same block of code to set the "Require Change-Id" value on a project. Factor this repeated code out to a utility method in AbstractDaemonTest and reuse it. The new method follows the same pattern as the existing setUseSignedOffBy method. Change-Id: I608f0f8e21336fbd39a2422b6f415aaeb8747e3d --- .../gerrit/acceptance/AbstractDaemonTest.java | 9 +++++++++ .../acceptance/git/AbstractPushForReview.java | 16 ++++------------ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 61d748f43c..a21b7d0386 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -733,6 +733,15 @@ public abstract class AbstractDaemonTest { } } + protected void setRequireChangeId(InheritableBoolean value) throws Exception { + try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) { + ProjectConfig config = ProjectConfig.read(md); + config.getProject().setRequireChangeID(value); + config.commit(md); + projectCache.evict(config.getProject()); + } + } + protected void deny(String permission, AccountGroup.UUID id, String ref) throws Exception { deny(project, permission, id, ref); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index e689d4fa02..cb6d26b813 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -674,9 +674,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { pushForReviewRejected(testRepo, "missing Change-Id in commit message footer"); - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config.getProject().setRequireChangeID(InheritableBoolean.FALSE); - saveProjectConfig(project, config); + setRequireChangeId(InheritableBoolean.FALSE); pushForReviewOk(testRepo); } @@ -701,9 +699,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { pushForReviewRejected(testRepo, "multiple Change-Id lines in commit message footer"); - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config.getProject().setRequireChangeID(InheritableBoolean.FALSE); - saveProjectConfig(project, config); + setRequireChangeId(InheritableBoolean.FALSE); pushForReviewRejected(testRepo, "multiple Change-Id lines in commit message footer"); } @@ -727,9 +723,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer"); - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config.getProject().setRequireChangeID(InheritableBoolean.FALSE); - saveProjectConfig(project, config); + setRequireChangeId(InheritableBoolean.FALSE); pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer"); } @@ -753,9 +747,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer"); - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config.getProject().setRequireChangeID(InheritableBoolean.FALSE); - saveProjectConfig(project, config); + setRequireChangeId(InheritableBoolean.FALSE); pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer"); } From 3e2ef67a7f8b62da81d3e6efc005ebe407cbb8a6 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sun, 4 Feb 2018 17:31:58 +0900 Subject: [PATCH 3/6] CommitValidators: Always check for Change-Id in subject line A commit with an empty commit message except for a Change-Id line in the subject was only rejected if "Require Change-Id" was enabled. The consequence of the commit being accepted is that the change gets assigned a new Change-Id automatically by Gerrit, and it doesn't match the one in the commit message. If the user then amends the local commit with a full commit message, moving the Change-Id line from the subject to the correct place in the footer, on pushing it will result in a new change being created rather than a new patch set on the existing change. Modify the logic so that it always checks for a Change-Id in the subject line, not only when "Require Change-Id" is enabled. Bug: Issue 8279 Change-Id: I416143f6ca04c9f2ed72c6174ff77b8ffba4256d --- .../acceptance/git/AbstractPushForReview.java | 11 +++++++++++ .../server/git/validators/CommitValidators.java | 14 +++++++------- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index cb6d26b813..d71cfb4944 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -752,6 +752,17 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { "invalid Change-Id line format in commit message footer"); } + @Test + public void pushWithChangeIdInSubjectLine() throws Exception { + createCommit(testRepo, "Change-Id: I1234000000000000000000000000000000000000"); + pushForReviewRejected(testRepo, + "missing subject; Change-Id must be in commit message footer"); + + setRequireChangeId(InheritableBoolean.FALSE); + pushForReviewRejected(testRepo, + "missing subject; Change-Id must be in commit message footer"); + } + private static RevCommit createCommit(TestRepository testRepo, String message) throws Exception { return testRepo.branch("HEAD").commit().message(message) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index a49e02c42b..b38f797e5f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -227,14 +227,14 @@ public class CommitValidators { String sha1 = commit.abbreviate(SHA1_LENGTH).name(); if (idList.isEmpty()) { + String shortMsg = commit.getShortMessage(); + if (shortMsg.startsWith(CHANGE_ID_PREFIX) + && CHANGE_ID.matcher(shortMsg.substring( + CHANGE_ID_PREFIX.length()).trim()).matches()) { + String errMsg = String.format(MISSING_SUBJECT_MSG, sha1); + throw new CommitValidationException(errMsg); + } if (projectControl.getProjectState().isRequireChangeID()) { - String shortMsg = commit.getShortMessage(); - if (shortMsg.startsWith(CHANGE_ID_PREFIX) - && CHANGE_ID.matcher(shortMsg.substring( - CHANGE_ID_PREFIX.length()).trim()).matches()) { - String errMsg = String.format(MISSING_SUBJECT_MSG, sha1); - throw new CommitValidationException(errMsg); - } String errMsg = String.format(MISSING_CHANGE_ID_MSG, sha1); messages.add(getMissingChangeIdErrorMsg(errMsg, commit)); throw new CommitValidationException(errMsg, messages); From 181a3d0ba0aa41bcc2a86f539db8c3dc6727f3e0 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 5 Feb 2018 09:50:43 +0900 Subject: [PATCH 4/6] CreateChangeIT#createNewChangeSignedOffByFooter: Remove unnecessary assume The test creates a change with status NEW, so it is not necessary to assume that creation of drafts is allowed. Change-Id: Ia96b211e49332a4eecfcc0cc02bb50e6069d3a2b --- .../com/google/gerrit/acceptance/rest/change/CreateChangeIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index d696af40fd..5eddf6c22d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -102,7 +102,6 @@ public class CreateChangeIT extends AbstractDaemonTest { @Test public void createNewChangeSignedOffByFooter() throws Exception { - assume().that(isAllowDrafts()).isTrue(); setSignedOffByFooter(); ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW)); String message = info.revisions.get(info.currentRevision).commit.message; From 67e05395161bd7a2fc231d55d153ac63a4cee75e Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 5 Feb 2018 10:03:39 +0900 Subject: [PATCH 5/6] ReceiveCommits: Fix NEW_PATCHSET regex pattern The pattern did not match refs in the form refs/changes/01/1/1 which meant validation of Change-Id footers was skipped when the change was created from the web UI. This resulted in it being possible to use the UI to create a new change with an invalid Change-Id footer. Bug: Issue 8280 Change-Id: I4eae945a68eade6f3aa8664318e01ef85cf4a1c7 --- .../gerrit/acceptance/rest/change/CreateChangeIT.java | 9 +++++++++ .../com/google/gerrit/server/git/ReceiveCommits.java | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index 5eddf6c22d..e2cd7bc0e3 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -36,6 +36,7 @@ import com.google.gerrit.extensions.common.ChangeInput; import com.google.gerrit.extensions.common.MergeInput; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; +import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; @@ -95,6 +96,14 @@ public class CreateChangeIT extends AbstractDaemonTest { "unsupported change status"); } + @Test + public void createEmptyChange_InvalidChangeId() throws Exception { + ChangeInput ci = newChangeInput(ChangeStatus.NEW); + ci.subject = "Subject\n\nChange-Id: I0000000000000000000000000000000000000000"; + assertCreateFails(ci, ResourceConflictException.class, + "invalid Change-Id line format in commit message footer"); + } + @Test public void createNewChange() throws Exception { assertCreateSucceeds(newChangeInput(ChangeStatus.NEW)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 88beacf6b9..49b66331fb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -180,7 +180,7 @@ public class ReceiveCommits { LoggerFactory.getLogger(ReceiveCommits.class); public static final Pattern NEW_PATCHSET = Pattern.compile( - "^" + REFS_CHANGES + "(?:[0-9][0-9]/)?([1-9][0-9]*)(?:/new)?$"); + "^" + REFS_CHANGES + "(?:[0-9][0-9]/)?([1-9][0-9]*)(?:/[1-9][0-9]*)?$"); private static final String COMMAND_REJECTION_MESSAGE_FOOTER = "Please read the documentation and contact an administrator\n" From 525c98bf3d75b73c216f5b50113ea2eb6be687b7 Mon Sep 17 00:00:00 2001 From: Kasper Nilsson Date: Wed, 27 Dec 2017 14:12:15 -0800 Subject: [PATCH 6/6] Reset state flags on loadData calls In some cases, bound values were setting the flags tracking changes in gr-account-info. Resetting the flags when loadData is called ensures no false positives occur. In addition, upgrades the _usernameChanged logic to compare to the existing account username value, as opposed to just setting the flag to true on any modification. Bug: Issue 7893 Bug: Issue 8287 Change-Id: I75a3cda93bb065a7b9640d4b40905a5e21042bb5 (cherry picked from commit 5cb34e7fa3896a8290023762729690cf15b17ef7) --- .../gr-account-info/gr-account-info.js | 23 ++++++++----------- .../gr-account-info/gr-account-info_test.html | 18 +++++++++++++++ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.js b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.js index a698c71499..3cec65a594 100644 --- a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.js +++ b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.js @@ -41,18 +41,9 @@ '_hasUsernameChange, _hasStatusChange)', }, - _hasNameChange: { - type: Boolean, - value: false, - }, - _hasUsernameChange: { - type: Boolean, - value: false, - }, - _hasStatusChange: { - type: Boolean, - value: false, - }, + _hasNameChange: Boolean, + _hasUsernameChange: Boolean, + _hasStatusChange: Boolean, _loading: { type: Boolean, value: false, @@ -85,6 +76,9 @@ })); promises.push(this.$.restAPI.getAccount().then(account => { + this._hasNameChange = false; + this._hasUsernameChange = false; + this._hasStatusChange = false; // Provide predefined value for username to trigger computation of // username mutability. account.username = account.username || ''; @@ -154,8 +148,9 @@ }, _usernameChanged() { - if (this._loading) { return; } - this._hasUsernameChange = true; + if (this._loading || !this._account) { return; } + this._hasUsernameChange = + (this._account.username || '') !== (this._username || ''); }, _nameChanged() { diff --git a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.html b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.html index d27d15393a..82997a5ec3 100644 --- a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.html +++ b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.html @@ -311,5 +311,23 @@ limitations under the License. }); }); }); + + test('_usernameChanged compares usernames with loose equality', () => { + element._account = {}; + element._username = ''; + element._hasUsernameChange = false; + element._loading = false; + // _usernameChanged is an observer, but call it here after setting + // _hasUsernameChange in the test to force recomputation. + element._usernameChanged(); + flushAsynchronousOperations(); + + assert.isFalse(element._hasUsernameChange); + + element.set('_username', 'test'); + flushAsynchronousOperations(); + + assert.isTrue(element._hasUsernameChange); + }); });