Merge "Move LoadingStatus to types"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 6b1dea5..d91590e 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -828,8 +828,7 @@
[[cache.name.maxAge]]cache.<name>.maxAge::
+
-Maximum age to keep an entry in the cache. Entries are removed from
-the cache and refreshed from source data every maxAge interval.
+Maximum age to keep an entry in the cache.
Values should use common unit suffixes to express their setting:
+
* s, sec, second, seconds
@@ -924,20 +923,6 @@
+
If 0 or negative, disk storage for the cache is disabled.
-[[cache.name.expireAfterWrite]]cache.<name>.expireAfterWrite::
-+
-Duration after which a cached value will be evicted and not
-read anymore.
-+
-Values should use common unit suffixes to express their setting:
-+
-* ms, milliseconds
-* s, sec, second, seconds
-* m, min, minute, minutes
-* h, hr, hour, hours
-+
-Disabled by default.
-
[[cache.name.refreshAfterWrite]]cache.<name>.refreshAfterWrite::
+
Duration after which we asynchronously refresh the cached value.
@@ -2618,6 +2603,10 @@
same `serverId` (i.e.: a Gerrit cluster).
Unlike `instanceName` this value is not available in the email templates.
+The instance ID can also be configured by setting the Java system property
+`gerrit.instanceId` on startup. This will override the configuration in the
+gerrit.config.
+
[[gerrit.instanceName]]gerrit.instanceName::
+
Short identifier for this Gerrit instance.
diff --git a/Documentation/dev-eclipse.txt b/Documentation/dev-eclipse.txt
index a76282e..f4238d1 100644
--- a/Documentation/dev-eclipse.txt
+++ b/Documentation/dev-eclipse.txt
@@ -82,9 +82,10 @@
link:dev-build-plugins.html#_bundle_custom_plugin_in_release_war[bundling in release.war]
and run `tools/eclipse/project.py`.
-If a plugin requires additional test dependencies (not available in the Gerrit), then in order to
-execute tests directly from Eclipse, that plugin must be also added to `CUSTOM_PLUGINS_TEST_DEPS`
-list in `tools/bzl/plugins.bzl` (note that `tools/eclipse/project.py` has to be run again).
+If a plugin requires additional test dependencies (not available in the Gerrit),
+then in order to execute tests directly from Eclipse, that plugin must be also
+added to `CUSTOM_PLUGINS_TEST_DEPS` list in `tools/bzl/plugins.bzl` and Eclipse
+project configuration needs to be updated by running `tools/eclipse/project.py`.
== Java Versions
diff --git a/Documentation/pgm-reindex.txt b/Documentation/pgm-reindex.txt
index b74829d..183c132 100644
--- a/Documentation/pgm-reindex.txt
+++ b/Documentation/pgm-reindex.txt
@@ -39,6 +39,12 @@
--show-cache-stats::
Show cache statistics at the end of program.
+--build-bloom-filter::
+ Whether to build bloom filters for H2 disk caches. When using fully
+ populated disk caches on large Gerrit sites, it is recommended that
+ bloom filters are disabled to improve performance.
+
+
== CONTEXT
The secondary index must be enabled. See
link:config-gerrit.html#index.type[index.type].
diff --git a/Documentation/user-review-ui.txt b/Documentation/user-review-ui.txt
index 39929e1..3f50afe 100644
--- a/Documentation/user-review-ui.txt
+++ b/Documentation/user-review-ui.txt
@@ -280,9 +280,14 @@
** [[cherry-pick]]`Cherry-Pick`:
+
-Allows to cherry-pick the change to another branch. The destination
-branch can be selected from a dialog. Cherry-picking a change creates a
-new open change on the selected destination branch.
+Allows to cherry-pick the change to another branch. The destination branch
+can be selected from a dialog. Cherry-picking a change creates a new open
+change on the selected destination branch. 'Cherry-pick committer email'
+drop-down is visible for single change cherry-picks when user has more than
+one email registered to their account. It is possible to select any of the
+registered emails to be used as the cherry-pick committer email. It defaults
+to source commit's committer email if it is a registered email of the calling
+user, else defaults to calling user's preferred email.
+
It is also possible to cherry-pick a change to the same branch. This is
effectively the same as rebasing it to the current tip of the
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index a6ba8df..0539582 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -51,6 +51,8 @@
import com.google.common.testing.FakeTicker;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
import com.google.gerrit.acceptance.PushOneCommit.Result;
+import com.google.gerrit.acceptance.config.ConfigAnnotationParser;
+import com.google.gerrit.acceptance.config.GerritSystemProperty;
import com.google.gerrit.acceptance.testsuite.account.TestSshKeys;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
@@ -437,6 +439,14 @@
GerritServer.Description.forTestMethod(description, configName);
testMethodDescription = methodDesc;
+ if (methodDesc.systemProperties() != null) {
+ ConfigAnnotationParser.parse(methodDesc.systemProperties());
+ }
+
+ if (methodDesc.systemProperty() != null) {
+ ConfigAnnotationParser.parse(methodDesc.systemProperty());
+ }
+
testRequiresSsh = classDesc.useSshAnnotation() || methodDesc.useSshAnnotation();
if (!testRequiresSsh) {
baseConfig.setString("sshd", null, "listenAddress", "off");
@@ -701,6 +711,19 @@
server.close();
server = null;
}
+
+ GerritServer.Description methodDesc =
+ GerritServer.Description.forTestMethod(description, configName);
+ if (methodDesc.systemProperties() != null) {
+ for (GerritSystemProperty sysProp : methodDesc.systemProperties().value()) {
+ System.clearProperty(sysProp.name());
+ }
+ }
+
+ if (methodDesc.systemProperty() != null) {
+ System.clearProperty(methodDesc.systemProperty().name());
+ }
+
SystemReader.setInstance(oldSystemReader);
oldSystemReader = null;
// Set useDefaultTicker in afterTest, so the next beforeTest will use the default ticker
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index 7c6df1e..73631e9 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -31,6 +31,8 @@
import com.google.gerrit.acceptance.config.ConfigAnnotationParser;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.config.GerritConfigs;
+import com.google.gerrit.acceptance.config.GerritSystemProperties;
+import com.google.gerrit.acceptance.config.GerritSystemProperty;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
import com.google.gerrit.acceptance.config.GlobalPluginConfigs;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
@@ -137,6 +139,8 @@
false, // @UseSystemTime is only valid on methods.
get(UseClockStep.class, testDesc.getTestClass()),
get(UseTimezone.class, testDesc.getTestClass()),
+ null, // @GerritSystemProperty is only valid on methods.
+ null, // @GerritSystemProperties is only valid on methods.
null, // @GerritConfig is only valid on methods.
null, // @GerritConfigs is only valid on methods.
null, // @GlobalPluginConfig is only valid on methods.
@@ -179,6 +183,8 @@
testDesc.getAnnotation(UseTimezone.class) != null
? testDesc.getAnnotation(UseTimezone.class)
: get(UseTimezone.class, testDesc.getTestClass()),
+ testDesc.getAnnotation(GerritSystemProperty.class),
+ testDesc.getAnnotation(GerritSystemProperties.class),
testDesc.getAnnotation(GerritConfig.class),
testDesc.getAnnotation(GerritConfigs.class),
testDesc.getAnnotation(GlobalPluginConfig.class),
@@ -234,6 +240,12 @@
abstract UseTimezone useTimezone();
@Nullable
+ abstract GerritSystemProperty systemProperty();
+
+ @Nullable
+ abstract GerritSystemProperties systemProperties();
+
+ @Nullable
abstract GerritConfig config();
@Nullable
@@ -249,6 +261,10 @@
if (useClockStep() != null && useSystemTime()) {
throw new IllegalStateException("Use either @UseClockStep or @UseSystemTime, not both");
}
+ if (systemProperties() != null && systemProperty() != null) {
+ throw new IllegalStateException(
+ "Use either @GerritSystemProperties or @GerritSystemProperty, not both");
+ }
if (configs() != null && config() != null) {
throw new IllegalStateException("Use either @GerritConfigs or @GerritConfig, not both");
}
diff --git a/java/com/google/gerrit/acceptance/config/ConfigAnnotationParser.java b/java/com/google/gerrit/acceptance/config/ConfigAnnotationParser.java
index 27ce857..fc6be03 100644
--- a/java/com/google/gerrit/acceptance/config/ConfigAnnotationParser.java
+++ b/java/com/google/gerrit/acceptance/config/ConfigAnnotationParser.java
@@ -47,6 +47,16 @@
return cfg;
}
+ public static void parse(GerritSystemProperties annotation) {
+ for (GerritSystemProperty prop : annotation.value()) {
+ parse(prop);
+ }
+ }
+
+ public static void parse(GerritSystemProperty annotation) {
+ System.setProperty(annotation.name(), annotation.value());
+ }
+
@Nullable
public static Map<String, Config> parse(GlobalPluginConfigs annotation) {
if (annotation == null || annotation.value().length < 1) {
diff --git a/java/com/google/gerrit/acceptance/config/GerritSystemProperties.java b/java/com/google/gerrit/acceptance/config/GerritSystemProperties.java
new file mode 100644
index 0000000..cc6389c
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/config/GerritSystemProperties.java
@@ -0,0 +1,27 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.config;
+
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+@Target({METHOD})
+@Retention(RUNTIME)
+public @interface GerritSystemProperties {
+ GerritSystemProperty[] value();
+}
diff --git a/java/com/google/gerrit/acceptance/config/GerritSystemProperty.java b/java/com/google/gerrit/acceptance/config/GerritSystemProperty.java
new file mode 100644
index 0000000..a2bf735
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/config/GerritSystemProperty.java
@@ -0,0 +1,33 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.config;
+
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import java.lang.annotation.Repeatable;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+@Target({METHOD})
+@Retention(RUNTIME)
+@Repeatable(GerritSystemProperties.class)
+public @interface GerritSystemProperty {
+ /** System property name. */
+ String name();
+
+ /** Value of the system property. */
+ String value() default "";
+}
diff --git a/java/com/google/gerrit/httpd/auth/container/HttpAuthFilter.java b/java/com/google/gerrit/httpd/auth/container/HttpAuthFilter.java
index be833ea..f0a8b89 100644
--- a/java/com/google/gerrit/httpd/auth/container/HttpAuthFilter.java
+++ b/java/com/google/gerrit/httpd/auth/container/HttpAuthFilter.java
@@ -147,8 +147,10 @@
@Nullable
String getRemoteDisplayname(HttpServletRequest req) {
if (displaynameHeader != null) {
- String raw = req.getHeader(displaynameHeader);
- return emptyToNull(new String(raw.getBytes(ISO_8859_1), UTF_8));
+ String raw = emptyToNull(req.getHeader(displaynameHeader));
+ if (raw != null) {
+ return new String(raw.getBytes(ISO_8859_1), UTF_8);
+ }
}
return null;
}
diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index dcc6af6..5dfcef3 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -61,7 +61,7 @@
protected static class Metrics {
final Timer1<String> executionTime;
- Metrics(MetricMaker metricMaker) {
+ protected Metrics(MetricMaker metricMaker) {
executionTime =
metricMaker.newTimer(
"query/query_latency",
@@ -95,14 +95,14 @@
private Set<String> requestedFields;
protected QueryProcessor(
- MetricMaker metricMaker,
+ Metrics metrics,
SchemaDefinitions<T> schemaDef,
IndexConfig indexConfig,
IndexCollection<?, T, ? extends Index<?, T>> indexes,
IndexRewriter<T> rewriter,
String limitField,
IntSupplier userQueryLimit) {
- this.metrics = new Metrics(metricMaker);
+ this.metrics = metrics;
this.schemaDef = schemaDef;
this.indexConfig = indexConfig;
this.indexes = indexes;
diff --git a/java/com/google/gerrit/pgm/Reindex.java b/java/com/google/gerrit/pgm/Reindex.java
index f22ceb5..bbba28c 100644
--- a/java/com/google/gerrit/pgm/Reindex.java
+++ b/java/com/google/gerrit/pgm/Reindex.java
@@ -42,6 +42,7 @@
import com.google.gerrit.server.index.IndexModule;
import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
import com.google.gerrit.server.index.options.AutoFlush;
+import com.google.gerrit.server.index.options.BuildBloomFilter;
import com.google.gerrit.server.index.options.IsFirstInsertForEntry;
import com.google.gerrit.server.plugins.PluginGuiceEnvironment;
import com.google.gerrit.server.util.ReplicaUtil;
@@ -92,6 +93,9 @@
@Option(name = "--show-cache-stats", usage = "Show cache statistics at the end.")
private boolean showCacheStats;
+ @Option(name = "--build-bloom-filter", usage = "Build bloom filter for H2 disk caches.")
+ private boolean buildBloomFilter;
+
private Injector dbInjector;
private Injector sysInjector;
private Injector cfgInjector;
@@ -209,6 +213,9 @@
OptionalBinder.newOptionalBinder(binder(), IsFirstInsertForEntry.class)
.setBinding()
.toInstance(IsFirstInsertForEntry.YES);
+ OptionalBinder.newOptionalBinder(binder(), BuildBloomFilter.class)
+ .setBinding()
+ .toInstance(buildBloomFilter ? BuildBloomFilter.TRUE : BuildBloomFilter.FALSE);
}
});
modules.add(new BatchProgramModule(dbInjector));
diff --git a/java/com/google/gerrit/server/approval/ApprovalsUtil.java b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
index 9b40a57..e64c273 100644
--- a/java/com/google/gerrit/server/approval/ApprovalsUtil.java
+++ b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
@@ -377,6 +377,11 @@
return notes.load().getApprovals().onlyNonCopied();
}
+ public ListMultimap<PatchSet.Id, PatchSetApproval> byChangeIncludingCopiedApprovals(
+ ChangeNotes notes) {
+ return notes.load().getApprovals().all();
+ }
+
/**
* Copies approvals to a new patch set.
*
diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java b/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
index 445d8a0..fdd55ac 100644
--- a/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
+++ b/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java
@@ -14,11 +14,15 @@
package com.google.gerrit.server.cache.h2;
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+import com.google.common.base.Strings;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.flogger.FluentLogger;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.server.cache.MemoryCacheFactory;
@@ -26,13 +30,17 @@
import com.google.gerrit.server.cache.PersistentCacheDef;
import com.google.gerrit.server.cache.h2.H2CacheImpl.SqlStore;
import com.google.gerrit.server.cache.h2.H2CacheImpl.ValueHolder;
+import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.index.options.BuildBloomFilter;
+import com.google.gerrit.server.index.options.IsFirstInsertForEntry;
import com.google.gerrit.server.logging.LoggingContextAwareExecutorService;
import com.google.gerrit.server.logging.LoggingContextAwareScheduledExecutorService;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
@@ -57,32 +65,43 @@
private final ScheduledExecutorService cleanup;
private final long h2CacheSize;
private final boolean h2AutoServer;
+ private final boolean isOfflineReindex;
+ private final boolean buildBloomFilter;
@Inject
H2CacheFactory(
MemoryCacheFactory memCacheFactory,
@GerritServerConfig Config cfg,
SitePaths site,
- DynamicMap<Cache<?, ?>> cacheMap) {
+ DynamicMap<Cache<?, ?>> cacheMap,
+ @Nullable IsFirstInsertForEntry isFirstInsertForEntry,
+ @Nullable BuildBloomFilter buildBloomFilter) {
super(memCacheFactory, cfg, site);
h2CacheSize = cfg.getLong("cache", null, "h2CacheSize", -1);
h2AutoServer = cfg.getBoolean("cache", null, "h2AutoServer", false);
caches = new ArrayList<>();
this.cacheMap = cacheMap;
+ this.isOfflineReindex =
+ isFirstInsertForEntry != null && isFirstInsertForEntry.equals(IsFirstInsertForEntry.YES);
+ this.buildBloomFilter =
+ !(buildBloomFilter != null && buildBloomFilter.equals(BuildBloomFilter.FALSE));
if (diskEnabled) {
executor =
new LoggingContextAwareExecutorService(
Executors.newFixedThreadPool(
1, new ThreadFactoryBuilder().setNameFormat("DiskCache-Store-%d").build()));
+
cleanup =
- new LoggingContextAwareScheduledExecutorService(
- Executors.newScheduledThreadPool(
- 1,
- new ThreadFactoryBuilder()
- .setNameFormat("DiskCache-Prune-%d")
- .setDaemon(true)
- .build()));
+ isOfflineReindex
+ ? null
+ : new LoggingContextAwareScheduledExecutorService(
+ Executors.newScheduledThreadPool(
+ 1,
+ new ThreadFactoryBuilder()
+ .setNameFormat("DiskCache-Prune-%d")
+ .setDaemon(true)
+ .build()));
} else {
executor = null;
cleanup = null;
@@ -94,9 +113,11 @@
if (executor != null) {
for (H2CacheImpl<?, ?> cache : caches) {
executor.execute(cache::start);
- @SuppressWarnings("unused")
- Future<?> possiblyIgnoredError =
- cleanup.schedule(() -> cache.prune(cleanup), 30, TimeUnit.SECONDS);
+ if (cleanup != null) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ cleanup.schedule(() -> cache.prune(cleanup), 30, TimeUnit.SECONDS);
+ }
}
}
}
@@ -105,7 +126,9 @@
public void stop() {
if (executor != null) {
try {
- cleanup.shutdownNow();
+ if (cleanup != null) {
+ cleanup.shutdownNow();
+ }
List<Runnable> pending = executor.shutdownNow();
if (executor.awaitTermination(15, TimeUnit.MINUTES)) {
@@ -183,6 +206,22 @@
if (h2AutoServer) {
url.append(";AUTO_SERVER=TRUE");
}
+ Duration refreshAfterWrite = def.refreshAfterWrite();
+ if (has(def.configKey(), "refreshAfterWrite")) {
+ long refreshAfterWriteInSec =
+ ConfigUtil.getTimeUnit(config, "cache", def.configKey(), "refreshAfterWrite", 0, SECONDS);
+ if (refreshAfterWriteInSec != 0) {
+ refreshAfterWrite = Duration.ofSeconds(refreshAfterWriteInSec);
+ }
+ }
+ Duration expireAfterWrite = def.expireAfterWrite();
+ if (has(def.configKey(), "maxAge")) {
+ long expireAfterWriteInsec =
+ ConfigUtil.getTimeUnit(config, "cache", def.configKey(), "maxAge", 0, SECONDS);
+ if (expireAfterWriteInsec != 0) {
+ expireAfterWrite = Duration.ofSeconds(expireAfterWriteInsec);
+ }
+ }
return new SqlStore<>(
url.toString(),
def.keyType(),
@@ -190,7 +229,12 @@
def.valueSerializer(),
def.version(),
maxSize,
- def.expireAfterWrite(),
- def.expireFromMemoryAfterAccess());
+ expireAfterWrite,
+ refreshAfterWrite,
+ buildBloomFilter);
+ }
+
+ private boolean has(String name, String var) {
+ return !Strings.isNullOrEmpty(config.getString("cache", name, var));
}
}
diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java b/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
index 8327b88..29bf0e6 100644
--- a/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
+++ b/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
@@ -365,6 +365,7 @@
private final AtomicLong missCount = new AtomicLong();
private volatile BloomFilter<K> bloomFilter;
private int estimatedSize;
+ private boolean buildBloomFilter;
SqlStore(
String jdbcUrl,
@@ -374,7 +375,8 @@
int version,
long maxSize,
@Nullable Duration expireAfterWrite,
- @Nullable Duration refreshAfterWrite) {
+ @Nullable Duration refreshAfterWrite,
+ boolean buildBloomFilter) {
this.url = jdbcUrl;
this.keyType = createKeyType(keyType, keySerializer);
this.valueSerializer = valueSerializer;
@@ -382,6 +384,7 @@
this.maxSize = maxSize;
this.expireAfterWrite = expireAfterWrite;
this.refreshAfterWrite = refreshAfterWrite;
+ this.buildBloomFilter = buildBloomFilter;
int cores = Runtime.getRuntime().availableProcessors();
int keep = Math.min(cores, 16);
@@ -398,7 +401,7 @@
}
synchronized void open() {
- if (bloomFilter == null) {
+ if (buildBloomFilter && bloomFilter == null) {
bloomFilter = buildBloomFilter();
}
}
@@ -412,7 +415,7 @@
boolean mightContain(K key) {
BloomFilter<K> b = bloomFilter;
- if (b == null) {
+ if (buildBloomFilter && b == null) {
synchronized (this) {
b = bloomFilter;
if (b == null) {
diff --git a/java/com/google/gerrit/server/config/GerritInstanceIdProvider.java b/java/com/google/gerrit/server/config/GerritInstanceIdProvider.java
index 891ca76..6523f18 100644
--- a/java/com/google/gerrit/server/config/GerritInstanceIdProvider.java
+++ b/java/com/google/gerrit/server/config/GerritInstanceIdProvider.java
@@ -22,11 +22,14 @@
/** Provides {@link GerritInstanceId} from {@code gerrit.instanceId}. */
@Singleton
public class GerritInstanceIdProvider implements Provider<String> {
+ public static final String INSTANCE_ID_SYSTEM_PROPERTY = "gerrit.instanceId";
private final String instanceId;
@Inject
public GerritInstanceIdProvider(@GerritServerConfig Config cfg) {
- instanceId = cfg.getString("gerrit", null, "instanceId");
+ instanceId =
+ System.getProperty(
+ INSTANCE_ID_SYSTEM_PROPERTY, cfg.getString("gerrit", null, "instanceId"));
}
@Override
diff --git a/java/com/google/gerrit/server/index/IndexModule.java b/java/com/google/gerrit/server/index/IndexModule.java
index fbe18ca..c0bd62f 100644
--- a/java/com/google/gerrit/server/index/IndexModule.java
+++ b/java/com/google/gerrit/server/index/IndexModule.java
@@ -53,6 +53,7 @@
import com.google.gerrit.server.index.group.GroupIndexer;
import com.google.gerrit.server.index.group.GroupIndexerImpl;
import com.google.gerrit.server.index.group.GroupSchemaDefinitions;
+import com.google.gerrit.server.index.options.BuildBloomFilter;
import com.google.gerrit.server.index.options.IsFirstInsertForEntry;
import com.google.gerrit.server.index.project.ProjectIndexDefinition;
import com.google.gerrit.server.index.project.ProjectIndexerImpl;
@@ -155,6 +156,9 @@
OptionalBinder.newOptionalBinder(binder(), IsFirstInsertForEntry.class)
.setDefault()
.toInstance(IsFirstInsertForEntry.NO);
+ OptionalBinder.newOptionalBinder(binder(), BuildBloomFilter.class)
+ .setDefault()
+ .toInstance(BuildBloomFilter.TRUE);
}
@Provides
diff --git a/java/com/google/gerrit/server/index/options/BuildBloomFilter.java b/java/com/google/gerrit/server/index/options/BuildBloomFilter.java
new file mode 100644
index 0000000..021f0fe
--- /dev/null
+++ b/java/com/google/gerrit/server/index/options/BuildBloomFilter.java
@@ -0,0 +1,21 @@
+// Copyright (C) 2023 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.index.options;
+
+/** This enum can be used to decide if bloom filters for H2 disk caches should be built. */
+public enum BuildBloomFilter {
+ TRUE,
+ FALSE
+}
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 0e37049..3d35321 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -156,7 +156,7 @@
Permission.EDIT_TOPIC_NAME) // user can edit topic on a specific ref
|| getProjectControl().isAdmin();
}
- return refControl.canForceEditTopicName();
+ return refControl.canForceEditTopicName(isOwner());
}
/** Can this user toggle WorkInProgress state? */
diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
index eebaa8f..a706951 100644
--- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
+++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
+import com.google.inject.Singleton;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.util.ArrayList;
@@ -65,6 +66,27 @@
DefaultRefFilter create(ProjectControl projectControl);
}
+ @Singleton
+ private static class Metrics {
+ final Counter0 fullFilterCount;
+ final Counter0 skipFilterCount;
+
+ @Inject
+ Metrics(MetricMaker metricMaker) {
+ fullFilterCount =
+ metricMaker.newCounter(
+ "permissions/ref_filter/full_filter_count",
+ new Description("Rate of full ref filter operations").setRate());
+ skipFilterCount =
+ metricMaker.newCounter(
+ "permissions/ref_filter/skip_filter_count",
+ new Description(
+ "Rate of ref filter operations where we skip full evaluation"
+ + " because the user can read all refs")
+ .setRate());
+ }
+ }
+
private final TagCache tagCache;
private final PermissionBackend permissionBackend;
private final RefVisibilityControl refVisibilityControl;
@@ -75,8 +97,7 @@
private final @Nullable SearchingChangeCacheImpl searchingChangeDataProvider;
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory changeNotesFactory;
- private final Counter0 fullFilterCount;
- private final Counter0 skipFilterCount;
+ private final Metrics metrics;
private final boolean skipFullRefEvaluationIfAllRefsAreVisible;
@Inject
@@ -85,7 +106,7 @@
PermissionBackend permissionBackend,
RefVisibilityControl refVisibilityControl,
@GerritServerConfig Config config,
- MetricMaker metricMaker,
+ Metrics metrics,
@Nullable SearchingChangeCacheImpl searchingChangeDataProvider,
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory changeNotesFactory,
@@ -104,17 +125,7 @@
this.projectState = projectControl.getProjectState();
this.permissionBackendForProject =
permissionBackend.user(user).project(projectState.getNameKey());
- this.fullFilterCount =
- metricMaker.newCounter(
- "permissions/ref_filter/full_filter_count",
- new Description("Rate of full ref filter operations").setRate());
- this.skipFilterCount =
- metricMaker.newCounter(
- "permissions/ref_filter/skip_filter_count",
- new Description(
- "Rate of ref filter operations where we skip full evaluation"
- + " because the user can read all refs")
- .setRate());
+ this.metrics = metrics;
}
/** Filters given refs and tags by visibility. */
@@ -202,13 +213,13 @@
logger.atFinest().log("User has READ on refs/* = %s", hasReadOnRefsStar);
if (skipFullRefEvaluationIfAllRefsAreVisible && !projectState.isAllUsers()) {
if (hasReadOnRefsStar) {
- skipFilterCount.increment();
+ metrics.skipFilterCount.increment();
logger.atFinest().log(
"Fast path, all refs are visible because user has READ on refs/*: %s", refs);
return new AutoValue_DefaultRefFilter_Result(
ImmutableList.copyOf(refs), ImmutableList.of());
} else if (projectControl.allRefsAreVisible(ImmutableSet.of(RefNames.REFS_CONFIG))) {
- skipFilterCount.increment();
+ metrics.skipFilterCount.increment();
refs = fastHideRefsMetaConfig(refs);
logger.atFinest().log(
"Fast path, all refs except %s are visible: %s", RefNames.REFS_CONFIG, refs);
@@ -217,7 +228,7 @@
}
}
logger.atFinest().log("Doing full ref filtering");
- fullFilterCount.increment();
+ metrics.fullFilterCount.increment();
boolean hasAccessDatabase =
permissionBackend
diff --git a/java/com/google/gerrit/server/permissions/GitVisibleChangeFilter.java b/java/com/google/gerrit/server/permissions/GitVisibleChangeFilter.java
index 0e5ff48..ae72b06 100644
--- a/java/com/google/gerrit/server/permissions/GitVisibleChangeFilter.java
+++ b/java/com/google/gerrit/server/permissions/GitVisibleChangeFilter.java
@@ -20,7 +20,6 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.server.git.SearchingChangeCacheImpl;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.query.change.ChangeData;
@@ -88,7 +87,11 @@
try {
return forProject.change(cd).test(ChangePermission.READ);
} catch (PermissionBackendException e) {
- throw new StorageException(e);
+ // This is almost the same as the message .testOrFalse() would log, but with the
+ // added context of the change and coming from this class
+ logger.atWarning().withCause(e).log(
+ "Cannot test read permission for %s; assuming not visible", cd);
+ return false;
}
})
.forEach(
@@ -147,7 +150,8 @@
})
.filter(Objects::nonNull);
} catch (IOException e) {
- throw new StorageException(e);
+ logger.atWarning().withCause(e).log("Unable to scanChangeIds for %s", projectName);
+ return Stream.empty();
}
return cds;
}
diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java
index a3a041b..aba9522 100644
--- a/java/com/google/gerrit/server/permissions/RefControl.java
+++ b/java/com/google/gerrit/server/permissions/RefControl.java
@@ -162,8 +162,8 @@
}
/** Returns true if this user can force edit topic names. */
- boolean canForceEditTopicName() {
- return canPerform(Permission.EDIT_TOPIC_NAME, false, true);
+ boolean canForceEditTopicName(boolean isChangeOwner) {
+ return canPerform(Permission.EDIT_TOPIC_NAME, isChangeOwner, true);
}
/** Returns true if this user can delete changes. */
diff --git a/java/com/google/gerrit/server/project/PeriodicProjectListCacheWarmer.java b/java/com/google/gerrit/server/project/PeriodicProjectListCacheWarmer.java
index df2e1cf..47d43b9 100644
--- a/java/com/google/gerrit/server/project/PeriodicProjectListCacheWarmer.java
+++ b/java/com/google/gerrit/server/project/PeriodicProjectListCacheWarmer.java
@@ -92,6 +92,11 @@
}
@Override
+ public String toString() {
+ return "Project List Cache Warmer";
+ }
+
+ @Override
public void run() {
logger.atFine().log("Loading project_list cache");
cache.refreshProjectList();
diff --git a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
index 1fe8641..62cb797 100644
--- a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
+++ b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
@@ -37,6 +37,7 @@
import com.google.gerrit.server.rules.PrologSubmitRuleUtil;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.Inject;
+import com.google.inject.Singleton;
import com.google.inject.assistedinject.Assisted;
import java.util.List;
import java.util.Optional;
@@ -48,41 +49,51 @@
public class SubmitRuleEvaluator {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private final ProjectCache projectCache;
- private final PrologSubmitRuleUtil prologSubmitRuleUtil;
- private final PluginSetContext<SubmitRule> submitRules;
- private final Timer0 submitRuleEvaluationLatency;
- private final Timer0 submitTypeEvaluationLatency;
- private final SubmitRuleOptions opts;
- private final CallerFinder callerFinder;
-
public interface Factory {
/** Returns a new {@link SubmitRuleEvaluator} with the specified options */
SubmitRuleEvaluator create(SubmitRuleOptions options);
}
+ @Singleton
+ private static class Metrics {
+ final Timer0 submitRuleEvaluationLatency;
+ final Timer0 submitTypeEvaluationLatency;
+
+ @Inject
+ Metrics(MetricMaker metricMaker) {
+ submitRuleEvaluationLatency =
+ metricMaker.newTimer(
+ "change/submit_rule_evaluation",
+ new Description("Latency for evaluating submit rules on a change.")
+ .setCumulative()
+ .setUnit(Units.MILLISECONDS));
+ submitTypeEvaluationLatency =
+ metricMaker.newTimer(
+ "change/submit_type_evaluation",
+ new Description("Latency for evaluating the submit type on a change.")
+ .setCumulative()
+ .setUnit(Units.MILLISECONDS));
+ }
+ }
+
+ private final ProjectCache projectCache;
+ private final PrologSubmitRuleUtil prologSubmitRuleUtil;
+ private final PluginSetContext<SubmitRule> submitRules;
+ private final Metrics metrics;
+ private final SubmitRuleOptions opts;
+ private final CallerFinder callerFinder;
+
@Inject
private SubmitRuleEvaluator(
ProjectCache projectCache,
PrologSubmitRuleUtil prologSubmitRuleUtil,
PluginSetContext<SubmitRule> submitRules,
- MetricMaker metricMaker,
+ Metrics metrics,
@Assisted SubmitRuleOptions options) {
this.projectCache = projectCache;
this.prologSubmitRuleUtil = prologSubmitRuleUtil;
this.submitRules = submitRules;
- this.submitRuleEvaluationLatency =
- metricMaker.newTimer(
- "change/submit_rule_evaluation",
- new Description("Latency for evaluating submit rules on a change.")
- .setCumulative()
- .setUnit(Units.MILLISECONDS));
- this.submitTypeEvaluationLatency =
- metricMaker.newTimer(
- "change/submit_type_evaluation",
- new Description("Latency for evaluating the submit type on a change.")
- .setCumulative()
- .setUnit(Units.MILLISECONDS));
+ this.metrics = metrics;
this.opts = options;
@@ -106,7 +117,7 @@
logger.atFine().log(
"Evaluate submit rules for change %d (caller: %s)",
cd.change().getId().get(), callerFinder.findCallerLazy());
- try (Timer0.Context ignored = submitRuleEvaluationLatency.start()) {
+ try (Timer0.Context ignored = metrics.submitRuleEvaluationLatency.start()) {
Change change;
ProjectState projectState;
try {
@@ -173,7 +184,7 @@
* @return record from the evaluated rules.
*/
public SubmitTypeRecord getSubmitType(ChangeData cd) {
- try (Timer0.Context ignored = submitTypeEvaluationLatency.start()) {
+ try (Timer0.Context ignored = metrics.submitTypeEvaluationLatency.start()) {
try {
Project.NameKey name = cd.project();
Optional<ProjectState> project = projectCache.get(name);
diff --git a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
index eef913e..d812eef 100644
--- a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
@@ -34,6 +34,7 @@
import com.google.gerrit.server.notedb.Sequences;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import com.google.inject.Singleton;
/**
* Query processor for the account index.
@@ -46,6 +47,14 @@
private final Sequences sequences;
private final IndexConfig indexConfig;
+ @Singleton
+ protected static class AccountQueryMetrics extends QueryProcessor.Metrics {
+ @Inject
+ protected AccountQueryMetrics(MetricMaker metricMaker) {
+ super(metricMaker);
+ }
+ }
+
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
checkState(
@@ -57,14 +66,14 @@
protected AccountQueryProcessor(
Provider<CurrentUser> userProvider,
AccountLimits.Factory limitsFactory,
- MetricMaker metricMaker,
+ AccountQueryMetrics accountQueryMetrics,
IndexConfig indexConfig,
AccountIndexCollection indexes,
AccountIndexRewriter rewriter,
AccountControl.Factory accountControlFactory,
Sequences sequences) {
super(
- metricMaker,
+ accountQueryMetrics,
AccountSchemaDefinitions.INSTANCE,
indexConfig,
indexes,
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 81aad7f..c5a8794 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -379,6 +379,8 @@
private PatchSet currentPatchSet;
private Collection<PatchSet> patchSets;
private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals;
+
+ private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovalsWithCopied;
private List<PatchSetApproval> currentApprovals;
private List<String> currentFiles;
private Optional<DiffSummary> diffSummary;
@@ -868,6 +870,16 @@
return allApprovals;
}
+ public ListMultimap<PatchSet.Id, PatchSetApproval> conditionallyLoadApprovalsWithCopied() {
+ if (allApprovalsWithCopied == null) {
+ if (!lazyload()) {
+ return ImmutableListMultimap.of();
+ }
+ allApprovalsWithCopied = approvalsUtil.byChangeIncludingCopiedApprovals(notes());
+ }
+ return allApprovalsWithCopied;
+ }
+
/* @return legacy submit ('SUBM') approval label */
// TODO(mariasavtchouk): Deprecate legacy submit label,
// see com.google.gerrit.entities.LabelId.LEGACY_SUBMIT_NAME
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
index b7dc127..3097224 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
@@ -43,6 +43,7 @@
import com.google.gerrit.server.notedb.Sequences;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import com.google.inject.Singleton;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
@@ -66,6 +67,14 @@
private final Sequences sequences;
private final IndexConfig indexConfig;
+ @Singleton
+ protected static class ChangeQueryMetrics extends QueryProcessor.Metrics {
+ @Inject
+ protected ChangeQueryMetrics(MetricMaker metricMaker) {
+ super(metricMaker);
+ }
+ }
+
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
checkState(
@@ -77,7 +86,7 @@
ChangeQueryProcessor(
Provider<CurrentUser> userProvider,
AccountLimits.Factory limitsFactory,
- MetricMaker metricMaker,
+ ChangeQueryMetrics changeQueryMetrics,
IndexConfig indexConfig,
ChangeIndexCollection indexes,
ChangeIndexRewriter rewriter,
@@ -85,7 +94,7 @@
ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory,
DynamicSet<ChangePluginDefinedInfoFactory> changePluginDefinedInfoFactories) {
super(
- metricMaker,
+ changeQueryMetrics,
ChangeSchemaDefinitions.INSTANCE,
indexConfig,
indexes,
diff --git a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
index 961404a..d21f5b6 100644
--- a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
+++ b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
@@ -303,7 +303,7 @@
rw,
c,
d.patchSets(),
- includeApprovals ? d.approvals().asMap() : null,
+ includeApprovals ? d.conditionallyLoadApprovalsWithCopied().asMap() : null,
includeFiles,
d.change(),
labelTypes,
diff --git a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
index 344a978..74c8d39 100644
--- a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
@@ -34,6 +34,7 @@
import com.google.gerrit.server.notedb.Sequences;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import com.google.inject.Singleton;
/**
* Query processor for the group index.
@@ -47,6 +48,14 @@
private final Sequences sequences;
private final IndexConfig indexConfig;
+ @Singleton
+ protected static class GroupQueryMetrics extends QueryProcessor.Metrics {
+ @Inject
+ protected GroupQueryMetrics(MetricMaker metricMaker) {
+ super(metricMaker);
+ }
+ }
+
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
checkState(
@@ -58,14 +67,14 @@
protected GroupQueryProcessor(
Provider<CurrentUser> userProvider,
AccountLimits.Factory limitsFactory,
- MetricMaker metricMaker,
+ GroupQueryMetrics groupQueryMetrics,
IndexConfig indexConfig,
GroupIndexCollection indexes,
GroupIndexRewriter rewriter,
GroupControl.GenericFactory groupControlFactory,
Sequences sequences) {
super(
- metricMaker,
+ groupQueryMetrics,
GroupSchemaDefinitions.INSTANCE,
indexConfig,
indexes,
diff --git a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
index 3877c25..ddc7ccc 100644
--- a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java
@@ -34,6 +34,7 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.inject.Inject;
import com.google.inject.Provider;
+import com.google.inject.Singleton;
/**
* Query processor for the project index.
@@ -49,6 +50,14 @@
private final ProjectCache projectCache;
private final IndexConfig indexConfig;
+ @Singleton
+ protected static class ProjectQueryMetrics extends QueryProcessor.Metrics {
+ @Inject
+ protected ProjectQueryMetrics(MetricMaker metricMaker) {
+ super(metricMaker);
+ }
+ }
+
static {
// It is assumed that basic rewrites do not touch visibleto predicates.
checkState(
@@ -60,14 +69,14 @@
protected ProjectQueryProcessor(
Provider<CurrentUser> userProvider,
AccountLimits.Factory limitsFactory,
- MetricMaker metricMaker,
+ ProjectQueryMetrics projectQueryMetrics,
IndexConfig indexConfig,
ProjectIndexCollection indexes,
ProjectIndexRewriter rewriter,
PermissionBackend permissionBackend,
ProjectCache projectCache) {
super(
- metricMaker,
+ projectQueryMetrics,
ProjectSchemaDefinitions.INSTANCE,
indexConfig,
indexes,
diff --git a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
index 39165d0..9e0eac7 100644
--- a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
+++ b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
@@ -52,6 +52,7 @@
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.server.config.ConfigResource;
import com.google.gerrit.server.config.ConfigUtil;
+import com.google.gerrit.server.config.GerritInstanceId;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.documentation.QueryDocumentationExecutor;
@@ -92,6 +93,7 @@
private final ProjectCache projectCache;
private final AgreementJson agreementJson;
private final SitePaths sitePaths;
+ private final @Nullable @GerritInstanceId String instanceId;
@Inject
public GetServerInfo(
@@ -113,7 +115,8 @@
QueryDocumentationExecutor docSearcher,
ProjectCache projectCache,
AgreementJson agreementJson,
- SitePaths sitePaths) {
+ SitePaths sitePaths,
+ @Nullable @GerritInstanceId String instanceId) {
this.config = config;
this.accountVisibilityProvider = accountVisibilityProvider;
this.accountDefaultDisplayName = accountDefaultDisplayName;
@@ -133,6 +136,7 @@
this.projectCache = projectCache;
this.agreementJson = agreementJson;
this.sitePaths = sitePaths;
+ this.instanceId = instanceId;
}
@Override
@@ -289,7 +293,7 @@
info.editGpgKeys =
toBoolean(enableSignedPush && config.getBoolean("gerrit", null, "editGpgKeys", true));
info.primaryWeblinkName = config.getString("gerrit", null, "primaryWeblinkName");
- info.instanceId = config.getString("gerrit", null, "instanceId");
+ info.instanceId = instanceId;
info.defaultBranch = config.getString("gerrit", null, "defaultBranch");
return info;
}
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteRef.java b/java/com/google/gerrit/server/restapi/project/DeleteRef.java
index 4fc2b86..388946e 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteRef.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteRef.java
@@ -117,9 +117,12 @@
.check(RefPermission.DELETE);
try (Repository repository = repoManager.openRepository(projectState.getNameKey())) {
- RefUpdate.Result result;
+ Ref refObj = repository.exactRef(ref);
+ if (refObj == null) {
+ throw new ResourceConflictException(String.format("ref %s doesn't exist", ref));
+ }
RefUpdate u = repository.updateRef(ref);
- u.setExpectedOldObjectId(repository.exactRef(ref).getObjectId());
+ u.setExpectedOldObjectId(refObj.getObjectId());
u.setNewObjectId(ObjectId.zeroId());
u.setForceUpdate(true);
refDeletionValidator.validateRefOperation(
@@ -127,7 +130,7 @@
identifiedUser.get(),
u,
/* pushOptions */ ImmutableListMultimap.of());
- result = u.delete();
+ RefUpdate.Result result = u.delete();
switch (result) {
case NEW:
diff --git a/java/com/google/gerrit/sshd/InactiveAccountDisconnector.java b/java/com/google/gerrit/sshd/InactiveAccountDisconnector.java
index 1086626..2f96915 100644
--- a/java/com/google/gerrit/sshd/InactiveAccountDisconnector.java
+++ b/java/com/google/gerrit/sshd/InactiveAccountDisconnector.java
@@ -39,7 +39,9 @@
sshDaemon,
(sshId, sshSession, abstractSession, ioSession) -> {
CurrentUser sessionUser = sshSession.getUser();
- if (sessionUser.isIdentifiedUser() && sessionUser.getAccountId().get() == id) {
+ if (sessionUser != null
+ && sessionUser.isIdentifiedUser()
+ && sessionUser.getAccountId().get() == id) {
logger.atInfo().log(
"Disconnecting SSH session %s because user %s(%d) got deactivated",
abstractSession, sessionUser.getLoggableName(), id);
diff --git a/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java b/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java
index 4f23d1d..f42eb5c 100644
--- a/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java
+++ b/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java
@@ -27,6 +27,8 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.restapi.change.DeleteReviewer;
import com.google.gerrit.server.restapi.change.PostReviewers;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryableAction;
import com.google.gerrit.sshd.ChangeArgumentParser;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
@@ -89,6 +91,8 @@
@Inject private ChangeArgumentParser changeArgumentParser;
+ @Inject private RetryHelper retryHelper;
+
private Set<Account.Id> toRemove = new HashSet<>();
private Map<Change.Id, ChangeResource> changes = new LinkedHashMap<>();
@@ -121,7 +125,15 @@
ReviewerResource rsrc = reviewerFactory.create(changeRsrc, reviewer);
String error = null;
try {
- deleteReviewer.apply(rsrc, new DeleteReviewerInput());
+ retryHelper
+ .action(
+ RetryableAction.ActionType.CHANGE_UPDATE,
+ "removeReviewers",
+ () -> {
+ deleteReviewer.apply(rsrc, new DeleteReviewerInput());
+ return null;
+ })
+ .call();
} catch (ResourceNotFoundException e) {
error = String.format("could not remove %s: not found", reviewer);
} catch (Exception e) {
@@ -139,15 +151,26 @@
ReviewerInput input = new ReviewerInput();
input.reviewer = reviewer;
input.confirmed = true;
- String error;
+ var error =
+ new Object() {
+ String value;
+ };
try {
- error = postReviewers.apply(changeRsrc, input).value().error;
+ retryHelper
+ .action(
+ RetryableAction.ActionType.CHANGE_UPDATE,
+ "applyReview",
+ () -> {
+ error.value = postReviewers.apply(changeRsrc, input).value().error;
+ return null;
+ })
+ .call();
} catch (Exception e) {
- error = String.format("could not add %s: %s", reviewer, e.getMessage());
+ error.value = String.format("could not add %s: %s", reviewer, e.getMessage());
}
- if (error != null) {
+ if (error.value != null) {
ok = false;
- writeError("error", error);
+ writeError("error", error.value);
}
}
diff --git a/java/com/google/gerrit/sshd/commands/SetTopicCommand.java b/java/com/google/gerrit/sshd/commands/SetTopicCommand.java
index 244fdbe..b590740 100644
--- a/java/com/google/gerrit/sshd/commands/SetTopicCommand.java
+++ b/java/com/google/gerrit/sshd/commands/SetTopicCommand.java
@@ -16,13 +16,13 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.api.changes.TopicInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.change.ChangeResource;
-import com.google.gerrit.server.change.SetTopicOp;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.update.BatchUpdate;
-import com.google.gerrit.server.util.time.TimeUtil;
+import com.google.gerrit.server.restapi.change.PutTopic;
import com.google.gerrit.sshd.ChangeArgumentParser;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
@@ -34,9 +34,8 @@
@CommandMetaData(name = "set-topic", description = "Set the topic for one or more changes")
public class SetTopicCommand extends SshCommand {
- private final BatchUpdate.Factory updateFactory;
private final ChangeArgumentParser changeArgumentParser;
- private final SetTopicOp.Factory topicOpFactory;
+ private final PutTopic putTopic;
private Map<Change.Id, ChangeResource> changes = new LinkedHashMap<>();
@@ -62,17 +61,14 @@
private String topic;
@Inject
- SetTopicCommand(
- BatchUpdate.Factory updateFactory,
- ChangeArgumentParser changeArgumentParser,
- SetTopicOp.Factory topicOpFactory) {
- this.updateFactory = updateFactory;
+ SetTopicCommand(ChangeArgumentParser changeArgumentParser, PutTopic putTopic) {
this.changeArgumentParser = changeArgumentParser;
- this.topicOpFactory = topicOpFactory;
+ this.putTopic = putTopic;
}
@Override
public void run() throws Exception {
+ boolean ok = true;
if (topic != null) {
topic = topic.trim();
}
@@ -83,11 +79,28 @@
}
for (ChangeResource r : changes.values()) {
- SetTopicOp op = topicOpFactory.create(topic);
- try (BatchUpdate u = updateFactory.create(r.getChange().getProject(), user, TimeUtil.now())) {
- u.addOp(r.getId(), op);
- u.execute();
+ TopicInput input = new TopicInput();
+ input.topic = topic;
+ try {
+ putTopic.apply(r, input);
+ } catch (ResourceNotFoundException e) {
+ ok = false;
+ writeError(
+ "error",
+ String.format(
+ "could not add topic to change %d: not found", r.getChange().getChangeId()));
+ } catch (Exception e) {
+ ok = false;
+ writeError(
+ "error",
+ String.format(
+ "could not add topic to change %d: %s",
+ r.getChange().getChangeId(), e.getMessage()));
}
}
+
+ if (!ok) {
+ throw die("one or more updates failed");
+ }
}
}
diff --git a/javatests/com/google/gerrit/acceptance/config/GerritInstanceIdIT.java b/javatests/com/google/gerrit/acceptance/config/GerritInstanceIdIT.java
index 0dd6a83..d5e9351 100644
--- a/javatests/com/google/gerrit/acceptance/config/GerritInstanceIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/config/GerritInstanceIdIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.config;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.config.GerritInstanceIdProvider.INSTANCE_ID_SYSTEM_PROPERTY;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import org.junit.Test;
@@ -28,6 +29,13 @@
}
@Test
+ @GerritConfig(name = "gerrit.instanceId", value = "testInstanceId")
+ @GerritSystemProperty(name = INSTANCE_ID_SYSTEM_PROPERTY, value = "sysPropInstanceId")
+ public void instanceIdSystemPropertyOverridesConfig() {
+ assertThat(instanceId).isEqualTo("sysPropInstanceId");
+ }
+
+ @Test
public void shouldReturnNullWhenNotDefined() {
assertThat(instanceId).isNull();
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
index 364ce84..b8b63e6 100644
--- a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
@@ -15,12 +15,14 @@
package com.google.gerrit.acceptance.rest.config;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.config.GerritInstanceIdProvider.INSTANCE_ID_SYSTEM_PROPERTY;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.UseSsh;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.config.GerritSystemProperty;
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.extensions.api.plugins.InstallPluginInput;
import com.google.gerrit.extensions.client.AccountFieldName;
@@ -225,4 +227,11 @@
ServerInfo i = gApi.config().server().getInfo();
assertThat(i.download.schemes).isEmpty();
}
+
+ @Test
+ @GerritSystemProperty(name = INSTANCE_ID_SYSTEM_PROPERTY, value = "sysPropInstanceId")
+ public void instanceIdFromSystemProperty() throws Exception {
+ ServerInfo i = gApi.config().server().getInfo();
+ assertThat(i.gerrit.instanceId).isEqualTo("sysPropInstanceId");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/ssh/QueryIT.java b/javatests/com/google/gerrit/acceptance/ssh/QueryIT.java
index 912c464..8f4458d 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/QueryIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/QueryIT.java
@@ -24,6 +24,8 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.SshSession;
import com.google.gerrit.acceptance.UseSsh;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewerInput;
@@ -325,6 +327,33 @@
}
}
+ @Test
+ public void allApprovalsAllPatchSetsOptionsWithCopyConditionJSON() throws Exception {
+ // Copy min Code-Review votes
+ try (ProjectConfigUpdate u = updateProject(Project.NameKey.parse("All-Projects"))) {
+ u.getConfig().updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("is:MIN"));
+ u.save();
+ }
+
+ // Create a change and add Code-Review -2 on first patch-set
+ String changeId = createChange().getChangeId();
+ gApi.changes().id(changeId).current().review(ReviewInput.reject());
+
+ // Create second patch-set
+ amendChange(changeId);
+
+ // Assert that second patch-set has Code-Review -2 vote
+ List<ChangeAttribute> changes =
+ executeSuccessfulQuery("--all-approvals --patch-sets " + changeId);
+ assertThat(changes).hasSize(1);
+ assertThat(changes.get(0).patchSets).hasSize(2);
+ assertThat(changes.get(0).patchSets.get(1).approvals).isNotNull();
+ assertThat(changes.get(0).patchSets.get(1).approvals).hasSize(1);
+ assertThat(changes.get(0).patchSets.get(1).approvals.get(0).type)
+ .isEqualTo(LabelId.CODE_REVIEW);
+ assertThat(changes.get(0).patchSets.get(1).approvals.get(0).value).isEqualTo("-2");
+ }
+
protected static class SamplePluginModule extends AbstractModule {
@Override
public void configure() {
diff --git a/javatests/com/google/gerrit/httpd/auth/container/HttpAuthFilterTest.java b/javatests/com/google/gerrit/httpd/auth/container/HttpAuthFilterTest.java
new file mode 100644
index 0000000..a5f8349
--- /dev/null
+++ b/javatests/com/google/gerrit/httpd/auth/container/HttpAuthFilterTest.java
@@ -0,0 +1,78 @@
+// Copyright (C) 2023 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.auth.container;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.doReturn;
+
+import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.httpd.WebSession;
+import com.google.gerrit.server.account.externalids.ExternalIdKeyFactory;
+import com.google.gerrit.server.config.AuthConfig;
+import com.google.gerrit.util.http.testutil.FakeHttpServletRequest;
+import java.io.IOException;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.StrictStubs.class)
+public class HttpAuthFilterTest {
+
+ private static String DISPLAYNAME_HEADER = "displaynameHeader";
+ private static String DISPLAYNAME = "displayname";
+
+ @Mock private DynamicItem<WebSession> webSession;
+ @Mock private ExternalIdKeyFactory externalIdKeyFactory;
+ @Mock private AuthConfig authConfig;
+
+ @Test
+ public void getRemoteDisplaynameShouldReturnDisplaynameHeaderWhenHeaderIsConfiguredAndSet()
+ throws IOException {
+ doReturn(DISPLAYNAME_HEADER).when(authConfig).getHttpDisplaynameHeader();
+ HttpAuthFilter httpAuthFilter =
+ new HttpAuthFilter(webSession, authConfig, externalIdKeyFactory);
+
+ FakeHttpServletRequest req = new FakeHttpServletRequest();
+ req.addHeader(DISPLAYNAME_HEADER, DISPLAYNAME);
+
+ assertThat(httpAuthFilter.getRemoteDisplayname(req)).isEqualTo(DISPLAYNAME);
+ }
+
+ @Test
+ public void getRemoteDisplaynameShouldReturnNullWhenDisplaynameHeaderIsConfiguredAndNotSet()
+ throws IOException {
+ doReturn(DISPLAYNAME_HEADER).when(authConfig).getHttpDisplaynameHeader();
+ HttpAuthFilter httpAuthFilter =
+ new HttpAuthFilter(webSession, authConfig, externalIdKeyFactory);
+
+ FakeHttpServletRequest req = new FakeHttpServletRequest();
+
+ assertThat(httpAuthFilter.getRemoteDisplayname(req)).isNull();
+ }
+
+ @Test
+ public void getRemoteDisplaynameShouldReturnNullWhenDisplaynameHeaderIsConfiguredAndEmpty()
+ throws IOException {
+ doReturn(DISPLAYNAME_HEADER).when(authConfig).getHttpDisplaynameHeader();
+ HttpAuthFilter httpAuthFilter =
+ new HttpAuthFilter(webSession, authConfig, externalIdKeyFactory);
+
+ FakeHttpServletRequest req = new FakeHttpServletRequest();
+ req.addHeader(DISPLAYNAME_HEADER, "");
+
+ assertThat(httpAuthFilter.getRemoteDisplayname(req)).isNull();
+ }
+}
diff --git a/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java b/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
index 16fd4ca..8c4eb08 100644
--- a/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
+++ b/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
@@ -71,7 +71,8 @@
version,
1 << 20,
expireAfterWrite,
- refreshAfterWrite);
+ refreshAfterWrite,
+ true);
}
@Test
diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
index c5bef59..43b0eba 100644
--- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java
+++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
@@ -981,6 +981,20 @@
}
@Test
+ public void changeOwnerEditTopicName() throws Exception {
+ projectOperations
+ .project(localKey)
+ .forUpdate()
+ .add(allow(EDIT_TOPIC_NAME).ref("refs/heads/*").group(CHANGE_OWNER).force(true))
+ .update();
+
+ ProjectControl u = user(localKey, DEVS);
+ assertWithMessage("u can edit topic name")
+ .that(u.controlForRef("refs/heads/master").canForceEditTopicName(true))
+ .isTrue();
+ }
+
+ @Test
public void unblockForceEditTopicName() throws Exception {
projectOperations
.project(localKey)
@@ -991,7 +1005,7 @@
ProjectControl u = user(localKey, DEVS);
assertWithMessage("u can edit topic name")
- .that(u.controlForRef("refs/heads/master").canForceEditTopicName())
+ .that(u.controlForRef("refs/heads/master").canForceEditTopicName(false))
.isTrue();
}
@@ -1010,7 +1024,7 @@
ProjectControl u = user(localKey, REGISTERED_USERS);
assertWithMessage("u can't edit topic name")
- .that(u.controlForRef("refs/heads/master").canForceEditTopicName())
+ .that(u.controlForRef("refs/heads/master").canForceEditTopicName(false))
.isFalse();
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 0be80bb..8f835de 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -1610,6 +1610,7 @@
base: el.baseCommit ? el.baseCommit : null,
message: el.message,
allow_conflicts: conflicts,
+ committer_email: el.committerEmail ? el.committerEmail : null,
}
);
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 631d946..15500de 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -1000,6 +1000,7 @@
base: null,
message: 'foo message',
allow_conflicts: false,
+ committer_email: null,
},
]);
});
@@ -1048,6 +1049,7 @@
base: null,
message: 'foo message',
allow_conflicts: true,
+ committer_email: null,
},
]);
});
diff --git a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts
index 73521a6..1285664 100644
--- a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts
+++ b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info.ts
@@ -20,6 +20,7 @@
import {createSearchUrl} from '../../../models/views/search';
import {getBrowseCommitWeblink} from '../../../utils/weblink-util';
import {shorten} from '../../../utils/patch-set-util';
+import {when} from 'lit/directives/when.js';
declare global {
interface HTMLElementTagNameMap {
@@ -31,7 +32,10 @@
export class GrCommitInfo extends LitElement {
// TODO(TS): Maybe limit to StandaloneCommitInfo.
@property({type: Object})
- commitInfo?: CommitInfo;
+ commitInfo?: Partial<CommitInfo>;
+
+ @property({type: Boolean})
+ showCopyButton = true;
@state() serverConfig?: ServerInfo;
@@ -45,6 +49,9 @@
align-items: center;
display: flex;
}
+ gr-weblink {
+ margin-right: 0;
+ }
`,
];
}
@@ -63,13 +70,18 @@
if (!commit) return nothing;
return html` <div class="container">
<gr-weblink imageAndText .info=${this.getWeblink(commit)}></gr-weblink>
- <gr-copy-clipboard
- hastooltip
- .buttonTitle=${'Copy full SHA to clipboard'}
- hideinput
- .text=${commit}
- >
- </gr-copy-clipboard>
+ ${when(
+ this.showCopyButton,
+ () => html`
+ <gr-copy-clipboard
+ hastooltip
+ .buttonTitle=${'Copy full SHA to clipboard'}
+ hideinput
+ .text=${commit}
+ >
+ </gr-copy-clipboard>
+ `
+ )}
</div>`;
}
@@ -86,6 +98,6 @@
this.serverConfig
);
if (primaryLink) return {...primaryLink, name};
- return {name, url: createSearchUrl({query: name})};
+ return {name, url: createSearchUrl({query: commit})};
}
}
diff --git a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info_test.ts b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info_test.ts
index 7bc4e60..3fed767 100644
--- a/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-commit-info/gr-commit-info_test.ts
@@ -79,7 +79,11 @@
assert.shadowDom.equal(
weblink,
/* HTML */ `
- <a href="/q/sha4567" rel="noopener noreferrer" target="_blank">
+ <a
+ href="/q/sha45678901234567890"
+ rel="noopener noreferrer"
+ target="_blank"
+ >
<gr-tooltip-content>
<span> sha4567 </span>
</gr-tooltip-content>
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
index b66b2cf..bb76a3e 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
@@ -18,6 +18,8 @@
ChangeInfoId,
TopicName,
ChangeActionDialog,
+ EmailInfo,
+ GitPersonInfo,
} from '../../../types/common';
import {customElement, property, query, state} from 'lit/decorators.js';
import {
@@ -30,6 +32,7 @@
ChangeStatus,
ProgressStatus,
} from '../../../constants/constants';
+import {subscribe} from '../../lit/subscription-controller';
import {fire, fireNoBubble} from '../../../utils/event-util';
import {css, html, LitElement, PropertyValues} from 'lit';
import {sharedStyles} from '../../../styles/shared-styles';
@@ -43,6 +46,7 @@
import {ParsedChangeInfo} from '../../../types/types';
import {formStyles} from '../../../styles/form-styles';
import {branchName} from '../../../utils/patch-set-util';
+import {changeModelToken} from '../../../models/change/change-model';
const SUGGESTIONS_LIMIT = 15;
const CHANGE_SUBJECT_LIMIT = 50;
@@ -127,13 +131,24 @@
@state()
private invalidBranch = false;
+ @state()
+ emails: EmailInfo[] = [];
+
@query('#branchInput')
branchInput!: GrTypedAutocomplete<BranchName>;
+ @state()
+ committerEmail?: string;
+
+ @state()
+ latestCommitter?: GitPersonInfo;
+
private selectedChangeIds = new Set<ChangeInfoId>();
private readonly restApiService = getAppContext().restApiService;
+ private readonly getChangeModel = resolve(this, changeModelToken);
+
private readonly reporting = getAppContext().reportingService;
private readonly getNavigation = resolve(this, navigationToken);
@@ -142,6 +157,16 @@
super();
this.statuses = {};
this.query = (text: string) => this.getProjectBranchesSuggestions(text);
+ subscribe(
+ this,
+ () => this.getChangeModel().latestCommitter$,
+ x => (this.latestCommitter = x)
+ );
+ }
+
+ override connectedCallback() {
+ super.connectedCallback();
+ this.loadEmails();
}
override willUpdate(changedProperties: PropertyValues) {
@@ -341,6 +366,17 @@
@bind-value-changed=${(e: BindValueChangeEvent) =>
(this.message = e.detail.value ?? '')}
></iron-autogrow-textarea>
+ ${when(
+ this.canShowEmailDropdown(),
+ () => html`<div id="cherryPickEmailDropdown">Cherry Pick Committer Email
+ <gr-dropdown-list
+ .items=${this.getEmailDropdownItems()}
+ .value=${this.committerEmail}
+ @value-change=${this.setCommitterEmail}
+ >
+ </gr-dropdown-list>
+ <span></div>`
+ )}
`;
}
@@ -580,6 +616,7 @@
topic,
allow_conflicts: true,
allow_empty: true,
+ committer_email: this.committerEmail ? this.committerEmail : null,
};
const handleError = (response?: Response | null) => {
this.handleCherryPickFailed(change, response);
@@ -654,4 +691,38 @@
return branches;
});
}
+
+ async loadEmails() {
+ const accountEmails: EmailInfo[] =
+ (await this.restApiService.getAccountEmails()) ?? [];
+ let selectedEmail: string | undefined;
+ accountEmails.forEach(e => {
+ if (e.preferred) {
+ selectedEmail = e.email;
+ }
+ });
+
+ if (accountEmails.some(e => e.email === this.latestCommitter?.email)) {
+ selectedEmail = this.latestCommitter?.email;
+ }
+ this.emails = accountEmails;
+ this.committerEmail = selectedEmail;
+ }
+
+ private canShowEmailDropdown() {
+ return this.emails.length > 1;
+ }
+
+ private getEmailDropdownItems() {
+ return this.emails.map(e => {
+ return {
+ text: e.email,
+ value: e.email,
+ };
+ });
+ }
+
+ private setCommitterEmail(e: CustomEvent<{value: string}>) {
+ this.committerEmail = e.detail.value;
+ }
}
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
index dc8dba9..83e6f9e 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
@@ -5,7 +5,12 @@
*/
import '../../../test/common-test-setup';
import './gr-confirm-cherrypick-dialog';
-import {queryAll, queryAndAssert, stubRestApi} from '../../../test/test-utils';
+import {
+ query,
+ queryAll,
+ queryAndAssert,
+ stubRestApi,
+} from '../../../test/test-utils';
import {GrConfirmCherrypickDialog} from './gr-confirm-cherrypick-dialog';
import {
BranchName,
@@ -24,11 +29,53 @@
import {GrDialog} from '../../shared/gr-dialog/gr-dialog';
import {ProgressStatus} from '../../../constants/constants';
import {fixture, html, assert} from '@open-wc/testing';
+import {GrDropdownList} from '../../shared/gr-dropdown-list/gr-dropdown-list';
const CHERRY_PICK_TYPES = {
SINGLE_CHANGE: 1,
TOPIC: 2,
};
+
+const changes: ChangeInfo[] = [
+ {
+ ...createChange(),
+ id: '1234' as ChangeInfoId,
+ change_id: '12345678901234' as ChangeId,
+ topic: 'T' as TopicName,
+ subject: 'random',
+ project: 'A' as RepoName,
+ _number: 1 as NumericChangeId,
+ revisions: {
+ a: createRevision(),
+ },
+ current_revision: 'a' as CommitId,
+ },
+ {
+ ...createChange(),
+ id: '5678' as ChangeInfoId,
+ change_id: '23456' as ChangeId,
+ topic: 'T' as TopicName,
+ subject: 'a'.repeat(100),
+ project: 'B' as RepoName,
+ _number: 2 as NumericChangeId,
+ revisions: {
+ a: createRevision(),
+ },
+ current_revision: 'a' as CommitId,
+ },
+];
+
+const emails = [
+ {
+ email: 'primary@email.com',
+ preferred: true,
+ },
+ {
+ email: 'secondary@email.com',
+ preferred: false,
+ },
+];
+
suite('gr-confirm-cherrypick-dialog tests', () => {
let element: GrConfirmCherrypickDialog;
@@ -149,34 +196,6 @@
});
suite('cherry pick topic', () => {
- const changes: ChangeInfo[] = [
- {
- ...createChange(),
- id: '1234' as ChangeInfoId,
- change_id: '12345678901234' as ChangeId,
- topic: 'T' as TopicName,
- subject: 'random',
- project: 'A' as RepoName,
- _number: 1 as NumericChangeId,
- revisions: {
- a: createRevision(),
- },
- current_revision: 'a' as CommitId,
- },
- {
- ...createChange(),
- id: '5678' as ChangeInfoId,
- change_id: '23456' as ChangeId,
- topic: 'T' as TopicName,
- subject: 'a'.repeat(100),
- project: 'B' as RepoName,
- _number: 2 as NumericChangeId,
- revisions: {
- a: createRevision(),
- },
- current_revision: 'a' as CommitId,
- },
- ];
setup(async () => {
element.updateChanges(changes);
element.cherryPickType = CHERRY_PICK_TYPES.TOPIC;
@@ -290,4 +309,36 @@
assert.equal(branches.length, 1);
assert.equal(branches[0].name, 'test-branch');
});
+
+ suite('cherry pick single change with committer email', () => {
+ test('hide email dropdown when user has one email', async () => {
+ element.emails = emails.slice(0, 1);
+ await element.updateComplete;
+ assert.notExists(query(element, '#cherryPickEmailDropdown'));
+ });
+
+ test('show email dropdown when user has more than one email', async () => {
+ element.emails = emails;
+ await element.updateComplete;
+ const cherryPickEmailDropdown = queryAndAssert(
+ element,
+ '#cherryPickEmailDropdown'
+ );
+ assert.dom.equal(
+ cherryPickEmailDropdown,
+ `<div id="cherryPickEmailDropdown">Cherry Pick Committer Email
+ <gr-dropdown-list></gr-dropdown-list>
+ <span></span>
+ </div>`
+ );
+ const emailDropdown = queryAndAssert<GrDropdownList>(
+ cherryPickEmailDropdown,
+ 'gr-dropdown-list'
+ );
+ assert.deepEqual(
+ emailDropdown.items?.map(e => e.value),
+ emails.map(e => e.email)
+ );
+ });
+ });
});
diff --git a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
index 7cfebf1..72c2e43 100644
--- a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
+++ b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
@@ -4,21 +4,34 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {customElement, state} from 'lit/decorators.js';
-import {css, html, LitElement} from 'lit';
+import {css, html, HTMLTemplateResult, LitElement} from 'lit';
import {resolve} from '../../../models/dependency';
import {subscribe} from '../../lit/subscription-controller';
import {changeModelToken} from '../../../models/change/change-model';
-import {EDIT, RevisionInfo} from '../../../api/rest-api';
+import {
+ CommitId,
+ EDIT,
+ NumericChangeId,
+ ParentInfo,
+ PatchSetNumber,
+ RepoName,
+ RevisionInfo,
+} from '../../../api/rest-api';
import {fontStyles} from '../../../styles/gr-font-styles';
-import {branchName, shorten} from '../../../utils/patch-set-util';
+import {branchName} from '../../../utils/patch-set-util';
import {when} from 'lit/directives/when.js';
+import {createChangeUrl} from '../../../models/views/change';
@customElement('gr-revision-parents')
export class GrRevisionParents extends LitElement {
+ @state() repo?: RepoName;
+
@state() revision?: RevisionInfo;
@state() baseRevision?: RevisionInfo;
+ @state() showDetails = false;
+
private readonly getChangeModel = resolve(this, changeModelToken);
constructor() {
@@ -33,6 +46,11 @@
);
subscribe(
this,
+ () => this.getChangeModel().repo$,
+ x => (this.repo = x)
+ );
+ subscribe(
+ this,
() => this.getChangeModel().baseRevision$,
x => (this.baseRevision = x)
);
@@ -50,7 +68,7 @@
border-top: 1px solid var(--border-color);
background-color: var(--yellow-50);
}
- .flex {
+ .sections {
display: flex;
}
.section {
@@ -63,53 +81,312 @@
.title {
font-weight: var(--font-weight-bold);
}
+ .messageContainer {
+ display: flex;
+ padding: var(--spacing-m) var(--spacing-l);
+ border-top: 1px solid var(--border-color);
+ }
+ .messageContainer.info {
+ background-color: var(--info-background);
+ }
+ .messageContainer.warning {
+ background-color: var(--warning-background);
+ }
+ .messageContainer gr-icon {
+ margin-right: var(--spacing-m);
+ }
+ .messageContainer.info gr-icon {
+ color: var(--info-foreground);
+ }
+ .messageContainer.warning gr-icon {
+ color: var(--warning-foreground);
+ }
+ .messageContainer .text {
+ max-width: 600px;
+ }
+ .messageContainer .text p {
+ margin: 0;
+ }
+ .messageContainer .text gr-button {
+ margin-left: -4px;
+ }
+ gr-commit-info {
+ display: inline-block;
+ }
`,
];
}
override render() {
- // TODO(revision-parents): Figure out what to do about multiple parents.
- const baseParent = this.baseRevision?.parents_data?.[0];
- const parent = this.revision?.parents_data?.[0];
- if (!parent || !baseParent) return;
- // TODO(revision-parents): Design something nicer for the various cases.
+ return html`${this.renderMessage()}${this.renderDetails()}`;
+ }
+
+ private renderMessage() {
+ if (!this.baseRevision || !this.revision) return;
+ // For merges we are only interested in the target branch parent, which is [0].
+ // And for non-merges there is no more than 1 parent, so [0] is the only choice.
+ const parentLeft = this.baseRevision?.parents_data?.[0];
+ const parentRight = this.revision?.parents_data?.[0];
+ // Note that is you diff a patchset against its base, then baseRevision will be
+ // `undefined`. Thus after this line we know that we are dealing with diffs
+ // of the type "patchset x vs patchset y".
+ if (!parentLeft || !parentRight) return;
+
+ const psLeft = this.baseRevision?._number;
+ const psRight = this.revision?._number;
+ const parentCommitLeft = parentLeft.commit_id;
+ const parentCommitRight = parentRight.commit_id;
+ const branchLeft = branchName(parentLeft.branch_name);
+ const branchRight = branchName(parentRight.branch_name);
+ const isMergedLeft = parentLeft.is_merged_in_target_branch;
+ const isMergedRight = parentRight.is_merged_in_target_branch;
+ const changeNumLeft = parentLeft.change_number;
+ const changeNumRight = parentRight.change_number;
+ const changePsLeft = parentLeft.patch_set_number;
+ const changePsRight = parentRight.patch_set_number;
+
+ if (parentCommitLeft === parentCommitRight) return;
+
+ // Subsequently: different commit
+
+ if (branchLeft !== branchRight) {
+ return html`
+ ${this.renderWarning(
+ 'warning',
+ html`
+ Patchset ${psLeft} and ${psRight} are targeting different branches.
+ `
+ )}
+ `;
+ }
+
+ // Subsequently: different commit, same target branch
+
+ // Such a situation is really rare and weird. You have to do something like committing to one
+ // branch and then uploading to another. This warning should actually also be shown, if
+ // you are not comparing PS X and PS Y, because it is generally a weird patchset state.
+ const isWeirdLeft = !isMergedLeft && !changeNumLeft;
+ const isWeirdRight = !isMergedRight && !changeNumRight;
+ if (isWeirdLeft || isWeirdRight) {
+ const weirdPs =
+ isWeirdLeft && isWeirdRight
+ ? `${psLeft} and ${psRight} are`
+ : isWeirdLeft
+ ? `${psLeft} is`
+ : `${psRight} is`;
+ return html`
+ ${this.renderWarning(
+ 'warning',
+ html`
+ Patchset ${weirdPs} based on a commit that neither exists in its
+ target branch, nor is it a commit of another active change.
+ `
+ )}
+ `;
+ }
+
+ if (
+ changeNumLeft &&
+ changeNumRight &&
+ changeNumLeft === changeNumRight &&
+ // This check is probably redundant, because "same change and ps" should mean "same commit".
+ psLeft !== psRight
+ ) {
+ return html`
+ ${this.renderWarning(
+ 'info',
+ html`
+ The change was rebased from patchset
+ ${this.renderPatchsetLink(changeNumLeft, changePsLeft)} onto
+ patchset ${this.renderPatchsetLink(changeNumLeft, changePsRight)} of
+ change ${this.renderChangeLink(changeNumLeft)}
+ ${when(isMergedRight, () => html` (MERGED)`)}.
+ `
+ )}
+ `;
+ }
+
+ // No additional info? Then "different commit" and "same branch" means "standard rebase".
+ if (isMergedLeft && isMergedRight) {
+ return html`
+ ${this.renderWarning(
+ 'info',
+ html`
+ The change was rebased from
+ ${this.renderCommitLink(parentCommitLeft, false)} onto
+ ${this.renderCommitLink(parentCommitRight, false)}.
+ `
+ )}
+ `;
+ }
+
+ // By now we know that we have different commit, same target branch, no weird parent,
+ // and not a standard rebase. So let's spell out what the left and right side are based on.
+ return this.renderWarning(
+ 'warning',
+ html`${this.renderInfo(this.baseRevision)}<br />${this.renderInfo(
+ this.revision
+ )}`
+ );
+ }
+
+ private renderInfo(rev: RevisionInfo) {
+ const parent = rev.parents_data?.[0];
+ if (!parent) return;
+ const ps = rev._number;
+ const isMerged = parent.is_merged_in_target_branch;
+ const changeNum = parent.change_number;
+
+ if (changeNum && !isMerged) {
+ return html`
+ Patchset ${ps} is based on patchset
+ ${this.renderPatchsetLink(changeNum, parent.patch_set_number)} of change
+ ${this.renderChangeLink(changeNum)}.
+ `;
+ } else {
+ return html`
+ Patchset ${ps} is based on commit
+ ${this.renderCommitLink(parent.commit_id, false)} in the target branch
+ (${branchName(parent.branch_name)}).
+ `;
+ }
+ }
+
+ private renderWarning(icon: string, message: HTMLTemplateResult) {
+ const isWarning = icon === 'warning';
+ return html`
+ <div class="messageContainer ${icon}">
+ <div class="icon">
+ <gr-icon icon=${icon}></gr-icon>
+ </div>
+ <div class="text">
+ <p>
+ ${message}${when(
+ isWarning,
+ () => html`
+ <br />
+ The diff below may not be meaningful and may even be hiding
+ relevant changes.
+ `
+ )}
+ </p>
+ ${when(
+ isWarning,
+ () => html`
+ <p>
+ <gr-button
+ link
+ @click=${() => (this.showDetails = !this.showDetails)}
+ >${this.showDetails ? 'Hide' : 'Show'} details</gr-button
+ >
+ </p>
+ `
+ )}
+ </div>
+ </div>
+ `;
+ }
+
+ private renderDetails() {
+ if (!this.showDetails) return;
+ if (!this.baseRevision || !this.revision) return;
+ const parentLeft = this.baseRevision.parents_data?.[0];
+ const parentRight = this.revision.parents_data?.[0];
+ if (!parentRight || !parentLeft) return;
+
return html`
<div class="container">
- <div class="flex">
- <div class="section">
- <h4 class="heading-4">Patchset ${this.baseRevision?._number}</h4>
- <div>Branch: ${branchName(parent.branch_name)}</div>
- <div>Commit: ${shorten(parent.commit_id)}</div>
- <div>Is Merged: ${parent.is_merged_in_target_branch}</div>
- ${when(
- !!parent.change_number,
- () => html` <div>
- Change ID: ${parent.change_id?.substring(0, 10)}
- </div>
- <div>Change Number: ${parent.change_number}</div>
- <div>Patchset Number: ${parent.patch_set_number}</div>
- <div>Change Status: ${parent.change_status}</div>`
- )}
- </div>
- <div class="section">
- <h4 class="heading-4">Patchset ${this.revision?._number}</h4>
- <div>Branch: ${branchName(baseParent.branch_name)}</div>
- <div>Commit: ${shorten(baseParent.commit_id)}</div>
- <div>Is Merged: ${baseParent.is_merged_in_target_branch}</div>
- ${when(
- !!baseParent.change_number,
- () => html`<div>
- Change ID: ${baseParent.change_id?.substring(0, 10)}
- </div>
- <div>Change Number: ${baseParent.change_number}</div>
- <div>Patchset Number: ${baseParent.patch_set_number}</div>
- <div>Change Status: ${baseParent.change_status}</div>`
- )}
- </div>
+ <div class="sections">
+ ${this.renderSection(
+ this.baseRevision,
+ parentLeft,
+ parentLeft.change_number === parentRight.change_number
+ )}
+ ${this.renderSection(
+ this.revision,
+ parentRight,
+ parentLeft.change_number === parentRight.change_number
+ )}
</div>
</div>
`;
}
+
+ private renderCommitLink(commit?: CommitId, showCopyButton = true) {
+ if (!commit) return;
+ return html`<gr-commit-info
+ .commitInfo=${{commit}}
+ .showCopyButton=${showCopyButton}
+ ></gr-commit-info>`;
+ }
+
+ private renderChangeLink(changeNum: NumericChangeId) {
+ return html`
+ <a href=${createChangeUrl({changeNum, repo: this.repo!})}>${changeNum}</a>
+ `;
+ }
+
+ private renderPatchsetLink(
+ changeNum: NumericChangeId,
+ patchNum?: PatchSetNumber
+ ) {
+ if (!patchNum) return;
+ return html`
+ <a
+ href=${createChangeUrl({
+ changeNum,
+ repo: this.repo!,
+ patchNum,
+ })}
+ >${patchNum}</a
+ >
+ `;
+ }
+
+ private renderSection(
+ revision: RevisionInfo,
+ parent: ParentInfo,
+ sameChange: boolean
+ ) {
+ const ps = revision._number;
+ const commit = parent.commit_id;
+ const branch = branchName(parent.branch_name);
+ const isMerged = parent.is_merged_in_target_branch;
+ const changeNum = parent.change_number as NumericChangeId;
+ const changePs = parent.patch_set_number;
+
+ createChangeUrl({changeNum, repo: this.repo!});
+
+ return html`
+ <div class="section">
+ <h4 class="heading-4">Patchset ${ps}</h4>
+ <div>Target branch: ${branch}</div>
+ <div>Base commit: ${this.renderCommitLink(commit)}</div>
+ ${when(
+ !changeNum && !isMerged,
+ () => html`
+ <div>
+ <gr-icon icon="warning"></gr-icon>
+ <span
+ >Warning: The base commit is not known (aka reachable) in the
+ target branch.</span
+ >
+ </div>
+ `
+ )}
+ ${when(
+ changeNum && (sameChange || !isMerged),
+ () => html`
+ <div>
+ Base change: ${this.renderChangeLink(changeNum)}, patchset
+ ${this.renderPatchsetLink(changeNum, changePs)}
+ ${when(isMerged, () => html`(MERGED)`)}
+ </div>
+ `
+ )}
+ </div>
+ `;
+ }
}
declare global {
diff --git a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
index a1b9fdd..7b97550 100644
--- a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
@@ -13,12 +13,68 @@
ChangeStatus,
CommitId,
NumericChangeId,
+ ParentInfo,
PatchSetNumber,
} from '../../../api/rest-api';
+import {queryAll} from '../../../utils/common-util';
+
+const PARENT_DEFAULT: ParentInfo = {
+ branch_name: 'refs/heads/master',
+ commit_id: '78e52ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+ is_merged_in_target_branch: true,
+};
+
+const PARENT_REBASED: ParentInfo = {
+ ...PARENT_DEFAULT,
+ commit_id: '00002ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+};
+
+const PARENT_OTHER_BRANCH: ParentInfo = {
+ ...PARENT_DEFAULT,
+ branch_name: 'refs/heads/otherbranch',
+ commit_id: '11112ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+};
+
+const PARENT_WEIRD: ParentInfo = {
+ ...PARENT_DEFAULT,
+ commit_id: '22222ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+ is_merged_in_target_branch: false,
+};
+
+const PARENT_CHANGE_123_1: ParentInfo = {
+ ...PARENT_DEFAULT,
+ commit_id: '12312ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+ is_merged_in_target_branch: false,
+ change_id: 'Idc69e6d7bba0ce0a9a0bdcd22adb506c0b76e628' as ChangeId,
+ change_number: 123 as NumericChangeId,
+ patch_set_number: 1 as PatchSetNumber,
+ change_status: ChangeStatus.NEW,
+};
+
+const PARENT_CHANGE_123_2: ParentInfo = {
+ ...PARENT_CHANGE_123_1,
+ commit_id: '12322ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
+ patch_set_number: 2 as PatchSetNumber,
+};
suite('gr-revision-parents tests', () => {
let element: GrRevisionParents;
+ const setParents = async (
+ parentLeft: ParentInfo,
+ parentRight: ParentInfo
+ ) => {
+ element.baseRevision = {
+ ...createRevision(1),
+ parents_data: [parentLeft],
+ };
+ element.revision = {
+ ...createRevision(2),
+ parents_data: [parentRight],
+ };
+ await element.updateComplete;
+ };
+
setup(async () => {
element = await fixture(html`<gr-revision-parents></gr-revision-parents>`);
await element.updateComplete;
@@ -28,65 +84,155 @@
assert.shadowDom.equal(element, '');
});
- test('render', async () => {
- element.baseRevision = {
- ...createRevision(1),
- parents_data: [
- {
- branch_name: 'refs/heads/master',
- commit_id: '78e52ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
- is_merged_in_target_branch: false,
- change_id: 'Idc69e6d7bba0ce0a9a0bdcd22adb506c0b76e628' as ChangeId,
- change_number: 1500 as NumericChangeId,
- patch_set_number: 1 as PatchSetNumber,
- change_status: ChangeStatus.NEW,
- },
- ],
- };
- element.revision = {
- ...createRevision(2),
- parents_data: [
- {
- branch_name: 'refs/heads/master',
- commit_id: '78e52ce873b1c08396422f51ad6aacf77ed95541' as CommitId,
- is_merged_in_target_branch: false,
- change_id: 'Idc69e6d7bba0ce0a9a0bdcd22adb506c0b76e628' as ChangeId,
- change_number: 1500 as NumericChangeId,
- patch_set_number: 2 as PatchSetNumber,
- change_status: ChangeStatus.NEW,
- },
- ],
- };
- await element.updateComplete;
-
- assert.shadowDom.equal(
- element,
+ test('render details: PARENT_DEFAULT', async () => {
+ element.showDetails = true;
+ await setParents(PARENT_DEFAULT, PARENT_DEFAULT);
+ assert.dom.equal(
+ queryAll(element, '.section')[0],
/* HTML */ `
- <div class="container">
- <div class="flex">
- <div class="section">
- <h4 class="heading-4">Patchset 1</h4>
- <div>Branch: master</div>
- <div>Commit: 78e52ce</div>
- <div>Is Merged: false</div>
- <div>Change ID: Idc69e6d7b</div>
- <div>Change Number: 1500</div>
- <div>Patchset Number: 2</div>
- <div>Change Status: NEW</div>
- </div>
- <div class="section">
- <h4 class="heading-4">Patchset 2</h4>
- <div>Branch: master</div>
- <div>Commit: 78e52ce</div>
- <div>Is Merged: false</div>
- <div>Change ID: Idc69e6d7b</div>
- <div>Change Number: 1500</div>
- <div>Patchset Number: 1</div>
- <div>Change Status: NEW</div>
- </div>
+ <div class="section">
+ <h4 class="heading-4">Patchset 1</h4>
+ <div>Target branch: master</div>
+ <div>
+ Base commit:
+ <gr-commit-info> </gr-commit-info>
</div>
</div>
`
);
});
+
+ test('render details: PARENT_WEIRD', async () => {
+ element.showDetails = true;
+ await setParents(PARENT_WEIRD, PARENT_WEIRD);
+ assert.dom.equal(
+ queryAll(element, '.section')[0],
+ `
+ <div class="section">
+ <h4 class="heading-4">Patchset 1</h4>
+ <div>Target branch: master</div>
+ <div>
+ Base commit:
+ <gr-commit-info> </gr-commit-info>
+ </div>
+ <div>
+ <gr-icon icon="warning"> </gr-icon>
+ <span>
+ Warning: The base commit is not known (aka reachable) in the
+ target branch.
+ </span>
+ </div>
+ </div>
+ `
+ );
+ });
+
+ test('render details: PARENT_CHANGE_123_1', async () => {
+ element.showDetails = true;
+ await setParents(PARENT_CHANGE_123_1, PARENT_CHANGE_123_2);
+ assert.dom.equal(
+ queryAll(element, '.section')[0],
+ /* HTML */ `
+ <div class="section">
+ <h4 class="heading-4">Patchset 1</h4>
+ <div>Target branch: master</div>
+ <div>
+ Base commit:
+ <gr-commit-info> </gr-commit-info>
+ </div>
+ <div>
+ Base change:
+ <a href="/c/123"> 123 </a>
+ , patchset
+ <a href="/c/123/1"> 1 </a>
+ </div>
+ </div>
+ `
+ );
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_DEFAULT', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_DEFAULT);
+ assert.shadowDom.equal(element, '');
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_OTHER_BRANCH', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_OTHER_BRANCH);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer warning">
+ <div class="icon"><gr-icon icon="warning"></gr-icon></div>
+ <div class="text"><p>
+ Patchset 1 and 2 are targeting different branches.<br/>
+ The diff below may not be meaningful and may even be hiding
+ relevant changes.
+ </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ );
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_WEIRD', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_WEIRD);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer warning">
+ <div class="icon"><gr-icon icon="warning"></gr-icon></div>
+ <div class="text"><p>
+ Patchset 2 is based on a commit that neither exists in its
+ target branch, nor is it a commit of another active change.<br/>
+ The diff below may not be meaningful and may even be hiding
+ relevant changes.
+ </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ );
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_REBASED', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_REBASED);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer info">
+ <div class="icon"><gr-icon icon="info"></gr-icon></div>
+ <div class="text"><p>
+ The change was rebased from <gr-commit-info></gr-commit-info>
+ onto <gr-commit-info></gr-commit-info>.
+ </p></div></div>`
+ );
+ });
+
+ test('render message PARENT_CHANGE_123_1 vs PARENT_CHANGE_123_2', async () => {
+ await setParents(PARENT_CHANGE_123_1, PARENT_CHANGE_123_2);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer info">
+ <div class="icon"><gr-icon icon="info"></gr-icon></div>
+ <div class="text"><p>
+ The change was rebased from patchset
+ <a href="/c/123/1">1</a> onto
+ patchset
+ <a href="/c/123/2">2</a> of
+ change
+ <a href="/c/123">123</a>.
+ </p></div></div>`
+ );
+ });
+
+ test('render message PARENT_DEFAULT vs PARENT_CHANGE_123_1', async () => {
+ await setParents(PARENT_DEFAULT, PARENT_CHANGE_123_1);
+ assert.shadowDom.equal(
+ element,
+ `<div class="messageContainer warning">
+ <div class="icon"><gr-icon icon="warning"></gr-icon></div>
+ <div class="text"><p>
+ Patchset 1 is based on commit
+ <gr-commit-info></gr-commit-info>
+ in the target branch
+ (master).<br>
+ Patchset 2 is based on patchset
+ <a href="/c/123/1">1</a>
+ of change
+ <a href="/c/123">123</a>.<br>
+ The diff below may not be meaningful and may even be hiding
+ relevant changes.
+ </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ );
+ });
});
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 68a0c5a..ac35264 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -79,7 +79,6 @@
SettingsViewState,
} from '../../../models/views/settings';
import {define} from '../../../models/dependency';
-import {Finalizable} from '../../../services/registry';
import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
import {
@@ -105,6 +104,7 @@
noAwait,
timeoutPromise,
} from '../../../utils/async-util';
+import {Finalizable} from '../../../types/types';
// TODO: Move all patterns to view model files and use the `Route` interface,
// which will enforce using `RegExp` in its `urlPattern` property.
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
index 6a05d86..6bf1adc 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
@@ -18,6 +18,7 @@
convertToPatchSetNum,
getParentInfoString,
shorten,
+ getParentCommit,
} from '../../../utils/patch-set-util';
import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
import {
@@ -276,7 +277,8 @@
KnownExperimentId.REVISION_PARENTS_DATA
);
dropdownContent.push({
- text: isMerge ? 'Auto Merge' : 'Base',
+ triggerText: isMerge ? 'Auto Merge' : 'Base',
+ text: isMerge ? 'Auto Merge' : `Base | ${getParentCommit(rev, 0)}`,
bottomText:
showParentsData && !isMerge ? getParentInfoString(rev, 0) : undefined,
value: PARENT,
@@ -286,7 +288,7 @@
dropdownContent.push({
disabled: idx >= parentCount,
triggerText: `Parent ${idx + 1}`,
- text: `Parent ${idx + 1}`,
+ text: `Parent ${idx + 1} | ${getParentCommit(rev, idx)}`,
bottomText: showParentsData ? getParentInfoString(rev, idx) : undefined,
mobileText: `Parent ${idx + 1}`,
value: -(idx + 1),
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
index 0f2dd1e..e7ed97b 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
@@ -180,7 +180,8 @@
commentThreads: [],
} as DropdownItem,
{
- text: 'Base',
+ text: 'Base | ',
+ triggerText: 'Base',
value: PARENT,
bottomText: undefined,
} as DropdownItem,
diff --git a/polygerrit-ui/app/elements/gr-app-global-var-init.ts b/polygerrit-ui/app/elements/gr-app-global-var-init.ts
index ec46d8d..45dc3b6 100644
--- a/polygerrit-ui/app/elements/gr-app-global-var-init.ts
+++ b/polygerrit-ui/app/elements/gr-app-global-var-init.ts
@@ -22,7 +22,7 @@
initClickReporter,
initInteractionReporter,
} from '../services/gr-reporting/gr-reporting_impl';
-import {Finalizable} from '../services/registry';
+import {Finalizable} from '../types/types';
export function initGlobalVariables(
appContext: AppContext & Finalizable,
diff --git a/polygerrit-ui/app/elements/gr-app.ts b/polygerrit-ui/app/elements/gr-app.ts
index 40869a9..2ea6983 100644
--- a/polygerrit-ui/app/elements/gr-app.ts
+++ b/polygerrit-ui/app/elements/gr-app.ts
@@ -24,7 +24,6 @@
import {initGerrit, initGlobalVariables} from './gr-app-global-var-init';
import './gr-app-element';
-import {Finalizable} from '../services/registry';
import {
DependencyError,
DependencyToken,
@@ -46,6 +45,7 @@
} from '../services/service-worker-installer';
import {pluginLoaderToken} from './shared/gr-js-api-interface/gr-plugin-loader';
import {getAppContext} from '../services/app-context';
+import {Finalizable} from '../types/types';
initGlobalVariables(createAppContext(), true);
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
index cd73727..6b9c684 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
@@ -20,9 +20,8 @@
ShowRevisionActionsDetail,
} from './gr-js-api-types';
import {EventType, TargetElement} from '../../../api/plugin';
-import {ParsedChangeInfo} from '../../../types/types';
+import {Finalizable, ParsedChangeInfo} from '../../../types/types';
import {MenuLink} from '../../../api/admin';
-import {Finalizable} from '../../../services/registry';
import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
import {Provider} from '../../../models/dependency';
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
index afa16b9..6c180d7 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
@@ -11,9 +11,8 @@
ReviewInput,
RevisionInfo,
} from '../../../types/common';
-import {Finalizable} from '../../../services/registry';
import {EventType, TargetElement} from '../../../api/plugin';
-import {ParsedChangeInfo} from '../../../types/types';
+import {Finalizable, ParsedChangeInfo} from '../../../types/types';
import {MenuLink} from '../../../api/admin';
import {FileRange, PatchRange} from '../../../api/diff';
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.ts
index e5ead0b..b78af2a 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.ts
@@ -17,7 +17,6 @@
import {fireAlert} from '../../../utils/event-util';
import {JsApiService} from './gr-js-api-types';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
-import {Finalizable} from '../../../services/registry';
import {PluginsModel} from '../../../models/plugins/plugins-model';
import {Gerrit} from '../../../api/gerrit';
import {fontStyles} from '../../../styles/gr-font-styles';
@@ -30,6 +29,7 @@
import {GrJsApiInterface} from './gr-js-api-interface-element';
import {define} from '../../../models/dependency';
import {modalStyles} from '../../../styles/gr-modal-styles';
+import {Finalizable} from '../../../types/types';
enum PluginState {
/** State that indicates the plugin is pending to be loaded. */
diff --git a/polygerrit-ui/app/embed/gr-diff-app-context-init.ts b/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
index 36ebb9f..bad23cf 100644
--- a/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
+++ b/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
@@ -3,11 +3,12 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {create, Registry, Finalizable} from '../services/registry';
+import {create, Registry} from '../services/registry';
import {AppContext} from '../services/app-context';
import {AuthService} from '../services/gr-auth/gr-auth';
import {FlagsService} from '../services/flags/flags';
import {grReportingMock} from '../services/gr-reporting/gr-reporting_mock';
+import {Finalizable} from '../types/types';
class MockFlagsService implements FlagsService {
isEnabled() {
diff --git a/polygerrit-ui/app/models/base/model.ts b/polygerrit-ui/app/models/base/model.ts
index 4ddef48..4574b31f 100644
--- a/polygerrit-ui/app/models/base/model.ts
+++ b/polygerrit-ui/app/models/base/model.ts
@@ -4,8 +4,8 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {BehaviorSubject, Observable, Subscription} from 'rxjs';
-import {Finalizable} from '../../services/registry';
import {deepEqual} from '../../utils/deep-util';
+import {Finalizable} from '../../types/types';
/**
* A Model stores a value <T> and controls changes to that value via `subject$`
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index f1d28db..2414107 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -248,6 +248,11 @@
change => change?.revisions[change.current_revision]?.uploader
);
+ public readonly latestCommitter$ = select(
+ this.change$,
+ change => change?.revisions[change.current_revision]?.commit?.committer
+ );
+
/**
* Emits the current patchset number. If the route does not define the current
* patchset num, then this selector waits for the change to be defined and
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 8589ae3..87972de 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {AppContext} from './app-context';
-import {create, Finalizable, Registry} from './registry';
+import {create, Registry} from './registry';
import {DependencyToken} from '../models/dependency';
import {FlagsServiceImplementation} from './flags/flags_impl';
import {GrReporting} from './gr-reporting/gr-reporting_impl';
@@ -72,6 +72,7 @@
RelatedChangesModel,
relatedChangesModelToken,
} from '../models/change/related-changes-model';
+import {Finalizable} from '../types/types';
/**
* The AppContext lazy initializator for all services
diff --git a/polygerrit-ui/app/services/app-context-init_test.ts b/polygerrit-ui/app/services/app-context-init_test.ts
index 7834e53..3572189 100644
--- a/polygerrit-ui/app/services/app-context-init_test.ts
+++ b/polygerrit-ui/app/services/app-context-init_test.ts
@@ -5,9 +5,9 @@
*/
import '../test/common-test-setup';
import {AppContext} from './app-context';
-import {Finalizable} from './registry';
import {createTestAppContext} from '../test/test-app-context-init';
import {assert} from '@open-wc/testing';
+import {Finalizable} from '../types/types';
suite('app context initializer tests', () => {
let appContext: AppContext & Finalizable;
diff --git a/polygerrit-ui/app/services/app-context.ts b/polygerrit-ui/app/services/app-context.ts
index aa2c032..eeddc8e 100644
--- a/polygerrit-ui/app/services/app-context.ts
+++ b/polygerrit-ui/app/services/app-context.ts
@@ -3,11 +3,11 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {Finalizable} from './registry';
import {FlagsService} from './flags/flags';
import {ReportingService} from './gr-reporting/gr-reporting';
import {AuthService} from './gr-auth/gr-auth';
import {RestApiService} from './gr-rest-api/gr-rest-api';
+import {Finalizable} from '../types/types';
export interface AppContext {
flagsService: FlagsService;
diff --git a/polygerrit-ui/app/services/flags/flags.ts b/polygerrit-ui/app/services/flags/flags.ts
index 1ae23cf..b7d36f4 100644
--- a/polygerrit-ui/app/services/flags/flags.ts
+++ b/polygerrit-ui/app/services/flags/flags.ts
@@ -3,7 +3,8 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {Finalizable} from '../registry';
+
+import {Finalizable} from '../../types/types';
export interface FlagsService extends Finalizable {
isEnabled(experimentId: string): boolean;
diff --git a/polygerrit-ui/app/services/flags/flags_impl.ts b/polygerrit-ui/app/services/flags/flags_impl.ts
index 4ef55a2..ca9aa60 100644
--- a/polygerrit-ui/app/services/flags/flags_impl.ts
+++ b/polygerrit-ui/app/services/flags/flags_impl.ts
@@ -3,8 +3,8 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
+import {Finalizable} from '../../types/types';
import {FlagsService} from './flags';
-import {Finalizable} from '../registry';
declare global {
interface Window {
diff --git a/polygerrit-ui/app/services/gr-auth/gr-auth.ts b/polygerrit-ui/app/services/gr-auth/gr-auth.ts
index 945d6f9..a6b4fdd 100644
--- a/polygerrit-ui/app/services/gr-auth/gr-auth.ts
+++ b/polygerrit-ui/app/services/gr-auth/gr-auth.ts
@@ -4,8 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {define} from '../../models/dependency';
-import {AuthRequestInit} from '../../types/types';
-import {Finalizable} from '../registry';
+import {AuthRequestInit, Finalizable} from '../../types/types';
export enum AuthType {
XSRF_TOKEN = 'xsrf_token',
ACCESS_TOKEN = 'access_token',
diff --git a/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts b/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
index 2312fc9..fb53d5e 100644
--- a/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
+++ b/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
@@ -3,10 +3,9 @@
* Copyright 2017 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {AuthRequestInit} from '../../types/types';
+import {AuthRequestInit, Finalizable} from '../../types/types';
import {fire} from '../../utils/event-util';
import {getBaseUrl} from '../../utils/url-util';
-import {Finalizable} from '../registry';
import {
AuthService,
AuthStatus,
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
index 9b08a6e..6df2c67 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
@@ -3,7 +3,6 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {Finalizable} from '../registry';
import {NumericChangeId} from '../../types/common';
import {EventDetails, ReportingOptions} from '../../api/reporting';
import {PluginApi} from '../../api/plugin';
@@ -13,6 +12,7 @@
LifeCycle,
Timing,
} from '../../constants/reporting';
+import {Finalizable} from '../../types/types';
export type EventValue = string | number | {error?: Error};
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
index 61534a4..84b9481 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
@@ -9,7 +9,6 @@
import {NumericChangeId} from '../../types/common';
import {Deduping, EventDetails, ReportingOptions} from '../../api/reporting';
import {PluginApi} from '../../api/plugin';
-import {Finalizable} from '../registry';
import {
Execution,
Interaction,
@@ -18,6 +17,7 @@
} from '../../constants/reporting';
import {onCLS, onFID, onLCP, Metric, onINP} from 'web-vitals';
import {getEventPath, isElementTarget} from '../../utils/dom-util';
+import {Finalizable} from '../../types/types';
// Latency reporting constants.
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
index fd1b88c..fb1f0c3 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
@@ -7,7 +7,7 @@
import {EventDetails} from '../../api/reporting';
import {PluginApi} from '../../api/plugin';
import {Execution, Interaction} from '../../constants/reporting';
-import {Finalizable} from '../registry';
+import {Finalizable} from '../../types/types';
export class MockTimer implements Timer {
end(): this {
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
index 5339e38..a942cd4 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
@@ -20,7 +20,6 @@
import {GrReviewerUpdatesParser} from '../../elements/shared/gr-rest-api-interface/gr-reviewer-updates-parser';
import {parseDate} from '../../utils/date-util';
import {getBaseUrl} from '../../utils/url-util';
-import {Finalizable} from '../registry';
import {getParentIndex, isMergeParent} from '../../utils/patch-set-util';
import {listChangesOptionsToHex} from '../../utils/change-util';
import {assertNever, hasOwnProperty} from '../../utils/common-util';
@@ -139,7 +138,11 @@
ReviewerState,
} from '../../constants/constants';
import {firePageError, fireServerError} from '../../utils/event-util';
-import {AuthRequestInit, ParsedChangeInfo} from '../../types/types';
+import {
+ AuthRequestInit,
+ Finalizable,
+ ParsedChangeInfo,
+} from '../../types/types';
import {ErrorCallback} from '../../api/rest';
import {addDraftProp} from '../../utils/comment-util';
import {BaseScheduler, Scheduler} from '../scheduler/scheduler';
@@ -790,11 +793,14 @@
}) as Promise<AccountDetailInfo | undefined>;
}
- getAccountEmails() {
- return this._fetchSharedCacheURL({
- url: '/accounts/self/emails',
- reportUrlAsIs: true,
- }) as Promise<EmailInfo[] | undefined>;
+ async getAccountEmails() {
+ const isloggedIn = await this.getLoggedIn();
+ if (isloggedIn) {
+ return this._fetchSharedCacheURL({
+ url: '/accounts/self/emails',
+ reportUrlAsIs: true,
+ }) as Promise<EmailInfo[] | undefined>;
+ } else return;
}
addAccountEmail(email: string): Promise<Response> {
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
index 3916094..b081ff8 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
@@ -4,7 +4,6 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {HttpMethod} from '../../constants/constants';
-import {Finalizable} from '../registry';
import {
AccountCapabilityInfo,
AccountDetailInfo,
@@ -98,7 +97,7 @@
DiffPreferencesInfo,
IgnoreWhitespaceType,
} from '../../types/diff';
-import {ParsedChangeInfo} from '../../types/types';
+import {Finalizable, ParsedChangeInfo} from '../../types/types';
import {ErrorCallback} from '../../api/rest';
export type CancelConditionCallback = () => boolean;
diff --git a/polygerrit-ui/app/services/highlight/highlight-service.ts b/polygerrit-ui/app/services/highlight/highlight-service.ts
index d10d875..d59431b 100644
--- a/polygerrit-ui/app/services/highlight/highlight-service.ts
+++ b/polygerrit-ui/app/services/highlight/highlight-service.ts
@@ -11,10 +11,10 @@
SyntaxWorkerMessageType,
SyntaxLayerLine,
} from '../../types/syntax-worker-api';
+import {Finalizable} from '../../types/types';
import {prependOrigin} from '../../utils/url-util';
import {createWorker} from '../../utils/worker-util';
import {ReportingService} from '../gr-reporting/gr-reporting';
-import {Finalizable} from '../registry';
const hljsLibUrl = `${
window.STATIC_RESOURCE_PATH ?? ''
diff --git a/polygerrit-ui/app/services/registry.ts b/polygerrit-ui/app/services/registry.ts
index 48a5241..a88aec3 100644
--- a/polygerrit-ui/app/services/registry.ts
+++ b/polygerrit-ui/app/services/registry.ts
@@ -4,11 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
-// A finalizable object has a single method `finalize` that is called when
-// the object is no longer needed and should clean itself up.
-export interface Finalizable {
- finalize(): void;
-}
+import {Finalizable} from '../types/types';
// A factory can take a partially created TContext and generate a property
// for a given key on that TContext.
diff --git a/polygerrit-ui/app/services/registry_test.ts b/polygerrit-ui/app/services/registry_test.ts
index 639cd64..15d37d9 100644
--- a/polygerrit-ui/app/services/registry_test.ts
+++ b/polygerrit-ui/app/services/registry_test.ts
@@ -3,9 +3,10 @@
* Copyright 2021 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {create, Finalizable, Registry} from './registry';
+import {create, Registry} from './registry';
import '../test/common-test-setup';
import {assert} from '@open-wc/testing';
+import {Finalizable} from '../types/types';
class Foo implements Finalizable {
constructor(private readonly final: string[]) {}
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
index 756c209..db9d8fe 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -22,10 +22,10 @@
ShortcutOptions,
} from '../../utils/dom-util';
import {ReportingService} from '../gr-reporting/gr-reporting';
-import {Finalizable} from '../registry';
import {UserModel} from '../../models/user/user-model';
import {define} from '../../models/dependency';
import {isCharacterLetter, isUpperCase} from '../../utils/string-util';
+import {Finalizable} from '../../types/types';
export {Shortcut, ShortcutSection};
diff --git a/polygerrit-ui/app/services/storage/gr-storage.ts b/polygerrit-ui/app/services/storage/gr-storage.ts
index d7eb09a..0b34614 100644
--- a/polygerrit-ui/app/services/storage/gr-storage.ts
+++ b/polygerrit-ui/app/services/storage/gr-storage.ts
@@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {NumericChangeId} from '../../types/common';
-import {Finalizable} from '../registry';
+import {Finalizable} from '../../types/types';
export interface StorageObject {
message?: string;
diff --git a/polygerrit-ui/app/services/storage/gr-storage_impl.ts b/polygerrit-ui/app/services/storage/gr-storage_impl.ts
index 7a47e0e..9ddba01 100644
--- a/polygerrit-ui/app/services/storage/gr-storage_impl.ts
+++ b/polygerrit-ui/app/services/storage/gr-storage_impl.ts
@@ -4,9 +4,9 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {StorageObject, StorageService} from './gr-storage';
-import {Finalizable} from '../registry';
import {NumericChangeId} from '../../types/common';
import {define} from '../../models/dependency';
+import {Finalizable} from '../../types/types';
export const DURATION_DAY = 24 * 60 * 60 * 1000;
diff --git a/polygerrit-ui/app/test/common-test-setup.ts b/polygerrit-ui/app/test/common-test-setup.ts
index ed472cc..82c1bb9 100644
--- a/polygerrit-ui/app/test/common-test-setup.ts
+++ b/polygerrit-ui/app/test/common-test-setup.ts
@@ -7,7 +7,6 @@
// https://github.com/Polymer/polymer-resin/issues/9 is resolved.
import '../scripts/bundled-polymer';
import {getAppContext} from '../services/app-context';
-import {Finalizable} from '../services/registry';
import {
createTestAppContext,
createTestDependencies,
@@ -40,6 +39,7 @@
import '../styles/themes/app-theme';
import {Creator} from '../services/app-context-init';
import {pluginLoaderToken} from '../elements/shared/gr-js-api-interface/gr-plugin-loader';
+import {Finalizable} from '../types/types';
declare global {
interface Window {
diff --git a/polygerrit-ui/app/test/test-app-context-init.ts b/polygerrit-ui/app/test/test-app-context-init.ts
index da6a92d..6b9f507 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -5,7 +5,7 @@
*/
// Init app context before any other imports
-import {create, Registry, Finalizable} from '../services/registry';
+import {create, Registry} from '../services/registry';
import {AppContext} from '../services/app-context';
import {grReportingMock} from '../services/gr-reporting/gr-reporting_mock';
import {grRestApiMock} from './mocks/gr-rest-api_mock';
@@ -22,6 +22,7 @@
diffModelToken,
DiffModel,
} from '../embed/diff/gr-diff-model/gr-diff-model';
+import {Finalizable} from '../types/types';
export function createTestAppContext(): AppContext & Finalizable {
const appRegistry: Registry<AppContext> = {
diff --git a/polygerrit-ui/app/types/types.ts b/polygerrit-ui/app/types/types.ts
index 17720fd..43e06a4 100644
--- a/polygerrit-ui/app/types/types.ts
+++ b/polygerrit-ui/app/types/types.ts
@@ -19,6 +19,12 @@
Timestamp,
} from './common';
+// A finalizable object has a single method `finalize` that is called when
+// the object is no longer needed and should clean itself up.
+export interface Finalizable {
+ finalize(): void;
+}
+
export function isDefined<T>(x: T): x is NonNullable<T> {
return x !== undefined && x !== null;
}
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index dffc091..7a5cd45 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -327,24 +327,32 @@
return branch as BranchName;
}
-export function getParentInfoString(
+export function getParentCommit(
rev?: RevisionInfo | EditRevisionInfo,
index?: number
) {
const parents = rev?.parents_data ?? [];
const parent = parents[index ?? 0];
if (!parent) return '';
+ return shorten(parent.commit_id) ?? '';
+}
- let info = '';
- if (parent.change_number) {
- info = `${info}Change ${parent.change_number} at patchset ${parent.patch_set_number}\n`;
- }
+export function getParentInfoString(
+ rev?: RevisionInfo | EditRevisionInfo,
+ index?: number
+) {
+ const parents = rev?.parents_data ?? [];
+ const parent = parents[index ?? 0];
+ if (!parent || parent.is_merged_in_target_branch) return '';
- if (parent.branch_name) {
- const commit = shorten(parent.commit_id) ?? 'unknown';
- info = `${info}Branch ${branchName(
- parent.branch_name
- )} at commit ${commit} `;
+ if (index === 0) {
+ if (parent.change_number) {
+ return `Patchset ${parent.patch_set_number} of Change ${parent.change_number}`;
+ } else {
+ return 'Warning: The base commit is not known (aka reachable) in the target branch.';
+ }
+ } else {
+ // For merge changes the parents with index > 0 are expected to be from a different branch.
+ return 'Other branch';
}
- return info;
}
