Merge "DeleteTag: remove unreachable check"
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index f5b7282..159e2fc 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -1007,17 +1007,9 @@
Runtime exceptions generated by the implementors of ChangePluginDefinedInfoFactory
are encapsulated in PluginDefinedInfo objects which are part of SSH/REST query output.
-==== ChangeAttributeFactory
-
-Alternatively, there is also `ChangeAttributeFactory` which takes in one single
-`ChangeData` at a time. `ChangePluginDefinedInfoFactory` should be preferred
-over this as it handles many changes at once which also decreases the round-trip
-time for queries resulting in performance increase for bulk queries.
-
-Implementors of the `ChangePluginDefinedInfoFactory` and `ChangeAttributeFactory`
-interfaces should check whether they need to contribute to the
-link:#change-etag-computation[change ETag computation] to prevent callers using
-ETags from potentially seeing outdated plugin attributes.
+Implementors of the `ChangePluginDefinedInfoFactory` interface should check whether
+they need to contribute to the link:#change-etag-computation[change ETag computation]
+to prevent callers using ETags from potentially seeing outdated plugin attributes.
[[simple-configuration]]
== Simple Configuration in `gerrit.config`
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index 50ebad7..a809eab 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -1848,8 +1848,9 @@
Gets whether the source is mergeable with the target branch.
-The `source` query parameter is required, which can be anything that could be
-resolved to a commit, see examples of the `source` attribute in
+The `source` query parameter is required, which can be anything that
+could be resolved to a commit, and is visible to the caller. See
+examples of the `source` attribute in
link:rest-api-changes.html#merge-input[MergeInput].
Also takes an optional parameter `strategy`, which can be `recursive`, `resolve`,
diff --git a/java/com/google/gerrit/acceptance/AbstractPluginFieldsTest.java b/java/com/google/gerrit/acceptance/AbstractPluginFieldsTest.java
index a91bc49..29dc6a3 100644
--- a/java/com/google/gerrit/acceptance/AbstractPluginFieldsTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractPluginFieldsTest.java
@@ -30,7 +30,6 @@
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.DynamicOptions.DynamicBean;
-import com.google.gerrit.server.change.ChangeAttributeFactory;
import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.restapi.change.GetChange;
@@ -40,7 +39,6 @@
import com.google.gson.reflect.TypeToken;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
-import com.google.inject.Module;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
@@ -86,21 +84,6 @@
}
}
- protected static class NullAttributeModule extends AbstractModule {
- @Override
- public void configure() {
- DynamicSet.bind(binder(), ChangeAttributeFactory.class).toInstance((cd, bp, p) -> null);
- }
- }
-
- protected static class SimpleAttributeModule extends AbstractModule {
- @Override
- public void configure() {
- DynamicSet.bind(binder(), ChangeAttributeFactory.class)
- .toInstance((cd, bp, p) -> new MyInfo("change " + cd.getId()));
- }
- }
-
protected static class PluginDefinedSimpleAttributeModule extends AbstractModule {
@Override
public void configure() {
@@ -170,21 +153,6 @@
private String opt;
}
- protected static class OptionAttributeModule extends AbstractModule {
- @Override
- public void configure() {
- DynamicSet.bind(binder(), ChangeAttributeFactory.class)
- .toInstance(
- (cd, bp, p) -> {
- MyOptions opts = (MyOptions) bp.getDynamicBean(p);
- return opts != null ? new MyInfo("opt " + opts.opt) : null;
- });
- bind(DynamicBean.class).annotatedWith(Exports.named(Query.class)).to(MyOptions.class);
- bind(DynamicBean.class).annotatedWith(Exports.named(QueryChanges.class)).to(MyOptions.class);
- bind(DynamicBean.class).annotatedWith(Exports.named(GetChange.class)).to(MyOptions.class);
- }
- }
-
public static class BulkAttributeFactoryWithOption implements ChangePluginDefinedInfoFactory {
protected MyOptions opts;
@@ -211,33 +179,6 @@
}
}
- protected void getChangeWithNullAttribute(PluginInfoGetter getter) throws Exception {
- Change.Id id = createChange().getChange().getId();
- assertThat(getter.call(id)).isNull();
-
- try (AutoCloseable ignored = installPlugin("my-plugin", NullAttributeModule.class)) {
- assertThat(getter.call(id)).isNull();
- }
-
- assertThat(getter.call(id)).isNull();
- }
-
- protected void getChangeWithSimpleAttribute(PluginInfoGetter getter) throws Exception {
- getChangeWithSimpleAttribute(getter, SimpleAttributeModule.class);
- }
-
- protected void getChangeWithSimpleAttribute(
- PluginInfoGetter getter, Class<? extends Module> moduleClass) throws Exception {
- Change.Id id = createChange().getChange().getId();
- assertThat(getter.call(id)).isNull();
-
- try (AutoCloseable ignored = installPlugin("my-plugin", moduleClass)) {
- assertThat(getter.call(id)).containsExactly(new MyInfo("my-plugin", "change " + id));
- }
-
- assertThat(getter.call(id)).isNull();
- }
-
protected void getSingleChangeWithPluginDefinedBulkAttribute(BulkPluginInfoGetterWithId getter)
throws Exception {
Change.Id id = createChange().getChange().getId();
@@ -298,30 +239,6 @@
assertThat(pluginInfos.get(changeWithInfo)).isNull();
}
- protected void getMultipleChangesWithPluginDefinedBulkAndChangeAttributes(
- BulkPluginInfoGetter getter) throws Exception {
- Change.Id id1 = createChange().getChange().getId();
- Change.Id id2 = createChange().getChange().getId();
-
- Map<Change.Id, List<PluginDefinedInfo>> pluginInfos = getter.call();
- assertThat(pluginInfos.get(id1)).isNull();
- assertThat(pluginInfos.get(id2)).isNull();
-
- try (AutoCloseable ignored =
- installPlugin("my-plugin-1", PluginDefinedSimpleAttributeModule.class);
- AutoCloseable ignored1 = installPlugin("my-plugin-2", SimpleAttributeModule.class)) {
- pluginInfos = getter.call();
- assertThat(pluginInfos.get(id1)).contains(new MyInfo("my-plugin-1", "change " + id1));
- assertThat(pluginInfos.get(id1)).contains(new MyInfo("my-plugin-2", "change " + id1));
- assertThat(pluginInfos.get(id2)).contains(new MyInfo("my-plugin-1", "change " + id2));
- assertThat(pluginInfos.get(id2)).contains(new MyInfo("my-plugin-2", "change " + id2));
- }
-
- pluginInfos = getter.call();
- assertThat(pluginInfos.get(id1)).isNull();
- assertThat(pluginInfos.get(id2)).isNull();
- }
-
protected void getMultipleChangesWithPluginDefinedBulkAttributeInSingleCall(
BulkPluginInfoGetter getter) throws Exception {
Change.Id id1 = createChange().getChange().getId();
@@ -345,22 +262,6 @@
assertThat(pluginInfos.get(id2)).isNull();
}
- protected void getChangeWithOption(
- PluginInfoGetter getterWithoutOptions, PluginInfoGetterWithOptions getterWithOptions)
- throws Exception {
- Change.Id id = createChange().getChange().getId();
- assertThat(getterWithoutOptions.call(id)).isNull();
-
- try (AutoCloseable ignored = installPlugin("my-plugin", OptionAttributeModule.class)) {
- assertThat(getterWithoutOptions.call(id))
- .containsExactly(new MyInfo("my-plugin", "opt null"));
- assertThat(getterWithOptions.call(id, ImmutableListMultimap.of("my-plugin--opt", "foo")))
- .containsExactly(new MyInfo("my-plugin", "opt foo"));
- }
-
- assertThat(getterWithoutOptions.call(id)).isNull();
- }
-
protected void getChangeWithPluginDefinedBulkAttributeOption(
BulkPluginInfoGetterWithId getterWithoutOptions,
BulkPluginInfoGetterWithIdAndOptions getterWithOptions)
@@ -455,11 +356,6 @@
}
@FunctionalInterface
- protected interface PluginInfoGetter {
- List<PluginDefinedInfo> call(Change.Id id) throws Exception;
- }
-
- @FunctionalInterface
protected interface BulkPluginInfoGetter {
Map<Change.Id, List<PluginDefinedInfo>> call() throws Exception;
}
@@ -474,10 +370,4 @@
Map<Change.Id, List<PluginDefinedInfo>> call(
Change.Id id, ImmutableListMultimap<String, String> pluginOptions) throws Exception;
}
-
- @FunctionalInterface
- protected interface PluginInfoGetterWithOptions {
- List<PluginDefinedInfo> call(Change.Id id, ImmutableListMultimap<String, String> pluginOptions)
- throws Exception;
- }
}
diff --git a/java/com/google/gerrit/entities/Permission.java b/java/com/google/gerrit/entities/Permission.java
index 3f04fa5..322c79e 100644
--- a/java/com/google/gerrit/entities/Permission.java
+++ b/java/com/google/gerrit/entities/Permission.java
@@ -140,6 +140,7 @@
return true;
}
+ /** The permission name, eg. {@code Permission.SUBMIT} */
public abstract String getName();
protected abstract boolean isExclusiveGroup();
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index e9c0136..894757b 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -39,7 +39,6 @@
import com.google.gerrit.server.account.externalids.ExternalIdModule;
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.cache.mem.DefaultMemoryCacheModule;
-import com.google.gerrit.server.change.ChangeAttributeFactory;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeKindCacheImpl;
import com.google.gerrit.server.change.MergeabilityCacheImpl;
@@ -116,8 +115,6 @@
bind(new TypeLiteral<List<CommentLinkInfo>>() {})
.toProvider(CommentLinkProvider.class)
.in(SINGLETON);
- bind(new TypeLiteral<DynamicSet<ChangeAttributeFactory>>() {})
- .toInstance(DynamicSet.emptySet());
bind(new TypeLiteral<DynamicMap<RestView<CommitResource>>>() {})
.toInstance(DynamicMap.emptyMap());
bind(String.class)
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index f861ea7..8c957ec 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -492,7 +492,7 @@
}
/**
- * Resolves all accounts matching the input string.
+ * Resolves all accounts matching the input string, visible to the current user.
*
* <p>The following input formats are recognized:
*
diff --git a/java/com/google/gerrit/server/account/VersionedAuthorizedKeys.java b/java/com/google/gerrit/server/account/VersionedAuthorizedKeys.java
index 235537c..30021e6 100644
--- a/java/com/google/gerrit/server/account/VersionedAuthorizedKeys.java
+++ b/java/com/google/gerrit/server/account/VersionedAuthorizedKeys.java
@@ -61,6 +61,8 @@
* <p>Other comment lines are ignored on read, and are not written back when the file is modified.
*/
public class VersionedAuthorizedKeys extends VersionedMetaData {
+
+ /** Read/write SSH keys by user ID. */
@Singleton
public static class Accessor {
private final GitRepositoryManager repoManager;
diff --git a/java/com/google/gerrit/server/change/ChangeAttributeFactory.java b/java/com/google/gerrit/server/change/ChangeAttributeFactory.java
deleted file mode 100644
index 663d7aa..0000000
--- a/java/com/google/gerrit/server/change/ChangeAttributeFactory.java
+++ /dev/null
@@ -1,49 +0,0 @@
-// Copyright (C) 2019 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.change;
-
-import com.google.gerrit.extensions.common.PluginDefinedInfo;
-import com.google.gerrit.server.DynamicOptions.BeanProvider;
-import com.google.gerrit.server.query.change.ChangeData;
-
-/**
- * Interface for plugins to provide additional fields in {@link
- * com.google.gerrit.extensions.common.ChangeInfo ChangeInfo}.
- *
- * <p>Register a {@code ChangeAttributeFactory} in a plugin {@code Module} like this:
- *
- * <pre>
- * DynamicSet.bind(binder(), ChangeAttributeFactory.class).to(YourClass.class);
- * </pre>
- *
- * <p>See the <a
- * href="https://gerrit-review.googlesource.com/Documentation/dev-plugins.html#query_attributes">plugin
- * developer documentation for more details and examples.
- */
-@Deprecated
-public interface ChangeAttributeFactory {
-
- /**
- * Create a plugin-provided info field.
- *
- * <p>Typically, implementations will subclass {@code PluginDefinedInfo} to add additional fields.
- *
- * @param cd change.
- * @param beanProvider provider of {@code DynamicBean}s, which may be used for reading options.
- * @param plugin plugin name.
- * @return the plugin's special change info.
- */
- PluginDefinedInfo create(ChangeData cd, BeanProvider beanProvider, String plugin);
-}
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 6ab0c61..02da518 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -158,17 +158,12 @@
}
public ChangeJson create(Iterable<ListChangesOption> options) {
- return factory.create(options, Optional.empty(), Optional.empty());
+ return factory.create(options, Optional.empty());
}
public ChangeJson create(
- Iterable<ListChangesOption> options,
- PluginDefinedAttributesFactory pluginDefinedAttributesFactory,
- PluginDefinedInfosFactory pluginDefinedInfosFactory) {
- return factory.create(
- options,
- Optional.of(pluginDefinedAttributesFactory),
- Optional.of(pluginDefinedInfosFactory));
+ Iterable<ListChangesOption> options, PluginDefinedInfosFactory pluginDefinedInfosFactory) {
+ return factory.create(options, Optional.of(pluginDefinedInfosFactory));
}
public ChangeJson create(ListChangesOption first, ListChangesOption... rest) {
@@ -179,7 +174,6 @@
public interface AssistedFactory {
ChangeJson create(
Iterable<ListChangesOption> options,
- Optional<PluginDefinedAttributesFactory> pluginDefinedAttributesFactory,
Optional<PluginDefinedInfosFactory> pluginDefinedInfosFactory);
}
@@ -226,7 +220,6 @@
private final TrackingFooters trackingFooters;
private final Metrics metrics;
private final RevisionJson revisionJson;
- private final Optional<PluginDefinedAttributesFactory> pluginDefinedAttributesFactory;
private final Optional<PluginDefinedInfosFactory> pluginDefinedInfosFactory;
private final boolean includeMergeable;
private final boolean lazyLoad;
@@ -251,7 +244,6 @@
RevisionJson.Factory revisionJsonFactory,
@GerritServerConfig Config cfg,
@Assisted Iterable<ListChangesOption> options,
- @Assisted Optional<PluginDefinedAttributesFactory> pluginDefinedAttributesFactory,
@Assisted Optional<PluginDefinedInfosFactory> pluginDefinedInfosFactory) {
this.userProvider = user;
this.changeDataFactory = cdf;
@@ -269,7 +261,6 @@
this.options = Sets.immutableEnumSet(options);
this.includeMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInApi();
this.lazyLoad = containsAnyOf(this.options, REQUIRE_LAZY_LOAD);
- this.pluginDefinedAttributesFactory = pluginDefinedAttributesFactory;
this.pluginDefinedInfosFactory = pluginDefinedInfosFactory;
logger.atFine().log("options = %s", options);
@@ -611,17 +602,9 @@
}
setSubmitter(cd, out);
- if (pluginDefinedAttributesFactory.isPresent()) {
- out.plugins = pluginDefinedAttributesFactory.get().create(cd);
- }
if (!pluginInfos.isEmpty()) {
- if (out.plugins == null) {
- out.plugins = pluginInfos;
- } else {
- out.plugins = new ArrayList<>(out.plugins);
- out.plugins.addAll(pluginInfos);
- }
+ out.plugins = pluginInfos;
}
out.revertOf = cd.change().getRevertOf() != null ? cd.change().getRevertOf().get() : null;
out.submissionId = cd.change().getSubmissionId();
diff --git a/java/com/google/gerrit/server/change/PluginDefinedAttributesFactories.java b/java/com/google/gerrit/server/change/PluginDefinedAttributesFactories.java
index b474dab..db21f11 100644
--- a/java/com/google/gerrit/server/change/PluginDefinedAttributesFactories.java
+++ b/java/com/google/gerrit/server/change/PluginDefinedAttributesFactories.java
@@ -14,55 +14,22 @@
package com.google.gerrit.server.change;
-import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.util.concurrent.TimeUnit.MINUTES;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.common.PluginDefinedInfo;
import com.google.gerrit.extensions.registration.Extension;
import com.google.gerrit.server.DynamicOptions.BeanProvider;
import com.google.gerrit.server.query.change.ChangeData;
import java.util.Collection;
-import java.util.Objects;
import java.util.stream.Stream;
-/** Static helpers for use by {@link PluginDefinedAttributesFactory} implementations. */
+/** Static helpers for use by {@link PluginDefinedInfosFactory} implementations. */
public class PluginDefinedAttributesFactories {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- @Nullable
- public static ImmutableList<PluginDefinedInfo> createAll(
- ChangeData cd,
- BeanProvider beanProvider,
- Stream<Extension<ChangeAttributeFactory>> attrFactories) {
- ImmutableList<PluginDefinedInfo> result =
- attrFactories
- .map(e -> tryCreate(cd, beanProvider, e.getPluginName(), e.get()))
- .filter(Objects::nonNull)
- .collect(toImmutableList());
- return !result.isEmpty() ? result : null;
- }
-
- @Nullable
- private static PluginDefinedInfo tryCreate(
- ChangeData cd, BeanProvider beanProvider, String plugin, ChangeAttributeFactory attrFactory) {
- PluginDefinedInfo pdi = null;
- try {
- pdi = attrFactory.create(cd, beanProvider, plugin);
- } catch (RuntimeException ex) {
- logger.atWarning().atMostEvery(1, MINUTES).withCause(ex).log(
- "error populating attribute on change %s from plugin %s", cd.getId(), plugin);
- }
- if (pdi != null) {
- pdi.name = plugin;
- }
- return pdi;
- }
-
public static ImmutableListMultimap<Change.Id, PluginDefinedInfo> createAll(
Collection<ChangeData> cds,
BeanProvider beanProvider,
diff --git a/java/com/google/gerrit/server/change/PluginDefinedAttributesFactory.java b/java/com/google/gerrit/server/change/PluginDefinedAttributesFactory.java
deleted file mode 100644
index 08d6ce7..0000000
--- a/java/com/google/gerrit/server/change/PluginDefinedAttributesFactory.java
+++ /dev/null
@@ -1,23 +0,0 @@
-// Copyright (C) 2019 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.change;
-
-import com.google.gerrit.extensions.common.PluginDefinedInfo;
-import com.google.gerrit.server.query.change.ChangeData;
-import java.util.List;
-
-public interface PluginDefinedAttributesFactory {
- List<PluginDefinedInfo> create(ChangeData cd);
-}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index ee37d17..9308662 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -106,7 +106,6 @@
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.change.AbandonOp;
import com.google.gerrit.server.change.AccountPatchReviewStore;
-import com.google.gerrit.server.change.ChangeAttributeFactory;
import com.google.gerrit.server.change.ChangeETagComputation;
import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.change.ChangeJson;
@@ -441,7 +440,6 @@
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeOperatorFactory.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeHasOperandFactory.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeIsOperandFactory.class);
- DynamicSet.setOf(binder(), ChangeAttributeFactory.class);
DynamicSet.setOf(binder(), ChangePluginDefinedInfoFactory.class);
install(new GitwebConfig.LegacyModule(cfg));
diff --git a/java/com/google/gerrit/server/permissions/GlobalPermission.java b/java/com/google/gerrit/server/permissions/GlobalPermission.java
index 07c9e84..d4f22e6 100644
--- a/java/com/google/gerrit/server/permissions/GlobalPermission.java
+++ b/java/com/google/gerrit/server/permissions/GlobalPermission.java
@@ -18,6 +18,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.extensions.annotations.CapabilityScope;
import com.google.gerrit.extensions.annotations.RequiresAnyCapability;
import com.google.gerrit.extensions.annotations.RequiresCapability;
@@ -31,7 +32,12 @@
import java.util.Optional;
import java.util.Set;
-/** Global server permissions built into Gerrit. */
+/**
+ * Global server permissions built into Gerrit.
+ *
+ * <p>See also {@link GlobalCapability} which lists the equivalent strings used in the
+ * refs/meta/config settings in All-Projects.
+ */
public enum GlobalPermission implements GlobalOrPluginPermission {
ACCESS_DATABASE,
ADMINISTRATE_SERVER,
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 89038e2..a1c48bc 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -385,6 +385,7 @@
this.accountsSection = accountsSection;
}
+ /** Returns an access section, {@code name} typically is a ref pattern. */
public AccessSection getAccessSection(String name) {
return accessSections.get(name);
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
index 370bc75..ed1f2f1 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
@@ -17,7 +17,6 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.query.change.ChangeQueryBuilder.FIELD_LIMIT;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.common.PluginDefinedInfo;
@@ -33,10 +32,8 @@
import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.DynamicOptions.DynamicBean;
import com.google.gerrit.server.account.AccountLimits;
-import com.google.gerrit.server.change.ChangeAttributeFactory;
import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory;
import com.google.gerrit.server.change.PluginDefinedAttributesFactories;
-import com.google.gerrit.server.change.PluginDefinedAttributesFactory;
import com.google.gerrit.server.change.PluginDefinedInfosFactory;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.index.change.ChangeIndexRewriter;
@@ -60,7 +57,6 @@
public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
implements DynamicOptions.BeanReceiver, DynamicOptions.BeanProvider, PluginDefinedInfosFactory {
private final Provider<CurrentUser> userProvider;
- private final ImmutableListMultimap<String, ChangeAttributeFactory> attributeFactoriesByPlugin;
private final ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory;
private final Map<String, DynamicBean> dynamicBeans = new HashMap<>();
private final List<Extension<ChangePluginDefinedInfoFactory>>
@@ -81,7 +77,6 @@
IndexConfig indexConfig,
ChangeIndexCollection indexes,
ChangeIndexRewriter rewriter,
- DynamicSet<ChangeAttributeFactory> attributeFactories,
ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory,
DynamicSet<ChangePluginDefinedInfoFactory> changePluginDefinedInfoFactories) {
super(
@@ -95,14 +90,6 @@
this.userProvider = userProvider;
this.changeIsVisibleToPredicateFactory = changeIsVisibleToPredicateFactory;
- ImmutableListMultimap.Builder<String, ChangeAttributeFactory> factoriesBuilder =
- ImmutableListMultimap.builder();
- ImmutableListMultimap.Builder<String, ChangePluginDefinedInfoFactory> infosFactoriesBuilder =
- ImmutableListMultimap.builder();
- // Eagerly call Extension#get() rather than storing Extensions, since that method invokes the
- // Provider on every call, which could be expensive if we invoke it once for every change.
- attributeFactories.entries().forEach(e -> factoriesBuilder.put(e.getPluginName(), e.get()));
- attributeFactoriesByPlugin = factoriesBuilder.build();
changePluginDefinedInfoFactories
.entries()
.forEach(e -> changePluginDefinedInfoFactoriesByPlugin.add(e));
@@ -130,18 +117,6 @@
return dynamicBeans.get(plugin);
}
- public PluginDefinedAttributesFactory getAttributesFactory() {
- return this::buildPluginInfo;
- }
-
- private ImmutableList<PluginDefinedInfo> buildPluginInfo(ChangeData cd) {
- return PluginDefinedAttributesFactories.createAll(
- cd,
- this,
- attributeFactoriesByPlugin.entries().stream()
- .map(e -> new Extension<>(e.getKey(), e::getValue)));
- }
-
public PluginDefinedInfosFactory getInfosFactory() {
return this::createPluginDefinedInfos;
}
diff --git a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
index b931457..4922b57 100644
--- a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
+++ b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
@@ -330,15 +330,9 @@
eventFactory.addDependencies(rw, c, d.change(), d.currentPatchSet());
}
- c.plugins = queryProcessor.getAttributesFactory().create(d);
List<PluginDefinedInfo> pluginInfos = pluginInfosByChange.get(d.getId());
if (!pluginInfos.isEmpty()) {
- if (c.plugins == null) {
- c.plugins = pluginInfos;
- } else {
- c.plugins = new ArrayList<>(c.plugins);
- c.plugins.addAll(pluginInfos);
- }
+ c.plugins = pluginInfos;
}
return c;
}
diff --git a/java/com/google/gerrit/server/restapi/account/AccountsCollection.java b/java/com/google/gerrit/server/restapi/account/AccountsCollection.java
index 119e2e4..61ff6b8 100644
--- a/java/com/google/gerrit/server/restapi/account/AccountsCollection.java
+++ b/java/com/google/gerrit/server/restapi/account/AccountsCollection.java
@@ -53,6 +53,7 @@
return new AccountResource(accountResolver.resolve(id.get()).asUniqueUser());
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
+ // Must be authenticated to use 'me' or 'self'.
throw new AuthException(e.getMessage(), e);
}
throw new ResourceNotFoundException(e.getMessage(), e);
diff --git a/java/com/google/gerrit/server/restapi/account/GetSshKeys.java b/java/com/google/gerrit/server/restapi/account/GetSshKeys.java
index 6df6c3c..9952987 100644
--- a/java/com/google/gerrit/server/restapi/account/GetSshKeys.java
+++ b/java/com/google/gerrit/server/restapi/account/GetSshKeys.java
@@ -60,8 +60,7 @@
@Override
public Response<List<SshKeyInfo>> apply(AccountResource rsrc)
- throws AuthException, RepositoryNotFoundException, IOException, ConfigInvalidException,
- PermissionBackendException {
+ throws AuthException, IOException, ConfigInvalidException, PermissionBackendException {
if (!self.get().hasSameAccountId(rsrc.getUser())) {
permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
}
diff --git a/java/com/google/gerrit/server/restapi/account/PutActive.java b/java/com/google/gerrit/server/restapi/account/PutActive.java
index a80ab3f..3b431db 100644
--- a/java/com/google/gerrit/server/restapi/account/PutActive.java
+++ b/java/com/google/gerrit/server/restapi/account/PutActive.java
@@ -32,7 +32,7 @@
*
* <p>This REST endpoint handles {@code PUT /accounts/<account-identifier>/active} requests.
*
- * <p>Only active accounts can login into Gerrit.
+ * <p>Only active accounts can login into Gerrit, or are suggested as reviewers.
*
* <p>Marking an account as inactive is handled by {@link DeleteActive}.
*/
diff --git a/java/com/google/gerrit/server/restapi/account/Stars.java b/java/com/google/gerrit/server/restapi/account/Stars.java
index c27bdd8..cc362f2 100644
--- a/java/com/google/gerrit/server/restapi/account/Stars.java
+++ b/java/com/google/gerrit/server/restapi/account/Stars.java
@@ -46,6 +46,12 @@
import java.util.Set;
import java.util.SortedSet;
+/**
+ * Implements adding label stars to changes.
+ *
+ * <p>This handles {@code POST} and {@code GET} for {@code
+ * /accounts/<account-identifier>/stars.changes/<change ID>}.
+ */
@Singleton
public class Stars implements ChildCollection<AccountResource, AccountResource.Star> {
@@ -70,6 +76,7 @@
public Star parse(AccountResource parent, IdString id)
throws RestApiException, PermissionBackendException, IOException {
IdentifiedUser user = parent.getUser();
+ // This enforces visibility of the change.
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
Set<String> labels = starredChangesUtil.getLabels(user.getAccountId(), change.getId());
return new AccountResource.Star(user, change, labels);
@@ -87,6 +94,7 @@
@Singleton
public static class ListStarredChanges implements RestReadView<AccountResource> {
+
private final Provider<CurrentUser> self;
private final ChangesCollection changes;
@@ -121,6 +129,7 @@
@Singleton
public static class Get implements RestReadView<AccountResource.Star> {
+
private final Provider<CurrentUser> self;
private final StarredChangesUtil starredChangesUtil;
@@ -142,6 +151,7 @@
@Singleton
public static class Post implements RestModifyView<AccountResource.Star, StarsInput> {
+
private final Provider<CurrentUser> self;
private final StarredChangesUtil starredChangesUtil;
diff --git a/java/com/google/gerrit/server/restapi/change/GetChange.java b/java/com/google/gerrit/server/restapi/change/GetChange.java
index 1ef3c4b..f28d2b3 100644
--- a/java/com/google/gerrit/server/restapi/change/GetChange.java
+++ b/java/com/google/gerrit/server/restapi/change/GetChange.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.restapi.change;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Streams;
import com.google.gerrit.entities.Change;
@@ -27,7 +26,6 @@
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.DynamicOptions.DynamicBean;
-import com.google.gerrit.server.change.ChangeAttributeFactory;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory;
import com.google.gerrit.server.change.ChangeResource;
@@ -46,7 +44,6 @@
DynamicOptions.BeanReceiver,
DynamicOptions.BeanProvider {
private final ChangeJson.Factory json;
- private final DynamicSet<ChangeAttributeFactory> attrFactories;
private final DynamicSet<ChangePluginDefinedInfoFactory> pdiFactories;
private final EnumSet<ListChangesOption> options = EnumSet.noneOf(ListChangesOption.class);
private final Map<String, DynamicBean> dynamicBeans = new HashMap<>();
@@ -62,12 +59,8 @@
}
@Inject
- GetChange(
- ChangeJson.Factory json,
- DynamicSet<ChangeAttributeFactory> attrFactories,
- DynamicSet<ChangePluginDefinedInfoFactory> pdiFactories) {
+ GetChange(ChangeJson.Factory json, DynamicSet<ChangePluginDefinedInfoFactory> pdiFactories) {
this.json = json;
- this.attrFactories = attrFactories;
this.pdiFactories = pdiFactories;
}
@@ -91,12 +84,7 @@
}
private ChangeJson newChangeJson() {
- return json.create(options, this::buildPluginInfo, this::createPluginDefinedInfos);
- }
-
- private ImmutableList<PluginDefinedInfo> buildPluginInfo(ChangeData cd) {
- return PluginDefinedAttributesFactories.createAll(
- cd, this, Streams.stream(attrFactories.entries()));
+ return json.create(options, this::createPluginDefinedInfos);
}
private ImmutableListMultimap<Change.Id, PluginDefinedInfo> createPluginDefinedInfos(
diff --git a/java/com/google/gerrit/server/restapi/change/QueryChanges.java b/java/com/google/gerrit/server/restapi/change/QueryChanges.java
index 3c8157b..cf0d4cf 100644
--- a/java/com/google/gerrit/server/restapi/change/QueryChanges.java
+++ b/java/com/google/gerrit/server/restapi/change/QueryChanges.java
@@ -187,9 +187,7 @@
int cnt = queries.size();
List<QueryResult<ChangeData>> results = queryProcessor.query(qb.parse(queries));
List<List<ChangeInfo>> res =
- json.create(
- options, queryProcessor.getAttributesFactory(), queryProcessor.getInfosFactory())
- .format(results);
+ json.create(options, queryProcessor.getInfosFactory()).format(results);
for (int n = 0; n < cnt; n++) {
List<ChangeInfo> info = res.get(n);
if (results.get(n).more() && !info.isEmpty()) {
diff --git a/java/com/google/gerrit/server/restapi/project/CheckAccess.java b/java/com/google/gerrit/server/restapi/project/CheckAccess.java
index 4ef724a..37616cd 100644
--- a/java/com/google/gerrit/server/restapi/project/CheckAccess.java
+++ b/java/com/google/gerrit/server/restapi/project/CheckAccess.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2017 The Android Open Source Project
+// Copyright (C) 2018 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
@@ -17,6 +17,7 @@
import static com.google.gerrit.entities.RefNames.REFS_HEADS;
import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.extensions.api.config.AccessCheckInfo;
@@ -25,7 +26,7 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.logging.TraceContext;
@@ -37,15 +38,14 @@
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ProjectResource;
import com.google.inject.Inject;
-import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Optional;
import javax.servlet.http.HttpServletResponse;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Repository;
+import org.kohsuke.args4j.Option;
-@Singleton
-public class CheckAccess implements RestModifyView<ProjectResource, AccessCheckInput> {
+public class CheckAccess implements RestReadView<ProjectResource> {
private final AccountResolver accountResolver;
private final PermissionBackend permissionBackend;
private final GitRepositoryManager gitRepositoryManager;
@@ -60,7 +60,15 @@
this.gitRepositoryManager = gitRepositoryManager;
}
- @Override
+ @Option(name = "--ref", usage = "ref name to check permission for")
+ String refName;
+
+ @Option(name = "--account", usage = "account to check acccess for")
+ String account;
+
+ @Option(name = "--perm", usage = "permission to check; default: read of any ref.")
+ String permission;
+
public Response<AccessCheckInfo> apply(ProjectResource rsrc, AccessCheckInput input)
throws PermissionBackendException, RestApiException, IOException, ConfigInvalidException {
permissionBackend.user(rsrc.getUser()).check(GlobalPermission.VIEW_ACCESS);
@@ -142,6 +150,22 @@
info.status = statusCode;
info.message = message;
info.debugLogs = traceContext.getAclLogRecords();
+ if (info.debugLogs.isEmpty()) {
+ info.debugLogs =
+ ImmutableList.of("Found no rules that apply, so defaulting to no permission");
+ }
return info;
}
+
+ @Override
+ public Response<AccessCheckInfo> apply(ProjectResource rsrc)
+ throws PermissionBackendException, RestApiException, IOException, ConfigInvalidException {
+
+ AccessCheckInput input = new AccessCheckInput();
+ input.ref = refName;
+ input.account = account;
+ input.permission = permission;
+
+ return apply(rsrc, input);
+ }
}
diff --git a/java/com/google/gerrit/server/restapi/project/CheckAccessReadView.java b/java/com/google/gerrit/server/restapi/project/CheckAccessReadView.java
deleted file mode 100644
index 6aaa678..0000000
--- a/java/com/google/gerrit/server/restapi/project/CheckAccessReadView.java
+++ /dev/null
@@ -1,62 +0,0 @@
-// Copyright (C) 2018 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.restapi.project;
-
-import com.google.gerrit.extensions.api.config.AccessCheckInfo;
-import com.google.gerrit.extensions.api.config.AccessCheckInput;
-import com.google.gerrit.extensions.restapi.Response;
-import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.ProjectResource;
-import com.google.inject.Inject;
-import java.io.IOException;
-import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.kohsuke.args4j.Option;
-
-public class CheckAccessReadView implements RestReadView<ProjectResource> {
- String refName;
- String account;
- String permission;
-
- @Inject CheckAccess checkAccess;
-
- @Option(name = "--ref", usage = "ref name to check permission for")
- void addOption(String refName) {
- this.refName = refName;
- }
-
- @Option(name = "--account", usage = "account to check acccess for")
- void setAccount(String account) {
- this.account = account;
- }
-
- @Option(name = "--perm", usage = "permission to check; default: read of any ref.")
- void setPermission(String perm) {
- this.permission = perm;
- }
-
- @Override
- public Response<AccessCheckInfo> apply(ProjectResource rsrc)
- throws PermissionBackendException, RestApiException, IOException, ConfigInvalidException {
-
- AccessCheckInput input = new AccessCheckInput();
- input.ref = refName;
- input.account = account;
- input.permission = permission;
-
- return checkAccess.apply(rsrc, input);
- }
-}
diff --git a/java/com/google/gerrit/server/restapi/project/ListProjects.java b/java/com/google/gerrit/server/restapi/project/ListProjects.java
index 5418876..8a2ab0c 100644
--- a/java/com/google/gerrit/server/restapi/project/ListProjects.java
+++ b/java/com/google/gerrit/server/restapi/project/ListProjects.java
@@ -90,7 +90,11 @@
import org.eclipse.jgit.lib.Repository;
import org.kohsuke.args4j.Option;
-/** List projects visible to the calling user. */
+/**
+ * List projects visible to the calling user.
+ *
+ * <p>This is like {@link QueryProjects} but with a slightly different calling convention.
+ */
public class ListProjects implements RestReadView<TopLevelResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
diff --git a/java/com/google/gerrit/server/restapi/project/Module.java b/java/com/google/gerrit/server/restapi/project/Module.java
index ee3914d..70fb6ea 100644
--- a/java/com/google/gerrit/server/restapi/project/Module.java
+++ b/java/com/google/gerrit/server/restapi/project/Module.java
@@ -57,7 +57,7 @@
get(PROJECT_KIND, "access").to(GetAccess.class);
post(PROJECT_KIND, "access").to(SetAccess.class);
put(PROJECT_KIND, "access:review").to(CreateAccessChange.class);
- get(PROJECT_KIND, "check.access").to(CheckAccessReadView.class);
+ get(PROJECT_KIND, "check.access").to(CheckAccess.class);
post(PROJECT_KIND, "check").to(Check.class);
diff --git a/java/com/google/gerrit/server/restapi/project/QueryProjects.java b/java/com/google/gerrit/server/restapi/project/QueryProjects.java
index e4f7df5..a9d818d 100644
--- a/java/com/google/gerrit/server/restapi/project/QueryProjects.java
+++ b/java/com/google/gerrit/server/restapi/project/QueryProjects.java
@@ -36,6 +36,7 @@
import java.util.List;
import org.kohsuke.args4j.Option;
+/** Implements the {@code GET /projects/?query=QUERY} endpoint. */
public class QueryProjects implements RestReadView<TopLevelResource> {
private final ProjectIndexCollection indexes;
private final ProjectQueryBuilder queryBuilder;
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java b/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
index 31198d5..cebce0b 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
@@ -16,9 +16,6 @@
import com.google.gerrit.acceptance.AbstractPluginFieldsTest;
import com.google.gerrit.acceptance.NoHttpd;
-import com.google.gerrit.extensions.annotations.Exports;
-import com.google.gerrit.server.change.ChangeAttributeFactory;
-import com.google.inject.AbstractModule;
import java.util.Arrays;
import org.junit.Test;
@@ -27,30 +24,6 @@
// No tests for /detail via the extension API, since the extension API doesn't have that method.
@Test
- public void queryChangeWithNullAttribute() throws Exception {
- getChangeWithNullAttribute(
- id -> pluginInfoFromSingletonList(gApi.changes().query(id.toString()).get()));
- }
-
- @Test
- public void getChangeWithNullAttribute() throws Exception {
- getChangeWithNullAttribute(
- id -> pluginInfoFromChangeInfo(gApi.changes().id(id.toString()).get()));
- }
-
- @Test
- public void queryChangeWithSimpleAttribute() throws Exception {
- getChangeWithSimpleAttribute(
- id -> pluginInfoFromSingletonList(gApi.changes().query(id.toString()).get()));
- }
-
- @Test
- public void getChangeWithSimpleAttribute() throws Exception {
- getChangeWithSimpleAttribute(
- id -> pluginInfoFromChangeInfo(gApi.changes().id(id.toString()).get()));
- }
-
- @Test
public void querySingleChangeWithBulkAttribute() throws Exception {
getSingleChangeWithPluginDefinedBulkAttribute(
id -> pluginInfosFromChangeInfos(gApi.changes().query(id.toString()).get()));
@@ -63,22 +36,6 @@
}
@Test
- public void queryChangeWithOption() throws Exception {
- getChangeWithOption(
- id -> pluginInfoFromSingletonList(gApi.changes().query(id.toString()).get()),
- (id, opts) ->
- pluginInfoFromSingletonList(
- gApi.changes().query(id.toString()).withPluginOptions(opts).get()));
- }
-
- @Test
- public void getChangeWithOption() throws Exception {
- getChangeWithOption(
- id -> pluginInfoFromChangeInfo(gApi.changes().id(id.get()).get()),
- (id, opts) -> pluginInfoFromChangeInfo(gApi.changes().id(id.get()).get(opts)));
- }
-
- @Test
public void queryChangeWithOptionBulkAttribute() throws Exception {
getChangeWithPluginDefinedBulkAttributeOption(
id -> pluginInfosFromChangeInfos(gApi.changes().query(id.toString()).get()),
@@ -108,12 +65,6 @@
}
@Test
- public void getMultipleChangesWithPluginDefinedAndChangeAttributes() throws Exception {
- getMultipleChangesWithPluginDefinedBulkAndChangeAttributes(
- () -> pluginInfosFromChangeInfos(gApi.changes().query("status:open").get()));
- }
-
- @Test
public void getMultipleChangesWithPluginDefinedAttributeInSingleCall() throws Exception {
getMultipleChangesWithPluginDefinedBulkAttributeInSingleCall(
() -> pluginInfosFromChangeInfos(gApi.changes().query("status:open").get()));
@@ -124,23 +75,4 @@
getChangeWithPluginDefinedBulkAttributeWithException(
id -> pluginInfosFromChangeInfos(Arrays.asList(gApi.changes().id(id.get()).get())));
}
-
- static class SimpleAttributeWithExplicitExportModule extends AbstractModule {
- @Override
- public void configure() {
- bind(ChangeAttributeFactory.class)
- .annotatedWith(Exports.named("simple"))
- .toInstance((cd, bp, p) -> new MyInfo("change " + cd.getId()));
- }
- }
-
- @Test
- public void getChangeWithSimpleAttributeWithExplicitExport() throws Exception {
- // For backwards compatibility with old plugins, allow modules to bind into the
- // DynamicSet<ChangeAttributeFactory> as if it were a DynamicMap. We only need one variant of
- // this test to prove that the mapping works.
- getChangeWithSimpleAttribute(
- id -> pluginInfoFromChangeInfo(gApi.changes().id(id.toString()).get()),
- SimpleAttributeWithExplicitExportModule.class);
- }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java b/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
index 709facc..45a895a 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
@@ -19,14 +19,17 @@
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.deny;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.AccessSection;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
@@ -36,9 +39,11 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.inject.Inject;
import java.util.List;
+import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
@@ -48,6 +53,7 @@
public class CheckAccessIT extends AbstractDaemonTest {
@Inject private ProjectOperations projectOperations;
@Inject private GroupOperations groupOperations;
+ @Inject private AllProjectsName allProjectsName;
private Project.NameKey normalProject;
private Project.NameKey secretProject;
@@ -391,4 +397,34 @@
assertThat(info.status).isEqualTo(200);
assertThat(info.message).contains("no branches");
}
+
+ @Test
+ @Sandboxed
+ public void noRules() throws Exception {
+ normalProject = projectOperations.newProject().create();
+
+ for (AccessSection section :
+ projectOperations.project(allProjectsName).getProjectConfig().getAccessSections()) {
+ if (!section.getName().startsWith(Constants.R_REFS)) {
+ continue;
+ }
+ for (Permission permission : section.getPermissions()) {
+ projectOperations
+ .project(allProjectsName)
+ .forUpdate()
+ .remove(permissionKey(permission.getName()).ref(section.getName()).build())
+ .update();
+ }
+ }
+ AccessCheckInput input = new AccessCheckInput();
+ input.account = privilegedUser.email();
+ input.permission = Permission.READ;
+ input.ref = "refs/heads/main";
+
+ AccessCheckInfo info = gApi.projects().name(normalProject.get()).checkAccess(input);
+ assertThat(info.status).isEqualTo(403);
+
+ assertThat(info.debugLogs).isNotEmpty();
+ assertThat(info.debugLogs.get(0)).contains("Found no rules");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/PluginFieldsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/PluginFieldsIT.java
index 17bf37e..a4ec40e 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/PluginFieldsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/PluginFieldsIT.java
@@ -14,8 +14,6 @@
package com.google.gerrit.acceptance.rest.change;
-import static com.google.common.truth.Truth.assertThat;
-
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.acceptance.AbstractPluginFieldsTest;
@@ -35,41 +33,6 @@
private static final Gson GSON = OutputFormat.JSON.newGson();
@Test
- public void queryChangeWithNullAttribute() throws Exception {
- getChangeWithNullAttribute(
- id -> pluginInfoFromSingletonList(adminRestSession.get(changeQueryUrl(id))));
- }
-
- @Test
- public void getChangeWithNullAttribute() throws Exception {
- getChangeWithNullAttribute(id -> pluginInfoFromChangeInfo(adminRestSession.get(changeUrl(id))));
- }
-
- @Test
- public void getChangeDetailWithNullAttribute() throws Exception {
- getChangeWithNullAttribute(
- id -> pluginInfoFromChangeInfo(adminRestSession.get(changeDetailUrl(id))));
- }
-
- @Test
- public void queryChangeWithSimpleAttribute() throws Exception {
- getChangeWithSimpleAttribute(
- id -> pluginInfoFromSingletonList(adminRestSession.get(changeQueryUrl(id))));
- }
-
- @Test
- public void getChangeWithSimpleAttribute() throws Exception {
- getChangeWithSimpleAttribute(
- id -> pluginInfoFromChangeInfo(adminRestSession.get(changeUrl(id))));
- }
-
- @Test
- public void getChangeDetailWithSimpleAttribute() throws Exception {
- getChangeWithSimpleAttribute(
- id -> pluginInfoFromChangeInfo(adminRestSession.get(changeDetailUrl(id))));
- }
-
- @Test
public void querySingleChangeWithBulkAttribute() throws Exception {
getSingleChangeWithPluginDefinedBulkAttribute(
id -> pluginInfosFromChangeInfos(adminRestSession.get(changeQueryUrl(id))));
@@ -88,27 +51,6 @@
}
@Test
- public void queryChangeWithOption() throws Exception {
- getChangeWithOption(
- id -> pluginInfoFromSingletonList(adminRestSession.get(changeQueryUrl(id))),
- (id, opts) -> pluginInfoFromSingletonList(adminRestSession.get(changeQueryUrl(id, opts))));
- }
-
- @Test
- public void getChangeWithOption() throws Exception {
- getChangeWithOption(
- id -> pluginInfoFromChangeInfo(adminRestSession.get(changeUrl(id))),
- (id, opts) -> pluginInfoFromChangeInfo(adminRestSession.get(changeUrl(id, opts))));
- }
-
- @Test
- public void getChangeDetailWithOption() throws Exception {
- getChangeWithOption(
- id -> pluginInfoFromChangeInfo(adminRestSession.get(changeDetailUrl(id))),
- (id, opts) -> pluginInfoFromChangeInfo(adminRestSession.get(changeDetailUrl(id, opts))));
- }
-
- @Test
public void pluginDefinedQueryChangeWithOption() throws Exception {
getChangeWithPluginDefinedBulkAttributeOption(
id -> pluginInfosFromChangeInfos(adminRestSession.get(changeQueryUrl(id))),
@@ -142,12 +84,6 @@
}
@Test
- public void getMultipleChangesWithPluginDefinedAndChangeAttributes() throws Exception {
- getMultipleChangesWithPluginDefinedBulkAndChangeAttributes(
- () -> pluginInfosFromChangeInfos(adminRestSession.get("/changes/?q=status:open")));
- }
-
- @Test
public void getMultipleChangesWithPluginDefinedAttributeInSingleCall() throws Exception {
getMultipleChangesWithPluginDefinedBulkAttributeInSingleCall(
() -> pluginInfosFromChangeInfos(adminRestSession.get("/changes/?q=status:open")));
@@ -204,24 +140,6 @@
}
@Nullable
- private static List<PluginDefinedInfo> pluginInfoFromSingletonList(RestResponse res)
- throws Exception {
- res.assertOK();
- List<Map<String, Object>> changeInfos =
- GSON.fromJson(res.getReader(), new TypeToken<List<Map<String, Object>>>() {}.getType());
- assertThat(changeInfos).hasSize(1);
- return decodeRawPluginsList(GSON, changeInfos.get(0).get("plugins"));
- }
-
- @Nullable
- private List<PluginDefinedInfo> pluginInfoFromChangeInfo(RestResponse res) throws Exception {
- res.assertOK();
- Map<String, Object> changeInfo =
- GSON.fromJson(res.getReader(), new TypeToken<Map<String, Object>>() {}.getType());
- return decodeRawPluginsList(GSON, changeInfo.get("plugins"));
- }
-
- @Nullable
private Map<Change.Id, List<PluginDefinedInfo>> pluginInfoMapFromChangeInfo(RestResponse res)
throws Exception {
res.assertOK();
diff --git a/javatests/com/google/gerrit/acceptance/ssh/PluginChangeFieldsIT.java b/javatests/com/google/gerrit/acceptance/ssh/PluginChangeFieldsIT.java
index 38293f9..009e05d 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/PluginChangeFieldsIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/PluginChangeFieldsIT.java
@@ -14,7 +14,6 @@
package com.google.gerrit.acceptance.ssh;
-import static com.google.common.truth.Truth.assertThat;
import static java.util.stream.Collectors.joining;
import com.google.common.collect.ImmutableListMultimap;
@@ -41,31 +40,12 @@
private static final Gson GSON = OutputStreamQuery.GSON;
@Test
- public void queryChangeWithNullAttribute() throws Exception {
- getChangeWithNullAttribute(
- id -> pluginInfoFromSingletonList(adminSshSession.exec(changeQueryCmd(id))));
- }
-
- @Test
- public void queryChangeWithSimpleAttribute() throws Exception {
- getChangeWithSimpleAttribute(
- id -> pluginInfoFromSingletonList(adminSshSession.exec(changeQueryCmd(id))));
- }
-
- @Test
public void querySingleChangeWithBulkAttribute() throws Exception {
getSingleChangeWithPluginDefinedBulkAttribute(
id -> pluginInfosFromList(adminSshSession.exec(changeQueryCmd(id))));
}
@Test
- public void queryChangeWithOption() throws Exception {
- getChangeWithOption(
- id -> pluginInfoFromSingletonList(adminSshSession.exec(changeQueryCmd(id))),
- (id, opts) -> pluginInfoFromSingletonList(adminSshSession.exec(changeQueryCmd(id, opts))));
- }
-
- @Test
public void queryPluginDefinedAttributeChangeWithOption() throws Exception {
getChangeWithPluginDefinedBulkAttributeOption(
id -> pluginInfosFromList(adminSshSession.exec(changeQueryCmd(id))),
@@ -85,12 +65,6 @@
}
@Test
- public void getMultipleChangesWithPluginDefinedAndChangeAttributes() throws Exception {
- getMultipleChangesWithPluginDefinedBulkAndChangeAttributes(
- () -> pluginInfosFromList(adminSshSession.exec("gerrit query --format json status:open")));
- }
-
- @Test
public void getMultipleChangesWithPluginDefinedAttributeInSingleCall() throws Exception {
getMultipleChangesWithPluginDefinedBulkAttributeInSingleCall(
() -> pluginInfosFromList(adminSshSession.exec("gerrit query --format json status:open")));
@@ -116,15 +90,6 @@
}
@Nullable
- private static List<PluginDefinedInfo> pluginInfoFromSingletonList(String sshOutput)
- throws Exception {
- List<Map<String, Object>> changeAttrs = getChangeAttrs(sshOutput);
-
- assertThat(changeAttrs).hasSize(1);
- return decodeRawPluginsList(GSON, changeAttrs.get(0).get("plugins"));
- }
-
- @Nullable
private static Map<Change.Id, List<PluginDefinedInfo>> pluginInfosFromList(String sshOutput)
throws Exception {
List<Map<String, Object>> changeAttrs = getChangeAttrs(sshOutput);
diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor
index 7357ab4..740c35a 160000
--- a/plugins/codemirror-editor
+++ b/plugins/codemirror-editor
@@ -1 +1 @@
-Subproject commit 7357ab473599d16ae33cc982bbd65472f08c2dd6
+Subproject commit 740c35ae36f44748b3c91e60ee7dcb2fb6e99549
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 febf18a..fdf2811 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
@@ -38,11 +38,7 @@
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {appContext} from '../../../services/app-context';
-import {
- fetchChangeUpdates,
- patchNumEquals,
- CURRENT,
-} from '../../../utils/patch-set-util';
+import {fetchChangeUpdates, CURRENT} from '../../../utils/patch-set-util';
import {
changeIsOpen,
isOwner,
@@ -1153,7 +1149,7 @@
_getRevision(change: ChangeViewChangeInfo, patchNum?: PatchSetNum) {
for (const rev of Object.values(change.revisions)) {
- if (patchNumEquals(rev._number, patchNum)) {
+ if (rev._number === patchNum) {
return rev;
}
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
index 0279b7e..f7bca82 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
@@ -258,7 +258,7 @@
rev2: revObj,
},
};
- assert.deepEqual(element._getRevision(change, '2'), revObj);
+ assert.deepEqual(element._getRevision(change, 2), revObj);
});
test('_actionComparator sort order', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 684891f..126d017 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -69,7 +69,6 @@
fetchChangeUpdates,
hasEditBasedOnCurrentPatchSet,
hasEditPatchsetLoaded,
- patchNumEquals,
PatchSet,
} from '../../../utils/patch-set-util';
import {changeStatuses, changeStatusString} from '../../../utils/change-util';
@@ -1656,7 +1655,7 @@
if (!this._change) throw new Error('missing required change property');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
- if (patchNumEquals(this._patchRange.basePatchNum, ParentPatchSetNum)) {
+ if (this._patchRange.basePatchNum === ParentPatchSetNum) {
fireAlert(this, 'Base is already selected.');
return;
}
@@ -1670,7 +1669,7 @@
if (!this._change) throw new Error('missing required change property');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
- if (patchNumEquals(this._patchRange.basePatchNum, ParentPatchSetNum)) {
+ if (this._patchRange.basePatchNum === ParentPatchSetNum) {
fireAlert(this, 'Left is already base.');
return;
}
@@ -1685,7 +1684,7 @@
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
- if (patchNumEquals(this._patchRange.patchNum, latestPatchNum)) {
+ if (this._patchRange.patchNum === latestPatchNum) {
fireAlert(this, 'Latest is already selected.');
return;
}
@@ -1704,7 +1703,7 @@
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
if (!this._patchRange)
throw new Error('missing required _patchRange property');
- if (patchNumEquals(this._patchRange.patchNum, latestPatchNum)) {
+ if (this._patchRange.patchNum === latestPatchNum) {
fireAlert(this, 'Right is already latest.');
return;
}
@@ -1724,8 +1723,8 @@
throw new Error('missing required _patchRange property');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
if (
- patchNumEquals(this._patchRange.patchNum, latestPatchNum) &&
- patchNumEquals(this._patchRange.basePatchNum, ParentPatchSetNum)
+ this._patchRange.patchNum === latestPatchNum &&
+ this._patchRange.basePatchNum === ParentPatchSetNum
) {
fireAlert(this, 'Already diffing base against latest.');
return;
@@ -1967,7 +1966,7 @@
if (
!this._patchRange ||
!this._patchRange.patchNum ||
- patchNumEquals(this._patchRange.patchNum, currentRevision._number)
+ this._patchRange.patchNum === currentRevision._number
) {
// CommitInfo.commit is optional, and may need patching.
if (currentRevision.commit && !currentRevision.commit.commit) {
@@ -2567,7 +2566,7 @@
}
const patchRange = patchRangeRecord.base || {};
- return patchNumEquals(patchRange.patchNum, EditPatchSetNum);
+ return patchRange.patchNum === EditPatchSetNum;
}
_handleFileActionTap(e: CustomEvent<{path: string; action: string}>) {
@@ -2651,10 +2650,7 @@
throw new Error('missing required _patchRange property');
let patchNum;
if (
- !patchNumEquals(
- this._patchRange.patchNum,
- computeLatestPatchNum(this._allPatchSets)
- )
+ !(this._patchRange.patchNum === computeLatestPatchNum(this._allPatchSets))
) {
patchNum = this._patchRange.patchNum;
}
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 1195ea6..c4526bb 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
@@ -246,8 +246,10 @@
_computeDisableCherryPick(
cherryPickType: CherryPickType,
duplicateProjectChanges: boolean,
- statuses: Statuses
+ statuses: Statuses,
+ branch?: BranchName
) {
+ if (!branch) return true;
const duplicateProject =
cherryPickType === CherryPickType.TOPIC && duplicateProjectChanges;
if (duplicateProject) return true;
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_html.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_html.ts
index 16262c6..4de395c 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_html.ts
@@ -85,7 +85,7 @@
<gr-dialog
confirm-label="Cherry Pick"
cancel-label="[[_computeCancelLabel(_statuses)]]"
- disabled$="[[_computeDisableCherryPick(_cherryPickType, _duplicateProjectChanges, _statuses)]]"
+ disabled$="[[_computeDisableCherryPick(_cherryPickType, _duplicateProjectChanges, _statuses, branch)]]"
on-confirm="_handleConfirmTap"
on-cancel="_handleCancelTap"
>
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.js b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.js
index ae34b70..5564fcf 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.js
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.js
@@ -174,6 +174,9 @@
test('submit button is blocked while cherry picks is running', done => {
const confirmButton = element.shadowRoot.querySelector('gr-dialog').$
.confirm;
+ assert.isTrue(confirmButton.hasAttribute('disabled'));
+ element.branch = 'b';
+ flush();
assert.isFalse(confirmButton.hasAttribute('disabled'));
element.updateStatus(changes[0], {status: 'RUNNING'});
flush(() => {
diff --git a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
index 27d9756..5949513 100644
--- a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
@@ -20,7 +20,6 @@
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-download-dialog_html';
-import {patchNumEquals} from '../../../utils/patch-set-util';
import {changeBaseURL} from '../../../utils/change-util';
import {customElement, property, computed, observe} from '@polymer/decorators';
import {ChangeInfo, ServerInfo, PatchSetNum} from '../../../types/common';
@@ -72,7 +71,7 @@
}
for (const rev of Object.values(this.change.revisions || {})) {
- if (patchNumEquals(rev._number, this.patchNum)) {
+ if (rev._number === this.patchNum) {
const fetch = rev.fetch;
if (fetch) {
return Object.keys(fetch).sort();
@@ -113,7 +112,7 @@
if (!change || !selectedScheme) return [];
for (const rev of Object.values(change.revisions || {})) {
if (
- patchNumEquals(rev._number, patchNum) &&
+ rev._number === patchNum &&
rev &&
rev.fetch &&
hasOwnProperty(rev.fetch, selectedScheme)
@@ -171,7 +170,7 @@
let shortRev = '';
for (const rev in change.revisions) {
- if (patchNumEquals(change.revisions[rev]._number, patchNum)) {
+ if (change.revisions[rev]._number === patchNum) {
shortRev = rev.substr(0, 7);
break;
}
@@ -185,7 +184,7 @@
return false;
}
for (const rev of Object.values(change.revisions || {})) {
- if (patchNumEquals(rev._number, patchNum)) {
+ if (rev._number === patchNum) {
const parentLength =
rev.commit && rev.commit.parents ? rev.commit.parents.length : 0;
return parentLength === 0 || parentLength > 1;
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
index 3c99648..146b3e2 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
@@ -35,7 +35,6 @@
import {
computeLatestPatchNum,
getRevisionByPatchNum,
- patchNumEquals,
PatchSet,
} from '../../../utils/patch-set-util';
import {property, computed, observe, customElement} from '@polymer/decorators';
@@ -321,8 +320,7 @@
_handlePatchChange(e: CustomEvent) {
const {basePatchNum, patchNum} = e.detail;
if (
- (patchNumEquals(basePatchNum, this.basePatchNum) &&
- patchNumEquals(patchNum, this.patchNum)) ||
+ (basePatchNum === this.basePatchNum && patchNum === this.patchNum) ||
!this.change
) {
return;
@@ -354,7 +352,7 @@
_computePatchInfoClass(patchNum?: PatchSetNum, allPatchSets?: PatchSet[]) {
const latestNum = computeLatestPatchNum(allPatchSets);
- if (patchNumEquals(patchNum, latestNum)) {
+ if (patchNum === latestNum) {
return '';
}
return 'patchInfoOldPatchSet';
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js
index cee0262..b8670c0 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.js
@@ -249,11 +249,11 @@
test('class is applied to file list on old patch set', () => {
const allPatchSets = [{num: 4}, {num: 2}, {num: 1}];
- assert.equal(element._computePatchInfoClass('1', allPatchSets),
+ assert.equal(element._computePatchInfoClass(1, allPatchSets),
'patchInfoOldPatchSet');
- assert.equal(element._computePatchInfoClass('2', allPatchSets),
+ assert.equal(element._computePatchInfoClass(2, allPatchSets),
'patchInfoOldPatchSet');
- assert.equal(element._computePatchInfoClass('4', allPatchSets), '');
+ assert.equal(element._computePatchInfoClass(4, allPatchSets), '');
});
suite('editMode behavior', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
index fb20f0c..bc61dad 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
@@ -46,6 +46,11 @@
import {appContext} from '../../../services/app-context';
import {pluralize} from '../../../utils/string-util';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
+import {
+ computeAllPatchSets,
+ computeLatestPatchNum,
+ computePredecessor,
+} from '../../../utils/patch-set-util';
const PATCH_SET_PREFIX_PATTERN = /^(?:Uploaded\s*)?(?:P|p)atch (?:S|s)et \d+:\s*(.*)/;
const LABEL_TITLE_SCORE_PATTERN = /^(-?)([A-Za-z0-9-]+?)([+-]\d+)?[.]?$/;
@@ -270,13 +275,20 @@
_handleViewPatchsetDiff(e: Event) {
if (!this.message || !this.change) return;
const match = this.message.message.match(/Uploaded patch set (\d+)./);
- if (!match || match.length < 1) return;
- const patchNum = Number(match[1]);
- if (isNaN(patchNum)) throw new Error('invalid patchnum in message');
+ let patchNum: PatchSetNum;
+ // Message is of the form "Commit Message was updated" or "Patchset X
+ // was rebased"
+ if (!match || match.length < 1) {
+ patchNum = computeLatestPatchNum(computeAllPatchSets(this.change))!;
+ } else {
+ if (isNaN(Number(match[1])))
+ throw new Error('invalid patchnum in message');
+ patchNum = Number(match[1]) as PatchSetNum;
+ }
GerritNav.navigateToChange(
this.change,
- patchNum as PatchSetNum,
- (patchNum === 1 ? 'PARENT' : patchNum - 1) as PatchSetNum
+ patchNum,
+ computePredecessor(patchNum)
);
// stop propagation to stop message expansion
e.stopPropagation();
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.js b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.js
index 94507e6..facde01 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.js
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.js
@@ -18,6 +18,7 @@
import '../../../test/common-test-setup-karma.js';
import './gr-message.js';
import {GerritNav} from '../../core/gr-navigation/gr-navigation.js';
+import {createChange, createRevisions} from '../../../test/test-data-generators.js';
const basicFixture = fixtureFromElement('gr-message');
@@ -232,7 +233,7 @@
suite('uploaded patchset X message navigates to X - 1 vs X', () => {
let navStub;
setup(() => {
- element.change = {changeNum: 12345};
+ element.change = {...createChange(), revisions: createRevisions(4)};
navStub = sinon.stub(GerritNav, 'navigateToChange');
});
@@ -241,7 +242,7 @@
message: 'Uploaded patch set 1.',
};
element._handleViewPatchsetDiff(new MouseEvent('click'));
- assert.isTrue(navStub.calledWithExactly({changeNum: 12345}, 1,
+ assert.isTrue(navStub.calledWithExactly(element.change, 1,
'PARENT'));
});
@@ -250,21 +251,21 @@
message: 'Uploaded patch set 2.',
};
element._handleViewPatchsetDiff(new MouseEvent('click'));
- assert.isTrue(navStub.calledWithExactly({changeNum: 12345}, 2, 1));
+ assert.isTrue(navStub.calledWithExactly(element.change, 2, 1));
element.message = {
message: 'Uploaded patch set 200.',
};
element._handleViewPatchsetDiff(new MouseEvent('click'));
- assert.isTrue(navStub.calledWithExactly({changeNum: 12345}, 200, 199));
+ assert.isTrue(navStub.calledWithExactly(element.change, 200, 199));
});
- test('invalid patchset does not cause navigation', () => {
+ test('Commit message updated', () => {
element.message = {
- message: 'Uploaded patch set XYZ.',
+ message: 'Commit message updated.',
};
element._handleViewPatchsetDiff(new MouseEvent('click'));
- assert.isFalse(navStub.called);
+ assert.isTrue(navStub.calledWithExactly(element.change, 4, 3));
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index fc6b5ba..e6c9f42 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -25,7 +25,7 @@
import {htmlTemplate} from './gr-related-changes-list_html';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
import {ChangeStatus} from '../../../constants/constants';
-import {patchNumEquals} from '../../../utils/patch-set-util';
+
import {changeIsOpen} from '../../../utils/change-util';
import {getPluginEndpoints} from '../../shared/gr-js-api-interface/gr-plugin-endpoints';
import {customElement, observe, property} from '@polymer/decorators';
@@ -410,7 +410,7 @@
return [];
}
for (const rev in change.revisions) {
- if (patchNumEquals(change.revisions[rev]._number, patchNum)) {
+ if (change.revisions[rev]._number === patchNum) {
changeRevision = rev;
}
}
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 07045cd..2f3d03c 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -46,10 +46,7 @@
WeblinkType,
} from '../gr-navigation/gr-navigation';
import {appContext} from '../../../services/app-context';
-import {
- convertToPatchSetNum,
- patchNumEquals,
-} from '../../../utils/patch-set-util';
+import {convertToPatchSetNum} from '../../../utils/patch-set-util';
import {customElement, property} from '@polymer/decorators';
import {assertNever} from '../../../utils/common-util';
import {
@@ -693,10 +690,7 @@
// Diffing a patch against itself is invalid, so if the base and revision
// patches are equal clear the base.
- if (
- params.patchNum &&
- patchNumEquals(params.basePatchNum, params.patchNum)
- ) {
+ if (params.patchNum && params.basePatchNum === params.patchNum) {
needsRedirect = true;
params.basePatchNum = null;
} else if (!hasPatchNum) {
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
index 9988116..5210616 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -18,7 +18,7 @@
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-comment-api_html';
-import {patchNumEquals, CURRENT} from '../../../utils/patch-set-util';
+import {CURRENT} from '../../../utils/patch-set-util';
import {customElement, property} from '@polymer/decorators';
import {
CommentBasics,
@@ -30,6 +30,7 @@
NumericChangeId,
PathToCommentsInfoMap,
FileInfo,
+ ParentPatchSetNum,
} from '../../../types/common';
import {hasOwnProperty} from '../../../utils/common-util';
import {
@@ -235,9 +236,7 @@
allComments = allComments.concat(drafts);
}
if (patchNum) {
- allComments = allComments.filter(c =>
- patchNumEquals(c.patch_set, patchNum)
- );
+ allComments = allComments.filter(c => c.patch_set === patchNum);
}
return allComments.map(c => {
return {...c};
@@ -284,7 +283,7 @@
getAllDraftsForPath(path: string, patchNum?: PatchSetNum): Comment[] {
let comments = this._drafts[path] || [];
if (patchNum) {
- comments = comments.filter(c => patchNumEquals(c.patch_set, patchNum));
+ comments = comments.filter(c => c.patch_set === patchNum);
}
return comments.map(c => {
return {...c, __draft: true};
@@ -369,6 +368,10 @@
* does not return the range of the ported thread and it becomes a file level
* thread.
*
+ * If a comment was created with Side=PARENT, then we only show this ported
+ * comment if Base is part of the patch range, always on the left side of
+ * the diff.
+ *
* @return only the ported threads for the specified file and patch range
*/
_getPortedCommentThreads(
@@ -407,15 +410,22 @@
return false;
}
- // TODO(dhruvsri): Add handling for thread.commentSide = PARENT
- if (thread.commentSide === CommentSide.PARENT) return false;
+ thread.diffSide = Side.RIGHT;
+ if (thread.commentSide === CommentSide.PARENT) {
+ // TODO(dhruvsri): Add handling for merge parents
+ if (
+ patchRange.basePatchNum !== ParentPatchSetNum ||
+ !!thread.mergeParentNum
+ )
+ return false;
+ thread.diffSide = Side.LEFT;
+ }
if (!isUnresolved(thread) && !isDraftThread(thread)) return false;
thread.range = portedComment.range;
thread.line = portedComment.line;
thread.ported = true;
- thread.diffSide = Side.RIGHT;
return true;
});
}
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js
index dd36613..e107c16 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js
@@ -20,6 +20,7 @@
import {ChangeComments} from './gr-comment-api.js';
import {isInRevisionOfPatchRange, isInBaseOfPatchRange, isDraftThread, isUnresolved, createCommentThreads} from '../../../utils/comment-util.js';
import {createDraft, createComment, createChangeComments, createCommentThread} from '../../../test/test-data-generators.js';
+import {CommentSide, Side} from '../../../constants/constants.js';
const basicFixture = fixtureFromElement('gr-comment-api');
@@ -153,7 +154,7 @@
const comment1 = {
...createComment(),
unresolved: true,
- id: 'db977012_e1f13818',
+ id: '1',
line: 136,
patch_set: 2,
range: {
@@ -167,10 +168,22 @@
const comment2 = {
...createComment(),
patch_set: 2,
- id: 'ecf0b9fa_fe1a5f62',
+ id: '2',
line: 5,
};
+ const comment3 = {
+ ...createComment(),
+ side: CommentSide.PARENT,
+ line: 10,
+ unresolved: true,
+ };
+
+ const comment4 = {
+ ...comment3,
+ parent: -2,
+ };
+
const draft1 = {
...createDraft(),
id: 'db977012_e1f13828',
@@ -277,6 +290,74 @@
).length, 0);
});
+ test('comments with side=PARENT are ported over', () => {
+ changeComments = new ChangeComments(
+ {/* comments */
+ // comment left on Base
+ 'karma.conf.js': [comment3],
+ },
+ {}/* robot comments */,
+ {/* drafts */
+ 'karma.conf.js': [draft2],
+ },
+ {/* ported comments */
+ 'karma.conf.js': [{
+ ...comment3,
+ line: 31,
+ patch_set: 4,
+ }],
+ },
+ {}/* ported drafts */
+ );
+
+ const portedThreads = changeComments._getPortedCommentThreads(
+ {path: 'karma.conf.js'}, {patchNum: 4, basePatchNum: 'PARENT'});
+ assert.equal(portedThreads.length, 1);
+ assert.equal(portedThreads[0].line, 31);
+ assert.equal(portedThreads[0].diffSide, Side.LEFT);
+
+ assert.equal(changeComments._getPortedCommentThreads(
+ {path: 'karma.conf.js'}, {patchNum: 4, basePatchNum: -2}
+ ).length, 0);
+
+ assert.equal(changeComments._getPortedCommentThreads(
+ {path: 'karma.conf.js'}, {patchNum: 4, basePatchNum: 2}
+ ).length, 0);
+ });
+
+ test('comments left on merge parent is not ported over', () => {
+ changeComments = new ChangeComments(
+ {/* comments */
+ // comment left on Base
+ 'karma.conf.js': [comment4],
+ },
+ {}/* robot comments */,
+ {/* drafts */
+ 'karma.conf.js': [draft2],
+ },
+ {/* ported comments */
+ 'karma.conf.js': [{
+ ...comment4,
+ line: 31,
+ patch_set: 4,
+ }],
+ },
+ {}/* ported drafts */
+ );
+
+ const portedThreads = changeComments._getPortedCommentThreads(
+ {path: 'karma.conf.js'}, {patchNum: 4, basePatchNum: 'PARENT'});
+ assert.equal(portedThreads.length, 0);
+
+ assert.equal(changeComments._getPortedCommentThreads(
+ {path: 'karma.conf.js'}, {patchNum: 4, basePatchNum: -2}
+ ).length, 0);
+
+ assert.equal(changeComments._getPortedCommentThreads(
+ {path: 'karma.conf.js'}, {patchNum: 4, basePatchNum: 2}
+ ).length, 0);
+ });
+
test('ported comments contribute to comment count', () => {
assert.equal(changeComments.computeCommentsString(
{basePatchNum: 'PARENT', patchNum: 2}, 'karma.conf.js',
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index 2c050f9..d09c811 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -1040,6 +1040,7 @@
commentSide: CommentSide.REVISION,
path: '/p',
rootId: 'betsys_confession',
+ mergeParentNum: undefined,
comments: [{
id: 'betsys_confession',
path: '/p',
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 828c53d..84b227b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -44,7 +44,6 @@
import {
computeAllPatchSets,
computeLatestPatchNum,
- patchNumEquals,
PatchSet,
} from '../../../utils/patch-set-util';
import {
@@ -880,12 +879,10 @@
_displayDiffAgainstLatestToast(latestPatchNum?: PatchSetNum) {
if (!this._patchRange) return;
- const leftPatchset = patchNumEquals(
- this._patchRange.basePatchNum,
- ParentPatchSetNum
- )
- ? 'Base'
- : `Patchset ${this._patchRange.basePatchNum}`;
+ const leftPatchset =
+ this._patchRange.basePatchNum === ParentPatchSetNum
+ ? 'Base'
+ : `Patchset ${this._patchRange.basePatchNum}`;
fireAlert(
this,
`${leftPatchset} vs
@@ -896,12 +893,12 @@
_displayToasts() {
if (!this._patchRange) return;
- if (!patchNumEquals(this._patchRange.basePatchNum, ParentPatchSetNum)) {
+ if (this._patchRange.basePatchNum !== ParentPatchSetNum) {
this._displayDiffBaseAgainstLeftToast();
return;
}
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
- if (!patchNumEquals(this._patchRange.patchNum, latestPatchNum)) {
+ if (this._patchRange.patchNum !== latestPatchNum) {
this._displayDiffAgainstLatestToast(latestPatchNum);
return;
}
@@ -916,17 +913,17 @@
if (!hasOwnProperty(this._change.revisions, commitSha)) continue;
const revision = this._change.revisions[commitSha];
const patchNum = revision._number;
- if (patchNumEquals(patchNum, this._patchRange.patchNum)) {
+ if (patchNum === this._patchRange.patchNum) {
commit = commitSha as CommitId;
const commitObj = revision.commit;
const parents = commitObj?.parents || [];
if (
- patchNumEquals(this._patchRange.basePatchNum, ParentPatchSetNum) &&
+ this._patchRange.basePatchNum === ParentPatchSetNum &&
parents.length
) {
baseCommit = parents[parents.length - 1].commit;
}
- } else if (patchNumEquals(patchNum, this._patchRange.basePatchNum)) {
+ } else if (patchNum === this._patchRange.basePatchNum) {
baseCommit = commitSha as CommitId;
}
}
@@ -1250,7 +1247,7 @@
if (!patchRange) return {patchNum, basePatchNum};
if (
patchRange.basePatchNum !== ParentPatchSetNum ||
- !patchNumEquals(patchRange.patchNum, latestPatchNum as PatchSetNum)
+ patchRange.patchNum !== latestPatchNum
) {
patchNum = patchRange.patchNum;
basePatchNum = patchRange.basePatchNum;
@@ -1352,8 +1349,8 @@
const {basePatchNum, patchNum} = e.detail;
if (
- patchNumEquals(basePatchNum, this._patchRange.basePatchNum) &&
- patchNumEquals(patchNum, this._patchRange.patchNum)
+ basePatchNum === this._patchRange.basePatchNum &&
+ patchNum === this._patchRange.patchNum
) {
return;
}
@@ -1582,7 +1579,7 @@
patchRangeRecord: PolymerDeepPropertyChange<PatchRange, PatchRange>
) {
const patchRange = patchRangeRecord.base || {};
- return patchNumEquals(patchRange.patchNum, EditPatchSetNum);
+ return patchRange.patchNum === EditPatchSetNum;
}
_computeBlameToggleLabel(loaded?: boolean, loading?: boolean) {
@@ -1641,7 +1638,7 @@
if (!this._path) return;
if (!this._patchRange) return;
- if (patchNumEquals(this._patchRange.basePatchNum, ParentPatchSetNum)) {
+ if (this._patchRange.basePatchNum === ParentPatchSetNum) {
fireAlert(this, 'Base is already selected.');
return;
}
@@ -1658,7 +1655,7 @@
if (!this._path) return;
if (!this._patchRange) return;
- if (patchNumEquals(this._patchRange.basePatchNum, ParentPatchSetNum)) {
+ if (this._patchRange.basePatchNum === ParentPatchSetNum) {
fireAlert(this, 'Left is already base.');
return;
}
@@ -1680,7 +1677,7 @@
if (!this._patchRange) return;
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
- if (patchNumEquals(this._patchRange.patchNum, latestPatchNum)) {
+ if (this._patchRange.patchNum === latestPatchNum) {
fireAlert(this, 'Latest is already selected.');
return;
}
@@ -1700,7 +1697,7 @@
if (!this._patchRange) return;
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
- if (patchNumEquals(this._patchRange.patchNum, latestPatchNum)) {
+ if (this._patchRange.patchNum === latestPatchNum) {
fireAlert(this, 'Right is already latest.');
return;
}
@@ -1720,8 +1717,8 @@
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
if (
- patchNumEquals(this._patchRange.patchNum, latestPatchNum) &&
- patchNumEquals(this._patchRange.basePatchNum, ParentPatchSetNum)
+ this._patchRange.patchNum === latestPatchNum &&
+ this._patchRange.basePatchNum === ParentPatchSetNum
) {
fireAlert(this, 'Already diffing base against latest.');
return;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index 04714bb..39d645e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -38,7 +38,7 @@
rangesEqual,
} from './gr-diff-utils';
import {getHiddenScroll} from '../../../scripts/hiddenscroll';
-import {isMergeParent, patchNumEquals} from '../../../utils/patch-set-util';
+import {isMergeParent} from '../../../utils/patch-set-util';
import {customElement, observe, property} from '@polymer/decorators';
import {
BlameInfo,
@@ -605,10 +605,10 @@
? this.patchRange.basePatchNum
: this.patchRange.patchNum;
- const isEdit = patchNumEquals(patchNum, EditPatchSetNum);
+ const isEdit = patchNum === EditPatchSetNum;
const isEditBase =
- patchNumEquals(patchNum, ParentPatchSetNum) &&
- patchNumEquals(this.patchRange.patchNum, EditPatchSetNum);
+ patchNum === ParentPatchSetNum &&
+ this.patchRange.patchNum === EditPatchSetNum;
if (isEdit) {
fireAlert(this, 'You cannot comment on an edit.');
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 ec39da2..671bd41 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
@@ -30,7 +30,6 @@
getParentIndex,
getRevisionByPatchNum,
isMergeParent,
- patchNumEquals,
sortRevisions,
PatchSet,
} from '../../../utils/patch-set-util';
@@ -346,7 +345,7 @@
patchNum: PatchSetNum,
sortedRevisions: RevisionInfo[]
): boolean {
- if (patchNumEquals(basePatchNum, ParentPatchSetNum)) {
+ if (basePatchNum === ParentPatchSetNum) {
return false;
}
@@ -441,7 +440,7 @@
});
detail.patchNum = patchSetValue;
} else {
- if (patchNumEquals(detail.basePatchNum, patchSetValue)) return;
+ if (detail.basePatchNum === patchSetValue) return;
this.reporting.reportInteraction('left-patchset-changed', {
previous: detail.basePatchNum,
current: e.detail.value,
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_html.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_html.ts
index d9a3187..fbf7f47 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_html.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_html.ts
@@ -264,10 +264,16 @@
</gr-account-label>
</template>
<template is="dom-if" if="[[showPortedComment]]">
- <a href="[[_getUrlForComment(comment)]]">
- <span class="portedMessage">
- Ported from patchset [[comment.patch_set]]
- </span>
+ <a href="[[_getUrlForComment(comment)]]"
+ ><span class="portedMessage"
+ >Ported from patchset [[comment.patch_set]]</span
+ ></a
+ >
+ <a
+ href="https://bugs.chromium.org/p/gerrit/issues/entry?template=Porting+Comments"
+ target="_blank"
+ >
+ <iron-icon icon="gr-icons:bug" title="report a problem"></iron-icon>
</a>
</template>
<gr-tooltip-content
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list_html.ts b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list_html.ts
index f313174..c163924 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list_html.ts
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list_html.ts
@@ -145,7 +145,6 @@
slot="dropdown-content"
attr-for-selected="data-value"
selected="{{value}}"
- on-tap="_handleDropdownTap"
>
<template
is="dom-repeat"
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 ae6e155..00f9a40 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
@@ -18,7 +18,7 @@
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {getPluginLoader} from './gr-plugin-loader';
-import {patchNumEquals} from '../../../utils/patch-set-util';
+
import {customElement} from '@polymer/decorators';
import {
ChangeInfo,
@@ -154,7 +154,7 @@
let revision;
for (const rev of Object.values(change.revisions || {})) {
- if (patchNumEquals(rev._number, patchNum)) {
+ if (rev._number === patchNum) {
revision = rev;
break;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
index 078e07f..9e77784 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
@@ -36,11 +36,7 @@
import {parseDate} from '../../../utils/date-util';
import {getBaseUrl} from '../../../utils/url-util';
import {appContext} from '../../../services/app-context';
-import {
- getParentIndex,
- isMergeParent,
- patchNumEquals,
-} from '../../../utils/patch-set-util';
+import {getParentIndex, isMergeParent} from '../../../utils/patch-set-util';
import {
ListChangesOption,
listChangesOptionsToHex,
@@ -1354,7 +1350,7 @@
let params = undefined;
if (isMergeParent(patchRange.basePatchNum)) {
params = {parent: getParentIndex(patchRange.basePatchNum)};
- } else if (!patchNumEquals(patchRange.basePatchNum, ParentPatchSetNum)) {
+ } else if (patchRange.basePatchNum !== ParentPatchSetNum) {
params = {base: patchRange.basePatchNum};
}
return this._getChangeURLAndFetch({
@@ -1401,7 +1397,7 @@
changeNum: NumericChangeId,
patchRange: PatchRange
): Promise<FileNameToFileInfoMap | undefined> {
- if (patchNumEquals(patchRange.patchNum, EditPatchSetNum)) {
+ if (patchRange.patchNum === EditPatchSetNum) {
return this.getChangeEditFiles(changeNum, patchRange).then(
res => res && res.files
);
@@ -1983,9 +1979,10 @@
}
return res;
};
- const promise = patchNumEquals(patchNum, EditPatchSetNum)
- ? this._getFileInChangeEdit(changeNum, path)
- : this._getFileInRevision(changeNum, path, patchNum, suppress404s);
+ const promise =
+ patchNum === EditPatchSetNum
+ ? this._getFileInChangeEdit(changeNum, path)
+ : this._getFileInRevision(changeNum, path, patchNum, suppress404s);
return promise.then(res => {
if (!res || !res.ok) {
@@ -2277,8 +2274,7 @@
};
if (isMergeParent(basePatchNum)) {
params.parent = getParentIndex(basePatchNum);
- } else if (!patchNumEquals(basePatchNum, ParentPatchSetNum)) {
- // TODO (TS): fix as PatchSetNum in the condition above
+ } else if (basePatchNum !== ParentPatchSetNum) {
params.base = basePatchNum;
}
const endpoint = `/files/${encodeURIComponent(path)}/diff`;
diff --git a/polygerrit-ui/app/elements/shared/revision-info/revision-info.ts b/polygerrit-ui/app/elements/shared/revision-info/revision-info.ts
index fadbfa7..d4bbe16 100644
--- a/polygerrit-ui/app/elements/shared/revision-info/revision-info.ts
+++ b/polygerrit-ui/app/elements/shared/revision-info/revision-info.ts
@@ -15,7 +15,6 @@
* limitations under the License.
*/
-import {patchNumEquals} from '../../../utils/patch-set-util';
import {ChangeInfo, PatchSetNum} from '../../../types/common';
import {ParsedChangeInfo} from '../gr-rest-api-interface/gr-reviewer-updates-parser';
@@ -71,8 +70,8 @@
getParentId(patchNum: PatchSetNum, parentIndex: number) {
if (!this.change.revisions) return;
- const rev = Object.values(this.change.revisions).find(rev =>
- patchNumEquals(rev._number, patchNum)
+ const rev = Object.values(this.change.revisions).find(
+ rev => rev._number === patchNum
);
if (!rev || !rev.commit) return;
return rev.commit.parents[parentIndex].commit;
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 5d8ea37..f4c0692 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -29,7 +29,7 @@
import {parseDate} from './date-util';
import {LineNumber} from '../elements/diff/gr-diff/gr-diff-line';
import {CommentIdToCommentThreadMap} from '../elements/diff/gr-comment-api/gr-comment-api';
-import {isMergeParent, getParentIndex, patchNumEquals} from './patch-set-util';
+import {isMergeParent, getParentIndex} from './patch-set-util';
export interface DraftCommentProps {
__draft?: boolean;
@@ -126,6 +126,7 @@
comments: [comment],
patchNum: comment.patch_set,
commentSide: comment.side ?? CommentSide.REVISION,
+ mergeParentNum: comment.parent,
path: comment.path,
line: comment.line,
range: comment.range,
@@ -151,6 +152,11 @@
comments: UIComment[];
path: string;
commentSide: CommentSide;
+ /* mergeParentNum is the merge parent number only valid for merge commits
+ when commentSide is PARENT.
+ mergeParentNum is undefined for auto merge commits
+ */
+ mergeParentNum?: number;
patchNum?: PatchSetNum;
line?: LineNumber;
/* rootId is optional since we create a empty comment thread element for
@@ -196,7 +202,7 @@
if (
range.basePatchNum === ParentPatchSetNum &&
comment.side === CommentSide.PARENT &&
- patchNumEquals(comment.patch_set, range.patchNum)
+ comment.patch_set === range.patchNum
) {
return true;
}
@@ -204,7 +210,7 @@
return (
range.basePatchNum !== ParentPatchSetNum &&
comment.side !== CommentSide.PARENT &&
- patchNumEquals(comment.patch_set, range.basePatchNum)
+ comment.patch_set === range.basePatchNum
);
}
@@ -217,8 +223,7 @@
range: PatchRange
) {
return (
- comment.side !== CommentSide.PARENT &&
- patchNumEquals(comment.patch_set, range.patchNum)
+ comment.side !== CommentSide.PARENT && comment.patch_set === range.patchNum
);
}
@@ -247,7 +252,7 @@
patchNum: comment.patch_set,
basePatchNum: ParentPatchSetNum,
};
- } else if (patchNumEquals(latestPatchNum, comment.patch_set)) {
+ } else if (latestPatchNum === comment.patch_set) {
return {
patchNum: latestPatchNum,
basePatchNum: ParentPatchSetNum,
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index fda302b..cb0bdec 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -60,19 +60,6 @@
}
/**
- * As patchNum can be either a string (e.g. 'edit', 'PARENT') OR a number,
- * this function checks for patchNum equality.
- *
- */
-export function patchNumEquals(a?: PatchSetNum, b?: PatchSetNum) {
- if (a === undefined) {
- return a === b;
- }
- // TODO(TS): replace with a===b when the whole code is converted to ts
- return `${a}` === `${b}`;
-}
-
-/**
* Whether the given patch is a numbered parent of a merge (i.e. a negative
* number).
*/
@@ -114,7 +101,7 @@
patchNum: PatchSetNum
) {
for (const rev of revisions) {
- if (patchNumEquals(rev._number, patchNum)) {
+ if (rev._number === patchNum) {
return rev;
}
}
@@ -275,6 +262,20 @@
return allPatchSets[0].num;
}
+export function computePredecessor(
+ patchset?: PatchSetNum
+): PatchSetNum | undefined {
+ if (
+ !patchset ||
+ patchset === ParentPatchSetNum ||
+ patchset === EditPatchSetNum
+ ) {
+ return undefined;
+ }
+ if (patchset === 1) return ParentPatchSetNum;
+ return (Number(patchset) - 1) as PatchSetNum;
+}
+
export function hasEditBasedOnCurrentPatchSet(allPatchSets: PatchSet[]) {
if (!allPatchSets || allPatchSets.length < 2) {
return false;
diff --git a/polygerrit-ui/app/utils/patch-set-util_test.js b/polygerrit-ui/app/utils/patch-set-util_test.js
index 29cc370..12f5ed4 100644
--- a/polygerrit-ui/app/utils/patch-set-util_test.js
+++ b/polygerrit-ui/app/utils/patch-set-util_test.js
@@ -21,7 +21,7 @@
fetchChangeUpdates, findEditParentPatchNum, findEditParentRevision,
getParentIndex, getRevisionByPatchNum,
isMergeParent,
- patchNumEquals, sortRevisions,
+ sortRevisions,
} from './patch-set-util.js';
suite('gr-patch-set-util tests', () => {
@@ -31,9 +31,9 @@
{_number: 1},
{_number: 2},
];
- assert.deepEqual(getRevisionByPatchNum(revisions, '1'), revisions[1]);
+ assert.deepEqual(getRevisionByPatchNum(revisions, 1), revisions[1]);
assert.deepEqual(getRevisionByPatchNum(revisions, 2), revisions[2]);
- assert.equal(getRevisionByPatchNum(revisions, '3'), undefined);
+ assert.equal(getRevisionByPatchNum(revisions, 3), undefined);
});
test('fetchChangeUpdates on latest', done => {
@@ -231,17 +231,6 @@
.assertWip(6, true); // PS6 was uploaded with WIP option
});
- test('patchNumEquals', () => {
- assert.isFalse(patchNumEquals('edit', 'PARENT'));
- assert.isFalse(patchNumEquals('edit', NaN));
- assert.isFalse(patchNumEquals(1, '2'));
-
- assert.isTrue(patchNumEquals(1, '1'));
- assert.isTrue(patchNumEquals(1, 1));
- assert.isTrue(patchNumEquals('edit', 'edit'));
- assert.isTrue(patchNumEquals('PARENT', 'PARENT'));
- });
-
test('isMergeParent', () => {
assert.isFalse(isMergeParent(1));
assert.isFalse(isMergeParent(4321));
