Merge "Fix null view mode for expanding diffs"
diff --git a/Documentation/database-setup.txt b/Documentation/database-setup.txt
index 159966a..47fef6d 100644
--- a/Documentation/database-setup.txt
+++ b/Documentation/database-setup.txt
@@ -55,6 +55,8 @@
[[createdb_mysql]]
=== MySQL
+Requirements: MySQL version 5.1 or later.
+
This option is also more complicated than the H2 option. Just as with
PostgreSQL it's also recommended for larger installations.
@@ -77,6 +79,8 @@
[[createdb_mariadb]]
=== MariaDB
+Requirements: MariaDB version 5.5 or later.
+
Refer to MySQL section above how to create MariaDB database.
Visit MariaDB's link:https://mariadb.com/kb/en/mariadb/[documentation] for further
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index bd7e7a5..ad2c50c 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -2389,7 +2389,7 @@
and link:https://gerrit.googlesource.com/plugins/metrics-reporter-graphite/[Graphite].
There is also a working example of reporting metrics to the console in the
-link:https://gerrit.googlesource.com/plugins/cookbook-plugin/+/master/src/main/java/com/googlesource/gerrit/plugins/cookbook/ConsoleMetricReporter.jave[
+link:https://gerrit.googlesource.com/plugins/cookbook-plugin/+/master/src/main/java/com/googlesource/gerrit/plugins/cookbook/ConsoleMetricReporter.java[
cookbook plugin].
=== Providing own metrics
diff --git a/Documentation/note-db.txt b/Documentation/note-db.txt
index 728d630..815cda8 100644
--- a/Documentation/note-db.txt
+++ b/Documentation/note-db.txt
@@ -137,14 +137,23 @@
To continue with the full migration after running the trial migration, use
either the online or offline migration steps as normal. To revert to
ReviewDb-only, remove `noteDb.changes.read` and `noteDb.changes.write` from
-`gerrit.config` and restart Gerrit.
+`notedb.config` and restart Gerrit.
== Configuration
-The migration process works by setting a configuration option in `gerrit.config`
+The migration process works by setting a configuration option in `notedb.config`
for each step in the process, then performing the corresponding data migration.
-In general, users should not set these options manually; this section serves
-primarily as a reference.
+
+Config options are read from `notedb.config` first, falling back to
+`gerrit.config`. If editing config manually, you may edit either file, but the
+migration process itself only touches `notedb.config`. This means if your
+`gerrit.config` is managed with Puppet or a similar tool, it can overwrite
+`gerrit.config` without affecting the migration process. You should not manage
+`notedb.config` with Puppet, but you may copy values back into `gerrit.config`
+and delete `notedb.config` at some later point after completing the migration.
+
+In general, users should not set the options described below manually; this
+section serves primarily as a reference.
- `noteDb.changes.write=true`: During a ReviewDb write, the state of the change
in NoteDb is written to the `note_db_state` field in the `Change` entity.
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index 77eb9af..11277f3 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -1112,6 +1112,73 @@
}
----
+
+[[create-access-change]]
+=== Create Access Rights Change for review.
+--
+'PUT /projects/link:rest-api-projects.html#project-name[\{project-name\}]/access:review
+--
+
+Sets access rights for the project using the diff schema provided by
+link:#project-access-input[ProjectAccessInput]
+
+This takes the same input as link:#set-access[Update Access Rights], but creates a pending
+change for review. Like link:#create-change[Create Change], it returns
+a link:#change-info[ChangeInfo] entity describing the resulting change.
+
+.Request
+----
+ PUT /projects/MyProject/access:review HTTP/1.0
+ Content-Type: application/json; charset=UTF-8
+
+ {
+ "add":{
+ "refs/heads/*":{
+ "permissions":{
+ "read":{
+ "rules":{
+ "global:Anonymous-Users": {
+ "action":"DENY",
+ "force":false
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+----
+
+.Response
+----
+ HTTP/1.1 200 OK
+ Content-Type: application/json; charset=UTF-8
+
+ )]}'
+ {
+ "id": "testproj~refs%2Fmeta%2Fconfig~Ieaf185bf90a1fc3b58461e399385e158a20b31a2",
+ "project": "testproj",
+ "branch": "refs/meta/config",
+ "hashtags": [],
+ "change_id": "Ieaf185bf90a1fc3b58461e399385e158a20b31a2",
+ "subject": "Review access change",
+ "status": "NEW",
+ "created": "2017-09-07 14:31:11.852000000",
+ "updated": "2017-09-07 14:31:11.852000000",
+ "submit_type": "CHERRY_PICK",
+ "mergeable": true,
+ "insertions": 2,
+ "deletions": 0,
+ "unresolved_comment_count": 0,
+ "has_review_started": true,
+ "_number": 7,
+ "owner": {
+ "_account_id": 1000000
+ }
+ }
+----
+
+
[[index]]
=== Index all changes in a project
diff --git a/WORKSPACE b/WORKSPACE
index d76b8d4..5f3d1e9 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -716,18 +716,18 @@
sha1 = "4785a3c21320980282f9f33d0d1264a69040538f",
)
-TRUTH_VERS = "0.34"
+TRUTH_VERS = "0.35"
maven_jar(
name = "truth",
artifact = "com.google.truth:truth:" + TRUTH_VERS,
- sha1 = "73379200e907386d27eb10da60c5c2ed339f2bec",
+ sha1 = "c08a7fde45e058323bcfa3f510d4fe1e2b028f37",
)
maven_jar(
name = "truth-java8-extension",
artifact = "com.google.truth.extensions:truth-java8-extension:" + TRUTH_VERS,
- sha1 = "2d4aa36f7c048805af211406c4da4ddd1de15474",
+ sha1 = "5457fdf91b1e954b070ad7f2db9bea5505da4bca",
)
# When bumping the easymock version number, make sure to also move powermock to a compatible version
diff --git a/gerrit-acceptance-framework/BUILD b/gerrit-acceptance-framework/BUILD
index 5a7f3b7..8a95346 100644
--- a/gerrit-acceptance-framework/BUILD
+++ b/gerrit-acceptance-framework/BUILD
@@ -1,6 +1,12 @@
load("//tools/bzl:java.bzl", "java_library2")
+load("//tools/bzl:junit.bzl", "junit_tests")
-SRCS = glob(["src/test/java/com/google/gerrit/acceptance/*.java"])
+TEST_SRCS = ["src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfigTest.java"]
+
+SRCS = glob(
+ ["src/test/java/com/google/gerrit/acceptance/*.java"],
+ exclude = TEST_SRCS,
+)
PROVIDED = [
"//gerrit-common:annotations",
@@ -77,3 +83,14 @@
title = "Gerrit Acceptance Test Framework Documentation",
visibility = ["//visibility:public"],
)
+
+junit_tests(
+ name = "acceptance_framework_tests",
+ srcs = TEST_SRCS,
+ deps = [
+ ":lib",
+ "//lib:guava",
+ "//lib:truth",
+ "//lib/jgit/org.eclipse.jgit:jgit",
+ ],
+)
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 6a1e3b9..0db732b 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -253,6 +253,7 @@
@Inject private InProcessProtocol inProcessProtocol;
@Inject private Provider<AnonymousUser> anonymousUser;
@Inject private SchemaFactory<ReviewDb> reviewDbProvider;
+ @Inject private ChangeControl.GenericFactory changeControlFactory;
private List<Repository> toClose;
@@ -1116,9 +1117,10 @@
}
protected ChangeResource parseChangeResource(String changeId) throws Exception {
- List<ChangeControl> ctls = changeFinder.find(changeId, atrScope.get().getUser());
- assertThat(ctls).hasSize(1);
- return changeResourceFactory.create(ctls.get(0));
+ List<ChangeNotes> notes = changeFinder.find(changeId);
+ assertThat(notes).hasSize(1);
+ return changeResourceFactory.create(
+ changeControlFactory.controlFor(notes.get(0), atrScope.get().getUser()));
}
protected String createGroup(String name) throws Exception {
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfig.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfig.java
index c651d48..bd4f653 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfig.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfig.java
@@ -40,7 +40,7 @@
}
for (String section : s.getSections()) {
for (String subsection : s.getSubsections(section)) {
- for (String name : s.getNames(section, subsection)) {
+ for (String name : s.getNames(section, subsection, true)) {
setStringList(
section,
subsection,
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfigTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfigTest.java
new file mode 100644
index 0000000..49c23e3
--- /dev/null
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfigTest.java
@@ -0,0 +1,118 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.collect.ImmutableList;
+import java.io.File;
+import java.nio.file.Files;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.util.FS;
+import org.junit.Test;
+
+public class MergeableFileBasedConfigTest {
+ @Test
+ public void mergeNull() throws Exception {
+ MergeableFileBasedConfig cfg = newConfig();
+ cfg.setString("foo", null, "bar", "value");
+ String expected = "[foo]\n\tbar = value\n";
+ assertConfig(cfg, expected);
+ cfg.merge(null);
+ assertConfig(cfg, expected);
+ }
+
+ @Test
+ public void mergeFlatConfig() throws Exception {
+ MergeableFileBasedConfig cfg = newConfig();
+ cfg.setString("foo", null, "bar1", "value1");
+ cfg.setString("foo", null, "bar2", "value2");
+ cfg.setString("foo", "sub", "bar1", "value1");
+ cfg.setString("foo", "sub", "bar2", "value2");
+
+ assertConfig(
+ cfg,
+ "[foo]\n"
+ + "\tbar1 = value1\n"
+ + "\tbar2 = value2\n"
+ + "[foo \"sub\"]\n"
+ + "\tbar1 = value1\n"
+ + "\tbar2 = value2\n");
+
+ Config toMerge = new Config();
+ toMerge.setStringList("foo", null, "bar2", ImmutableList.of("merge1", "merge2"));
+ toMerge.setStringList("foo", "sub", "bar2", ImmutableList.of("merge1", "merge2"));
+ cfg.merge(toMerge);
+
+ assertConfig(
+ cfg,
+ "[foo]\n"
+ + "\tbar1 = value1\n"
+ + "\tbar2 = merge1\n"
+ + "\tbar2 = merge2\n"
+ + "[foo \"sub\"]\n"
+ + "\tbar1 = value1\n"
+ + "\tbar2 = merge1\n"
+ + "\tbar2 = merge2\n");
+ }
+
+ @Test
+ public void mergeStackedConfig() throws Exception {
+ MergeableFileBasedConfig cfg = newConfig();
+ cfg.setString("foo", null, "bar1", "value1");
+ cfg.setString("foo", null, "bar2", "value2");
+ cfg.setString("foo", "sub", "bar1", "value1");
+ cfg.setString("foo", "sub", "bar2", "value2");
+
+ assertConfig(
+ cfg,
+ "[foo]\n"
+ + "\tbar1 = value1\n"
+ + "\tbar2 = value2\n"
+ + "[foo \"sub\"]\n"
+ + "\tbar1 = value1\n"
+ + "\tbar2 = value2\n");
+
+ Config base = new Config();
+ Config toMerge = new Config(base);
+ base.setStringList("foo", null, "bar2", ImmutableList.of("merge1", "merge2"));
+ base.setStringList("foo", "sub", "bar2", ImmutableList.of("merge1", "merge2"));
+ cfg.merge(toMerge);
+
+ assertConfig(
+ cfg,
+ "[foo]\n"
+ + "\tbar1 = value1\n"
+ + "\tbar2 = merge1\n"
+ + "\tbar2 = merge2\n"
+ + "[foo \"sub\"]\n"
+ + "\tbar1 = value1\n"
+ + "\tbar2 = merge1\n"
+ + "\tbar2 = merge2\n");
+ }
+
+ private MergeableFileBasedConfig newConfig() throws Exception {
+ File f = File.createTempFile(getClass().getSimpleName(), ".config");
+ f.deleteOnExit();
+ return new MergeableFileBasedConfig(f, FS.detect());
+ }
+
+ private void assertConfig(MergeableFileBasedConfig cfg, String expected) throws Exception {
+ assertThat(cfg.toText()).isEqualTo(expected);
+ cfg.save();
+ assertThat(new String(Files.readAllBytes(cfg.getFile().toPath()), UTF_8)).isEqualTo(expected);
+ }
+}
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
index 03dcbca..5fd8da2 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -480,6 +480,11 @@
assertThat(message(refUpdate).toLowerCase()).contains(expectedMessage.toLowerCase());
}
+ public void assertNotMessage(String message) {
+ RemoteRefUpdate refUpdate = result.getRemoteUpdate(ref);
+ assertThat(message(refUpdate).toLowerCase()).doesNotContain(message.toLowerCase());
+ }
+
public String getMessage() {
RemoteRefUpdate refUpdate = result.getRemoteUpdate(ref);
return message(refUpdate);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/AbandonIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/AbandonIT.java
index cb9d705..52e1ea4 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/AbandonIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/AbandonIT.java
@@ -33,7 +33,6 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.AbandonUtil;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.testutil.TestTimeUtil;
import com.google.inject.Inject;
@@ -64,14 +63,10 @@
public void batchAbandon() throws Exception {
CurrentUser user = atrScope.get().getUser();
PushOneCommit.Result a = createChange();
- List<ChangeControl> controlA = changeFinder.find(a.getChangeId(), user);
- assertThat(controlA).hasSize(1);
PushOneCommit.Result b = createChange();
- List<ChangeControl> controlB = changeFinder.find(b.getChangeId(), user);
- assertThat(controlB).hasSize(1);
- List<ChangeControl> list = ImmutableList.of(controlA.get(0), controlB.get(0));
+ List<ChangeData> list = ImmutableList.of(a.getChange(), b.getChange());
changeAbandoner.batchAbandon(
- batchUpdateFactory, controlA.get(0).getProject().getNameKey(), user, list, "deadbeef");
+ batchUpdateFactory, a.getChange().project(), user, list, "deadbeef");
ChangeInfo info = get(a.getChangeId());
assertThat(info.status).isEqualTo(ChangeStatus.ABANDONED);
@@ -95,12 +90,8 @@
CurrentUser user = atrScope.get().getUser();
PushOneCommit.Result a = createChange(project1, "master", "x", "x", "x", "");
- List<ChangeControl> controlA = changeFinder.find(a.getChangeId(), user);
- assertThat(controlA).hasSize(1);
PushOneCommit.Result b = createChange(project2, "master", "x", "x", "x", "");
- List<ChangeControl> controlB = changeFinder.find(b.getChangeId(), user);
- assertThat(controlB).hasSize(1);
- List<ChangeControl> list = ImmutableList.of(controlA.get(0), controlB.get(0));
+ List<ChangeData> list = ImmutableList.of(a.getChange(), b.getChange());
exception.expect(ResourceConflictException.class);
exception.expectMessage(
String.format("Project name \"%s\" doesn't match \"%s\"", project2Name, project1Name));
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 4ce412c..8041e9f 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -116,7 +116,6 @@
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -3304,9 +3303,7 @@
}
private ChangeResource parseResource(PushOneCommit.Result r) throws Exception {
- List<ChangeControl> ctls = changeFinder.find(r.getChangeId(), atrScope.get().getUser());
- assertThat(ctls).hasSize(1);
- return changeResourceFactory.create(ctls.get(0));
+ return parseChangeResource(r.getChangeId());
}
private Optional<ReviewerState> getReviewerState(String changeId, Account.Id accountId)
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 0dc1389..930b996 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -256,7 +256,7 @@
@Test
public void output() throws Exception {
- String url = canonicalWebUrl.get();
+ String url = canonicalWebUrl.get() + "#/c/" + project.get() + "/+/";
ObjectId initialHead = testRepo.getRepository().resolve("HEAD");
PushOneCommit.Result r1 = pushTo("refs/for/master");
Change.Id id1 = r1.getChange().getId();
@@ -298,6 +298,22 @@
}
@Test
+ public void autoclose() throws Exception {
+ // Create a change
+ PushOneCommit.Result r = pushTo("refs/for/master");
+ r.assertOkStatus();
+
+ // Force push it, closing it
+ String master = "refs/heads/master";
+ assertPushOk(pushHead(testRepo, master, false), master);
+
+ // Attempt to push amended commit to same change
+ String url = canonicalWebUrl.get() + "#/c/" + project.get() + "/+/" + r.getChange().getId();
+ r = amendChange(r.getChangeId(), "refs/for/master");
+ r.assertErrorStatus("change " + url + " closed");
+ }
+
+ @Test
public void pushForMasterWithTopic() throws Exception {
// specify topic in ref
String topic = "my/topic";
@@ -486,26 +502,31 @@
// Push a private change.
PushOneCommit.Result r = pushTo("refs/for/master%private");
r.assertOkStatus();
+ r.assertMessage(" [PRIVATE]");
assertThat(r.getChange().change().isPrivate()).isTrue();
// Pushing a new patch set without --private doesn't remove the privacy flag from the change.
r = amendChange(r.getChangeId(), "refs/for/master");
r.assertOkStatus();
+ r.assertMessage(" [PRIVATE]");
assertThat(r.getChange().change().isPrivate()).isTrue();
// Remove the privacy flag from the change.
r = amendChange(r.getChangeId(), "refs/for/master%remove-private");
r.assertOkStatus();
+ r.assertNotMessage(" [PRIVATE]");
assertThat(r.getChange().change().isPrivate()).isFalse();
// Normal push: privacy flag is not added back.
r = amendChange(r.getChangeId(), "refs/for/master");
r.assertOkStatus();
+ r.assertNotMessage(" [PRIVATE]");
assertThat(r.getChange().change().isPrivate()).isFalse();
// Make the change private again.
r = pushTo("refs/for/master%private");
r.assertOkStatus();
+ r.assertMessage(" [PRIVATE]");
assertThat(r.getChange().change().isPrivate()).isTrue();
// Can't use --private and --remove-private together.
@@ -518,30 +539,35 @@
// Push a work-in-progress change.
PushOneCommit.Result r = pushTo("refs/for/master%wip");
r.assertOkStatus();
+ r.assertMessage(" [WIP]");
assertThat(r.getChange().change().isWorkInProgress()).isTrue();
assertUploadTag(r.getChange(), ChangeMessagesUtil.TAG_UPLOADED_WIP_PATCH_SET);
// Pushing a new patch set without --wip doesn't remove the wip flag from the change.
r = amendChange(r.getChangeId(), "refs/for/master");
r.assertOkStatus();
+ r.assertMessage(" [WIP]");
assertThat(r.getChange().change().isWorkInProgress()).isTrue();
assertUploadTag(r.getChange(), ChangeMessagesUtil.TAG_UPLOADED_WIP_PATCH_SET);
// Remove the wip flag from the change.
r = amendChange(r.getChangeId(), "refs/for/master%ready");
r.assertOkStatus();
+ r.assertNotMessage(" [WIP]");
assertThat(r.getChange().change().isWorkInProgress()).isFalse();
assertUploadTag(r.getChange(), ChangeMessagesUtil.TAG_UPLOADED_PATCH_SET);
// Normal push: wip flag is not added back.
r = amendChange(r.getChangeId(), "refs/for/master");
r.assertOkStatus();
+ r.assertNotMessage(" [WIP]");
assertThat(r.getChange().change().isWorkInProgress()).isFalse();
assertUploadTag(r.getChange(), ChangeMessagesUtil.TAG_UPLOADED_PATCH_SET);
// Make the change work-in-progress again.
r = amendChange(r.getChangeId(), "refs/for/master%wip");
r.assertOkStatus();
+ r.assertMessage(" [WIP]");
assertThat(r.getChange().change().isWorkInProgress()).isTrue();
assertUploadTag(r.getChange(), ChangeMessagesUtil.TAG_UPLOADED_WIP_PATCH_SET);
@@ -635,6 +661,9 @@
r.assertMessage(
"Updated Changes:\n "
+ canonicalWebUrl.get()
+ + "#/c/"
+ + project.get()
+ + "/+/"
+ r.getChange().getId()
+ " "
+ editInfo.commit.subject
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java
index 0517675..8c50b90 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java
@@ -61,6 +61,7 @@
@NoHttpd
public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
private StoredConfig gerritConfig;
+ private StoredConfig noteDbConfig;
private Project.NameKey project;
private Change.Id changeId;
@@ -69,10 +70,14 @@
public void setUp() throws Exception {
assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.OFF);
gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
+ // Unlike in the running server, for tests, we don't stack notedb.config on gerrit.config.
+ noteDbConfig = new FileBasedConfig(sitePaths.notedb_config.toFile(), FS.detect());
}
@Test
public void rebuildOneChangeTrialMode() throws Exception {
+ assertNoAutoMigrateConfig(gerritConfig);
+ assertNoAutoMigrateConfig(noteDbConfig);
assertNotesMigrationState(NotesMigrationState.REVIEW_DB);
setUpOneChange();
@@ -101,6 +106,8 @@
@Test
public void migrateOneChange() throws Exception {
+ assertNoAutoMigrateConfig(gerritConfig);
+ assertNoAutoMigrateConfig(noteDbConfig);
assertNotesMigrationState(NotesMigrationState.REVIEW_DB);
setUpOneChange();
@@ -128,6 +135,8 @@
assertThat(db.changes().get(id2)).isNull();
}
}
+ assertNoAutoMigrateConfig(gerritConfig);
+ assertAutoMigrateConfig(noteDbConfig, false);
}
@Test
@@ -151,6 +160,40 @@
@Test
public void onlineMigrationViaDaemon() throws Exception {
+ assertNoAutoMigrateConfig(gerritConfig);
+ assertNoAutoMigrateConfig(noteDbConfig);
+
+ testOnlineMigration(u -> startServer(u.module(), "--migrate-to-note-db", "true"));
+
+ assertNoAutoMigrateConfig(gerritConfig);
+ assertAutoMigrateConfig(noteDbConfig, false);
+ }
+
+ @Test
+ public void onlineMigrationViaConfig() throws Exception {
+ assertNoAutoMigrateConfig(gerritConfig);
+ assertNoAutoMigrateConfig(noteDbConfig);
+
+ testOnlineMigration(
+ u -> {
+ gerritConfig.setBoolean("noteDb", "changes", "autoMigrate", true);
+ gerritConfig.save();
+ return startServer(u.module());
+ });
+
+ // Auto-migration is turned off in notedb.config, which takes precedence, but is still on in
+ // gerrit.config. This means Puppet can continue overwriting gerrit.config without turning
+ // auto-migration back on.
+ assertAutoMigrateConfig(gerritConfig, true);
+ assertAutoMigrateConfig(noteDbConfig, false);
+ }
+
+ @FunctionalInterface
+ private interface StartServerWithMigration {
+ ServerContext start(IndexUpgradeController u) throws Exception;
+ }
+
+ private void testOnlineMigration(StartServerWithMigration start) throws Exception {
assertNotesMigrationState(NotesMigrationState.REVIEW_DB);
int prevVersion = ChangeSchemaDefinitions.INSTANCE.getPrevious().getVersion();
int currVersion = ChangeSchemaDefinitions.INSTANCE.getLatest().getVersion();
@@ -166,7 +209,7 @@
setOnlineUpgradeConfig(true);
IndexUpgradeController u = new IndexUpgradeController(1);
- try (ServerContext ctx = startServer(u.module(), "--migrate-to-note-db", "true")) {
+ try (ServerContext ctx = start.start(u)) {
ChangeIndexCollection indexes = ctx.getInjector().getInstance(ChangeIndexCollection.class);
assertThat(indexes.getSearchIndex().getSchema().getVersion()).isEqualTo(prevVersion);
@@ -199,8 +242,8 @@
}
private void assertNotesMigrationState(NotesMigrationState expected) throws Exception {
- gerritConfig.load();
- assertThat(NotesMigrationState.forConfig(gerritConfig)).hasValue(expected);
+ noteDbConfig.load();
+ assertThat(NotesMigrationState.forConfig(noteDbConfig)).hasValue(expected);
}
private ReviewDb openUnderlyingReviewDb(ServerContext ctx) throws Exception {
@@ -209,6 +252,17 @@
.open();
}
+ private static void assertNoAutoMigrateConfig(StoredConfig cfg) throws Exception {
+ cfg.load();
+ assertThat(cfg.getString("noteDb", "changes", "autoMigrate")).isNull();
+ }
+
+ private static void assertAutoMigrateConfig(StoredConfig cfg, boolean expected) throws Exception {
+ cfg.load();
+ assertThat(cfg.getString("noteDb", "changes", "autoMigrate")).isNotNull();
+ assertThat(cfg.getBoolean("noteDb", "changes", "autoMigrate", false)).isEqualTo(expected);
+ }
+
private void setOnlineUpgradeConfig(boolean enable) throws Exception {
gerritConfig.load();
gerritConfig.setBoolean("index", null, "onlineUpgrade", enable);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
index ff2b2e1..dd0c7f0 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
@@ -14,6 +14,7 @@
package com.google.gerrit.acceptance.rest.project;
import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
@@ -26,7 +27,11 @@
import com.google.gerrit.extensions.api.access.PermissionRuleInfo;
import com.google.gerrit.extensions.api.access.ProjectAccessInfo;
import com.google.gerrit.extensions.api.access.ProjectAccessInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.projects.BranchInfo;
import com.google.gerrit.extensions.api.projects.ProjectApi;
+import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -87,6 +92,66 @@
}
@Test
+ public void createAccessChange() throws Exception {
+ // User can see the branch
+ setApiUser(user);
+ gApi.projects().name(newProjectName).branch("refs/heads/master").get();
+
+ ProjectAccessInput accessInput = newProjectAccessInput();
+
+ AccessSectionInfo accessSection = newAccessSectionInfo();
+
+ // Deny read to registered users.
+ PermissionInfo read = newPermissionInfo();
+ PermissionRuleInfo pri = new PermissionRuleInfo(PermissionRuleInfo.Action.DENY, false);
+ read.rules.put(SystemGroupBackend.REGISTERED_USERS.get(), pri);
+ read.exclusive = true;
+ accessSection.permissions.put(Permission.READ, read);
+ accessInput.add.put(REFS_HEADS, accessSection);
+
+ setApiUser(user);
+ ChangeInfo out = pApi.accessChange(accessInput);
+
+ assertThat(out.project).isEqualTo(newProjectName);
+ assertThat(out.branch).isEqualTo(RefNames.REFS_CONFIG);
+ assertThat(out.status).isEqualTo(ChangeStatus.NEW);
+ assertThat(out.submitted).isNull();
+
+ setApiUser(admin);
+
+ ReviewInput reviewIn = new ReviewInput();
+ reviewIn.label("Code-Review", (short) 2);
+ gApi.changes().id(out._number).current().review(reviewIn);
+ gApi.changes().id(out._number).current().submit();
+
+ // check that the change took effect.
+ setApiUser(user);
+ try {
+ BranchInfo info = gApi.projects().name(newProjectName).branch("refs/heads/master").get();
+ fail("wanted failure, got " + newGson().toJson(info));
+ } catch (ResourceNotFoundException e) {
+ // OK.
+ }
+
+ // Restore.
+ accessInput.add.clear();
+ accessInput.remove.put(REFS_HEADS, accessSection);
+ setApiUser(user);
+
+ pApi.accessChange(accessInput);
+
+ setApiUser(admin);
+ out = pApi.accessChange(accessInput);
+
+ gApi.changes().id(out._number).current().review(reviewIn);
+ gApi.changes().id(out._number).current().submit();
+
+ // Now it works again.
+ setApiUser(user);
+ gApi.projects().name(newProjectName).branch("refs/heads/master").get();
+ }
+
+ @Test
public void removePermission() throws Exception {
// Add initial permission set
ProjectAccessInput accessInput = newProjectAccessInput();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
index b3a53b0..06639fe 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
@@ -47,6 +47,7 @@
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.update.BatchUpdate;
@@ -76,6 +77,8 @@
public class ConsistencyCheckerIT extends AbstractDaemonTest {
@Inject private ChangeControl.GenericFactory changeControlFactory;
+ @Inject private ChangeNotes.Factory changeNotesFactory;
+
@Inject private Provider<ConsistencyChecker> checkerProvider;
@Inject private IdentifiedUser.GenericFactory userFactory;
@@ -118,17 +121,17 @@
@Test
public void validMergedChange() throws Exception {
- ChangeControl ctl = mergeChange(incrementPatchSet(insertChange()));
- assertNoProblems(ctl, null);
+ ChangeNotes notes = mergeChange(incrementPatchSet(insertChange()));
+ assertNoProblems(notes, null);
}
@Test
public void missingOwner() throws Exception {
TestAccount owner = accountCreator.create("missing");
- ChangeControl ctl = insertChange(owner);
+ ChangeNotes notes = insertChange(owner);
accountsUpdate.create().deleteByKey(owner.getId());
- assertProblems(ctl, null, problem("Missing change owner: " + owner.getId()));
+ assertProblems(notes, null, problem("Missing change owner: " + owner.getId()));
}
@Test
@@ -136,11 +139,14 @@
// NoteDb can't have a change without a repo.
assumeNoteDbDisabled();
- ChangeControl ctl = insertChange();
- Project.NameKey name = ctl.getProject().getNameKey();
+ ChangeNotes notes = insertChange();
+ Project.NameKey name = notes.getProjectName();
+ // Create control before deleting repo to avoid NoSuchProjectException
+ ChangeControl ctl = controlForNotes(notes);
((InMemoryRepositoryManager) repoManager).deleteRepository(name);
- assertProblems(ctl, null, problem("Destination repository not found: " + name));
+ assertThat(checker.check(ctl, null).problems())
+ .containsExactly(problem("Destination repository not found: " + name));
}
@Test
@@ -149,16 +155,16 @@
// create an invalid patch set.
assumeNoteDbDisabled();
- ChangeControl ctl = insertChange();
+ ChangeNotes notes = insertChange();
PatchSet ps =
newPatchSet(
- ctl.getChange().currentPatchSetId(),
+ notes.getChange().currentPatchSetId(),
"fooooooooooooooooooooooooooooooooooooooo",
adminId);
db.patchSets().update(singleton(ps));
assertProblems(
- ctl,
+ notes,
null,
problem("Invalid revision on patch set 1: fooooooooooooooooooooooooooooooooooooooo"));
}
@@ -169,11 +175,11 @@
@Test
public void patchSetObjectAndRefMissing() throws Exception {
String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
- ChangeControl ctl = insertChange();
- PatchSet ps = insertMissingPatchSet(ctl, rev);
- ctl = reload(ctl);
+ ChangeNotes notes = insertChange();
+ PatchSet ps = insertMissingPatchSet(notes, rev);
+ notes = reload(notes);
assertProblems(
- ctl,
+ notes,
null,
problem("Ref missing: " + ps.getId().toRefName()),
problem("Object missing: patch set 2: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
@@ -182,13 +188,13 @@
@Test
public void patchSetObjectAndRefMissingWithFix() throws Exception {
String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
- ChangeControl ctl = insertChange();
- PatchSet ps = insertMissingPatchSet(ctl, rev);
- ctl = reload(ctl);
+ ChangeNotes notes = insertChange();
+ PatchSet ps = insertMissingPatchSet(notes, rev);
+ notes = reload(notes);
String refName = ps.getId().toRefName();
assertProblems(
- ctl,
+ notes,
new FixInput(),
problem("Ref missing: " + refName),
problem("Object missing: patch set 2: " + rev));
@@ -196,83 +202,82 @@
@Test
public void patchSetRefMissing() throws Exception {
- ChangeControl ctl = insertChange();
+ ChangeNotes notes = insertChange();
testRepo.update(
- "refs/other/foo",
- ObjectId.fromString(psUtil.current(db, ctl.getNotes()).getRevision().get()));
- String refName = ctl.getChange().currentPatchSetId().toRefName();
+ "refs/other/foo", ObjectId.fromString(psUtil.current(db, notes).getRevision().get()));
+ String refName = notes.getChange().currentPatchSetId().toRefName();
deleteRef(refName);
- assertProblems(ctl, null, problem("Ref missing: " + refName));
+ assertProblems(notes, null, problem("Ref missing: " + refName));
}
@Test
public void patchSetRefMissingWithFix() throws Exception {
- ChangeControl ctl = insertChange();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ ChangeNotes notes = insertChange();
+ String rev = psUtil.current(db, notes).getRevision().get();
testRepo.update("refs/other/foo", ObjectId.fromString(rev));
- String refName = ctl.getChange().currentPatchSetId().toRefName();
+ String refName = notes.getChange().currentPatchSetId().toRefName();
deleteRef(refName);
assertProblems(
- ctl, new FixInput(), problem("Ref missing: " + refName, FIXED, "Repaired patch set ref"));
+ notes, new FixInput(), problem("Ref missing: " + refName, FIXED, "Repaired patch set ref"));
assertThat(testRepo.getRepository().exactRef(refName).getObjectId().name()).isEqualTo(rev);
}
@Test
public void patchSetObjectAndRefMissingWithDeletingPatchSet() throws Exception {
- ChangeControl ctl = insertChange();
- PatchSet ps1 = psUtil.current(db, ctl.getNotes());
+ ChangeNotes notes = insertChange();
+ PatchSet ps1 = psUtil.current(db, notes);
String rev2 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
- PatchSet ps2 = insertMissingPatchSet(ctl, rev2);
- ctl = reload(ctl);
+ PatchSet ps2 = insertMissingPatchSet(notes, rev2);
+ notes = reload(notes);
FixInput fix = new FixInput();
fix.deletePatchSetIfCommitMissing = true;
assertProblems(
- ctl,
+ notes,
fix,
problem("Ref missing: " + ps2.getId().toRefName()),
problem("Object missing: patch set 2: " + rev2, FIXED, "Deleted patch set"));
- ctl = reload(ctl);
- assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(1);
- assertThat(psUtil.get(db, ctl.getNotes(), ps1.getId())).isNotNull();
- assertThat(psUtil.get(db, ctl.getNotes(), ps2.getId())).isNull();
+ notes = reload(notes);
+ assertThat(notes.getChange().currentPatchSetId().get()).isEqualTo(1);
+ assertThat(psUtil.get(db, notes, ps1.getId())).isNotNull();
+ assertThat(psUtil.get(db, notes, ps2.getId())).isNull();
}
@Test
public void patchSetMultipleObjectsMissingWithDeletingPatchSets() throws Exception {
- ChangeControl ctl = insertChange();
- PatchSet ps1 = psUtil.current(db, ctl.getNotes());
+ ChangeNotes notes = insertChange();
+ PatchSet ps1 = psUtil.current(db, notes);
String rev2 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
- PatchSet ps2 = insertMissingPatchSet(ctl, rev2);
+ PatchSet ps2 = insertMissingPatchSet(notes, rev2);
- ctl = incrementPatchSet(reload(ctl));
- PatchSet ps3 = psUtil.current(db, ctl.getNotes());
+ notes = incrementPatchSet(reload(notes));
+ PatchSet ps3 = psUtil.current(db, notes);
String rev4 = "c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee";
- PatchSet ps4 = insertMissingPatchSet(ctl, rev4);
- ctl = reload(ctl);
+ PatchSet ps4 = insertMissingPatchSet(notes, rev4);
+ notes = reload(notes);
FixInput fix = new FixInput();
fix.deletePatchSetIfCommitMissing = true;
assertProblems(
- ctl,
+ notes,
fix,
problem("Ref missing: " + ps2.getId().toRefName()),
problem("Object missing: patch set 2: " + rev2, FIXED, "Deleted patch set"),
problem("Ref missing: " + ps4.getId().toRefName()),
problem("Object missing: patch set 4: " + rev4, FIXED, "Deleted patch set"));
- ctl = reload(ctl);
- assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(3);
- assertThat(psUtil.get(db, ctl.getNotes(), ps1.getId())).isNotNull();
- assertThat(psUtil.get(db, ctl.getNotes(), ps2.getId())).isNull();
- assertThat(psUtil.get(db, ctl.getNotes(), ps3.getId())).isNotNull();
- assertThat(psUtil.get(db, ctl.getNotes(), ps4.getId())).isNull();
+ notes = reload(notes);
+ assertThat(notes.getChange().currentPatchSetId().get()).isEqualTo(3);
+ assertThat(psUtil.get(db, notes, ps1.getId())).isNotNull();
+ assertThat(psUtil.get(db, notes, ps2.getId())).isNull();
+ assertThat(psUtil.get(db, notes, ps3.getId())).isNotNull();
+ assertThat(psUtil.get(db, notes, ps4.getId())).isNull();
}
@Test
@@ -309,13 +314,12 @@
+ rev
+ "\n");
indexer.index(db, c.getProject(), c.getId());
- IdentifiedUser user = userFactory.create(admin.getId());
- ChangeControl ctl = changeControlFactory.controlFor(db, c.getProject(), c.getId(), user);
+ ChangeNotes notes = changeNotesFactory.create(db, c.getProject(), c.getId());
FixInput fix = new FixInput();
fix.deletePatchSetIfCommitMissing = true;
assertProblems(
- ctl,
+ notes,
fix,
problem("Ref missing: " + ps.getId().toRefName()),
problem(
@@ -323,9 +327,9 @@
FIX_FAILED,
"Cannot delete patch set; no patch sets would remain"));
- ctl = reload(ctl);
- assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(1);
- assertThat(psUtil.current(db, ctl.getNotes())).isNotNull();
+ notes = reload(notes);
+ assertThat(notes.getChange().currentPatchSetId().get()).isEqualTo(1);
+ assertThat(psUtil.current(db, notes)).isNotNull();
}
@Test
@@ -333,25 +337,25 @@
// NoteDb can't create a change without a patch set.
assumeNoteDbDisabled();
- ChangeControl ctl = insertChange();
- db.patchSets().deleteKeys(singleton(ctl.getChange().currentPatchSetId()));
- assertProblems(ctl, null, problem("Current patch set 1 not found"));
+ ChangeNotes notes = insertChange();
+ db.patchSets().deleteKeys(singleton(notes.getChange().currentPatchSetId()));
+ assertProblems(notes, null, problem("Current patch set 1 not found"));
}
@Test
public void duplicatePatchSetRevisions() throws Exception {
- ChangeControl ctl = insertChange();
- PatchSet ps1 = psUtil.current(db, ctl.getNotes());
+ ChangeNotes notes = insertChange();
+ PatchSet ps1 = psUtil.current(db, notes);
String rev = ps1.getRevision().get();
- ctl = incrementPatchSet(ctl, testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
+ notes = incrementPatchSet(notes, testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
- assertProblems(ctl, null, problem("Multiple patch sets pointing to " + rev + ": [1, 2]"));
+ assertProblems(notes, null, problem("Multiple patch sets pointing to " + rev + ": [1, 2]"));
}
@Test
public void missingDestRef() throws Exception {
- ChangeControl ctl = insertChange();
+ ChangeNotes notes = insertChange();
String ref = "refs/heads/master";
// Detach head so we're allowed to delete ref.
@@ -360,16 +364,16 @@
ru.setForceUpdate(true);
assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
- assertProblems(ctl, null, problem("Destination ref not found (may be new branch): " + ref));
+ assertProblems(notes, null, problem("Destination ref not found (may be new branch): " + ref));
}
@Test
public void mergedChangeIsNotMerged() throws Exception {
- ChangeControl ctl = insertChange();
+ ChangeNotes notes = insertChange();
try (BatchUpdate bu = newUpdate(adminId)) {
bu.addOp(
- ctl.getId(),
+ notes.getChangeId(),
new BatchUpdateOp() {
@Override
public boolean updateChange(ChangeContext ctx) throws OrmException {
@@ -380,12 +384,12 @@
});
bu.execute();
}
- ctl = reload(ctl);
+ notes = reload(notes);
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
- ObjectId tip = getDestRef(ctl);
+ String rev = psUtil.current(db, notes).getRevision().get();
+ ObjectId tip = getDestRef(notes);
assertProblems(
- ctl,
+ notes,
null,
problem(
"Patch set 1 ("
@@ -398,14 +402,14 @@
@Test
public void newChangeIsMerged() throws Exception {
- ChangeControl ctl = insertChange();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ ChangeNotes notes = insertChange();
+ String rev = psUtil.current(db, notes).getRevision().get();
testRepo
- .branch(ctl.getChange().getDest().get())
+ .branch(notes.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
assertProblems(
- ctl,
+ notes,
null,
problem(
"Patch set 1 ("
@@ -418,14 +422,14 @@
@Test
public void newChangeIsMergedWithFix() throws Exception {
- ChangeControl ctl = insertChange();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ ChangeNotes notes = insertChange();
+ String rev = psUtil.current(db, notes).getRevision().get();
testRepo
- .branch(ctl.getChange().getDest().get())
+ .branch(notes.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
assertProblems(
- ctl,
+ notes,
new FixInput(),
problem(
"Patch set 1 ("
@@ -437,38 +441,38 @@
FIXED,
"Marked change as merged"));
- ctl = reload(ctl);
- assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
- assertNoProblems(ctl, null);
+ notes = reload(notes);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
+ assertNoProblems(notes, null);
}
@Test
public void extensionApiReturnsUpdatedValueAfterFix() throws Exception {
- ChangeControl ctl = insertChange();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ ChangeNotes notes = insertChange();
+ String rev = psUtil.current(db, notes).getRevision().get();
testRepo
- .branch(ctl.getChange().getDest().get())
+ .branch(notes.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
- ChangeInfo info = gApi.changes().id(ctl.getId().get()).info();
+ ChangeInfo info = gApi.changes().id(notes.getChangeId().get()).info();
assertThat(info.status).isEqualTo(ChangeStatus.NEW);
- info = gApi.changes().id(ctl.getId().get()).check(new FixInput());
+ info = gApi.changes().id(notes.getChangeId().get()).check(new FixInput());
assertThat(info.status).isEqualTo(ChangeStatus.MERGED);
}
@Test
public void expectedMergedCommitIsLatestPatchSet() throws Exception {
- ChangeControl ctl = insertChange();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ ChangeNotes notes = insertChange();
+ String rev = psUtil.current(db, notes).getRevision().get();
testRepo
- .branch(ctl.getChange().getDest().get())
+ .branch(notes.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
FixInput fix = new FixInput();
fix.expectMergedAs = rev;
assertProblems(
- ctl,
+ notes,
fix,
problem(
"Patch set 1 ("
@@ -480,23 +484,23 @@
FIXED,
"Marked change as merged"));
- ctl = reload(ctl);
- assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
- assertNoProblems(ctl, null);
+ notes = reload(notes);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
+ assertNoProblems(notes, null);
}
@Test
public void expectedMergedCommitNotMergedIntoDestination() throws Exception {
- ChangeControl ctl = insertChange();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ ChangeNotes notes = insertChange();
+ String rev = psUtil.current(db, notes).getRevision().get();
RevCommit commit = testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
- testRepo.branch(ctl.getChange().getDest().get()).update(commit);
+ testRepo.branch(notes.getChange().getDest().get()).update(commit);
FixInput fix = new FixInput();
RevCommit other = testRepo.commit().message(commit.getFullMessage()).create();
fix.expectMergedAs = other.name();
assertProblems(
- ctl,
+ notes,
fix,
problem(
"Expected merged commit "
@@ -509,9 +513,9 @@
@Test
public void createNewPatchSetForExpectedMergeCommitWithNoChangeId() throws Exception {
- ChangeControl ctl = insertChange();
- String dest = ctl.getChange().getDest().get();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ ChangeNotes notes = insertChange();
+ String dest = notes.getChange().getDest().get();
+ String rev = psUtil.current(db, notes).getRevision().get();
RevCommit commit = testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
RevCommit mergedAs =
@@ -520,12 +524,12 @@
assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)).isEmpty();
testRepo.update(dest, mergedAs);
- assertNoProblems(ctl, null);
+ assertNoProblems(notes, null);
FixInput fix = new FixInput();
fix.expectMergedAs = mergedAs.name();
assertProblems(
- ctl,
+ notes,
fix,
problem(
"No patch set found for merged commit " + mergedAs.name(),
@@ -536,20 +540,19 @@
FIXED,
"Inserted as patch set 2"));
- ctl = reload(ctl);
- PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2);
- assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId2);
- assertThat(psUtil.get(db, ctl.getNotes(), psId2).getRevision().get())
- .isEqualTo(mergedAs.name());
+ notes = reload(notes);
+ PatchSet.Id psId2 = new PatchSet.Id(notes.getChangeId(), 2);
+ assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId2);
+ assertThat(psUtil.get(db, notes, psId2).getRevision().get()).isEqualTo(mergedAs.name());
- assertNoProblems(ctl, null);
+ assertNoProblems(notes, null);
}
@Test
public void createNewPatchSetForExpectedMergeCommitWithChangeId() throws Exception {
- ChangeControl ctl = insertChange();
- String dest = ctl.getChange().getDest().get();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ ChangeNotes notes = insertChange();
+ String dest = notes.getChange().getDest().get();
+ String rev = psUtil.current(db, notes).getRevision().get();
RevCommit commit = testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
RevCommit mergedAs =
@@ -561,20 +564,20 @@
+ "\n"
+ "\n"
+ "Change-Id: "
- + ctl.getChange().getKey().get()
+ + notes.getChange().getKey().get()
+ "\n")
.create();
testRepo.getRevWalk().parseBody(mergedAs);
assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID))
- .containsExactly(ctl.getChange().getKey().get());
+ .containsExactly(notes.getChange().getKey().get());
testRepo.update(dest, mergedAs);
- assertNoProblems(ctl, null);
+ assertNoProblems(notes, null);
FixInput fix = new FixInput();
fix.expectMergedAs = mergedAs.name();
assertProblems(
- ctl,
+ notes,
fix,
problem(
"No patch set found for merged commit " + mergedAs.name(),
@@ -585,30 +588,29 @@
FIXED,
"Inserted as patch set 2"));
- ctl = reload(ctl);
- PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2);
- assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId2);
- assertThat(psUtil.get(db, ctl.getNotes(), psId2).getRevision().get())
- .isEqualTo(mergedAs.name());
+ notes = reload(notes);
+ PatchSet.Id psId2 = new PatchSet.Id(notes.getChangeId(), 2);
+ assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId2);
+ assertThat(psUtil.get(db, notes, psId2).getRevision().get()).isEqualTo(mergedAs.name());
- assertNoProblems(ctl, null);
+ assertNoProblems(notes, null);
}
@Test
public void expectedMergedCommitIsOldPatchSetOfSameChange() throws Exception {
- ChangeControl ctl = insertChange();
- PatchSet ps1 = psUtil.current(db, ctl.getNotes());
+ ChangeNotes notes = insertChange();
+ PatchSet ps1 = psUtil.current(db, notes);
String rev1 = ps1.getRevision().get();
- ctl = incrementPatchSet(ctl);
- PatchSet ps2 = psUtil.current(db, ctl.getNotes());
+ notes = incrementPatchSet(notes);
+ PatchSet ps2 = psUtil.current(db, notes);
testRepo
- .branch(ctl.getChange().getDest().get())
+ .branch(notes.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev1)));
FixInput fix = new FixInput();
fix.expectMergedAs = rev1;
assertProblems(
- ctl,
+ notes,
fix,
problem("No patch set found for merged commit " + rev1, FIXED, "Marked change as merged"),
problem(
@@ -626,38 +628,37 @@
FIXED,
"Inserted as patch set 3"));
- ctl = reload(ctl);
- PatchSet.Id psId3 = new PatchSet.Id(ctl.getId(), 3);
- assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId3);
- assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
- assertThat(psUtil.byChangeAsMap(db, ctl.getNotes()).keySet())
- .containsExactly(ps2.getId(), psId3);
- assertThat(psUtil.get(db, ctl.getNotes(), psId3).getRevision().get()).isEqualTo(rev1);
+ notes = reload(notes);
+ PatchSet.Id psId3 = new PatchSet.Id(notes.getChangeId(), 3);
+ assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId3);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
+ assertThat(psUtil.byChangeAsMap(db, notes).keySet()).containsExactly(ps2.getId(), psId3);
+ assertThat(psUtil.get(db, notes, psId3).getRevision().get()).isEqualTo(rev1);
}
@Test
public void expectedMergedCommitIsDanglingPatchSetOlderThanCurrent() throws Exception {
- ChangeControl ctl = insertChange();
- PatchSet ps1 = psUtil.current(db, ctl.getNotes());
+ ChangeNotes notes = insertChange();
+ PatchSet ps1 = psUtil.current(db, notes);
// Create dangling ref so next ID in the database becomes 3.
- PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2);
+ PatchSet.Id psId2 = new PatchSet.Id(notes.getChangeId(), 2);
RevCommit commit2 = patchSetCommit(psId2);
String rev2 = commit2.name();
testRepo.branch(psId2.toRefName()).update(commit2);
- ctl = incrementPatchSet(ctl);
- PatchSet ps3 = psUtil.current(db, ctl.getNotes());
+ notes = incrementPatchSet(notes);
+ PatchSet ps3 = psUtil.current(db, notes);
assertThat(ps3.getId().get()).isEqualTo(3);
testRepo
- .branch(ctl.getChange().getDest().get())
+ .branch(notes.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev2)));
FixInput fix = new FixInput();
fix.expectMergedAs = rev2;
assertProblems(
- ctl,
+ notes,
fix,
problem("No patch set found for merged commit " + rev2, FIXED, "Marked change as merged"),
problem(
@@ -675,34 +676,34 @@
FIXED,
"Inserted as patch set 4"));
- ctl = reload(ctl);
- PatchSet.Id psId4 = new PatchSet.Id(ctl.getId(), 4);
- assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId4);
- assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
- assertThat(psUtil.byChangeAsMap(db, ctl.getNotes()).keySet())
+ notes = reload(notes);
+ PatchSet.Id psId4 = new PatchSet.Id(notes.getChangeId(), 4);
+ assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId4);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
+ assertThat(psUtil.byChangeAsMap(db, notes).keySet())
.containsExactly(ps1.getId(), ps3.getId(), psId4);
- assertThat(psUtil.get(db, ctl.getNotes(), psId4).getRevision().get()).isEqualTo(rev2);
+ assertThat(psUtil.get(db, notes, psId4).getRevision().get()).isEqualTo(rev2);
}
@Test
public void expectedMergedCommitIsDanglingPatchSetNewerThanCurrent() throws Exception {
- ChangeControl ctl = insertChange();
- PatchSet ps1 = psUtil.current(db, ctl.getNotes());
+ ChangeNotes notes = insertChange();
+ PatchSet ps1 = psUtil.current(db, notes);
// Create dangling ref with no patch set.
- PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2);
+ PatchSet.Id psId2 = new PatchSet.Id(notes.getChangeId(), 2);
RevCommit commit2 = patchSetCommit(psId2);
String rev2 = commit2.name();
testRepo.branch(psId2.toRefName()).update(commit2);
testRepo
- .branch(ctl.getChange().getDest().get())
+ .branch(notes.getChange().getDest().get())
.update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev2)));
FixInput fix = new FixInput();
fix.expectMergedAs = rev2;
assertProblems(
- ctl,
+ notes,
fix,
problem("No patch set found for merged commit " + rev2, FIXED, "Marked change as merged"),
problem(
@@ -713,20 +714,19 @@
FIXED,
"Inserted as patch set 2"));
- ctl = reload(ctl);
- assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId2);
- assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
- assertThat(psUtil.byChangeAsMap(db, ctl.getNotes()).keySet())
- .containsExactly(ps1.getId(), psId2);
- assertThat(psUtil.get(db, ctl.getNotes(), psId2).getRevision().get()).isEqualTo(rev2);
+ notes = reload(notes);
+ assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId2);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
+ assertThat(psUtil.byChangeAsMap(db, notes).keySet()).containsExactly(ps1.getId(), psId2);
+ assertThat(psUtil.get(db, notes, psId2).getRevision().get()).isEqualTo(rev2);
}
@Test
public void expectedMergedCommitWithMismatchedChangeId() throws Exception {
- ChangeControl ctl = insertChange();
- String dest = ctl.getChange().getDest().get();
+ ChangeNotes notes = insertChange();
+ String dest = notes.getChange().getDest().get();
RevCommit parent = testRepo.branch(dest).commit().message("parent").create();
- String rev = psUtil.current(db, ctl.getNotes()).getRevision().get();
+ String rev = psUtil.current(db, notes).getRevision().get();
RevCommit commit = testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
testRepo.branch(dest).update(commit);
@@ -741,12 +741,12 @@
assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)).containsExactly(badId);
testRepo.update(dest, mergedAs);
- assertNoProblems(ctl, null);
+ assertNoProblems(notes, null);
FixInput fix = new FixInput();
fix.expectMergedAs = mergedAs.name();
assertProblems(
- ctl,
+ notes,
fix,
problem(
"Expected merged commit "
@@ -754,30 +754,30 @@
+ " has Change-Id: "
+ badId
+ ", but expected "
- + ctl.getChange().getKey().get()));
+ + notes.getChange().getKey().get()));
}
@Test
public void expectedMergedCommitMatchesMultiplePatchSets() throws Exception {
- ChangeControl ctl1 = insertChange();
- PatchSet.Id psId1 = psUtil.current(db, ctl1.getNotes()).getId();
- String dest = ctl1.getChange().getDest().get();
- String rev = psUtil.current(db, ctl1.getNotes()).getRevision().get();
+ ChangeNotes notes1 = insertChange();
+ PatchSet.Id psId1 = psUtil.current(db, notes1).getId();
+ String dest = notes1.getChange().getDest().get();
+ String rev = psUtil.current(db, notes1).getRevision().get();
RevCommit commit = testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
testRepo.branch(dest).update(commit);
- ChangeControl ctl2 = insertChange();
- ctl2 = incrementPatchSet(ctl2, commit);
- PatchSet.Id psId2 = psUtil.current(db, ctl2.getNotes()).getId();
+ ChangeNotes notes2 = insertChange();
+ notes2 = incrementPatchSet(notes2, commit);
+ PatchSet.Id psId2 = psUtil.current(db, notes2).getId();
- ChangeControl ctl3 = insertChange();
- ctl3 = incrementPatchSet(ctl3, commit);
- PatchSet.Id psId3 = psUtil.current(db, ctl3.getNotes()).getId();
+ ChangeNotes notes3 = insertChange();
+ notes3 = incrementPatchSet(notes3, commit);
+ PatchSet.Id psId3 = psUtil.current(db, notes3).getId();
FixInput fix = new FixInput();
fix.expectMergedAs = commit.name();
assertProblems(
- ctl1,
+ notes1,
fix,
problem(
"Multiple patch sets for expected merged commit "
@@ -795,15 +795,15 @@
return batchUpdateFactory.create(db, project, userFactory.create(owner), TimeUtil.nowTs());
}
- private ChangeControl insertChange() throws Exception {
+ private ChangeNotes insertChange() throws Exception {
return insertChange(admin);
}
- private ChangeControl insertChange(TestAccount owner) throws Exception {
+ private ChangeNotes insertChange(TestAccount owner) throws Exception {
return insertChange(owner, "refs/heads/master");
}
- private ChangeControl insertChange(TestAccount owner, String dest) throws Exception {
+ private ChangeNotes insertChange(TestAccount owner, String dest) throws Exception {
Change.Id id = new Change.Id(sequences.nextChangeId());
ChangeInserter ins;
try (BatchUpdate bu = newUpdate(owner.getId())) {
@@ -817,35 +817,34 @@
.setSendMail(false);
bu.insertChange(ins).execute();
}
- // Return control for admin regardless of owner.
- return changeControlFactory.controlFor(db, ins.getChange(), userFactory.create(adminId));
+ return changeNotesFactory.create(db, project, ins.getChange().getId());
}
- private PatchSet.Id nextPatchSetId(ChangeControl ctl) throws Exception {
- return ChangeUtil.nextPatchSetId(testRepo.getRepository(), ctl.getChange().currentPatchSetId());
+ private PatchSet.Id nextPatchSetId(ChangeNotes notes) throws Exception {
+ return ChangeUtil.nextPatchSetId(
+ testRepo.getRepository(), notes.getChange().currentPatchSetId());
}
- private ChangeControl incrementPatchSet(ChangeControl ctl) throws Exception {
- return incrementPatchSet(ctl, patchSetCommit(nextPatchSetId(ctl)));
+ private ChangeNotes incrementPatchSet(ChangeNotes notes) throws Exception {
+ return incrementPatchSet(notes, patchSetCommit(nextPatchSetId(notes)));
}
- private ChangeControl incrementPatchSet(ChangeControl ctl, RevCommit commit) throws Exception {
+ private ChangeNotes incrementPatchSet(ChangeNotes notes, RevCommit commit) throws Exception {
PatchSetInserter ins;
- try (BatchUpdate bu = newUpdate(ctl.getChange().getOwner())) {
+ try (BatchUpdate bu = newUpdate(notes.getChange().getOwner())) {
ins =
patchSetInserterFactory
- .create(ctl.getNotes(), nextPatchSetId(ctl), commit)
+ .create(notes, nextPatchSetId(notes), commit)
.setValidate(false)
.setFireRevisionCreated(false)
.setNotify(NotifyHandling.NONE);
- bu.addOp(ctl.getId(), ins).execute();
+ bu.addOp(notes.getChangeId(), ins).execute();
}
- return reload(ctl);
+ return reload(notes);
}
- private ChangeControl reload(ChangeControl ctl) throws Exception {
- return changeControlFactory.controlFor(
- db, ctl.getChange().getProject(), ctl.getId(), ctl.getUser());
+ private ChangeNotes reload(ChangeNotes notes) throws Exception {
+ return changeNotesFactory.create(db, notes.getChange().getProject(), notes.getChangeId());
}
private RevCommit patchSetCommit(PatchSet.Id psId) throws Exception {
@@ -853,12 +852,12 @@
return testRepo.parseBody(c);
}
- private PatchSet insertMissingPatchSet(ChangeControl ctl, String rev) throws Exception {
+ private PatchSet insertMissingPatchSet(ChangeNotes notes, String rev) throws Exception {
// Don't use BatchUpdate since we're manually updating the meta ref rather
// than using ChangeUpdate.
String subject = "Subject for missing commit";
- Change c = new Change(ctl.getChange());
- PatchSet.Id psId = nextPatchSetId(ctl);
+ Change c = new Change(notes.getChange());
+ PatchSet.Id psId = nextPatchSetId(notes);
c.setCurrentPatchSet(psId, subject, c.getOriginalSubject());
PatchSet ps = newPatchSet(psId, rev, adminId);
@@ -913,19 +912,18 @@
.create();
}
- private ObjectId getDestRef(ChangeControl ctl) throws Exception {
- return testRepo.getRepository().exactRef(ctl.getChange().getDest().get()).getObjectId();
+ private ObjectId getDestRef(ChangeNotes notes) throws Exception {
+ return testRepo.getRepository().exactRef(notes.getChange().getDest().get()).getObjectId();
}
- private ChangeControl mergeChange(ChangeControl ctl) throws Exception {
- final ObjectId oldId = getDestRef(ctl);
- final ObjectId newId =
- ObjectId.fromString(psUtil.current(db, ctl.getNotes()).getRevision().get());
- final String dest = ctl.getChange().getDest().get();
+ private ChangeNotes mergeChange(ChangeNotes notes) throws Exception {
+ final ObjectId oldId = getDestRef(notes);
+ final ObjectId newId = ObjectId.fromString(psUtil.current(db, notes).getRevision().get());
+ final String dest = notes.getChange().getDest().get();
try (BatchUpdate bu = newUpdate(adminId)) {
bu.addOp(
- ctl.getId(),
+ notes.getChangeId(),
new BatchUpdateOp() {
@Override
public void updateRepo(RepoContext ctx) throws IOException {
@@ -941,7 +939,7 @@
});
bu.execute();
}
- return reload(ctl);
+ return reload(notes);
}
private static ProblemInfo problem(String message) {
@@ -958,14 +956,22 @@
}
private void assertProblems(
- ChangeControl ctl, @Nullable FixInput fix, ProblemInfo first, ProblemInfo... rest) {
+ ChangeNotes notes, @Nullable FixInput fix, ProblemInfo first, ProblemInfo... rest)
+ throws Exception {
List<ProblemInfo> expected = new ArrayList<>(1 + rest.length);
expected.add(first);
expected.addAll(Arrays.asList(rest));
- assertThat(checker.check(ctl, fix).problems()).containsExactlyElementsIn(expected).inOrder();
+ assertThat(checker.check(controlForNotes(notes), fix).problems())
+ .containsExactlyElementsIn(expected)
+ .inOrder();
}
- private void assertNoProblems(ChangeControl ctl, @Nullable FixInput fix) {
- assertThat(checker.check(ctl, fix).problems()).isEmpty();
+ private void assertNoProblems(ChangeNotes notes, @Nullable FixInput fix) throws Exception {
+ assertThat(checker.check(controlForNotes(notes), fix).problems()).isEmpty();
+ }
+
+ /** @return {@link ChangeControl} for notes and admin regardless of owner. */
+ private ChangeControl controlForNotes(ChangeNotes notes) throws Exception {
+ return changeControlFactory.controlFor(notes, userFactory.create(admin.id));
}
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
index f08a16e..8aa2497 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
@@ -32,6 +32,7 @@
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.Sandboxed;
+import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
@@ -65,6 +66,7 @@
import org.junit.Test;
@Sandboxed
+@UseLocalDisk
@NoHttpd
public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
private static final String INVALID_STATE = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
@@ -85,12 +87,13 @@
@Inject private Provider<NoteDbMigrator.Builder> migratorBuilderProvider;
@Inject private Sequences sequences;
- private FileBasedConfig gerritConfig;
+ private FileBasedConfig noteDbConfig;
@Before
public void setUp() throws Exception {
assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.OFF);
- gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
+ // Unlike in the running server, for tests, we don't stack notedb.config on gerrit.config.
+ noteDbConfig = new FileBasedConfig(sitePaths.notedb_config.toFile(), FS.detect());
assertNotesMigrationState(REVIEW_DB);
}
@@ -365,27 +368,27 @@
migrate(b -> b.setStopAtStateForTesting(WRITE));
assertNotesMigrationState(WRITE);
- assertThat(NoteDbMigrator.getAutoMigrate(gerritConfig)).isFalse();
+ assertThat(NoteDbMigrator.getAutoMigrate(noteDbConfig)).isFalse();
migrate(b -> b.setAutoMigrate(true).setStopAtStateForTesting(READ_WRITE_NO_SEQUENCE));
assertNotesMigrationState(READ_WRITE_NO_SEQUENCE);
- assertThat(NoteDbMigrator.getAutoMigrate(gerritConfig)).isTrue();
+ assertThat(NoteDbMigrator.getAutoMigrate(noteDbConfig)).isTrue();
migrate(b -> b);
assertNotesMigrationState(NOTE_DB);
- assertThat(NoteDbMigrator.getAutoMigrate(gerritConfig)).isFalse();
+ assertThat(NoteDbMigrator.getAutoMigrate(noteDbConfig)).isFalse();
}
private void assertNotesMigrationState(NotesMigrationState expected) throws Exception {
assertThat(NotesMigrationState.forNotesMigration(notesMigration)).hasValue(expected);
- gerritConfig.load();
- assertThat(NotesMigrationState.forConfig(gerritConfig)).hasValue(expected);
+ noteDbConfig.load();
+ assertThat(NotesMigrationState.forConfig(noteDbConfig)).hasValue(expected);
}
private void setNotesMigrationState(NotesMigrationState state) throws Exception {
- gerritConfig.load();
- state.setConfigValues(gerritConfig);
- gerritConfig.save();
+ noteDbConfig.load();
+ state.setConfigValues(noteDbConfig);
+ noteDbConfig.save();
notesMigration.setFrom(state);
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ProjectApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ProjectApi.java
index 55849cc..734acb1 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ProjectApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ProjectApi.java
@@ -16,6 +16,7 @@
import com.google.gerrit.extensions.api.access.ProjectAccessInfo;
import com.google.gerrit.extensions.api.access.ProjectAccessInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ProjectInfo;
import com.google.gerrit.extensions.restapi.NotImplementedException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -36,6 +37,8 @@
ProjectAccessInfo access(ProjectAccessInput p) throws RestApiException;
+ ChangeInfo accessChange(ProjectAccessInput p) throws RestApiException;
+
ConfigInfo config() throws RestApiException;
ConfigInfo config(ConfigInput in) throws RestApiException;
@@ -163,6 +166,11 @@
}
@Override
+ public ChangeInfo accessChange(ProjectAccessInput input) throws RestApiException {
+ throw new NotImplementedException();
+ }
+
+ @Override
public ConfigInfo config() throws RestApiException {
throw new NotImplementedException();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
index 538c7c2..6971b48 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
@@ -29,9 +29,9 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.git.LabelNormalizer;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
@@ -79,7 +79,8 @@
* Apply approval copy settings from prior PatchSets to a new PatchSet.
*
* @param db review database.
- * @param ctl change control for user uploading PatchSet
+ * @param notes change notes for user uploading PatchSet
+ * @param user user uploading PatchSet
* @param ps new PatchSet
* @param rw open walk that can read the patch set commit; null to open the repo on demand.
* @param repoConfig repo config used for change kind detection; null to read from repo on demand.
@@ -87,19 +88,21 @@
*/
public void copyInReviewDb(
ReviewDb db,
- ChangeControl ctl,
+ ChangeNotes notes,
+ CurrentUser user,
PatchSet ps,
@Nullable RevWalk rw,
@Nullable Config repoConfig)
throws OrmException {
- copyInReviewDb(db, ctl, ps, rw, repoConfig, Collections.emptyList());
+ copyInReviewDb(db, notes, user, ps, rw, repoConfig, Collections.emptyList());
}
/**
* Apply approval copy settings from prior PatchSets to a new PatchSet.
*
* @param db review database.
- * @param ctl change control for user uploading PatchSet
+ * @param notes change notes for user uploading PatchSet
+ * @param user user uploading PatchSet
* @param ps new PatchSet
* @param rw open walk that can read the patch set commit; null to open the repo on demand.
* @param repoConfig repo config used for change kind detection; null to read from repo on demand.
@@ -108,52 +111,57 @@
*/
public void copyInReviewDb(
ReviewDb db,
- ChangeControl ctl,
+ ChangeNotes notes,
+ CurrentUser user,
PatchSet ps,
@Nullable RevWalk rw,
@Nullable Config repoConfig,
Iterable<PatchSetApproval> dontCopy)
throws OrmException {
- if (PrimaryStorage.of(ctl.getChange()) == PrimaryStorage.REVIEW_DB) {
- db.patchSetApprovals().insert(getForPatchSet(db, ctl, ps, rw, repoConfig, dontCopy));
+ if (PrimaryStorage.of(notes.getChange()) == PrimaryStorage.REVIEW_DB) {
+ db.patchSetApprovals().insert(getForPatchSet(db, notes, user, ps, rw, repoConfig, dontCopy));
}
}
Iterable<PatchSetApproval> getForPatchSet(
ReviewDb db,
- ChangeControl ctl,
+ ChangeNotes notes,
+ CurrentUser user,
PatchSet.Id psId,
@Nullable RevWalk rw,
@Nullable Config repoConfig)
throws OrmException {
- return getForPatchSet(db, ctl, psId, rw, repoConfig, Collections.<PatchSetApproval>emptyList());
+ return getForPatchSet(
+ db, notes, user, psId, rw, repoConfig, Collections.<PatchSetApproval>emptyList());
}
Iterable<PatchSetApproval> getForPatchSet(
ReviewDb db,
- ChangeControl ctl,
+ ChangeNotes notes,
+ CurrentUser user,
PatchSet.Id psId,
@Nullable RevWalk rw,
@Nullable Config repoConfig,
Iterable<PatchSetApproval> dontCopy)
throws OrmException {
- PatchSet ps = psUtil.get(db, ctl.getNotes(), psId);
+ PatchSet ps = psUtil.get(db, notes, psId);
if (ps == null) {
return Collections.emptyList();
}
- return getForPatchSet(db, ctl, ps, rw, repoConfig, dontCopy);
+ return getForPatchSet(db, notes, user, ps, rw, repoConfig, dontCopy);
}
private Iterable<PatchSetApproval> getForPatchSet(
ReviewDb db,
- ChangeControl ctl,
+ ChangeNotes notes,
+ CurrentUser user,
PatchSet ps,
@Nullable RevWalk rw,
@Nullable Config repoConfig,
Iterable<PatchSetApproval> dontCopy)
throws OrmException {
checkNotNull(ps, "ps should not be null");
- ChangeData cd = changeDataFactory.create(db, ctl);
+ ChangeData cd = changeDataFactory.create(db, notes);
try {
ProjectState project = projectCache.checkedGet(cd.change().getDest().getParentKey());
ListMultimap<PatchSet.Id, PatchSetApproval> all = cd.approvals();
@@ -204,7 +212,7 @@
byUser.put(psa.getLabel(), psa.getAccountId(), copy(psa, ps.getId()));
}
}
- return labelNormalizer.normalize(ctl, byUser.values()).getNormalized();
+ return labelNormalizer.normalize(notes, user, byUser.values()).getNormalized();
} catch (IOException | PermissionBackendException e) {
throw new OrmException(e);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
index d858da5..31107c9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -49,7 +49,6 @@
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.util.LabelVote;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -385,7 +384,8 @@
public Iterable<PatchSetApproval> byPatchSet(
ReviewDb db,
- ChangeControl ctl,
+ ChangeNotes notes,
+ CurrentUser user,
PatchSet.Id psId,
@Nullable RevWalk rw,
@Nullable Config repoConfig)
@@ -393,12 +393,13 @@
if (!migration.readChanges()) {
return sortApprovals(db.patchSetApprovals().byPatchSet(psId));
}
- return copier.getForPatchSet(db, ctl, psId, rw, repoConfig);
+ return copier.getForPatchSet(db, notes, user, psId, rw, repoConfig);
}
public Iterable<PatchSetApproval> byPatchSetUser(
ReviewDb db,
- ChangeControl ctl,
+ ChangeNotes notes,
+ CurrentUser user,
PatchSet.Id psId,
Account.Id accountId,
@Nullable RevWalk rw,
@@ -407,7 +408,7 @@
if (!migration.readChanges()) {
return sortApprovals(db.patchSetApprovals().byPatchSetUser(psId, accountId));
}
- return filterApprovals(byPatchSet(db, ctl, psId, rw, repoConfig), accountId);
+ return filterApprovals(byPatchSet(db, notes, user, psId, rw, repoConfig), accountId);
}
public PatchSetApproval getSubmitter(ReviewDb db, ChangeNotes notes, PatchSet.Id c) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java
index b13c43b..75dfe6a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java
@@ -25,7 +25,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.change.ChangeTriplet;
-import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
@@ -59,7 +59,7 @@
private final Cache<Change.Id, String> changeIdProjectCache;
private final Provider<InternalChangeQuery> queryProvider;
private final Provider<ReviewDb> reviewDb;
- private final ChangeControl.GenericFactory changeControlFactory;
+ private final ChangeNotes.Factory changeNotesFactory;
@Inject
ChangeFinder(
@@ -67,24 +67,22 @@
@Named(CACHE_NAME) Cache<Change.Id, String> changeIdProjectCache,
Provider<InternalChangeQuery> queryProvider,
Provider<ReviewDb> reviewDb,
- ChangeControl.GenericFactory changeControlFactory) {
+ ChangeNotes.Factory changeNotesFactory) {
this.indexConfig = indexConfig;
this.changeIdProjectCache = changeIdProjectCache;
this.queryProvider = queryProvider;
this.reviewDb = reviewDb;
- this.changeControlFactory = changeControlFactory;
+ this.changeNotesFactory = changeNotesFactory;
}
/**
* Find changes matching the given identifier.
*
* @param id change identifier, either a numeric ID, a Change-Id, or project~branch~id triplet.
- * @param user user to wrap in controls.
- * @return possibly-empty list of controls for all matching changes, corresponding to the given
- * user; may or may not be visible.
+ * @return possibly-empty list of notes for all matching changes; may or may not be visible.
* @throws OrmException if an error occurred querying the database.
*/
- public List<ChangeControl> find(String id, CurrentUser user) throws OrmException {
+ public List<ChangeNotes> find(String id) throws OrmException {
if (id.isEmpty()) {
return Collections.emptyList();
}
@@ -95,7 +93,7 @@
// Try project~numericChangeId
Integer n = Ints.tryParse(id.substring(z + 1));
if (n != null) {
- return fromProjectNumber(user, id.substring(0, z), n.intValue());
+ return fromProjectNumber(id.substring(0, z), n.intValue());
}
}
@@ -103,7 +101,7 @@
// Try numeric changeId
Integer n = Ints.tryParse(id);
if (n != null) {
- return find(new Change.Id(n), user);
+ return find(new Change.Id(n));
}
}
@@ -115,33 +113,22 @@
Optional<ChangeTriplet> triplet = ChangeTriplet.parse(id, y, z);
if (triplet.isPresent()) {
ChangeTriplet t = triplet.get();
- return asChangeControls(query.byBranchKey(t.branch(), t.id()), user);
+ return asChangeNotes(query.byBranchKey(t.branch(), t.id()));
}
}
// Try isolated Ihash... format ("Change-Id: Ihash").
- return asChangeControls(query.byKeyPrefix(id), user);
+ return asChangeNotes(query.byKeyPrefix(id));
}
- private List<ChangeControl> fromProjectNumber(CurrentUser user, String project, int changeNumber)
+ private List<ChangeNotes> fromProjectNumber(String project, int changeNumber)
throws OrmException {
Change.Id cId = new Change.Id(changeNumber);
try {
return ImmutableList.of(
- changeControlFactory.controlFor(
- reviewDb.get(), Project.NameKey.parse(project), cId, user));
+ changeNotesFactory.createChecked(reviewDb.get(), Project.NameKey.parse(project), cId));
} catch (NoSuchChangeException e) {
return Collections.emptyList();
- } catch (IllegalArgumentException e) {
- String changeNotFound = String.format("change %s not found in ReviewDb", cId);
- String projectNotFound =
- String.format(
- "passed project %s when creating ChangeNotes for %s, but actual project is",
- project, cId);
- if (e.getMessage().equals(changeNotFound) || e.getMessage().startsWith(projectNotFound)) {
- return Collections.emptyList();
- }
- throw e;
} catch (OrmException e) {
// Distinguish between a RepositoryNotFoundException (project argument invalid) and
// other OrmExceptions (failure in the persistence layer).
@@ -152,18 +139,18 @@
}
}
- public ChangeControl findOne(Change.Id id, CurrentUser user) throws OrmException {
- List<ChangeControl> ctls = find(id, user);
- if (ctls.size() != 1) {
+ public ChangeNotes findOne(Change.Id id) throws OrmException {
+ List<ChangeNotes> notes = find(id);
+ if (notes.size() != 1) {
throw new NoSuchChangeException(id);
}
- return ctls.get(0);
+ return notes.get(0);
}
- public List<ChangeControl> find(Change.Id id, CurrentUser user) throws OrmException {
+ public List<ChangeNotes> find(Change.Id id) throws OrmException {
String project = changeIdProjectCache.getIfPresent(id);
if (project != null) {
- return fromProjectNumber(user, project, id.get());
+ return fromProjectNumber(project, id.get());
}
// Use the index to search for changes, but don't return any stored fields,
@@ -173,17 +160,16 @@
if (r.size() == 1) {
changeIdProjectCache.put(id, r.get(0).project().get());
}
- return asChangeControls(r, user);
+ return asChangeNotes(r);
}
- private List<ChangeControl> asChangeControls(List<ChangeData> cds, CurrentUser user)
- throws OrmException {
- List<ChangeControl> ctls = new ArrayList<>(cds.size());
+ private List<ChangeNotes> asChangeNotes(List<ChangeData> cds) throws OrmException {
+ List<ChangeNotes> notes = new ArrayList<>(cds.size());
if (!indexConfig.separateChangeSubIndexes()) {
for (ChangeData cd : cds) {
- checkedAdd(cd, ctls, user);
+ notes.add(cd.notes());
}
- return ctls;
+ return notes;
}
// If an index implementation uses separate non-atomic subindexes, it's possible to temporarily
@@ -195,18 +181,9 @@
Set<Change.Id> seen = Sets.newHashSetWithExpectedSize(cds.size());
for (ChangeData cd : cds) {
if (seen.add(cd.getId())) {
- checkedAdd(cd, ctls, user);
+ notes.add(cd.notes());
}
}
- return ctls;
- }
-
- private static void checkedAdd(ChangeData cd, List<ChangeControl> ctls, CurrentUser user)
- throws OrmException {
- try {
- ctls.add(cd.changeControl(user));
- } catch (NoSuchChangeException e) {
- // Ignore
- }
+ return notes;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
index c9b726b..efd26de 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
@@ -42,6 +42,10 @@
public static final Ordering<PatchSet> PS_ID_ORDER =
Ordering.from(comparingInt(PatchSet::getPatchSetId));
+ public static String formatChangeUrl(String canonicalWebUrl, Change change) {
+ return canonicalWebUrl + "#/c/" + change.getProject().get() + "/+/" + change.getChangeId();
+ }
+
/** @return a new unique identifier for change message entities. */
public static String messageUuid() {
byte[] buf = new byte[8];
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java
index a9fe235..6509247 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java
@@ -31,6 +31,7 @@
import com.google.gerrit.extensions.api.projects.ProjectInput;
import com.google.gerrit.extensions.api.projects.TagApi;
import com.google.gerrit.extensions.api.projects.TagInfo;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ProjectInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
@@ -43,6 +44,7 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ChildProjectsCollection;
import com.google.gerrit.server.project.CommitsCollection;
+import com.google.gerrit.server.project.CreateAccessChange;
import com.google.gerrit.server.project.CreateProject;
import com.google.gerrit.server.project.DeleteBranches;
import com.google.gerrit.server.project.DeleteTags;
@@ -85,6 +87,7 @@
private final TagApiImpl.Factory tagApi;
private final GetAccess getAccess;
private final SetAccess setAccess;
+ private final CreateAccessChange createAccessChange;
private final GetConfig getConfig;
private final PutConfig putConfig;
private final ListBranches listBranches;
@@ -110,6 +113,7 @@
TagApiImpl.Factory tagApiFactory,
GetAccess getAccess,
SetAccess setAccess,
+ CreateAccessChange createAccessChange,
GetConfig getConfig,
PutConfig putConfig,
ListBranches listBranches,
@@ -134,6 +138,7 @@
tagApiFactory,
getAccess,
setAccess,
+ createAccessChange,
getConfig,
putConfig,
listBranches,
@@ -162,6 +167,7 @@
TagApiImpl.Factory tagApiFactory,
GetAccess getAccess,
SetAccess setAccess,
+ CreateAccessChange createAccessChange,
GetConfig getConfig,
PutConfig putConfig,
ListBranches listBranches,
@@ -186,6 +192,7 @@
tagApiFactory,
getAccess,
setAccess,
+ createAccessChange,
getConfig,
putConfig,
listBranches,
@@ -213,6 +220,7 @@
TagApiImpl.Factory tagApiFactory,
GetAccess getAccess,
SetAccess setAccess,
+ CreateAccessChange createAccessChange,
GetConfig getConfig,
PutConfig putConfig,
ListBranches listBranches,
@@ -247,6 +255,7 @@
this.deleteTags = deleteTags;
this.commitsCollection = commitsCollection;
this.commitApi = commitApi;
+ this.createAccessChange = createAccessChange;
}
@Override
@@ -304,6 +313,15 @@
}
@Override
+ public ChangeInfo accessChange(ProjectAccessInput p) throws RestApiException {
+ try {
+ return createAccessChange.apply(checkExists(), p).value();
+ } catch (Exception e) {
+ throw asRestApiException("Cannot put access right change", e);
+ }
+ }
+
+ @Override
public void description(DescriptionInput in) throws RestApiException {
try {
putDescription.apply(checkExists(), in);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java
index cd0f34e..db13c14 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java
@@ -35,7 +35,7 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
@@ -138,31 +138,31 @@
* should use the batch instead of abandoning one by one.
*
* <p>It's the caller's responsibility to ensure that all jobs inside the same batch have the
- * matching project from its ChangeControl. Violations will result in a ResourceConflictException.
+ * matching project from its ChangeData. Violations will result in a ResourceConflictException.
*/
public void batchAbandon(
BatchUpdate.Factory updateFactory,
Project.NameKey project,
CurrentUser user,
- Collection<ChangeControl> controls,
+ Collection<ChangeData> changes,
String msgTxt,
NotifyHandling notifyHandling,
ListMultimap<RecipientType, Account.Id> accountsToNotify)
throws RestApiException, UpdateException {
- if (controls.isEmpty()) {
+ if (changes.isEmpty()) {
return;
}
Account account = user.isIdentifiedUser() ? user.asIdentifiedUser().getAccount() : null;
try (BatchUpdate u = updateFactory.create(dbProvider.get(), project, user, TimeUtil.nowTs())) {
- for (ChangeControl control : controls) {
- if (!project.equals(control.getProject().getNameKey())) {
+ for (ChangeData change : changes) {
+ if (!project.equals(change.project())) {
throw new ResourceConflictException(
String.format(
"Project name \"%s\" doesn't match \"%s\"",
- control.getProject().getNameKey().get(), project.get()));
+ change.project().get(), project.get()));
}
u.addOp(
- control.getId(),
+ change.getId(),
abandonOpFactory.create(account, msgTxt, notifyHandling, accountsToNotify));
}
u.execute();
@@ -173,14 +173,14 @@
BatchUpdate.Factory updateFactory,
Project.NameKey project,
CurrentUser user,
- Collection<ChangeControl> controls,
+ Collection<ChangeData> changes,
String msgTxt)
throws RestApiException, UpdateException {
batchAbandon(
updateFactory,
project,
user,
- controls,
+ changes,
msgTxt,
NotifyHandling.ALL,
ImmutableListMultimap.of());
@@ -190,10 +190,10 @@
BatchUpdate.Factory updateFactory,
Project.NameKey project,
CurrentUser user,
- Collection<ChangeControl> controls)
+ Collection<ChangeData> changes)
throws RestApiException, UpdateException {
batchAbandon(
- updateFactory, project, user, controls, "", NotifyHandling.ALL, ImmutableListMultimap.of());
+ updateFactory, project, user, changes, "", NotifyHandling.ALL, ImmutableListMultimap.of());
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java
index 58a908d..3239813 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java
@@ -20,7 +20,6 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.config.ChangeCleanupConfig;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ChangeQueryProcessor;
@@ -74,24 +73,23 @@
List<ChangeData> changesToAbandon =
queryProvider.get().enforceVisibility(false).query(queryBuilder.parse(query)).entities();
- ImmutableListMultimap.Builder<Project.NameKey, ChangeControl> builder =
+ ImmutableListMultimap.Builder<Project.NameKey, ChangeData> builder =
ImmutableListMultimap.builder();
for (ChangeData cd : changesToAbandon) {
- ChangeControl control = cd.changeControl(internalUser);
- builder.put(control.getProject().getNameKey(), control);
+ builder.put(cd.project(), cd);
}
int count = 0;
- ListMultimap<Project.NameKey, ChangeControl> abandons = builder.build();
+ ListMultimap<Project.NameKey, ChangeData> abandons = builder.build();
String message = cfg.getAbandonMessage();
for (Project.NameKey project : abandons.keySet()) {
- Collection<ChangeControl> changes = getValidChanges(abandons.get(project), query);
+ Collection<ChangeData> changes = getValidChanges(abandons.get(project), query);
try {
abandon.batchAbandon(updateFactory, project, internalUser, changes, message);
count += changes.size();
} catch (Throwable e) {
StringBuilder msg = new StringBuilder("Failed to auto-abandon inactive change(s):");
- for (ChangeControl change : changes) {
+ for (ChangeData change : changes) {
msg.append(" ").append(change.getId().get());
}
msg.append(".");
@@ -104,12 +102,11 @@
}
}
- private Collection<ChangeControl> getValidChanges(
- Collection<ChangeControl> changeControls, String query)
+ private Collection<ChangeData> getValidChanges(Collection<ChangeData> changes, String query)
throws OrmException, QueryParseException {
- Collection<ChangeControl> validChanges = new ArrayList<>();
- for (ChangeControl cc : changeControls) {
- String newQuery = query + " change:" + cc.getId();
+ Collection<ChangeData> validChanges = new ArrayList<>();
+ for (ChangeData cd : changes) {
+ String newQuery = query + " change:" + cd.getId();
List<ChangeData> changesToAbandon =
queryProvider
.get()
@@ -117,12 +114,12 @@
.query(queryBuilder.parse(newQuery))
.entities();
if (!changesToAbandon.isEmpty()) {
- validChanges.add(cc);
+ validChanges.add(cd);
} else {
log.debug(
"Change data with id \"{}\" does not satisfy the query \"{}\""
+ " any more, hence skipping it in clean up",
- cc.getId(),
+ cd.getId(),
query);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 0a44b34..c2b93a6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -714,7 +714,6 @@
private Map<String, LabelWithStatus> initLabels(
ChangeData cd, LabelTypes labelTypes, boolean standard) throws OrmException {
- // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
Map<String, LabelWithStatus> labels = new TreeMap<>(labelTypes.nameComparator());
for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels == null) {
@@ -1081,11 +1080,16 @@
private Map<String, Short> currentLabels(PermissionBackend.ForChange perm, ChangeData cd)
throws OrmException {
IdentifiedUser user = perm.user().asIdentifiedUser();
- ChangeControl ctl = cd.changeControl().forUser(user);
Map<String, Short> result = new HashMap<>();
for (PatchSetApproval psa :
approvalsUtil.byPatchSetUser(
- db.get(), ctl, cd.change().currentPatchSetId(), user.getAccountId(), null, null)) {
+ db.get(),
+ cd.notes(),
+ user,
+ cd.change().currentPatchSetId(),
+ user.getAccountId(),
+ null,
+ null)) {
result.put(psa.getLabel(), psa.getValue());
}
return result;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
index d967ab8..3556b42 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
@@ -26,10 +26,12 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeFinder;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.QueryChanges;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -48,6 +50,7 @@
private final CreateChange createChange;
private final ChangeResource.Factory changeResourceFactory;
private final PermissionBackend permissionBackend;
+ private final ChangeControl.GenericFactory changeControlFactory;
@Inject
ChangesCollection(
@@ -58,7 +61,8 @@
ChangeFinder changeFinder,
CreateChange createChange,
ChangeResource.Factory changeResourceFactory,
- PermissionBackend permissionBackend) {
+ PermissionBackend permissionBackend,
+ ChangeControl.GenericFactory changeControlFactory) {
this.db = db;
this.user = user;
this.queryFactory = queryFactory;
@@ -67,6 +71,7 @@
this.createChange = createChange;
this.changeResourceFactory = changeResourceFactory;
this.permissionBackend = permissionBackend;
+ this.changeControlFactory = changeControlFactory;
}
@Override
@@ -82,34 +87,34 @@
@Override
public ChangeResource parse(TopLevelResource root, IdString id)
throws ResourceNotFoundException, OrmException, PermissionBackendException {
- List<ChangeControl> ctls = changeFinder.find(id.encoded(), user.get());
- if (ctls.isEmpty()) {
+ List<ChangeNotes> notes = changeFinder.find(id.encoded());
+ if (notes.isEmpty()) {
throw new ResourceNotFoundException(id);
- } else if (ctls.size() != 1) {
+ } else if (notes.size() != 1) {
throw new ResourceNotFoundException("Multiple changes found for " + id);
}
- ChangeControl ctl = ctls.get(0);
- if (!canRead(ctl)) {
+ ChangeNotes change = notes.get(0);
+ if (!canRead(change)) {
throw new ResourceNotFoundException(id);
}
- return changeResourceFactory.create(ctl);
+ return changeResourceFactory.create(controlFor(change));
}
public ChangeResource parse(Change.Id id)
throws ResourceNotFoundException, OrmException, PermissionBackendException {
- List<ChangeControl> ctls = changeFinder.find(id, user.get());
- if (ctls.isEmpty()) {
+ List<ChangeNotes> notes = changeFinder.find(id);
+ if (notes.isEmpty()) {
throw new ResourceNotFoundException(toIdString(id));
- } else if (ctls.size() != 1) {
+ } else if (notes.size() != 1) {
throw new ResourceNotFoundException("Multiple changes found for " + id);
}
- ChangeControl ctl = ctls.get(0);
- if (!canRead(ctl)) {
+ ChangeNotes change = notes.get(0);
+ if (!canRead(change)) {
throw new ResourceNotFoundException(toIdString(id));
}
- return changeResourceFactory.create(ctl);
+ return changeResourceFactory.create(controlFor(change));
}
private static IdString toIdString(Change.Id id) {
@@ -126,11 +131,16 @@
return createChange;
}
- private boolean canRead(ChangeControl ctl) throws PermissionBackendException {
- return permissionBackend
- .user(user)
- .change(ctl.getNotes())
- .database(db)
- .test(ChangePermission.READ);
+ private boolean canRead(ChangeNotes notes) throws PermissionBackendException {
+ return permissionBackend.user(user).change(notes).database(db).test(ChangePermission.READ);
+ }
+
+ private ChangeControl controlFor(ChangeNotes notes) throws ResourceNotFoundException {
+ try {
+ return changeControlFactory.controlFor(notes, user.get());
+ } catch (NoSuchChangeException e) {
+ throw new ResourceNotFoundException(
+ String.format("Change %s not found", notes.getChangeId()));
+ }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
index 3cdab63..da2f2fd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
@@ -52,11 +52,11 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.CommitsCollection;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectControl;
@@ -194,19 +194,15 @@
ObjectId parentCommit;
List<String> groups;
if (input.baseChange != null) {
- List<ChangeControl> ctls = changeFinder.find(input.baseChange, rsrc.getUser());
- if (ctls.size() != 1) {
+ List<ChangeNotes> notes = changeFinder.find(input.baseChange);
+ if (notes.size() != 1) {
throw new UnprocessableEntityException("Base change not found: " + input.baseChange);
}
- ChangeControl ctl = Iterables.getOnlyElement(ctls);
- if (!permissionBackend
- .user(user)
- .change(ctl.getNotes())
- .database(db)
- .test(ChangePermission.READ)) {
+ ChangeNotes change = Iterables.getOnlyElement(notes);
+ if (!permissionBackend.user(user).change(change).database(db).test(ChangePermission.READ)) {
throw new UnprocessableEntityException("Base change not found: " + input.baseChange);
}
- PatchSet ps = psUtil.current(db.get(), ctl.getNotes());
+ PatchSet ps = psUtil.current(db.get(), change);
parentCommit = ObjectId.fromString(ps.getRevision().get());
groups = ps.getGroups();
} else {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
index c725089..10164ce 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
@@ -41,7 +41,6 @@
import com.google.gerrit.server.mail.send.DeleteVoteSender;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RemoveReviewerControl;
@@ -164,10 +163,9 @@
public boolean updateChange(ChangeContext ctx)
throws OrmException, AuthException, ResourceNotFoundException, IOException,
PermissionBackendException {
- ChangeControl ctl = ctx.getControl();
- change = ctl.getChange();
+ change = ctx.getChange();
PatchSet.Id psId = change.currentPatchSetId();
- ps = psUtil.current(db.get(), ctl.getNotes());
+ ps = psUtil.current(db.get(), ctx.getNotes());
boolean found = false;
LabelTypes labelTypes = projectState.getLabelTypes(ctx.getNotes(), ctx.getUser());
@@ -175,7 +173,8 @@
for (PatchSetApproval a :
approvalsUtil.byPatchSetUser(
ctx.getDb(),
- ctl,
+ ctx.getNotes(),
+ ctx.getUser(),
psId,
account.getId(),
ctx.getRevWalk(),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index 77f4e5c..05f10b5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -49,7 +49,6 @@
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -228,7 +227,6 @@
public boolean updateChange(ChangeContext ctx)
throws ResourceConflictException, OrmException, IOException {
ReviewDb db = ctx.getDb();
- ChangeControl ctl = ctx.getControl();
change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(psId);
@@ -261,7 +259,7 @@
description);
if (notify != NotifyHandling.NONE) {
- oldReviewers = approvalsUtil.getReviewers(db, ctl.getNotes());
+ oldReviewers = approvalsUtil.getReviewers(db, ctx.getNotes());
}
if (message != null) {
@@ -283,7 +281,12 @@
change.setCurrentPatchSet(patchSetInfo);
if (copyApprovals) {
approvalCopier.copyInReviewDb(
- db, ctl, patchSet, ctx.getRevWalk(), ctx.getRepoView().getConfig());
+ db,
+ ctx.getNotes(),
+ ctx.getUser(),
+ patchSet,
+ ctx.getRevWalk(),
+ ctx.getRepoView().getConfig());
}
if (changeMessage != null) {
cmUtil.addChangeMessage(db, update, changeMessage);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 8c4dd04..ca62719 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -1311,7 +1311,8 @@
for (PatchSetApproval a :
approvalsUtil.byPatchSetUser(
ctx.getDb(),
- ctx.getControl(),
+ ctx.getNotes(),
+ ctx.getUser(),
psId,
user.getAccountId(),
ctx.getRevWalk(),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
index c71c050..60ec582 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
@@ -111,7 +111,7 @@
perm,
cd,
approvalsUtil.byPatchSetUser(
- db.get(), ctl, psId, new Account.Id(out._accountId), null, null));
+ db.get(), cd.notes(), ctl.getUser(), psId, new Account.Id(out._accountId), null, null));
}
public ReviewerInfo format(
@@ -122,7 +122,6 @@
throws OrmException, PermissionBackendException {
LabelTypes labelTypes = cd.getLabelTypes();
- // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
out.approvals = new TreeMap<>(labelTypes.nameComparator());
for (PatchSetApproval ca : approvals) {
LabelType at = labelTypes.byLabel(ca.getLabelId());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Votes.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Votes.java
index 980e6e5..c2631d5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Votes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Votes.java
@@ -84,7 +84,8 @@
Iterable<PatchSetApproval> byPatchSetUser =
approvalsUtil.byPatchSetUser(
db.get(),
- rsrc.getChangeResource().getControl(),
+ rsrc.getChangeResource().getNotes(),
+ rsrc.getChangeResource().getUser(),
rsrc.getChange().currentPatchSetId(),
rsrc.getReviewerUser().getAccountId(),
null,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java
index fdc3b7f..0715f8e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java
@@ -18,10 +18,12 @@
import org.eclipse.jgit.lib.Config;
class GerritConfig extends Config {
+ private final Config gerritConfig;
private final SecureStore secureStore;
- GerritConfig(Config baseConfig, SecureStore secureStore) {
- super(baseConfig);
+ GerritConfig(Config noteDbConfigOverGerritConfig, Config gerritConfig, SecureStore secureStore) {
+ super(noteDbConfigOverGerritConfig);
+ this.gerritConfig = gerritConfig;
this.secureStore = secureStore;
}
@@ -42,4 +44,11 @@
}
return super.getStringList(section, subsection, name);
}
+
+ @Override
+ public String toText() {
+ // Only show the contents of gerrit.config, hiding the implementation detail that some values
+ // may come from secure.config (or another secure store) and notedb.config.
+ return gerritConfig.toText();
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigProvider.java
index 494b63a..1a59c52 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigProvider.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigProvider.java
@@ -14,11 +14,18 @@
package com.google.gerrit.server.config;
+import static java.util.stream.Collectors.joining;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.securestore.SecureStore;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.ProvisionException;
import java.io.IOException;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.storage.file.FileBasedConfig;
@@ -41,19 +48,46 @@
@Override
public Config get() {
- FileBasedConfig cfg = new FileBasedConfig(site.gerrit_config.toFile(), FS.DETECTED);
-
- if (!cfg.getFile().exists()) {
+ FileBasedConfig baseConfig = loadConfig(null, site.gerrit_config);
+ if (!baseConfig.getFile().exists()) {
log.info("No " + site.gerrit_config.toAbsolutePath() + "; assuming defaults");
- return new GerritConfig(cfg, secureStore);
}
+ FileBasedConfig noteDbConfigOverBaseConfig = loadConfig(baseConfig, site.notedb_config);
+ checkNoteDbConfig(noteDbConfigOverBaseConfig);
+
+ return new GerritConfig(noteDbConfigOverBaseConfig, baseConfig, secureStore);
+ }
+
+ private static FileBasedConfig loadConfig(@Nullable Config base, Path path) {
+ FileBasedConfig cfg = new FileBasedConfig(base, path.toFile(), FS.DETECTED);
try {
cfg.load();
} catch (IOException | ConfigInvalidException e) {
throw new ProvisionException(e.getMessage(), e);
}
+ return cfg;
+ }
- return new GerritConfig(cfg, secureStore);
+ private static void checkNoteDbConfig(FileBasedConfig noteDbConfig) {
+ List<String> bad = new ArrayList<>();
+ for (String section : noteDbConfig.getSections()) {
+ if (section.equals(NotesMigration.SECTION_NOTE_DB)) {
+ continue;
+ }
+ for (String subsection : noteDbConfig.getSubsections(section)) {
+ noteDbConfig
+ .getNames(section, subsection, false)
+ .forEach(n -> bad.add(section + "." + subsection + "." + n));
+ }
+ noteDbConfig.getNames(section, false).forEach(n -> bad.add(section + "." + n));
+ }
+ if (!bad.isEmpty()) {
+ throw new ProvisionException(
+ "Non-NoteDb config options not allowed in "
+ + noteDbConfig.getFile()
+ + ":\n"
+ + bad.stream().collect(joining("\n")));
+ }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java
index c27f5b9..3748bfd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java
@@ -53,6 +53,7 @@
public final Path gerrit_config;
public final Path secure_config;
+ public final Path notedb_config;
public final Path ssl_keystore;
public final Path ssh_key;
@@ -100,6 +101,7 @@
gerrit_config = etc_dir.resolve("gerrit.config");
secure_config = etc_dir.resolve("secure.config");
+ notedb_config = etc_dir.resolve("notedb.config");
ssl_keystore = etc_dir.resolve("keystore");
ssh_key = etc_dir.resolve("ssh_host_key");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeReportFormatter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeReportFormatter.java
new file mode 100644
index 0000000..6b1fd85
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeReportFormatter.java
@@ -0,0 +1,87 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.git;
+
+import com.google.gerrit.reviewdb.client.Change;
+
+public interface ChangeReportFormatter {
+ public static class Input {
+ private final Change change;
+ private String subject;
+ private Boolean draft;
+ private Boolean edit;
+ private Boolean isPrivate;
+ private Boolean wip;
+
+ public Input(Change change) {
+ this.change = change;
+ }
+
+ public Input setPrivate(boolean isPrivate) {
+ this.isPrivate = isPrivate;
+ return this;
+ }
+
+ public Input setDraft(boolean draft) {
+ this.draft = draft;
+ return this;
+ }
+
+ public Input setEdit(boolean edit) {
+ this.edit = edit;
+ return this;
+ }
+
+ public Input setWorkInProgress(boolean wip) {
+ this.wip = wip;
+ return this;
+ }
+
+ public Input setSubject(String subject) {
+ this.subject = subject;
+ return this;
+ }
+
+ public Change getChange() {
+ return change;
+ }
+
+ public String getSubject() {
+ return subject == null ? change.getSubject() : subject;
+ }
+
+ public boolean isDraft() {
+ return draft == null ? Change.Status.DRAFT == change.getStatus() : draft;
+ }
+
+ public boolean isEdit() {
+ return edit == null ? false : edit;
+ }
+
+ public boolean isPrivate() {
+ return isPrivate == null ? change.isPrivate() : isPrivate;
+ }
+
+ public boolean isWorkInProgress() {
+ return wip == null ? change.isWorkInProgress() : wip;
+ }
+ }
+
+ String newChange(Input input);
+
+ String changeUpdated(Input input);
+
+ String changeClosed(Input input);
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java
new file mode 100644
index 0000000..297770c
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java
@@ -0,0 +1,66 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.git;
+
+import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.config.CanonicalWebUrl;
+import com.google.inject.Inject;
+
+public class DefaultChangeReportFormatter implements ChangeReportFormatter {
+ private final String canonicalWebUrl;
+
+ @Inject
+ DefaultChangeReportFormatter(@CanonicalWebUrl String canonicalWebUrl) {
+ this.canonicalWebUrl = canonicalWebUrl;
+ }
+
+ @Override
+ public String newChange(ChangeReportFormatter.Input input) {
+ return formatChangeUrl(canonicalWebUrl, input);
+ }
+
+ @Override
+ public String changeUpdated(ChangeReportFormatter.Input input) {
+ return formatChangeUrl(canonicalWebUrl, input);
+ }
+
+ @Override
+ public String changeClosed(ChangeReportFormatter.Input input) {
+ return String.format(
+ "change %s closed", ChangeUtil.formatChangeUrl(canonicalWebUrl, input.getChange()));
+ }
+
+ private String formatChangeUrl(String url, Input input) {
+ StringBuilder m =
+ new StringBuilder()
+ .append(" ")
+ .append(ChangeUtil.formatChangeUrl(url, input.getChange()))
+ .append(" ")
+ .append(ChangeUtil.cropSubject(input.getSubject()));
+ if (input.isDraft()) {
+ m.append(" [DRAFT]");
+ }
+ if (input.isEdit()) {
+ m.append(" [EDIT]");
+ }
+ if (input.isPrivate()) {
+ m.append(" [PRIVATE]");
+ }
+ if (input.isWorkInProgress()) {
+ m.append(" [WIP]");
+ }
+ return m.toString();
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/GitModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/GitModule.java
index 15a9d74..92514da 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/GitModule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/GitModule.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.git;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicSet;
import org.eclipse.jgit.transport.PostUploadHook;
@@ -26,5 +27,7 @@
factory(MetaDataUpdate.InternalFactory.class);
bind(MetaDataUpdate.Server.class);
DynamicSet.bind(binder(), PostUploadHook.class).to(UploadPackMetricsHook.class);
+ DynamicItem.itemOf(binder(), ChangeReportFormatter.class);
+ DynamicItem.bind(binder(), ChangeReportFormatter.class).to(DefaultChangeReportFormatter.class);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
index 2eeed24..6a332cd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
@@ -28,16 +28,18 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.io.IOException;
import java.util.Collection;
import java.util.List;
@@ -76,57 +78,59 @@
}
private final Provider<ReviewDb> db;
- private final ChangeControl.GenericFactory changeFactory;
private final IdentifiedUser.GenericFactory userFactory;
private final PermissionBackend permissionBackend;
+ private final ProjectCache projectCache;
@Inject
LabelNormalizer(
Provider<ReviewDb> db,
- ChangeControl.GenericFactory changeFactory,
IdentifiedUser.GenericFactory userFactory,
- PermissionBackend permissionBackend) {
+ PermissionBackend permissionBackend,
+ ProjectCache projectCache) {
this.db = db;
- this.changeFactory = changeFactory;
this.userFactory = userFactory;
this.permissionBackend = permissionBackend;
+ this.projectCache = projectCache;
}
/**
- * @param change change containing the given approvals.
+ * @param notes change containing the given approvals.
* @param approvals list of approvals.
* @return copies of approvals normalized to the defined ranges for the label type and permissions
* for the user. Approvals for unknown labels are not included in the output, nor are
* approvals where the user has no permissions for that label.
* @throws OrmException
*/
- public Result normalize(Change change, Collection<PatchSetApproval> approvals)
- throws OrmException, PermissionBackendException {
- IdentifiedUser user = userFactory.create(change.getOwner());
- return normalize(changeFactory.controlFor(db.get(), change, user), approvals);
+ public Result normalize(ChangeNotes notes, Collection<PatchSetApproval> approvals)
+ throws OrmException, PermissionBackendException, IOException {
+ IdentifiedUser user = userFactory.create(notes.getChange().getOwner());
+ return normalize(notes, user, approvals);
}
/**
- * @param ctl change control containing the given approvals.
+ * @param notes change notes containing the given approvals.
+ * @param user current user.
* @param approvals list of approvals.
* @return copies of approvals normalized to the defined ranges for the label type and permissions
* for the user. Approvals for unknown labels are not included in the output, nor are
* approvals where the user has no permissions for that label.
*/
- public Result normalize(ChangeControl ctl, Collection<PatchSetApproval> approvals)
- throws PermissionBackendException {
+ public Result normalize(
+ ChangeNotes notes, CurrentUser user, Collection<PatchSetApproval> approvals)
+ throws PermissionBackendException, IOException {
List<PatchSetApproval> unchanged = Lists.newArrayListWithCapacity(approvals.size());
List<PatchSetApproval> updated = Lists.newArrayListWithCapacity(approvals.size());
List<PatchSetApproval> deleted = Lists.newArrayListWithCapacity(approvals.size());
LabelTypes labelTypes =
- ctl.getProjectControl().getProjectState().getLabelTypes(ctl.getNotes(), ctl.getUser());
+ projectCache.checkedGet(notes.getProjectName()).getLabelTypes(notes, user);
for (PatchSetApproval psa : approvals) {
Change.Id changeId = psa.getKey().getParentKey().getParentKey();
checkArgument(
- changeId.equals(ctl.getId()),
+ changeId.equals(notes.getChangeId()),
"Approval %s does not match change %s",
psa.getKey(),
- ctl.getChange().getKey());
+ notes.getChange().getKey());
if (psa.isLegacySubmit()) {
unchanged.add(psa);
continue;
@@ -138,7 +142,7 @@
}
PatchSetApproval copy = copy(psa);
applyTypeFloor(label, copy);
- if (!applyRightFloor(ctl.getNotes(), label, copy)) {
+ if (!applyRightFloor(notes, label, copy)) {
deleted.add(psa);
} else if (copy.getValue() != psa.getValue()) {
updated.add(copy);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
index 1cabc53..8f409ec 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
@@ -39,11 +39,13 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.strategy.CommitMergeStatus;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectState;
import com.google.gwtorm.server.OrmException;
@@ -355,7 +357,7 @@
PatchSetApproval submitAudit = null;
- for (PatchSetApproval a : safeGetApprovals(ctl, psId)) {
+ for (PatchSetApproval a : safeGetApprovals(ctl.getNotes(), ctl.getUser(), psId)) {
if (a.getValue() <= 0) {
// Negative votes aren't counted.
continue;
@@ -448,9 +450,10 @@
return "Verified".equalsIgnoreCase(id.get());
}
- private Iterable<PatchSetApproval> safeGetApprovals(ChangeControl ctl, PatchSet.Id psId) {
+ private Iterable<PatchSetApproval> safeGetApprovals(
+ ChangeNotes notes, CurrentUser user, PatchSet.Id psId) {
try {
- return approvalsUtil.byPatchSet(db.get(), ctl, psId, null, null);
+ return approvalsUtil.byPatchSet(db.get(), notes, user, psId, null, null);
} catch (OrmException e) {
log.error("Can't read approval records for " + psId, e);
return Collections.emptyList();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 042dad2..b1edb92 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -66,6 +66,7 @@
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.api.projects.ProjectConfigEntryType;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
+import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicMap.Entry;
import com.google.gerrit.extensions.registration.DynamicSet;
@@ -93,13 +94,13 @@
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.SetHashtagsOp;
import com.google.gerrit.server.config.AllProjectsName;
-import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.PluginConfig;
import com.google.gerrit.server.config.ProjectConfigEntry;
import com.google.gerrit.server.edit.ChangeEdit;
import com.google.gerrit.server.edit.ChangeEditUtil;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.BanCommit;
+import com.google.gerrit.server.git.ChangeReportFormatter;
import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.git.MergeOp;
import com.google.gerrit.server.git.MergeOpRepoManager;
@@ -318,7 +319,6 @@
private final Sequences seq;
private final SetHashtagsOp.Factory hashtagsFactory;
private final SshInfo sshInfo;
- private final String canonicalWebUrl;
private final SubmoduleOp.Factory subOpFactory;
private final TagCache tagCache;
private final CreateRefControl createRefControl;
@@ -370,10 +370,10 @@
private Task closeProgress;
private Task commandProgress;
private MessageSender messageSender;
+ private final ChangeReportFormatter changeFormatter;
@Inject
ReceiveCommits(
- @CanonicalWebUrl String canonicalWebUrl,
AccountResolver accountResolver,
AccountsUpdate.Server accountsUpdate,
AllProjectsName allProjectsName,
@@ -406,6 +406,7 @@
SubmoduleOp.Factory subOpFactory,
TagCache tagCache,
CreateRefControl createRefControl,
+ DynamicItem<ChangeReportFormatter> changeFormatterProvider,
@Assisted ProjectControl projectControl,
@Assisted ReceivePack rp,
@Assisted AllRefsWatcher allRefsWatcher,
@@ -416,9 +417,10 @@
this.accountsUpdate = accountsUpdate;
this.allProjectsName = allProjectsName;
this.batchUpdateFactory = batchUpdateFactory;
- this.canonicalWebUrl = canonicalWebUrl;
this.changeInserterFactory = changeInserterFactory;
this.commitValidatorsFactory = commitValidatorsFactory;
+ this.changeFormatter = changeFormatterProvider.get();
+ this.user = projectControl.getUser().asIdentifiedUser();
this.db = db;
this.editUtil = editUtil;
this.hashtagsFactory = hashtagsFactory;
@@ -454,7 +456,6 @@
// Immutable fields derived from constructor arguments.
repo = rp.getRepository();
- user = projectControl.getUser().asIdentifiedUser();
project = projectControl.getProject();
labelTypes = projectControl.getProjectState().getLabelTypes();
permissions = permissionBackend.user(user).project(project.getNameKey());
@@ -604,13 +605,7 @@
addMessage("");
addMessage("New Changes:");
for (CreateRequest c : created) {
- addMessage(
- formatChangeUrl(
- canonicalWebUrl,
- c.change,
- c.change.getSubject(),
- c.change.getStatus() == Change.Status.DRAFT,
- false));
+ addMessage(changeFormatter.newChange(new ChangeReportFormatter.Input(c.change)));
}
addMessage("");
}
@@ -626,6 +621,20 @@
addMessage("");
addMessage("Updated Changes:");
boolean edit = magicBranch != null && magicBranch.edit;
+ Boolean isPrivate = null;
+ Boolean wip = null;
+ if (magicBranch != null) {
+ if (magicBranch.isPrivate) {
+ isPrivate = true;
+ } else if (magicBranch.removePrivate) {
+ isPrivate = false;
+ }
+ if (magicBranch.workInProgress) {
+ wip = true;
+ } else if (magicBranch.ready) {
+ wip = false;
+ }
+ }
for (ReplaceRequest u : updated) {
String subject;
if (edit) {
@@ -639,36 +648,27 @@
} else {
subject = u.info.getSubject();
}
- addMessage(
- formatChangeUrl(
- canonicalWebUrl,
- u.notes.getChange(),
- subject,
- u.replaceOp != null && u.replaceOp.getPatchSet().isDraft(),
- edit));
+
+ if (isPrivate == null) {
+ isPrivate = u.notes.getChange().isPrivate();
+ }
+ if (wip == null) {
+ wip = u.notes.getChange().isWorkInProgress();
+ }
+
+ ChangeReportFormatter.Input input =
+ new ChangeReportFormatter.Input(u.notes.getChange())
+ .setSubject(subject)
+ .setDraft(u.replaceOp != null && u.replaceOp.getPatchSet().isDraft())
+ .setEdit(edit)
+ .setPrivate(isPrivate)
+ .setWorkInProgress(wip);
+ addMessage(changeFormatter.changeUpdated(input));
}
addMessage("");
}
}
- private static String formatChangeUrl(
- String url, Change change, String subject, boolean draft, boolean edit) {
- StringBuilder m =
- new StringBuilder()
- .append(" ")
- .append(url)
- .append(change.getChangeId())
- .append(" ")
- .append(ChangeUtil.cropSubject(subject));
- if (draft) {
- m.append(" [DRAFT]");
- }
- if (edit) {
- m.append(" [EDIT]");
- }
- return m.toString();
- }
-
private void insertChangesAndPatchSets() {
ReceiveCommand magicBranchCmd = magicBranch != null ? magicBranch.cmd : null;
if (magicBranchCmd != null && magicBranchCmd.getResult() != NOT_ATTEMPTED) {
@@ -1678,7 +1678,7 @@
private boolean requestReplace(
ReceiveCommand cmd, boolean checkMergedInto, Change change, RevCommit newCommit) {
if (change.getStatus().isClosed()) {
- reject(cmd, "change " + canonicalWebUrl + change.getId() + " closed");
+ reject(cmd, changeFormatter.changeClosed(new ChangeReportFormatter.Input(change)));
return false;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 3399071..8c67c72 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -311,7 +311,8 @@
approvals);
approvalCopier.copyInReviewDb(
ctx.getDb(),
- ctx.getControl(),
+ ctx.getNotes(),
+ ctx.getUser(),
newPatchSet,
ctx.getRevWalk(),
ctx.getRepoView().getConfig(),
@@ -401,7 +402,8 @@
for (PatchSetApproval a :
approvalsUtil.byPatchSetUser(
ctx.getDb(),
- ctx.getControl(),
+ ctx.getNotes(),
+ ctx.getUser(),
priorPatchSetId,
ctx.getAccountId(),
ctx.getRevWalk(),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
index affa918..1284fca 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
@@ -358,7 +358,12 @@
Map<PatchSetApproval.Key, PatchSetApproval> byKey = new HashMap<>();
for (PatchSetApproval psa :
args.approvalsUtil.byPatchSet(
- ctx.getDb(), ctx.getControl(), psId, ctx.getRevWalk(), ctx.getRepoView().getConfig())) {
+ ctx.getDb(),
+ ctx.getNotes(),
+ ctx.getUser(),
+ psId,
+ ctx.getRevWalk(),
+ ctx.getRepoView().getConfig())) {
byKey.put(psa.getKey(), psa);
}
@@ -372,7 +377,7 @@
// was added. So we need to make sure votes are accurate now. This way if
// permissions get modified in the future, historical records stay accurate.
LabelNormalizer.Result normalized =
- args.labelNormalizer.normalize(ctx.getControl(), byKey.values());
+ args.labelNormalizer.normalize(ctx.getNotes(), ctx.getUser(), byKey.values());
update.putApproval(submitter.getLabel(), submitter.getValue());
saveApprovals(normalized, ctx, update, false);
return normalized;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index 68bcda4..0f4f376 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -292,7 +292,8 @@
approvalsUtil
.byPatchSetUser(
ctx.getDb(),
- changeControl,
+ changeControl.getNotes(),
+ changeControl.getUser(),
psId,
ctx.getAccountId(),
ctx.getRevWalk(),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java
index 9c2e39d..2afb4a5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java
@@ -70,7 +70,12 @@
Table<Account.Id, String, PatchSetApproval> neg = HashBasedTable.create();
for (PatchSetApproval ca :
args.approvalsUtil.byPatchSet(
- args.db.get(), changeData.changeControl(), patchSet.getId(), null, null)) {
+ args.db.get(),
+ changeData.notes(),
+ changeData.changeControl().getUser(),
+ patchSet.getId(),
+ null,
+ null)) {
LabelType lt = labelTypes.byLabel(ca.getLabelId());
if (lt == null) {
continue;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index cd1e226..ad74108 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -172,7 +172,7 @@
// Prepopulate the change exists with proper noteDbState field.
change = newNoteDbOnlyChange(project, changeId);
} else {
- checkArgument(change != null, "change %s not found in ReviewDb", changeId);
+ checkNotNull(change, "change %s not found in ReviewDb", changeId);
checkArgument(
change.getProject().equals(project),
"passed project %s when creating ChangeNotes for %s, but actual project is %s",
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
index 7323c2e..61a0cd6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
@@ -320,6 +320,7 @@
}
private final FileBasedConfig gerritConfig;
+ private final FileBasedConfig noteDbConfig;
private final SchemaFactory<ReviewDb> schemaFactory;
private final GitRepositoryManager repoManager;
private final AllProjectsName allProjects;
@@ -374,7 +375,6 @@
this.userFactory = userFactory;
this.globalNotesMigration = globalNotesMigration;
this.primaryStorageMigrator = primaryStorageMigrator;
- this.gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
this.executor = executor;
this.projects = projects;
this.changes = changes;
@@ -384,6 +384,11 @@
this.forceRebuild = forceRebuild;
this.sequenceGap = sequenceGap;
this.autoMigrate = autoMigrate;
+
+ // Stack notedb.config over gerrit.config, in the same way as GerritServerConfigProvider.
+ this.gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
+ this.noteDbConfig =
+ new FileBasedConfig(gerritConfig, sitePaths.notedb_config.toFile(), FS.detect());
}
@Override
@@ -564,9 +569,10 @@
private Optional<NotesMigrationState> loadState() throws IOException {
try {
gerritConfig.load();
- return NotesMigrationState.forConfig(gerritConfig);
+ noteDbConfig.load();
+ return NotesMigrationState.forConfig(noteDbConfig);
} catch (ConfigInvalidException | IllegalArgumentException e) {
- log.warn("error reading NoteDb migration options from " + gerritConfig.getFile(), e);
+ log.warn("error reading NoteDb migration options from " + noteDbConfig.getFile(), e);
return Optional.empty();
}
}
@@ -597,9 +603,9 @@
? "But found this state:\n" + actualOldState.get().toText()
: "But could not parse the current state"));
}
- newState.setConfigValues(gerritConfig);
- additionalUpdates.accept(gerritConfig);
- gerritConfig.save();
+ newState.setConfigValues(noteDbConfig);
+ additionalUpdates.accept(noteDbConfig);
+ noteDbConfig.save();
// Only set in-memory state once it's been persisted to storage.
globalNotesMigration.setFrom(newState);
@@ -610,9 +616,9 @@
private void enableAutoMigrate() throws MigrationException {
try {
- gerritConfig.load();
- setAutoMigrate(gerritConfig, true);
- gerritConfig.save();
+ noteDbConfig.load();
+ setAutoMigrate(noteDbConfig, true);
+ noteDbConfig.save();
} catch (ConfigInvalidException | IOException e) {
throw new MigrationException("Error saving auto-migration config", e);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index f5e4542..13fc362 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -293,7 +293,8 @@
}
for (PatchSetApproval ap :
- approvalsUtil.byPatchSet(db, this, getChange().currentPatchSetId(), null, null)) {
+ approvalsUtil.byPatchSet(
+ db, getNotes(), getUser(), getChange().currentPatchSetId(), null, null)) {
LabelType type =
getProjectControl()
.getProjectState()
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateAccessChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateAccessChange.java
new file mode 100644
index 0000000..84f7e27
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateAccessChange.java
@@ -0,0 +1,149 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.project;
+
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.common.data.AccessSection;
+import com.google.gerrit.common.errors.InvalidNameException;
+import com.google.gerrit.common.errors.PermissionDeniedException;
+import com.google.gerrit.extensions.api.access.ProjectAccessInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.Sequences;
+import com.google.gerrit.server.change.ChangeInserter;
+import com.google.gerrit.server.change.ChangeJson;
+import com.google.gerrit.server.git.MetaDataUpdate;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
+import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.UpdateException;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.List;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+@Singleton
+public class CreateAccessChange implements RestModifyView<ProjectResource, ProjectAccessInput> {
+ private final PermissionBackend permissionBackend;
+ private final Sequences seq;
+ private final ChangeInserter.Factory changeInserterFactory;
+ private final BatchUpdate.Factory updateFactory;
+ private final Provider<MetaDataUpdate.User> metaDataUpdateFactory;
+ private final Provider<ReviewDb> db;
+ private final SetAccessUtil setAccess;
+ private final ChangeJson.Factory jsonFactory;
+
+ @Inject
+ CreateAccessChange(
+ PermissionBackend permissionBackend,
+ ChangeInserter.Factory changeInserterFactory,
+ BatchUpdate.Factory updateFactory,
+ Sequences seq,
+ Provider<MetaDataUpdate.User> metaDataUpdateFactory,
+ Provider<ReviewDb> db,
+ SetAccessUtil accessUtil,
+ ChangeJson.Factory jsonFactory) {
+ this.permissionBackend = permissionBackend;
+ this.seq = seq;
+ this.changeInserterFactory = changeInserterFactory;
+ this.updateFactory = updateFactory;
+ this.metaDataUpdateFactory = metaDataUpdateFactory;
+ this.db = db;
+ this.setAccess = accessUtil;
+ this.jsonFactory = jsonFactory;
+ }
+
+ @Override
+ public Response<ChangeInfo> apply(ProjectResource rsrc, ProjectAccessInput input)
+ throws PermissionBackendException, PermissionDeniedException, IOException,
+ ConfigInvalidException, OrmException, InvalidNameException, UpdateException,
+ RestApiException {
+ MetaDataUpdate.User metaDataUpdateUser = metaDataUpdateFactory.get();
+ List<AccessSection> removals = setAccess.getAccessSections(input.remove);
+ List<AccessSection> additions = setAccess.getAccessSections(input.add);
+
+ PermissionBackend.ForRef metaRef =
+ permissionBackend.user(rsrc.getUser()).project(rsrc.getNameKey()).ref(RefNames.REFS_CONFIG);
+ try {
+ metaRef.check(RefPermission.READ);
+ } catch (AuthException denied) {
+ throw new PermissionDeniedException(RefNames.REFS_CONFIG + " not visible");
+ }
+ if (!rsrc.getControl().isOwner()) {
+ try {
+ metaRef.check(RefPermission.CREATE_CHANGE);
+ } catch (AuthException denied) {
+ throw new PermissionDeniedException("cannot create change for " + RefNames.REFS_CONFIG);
+ }
+ }
+
+ Project.NameKey newParentProjectName =
+ input.parent == null ? null : new Project.NameKey(input.parent);
+
+ try (MetaDataUpdate md = metaDataUpdateUser.create(rsrc.getNameKey())) {
+ ProjectConfig config = ProjectConfig.read(md);
+
+ setAccess.validateChanges(config, removals, additions);
+ setAccess.applyChanges(config, removals, additions);
+ try {
+ setAccess.setParentName(
+ rsrc.getUser().asIdentifiedUser(),
+ config,
+ rsrc.getNameKey(),
+ newParentProjectName,
+ false);
+ } catch (AuthException e) {
+ throw new IllegalStateException(e);
+ }
+
+ md.setMessage("Review access change");
+ md.setInsertChangeId(true);
+ Change.Id changeId = new Change.Id(seq.nextChangeId());
+ RevCommit commit =
+ config.commitToNewRef(
+ md, new PatchSet.Id(changeId, Change.INITIAL_PATCH_SET_ID).toRefName());
+
+ try (ObjectInserter objInserter = md.getRepository().newObjectInserter();
+ ObjectReader objReader = objInserter.newReader();
+ RevWalk rw = new RevWalk(objReader);
+ BatchUpdate bu =
+ updateFactory.create(db.get(), rsrc.getNameKey(), rsrc.getUser(), TimeUtil.nowTs())) {
+ bu.setRepository(md.getRepository(), rw, objInserter);
+ ChangeInserter ins = changeInserterFactory.create(changeId, commit, RefNames.REFS_CONFIG);
+ ins.setMessage("First patchset").setValidate(false).setUpdateRef(false);
+ bu.insertChange(ins);
+ bu.execute();
+ return Response.created(jsonFactory.noOptions().format(ins.getChange()));
+ }
+ }
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/Module.java
index 11f3805..1f33bc3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/Module.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/Module.java
@@ -48,6 +48,7 @@
get(PROJECT_KIND, "access").to(GetAccess.class);
post(PROJECT_KIND, "access").to(SetAccess.class);
+ put(PROJECT_KIND, "access:review").to(CreateAccessChange.class);
get(PROJECT_KIND, "parent").to(GetParent.class);
put(PROJECT_KIND, "parent").to(SetParent.class);
@@ -96,7 +97,6 @@
get(PROJECT_KIND, "config").to(GetConfig.class);
put(PROJECT_KIND, "config").to(PutConfig.class);
-
post(COMMIT_KIND, "cherrypick").to(CherryPickCommit.class);
factory(DeleteRef.Factory.class);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
index fb19ba2..3a1c60a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -718,7 +718,8 @@
try {
currentApprovals =
ImmutableList.copyOf(
- approvalsUtil.byPatchSet(db, changeControl(), c.currentPatchSetId(), null, null));
+ approvalsUtil.byPatchSet(
+ db, notes(), changeControl().getUser(), c.currentPatchSetId(), null, null));
} catch (OrmException e) {
if (e.getCause() instanceof NoSuchChangeException) {
currentApprovals = Collections.emptyList();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java
index c27a096..c274e56 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java
@@ -189,7 +189,7 @@
+ "patch_set_id INTEGER DEFAULT 0 NOT NULL, "
+ "file_name VARCHAR(4096) DEFAULT '' NOT NULL, "
+ "CONSTRAINT primary_key_account_patch_reviews "
- + "PRIMARY KEY (account_id, change_id, patch_set_id, file_name)"
+ + "PRIMARY KEY (change_id, patch_set_id, account_id, file_name)"
+ ")");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_139.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_139.java
index be919a1..4dfc41a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_139.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_139.java
@@ -15,6 +15,8 @@
package com.google.gerrit.server.schema;
import com.google.auto.value.AutoValue;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
import com.google.gerrit.common.Nullable;
@@ -92,11 +94,11 @@
ProjectWatch.builder()
.project(new Project.NameKey(rs.getString(2)))
.filter(rs.getString(3))
- .notifyAbandonedChanges(rs.getBoolean(4))
- .notifyAllComments(rs.getBoolean(5))
- .notifyNewChanges(rs.getBoolean(6))
- .notifyNewPatchSets(rs.getBoolean(7))
- .notifySubmittedChanges(rs.getBoolean(8));
+ .notifyAbandonedChanges(toBoolean(rs.getString(4)))
+ .notifyAllComments(toBoolean(rs.getString(5)))
+ .notifyNewChanges(toBoolean(rs.getString(6)))
+ .notifyNewPatchSets(toBoolean(rs.getString(7)))
+ .notifySubmittedChanges(toBoolean(rs.getString(8)));
imports.put(accountId, b.build());
}
}
@@ -196,4 +198,9 @@
abstract ProjectWatch build();
}
}
+
+ private static boolean toBoolean(String v) {
+ Preconditions.checkState(!Strings.isNullOrEmpty(v));
+ return v.equals("Y");
+ }
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/config/RepositoryConfigTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/config/RepositoryConfigTest.java
index 804e2f2..b3faef4 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/config/RepositoryConfigTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/config/RepositoryConfigTest.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.config;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.client.SubmitType;
@@ -147,7 +148,7 @@
@Test
public void basePathWhenNotConfigured() {
- assertThat((Object) repoCfg.getBasePath(new NameKey("someProject"))).isNull();
+ assertThat(repoCfg.getBasePath(new NameKey("someProject"))).isNull();
}
@Test
@@ -161,7 +162,7 @@
public void basePathForSpecificFilter() {
String basePath = "/someAbsolutePath/someDirectory";
configureBasePath("someProject", basePath);
- assertThat((Object) repoCfg.getBasePath(new NameKey("someOtherProject"))).isNull();
+ assertThat(repoCfg.getBasePath(new NameKey("someOtherProject"))).isNull();
assertThat(repoCfg.getBasePath(new NameKey("someProject")).toString()).isEqualTo(basePath);
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
index eabccf7..75b00fd 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java
@@ -40,6 +40,7 @@
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.git.LabelNormalizer.Result;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.schema.SchemaCreator;
import com.google.gerrit.server.util.RequestContext;
@@ -69,12 +70,14 @@
@Inject private ProjectCache projectCache;
@Inject private SchemaCreator schemaCreator;
@Inject protected ThreadLocalRequestContext requestContext;
+ @Inject private ChangeNotes.Factory changeNotesFactory;
private LifecycleManager lifecycle;
private ReviewDb db;
private Account.Id userId;
private IdentifiedUser user;
private Change change;
+ private ChangeNotes notes;
@Before
public void setUpInjector() throws Exception {
@@ -131,6 +134,7 @@
ps.setSubject("Test change");
change.setCurrentPatchSet(ps);
db.changes().insert(ImmutableList.of(change));
+ notes = changeNotesFactory.createChecked(db, change);
}
@After
@@ -155,7 +159,7 @@
PatchSetApproval cr = psa(userId, "Code-Review", 2);
PatchSetApproval v = psa(userId, "Verified", 1);
assertEquals(
- Result.create(list(v), list(copy(cr, 1)), list()), norm.normalize(change, list(cr, v)));
+ Result.create(list(v), list(copy(cr, 1)), list()), norm.normalize(notes, list(cr, v)));
}
@Test
@@ -169,14 +173,14 @@
PatchSetApproval v = psa(userId, "Verified", 5);
assertEquals(
Result.create(list(), list(copy(cr, 2), copy(v, 1)), list()),
- norm.normalize(change, list(cr, v)));
+ norm.normalize(notes, list(cr, v)));
}
@Test
public void emptyPermissionRangeOmitsResult() throws Exception {
PatchSetApproval cr = psa(userId, "Code-Review", 1);
PatchSetApproval v = psa(userId, "Verified", 1);
- assertEquals(Result.create(list(), list(), list(cr, v)), norm.normalize(change, list(cr, v)));
+ assertEquals(Result.create(list(), list(), list(cr, v)), norm.normalize(notes, list(cr, v)));
}
@Test
@@ -187,7 +191,7 @@
PatchSetApproval cr = psa(userId, "Code-Review", 0);
PatchSetApproval v = psa(userId, "Verified", 0);
- assertEquals(Result.create(list(cr), list(), list(v)), norm.normalize(change, list(cr, v)));
+ assertEquals(Result.create(list(cr), list(), list(v)), norm.normalize(notes, list(cr, v)));
}
private ProjectConfig loadAllProjects() throws Exception {
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/ChangeArgumentParser.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/ChangeArgumentParser.java
index 0801447..da61db8 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/ChangeArgumentParser.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/ChangeArgumentParser.java
@@ -14,9 +14,8 @@
package com.google.gerrit.sshd;
-import static java.util.stream.Collectors.toList;
-
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -30,7 +29,6 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.sshd.BaseCommand.UnloggedFailure;
import com.google.gwtorm.server.OrmException;
@@ -39,7 +37,6 @@
import java.util.Arrays;
import java.util.List;
import java.util.Map;
-import java.util.Optional;
public class ChangeArgumentParser {
private final CurrentUser currentUser;
@@ -85,9 +82,8 @@
ProjectControl projectControl,
boolean useIndex)
throws UnloggedFailure, OrmException, PermissionBackendException {
- List<ChangeControl> matched =
- useIndex ? changeFinder.find(id, currentUser) : changeFromNotesFactory(id, currentUser);
- List<ChangeControl> toAdd = new ArrayList<>(changes.size());
+ List<ChangeNotes> matched = useIndex ? changeFinder.find(id) : changeFromNotesFactory(id);
+ List<ChangeNotes> toAdd = new ArrayList<>(changes.size());
boolean canMaintainServer;
try {
permissionBackend.user(currentUser).check(GlobalPermission.MAINTAIN_SERVER);
@@ -95,16 +91,16 @@
} catch (AuthException | PermissionBackendException e) {
canMaintainServer = false;
}
- for (ChangeControl ctl : matched) {
- if (!changes.containsKey(ctl.getId())
- && inProject(projectControl, ctl.getProject())
+ for (ChangeNotes notes : matched) {
+ if (!changes.containsKey(notes.getChangeId())
+ && inProject(projectControl, notes.getProjectName())
&& (canMaintainServer
|| permissionBackend
.user(currentUser)
- .change(ctl.getNotes())
+ .change(notes)
.database(db)
.test(ChangePermission.READ))) {
- toAdd.add(ctl);
+ toAdd.add(notes);
}
}
@@ -113,19 +109,18 @@
} else if (toAdd.size() > 1) {
throw new UnloggedFailure(1, "\"" + id + "\" matches multiple changes");
}
- ChangeControl ctl = toAdd.get(0);
- changes.put(ctl.getId(), changesCollection.parse(ctl));
+ Change.Id cId = toAdd.get(0).getChangeId();
+ ChangeResource changeResource;
+ try {
+ changeResource = changesCollection.parse(cId);
+ } catch (ResourceNotFoundException e) {
+ throw new UnloggedFailure(1, "\"" + id + "\" no such change");
+ }
+ changes.put(cId, changeResource);
}
- private List<ChangeControl> changeFromNotesFactory(String id, CurrentUser currentUser)
- throws OrmException, UnloggedFailure {
- return changeNotesFactory
- .create(db, parseId(id))
- .stream()
- .map(changeNote -> controlForChange(changeNote, currentUser))
- .filter(changeControl -> changeControl.isPresent())
- .map(changeControl -> changeControl.get())
- .collect(toList());
+ private List<ChangeNotes> changeFromNotesFactory(String id) throws OrmException, UnloggedFailure {
+ return changeNotesFactory.create(db, parseId(id));
}
private List<Change.Id> parseId(String id) throws UnloggedFailure {
@@ -136,17 +131,9 @@
}
}
- private Optional<ChangeControl> controlForChange(ChangeNotes change, CurrentUser user) {
- try {
- return Optional.of(changeControlFactory.controlFor(change, user));
- } catch (NoSuchChangeException e) {
- return Optional.empty();
- }
- }
-
- private boolean inProject(ProjectControl projectControl, Project project) {
+ private boolean inProject(ProjectControl projectControl, Project.NameKey project) {
if (projectControl != null) {
- return projectControl.getProject().getNameKey().equals(project.getNameKey());
+ return projectControl.getProject().getNameKey().equals(project);
}
// No --project option, so they want every project.
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PatchSetParser.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PatchSetParser.java
index 9b02f38..c3613b1 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PatchSetParser.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PatchSetParser.java
@@ -21,10 +21,8 @@
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeFinder;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
@@ -44,7 +42,6 @@
private final ChangeNotes.Factory notesFactory;
private final PatchSetUtil psUtil;
private final ChangeFinder changeFinder;
- private final Provider<CurrentUser> self;
@Inject
PatchSetParser(
@@ -52,14 +49,12 @@
Provider<InternalChangeQuery> queryProvider,
ChangeNotes.Factory notesFactory,
PatchSetUtil psUtil,
- ChangeFinder changeFinder,
- Provider<CurrentUser> self) {
+ ChangeFinder changeFinder) {
this.db = db;
this.queryProvider = queryProvider;
this.notesFactory = notesFactory;
this.psUtil = psUtil;
this.changeFinder = changeFinder;
- this.self = self;
}
public PatchSet parsePatchSet(String token, ProjectControl projectControl, String branch)
@@ -141,8 +136,8 @@
return notesFactory.create(db.get(), projectControl.getProject().getNameKey(), changeId);
}
try {
- ChangeControl ctl = changeFinder.findOne(changeId, self.get());
- return notesFactory.create(db.get(), ctl.getProject().getNameKey(), changeId);
+ ChangeNotes notes = changeFinder.findOne(changeId);
+ return notesFactory.create(db.get(), notes.getProjectName(), changeId);
} catch (NoSuchChangeException e) {
throw error("\"" + changeId + "\" no such change");
}
diff --git a/plugins/replication b/plugins/replication
index 643d635..e145010 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit 643d635a4502e2a2df6cb02edade88bad3fd953a
+Subproject commit e145010de7911623c98e6111537ffa4997d1a82e
diff --git a/plugins/reviewnotes b/plugins/reviewnotes
index fe99eb4..5e6d0fb 160000
--- a/plugins/reviewnotes
+++ b/plugins/reviewnotes
@@ -1 +1 @@
-Subproject commit fe99eb4399054d3fd67ff2330aa92841d76e53bb
+Subproject commit 5e6d0fb102a2d5486c94a0ea7463efd1508f5ade
diff --git a/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.html b/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.html
index bfced55..9371b17 100644
--- a/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.html
+++ b/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.html
@@ -64,7 +64,7 @@
}
th {
border-bottom: 1px solid #eee;
- font-weight: bold;
+ font-family: var(--font-family-bold);
text-align: left;
}
</style>
diff --git a/polygerrit-ui/app/elements/admin/gr-project-access/gr-project-access.html b/polygerrit-ui/app/elements/admin/gr-project-access/gr-project-access.html
index 686aa10..7b23506 100644
--- a/polygerrit-ui/app/elements/admin/gr-project-access/gr-project-access.html
+++ b/polygerrit-ui/app/elements/admin/gr-project-access/gr-project-access.html
@@ -32,6 +32,12 @@
<style include="shared-styles"></style>
<style include="gr-menu-page-styles"></style>
<main>
+ <template is="dom-if" if="[[_inheritsFrom]]">
+ <h3 id="inheritsFrom">Rights Inherit From
+ <a href$="[[_computeParentHref(_inheritsFrom.name)]]" rel="noopener">
+ [[_inheritsFrom.name]]</a>
+ </h3>
+ </template>
<template
is="dom-repeat"
items="{{_sections}}"
@@ -43,12 +49,6 @@
editing="[[_editing]]"
groups="[[_groups]]"></gr-access-section>
</template>
- <template is="dom-if" if="[[_inheritsFrom]]">
- <h3 id="inheritsFrom">Rights Inherit From
- <a href$="[[_computeParentHref(_inheritsFrom.name)]]" rel="noopener">
- [[_inheritsFrom.name]]</a>
- </h3>
- </template>
</main>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
</template>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
index 70275b3..4ff10cb 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
@@ -39,7 +39,7 @@
background-color: #ebf5fb;
}
:host([needs-review]) {
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
:host([assigned]) {
background-color: #fcfad6;
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.html b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.html
index bec7429..f04b7c6 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.html
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.html
@@ -16,8 +16,9 @@
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
<link rel="import" href="../../../bower_components/polymer/polymer.html">
-<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../../styles/shared-styles.html">
+<link rel="import" href="../../change-list/gr-change-list/gr-change-list.html">
+<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<dom-module id="gr-dashboard-view">
<template>
diff --git a/polygerrit-ui/app/elements/change/gr-account-list/gr-account-list.html b/polygerrit-ui/app/elements/change/gr-account-list/gr-account-list.html
index 7b18b5e..3d98ecb 100644
--- a/polygerrit-ui/app/elements/change/gr-account-list/gr-account-list.html
+++ b/polygerrit-ui/app/elements/change/gr-account-list/gr-account-list.html
@@ -37,12 +37,15 @@
.pending-add {
font-style: italic;
}
+ .list {
+ @apply --account-list-style;
+ }
</style>
<!--
NOTE(Issue 6419): Nest the inner dom-repeat template in a div rather than
as a direct child of the dom-module's template.
-->
- <div>
+ <div class="list">
<template id="chips" is="dom-repeat" items="[[accounts]]" as="account">
<gr-account-chip
account="[[account]]"
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 3bccdd8..ff77142 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
@@ -892,6 +892,11 @@
*/
_send(method, payload, actionEndpoint, revisionAction, cleanupFn,
opt_errorFn) {
+ const handleError = response => {
+ cleanupFn.call(this);
+ this._handleResponseError(response);
+ };
+
return this.fetchIsLatestKnown(this.change, this.$.restAPI)
.then(isLatest => {
if (!isLatest) {
@@ -904,13 +909,17 @@
Gerrit.Nav.navigateToChange(this.change);
},
});
+
+ // Because this is not a network error, call the cleanup function
+ // but not the error handler.
cleanupFn();
+
return Promise.resolve();
}
const patchNum = revisionAction ? this.patchNum : null;
return this.$.restAPI.getChangeURLAndSend(this.changeNum, method,
- patchNum, actionEndpoint, payload, this._handleResponseError,
- this).then(response => {
+ patchNum, actionEndpoint, payload, handleError, this)
+ .then(response => {
cleanupFn.call(this);
return response;
});
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 55cd982..11a2569 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
@@ -1084,5 +1084,88 @@
});
});
});
+
+ suite('_send', () => {
+ let cleanup;
+ let payload;
+ let onShowAlert;
+
+ setup(() => {
+ cleanup = sinon.stub();
+ element.changeNum = 42;
+ element.patchNum = 12;
+ payload = {foo: 'bar'};
+
+ onShowAlert = sinon.stub();
+ element.addEventListener('show-alert', onShowAlert);
+ });
+
+ suite('happy path', () => {
+ let sendStub;
+
+ setup(() => {
+ sandbox.stub(element, 'fetchIsLatestKnown')
+ .returns(Promise.resolve(true));
+ sendStub = sandbox.stub(element.$.restAPI, 'getChangeURLAndSend')
+ .returns(Promise.resolve({}));
+ });
+
+ test('change action', () => {
+ return element._send('DELETE', payload, '/endpoint', false, cleanup)
+ .then(() => {
+ assert.isFalse(onShowAlert.called);
+ assert.isTrue(cleanup.calledOnce);
+ assert.isTrue(sendStub.calledWith(42, 'DELETE', null,
+ '/endpoint', payload));
+ });
+ });
+
+ test('revision action', () => {
+ return element._send('DELETE', payload, '/endpoint', true, cleanup)
+ .then(() => {
+ assert.isFalse(onShowAlert.called);
+ assert.isTrue(cleanup.calledOnce);
+ assert.isTrue(sendStub.calledWith(42, 'DELETE', 12, '/endpoint',
+ payload));
+ });
+ });
+ });
+
+ suite('failure modes', () => {
+ test('non-latest', () => {
+ sandbox.stub(element, 'fetchIsLatestKnown')
+ .returns(Promise.resolve(false));
+ const sendStub = sandbox.stub(element.$.restAPI,
+ 'getChangeURLAndSend');
+
+ return element._send('DELETE', payload, '/endpoint', true, cleanup)
+ .then(() => {
+ assert.isTrue(onShowAlert.calledOnce);
+ assert.isTrue(cleanup.calledOnce);
+ assert.isFalse(sendStub.called);
+ });
+ });
+
+ test('send fails', () => {
+ sandbox.stub(element, 'fetchIsLatestKnown')
+ .returns(Promise.resolve(true));
+ const sendStub = sandbox.stub(element.$.restAPI,
+ 'getChangeURLAndSend',
+ (num, method, patchNum, endpoint, payload, onErr) => {
+ onErr();
+ return Promise.resolve(null);
+ });
+ const handleErrorStub = sandbox.stub(element, '_handleResponseError');
+
+ return element._send('DELETE', payload, '/endpoint', true, cleanup)
+ .then(() => {
+ assert.isFalse(onShowAlert.called);
+ assert.isTrue(cleanup.called);
+ assert.isTrue(sendStub.calledOnce);
+ assert.isTrue(handleErrorStub.called);
+ });
+ });
+ });
+ });
});
</script>
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
index 5a834b7..388a3f7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
@@ -43,7 +43,7 @@
}
.title {
color: #666;
- font-weight: bold;
+ font-family: var(--font-family-bold);
max-width: 20em;
word-break: break-word;
}
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 39ae763..5642fc0 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
@@ -77,7 +77,7 @@
.header-title {
flex: 1;
font-size: 1.2em;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
gr-change-star {
margin-right: .25em;
@@ -207,6 +207,9 @@
#fileList {
padding: .5em calc(var(--default-horizontal-margin) / 2);
}
+ .scrollable {
+ overflow: auto;
+ }
@media screen and (min-width: 80em) {
.commitMessage {
max-width: var(--commit-message-max-width, 100ch);
@@ -278,9 +281,6 @@
flex: initial;
margin-right: 0;
}
- .scrollable {
- overflow: auto;
- }
/* Change actions are the only thing thant need to remain visible due
to the fact that they may have the currently visible overlay open. */
#mainContent.overlayOpen .hideOnMobileOverlay {
diff --git a/polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list.html b/polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list.html
index 2b7b0fd..705cf52 100644
--- a/polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list.html
+++ b/polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list.html
@@ -36,7 +36,7 @@
}
.file {
border-top: 1px solid #ddd;
- font-weight: bold;
+ font-family: var(--font-family-bold);
margin: 10px 0 3px;
padding: 10px 0 5px;
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index a2a64c8..95bfc70 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -104,7 +104,7 @@
}
.drafts {
color: #C62828;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
.show-hide {
margin-left: .4em;
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.html b/polygerrit-ui/app/elements/change/gr-message/gr-message.html
index ae43edf..c0e55ef 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.html
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.html
@@ -76,7 +76,7 @@
width: 2.5em;
}
.name {
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
.message {
--gr-formatted-text-prose-max-width: 80ch;
@@ -136,7 +136,7 @@
}
gr-account-label {
--gr-account-label-text-style: {
- font-weight: bold;
+ font-family: var(--font-family-bold);
};
}
</style>
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html
index 8c02e65..2ebf7c7 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html
@@ -68,7 +68,7 @@
}
.status {
color: #666;
- font-weight: bold;
+ font-family: var(--font-family-bold);
margin-left: .25em;
}
.notCurrent {
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 04a1b16..2f9f78c 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
@@ -76,8 +76,10 @@
display: flex;
flex-wrap: wrap;
flex: 1;
- max-height: 12em;
- overflow-y: auto;
+ --account-list-style: {
+ max-height: 12em;
+ overflow-y: auto;
+ }
}
#reviewerConfirmationOverlay {
padding: 1em;
@@ -87,7 +89,7 @@
margin-top: 1em;
}
.groupName {
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
.groupSize {
font-style: italic;
diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
index 3f3350e..2318657 100644
--- a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
+++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
@@ -48,12 +48,12 @@
text-align: right;
}
.header {
- font-weight: bold;
+ font-family: var(--font-family-bold);
padding-top: 1em;
}
.key {
display: inline-block;
- font-weight: bold;
+ font-family: var(--font-family-bold);
border-radius: 3px;
background-color: #f1f2f3;
padding: .1em .5em;
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.html b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.html
index 5523032..09f3029 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.html
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.html
@@ -120,7 +120,7 @@
@media screen and (max-width: 50em) {
.bigTitle {
font-size: 14px;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
gr-search-bar,
.browse,
diff --git a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
index 7089b2e..a056cf1 100644
--- a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
+++ b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
@@ -47,6 +47,8 @@
// - `leftSide`, optional, Boolean, if a `lineNum` is provided, a value
// of true selects the line from base of the patch range. False by
// default.
+ // - `edit`, optional, Boolean, whether or not the editor view of the
+ // diff should be loaded.
window.Gerrit = window.Gerrit || {};
@@ -211,6 +213,21 @@
},
/**
+ * Gets the link to load a file in an editor.
+ *
+ * @param {{ _number: number, project: string }} change The change object.
+ * @param {string} path The file path.
+ * @param {number=} opt_patchNum
+ * @param {number|string=} opt_basePatchNum The string 'PARENT' can be
+ * used for none.
+ * @return {string}
+ */
+ getEditUrlForDiff(change, path, opt_patchNum, opt_basePatchNum) {
+ return this.getUrlForDiffById(change._number, change.project, path,
+ opt_patchNum, opt_basePatchNum, null, null, true);
+ },
+
+ /**
* @param {number} changeNum
* @param {string} project The name of the project.
* @param {string} path The file path.
@@ -219,10 +236,11 @@
* used for none.
* @param {number=} opt_lineNum
* @param {boolean=} opt_leftSide
+ * @param {boolean=} opt_edit
* @return {string}
*/
getUrlForDiffById(changeNum, project, path, opt_patchNum,
- opt_basePatchNum, opt_lineNum, opt_leftSide) {
+ opt_basePatchNum, opt_lineNum, opt_leftSide, opt_edit) {
if (opt_basePatchNum === PARENT_PATCHNUM) {
opt_basePatchNum = undefined;
}
@@ -237,6 +255,7 @@
basePatchNum: opt_basePatchNum,
lineNum: opt_lineNum,
leftSide: opt_leftSide,
+ edit: opt_edit,
});
},
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.js b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
index 2de74e1..a2861c6 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
@@ -91,7 +91,7 @@
QUERY_OFFSET: '/q/:query,:offset',
// Matches /c/<changeNum>/[<basePatchNum>..][<patchNum>][/].
- CHNAGE_LEGACY: /^\/c\/(\d+)\/?(((\d+|edit)(\.\.(\d+|edit))?))?\/?$/,
+ CHANGE_LEGACY: /^\/c\/(\d+)\/?(((\d+|edit)(\.\.(\d+|edit))?))?\/?$/,
CHANGE_NUMBER_LEGACY: /^\/(\d+)\/?/,
// Matches
@@ -112,6 +112,10 @@
SETTINGS: /^\/settings\/?/,
SETTINGS_LEGACY: /^\/settings\/VE\/(\S+)/,
+
+ // Matches /c/<changeNum>/ /<URL tail>
+ // Catches improperly encoded URLs (context: Issue 7100)
+ IMPROPERLY_ENCODED_PLUS: /^\/c\/(.+)\/\ \/(.+)$/,
};
/**
@@ -514,7 +518,7 @@
this._mapRoute(RoutePattern.CHANGE_OR_DIFF, '_handleChangeOrDiffRoute');
- this._mapRoute(RoutePattern.CHNAGE_LEGACY, '_handleChangeLegacyRoute');
+ this._mapRoute(RoutePattern.CHANGE_LEGACY, '_handleChangeLegacyRoute');
this._mapRoute(RoutePattern.DIFF_LEGACY, '_handleDiffLegacyRoute');
@@ -529,6 +533,9 @@
this._mapRoute(RoutePattern.LOG_IN_OR_OUT, '_handlePassThroughRoute');
+ this._mapRoute(RoutePattern.IMPROPERLY_ENCODED_PLUS,
+ '_handleImproperlyEncodedPlusRoute');
+
// Note: this route should appear last so it only catches URLs unmatched
// by other patterns.
this._mapRoute(RoutePattern.DEFAULT, '_handleDefaultRoute');
@@ -957,6 +964,17 @@
location.reload();
},
+
+ /**
+ * URL may sometimes have /+/ encoded to / /.
+ * Context: Issue 6888, Issue 7100
+ */
+ _handleImproperlyEncodedPlusRoute(ctx) {
+ let hash = this._getHashFromCanonicalPath(ctx.canonicalPath);
+ if (hash.length) { hash = '#' + hash; }
+ this._redirect(`/c/${ctx.params[0]}/+/${ctx.params[1]}${hash}`);
+ },
+
/**
* Catchall route for when no other route is matched.
*/
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
index 831b905..df46a38 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
@@ -154,6 +154,7 @@
'_handleChangeLegacyRoute',
'_handleDiffLegacyRoute',
'_handleGroupMembersRoute',
+ '_handleImproperlyEncodedPlusRoute',
'_handlePassThroughRoute',
'_handleProjectAccessRoute',
'_handleProjectListFilterOffsetRoute',
@@ -433,6 +434,23 @@
404);
});
+ test('_handleImproperlyEncodedPlusRoute', () => {
+ // Regression test for Issue 7100.
+ element._handleImproperlyEncodedPlusRoute(
+ {canonicalPath: '/c/test/%20/42', params: ['test', '42']});
+ assert.isTrue(redirectStub.calledOnce);
+ assert.equal(
+ redirectStub.lastCall.args[0],
+ '/c/test/+/42');
+
+ sandbox.stub(element, '_getHashFromCanonicalPath').returns('foo');
+ element._handleImproperlyEncodedPlusRoute(
+ {canonicalPath: '/c/test/%20/42', params: ['test', '42']});
+ assert.equal(
+ redirectStub.lastCall.args[0],
+ '/c/test/+/42#foo');
+ });
+
suite('_handleRootRoute', () => {
test('closes for closeAfterLogin', () => {
const data = {querystring: 'closeAfterLogin', canonicalPath: ''};
@@ -547,10 +565,10 @@
});
suite('_handleDashboardRoute', () => {
- let reidrectToLoginStub;
+ let redirectToLoginStub;
setup(() => {
- reidrectToLoginStub = sandbox.stub(element, '_redirectToLogin');
+ redirectToLoginStub = sandbox.stub(element, '_redirectToLogin');
});
test('no user specified', () => {
@@ -558,7 +576,7 @@
const result = element._handleDashboardRoute(data);
assert.isNotOk(result);
assert.isFalse(setParamsStub.called);
- assert.isFalse(reidrectToLoginStub.called);
+ assert.isFalse(redirectToLoginStub.called);
assert.isTrue(redirectStub.called);
assert.equal(redirectStub.lastCall.args[0], '/dashboard/self');
});
@@ -570,7 +588,7 @@
const result = element._handleDashboardRoute(data);
assert.isOk(result);
return result.then(() => {
- assert.isTrue(reidrectToLoginStub.calledOnce);
+ assert.isTrue(redirectToLoginStub.calledOnce);
assert.isFalse(redirectStub.called);
assert.isFalse(setParamsStub.called);
});
@@ -583,7 +601,7 @@
const result = element._handleDashboardRoute(data);
assert.isOk(result);
return result.then(() => {
- assert.isFalse(reidrectToLoginStub.called);
+ assert.isFalse(redirectToLoginStub.called);
assert.isFalse(setParamsStub.called);
assert.isTrue(redirectStub.calledOnce);
assert.equal(redirectStub.lastCall.args[0], '/q/owner:foo');
@@ -597,7 +615,7 @@
const result = element._handleDashboardRoute(data);
assert.isOk(result);
return result.then(() => {
- assert.isFalse(reidrectToLoginStub.called);
+ assert.isFalse(redirectToLoginStub.called);
assert.isFalse(redirectStub.called);
assert.isTrue(setParamsStub.calledOnce);
assert.deepEqual(setParamsStub.lastCall.args[0], {
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
index 07e306a..bd8e680 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
@@ -14,7 +14,7 @@
(function() {
'use strict';
- // Possible static search options for auto complete.
+ // Possible static search options for auto complete, without negations.
const SEARCH_OPERATORS = [
'added:',
'age:',
@@ -56,6 +56,7 @@
'is:reviewer',
'is:starred',
'is:watched',
+ 'is:wip',
'label:',
'message:',
'owner:',
@@ -83,6 +84,10 @@
'tr:',
];
+ // All of the ops, with corresponding negations.
+ const SEARCH_OPERATORS_WITH_NEGATIONS =
+ SEARCH_OPERATORS.concat(SEARCH_OPERATORS.map(op => `-${op}`));
+
const SELF_EXPRESSION = 'self';
const ME_EXPRESSION = 'me';
@@ -269,7 +274,7 @@
return this._fetchAccounts(predicate, expression);
default:
- return Promise.resolve(SEARCH_OPERATORS
+ return Promise.resolve(SEARCH_OPERATORS_WITH_NEGATIONS
.filter(operator => operator.includes(input)));
}
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
index 9ad0bf9..ca22942 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
@@ -69,7 +69,7 @@
.authorName,
.draftLabel,
.draftTooltip {
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
.draftLabel,
.draftTooltip {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html b/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html
index 99a7054..f9ca92e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-preferences/gr-diff-preferences.html
@@ -45,7 +45,7 @@
}
.header {
border-bottom: 1px solid #ddd;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
.mainContainer {
padding: 1em 0;
@@ -69,7 +69,7 @@
justify-content: space-between;
}
.beta {
- font-weight: bold;
+ font-family: var(--font-family-bold);
color: #888;
}
</style>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
index b5da3a0..10d30e4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -113,7 +113,7 @@
}
.dropdown-content a[selected] {
color: #000;
- font-weight: bold;
+ font-family: var(--font-family-bold);
pointer-events: none;
text-decoration: none;
}
@@ -194,7 +194,7 @@
.mobileNavLink {
color: #000;
font-size: 1.5em;
- font-weight: bold;
+ font-family: var(--font-family-bold);
text-decoration: none;
}
.mobileNavLink:not([href]) {
diff --git a/polygerrit-ui/app/elements/gr-app.html b/polygerrit-ui/app/elements/gr-app.html
index 406a4a7..6f8a4a1 100644
--- a/polygerrit-ui/app/elements/gr-app.html
+++ b/polygerrit-ui/app/elements/gr-app.html
@@ -91,6 +91,7 @@
}
main {
flex: 1;
+ padding-bottom: 2em;
position: relative;
}
.errorView {
diff --git a/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog.html b/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog.html
index 34f0b16..1bd345e 100644
--- a/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog.html
+++ b/polygerrit-ui/app/elements/settings/gr-registration-dialog/gr-registration-dialog.html
@@ -36,7 +36,7 @@
}
header {
border-bottom: 1px solid #ddd;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
header,
main,
diff --git a/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html b/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
index bf5e4e0..51fa616 100644
--- a/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
+++ b/polygerrit-ui/app/elements/shared/gr-alert/gr-alert.html
@@ -53,7 +53,7 @@
}
.action {
color: #a1c2fa;
- font-weight: bold;
+ font-family: var(--font-family-bold);
margin-left: 1em;
text-decoration: none;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
index c3031e2..6df6c98 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.js
@@ -135,7 +135,22 @@
type: Boolean,
value: false,
},
+
+ /** The DOM element of the selected suggestion. */
_selected: Object,
+
+ _textChangedSinceCommit: {
+ type: Boolean,
+ value: false,
+ },
+ },
+
+ observers: [
+ '_textChanged(text)',
+ ],
+
+ _textChanged() {
+ this._textChangedSinceCommit = true;
},
attached() {
@@ -275,6 +290,9 @@
* @param {boolean=} opt_tabComplete
*/
_handleInputCommit(opt_tabComplete) {
+ // Nothing to do if new input hasn't been entered.
+ if (!this._textChangedSinceCommit) { return; }
+
this._selected = this.$.suggestions.getCursorTarget();
this._commit(opt_tabComplete);
},
@@ -341,6 +359,8 @@
if (!opt_silent) {
this.fire('commit', {value});
}
+
+ this._textChangedSinceCommit = false;
},
});
})();
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
index 75e221b..038962c 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete_test.html
@@ -296,6 +296,32 @@
assert.isFalse(element._computeSuggestionsHidden(suggestions, true));
});
+ test('changing input sets _textChangedSinceCommit', () => {
+ element._textChangedSinceCommit = false;
+ element.text = 'foo bar baz';
+ assert.isTrue(element._textChangedSinceCommit);
+ });
+
+ test('_handleInputCommit with no change does nothing', () => {
+ const commitStub = sandbox.stub(element, '_commit');
+ element._textChangedSinceCommit = false;
+ element._handleInputCommit();
+ assert.isFalse(commitStub.called);
+ });
+
+ test('_textChangedSinceCommit reset after commit', () => {
+ element._textChangedSinceCommit = true;
+ element._commit();
+ assert.isFalse(element._textChangedSinceCommit);
+ });
+
+ test('_handleInputCommit with change to text', () => {
+ const commitStub = sandbox.stub(element, '_commit');
+ element._textChangedSinceCommit = true;
+ element._handleInputCommit();
+ assert.isTrue(commitStub.calledOnce);
+ });
+
suite('focus', () => {
let commitSpy;
let focusSpy;
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 1fecbe7..d16e57f 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
@@ -30,9 +30,8 @@
color: #333;
cursor: pointer;
display: inline-block;
- font-family: var(--font-family);
+ font-family: var(--font-family-bold);
font-size: 12px;
- font-weight: bold;
outline-width: 0;
padding: .4em .85em;
position: relative;
@@ -65,7 +64,7 @@
border: none;
color: #00f;
font-size: inherit;
- font-weight: normal;
+ font-family: var(--font-family);
padding: 0;
text-decoration: underline;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-confirm-dialog/gr-confirm-dialog.html b/polygerrit-ui/app/elements/shared/gr-confirm-dialog/gr-confirm-dialog.html
index d4a98e4..27c0355 100644
--- a/polygerrit-ui/app/elements/shared/gr-confirm-dialog/gr-confirm-dialog.html
+++ b/polygerrit-ui/app/elements/shared/gr-confirm-dialog/gr-confirm-dialog.html
@@ -33,7 +33,7 @@
header {
border-bottom: 1px solid #ddd;
flex-shrink: 0;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
main {
display: flex;
diff --git a/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands.html b/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands.html
index 05075bd..68c3848 100644
--- a/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands.html
+++ b/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands.html
@@ -40,11 +40,11 @@
display: block;
}
label {
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
li[selected] gr-button {
color: #000;
- font-weight: bold;
+ font-family: var(--font-family-bold);
text-decoration: none;
}
.schemes {
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.html b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.html
index 8f2994d..f4813fa 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.html
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.html
@@ -61,7 +61,7 @@
list-style: none;
}
ul .accountName {
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
li .accountInfo,
li .itemAction {
@@ -92,7 +92,7 @@
padding: .85em 1em;
}
.bold-text {
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
:host:not([down-arrow]) .downArrow { display: none; }
:host([down-arrow]) .downArrow {
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js
index 21aa6cb..3570081 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js
@@ -25,6 +25,14 @@
this._payloadCache = new Map();
}
+ /**
+ * Get or upgrade fetch options to include an ETag in a request.
+ * @param {string} url The URL being fetched.
+ * @param {!Object=} opt_options Optional options object in which to include
+ * the ETag request header. If omitted, the result will be a fresh option
+ * set.
+ * @return {!Object}
+ */
GrEtagDecorator.prototype.getOptions = function(url, opt_options) {
const etag = this._etags.get(url);
if (!etag) {
@@ -36,6 +44,16 @@
return options;
};
+ /**
+ * Handle a response to a request with ETag headers, potentially incorporating
+ * its result in the payload cache.
+ *
+ * @param {string} url The URL of the request.
+ * @param {!Response} response The response object.
+ * @param {string} payload The raw, unparsed JSON contained in the response
+ * body. Note: because response.text() cannot be read twice, this must be
+ * provided separately.
+ */
GrEtagDecorator.prototype.collect = function(url, response, payload) {
if (!response ||
!response.ok ||
@@ -54,19 +72,19 @@
}
};
+ /**
+ * Get the cached payload for a given URL.
+ * @param {string} url
+ * @return {string|undefined} Returns the unparsed JSON payload from the
+ * cache.
+ */
GrEtagDecorator.prototype.getCachedPayload = function(url) {
- let payload = this._payloadCache.get(url);
-
- if (typeof payload === 'object') {
- // Note: For the sake of cache transparency, deep clone the response
- // object so that cache hits are not equal object references. Some code
- // expects every network response to deserialize to a fresh object.
- payload = JSON.parse(JSON.stringify(payload));
- }
-
- return payload;
+ return this._payloadCache.get(url);
};
+ /**
+ * Limit the cache size to MAX_CACHE_SIZE.
+ */
GrEtagDecorator.prototype._truncateCache = function() {
for (const url of this._etags.keys()) {
if (this._etags.size <= MAX_CACHE_SIZE) {
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html
index 40e639e..77edae7 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html
@@ -92,25 +92,5 @@
etag.collect('/foo', fakeRequest('bar', 200), 'new payload');
assert.strictEqual(etag.getCachedPayload('/foo'), 'new payload');
});
-
- test('getCachedPayload does not preserve object equality', () => {
- const payload = {foo: 'bar'};
- etag.collect('/foo', fakeRequest('bar'), payload);
- assert.deepEqual(etag.getCachedPayload('/foo'), payload);
- assert.notStrictEqual(etag.getCachedPayload('/foo'), payload);
- etag.collect('/foo', fakeRequest('bar', 304), {foo: 'baz'});
- assert.deepEqual(etag.getCachedPayload('/foo'), payload);
- assert.notStrictEqual(etag.getCachedPayload('/foo'), payload);
- etag.collect('/foo', fakeRequest('bar', 200), {foo: 'bar baz'});
- assert.deepEqual(etag.getCachedPayload('/foo'), {foo: 'bar baz'});
- assert.notStrictEqual(etag.getCachedPayload('/foo'), {foo: 'bar baz'});
- });
-
- test('getCachedPayload clones the response deeply', () => {
- const payload = {foo: {bar: 'baz'}};
- etag.collect('/foo', fakeRequest('bar'), payload);
- assert.deepEqual(etag.getCachedPayload('/foo'), payload);
- assert.notStrictEqual(etag.getCachedPayload('/foo').foo, payload.foo);
- });
});
</script>
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 684a967..210c42f 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
@@ -186,17 +186,34 @@
* @return {?}
*/
getResponseObject(response) {
+ return this._readResponsePayload(response)
+ .then(payload => payload.parsed);
+ },
+
+ /**
+ * @param {!Object} response
+ * @return {!Object}
+ */
+ _readResponsePayload(response) {
return response.text().then(text => {
let result;
try {
- result = JSON.parse(text.substring(JSON_PREFIX.length));
+ result = this._parsePrefixedJSON(text);
} catch (_) {
result = null;
}
- return result;
+ return {parsed: result, raw: text};
});
},
+ /**
+ * @param {string} source
+ * @return {?}
+ */
+ _parsePrefixedJSON(source) {
+ return JSON.parse(source.substring(JSON_PREFIX.length));
+ },
+
getConfig() {
return this._fetchSharedCacheURL('/config/server/info');
},
@@ -820,8 +837,8 @@
this._etags.getOptions(urlWithParams))
.then(response => {
if (response && response.status === 304) {
- return Promise.resolve(
- this._etags.getCachedPayload(urlWithParams));
+ return Promise.resolve(this._parsePrefixedJSON(
+ this._etags.getCachedPayload(urlWithParams)));
}
if (response && !response.ok) {
@@ -834,13 +851,17 @@
}
const payloadPromise = response ?
- this.getResponseObject(response) :
- Promise.resolve();
- payloadPromise.then(payload => {
- this._etags.collect(urlWithParams, response, payload);
+ this._readResponsePayload(response) :
+ Promise.resolve(null);
+
+ return payloadPromise.then(payload => {
+ if (!payload) { return null; }
+
+ this._etags.collect(urlWithParams, response, payload.raw);
this._maybeInsertInLookup(payload);
+
+ return payload.parsed;
});
- return payloadPromise;
});
});
},
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
index b63258f..2058f59 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
@@ -762,25 +762,90 @@
});
});
- test('_getChangeDetail passes params to ETags decorator', () => {
- const changeNum = 4321;
- element._projectLookup[changeNum] = 'test';
- const params = {foo: 'bar'};
- const expectedUrl = '/changes/test~4321/detail?foo=bar';
- sandbox.stub(element._etags, 'getOptions');
- sandbox.stub(element._etags, 'collect');
- return element._getChangeDetail(changeNum, params).then(() => {
- assert.isTrue(element._etags.getOptions.calledWithExactly(expectedUrl));
- assert.equal(element._etags.collect.lastCall.args[0], expectedUrl);
+ suite('_getChangeDetail', () => {
+ test('_getChangeDetail passes params to ETags decorator', () => {
+ const changeNum = 4321;
+ element._projectLookup[changeNum] = 'test';
+ const params = {foo: 'bar'};
+ const expectedUrl = '/changes/test~4321/detail?foo=bar';
+ sandbox.stub(element._etags, 'getOptions');
+ sandbox.stub(element._etags, 'collect');
+ return element._getChangeDetail(changeNum, params).then(() => {
+ assert.isTrue(element._etags.getOptions.calledWithExactly(
+ expectedUrl));
+ assert.equal(element._etags.collect.lastCall.args[0], expectedUrl);
+ });
});
- });
- test('_getChangeDetail calls errFn on 500', () => {
- const errFn = sinon.stub();
- sandbox.stub(element, '_fetchRawJSON')
- .returns(Promise.resolve({ok: false, status: 500}));
- return element._getChangeDetail(123, {}, errFn).then(() => {
- assert.isTrue(errFn.called);
+ test('_getChangeDetail calls errFn on 500', () => {
+ const errFn = sinon.stub();
+ sandbox.stub(element, '_fetchRawJSON')
+ .returns(Promise.resolve({ok: false, status: 500}));
+ return element._getChangeDetail(123, {}, errFn).then(() => {
+ assert.isTrue(errFn.called);
+ });
+ });
+
+ test('_getChangeDetail populates _projectLookup', () => {
+ sandbox.stub(element, '_fetchRawJSON')
+ .returns(Promise.resolve({ok: true}));
+
+ const mockResponse = {_number: 1, project: 'test'};
+ sandbox.stub(element, '_readResponsePayload').returns(Promise.resolve({
+ parsed: mockResponse,
+ raw: JSON.stringify(mockResponse),
+ }));
+ return element._getChangeDetail(1).then(() => {
+ assert.equal(Object.keys(element._projectLookup).length, 1);
+ assert.equal(element._projectLookup[1], 'test');
+ });
+ });
+
+ suite('_getChangeDetail ETag cache', () => {
+ let requestUrl;
+ let mockResponseSerial;
+ let collectSpy;
+ let getPayloadSpy;
+
+ setup(() => {
+ requestUrl = '/foo/bar';
+ const mockResponse = {foo: 'bar', baz: 42};
+ mockResponseSerial = element.JSON_PREFIX +
+ JSON.stringify(mockResponse);
+ sandbox.stub(element, '_urlWithParams').returns(requestUrl);
+ sandbox.stub(element, 'getChangeActionURL')
+ .returns(Promise.resolve(requestUrl));
+ collectSpy = sandbox.spy(element._etags, 'collect');
+ getPayloadSpy = sandbox.spy(element._etags, 'getCachedPayload');
+ });
+
+ test('contributes to cache', () => {
+ sandbox.stub(element, '_fetchRawJSON').returns(Promise.resolve({
+ text: () => Promise.resolve(mockResponseSerial),
+ status: 200,
+ ok: true,
+ }));
+
+ return element._getChangeDetail(123, {}).then(detail => {
+ assert.isFalse(getPayloadSpy.called);
+ assert.isTrue(collectSpy.calledOnce);
+ const cachedResponse = element._etags.getCachedPayload(requestUrl);
+ assert.equal(cachedResponse, mockResponseSerial);
+ });
+ });
+
+ test('uses cache on HTTP 304', () => {
+ sandbox.stub(element, '_fetchRawJSON').returns(Promise.resolve({
+ text: () => Promise.resolve(mockResponseSerial),
+ status: 304,
+ ok: true,
+ }));
+
+ return element._getChangeDetail(123, {}).then(detail => {
+ assert.isFalse(collectSpy.called);
+ assert.isTrue(getPayloadSpy.calledOnce);
+ });
+ });
});
});
@@ -858,17 +923,6 @@
});
});
- test('getChangeDetail populates _projectLookup', () => {
- sandbox.stub(element, '_fetchRawJSON')
- .returns(Promise.resolve({ok: true}));
- sandbox.stub(element, 'getResponseObject')
- .returns(Promise.resolve({_number: 1, project: 'test'}));
- return element._getChangeDetail(1).then(() => {
- assert.equal(Object.keys(element._projectLookup).length, 1);
- assert.equal(element._projectLookup[1], 'test');
- });
- });
-
test('_getChangeURLAndFetch', () => {
element._projectLookup = {1: 'test'};
const fetchStub = sandbox.stub(element, 'fetchJSON')
@@ -886,5 +940,24 @@
'/changes/test~1/revisions/1/test'));
});
});
+
+ suite('reading responses', () => {
+ test('_readResponsePayload', () => {
+ const mockObject = {foo: 'bar', baz: 'foo'};
+ const serial = element.JSON_PREFIX + JSON.stringify(mockObject);
+ const mockResponse = {text: () => Promise.resolve(serial)};
+ return element._readResponsePayload(mockResponse).then(payload => {
+ assert.deepEqual(payload.parsed, mockObject);
+ assert.equal(payload.raw, serial);
+ });
+ });
+
+ test('_parsePrefixedJSON', () => {
+ const obj = {x: 3, y: {z: 4}, w: 23};
+ const serial = element.JSON_PREFIX + JSON.stringify(obj);
+ const result = element._parsePrefixedJSON(serial);
+ assert.deepEqual(result, obj);
+ });
+ });
});
</script>
diff --git a/polygerrit-ui/app/styles/app-theme.html b/polygerrit-ui/app/styles/app-theme.html
index db83b0c..4318757 100644
--- a/polygerrit-ui/app/styles/app-theme.html
+++ b/polygerrit-ui/app/styles/app-theme.html
@@ -30,6 +30,7 @@
--view-background-color: #fff;
--default-horizontal-margin: 1rem;
--font-family: 'Roboto', -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';
+ --font-family-bold: 'Roboto Medium', -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';
--monospace-font-family: 'Roboto Mono', Menlo, 'Lucida Console', Monaco, monospace;
--iron-overlay-backdrop: {
transition: none;
diff --git a/polygerrit-ui/app/styles/gr-change-list-styles.html b/polygerrit-ui/app/styles/gr-change-list-styles.html
index d283aac..00ea613 100644
--- a/polygerrit-ui/app/styles/gr-change-list-styles.html
+++ b/polygerrit-ui/app/styles/gr-change-list-styles.html
@@ -22,7 +22,7 @@
.topHeader,
.groupHeader {
border-bottom: 1px solid #eee;
- font-weight: bold;
+ font-family: var(--font-family-bold);
padding: .3em .5em;
}
.topHeader {
diff --git a/polygerrit-ui/app/styles/gr-form-styles.html b/polygerrit-ui/app/styles/gr-form-styles.html
index 7e0bf5a..603f610 100644
--- a/polygerrit-ui/app/styles/gr-form-styles.html
+++ b/polygerrit-ui/app/styles/gr-form-styles.html
@@ -39,7 +39,7 @@
}
.gr-form-styles .title {
color: #666;
- font-weight: bold;
+ font-family: var(--font-family-bold);
padding-right: .5em;
width: 15em;
}
diff --git a/polygerrit-ui/app/styles/gr-page-nav-styles.html b/polygerrit-ui/app/styles/gr-page-nav-styles.html
index e6e9c96..0c4d20f 100644
--- a/polygerrit-ui/app/styles/gr-page-nav-styles.html
+++ b/polygerrit-ui/app/styles/gr-page-nav-styles.html
@@ -44,14 +44,14 @@
margin-top: 1em;
}
.navStyles .title {
- font-weight: bold;
+ font-family: var(--font-family-bold);
margin: .4em 0;
}
.navStyles .selected {
background-color: #fff;
border-bottom: 1px dotted #808080;
border-top: 1px dotted #808080;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
.navStyles a {
color: black;
diff --git a/polygerrit-ui/app/styles/gr-table-styles.html b/polygerrit-ui/app/styles/gr-table-styles.html
index d296530..6b8c88d0 100644
--- a/polygerrit-ui/app/styles/gr-table-styles.html
+++ b/polygerrit-ui/app/styles/gr-table-styles.html
@@ -34,7 +34,7 @@
.genericList th {
background-color: #ddd;
border-bottom: 1px solid #eee;
- font-weight: bold;
+ font-family: var(--font-family-bold);
padding: .3em .5em;
text-align: left;
}
diff --git a/polygerrit-ui/app/styles/shared-styles.html b/polygerrit-ui/app/styles/shared-styles.html
index 9725251..5c11d60 100644
--- a/polygerrit-ui/app/styles/shared-styles.html
+++ b/polygerrit-ui/app/styles/shared-styles.html
@@ -65,15 +65,15 @@
/* Other Shared Styles*/
h1 {
font-size: 2em;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
h2 {
font-size: 1.5em;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
h3 {
font-size: 1.17em;
- font-weight: bold;
+ font-family: var(--font-family-bold);
}
/* Stopgap solution until we remove hidden$ attributes. */
[hidden] {
