Merge "Rename gr-confirm-dialog to gr-dialog"
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index 3db621d..1aa6cd7c 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -238,7 +238,9 @@
The `PatchSetLock` function provides a locking mechanism for patch
sets. This function's values are not considered when determining
whether a change is submittable. When set, no new patchsets can be
-created and rebase and abandon are blocked.
+created and rebase and abandon are blocked. This is useful to prevent
+updates to a change while (potentially expensive) CI
+validation is running.
+
This function is designed to allow overlapping locks, so several lock
accounts could lock the same change.
diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index b318199..27ed72f 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -38,6 +38,7 @@
import com.google.gwtorm.server.ResultSet;
import java.util.ArrayList;
import java.util.List;
+import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
@@ -199,58 +200,67 @@
return disabledResults(queryStrings, queries);
}
- // Parse and rewrite all queries.
- List<Integer> limits = new ArrayList<>(cnt);
- List<Predicate<T>> predicates = new ArrayList<>(cnt);
- List<DataSource<T>> sources = new ArrayList<>(cnt);
- for (Predicate<T> q : queries) {
- int limit = getEffectiveLimit(q);
- limits.add(limit);
+ List<QueryResult<T>> out;
+ try {
+ // Parse and rewrite all queries.
+ List<Integer> limits = new ArrayList<>(cnt);
+ List<Predicate<T>> predicates = new ArrayList<>(cnt);
+ List<DataSource<T>> sources = new ArrayList<>(cnt);
+ for (Predicate<T> q : queries) {
+ int limit = getEffectiveLimit(q);
+ limits.add(limit);
- if (limit == getBackendSupportedLimit()) {
- limit--;
+ if (limit == getBackendSupportedLimit()) {
+ limit--;
+ }
+
+ int page = (start / limit) + 1;
+ if (page > indexConfig.maxPages()) {
+ throw new QueryParseException(
+ "Cannot go beyond page " + indexConfig.maxPages() + " of results");
+ }
+
+ // Always bump limit by 1, even if this results in exceeding the permitted
+ // max for this user. The only way to see if there are more entities is to
+ // ask for one more result from the query.
+ QueryOptions opts = createOptions(indexConfig, start, limit + 1, getRequestedFields());
+ Predicate<T> pred = rewriter.rewrite(q, opts);
+ if (enforceVisibility) {
+ pred = enforceVisibility(pred);
+ }
+ predicates.add(pred);
+
+ @SuppressWarnings("unchecked")
+ DataSource<T> s = (DataSource<T>) pred;
+ sources.add(s);
}
- int page = (start / limit) + 1;
- if (page > indexConfig.maxPages()) {
- throw new QueryParseException(
- "Cannot go beyond page " + indexConfig.maxPages() + " of results");
+ // Run each query asynchronously, if supported.
+ List<ResultSet<T>> matches = new ArrayList<>(cnt);
+ for (DataSource<T> s : sources) {
+ matches.add(s.read());
}
- // Always bump limit by 1, even if this results in exceeding the permitted
- // max for this user. The only way to see if there are more entities is to
- // ask for one more result from the query.
- QueryOptions opts = createOptions(indexConfig, start, limit + 1, getRequestedFields());
- Predicate<T> pred = rewriter.rewrite(q, opts);
- if (enforceVisibility) {
- pred = enforceVisibility(pred);
+ out = new ArrayList<>(cnt);
+ for (int i = 0; i < cnt; i++) {
+ out.add(
+ QueryResult.create(
+ queryStrings != null ? queryStrings.get(i) : null,
+ predicates.get(i),
+ limits.get(i),
+ matches.get(i).toList()));
}
- predicates.add(pred);
- @SuppressWarnings("unchecked")
- DataSource<T> s = (DataSource<T>) pred;
- sources.add(s);
+ // Only measure successful queries that actually touched the index.
+ metrics.executionTime.record(
+ schemaDef.getName(), System.nanoTime() - startNanos, TimeUnit.NANOSECONDS);
+ } catch (OrmException | OrmRuntimeException e) {
+ Optional<QueryParseException> qpe = findQueryParseException(e);
+ if (qpe.isPresent()) {
+ throw new QueryParseException(qpe.get().getMessage(), e);
+ }
+ throw e;
}
-
- // Run each query asynchronously, if supported.
- List<ResultSet<T>> matches = new ArrayList<>(cnt);
- for (DataSource<T> s : sources) {
- matches.add(s.read());
- }
-
- List<QueryResult<T>> out = new ArrayList<>(cnt);
- for (int i = 0; i < cnt; i++) {
- out.add(
- QueryResult.create(
- queryStrings != null ? queryStrings.get(i) : null,
- predicates.get(i),
- limits.get(i),
- matches.get(i).toList()));
- }
-
- // Only measure successful queries that actually touched the index.
- metrics.executionTime.record(
- schemaDef.getName(), System.nanoTime() - startNanos, TimeUnit.NANOSECONDS);
return out;
}
@@ -332,4 +342,12 @@
checkState(result > 0, "effective limit should be positive");
return result;
}
+
+ private static Optional<QueryParseException> findQueryParseException(Throwable t) {
+ return Throwables.getCausalChain(t)
+ .stream()
+ .filter(c -> c instanceof QueryParseException)
+ .map(QueryParseException.class::cast)
+ .findFirst();
+ }
}
diff --git a/java/com/google/gerrit/proto/BUILD b/java/com/google/gerrit/proto/BUILD
new file mode 100644
index 0000000..48185d6
--- /dev/null
+++ b/java/com/google/gerrit/proto/BUILD
@@ -0,0 +1,14 @@
+java_binary(
+ name = "ProtoGen",
+ srcs = ["ProtoGen.java"],
+ resource_strip_prefix = "resources",
+ resources = ["//resources/com/google/gerrit/proto"],
+ visibility = ["//proto:__pkg__"],
+ deps = [
+ "//java/com/google/gerrit/reviewdb:server",
+ "//lib:args4j",
+ "//lib:guava",
+ "//lib:gwtorm",
+ "//lib/jgit/org.eclipse.jgit:jgit",
+ ],
+)
diff --git a/java/com/google/gerrit/pgm/ProtoGen.java b/java/com/google/gerrit/proto/ProtoGen.java
similarity index 67%
rename from java/com/google/gerrit/pgm/ProtoGen.java
rename to java/com/google/gerrit/proto/ProtoGen.java
index a882412..1c55a05 100644
--- a/java/com/google/gerrit/pgm/ProtoGen.java
+++ b/java/com/google/gerrit/proto/ProtoGen.java
@@ -12,11 +12,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package com.google.gerrit.pgm;
+package com.google.gerrit.proto;
+import static com.google.common.base.Preconditions.checkState;
import static java.nio.charset.StandardCharsets.UTF_8;
-import com.google.gerrit.pgm.util.AbstractProgram;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gwtorm.schema.java.JavaSchemaModel;
import java.io.BufferedWriter;
@@ -28,23 +28,37 @@
import java.nio.ByteBuffer;
import org.eclipse.jgit.internal.storage.file.LockFile;
import org.eclipse.jgit.util.IO;
+import org.kohsuke.args4j.CmdLineException;
+import org.kohsuke.args4j.CmdLineParser;
import org.kohsuke.args4j.Option;
-public class ProtoGen extends AbstractProgram {
+public class ProtoGen {
@Option(
- name = "--output",
- aliases = {"-o"},
- required = true,
- metaVar = "FILE",
- usage = "File to write .proto into")
+ name = "--output",
+ aliases = {"-o"},
+ required = true,
+ metaVar = "FILE",
+ usage = "File to write .proto into"
+ )
private File file;
- @Override
- public int run() throws Exception {
- LockFile lock = new LockFile(file.getAbsoluteFile());
- if (!lock.lock()) {
- throw die("Cannot lock " + file);
+ public static void main(String[] argv) throws Exception {
+ System.exit(new ProtoGen().run(argv));
+ }
+
+ private int run(String[] argv) throws Exception {
+ CmdLineParser parser = new CmdLineParser(this);
+ try {
+ parser.parseArgument(argv);
+ } catch (CmdLineException e) {
+ System.err.println(e.getMessage());
+ System.err.println(getClass().getSimpleName() + " -o output.proto");
+ parser.printUsage(System.err);
+ return 1;
}
+
+ LockFile lock = new LockFile(file.getAbsoluteFile());
+ checkState(lock.lock(), "cannot lock %s", file);
try {
JavaSchemaModel jsm = new JavaSchemaModel(ReviewDb.class);
try (OutputStream o = lock.getOutputStream();
@@ -61,9 +75,7 @@
jsm.generateProto(out);
out.flush();
}
- if (!lock.commit()) {
- throw die("Could not write to " + file);
- }
+ checkState(lock.commit(), "Could not write to %s", file);
} finally {
lock.unlock();
}
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 6879ec6..3c5585c 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -38,6 +38,7 @@
import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_MISSING_OBJECT;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_REASON;
+import com.google.auto.value.AutoValue;
import com.google.common.base.Function;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
@@ -370,7 +371,15 @@
private final ListMultimap<ReceiveError, String> errors;
private final ListMultimap<String, String> pushOptions;
private final Map<Change.Id, ReplaceRequest> replaceByChange;
- private final Set<ObjectId> validCommits;
+
+ @AutoValue
+ protected abstract static class ValidCommitKey {
+ abstract ObjectId getObjectId();
+
+ abstract Branch.NameKey getBranch();
+ }
+
+ private final Set<ValidCommitKey> validCommits;
/**
* Actual commands to be executed, as opposed to the mix of actual and magic commands that were
@@ -1893,13 +1902,7 @@
logDebug("Creating new change for %s even though it is already tracked", name);
}
- if (!validCommit(
- receivePack.getRevWalk(),
- magicBranch.perm,
- magicBranch.dest,
- magicBranch.cmd,
- c,
- null)) {
+ if (!validCommit(receivePack.getRevWalk(), magicBranch.dest, magicBranch.cmd, c, null)) {
// Not a change the user can propose? Abort as early as possible.
logDebug("Aborting early due to invalid commit");
return Collections.emptyList();
@@ -2486,9 +2489,8 @@
}
}
- PermissionBackend.ForRef perm = permissions.ref(change.getDest().get());
if (!validCommit(
- receivePack.getRevWalk(), perm, change.getDest(), inputCommand, newCommit, change)) {
+ receivePack.getRevWalk(), change.getDest(), inputCommand, newCommit, change)) {
return false;
}
receivePack.getRevWalk().parseBody(priorCommit);
@@ -2860,7 +2862,7 @@
}
if (existing.keySet().contains(c)) {
continue;
- } else if (!validCommit(walk, perm, branch, cmd, c, null)) {
+ } else if (!validCommit(walk, branch, cmd, c, null)) {
break;
}
@@ -2878,15 +2880,12 @@
}
private boolean validCommit(
- RevWalk rw,
- PermissionBackend.ForRef perm,
- Branch.NameKey branch,
- ReceiveCommand cmd,
- ObjectId id,
- @Nullable Change change)
+ RevWalk rw, Branch.NameKey branch, ReceiveCommand cmd, ObjectId id, @Nullable Change change)
throws IOException {
+ PermissionBackend.ForRef perm = permissions.ref(branch.get());
- if (validCommits.contains(id)) {
+ ValidCommitKey key = new AutoValue_ReceiveCommits_ValidCommitKey(id.copy(), branch);
+ if (validCommits.contains(key)) {
return true;
}
@@ -2912,7 +2911,7 @@
reject(cmd, e.getMessage());
return false;
}
- validCommits.add(c.copy());
+ validCommits.add(key);
return true;
}
diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java
index 0ed189e..319e2e1 100644
--- a/java/com/google/gerrit/server/submit/SubmoduleOp.java
+++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java
@@ -516,13 +516,26 @@
+ "doesn't have gitlink file mode.";
throw new SubmoduleException(errMsg);
}
+ // Parse the current gitlink entry commit in the subproject repo. This is used to add a
+ // shortlog for this submodule to the commit message in the superproject.
+ //
+ // Even if we don't strictly speaking need that commit message, parsing the commit is a sanity
+ // check that the old gitlink is a commit that actually exists. If not, then there is an
+ // inconsistency between the superproject and subproject state, and we don't want to risk
+ // making things worse by updating the gitlink to something else.
oldCommit = subOr.rw.parseCommit(dce.getObjectId());
}
final CodeReviewCommit newCommit;
if (branchTips.containsKey(s.getSubmodule())) {
+ // This submodule's branch was updated as part of this specific submit batch: update the
+ // gitlink to point to the new commit from the batch.
newCommit = branchTips.get(s.getSubmodule());
} else {
+ // For whatever reason, this submodule was not updated as part of this submit batch, but the
+ // superproject is still subscribed to this branch. Re-read the ref to see if anything has
+ // changed since the last time the gitlink was updated, and roll that update into the same
+ // commit as all other submodule updates.
Ref ref = subOr.repo.getRefDatabase().exactRef(s.getSubmodule().get());
if (ref == null) {
ed.add(new DeletePath(s.getPath()));
diff --git a/javatests/com/google/gerrit/proto/BUILD b/javatests/com/google/gerrit/proto/BUILD
new file mode 100644
index 0000000..0940f6b
--- /dev/null
+++ b/javatests/com/google/gerrit/proto/BUILD
@@ -0,0 +1,17 @@
+load("//tools/bzl:junit.bzl", "junit_tests")
+
+junit_tests(
+ name = "proto_tests",
+ srcs = glob(["*.java"]),
+ deps = [
+ "//lib/truth:truth-proto-extension",
+ "//proto:reviewdb_java_proto",
+
+ # TODO(dborowitz): These are already runtime_deps of
+ # truth-proto-extension, but either omitting them or adding them as
+ # runtime_deps to this target fails with:
+ # class file for com.google.common.collect.Multimap not found
+ "//lib:guava",
+ "//lib/truth",
+ ],
+)
diff --git a/javatests/com/google/gerrit/proto/ReviewDbProtoTest.java b/javatests/com/google/gerrit/proto/ReviewDbProtoTest.java
new file mode 100644
index 0000000..49cd2921
--- /dev/null
+++ b/javatests/com/google/gerrit/proto/ReviewDbProtoTest.java
@@ -0,0 +1,31 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.proto;
+
+import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
+
+import com.google.gerrit.proto.reviewdb.Reviewdb.Change;
+import com.google.gerrit.proto.reviewdb.Reviewdb.Change_Id;
+import org.junit.Test;
+
+public class ReviewDbProtoTest {
+ @Test
+ public void generatedProtoApi() {
+ Change c1 = Change.newBuilder().setChangeId(Change_Id.newBuilder().setId(1234).build()).build();
+ Change c2 = Change.newBuilder().setChangeId(Change_Id.newBuilder().setId(5678).build()).build();
+ assertThat(c1).isEqualTo(c1);
+ assertThat(c1).isNotEqualTo(c2);
+ }
+}
diff --git a/lib/BUILD b/lib/BUILD
index a9d82e9..38b0b80 100644
--- a/lib/BUILD
+++ b/lib/BUILD
@@ -72,7 +72,11 @@
name = "gwtorm",
visibility = ["//visibility:public"],
exports = [":gwtorm-client"],
- runtime_deps = [":protobuf"],
+ runtime_deps = [
+ ":protobuf",
+ "//lib/antlr:java-runtime",
+ "//lib/ow2:ow2-asm",
+ ],
)
java_library(
diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
index 89f3038..60eaf1f 100644
--- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
+++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
@@ -48,7 +48,9 @@
shouldSuppressKeyboardShortcut(e) {
e = getKeyboardEvent(e);
const tagName = Polymer.dom(e).rootTarget.tagName;
- if (tagName === 'INPUT' || tagName === 'TEXTAREA') {
+ if (tagName === 'INPUT' || tagName === 'TEXTAREA' ||
+ (e.keyCode === 13 && tagName === 'A')) {
+ // Suppress shortcuts if the key is 'enter' and target is an anchor.
return true;
}
for (let i = 0; e.path && i < e.path.length; i++) {
diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
index 8e10302..04193dd 100644
--- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
+++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
@@ -51,6 +51,7 @@
behaviors: [Gerrit.KeyboardShortcutBehavior],
keyBindings: {
k: '_handleKey',
+ enter: '_handleKey',
},
_handleKey() {},
});
@@ -107,6 +108,17 @@
MockInteractions.keyDownOn(divEl, 75, null, 'k');
});
+ test('blocks enter shortcut on an anchor', done => {
+ const anchorEl = document.createElement('a');
+ const element = overlay.querySelector('test-element');
+ element.appendChild(anchorEl);
+ element._handleKey = e => {
+ assert.isTrue(element.shouldSuppressKeyboardShortcut(e));
+ done();
+ };
+ MockInteractions.keyDownOn(anchorEl, 13, null, 'enter');
+ });
+
test('modifierPressed returns accurate values', () => {
const spy = sandbox.spy(element, 'modifierPressed');
element._handleKey = e => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
index edf9f5a..90d465f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -449,6 +449,13 @@
this.fire('comment-discard', this._getEventPayload());
},
+ _handleFix() {
+ this.dispatchEvent(new CustomEvent('create-fix-comment', {
+ bubbles: true,
+ detail: this._getEventPayload(),
+ }));
+ },
+
_handleDiscard(e) {
e.preventDefault();
this.$.reporting.recordDraftInteraction();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
index f1ac649..ca85892 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -841,5 +841,16 @@
element.save();
assert.isTrue(discardStub.called);
});
+
+ test('_handleFix fires create-fix event', done => {
+ element.addEventListener('create-fix-comment', e => {
+ assert.deepEqual(e.detail, element._getEventPayload());
+ done();
+ });
+ element.isRobotComment = true;
+ flushAsynchronousOperations();
+
+ MockInteractions.tap(element.$$('.fix'));
+ });
});
</script>
diff --git a/proto/BUILD b/proto/BUILD
index 4528dcb..0578ca5 100644
--- a/proto/BUILD
+++ b/proto/BUILD
@@ -8,3 +8,21 @@
visibility = ["//visibility:public"],
deps = [":cache_proto"],
)
+
+genrule(
+ name = "gen_reviewdb_proto",
+ outs = ["reviewdb.proto"],
+ cmd = "$(location //java/com/google/gerrit/proto:ProtoGen) -o $@",
+ tools = ["//java/com/google/gerrit/proto:ProtoGen"],
+)
+
+proto_library(
+ name = "reviewdb_proto",
+ srcs = [":reviewdb.proto"],
+)
+
+java_proto_library(
+ name = "reviewdb_java_proto",
+ visibility = ["//javatests/com/google/gerrit/proto:__pkg__"],
+ deps = [":reviewdb_proto"],
+)
diff --git a/resources/com/google/gerrit/proto/BUILD b/resources/com/google/gerrit/proto/BUILD
new file mode 100644
index 0000000..ec2e05c
--- /dev/null
+++ b/resources/com/google/gerrit/proto/BUILD
@@ -0,0 +1,8 @@
+filegroup(
+ name = "proto",
+ srcs = glob(
+ ["**/*"],
+ exclude = ["BUILD"],
+ ),
+ visibility = ["//visibility:public"],
+)
diff --git a/resources/com/google/gerrit/pgm/ProtoGenHeader.txt b/resources/com/google/gerrit/proto/ProtoGenHeader.txt
similarity index 95%
rename from resources/com/google/gerrit/pgm/ProtoGenHeader.txt
rename to resources/com/google/gerrit/proto/ProtoGenHeader.txt
index a380955..8797790 100644
--- a/resources/com/google/gerrit/pgm/ProtoGenHeader.txt
+++ b/resources/com/google/gerrit/proto/ProtoGenHeader.txt
@@ -14,7 +14,6 @@
syntax = "proto2";
-option java_api_version = 2;
option java_package = "com.google.gerrit.proto.reviewdb";
package devtools.gerritcodereview;
