Merge changes I5a7749fc,Ia8703c0d
* changes:
Do not fail with ISE when an invalid base is specified on push
Fix GetRelated if multiple changes exist for the same commit
diff --git a/java/com/google/gerrit/server/args4j/ObjectIdHandler.java b/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
index aa8a958..3cad7ce 100644
--- a/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
+++ b/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
@@ -16,9 +16,11 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.lib.ObjectId;
import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
+import org.kohsuke.args4j.NamedOptionDef;
import org.kohsuke.args4j.OptionDef;
import org.kohsuke.args4j.spi.OptionHandler;
import org.kohsuke.args4j.spi.Parameters;
@@ -37,7 +39,14 @@
@Override
public int parseArguments(Parameters params) throws CmdLineException {
final String n = params.getParameter(0);
- setter.addValue(ObjectId.fromString(n));
+ try {
+ setter.addValue(ObjectId.fromString(n));
+ } catch (InvalidObjectIdException e) {
+ throw new CmdLineException(
+ owner,
+ String.format("expected SHA1 for option %s: %s", ((NamedOptionDef) option).name(), n),
+ e);
+ }
return 1;
}
diff --git a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
index b1f9726..194a4f0 100644
--- a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
+++ b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
@@ -72,8 +72,8 @@
}
List<ChangeData> cds =
- InternalChangeQuery.byProjectGroups(
- queryProvider, indexConfig, changeData.project(), groups);
+ InternalChangeQuery.byBranchGroups(
+ queryProvider, indexConfig, changeData.change().getDest(), groups);
if (cds.isEmpty()) {
return Collections.emptyList();
}
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index 60e30bc..cd7e29a 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/java/com/google/gerrit/server/events/EventFactory.java
@@ -295,8 +295,8 @@
// Find changes in the same related group as this patch set, having a patch
// set whose parent matches this patch set's revision.
for (ChangeData cd :
- InternalChangeQuery.byProjectGroups(
- queryProvider, indexConfig, change.getProject(), currentPs.groups())) {
+ InternalChangeQuery.byBranchGroups(
+ queryProvider, indexConfig, change.getDest(), currentPs.groups())) {
PATCH_SETS:
for (PatchSet ps : cd.patchSets()) {
RevCommit commit = rw.parseCommit(ps.commitId());
diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index 99c1ca1..ccd645b 100644
--- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -271,21 +271,21 @@
return query(ChangePredicates.submissionId(cs));
}
- private static Predicate<ChangeData> byProjectGroupsPredicate(
- IndexConfig indexConfig, Project.NameKey project, Collection<String> groups) {
- int n = indexConfig.maxTerms() - 1;
+ private static Predicate<ChangeData> byBranchGroupsPredicate(
+ IndexConfig indexConfig, BranchNameKey branchAndProject, Collection<String> groups) {
+ int n = indexConfig.maxTerms() - 2;
checkArgument(groups.size() <= n, "cannot exceed %s groups", n);
List<GroupPredicate> groupPredicates = new ArrayList<>(groups.size());
for (String g : groups) {
groupPredicates.add(new GroupPredicate(g));
}
- return and(project(project), or(groupPredicates));
+ return and(project(branchAndProject.project()), ref(branchAndProject), or(groupPredicates));
}
- public static ImmutableList<ChangeData> byProjectGroups(
+ public static ImmutableList<ChangeData> byBranchGroups(
Provider<InternalChangeQuery> queryProvider,
IndexConfig indexConfig,
- Project.NameKey project,
+ BranchNameKey branchAndProject,
Collection<String> groups) {
// These queries may be complex along multiple dimensions:
// * Many groups per change, if there are very many patch sets. This requires partitioning the
@@ -296,16 +296,17 @@
// InternalChangeQuery is single-use.
Supplier<InternalChangeQuery> querySupplier = () -> queryProvider.get().enforceVisibility(true);
- int batchSize = indexConfig.maxTerms() - 1;
+ int batchSize = indexConfig.maxTerms() - 2;
if (groups.size() <= batchSize) {
return queryExhaustively(
- querySupplier, byProjectGroupsPredicate(indexConfig, project, groups));
+ querySupplier, byBranchGroupsPredicate(indexConfig, branchAndProject, groups));
}
Set<Change.Id> seen = new HashSet<>();
ImmutableList.Builder<ChangeData> result = ImmutableList.builder();
for (List<String> part : Iterables.partition(groups, batchSize)) {
for (ChangeData cd :
- queryExhaustively(querySupplier, byProjectGroupsPredicate(indexConfig, project, part))) {
+ queryExhaustively(
+ querySupplier, byBranchGroupsPredicate(indexConfig, branchAndProject, part))) {
if (!seen.add(cd.getId())) {
result.add(cd);
}
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 50bb8ae..b738324 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -3082,6 +3082,12 @@
assertThat(r.getChange().attentionSet()).isEmpty();
}
+ @Test
+ public void pushWithInvalidBaseIsRejected() throws Exception {
+ PushOneCommit.Result r = pushTo("refs/for/master%base=invalid");
+ r.assertErrorStatus("expected SHA1 for option --base: invalid");
+ }
+
private DraftInput newDraft(String path, int line, String message) {
DraftInput d = new DraftInput();
d.path = path;
diff --git a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
index 70b5701..21db45c 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
@@ -39,11 +39,13 @@
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.GetRelatedOption;
import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.index.IndexConfig;
@@ -67,6 +69,8 @@
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
import org.junit.Test;
@NoHttpd
@@ -664,6 +668,71 @@
}
}
+ @Test
+ public void getRelatedLinearSameCommitPushedTwice() throws Exception {
+ RevCommit base = projectOperations.project(project).getHead("master");
+
+ // 1,1---2,1 on master
+ PushOneCommit.Result r1 =
+ createChange(
+ testRepo,
+ "master",
+ "subject: 1",
+ "a.txt",
+ "1",
+ /** topic= */
+ null);
+ RevCommit c1_1 = r1.getCommit();
+ PatchSet.Id ps1_1 = r1.getPatchSetId();
+
+ PushOneCommit.Result r2 =
+ createChange(
+ testRepo,
+ "master",
+ "subject: 2",
+ "b.txt",
+ "2",
+ /** topic= */
+ null);
+ RevCommit c2_1 = r2.getCommit();
+ PatchSet.Id ps2_1 = r2.getPatchSetId();
+
+ // 3,1---4,1 on stable
+ gApi.projects().name(project.get()).branch("stable").create(new BranchInput());
+ testRepo.reset(c1_1);
+ PushResult r3 = pushHead(testRepo, "refs/for/stable%base=" + base.getName());
+ assertThat(r3.getRemoteUpdate("refs/for/stable%base=" + base.getName()).getStatus())
+ .isEqualTo(RemoteRefUpdate.Status.OK);
+ ChangeData change3 =
+ Iterables.getOnlyElement(
+ queryProvider
+ .get()
+ .byBranchCommit(BranchNameKey.create(project, "stable"), c1_1.getName()));
+ assertThat(change3.currentPatchSet().commitId()).isEqualTo(c1_1);
+ RevCommit c3_1 = c1_1;
+ PatchSet.Id ps3_1 = change3.currentPatchSet().id();
+
+ PushOneCommit.Result r4 =
+ createChange(
+ testRepo,
+ "stable",
+ "subject: 4",
+ "d.txt",
+ "4",
+ /** topic= */
+ null);
+ RevCommit c4_1 = r4.getCommit();
+ PatchSet.Id ps4_1 = r4.getPatchSetId();
+
+ for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
+ assertRelated(ps, changeAndCommit(ps2_1, c2_1, 1), changeAndCommit(ps1_1, c1_1, 1));
+ }
+
+ for (PatchSet.Id ps : ImmutableList.of(ps4_1, ps3_1)) {
+ assertRelated(ps, changeAndCommit(ps4_1, c4_1, 1), changeAndCommit(ps3_1, c3_1, 1));
+ }
+ }
+
private static Correspondence<RelatedChangeAndCommitInfo, String>
getRelatedChangeToStatusCorrespondence() {
return Correspondence.transforming(
