Merge "Add algorithm description to FixCalculator"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index fc4b00a..82cb3f9 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1171,15 +1171,6 @@
+
Default is `ALL`.
-[[change.api.excludeMergeableInChangeInfo]]change.api.excludeMergeableInChangeInfo::
-+
-If true, the mergeability bit in
-link:rest-api-changes.html#change-info[ChangeInfo] will never be set. It can
-be requested separately through the
-link:rest-api-changes.html#get-mergeable[get-mergeable] endpoint.
-+
-Default is false.
-
[[change.cacheAutomerge]]change.cacheAutomerge::
+
When reviewing diff commits, the left-hand side shows the output of the
@@ -1195,6 +1186,15 @@
+
Default is true.
+[[change.commentSizeLimit]]change.commentSizeLimit::
++
+Maximum allowed size in characters of a regular (non-robot) comment. Comments
+which exceed this size will be rejected. Size computation is approximate and may
+be off by roughly 1%. Common unit suffixes of 'k', 'm', or 'g' are supported.
+The value must be positive.
++
+The default limit is 16kB.
+
[[change.disablePrivateChanges]]change.disablePrivateChanges::
+
If set to true, users are not allowed to create private changes.
@@ -1212,6 +1212,13 @@
+
By default 500.
+[[change.maxComments]]change.maxComments::
++
+Maximum number of comments (regular plus robot) allowed per change. Additional
+comments are rejected.
++
+By default 5,000.
+
[[change.maxUpdates]]change.maxUpdates::
+
Maximum number of updates to a change. Counts only updates to the main NoteDb
@@ -1232,6 +1239,34 @@
By default 1000.
+[[change.mergeabilityComputationBehavior]]change.mergeabilityComputationBehavior::
++
+This setting determines when Gerrit computes if a change is mergeable or not.
+This computation is expensive, especially when the repository is large or when
+there are many open changes.
++
+This config can have the following states:
++
+* `API_REF_UPDATED_AND_CHANGE_REINDEX`: Gerrit indexes `mergeability` enabling
+ the `is:mergeable` predicate in change search and allowing fast retrieval of
+ this bit in query responses. Gerrit will always serve `mergeable` in
+ link:rest-api-changes.html#change-info[ChangeInfo] objects.
+ Gerrit will reindex all open changes when the target ref advances (expensive).
+* `REF_UPDATED_AND_CHANGE_REINDEX`: Gerrit indexes `mergeability` enabling the
+ `is:mergeable` predicate in change search and allowing fast retrieval of this
+ bit in query responses. Gerrit will never serve `mergeable` in
+ link:rest-api-changes.html#change-info[ChangeInfo]
+ objects. This state can be a final state for instances that only want to
+ optimize the read path, but not the write path for speed or serve as an
+ intermediary step for instances that want to optimize both and need to migrate
+ callers of their API.
+ Gerrit will reindex all open changes when the target ref advances (expensive).
+* `NEVER`: Gerrit does not index `mergeable`, so `is:mergeable` is disabled as
+ query operator. Gerrit does not serve `mergeable` in
+ link:rest-api-changes.html#change-info[ChangeInfo].
+
+Default is `REF_UPDATED_AND_CHANGE_REINDEX`.
+
[[change.move]]change.move::
+
Whether the link:rest-api-changes.html#move-change[Move Change] REST
@@ -1266,11 +1301,10 @@
[[change.robotCommentSizeLimit]]change.robotCommentSizeLimit::
+
-Maximum allowed size of a robot comment that will be accepted. Robot comments
-which exceed the indicated size will be rejected on addition. The specified
-value is interpreted as the maximum size in bytes of the JSON representation of
-the robot comment. Common unit suffixes of 'k', 'm', or 'g' are supported.
-Zero or negative values allow robot comments of unlimited size.
+Maximum allowed size in characters of a robot comment. Robot comments which
+exceed this size will be rejected on addition. Size computation is approximate
+and may be off by roughly 1%. Common unit suffixes of 'k', 'm', or 'g' are
+supported. Zero or negative values allow robot comments of unlimited size.
+
The default limit is 1024kB.
@@ -1400,9 +1434,10 @@
+
By default `true`, meaning mergeable changes are auto-abandoned.
+
-If link:#index.change.indexMergeable[`index.change.indexMergeable`]
-is disabled, setting this option to `false` has no effect and it
-behaves as though it were set to `true`.
+If
+link:#change.mergeabilityComputationBehavior[`change.mergeabilityComputationBehavior`]
+is set to `NEVER`, setting this option to `false` has no effect and it behaves
+as though it were set to `true`.
[[changeCleanup.cleanupAccountPatchReview]]changeCleanup.cleanupAccountPatchReview::
+
@@ -2776,22 +2811,6 @@
If not set or set to a zero, defaults to the number of logical CPUs as returned
by the JVM. If set to a negative value, defaults to a direct executor.
-[[index.change.indexMergeable]]index.change.indexMergeable::
-+
-Specifies if `mergeable` should be index or not. Indexing this field enables
-queries that contain the mergeability operator (`is:mergeable`). If enabled,
-Gerrit will check if the change is mergeable into the target branch when
-reindexing a change. This is an expensive operation.
-+
-If true, Gerrit will reindex all open changes when the target ref advances.
-Depending on the frequency of updates to the ref and the number of open changes,
-this can be very expensive.
-+
-When this setting is changed from `false` to `true`, all changes need to be
-reindexed.
-+
-Defaults to true.
-
[[index.onlineUpgrade]]index.onlineUpgrade::
+
Whether to upgrade to new index schema versions while the server is
@@ -3102,7 +3121,7 @@
events.
+
This can be set to the set of minimal options that consumers of Gerrit's
-events need. A minimal set would be (`SKIP_MERGEABLE`,`SKIP_DIFFSTAT`).
+events need. A minimal set would be (`SKIP_DIFFSTAT`).
+
Every option that gets added here will have a performance impact. The
general recommendation is therefore to set this to a minimal set of
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt
index e3e9009..bb83d95 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -2428,8 +2428,8 @@
|Field Name |Description
|`change` |
link:rest-api-changes.html#change-info[ChangeInfo] entity describing the change
-on which one or more comments was deleted. Populated with only the
-link:rest-api-changes.html#skip_mergeable[SKIP_MERGEABLE] option.
+on which one or more comments was deleted. Populated with no change list
+options.
|`deleted` |
List of link:rest-api-changes.html#comment-info[CommentInfo] entities for each
comment that was deleted.
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 6da455d..0f36763 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -306,26 +306,6 @@
from the change owner, i.e. this change would show up in the results of
link:user-search.html#reviewedby[reviewedby:self].
--
-
-[[skip_mergeable]]
---
-* `SKIP_MERGEABLE`: skip the `mergeable` field in
-link:#change-info[ChangeInfo]. For fast moving projects, this field must
-be recomputed often, which is slow for projects with big trees.
-+
-When link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[
-`change.api.excludeMergeableInChangeInfo`] is set in the `gerrit.config`,
-the `mergeable` field will always be omitted and `SKIP_MERGEABLE` has no
-effect.
-+
-When link:config-gerrit.html#index.change.indexMergeable[
-`index.change.indexMergeable`] is set to `false` in the `gerrit.config`,
-the `mergeable` field will always be omitted when querying changes and
-`SKIP_MERGEABLE` has no effect.
-+
-A change's mergeability can be requested separately by calling the
-link:#get-mergeable[get-mergeable] endpoint.
---
[[skip_diffstat]]
--
* `SKIP_DIFFSTAT`: skip the 'insertions' and 'deletions' field in link:#change-info[ChangeInfo].
@@ -2098,8 +2078,8 @@
"name": "Code Analyzer",
"email": "code.analyzer@example.com"
},
- "robotId": "importChecker",
- "robotRunId": "76b1375aa8626ea7149792831fe2ed85e80d9e04"
+ "robot_id": "importChecker",
+ "robot_run_id": "76b1375aa8626ea7149792831fe2ed85e80d9e04"
},
{
"id": "TveXwFiA",
@@ -2111,8 +2091,8 @@
"name": "Code Analyzer",
"email": "code.analyzer@example.com"
},
- "robotId": "styleChecker",
- "robotRunId": "5c606c425dd45184484f9d0a2ffd725a7607839b"
+ "robot_id": "styleChecker",
+ "robot_run_id": "5c606c425dd45184484f9d0a2ffd725a7607839b"
}
]
}
@@ -4900,8 +4880,8 @@
"name": "Code Analyzer",
"email": "code.analyzer@example.com"
},
- "robotId": "importChecker",
- "robotRunId": "76b1375aa8626ea7149792831fe2ed85e80d9e04"
+ "robot_id": "importChecker",
+ "robot_run_id": "76b1375aa8626ea7149792831fe2ed85e80d9e04"
},
{
"id": "TveXwFiA",
@@ -4913,8 +4893,8 @@
"name": "Code Analyzer",
"email": "code.analyzer@example.com"
},
- "robotId": "styleChecker",
- "robotRunId": "5c606c425dd45184484f9d0a2ffd725a7607839b"
+ "robot_id": "styleChecker",
+ "robot_run_id": "5c606c425dd45184484f9d0a2ffd725a7607839b"
}
]
}
@@ -4954,8 +4934,8 @@
"name": "Code Analyzer",
"email": "code.analyzer@example.com"
},
- "robotId": "importChecker",
- "robotRunId": "76b1375aa8626ea7149792831fe2ed85e80d9e04"
+ "robot_id": "importChecker",
+ "robot_run_id": "76b1375aa8626ea7149792831fe2ed85e80d9e04"
}
----
@@ -5964,10 +5944,9 @@
Not set for merged changes.
|`mergeable` |optional|
Whether the change is mergeable. +
-Not set for merged changes, if the change has not yet been tested, or
-if the link:#skip_mergeable[skip_mergeable] option is set or when
-link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[change.api.excludeMergeableInChangeInfo]
-is set.
+Only set for open changes if
+link:config-gerrit.html#change.mergeabilityComputationBehavior[change.mergeabilityComputationBehavior]
+is `API_REF_UPDATED_AND_CHANGE_REINDEX`.
|`submittable` |optional|
Whether the change has been approved by the project submit rules. +
Only set if link:#submittable[requested].
@@ -6848,7 +6827,7 @@
|`subject` |optional|
The new subject for the change, if not specified, will reuse the current patch
set's subject
-|`inherit_parent` |optional, default to `false`|
+|`inherit_parent` |optional, default to `false`|
Use the current patch set's first parent as the merge tip when set to `true`.
|`base_change` |optional|
A link:#change-id[\{change-id\}] that identifies a change. When `inherit_parent`
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index e9e3b94..828674f 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -1566,10 +1566,11 @@
the whole topic is submitted].
|`disable_private_changes` |not set if `false`|
Returns true if private changes are disabled.
-|`exclude_mergeable_in_change_info` |not set if `false`|
-Value of the link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[
+|`mergeability_computation_behavior` ||
+Value of the link:config-gerrit.html#change.mergeabilityComputationBehavior[
configuration parameter] that controls whether the mergeability bit in
-link:rest-api-changes.html#change-info[ChangeInfo] will never be set.
+link:rest-api-changes.html#change-info[ChangeInfo] will never be set and if the
+bit is indexed.
|=============================
[[change-index-config-info]]
diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD
index 135a80e..b23aca1 100644
--- a/java/com/google/gerrit/acceptance/BUILD
+++ b/java/com/google/gerrit/acceptance/BUILD
@@ -1,5 +1,4 @@
load("@rules_java//java:defs.bzl", "java_binary", "java_library")
-load("//tools/bzl:java.bzl", "java_library2")
load("//tools/bzl:javadoc.bzl", "java_doc")
FUNCTION_SRCS = [
@@ -103,28 +102,30 @@
runtime_deps = DEPLOY_ENV,
)
-java_library2(
+exported_deps = [
+ ":function",
+ "//lib:jgit-junit",
+ "//lib:jimfs",
+ "//lib:servlet-api",
+ "//lib/httpcomponents:fluent-hc",
+ "//lib/httpcomponents:httpclient",
+ "//lib/httpcomponents:httpcore",
+ "//lib/mockito",
+ "//lib/truth",
+ "//lib/truth:truth-java8-extension",
+ "//lib/greenmail",
+] + TEST_DEPS
+
+java_library(
name = "framework-lib",
testonly = True,
srcs = glob(
["**/*.java"],
exclude = FUNCTION_SRCS,
),
- exported_deps = [
- ":function",
- "//lib:jgit-junit",
- "//lib:jimfs",
- "//lib:servlet-api",
- "//lib/httpcomponents:fluent-hc",
- "//lib/httpcomponents:httpclient",
- "//lib/httpcomponents:httpcore",
- "//lib/mockito",
- "//lib/truth",
- "//lib/truth:truth-java8-extension",
- "//lib/greenmail",
- ] + TEST_DEPS,
visibility = ["//visibility:public"],
- deps = DEPLOY_ENV,
+ exports = exported_deps,
+ deps = DEPLOY_ENV + exported_deps,
)
java_library(
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index f90df67..95afad7 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -571,8 +571,8 @@
cfg.setInt("sshd", null, "commandStartThreads", 1);
cfg.setInt("receive", null, "threadPoolSize", 1);
cfg.setInt("index", null, "threads", 1);
- if (cfg.getString("index", "change", "indexMergeable") == null) {
- cfg.setBoolean("index", "change", "indexMergeable", false);
+ if (cfg.getString("index", null, "mergeabilityComputationBehavior") == null) {
+ cfg.setString("index", null, "mergeabilityComputationBehavior", "NEVER");
}
}
diff --git a/java/com/google/gerrit/acceptance/InProcessProtocol.java b/java/com/google/gerrit/acceptance/InProcessProtocol.java
index 8a46d57..8fcc1e7 100644
--- a/java/com/google/gerrit/acceptance/InProcessProtocol.java
+++ b/java/com/google/gerrit/acceptance/InProcessProtocol.java
@@ -232,7 +232,7 @@
try {
perm.check(ProjectPermission.RUN_UPLOAD_PACK);
} catch (AuthException e) {
- throw new ServiceNotAuthorizedException();
+ throw new ServiceNotAuthorizedException(e.getMessage(), e);
} catch (PermissionBackendException e) {
throw new RuntimeException(e);
}
@@ -307,7 +307,7 @@
.project(req.project)
.check(ProjectPermission.RUN_RECEIVE_PACK);
} catch (AuthException e) {
- throw new ServiceNotAuthorizedException();
+ throw new ServiceNotAuthorizedException(e.getMessage(), e);
} catch (PermissionBackendException e) {
throw new RuntimeException(e);
}
diff --git a/java/com/google/gerrit/acceptance/StandaloneSiteTest.java b/java/com/google/gerrit/acceptance/StandaloneSiteTest.java
index 1d6f51f..da93610 100644
--- a/java/com/google/gerrit/acceptance/StandaloneSiteTest.java
+++ b/java/com/google/gerrit/acceptance/StandaloneSiteTest.java
@@ -220,8 +220,11 @@
try {
status = p.waitFor();
} catch (InterruptedException e) {
- throw new InterruptedIOException(
- "interrupted waiting for: " + Joiner.on(' ').join(pb.command()));
+ InterruptedIOException iioe =
+ new InterruptedIOException(
+ "interrupted waiting for: " + Joiner.on(' ').join(pb.command()));
+ iioe.initCause(e);
+ throw iioe;
}
String result = new String(out, UTF_8);
diff --git a/java/com/google/gerrit/common/data/CommentDetail.java b/java/com/google/gerrit/common/data/CommentDetail.java
index d69f0bb..55e0143 100644
--- a/java/com/google/gerrit/common/data/CommentDetail.java
+++ b/java/com/google/gerrit/common/data/CommentDetail.java
@@ -18,10 +18,7 @@
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.PatchSet;
import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
import java.util.List;
-import java.util.Map;
public class CommentDetail {
protected List<Comment> a;
@@ -29,8 +26,6 @@
private transient PatchSet.Id idA;
private transient PatchSet.Id idB;
- private transient Map<Integer, List<Comment>> forA;
- private transient Map<Integer, List<Comment>> forB;
public CommentDetail(PatchSet.Id idA, PatchSet.Id idB) {
this.a = new ArrayList<>();
@@ -67,88 +62,4 @@
public boolean isEmpty() {
return a.isEmpty() && b.isEmpty();
}
-
- public List<Comment> getForA(int lineNbr) {
- if (forA == null) {
- forA = index(a);
- }
- return get(forA, lineNbr);
- }
-
- public List<Comment> getForB(int lineNbr) {
- if (forB == null) {
- forB = index(b);
- }
- return get(forB, lineNbr);
- }
-
- private static List<Comment> get(Map<Integer, List<Comment>> m, int i) {
- List<Comment> r = m.get(i);
- return r != null ? orderComments(r) : Collections.emptyList();
- }
-
- /**
- * Order the comments based on their parent_uuid parent. It is possible to do this by iterating
- * over the list only once but it's probably overkill since the number of comments on a given line
- * will be small most of the time.
- *
- * @param comments The list of comments for a given line.
- * @return The comments sorted as they should appear in the UI
- */
- private static List<Comment> orderComments(List<Comment> comments) {
- // Map of comments keyed by their parent. The values are lists of comments since it is
- // possible for several comments to have the same parent (this can happen if two reviewers
- // click Reply on the same comment at the same time). Such comments will be displayed under
- // their correct parent in chronological order.
- Map<String, List<Comment>> parentMap = new HashMap<>();
-
- // It's possible to have more than one root comment if two reviewers create a comment on the
- // same line at the same time
- List<Comment> rootComments = new ArrayList<>();
-
- // Store all the comments in parentMap, keyed by their parent
- for (Comment c : comments) {
- String parentUuid = c.parentUuid;
- List<Comment> l = parentMap.get(parentUuid);
- if (l == null) {
- l = new ArrayList<>();
- parentMap.put(parentUuid, l);
- }
- l.add(c);
- if (parentUuid == null) {
- rootComments.add(c);
- }
- }
-
- // Add the comments in the list, starting with the head and then going through all the
- // comments that have it as a parent, and so on
- List<Comment> result = new ArrayList<>();
- addChildren(parentMap, rootComments, result);
-
- return result;
- }
-
- /** Add the comments to {@code outResult}, depth first */
- private static void addChildren(
- Map<String, List<Comment>> parentMap, List<Comment> children, List<Comment> outResult) {
- if (children != null) {
- for (Comment c : children) {
- outResult.add(c);
- addChildren(parentMap, parentMap.get(c.key.uuid), outResult);
- }
- }
- }
-
- private Map<Integer, List<Comment>> index(List<Comment> in) {
- HashMap<Integer, List<Comment>> r = new HashMap<>();
- for (Comment p : in) {
- List<Comment> l = r.get(p.lineNbr);
- if (l == null) {
- l = new ArrayList<>();
- r.put(p.lineNbr, l);
- }
- l.add(p);
- }
- return r;
- }
}
diff --git a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java
index 084c2ec..f659bca 100644
--- a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java
+++ b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java
@@ -49,6 +49,7 @@
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.change.MergeabilityComputationBehavior;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.IndexUtils;
@@ -111,7 +112,7 @@
this.idField =
this.schema.useLegacyNumericFields() ? ChangeField.LEGACY_ID : ChangeField.LEGACY_ID_STR;
this.skipFields =
- gerritConfig.getBoolean("index", "change", "indexMergeable", true)
+ MergeabilityComputationBehavior.fromConfig(gerritConfig).includeInIndex()
? ImmutableSet.of()
: ImmutableSet.of(ChangeField.MERGEABLE.getName());
}
diff --git a/java/com/google/gerrit/entities/Comment.java b/java/com/google/gerrit/entities/Comment.java
index 55d739a..9c58fef 100644
--- a/java/com/google/gerrit/entities/Comment.java
+++ b/java/com/google/gerrit/entities/Comment.java
@@ -29,6 +29,8 @@
*
* <p>Changing fields in this class changes the storage format of inline comments in NoteDb and may
* require a corresponding data migration (adding new optional fields is generally okay).
+ *
+ * <p>Consider updating {@link #getApproximateSize()} when adding/changing fields.
*/
public class Comment {
public enum Status {
@@ -56,7 +58,7 @@
}
}
- public static class Key {
+ public static final class Key {
public String uuid;
public String filename;
public int patchSetId;
@@ -97,7 +99,7 @@
}
}
- public static class Identity {
+ public static final class Identity {
int id;
public Identity(Account.Id id) {
@@ -147,7 +149,7 @@
* </ul>
* </ul>
*/
- public static class Range implements Comparable<Range> {
+ public static final class Range implements Comparable<Range> {
private static final Comparator<Range> RANGE_COMPARATOR =
Comparator.<Range>comparingInt(range -> range.startLine)
.thenComparingInt(range -> range.startChar)
@@ -220,10 +222,12 @@
public Range range;
public String tag;
- // Hex commit SHA1 of the commit of the patchset to which this comment applies. Other classes call
- // this "commitId", but this class uses the old ReviewDb term "revId", and this field name is
- // serialized into JSON in NoteDb, so it can't easily be changed. Callers do not access this field
- // directly, and instead use the public getter/setter that wraps an ObjectId.
+ /**
+ * Hex commit SHA1 of the commit of the patchset to which this comment applies. Other classes call
+ * this "commitId", but this class uses the old ReviewDb term "revId", and this field name is
+ * serialized into JSON in NoteDb, so it can't easily be changed. Callers do not access this field
+ * directly, and instead use the public getter/setter that wraps an ObjectId.
+ */
private String revId;
public String serverId;
@@ -293,6 +297,23 @@
return realAuthor != null ? realAuthor : author;
}
+ /**
+ * Returns the comment's approximate size. This is used to enforce size limits and should
+ * therefore include all unbounded fields (e.g. String-s).
+ */
+ public int getApproximateSize() {
+ return nullableLength(message, parentUuid, tag, revId, serverId)
+ + (key != null ? nullableLength(key.filename, key.uuid) : 0);
+ }
+
+ static int nullableLength(String... strings) {
+ int length = 0;
+ for (String s : strings) {
+ length += s == null ? 0 : s.length();
+ }
+ return length;
+ }
+
@Override
public boolean equals(Object o) {
if (!(o instanceof Comment)) {
diff --git a/java/com/google/gerrit/entities/FixReplacement.java b/java/com/google/gerrit/entities/FixReplacement.java
index 046300e..fbbf746 100644
--- a/java/com/google/gerrit/entities/FixReplacement.java
+++ b/java/com/google/gerrit/entities/FixReplacement.java
@@ -14,10 +14,10 @@
package com.google.gerrit.entities;
-public class FixReplacement {
- public String path;
- public Comment.Range range;
- public String replacement;
+public final class FixReplacement {
+ public final String path;
+ public final Comment.Range range;
+ public final String replacement;
public FixReplacement(String path, Comment.Range range, String replacement) {
this.path = path;
@@ -38,4 +38,9 @@
+ '\''
+ '}';
}
+
+ /** Returns this instance's approximate size in bytes for the purpose of applying size limits. */
+ int getApproximateSize() {
+ return path.length() + replacement.length();
+ }
}
diff --git a/java/com/google/gerrit/entities/FixSuggestion.java b/java/com/google/gerrit/entities/FixSuggestion.java
index ac4e720..c88afe7 100644
--- a/java/com/google/gerrit/entities/FixSuggestion.java
+++ b/java/com/google/gerrit/entities/FixSuggestion.java
@@ -16,10 +16,10 @@
import java.util.List;
-public class FixSuggestion {
- public String fixId;
- public String description;
- public List<FixReplacement> replacements;
+public final class FixSuggestion {
+ public final String fixId;
+ public final String description;
+ public final List<FixReplacement> replacements;
public FixSuggestion(String fixId, String description, List<FixReplacement> replacements) {
this.fixId = fixId;
@@ -40,4 +40,11 @@
+ replacements
+ '}';
}
+
+ /** Returns this instance's approximate size in bytes for the purpose of applying size limits. */
+ int getApproximateSize() {
+ return fixId.length()
+ + description.length()
+ + replacements.stream().map(FixReplacement::getApproximateSize).reduce(0, Integer::sum);
+ }
}
diff --git a/java/com/google/gerrit/entities/KeyUtil.java b/java/com/google/gerrit/entities/KeyUtil.java
index d000b31..40fb757 100644
--- a/java/com/google/gerrit/entities/KeyUtil.java
+++ b/java/com/google/gerrit/entities/KeyUtil.java
@@ -97,7 +97,7 @@
}
}
} catch (ArrayIndexOutOfBoundsException err) {
- throw new IllegalArgumentException("Bad encoding: " + e);
+ throw new IllegalArgumentException("Bad encoding" + e, err);
}
try {
return new String(b, 0, bPtr, "UTF-8");
diff --git a/java/com/google/gerrit/entities/RobotComment.java b/java/com/google/gerrit/entities/RobotComment.java
index a7951ad..4a17566 100644
--- a/java/com/google/gerrit/entities/RobotComment.java
+++ b/java/com/google/gerrit/entities/RobotComment.java
@@ -19,7 +19,7 @@
import java.util.Map;
import java.util.Objects;
-public class RobotComment extends Comment {
+public final class RobotComment extends Comment {
public String robotId;
public String robotRunId;
public String url;
@@ -41,6 +41,22 @@
}
@Override
+ public int getApproximateSize() {
+ int approximateSize = super.getApproximateSize() + nullableLength(robotId, robotRunId, url);
+ approximateSize +=
+ properties != null
+ ? properties.entrySet().stream()
+ .map(entry -> nullableLength(entry.getKey(), entry.getValue()))
+ .reduce(0, Integer::sum)
+ : 0;
+ approximateSize +=
+ fixSuggestions != null
+ ? fixSuggestions.stream().map(FixSuggestion::getApproximateSize).reduce(0, Integer::sum)
+ : 0;
+ return approximateSize;
+ }
+
+ @Override
public String toString() {
return toStringHelper()
.add("robotId", robotId)
diff --git a/java/com/google/gerrit/exceptions/InvalidSshKeyException.java b/java/com/google/gerrit/exceptions/InvalidSshKeyException.java
index 8baba20..dda80cf 100644
--- a/java/com/google/gerrit/exceptions/InvalidSshKeyException.java
+++ b/java/com/google/gerrit/exceptions/InvalidSshKeyException.java
@@ -23,4 +23,8 @@
public InvalidSshKeyException() {
super(MESSAGE);
}
+
+ public InvalidSshKeyException(Throwable cause) {
+ super(MESSAGE, cause);
+ }
}
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
index 119d941..96455a6 100644
--- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -259,17 +259,12 @@
*
* <ul>
* <li>{@code CHECK} is omitted, to skip consistency checks.
- * <li>{@code SKIP_MERGEABLE} is omitted, so the {@code mergeable} bit <em>is</em> set.
* <li>{@code SKIP_DIFFSTAT} is omitted to ensure diffstat calculations.
* </ul>
*/
default ChangeInfo get() throws RestApiException {
return get(
- EnumSet.complementOf(
- EnumSet.of(
- ListChangesOption.CHECK,
- ListChangesOption.SKIP_MERGEABLE,
- ListChangesOption.SKIP_DIFFSTAT)));
+ EnumSet.complementOf(EnumSet.of(ListChangesOption.CHECK, ListChangesOption.SKIP_DIFFSTAT)));
}
/** {@link #get(ListChangesOption...)} with no options included. */
diff --git a/java/com/google/gerrit/extensions/client/ListChangesOption.java b/java/com/google/gerrit/extensions/client/ListChangesOption.java
index 425265b..6071cc7 100644
--- a/java/com/google/gerrit/extensions/client/ListChangesOption.java
+++ b/java/com/google/gerrit/extensions/client/ListChangesOption.java
@@ -74,7 +74,11 @@
/** If tracking Ids are included, include detailed tracking Ids info. */
TRACKING_IDS(21),
- /** Skip mergeability data */
+ /**
+ * Use {@code gerrit.config} instead to turn this off for your instance. See {@code
+ * change.mergeabilityComputationBehavior}.
+ */
+ @Deprecated
SKIP_MERGEABLE(22),
/**
diff --git a/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java b/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java
index 67d0b0d9..66a2dd5 100644
--- a/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java
@@ -24,5 +24,5 @@
public String replyTooltip;
public int updateDelay;
public Boolean submitWholeTopic;
- public Boolean excludeMergeableInChangeInfo;
+ public String mergeabilityComputationBehavior;
}
diff --git a/java/com/google/gerrit/extensions/common/ChangeIndexConfigInfo.java b/java/com/google/gerrit/extensions/common/ChangeIndexConfigInfo.java
deleted file mode 100644
index 0cce106..0000000
--- a/java/com/google/gerrit/extensions/common/ChangeIndexConfigInfo.java
+++ /dev/null
@@ -1,22 +0,0 @@
-// Copyright (C) 2019 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.extensions.common;
-
-/**
- * API response containing values from the {@code index.change} section of {@code gerrit.config}.
- */
-public class ChangeIndexConfigInfo {
- public Boolean indexMergeable;
-}
diff --git a/java/com/google/gerrit/extensions/common/IndexConfigInfo.java b/java/com/google/gerrit/extensions/common/IndexConfigInfo.java
deleted file mode 100644
index f3f510b..0000000
--- a/java/com/google/gerrit/extensions/common/IndexConfigInfo.java
+++ /dev/null
@@ -1,20 +0,0 @@
-// Copyright (C) 2019 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.extensions.common;
-
-/** API response containing values from the {@code index} section of {@code gerrit.config}. */
-public class IndexConfigInfo {
- public ChangeIndexConfigInfo change;
-}
diff --git a/java/com/google/gerrit/extensions/common/ServerInfo.java b/java/com/google/gerrit/extensions/common/ServerInfo.java
index 555c363..bc7fcfd 100644
--- a/java/com/google/gerrit/extensions/common/ServerInfo.java
+++ b/java/com/google/gerrit/extensions/common/ServerInfo.java
@@ -21,7 +21,6 @@
public ChangeConfigInfo change;
public DownloadInfo download;
public GerritInfo gerrit;
- public IndexConfigInfo index;
public Boolean noteDbEnabled;
public PluginConfigInfo plugin;
public SshdInfo sshd;
diff --git a/java/com/google/gerrit/extensions/restapi/MethodNotAllowedException.java b/java/com/google/gerrit/extensions/restapi/MethodNotAllowedException.java
index 2d59f27..8422b61 100644
--- a/java/com/google/gerrit/extensions/restapi/MethodNotAllowedException.java
+++ b/java/com/google/gerrit/extensions/restapi/MethodNotAllowedException.java
@@ -22,4 +22,12 @@
public MethodNotAllowedException(String msg) {
super(msg);
}
+
+ /**
+ * @param msg error text for client describing why the method is not allowed.
+ * @param cause reason for the method not being allowed.
+ */
+ public MethodNotAllowedException(String msg, Throwable cause) {
+ super(msg, cause);
+ }
}
diff --git a/java/com/google/gerrit/extensions/restapi/NotImplementedException.java b/java/com/google/gerrit/extensions/restapi/NotImplementedException.java
index 566159d..d74986a7 100644
--- a/java/com/google/gerrit/extensions/restapi/NotImplementedException.java
+++ b/java/com/google/gerrit/extensions/restapi/NotImplementedException.java
@@ -25,4 +25,8 @@
public NotImplementedException(String message) {
super(message);
}
+
+ public NotImplementedException(String message, Throwable cause) {
+ super(message, cause);
+ }
}
diff --git a/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java b/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java
index 75cf713..270a040 100644
--- a/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java
+++ b/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java
@@ -34,6 +34,10 @@
super("Not found: " + id.get());
}
+ public ResourceNotFoundException(IdString id, Throwable cause) {
+ super("Not found: " + id.get(), cause);
+ }
+
public ResourceNotFoundException caching(CacheControl c) {
setCaching(c);
return this;
diff --git a/java/com/google/gerrit/extensions/validators/CommentForValidation.java b/java/com/google/gerrit/extensions/validators/CommentForValidation.java
index 51ae5ae..9b91dff 100644
--- a/java/com/google/gerrit/extensions/validators/CommentForValidation.java
+++ b/java/com/google/gerrit/extensions/validators/CommentForValidation.java
@@ -17,13 +17,21 @@
import com.google.auto.value.AutoValue;
/**
- * Holds a comment's text and {@link CommentType} in order to pass it to a validation plugin.
+ * Holds a comment's text and some metadata in order to pass it to a validation plugin.
*
* @see CommentValidator
*/
@AutoValue
public abstract class CommentForValidation {
+ /** The creator of the comment. */
+ public enum CommentSource {
+ /** A regular user comment. */
+ HUMAN,
+ /** A robot comment. */
+ ROBOT
+ }
+
/** The type of comment. */
public enum CommentType {
/** A regular (inline) comment. */
@@ -34,14 +42,27 @@
CHANGE_MESSAGE
}
- public static CommentForValidation create(CommentType type, String text) {
- return new AutoValue_CommentForValidation(type, text);
+ public static CommentForValidation create(
+ CommentSource source, CommentType type, String text, long size) {
+ return new AutoValue_CommentForValidation(source, type, text, size);
}
+ public abstract CommentSource getSource();
+
public abstract CommentType getType();
+ /**
+ * Returns the comment text. Note that especially for robot comments the total size may be
+ * significantly larger and should be determined by using {@link #getApproximateSize()}.
+ */
public abstract String getText();
+ /**
+ * Returns this instance's approximate size in bytes for the purpose of applying size limits. For
+ * robot comments this may be significantly larger than the size of the comment text.
+ */
+ public abstract long getApproximateSize();
+
public CommentValidationFailure failValidation(String message) {
return CommentValidationFailure.create(this, message);
}
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidationContext.java b/java/com/google/gerrit/extensions/validators/CommentValidationContext.java
index 1cb00e3..db08058 100644
--- a/java/com/google/gerrit/extensions/validators/CommentValidationContext.java
+++ b/java/com/google/gerrit/extensions/validators/CommentValidationContext.java
@@ -34,16 +34,7 @@
/** Returns the project the comment is being added to. */
public abstract String getProject();
- public static Builder builder() {
- return new AutoValue_CommentValidationContext.Builder();
- }
-
- @AutoValue.Builder
- public abstract static class Builder {
- public abstract Builder changeId(int value);
-
- public abstract Builder project(String value);
-
- public abstract CommentValidationContext build();
+ public static CommentValidationContext create(int changeId, String project) {
+ return new AutoValue_CommentValidationContext(changeId, project);
}
}
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidator.java b/java/com/google/gerrit/extensions/validators/CommentValidator.java
index ba73e46..2dfd6e2 100644
--- a/java/com/google/gerrit/extensions/validators/CommentValidator.java
+++ b/java/com/google/gerrit/extensions/validators/CommentValidator.java
@@ -25,7 +25,10 @@
public interface CommentValidator {
/**
- * Validate the specified comments.
+ * Validate the specified comments. This method will be called with all new comments that need to
+ * be validated. This allows validators to statelessly count the new comments. Note that the
+ * method may be called separately for texts that are not comments, but similar in nature and also
+ * subject to size limits, such as a change message.
*
* @return An empty list if all comments are valid, or else a list of validation failures.
*/
diff --git a/java/com/google/gerrit/httpd/HtmlDomUtil.java b/java/com/google/gerrit/httpd/HtmlDomUtil.java
index 25ae71c..57f2664 100644
--- a/java/com/google/gerrit/httpd/HtmlDomUtil.java
+++ b/java/com/google/gerrit/httpd/HtmlDomUtil.java
@@ -131,7 +131,7 @@
try {
d = newBuilder().newDocument();
} catch (ParserConfigurationException e) {
- throw new IOException("Cannot clone document");
+ throw new IOException("Cannot clone document", e);
}
Node n = d.importNode(doc.getDocumentElement(), true);
d.appendChild(n);
diff --git a/java/com/google/gerrit/httpd/ProjectOAuthFilter.java b/java/com/google/gerrit/httpd/ProjectOAuthFilter.java
index 693232f..dab36c4 100644
--- a/java/com/google/gerrit/httpd/ProjectOAuthFilter.java
+++ b/java/com/google/gerrit/httpd/ProjectOAuthFilter.java
@@ -195,7 +195,7 @@
defaultAuthPlugin = loginProvider.getPluginName();
defaultAuthProvider = loginProvider.getExportName();
} catch (NoSuchElementException e) {
- throw new ServletException("No OAuth login provider installed");
+ throw new ServletException("No OAuth login provider installed", e);
} catch (IllegalArgumentException e) {
// multiple providers found => do not pick any
}
diff --git a/java/com/google/gerrit/httpd/plugins/PluginServletContext.java b/java/com/google/gerrit/httpd/plugins/PluginServletContext.java
index 6a8ef32..40083e4 100644
--- a/java/com/google/gerrit/httpd/plugins/PluginServletContext.java
+++ b/java/com/google/gerrit/httpd/plugins/PluginServletContext.java
@@ -61,9 +61,11 @@
try {
handler = API.class.getDeclaredMethod(method.getName(), method.getParameterTypes());
} catch (NoSuchMethodException e) {
- throw new NoSuchMethodError(
+ String msg =
String.format(
- "%s does not implement %s", PluginServletContext.class, method.toGenericString()));
+ "%s does not implement %s", PluginServletContext.class, method.toGenericString());
+ logger.atSevere().withCause(e).log(msg);
+ throw new NoSuchMethodError(msg);
}
return handler.invoke(this, args);
}
diff --git a/java/com/google/gerrit/httpd/raw/BazelBuild.java b/java/com/google/gerrit/httpd/raw/BazelBuild.java
index a13078d..7677e97 100644
--- a/java/com/google/gerrit/httpd/raw/BazelBuild.java
+++ b/java/com/google/gerrit/httpd/raw/BazelBuild.java
@@ -63,8 +63,9 @@
try {
status = rebuild.waitFor();
} catch (InterruptedException e) {
- throw new InterruptedIOException(
- "interrupted waiting for: " + Joiner.on(' ').join(proc.command()));
+ String msg = "interrupted waiting for: " + Joiner.on(' ').join(proc.command());
+ logger.atSevere().withCause(e).log(msg);
+ throw new InterruptedIOException(msg);
}
if (status != 0) {
logger.atWarning().log("build failed: %s", new String(out, UTF_8));
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 722955c..a81a14e6 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -1182,7 +1182,7 @@
try {
first = json.peek();
} catch (EOFException e) {
- throw new BadRequestException("Expected JSON object");
+ throw new BadRequestException("Expected JSON object", e);
}
if (first == JsonToken.STRING) {
return parseString(json.nextString(), type);
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 3b277dd..375967b 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -55,6 +55,7 @@
import com.google.gerrit.index.query.ResultSet;
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.change.MergeabilityComputationBehavior;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.IndexExecutor;
@@ -182,7 +183,7 @@
this.changeDataFactory = changeDataFactory;
this.schema = schema;
this.skipFields =
- cfg.getBoolean("index", "change", "indexMergeable", true)
+ MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex()
? ImmutableSet.of()
: ImmutableSet.of(ChangeField.MERGEABLE.getName());
diff --git a/java/com/google/gerrit/lucene/QueryBuilder.java b/java/com/google/gerrit/lucene/QueryBuilder.java
index e8ef95f..7d82bf5 100644
--- a/java/com/google/gerrit/lucene/QueryBuilder.java
+++ b/java/com/google/gerrit/lucene/QueryBuilder.java
@@ -208,7 +208,7 @@
// subclasses of OperatorPredicate.
value = Integer.parseInt(p.getValue());
} catch (NumberFormatException e) {
- throw new QueryParseException("not an integer: " + p.getValue());
+ throw new QueryParseException("not an integer: " + p.getValue(), e);
}
return intTermQuery.get(p.getField().getName(), value);
}
diff --git a/java/com/google/gerrit/pgm/BUILD b/java/com/google/gerrit/pgm/BUILD
index 03b7c3d..aa4df82 100644
--- a/java/com/google/gerrit/pgm/BUILD
+++ b/java/com/google/gerrit/pgm/BUILD
@@ -23,7 +23,7 @@
"//java/com/google/gerrit/lifecycle",
"//java/com/google/gerrit/lucene",
"//java/com/google/gerrit/metrics/dropwizard",
- "//java/com/google/gerrit/pgm/http",
+ "//java/com/google/gerrit/pgm/http/jetty",
"//java/com/google/gerrit/pgm/init",
"//java/com/google/gerrit/pgm/init/api",
"//java/com/google/gerrit/pgm/util",
diff --git a/java/com/google/gerrit/pgm/http/BUILD b/java/com/google/gerrit/pgm/http/BUILD
deleted file mode 100644
index 34115ae..0000000
--- a/java/com/google/gerrit/pgm/http/BUILD
+++ /dev/null
@@ -1,7 +0,0 @@
-load("@rules_java//java:defs.bzl", "java_library")
-
-java_library(
- name = "http",
- visibility = ["//visibility:public"],
- exports = ["//java/com/google/gerrit/pgm/http/jetty"],
-)
diff --git a/java/com/google/gerrit/pgm/init/BaseInit.java b/java/com/google/gerrit/pgm/init/BaseInit.java
index 7fbf2d7..62ff66a 100644
--- a/java/com/google/gerrit/pgm/init/BaseInit.java
+++ b/java/com/google/gerrit/pgm/init/BaseInit.java
@@ -332,7 +332,7 @@
IoUtil.loadJARs(secureStoreLib);
return new SecureStoreInitData(secureStoreLib, secureStores.get(0));
} catch (IOException e) {
- throw new InvalidSecureStoreException(String.format("%s is not a valid jar", secureStore));
+ throw new InvalidSecureStoreException(String.format("%s is not a valid jar", secureStore), e);
}
}
diff --git a/java/com/google/gerrit/pgm/init/InitHttpd.java b/java/com/google/gerrit/pgm/init/InitHttpd.java
index 027a3ac..b9fe874 100644
--- a/java/com/google/gerrit/pgm/init/InitHttpd.java
+++ b/java/com/google/gerrit/pgm/init/InitHttpd.java
@@ -169,7 +169,7 @@
uri = new URI(s + "://" + uri.getHost() + uri.getPath());
}
} catch (URISyntaxException e) {
- throw die("invalid httpd.listenUrl");
+ throw die("invalid httpd.listenUrl", e);
}
httpd.set("listenUrl", urlbuf.toString());
gerrit.string("Canonical URL", "canonicalWebUrl", uri.toString());
diff --git a/java/com/google/gerrit/server/ApprovalsUtil.java b/java/com/google/gerrit/server/ApprovalsUtil.java
index 29a5748..58b601f 100644
--- a/java/com/google/gerrit/server/ApprovalsUtil.java
+++ b/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -331,7 +331,7 @@
forChange.check(new LabelPermission.WithValue(name, value));
} catch (AuthException e) {
throw new AuthException(
- String.format("applying label \"%s\": %d is restricted", name, value));
+ String.format("applying label \"%s\": %d is restricted", name, value), e);
}
}
}
diff --git a/java/com/google/gerrit/server/PublishCommentsOp.java b/java/com/google/gerrit/server/PublishCommentsOp.java
index 57d50fb..df57629 100644
--- a/java/com/google/gerrit/server/PublishCommentsOp.java
+++ b/java/com/google/gerrit/server/PublishCommentsOp.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server;
-import com.google.common.flogger.FluentLogger;
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.PatchSet;
@@ -45,8 +45,6 @@
* necessary event for this.
*/
public class PublishCommentsOp implements BatchUpdateOp {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final PatchSetUtil psUtil;
private final ChangeNotes.Factory changeNotesFactory;
private final ChangeMessagesUtil cmUtil;
@@ -118,18 +116,14 @@
if (notify.shouldNotify()) {
email.create(notify, changeNotes, ps, user, message, comments, null, labelDelta).sendAsync();
}
- try {
- commentAdded.fire(
- changeNotes.getChange(),
- ps,
- ctx.getAccount(),
- message.getMessage(),
- null,
- null,
- ctx.getWhen());
- } catch (Exception e) {
- logger.atWarning().withCause(e).log("comment-added event invocation failed");
- }
+ commentAdded.fire(
+ changeNotes.getChange(),
+ ps,
+ ctx.getAccount(),
+ message.getMessage(),
+ ImmutableMap.of(),
+ ImmutableMap.of(),
+ ctx.getWhen());
}
private boolean insertMessage(ChangeContext ctx, ChangeUpdate changeUpdate) {
diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java
index 8137b92..6a16a53 100644
--- a/java/com/google/gerrit/server/account/AccountManager.java
+++ b/java/com/google/gerrit/server/account/AccountManager.java
@@ -391,7 +391,7 @@
try {
groupsUpdate.updateGroup(groupUuid, groupUpdate);
} catch (NoSuchGroupException e) {
- throw new AccountException(String.format("Group %s not found", groupUuid));
+ throw new AccountException(String.format("Group %s not found", groupUuid), e);
}
}
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalId.java b/java/com/google/gerrit/server/account/externalids/ExternalId.java
index b18b27b..2d501ad 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalId.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalId.java
@@ -24,6 +24,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.common.flogger.FluentLogger;
import com.google.common.hash.Hashing;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
@@ -43,6 +44,8 @@
@AutoValue
public abstract class ExternalId implements Serializable {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
// If these regular expressions are modified the same modifications should be done to the
// corresponding regular expressions in the
// com.google.gerrit.client.account.UsernameField class.
@@ -391,11 +394,12 @@
}
return accountId;
} catch (IllegalArgumentException e) {
- throw invalidConfig(
- noteId,
+ String msg =
String.format(
"Value %s for '%s.%s.%s' is invalid, expected account ID",
- accountIdStr, EXTERNAL_ID_SECTION, externalIdKeyStr, ACCOUNT_ID_KEY));
+ accountIdStr, EXTERNAL_ID_SECTION, externalIdKeyStr, ACCOUNT_ID_KEY);
+ logger.atSevere().withCause(e).log(msg);
+ throw invalidConfig(noteId, msg);
}
}
diff --git a/java/com/google/gerrit/server/args4j/AccountIdHandler.java b/java/com/google/gerrit/server/args4j/AccountIdHandler.java
index 4d0af53..73a970b 100644
--- a/java/com/google/gerrit/server/args4j/AccountIdHandler.java
+++ b/java/com/google/gerrit/server/args4j/AccountIdHandler.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.util.cli.Localizable.localizable;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.AuthType;
@@ -38,6 +39,8 @@
import org.kohsuke.args4j.spi.Setter;
public class AccountIdHandler extends OptionHandler<Account.Id> {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private final AccountResolver accountResolver;
private final AccountManager accountManager;
private final AuthType authType;
@@ -78,11 +81,15 @@
case OPENID:
case OPENID_SSO:
default:
- throw new CmdLineException(owner, localizable("user \"%s\" not found"), token);
+ String msg = "user \"%s\" not found";
+ logger.atSevere().withCause(e).log(msg, token);
+ throw new CmdLineException(owner, localizable(msg), token);
}
}
} catch (StorageException e) {
- throw new CmdLineException(owner, localizable("database is down"));
+ String msg = "database is down";
+ logger.atSevere().withCause(e).log(msg);
+ throw new CmdLineException(owner, localizable(msg));
} catch (IOException e) {
throw new CmdLineException(owner, "Failed to load account", e);
} catch (ConfigInvalidException e) {
@@ -102,7 +109,9 @@
req.setSkipAuthentication(true);
return accountManager.authenticate(req).getAccountId();
} catch (AccountException e) {
- throw new CmdLineException(owner, localizable("user \"%s\" not found"), user);
+ String msg = "user \"%s\" not found";
+ logger.atSevere().withCause(e).log(msg, user);
+ throw new CmdLineException(owner, localizable(msg), user);
}
}
diff --git a/java/com/google/gerrit/server/args4j/ChangeIdHandler.java b/java/com/google/gerrit/server/args4j/ChangeIdHandler.java
index 9c14aca..ddcd4db 100644
--- a/java/com/google/gerrit/server/args4j/ChangeIdHandler.java
+++ b/java/com/google/gerrit/server/args4j/ChangeIdHandler.java
@@ -66,7 +66,7 @@
return 1;
}
} catch (IllegalArgumentException e) {
- throw new CmdLineException(owner, localizable("Change-Id is not valid"));
+ throw new CmdLineException(owner, localizable("Change-Id is not valid: %s"), e.getMessage());
} catch (StorageException e) {
throw new CmdLineException(owner, localizable("Database error: %s"), e.getMessage());
}
diff --git a/java/com/google/gerrit/server/args4j/PatchSetIdHandler.java b/java/com/google/gerrit/server/args4j/PatchSetIdHandler.java
index 5016363..8b7cbd6 100644
--- a/java/com/google/gerrit/server/args4j/PatchSetIdHandler.java
+++ b/java/com/google/gerrit/server/args4j/PatchSetIdHandler.java
@@ -43,7 +43,8 @@
try {
id = PatchSet.Id.parse(token);
} catch (IllegalArgumentException e) {
- throw new CmdLineException(owner, localizable("\"%s\" is not a valid patch set"), token);
+ throw new CmdLineException(
+ owner, localizable("\"%s\" is not a valid patch set: %s"), token, e.getMessage());
}
setter.addValue(id);
diff --git a/java/com/google/gerrit/server/args4j/ProjectHandler.java b/java/com/google/gerrit/server/args4j/ProjectHandler.java
index abb6a1f..61dbd2c 100644
--- a/java/com/google/gerrit/server/args4j/ProjectHandler.java
+++ b/java/com/google/gerrit/server/args4j/ProjectHandler.java
@@ -89,7 +89,7 @@
permissionBackend.currentUser().project(nameKey).check(permissionToCheck);
} catch (AuthException e) {
throw new CmdLineException(
- owner, localizable(new NoSuchProjectException(nameKey).getMessage()));
+ owner, localizable(new NoSuchProjectException(nameKey, e).getMessage()));
} catch (PermissionBackendException | IOException e) {
logger.atWarning().withCause(e).log("Cannot load project %s", nameWithoutSuffix);
throw new CmdLineException(
diff --git a/java/com/google/gerrit/server/cache/serialize/IntegerCacheSerializer.java b/java/com/google/gerrit/server/cache/serialize/IntegerCacheSerializer.java
index 4494454..aca9b05 100644
--- a/java/com/google/gerrit/server/cache/serialize/IntegerCacheSerializer.java
+++ b/java/com/google/gerrit/server/cache/serialize/IntegerCacheSerializer.java
@@ -37,7 +37,7 @@
cout.writeInt32NoTag(requireNonNull(object));
cout.flush();
} catch (IOException e) {
- throw new IllegalStateException("Failed to serialize int");
+ throw new IllegalStateException("Failed to serialize int", e);
}
int n = cout.getTotalBytesWritten();
return n == buf.length ? buf : Arrays.copyOfRange(buf, 0, n);
@@ -50,7 +50,7 @@
try {
ret = cin.readRawVarint32();
} catch (IOException e) {
- throw new IllegalArgumentException("Failed to deserialize int");
+ throw new IllegalArgumentException("Failed to deserialize int", e);
}
int n = cin.getTotalBytesRead();
if (n != in.length) {
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 974cb47..70e7967 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -29,7 +29,6 @@
import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED;
import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWER_UPDATES;
import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_DIFFSTAT;
-import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_MERGEABLE;
import static com.google.gerrit.extensions.client.ListChangesOption.SUBMITTABLE;
import static com.google.gerrit.extensions.client.ListChangesOption.TRACKING_IDS;
import static com.google.gerrit.server.ChangeMessagesUtil.createChangeMessageInfo;
@@ -219,7 +218,7 @@
private final Metrics metrics;
private final RevisionJson revisionJson;
private final Optional<PluginDefinedAttributesFactory> pluginDefinedAttributesFactory;
- private final boolean excludeMergeableInChangeInfo;
+ private final boolean includeMergeable;
private final boolean lazyLoad;
private AccountLoader accountLoader;
@@ -257,8 +256,7 @@
this.metrics = metrics;
this.revisionJson = revisionJsonFactory.create(options);
this.options = Sets.immutableEnumSet(options);
- this.excludeMergeableInChangeInfo =
- cfg.getBoolean("change", "api", "excludeMergeableInChangeInfo", false);
+ this.includeMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInApi();
this.lazyLoad = containsAnyOf(this.options, REQUIRE_LAZY_LOAD);
this.pluginDefinedAttributesFactory = pluginDefinedAttributesFactory;
@@ -525,7 +523,7 @@
if (str.isOk()) {
out.submitType = str.type;
}
- if (!excludeMergeableInChangeInfo && !has(SKIP_MERGEABLE)) {
+ if (includeMergeable) {
out.mergeable = cd.isMergeable();
}
if (has(SUBMITTABLE)) {
diff --git a/java/com/google/gerrit/server/change/MergeabilityComputationBehavior.java b/java/com/google/gerrit/server/change/MergeabilityComputationBehavior.java
new file mode 100644
index 0000000..26e7a89
--- /dev/null
+++ b/java/com/google/gerrit/server/change/MergeabilityComputationBehavior.java
@@ -0,0 +1,58 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import org.eclipse.jgit.lib.Config;
+
+/**
+ * State that is used to decide if {@code mergeable} should be included in the REST API or the
+ * change index.
+ *
+ * <p>Computing mergeability of a change is an expensive operation - especially for Gerrit
+ * installations with a large number of open changes or large repositories. Therefore, we want to
+ * control when this computation is performed.
+ */
+public enum MergeabilityComputationBehavior {
+ NEVER(false, false),
+ REF_UPDATED_AND_CHANGE_REINDEX(true, false),
+ API_REF_UPDATED_AND_CHANGE_REINDEX(true, true);
+
+ private final boolean includeInIndex;
+ private final boolean includeInApi;
+
+ MergeabilityComputationBehavior(boolean includeInIndex, boolean includeInApi) {
+ this.includeInIndex = includeInIndex;
+ this.includeInApi = includeInApi;
+ }
+
+ /** Returns a {@link MergeabilityComputationBehavior} constructed from a Gerrit server config. */
+ public static MergeabilityComputationBehavior fromConfig(Config cfg) {
+ return cfg.getEnum(
+ "change",
+ null,
+ "mergeabilityComputationBehavior",
+ MergeabilityComputationBehavior.API_REF_UPDATED_AND_CHANGE_REINDEX);
+ }
+
+ /** Whether {@code mergeable} should be included in the change API. */
+ public boolean includeInApi() {
+ return includeInApi;
+ }
+
+ /** Whether {@code mergeable} should be included in the change index. */
+ public boolean includeInIndex() {
+ return includeInIndex;
+ }
+}
diff --git a/java/com/google/gerrit/server/change/PureRevert.java b/java/com/google/gerrit/server/change/PureRevert.java
index f66d9bc..cb632dc 100644
--- a/java/com/google/gerrit/server/change/PureRevert.java
+++ b/java/com/google/gerrit/server/change/PureRevert.java
@@ -50,7 +50,7 @@
try {
claimedOriginalObjectId = ObjectId.fromString(claimedOriginal.get());
} catch (InvalidObjectIdException e) {
- throw new BadRequestException("invalid object ID");
+ throw new BadRequestException("invalid object ID", e);
}
return pureRevertCache.isPureRevert(
diff --git a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java
index afd159c..bd51fc7 100644
--- a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java
+++ b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java
@@ -17,6 +17,7 @@
import com.google.common.base.Strings;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.server.change.MergeabilityComputationBehavior;
import com.google.gerrit.server.config.ScheduleConfig.Schedule;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -51,7 +52,7 @@
this.urlFormatter = urlFormatter;
schedule = ScheduleConfig.createSchedule(cfg, SECTION);
abandonAfter = readAbandonAfter(cfg);
- boolean indexMergeable = cfg.getBoolean("index", "change", "indexMergeable", true);
+ boolean indexMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex();
if (!indexMergeable) {
if (!readAbandonIfMergeable(cfg)) {
logger.atWarning().log(
diff --git a/java/com/google/gerrit/server/config/ConfigUtil.java b/java/com/google/gerrit/server/config/ConfigUtil.java
index f476adf..43c05e0 100644
--- a/java/com/google/gerrit/server/config/ConfigUtil.java
+++ b/java/com/google/gerrit/server/config/ConfigUtil.java
@@ -30,7 +30,6 @@
import org.eclipse.jgit.lib.Config;
public class ConfigUtil {
-
@SuppressWarnings("unchecked")
private static <T> T[] allValuesOf(T defaultValue) {
try {
@@ -182,7 +181,7 @@
try {
return getTimeUnit(s, defaultValue, wantUnit);
} catch (IllegalArgumentException notTime) {
- throw notTimeUnit(section, subsection, setting, valueString);
+ throw notTimeUnit(section, subsection, setting, valueString, notTime);
}
}
@@ -250,7 +249,7 @@
try {
return wantUnit.convert(Long.parseLong(digits) * inputMul, inputUnit);
} catch (NumberFormatException nfe) {
- throw notTimeUnit(valueString);
+ throw notTimeUnit(valueString, nfe);
}
}
@@ -421,13 +420,21 @@
}
private static IllegalArgumentException notTimeUnit(
- final String section,
- final String subsection,
- final String setting,
- final String valueString) {
- return new IllegalArgumentException(
- "Invalid time unit value: "
- + section
+ String section, String subsection, String setting, String valueString, Throwable why) {
+ return notTimeUnit(
+ section
+ + (subsection != null ? "." + subsection : "")
+ + "."
+ + setting
+ + " = "
+ + valueString,
+ why);
+ }
+
+ private static IllegalArgumentException notTimeUnit(
+ String section, String subsection, String setting, String valueString) {
+ return notTimeUnit(
+ section
+ (subsection != null ? "." + subsection : "")
+ "."
+ setting
@@ -439,5 +446,9 @@
return new IllegalArgumentException("Invalid time unit value: " + val);
}
+ private static IllegalArgumentException notTimeUnit(String val, Throwable why) {
+ return new IllegalArgumentException("Invalid time unit value: " + val, why);
+ }
+
private ConfigUtil() {}
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 7df3902..012e4cd 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -128,7 +128,8 @@
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.TransferConfig;
import com.google.gerrit.server.git.receive.ReceiveCommitsModule;
-import com.google.gerrit.server.git.validators.CommentLimitsValidator;
+import com.google.gerrit.server.git.validators.CommentCountValidator;
+import com.google.gerrit.server.git.validators.CommentSizeValidator;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.gerrit.server.git.validators.MergeValidators;
@@ -410,8 +411,11 @@
DynamicSet.setOf(binder(), UploadValidationListener.class);
bind(CommentValidator.class)
- .annotatedWith(Exports.named(CommentLimitsValidator.class.getSimpleName()))
- .to(CommentLimitsValidator.class);
+ .annotatedWith(Exports.named(CommentCountValidator.class.getSimpleName()))
+ .to(CommentCountValidator.class);
+ bind(CommentValidator.class)
+ .annotatedWith(Exports.named(CommentSizeValidator.class.getSimpleName()))
+ .to(CommentSizeValidator.class);
DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeOperatorFactory.class);
diff --git a/java/com/google/gerrit/server/git/MergeUtil.java b/java/com/google/gerrit/server/git/MergeUtil.java
index 1dbf029..801300e 100644
--- a/java/com/google/gerrit/server/git/MergeUtil.java
+++ b/java/com/google/gerrit/server/git/MergeUtil.java
@@ -760,6 +760,7 @@
try {
failed(rw, mergeTip, n, getCommitMergeStatus(e.getReason()));
} catch (IOException e2) {
+ logger.atSevere().withCause(e2).log("Failed to set merge failure status for " + n.name());
throw new IntegrationException("Cannot merge " + n.name(), e);
}
} catch (IOException e) {
diff --git a/java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java b/java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java
index 1bd8b45..b7dc2b3 100644
--- a/java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java
+++ b/java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java
@@ -106,7 +106,7 @@
try {
result = forProject.filter(refs, getDelegate(), RefFilterOptions.defaults());
} catch (PermissionBackendException e) {
- throw new IOException("");
+ throw new IOException("", e);
}
return buildPrefixRefMap(prefix, result);
}
diff --git a/java/com/google/gerrit/server/git/PureRevertCache.java b/java/com/google/gerrit/server/git/PureRevertCache.java
index b21cb5c..9f9530c 100644
--- a/java/com/google/gerrit/server/git/PureRevertCache.java
+++ b/java/com/google/gerrit/server/git/PureRevertCache.java
@@ -166,7 +166,7 @@
try {
claimedOriginalCommit = rw.parseCommit(original);
} catch (InvalidObjectIdException | MissingObjectException e) {
- throw new BadRequestException("invalid object ID");
+ throw new BadRequestException("invalid object ID", e);
}
if (claimedOriginalCommit.getParentCount() == 0) {
throw new BadRequestException("can't check against initial commit");
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index d8aa054..4b6a4cd 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -92,6 +92,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.validators.CommentForValidation;
+import com.google.gerrit.extensions.validators.CommentForValidation.CommentSource;
import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
@@ -126,6 +127,8 @@
import com.google.gerrit.server.git.ReceivePackInitializer;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.ValidationError;
+import com.google.gerrit.server.git.validators.CommentCountValidator;
+import com.google.gerrit.server.git.validators.CommentSizeValidator;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.RefOperationValidationException;
import com.google.gerrit.server.git.validators.RefOperationValidators;
@@ -1441,11 +1444,18 @@
final ReceiveCommand cmd;
final LabelTypes labelTypes;
/**
- * Result of running {@link CommentValidator}-s on drafts that are published with the commit
- * (which happens iff {@code --publish-comments} is set). Remains {@code true} if none are
- * installed.
+ * Draft comments are published with the commit iff {@code --publish-comments} is set. All
+ * drafts are withheld (overriding the option) if at least one of the following conditions are
+ * met:
+ *
+ * <ul>
+ * <li>Installed {@link CommentValidator} plugins reject one or more draft comments.
+ * <li>One or more comments exceed the maximum comment size (see {@link
+ * CommentSizeValidator}).
+ * <li>The maximum number of comments would be exceeded (see {@link CommentCountValidator}).
+ * </ul>
*/
- private boolean commentsValid = true;
+ private boolean withholdComments = false;
BranchNameKey dest;
PermissionBackend.ForRef perm;
@@ -1642,18 +1652,19 @@
.collect(toImmutableSet());
}
- void setCommentsValid(boolean commentsValid) {
- this.commentsValid = commentsValid;
+ void setWithholdComments(boolean withholdComments) {
+ this.withholdComments = withholdComments;
}
boolean shouldPublishComments() {
- if (!commentsValid) {
+ if (withholdComments) {
// Validation messages of type WARNING have already been added, now withhold the comments.
return false;
}
if (publishComments) {
return true;
- } else if (noPublishComments) {
+ }
+ if (noPublishComments) {
return false;
}
return defaultPublishComments;
@@ -2010,19 +2021,18 @@
.map(
comment ->
CommentForValidation.create(
+ CommentSource.HUMAN,
comment.lineNbr > 0
? CommentType.INLINE_COMMENT
: CommentType.FILE_COMMENT,
- comment.message))
+ comment.message,
+ comment.message.length()))
.collect(toImmutableList());
CommentValidationContext ctx =
- CommentValidationContext.builder()
- .changeId(change.getChangeId())
- .project(change.getProject().get())
- .build();
+ CommentValidationContext.create(change.getChangeId(), change.getProject().get());
ImmutableList<CommentValidationFailure> commentValidationFailures =
PublishCommentUtil.findInvalidComments(ctx, commentValidators, draftsForValidation);
- magicBranch.setCommentsValid(commentValidationFailures.isEmpty());
+ magicBranch.setWithholdComments(!commentValidationFailures.isEmpty());
commentValidationFailures.forEach(
failure ->
addMessage(
diff --git a/java/com/google/gerrit/server/git/validators/CommentCountValidator.java b/java/com/google/gerrit/server/git/validators/CommentCountValidator.java
new file mode 100644
index 0000000..2c939c8
--- /dev/null
+++ b/java/com/google/gerrit/server/git/validators/CommentCountValidator.java
@@ -0,0 +1,61 @@
+// Copyright (C) 2020 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.git.validators;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.validators.CommentForValidation;
+import com.google.gerrit.extensions.validators.CommentValidationContext;
+import com.google.gerrit.extensions.validators.CommentValidationFailure;
+import com.google.gerrit.extensions.validators.CommentValidator;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.inject.Inject;
+import org.eclipse.jgit.lib.Config;
+
+/** Limits number of comments to prevent space/time complexity issues. */
+public class CommentCountValidator implements CommentValidator {
+ private final int maxComments;
+ private final ChangeNotes.Factory notesFactory;
+
+ @Inject
+ CommentCountValidator(@GerritServerConfig Config serverConfig, ChangeNotes.Factory notesFactory) {
+ this.notesFactory = notesFactory;
+ maxComments = serverConfig.getInt("change", "maxComments", 5_000);
+ }
+
+ @Override
+ public ImmutableList<CommentValidationFailure> validateComments(
+ CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) {
+ ImmutableList.Builder<CommentValidationFailure> failures = ImmutableList.builder();
+ ChangeNotes notes =
+ notesFactory.createChecked(Project.nameKey(ctx.getProject()), Change.id(ctx.getChangeId()));
+ int numExistingComments = notes.getComments().size() + notes.getRobotComments().size();
+ if (!comments.isEmpty() && numExistingComments + comments.size() > maxComments) {
+ // This warning really applies to the set of all comments, but we need to pick one to attach
+ // the message to.
+ CommentForValidation commentForFailureMessage = Iterables.getLast(comments);
+
+ failures.add(
+ commentForFailureMessage.failValidation(
+ String.format(
+ "Exceeding maximum number of comments: %d (existing) + %d (new) > %d",
+ numExistingComments, comments.size(), maxComments)));
+ }
+ return failures.build();
+ }
+}
diff --git a/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java b/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java
deleted file mode 100644
index 3a8bcac..0000000
--- a/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java
+++ /dev/null
@@ -1,47 +0,0 @@
-// Copyright (C) 2020 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.git.validators;
-
-import com.google.common.collect.ImmutableList;
-import com.google.gerrit.extensions.validators.CommentForValidation;
-import com.google.gerrit.extensions.validators.CommentValidationContext;
-import com.google.gerrit.extensions.validators.CommentValidationFailure;
-import com.google.gerrit.extensions.validators.CommentValidator;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.inject.Inject;
-import org.eclipse.jgit.lib.Config;
-
-/** Limits the size of comments to prevent large comments from causing issues. */
-public class CommentLimitsValidator implements CommentValidator {
- private final int maxCommentLength;
-
- @Inject
- CommentLimitsValidator(@GerritServerConfig Config serverConfig) {
- maxCommentLength = serverConfig.getInt("change", null, "maxCommentLength", 16 << 10);
- }
-
- @Override
- public ImmutableList<CommentValidationFailure> validateComments(
- CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) {
- return comments.stream()
- .filter(c -> c.getText().length() > maxCommentLength)
- .map(
- c ->
- c.failValidation(
- String.format(
- "Comment too large (%d > %d)", c.getText().length(), maxCommentLength)))
- .collect(ImmutableList.toImmutableList());
- }
-}
diff --git a/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java b/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java
new file mode 100644
index 0000000..58b0cb1
--- /dev/null
+++ b/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java
@@ -0,0 +1,71 @@
+// Copyright (C) 2020 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.git.validators;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.extensions.validators.CommentForValidation;
+import com.google.gerrit.extensions.validators.CommentValidationContext;
+import com.google.gerrit.extensions.validators.CommentValidationFailure;
+import com.google.gerrit.extensions.validators.CommentValidator;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.inject.Inject;
+import org.eclipse.jgit.lib.Config;
+
+/** Limits the size of comments to prevent space/time complexity issues. */
+public class CommentSizeValidator implements CommentValidator {
+ private final int commentSizeLimit;
+ private final int robotCommentSizeLimit;
+
+ @Inject
+ CommentSizeValidator(@GerritServerConfig Config serverConfig) {
+ commentSizeLimit = serverConfig.getInt("change", "commentSizeLimit", 16 << 10);
+ robotCommentSizeLimit = serverConfig.getInt("change", "robotCommentSizeLimit", 1 << 20);
+ }
+
+ @Override
+ public ImmutableList<CommentValidationFailure> validateComments(
+ CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) {
+ return comments.stream()
+ .filter(this::exceedsSizeLimit)
+ .map(c -> c.failValidation(buildErrorMessage(c)))
+ .collect(ImmutableList.toImmutableList());
+ }
+
+ private boolean exceedsSizeLimit(CommentForValidation comment) {
+ switch (comment.getSource()) {
+ case HUMAN:
+ return comment.getApproximateSize() > commentSizeLimit;
+ case ROBOT:
+ return robotCommentSizeLimit > 0 && comment.getApproximateSize() > robotCommentSizeLimit;
+ }
+ throw new RuntimeException(
+ "Unknown comment source (should not have compiled): " + comment.getSource());
+ }
+
+ private String buildErrorMessage(CommentForValidation comment) {
+ switch (comment.getSource()) {
+ case HUMAN:
+ return String.format(
+ "Comment size exceeds limit (%d > %d)", comment.getApproximateSize(), commentSizeLimit);
+
+ case ROBOT:
+ return String.format(
+ "Size %d (bytes) of robot comment is greater than limit %d (bytes)",
+ comment.getApproximateSize(), robotCommentSizeLimit);
+ }
+ throw new RuntimeException(
+ "Unknown comment source (should not have compiled): " + comment.getSource());
+ }
+}
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidationException.java b/java/com/google/gerrit/server/git/validators/CommitValidationException.java
index 4dd2951..220fc12 100644
--- a/java/com/google/gerrit/server/git/validators/CommitValidationException.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidationException.java
@@ -32,6 +32,11 @@
this.messages = ImmutableList.of(message);
}
+ public CommitValidationException(String reason, CommitValidationMessage message, Throwable why) {
+ super(reason, why);
+ this.messages = ImmutableList.of(message);
+ }
+
public CommitValidationException(String reason, List<CommitValidationMessage> messages) {
super(reason);
this.messages = ImmutableList.copyOf(messages);
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java
index 4159ebb..4d2dbef 100644
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -151,6 +151,7 @@
new ProjectStateValidationListener(projectState),
new AmendedGerritMergeCommitValidationListener(perm, gerritIdent),
new AuthorUploaderValidator(user, perm, urlFormatter.get()),
+ new FileCountValidator(patchListCache, config),
new CommitterUploaderValidator(user, perm, urlFormatter.get()),
new SignedOffByValidator(user, perm, projectState),
new ChangeIdValidator(
@@ -414,7 +415,11 @@
diffSummary.getPaths().size(), maxFileCount));
}
} catch (PatchListNotAvailableException e) {
- logger.atWarning().withCause(e).log("Failed to validate file count");
+ // This happens e.g. for cherrypicks.
+ if (!receiveEvent.command.getRefName().startsWith(REFS_CHANGES)) {
+ logger.atWarning().withCause(e).log(
+ "Failed to validate file count for commit: %s", receiveEvent.commit.toString());
+ }
}
return Collections.emptyList();
}
@@ -500,7 +505,7 @@
perm.check(RefPermission.MERGE);
return Collections.emptyList();
} catch (AuthException e) {
- throw new CommitValidationException("you are not allowed to upload merges");
+ throw new CommitValidationException("you are not allowed to upload merges", e);
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check MERGE");
throw new CommitValidationException("internal auth error");
@@ -597,7 +602,7 @@
perm.check(RefPermission.FORGE_COMMITTER);
} catch (AuthException denied) {
throw new CommitValidationException(
- "not Signed-off-by author/committer/uploader in message footer");
+ "not Signed-off-by author/committer/uploader in message footer", denied);
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check FORGE_COMMITTER");
throw new CommitValidationException("internal auth error");
@@ -632,7 +637,7 @@
return Collections.emptyList();
} catch (AuthException e) {
throw new CommitValidationException(
- "invalid author", invalidEmail("author", author, user, urlFormatter));
+ "invalid author", invalidEmail("author", author, user, urlFormatter), e);
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check FORGE_AUTHOR");
throw new CommitValidationException("internal auth error");
@@ -665,7 +670,7 @@
return Collections.emptyList();
} catch (AuthException e) {
throw new CommitValidationException(
- "invalid committer", invalidEmail("committer", committer, user, urlFormatter));
+ "invalid committer", invalidEmail("committer", committer, user, urlFormatter), e);
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check FORGE_COMMITTER");
throw new CommitValidationException("internal auth error");
@@ -704,7 +709,8 @@
"pushing merge commit %s by %s requires '%s' permission",
receiveEvent.commit.getId(),
gerritIdent.getEmailAddress(),
- RefPermission.FORGE_SERVER.name()));
+ RefPermission.FORGE_SERVER.name()),
+ denied);
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check FORGE_SERVER");
throw new CommitValidationException("internal auth error");
diff --git a/java/com/google/gerrit/server/git/validators/MergeValidationException.java b/java/com/google/gerrit/server/git/validators/MergeValidationException.java
index 3624fe0..2b7803d 100644
--- a/java/com/google/gerrit/server/git/validators/MergeValidationException.java
+++ b/java/com/google/gerrit/server/git/validators/MergeValidationException.java
@@ -28,4 +28,8 @@
public MergeValidationException(String msg) {
super(msg);
}
+
+ public MergeValidationException(String msg, Throwable why) {
+ super(msg, why);
+ }
}
diff --git a/java/com/google/gerrit/server/git/validators/MergeValidators.java b/java/com/google/gerrit/server/git/validators/MergeValidators.java
index 8abe5c6..9c557a7 100644
--- a/java/com/google/gerrit/server/git/validators/MergeValidators.java
+++ b/java/com/google/gerrit/server/git/validators/MergeValidators.java
@@ -192,7 +192,7 @@
try {
permissionBackend.user(caller).check(GlobalPermission.ADMINISTRATE_SERVER);
} catch (AuthException e) {
- throw new MergeValidationException(SET_BY_ADMIN);
+ throw new MergeValidationException(SET_BY_ADMIN, e);
} catch (PermissionBackendException e) {
logger.atWarning().withCause(e).log("Cannot check ADMINISTRATE_SERVER");
throw new MergeValidationException("validation unavailable");
@@ -204,7 +204,7 @@
.project(destProject.getNameKey())
.check(ProjectPermission.WRITE_CONFIG);
} catch (AuthException e) {
- throw new MergeValidationException(SET_BY_OWNER);
+ throw new MergeValidationException(SET_BY_OWNER, e);
} catch (PermissionBackendException e) {
logger.atWarning().withCause(e).log("Cannot check WRITE_CONFIG");
throw new MergeValidationException("validation unavailable");
@@ -245,7 +245,7 @@
}
}
} catch (ConfigInvalidException | IOException e) {
- throw new MergeValidationException(INVALID_CONFIG);
+ throw new MergeValidationException(INVALID_CONFIG, e);
}
}
}
diff --git a/java/com/google/gerrit/server/git/validators/RefOperationValidators.java b/java/com/google/gerrit/server/git/validators/RefOperationValidators.java
index f3d8f4a..f3b6983 100644
--- a/java/com/google/gerrit/server/git/validators/RefOperationValidators.java
+++ b/java/com/google/gerrit/server/git/validators/RefOperationValidators.java
@@ -134,7 +134,7 @@
try {
perm.check(GlobalPermission.ACCESS_DATABASE);
} catch (AuthException | PermissionBackendException e) {
- throw new ValidationException("Not allowed to create user branch.");
+ throw new ValidationException("Not allowed to create user branch.", e);
}
if (Account.Id.fromRef(refEvent.command.getRefName()) == null) {
throw new ValidationException(
@@ -145,7 +145,7 @@
try {
perm.check(GlobalPermission.ACCESS_DATABASE);
} catch (AuthException | PermissionBackendException e) {
- throw new ValidationException("Not allowed to delete user branch.");
+ throw new ValidationException("Not allowed to delete user branch.", e);
}
}
}
diff --git a/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java b/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
index 4e27859..19bb423 100644
--- a/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
+++ b/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
@@ -28,6 +28,7 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.change.MergeabilityComputationBehavior;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.QueueProvider.QueueType;
@@ -85,7 +86,7 @@
this.accountCache = accountCache;
this.indexer = indexer;
this.executor = executor;
- this.enabled = cfg.getBoolean("index", "change", "indexMergeable", true);
+ this.enabled = MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex();
}
@Override
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index e79696a..9c3dd02 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -19,6 +19,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@@ -243,7 +244,7 @@
sendRejectionEmail(message, InboundEmailRejectionSender.Error.INTERNAL_EXCEPTION);
return;
}
- ChangeData cd = changeDataList.get(0);
+ ChangeData cd = Iterables.getOnlyElement(changeDataList);
if (existingMessageIds(cd).contains(message.id())) {
logger.atInfo().log("Message %s was already processed. Will delete message.", message.id());
return;
@@ -285,14 +286,14 @@
.map(
comment ->
CommentForValidation.create(
+ CommentForValidation.CommentSource.HUMAN,
MAIL_COMMENT_TYPE_TO_VALIDATION_TYPE.get(comment.getType()),
- comment.getMessage()))
+ comment.getMessage(),
+ comment.getMessage().length()))
.collect(ImmutableList.toImmutableList());
CommentValidationContext commentValidationCtx =
- CommentValidationContext.builder()
- .changeId(cd.change().getChangeId())
- .project(cd.change().getProject().get())
- .build();
+ CommentValidationContext.create(
+ cd.change().getChangeId(), cd.change().getProject().get());
ImmutableList<CommentValidationFailure> commentValidationFailures =
PublishCommentUtil.findInvalidComments(
commentValidationCtx, commentValidators, parsedCommentsForValidation);
diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
index d7f09d2..9e53c65 100644
--- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java
+++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -293,7 +293,7 @@
try {
ps = args.patchSetUtil.get(changeData.notes(), PatchSet.id(change.getId(), patchSetId));
} catch (StorageException e) {
- throw new PatchListNotAvailableException("Failed to get patchSet");
+ throw new PatchListNotAvailableException("Failed to get patchSet", e);
}
}
return args.patchListCache.get(change, ps);
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 5468e23..428df16 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -907,7 +907,9 @@
try {
adr = Address.parse(line);
} catch (IllegalArgumentException e) {
- throw invalidFooter(state.getByEmailFooterKey(), line);
+ ConfigInvalidException cie = invalidFooter(state.getByEmailFooterKey(), line);
+ cie.initCause(e);
+ throw cie;
}
if (!reviewersByEmail.containsRow(adr)) {
reviewersByEmail.put(adr, state, ts);
diff --git a/java/com/google/gerrit/server/notedb/LimitExceededException.java b/java/com/google/gerrit/server/notedb/LimitExceededException.java
index 69f9241..0a57d96 100644
--- a/java/com/google/gerrit/server/notedb/LimitExceededException.java
+++ b/java/com/google/gerrit/server/notedb/LimitExceededException.java
@@ -23,12 +23,13 @@
* <ul>
* <li>The number of NoteDb updates per change.
* <li>The number of patch sets per change.
+ * <li>The number of files per change.
* </ul>
*/
public class LimitExceededException extends StorageException {
private static final long serialVersionUID = 1L;
- LimitExceededException(String message) {
+ public LimitExceededException(String message) {
super(message);
}
}
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index afc1ab1..b7b3d81 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -193,7 +193,7 @@
try {
permissionBackend.currentUser().change(notes).check(ChangePermission.READ);
} catch (AuthException e) {
- throw new NoSuchChangeException(changeId);
+ throw new NoSuchChangeException(changeId, e);
}
if (!projectCache.checkedGet(notes.getProjectName()).statePermitsRead()) {
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java b/java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java
index cf07190..2e39607 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java
@@ -98,7 +98,7 @@
try {
permissionBackend.currentUser().change(notes).check(ChangePermission.READ);
} catch (AuthException e) {
- throw new NoSuchChangeException(changeId);
+ throw new NoSuchChangeException(changeId, e);
}
if (!projectCache.checkedGet(notes.getProjectName()).statePermitsRead()) {
diff --git a/java/com/google/gerrit/server/project/ProjectCreator.java b/java/com/google/gerrit/server/project/ProjectCreator.java
index ba44142..c4c466e 100644
--- a/java/com/google/gerrit/server/project/ProjectCreator.java
+++ b/java/com/google/gerrit/server/project/ProjectCreator.java
@@ -132,9 +132,10 @@
+ nameKey.get()
+ " because the name is already occupied by another project."
+ " The other project has the same name, only spelled in a"
- + " different case.");
+ + " different case.",
+ e);
} catch (RepositoryNotFoundException badName) {
- throw new BadRequestException("invalid project name: " + nameKey);
+ throw new BadRequestException("invalid project name: " + nameKey, badName);
} catch (ConfigInvalidException e) {
String msg = "Cannot create " + nameKey;
logger.atSevere().withCause(e).log(msg);
diff --git a/java/com/google/gerrit/server/project/RefUtil.java b/java/com/google/gerrit/server/project/RefUtil.java
index 5d6379a..dc8cdc7 100644
--- a/java/com/google/gerrit/server/project/RefUtil.java
+++ b/java/com/google/gerrit/server/project/RefUtil.java
@@ -55,7 +55,7 @@
"Cannot resolve \"%s\" in project \"%s\"", baseRevision, projectName.get());
throw new InvalidRevisionException(baseRevision);
} catch (RevisionSyntaxException err) {
- throw new InvalidRevisionException(baseRevision);
+ throw new InvalidRevisionException(baseRevision, err);
}
}
@@ -66,7 +66,7 @@
try {
rw.markStart(rw.parseCommit(revid));
} catch (IncorrectObjectTypeException err) {
- throw new InvalidRevisionException(revid.name());
+ throw new InvalidRevisionException(revid.name(), err);
}
RefDatabase refDb = repo.getRefDatabase();
Iterable<Ref> refs =
@@ -86,7 +86,7 @@
rw.checkConnectivity();
return rw;
} catch (IncorrectObjectTypeException | MissingObjectException err) {
- throw new InvalidRevisionException(revid.name());
+ throw new InvalidRevisionException(revid.name(), err);
} catch (IOException err) {
logger.atSevere().withCause(err).log(
"Repository \"%s\" may be corrupt; suggest running git fsck", repo.getDirectory());
@@ -128,5 +128,9 @@
InvalidRevisionException(@Nullable String invalidRevision) {
super(MESSAGE + ": " + invalidRevision);
}
+
+ InvalidRevisionException(@Nullable String invalidRevision, Throwable why) {
+ super(MESSAGE + ": " + invalidRevision, why);
+ }
}
}
diff --git a/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java b/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
index 664df70..42a8310 100644
--- a/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java
@@ -123,7 +123,9 @@
try {
args.permissionBackend.user(args.getUser()).change(changeNotes).check(ChangePermission.READ);
} catch (AuthException e) {
- throw error(String.format("change %s not found", change));
+ String msg = String.format("change %s not found", change);
+ logger.atSevere().withCause(e).log(msg);
+ throw error(msg);
}
return AccountPredicates.cansee(args, changeNotes);
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 7747998..327c21f 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -63,6 +63,7 @@
import com.google.gerrit.server.account.VersionedAccountDestinations;
import com.google.gerrit.server.account.VersionedAccountQueries;
import com.google.gerrit.server.change.ChangeTriplet;
+import com.google.gerrit.server.change.MergeabilityComputationBehavior;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -293,7 +294,7 @@
groupMembers,
anonymousUserProvider,
operatorAliasConfig,
- gerritConfig.getBoolean("index", "change", "indexMergeable", true));
+ MergeabilityComputationBehavior.fromConfig(gerritConfig).includeInIndex());
}
private Arguments(
@@ -1288,7 +1289,8 @@
throw new QueryParseException(
"'"
+ value
- + "' is not a valid input. It must be in the 'ChangeNumber[,PatchsetNumber]' format.");
+ + "' is not a valid input. It must be in the 'ChangeNumber[,PatchsetNumber]' format.",
+ e);
}
}
throw new QueryParseException(
diff --git a/java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java b/java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java
index 2992869..d234546 100644
--- a/java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java
@@ -75,7 +75,7 @@
try {
parsedState = ProjectState.valueOf(state.replace('-', '_').toUpperCase());
} catch (IllegalArgumentException e) {
- throw error("state operator must be either 'active' or 'read-only'");
+ throw error("state operator must be either 'active' or 'read-only'", e);
}
if (parsedState == ProjectState.HIDDEN) {
throw error("state operator must be either 'active' or 'read-only'");
diff --git a/java/com/google/gerrit/server/restapi/account/Capabilities.java b/java/com/google/gerrit/server/restapi/account/Capabilities.java
index 07b1214..3d719ff9 100644
--- a/java/com/google/gerrit/server/restapi/account/Capabilities.java
+++ b/java/com/google/gerrit/server/restapi/account/Capabilities.java
@@ -75,7 +75,7 @@
permissionBackend.absentUser(target.getAccountId()).check(perm);
return new AccountResource.Capability(target, globalOrPluginPermissionName(perm));
} catch (AuthException e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/account/CreateAccount.java b/java/com/google/gerrit/server/restapi/account/CreateAccount.java
index 8f525ef..907dd18 100644
--- a/java/com/google/gerrit/server/restapi/account/CreateAccount.java
+++ b/java/com/google/gerrit/server/restapi/account/CreateAccount.java
@@ -163,7 +163,7 @@
try {
addGroupMember(groupUuid, accountId);
} catch (NoSuchGroupException e) {
- throw new UnprocessableEntityException(String.format("Group %s not found", groupUuid));
+ throw new UnprocessableEntityException(String.format("Group %s not found", groupUuid), e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
index a6fbb10..4b505c6 100644
--- a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
+++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
@@ -27,7 +27,6 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.accounts.DeleteDraftCommentsInput;
import com.google.gerrit.extensions.api.accounts.DeletedDraftCommentInfo;
-import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -190,9 +189,7 @@
if (dirty) {
result = new DeletedDraftCommentInfo();
result.change =
- changeJsonFactory
- .create(ListChangesOption.SKIP_MERGEABLE)
- .format(changeDataFactory.create(ctx.getNotes()));
+ changeJsonFactory.noOptions().format(changeDataFactory.create(ctx.getNotes()));
result.deleted = comments.build();
}
return dirty;
diff --git a/java/com/google/gerrit/server/restapi/account/PutAgreement.java b/java/com/google/gerrit/server/restapi/account/PutAgreement.java
index 459ff26..42504a0 100644
--- a/java/com/google/gerrit/server/restapi/account/PutAgreement.java
+++ b/java/com/google/gerrit/server/restapi/account/PutAgreement.java
@@ -100,7 +100,7 @@
try {
addMembers.addMembers(uuid, ImmutableSet.of(accountState.account().id()));
} catch (NoSuchGroupException e) {
- throw new ResourceConflictException("autoverify group not found");
+ throw new ResourceConflictException("autoverify group not found", e);
}
agreementSignup.fire(accountState, agreementName);
diff --git a/java/com/google/gerrit/server/restapi/account/PutUsername.java b/java/com/google/gerrit/server/restapi/account/PutUsername.java
index dabfaa5..dc841b8 100644
--- a/java/com/google/gerrit/server/restapi/account/PutUsername.java
+++ b/java/com/google/gerrit/server/restapi/account/PutUsername.java
@@ -122,7 +122,7 @@
}
// Otherwise, someone else has this identity.
- throw new ResourceConflictException("username already used");
+ throw new ResourceConflictException("username already used", dupeErr);
}
sshKeyCache.evict(input.username);
diff --git a/java/com/google/gerrit/server/restapi/account/SshKeys.java b/java/com/google/gerrit/server/restapi/account/SshKeys.java
index 6e3f905..a6c4d80 100644
--- a/java/com/google/gerrit/server/restapi/account/SshKeys.java
+++ b/java/com/google/gerrit/server/restapi/account/SshKeys.java
@@ -70,7 +70,7 @@
permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
} catch (AuthException e) {
// If lacking MODIFY_ACCOUNT claim the resource does not exist.
- throw new ResourceNotFoundException();
+ throw new ResourceNotFoundException(id, e);
}
}
return parse(rsrc.getUser(), id);
@@ -86,7 +86,7 @@
}
return new AccountResource.SshKey(user, sshKey);
} catch (NumberFormatException e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/account/StarredChanges.java b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
index 3c14ad3..c108dcb 100644
--- a/java/com/google/gerrit/server/restapi/account/StarredChanges.java
+++ b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
@@ -123,10 +123,10 @@
try {
change = changes.parse(TopLevelResource.INSTANCE, id);
} catch (ResourceNotFoundException e) {
- throw new UnprocessableEntityException(String.format("change %s not found", id.get()));
+ throw new UnprocessableEntityException(String.format("change %s not found", id.get()), e);
} catch (StorageException | PermissionBackendException | IOException e) {
logger.atSevere().withCause(e).log("cannot resolve change");
- throw new UnprocessableEntityException("internal server error");
+ throw new UnprocessableEntityException("internal server error", e);
}
try {
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index 3496491..ac81b45 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -422,7 +422,8 @@
try {
baseObjectId = ObjectId.fromString(base);
} catch (InvalidObjectIdException e) {
- throw new BadRequestException(String.format("Base %s doesn't represent a valid SHA-1", base));
+ throw new BadRequestException(
+ String.format("Base %s doesn't represent a valid SHA-1", base), e);
}
RevCommit baseCommit;
diff --git a/java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java b/java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java
index 8d50b86..06478ac 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java
@@ -205,7 +205,7 @@
try {
permissionBackend.currentUser().change(change).check(ChangePermission.READ);
} catch (AuthException e) {
- throw new UnprocessableEntityException("Read not permitted for " + baseChange);
+ throw new UnprocessableEntityException("Read not permitted for " + baseChange, e);
}
return psUtil.current(change);
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetDiff.java b/java/com/google/gerrit/server/restapi/change/GetDiff.java
index c1d286a..c3deb79 100644
--- a/java/com/google/gerrit/server/restapi/change/GetDiff.java
+++ b/java/com/google/gerrit/server/restapi/change/GetDiff.java
@@ -293,6 +293,7 @@
throw new NumberFormatException();
}
} catch (NumberFormatException e) {
+ logger.atFine().withCause(e).log("invalid numeric value");
throw new CmdLineException(
owner,
localizable("\"%s\" is not a valid value for \"%s\""),
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 324069d..129df59 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -80,7 +80,6 @@
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
-import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.mail.Address;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
@@ -125,11 +124,9 @@
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.time.TimeUtil;
-import com.google.gson.Gson;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
-import java.nio.charset.StandardCharsets;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
@@ -141,7 +138,6 @@
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
-import java.util.OptionalInt;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
@@ -159,9 +155,6 @@
public static final String START_REVIEW_MESSAGE = "This change is ready for review.";
- private static final Gson GSON = OutputFormat.JSON_COMPACT.newGson();
- private static final int DEFAULT_ROBOT_COMMENT_SIZE_LIMIT_IN_BYTES = 1024 * 1024;
-
private final BatchUpdate.Factory updateFactory;
private final ChangeResource.Factory changeResourceFactory;
private final ChangeData.Factory changeDataFactory;
@@ -177,7 +170,6 @@
private final ReviewerAdder reviewerAdder;
private final AddReviewersEmail addReviewersEmail;
private final NotifyResolver notifyResolver;
- private final Config gerritConfig;
private final WorkInProgressOp.Factory workInProgressOpFactory;
private final ProjectCache projectCache;
private final PermissionBackend permissionBackend;
@@ -221,7 +213,6 @@
this.reviewerAdder = reviewerAdder;
this.addReviewersEmail = addReviewersEmail;
this.notifyResolver = notifyResolver;
- this.gerritConfig = gerritConfig;
this.workInProgressOpFactory = workInProgressOpFactory;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
@@ -514,7 +505,8 @@
throw new AuthException(
String.format(
"not permitted to modify label \"%s\" on behalf of \"%s\"",
- type.getName(), in.onBehalfOf));
+ type.getName(), in.onBehalfOf),
+ e);
}
}
}
@@ -530,7 +522,7 @@
permissionBackend.user(reviewer).change(rev.getNotes()).check(ChangePermission.READ);
} catch (AuthException e) {
throw new UnprocessableEntityException(
- String.format("on_behalf_of account %s cannot see change", reviewer.getAccountId()));
+ String.format("on_behalf_of account %s cannot see change", reviewer.getAccountId()), e);
}
return new RevisionResource(
@@ -580,7 +572,7 @@
perm.check(new LabelPermission.WithValue(lt, val));
} catch (AuthException e) {
throw new AuthException(
- String.format("Applying label \"%s\": %d is restricted", lt.getName(), val));
+ String.format("Applying label \"%s\": %d is restricted", lt.getName(), val), e);
}
}
}
@@ -671,41 +663,15 @@
for (Map.Entry<String, List<RobotCommentInput>> e : in.entrySet()) {
String commentPath = e.getKey();
for (RobotCommentInput c : e.getValue()) {
- ensureSizeOfJsonInputIsWithinBounds(c);
ensureRobotIdIsSet(c.robotId, commentPath);
ensureRobotRunIdIsSet(c.robotRunId, commentPath);
ensureFixSuggestionsAreAddable(c.fixSuggestions, commentPath);
+ // Size is validated later, in CommentLimitsValidator.
}
}
checkComments(revision, in);
}
- private void ensureSizeOfJsonInputIsWithinBounds(RobotCommentInput robotCommentInput)
- throws BadRequestException {
- OptionalInt robotCommentSizeLimit = getRobotCommentSizeLimit();
- if (robotCommentSizeLimit.isPresent()) {
- int sizeLimit = robotCommentSizeLimit.getAsInt();
- byte[] robotCommentBytes = GSON.toJson(robotCommentInput).getBytes(StandardCharsets.UTF_8);
- int robotCommentSize = robotCommentBytes.length;
- if (robotCommentSize > sizeLimit) {
- throw new BadRequestException(
- String.format(
- "Size %d (bytes) of robot comment is greater than limit %d (bytes)",
- robotCommentSize, sizeLimit));
- }
- }
- }
-
- private OptionalInt getRobotCommentSizeLimit() {
- int robotCommentSizeLimit =
- gerritConfig.getInt(
- "change", "robotCommentSizeLimit", DEFAULT_ROBOT_COMMENT_SIZE_LIMIT_IN_BYTES);
- if (robotCommentSizeLimit <= 0) {
- return OptionalInt.empty();
- }
- return OptionalInt.of(robotCommentSizeLimit);
- }
-
private static void ensureRobotIdIsSet(String robotId, String commentPath)
throws BadRequestException {
if (robotId == null) {
@@ -909,8 +875,10 @@
user = ctx.getIdentifiedUser();
notes = ctx.getNotes();
ps = psUtil.get(ctx.getNotes(), psId);
- boolean dirty = insertComments(ctx);
- dirty |= insertRobotComments(ctx);
+ List<RobotComment> newRobotComments =
+ in.robotComments == null ? ImmutableList.of() : getNewRobotComments(ctx);
+ boolean dirty = insertComments(ctx, newRobotComments);
+ dirty |= insertRobotComments(ctx, newRobotComments);
dirty |= updateLabels(projectState, ctx);
dirty |= insertMessage(ctx);
return dirty;
@@ -937,7 +905,7 @@
ctx.getWhen());
}
- private boolean insertComments(ChangeContext ctx)
+ private boolean insertComments(ChangeContext ctx, List<RobotComment> newRobotComments)
throws UnprocessableEntityException, PatchListNotAvailableException,
CommentsRejectedException {
Map<String, List<CommentInput>> inputComments = in.comments;
@@ -1001,21 +969,21 @@
}
CommentValidationContext commentValidationCtx =
- CommentValidationContext.builder()
- .changeId(ctx.getChange().getChangeId())
- .project(ctx.getChange().getProject().get())
- .build();
+ CommentValidationContext.create(
+ ctx.getChange().getChangeId(), ctx.getChange().getProject().get());
switch (in.drafts) {
case PUBLISH:
case PUBLISH_ALL_REVISIONS:
validateComments(
- commentValidationCtx, Streams.concat(drafts.values().stream(), toPublish.stream()));
+ commentValidationCtx,
+ Streams.concat(
+ drafts.values().stream(), toPublish.stream(), newRobotComments.stream()));
publishCommentUtil.publish(ctx, ctx.getUpdate(psId), drafts.values(), in.tag);
comments.addAll(drafts.values());
break;
case KEEP:
- default:
- validateComments(commentValidationCtx, toPublish.stream());
+ validateComments(
+ commentValidationCtx, Stream.concat(toPublish.stream(), newRobotComments.stream()));
break;
}
ChangeUpdate changeUpdate = ctx.getUpdate(psId);
@@ -1031,10 +999,14 @@
.map(
comment ->
CommentForValidation.create(
+ comment instanceof RobotComment
+ ? CommentForValidation.CommentSource.ROBOT
+ : CommentForValidation.CommentSource.HUMAN,
comment.lineNbr > 0
? CommentForValidation.CommentType.INLINE_COMMENT
: CommentForValidation.CommentType.FILE_COMMENT,
- comment.message))
+ comment.message,
+ comment.getApproximateSize()))
.collect(toImmutableList());
ImmutableList<CommentValidationFailure> draftValidationFailures =
PublishCommentUtil.findInvalidComments(ctx, commentValidators, draftsForValidation);
@@ -1043,12 +1015,10 @@
}
}
- private boolean insertRobotComments(ChangeContext ctx) throws PatchListNotAvailableException {
+ private boolean insertRobotComments(ChangeContext ctx, List<RobotComment> newRobotComments) {
if (in.robotComments == null) {
return false;
}
-
- List<RobotComment> newRobotComments = getNewRobotComments(ctx);
commentsUtil.putRobotComments(ctx.getUpdate(psId), newRobotComments);
comments.addAll(newRobotComments);
return !newRobotComments.isEmpty();
@@ -1424,17 +1394,18 @@
}
if (!msg.isEmpty()) {
CommentValidationContext commentValidationCtx =
- CommentValidationContext.builder()
- .changeId(ctx.getChange().getChangeId())
- .project(ctx.getChange().getProject().get())
- .build();
+ CommentValidationContext.create(
+ ctx.getChange().getChangeId(), ctx.getChange().getProject().get());
ImmutableList<CommentValidationFailure> messageValidationFailure =
PublishCommentUtil.findInvalidComments(
commentValidationCtx,
commentValidators,
ImmutableList.of(
CommentForValidation.create(
- CommentForValidation.CommentType.CHANGE_MESSAGE, msg)));
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.CHANGE_MESSAGE,
+ msg,
+ msg.length())));
if (!messageValidationFailure.isEmpty()) {
throw new CommentsRejectedException(messageValidationFailure);
}
diff --git a/java/com/google/gerrit/server/restapi/change/PreviewSubmit.java b/java/com/google/gerrit/server/restapi/change/PreviewSubmit.java
index ee1f2e3..e6a60d5 100644
--- a/java/com/google/gerrit/server/restapi/change/PreviewSubmit.java
+++ b/java/com/google/gerrit/server/restapi/change/PreviewSubmit.java
@@ -174,7 +174,7 @@
archiveFormat.putEntry(aos, path, bos.toByteArray());
}
} catch (LimitExceededException e) {
- throw new NotImplementedException("The bundle is too big to generate at the server");
+ throw new NotImplementedException("The bundle is too big to generate at the server", e);
} catch (NoSuchProjectException e) {
throw new IOException(e);
}
diff --git a/java/com/google/gerrit/server/restapi/change/PutAssignee.java b/java/com/google/gerrit/server/restapi/change/PutAssignee.java
index dc1adfa..7b0d905 100644
--- a/java/com/google/gerrit/server/restapi/change/PutAssignee.java
+++ b/java/com/google/gerrit/server/restapi/change/PutAssignee.java
@@ -94,7 +94,7 @@
.change(rsrc.getNotes())
.check(ChangePermission.READ);
} catch (AuthException e) {
- throw new AuthException("read not permitted for " + input.assignee);
+ throw new AuthException("read not permitted for " + input.assignee, e);
}
try (BatchUpdate bu =
diff --git a/java/com/google/gerrit/server/restapi/change/QueryChanges.java b/java/com/google/gerrit/server/restapi/change/QueryChanges.java
index 544177f..878e714 100644
--- a/java/com/google/gerrit/server/restapi/change/QueryChanges.java
+++ b/java/com/google/gerrit/server/restapi/change/QueryChanges.java
@@ -129,7 +129,7 @@
try {
out = query();
} catch (QueryRequiresAuthException e) {
- throw new AuthException("Must be signed-in to use this operator");
+ throw new AuthException("Must be signed-in to use this operator", e);
} catch (QueryParseException e) {
logger.atFine().withCause(e).log("Reject change query with 400 Bad Request: %s", queries);
throw new BadRequestException(e.getMessage(), e);
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index 53e96f7..6fc3ece 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -438,7 +438,7 @@
permissionBackend.user(submitter).change(rsrc.getNotes()).check(ChangePermission.READ);
} catch (AuthException e) {
throw new UnprocessableEntityException(
- String.format("on_behalf_of account %s cannot see change", submitter.getAccountId()));
+ String.format("on_behalf_of account %s cannot see change", submitter.getAccountId()), e);
}
return submitter;
}
diff --git a/java/com/google/gerrit/server/restapi/config/ConfirmEmail.java b/java/com/google/gerrit/server/restapi/config/ConfirmEmail.java
index 03715c9..b55562e 100644
--- a/java/com/google/gerrit/server/restapi/config/ConfirmEmail.java
+++ b/java/com/google/gerrit/server/restapi/config/ConfirmEmail.java
@@ -88,7 +88,7 @@
}
throw new UnprocessableEntityException("invalid token");
} catch (EmailTokenVerifier.InvalidTokenException e) {
- throw new UnprocessableEntityException("invalid token");
+ throw new UnprocessableEntityException("invalid token", e);
} catch (AccountException e) {
throw new UnprocessableEntityException(e.getMessage());
}
diff --git a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
index 2d4bfe6..bbe38a3 100644
--- a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
+++ b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
@@ -23,11 +23,9 @@
import com.google.gerrit.extensions.common.AccountsInfo;
import com.google.gerrit.extensions.common.AuthInfo;
import com.google.gerrit.extensions.common.ChangeConfigInfo;
-import com.google.gerrit.extensions.common.ChangeIndexConfigInfo;
import com.google.gerrit.extensions.common.DownloadInfo;
import com.google.gerrit.extensions.common.DownloadSchemeInfo;
import com.google.gerrit.extensions.common.GerritInfo;
-import com.google.gerrit.extensions.common.IndexConfigInfo;
import com.google.gerrit.extensions.common.PluginConfigInfo;
import com.google.gerrit.extensions.common.ReceiveInfo;
import com.google.gerrit.extensions.common.ServerInfo;
@@ -45,6 +43,7 @@
import com.google.gerrit.server.account.Realm;
import com.google.gerrit.server.avatar.AvatarProvider;
import com.google.gerrit.server.change.ArchiveFormat;
+import com.google.gerrit.server.change.MergeabilityComputationBehavior;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AnonymousCowardName;
@@ -143,7 +142,6 @@
info.change = getChangeInfo();
info.download = getDownloadInfo();
info.gerrit = getGerritInfo();
- info.index = getIndexInfo();
info.noteDbEnabled = true;
info.plugin = getPluginInfo();
info.defaultTheme = getDefaultTheme();
@@ -230,10 +228,10 @@
info.updateDelay =
(int) ConfigUtil.getTimeUnit(config, "change", null, "updateDelay", 300, TimeUnit.SECONDS);
info.submitWholeTopic = toBoolean(MergeSuperSet.wholeTopicEnabled(config));
- info.excludeMergeableInChangeInfo =
- toBoolean(this.config.getBoolean("change", "api", "excludeMergeableInChangeInfo", false));
info.disablePrivateChanges =
toBoolean(this.config.getBoolean("change", null, "disablePrivateChanges", false));
+ info.mergeabilityComputationBehavior =
+ MergeabilityComputationBehavior.fromConfig(config).name();
return info;
}
@@ -299,14 +297,6 @@
return info;
}
- private IndexConfigInfo getIndexInfo() {
- ChangeIndexConfigInfo change = new ChangeIndexConfigInfo();
- change.indexMergeable = toBoolean(config.getBoolean("index", "change", "indexMergeable", true));
- IndexConfigInfo index = new IndexConfigInfo();
- index.change = change;
- return index;
- }
-
private String getDocUrl() {
String docUrl = config.getString("gerrit", null, "docUrl");
if (Strings.isNullOrEmpty(docUrl)) {
diff --git a/java/com/google/gerrit/server/restapi/config/TasksCollection.java b/java/com/google/gerrit/server/restapi/config/TasksCollection.java
index 289f76f..837d071 100644
--- a/java/com/google/gerrit/server/restapi/config/TasksCollection.java
+++ b/java/com/google/gerrit/server/restapi/config/TasksCollection.java
@@ -81,7 +81,7 @@
try {
taskId = (int) Long.parseLong(id.get(), 16);
} catch (NumberFormatException e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
}
Task<?> task = workQueue.getTask(taskId);
diff --git a/java/com/google/gerrit/server/restapi/group/AddMembers.java b/java/com/google/gerrit/server/restapi/group/AddMembers.java
index b60d78e..93d095d 100644
--- a/java/com/google/gerrit/server/restapi/group/AddMembers.java
+++ b/java/com/google/gerrit/server/restapi/group/AddMembers.java
@@ -138,7 +138,7 @@
try {
addMembers(groupUuid, newMemberIds);
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+ throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid), e);
}
return Response.ok(toAccountInfoList(newMemberIds));
}
@@ -234,7 +234,7 @@
}
throw new IllegalStateException();
} catch (UnprocessableEntityException e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/group/AddSubgroups.java b/java/com/google/gerrit/server/restapi/group/AddSubgroups.java
index c969ace..3fd3f29 100644
--- a/java/com/google/gerrit/server/restapi/group/AddSubgroups.java
+++ b/java/com/google/gerrit/server/restapi/group/AddSubgroups.java
@@ -116,7 +116,7 @@
try {
addSubgroups(groupUuid, subgroupUuids);
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+ throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid), e);
}
return Response.ok(result);
}
@@ -153,7 +153,7 @@
}
throw new IllegalStateException();
} catch (UnprocessableEntityException e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/group/CreateGroup.java b/java/com/google/gerrit/server/restapi/group/CreateGroup.java
index b80b0e4..531d350 100644
--- a/java/com/google/gerrit/server/restapi/group/CreateGroup.java
+++ b/java/com/google/gerrit/server/restapi/group/CreateGroup.java
@@ -220,7 +220,7 @@
return groupsUpdateProvider.get().createGroup(groupCreation, groupUpdateBuilder.build());
} catch (DuplicateKeyException e) {
throw new ResourceConflictException(
- "group '" + createGroupArgs.getGroupName() + "' already exists");
+ "group '" + createGroupArgs.getGroupName() + "' already exists", e);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/group/DeleteMembers.java b/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
index fa1c5c6..a7b2e2d 100644
--- a/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
+++ b/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
@@ -78,7 +78,7 @@
try {
removeGroupMembers(groupUuid, membersToRemove);
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+ throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid), e);
}
return Response.none();
diff --git a/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java b/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java
index ef8612e..b9d6ca8 100644
--- a/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java
+++ b/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java
@@ -77,7 +77,7 @@
try {
removeSubgroups(groupUuid, subgroupsToRemove);
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+ throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid), e);
}
return Response.none();
diff --git a/java/com/google/gerrit/server/restapi/group/GetOwner.java b/java/com/google/gerrit/server/restapi/group/GetOwner.java
index 30d5c16..e8bdfaa 100644
--- a/java/com/google/gerrit/server/restapi/group/GetOwner.java
+++ b/java/com/google/gerrit/server/restapi/group/GetOwner.java
@@ -47,7 +47,7 @@
GroupControl c = controlFactory.validateFor(group.getOwnerGroupUUID());
return Response.ok(json.format(c.getGroup()));
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException();
+ throw new ResourceNotFoundException(group.getOwnerGroupUUID().get(), e);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/group/PutDescription.java b/java/com/google/gerrit/server/restapi/group/PutDescription.java
index 98c773e..8fe4b20 100644
--- a/java/com/google/gerrit/server/restapi/group/PutDescription.java
+++ b/java/com/google/gerrit/server/restapi/group/PutDescription.java
@@ -66,7 +66,7 @@
try {
groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate);
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+ throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid), e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/group/PutName.java b/java/com/google/gerrit/server/restapi/group/PutName.java
index 802956a..9a3c87d 100644
--- a/java/com/google/gerrit/server/restapi/group/PutName.java
+++ b/java/com/google/gerrit/server/restapi/group/PutName.java
@@ -79,9 +79,9 @@
try {
groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate);
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+ throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid), e);
} catch (DuplicateKeyException e) {
- throw new ResourceConflictException("group with name " + newName + " already exists");
+ throw new ResourceConflictException("group with name " + newName + " already exists", e);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/group/PutOptions.java b/java/com/google/gerrit/server/restapi/group/PutOptions.java
index c96972f..53bf571 100644
--- a/java/com/google/gerrit/server/restapi/group/PutOptions.java
+++ b/java/com/google/gerrit/server/restapi/group/PutOptions.java
@@ -66,7 +66,7 @@
try {
groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate);
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+ throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid), e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/group/PutOwner.java b/java/com/google/gerrit/server/restapi/group/PutOwner.java
index 709f7e3..04129af 100644
--- a/java/com/google/gerrit/server/restapi/group/PutOwner.java
+++ b/java/com/google/gerrit/server/restapi/group/PutOwner.java
@@ -77,7 +77,7 @@
try {
groupsUpdateProvider.get().updateGroup(groupUuid, groupUpdate);
} catch (NoSuchGroupException e) {
- throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
+ throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid), e);
}
}
return Response.ok(json.format(owner));
diff --git a/java/com/google/gerrit/server/restapi/project/BranchesCollection.java b/java/com/google/gerrit/server/restapi/project/BranchesCollection.java
index fe2fb0f..2d78bb0 100644
--- a/java/com/google/gerrit/server/restapi/project/BranchesCollection.java
+++ b/java/com/google/gerrit/server/restapi/project/BranchesCollection.java
@@ -82,9 +82,9 @@
.check(RefPermission.READ);
return new BranchResource(parent.getProjectState(), parent.getUser(), ref);
} catch (AuthException notAllowed) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, notAllowed);
} catch (RepositoryNotFoundException noRepo) {
- throw new ResourceNotFoundException();
+ throw new ResourceNotFoundException(id, noRepo);
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/CommitsCollection.java b/java/com/google/gerrit/server/restapi/project/CommitsCollection.java
index 757b650..21d7f0b 100644
--- a/java/com/google/gerrit/server/restapi/project/CommitsCollection.java
+++ b/java/com/google/gerrit/server/restapi/project/CommitsCollection.java
@@ -87,7 +87,7 @@
try {
objectId = ObjectId.fromString(id.get());
} catch (IllegalArgumentException e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
}
try (Repository repo = repoManager.openRepository(parent.getNameKey());
@@ -102,7 +102,7 @@
}
return new CommitResource(parent, commit);
} catch (MissingObjectException | IncorrectObjectTypeException e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/CreateAccessChange.java b/java/com/google/gerrit/server/restapi/project/CreateAccessChange.java
index 76f1105..fe48301 100644
--- a/java/com/google/gerrit/server/restapi/project/CreateAccessChange.java
+++ b/java/com/google/gerrit/server/restapi/project/CreateAccessChange.java
@@ -102,7 +102,7 @@
try {
forProject.ref(RefNames.REFS_CONFIG).check(RefPermission.CREATE_CHANGE);
} catch (AuthException denied) {
- throw new AuthException("cannot create change for " + RefNames.REFS_CONFIG);
+ throw new AuthException("cannot create change for " + RefNames.REFS_CONFIG, denied);
}
}
projectCache.checkedGet(rsrc.getNameKey()).checkStatePermitsWrite();
diff --git a/java/com/google/gerrit/server/restapi/project/CreateBranch.java b/java/com/google/gerrit/server/restapi/project/CreateBranch.java
index 67213c5..c15fdeb 100644
--- a/java/com/google/gerrit/server/restapi/project/CreateBranch.java
+++ b/java/com/google/gerrit/server/restapi/project/CreateBranch.java
@@ -127,7 +127,7 @@
try {
object = rw.parseCommit(object);
} catch (IncorrectObjectTypeException notCommit) {
- throw new BadRequestException("\"" + input.revision + "\" not a commit");
+ throw new BadRequestException("\"" + input.revision + "\" not a commit", notCommit);
}
}
@@ -197,7 +197,7 @@
throw err;
}
} catch (RefUtil.InvalidRevisionException e) {
- throw new BadRequestException("invalid revision \"" + input.revision + "\"");
+ throw new BadRequestException("invalid revision \"" + input.revision + "\"", e);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/CreateTag.java b/java/com/google/gerrit/server/restapi/project/CreateTag.java
index 8fdf5e4..b087f15 100644
--- a/java/com/google/gerrit/server/restapi/project/CreateTag.java
+++ b/java/com/google/gerrit/server/restapi/project/CreateTag.java
@@ -149,7 +149,7 @@
}
}
} catch (InvalidRevisionException e) {
- throw new BadRequestException("Invalid base revision");
+ throw new BadRequestException("Invalid base revision", e);
} catch (GitAPIException e) {
logger.atSevere().withCause(e).log("Cannot create tag \"%s\"", ref);
throw new IOException(e);
diff --git a/java/com/google/gerrit/server/restapi/project/DashboardsCollection.java b/java/com/google/gerrit/server/restapi/project/DashboardsCollection.java
index 606f3bef..ca48109 100644
--- a/java/com/google/gerrit/server/restapi/project/DashboardsCollection.java
+++ b/java/com/google/gerrit/server/restapi/project/DashboardsCollection.java
@@ -103,14 +103,14 @@
try {
info = newDashboardInfo(id.get());
} catch (InvalidDashboardId e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
}
for (ProjectState ps : parent.getProjectState().tree()) {
try {
return parse(ps, parent.getProjectState(), parent.getUser(), info);
} catch (AmbiguousObjectException | ConfigInvalidException | IncorrectObjectTypeException e) {
- throw new ResourceNotFoundException(id);
+ throw new ResourceNotFoundException(id, e);
} catch (ResourceNotFoundException e) {
continue;
}
@@ -135,7 +135,7 @@
permissionBackend.user(user).project(parent.getNameKey()).ref(ref).check(RefPermission.READ);
} catch (AuthException e) {
// Don't leak the project's existence
- throw new ResourceNotFoundException(info.id);
+ throw new ResourceNotFoundException(info.id, e);
}
if (!Repository.isValidRefName(ref)) {
throw new ResourceNotFoundException(info.id);
@@ -151,7 +151,7 @@
BlobBasedConfig cfg = new BlobBasedConfig(null, git, objId);
return new DashboardResource(current, user, ref, info.path, cfg, false);
} catch (RepositoryNotFoundException e) {
- throw new ResourceNotFoundException(info.id);
+ throw new ResourceNotFoundException(info.id, e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/GetAccess.java b/java/com/google/gerrit/server/restapi/project/GetAccess.java
index 550c4c4..a9a9403 100644
--- a/java/com/google/gerrit/server/restapi/project/GetAccess.java
+++ b/java/com/google/gerrit/server/restapi/project/GetAccess.java
@@ -165,7 +165,7 @@
} catch (ConfigInvalidException e) {
throw new ResourceConflictException(e.getMessage());
} catch (RepositoryNotFoundException e) {
- throw new ResourceNotFoundException(rsrc.getName());
+ throw new ResourceNotFoundException(rsrc.getName(), e);
}
// The following implementation must match the ProjectAccessFactory JSON RPC endpoint.
diff --git a/java/com/google/gerrit/server/restapi/project/GetHead.java b/java/com/google/gerrit/server/restapi/project/GetHead.java
index 928db97..4e0a144 100644
--- a/java/com/google/gerrit/server/restapi/project/GetHead.java
+++ b/java/com/google/gerrit/server/restapi/project/GetHead.java
@@ -82,13 +82,13 @@
.project(rsrc.getNameKey())
.check(ProjectPermission.WRITE_CONFIG);
} catch (AuthException ae) {
- throw new AuthException("not allowed to see HEAD");
+ throw new AuthException("not allowed to see HEAD", ae);
}
}
}
throw new ResourceNotFoundException(Constants.HEAD);
} catch (RepositoryNotFoundException e) {
- throw new ResourceNotFoundException(rsrc.getName());
+ throw new ResourceNotFoundException(rsrc.getName(), e);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/GetReflog.java b/java/com/google/gerrit/server/restapi/project/GetReflog.java
index a249f26..f9c6fd9 100644
--- a/java/com/google/gerrit/server/restapi/project/GetReflog.java
+++ b/java/com/google/gerrit/server/restapi/project/GetReflog.java
@@ -104,7 +104,7 @@
} catch (UnsupportedOperationException e) {
String msg = "reflog not supported on repo " + rsrc.getNameKey().get();
logger.atSevere().log(msg);
- throw new MethodNotAllowedException(msg);
+ throw new MethodNotAllowedException(msg, e);
}
if (r == null) {
throw new ResourceNotFoundException(rsrc.getRef());
diff --git a/java/com/google/gerrit/server/restapi/project/GetStatistics.java b/java/com/google/gerrit/server/restapi/project/GetStatistics.java
index d68e0af..db97855 100644
--- a/java/com/google/gerrit/server/restapi/project/GetStatistics.java
+++ b/java/com/google/gerrit/server/restapi/project/GetStatistics.java
@@ -51,7 +51,7 @@
} catch (GitAPIException | JGitInternalException e) {
throw new ResourceConflictException(e.getMessage());
} catch (IOException e) {
- throw new ResourceNotFoundException(rsrc.getName());
+ throw new ResourceNotFoundException(rsrc.getName(), e);
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/ListBranches.java b/java/com/google/gerrit/server/restapi/project/ListBranches.java
index cd20820..fecdc8e 100644
--- a/java/com/google/gerrit/server/restapi/project/ListBranches.java
+++ b/java/com/google/gerrit/server/restapi/project/ListBranches.java
@@ -149,7 +149,7 @@
}
return toBranchInfo(rsrc, ImmutableList.of(r)).get(0);
} catch (RepositoryNotFoundException noRepo) {
- throw new ResourceNotFoundException();
+ throw new ResourceNotFoundException(rsrc.getNameKey().get(), noRepo);
}
}
@@ -165,7 +165,7 @@
.exactRef(Constants.HEAD, RefNames.REFS_CONFIG, RefNames.REFS_USERS_DEFAULT)
.values());
} catch (RepositoryNotFoundException noGitRepository) {
- throw new ResourceNotFoundException();
+ throw new ResourceNotFoundException(rsrc.getNameKey().get(), noGitRepository);
}
return toBranchInfo(rsrc, refs);
}
diff --git a/java/com/google/gerrit/server/restapi/project/ListDashboards.java b/java/com/google/gerrit/server/restapi/project/ListDashboards.java
index 058083c..4406719 100644
--- a/java/com/google/gerrit/server/restapi/project/ListDashboards.java
+++ b/java/com/google/gerrit/server/restapi/project/ListDashboards.java
@@ -120,7 +120,7 @@
}
return all;
} catch (RepositoryNotFoundException e) {
- throw new ResourceNotFoundException();
+ throw new ResourceNotFoundException(project, e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/ListTags.java b/java/com/google/gerrit/server/restapi/project/ListTags.java
index 8cea7f5..123c78a 100644
--- a/java/com/google/gerrit/server/restapi/project/ListTags.java
+++ b/java/com/google/gerrit/server/restapi/project/ListTags.java
@@ -219,7 +219,7 @@
try {
return repoManager.openRepository(project);
} catch (RepositoryNotFoundException noGitRepository) {
- throw new ResourceNotFoundException();
+ throw new ResourceNotFoundException(project.get(), noGitRepository);
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/PutConfig.java b/java/com/google/gerrit/server/restapi/project/PutConfig.java
index a0badd7..9f9433b 100644
--- a/java/com/google/gerrit/server/restapi/project/PutConfig.java
+++ b/java/com/google/gerrit/server/restapi/project/PutConfig.java
@@ -190,7 +190,7 @@
uiActions,
views);
} catch (RepositoryNotFoundException notFound) {
- throw new ResourceNotFoundException(projectName.get());
+ throw new ResourceNotFoundException(projectName.get(), notFound);
} catch (ConfigInvalidException err) {
throw new ResourceConflictException("Cannot read project " + projectName, err);
} catch (IOException err) {
diff --git a/java/com/google/gerrit/server/restapi/project/PutDescription.java b/java/com/google/gerrit/server/restapi/project/PutDescription.java
index 0281b8d..a0b9feb 100644
--- a/java/com/google/gerrit/server/restapi/project/PutDescription.java
+++ b/java/com/google/gerrit/server/restapi/project/PutDescription.java
@@ -92,7 +92,7 @@
? Response.none()
: Response.ok(project.getDescription());
} catch (RepositoryNotFoundException notFound) {
- throw new ResourceNotFoundException(resource.getName());
+ throw new ResourceNotFoundException(resource.getName(), notFound);
} catch (ConfigInvalidException e) {
throw new ResourceConflictException(
String.format("invalid project.config: %s", e.getMessage()));
diff --git a/java/com/google/gerrit/server/restapi/project/SetAccess.java b/java/com/google/gerrit/server/restapi/project/SetAccess.java
index f8c7234..02c1b54 100644
--- a/java/com/google/gerrit/server/restapi/project/SetAccess.java
+++ b/java/com/google/gerrit/server/restapi/project/SetAccess.java
@@ -130,7 +130,7 @@
} catch (InvalidNameException e) {
throw new BadRequestException(e.toString());
} catch (ConfigInvalidException e) {
- throw new ResourceConflictException(rsrc.getName());
+ throw new ResourceConflictException(rsrc.getName(), e);
}
return Response.ok(getAccess.apply(rsrc.getNameKey()));
diff --git a/java/com/google/gerrit/server/restapi/project/SetDefaultDashboard.java b/java/com/google/gerrit/server/restapi/project/SetDefaultDashboard.java
index 8879fae..9920be0 100644
--- a/java/com/google/gerrit/server/restapi/project/SetDefaultDashboard.java
+++ b/java/com/google/gerrit/server/restapi/project/SetDefaultDashboard.java
@@ -89,7 +89,7 @@
new ProjectResource(rsrc.getProjectState(), rsrc.getUser()),
IdString.fromUrl(input.id));
} catch (ResourceNotFoundException e) {
- throw new BadRequestException("dashboard " + input.id + " not found");
+ throw new BadRequestException("dashboard " + input.id + " not found", e);
} catch (ConfigInvalidException e) {
throw new ResourceConflictException(e.getMessage());
}
@@ -125,7 +125,7 @@
}
return Response.none();
} catch (RepositoryNotFoundException notFound) {
- throw new ResourceNotFoundException(rsrc.getProjectState().getProject().getName());
+ throw new ResourceNotFoundException(rsrc.getProjectState().getProject().getName(), notFound);
} catch (ConfigInvalidException e) {
throw new ResourceConflictException(
String.format("invalid project.config: %s", e.getMessage()));
diff --git a/java/com/google/gerrit/server/restapi/project/SetHead.java b/java/com/google/gerrit/server/restapi/project/SetHead.java
index 946695e..ae70e46 100644
--- a/java/com/google/gerrit/server/restapi/project/SetHead.java
+++ b/java/com/google/gerrit/server/restapi/project/SetHead.java
@@ -114,7 +114,7 @@
}
return Response.ok(ref);
} catch (RepositoryNotFoundException e) {
- throw new ResourceNotFoundException(rsrc.getName());
+ throw new ResourceNotFoundException(rsrc.getName(), e);
}
}
diff --git a/java/com/google/gerrit/server/restapi/project/SetParent.java b/java/com/google/gerrit/server/restapi/project/SetParent.java
index 7241fcf..4cf0182 100644
--- a/java/com/google/gerrit/server/restapi/project/SetParent.java
+++ b/java/com/google/gerrit/server/restapi/project/SetParent.java
@@ -121,7 +121,7 @@
requireNonNull(parent);
return parent.get();
} catch (RepositoryNotFoundException notFound) {
- throw new ResourceNotFoundException(rsrc.getName());
+ throw new ResourceNotFoundException(rsrc.getName(), notFound);
} catch (ConfigInvalidException e) {
throw new ResourceConflictException(
String.format("invalid project.config: %s", e.getMessage()));
diff --git a/java/com/google/gerrit/server/rules/StoredValues.java b/java/com/google/gerrit/server/rules/StoredValues.java
index 085475e..1e08a24 100644
--- a/java/com/google/gerrit/server/rules/StoredValues.java
+++ b/java/com/google/gerrit/server/rules/StoredValues.java
@@ -57,7 +57,8 @@
try {
return cd.change();
} catch (StorageException e) {
- throw new SystemException("Cannot load change " + cd.getId());
+ throw new SystemException(
+ String.format("Cannot load change %s: %s", cd.getId(), e.getMessage()));
}
}
@@ -101,7 +102,7 @@
try {
patchList = plCache.get(plKey, project);
} catch (PatchListNotAvailableException e) {
- throw new SystemException("Cannot create " + plKey);
+ throw new SystemException(String.format("Cannot create %s: %s", plKey, e.getMessage()));
}
return patchList;
}
diff --git a/java/com/google/gerrit/server/schema/NoteDbSchemaUpdater.java b/java/com/google/gerrit/server/schema/NoteDbSchemaUpdater.java
index 5dec581..0e22af9 100644
--- a/java/com/google/gerrit/server/schema/NoteDbSchemaUpdater.java
+++ b/java/com/google/gerrit/server/schema/NoteDbSchemaUpdater.java
@@ -104,7 +104,7 @@
try {
schemaCreator.ensureCreated();
} catch (IOException | ConfigInvalidException e) {
- throw new StorageException("Cannot initialize Gerrit site");
+ throw new StorageException("Cannot initialize Gerrit site", e);
}
}
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index a6e01af..65e18ad 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -161,7 +161,8 @@
} catch (MergeConflictException mce) {
// Unlike in Cherry-pick case, this should never happen.
toMerge.setStatusCode(CommitMergeStatus.REBASE_MERGE_CONFLICT);
- throw new IllegalStateException("MergeConflictException on message edit must not happen");
+ throw new IllegalStateException(
+ "MergeConflictException on message edit must not happen", mce);
} catch (MergeIdenticalTreeException mie) {
// this should not happen
toMerge.setStatusCode(SKIPPED_IDENTICAL_TREE);
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 6be4a91..cc40a30 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -550,7 +550,7 @@
return args.submoduleOp.composeGitlinksCommit(args.destBranch, commit);
} catch (SubmoduleException | IOException e) {
throw new IntegrationException(
- "cannot update gitlink for the commit at branch: " + args.destBranch);
+ "cannot update gitlink for the commit at branch: " + args.destBranch, e);
}
}
}
diff --git a/java/com/google/gerrit/server/util/SocketUtil.java b/java/com/google/gerrit/server/util/SocketUtil.java
index afa2aee..e617650 100644
--- a/java/com/google/gerrit/server/util/SocketUtil.java
+++ b/java/com/google/gerrit/server/util/SocketUtil.java
@@ -96,7 +96,7 @@
try {
port = Integer.parseInt(portStr);
} catch (NumberFormatException e) {
- throw new IllegalArgumentException("invalid port: " + desc);
+ throw new IllegalArgumentException("invalid port: " + desc, e);
}
} else {
port = defaultPort;
diff --git a/java/com/google/gerrit/sshd/BaseCommand.java b/java/com/google/gerrit/sshd/BaseCommand.java
index ac22dfc..5d7dba2 100644
--- a/java/com/google/gerrit/sshd/BaseCommand.java
+++ b/java/com/google/gerrit/sshd/BaseCommand.java
@@ -387,6 +387,10 @@
return new UnloggedFailure(1, "fatal: " + msg);
}
+ protected UnloggedFailure die(String msg, Throwable why) {
+ return new UnloggedFailure(1, "fatal: " + msg, why);
+ }
+
protected UnloggedFailure die(Throwable why) {
return new UnloggedFailure(1, "fatal: " + why.getMessage(), why);
}
diff --git a/java/com/google/gerrit/sshd/ChangeArgumentParser.java b/java/com/google/gerrit/sshd/ChangeArgumentParser.java
index c273b4c..491bcb8 100644
--- a/java/com/google/gerrit/sshd/ChangeArgumentParser.java
+++ b/java/com/google/gerrit/sshd/ChangeArgumentParser.java
@@ -111,7 +111,7 @@
try {
changeResource = changesCollection.parse(cId);
} catch (RestApiException e) {
- throw new UnloggedFailure(1, "\"" + id + "\" no such change");
+ throw new UnloggedFailure(1, "\"" + id + "\" no such change", e);
}
changes.put(cId, changeResource);
}
diff --git a/java/com/google/gerrit/sshd/SshKeyCreatorImpl.java b/java/com/google/gerrit/sshd/SshKeyCreatorImpl.java
index e6c3a40..02a9e48 100644
--- a/java/com/google/gerrit/sshd/SshKeyCreatorImpl.java
+++ b/java/com/google/gerrit/sshd/SshKeyCreatorImpl.java
@@ -34,11 +34,10 @@
SshUtil.parse(key);
return key;
} catch (NoSuchAlgorithmException | InvalidKeySpecException e) {
- throw new InvalidSshKeyException();
-
+ throw new InvalidSshKeyException(e);
} catch (NoSuchProviderException e) {
logger.atSevere().withCause(e).log("Cannot parse SSH key");
- throw new InvalidSshKeyException();
+ throw new InvalidSshKeyException(e);
}
}
}
diff --git a/java/com/google/gerrit/sshd/SuExec.java b/java/com/google/gerrit/sshd/SuExec.java
index f485bd7..ea163d5 100644
--- a/java/com/google/gerrit/sshd/SuExec.java
+++ b/java/com/google/gerrit/sshd/SuExec.java
@@ -127,9 +127,9 @@
try {
permissionBackend.user(caller).check(GlobalPermission.RUN_AS);
} catch (AuthException e) {
- throw die("suexec not permitted");
+ throw die("suexec not permitted", e);
} catch (PermissionBackendException e) {
- throw die("suexec not available: " + e);
+ throw die("suexec not available", e);
}
}
}
diff --git a/java/com/google/gerrit/sshd/commands/LsUserRefs.java b/java/com/google/gerrit/sshd/commands/LsUserRefs.java
index b4a2b42..80aee01 100644
--- a/java/com/google/gerrit/sshd/commands/LsUserRefs.java
+++ b/java/com/google/gerrit/sshd/commands/LsUserRefs.java
@@ -104,9 +104,9 @@
throw new Failure(1, "fatal: Error reading refs: '" + projectName, e);
}
} catch (RepositoryNotFoundException e) {
- throw die("'" + projectName + "': not a git archive");
+ throw die("'" + projectName + "': not a git archive", e);
} catch (IOException e) {
- throw die("Error opening: '" + projectName);
+ throw die("Error opening: '" + projectName, e);
}
}
}
diff --git a/java/com/google/gerrit/sshd/commands/PatchSetParser.java b/java/com/google/gerrit/sshd/commands/PatchSetParser.java
index e40ab87..f804c2d 100644
--- a/java/com/google/gerrit/sshd/commands/PatchSetParser.java
+++ b/java/com/google/gerrit/sshd/commands/PatchSetParser.java
@@ -99,7 +99,7 @@
try {
patchSetId = PatchSet.Id.parse(token);
} catch (IllegalArgumentException e) {
- throw error("\"" + token + "\" is not a valid patch set");
+ throw error("\"" + token + "\" is not a valid patch set", e);
}
ChangeNotes notes = getNotes(projectState, patchSetId.changeId());
PatchSet patchSet = psUtil.get(notes, patchSetId);
@@ -130,7 +130,7 @@
ChangeNotes notes = changeFinder.findOne(changeId);
return notesFactory.create(notes.getProjectName(), changeId);
} catch (NoSuchChangeException e) {
- throw error("\"" + changeId + "\" no such change");
+ throw error("\"" + changeId + "\" no such change", e);
}
}
@@ -153,4 +153,8 @@
public static UnloggedFailure error(String msg) {
return new UnloggedFailure(1, msg);
}
+
+ public static UnloggedFailure error(String msg, Throwable why) {
+ return new UnloggedFailure(1, msg, why);
+ }
}
diff --git a/java/com/google/gerrit/sshd/commands/PluginInstallCommand.java b/java/com/google/gerrit/sshd/commands/PluginInstallCommand.java
index 8b045ec..cfb47f7 100644
--- a/java/com/google/gerrit/sshd/commands/PluginInstallCommand.java
+++ b/java/com/google/gerrit/sshd/commands/PluginInstallCommand.java
@@ -70,21 +70,21 @@
try {
data = Files.newInputStream(new File(source).toPath());
} catch (IOException e) {
- throw die("cannot read " + source);
+ throw die("cannot read " + source, e);
}
} else {
try {
data = new URL(source).openStream();
} catch (MalformedURLException e) {
- throw die("invalid url " + source);
+ throw die("invalid url " + source, e);
} catch (IOException e) {
- throw die("cannot read " + source);
+ throw die("cannot read " + source, e);
}
}
try {
loader.installPluginFromStream(name, data);
} catch (IOException e) {
- throw die("cannot install plugin");
+ throw die("cannot install plugin", e);
} catch (PluginInstallException e) {
e.printStackTrace(stderr);
String msg = String.format("Plugin failed to install. Cause: %s", e.getMessage());
diff --git a/java/com/google/gerrit/sshd/commands/Receive.java b/java/com/google/gerrit/sshd/commands/Receive.java
index e2fb633..6791b70 100644
--- a/java/com/google/gerrit/sshd/commands/Receive.java
+++ b/java/com/google/gerrit/sshd/commands/Receive.java
@@ -78,7 +78,7 @@
.project(project.getNameKey())
.check(ProjectPermission.RUN_RECEIVE_PACK);
} catch (AuthException e) {
- throw new Failure(1, "fatal: receive-pack not permitted on this server");
+ throw new Failure(1, "fatal: receive-pack not permitted on this server", e);
} catch (PermissionBackendException e) {
throw new Failure(1, "fatal: unable to check permissions " + e);
}
diff --git a/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/java/com/google/gerrit/sshd/commands/ReviewCommand.java
index 886f21e..3a1bf6c 100644
--- a/java/com/google/gerrit/sshd/commands/ReviewCommand.java
+++ b/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -327,7 +327,7 @@
try {
allProjectsState = projectCache.checkedGet(allProjects);
} catch (IOException e) {
- throw die("missing " + allProjects.get());
+ throw die("missing " + allProjects.get(), e);
}
for (LabelType type : allProjectsState.getLabelTypes().getLabelTypes()) {
diff --git a/java/com/google/gerrit/sshd/commands/Upload.java b/java/com/google/gerrit/sshd/commands/Upload.java
index 87ae493..6c4e80a 100644
--- a/java/com/google/gerrit/sshd/commands/Upload.java
+++ b/java/com/google/gerrit/sshd/commands/Upload.java
@@ -61,9 +61,9 @@
try {
perm.check(ProjectPermission.RUN_UPLOAD_PACK);
} catch (AuthException e) {
- throw new Failure(1, "fatal: upload-pack not permitted on this server");
+ throw new Failure(1, "fatal: upload-pack not permitted on this server", e);
} catch (PermissionBackendException e) {
- throw new Failure(1, "fatal: unable to check permissions " + e);
+ throw new Failure(1, "fatal: unable to check permissions ", e);
}
Repository permissionAwareRepo = PermissionAwareRepositoryManager.wrap(repo, perm);
diff --git a/java/com/google/gerrit/testing/IndexConfig.java b/java/com/google/gerrit/testing/IndexConfig.java
index fb6c926..a8ed5be 100644
--- a/java/com/google/gerrit/testing/IndexConfig.java
+++ b/java/com/google/gerrit/testing/IndexConfig.java
@@ -14,6 +14,7 @@
package com.google.gerrit.testing;
+import com.google.gerrit.server.change.MergeabilityComputationBehavior;
import org.eclipse.jgit.lib.Config;
public class IndexConfig {
@@ -23,9 +24,10 @@
public static Config createFromExistingConfig(Config cfg) {
cfg.setInt("index", null, "maxPages", 10);
- // To avoid this flakiness indexMergeable is switched off for the tests as it incurs background
+ // To avoid this flakiness indexing mergeable is disabled for the tests as it incurs background
// reindex calls.
- cfg.setBoolean("index", "change", "indexMergeable", false);
+ cfg.setEnum(
+ "change", null, "mergeabilityComputationBehavior", MergeabilityComputationBehavior.NEVER);
cfg.setString("trackingid", "query-bug", "footer", "Bug:");
cfg.setString("trackingid", "query-bug", "match", "QUERY\\d{2,8}");
cfg.setString("trackingid", "query-bug", "system", "querytests");
diff --git a/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java b/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java
index 7ecbe69..d04eebd 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java
@@ -143,7 +143,9 @@
@UseClockStep
@GerritConfig(name = "changeCleanup.abandonAfter", value = "1w")
@GerritConfig(name = "changeCleanup.abandonIfMergeable", value = "false")
- @GerritConfig(name = "index.change.indexMergeable", value = "true")
+ @GerritConfig(
+ name = "change.mergeabilityComputationBehavior",
+ value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
public void notAbandonedIfMergeableWhenMergeableOperatorIsEnabled() throws Exception {
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
@@ -180,14 +182,14 @@
}
/**
- * When indexMergeable is disabled then the abandonIfMergeable option is ineffective and the auto
- * abandon behaves as though it were set to its default value (true).
+ * When indexing mergeable is disabled then the abandonIfMergeable option is ineffective and the
+ * auto abandon behaves as though it were set to its default value (true).
*/
@Test
@UseClockStep
@GerritConfig(name = "changeCleanup.abandonAfter", value = "1w")
@GerritConfig(name = "changeCleanup.abandonIfMergeable", value = "false")
- @GerritConfig(name = "index.change.indexMergeable", value = "false")
+ @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
public void abandonedIfMergeableWhenMergeableOperatorIsDisabled() throws Exception {
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 74f753d..d5686d1 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -295,19 +295,7 @@
}
@Test
- public void skipMergeable() throws Exception {
- PushOneCommit.Result r = createChange();
- String triplet = project.get() + "~master~" + r.getChangeId();
- ChangeInfo c =
- gApi.changes().id(triplet).get(ImmutableList.of(ListChangesOption.SKIP_MERGEABLE));
- assertThat(c.mergeable).isNull();
-
- c = gApi.changes().id(triplet).get();
- assertThat(c.mergeable).isTrue();
- }
-
- @Test
- @GerritConfig(name = "change.api.excludeMergeableInChangeInfo", value = "true")
+ @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
public void excludeMergeableInChangeInfo() throws Exception {
PushOneCommit.Result r = createChange();
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
@@ -4425,7 +4413,6 @@
ListChangesOption.MESSAGES,
ListChangesOption.SUBMITTABLE,
ListChangesOption.WEB_LINKS,
- ListChangesOption.SKIP_MERGEABLE,
ListChangesOption.SKIP_DIFFSTAT);
PushOneCommit.Result change = createChange();
@@ -4438,14 +4425,16 @@
}
@Test
- @GerritConfig(name = "index.change.indexMergeable", value = "true")
+ @GerritConfig(
+ name = "change.mergeabilityComputationBehavior",
+ value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
public void changeQueryReturnsMergeableWhenGerritIndexMergeable() throws Exception {
String changeId = createChange().getChangeId();
assertThat(gApi.changes().query(changeId).get().get(0).mergeable).isTrue();
}
@Test
- @GerritConfig(name = "index.change.indexMergeable", value = "false")
+ @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
public void changeQueryDoesNotReturnMergeableWhenGerritDoesNotIndexMergeable() throws Exception {
String changeId = createChange().getChangeId();
assertThat(gApi.changes().query(changeId).get().get(0).mergeable).isNull();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 524a05e..707076c 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -16,7 +16,10 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.util.stream.Collectors.toList;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -25,19 +28,23 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
+import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
+import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
+import com.google.gerrit.extensions.common.RobotCommentInfo;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.validators.CommentForValidation;
-import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.restapi.change.PostReview;
@@ -46,9 +53,12 @@
import com.google.inject.Inject;
import com.google.inject.Module;
import java.sql.Timestamp;
+import java.util.Collection;
+import java.util.List;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
+import org.mockito.ArgumentMatcher;
import org.mockito.Captor;
/** Tests for comment validation in {@link PostReview}. */
@@ -57,8 +67,43 @@
@Inject private TestCommentHelper testCommentHelper;
private static final String COMMENT_TEXT = "The comment text";
+ private static final CommentForValidation FILE_COMMENT_FOR_VALIDATION =
+ CommentForValidation.create(
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.FILE_COMMENT,
+ COMMENT_TEXT,
+ COMMENT_TEXT.length());
+ private static final CommentForValidation INLINE_COMMENT_FOR_VALIDATION =
+ CommentForValidation.create(
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.INLINE_COMMENT,
+ COMMENT_TEXT,
+ COMMENT_TEXT.length());
+ private static final CommentForValidation CHANGE_MESSAGE_FOR_VALIDATION =
+ CommentForValidation.create(
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.CHANGE_MESSAGE,
+ COMMENT_TEXT,
+ COMMENT_TEXT.length());
- @Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> capture;
+ @Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> captor;
+
+ private static class SingleCommentMatcher
+ implements ArgumentMatcher<ImmutableList<CommentForValidation>> {
+ final CommentForValidation left;
+
+ SingleCommentMatcher(CommentForValidation left) {
+ this.left = left;
+ }
+
+ @Override
+ public boolean matches(ImmutableList<CommentForValidation> rightList) {
+ CommentForValidation right = Iterables.getOnlyElement(rightList);
+ return left.getSource() == right.getSource()
+ && left.getType() == right.getType()
+ && left.getText().equals(right.getText());
+ }
+ }
@Override
public Module createModule() {
@@ -84,13 +129,7 @@
public void validateCommentsInInput_commentOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder()
- .changeId(r.getChange().getId().get())
- .project(r.getChange().project().get())
- .build(),
- ImmutableList.of(
- CommentForValidation.create(
- CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
+ eq(contextFor(r)), argThat(new SingleCommentMatcher(FILE_COMMENT_FOR_VALIDATION))))
.thenReturn(ImmutableList.of());
ReviewInput input = new ReviewInput();
@@ -107,15 +146,9 @@
@Test
public void validateCommentsInInput_commentRejected() throws Exception {
PushOneCommit.Result r = createChange();
- CommentForValidation commentForValidation =
- CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder()
- .changeId(r.getChange().getId().get())
- .project(r.getChange().project().get())
- .build(),
- ImmutableList.of(CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT))))
- .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ eq(contextFor(r)), argThat(new SingleCommentMatcher(FILE_COMMENT_FOR_VALIDATION))))
+ .thenReturn(ImmutableList.of(FILE_COMMENT_FOR_VALIDATION.failValidation("Oh no!")));
ReviewInput input = new ReviewInput();
CommentInput comment = newComment(r.getChange().currentFilePaths().get(0));
@@ -161,19 +194,13 @@
public void validateDrafts_draftOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder()
- .changeId(r.getChange().getId().get())
- .project(r.getChange().project().get())
- .build(),
- ImmutableList.of(
- CommentForValidation.create(
- CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
+ eq(contextFor(r)), argThat(new SingleCommentMatcher((INLINE_COMMENT_FOR_VALIDATION)))))
.thenReturn(ImmutableList.of());
DraftInput draft =
testCommentHelper.newDraft(
r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
- gApi.changes().id(r.getChangeId()).revision(r.getCommit().getName()).createDraft(draft).get();
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().getName()).createDraft(draft);
assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
ReviewInput input = new ReviewInput();
@@ -186,17 +213,9 @@
@Test
public void validateDrafts_draftRejected() throws Exception {
PushOneCommit.Result r = createChange();
- CommentForValidation commentForValidation =
- CommentForValidation.create(CommentType.INLINE_COMMENT, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder()
- .changeId(r.getChange().getId().get())
- .project(r.getChange().project().get())
- .build(),
- ImmutableList.of(
- CommentForValidation.create(
- CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
- .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ eq(contextFor(r)), argThat(new SingleCommentMatcher(INLINE_COMMENT_FOR_VALIDATION))))
+ .thenReturn(ImmutableList.of(INLINE_COMMENT_FOR_VALIDATION.failValidation("Oh no!")));
DraftInput draft =
testCommentHelper.newDraft(
@@ -233,7 +252,7 @@
testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draftFile);
assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
- when(mockCommentValidator.validateComments(any(), capture.capture()))
+ when(mockCommentValidator.validateComments(any(), captor.capture()))
.thenReturn(ImmutableList.of());
ReviewInput input = new ReviewInput();
@@ -241,25 +260,33 @@
gApi.changes().id(r.getChangeId()).current().review(input);
assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(2);
- assertThat(capture.getAllValues()).hasSize(1);
- assertThat(capture.getValue())
+ assertThat(captor.getAllValues()).hasSize(1);
+ assertThat(captor.getValue())
+ .comparingElementsUsing(
+ Correspondence.<CommentForValidation, CommentForValidation>from(
+ (a, b) ->
+ a.getSource() == b.getSource()
+ && a.getType() == b.getType()
+ && a.getText().equals(b.getText()),
+ "matches (ignoring size approximation)"))
.containsExactly(
CommentForValidation.create(
- CommentForValidation.CommentType.INLINE_COMMENT, draftInline.message),
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.INLINE_COMMENT,
+ draftInline.message,
+ draftInline.message.length()),
CommentForValidation.create(
- CommentForValidation.CommentType.FILE_COMMENT, draftFile.message));
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.FILE_COMMENT,
+ draftFile.message,
+ draftFile.message.length()));
}
@Test
public void validateCommentsInChangeMessage_messageOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder()
- .changeId(r.getChange().getId().get())
- .project(r.getChange().project().get())
- .build(),
- ImmutableList.of(
- CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
+ contextFor(r), ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION)))
.thenReturn(ImmutableList.of());
ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
@@ -274,16 +301,9 @@
@Test
public void validateCommentsInChangeMessage_messageRejected() throws Exception {
PushOneCommit.Result r = createChange();
- CommentForValidation commentForValidation =
- CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder()
- .changeId(r.getChange().getId().get())
- .project(r.getChange().project().get())
- .build(),
- ImmutableList.of(
- CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
- .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ contextFor(r), ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION)))
+ .thenReturn(ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION.failValidation("Oh no!")));
ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
assertThat(gApi.changes().id(r.getChangeId()).get().messages)
@@ -308,7 +328,56 @@
assertThat(message.message).doesNotContain(COMMENT_TEXT);
}
+ @Test
+ @GerritConfig(name = "change.maxComments", value = "4")
+ public void restrictNumberOfComments() throws Exception {
+ when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
+
+ PushOneCommit.Result r = createChange();
+ String filePath = r.getChange().currentFilePaths().get(0);
+ CommentInput commentInput = new CommentInput();
+ commentInput.line = 1;
+ commentInput.message = "foo";
+ commentInput.path = filePath;
+ RobotCommentInput robotCommentInput =
+ TestCommentHelper.createRobotCommentInputWithMandatoryFields(filePath);
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.comments = ImmutableMap.of(filePath, ImmutableList.of(commentInput));
+ reviewInput.robotComments = ImmutableMap.of(filePath, ImmutableList.of(robotCommentInput));
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+
+ // reviewInput still has both a user and a robot comment (and deduplication is false). We also
+ // create a draft so that in total there would be 5 comments. The limit is set to 4, so this
+ // verifies that all three channels are considered.
+ DraftInput draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, "a draft");
+ testCommentHelper.addDraft(r.getChangeId(), r.getPatchSetId().getId(), draftInline);
+ reviewInput.drafts = DraftHandling.PUBLISH;
+ reviewInput.omitDuplicateComments = false;
+
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().review(reviewInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .contains("Exceeding maximum number of comments: 2 (existing) + 3 (new) > 4");
+
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(1);
+ assertThat(getRobotComments(r.getChangeId())).hasSize(1);
+ }
+
+ private List<RobotCommentInfo> getRobotComments(String changeId) throws RestApiException {
+ return gApi.changes().id(changeId).robotComments().values().stream()
+ .flatMap(Collection::stream)
+ .collect(toList());
+ }
+
private static CommentInput newComment(String path) {
return TestCommentHelper.populate(new CommentInput(), path, PostReviewIT.COMMENT_TEXT);
}
+
+ private static CommentValidationContext contextFor(PushOneCommit.Result result) {
+ return CommentValidationContext.create(
+ result.getChange().getId().get(), result.getChange().project().get());
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index e6b2190..36b7265 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -1102,7 +1102,9 @@
}
@Test
- @GerritConfig(name = "index.change.indexMergeable", value = "true")
+ @GerritConfig(
+ name = "change.mergeabilityComputationBehavior",
+ value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
public void mergeable() throws Exception {
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
index c35ded6..6b92c16 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
@@ -173,9 +173,8 @@
@Test
public void hugeRobotCommentIsRejected() {
- int defaultSizeLimit = 1024 * 1024;
- int sizeOfRest = 451;
- fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - sizeOfRest + 1);
+ int defaultSizeLimit = 1 << 20;
+ fixReplacementInfo.replacement = getStringFor(defaultSizeLimit + 1);
BadRequestException thrown =
assertThrows(
@@ -186,9 +185,9 @@
@Test
public void reasonablyLargeRobotCommentIsAccepted() throws Exception {
- int defaultSizeLimit = 1024 * 1024;
- int sizeOfRest = 451;
- fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - sizeOfRest);
+ int defaultSizeLimit = 1 << 20;
+ // Allow for a few hundred bytes in other fields.
+ fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - 666);
testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);
@@ -199,7 +198,7 @@
@Test
@GerritConfig(name = "change.robotCommentSizeLimit", value = "10k")
public void maximumAllowedSizeOfRobotCommentCanBeAdjusted() {
- int sizeLimit = 10 * 1024;
+ int sizeLimit = 10 << 20;
fixReplacementInfo.replacement = getStringFor(sizeLimit);
BadRequestException thrown =
@@ -212,8 +211,8 @@
@Test
@GerritConfig(name = "change.robotCommentSizeLimit", value = "0")
public void zeroForMaximumAllowedSizeOfRobotCommentRemovesRestriction() throws Exception {
- int defaultSizeLimit = 1024 * 1024;
- fixReplacementInfo.replacement = getStringFor(defaultSizeLimit);
+ int defaultSizeLimit = 1 << 20;
+ fixReplacementInfo.replacement = getStringFor(2 * defaultSizeLimit);
testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);
@@ -225,8 +224,8 @@
@GerritConfig(name = "change.robotCommentSizeLimit", value = "-1")
public void negativeValueForMaximumAllowedSizeOfRobotCommentRemovesRestriction()
throws Exception {
- int defaultSizeLimit = 1024 * 1024;
- fixReplacementInfo.replacement = getStringFor(defaultSizeLimit);
+ int defaultSizeLimit = 1 << 20;
+ fixReplacementInfo.replacement = getStringFor(2 * defaultSizeLimit);
testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index 0c0a8ed..cdffa3c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -1205,7 +1205,9 @@
}
@Test
- @GerritConfig(name = "index.change.indexMergeable", value = "true")
+ @GerritConfig(
+ name = "change.mergeabilityComputationBehavior",
+ value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
public void submitSchedulesOpenChangesOfSameBranchForReindexing() throws Throwable {
// Create a merged change.
PushOneCommit push =
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java
index ea3a6a0..61dc4d4 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java
@@ -59,7 +59,9 @@
}
@Test
- @GerritConfig(name = "index.change.indexMergeable", value = "true")
+ @GerritConfig(
+ name = "change.mergeabilityComputationBehavior",
+ value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
public void indexChangeAfterOwnerLosesVisibility() throws Exception {
// Create a test group with 2 users as members
TestAccount user2 = accountCreator.user2();
diff --git a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
index 9573eb0..14a04cd 100644
--- a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
@@ -169,7 +169,8 @@
assertThat(i.change.updateDelay).isEqualTo(300);
assertThat(i.change.disablePrivateChanges).isNull();
assertThat(i.change.submitWholeTopic).isNull();
- assertThat(i.change.excludeMergeableInChangeInfo).isNull();
+ assertThat(i.change.mergeabilityComputationBehavior)
+ .isEqualTo("API_REF_UPDATED_AND_CHANGE_REINDEX");
// download
assertThat(i.download.archives).containsExactly("tar", "tbz2", "tgz", "txz");
@@ -180,9 +181,6 @@
assertThat(i.gerrit.allUsers).isEqualTo(AllUsersNameProvider.DEFAULT);
assertThat(i.gerrit.reportBugUrl).isNull();
- // index
- assertThat(i.index.change.indexMergeable).isNull(); // false in all tests
-
// plugin
assertThat(i.plugin.jsResourcePaths).isEmpty();
@@ -204,16 +202,9 @@
}
@Test
- @GerritConfig(name = "change.api.excludeMergeableInChangeInfo", value = "true")
- public void serverConfigWithExcludeMergeableInChangeInfo() throws Exception {
+ @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
+ public void mergeabilityComputationBehavior_neverCompute() throws Exception {
ServerInfo i = gApi.config().server().getInfo();
- assertThat(i.change.excludeMergeableInChangeInfo).isTrue();
- }
-
- @Test
- @GerritConfig(name = "index.change.indexMergeable", value = "true")
- public void indexMergeableIsTrueWhenTrueInConfig() throws Exception {
- ServerInfo i = gApi.config().server().getInfo();
- assertThat(i.index.change.indexMergeable).isTrue();
+ assertThat(i.change.mergeabilityComputationBehavior).isEqualTo("NEVER");
}
}
diff --git a/javatests/com/google/gerrit/acceptance/server/event/EventPayloadIT.java b/javatests/com/google/gerrit/acceptance/server/event/EventPayloadIT.java
index d973934..3bc863f 100644
--- a/javatests/com/google/gerrit/acceptance/server/event/EventPayloadIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/event/EventPayloadIT.java
@@ -32,32 +32,26 @@
@Test
public void defaultOptions() throws Exception {
RevisionCreatedListener listener =
- new RevisionCreatedListener() {
- @Override
- public void onRevisionCreated(Event event) {
- assertThat(event.getChange().submittable).isNotNull();
- assertThat(event.getRevision().files).isNotEmpty();
- }
+ event -> {
+ assertThat(event.getChange().submittable).isNotNull();
+ assertThat(event.getRevision().files).isNotEmpty();
};
- try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
+ try (Registration ignored = extensionRegistry.newRegistration().add(listener)) {
createChange();
}
}
@Test
- @GerritConfig(name = "event.payload.listChangeOptions", value = "SKIP_MERGEABLE")
+ @GerritConfig(name = "event.payload.listChangeOptions", value = "SKIP_DIFFSTAT")
public void configuredOptions() throws Exception {
RevisionCreatedListener listener =
- new RevisionCreatedListener() {
- @Override
- public void onRevisionCreated(Event event) {
- assertThat(event.getChange().submittable).isNull();
- assertThat(event.getChange().mergeable).isNull();
- assertThat(event.getRevision().files).isNull();
- assertThat(event.getChange().subject).isNotEmpty();
- }
+ event -> {
+ assertThat(event.getChange().submittable).isNull();
+ assertThat(event.getChange().insertions).isNull();
+ assertThat(event.getRevision().files).isNull();
+ assertThat(event.getChange().subject).isNotEmpty();
};
- try (Registration registration = extensionRegistry.newRegistration().add(listener)) {
+ try (Registration ignored = extensionRegistry.newRegistration().add(listener)) {
createChange();
}
}
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
index ccfe783..eb1f907 100644
--- a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
@@ -31,7 +31,6 @@
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.validators.CommentForValidation;
-import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.testing.TestCommentHelper;
@@ -49,9 +48,15 @@
@Inject private CommentValidator mockCommentValidator;
@Inject private TestCommentHelper testCommentHelper;
- private static final int MAX_COMMENT_LENGTH = 666;
+ private static final int COMMENT_SIZE_LIMIT = 666;
private static final String COMMENT_TEXT = "The comment text";
+ private static final CommentForValidation COMMENT_FOR_VALIDATION =
+ CommentForValidation.create(
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.FILE_COMMENT,
+ COMMENT_TEXT,
+ COMMENT_TEXT.length());
@Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> capture;
@Captor private ArgumentCaptor<CommentValidationContext> captureCtx;
@@ -82,13 +87,9 @@
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder()
- .changeId(result.getChange().getId().get())
- .project(result.getChange().project().get())
- .build(),
- ImmutableList.of(
- CommentForValidation.create(
- CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
+ CommentValidationContext.create(
+ result.getChange().getId().get(), result.getChange().project().get()),
+ ImmutableList.of(COMMENT_FOR_VALIDATION)))
.thenReturn(ImmutableList.of());
DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT);
testCommentHelper.addDraft(changeId, revId, comment);
@@ -101,20 +102,14 @@
@Test
public void validateComments_commentRejected() throws Exception {
- CommentForValidation commentForValidation =
- CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT);
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder()
- .changeId(result.getChange().getId().get())
- .project(result.getChange().project().get())
- .build(),
- ImmutableList.of(
- CommentForValidation.create(
- CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
- .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ CommentValidationContext.create(
+ result.getChange().getId().get(), result.getChange().project().get()),
+ ImmutableList.of(COMMENT_FOR_VALIDATION)))
+ .thenReturn(ImmutableList.of(COMMENT_FOR_VALIDATION.failValidation("Oh no!")));
DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT);
testCommentHelper.addDraft(changeId, revId, comment);
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
@@ -149,18 +144,24 @@
assertThat(capture.getAllValues().get(0))
.containsExactly(
CommentForValidation.create(
- CommentForValidation.CommentType.INLINE_COMMENT, draftInline.message),
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.INLINE_COMMENT,
+ draftInline.message,
+ draftInline.message.length()),
CommentForValidation.create(
- CommentForValidation.CommentType.FILE_COMMENT, draftFile.message));
+ CommentForValidation.CommentSource.HUMAN,
+ CommentForValidation.CommentType.FILE_COMMENT,
+ draftFile.message,
+ draftFile.message.length()));
}
@Test
- @GerritConfig(name = "change.maxCommentLength", value = "" + MAX_COMMENT_LENGTH)
+ @GerritConfig(name = "change.commentSizeLimit", value = "" + COMMENT_SIZE_LIMIT)
public void validateComments_enforceLimits_commentTooLarge() throws Exception {
when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
- int commentLength = MAX_COMMENT_LENGTH + 1;
+ int commentLength = COMMENT_SIZE_LIMIT + 1;
DraftInput comment =
testCommentHelper.newDraft(new String(new char[commentLength]).replace("\0", "x"));
testCommentHelper.addDraft(changeId, result.getCommit().getName(), comment);
@@ -169,7 +170,33 @@
Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
amendResult.assertOkStatus();
amendResult.assertMessage(
- String.format("Comment too large (%d > %d)", commentLength, MAX_COMMENT_LENGTH));
+ String.format("Comment size exceeds limit (%d > %d)", commentLength, COMMENT_SIZE_LIMIT));
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
}
+
+ @Test
+ @GerritConfig(name = "change.maxComments", value = "3")
+ public void countComments_limitNumberOfComments() throws Exception {
+ when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ String revId = result.getCommit().getName();
+ String filePath = result.getChange().currentFilePaths().get(0);
+ DraftInput draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, COMMENT_TEXT);
+ testCommentHelper.addDraft(changeId, revId, draftInline);
+ amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
+ assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1);
+
+ for (int i = 1; i < 3; ++i) {
+ testCommentHelper.addRobotComment(
+ changeId,
+ TestCommentHelper.createRobotCommentInput(result.getChange().currentFilePaths().get(0)));
+ }
+
+ draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, COMMENT_TEXT);
+ testCommentHelper.addDraft(changeId, revId, draftInline);
+ Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
+ assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1);
+ amendResult.assertMessage("exceeding maximum number of comments");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java
new file mode 100644
index 0000000..c32827a
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java
@@ -0,0 +1,67 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.server.git.receive;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.cache.Cache;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.server.patch.DiffSummary;
+import com.google.gerrit.server.patch.DiffSummaryKey;
+import com.google.inject.Inject;
+import com.google.inject.name.Named;
+import org.junit.Test;
+
+/** Tests for applying limits to e.g. number of files per change. */
+public class ReceiveCommitsLimitsIT extends AbstractDaemonTest {
+
+ @Inject
+ private @Named("diff_summary") Cache<DiffSummaryKey, DiffSummary> diffSummaryCache;
+
+ @Test
+ @GerritConfig(name = "change.maxFiles", value = "1")
+ public void limitFileCount() throws Exception {
+ PushOneCommit.Result result =
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ "foo",
+ ImmutableMap.of("foo", "foo-1.0", "bar", "bar-1.0"))
+ .to("refs/for/master");
+ result.assertErrorStatus("Exceeding maximum number of files per change (2 > 1)");
+ }
+
+ @Test
+ public void cacheKeyMatches() throws Exception {
+ int cacheSizeBefore = diffSummaryCache.asMap().size();
+ PushOneCommit.Result result =
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ "foo",
+ ImmutableMap.of("foo", "foo-1.0", "bar", "bar-1.0"))
+ .to("refs/for/master");
+ result.assertOkStatus();
+
+ // Assert that we only performed the diff computation once. This would e.g. catch
+ // bugs/deviations in the computation of the cache key.
+ assertThat(diffSummaryCache.asMap()).hasSize(cacheSizeBefore + 1);
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java b/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
index 768c269..3b0e27b 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
@@ -33,7 +33,9 @@
public class AbstractMailIT extends AbstractDaemonTest {
@Inject private RequestScopeOperations requestScopeOperations;
- protected MailMessage.Builder messageBuilderWithDefaultFields() {
+ static final String FILE_NAME = "gerrit-server/test.txt";
+
+ MailMessage.Builder messageBuilderWithDefaultFields() {
MailMessage.Builder b = MailMessage.builder();
b.id("some id");
b.from(user.getEmailAddress());
@@ -43,16 +45,15 @@
return b;
}
- protected String createChangeWithReview() throws Exception {
+ String createChangeWithReview() throws Exception {
return createChangeWithReview(admin);
}
- protected String createChangeWithReview(TestAccount reviewer) throws Exception {
+ String createChangeWithReview(TestAccount reviewer) throws Exception {
// Create change
- String file = "gerrit-server/test.txt";
String contents = "contents \nlorem \nipsum \nlorem";
PushOneCommit push =
- pushFactory.create(admin.newIdent(), testRepo, "first subject", file, contents);
+ pushFactory.create(admin.newIdent(), testRepo, "first subject", FILE_NAME, contents);
PushOneCommit.Result r = push.to("refs/for/master");
String changeId = r.getChangeId();
@@ -61,8 +62,8 @@
ReviewInput input = new ReviewInput();
input.message = "I have two comments";
input.comments = new HashMap<>();
- CommentInput c1 = newComment(file, Side.REVISION, 0, "comment on file");
- CommentInput c2 = newComment(file, Side.REVISION, 2, "inline comment");
+ CommentInput c1 = newComment(FILE_NAME, Side.REVISION, 0, "comment on file");
+ CommentInput c2 = newComment(FILE_NAME, Side.REVISION, 2, "inline comment");
input.comments.put(c1.path, ImmutableList.of(c1, c2));
revision(r).review(input);
return changeId;
@@ -94,7 +95,7 @@
* @param fc1 Comment in reply to a comment of file 1.
* @return A string with all inline comments and the original quoted email.
*/
- protected static String newPlaintextBody(
+ static String newPlaintextBody(
String changeURL, String changeMessage, String c1, String f1, String fc1) {
return (changeMessage == null ? "" : changeMessage + "\n")
+ "> Foo Bar has posted comments on this change. ( \n"
@@ -137,7 +138,7 @@
+ "> \n";
}
- protected static String textFooterForChange(int changeNumber, String timestamp) {
+ static String textFooterForChange(int changeNumber, String timestamp) {
return "Gerrit-Change-Number: "
+ changeNumber
+ "\n"
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
index 2409f52..502194a 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -21,9 +21,14 @@
import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.CommentInfo;
@@ -341,17 +346,53 @@
assertThat(message.body()).contains("rejected one or more comments");
}
+ @Test
+ @GerritConfig(name = "change.maxComments", value = "4")
+ public void limitNumberOfComments() throws Exception {
+ String changeId = createChangeWithReview();
+ CommentInput commentInput = new CommentInput();
+ commentInput.line = 1;
+ commentInput.message = "foo";
+ commentInput.path = FILE_NAME;
+ RobotCommentInput robotCommentInput =
+ TestCommentHelper.createRobotCommentInputWithMandatoryFields(FILE_NAME);
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.comments = ImmutableMap.of(FILE_NAME, ImmutableList.of(commentInput));
+ reviewInput.robotComments = ImmutableMap.of(FILE_NAME, ImmutableList.of(robotCommentInput));
+ gApi.changes().id(changeId).current().review(reviewInput);
+
+ ChangeInfo changeInfo = gApi.changes().id(changeId).get();
+ List<CommentInfo> comments = gApi.changes().id(changeId).current().commentsAsList();
+ String ts =
+ MailProcessingUtil.rfcDateformatter.format(
+ ZonedDateTime.ofInstant(comments.get(0).updated.toInstant(), ZoneId.of("UTC")));
+
+ MailMessage.Builder b = messageBuilderWithDefaultFields();
+ String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, null, COMMENT_TEXT, null);
+ b.textContent(txt + textFooterForChange(changeInfo._number, ts));
+
+ Collection<CommentInfo> commentsBefore = testCommentHelper.getPublishedComments(changeId);
+ mailProcessor.process(b.build());
+ assertThat(testCommentHelper.getPublishedComments(changeId)).isEqualTo(commentsBefore);
+
+ assertNotifyTo(user);
+ Message message = sender.nextMessage();
+ assertThat(message.body()).contains("rejected one or more comments");
+ }
+
private String getChangeUrl(ChangeInfo changeInfo) {
return canonicalWebUrl.get() + "c/" + changeInfo.project + "/+/" + changeInfo._number;
}
private void setupFailValidation(
CommentForValidation.CommentType type, String failProject, int failChange) {
- CommentForValidation commentForValidation = CommentForValidation.create(type, COMMENT_TEXT);
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(
+ CommentForValidation.CommentSource.HUMAN, type, COMMENT_TEXT, COMMENT_TEXT.length());
when(mockCommentValidator.validateComments(
- CommentValidationContext.builder().changeId(failChange).project(failProject).build(),
- ImmutableList.of(CommentForValidation.create(type, COMMENT_TEXT))))
+ CommentValidationContext.create(failChange, failProject),
+ ImmutableList.of(commentForValidation)))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
}
}
diff --git a/javatests/com/google/gerrit/pgm/BUILD b/javatests/com/google/gerrit/pgm/BUILD
index 0cae937..b68aacc 100644
--- a/javatests/com/google/gerrit/pgm/BUILD
+++ b/javatests/com/google/gerrit/pgm/BUILD
@@ -5,7 +5,7 @@
name = "pgm_tests",
srcs = glob(["**/*.java"]),
deps = [
- "//java/com/google/gerrit/pgm/http",
+ "//java/com/google/gerrit/pgm/http/jetty",
"//java/com/google/gerrit/pgm/init/api",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/securestore/testing",
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 9c12052..38e4ca4 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -2025,7 +2025,9 @@
}
@Test
- @GerritConfig(name = "index.change.indexMergeable", value = "true")
+ @GerritConfig(
+ name = "change.mergeabilityComputationBehavior",
+ value = "API_REF_UPDATED_AND_CHANGE_REINDEX")
public void mergeable() throws Exception {
TestRepository<Repo> repo = createProject("repo");
RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create());
@@ -2043,7 +2045,7 @@
// If a change gets submitted, the remaining open changes get reindexed asynchronously to update
// their mergeability information. If the further assertions in this test are done before the
// asynchronous reindex completed they fail because the mergeability information in the index
- // was not updated yet. To avoid this flakiness indexMergeable is switched off for the
+ // was not updated yet. To avoid this flakiness indexing mergeable is switched off for the
// tests and we index change2 synchronously here.
gApi.changes().id(change2.getChangeId()).index();
@@ -3083,7 +3085,7 @@
}
@Test
- @GerritConfig(name = "index.change.indexMergeable", value = "false")
+ @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER")
public void mergeableFailsWhenNotIndexed() throws Exception {
TestRepository<Repo> repo = createProject("repo");
RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create());
diff --git a/plugins/BUILD b/plugins/BUILD
index ccb97e8..8d9682f 100644
--- a/plugins/BUILD
+++ b/plugins/BUILD
@@ -93,7 +93,18 @@
]
java_binary(
+ name = "bouncycastle-deploy-env",
+ main_class = "Dummy",
+ runtime_deps = [
+ "//lib/bouncycastle:bcpg",
+ "//lib/bouncycastle:bcpkix",
+ "//lib/bouncycastle:bcprov",
+ ],
+)
+
+java_binary(
name = "plugin-api",
+ deploy_env = ["bouncycastle-deploy-env"],
main_class = "Dummy",
visibility = ["//visibility:public"],
runtime_deps = [":plugin-lib"],
diff --git a/plugins/delete-project b/plugins/delete-project
index 7885a9f..180fd9d 160000
--- a/plugins/delete-project
+++ b/plugins/delete-project
@@ -1 +1 @@
-Subproject commit 7885a9fa8d262b6127a7a3aa294108d6aac47a4d
+Subproject commit 180fd9dbd7f1661d16bf05ca8a16c74bfcc9bc67
diff --git a/plugins/replication b/plugins/replication
index 90ca234..0bf9f5a 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit 90ca234c456767b79608ad2254f3e80f02f308ca
+Subproject commit 0bf9f5ae26220bfdcff0b3332e810f00aa9a7789
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
index 283bd74..f4bd6a6 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
@@ -205,9 +205,6 @@
<gr-confirm-revert-dialog id="confirmRevertDialog"
class="confirmDialog"
on-confirm="_handleRevertDialogConfirm"
- commit-message="[[commitMessage]]"
- change="[[change]]"
- changes="[[_revertChanges]]"
on-cancel="_handleConfirmDialogCancel"
hidden></gr-confirm-revert-dialog>
<gr-confirm-revert-submission-dialog id="confirmRevertSubmissionDialog"
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
index c0a603e..299c12a 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
@@ -923,9 +923,13 @@
showRevertDialog() {
const query = 'submissionid:' + this.change.submission_id;
+ /* A chromium plugin expects that the modifyRevertMsg hook will only
+ be called after the revert button is pressed, hence we populate the
+ revert dialog after revert button is pressed. */
this.$.restAPI.getChanges('', query)
.then(changes => {
- this._revertChanges = changes;
+ this.$.confirmRevertDialog.populate(this.change,
+ this.commitMessage, changes);
this._showActionDialog(this.$.confirmRevertDialog);
});
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
index 6495adb..088bebd 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
@@ -820,6 +820,11 @@
element.change = {
current_revision: 'abc1234',
};
+ sandbox.stub(element.$.restAPI, 'getChanges')
+ .returns(Promise.resolve([
+ {change_id: '12345678901234', topic: 'T', subject: 'random'},
+ {change_id: '23456', topic: 'T', subject: 'a'.repeat(100)},
+ ]));
sandbox.stub(element.$.confirmRevertDialog,
'_populateRevertSubmissionMessage', () => 'original msg');
flush(() => {
@@ -970,7 +975,7 @@
});
});
- test('confirm revert dialog shows one radio button', done => {
+ test('confirm revert dialog shows no radio button', done => {
const revertButton = element.shadowRoot
.querySelector('gr-button[data-action-key="revert"]');
MockInteractions.tap(revertButton);
@@ -978,7 +983,7 @@
const confirmRevertDialog = element.$.confirmRevertDialog;
const radioInputs = confirmRevertDialog.shadowRoot
.querySelectorAll('input[name="revertOptions"]');
- assert.equal(radioInputs.length, 1);
+ assert.equal(radioInputs.length, 0);
const msg = 'Revert "random commit message"\n\n'
+ 'This reverts commit 2000.\n\nReason '
+ 'for revert: <INSERT REASONING HERE>\n';
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 02b017b..623e8d1 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -613,7 +613,8 @@
<span>Comment Threads</span></gr-tooltip-content>
</paper-tab>
</paper-tabs>
- <template is="dom-if" if="[[_showMessagesView]]">
+ <template is="dom-if" if="[[_isSelectedView(_currentView,
+ _commentTabs.CHANGE_LOG)]]">
<gr-messages-list
class="hideOnMobileOverlay"
change-num="[[_changeNum]]"
@@ -626,7 +627,8 @@
on-message-anchor-tap="_handleMessageAnchorTap"
on-reply="_handleMessageReply"></gr-messages-list>
</template>
- <template is="dom-if" if="[[!_showMessagesView]]">
+ <template is="dom-if" if="[[_isSelectedView(_currentView,
+ _commentTabs.COMMENT_THREADS)]]">
<gr-thread-list
threads="[[_commentThreads]]"
change="[[_change]]"
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 7536c2c..6a04445 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
@@ -59,6 +59,11 @@
UNIFIED: 'UNIFIED_DIFF',
};
+ const CommentTabs = {
+ CHANGE_LOG: 0,
+ COMMENT_THREADS: 1,
+ };
+
const CHANGE_DATA_TIMING_LABEL = 'ChangeDataLoaded';
const CHANGE_RELOAD_TIMING_LABEL = 'ChangeReloaded';
const SEND_REPLY_TIMING_LABEL = 'SendReply';
@@ -193,6 +198,10 @@
type: String,
value: '',
},
+ _commentTabs: {
+ type: Object,
+ value: CommentTabs,
+ },
_lineHeight: Number,
_changeIdCommitMessageError: {
type: String,
@@ -280,9 +289,9 @@
type: Boolean,
value: undefined,
},
- _showMessagesView: {
- type: Boolean,
- value: true,
+ _currentView: {
+ type: Number,
+ value: CommentTabs.CHANGE_LOG,
},
_showFileTabContent: {
type: Boolean,
@@ -460,7 +469,11 @@
}
_handleCommentTabChange() {
- this._showMessagesView = this.$.commentTabs.selected === 0;
+ this._currentView = this.$.commentTabs.selected;
+ }
+
+ _isSelectedView(currentView, view) {
+ return currentView === view;
}
_handleFileTabChange(e) {
@@ -773,8 +786,7 @@
// get messed up if changed here, because it requires the tabs to be on
// the streen, and they are hidden shortly after this. The tab switching
// animation will happen in post render tasks.
- this._showMessagesView = true;
-
+ this._currentView = CommentTabs.CHANGE_LOG;
if (value.view !== Gerrit.Nav.View.CHANGE) {
this._initialLoadComplete = false;
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 1ef79cf..5d38c2d 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
@@ -63,6 +63,11 @@
let navigateToChangeStub;
const TEST_SCROLL_TOP_PX = 100;
+ const CommentTabs = {
+ CHANGE_LOG: 0,
+ COMMENT_THREADS: 1,
+ };
+
setup(() => {
sandbox = sinon.sandbox.create();
stub('gr-endpoint-decorator', {
@@ -375,7 +380,7 @@
test('thread list modified', () => {
sandbox.spy(element, '_handleReloadDiffComments');
- element._showMessagesView = false;
+ element._currentView = CommentTabs.COMMENT_THREADS;
flushAsynchronousOperations();
return element._reloadComments().then(() => {
@@ -442,18 +447,18 @@
// Wait for tab to get selected
flush(() => {
assert.equal(element.$.commentTabs.selected, 0);
- assert.isTrue(element._showMessagesView);
+ assert.equal(element._currentView, CommentTabs.CHANGE_LOG);
// Switch to comment thread tab
MockInteractions.tap(element.$$('paper-tab.commentThreads'));
assert.equal(element.$.commentTabs.selected, 1);
- assert.isFalse(element._showMessagesView);
+ assert.equal(element._currentView, CommentTabs.COMMENT_THREADS);
// When the change is partially reloaded (ex: Shift+R), the content
// is swapped out before the tab, so messages list will display even
// though the tab for comment threads is still temporarily selected.
element._paramsChanged(element.params);
assert.equal(element.$.commentTabs.selected, 1);
- assert.isTrue(element._showMessagesView);
+ assert.equal(element._currentView, CommentTabs.CHANGE_LOG);
done();
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html
index 0ad3ed3..144cf20 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html
+++ b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.html
@@ -67,56 +67,40 @@
<div class="error" hidden$="[[!_showErrorMessage]]">
<span> A reason is required </span>
</div>
- <div class="revertSubmissionLayout">
- <input
- name="revertOptions"
- type="radio"
- id="revertSingleChange"
- on-change="_handleRevertSingleChangeClicked"
- checked="[[_computeIfSingleRevert(_revertType)]]">
- <label for="revertSingleChange" class="label revertSingleChange">
- Revert single change
- </label>
- </div>
<template is="dom-if" if="[[_showRevertSubmission]]">
<div class="revertSubmissionLayout">
<input
name="revertOptions"
type="radio"
+ id="revertSingleChange"
+ on-change="_handleRevertSingleChangeClicked"
+ checked="[[_computeIfSingleRevert(_revertType)]]">
+ <label for="revertSingleChange" class="label revertSingleChange">
+ Revert single change
+ </label>
+ </div>
+ <div class="revertSubmissionLayout">
+ <input
+ name="revertOptions"
+ type="radio"
id="revertSubmission"
on-change="_handleRevertSubmissionClicked"
checked="[[_computeIfRevertSubmission(_revertType)]]">
<label for="revertSubmission" class="label revertSubmission">
- Revert entire submission ([[changes.length]] Changes)
+ Revert entire submission ([[_changesCount]] Changes)
</label>
</template>
<gr-endpoint-decorator name="confirm-revert-change">
- <!-- Duplicating the text-area as a plugin in the case of a single
- revert will override the entire textarea which should not happen
- for multiple revert -->
- <template is="dom-if" if="[[_computeIfSingleRevert(_revertType)]]">
- <label for="messageInput">
- Revert Commit Message
- </label>
- <iron-autogrow-textarea
- id="messageInput"
- class="message"
- autocomplete="on"
- max-rows="15"
- bind-value="{{_message}}"></iron-autogrow-textarea>
- </template>
- </gr-endpoint-decorator>
- <template is="dom-if" if="[[_computeIfRevertSubmission(_revertType)]]">
<label for="messageInput">
Revert Commit Message
</label>
<iron-autogrow-textarea
- id="messageInput"
- class="message"
- autocomplete="on"
- max-rows="15"
- bind-value="{{_message}}"></iron-autogrow-textarea>
- </template>
+ id="messageInput"
+ class="message"
+ autocomplete="on"
+ max-rows="15"
+ bind-value="{{_message}}"></iron-autogrow-textarea>
+ </gr-endpoint-decorator>
</div>
</gr-dialog>
<gr-js-api-interface id="jsAPI"></gr-js-api-interface>
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
index b07738a..05660bf 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js
@@ -62,22 +62,14 @@
type: Boolean,
value: false,
},
- /* List of changes with the same topic
- value is empty if only a single change is being reverted */
- changes: {
- type: Array,
- value() { return []; },
- },
- change: Object,
- /* commit message is _latestCommitMessage coming from gr-change-view
- read only and is not meant to be modified */
- commitMessage: String,
+ _changesCount: Number,
_showErrorMessage: {
type: Boolean,
value: false,
},
/* store the default revert messages per revert type so that we can
- check if user has edited the revert message or not */
+ check if user has edited the revert message or not
+ Set when populate() is called */
_originalRevertMessages: {
type: Array,
value() { return []; },
@@ -90,12 +82,6 @@
};
}
- static get observers() {
- return [
- 'onInputUpdate(change, commitMessage, changes)',
- ];
- }
-
_computeIfSingleRevert(revertType) {
return revertType === REVERT_TYPES.REVERT_SINGLE_CHANGE;
}
@@ -109,12 +95,12 @@
message, commitMessage);
}
- onInputUpdate(change, commitMessage, changes) {
- if (!change || !changes) return;
+ populate(change, commitMessage, changes) {
+ this._changesCount = changes.length;
// The option to revert a single change is always available
this._populateRevertSingleChangeMessage(
change, commitMessage, change.current_revision);
- this._populateRevertSubmissionMessage(change, changes);
+ this._populateRevertSubmissionMessage(change, changes, commitMessage);
}
_populateRevertSingleChangeMessage(change, commitMessage, commitHash) {
@@ -144,12 +130,12 @@
return subject.substring(0, CHANGE_SUBJECT_LIMIT) + '...';
}
- _modifyRevertSubmissionMsg(change, msg) {
+ _modifyRevertSubmissionMsg(change, msg, commitMessage) {
return this.$.jsAPI.modifyRevertSubmissionMsg(change, msg,
- this.commitMessage);
+ commitMessage);
}
- _populateRevertSubmissionMessage(change, changes) {
+ _populateRevertSubmissionMessage(change, changes, commitMessage) {
// Follow the same convention of the revert
const commitHash = change.current_revision;
if (!commitHash) {
@@ -166,7 +152,8 @@
this._message += change.change_id.substring(0, 10) + ':'
+ this._getTrimmedChangeSubject(change.subject) + '\n';
});
- this._message = this._modifyRevertSubmissionMsg(change, this._message);
+ this._message = this._modifyRevertSubmissionMsg(change, this._message,
+ commitMessage);
this._revertType = REVERT_TYPES.REVERT_SUBMISSION;
this._revertMessages[this._revertType] = this._message;
this._originalRevertMessages[this._revertType] = this._message;
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 c177283..d54669f 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
@@ -86,19 +86,17 @@
}
.peopleList {
display: flex;
- padding-top: var(--spacing-xxs);
}
.peopleListLabel {
color: var(--deemphasized-text-color);
margin-top: var(--spacing-xs);
- min-width: 7em;
+ min-width: 6em;
padding-right: var(--spacing-m);
}
gr-account-list {
display: flex;
flex-wrap: wrap;
flex: 1;
- min-height: 1.8em;
}
#reviewerConfirmationOverlay {
padding: var(--spacing-l);
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.html b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.html
index a5875ab1..132ce11 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.html
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.html
@@ -32,7 +32,15 @@
opacity: .8;
pointer-events: none;
}
- .container > :not(:first-child) {
+ .container {
+ display: block;
+ /* This is a bit of a hack. We tried to use margin-top with
+ :not(:first-child) before, but :first-child does not understand
+ whether a child is visible or not. So adding a margin for every
+ child and then a negative one at the top does the trick. */
+ margin-top: calc(0px - var(--spacing-s));
+ }
+ .container > * {
margin-top: var(--spacing-s);
}
gr-button {
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
index 72f5d5b..76d12e9 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html
@@ -321,12 +321,13 @@
element.fire('show-alert', {
message: 'test-alert', action: 'reload',
});
- toast = toastSpy.lastCall.returnValue;
- assert.isOk(toast);
- assert.include(
- Polymer.dom(toast.root).textContent, 'Credentails expired.');
-
- done();
+ flush(() => {
+ toast = toastSpy.lastCall.returnValue;
+ assert.isOk(toast);
+ assert.include(
+ Polymer.dom(toast.root).textContent, 'Credentails expired.');
+ done();
+ });
});
});
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
index 05a7525..726ac10 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
@@ -237,6 +237,11 @@
this.moveToNextChunk(true);
}
+ moveToLastChunk() {
+ this.$.cursorManager.moveToEnd();
+ this.moveToPreviousChunk();
+ }
+
reInitCursor() {
this._updateStops();
if (this.initialLineNumber) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
index dff9e79..24ca3d1 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
@@ -108,6 +108,18 @@
assert.equal(cursorElement.diffRow, firstDeltaRow);
});
+ test('moveToLastChunk', () => {
+ const chunks = Array.from(Polymer.dom(diffElement.root).querySelectorAll(
+ '.section.delta'));
+ assert.isAbove(chunks.length, 1);
+ assert.equal(chunks.indexOf(cursorElement.diffRow.parentElement), 0);
+
+ cursorElement.moveToLastChunk();
+
+ assert.equal(chunks.indexOf(cursorElement.diffRow.parentElement),
+ chunks.length - 1);
+ });
+
test('cursor scroll behavior', () => {
cursorElement._handleDiffRenderStart();
assert.equal(cursorElement._scrollBehavior, 'keep-visible');
diff --git a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.html b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.html
index 7ed7962..4bf1f1b 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.html
+++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.html
@@ -33,10 +33,9 @@
content: var(--account-label-suffix);
}
gr-avatar {
- height: 1.3em;
- width: 1.3em;
- margin-right: var(--spacing-xs);
- vertical-align: -.25em;
+ height: var(--line-height-normal);
+ width: var(--line-height-normal);
+ vertical-align: top;
}
.text {
@apply --gr-account-label-text-style;
diff --git a/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.html b/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.html
index 6103d6f..37591d8 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.html
+++ b/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.html
@@ -32,6 +32,7 @@
display: flex;
flex: 1;
min-width: 10em;
+ margin: var(--spacing-xs) var(--spacing-xs) var(--spacing-xs) 0;
}
.group {
--account-label-suffix: ' (group)';
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 fd75c7c..7cac2d8 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
@@ -21,6 +21,7 @@
<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
<link rel="import" href="/bower_components/paper-button/paper-button.html">
<link rel="import" href="../../../styles/shared-styles.html">
+<link rel="import" href="../../core/gr-reporting/gr-reporting.html">
<dom-module id="gr-button">
<template strip-whitespace>
@@ -160,6 +161,7 @@
<slot></slot>
<i class="downArrow"></i>
</paper-button>
+ <gr-reporting id="reporting"></gr-reporting>
</template>
<script src="gr-button.js"></script>
</dom-module>
diff --git a/polygerrit-ui/app/elements/shared/gr-button/gr-button.js b/polygerrit-ui/app/elements/shared/gr-button/gr-button.js
index abfbb18..681717f 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button.js
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button.js
@@ -90,6 +90,19 @@
e.preventDefault();
e.stopImmediatePropagation();
}
+ let el = this.root;
+ let path = '';
+ while (el = el.parentNode || el.host) {
+ if (el.tagName && el.tagName.startsWith('GR-APP')) {
+ break;
+ }
+ if (el.tagName) {
+ const idString = el.id ? '#' + el.id : '';
+ path = el.tagName + idString + ' ' + path;
+ }
+ }
+ this.$.reporting.reportInteraction('button-click',
+ path.trim().toLowerCase());
}
_disabledChanged(disabled) {
diff --git a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js
index 374204b..fd70fd9 100644
--- a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js
+++ b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js
@@ -163,6 +163,12 @@
}
}
+ moveToEnd() {
+ if (this.stops.length) {
+ this.setCursor(this.stops[this.stops.length - 1]);
+ }
+ }
+
setCursorAtIndex(index, opt_noScroll) {
this.setCursor(this.stops[index], opt_noScroll);
}
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.html b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.html
index ee9e7a0..f7349eb 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.html
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.html
@@ -59,16 +59,19 @@
cursor: pointer;
flex-direction: column;
font-size: inherit;
+ // This variable was introduced in Dec 2019. We keep both min-height
+ // rules around, because --paper-item-min-height is not yet upstreamed.
+ --paper-item-min-height: 0;
--paper-item: {
min-height: 0;
padding: 10px 16px;
- }
+ };
--paper-item-focused-before: {
background-color: var(--selection-background-color);
- }
+ };
--paper-item-focused: {
background-color: var(--selection-background-color);
- }
+ };
}
paper-item:hover {
background-color: var(--hover-background-color);
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context_test.html b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context_test.html
index f7b7cf8..ca43fc0 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context_test.html
@@ -88,13 +88,17 @@
assert.equal(div.textContent, 'foobar');
});
- test('button', () => {
+ test('button', done => {
const clickStub = sandbox.stub();
const button = instance.button('foo', {onclick: clickStub});
+ // If you don't attach a Polymer element to the DOM, then the ready()
+ // callback will not be called and then e.g. this.$ is undefined.
+ Polymer.dom(document.body).appendChild(button);
MockInteractions.tap(button);
flush(() => {
assert.isTrue(clickStub.called);
assert.equal(button.textContent, 'foo');
+ done();
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 472a7ea..4aefe46 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1001,7 +1001,6 @@
this.ListChangesOption.MESSAGES,
this.ListChangesOption.SUBMITTABLE,
this.ListChangesOption.WEB_LINKS,
- this.ListChangesOption.SKIP_MERGEABLE,
this.ListChangesOption.SKIP_DIFFSTAT,
];
return this.getConfig(false).then(config => {
@@ -1024,7 +1023,6 @@
const optionsHex = this.listChangesOptionsToHex(
this.ListChangesOption.ALL_COMMITS,
this.ListChangesOption.ALL_REVISIONS,
- this.ListChangesOption.SKIP_MERGEABLE,
this.ListChangesOption.SKIP_DIFFSTAT
);
return this._getChangeDetail(changeNum, optionsHex, opt_errFn,
diff --git a/polygerrit-ui/app/styles/themes/app-theme.html b/polygerrit-ui/app/styles/themes/app-theme.html
index 8ad2576..fc7478b 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.html
+++ b/polygerrit-ui/app/styles/themes/app-theme.html
@@ -68,7 +68,7 @@
/* comment background colors */
--comment-background-color: #e8eaed;
--robot-comment-background-color: #e8f0fe;
- --unresolved-comment-background-color: #fef7f0;
+ --unresolved-comment-background-color: #fef7e0;
/* vote background colors */
--vote-color-approved: #9fcc6b;
--vote-color-disliked: #f7c4cb;
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
index e88c424..16c5c8d 100644
--- a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
+++ b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
@@ -64,6 +64,7 @@
{/template}
{template .InboundEmailRejection_COMMENT_REJECTED kind="text"}
- Gerrit Code Review rejected one or more comments because they did not pass validation.
+ Gerrit Code Review rejected one or more comments because they did not pass validation, or
+ because the maximum number of comments per change would be exceeded.
{call .InboundEmailRejectionFooter /}
{/template}
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
index e17508d..8762e10 100644
--- a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
+++ b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
@@ -81,7 +81,8 @@
{template .InboundEmailRejectionHtml_COMMENT_REJECTED}
<p>
- Gerrit Code Review rejected one or more comments because they did not pass validation.
+ Gerrit Code Review rejected one or more comments because they did not pass validation, or
+ because the maximum number of comments per change would be exceeded.
</p>
{call .InboundEmailRejectionFooterHtml /}
{/template}
diff --git a/tools/BUILD b/tools/BUILD
index 32599aa..2d702fe 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -86,6 +86,7 @@
"-Xep:TypeParameterShadowing:ERROR",
"-Xep:TypeParameterUnusedInFormals:ERROR",
"-Xep:URLEqualsHashCode:ERROR",
+ "-Xep:UnusedException:ERROR",
"-Xep:UnsynchronizedOverridesSynchronized:ERROR",
"-Xep:WaitNotInLoop:ERROR",
"-Xep:WildcardImport:ERROR",
diff --git a/tools/bzl/java.bzl b/tools/bzl/java.bzl
deleted file mode 100644
index 8996b69..0000000
--- a/tools/bzl/java.bzl
+++ /dev/null
@@ -1,28 +0,0 @@
-# Copyright (C) 2016 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.
-
-# Syntactic sugar for native java_library() rule:
-# accept exported_deps attributes
-
-load("@rules_java//java:defs.bzl", "java_library")
-
-def java_library2(deps = [], exported_deps = [], exports = [], **kwargs):
- if exported_deps:
- deps = deps + exported_deps
- exports = exports + exported_deps
- java_library(
- deps = deps,
- exports = exports,
- **kwargs
- )
diff --git a/tools/bzl/plugin.bzl b/tools/bzl/plugin.bzl
index ed64d1b..64a6e22 100644
--- a/tools/bzl/plugin.bzl
+++ b/tools/bzl/plugin.bzl
@@ -21,6 +21,7 @@
manifest_entries = [],
dir_name = None,
target_suffix = "",
+ deploy_env = [],
**kwargs):
java_library(
name = name + "__plugin",
@@ -43,6 +44,7 @@
runtime_deps = [
":%s__plugin" % name,
] + static_jars,
+ deploy_env = deploy_env,
visibility = ["//visibility:public"],
**kwargs
)
diff --git a/tools/coverage.sh b/tools/coverage.sh
index 11e50e6..c92d5cf 100755
--- a/tools/coverage.sh
+++ b/tools/coverage.sh
@@ -33,7 +33,7 @@
cp -r {java,javatests}/* ${destdir}/java
mkdir -p ${destdir}/plugins
-for plugin in `find plugins/ -type d` -maxdepth 1
+for plugin in `find plugins/ -maxdepth 1 -type d`
do
mkdir -p ${destdir}/${plugin}/java
cp -r plugins/*/{java,javatests}/* ${destdir}/${plugin}/java
diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl
index acf01fa..1210504 100644
--- a/tools/nongoogle.bzl
+++ b/tools/nongoogle.bzl
@@ -126,18 +126,18 @@
sha1 = "dc13ae4faca6df981fc7aeb5a522d9db446d5d50",
)
- TESTCONTAINERS_VERSION = "1.12.4"
+ TESTCONTAINERS_VERSION = "1.12.5"
maven_jar(
name = "testcontainers",
artifact = "org.testcontainers:testcontainers:" + TESTCONTAINERS_VERSION,
- sha1 = "456b6facac12c4b67130d9056a43c011679e9f0c",
+ sha1 = "ca8a8dfbd3b194fb6c541e3bad858898865ce069",
)
maven_jar(
name = "testcontainers-elasticsearch",
artifact = "org.testcontainers:elasticsearch:" + TESTCONTAINERS_VERSION,
- sha1 = "9e210c277a35a95a76d03a79e2812575bd07391c",
+ sha1 = "d88f79c83e7057edd23d33621c36d6267b9c4e96",
)
maven_jar(
diff --git a/tools/remote-bazelrc b/tools/remote-bazelrc
index b26a8e8..826be1d 100644
--- a/tools/remote-bazelrc
+++ b/tools/remote-bazelrc
@@ -70,9 +70,6 @@
# Enable remote execution so actions are performed on the remote systems.
build:remote --remote_executor=remotebuildexecution.googleapis.com
-# Enable encryption.
-build:remote --tls_enabled=true
-
# Set a higher timeout value, just in case.
build:remote --remote_timeout=3600
