diff --git a/Documentation/user-review-ui.txt b/Documentation/user-review-ui.txt index 57dca8c445..8553634565 100644 --- a/Documentation/user-review-ui.txt +++ b/Documentation/user-review-ui.txt @@ -186,10 +186,6 @@ change is merged into the destination branch. The `Submit` button is available if the change is submittable and the link:access-control.html#category_submit[Submit] access right is assigned. -+ -It is also possible to submit changes that have merge conflicts. This -allows to do the conflict resolution for a change series in a single -merge commit and submit the changes in reverse order. ** [[abandon]]`Abandon`: + diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt index eda4b5d93d..b2b361485a 100644 --- a/Documentation/user-search.txt +++ b/Documentation/user-search.txt @@ -299,7 +299,7 @@ is:closed:: + True if the change is either merged or abandoned. -is:submitted, is:merged, is:abandoned:: +is:merged, is:abandoned:: + Same as <>. @@ -320,10 +320,6 @@ Same as 'is:reviewed', matches if any user has commented on the change more recently than the last update (comment or patch set) from the change owner. -status:submitted:: -+ -Change has been submitted, but is waiting for a dependency. - status:closed:: + True if the change is either 'merged' or 'abandoned'. diff --git a/ReleaseNotes/ReleaseNotes-2.11.8.txt b/ReleaseNotes/ReleaseNotes-2.11.8.txt new file mode 100644 index 0000000000..0f9dc21bca --- /dev/null +++ b/ReleaseNotes/ReleaseNotes-2.11.8.txt @@ -0,0 +1,43 @@ +Release notes for Gerrit 2.11.8 +=============================== + +Gerrit 2.11.8 is now available: + +link:https://gerrit-releases.storage.googleapis.com/gerrit-2.11.8.war[ +https://gerrit-releases.storage.googleapis.com/gerrit-2.11.8.war] + +There are no schema changes from link:ReleaseNotes-2.11.7.html[2.11.7]. + +Bug Fixes +--------- + +* Upgrade Apache commons-collections to version 3.2.2. ++ +Includes a fix for a link:https://issues.apache.org/jira/browse/COLLECTIONS-580[ +remote code execution exploit]. + +* link:https://code.google.com/p/gerrit/issues/detail?id=1207[Issue 1207]: +Fix keyboard shortcuts for non-US keyboards on side-by-side diff screen. ++ +The forward/backward navigation keys `[` and `]` only worked on keyboards where +these characters could be typed without using any modifier key (like CTRL, ALT, +etc.). ++ +Note that the problem still exists on the unified diff screen. + +* link:https://code.google.com/p/gerrit/issues/detail?id=3919[Issue 3919]: +Explicitly set parent project to 'All-Projects' when a project is created +without giving the parent. + +* Don't add message twice on abandon or restore via ssh review command. ++ +When abandoning or reviewing a change via the ssh `review` command, and +providing a message with the `--message` option, the message was added to +the change twice. + +* Clear the input box after cancelling add reviewer action. ++ +When the action was cancelled, the content of the input box was still +there when opening it again. + +* Fix internal server error when aborting ssh command. diff --git a/ReleaseNotes/ReleaseNotes-2.12.1.txt b/ReleaseNotes/ReleaseNotes-2.12.1.txt index 379572edec..f49de7d4b1 100644 --- a/ReleaseNotes/ReleaseNotes-2.12.1.txt +++ b/ReleaseNotes/ReleaseNotes-2.12.1.txt @@ -11,31 +11,48 @@ link:ReleaseNotes-2.11.6.html[Gerrit 2.11.6] and link:ReleaseNotes-2.11.7.html[Gerrit 2.11.7]. These bug fixes are *not* listed in these release notes. +Schema Upgrade +-------------- + *WARNING:* This version includes a manual schema upgrade when upgrading from 2.12. -+ -If you have already upgraded to 2.12, you need to issue this SQL statement -manually (e.g. using the `gerrit gsql` SSH command or the `gqsl` site -program): -+ + +When upgrading a site that is already running version 2.12, the `patch_sets` +table must be manually migrated using the `gerrit gsql` SSH command or the +`gqsl` site program. + +For the default H2 database, execute the command: + +---- alter table patch_sets modify push_certficate clob; -+ -Or with this command if the site is configured to use PostgreSQL: -+ +---- + +For MySQL, execute the command: + +---- + alter table patch_sets modify push_certficate text; +---- + +For PostgreSQL, execute the command: + +---- alter table patch_sets alter column push_certficate type text; -+ +---- + +For other database types, execute the appropriate equivalent command. + Note that the misspelled `push_certficate` is the actual name of the column. -+ -If you are upgrading from a version earlier than 2.12, this manual step is -not necessary and should be omitted. + +When upgrading from a version earlier than 2.12, this manual step is not +necessary and should be omitted. Bug Fixes --------- General -^^^^^^^ +~~~~~~~ * Fix column type for signed push certificates. + @@ -69,7 +86,7 @@ in other tables, a null change was returned which would then lead to an 'Internal Server Error' that was difficult to track down. Now an error is raised earlier which will help administrators to find the root cause. -* https://code.google.com/p/gerrit/issues/detail?id=3743[Issue 3743]: +* link:https://code.google.com/p/gerrit/issues/detail?id=3743[Issue 3743]: Use submitter identity as committer when using 'Rebase if Necessary' merge strategy. + @@ -125,8 +142,29 @@ is reduced from 'no limit' to 1024. Explicitly set parent project to 'All-Projects' when a project is created without giving the parent. +* link:https://code.google.com/p/gerrit/issues/detail?id=3948[Issue 3948]: +Fix submit of project parent updates on `refs/meta/config`. ++ +When submitting a change on `refs/meta/config` to update a project's parent, +the error 'The change must be submitted by a Gerrit administrator' was being +displayed even when the submitter was an admin. The submit was successful +when clicking 'Submit' a second time. + +* link:https://code.google.com/p/gerrit/issues/detail?id=3811[Issue 3811]: +Fix submittability of merge commits that resolve merge conflicts. ++ +If a series of changes contained a change that conflicted with the destination +branch, but the conflict was solved by a merge commit at the tip of the +series, the series was not submittable. + +* link:https://code.google.com/p/gerrit/issues/detail?id=3883[Issue 3883]: +Respect the `core.commentchar` setting from `.gitconfig` in `commit-msg` hook. + UI -^^ +~~ + +* link:https://code.google.com/p/gerrit/issues/detail?id=3894[Issue 3894]: +Fix display of 'Related changes' after change is rebased in web UI: * link:https://code.google.com/p/gerrit/issues/detail?id=3071[Issue 3071]: Fix display of submodule differences in side-by-side view. @@ -153,22 +191,11 @@ etc..). + Note that the problem still exists on the unified diff screen. +* Improve tooltip on 'Submit' button when 'Submit whole topic' is enabled +and the topic can't be submitted due to some changes not being ready. + Plugins -^^^^^^^ - -* Allow plugins to get the caller in merge validation requests. -+ -Plugins that implement the `MergeValidationListener` interface now get the -caller (the user who initiated the merge) in the `onPreMerge` method. -+ -Existing plugins that implement this interface must be adapted to the new -method signature. - -* link:https://code.google.com/p/gerrit/issues/detail?id=3741[Issue 3741]: -Fix handling of merge validation exceptions emitted by plugins. -+ -If a plugin raised an exception, it was reported to the user as 'Change is -new', rather than 'Missing dependency'. +~~~~~~~ * link:https://code.google.com/p/gerrit/issues/detail?id=3821[Issue 3821]: Fix repeated reloading of plugins when running on OpenJDK 8. @@ -178,11 +205,31 @@ are POSIX 2008 compatible. This leads to precision incompatibility when comparing the plugin's JAR file timestamp, resulting in the plugin being reloaded every minute. +* link:https://code.google.com/p/gerrit/issues/detail?id=3741[Issue 3741]: +Fix handling of merge validation exceptions emitted by plugins. ++ +If a plugin raised an exception, it was reported to the user as 'Change is +new', rather than 'Missing dependency'. + +* Allow plugins to get the caller in merge validation requests. ++ +Plugins that implement the `MergeValidationListener` interface now get the +caller (the user who initiated the merge) in the `onPreMerge` method. ++ +Existing plugins that implement this interface must be adapted to the new +method signature. + +* link:https://code.google.com/p/gerrit/issues/detail?id=3892[Issue 3892]: +Allow plugins to suggest reviewers based on either change or project +resources. + Documentation -^^^^^^^^^^^^^ +~~~~~~~~~~~~~ * Update documentation of `commentlink` to reflect changed search URL. +* Add missing documentation of valid `database.type` values. + Upgrades -------- diff --git a/ReleaseNotes/index.txt b/ReleaseNotes/index.txt index af33d7a24c..5e5ba9e821 100644 --- a/ReleaseNotes/index.txt +++ b/ReleaseNotes/index.txt @@ -15,6 +15,7 @@ Version 2.12.x [[2_11]] Version 2.11.x -------------- +* link:ReleaseNotes-2.11.8.html[2.11.8] * link:ReleaseNotes-2.11.7.html[2.11.7] * link:ReleaseNotes-2.11.6.html[2.11.6] * link:ReleaseNotes-2.11.5.html[2.11.5] diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java index e5281b1568..e26747b580 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java @@ -103,7 +103,7 @@ public class ActionsIT extends AbstractDaemonTest { assertThat(info.label).isEqualTo("Submit whole topic"); assertThat(info.method).isEqualTo("POST"); assertThat(info.title).isEqualTo( - "See the \"Submitted Together\" tab for problems"); + "See the \"Submitted Together\" tab for problems, specially see: 2"); } else { noSubmitWholeTopicAssertions(actions, 1); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ConfigChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ConfigChangeIT.java new file mode 100644 index 0000000000..c4216cd5fd --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ConfigChangeIT.java @@ -0,0 +1,154 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.rest.change; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.fail; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.TestProjectInput; +import com.google.gerrit.common.data.Permission; +import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.api.projects.ProjectInput; +import com.google.gerrit.extensions.client.ChangeStatus; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.project.Util; + +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectLoader; +import org.eclipse.jgit.revwalk.RevObject; +import org.eclipse.jgit.revwalk.RevTree; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.RefSpec; +import org.junit.Before; +import org.junit.Test; + +public class ConfigChangeIT extends AbstractDaemonTest { + @Before + public void setUp() throws Exception { + ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); + Util.allow(cfg, Permission.OWNER, REGISTERED_USERS, "refs/*"); + Util.allow( + cfg, Permission.PUSH, REGISTERED_USERS, "refs/for/refs/meta/config"); + Util.allow(cfg, Permission.SUBMIT, REGISTERED_USERS, "refs/meta/config"); + saveProjectConfig(project, cfg); + + setApiUser(user); + fetchRefsMetaConfig(); + } + + @Test + @TestProjectInput(cloneAs = "user") + public void updateProjectConfig() throws Exception { + Config cfg = readProjectConfig(); + assertThat(cfg.getString("project", null, "description")).isNull(); + String desc = "new project description"; + cfg.setString("project", null, "description", desc); + + PushOneCommit.Result r = createConfigChange(cfg); + String id = r.getChangeId(); + + gApi.changes().id(id).current().review(ReviewInput.approve()); + gApi.changes().id(id).current().submit(); + + assertThat(gApi.changes().id(id).info().status) + .isEqualTo(ChangeStatus.MERGED); + assertThat(gApi.projects().name(project.get()).get().description) + .isEqualTo(desc); + fetchRefsMetaConfig(); + assertThat(readProjectConfig().getString("project", null, "description")) + .isEqualTo(desc); + } + + @Test + @TestProjectInput(cloneAs = "user") + public void onlyAdminMayUpdateProjectParent() throws Exception { + setApiUser(admin); + ProjectInput parent = new ProjectInput(); + parent.name = name("parent"); + parent.permissionsOnly = true; + gApi.projects().create(parent); + + setApiUser(user); + Config cfg = readProjectConfig(); + assertThat(cfg.getString("access", null, "inheritFrom")) + .isAnyOf(null, allProjects.get()); + cfg.setString("access", null, "inheritFrom", parent.name); + + PushOneCommit.Result r = createConfigChange(cfg); + String id = r.getChangeId(); + + gApi.changes().id(id).current().review(ReviewInput.approve()); + try { + gApi.changes().id(id).current().submit(); + fail("expected submit to fail"); + } catch (ResourceConflictException e) { + int n = gApi.changes().id(id).info()._number; + assertThat(e).hasMessage( + "Failed to submit 1 change due to the following problems:\n" + + "Change " + n + ": Change contains a project configuration that" + +" changes the parent project.\n" + + "The change must be submitted by a Gerrit administrator."); + } + + assertThat(gApi.projects().name(project.get()).get().parent) + .isEqualTo(allProjects.get()); + fetchRefsMetaConfig(); + assertThat(readProjectConfig().getString("access", null, "inheritFrom")) + .isAnyOf(null, allProjects.get()); + + setApiUser(admin); + gApi.changes().id(id).current().submit(); + assertThat(gApi.changes().id(id).info().status) + .isEqualTo(ChangeStatus.MERGED); + assertThat(gApi.projects().name(project.get()).get().parent) + .isEqualTo(parent.name); + fetchRefsMetaConfig(); + assertThat(readProjectConfig().getString("access", null, "inheritFrom")) + .isEqualTo(parent.name); + } + + private void fetchRefsMetaConfig() throws Exception { + git().fetch().setRefSpecs(new RefSpec("refs/meta/config:refs/meta/config")) + .call(); + testRepo.reset("refs/meta/config"); + } + + private Config readProjectConfig() throws Exception { + RevWalk rw = testRepo.getRevWalk(); + RevTree tree = rw.parseTree(testRepo.getRepository().resolve("HEAD")); + RevObject obj = rw.parseAny(testRepo.get(tree, "project.config")); + ObjectLoader loader = rw.getObjectReader().open(obj); + String text = new String(loader.getCachedBytes(), UTF_8); + Config cfg = new Config(); + cfg.fromText(text); + return cfg; + } + + private PushOneCommit.Result createConfigChange(Config cfg) throws Exception { + PushOneCommit.Result r = pushFactory.create( + db, user.getIdent(), testRepo, + "Update project config", + "project.config", + cfg.toText()) + .to("refs/for/refs/meta/config"); + r.assertOkStatus(); + return r; + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java index a7c75efb68..5a6c36addc 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java @@ -294,7 +294,7 @@ public class SubmitResolvingMergeCommitIT extends AbstractDaemonTest { OrmException { ChangeSet cs = mergeSuperSet.completeChangeSet(db, change.change(), user(admin)); - assertThat(submit.isPatchSetMergeable(cs)).isEqualTo(expected); + assertThat(submit.unmergeableChanges(cs).isEmpty()).isEqualTo(expected); } private void assertMergeable(ChangeData change, boolean expected) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/AbandonRestoreIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/AbandonRestoreIT.java new file mode 100644 index 0000000000..e07405fc69 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/AbandonRestoreIT.java @@ -0,0 +1,89 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.ssh; + +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.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.PushOneCommit.Result; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.ChangeMessageInfo; + +import org.junit.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; + +@NoHttpd +public class AbandonRestoreIT extends AbstractDaemonTest { + + @Test + public void withMessage() throws Exception { + Result result = createChange(); + String commit = result.getCommit().name(); + executeCmd(commit, "abandon", "'abandon it'"); + executeCmd(commit, "restore", "'restore it'"); + assertChangeMessages(result.getChangeId(), ImmutableList.of( + "Uploaded patch set 1.", + "Abandoned\n\nabandon it", + "Restored\n\nrestore it")); + } + + @Test + public void withoutMessage() throws Exception { + Result result = createChange(); + String commit = result.getCommit().name(); + executeCmd(commit, "abandon", null); + executeCmd(commit, "restore", null); + assertChangeMessages(result.getChangeId(), ImmutableList.of( + "Uploaded patch set 1.", + "Abandoned", + "Restored")); + } + + private void executeCmd(String commit, String op, String message) + throws Exception { + StringBuilder command = new StringBuilder("gerrit review ") + .append(commit) + .append(" --") + .append(op); + if (message != null) { + command.append(" --message ").append(message); + } + String response = sshSession.exec(command.toString()); + assert_() + .withFailureMessage(sshSession.getError()) + .that(sshSession.hasError()) + .isFalse(); + assertThat(response.toLowerCase(Locale.US)).doesNotContain("error"); + } + + private void assertChangeMessages(String changeId, List expected) + throws Exception { + ChangeInfo c = get(changeId); + Iterable messages = c.messages; + assertThat(messages).isNotNull(); + assertThat(messages).hasSize(expected.size()); + List actual = new ArrayList<>(); + for (ChangeMessageInfo info : messages) { + actual.add(info.message); + } + assertThat(actual).containsExactlyElementsIn(expected); + } +} \ No newline at end of file diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java index e04509bfe4..71942cedf0 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java @@ -142,6 +142,7 @@ public class Reviewers extends Composite { addReviewerIcon.setVisible(true); UIObject.setVisible(form, false); suggestBox.setFocus(false); + suggestBox.setText(""); } private void addReviewer(final String reviewer, boolean confirmed) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java new file mode 100644 index 0000000000..e38f88c731 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java @@ -0,0 +1,286 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server; + +import com.google.common.base.Function; +import com.google.common.base.MoreObjects; +import com.google.common.base.Strings; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; +import com.google.common.collect.Ordering; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.common.errors.NoSuchGroupException; +import com.google.gerrit.extensions.common.AccountInfo; +import com.google.gerrit.extensions.common.GroupBaseInfo; +import com.google.gerrit.extensions.common.SuggestedReviewerInfo; +import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.Url; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.AccountExternalId; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.account.AccountCache; +import com.google.gerrit.server.account.AccountControl; +import com.google.gerrit.server.account.AccountLoader; +import com.google.gerrit.server.account.GroupBackend; +import com.google.gerrit.server.account.GroupMembers; +import com.google.gerrit.server.change.PostReviewers; +import com.google.gerrit.server.change.ReviewerSuggestionCache; +import com.google.gerrit.server.change.SuggestReviewers; +import com.google.gerrit.server.project.NoSuchProjectException; +import com.google.gerrit.server.project.ProjectControl; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; + +public class ReviewersUtil { + private static final String MAX_SUFFIX = "\u9fa5"; + private static final Ordering ORDERING = + Ordering.natural().onResultOf(new Function() { + @Nullable + @Override + public String apply(@Nullable SuggestedReviewerInfo suggestedReviewerInfo) { + if (suggestedReviewerInfo == null) { + return null; + } + return suggestedReviewerInfo.account != null + ? MoreObjects.firstNonNull(suggestedReviewerInfo.account.email, + Strings.nullToEmpty(suggestedReviewerInfo.account.name)) + : Strings.nullToEmpty(suggestedReviewerInfo.group.name); + } + }); + private final AccountLoader accountLoader; + private final AccountCache accountCache; + private final ReviewerSuggestionCache reviewerSuggestionCache; + private final AccountControl accountControl; + private final Provider dbProvider; + private final GroupBackend groupBackend; + private final GroupMembers.Factory groupMembersFactory; + private final Provider currentUser; + + @Inject + ReviewersUtil(AccountLoader.Factory accountLoaderFactory, + AccountCache accountCache, + ReviewerSuggestionCache reviewerSuggestionCache, + AccountControl.Factory accountControlFactory, + Provider dbProvider, + GroupBackend groupBackend, + GroupMembers.Factory groupMembersFactory, + Provider currentUser) { + this.accountLoader = accountLoaderFactory.create(true); + this.accountCache = accountCache; + this.reviewerSuggestionCache = reviewerSuggestionCache; + this.accountControl = accountControlFactory.get(); + this.dbProvider = dbProvider; + this.groupBackend = groupBackend; + this.groupMembersFactory = groupMembersFactory; + this.currentUser = currentUser; + } + + public interface VisibilityControl { + boolean isVisibleTo(Account.Id account) throws OrmException; + } + + public List suggestReviewers( + SuggestReviewers suggestReviewers, ProjectControl projectControl, + VisibilityControl visibilityControl) + throws IOException, OrmException, BadRequestException { + String query = suggestReviewers.getQuery(); + boolean suggestAccounts = suggestReviewers.getSuggestAccounts(); + int suggestFrom = suggestReviewers.getSuggestFrom(); + boolean useFullTextSearch = suggestReviewers.getUseFullTextSearch(); + int limit = suggestReviewers.getLimit(); + + if (Strings.isNullOrEmpty(query)) { + throw new BadRequestException("missing query field"); + } + + if (!suggestAccounts || query.length() < suggestFrom) { + return Collections.emptyList(); + } + + List suggestedAccounts; + if (useFullTextSearch) { + suggestedAccounts = suggestAccountFullTextSearch(suggestReviewers, visibilityControl); + } else { + suggestedAccounts = suggestAccount(suggestReviewers, visibilityControl); + } + + List reviewer = Lists.newArrayList(); + for (AccountInfo a : suggestedAccounts) { + SuggestedReviewerInfo info = new SuggestedReviewerInfo(); + info.account = a; + reviewer.add(info); + } + + for (GroupReference g : suggestAccountGroup(suggestReviewers, projectControl)) { + if (suggestGroupAsReviewer(suggestReviewers, projectControl.getProject(), + g, visibilityControl)) { + GroupBaseInfo info = new GroupBaseInfo(); + info.id = Url.encode(g.getUUID().get()); + info.name = g.getName(); + SuggestedReviewerInfo suggestedReviewerInfo = new SuggestedReviewerInfo(); + suggestedReviewerInfo.group = info; + reviewer.add(suggestedReviewerInfo); + } + } + + reviewer = ORDERING.immutableSortedCopy(reviewer); + if (reviewer.size() <= limit) { + return reviewer; + } else { + return reviewer.subList(0, limit); + } + } + + private List suggestAccountFullTextSearch( + SuggestReviewers suggestReviewers, VisibilityControl visibilityControl) + throws IOException, OrmException { + List results = reviewerSuggestionCache.search( + suggestReviewers.getQuery(), suggestReviewers.getFullTextMaxMatches()); + + Iterator it = results.iterator(); + while (it.hasNext()) { + Account.Id accountId = new Account.Id(it.next()._accountId); + if (!(visibilityControl.isVisibleTo(accountId) + && accountControl.canSee(accountId))) { + it.remove(); + } + } + + return results; + } + + private List suggestAccount(SuggestReviewers suggestReviewers, + VisibilityControl visibilityControl) + throws OrmException { + String query = suggestReviewers.getQuery(); + int limit = suggestReviewers.getLimit(); + + String a = query; + String b = a + MAX_SUFFIX; + + Map r = new LinkedHashMap<>(); + Map queryEmail = new HashMap<>(); + + for (Account p : dbProvider.get().accounts() + .suggestByFullName(a, b, limit)) { + if (p.isActive()) { + addSuggestion(r, p.getId(), visibilityControl); + } + } + + if (r.size() < limit) { + for (Account p : dbProvider.get().accounts() + .suggestByPreferredEmail(a, b, limit - r.size())) { + if (p.isActive()) { + addSuggestion(r, p.getId(), visibilityControl); + } + } + } + + if (r.size() < limit) { + for (AccountExternalId e : dbProvider.get().accountExternalIds() + .suggestByEmailAddress(a, b, limit - r.size())) { + if (!r.containsKey(e.getAccountId())) { + Account p = accountCache.get(e.getAccountId()).getAccount(); + if (p.isActive()) { + if (addSuggestion(r, p.getId(), visibilityControl)) { + queryEmail.put(e.getAccountId(), e.getEmailAddress()); + } + } + } + } + } + + accountLoader.fill(); + for (Map.Entry p : queryEmail.entrySet()) { + AccountInfo info = r.get(p.getKey()); + if (info != null) { + info.email = p.getValue(); + } + } + return new ArrayList<>(r.values()); + } + + private boolean addSuggestion(Map map, + Account.Id account, VisibilityControl visibilityControl) + throws OrmException { + if (!map.containsKey(account) + // Can the suggestion see the change? + && visibilityControl.isVisibleTo(account) + // Can the account see the current user? + && accountControl.canSee(account)) { + map.put(account, accountLoader.get(account)); + return true; + } + return false; + } + + private List suggestAccountGroup( + SuggestReviewers suggestReviewers, ProjectControl ctl) { + return Lists.newArrayList( + Iterables.limit(groupBackend.suggest(suggestReviewers.getQuery(), ctl), + suggestReviewers.getLimit())); + } + + private boolean suggestGroupAsReviewer(SuggestReviewers suggestReviewers, + Project project, GroupReference group, + VisibilityControl visibilityControl) throws OrmException, IOException { + int maxAllowed = suggestReviewers.getMaxAllowed(); + + if (!PostReviewers.isLegalReviewerGroup(group.getUUID())) { + return false; + } + + try { + Set members = groupMembersFactory + .create(currentUser.get()) + .listAccounts(group.getUUID(), project.getNameKey()); + + if (members.isEmpty()) { + return false; + } + + if (maxAllowed > 0 && members.size() > maxAllowed) { + return false; + } + + // require that at least one member in the group can see the change + for (Account account : members) { + if (visibilityControl.isVisibleTo(account.getId())) { + return true; + } + } + } catch (NoSuchGroupException e) { + return false; + } catch (NoSuchProjectException e) { + return false; + } + + return false; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index 446c8ed306..a9bf220e20 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java @@ -52,7 +52,7 @@ import com.google.gerrit.server.change.Revert; import com.google.gerrit.server.change.Reviewers; import com.google.gerrit.server.change.Revisions; import com.google.gerrit.server.change.SubmittedTogether; -import com.google.gerrit.server.change.SuggestReviewers; +import com.google.gerrit.server.change.SuggestChangeReviewers; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; @@ -77,7 +77,7 @@ class ChangeApiImpl implements ChangeApi { private final Revisions revisions; private final ReviewerApiImpl.Factory reviewerApi; private final RevisionApiImpl.Factory revisionApi; - private final Provider suggestReviewers; + private final Provider suggestReviewers; private final ChangeResource change; private final Abandon abandon; private final Revert revert; @@ -104,7 +104,7 @@ class ChangeApiImpl implements ChangeApi { Revisions revisions, ReviewerApiImpl.Factory reviewerApi, RevisionApiImpl.Factory revisionApi, - Provider suggestReviewers, + Provider suggestReviewers, Abandon abandon, Revert revert, Restore restore, @@ -304,7 +304,7 @@ class ChangeApiImpl implements ChangeApi { private List suggestReviewers(SuggestedReviewersRequest r) throws RestApiException { try { - SuggestReviewers mySuggestReviewers = suggestReviewers.get(); + SuggestChangeReviewers mySuggestReviewers = suggestReviewers.get(); mySuggestReviewers.setQuery(r.getQuery()); mySuggestReviewers.setLimit(r.getLimit()); return mySuggestReviewers.apply(change); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index 0ef8b514c6..52446fe68f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java @@ -26,6 +26,7 @@ import static com.google.gerrit.server.change.VoteResource.VOTE_KIND; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.RestApiModule; import com.google.gerrit.server.account.AccountLoader; +import com.google.gerrit.server.change.SuggestChangeReviewers; import com.google.gerrit.server.change.Reviewed.DeleteReviewed; import com.google.gerrit.server.change.Reviewed.PutReviewed; @@ -72,7 +73,7 @@ public class Module extends RestApiModule { post(CHANGE_KIND, "index").to(Index.class); post(CHANGE_KIND, "reviewers").to(PostReviewers.class); - get(CHANGE_KIND, "suggest_reviewers").to(SuggestReviewers.class); + get(CHANGE_KIND, "suggest_reviewers").to(SuggestChangeReviewers.class); child(CHANGE_KIND, "reviewers").to(Reviewers.class); get(REVIEWER_KIND).to(GetReviewer.class); delete(REVIEWER_KIND).to(DeleteReviewer.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java index ffb63f626c..20078cca1b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java @@ -102,7 +102,7 @@ public class ReviewerSuggestionCache { }); } - List search(String query, int n) throws IOException { + public List search(String query, int n) throws IOException { IndexSearcher searcher = get(); if (searcher == null) { return Collections.emptyList(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index 4e96704dc7..a263096669 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -15,11 +15,14 @@ package com.google.gerrit.server.change; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Function; +import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.common.collect.Multimap; import com.google.common.collect.Sets; import com.google.gerrit.common.data.ParameterizedString; @@ -71,6 +74,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -96,7 +100,7 @@ public class Submit implements RestModifyView, private static final String CLICK_FAILURE_TOOLTIP = "Clicking the button would fail"; private static final String CHANGES_NOT_MERGEABLE = - "See the \"Submitted Together\" tab for problems"; + "See the \"Submitted Together\" tab for problems, specially see: "; public static class Output { transient Change change; @@ -263,11 +267,18 @@ public class Submit implements RestModifyView, MergeOp.checkSubmitRule(c); } - Boolean csIsMergeable = isPatchSetMergeable(cs); - if (csIsMergeable == null) { + Collection unmergeable = unmergeableChanges(cs); + if (unmergeable == null) { return CLICK_FAILURE_TOOLTIP; - } else if (!csIsMergeable) { - return CHANGES_NOT_MERGEABLE; + } else if (!unmergeable.isEmpty()) { + return CHANGES_NOT_MERGEABLE + Joiner.on(", ").join( + Iterables.transform(unmergeable, + new Function() { + @Override + public String apply(ChangeData cd) { + return String.valueOf(cd.getId().get()); + } + })); } } catch (ResourceConflictException e) { return BLOCKED_SUBMIT_TOOLTIP; @@ -407,11 +418,11 @@ public class Submit implements RestModifyView, return change != null ? change.getStatus().name().toLowerCase() : "deleted"; } - public Boolean isPatchSetMergeable(ChangeSet cs) + public Collection unmergeableChanges(ChangeSet cs) throws OrmException, IOException { - Map mergeabilityMap = new HashMap<>(); + Set mergeabilityMap = new HashSet<>(); for (ChangeData change : cs.changes()) { - mergeabilityMap.put(change, false); + mergeabilityMap.add(change); } Multimap cbb = cs.changesByBranch(); @@ -442,17 +453,19 @@ public class Submit implements RestModifyView, // Skip whole check, cannot determine if mergeable return null; } - mergeabilityMap.put(change, mergeable); + if (mergeable) { + mergeabilityMap.remove(change); + } if (isLastInChain && isMergeCommit && mergeable) { for (ChangeData c : targetBranch) { - mergeabilityMap.put(c, true); + mergeabilityMap.remove(c); } break; } } } - return !mergeabilityMap.values().contains(Boolean.FALSE); + return mergeabilityMap; } private HashMap findCommits( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestChangeReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestChangeReviewers.java new file mode 100644 index 0000000000..1596e316fc --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestChangeReviewers.java @@ -0,0 +1,76 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.change; + +import com.google.gerrit.extensions.common.SuggestedReviewerInfo; +import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.RestReadView; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.ReviewersUtil; +import com.google.gerrit.server.IdentifiedUser.GenericFactory; +import com.google.gerrit.server.ReviewersUtil.VisibilityControl; +import com.google.gerrit.server.account.AccountVisibility; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; + +import org.eclipse.jgit.lib.Config; + +import java.io.IOException; +import java.util.List; + +public class SuggestChangeReviewers extends SuggestReviewers + implements RestReadView { + @Inject + SuggestChangeReviewers(AccountVisibility av, + GenericFactory identifiedUserFactory, + Provider dbProvider, + @GerritServerConfig Config cfg, + ReviewersUtil reviewersUtil) { + super(av, identifiedUserFactory, dbProvider, cfg, reviewersUtil); + } + + @Override + public List apply(ChangeResource rsrc) + throws BadRequestException, OrmException, IOException { + return reviewersUtil.suggestReviewers(this, + rsrc.getControl().getProjectControl(), getVisibility(rsrc)); + } + + private VisibilityControl getVisibility(final ChangeResource rsrc) { + if (rsrc.getControl().getRefControl().isVisibleByRegisteredUsers()) { + return new VisibilityControl() { + @Override + public boolean isVisibleTo(Account.Id account) throws OrmException { + return true; + } + }; + } else { + return new VisibilityControl() { + @Override + public boolean isVisibleTo(Account.Id account) throws OrmException { + IdentifiedUser who = + identifiedUserFactory.create(dbProvider, account); + // we can't use changeControl directly as it won't suggest reviewers + // to drafts + return rsrc.getControl().forUser(who).isRefVisible(); + } + }; + } + } +} \ No newline at end of file diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java index 4561ae45bc..3b610336e2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java @@ -14,89 +14,33 @@ package com.google.gerrit.server.change; -import com.google.common.base.Function; -import com.google.common.base.MoreObjects; -import com.google.common.base.Strings; -import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; -import com.google.common.collect.Ordering; -import com.google.gerrit.common.Nullable; -import com.google.gerrit.common.data.GroupReference; -import com.google.gerrit.common.errors.NoSuchGroupException; -import com.google.gerrit.extensions.common.AccountInfo; -import com.google.gerrit.extensions.common.GroupBaseInfo; -import com.google.gerrit.extensions.common.SuggestedReviewerInfo; -import com.google.gerrit.extensions.restapi.BadRequestException; -import com.google.gerrit.extensions.restapi.RestReadView; -import com.google.gerrit.extensions.restapi.Url; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.AccountExternalId; -import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.account.AccountCache; -import com.google.gerrit.server.account.AccountControl; -import com.google.gerrit.server.account.AccountLoader; +import com.google.gerrit.server.ReviewersUtil; import com.google.gerrit.server.account.AccountVisibility; -import com.google.gerrit.server.account.GroupBackend; -import com.google.gerrit.server.account.GroupMembers; import com.google.gerrit.server.config.GerritServerConfig; -import com.google.gerrit.server.project.NoSuchProjectException; -import com.google.gerrit.server.project.ProjectControl; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import org.eclipse.jgit.lib.Config; import org.kohsuke.args4j.Option; -import java.io.IOException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.Iterator; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import java.util.Set; - -public class SuggestReviewers implements RestReadView { - private static final String MAX_SUFFIX = "\u9fa5"; +public class SuggestReviewers { private static final int DEFAULT_MAX_SUGGESTED = 10; private static final int DEFAULT_MAX_MATCHES = 100; - private static final Ordering ORDERING = - Ordering.natural().onResultOf(new Function() { - @Nullable - @Override - public String apply(@Nullable SuggestedReviewerInfo suggestedReviewerInfo) { - if (suggestedReviewerInfo == null) { - return null; - } - return suggestedReviewerInfo.account != null - ? MoreObjects.firstNonNull(suggestedReviewerInfo.account.email, - Strings.nullToEmpty(suggestedReviewerInfo.account.name)) - : Strings.nullToEmpty(suggestedReviewerInfo.group.name); - } - }); - private final AccountLoader accountLoader; - private final AccountControl accountControl; - private final GroupMembers.Factory groupMembersFactory; - private final AccountCache accountCache; - private final Provider dbProvider; - private final Provider currentUser; - private final IdentifiedUser.GenericFactory identifiedUserFactory; - private final GroupBackend groupBackend; + protected final Provider dbProvider; + protected final IdentifiedUser.GenericFactory identifiedUserFactory; + protected final ReviewersUtil reviewersUtil; + private final boolean suggestAccounts; private final int suggestFrom; private final int maxAllowed; - private int limit; - private String query; + protected int limit; + protected String query; private boolean useFullTextSearch; private final int fullTextMaxMatches; - private final int maxSuggestedReviewers; - private final ReviewerSuggestionCache reviewerSuggestionCache; + protected final int maxSuggestedReviewers; @Option(name = "--limit", aliases = {"-n"}, metaVar = "CNT", usage = "maximum number of reviewers to list") @@ -112,27 +56,43 @@ public class SuggestReviewers implements RestReadView { this.query = q; } + public String getQuery() { + return query; + } + + public boolean getSuggestAccounts() { + return suggestAccounts; + } + + public int getSuggestFrom() { + return suggestFrom; + } + + public boolean getUseFullTextSearch() { + return useFullTextSearch; + } + + public int getFullTextMaxMatches() { + return fullTextMaxMatches; + } + + public int getLimit() { + return limit; + } + + public int getMaxAllowed() { + return maxAllowed; + } + @Inject - SuggestReviewers(AccountVisibility av, - AccountLoader.Factory accountLoaderFactory, - AccountControl.Factory accountControlFactory, - AccountCache accountCache, - GroupMembers.Factory groupMembersFactory, + public SuggestReviewers(AccountVisibility av, IdentifiedUser.GenericFactory identifiedUserFactory, - Provider currentUser, Provider dbProvider, @GerritServerConfig Config cfg, - GroupBackend groupBackend, - ReviewerSuggestionCache reviewerSuggestionCache) { - this.accountLoader = accountLoaderFactory.create(true); - this.accountControl = accountControlFactory.get(); - this.accountCache = accountCache; - this.groupMembersFactory = groupMembersFactory; + ReviewersUtil reviewersUtil) { this.dbProvider = dbProvider; this.identifiedUserFactory = identifiedUserFactory; - this.currentUser = currentUser; - this.groupBackend = groupBackend; - this.reviewerSuggestionCache = reviewerSuggestionCache; + this.reviewersUtil = reviewersUtil; this.maxSuggestedReviewers = cfg.getInt("suggest", "maxSuggestedReviewers", DEFAULT_MAX_SUGGESTED); this.limit = this.maxSuggestedReviewers; @@ -152,196 +112,4 @@ public class SuggestReviewers implements RestReadView { this.maxAllowed = cfg.getInt("addreviewer", "maxAllowed", PostReviewers.DEFAULT_MAX_REVIEWERS); } - - private interface VisibilityControl { - boolean isVisibleTo(Account.Id account) throws OrmException; - } - - @Override - public List apply(ChangeResource rsrc) - throws BadRequestException, OrmException, IOException { - if (Strings.isNullOrEmpty(query)) { - throw new BadRequestException("missing query field"); - } - - if (!suggestAccounts || query.length() < suggestFrom) { - return Collections.emptyList(); - } - - VisibilityControl visibilityControl = getVisibility(rsrc); - List suggestedAccounts; - if (useFullTextSearch) { - suggestedAccounts = suggestAccountFullTextSearch(visibilityControl); - } else { - suggestedAccounts = suggestAccount(visibilityControl); - } - - List reviewer = Lists.newArrayList(); - for (AccountInfo a : suggestedAccounts) { - SuggestedReviewerInfo info = new SuggestedReviewerInfo(); - info.account = a; - reviewer.add(info); - } - - Project p = rsrc.getControl().getProject(); - for (GroupReference g : suggestAccountGroup( - rsrc.getControl().getProjectControl())) { - if (suggestGroupAsReviewer(p, g, visibilityControl)) { - GroupBaseInfo info = new GroupBaseInfo(); - info.id = Url.encode(g.getUUID().get()); - info.name = g.getName(); - SuggestedReviewerInfo suggestedReviewerInfo = new SuggestedReviewerInfo(); - suggestedReviewerInfo.group = info; - reviewer.add(suggestedReviewerInfo); - } - } - - reviewer = ORDERING.immutableSortedCopy(reviewer); - if (reviewer.size() <= limit) { - return reviewer; - } else { - return reviewer.subList(0, limit); - } - } - - private VisibilityControl getVisibility(final ChangeResource rsrc) { - if (rsrc.getControl().getRefControl().isVisibleByRegisteredUsers()) { - return new VisibilityControl() { - @Override - public boolean isVisibleTo(Account.Id account) throws OrmException { - return true; - } - }; - } else { - return new VisibilityControl() { - @Override - public boolean isVisibleTo(Account.Id account) throws OrmException { - IdentifiedUser who = - identifiedUserFactory.create(dbProvider, account); - // we can't use changeControl directly as it won't suggest reviewers - // to drafts - return rsrc.getControl().forUser(who).isRefVisible(); - } - }; - } - } - - private List suggestAccountGroup(ProjectControl ctl) { - return Lists.newArrayList( - Iterables.limit(groupBackend.suggest(query, ctl), limit)); - } - - private List suggestAccount(VisibilityControl visibilityControl) - throws OrmException { - String a = query; - String b = a + MAX_SUFFIX; - - Map r = new LinkedHashMap<>(); - Map queryEmail = new HashMap<>(); - - for (Account p : dbProvider.get().accounts() - .suggestByFullName(a, b, limit)) { - if (p.isActive()) { - addSuggestion(r, p.getId(), visibilityControl); - } - } - - if (r.size() < limit) { - for (Account p : dbProvider.get().accounts() - .suggestByPreferredEmail(a, b, limit - r.size())) { - if (p.isActive()) { - addSuggestion(r, p.getId(), visibilityControl); - } - } - } - - if (r.size() < limit) { - for (AccountExternalId e : dbProvider.get().accountExternalIds() - .suggestByEmailAddress(a, b, limit - r.size())) { - if (!r.containsKey(e.getAccountId())) { - Account p = accountCache.get(e.getAccountId()).getAccount(); - if (p.isActive()) { - if (addSuggestion(r, p.getId(), visibilityControl)) { - queryEmail.put(e.getAccountId(), e.getEmailAddress()); - } - } - } - } - } - - accountLoader.fill(); - for (Map.Entry p : queryEmail.entrySet()) { - AccountInfo info = r.get(p.getKey()); - if (info != null) { - info.email = p.getValue(); - } - } - return new ArrayList<>(r.values()); - } - - private List suggestAccountFullTextSearch( - VisibilityControl visibilityControl) throws IOException, OrmException { - List results = reviewerSuggestionCache.search( - query, fullTextMaxMatches); - - Iterator it = results.iterator(); - while (it.hasNext()) { - Account.Id accountId = new Account.Id(it.next()._accountId); - if (!(visibilityControl.isVisibleTo(accountId) - && accountControl.canSee(accountId))) { - it.remove(); - } - } - - return results; - } - - private boolean addSuggestion(Map map, - Account.Id account, VisibilityControl visibilityControl) - throws OrmException { - if (!map.containsKey(account) - // Can the suggestion see the change? - && visibilityControl.isVisibleTo(account) - // Can the account see the current user? - && accountControl.canSee(account)) { - map.put(account, accountLoader.get(account)); - return true; - } - return false; - } - - private boolean suggestGroupAsReviewer(Project project, - GroupReference group, VisibilityControl visibilityControl) - throws OrmException, IOException { - if (!PostReviewers.isLegalReviewerGroup(group.getUUID())) { - return false; - } - - try { - Set members = groupMembersFactory - .create(currentUser.get()) - .listAccounts(group.getUUID(), project.getNameKey()); - - if (members.isEmpty()) { - return false; - } - - if (maxAllowed > 0 && members.size() > maxAllowed) { - return false; - } - - // require that at least one member in the group can see the change - for (Account account : members) { - if (visibilityControl.isVisibleTo(account.getId())) { - return true; - } - } - } catch (NoSuchGroupException e) { - return false; - } catch (NoSuchProjectException e) { - return false; - } - - return false; - } } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java index cec3249d52..fed8226e6a 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java @@ -157,6 +157,9 @@ class SshLog implements LifecycleListener { } private Multimap extractParameters(DispatchCommand dcmd) { + if (dcmd == null) { + return ArrayListMultimap.create(0, 0); + } String[] cmdArgs = dcmd.getArguments(); String paramName = null; int argPos = 0; @@ -274,6 +277,9 @@ class SshLog implements LifecycleListener { } private String extractWhat(DispatchCommand dcmd) { + if (dcmd == null) { + return "Command was already destroyed"; + } StringBuilder commandName = new StringBuilder(dcmd.getCommandName()); String[] args = dcmd.getArguments(); for (int i = 1; i < args.length; i++) { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java index ce969da7f7..00cf53f36f 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java @@ -262,9 +262,6 @@ public class ReviewCommand extends SshCommand { } private void reviewPatchSet(final PatchSet patchSet) throws Exception { - if (changeComment == null) { - changeComment = ""; - } if (notify == null) { notify = NotifyHandling.ALL; } @@ -283,22 +280,20 @@ public class ReviewCommand extends SshCommand { } review.labels.putAll(customLabels); - // If review labels are being applied, the comment will be included - // on the review note. We don't need to add it again on the abandon - // or restore comment. - if (!review.labels.isEmpty() && (abandonChange || restoreChange)) { - changeComment = null; + // We don't need to add the review comment when abandoning/restoring. + if (abandonChange || restoreChange) { + review.message = null; } try { if (abandonChange) { AbandonInput input = new AbandonInput(); - input.message = changeComment; + input.message = Strings.emptyToNull(changeComment); applyReview(patchSet, review); changeApi(patchSet).abandon(input); } else if (restoreChange) { RestoreInput input = new RestoreInput(); - input.message = changeComment; + input.message = Strings.emptyToNull(changeComment); changeApi(patchSet).restore(input); applyReview(patchSet, review); } else { diff --git a/lib/commons/BUCK b/lib/commons/BUCK index 2ed62f670b..cc503a39f8 100644 --- a/lib/commons/BUCK +++ b/lib/commons/BUCK @@ -10,8 +10,8 @@ maven_jar( maven_jar( name = 'collections', - id = 'commons-collections:commons-collections:3.2.1', - sha1 = '761ea405b9b37ced573d2df0d1e3a4e0f9edc668', + id = 'commons-collections:commons-collections:3.2.2', + sha1 = '8ad72fe39fa8c91eaaf12aadb21e0c3661fe26d5', license = 'Apache2.0', exclude = ['META-INF/LICENSE.txt', 'META-INF/NOTICE.txt'], attach_source = False,