Merge "PolyGerrit: Add support for "Included In""
diff --git a/.mailmap b/.mailmap
index bd4d222..f5f8f3e 100644
--- a/.mailmap
+++ b/.mailmap
@@ -11,6 +11,7 @@
Bruce Zu <bruce.zu.run10@gmail.com> <bruce.zu@sonymobile.com>
Carlos Eduardo Baldacin <carloseduardo.baldacin@sonyericsson.com> carloseduardo.baldacin <carloseduardo.baldacin@sonyericsson.com>
Dariusz Luksza <dluksza@collab.net> <dariusz@luksza.org>
+Dave Borowitz <dborowitz@google.com> <dborowitz@google.com>
David Ostrovsky <david@ostrovsky.org> <d.ostrovsky@gmx.de>
David Ostrovsky <david@ostrovsky.org> <david.ostrovsky@gmail.com>
David Pursehouse <dpursehouse@collab.net> <david.pursehouse@sonymobile.com>
@@ -43,6 +44,7 @@
Mark Derricutt <mark.derricutt@smxemail.com> <mark@talios.com>
Martin Fick <mfick@codeaurora.org> <mogulguy10@gmail.com>
Martin Fick <mfick@codeaurora.org> <mogulguy@yahoo.com>
+Maxime Guerreiro <maximeg@google.com> <maximeg@google.com>
Michael Zhou <moz@google.com> <zhoumotongxue008@gmail.com>
Mônica Dionísio <monica.dionisio@sonyericsson.com> monica.dionisio <monica.dionisio@sonyericsson.com>
Nasser Grainawi <nasser@grainawi.org> <nasser@codeaurora.org>
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index 189c6c7..e6a6227 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -2731,10 +2731,114 @@
@Override
public String intercept(String in) {
return pluginName + " mycommand";
+----
+
+
+[[pre-submit-evaluator]]
+== Pre-submit Validation Plugins
+
+Gerrit provides an extension point that enables plugins to prevent a change
+from being submitted.
+
+[IMPORTANT]
+This extension point **must NOT** be used for long or slow operations, like
+calling external programs or content, running unit tests...
+Slow operations will hurt the whole Gerrit instance.
+
+This can be used to implement custom rules that changes have to match to become
+submittable. A more concrete example: the Prolog rules engine can be
+implemented using this.
+
+Gerrit calls the plugins once per change and caches the results. Although it is
+possible to predict when this interface will be triggered, this should not be
+considered as a feature. Plugins should only rely on the internal state of the
+ChangeData, not on external values like date and time, remote content or
+randomness.
+
+Plugins are expected to support rules inheritance themselves, providing ways
+to configure it and handling the logic behind it.
+Please note that no inheritance is sometimes better than badly handled
+inheritance: mis-communication and strange behaviors caused by inheritance
+may and will confuse the users. Each plugins is responsible for handling the
+project hierarchy and taking wise actions. Gerrit does not enforce it.
+
+Once Gerrit has gathered every plugins' SubmitRecords, it stores them.
+
+Plugins accept or reject a given change using `SubmitRecord.Status`.
+If a change is ready to be submitted, `OK`. If it is not ready and requires
+modifications, `NOT_READY`. Other statuses are available for particular cases.
+A change can be submitted if all the plugins accept the change.
+
+Plugins may also decide not to vote on a given change by returning an empty
+Collection (ie: the plugin is not enabled for this repository), or to vote
+several times (ie: one SubmitRecord per project in the hierarchy).
+The results are handled as if multiple plugins voted for the change.
+
+If a plugin decides not to vote, it's name will not be displayed in the UI and
+it will not be recoded in the database.
+
+.Gerrit's Pre-submit handling with three plugins
+[width="50%",cols="^m,^m,^m,^m",frame="topbot",options="header"]
+|=======================================================
+| Plugin A | Plugin B | Plugin C | Final decision
+| OK | OK | OK | OK
+| OK | OK | / | OK
+| OK | OK | RULE_ERROR | NOT_READY
+| OK | NOT_READY | OK | NOT_READY
+| NOT_READY | OK | OK | NOT_READY
+|=======================================================
+
+
+This makes composing plugins really easy.
+
+- If a plugin places a veto on a change, it can't be submitted.
+- If a plugin isn't enabled for a project (or isn't needed for this change),
+ it returns an empty collection.
+- If all the plugins answer `OK`, the change can be submitted.
+
+
+A more rare case, but worth documenting: if there are no installed plugins,
+the labels will be compared to the rules defined in the project's config,
+and the permission system will be used to allow or deny a submit request.
+
+Some rules are defined internally to provide a common base ground (and sanity):
+changes that are marked as WIP or that are closed (abandoned, merged) can't be merged.
+
+
+[source, java]
+----
+import java.util.Collection;
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitRecord.Status;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.SubmitRule;
+
+public class MyPluginRules implements SubmitRule {
+ public Collection<SubmitRecord> evaluate(ChangeData changeData) {
+ // Implement your submitability logic here
+
+ // Assuming we want to prevent this change from being submitted:
+ SubmitRecord record;
+ record.status = Status.NOT_READY;
+ return record;
}
}
----
+Don't forget to register your class!
+
+[source, java]
+----
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.inject.AbstractModule;
+
+public class MyPluginModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ bind(SubmitRule.class).annotatedWith(Exports.named("myPlugin")).to(MyPluginRules.class);
+ }
+}
+----
== SEE ALSO
diff --git a/Documentation/json.txt b/Documentation/json.txt
index f1dbe73..2b34185 100644
--- a/Documentation/json.txt
+++ b/Documentation/json.txt
@@ -178,6 +178,21 @@
labels:: This describes the state of each code review
<<label,label attribute>>, unless the status is RULE_ERROR.
+requirements:: Each <<requirement>> describes what needs to be changed
+in order for the change to be submittable.
+
+
+[[requirement]]
+== requirement
+Information about a requirement (not met) in order to submit a change.
+
+shortReason:: A short description of the requirement (a hint).
+
+fullReason:: A longer and descriptive message explaining what needs to
+be changed to meet the requirement.
+
+label:: (Optional) The name of the linked label, if set by a pre-submit rule.
+
[[label]]
== label
Information about a code review label for a change.
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index c1ca595..70c00ff 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6653,12 +6653,20 @@
The `RevertInput` entity contains information for reverting a change.
[options="header",cols="1,^1,5"]
-|===========================
-|Field Name ||Description
-|`message` |optional|
+|=============================
+|Field Name ||Description
+|`message` |optional|
Message to be added as review comment to the change when reverting the
change.
-|===========================
+|`notify` |optional|
+Notify handling that defines to whom email notifications should be sent
+for reverting the change. +
+Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. +
+If not set, the default is `ALL`.
+|`notify_details`|optional|
+Additional information about whom to notify about the revert as a map
+of recipient type to link:#notify-info[NotifyInfo] entity.
+|=============================
[[review-info]]
=== ReviewInfo
diff --git a/Documentation/rest-api-groups.txt b/Documentation/rest-api-groups.txt
index a3bcab1..49d5ee5 100644
--- a/Documentation/rest-api-groups.txt
+++ b/Documentation/rest-api-groups.txt
@@ -1518,7 +1518,9 @@
`REMOVE_USER` the member is returned as detailed
link:rest-api-accounts.html#account-info[AccountInfo] entity, if `type`
is `ADD_GROUP` or `REMOVE_GROUP` the member is returned as
-link:#group-info[GroupInfo] entity.
+link:#group-info[GroupInfo] entity. Note that the `name` in
+link:#group-info[GroupInfo] will not be set if the member group is not
+available.
|`type` |
The event type, can be: `ADD_USER`, `REMOVE_USER`, `ADD_GROUP` or
`REMOVE_GROUP`.
diff --git a/WORKSPACE b/WORKSPACE
index b915ed10..d782c23 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -417,8 +417,8 @@
maven_jar(
name = "auto_value",
- artifact = "com.google.auto.value:auto-value:1.5.3",
- sha1 = "514df6a7c7938de35c7f68dc8b8f22df86037f38",
+ artifact = "com.google.auto.value:auto-value:1.5.4",
+ sha1 = "65183ddd1e9542d69d8f613fdae91540d04e1476",
)
# Transitive dependency of commons-compress
@@ -701,18 +701,18 @@
sha1 = "4785a3c21320980282f9f33d0d1264a69040538f",
)
-TRUTH_VERS = "0.36"
+TRUTH_VERS = "0.39"
maven_jar(
name = "truth",
artifact = "com.google.truth:truth:" + TRUTH_VERS,
- sha1 = "7485219d2c1d341097a19382c02bde07e69ff5d2",
+ sha1 = "bd1bf5706ff34eb7ff80fef8b0c4320f112ef899",
)
maven_jar(
name = "truth-java8-extension",
artifact = "com.google.truth.extensions:truth-java8-extension:" + TRUTH_VERS,
- sha1 = "dcc60988c8f9a051840766ef192a2ef41e7992f1",
+ sha1 = "1499bc88cda9d674afb30da9813b44bcd4512d0d",
)
# When bumping the easymock version number, make sure to also move powermock to a compatible version
diff --git a/contrib/populate-fixture-data.py b/contrib/populate-fixture-data.py
old mode 100644
new mode 100755
index 0e3dffe..99d9a49
--- a/contrib/populate-fixture-data.py
+++ b/contrib/populate-fixture-data.py
@@ -17,34 +17,24 @@
This script will populate an empty standard Gerrit instance with some
data for local testing.
-This script requires 'requests'. If you do not have this module, run
-'pip3 install requests' to install it.
-
TODO(hiesel): Make real git commits instead of empty changes
TODO(hiesel): Add comments
"""
import atexit
import json
+import optparse
import os
import random
import shutil
import subprocess
import tempfile
-
import requests
import requests.auth
DEFAULT_TMP_PATH = "/tmp"
TMP_PATH = ""
-BASE_URL = "http://localhost:8080/a/"
-ACCESS_URL = BASE_URL + "access/"
-ACCOUNTS_URL = BASE_URL + "accounts/"
-CHANGES_URL = BASE_URL + "changes/"
-CONFIG_URL = BASE_URL + "config/"
-GROUPS_URL = BASE_URL + "groups/"
-PLUGINS_URL = BASE_URL + "plugins/"
-PROJECTS_URL = BASE_URL + "projects/"
+BASE_URL = "http://localhost:%d/a/"
ADMIN_BASIC_AUTH = requests.auth.HTTPBasicAuth("admin", "secret")
@@ -158,7 +148,7 @@
def fetch_admin_group():
global GROUP_ADMIN
# Get admin group
- r = json.loads(clean(requests.get(GROUPS_URL + "?suggest=ad&p=All-Projects",
+ r = json.loads(clean(requests.get(BASE_URL + "groups/" + "?suggest=ad&p=All-Projects",
headers=HEADERS,
auth=ADMIN_BASIC_AUTH).text))
admin_group_name = r.keys()[0]
@@ -222,7 +212,7 @@
"visible_to_all": False, "owner": GROUP_ADMIN["name"],
"owner_id": GROUP_ADMIN["id"]}]
for g in groups:
- requests.put(GROUPS_URL + g["name"],
+ requests.put(BASE_URL + "groups/" + g["name"],
json.dumps(g),
headers=HEADERS,
auth=ADMIN_BASIC_AUTH)
@@ -244,7 +234,7 @@
"branches": ["master"], "description": "some small scripts.",
"owners": [owner_groups[3]], "create_empty_commit": True}]
for p in projects:
- requests.put(PROJECTS_URL + p["name"],
+ requests.put(BASE_URL + "projects/" + p["name"],
json.dumps(p),
headers=HEADERS,
auth=ADMIN_BASIC_AUTH)
@@ -253,7 +243,7 @@
def create_gerrit_users(gerrit_users):
for user in gerrit_users:
- requests.put(ACCOUNTS_URL + user["username"],
+ requests.put(BASE_URL + "accounts/" + user["username"],
json.dumps(user),
headers=HEADERS,
auth=ADMIN_BASIC_AUTH)
@@ -267,7 +257,7 @@
"branch": "master",
"status": "NEW",
}
- requests.post(CHANGES_URL,
+ requests.post(BASE_URL + "changes/",
json.dumps(change),
headers=HEADERS,
auth=basic_auth(user))
@@ -278,8 +268,22 @@
def main():
+ p = optparse.OptionParser()
+ p.add_option("-u", "--user_count", action="store",
+ default=100,
+ type='int',
+ help="number of users to generate")
+ p.add_option("-p", "--port", action="store",
+ default=8080,
+ type='int',
+ help="port of server")
+ (options, _) = p.parse_args()
+ global BASE_URL
+ BASE_URL = BASE_URL % options.port
+ print BASE_URL
+
set_up()
- gerrit_users = get_random_users(100)
+ gerrit_users = get_random_users(options.user_count)
group_names = create_gerrit_groups()
for idx, u in enumerate(gerrit_users):
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 06efbfd..54ba802 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -103,6 +103,7 @@
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.group.db.Groups;
+import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.index.change.ChangeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.index.change.ChangeIndexer;
@@ -201,7 +202,12 @@
firstTest = description;
}
beforeTest(description);
- try (ProjectResetter resetter = resetProjects(projectResetter.builder())) {
+ ProjectResetter.Config input = resetProjects();
+ if (input == null) {
+ input = defaultResetProjects();
+ }
+
+ try (ProjectResetter resetter = projectResetter.builder().build(input)) {
AbstractDaemonTest.this.resetter = resetter;
base.evaluate();
} finally {
@@ -268,6 +274,7 @@
@Inject private InProcessProtocol inProcessProtocol;
@Inject private Provider<AnonymousUser> anonymousUser;
@Inject private SchemaFactory<ReviewDb> reviewDbProvider;
+ @Inject private AccountIndexer accountIndexer;
@Inject private Groups groups;
@Inject private GroupIndexer groupIndexer;
@@ -317,8 +324,12 @@
}
/** Controls which project and branches should be reset after each test case. */
- protected ProjectResetter resetProjects(ProjectResetter.Builder resetter) throws IOException {
- return resetter
+ protected ProjectResetter.Config resetProjects() {
+ return null;
+ }
+
+ private ProjectResetter.Config defaultResetProjects() {
+ return new ProjectResetter.Config()
// Don't reset all refs so that refs/sequences/changes is not touched and change IDs are
// not reused.
.reset(allProjects, RefNames.REFS_CONFIG)
@@ -331,8 +342,7 @@
RefNames.REFS_USERS + "*",
RefNames.REFS_EXTERNAL_IDS,
RefNames.REFS_STARRED_CHANGES + "*",
- RefNames.REFS_DRAFT_COMMENTS + "*")
- .build();
+ RefNames.REFS_DRAFT_COMMENTS + "*");
}
protected void restartAsSlave() throws Exception {
@@ -345,6 +355,11 @@
initSsh();
}
+ protected void evictAndReindexAccount(Account.Id accountId) throws IOException {
+ accountCache.evict(accountId);
+ accountIndexer.index(accountId);
+ }
+
private void reindexAllGroups() throws OrmException, IOException, ConfigInvalidException {
Iterable<GroupReference> allGroups = groups.getAllGroupReferences(db)::iterator;
for (GroupReference group : allGroups) {
@@ -408,9 +423,9 @@
admin = accountCreator.admin();
user = accountCreator.user();
- // Evict cached user state in case tests modify it.
- accountCache.evict(admin.getId());
- accountCache.evict(user.getId());
+ // Evict and reindex accounts in case tests modify them.
+ evictAndReindexAccount(admin.getId());
+ evictAndReindexAccount(user.getId());
adminRestSession = new RestSession(server, admin);
userRestSession = new RestSession(server, user);
diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
index 086cacb..303a16a 100644
--- a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
@@ -25,7 +25,6 @@
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.Subject;
import com.google.common.truth.Truth;
-import com.google.gerrit.acceptance.ProjectResetter.Builder;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -63,10 +62,10 @@
}
@Override
- protected ProjectResetter resetProjects(Builder resetter) throws IOException {
+ protected ProjectResetter.Config resetProjects() {
// Don't reset anything so that stagedUsers can be cached across all tests.
// Without this caching these tests become much too slow.
- return resetter.build();
+ return new ProjectResetter.Config();
}
protected static FakeEmailSenderSubject assertThat(FakeEmailSender sender) {
@@ -323,7 +322,7 @@
}
private TestAccount evictAndCopy(TestAccount account) throws IOException {
- accountCache.evict(account.id);
+ evictAndReindexAccount(account.id);
return account;
}
diff --git a/java/com/google/gerrit/acceptance/ProjectResetter.java b/java/com/google/gerrit/acceptance/ProjectResetter.java
index 569a0c1..86718be 100644
--- a/java/com/google/gerrit/acceptance/ProjectResetter.java
+++ b/java/com/google/gerrit/acceptance/ProjectResetter.java
@@ -30,6 +30,7 @@
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.index.RefState;
+import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.RefPatternMatcher;
import com.google.inject.Inject;
@@ -86,26 +87,45 @@
private final AllUsersName allUsersName;
@Nullable private final AccountCreator accountCreator;
@Nullable private final AccountCache accountCache;
+ @Nullable private final AccountIndexer accountIndexer;
@Nullable private final ProjectCache projectCache;
- private final Multimap<Project.NameKey, String> refsByProject;
-
@Inject
public Builder(
GitRepositoryManager repoManager,
AllUsersName allUsersName,
@Nullable AccountCreator accountCreator,
@Nullable AccountCache accountCache,
+ @Nullable AccountIndexer accountIndexer,
@Nullable ProjectCache projectCache) {
this.repoManager = repoManager;
this.allUsersName = allUsersName;
this.accountCreator = accountCreator;
this.accountCache = accountCache;
+ this.accountIndexer = accountIndexer;
this.projectCache = projectCache;
+ }
+
+ public ProjectResetter build(ProjectResetter.Config input) throws IOException {
+ return new ProjectResetter(
+ repoManager,
+ allUsersName,
+ accountCreator,
+ accountCache,
+ accountIndexer,
+ projectCache,
+ input.refsByProject);
+ }
+ }
+
+ public static class Config {
+ private final Multimap<Project.NameKey, String> refsByProject;
+
+ public Config() {
this.refsByProject = MultimapBuilder.hashKeys().arrayListValues().build();
}
- public Builder reset(Project.NameKey project, String... refPatterns) {
+ public Config reset(Project.NameKey project, String... refPatterns) {
List<String> refPatternList = Arrays.asList(refPatterns);
if (refPatternList.isEmpty()) {
refPatternList = ImmutableList.of(RefNames.REFS + "*");
@@ -113,17 +133,13 @@
refsByProject.putAll(project, refPatternList);
return this;
}
-
- public ProjectResetter build() throws IOException {
- return new ProjectResetter(
- repoManager, allUsersName, accountCreator, accountCache, projectCache, refsByProject);
- }
}
@Inject private GitRepositoryManager repoManager;
@Inject private AllUsersName allUsersName;
@Inject @Nullable private AccountCreator accountCreator;
@Inject @Nullable private AccountCache accountCache;
+ @Inject @Nullable private AccountIndexer accountIndexer;
@Inject @Nullable private ProjectCache projectCache;
private final Multimap<Project.NameKey, String> refsPatternByProject;
@@ -138,6 +154,7 @@
AllUsersName allUsersName,
@Nullable AccountCreator accountCreator,
@Nullable AccountCache accountCache,
+ @Nullable AccountIndexer accountIndexer,
@Nullable ProjectCache projectCache,
Multimap<Project.NameKey, String> refPatternByProject)
throws IOException {
@@ -145,6 +162,7 @@
this.allUsersName = allUsersName;
this.accountCreator = accountCreator;
this.accountCache = accountCache;
+ this.accountIndexer = accountIndexer;
this.projectCache = projectCache;
this.refsPatternByProject = refPatternByProject;
this.savedRefStatesByProject = readRefStates();
@@ -282,7 +300,7 @@
if (accountCreator != null) {
accountCreator.evict(deletedAccounts);
}
- if (accountCache != null) {
+ if (accountCache != null || accountIndexer != null) {
Set<Account.Id> modifiedAccounts =
new HashSet<>(accountIds(restoredRefsByProject.get(allUsersName)));
@@ -294,23 +312,33 @@
for (Account.Id id :
accountIds(
repo.getAllRefs().values().stream().map(r -> r.getName()).collect(toSet()))) {
- accountCache.evict(id);
+ evictAndReindexAccount(id);
}
}
// Remove deleted accounts from the cache and index.
for (Account.Id id : deletedAccounts) {
- accountCache.evict(id);
+ evictAndReindexAccount(id);
}
} else {
// Evict and reindex all modified and deleted accounts.
for (Account.Id id : Sets.union(modifiedAccounts, deletedAccounts)) {
- accountCache.evict(id);
+ evictAndReindexAccount(id);
}
}
}
}
+ private void evictAndReindexAccount(Account.Id accountId) throws IOException {
+ if (accountCache != null) {
+ accountCache.evict(accountId);
+ }
+
+ if (accountIndexer != null) {
+ accountIndexer.index(accountId);
+ }
+ }
+
private Set<Account.Id> accountIds(Collection<String> refs) {
return refs.stream()
.filter(r -> r.startsWith(REFS_USERS))
diff --git a/java/com/google/gerrit/common/data/SubmitRecord.java b/java/com/google/gerrit/common/data/SubmitRecord.java
index 9151222..3cd9c43 100644
--- a/java/com/google/gerrit/common/data/SubmitRecord.java
+++ b/java/com/google/gerrit/common/data/SubmitRecord.java
@@ -18,15 +18,20 @@
import java.util.Collection;
import java.util.List;
import java.util.Objects;
-import java.util.Optional;
-/** Describes the state required to submit a change. */
+/** Describes the state and edits required to submit a change. */
public class SubmitRecord {
- public static Optional<SubmitRecord> findOkRecord(Collection<SubmitRecord> in) {
- if (in == null) {
- return Optional.empty();
+ public static boolean allRecordsOK(Collection<SubmitRecord> in) {
+ if (in == null || in.isEmpty()) {
+ // If the list is null or empty, it means that this Gerrit installation does not
+ // have any form of validation rules.
+ // Hence, the permission system should be used to determine if the change can be merged
+ // or not.
+ return true;
}
- return in.stream().filter(r -> r.status == Status.OK).findFirst();
+
+ // The change can be submitted, unless at least one plugin prevents it.
+ return in.stream().noneMatch(r -> r.status != Status.OK);
}
public enum Status {
@@ -36,7 +41,7 @@
/** The change is ready for submission. */
OK,
- /** The change is missing a required label. */
+ /** Something is preventing this change from being submitted. */
NOT_READY,
/** The change has been closed. */
@@ -55,6 +60,7 @@
public Status status;
public List<Label> labels;
+ public List<SubmitRequirement> requirements;
public String errorMessage;
public static class Label {
@@ -140,6 +146,14 @@
delimiter = ", ";
}
}
+ sb.append("],[");
+ if (requirements != null) {
+ String delimiter = "";
+ for (SubmitRequirement requirement : requirements) {
+ sb.append(delimiter).append(requirement);
+ delimiter = ", ";
+ }
+ }
sb.append(']');
return sb.toString();
}
@@ -150,13 +164,14 @@
SubmitRecord r = (SubmitRecord) o;
return Objects.equals(status, r.status)
&& Objects.equals(labels, r.labels)
- && Objects.equals(errorMessage, r.errorMessage);
+ && Objects.equals(errorMessage, r.errorMessage)
+ && Objects.equals(requirements, r.requirements);
}
return false;
}
@Override
public int hashCode() {
- return Objects.hash(status, labels, errorMessage);
+ return Objects.hash(status, labels, errorMessage, requirements);
}
}
diff --git a/java/com/google/gerrit/common/data/SubmitRequirement.java b/java/com/google/gerrit/common/data/SubmitRequirement.java
new file mode 100644
index 0000000..a75f3c0
--- /dev/null
+++ b/java/com/google/gerrit/common/data/SubmitRequirement.java
@@ -0,0 +1,80 @@
+// Copyright (C) 2018 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.common.data;
+
+import static java.util.Objects.requireNonNull;
+
+import com.google.gerrit.common.Nullable;
+import java.util.Objects;
+import java.util.Optional;
+
+/** Describes a requirement to submit a change. */
+public final class SubmitRequirement {
+ private final String shortReason;
+ private final String fullReason;
+ @Nullable private final String label;
+
+ public SubmitRequirement(String shortReason, String fullReason, @Nullable String label) {
+ this.shortReason = requireNonNull(shortReason);
+ this.fullReason = requireNonNull(fullReason);
+ this.label = label;
+ }
+
+ public String shortReason() {
+ return shortReason;
+ }
+
+ public String fullReason() {
+ return fullReason;
+ }
+
+ public Optional<String> label() {
+ return Optional.ofNullable(label);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o instanceof SubmitRequirement) {
+ SubmitRequirement that = (SubmitRequirement) o;
+ return Objects.equals(shortReason, that.shortReason)
+ && Objects.equals(fullReason, that.fullReason)
+ && Objects.equals(label, that.label);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(shortReason, fullReason, label);
+ }
+
+ @Override
+ public String toString() {
+ return "SubmitRequirement{"
+ + "shortReason='"
+ + shortReason
+ + '\''
+ + ", fullReason='"
+ + fullReason
+ + '\''
+ + ", label='"
+ + label
+ + '\''
+ + '}';
+ }
+}
diff --git a/java/com/google/gerrit/extensions/api/changes/RevertInput.java b/java/com/google/gerrit/extensions/api/changes/RevertInput.java
index 893472e..c1be9b0 100644
--- a/java/com/google/gerrit/extensions/api/changes/RevertInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/RevertInput.java
@@ -15,7 +15,13 @@
package com.google.gerrit.extensions.api.changes;
import com.google.gerrit.extensions.restapi.DefaultInput;
+import java.util.Map;
public class RevertInput {
@DefaultInput public String message;
+
+ /** Who to send email notifications to after change is created. */
+ public NotifyHandling notify = NotifyHandling.ALL;
+
+ public Map<RecipientType, NotifyInfo> notifyDetails;
}
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index d070966..131dbf9 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -46,6 +46,7 @@
import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT;
import static javax.servlet.http.HttpServletResponse.SC_OK;
import static javax.servlet.http.HttpServletResponse.SC_PRECONDITION_FAILED;
+import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE;
import com.google.common.base.CharMatcher;
import com.google.common.base.Joiner;
@@ -102,9 +103,11 @@
import com.google.gerrit.server.audit.AuditService;
import com.google.gerrit.server.audit.ExtendedHttpAuditEvent;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.LockFailureException;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.util.http.RequestUtil;
import com.google.gson.ExclusionStrategy;
import com.google.gson.FieldAttributes;
@@ -480,6 +483,15 @@
} catch (NotImplementedException e) {
responseBytes =
replyError(req, res, status = SC_NOT_IMPLEMENTED, messageOr(e, "Not Implemented"), e);
+ } catch (UpdateException e) {
+ Throwable t = e.getCause();
+ if (t instanceof LockFailureException) {
+ responseBytes =
+ replyError(req, res, status = SC_SERVICE_UNAVAILABLE, messageOr(t, "Lock failure"), e);
+ } else {
+ status = SC_INTERNAL_SERVER_ERROR;
+ responseBytes = handleException(e, req, res);
+ }
} catch (Exception e) {
status = SC_INTERNAL_SERVER_ERROR;
responseBytes = handleException(e, req, res);
diff --git a/java/com/google/gerrit/metrics/proc/JGitMetricModule.java b/java/com/google/gerrit/metrics/proc/JGitMetricModule.java
index c3eb39f..4487ae5 100644
--- a/java/com/google/gerrit/metrics/proc/JGitMetricModule.java
+++ b/java/com/google/gerrit/metrics/proc/JGitMetricModule.java
@@ -18,7 +18,7 @@
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
import com.google.gerrit.metrics.MetricMaker;
-import org.eclipse.jgit.internal.storage.file.WindowCacheStatAccessor;
+import org.eclipse.jgit.storage.file.WindowCacheStats;
public class JGitMetricModule extends MetricModule {
@Override
@@ -32,7 +32,7 @@
new Supplier<Long>() {
@Override
public Long get() {
- return WindowCacheStatAccessor.getOpenBytes();
+ return WindowCacheStats.getOpenBytes();
}
});
@@ -43,7 +43,7 @@
new Supplier<Integer>() {
@Override
public Integer get() {
- return WindowCacheStatAccessor.getOpenFiles();
+ return WindowCacheStats.getOpenFiles();
}
});
}
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index a7803a9..357674d 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -157,7 +157,6 @@
install(new ExternalIdModule());
install(new GroupModule());
install(new NoteDbModule(cfg));
- install(new PrologModule());
install(AccountCacheImpl.module());
install(GroupCacheImpl.module());
install(GroupIncludeCacheImpl.module());
@@ -169,7 +168,10 @@
factory(CapabilityCollection.Factory.class);
factory(ChangeData.AssistedFactory.class);
factory(ProjectState.Factory.class);
+
+ // Submit rule evaluator
factory(SubmitRuleEvaluator.Factory.class);
+ install(new PrologModule());
bind(ChangeJson.Factory.class).toProvider(Providers.<ChangeJson.Factory>of(null));
bind(EventUtil.class).toProvider(Providers.<EventUtil>of(null));
diff --git a/java/com/google/gerrit/server/account/AccountCache.java b/java/com/google/gerrit/server/account/AccountCache.java
index 875bf5f..b6ca1cb 100644
--- a/java/com/google/gerrit/server/account/AccountCache.java
+++ b/java/com/google/gerrit/server/account/AccountCache.java
@@ -16,7 +16,6 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
-import java.io.IOException;
import java.util.Optional;
/** Caches important (but small) account state to avoid database hits. */
@@ -60,13 +59,12 @@
Optional<AccountState> getByUsername(String username);
/**
- * Evicts the account from the cache and triggers a reindex for it.
+ * Evicts the account from the cache.
*
* @param accountId account ID of the account that should be evicted
- * @throws IOException thrown if reindexing fails
*/
- void evict(@Nullable Account.Id accountId) throws IOException;
+ void evict(@Nullable Account.Id accountId);
- /** Evict all accounts from the cache, but doesn't trigger reindex of all accounts. */
- void evictAllNoReindex();
+ /** Evict all accounts from the cache. */
+ void evictAll();
}
diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java
index a8c3436..105a457 100644
--- a/java/com/google/gerrit/server/account/AccountCacheImpl.java
+++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -25,10 +25,8 @@
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.inject.Inject;
import com.google.inject.Module;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
import com.google.inject.TypeLiteral;
import com.google.inject.name.Named;
@@ -62,18 +60,15 @@
private final AllUsersName allUsersName;
private final ExternalIds externalIds;
private final LoadingCache<Account.Id, Optional<AccountState>> byId;
- private final Provider<AccountIndexer> indexer;
@Inject
AccountCacheImpl(
AllUsersName allUsersName,
ExternalIds externalIds,
- @Named(BYID_NAME) LoadingCache<Account.Id, Optional<AccountState>> byId,
- Provider<AccountIndexer> indexer) {
+ @Named(BYID_NAME) LoadingCache<Account.Id, Optional<AccountState>> byId) {
this.allUsersName = allUsersName;
this.externalIds = externalIds;
this.byId = byId;
- this.indexer = indexer;
}
@Override
@@ -111,15 +106,14 @@
}
@Override
- public void evict(@Nullable Account.Id accountId) throws IOException {
+ public void evict(@Nullable Account.Id accountId) {
if (accountId != null) {
byId.invalidate(accountId);
- indexer.get().index(accountId);
}
}
@Override
- public void evictAllNoReindex() {
+ public void evictAll() {
byId.invalidateAll();
}
diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java
index d86be16..79c57484 100644
--- a/java/com/google/gerrit/server/account/AccountManager.java
+++ b/java/com/google/gerrit/server/account/AccountManager.java
@@ -143,7 +143,21 @@
try (ReviewDb db = schema.open()) {
ExternalId id = externalIds.get(who.getExternalIdKey());
if (id == null) {
+ if (who.getUserName().isPresent()) {
+ ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, who.getUserName().get());
+ ExternalId existingId = externalIds.get(key);
+ if (existingId != null) {
+ // An inconsistency is detected in the database, having a record for scheme "username:"
+ // but no record for scheme "gerrit:". Try to recover by linking
+ // "gerrit:" identity to the existing account.
+ log.warn(
+ "User {} already has an account; link new identity to the existing account.",
+ who.getUserName());
+ return link(existingId.accountId(), who);
+ }
+ }
// New account, automatically create and return.
+ log.info("External ID not found. Attempting to create new account.");
return create(db, who);
}
@@ -383,6 +397,7 @@
public AuthResult link(Account.Id to, AuthRequest who)
throws AccountException, OrmException, IOException, ConfigInvalidException {
ExternalId extId = externalIds.get(who.getExternalIdKey());
+ log.info("Link another authentication identity to an existing account");
if (extId != null) {
if (!extId.accountId().equals(to)) {
throw new AccountException(
@@ -390,6 +405,7 @@
}
update(who, extId);
} else {
+ log.info("Linking new external ID to the existing account");
accountsUpdateProvider
.get()
.update(
diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java
index c8244b8..d6e3ce5 100644
--- a/java/com/google/gerrit/server/account/AccountsUpdate.java
+++ b/java/com/google/gerrit/server/account/AccountsUpdate.java
@@ -90,13 +90,23 @@
* are stored separately in the {@code refs/meta/external-ids} notes branch (see {@link
* ExternalIdNotes}).
*
- * <p>On updating an account the account is evicted from the account cache and thus reindexed. The
- * eviction from the account cache is done by the {@link ReindexAfterRefUpdate} class which receives
- * the event about updating the user branch that is triggered by this class. By passing an {@link
- * com.google.gerrit.server.account.externalids.ExternalIdNotes.FactoryNoReindex} factory as
- * parameter of {@link AccountsUpdate.Factory#create(IdentifiedUser, ExternalIdNotesLoader)},
- * reindexing and flushing the account from the account cache can be disabled. If external IDs are
- * updated, the ExternalIdCache is automatically updated.
+ * <p>On updating an account the account is evicted from the account cache and reindexed. The
+ * eviction from the account cache and the reindexing is done by the {@link ReindexAfterRefUpdate}
+ * class which receives the event about updating the user branch that is triggered by this class.
+ *
+ * <p>If external IDs are updated, the ExternalIdCache is automatically updated by {@link
+ * ExternalIdNotes}. In addition {@link ExternalIdNotes} takes care about evicting and reindexing
+ * corresponding accounts. This is needed because external ID updates don't touch the user branches.
+ * Hence in this case the accounts are not evicted and reindexed via {@link ReindexAfterRefUpdate}.
+ *
+ * <p>Reindexing and flushing accounts from the account cache can be disabled by
+ *
+ * <ul>
+ * <li>binding {@link GitReferenceUpdated#DISABLED} and
+ * <li>passing an {@link
+ * com.google.gerrit.server.account.externalids.ExternalIdNotes.FactoryNoReindex} factory as
+ * parameter of {@link AccountsUpdate.Factory#create(IdentifiedUser, ExternalIdNotesLoader)}
+ * </ul>
*
* <p>If there are concurrent account updates updating the user branch in NoteDb may fail with
* {@link LockFailureException}. In this case the account update is automatically retried and the
diff --git a/java/com/google/gerrit/server/account/GroupControl.java b/java/com/google/gerrit/server/account/GroupControl.java
index 4b223bf..5649629 100644
--- a/java/com/google/gerrit/server/account/GroupControl.java
+++ b/java/com/google/gerrit/server/account/GroupControl.java
@@ -177,6 +177,6 @@
if (group instanceof GroupDescription.Internal) {
return ((GroupDescription.Internal) group).isVisibleToAll() || isOwner();
}
- return false;
+ return canAdministrateServer();
}
}
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
index 29f69ba..972dfbd 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
@@ -31,7 +31,9 @@
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.VersionedMetaData;
+import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
@@ -109,23 +111,30 @@
public static class Factory implements ExternalIdNotesLoader {
private final ExternalIdCache externalIdCache;
private final AccountCache accountCache;
+ private final Provider<AccountIndexer> accountIndexer;
@Inject
- Factory(ExternalIdCache externalIdCache, AccountCache accountCache) {
+ Factory(
+ ExternalIdCache externalIdCache,
+ AccountCache accountCache,
+ Provider<AccountIndexer> accountIndexer) {
this.externalIdCache = externalIdCache;
this.accountCache = accountCache;
+ this.accountIndexer = accountIndexer;
}
@Override
public ExternalIdNotes load(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(externalIdCache, accountCache, allUsersRepo).load();
+ return new ExternalIdNotes(externalIdCache, accountCache, accountIndexer, allUsersRepo)
+ .load();
}
@Override
public ExternalIdNotes load(Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(externalIdCache, accountCache, allUsersRepo).load(rev);
+ return new ExternalIdNotes(externalIdCache, accountCache, accountIndexer, allUsersRepo)
+ .load(rev);
}
}
@@ -141,13 +150,13 @@
@Override
public ExternalIdNotes load(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(externalIdCache, null, allUsersRepo).load();
+ return new ExternalIdNotes(externalIdCache, null, null, allUsersRepo).load();
}
@Override
public ExternalIdNotes load(Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(externalIdCache, null, allUsersRepo).load(rev);
+ return new ExternalIdNotes(externalIdCache, null, null, allUsersRepo).load(rev);
}
}
@@ -159,7 +168,7 @@
*/
public static ExternalIdNotes loadReadOnly(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(new DisabledExternalIdCache(), null, allUsersRepo)
+ return new ExternalIdNotes(new DisabledExternalIdCache(), null, null, allUsersRepo)
.setReadOnly()
.load();
}
@@ -176,7 +185,7 @@
*/
public static ExternalIdNotes loadReadOnly(Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(new DisabledExternalIdCache(), null, allUsersRepo)
+ return new ExternalIdNotes(new DisabledExternalIdCache(), null, null, allUsersRepo)
.setReadOnly()
.load(rev);
}
@@ -189,11 +198,12 @@
*/
public static ExternalIdNotes loadNoCacheUpdate(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(new DisabledExternalIdCache(), null, allUsersRepo).load();
+ return new ExternalIdNotes(new DisabledExternalIdCache(), null, null, allUsersRepo).load();
}
private final ExternalIdCache externalIdCache;
@Nullable private final AccountCache accountCache;
+ @Nullable private final Provider<AccountIndexer> accountIndexer;
private final Repository repo;
private NoteMap noteMap;
@@ -211,9 +221,11 @@
private ExternalIdNotes(
ExternalIdCache externalIdCache,
@Nullable AccountCache accountCache,
+ @Nullable Provider<AccountIndexer> accountIndexer,
Repository allUsersRepo) {
this.externalIdCache = checkNotNull(externalIdCache, "externalIdCache");
this.accountCache = accountCache;
+ this.accountIndexer = accountIndexer;
this.repo = checkNotNull(allUsersRepo, "allUsersRepo");
}
@@ -598,7 +610,8 @@
*
* <p>No-op if this instance was created by {@link #loadNoCacheUpdate(Repository)}.
*
- * <p>No eviction from account cache if this instance was created by {@link FactoryNoReindex}.
+ * <p>No eviction from account cache and no reindex if this instance was created by {@link
+ * FactoryNoReindex}.
*/
public void updateCaches() throws IOException {
checkState(oldRev != null, "no changes committed yet");
@@ -614,14 +627,19 @@
externalIdCacheUpdates.getRemoved(),
externalIdCacheUpdates.getAdded());
- if (accountCache != null) {
+ if (accountCache != null || accountIndexer != null) {
for (Account.Id id :
Streams.concat(
externalIdCacheUpdates.getAdded().stream(),
externalIdCacheUpdates.getRemoved().stream())
.map(ExternalId::accountId)
.collect(toSet())) {
- accountCache.evict(id);
+ if (accountCache != null) {
+ accountCache.evict(id);
+ }
+ if (accountIndexer != null) {
+ accountIndexer.get().index(id);
+ }
}
}
diff --git a/java/com/google/gerrit/server/args4j/AccountGroupUUIDHandler.java b/java/com/google/gerrit/server/args4j/AccountGroupUUIDHandler.java
index 5958ed6..4d589a0 100644
--- a/java/com/google/gerrit/server/args4j/AccountGroupUUIDHandler.java
+++ b/java/com/google/gerrit/server/args4j/AccountGroupUUIDHandler.java
@@ -20,7 +20,6 @@
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupBackends;
import com.google.gerrit.server.account.GroupCache;
-import com.google.gerrit.server.account.GroupControl;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.InternalGroupDescription;
import com.google.inject.Inject;
@@ -35,20 +34,17 @@
public class AccountGroupUUIDHandler extends OptionHandler<AccountGroup.UUID> {
private final GroupBackend groupBackend;
- private final GroupControl.Factory groupControlFactory;
private final GroupCache groupCache;
@Inject
public AccountGroupUUIDHandler(
final GroupBackend groupBackend,
- final GroupControl.Factory groupControlFactory,
@Assisted final CmdLineParser parser,
@Assisted final OptionDef option,
@Assisted final Setter<AccountGroup.UUID> setter,
GroupCache groupCache) {
super(parser, option, setter);
this.groupBackend = groupBackend;
- this.groupControlFactory = groupControlFactory;
this.groupCache = groupCache;
}
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 113a6bf..19c9666 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -657,7 +657,7 @@
}
private boolean submittable(ChangeData cd) {
- return SubmitRecord.findOkRecord(cd.submitRecords(SUBMIT_RULE_OPTIONS_STRICT)).isPresent();
+ return SubmitRecord.allRecordsOK(cd.submitRecords(SUBMIT_RULE_OPTIONS_STRICT));
}
private List<SubmitRecord> submitRecords(ChangeData cd) {
diff --git a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
index 7a6c209..fc9ae4b 100644
--- a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
+++ b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
@@ -16,8 +16,8 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.Cache;
@@ -172,14 +172,14 @@
}
private void writeObject(ObjectOutputStream out) throws IOException {
- writeNotNull(out, prior);
- writeNotNull(out, next);
+ writeWithoutMarker(out, prior);
+ writeWithoutMarker(out, next);
out.writeUTF(strategyName);
}
private void readObject(ObjectInputStream in) throws IOException {
- prior = readNotNull(in);
- next = readNotNull(in);
+ prior = readWithoutMarker(in);
+ next = readWithoutMarker(in);
strategyName = in.readUTF();
}
}
diff --git a/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java b/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java
index 7ba18e8..4c80eab 100644
--- a/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java
+++ b/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java
@@ -19,8 +19,8 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.ioutil.BasicSerialization.readString;
import static com.google.gerrit.server.ioutil.BasicSerialization.writeString;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker;
import com.google.common.base.MoreObjects;
import com.google.common.cache.Cache;
@@ -155,8 +155,8 @@
}
private void writeObject(ObjectOutputStream out) throws IOException {
- writeNotNull(out, commit);
- writeNotNull(out, into);
+ writeWithoutMarker(out, commit);
+ writeWithoutMarker(out, into);
Character c = SUBMIT_TYPES.get(submitType);
if (c == null) {
throw new IOException("Invalid submit type: " + submitType);
@@ -166,8 +166,8 @@
}
private void readObject(ObjectInputStream in) throws IOException {
- commit = readNotNull(in);
- into = readNotNull(in);
+ commit = readWithoutMarker(in);
+ into = readWithoutMarker(in);
char t = in.readChar();
submitType = SUBMIT_TYPES.inverse().get(t);
if (submitType == null) {
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 77312be..9536d55 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -177,6 +177,7 @@
import com.google.gerrit.server.restapi.group.GroupModule;
import com.google.gerrit.server.rules.PrologModule;
import com.google.gerrit.server.rules.RulesCache;
+import com.google.gerrit.server.rules.SubmitRule;
import com.google.gerrit.server.ssh.SshAddressesModule;
import com.google.gerrit.server.tools.ToolsCatalog;
import com.google.gerrit.server.update.BatchUpdate;
@@ -391,6 +392,7 @@
DynamicSet.setOf(binder(), ActionVisitor.class);
DynamicItem.itemOf(binder(), MergeSuperSetComputation.class);
DynamicItem.itemOf(binder(), ProjectNameLockManager.class);
+ DynamicSet.setOf(binder(), SubmitRule.class);
DynamicMap.mapOf(binder(), MailFilter.class);
bind(MailFilter.class).annotatedWith(Exports.named("ListMailFilter")).to(ListMailFilter.class);
diff --git a/java/com/google/gerrit/server/config/PluginConfigFactory.java b/java/com/google/gerrit/server/config/PluginConfigFactory.java
index 6310c08..41230b2 100644
--- a/java/com/google/gerrit/server/config/PluginConfigFactory.java
+++ b/java/com/google/gerrit/server/config/PluginConfigFactory.java
@@ -288,7 +288,7 @@
*/
public Config getProjectPluginConfigWithInheritance(
Project.NameKey projectName, String pluginName) throws NoSuchProjectException {
- return getPluginConfig(projectName, pluginName).getWithInheritance();
+ return getPluginConfig(projectName, pluginName).getWithInheritance(false);
}
/**
@@ -310,7 +310,55 @@
*/
public Config getProjectPluginConfigWithInheritance(
ProjectState projectState, String pluginName) {
- return projectState.getConfig(pluginName + EXTENSION).getWithInheritance();
+ return projectState.getConfig(pluginName + EXTENSION).getWithInheritance(false);
+ }
+
+ /**
+ * Returns the configuration for the specified plugin that is stored in the '{@code
+ * <plugin-name>.config}' file in the 'refs/meta/config' branch of the specified project.
+ * Parameters from the '{@code <plugin-name>.config}' of the parent project are appended to this
+ * project's '{@code <plugin-name>.config}' files.
+ *
+ * <p>E.g.: child project: [mySection "mySubsection"] myKey = childValue
+ *
+ * <p>parent project: [mySection "mySubsection"] myKey = parentValue anotherKey = someValue
+ *
+ * <p>return: [mySection "mySubsection"] myKey = childValue myKey = parentValue anotherKey =
+ * someValue
+ *
+ * @param projectName the name of the project for which the plugin configuration should be
+ * returned
+ * @param pluginName the name of the plugin for which the configuration should be returned
+ * @return the plugin configuration from the '{@code <plugin-name>.config}' file of the specified
+ * project with parameters from the parent projects appended to the project values
+ * @throws NoSuchProjectException thrown if the specified project does not exist
+ */
+ public Config getProjectPluginConfigWithMergedInheritance(
+ Project.NameKey projectName, String pluginName) throws NoSuchProjectException {
+ return getPluginConfig(projectName, pluginName).getWithInheritance(true);
+ }
+
+ /**
+ * Returns the configuration for the specified plugin that is stored in the '{@code
+ * <plugin-name>.config}' file in the 'refs/meta/config' branch of the specified project.
+ * Parameters from the '{@code <plugin-name>.config}' of the parent project are appended to this
+ * project's '{@code <plugin-name>.config}' files.
+ *
+ * <p>E.g.: child project: [mySection "mySubsection"] myKey = childValue
+ *
+ * <p>parent project: [mySection "mySubsection"] myKey = parentValue anotherKey = someValue
+ *
+ * <p>return: [mySection "mySubsection"] myKey = childValue myKey = parentValue anotherKey =
+ * someValue
+ *
+ * @param projectState the project for which the plugin configuration should be returned
+ * @param pluginName the name of the plugin for which the configuration should be returned
+ * @return the plugin configuration from the '{@code <plugin-name>.config}' file of the specified
+ * project with inheriting non-set parameters from the parent projects
+ */
+ public Config getProjectPluginConfigWithMergedInheritance(
+ ProjectState projectState, String pluginName) {
+ return projectState.getConfig(pluginName + EXTENSION).getWithInheritance(true);
}
private ProjectLevelConfig getPluginConfig(Project.NameKey projectName, String pluginName)
diff --git a/java/com/google/gerrit/server/data/SubmitLabelAttribute.java b/java/com/google/gerrit/server/data/SubmitLabelAttribute.java
index 1b3c6a48..fec8f7f 100644
--- a/java/com/google/gerrit/server/data/SubmitLabelAttribute.java
+++ b/java/com/google/gerrit/server/data/SubmitLabelAttribute.java
@@ -14,6 +14,10 @@
package com.google.gerrit.server.data;
+/**
+ * Represents a {@link com.google.gerrit.common.data.SubmitRecord.Label} that does not depend on
+ * Gerrit internal classes, to be serialized.
+ */
public class SubmitLabelAttribute {
public String label;
public String status;
diff --git a/java/com/google/gerrit/server/data/SubmitRecordAttribute.java b/java/com/google/gerrit/server/data/SubmitRecordAttribute.java
index fec870f..2c3d401 100644
--- a/java/com/google/gerrit/server/data/SubmitRecordAttribute.java
+++ b/java/com/google/gerrit/server/data/SubmitRecordAttribute.java
@@ -16,7 +16,12 @@
import java.util.List;
+/**
+ * Represents a {@link com.google.gerrit.common.data.SubmitRecord} that does not depend on Gerrit
+ * internal classes, to be serialized.
+ */
public class SubmitRecordAttribute {
public String status;
public List<SubmitLabelAttribute> labels;
+ public List<SubmitRequirementAttribute> requirements;
}
diff --git a/java/com/google/gerrit/server/data/SubmitRequirementAttribute.java b/java/com/google/gerrit/server/data/SubmitRequirementAttribute.java
new file mode 100644
index 0000000..8a0ea51
--- /dev/null
+++ b/java/com/google/gerrit/server/data/SubmitRequirementAttribute.java
@@ -0,0 +1,25 @@
+// Copyright (C) 2018 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.data;
+
+/**
+ * Represents a {@link com.google.gerrit.common.data.SubmitRequirement} that does not depend on
+ * Gerrit internal classes, to be serialized
+ */
+public class SubmitRequirementAttribute {
+ public String shortReason;
+ public String fullReason;
+ public String label;
+}
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index d356c2c..d3d52e4 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/java/com/google/gerrit/server/events/EventFactory.java
@@ -23,6 +23,7 @@
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitRequirement;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
@@ -52,6 +53,7 @@
import com.google.gerrit.server.data.RefUpdateAttribute;
import com.google.gerrit.server.data.SubmitLabelAttribute;
import com.google.gerrit.server.data.SubmitRecordAttribute;
+import com.google.gerrit.server.data.SubmitRequirementAttribute;
import com.google.gerrit.server.data.TrackingIdAttribute;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchList;
@@ -229,6 +231,7 @@
sa.status = submitRecord.status.name();
if (submitRecord.status != SubmitRecord.Status.RULE_ERROR) {
addSubmitRecordLabels(submitRecord, sa);
+ addSubmitRecordRequirements(submitRecord, sa);
}
ca.submitRecords.add(sa);
}
@@ -253,6 +256,19 @@
}
}
+ private void addSubmitRecordRequirements(SubmitRecord submitRecord, SubmitRecordAttribute sa) {
+ if (submitRecord.requirements != null && !submitRecord.requirements.isEmpty()) {
+ sa.requirements = new ArrayList<>();
+ for (SubmitRequirement req : submitRecord.requirements) {
+ SubmitRequirementAttribute re = new SubmitRequirementAttribute();
+ re.shortReason = req.shortReason();
+ re.fullReason = req.fullReason();
+ re.label = req.label().orElse(null);
+ sa.requirements.add(re);
+ }
+ }
+ }
+
public void addDependencies(RevWalk rw, ChangeAttribute ca, Change change, PatchSet currentPs) {
if (change == null || currentPs == null) {
return;
diff --git a/java/com/google/gerrit/server/git/MergeOp.java b/java/com/google/gerrit/server/git/MergeOp.java
index b4a66f4..436faa7 100644
--- a/java/com/google/gerrit/server/git/MergeOp.java
+++ b/java/com/google/gerrit/server/git/MergeOp.java
@@ -291,7 +291,7 @@
throw new ResourceConflictException("missing current patch set for change " + cd.getId());
}
List<SubmitRecord> results = getSubmitRecords(cd, allowClosed);
- if (SubmitRecord.findOkRecord(results).isPresent()) {
+ if (SubmitRecord.allRecordsOK(results)) {
// Rules supplied a valid solution.
return;
} else if (results.isEmpty()) {
@@ -303,6 +303,9 @@
for (SubmitRecord record : results) {
switch (record.status) {
+ case OK:
+ break;
+
case CLOSED:
throw new ResourceConflictException("change is closed");
@@ -313,7 +316,6 @@
throw new ResourceConflictException(describeLabels(cd, record.labels));
case FORCED:
- case OK:
default:
throw new IllegalStateException(
String.format(
diff --git a/java/com/google/gerrit/server/git/ProjectLevelConfig.java b/java/com/google/gerrit/server/git/ProjectLevelConfig.java
index 18dcef8..2044db0 100644
--- a/java/com/google/gerrit/server/git/ProjectLevelConfig.java
+++ b/java/com/google/gerrit/server/git/ProjectLevelConfig.java
@@ -14,12 +14,16 @@
package com.google.gerrit.server.git;
+import static java.util.stream.Collectors.toList;
+
import com.google.common.collect.Iterables;
+import com.google.common.collect.Streams;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.project.ProjectState;
import java.io.IOException;
import java.util.Arrays;
import java.util.Set;
+import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Config;
@@ -53,6 +57,22 @@
}
public Config getWithInheritance() {
+ return getWithInheritance(false);
+ }
+
+ /**
+ * Get a Config that includes the values from all parent projects.
+ *
+ * <p>Merging means that matching sections/subsection will be merged to include the values from
+ * both parent and child config.
+ *
+ * <p>No merging means that matching sections/subsections in the child project will replace the
+ * corresponding value from the parent.
+ *
+ * @param merge whether to merge parent values with child values or not.
+ * @return a combined config.
+ */
+ public Config getWithInheritance(boolean merge) {
Config cfgWithInheritance = new Config();
try {
cfgWithInheritance.fromText(get().toText());
@@ -65,21 +85,41 @@
for (String section : parentCfg.getSections()) {
Set<String> allNames = get().getNames(section);
for (String name : parentCfg.getNames(section)) {
+ String[] parentValues = parentCfg.getStringList(section, null, name);
if (!allNames.contains(name)) {
+ cfgWithInheritance.setStringList(section, null, name, Arrays.asList(parentValues));
+ } else if (merge) {
cfgWithInheritance.setStringList(
- section, null, name, Arrays.asList(parentCfg.getStringList(section, null, name)));
+ section,
+ null,
+ name,
+ Stream.concat(
+ Arrays.stream(cfg.getStringList(section, null, name)),
+ Arrays.stream(parentValues))
+ .sorted()
+ .distinct()
+ .collect(toList()));
}
}
for (String subsection : parentCfg.getSubsections(section)) {
allNames = get().getNames(section, subsection);
for (String name : parentCfg.getNames(section, subsection)) {
+ String[] parentValues = parentCfg.getStringList(section, subsection, name);
if (!allNames.contains(name)) {
cfgWithInheritance.setStringList(
+ section, subsection, name, Arrays.asList(parentValues));
+ } else if (merge) {
+ cfgWithInheritance.setStringList(
section,
subsection,
name,
- Arrays.asList(parentCfg.getStringList(section, subsection, name)));
+ Streams.concat(
+ Arrays.stream(cfg.getStringList(section, subsection, name)),
+ Arrays.stream(parentValues))
+ .sorted()
+ .distinct()
+ .collect(toList()));
}
}
}
diff --git a/java/com/google/gerrit/server/git/SubmoduleOp.java b/java/com/google/gerrit/server/git/SubmoduleOp.java
index 56ed432..0e3e146 100644
--- a/java/com/google/gerrit/server/git/SubmoduleOp.java
+++ b/java/com/google/gerrit/server/git/SubmoduleOp.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.git;
+import static java.util.Comparator.comparing;
+
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.SetMultimap;
@@ -47,6 +49,7 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
@@ -416,7 +419,10 @@
DirCache dc = readTree(or.rw, currentCommit);
DirCacheEditor ed = dc.editor();
int count = 0;
- for (SubmoduleSubscription s : targets.get(subscriber)) {
+
+ List<SubmoduleSubscription> subscriptions = new ArrayList<>(targets.get(subscriber));
+ Collections.sort(subscriptions, comparing((SubmoduleSubscription s) -> s.getPath()));
+ for (SubmoduleSubscription s : subscriptions) {
if (count > 0) {
msgbuf.append("\n\n");
}
diff --git a/java/com/google/gerrit/server/git/TagSet.java b/java/com/google/gerrit/server/git/TagSet.java
index 1f54cd0..118223b 100644
--- a/java/com/google/gerrit/server/git/TagSet.java
+++ b/java/com/google/gerrit/server/git/TagSet.java
@@ -14,8 +14,8 @@
package com.google.gerrit.server.git;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
@@ -194,13 +194,13 @@
for (int i = 0; i < refCnt; i++) {
String name = in.readUTF();
int flag = in.readInt();
- ObjectId id = readNotNull(in);
+ ObjectId id = readWithoutMarker(in);
refs.put(name, new CachedRef(flag, id));
}
int tagCnt = in.readInt();
for (int i = 0; i < tagCnt; i++) {
- ObjectId id = readNotNull(in);
+ ObjectId id = readWithoutMarker(in);
BitSet flags = (BitSet) in.readObject();
tags.add(new Tag(id, flags));
}
@@ -211,12 +211,12 @@
for (Map.Entry<String, CachedRef> e : refs.entrySet()) {
out.writeUTF(e.getKey());
out.writeInt(e.getValue().flag);
- writeNotNull(out, e.getValue().get());
+ writeWithoutMarker(out, e.getValue().get());
}
out.writeInt(tags.size());
for (Tag tag : tags) {
- writeNotNull(out, tag);
+ writeWithoutMarker(out, tag);
out.writeObject(tag.refFlags);
}
}
diff --git a/java/com/google/gerrit/server/index/account/AccountIndexerImpl.java b/java/com/google/gerrit/server/index/account/AccountIndexerImpl.java
index d055a46..b7bb0dd 100644
--- a/java/com/google/gerrit/server/index/account/AccountIndexerImpl.java
+++ b/java/com/google/gerrit/server/index/account/AccountIndexerImpl.java
@@ -14,11 +14,7 @@
package com.google.gerrit.server.index.account;
-import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH;
-
import com.google.common.collect.ImmutableSet;
-import com.google.common.util.concurrent.Futures;
-import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.events.AccountIndexedListener;
import com.google.gerrit.extensions.registration.DynamicSet;
@@ -26,17 +22,12 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.index.IndexExecutor;
-import com.google.gerrit.server.index.IndexUtils;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.Optional;
-import java.util.concurrent.Future;
-import org.eclipse.jgit.lib.Config;
public class AccountIndexerImpl implements AccountIndexer {
public interface Factory {
@@ -48,8 +39,6 @@
private final AccountCache byIdCache;
private final DynamicSet<AccountIndexedListener> indexedListener;
private final StalenessChecker stalenessChecker;
- private final ListeningExecutorService batchExecutor;
- private final boolean autoReindexIfStale;
@Nullable private final AccountIndexCollection indexes;
@Nullable private final AccountIndex index;
@@ -58,14 +47,10 @@
AccountCache byIdCache,
DynamicSet<AccountIndexedListener> indexedListener,
StalenessChecker stalenessChecker,
- @IndexExecutor(BATCH) ListeningExecutorService batchExecutor,
- @GerritServerConfig Config config,
@Assisted AccountIndexCollection indexes) {
this.byIdCache = byIdCache;
this.indexedListener = indexedListener;
this.stalenessChecker = stalenessChecker;
- this.batchExecutor = batchExecutor;
- this.autoReindexIfStale = autoReindexIfStale(config);
this.indexes = indexes;
this.index = null;
}
@@ -75,14 +60,10 @@
AccountCache byIdCache,
DynamicSet<AccountIndexedListener> indexedListener,
StalenessChecker stalenessChecker,
- @IndexExecutor(BATCH) ListeningExecutorService batchExecutor,
- @GerritServerConfig Config config,
@Assisted @Nullable AccountIndex index) {
this.byIdCache = byIdCache;
this.indexedListener = indexedListener;
this.stalenessChecker = stalenessChecker;
- this.batchExecutor = batchExecutor;
- this.autoReindexIfStale = autoReindexIfStale(config);
this.indexes = null;
this.index = index;
}
@@ -90,6 +71,8 @@
@Override
public void index(Account.Id id) throws IOException {
for (Index<Account.Id, AccountState> i : getWriteIndexes()) {
+ // Evict the cache to get an up-to-date value for sure.
+ byIdCache.evict(id);
Optional<AccountState> accountState = byIdCache.get(id);
if (accountState.isPresent()) {
i.replace(accountState.get());
@@ -98,7 +81,6 @@
}
}
fireAccountIndexedEvent(id.get());
- autoReindexIfStale(id);
}
@Override
@@ -110,35 +92,6 @@
return false;
}
- private static boolean autoReindexIfStale(Config cfg) {
- return cfg.getBoolean("index", null, "autoReindexIfStale", true);
- }
-
- private void autoReindexIfStale(Account.Id id) {
- if (autoReindexIfStale) {
- // Don't retry indefinitely; if this fails the account will be stale.
- @SuppressWarnings("unused")
- Future<?> possiblyIgnoredError = reindexIfStaleAsync(id);
- }
- }
-
- /**
- * Asynchronously check if a account is stale, and reindex if it is.
- *
- * <p>Always run on the batch executor, even if this indexer instance is configured to use a
- * different executor.
- *
- * @param id the ID of the account.
- * @return future for reindexing the account; returns true if the account was stale.
- */
- @SuppressWarnings("deprecation")
- private com.google.common.util.concurrent.CheckedFuture<Boolean, IOException> reindexIfStaleAsync(
- Account.Id id) {
- return Futures.makeChecked(
- Futures.nonCancellationPropagating(batchExecutor.submit(() -> reindexIfStale(id))),
- IndexUtils.MAPPER);
- }
-
private void fireAccountIndexedEvent(int id) {
for (AccountIndexedListener listener : indexedListener) {
listener.onAccountIndexed(id);
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 344048f..262c732 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -36,7 +36,9 @@
import com.google.common.collect.Lists;
import com.google.common.collect.Table;
import com.google.common.primitives.Longs;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitRequirement;
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.SchemaUtil;
import com.google.gerrit.reviewdb.client.Account;
@@ -657,8 +659,15 @@
Integer appliedBy;
}
+ static class StoredRequirement {
+ String shortReason;
+ String fullReason;
+ @Nullable String label;
+ }
+
SubmitRecord.Status status;
List<StoredLabel> labels;
+ List<StoredRequirement> requirements;
String errorMessage;
StoredSubmitRecord(SubmitRecord rec) {
@@ -674,6 +683,16 @@
this.labels.add(sl);
}
}
+ if (rec.requirements != null) {
+ this.requirements = new ArrayList<>(rec.requirements.size());
+ for (SubmitRequirement requirement : rec.requirements) {
+ StoredRequirement sr = new StoredRequirement();
+ sr.shortReason = requirement.shortReason();
+ sr.fullReason = requirement.fullReason();
+ sr.label = requirement.label().orElse(null);
+ this.requirements.add(sr);
+ }
+ }
}
private SubmitRecord toSubmitRecord() {
@@ -690,6 +709,15 @@
rec.labels.add(srl);
}
}
+ if (requirements != null) {
+ rec.requirements = new ArrayList<>(requirements.size());
+ for (StoredRequirement requirement : requirements) {
+ SubmitRequirement sr =
+ new SubmitRequirement(
+ requirement.shortReason, requirement.fullReason, requirement.label);
+ rec.requirements.add(sr);
+ }
+ }
return rec;
}
}
diff --git a/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java b/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
index 2e1da65..0b76b1e 100644
--- a/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
+++ b/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.QueueProvider.QueueType;
import com.google.gerrit.server.index.IndexExecutor;
+import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.InternalChangeQuery;
@@ -59,6 +60,7 @@
private final ChangeNotes.Factory notesFactory;
private final AllUsersName allUsersName;
private final AccountCache accountCache;
+ private final Provider<AccountIndexer> indexer;
private final ListeningExecutorService executor;
private final boolean enabled;
@@ -72,6 +74,7 @@
ChangeNotes.Factory notesFactory,
AllUsersName allUsersName,
AccountCache accountCache,
+ Provider<AccountIndexer> indexer,
@IndexExecutor(QueueType.BATCH) ListeningExecutorService executor) {
this.requestContext = requestContext;
this.queryProvider = queryProvider;
@@ -80,6 +83,7 @@
this.notesFactory = notesFactory;
this.allUsersName = allUsersName;
this.accountCache = accountCache;
+ this.indexer = indexer;
this.executor = executor;
this.enabled = cfg.getBoolean("index", null, "reindexAfterRefUpdate", true);
}
@@ -91,6 +95,7 @@
if (accountId != null && !event.getRefName().startsWith(RefNames.REFS_STARRED_CHANGES)) {
try {
accountCache.evict(accountId);
+ indexer.get().index(accountId);
} catch (IOException e) {
log.error(String.format("Reindex account %s failed.", accountId), e);
}
diff --git a/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java b/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java
index a1da4ea..fcbdc57 100644
--- a/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java
+++ b/java/com/google/gerrit/server/index/group/GroupIndexerImpl.java
@@ -14,29 +14,20 @@
package com.google.gerrit.server.index.group;
-import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH;
-
import com.google.common.collect.ImmutableSet;
-import com.google.common.util.concurrent.Futures;
-import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.events.GroupIndexedListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.index.Index;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.account.GroupCache;
-import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.group.InternalGroup;
-import com.google.gerrit.server.index.IndexExecutor;
-import com.google.gerrit.server.index.IndexUtils;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.Optional;
-import java.util.concurrent.Future;
-import org.eclipse.jgit.lib.Config;
public class GroupIndexerImpl implements GroupIndexer {
public interface Factory {
@@ -48,8 +39,6 @@
private final GroupCache groupCache;
private final DynamicSet<GroupIndexedListener> indexedListener;
private final StalenessChecker stalenessChecker;
- private final ListeningExecutorService batchExecutor;
- private final boolean autoReindexIfStale;
@Nullable private final GroupIndexCollection indexes;
@Nullable private final GroupIndex index;
@@ -58,14 +47,10 @@
GroupCache groupCache,
DynamicSet<GroupIndexedListener> indexedListener,
StalenessChecker stalenessChecker,
- @IndexExecutor(BATCH) ListeningExecutorService batchExecutor,
- @GerritServerConfig Config config,
@Assisted GroupIndexCollection indexes) {
this.groupCache = groupCache;
this.indexedListener = indexedListener;
this.stalenessChecker = stalenessChecker;
- this.batchExecutor = batchExecutor;
- this.autoReindexIfStale = autoReindexIfStale(config);
this.indexes = indexes;
this.index = null;
}
@@ -75,14 +60,10 @@
GroupCache groupCache,
DynamicSet<GroupIndexedListener> indexedListener,
StalenessChecker stalenessChecker,
- @IndexExecutor(BATCH) ListeningExecutorService batchExecutor,
- @GerritServerConfig Config config,
@Assisted @Nullable GroupIndex index) {
this.groupCache = groupCache;
this.indexedListener = indexedListener;
this.stalenessChecker = stalenessChecker;
- this.batchExecutor = batchExecutor;
- this.autoReindexIfStale = autoReindexIfStale(config);
this.indexes = null;
this.index = index;
}
@@ -100,7 +81,6 @@
}
}
fireGroupIndexedEvent(uuid.get());
- autoReindexIfStale(uuid);
}
@Override
@@ -112,35 +92,6 @@
return false;
}
- private static boolean autoReindexIfStale(Config cfg) {
- return cfg.getBoolean("index", null, "autoReindexIfStale", true);
- }
-
- private void autoReindexIfStale(AccountGroup.UUID uuid) {
- if (autoReindexIfStale) {
- // Don't retry indefinitely; if this fails the group will be stale.
- @SuppressWarnings("unused")
- Future<?> possiblyIgnoredError = reindexIfStaleAsync(uuid);
- }
- }
-
- /**
- * Asynchronously check if a group is stale, and reindex if it is.
- *
- * <p>Always run on the batch executor, even if this indexer instance is configured to use a
- * different executor.
- *
- * @param uuid the unique identifier of the group.
- * @return future for reindexing the group; returns true if the group was stale.
- */
- @SuppressWarnings("deprecation")
- private com.google.common.util.concurrent.CheckedFuture<Boolean, IOException> reindexIfStaleAsync(
- AccountGroup.UUID uuid) {
- return Futures.makeChecked(
- Futures.nonCancellationPropagating(batchExecutor.submit(() -> reindexIfStale(uuid))),
- IndexUtils.MAPPER);
- }
-
private void fireGroupIndexedEvent(String uuid) {
for (GroupIndexedListener listener : indexedListener) {
listener.onGroupIndexed(uuid);
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index 7010be7..ccc5859 100644
--- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -728,6 +728,7 @@
msg.append('\n');
}
}
+ // TODO(maximeg) We might want to list plugins that validated this submission.
}
}
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
index b9f5fe6..c599c8e 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
@@ -454,7 +454,11 @@
update.getProjectName(),
projectName);
checkState(staged == null, "cannot add new update after staging");
- changeUpdates.put(update.getRefName(), update);
+ checkArgument(
+ !rewriters.containsKey(update.getRefName()),
+ "cannot update & rewrite ref %s in one BatchUpdate",
+ update.getRefName());
+
ChangeDraftUpdate du = update.getDraftUpdate();
if (du != null) {
draftUpdates.put(du.getRefName(), du);
@@ -463,10 +467,17 @@
if (rcu != null) {
robotCommentUpdates.put(rcu.getRefName(), rcu);
}
- DeleteCommentRewriter deleteCommentRewriter = update.getDeleteCommentRewriter();
- if (deleteCommentRewriter != null) {
- rewriters.put(deleteCommentRewriter.getRefName(), deleteCommentRewriter);
+ DeleteCommentRewriter rwt = update.getDeleteCommentRewriter();
+ if (rwt != null) {
+ // Checks whether there is any ChangeUpdate added earlier trying to update the same ref.
+ checkArgument(
+ !changeUpdates.containsKey(rwt.getRefName()),
+ "cannot update & rewrite ref %s in one BatchUpdate",
+ rwt.getRefName());
+ rewriters.put(rwt.getRefName(), rwt);
}
+
+ changeUpdates.put(update.getRefName(), update);
}
public void add(ChangeDraftUpdate draftUpdate) {
@@ -695,17 +706,6 @@
addUpdates(robotCommentUpdates, changeRepo);
}
if (!rewriters.isEmpty()) {
- Optional<String> conflictKey =
- rewriters
- .keySet()
- .stream()
- .filter(k -> (draftUpdates.containsKey(k) || robotCommentUpdates.containsKey(k)))
- .findAny();
- if (conflictKey.isPresent()) {
- throw new IllegalArgumentException(
- String.format(
- "cannot update and rewrite ref %s in one BatchUpdate", conflictKey.get()));
- }
addRewrites(rewriters, changeRepo);
}
diff --git a/java/com/google/gerrit/server/patch/DiffSummaryKey.java b/java/com/google/gerrit/server/patch/DiffSummaryKey.java
index 0a02e36..0f4757a 100644
--- a/java/com/google/gerrit/server/patch/DiffSummaryKey.java
+++ b/java/com/google/gerrit/server/patch/DiffSummaryKey.java
@@ -14,10 +14,10 @@
package com.google.gerrit.server.patch;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readCanBeNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeCanBeNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.read;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.write;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker;
import com.google.common.base.Preconditions;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
@@ -93,9 +93,9 @@
}
private void writeObject(ObjectOutputStream out) throws IOException {
- writeCanBeNull(out, oldId);
+ write(out, oldId);
out.writeInt(parentNum == null ? 0 : parentNum);
- writeNotNull(out, newId);
+ writeWithoutMarker(out, newId);
Character c = PatchListKey.WHITESPACE_TYPES.get(whitespace);
if (c == null) {
throw new IOException("Invalid whitespace type: " + whitespace);
@@ -104,10 +104,10 @@
}
private void readObject(ObjectInputStream in) throws IOException {
- oldId = readCanBeNull(in);
+ oldId = read(in);
int n = in.readInt();
parentNum = n == 0 ? null : Integer.valueOf(n);
- newId = readNotNull(in);
+ newId = readWithoutMarker(in);
char t = in.readChar();
whitespace = PatchListKey.WHITESPACE_TYPES.inverse().get(t);
if (whitespace == null) {
diff --git a/java/com/google/gerrit/server/patch/PatchList.java b/java/com/google/gerrit/server/patch/PatchList.java
index 16ede58..cf5df4a 100644
--- a/java/com/google/gerrit/server/patch/PatchList.java
+++ b/java/com/google/gerrit/server/patch/PatchList.java
@@ -18,10 +18,10 @@
import static com.google.gerrit.server.ioutil.BasicSerialization.readVarInt32;
import static com.google.gerrit.server.ioutil.BasicSerialization.writeBytes;
import static com.google.gerrit.server.ioutil.BasicSerialization.writeVarInt32;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readCanBeNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeCanBeNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.read;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.write;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker;
import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.common.Nullable;
@@ -173,8 +173,8 @@
private void writeObject(ObjectOutputStream output) throws IOException {
final ByteArrayOutputStream buf = new ByteArrayOutputStream();
try (DeflaterOutputStream out = new DeflaterOutputStream(buf)) {
- writeCanBeNull(out, oldId);
- writeNotNull(out, newId);
+ write(out, oldId);
+ writeWithoutMarker(out, newId);
writeVarInt32(out, isMerge ? 1 : 0);
comparisonType.writeTo(out);
writeVarInt32(out, insertions);
@@ -190,8 +190,8 @@
private void readObject(ObjectInputStream input) throws IOException {
final ByteArrayInputStream buf = new ByteArrayInputStream(readBytes(input));
try (InflaterInputStream in = new InflaterInputStream(buf)) {
- oldId = readCanBeNull(in);
- newId = readNotNull(in);
+ oldId = read(in);
+ newId = readWithoutMarker(in);
isMerge = readVarInt32(in) != 0;
comparisonType = ComparisonType.readFrom(in);
insertions = readVarInt32(in);
diff --git a/java/com/google/gerrit/server/patch/PatchListKey.java b/java/com/google/gerrit/server/patch/PatchListKey.java
index 73e82a1..e5be70c 100644
--- a/java/com/google/gerrit/server/patch/PatchListKey.java
+++ b/java/com/google/gerrit/server/patch/PatchListKey.java
@@ -15,10 +15,10 @@
package com.google.gerrit.server.patch;
import static com.google.common.base.Preconditions.checkState;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readCanBeNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeCanBeNull;
-import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.read;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.write;
+import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker;
import com.google.common.collect.ImmutableBiMap;
import com.google.gerrit.common.Nullable;
@@ -186,9 +186,9 @@
}
private void writeObject(ObjectOutputStream out) throws IOException {
- writeCanBeNull(out, oldId);
+ write(out, oldId);
out.writeInt(parentNum == null ? 0 : parentNum);
- writeNotNull(out, newId);
+ writeWithoutMarker(out, newId);
Character c = WHITESPACE_TYPES.get(whitespace);
if (c == null) {
throw new IOException("Invalid whitespace type: " + whitespace);
@@ -198,10 +198,10 @@
}
private void readObject(ObjectInputStream in) throws IOException {
- oldId = readCanBeNull(in);
+ oldId = read(in);
int n = in.readInt();
parentNum = n == 0 ? null : Integer.valueOf(n);
- newId = readNotNull(in);
+ newId = readWithoutMarker(in);
char t = in.readChar();
whitespace = WHITESPACE_TYPES.inverse().get(t);
if (whitespace == null) {
diff --git a/java/com/google/gerrit/server/project/RuleEvalException.java b/java/com/google/gerrit/server/project/RuleEvalException.java
index 9dae11c..7e3c739 100644
--- a/java/com/google/gerrit/server/project/RuleEvalException.java
+++ b/java/com/google/gerrit/server/project/RuleEvalException.java
@@ -14,13 +14,14 @@
package com.google.gerrit.server.project;
-@SuppressWarnings("serial")
public class RuleEvalException extends Exception {
+ private static final long serialVersionUID = 1L;
+
public RuleEvalException(String message) {
super(message);
}
- RuleEvalException(String message, Throwable cause) {
+ public RuleEvalException(String message, Throwable cause) {
super(message, cause);
}
}
diff --git a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
index c1da015..cb01b81 100644
--- a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
+++ b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
@@ -14,38 +14,21 @@
package com.google.gerrit.server.project;
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkState;
-
-import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.data.SubmitTypeRecord;
-import com.google.gerrit.extensions.client.SubmitType;
-import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.server.account.AccountCache;
-import com.google.gerrit.server.account.Accounts;
-import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.rules.PrologEnvironment;
-import com.google.gerrit.server.rules.StoredValues;
+import com.google.gerrit.server.rules.PrologRule;
+import com.google.gerrit.server.rules.SubmitRule;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
-import com.googlecode.prolog_cafe.exceptions.CompileException;
-import com.googlecode.prolog_cafe.exceptions.ReductionLimitException;
-import com.googlecode.prolog_cafe.lang.IntegerTerm;
-import com.googlecode.prolog_cafe.lang.ListTerm;
-import com.googlecode.prolog_cafe.lang.Prolog;
-import com.googlecode.prolog_cafe.lang.StructureTerm;
-import com.googlecode.prolog_cafe.lang.SymbolTerm;
-import com.googlecode.prolog_cafe.lang.Term;
-import com.googlecode.prolog_cafe.lang.VariableTerm;
-import java.io.StringReader;
-import java.util.ArrayList;
+import java.util.Collection;
import java.util.Collections;
import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -55,9 +38,31 @@
*/
public class SubmitRuleEvaluator {
private static final Logger log = LoggerFactory.getLogger(SubmitRuleEvaluator.class);
-
private static final String DEFAULT_MSG = "Error evaluating project rules, check server log";
+ private final ProjectCache projectCache;
+ private final PrologRule prologRule;
+ private final DynamicSet<SubmitRule> submitRules;
+ private final SubmitRuleOptions opts;
+
+ public interface Factory {
+ /** Returns a new {@link SubmitRuleEvaluator} with the specified options */
+ SubmitRuleEvaluator create(SubmitRuleOptions options);
+ }
+
+ @Inject
+ private SubmitRuleEvaluator(
+ ProjectCache projectCache,
+ PrologRule prologRule,
+ DynamicSet<SubmitRule> submitRules,
+ @Assisted SubmitRuleOptions options) {
+ this.projectCache = projectCache;
+ this.prologRule = prologRule;
+ this.submitRules = submitRules;
+
+ this.opts = options;
+ }
+
public static List<SubmitRecord> defaultRuleError() {
return createRuleError(DEFAULT_MSG);
}
@@ -74,143 +79,25 @@
}
/**
- * Exception thrown when the label term of a submit record unexpectedly didn't contain a user
- * term.
- */
- private static class UserTermExpected extends Exception {
- private static final long serialVersionUID = 1L;
-
- UserTermExpected(SubmitRecord.Label label) {
- super(String.format("A label with the status %s must contain a user.", label.toString()));
- }
- }
-
- public interface Factory {
- SubmitRuleEvaluator create(ChangeData cd);
- }
-
- private final AccountCache accountCache;
- private final Accounts accounts;
- private final Emails emails;
- private final ProjectCache projectCache;
- private final ChangeData cd;
-
- private SubmitRuleOptions.Builder optsBuilder = SubmitRuleOptions.builder();
- private SubmitRuleOptions opts;
- private Change change;
- private PatchSet patchSet;
- private boolean logErrors = true;
- private long reductionsConsumed;
- private ProjectState projectState;
-
- private Term submitRule;
-
- @Inject
- SubmitRuleEvaluator(
- AccountCache accountCache,
- Accounts accounts,
- Emails emails,
- ProjectCache projectCache,
- @Assisted ChangeData cd) {
- this.accountCache = accountCache;
- this.accounts = accounts;
- this.emails = emails;
- this.projectCache = projectCache;
- this.cd = cd;
- }
-
- /**
- * @return immutable snapshot of options configured so far. If neither {@link #getSubmitRule()}
- * nor {@link #getSubmitType()} have been called yet, state within this instance is still
- * mutable, so may change before evaluation. The instance's options are frozen at evaluation
- * time.
- */
- public SubmitRuleOptions getOptions() {
- if (opts != null) {
- return opts;
- }
- return optsBuilder.build();
- }
-
- public SubmitRuleEvaluator setOptions(SubmitRuleOptions opts) {
- checkNotStarted();
- if (opts != null) {
- optsBuilder = opts.toBuilder();
- } else {
- optsBuilder = SubmitRuleOptions.builder();
- }
- return this;
- }
-
- /**
- * @param ps patch set of the change to evaluate. If not set, the current patch set will be loaded
- * from {@link #evaluate()} or {@link #getSubmitType}.
- * @return this
- */
- public SubmitRuleEvaluator setPatchSet(PatchSet ps) {
- checkArgument(
- ps.getId().getParentKey().equals(cd.getId()),
- "Patch set %s does not match change %s",
- ps.getId(),
- cd.getId());
- patchSet = ps;
- return this;
- }
-
- /**
- * @param allow whether to allow {@link #evaluate()} on closed changes.
- * @return this
- */
- public SubmitRuleEvaluator setAllowClosed(boolean allow) {
- checkNotStarted();
- optsBuilder.allowClosed(allow);
- return this;
- }
-
- /**
- * @param skip if true, submit filter will not be applied.
- * @return this
- */
- public SubmitRuleEvaluator setSkipSubmitFilters(boolean skip) {
- checkNotStarted();
- optsBuilder.skipFilters(skip);
- return this;
- }
-
- /**
- * @param rule custom rule to use, or null to use refs/meta/config:rules.pl.
- * @return this
- */
- public SubmitRuleEvaluator setRule(@Nullable String rule) {
- checkNotStarted();
- optsBuilder.rule(rule);
- return this;
- }
-
- /**
- * @param log whether to log error messages in addition to returning error records. If true, error
- * record messages will be less descriptive.
- */
- public SubmitRuleEvaluator setLogErrors(boolean log) {
- logErrors = log;
- return this;
- }
-
- /** @return Prolog reductions consumed during evaluation. */
- public long getReductionsConsumed() {
- return reductionsConsumed;
- }
-
- /**
* Evaluate the submit rules.
*
* @return List of {@link SubmitRecord} objects returned from the evaluated rules, including any
* errors.
+ * @param cd ChangeData to evaluate
*/
- public List<SubmitRecord> evaluate() {
- initOptions();
+ public List<SubmitRecord> evaluate(ChangeData cd) {
+ Change change;
+ ProjectState projectState;
try {
- init();
+ change = cd.change();
+ if (change == null) {
+ throw new OrmException("Change not found");
+ }
+
+ projectState = projectCache.get(cd.project());
+ if (projectState == null) {
+ throw new NoSuchProjectException(cd.project());
+ }
} catch (OrmException | NoSuchProjectException e) {
return ruleError("Error looking up change " + cd.getId(), e);
}
@@ -221,138 +108,16 @@
return Collections.singletonList(rec);
}
- List<Term> results;
- try {
- results =
- evaluateImpl(
- "locate_submit_rule", "can_submit", "locate_submit_filter", "filter_submit_results");
- } catch (RuleEvalException e) {
- return ruleError(e.getMessage(), e);
- }
-
- if (results.isEmpty()) {
- // This should never occur. A well written submit rule will always produce
- // at least one result informing the caller of the labels that are
- // required for this change to be submittable. Each label will indicate
- // whether or not that is actually possible given the permissions.
- return ruleError(
- String.format(
- "Submit rule '%s' for change %s of %s has no solution.",
- getSubmitRuleName(), cd.getId(), getProjectName()));
- }
-
- return resultsToSubmitRecord(getSubmitRule(), results);
- }
-
- /**
- * Convert the results from Prolog Cafe's format to Gerrit's common format.
- *
- * <p>can_submit/1 terminates when an ok(P) record is found. Therefore walk the results backwards,
- * using only that ok(P) record if it exists. This skips partial results that occur early in the
- * output. Later after the loop the out collection is reversed to restore it to the original
- * ordering.
- */
- private List<SubmitRecord> resultsToSubmitRecord(Term submitRule, List<Term> results) {
- List<SubmitRecord> out = new ArrayList<>(results.size());
- for (int resultIdx = results.size() - 1; 0 <= resultIdx; resultIdx--) {
- Term submitRecord = results.get(resultIdx);
- SubmitRecord rec = new SubmitRecord();
- out.add(rec);
-
- if (!(submitRecord instanceof StructureTerm) || 1 != submitRecord.arity()) {
- return invalidResult(submitRule, submitRecord);
- }
-
- if ("ok".equals(submitRecord.name())) {
- rec.status = SubmitRecord.Status.OK;
-
- } else if ("not_ready".equals(submitRecord.name())) {
- rec.status = SubmitRecord.Status.NOT_READY;
-
- } else {
- return invalidResult(submitRule, submitRecord);
- }
-
- // Unpack the one argument. This should also be a structure with one
- // argument per label that needs to be reported on to the caller.
- //
- submitRecord = submitRecord.arg(0);
-
- if (!(submitRecord instanceof StructureTerm)) {
- return invalidResult(submitRule, submitRecord);
- }
-
- rec.labels = new ArrayList<>(submitRecord.arity());
-
- for (Term state : ((StructureTerm) submitRecord).args()) {
- if (!(state instanceof StructureTerm)
- || 2 != state.arity()
- || !"label".equals(state.name())) {
- return invalidResult(submitRule, submitRecord);
- }
-
- SubmitRecord.Label lbl = new SubmitRecord.Label();
- rec.labels.add(lbl);
-
- lbl.label = state.arg(0).name();
- Term status = state.arg(1);
-
- try {
- if ("ok".equals(status.name())) {
- lbl.status = SubmitRecord.Label.Status.OK;
- appliedBy(lbl, status);
-
- } else if ("reject".equals(status.name())) {
- lbl.status = SubmitRecord.Label.Status.REJECT;
- appliedBy(lbl, status);
-
- } else if ("need".equals(status.name())) {
- lbl.status = SubmitRecord.Label.Status.NEED;
-
- } else if ("may".equals(status.name())) {
- lbl.status = SubmitRecord.Label.Status.MAY;
-
- } else if ("impossible".equals(status.name())) {
- lbl.status = SubmitRecord.Label.Status.IMPOSSIBLE;
-
- } else {
- return invalidResult(submitRule, submitRecord);
- }
- } catch (UserTermExpected e) {
- return invalidResult(submitRule, submitRecord, e.getMessage());
- }
- }
-
- if (rec.status == SubmitRecord.Status.OK) {
- break;
- }
- }
- Collections.reverse(out);
-
- return out;
- }
-
- private List<SubmitRecord> invalidResult(Term rule, Term record, String reason) {
- return ruleError(
- String.format(
- "Submit rule %s for change %s of %s output invalid result: %s%s",
- rule,
- cd.getId(),
- getProjectName(),
- record,
- (reason == null ? "" : ". Reason: " + reason)));
- }
-
- private List<SubmitRecord> invalidResult(Term rule, Term record) {
- return invalidResult(rule, record, null);
- }
-
- private List<SubmitRecord> ruleError(String err) {
- return ruleError(err, null);
+ // We evaluate all the plugin-defined evaluators,
+ // and then we collect the results in one list.
+ return StreamSupport.stream(submitRules.spliterator(), false)
+ .map(s -> s.evaluate(cd, opts))
+ .flatMap(Collection::stream)
+ .collect(Collectors.toList());
}
private List<SubmitRecord> ruleError(String err, Exception e) {
- if (logErrors) {
+ if (opts.logErrors()) {
if (e == null) {
log.error(err);
} else {
@@ -367,73 +132,24 @@
* Evaluate the submit type rules to get the submit type.
*
* @return record from the evaluated rules.
+ * @param cd
*/
- public SubmitTypeRecord getSubmitType() {
- initOptions();
+ public SubmitTypeRecord getSubmitType(ChangeData cd) {
+ ProjectState projectState;
try {
- init();
- } catch (OrmException | NoSuchProjectException e) {
+ projectState = projectCache.get(cd.project());
+ if (projectState == null) {
+ throw new NoSuchProjectException(cd.project());
+ }
+ } catch (NoSuchProjectException e) {
return typeError("Error looking up change " + cd.getId(), e);
}
- List<Term> results;
- try {
- results =
- evaluateImpl(
- "locate_submit_type",
- "get_submit_type",
- "locate_submit_type_filter",
- "filter_submit_type_results");
- } catch (RuleEvalException e) {
- return typeError(e.getMessage(), e);
- }
-
- if (results.isEmpty()) {
- // Should never occur for a well written rule
- return typeError(
- "Submit rule '"
- + getSubmitRuleName()
- + "' for change "
- + cd.getId()
- + " of "
- + getProjectName()
- + " has no solution.");
- }
-
- Term typeTerm = results.get(0);
- if (!(typeTerm instanceof SymbolTerm)) {
- return typeError(
- "Submit rule '"
- + getSubmitRuleName()
- + "' for change "
- + cd.getId()
- + " of "
- + getProjectName()
- + " did not return a symbol.");
- }
-
- String typeName = ((SymbolTerm) typeTerm).name();
- try {
- return SubmitTypeRecord.OK(SubmitType.valueOf(typeName.toUpperCase()));
- } catch (IllegalArgumentException e) {
- return typeError(
- "Submit type rule "
- + getSubmitRule()
- + " for change "
- + cd.getId()
- + " of "
- + getProjectName()
- + " output invalid result: "
- + typeName);
- }
- }
-
- private SubmitTypeRecord typeError(String err) {
- return typeError(err, null);
+ return prologRule.getSubmitType(cd, opts);
}
private SubmitTypeRecord typeError(String err, Exception e) {
- if (logErrors) {
+ if (opts.logErrors()) {
if (e == null) {
log.error(err);
} else {
@@ -443,194 +159,4 @@
}
return SubmitTypeRecord.error(err);
}
-
- private List<Term> evaluateImpl(
- String userRuleLocatorName,
- String userRuleWrapperName,
- String filterRuleLocatorName,
- String filterRuleWrapperName)
- throws RuleEvalException {
- PrologEnvironment env = getPrologEnvironment();
- try {
- Term sr = env.once("gerrit", userRuleLocatorName, new VariableTerm());
- List<Term> results = new ArrayList<>();
- try {
- for (Term[] template : env.all("gerrit", userRuleWrapperName, sr, new VariableTerm())) {
- results.add(template[1]);
- }
- } catch (ReductionLimitException err) {
- throw new RuleEvalException(
- String.format(
- "%s on change %d of %s", err.getMessage(), cd.getId().get(), getProjectName()));
- } catch (RuntimeException err) {
- throw new RuleEvalException(
- String.format(
- "Exception calling %s on change %d of %s", sr, cd.getId().get(), getProjectName()),
- err);
- } finally {
- reductionsConsumed = env.getReductions();
- }
-
- Term resultsTerm = toListTerm(results);
- if (!opts.skipFilters()) {
- resultsTerm =
- runSubmitFilters(resultsTerm, env, filterRuleLocatorName, filterRuleWrapperName);
- }
- List<Term> r;
- if (resultsTerm instanceof ListTerm) {
- r = new ArrayList<>();
- for (Term t = resultsTerm; t instanceof ListTerm; ) {
- ListTerm l = (ListTerm) t;
- r.add(l.car().dereference());
- t = l.cdr().dereference();
- }
- } else {
- r = Collections.emptyList();
- }
- submitRule = sr;
- return r;
- } finally {
- env.close();
- }
- }
-
- private PrologEnvironment getPrologEnvironment() throws RuleEvalException {
- PrologEnvironment env;
- try {
- if (opts.rule() == null) {
- env = projectState.newPrologEnvironment();
- } else {
- env = projectState.newPrologEnvironment("stdin", new StringReader(opts.rule()));
- }
- } catch (CompileException err) {
- String msg;
- if (opts.rule() == null) {
- msg = String.format("Cannot load rules.pl for %s: %s", getProjectName(), err.getMessage());
- } else {
- msg = err.getMessage();
- }
- throw new RuleEvalException(msg, err);
- }
- env.set(StoredValues.ACCOUNTS, accounts);
- env.set(StoredValues.ACCOUNT_CACHE, accountCache);
- env.set(StoredValues.EMAILS, emails);
- env.set(StoredValues.REVIEW_DB, cd.db());
- env.set(StoredValues.CHANGE_DATA, cd);
- env.set(StoredValues.PROJECT_STATE, projectState);
- return env;
- }
-
- private Term runSubmitFilters(
- Term results,
- PrologEnvironment env,
- String filterRuleLocatorName,
- String filterRuleWrapperName)
- throws RuleEvalException {
- PrologEnvironment childEnv = env;
- for (ProjectState parentState : projectState.parents()) {
- PrologEnvironment parentEnv;
- try {
- parentEnv = parentState.newPrologEnvironment();
- } catch (CompileException err) {
- throw new RuleEvalException("Cannot consult rules.pl for " + parentState.getName(), err);
- }
-
- parentEnv.copyStoredValues(childEnv);
- Term filterRule = parentEnv.once("gerrit", filterRuleLocatorName, new VariableTerm());
- try {
- Term[] template =
- parentEnv.once(
- "gerrit", filterRuleWrapperName, filterRule, results, new VariableTerm());
- results = template[2];
- } catch (ReductionLimitException err) {
- throw new RuleEvalException(
- String.format(
- "%s on change %d of %s",
- err.getMessage(), cd.getId().get(), parentState.getName()));
- } catch (RuntimeException err) {
- throw new RuleEvalException(
- String.format(
- "Exception calling %s on change %d of %s",
- filterRule, cd.getId().get(), parentState.getName()),
- err);
- } finally {
- reductionsConsumed += env.getReductions();
- }
- childEnv = parentEnv;
- }
- return results;
- }
-
- private static Term toListTerm(List<Term> terms) {
- Term list = Prolog.Nil;
- for (int i = terms.size() - 1; i >= 0; i--) {
- list = new ListTerm(terms.get(i), list);
- }
- return list;
- }
-
- private void appliedBy(SubmitRecord.Label label, Term status) throws UserTermExpected {
- if (status instanceof StructureTerm && status.arity() == 1) {
- Term who = status.arg(0);
- if (isUser(who)) {
- label.appliedBy = new Account.Id(((IntegerTerm) who.arg(0)).intValue());
- } else {
- throw new UserTermExpected(label);
- }
- }
- }
-
- private static boolean isUser(Term who) {
- return who instanceof StructureTerm
- && who.arity() == 1
- && who.name().equals("user")
- && who.arg(0) instanceof IntegerTerm;
- }
-
- public Term getSubmitRule() {
- checkState(submitRule != null, "getSubmitRule() invalid before evaluation");
- return submitRule;
- }
-
- public String getSubmitRuleName() {
- return submitRule != null ? submitRule.toString() : "<unknown rule>";
- }
-
- private void checkNotStarted() {
- checkState(opts == null, "cannot set options after starting evaluation");
- }
-
- private void initOptions() {
- if (opts == null) {
- opts = optsBuilder.build();
- optsBuilder = null;
- }
- }
-
- private void init() throws OrmException, NoSuchProjectException {
- if (change == null) {
- change = cd.change();
- if (change == null) {
- throw new OrmException("No change found");
- }
- }
-
- if (projectState == null) {
- projectState = projectCache.get(change.getProject());
- if (projectState == null) {
- throw new NoSuchProjectException(change.getProject());
- }
- }
-
- if (patchSet == null) {
- patchSet = cd.currentPatchSet();
- if (patchSet == null) {
- throw new OrmException("No patch set found");
- }
- }
- }
-
- private String getProjectName() {
- return projectState.getName();
- }
}
diff --git a/java/com/google/gerrit/server/project/SubmitRuleOptions.java b/java/com/google/gerrit/server/project/SubmitRuleOptions.java
index 332aa75..a4340b2 100644
--- a/java/com/google/gerrit/server/project/SubmitRuleOptions.java
+++ b/java/com/google/gerrit/server/project/SubmitRuleOptions.java
@@ -25,17 +25,28 @@
*/
@AutoValue
public abstract class SubmitRuleOptions {
+ private static final SubmitRuleOptions defaults =
+ new AutoValue_SubmitRuleOptions.Builder()
+ .allowClosed(false)
+ .skipFilters(false)
+ .logErrors(true)
+ .rule(null)
+ .build();
+
+ public static SubmitRuleOptions defaults() {
+ return defaults;
+ }
+
public static Builder builder() {
- return new AutoValue_SubmitRuleOptions.Builder()
- .allowClosed(false)
- .skipFilters(false)
- .rule(null);
+ return defaults.toBuilder();
}
public abstract boolean allowClosed();
public abstract boolean skipFilters();
+ public abstract boolean logErrors();
+
@Nullable
public abstract String rule();
@@ -49,6 +60,8 @@
public abstract SubmitRuleOptions.Builder rule(@Nullable String rule);
+ public abstract SubmitRuleOptions.Builder logErrors(boolean logErrors);
+
public abstract SubmitRuleOptions build();
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 6dfc40b..fccb14a 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -943,7 +943,7 @@
if (!lazyLoad) {
return Collections.emptyList();
}
- records = submitRuleEvaluatorFactory.create(this).setOptions(options).evaluate();
+ records = submitRuleEvaluatorFactory.create(options).evaluate(this);
submitRecords.put(options, records);
}
return records;
@@ -960,7 +960,8 @@
public SubmitTypeRecord submitTypeRecord() {
if (submitTypeRecord == null) {
- submitTypeRecord = submitRuleEvaluatorFactory.create(this).getSubmitType();
+ submitTypeRecord =
+ submitRuleEvaluatorFactory.create(SubmitRuleOptions.defaults()).getSubmitType(this);
}
return submitTypeRecord;
}
diff --git a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
index 64c6208..0806edf 100644
--- a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
+++ b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
@@ -31,6 +31,7 @@
import com.google.gerrit.server.events.EventFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
+import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gson.Gson;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -249,8 +250,8 @@
}
if (includeSubmitRecords) {
- eventFactory.addSubmitRecords(
- c, submitRuleEvaluatorFactory.create(d).setAllowClosed(true).evaluate());
+ SubmitRuleOptions options = SubmitRuleOptions.builder().allowClosed(true).build();
+ eventFactory.addSubmitRecords(c, submitRuleEvaluatorFactory.create(options).evaluate(d));
}
if (includeCommitMessage) {
diff --git a/java/com/google/gerrit/server/restapi/account/GetGroups.java b/java/com/google/gerrit/server/restapi/account/GetGroups.java
index 992a85a..486a151 100644
--- a/java/com/google/gerrit/server/restapi/account/GetGroups.java
+++ b/java/com/google/gerrit/server/restapi/account/GetGroups.java
@@ -28,6 +28,7 @@
import com.google.inject.Singleton;
import java.util.ArrayList;
import java.util.List;
+import java.util.Set;
@Singleton
public class GetGroups implements RestReadView<AccountResource> {
@@ -44,8 +45,9 @@
public List<GroupInfo> apply(AccountResource resource) throws OrmException {
IdentifiedUser user = resource.getUser();
Account.Id userId = user.getAccountId();
- List<GroupInfo> groups = new ArrayList<>();
- for (AccountGroup.UUID uuid : user.getEffectiveGroups().getKnownGroups()) {
+ Set<AccountGroup.UUID> knownGroups = user.getEffectiveGroups().getKnownGroups();
+ List<GroupInfo> visibleGroups = new ArrayList<>();
+ for (AccountGroup.UUID uuid : knownGroups) {
GroupControl ctl;
try {
ctl = groupControlFactory.controlFor(uuid);
@@ -53,9 +55,9 @@
continue;
}
if (ctl.isVisible() && ctl.canSeeMember(userId)) {
- groups.add(json.format(ctl.getGroup()));
+ visibleGroups.add(json.format(ctl.getGroup()));
}
}
- return groups;
+ return visibleGroups;
}
}
diff --git a/java/com/google/gerrit/server/restapi/account/Index.java b/java/com/google/gerrit/server/restapi/account/Index.java
index 20a381a..fc6b7b3 100644
--- a/java/com/google/gerrit/server/restapi/account/Index.java
+++ b/java/com/google/gerrit/server/restapi/account/Index.java
@@ -19,8 +19,8 @@
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountResource;
+import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -32,14 +32,16 @@
@Singleton
public class Index implements RestModifyView<AccountResource, Input> {
- private final AccountCache accountCache;
+ private final Provider<AccountIndexer> accountIndexer;
private final PermissionBackend permissionBackend;
private final Provider<CurrentUser> self;
@Inject
Index(
- AccountCache accountCache, PermissionBackend permissionBackend, Provider<CurrentUser> self) {
- this.accountCache = accountCache;
+ Provider<AccountIndexer> accountIndexer,
+ PermissionBackend permissionBackend,
+ Provider<CurrentUser> self) {
+ this.accountIndexer = accountIndexer;
this.permissionBackend = permissionBackend;
this.self = self;
}
@@ -51,8 +53,7 @@
permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT);
}
- // evicting the account from the cache, reindexes the account
- accountCache.evict(rsrc.getUser().getAccountId());
+ accountIndexer.get().index(rsrc.getUser().getAccountId());
return Response.none();
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Mergeable.java b/java/com/google/gerrit/server/restapi/change/Mergeable.java
index 47d76b2..4f8064b 100644
--- a/java/com/google/gerrit/server/restapi/change/Mergeable.java
+++ b/java/com/google/gerrit/server/restapi/change/Mergeable.java
@@ -35,6 +35,7 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
+import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -70,7 +71,7 @@
private final Provider<ReviewDb> db;
private final ChangeIndexer indexer;
private final MergeabilityCache cache;
- private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
+ private final SubmitRuleEvaluator submitRuleEvaluator;
@Inject
Mergeable(
@@ -89,7 +90,7 @@
this.db = db;
this.indexer = indexer;
this.cache = cache;
- this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
+ submitRuleEvaluator = submitRuleEvaluatorFactory.create(SubmitRuleOptions.defaults());
}
public void setOtherBranches(boolean otherBranches) {
@@ -112,7 +113,7 @@
}
ChangeData cd = changeDataFactory.create(db.get(), resource.getNotes());
- result.submitType = getSubmitType(cd, ps);
+ result.submitType = getSubmitType(cd);
try (Repository git = gitManager.openRepository(change.getProject())) {
ObjectId commit = toId(ps);
@@ -144,9 +145,8 @@
return result;
}
- private SubmitType getSubmitType(ChangeData cd, PatchSet patchSet) throws OrmException {
- SubmitTypeRecord rec =
- submitRuleEvaluatorFactory.create(cd).setPatchSet(patchSet).getSubmitType();
+ private SubmitType getSubmitType(ChangeData cd) throws OrmException {
+ SubmitTypeRecord rec = submitRuleEvaluator.getSubmitType(cd);
if (rec.status != SubmitTypeRecord.Status.OK) {
throw new OrmException("Submit type rule failed: " + rec);
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
index 4ff3862..f9d7083 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
@@ -194,13 +194,20 @@
Addition byAccountId =
addByAccountId(reviewer, rsrc, state, notify, accountsToNotify, allowGroup, allowByEmail);
+
+ Addition wholeGroup = null;
+ if (byAccountId == null || !byAccountId.exactMatchFound) {
+ wholeGroup =
+ addWholeGroup(
+ reviewer, rsrc, state, notify, accountsToNotify, confirmed, allowGroup, allowByEmail);
+ if (wholeGroup != null && wholeGroup.exactMatchFound) {
+ return wholeGroup;
+ }
+ }
+
if (byAccountId != null) {
return byAccountId;
}
-
- Addition wholeGroup =
- addWholeGroup(
- reviewer, rsrc, state, notify, accountsToNotify, confirmed, allowGroup, allowByEmail);
if (wholeGroup != null) {
return wholeGroup;
}
@@ -216,7 +223,8 @@
null,
CC,
NotifyHandling.NONE,
- ImmutableListMultimap.of());
+ ImmutableListMultimap.of(),
+ true);
}
@Nullable
@@ -229,9 +237,14 @@
boolean allowGroup,
boolean allowByEmail)
throws OrmException, PermissionBackendException, IOException, ConfigInvalidException {
- Account.Id accountId = null;
+ IdentifiedUser user = null;
+ boolean exactMatchFound = false;
try {
- accountId = accounts.parse(reviewer).getAccountId();
+ user = accounts.parse(reviewer);
+ if (reviewer.equalsIgnoreCase(user.getName())
+ || reviewer.equals(String.valueOf(user.getAccountId()))) {
+ exactMatchFound = true;
+ }
} catch (UnprocessableEntityException | AuthException e) {
// AuthException won't occur since the user is authenticated at this point.
if (!allowGroup && !allowByEmail) {
@@ -242,13 +255,20 @@
return null;
}
- ReviewerResource rrsrc = reviewerFactory.create(rsrc, accountId);
+ ReviewerResource rrsrc = reviewerFactory.create(rsrc, user.getAccountId());
Account member = rrsrc.getReviewerUser().getAccount();
PermissionBackend.ForRef perm =
permissionBackend.user(rrsrc.getReviewerUser()).ref(rrsrc.getChange().getDest());
if (isValidReviewer(member, perm)) {
return new Addition(
- reviewer, rsrc, ImmutableSet.of(member.getId()), null, state, notify, accountsToNotify);
+ reviewer,
+ rsrc,
+ ImmutableSet.of(member.getId()),
+ null,
+ state,
+ notify,
+ accountsToNotify,
+ exactMatchFound);
}
if (!member.isActive()) {
if (allowByEmail && state == CC) {
@@ -328,7 +348,7 @@
}
}
- return new Addition(reviewer, rsrc, reviewers, null, state, notify, accountsToNotify);
+ return new Addition(reviewer, rsrc, reviewers, null, state, notify, accountsToNotify, true);
}
@Nullable
@@ -357,7 +377,7 @@
return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInvalid, reviewer));
}
return new Addition(
- reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify);
+ reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify, true);
}
private boolean isValidReviewer(Account member, PermissionBackend.ForRef perm)
@@ -395,6 +415,7 @@
final ReviewerState state;
final ChangeNotes notes;
final IdentifiedUser caller;
+ final boolean exactMatchFound;
Addition(String reviewer) {
result = new AddReviewerResult(reviewer);
@@ -404,6 +425,7 @@
state = REVIEWER;
notes = null;
caller = null;
+ exactMatchFound = false;
}
protected Addition(
@@ -413,7 +435,8 @@
@Nullable Collection<Address> reviewersByEmail,
ReviewerState state,
@Nullable NotifyHandling notify,
- ListMultimap<RecipientType, Account.Id> accountsToNotify) {
+ ListMultimap<RecipientType, Account.Id> accountsToNotify,
+ boolean exactMatchFound) {
checkArgument(
reviewers != null || reviewersByEmail != null,
"must have either reviewers or reviewersByEmail");
@@ -427,6 +450,7 @@
op =
postReviewersOpFactory.create(
rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
+ this.exactMatchFound = exactMatchFound;
}
void gatherResults() throws OrmException, PermissionBackendException {
diff --git a/java/com/google/gerrit/server/restapi/change/Revert.java b/java/com/google/gerrit/server/restapi/change/Revert.java
index b55ca5e..4545794 100644
--- a/java/com/google/gerrit/server/restapi/change/Revert.java
+++ b/java/com/google/gerrit/server/restapi/change/Revert.java
@@ -18,7 +18,10 @@
import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE;
import com.google.common.base.Strings;
+import com.google.common.collect.ListMultimap;
import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
+import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.api.changes.RevertInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -43,6 +46,7 @@
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.NotifyUtil;
import com.google.gerrit.server.extensions.events.ChangeReverted;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.mail.send.RevertedSender;
@@ -70,6 +74,7 @@
import java.text.MessageFormat;
import java.util.HashSet;
import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.ObjectId;
@@ -102,6 +107,7 @@
private final ChangeReverted changeReverted;
private final ContributorAgreementsChecker contributorAgreements;
private final ProjectCache projectCache;
+ private final NotifyUtil notifyUtil;
@Inject
Revert(
@@ -119,7 +125,8 @@
ApprovalsUtil approvalsUtil,
ChangeReverted changeReverted,
ContributorAgreementsChecker contributorAgreements,
- ProjectCache projectCache) {
+ ProjectCache projectCache,
+ NotifyUtil notifyUtil) {
super(retryHelper);
this.db = db;
this.permissionBackend = permissionBackend;
@@ -135,13 +142,14 @@
this.changeReverted = changeReverted;
this.contributorAgreements = contributorAgreements;
this.projectCache = projectCache;
+ this.notifyUtil = notifyUtil;
}
@Override
public ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, RevertInput input)
throws IOException, OrmException, RestApiException, UpdateException, NoSuchChangeException,
- PermissionBackendException, NoSuchProjectException {
+ PermissionBackendException, NoSuchProjectException, ConfigInvalidException {
Change change = rsrc.getChange();
if (change.getStatus() != Change.Status.MERGED) {
throw new ResourceConflictException("change is " + ChangeUtil.status(change));
@@ -151,14 +159,14 @@
permissionBackend.user(rsrc.getUser()).ref(change.getDest()).check(CREATE_CHANGE);
projectCache.checkedGet(rsrc.getProject()).checkStatePermitsWrite();
- Change.Id revertId =
- revert(updateFactory, rsrc.getNotes(), rsrc.getUser(), Strings.emptyToNull(input.message));
+ Change.Id revertId = revert(updateFactory, rsrc.getNotes(), rsrc.getUser(), input);
return json.noOptions().format(rsrc.getProject(), revertId);
}
private Change.Id revert(
- BatchUpdate.Factory updateFactory, ChangeNotes notes, CurrentUser user, String message)
- throws OrmException, IOException, RestApiException, UpdateException {
+ BatchUpdate.Factory updateFactory, ChangeNotes notes, CurrentUser user, RevertInput input)
+ throws OrmException, IOException, RestApiException, UpdateException, ConfigInvalidException {
+ String message = Strings.emptyToNull(input.message);
Change.Id changeIdToRevert = notes.getChangeId();
PatchSet.Id patchSetId = notes.getChange().currentPatchSetId();
PatchSet patch = psUtil.get(db.get(), notes, patchSetId);
@@ -213,11 +221,16 @@
ObjectId id = oi.insert(revertCommitBuilder);
RevCommit revertCommit = revWalk.parseCommit(id);
+ ListMultimap<RecipientType, Account.Id> accountsToNotify =
+ notifyUtil.resolveAccounts(input.notifyDetails);
+
ChangeInserter ins =
changeInserterFactory
.create(changeId, revertCommit, notes.getChange().getDest().get())
.setTopic(changeToRevert.getTopic());
ins.setMessage("Uploaded patch set 1.");
+ ins.setNotify(input.notify);
+ ins.setAccountsToNotify(accountsToNotify);
ReviewerSet reviewerSet = approvalsUtil.getReviewers(db.get(), notes);
@@ -235,7 +248,7 @@
try (BatchUpdate bu = updateFactory.create(db.get(), project, user, now)) {
bu.setRepository(git, revWalk, oi);
bu.insertChange(ins);
- bu.addOp(changeId, new NotifyOp(notes.getChange(), ins));
+ bu.addOp(changeId, new NotifyOp(changeToRevert, ins, input.notify, accountsToNotify));
bu.addOp(changeToRevert.getId(), new PostRevertedMessageOp(computedChangeId));
bu.execute();
}
@@ -269,23 +282,31 @@
private class NotifyOp implements BatchUpdateOp {
private final Change change;
private final ChangeInserter ins;
+ private final NotifyHandling notifyHandling;
+ private final ListMultimap<RecipientType, Account.Id> accountsToNotify;
- NotifyOp(Change change, ChangeInserter ins) {
+ NotifyOp(
+ Change change,
+ ChangeInserter ins,
+ NotifyHandling notifyHandling,
+ ListMultimap<RecipientType, Account.Id> accountsToNotify) {
this.change = change;
this.ins = ins;
+ this.notifyHandling = notifyHandling;
+ this.accountsToNotify = accountsToNotify;
}
@Override
public void postUpdate(Context ctx) throws Exception {
changeReverted.fire(change, ins.getChange(), ctx.getWhen());
- Change.Id changeId = ins.getChange().getId();
try {
- RevertedSender cm = revertedSenderFactory.create(ctx.getProject(), changeId);
+ RevertedSender cm = revertedSenderFactory.create(ctx.getProject(), change.getId());
cm.setFrom(ctx.getAccountId());
- cm.setChangeMessage(ins.getChangeMessage().getMessage(), ctx.getWhen());
+ cm.setNotify(notifyHandling);
+ cm.setAccountsToNotify(accountsToNotify);
cm.send();
} catch (Exception err) {
- log.error("Cannot send email for revert change " + changeId, err);
+ log.error("Cannot send email for revert change " + change.getId(), err);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/ReviewerJson.java b/java/com/google/gerrit/server/restapi/change/ReviewerJson.java
index ef5b432..d74be7d 100644
--- a/java/com/google/gerrit/server/restapi/change/ReviewerJson.java
+++ b/java/com/google/gerrit/server/restapi/change/ReviewerJson.java
@@ -33,6 +33,7 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
+import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -49,7 +50,7 @@
private final ChangeData.Factory changeDataFactory;
private final ApprovalsUtil approvalsUtil;
private final AccountLoader.Factory accountLoaderFactory;
- private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
+ private final SubmitRuleEvaluator submitRuleEvaluator;
@Inject
ReviewerJson(
@@ -64,7 +65,7 @@
this.changeDataFactory = changeDataFactory;
this.approvalsUtil = approvalsUtil;
this.accountLoaderFactory = accountLoaderFactory;
- this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
+ submitRuleEvaluator = submitRuleEvaluatorFactory.create(SubmitRuleOptions.defaults());
}
public List<ReviewerInfo> format(Collection<ReviewerResource> rsrcs)
@@ -124,7 +125,7 @@
// do not exist in the DB.
PatchSet ps = cd.currentPatchSet();
if (ps != null) {
- for (SubmitRecord rec : submitRuleEvaluatorFactory.create(cd).evaluate()) {
+ for (SubmitRecord rec : submitRuleEvaluator.evaluate(cd)) {
if (rec.labels == null) {
continue;
}
diff --git a/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java b/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
index eb356ed..2a18612 100644
--- a/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
+++ b/java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
@@ -26,6 +26,7 @@
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
+import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.RulesCache;
import com.google.gwtorm.server.OrmException;
@@ -70,24 +71,22 @@
throw new AuthException("project rules are disabled");
}
input.filters = MoreObjects.firstNonNull(input.filters, filters);
- SubmitRuleEvaluator evaluator =
- submitRuleEvaluatorFactory.create(changeDataFactory.create(db.get(), rsrc.getNotes()));
- List<SubmitRecord> records =
- evaluator
- .setPatchSet(rsrc.getPatchSet())
- .setLogErrors(false)
- .setSkipSubmitFilters(input.filters == Filters.SKIP)
- .setRule(input.rule)
- .evaluate();
+ SubmitRuleOptions opts =
+ SubmitRuleOptions.builder()
+ .skipFilters(input.filters == Filters.SKIP)
+ .rule(input.rule)
+ .logErrors(false)
+ .build();
+
+ ChangeData cd = changeDataFactory.create(db.get(), rsrc.getNotes());
+ List<SubmitRecord> records = submitRuleEvaluatorFactory.create(opts).evaluate(cd);
+
List<Record> out = Lists.newArrayListWithCapacity(records.size());
AccountLoader accounts = accountInfoFactory.create(true);
for (SubmitRecord r : records) {
out.add(new Record(r, accounts));
}
- if (!out.isEmpty()) {
- out.get(0).prologReductionCount = evaluator.getReductionsConsumed();
- }
accounts.fill();
return out;
}
@@ -100,7 +99,6 @@
Map<String, None> need;
Map<String, AccountInfo> may;
Map<String, None> impossible;
- Long prologReductionCount;
Record(SubmitRecord r, AccountLoader accounts) {
this.status = r.status;
diff --git a/java/com/google/gerrit/server/restapi/change/TestSubmitType.java b/java/com/google/gerrit/server/restapi/change/TestSubmitType.java
index d8d5d0a..c1be1ce 100644
--- a/java/com/google/gerrit/server/restapi/change/TestSubmitType.java
+++ b/java/com/google/gerrit/server/restapi/change/TestSubmitType.java
@@ -26,6 +26,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
+import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.RulesCache;
import com.google.gwtorm.server.OrmException;
@@ -64,19 +65,20 @@
throw new AuthException("project rules are disabled");
}
input.filters = MoreObjects.firstNonNull(input.filters, filters);
- SubmitRuleEvaluator evaluator =
- submitRuleEvaluatorFactory.create(changeDataFactory.create(db.get(), rsrc.getNotes()));
- SubmitTypeRecord rec =
- evaluator
- .setPatchSet(rsrc.getPatchSet())
- .setLogErrors(false)
- .setSkipSubmitFilters(input.filters == Filters.SKIP)
- .setRule(input.rule)
- .getSubmitType();
+ SubmitRuleOptions opts =
+ SubmitRuleOptions.builder()
+ .logErrors(false)
+ .skipFilters(input.filters == Filters.SKIP)
+ .rule(input.rule)
+ .build();
+
+ SubmitRuleEvaluator evaluator = submitRuleEvaluatorFactory.create(opts);
+ ChangeData cd = changeDataFactory.create(db.get(), rsrc.getNotes());
+ SubmitTypeRecord rec = evaluator.getSubmitType(cd);
+
if (rec.status != SubmitTypeRecord.Status.OK) {
- throw new BadRequestException(
- String.format("rule %s produced invalid result: %s", evaluator.getSubmitRuleName(), rec));
+ throw new BadRequestException(String.format("rule produced invalid result: %s", rec));
}
return rec.type;
diff --git a/java/com/google/gerrit/server/restapi/config/GetSummary.java b/java/com/google/gerrit/server/restapi/config/GetSummary.java
index 26f069c..a382436 100644
--- a/java/com/google/gerrit/server/restapi/config/GetSummary.java
+++ b/java/com/google/gerrit/server/restapi/config/GetSummary.java
@@ -37,7 +37,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import org.eclipse.jgit.internal.storage.file.WindowCacheStatAccessor;
+import org.eclipse.jgit.storage.file.WindowCacheStats;
import org.kohsuke.args4j.Option;
@RequiresCapability(GlobalCapability.MAINTAIN_SERVER)
@@ -125,8 +125,8 @@
long mTotal = r.totalMemory();
long mInuse = mTotal - mFree;
- int jgitOpen = WindowCacheStatAccessor.getOpenFiles();
- long jgitBytes = WindowCacheStatAccessor.getOpenBytes();
+ int jgitOpen = WindowCacheStats.getOpenFiles();
+ long jgitBytes = WindowCacheStats.getOpenBytes();
MemSummaryInfo memSummaryInfo = new MemSummaryInfo();
memSummaryInfo.total = bytes(mTotal);
diff --git a/java/com/google/gerrit/server/restapi/config/SetDiffPreferences.java b/java/com/google/gerrit/server/restapi/config/SetDiffPreferences.java
index 1f6feb4..2aa3fb9 100644
--- a/java/com/google/gerrit/server/restapi/config/SetDiffPreferences.java
+++ b/java/com/google/gerrit/server/restapi/config/SetDiffPreferences.java
@@ -66,7 +66,7 @@
try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName)) {
DiffPreferencesInfo updatedPrefs = Preferences.updateDefaultDiffPreferences(md, input);
- accountCache.evictAllNoReindex();
+ accountCache.evictAll();
return updatedPrefs;
}
}
diff --git a/java/com/google/gerrit/server/restapi/config/SetEditPreferences.java b/java/com/google/gerrit/server/restapi/config/SetEditPreferences.java
index a531152..36bcdb3 100644
--- a/java/com/google/gerrit/server/restapi/config/SetEditPreferences.java
+++ b/java/com/google/gerrit/server/restapi/config/SetEditPreferences.java
@@ -66,7 +66,7 @@
try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName)) {
EditPreferencesInfo updatedPrefs = Preferences.updateDefaultEditPreferences(md, input);
- accountCache.evictAllNoReindex();
+ accountCache.evictAll();
return updatedPrefs;
}
}
diff --git a/java/com/google/gerrit/server/restapi/config/SetPreferences.java b/java/com/google/gerrit/server/restapi/config/SetPreferences.java
index f3ba8f9..0815af7 100644
--- a/java/com/google/gerrit/server/restapi/config/SetPreferences.java
+++ b/java/com/google/gerrit/server/restapi/config/SetPreferences.java
@@ -63,7 +63,7 @@
Preferences.validateMy(input.my);
try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName)) {
GeneralPreferencesInfo updatedPrefs = Preferences.updateDefaultGeneralPreferences(md, input);
- accountCache.evictAllNoReindex();
+ accountCache.evictAll();
return updatedPrefs;
}
}
diff --git a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java
index 2c583c6..51fffbb 100644
--- a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java
+++ b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java
@@ -117,10 +117,12 @@
if (includedGroup.isPresent()) {
member = groupJson.format(new InternalGroupDescription(includedGroup.get()));
} else {
- GroupDescription.Basic groupDescription = groupBackend.get(includedGroupUUID);
member = new GroupInfo();
member.id = Url.encode(includedGroupUUID.get());
- member.name = groupDescription.getName();
+ GroupDescription.Basic groupDescription = groupBackend.get(includedGroupUUID);
+ if (groupDescription != null) {
+ member.name = groupDescription.getName();
+ }
}
auditEvents.add(
diff --git a/java/com/google/gerrit/server/restapi/project/CreateProject.java b/java/com/google/gerrit/server/restapi/project/CreateProject.java
index eb44118..365fe89 100644
--- a/java/com/google/gerrit/server/restapi/project/CreateProject.java
+++ b/java/com/google/gerrit/server/restapi/project/CreateProject.java
@@ -47,6 +47,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.ProjectOwnerGroupsProvider;
import com.google.gerrit.server.config.RepositoryConfig;
import com.google.gerrit.server.extensions.events.AbstractNoNotifyEvent;
@@ -110,6 +111,7 @@
private final Provider<IdentifiedUser> identifiedUser;
private final Provider<PutConfig> putConfig;
private final AllProjectsName allProjects;
+ private final AllUsersName allUsers;
private final DynamicItem<ProjectNameLockManager> lockManager;
private final String name;
@@ -131,6 +133,7 @@
Provider<IdentifiedUser> identifiedUser,
Provider<PutConfig> putConfig,
AllProjectsName allProjects,
+ AllUsersName allUsers,
DynamicItem<ProjectNameLockManager> lockManager,
@Assisted String name) {
this.projectsCollection = projectsCollection;
@@ -149,6 +152,7 @@
this.identifiedUser = identifiedUser;
this.putConfig = putConfig;
this.allProjects = allProjects;
+ this.allUsers = allUsers;
this.lockManager = lockManager;
this.name = name;
}
@@ -169,6 +173,10 @@
String parentName =
MoreObjects.firstNonNull(Strings.emptyToNull(input.parent), allProjects.get());
args.newParent = projectsCollection.get().parse(parentName, false).getNameKey();
+ if (args.newParent.equals(allUsers)) {
+ throw new ResourceConflictException(
+ String.format("Cannot inherit from '%s' project", allUsers.get()));
+ }
args.createEmptyCommit = input.createEmptyCommit;
args.permissionsOnly = input.permissionsOnly;
args.projectDescription = Strings.emptyToNull(input.description);
diff --git a/java/com/google/gerrit/server/restapi/project/SetParent.java b/java/com/google/gerrit/server/restapi/project/SetParent.java
index 21fef97..2e51929 100644
--- a/java/com/google/gerrit/server/restapi/project/SetParent.java
+++ b/java/com/google/gerrit/server/restapi/project/SetParent.java
@@ -126,6 +126,11 @@
throw new ResourceConflictException("cannot set parent of " + allProjects.get());
}
+ if (allUsers.get().equals(newParent)) {
+ throw new ResourceConflictException(
+ String.format("Cannot inherit from '%s' project", allUsers.get()));
+ }
+
newParent = Strings.emptyToNull(newParent);
if (newParent != null) {
ProjectState parent = cache.get(new Project.NameKey(newParent));
diff --git a/java/com/google/gerrit/server/rules/PrologModule.java b/java/com/google/gerrit/server/rules/PrologModule.java
index 1a8b46c..e6de229 100644
--- a/java/com/google/gerrit/server/rules/PrologModule.java
+++ b/java/com/google/gerrit/server/rules/PrologModule.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.rules;
+import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.registration.DynamicSet;
@@ -22,6 +23,9 @@
protected void configure() {
install(new EnvironmentModule());
bind(PrologEnvironment.Args.class);
+ factory(PrologRuleEvaluator.Factory.class);
+
+ bind(SubmitRule.class).annotatedWith(Exports.named("PrologRule")).to(PrologRule.class);
}
static class EnvironmentModule extends FactoryModule {
diff --git a/java/com/google/gerrit/server/rules/PrologRule.java b/java/com/google/gerrit/server/rules/PrologRule.java
new file mode 100644
index 0000000..44f18c5
--- /dev/null
+++ b/java/com/google/gerrit/server/rules/PrologRule.java
@@ -0,0 +1,48 @@
+// Copyright (C) 2018 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.rules;
+
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitTypeRecord;
+import com.google.gerrit.server.project.SubmitRuleOptions;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.PrologRuleEvaluator.Factory;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.Collection;
+
+@Singleton
+public class PrologRule implements SubmitRule {
+
+ private final Factory factory;
+
+ @Inject
+ private PrologRule(PrologRuleEvaluator.Factory factory) {
+ this.factory = factory;
+ }
+
+ @Override
+ public Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions opts) {
+ return getEvaluator(cd, opts).evaluate();
+ }
+
+ private PrologRuleEvaluator getEvaluator(ChangeData cd, SubmitRuleOptions opts) {
+ return factory.create(cd, opts);
+ }
+
+ public SubmitTypeRecord getSubmitType(ChangeData cd, SubmitRuleOptions opts) {
+ return getEvaluator(cd, opts).getSubmitType();
+ }
+}
diff --git a/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java b/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java
new file mode 100644
index 0000000..f2d3c19
--- /dev/null
+++ b/java/com/google/gerrit/server/rules/PrologRuleEvaluator.java
@@ -0,0 +1,519 @@
+// Copyright (C) 2012 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.rules;
+
+import static com.google.gerrit.server.project.SubmitRuleEvaluator.createRuleError;
+import static com.google.gerrit.server.project.SubmitRuleEvaluator.defaultRuleError;
+import static com.google.gerrit.server.project.SubmitRuleEvaluator.defaultTypeError;
+
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitTypeRecord;
+import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Accounts;
+import com.google.gerrit.server.account.Emails;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.project.RuleEvalException;
+import com.google.gerrit.server.project.SubmitRuleOptions;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
+import com.googlecode.prolog_cafe.exceptions.CompileException;
+import com.googlecode.prolog_cafe.exceptions.ReductionLimitException;
+import com.googlecode.prolog_cafe.lang.IntegerTerm;
+import com.googlecode.prolog_cafe.lang.ListTerm;
+import com.googlecode.prolog_cafe.lang.Prolog;
+import com.googlecode.prolog_cafe.lang.StructureTerm;
+import com.googlecode.prolog_cafe.lang.SymbolTerm;
+import com.googlecode.prolog_cafe.lang.Term;
+import com.googlecode.prolog_cafe.lang.VariableTerm;
+import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Evaluates a submit-like Prolog rule found in the rules.pl file of the current project and filters
+ * the results through rules found in the parent projects, all the way up to All-Projects.
+ */
+public class PrologRuleEvaluator {
+ private static final Logger log = LoggerFactory.getLogger(PrologRuleEvaluator.class);
+
+ public interface Factory {
+ /** Returns a new {@link PrologRuleEvaluator} with the specified options */
+ PrologRuleEvaluator create(ChangeData cd, SubmitRuleOptions options);
+ }
+
+ /**
+ * Exception thrown when the label term of a submit record unexpectedly didn't contain a user
+ * term.
+ */
+ private static class UserTermExpected extends Exception {
+ private static final long serialVersionUID = 1L;
+
+ UserTermExpected(SubmitRecord.Label label) {
+ super(String.format("A label with the status %s must contain a user.", label.toString()));
+ }
+ }
+
+ private final AccountCache accountCache;
+ private final Accounts accounts;
+ private final Emails emails;
+ private final ChangeData cd;
+ private final ProjectState projectState;
+ private final SubmitRuleOptions opts;
+ private Term submitRule;
+
+ @AssistedInject
+ private PrologRuleEvaluator(
+ AccountCache accountCache,
+ Accounts accounts,
+ Emails emails,
+ ProjectCache projectCache,
+ @Assisted ChangeData cd,
+ @Assisted SubmitRuleOptions options) {
+ this.accountCache = accountCache;
+ this.accounts = accounts;
+ this.emails = emails;
+ this.cd = cd;
+ this.opts = options;
+
+ this.projectState = projectCache.get(cd.project());
+ }
+
+ private static Term toListTerm(List<Term> terms) {
+ Term list = Prolog.Nil;
+ for (int i = terms.size() - 1; i >= 0; i--) {
+ list = new ListTerm(terms.get(i), list);
+ }
+ return list;
+ }
+
+ private static boolean isUser(Term who) {
+ return who instanceof StructureTerm
+ && who.arity() == 1
+ && who.name().equals("user")
+ && who.arg(0) instanceof IntegerTerm;
+ }
+
+ private Term getSubmitRule() {
+ return submitRule;
+ }
+
+ /**
+ * Evaluate the submit rules.
+ *
+ * @return List of {@link SubmitRecord} objects returned from the evaluated rules, including any
+ * errors.
+ */
+ public Collection<SubmitRecord> evaluate() {
+ Change change;
+ try {
+ change = cd.change();
+ if (change == null) {
+ throw new OrmException("No change found");
+ }
+
+ if (projectState == null) {
+ throw new NoSuchProjectException(cd.project());
+ }
+ } catch (OrmException | NoSuchProjectException e) {
+ return ruleError("Error looking up change " + cd.getId(), e);
+ }
+
+ if (!opts.allowClosed() && change.getStatus().isClosed()) {
+ SubmitRecord rec = new SubmitRecord();
+ rec.status = SubmitRecord.Status.CLOSED;
+ return Collections.singletonList(rec);
+ }
+
+ List<Term> results;
+ try {
+ results =
+ evaluateImpl(
+ "locate_submit_rule", "can_submit", "locate_submit_filter", "filter_submit_results");
+ } catch (RuleEvalException e) {
+ return ruleError(e.getMessage(), e);
+ }
+
+ if (results.isEmpty()) {
+ // This should never occur. A well written submit rule will always produce
+ // at least one result informing the caller of the labels that are
+ // required for this change to be submittable. Each label will indicate
+ // whether or not that is actually possible given the permissions.
+ return ruleError(
+ String.format(
+ "Submit rule '%s' for change %s of %s has no solution.",
+ getSubmitRuleName(), cd.getId(), projectState.getName()));
+ }
+
+ return resultsToSubmitRecord(getSubmitRule(), results);
+ }
+
+ private String getSubmitRuleName() {
+ return submitRule == null ? "<unknown>" : submitRule.name();
+ }
+
+ /**
+ * Convert the results from Prolog Cafe's format to Gerrit's common format.
+ *
+ * <p>can_submit/1 terminates when an ok(P) record is found. Therefore walk the results backwards,
+ * using only that ok(P) record if it exists. This skips partial results that occur early in the
+ * output. Later after the loop the out collection is reversed to restore it to the original
+ * ordering.
+ */
+ public List<SubmitRecord> resultsToSubmitRecord(Term submitRule, List<Term> results) {
+ boolean foundOk = false;
+ List<SubmitRecord> out = new ArrayList<>(results.size());
+ for (int resultIdx = results.size() - 1; 0 <= resultIdx; resultIdx--) {
+ Term submitRecord = results.get(resultIdx);
+ SubmitRecord rec = new SubmitRecord();
+ out.add(rec);
+
+ if (!(submitRecord instanceof StructureTerm) || 1 != submitRecord.arity()) {
+ return invalidResult(submitRule, submitRecord);
+ }
+
+ if ("ok".equals(submitRecord.name())) {
+ rec.status = SubmitRecord.Status.OK;
+
+ } else if ("not_ready".equals(submitRecord.name())) {
+ rec.status = SubmitRecord.Status.NOT_READY;
+
+ } else {
+ return invalidResult(submitRule, submitRecord);
+ }
+
+ // Unpack the one argument. This should also be a structure with one
+ // argument per label that needs to be reported on to the caller.
+ //
+ submitRecord = submitRecord.arg(0);
+
+ if (!(submitRecord instanceof StructureTerm)) {
+ return invalidResult(submitRule, submitRecord);
+ }
+
+ rec.labels = new ArrayList<>(submitRecord.arity());
+
+ for (Term state : ((StructureTerm) submitRecord).args()) {
+ if (!(state instanceof StructureTerm)
+ || 2 != state.arity()
+ || !"label".equals(state.name())) {
+ return invalidResult(submitRule, submitRecord);
+ }
+
+ SubmitRecord.Label lbl = new SubmitRecord.Label();
+ rec.labels.add(lbl);
+
+ lbl.label = state.arg(0).name();
+ Term status = state.arg(1);
+
+ try {
+ if ("ok".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.OK;
+ appliedBy(lbl, status);
+
+ } else if ("reject".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.REJECT;
+ appliedBy(lbl, status);
+
+ } else if ("need".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.NEED;
+
+ } else if ("may".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.MAY;
+
+ } else if ("impossible".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.IMPOSSIBLE;
+
+ } else {
+ return invalidResult(submitRule, submitRecord);
+ }
+ } catch (UserTermExpected e) {
+ return invalidResult(submitRule, submitRecord, e.getMessage());
+ }
+ }
+
+ if (rec.status == SubmitRecord.Status.OK) {
+ foundOk = true;
+ break;
+ }
+ }
+ Collections.reverse(out);
+
+ // This transformation is required to adapt Prolog's behavior to the way Gerrit handles
+ // SubmitRecords, as defined in the SubmitRecord#allRecordsOK method.
+ // When several rules are defined in Prolog, they are all matched to a SubmitRecord. We want
+ // the change to be submittable when at least one result is OK.
+ if (foundOk) {
+ for (SubmitRecord record : out) {
+ record.status = SubmitRecord.Status.OK;
+ }
+ }
+
+ return out;
+ }
+
+ private List<SubmitRecord> invalidResult(Term rule, Term record, String reason) {
+ return ruleError(
+ String.format(
+ "Submit rule %s for change %s of %s output invalid result: %s%s",
+ rule,
+ cd.getId(),
+ cd.project().get(),
+ record,
+ (reason == null ? "" : ". Reason: " + reason)));
+ }
+
+ private List<SubmitRecord> invalidResult(Term rule, Term record) {
+ return invalidResult(rule, record, null);
+ }
+
+ private List<SubmitRecord> ruleError(String err) {
+ return ruleError(err, null);
+ }
+
+ private List<SubmitRecord> ruleError(String err, Exception e) {
+ if (opts.logErrors()) {
+ if (e == null) {
+ log.error(err);
+ } else {
+ log.error(err, e);
+ }
+ return defaultRuleError();
+ }
+ return createRuleError(err);
+ }
+
+ /**
+ * Evaluate the submit type rules to get the submit type.
+ *
+ * @return record from the evaluated rules.
+ */
+ public SubmitTypeRecord getSubmitType() {
+ try {
+ if (projectState == null) {
+ throw new NoSuchProjectException(cd.project());
+ }
+ } catch (NoSuchProjectException e) {
+ return typeError("Error looking up change " + cd.getId(), e);
+ }
+
+ List<Term> results;
+ try {
+ results =
+ evaluateImpl(
+ "locate_submit_type",
+ "get_submit_type",
+ "locate_submit_type_filter",
+ "filter_submit_type_results");
+ } catch (RuleEvalException e) {
+ return typeError(e.getMessage(), e);
+ }
+
+ if (results.isEmpty()) {
+ // Should never occur for a well written rule
+ return typeError(
+ "Submit rule '"
+ + getSubmitRuleName()
+ + "' for change "
+ + cd.getId()
+ + " of "
+ + projectState.getName()
+ + " has no solution.");
+ }
+
+ Term typeTerm = results.get(0);
+ if (!(typeTerm instanceof SymbolTerm)) {
+ return typeError(
+ "Submit rule '"
+ + getSubmitRuleName()
+ + "' for change "
+ + cd.getId()
+ + " of "
+ + projectState.getName()
+ + " did not return a symbol.");
+ }
+
+ String typeName = typeTerm.name();
+ try {
+ return SubmitTypeRecord.OK(SubmitType.valueOf(typeName.toUpperCase()));
+ } catch (IllegalArgumentException e) {
+ return typeError(
+ "Submit type rule "
+ + getSubmitRule()
+ + " for change "
+ + cd.getId()
+ + " of "
+ + projectState.getName()
+ + " output invalid result: "
+ + typeName);
+ }
+ }
+
+ private SubmitTypeRecord typeError(String err) {
+ return typeError(err, null);
+ }
+
+ private SubmitTypeRecord typeError(String err, Exception e) {
+ if (opts.logErrors()) {
+ if (e == null) {
+ log.error(err);
+ } else {
+ log.error(err, e);
+ }
+ return defaultTypeError();
+ }
+ return SubmitTypeRecord.error(err);
+ }
+
+ private List<Term> evaluateImpl(
+ String userRuleLocatorName,
+ String userRuleWrapperName,
+ String filterRuleLocatorName,
+ String filterRuleWrapperName)
+ throws RuleEvalException {
+ PrologEnvironment env = getPrologEnvironment();
+ try {
+ Term sr = env.once("gerrit", userRuleLocatorName, new VariableTerm());
+ List<Term> results = new ArrayList<>();
+ try {
+ for (Term[] template : env.all("gerrit", userRuleWrapperName, sr, new VariableTerm())) {
+ results.add(template[1]);
+ }
+ } catch (ReductionLimitException err) {
+ throw new RuleEvalException(
+ String.format(
+ "%s on change %d of %s",
+ err.getMessage(), cd.getId().get(), projectState.getName()));
+ } catch (RuntimeException err) {
+ throw new RuleEvalException(
+ String.format(
+ "Exception calling %s on change %d of %s",
+ sr, cd.getId().get(), projectState.getName()),
+ err);
+ }
+
+ Term resultsTerm = toListTerm(results);
+ if (!opts.skipFilters()) {
+ resultsTerm =
+ runSubmitFilters(resultsTerm, env, filterRuleLocatorName, filterRuleWrapperName);
+ }
+ List<Term> r;
+ if (resultsTerm instanceof ListTerm) {
+ r = new ArrayList<>();
+ for (Term t = resultsTerm; t instanceof ListTerm; ) {
+ ListTerm l = (ListTerm) t;
+ r.add(l.car().dereference());
+ t = l.cdr().dereference();
+ }
+ } else {
+ r = Collections.emptyList();
+ }
+ submitRule = sr;
+ return r;
+ } finally {
+ env.close();
+ }
+ }
+
+ private PrologEnvironment getPrologEnvironment() throws RuleEvalException {
+ PrologEnvironment env;
+ try {
+ if (opts.rule() == null) {
+ env = projectState.newPrologEnvironment();
+ } else {
+ env = projectState.newPrologEnvironment("stdin", new StringReader(opts.rule()));
+ }
+ } catch (CompileException err) {
+ String msg;
+ if (opts.rule() == null) {
+ msg =
+ String.format(
+ "Cannot load rules.pl for %s: %s", projectState.getName(), err.getMessage());
+ } else {
+ msg = err.getMessage();
+ }
+ throw new RuleEvalException(msg, err);
+ }
+ env.set(StoredValues.ACCOUNTS, accounts);
+ env.set(StoredValues.ACCOUNT_CACHE, accountCache);
+ env.set(StoredValues.EMAILS, emails);
+ env.set(StoredValues.REVIEW_DB, cd.db());
+ env.set(StoredValues.CHANGE_DATA, cd);
+ env.set(StoredValues.PROJECT_STATE, projectState);
+ return env;
+ }
+
+ private Term runSubmitFilters(
+ Term results,
+ PrologEnvironment env,
+ String filterRuleLocatorName,
+ String filterRuleWrapperName)
+ throws RuleEvalException {
+ PrologEnvironment childEnv = env;
+ ChangeData cd = env.get(StoredValues.CHANGE_DATA);
+ ProjectState projectState = env.get(StoredValues.PROJECT_STATE);
+ for (ProjectState parentState : projectState.parents()) {
+ PrologEnvironment parentEnv;
+ try {
+ parentEnv = parentState.newPrologEnvironment();
+ } catch (CompileException err) {
+ throw new RuleEvalException("Cannot consult rules.pl for " + parentState.getName(), err);
+ }
+
+ parentEnv.copyStoredValues(childEnv);
+ Term filterRule = parentEnv.once("gerrit", filterRuleLocatorName, new VariableTerm());
+ try {
+ Term[] template =
+ parentEnv.once(
+ "gerrit", filterRuleWrapperName, filterRule, results, new VariableTerm());
+ results = template[2];
+ } catch (ReductionLimitException err) {
+ throw new RuleEvalException(
+ String.format(
+ "%s on change %d of %s",
+ err.getMessage(), cd.getId().get(), parentState.getName()));
+ } catch (RuntimeException err) {
+ throw new RuleEvalException(
+ String.format(
+ "Exception calling %s on change %d of %s",
+ filterRule, cd.getId().get(), parentState.getName()),
+ err);
+ }
+ childEnv = parentEnv;
+ }
+ return results;
+ }
+
+ private void appliedBy(SubmitRecord.Label label, Term status) throws UserTermExpected {
+ if (status instanceof StructureTerm && status.arity() == 1) {
+ Term who = status.arg(0);
+ if (isUser(who)) {
+ label.appliedBy = new Account.Id(((IntegerTerm) who.arg(0)).intValue());
+ } else {
+ throw new UserTermExpected(label);
+ }
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/rules/SubmitRule.java b/java/com/google/gerrit/server/rules/SubmitRule.java
new file mode 100644
index 0000000..2a68683
--- /dev/null
+++ b/java/com/google/gerrit/server/rules/SubmitRule.java
@@ -0,0 +1,44 @@
+// Copyright (C) 2018 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.rules;
+
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+import com.google.gerrit.server.project.SubmitRuleOptions;
+import com.google.gerrit.server.query.change.ChangeData;
+import java.util.Collection;
+
+/**
+ * Allows plugins to decide whether a change is ready to be submitted or not.
+ *
+ * <p>For a given {@link ChangeData}, each plugin is called and returns a {@link Collection} of
+ * {@link SubmitRecord}. This collection can be empty, or contain one or several values.
+ *
+ * <p>A Change can only be submitted if all the plugins give their consent.
+ *
+ * <p>Each {@link SubmitRecord} represents a decision made by the plugin. If the plugin rejects a
+ * change, it should hold valuable informations to help the end user understand and correct the
+ * blocking points.
+ *
+ * <p>It should be noted that each plugin can handle rules inheritance.
+ *
+ * <p>This interface should be used to write pre-submit validation rules. This includes both simple
+ * checks, coded in Java, and more complex fully fledged expression evaluators (think: Prolog,
+ * JavaCC, or even JavaScript rules).
+ */
+@ExtensionPoint
+public interface SubmitRule {
+ /** Returns a {@link Collection} of {@link SubmitRecord} status for the change. */
+ Collection<SubmitRecord> evaluate(ChangeData changeData, SubmitRuleOptions options);
+}
diff --git a/java/com/google/gerrit/testing/FakeAccountCache.java b/java/com/google/gerrit/testing/FakeAccountCache.java
index a3b4d90..e549e08 100644
--- a/java/com/google/gerrit/testing/FakeAccountCache.java
+++ b/java/com/google/gerrit/testing/FakeAccountCache.java
@@ -60,7 +60,7 @@
}
@Override
- public synchronized void evictAllNoReindex() {
+ public synchronized void evictAll() {
byId.clear();
}
diff --git a/java/org/eclipse/jgit/BUILD b/java/org/eclipse/jgit/BUILD
index 65bc118..95fef28 100644
--- a/java/org/eclipse/jgit/BUILD
+++ b/java/org/eclipse/jgit/BUILD
@@ -39,8 +39,6 @@
srcs = [
"diff/EditDeserializer.java",
"diff/ReplaceEdit.java",
- "internal/storage/file/WindowCacheStatAccessor.java",
- "lib/ObjectIdSerialization.java",
],
visibility = ["//visibility:public"],
deps = [
diff --git a/java/org/eclipse/jgit/internal/storage/file/WindowCacheStatAccessor.java b/java/org/eclipse/jgit/internal/storage/file/WindowCacheStatAccessor.java
deleted file mode 100644
index f8c4340..0000000
--- a/java/org/eclipse/jgit/internal/storage/file/WindowCacheStatAccessor.java
+++ /dev/null
@@ -1,30 +0,0 @@
-// Copyright (C) 2009 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 org.eclipse.jgit.internal.storage.file;
-
-// Hack to obtain visibility to package level methods only.
-// These aren't yet part of the public JGit API.
-
-public class WindowCacheStatAccessor {
- public static int getOpenFiles() {
- return WindowCache.getInstance().getOpenFiles();
- }
-
- public static long getOpenBytes() {
- return WindowCache.getInstance().getOpenBytes();
- }
-
- private WindowCacheStatAccessor() {}
-}
diff --git a/java/org/eclipse/jgit/lib/ObjectIdSerialization.java b/java/org/eclipse/jgit/lib/ObjectIdSerialization.java
deleted file mode 100644
index c98da64..0000000
--- a/java/org/eclipse/jgit/lib/ObjectIdSerialization.java
+++ /dev/null
@@ -1,54 +0,0 @@
-// Copyright (C) 2009 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 org.eclipse.jgit.lib;
-
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.OutputStream;
-import org.eclipse.jgit.util.IO;
-
-public class ObjectIdSerialization {
- public static void writeCanBeNull(OutputStream out, AnyObjectId id) throws IOException {
- if (id != null) {
- out.write((byte) 1);
- writeNotNull(out, id);
- } else {
- out.write((byte) 0);
- }
- }
-
- public static void writeNotNull(OutputStream out, AnyObjectId id) throws IOException {
- id.copyRawTo(out);
- }
-
- public static ObjectId readCanBeNull(InputStream in) throws IOException {
- switch (in.read()) {
- case 0:
- return null;
- case 1:
- return readNotNull(in);
- default:
- throw new IOException("Invalid flag before ObjectId");
- }
- }
-
- public static ObjectId readNotNull(InputStream in) throws IOException {
- final byte[] b = new byte[20];
- IO.readFully(in, b, 0, 20);
- return ObjectId.fromRaw(b);
- }
-
- private ObjectIdSerialization() {}
-}
diff --git a/javatests/com/google/gerrit/acceptance/ProjectResetterTest.java b/javatests/com/google/gerrit/acceptance/ProjectResetterTest.java
index 6c3a4b0..fce28de 100644
--- a/javatests/com/google/gerrit/acceptance/ProjectResetterTest.java
+++ b/javatests/com/google/gerrit/acceptance/ProjectResetterTest.java
@@ -25,6 +25,7 @@
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
+import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.testing.GerritBaseTests;
import com.google.gerrit.testing.InMemoryRepositoryManager;
@@ -71,7 +72,8 @@
public void resetAllRefs() throws Exception {
Ref matchingRef = createRef("refs/any/test");
- try (ProjectResetter resetProject = builder().reset(project).build()) {
+ try (ProjectResetter resetProject =
+ builder().build(new ProjectResetter.Config().reset(project))) {
updateRef(matchingRef);
}
@@ -87,7 +89,10 @@
Ref updatedNonMatchingRef;
try (ProjectResetter resetProject =
- builder().reset(project, "refs/match/*", "refs/another-match/*").build()) {
+ builder()
+ .build(
+ new ProjectResetter.Config()
+ .reset(project, "refs/match/*", "refs/another-match/*"))) {
updateRef(matchingRef);
updateRef(anotherMatchingRef);
updatedNonMatchingRef = updateRef(nonMatchingRef);
@@ -107,7 +112,10 @@
Ref anotherMatchingRef;
Ref nonMatchingRef;
try (ProjectResetter resetProject =
- builder().reset(project, "refs/match/*", "refs/another-match/*").build()) {
+ builder()
+ .build(
+ new ProjectResetter.Config()
+ .reset(project, "refs/match/*", "refs/another-match/*"))) {
matchingRef = createRef("refs/match/test");
anotherMatchingRef = createRef("refs/another-match/test");
nonMatchingRef = createRef("refs/no-match/test");
@@ -135,7 +143,11 @@
Ref updatedNonMatchingRefProject1;
Ref updatedNonMatchingRefProject2;
try (ProjectResetter resetProject =
- builder().reset(project, "refs/foo/*").reset(project2, "refs/bar/*").build()) {
+ builder()
+ .build(
+ new ProjectResetter.Config()
+ .reset(project, "refs/foo/*")
+ .reset(project2, "refs/bar/*"))) {
updateRef(matchingRefProject1);
updatedNonMatchingRefProject1 = updateRef(nonMatchingRefProject1);
@@ -162,7 +174,11 @@
Ref matchingRefProject2;
Ref nonMatchingRefProject2;
try (ProjectResetter resetProject =
- builder().reset(project, "refs/foo/*").reset(project2, "refs/bar/*").build()) {
+ builder()
+ .build(
+ new ProjectResetter.Config()
+ .reset(project, "refs/foo/*")
+ .reset(project2, "refs/bar/*"))) {
matchingRefProject1 = createRef("refs/foo/test");
nonMatchingRefProject1 = createRef("refs/bar/test");
@@ -183,7 +199,9 @@
public void onlyDeleteNewlyCreatedWithOverlappingRefPatterns() throws Exception {
Ref matchingRef;
try (ProjectResetter resetProject =
- builder().reset(project, "refs/match/*", "refs/match/test").build()) {
+ builder()
+ .build(
+ new ProjectResetter.Config().reset(project, "refs/match/*", "refs/match/test"))) {
// This ref matches 2 ref pattern, ProjectResetter should try to delete it only once.
matchingRef = createRef("refs/match/test");
}
@@ -206,7 +224,8 @@
Ref nonMetaConfig = createRef("refs/heads/master");
try (ProjectResetter resetProject =
- builder(null, null, projectCache).reset(project).reset(project2).build()) {
+ builder(null, null, null, projectCache)
+ .build(new ProjectResetter.Config().reset(project).reset(project2))) {
updateRef(nonMetaConfig);
updateRef(repo2, metaConfig);
}
@@ -225,7 +244,8 @@
EasyMock.replay(projectCache);
try (ProjectResetter resetProject =
- builder(null, null, projectCache).reset(project).reset(project2).build()) {
+ builder(null, null, null, projectCache)
+ .build(new ProjectResetter.Config().reset(project).reset(project2))) {
createRef("refs/heads/master");
createRef(repo2, RefNames.REFS_CONFIG);
}
@@ -245,16 +265,22 @@
EasyMock.expectLastCall();
EasyMock.replay(accountCache);
+ AccountIndexer accountIndexer = EasyMock.createNiceMock(AccountIndexer.class);
+ accountIndexer.index(accountId);
+ EasyMock.expectLastCall();
+ EasyMock.replay(accountIndexer);
+
// Non-user branch because it's not in All-Users.
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(2)));
try (ProjectResetter resetProject =
- builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
+ builder(null, accountCache, accountIndexer, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
updateRef(allUsersRepo, userBranch);
}
- EasyMock.verify(accountCache);
+ EasyMock.verify(accountCache, accountIndexer);
}
@Test
@@ -268,15 +294,21 @@
EasyMock.expectLastCall();
EasyMock.replay(accountCache);
+ AccountIndexer accountIndexer = EasyMock.createNiceMock(AccountIndexer.class);
+ accountIndexer.index(accountId);
+ EasyMock.expectLastCall();
+ EasyMock.replay(accountIndexer);
+
try (ProjectResetter resetProject =
- builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
+ builder(null, accountCache, accountIndexer, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
// Non-user branch because it's not in All-Users.
createRef(RefNames.refsUsers(new Account.Id(2)));
createRef(allUsersRepo, RefNames.refsUsers(accountId));
}
- EasyMock.verify(accountCache);
+ EasyMock.verify(accountCache, accountIndexer);
}
@Test
@@ -296,17 +328,25 @@
EasyMock.expectLastCall();
EasyMock.replay(accountCache);
+ AccountIndexer accountIndexer = EasyMock.createNiceMock(AccountIndexer.class);
+ accountIndexer.index(accountId);
+ EasyMock.expectLastCall();
+ accountIndexer.index(accountId2);
+ EasyMock.expectLastCall();
+ EasyMock.replay(accountIndexer);
+
// Non-user branch because it's not in All-Users.
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(3)));
try (ProjectResetter resetProject =
- builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
+ builder(null, accountCache, accountIndexer, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
updateRef(allUsersRepo, externalIds);
createRef(allUsersRepo, RefNames.refsUsers(accountId2));
}
- EasyMock.verify(accountCache);
+ EasyMock.verify(accountCache, accountIndexer);
}
@Test
@@ -325,17 +365,25 @@
EasyMock.expectLastCall();
EasyMock.replay(accountCache);
+ AccountIndexer accountIndexer = EasyMock.createNiceMock(AccountIndexer.class);
+ accountIndexer.index(accountId);
+ EasyMock.expectLastCall();
+ accountIndexer.index(accountId2);
+ EasyMock.expectLastCall();
+ EasyMock.replay(accountIndexer);
+
// Non-user branch because it's not in All-Users.
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(3)));
try (ProjectResetter resetProject =
- builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
+ builder(null, accountCache, accountIndexer, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
createRef(allUsersRepo, RefNames.REFS_EXTERNAL_IDS);
createRef(allUsersRepo, RefNames.refsUsers(accountId2));
}
- EasyMock.verify(accountCache);
+ EasyMock.verify(accountCache, accountIndexer);
}
@Test
@@ -350,7 +398,8 @@
EasyMock.replay(accountCreator);
try (ProjectResetter resetProject =
- builder(accountCreator, null, null).reset(project).reset(allUsers).build()) {
+ builder(accountCreator, null, null, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
createRef(allUsersRepo, RefNames.refsUsers(accountId));
}
@@ -425,18 +474,20 @@
}
private ProjectResetter.Builder builder() {
- return builder(null, null, null);
+ return builder(null, null, null, null);
}
private ProjectResetter.Builder builder(
@Nullable AccountCreator accountCreator,
@Nullable AccountCache accountCache,
+ @Nullable AccountIndexer accountIndexer,
@Nullable ProjectCache projectCache) {
return new ProjectResetter.Builder(
repoManager,
new AllUsersName(AllUsersNameProvider.DEFAULT),
accountCreator,
accountCache,
+ accountIndexer,
projectCache);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index f8781b5..83d166a 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -1982,13 +1982,11 @@
assertGroups(
admin.username, ImmutableList.of("Anonymous Users", "Registered Users", "Administrators"));
- // TODO: update when test user is fixed to be included in "Anonymous Users" and
- // "Registered Users" groups
- assertGroups(user.username, ImmutableList.of());
+ assertGroups(user.username, ImmutableList.of("Anonymous Users", "Registered Users"));
String group = createGroup("group");
String newUser = createAccount("user1", group);
- assertGroups(newUser, ImmutableList.of(group));
+ assertGroups(newUser, ImmutableList.of("Anonymous Users", "Registered Users", group));
}
@Test
@@ -2367,11 +2365,14 @@
}
private void assertGroups(String user, List<String> expected) throws Exception {
- List<String> actual =
- gApi.accounts().id(user).getGroups().stream().map(g -> g.name).collect(toList());
+ List<String> actual = getNamesOfGroupsOfUser(user);
assertThat(actual).containsExactlyElementsIn(expected);
}
+ private List<String> getNamesOfGroupsOfUser(String user) throws RestApiException {
+ return gApi.accounts().id(user).getGroups().stream().map(g -> g.name).collect(toList());
+ }
+
private void assertSequenceNumbers(List<SshKeyInfo> sshKeys) {
int seq = 1;
for (SshKeyInfo key : sshKeys) {
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIndexerIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIndexerIT.java
new file mode 100644
index 0000000..310a1e19
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIndexerIT.java
@@ -0,0 +1,170 @@
+// Copyright (C) 2018 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.acceptance.api.accounts;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
+
+import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountConfig;
+import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.account.InternalAccountUpdate;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.MetaDataUpdate;
+import com.google.gerrit.server.index.account.AccountIndexer;
+import com.google.gerrit.server.query.account.InternalAccountQuery;
+import com.google.gerrit.testing.InMemoryTestEnvironment;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.io.IOException;
+import java.util.List;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Rule;
+import org.junit.Test;
+
+public class AccountIndexerIT {
+ @Rule public InMemoryTestEnvironment testEnvironment = new InMemoryTestEnvironment();
+
+ @Inject private AccountIndexer accountIndexer;
+ @Inject private GerritApi gApi;
+ @Inject private AccountCache accountCache;
+ @Inject private Provider<InternalAccountQuery> accountQueryProvider;
+ @Inject private GitRepositoryManager repoManager;
+ @Inject private AllUsersName allUsersName;
+ @Inject @GerritPersonIdent protected Provider<PersonIdent> serverIdent;
+
+ @Test
+ public void indexingUpdatesTheIndex() throws Exception {
+ Account.Id accountId = createAccount("foo");
+ String preferredEmail = "foo@example.com";
+ updateAccountWithoutCacheOrIndex(
+ accountId, newAccountUpdate().setPreferredEmail(preferredEmail).build());
+ assertThat(accountQueryProvider.get().byPreferredEmail(preferredEmail)).isEmpty();
+
+ accountIndexer.index(accountId);
+ List<AccountState> matchedAccountStates =
+ accountQueryProvider.get().byPreferredEmail(preferredEmail);
+ assertThat(matchedAccountStates).hasSize(1);
+ assertThat(matchedAccountStates.get(0).getAccount().getId()).isEqualTo(accountId);
+ }
+
+ @Test
+ public void indexCannotBeCorruptedByStaleCache() throws Exception {
+ Account.Id accountId = createAccount("foo");
+ loadAccountToCache(accountId);
+ String preferredEmail = "foo@example.com";
+ updateAccountWithoutCacheOrIndex(
+ accountId, newAccountUpdate().setPreferredEmail(preferredEmail).build());
+ assertThat(accountQueryProvider.get().byPreferredEmail(preferredEmail)).isEmpty();
+
+ accountIndexer.index(accountId);
+ List<AccountState> matchedAccountStates =
+ accountQueryProvider.get().byPreferredEmail(preferredEmail);
+ assertThat(matchedAccountStates).hasSize(1);
+ assertThat(matchedAccountStates.get(0).getAccount().getId()).isEqualTo(accountId);
+ }
+
+ @Test
+ public void indexingUpdatesStaleCache() throws Exception {
+ Account.Id accountId = createAccount("foo");
+ loadAccountToCache(accountId);
+ String status = "ooo";
+ updateAccountWithoutCacheOrIndex(accountId, newAccountUpdate().setStatus(status).build());
+ assertThat(accountCache.get(accountId).get().getAccount().getStatus()).isNull();
+
+ accountIndexer.index(accountId);
+ assertThat(accountCache.get(accountId).get().getAccount().getStatus()).isEqualTo(status);
+ }
+
+ @Test
+ public void reindexingStaleAccountUpdatesTheIndex() throws Exception {
+ Account.Id accountId = createAccount("foo");
+ String preferredEmail = "foo@example.com";
+ updateAccountWithoutCacheOrIndex(
+ accountId, newAccountUpdate().setPreferredEmail(preferredEmail).build());
+ assertThat(accountQueryProvider.get().byPreferredEmail(preferredEmail)).isEmpty();
+
+ accountIndexer.reindexIfStale(accountId);
+ List<AccountState> matchedAccountStates =
+ accountQueryProvider.get().byPreferredEmail(preferredEmail);
+ assertThat(matchedAccountStates).hasSize(1);
+ assertThat(matchedAccountStates.get(0).getAccount().getId()).isEqualTo(accountId);
+ }
+
+ @Test
+ public void notStaleAccountIsNotReindexed() throws Exception {
+ Account.Id accountId = createAccount("foo");
+ updateAccountWithoutCacheOrIndex(
+ accountId, newAccountUpdate().setPreferredEmail("foo@example.com").build());
+ accountIndexer.index(accountId);
+
+ boolean reindexed = accountIndexer.reindexIfStale(accountId);
+ assertWithMessage("Account should not have been reindexed").that(reindexed).isFalse();
+ }
+
+ @Test
+ public void indexStalenessIsNotDerivedFromCacheStaleness() throws Exception {
+ Account.Id accountId = createAccount("foo");
+ updateAccountWithoutCacheOrIndex(
+ accountId, newAccountUpdate().setPreferredEmail("foo@example.com").build());
+ reloadAccountToCache(accountId);
+
+ boolean reindexed = accountIndexer.reindexIfStale(accountId);
+ assertWithMessage("Account should have been reindexed").that(reindexed).isTrue();
+ }
+
+ private Account.Id createAccount(String name) throws RestApiException {
+ AccountInfo account = gApi.accounts().create(name).get();
+ return new Account.Id(account._accountId);
+ }
+
+ private void reloadAccountToCache(Account.Id accountId) {
+ accountCache.evict(accountId);
+ loadAccountToCache(accountId);
+ }
+
+ private void loadAccountToCache(Account.Id accountId) {
+ accountCache.get(accountId);
+ }
+
+ private static InternalAccountUpdate.Builder newAccountUpdate() {
+ return InternalAccountUpdate.builder();
+ }
+
+ private void updateAccountWithoutCacheOrIndex(
+ Account.Id accountId, InternalAccountUpdate accountUpdate)
+ throws IOException, ConfigInvalidException {
+ try (Repository allUsersRepo = repoManager.openRepository(allUsersName);
+ MetaDataUpdate md =
+ new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, allUsersRepo)) {
+ PersonIdent ident = serverIdent.get();
+ md.getCommitBuilder().setAuthor(ident);
+ md.getCommitBuilder().setCommitter(ident);
+
+ AccountConfig accountConfig = new AccountConfig(accountId, allUsersRepo).load();
+ accountConfig.setAccountUpdate(accountUpdate);
+ accountConfig.commit(md);
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 8f27de3..eb40513 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -80,6 +80,7 @@
import com.google.gerrit.extensions.api.changes.NotifyInfo;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.changes.RecipientType;
+import com.google.gerrit.extensions.api.changes.RevertInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.api.changes.ReviewResult;
@@ -168,7 +169,6 @@
import org.eclipse.jgit.transport.PushResult;
import org.junit.After;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Test;
@NoHttpd
@@ -675,6 +675,37 @@
}
@Test
+ public void revertNotifications() throws Exception {
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).addReviewer(user.email);
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
+
+ sender.clear();
+ ChangeInfo revertChange = gApi.changes().id(r.getChangeId()).revert().get();
+
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(2);
+ assertThat(sender.getMessages(revertChange.changeId, "newchange")).hasSize(1);
+ assertThat(sender.getMessages(r.getChangeId(), "revert")).hasSize(1);
+ }
+
+ @Test
+ public void suppressRevertNotifications() throws Exception {
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).addReviewer(user.email);
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
+
+ RevertInput revertInput = new RevertInput();
+ revertInput.notify = NotifyHandling.NONE;
+
+ sender.clear();
+ gApi.changes().id(r.getChangeId()).revert(revertInput).get();
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
+ @Test
public void revertPreservesReviewersAndCcs() throws Exception {
PushOneCommit.Result r = createChange();
@@ -1506,7 +1537,6 @@
assertThat(sender.getMessages()).hasSize(1);
}
- @Ignore
@Test
public void addReviewerThatIsNotPerfectMatch() throws Exception {
TestTimeUtil.resetWithClockStep(1, SECONDS);
@@ -1551,7 +1581,6 @@
assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
}
- @Ignore
@Test
public void addGroupAsReviewersWhenANotPerfectMatchedUserExists() throws Exception {
TestTimeUtil.resetWithClockStep(1, SECONDS);
@@ -1560,16 +1589,17 @@
String oldETag = rsrc.getETag();
Timestamp oldTs = rsrc.getChange().getLastUpdatedOn();
- //create a group named "us" with one user: testUser
- TestAccount testUser = accountCreator.create("testUser", "testUser@test.com", "testUser");
- String testGroup =
- createGroupWithRealName(user.fullName.substring(0, user.fullName.length() / 2));
+ //create a group named "kobe" with one user: lee
+ TestAccount testUser = accountCreator.create("kobebryant", "kobebryant@test.com", "kobebryant");
+ TestAccount myGroupUser = accountCreator.create("lee", "lee@test.com", "lee");
+
+ String testGroup = createGroupWithRealName("kobe");
GroupApi groupApi = gApi.groups().id(testGroup);
groupApi.description("test group");
- groupApi.addMembers(testUser.fullName);
+ groupApi.addMembers(myGroupUser.fullName);
//ensure that user "user" is not in the group
- groupApi.removeMembers(user.fullName);
+ groupApi.removeMembers(testUser.fullName);
AddReviewerInput in = new AddReviewerInput();
in.reviewer = testGroup;
@@ -1578,11 +1608,11 @@
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
Message m = messages.get(0);
- assertThat(m.rcpt()).containsExactly(testUser.emailAddress);
- assertThat(m.body()).contains("Hello " + testUser.fullName + ",\n");
+ assertThat(m.rcpt()).containsExactly(myGroupUser.emailAddress);
+ assertThat(m.body()).contains("Hello " + myGroupUser.fullName + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
- assertMailReplyTo(m, testUser.email);
+ assertMailReplyTo(m, myGroupUser.email);
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
// When NoteDb is enabled adding a reviewer records that user as reviewer
@@ -1592,7 +1622,7 @@
Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
- assertThat(reviewers.iterator().next()._accountId).isEqualTo(testUser.getId().get());
+ assertThat(reviewers.iterator().next()._accountId).isEqualTo(myGroupUser.getId().get());
// Ensure ETag and lastUpdatedOn are updated.
rsrc = parseResource(r);
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 2d5703d..8aa03a4 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -40,7 +40,6 @@
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.ProjectResetter;
-import com.google.gerrit.acceptance.ProjectResetter.Builder;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestAccount;
@@ -166,11 +165,11 @@
}
@Override
- protected ProjectResetter resetProjects(Builder resetter) throws IOException {
+ protected ProjectResetter.Config resetProjects() {
// Don't reset All-Users since deleting users makes groups inconsistent (e.g. groups would
// contain members that no longer exist) and as result of this the group consistency checker
// that is executed after each test would fail.
- return resetter.reset(allProjects, RefNames.REFS_CONFIG).build();
+ return new ProjectResetter.Config().reset(allProjects, RefNames.REFS_CONFIG);
}
@Test
@@ -281,7 +280,7 @@
List<? extends GroupAuditEventInfo> auditEvents = gApi.groups().id(g).auditLog();
assertThat(auditEvents).hasSize(1);
- assertAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, "Registered Users");
+ assertSubgroupAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, "Registered Users");
}
@Test
@@ -858,41 +857,41 @@
GroupApi g = gApi.groups().create(name("group"));
List<? extends GroupAuditEventInfo> auditEvents = g.auditLog();
assertThat(auditEvents).hasSize(1);
- assertAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, admin.id);
+ assertMemberAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, admin.id);
g.addMembers(user.username);
auditEvents = g.auditLog();
assertThat(auditEvents).hasSize(2);
- assertAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id);
+ assertMemberAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id);
g.removeMembers(user.username);
auditEvents = g.auditLog();
assertThat(auditEvents).hasSize(3);
- assertAuditEvent(auditEvents.get(0), Type.REMOVE_USER, admin.id, user.id);
+ assertMemberAuditEvent(auditEvents.get(0), Type.REMOVE_USER, admin.id, user.id);
String otherGroup = name("otherGroup");
gApi.groups().create(otherGroup);
g.addGroups(otherGroup);
auditEvents = g.auditLog();
assertThat(auditEvents).hasSize(4);
- assertAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup);
+ assertSubgroupAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup);
g.removeGroups(otherGroup);
auditEvents = g.auditLog();
assertThat(auditEvents).hasSize(5);
- assertAuditEvent(auditEvents.get(0), Type.REMOVE_GROUP, admin.id, otherGroup);
+ assertSubgroupAuditEvent(auditEvents.get(0), Type.REMOVE_GROUP, admin.id, otherGroup);
// Add a removed member back again.
g.addMembers(user.username);
auditEvents = g.auditLog();
assertThat(auditEvents).hasSize(6);
- assertAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id);
+ assertMemberAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id);
// Add a removed group back again.
g.addGroups(otherGroup);
auditEvents = g.auditLog();
assertThat(auditEvents).hasSize(7);
- assertAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup);
+ assertSubgroupAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup);
Timestamp lastDate = null;
for (GroupAuditEventInfo auditEvent : auditEvents) {
@@ -903,6 +902,52 @@
}
}
+ /**
+ * @Sandboxed is used by this test because it deletes a group reference which introduces an
+ * inconsistency for the group storage. Once group deletion is supported, this test should be
+ * updated to use the API instead.
+ */
+ @Test
+ @Sandboxed
+ @IgnoreGroupInconsistencies
+ public void getAuditLogAfterDeletingASubgroup() throws Exception {
+ assume().that(readGroupsFromNoteDb()).isTrue();
+
+ GroupInfo parentGroup = gApi.groups().create(name("parent-group")).get();
+
+ // Creates a subgroup and adds it to "parent-group" as a subgroup.
+ GroupInfo subgroup = gApi.groups().create(name("sub-group")).get();
+ gApi.groups().id(parentGroup.id).addGroups(subgroup.id);
+
+ // Deletes the subgroup.
+ deleteGroupRef(subgroup.id);
+
+ List<? extends GroupAuditEventInfo> auditEvents = gApi.groups().id(parentGroup.id).auditLog();
+ assertThat(auditEvents).hasSize(2);
+ // Verify the unavailable subgroup's name is null.
+ assertSubgroupAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, null);
+ }
+
+ private void deleteGroupRef(String groupId) throws Exception {
+ AccountGroup.UUID uuid = new AccountGroup.UUID(groupId);
+ try (Repository repo = repoManager.openRepository(allUsers)) {
+ RefUpdate ru = repo.updateRef(RefNames.refsGroups(uuid));
+ ru.setForceUpdate(true);
+ ru.setNewObjectId(ObjectId.zeroId());
+ assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
+ }
+
+ // Reindex the group.
+ gApi.groups().id(uuid.get()).index();
+
+ // Verify "sub-group" has been deleted.
+ try {
+ gApi.groups().id(uuid.get()).get();
+ fail();
+ } catch (ResourceNotFoundException e) {
+ }
+ }
+
// reindex is tested by {@link AbstractQueryGroupsTest#reindex}
@Test
public void reindexPermissions() throws Exception {
@@ -1076,7 +1121,9 @@
// Use ProjectResetter to restore the group names ref
try (ProjectResetter resetter =
- projectResetter.builder().reset(allUsers, RefNames.REFS_GROUPNAMES).build()) {
+ projectResetter
+ .builder()
+ .build(new ProjectResetter.Config().reset(allUsers, RefNames.REFS_GROUPNAMES))) {
// Manually delete group names ref
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {
@@ -1410,7 +1457,7 @@
}
}
- private void assertAuditEvent(
+ private void assertMemberAuditEvent(
GroupAuditEventInfo info,
Type expectedType,
Account.Id expectedUser,
@@ -1421,7 +1468,7 @@
assertThat(((UserMemberAuditEventInfo) info).member._accountId).isEqualTo(expectedMember.get());
}
- private void assertAuditEvent(
+ private void assertSubgroupAuditEvent(
GroupAuditEventInfo info,
Type expectedType,
Account.Id expectedUser,
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
index c986c5e..84cfb0d 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
@@ -158,6 +158,38 @@
}
@Test
+ public void createProjectWithNonExistingParent() throws Exception {
+ ProjectInput in = new ProjectInput();
+ in.name = name("baz");
+ in.parent = "non-existing";
+
+ exception.expect(UnprocessableEntityException.class);
+ exception.expectMessage("Project Not Found: " + in.parent);
+ gApi.projects().create(in);
+ }
+
+ @Test
+ public void createProjectWithSelfAsParentNotPossible() throws Exception {
+ ProjectInput in = new ProjectInput();
+ in.name = name("baz");
+ in.parent = in.name;
+
+ exception.expect(UnprocessableEntityException.class);
+ exception.expectMessage("Project Not Found: " + in.parent);
+ gApi.projects().create(in);
+ }
+
+ @Test
+ public void createProjectUnderAllUsersNotAllowed() throws Exception {
+ ProjectInput in = new ProjectInput();
+ in.name = name("foo");
+ in.parent = allUsers.get();
+ exception.expect(ResourceConflictException.class);
+ exception.expectMessage(String.format("Cannot inherit from '%s' project", allUsers.get()));
+ gApi.projects().create(in);
+ }
+
+ @Test
public void createAndDeleteBranch() throws Exception {
assertThat(getRemoteHead(project.get(), "foo")).isNull();
diff --git a/javatests/com/google/gerrit/acceptance/api/project/SetParentIT.java b/javatests/com/google/gerrit/acceptance/api/project/SetParentIT.java
index c1bc83e..ec4f327 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/SetParentIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/SetParentIT.java
@@ -90,6 +90,13 @@
}
@Test
+ public void setParentToAllUsersNotAllowed() throws Exception {
+ exception.expect(ResourceConflictException.class);
+ exception.expectMessage(String.format("Cannot inherit from '%s' project", allUsers.get()));
+ gApi.projects().name(project.get()).parent(allUsers.get());
+ }
+
+ @Test
public void setParentForAllUsersMustBeAllProjects() throws Exception {
gApi.projects().name(allUsers.get()).parent(allProjects.get());
diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index e0c16e0f..2c95e27 100644
--- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -785,7 +785,8 @@
private ProjectResetter resetGroups() throws IOException {
return projectResetter
.builder()
- .reset(allUsers, RefNames.REFS_GROUPS + "*", RefNames.REFS_GROUPNAMES)
- .build();
+ .build(
+ new ProjectResetter.Config()
+ .reset(allUsers, RefNames.REFS_GROUPS + "*", RefNames.REFS_GROUPNAMES));
}
}
diff --git a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
index 7583830..2812c86 100644
--- a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
@@ -346,17 +346,17 @@
"master",
"Update git submodules\n\n"
+ "* Update "
- + name("sub3")
+ + name("sub1")
+ " from branch 'master'\n to "
- + sub3HEAD
+ + sub1HEAD
+ "\n\n* Update "
+ name("sub2")
+ " from branch 'master'\n to "
+ sub2HEAD
+ "\n\n* Update "
- + name("sub1")
+ + name("sub3")
+ " from branch 'master'\n to "
- + sub1HEAD);
+ + sub3HEAD);
}
superRepo
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java b/javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java
index c1bbf2e..c78b47b 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.project.ProjectState;
+import java.util.Arrays;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.junit.Before;
@@ -114,4 +115,67 @@
assertThat(state.getConfig(configName).get().toText()).isEqualTo(cfg.toText());
}
+
+ @Test
+ public void withMergedInheritance() throws Exception {
+ String configName = "test.config";
+
+ Config parentCfg = new Config();
+ parentCfg.setString("s1", null, "k1", "parentValue1");
+ parentCfg.setString("s1", null, "k2", "parentValue2");
+ parentCfg.setString("s2", "ss", "k3", "parentValue3");
+ parentCfg.setString("s2", "ss", "k4", "parentValue4");
+
+ pushFactory
+ .create(
+ db,
+ admin.getIdent(),
+ testRepo,
+ "Create Project Level Config",
+ configName,
+ parentCfg.toText())
+ .to(RefNames.REFS_CONFIG)
+ .assertOkStatus();
+
+ Project.NameKey childProject = createProject("child", project);
+ TestRepository<?> childTestRepo = cloneProject(childProject);
+ fetch(childTestRepo, RefNames.REFS_CONFIG + ":refs/heads/config");
+ childTestRepo.reset("refs/heads/config");
+
+ Config cfg = new Config();
+ cfg.setString("s1", null, "k1", "parentValue1");
+ cfg.setString("s1", null, "k2", "parentValue2");
+ cfg.setString("s2", "ss", "k3", "parentValue3");
+ cfg.setString("s2", "ss", "k4", "parentValue4");
+ cfg.setString("s1", null, "k1", "childValue1");
+ cfg.setString("s2", "ss", "k3", "childValue2");
+ cfg.setString("s3", null, "k5", "childValue3");
+ cfg.setString("s3", "ss", "k6", "childValue4");
+
+ pushFactory
+ .create(
+ db,
+ admin.getIdent(),
+ childTestRepo,
+ "Create Project Level Config",
+ configName,
+ cfg.toText())
+ .to(RefNames.REFS_CONFIG)
+ .assertOkStatus();
+
+ ProjectState state = projectCache.get(childProject);
+
+ Config expectedCfg = new Config();
+ expectedCfg.setStringList("s1", null, "k1", Arrays.asList("childValue1", "parentValue1"));
+ expectedCfg.setString("s1", null, "k2", "parentValue2");
+ expectedCfg.setStringList("s2", "ss", "k3", Arrays.asList("childValue2", "parentValue3"));
+ expectedCfg.setString("s2", "ss", "k4", "parentValue4");
+ expectedCfg.setString("s3", null, "k5", "childValue3");
+ expectedCfg.setString("s3", "ss", "k6", "childValue4");
+
+ assertThat(state.getConfig(configName).getWithInheritance(true).toText())
+ .isEqualTo(expectedCfg.toText());
+
+ assertThat(state.getConfig(configName).get().toText()).isEqualTo(cfg.toText());
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index 417c9e3..a6c1153 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -2219,17 +2219,22 @@
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageChange();
revert(sc, sc.owner);
+
+ // email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.reviewer, sc.ccer, sc.watchingProjectOwner, admin)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
+ // email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.cc(sc.reviewer, sc.ccer, admin)
+ .cc(sc.reviewerByEmail)
+ .bcc(sc.starrer)
.bcc(ALL_COMMENTS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
}
@Test
@@ -2237,18 +2242,23 @@
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageChange();
revert(sc, sc.owner);
+
+ // email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.reviewer, sc.watchingProjectOwner, admin)
.cc(sc.ccer)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
+ // email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.cc(sc.reviewer, sc.ccer, admin)
+ .cc(sc.reviewerByEmail, sc.ccerByEmail)
+ .bcc(sc.starrer)
.bcc(ALL_COMMENTS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
}
@Test
@@ -2256,19 +2266,24 @@
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageChange();
revert(sc, sc.owner, CC_ON_OWN_COMMENTS);
+
+ // email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.reviewer, sc.ccer, sc.watchingProjectOwner, admin)
.cc(sc.owner)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
+ // email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.to(sc.owner)
.cc(sc.reviewer, sc.ccer, admin)
+ .cc(sc.reviewerByEmail)
+ .bcc(sc.starrer)
.bcc(ALL_COMMENTS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
}
@Test
@@ -2276,19 +2291,24 @@
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageChange();
revert(sc, sc.owner, CC_ON_OWN_COMMENTS);
+
+ // email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.reviewer, sc.watchingProjectOwner, admin)
.cc(sc.owner, sc.ccer)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
+ // email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.to(sc.owner)
.cc(sc.reviewer, sc.ccer, admin)
+ .cc(sc.reviewerByEmail, sc.ccerByEmail)
+ .bcc(sc.starrer)
.bcc(ALL_COMMENTS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
}
@Test
@@ -2296,17 +2316,23 @@
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageChange();
revert(sc, other);
+
+ // email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.owner, sc.reviewer, sc.ccer, sc.watchingProjectOwner, admin)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
+ // email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
- .cc(sc.owner, sc.reviewer, sc.ccer, admin)
+ .to(sc.owner)
+ .cc(sc.reviewer, sc.ccer, admin)
+ .cc(sc.reviewerByEmail)
+ .bcc(sc.starrer)
.bcc(ALL_COMMENTS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
}
@Test
@@ -2314,18 +2340,24 @@
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageChange();
revert(sc, other);
+
+ // email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.owner, sc.reviewer, sc.watchingProjectOwner, admin)
.cc(sc.ccer)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
+ // email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
- .cc(sc.owner, sc.reviewer, sc.ccer, admin)
+ .to(sc.owner)
+ .cc(sc.reviewer, sc.ccer, admin)
+ .cc(sc.reviewerByEmail, sc.ccerByEmail)
+ .bcc(sc.starrer)
.bcc(ALL_COMMENTS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
}
@Test
@@ -2333,19 +2365,24 @@
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageChange();
revert(sc, other, CC_ON_OWN_COMMENTS);
+
+ // email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.owner, sc.reviewer, sc.ccer, sc.watchingProjectOwner, admin)
.cc(other)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
+ // email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
- .to(other)
- .cc(sc.owner, sc.reviewer, sc.ccer, admin)
+ .to(sc.owner)
+ .cc(other, sc.reviewer, sc.ccer, admin)
+ .cc(sc.reviewerByEmail)
+ .bcc(sc.starrer)
.bcc(ALL_COMMENTS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
}
@Test
@@ -2353,19 +2390,24 @@
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageChange();
revert(sc, other, CC_ON_OWN_COMMENTS);
+
+ // email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.owner, sc.reviewer, sc.watchingProjectOwner, admin)
.cc(sc.ccer, other)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
+ // email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
- .to(other)
- .cc(sc.owner, sc.reviewer, sc.ccer, admin)
+ .to(sc.owner)
+ .cc(other, sc.reviewer, sc.ccer, admin)
+ .cc(sc.reviewerByEmail, sc.ccerByEmail)
+ .bcc(sc.starrer)
.bcc(ALL_COMMENTS)
- .noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
+ .noOneElse();
}
private StagedChange stageChange() throws Exception {
diff --git a/javatests/com/google/gerrit/acceptance/server/rules/BUILD b/javatests/com/google/gerrit/acceptance/server/rules/BUILD
new file mode 100644
index 0000000..2e96c0b
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/rules/BUILD
@@ -0,0 +1,10 @@
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
+
+acceptance_tests(
+ srcs = glob(["*IT.java"]),
+ group = "server_rules",
+ labels = ["server"],
+ deps = [
+ "@prolog_runtime//jar",
+ ],
+)
diff --git a/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java b/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java
new file mode 100644
index 0000000..8fc32b4
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java
@@ -0,0 +1,160 @@
+// Copyright (C) 2018 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.acceptance.server.rules;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.project.SubmitRuleOptions;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.PrologRuleEvaluator;
+import com.google.gerrit.testing.TestChanges;
+import com.google.inject.Inject;
+import com.googlecode.prolog_cafe.lang.IntegerTerm;
+import com.googlecode.prolog_cafe.lang.StructureTerm;
+import com.googlecode.prolog_cafe.lang.Term;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.junit.Test;
+
+public class PrologRuleEvaluatorIT extends AbstractDaemonTest {
+ @Inject private PrologRuleEvaluator.Factory evaluatorFactory;
+
+ @Test
+ public void convertsPrologToSubmitRecord() {
+ PrologRuleEvaluator evaluator = makeEvaluator();
+
+ StructureTerm verifiedLabel = makeLabel("Verified", "may");
+ StructureTerm labels = new StructureTerm("label", verifiedLabel);
+
+ List<Term> terms = ImmutableList.of(makeTerm("ok", labels));
+ Collection<SubmitRecord> records = evaluator.resultsToSubmitRecord(null, terms);
+
+ assertThat(records).hasSize(1);
+ }
+
+ /**
+ * The Prolog behavior is everything but intuitive. Several submit_rules can be defined, and each
+ * will provide a different SubmitRecord answer when queried. The current implementation stops
+ * parsing the Prolog terms into SubmitRecord objects once it finds an OK record. This might lead
+ * to tangling results, as reproduced by this test.
+ *
+ * <p>Let's consider this rules.pl file (equivalent to the code in this test)
+ *
+ * <pre>{@code
+ * submit_rule(submit(R)) :-
+ * gerrit:uploader(U),
+ * R = label('Verified', reject(U)).
+ *
+ * submit_rule(submit(CR, V)) :-
+ * gerrit:uploader(U),
+ * V = label('Code-Review', ok(U)).
+ *
+ * submit_rule(submit(R)) :-
+ * gerrit:uploader(U),
+ * R = label('Any-Label-Name', reject(U)).
+ * }</pre>
+ *
+ * The first submit_rule always fails because the Verified label is rejected.
+ *
+ * <p>The second submit_rule is always valid, and provides two labels: OK and Code-Review.
+ *
+ * <p>The third submit_rule always fails because the Any-Label-Name label is rejected.
+ *
+ * <p>In this case, the last two SubmitRecords are used, the first one is discarded.
+ */
+ @Test
+ public void abortsEarlyWithOkayRecord() {
+ PrologRuleEvaluator evaluator = makeEvaluator();
+
+ SubmitRecord.Label submitRecordLabel1 = new SubmitRecord.Label();
+ submitRecordLabel1.label = "Verified";
+ submitRecordLabel1.status = SubmitRecord.Label.Status.REJECT;
+ submitRecordLabel1.appliedBy = admin.id;
+
+ SubmitRecord.Label submitRecordLabel2 = new SubmitRecord.Label();
+ submitRecordLabel2.label = "Code-Review";
+ submitRecordLabel2.status = SubmitRecord.Label.Status.OK;
+ submitRecordLabel2.appliedBy = admin.id;
+
+ SubmitRecord.Label submitRecordLabel3 = new SubmitRecord.Label();
+ submitRecordLabel3.label = "Any-Label-Name";
+ submitRecordLabel3.status = SubmitRecord.Label.Status.REJECT;
+ submitRecordLabel3.appliedBy = user.id;
+
+ List<Term> terms = new ArrayList<>();
+
+ StructureTerm label1 = makeLabel(submitRecordLabel1.label, "reject", admin);
+
+ StructureTerm label2 = makeLabel(submitRecordLabel2.label, "ok", admin);
+
+ StructureTerm label3 = makeLabel(submitRecordLabel3.label, "reject", user);
+
+ terms.add(makeTerm("not_ready", makeLabels(label1)));
+ terms.add(makeTerm("ok", makeLabels(label2)));
+ terms.add(makeTerm("not_ready", makeLabels(label3)));
+
+ // When
+ List<SubmitRecord> records = evaluator.resultsToSubmitRecord(null, terms);
+
+ // assert that
+ SubmitRecord record1Expected = new SubmitRecord();
+ record1Expected.status = SubmitRecord.Status.OK;
+ record1Expected.labels = new ArrayList<>();
+ record1Expected.labels.add(submitRecordLabel2);
+
+ SubmitRecord record2Expected = new SubmitRecord();
+ record2Expected.status = SubmitRecord.Status.OK;
+ record2Expected.labels = new ArrayList<>();
+ record2Expected.labels.add(submitRecordLabel3);
+
+ assertThat(records).hasSize(2);
+
+ assertThat(records.get(0)).isEqualTo(record1Expected);
+ assertThat(records.get(1)).isEqualTo(record2Expected);
+ }
+
+ private static Term makeTerm(String status, StructureTerm labels) {
+ return new StructureTerm(status, labels);
+ }
+
+ private static StructureTerm makeLabel(String name, String status) {
+ return new StructureTerm("label", new StructureTerm(name), new StructureTerm(status));
+ }
+
+ private static StructureTerm makeLabel(String name, String status, TestAccount account) {
+ StructureTerm user = new StructureTerm("user", new IntegerTerm(account.id.get()));
+ return new StructureTerm("label", new StructureTerm(name), new StructureTerm(status, user));
+ }
+
+ private static StructureTerm makeLabels(StructureTerm... labels) {
+ return new StructureTerm("label", labels);
+ }
+
+ private ChangeData makeChangeData() {
+ ChangeData cd = ChangeData.createForTest(project, new Change.Id(1), 1);
+ cd.setChange(TestChanges.newChange(project, admin.id));
+ return cd;
+ }
+
+ private PrologRuleEvaluator makeEvaluator() {
+ return evaluatorFactory.create(makeChangeData(), SubmitRuleOptions.defaults());
+ }
+}
diff --git a/javatests/com/google/gerrit/common/data/SubmitRecordTest.java b/javatests/com/google/gerrit/common/data/SubmitRecordTest.java
new file mode 100644
index 0000000..6a90f45
--- /dev/null
+++ b/javatests/com/google/gerrit/common/data/SubmitRecordTest.java
@@ -0,0 +1,59 @@
+// Copyright (C) 2018 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.common.data;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import org.junit.Test;
+
+public class SubmitRecordTest {
+
+ private static SubmitRecord OK_RECORD;
+ private static SubmitRecord NOT_READY_RECORD;
+
+ static {
+ OK_RECORD = new SubmitRecord();
+ OK_RECORD.status = SubmitRecord.Status.OK;
+
+ NOT_READY_RECORD = new SubmitRecord();
+ NOT_READY_RECORD.status = SubmitRecord.Status.NOT_READY;
+ }
+
+ @Test
+ public void okIfAllOkay() {
+ Collection<SubmitRecord> submitRecords = new ArrayList<>();
+ submitRecords.add(OK_RECORD);
+
+ assertThat(SubmitRecord.allRecordsOK(submitRecords)).isTrue();
+ }
+
+ @Test
+ public void okWhenEmpty() {
+ Collection<SubmitRecord> submitRecords = new ArrayList<>();
+
+ assertThat(SubmitRecord.allRecordsOK(submitRecords)).isTrue();
+ }
+
+ @Test
+ public void emptyResultIfInvalid() {
+ Collection<SubmitRecord> submitRecords = new ArrayList<>();
+ submitRecords.add(NOT_READY_RECORD);
+ submitRecords.add(OK_RECORD);
+
+ assertThat(SubmitRecord.allRecordsOK(submitRecords)).isFalse();
+ }
+}
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
index 031284d..95ac3e3 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
+++ b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
@@ -23,6 +23,7 @@
import com.google.common.collect.Table;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitRequirement;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.ReviewerSet;
@@ -30,6 +31,7 @@
import com.google.gerrit.testing.GerritBaseTests;
import com.google.gerrit.testing.TestTimeUtil;
import java.sql.Timestamp;
+import java.util.Collections;
import java.util.List;
import java.util.concurrent.TimeUnit;
import org.junit.After;
@@ -80,11 +82,32 @@
@Test
public void storedSubmitRecords() {
assertStoredRecordRoundTrip(record(SubmitRecord.Status.CLOSED));
- assertStoredRecordRoundTrip(
+
+ SubmitRecord r =
record(
SubmitRecord.Status.OK,
label(SubmitRecord.Label.Status.MAY, "Label-1", null),
- label(SubmitRecord.Label.Status.OK, "Label-2", 1)));
+ label(SubmitRecord.Label.Status.OK, "Label-2", 1));
+
+ assertStoredRecordRoundTrip(r);
+ }
+
+ @Test
+ public void storedSubmitRecordsWithRequirements() {
+ SubmitRecord r =
+ record(
+ SubmitRecord.Status.OK,
+ label(SubmitRecord.Label.Status.MAY, "Label-1", null),
+ label(SubmitRecord.Label.Status.OK, "Label-2", 1));
+
+ SubmitRequirement sr =
+ new SubmitRequirement(
+ "short reason",
+ "Full reason can be a long string with special symbols like < > \\ / ; :",
+ null);
+ r.requirements = Collections.singletonList(sr);
+
+ assertStoredRecordRoundTrip(r);
}
private static SubmitRecord record(SubmitRecord.Status status, SubmitRecord.Label... labels) {
diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
index 4f14dda..019338e 100644
--- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
+++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
@@ -62,6 +62,7 @@
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.index.account.AccountField;
import com.google.gerrit.server.index.account.AccountIndexCollection;
+import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.schema.SchemaCreator;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
@@ -93,6 +94,8 @@
@Inject protected AccountCache accountCache;
+ @Inject protected AccountIndexer accountIndexer;
+
@Inject protected AccountManager accountManager;
@Inject protected GerritApi gApi;
@@ -672,6 +675,7 @@
accountManager.link(id, AuthRequest.forEmail(email));
}
accountCache.evict(id);
+ accountIndexer.index(id);
}
protected QueryRequest newQuery(Object query) throws RestApiException {
diff --git a/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java b/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java
index a77f807..8dc4244 100644
--- a/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java
+++ b/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.testing.InMemoryTestEnvironment;
import com.google.gerrit.testing.TestUpdateUI;
import com.google.inject.Inject;
@@ -48,6 +49,7 @@
@Rule public InMemoryTestEnvironment testEnv = new InMemoryTestEnvironment();
@Inject private AccountCache accountCache;
+ @Inject private AccountIndexer accountIndexer;
@Inject private AllUsersName allUsersName;
@Inject private GerritApi gApi;
@Inject private GitRepositoryManager repoManager;
@@ -95,6 +97,7 @@
schema160.migrateData(db, new TestUpdateUI());
accountCache.evict(accountId);
+ accountIndexer.index(accountId);
testEnv.setApiUser(accountId);
assertThat(metaRef(accountId)).isNotEqualTo(oldMetaId);
diff --git a/lib/guava.bzl b/lib/guava.bzl
index 0aa3cf7a..db85900 100644
--- a/lib/guava.bzl
+++ b/lib/guava.bzl
@@ -1,5 +1,5 @@
-GUAVA_VERSION = "23.6-jre"
+GUAVA_VERSION = "24.1-jre"
-GUAVA_BIN_SHA1 = "c0b638df79e7b2e1ed98f8d68ac62538a715ab1d"
+GUAVA_BIN_SHA1 = "96c528475465aeb22cce60605d230a7e67cebd7b"
GUAVA_DOC_URL = "https://google.github.io/guava/releases/" + GUAVA_VERSION + "/api/docs/"
diff --git a/lib/jgit/jgit.bzl b/lib/jgit/jgit.bzl
index 7bee9e34..cf389850 100644
--- a/lib/jgit/jgit.bzl
+++ b/lib/jgit/jgit.bzl
@@ -1,8 +1,8 @@
load("//tools/bzl:maven_jar.bzl", "GERRIT", "MAVEN_LOCAL", "MAVEN_CENTRAL", "maven_jar")
-_JGIT_VERS = "4.10.0.201712302008-r.24-gf3bb0e268"
+_JGIT_VERS = "4.11.0.201803080745-r.2-g61e4f1665"
-_DOC_VERS = "4.10.0.201712302008-r" # Set to _JGIT_VERS unless using a snapshot
+_DOC_VERS = "4.11.0.201803080745-r" # Set to _JGIT_VERS unless using a snapshot
JGIT_DOC_URL = "http://download.eclipse.org/jgit/site/" + _DOC_VERS + "/apidocs"
@@ -26,28 +26,28 @@
name = "jgit_lib",
artifact = "org.eclipse.jgit:org.eclipse.jgit:" + _JGIT_VERS,
repository = _JGIT_REPO,
- sha1 = "1813447ae544b38e36bda9f1599df5f7cff6cfac",
- src_sha1 = "f0bb1ac954ff529b4e1e01e65ceb0d46d949da9e",
+ sha1 = "38489eca0a4308087081d07774af86aa6a50b2ab",
+ src_sha1 = "e43c58829c72b5b18e16d1b2bbd1396ddd93098f",
unsign = True,
)
maven_jar(
name = "jgit_servlet",
artifact = "org.eclipse.jgit:org.eclipse.jgit.http.server:" + _JGIT_VERS,
repository = _JGIT_REPO,
- sha1 = "f7a88c3744f864587d50096ab99a58e09e4afd95",
+ sha1 = "f5be45e4f97f0bf0825e4ff8fcb2f47588dd7e92",
unsign = True,
)
maven_jar(
name = "jgit_archive",
artifact = "org.eclipse.jgit:org.eclipse.jgit.archive:" + _JGIT_VERS,
repository = _JGIT_REPO,
- sha1 = "af579bcc932fa26f3c4d4ae00e812dc50d50a355",
+ sha1 = "f202f169b2e3a50be90b4123baa941136eda3ed6",
)
maven_jar(
name = "jgit_junit",
artifact = "org.eclipse.jgit:org.eclipse.jgit.junit:" + _JGIT_VERS,
repository = _JGIT_REPO,
- sha1 = "b5fc2330bf0418e3a0f773925c57497131f13380",
+ sha1 = "dd9f7e4cc41b4f47591ce51c4752ccfef012c553",
unsign = True,
)
diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor
index 32753c4..fa9df30 160000
--- a/plugins/codemirror-editor
+++ b/plugins/codemirror-editor
@@ -1 +1 @@
-Subproject commit 32753c4e226fe6297795c7d80166203719d6e48f
+Subproject commit fa9df3035c306069758712bfe9ae3425b119bb0c
diff --git a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.html b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.html
index 8bceff1..9f5df11 100644
--- a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.html
+++ b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.html
@@ -24,7 +24,6 @@
<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
<link rel="import" href="../../shared/gr-autocomplete/gr-autocomplete.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
-<link rel="import" href="../../shared/gr-confirm-dialog/gr-confirm-dialog.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../shared/gr-select/gr-select.html">
@@ -102,17 +101,7 @@
<input
type="checkbox"
id="privateChangeCheckBox"
- checked$="[[_repoConfig.private_by_default.inherited_value]]">
- </span>
- </section>
- <section>
- <label
- class="title"
- for="wipChangeCheckBox">WIP Change</label>
- <span class="value">
- <input
- type="checkbox"
- id="wipChangeCheckBox">
+ checked$="[[_formatBooleanString(_repoConfig.private_by_default)]]">
</span>
</section>
</div>
diff --git a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.js b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.js
index eace34a..367d324 100644
--- a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.js
+++ b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.js
@@ -61,13 +61,11 @@
handleCreateChange() {
const isPrivate = this.$.privateChangeCheckBox.checked;
- const isWip = this.$.wipChangeCheckBox.checked;
+ const isWip = true;
return this.$.restAPI.createChange(this.repoName, this.branch,
this.subject, this.topic, isPrivate, isWip)
.then(changeCreated => {
- if (!changeCreated) {
- return;
- }
+ if (!changeCreated) { return; }
Gerrit.Nav.navigateToChange(changeCreated);
});
},
@@ -94,5 +92,21 @@
return branches;
});
},
+
+ _formatBooleanString(config) {
+ if (config && config.configured_value === 'TRUE') {
+ return true;
+ } else if (config && config.configured_value === 'FALSE') {
+ return false;
+ } else if (config && config.configured_value === 'INHERIT') {
+ if (config && config.inherited_value) {
+ return true;
+ } else {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog_test.html b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog_test.html
index afe3801..4a51d8f 100644
--- a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog_test.html
+++ b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog_test.html
@@ -55,9 +55,12 @@
},
});
element = fixture('basic');
- element.repoName = 'test-repo';
+ element.repoName = 'test-repo',
element._repoConfig = {
- private_by_default: {},
+ private_by_default: {
+ configured_value: 'FALSE',
+ inherited_value: false,
+ },
};
});
@@ -65,18 +68,57 @@
sandbox.restore();
});
- test('new change created with private', () => {
+ test('new change created with default', done => {
+ const configInputObj = {
+ branch: 'test-branch',
+ subject: 'first change created with polygerrit ui',
+ topic: 'test-topic',
+ is_private: false,
+ work_in_progress: true,
+ };
+
+ const saveStub = sandbox.stub(element.$.restAPI,
+ 'createChange', () => {
+ return Promise.resolve({});
+ });
+
+ element.branch = 'test-branch';
+ element.topic = 'test-topic';
+ element.subject = 'first change created with polygerrit ui';
+ assert.isFalse(element.$.privateChangeCheckBox.checked);
+
+ element.$.branchInput.bindValue = configInputObj.branch;
+ element.$.tagNameInput.bindValue = configInputObj.topic;
+ element.$.messageInput.bindValue = configInputObj.subject;
+
+ element.handleCreateChange().then(() => {
+ // Private change
+ assert.isFalse(saveStub.lastCall.args[4]);
+ // WIP Change
+ assert.isTrue(saveStub.lastCall.args[5]);
+ assert.isTrue(saveStub.called);
+ done();
+ });
+ });
+
+ test('new change created with private', done => {
element._repoConfig = {
private_by_default: {
- inherited_value: true,
+ configured_value: 'TRUE',
+ inherited_value: false,
},
};
+ sandbox.stub(element, '_formatBooleanString', () => {
+ return Promise.resolve(true);
+ });
+ flushAsynchronousOperations();
const configInputObj = {
branch: 'test-branch',
- topic: 'test-topic',
subject: 'first change created with polygerrit ui',
- work_in_progress: false,
+ topic: 'test-topic',
+ is_private: true,
+ work_in_progress: true,
};
const saveStub = sandbox.stub(element.$.restAPI,
@@ -94,34 +136,12 @@
element.$.messageInput.bindValue = configInputObj.subject;
element.handleCreateChange().then(() => {
- assert.isTrue(saveStub.lastCall.calledWithExactly(configInputObj));
- });
- });
-
- test('new change created with wip', () => {
- const configInputObj = {
- branch: 'test-branch',
- topic: 'test-topic',
- subject: 'first change created with polygerrit ui',
- };
-
- const saveStub = sandbox.stub(element.$.restAPI,
- 'createChange', () => {
- return Promise.resolve({});
- });
-
- element.branch = 'test-branch';
- element.topic = 'test-topic';
- element.subject = 'first change created with polygerrit ui';
- element.$.wipChangeCheckBox.checked = true;
- assert.isFalse(element.$.privateChangeCheckBox.checked);
-
- element.$.branchInput.bindValue = configInputObj.branch;
- element.$.tagNameInput.bindValue = configInputObj.topic;
- element.$.messageInput.bindValue = configInputObj.subject;
-
- element.handleCreateChange().then(() => {
- assert.isTrue(saveStub.lastCall.calledWithExactly(configInputObj));
+ // Private change
+ assert.isTrue(saveStub.lastCall.args[4]);
+ // WIP Change
+ assert.isTrue(saveStub.lastCall.args[5]);
+ assert.isTrue(saveStub.called);
+ done();
});
});
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html
index c887367..5cca8b8 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html
+++ b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html
@@ -78,7 +78,7 @@
</template>
</div>
<gr-button id="editBtn"
- class$="[[_computeShowEditClass(_sections)]]"
+ class="visible"
on-tap="_handleEdit">[[_editOrCancel(_editing)]]</gr-button>
<gr-button id="saveBtn"
primary
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js
index 2bebb96..d0af0f1 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js
+++ b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js
@@ -316,11 +316,6 @@
});
},
- _computeShowEditClass(sections) {
- if (!sections.length) { return ''; }
- return 'visible';
- },
-
_computeShowSaveClass(editing) {
if (!editing) { return ''; }
return 'visible';
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
index 46ff469..c4a7f63 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
@@ -85,7 +85,9 @@
display: none;
}
.size gr-tooltip-content {
+ margin: -.4rem -.6rem;
max-width: 2.5rem;
+ padding: .4rem .6rem;
}
a {
color: var(--default-text-color);
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 1fa3c7f9..8073983 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -30,12 +30,14 @@
<link rel="import" href="../../shared/gr-button/gr-button.html">
<link rel="import" href="../../shared/gr-change-star/gr-change-star.html">
<link rel="import" href="../../shared/gr-change-status/gr-change-status.html">
+<link rel="import" href="../../shared/gr-count-string-formatter/gr-count-string-formatter.html">
<link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html">
<link rel="import" href="../../shared/gr-editable-content/gr-editable-content.html">
<link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
<link rel="import" href="../../shared/gr-linked-text/gr-linked-text.html">
<link rel="import" href="../../shared/gr-overlay/gr-overlay.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
+<link rel="import" href="../../shared/gr-tooltip-content/gr-tooltip-content.html">
<link rel="import" href="../gr-change-actions/gr-change-actions.html">
<link rel="import" href="../gr-change-metadata/gr-change-metadata.html">
<link rel="import" href="../gr-commit-info/gr-commit-info.html">
@@ -109,11 +111,6 @@
.changeStatus {
text-transform: capitalize;
}
- .container {
- /* Needed to restrict the change view from allowing horizontal
- scrolling */
- width: 100vw;
- }
/* Strong specificity here is needed due to
https://github.com/Polymer/polymer/issues/2531 */
.container section.changeInfo {
@@ -556,7 +553,13 @@
id="commentTabs"
on-selected-changed="_handleTabChange">
<paper-tab class="changeLog">Change Log</paper-tab>
- <paper-tab class="commentThreads">Comment Threads</paper-tab>
+ <paper-tab
+ class="commentThreads">
+ <gr-tooltip-content
+ has-tooltip
+ title$="[[_computeTotalCommentCounts(_change.unresolved_comment_count, _changeComments)]]">
+ <span>Comment Threads</span></gr-tooltip-content>
+ </paper-tab>
</paper-tabs>
<gr-messages-list id="messageList"
class$="hideOnMobileOverlay [[_computeShowMessages(_showMessagesView)]]"
@@ -574,6 +577,7 @@
change="[[_change]]"
change-num="[[_changeNum]]"
class$="[[_computeShowThreads(_showMessagesView)]]"
+ logged-in="[[_loggedIn]]"
on-thread-list-modified="_handleReloadDiffComments"></gr-thread-list>
</div>
<gr-overlay id="downloadOverlay" with-backdrop>
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index a2e9290..e552dd1 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -96,6 +96,7 @@
type: Object,
value() { return document.body; },
},
+ _commentThreads: Array,
/** @type {?} */
_serverConfig: {
type: Object,
@@ -437,6 +438,19 @@
});
},
+ _computeTotalCommentCounts(unresolvedCount, changeComments) {
+ const draftCount = changeComments.computeDraftCount();
+ const unresolvedString = GrCountStringFormatter.computeString(
+ unresolvedCount, 'unresolved');
+ const draftString = GrCountStringFormatter.computePluralString(
+ draftCount, 'draft');
+
+ return unresolvedString +
+ // Add a comma and space if both unresolved and draft comments exist.
+ (unresolvedString && draftString ? ', ' : '') +
+ draftString;
+ },
+
_handleCommentSave(e) {
if (!e.target.comment.__draft) { return; }
@@ -595,6 +609,13 @@
},
_paramsChanged(value) {
+ // Change the content of the comment tabs back to messages list, but
+ // do not yet change the tab itself. The animation of tab switching will
+ // get messed up if changed here, because it requires the tabs to be on
+ // the streen, and they are hidden shortly after this. The tab switching
+ // animation will happen in post render tasks.
+ this._showMessagesView = true;
+
if (value.view !== Gerrit.Nav.View.CHANGE) {
this._initialLoadComplete = false;
return;
@@ -630,15 +651,6 @@
this._changeNum = value.changeNum;
this.$.relatedChanges.clear();
- // If the comment tabs were already rendered, but set to the wrong initial
- // value, swap them here so the thread tab doesn't flash before being
- // swapped out. If the selected tab is undefined, we have to wait until
- // the page is finished rendering to set selected to 0, otherwise the
- // animation will not show.
- if (this.$.commentTabs.selected === 1) {
- this.$.commentTabs.selected = 0;
- }
-
this._reload().then(() => {
this._performPostLoadTasks();
});
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index e514143..2101856 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -332,6 +332,30 @@
});
});
+ test('thread list modified', () => {
+ return element._reloadComments().then(() => {
+ let draftStub = sinon.stub(element._changeComments, 'computeDraftCount')
+ .returns(1);
+ assert.equal(element._computeTotalCommentCounts(5,
+ element._changeComments), '5 unresolved, 1 draft');
+ assert.equal(element._computeTotalCommentCounts(0,
+ element._changeComments), '1 draft');
+ draftStub.restore();
+ draftStub = sinon.stub(element._changeComments, 'computeDraftCount')
+ .returns(0);
+ assert.equal(element._computeTotalCommentCounts(0,
+ element._changeComments), '');
+ assert.equal(element._computeTotalCommentCounts(1,
+ element._changeComments), '1 unresolved');
+ draftStub.restore();
+ draftStub = sinon.stub(element._changeComments, 'computeDraftCount')
+ .returns(2);
+ assert.equal(element._computeTotalCommentCounts(1,
+ element._changeComments), '1 unresolved, 2 drafts');
+ draftStub.restore();
+ });
+ });
+
test('thread list and change log tabs', done => {
element._changeNum = '1';
element._patchRange = {
@@ -382,10 +406,15 @@
assert.notEqual(getComputedStyle(element.$.threadList).display,
'none');
- // When the change is partially reloaded (ex: Shift+R), paramsChanged
- // will set the tab, if it was previously set to the thread tab.
+ // When the change is partially reloaded (ex: Shift+R), the content
+ // is swapped out before the tab, so messages list will display even
+ // though the tab for comment threads is still temporarily selected.
element._paramsChanged(element.params);
- assert.equal(element.$.commentTabs.selected, 0);
+ assert.equal(element.$.commentTabs.selected, 1);
+ assert.notEqual(getComputedStyle(element.$.messageList).display,
+ 'none');
+ assert.equal(getComputedStyle(element.$.threadList).display,
+ 'none');
done();
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.html b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.html
index ea70de9..55cea07 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.html
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.html
@@ -104,6 +104,7 @@
<gr-autocomplete
id="parentInput"
query="[[_query]]"
+ no-debounce
text="{{_inputText}}"
on-tap="_handleEnterChangeNumberTap"
on-commit="_handleBaseSelected"
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
index ccfc368..7a43732 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
@@ -165,6 +165,7 @@
test('input text change triggers function', () => {
sandbox.spy(element, '_getRecentChanges');
+ element.$.parentInput.noDebounce = true;
element._inputText = '1';
assert.isTrue(element._getRecentChanges.calledOnce);
element._inputText = '12';
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
index fb7b89c..93f010a 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
@@ -149,7 +149,7 @@
.then(res => {
if (res.ok) {
if (target) { target.disabled = false; }
- this.set(['_change', 'revisions', sha, 'description'], desc);
+ this.set(['change', 'revisions', sha, 'description'], desc);
this._patchsetDescription = desc;
}
}).catch(err => {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
index a92d783..f7056dc 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html
@@ -134,6 +134,7 @@
// The API stub should be called with an empty string for the new
// description.
assert.equal(putDescStub.lastCall.args[2], '');
+ assert.equal(element.change.revisions.rev1.description, '');
flushAsynchronousOperations();
// The editable label should now be visible and the chip hidden.
@@ -154,6 +155,7 @@
}).then(() => {
flushAsynchronousOperations();
// The chip should be visible again, and the label hidden.
+ assert.equal(element.change.revisions.rev1.description, 'test2');
assert.equal(getComputedStyle(label).display, 'none');
assert.notEqual(getComputedStyle(chip).display, 'none');
});
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html
index 355b90a..b94b364 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.html
@@ -42,6 +42,12 @@
min-height: 3.2em;
padding: .5em var(--default-horizontal-margin);
}
+ .toggleItem.draftToggle {
+ display: none;
+ }
+ .toggleItem.draftToggle.show {
+ display: flex;
+ }
.toggleItem {
align-items: center;
display: flex;
@@ -62,8 +68,8 @@
<paper-toggle-button
id="unresolvedToggle"
on-change="_toggleUnresolved"></paper-toggle-button>
- Only Unresolved threads</div>
- <div class="toggleItem">
+ Only unresolved threads</div>
+ <div class$="toggleItem draftToggle [[_computeShowDraftToggle(loggedIn)]]">
<paper-toggle-button
id="draftToggle"
on-change="_toggleDrafts"></paper-toggle-button>
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js
index 1ca667d..177bc9b 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js
@@ -24,22 +24,28 @@
is: 'gr-thread-list',
properties: {
+ /** @type {?} */
change: Object,
threads: Array,
changeNum: String,
+ loggedIn: Boolean,
_sortedThreads: {
type: Array,
computed: '_computeSortedThreads(threads.*)',
},
},
+ _computeShowDraftToggle(loggedIn) {
+ return loggedIn ? 'show' : '';
+ },
+
/**
* Order as follows:
* - Unresolved threads with drafts (reverse chronological)
* - Unresolved threads without drafts (reverse chronological)
* - Resolved threads with drafts (reverse chronological)
* - Resolved threads without drafts (reverse chronological)
- * @param {!Array} threads
+ * @param {!Object} changeRecord
* @return {!Array}
*/
_computeSortedThreads(changeRecord) {
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html
index c0be7d7..47c5b9c 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_test.html
@@ -186,6 +186,14 @@
sandbox.restore();
});
+ test('draft toggle only appears when logged in', () => {
+ assert.equal(getComputedStyle(element.$$('.draftToggle')).display,
+ 'none');
+ element.loggedIn = true;
+ assert.notEqual(getComputedStyle(element.$$('.draftToggle')).display,
+ 'none');
+ });
+
test('there are five threads by default', () => {
assert.equal(computeVisibleNumber(threadElements), 5);
});
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
index 94e3e4a19..d5b394c 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
@@ -47,7 +47,6 @@
id="searchInput"
text="{{_inputVal}}"
query="[[query]]"
- debounce-wait="200"
on-commit="_handleInputCommit"
allow-non-suggested-values
multi
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js
index 67b4f51..cf66e57 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js
@@ -178,7 +178,7 @@
for (const path of Object.keys(paths)) {
let commentsToAdd = this.getAllCommentsForPath(path, opt_patchNum);
if (opt_includeDrafts) {
- const drafts = this.getAllDraftsForPath(path)
+ const drafts = this.getAllDraftsForPath(path, opt_patchNum)
.map(d => Object.assign({__draft: true}, d));
commentsToAdd = commentsToAdd.concat(drafts);
}
@@ -207,6 +207,7 @@
*
* @param {!string} path
* @param {number=} opt_patchNum
+ * @param {boolean=} opt_includeDrafts
* @return {!Array}
*/
ChangeComments.prototype.getAllCommentsForPath = function(path,
@@ -323,18 +324,20 @@
};
/**
- * Computes a string counting the number of commens in a given file and path.
+ * Computes a string counting the number of draft comments in the entire
+ * change, optionally filtered by path and/or patchNum.
*
- * @param {number} patchNum
+ * @param {number=} opt_patchNum
* @param {string=} opt_path
* @return {number}
*/
- ChangeComments.prototype.computeDraftCount = function(patchNum, opt_path) {
+ ChangeComments.prototype.computeDraftCount = function(opt_patchNum,
+ opt_path) {
if (opt_path) {
- return this.getAllDraftsForPath(opt_path, patchNum).length;
+ return this.getAllDraftsForPath(opt_path, opt_patchNum).length;
}
- const allComments = this.getAllDrafts(patchNum);
- return this._commentObjToArray(allComments).length;
+ const allDrafts = this.getAllDrafts(opt_patchNum);
+ return this._commentObjToArray(allDrafts).length;
};
/**
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html
index 4cf79fa..2b06a10 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html
@@ -248,6 +248,14 @@
updated: makeTime(3),
},
],
+ 'file/two': [
+ {
+ id: 5,
+ patch_set: 3,
+ line: 1,
+ updated: makeTime(3),
+ },
+ ],
};
element._changeComments._robotComments = {
'file/one': [
@@ -340,13 +348,13 @@
comments = element._changeComments.getCommentsBySideForPath(path,
patchRange);
assert.equal(comments.left.length, 0);
- assert.equal(comments.right.length, 1);
+ assert.equal(comments.right.length, 2);
patchRange.basePatchNum = 2;
comments = element._changeComments.getCommentsBySideForPath(path,
patchRange);
assert.equal(comments.left.length, 1);
- assert.equal(comments.right.length, 1);
+ assert.equal(comments.right.length, 2);
patchRange.basePatchNum = PARENT;
path = 'file/three';
@@ -429,6 +437,8 @@
.computeDraftCount(1, 'file/one'), 0);
assert.equal(element._changeComments
.computeDraftCount(2, 'file/three'), 0);
+ assert.equal(element._changeComments
+ .computeDraftCount(), 3);
});
test('getAllPublishedComments', () => {
@@ -456,7 +466,7 @@
comments = element._changeComments.getAllComments(true);
assert.equal(Object.keys(comments).length, 4);
assert.equal(Object.keys(comments[['file/one']]).length, 6);
- assert.equal(Object.keys(comments[['file/two']]).length, 2);
+ assert.equal(Object.keys(comments[['file/two']]).length, 3);
comments = element._changeComments.getAllComments(true, 2);
assert.equal(Object.keys(comments).length, 4);
assert.equal(Object.keys(comments[['file/one']]).length, 6);
@@ -627,6 +637,21 @@
}, {
comments: [
{
+ id: 5,
+ patch_set: 3,
+ line: 1,
+ __path: 'file/two',
+ __draft: true,
+ updated: '2013-02-26 15:03:43.986000000',
+ },
+ ],
+ rootId: 5,
+ patchNum: 3,
+ path: 'file/two',
+ line: 1,
+ }, {
+ comments: [
+ {
id: 11,
patch_set: 2,
side: 'PARENT',
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
index e1ebbf2..44d703c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
@@ -52,9 +52,6 @@
:host([disabled]) .date {
opacity: .5;
}
- :host([is-robot-comment]) {
- background-color: #cfe8fc;
- }
.header {
align-items: baseline;
cursor: pointer;
@@ -101,6 +98,16 @@
margin-left: 1em;
--gr-button-color: #212121;
}
+ .robotActions {
+ display: flex;
+ justify-content: flex-start;
+ padding-top: 0;
+ }
+ .robotActions .action {
+ /* Keep button text lined up with output text */
+ margin-left: -.3rem;
+ margin-right: 1em;
+ }
.rightActions {
display: flex;
justify-content: flex-end;
@@ -149,7 +156,7 @@
margin-top: -.4em;
}
.runIdInformation {
- margin: 1em 0;
+ margin: .7em 0;
}
.robotRun {
margin-left: .5em;
@@ -273,17 +280,24 @@
disabled="{{disabled}}"
rows="4"
text="{{_messageText}}"></gr-textarea>
+ <!--The message class is needed to ensure selectability from
+ gr-diff-selection.-->
<gr-formatted-text class="message"
content="[[comment.message]]"
no-trailing-margin="[[!comment.__draft]]"
collapsed="[[collapsed]]"
config="[[projectConfig.commentlinks]]"></gr-formatted-text>
- <div hidden$="[[!comment.robot_run_id]]">
+ <div hidden$="[[!comment.robot_run_id]]" class="message">
<div class="runIdInformation" hidden$="[[collapsed]]">
Run ID:
- <a class="robotRunLink" href$="[[comment.url]]">
- <span class="robotRun">[[comment.robot_run_id]]</span>
- </a>
+ <template is="dom-if" if="[[comment.url]]">
+ <a class="robotRunLink" href$="[[comment.url]]">
+ <span class="robotRun link">[[comment.robot_run_id]]</span>
+ </a>
+ </template>
+ <template is="dom-if" if="[[!comment.url]]">
+ <span class="robotRun text">[[comment.robot_run_id]]</span>
+ </template>
</div>
</div>
<div class="actions humanActions" hidden$="[[!_showHumanActions]]">
@@ -309,17 +323,17 @@
</gr-button>
</div>
</div>
- <div class="actions robotActions" hidden$="[[!_showRobotActions]]">
+ <div class="robotActions" hidden$="[[!_showRobotActions]]">
<template is="dom-if" if="[[isRobotComment]]">
- <gr-endpoint-decorator name="robot-comment-controls">
- <gr-endpoint-param name="comment" value="[[comment]]">
- </gr-endpoint-param>
- </gr-endpoint-decorator>
<gr-button link class="action fix"
on-tap="_handleFix"
disabled="[[robotButtonDisabled]]">
Please Fix
</gr-button>
+ <gr-endpoint-decorator name="robot-comment-controls">
+ <gr-endpoint-param name="comment" value="[[comment]]">
+ </gr-endpoint-param>
+ </gr-endpoint-decorator>
</template>
</div>
</div>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
index 8c78946..01f5ae7 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -385,6 +385,23 @@
element.draft = false;
assert.isTrue(element.$$('.humanActions').hasAttribute('hidden'));
assert.isFalse(element.$$('.robotActions').hasAttribute('hidden'));
+
+ // A robot comment with run ID should display plain text.
+ element.set(['comment', 'robot_run_id'], 'text');
+ element.editing = false;
+ element.collapsed = false;
+ flushAsynchronousOperations();
+ assert.isNotOk(element.$$('.robotRun.link'));
+ assert.notEqual(getComputedStyle(element.$$('.robotRun.text')).display,
+ 'none');
+
+ // A robot comment with run ID and url should display a link.
+ element.set(['comment', 'url'], '/path/to/run');
+ flushAsynchronousOperations();
+ assert.notEqual(getComputedStyle(element.$$('.robotRun.link')).display,
+ 'none');
+ assert.equal(getComputedStyle(element.$$('.robotRun.text')).display,
+ 'none');
});
test('collapsible drafts', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
index 3bf8df7..22433fc 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -25,6 +25,7 @@
<link rel="import" href="../../shared/gr-button/gr-button.html">
<link rel="import" href="../../shared/gr-count-string-formatter/gr-count-string-formatter.html">
<link rel="import" href="../../shared/gr-dropdown-list/gr-dropdown-list.html">
+<link rel="import" href="../../shared/gr-fixed-panel/gr-fixed-panel.html">
<link rel="import" href="../../shared/gr-icons/gr-icons.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../shared/gr-select/gr-select.html">
@@ -54,6 +55,11 @@
border-bottom: 1px solid #eee;
}
}
+ gr-fixed-panel {
+ background-color: #fff;
+ border-bottom: 1px #eee solid;
+ z-index: 1;
+ }
header,
.subHeader {
align-items: center;
@@ -141,15 +147,6 @@
.separator.hide {
display: none;
}
- .sticky {
- background: #fff;
- border-bottom: 1px solid #eee;
- left: 0;
- position: sticky;
- top: 0;
- width: 100vw;
- z-index: 1;
- }
gr-dropdown-list {
--trigger-style: {
text-transform: none;
@@ -208,136 +205,138 @@
}
}
</style>
- <div>
- <div class="sticky">
- <header class$="[[_computeContainerClass(_editMode)]]">
- <h3>
- <a href$="[[_computeChangePath(_change, _patchRange.*, _change.revisions)]]">
- [[_changeNum]]</a><span>:</span>
- <span>[[_change.subject]]</span>
- <span class="dash">—</span>
- <input id="reviewed"
- class="reviewed hideOnEdit"
- type="checkbox"
- on-change="_handleReviewedChange"
- hidden$="[[!_loggedIn]]" hidden>
- <div class="jumpToFileContainer">
- <gr-dropdown-list
- id="dropdown"
- value="[[_path]]"
- on-value-change="_handleFileChange"
- items="[[_formattedFiles]]"
- initial-count="75">
- </gr-dropdown-list>
- </div>
- </h3>
- <div class="navLinks desktop">
- <span class$="fileNum [[_computeFileNumClass(_fileNum, _formattedFiles)]]">
- File [[_fileNum]] of [[_formattedFiles.length]]
- <span class="separator"></span>
- </span>
- <a class="navLink"
- href$="[[_computeNavLinkURL(_change, _path, _fileList, -1, 1)]]">
- Prev</a>
- <span class="separator"></span>
- <a class="navLink"
- href$="[[_computeChangePath(_change, _patchRange.*, _change.revisions)]]">
- Up</a>
- <span class="separator"></span>
- <a class="navLink"
- href$="[[_computeNavLinkURL(_change, _path, _fileList, 1, 1)]]">
- Next</a>
+ <gr-fixed-panel
+ class$="[[_computeContainerClass(_editMode)]]"
+ floating-disabled="[[_panelFloatingDisabled]]"
+ keep-on-scroll
+ ready-for-measure="[[!_loading]]">
+ <header>
+ <h3>
+ <a href$="[[_computeChangePath(_change, _patchRange.*, _change.revisions)]]">
+ [[_changeNum]]</a><span>:</span>
+ <span>[[_change.subject]]</span>
+ <span class="dash">—</span>
+ <input id="reviewed"
+ class="reviewed hideOnEdit"
+ type="checkbox"
+ on-change="_handleReviewedChange"
+ hidden$="[[!_loggedIn]]" hidden>
+ <div class="jumpToFileContainer">
+ <gr-dropdown-list
+ id="dropdown"
+ value="[[_path]]"
+ on-value-change="_handleFileChange"
+ items="[[_formattedFiles]]"
+ initial-count="75">
+ </gr-dropdown-list>
</div>
- </header>
- <div class="subHeader">
- <div class="patchRangeLeft">
- <gr-patch-range-select
- id="rangeSelect"
- change-num="[[_changeNum]]"
- change-comments="[[_changeComments]]"
- patch-num="[[_patchRange.patchNum]]"
- base-patch-num="[[_patchRange.basePatchNum]]"
- files-weblinks="[[_filesWeblinks]]"
- available-patches="[[_allPatchSets]]"
- revisions="[[_change.revisions]]"
- revision-info="[[_revisionInfo]]"
- on-patch-range-change="_handlePatchChange">
- </gr-patch-range-select>
- <span class="download desktop">
- <span class="separator"></span>
- <a
- class="downloadLink"
- download
- href$="[[_computeDownloadLink(_changeNum, _patchRange, _path)]]">
- Download
- </a>
- </span>
+ </h3>
+ <div class="navLinks desktop">
+ <span class$="fileNum [[_computeFileNumClass(_fileNum, _formattedFiles)]]">
+ File [[_fileNum]] of [[_formattedFiles.length]]
+ <span class="separator"></span>
+ </span>
+ <a class="navLink"
+ href$="[[_computeNavLinkURL(_change, _path, _fileList, -1, 1)]]">
+ Prev</a>
+ <span class="separator"></span>
+ <a class="navLink"
+ href$="[[_computeChangePath(_change, _patchRange.*, _change.revisions)]]">
+ Up</a>
+ <span class="separator"></span>
+ <a class="navLink"
+ href$="[[_computeNavLinkURL(_change, _path, _fileList, 1, 1)]]">
+ Next</a>
+ </div>
+ </header>
+ <div class="subHeader">
+ <div class="patchRangeLeft">
+ <gr-patch-range-select
+ id="rangeSelect"
+ change-num="[[_changeNum]]"
+ change-comments="[[_changeComments]]"
+ patch-num="[[_patchRange.patchNum]]"
+ base-patch-num="[[_patchRange.basePatchNum]]"
+ files-weblinks="[[_filesWeblinks]]"
+ available-patches="[[_allPatchSets]]"
+ revisions="[[_change.revisions]]"
+ revision-info="[[_revisionInfo]]"
+ on-patch-range-change="_handlePatchChange">
+ </gr-patch-range-select>
+ <span class="download desktop">
+ <span class="separator"></span>
+ <a
+ class="downloadLink"
+ download
+ href$="[[_computeDownloadLink(_changeNum, _patchRange, _path)]]">
+ Download
+ </a>
+ </span>
+ </div>
+ <div class="rightControls">
+ <span class$="blameLoader [[_computeBlameLoaderClass(_isImageDiff, _isBlameSupported)]]">
+ <gr-button
+ link
+ disabled="[[_isBlameLoading]]"
+ on-tap="_toggleBlame">[[_computeBlameToggleLabel(_isBlameLoaded, _isBlameLoading)]]</gr-button>
+ <span class="separator"></span>
+ </span>
+ <div class$="diffModeSelector [[_computeModeSelectHideClass(_isImageDiff)]]">
+ <span>Diff view:</span>
+ <gr-diff-mode-selector
+ id="modeSelect"
+ save-on-change="[[_loggedIn]]"
+ mode="{{changeViewState.diffMode}}"></gr-diff-mode-selector>
</div>
- <div class="rightControls">
- <span class$="blameLoader [[_computeBlameLoaderClass(_isImageDiff, _isBlameSupported)]]">
+ <span id="diffPrefsContainer"
+ hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]" hidden>
+ <span class="preferences desktop">
<gr-button
link
- disabled="[[_isBlameLoading]]"
- on-tap="_toggleBlame">[[_computeBlameToggleLabel(_isBlameLoaded, _isBlameLoading)]]</gr-button>
- <span class="separator"></span>
+ class="prefsButton"
+ has-tooltip
+ title="Diff preferences"
+ on-tap="_handlePrefsTap"><iron-icon icon="gr-icons:settings"></iron-icon></gr-button>
</span>
- <div class$="diffModeSelector [[_computeModeSelectHideClass(_isImageDiff)]]">
- <span>Diff view:</span>
- <gr-diff-mode-selector
- id="modeSelect"
- save-on-change="[[_loggedIn]]"
- mode="{{changeViewState.diffMode}}"></gr-diff-mode-selector>
- </div>
- <span id="diffPrefsContainer"
- hidden$="[[_computePrefsButtonHidden(_prefs, _loggedIn)]]" hidden>
- <span class="preferences desktop">
- <gr-button
- link
- class="prefsButton"
- has-tooltip
- title="Diff preferences"
- on-tap="_handlePrefsTap"><iron-icon icon="gr-icons:settings"></iron-icon></gr-button>
- </span>
+ </span>
+ <gr-endpoint-decorator name="annotation-toggler">
+ <span hidden id="annotation-span">
+ <label for="annotation-checkbox" id="annotation-label"></label>
+ <input is="iron-input" type="checkbox" id="annotation-checkbox" disabled>
</span>
- <gr-endpoint-decorator name="annotation-toggler">
- <span hidden id="annotation-span">
- <label for="annotation-checkbox" id="annotation-label"></label>
- <input is="iron-input" type="checkbox" id="annotation-checkbox" disabled>
- </span>
- </gr-endpoint-decorator>
- </div>
- </div>
- <div class="fileNav mobile">
- <a class="mobileNavLink"
- href$="[[_computeNavLinkURL(_change, _path, _fileList, -1, 1)]]">
- <</a>
- <div class="fullFileName mobile">[[computeDisplayPath(_path)]]
- </div>
- <a class="mobileNavLink"
- href$="[[_computeNavLinkURL(_change, _path, _fileList, 1, 1)]]">
- ></a>
+ </gr-endpoint-decorator>
</div>
</div>
- <div class="loading" hidden$="[[!_loading]]">Loading...</div>
- <gr-diff
- id="diff"
- hidden
- hidden$="[[_loading]]"
- class$="[[_computeDiffClass(_panelFloatingDisabled)]]"
- is-image-diff="{{_isImageDiff}}"
- files-weblinks="{{_filesWeblinks}}"
- change-num="[[_changeNum]]"
- commit-range="[[_commitRange]]"
- patch-range="[[_patchRange]]"
- path="[[_path]]"
- prefs="[[_prefs]]"
- project-config="[[_projectConfig]]"
- project-name="[[_change.project]]"
- view-mode="[[_diffMode]]"
- is-blame-loaded="{{_isBlameLoaded}}"
- on-line-selected="_onLineSelected">
- </gr-diff>
- </div>
+ <div class="fileNav mobile">
+ <a class="mobileNavLink"
+ href$="[[_computeNavLinkURL(_change, _path, _fileList, -1, 1)]]">
+ <</a>
+ <div class="fullFileName mobile">[[computeDisplayPath(_path)]]
+ </div>
+ <a class="mobileNavLink"
+ href$="[[_computeNavLinkURL(_change, _path, _fileList, 1, 1)]]">
+ ></a>
+ </div>
+ </gr-fixed-panel>
+ <div class="loading" hidden$="[[!_loading]]">Loading...</div>
+ <gr-diff
+ id="diff"
+ hidden
+ hidden$="[[_loading]]"
+ class$="[[_computeDiffClass(_panelFloatingDisabled)]]"
+ is-image-diff="{{_isImageDiff}}"
+ files-weblinks="{{_filesWeblinks}}"
+ change-num="[[_changeNum]]"
+ commit-range="[[_commitRange]]"
+ patch-range="[[_patchRange]]"
+ path="[[_path]]"
+ prefs="[[_prefs]]"
+ project-config="[[_projectConfig]]"
+ project-name="[[_change.project]]"
+ view-mode="[[_diffMode]]"
+ is-blame-loaded="{{_isBlameLoaded}}"
+ on-line-selected="_onLineSelected">
+ </gr-diff>
<gr-diff-preferences
id="diffPreferences"
prefs="{{_prefs}}"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 079600c..5ba62af 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -48,7 +48,6 @@
display: none;
}
.diffContainer {
- border-bottom: 1px solid #eee;
display: flex;
font: var(--font-size-small) var(--monospace-font-family);
@apply --diff-container-styles;
diff --git a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.html b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.html
index bbb5111..6e6f7d9 100644
--- a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.html
+++ b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.html
@@ -60,12 +60,14 @@
suite('edit button CUJ', () => {
let navStubs;
+ let openAutoCcmplete;
setup(() => {
navStubs = [
sandbox.stub(Gerrit.Nav, 'getEditUrlForDiff'),
sandbox.stub(Gerrit.Nav, 'navigateToRelativeUrl'),
];
+ openAutoCcmplete = element.$.openDialog.querySelector('gr-autocomplete');
});
test('_isValidPath', () => {
@@ -83,8 +85,8 @@
assert.isTrue(element._hideAllDialogs.called);
assert.isTrue(element.$.openDialog.disabled);
assert.isFalse(queryStub.called);
- element.$.openDialog.querySelector('gr-autocomplete').text =
- 'src/test.cpp';
+ openAutoCcmplete.noDebounce = true;
+ openAutoCcmplete.text = 'src/test.cpp';
assert.isTrue(queryStub.called);
assert.isFalse(element.$.openDialog.disabled);
MockInteractions.tap(element.$.openDialog.$$('gr-button[primary]'));
@@ -99,8 +101,8 @@
MockInteractions.tap(element.$$('#open'));
return showDialogSpy.lastCall.returnValue.then(() => {
assert.isTrue(element.$.openDialog.disabled);
- element.$.openDialog.querySelector('gr-autocomplete').text =
- 'src/test.cpp';
+ openAutoCcmplete.noDebounce = true;
+ openAutoCcmplete.text = 'src/test.cpp';
assert.isFalse(element.$.openDialog.disabled);
MockInteractions.tap(element.$.openDialog.$$('gr-button'));
for (const stub of navStubs) { assert.isFalse(stub.called); }
@@ -113,10 +115,13 @@
suite('delete button CUJ', () => {
let navStub;
let deleteStub;
+ let deleteAutocomplete;
setup(() => {
navStub = sandbox.stub(Gerrit.Nav, 'navigateToChange');
deleteStub = sandbox.stub(element.$.restAPI, 'deleteFileInChangeEdit');
+ deleteAutocomplete =
+ element.$.deleteDialog.querySelector('gr-autocomplete');
});
test('delete', () => {
@@ -125,8 +130,8 @@
return showDialogSpy.lastCall.returnValue.then(() => {
assert.isTrue(element.$.deleteDialog.disabled);
assert.isFalse(queryStub.called);
- element.$.deleteDialog.querySelector('gr-autocomplete').text =
- 'src/test.cpp';
+ deleteAutocomplete.noDebounce = true;
+ deleteAutocomplete.text = 'src/test.cpp';
assert.isTrue(queryStub.called);
assert.isFalse(element.$.deleteDialog.disabled);
MockInteractions.tap(element.$.deleteDialog.$$('gr-button[primary]'));
@@ -148,8 +153,8 @@
return showDialogSpy.lastCall.returnValue.then(() => {
assert.isTrue(element.$.deleteDialog.disabled);
assert.isFalse(queryStub.called);
- element.$.deleteDialog.querySelector('gr-autocomplete').text =
- 'src/test.cpp';
+ deleteAutocomplete.noDebounce = true;
+ deleteAutocomplete.text = 'src/test.cpp';
assert.isTrue(queryStub.called);
assert.isFalse(element.$.deleteDialog.disabled);
MockInteractions.tap(element.$.deleteDialog.$$('gr-button[primary]'));
@@ -182,10 +187,13 @@
suite('rename button CUJ', () => {
let navStub;
let renameStub;
+ let renameAutocomplete;
setup(() => {
navStub = sandbox.stub(Gerrit.Nav, 'navigateToChange');
renameStub = sandbox.stub(element.$.restAPI, 'renameFileInChangeEdit');
+ renameAutocomplete =
+ element.$.renameDialog.querySelector('gr-autocomplete');
});
test('rename', () => {
@@ -194,8 +202,8 @@
return showDialogSpy.lastCall.returnValue.then(() => {
assert.isTrue(element.$.renameDialog.disabled);
assert.isFalse(queryStub.called);
- element.$.renameDialog.querySelector('gr-autocomplete').text =
- 'src/test.cpp';
+ renameAutocomplete.noDebounce = true;
+ renameAutocomplete.text = 'src/test.cpp';
assert.isTrue(queryStub.called);
assert.isTrue(element.$.renameDialog.disabled);
@@ -222,8 +230,8 @@
return showDialogSpy.lastCall.returnValue.then(() => {
assert.isTrue(element.$.renameDialog.disabled);
assert.isFalse(queryStub.called);
- element.$.renameDialog.querySelector('gr-autocomplete').text =
- 'src/test.cpp';
+ renameAutocomplete.noDebounce = true;
+ renameAutocomplete.text = 'src/test.cpp';
assert.isTrue(queryStub.called);
assert.isTrue(element.$.renameDialog.disabled);
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.html b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.html
index 46c69e4..8f2f166 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.html
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.html
@@ -24,6 +24,7 @@
<link rel="import" href="../../plugins/gr-endpoint-param/gr-endpoint-param.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
<link rel="import" href="../../shared/gr-editable-label/gr-editable-label.html">
+<link rel="import" href="../../shared/gr-fixed-panel/gr-fixed-panel.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../gr-default-editor/gr-default-editor.html">
<link rel="import" href="../../../styles/shared-styles.html">
@@ -34,13 +35,9 @@
:host {
background-color: var(--view-background-color);
}
- .sticky {
+ gr-fixed-panel {
background-color: #ebf5fb;
border-bottom: 1px #ddd solid;
- position: -webkit-sticky;
- position: sticky;
- top: 0;
- width: 100vw;
z-index: 1;
}
header,
@@ -88,7 +85,7 @@
}
}
</style>
- <div class="sticky">
+ <gr-fixed-panel keep-on-scroll>
<header>
<span class="controlGroup">
<span>Edit mode</span>
@@ -112,7 +109,7 @@
on-tap="_saveEdit">Save</gr-button>
</span>
</header>
- </div>
+ </gr-fixed-panel>
<div class="textareaWrapper">
<gr-endpoint-decorator id="editorEndpoint" name="editor">
<gr-endpoint-param name="fileContent" value="[[_newContent]]"></gr-endpoint-param>
diff --git a/polygerrit-ui/app/elements/gr-app.html b/polygerrit-ui/app/elements/gr-app.html
index 2f2aa51..325072c 100644
--- a/polygerrit-ui/app/elements/gr-app.html
+++ b/polygerrit-ui/app/elements/gr-app.html
@@ -54,6 +54,7 @@
<link rel="import" href="./settings/gr-cla-view/gr-cla-view.html">
<link rel="import" href="./settings/gr-registration-dialog/gr-registration-dialog.html">
<link rel="import" href="./settings/gr-settings-view/gr-settings-view.html">
+<link rel="import" href="./shared/gr-fixed-panel/gr-fixed-panel.html">
<link rel="import" href="./shared/gr-rest-api-interface/gr-rest-api-interface.html">
<script src="../scripts/util.js"></script>
@@ -62,44 +63,35 @@
<template>
<style include="shared-styles">
:host {
- /* This needs to be inline-flex so that the app container inherits the
- width of its children. From here, can use position: sticky to display
- items that we want to be 'floating' like the header and footer, with
- a combination of: position: sticky, width=100vw, and left: 0. */
- display: inline-flex;
+ display: flex;
min-height: 100%;
flex-direction: column;
}
+ gr-fixed-panel {
+ /**
+ * This one should be greater that the z-index in gr-diff-view
+ * because gr-main-header contains overlay.
+ */
+ z-index: 10;
+ }
gr-main-header,
footer {
color: var(--primary-text-color);
}
+ gr-main-header {
+ background-color: var(--header-background-color);
+ padding: 0 var(--default-horizontal-margin);
+ border-bottom: 1px solid #ddd;
+ }
gr-main-header.shadow {
/* Make it obvious for shadow dom testing */
border-bottom: 1px solid pink;
}
- gr-main-header {
- /* The combination of left, width, position allow this to be
- floating. */
- left: 0;
- width: 100vw;
- background-color: var(--header-background-color);
- padding: 0 var(--default-horizontal-margin);
- position: sticky;
- /* Needed for the menu dropdowns to display on top of the sticky
- headers with z-index of 1.*/
- z-index: 2;
- }
footer {
- /* The combination of left, width, position allow this to be
- floating. */
background-color: var(--footer-background-color);
display: flex;
- left: 0;
- width: 100vw;
justify-content: space-between;
padding: .5rem var(--default-horizontal-margin);
- position: sticky;
z-index: 100;
}
main {
@@ -138,11 +130,13 @@
color: #b71c1c;
}
</style>
- <gr-main-header
- id="mainHeader"
- search-query="{{params.query}}"
- class$="[[_computeShadowClass(_isShadowDom)]]">
- </gr-main-header>
+ <gr-fixed-panel id="header">
+ <gr-main-header
+ id="mainHeader"
+ search-query="{{params.query}}"
+ class$="[[_computeShadowClass(_isShadowDom)]]">
+ </gr-main-header>
+ </gr-fixed-panel>
<main>
<template is="dom-if" if="[[_showChangeListView]]" restamp="true">
<gr-change-list-view
diff --git a/polygerrit-ui/app/elements/gr-app.js b/polygerrit-ui/app/elements/gr-app.js
index 99c4d12..6c96fc1 100644
--- a/polygerrit-ui/app/elements/gr-app.js
+++ b/polygerrit-ui/app/elements/gr-app.js
@@ -182,6 +182,7 @@
if (this.params.justRegistered) {
this.$.registration.open();
}
+ this.$.header.unfloat();
},
_computeShowGwtUiLink(config) {
diff --git a/polygerrit-ui/app/elements/settings/gr-watched-projects-editor/gr-watched-projects-editor.html b/polygerrit-ui/app/elements/settings/gr-watched-projects-editor/gr-watched-projects-editor.html
index e3be10d..cf5e0b1 100644
--- a/polygerrit-ui/app/elements/settings/gr-watched-projects-editor/gr-watched-projects-editor.html
+++ b/polygerrit-ui/app/elements/settings/gr-watched-projects-editor/gr-watched-projects-editor.html
@@ -95,7 +95,6 @@
<th>
<gr-autocomplete
id="newProject"
- debounce-wait="200"
query="[[_query]]"
threshold="1"
placeholder="Project"></gr-autocomplete>
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
index 66a50f0..475821d 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
@@ -15,6 +15,7 @@
'use strict';
const TOKENIZE_REGEX = /(?:[^\s"]+|"[^"]*")+/g;
+ const DEBOUNCE_WAIT_MS = 200;
Polymer({
is: 'gr-autocomplete',
@@ -130,12 +131,11 @@
},
/**
- * The number of milliseconds to use as the debounce wait time. If null,
- * no debouncing is used.
+ * When true, querying for suggestions is not debounced w/r/t keypresses
*/
- debounceWait: {
- type: Number,
- value: null,
+ noDebounce: {
+ type: Boolean,
+ value: false,
},
/** @type {?} */
@@ -173,6 +173,7 @@
detached() {
this.unlisten(document.body, 'tap', '_handleBodyTap');
+ this.cancelDebouncer('update-suggestions');
},
get focusStart() {
@@ -255,10 +256,10 @@
});
};
- if (this.debounceWait) {
- this.debounce('update-suggestions', update, this.debounceWait);
- } else {
+ if (this.noDebounce) {
update();
+ } else {
+ this.debounce('update-suggestions', update, DEBOUNCE_WAIT_MS);
}
},
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
index dba0d6c..76f70f4 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
@@ -27,7 +27,7 @@
<test-fixture id="basic">
<template>
- <gr-autocomplete></gr-autocomplete>
+ <gr-autocomplete no-debounce></gr-autocomplete>
</template>
</test-fixture>
@@ -235,7 +235,7 @@
assert.isTrue(queryStub.called);
});
- test('debounceWait debounces the query', () => {
+ test('noDebounce=false debounces the query', () => {
const queryStub = sandbox.spy(() => {
return Promise.resolve([]);
});
@@ -243,11 +243,11 @@
const debounceStub = sandbox.stub(element, 'debounce',
(name, cb) => { callback = cb; });
element.query = queryStub;
- element.debounceWait = 100;
+ element.noDebounce = false;
element.text = 'a';
assert.isFalse(queryStub.called);
assert.isTrue(debounceStub.called);
- assert.equal(debounceStub.lastCall.args[2], 100);
+ assert.equal(debounceStub.lastCall.args[2], 200);
assert.isFunction(callback);
callback();
assert.isTrue(queryStub.called);
diff --git a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.html b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.html
new file mode 100644
index 0000000..967ff02
--- /dev/null
+++ b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.html
@@ -0,0 +1,50 @@
+<!--
+Copyright (C) 2017 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.
+-->
+
+<link rel="import" href="../../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../../styles/shared-styles.html">
+
+<dom-module id="gr-fixed-panel">
+ <template>
+ <style include="shared-styles">
+ :host {
+ display: block;
+ min-height: var(--header-height);
+ position: relative;
+ }
+ header {
+ background: inherit;
+ border: inherit;
+ display: inline;
+ height: inherit;
+ }
+ .floating {
+ left: 0;
+ position: fixed;
+ width: 100%;
+ will-change: top;
+ }
+ .fixedAtTop {
+ border-bottom: 1px solid #a4a4a4;
+ box-shadow: 0 4px 4px rgba(0,0,0,0.1);
+ }
+ </style>
+ <header id="header" class$="[[_computeHeaderClass(_headerFloating, _topLast)]]">
+ <slot></slot>
+ </header>
+ </template>
+ <script src="gr-fixed-panel.js"></script>
+</dom-module>
diff --git a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.js b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.js
new file mode 100644
index 0000000..f4f1a93
--- /dev/null
+++ b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel.js
@@ -0,0 +1,194 @@
+// Copyright (C) 2017 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.
+(function() {
+ 'use strict';
+
+ Polymer({
+ is: 'gr-fixed-panel',
+
+ properties: {
+ floatingDisabled: Boolean,
+ readyForMeasure: {
+ type: Boolean,
+ observer: '_readyForMeasureObserver',
+ },
+ keepOnScroll: {
+ type: Boolean,
+ value: false,
+ },
+ _isMeasured: {
+ type: Boolean,
+ value: false,
+ },
+
+ /**
+ * Initial offset from the top of the document, in pixels.
+ */
+ _topInitial: Number,
+
+ /**
+ * Current offset from the top of the window, in pixels.
+ */
+ _topLast: Number,
+
+ _headerHeight: Number,
+ _headerFloating: {
+ type: Boolean,
+ value: false,
+ },
+ _observer: {
+ type: Object,
+ value: null,
+ },
+ _webComponentsReady: Boolean,
+ },
+
+ attached() {
+ if (this.floatingDisabled) {
+ return;
+ }
+ // Enable content measure unless blocked by param.
+ if (this.readyForMeasure !== false) {
+ this.readyForMeasure = true;
+ }
+ this.listen(window, 'resize', 'update');
+ this.listen(window, 'scroll', '_updateOnScroll');
+ this._observer = new MutationObserver(this.update.bind(this));
+ this._observer.observe(this.$.header, {childList: true, subtree: true});
+ },
+
+ detached() {
+ this.unlisten(window, 'scroll', '_updateOnScroll');
+ this.unlisten(window, 'resize', 'update');
+ if (this._observer) {
+ this._observer.disconnect();
+ }
+ },
+
+ _readyForMeasureObserver(readyForMeasure) {
+ if (readyForMeasure) {
+ this.update();
+ }
+ },
+
+ _computeHeaderClass(headerFloating, topLast) {
+ const fixedAtTop = this.keepOnScroll && topLast === 0;
+ return [
+ headerFloating ? 'floating' : '',
+ fixedAtTop ? 'fixedAtTop' : '',
+ ].join(' ');
+ },
+
+ _getScrollY() {
+ return window.scrollY;
+ },
+
+ unfloat() {
+ if (this.floatingDisabled) {
+ return;
+ }
+ this.$.header.style.top = '';
+ this._headerFloating = false;
+ this.updateStyles({'--header-height': ''});
+ },
+
+ update() {
+ this.debounce('update', () => {
+ this._updateDebounced();
+ }, 100);
+ },
+
+ _updateOnScroll() {
+ this.debounce('update', () => {
+ this._updateDebounced();
+ });
+ },
+
+ _updateDebounced() {
+ if (this.floatingDisabled) {
+ return;
+ }
+ this._isMeasured = false;
+ this._maybeFloatHeader();
+ this._reposition();
+ },
+
+ _reposition() {
+ if (!this._headerFloating) {
+ return;
+ }
+ const header = this.$.header;
+ const scrollY = this._topInitial - this._getScrollY();
+ let newTop;
+ if (this.keepOnScroll) {
+ if (scrollY > 0) {
+ // Reposition to imitate natural scrolling.
+ newTop = scrollY;
+ } else {
+ newTop = 0;
+ }
+ } else if (scrollY > -this._headerHeight ||
+ this._topLast < -this._headerHeight) {
+ // Allow to scroll away, but ignore when far behind the edge.
+ newTop = scrollY;
+ } else {
+ newTop = -this._headerHeight;
+ }
+ if (this._topLast !== newTop) {
+ if (newTop === undefined) {
+ header.style.top = '';
+ } else {
+ header.style.top = newTop + 'px';
+ }
+ this._topLast = newTop;
+ }
+ },
+
+ _measure() {
+ if (this._isMeasured) {
+ return; // Already measured.
+ }
+ const rect = this.$.header.getBoundingClientRect();
+ if (rect.height === 0 && rect.width === 0) {
+ return; // Not ready for measurement yet.
+ }
+ const top = document.body.scrollTop + rect.top;
+ this._topLast = top;
+ this._headerHeight = rect.height;
+ this._topInitial =
+ this.getBoundingClientRect().top + document.body.scrollTop;
+ this._isMeasured = true;
+ },
+
+ _isFloatingNeeded() {
+ return this.keepOnScroll ||
+ document.body.scrollWidth > document.body.clientWidth;
+ },
+
+ _maybeFloatHeader() {
+ if (!this._isFloatingNeeded()) {
+ return;
+ }
+ this._measure();
+ if (this._isMeasured) {
+ this._floatHeader();
+ }
+ },
+
+ _floatHeader() {
+ this.updateStyles({'--header-height': this._headerHeight + 'px'});
+ this._headerFloating = true;
+ },
+ });
+})();
diff --git a/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.html b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.html
new file mode 100644
index 0000000..408b7c9
--- /dev/null
+++ b/polygerrit-ui/app/elements/shared/gr-fixed-panel/gr-fixed-panel_test.html
@@ -0,0 +1,111 @@
+<!DOCTYPE html>
+<!--
+Copyright (C) 2017 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.
+-->
+
+<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
+<title>gr-fixed-panel</title>
+
+<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
+<script src="../../../bower_components/web-component-tester/browser.js"></script>
+<link rel="import" href="../../../test/common-test-setup.html"/>
+<link rel="import" href="gr-fixed-panel.html">
+
+<script>void(0);</script>
+
+<test-fixture id="basic">
+ <template>
+ <gr-fixed-panel>
+ <div style="height: 100px"></div>
+ </gr-fixed-panel>
+ </template>
+</test-fixture>
+
+<script>
+ suite('gr-fixed-panel', () => {
+ let element;
+ let sandbox;
+
+ setup(() => {
+ sandbox = sinon.sandbox.create();
+ element = fixture('basic');
+ element.readyForMeasure = true;
+ });
+
+ teardown(() => {
+ sandbox.restore();
+ });
+
+ test('can be disabled with floatingDisabled', () => {
+ element.floatingDisabled = true;
+ sandbox.stub(element, '_reposition');
+ window.dispatchEvent(new CustomEvent('resize'));
+ element.flushDebouncer('update');
+ assert.isFalse(element._reposition.called);
+ });
+
+ test('header is the height of the content', () => {
+ assert.equal(element.getBoundingClientRect().height, 100);
+ });
+
+ test('scroll triggers _reposition', () => {
+ sandbox.stub(element, '_reposition');
+ window.dispatchEvent(new CustomEvent('scroll'));
+ element.flushDebouncer('update');
+ assert.isTrue(element._reposition.called);
+ });
+
+ suite('_reposition', () => {
+ const getHeaderTop = function() {
+ return element.$.header.style.top;
+ };
+
+ const emulateScrollY = function(distance) {
+ element._getScrollY.returns(distance);
+ element._updateDebounced();
+ element.flushDebouncer('scroll');
+ };
+
+ setup(() => {
+ element._headerTopInitial = 10;
+ sandbox.stub(element, '_getScrollY').returns(0);
+ });
+
+ test('scrolls header along with document', () => {
+ emulateScrollY(20);
+ assert.equal(getHeaderTop(), '-12px');
+ });
+
+ test('does not stick to the top by default', () => {
+ emulateScrollY(150);
+ assert.equal(getHeaderTop(), '-100px');
+ });
+
+ test('sticks to the top if enabled', () => {
+ element.keepOnScroll = true;
+ emulateScrollY(120);
+ assert.equal(getHeaderTop(), '0px');
+ });
+
+ test('drops a shadow when fixed to the top', () => {
+ element.keepOnScroll = true;
+ emulateScrollY(5);
+ assert.isFalse(element.$.header.classList.contains('fixedAtTop'));
+ emulateScrollY(120);
+ assert.isTrue(element.$.header.classList.contains('fixedAtTop'));
+ });
+ });
+ });
+</script>
diff --git a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.html b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.html
index 06aa43b..8e47485 100644
--- a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.html
+++ b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.html
@@ -42,7 +42,7 @@
<!-- This is a custom PolyGerrit SVG -->
<g id="side-by-side"><path d="M17.1578947,10.8888889 L2.84210526,10.8888889 C2.37894737,10.8888889 2,11.2888889 2,11.7777778 L2,17.1111111 C2,17.6 2.37894737,18 2.84210526,18 L17.1578947,18 C17.6210526,18 18,17.6 18,17.1111111 L18,11.7777778 C18,11.2888889 17.6210526,10.8888889 17.1578947,10.8888889 Z M17.1578947,2 L2.84210526,2 C2.37894737,2 2,2.4 2,2.88888889 L2,8.22222222 C2,8.71111111 2.37894737,9.11111111 2.84210526,9.11111111 L17.1578947,9.11111111 C17.6210526,9.11111111 18,8.71111111 18,8.22222222 L18,2.88888889 C18,2.4 17.6210526,2 17.1578947,2 Z M16.1973628,2 L2.78874238,2 C2.35493407,2 2,2.4 2,2.88888889 L2,8.22222222 C2,8.71111111 2.35493407,9.11111111 2.78874238,9.11111111 L16.1973628,9.11111111 C16.6311711,9.11111111 16.9861052,8.71111111 16.9861052,8.22222222 L16.9861052,2.88888889 C16.9861052,2.4 16.6311711,2 16.1973628,2 Z" id="Shape" transform="scale(1.2) translate(10.000000, 10.000000) rotate(-90.000000) translate(-10.000000, -10.000000)"/></g>
<!-- This is a custom PolyGerrit SVG -->
- <g id="unified"><path d="M4,2 L17,2 C18.1045695,2 19,2.8954305 19,4 L19,16 C19,17.1045695 18.1045695,18 17,18 L4,18 C2.8954305,18 2,17.1045695 2,16 L2,4 L2,4 C2,2.8954305 2.8954305,2 4,2 L4,2 Z M4,7 L4,9 L17,9 L17,7 L4,7 Z M4,11 L4,13 L17,13 L17,11 L4,11 Z" id="Combined-Shape" transform="scale(1.2)"/></g>
+ <g id="unified"><path d="M4,2 L17,2 C18.1045695,2 19,2.8954305 19,4 L19,16 C19,17.1045695 18.1045695,18 17,18 L4,18 C2.8954305,18 2,17.1045695 2,16 L2,4 L2,4 C2,2.8954305 2.8954305,2 4,2 L4,2 Z M4,7 L4,9 L17,9 L17,7 L4,7 Z M4,11 L4,13 L17,13 L17,11 L4,11 Z" id="Combined-Shape" transform="scale(1.12, 1.2)"/></g>
<!-- This is a custom PolyGerrit SVG -->
<g id="content-copy"><path d="M16 1H4c-1.1 0-2 .9-2 2v14h2V3h12V1zm3 4H8c-1.1 0-2 .9-2 2v14c0 1.1.9 2 2 2h11c1.1 0 2-.9 2-2V7c0-1.1-.9-2-2-2zm0 16H8V7h11v14z"/></g>
<!-- This is a custom PolyGerrit SVG -->
