Merge "Cut down to below 73 characters when reverting changes"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 1396cf5..d5abbe3 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -3910,6 +3910,10 @@
The review must be provided in the request body as a
link:#review-input[ReviewInput] entity.
+If the labels are set, the user sending the request will automatically be
+added as a reviewer, otherwise (if they only commented) they are added to
+the CC list.
+
A review cannot be set on a change edit. Trying to post a review for a
change edit fails with `409 Conflict`.
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeEditApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeEditApi.java
index 25eb7a8..6bd4b73 100644
--- a/java/com/google/gerrit/extensions/api/changes/ChangeEditApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/ChangeEditApi.java
@@ -150,7 +150,22 @@
* @param newContent the desired content of the file
* @throws RestApiException if the content of the file couldn't be modified
*/
- void modifyFile(String filePath, RawInput newContent) throws RestApiException;
+ default void modifyFile(String filePath, RawInput newContent) throws RestApiException {
+ FileContentInput input = new FileContentInput();
+ input.content = newContent;
+ modifyFile(filePath, input);
+ }
+
+ /**
+ * Modify the contents of the specified file of the change edit. If no content is provided, the
+ * content of the file is erased but the file isn't deleted. If the change edit doesn't exist, it
+ * will be created based on the current patch set of the change.
+ *
+ * @param filePath the path of the file which should be modified
+ * @param input the desired content of the file
+ * @throws RestApiException if the content of the file couldn't be modified
+ */
+ void modifyFile(String filePath, FileContentInput input) throws RestApiException;
/**
* Deletes the specified file from the change edit. If the change edit doesn't exist, it will be
@@ -235,7 +250,7 @@
}
@Override
- public void modifyFile(String filePath, RawInput newContent) throws RestApiException {
+ public void modifyFile(String filePath, FileContentInput input) throws RestApiException {
throw new NotImplementedException();
}
diff --git a/java/com/google/gerrit/extensions/api/changes/FileContentInput.java b/java/com/google/gerrit/extensions/api/changes/FileContentInput.java
new file mode 100644
index 0000000..93c253d
--- /dev/null
+++ b/java/com/google/gerrit/extensions/api/changes/FileContentInput.java
@@ -0,0 +1,23 @@
+// 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.extensions.api.changes;
+
+import com.google.gerrit.extensions.restapi.DefaultInput;
+import com.google.gerrit.extensions.restapi.RawInput;
+
+/** Content to be added to a file (new or existing) via change edit. */
+public class FileContentInput {
+ @DefaultInput public RawInput content;
+}
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index b8ecb23..f390ebc 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -52,6 +52,7 @@
import com.google.common.base.Joiner;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
@@ -113,10 +114,12 @@
import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.group.GroupAuditService;
+import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.PerformanceLogContext;
import com.google.gerrit.server.logging.PerformanceLogger;
import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.logging.TraceContext.TraceTimer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -492,7 +495,7 @@
checkRequiresCapability(viewData);
}
- if (notModified(req, rsrc, viewData.view)) {
+ if (notModified(req, traceContext, viewData, rsrc)) {
logger.atFinest().log("REST call succeeded: %d", SC_NOT_MODIFIED);
res.sendError(SC_NOT_MODIFIED);
return;
@@ -586,7 +589,7 @@
}
status = response.statusCode();
- configureCaching(req, res, rsrc, viewData.view, response.caching());
+ configureCaching(req, res, traceContext, rsrc, viewData, response.caching());
res.setStatus(status);
logger.atFinest().log("REST call succeeded: %d", status);
}
@@ -711,6 +714,46 @@
}
}
+ private String getEtagWithRetry(
+ HttpServletRequest req,
+ TraceContext traceContext,
+ ViewData viewData,
+ ETagView<RestResource> view,
+ RestResource rsrc) {
+ try (TraceTimer ignored =
+ TraceContext.newTimer(
+ "RestApiServlet#getEtagWithRetry:view",
+ Metadata.builder().restViewName(getViewName(viewData)).build())) {
+ return invokeRestEndpointWithRetry(
+ req,
+ traceContext,
+ getViewName(viewData) + "#etag",
+ ActionType.REST_READ_REQUEST,
+ () -> view.getETag(rsrc));
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ throw new IllegalStateException("Failed to get ETag for view", e);
+ }
+ }
+
+ private String getEtagWithRetry(
+ HttpServletRequest req, TraceContext traceContext, RestResource.HasETag rsrc) {
+ try (TraceTimer ignored =
+ TraceContext.newTimer(
+ "RestApiServlet#getEtagWithRetry:resource",
+ Metadata.builder().restViewName(rsrc.getClass().getSimpleName()).build())) {
+ return invokeRestEndpointWithRetry(
+ req,
+ traceContext,
+ rsrc.getClass().getSimpleName() + "#etag",
+ ActionType.REST_READ_REQUEST,
+ () -> rsrc.getETag());
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ throw new IllegalStateException("Failed to get ETag for resource", e);
+ }
+ }
+
private RestResource parseResourceWithRetry(
HttpServletRequest req,
TraceContext traceContext,
@@ -962,24 +1005,27 @@
return defaultMessage;
}
- @SuppressWarnings({"unchecked", "rawtypes"})
- private static boolean notModified(
- HttpServletRequest req, RestResource rsrc, RestView<RestResource> view) {
+ private boolean notModified(
+ HttpServletRequest req, TraceContext traceContext, ViewData viewData, RestResource rsrc) {
if (!isRead(req)) {
return false;
}
+ RestView<RestResource> view = viewData.view;
if (view instanceof ETagView) {
String have = req.getHeader(HttpHeaders.IF_NONE_MATCH);
if (have != null) {
- return have.equals(((ETagView) view).getETag(rsrc));
+ String eTag =
+ getEtagWithRetry(req, traceContext, viewData, (ETagView<RestResource>) view, rsrc);
+ return have.equals(eTag);
}
}
if (rsrc instanceof RestResource.HasETag) {
String have = req.getHeader(HttpHeaders.IF_NONE_MATCH);
if (have != null) {
- return have.equals(((RestResource.HasETag) rsrc).getETag());
+ String eTag = getEtagWithRetry(req, traceContext, (RestResource.HasETag) rsrc);
+ return have.equals(eTag);
}
}
@@ -993,21 +1039,48 @@
return false;
}
- private static <R extends RestResource> void configureCaching(
- HttpServletRequest req, HttpServletResponse res, R rsrc, RestView<R> view, CacheControl c) {
+ private <R extends RestResource> void configureCaching(
+ HttpServletRequest req,
+ HttpServletResponse res,
+ TraceContext traceContext,
+ R rsrc,
+ ViewData viewData,
+ CacheControl cacheControl) {
+ setCacheHeaders(req, res, cacheControl);
if (isRead(req)) {
- switch (c.getType()) {
+ switch (cacheControl.getType()) {
+ case NONE:
+ default:
+ break;
+ case PRIVATE:
+ addResourceStateHeaders(req, res, traceContext, viewData, rsrc);
+ break;
+ case PUBLIC:
+ addResourceStateHeaders(req, res, traceContext, viewData, rsrc);
+ break;
+ }
+ }
+ }
+
+ private static <R extends RestResource> void setCacheHeaders(
+ HttpServletRequest req, HttpServletResponse res, CacheControl cacheControl) {
+ if (isRead(req)) {
+ switch (cacheControl.getType()) {
case NONE:
default:
CacheHeaders.setNotCacheable(res);
break;
case PRIVATE:
- addResourceStateHeaders(res, rsrc, view);
- CacheHeaders.setCacheablePrivate(res, c.getAge(), c.getUnit(), c.isMustRevalidate());
+ CacheHeaders.setCacheablePrivate(
+ res, cacheControl.getAge(), cacheControl.getUnit(), cacheControl.isMustRevalidate());
break;
case PUBLIC:
- addResourceStateHeaders(res, rsrc, view);
- CacheHeaders.setCacheable(req, res, c.getAge(), c.getUnit(), c.isMustRevalidate());
+ CacheHeaders.setCacheable(
+ req,
+ res,
+ cacheControl.getAge(),
+ cacheControl.getUnit(),
+ cacheControl.isMustRevalidate());
break;
}
} else {
@@ -1015,12 +1088,20 @@
}
}
- private static <R extends RestResource> void addResourceStateHeaders(
- HttpServletResponse res, R rsrc, RestView<R> view) {
+ private void addResourceStateHeaders(
+ HttpServletRequest req,
+ HttpServletResponse res,
+ TraceContext traceContext,
+ ViewData viewData,
+ RestResource rsrc) {
+ RestView<RestResource> view = viewData.view;
if (view instanceof ETagView) {
- res.setHeader(HttpHeaders.ETAG, ((ETagView<R>) view).getETag(rsrc));
+ String eTag =
+ getEtagWithRetry(req, traceContext, viewData, (ETagView<RestResource>) view, rsrc);
+ res.setHeader(HttpHeaders.ETAG, eTag);
} else if (rsrc instanceof RestResource.HasETag) {
- res.setHeader(HttpHeaders.ETAG, ((RestResource.HasETag) rsrc).getETag());
+ String eTag = getEtagWithRetry(req, traceContext, (RestResource.HasETag) rsrc);
+ res.setHeader(HttpHeaders.ETAG, eTag);
}
if (rsrc instanceof RestResource.HasLastModified) {
res.setDateHeader(
@@ -1662,43 +1743,47 @@
private long handleException(
TraceContext traceContext, Throwable err, HttpServletRequest req, HttpServletResponse res)
throws IOException {
- logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req));
- if (!res.isCommitted()) {
- res.reset();
- traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
- ImmutableList<String> userMessages =
- globals.exceptionHooks.stream()
- .map(h -> h.getUserMessage(err))
- .filter(Optional::isPresent)
- .map(Optional::get)
- .collect(toImmutableList());
+ if (res.isCommitted()) {
+ logger.atSevere().withCause(err).log(
+ "Error in %s %s, response already committed", req.getMethod(), uriForLogging(req));
+ return 0;
+ }
- Optional<Integer> statusCode =
- globals.exceptionHooks.stream()
- .map(h -> h.getStatusCode(err))
- .filter(Optional::isPresent)
- .map(Optional::get)
- .findFirst();
- if (statusCode.isPresent() && statusCode.get() < 400) {
- StringBuilder msg = new StringBuilder();
- if (userMessages.size() == 1) {
- msg.append(userMessages.get(0));
- } else {
- userMessages.forEach(m -> msg.append("\n* ").append(m));
- }
+ res.reset();
+ traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
+ ImmutableList<String> userMessages =
+ globals.exceptionHooks.stream()
+ .map(h -> h.getUserMessage(err))
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .collect(toImmutableList());
- res.setStatus(statusCode.get());
- logger.atFinest().withCause(err).log("REST call finished: %d", statusCode.get().intValue());
- return replyText(req, res, true, msg.toString());
- }
-
- StringBuilder msg = new StringBuilder("Internal server error");
- if (!userMessages.isEmpty()) {
+ Optional<Integer> statusCode =
+ globals.exceptionHooks.stream()
+ .map(h -> h.getStatusCode(err))
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .findFirst();
+ if (statusCode.isPresent() && statusCode.get() < 400) {
+ StringBuilder msg = new StringBuilder();
+ if (userMessages.size() == 1) {
+ msg.append(userMessages.get(0));
+ } else {
userMessages.forEach(m -> msg.append("\n* ").append(m));
}
- return replyError(req, res, statusCode.orElse(SC_INTERNAL_SERVER_ERROR), msg.toString(), err);
+
+ res.setStatus(statusCode.get());
+ logger.atFinest().withCause(err).log("REST call finished: %d", statusCode.get().intValue());
+ return replyText(req, res, true, msg.toString());
}
- return 0;
+
+ logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req));
+
+ StringBuilder msg = new StringBuilder("Internal server error");
+ if (!userMessages.isEmpty()) {
+ userMessages.forEach(m -> msg.append("\n* ").append(m));
+ }
+ return replyError(req, res, statusCode.orElse(SC_INTERNAL_SERVER_ERROR), msg.toString(), err);
}
private static String uriForLogging(HttpServletRequest req) {
@@ -1724,13 +1809,13 @@
HttpServletResponse res,
int statusCode,
String msg,
- CacheControl c,
+ CacheControl cacheControl,
@Nullable Throwable err)
throws IOException {
if (err != null) {
RequestUtil.setErrorTraceAttribute(req, err);
}
- configureCaching(req, res, null, null, c);
+ setCacheHeaders(req, res, cacheControl);
checkArgument(statusCode >= 400, "non-error status: %s", statusCode);
res.setStatus(statusCode);
logger.atFinest().withCause(err).log("REST call failed: %d", statusCode);
diff --git a/java/com/google/gerrit/server/api/changes/ChangeEditApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeEditApiImpl.java
index 7f0feba..1b0f0c5 100644
--- a/java/com/google/gerrit/server/api/changes/ChangeEditApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangeEditApiImpl.java
@@ -17,6 +17,7 @@
import static com.google.gerrit.server.api.ApiUtil.asRestApiException;
import com.google.gerrit.extensions.api.changes.ChangeEditApi;
+import com.google.gerrit.extensions.api.changes.FileContentInput;
import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
import com.google.gerrit.extensions.client.ChangeEditDetailOption;
import com.google.gerrit.extensions.common.EditInfo;
@@ -24,7 +25,6 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.IdString;
-import com.google.gerrit.extensions.restapi.RawInput;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -200,9 +200,9 @@
}
@Override
- public void modifyFile(String filePath, RawInput newContent) throws RestApiException {
+ public void modifyFile(String filePath, FileContentInput input) throws RestApiException {
try {
- changeEditsPut.apply(changeResource, filePath, newContent);
+ changeEditsPut.apply(changeResource, filePath, input);
} catch (Exception e) {
throw asRestApiException("Cannot modify file of change edit", e);
}
diff --git a/java/com/google/gerrit/server/patch/AutoMerger.java b/java/com/google/gerrit/server/patch/AutoMerger.java
index a9cc9b5..2e0214c 100644
--- a/java/com/google/gerrit/server/patch/AutoMerger.java
+++ b/java/com/google/gerrit/server/patch/AutoMerger.java
@@ -16,14 +16,17 @@
import static com.google.common.base.Preconditions.checkArgument;
-import com.google.common.flogger.FluentLogger;
+import com.google.common.base.Throwables;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.git.LockFailureException;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.InMemoryInserter;
import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryableAction.ActionType;
import com.google.inject.Inject;
import java.io.IOException;
import org.eclipse.jgit.dircache.DirCache;
@@ -42,28 +45,54 @@
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevWalk;
+/**
+ * Utility class for creating an auto-merge commit of a merge commit.
+ *
+ * <p>An auto-merge commit is the result of merging the 2 parents of a merge commit automatically.
+ * If there are conflicts the auto-merge commit contains Git conflict markers that indicate these
+ * conflicts.
+ *
+ * <p>Creating auto-merge commits for octopus merges (merge commits with more than 2 parents) is not
+ * supported. In this case the auto-merge is created between the first 2 parent commits.
+ *
+ * <p>All created auto-merge commits are stored in the repository of their merge commit as {@code
+ * refs/cache-automerge/} branches. These branches serve:
+ *
+ * <ul>
+ * <li>as a cache so that the each auto-merge gets computed only once
+ * <li>as base for merge commits on which users can comment
+ * </ul>
+ *
+ * <p>The second point means that these commits are referenced from NoteDb. The consequence of this
+ * is that these refs should never be deleted.
+ */
public class AutoMerger {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
@UsedAt(UsedAt.Project.GOOGLE)
public static boolean cacheAutomerge(Config cfg) {
return cfg.getBoolean("change", null, "cacheAutomerge", true);
}
+ private final RetryHelper retryHelper;
private final PersonIdent gerritIdent;
private final boolean save;
@Inject
- AutoMerger(@GerritServerConfig Config cfg, @GerritPersonIdent PersonIdent gerritIdent) {
+ AutoMerger(
+ RetryHelper retryHelper,
+ @GerritServerConfig Config cfg,
+ @GerritPersonIdent PersonIdent gerritIdent) {
+ this.retryHelper = retryHelper;
save = cacheAutomerge(cfg);
this.gerritIdent = gerritIdent;
}
/**
- * Perform an auto-merge of the parents of the given merge commit.
+ * Creates an auto-merge commit of the parents of the given merge commit.
*
- * @return auto-merge commit or {@code null} if an auto-merge commit couldn't be created. Headers
- * of the returned RevCommit are parsed.
+ * <p>In case of an exception the creation of the auto-merge commit is retried a few times. E.g.
+ * this allows the operation to succeed if a Git update fails due to a temporary issue.
+ *
+ * @return auto-merge commit. Headers of the returned RevCommit are parsed.
*/
public RevCommit merge(
Repository repo,
@@ -72,7 +101,34 @@
RevCommit merge,
ThreeWayMergeStrategy mergeStrategy)
throws IOException {
+ try {
+ return retryHelper
+ .action(
+ ActionType.GIT_UPDATE,
+ "createAutoMerge",
+ () -> createAutoMergeCommit(repo, rw, ins, merge, mergeStrategy))
+ .call();
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ Throwables.throwIfInstanceOf(e, IOException.class);
+ throw new IllegalStateException(e);
+ }
+ }
+
+ /**
+ * Creates an auto-merge commit of the parents of the given merge commit.
+ *
+ * @return auto-merge commit. Headers of the returned RevCommit are parsed.
+ */
+ private RevCommit createAutoMergeCommit(
+ Repository repo,
+ RevWalk rw,
+ ObjectInserter ins,
+ RevCommit merge,
+ ThreeWayMergeStrategy mergeStrategy)
+ throws IOException {
checkArgument(rw.getObjectReader().getCreatedFromInserter() == ins);
+
InMemoryInserter tmpIns = null;
if (ins instanceof InMemoryInserter) {
// Caller gave us an in-memory inserter, so ensure anything we write from
@@ -102,17 +158,7 @@
m.setDirCache(dc);
m.setObjectInserter(tmpIns == null ? new NonFlushingWrapper(ins) : tmpIns);
- boolean couldMerge;
- try {
- couldMerge = m.merge(merge.getParents());
- } catch (IOException | RuntimeException e) {
- // It is not safe to continue further down in this method as throwing
- // an exception most likely means that the merge tree was not created
- // and m.getMergeResults() is empty. This would mean that all paths are
- // unmerged and Gerrit UI would show all paths in the patch list.
- logger.atWarning().withCause(e).log("Error attempting automerge %s", refName);
- return null;
- }
+ boolean couldMerge = m.merge(merge.getParents());
ObjectId treeId;
if (couldMerge) {
@@ -173,8 +219,28 @@
RefUpdate ru = repo.updateRef(refName);
ru.setNewObjectId(commitId);
ru.disableRefLog();
- ru.forceUpdate();
- return rw.parseCommit(commitId);
+ switch (ru.forceUpdate()) {
+ case FAST_FORWARD:
+ case FORCED:
+ case NEW:
+ case NO_CHANGE:
+ return rw.parseCommit(commitId);
+ case LOCK_FAILURE:
+ throw new LockFailureException(
+ String.format("Failed to create auto-merge of %s", merge.name()), ru);
+ case IO_FAILURE:
+ case NOT_ATTEMPTED:
+ case REJECTED:
+ case REJECTED_CURRENT_BRANCH:
+ case REJECTED_MISSING_OBJECT:
+ case REJECTED_OTHER_REASON:
+ case RENAMED:
+ default:
+ throw new IOException(
+ String.format(
+ "Failed to create auto-merge of %s: Cannot write %s (%s)",
+ merge.name(), refName, ru.getResult()));
+ }
}
private static class NonFlushingWrapper extends ObjectInserter.Filter {
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 07cb50d..253f50c 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -89,7 +89,7 @@
}
/** Can this user see this change? */
- private boolean isVisible(@Nullable ChangeData cd) {
+ private boolean isVisible(ChangeData cd) {
if (getChange().isPrivate() && !isPrivateVisible(cd)) {
return false;
}
@@ -144,7 +144,7 @@
/** Is this user assigned to this change? */
private boolean isAssignee() {
- Account.Id currentAssignee = notes.getChange().getAssignee();
+ Account.Id currentAssignee = getChange().getAssignee();
if (currentAssignee != null && getUser().isIdentifiedUser()) {
Account.Id id = getUser().getAccountId();
return id.equals(currentAssignee);
@@ -153,9 +153,8 @@
}
/** Is this user a reviewer for the change? */
- private boolean isReviewer(@Nullable ChangeData cd) {
+ private boolean isReviewer(ChangeData cd) {
if (getUser().isIdentifiedUser()) {
- cd = cd != null ? cd : changeDataFactory.create(notes);
Collection<Account.Id> results = cd.reviewers().all();
return results.contains(getUser().getAccountId());
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index df5d291..5f076b1 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -25,6 +25,7 @@
import com.google.common.base.Enums;
import com.google.common.base.Splitter;
import com.google.common.collect.Lists;
+import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
@@ -100,6 +101,8 @@
/** Parses a query string meant to be applied to change objects. */
public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuilder> {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
public interface ChangeOperatorFactory extends OperatorFactory<ChangeData, ChangeQueryBuilder> {}
/**
@@ -1441,8 +1444,14 @@
accounts.stream()
.map(id -> ReviewerPredicate.forState(id, state))
.collect(toList()));
+ } else {
+ logger.atFine().log(
+ "Skipping reviewer predicate for %s in default field query"
+ + " because the number of matched accounts (%d) exceeds the limit of %d",
+ who, accounts.size(), MAX_ACCOUNTS_PER_DEFAULT_FIELD);
}
} catch (QueryParseException e) {
+ logger.atFine().log("Parsing %s as account failed: %s", who, e.getMessage());
// Propagate this exception only if we can't use 'who' to query by email
if (reviewerByEmailPredicate == null) {
throw e;
diff --git a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
index 1808fa6..e220f35 100644
--- a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
@@ -19,6 +19,7 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.api.changes.FileContentInput;
import com.google.gerrit.extensions.common.DiffWebLinkInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.Input;
@@ -109,7 +110,7 @@
*/
@Singleton
public static class Create
- implements RestCollectionCreateView<ChangeResource, ChangeEditResource, Put.Input> {
+ implements RestCollectionCreateView<ChangeResource, ChangeEditResource, FileContentInput> {
private final Put putEdit;
@Inject
@@ -118,9 +119,9 @@
}
@Override
- public Response<?> apply(ChangeResource resource, IdString id, Put.Input input)
+ public Response<?> apply(ChangeResource resource, IdString id, FileContentInput input)
throws AuthException, ResourceConflictException, IOException, PermissionBackendException {
- putEdit.apply(resource, id.get(), input.content);
+ putEdit.apply(resource, id.get(), input);
return Response.none();
}
}
@@ -267,11 +268,7 @@
/** Put handler that is activated when PUT request is called on collection element. */
@Singleton
- public static class Put implements RestModifyView<ChangeEditResource, Put.Input> {
- public static class Input {
- @DefaultInput public RawInput content;
- }
-
+ public static class Put implements RestModifyView<ChangeEditResource, FileContentInput> {
private final ChangeEditModifier editModifier;
private final GitRepositoryManager repositoryManager;
@@ -282,17 +279,18 @@
}
@Override
- public Response<?> apply(ChangeEditResource rsrc, Input input)
+ public Response<?> apply(ChangeEditResource rsrc, FileContentInput input)
throws AuthException, ResourceConflictException, IOException, PermissionBackendException {
- return apply(rsrc.getChangeResource(), rsrc.getPath(), input.content);
+ return apply(rsrc.getChangeResource(), rsrc.getPath(), input);
}
- public Response<?> apply(ChangeResource rsrc, String path, RawInput newContent)
+ public Response<?> apply(ChangeResource rsrc, String path, FileContentInput input)
throws ResourceConflictException, AuthException, IOException, PermissionBackendException {
if (Strings.isNullOrEmpty(path) || path.charAt(0) == '/') {
throw new ResourceConflictException("Invalid path: " + path);
}
+ RawInput newContent = input.content;
try (Repository repository = repositoryManager.openRepository(rsrc.getProject())) {
editModifier.modifyFile(repository, rsrc.getNotes(), path, newContent);
} catch (InvalidChangeOperationException e) {
diff --git a/java/com/google/gerrit/server/restapi/change/GetRelated.java b/java/com/google/gerrit/server/restapi/change/GetRelated.java
index a846d50..5f075f6 100644
--- a/java/com/google/gerrit/server/restapi/change/GetRelated.java
+++ b/java/com/google/gerrit/server/restapi/change/GetRelated.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.restapi.change;
+import static com.google.common.base.Preconditions.checkArgument;
import static java.util.stream.Collectors.toSet;
import com.google.common.annotations.VisibleForTesting;
@@ -55,17 +56,20 @@
private final PatchSetUtil psUtil;
private final RelatedChangesSorter sorter;
private final IndexConfig indexConfig;
+ private final ChangeData.Factory changeDataFactory;
@Inject
GetRelated(
Provider<InternalChangeQuery> queryProvider,
PatchSetUtil psUtil,
RelatedChangesSorter sorter,
- IndexConfig indexConfig) {
+ IndexConfig indexConfig,
+ ChangeData.Factory changeDataFactory) {
this.queryProvider = queryProvider;
this.psUtil = psUtil;
this.sorter = sorter;
this.indexConfig = indexConfig;
+ this.changeDataFactory = changeDataFactory;
}
@Override
@@ -98,7 +102,7 @@
boolean isEdit = rsrc.getEdit().isPresent();
PatchSet basePs = isEdit ? rsrc.getEdit().get().getBasePatchSet() : rsrc.getPatchSet();
- reloadChangeIfStale(cds, basePs);
+ cds = reloadChangeIfStale(cds, rsrc.getChange(), basePs);
for (RelatedChangesSorter.PatchSetData d : sorter.sort(cds, basePs)) {
PatchSet ps = d.patchSet();
@@ -127,14 +131,30 @@
return psUtil.byChange(notes).stream().flatMap(ps -> ps.groups().stream()).collect(toSet());
}
- private void reloadChangeIfStale(List<ChangeData> cds, PatchSet wantedPs) {
- for (ChangeData cd : cds) {
- if (cd.getId().equals(wantedPs.id().changeId())) {
- if (cd.patchSet(wantedPs.id()) == null) {
- cd.reloadChange();
- }
- }
+ private List<ChangeData> reloadChangeIfStale(
+ List<ChangeData> changeDatasFromIndex, Change wantedChange, PatchSet wantedPs) {
+ checkArgument(
+ wantedChange.getId().equals(wantedPs.id().changeId()),
+ "change of wantedPs (%s) doesn't match wantedChange (%s)",
+ wantedPs.id().changeId(),
+ wantedChange.getId());
+
+ List<ChangeData> changeDatas = new ArrayList<>(changeDatasFromIndex.size() + 1);
+ changeDatas.addAll(changeDatasFromIndex);
+
+ // Reload the change in case the patch set is absent.
+ changeDatas.stream()
+ .filter(
+ cd -> cd.getId().equals(wantedPs.id().changeId()) && cd.patchSet(wantedPs.id()) == null)
+ .forEach(ChangeData::reloadChange);
+
+ if (changeDatas.stream().noneMatch(cd -> cd.getId().equals(wantedPs.id().changeId()))) {
+ // The change of the wanted patch set is missing in the result from the index.
+ // Load it from NoteDb and add it to the result.
+ changeDatas.add(changeDataFactory.create(wantedChange));
}
+
+ return changeDatas;
}
static RelatedChangeAndCommitInfo newChangeAndCommit(
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 64c4578..79ae43b 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -16,7 +16,6 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkState;
import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toSet;
@@ -459,11 +458,17 @@
try {
ChangeSet indexBackedChangeSet =
mergeSuperSet.setMergeOpRepoManager(orm).completeChangeSet(change, caller);
- checkState(
- indexBackedChangeSet.ids().contains(change.getId()),
- "change %s missing from %s",
- change.getId(),
- indexBackedChangeSet);
+ if (!indexBackedChangeSet.ids().contains(change.getId())) {
+ // indexBackedChangeSet contains only open changes, if the change is missing in this set
+ // it might be that the change was concurrently submitted in the meantime.
+ change = changeDataFactory.create(change).reloadChange();
+ if (!change.isNew()) {
+ throw new ResourceConflictException("change is " + ChangeUtil.status(change));
+ }
+ throw new IllegalStateException(
+ String.format("change %s missing from %s", change.getId(), indexBackedChangeSet));
+ }
+
if (indexBackedChangeSet.furtherHiddenChanges()) {
throw new AuthException(
"A change to be submitted with " + change.getId() + " is not visible");
diff --git a/java/com/google/gerrit/server/submit/MergeSuperSet.java b/java/com/google/gerrit/server/submit/MergeSuperSet.java
index bcebc7f..b36eb7d 100644
--- a/java/com/google/gerrit/server/submit/MergeSuperSet.java
+++ b/java/com/google/gerrit/server/submit/MergeSuperSet.java
@@ -19,16 +19,19 @@
import com.google.common.base.Strings;
import com.google.common.collect.Iterables;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.plugincontext.PluginContext;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
@@ -53,6 +56,7 @@
* included.
*/
public class MergeSuperSet {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final Provider<InternalChangeQuery> queryProvider;
private final Provider<MergeOpRepoManager> repoManagerProvider;
@@ -60,6 +64,7 @@
private final PermissionBackend permissionBackend;
private final Config cfg;
private final ProjectCache projectCache;
+ private final ChangeNotes.Factory notesFactory;
private MergeOpRepoManager orm;
private boolean closeOrm;
@@ -71,13 +76,15 @@
Provider<MergeOpRepoManager> repoManagerProvider,
DynamicItem<MergeSuperSetComputation> mergeSuperSetComputation,
PermissionBackend permissionBackend,
- ProjectCache projectCache) {
+ ProjectCache projectCache,
+ ChangeNotes.Factory notesFactory) {
this.cfg = cfg;
this.queryProvider = queryProvider;
this.repoManagerProvider = repoManagerProvider;
this.mergeSuperSetComputation = mergeSuperSetComputation;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
+ this.notesFactory = notesFactory;
}
public static boolean wholeTopicEnabled(Config config) {
@@ -215,8 +222,23 @@
return false;
}
+ ChangeNotes notes;
try {
- permissionBackend.user(user).change(cd).check(ChangePermission.READ);
+ notes = cd.notes();
+ } catch (NoSuchChangeException e) {
+ // The change was found in the index but is missing in NoteDb.
+ // This can happen in systems with multiple primary nodes when the replication of the index
+ // documents is faster than the replication of the Git data.
+ // Instead of failing, create the change notes from the index data so that the read permission
+ // check can be performed successfully.
+ logger.atWarning().log(
+ "Got change %d of project %s from index, but couldn't find it in NoteDb",
+ cd.getId().get(), cd.project().get());
+ notes = notesFactory.createFromIndexedChange(cd.change());
+ }
+
+ try {
+ permissionBackend.user(user).change(notes).check(ChangePermission.READ);
return true;
} catch (AuthException e) {
return false;
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index bb69453..e13cab1 100644
--- a/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/java/com/google/gerrit/server/update/RetryHelper.java
@@ -481,9 +481,9 @@
if (!traceContext.isTracing()) {
String traceId = "retry-on-failure-" + new RequestId();
traceContext.addTag(RequestId.Type.TRACE_ID, traceId).forceLogging();
- opts.onAutoTrace().ifPresent(c -> c.accept(traceId));
logger.atFine().withCause(t).log(
"AutoRetry: %s failed, retry with tracing enabled", actionName);
+ opts.onAutoTrace().ifPresent(c -> c.accept(traceId));
metrics.autoRetryCount.increment(actionType, actionName, cause);
return true;
}
diff --git a/java/com/google/gerrit/server/update/RetryableAction.java b/java/com/google/gerrit/server/update/RetryableAction.java
index 75ebeb37..167b209 100644
--- a/java/com/google/gerrit/server/update/RetryableAction.java
+++ b/java/com/google/gerrit/server/update/RetryableAction.java
@@ -51,6 +51,7 @@
public enum ActionType {
ACCOUNT_UPDATE,
CHANGE_UPDATE,
+ GIT_UPDATE,
GROUP_UPDATE,
INDEX_QUERY,
PLUGIN_UPDATE,
diff --git a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
index cc8d233..bf93d58 100644
--- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
+++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -45,6 +45,7 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
+import com.google.gerrit.extensions.api.changes.FileContentInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -64,7 +65,6 @@
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.gerrit.server.restapi.change.ChangeEdits.EditMessage;
import com.google.gerrit.server.restapi.change.ChangeEdits.Post;
-import com.google.gerrit.server.restapi.change.ChangeEdits.Put;
import com.google.gson.reflect.TypeToken;
import com.google.gson.stream.JsonReader;
import com.google.inject.Inject;
@@ -501,7 +501,7 @@
@Test
public void createAndChangeEditInOneRequestRest() throws Exception {
- Put.Input in = new Put.Input();
+ FileContentInput in = new FileContentInput();
in.content = RawInputUtil.create(CONTENT_NEW);
adminRestSession.putRaw(urlEditFile(changeId, FILE_NAME), in.content).assertNoContent();
ensureSameBytes(getFileContentOfEdit(changeId, FILE_NAME), CONTENT_NEW);
@@ -513,7 +513,7 @@
@Test
public void changeEditRest() throws Exception {
createEmptyEditFor(changeId);
- Put.Input in = new Put.Input();
+ FileContentInput in = new FileContentInput();
in.content = RawInputUtil.create(CONTENT_NEW);
adminRestSession.putRaw(urlEditFile(changeId, FILE_NAME), in.content).assertNoContent();
ensureSameBytes(getFileContentOfEdit(changeId, FILE_NAME), CONTENT_NEW);
@@ -534,7 +534,7 @@
@Test
public void getFileContentRest() throws Exception {
- Put.Input in = new Put.Input();
+ FileContentInput in = new FileContentInput();
in.content = RawInputUtil.create(CONTENT_NEW);
adminRestSession.putRaw(urlEditFile(changeId, FILE_NAME), in.content).assertNoContent();
gApi.changes().id(changeId).edit().modifyFile(FILE_NAME, RawInputUtil.create(CONTENT_NEW2));
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index ed62918..fa84b95 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -1821,7 +1821,8 @@
if (!this._selectedRevision) {
return;
}
- const patchNum = parseInt(patchNumStr, 10);
+ // If patchNumStr is"edit", then patchNum is undefined hence an OR
+ const patchNum = parseInt(patchNumStr, 10) || patchNumStr;
if (patchNum === this._selectedRevision._number) {
return;
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index d9b8464..b80b51e 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -1617,6 +1617,30 @@
});
});
+ test('_selectedRevision is assigned when patchNum is edit', () => {
+ const revision1 = {_number: 1, commit: {parents: []}};
+ const revision2 = {_number: 2, commit: {parents: []}};
+ const revision3 = {_number: 'edit', commit: {parents: []}};
+ sandbox.stub(element.$.restAPI, 'getChangeDetail').returns(
+ Promise.resolve({
+ revisions: {
+ aaa: revision1,
+ bbb: revision2,
+ ccc: revision3,
+ },
+ labels: {},
+ actions: {},
+ current_revision: 'ccc',
+ change_id: 'loremipsumdolorsitamet',
+ }));
+ sandbox.stub(element, '_getEdit').returns(Promise.resolve());
+ sandbox.stub(element, '_getPreferences').returns(Promise.resolve({}));
+ element._patchRange = {patchNum: 'edit'};
+ return element._getChangeDetail().then(() => {
+ assert.strictEqual(element._selectedRevision, revision3);
+ });
+ });
+
test('_sendShowChangeEvent', () => {
element._change = {labels: {}};
element._patchRange = {patchNum: 4};
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
index e836ccc..c177283 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
@@ -269,16 +269,6 @@
</section>
<section class="actions">
<div class="left">
- <template is="dom-if" if="[[canBeStarted]]">
- <gr-button
- link
- secondary
- disabled="[[_isState(knownLatestState, 'not-latest')]]"
- class="action save"
- has-tooltip
- title="[[_saveTooltip]]"
- on-click="_saveTapHandler">Save</gr-button>
- </template>
<span
id="checkingStatusLabel"
hidden$="[[!_isState(knownLatestState, 'checking')]]">
@@ -297,6 +287,18 @@
id="cancelButton"
class="action cancel"
on-click="_cancelTapHandler">Cancel</gr-button>
+ <template is="dom-if" if="[[canBeStarted]]">
+ <!-- Use 'Send' here as the change may only about reviewers / ccs
+ and when this button is visible, the next button will always
+ be 'Start review' -->
+ <gr-button
+ link
+ disabled="[[_isState(knownLatestState, 'not-latest')]]"
+ class="action save"
+ has-tooltip
+ title="[[_saveTooltip]]"
+ on-click="_saveClickHandler">Send</gr-button>
+ </template>
<gr-button
id="sendButton"
link
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
index 9551a46..3ce4f1a 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
@@ -43,7 +43,7 @@
};
const ButtonTooltips = {
- SAVE: 'Save reply but do not send notification',
+ SAVE: 'Send but do not send notification or change review state',
START_REVIEW: 'Mark as ready for review and send reply',
SEND: 'Send reply',
};
@@ -686,7 +686,7 @@
this._rebuildReviewerArrays(this.change.reviewers, this._owner);
}
- _saveTapHandler(e) {
+ _saveClickHandler(e) {
e.preventDefault();
if (!this.$.ccs.submitEntryText()) {
// Do not proceed with the save if there is an invalid email entry in
diff --git a/polygerrit-ui/app/elements/shared/gr-button/gr-button.html b/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
index 87caf64..fd75c7c 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
@@ -116,13 +116,6 @@
:host([link][primary]) {
--text-color: var(--primary-button-background-color);
}
- :host([secondary]) {
- --background-color: var(--secondary-button-text-color);
- --text-color: var(--secondary-button-background-color);
- }
- :host([link][secondary]) {
- --text-color: var(--secondary-button-text-color);
- }
/* Keep below color definition for primary so that this takes precedence
when disabled. */
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.html b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.html
index 0567777..d0a07b6 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.html
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.html
@@ -110,25 +110,21 @@
<gr-button
id="replyBtn"
link
- secondary
class="action reply"
on-click="_handleCommentReply">Reply</gr-button>
<gr-button
id="quoteBtn"
link
- secondary
class="action quote"
on-click="_handleCommentQuote">Quote</gr-button>
<gr-button
id="ackBtn"
link
- secondary
class="action ack"
on-click="_handleCommentAck">Ack</gr-button>
<gr-button
id="doneBtn"
link
- secondary
class="action done"
on-click="_handleCommentDone">Done</gr-button>
</div>
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
index ea68e5f..a96cf44 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.html
@@ -119,7 +119,6 @@
--gr-button: {
height: 20px;
padding: 0 var(--spacing-s);
- color: var(--default-button-text-color);
}
}
.editMessage {
@@ -268,7 +267,6 @@
<gr-button
id="deleteBtn"
link
- secondary
class$="action delete [[_computeDeleteButtonClass(_isAdmin, draft)]]"
on-click="_handleCommentDelete">
<iron-icon id="icon" icon="gr-icons:delete"></iron-icon>
@@ -325,22 +323,18 @@
<div class="rightActions">
<gr-button
link
- secondary
class="action cancel hideOnPublished"
on-click="_handleCancel">Cancel</gr-button>
<gr-button
link
- secondary
class="action discard hideOnPublished"
on-click="_handleDiscard">Discard</gr-button>
<gr-button
link
- secondary
class="action edit hideOnPublished"
on-click="_handleEdit">Edit</gr-button>
<gr-button
link
- secondary
disabled$="[[_computeSaveDisabled(_messageText, comment, resolved)]]"
class="action save hideOnPublished"
on-click="_handleSave">Save</gr-button>
@@ -351,7 +345,6 @@
<template is="dom-if" if="[[!_hasHumanReply]]">
<gr-button
link
- secondary
class="action fix"
on-click="_handleFix"
disabled="[[robotButtonDisabled]]">
diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js
index 25f406e..5d2a1ca 100644
--- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js
+++ b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js
@@ -60,6 +60,7 @@
/**
* Because either the source text or the linkification config has changed,
* the content should be re-parsed.
+ *
* @param {string|null|undefined} content The raw, un-linkified source
* string to parse.
* @param {Object|null|undefined} config The server config specifying
@@ -90,6 +91,7 @@
* element should be created and attached to the resulting DOM.
* - To attach an arbitrary fragment: when called with only the `fragment`
* argument, the fragment should be attached to the resulting DOM as is.
+ *
* @param {string|null} text
* @param {string|null} href
* @param {DocumentFragment|undefined} fragment
diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js b/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js
index 61e28cc..1aa707f 100644
--- a/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js
+++ b/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js
@@ -19,6 +19,7 @@
/**
* Pattern describing URLs with supported protocols.
+ *
* @type {RegExp}
*/
const URL_PROTOCOL_PATTERN = /^(https?:\/\/|mailto:)/;
@@ -27,6 +28,7 @@
* Construct a parser for linkifying text. Will linkify plain URLs that appear
* in the text as well as custom links if any are specified in the linkConfig
* parameter.
+ *
* @param {Object|null|undefined} linkConfig Comment links as specified by the
* commentlinks field on a project config.
* @param {Function} callback The callback to be fired when an intermediate
@@ -45,6 +47,7 @@
/**
* Emit a callback to create a link element.
+ *
* @param {string} text The text of the link.
* @param {string} href The URL to use as the href of the link.
*/
@@ -56,6 +59,7 @@
/**
* Given the source text and a list of CommentLinkItem objects that were
* generated by the commentlinks config, emit parsing callbacks.
+ *
* @param {string} text The chuml of source text over which the outputArray
* items range.
* @param {!Array<Gerrit.CommentLinkItem>} outputArray The list of items to add
@@ -93,6 +97,7 @@
/**
* Sort the given array of CommentLinkItems such that the positions are in
* reverse order.
+ *
* @param {!Array<Gerrit.CommentLinkItem>} outputArray
*/
GrLinkTextParser.prototype.sortArrayReverse = function(outputArray) {
@@ -108,6 +113,7 @@
* - With the `html` paremeter provided, and the `text` and `href` parameters
* passed as `null`. In this case, the string of HTML will be parsed and the
* first resulting node will be used as the resulting content.
+ *
* @param {string|null} text The text to use if creating a link.
* @param {string|null} href The href to use as the URL if creating a link.
* @param {string|null} html The html to parse and use as the result.
@@ -150,6 +156,7 @@
/**
* Create a CommentLinkItem for a link and append it to the given output
* array.
+ *
* @param {string|null} text The text for the link.
* @param {string|null} href The href to use as the URL of the link.
* @param {number} position The position inside the source text where the link
@@ -172,6 +179,7 @@
/**
* Create a CommentLinkItem specified by an HTMl string and append it to the
* given output array.
+ *
* @param {string|null} html The html to parse and use as the result.
* @param {number} position The position inside the source text where the item
* starts.
@@ -192,6 +200,7 @@
/**
* Does the given range overlap with anything already in the item list.
+ *
* @param {number} position
* @param {number} length
* @param {!Array<Gerrit.CommentLinkItem>} outputArray
@@ -214,6 +223,7 @@
/**
* Parse the given source text and emit callbacks for the items that are
* parsed.
+ *
* @param {string} text
*/
GrLinkTextParser.prototype.parse = function(text) {
@@ -231,6 +241,7 @@
* ba-linkify has found a plain URL and wants it linkified.
* - With only a `text` parameter provided: this represents the non-link
* content that lies between the links the library has found.
+ *
* @param {string} text
* @param {string|null|undefined} href
*/
@@ -257,6 +268,7 @@
/**
* Walk over the given source text to find matches for comemntlink patterns
* and emit parse result callbacks.
+ *
* @param {string} text The raw source text.
* @param {Object|null|undefined} patterns A comment links specification
* object.
diff --git a/polygerrit-ui/app/lint_test.sh b/polygerrit-ui/app/lint_test.sh
index df37e54..4e7d8c9 100755
--- a/polygerrit-ui/app/lint_test.sh
+++ b/polygerrit-ui/app/lint_test.sh
@@ -37,4 +37,4 @@
# eslint installation.
npm link eslint eslint-config-google eslint-plugin-html eslint-plugin-jsdoc
-${eslint_bin} -c ${UI_PATH}/.eslintrc.json --ignore-pattern 'node_modules/' --ignore-pattern 'bower_components/' --ignore-pattern 'gr-linked-text' --ignore-pattern 'scripts/vendor' --ext .html,.js ${UI_PATH}
+${eslint_bin} -c ${UI_PATH}/.eslintrc.json --ignore-pattern 'node_modules/' --ignore-pattern 'bower_components/' --ignore-pattern 'scripts/vendor' --ext .html,.js ${UI_PATH}
diff --git a/polygerrit-ui/app/styles/themes/app-theme.html b/polygerrit-ui/app/styles/themes/app-theme.html
index 87230e6..53827b5 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.html
+++ b/polygerrit-ui/app/styles/themes/app-theme.html
@@ -36,7 +36,6 @@
--primary-button-text-color: white;
/* Used on text color for change list that doesn't need user's attention. */
--reviewed-text-color: black;
- --secondary-button-text-color: #212121;
--tooltip-text-color: white;
--vote-text-color-recommended: #388e3c;
--vote-text-color-disliked: #d32f2f;
@@ -52,7 +51,6 @@
--dialog-background-color: var(--background-color-primary);
--dropdown-background-color: var(--background-color-primary);
--expanded-background-color: var(--background-color-tertiary);
- --secondary-button-background-color: var(--background-color-primary);
--select-background-color: var(--background-color-secondary);
--shell-command-background-color: var(--background-color-secondary);
--shell-command-decoration-background-color: var(--background-color-tertiary);
diff --git a/polygerrit-ui/app/styles/themes/dark-theme.html b/polygerrit-ui/app/styles/themes/dark-theme.html
index 78bc48b..cfc1f62 100644
--- a/polygerrit-ui/app/styles/themes/dark-theme.html
+++ b/polygerrit-ui/app/styles/themes/dark-theme.html
@@ -36,7 +36,6 @@
--primary-button-text-color: var(--primary-text-color);
/* Used on text color for change list doesn't need user's attention. */
--reviewed-text-color: #dadce0;
- --secondary-button-text-color: var(--deemphasized-text-color);
--tooltip-text-color: white;
--vote-text-color-recommended: #388e3c;
--vote-text-color-disliked: #d32f2f;
@@ -51,12 +50,11 @@
/* unique background colors */
--assignee-highlight-color: #3a361c;
--comment-background-color: #0b162b;
- --robot-comment-background-color: #e8f0fe;
+ --robot-comment-background-color: rgba(232, 234, 237, 0.08);
--edit-mode-background-color: #5c0a36;
--emphasis-color: #383f4a;
--hover-background-color: rgba(161, 194, 250, 0.2);
--primary-button-background-color: var(--link-color);
- --secondary-button-background-color: var(--primary-text-color);
--selection-background-color: rgba(161, 194, 250, 0.1);
--tooltip-background-color: #111;
--unresolved-comment-background-color: #385a9a;
