From 21fa49d50fd431e467724a5f97d9bb5b7de4c680 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 14 Jan 2016 15:29:56 -0500 Subject: [PATCH] Remove PatchSetAccess#byRevision(Range) These cannot be implemented efficiently in notedb, and we can get what we need from the secondary index in most cases, or else (in the case of ConsistencyChecker) directly from the repo. This change intends to provide the same behavior, and does not correct for some sketchy usages of patch sets, like failing to check visibility. Change-Id: I2d961f8d2436b44308116c19597f0ed74b9d1337 --- .../server/change/ConsistencyCheckerIT.java | 2 +- .../google/gerrit/reviewdb/client/RevId.java | 4 ++ .../reviewdb/server/PatchSetAccess.java | 8 --- .../server/change/ConsistencyChecker.java | 48 +++++++++-------- .../google/gerrit/server/change/Rebase.java | 27 +++++++--- .../gerrit/server/change/RebaseUtil.java | 52 +++++++++--------- .../gerrit/server/change/Revisions.java | 42 --------------- .../query/change/InternalChangeQuery.java | 24 ++++++++- .../gerrit/server/schema/SchemaVersion.java | 2 +- .../gerrit/server/schema/Schema_118.java | 25 +++++++++ .../gerrit/sshd/commands/CommandUtils.java | 54 +++++++++++-------- .../gerrit/sshd/commands/ReviewCommand.java | 8 ++- plugins/reviewnotes | 2 +- 13 files changed, 164 insertions(+), 134 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_118.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index 272cdcda9a..296b877fc1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -564,7 +564,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { ProblemInfo p = problems.get(0); assertThat(p.message).isEqualTo( "Multiple patch sets for expected merged commit " + commit.name() - + ": [" + ps2 + ", " + ps3 + "]"); + + ": [" + ps2.getId() + ", " + ps3.getId() + "]"); } private Change insertChange() throws Exception { diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RevId.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RevId.java index a73fe5d1fd..42f3017f09 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RevId.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RevId.java @@ -69,4 +69,8 @@ public final class RevId { public String toString() { return getClass().getSimpleName() + "{" + id + "}"; } + + public boolean matches(String str) { + return id.startsWith(str.toLowerCase()); + } } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/PatchSetAccess.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/PatchSetAccess.java index 36e969e282..f7452c5f34 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/PatchSetAccess.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/PatchSetAccess.java @@ -16,7 +16,6 @@ package com.google.gerrit.reviewdb.server; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.RevId; import com.google.gwtorm.server.Access; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.PrimaryKey; @@ -30,11 +29,4 @@ public interface PatchSetAccess extends Access { @Query("WHERE id.changeId = ? ORDER BY id.patchSetId") ResultSet byChange(Change.Id id) throws OrmException; - - @Query("WHERE revision = ?") - ResultSet byRevision(RevId rev) throws OrmException; - - @Query("WHERE revision >= ? AND revision <= ?") - ResultSet byRevisionRange(RevId reva, RevId revb) - throws OrmException; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index 349117fbc2..4447c50332 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -14,14 +14,14 @@ package com.google.gerrit.server.change; +import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES; +import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering; import static com.google.gerrit.server.ChangeUtil.PS_ID_ORDER; import static com.google.gerrit.server.ChangeUtil.TO_PS_ID; import com.google.auto.value.AutoValue; import com.google.common.base.Function; -import com.google.common.base.Predicate; import com.google.common.collect.Collections2; -import com.google.common.collect.FluentIterable; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Multimap; @@ -36,7 +36,6 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CurrentUser; @@ -406,22 +405,27 @@ public class ConsistencyChecker { return; } - RevId revId = new RevId(commit.name()); - List patchSets = FluentIterable - .from(db.get().patchSets().byRevision(revId)) - .filter(new Predicate() { - @Override - public boolean apply(PatchSet ps) { - try { - Change c = db.get().changes().get(ps.getId().getParentKey()); - return c != null && c.getDest().equals(change.getDest()); - } catch (OrmException e) { - warn(e); - return true; // Should cause an error below, that's good. - } - } - }).toSortedList(ChangeUtil.PS_ID_ORDER); - switch (patchSets.size()) { + List psIds = new ArrayList<>(); + for (Ref ref : repo.getRefDatabase().getRefs(REFS_CHANGES).values()) { + if (!ref.getObjectId().equals(commit)) { + continue; + } + PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); + if (psId == null) { + continue; + } + try { + Change c = db.get().changes().get(psId.getParentKey()); + if (c == null || !c.getDest().equals(change.getDest())) { + continue; + } + } catch (OrmException e) { + warn(e); + // Include this patch set; should cause an error below, which is good. + } + psIds.add(psId); + } + switch (psIds.size()) { case 0: // No patch set for this commit; insert one. rw.parseBody(commit); @@ -445,7 +449,7 @@ public class ConsistencyChecker { // patch set. // TODO(dborowitz): This could be fixed if it's an older patch set of // the current change. - PatchSet.Id id = patchSets.get(0).getId(); + PatchSet.Id id = psIds.get(0); if (!id.equals(change.currentPatchSetId())) { problem(String.format("Expected merged commit %s corresponds to" + " patch set %s, which is not the current patch set %s", @@ -456,10 +460,10 @@ public class ConsistencyChecker { default: problem(String.format( "Multiple patch sets for expected merged commit %s: %s", - commit.name(), patchSets)); + commit.name(), intKeyOrdering().sortedCopy(psIds))); break; } - } catch (OrmException | IOException e) { + } catch (IOException e) { error("Error looking up expected merged commit " + fix.expectMergedAs, e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java index e01be621bd..a4d38df26b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java @@ -29,13 +29,15 @@ import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -66,6 +68,7 @@ public class Rebase implements RestModifyView, private final RebaseUtil rebaseUtil; private final ChangeJson.Factory json; private final Provider dbProvider; + private final Provider queryProvider; @Inject public Rebase(BatchUpdate.Factory updateFactory, @@ -73,13 +76,15 @@ public class Rebase implements RestModifyView, RebaseChangeOp.Factory rebaseFactory, RebaseUtil rebaseUtil, ChangeJson.Factory json, - Provider dbProvider) { + Provider dbProvider, + Provider queryProvider) { this.updateFactory = updateFactory; this.repoManager = repoManager; this.rebaseFactory = rebaseFactory; this.rebaseUtil = rebaseUtil; this.json = json; this.dbProvider = dbProvider; + this.queryProvider = queryProvider; } @Override @@ -130,7 +135,7 @@ public class Rebase implements RestModifyView, @SuppressWarnings("resource") ReviewDb db = dbProvider.get(); - PatchSet basePatchSet = parseBase(base); + PatchSet basePatchSet = parseBase(change.getProject(), base); if (basePatchSet == null) { throw new ResourceConflictException("base revision is missing: " + base); } else if (!rsrc.getControl().isPatchVisible(basePatchSet, db)) { @@ -168,7 +173,8 @@ public class Rebase implements RestModifyView, return rw.isMergedInto(rw.parseCommit(baseId), rw.parseCommit(tipId)); } - private PatchSet parseBase(String base) throws OrmException { + private PatchSet parseBase(Project.NameKey project, String base) + throws OrmException { ReviewDb db = dbProvider.get(); PatchSet.Id basePatchSetId = PatchSet.Id.fromRef(base); @@ -193,10 +199,15 @@ public class Rebase implements RestModifyView, } // Try parsing as SHA-1. - for (PatchSet ps : db.patchSets().byRevision(new RevId(base))) { - if (basePatchSet == null - || basePatchSet.getId().get() < ps.getId().get()) { - basePatchSet = ps; + for (ChangeData cd : queryProvider.get().byProjectCommit(project, base)) { + for (PatchSet ps : cd.patchSets()) { + if (!ps.getRevision().matches(base)) { + continue; + } + if (basePatchSet == null + || basePatchSet.getId().get() < ps.getId().get()) { + basePatchSet = ps; + } } } return basePatchSet; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseUtil.java index fecd776d48..b75a6595a1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseUtil.java @@ -22,7 +22,8 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RevId; -import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -41,11 +42,11 @@ import java.io.IOException; public class RebaseUtil { private static final Logger log = LoggerFactory.getLogger(RebaseUtil.class); - private final Provider db; + private final Provider queryProvider; @Inject - RebaseUtil(Provider db) { - this.db = db; + RebaseUtil(Provider queryProvider) { + this.queryProvider = queryProvider; } public boolean canRebase(PatchSet patchSet, Branch.NameKey dest, @@ -97,30 +98,29 @@ public class RebaseUtil { RevId parentRev = new RevId(commit.getParent(0).name()); - for (PatchSet depPatchSet : db.get().patchSets().byRevision(parentRev)) { - Change.Id depChangeId = depPatchSet.getId().getParentKey(); - Change depChange = db.get().changes().get(depChangeId); - if (!depChange.getDest().equals(destBranch)) { - continue; - } - - if (depChange.getStatus() == Status.ABANDONED) { - throw new ResourceConflictException( - "Cannot rebase a change with an abandoned parent: " - + depChange.getKey()); - } - - if (depChange.getStatus().isOpen()) { - if (depPatchSet.getId().equals(depChange.currentPatchSetId())) { - throw new ResourceConflictException( - "Change is already based on the latest patch set of the" - + " dependent change."); + CHANGES: for (ChangeData cd : queryProvider.get() + .byBranchCommit(destBranch, parentRev.get())) { + for (PatchSet depPatchSet : cd.patchSets()) { + if (!depPatchSet.getRevision().equals(parentRev)) { + continue; } - PatchSet latestDepPatchSet = - db.get().patchSets().get(depChange.currentPatchSetId()); - baseRev = latestDepPatchSet.getRevision().get(); + Change depChange = cd.change(); + if (depChange.getStatus() == Status.ABANDONED) { + throw new ResourceConflictException( + "Cannot rebase a change with an abandoned parent: " + + depChange.getKey()); + } + + if (depChange.getStatus().isOpen()) { + if (depPatchSet.getId().equals(depChange.currentPatchSetId())) { + throw new ResourceConflictException( + "Change is already based on the latest patch set of the" + + " dependent change."); + } + baseRev = cd.currentPatchSet().getRevision().get(); + } + break CHANGES; } - break; } if (baseRev == null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java index 734ebb68d5..3a1fb55075 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java @@ -14,11 +14,8 @@ package com.google.gerrit.server.change; -import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.base.Optional; -import com.google.common.base.Predicate; -import com.google.common.collect.FluentIterable; import com.google.common.collect.Lists; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.AuthException; @@ -26,14 +23,12 @@ import com.google.gerrit.extensions.restapi.ChildCollection; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestView; -import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.edit.ChangeEdit; import com.google.gerrit.server.edit.ChangeEditUtil; import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -114,19 +109,7 @@ public class Revisions implements ChildCollection= 8) { - // Commit names are rather unique. Query for the commit and later - // match to the change. This is most likely going to identify 1 or - // at most 2 patch sets to consider, which is smaller than looking - // for all patch sets in the change. - RevId revid = new RevId(id); - if (revid.isComplete()) { - List m = toResources(change, findExactMatch(revid)); - return m.isEmpty() ? loadEdit(change, revid) : m; - } - return toResources(change, findByPrefix(revid)); } else { - // Chance of collision rises; look at all patch sets on the change. List out = Lists.newArrayList(); for (PatchSet ps : dbProvider.get().patchSets() .byChange(change.getId())) { @@ -149,14 +132,6 @@ public class Revisions implements ChildCollection findExactMatch(RevId revid) throws OrmException { - return dbProvider.get().patchSets().byRevision(revid); - } - - private ResultSet findByPrefix(RevId revid) throws OrmException { - return dbProvider.get().patchSets().byRevisionRange(revid, revid.max()); - } - private List loadEdit(ChangeResource change, RevId revid) throws AuthException, IOException { Optional edit = editUtil.byChange(change.getChange()); @@ -170,21 +145,4 @@ public class Revisions implements ChildCollection toResources(final ChangeResource change, - Iterable patchSets) { - final Change.Id changeId = change.getId(); - return FluentIterable.from(patchSets) - .filter(new Predicate() { - @Override - public boolean apply(PatchSet in) { - return changeId.equals(in.getId().getParentKey()); - } - }).transform(new Function() { - @Override - public RevisionResource apply(PatchSet in) { - return new RevisionResource(change, in); - } - }).toList(); - } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java index a2b62086df..9bd3680489 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java @@ -218,7 +218,16 @@ public class InternalChangeQuery { } public List byCommit(ObjectId id) throws OrmException { - return query(commit(schema(indexes), id.name())); + return byCommit(id.name()); + } + + public List byCommit(String hash) throws OrmException { + return query(commit(schema(indexes), hash)); + } + + public List byProjectCommit(Project.NameKey project, + String hash) throws OrmException { + return query(and(project(project), commit(schema(indexes), hash))); } public List byProjectCommits(Project.NameKey project, @@ -228,6 +237,19 @@ public class InternalChangeQuery { return query(and(project(project), or(commits(schema(indexes), hashes)))); } + public List byBranchCommit(String project, String branch, + String hash) throws OrmException { + return query(and( + new ProjectPredicate(project), + new RefPredicate(branch), + commit(schema(indexes), hash))); + } + + public List byBranchCommit(Branch.NameKey branch, String hash) + throws OrmException { + return byBranchCommit(branch.getParentKey().get(), branch.get(), hash); + } + public List bySubmissionId(String cs) throws OrmException { if (Strings.isNullOrEmpty(cs) || !schema(indexes).hasField(SUBMISSIONID)) { return Collections.emptyList(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java index 374a176ac0..dc2a02be41 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java @@ -32,7 +32,7 @@ import java.util.List; /** A version of the database schema. */ public abstract class SchemaVersion { /** The current schema version. */ - public static final Class C = Schema_117.class; + public static final Class C = Schema_118.class; public static int getBinaryVersion() { return guessVersion(C); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_118.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_118.java new file mode 100644 index 0000000000..8c2c740b16 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_118.java @@ -0,0 +1,25 @@ +// 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.schema; + +import com.google.inject.Inject; +import com.google.inject.Provider; + +public class Schema_118 extends SchemaVersion { + @Inject + Schema_118(Provider prior) { + super(prior); + } +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CommandUtils.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CommandUtils.java index 1e89986945..d491759642 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CommandUtils.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/CommandUtils.java @@ -16,36 +16,46 @@ package com.google.gerrit.sshd.commands; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.project.ProjectControl; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.sshd.BaseCommand.UnloggedFailure; import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.ResultSet; -import java.util.HashSet; -import java.util.Set; +import java.util.ArrayList; +import java.util.List; public class CommandUtils { - public static PatchSet parsePatchSet(final String patchIdentity, ReviewDb db, - ProjectControl projectControl, String branch) + public static PatchSet parsePatchSet(String patchIdentity, ReviewDb db, + InternalChangeQuery query, ProjectControl projectControl, String branch) throws UnloggedFailure, OrmException { // By commit? // if (patchIdentity.matches("^([0-9a-fA-F]{4," + RevId.LEN + "})$")) { - final RevId id = new RevId(patchIdentity); - final ResultSet patches; - if (id.isComplete()) { - patches = db.patchSets().byRevision(id); + List cds; + if (projectControl != null) { + Project.NameKey p = projectControl.getProject().getNameKey(); + if (branch != null) { + cds = query.byBranchCommit(p.get(), branch, patchIdentity); + } else { + cds = query.byProjectCommit(p, patchIdentity); + } } else { - patches = db.patchSets().byRevisionRange(id, id.max()); + cds = query.byCommit(patchIdentity); } - - final Set matches = new HashSet<>(); - for (final PatchSet ps : patches) { - final Change change = db.changes().get(ps.getId().getParentKey()); - if (inProject(change, projectControl) && inBranch(change, branch)) { - matches.add(ps); + List matches = new ArrayList<>(cds.size()); + for (ChangeData cd : cds) { + Change c = cd.change(); + if (!(inProject(c, projectControl) && inBranch(c, branch))) { + continue; + } + for (PatchSet ps : cd.patchSets()) { + if (ps.getRevision().matches(patchIdentity)) { + matches.add(ps); + } } } @@ -62,18 +72,18 @@ public class CommandUtils { // By older style change,patchset? // if (patchIdentity.matches("^[1-9][0-9]*,[1-9][0-9]*$")) { - final PatchSet.Id patchSetId; + PatchSet.Id patchSetId; try { patchSetId = PatchSet.Id.parse(patchIdentity); } catch (IllegalArgumentException e) { throw error("\"" + patchIdentity + "\" is not a valid patch set"); } - final PatchSet patchSet = db.patchSets().get(patchSetId); + PatchSet patchSet = db.patchSets().get(patchSetId); if (patchSet == null) { throw error("\"" + patchIdentity + "\" no such patch set"); } if (projectControl != null || branch != null) { - final Change change = db.changes().get(patchSetId.getParentKey()); + Change change = db.changes().get(patchSetId.getParentKey()); if (!inProject(change, projectControl)) { throw error("change " + change.getId() + " not in project " + projectControl.getProject().getName()); @@ -89,7 +99,7 @@ public class CommandUtils { throw error("\"" + patchIdentity + "\" is not a valid patch set"); } - private static boolean inProject(final Change change, + private static boolean inProject(Change change, ProjectControl projectControl) { if (projectControl == null) { // No --project option, so they want every project. @@ -98,7 +108,7 @@ public class CommandUtils { return projectControl.getProject().getNameKey().equals(change.getProject()); } - private static boolean inBranch(final Change change, String branch) { + private static boolean inBranch(Change change, String branch) { if (branch == null) { // No --branch option, so they want every branch. return true; @@ -106,7 +116,7 @@ public class CommandUtils { return change.getDest().get().equals(branch); } - public static UnloggedFailure error(final String msg) { + public static UnloggedFailure error(String msg) { return new UnloggedFailure(1, msg); } } 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 54e07aa333..5fb8939825 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 @@ -36,6 +36,7 @@ import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectControl; +import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.util.LabelVote; import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.SshCommand; @@ -79,8 +80,8 @@ public class ReviewCommand extends SshCommand { usage = "list of commits or patch sets to review") void addPatchSetId(final String token) { try { - PatchSet ps = CommandUtils.parsePatchSet(token, db, projectControl, - branch); + PatchSet ps = CommandUtils.parsePatchSet(token, db, queryProvider.get(), + projectControl, branch); patchSets.add(ps); } catch (UnloggedFailure e) { throw new IllegalArgumentException(e.getMessage(), e); @@ -145,6 +146,9 @@ public class ReviewCommand extends SshCommand { @Inject private Provider gApi; + @Inject + private Provider queryProvider; + private List optionList; private Map customLabels; diff --git a/plugins/reviewnotes b/plugins/reviewnotes index 26f38c4514..34678f04b2 160000 --- a/plugins/reviewnotes +++ b/plugins/reviewnotes @@ -1 +1 @@ -Subproject commit 26f38c4514687c388472be19c9789eaa84b1d564 +Subproject commit 34678f04b28c02cda754dd49c9dd99ff4db60415