From cf1bb5487e565532a5028596f6f241b18499e5ea Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Wed, 21 Oct 2015 10:41:50 +0200 Subject: [PATCH 01/10] Prevent usage of not initialized IndexCollection There is a slight window of opportunity that IndexCollection will be used not being fully initialized. Which will lead to QueryParseException[1]. Such situation happen when commit to refs/meta/config was done from plugin triggered by LifecycleListener.start() method. In such situation the online-reindex didn't manage to finish its job and provide results using setSeachIndex(ChangeIndex) method. Then not initialized IndexCollection will be used to reindex changes after commit done by the plugin. This leads to exception described previously. The solution proposed here is to wait in getSearchIndex() method until setSearchIndex() is called at least once. [1] http://paste.openstack.org/show/477095/ Change-Id: I2a9f7d513d152ad1caec839b9ba0aaecbe7abc49 Signed-off-by: Dariusz Luksza --- .../com/google/gerrit/server/index/IndexCollection.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexCollection.java index 2380c76d7e..7a5e89e15c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexCollection.java @@ -23,6 +23,7 @@ import com.google.inject.Singleton; import java.util.Collection; import java.util.Collections; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicReference; /** Dynamic pointers to the index versions used for searching and writing. */ @@ -30,21 +31,29 @@ import java.util.concurrent.atomic.AtomicReference; public class IndexCollection implements LifecycleListener { private final CopyOnWriteArrayList writeIndexes; private final AtomicReference searchIndex; + private final CountDownLatch initLatch; @Inject @VisibleForTesting public IndexCollection() { this.writeIndexes = Lists.newCopyOnWriteArrayList(); this.searchIndex = new AtomicReference<>(); + this.initLatch = new CountDownLatch(1); } /** @return the current search index version. */ public ChangeIndex getSearchIndex() { + try { + initLatch.await(); + } catch (InterruptedException e) { + // ignore + } return searchIndex.get(); } public void setSearchIndex(ChangeIndex index) { ChangeIndex old = searchIndex.getAndSet(index); + initLatch.countDown(); if (old != null && old != index && !writeIndexes.contains(old)) { old.close(); } From 5f239bb3b9b82681a32cb26b2a256b0322cabe7d Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 23 Jun 2015 10:16:42 +0900 Subject: [PATCH 02/10] ChangeEditUtil.publish: Remove declaration of unthrown exception The method does not throw AuthException. Remove the declaration and Javadoc. Change-Id: I282b1f845b5291a0e62e8feefee12424608227f6 --- .../java/com/google/gerrit/server/edit/ChangeEditUtil.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java index 0a65527aad..12a89893f5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java @@ -132,15 +132,14 @@ public class ChangeEditUtil { * its parent. * * @param edit change edit to publish - * @throws AuthException * @throws NoSuchChangeException * @throws IOException * @throws InvalidChangeOperationException * @throws OrmException * @throws ResourceConflictException */ - public void publish(ChangeEdit edit) throws AuthException, - NoSuchChangeException, IOException, InvalidChangeOperationException, + public void publish(ChangeEdit edit) throws NoSuchChangeException, + IOException, InvalidChangeOperationException, OrmException, ResourceConflictException { Change change = edit.getChange(); try (Repository repo = gitManager.openRepository(change.getProject()); From bcee7ff75a237f5bb9e16156f807853ad3ece6ef Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 22 Jun 2015 16:25:37 +0900 Subject: [PATCH 03/10] Handle commit validation errors when publishing change edit When a commit validator throws a CommitValidationException during publish of a new patch set from the inline editor, it is translated to an InvalidChangeOperationException which results in an internal server error message being displayed to the user. Instead, catch the InvalidChangeOperationException and throw a ResourceConflictException, which results in the actual error message being displayed to the user. The corresponding change in the cookbook plugin includes an example of commit validation to reproduce this case. Bug: Issue 3442 Change-Id: I95b88dc6f2bc76df088ebef50a52974b33cab5ad --- .../gerrit/server/change/PublishChangeEdit.java | 3 +-- .../gerrit/server/edit/ChangeEditUtil.java | 16 +++++++++------- plugins/cookbook-plugin | 2 +- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java index 711cf1829e..b88931ed20 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java @@ -29,7 +29,6 @@ import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.server.edit.ChangeEdit; import com.google.gerrit.server.edit.ChangeEditUtil; -import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -85,7 +84,7 @@ public class PublishChangeEdit implements @Override public Response apply(ChangeResource rsrc, Publish.Input in) throws AuthException, ResourceConflictException, NoSuchChangeException, - IOException, InvalidChangeOperationException, OrmException { + IOException, OrmException { Capable r = rsrc.getControl().getProjectControl().canPushToAtLeastOneRef(); if (r != Capable.OK) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java index 12a89893f5..29bda1dcf0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java @@ -134,13 +134,11 @@ public class ChangeEditUtil { * @param edit change edit to publish * @throws NoSuchChangeException * @throws IOException - * @throws InvalidChangeOperationException * @throws OrmException * @throws ResourceConflictException */ public void publish(ChangeEdit edit) throws NoSuchChangeException, - IOException, InvalidChangeOperationException, - OrmException, ResourceConflictException { + IOException, OrmException, ResourceConflictException { Change change = edit.getChange(); try (Repository repo = gitManager.openRepository(change.getProject()); RevWalk rw = new RevWalk(repo); @@ -151,10 +149,14 @@ public class ChangeEditUtil { "only edit for current patch set can be published"); } - insertPatchSet(edit, change, repo, rw, basePatchSet, - squashEdit(rw, inserter, edit.getEditCommit(), basePatchSet)); - // TODO(davido): This should happen in the same BatchRefUpdate. - deleteRef(repo, edit); + try { + insertPatchSet(edit, change, repo, rw, basePatchSet, + squashEdit(rw, inserter, edit.getEditCommit(), basePatchSet)); + // TODO(davido): This should happen in the same BatchRefUpdate. + deleteRef(repo, edit); + } catch (InvalidChangeOperationException e) { + throw new ResourceConflictException(e.getMessage()); + } } } diff --git a/plugins/cookbook-plugin b/plugins/cookbook-plugin index 955332734b..abba494014 160000 --- a/plugins/cookbook-plugin +++ b/plugins/cookbook-plugin @@ -1 +1 @@ -Subproject commit 955332734bd554527064f8af32907ee1c1561d2c +Subproject commit abba4940141178bbc7c6ad38690262fd93533ded From 3854919bfa911c3b9ea4b408a67399cbc7f5a2f4 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 23 Jun 2015 13:20:16 +0200 Subject: [PATCH 04/10] Handle commit validation errors when creating new change via REST When a commit validator throws a CommitValidationException during the creation of a new change through the create change REST endpoint, it is translated to an InvalidChangeOperationException. This results in an internal server error message being displayed to the user when the user tries to create a new change from the project info screen by clicking on the 'Create Change' button under 'Project Commands'. Instead, translate the CommitValidationException into a ResourceConflictException, which results in the actual error message being displayed to the user. Change-Id: I93a855ad61f04317640c1f615949dda0459554dd Signed-off-by: Edwin Kempin --- .../java/com/google/gerrit/server/change/CreateChange.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java index 4b5afe24e0..22aa84a4ed 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java @@ -24,6 +24,7 @@ import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; +import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; @@ -117,7 +118,7 @@ public class CreateChange implements ChangeInfo input) throws AuthException, OrmException, BadRequestException, UnprocessableEntityException, IOException, InvalidChangeOperationException, ResourceNotFoundException, - MethodNotAllowedException { + MethodNotAllowedException, ResourceConflictException { if (Strings.isNullOrEmpty(input.project)) { throw new BadRequestException("project must be non-empty"); } @@ -230,7 +231,7 @@ public class CreateChange implements private void validateCommit(Repository git, RefControl refControl, RevCommit c, IdentifiedUser me, ChangeInserter ins) - throws InvalidChangeOperationException { + throws ResourceConflictException { PatchSet newPatchSet = ins.getPatchSet(); CommitValidators commitValidators = commitValidatorsFactory.create(refControl, new NoSshInfo(), git); @@ -247,7 +248,7 @@ public class CreateChange implements try { commitValidators.validateForGerritCommits(commitReceivedEvent); } catch (CommitValidationException e) { - throw new InvalidChangeOperationException(e.getMessage()); + throw new ResourceConflictException(e.getMessage()); } } From 95248e05967687a3b172283d7b34732b2e833aa5 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 29 Oct 2015 13:51:07 +0000 Subject: [PATCH 05/10] Revert "Prevent usage of not initialized IndexCollection" On master this change is causing an endless loop when running the acceptance tests, see comments on I487111ad. This reverts commit cf1bb5487e565532a5028596f6f241b18499e5ea. Change-Id: Ie81e9961b96e4206d55d9dc4c60514cd3704b4a7 --- .../com/google/gerrit/server/index/IndexCollection.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexCollection.java index 7a5e89e15c..2380c76d7e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexCollection.java @@ -23,7 +23,6 @@ import com.google.inject.Singleton; import java.util.Collection; import java.util.Collections; import java.util.concurrent.CopyOnWriteArrayList; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicReference; /** Dynamic pointers to the index versions used for searching and writing. */ @@ -31,29 +30,21 @@ import java.util.concurrent.atomic.AtomicReference; public class IndexCollection implements LifecycleListener { private final CopyOnWriteArrayList writeIndexes; private final AtomicReference searchIndex; - private final CountDownLatch initLatch; @Inject @VisibleForTesting public IndexCollection() { this.writeIndexes = Lists.newCopyOnWriteArrayList(); this.searchIndex = new AtomicReference<>(); - this.initLatch = new CountDownLatch(1); } /** @return the current search index version. */ public ChangeIndex getSearchIndex() { - try { - initLatch.await(); - } catch (InterruptedException e) { - // ignore - } return searchIndex.get(); } public void setSearchIndex(ChangeIndex index) { ChangeIndex old = searchIndex.getAndSet(index); - initLatch.countDown(); if (old != null && old != index && !writeIndexes.contains(old)) { old.close(); } From ca1e88f882f52b113b0343a78cda159047b1a5ec Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 30 Oct 2015 11:29:26 +0900 Subject: [PATCH 06/10] Set correct revision of cookbook plugin Commit bcee7ff updated the plugin to revision abba494, but that was not based on the head of the branch and resulted in a merge commit when it was submitted. Thus, the revision set on the plugin was not the latest on the branch. Set it to the correct revision. Change-Id: I421c73fefcf382dd84780e09b5e05b3b20413655 --- plugins/cookbook-plugin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/cookbook-plugin b/plugins/cookbook-plugin index abba494014..0f50526955 160000 --- a/plugins/cookbook-plugin +++ b/plugins/cookbook-plugin @@ -1 +1 @@ -Subproject commit abba4940141178bbc7c6ad38690262fd93533ded +Subproject commit 0f5052695546844f92e9730d619062957006055d From 81de6fcbe0b37272612127e35f45f9a4407f14e7 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Thu, 16 Apr 2015 14:17:40 -0700 Subject: [PATCH 07/10] Add username to stream-events queue entries When displaying the command queue, render the command as: "Stream Events (username)" Rather than the current: com.google.gerrit.sshd.commands.StreamEvents$3@6ad4a0ea Change-Id: I9649fa5b2dcdefc71656b2bf72848389fb77cbd5 --- .../java/com/google/gerrit/sshd/commands/StreamEvents.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java index 889d3fb7fd..e756d8687e 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java @@ -94,6 +94,11 @@ final class StreamEvents extends BaseCommand { public void cancel() { onExit(0); } + + @Override + public String toString() { + return "Stream Events (" + currentUser.getAccount().getUserName() + ")"; + } }; /** True if {@link #droppedOutputEvent} needs to be sent. */ From 4a0743226f3f8ef370c348fb50d57ac9997826b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Engelthaler?= Date: Mon, 9 Nov 2015 00:17:44 +0100 Subject: [PATCH 08/10] Remove unused account constants properties changeScreenNewUi and changeScreenOldUi constants are unused after removing old change screen support Change-Id: Ib037d0a0e69b2559a67993128892cdcf4ebe12d9 --- .../google/gerrit/client/account/AccountConstants.properties | 3 --- 1 file changed, 3 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties index 36cb765f49..fe09af5199 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountConstants.properties @@ -32,9 +32,6 @@ myMenuName = Name myMenuUrl = URL myMenuReset = Reset -changeScreenOldUi = Old Screen -changeScreenNewUi = New Screen - tabAccountSummary = Profile tabPreferences = Preferences tabWatchedProjects = Watched Projects From fe1a9138fab2c9ef666c3f2c31367c20a28d8252 Mon Sep 17 00:00:00 2001 From: Simon Hwang Date: Thu, 5 Nov 2015 14:29:27 -0500 Subject: [PATCH 09/10] Fix Incorrect owner group matching behaviour for creating projects This change fixes a critical bug that might add a wrong group as an owner of a project being created by ssh commands. Before this fix, when the --owner argument does not match any group, the commands tried to find a group whose name starts with the argument instead of throwing an error to notify the user. This behaviour seemed to be insecure and this fix prevents the commands to create the project and throws an error if the --owner argument is not valid, that is, if there is no exact match for the group name. In total, the ssh commands that are affected by this change are: CreateGroupCommand, CreateProjectCommand, ListGroupsCommand, and SetMembersCommand. Moreover, it makes more sense to use exactSuggestion instead of bestSuggestion for those commands as well. Bug: Issue 3655 Change-Id: Icec00651b4268a2b70799f1cba739db5248b04f5 --- .../acceptance/ssh/CreateProjectIT.java | 56 +++++++++++++++++++ .../args4j/AccountGroupUUIDHandler.java | 2 +- 2 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/CreateProjectIT.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/CreateProjectIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/CreateProjectIT.java new file mode 100644 index 0000000000..a7791368ff --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/CreateProjectIT.java @@ -0,0 +1,56 @@ +// Copyright (C) 2015 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.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.project.ProjectState; + +import org.junit.Test; + +public class CreateProjectIT extends AbstractDaemonTest { + + @Test + public void withValidGroupName() throws Exception { + String newGroupName = "newGroup"; + adminSession.put("/groups/" + newGroupName); + String newProjectName = "newProject"; + sshSession.exec("gerrit create-project --branch master --owner " + + newGroupName + " " + newProjectName); + assert_().withFailureMessage(sshSession.getError()) + .that(sshSession.hasError()).isFalse(); + ProjectState projectState = + projectCache.get(new Project.NameKey(newProjectName)); + assertThat(projectState).isNotNull(); + } + + @Test + public void withInvalidGroupName() throws Exception { + String newGroupName = "newGroup"; + adminSession.put("/groups/" + newGroupName); + String wrongGroupName = "newG"; + String newProjectName = "newProject"; + sshSession.exec("gerrit create-project --branch master --owner " + + wrongGroupName + " " + newProjectName); + assert_().withFailureMessage(sshSession.getError()) + .that(sshSession.hasError()).isTrue(); + ProjectState projectState = + projectCache.get(new Project.NameKey(newProjectName)); + assertThat(projectState).isNull(); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountGroupUUIDHandler.java b/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountGroupUUIDHandler.java index 406ca58b6d..674fe086c4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountGroupUUIDHandler.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountGroupUUIDHandler.java @@ -43,7 +43,7 @@ public class AccountGroupUUIDHandler extends OptionHandler { public final int parseArguments(final Parameters params) throws CmdLineException { final String n = params.getParameter(0); - final GroupReference group = GroupBackends.findBestSuggestion(groupBackend, n); + GroupReference group = GroupBackends.findExactSuggestion(groupBackend, n); if (group == null) { throw new CmdLineException(owner, "Group \"" + n + "\" does not exist"); } From 4d97777677abccbd86e4023abd01e635984f70ee Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sun, 8 Nov 2015 10:02:54 -0800 Subject: [PATCH 10/10] Update buck to ba9f239f69287a553ca93af76a27484d83693563 Cherry-picked to include the fix: Fix libSystem.dylib load on OS X 10.11 b3 Without this, "buck build gerrit" fails on OSX El Capitan with the error: OSError: dlopen(libSystem.dylib, 6): image not found Revision used here is cherry-picked from: ccae1feb597b8449ae2bdea3a0170077aab10828 on upstream buck. Change-Id: I177c229da3ffb1b155bbfd36e6f544397350940e --- .buckversion | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.buckversion b/.buckversion index 9c09744a03..ffb82a4abe 100644 --- a/.buckversion +++ b/.buckversion @@ -1 +1 @@ -79d36de9f5284f6e833cca81867d6088a25685fb +ba9f239f69287a553ca93af76a27484d83693563