Merge "Surface LockFailureException as 503 Service Unavailable"
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/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..09ee8f0 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -201,7 +201,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 {
@@ -317,8 +322,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 +340,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 {
diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
index 086cacb..36f9b82 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) {
diff --git a/java/com/google/gerrit/acceptance/ProjectResetter.java b/java/com/google/gerrit/acceptance/ProjectResetter.java
index 569a0c1..0e4ea19 100644
--- a/java/com/google/gerrit/acceptance/ProjectResetter.java
+++ b/java/com/google/gerrit/acceptance/ProjectResetter.java
@@ -88,8 +88,6 @@
@Nullable private final AccountCache accountCache;
@Nullable private final ProjectCache projectCache;
- private final Multimap<Project.NameKey, String> refsByProject;
-
@Inject
public Builder(
GitRepositoryManager repoManager,
@@ -102,10 +100,27 @@
this.accountCreator = accountCreator;
this.accountCache = accountCache;
this.projectCache = projectCache;
+ }
+
+ public ProjectResetter build(ProjectResetter.Config input) throws IOException {
+ return new ProjectResetter(
+ repoManager,
+ allUsersName,
+ accountCreator,
+ accountCache,
+ 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,11 +128,6 @@
refsByProject.putAll(project, refPatternList);
return this;
}
-
- public ProjectResetter build() throws IOException {
- return new ProjectResetter(
- repoManager, allUsersName, accountCreator, accountCache, projectCache, refsByProject);
- }
}
@Inject private GitRepositoryManager repoManager;
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/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/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/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/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..09f2837 100644
--- a/java/com/google/gerrit/server/config/PluginConfigFactory.java
+++ b/java/com/google/gerrit/server/config/PluginConfigFactory.java
@@ -288,7 +288,32 @@
*/
public Config getProjectPluginConfigWithInheritance(
Project.NameKey projectName, String pluginName) throws NoSuchProjectException {
- return getPluginConfig(projectName, pluginName).getWithInheritance();
+ return getPluginConfig(projectName, pluginName).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);
}
/**
@@ -310,7 +335,30 @@
*/
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 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/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/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/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/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/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/org/eclipse/jgit/BUILD b/java/org/eclipse/jgit/BUILD
index 65bc118..b9b807a 100644
--- a/java/org/eclipse/jgit/BUILD
+++ b/java/org/eclipse/jgit/BUILD
@@ -39,7 +39,6 @@
srcs = [
"diff/EditDeserializer.java",
"diff/ReplaceEdit.java",
- "internal/storage/file/WindowCacheStatAccessor.java",
"lib/ObjectIdSerialization.java",
],
visibility = ["//visibility:public"],
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/javatests/com/google/gerrit/acceptance/ProjectResetterTest.java b/javatests/com/google/gerrit/acceptance/ProjectResetterTest.java
index 6c3a4b0..74b23a4 100644
--- a/javatests/com/google/gerrit/acceptance/ProjectResetterTest.java
+++ b/javatests/com/google/gerrit/acceptance/ProjectResetterTest.java
@@ -71,7 +71,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 +88,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 +111,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 +142,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 +173,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 +198,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 +223,8 @@
Ref nonMetaConfig = createRef("refs/heads/master");
try (ProjectResetter resetProject =
- builder(null, null, projectCache).reset(project).reset(project2).build()) {
+ builder(null, null, projectCache)
+ .build(new ProjectResetter.Config().reset(project).reset(project2))) {
updateRef(nonMetaConfig);
updateRef(repo2, metaConfig);
}
@@ -225,7 +243,8 @@
EasyMock.replay(projectCache);
try (ProjectResetter resetProject =
- builder(null, null, projectCache).reset(project).reset(project2).build()) {
+ builder(null, null, projectCache)
+ .build(new ProjectResetter.Config().reset(project).reset(project2))) {
createRef("refs/heads/master");
createRef(repo2, RefNames.REFS_CONFIG);
}
@@ -249,7 +268,8 @@
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(2)));
try (ProjectResetter resetProject =
- builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
+ builder(null, accountCache, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
updateRef(allUsersRepo, userBranch);
}
@@ -269,7 +289,8 @@
EasyMock.replay(accountCache);
try (ProjectResetter resetProject =
- builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
+ builder(null, accountCache, 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)));
@@ -300,7 +321,8 @@
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(3)));
try (ProjectResetter resetProject =
- builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
+ builder(null, accountCache, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
updateRef(allUsersRepo, externalIds);
createRef(allUsersRepo, RefNames.refsUsers(accountId2));
@@ -329,7 +351,8 @@
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(3)));
try (ProjectResetter resetProject =
- builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
+ builder(null, accountCache, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
createRef(allUsersRepo, RefNames.REFS_EXTERNAL_IDS);
createRef(allUsersRepo, RefNames.refsUsers(accountId2));
@@ -350,7 +373,8 @@
EasyMock.replay(accountCreator);
try (ProjectResetter resetProject =
- builder(accountCreator, null, null).reset(project).reset(allUsers).build()) {
+ builder(accountCreator, null, null)
+ .build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
createRef(allUsersRepo, RefNames.refsUsers(accountId));
}
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..32dc5dd 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
@@ -1076,7 +1075,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)) {
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/lib/jgit/jgit.bzl b/lib/jgit/jgit.bzl
index 7bee9e34..71b91a5 100644
--- a/lib/jgit/jgit.bzl
+++ b/lib/jgit/jgit.bzl
@@ -1,12 +1,12 @@
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"
-_DOC_VERS = "4.10.0.201712302008-r" # Set to _JGIT_VERS unless using a snapshot
+_DOC_VERS = _JGIT_VERS # Set to _JGIT_VERS unless using a snapshot
JGIT_DOC_URL = "http://download.eclipse.org/jgit/site/" + _DOC_VERS + "/apidocs"
-_JGIT_REPO = GERRIT # Leave here even if set to MAVEN_CENTRAL.
+_JGIT_REPO = MAVEN_CENTRAL # Leave here even if set to MAVEN_CENTRAL.
# set this to use a local version.
# "/home/<user>/projects/jgit"
@@ -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 = "4ae44a6157e1bc4c5b373be0c274a8f1d9badd76",
+ src_sha1 = "ad6f30f7b7f016a1390f8d289be7da2a9a1f47c5",
unsign = True,
)
maven_jar(
name = "jgit_servlet",
artifact = "org.eclipse.jgit:org.eclipse.jgit.http.server:" + _JGIT_VERS,
repository = _JGIT_REPO,
- sha1 = "f7a88c3744f864587d50096ab99a58e09e4afd95",
+ sha1 = "687f1d10cfc6424dfbe06acb54b9e67afe2fd917",
unsign = True,
)
maven_jar(
name = "jgit_archive",
artifact = "org.eclipse.jgit:org.eclipse.jgit.archive:" + _JGIT_VERS,
repository = _JGIT_REPO,
- sha1 = "af579bcc932fa26f3c4d4ae00e812dc50d50a355",
+ sha1 = "134489ba021e0923735ec85d07f2adde2c8d1e8d",
)
maven_jar(
name = "jgit_junit",
artifact = "org.eclipse.jgit:org.eclipse.jgit.junit:" + _JGIT_VERS,
repository = _JGIT_REPO,
- sha1 = "b5fc2330bf0418e3a0f773925c57497131f13380",
+ sha1 = "532ad27983c0d77254020ab22d0b2bb8f3d7cd0f",
unsign = True,
)
diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor
index 714fc10..fa9df30 160000
--- a/plugins/codemirror-editor
+++ b/plugins/codemirror-editor
@@ -1 +1 @@
-Subproject commit 714fc1057174227bd5acf4753039fe582c930e18
+Subproject commit fa9df3035c306069758712bfe9ae3425b119bb0c
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 ed65dae..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
@@ -47,46 +47,60 @@
.weblinks.show {
display: block;
}
+ .loading {
+ display: none;
+ }
+ #loading.loading {
+ display: block;
+ }
+ #loading:not(.loading) {
+ display: none;
+ }
</style>
<style include="gr-menu-page-styles"></style>
<main class$="[[_computeAdminClass(_isAdmin, _canUpload)]]">
- <template is="dom-if" if="[[_inheritsFrom]]">
- <h3 id="inheritsFrom">Rights Inherit From
- <a href$="[[_computeParentHref(_inheritsFrom.name)]]" rel="noopener">
- [[_inheritsFrom.name]]</a>
- </h3>
- </template>
- <div class$="weblinks [[_computeWebLinkClass(_weblinks)]]">
- History:
- <template is="dom-repeat" items="[[_weblinks]]" as="link">
- <a href="[[link.url]]" class="weblink" rel="noopener" target="[[link.target]]">
- [[link.name]]
- </a>
- </template>
+ <div id="loading" class$="[[_computeLoadingClass(_loading)]]">
+ Loading...
</div>
- <gr-button id="editBtn"
- class$="[[_computeShowEditClass(_sections)]]"
- on-tap="_handleEdit">[[_editOrCancel(_editing)]]</gr-button>
- <gr-button id="saveBtn"
- primary
- class$="[[_computeShowSaveClass(_editing)]]"
- on-tap="_handleSaveForReview"
- disabled$="[[!_modified]]">
- Save for review</gr-button>
- <template
- is="dom-repeat"
- items="{{_sections}}"
- as="section">
- <gr-access-section
- capabilities="[[_capabilities]]"
- section="{{section}}"
- labels="[[_labels]]"
- editing="[[_editing]]"
- groups="[[_groups]]"></gr-access-section>
- </template>
- <gr-button id="addReferenceBtn"
- class$="[[_computeShowSaveClass(_editing)]]"
- on-tap="_handleCreateSection">Add Reference</gr-button>
+ <div id="loadedContent" class$="[[_computeLoadingClass(_loading)]]">
+ <template is="dom-if" if="[[_inheritsFrom]]">
+ <h3 id="inheritsFrom">Rights Inherit From
+ <a href$="[[_computeParentHref(_inheritsFrom.name)]]" rel="noopener">
+ [[_inheritsFrom.name]]</a>
+ </h3>
+ </template>
+ <div class$="weblinks [[_computeWebLinkClass(_weblinks)]]">
+ History:
+ <template is="dom-repeat" items="[[_weblinks]]" as="link">
+ <a href="[[link.url]]" class="weblink" rel="noopener" target="[[link.target]]">
+ [[link.name]]
+ </a>
+ </template>
+ </div>
+ <gr-button id="editBtn"
+ class="visible"
+ on-tap="_handleEdit">[[_editOrCancel(_editing)]]</gr-button>
+ <gr-button id="saveBtn"
+ primary
+ class$="[[_computeShowSaveClass(_editing)]]"
+ on-tap="_handleSaveForReview"
+ disabled$="[[!_modified]]">
+ Save for review</gr-button>
+ <template
+ is="dom-repeat"
+ items="{{_sections}}"
+ as="section">
+ <gr-access-section
+ capabilities="[[_capabilities]]"
+ section="{{section}}"
+ labels="[[_labels]]"
+ editing="[[_editing]]"
+ groups="[[_groups]]"></gr-access-section>
+ </template>
+ <gr-button id="addReferenceBtn"
+ class$="[[_computeShowSaveClass(_editing)]]"
+ on-tap="_handleCreateSection">Add Reference</gr-button>
+ </div>
</main>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
</template>
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 7149292..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
@@ -100,6 +100,10 @@
},
_sections: Array,
_weblinks: Array,
+ _loading: {
+ type: Boolean,
+ value: true,
+ },
},
behaviors: [
@@ -150,9 +154,14 @@
this._capabilities = capabilities;
this._labels = labels;
this._sections = sections;
+ this._loading = false;
});
},
+ _computeLoadingClass(loading) {
+ return loading ? 'loading' : '';
+ },
+
_handleEdit() {
this._editing = !this._editing;
},
@@ -307,11 +316,6 @@
});
},
- _computeShowEditClass(sections) {
- if (!sections.length) { return ''; }
- return 'visible';
- },
-
_computeShowSaveClass(editing) {
if (!editing) { return ''; }
return 'visible';
@@ -326,4 +330,4 @@
`/admin/repos/${this.encodeURL(repoName, true)},access`;
},
});
-})();
\ No newline at end of file
+})();
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html
index b5efbe3..242662c 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html
+++ b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html
@@ -117,6 +117,7 @@
});
repoStub = sandbox.stub(element.$.restAPI, 'getRepo').returns(
Promise.resolve(repoRes));
+ element._loading = false;
});
teardown(() => {
@@ -211,6 +212,11 @@
assert.isTrue(element._computeParentHref.called);
});
+ test('_computeLoadingClass', () => {
+ assert.equal(element._computeLoadingClass(true), 'loading');
+ assert.equal(element._computeLoadingClass(false), '');
+ });
+
suite('with defined sections', () => {
const testEditSaveCancelBtns = () => {
// Edit button is visible and Save button is hidden.
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 46f3e34..419fb56 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
@@ -18,6 +18,7 @@
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
+<link rel="import" href="../../../bower_components/paper-tabs/paper-tabs.html">
<link rel="import" href="../../../styles/shared-styles.html">
<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
@@ -29,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">
@@ -44,6 +47,7 @@
<link rel="import" href="../gr-messages-list/gr-messages-list.html">
<link rel="import" href="../gr-related-changes-list/gr-related-changes-list.html">
<link rel="import" href="../gr-reply-dialog/gr-reply-dialog.html">
+<link rel="import" href="../gr-thread-list/gr-thread-list.html">
<dom-module id="gr-change-view">
<template>
@@ -239,6 +243,24 @@
display: inline-block;
margin-right: -5px;
}
+ paper-tabs {
+ background-color: #fafafa;
+ border-top: 1px solid #ddd;
+ height: 3rem;
+ --paper-tabs-selection-bar-color: var(--color-link);
+ }
+ paper-tab {
+ max-width: 15rem;
+ --paper-tab-ink: var(--color-link);
+ }
+ #threadList,
+ #messageList {
+ display: none;
+ }
+ #threadList.visible,
+ #messageList.visible {
+ display: block;
+ }
@media screen and (min-width: 80em) {
.commitMessage {
max-width: var(--commit-message-max-width, 100ch);
@@ -526,15 +548,35 @@
<gr-endpoint-param name="revision" value="[[_selectedRevision]]">
</gr-endpoint-param>
</gr-endpoint-decorator>
+ <paper-tabs
+ id="commentTabs"
+ on-selected-changed="_handleTabChange">
+ <paper-tab class="changeLog">Change Log</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"
+ class$="hideOnMobileOverlay [[_computeShowMessages(_showMessagesView)]]"
change-num="[[_changeNum]]"
+ labels="[[_change.labels]]"
messages="[[_change.messages]]"
reviewer-updates="[[_change.reviewer_updates]]"
change-comments="[[_changeComments]]"
project-name="[[_change.project]]"
show-reply-buttons="[[_loggedIn]]"
on-reply="_handleMessageReply"></gr-messages-list>
+ <gr-thread-list
+ id="threadList"
+ threads="[[_commentThreads]]"
+ change="[[_change]]"
+ change-num="[[_changeNum]]"
+ class$="[[_computeShowThreads(_showMessagesView)]]"
+ on-thread-list-modified="_handleReloadDiffComments"></gr-thread-list>
</div>
<gr-overlay id="downloadOverlay" with-backdrop>
<gr-download-dialog
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 028d76a..9c90832 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,
@@ -228,6 +229,10 @@
type: Boolean,
value: undefined,
},
+ _showMessagesView: {
+ type: Boolean,
+ value: true,
+ },
},
behaviors: [
@@ -245,6 +250,7 @@
// again upon closing.
'fullscreen-overlay-opened': '_handleHideBackgroundContent',
'fullscreen-overlay-closed': '_handleShowBackgroundContent',
+ 'diff-comments-modified': '_handleReloadCommentThreads',
},
observers: [
'_labelsChanged(_change.labels.*)',
@@ -329,6 +335,18 @@
}
},
+ _handleTabChange() {
+ this._showMessagesView = this.$.commentTabs.selected === 0;
+ },
+
+ _computeShowMessages(showSection) {
+ return showSection ? 'visible' : '';
+ },
+
+ _computeShowThreads(showSection) {
+ return !showSection ? 'visible' : '';
+ },
+
_handleEditCommitMessage(e) {
this._editingCommitMessage = true;
this.$.commitMessageEditor.focusTextarea();
@@ -399,6 +417,40 @@
return false;
},
+ _handleReloadCommentThreads() {
+ // Get any new drafts that have been saved in the diff view and show
+ // in the comment thread view.
+ this._reloadDrafts().then(() => {
+ this._commentThreads = this._changeComments.getAllThreadsForChange()
+ .map(c => Object.assign({}, c));
+ Polymer.dom.flush();
+ });
+ },
+
+ _handleReloadDiffComments(e) {
+ // Keeps the file list counts updated.
+ this._reloadDrafts().then(() => {
+ // Get any new drafts that have been saved in the thread view and show
+ // in the diff view.
+ this.$.fileList.reloadCommentsForThreadWithRootId(e.detail.rootId,
+ e.detail.path);
+ Polymer.dom.flush();
+ });
+ },
+
+ _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; }
@@ -580,6 +632,15 @@
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();
});
@@ -600,6 +661,10 @@
this._sendShowChangeEvent();
+ // Selected has to be set after the paper-tabs are visible because
+ // the selected underline depends on calculations made by the browser.
+ this.$.commentTabs.selected = 0;
+
this.async(() => {
if (this.viewState.scrollTop) {
document.documentElement.scrollTop =
@@ -1112,6 +1177,8 @@
.then(comments => {
this._changeComments = comments;
this._diffDrafts = Object.assign({}, this._changeComments.drafts);
+ this._commentThreads = this._changeComments.getAllThreadsForChange()
+ .map(c => Object.assign({}, c));
});
},
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 2d2bf94..35c4bc1 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
@@ -58,6 +58,9 @@
stub('gr-rest-api-interface', {
getConfig() { return Promise.resolve({test: 'config'}); },
getAccount() { return Promise.resolve(null); },
+ getDiffComments() { return Promise.resolve({}); },
+ getDiffRobotComments() { return Promise.resolve({}); },
+ getDiffDrafts() { return Promise.resolve({}); },
_fetchSharedCacheURL() { return Promise.resolve({}); },
});
element = fixture('basic');
@@ -313,6 +316,104 @@
});
});
+ test('diff comments modified', () => {
+ sandbox.spy(element, '_handleReloadCommentThreads');
+ return element._reloadComments().then(() => {
+ element.fire('diff-comments-modified');
+ assert.isTrue(element._handleReloadCommentThreads.called);
+ });
+ });
+
+ test('thread list modified', () => {
+ sandbox.spy(element, '_handleReloadDiffComments');
+ return element._reloadComments().then(() => {
+ element.$.threadList.fire('thread-list-modified');
+ assert.isTrue(element._handleReloadDiffComments.called);
+ });
+ });
+
+ 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 = {
+ basePatchNum: 'PARENT',
+ patchNum: 1,
+ };
+ element._change = {
+ change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
+ revisions: {
+ rev2: {_number: 2, commit: {parents: []}},
+ rev1: {_number: 1, commit: {parents: []}},
+ rev13: {_number: 13, commit: {parents: []}},
+ rev3: {_number: 3, commit: {parents: []}},
+ },
+ current_revision: 'rev3',
+ status: 'NEW',
+ labels: {
+ test: {
+ all: [],
+ default_value: 0,
+ values: [],
+ approved: {},
+ },
+ },
+ };
+ sandbox.stub(element.$.relatedChanges, 'reload');
+ sandbox.stub(element, '_reload').returns(Promise.resolve());
+ sandbox.spy(element, '_paramsChanged');
+ element.params = {view: 'change', changeNum: '1'};
+
+ // When the change is hard reloaded, paramsChanged will not set the tab.
+ // It will be set in postLoadTasks, which requires the flush() to detect.
+ assert.isTrue(element._paramsChanged.called);
+ assert.isUndefined(element.$.commentTabs.selected);
+
+ // Wait for tab to get selected
+ flush(() => {
+ assert.equal(element.$.commentTabs.selected, 0);
+ assert.notEqual(getComputedStyle(element.$.messageList).display,
+ 'none');
+ assert.equal(getComputedStyle(element.$.threadList).display, 'none');
+
+ // Switch to comment thread tab
+ MockInteractions.tap(element.$$('paper-tab.commentThreads'));
+ assert.equal(element.$.commentTabs.selected, 1);
+ assert.equal(getComputedStyle(element.$.messageList).display,
+ 'none');
+ 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.
+ element._paramsChanged(element.params);
+ assert.equal(element.$.commentTabs.selected, 0);
+ done();
+ });
+ });
+
test('reply button is not visible when logged out', () => {
assert.equal(getComputedStyle(element.$.replyBtn).display, 'none');
element._loggedIn = true;
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index 4f086ce..640c273 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -962,15 +962,6 @@
const threadEl = diff.getThreadEls().find(t => t.rootId === rootId);
if (!threadEl) { return; }
- // In gr-diff-comment, _toggleResolved update the comment when
- // it was previously set and the comment is not being edited.
- // When comments was not reset/flushed prior to updating, the
- // comment would attempt to be saved again. If this becomes a
- // performance issue, will need to evaluate other options, but for
- // now, the reset and flush is necessary.
- threadEl.comments = [];
- Polymer.dom.flush();
-
const newComments = this.changeComments.getCommentsForThread(rootId);
// If newComments is null, it means that a single draft was
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.html b/polygerrit-ui/app/elements/change/gr-message/gr-message.html
index 973219e..0898a60 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.html
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.html
@@ -21,11 +21,13 @@
<link rel="import" href="../../shared/gr-formatted-text/gr-formatted-text.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../../styles/shared-styles.html">
+<link rel="import" href="../../../styles/gr-voting-styles.html">
<link rel="import" href="../gr-comment-list/gr-comment-list.html">
<dom-module id="gr-message">
<template>
+ <style include="gr-voting-styles"></style>
<style include="shared-styles">
:host {
border-bottom: 1px solid #ddd;
@@ -131,11 +133,25 @@
.replyContainer {
padding: .5em 0 0 0;
}
- .positiveVote {
- box-shadow: inset 0 3.8em #d4ffd4;
+ .score {
+ border: 1px solid rgba(0,0,0,.12);
+ border-radius: 3px;
+ color: #000;
+ display: inline-block;
+ margin: -.1em 0;
+ padding: 0 .1em;
}
- .negativeVote {
- box-shadow: inset 0 3.8em #ffd4d4;
+ .score.negative {
+ background-color: var(--vote-color-negative);
+ }
+ .score.negative.min {
+ background-color: var(--vote-color-min);
+ }
+ .score.positive {
+ background-color: var(--vote-color-positive);
+ }
+ .score.positive.max {
+ background-color: var(--vote-color-max);
}
gr-account-label {
--gr-account-label-text-style: {
@@ -154,6 +170,11 @@
<gr-account-label
account="[[author]]"
hide-avatar></gr-account-label>
+ <template is="dom-repeat" items="[[_getScores(message)]]" as="score">
+ <span class$="score [[_computeScoreClass(score, labelExtremes)]]">
+ [[score.label]] [[score.value]]
+ </span>
+ </template>
</div>
<template is="dom-if" if="[[message.message]]">
<div class="content">
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.js b/polygerrit-ui/app/elements/change/gr-message/gr-message.js
index 0e73c6e..fccde5b 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.js
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.js
@@ -14,7 +14,6 @@
(function() {
'use strict';
- const CI_LABELS = ['Trybot-Ready', 'Tryjob-Request', 'Commit-Queue'];
const PATCH_SET_PREFIX_PATTERN = /^Patch Set \d+: /;
const LABEL_TITLE_SCORE_PATTERN = /^([A-Za-z0-9-]+)([+-]\d+)$/;
@@ -79,6 +78,13 @@
type: String,
observer: '_projectNameChanged',
},
+
+ /**
+ * A mapping from label names to objects representing the minimum and
+ * maximum possible values for that label.
+ */
+ labelExtremes: Object,
+
/**
* @type {{ commentlinks: Array }}
*/
@@ -175,41 +181,42 @@
return event.type === 'REVIEWER_UPDATE';
},
- _isMessagePositive(message) {
- if (!message.message) { return null; }
+ _getScores(message) {
+ if (!message.message) { return []; }
const line = message.message.split('\n', 1)[0];
const patchSetPrefix = PATCH_SET_PREFIX_PATTERN;
- if (!line.match(patchSetPrefix)) { return null; }
+ if (!line.match(patchSetPrefix)) { return []; }
const scoresRaw = line.split(patchSetPrefix)[1];
- if (!scoresRaw) { return null; }
- const scores = scoresRaw.split(' ');
- if (!scores.length) { return null; }
- const {min, max} = scores
+ if (!scoresRaw) { return []; }
+ return scoresRaw.split(' ')
.map(s => s.match(LABEL_TITLE_SCORE_PATTERN))
.filter(ms => ms && ms.length === 3)
- .filter(([, label]) => !CI_LABELS.includes(label))
- .map(([, , score]) => score)
- .map(s => parseInt(s, 10))
- .reduce(({min, max}, s) =>
- ({min: (s < min ? s : min), max: (s > max ? s : max)}),
- {min: 0, max: 0});
- if (max - min === 0) {
- return 0;
- } else {
- return (max + min) > 0 ? 1 : -1;
+ .map(ms => ({label: ms[1], value: ms[2]}));
+ },
+
+ _computeScoreClass(score, labelExtremes) {
+ const classes = [];
+ if (score.value > 0) {
+ classes.push('positive');
+ } else if (score.value < 0) {
+ classes.push('negative');
}
+ const extremes = labelExtremes[score.label];
+ if (extremes) {
+ const intScore = parseInt(score.value, 10);
+ if (intScore === extremes.max) {
+ classes.push('max');
+ } else if (intScore === extremes.min) {
+ classes.push('min');
+ }
+ }
+ return classes.join(' ');
},
_computeClass(expanded, showAvatar, message) {
const classes = [];
classes.push(expanded ? 'expanded' : 'collapsed');
classes.push(showAvatar ? 'showAvatar' : 'hideAvatar');
- const scoreQuality = this._isMessagePositive(message);
- if (scoreQuality === 1) {
- classes.push('positiveVote');
- } else if (scoreQuality === -1) {
- classes.push('negativeVote');
- }
return classes.join(' ');
},
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
index 658dcad..b0ba2d9 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
@@ -163,22 +163,29 @@
});
});
- test('negative vote', () => {
+ test('votes', () => {
element.message = {
author: {},
expanded: false,
message: 'Patch Set 1: Verified+1 Code-Review-2 Trybot-Ready+1',
};
- assert.isOk(Polymer.dom(element.root).querySelector('.negativeVote'));
- });
-
- test('positive vote', () => {
- element.message = {
- author: {},
- expanded: false,
- message: 'Patch Set 1: Verified-1 Code-Review+2 Trybot-Ready-1',
+ element.labelExtremes = {
+ 'Verified': {max: 1, min: -1},
+ 'Code-Review': {max: 2, min: -2},
+ 'Trybot-Ready': {max: 3, min: 0},
};
- assert.isOk(Polymer.dom(element.root).querySelector('.positiveVote'));
+ flushAsynchronousOperations();
+ const scoreChips = Polymer.dom(element.root).querySelectorAll('.score');
+ assert.equal(scoreChips.length, 3);
+
+ assert.isTrue(scoreChips[0].classList.contains('positive'));
+ assert.isTrue(scoreChips[0].classList.contains('max'));
+
+ assert.isTrue(scoreChips[1].classList.contains('negative'));
+ assert.isTrue(scoreChips[1].classList.contains('min'));
+
+ assert.isTrue(scoreChips[2].classList.contains('positive'));
+ assert.isFalse(scoreChips[2].classList.contains('min'));
});
test('false negative vote', () => {
@@ -187,7 +194,9 @@
expanded: false,
message: 'Patch Set 1: Cherry Picked from branch stable-2.14.',
};
- assert.isNotOk(Polymer.dom(element.root).querySelector('.negativeVote'));
+ element.labelExtremes = {};
+ const scoreChips = Polymer.dom(element.root).querySelectorAll('.score');
+ assert.equal(scoreChips.length, 0);
});
});
</script>
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html
index df9ce54..2a173e5 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html
@@ -26,7 +26,8 @@
<style include="shared-styles">
:host,
.messageListControls {
- display: block;
+ display: flex;
+ justify-content: space-between;
}
.header {
align-items: center;
@@ -64,8 +65,6 @@
}
</style>
<div class="header">
- <h3>Messages</h3>
- <div class="messageListControls container">
<span
id="automatedMessageToggleContainer"
class="container"
@@ -82,7 +81,6 @@
[[_computeExpandCollapseMessage(_expanded)]]
</gr-button>
</div>
- </div>
<span
id="messageControlsContainer"
hidden$="[[_computeShowHideTextHidden(_visibleMessages, _processedMessages, _hideAutomated, _visibleMessages.length)]]">
@@ -111,6 +109,7 @@
project-name="[[projectName]]"
show-reply-button="[[showReplyButtons]]"
on-scroll-to="_handleScrollTo"
+ label-extremes="[[_labelExtremes]]"
data-message-id$="[[message.id]]"></gr-message>
</template>
<gr-reporting id="reporting" category="message-list"></gr-reporting>
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js
index 9ccadf7..00097b0 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js
@@ -41,6 +41,7 @@
type: Boolean,
value: false,
},
+ labels: Object,
_expanded: {
type: Boolean,
@@ -66,6 +67,11 @@
type: Array,
value() { return []; },
},
+
+ _labelExtremes: {
+ type: Object,
+ computed: '_computeLabelExtremes(labels.*)',
+ },
},
scrollToMessage(messageID) {
@@ -331,5 +337,24 @@
this._numRemaining(visibleMessages, messages, hideAutomated);
return total <= this._getDelta(visibleMessages, messages, hideAutomated);
},
+
+ /**
+ * Compute a mapping from label name to objects representing the minimum and
+ * maximum possible values for that label.
+ */
+ _computeLabelExtremes(labelRecord) {
+ const extremes = {};
+ const labels = labelRecord.base;
+ if (!labels) { return extremes; }
+ for (const key of Object.keys(labels)) {
+ if (!labels[key] || !labels[key].values) { continue; }
+ const values = Object.keys(labels[key].values)
+ .map(v => parseInt(v, 10));
+ values.sort();
+ if (!values.length) { continue; }
+ extremes[key] = {min: values[0], max: values[values.length - 1]};
+ }
+ return extremes;
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html
index 453d97a..06801b8 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html
@@ -540,5 +540,40 @@
assert.equal(messages.length, 5);
assert.isFalse(element._hasAutomatedMessages(messages));
});
+
+ test('_computeLabelExtremes', () => {
+ const computeSpy = sandbox.spy(element, '_computeLabelExtremes');
+
+ element.labels = null;
+ assert.isTrue(computeSpy.calledOnce);
+ assert.deepEqual(computeSpy.lastCall.returnValue, {});
+
+ element.labels = {};
+ assert.isTrue(computeSpy.calledTwice);
+ assert.deepEqual(computeSpy.lastCall.returnValue, {});
+
+ element.labels = {'my-label': {}};
+ assert.isTrue(computeSpy.calledThrice);
+ assert.deepEqual(computeSpy.lastCall.returnValue, {});
+
+ element.labels = {'my-label': {values: {}}};
+ assert.equal(computeSpy.callCount, 4);
+ assert.deepEqual(computeSpy.lastCall.returnValue, {});
+
+ element.labels = {'my-label': {values: {'-12': {}}}};
+ assert.equal(computeSpy.callCount, 5);
+ assert.deepEqual(computeSpy.lastCall.returnValue,
+ {'my-label': {min: -12, max: -12}});
+
+ element.labels = {
+ 'my-label': {values: {'-12': {}}},
+ 'other-label': {values: {'-1': {}, ' 0': {}, '+1': {}}},
+ };
+ assert.equal(computeSpy.callCount, 6);
+ assert.deepEqual(computeSpy.lastCall.returnValue, {
+ 'my-label': {min: -12, max: -12},
+ 'other-label': {min: -1, max: 1},
+ });
+ });
});
</script>
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 2572c20..c1f6820 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
@@ -62,7 +62,7 @@
<paper-toggle-button
id="unresolvedToggle"
on-change="_toggleUnresolved"></paper-toggle-button>
- Only Unresolved threads</div>
+ Only unresolved threads</div>
<div class="toggleItem">
<paper-toggle-button
id="draftToggle"
@@ -73,7 +73,7 @@
<template is="dom-if" if="[[!threads.length]]">
There are no inline comment threads on any diff for this change.
</template>
- <template is="dom-repeat" items="[[threads]]" as="thread">
+ <template is="dom-repeat" items="[[_sortedThreads]]" as="thread">
<gr-diff-comment-thread
show-file-path
change-num="[[changeNum]]"
@@ -84,7 +84,9 @@
line-num="[[thread.line]]"
patch-num="[[thread.patchNum]]"
path="[[thread.path]]"
- root-id="[[thread.rootId]]"></gr-diff-comment-thread>
+ root-id="{{thread.rootId}}"
+ on-thread-changed="_handleCommentsChanged"
+ on-thread-discard="_handleThreadDiscard"></gr-diff-comment-thread>
</template>
</div>
</template>
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 73d7e31..ba7bc5c 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
@@ -14,19 +14,88 @@
(function() {
'use strict';
+ /**
+ * Fired when a comment is saved or deleted
+ *
+ * @event thread-list-modified
+ */
+
Polymer({
is: 'gr-thread-list',
properties: {
+ /** @type {?} */
change: Object,
threads: Array,
changeNum: String,
- ignoreChanges: {
- type: Boolean,
- value: false,
+ _sortedThreads: {
+ type: Array,
+ computed: '_computeSortedThreads(threads.*)',
},
},
+ /**
+ * 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 {!Object} changeRecord
+ * @return {!Array}
+ */
+ _computeSortedThreads(changeRecord) {
+ const threads = changeRecord.base;
+ if (!threads) { return []; }
+ return threads.map(this._getThreadWithSortInfo).sort((c1, c2) => {
+ const c1Date = c1.__date || util.parseDate(c1.updated);
+ const c2Date = c2.__date || util.parseDate(c2.updated);
+ const dateCompare = c2Date - c1Date;
+ if (c2.unresolved || c1.unresolved) {
+ if (!c1.unresolved) { return 1; }
+ if (!c2.unresolved) { return -1; }
+ }
+ if (c2.hasDraft || c1.hasDraft) {
+ if (!c1.hasDraft) { return 1; }
+ if (!c2.hasDraft) { return -1; }
+ }
+
+ if (dateCompare === 0 && (!c1.id || !c1.id.localeCompare)) {
+ return 0;
+ }
+ return dateCompare ? dateCompare : c1.id.localeCompare(c2.id);
+ }).map(threadInfo => threadInfo.thread);
+ },
+
+ _getThreadWithSortInfo(thread) {
+ const lastComment = thread.comments[thread.comments.length - 1] || {};
+ return {
+ thread,
+ unresolved: !!lastComment.unresolved,
+ hasDraft: !!lastComment.__draft,
+ updated: lastComment.updated,
+ };
+ },
+
+ removeThread(rootId) {
+ for (let i = 0; i < this.threads.length; i++) {
+ if (this.threads[i].rootId === rootId) {
+ this.splice('threads', i, 1);
+ // Needed to ensure threads get re-rendered in the correct order.
+ Polymer.dom.flush();
+ return;
+ }
+ }
+ },
+
+ _handleThreadDiscard(e) {
+ this.removeThread(e.detail.rootId);
+ },
+
+ _handleCommentsChanged(e) {
+ this.dispatchEvent(new CustomEvent('thread-list-modified',
+ {detail: {rootId: e.detail.rootId, path: e.detail.path}}));
+ },
+
_isOnParent(side) {
return !!side;
},
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 2993e98..c0be7d7 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
@@ -35,7 +35,7 @@
suite('gr-thread-list tests', () => {
let element;
let sandbox;
- let threadsElements;
+ let threadElements;
const computeVisibleNumber = threads => {
let count = 0;
for (const thread of threads) {
@@ -132,9 +132,53 @@
rootId: '8caddf38_44770ec1',
start_datetime: '2018-02-13 22:48:40.000000000',
},
+ {
+ comments: [
+ {
+ __path: '/COMMIT_MSG',
+ author: {
+ _account_id: 1000000,
+ name: 'user',
+ username: 'user',
+ },
+ patch_set: 2,
+ id: 'scaddf38_44770ec1',
+ line: 4,
+ updated: '2018-02-14 22:48:40.000000000',
+ message: 'Yet another unresolved comment',
+ unresolved: true,
+ },
+ ],
+ patchNum: 2,
+ path: '/COMMIT_MSG',
+ line: 4,
+ rootId: 'scaddf38_44770ec1',
+ start_datetime: '2018-02-14 22:48:40.000000000',
+ },
+ {
+ comments: [
+ {
+ id: 'zcf0b9fa_fe1a5f62',
+ path: '/COMMIT_MSG',
+ line: 6,
+ updated: '2018-02-15 22:48:48.018000000',
+ message: 'resolved draft',
+ unresolved: false,
+ __draft: true,
+ __draftID: '0.m683trwff68',
+ __editing: false,
+ patch_set: '2',
+ },
+ ],
+ patchNum: 4,
+ path: '/COMMIT_MSG',
+ line: 6,
+ rootId: 'zcf0b9fa_fe1a5f62',
+ start_datetime: '2018-02-09 18:49:18.000000000',
+ },
];
flushAsynchronousOperations();
- threadsElements = Polymer.dom(element.root)
+ threadElements = Polymer.dom(element.root)
.querySelectorAll('gr-diff-comment-thread');
});
@@ -142,20 +186,47 @@
sandbox.restore();
});
- test('there are three threads by default', () => {
- assert.equal(computeVisibleNumber(threadsElements), 3);
+ test('there are five threads by default', () => {
+ assert.equal(computeVisibleNumber(threadElements), 5);
+ });
+
+ test('_computeSortedThreads', () => {
+ assert.equal(element._sortedThreads.length, 5);
+ // Draft and unresolved
+ assert.equal(element._sortedThreads[0].rootId, 'ecf0b9fa_fe1a5f62');
+ // unresolved
+ assert.equal(element._sortedThreads[1].rootId, 'scaddf38_44770ec1');
+ // unresolved
+ assert.equal(element._sortedThreads[2].rootId, '8caddf38_44770ec1');
+ // resolved and draft
+ assert.equal(element._sortedThreads[3].rootId, 'zcf0b9fa_fe1a5f62');
+ // resolved
+ assert.equal(element._sortedThreads[4].rootId, '09a9fb0a_1484e6cf');
+ });
+
+ test('thread removal', () => {
+ threadElements[1].fire('thread-discard', {rootId: 'scaddf38_44770ec1'});
+ flushAsynchronousOperations();
+ assert.equal(element._sortedThreads.length, 4);
+ assert.equal(element._sortedThreads[0].rootId, 'ecf0b9fa_fe1a5f62');
+ // unresolved
+ assert.equal(element._sortedThreads[1].rootId, '8caddf38_44770ec1');
+ // resolved and draft
+ assert.equal(element._sortedThreads[2].rootId, 'zcf0b9fa_fe1a5f62');
+ // resolved
+ assert.equal(element._sortedThreads[3].rootId, '09a9fb0a_1484e6cf');
});
test('toggle unresolved only shows unressolved comments', () => {
MockInteractions.tap(element.$.unresolvedToggle);
flushAsynchronousOperations();
- assert.equal(computeVisibleNumber(threadsElements), 2);
+ assert.equal(computeVisibleNumber(threadElements), 3);
});
test('toggle drafts only shows threads with draft comments', () => {
MockInteractions.tap(element.$.draftToggle);
flushAsynchronousOperations();
- assert.equal(computeVisibleNumber(threadsElements), 1);
+ assert.equal(computeVisibleNumber(threadElements), 2);
});
test('toggle drafts and unresolved only shows threads with drafts and ' +
@@ -163,7 +234,20 @@
MockInteractions.tap(element.$.draftToggle);
MockInteractions.tap(element.$.unresolvedToggle);
flushAsynchronousOperations();
- assert.equal(computeVisibleNumber(threadsElements), 1);
+ assert.equal(computeVisibleNumber(threadElements), 1);
+ });
+
+ test('modification events are consumed and displatched', () => {
+ sandbox.spy(element, '_handleCommentsChanged');
+ const dispatchSpy = sandbox.stub();
+ element.addEventListener('thread-list-modified', dispatchSpy);
+ threadElements[0].fire('thread-changed', {
+ rootId: 'ecf0b9fa_fe1a5f62', path: '/COMMIT_MSG'});
+ assert.isTrue(element._handleCommentsChanged.called);
+ assert.isTrue(dispatchSpy.called);
+ assert.equal(dispatchSpy.lastCall.args[0].detail.rootId,
+ 'ecf0b9fa_fe1a5f62');
+ assert.equal(dispatchSpy.lastCall.args[0].detail.path, '/COMMIT_MSG');
});
});
-</script>
+</script>
\ No newline at end of file
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-thread/gr-diff-comment-thread.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
index cb2eebe..34e006f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
@@ -83,7 +83,8 @@
root-id="[[rootId]]"
project-config="[[_projectConfig]]"
on-create-fix-comment="_handleCommentFix"
- on-comment-discard="_handleCommentDiscard"></gr-diff-comment>
+ on-comment-discard="_handleCommentDiscard"
+ on-comment-save="_handleCommentSavedOrDiscarded"></gr-diff-comment>
</template>
<div id="commentInfoContainer"
hidden$="[[_hideActions(_showActions, _lastComment)]]">
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
index d0a1f9b..8f5821f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
@@ -26,6 +26,12 @@
* @event thread-discard
*/
+ /**
+ * Fired when a comment in the thread is permanently modified.
+ *
+ * @event thread-changed
+ */
+
properties: {
changeNum: String,
comments: {
@@ -367,7 +373,9 @@
},
_computeRootId(comments) {
- if (!comments.base.length) { return null; }
+ // Keep the root ID even if the comment was removed, so that notification
+ // to sync will know which thread to remove.
+ if (!comments.base.length) { return this.rootId; }
const rootComment = comments.base[0];
return rootComment.id || rootComment.__draftID;
},
@@ -384,6 +392,7 @@
if (this.comments.length === 0) {
this.fireRemoveSelf();
}
+ this._handleCommentSavedOrDiscarded(e);
// Check to see if there are any other open comments getting edited and
// set the local storage value to its message value.
@@ -401,6 +410,12 @@
}
},
+ _handleCommentSavedOrDiscarded(e) {
+ this.dispatchEvent(new CustomEvent('thread-changed',
+ {detail: {rootId: this.rootId, path: this.path},
+ bubbles: false}));
+ },
+
_handleCommentUpdate(e) {
const comment = e.detail.comment;
const index = this._indexOf(comment, this.comments);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
index de6c3b01..5eb8b94 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
@@ -203,8 +203,10 @@
suite('comment action tests', () => {
let element;
+ let sandbox;
setup(() => {
+ sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
getLoggedIn() { return Promise.resolve(false); },
saveDiffDraft() {
@@ -235,10 +237,15 @@
line: 5,
message: 'is this a crossover episode!?',
updated: '2015-12-08 19:48:33.843000000',
+ path: '/path/to/file.txt',
}];
flushAsynchronousOperations();
});
+ teardown(() => {
+ sandbox.restore();
+ });
+
test('reply', done => {
const commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
@@ -344,6 +351,28 @@
});
});
+ test('save', done => {
+ element.changeNum = '42';
+ element.patchNum = '1';
+ element.path = '/path/to/file.txt';
+ const commentEl = element.$$('gr-diff-comment');
+ assert.ok(commentEl);
+
+ const saveOrDiscardStub = sandbox.stub();
+ element.addEventListener('thread-changed', saveOrDiscardStub);
+ element.$$('gr-diff-comment')._fireSave();
+
+ flush(() => {
+ assert.isTrue(saveOrDiscardStub.called);
+ assert.equal(saveOrDiscardStub.lastCall.args[0].detail.rootId,
+ 'baf0414d_60047215');
+ assert.equal(element.rootId, 'baf0414d_60047215');
+ assert.equal(saveOrDiscardStub.lastCall.args[0].detail.path,
+ '/path/to/file.txt');
+ done();
+ });
+ });
+
test('please fix', done => {
element.changeNum = '42';
element.patchNum = '1';
@@ -367,6 +396,7 @@
test('discard', done => {
element.changeNum = '42';
element.patchNum = '1';
+ element.path = '/path/to/file.txt';
element.push('comments', element._newReply(
element.comments[0].id,
element.comments[0].line,
@@ -374,6 +404,8 @@
'it’s pronouced jiff, not giff'));
flushAsynchronousOperations();
+ const saveOrDiscardStub = sandbox.stub();
+ element.addEventListener('thread-changed', saveOrDiscardStub);
const draftEl =
Polymer.dom(element.root).querySelectorAll('gr-diff-comment')[1];
assert.ok(draftEl);
@@ -382,11 +414,46 @@
return c.__draft == true;
});
assert.equal(drafts.length, 0);
+ assert.isTrue(saveOrDiscardStub.called);
+ assert.equal(saveOrDiscardStub.lastCall.args[0].detail.rootId,
+ element.rootId);
+ assert.equal(saveOrDiscardStub.lastCall.args[0].detail.path,
+ element.path);
done();
});
- draftEl.fire('comment-discard', null, {bubbles: false});
+ draftEl.fire('comment-discard', {comment: draftEl.comment},
+ {bubbles: false});
});
+ test('discard with a single comment still fires event with previous rootId',
+ done => {
+ element.changeNum = '42';
+ element.patchNum = '1';
+ element.path = '/path/to/file.txt';
+ element.comments = [];
+ element.addOrEditDraft('1');
+ flushAsynchronousOperations();
+ const rootId = element.rootId;
+ assert.isOk(rootId);
+
+ const saveOrDiscardStub = sandbox.stub();
+ element.addEventListener('thread-changed', saveOrDiscardStub);
+ const draftEl =
+ Polymer.dom(element.root).querySelectorAll('gr-diff-comment')[0];
+ assert.ok(draftEl);
+ draftEl.addEventListener('comment-discard', () => {
+ assert.equal(element.comments.length, 0);
+ assert.isTrue(saveOrDiscardStub.called);
+ assert.equal(saveOrDiscardStub.lastCall.args[0].detail.rootId,
+ rootId);
+ assert.equal(saveOrDiscardStub.lastCall.args[0].detail.path,
+ element.path);
+ done();
+ });
+ draftEl.fire('comment-discard', {comment: draftEl.comment},
+ {bubbles: false});
+ });
+
test('first editing comment does not add __otherEditing attribute', () => {
element.comments = [{
author: {
@@ -461,7 +528,8 @@
storageStub.restore();
done();
});
- draftEl.fire('comment-discard', null, {bubbles: false});
+ draftEl.fire('comment-discard', {comment: draftEl.comment},
+ {bubbles: false});
});
test('comment-update', () => {
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 514bc80..e1ebbf2 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
@@ -290,7 +290,8 @@
<div class="action resolve hideOnPublished">
<label>
<input type="checkbox"
- checked$="[[resolved]]"
+ id="resolvedCheckbox"
+ checked="[[resolved]]"
on-change="_handleToggleResolved">
Resolved
</label>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
index e45702c..c48f7ea 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -113,10 +113,7 @@
},
commentSide: String,
- resolved: {
- type: Boolean,
- observer: '_toggleResolved',
- },
+ resolved: Boolean,
_numPendingDraftRequests: {
type: Object,
@@ -581,17 +578,10 @@
_handleToggleResolved() {
this.resolved = !this.resolved;
- },
-
- _toggleResolved(resolved, previousValue) {
- // Do not proceed if this call is for the initial definition of the
- // resolved property.
- if (previousValue === undefined) { return; }
-
// Modify payload instead of this.comment, as this.comment is passed from
// the parent by ref.
const payload = this._getEventPayload();
- payload.comment.unresolved = !resolved;
+ payload.comment.unresolved = !this.$.resolvedCheckbox.checked;
this.fire('comment-update', payload);
if (!this.editing) {
// Save the resolved state immediately.
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 c09e289..8c78946 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
@@ -636,7 +636,9 @@
test('draft prevent save when disabled', () => {
const saveStub = sandbox.stub(element, 'save');
+ element.showActions = true;
element.draft = true;
+ MockInteractions.tap(element.$.header);
MockInteractions.tap(element.$$('.edit'));
element._messageText = 'good news, everyone!';
element.flushDebouncer('fire-update');
@@ -680,7 +682,7 @@
assert.isFalse(element.$$('.resolve input').checked);
});
- test('resolved checkbox saves when !editing', () => {
+ test('resolved checkbox saves with tap when !editing', () => {
element.editing = false;
const save = sandbox.stub(element, 'save');
@@ -688,6 +690,9 @@
assert.isTrue(element.$$('.resolve input').checked);
element.comment = {unresolved: true};
assert.isFalse(element.$$('.resolve input').checked);
+ assert.isFalse(save.called);
+ MockInteractions.tap(element.$.resolvedCheckbox);
+ assert.isTrue(element.$$('.resolve input').checked);
assert.isTrue(save.called);
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index 5b85c20..b1b0670 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -540,6 +540,12 @@
return {path: fileList[idx]};
},
+ _getReviewedStatus(editMode, changeNum, patchNum, path) {
+ if (editMode) { return Promise.resolve(false); }
+ return this.$.restAPI.getReviewedFiles(changeNum, patchNum)
+ .then(files => files.includes(path));
+ },
+
_paramsChanged(value) {
if (value.view !== Gerrit.Nav.View.DIFF) { return; }
@@ -631,7 +637,17 @@
_setReviewedObserver(_loggedIn, paramsRecord, _prefs) {
const params = paramsRecord.base || {};
- if (!_loggedIn || _prefs.manual_review) { return; }
+ if (!_loggedIn) { return; }
+
+ if (_prefs.manual_review) {
+ // Checkbox state needs to be set explicitly only when manual_review
+ // is specified.
+ this._getReviewedStatus(this.editMode, this._changeNum,
+ this._patchRange.patchNum, this._path).then(status => {
+ this.$.reviewed.checked = status;
+ });
+ return;
+ }
if (params.view === Gerrit.Nav.View.DIFF) {
this._setReviewed(true);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index 7c94a00..0e9c34b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -60,6 +60,7 @@
getDiffComments() { return Promise.resolve({}); },
getDiffRobotComments() { return Promise.resolve({}); },
getDiffDrafts() { return Promise.resolve({}); },
+ getReviewedFiles() { return Promise.resolve([]); },
});
element = fixture('basic');
return element._loadComments();
@@ -521,8 +522,10 @@
test('_prefs.manual_review is respected', () => {
const saveReviewedStub = sandbox.stub(element, '_saveReviewedState',
() => Promise.resolve());
- sandbox.stub(element.$.diff, 'reload');
+ const getReviewedStub = sandbox.stub(element, '_getReviewedStatus',
+ () => Promise.resolve());
+ sandbox.stub(element.$.diff, 'reload');
element._loggedIn = true;
element.params = {
view: Gerrit.Nav.View.DIFF,
@@ -535,10 +538,13 @@
flushAsynchronousOperations();
assert.isFalse(saveReviewedStub.called);
+ assert.isTrue(getReviewedStub.called);
+
element._prefs = {};
flushAsynchronousOperations();
assert.isTrue(saveReviewedStub.called);
+ assert.isTrue(getReviewedStub.calledOnce);
});
test('file review status', () => {
@@ -994,6 +1000,25 @@
[{value: '/foo'}, {value: '/bar'}]), 'show');
});
+ test('_getReviewedStatus', () => {
+ const promises = [];
+ element.$.restAPI.getReviewedFiles.restore();
+
+ sandbox.stub(element.$.restAPI, 'getReviewedFiles')
+ .returns(Promise.resolve(['path']));
+
+ promises.push(element._getReviewedStatus(true, null, null, 'path')
+ .then(reviewed => assert.isFalse(reviewed)));
+
+ promises.push(element._getReviewedStatus(false, null, null, 'otherPath')
+ .then(reviewed => assert.isFalse(reviewed)));
+
+ promises.push(element._getReviewedStatus(false, null, null, 'path')
+ .then(reviewed => assert.isTrue(reviewed)));
+
+ return Promise.all(promises);
+ });
+
suite('editMode behavior', () => {
setup(() => {
element._loggedIn = true;
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 65544d4..079600c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -158,8 +158,9 @@
content: '\A';
}
.contextControl {
- background-color: #fef;
- color: #849;
+ background-color: #fff7d4;
+ border: 1px solid #f6e6a5;
+ color: rgba(0,0,0.54);
}
.contextControl gr-button {
display: inline-block;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index c148a10..19b3c6d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -48,6 +48,12 @@
* @event show-auth-required
*/
+ /**
+ * Fired when a comment is saved or discarded
+ *
+ * @event diff-comments-modified
+ */
+
properties: {
changeNum: String,
noAutoRender: {
@@ -260,6 +266,11 @@
this.classList.remove('showBlame');
},
+ _handleCommentSaveOrDiscard() {
+ this.dispatchEvent(new CustomEvent('diff-comments-modified',
+ {bubbles: true}));
+ },
+
/** @return {boolean}} */
_canRender() {
return !!this.changeNum && !!this.patchRange && !!this.path &&
@@ -511,6 +522,7 @@
_handleCommentDiscard(e) {
const comment = e.detail.comment;
this._removeComment(comment);
+ this._handleCommentSaveOrDiscard();
},
_removeComment(comment) {
@@ -523,6 +535,7 @@
const side = e.detail.comment.__commentSide;
const idx = this._findDraftIndex(comment, side);
this.set(['comments', side, idx], comment);
+ this._handleCommentSaveOrDiscard();
},
/**
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 1f06dd5..7526141 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -984,6 +984,28 @@
assert.include(element.comments.left, comment);
});
+ test('discarding a draft', () => {
+ const draftID = 'tempID';
+ const id = 'savedID';
+ const comment = {
+ __draft: true,
+ __draftID: draftID,
+ side: 'PARENT',
+ __commentSide: 'left',
+ };
+ const diffCommentsModifiedStub = sandbox.stub();
+ element.addEventListener('diff-comments-modified',
+ diffCommentsModifiedStub);
+ element.comments.left.push(comment);
+ comment.id = id;
+ element.fire('comment-discard', {comment});
+ const drafts = element.comments.left.filter(item => {
+ return item.__draftID === draftID;
+ });
+ assert.equal(drafts.length, 0);
+ assert.isTrue(diffCommentsModifiedStub.called);
+ });
+
test('saving a draft', () => {
const draftID = 'tempID';
const id = 'savedID';
@@ -993,14 +1015,18 @@
side: 'PARENT',
__commentSide: 'left',
};
+ const diffCommentsModifiedStub = sandbox.stub();
+ element.addEventListener('diff-comments-modified',
+ diffCommentsModifiedStub);
element.comments.left.push(comment);
comment.id = id;
- element.fire('comment-update', {comment});
+ element.fire('comment-save', {comment});
const drafts = element.comments.left.filter(item => {
return item.__draftID === draftID;
});
assert.equal(drafts.length, 1);
assert.equal(drafts[0].id, id);
+ assert.isTrue(diffCommentsModifiedStub.called);
});
});
});
diff --git a/polygerrit-ui/app/elements/gr-app.html b/polygerrit-ui/app/elements/gr-app.html
index 2f2aa51..3879495 100644
--- a/polygerrit-ui/app/elements/gr-app.html
+++ b/polygerrit-ui/app/elements/gr-app.html
@@ -79,13 +79,14 @@
border-bottom: 1px solid pink;
}
gr-main-header {
+ background-color: var(--header-background-color);
+ border-bottom: 1px solid #ddd;
/* 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;
+ width: 100vw;
/* Needed for the menu dropdowns to display on top of the sticky
headers with z-index of 1.*/
z-index: 2;
diff --git a/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list.html b/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list.html
index bcae0bf..017f49f 100644
--- a/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list.html
+++ b/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list.html
@@ -15,9 +15,10 @@
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
-<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../../styles/shared-styles.html">
<link rel="import" href="../../../styles/gr-form-styles.html">
+<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
+<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<dom-module id="gr-group-list">
<template>
@@ -47,7 +48,11 @@
<tbody>
<template is="dom-repeat" items="[[_groups]]">
<tr>
- <td class="nameColumn">[[item.name]]</td>
+ <td class="nameColumn">
+ <a href$="[[_computeGroupPath(item)]]">
+ [[item.name]]
+ </a>
+ </td>
<td>[[item.description]]</td>
<td class="visibleCell">[[_computeVisibleToAll(item)]]</td>
</tr>
diff --git a/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list.js b/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list.js
index d26482d..8ca8598 100644
--- a/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list.js
+++ b/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list.js
@@ -32,5 +32,11 @@
_computeVisibleToAll(group) {
return group.options.visible_to_all ? 'Yes' : 'No';
},
+
+ _computeGroupPath(group) {
+ if (!group || !group.id) { return; }
+
+ return Gerrit.Nav.getUrlForGroup(group.id);
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list_test.html b/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list_test.html
index 3c4eff2..812b186 100644
--- a/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list_test.html
+++ b/polygerrit-ui/app/elements/settings/gr-group-list/gr-group-list_test.html
@@ -33,10 +33,12 @@
<script>
suite('gr-group-list tests', () => {
+ let sandbox;
let element;
let groups;
setup(done => {
+ sandbox = sinon.sandbox.create();
groups = [{
url: 'some url',
options: {},
@@ -65,13 +67,15 @@
element.loadData().then(() => { flush(done); });
});
+ teardown(() => { sandbox.restore(); });
+
test('renders', () => {
const rows = Polymer.dom(element.root).querySelectorAll('tbody tr');
assert.equal(rows.length, 3);
const nameCells = rows.map(row =>
- row.querySelectorAll('td')[0].textContent
+ row.querySelectorAll('td a')[0].textContent.trim()
);
assert.equal(nameCells[0], 'Group 1');
@@ -83,5 +87,23 @@
assert.equal(element._computeVisibleToAll(groups[0]), 'No');
assert.equal(element._computeVisibleToAll(groups[1]), 'Yes');
});
+
+ test('_computeGroupPath', () => {
+ sandbox.stub(Gerrit.Nav, 'getUrlForGroup',
+ () => '/admin/groups/e2cd66f88a2db4d391ac068a92d987effbe872f5');
+
+ let group = {
+ id: 'e2cd66f88a2db4d391ac068a92d987effbe872f5',
+ };
+
+ assert.equal(element._computeGroupPath(group),
+ '/admin/groups/e2cd66f88a2db4d391ac068a92d987effbe872f5');
+
+ group = {
+ name: 'admin',
+ };
+
+ assert.isUndefined(element._computeGroupPath(group));
+ });
});
</script>
diff --git a/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html b/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
index cc35e08..bebb1e8 100644
--- a/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
+++ b/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
@@ -45,11 +45,10 @@
}
.text {
display: inline-block;
- max-width: calc(100vw - 2.5rem);
- overflow: hidden;
- text-overflow: ellipsis;
+ max-height: 10rem;
+ max-width: 80vw;
vertical-align: bottom;
- white-space: nowrap;
+ word-break: break-all;
}
.action {
color: #a1c2fa;
