Merge "Document `DiffWebLinkInfo` as being unused"
diff --git a/.bazelrc b/.bazelrc
index 407b005..cf5403d 100644
--- a/.bazelrc
+++ b/.bazelrc
@@ -37,6 +37,6 @@
build --announce_rc
test --build_tests_only
-test --test_output=all
+test --test_output=errors
import %workspace%/tools/remote-bazelrc
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 9e76efe..19445b8 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -744,6 +744,19 @@
[[cache]]
=== Section cache
+[[cache.threads]]cache.threads::
++
+Number of threads to use when running asynchronous cache tasks.
+The threads executor is delegated to when sending removal notifications to listeners,
+when asynchronous computations like refresh, refreshAfterWrite are performed, or when
+performing periodic maintenance.
++
+**NOTE**: Setting it to 0 disables the dedicated thread pool and indexing will be done in the
+same thread as the operation. This may result in evictions taking longer because the
+listeners are executed in the caller's thread.
++
+By default, the JVM common ForkJoinPool is used.
+
[[cache.directory]]cache.directory::
+
Path to a local directory where Gerrit can write cached entities for
diff --git a/Documentation/externalid-case-insensitivity.txt b/Documentation/externalid-case-insensitivity.txt
index b4e8140..57f492c 100644
--- a/Documentation/externalid-case-insensitivity.txt
+++ b/Documentation/externalid-case-insensitivity.txt
@@ -1,29 +1,30 @@
:linkattrs:
= Gerrit Code Review - ExternalId case insensitivity
-Gerrit usernames are case insensitive by default: e.g. johndoe and JohnDoe
-represents the same account. However, for installations older than v3.5.x,
-the usernames were case sensitive, e.g. johndoe and JohnDoe can both exist
+Gerrit usernames are case insensitive by default: e.g. `johndoe` and `JohnDoe`
+represent the same account. However, for installations older than v3.5.x,
+the usernames were case sensitive, e.g. `johndoe` and `JohnDoe` can both exist
as separate accounts. This could lead to issues when migrating an account
-from LDAP to an internal account, if ldap.localUsernameToLowerCase was set.
-Such usernames can also be rather confusing for users, if they try to identify
-authors of comments or changes.
+from LDAP to an internal account, if
+xref:config-gerrit.txt#ldap.localUsernameToLowerCase[ldap.localUsernameToLowerCase]
+was set. Such usernames can also be rather confusing for users, if they try to
+identify authors of comments or changes.
When Gerrit handles case insensitive usernames (external IDs using the
-`gerrit:` or `username:` scheme, their external IDs SHA-1 is always computed
+`gerrit:` or `username:` scheme), their external IDs SHA-1 is always computed
using the lowercase external ID, hence there cannot be any account differing
only in the capitalization of their usernames.
Gerrit installations older than v3.5.x that are switching to the case-insensitive
-username need to migrating all their existing accounts SHA-1s.
+username need to migrate all their existing accounts SHA-1s.
[[migration]]
== Migration
Migrating external ID notes can take several minutes for large sites(for example
-migration ~45000 accounts can take up to five minutes), so administrators choose
-whether to do the migration offline or online, depending on their available
-resources and tolerance for downtime.
+migration ++~++45000 accounts can take up to five minutes), so administrators
+choose whether to do the migration offline or online, depending on their
+available resources and tolerance for downtime.
NOTE: Migration is required only on Gerrit primary instances.
@@ -31,8 +32,10 @@
=== Offline
To run the offline migration execute following steps:
+
* Stop all Gerrit primary instances
* Set the `auth.userNameCaseInsensitive` to false
+
----
[auth]
userNameCaseInsensitive = false
@@ -46,7 +49,7 @@
[--batch]
--
-See: link:pgm-ChangeExternalIdCaseSensitivity.html
+See: link:pgm-ChangeExternalIdCaseSensitivity.html[]
* During the migration `auth.userNameCaseInsensitive` will be set to true
on a node which is executing the migration. When the migration is finished,
@@ -69,13 +72,14 @@
$ ssh -p <port> <host> gerrit migrate-externalids-to-insensitive
----
-See: link:cmd-migrate-externalids-to-insensitive.html
+See: link:cmd-migrate-externalids-to-insensitive.html[]
[online-ha-migration]
== Online migration for high-availability setup
To start the online migration with a setup containing multiple primary
instances execute following steps:
+
* On all Gerrit primary instances set `auth.userNameCaseInsensitive` and
`auth.userNameCaseInsensitiveMigrationMode` and perform a rolling restart
----
@@ -88,7 +92,7 @@
$ ssh -p <port> <host> gerrit migrate-externalids-to-insensitive
----
-See: link:cmd-migrate-externalids-to-insensitive.html
+See: link:cmd-migrate-externalids-to-insensitive.html[]
* When the migration is finished, on all other primary nodes set
`auth.userNameCaseInsensitiveMigrationMode` to false and perform a
@@ -105,6 +109,7 @@
from the case sensitive external ID.
To rollback external ID notes migration execute following steps:
+
* Stop all Gerrit primary instances
* Set the `auth.userNameCaseInsensitive` to true
----
@@ -120,7 +125,7 @@
[--batch]
--
-See: link:pgm-ChangeExternalIdCaseSensitivity.html
+See: link:pgm-ChangeExternalIdCaseSensitivity.html[]
* During the migration `auth.userNameCaseInsensitive` will be set to false
on a node which is executing the migration. When the migration is finished,
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt
index 7a042ba..4642247 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -874,8 +874,8 @@
- [[show-change-number]]`Show Change Number In Changes Table`:
+
-Whether in change lists and dashboards an `ID` column with the numeric
-change IDs should be shown.
+Whether in change lists and dashboards an `ID` column with the change numbers
+should be shown.
- [[mute-common-path-prefixes]]`Mute Common Path Prefixes In File List`:
+
diff --git a/Documentation/pg-plugin-dev.txt b/Documentation/pg-plugin-dev.txt
index d7343c2..7c93cc0 100644
--- a/Documentation/pg-plugin-dev.txt
+++ b/Documentation/pg-plugin-dev.txt
@@ -30,6 +30,33 @@
});
```
+== TypeScript API ==
+
+Gerrit provides a TypeScript plugin API.
+
+For a plugin built inline, its `tsconfig.json` can extends Gerrit plugin
+TypeScript configuration:
+
+`tsconfig.json`:
+``` json
+{
+ "extends": "../tsconfig-plugins-base.json"
+}
+```
+
+For standalone plugins (outside of a Gerrit tree), a TypeScript plugin API is
+published:
+link:https://www.npmjs.com/package/@gerritcodereview/typescript-api[@gerritcodereview/typescript-api].
+It provides a TypeScript configuration `tsconfig-plugins-base.json` which can
+be used in your plugin `tsconfig.json`:
+
+``` json
+{
+ "extends": "node_modules/@gerritcodereview/typescript-api/tsconfig-plugins-base.json",
+ // your custom configuration and overrides
+}
+```
+
[[low-level-api-concepts]]
== Low-level DOM API concepts
diff --git a/Documentation/pgm-SwitchSecureStore.txt b/Documentation/pgm-SwitchSecureStore.txt
index 47de1be..818ce2b 100644
--- a/Documentation/pgm-SwitchSecureStore.txt
+++ b/Documentation/pgm-SwitchSecureStore.txt
@@ -7,7 +7,7 @@
[verse]
--
_java_ -jar gerrit.war _SwitchSecureStore_
- [--new-secure-store-lib]
+ [--new-secure-store-lib=<PATH_TO_JAR>]
--
== DESCRIPTION
diff --git a/Documentation/pgm-init.txt b/Documentation/pgm-init.txt
index f6c3c85..3c9e3fc 100644
--- a/Documentation/pgm-init.txt
+++ b/Documentation/pgm-init.txt
@@ -16,10 +16,11 @@
[--list-plugins]
[--install-plugin=<PLUGIN_NAME>]
[--install-all-plugins]
- [--secure-store-lib]
+ [--secure-store-lib=<PATH_TO_JAR>]
[--dev]
[--skip-all-downloads]
[--skip-download=<LIBRARY_NAME>]
+ [--reindex-threads=<N>]
--
== DESCRIPTION
@@ -99,6 +100,14 @@
The administrator must manually install the required library in the `lib/`
folder.
+--show-cache-stats::
+ Show cache statistics at the end of program.
+
+--reindex-threads::
+ Number of threads to use for reindex after init. Defaults to 1. Can be
+ set to -1 to skip reindex after init. Skipping reindex will also not
+ automatically start the daemon.
+
== CONTEXT
This command can only be run on a server which has direct local access to the
managed Git repositories.
diff --git a/Documentation/pgm-reindex.txt b/Documentation/pgm-reindex.txt
index 0653d8d..b74829d 100644
--- a/Documentation/pgm-reindex.txt
+++ b/Documentation/pgm-reindex.txt
@@ -36,9 +36,8 @@
Reindex only index with given name. This option can be supplied
more than once to reindex multiple indices.
---disable-cache-stats::
- Disables printing cache statistics at the end of program to reduce
- noise. Defaulted when reindex is run from init on a new site.
+--show-cache-stats::
+ Show cache statistics at the end of program.
== CONTEXT
The secondary index must be enabled. See
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index aceb38e..63c429e 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6559,16 +6559,25 @@
[[change-id]]
=== \{change-id\}
Identifier that uniquely identifies one change. It contains the URL-encoded
-project name as well as the change number: "'$$<project>~<numericId>$$'"
+project name as well as the change number: "'$$<project>~<changeNumber>$$'"
-Gerrit also supports the following identifiers:
+==== Alternative identifiers
+Gerrit also supports an array of other change identifiers.
+
+[NOTE]
+Even though these identifiers will work in the majority of cases it is highly
+recommended to use "'$$<project>~<changeNumber>$$'" whenever possible.
+Since these identifiers require additional lookups from index and caches, to
+be translated to the "'$$<project>~<changeNumber>$$'" identifier, they
+may result in both false-positives and false-negatives.
+Furthermore the additional lookup mean that they come with a performance penalty.
* an ID of the change in the format "'$$<project>~<branch>~<Change-Id>$$'",
where for the branch the `refs/heads/` prefix can be omitted
("$$myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940$$")
* a Change-Id if it uniquely identifies one change
("I8473b95934b5732ac55d26311a706c9c2bde9940")
-* a numeric change ID ("4247")
+* a change number if it uniquely identifies one change ("4247")
[[change-message-id]]
=== \{change-message-id\}
@@ -6932,8 +6941,8 @@
Number of unresolved inline comment threads across all patch sets. Not set if
the current change index doesn't have the data.
|`_number` ||
-The numeric ID of the change. (The underscore is just a relict of a prior
-attempt to deprecate the numeric ID.)
+The change number. (The underscore is just a relict of a prior
+attempt to deprecate the change number.)
|`owner` ||
The owner of the change as an link:rest-api-accounts.html#account-info[
AccountInfo] entity.
@@ -7026,15 +7035,15 @@
|`has_review_started` |optional, not set if `false`|
When present, change has been marked Ready at some point in time.
|`revert_of` |optional|
-The numeric Change-Id of the change that this change reverts.
+The change number of the change that this change reverts.
|`submission_id` |optional|
ID of the submission of this change. Only set if the status is `MERGED`.
-This ID is equal to the numeric ID of the change that triggered the submission.
-If the change that triggered the submission also has a topic, it will be
-"<id>-<topic>" of the change that triggered the submission.
+This ID is equal to the change number of the change that triggered the
+submission. If the change that triggered the submission also has a topic,
+it will be "<id>-<topic>" of the change that triggered the submission.
The callers must not rely on the format of the submission ID.
|`cherry_pick_of_change` |optional|
-The numeric Change-Id of the change that this change was cherry-picked from.
+The change number of the change that this change was cherry-picked from.
Only set if the cherry-pick has been done through the Gerrit REST API (and
not if a cherry-picked commit was pushed).
|`cherry_pick_of_patch_set`|optional|
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index 9f3ef6d..6328dfd 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -1875,7 +1875,7 @@
[[index-changes-input]]
=== IndexChangesInput
-The `IndexChangesInput` contains a list of numerical changes IDs to index.
+The `IndexChangesInput` contains a list of change numbers of changes to index.
[options="header",cols="1,^2,4"]
|================================
diff --git a/Documentation/user-notify.txt b/Documentation/user-notify.txt
index d2a22a7..f420fe7 100644
--- a/Documentation/user-notify.txt
+++ b/Documentation/user-notify.txt
@@ -189,7 +189,7 @@
[[Gerrit-Change-Number]]Gerrit-Change-Number::
-The change number footer states the numeric ID of the change, for
+The change number footer states the change number of the change, for
example `92191`.
[[Gerrit-PatchSet]]Gerrit-PatchSet::
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index 32cb639..44921ad 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -30,13 +30,13 @@
[options="header"]
|=============================================================
|Description | Examples
-|Legacy numerical id | 15183
+|Change Id | 15183
|Full or abbreviated Change-Id | Ic0ff33
|Full or abbreviated commit SHA-1 | d81b32ef
|Email address | user@example.com
|=============================================================
-For change searches (i.e. those using a numerical id, Change-Id, or commit
+For change searches (i.e. those using a change number, Change-Id, or commit
SHA-1), if the search results in a single change that change will be
presented instead of a list.
@@ -119,15 +119,13 @@
[[change]]
change:'ID'::
+
-Either a legacy numerical 'ID' such as 15183, or a newer style
-Change-Id that was scraped out of the commit message.
+Either a change number such as 15183, or a Change-Id from the Change-Id footer.
[[conflicts]]
conflicts:'ID'::
+
Changes that conflict with change 'ID'. Change 'ID' can be specified
-as a legacy numerical 'ID' such as 15183, or a newer style Change-Id
-that was scraped out of the commit message.
+as a change number such as 15183, or a Change-Id from the Change-Id footer.
[[destination]]
destination:'[name=]NAME[,user=USER]'::
@@ -185,7 +183,7 @@
[[revertof]]
revertof:'ID'::
+
-Changes that revert the change specified by the numeric 'ID'.
+Changes that revert the change specified by the change number.
[[submissionid]]
submissionid:'ID'::
@@ -218,8 +216,8 @@
[[parentof]]
parentof:'ID'::
Changes which are parent to the change specified by 'ID'. Change 'ID' can be
-specified as a legacy numerical 'ID' such as 15183, or a Change-Id that can be
-picked from the commit message. This operator will return immediate parents
+specified as a change number such as 15183, or a Change-Id from the 'Change-Id'
+footer of the commit message. This operator will return immediate parents
and will not return grand parents or higher level ancestors of the given change.
[[parentproject]]
diff --git a/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java b/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java
index 4270150..e909701 100644
--- a/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java
+++ b/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java
@@ -32,6 +32,7 @@
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
import com.google.common.base.Splitter;
import com.google.common.flogger.FluentLogger;
@@ -639,20 +640,39 @@
return env.getEnvArray();
}
- private String getProjectRoot(Project.NameKey nameKey)
- throws RepositoryNotFoundException, IOException {
+ /**
+ * Return the project root under which the specified project is stored.
+ *
+ * @param nameKey the name of the project
+ * @return base directory
+ */
+ @VisibleForTesting
+ String getProjectRoot(Project.NameKey nameKey) throws RepositoryNotFoundException, IOException {
try (Repository repo = repoManager.openRepository(nameKey)) {
- return getProjectRoot(repo);
+ return getRepositoryRoot(repo, nameKey).toString();
}
}
- private String getProjectRoot(Repository repo) {
+ /**
+ * Return the repository root under which the specified repository is stored.
+ *
+ * @param repo the name of the repository
+ * @param nameKey project name
+ * @return base path
+ * @throws ProvisionException if the repo is not DelegateRepository or FileRepository.
+ */
+ private static Path getRepositoryRoot(Repository repo, Project.NameKey nameKey) {
if (repo instanceof DelegateRepository) {
- return getProjectRoot(((DelegateRepository) repo).delegate());
+ return getRepositoryRoot(((DelegateRepository) repo).delegate(), nameKey);
}
if (repo instanceof FileRepository) {
- return repo.getDirectory().getAbsolutePath();
+ String name = nameKey.get();
+ Path current = repo.getDirectory().toPath();
+ for (int i = 0; i <= CharMatcher.is('/').countIn(name); i++) {
+ current = current.getParent();
+ }
+ return current;
}
throw new ProvisionException("Gitweb can only be used with FileRepository");
diff --git a/java/com/google/gerrit/pgm/Init.java b/java/com/google/gerrit/pgm/Init.java
index 4c7b47b..c05bff5 100644
--- a/java/com/google/gerrit/pgm/Init.java
+++ b/java/com/google/gerrit/pgm/Init.java
@@ -95,6 +95,9 @@
@Option(name = "--reindex-threads", usage = "Number of threads to use for reindex after init")
private int reindexThreads = 1;
+ @Option(name = "--show-cache-stats", usage = "Show cache statistics at the end")
+ private boolean showCacheStats;
+
@Inject Browser browser;
private GerritIndexStatus indexStatus;
@@ -160,14 +163,14 @@
});
modules.add(new GerritServerConfigModule());
Guice.createInjector(modules).injectMembers(this);
- if (!ReplicaUtil.isReplica(run.flags.cfg)) {
+ if (reindexThreads != -1 && !ReplicaUtil.isReplica(run.flags.cfg)) {
List<String> indicesToReindex = new ArrayList<>();
for (SchemaDefinitions<?> schemaDef : schemaDefs) {
if (!indexStatus.exists(schemaDef.getName())) {
indicesToReindex.add(schemaDef.getName());
}
}
- reindex(indicesToReindex, run.flags.isNew);
+ reindex(indicesToReindex);
}
start(run);
}
@@ -223,7 +226,7 @@
}
void start(SiteRun run) throws Exception {
- if (run.flags.autoStart) {
+ if (reindexThreads != -1 && run.flags.autoStart) {
if (HostPlatform.isWin32()) {
System.err.println("Automatic startup not supported on Win32.");
} else {
@@ -280,7 +283,7 @@
}
}
- private void reindex(List<String> indices, boolean isNewSite) throws Exception {
+ private void reindex(List<String> indices) throws Exception {
if (indices.isEmpty()) {
return;
}
@@ -291,8 +294,8 @@
reindexArgs.add("--index");
reindexArgs.add(index);
}
- if (isNewSite) {
- reindexArgs.add("--disable-cache-stats");
+ if (showCacheStats) {
+ reindexArgs.add("--show-cache-stats");
}
getConsoleUI()
diff --git a/java/com/google/gerrit/pgm/Reindex.java b/java/com/google/gerrit/pgm/Reindex.java
index 762d988..feae284 100644
--- a/java/com/google/gerrit/pgm/Reindex.java
+++ b/java/com/google/gerrit/pgm/Reindex.java
@@ -87,12 +87,8 @@
@Option(name = "--index", usage = "Only reindex specified indices")
private List<String> indices = new ArrayList<>();
- @Option(
- name = "--disable-cache-stats",
- usage =
- "Disables printing the cache statistics."
- + "Defaults to true when reindex is run from init on a new site, false otherwise")
- private boolean disableCacheStats;
+ @Option(name = "--show-cache-stats", usage = "Show cache statistics at the end.")
+ private boolean showCacheStats;
private Injector dbInjector;
private Injector sysInjector;
@@ -124,7 +120,7 @@
try {
boolean ok = list ? list() : reindex();
- if (!disableCacheStats) {
+ if (showCacheStats) {
printCacheStats();
}
return ok ? 0 : 1;
diff --git a/java/com/google/gerrit/server/account/GroupBackend.java b/java/com/google/gerrit/server/account/GroupBackend.java
index 91edaf2..e02c27b 100644
--- a/java/com/google/gerrit/server/account/GroupBackend.java
+++ b/java/com/google/gerrit/server/account/GroupBackend.java
@@ -46,4 +46,28 @@
/** Returns {@code true} if the group with the given UUID is visible to all registered users. */
boolean isVisibleToAll(AccountGroup.UUID uuid);
+
+ default boolean isOrContainsExternalGroup(AccountGroup.UUID groupId) {
+ if (groupId != null) {
+ GroupDescription.Basic groupDescription = get(groupId);
+ if (!(groupDescription instanceof GroupDescription.Internal)
+ || containsExternalSubGroups((GroupDescription.Internal) groupDescription)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private boolean containsExternalSubGroups(GroupDescription.Internal internalGroup) {
+ for (AccountGroup.UUID subGroupUuid : internalGroup.getSubgroups()) {
+ GroupDescription.Basic subGroupDescription = get(subGroupUuid);
+ if (!(subGroupDescription instanceof GroupDescription.Internal)) {
+ return true;
+ }
+ if (containsExternalSubGroups((GroupDescription.Internal) subGroupDescription)) {
+ return true;
+ }
+ }
+ return false;
+ }
}
diff --git a/java/com/google/gerrit/server/cache/ForwardingRemovalListener.java b/java/com/google/gerrit/server/cache/ForwardingRemovalListener.java
index 28d57e6..852d8a3 100644
--- a/java/com/google/gerrit/server/cache/ForwardingRemovalListener.java
+++ b/java/com/google/gerrit/server/cache/ForwardingRemovalListener.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.cache;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.cache.RemovalListener;
import com.google.common.cache.RemovalNotification;
@@ -37,7 +38,8 @@
private String pluginName = PluginName.GERRIT;
@Inject
- ForwardingRemovalListener(
+ @VisibleForTesting
+ protected ForwardingRemovalListener(
PluginSetContext<CacheRemovalListener> listeners, @Assisted String cacheName) {
this.listeners = listeners;
this.cacheName = cacheName;
diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
index 28a2ede..a580f6d 100644
--- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
+++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
@@ -26,26 +26,44 @@
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.cache.RemovalNotification;
+import com.google.common.util.concurrent.MoreExecutors;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.server.cache.CacheDef;
import com.google.gerrit.server.cache.ForwardingRemovalListener;
import com.google.gerrit.server.cache.MemoryCacheFactory;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.WorkQueue;
import com.google.inject.Inject;
import java.time.Duration;
+import java.util.concurrent.Executor;
import org.eclipse.jgit.lib.Config;
class DefaultMemoryCacheFactory implements MemoryCacheFactory {
+ static final String CACHE_EXECUTOR_PREFIX = "InMemoryCacheExecutor";
+ private static final int DEFAULT_CACHE_EXECUTOR_THREADS = -1;
+
private final Config cfg;
private final ForwardingRemovalListener.Factory forwardingRemovalListenerFactory;
+ private int executorThreads;
+ private final Executor executor;
@Inject
DefaultMemoryCacheFactory(
@GerritServerConfig Config config,
- ForwardingRemovalListener.Factory forwardingRemovalListenerFactory) {
+ ForwardingRemovalListener.Factory forwardingRemovalListenerFactory,
+ WorkQueue workQueue) {
this.cfg = config;
this.forwardingRemovalListenerFactory = forwardingRemovalListenerFactory;
+ this.executorThreads = config.getInt("cache", "threads", DEFAULT_CACHE_EXECUTOR_THREADS);
+
+ if (executorThreads == 0) {
+ executor = MoreExecutors.newDirectExecutorService();
+ } else if (executorThreads > DEFAULT_CACHE_EXECUTOR_THREADS) {
+ executor = workQueue.createQueue(executorThreads, CACHE_EXECUTOR_PREFIX);
+ } else {
+ executor = null;
+ }
}
@Override
@@ -65,6 +83,10 @@
builder.recordStats();
builder.maximumWeight(cacheMaximumWeight(def));
builder = builder.removalListener(newRemovalListener(def.name()));
+
+ if (executor != null) {
+ builder.executor(executor);
+ }
builder.weigher(newWeigher(def.weigher()));
Duration expireAfterWrite = def.expireAfterWrite();
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index d25e022..7b8c0d2 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -1033,93 +1033,112 @@
Strings.nullToEmpty(magicBranchCmd.getMessage()));
return;
}
+ try {
+ retryHelper
+ .changeUpdate(
+ "insertChangesAndPatchSets",
+ updateFactory -> {
+ try (BatchUpdate bu =
+ batchUpdateFactory.create(
+ project.getNameKey(), user.materializedCopy(), TimeUtil.now());
+ ObjectInserter ins = repo.newObjectInserter();
+ ObjectReader reader = ins.newReader();
+ RevWalk rw = new RevWalk(reader)) {
+ bu.setRepository(repo, rw, ins);
+ bu.setRefLogMessage("push");
+ if (magicBranch != null) {
+ bu.setNotify(magicBranch.getNotifyForNewChange());
+ }
- try (BatchUpdate bu =
- batchUpdateFactory.create(
- project.getNameKey(), user.materializedCopy(), TimeUtil.now());
- ObjectInserter ins = repo.newObjectInserter();
- ObjectReader reader = ins.newReader();
- RevWalk rw = new RevWalk(reader)) {
- bu.setRepository(repo, rw, ins);
- bu.setRefLogMessage("push");
- if (magicBranch != null) {
- bu.setNotify(magicBranch.getNotifyForNewChange());
- }
+ logger.atFine().log("Adding %d replace requests", newChanges.size());
+ for (ReplaceRequest replace : replaceByChange.values()) {
+ replace.addOps(bu, replaceProgress);
+ if (magicBranch != null) {
+ bu.setNotifyHandling(
+ replace.ontoChange, magicBranch.getNotifyHandling(replace.notes));
+ if (magicBranch.shouldPublishComments()) {
+ bu.addOp(
+ replace.notes.getChangeId(),
+ publishCommentsOp.create(replace.psId, project.getNameKey()));
+ Optional<ChangeNotes> changeNotes =
+ getChangeNotes(replace.notes.getChangeId());
+ if (!changeNotes.isPresent()) {
+ // If not present, no need to update attention set here since this is
+ // a new change.
+ continue;
+ }
+ List<HumanComment> drafts =
+ commentsUtil.draftByChangeAuthor(
+ changeNotes.get(), user.getAccountId());
+ if (drafts.isEmpty()) {
+ // If no comments, attention set shouldn't update since the user
+ // didn't reply.
+ continue;
+ }
+ replyAttentionSetUpdates.processAutomaticAttentionSetRulesOnReply(
+ bu,
+ changeNotes.get(),
+ isReadyForReview(changeNotes.get()),
+ user,
+ drafts);
+ }
+ }
+ }
- logger.atFine().log("Adding %d replace requests", newChanges.size());
- for (ReplaceRequest replace : replaceByChange.values()) {
- replace.addOps(bu, replaceProgress);
- if (magicBranch != null) {
- bu.setNotifyHandling(
- replace.ontoChange, magicBranch.getNotifyHandling(replace.notes));
- if (magicBranch.shouldPublishComments()) {
- bu.addOp(
- replace.notes.getChangeId(),
- publishCommentsOp.create(replace.psId, project.getNameKey()));
- Optional<ChangeNotes> changeNotes = getChangeNotes(replace.notes.getChangeId());
- if (!changeNotes.isPresent()) {
- // If not present, no need to update attention set here since this is a new
- // change.
- continue;
- }
- List<HumanComment> drafts =
- commentsUtil.draftByChangeAuthor(changeNotes.get(), user.getAccountId());
- if (drafts.isEmpty()) {
- // If no comments, attention set shouldn't update since the user didn't reply.
- continue;
- }
- replyAttentionSetUpdates.processAutomaticAttentionSetRulesOnReply(
- bu, changeNotes.get(), isReadyForReview(changeNotes.get()), user, drafts);
- }
- }
- }
+ logger.atFine().log("Adding %d create requests", newChanges.size());
+ for (CreateRequest create : newChanges) {
+ create.addOps(bu);
+ }
- logger.atFine().log("Adding %d create requests", newChanges.size());
- for (CreateRequest create : newChanges) {
- create.addOps(bu);
- }
+ logger.atFine().log("Adding %d group update requests", newChanges.size());
+ updateGroups.forEach(r -> r.addOps(bu));
- logger.atFine().log("Adding %d group update requests", newChanges.size());
- updateGroups.forEach(r -> r.addOps(bu));
+ logger.atFine().log("Executing batch");
+ try {
+ bu.execute();
+ } catch (UpdateException e) {
+ throw asRestApiException(e);
+ }
- logger.atFine().log("Executing batch");
- try {
- bu.execute();
- } catch (UpdateException e) {
- throw asRestApiException(e);
- }
+ replaceByChange.values().stream()
+ .forEach(
+ req ->
+ result.addChange(
+ ReceiveCommitsResult.ChangeStatus.REPLACED, req.ontoChange));
+ newChanges.stream()
+ .forEach(
+ req ->
+ result.addChange(
+ ReceiveCommitsResult.ChangeStatus.CREATED, req.changeId));
- replaceByChange.values().stream()
- .forEach(
- req ->
- result.addChange(ReceiveCommitsResult.ChangeStatus.REPLACED, req.ontoChange));
- newChanges.stream()
- .forEach(
- req -> result.addChange(ReceiveCommitsResult.ChangeStatus.CREATED, req.changeId));
-
- if (magicBranchCmd != null) {
- magicBranchCmd.setResult(OK);
- }
- for (ReplaceRequest replace : replaceByChange.values()) {
- String rejectMessage = replace.getRejectMessage();
- if (rejectMessage == null) {
- if (replace.inputCommand.getResult() == NOT_ATTEMPTED) {
- // Not necessarily the magic branch, so need to set OK on the original value.
- replace.inputCommand.setResult(OK);
- }
- } else {
- logger.atFine().log("Rejecting due to message from ReplaceOp");
- reject(replace.inputCommand, rejectMessage);
- }
- }
-
+ if (magicBranchCmd != null) {
+ magicBranchCmd.setResult(OK);
+ }
+ for (ReplaceRequest replace : replaceByChange.values()) {
+ String rejectMessage = replace.getRejectMessage();
+ if (rejectMessage == null) {
+ if (replace.inputCommand.getResult() == NOT_ATTEMPTED) {
+ // Not necessarily the magic branch, so need to set OK on the original
+ // value.
+ replace.inputCommand.setResult(OK);
+ }
+ } else {
+ logger.atFine().log("Rejecting due to message from ReplaceOp");
+ reject(replace.inputCommand, rejectMessage);
+ }
+ }
+ }
+ return null;
+ })
+ .defaultTimeoutMultiplier(5)
+ .call();
} catch (ResourceConflictException e) {
addError(e.getMessage());
reject(magicBranchCmd, "conflict");
} catch (BadRequestException | UnprocessableEntityException | AuthException e) {
logger.atFine().withCause(e).log("Rejecting due to client error");
reject(magicBranchCmd, e.getMessage());
- } catch (RestApiException | IOException e) {
+ } catch (RestApiException | UpdateException e) {
throw new StorageException("Can't insert change/patch set for " + project.getName(), e);
}
diff --git a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
index 2f063f6..c31d1b9 100644
--- a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
+++ b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
@@ -103,9 +103,7 @@
memberships.put(user, membership);
}
- /**
- * Remove a the memberships of the given user. No-op if the user does not have any memberships.
- */
+ /** Remove the memberships of the given user. No-op if the user does not have any memberships. */
public void removeMembershipsOf(Account.Id user) {
memberships.remove(user);
}
diff --git a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
index 340d956..7ee484b 100644
--- a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
+++ b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
@@ -268,7 +268,15 @@
@Override
public String toString() {
- return "Index project slice " + projectSlice;
+ if (projectSlice.slices() == 1) {
+ return "Index all changes of project " + projectSlice.name();
+ }
+ return "Index changes slice "
+ + projectSlice.slice()
+ + "/"
+ + projectSlice.slices()
+ + " of project "
+ + projectSlice.name();
}
}
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 7984737..92e722d 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -1049,6 +1049,10 @@
+ (count != null ? ",count=" + count : "");
}
+ public static String formatLabel(String label, String value, @Nullable Integer count) {
+ return formatLabel(label, value, /* accountId= */ null, count);
+ }
+
public static String formatLabel(
String label, String value, @Nullable Account.Id accountId, @Nullable Integer count) {
return label.toLowerCase()
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 56a0bbd..4e361ca 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -36,7 +36,6 @@
import com.google.gerrit.entities.Address;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.GroupDescription;
import com.google.gerrit.entities.GroupReference;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
@@ -1277,15 +1276,9 @@
@Operator
public Predicate<ChangeData> ownerin(String group) throws QueryParseException, IOException {
- GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
- if (g == null) {
- throw error("Group " + group + " not found");
- }
-
+ GroupReference g = parseGroup(group);
AccountGroup.UUID groupId = g.getUUID();
- GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
- if (!(groupDescription instanceof GroupDescription.Internal)
- || containsExernalSubGroups((GroupDescription.Internal) groupDescription)) {
+ if (args.groupBackend.isOrContainsExternalGroup(groupId)) {
return new OwnerinPredicate(args.userFactory, groupId);
}
@@ -1301,15 +1294,9 @@
public Predicate<ChangeData> uploaderin(String group) throws QueryParseException, IOException {
checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
- GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
- if (g == null) {
- throw error("Group " + group + " not found");
- }
-
+ GroupReference g = parseGroup(group);
AccountGroup.UUID groupId = g.getUUID();
- GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
- if (!(groupDescription instanceof GroupDescription.Internal)
- || containsExernalSubGroups((GroupDescription.Internal) groupDescription)) {
+ if (args.groupBackend.isOrContainsExternalGroup(groupId)) {
return new UploaderinPredicate(args.userFactory, groupId);
}
@@ -1356,10 +1343,7 @@
@Operator
public Predicate<ChangeData> reviewerin(String group) throws QueryParseException {
- GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
- if (g == null) {
- throw error("Group " + group + " not found");
- }
+ GroupReference g = parseGroup(group);
return new ReviewerinPredicate(args.userFactory, g.getUUID());
}
@@ -1682,22 +1666,6 @@
return Predicate.and(predicates);
}
- private boolean containsExernalSubGroups(GroupDescription.Internal internalGroup)
- throws IOException {
- for (AccountGroup.UUID subGroupUuid : internalGroup.getSubgroups()) {
- GroupDescription.Basic subGroupDescription = args.groupBackend.get(subGroupUuid);
- if (!(subGroupDescription instanceof GroupDescription.Internal)) {
- return true;
- }
- boolean containsExernalSubGroups =
- containsExernalSubGroups((GroupDescription.Internal) subGroupDescription);
- if (containsExernalSubGroups) {
- return true;
- }
- }
- return false;
- }
-
private Set<Account.Id> getMembers(AccountGroup.UUID g) throws IOException {
Set<Account.Id> accounts;
Set<Account.Id> allMembers =
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
deleted file mode 100644
index 83dd5ba..0000000
--- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ /dev/null
@@ -1,221 +0,0 @@
-// Copyright (C) 2013 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.query.change;
-
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.AccountGroup;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.entities.LabelTypes;
-import com.google.gerrit.entities.PatchSetApproval;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountResolver;
-import com.google.gerrit.server.account.AccountState;
-import com.google.gerrit.server.index.change.ChangeField;
-import com.google.gerrit.server.permissions.ChangePermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.query.change.ChangeData.StorageConstraint;
-import java.io.IOException;
-import java.util.List;
-import java.util.Optional;
-import org.eclipse.jgit.errors.ConfigInvalidException;
-
-public class EqualsLabelPredicate extends ChangeIndexPostFilterPredicate {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- protected final AccountResolver accountResolver;
- protected final ProjectCache projectCache;
- protected final PermissionBackend permissionBackend;
- protected final IdentifiedUser.GenericFactory userFactory;
- /** label name to be matched. */
- protected final String label;
-
- /** Expected vote value for the label. */
- protected final int expVal;
-
- /**
- * Number of times the value {@link #expVal} for label {@link #label} should occur. If null, match
- * with any count greater or equal to 1.
- */
- @Nullable protected final Integer count;
-
- /** Account ID that has voted on the label. */
- protected final Account.Id account;
-
- protected final AccountGroup.UUID group;
-
- public EqualsLabelPredicate(
- LabelPredicate.Args args,
- String label,
- int expVal,
- Account.Id account,
- @Nullable Integer count) {
- super(ChangeField.LABEL_SPEC, ChangeField.formatLabel(label, expVal, account, count));
- this.permissionBackend = args.permissionBackend;
- this.accountResolver = args.accountResolver;
- this.projectCache = args.projectCache;
- this.userFactory = args.userFactory;
- this.count = count;
- this.group = args.group;
- this.label = label;
- this.expVal = expVal;
- this.account = account;
- }
-
- @Override
- public boolean match(ChangeData object) {
- Change c = object.change();
- if (c == null) {
- // The change has disappeared.
- //
- return false;
- }
-
- if (Integer.valueOf(0).equals(count)) {
- // We don't match against count=0 so that the computation is identical to the stored values
- // in the index. We do that since computing count=0 requires looping on all {label_type,
- // vote_value} for the change and storing a {count=0} format for it in the change index which
- // is computationally expensive.
- return false;
- }
-
- Optional<ProjectState> project = projectCache.get(c.getDest().project());
- if (!project.isPresent()) {
- // The project has disappeared.
- //
- return false;
- }
-
- LabelType labelType = type(project.get().getLabelTypes(), label);
- if (labelType == null) {
- return false; // Label is not defined by this project.
- }
-
- boolean hasVote = false;
- int matchingVotes = 0;
- StorageConstraint currentStorageConstraint = object.getStorageConstraint();
- object.setStorageConstraint(ChangeData.StorageConstraint.INDEX_PRIMARY_NOTEDB_SECONDARY);
- for (PatchSetApproval p : object.currentApprovals()) {
- if (labelType.matches(p)) {
- hasVote = true;
- if (match(object, p.value(), p.accountId())) {
- matchingVotes += 1;
- }
- }
- }
- object.setStorageConstraint(currentStorageConstraint);
- if (!hasVote && expVal == 0) {
- return true;
- }
-
- return count == null ? matchingVotes >= 1 : matchingVotes == count;
- }
-
- @Nullable
- protected static LabelType type(LabelTypes types, String toFind) {
- if (types.byLabel(toFind).isPresent()) {
- return types.byLabel(toFind).get();
- }
-
- for (LabelType lt : types.getLabelTypes()) {
- if (toFind.equalsIgnoreCase(lt.getName())) {
- return lt;
- }
- }
- return null;
- }
-
- protected boolean match(ChangeData cd, short value, Account.Id approver) {
- if (value != expVal) {
- return false;
- }
-
- if (account != null) {
- // case when account in query is numeric
- if (!account.equals(approver) && !isMagicUser()) {
- return false;
- }
-
- // case when account in query = owner
- if (account.equals(ChangeQueryBuilder.OWNER_ACCOUNT_ID)
- && !cd.change().getOwner().equals(approver)) {
- return false;
- }
-
- // case when account in query = non_uploader
- if (account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
- && cd.currentPatchSet().uploader().equals(approver)) {
- return false;
- }
-
- if (account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID)) {
- if ((cd.currentPatchSet().uploader().equals(approver)
- || matchAccount(cd.getCommitter().getEmailAddress(), approver)
- || matchAccount(cd.getAuthor().getEmailAddress(), approver))) {
- return false;
- }
- }
- }
-
- IdentifiedUser reviewer = userFactory.create(approver);
- if (group != null && !reviewer.getEffectiveGroups().contains(group)) {
- return false;
- }
-
- // Check the user has 'READ' permission.
- try {
- PermissionBackend.ForChange perm = permissionBackend.absentUser(approver).change(cd);
- if (!projectCache.get(cd.project()).map(ProjectState::statePermitsRead).orElse(false)) {
- return false;
- }
-
- perm.check(ChangePermission.READ);
- return true;
- } catch (PermissionBackendException | AuthException e) {
- return false;
- }
- }
-
- /**
- * Returns true if the {@code email} parameter belongs to the account identified by the {@code
- * accountId} parameter.
- */
- private boolean matchAccount(String email, Account.Id accountId) {
- try {
- List<AccountState> accountsList = accountResolver.resolve(email).asList();
- return accountsList.stream().anyMatch(c -> c.account().id().equals(accountId));
- } catch (ConfigInvalidException | IOException e) {
- logger.atWarning().withCause(e).log("Failed to resolve account %s", email);
- }
- return false;
- }
-
- private boolean isMagicUser() {
- return account.equals(ChangeQueryBuilder.OWNER_ACCOUNT_ID)
- || account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
- || account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID);
- }
-
- @Override
- public int getCost() {
- return 1 + (group == null ? 0 : 1);
- }
-}
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicates.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicates.java
new file mode 100644
index 0000000..ffd4497
--- /dev/null
+++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicates.java
@@ -0,0 +1,268 @@
+// Copyright (C) 2013 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.query.change;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.LabelTypes;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.index.query.PostFilterPredicate;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.gerrit.server.query.change.ChangeData.StorageConstraint;
+import java.io.IOException;
+import java.util.List;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+
+public class EqualsLabelPredicates {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ public static class PostFilterEqualsLabelPredicate extends PostFilterPredicate<ChangeData> {
+ private final Matcher matcher;
+
+ public PostFilterEqualsLabelPredicate(
+ LabelPredicate.Args args, String label, int expVal, @Nullable Integer count) {
+ super(ChangeQueryBuilder.FIELD_LABEL, ChangeField.formatLabel(label, expVal, count));
+ matcher = new Matcher(args, label, expVal, count);
+ }
+
+ @Override
+ public boolean match(ChangeData object) {
+ return matcher.match(object);
+ }
+
+ @Override
+ public int getCost() {
+ return 2;
+ }
+ }
+
+ public static class IndexEqualsLabelPredicate extends ChangeIndexPostFilterPredicate {
+ private final Matcher matcher;
+
+ public IndexEqualsLabelPredicate(
+ LabelPredicate.Args args, String label, int expVal, @Nullable Integer count) {
+ this(args, label, expVal, null, count);
+ }
+
+ public IndexEqualsLabelPredicate(
+ LabelPredicate.Args args,
+ String label,
+ int expVal,
+ Account.Id account,
+ @Nullable Integer count) {
+ super(ChangeField.LABEL_SPEC, ChangeField.formatLabel(label, expVal, account, count));
+ this.matcher = new Matcher(args, label, expVal, account, count);
+ }
+
+ @Override
+ public boolean match(ChangeData object) {
+ return matcher.match(object);
+ }
+
+ @Override
+ public int getCost() {
+ return 1 + (matcher.group == null ? 0 : 1);
+ }
+ }
+
+ private static class Matcher {
+ protected final AccountResolver accountResolver;
+ protected final ProjectCache projectCache;
+ protected final PermissionBackend permissionBackend;
+ protected final IdentifiedUser.GenericFactory userFactory;
+ /** label name to be matched. */
+ protected final String label;
+ /** Expected vote value for the label. */
+ protected final int expVal;
+
+ /**
+ * Number of times the value {@link #expVal} for label {@link #label} should occur. If null,
+ * match with any count greater or equal to 1.
+ */
+ @Nullable protected final Integer count;
+
+ /** Account ID that has voted on the label. */
+ protected final Account.Id account;
+
+ protected final AccountGroup.UUID group;
+
+ public Matcher(LabelPredicate.Args args, String label, int expVal, @Nullable Integer count) {
+ this(args, label, expVal, null, count);
+ }
+
+ public Matcher(
+ LabelPredicate.Args args,
+ String label,
+ int expVal,
+ Account.Id account,
+ @Nullable Integer count) {
+ this.permissionBackend = args.permissionBackend;
+ this.accountResolver = args.accountResolver;
+ this.projectCache = args.projectCache;
+ this.userFactory = args.userFactory;
+ this.group = args.group;
+ this.label = label;
+ this.expVal = expVal;
+ this.account = account;
+ this.count = count;
+ }
+
+ public boolean match(ChangeData cd) {
+ Change c = cd.change();
+ if (c == null) {
+ // The change has disappeared.
+ return false;
+ }
+
+ if (Integer.valueOf(0).equals(count)) {
+ // We don't match against count=0 so that the computation is identical to the stored values
+ // in the index. We do that since computing count=0 requires looping on all {label_type,
+ // vote_value} for the change and storing a {count=0} format for it in the change index
+ // which is computationally expensive.
+ return false;
+ }
+
+ Optional<ProjectState> project = projectCache.get(c.getDest().project());
+ if (!project.isPresent()) {
+ // The project has disappeared.
+ return false;
+ }
+
+ LabelType labelType = type(project.get().getLabelTypes(), label);
+ if (labelType == null) {
+ return false; // Label is not defined by this project.
+ }
+
+ boolean hasVote = false;
+ int matchingVotes = 0;
+ StorageConstraint currentStorageConstraint = cd.getStorageConstraint();
+ cd.setStorageConstraint(ChangeData.StorageConstraint.INDEX_PRIMARY_NOTEDB_SECONDARY);
+ for (PatchSetApproval psa : cd.currentApprovals()) {
+ if (labelType.matches(psa)) {
+ hasVote = true;
+ if (match(cd, psa)) {
+ matchingVotes += 1;
+ }
+ }
+ }
+ cd.setStorageConstraint(currentStorageConstraint);
+ if (!hasVote && expVal == 0) {
+ return true;
+ }
+
+ return count == null ? matchingVotes >= 1 : matchingVotes == count;
+ }
+
+ private boolean match(ChangeData cd, PatchSetApproval psa) {
+ if (psa.value() != expVal) {
+ return false;
+ }
+ Account.Id approver = psa.accountId();
+
+ if (account != null) {
+ // case when account in query is numeric
+ if (!account.equals(approver) && !isMagicUser()) {
+ return false;
+ }
+
+ // case when account in query = owner
+ if (account.equals(ChangeQueryBuilder.OWNER_ACCOUNT_ID)
+ && !cd.change().getOwner().equals(approver)) {
+ return false;
+ }
+
+ // case when account in query = non_uploader
+ if (account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
+ && cd.currentPatchSet().uploader().equals(approver)) {
+ return false;
+ }
+
+ if (account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID)) {
+ if ((cd.currentPatchSet().uploader().equals(approver)
+ || matchAccount(cd.getCommitter().getEmailAddress(), approver)
+ || matchAccount(cd.getAuthor().getEmailAddress(), approver))) {
+ return false;
+ }
+ }
+ }
+
+ IdentifiedUser reviewer = userFactory.create(approver);
+ if (group != null && !reviewer.getEffectiveGroups().contains(group)) {
+ return false;
+ }
+
+ // Check the user has 'READ' permission.
+ try {
+ PermissionBackend.ForChange perm = permissionBackend.absentUser(approver).change(cd);
+ if (!projectCache.get(cd.project()).map(ProjectState::statePermitsRead).orElse(false)) {
+ return false;
+ }
+
+ perm.check(ChangePermission.READ);
+ return true;
+ } catch (PermissionBackendException | AuthException e) {
+ return false;
+ }
+ }
+
+ /**
+ * Returns true if the {@code email} parameter belongs to the account identified by the {@code
+ * accountId} parameter.
+ */
+ private boolean matchAccount(String email, Account.Id accountId) {
+ try {
+ List<AccountState> accountsList = accountResolver.resolve(email).asList();
+ return accountsList.stream().anyMatch(c -> c.account().id().equals(accountId));
+ } catch (ConfigInvalidException | IOException e) {
+ logger.atWarning().withCause(e).log("Failed to resolve account %s", email);
+ }
+ return false;
+ }
+
+ private boolean isMagicUser() {
+ return account.equals(ChangeQueryBuilder.OWNER_ACCOUNT_ID)
+ || account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
+ || account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID);
+ }
+ }
+
+ @Nullable
+ public static LabelType type(LabelTypes types, String toFind) {
+ if (types.byLabel(toFind).isPresent()) {
+ return types.byLabel(toFind).get();
+ }
+
+ for (LabelType lt : types.getLabelTypes()) {
+ if (toFind.equalsIgnoreCase(lt.getName())) {
+ return lt;
+ }
+ }
+ return null;
+ }
+}
diff --git a/java/com/google/gerrit/server/query/change/LabelPredicate.java b/java/com/google/gerrit/server/query/change/LabelPredicate.java
index d89940d..5a38958 100644
--- a/java/com/google/gerrit/server/query/change/LabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/LabelPredicate.java
@@ -24,6 +24,7 @@
import com.google.gerrit.index.query.RangeUtil.Range;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
@@ -46,6 +47,7 @@
protected final AccountGroup.UUID group;
protected final Integer count;
protected final PredicateArgs.Operator countOp;
+ protected final GroupBackend groupBackend;
protected Args(
AccountResolver accountResolver,
@@ -56,7 +58,8 @@
Set<Account.Id> accounts,
AccountGroup.UUID group,
@Nullable Integer count,
- @Nullable PredicateArgs.Operator countOp) {
+ @Nullable PredicateArgs.Operator countOp,
+ GroupBackend groupBackend) {
this.accountResolver = accountResolver;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
@@ -66,6 +69,7 @@
this.group = group;
this.count = count;
this.countOp = countOp;
+ this.groupBackend = groupBackend;
}
}
@@ -101,7 +105,8 @@
accounts,
group,
count,
- countOp)));
+ countOp,
+ a.groupBackend)));
this.value = value;
}
@@ -185,24 +190,36 @@
protected static Predicate<ChangeData> equalsLabelPredicate(
Args args, String label, int expVal, @Nullable Integer count) {
+ if (args.groupBackend.isOrContainsExternalGroup(args.group)) {
+ // We can only get members of internal groups and negating an index search that doesn't
+ // include the external group information leads to incorrect query results. Use a
+ // PostFilterPredicate in this case instead.
+ return new EqualsLabelPredicates.PostFilterEqualsLabelPredicate(args, label, expVal, count);
+ }
if (args.accounts == null || args.accounts.isEmpty()) {
- return new EqualsLabelPredicate(args, label, expVal, null, count);
+ return new EqualsLabelPredicates.IndexEqualsLabelPredicate(args, label, expVal, count);
}
List<Predicate<ChangeData>> r = new ArrayList<>();
for (Account.Id a : args.accounts) {
- r.add(new EqualsLabelPredicate(args, label, expVal, a, count));
+ r.add(new EqualsLabelPredicates.IndexEqualsLabelPredicate(args, label, expVal, a, count));
}
return or(r);
}
protected static Predicate<ChangeData> magicLabelPredicate(
Args args, MagicLabelVote mlv, @Nullable Integer count) {
+ if (args.groupBackend.isOrContainsExternalGroup(args.group)) {
+ // We can only get members of internal groups and negating an index search that doesn't
+ // include the external group information leads to incorrect query results. Use a
+ // PostFilterPredicate in this case instead.
+ return new MagicLabelPredicates.PostFilterMagicLabelPredicate(args, mlv, count);
+ }
if (args.accounts == null || args.accounts.isEmpty()) {
- return new MagicLabelPredicate(args, mlv, /* account= */ null, count);
+ return new MagicLabelPredicates.IndexMagicLabelPredicate(args, mlv, count);
}
List<Predicate<ChangeData>> r = new ArrayList<>();
for (Account.Id a : args.accounts) {
- r.add(new MagicLabelPredicate(args, mlv, a, count));
+ r.add(new MagicLabelPredicates.IndexMagicLabelPredicate(args, mlv, a, count));
}
return or(r);
}
diff --git a/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java b/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
deleted file mode 100644
index 9120069..0000000
--- a/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
+++ /dev/null
@@ -1,124 +0,0 @@
-// Copyright (C) 2021 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.query.change;
-
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.entities.LabelTypes;
-import com.google.gerrit.entities.LabelValue;
-import com.google.gerrit.index.query.Predicate;
-import com.google.gerrit.server.index.change.ChangeField;
-import com.google.gerrit.server.project.ProjectState;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Optional;
-
-public class MagicLabelPredicate extends ChangeIndexPredicate {
- protected final LabelPredicate.Args args;
- private final MagicLabelVote magicLabelVote;
- private final Account.Id account;
- @Nullable private final Integer count;
-
- public MagicLabelPredicate(
- LabelPredicate.Args args,
- MagicLabelVote magicLabelVote,
- Account.Id account,
- @Nullable Integer count) {
- super(
- ChangeField.LABEL_SPEC,
- ChangeField.formatLabel(
- magicLabelVote.label(), magicLabelVote.value().name(), account, count));
- this.account = account;
- this.args = args;
- this.magicLabelVote = magicLabelVote;
- this.count = count;
- }
-
- @Override
- public boolean match(ChangeData changeData) {
- Change change = changeData.change();
- if (change == null) {
- // The change has disappeared.
- //
- return false;
- }
-
- Optional<ProjectState> project = args.projectCache.get(change.getDest().project());
- if (!project.isPresent()) {
- // The project has disappeared.
- //
- return false;
- }
-
- LabelType labelType = type(project.get().getLabelTypes(), magicLabelVote.label());
- if (labelType == null) {
- return false; // Label is not defined by this project.
- }
-
- switch (magicLabelVote.value()) {
- case ANY:
- return matchAny(changeData, labelType);
- case MIN:
- return matchNumeric(changeData, magicLabelVote.label(), labelType.getMin().getValue());
- case MAX:
- return matchNumeric(changeData, magicLabelVote.label(), labelType.getMax().getValue());
- }
-
- throw new IllegalStateException("Unsupported magic label value: " + magicLabelVote.value());
- }
-
- private boolean matchAny(ChangeData changeData, LabelType labelType) {
- List<Predicate<ChangeData>> predicates = new ArrayList<>();
- for (LabelValue labelValue : labelType.getValues()) {
- if (labelValue.getValue() != 0) {
- predicates.add(numericPredicate(labelType.getName(), labelValue.getValue()));
- }
- }
- return or(predicates).asMatchable().match(changeData);
- }
-
- private boolean matchNumeric(ChangeData changeData, String label, short value) {
- return numericPredicate(label, value).match(changeData);
- }
-
- private EqualsLabelPredicate numericPredicate(String label, short value) {
- return new EqualsLabelPredicate(args, label, value, account, count);
- }
-
- public String getLabel() {
- return magicLabelVote.label();
- }
-
- public boolean ignoresUploaderApprovals() {
- return account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
- || account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID);
- }
-
- @Nullable
- protected static LabelType type(LabelTypes types, String toFind) {
- if (types.byLabel(toFind).isPresent()) {
- return types.byLabel(toFind).get();
- }
-
- for (LabelType lt : types.getLabelTypes()) {
- if (toFind.equalsIgnoreCase(lt.getName())) {
- return lt;
- }
- }
- return null;
- }
-}
diff --git a/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java b/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java
new file mode 100644
index 0000000..82d8717
--- /dev/null
+++ b/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java
@@ -0,0 +1,200 @@
+// Copyright (C) 2021 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.query.change;
+
+import static com.google.gerrit.server.query.change.EqualsLabelPredicates.type;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.LabelValue;
+import com.google.gerrit.index.query.PostFilterPredicate;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.project.ProjectState;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+
+public class MagicLabelPredicates {
+ public static class PostFilterMagicLabelPredicate extends PostFilterPredicate<ChangeData> {
+ private static class PostFilterMatcher extends Matcher {
+ public PostFilterMatcher(
+ LabelPredicate.Args args, MagicLabelVote magicLabelVote, @Nullable Integer count) {
+ super(args, magicLabelVote, count);
+ }
+
+ @Override
+ protected Predicate<ChangeData> numericPredicate(String label, short value) {
+ return new EqualsLabelPredicates.PostFilterEqualsLabelPredicate(args, label, value, count);
+ }
+ }
+
+ private final PostFilterMatcher matcher;
+
+ public PostFilterMagicLabelPredicate(
+ LabelPredicate.Args args, MagicLabelVote magicLabelVote, @Nullable Integer count) {
+ super(
+ ChangeQueryBuilder.FIELD_LABEL,
+ ChangeField.formatLabel(magicLabelVote.label(), magicLabelVote.value().name(), count));
+ this.matcher = new PostFilterMatcher(args, magicLabelVote, count);
+ }
+
+ @Override
+ public boolean match(ChangeData changeData) {
+ return matcher.match(changeData);
+ }
+
+ @Override
+ public int getCost() {
+ return 2;
+ }
+
+ public String getLabel() {
+ return matcher.getLabel();
+ }
+
+ public boolean ignoresUploaderApprovals() {
+ return matcher.ignoresUploaderApprovals();
+ }
+ }
+
+ public static class IndexMagicLabelPredicate extends ChangeIndexPredicate {
+ private static class IndexMatcher extends Matcher {
+ public IndexMatcher(
+ LabelPredicate.Args args,
+ MagicLabelVote magicLabelVote,
+ Account.Id account,
+ @Nullable Integer count) {
+ super(args, magicLabelVote, account, count);
+ }
+
+ @Override
+ protected Predicate<ChangeData> numericPredicate(String label, short value) {
+ return new EqualsLabelPredicates.IndexEqualsLabelPredicate(
+ args, label, value, account, count);
+ }
+ }
+
+ private final Matcher matcher;
+
+ public IndexMagicLabelPredicate(
+ LabelPredicate.Args args, MagicLabelVote magicLabelVote, @Nullable Integer count) {
+ this(args, magicLabelVote, null, count);
+ }
+
+ public IndexMagicLabelPredicate(
+ LabelPredicate.Args args,
+ MagicLabelVote magicLabelVote,
+ Account.Id account,
+ @Nullable Integer count) {
+ super(
+ ChangeField.LABEL_SPEC,
+ ChangeField.formatLabel(
+ magicLabelVote.label(), magicLabelVote.value().name(), account, count));
+ this.matcher = new IndexMatcher(args, magicLabelVote, account, count);
+ }
+
+ @Override
+ public boolean match(ChangeData changeData) {
+ return matcher.match(changeData);
+ }
+
+ public String getLabel() {
+ return matcher.getLabel();
+ }
+
+ public boolean ignoresUploaderApprovals() {
+ return matcher.ignoresUploaderApprovals();
+ }
+ }
+
+ private abstract static class Matcher {
+ protected final LabelPredicate.Args args;
+ protected final MagicLabelVote magicLabelVote;
+ protected final Account.Id account;
+ @Nullable protected final Integer count;
+
+ public Matcher(
+ LabelPredicate.Args args, MagicLabelVote magicLabelVote, @Nullable Integer count) {
+ this(args, magicLabelVote, null, count);
+ }
+
+ public Matcher(
+ LabelPredicate.Args args,
+ MagicLabelVote magicLabelVote,
+ Account.Id account,
+ @Nullable Integer count) {
+ this.account = account;
+ this.args = args;
+ this.magicLabelVote = magicLabelVote;
+ this.count = count;
+ }
+
+ public boolean match(ChangeData cd) {
+ Change change = cd.change();
+ if (change == null) {
+ return false; // The change has disappeared.
+ }
+
+ Optional<ProjectState> project = args.projectCache.get(change.getDest().project());
+ if (!project.isPresent()) {
+ return false; // The project has disappeared.
+ }
+
+ LabelType labelType = type(project.get().getLabelTypes(), magicLabelVote.label());
+ if (labelType == null) {
+ return false; // Label is not defined by this project.
+ }
+
+ switch (magicLabelVote.value()) {
+ case ANY:
+ return matchAny(cd, labelType);
+ case MIN:
+ return matchNumeric(cd, magicLabelVote.label(), labelType.getMin().getValue());
+ case MAX:
+ return matchNumeric(cd, magicLabelVote.label(), labelType.getMax().getValue());
+ }
+
+ throw new IllegalStateException("Unsupported magic label value: " + magicLabelVote.value());
+ }
+
+ public String getLabel() {
+ return magicLabelVote.label();
+ }
+
+ public boolean ignoresUploaderApprovals() {
+ return account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
+ || account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID);
+ }
+
+ private boolean matchAny(ChangeData changeData, LabelType labelType) {
+ List<Predicate<ChangeData>> predicates = new ArrayList<>();
+ for (LabelValue labelValue : labelType.getValues()) {
+ if (labelValue.getValue() != 0) {
+ predicates.add(numericPredicate(labelType.getName(), labelValue.getValue()));
+ }
+ }
+ return Predicate.or(predicates).asMatchable().match(changeData);
+ }
+
+ private boolean matchNumeric(ChangeData changeData, String label, short value) {
+ return numericPredicate(label, value).asMatchable().match(changeData);
+ }
+
+ protected abstract Predicate<ChangeData> numericPredicate(String label, short value);
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
index 29e453b..0036d87 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
@@ -284,14 +284,14 @@
// TODO(davido): Remove this workaround when patch set level comments are exposed in comment
// added event. For backwards compatibility, patchset level comment has a higher priority
// than change message and should be used as comment in comment added event.
- if (in.comments != null && in.comments.containsKey(PATCHSET_LEVEL)) {
- List<CommentInput> patchSetLevelComments = in.comments.get(PATCHSET_LEVEL);
- if (patchSetLevelComments != null && !patchSetLevelComments.isEmpty()) {
- CommentInput firstComment = patchSetLevelComments.get(0);
- if (!Strings.isNullOrEmpty(firstComment.message)) {
- comment = String.format("Patch Set %s:\n\n%s", psId.get(), firstComment.message);
- }
- }
+ String patchSetLevelComment =
+ comments.stream()
+ .filter(c -> c.key.filename.equals(PATCHSET_LEVEL))
+ .map(c -> Strings.nullToEmpty(c.message))
+ .collect(Collectors.joining("\n"))
+ .trim();
+ if (!patchSetLevelComment.isEmpty()) {
+ comment = String.format("Patch Set %s:\n\n%s", psId.get(), patchSetLevelComment);
}
}
commentAdded.fire(
diff --git a/java/com/google/gerrit/server/submit/MergeMetrics.java b/java/com/google/gerrit/server/submit/MergeMetrics.java
index 9eb8061..e9832fd 100644
--- a/java/com/google/gerrit/server/submit/MergeMetrics.java
+++ b/java/com/google/gerrit/server/submit/MergeMetrics.java
@@ -21,7 +21,7 @@
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.MagicLabelPredicate;
+import com.google.gerrit.server.query.change.MagicLabelPredicates;
import com.google.gerrit.server.query.change.SubmitRequirementChangeQueryBuilder;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -114,8 +114,16 @@
// the uploader are ignored, we must check if there is any MagicLabelPredicate for the
// Code-Review label that ignores approvals of the uploader (aka has user set to non_uploader
// or non_contributor).
- if (predicate instanceof MagicLabelPredicate) {
- MagicLabelPredicate magicLabelPredicate = (MagicLabelPredicate) predicate;
+ if (predicate instanceof MagicLabelPredicates.PostFilterMagicLabelPredicate) {
+ MagicLabelPredicates.PostFilterMagicLabelPredicate magicLabelPredicate =
+ (MagicLabelPredicates.PostFilterMagicLabelPredicate) predicate;
+ if (magicLabelPredicate.getLabel().equalsIgnoreCase("Code-Review")
+ && magicLabelPredicate.ignoresUploaderApprovals()) {
+ return true;
+ }
+ } else if (predicate instanceof MagicLabelPredicates.IndexMagicLabelPredicate) {
+ MagicLabelPredicates.IndexMagicLabelPredicate magicLabelPredicate =
+ (MagicLabelPredicates.IndexMagicLabelPredicate) predicate;
if (magicLabelPredicate.getLabel().equalsIgnoreCase("Code-Review")
&& magicLabelPredicate.ignoresUploaderApprovals()) {
return true;
diff --git a/javatests/com/google/gerrit/acceptance/ssh/StreamEventsIT.java b/javatests/com/google/gerrit/acceptance/ssh/StreamEventsIT.java
index f02d244..78e8c5e 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/StreamEventsIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/StreamEventsIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.ssh;
import static com.google.gerrit.acceptance.WaitUtil.waitUntil;
+import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
import com.google.common.base.Splitter;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -22,13 +23,17 @@
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.UseSsh;
import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
+import com.google.gerrit.server.query.change.ChangeData;
import java.io.IOException;
import java.io.Reader;
import java.time.Duration;
import java.util.List;
import java.util.function.Supplier;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
import java.util.stream.StreamSupport;
import org.junit.After;
import org.junit.Before;
@@ -40,7 +45,9 @@
public class StreamEventsIT extends AbstractDaemonTest {
private static final Duration MAX_DURATION_FOR_RECEIVING_EVENTS = Duration.ofSeconds(2);
private static final String TEST_REVIEW_COMMENT = "any comment";
+ private static final String TEST_REVIEW_DRAFT_COMMENT = "any draft comment";
private Reader streamEventsReader;
+ private ChangeData change;
@Before
public void setup() throws Exception {
@@ -59,6 +66,23 @@
}
@Test
+ public void publishedDraftPatchSetLevelCommentShowsUpInStreamEvents() throws Exception {
+ change = createChange().getChange();
+
+ String firstDraftComment = String.format("%s 1", TEST_REVIEW_DRAFT_COMMENT);
+ String secondDraftComment = String.format("%s 2", TEST_REVIEW_DRAFT_COMMENT);
+
+ draftReviewChange(PATCHSET_LEVEL, firstDraftComment);
+ draftReviewChange(PATCHSET_LEVEL, secondDraftComment);
+ publishDraftReviews();
+
+ waitForEvent(
+ () ->
+ pollEventsContaining("comment-added", firstDraftComment, secondDraftComment).size()
+ == 1);
+ }
+
+ @Test
public void batchRefsUpdatedShowSeparatelyInStreamEvents() throws Exception {
String refName = createChange().getChange().currentPatchSet().refName();
waitForEvent(
@@ -77,7 +101,22 @@
changeApi.current().review(reviewInput);
}
- private List<String> pollEventsContaining(String eventType, String expectedContent) {
+ private void draftReviewChange(String path, String reviewMessage) throws Exception {
+ DraftInput draftInput = new DraftInput();
+ draftInput.message = reviewMessage;
+ draftInput.path = path;
+ ChangeApi changeApi = gApi.changes().id(change.getId().get());
+ changeApi.current().createDraft(draftInput).get();
+ }
+
+ private void publishDraftReviews() throws Exception {
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.tag = "new_tag";
+ reviewInput.drafts = DraftHandling.PUBLISH;
+ gApi.changes().id(change.getId().get()).current().review(reviewInput);
+ }
+
+ private List<String> pollEventsContaining(String eventType, String... expectedContent) {
try {
char[] cbuf = new char[2048];
StringBuilder eventsOutput = new StringBuilder();
@@ -90,7 +129,7 @@
.filter(
event ->
event.contains(String.format("\"type\":\"%s\"", eventType))
- && event.contains(expectedContent))
+ && Stream.of(expectedContent).allMatch(event::contains))
.collect(Collectors.toList());
} catch (IOException e) {
throw new IllegalStateException(e);
diff --git a/javatests/com/google/gerrit/httpd/gitweb/GitwebServletTest.java b/javatests/com/google/gerrit/httpd/gitweb/GitwebServletTest.java
new file mode 100644
index 0000000..d1598b9
--- /dev/null
+++ b/javatests/com/google/gerrit/httpd/gitweb/GitwebServletTest.java
@@ -0,0 +1,107 @@
+// Copyright (C) 2022 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.httpd.gitweb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.mock;
+
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.config.AllProjectsNameProvider;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.config.GitwebCgiConfig;
+import com.google.gerrit.server.config.GitwebConfig;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.git.LocalDiskRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class GitwebServletTest {
+ @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ private Config cfg;
+ private SitePaths site;
+ private LocalDiskRepositoryManager repoManager;
+ private ProjectCache projectCache;
+ private PermissionBackend permissionBackendMock;
+ private GitwebCgiConfig gitWebCgiConfig;
+ private GitwebConfig gitWebConfig;
+ private GitwebServlet servlet;
+ private AllProjectsName allProjectsName;
+
+ @Before
+ public void setUp() throws Exception {
+ site = new SitePaths(temporaryFolder.newFolder().toPath());
+ site.resolve("git").toFile().mkdir();
+ cfg = new Config();
+ cfg.setString("gerrit", null, "basePath", "git");
+ repoManager =
+ Guice.createInjector(
+ new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(SitePaths.class).toInstance(site);
+ bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(cfg);
+ }
+ })
+ .getInstance(LocalDiskRepositoryManager.class);
+ projectCache = mock(ProjectCache.class);
+ permissionBackendMock = mock(PermissionBackend.class);
+ gitWebCgiConfig = mock(GitwebCgiConfig.class);
+ gitWebConfig = mock(GitwebConfig.class);
+ allProjectsName = new AllProjectsName(AllProjectsNameProvider.DEFAULT);
+ // All-Projects must exist prior to calling GitwebServlet ctor
+ repoManager.createRepository(allProjectsName);
+ servlet =
+ new GitwebServlet(
+ repoManager,
+ projectCache,
+ permissionBackendMock,
+ null,
+ site,
+ cfg,
+ null,
+ null,
+ gitWebConfig,
+ gitWebCgiConfig,
+ allProjectsName);
+ }
+
+ @Test
+ public void projectRootSetToBasePathForSimpleRepository() throws Exception {
+ Project.NameKey foo = Project.nameKey("foo");
+ try (Repository repo = repoManager.createRepository(foo)) {
+ assertThat(servlet.getProjectRoot(foo))
+ .isEqualTo(repoManager.getBasePath(foo).toAbsolutePath().toString());
+ }
+ }
+
+ @Test
+ public void projectRootSetToBasePathForNestedRepository() throws Exception {
+ Project.NameKey baz = Project.nameKey("foo/bar/baz");
+ try (Repository repo = repoManager.createRepository(baz)) {
+ assertThat(servlet.getProjectRoot(baz))
+ .isEqualTo(repoManager.getBasePath(baz).toAbsolutePath().toString());
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/server/cache/mem/BUILD b/javatests/com/google/gerrit/server/cache/mem/BUILD
index a263c7b..e50d95f 100644
--- a/javatests/com/google/gerrit/server/cache/mem/BUILD
+++ b/javatests/com/google/gerrit/server/cache/mem/BUILD
@@ -4,6 +4,8 @@
name = "tests",
srcs = glob(["*Test.java"]),
deps = [
+ "//java/com/google/gerrit/extensions:api",
+ "//java/com/google/gerrit/metrics",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/cache/mem",
"//lib:jgit",
diff --git a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
index 5958465..710d67a 100644
--- a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
+++ b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
@@ -19,15 +19,27 @@
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
+import com.google.common.cache.RemovalNotification;
import com.google.common.cache.Weigher;
import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.metrics.DisabledMetricMaker;
import com.google.gerrit.server.cache.CacheDef;
+import com.google.gerrit.server.cache.ForwardingRemovalListener;
+import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.plugincontext.PluginContext;
+import com.google.gerrit.server.plugincontext.PluginMapContext;
+import com.google.gerrit.server.util.IdGenerator;
+import com.google.inject.Guice;
import com.google.inject.TypeLiteral;
import java.time.Duration;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
@@ -47,20 +59,50 @@
private static final String TEST_CACHE = "test-cache";
private static final long TEST_TIMEOUT_SEC = 1;
private static final int TEST_CACHE_KEY = 1;
+ private static final int TEST_CACHE_VALUE = 2;
private DefaultMemoryCacheFactory memoryCacheFactory;
+ private DefaultMemoryCacheFactory memoryCacheFactoryDirectExecutor;
+ private DefaultMemoryCacheFactory memoryCacheFactoryWithThreadPool;
private Config memoryCacheConfig;
private ScheduledExecutorService executor;
+ private Config memoryCacheConfigDirectExecutor;
+ private Config memoryCacheConfigWithThreadPool;
private CyclicBarrier cacheGetStarted;
private CyclicBarrier cacheGetCompleted;
+ private CyclicBarrier evictionReceived;
+ private ForwardingRemovalTrackerListener forwardingRemovalListener;
+ private WorkQueue workQueue;
@Before
public void setUp() {
+ IdGenerator idGenerator = Guice.createInjector().getInstance(IdGenerator.class);
+ workQueue =
+ new WorkQueue(
+ idGenerator,
+ 10,
+ new DisabledMetricMaker(),
+ new PluginMapContext<>(
+ DynamicMap.emptyMap(), PluginContext.PluginMetrics.DISABLED_INSTANCE));
memoryCacheConfig = new Config();
- memoryCacheFactory = new DefaultMemoryCacheFactory(memoryCacheConfig, null);
+ memoryCacheConfigDirectExecutor = new Config();
+ memoryCacheConfigDirectExecutor.setInt("cache", null, "threads", 0);
+ memoryCacheConfigWithThreadPool = new Config();
+ memoryCacheConfigWithThreadPool.setInt("cache", null, "threads", 1);
+ forwardingRemovalListener = new ForwardingRemovalTrackerListener();
+ memoryCacheFactory =
+ new DefaultMemoryCacheFactory(
+ memoryCacheConfig, (cache) -> forwardingRemovalListener, workQueue);
+ memoryCacheFactoryDirectExecutor =
+ new DefaultMemoryCacheFactory(
+ memoryCacheConfigDirectExecutor, (cache) -> forwardingRemovalListener, workQueue);
+ memoryCacheFactoryWithThreadPool =
+ new DefaultMemoryCacheFactory(
+ memoryCacheConfigWithThreadPool, (cache) -> forwardingRemovalListener, workQueue);
executor = Executors.newScheduledThreadPool(1);
cacheGetStarted = new CyclicBarrier(2);
cacheGetCompleted = new CyclicBarrier(2);
+ evictionReceived = new CyclicBarrier(2);
}
@Test
@@ -96,6 +138,45 @@
}
@Test
+ public void shouldRunEvictionListenerInBackgroundByDefault() throws Exception {
+ shouldRunEvictionListenerInThreadPool(memoryCacheFactory, "ForkJoinPool");
+ }
+
+ @Test
+ public void shouldRunEvictionListenerInThreadPool() throws Exception {
+ shouldRunEvictionListenerInThreadPool(
+ memoryCacheFactoryWithThreadPool, DefaultMemoryCacheFactory.CACHE_EXECUTOR_PREFIX);
+ }
+
+ private void shouldRunEvictionListenerInThreadPool(
+ DefaultMemoryCacheFactory cacheFactory, String threadPoolPrefix) throws Exception {
+ LoadingCache<Integer, Integer> cache =
+ cacheFactory.build(newCacheDef(1), newCacheLoader(identity()));
+
+ cache.put(TEST_CACHE_KEY, TEST_CACHE_VALUE);
+ cache.invalidate(TEST_CACHE_KEY);
+
+ assertThat(forwardingRemovalListener.contains(TEST_CACHE_KEY, TEST_CACHE_VALUE)).isFalse();
+
+ evictionReceived.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+
+ assertThat(forwardingRemovalListener.contains(TEST_CACHE_KEY, TEST_CACHE_VALUE)).isTrue();
+ assertThat(forwardingRemovalListener.removalThreadName(TEST_CACHE_KEY))
+ .startsWith(threadPoolPrefix);
+ }
+
+ @Test
+ public void shouldRunEvictionListenerWithDirectExecutor() throws Exception {
+ LoadingCache<Integer, Integer> cache =
+ memoryCacheFactoryDirectExecutor.build(newCacheDef(1), newCacheLoader(identity()));
+
+ cache.put(TEST_CACHE_KEY, TEST_CACHE_VALUE);
+ cache.invalidate(TEST_CACHE_KEY);
+
+ assertThat(forwardingRemovalListener.contains(TEST_CACHE_KEY, TEST_CACHE_VALUE)).isTrue();
+ }
+
+ @Test
public void shouldLoadAllKeysWithDisabledCache() throws Exception {
LoadingCache<Integer, Integer> disabledCache =
memoryCacheFactory.build(newCacheDef(0), newCacheLoader(identity()));
@@ -146,6 +227,48 @@
};
}
+ private class ForwardingRemovalTrackerListener extends ForwardingRemovalListener<Object, Object> {
+ private final ConcurrentHashMap<Object, Set<Object>> removalEvents;
+ private final ConcurrentHashMap<Object, String> removalThreads;
+
+ public ForwardingRemovalTrackerListener() {
+ super(null, null);
+
+ removalEvents = new ConcurrentHashMap<>();
+ removalThreads = new ConcurrentHashMap<>();
+ }
+
+ @Override
+ public void onRemoval(RemovalNotification<Object, Object> notification) {
+ Set<Object> setOfValues =
+ removalEvents.computeIfAbsent(
+ notification.getKey(),
+ (key) -> {
+ Set<Object> elements = ConcurrentHashMap.newKeySet();
+ return elements;
+ });
+ setOfValues.add(notification.getValue());
+
+ removalThreads.put(notification.getKey(), Thread.currentThread().getName());
+
+ try {
+ evictionReceived.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+ } catch (InterruptedException | BrokenBarrierException | TimeoutException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private boolean contains(Object key, Object value) {
+ return Optional.ofNullable(removalEvents.get(key))
+ .map(sv -> sv.contains(value))
+ .orElse(false);
+ }
+
+ private String removalThreadName(Object key) {
+ return removalThreads.get(key);
+ }
+ }
+
private CacheDef<Integer, Integer> newCacheDef(long maximumWeight) {
return new CacheDef<>() {
diff --git a/javatests/com/google/gerrit/server/mail/send/MailSoySauceModuleTest.java b/javatests/com/google/gerrit/server/mail/send/MailSoySauceModuleTest.java
index 3ce60b8..5a6db42 100644
--- a/javatests/com/google/gerrit/server/mail/send/MailSoySauceModuleTest.java
+++ b/javatests/com/google/gerrit/server/mail/send/MailSoySauceModuleTest.java
@@ -25,6 +25,7 @@
import com.google.gerrit.server.cache.mem.DefaultMemoryCacheModule;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.git.WorkQueue;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
@@ -54,6 +55,7 @@
bind(SitePaths.class).toInstance(sitePaths);
bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(new Config());
bind(MetricMaker.class).to(DisabledMetricMaker.class);
+ install(new WorkQueue.WorkQueueModule());
install(new DefaultMemoryCacheModule());
}
});
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 9070ffc..0f6245f 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1611,12 +1611,25 @@
// create group and add users
AccountGroup.UUID external_group1 = AccountGroup.uuid("testbackend:group1");
AccountGroup.UUID external_group2 = AccountGroup.uuid("testbackend:group2");
+ String nameOfGroupThatContainsExternalGroupAsSubgroup = "test-group-1";
+ String nameOfGroupThatContainsExternalGroupAsSubSubgroup = "test-group-2";
testGroupBackend.create(external_group1);
testGroupBackend.create(external_group2);
testGroupBackend.setMembershipsOf(
user1, new ListGroupMembership(ImmutableList.of(external_group1)));
testGroupBackend.setMembershipsOf(
user2, new ListGroupMembership(ImmutableList.of(external_group2)));
+ AccountGroup.UUID uuidOfGroupThatContainsExternalGroupAsSubgroup =
+ groupOperations
+ .newGroup()
+ .name(nameOfGroupThatContainsExternalGroupAsSubgroup)
+ .addSubgroup(external_group1)
+ .create();
+ groupOperations
+ .newGroup()
+ .name(nameOfGroupThatContainsExternalGroupAsSubSubgroup)
+ .addSubgroup(uuidOfGroupThatContainsExternalGroupAsSubgroup)
+ .create();
Change change1 = insert("repo", newChange(repo), user1);
Change change2 = insert("repo", newChange(repo), user1);
@@ -1635,9 +1648,25 @@
assertQuery("label:Code-Review=+1," + external_group1.get(), change1);
assertQuery("label:Code-Review=+1,group=" + external_group1.get(), change1);
+ assertQuery(
+ "label:Code-Review=+1,group=" + nameOfGroupThatContainsExternalGroupAsSubgroup, change1);
+ assertQuery(
+ "label:Code-Review=+1,group=" + nameOfGroupThatContainsExternalGroupAsSubSubgroup, change1);
assertQuery("label:Code-Review=+1,user=" + user1, change1);
assertQuery("label:Code-Review=+1,user=" + user2);
assertQuery("label:Code-Review=+1,group=" + external_group2.get());
+
+ // Negated operator tests
+ assertQuery("-label:Code-Review=+1," + external_group1.get(), change2);
+ assertQuery("-label:Code-Review=+1,group=" + external_group1.get(), change2);
+ assertQuery(
+ "-label:Code-Review=+1,group=" + nameOfGroupThatContainsExternalGroupAsSubgroup, change2);
+ assertQuery(
+ "-label:Code-Review=+1,group=" + nameOfGroupThatContainsExternalGroupAsSubSubgroup,
+ change2);
+ assertQuery("-label:Code-Review=+1,user=" + user1, change2);
+ assertQuery("-label:Code-Review=+1,group=" + external_group2.get(), change2, change1);
+ assertQuery("-label:Code-Review=+1,user=" + user2, change2, change1);
}
@Test
diff --git a/modules/jgit b/modules/jgit
index 596c445..176f17d 160000
--- a/modules/jgit
+++ b/modules/jgit
@@ -1 +1 @@
-Subproject commit 596c445af22ed9b6e0b7e35de44c127fcb8ecf7d
+Subproject commit 176f17d05ec154ce455ab2bde7429017d43d67fb
diff --git a/polygerrit-ui/app/api/BUILD_for_publishing_api_only b/polygerrit-ui/app/api/BUILD_for_publishing_api_only
index 67a26cd..c1bb6bd 100644
--- a/polygerrit-ui/app/api/BUILD_for_publishing_api_only
+++ b/polygerrit-ui/app/api/BUILD_for_publishing_api_only
@@ -43,9 +43,13 @@
["**/*"],
exclude = [
"BUILD",
+ "BUILD_for_publishing_api_only",
"tsconfig.json",
"publish.sh",
],
),
- deps = [":js_plugin_api_compiled"],
+ deps = [
+ ":js_plugin_api_compiled",
+ "//plugins:tsconfig-plugins-base.json",
+ ],
)
diff --git a/polygerrit-ui/app/api/publish.sh b/polygerrit-ui/app/api/publish.sh
index 16de4c9..1d6368c 100755
--- a/polygerrit-ui/app/api/publish.sh
+++ b/polygerrit-ui/app/api/publish.sh
@@ -7,19 +7,24 @@
#
# Adding the `--upload` argument will also publish the package.
+set -e
+
bazel_bin=$(which bazelisk 2>/dev/null)
if [[ -z "$bazel_bin" ]]; then
echo "Warning: bazelisk is not installed; falling back to bazel."
bazel_bin=bazel
fi
api_path=polygerrit-ui/app/api
+plugins_path=plugins
function cleanup() {
echo "Cleaning up ..."
rm -f ${api_path}/BUILD
+ rm -f ${api_path}/tsconfig-plugins-base.json
}
trap cleanup EXIT
cp ${api_path}/BUILD_for_publishing_api_only ${api_path}/BUILD
+cp ${plugins_path}/tsconfig-plugins-base.json ${api_path}/tsconfig-plugins-base.json
${bazel_bin} build //${api_path}:js_plugin_api_npm_package
diff --git a/polygerrit-ui/app/api/tsconfig.json b/polygerrit-ui/app/api/tsconfig.json
index 037e4f2..6960192 100644
--- a/polygerrit-ui/app/api/tsconfig.json
+++ b/polygerrit-ui/app/api/tsconfig.json
@@ -1,9 +1,9 @@
{
"extends": "../../../plugins/tsconfig-plugins-base.json",
"compilerOptions": {
- "rootDir": ".",
+ "rootDir": "."
},
"include": [
- "**/*",
- ],
+ "**/*"
+ ]
}
diff --git a/polygerrit-ui/app/constants/reporting.ts b/polygerrit-ui/app/constants/reporting.ts
index 1877de7e..ad59edd 100644
--- a/polygerrit-ui/app/constants/reporting.ts
+++ b/polygerrit-ui/app/constants/reporting.ts
@@ -124,33 +124,6 @@
CHECKS_RUNS_PANEL_TOGGLE = 'checks-runs-panel-toggle',
CHECKS_RUNS_SELECTED_TRIGGERED = 'checks-runs-selected-triggered',
CHECKS_STATS = 'checks-stats',
- // The following interactions are logged for investigating a spurious bug of
- // auto-closing draft comments.
- COMMENTS_AUTOCLOSE_FIRST_UPDATE = 'comments-autoclose-first-update',
- COMMENTS_AUTOCLOSE_EDITING_FALSE_SAVE = 'comments-autoclose-editing-false-save',
- COMMENTS_AUTOCLOSE_EDITING_DISCONNECTED = 'comments-autoclose-editing-disconnected',
- COMMENTS_AUTOCLOSE_EDITING_THREAD_DISCONNECTED = 'comments-autoclose-editing-thread-disconnected',
- COMMENTS_AUTOCLOSE_CHECKS_UPDATED = 'comments-autoclose-checks-updated',
- COMMENTS_AUTOCLOSE_THREADS_UPDATED = 'comments-autoclose-threads-updated',
- COMMENTS_AUTOCLOSE_COMMENT_REMOVED = 'comments-autoclose-comment-removed',
- COMMENTS_AUTOCLOSE_MESSAGES_LIST_CREATED = 'comments-autoclose-messages-list-created',
- COMMENTS_AUTOCLOSE_MESSAGES_LIST_UPDATED = 'comments-autoclose-messages-list-updated',
- COMMENTS_AUTOCLOSE_THREAD_LIST_CREATED = 'comments-autoclose-thread-list-created',
- COMMENTS_AUTOCLOSE_THREAD_LIST_UPDATED = 'comments-autoclose-thread-list-updated',
- // The following interactions are logged for investigating a spurious bug of
- // auto-closing diffs.
- DIFF_AUTOCLOSE_DIFF_UNDEFINED = 'diff-autoclose-diff-undefined',
- DIFF_AUTOCLOSE_DIFF_ONGOING = 'diff-autoclose-diff-ongoing',
- DIFF_AUTOCLOSE_RELOAD_ON_WHITESPACE = 'diff-autoclose-reload-on-whitespace',
- DIFF_AUTOCLOSE_RELOAD_ON_SYNTAX = 'diff-autoclose-reload-on-syntax',
- DIFF_AUTOCLOSE_RELOAD_FILELIST_PREFS = 'diff-autoclose-reload-filelist-prefs',
- DIFF_AUTOCLOSE_DIFF_HOST_CREATED = 'diff-autoclose-diff-host-created',
- DIFF_AUTOCLOSE_DIFF_HOST_NOT_RENDERING = 'diff-autoclose-diff-not-rendering',
- DIFF_AUTOCLOSE_FILE_LIST_UPDATED = 'diff-autoclose-file-list-updated',
- DIFF_AUTOCLOSE_SHOWN_FILES_CHANGED = 'diff-autoclose-shown-files-changed',
- // The following interaction is logged for reporting and counting a suspected
- // Chrome bug that leads to html`` misbehavior.
- AUTOCLOSE_HTML_PATCHED = 'autoclose-html-patched',
CHANGE_ACTION_FIRED = 'change-action-fired',
BUTTON_CLICK = 'button-click',
LINK_CLICK = 'link-click',
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-plugin-config/gr-repo-plugin-config.ts b/polygerrit-ui/app/elements/admin/gr-repo-plugin-config/gr-repo-plugin-config.ts
index 2772519..fabf93d 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-plugin-config/gr-repo-plugin-config.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-plugin-config/gr-repo-plugin-config.ts
@@ -171,6 +171,7 @@
) {
return html`
<iron-input
+ .bindValue=${option.info.value ?? ''}
@input=${this._handleStringChange}
data-option-key=${option._key}
>
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index d07a6ac..bdb3e52 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -567,7 +567,7 @@
/** Simply reflects the router-model value. */
// visible for testing
- viewModelPatchNum?: PatchSetNum;
+ viewModelPatchNum?: RevisionPatchSetNum;
private readonly shortcutsController = new ShortcutController(this);
@@ -1208,7 +1208,6 @@
() => html`
<gr-reply-dialog
id="replyDialog"
- .patchNum=${computeLatestPatchNum(this.allPatchSets)}
.permittedLabels=${this.change?.permitted_labels}
.projectConfig=${this.projectConfig}
.canBeStarted=${this.canStartReview()}
@@ -1441,7 +1440,6 @@
id="relatedChanges"
.change=${this.change}
.mergeable=${this.mergeable}
- .patchNum=${computeLatestPatchNum(this.allPatchSets)}
></gr-related-changes-list>
</div>
<div class="emptySpace"></div>
@@ -1522,7 +1520,6 @@
<gr-file-list-header
id="fileListHeader"
.account=${this.account}
- .allPatchSets=${this.allPatchSets}
.change=${this.change}
.changeNum=${this.changeNum}
.commitInfo=${this.commitInfo}
@@ -2136,6 +2133,12 @@
// 2. We have to somehow trigger the change-model reloading. Otherwise
// this.change is not updated.
if (this.changeNum) {
+ if (!this._patchRange?.patchNum) {
+ this._patchRange = {
+ basePatchNum: PARENT,
+ patchNum: computeLatestPatchNum(this.allPatchSets),
+ };
+ }
fireReload(this);
return;
}
@@ -3155,19 +3158,10 @@
return;
}
- // Avoid putting patch set in the URL unless a non-latest patch set is
- // selected.
- assertIsDefined(this.patchRange, 'patchRange');
- let patchNum;
- if (
- !(this.patchRange.patchNum === computeLatestPatchNum(this.allPatchSets))
- ) {
- patchNum = this.patchRange.patchNum;
- }
this.getNavigation().setUrl(
createChangeUrl({
change: this.change,
- patchNum,
+ patchNum: this.viewModelPatchNum,
edit: true,
forceReload: true,
})
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index 34d11a3..577f925 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -2187,7 +2187,7 @@
const newChange = {...element.change};
newChange.revisions.rev2 = createRevision(2);
element.change = newChange;
- element.patchRange = {patchNum: 1 as RevisionPatchSetNum};
+ element.viewModelPatchNum = 1 as RevisionPatchSetNum;
await element.updateComplete;
fireEdit();
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
index 69297f6..f391d18 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
@@ -12,7 +12,6 @@
import '../gr-commit-info/gr-commit-info';
import {FilesExpandedState} from '../gr-file-list-constants';
import {navigationToken} from '../../core/gr-navigation/gr-navigation';
-import {computeLatestPatchNum, PatchSet} from '../../../utils/patch-set-util';
import {property, customElement, query, state} from 'lit/decorators.js';
import {
AccountInfo,
@@ -21,6 +20,7 @@
CommitInfo,
ServerInfo,
BasePatchSetNum,
+ PatchSetNumber,
} from '../../../types/common';
import {DiffPreferencesInfo} from '../../../types/diff';
import {GrDiffModeSelector} from '../../../embed/diff/gr-diff-mode-selector/gr-diff-mode-selector';
@@ -48,9 +48,6 @@
@property({type: Object})
account: AccountInfo | undefined;
- @property({type: Array})
- allPatchSets?: PatchSet[];
-
@property({type: Object})
change: ChangeInfo | undefined;
@@ -72,6 +69,8 @@
@property({type: String})
filesExpanded?: FilesExpandedState;
+ @state() latestPatchNum?: PatchSetNumber;
+
@state() patchNum?: PatchSetNum;
@state() basePatchNum?: BasePatchSetNum;
@@ -132,6 +131,11 @@
() => this.getChangeModel().basePatchNum$,
x => (this.basePatchNum = x)
);
+ subscribe(
+ this,
+ () => this.getChangeModel().latestPatchNum$,
+ x => (this.latestPatchNum = x)
+ );
}
static override styles = [
@@ -251,10 +255,7 @@
return;
}
const editModeClass = this.computeEditModeClass(this.editMode);
- const patchInfoClass = this.computePatchInfoClass(
- this.patchNum,
- this.allPatchSets
- );
+ const patchInfoClass = this.computePatchInfoClass();
const expandedClass = this.computeExpandedClass(this.filesExpanded);
return html`
<div class="patchInfo-header ${editModeClass} ${patchInfoClass}">
@@ -416,9 +417,8 @@
return editMode ? 'editMode' : '';
}
- computePatchInfoClass(patchNum?: PatchSetNum, allPatchSets?: PatchSet[]) {
- const latestNum = computeLatestPatchNum(allPatchSets);
- if (patchNum === latestNum) {
+ computePatchInfoClass() {
+ if (this.patchNum === this.latestPatchNum) {
return '';
}
return 'patchInfoOldPatchSet';
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
index 6c2282b..831366a 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.ts
@@ -23,7 +23,6 @@
PatchSetNumber,
} from '../../../types/common';
import {ChangeInfo, ChangeStatus} from '../../../api/rest-api';
-import {PatchSet} from '../../../utils/patch-set-util';
import {createDefaultDiffPrefs} from '../../../constants/constants';
import {fixture, html, assert} from '@open-wc/testing';
import {GrButton} from '../../shared/gr-button/gr-button';
@@ -250,23 +249,16 @@
});
test('class is applied to file list on old patch set', () => {
- const allPatchSets: PatchSet[] = [
- {num: 4 as PatchSetNumber, desc: undefined, sha: ''},
- {num: 2 as PatchSetNumber, desc: undefined, sha: ''},
- {num: 1 as PatchSetNumber, desc: undefined, sha: ''},
- ];
- assert.equal(
- element.computePatchInfoClass(1 as PatchSetNum, allPatchSets),
- 'patchInfoOldPatchSet'
- );
- assert.equal(
- element.computePatchInfoClass(2 as PatchSetNum, allPatchSets),
- 'patchInfoOldPatchSet'
- );
- assert.equal(
- element.computePatchInfoClass(4 as PatchSetNum, allPatchSets),
- ''
- );
+ element.latestPatchNum = 4 as PatchSetNumber;
+
+ element.patchNum = 1 as PatchSetNumber;
+ assert.equal(element.computePatchInfoClass(), 'patchInfoOldPatchSet');
+
+ element.patchNum = 2 as PatchSetNumber;
+ assert.equal(element.computePatchInfoClass(), 'patchInfoOldPatchSet');
+
+ element.patchNum = 4 as PatchSetNumber;
+ assert.equal(element.computePatchInfoClass(), '');
});
suite('editMode behavior', () => {
@@ -277,11 +269,6 @@
test('patch specific elements', async () => {
element.editMode = true;
- element.allPatchSets = [
- {num: 1 as PatchSetNumber, desc: undefined, sha: ''},
- {num: 2 as PatchSetNumber, desc: undefined, sha: ''},
- {num: 3 as PatchSetNumber, desc: undefined, sha: ''},
- ];
await element.updateComplete;
assert.isFalse(
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index ad75253..999190e 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -51,7 +51,7 @@
import {GrCursorManager} from '../../shared/gr-cursor-manager/gr-cursor-manager';
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
import {ParsedChangeInfo, PatchSetFile} from '../../../types/types';
-import {Interaction, Timing} from '../../../constants/reporting';
+import {Timing} from '../../../constants/reporting';
import {RevisionInfo} from '../../shared/revision-info/revision-info';
import {select} from '../../../utils/observable-util';
import {resolve} from '../../../models/dependency';
@@ -78,7 +78,6 @@
import {classMap} from 'lit/directives/class-map.js';
import {incrementalRepeat} from '../../lit/incremental-repeat';
import {ifDefined} from 'lit/directives/if-defined.js';
-import {HtmlPatched} from '../../../utils/lit-util';
import {
createDiffUrl,
createEditUrl,
@@ -309,13 +308,6 @@
private readonly getBrowserModel = resolve(this, browserModelToken);
- private readonly patched = new HtmlPatched(key => {
- this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, {
- component: this.tagName,
- key: key.substring(0, 300),
- });
- });
-
shortcutsController = new ShortcutController(this);
private readonly getNavigation = resolve(this, navigationToken);
@@ -1015,25 +1007,12 @@
);
}
- // for DIFF_AUTOCLOSE logging purposes only
- private shownFilesOld: NormalizedFileInfo[] = this.shownFiles;
-
private renderShownFiles() {
const showDynamicColumns = this.computeShowDynamicColumns();
const showPrependedDynamicColumns =
this.computeShowPrependedDynamicColumns();
const sizeBarLayout = this.computeSizeBarLayout();
- // for DIFF_AUTOCLOSE logging purposes only
- if (
- this.shownFilesOld.length > 0 &&
- this.shownFiles !== this.shownFilesOld
- ) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_SHOWN_FILES_CHANGED
- );
- }
- this.shownFilesOld = this.shownFiles;
return incrementalRepeat({
values: this.shownFiles,
mapFn: (f, i) =>
@@ -1084,7 +1063,7 @@
</div>
${when(
this.isFileExpanded(file.__path),
- () => this.patched.html`
+ () => html`
<gr-diff-host
?noAutoRender=${true}
?showLoadFailure=${true}
@@ -1658,17 +1637,6 @@
this.reporting.fileListDisplayed();
}
- protected override updated(): void {
- // for DIFF_AUTOCLOSE logging purposes only
- const ids = this.diffs.map(d => d.uid);
- if (ids.length > 0) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_FILE_LIST_UPDATED,
- {l: ids.length, ids: ids.slice(0, 10)}
- );
- }
- }
-
// TODO: Move into files-model.
// visible for testing
async updateCleanlyMergedPaths() {
@@ -1787,10 +1755,6 @@
if (!this.diffs.length) {
return;
}
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_RELOAD_FILELIST_PREFS
- );
-
// Re-render all expanded diffs sequentially.
this.renderInOrder(this.expandedFiles, this.diffs);
}
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
index 72969a7..45f24d7 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
@@ -128,9 +128,6 @@
private readonly getNavigation = resolve(this, navigationToken);
- // for COMMENTS_AUTOCLOSE logging purposes only
- readonly uid = performance.now().toString(36) + Math.random().toString(36);
-
constructor() {
super();
this.addEventListener('click', e => this.handleClick(e));
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
index c46f4fc..972b871 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
@@ -43,7 +43,6 @@
shortcutsServiceToken,
} from '../../../services/shortcuts/shortcuts-service';
import {GrFormattedText} from '../../shared/gr-formatted-text/gr-formatted-text';
-import {Interaction} from '../../../constants/reporting';
/**
* The content of the enum is also used in the UI for the button text.
@@ -353,21 +352,6 @@
this.changeNum = x;
}
);
- // for COMMENTS_AUTOCLOSE logging purposes only
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_MESSAGES_LIST_CREATED
- );
- }
-
- override updated(): void {
- // for COMMENTS_AUTOCLOSE logging purposes only
- const messages = this.shadowRoot!.querySelectorAll('gr-message');
- if (messages.length > 0) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_MESSAGES_LIST_UPDATED,
- {uid: messages[0].uid}
- );
- }
}
override willUpdate(changedProperties: PropertyValues): void {
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index cac9ae5..4aae939 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -16,7 +16,7 @@
import {
ChangeInfo,
CommitId,
- PatchSetNum,
+ PatchSetNumber,
RelatedChangeAndCommitInfo,
RelatedChangesInfo,
RevisionPatchSetNum,
@@ -33,6 +33,9 @@
} from '../../../utils/change-util';
import {DEFALT_NUM_CHANGES_WHEN_COLLAPSED} from './gr-related-collapse';
import {createChangeUrl} from '../../../models/views/change';
+import {subscribe} from '../../lit/subscription-controller';
+import {resolve} from '../../../models/dependency';
+import {changeModelToken} from '../../../models/change/change-model';
export interface ChangeMarkersInList {
showCurrentChangeArrow: boolean;
@@ -54,13 +57,13 @@
@property({type: Object})
change?: ParsedChangeInfo;
- @property({type: String})
- patchNum?: PatchSetNum;
-
@property({type: Boolean})
mergeable?: boolean;
@state()
+ latestPatchNum?: PatchSetNumber;
+
+ @state()
submittedTogether?: SubmittedTogetherInfo = {
changes: [],
non_visible_changes: 0,
@@ -80,6 +83,17 @@
private readonly restApiService = getAppContext().restApiService;
+ private readonly getChangeModel = resolve(this, changeModelToken);
+
+ constructor() {
+ super();
+ subscribe(
+ this,
+ () => this.getChangeModel().latestPatchNum$,
+ x => (this.latestPatchNum = x)
+ );
+ }
+
static override get styles() {
return [
sharedStyles,
@@ -197,7 +211,7 @@
);
const connectedRevisions = this._computeConnectedRevisions(
this.change,
- this.patchNum,
+ this.latestPatchNum,
this.relatedChanges
);
@@ -570,11 +584,12 @@
reload(getRelatedChanges?: Promise<RelatedChangesInfo | undefined>) {
const change = this.change;
if (!change) return Promise.reject(new Error('change missing'));
- if (!this.patchNum) return Promise.reject(new Error('patchNum missing'));
+ if (!this.latestPatchNum)
+ return Promise.reject(new Error('latestPatchNum missing'));
if (!getRelatedChanges) {
getRelatedChanges = this.restApiService.getRelatedChanges(
change._number,
- this.patchNum
+ this.latestPatchNum
);
}
const promises: Array<Promise<void>> = [
@@ -652,15 +667,15 @@
*/
_computeConnectedRevisions(
change?: ParsedChangeInfo,
- patchNum?: PatchSetNum,
+ latestPatchNum?: PatchSetNumber,
relatedChanges?: RelatedChangeAndCommitInfo[]
) {
- if (!patchNum || !relatedChanges || !change) {
+ if (!latestPatchNum || !relatedChanges || !change) {
return [];
}
const connected: CommitId[] = [];
- const changeRevision = getRevisionKey(change, patchNum);
+ const changeRevision = getRevisionKey(change, latestPatchNum);
const commits = relatedChanges.map(c => c.commit);
let pos = commits.length - 1;
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
index 3e90145..571b5b5 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
@@ -30,7 +30,7 @@
ChangeInfo,
CommitId,
NumericChangeId,
- PatchSetNum,
+ PatchSetNumber,
RelatedChangeAndCommitInfo,
RelatedChangesInfo,
SubmittedTogetherInfo,
@@ -192,7 +192,7 @@
setup(() => {
element.change = createParsedChange();
- element.patchNum = 1 as PatchSetNum;
+ element.latestPatchNum = 1 as PatchSetNumber;
});
test('render', async () => {
@@ -378,7 +378,7 @@
});
test('request conflicts if open and mergeable', () => {
- element.patchNum = 7 as PatchSetNum;
+ element.latestPatchNum = 7 as PatchSetNumber;
element.change = {
...createParsedChange(),
change_id: '123' as ChangeId,
@@ -390,7 +390,7 @@
});
test('does not request conflicts if closed and mergeable', () => {
- element.patchNum = 7 as PatchSetNum;
+ element.latestPatchNum = 7 as PatchSetNumber;
element.change = {
...createParsedChange(),
change_id: '123' as ChangeId,
@@ -401,7 +401,7 @@
});
test('does not request conflicts if open and not mergeable', () => {
- element.patchNum = 7 as PatchSetNum;
+ element.latestPatchNum = 7 as PatchSetNumber;
element.change = {
...createParsedChange(),
change_id: '123' as ChangeId,
@@ -413,7 +413,7 @@
});
test('doesnt request conflicts if closed and not mergeable', () => {
- element.patchNum = 7 as PatchSetNum;
+ element.latestPatchNum = 7 as PatchSetNumber;
element.change = {
...createParsedChange(),
change_id: '123' as ChangeId,
@@ -438,7 +438,7 @@
'9e593f6dcc2c0785a2ad2c895a34ad2aa9a0d8b6': createRevision(4),
},
};
- let patchNum = 7 as PatchSetNum;
+ let latestPatchNum = 7 as PatchSetNumber;
let relatedChanges: RelatedChangeAndCommitInfo[] = [
{
...createRelatedChangeAndCommitInfo(),
@@ -528,7 +528,7 @@
let connectedChanges = element._computeConnectedRevisions(
change,
- patchNum,
+ latestPatchNum,
relatedChanges
);
assert.deepEqual(connectedChanges, [
@@ -541,7 +541,7 @@
'2cebeedfb1e80f4b872d0a13ade529e70652c0c8',
]);
- patchNum = 4 as PatchSetNum;
+ latestPatchNum = 4 as PatchSetNumber;
relatedChanges = [
{
...createRelatedChangeAndCommitInfo(),
@@ -631,7 +631,7 @@
connectedChanges = element._computeConnectedRevisions(
change,
- patchNum,
+ latestPatchNum,
relatedChanges
);
assert.deepEqual(connectedChanges, [
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.ts
index 25e3e51..3a88eaf 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.ts
@@ -16,7 +16,7 @@
import {
AccountId,
NumericChangeId,
- PatchSetNum,
+ PatchSetNumber,
Timestamp,
} from '../../../types/common';
import {createChange} from '../../../test/test-data-generators';
@@ -27,7 +27,7 @@
suite('gr-reply-dialog-it tests', () => {
let element: GrReplyDialog;
let changeNum: NumericChangeId;
- let patchNum: PatchSetNum;
+ let latestPatchNum: PatchSetNumber;
const setupElement = (element: GrReplyDialog) => {
element.change = {
@@ -55,7 +55,7 @@
},
},
};
- element.patchNum = patchNum;
+ element.latestPatchNum = latestPatchNum;
element.permittedLabels = {
'Code-Review': ['-1', ' 0', '+1'],
Verified: ['-1', ' 0', '+1'],
@@ -64,7 +64,7 @@
setup(async () => {
changeNum = 42 as NumericChangeId;
- patchNum = 1 as PatchSetNum;
+ latestPatchNum = 1 as PatchSetNumber;
stubRestApi('getAccount').returns(
Promise.resolve({
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index a2ffcb8..d899771 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -54,7 +54,6 @@
isReviewerAccountSuggestion,
isReviewerGroupSuggestion,
ParsedJSON,
- PatchSetNum,
ReviewerInput,
ReviewInput,
ReviewResult,
@@ -187,9 +186,6 @@
@property({type: Object})
change?: ParsedChangeInfo | ChangeInfo;
- @property({type: String})
- patchNum?: PatchSetNum;
-
@property({type: Boolean})
canBeStarted = false;
@@ -220,6 +216,8 @@
@query('#reviewerConfirmationModal')
reviewerConfirmationModal?: HTMLDialogElement;
+ @state() latestPatchNum?: PatchSetNumber;
+
@state() serverConfig?: ServerInfo;
@state()
@@ -361,8 +359,6 @@
private readonly getUserModel = resolve(this, userModelToken);
- private latestPatchNum?: PatchSetNumber;
-
storeTask?: DelayedTask;
private isLoggedIn = false;
@@ -1172,7 +1168,7 @@
this.knownLatestState === LatestPatchState.CHECKING,
() => html`
<span id="checkingStatusLabel">
- Checking whether patch ${this.patchNum} is latest...
+ Checking whether patch ${this.latestPatchNum} is latest...
</span>
`
)}
@@ -1868,10 +1864,10 @@
saveReview(review: ReviewInput, errFn?: ErrorCallback) {
assertIsDefined(this.change, 'change');
- assertIsDefined(this.patchNum, 'patchNum');
+ assertIsDefined(this.latestPatchNum, 'latestPatchNum');
return this.restApiService.saveChangeReview(
this.change._number,
- this.patchNum,
+ this.latestPatchNum,
review,
errFn
);
@@ -2027,7 +2023,7 @@
}
computePatchSetWarning() {
- let str = `Patch ${this.patchNum} is not latest.`;
+ let str = `Patch ${this.latestPatchNum} is not latest.`;
if (this.labelsChanged) {
str += ' Voting may have no effect.';
}
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 5bf6464..22dd4b5 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -39,7 +39,7 @@
GroupId,
GroupName,
NumericChangeId,
- PatchSetNum,
+ PatchSetNumber,
ReviewerInput,
ReviewInput,
ReviewResult,
@@ -89,7 +89,7 @@
suite('gr-reply-dialog tests', () => {
let element: GrReplyDialog;
let changeNum: NumericChangeId;
- let patchNum: PatchSetNum;
+ let latestPatchNum: PatchSetNumber;
let commentsModel: CommentsModel;
let lastId = 1;
@@ -105,7 +105,7 @@
setup(async () => {
changeNum = 42 as NumericChangeId;
- patchNum = 1 as PatchSetNum;
+ latestPatchNum = 1 as PatchSetNumber;
stubRestApi('getChange').returns(Promise.resolve({...createChange()}));
stubRestApi('getChangeSuggestedReviewers').returns(Promise.resolve([]));
@@ -143,7 +143,7 @@
},
},
};
- element.patchNum = patchNum;
+ element.latestPatchNum = latestPatchNum;
element.permittedLabels = {
'Code-Review': ['-1', ' 0', '+1'],
Verified: ['-1', ' 0', '+1'],
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
index 599e38b..7015900 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
@@ -42,11 +42,8 @@
import {ParsedChangeInfo} from '../../../types/types';
import {repeat} from 'lit/directives/repeat.js';
import {GrCommentThread} from '../../shared/gr-comment-thread/gr-comment-thread';
-import {getAppContext} from '../../../services/app-context';
import {resolve} from '../../../models/dependency';
import {changeModelToken} from '../../../models/change/change-model';
-import {Interaction} from '../../../constants/reporting';
-import {HtmlPatched} from '../../../utils/lit-util';
import {userModelToken} from '../../../models/user/user-model';
import {specialFilePathCompare} from '../../../utils/path-list-util';
@@ -206,17 +203,8 @@
private readonly getChangeModel = resolve(this, changeModelToken);
- private readonly reporting = getAppContext().reportingService;
-
private readonly getUserModel = resolve(this, userModelToken);
- private readonly patched = new HtmlPatched(key => {
- this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, {
- component: this.tagName,
- key: key.substring(0, 300),
- });
- });
-
constructor() {
super();
subscribe(
@@ -234,10 +222,6 @@
() => this.getUserModel().account$,
x => (this.account = x)
);
- // for COMMENTS_AUTOCLOSE logging purposes only
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_THREAD_LIST_CREATED
- );
}
override willUpdate(changed: PropertyValues) {
@@ -341,17 +325,6 @@
];
}
- override updated(): void {
- // for COMMENTS_AUTOCLOSE logging purposes only
- const threads = this.shadowRoot!.querySelectorAll('gr-comment-thread');
- if (threads.length > 0) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_THREAD_LIST_UPDATED,
- {uid: threads[0].uid}
- );
- }
- }
-
override render() {
return html`
${this.renderDropdown()}
@@ -425,16 +398,16 @@
index === 0 || threads[index - 1].path !== threads[index].path;
const separator =
index !== 0 && isFirst
- ? this.patched.html`<div class="thread-separator"></div>`
+ ? html`<div class="thread-separator"></div>`
: undefined;
const commentThread = this.renderCommentThread(thread, isFirst);
- return this.patched.html`${separator}${commentThread}`;
+ return html`${separator}${commentThread}`;
}
);
}
private renderCommentThread(thread: CommentThread, isFirst: boolean) {
- return this.patched.html`
+ return html`
<gr-comment-thread
.thread=${thread}
show-file-path
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index f3be5c4..5071e4f 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -72,7 +72,6 @@
import {changeModelToken} from '../../models/change/change-model';
import {getAppContext} from '../../services/app-context';
import {when} from 'lit/directives/when.js';
-import {HtmlPatched} from '../../utils/lit-util';
import {DropdownItem} from '../shared/gr-dropdown-list/gr-dropdown-list';
import './gr-checks-attempt';
import {createDiffUrl, changeViewModelToken} from '../../models/views/change';
@@ -818,13 +817,6 @@
private readonly reporting = getAppContext().reportingService;
- private readonly patched = new HtmlPatched(key => {
- this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, {
- component: this.tagName,
- key: key.substring(0, 300),
- });
- });
-
constructor() {
super();
subscribe(
@@ -1502,7 +1494,7 @@
${repeat(
filtered,
result => result.internalResultId,
- (result?: RunResult) => this.patched.html`
+ (result?: RunResult) => html`
<gr-result-row
class=${charsOnly(result!.checkName)}
.result=${result}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 43a6819..0fa5503 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -64,7 +64,7 @@
import {assertIsDefined} from '../../../utils/common-util';
import {DiffContextExpandedEventDetail} from '../../../embed/diff/gr-diff-builder/gr-diff-builder';
import {TokenHighlightLayer} from '../../../embed/diff/gr-diff-builder/token-highlight-layer';
-import {Timing, Interaction} from '../../../constants/reporting';
+import {Timing} from '../../../constants/reporting';
import {ChangeComments} from '../gr-comment-api/gr-comment-api';
import {Subscription} from 'rxjs';
import {
@@ -336,9 +336,6 @@
private checksSubscription?: Subscription;
- // for DIFF_AUTOCLOSE logging purposes only
- readonly uid = performance.now().toString(36) + Math.random().toString(36);
-
constructor() {
super();
this.syntaxLayer = new GrSyntaxLayerWorker(
@@ -386,23 +383,6 @@
this.prefs = diffPreferences;
}
);
- this.logForDiffAutoClose();
- }
-
- // for DIFF_AUTOCLOSE logging purposes only
- private logForDiffAutoClose() {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_DIFF_HOST_CREATED,
- {uid: this.uid}
- );
- setTimeout(() => {
- if (!this.hasReloadBeenCalledOnce) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_DIFF_HOST_NOT_RENDERING,
- {uid: this.uid}
- );
- }
- }, /* 10 seconds */ 10000);
}
override connectedCallback() {
@@ -586,14 +566,7 @@
return this.reloadPromise;
}
- // for DIFF_AUTOCLOSE logging purposes only
- private reloadOngoing = false;
-
- // for DIFF_AUTOCLOSE logging purposes only
- private hasReloadBeenCalledOnce = false;
-
async reloadInternal(shouldReportMetric?: boolean) {
- this.hasReloadBeenCalledOnce = true;
this.reporting.time(Timing.DIFF_TOTAL);
this.reporting.time(Timing.DIFF_LOAD);
// TODO: Find better names for these 3 clear/cancel methods. Ideally the
@@ -606,10 +579,6 @@
this.diff = undefined;
this.errorMessage = null;
const whitespaceLevel = this.getIgnoreWhitespace();
- if (this.reloadOngoing) {
- this.reporting.reportInteraction(Interaction.DIFF_AUTOCLOSE_DIFF_ONGOING);
- }
- this.reloadOngoing = true;
try {
// We are carefully orchestrating operations that have to wait for another
@@ -619,11 +588,6 @@
// assets in parallel.
const layerPromise = this.initLayers();
const diff = await this.getDiff();
- if (diff === undefined) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_DIFF_UNDEFINED
- );
- }
this.loadedWhitespaceLevel = whitespaceLevel;
this.reportDiff(diff);
@@ -670,7 +634,6 @@
}
} finally {
this.reporting.timeEnd(Timing.DIFF_TOTAL, this.timingDetails());
- this.reloadOngoing = false;
}
}
@@ -758,9 +721,6 @@
const idToEl = new Map<string, GrDiffCheckResult>();
const checkEls = this.getCheckEls();
const dontRemove = new Set<GrDiffCheckResult>();
- let createdCount = 0;
- let updatedCount = 0;
- let removedCount = 0;
const checksCount = checks.length;
const checkElsCount = checkEls.length;
if (checksCount === 0 && checkElsCount === 0) return;
@@ -775,23 +735,16 @@
if (existingEl) {
existingEl.result = check;
dontRemove.add(existingEl);
- updatedCount++;
} else {
const newEl = this.createCheckEl(check);
dontRemove.add(newEl);
- createdCount++;
}
}
// Remove all check els that don't have a matching check anymore.
for (const el of checkEls) {
if (dontRemove.has(el)) continue;
el.remove();
- removedCount++;
}
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_CHECKS_UPDATED,
- {createdCount, updatedCount, removedCount, checksCount, checkElsCount}
- );
}
/**
@@ -1106,9 +1059,6 @@
}
}
const dontRemove = new Set<GrCommentThread>();
- let createdCount = 0;
- let updatedCount = 0;
- let removedCount = 0;
const threadCount = threads.length;
const threadElCount = threadEls.length;
if (threadCount === 0 && threadElCount === 0) return;
@@ -1150,12 +1100,10 @@
) {
existingThreadEl.thread = thread;
dontRemove.add(existingThreadEl);
- updatedCount++;
} else {
const threadEl = this.createThreadElement(thread);
this.attachThreadElement(threadEl);
dontRemove.add(threadEl);
- createdCount++;
}
}
// Remove all threads that are no longer existing.
@@ -1165,13 +1113,8 @@
// might be unsaved and thus not be reflected in `threads` yet, so let's
// keep them open.
if (threadEl.editing && threadEl.thread?.comments.length === 0) continue;
- removedCount++;
threadEl.remove();
}
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_THREADS_UPDATED,
- {createdCount, updatedCount, removedCount, threadCount, threadElCount}
- );
const portedThreadsCount = threads.filter(thread => thread.ported).length;
const portedThreadsWithoutRange = threads.filter(
thread => thread.ported && thread.rangeInfoLost
@@ -1371,9 +1314,6 @@
preferredWhitespaceLevel !== loadedWhitespaceLevel &&
!noRenderOnPrefsChange
) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_RELOAD_ON_WHITESPACE
- );
return this.reload();
}
}
@@ -1392,9 +1332,6 @@
if (oldPrefs?.syntax_highlighting === prefs.syntax_highlighting) return;
if (!noRenderOnPrefsChange) {
- this.reporting.reportInteraction(
- Interaction.DIFF_AUTOCLOSE_RELOAD_ON_SYNTAX
- );
return this.reload();
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 062aa54..aba0aea 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -70,8 +70,6 @@
import {commentsModelToken} from '../../../models/comments/comments-model';
import {changeModelToken} from '../../../models/change/change-model';
import {whenRendered} from '../../../utils/dom-util';
-import {Interaction} from '../../../constants/reporting';
-import {HtmlPatched} from '../../../utils/lit-util';
import {createChangeUrl, createDiffUrl} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
@@ -254,8 +252,6 @@
private readonly getUserModel = resolve(this, userModelToken);
- private readonly reporting = getAppContext().reportingService;
-
private readonly shortcuts = new ShortcutController(this);
private readonly syntaxLayer = new GrSyntaxLayerWorker(
@@ -263,16 +259,6 @@
() => getAppContext().reportingService
);
- // for COMMENTS_AUTOCLOSE logging purposes only
- readonly uid = performance.now().toString(36) + Math.random().toString(36);
-
- private readonly patched = new HtmlPatched(key => {
- this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, {
- component: this.tagName,
- key: key.substring(0, 300),
- });
- });
-
constructor() {
super();
this.shortcuts.addGlobal({key: 'e'}, () => this.handleExpandShortcut());
@@ -322,15 +308,6 @@
);
}
- override disconnectedCallback() {
- if (this.editing) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_EDITING_THREAD_DISCONNECTED
- );
- }
- super.disconnectedCallback();
- }
-
static override get styles() {
return [
a11yStyles,
@@ -519,16 +496,15 @@
(this.messageId
? comment.change_message_id !== this.messageId
: !this.unresolved);
- return this.patched.html`
+ return html`
<gr-comment
.comment=${comment}
.comments=${this.thread!.comments}
?initially-collapsed=${initiallyCollapsed}
?robot-button-disabled=${robotButtonDisabled}
?show-patchset=${this.showPatchset}
- ?show-ported-comment=${
- this.showPortedComment && comment.id === this.rootId
- }
+ ?show-ported-comment=${this.showPortedComment &&
+ comment.id === this.rootId}
@reply-to-comment=${this.handleReplyToComment}
@copy-comment-link=${this.handleCopyLink}
@comment-editing-changed=${(
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 0edfe03..f7e8edd 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -57,7 +57,6 @@
import {Subject} from 'rxjs';
import {debounceTime} from 'rxjs/operators';
import {changeModelToken} from '../../../models/change/change-model';
-import {Interaction} from '../../../constants/reporting';
import {KnownExperimentId} from '../../../services/flags/flags';
import {isBase64FileContent} from '../../../api/rest-api';
import {createDiffUrl} from '../../../models/views/change';
@@ -318,11 +317,6 @@
override disconnectedCallback() {
// Clean up emoji dropdown.
if (this.textarea) this.textarea.closeDropdown();
- if (this.editing) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_EDITING_DISCONNECTED
- );
- }
super.disconnectedCallback();
}
@@ -938,10 +932,6 @@
this.unresolved = this.comment.unresolved ?? true;
if (isUnsaved(this.comment)) this.editing = true;
if (isDraftOrUnsaved(this.comment)) {
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_FIRST_UPDATE,
- {editing: this.editing, unsaved: isUnsaved(this.comment)}
- );
this.collapsed = false;
} else {
this.collapsed = !!this.initiallyCollapsed;
@@ -1210,9 +1200,6 @@
await this.rawSave(messageToSave, {showToast: true});
}
}
- this.reporting.reportInteraction(
- Interaction.COMMENTS_AUTOCLOSE_EDITING_FALSE_SAVE
- );
if (!this.permanentEditingMode) {
this.editing = false;
}
diff --git a/polygerrit-ui/app/utils/lit-util.ts b/polygerrit-ui/app/utils/lit-util.ts
deleted file mode 100644
index 7ffab89..0000000
--- a/polygerrit-ui/app/utils/lit-util.ts
+++ /dev/null
@@ -1,69 +0,0 @@
-/**
- * @license
- * Copyright 2022 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {html} from 'lit';
-
-/**
- * This is a patched version of html`` to work around this Chrome bug:
- * https://bugs.chromium.org/p/v8/issues/detail?id=13190.
- *
- * The problem is that Chrome should guarantee that the TemplateStringsArray
- * is always the same instance, if the strings themselves are equal, but that
- * guarantee seems to be broken. So we are maintaining a map from
- * "concatenated strings" to TemplateStringsArray. If "concatenated strings"
- * are equal, then return the already known instance of TemplateStringsArray,
- * so html`` can use its strict equality check on it.
- */
-export class HtmlPatched {
- constructor(private readonly reporter?: (key: string) => void) {}
-
- /**
- * If `strings` are in this set, then we are sure that they are also in the
- * map, and that we will not run into the issue of "same key, but different
- * strings array". So this set allows us to optimize performance a bit, and
- * call the native html`` function early.
- */
- private readonly lookupSet = new Set<TemplateStringsArray>();
-
- private readonly lookupMap = new Map<string, TemplateStringsArray>();
-
- /**
- * Proxies lit's html`` tagges template literal. See
- * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
- * https://lit.dev/docs/libraries/standalone-templates/
- *
- * Example: If you call html`a${1}b${2}c`, then
- * ['a', 'b', 'c'] are the "strings", and 1, 2 are the "values".
- */
- html(strings: TemplateStringsArray, ...values: unknown[]) {
- if (this.lookupSet.has(strings)) {
- return this.nativeHtml(strings, ...values);
- }
-
- const key = strings.join('\0');
- const oldStrings = this.lookupMap.get(key);
-
- if (oldStrings === undefined) {
- this.lookupSet.add(strings);
- this.lookupMap.set(key, strings);
- return this.nativeHtml(strings, ...values);
- }
-
- if (oldStrings === strings) {
- return this.nativeHtml(strings, ...values);
- }
-
- // Without using HtmlPatcher html`` would be called with `strings`,
- // which will be considered different, although actually being equal.
- console.warn(`HtmlPatcher was required for '${key.substring(0, 100)}'.`);
- this.reporter?.(key);
- return this.nativeHtml(oldStrings, ...values);
- }
-
- // Allows spying on calls in tests.
- nativeHtml(strings: TemplateStringsArray, ...values: unknown[]) {
- return html(strings, ...values);
- }
-}
diff --git a/polygerrit-ui/app/utils/lit-util_test.ts b/polygerrit-ui/app/utils/lit-util_test.ts
deleted file mode 100644
index 17197f0..0000000
--- a/polygerrit-ui/app/utils/lit-util_test.ts
+++ /dev/null
@@ -1,118 +0,0 @@
-/**
- * @license
- * Copyright 2020 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {assert} from '@open-wc/testing';
-import '../test/common-test-setup';
-import {HtmlPatched} from './lit-util';
-
-function tsa(strings: string[]): TemplateStringsArray {
- return strings as unknown as TemplateStringsArray;
-}
-
-suite('lit-util HtmlPatched tests', () => {
- let patched: HtmlPatched;
- let nativeHtmlSpy: sinon.SinonSpy;
- let reporterSpy: sinon.SinonSpy;
-
- setup(async () => {
- reporterSpy = sinon.spy();
- patched = new HtmlPatched(reporterSpy);
- nativeHtmlSpy = sinon.spy(patched, 'nativeHtml');
- });
-
- test('simple call', () => {
- const instance1 = tsa(['1']);
- patched.html(instance1, 'a value');
- assert.equal(nativeHtmlSpy.callCount, 1);
- assert.equal(reporterSpy.callCount, 0);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].args[0], instance1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].args[1], 'a value');
- });
-
- test('two calls, same instance', () => {
- const instance1 = tsa(['1']);
- patched.html(instance1, 'a value');
- patched.html(instance1, 'a value');
- assert.equal(nativeHtmlSpy.callCount, 2);
- assert.equal(reporterSpy.callCount, 0);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1);
- });
-
- test('two calls, different strings', () => {
- const instance1 = tsa(['1']);
- const instance2 = tsa(['2']);
- patched.html(instance1, 'a value');
- patched.html(instance2, 'a value');
- assert.equal(nativeHtmlSpy.callCount, 2);
- assert.equal(reporterSpy.callCount, 0);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance2);
- });
-
- test('two calls, same strings, different instances', () => {
- const instance1 = tsa(['1']);
- const instance2 = tsa(['1']);
- patched.html(instance1, 'a value');
- patched.html(instance2, 'a value');
- assert.equal(nativeHtmlSpy.callCount, 2);
- assert.equal(reporterSpy.callCount, 1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1);
- assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1);
- });
-
- test('many calls', () => {
- const instance1a = tsa(['1']);
- const instance1b = tsa(['1']);
- const instance1c = tsa(['1']);
- const instance2a = tsa(['asdf', 'qwer']);
- const instance2b = tsa(['asdf', 'qwer']);
- const instance2c = tsa(['asdf', 'qwer']);
- const instance3a = tsa(['asd', 'fqwer']);
- const instance3b = tsa(['asd', 'fqwer']);
- const instance3c = tsa(['asd', 'fqwer']);
-
- patched.html(instance1a, 'a value');
- patched.html(instance1a, 'a value');
- patched.html(instance1b, 'a value');
- patched.html(instance1b, 'a value');
- patched.html(instance1c, 'a value');
- patched.html(instance1c, 'a value');
- patched.html(instance2a, 'a value');
- patched.html(instance2a, 'a value');
- patched.html(instance2b, 'a value');
- patched.html(instance2b, 'a value');
- patched.html(instance2c, 'a value');
- patched.html(instance2c, 'a value');
- patched.html(instance3a, 'a value');
- patched.html(instance3a, 'a value');
- patched.html(instance3b, 'a value');
- patched.html(instance3b, 'a value');
- patched.html(instance3c, 'a value');
- patched.html(instance3c, 'a value');
-
- assert.equal(nativeHtmlSpy.callCount, 18);
- assert.equal(reporterSpy.callCount, 12);
-
- assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[2].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[3].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[4].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[5].firstArg, instance1a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[6].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[7].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[8].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[9].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[10].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[11].firstArg, instance2a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[12].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[13].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[14].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[15].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[16].firstArg, instance3a);
- assert.strictEqual(nativeHtmlSpy.getCalls()[17].firstArg, instance3a);
- });
-});
