Merge "Add the new gerrit systemctl file to init"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 139284c..1e87c5b 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -998,7 +998,7 @@
+
If 0 the update polling is disabled.
+
-Default is 30 seconds.
+Default is 5 minutes.
[[change.allowBlame]]change.allowBlame::
+
diff --git a/Documentation/dev-note-db.txt b/Documentation/dev-note-db.txt
index 0f6873c..e84fecf 100644
--- a/Documentation/dev-note-db.txt
+++ b/Documentation/dev-note-db.txt
@@ -32,8 +32,8 @@
- Storing the rest of account data is a work in progress.
- Storing group data is a work in progress.
-To match the current configuration of `googlesource.com`, paste the following
-config snippet in your `gerrit.config`:
+To use a configuration similar to the current state of `googlesource.com`, paste
+the following config snippet in your `gerrit.config`:
----
[noteDb "changes"]
@@ -46,6 +46,9 @@
This is the configuration that will be produced if you enable experimental
NoteDb support on a new site with `init`.
+`googlesource.com` additionally uses `fuseUpdates = true`, because its repo
+backend supports atomic multi-ref transactions. Native JGit doesn't, so setting
+this option on a dev server would fail.
For an example NoteDb change, poke around at this one:
----
@@ -99,6 +102,11 @@
implementation of the `rebuild-note-db` program does not do this. +
In this phase, it would be possible to delete the Changes tables out from
under a running server with no effect.
+- `noteDb.changes.fuseUpdates=true`: Code and meta updates within a single
+ repository are fused into a single atomic `BatchRefUpdate`, so they either
+ all succeed or all fail. This mode is only possible on a backend that
+ supports atomic ref updates, which notably excludes the default file repository
+ backend.
[[migration]]
== Migration
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 2d417c5..63dc54c 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5949,7 +5949,9 @@
|===========================
|Field Name ||Description
|`all` |optional|List of all approvals for this label as a list
-of link:#approval-info[ApprovalInfo] entities.
+of link:#approval-info[ApprovalInfo] entities. Items in this list may
+not represent actual votes cast by users; if a user votes on any label,
+a corresponding ApprovalInfo will appear in this list for all labels.
|`values` |optional|A map of all values that are allowed for this
label. The map maps the values ("`-2`", "`-1`", " `0`", "`+1`", "`+2`")
to the value descriptions.
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index fe2025c..a0e9954 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -141,7 +141,7 @@
[[check-consistency]]
=== Check Consistency
--
-'POST /config/server/check'
+'POST /config/server/check.consistency'
--
Runs consistency checks and returns detected problems.
@@ -152,7 +152,7 @@
.Request
----
- POST /config/server/check HTTP/1.0
+ POST /config/server/check.consistency HTTP/1.0
Content-Type: application/json; charset=UTF-8
{
diff --git a/WORKSPACE b/WORKSPACE
index 2e95e9a..20b8af5 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -1068,8 +1068,8 @@
bower_archive(
name = "polymer",
package = "polymer/polymer",
- sha1 = "f2563ed9c8571057814b78d8f6cf275eeb953eeb",
- version = "1.7.1",
+ sha1 = "2c7dd638d55ea91242525139cba18a308b9426d5",
+ version = "1.9.1",
)
bower_archive(
@@ -1098,8 +1098,8 @@
bower_archive(
name = "web-component-tester",
package = "web-component-tester",
- sha1 = "a4a9bc7815a22d143e8f8593e37b3c2028b8c20f",
- version = "5.0.0",
+ sha1 = "4e778f8b7d784ba2a069d83d0cd146125c5c4fcb",
+ version = "5.0.1",
)
# Bower component transitive dependencies.
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 42b1014..d3e084a 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
@@ -348,7 +348,6 @@
}
server.getTestInjector().injectMembers(this);
- notesMigration.setFromEnv();
Transport.register(inProcessProtocol);
toClose = Collections.synchronizedList(new ArrayList<Repository>());
admin = accounts.admin();
@@ -528,6 +527,7 @@
server.stop();
server = null;
}
+ notesMigration.resetFromEnv();
}
protected TestRepository<?>.CommitBuilder commitBuilder() throws Exception {
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 0ed5d8d..f99c35a 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
@@ -896,7 +896,14 @@
PushResult pr =
GitUtil.pushHead(testRepo, "refs/for/foo%base=" + rBase.getCommit().name(), false, false);
- assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done");
+
+ if (notesMigration.fuseUpdates()) {
+ // InMemoryRepository's atomic BatchRefUpdate implementation doesn't update the progress
+ // monitor. That's fine, we just care that there was at least one new change and no errors.
+ assertThat(pr.getMessages()).contains("changes: new: 1, done");
+ } else {
+ assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done");
+ }
assertTwoChangesWithSameRevision(r);
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index ab04ba5..2a4c188 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -240,7 +240,6 @@
@Test
public void uploadPackSubsetOfBranchesVisibleWithEdit() throws Exception {
allow(Permission.READ, REGISTERED_USERS, "refs/heads/master");
- deny(Permission.READ, REGISTERED_USERS, "refs/heads/branch");
Change c = notesFactory.createChecked(db, project, c1.getId()).getChange();
String changeId = c.getKey().get();
@@ -265,6 +264,34 @@
}
@Test
+ public void uploadPackSubsetOfBranchesVisibleWithEditForOtherUser() throws Exception {
+ allow(Permission.READ, REGISTERED_USERS, "refs/heads/master");
+ allow(Permission.VIEW_PRIVATE_CHANGES, REGISTERED_USERS, "refs/*");
+
+ Change c = notesFactory.createChecked(db, project, c1.getId()).getChange();
+ String changeId = c.getKey().get();
+
+ // Admin's edit is visible.
+ setApiUser(admin);
+ gApi.changes().id(changeId).edit().create();
+
+ // User's edit is visible.
+ setApiUser(user);
+ gApi.changes().id(changeId).edit().create();
+
+ assertUploadPackRefs(
+ "HEAD",
+ r1 + "1",
+ r1 + "meta",
+ r3 + "1",
+ r3 + "meta",
+ "refs/heads/master",
+ "refs/tags/master-tag",
+ "refs/users/00/1000000/edit-" + c1.getId() + "/1",
+ "refs/users/01/1000001/edit-" + c1.getId() + "/1");
+ }
+
+ @Test
public void uploadPackSubsetOfRefsVisibleWithAccessDatabase() throws Exception {
allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
try {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
index f51bbf5..351623a 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
@@ -168,7 +168,7 @@
assertThat(i.change.largeChange).isEqualTo(500);
assertThat(i.change.replyTooltip).startsWith("Reply and score");
assertThat(i.change.replyLabel).isEqualTo("Reply\u2026");
- assertThat(i.change.updateDelay).isEqualTo(30);
+ assertThat(i.change.updateDelay).isEqualTo(300);
// download
assertThat(i.download.archives).containsExactly("tar", "tbz2", "tgz", "txz");
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java
index ada17b6..4a5d496 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java
@@ -31,7 +31,6 @@
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.RepoContext;
-import com.google.gerrit.testutil.NoteDbMode;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.EnumSet;
@@ -48,11 +47,12 @@
@Before
public void setUp() throws Exception {
- assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.DISABLE_CHANGE_REVIEW_DB);
+ assume().that(notesMigration.disableChangeReviewDb()).isTrue();
}
@Test
public void updateChangeFailureRollsBackRefUpdate() throws Exception {
+ assume().that(notesMigration.fuseUpdates()).isTrue();
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/GlobalOrPluginPermission.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/access/GlobalOrPluginPermission.java
similarity index 84%
rename from gerrit-server/src/main/java/com/google/gerrit/server/permissions/GlobalOrPluginPermission.java
rename to gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/access/GlobalOrPluginPermission.java
index d2198d3..deae084 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/GlobalOrPluginPermission.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/access/GlobalOrPluginPermission.java
@@ -12,9 +12,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package com.google.gerrit.server.permissions;
+package com.google.gerrit.extensions.api.access;
-/** A {@link GlobalPermission} or a {@link PluginPermission}. */
+/**
+ * A {@link com.google.gerrit.server.permissions.GlobalPermission} or a {@link PluginPermission}.
+ */
public interface GlobalOrPluginPermission {
/** @return name used in {@code project.config} permissions. */
public String permissionName();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PluginPermission.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/access/PluginPermission.java
similarity index 97%
rename from gerrit-server/src/main/java/com/google/gerrit/server/permissions/PluginPermission.java
rename to gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/access/PluginPermission.java
index 6d503df..33a85cd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PluginPermission.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/access/PluginPermission.java
@@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package com.google.gerrit.server.permissions;
+package com.google.gerrit.extensions.api.access;
import static com.google.common.base.Preconditions.checkNotNull;
diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/AuthInfo.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/AuthInfo.java
index c0ece8c..d2e5d49 100644
--- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/AuthInfo.java
+++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/AuthInfo.java
@@ -84,7 +84,8 @@
}
public final boolean isHttpPasswordSettingsEnabled() {
- return gitBasicAuthPolicy() != GitBasicAuthPolicy.LDAP;
+ return gitBasicAuthPolicy() == GitBasicAuthPolicy.HTTP
+ || gitBasicAuthPolicy() == GitBasicAuthPolicy.HTTP_LDAP;
}
public final GitBasicAuthPolicy gitBasicAuthPolicy() {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java
index bbc192d..a948595 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java
@@ -21,6 +21,7 @@
import com.google.gerrit.client.rpc.Natives;
import com.google.gerrit.client.ui.MenuScreen;
import com.google.gerrit.common.PageLinks;
+import com.google.gerrit.extensions.client.GitBasicAuthPolicy;
import java.util.HashSet;
import java.util.Set;
@@ -46,7 +47,8 @@
if (Gerrit.info().auth().isHttpPasswordSettingsEnabled()) {
linkByGerrit(Util.C.tabHttpAccess(), PageLinks.SETTINGS_HTTP_PASSWORD);
}
- if (Gerrit.info().auth().isOAuth()) {
+ if (Gerrit.info().auth().isOAuth()
+ && Gerrit.info().auth().gitBasicAuthPolicy() == GitBasicAuthPolicy.OAUTH) {
linkByGerrit(Util.C.tabOAuthToken(), PageLinks.SETTINGS_OAUTH_TOKEN);
}
if (Gerrit.info().gerrit().editGpgKeys()) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
index 0c176e8..1e29be8 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
@@ -1564,6 +1564,10 @@
nm = JsArray.createArray().cast();
}
+ if (om.length() == nm.length()) {
+ return;
+ }
+
if (updateAvailable == null) {
updateAvailable =
new UpdateAvailableBar() {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectMap.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectMap.java
index 4327c07..5ff300d 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectMap.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectMap.java
@@ -44,9 +44,9 @@
.get(NativeMap.copyKeysIntoChildren(callback));
}
- public static void suggest(String prefix, int limit, AsyncCallback<ProjectMap> cb) {
+ public static void suggest(String match, int limit, AsyncCallback<ProjectMap> cb) {
new RestApi("/projects/")
- .addParameter("p", prefix)
+ .addParameter("m", match)
.addParameter("n", limit)
.addParameterRaw("type", "ALL")
.addParameterTrue("d") // description
diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/EventBroker.java b/gerrit-server/src/main/java/com/google/gerrit/common/EventBroker.java
index 87cfea1..96c70c0 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/common/EventBroker.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/common/EventBroker.java
@@ -16,6 +16,7 @@
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
@@ -28,6 +29,9 @@
import com.google.gerrit.server.events.ProjectEvent;
import com.google.gerrit.server.events.RefEvent;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
@@ -54,6 +58,7 @@
/** Listeners to receive all changes as they happen. */
protected final DynamicSet<EventListener> unrestrictedListeners;
+ private final PermissionBackend permissionBackend;
protected final ProjectCache projectCache;
protected final ChangeNotes.Factory notesFactory;
@@ -64,11 +69,13 @@
public EventBroker(
DynamicSet<UserScopedEventListener> listeners,
DynamicSet<EventListener> unrestrictedListeners,
+ PermissionBackend permissionBackend,
ProjectCache projectCache,
ChangeNotes.Factory notesFactory,
Provider<ReviewDb> dbProvider) {
this.listeners = listeners;
this.unrestrictedListeners = unrestrictedListeners;
+ this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.notesFactory = notesFactory;
this.dbProvider = dbProvider;
@@ -137,11 +144,12 @@
}
protected boolean isVisibleTo(Project.NameKey project, CurrentUser user) {
- ProjectState pe = projectCache.get(project);
- if (pe == null) {
+ try {
+ permissionBackend.user(user).project(project).check(ProjectPermission.ACCESS);
+ return true;
+ } catch (AuthException | PermissionBackendException e) {
return false;
}
- return pe.controlFor(user).isVisible();
}
protected boolean isVisibleTo(Change change, CurrentUser user) throws OrmException {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/Capabilities.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/Capabilities.java
index 545c57f..08eecd7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/Capabilities.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/Capabilities.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.account;
+import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
+import com.google.gerrit.extensions.api.access.PluginPermission;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ChildCollection;
@@ -23,11 +25,9 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountResource.Capability;
-import com.google.gerrit.server.permissions.GlobalOrPluginPermission;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.PluginPermission;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
index 2bba536..01e16d7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
@@ -22,14 +22,14 @@
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action;
+import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
+import com.google.gerrit.extensions.api.access.PluginPermission;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PeerDaemonUser;
import com.google.gerrit.server.git.QueueProvider;
import com.google.gerrit.server.group.SystemGroupBackend;
-import com.google.gerrit.server.permissions.GlobalOrPluginPermission;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.PluginPermission;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java
index 5276e8d..f519b6c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java
@@ -19,6 +19,8 @@
import com.google.common.collect.Iterables;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.PermissionRange;
+import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
+import com.google.gerrit.extensions.api.access.PluginPermission;
import com.google.gerrit.extensions.config.CapabilityDefinition;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -29,11 +31,9 @@
import com.google.gerrit.server.OutputFormat;
import com.google.gerrit.server.account.AccountResource.Capability;
import com.google.gerrit.server.git.QueueProvider;
-import com.google.gerrit.server.permissions.GlobalOrPluginPermission;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.PluginPermission;
import com.google.gson.reflect.TypeToken;
import com.google.inject.Inject;
import com.google.inject.Provider;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java
index 944b2cb..b7a6e82 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java
@@ -14,10 +14,13 @@
package com.google.gerrit.server.change;
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gwtorm.server.OrmException;
import java.util.Collection;
+import java.util.Optional;
/**
* Store for reviewed flags on changes.
@@ -30,6 +33,19 @@
* <p>For a multi-master setup the store must replicate the data between the masters.
*/
public interface AccountPatchReviewStore {
+
+ /** Represents patch set id with reviewed files. */
+ @AutoValue
+ abstract class PatchSetWithReviewedFiles {
+ abstract PatchSet.Id patchSetId();
+
+ abstract ImmutableSet<String> files();
+
+ public static PatchSetWithReviewedFiles create(PatchSet.Id id, ImmutableSet<String> files) {
+ return new AutoValue_AccountPatchReviewStore_PatchSetWithReviewedFiles(id, files);
+ }
+ }
+
/**
* Marks the given file in the given patch set as reviewed by the given user.
*
@@ -72,12 +88,15 @@
void clearReviewed(PatchSet.Id psId) throws OrmException;
/**
- * Returns the paths of all files in the given patch set the have been reviewed by the given user.
+ * Find the latest patch set, that is smaller or equals to the given patch set, where at least,
+ * one file has been reviewed by the given user.
*
* @param psId patch set ID
* @param accountId account ID of the user
- * @return the paths of all files in the given patch set the have been reviewed by the given user
+ * @return optionally, all files the have been reviewed by the given user that belong to the patch
+ * set that is smaller or equals to the given patch set
* @throws OrmException thrown if accessing the reviewed flags failed
*/
- Collection<String> findReviewed(PatchSet.Id psId, Account.Id accountId) throws OrmException;
+ Optional<PatchSetWithReviewedFiles> findReviewed(PatchSet.Id psId, Account.Id accountId)
+ throws OrmException;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
index b7bd75a7..ebc9971 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.change;
import com.google.common.collect.Lists;
-import com.google.common.collect.Sets;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -34,6 +33,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.change.AccountPatchReviewStore.PatchSetWithReviewedFiles;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
@@ -45,8 +45,10 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -222,31 +224,24 @@
}
Account.Id userId = user.getAccountId();
- Collection<String> r =
- accountPatchReviewStore.get().findReviewed(resource.getPatchSet().getId(), userId);
+ PatchSet patchSetId = resource.getPatchSet();
+ Optional<PatchSetWithReviewedFiles> o =
+ accountPatchReviewStore.get().findReviewed(patchSetId.getId(), userId);
- if (r.isEmpty() && 1 < resource.getPatchSet().getPatchSetId()) {
- for (PatchSet ps : reversePatchSets(resource)) {
- Collection<String> o = accountPatchReviewStore.get().findReviewed(ps.getId(), userId);
- if (!o.isEmpty()) {
- try {
- r = copy(Sets.newHashSet(o), ps.getId(), resource, userId);
- } catch (IOException | PatchListNotAvailableException e) {
- log.warn("Cannot copy patch review flags", e);
- }
- break;
- }
+ if (o.isPresent()) {
+ PatchSetWithReviewedFiles res = o.get();
+ if (res.patchSetId().equals(patchSetId.getId())) {
+ return res.files();
+ }
+
+ try {
+ return copy(res.files(), res.patchSetId(), resource, userId);
+ } catch (IOException | PatchListNotAvailableException e) {
+ log.warn("Cannot copy patch review flags", e);
}
}
- return r;
- }
-
- private List<PatchSet> reversePatchSets(RevisionResource resource) throws OrmException {
- Collection<PatchSet> patchSets = psUtil.byChange(db.get(), resource.getNotes());
- List<PatchSet> list =
- (patchSets instanceof List) ? (List<PatchSet>) patchSets : new ArrayList<>(patchSets);
- return Lists.reverse(list);
+ return Collections.emptyList();
}
private List<String> copy(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GetServerInfo.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GetServerInfo.java
index 5c09055..7450b32 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GetServerInfo.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GetServerInfo.java
@@ -216,7 +216,7 @@
info.replyLabel =
Optional.ofNullable(cfg.getString("change", null, "replyLabel")).orElse("Reply") + "\u2026";
info.updateDelay =
- (int) ConfigUtil.getTimeUnit(cfg, "change", null, "updateDelay", 30, TimeUnit.SECONDS);
+ (int) ConfigUtil.getTimeUnit(cfg, "change", null, "updateDelay", 300, TimeUnit.SECONDS);
info.submitWholeTopic = Submit.wholeTopicEnabled(cfg);
return info;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/Module.java
index 612fea2..7bf5ad5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/Module.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/Module.java
@@ -36,7 +36,7 @@
child(CONFIG_KIND, "top-menus").to(TopMenuCollection.class);
get(CONFIG_KIND, "version").to(GetVersion.class);
get(CONFIG_KIND, "info").to(GetServerInfo.class);
- post(CONFIG_KIND, "check").to(CheckConsistency.class);
+ post(CONFIG_KIND, "check.consistency").to(CheckConsistency.class);
get(CONFIG_KIND, "preferences").to(GetPreferences.class);
put(CONFIG_KIND, "preferences").to(SetPreferences.class);
get(CONFIG_KIND, "preferences.diff").to(GetDiffPreferences.class);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/extensions/webui/UiActions.java b/gerrit-server/src/main/java/com/google/gerrit/server/extensions/webui/UiActions.java
index bd5d6a4..ae15cfd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/extensions/webui/UiActions.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/extensions/webui/UiActions.java
@@ -17,6 +17,7 @@
import com.google.common.base.Predicate;
import com.google.common.collect.FluentIterable;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.RestCollection;
import com.google.gerrit.extensions.restapi.RestResource;
@@ -24,7 +25,6 @@
import com.google.gerrit.extensions.webui.PrivateInternals_UiActionDescription;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.permissions.GlobalOrPluginPermission;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 73d5383..3aaca53 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -476,7 +476,7 @@
// If the user lacks READ permission, some references may be filtered and hidden from view.
// Check objects mentioned inside the incoming pack file are reachable from visible refs.
try {
- permissions.check(ProjectPermission.READ);
+ permissionBackend.user(user).project(project.getNameKey()).check(ProjectPermission.READ);
} catch (AuthException e) {
rp.setCheckReferencedObjectsAreReachable(receiveConfig.checkReferencedObjectsAreReachable);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
index 47d416c..b58fb55 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
@@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
@@ -35,10 +36,8 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
-import java.util.HashSet;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefDatabase;
@@ -62,7 +61,7 @@
private final ReviewDb reviewDb;
private final boolean showMetadata;
private String userEditPrefix;
- private Set<Change.Id> visibleChanges;
+ private Map<Change.Id, Branch.NameKey> visibleChanges;
public VisibleRefFilter(
TagCache tagCache,
@@ -221,26 +220,29 @@
visibleChanges = visibleChangesBySearch();
}
}
- return visibleChanges.contains(changeId);
+ return visibleChanges.containsKey(changeId);
}
private boolean visibleEdit(String name) {
- if (userEditPrefix != null && name.startsWith(userEditPrefix)) {
- Change.Id id = Change.Id.fromEditRefPart(name);
- if (id != null) {
- return visible(id);
- }
+ Change.Id id = Change.Id.fromEditRefPart(name);
+ // Initialize if it wasn't yet
+ if (visibleChanges == null) {
+ visible(id);
+ }
+ if (id != null) {
+ return (userEditPrefix != null && name.startsWith(userEditPrefix) && visible(id))
+ || projectCtl.controlForRef(visibleChanges.get(id)).isEditVisible();
}
return false;
}
- private Set<Change.Id> visibleChangesBySearch() {
+ private Map<Change.Id, Branch.NameKey> visibleChangesBySearch() {
Project project = projectCtl.getProject();
try {
- Set<Change.Id> visibleChanges = new HashSet<>();
+ Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>();
for (ChangeData cd : changeCache.getChangeData(reviewDb, project.getNameKey())) {
if (projectCtl.controlForIndexedChange(cd.change()).isVisible(reviewDb, cd)) {
- visibleChanges.add(cd.getId());
+ visibleChanges.put(cd.getId(), cd.change().getDest());
}
}
return visibleChanges;
@@ -250,24 +252,24 @@
+ project.getName()
+ ", assuming no changes are visible",
e);
- return Collections.emptySet();
+ return Collections.emptyMap();
}
}
- private Set<Change.Id> visibleChangesByScan() {
+ private Map<Change.Id, Branch.NameKey> visibleChangesByScan() {
Project.NameKey project = projectCtl.getProject().getNameKey();
try {
- Set<Change.Id> visibleChanges = new HashSet<>();
+ Map<Change.Id, Branch.NameKey> visibleChanges = new HashMap<>();
for (ChangeNotes cn : changeNotesFactory.scan(db, reviewDb, project)) {
if (projectCtl.controlFor(cn).isVisible(reviewDb)) {
- visibleChanges.add(cn.getChangeId());
+ visibleChanges.put(cn.getChangeId(), cn.getChange().getDest());
}
}
return visibleChanges;
} catch (IOException | OrmException e) {
log.error(
"Cannot load changes for project " + project + ", assuming no changes are visible", e);
- return Collections.emptySet();
+ return Collections.emptyMap();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java
index 6ee3dfe..cd00149 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java
@@ -17,13 +17,11 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
-import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.Singleton;
-import java.util.Set;
import org.eclipse.jgit.lib.Config;
/**
@@ -46,30 +44,13 @@
public static final String SECTION_NOTE_DB = "noteDb";
- // All of these names must be reflected in the allowed set in checkConfig.
private static final String DISABLE_REVIEW_DB = "disableReviewDb";
+ private static final String FUSE_UPDATES = "fuseUpdates";
private static final String PRIMARY_STORAGE = "primaryStorage";
private static final String READ = "read";
private static final String SEQUENCE = "sequence";
private static final String WRITE = "write";
- private static void checkConfig(Config cfg) {
- Set<String> keys = ImmutableSet.of(CHANGES.key());
- Set<String> allowed =
- ImmutableSet.of(
- DISABLE_REVIEW_DB.toLowerCase(),
- PRIMARY_STORAGE.toLowerCase(),
- READ.toLowerCase(),
- WRITE.toLowerCase(),
- SEQUENCE.toLowerCase());
- for (String t : cfg.getSubsections(SECTION_NOTE_DB)) {
- checkArgument(keys.contains(t.toLowerCase()), "invalid NoteDb table: %s", t);
- for (String key : cfg.getNames(SECTION_NOTE_DB, t)) {
- checkArgument(allowed.contains(key.toLowerCase()), "invalid NoteDb key: %s.%s", t, key);
- }
- }
- }
-
public static Config allEnabledConfig() {
Config cfg = new Config();
cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, true);
@@ -77,6 +58,8 @@
cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, true);
cfg.setString(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.NOTE_DB.name());
cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, true);
+ // TODO(dborowitz): Set to true when FileRepository supports it.
+ cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false);
return cfg;
}
@@ -85,11 +68,10 @@
private final boolean readChangeSequence;
private final PrimaryStorage changePrimaryStorage;
private final boolean disableChangeReviewDb;
+ private final boolean fuseUpdates;
@Inject
public ConfigNotesMigration(@GerritServerConfig Config cfg) {
- checkConfig(cfg);
-
writeChanges = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, false);
readChanges = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), READ, false);
@@ -103,6 +85,7 @@
cfg.getEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB);
disableChangeReviewDb =
cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false);
+ fuseUpdates = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false);
checkArgument(
!(disableChangeReviewDb && changePrimaryStorage != PrimaryStorage.NOTE_DB),
@@ -133,4 +116,9 @@
public boolean disableChangeReviewDb() {
return disableChangeReviewDb;
}
+
+ @Override
+ public boolean fuseUpdates() {
+ return fuseUpdates;
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
index 4ecd684..83193d5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
@@ -22,6 +22,7 @@
import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
@@ -53,6 +54,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import org.eclipse.jgit.internal.JGitText;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.NullProgressMonitor;
import org.eclipse.jgit.lib.ObjectId;
@@ -516,21 +518,53 @@
if (!dryrun) {
bru.execute(or.rw, NullProgressMonitor.INSTANCE);
- boolean lockFailure = false;
- for (ReceiveCommand cmd : bru.getCommands()) {
- if (cmd.getResult() == ReceiveCommand.Result.LOCK_FAILURE) {
- lockFailure = true;
- } else if (cmd.getResult() != ReceiveCommand.Result.OK) {
- throw new IOException("Update failed: " + bru);
- }
- }
- if (lockFailure) {
- throw new LockFailureException("Update failed with one or more lock failures: " + bru);
- }
+ checkResults(bru);
}
return bru;
}
+ /**
+ * Check results of all commands in the update batch, reducing to a single exception if there was
+ * a failure.
+ *
+ * <p>Throws {@link LockFailureException} if at least one command failed with {@code
+ * LOCK_FAILURE}, and the entire transaction was aborted, i.e. any non-{@code LOCK_FAILURE}
+ * results, if there were any, failed with "transaction aborted".
+ *
+ * <p>In particular, if the underlying ref database does not {@link
+ * org.eclipse.jgit.lib.RefDatabase#performsAtomicTransactions() perform atomic transactions},
+ * then a combination of {@code LOCK_FAILURE} on one ref and {@code OK} or another result on other
+ * refs will <em>not</em> throw {@code LockFailureException}.
+ *
+ * @param bru batch update; should already have been executed.
+ * @throws LockFailureException if the transaction was aborted due to lock failure.
+ * @throws IOException if any result was not {@code OK}.
+ */
+ @VisibleForTesting
+ static void checkResults(BatchRefUpdate bru) throws LockFailureException, IOException {
+ int lockFailure = 0;
+ int aborted = 0;
+ int failure = 0;
+
+ for (ReceiveCommand cmd : bru.getCommands()) {
+ if (cmd.getResult() != ReceiveCommand.Result.OK) {
+ failure++;
+ }
+ if (cmd.getResult() == ReceiveCommand.Result.LOCK_FAILURE) {
+ lockFailure++;
+ } else if (cmd.getResult() == ReceiveCommand.Result.REJECTED_OTHER_REASON
+ && JGitText.get().transactionAborted.equals(cmd.getMessage())) {
+ aborted++;
+ }
+ }
+
+ if (lockFailure + aborted == bru.getCommands().size()) {
+ throw new LockFailureException("Update aborted with one or more lock failures: " + bru);
+ } else if (failure > 0) {
+ throw new IOException("Update failed: " + bru);
+ }
+ }
+
private void addCommands() throws OrmException, IOException {
if (isEmpty()) {
return;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
index 995847f..7b0d76f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
@@ -83,6 +83,19 @@
public abstract boolean disableChangeReviewDb();
/**
+ * Fuse meta ref updates in the same batch as code updates.
+ *
+ * <p>When set, each {@link com.google.gerrit.server.update.BatchUpdate} results in a single
+ * {@link org.eclipse.jgit.lib.BatchRefUpdate} to update both code and meta refs atomically.
+ * Setting this option with a repository backend that does not support atomic multi-ref
+ * transactions ({@link org.eclipse.jgit.lib.RefDatabase#performsAtomicTransactions()}) is a
+ * configuration error, and all updates will fail at runtime.
+ *
+ * <p>Has no effect if {@link #disableChangeReviewDb()} is false.
+ */
+ public abstract boolean fuseUpdates();
+
+ /**
* Whether to fail when reading any data from NoteDb.
*
* <p>Used in conjunction with {@link #readChanges()} for tests.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/GlobalPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/GlobalPermission.java
index 4111253..926057b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/GlobalPermission.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/GlobalPermission.java
@@ -20,6 +20,8 @@
import com.google.gerrit.extensions.annotations.CapabilityScope;
import com.google.gerrit.extensions.annotations.RequiresAnyCapability;
import com.google.gerrit.extensions.annotations.RequiresCapability;
+import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
+import com.google.gerrit.extensions.api.access.PluginPermission;
import java.lang.annotation.Annotation;
import java.util.Collections;
import java.util.LinkedHashSet;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
index f7c373e..156f106 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -19,6 +19,7 @@
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/DelegatingClassLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/DelegatingClassLoader.java
index 3908b72..daee9c7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/DelegatingClassLoader.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/DelegatingClassLoader.java
@@ -21,7 +21,7 @@
import java.util.Enumeration;
public class DelegatingClassLoader extends ClassLoader {
- public ClassLoader target;
+ private final ClassLoader target;
public DelegatingClassLoader(ClassLoader parent, ClassLoader target) {
super(parent);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultPermissionBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultPermissionBackend.java
index feaaccc..0e62c3a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultPermissionBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultPermissionBackend.java
@@ -17,11 +17,11 @@
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.common.collect.Sets;
+import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.permissions.FailedPermissionBackend;
-import com.google.gerrit.server.permissions.GlobalOrPluginPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java
index 373f561..78a78c2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java
@@ -170,6 +170,7 @@
private void deleteMultipleRefs(Repository r)
throws OrmException, IOException, ResourceConflictException {
BatchRefUpdate batchUpdate = r.getRefDatabase().newBatchUpdate();
+ batchUpdate.setAtomic(false);
List<String> refs =
prefix == null
? refsToDelete
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index d4c5edc..916637f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -76,9 +76,6 @@
/** Access control management for a user accessing a project's data. */
public class ProjectControl {
- public static final int VISIBLE = 1 << 0;
- public static final int OWNER = 1 << 1;
-
private static final Logger log = LoggerFactory.getLogger(ProjectControl.class);
public static class GenericFactory {
@@ -97,18 +94,6 @@
}
return p.controlFor(user);
}
-
- public ProjectControl validateFor(Project.NameKey nameKey, int need, CurrentUser user)
- throws NoSuchProjectException, IOException {
- final ProjectControl c = controlFor(nameKey, user);
- if ((need & VISIBLE) == VISIBLE && c.isVisible()) {
- return c;
- }
- if ((need & OWNER) == OWNER && c.isOwner()) {
- return c;
- }
- throw new NoSuchProjectException(nameKey);
- }
}
public static class Factory {
@@ -122,26 +107,6 @@
public ProjectControl controlFor(final Project.NameKey nameKey) throws NoSuchProjectException {
return userCache.get().get(nameKey);
}
-
- public ProjectControl validateFor(final Project.NameKey nameKey) throws NoSuchProjectException {
- return validateFor(nameKey, VISIBLE);
- }
-
- public ProjectControl ownerFor(final Project.NameKey nameKey) throws NoSuchProjectException {
- return validateFor(nameKey, OWNER);
- }
-
- public ProjectControl validateFor(final Project.NameKey nameKey, final int need)
- throws NoSuchProjectException {
- final ProjectControl c = controlFor(nameKey);
- if ((need & VISIBLE) == VISIBLE && c.isVisible()) {
- return c;
- }
- if ((need & OWNER) == OWNER && c.isOwner()) {
- return c;
- }
- throw new NoSuchProjectException(nameKey);
- }
}
public interface AssistedFactory {
@@ -280,21 +245,6 @@
return getProject().getState().equals(com.google.gerrit.extensions.client.ProjectState.HIDDEN);
}
- /**
- * Returns whether the project is readable to the current user. Note that the project could still
- * be hidden.
- */
- public boolean isReadable() {
- return (user.isInternalUser() || canPerformOnAnyRef(Permission.READ));
- }
-
- /**
- * Returns whether the project is accessible to the current user, i.e. readable and not hidden.
- */
- public boolean isVisible() {
- return isReadable() && !isHidden();
- }
-
public boolean canAddRefs() {
return (canPerformOnAnyRef(Permission.CREATE) || isOwnerAnyRef());
}
@@ -312,16 +262,11 @@
return false;
}
- /** Can this user see all the refs in this projects? */
- public boolean allRefsAreVisible() {
- return allRefsAreVisible(Collections.<String>emptySet());
- }
-
public boolean allRefsAreVisible(Set<String> ignore) {
return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore);
}
- /** Is this user a project owner? Ownership does not imply {@link #isVisible()} */
+ /** Is this user a project owner? */
public boolean isOwner() {
return (isDeclaredOwner() && !controlForRef("refs/*").isBlocked(Permission.OWNER))
|| user.getCapabilities().isAdmin_DoNotUse();
@@ -609,10 +554,11 @@
private boolean can(ProjectPermission perm) throws PermissionBackendException {
switch (perm) {
case ACCESS:
- return (!isHidden() && isReadable()) || isOwner();
+ return (!isHidden() && (user.isInternalUser() || canPerformOnAnyRef(Permission.READ)))
+ || isOwner();
case READ:
- return (!isHidden() && allRefsAreVisible()) || isOwner();
+ return (!isHidden() && allRefsAreVisible(Collections.emptySet())) || isOwner();
}
throw new PermissionBackendException(perm + " unsupported");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
index 0345e7a..812872c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
@@ -121,6 +121,11 @@
return isVisible;
}
+ /** Can this user see other users change edits? */
+ public boolean isEditVisible() {
+ return canViewPrivateChanges();
+ }
+
/** True if this reference is visible by all REGISTERED_USERS */
public boolean isVisibleByRegisteredUsers() {
List<PermissionRule> access = relevant.getPermission(Permission.READ);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java
index 2dd84fc..822ed6b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.schema;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.Ints;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicItem;
@@ -34,9 +35,8 @@
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
-import java.util.ArrayList;
import java.util.Collection;
-import java.util.List;
+import java.util.Optional;
import javax.sql.DataSource;
import org.apache.commons.dbcp.BasicDataSource;
import org.eclipse.jgit.lib.Config;
@@ -130,20 +130,20 @@
private static void doCreateTable(Statement stmt) throws SQLException {
stmt.executeUpdate(
- "CREATE TABLE IF NOT EXISTS ACCOUNT_PATCH_REVIEWS ("
- + "ACCOUNT_ID INTEGER DEFAULT 0 NOT NULL, "
- + "CHANGE_ID INTEGER DEFAULT 0 NOT NULL, "
- + "PATCH_SET_ID INTEGER DEFAULT 0 NOT NULL, "
- + "FILE_NAME VARCHAR(255) DEFAULT '' NOT NULL, "
- + "CONSTRAINT PRIMARY_KEY_ACCOUNT_PATCH_REVIEWS "
- + "PRIMARY KEY (ACCOUNT_ID, CHANGE_ID, PATCH_SET_ID, FILE_NAME)"
+ "CREATE TABLE IF NOT EXISTS account_patch_reviews ("
+ + "account_id INTEGER DEFAULT 0 NOT NULL, "
+ + "change_id INTEGER DEFAULT 0 NOT NULL, "
+ + "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)"
+ ")");
}
public static void dropTableIfExists(String url) throws OrmException {
try (Connection con = DriverManager.getConnection(url);
Statement stmt = con.createStatement()) {
- stmt.executeUpdate("DROP TABLE IF EXISTS ACCOUNT_PATCH_REVIEWS");
+ stmt.executeUpdate("DROP TABLE IF EXISTS account_patch_reviews");
} catch (SQLException e) {
throw convertError("create", e);
}
@@ -158,8 +158,8 @@
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
- "INSERT INTO ACCOUNT_PATCH_REVIEWS "
- + "(ACCOUNT_ID, CHANGE_ID, PATCH_SET_ID, FILE_NAME) VALUES "
+ "INSERT INTO account_patch_reviews "
+ + "(account_id, change_id, patch_set_id, file_name) VALUES "
+ "(?, ?, ?, ?)")) {
stmt.setInt(1, accountId.get());
stmt.setInt(2, psId.getParentKey().get());
@@ -186,8 +186,8 @@
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
- "INSERT INTO ACCOUNT_PATCH_REVIEWS "
- + "(ACCOUNT_ID, CHANGE_ID, PATCH_SET_ID, FILE_NAME) VALUES "
+ "INSERT INTO account_patch_reviews "
+ + "(account_id, change_id, patch_set_id, file_name) VALUES "
+ "(?, ?, ?, ?)")) {
for (String path : paths) {
stmt.setInt(1, accountId.get());
@@ -212,9 +212,9 @@
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
- "DELETE FROM ACCOUNT_PATCH_REVIEWS "
- + "WHERE ACCOUNT_ID = ? AND CHANGE_ID + ? AND "
- + "PATCH_SET_ID = ? AND FILE_NAME = ?")) {
+ "DELETE FROM account_patch_reviews "
+ + "WHERE account_id = ? AND change_id = ? AND "
+ + "patch_set_id = ? AND file_name = ?")) {
stmt.setInt(1, accountId.get());
stmt.setInt(2, psId.getParentKey().get());
stmt.setInt(3, psId.get());
@@ -230,8 +230,8 @@
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
- "DELETE FROM ACCOUNT_PATCH_REVIEWS "
- + "WHERE CHANGE_ID + ? AND PATCH_SET_ID = ?")) {
+ "DELETE FROM account_patch_reviews "
+ + "WHERE change_id = ? AND patch_set_id = ?")) {
stmt.setInt(1, psId.getParentKey().get());
stmt.setInt(2, psId.get());
stmt.executeUpdate();
@@ -241,22 +241,33 @@
}
@Override
- public Collection<String> findReviewed(PatchSet.Id psId, Account.Id accountId)
+ public Optional<PatchSetWithReviewedFiles> findReviewed(PatchSet.Id psId, Account.Id accountId)
throws OrmException {
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
- "SELECT FILE_NAME FROM ACCOUNT_PATCH_REVIEWS "
- + "WHERE ACCOUNT_ID = ? AND CHANGE_ID = ? AND PATCH_SET_ID = ?")) {
+ "SELECT patch_set_id, file_name FROM account_patch_reviews APR1 "
+ + "WHERE account_id = ? AND change_id = ? AND patch_set_id = "
+ + "(SELECT MAX(patch_set_id) FROM account_patch_reviews APR2 WHERE "
+ + "APR1.account_id = APR2.account_id "
+ + "AND APR1.change_id = APR2.change_id "
+ + "AND patch_set_id <= ?)")) {
stmt.setInt(1, accountId.get());
stmt.setInt(2, psId.getParentKey().get());
stmt.setInt(3, psId.get());
try (ResultSet rs = stmt.executeQuery()) {
- List<String> files = new ArrayList<>();
- while (rs.next()) {
- files.add(rs.getString("FILE_NAME"));
+ if (rs.next()) {
+ PatchSet.Id id = new PatchSet.Id(psId.getParentKey(), rs.getInt("PATCH_SET_ID"));
+ ImmutableSet.Builder<String> builder = ImmutableSet.builder();
+ do {
+ builder.add(rs.getString("FILE_NAME"));
+ } while (rs.next());
+
+ return Optional.of(
+ AccountPatchReviewStore.PatchSetWithReviewedFiles.create(id, builder.build()));
}
- return files;
+
+ return Optional.empty();
}
} catch (SQLException e) {
throw convertError("select", e);
@@ -267,13 +278,13 @@
switch (getSQLStateInt(err)) {
case 23001: // UNIQUE CONSTRAINT VIOLATION
case 23505: // DUPLICATE_KEY_1
- return new OrmDuplicateKeyException("ACCOUNT_PATCH_REVIEWS", err);
+ return new OrmDuplicateKeyException("account_patch_reviews", err);
default:
if (err.getCause() == null && err.getNextException() != null) {
err.initCause(err.getNextException());
}
- return new OrmException(op + " failure on ACCOUNT_PATCH_REVIEWS", err);
+ return new OrmException(op + " failure on account_patch_reviews", err);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
index 03a7c37..22329fd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -94,7 +94,8 @@
@Override
public void configure() {
factory(ReviewDbBatchUpdate.AssistedFactory.class);
- factory(NoteDbBatchUpdate.AssistedFactory.class);
+ factory(FusedNoteDbBatchUpdate.AssistedFactory.class);
+ factory(UnfusedNoteDbBatchUpdate.AssistedFactory.class);
}
};
}
@@ -103,22 +104,28 @@
public static class Factory {
private final NotesMigration migration;
private final ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory;
- private final NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory;
+ private final FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory;
+ private final UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory;
@Inject
Factory(
NotesMigration migration,
ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory,
- NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) {
+ FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory,
+ UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory) {
this.migration = migration;
this.reviewDbBatchUpdateFactory = reviewDbBatchUpdateFactory;
- this.noteDbBatchUpdateFactory = noteDbBatchUpdateFactory;
+ this.fusedNoteDbBatchUpdateFactory = fusedNoteDbBatchUpdateFactory;
+ this.unfusedNoteDbBatchUpdateFactory = unfusedNoteDbBatchUpdateFactory;
}
public BatchUpdate create(
ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when) {
if (migration.disableChangeReviewDb()) {
- return noteDbBatchUpdateFactory.create(db, project, user, when);
+ if (migration.fuseUpdates()) {
+ return fusedNoteDbBatchUpdateFactory.create(db, project, user, when);
+ }
+ return unfusedNoteDbBatchUpdateFactory.create(db, project, user, when);
}
return reviewDbBatchUpdateFactory.create(db, project, user, when);
}
@@ -138,9 +145,15 @@
// copy them into an ImmutableList so there is no chance the callee can pollute the input
// collection.
if (migration.disableChangeReviewDb()) {
- ImmutableList<NoteDbBatchUpdate> noteDbUpdates =
- (ImmutableList) ImmutableList.copyOf(updates);
- NoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun);
+ if (migration.fuseUpdates()) {
+ ImmutableList<FusedNoteDbBatchUpdate> noteDbUpdates =
+ (ImmutableList) ImmutableList.copyOf(updates);
+ FusedNoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun);
+ } else {
+ ImmutableList<UnfusedNoteDbBatchUpdate> noteDbUpdates =
+ (ImmutableList) ImmutableList.copyOf(updates);
+ UnfusedNoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun);
+ }
} else {
ImmutableList<ReviewDbBatchUpdate> reviewDbUpdates =
(ImmutableList) ImmutableList.copyOf(updates);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java
similarity index 92%
rename from gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
rename to gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java
index f7988cf..ad758484 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java
@@ -16,6 +16,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
import static java.util.Comparator.comparing;
import com.google.common.base.Throwables;
@@ -51,23 +52,25 @@
import java.util.TreeMap;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
/**
- * {@link BatchUpdate} implementation that only supports NoteDb.
+ * {@link BatchUpdate} implementation using only NoteDb that updates code refs and meta refs in a
+ * single {@link org.eclipse.jgit.lib.BatchRefUpdate}.
*
* <p>Used when {@code noteDb.changes.disableReviewDb=true}, at which point ReviewDb is not
* consulted during updates.
*/
-class NoteDbBatchUpdate extends BatchUpdate {
+class FusedNoteDbBatchUpdate extends BatchUpdate {
interface AssistedFactory {
- NoteDbBatchUpdate create(
+ FusedNoteDbBatchUpdate create(
ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when);
}
static void execute(
- ImmutableList<NoteDbBatchUpdate> updates,
+ ImmutableList<FusedNoteDbBatchUpdate> updates,
BatchUpdateListener listener,
@Nullable RequestId requestId,
boolean dryrun)
@@ -84,11 +87,11 @@
try {
switch (order) {
case REPO_BEFORE_DB:
- for (NoteDbBatchUpdate u : updates) {
+ for (FusedNoteDbBatchUpdate u : updates) {
u.executeUpdateRepo();
}
listener.afterUpdateRepos();
- for (NoteDbBatchUpdate u : updates) {
+ for (FusedNoteDbBatchUpdate u : updates) {
handles.add(u.executeChangeOps(dryrun));
}
for (ChangesHandle h : handles) {
@@ -109,10 +112,10 @@
// TODO(dborowitz): This may still result in multiple updates to All-Users, but that's
// currently not a big deal because multi-change batches generally aren't affecting
// drafts anyway.
- for (NoteDbBatchUpdate u : updates) {
+ for (FusedNoteDbBatchUpdate u : updates) {
handles.add(u.executeChangeOps(dryrun));
}
- for (NoteDbBatchUpdate u : updates) {
+ for (FusedNoteDbBatchUpdate u : updates) {
u.executeUpdateRepo();
}
for (ChangesHandle h : handles) {
@@ -147,7 +150,7 @@
u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null)));
if (!dryrun) {
- for (NoteDbBatchUpdate u : updates) {
+ for (FusedNoteDbBatchUpdate u : updates) {
u.executePostOps();
}
}
@@ -159,7 +162,7 @@
class ContextImpl implements Context {
@Override
public RepoView getRepoView() throws IOException {
- return NoteDbBatchUpdate.this.getRepoView();
+ return FusedNoteDbBatchUpdate.this.getRepoView();
}
@Override
@@ -268,7 +271,7 @@
private final ReviewDb db;
@Inject
- NoteDbBatchUpdate(
+ FusedNoteDbBatchUpdate(
GitRepositoryManager repoManager,
@GerritPersonIdent PersonIdent serverIdent,
ChangeNotes.Factory changeNotesFactory,
@@ -351,7 +354,7 @@
}
void execute() throws OrmException, IOException {
- NoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
+ FusedNoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
}
List<CheckedFuture<?, IOException>> startIndexFutures() {
@@ -382,16 +385,19 @@
private ChangesHandle executeChangeOps(boolean dryrun) throws Exception {
logDebug("Executing change ops");
initRepository();
+ Repository repo = repoView.getRepository();
+ checkState(
+ repo.getRefDatabase().performsAtomicTransactions(),
+ "cannot use noteDb.changes.fuseUpdates=true with a repository that does not support atomic"
+ + " batch ref updates: %s",
+ repo);
ChangesHandle handle =
new ChangesHandle(
updateManagerFactory
.create(project)
.setChangeRepo(
- repoView.getRepository(),
- repoView.getRevWalk(),
- repoView.getInserter(),
- repoView.getCommands()),
+ repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()),
dryrun);
if (user.isIdentifiedUser()) {
handle.manager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java
similarity index 63%
copy from gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
copy to gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java
index f7988cf..2f7de46 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java
@@ -17,9 +17,11 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static java.util.Comparator.comparing;
+import static java.util.stream.Collectors.toList;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Maps;
import com.google.common.util.concurrent.CheckedFuture;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -44,13 +46,15 @@
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.TimeZone;
import java.util.TreeMap;
+import org.eclipse.jgit.lib.NullProgressMonitor;
import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
@@ -60,14 +64,14 @@
* <p>Used when {@code noteDb.changes.disableReviewDb=true}, at which point ReviewDb is not
* consulted during updates.
*/
-class NoteDbBatchUpdate extends BatchUpdate {
+class UnfusedNoteDbBatchUpdate extends BatchUpdate {
interface AssistedFactory {
- NoteDbBatchUpdate create(
+ UnfusedNoteDbBatchUpdate create(
ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when);
}
static void execute(
- ImmutableList<NoteDbBatchUpdate> updates,
+ ImmutableList<UnfusedNoteDbBatchUpdate> updates,
BatchUpdateListener listener,
@Nullable RequestId requestId,
boolean dryrun)
@@ -78,64 +82,43 @@
setRequestIds(updates, requestId);
try {
- List<CheckedFuture<?, IOException>> indexFutures = new ArrayList<>();
- List<ChangesHandle> handles = new ArrayList<>(updates.size());
Order order = getOrder(updates, listener);
- try {
- switch (order) {
- case REPO_BEFORE_DB:
- for (NoteDbBatchUpdate u : updates) {
- u.executeUpdateRepo();
- }
- listener.afterUpdateRepos();
- for (NoteDbBatchUpdate u : updates) {
- handles.add(u.executeChangeOps(dryrun));
- }
- for (ChangesHandle h : handles) {
- h.execute();
- indexFutures.addAll(h.startIndexFutures());
- }
- listener.afterUpdateRefs();
- listener.afterUpdateChanges();
- break;
-
- case DB_BEFORE_REPO:
- // Call updateChange for each op before updateRepo, but defer executing the
- // NoteDbUpdateManager until after calling updateRepo. They share an inserter and
- // BatchRefUpdate, so it will all execute as a single batch. But we have to let
- // NoteDbUpdateManager actually execute the update, since it has to interleave it
- // properly with All-Users updates.
- //
- // TODO(dborowitz): This may still result in multiple updates to All-Users, but that's
- // currently not a big deal because multi-change batches generally aren't affecting
- // drafts anyway.
- for (NoteDbBatchUpdate u : updates) {
- handles.add(u.executeChangeOps(dryrun));
- }
- for (NoteDbBatchUpdate u : updates) {
- u.executeUpdateRepo();
- }
- for (ChangesHandle h : handles) {
- // TODO(dborowitz): This isn't quite good enough: in theory updateRepo may want to
- // see the results of change meta commands, but they aren't actually added to the
- // BatchUpdate until the body of execute. To fix this, execute needs to be split up
- // into a method that returns a BatchRefUpdate before execution. Not a big deal at the
- // moment, because this order is only used for deleting changes, and those updateRepo
- // implementations definitely don't need to observe the updated change meta refs.
- h.execute();
- indexFutures.addAll(h.startIndexFutures());
- }
- break;
- default:
- throw new IllegalStateException("invalid execution order: " + order);
- }
- } finally {
- for (ChangesHandle h : handles) {
- h.close();
- }
+ // TODO(dborowitz): Fuse implementations to use a single BatchRefUpdate between phases. Note
+ // that we may still need to respect the order, since op implementations may make assumptions
+ // about the order in which their methods are called.
+ switch (order) {
+ case REPO_BEFORE_DB:
+ for (UnfusedNoteDbBatchUpdate u : updates) {
+ u.executeUpdateRepo();
+ }
+ listener.afterUpdateRepos();
+ for (UnfusedNoteDbBatchUpdate u : updates) {
+ u.executeRefUpdates(dryrun);
+ }
+ listener.afterUpdateRefs();
+ for (UnfusedNoteDbBatchUpdate u : updates) {
+ u.reindexChanges(u.executeChangeOps(dryrun), dryrun);
+ }
+ listener.afterUpdateChanges();
+ break;
+ case DB_BEFORE_REPO:
+ for (UnfusedNoteDbBatchUpdate u : updates) {
+ u.reindexChanges(u.executeChangeOps(dryrun), dryrun);
+ }
+ for (UnfusedNoteDbBatchUpdate u : updates) {
+ u.executeUpdateRepo();
+ }
+ for (UnfusedNoteDbBatchUpdate u : updates) {
+ u.executeRefUpdates(dryrun);
+ }
+ break;
+ default:
+ throw new IllegalStateException("invalid execution order: " + order);
}
- ChangeIndexer.allAsList(indexFutures).get();
+ ChangeIndexer.allAsList(
+ updates.stream().flatMap(u -> u.indexFutures.stream()).collect(toList()))
+ .get();
// Fire ref update events only after all mutations are finished, since callers may assume a
// patch set ref being created means the change was created, or a branch advancing meaning
@@ -147,7 +130,7 @@
u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null)));
if (!dryrun) {
- for (NoteDbBatchUpdate u : updates) {
+ for (UnfusedNoteDbBatchUpdate u : updates) {
u.executePostOps();
}
}
@@ -159,7 +142,7 @@
class ContextImpl implements Context {
@Override
public RepoView getRepoView() throws IOException {
- return NoteDbBatchUpdate.this.getRepoView();
+ return UnfusedNoteDbBatchUpdate.this.getRepoView();
}
@Override
@@ -267,8 +250,10 @@
private final GitReferenceUpdated gitRefUpdated;
private final ReviewDb db;
+ private List<CheckedFuture<?, IOException>> indexFutures;
+
@Inject
- NoteDbBatchUpdate(
+ UnfusedNoteDbBatchUpdate(
GitRepositoryManager repoManager,
@GerritPersonIdent PersonIdent serverIdent,
ChangeNotes.Factory changeNotesFactory,
@@ -290,6 +275,7 @@
this.indexer = indexer;
this.gitRefUpdated = gitRefUpdated;
this.db = db;
+ this.indexFutures = new ArrayList<>();
}
@Override
@@ -323,104 +309,101 @@
onSubmitValidators.validate(
project, ctx.getRevWalk().getObjectReader(), repoView.getCommands());
}
+
+ // TODO(dborowitz): Don't flush when fusing phases.
+ if (repoView != null) {
+ logDebug("Flushing inserter");
+ repoView.getInserter().flush();
+ } else {
+ logDebug("No objects to flush");
+ }
} catch (Exception e) {
Throwables.throwIfInstanceOf(e, RestApiException.class);
throw new UpdateException(e);
}
}
- private class ChangesHandle implements AutoCloseable {
- private final NoteDbUpdateManager manager;
- private final boolean dryrun;
- private final Map<Change.Id, ChangeResult> results;
-
- ChangesHandle(NoteDbUpdateManager manager, boolean dryrun) {
- this.manager = manager;
- this.dryrun = dryrun;
- results = new HashMap<>();
+ // TODO(dborowitz): Don't execute non-change ref updates separately when fusing phases.
+ private void executeRefUpdates(boolean dryrun) throws IOException, RestApiException {
+ if (getRefUpdates().isEmpty()) {
+ logDebug("No ref updates to execute");
+ return;
+ }
+ // May not be opened if the caller added ref updates but no new objects.
+ initRepository();
+ batchRefUpdate = repoView.getRepository().getRefDatabase().newBatchUpdate();
+ repoView.getCommands().addTo(batchRefUpdate);
+ logDebug("Executing batch of {} ref updates", batchRefUpdate.getCommands().size());
+ if (dryrun) {
+ return;
}
- @Override
- public void close() {
- manager.close();
+ // Force BatchRefUpdate to read newly referenced objects using a new RevWalk, rather than one
+ // that might have access to unflushed objects.
+ try (RevWalk updateRw = new RevWalk(repoView.getRepository())) {
+ batchRefUpdate.execute(updateRw, NullProgressMonitor.INSTANCE);
}
-
- void setResult(Change.Id id, ChangeResult result) {
- ChangeResult old = results.putIfAbsent(id, result);
- checkArgument(old == null, "result for change %s already set: %s", id, old);
- }
-
- void execute() throws OrmException, IOException {
- NoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
- }
-
- List<CheckedFuture<?, IOException>> startIndexFutures() {
- if (dryrun) {
- return ImmutableList.of();
+ boolean ok = true;
+ for (ReceiveCommand cmd : batchRefUpdate.getCommands()) {
+ if (cmd.getResult() != ReceiveCommand.Result.OK) {
+ ok = false;
+ break;
}
- logDebug("Reindexing {} changes", results.size());
- List<CheckedFuture<?, IOException>> indexFutures = new ArrayList<>(results.size());
- for (Map.Entry<Change.Id, ChangeResult> e : results.entrySet()) {
- Change.Id id = e.getKey();
- switch (e.getValue()) {
- case UPSERTED:
- indexFutures.add(indexer.indexAsync(project, id));
- break;
- case DELETED:
- indexFutures.add(indexer.deleteAsync(id));
- break;
- case SKIPPED:
- break;
- default:
- throw new IllegalStateException("unexpected result: " + e.getValue());
- }
- }
- return indexFutures;
+ }
+ if (!ok) {
+ throw new RestApiException("BatchRefUpdate failed: " + batchRefUpdate);
}
}
- private ChangesHandle executeChangeOps(boolean dryrun) throws Exception {
+ private Map<Change.Id, ChangeResult> executeChangeOps(boolean dryrun) throws Exception {
logDebug("Executing change ops");
+ Map<Change.Id, ChangeResult> result =
+ Maps.newLinkedHashMapWithExpectedSize(ops.keySet().size());
initRepository();
-
- ChangesHandle handle =
- new ChangesHandle(
+ Repository repo = repoView.getRepository();
+ // TODO(dborowitz): Teach NoteDbUpdateManager to allow reusing the same inserter and batch ref
+ // update as in executeUpdateRepo.
+ try (ObjectInserter ins = repo.newObjectInserter();
+ ObjectReader reader = ins.newReader();
+ RevWalk rw = new RevWalk(reader);
+ NoteDbUpdateManager updateManager =
updateManagerFactory
.create(project)
- .setChangeRepo(
- repoView.getRepository(),
- repoView.getRevWalk(),
- repoView.getInserter(),
- repoView.getCommands()),
- dryrun);
- if (user.isIdentifiedUser()) {
- handle.manager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz));
- }
- for (Map.Entry<Change.Id, Collection<BatchUpdateOp>> e : ops.asMap().entrySet()) {
- Change.Id id = e.getKey();
- ChangeContextImpl ctx = newChangeContext(id);
- boolean dirty = false;
- logDebug("Applying {} ops for change {}", e.getValue().size(), id);
- for (BatchUpdateOp op : e.getValue()) {
- dirty |= op.updateChange(ctx);
+ .setChangeRepo(repo, rw, ins, new ChainedReceiveCommands(repo))) {
+ if (user.isIdentifiedUser()) {
+ updateManager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz));
}
- if (!dirty) {
- logDebug("No ops reported dirty, short-circuiting");
- handle.setResult(id, ChangeResult.SKIPPED);
- continue;
+ for (Map.Entry<Change.Id, Collection<BatchUpdateOp>> e : ops.asMap().entrySet()) {
+ Change.Id id = e.getKey();
+ ChangeContextImpl ctx = newChangeContext(id);
+ boolean dirty = false;
+ logDebug("Applying {} ops for change {}", e.getValue().size(), id);
+ for (BatchUpdateOp op : e.getValue()) {
+ dirty |= op.updateChange(ctx);
+ }
+ if (!dirty) {
+ logDebug("No ops reported dirty, short-circuiting");
+ result.put(id, ChangeResult.SKIPPED);
+ continue;
+ }
+ for (ChangeUpdate u : ctx.updates.values()) {
+ updateManager.add(u);
+ }
+ if (ctx.deleted) {
+ logDebug("Change {} was deleted", id);
+ updateManager.deleteChange(id);
+ result.put(id, ChangeResult.DELETED);
+ } else {
+ result.put(id, ChangeResult.UPSERTED);
+ }
}
- for (ChangeUpdate u : ctx.updates.values()) {
- handle.manager.add(u);
- }
- if (ctx.deleted) {
- logDebug("Change {} was deleted", id);
- handle.manager.deleteChange(id);
- handle.setResult(id, ChangeResult.DELETED);
- } else {
- handle.setResult(id, ChangeResult.UPSERTED);
+
+ if (!dryrun) {
+ logDebug("Executing NoteDb updates");
+ updateManager.execute();
}
}
- return handle;
+ return result;
}
private ChangeContextImpl newChangeContext(Change.Id id) throws OrmException {
@@ -440,6 +423,28 @@
return new ChangeContextImpl(ctl);
}
+ private void reindexChanges(Map<Change.Id, ChangeResult> updateResults, boolean dryrun) {
+ if (dryrun) {
+ return;
+ }
+ logDebug("Reindexing {} changes", updateResults.size());
+ for (Map.Entry<Change.Id, ChangeResult> e : updateResults.entrySet()) {
+ Change.Id id = e.getKey();
+ switch (e.getValue()) {
+ case UPSERTED:
+ indexFutures.add(indexer.indexAsync(project, id));
+ break;
+ case DELETED:
+ indexFutures.add(indexer.deleteAsync(id));
+ break;
+ case SKIPPED:
+ break;
+ default:
+ throw new IllegalStateException("unexpected result: " + e.getValue());
+ }
+ }
+ }
+
private void executePostOps() throws Exception {
ContextImpl ctx = new ContextImpl();
for (BatchUpdateOp op : ops.values()) {
diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentHtml.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentHtml.soy
index 59790dc..9adff05 100644
--- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentHtml.soy
+++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentHtml.soy
@@ -38,7 +38,17 @@
{let $ulStyle kind="css"}
list-style: none;
- padding-left: 20px;
+ padding: 0;
+ {/let}
+
+ {let $fileLiStyle kind="css"}
+ margin: 0;
+ padding: 0;
+ {/let}
+
+ {let $commentLiStyle kind="css"}
+ margin: 0;
+ padding: 0 0 0 16px;
{/let}
{let $voteStyle kind="css"}
@@ -104,14 +114,14 @@
<ul style="{$ulStyle}">
{foreach $group in $commentFiles}
- <li>
+ <li style="{$fileLiStyle}">
<p>
<a href="{$group.link}">{$group.title}:</a>
</p>
<ul style="{$ulStyle}">
{foreach $comment in $group.comments}
- <li>
+ <li style="{$commentLiStyle}">
{if $comment.isRobotComment}
<p style="{$commentHeaderStyle}">
Robot Comment from{sp}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/NoteDbUpdateManagerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/NoteDbUpdateManagerTest.java
new file mode 100644
index 0000000..12eb39d
--- /dev/null
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/NoteDbUpdateManagerTest.java
@@ -0,0 +1,119 @@
+// 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.notedb;
+
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.server.git.LockFailureException;
+import java.io.IOException;
+import java.util.function.Consumer;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.transport.ReceiveCommand;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link NoteDbUpdateManager}. */
+@RunWith(JUnit4.class)
+public class NoteDbUpdateManagerTest {
+ private static final Consumer<ReceiveCommand> OK = c -> c.setResult(ReceiveCommand.Result.OK);
+ private static final Consumer<ReceiveCommand> LOCK_FAILURE =
+ c -> c.setResult(ReceiveCommand.Result.LOCK_FAILURE);
+ private static final Consumer<ReceiveCommand> REJECTED =
+ c -> c.setResult(ReceiveCommand.Result.REJECTED_OTHER_REASON);
+ private static final Consumer<ReceiveCommand> ABORTED =
+ c -> {
+ c.setResult(ReceiveCommand.Result.NOT_ATTEMPTED);
+ ReceiveCommand.abort(ImmutableList.of(c));
+ checkState(
+ c.getResult() != ReceiveCommand.Result.NOT_ATTEMPTED
+ && c.getResult() != ReceiveCommand.Result.LOCK_FAILURE
+ && c.getResult() != ReceiveCommand.Result.OK,
+ "unexpected state after abort: %s",
+ c);
+ };
+
+ @Test
+ public void checkBatchRefUpdateResults() throws Exception {
+ checkResults(OK);
+ checkResults(OK, OK);
+
+ assertIoException(REJECTED);
+ assertIoException(OK, REJECTED);
+ assertIoException(LOCK_FAILURE, REJECTED);
+ assertIoException(LOCK_FAILURE, OK);
+ assertIoException(LOCK_FAILURE, REJECTED, OK);
+ assertIoException(LOCK_FAILURE, LOCK_FAILURE, REJECTED);
+ assertIoException(LOCK_FAILURE, ABORTED, REJECTED);
+ assertIoException(LOCK_FAILURE, ABORTED, OK);
+
+ assertLockFailureException(LOCK_FAILURE);
+ assertLockFailureException(LOCK_FAILURE, LOCK_FAILURE);
+ assertLockFailureException(LOCK_FAILURE, LOCK_FAILURE, ABORTED);
+ assertLockFailureException(LOCK_FAILURE, LOCK_FAILURE, ABORTED, ABORTED);
+ assertLockFailureException(ABORTED);
+ assertLockFailureException(ABORTED, ABORTED);
+ }
+
+ @SafeVarargs
+ private static void checkResults(Consumer<ReceiveCommand>... resultSetters) throws Exception {
+ NoteDbUpdateManager.checkResults(newBatchRefUpdate(resultSetters));
+ }
+
+ @SafeVarargs
+ private static void assertIoException(Consumer<ReceiveCommand>... resultSetters) {
+ try {
+ NoteDbUpdateManager.checkResults(newBatchRefUpdate(resultSetters));
+ assert_().fail("expected IOException");
+ } catch (IOException e) {
+ assertThat(e).isNotInstanceOf(LockFailureException.class);
+ }
+ }
+
+ @SafeVarargs
+ private static void assertLockFailureException(Consumer<ReceiveCommand>... resultSetters)
+ throws Exception {
+ try {
+ NoteDbUpdateManager.checkResults(newBatchRefUpdate(resultSetters));
+ assert_().fail("expected LockFailureException");
+ } catch (LockFailureException e) {
+ // Expected.
+ }
+ }
+
+ @SafeVarargs
+ private static BatchRefUpdate newBatchRefUpdate(Consumer<ReceiveCommand>... resultSetters) {
+ try (Repository repo = new InMemoryRepository(new DfsRepositoryDescription("repo"))) {
+ BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
+ for (int i = 0; i < resultSetters.length; i++) {
+ ReceiveCommand cmd =
+ new ReceiveCommand(
+ ObjectId.fromString(String.format("%039x1", i)),
+ ObjectId.fromString(String.format("%039x2", i)),
+ "refs/heads/branch" + i);
+ bru.addCommand(cmd);
+ resultSetters[i].accept(cmd);
+ }
+ return bru;
+ }
+ }
+}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
index 60d6f71..ebd5b49 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
@@ -58,6 +58,7 @@
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.index.SingleVersionModule.SingleVersionListener;
+import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.schema.SchemaCreator;
@@ -110,12 +111,14 @@
assertThat(u.controlForRef(ref).isOwner()).named("NOT OWN " + ref).isFalse();
}
- private void assertCanRead(ProjectControl u) {
- assertThat(u.isVisible()).named("can read").isTrue();
+ private void assertCanAccess(ProjectControl u) {
+ boolean access = u.asForProject().testOrFalse(ProjectPermission.ACCESS);
+ assertThat(access).named("can access").isTrue();
}
- private void assertCannotRead(ProjectControl u) {
- assertThat(u.isVisible()).named("cannot read").isFalse();
+ private void assertAccessDenied(ProjectControl u) {
+ boolean access = u.asForProject().testOrFalse(ProjectPermission.ACCESS);
+ assertThat(access).named("cannot access").isFalse();
}
private void assertCanRead(String ref, ProjectControl u) {
@@ -443,13 +446,13 @@
public void inheritDuplicateSections() throws Exception {
allow(parent, READ, ADMIN, "refs/*");
allow(local, READ, DEVS, "refs/heads/*");
- assertCanRead(user(local, "a", ADMIN));
+ assertCanAccess(user(local, "a", ADMIN));
local = new ProjectConfig(localKey);
local.load(newRepository(localKey));
local.getProject().setParentName(parentKey);
allow(local, READ, DEVS, "refs/*");
- assertCanRead(user(local, "d", DEVS));
+ assertCanAccess(user(local, "d", DEVS));
}
@Test
@@ -457,7 +460,7 @@
allow(parent, READ, REGISTERED_USERS, "refs/*");
deny(local, READ, REGISTERED_USERS, "refs/*");
- assertCannotRead(user(local));
+ assertAccessDenied(user(local));
}
@Test
@@ -466,7 +469,7 @@
deny(local, READ, REGISTERED_USERS, "refs/heads/*");
ProjectControl u = user(local);
- assertCanRead(u);
+ assertCanAccess(u);
assertCanRead("refs/master", u);
assertCanRead("refs/tags/foobar", u);
assertCanRead("refs/heads/master", u);
@@ -479,7 +482,7 @@
allow(local, READ, REGISTERED_USERS, "refs/heads/*");
ProjectControl u = user(local);
- assertCanRead(u);
+ assertCanAccess(u);
assertCannotRead("refs/foobar", u);
assertCannotRead("refs/tags/foobar", u);
assertCanRead("refs/heads/foobar", u);
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/update/BatchUpdateTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/update/BatchUpdateTest.java
index 892a9ff..dba3b3d 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -31,6 +31,7 @@
import com.google.gerrit.testutil.InMemoryDatabase;
import com.google.gerrit.testutil.InMemoryModule;
import com.google.gerrit.testutil.InMemoryRepositoryManager;
+import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
@@ -45,19 +46,16 @@
public class BatchUpdateTest {
@Inject private AccountManager accountManager;
-
@Inject private IdentifiedUser.GenericFactory userFactory;
-
- @Inject private InMemoryDatabase schemaFactory;
-
+ @Inject private SchemaFactory<ReviewDb> schemaFactory;
@Inject private InMemoryRepositoryManager repoManager;
-
@Inject private SchemaCreator schemaCreator;
-
@Inject private ThreadLocalRequestContext requestContext;
-
@Inject private BatchUpdate.Factory batchUpdateFactory;
+ // Only for use in setting up/tearing down injector; other users should use schemaFactory.
+ @Inject private InMemoryDatabase inMemoryDatabase;
+
private LifecycleManager lifecycle;
private ReviewDb db;
private TestRepository<InMemoryRepository> repo;
@@ -72,8 +70,10 @@
lifecycle.add(injector);
lifecycle.start();
+ try (ReviewDb underlyingDb = inMemoryDatabase.getDatabase().open()) {
+ schemaCreator.create(underlyingDb);
+ }
db = schemaFactory.open();
- schemaCreator.create(db);
Account.Id userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId();
user = userFactory.create(userId);
@@ -108,7 +108,7 @@
if (db != null) {
db.close();
}
- InMemoryDatabase.drop(schemaFactory);
+ InMemoryDatabase.drop(inMemoryDatabase);
}
@Test
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java
index 038baac..dd42b67 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java
@@ -49,8 +49,10 @@
};
public void beforeTest() throws Exception {
- notesMigration = new TestNotesMigration().setFromEnv();
+ notesMigration = new TestNotesMigration();
}
- public void afterTest() {}
+ public void afterTest() {
+ notesMigration.resetFromEnv();
+ }
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java
index 4826d9e..a90cbb9 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java
@@ -19,6 +19,8 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.RepositoryCaseMismatchException;
+import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.inject.Inject;
import java.util.HashMap;
import java.util.Map;
import java.util.SortedSet;
@@ -30,7 +32,7 @@
/** Repository manager that uses in-memory repositories. */
public class InMemoryRepositoryManager implements GitRepositoryManager {
public static InMemoryRepository newRepository(Project.NameKey name) {
- return new Repo(name);
+ return new Repo(new TestNotesMigration(), name);
}
public static class Description extends DfsRepositoryDescription {
@@ -49,11 +51,15 @@
public static class Repo extends InMemoryRepository {
private String description;
- private Repo(Project.NameKey name) {
+ private Repo(NotesMigration notesMigration, Project.NameKey name) {
super(new Description(name));
- // TODO(dborowitz): Allow atomic transactions when this is supported:
- // https://git.eclipse.org/r/#/c/61841/2/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java@313
- setPerformsAtomicTransactions(false);
+ // Normally, mimic the behavior of JGit FileRepository, the standard Gerrit repository
+ // backend, and don't support atomic ref updates. The exception is when we're testing with
+ // fused ref updates, which requires atomic ref updates to function.
+ //
+ // TODO(dborowitz): Change to match the behavior of JGit FileRepository after fixing
+ // https://bugs.eclipse.org/bugs/show_bug.cgi?id=515678
+ setPerformsAtomicTransactions(notesMigration.fuseUpdates());
}
@Override
@@ -72,7 +78,18 @@
}
}
- private Map<String, Repo> repos = new HashMap<>();
+ private final NotesMigration notesMigration;
+ private final Map<String, Repo> repos;
+
+ public InMemoryRepositoryManager() {
+ this(new TestNotesMigration());
+ }
+
+ @Inject
+ InMemoryRepositoryManager(NotesMigration notesMigration) {
+ this.notesMigration = notesMigration;
+ this.repos = new HashMap<>();
+ }
@Override
public synchronized Repo openRepository(Project.NameKey name) throws RepositoryNotFoundException {
@@ -89,7 +106,7 @@
throw new RepositoryCaseMismatchException(name);
}
} catch (RepositoryNotFoundException e) {
- repo = new Repo(name);
+ repo = new Repo(notesMigration, name);
repos.put(normalize(name), repo);
}
return repo;
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java
index 552f6f1..e8446a2 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java
@@ -35,6 +35,9 @@
/** All change tables are entirely disabled. */
DISABLE_CHANGE_REVIEW_DB(true),
+ /** All change tables are entirely disabled, and code/meta ref updates are fused. */
+ FUSED(true),
+
/**
* Run tests with NoteDb disabled, then convert ReviewDb to NoteDb and check that the results
* match.
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java
index d4ddaf1..c05dd01 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java
@@ -27,9 +27,12 @@
private volatile boolean writeChanges;
private volatile PrimaryStorage changePrimaryStorage = PrimaryStorage.REVIEW_DB;
private volatile boolean disableChangeReviewDb;
+ private volatile boolean fuseUpdates;
private volatile boolean failOnLoad;
- public TestNotesMigration() {}
+ public TestNotesMigration() {
+ resetFromEnv();
+ }
@Override
public boolean readChanges() {
@@ -53,6 +56,11 @@
return disableChangeReviewDb;
}
+ @Override
+ public boolean fuseUpdates() {
+ return fuseUpdates;
+ }
+
// Increase visbility from superclass, as tests may want to check whether
// NoteDb data is written in specific migration scenarios.
@Override
@@ -85,6 +93,11 @@
return this;
}
+ public TestNotesMigration setFuseUpdates(boolean fuseUpdates) {
+ this.fuseUpdates = fuseUpdates;
+ return this;
+ }
+
public TestNotesMigration setFailOnLoad(boolean failOnLoad) {
this.failOnLoad = failOnLoad;
return this;
@@ -94,31 +107,42 @@
return setReadChanges(enabled).setWriteChanges(enabled);
}
- public TestNotesMigration setFromEnv() {
+ public TestNotesMigration resetFromEnv() {
switch (NoteDbMode.get()) {
case READ_WRITE:
setWriteChanges(true);
setReadChanges(true);
setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
setDisableChangeReviewDb(false);
+ setFuseUpdates(false);
break;
case WRITE:
setWriteChanges(true);
setReadChanges(false);
setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
setDisableChangeReviewDb(false);
+ setFuseUpdates(false);
break;
case PRIMARY:
setWriteChanges(true);
setReadChanges(true);
setChangePrimaryStorage(PrimaryStorage.NOTE_DB);
setDisableChangeReviewDb(false);
+ setFuseUpdates(false);
break;
case DISABLE_CHANGE_REVIEW_DB:
setWriteChanges(true);
setReadChanges(true);
setChangePrimaryStorage(PrimaryStorage.NOTE_DB);
setDisableChangeReviewDb(true);
+ setFuseUpdates(false);
+ break;
+ case FUSED:
+ setWriteChanges(true);
+ setReadChanges(true);
+ setChangePrimaryStorage(PrimaryStorage.NOTE_DB);
+ setDisableChangeReviewDb(true);
+ setFuseUpdates(true);
break;
case CHECK:
case OFF:
@@ -127,6 +151,7 @@
setReadChanges(false);
setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
setDisableChangeReviewDb(false);
+ setFuseUpdates(false);
break;
}
return this;
@@ -137,6 +162,7 @@
setReadChanges(other.readChanges());
setChangePrimaryStorage(other.changePrimaryStorage());
setDisableChangeReviewDb(other.disableChangeReviewDb());
+ setFuseUpdates(other.fuseUpdates());
return this;
}
}
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/AliasCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/AliasCommand.java
index 7b7893a..0ac7765 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/AliasCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/AliasCommand.java
@@ -16,9 +16,9 @@
import com.google.common.base.Throwables;
import com.google.common.util.concurrent.Atomics;
+import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.permissions.GlobalOrPluginPermission;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
diff --git a/lib/elasticsearch/BUILD b/lib/elasticsearch/BUILD
index 8dc4bce..c40925e 100644
--- a/lib/elasticsearch/BUILD
+++ b/lib/elasticsearch/BUILD
@@ -26,9 +26,6 @@
],
)
-# Java REST client for Elasticsearch.
-VERSION = "0.1.7"
-
java_library(
name = "jest-common",
data = ["//lib:LICENSE-Apache2.0"],
diff --git a/lib/js/bower_archives.bzl b/lib/js/bower_archives.bzl
index 67cc7c0..9b96521 100644
--- a/lib/js/bower_archives.bzl
+++ b/lib/js/bower_archives.bzl
@@ -10,8 +10,8 @@
bower_archive(
name = "accessibility-developer-tools",
package = "accessibility-developer-tools",
- version = "2.11.0",
- sha1 = "792cb24b649dafb316e7e536f8ae65d0d7b52bab")
+ version = "2.12.0",
+ sha1 = "88ae82dcdeb6c658f76eff509d0ee425cae14d49")
bower_archive(
name = "async",
package = "async",
@@ -40,13 +40,13 @@
bower_archive(
name = "iron-fit-behavior",
package = "iron-fit-behavior",
- version = "1.2.5",
- sha1 = "5938815cd227843fc77ebeac480b999600a76157")
+ version = "1.2.6",
+ sha1 = "59daa8526aac59aa72b8edcbbd24d9eed555a0f5")
bower_archive(
name = "iron-flex-layout",
package = "iron-flex-layout",
- version = "1.3.1",
- sha1 = "ba696394abff5e799fc06eb11bff4720129a1b52")
+ version = "1.3.2",
+ sha1 = "b896041aad049a5e889a0165828d7b1262e32612")
bower_archive(
name = "iron-form-element-behavior",
package = "iron-form-element-behavior",
@@ -105,5 +105,5 @@
bower_archive(
name = "webcomponentsjs",
package = "webcomponentsjs",
- version = "0.7.23",
- sha1 = "3d62269e614175573b0a0f3039aab05d40f0a763")
+ version = "0.7.24",
+ sha1 = "559227f8ee9db9bfbd81989f24510cc0c1bfc65c")
diff --git a/plugins/replication b/plugins/replication
index 032d195..a6cba7b 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit 032d195285b11213821ed695ecadce51238d9bb8
+Subproject commit a6cba7b3ab4aa1e1ad0f0deb95078aacaa29b37d
diff --git a/polygerrit-ui/README.md b/polygerrit-ui/README.md
index 864e294..77d5781 100644
--- a/polygerrit-ui/README.md
+++ b/polygerrit-ui/README.md
@@ -68,9 +68,10 @@
that serves PolyGerrit:
```sh
-bazel build polygerrit && \
-java -jar bazel-bin/polygerrit.war daemon --polygerrit-dev \
--d ../gerrit_testsite --console-log --show-stack-trace
+bazel build polygerrit &&
+ $(bazel info output_base)/external/local_jdk/bin/java \
+ -jar bazel-bin/polygerrit.war daemon --polygerrit-dev \
+ -d ../gerrit_testsite --console-log --show-stack-trace
```
## Running Tests
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index e576a71..f784bcf 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -529,6 +529,9 @@
if (this.viewState.showReplyDialog) {
this._openReplyDialog();
+ // TODO(kaspern@): Find a better signal for when to call center.
+ this.async(function() { this.$.replyOverlay.center(); }, 100);
+ this.async(function() { this.$.replyOverlay.center(); }, 1000);
this.set('viewState.showReplyDialog', false);
}
}.bind(this));
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.html b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.html
index 75b98de..4287b97 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.html
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.html
@@ -76,7 +76,7 @@
selected="[[_computeIndexOfLabelValue(change.labels, permittedLabels, label)]]"
hidden$="[[!_computeAnyPermittedLabelValues(permittedLabels, label.name)]]">
<template is="dom-repeat"
- items="[[_computeBlankItems(permittedLabels, label.name)]]"
+ items="[[_computeBlankItems(permittedLabels, label.name, 'start')]]"
as="value">
<span class="placeholder"> </span>
</template>
@@ -88,7 +88,7 @@
[[value]]</gr-button>
</template>
<template is="dom-repeat"
- items="[[_computeBlankItems(permittedLabels, label.name)]]"
+ items="[[_computeBlankItems(permittedLabels, label.name, 'end')]]"
as="value">
<span class="placeholder"> </span>
</template>
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.js b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.js
index f3f374a..d0f5dd1c 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.js
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores.js
@@ -104,8 +104,8 @@
var labelValue = label.value;
var len = permittedLabels[label.name] != null ?
permittedLabels[label.name].length : 0;
- var numberBlanks = this._computeBlankItems(permittedLabels, label.name)
- .length;
+ var numberBlanks =
+ this._computeBlankItems(permittedLabels, label.name, 'start').length;
for (var i = 0; i < len; i++) {
var val = parseInt(permittedLabels[label.name][i], 10);
if (val == labelValue) {
@@ -119,10 +119,16 @@
return permittedLabels[label];
},
- _computeBlankItems: function(permittedLabels, label) {
+ _computeBlankItems: function(permittedLabels, label, side) {
+ if (!permittedLabels[label]) { return []; }
var startPosition = this._labelValues[parseInt(
permittedLabels[label][0])];
- return new Array(startPosition);
+ if (side === 'start') {
+ return new Array(startPosition);
+ }
+ var endPosition = this._labelValues[parseInt(
+ permittedLabels[label][permittedLabels[label].length - 1])];
+ return new Array(Object.keys(this._labelValues).length - endPosition - 1);
},
_computeAnyPermittedLabelValues: function(permittedLabels, label) {
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_test.html b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_test.html
index cbb4c36..c7f5a59 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_test.html
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_test.html
@@ -203,7 +203,8 @@
assert.equal(element._computeBlankItems(element.permittedLabels,
'Code-Review').length, 0);
- assert.deepEqual(element._computeBlankItems(element.permittedLabels,
+ assert.deepEqual(
+ element._computeBlankItems(element.permittedLabels,
'Verified').length, 1);
});
@@ -240,5 +241,42 @@
flushAsynchronousOperations();
assert.equal(selector.selected, 3); // Index 3, value 1
});
+
+ test('without permitted labels', function() {
+ element.permittedLabels = {
+ Verified: [
+ '-1',
+ ' 0',
+ '+1',
+ ],
+ };
+ flushAsynchronousOperations();
+ assert.isOk(element.$$('iron-selector[data-label="Verified"]'));
+ assert.isFalse(element.$$('iron-selector[data-label="Verified"]').hidden);
+ assert.isOk(element.$$('iron-selector[data-label="Code-Review"]'));
+ assert.isTrue(
+ element.$$('iron-selector[data-label="Code-Review"]').hidden);
+ });
+
+ test('asymetrical labels', function() {
+ element.permittedLabels = {
+ 'Code-Review': [
+ '-2',
+ '-1',
+ ' 0',
+ '+1',
+ '+2',
+ ],
+ Verified: [
+ ' 0',
+ '+1',
+ ],
+ };
+ flushAsynchronousOperations();
+ assert.equal(element.$$('iron-selector[data-label="Verified"]')
+ .items.length, 5);
+ assert.equal(element.$$('iron-selector[data-label="Code-Review"]')
+ .items.length, 5);
+ });
});
</script>
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js
index 487074e..affa8e5 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js
@@ -58,6 +58,33 @@
],
}];
+ var DOCUMENTATION_LINKS = [
+ {
+ url : '/index.html',
+ name : 'Table of Contents',
+ },
+ {
+ url : '/user-search.html',
+ name : 'Searching',
+ },
+ {
+ url : '/user-upload.html',
+ name : 'Uploading',
+ },
+ {
+ url : '/access-control.html',
+ name : 'Access Control',
+ },
+ {
+ url : '/rest-api.html',
+ name : 'REST API',
+ },
+ {
+ url : '/intro-project-owner.html',
+ name : 'Project Owner Guide',
+ }
+ ];
+
Polymer({
is: 'gr-main-header',
@@ -82,9 +109,13 @@
return DEFAULT_LINKS;
},
},
+ _docBaseUrl: {
+ type: String,
+ },
_links: {
type: Array,
- computed: '_computeLinks(_defaultLinks, _userLinks, _adminLinks)',
+ computed: '_computeLinks(_defaultLinks, _userLinks, _adminLinks, ' +
+ '_docBaseUrl)',
},
_loginURL: {
type: String,
@@ -106,6 +137,7 @@
attached: function() {
this._loadAccount();
+ this._loadConfig();
this.listen(window, 'location-change', '_handleLocationChange');
},
@@ -137,7 +169,7 @@
return '//' + window.location.host + this.getBaseUrl() + path;
},
- _computeLinks: function(defaultLinks, userLinks, adminLinks) {
+ _computeLinks: function(defaultLinks, userLinks, adminLinks, docBaseUrl) {
var links = defaultLinks.slice();
if (userLinks && userLinks.length > 0) {
links.push({
@@ -151,9 +183,33 @@
links: adminLinks,
});
}
+ var docLinks = this._getDocLinks(docBaseUrl, DOCUMENTATION_LINKS);
+ if (docLinks.length) {
+ links.push({
+ title: 'Documentation',
+ links: docLinks,
+ });
+ }
return links;
},
+ _getDocLinks: function(docBaseUrl, docLinks) {
+ if (!docBaseUrl || !docLinks) {
+ return [];
+ }
+ return docLinks.map(function(link) {
+ var url = docBaseUrl;
+ if (url && url[url.length - 1] === '/') {
+ url = url.substring(0, url.length - 1);
+ }
+ return {
+ url: url + link.url,
+ name: link.name,
+ target: '_blank',
+ };
+ });
+ },
+
_loadAccount: function() {
this.$.restAPI.getAccount().then(function(account) {
this._account = account;
@@ -162,6 +218,27 @@
}.bind(this));
},
+ _loadConfig: function() {
+ this.$.restAPI.getConfig().then(function(config) {
+ if (config && config.gerrit && config.gerrit.doc_url) {
+ this._docBaseUrl = config.gerrit.doc_url;
+ }
+ if (!this._docBaseUrl) {
+ return this._probeDocLink('/Documentation/index.html');
+ }
+ }.bind(this));
+ },
+
+ _probeDocLink: function(path) {
+ return this.$.restAPI.probePath(this.getBaseUrl() + path).then(function(ok) {
+ if (ok) {
+ this._docBaseUrl = this.getBaseUrl() + '/Documentation';
+ } else {
+ this._docBaseUrl = null;
+ }
+ }.bind(this));
+ },
+
_accountLoaded: function(account) {
if (!account) { return; }
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html
index eebf397..4582b4f 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html
@@ -41,6 +41,7 @@
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
getConfig: function() { return Promise.resolve({}); },
+ probePath: function(path) { return Promise.resolve(false); },
});
stub('gr-main-header', {
_loadAccount: function() {},
@@ -130,5 +131,31 @@
links: adminLinks,
}));
});
+
+ test('documentation links', function() {
+ var docLinks = [
+ {
+ name: 'Table of Contents',
+ url: '/index.html',
+ },
+ ];
+
+ assert.deepEqual(element._getDocLinks(null, docLinks), []);
+ assert.deepEqual(element._getDocLinks('', docLinks), []);
+ assert.deepEqual(element._getDocLinks('base', null), []);
+ assert.deepEqual(element._getDocLinks('base', []), []);
+
+ assert.deepEqual(element._getDocLinks('base', docLinks), [{
+ name: 'Table of Contents',
+ target: '_blank',
+ url: 'base/index.html',
+ }]);
+
+ assert.deepEqual(element._getDocLinks('base/', docLinks), [{
+ name: 'Table of Contents',
+ target: '_blank',
+ url: 'base/index.html',
+ }]);
+ });
});
</script>
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 0a555cf..e89bf05 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.html
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.html
@@ -138,7 +138,9 @@
hidden$="[[link.url]]">[[link.name]]</span>
<a
class="itemAction"
- href$="[[_computeRelativeURL(link.url)]]"
+ href$="[[_computeLinkURL(link)]]"
+ rel$="[[_computeLinkRel(link)]]"
+ target$="[[link.target]]"
hidden$="[[!link.url]]">[[link.name]]</a>
</li>
</template>
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.js b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.js
index 6e1637c..5a40e6f 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.js
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown.js
@@ -97,6 +97,17 @@
return this._computeURLHelper(host, path);
},
+ _computeLinkURL: function(link) {
+ if (link.target) {
+ return link.url;
+ }
+ return this._computeRelativeURL(link.url);
+ },
+
+ _computeLinkRel: function(link) {
+ return link.target ? 'noopener' : null;
+ },
+
_handleItemTap: function(e) {
var id = e.target.getAttribute('data-id');
var item = this.items.find(function(item) {
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_test.html b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_test.html
index f8b11ba..5b2d0486 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_test.html
@@ -49,13 +49,29 @@
assert.isTrue(element.$.dropdown.opened);
});
- test('_computeRelativeURL', function() {
+ test('_computeURLHelper', function() {
var path = '/test';
var host = 'http://www.testsite.com';
var computedPath = element._computeURLHelper(host, path);
assert.equal(computedPath, '//http://www.testsite.com/test');
});
+ test('link URLs', function() {
+ assert.equal(
+ element._computeLinkURL({url: '/test'}),
+ '//' + window.location.host + '/test');
+ assert.equal(
+ element._computeLinkURL({url: '/test', target: '_blank'}),
+ '/test');
+ });
+
+ test('link rel', function() {
+ assert.isNull(element._computeLinkRel({url: '/test'}));
+ assert.equal(
+ element._computeLinkRel({url: '/test', target: '_blank'}),
+ 'noopener');
+ });
+
test('_getClassIfBold', function() {
var bold = true;
assert.equal(element._getClassIfBold(bold), 'bold-text');
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 95fa2a7..f340184 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
@@ -583,7 +583,7 @@
body = {reviewer: reviewerID};
break;
case 'DELETE':
- url += '/' + reviewerID;
+ url += '/' + encodeURIComponent(reviewerID);
break;
default:
throw Error('Unsupported HTTP method: ' + method);
@@ -1107,5 +1107,12 @@
return this.send('DELETE',
this.getChangeActionURL(changeNum, null, '/assignee'));
},
+
+ probePath: function(path) {
+ return fetch(new Request(path, {method: 'HEAD'}))
+ .then(function(response) {
+ return response.ok;
+ });
+ },
});
})();
