Merge "Create plugin attributeFactories at query start"
diff --git a/.mailmap b/.mailmap
index f2fe6ca..cbf1f3b 100644
--- a/.mailmap
+++ b/.mailmap
@@ -38,6 +38,7 @@
Johan Björk <jbjoerk@gmail.com> Johan Bjork <phb@spotify.com>
JT Olds <hello@jtolds.com> <jtolds@gmail.com>
Kasper Nilsson <kaspern@google.com> <kaspern@google.com>
+Lawrence Dubé <ldube@audiokinetic.com> <ldube@audiokinetic.com>
Lei Sun <lei.sun01@sap.com> LeiSun <lei.sun01@sap.com>
Lincoln Oliveira Campos Do Nascimento <lincoln.oliveiracamposdonascimento@sonyericsson.com> lincoln <lincoln.oliveiracamposdonascimento@sonyericsson.com>
Luca Milanesio <luca.milanesio@gmail.com> <luca@gitent-scm.com>
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 589071a..e13a9b9 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -4654,103 +4654,6 @@
+
By default 0.
-[[theme]]
-=== Section theme
-
-[[theme.backgroundColor]]theme.backgroundColor::
-+
-_(GWT UI only)_ Background color for the page, and major data tables like the all
-open changes table or the account dashboard. The value must be a
-valid HTML hex color code, or standard color name.
-+
-By default white, `FFFFFF`.
-
-[[theme.topMenuColor]]theme.topMenuColor::
-+
-_(GWT UI only)_ This is the color of the main menu bar at the top of the page.
-The value must be a valid HTML hex color code, or standard color
-name.
-+
-By default white, `FFFFFF`.
-
-[[theme.textColor]]theme.textColor::
-+
-_(GWT UI only)_ Text color for the page, and major data tables like the all open
-changes table or the account dashboard. The value must be a valid HTML hex color
-code, or standard color name.
-+
-By default dark grey, `353535`.
-
-[[theme.trimColor]]theme.trimColor::
-+
-_(GWT UI only)_ Primary color used as a background color behind text. This is
-the color of the main menu bar at the top, of table headers, and of major UI
-areas that we want to offset from other portions of the page. The value must be
-a valid HTML hex color code, or standard color name.
-+
-By default a light grey, `EEEEEE`.
-
-[[theme.selectionColor]]theme.selectionColor::
-+
-_(GWT UI only)_ Background color used within a trimColor area to denote the
-currently selected tab, or the background color used in a table to denote the
-currently selected row. The value must be a valid HTML hex color code, or
-standard color name.
-+
-By default a pale blue, `D8EDF9`.
-
-[[theme.changeTableOutdatedColor]]theme.changeTableOutdatedColor::
-+
-_(GWT UI only)_ Background color used for patch outdated messages. The value
-must be a valid HTML hex color code, or standard color name.
-+
-By default a shade of red, `F08080`.
-
-[[theme.tableOddRowColor]]theme.tableOddRowColor::
-+
-_(GWT UI only)_ Background color for tables such as lists of open reviews for
-odd rows. This is so you can have a different color for odd and even rows of
-the table. The value must be a valid HTML hex color code, or standard color
-name.
-+
-By default transparent.
-
-[[theme.tableEvenRowColor]]theme.tableEvenRowColor::
-+
-_(GWT UI only)_ Background color for tables such as lists of open reviews for
-even rows. This is so you can have a different color for odd and even rows of
-the table. The value must be a valid HTML hex color code, or standard color
-name.
-+
-By default transparent.
-
-A different theme may be used for signed-in vs. signed-out user status
-by using the "signed-in" and "signed-out" theme sections. Variables
-not specified in a section are inherited from the default theme.
-
-----
-[theme]
- backgroundColor = FFFFFF
-[theme "signed-in"]
- backgroundColor = C0C0C0
-[theme "signed-out"]
- backgroundColor = 00FFFF
-----
-
-As example, here is the theme configuration to have the old green look:
-
-----
-[theme]
- backgroundColor = FCFEEF
- textColor = 000000
- trimColor = D4E9A9
- selectionColor = FFFFCC
- topMenuColor = D4E9A9
- changeTableOutdatedColor = F08080
-[theme "signed-in"]
- backgroundColor = FFFFFF
-----
-
[[trackingid]]
=== Section trackingid
@@ -5095,10 +4998,6 @@
The format is one Base-64 encoded public key per line.
-== Configuring the Polygerrit UI
-
-Please see link:dev-polygerrit.html[UI] on configuring the Polygerrit UI.
-
=== Configurable Parameters
site_path::
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index d5ffda2..d7bd8b3 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -801,6 +801,64 @@
}
----
+=== Calling Command Options ===
+
+Within an OptionHandler, during the processing of an option, plugins can
+provide and call extra parameters on the current command during parsing
+simulating as if they had been passed from the command line originally.
+
+To call additional parameters from within an option handler, instantiate
+the com.google.gerrit.util.cli.CmdLineParser.Parameters class with the
+existing parameters, and then call callParameters() with the additional
+parameters to be parsed. OptionHandlers may optionally pass this class to
+other methods which may then both parse/consume more parameters and call
+additional parameters.
+
+When calling command options not provided by your plugin, there is always
+a risk that the options may not exist, perhaps because the options being
+called are to be provided by another plugin, and said plugin is not
+currently installed. To protect againt this situation, it is possible to
+define an option as being dependent on other options using the
+@RequiresOptions() annotation. If the required options are not all not
+currently present, then the dependent option will not be available or
+visible in the help.
+
+The example below shows a plugin that adds a "--special" option (perhaps
+for use with the Query command) that calls (and requires) the
+"--format json" option.
+
+[source, java]
+----
+public class JsonOutputOptionHandler<T> extends OptionHandler<T> {
+ protected com.google.gerrit.util.cli.CmdLineParser.MyParser myParser;
+
+ public JsonOutputOptionHandler(CmdLineParser parser, OptionDef option, Setter<? super T> setter) {
+ super(parser, option, setter);
+ myParser = (com.google.gerrit.util.cli.CmdLineParser.MyParser) owner;
+ }
+
+ @Override
+ public int parseArguments(org.kohsuke.args4j.spi.Parameters params) throws CmdLineException {
+ new Parameters(params, myParser).callParameters("--format", "json");
+ setter.addValue(true);
+ return 0; // we didn't consume any additional args
+ }
+
+ @Override
+ public String getDefaultMetaVariable() {
+ ...
+ }
+}
+
+@RequiresOptions("--format")
+@Option(
+ name = "--special",
+ usage = "ouptut results using json",
+ handler = JsonOutputOptionHandler.class
+)
+boolean json;
+----
+
[[query_attributes]]
=== Query Attributes ===
diff --git a/Documentation/dev-polygerrit.txt b/Documentation/dev-polygerrit.txt
deleted file mode 100644
index 5621d32..0000000
--- a/Documentation/dev-polygerrit.txt
+++ /dev/null
@@ -1,34 +0,0 @@
-= PolyGerrit - GUI
-
-== Configuring
-
-By default both GWT and PolyGerrit UI are available to users.
-
-To make PolyGerrit the default UI but keep GWT as a secondary UI:
-----
-[gerrit]
- ui = POLYGERRIT
-----
-
-To disable GWT but not PolyGerrit:
-----
-[gerrit]
- enableGwtUi = false
- enablePolyGerrit = true
-----
-
-To enable GWT but not PolyGerrit:
-----
-[gerrit]
- enableGwtUi = true
- enablePolyGerrit = false
-----
-
-To switch to the PolyGerrit UI you have to add `?polygerrit=1` in the URL.
-
-for example https://gerrit.example.org/?polygerrit=1
-
-To disable PolyGerrit UI, change 1 to 0, which will take you back to GWT UI.
-
-
-More information can be found in the link:https://gerrit.googlesource.com/gerrit/+/master/polygerrit-ui/[README]
diff --git a/WORKSPACE b/WORKSPACE
index 50714f1..8c8102b 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -1016,8 +1016,8 @@
# and httpasyncclient as necessary.
maven_jar(
name = "elasticsearch-rest-client",
- artifact = "org.elasticsearch.client:elasticsearch-rest-client:6.4.3",
- sha1 = "5c24325430971ba2fa4769eb446f026b7680d5e7",
+ artifact = "org.elasticsearch.client:elasticsearch-rest-client:6.5.0",
+ sha1 = "241436d27cf65b84d17126dc7b6b947e8e2c173c",
)
JACKSON_VERSION = "2.9.7"
diff --git a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java
index fec7137..6da19cd 100644
--- a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java
+++ b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java
@@ -373,7 +373,7 @@
List<T> results = Collections.emptyList();
String uri = getURI(index, SEARCH);
Response response =
- performRequest(HttpPost.METHOD_NAME, search, uri, Collections.emptyMap());
+ performRequest(HttpPost.METHOD_NAME, uri, search, Collections.emptyMap());
StatusLine statusLine = response.getStatusLine();
if (statusLine.getStatusCode() == HttpStatus.SC_OK) {
String content = getContent(response);
diff --git a/java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java b/java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java
index 05fd7a7..65d2916 100644
--- a/java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java
+++ b/java/com/google/gerrit/elasticsearch/ElasticQueryAdapter.java
@@ -40,6 +40,7 @@
case V6_2:
case V6_3:
case V6_4:
+ case V6_5:
this.searchFilteringName = "_source";
this.indicesExistParam = "?allow_no_indices=false";
this.exactFieldType = "keyword";
diff --git a/java/com/google/gerrit/elasticsearch/ElasticVersion.java b/java/com/google/gerrit/elasticsearch/ElasticVersion.java
index dfa5d21..4c98df1 100644
--- a/java/com/google/gerrit/elasticsearch/ElasticVersion.java
+++ b/java/com/google/gerrit/elasticsearch/ElasticVersion.java
@@ -22,7 +22,8 @@
V5_6("5.6.*"),
V6_2("6.2.*"),
V6_3("6.3.*"),
- V6_4("6.4.*");
+ V6_4("6.4.*"),
+ V6_5("6.5.*");
private final String version;
private final Pattern pattern;
diff --git a/java/com/google/gerrit/pgm/Init.java b/java/com/google/gerrit/pgm/Init.java
index 1739de9..4ea31da 100644
--- a/java/com/google/gerrit/pgm/Init.java
+++ b/java/com/google/gerrit/pgm/Init.java
@@ -245,7 +245,7 @@
}
private void verifyInstallPluginList(ConsoleUI ui, List<PluginData> plugins) {
- if (nullOrEmpty(installPlugins) || nullOrEmpty(plugins)) {
+ if (nullOrEmpty(installPlugins)) {
return;
}
Set<String> missing = Sets.newHashSet(installPlugins);
diff --git a/java/com/google/gerrit/server/AccessPath.java b/java/com/google/gerrit/server/AccessPath.java
index cb720c8..4d07d62 100644
--- a/java/com/google/gerrit/server/AccessPath.java
+++ b/java/com/google/gerrit/server/AccessPath.java
@@ -22,9 +22,6 @@
/** Access through the REST API. */
REST_API,
- /** Access through the old JSON-RPC interface. */
- JSON_RPC,
-
/** Access by a web cookie. This path is not protected like REST_API. */
WEB_BROWSER,
diff --git a/java/com/google/gerrit/server/DynamicOptions.java b/java/com/google/gerrit/server/DynamicOptions.java
index 3759f09..dc5a262 100644
--- a/java/com/google/gerrit/server/DynamicOptions.java
+++ b/java/com/google/gerrit/server/DynamicOptions.java
@@ -50,8 +50,18 @@
* }
* </pre>
*
- * The option will be prefixed by the plugin name. In the example above, if the plugin name was
+ * <p>The option will be prefixed by the plugin name. In the example above, if the plugin name was
* my-plugin, then the --verbose option as used by the caller would be --my-plugin--verbose.
+ *
+ * <p>Additional options can be annotated with @RequiresOption which will cause them to be ignored
+ * unless the required option is present. For example:
+ *
+ * <pre>
+ * {@literal @}RequiresOptions("--help")
+ * {@literal @}Option(name = "--help-as-json",
+ * usage = "display help text in json format")
+ * public boolean displayHelpAsJson;
+ * </pre>
*/
public interface DynamicBean {}
@@ -261,6 +271,7 @@
for (Entry<String, DynamicBean> e : beansByPlugin.entrySet()) {
clp.parseWithPrefix("--" + e.getKey(), e.getValue());
}
+ clp.drainOptionQueue();
}
public void setDynamicBeans() {
diff --git a/java/com/google/gerrit/server/audit/RpcAuditEvent.java b/java/com/google/gerrit/server/audit/RpcAuditEvent.java
deleted file mode 100644
index 6c53bb2..0000000
--- a/java/com/google/gerrit/server/audit/RpcAuditEvent.java
+++ /dev/null
@@ -1,47 +0,0 @@
-// Copyright (C) 2013 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.audit;
-
-import com.google.common.collect.ListMultimap;
-import com.google.gerrit.server.CurrentUser;
-
-public class RpcAuditEvent extends HttpAuditEvent {
-
- /**
- * Creates a new audit event with results
- *
- * @param sessionId session id the event belongs to
- * @param who principal that has generated the event
- * @param what object of the event
- * @param when time-stamp of when the event started
- * @param params parameters of the event
- * @param httpMethod HTTP method
- * @param input input
- * @param status HTTP status
- * @param result result of the event
- */
- public RpcAuditEvent(
- String sessionId,
- CurrentUser who,
- String what,
- long when,
- ListMultimap<String, ?> params,
- String httpMethod,
- Object input,
- int status,
- Object result) {
- super(sessionId, who, what, when, params, httpMethod, input, status, result);
- }
-}
diff --git a/java/com/google/gerrit/server/notedb/IntBlob.java b/java/com/google/gerrit/server/notedb/IntBlob.java
index 9bac2a4..f8c713c 100644
--- a/java/com/google/gerrit/server/notedb/IntBlob.java
+++ b/java/com/google/gerrit/server/notedb/IntBlob.java
@@ -18,6 +18,7 @@
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
@@ -28,6 +29,7 @@
import java.io.IOException;
import java.util.Optional;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectLoader;
@@ -116,8 +118,9 @@
return result == RefUpdate.Result.NEW || result == RefUpdate.Result.FORCED;
}
- private static IntBlob create(ObjectId id, int value) {
- return new AutoValue_IntBlob(id, value);
+ @VisibleForTesting
+ static IntBlob create(AnyObjectId id, int value) {
+ return new AutoValue_IntBlob(id.copy(), value);
}
public abstract ObjectId id();
diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java
index 74b04a3..83ea7f8 100644
--- a/java/com/google/gerrit/server/permissions/RefControl.java
+++ b/java/com/google/gerrit/server/permissions/RefControl.java
@@ -197,7 +197,6 @@
case GIT:
return false;
- case JSON_RPC:
case REST_API:
case SSH_COMMAND:
case UNKNOWN:
@@ -229,7 +228,6 @@
case GIT:
return canPushWithForce() || canPerform(Permission.DELETE);
- case JSON_RPC:
case REST_API:
case SSH_COMMAND:
case UNKNOWN:
diff --git a/java/com/google/gerrit/server/project/ProjectCreator.java b/java/com/google/gerrit/server/project/ProjectCreator.java
new file mode 100644
index 0000000..b50b046
--- /dev/null
+++ b/java/com/google/gerrit/server/project/ProjectCreator.java
@@ -0,0 +1,255 @@
+// 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.project;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.data.AccessSection;
+import com.google.gerrit.common.data.GroupDescription;
+import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.common.data.PermissionRule;
+import com.google.gerrit.extensions.events.NewProjectCreatedListener;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.config.RepositoryConfig;
+import com.google.gerrit.server.extensions.events.AbstractNoNotifyEvent;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.RepositoryCaseMismatchException;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.plugincontext.PluginSetContext;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.io.IOException;
+import java.util.List;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.RefUpdate.Result;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+public class ProjectCreator {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final GitRepositoryManager repoManager;
+ private final PluginSetContext<NewProjectCreatedListener> createdListeners;
+ private final ProjectCache projectCache;
+ private final GroupBackend groupBackend;
+ private final MetaDataUpdate.User metaDataUpdateFactory;
+ private final GitReferenceUpdated referenceUpdated;
+ private final RepositoryConfig repositoryCfg;
+ private final Provider<PersonIdent> serverIdent;
+ private final Provider<IdentifiedUser> identifiedUser;
+ private final ProjectConfig.Factory projectConfigFactory;
+
+ @Inject
+ ProjectCreator(
+ GitRepositoryManager repoManager,
+ PluginSetContext<NewProjectCreatedListener> createdListeners,
+ ProjectCache projectCache,
+ GroupBackend groupBackend,
+ MetaDataUpdate.User metaDataUpdateFactory,
+ GitReferenceUpdated referenceUpdated,
+ RepositoryConfig repositoryCfg,
+ @GerritPersonIdent Provider<PersonIdent> serverIdent,
+ Provider<IdentifiedUser> identifiedUser,
+ ProjectConfig.Factory projectConfigFactory) {
+ this.repoManager = repoManager;
+ this.createdListeners = createdListeners;
+ this.projectCache = projectCache;
+ this.groupBackend = groupBackend;
+ this.metaDataUpdateFactory = metaDataUpdateFactory;
+ this.referenceUpdated = referenceUpdated;
+ this.repositoryCfg = repositoryCfg;
+ this.serverIdent = serverIdent;
+ this.identifiedUser = identifiedUser;
+ this.projectConfigFactory = projectConfigFactory;
+ }
+
+ public ProjectState createProject(CreateProjectArgs args)
+ throws BadRequestException, ResourceConflictException, IOException, ConfigInvalidException {
+ final Project.NameKey nameKey = args.getProject();
+ try {
+ final String head = args.permissionsOnly ? RefNames.REFS_CONFIG : args.branch.get(0);
+ try (Repository repo = repoManager.openRepository(nameKey)) {
+ if (repo.getObjectDatabase().exists()) {
+ throw new ResourceConflictException("project \"" + nameKey + "\" exists");
+ }
+ } catch (RepositoryNotFoundException e) {
+ // It does not exist, safe to ignore.
+ }
+ try (Repository repo = repoManager.createRepository(nameKey)) {
+ RefUpdate u = repo.updateRef(Constants.HEAD);
+ u.disableRefLog();
+ u.link(head);
+
+ createProjectConfig(args);
+
+ if (!args.permissionsOnly && args.createEmptyCommit) {
+ createEmptyCommits(repo, nameKey, args.branch);
+ }
+
+ fire(nameKey, head);
+
+ return projectCache.get(nameKey);
+ }
+ } catch (RepositoryCaseMismatchException e) {
+ throw new ResourceConflictException(
+ "Cannot create "
+ + nameKey.get()
+ + " because the name is already occupied by another project."
+ + " The other project has the same name, only spelled in a"
+ + " different case.");
+ } catch (RepositoryNotFoundException badName) {
+ throw new BadRequestException("invalid project name: " + nameKey);
+ } catch (ConfigInvalidException e) {
+ String msg = "Cannot create " + nameKey;
+ logger.atSevere().withCause(e).log(msg);
+ throw e;
+ }
+ }
+
+ private void createProjectConfig(CreateProjectArgs args)
+ throws IOException, ConfigInvalidException {
+ try (MetaDataUpdate md = metaDataUpdateFactory.create(args.getProject())) {
+ ProjectConfig config = projectConfigFactory.read(md);
+
+ Project newProject = config.getProject();
+ newProject.setDescription(args.projectDescription);
+ newProject.setSubmitType(
+ MoreObjects.firstNonNull(
+ args.submitType, repositoryCfg.getDefaultSubmitType(args.getProject())));
+ newProject.setBooleanConfig(
+ BooleanProjectConfig.USE_CONTRIBUTOR_AGREEMENTS, args.contributorAgreements);
+ newProject.setBooleanConfig(BooleanProjectConfig.USE_SIGNED_OFF_BY, args.signedOffBy);
+ newProject.setBooleanConfig(BooleanProjectConfig.USE_CONTENT_MERGE, args.contentMerge);
+ newProject.setBooleanConfig(
+ BooleanProjectConfig.CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
+ args.newChangeForAllNotInTarget);
+ newProject.setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, args.changeIdRequired);
+ newProject.setBooleanConfig(BooleanProjectConfig.REJECT_EMPTY_COMMIT, args.rejectEmptyCommit);
+ newProject.setMaxObjectSizeLimit(args.maxObjectSizeLimit);
+ newProject.setBooleanConfig(BooleanProjectConfig.ENABLE_SIGNED_PUSH, args.enableSignedPush);
+ newProject.setBooleanConfig(BooleanProjectConfig.REQUIRE_SIGNED_PUSH, args.requireSignedPush);
+ if (args.newParent != null) {
+ newProject.setParentName(args.newParent);
+ }
+
+ if (!args.ownerIds.isEmpty()) {
+ AccessSection all = config.getAccessSection(AccessSection.ALL, true);
+ for (AccountGroup.UUID ownerId : args.ownerIds) {
+ GroupDescription.Basic g = groupBackend.get(ownerId);
+ if (g != null) {
+ GroupReference group = config.resolve(GroupReference.forGroup(g));
+ all.getPermission(Permission.OWNER, true).add(new PermissionRule(group));
+ }
+ }
+ }
+
+ md.setMessage("Created project\n");
+ config.commit(md);
+ md.getRepository().setGitwebDescription(args.projectDescription);
+ }
+ projectCache.onCreateProject(args.getProject());
+ }
+
+ private void createEmptyCommits(Repository repo, Project.NameKey project, List<String> refs)
+ throws IOException {
+ try (ObjectInserter oi = repo.newObjectInserter()) {
+ CommitBuilder cb = new CommitBuilder();
+ cb.setTreeId(oi.insert(Constants.OBJ_TREE, new byte[] {}));
+ cb.setAuthor(metaDataUpdateFactory.getUserPersonIdent());
+ cb.setCommitter(serverIdent.get());
+ cb.setMessage("Initial empty repository\n");
+
+ ObjectId id = oi.insert(cb);
+ oi.flush();
+
+ for (String ref : refs) {
+ RefUpdate ru = repo.updateRef(ref);
+ ru.setNewObjectId(id);
+ Result result = ru.update();
+ switch (result) {
+ case NEW:
+ referenceUpdated.fire(
+ project, ru, ReceiveCommand.Type.CREATE, identifiedUser.get().state());
+ break;
+ case FAST_FORWARD:
+ case FORCED:
+ case IO_FAILURE:
+ case LOCK_FAILURE:
+ case NOT_ATTEMPTED:
+ case NO_CHANGE:
+ case REJECTED:
+ case REJECTED_CURRENT_BRANCH:
+ case RENAMED:
+ case REJECTED_MISSING_OBJECT:
+ case REJECTED_OTHER_REASON:
+ default:
+ {
+ throw new IOException(
+ String.format("Failed to create ref \"%s\": %s", ref, result.name()));
+ }
+ }
+ }
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log("Cannot create empty commit for %s", project.get());
+ throw e;
+ }
+ }
+
+ private void fire(Project.NameKey name, String head) {
+ if (createdListeners.isEmpty()) {
+ return;
+ }
+
+ ProjectCreator.Event event = new ProjectCreator.Event(name, head);
+ createdListeners.runEach(l -> l.onNewProjectCreated(event));
+ }
+
+ static class Event extends AbstractNoNotifyEvent implements NewProjectCreatedListener.Event {
+ private final Project.NameKey name;
+ private final String head;
+
+ Event(Project.NameKey name, String head) {
+ this.name = name;
+ this.head = head;
+ }
+
+ @Override
+ public String getProjectName() {
+ return name.get();
+ }
+
+ @Override
+ public String getHeadName() {
+ return head;
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/project/CreateProject.java b/java/com/google/gerrit/server/restapi/project/CreateProject.java
index 60a24d8..3785784 100644
--- a/java/com/google/gerrit/server/restapi/project/CreateProject.java
+++ b/java/com/google/gerrit/server/restapi/project/CreateProject.java
@@ -19,21 +19,14 @@
import com.google.common.base.MoreObjects;
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.ProjectUtil;
-import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
-import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupReference;
-import com.google.gerrit.common.data.Permission;
-import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.annotations.RequiresCapability;
import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.api.projects.ProjectInput;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ProjectInfo;
-import com.google.gerrit.extensions.events.NewProjectCreatedListener;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -41,29 +34,17 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestCollectionCreateView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
-import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
-import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.server.GerritPersonIdent;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.ProjectOwnerGroupsProvider;
-import com.google.gerrit.server.config.RepositoryConfig;
-import com.google.gerrit.server.extensions.events.AbstractNoNotifyEvent;
-import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
-import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.RepositoryCaseMismatchException;
-import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.group.GroupResolver;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.plugincontext.PluginItemContext;
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.project.CreateProjectArgs;
-import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectConfig;
+import com.google.gerrit.server.project.ProjectCreator;
import com.google.gerrit.server.project.ProjectJson;
import com.google.gerrit.server.project.ProjectNameLockManager;
import com.google.gerrit.server.project.ProjectResource;
@@ -79,84 +60,47 @@
import java.util.List;
import java.util.concurrent.locks.Lock;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Constants;
-import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.PersonIdent;
-import org.eclipse.jgit.lib.RefUpdate;
-import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.transport.ReceiveCommand;
@RequiresCapability(GlobalCapability.CREATE_PROJECT)
@Singleton
public class CreateProject
implements RestCollectionCreateView<TopLevelResource, ProjectResource, ProjectInput> {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final Provider<ProjectsCollection> projectsCollection;
private final Provider<GroupResolver> groupResolver;
private final PluginSetContext<ProjectCreationValidationListener>
projectCreationValidationListeners;
private final ProjectJson json;
- private final GitRepositoryManager repoManager;
- private final PluginSetContext<NewProjectCreatedListener> createdListeners;
- private final ProjectCache projectCache;
- private final GroupBackend groupBackend;
private final ProjectOwnerGroupsProvider.Factory projectOwnerGroups;
- private final MetaDataUpdate.User metaDataUpdateFactory;
- private final GitReferenceUpdated referenceUpdated;
- private final RepositoryConfig repositoryCfg;
- private final Provider<PersonIdent> serverIdent;
- private final Provider<IdentifiedUser> identifiedUser;
private final Provider<PutConfig> putConfig;
private final AllProjectsName allProjects;
private final AllUsersName allUsers;
private final PluginItemContext<ProjectNameLockManager> lockManager;
- private final ProjectConfig.Factory projectConfigFactory;
+ private final ProjectCreator projectCreator;
@Inject
CreateProject(
+ ProjectCreator projectCreator,
Provider<ProjectsCollection> projectsCollection,
Provider<GroupResolver> groupResolver,
ProjectJson json,
PluginSetContext<ProjectCreationValidationListener> projectCreationValidationListeners,
- GitRepositoryManager repoManager,
- PluginSetContext<NewProjectCreatedListener> createdListeners,
- ProjectCache projectCache,
- GroupBackend groupBackend,
ProjectOwnerGroupsProvider.Factory projectOwnerGroups,
- MetaDataUpdate.User metaDataUpdateFactory,
- GitReferenceUpdated referenceUpdated,
- RepositoryConfig repositoryCfg,
- @GerritPersonIdent Provider<PersonIdent> serverIdent,
- Provider<IdentifiedUser> identifiedUser,
Provider<PutConfig> putConfig,
AllProjectsName allProjects,
AllUsersName allUsers,
- PluginItemContext<ProjectNameLockManager> lockManager,
- ProjectConfig.Factory projectConfigFactory) {
+ PluginItemContext<ProjectNameLockManager> lockManager) {
this.projectsCollection = projectsCollection;
+ this.projectCreator = projectCreator;
this.groupResolver = groupResolver;
this.projectCreationValidationListeners = projectCreationValidationListeners;
this.json = json;
- this.repoManager = repoManager;
- this.createdListeners = createdListeners;
- this.projectCache = projectCache;
- this.groupBackend = groupBackend;
this.projectOwnerGroups = projectOwnerGroups;
- this.metaDataUpdateFactory = metaDataUpdateFactory;
- this.referenceUpdated = referenceUpdated;
- this.repositoryCfg = repositoryCfg;
- this.serverIdent = serverIdent;
- this.identifiedUser = identifiedUser;
this.putConfig = putConfig;
this.allProjects = allProjects;
this.allUsers = allUsers;
this.lockManager = lockManager;
- this.projectConfigFactory = projectConfigFactory;
}
@Override
@@ -227,7 +171,7 @@
throw new ResourceConflictException(e.getMessage(), e);
}
- ProjectState projectState = createProject(args);
+ ProjectState projectState = projectCreator.createProject(args);
requireNonNull(
projectState,
() -> String.format("failed to create project %s", args.getProject().get()));
@@ -243,93 +187,6 @@
}
}
- private ProjectState createProject(CreateProjectArgs args)
- throws BadRequestException, ResourceConflictException, IOException, ConfigInvalidException {
- final Project.NameKey nameKey = args.getProject();
- try {
- final String head = args.permissionsOnly ? RefNames.REFS_CONFIG : args.branch.get(0);
- try (Repository repo = repoManager.openRepository(nameKey)) {
- if (repo.getObjectDatabase().exists()) {
- throw new ResourceConflictException("project \"" + nameKey + "\" exists");
- }
- } catch (RepositoryNotFoundException e) {
- // It does not exist, safe to ignore.
- }
- try (Repository repo = repoManager.createRepository(nameKey)) {
- RefUpdate u = repo.updateRef(Constants.HEAD);
- u.disableRefLog();
- u.link(head);
-
- createProjectConfig(args);
-
- if (!args.permissionsOnly && args.createEmptyCommit) {
- createEmptyCommits(repo, nameKey, args.branch);
- }
-
- fire(nameKey, head);
-
- return projectCache.get(nameKey);
- }
- } catch (RepositoryCaseMismatchException e) {
- throw new ResourceConflictException(
- "Cannot create "
- + nameKey.get()
- + " because the name is already occupied by another project."
- + " The other project has the same name, only spelled in a"
- + " different case.");
- } catch (RepositoryNotFoundException badName) {
- throw new BadRequestException("invalid project name: " + nameKey);
- } catch (ConfigInvalidException e) {
- String msg = "Cannot create " + nameKey;
- logger.atSevere().withCause(e).log(msg);
- throw e;
- }
- }
-
- private void createProjectConfig(CreateProjectArgs args)
- throws IOException, ConfigInvalidException {
- try (MetaDataUpdate md = metaDataUpdateFactory.create(args.getProject())) {
- ProjectConfig config = projectConfigFactory.read(md);
-
- Project newProject = config.getProject();
- newProject.setDescription(args.projectDescription);
- newProject.setSubmitType(
- MoreObjects.firstNonNull(
- args.submitType, repositoryCfg.getDefaultSubmitType(args.getProject())));
- newProject.setBooleanConfig(
- BooleanProjectConfig.USE_CONTRIBUTOR_AGREEMENTS, args.contributorAgreements);
- newProject.setBooleanConfig(BooleanProjectConfig.USE_SIGNED_OFF_BY, args.signedOffBy);
- newProject.setBooleanConfig(BooleanProjectConfig.USE_CONTENT_MERGE, args.contentMerge);
- newProject.setBooleanConfig(
- BooleanProjectConfig.CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
- args.newChangeForAllNotInTarget);
- newProject.setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, args.changeIdRequired);
- newProject.setBooleanConfig(BooleanProjectConfig.REJECT_EMPTY_COMMIT, args.rejectEmptyCommit);
- newProject.setMaxObjectSizeLimit(args.maxObjectSizeLimit);
- newProject.setBooleanConfig(BooleanProjectConfig.ENABLE_SIGNED_PUSH, args.enableSignedPush);
- newProject.setBooleanConfig(BooleanProjectConfig.REQUIRE_SIGNED_PUSH, args.requireSignedPush);
- if (args.newParent != null) {
- newProject.setParentName(args.newParent);
- }
-
- if (!args.ownerIds.isEmpty()) {
- AccessSection all = config.getAccessSection(AccessSection.ALL, true);
- for (AccountGroup.UUID ownerId : args.ownerIds) {
- GroupDescription.Basic g = groupBackend.get(ownerId);
- if (g != null) {
- GroupReference group = config.resolve(GroupReference.forGroup(g));
- all.getPermission(Permission.OWNER, true).add(new PermissionRule(group));
- }
- }
- }
-
- md.setMessage("Created project\n");
- config.commit(md);
- md.getRepository().setGitwebDescription(args.projectDescription);
- }
- projectCache.onCreateProject(args.getProject());
- }
-
private List<String> normalizeBranchNames(List<String> branches) throws BadRequestException {
if (branches == null || branches.isEmpty()) {
return Collections.singletonList(Constants.R_HEADS + Constants.MASTER);
@@ -350,78 +207,4 @@
}
return normalizedBranches;
}
-
- private void createEmptyCommits(Repository repo, Project.NameKey project, List<String> refs)
- throws IOException {
- try (ObjectInserter oi = repo.newObjectInserter()) {
- CommitBuilder cb = new CommitBuilder();
- cb.setTreeId(oi.insert(Constants.OBJ_TREE, new byte[] {}));
- cb.setAuthor(metaDataUpdateFactory.getUserPersonIdent());
- cb.setCommitter(serverIdent.get());
- cb.setMessage("Initial empty repository\n");
-
- ObjectId id = oi.insert(cb);
- oi.flush();
-
- for (String ref : refs) {
- RefUpdate ru = repo.updateRef(ref);
- ru.setNewObjectId(id);
- Result result = ru.update();
- switch (result) {
- case NEW:
- referenceUpdated.fire(
- project, ru, ReceiveCommand.Type.CREATE, identifiedUser.get().state());
- break;
- case FAST_FORWARD:
- case FORCED:
- case IO_FAILURE:
- case LOCK_FAILURE:
- case NOT_ATTEMPTED:
- case NO_CHANGE:
- case REJECTED:
- case REJECTED_CURRENT_BRANCH:
- case RENAMED:
- case REJECTED_MISSING_OBJECT:
- case REJECTED_OTHER_REASON:
- default:
- {
- throw new IOException(
- String.format("Failed to create ref \"%s\": %s", ref, result.name()));
- }
- }
- }
- } catch (IOException e) {
- logger.atSevere().withCause(e).log("Cannot create empty commit for %s", project.get());
- throw e;
- }
- }
-
- private void fire(Project.NameKey name, String head) {
- if (createdListeners.isEmpty()) {
- return;
- }
-
- Event event = new Event(name, head);
- createdListeners.runEach(l -> l.onNewProjectCreated(event));
- }
-
- static class Event extends AbstractNoNotifyEvent implements NewProjectCreatedListener.Event {
- private final Project.NameKey name;
- private final String head;
-
- Event(Project.NameKey name, String head) {
- this.name = name;
- this.head = head;
- }
-
- @Override
- public String getProjectName() {
- return name.get();
- }
-
- @Override
- public String getHeadName() {
- return head;
- }
- }
}
diff --git a/java/com/google/gerrit/server/schema/Schema_151.java b/java/com/google/gerrit/server/schema/Schema_151.java
index d244018..0e8700f 100644
--- a/java/com/google/gerrit/server/schema/Schema_151.java
+++ b/java/com/google/gerrit/server/schema/Schema_151.java
@@ -60,7 +60,7 @@
PreparedStatement addedOnRetrieval, AccountGroup.Id groupId) throws SQLException {
addedOnRetrieval.setInt(1, groupId.get());
try (ResultSet resultSet = addedOnRetrieval.executeQuery()) {
- if (resultSet.first()) {
+ if (resultSet.next()) {
return Optional.of(resultSet.getTimestamp(1));
}
}
diff --git a/java/com/google/gerrit/server/schema/Schema_87.java b/java/com/google/gerrit/server/schema/Schema_87.java
index fe21565..79884ba 100644
--- a/java/com/google/gerrit/server/schema/Schema_87.java
+++ b/java/com/google/gerrit/server/schema/Schema_87.java
@@ -59,7 +59,7 @@
PreparedStatement uuidRetrieval, AccountGroup.Id id) throws SQLException {
uuidRetrieval.setInt(1, id.get());
try (ResultSet uuidResults = uuidRetrieval.executeQuery()) {
- if (uuidResults.first()) {
+ if (uuidResults.next()) {
Optional.of(new AccountGroup.UUID(uuidResults.getString(1)));
}
}
diff --git a/java/com/google/gerrit/util/cli/CmdLineParser.java b/java/com/google/gerrit/util/cli/CmdLineParser.java
index 5b7ea3f..13447c6 100644
--- a/java/com/google/gerrit/util/cli/CmdLineParser.java
+++ b/java/com/google/gerrit/util/cli/CmdLineParser.java
@@ -50,8 +50,11 @@
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.ResourceBundle;
@@ -85,6 +88,90 @@
CmdLineParser create(Object bean);
}
+ /**
+ * This may be used by an option handler during parsing to "call" additional parameters simulating
+ * as if they had been passed from the command line originally.
+ *
+ * <p>To call additional parameters from within an option handler, instantiate this class with the
+ * parameters and then call callParameters() with the additional parameters to be parsed.
+ * OptionHandlers may optionally pass this class to other methods which may then both
+ * parse/consume more parameters and call additional parameters.
+ */
+ public static class Parameters implements org.kohsuke.args4j.spi.Parameters {
+ protected final String[] args;
+ protected MyParser parser;
+ protected int consumed = 0;
+
+ public Parameters(org.kohsuke.args4j.spi.Parameters args, MyParser parser)
+ throws CmdLineException {
+ this.args = new String[args.size()];
+ for (int i = 0; i < args.size(); i++) {
+ this.args[i] = args.getParameter(i);
+ }
+ this.parser = parser;
+ }
+
+ public Parameters(String[] args, MyParser parser) {
+ this.args = args;
+ this.parser = parser;
+ }
+
+ @Override
+ public String getParameter(int idx) throws CmdLineException {
+ return args[idx];
+ }
+
+ /**
+ * get and consume (consider parsed) a parameter
+ *
+ * @param name name
+ * @return the consumed parameter
+ */
+ public String consumeParameter() throws CmdLineException {
+ return getParameter(consumed++);
+ }
+
+ @Override
+ public int size() {
+ return args.length;
+ }
+
+ /**
+ * Add 'count' to the value of parsed parameters. May be called more than once.
+ *
+ * @param count How many parameters were just parsed.
+ */
+ public void consume(int count) {
+ consumed += count;
+ }
+
+ /**
+ * Reports handlers how many parameters were parsed
+ *
+ * @return the count of parsed parameters
+ */
+ public int getConsumed() {
+ return consumed;
+ }
+
+ /**
+ * Use during parsing to call additional parameters simulating as if they had been passed from
+ * the command line originally.
+ *
+ * @param String... args A variable amount of parameters to call immediately
+ * <p>The parameters will be parsed immediately, before the remaining parameter will be
+ * parsed.
+ * <p>Note: Since this is done outside of the arg4j parsing loop, it will not match exactly
+ * what would happen if they were actually passed from the command line, but it will be
+ * pretty close. If this were moved to args4j, the interface could be the same and it could
+ * match exactly the behavior as if passed from the command line originally.
+ */
+ public void callParameters(String... args) throws CmdLineException {
+ Parameters impl = new Parameters(Arrays.copyOfRange(args, 1, args.length), parser);
+ parser.findOptionByName(args[0]).parseArguments(impl);
+ }
+ }
+
private final OptionHandlers handlers;
private final MyParser parser;
@@ -270,6 +357,10 @@
parser.parseWithPrefix(prefix, bean);
}
+ public void drainOptionQueue() {
+ parser.addOptionsWithMetRequirements();
+ }
+
private String makeOption(String name) {
if (!name.startsWith("-")) {
if (name.length() == 1) {
@@ -404,18 +495,58 @@
}
}
- private class MyParser extends org.kohsuke.args4j.CmdLineParser {
+ public class MyParser extends org.kohsuke.args4j.CmdLineParser {
@SuppressWarnings("rawtypes")
private List<OptionHandler> optionsList;
+ private Map<String, QueuedOption> queuedOptionsByName = new LinkedHashMap<>();
private HelpOption help;
+ private class QueuedOption {
+ public final Option option;
+ public final Setter setter;
+ public final String[] requiredOptions;
+
+ private QueuedOption(Option option, Setter setter, RequiresOptions requiresOptions) {
+ this.option = option;
+ this.setter = setter;
+ this.requiredOptions = requiresOptions != null ? requiresOptions.value() : new String[0];
+ }
+ }
+
MyParser(Object bean) {
super(bean, ParserProperties.defaults().withAtSyntax(false));
parseAdditionalOptions(bean, new HashSet<>());
+ addOptionsWithMetRequirements();
ensureOptionsInitialized();
}
+ public int addOptionsWithMetRequirements() {
+ int count = 0;
+ for (Iterator<Map.Entry<String, QueuedOption>> it = queuedOptionsByName.entrySet().iterator();
+ it.hasNext(); ) {
+ QueuedOption queuedOption = it.next().getValue();
+ if (hasAllRequiredOptions(queuedOption)) {
+ addOption(queuedOption.setter, queuedOption.option);
+ it.remove();
+ count++;
+ }
+ }
+ if (count > 0) {
+ count += addOptionsWithMetRequirements();
+ }
+ return count;
+ }
+
+ private boolean hasAllRequiredOptions(QueuedOption queuedOption) {
+ for (String name : queuedOption.requiredOptions) {
+ if (findOptionByName(name) == null) {
+ return false;
+ }
+ }
+ return true;
+ }
+
// NOTE: Argument annotations on bean are ignored.
public void parseWithPrefix(String prefix, Object bean) {
parseWithPrefix(prefix, bean, new HashSet<>());
@@ -430,13 +561,19 @@
for (Method m : c.getDeclaredMethods()) {
Option o = m.getAnnotation(Option.class);
if (o != null) {
- addOption(new MethodSetter(this, bean, m), new PrefixedOption(prefix, o));
+ queueOption(
+ new PrefixedOption(prefix, o),
+ new MethodSetter(this, bean, m),
+ m.getAnnotation(RequiresOptions.class));
}
}
for (Field f : c.getDeclaredFields()) {
Option o = f.getAnnotation(Option.class);
if (o != null) {
- addOption(Setters.create(f, bean), new PrefixedOption(prefix, o));
+ queueOption(
+ new PrefixedOption(prefix, o),
+ Setters.create(f, bean),
+ f.getAnnotation(RequiresOptions.class));
}
if (f.isAnnotationPresent(Options.class)) {
try {
@@ -480,6 +617,37 @@
return add(super.createOptionHandler(option, setter));
}
+ /**
+ * Finds a registered {@code OptionHandler} by its name or its alias.
+ *
+ * @param name name
+ * @return the {@code OptionHandler} or {@code null}
+ * <p>Note: this is cut & pasted from the parent class in arg4j, it was private and it
+ * needed to be exposed.
+ */
+ public OptionHandler findOptionByName(String name) {
+ for (OptionHandler h : optionsList) {
+ NamedOptionDef option = (NamedOptionDef) h.option;
+ if (name.equals(option.name())) {
+ return h;
+ }
+ for (String alias : option.aliases()) {
+ if (name.equals(alias)) {
+ return h;
+ }
+ }
+ }
+ return null;
+ }
+
+ private void queueOption(Option option, Setter setter, RequiresOptions requiresOptions) {
+ if (queuedOptionsByName.put(option.name(), new QueuedOption(option, setter, requiresOptions))
+ != null) {
+ throw new IllegalAnnotationError(
+ "Option name " + option.name() + " is used more than once");
+ }
+ }
+
@SuppressWarnings("rawtypes")
private OptionHandler add(OptionHandler handler) {
ensureOptionsInitialized();
diff --git a/java/com/google/gerrit/util/cli/RequiresOptions.java b/java/com/google/gerrit/util/cli/RequiresOptions.java
new file mode 100644
index 0000000..de6ba44
--- /dev/null
+++ b/java/com/google/gerrit/util/cli/RequiresOptions.java
@@ -0,0 +1,45 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.util.cli;
+
+import static java.lang.annotation.ElementType.FIELD;
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+/**
+ * Marks a field/setter annotated with {@literal @}Option as having a dependency on multiple other
+ * command line option.
+ *
+ * <p>If any of the required command line options are not present, the {@literal @}Option will be
+ * ignored.
+ *
+ * <p>For example:
+ *
+ * <pre>
+ * {@literal @}RequiresOptions({"--help", "--usage"})
+ * {@literal @}Option(name = "--help-as-json",
+ * usage = "display help text in json format")
+ * public boolean displayHelpAsJson;
+ * </pre>
+ */
+@Retention(RUNTIME)
+@Target({FIELD, METHOD, PARAMETER})
+public @interface RequiresOptions {
+ String[] value();
+}
diff --git a/javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java b/javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java
index 0d5d2cd..29a5bd0 100644
--- a/javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java
+++ b/javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java
@@ -14,28 +14,17 @@
package com.google.gerrit.acceptance.pgm;
-import com.google.gerrit.elasticsearch.ElasticContainer;
-import com.google.gerrit.elasticsearch.ElasticTestUtils;
-import com.google.gerrit.elasticsearch.ElasticTestUtils.ElasticNodeInfo;
+import static com.google.gerrit.elasticsearch.ElasticTestUtils.createAllIndexes;
+import static com.google.gerrit.elasticsearch.ElasticTestUtils.getConfig;
+
import com.google.gerrit.elasticsearch.ElasticVersion;
import com.google.gerrit.testing.ConfigSuite;
import com.google.inject.Injector;
-import java.util.UUID;
import org.eclipse.jgit.lib.Config;
import org.junit.Before;
public class ElasticReindexIT extends AbstractReindexTests {
- private static Config getConfig(ElasticVersion version) {
- ElasticNodeInfo elasticNodeInfo;
- ElasticContainer<?> container = ElasticContainer.createAndStart(version);
- elasticNodeInfo = new ElasticNodeInfo(container.getHttpHost().getPort());
- String indicesPrefix = UUID.randomUUID().toString();
- Config cfg = new Config();
- ElasticTestUtils.configure(cfg, elasticNodeInfo.port, indicesPrefix, version);
- return cfg;
- }
-
@ConfigSuite.Default
public static Config elasticsearchV2() {
return getConfig(ElasticVersion.V2_4);
@@ -48,12 +37,12 @@
@ConfigSuite.Config
public static Config elasticsearchV6() {
- return getConfig(ElasticVersion.V6_4);
+ return getConfig(ElasticVersion.V6_5);
}
@Override
public void configureIndex(Injector injector) throws Exception {
- ElasticTestUtils.createAllIndexes(injector);
+ createAllIndexes(injector);
}
@Before
diff --git a/javatests/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java b/javatests/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java
index 9d69955..1e60071 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java
@@ -14,27 +14,16 @@
package com.google.gerrit.acceptance.ssh;
-import com.google.gerrit.elasticsearch.ElasticContainer;
-import com.google.gerrit.elasticsearch.ElasticTestUtils;
-import com.google.gerrit.elasticsearch.ElasticTestUtils.ElasticNodeInfo;
+import static com.google.gerrit.elasticsearch.ElasticTestUtils.createAllIndexes;
+import static com.google.gerrit.elasticsearch.ElasticTestUtils.getConfig;
+
import com.google.gerrit.elasticsearch.ElasticVersion;
import com.google.gerrit.testing.ConfigSuite;
import com.google.inject.Injector;
-import java.util.UUID;
import org.eclipse.jgit.lib.Config;
public class ElasticIndexIT extends AbstractIndexTests {
- private static Config getConfig(ElasticVersion version) {
- ElasticNodeInfo elasticNodeInfo;
- ElasticContainer<?> container = ElasticContainer.createAndStart(version);
- elasticNodeInfo = new ElasticNodeInfo(container.getHttpHost().getPort());
- String indicesPrefix = UUID.randomUUID().toString();
- Config cfg = new Config();
- ElasticTestUtils.configure(cfg, elasticNodeInfo.port, indicesPrefix, version);
- return cfg;
- }
-
@ConfigSuite.Default
public static Config elasticsearchV2() {
return getConfig(ElasticVersion.V2_4);
@@ -47,11 +36,11 @@
@ConfigSuite.Config
public static Config elasticsearchV6() {
- return getConfig(ElasticVersion.V6_4);
+ return getConfig(ElasticVersion.V6_5);
}
@Override
public void configureIndex(Injector injector) throws Exception {
- ElasticTestUtils.createAllIndexes(injector);
+ createAllIndexes(injector);
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java b/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java
index 93e97c4..c3150f1 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticContainer.java
@@ -52,6 +52,8 @@
return "docker.elastic.co/elasticsearch/elasticsearch-oss:6.3.2";
case V6_4:
return "docker.elastic.co/elasticsearch/elasticsearch-oss:6.4.3";
+ case V6_5:
+ return "docker.elastic.co/elasticsearch/elasticsearch-oss:6.5.0";
}
throw new IllegalStateException("No tests for version: " + version.name());
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticTestUtils.java b/javatests/com/google/gerrit/elasticsearch/ElasticTestUtils.java
index b46e040..9f7b60c 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticTestUtils.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticTestUtils.java
@@ -21,6 +21,7 @@
import com.google.inject.TypeLiteral;
import java.io.IOException;
import java.util.Collection;
+import java.util.UUID;
import org.eclipse.jgit.lib.Config;
public final class ElasticTestUtils {
@@ -55,6 +56,16 @@
}
}
+ public static Config getConfig(ElasticVersion version) {
+ ElasticNodeInfo elasticNodeInfo;
+ ElasticContainer<?> container = ElasticContainer.createAndStart(version);
+ elasticNodeInfo = new ElasticNodeInfo(container.getHttpHost().getPort());
+ String indicesPrefix = UUID.randomUUID().toString();
+ Config cfg = new Config();
+ configure(cfg, elasticNodeInfo.port, indicesPrefix, version);
+ return cfg;
+ }
+
private ElasticTestUtils() {
// hide default constructor
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryAccountsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryAccountsTest.java
index b8154ce..eeb4c09 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryAccountsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryAccountsTest.java
@@ -41,7 +41,7 @@
return;
}
- container = ElasticContainer.createAndStart(ElasticVersion.V6_4);
+ container = ElasticContainer.createAndStart(ElasticVersion.V6_5);
nodeInfo = new ElasticNodeInfo(container.getHttpHost().getPort());
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryChangesTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryChangesTest.java
index 3445b36..7525b65 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryChangesTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryChangesTest.java
@@ -41,7 +41,7 @@
return;
}
- container = ElasticContainer.createAndStart(ElasticVersion.V6_4);
+ container = ElasticContainer.createAndStart(ElasticVersion.V6_5);
nodeInfo = new ElasticNodeInfo(container.getHttpHost().getPort());
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryGroupsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryGroupsTest.java
index 851b27d..e8d5683 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryGroupsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryGroupsTest.java
@@ -41,7 +41,7 @@
return;
}
- container = ElasticContainer.createAndStart(ElasticVersion.V6_4);
+ container = ElasticContainer.createAndStart(ElasticVersion.V6_5);
nodeInfo = new ElasticNodeInfo(container.getHttpHost().getPort());
}
diff --git a/javatests/com/google/gerrit/server/notedb/IntBlobTest.java b/javatests/com/google/gerrit/server/notedb/IntBlobTest.java
new file mode 100644
index 0000000..1abaa22
--- /dev/null
+++ b/javatests/com/google/gerrit/server/notedb/IntBlobTest.java
@@ -0,0 +1,202 @@
+// 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.notedb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
+import static com.google.gerrit.truth.OptionalSubject.assertThat;
+
+import com.google.gerrit.git.LockFailureException;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gwtorm.server.OrmException;
+import java.io.IOException;
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.junit.Before;
+import org.junit.Test;
+
+public class IntBlobTest {
+ // Note: Can't easily test GitRefUpdated behavior, since binding GitRefUpdated requires a thick
+ // stack of dependencies, and it's not just a simple interface or abstract class.
+
+ private Project.NameKey projectName;
+ private InMemoryRepository repo;
+ private TestRepository<InMemoryRepository> tr;
+ private RevWalk rw;
+
+ @Before
+ public void setUp() throws Exception {
+ projectName = new Project.NameKey("repo");
+ repo = new InMemoryRepository(new DfsRepositoryDescription(projectName.get()));
+ tr = new TestRepository<>(repo);
+ rw = tr.getRevWalk();
+ }
+
+ @Test
+ public void parseNoRef() throws Exception {
+ assertThat(IntBlob.parse(repo, "refs/nothing")).isEmpty();
+ }
+
+ @Test
+ public void parseNonBlob() throws Exception {
+ String refName = "refs/foo/master";
+ tr.branch(refName).commit().create();
+ try {
+ IntBlob.parse(repo, refName);
+ assert_().fail("Expected IncorrectObjectTypeException");
+ } catch (IncorrectObjectTypeException e) {
+ // Expected.
+ }
+ }
+
+ @Test
+ public void parseValid() throws Exception {
+ String refName = "refs/foo";
+ ObjectId id = tr.update(refName, tr.blob("123"));
+ assertThat(IntBlob.parse(repo, refName)).value().isEqualTo(IntBlob.create(id, 123));
+ }
+
+ @Test
+ public void parseWithWhitespace() throws Exception {
+ String refName = "refs/foo";
+ ObjectId id = tr.update(refName, tr.blob(" 123 "));
+ assertThat(IntBlob.parse(repo, refName)).value().isEqualTo(IntBlob.create(id, 123));
+ }
+
+ @Test
+ public void parseInvalid() throws Exception {
+ String refName = "refs/foo";
+ ObjectId id = tr.update(refName, tr.blob("1 2 3"));
+ try {
+ IntBlob.parse(repo, refName);
+ assert_().fail("Expected OrmException");
+ } catch (OrmException e) {
+ assertThat(e).hasMessageThat().isEqualTo("invalid value in refs/foo blob at " + id.name());
+ }
+ }
+
+ @Test
+ public void tryStoreNoOldId() throws Exception {
+ String refName = "refs/foo";
+ RefUpdate ru =
+ IntBlob.tryStore(repo, rw, projectName, refName, null, 123, GitReferenceUpdated.DISABLED);
+ assertThat(ru.getResult()).isEqualTo(RefUpdate.Result.NEW);
+ assertThat(ru.getName()).isEqualTo(refName);
+ assertThat(IntBlob.parse(repo, refName))
+ .value()
+ .isEqualTo(IntBlob.create(ru.getNewObjectId(), 123));
+ }
+
+ @Test
+ public void tryStoreOldIdZero() throws Exception {
+ String refName = "refs/foo";
+ RefUpdate ru =
+ IntBlob.tryStore(
+ repo, rw, projectName, refName, ObjectId.zeroId(), 123, GitReferenceUpdated.DISABLED);
+ assertThat(ru.getResult()).isEqualTo(RefUpdate.Result.NEW);
+ assertThat(ru.getName()).isEqualTo(refName);
+ assertThat(IntBlob.parse(repo, refName))
+ .value()
+ .isEqualTo(IntBlob.create(ru.getNewObjectId(), 123));
+ }
+
+ @Test
+ public void tryStoreCorrectOldId() throws Exception {
+ String refName = "refs/foo";
+ ObjectId id = tr.update(refName, tr.blob("123"));
+ RefUpdate ru =
+ IntBlob.tryStore(repo, rw, projectName, refName, id, 456, GitReferenceUpdated.DISABLED);
+ assertThat(ru.getResult()).isEqualTo(RefUpdate.Result.FORCED);
+ assertThat(ru.getName()).isEqualTo(refName);
+ assertThat(IntBlob.parse(repo, refName))
+ .value()
+ .isEqualTo(IntBlob.create(ru.getNewObjectId(), 456));
+ }
+
+ @Test
+ public void tryStoreWrongOldId() throws Exception {
+ String refName = "refs/foo";
+ RefUpdate ru =
+ IntBlob.tryStore(
+ repo,
+ rw,
+ projectName,
+ refName,
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
+ 123,
+ GitReferenceUpdated.DISABLED);
+ assertThat(ru.getResult()).isEqualTo(RefUpdate.Result.LOCK_FAILURE);
+ assertThat(ru.getName()).isEqualTo(refName);
+ assertThat(IntBlob.parse(repo, refName)).isEmpty();
+ }
+
+ @Test
+ public void storeNoOldId() throws Exception {
+ String refName = "refs/foo";
+ IntBlob.store(repo, rw, projectName, refName, null, 123, GitReferenceUpdated.DISABLED);
+ assertThat(IntBlob.parse(repo, refName))
+ .value()
+ .isEqualTo(IntBlob.create(getRef(refName), 123));
+ }
+
+ @Test
+ public void storeOldIdZero() throws Exception {
+ String refName = "refs/foo";
+ IntBlob.store(
+ repo, rw, projectName, refName, ObjectId.zeroId(), 123, GitReferenceUpdated.DISABLED);
+ assertThat(IntBlob.parse(repo, refName))
+ .value()
+ .isEqualTo(IntBlob.create(getRef(refName), 123));
+ }
+
+ @Test
+ public void storeCorrectOldId() throws Exception {
+ String refName = "refs/foo";
+ ObjectId id = tr.update(refName, tr.blob("123"));
+ IntBlob.store(repo, rw, projectName, refName, id, 456, GitReferenceUpdated.DISABLED);
+ assertThat(IntBlob.parse(repo, refName))
+ .value()
+ .isEqualTo(IntBlob.create(getRef(refName), 456));
+ }
+
+ @Test
+ public void storeWrongOldId() throws Exception {
+ String refName = "refs/foo";
+ try {
+ IntBlob.store(
+ repo,
+ rw,
+ projectName,
+ refName,
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
+ 123,
+ GitReferenceUpdated.DISABLED);
+ assert_().fail("expected LockFailureException");
+ } catch (LockFailureException e) {
+ assertThat(e.getFailedRefs()).containsExactly("refs/foo");
+ }
+ assertThat(IntBlob.parse(repo, refName)).isEmpty();
+ }
+
+ private ObjectId getRef(String refName) throws IOException {
+ return repo.exactRef(refName).getObjectId();
+ }
+}
diff --git a/plugins/BUILD b/plugins/BUILD
index 1ce0fba7..0feac10 100644
--- a/plugins/BUILD
+++ b/plugins/BUILD
@@ -42,6 +42,7 @@
"//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/schema",
"//java/com/google/gerrit/server/util/time",
+ "//java/com/google/gerrit/util/cli",
"//java/com/google/gerrit/util/http",
"//lib/commons:compress",
"//lib/commons:dbcp",
diff --git a/plugins/download-commands b/plugins/download-commands
index cf58d79..edd7156 160000
--- a/plugins/download-commands
+++ b/plugins/download-commands
@@ -1 +1 @@
-Subproject commit cf58d79bc034e8904aa459d8974df5796a734e1d
+Subproject commit edd715618415d9a5e03b4555d9a2d3cca8fff6e8
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index aeaee9d..6aea35b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -113,6 +113,7 @@
/** @type {!Array<!Gerrit.HoveredRange>} */
commentRanges: {
type: Array,
+ value: () => [],
},
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
index d335e7a..3eb7b37 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
@@ -37,7 +37,6 @@
commit-range="[[commitRange]]"
hidden$="[[hidden]]"
no-render-on-prefs-change="[[noRenderOnPrefsChange]]"
- comments="[[comments]]"
line-wrapping="[[lineWrapping]]"
view-mode="[[viewMode]]"
line-of-interest="[[lineOfInterest]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
index 814c7268..5839141 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
@@ -199,7 +199,7 @@
_threadEls: {
type: Array,
- value: [],
+ value: () => [],
},
},
@@ -208,7 +208,16 @@
],
listeners: {
+ // These are named inconsistently for a reason:
+ // The create-comment event is fired to indicate that we should
+ // create a comment.
+ // The comment-* events are just notifying that the comments did already
+ // change in some way, and that we should update any models we may want
+ // to keep in sync.
'create-comment': '_handleCreateComment',
+ 'comment-discard': '_handleCommentDiscard',
+ 'comment-update': '_handleCommentUpdate',
+ 'comment-save': '_handleCommentSave',
},
observers: [
@@ -723,5 +732,76 @@
this.getParentIndex(patchRangeRecord.base.basePatchNum) : null;
},
+ _handleCommentSave(e) {
+ const comment = e.detail.comment;
+ const side = e.detail.comment.__commentSide;
+ const idx = this._findDraftIndex(comment, side);
+ this.set(['comments', side, idx], comment);
+ this._handleCommentSaveOrDiscard();
+ },
+
+ _handleCommentDiscard(e) {
+ const comment = e.detail.comment;
+ this._removeComment(comment);
+ this._handleCommentSaveOrDiscard();
+ },
+
+ /**
+ * Closure annotation for Polymer.prototype.push is off. Submitted PR:
+ * https://github.com/Polymer/polymer/pull/4776
+ * but for not supressing annotations.
+ *
+ * @suppress {checkTypes}
+ */
+ _handleCommentUpdate(e) {
+ const comment = e.detail.comment;
+ const side = e.detail.comment.__commentSide;
+ let idx = this._findCommentIndex(comment, side);
+ if (idx === -1) {
+ idx = this._findDraftIndex(comment, side);
+ }
+ if (idx !== -1) { // Update draft or comment.
+ this.set(['comments', side, idx], comment);
+ } else { // Create new draft.
+ this.push(['comments', side], comment);
+ }
+ },
+
+ _handleCommentSaveOrDiscard() {
+ this.dispatchEvent(new CustomEvent('diff-comments-modified',
+ {bubbles: true}));
+ },
+
+ _removeComment(comment) {
+ const side = comment.__commentSide;
+ this._removeCommentFromSide(comment, side);
+ },
+
+ _removeCommentFromSide(comment, side) {
+ let idx = this._findCommentIndex(comment, side);
+ if (idx === -1) {
+ idx = this._findDraftIndex(comment, side);
+ }
+ if (idx !== -1) {
+ this.splice('comments.' + side, idx, 1);
+ }
+ },
+
+ /** @return {number} */
+ _findCommentIndex(comment, side) {
+ if (!comment.id || !this.comments[side]) {
+ return -1;
+ }
+ return this.comments[side].findIndex(item => item.id === comment.id);
+ },
+
+ /** @return {number} */
+ _findDraftIndex(comment, side) {
+ if (!comment.__draftID || !this.comments[side]) {
+ return -1;
+ }
+ return this.comments[side].findIndex(
+ item => item.__draftID === comment.__draftID);
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
index 423bdc6..5344256 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
@@ -46,15 +46,195 @@
async getLoggedIn() { return getLoggedIn; },
});
element = fixture('basic');
- // For reasons beyond me, fixture reuses elements, cleans out some
- // stuff but not that list.
- element._threadEls = [];
});
teardown(() => {
sandbox.restore();
});
+ suite('handle comment-update', () => {
+ setup(() => {
+ sandbox.stub(element, '_commentsChanged');
+ element.comments = {
+ meta: {
+ changeNum: '42',
+ patchRange: {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ },
+ path: '/path/to/foo',
+ projectConfig: {foo: 'bar'},
+ },
+ left: [
+ {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ ],
+ right: [
+ {id: 'c1', __commentSide: 'right'},
+ {id: 'c2', __commentSide: 'right'},
+ {id: 'd1', __draft: true, __commentSide: 'right'},
+ {id: 'd2', __draft: true, __commentSide: 'right'},
+ ],
+ };
+ });
+
+ test('creating a draft', () => {
+ const comment = {__draft: true, __draftID: 'tempID', side: 'PARENT',
+ __commentSide: 'left'};
+ element.fire('comment-update', {comment});
+ assert.include(element.comments.left, comment);
+ });
+
+ test('discarding a draft', () => {
+ const draftID = 'tempID';
+ const id = 'savedID';
+ const comment = {
+ __draft: true,
+ __draftID: draftID,
+ side: 'PARENT',
+ __commentSide: 'left',
+ };
+ const diffCommentsModifiedStub = sandbox.stub();
+ element.addEventListener('diff-comments-modified',
+ diffCommentsModifiedStub);
+ element.comments.left.push(comment);
+ comment.id = id;
+ element.fire('comment-discard', {comment});
+ const drafts = element.comments.left.filter(item => {
+ return item.__draftID === draftID;
+ });
+ assert.equal(drafts.length, 0);
+ assert.isTrue(diffCommentsModifiedStub.called);
+ });
+
+ test('saving a draft', () => {
+ const draftID = 'tempID';
+ const id = 'savedID';
+ const comment = {
+ __draft: true,
+ __draftID: draftID,
+ side: 'PARENT',
+ __commentSide: 'left',
+ };
+ const diffCommentsModifiedStub = sandbox.stub();
+ element.addEventListener('diff-comments-modified',
+ diffCommentsModifiedStub);
+ element.comments.left.push(comment);
+ comment.id = id;
+ element.fire('comment-save', {comment});
+ const drafts = element.comments.left.filter(item => {
+ return item.__draftID === draftID;
+ });
+ assert.equal(drafts.length, 1);
+ assert.equal(drafts[0].id, id);
+ assert.isTrue(diffCommentsModifiedStub.called);
+ });
+ });
+
+ test('remove comment', () => {
+ sandbox.stub(element, '_commentsChanged');
+ element.comments = {
+ meta: {
+ changeNum: '42',
+ patchRange: {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ },
+ path: '/path/to/foo',
+ projectConfig: {foo: 'bar'},
+ },
+ left: [
+ {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ ],
+ right: [
+ {id: 'c1', __commentSide: 'right'},
+ {id: 'c2', __commentSide: 'right'},
+ {id: 'd1', __draft: true, __commentSide: 'right'},
+ {id: 'd2', __draft: true, __commentSide: 'right'},
+ ],
+ };
+
+ element._removeComment({});
+ // Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem
+ // to believe that one object deepEquals another even when they do :-/.
+ assert.equal(JSON.stringify(element.comments), JSON.stringify({
+ meta: {
+ changeNum: '42',
+ patchRange: {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ },
+ path: '/path/to/foo',
+ projectConfig: {foo: 'bar'},
+ },
+ left: [
+ {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ ],
+ right: [
+ {id: 'c1', __commentSide: 'right'},
+ {id: 'c2', __commentSide: 'right'},
+ {id: 'd1', __draft: true, __commentSide: 'right'},
+ {id: 'd2', __draft: true, __commentSide: 'right'},
+ ],
+ }));
+
+ element._removeComment({id: 'bc2', side: 'PARENT',
+ __commentSide: 'left'});
+ assert.deepEqual(element.comments, {
+ meta: {
+ changeNum: '42',
+ patchRange: {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ },
+ path: '/path/to/foo',
+ projectConfig: {foo: 'bar'},
+ },
+ left: [
+ {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ ],
+ right: [
+ {id: 'c1', __commentSide: 'right'},
+ {id: 'c2', __commentSide: 'right'},
+ {id: 'd1', __draft: true, __commentSide: 'right'},
+ {id: 'd2', __draft: true, __commentSide: 'right'},
+ ],
+ });
+
+ element._removeComment({id: 'd2', __commentSide: 'right'});
+ assert.deepEqual(element.comments, {
+ meta: {
+ changeNum: '42',
+ patchRange: {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ },
+ path: '/path/to/foo',
+ projectConfig: {foo: 'bar'},
+ },
+ left: [
+ {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
+ ],
+ right: [
+ {id: 'c1', __commentSide: 'right'},
+ {id: 'c2', __commentSide: 'right'},
+ {id: 'd1', __draft: true, __commentSide: 'right'},
+ ],
+ });
+ });
+
test('thread-discard handling', () => {
const threads = [
{comments: [{id: 4711}]},
@@ -685,12 +865,6 @@
assert.equal(element.$.diff.noRenderOnPrefsChange, value);
});
- test('passes in comments', () => {
- const value = {left: [], right: []};
- element.comments = value;
- assert.equal(element.$.diff.comments, value);
- });
-
test('passes in lineWrapping', () => {
const value = true;
element.lineWrapping = value;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index d5f92f1..a8cf320 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -74,12 +74,6 @@
* @event show-auth-required
*/
- /**
- * Fired when a comment is saved or discarded
- *
- * @event diff-comments-modified
- */
-
/**
* Fired when a comment is created
*
@@ -113,14 +107,10 @@
reflectToAttribute: true,
},
noRenderOnPrefsChange: Boolean,
- comments: {
- type: Object,
- value: {left: [], right: []},
- },
/** @type {!Array<!Gerrit.HoveredRange>} */
_commentRanges: {
type: Array,
- value: [],
+ value: () => [],
},
lineWrapping: {
type: Boolean,
@@ -136,6 +126,21 @@
/** @type ?Defs.LineOfInterest */
lineOfInterest: Object,
+ /**
+ * The key locations based on the comments and line of interests,
+ * where lines should not be collapsed.
+ *
+ * @type {{left: Object<(string|number), number>,
+ * right: Object<(string|number), number>}}
+ */
+ _keyLocations: {
+ type: Object,
+ value: () => ({
+ left: {},
+ right: {},
+ }),
+ },
+
loading: {
type: Boolean,
value: false,
@@ -222,15 +227,12 @@
],
listeners: {
- 'comment-discard': '_handleCommentDiscard',
- 'comment-update': '_handleCommentUpdate',
- 'comment-save': '_handleCommentSave',
'create-range-comment': '_handleCreateRangeComment',
'render-content': '_handleRenderContent',
},
attached() {
- this._updateRangesWhenNodesChange();
+ this._observeNodes();
},
detached() {
@@ -238,25 +240,38 @@
this._unobserveNodes();
},
- _updateRangesWhenNodesChange() {
+ _observeNodes() {
+ this._nodeObserver = Polymer.dom(this).observeNodes(info => {
+ const addedThreadEls = info.addedNodes.filter(isThreadEl);
+ // In principle we should also handle removed nodes, but I have not
+ // figured out how to do that yet without also catching all the removals
+ // caused by further redistribution. Right now, comments are never
+ // removed by no longer slotting them in, so I decided to not handle
+ // this situation until it occurs.
+ this._updateRanges(addedThreadEls);
+ this._updateKeyLocations(addedThreadEls);
+ });
+ },
+
+ _updateRanges(addedThreadEls) {
function commentRangeFromThreadEl(threadEl) {
const side = threadEl.getAttribute('comment-side');
const range = JSON.parse(threadEl.getAttribute('range'));
return {side, range, hovering: false};
}
- this._nodeObserver = Polymer.dom(this).observeNodes(info => {
- const addedThreadEls = info.addedNodes.filter(isThreadEl);
- const addedCommentRanges = addedThreadEls
- .map(commentRangeFromThreadEl)
- .filter(({range}) => range);
- this.push('_commentRanges', ...addedCommentRanges);
- // In principal we should also handle removed nodes, but I have not
- // figured out how to do that yet without also catching all the removals
- // caused by further redistribution. Right now, comments are never
- // removed by no longer slotting them in, so I decided to not handle
- // this situation until it occurs.
- });
+ const addedCommentRanges = addedThreadEls
+ .map(commentRangeFromThreadEl)
+ .filter(({range}) => range);
+ this.push('_commentRanges', ...addedCommentRanges);
+ },
+
+ _updateKeyLocations(addedThreadEls) {
+ for (const threadEl of addedThreadEls) {
+ const commentSide = threadEl.getAttribute('comment-side');
+ const lineNum = threadEl.getAttribute('line-num') || GrDiffLine.FILE;
+ this._keyLocations[commentSide][lineNum] = true;
+ }
},
/** Cancel any remaining diff builder rendering work. */
@@ -291,11 +306,6 @@
}
},
- _handleCommentSaveOrDiscard() {
- this.dispatchEvent(new CustomEvent('diff-comments-modified',
- {bubbles: true}));
- },
-
/** @return {string} */
_computeContainerClass(loggedIn, viewMode, displayLine) {
const classes = ['diffContainer'];
@@ -490,75 +500,6 @@
return side;
},
- _handleCommentDiscard(e) {
- const comment = e.detail.comment;
- this._removeComment(comment);
- this._handleCommentSaveOrDiscard();
- },
-
- _removeComment(comment) {
- const side = comment.__commentSide;
- this._removeCommentFromSide(comment, side);
- },
-
- _handleCommentSave(e) {
- const comment = e.detail.comment;
- const side = e.detail.comment.__commentSide;
- const idx = this._findDraftIndex(comment, side);
- this.set(['comments', side, idx], comment);
- this._handleCommentSaveOrDiscard();
- },
-
- /**
- * Closure annotation for Polymer.prototype.push is off. Submitted PR:
- * https://github.com/Polymer/polymer/pull/4776
- * but for not supressing annotations.
- *
- * @suppress {checkTypes} */
- _handleCommentUpdate(e) {
- const comment = e.detail.comment;
- const side = e.detail.comment.__commentSide;
- let idx = this._findCommentIndex(comment, side);
- if (idx === -1) {
- idx = this._findDraftIndex(comment, side);
- }
- if (idx !== -1) { // Update draft or comment.
- this.set(['comments', side, idx], comment);
- } else { // Create new draft.
- this.push(['comments', side], comment);
- }
- },
-
- _removeCommentFromSide(comment, side) {
- let idx = this._findCommentIndex(comment, side);
- if (idx === -1) {
- idx = this._findDraftIndex(comment, side);
- }
- if (idx !== -1) {
- this.splice('comments.' + side, idx, 1);
- }
- },
-
- /** @return {number} */
- _findCommentIndex(comment, side) {
- if (!comment.id || !this.comments[side]) {
- return -1;
- }
- return this.comments[side].findIndex(item => {
- return item.id === comment.id;
- });
- },
-
- /** @return {number} */
- _findDraftIndex(comment, side) {
- if (!comment.__draftID || !this.comments[side]) {
- return -1;
- }
- return this.comments[side].findIndex(item => {
- return item.__draftID === comment.__draftID;
- });
- },
-
_prefsObserver(newPrefs, oldPrefs) {
// Scan the preference objects one level deep to see if they differ.
let differ = !oldPrefs;
@@ -618,7 +559,7 @@
this.updateStyles(stylesToUpdate);
- if (this.diff && this.comments && !this.noRenderOnPrefsChange) {
+ if (this.diff && !this.noRenderOnPrefsChange) {
this._renderDiffTable();
}
},
@@ -645,15 +586,18 @@
}
this._showWarning = false;
- const keyLocations = this._getKeyLocations(this.comments,
- this.lineOfInterest);
- this.$.diffBuilder.render(keyLocations, this._getBypassPrefs());
+
+ if (this.lineOfInterest) {
+ const side = this.lineOfInterest.leftSide ? 'left' : 'right';
+ this._keyLocations[side][this.lineOfInterest.number] = true;
+ }
+ this.$.diffBuilder.render(this._keyLocations, this._getBypassPrefs());
},
_handleRenderContent() {
this._incrementalNodeObserver = Polymer.dom(this).observeNodes(info => {
const addedThreadEls = info.addedNodes.filter(isThreadEl);
- // In principal we should also handle removed nodes, but I have not
+ // In principle we should also handle removed nodes, but I have not
// figured out how to do that yet without also catching all the removals
// caused by further redistribution. Right now, comments are never
// removed by no longer slotting them in, so I decided to not handle
@@ -684,39 +628,6 @@
},
/**
- * Returns the key locations based on the comments and line of interests,
- * where lines should not be collapsed.
- *
- * @param {!Object} comments
- * @param {Defs.LineOfInterest|null} lineOfInterest
- *
- * @return {{left: Object<(string|number), boolean>,
- * right: Object<(string|number), boolean>}}
- */
- _getKeyLocations(comments, lineOfInterest) {
- const result = {
- left: {},
- right: {},
- };
- for (const side in comments) {
- if (side !== GrDiffBuilder.Side.LEFT &&
- side !== GrDiffBuilder.Side.RIGHT) {
- continue;
- }
- for (const c of comments[side]) {
- result[side][c.line || GrDiffLine.FILE] = true;
- }
- }
-
- if (lineOfInterest) {
- const side = lineOfInterest.leftSide ? 'left' : 'right';
- result[side][lineOfInterest.number] = true;
- }
-
- return result;
- },
-
- /**
* Get the preferences object including the safety bypass context (if any).
*/
_getBypassPrefs() {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index c6ef806..99f2ded9 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -192,107 +192,6 @@
element.$$('.diffContainer').classList.contains('displayLine'));
});
- test('remove comment', () => {
- element.comments = {
- meta: {
- changeNum: '42',
- patchRange: {
- basePatchNum: 'PARENT',
- patchNum: 3,
- },
- path: '/path/to/foo',
- projectConfig: {foo: 'bar'},
- },
- left: [
- {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
- {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
- {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
- {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
- ],
- right: [
- {id: 'c1', __commentSide: 'right'},
- {id: 'c2', __commentSide: 'right'},
- {id: 'd1', __draft: true, __commentSide: 'right'},
- {id: 'd2', __draft: true, __commentSide: 'right'},
- ],
- };
-
- element._removeComment({});
- // Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem
- // to believe that one object deepEquals another even when they do :-/.
- assert.equal(JSON.stringify(element.comments), JSON.stringify({
- meta: {
- changeNum: '42',
- patchRange: {
- basePatchNum: 'PARENT',
- patchNum: 3,
- },
- path: '/path/to/foo',
- projectConfig: {foo: 'bar'},
- },
- left: [
- {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
- {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
- {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
- {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
- ],
- right: [
- {id: 'c1', __commentSide: 'right'},
- {id: 'c2', __commentSide: 'right'},
- {id: 'd1', __draft: true, __commentSide: 'right'},
- {id: 'd2', __draft: true, __commentSide: 'right'},
- ],
- }));
-
- element._removeComment({id: 'bc2', side: 'PARENT',
- __commentSide: 'left'});
- assert.deepEqual(element.comments, {
- meta: {
- changeNum: '42',
- patchRange: {
- basePatchNum: 'PARENT',
- patchNum: 3,
- },
- path: '/path/to/foo',
- projectConfig: {foo: 'bar'},
- },
- left: [
- {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
- {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
- {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
- ],
- right: [
- {id: 'c1', __commentSide: 'right'},
- {id: 'c2', __commentSide: 'right'},
- {id: 'd1', __draft: true, __commentSide: 'right'},
- {id: 'd2', __draft: true, __commentSide: 'right'},
- ],
- });
-
- element._removeComment({id: 'd2', __commentSide: 'right'});
- assert.deepEqual(element.comments, {
- meta: {
- changeNum: '42',
- patchRange: {
- basePatchNum: 'PARENT',
- patchNum: 3,
- },
- path: '/path/to/foo',
- projectConfig: {foo: 'bar'},
- },
- left: [
- {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
- {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
- {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
- ],
- right: [
- {id: 'c1', __commentSide: 'right'},
- {id: 'c2', __commentSide: 'right'},
- {id: 'd1', __draft: true, __commentSide: 'right'},
- ],
- });
- });
-
test('thread groups', () => {
const contentEl = document.createElement('div');
@@ -333,11 +232,6 @@
};
element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
- element.comments = {
- left: [],
- right: [],
- meta: {patchRange: undefined},
- };
element.isImageDiff = true;
element.prefs = {
auto_hide_diff_table_header: true,
@@ -663,11 +557,6 @@
const setupDiff = function() {
const mock = document.createElement('mock-diff-response');
element.diff = mock.diffResponse;
- element.comments = {
- left: [],
- right: [],
- meta: {patchRange: undefined},
- };
element.prefs = {
context: 10,
tab_size: 8,
@@ -766,29 +655,6 @@
change_type: 'MODIFIED',
content: [{skip: 66}],
};
- element.comments = {
- meta: {
- changeNum: '42',
- patchRange: {
- basePatchNum: 'PARENT',
- patchNum: 3,
- },
- path: '/path/to/foo',
- projectConfig: {foo: 'bar'},
- },
- left: [
- {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
- {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
- {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
- {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
- ],
- right: [
- {id: 'c1', __commentSide: 'right'},
- {id: 'c2', __commentSide: 'right'},
- {id: 'd1', __draft: true, __commentSide: 'right'},
- {id: 'd2', __draft: true, __commentSide: 'right'},
- ],
- };
});
test('change in preferences re-renders diff', () => {
@@ -807,86 +673,6 @@
assert.isFalse(element._renderDiffTable.called);
});
});
-
- suite('handle comment-update', () => {
- setup(() => {
- element.comments = {
- meta: {
- changeNum: '42',
- patchRange: {
- basePatchNum: 'PARENT',
- patchNum: 3,
- },
- path: '/path/to/foo',
- projectConfig: {foo: 'bar'},
- },
- left: [
- {id: 'bc1', side: 'PARENT', __commentSide: 'left'},
- {id: 'bc2', side: 'PARENT', __commentSide: 'left'},
- {id: 'bd1', __draft: true, side: 'PARENT', __commentSide: 'left'},
- {id: 'bd2', __draft: true, side: 'PARENT', __commentSide: 'left'},
- ],
- right: [
- {id: 'c1', __commentSide: 'right'},
- {id: 'c2', __commentSide: 'right'},
- {id: 'd1', __draft: true, __commentSide: 'right'},
- {id: 'd2', __draft: true, __commentSide: 'right'},
- ],
- };
- });
-
- test('creating a draft', () => {
- const comment = {__draft: true, __draftID: 'tempID', side: 'PARENT',
- __commentSide: 'left'};
- element.fire('comment-update', {comment});
- assert.include(element.comments.left, comment);
- });
-
- test('discarding a draft', () => {
- const draftID = 'tempID';
- const id = 'savedID';
- const comment = {
- __draft: true,
- __draftID: draftID,
- side: 'PARENT',
- __commentSide: 'left',
- };
- const diffCommentsModifiedStub = sandbox.stub();
- element.addEventListener('diff-comments-modified',
- diffCommentsModifiedStub);
- element.comments.left.push(comment);
- comment.id = id;
- element.fire('comment-discard', {comment});
- const drafts = element.comments.left.filter(item => {
- return item.__draftID === draftID;
- });
- assert.equal(drafts.length, 0);
- assert.isTrue(diffCommentsModifiedStub.called);
- });
-
- test('saving a draft', () => {
- const draftID = 'tempID';
- const id = 'savedID';
- const comment = {
- __draft: true,
- __draftID: draftID,
- side: 'PARENT',
- __commentSide: 'left',
- };
- const diffCommentsModifiedStub = sandbox.stub();
- element.addEventListener('diff-comments-modified',
- diffCommentsModifiedStub);
- element.comments.left.push(comment);
- comment.id = id;
- element.fire('comment-save', {comment});
- const drafts = element.comments.left.filter(item => {
- return item.__draftID === draftID;
- });
- assert.equal(drafts.length, 1);
- assert.equal(drafts[0].id, id);
- assert.isTrue(diffCommentsModifiedStub.called);
- });
- });
});
suite('diff header', () => {
@@ -946,7 +732,6 @@
const mock = document.createElement('mock-diff-response');
sandbox.stub(element.$.diffBuilder, 'getDiffLength').returns(10000);
element.diff = mock.diffResponse;
- element.comments = {left: [], right: []};
element.noRenderOnPrefsChange = true;
});
@@ -1128,31 +913,55 @@
});
});
- test('_getKeyLocations', () => {
- assert.deepEqual(element._getKeyLocations({left: [], right: []}, null),
- {left: {}, right: {}});
- const comments = {
- left: [{line: 123}, {}],
- right: [{line: 456}],
- };
- assert.deepEqual(element._getKeyLocations(comments, null), {
- left: {FILE: true, 123: true},
- right: {456: true},
+ suite('key locations', () => {
+ let renderStub;
+
+ setup(() => {
+ element = fixture('basic');
+ element.prefs = {};
+ renderStub = sandbox.stub(element.$.diffBuilder, 'render');
});
- const lineOfInterest = {number: 789, leftSide: true};
- assert.deepEqual(
- element._getKeyLocations(comments, lineOfInterest), {
- left: {FILE: true, 123: true, 789: true},
- right: {456: true},
- });
+ test('lineOfInterest is a key location', () => {
+ element.lineOfInterest = {number: 789, leftSide: true};
+ element._renderDiffTable();
+ assert.isTrue(renderStub.called);
+ assert.deepEqual(renderStub.lastCall.args[0], {
+ left: {789: true},
+ right: {},
+ });
+ });
- delete lineOfInterest.leftSide;
- assert.deepEqual(
- element._getKeyLocations(comments, lineOfInterest), {
- left: {FILE: true, 123: true},
- right: {456: true, 789: true},
- });
+ test('line comments are key locations', () => {
+ const threadEl = document.createElement('div');
+ threadEl.className = 'comment-thread';
+ threadEl.setAttribute('comment-side', 'right');
+ threadEl.setAttribute('line-num', 3);
+ Polymer.dom(element).appendChild(threadEl);
+ Polymer.dom.flush();
+
+ element._renderDiffTable();
+ assert.isTrue(renderStub.called);
+ assert.deepEqual(renderStub.lastCall.args[0], {
+ left: {},
+ right: {3: true},
+ });
+ });
+
+ test('file comments are key locations', () => {
+ const threadEl = document.createElement('div');
+ threadEl.className = 'comment-thread';
+ threadEl.setAttribute('comment-side', 'left');
+ Polymer.dom(element).appendChild(threadEl);
+ Polymer.dom.flush();
+
+ element._renderDiffTable();
+ assert.isTrue(renderStub.called);
+ assert.deepEqual(renderStub.lastCall.args[0], {
+ left: {FILE: true},
+ right: {},
+ });
+ });
});
});
diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml
index c5faa49..75f632d 100644
--- a/tools/maven/gerrit-acceptance-framework_pom.xml
+++ b/tools/maven/gerrit-acceptance-framework_pom.xml
@@ -56,6 +56,9 @@
<name>Luca Milanesio</name>
</developer>
<developer>
+ <name>Marco Miller</name>
+ </developer>
+ <developer>
<name>Martin Fick</name>
</developer>
<developer>
diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml
index 52b11c1..cb8494b 100644
--- a/tools/maven/gerrit-extension-api_pom.xml
+++ b/tools/maven/gerrit-extension-api_pom.xml
@@ -56,6 +56,9 @@
<name>Luca Milanesio</name>
</developer>
<developer>
+ <name>Marco Miller</name>
+ </developer>
+ <developer>
<name>Martin Fick</name>
</developer>
<developer>
diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml
index d22c3ee..f58a6c7 100644
--- a/tools/maven/gerrit-plugin-api_pom.xml
+++ b/tools/maven/gerrit-plugin-api_pom.xml
@@ -56,6 +56,9 @@
<name>Luca Milanesio</name>
</developer>
<developer>
+ <name>Marco Miller</name>
+ </developer>
+ <developer>
<name>Martin Fick</name>
</developer>
<developer>
diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml
index e6c04e2..9849237 100644
--- a/tools/maven/gerrit-war_pom.xml
+++ b/tools/maven/gerrit-war_pom.xml
@@ -56,6 +56,9 @@
<name>Luca Milanesio</name>
</developer>
<developer>
+ <name>Marco Miller</name>
+ </developer>
+ <developer>
<name>Martin Fick</name>
</developer>
<developer>
