Merge "Fix reply dialog dropdown obstruction"
diff --git a/Documentation/cmd-apropos.txt b/Documentation/cmd-apropos.txt
index 31d21c1..2ef71bf 100644
--- a/Documentation/cmd-apropos.txt
+++ b/Documentation/cmd-apropos.txt
@@ -15,7 +15,7 @@
from the matched documents.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-create-project.txt b/Documentation/cmd-create-project.txt
index 503bd12..4d1ea05 100644
--- a/Documentation/cmd-create-project.txt
+++ b/Documentation/cmd-create-project.txt
@@ -102,6 +102,7 @@
* FAST_FORWARD_ONLY: produces a strictly linear history.
* MERGE_IF_NECESSARY: create a merge commit when required.
* REBASE_IF_NECESSARY: rebase the commit when required.
+* REBASE_ALWAYS: always rebase the commit including dependencies.
* MERGE_ALWAYS: always create a merge commit.
* CHERRY_PICK: always cherry-pick the commit.
diff --git a/Documentation/cmd-ls-groups.txt b/Documentation/cmd-ls-groups.txt
index d8eef8b..6d4bdc5 100644
--- a/Documentation/cmd-ls-groups.txt
+++ b/Documentation/cmd-ls-groups.txt
@@ -23,7 +23,7 @@
all groups are listed.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-ls-members.txt b/Documentation/cmd-ls-members.txt
index a6d492c..273451b 100644
--- a/Documentation/cmd-ls-members.txt
+++ b/Documentation/cmd-ls-members.txt
@@ -16,7 +16,7 @@
shown tab-separated.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts. Output is either an error
diff --git a/Documentation/cmd-ls-projects.txt b/Documentation/cmd-ls-projects.txt
index e2e71ff..486ca44 100644
--- a/Documentation/cmd-ls-projects.txt
+++ b/Documentation/cmd-ls-projects.txt
@@ -25,7 +25,7 @@
group, all projects are listed.
== ACCESS
-Any user who has configured an SSH key, or by an user over HTTP.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-query.txt b/Documentation/cmd-query.txt
index 1faf1b0..90e5cdd 100644
--- a/Documentation/cmd-query.txt
+++ b/Documentation/cmd-query.txt
@@ -108,7 +108,7 @@
will be used to cut the result set.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-receive-pack.txt b/Documentation/cmd-receive-pack.txt
index 798f872..b62b9a9 100644
--- a/Documentation/cmd-receive-pack.txt
+++ b/Documentation/cmd-receive-pack.txt
@@ -37,7 +37,7 @@
Deprecated, use `refs/for/branch%cc=address` instead.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== EXAMPLES
diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt
index 4e24701..8f40d6c 100644
--- a/Documentation/cmd-review.txt
+++ b/Documentation/cmd-review.txt
@@ -150,7 +150,7 @@
invocations of the SSH command are required.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-set-members.txt b/Documentation/cmd-set-members.txt
index ae44843..5fb2bb9 100644
--- a/Documentation/cmd-set-members.txt
+++ b/Documentation/cmd-set-members.txt
@@ -49,7 +49,7 @@
order: `--remove`, `--exclude`, `--add`, `--include`
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-set-project.txt b/Documentation/cmd-set-project.txt
index 62d6e92..7282e28 100644
--- a/Documentation/cmd-set-project.txt
+++ b/Documentation/cmd-set-project.txt
@@ -53,6 +53,7 @@
* FAST_FORWARD_ONLY: produces a strictly linear history.
* MERGE_IF_NECESSARY: create a merge commit when required.
* REBASE_IF_NECESSARY: rebase the commit when required.
+* REBASE_ALWAYS: always rebase the commit including dependencies.
* MERGE_ALWAYS: always create a merge commit.
* CHERRY_PICK: always cherry-pick the commit.
diff --git a/Documentation/cmd-set-reviewers.txt b/Documentation/cmd-set-reviewers.txt
index 3d53456..0a757fd 100644
--- a/Documentation/cmd-set-reviewers.txt
+++ b/Documentation/cmd-set-reviewers.txt
@@ -47,7 +47,7 @@
Display site-specific usage information
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/cmd-version.txt b/Documentation/cmd-version.txt
index cc797cc..85b0491 100644
--- a/Documentation/cmd-version.txt
+++ b/Documentation/cmd-version.txt
@@ -26,7 +26,7 @@
`<n>` is computed.
== ACCESS
-Any user who has configured an SSH key.
+Any user who has SSH access to Gerrit.
== SCRIPTING
This command is intended to be used in scripts.
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index a82370c..fccc32e 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -3650,7 +3650,9 @@
+
The default submit type for newly created projects. Supported values
are `MERGE_IF_NECESSARY`, `FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`,
-`MERGE_ALWAYS` and `CHERRY_PICK`.
+`REBASE_ALWAYS`, `MERGE_ALWAYS` and `CHERRY_PICK`.
++
+For more details see link:project-configuration.html#submit_type[Submit Types].
+
By default, `MERGE_IF_NECESSARY`.
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index e4b9a83..532b8c42 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -11,11 +11,10 @@
[[label_Code-Review]]
== Label: Code-Review
-The code review label is the second of two default labels that is
-configured upon the creation of a Gerrit instance. It may have any
-meaning the project desires. It was originally invented by the Android
-Open Source Project to mean 'I read the code and it seems reasonably
-correct'.
+The Code-Review label is configured upon the creation of a Gerrit
+instance. It may have any meaning the project desires. It was
+originally invented by the Android Open Source Project to mean
+'I read the code and it seems reasonably correct'.
The range of values is:
@@ -87,8 +86,10 @@
Project to mean 'compiles, passes basic unit tests'. Some CI tools
expect to use the Verified label to vote on a change after running.
-Administrators can install the Verified label by adding the following
-text to `project.config`:
+During site initialization the administrator may have chosen to
+configure the default Verified label for all projects. In case it is
+desired to configure it at a later time, administrators can do this by
+adding the following to `project.config` in `All-Projects`:
----
[label "Verified"]
@@ -96,6 +97,7 @@
value = -1 Fails
value = 0 No score
value = +1 Verified
+ copyAllScoresIfNoCodeChange = true
----
The range of values is:
@@ -315,8 +317,8 @@
the commit message is different. This can be used to enable sticky
approvals on labels that only depend on the code, reducing turn-around
if only the commit message is changed prior to submitting a change.
-For the Verified label that is installed by the link:pgm-init.html[init]
-site program this is enabled by default.
+For the Verified label that is optionally installed by the
+link:pgm-init.html[init] site program this is enabled by default.
Defaults to false.
diff --git a/Documentation/error-permission-denied.txt b/Documentation/error-permission-denied.txt
index 574818d..879273d 100644
--- a/Documentation/error-permission-denied.txt
+++ b/Documentation/error-permission-denied.txt
@@ -3,15 +3,20 @@
With this error message an SSH command to Gerrit is rejected if the
SSH authentication is not successful.
-The link:http://en.wikipedia.org/wiki/Secure_Shell[SSH] protocol uses link:http://en.wikipedia.org/wiki/Public-key_cryptography[Public-key Cryptography] for authentication.
-This means for a successful SSH authentication you need your private
-SSH key and the corresponding public SSH key must be known to Gerrit.
+The link:http://en.wikipedia.org/wiki/Secure_Shell[SSH] protocol can use
+link:http://en.wikipedia.org/wiki/Public-key_cryptography[Public-key Cryptography]
+for authentication.
+In general configurations, Gerrit will authenticate you by the public keys
+known to you. Optionally, it can be configured by the administrator to allow
+for link:config-gerrit.html#sshd.kerberosKeytab[kerberos] authentication
+instead.
-If you are facing this problem, do the following:
+In any case, verify that you are using the correct username for the SSH command
+and that it is typed correctly (case sensitive). You can look up your username
+in the Gerrit Web UI under 'Settings' -> 'Profile'.
-. Verify that you are using the correct username for the SSH command
- and that it is typed correctly (case sensitive). You can look up
- your username in the Gerrit Web UI under 'Settings' -> 'Profile'.
+If you are facing this problem and using an SSH keypair, do the following:
+
. Verify that you have uploaded your public SSH key for your Gerrit
account. To do this go in the Gerrit Web UI to 'Settings' ->
'SSH Public Keys' and check that your public SSH key is there. If
@@ -21,6 +26,19 @@
described below. From the trace you should see which private SSH
key is used.
+Debugging kerberos issues can be quite hard given the complexity of the
+protocol. In case you are using kerberos authentication, do the following:
+
+. Verify that you have acquired a valid initial ticket. On a Linux machine, you
+ can acquire one using the `kinit` command. List all your tickets using the
+ `klist` command. It should list all principals for which you have acquired a
+ ticket and include a principal name corresponding to your Gerrit server, for
+ example `HOST/gerrit.mydomain.tld@MYDOMAIN.TLD`.
+ Note that tickets can expire and require you to re-run `kinit` periodically.
+. Verify that your SSH client is using kerberos authentication. For OpenSSH
+ clients this can be controlled using the `GSSAPIAuthentication` setting.
+ For more information see
+ link:user-upload.html#configure_ssh_kerberos[SSH kerberos configuration].
== Test SSH authentication
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt
index 9aa0a3b..948ec25 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -56,8 +56,8 @@
and the link:user-upload.html#http[HTTP/HTTPS] protocols.
[NOTE]
-To use SSH you must link:user-upload.html#configure_ssh[generate an SSH
-key pair and upload the public SSH key to Gerrit].
+To use SSH you may need to link:user-upload.html#ssh[configure your SSH public
+key in your `Settings`].
[[code-review]]
== Code Review Workflow
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index 8cddce2..901f15a 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -86,6 +86,10 @@
* `batch_update/execute_change_ops`: BatchUpdate change update latency,
excluding reindexing
+* `batch_update/retry_attempt_counts`: Distribution of number of attempts made
+by RetryHelper (1 == single attempt, no retry)
+* `batch_update/retry_timeout_count`: Number of executions of RetryHelper that
+ultimately timed out
=== NoteDb
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index cb161af..09fea83 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6418,7 +6418,7 @@
|Field Name ||Description
|`submit_type` ||
Submit type used for this change, can be `MERGE_IF_NECESSARY`,
-`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `MERGE_ALWAYS` or
+`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `REBASE_ALWAYS`, `MERGE_ALWAYS` or
`CHERRY_PICK`.
|`strategy` |optional|
The strategy of the merge, can be `recursive`, `resolve`,
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index 54682ed..7ee7336 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -2582,7 +2582,7 @@
MaxObjectSizeLimitInfo] entity.
|`submit_type` ||
The default submit type of the project, can be `MERGE_IF_NECESSARY`,
-`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `MERGE_ALWAYS` or
+`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `REBASE_ALWAYS`, `MERGE_ALWAYS` or
`CHERRY_PICK`.
|`match_author_to_committer_date` |optional|
link:#inherited-boolean-info[InheritedBooleanInfo] that indicates whether
@@ -2660,7 +2660,7 @@
If not set, this setting is not updated.
|`submit_type` |optional|
The default submit type of the project, can be `MERGE_IF_NECESSARY`,
-`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `MERGE_ALWAYS` or
+`FAST_FORWARD_ONLY`, `REBASE_IF_NECESSARY`, `REBASE_ALWAYS`, `MERGE_ALWAYS` or
`CHERRY_PICK`. +
If not set, the submit type is not updated.
|`state` |optional|
@@ -2966,8 +2966,8 @@
Whether an empty initial commit should be created.
|`submit_type` |optional|
The submit type that should be set for the project
-(`MERGE_IF_NECESSARY`, `REBASE_IF_NECESSARY`, `FAST_FORWARD_ONLY`,
-`MERGE_ALWAYS`, `CHERRY_PICK`). +
+(`MERGE_IF_NECESSARY`, `REBASE_IF_NECESSARY`, `REBASE_ALWAYS`,
+`FAST_FORWARD_ONLY`, `MERGE_ALWAYS`, `CHERRY_PICK`). +
If not set, `MERGE_IF_NECESSARY` is set as submit type unless
link:config-gerrit.html#repository.name.defaultSubmitType[
repository.<name>.defaultSubmitType] is set to a different value.
diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt
index deec660..cb00a84 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -9,8 +9,8 @@
All three methods rely on authentication, which must first be configured
by the uploading user.
-Gerrit supports two methods of authenticating the uploading user. SSH
-public key, and HTTP/HTTPS.
+Gerrit supports two protocols for uploading changes; SSH and HTTP/HTTPS. These
+may not all be available for you, depending on the server configuration.
[[http]]
== HTTP/HTTPS
@@ -41,13 +41,15 @@
[[ssh]]
== SSH
-Each user uploading changes to Gerrit must configure one or more SSH
-public keys. The per-user SSH key list can be accessed over the web
-within Gerrit by `Settings`, and then accessing the `SSH Public Keys`
-tab.
+To upload changes over SSH, Gerrit supports two forms of authentication: a
+user's public key or kerberos.
-[[configure_ssh]]
-=== Configuration
+Unless your Gerrit instance is configured to support
+link:config-gerrit.html#sshd.kerberosKeytab[kerberos] in your domain, only
+public key authentication can be used.
+
+[[configure_ssh_public_keys]]
+=== Public keys
To register a new SSH key for use with Gerrit, paste the contents of
your `id_rsa.pub` or `id_dsa.pub` file into the text box and click
@@ -79,10 +81,29 @@
documentation, for more details on configuration of the agent
process and how to add the private key.
+[[configure_ssh_kerberos]]
+=== Kerberos
+
+A kerberos-enabled server configuration allows for zero configuration in an
+existing single-sign-on environment.
+
+Your SSH client should be configured to enable kerberos authentication. For
+OpenSSH clients, this is controlled by the option `GSSAPIAuthentication` which
+should be set to `yes`.
+
+Some Linux distributions have packaged OpenSSH to enable this by default (e.g.
+Debian, Ubuntu). If this is not the case for your distribution, enable it for
+Gerrit with this entry in your local SSH configuration:
+
+----
+Host gerrit.mydomain.tld
+ GSSAPIAuthentication yes
+----
+
[[test_ssh]]
=== Testing Connections
-To verify your SSH key is working correctly, try using an SSH client
+To verify your SSH authentication is working correctly, try using an SSH client
to connect to Gerrit's SSHD port. By default Gerrit runs on
port 29418, using the same hostname as the web server:
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 6733380..6a1e3b9 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
@@ -1134,6 +1134,12 @@
return name;
}
+ protected String createAccount(String name, String group) throws Exception {
+ name = name(name);
+ accountCreator.create(name, group);
+ return name;
+ }
+
protected RevCommit getHead(Repository repo, String name) throws Exception {
try (RevWalk rw = new RevWalk(repo)) {
Ref r = repo.exactRef(name);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index f472b50..0d68f4a 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -31,6 +31,7 @@
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import static org.junit.Assert.fail;
@@ -1313,6 +1314,26 @@
}
}
+ @Test
+ public void groups() throws Exception {
+ assertGroups(
+ admin.username, ImmutableList.of("Anonymous Users", "Registered Users", "Administrators"));
+
+ //TODO: update when test user is fixed to be included in "Anonymous Users" and
+ // "Registered Users" groups
+ assertGroups(user.username, ImmutableList.of());
+
+ String group = createGroup("group");
+ String newUser = createAccount("user1", group);
+ assertGroups(newUser, ImmutableList.of(group));
+ }
+
+ private void assertGroups(String user, List<String> expected) throws Exception {
+ List<String> actual =
+ gApi.accounts().id(user).getGroups().stream().map(g -> g.name).collect(toList());
+ assertThat(actual).containsExactlyElementsIn(expected);
+ }
+
private void assertSequenceNumbers(List<SshKeyInfo> sshKeys) {
int seq = 1;
for (SshKeyInfo key : sshKeys) {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 320faa0..69315a2 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -1797,7 +1797,7 @@
setApiUser(user);
exception.expect(AuthException.class);
- exception.expectMessage("delete reviewer not permitted");
+ exception.expectMessage("remove reviewer not permitted");
gApi.changes().id(r.getChangeId()).reviewer(admin.getId().toString()).remove();
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 26383e5..1b5e544a 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -687,12 +687,6 @@
return groupCache.get(new AccountGroup.NameKey(name));
}
- private String createAccount(String name, String group) throws Exception {
- name = name(name);
- accountCreator.create(name, group);
- return name;
- }
-
private void setCreatedOnToNull(AccountGroup.UUID groupUuid) throws Exception {
groupsUpdateProvider.get().updateGroup(db, groupUuid, group -> group.setCreatedOn(null));
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
index b88097c..912ad64 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/AccountApi.java
@@ -25,6 +25,7 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.EmailInfo;
import com.google.gerrit.extensions.common.GpgKeyInfo;
+import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.SshKeyInfo;
import com.google.gerrit.extensions.restapi.NotImplementedException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -69,6 +70,8 @@
List<ChangeInfo> getStarredChanges() throws RestApiException;
+ List<GroupInfo> getGroups() throws RestApiException;
+
List<EmailInfo> getEmails() throws RestApiException;
void addEmail(EmailInput input) throws RestApiException;
@@ -197,6 +200,11 @@
}
@Override
+ public List<GroupInfo> getGroups() throws RestApiException {
+ throw new NotImplementedException();
+ }
+
+ @Override
public List<EmailInfo> getEmails() throws RestApiException {
throw new NotImplementedException();
}
diff --git a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
index c86d804..7044547 100644
--- a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
+++ b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
@@ -37,6 +37,17 @@
}
@Test
+ public void changeRefs() throws Exception {
+ String changeMetaRef = RefNames.changeMetaRef(changeId);
+ assertThat(changeMetaRef).isEqualTo("refs/changes/73/67473/meta");
+ assertThat(RefNames.isNoteDbMetaRef(changeMetaRef)).isTrue();
+
+ String robotCommentsRef = RefNames.robotCommentsRef(changeId);
+ assertThat(robotCommentsRef).isEqualTo("refs/changes/73/67473/robot-comments");
+ assertThat(RefNames.isNoteDbMetaRef(robotCommentsRef)).isTrue();
+ }
+
+ @Test
public void refsUsers() throws Exception {
assertThat(RefNames.refsUsers(accountId)).isEqualTo("refs/users/23/1011123");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
index 64760a65..f8539d9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
@@ -33,6 +33,7 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.EmailInfo;
import com.google.gerrit.extensions.common.GpgKeyInfo;
+import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.SshKeyInfo;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.Response;
@@ -54,6 +55,7 @@
import com.google.gerrit.server.account.GetEditPreferences;
import com.google.gerrit.server.account.GetEmails;
import com.google.gerrit.server.account.GetExternalIds;
+import com.google.gerrit.server.account.GetGroups;
import com.google.gerrit.server.account.GetPreferences;
import com.google.gerrit.server.account.GetSshKeys;
import com.google.gerrit.server.account.GetWatchedProjects;
@@ -70,6 +72,7 @@
import com.google.gerrit.server.account.Stars;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.List;
@@ -116,6 +119,7 @@
private final GetExternalIds getExternalIds;
private final DeleteExternalIds deleteExternalIds;
private final PutStatus putStatus;
+ private final GetGroups getGroups;
@Inject
AccountApiImpl(
@@ -153,6 +157,7 @@
GetExternalIds getExternalIds,
DeleteExternalIds deleteExternalIds,
PutStatus putStatus,
+ GetGroups getGroups,
@Assisted AccountResource account) {
this.account = account;
this.accountLoaderFactory = ailf;
@@ -189,6 +194,7 @@
this.getExternalIds = getExternalIds;
this.deleteExternalIds = deleteExternalIds;
this.putStatus = putStatus;
+ this.getGroups = getGroups;
}
@Override
@@ -363,6 +369,15 @@
}
@Override
+ public List<GroupInfo> getGroups() throws RestApiException {
+ try {
+ return getGroups.apply(account);
+ } catch (OrmException e) {
+ throw asRestApiException("Cannot get groups", e);
+ }
+ }
+
+ @Override
public List<EmailInfo> getEmails() {
return getEmails.apply(account);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
index d400999..ac7248b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -57,9 +57,7 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.project.ProjectControl;
-import com.google.gerrit.server.project.RefControl;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -94,7 +92,7 @@
private static final Logger log = LoggerFactory.getLogger(ChangeInserter.class);
private final PermissionBackend permissionBackend;
- private final ProjectControl.GenericFactory projectControlFactory;
+ private final ProjectCache projectCache;
private final IdentifiedUser.GenericFactory userFactory;
private final ChangeControl.GenericFactory changeControlFactory;
private final PatchSetInfoFactory patchSetInfoFactory;
@@ -144,7 +142,7 @@
@Inject
ChangeInserter(
PermissionBackend permissionBackend,
- ProjectControl.GenericFactory projectControlFactory,
+ ProjectCache projectCache,
IdentifiedUser.GenericFactory userFactory,
ChangeControl.GenericFactory changeControlFactory,
PatchSetInfoFactory patchSetInfoFactory,
@@ -161,7 +159,7 @@
@Assisted ObjectId commitId,
@Assisted String refName) {
this.permissionBackend = permissionBackend;
- this.projectControlFactory = projectControlFactory;
+ this.projectCache = projectCache;
this.userFactory = userFactory;
this.changeControlFactory = changeControlFactory;
this.patchSetInfoFactory = patchSetInfoFactory;
@@ -567,24 +565,25 @@
PermissionBackend.ForRef perm =
permissionBackend.user(ctx.getUser()).project(ctx.getProject()).ref(refName);
try {
- RefControl refControl =
- projectControlFactory.controlFor(ctx.getProject(), ctx.getUser()).controlForRef(refName);
try (CommitReceivedEvent event =
new CommitReceivedEvent(
cmd,
- refControl.getProjectControl().getProject(),
+ projectCache.checkedGet(ctx.getProject()).getProject(),
change.getDest().get(),
ctx.getRevWalk().getObjectReader(),
commitId,
ctx.getIdentifiedUser())) {
commitValidatorsFactory
- .forGerritCommits(perm, refControl, new NoSshInfo(), ctx.getRevWalk())
+ .forGerritCommits(
+ perm,
+ new Branch.NameKey(ctx.getProject(), refName),
+ ctx.getIdentifiedUser(),
+ new NoSshInfo(),
+ ctx.getRevWalk())
.validate(event);
}
} catch (CommitValidationException e) {
throw new ResourceConflictException(e.getFullMessage());
- } catch (NoSuchProjectException e) {
- throw new ResourceConflictException(e.getMessage());
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 6471788..3ceeb24 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -116,7 +116,9 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
@@ -212,6 +214,7 @@
private final ChangeKindCache changeKindCache;
private final ChangeIndexCollection indexes;
private final ApprovalsUtil approvalsUtil;
+ private final RemoveReviewerControl removeReviewerControl;
private boolean lazyLoad = true;
private AccountLoader accountLoader;
@@ -243,6 +246,7 @@
ChangeKindCache changeKindCache,
ChangeIndexCollection indexes,
ApprovalsUtil approvalsUtil,
+ RemoveReviewerControl removeReviewerControl,
@Assisted Iterable<ListChangesOption> options) {
this.db = db;
this.userProvider = user;
@@ -267,6 +271,7 @@
this.changeKindCache = changeKindCache;
this.indexes = indexes;
this.approvalsUtil = approvalsUtil;
+ this.removeReviewerControl = removeReviewerControl;
this.options = Sets.immutableEnumSet(options);
}
@@ -1100,7 +1105,8 @@
return result;
}
- private Collection<AccountInfo> removableReviewers(ChangeControl ctl, ChangeInfo out) {
+ private Collection<AccountInfo> removableReviewers(ChangeControl ctl, ChangeInfo out)
+ throws PermissionBackendException, NoSuchChangeException {
// Although this is called removableReviewers, this method also determines
// which CCs are removable.
//
@@ -1120,7 +1126,9 @@
}
for (ApprovalInfo ai : label.all) {
Account.Id id = new Account.Id(ai._accountId);
- if (ctl.canRemoveReviewer(id, MoreObjects.firstNonNull(ai.value, 0))) {
+
+ if (removeReviewerControl.testRemoveReviewer(
+ ctl.getNotes(), ctl.getUser(), id, MoreObjects.firstNonNull(ai.value, 0))) {
removable.add(id);
} else {
fixed.add(id);
@@ -1137,7 +1145,7 @@
for (AccountInfo ai : ccs) {
if (ai._accountId != null) {
Account.Id id = new Account.Id(ai._accountId);
- if (ctl.canRemoveReviewer(id, 0)) {
+ if (removeReviewerControl.testRemoveReviewer(ctl.getNotes(), ctl.getUser(), id, 0)) {
removable.add(id);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java
index 930cb8b..f3c5f0a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java
@@ -22,6 +22,7 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.webui.UiAction;
+import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.CurrentUser;
@@ -32,6 +33,7 @@
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
@@ -70,7 +72,7 @@
public ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, RevisionResource rsrc, CherryPickInput input)
throws OrmException, IOException, UpdateException, RestApiException,
- PermissionBackendException, ConfigInvalidException {
+ PermissionBackendException, ConfigInvalidException, NoSuchProjectException {
input.parent = input.parent == null ? 1 : input.parent;
if (input.message == null || input.message.trim().isEmpty()) {
throw new BadRequestException("message must be non-empty");
@@ -93,7 +95,7 @@
rsrc.getChange(),
rsrc.getPatchSet(),
input,
- rsrc.getControl().getProjectControl().controlForRef(refName));
+ new Branch.NameKey(rsrc.getProject(), refName));
return json.noOptions().format(rsrc.getProject(), cherryPickedChangeId);
} catch (InvalidChangeOperationException e) {
throw new BadRequestException(e.getMessage());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
index deb379d..6e555e5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
@@ -31,7 +31,6 @@
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
@@ -51,8 +50,9 @@
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.update.BatchUpdate;
@@ -94,6 +94,7 @@
private final PatchSetInserter.Factory patchSetInserterFactory;
private final MergeUtil.Factory mergeUtilFactory;
private final ChangeNotes.Factory changeNotesFactory;
+ private final ProjectControl.GenericFactory projectControlFactory;
private final ApprovalsUtil approvalsUtil;
private final ChangeMessagesUtil changeMessagesUtil;
private final PatchSetUtil psUtil;
@@ -111,6 +112,7 @@
PatchSetInserter.Factory patchSetInserterFactory,
MergeUtil.Factory mergeUtilFactory,
ChangeNotes.Factory changeNotesFactory,
+ ProjectControl.GenericFactory projectControlFactory,
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil changeMessagesUtil,
PatchSetUtil psUtil,
@@ -125,6 +127,7 @@
this.patchSetInserterFactory = patchSetInserterFactory;
this.mergeUtilFactory = mergeUtilFactory;
this.changeNotesFactory = changeNotesFactory;
+ this.projectControlFactory = projectControlFactory;
this.approvalsUtil = approvalsUtil;
this.changeMessagesUtil = changeMessagesUtil;
this.psUtil = psUtil;
@@ -136,9 +139,9 @@
Change change,
PatchSet patch,
CherryPickInput input,
- RefControl refControl)
+ Branch.NameKey dest)
throws OrmException, IOException, InvalidChangeOperationException, IntegrationException,
- UpdateException, RestApiException, ConfigInvalidException {
+ UpdateException, RestApiException, ConfigInvalidException, NoSuchProjectException {
return cherryPick(
batchUpdateFactory,
change,
@@ -146,7 +149,7 @@
change.getProject(),
ObjectId.fromString(patch.getRevision().get()),
input,
- refControl);
+ dest);
}
public Change.Id cherryPick(
@@ -156,9 +159,9 @@
Project.NameKey project,
ObjectId sourceCommit,
CherryPickInput input,
- RefControl destRefControl)
+ Branch.NameKey dest)
throws OrmException, IOException, InvalidChangeOperationException, IntegrationException,
- UpdateException, RestApiException, ConfigInvalidException {
+ UpdateException, RestApiException, ConfigInvalidException, NoSuchProjectException {
IdentifiedUser identifiedUser = user.get();
try (Repository git = gitManager.openRepository(project);
@@ -168,11 +171,10 @@
ObjectInserter oi = git.newObjectInserter();
ObjectReader reader = oi.newReader();
CodeReviewRevWalk revWalk = CodeReviewCommit.newRevWalk(reader)) {
- String destRefName = destRefControl.getRefName();
- Ref destRef = git.getRefDatabase().exactRef(destRefName);
+ Ref destRef = git.getRefDatabase().exactRef(dest.get());
if (destRef == null) {
throw new InvalidChangeOperationException(
- String.format("Branch %s does not exist.", destRefName));
+ String.format("Branch %s does not exist.", dest.get()));
}
RevCommit baseCommit = getBaseCommit(destRef, project.get(), revWalk, input.base);
@@ -200,8 +202,10 @@
String commitMessage = ChangeIdUtil.insertId(input.message, computedChangeId).trim() + '\n';
CodeReviewCommit cherryPickCommit;
+ ProjectControl projectControl =
+ projectControlFactory.controlFor(dest.getParentKey(), identifiedUser);
try {
- ProjectState projectState = destRefControl.getProjectControl().getProjectState();
+ ProjectState projectState = projectControl.getProjectState();
cherryPickCommit =
mergeUtilFactory
.create(projectState)
@@ -242,8 +246,7 @@
if (destChanges.size() == 1) {
// The change key exists on the destination branch. The cherry pick
// will be added as a new patch set.
- ChangeControl destCtl =
- destRefControl.getProjectControl().controlFor(destChanges.get(0).notes());
+ ChangeControl destCtl = projectControl.controlFor(destChanges.get(0).notes());
result = insertPatchSet(bu, git, destCtl, cherryPickCommit, input);
} else {
// Change key not found on destination branch. We can create a new
@@ -254,16 +257,13 @@
}
result =
createNewChange(
- bu, cherryPickCommit, destRefName, newTopic, sourceChange, sourceCommit, input);
+ bu, cherryPickCommit, dest.get(), newTopic, sourceChange, sourceCommit, input);
if (sourceChange != null && sourcePatchId != null) {
bu.addOp(
sourceChange.getId(),
new AddMessageToSourceChangeOp(
- changeMessagesUtil,
- sourcePatchId,
- RefNames.shortName(destRefName),
- cherryPickCommit));
+ changeMessagesUtil, sourcePatchId, dest.getShortName(), cherryPickCommit));
}
}
bu.execute();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java
index c7ad77c..5d5a6ae 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java
@@ -20,6 +20,7 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
+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;
@@ -30,6 +31,7 @@
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.CommitResource;
import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
@@ -68,7 +70,7 @@
public ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, CommitResource rsrc, CherryPickInput input)
throws OrmException, IOException, UpdateException, RestApiException,
- PermissionBackendException, ConfigInvalidException {
+ PermissionBackendException, ConfigInvalidException, NoSuchProjectException {
RevCommit commit = rsrc.getCommit();
String message = Strings.nullToEmpty(input.message).trim();
input.message = message.isEmpty() ? commit.getFullMessage() : message;
@@ -97,7 +99,7 @@
projectName,
commit,
input,
- rsrc.getProject().controlForRef(refName));
+ new Branch.NameKey(rsrc.getProject().getProject().getNameKey(), refName));
return json.noOptions().format(projectName, cherryPickedChangeId);
} catch (InvalidChangeOperationException e) {
throw new BadRequestException(e.getMessage());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
index 615c32b..fd425ef 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
@@ -135,7 +135,7 @@
if (!allowDrafts) {
throw new MethodNotAllowedException("Draft workflow is disabled");
}
- if (!ctx.getControl().canDelete(ctx.getDb(), Change.Status.DRAFT)) {
+ if (!ctx.getControl().canDeleteDraft(ctx.getDb())) {
throw new AuthException("Not permitted to delete this draft patch set");
}
@@ -220,7 +220,7 @@
allowDrafts
&& rsrc.getPatchSet().isDraft()
&& psUtil.byChange(db.get(), rsrc.getNotes()).size() > 1
- && rsrc.getControl().canDelete(db.get(), Change.Status.DRAFT));
+ && rsrc.getControl().canDeleteDraft(db.get()));
} catch (OrmException e) {
throw new IllegalStateException(e);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index df4b435..933705a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -38,6 +38,8 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.BatchUpdateReviewDb;
import com.google.gerrit.server.update.ChangeContext;
@@ -70,6 +72,7 @@
private final DeleteReviewerSender.Factory deleteReviewerSenderFactory;
private final NotesMigration migration;
private final NotifyUtil notifyUtil;
+ private final RemoveReviewerControl removeReviewerControl;
private final Account reviewer;
private final DeleteReviewerInput input;
@@ -91,6 +94,7 @@
DeleteReviewerSender.Factory deleteReviewerSenderFactory,
NotesMigration migration,
NotifyUtil notifyUtil,
+ RemoveReviewerControl removeReviewerControl,
@Assisted Account reviewerAccount,
@Assisted DeleteReviewerInput input) {
this.approvalsUtil = approvalsUtil;
@@ -102,6 +106,7 @@
this.deleteReviewerSenderFactory = deleteReviewerSenderFactory;
this.migration = migration;
this.notifyUtil = notifyUtil;
+ this.removeReviewerControl = removeReviewerControl;
this.reviewer = reviewerAccount;
this.input = input;
@@ -109,7 +114,7 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws AuthException, ResourceNotFoundException, OrmException {
+ throws AuthException, ResourceNotFoundException, OrmException, PermissionBackendException {
Account.Id reviewerId = reviewer.getId();
if (!approvalsUtil.getReviewers(ctx.getDb(), ctx.getNotes()).all().contains(reviewerId)) {
throw new ResourceNotFoundException();
@@ -130,21 +135,18 @@
List<PatchSetApproval> del = new ArrayList<>();
boolean votesRemoved = false;
for (PatchSetApproval a : approvals(ctx, reviewerId)) {
- if (ctx.getControl().canRemoveReviewer(a)) {
- del.add(a);
- if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) {
- oldApprovals.put(a.getLabel(), a.getValue());
- removedVotesMsg
- .append("* ")
- .append(a.getLabel())
- .append(formatLabelValue(a.getValue()))
- .append(" by ")
- .append(userFactory.create(a.getAccountId()).getNameEmail())
- .append("\n");
- votesRemoved = true;
- }
- } else {
- throw new AuthException("delete reviewer not permitted");
+ removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
+ del.add(a);
+ if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) {
+ oldApprovals.put(a.getLabel(), a.getValue());
+ removedVotesMsg
+ .append("* ")
+ .append(a.getLabel())
+ .append(formatLabelValue(a.getValue()))
+ .append(" by ")
+ .append(userFactory.create(a.getAccountId()).getNameEmail())
+ .append("\n");
+ votesRemoved = true;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
index c31f72d..7029101 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java
@@ -40,7 +40,9 @@
import com.google.gerrit.server.extensions.events.VoteDeleted;
import com.google.gerrit.server.mail.send.DeleteVoteSender;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -72,6 +74,7 @@
private final VoteDeleted voteDeleted;
private final DeleteVoteSender.Factory deleteVoteSenderFactory;
private final NotifyUtil notifyUtil;
+ private final RemoveReviewerControl removeReviewerControl;
@Inject
DeleteVote(
@@ -83,7 +86,8 @@
IdentifiedUser.GenericFactory userFactory,
VoteDeleted voteDeleted,
DeleteVoteSender.Factory deleteVoteSenderFactory,
- NotifyUtil notifyUtil) {
+ NotifyUtil notifyUtil,
+ RemoveReviewerControl removeReviewerControl) {
super(retryHelper);
this.db = db;
this.approvalsUtil = approvalsUtil;
@@ -93,6 +97,7 @@
this.voteDeleted = voteDeleted;
this.deleteVoteSenderFactory = deleteVoteSenderFactory;
this.notifyUtil = notifyUtil;
+ this.removeReviewerControl = removeReviewerControl;
}
@Override
@@ -143,7 +148,8 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws OrmException, AuthException, ResourceNotFoundException, IOException {
+ throws OrmException, AuthException, ResourceNotFoundException, IOException,
+ PermissionBackendException {
ChangeControl ctl = ctx.getControl();
change = ctl.getChange();
PatchSet.Id psId = change.currentPatchSetId();
@@ -166,8 +172,12 @@
// Populate map for non-matching labels, needed by VoteDeleted.
newApprovals.put(a.getLabel(), a.getValue());
continue;
- } else if (!ctl.canRemoveReviewer(a)) {
- throw new AuthException("delete vote not permitted");
+ } else {
+ try {
+ removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
+ } catch (AuthException e) {
+ throw new AuthException("delete vote not permitted", e);
+ }
}
// Set the approval to 0 if vote is being removed.
newApprovals.put(a.getLabel(), (short) 0);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index 8f2c3a8..5e26305 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -27,6 +27,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
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.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -338,7 +339,13 @@
commitId,
ctx.getIdentifiedUser())) {
commitValidatorsFactory
- .forGerritCommits(perm, origCtl.getRefControl(), new NoSshInfo(), ctx.getRevWalk())
+ .forGerritCommits(
+ perm,
+ new Branch.NameKey(
+ origCtl.getProject().getNameKey(), origCtl.getRefControl().getRefName()),
+ ctx.getIdentifiedUser(),
+ new NoSshInfo(),
+ ctx.getRevWalk())
.validate(event);
} catch (CommitValidationException e) {
throw new ResourceConflictException(e.getFullMessage());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index fcd6e0d..d4e085c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -134,7 +134,6 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.ssh.SshInfo;
@@ -1012,8 +1011,7 @@
return;
}
- RefControl ctl = projectControl.controlForRef(cmd.getRefName());
- validateNewCommits(ctl, cmd);
+ validateNewCommits(new Branch.NameKey(project.getNameKey(), cmd.getRefName()), cmd);
actualCommands.add(cmd);
}
@@ -1033,7 +1031,7 @@
if (!validRefOperation(cmd)) {
return;
}
- validateNewCommits(projectControl.controlForRef(cmd.getRefName()), cmd);
+ validateNewCommits(new Branch.NameKey(project.getNameKey(), cmd.getRefName()), cmd);
actualCommands.add(cmd);
} else {
if (RefNames.REFS_CONFIG.equals(cmd.getRefName())) {
@@ -1104,9 +1102,8 @@
}
logDebug("Rewinding {}", cmd);
- RefControl ctl = projectControl.controlForRef(cmd.getRefName());
if (newObject != null) {
- validateNewCommits(ctl, cmd);
+ validateNewCommits(new Branch.NameKey(project.getNameKey(), cmd.getRefName()), cmd);
if (cmd.getResult() != NOT_ATTEMPTED) {
return;
}
@@ -1138,7 +1135,6 @@
final NotesMigration notesMigration;
private final boolean defaultPublishComments;
Branch.NameKey dest;
- RefControl ctl;
PermissionBackend.ForRef perm;
Set<Account.Id> reviewer = Sets.newLinkedHashSet();
Set<Account.Id> cc = Sets.newLinkedHashSet();
@@ -1451,7 +1447,6 @@
}
magicBranch.dest = new Branch.NameKey(project.getNameKey(), ref);
- magicBranch.ctl = projectControl.controlForRef(ref);
magicBranch.perm = permissions.ref(ref);
if (!projectControl.getProject().getState().permitsWrite()) {
reject(cmd, "project state does not permit write");
@@ -1590,7 +1585,7 @@
// commits and the target branch head.
//
try {
- Ref targetRef = rp.getAdvertisedRefs().get(magicBranch.ctl.getRefName());
+ Ref targetRef = rp.getAdvertisedRefs().get(magicBranch.dest.get());
if (targetRef == null || targetRef.getObjectId() == null) {
// The destination branch does not yet exist. Assume the
// history being sent for review will start it and thus
@@ -1798,7 +1793,7 @@
logDebug("Creating new change for {} even though it is already tracked", name);
}
- if (!validCommit(rp.getRevWalk(), magicBranch.perm, magicBranch.ctl, magicBranch.cmd, c)) {
+ if (!validCommit(rp.getRevWalk(), magicBranch.perm, magicBranch.dest, magicBranch.cmd, c)) {
// Not a change the user can propose? Abort as early as possible.
newChanges = Collections.emptyList();
logDebug("Aborting early due to invalid commit");
@@ -1992,7 +1987,7 @@
rw.markUninteresting(c);
}
} else {
- markHeadsAsUninteresting(rw, magicBranch.ctl != null ? magicBranch.ctl.getRefName() : null);
+ markHeadsAsUninteresting(rw, magicBranch.dest != null ? magicBranch.dest.get() : null);
}
return start;
}
@@ -2002,11 +1997,11 @@
for (RevCommit c : magicBranch.baseCommit) {
rp.getRevWalk().markUninteresting(c);
}
- Ref targetRef = allRefs().get(magicBranch.ctl.getRefName());
+ Ref targetRef = allRefs().get(magicBranch.dest.get());
if (targetRef != null) {
logDebug(
"Marking target ref {} ({}) uninteresting",
- magicBranch.ctl.getRefName(),
+ magicBranch.dest.get(),
targetRef.getObjectId().name());
rp.getRevWalk().markUninteresting(rp.getRevWalk().parseCommit(targetRef.getObjectId()));
}
@@ -2014,7 +2009,7 @@
private void rejectImplicitMerges(Set<RevCommit> mergedParents) throws IOException {
if (!mergedParents.isEmpty()) {
- Ref targetRef = allRefs().get(magicBranch.ctl.getRefName());
+ Ref targetRef = allRefs().get(magicBranch.dest.get());
if (targetRef != null) {
RevWalk rw = rp.getRevWalk();
RevCommit tip = rw.parseCommit(targetRef.getObjectId());
@@ -2380,8 +2375,7 @@
}
PermissionBackend.ForRef perm = permissions.ref(change.getDest().get());
- RefControl refctl = projectControl.controlForRef(change.getDest());
- if (!validCommit(rp.getRevWalk(), perm, refctl, inputCommand, newCommit)) {
+ if (!validCommit(rp.getRevWalk(), perm, change.getDest(), inputCommand, newCommit)) {
return false;
}
rp.getRevWalk().parseBody(priorCommit);
@@ -2685,9 +2679,9 @@
return true;
}
- private void validateNewCommits(RefControl ctl, ReceiveCommand cmd)
+ private void validateNewCommits(Branch.NameKey branch, ReceiveCommand cmd)
throws PermissionBackendException {
- PermissionBackend.ForRef perm = permissions.ref(ctl.getRefName());
+ PermissionBackend.ForRef perm = permissions.ref(branch.get());
if (!RefNames.REFS_CONFIG.equals(cmd.getRefName())
&& !(MagicBranch.isMagicBranch(cmd.getRefName())
|| NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
@@ -2721,7 +2715,7 @@
i++;
if (existing.keySet().contains(c)) {
continue;
- } else if (!validCommit(walk, perm, ctl, cmd, c)) {
+ } else if (!validCommit(walk, perm, branch, cmd, c)) {
break;
}
@@ -2757,7 +2751,11 @@
}
private boolean validCommit(
- RevWalk rw, PermissionBackend.ForRef perm, RefControl ctl, ReceiveCommand cmd, ObjectId id)
+ RevWalk rw,
+ PermissionBackend.ForRef perm,
+ Branch.NameKey branch,
+ ReceiveCommand cmd,
+ ObjectId id)
throws IOException {
if (validCommits.contains(id)) {
@@ -2768,15 +2766,16 @@
rw.parseBody(c);
try (CommitReceivedEvent receiveEvent =
- new CommitReceivedEvent(cmd, project, ctl.getRefName(), rw.getObjectReader(), c, user)) {
+ new CommitReceivedEvent(cmd, project, branch.get(), rw.getObjectReader(), c, user)) {
boolean isMerged =
magicBranch != null
&& cmd.getRefName().equals(magicBranch.cmd.getRefName())
&& magicBranch.merged;
CommitValidators validators =
isMerged
- ? commitValidatorsFactory.forMergedCommits(perm, ctl)
- : commitValidatorsFactory.forReceiveCommits(perm, ctl, sshInfo, repo, rw);
+ ? commitValidatorsFactory.forMergedCommits(perm, user.asIdentifiedUser())
+ : commitValidatorsFactory.forReceiveCommits(
+ perm, branch, user.asIdentifiedUser(), sshInfo, repo, rw);
messages.addAll(validators.validate(receiveEvent));
} catch (CommitValidationException e) {
logDebug("Commit validation failed on {}", c.name());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
index 17303f8..41381e8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -28,6 +28,7 @@
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
@@ -44,9 +45,8 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
-import com.google.gerrit.server.project.ProjectControl;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.ssh.SshInfo;
import com.google.gerrit.server.util.MagicBranch;
import com.google.inject.Inject;
@@ -89,6 +89,7 @@
private final AllUsersName allUsers;
private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker;
private final String installCommitMsgHookCommand;
+ private final ProjectCache projectCache;
@Inject
Factory(
@@ -97,7 +98,8 @@
@GerritServerConfig Config cfg,
DynamicSet<CommitValidationListener> pluginValidators,
AllUsersName allUsers,
- ExternalIdsConsistencyChecker externalIdsConsistencyChecker) {
+ ExternalIdsConsistencyChecker externalIdsConsistencyChecker,
+ ProjectCache projectCache) {
this.gerritIdent = gerritIdent;
this.canonicalWebUrl = canonicalWebUrl;
this.pluginValidators = pluginValidators;
@@ -105,26 +107,29 @@
this.externalIdsConsistencyChecker = externalIdsConsistencyChecker;
this.installCommitMsgHookCommand =
cfg != null ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null;
+ this.projectCache = projectCache;
}
public CommitValidators forReceiveCommits(
PermissionBackend.ForRef perm,
- RefControl refctl,
+ Branch.NameKey branch,
+ IdentifiedUser user,
SshInfo sshInfo,
Repository repo,
RevWalk rw)
throws IOException {
NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw);
- IdentifiedUser user = refctl.getUser().asIdentifiedUser();
+ ProjectState projectState = projectCache.checkedGet(branch.getParentKey());
return new CommitValidators(
ImmutableList.of(
new UploadMergesPermissionValidator(perm),
new AmendedGerritMergeCommitValidationListener(perm, gerritIdent),
new AuthorUploaderValidator(user, perm, canonicalWebUrl),
new CommitterUploaderValidator(user, perm, canonicalWebUrl),
- new SignedOffByValidator(user, perm, refctl.getProjectControl().getProjectState()),
- new ChangeIdValidator(refctl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
- new ConfigValidator(refctl, rw, allUsers),
+ new SignedOffByValidator(user, perm, projectState),
+ new ChangeIdValidator(
+ projectState, user, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
+ new ConfigValidator(branch, user, rw, allUsers),
new BannedCommitsValidator(rejectCommits),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
@@ -132,23 +137,31 @@
}
public CommitValidators forGerritCommits(
- PermissionBackend.ForRef perm, RefControl refctl, SshInfo sshInfo, RevWalk rw) {
- IdentifiedUser user = refctl.getUser().asIdentifiedUser();
+ PermissionBackend.ForRef perm,
+ Branch.NameKey branch,
+ IdentifiedUser user,
+ SshInfo sshInfo,
+ RevWalk rw)
+ throws IOException {
return new CommitValidators(
ImmutableList.of(
new UploadMergesPermissionValidator(perm),
new AmendedGerritMergeCommitValidationListener(perm, gerritIdent),
new AuthorUploaderValidator(user, perm, canonicalWebUrl),
- new SignedOffByValidator(user, perm, refctl.getProjectControl().getProjectState()),
- new ChangeIdValidator(refctl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
- new ConfigValidator(refctl, rw, allUsers),
+ new SignedOffByValidator(user, perm, projectCache.checkedGet(branch.getParentKey())),
+ new ChangeIdValidator(
+ projectCache.checkedGet(branch.getParentKey()),
+ user,
+ canonicalWebUrl,
+ installCommitMsgHookCommand,
+ sshInfo),
+ new ConfigValidator(branch, user, rw, allUsers),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
new AccountValidator(allUsers)));
}
- public CommitValidators forMergedCommits(PermissionBackend.ForRef perm, RefControl refControl) {
- IdentifiedUser user = refControl.getUser().asIdentifiedUser();
+ public CommitValidators forMergedCommits(PermissionBackend.ForRef perm, IdentifiedUser user) {
// Generally only include validators that are based on permissions of the
// user creating a change for a merged commit; generally exclude
// validators that would require amending the change in order to correct.
@@ -208,22 +221,23 @@
+ " line format in commit message footer";
private static final Pattern CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN);
- private final ProjectControl projectControl;
+ private final ProjectState projectState;
private final String canonicalWebUrl;
private final String installCommitMsgHookCommand;
private final SshInfo sshInfo;
private final IdentifiedUser user;
public ChangeIdValidator(
- RefControl refControl,
+ ProjectState projectState,
+ IdentifiedUser user,
String canonicalWebUrl,
String installCommitMsgHookCommand,
SshInfo sshInfo) {
- this.projectControl = refControl.getProjectControl();
+ this.projectState = projectState;
this.canonicalWebUrl = canonicalWebUrl;
this.installCommitMsgHookCommand = installCommitMsgHookCommand;
this.sshInfo = sshInfo;
- this.user = projectControl.getUser().asIdentifiedUser();
+ this.user = user;
}
@Override
@@ -238,7 +252,7 @@
String sha1 = commit.abbreviate(SHA1_LENGTH).name();
if (idList.isEmpty()) {
- if (projectControl.getProjectState().isRequireChangeID()) {
+ if (projectState.isRequireChangeID()) {
String shortMsg = commit.getShortMessage();
if (shortMsg.startsWith(CHANGE_ID_PREFIX)
&& CHANGE_ID
@@ -342,12 +356,15 @@
/** If this is the special project configuration branch, validate the config. */
public static class ConfigValidator implements CommitValidationListener {
- private final RefControl refControl;
+ private final Branch.NameKey branch;
+ private final IdentifiedUser user;
private final RevWalk rw;
private final AllUsersName allUsers;
- public ConfigValidator(RefControl refControl, RevWalk rw, AllUsersName allUsers) {
- this.refControl = refControl;
+ public ConfigValidator(
+ Branch.NameKey branch, IdentifiedUser user, RevWalk rw, AllUsersName allUsers) {
+ this.branch = branch;
+ this.user = user;
this.rw = rw;
this.allUsers = allUsers;
}
@@ -355,9 +372,7 @@
@Override
public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
throws CommitValidationException {
- IdentifiedUser currentUser = refControl.getUser().asIdentifiedUser();
-
- if (REFS_CONFIG.equals(refControl.getRefName())) {
+ if (REFS_CONFIG.equals(branch.get())) {
List<CommitValidationMessage> messages = new ArrayList<>();
try {
@@ -373,7 +388,7 @@
} catch (ConfigInvalidException | IOException e) {
log.error(
"User "
- + currentUser.getUserName()
+ + user.getUserName()
+ " tried to push an invalid project configuration "
+ receiveEvent.command.getNewId().name()
+ " for project "
@@ -383,10 +398,9 @@
}
}
- if (allUsers.equals(refControl.getProjectControl().getProject().getNameKey())
- && RefNames.isRefsUsers(refControl.getRefName())) {
+ if (allUsers.equals(branch.getParentKey()) && RefNames.isRefsUsers(branch.get())) {
List<CommitValidationMessage> messages = new ArrayList<>();
- Account.Id accountId = Account.Id.fromRef(refControl.getRefName());
+ Account.Id accountId = Account.Id.fromRef(branch.get());
if (accountId != null) {
try {
WatchConfig wc = new WatchConfig(accountId);
@@ -401,7 +415,7 @@
} catch (IOException | ConfigInvalidException e) {
log.error(
"User "
- + currentUser.getUserName()
+ + user.getUserName()
+ " tried to push an invalid watch configuration "
+ receiveEvent.command.getNewId().name()
+ " for account "
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index f1ebf64..78bd167 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -234,8 +234,14 @@
return (isOwner() || getRefControl().canPublishDrafts()) && isVisible(db);
}
+ /** Can this user delete this draft change or any patch set of this change? */
+ public boolean canDeleteDraft(ReviewDb db) throws OrmException {
+ // TODO(hiesel) These don't need to be migrated, just remove after support for drafts is removed
+ return canDelete(db, Change.Status.DRAFT);
+ }
+
/** Can this user delete this change or any patch set of this change? */
- public boolean canDelete(ReviewDb db, Change.Status status) throws OrmException {
+ private boolean canDelete(ReviewDb db, Change.Status status) throws OrmException {
if (!isVisible(db)) {
return false;
}
@@ -331,7 +337,7 @@
}
/** Is this user the owner of the change? */
- private boolean isOwner() {
+ boolean isOwner() {
if (getUser().isIdentifiedUser()) {
Account.Id id = getUser().asIdentifiedUser().getAccountId();
return id.equals(getChange().getOwner());
@@ -358,40 +364,6 @@
return false;
}
- /** @return true if the user is allowed to remove this reviewer. */
- public boolean canRemoveReviewer(PatchSetApproval approval) {
- return canRemoveReviewer(approval.getAccountId(), approval.getValue());
- }
-
- public boolean canRemoveReviewer(Account.Id reviewer, int value) {
- if (getChange().getStatus().isOpen()) {
- // A user can always remove themselves.
- //
- if (getUser().isIdentifiedUser()) {
- if (getUser().getAccountId().equals(reviewer)) {
- return true; // can remove self
- }
- }
-
- // The change owner may remove any zero or positive score.
- //
- if (isOwner() && 0 <= value) {
- return true;
- }
-
- // Users with the remove reviewer permission, the branch owner, project
- // owner and site admin can remove anyone
- if (getRefControl().canRemoveReviewer() // has removal permissions
- || getRefControl().isOwner() // branch owner
- || getProjectControl().isOwner() // project owner
- || getProjectControl().isAdmin()) {
- return true;
- }
- }
-
- return false;
- }
-
/** Can this user edit the topic name? */
private boolean canEditTopicName() {
if (getChange().getStatus().isOpen()) {
@@ -553,7 +525,7 @@
case SUBMIT:
return getRefControl().canSubmit(isOwner());
- case REMOVE_REVIEWER: // TODO Honor specific removal filters?
+ case REMOVE_REVIEWER:
case SUBMIT_AS:
return getRefControl().canPerform(perm.permissionName().get());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RemoveReviewerControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RemoveReviewerControl.java
new file mode 100644
index 0000000..591fcc2
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RemoveReviewerControl.java
@@ -0,0 +1,102 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.project;
+
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+@Singleton
+public class RemoveReviewerControl {
+ private final PermissionBackend permissionBackend;
+ private final Provider<ReviewDb> dbProvider;
+ private final ChangeControl.GenericFactory changeControlFactory;
+
+ @Inject
+ RemoveReviewerControl(
+ PermissionBackend permissionBackend,
+ Provider<ReviewDb> dbProvider,
+ ChangeControl.GenericFactory changeControlFactory) {
+ this.permissionBackend = permissionBackend;
+ this.dbProvider = dbProvider;
+ this.changeControlFactory = changeControlFactory;
+ }
+
+ /** @throws AuthException if this user is not allowed to remove this approval. */
+ public void checkRemoveReviewer(
+ ChangeNotes notes, CurrentUser currentUser, PatchSetApproval approval)
+ throws PermissionBackendException, AuthException, NoSuchChangeException {
+ if (canRemoveReviewerWithoutPermissionCheck(
+ notes, currentUser, approval.getAccountId(), approval.getValue())) {
+ return;
+ }
+
+ permissionBackend
+ .user(currentUser)
+ .change(notes)
+ .database(dbProvider)
+ .check(ChangePermission.REMOVE_REVIEWER);
+ }
+
+ /** @return true if the user is allowed to remove this reviewer. */
+ public boolean testRemoveReviewer(
+ ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
+ throws PermissionBackendException, NoSuchChangeException {
+ if (canRemoveReviewerWithoutPermissionCheck(notes, currentUser, reviewer, value)) {
+ return true;
+ }
+ return permissionBackend
+ .user(currentUser)
+ .change(notes)
+ .database(dbProvider)
+ .test(ChangePermission.REMOVE_REVIEWER);
+ }
+
+ private boolean canRemoveReviewerWithoutPermissionCheck(
+ ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
+ throws NoSuchChangeException {
+ ChangeControl changeControl = changeControlFactory.controlFor(notes, currentUser);
+ if (!changeControl.getChange().getStatus().isOpen()) {
+ return false;
+ }
+ // A user can always remove themselves.
+ if (changeControl.getUser().isIdentifiedUser()) {
+ if (changeControl.getUser().getAccountId().equals(reviewer)) {
+ return true; // can remove self
+ }
+ }
+ // The change owner may remove any zero or positive score.
+ if (changeControl.isOwner() && 0 <= value) {
+ return true;
+ }
+ // Users with the remove reviewer permission, the branch owner, project
+ // owner and site admin can remove anyone
+ if (changeControl.getRefControl().isOwner() // branch owner
+ || changeControl.getProjectControl().isOwner() // project owner
+ || changeControl.getProjectControl().isAdmin()) { // project admin
+ return true;
+ }
+ return false;
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDb.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDb.java
index ed18a86..6c5dd35 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDb.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/MariaDb.java
@@ -40,6 +40,7 @@
b.append(port(dbs.optional("port")));
b.append("/");
b.append(dbs.required("database"));
+ b.append("?useBulkStmts=false");
return b.toString();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java
index d8a5278..4cbaffd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java
@@ -18,6 +18,7 @@
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
+import com.github.rholder.retry.Attempt;
import com.github.rholder.retry.RetryException;
import com.github.rholder.retry.RetryListener;
import com.github.rholder.retry.RetryerBuilder;
@@ -28,6 +29,10 @@
import com.google.common.base.Throwables;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.metrics.Counter0;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Histogram0;
+import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.LockFailureException;
import com.google.gerrit.server.notedb.NotesMigration;
@@ -75,6 +80,30 @@
}
}
+ @Singleton
+ private static class Metrics {
+ final Histogram0 attemptCounts;
+ final Counter0 timeoutCount;
+
+ @Inject
+ Metrics(MetricMaker metricMaker) {
+ attemptCounts =
+ metricMaker.newHistogram(
+ "batch_update/retry_attempt_counts",
+ new Description(
+ "Distribution of number of attempts made by RetryHelper"
+ + " (1 == single attempt, no retry)")
+ .setCumulative()
+ .setUnit("attempts"));
+ timeoutCount =
+ metricMaker.newCounter(
+ "batch_update/retry_timeout_count",
+ new Description("Number of executions of RetryHelper that ultimately timed out")
+ .setCumulative()
+ .setUnit("timeouts"));
+ }
+ }
+
public static Options.Builder options() {
return new AutoValue_RetryHelper_Options.Builder();
}
@@ -84,6 +113,7 @@
}
private final NotesMigration migration;
+ private final Metrics metrics;
private final BatchUpdate.Factory updateFactory;
private final Duration defaultTimeout;
private final WaitStrategy waitStrategy;
@@ -91,9 +121,11 @@
@Inject
RetryHelper(
@GerritServerConfig Config cfg,
+ Metrics metrics,
NotesMigration migration,
ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory,
NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) {
+ this.metrics = metrics;
this.migration = migration;
this.updateFactory =
new BatchUpdate.Factory(migration, reviewDbBatchUpdateFactory, noteDbBatchUpdateFactory);
@@ -117,18 +149,18 @@
}
public <T> T execute(Action<T> action, Options opts) throws RestApiException, UpdateException {
+ MetricListener listener = null;
try {
RetryerBuilder<T> builder = RetryerBuilder.newBuilder();
if (migration.disableChangeReviewDb()) {
+ listener = new MetricListener(opts.listener());
builder
+ .withRetryListener(listener)
.withStopStrategy(
StopStrategies.stopAfterDelay(
firstNonNull(opts.timeout(), defaultTimeout).toMillis(), MILLISECONDS))
.withWaitStrategy(waitStrategy)
.retryIfException(RetryHelper::isLockFailure);
- if (opts.listener() != null) {
- builder.withRetryListener(opts.listener());
- }
} else {
// Either we aren't full-NoteDb, or the underlying ref storage doesn't support atomic
// transactions. Either way, retrying a partially-failed operation is not idempotent, so
@@ -136,11 +168,18 @@
}
return builder.build().call(() -> action.call(updateFactory));
} catch (ExecutionException | RetryException e) {
+ if (e instanceof RetryException) {
+ metrics.timeoutCount.increment();
+ }
if (e.getCause() != null) {
Throwables.throwIfInstanceOf(e.getCause(), UpdateException.class);
Throwables.throwIfInstanceOf(e.getCause(), RestApiException.class);
}
throw new UpdateException(e);
+ } finally {
+ if (listener != null) {
+ metrics.attemptCounts.record(listener.getAttemptCount());
+ }
}
}
@@ -150,4 +189,26 @@
}
return t instanceof LockFailureException;
}
+
+ private static class MetricListener implements RetryListener {
+ private final RetryListener delegate;
+ private long attemptCount;
+
+ MetricListener(@Nullable RetryListener delegate) {
+ this.delegate = delegate;
+ attemptCount = 1;
+ }
+
+ @Override
+ public <V> void onRetry(Attempt<V> attempt) {
+ attemptCount = attempt.getAttemptNumber();
+ if (delegate != null) {
+ delegate.onRetry(attempt);
+ }
+ }
+
+ long getAttemptCount() {
+ return attemptCount;
+ }
+ }
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/util/LabelVoteTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/util/LabelVoteTest.java
index d3cb927..9069928 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/util/LabelVoteTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/util/LabelVoteTest.java
@@ -14,79 +14,48 @@
package com.google.gerrit.server.util;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
+import static com.google.gerrit.server.util.LabelVote.parse;
+import static com.google.gerrit.server.util.LabelVote.parseWithEquals;
import org.junit.Test;
public class LabelVoteTest {
@Test
- public void parse() {
- LabelVote l;
- l = LabelVote.parse("Code-Review-2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) -2, l.value());
- l = LabelVote.parse("Code-Review-1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) -1, l.value());
- l = LabelVote.parse("-Code-Review");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 0, l.value());
- l = LabelVote.parse("Code-Review");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 1, l.value());
- l = LabelVote.parse("Code-Review+1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 1, l.value());
- l = LabelVote.parse("Code-Review+2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 2, l.value());
+ public void labelVoteParse() {
+ assertLabelVoteEquals(parse("Code-Review-2"), "Code-Review", -2);
+ assertLabelVoteEquals(parse("Code-Review-1"), "Code-Review", -1);
+ assertLabelVoteEquals(parse("-Code-Review"), "Code-Review", 0);
+ assertLabelVoteEquals(parse("Code-Review"), "Code-Review", 1);
+ assertLabelVoteEquals(parse("Code-Review+1"), "Code-Review", 1);
+ assertLabelVoteEquals(parse("Code-Review+2"), "Code-Review", 2);
}
@Test
- public void format() {
- assertEquals("Code-Review-2", LabelVote.parse("Code-Review-2").format());
- assertEquals("Code-Review-1", LabelVote.parse("Code-Review-1").format());
- assertEquals("-Code-Review", LabelVote.parse("-Code-Review").format());
- assertEquals("Code-Review+1", LabelVote.parse("Code-Review+1").format());
- assertEquals("Code-Review+2", LabelVote.parse("Code-Review+2").format());
+ public void labelVoteFormat() {
+ assertThat(parse("Code-Review-2").format()).isEqualTo("Code-Review-2");
+ assertThat(parse("Code-Review-1").format()).isEqualTo("Code-Review-1");
+ assertThat(parse("-Code-Review").format()).isEqualTo("-Code-Review");
+ assertThat(parse("Code-Review+1").format()).isEqualTo("Code-Review+1");
+ assertThat(parse("Code-Review+2").format()).isEqualTo("Code-Review+2");
}
@Test
- public void parseWithEquals() {
- LabelVote l;
- l = LabelVote.parseWithEquals("Code-Review=-2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) -2, l.value());
- l = LabelVote.parseWithEquals("Code-Review=-1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) -1, l.value());
- l = LabelVote.parseWithEquals("Code-Review=0");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 0, l.value());
- l = LabelVote.parseWithEquals("Code-Review=1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 1, l.value());
- l = LabelVote.parseWithEquals("Code-Review=+1");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 1, l.value());
- l = LabelVote.parseWithEquals("Code-Review=2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 2, l.value());
- l = LabelVote.parseWithEquals("Code-Review=+2");
- assertEquals("Code-Review", l.label());
- assertEquals((short) 2, l.value());
- l = LabelVote.parseWithEquals("R=0");
- assertEquals("R", l.label());
- assertEquals((short) 0, l.value());
+ public void labelVoteParseWithEquals() {
+ assertLabelVoteEquals(parseWithEquals("Code-Review=-2"), "Code-Review", -2);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=-1"), "Code-Review", -1);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=0"), "Code-Review", 0);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=1"), "Code-Review", 1);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=+1"), "Code-Review", 1);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=2"), "Code-Review", 2);
+ assertLabelVoteEquals(parseWithEquals("Code-Review=+2"), "Code-Review", 2);
+ assertLabelVoteEquals(parseWithEquals("R=0"), "R", 0);
String longName = "A-loooooooooooooooooooooooooooooooooooooooooooooooooong-label";
// Regression test: an old bug passed the string length as a radix to Short#parseShort.
- assertTrue(longName.length() > Character.MAX_RADIX);
- l = LabelVote.parseWithEquals(longName + "=+1");
- assertEquals(longName, l.label());
- assertEquals((short) 1, l.value());
+ assertThat(longName.length()).isGreaterThan(Character.MAX_RADIX);
+ assertLabelVoteEquals(parseWithEquals(longName + "=+1"), longName, 1);
assertParseWithEqualsFails(null);
assertParseWithEqualsFails("");
@@ -99,18 +68,23 @@
}
@Test
- public void formatWithEquals() {
- assertEquals("Code-Review=-2", LabelVote.parseWithEquals("Code-Review=-2").formatWithEquals());
- assertEquals("Code-Review=-1", LabelVote.parseWithEquals("Code-Review=-1").formatWithEquals());
- assertEquals("Code-Review=0", LabelVote.parseWithEquals("Code-Review=0").formatWithEquals());
- assertEquals("Code-Review=+1", LabelVote.parseWithEquals("Code-Review=+1").formatWithEquals());
- assertEquals("Code-Review=+2", LabelVote.parseWithEquals("Code-Review=+2").formatWithEquals());
+ public void labelVoteFormatWithEquals() {
+ assertThat(parseWithEquals("Code-Review=-2").formatWithEquals()).isEqualTo("Code-Review=-2");
+ assertThat(parseWithEquals("Code-Review=-1").formatWithEquals()).isEqualTo("Code-Review=-1");
+ assertThat(parseWithEquals("Code-Review=0").formatWithEquals()).isEqualTo("Code-Review=0");
+ assertThat(parseWithEquals("Code-Review=+1").formatWithEquals()).isEqualTo("Code-Review=+1");
+ assertThat(parseWithEquals("Code-Review=+2").formatWithEquals()).isEqualTo("Code-Review=+2");
+ }
+
+ private void assertLabelVoteEquals(LabelVote actual, String expectedLabel, int expectedValue) {
+ assertThat(actual.label()).isEqualTo(expectedLabel);
+ assertThat((int) actual.value()).isEqualTo(expectedValue);
}
private void assertParseWithEqualsFails(String value) {
try {
- LabelVote.parseWithEquals(value);
- fail("expected IllegalArgumentException when parsing \"" + value + "\"");
+ parseWithEquals(value);
+ assert_().fail("expected IllegalArgumentException when parsing \"%s\"", value);
} catch (IllegalArgumentException e) {
// Expected.
}
diff --git a/polygerrit-ui/server.go b/polygerrit-ui/server.go
index f7cab2e..79cf4bf 100644
--- a/polygerrit-ui/server.go
+++ b/polygerrit-ui/server.go
@@ -200,7 +200,7 @@
// Any path prefixes that should resolve to index.html.
var (
- fePaths = []string{"/q/", "/c/", "/dashboard/"}
+ fePaths = []string{"/q/", "/c/", "/dashboard/", "/admin/"}
issueNumRE = regexp.MustCompile(`^\/\d+\/?$`)
)
diff --git a/tools/bzl/classpath.bzl b/tools/bzl/classpath.bzl
index 5b9242e..9448ed1 100644
--- a/tools/bzl/classpath.bzl
+++ b/tools/bzl/classpath.bzl
@@ -1,5 +1,5 @@
def _classpath_collector(ctx):
- all = set()
+ all = depset()
for d in ctx.attr.deps:
if hasattr(d, 'java'):
all += d.java.transitive_runtime_deps
diff --git a/tools/bzl/gwt.bzl b/tools/bzl/gwt.bzl
index ef182bf..b0e250d 100644
--- a/tools/bzl/gwt.bzl
+++ b/tools/bzl/gwt.bzl
@@ -189,7 +189,7 @@
)
def _get_transitive_closure(ctx):
- deps = set()
+ deps = depset()
for dep in ctx.attr.module_deps:
deps += dep.java.transitive_runtime_deps
deps += dep.java.transitive_source_jars
diff --git a/tools/bzl/javadoc.bzl b/tools/bzl/javadoc.bzl
index 341b9c1..18ca129 100644
--- a/tools/bzl/javadoc.bzl
+++ b/tools/bzl/javadoc.bzl
@@ -17,8 +17,8 @@
def _impl(ctx):
zip_output = ctx.outputs.zip
- transitive_jar_set = set()
- source_jars = set()
+ transitive_jar_set = depset()
+ source_jars = depset()
for l in ctx.attr.libs:
source_jars += l.java.source_jars
transitive_jar_set += l.java.transitive_deps
diff --git a/tools/bzl/js.bzl b/tools/bzl/js.bzl
index 788301c..39d6acf 100644
--- a/tools/bzl/js.bzl
+++ b/tools/bzl/js.bzl
@@ -124,18 +124,18 @@
)
def _bower_component_impl(ctx):
- transitive_zipfiles = set([ctx.file.zipfile])
+ transitive_zipfiles = depset([ctx.file.zipfile])
for d in ctx.attr.deps:
transitive_zipfiles += d.transitive_zipfiles
- transitive_licenses = set()
+ transitive_licenses = depset()
if ctx.file.license:
- transitive_licenses += set([ctx.file.license])
+ transitive_licenses += depset([ctx.file.license])
for d in ctx.attr.deps:
transitive_licenses += d.transitive_licenses
- transitive_versions = set(ctx.files.version_json)
+ transitive_versions = depset(ctx.files.version_json)
for d in ctx.attr.deps:
transitive_versions += d.transitive_versions
@@ -173,13 +173,13 @@
command = cmd,
mnemonic = "GenBowerZip")
- licenses = set()
+ licenses = depset()
if ctx.file.license:
- licenses += set([ctx.file.license])
+ licenses += depset([ctx.file.license])
return struct(
transitive_zipfiles=list([ctx.outputs.zip]),
- transitive_versions=set([]),
+ transitive_versions=depset(),
transitive_licenses=licenses)
js_component = rule(
@@ -219,15 +219,15 @@
def _bower_component_bundle_impl(ctx):
"""A bunch of bower components zipped up."""
- zips = set([])
+ zips = depset()
for d in ctx.attr.deps:
zips += d.transitive_zipfiles
- versions = set([])
+ versions = depset()
for d in ctx.attr.deps:
versions += d.transitive_versions
- licenses = set([])
+ licenses = depset()
for d in ctx.attr.deps:
licenses += d.transitive_versions
diff --git a/tools/bzl/pkg_war.bzl b/tools/bzl/pkg_war.bzl
index edaaab0..ebb632f 100644
--- a/tools/bzl/pkg_war.bzl
+++ b/tools/bzl/pkg_war.bzl
@@ -73,7 +73,7 @@
]
# Add lib
- transitive_lib_deps = set()
+ transitive_lib_deps = depset()
for l in ctx.attr.libs:
if hasattr(l, 'java'):
transitive_lib_deps += l.java.transitive_runtime_deps
@@ -85,7 +85,7 @@
inputs.append(dep)
# Add pgm lib
- transitive_pgmlib_deps = set()
+ transitive_pgmlib_deps = depset()
for l in ctx.attr.pgmlibs:
transitive_pgmlib_deps += l.java.transitive_runtime_deps
@@ -95,7 +95,7 @@
inputs.append(dep)
# Add context
- transitive_context_deps = set()
+ transitive_context_deps = depset()
if ctx.attr.context:
for jar in ctx.attr.context:
if hasattr(jar, 'java'):
diff --git a/tools/bzl/plugin.bzl b/tools/bzl/plugin.bzl
index 11ac572..1223f02 100644
--- a/tools/bzl/plugin.bzl
+++ b/tools/bzl/plugin.bzl
@@ -59,7 +59,7 @@
if gwt_module:
native.java_library(
name = name + '__gwt_module',
- resources = list(set(srcs + resources)),
+ resources = depset(srcs + resources).to_list(),
runtime_deps = deps + GWT_PLUGIN_DEPS,
visibility = ['//visibility:public'],
**kwargs
