From e2b9221ad4c3566161d1996c071912da3d3eeac8 Mon Sep 17 00:00:00 2001 From: Colby Ranger Date: Thu, 12 Dec 2013 09:30:31 -0800 Subject: [PATCH] Don't include the same change twice in GetRelated. If the order of commits was modified, while keeping the same change number, GetRelated would return both the new patch set and the old patch set. Instead of walking the children first, walk the parents first and exclude the changes from which the parents came. The changes are still used for generating the children queue, since there could be new children on top of the excluded ones. + Add test for listing related revisions. Change-Id: I297aef2cd966c3ea38501fc3d560d6776f58c09f --- .../gerrit/acceptance/server/change/BUCK | 21 ++ .../server/change/ChangeAndCommit.java | 24 +++ .../server/change/GetRelatedIT.java | 199 ++++++++++++++++++ .../acceptance/server/change/RelatedInfo.java | 21 ++ .../gerrit/server/change/ChangeJson.java | 2 +- .../gerrit/server/change/GetRelated.java | 20 +- 6 files changed, 281 insertions(+), 6 deletions(-) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/BUCK create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ChangeAndCommit.java create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/RelatedInfo.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/BUCK b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/BUCK new file mode 100644 index 0000000000..0157bd61ff --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/BUCK @@ -0,0 +1,21 @@ +include_defs('//gerrit-acceptance-tests/tests.defs') + +acceptance_tests( + srcs = glob(['*IT.java']), + deps = [ + ':util', + '//gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git:util', + '//lib:gwtjsonrpc', + ], +) + +java_library( + name = 'util', + srcs = [ + 'ChangeAndCommit.java', + 'RelatedInfo.java', + ], + deps = [ + '//gerrit-server:server', + ], +) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ChangeAndCommit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ChangeAndCommit.java new file mode 100644 index 0000000000..d9959b2237 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ChangeAndCommit.java @@ -0,0 +1,24 @@ +// Copyright (C) 2013 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.server.change; + +import com.google.gerrit.server.change.ChangeJson.CommitInfo; + +public class ChangeAndCommit { + public String changeId; + public CommitInfo commit; + public Integer _changeNumber; + public Integer _revisionNumber; +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java new file mode 100644 index 0000000000..3ae31b4f42 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java @@ -0,0 +1,199 @@ +// Copyright (C) 2013 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.server.change; + +import static com.google.gerrit.acceptance.git.GitUtil.add; +import static com.google.gerrit.acceptance.git.GitUtil.cloneProject; +import static com.google.gerrit.acceptance.git.GitUtil.createCommit; +import static com.google.gerrit.acceptance.git.GitUtil.createProject; +import static com.google.gerrit.acceptance.git.GitUtil.initSsh; +import static com.google.gerrit.acceptance.git.GitUtil.pushHead; +import static org.junit.Assert.assertEquals; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.AccountCreator; +import com.google.gerrit.acceptance.RestSession; +import com.google.gerrit.acceptance.SshSession; +import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.git.PushOneCommit; +import com.google.gerrit.acceptance.git.GitUtil.Commit; +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.server.ReviewDb; +import com.google.gerrit.server.OutputFormat; +import com.google.gson.reflect.TypeToken; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.SchemaFactory; +import com.google.inject.Inject; + +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.ResetCommand.ResetType; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; +import java.util.List; + +public class GetRelatedIT extends AbstractDaemonTest { + + @Inject + private AccountCreator accounts; + + @Inject + private SchemaFactory reviewDbProvider; + + private TestAccount admin; + private RestSession session; + private Git git; + private ReviewDb db; + + @Before + public void setUp() throws Exception { + admin = accounts.admin(); + session = new RestSession(server, admin); + + initSsh(admin); + Project.NameKey project = new Project.NameKey("p"); + SshSession sshSession = new SshSession(server, admin); + createProject(sshSession, project.get()); + git = cloneProject(sshSession.getUrl() + "/" + project.get()); + sshSession.close(); + db = reviewDbProvider.open(); + } + + @After + public void cleanup() { + db.close(); + } + + @Test + public void getRelatedNoResult() throws GitAPIException, + IOException, Exception { + PushOneCommit push = new PushOneCommit(db, admin.getIdent()); + PatchSet.Id ps = push.to(git, "refs/for/master").getPatchSetId(); + List related = getRelated(ps); + assertEquals(0, related.size()); + } + + @Test + public void getRelatedLinear() throws GitAPIException, + IOException, Exception { + add(git, "a.txt", "1"); + Commit c1 = createCommit(git, admin.getIdent(), "subject: 1"); + add(git, "b.txt", "2"); + Commit c2 = createCommit(git, admin.getIdent(), "subject: 2"); + pushHead(git, "refs/for/master", false); + + for (Commit c : ImmutableList.of(c2, c1)) { + List related = getRelated(getPatchSetId(c)); + assertEquals(2, related.size()); + assertEquals("related to " + c.getChangeId(), c2.getChangeId(), related.get(0).changeId); + assertEquals("related to " + c.getChangeId(), c1.getChangeId(), related.get(1).changeId); + } + } + + @Test + public void getRelatedReorder() throws GitAPIException, + IOException, Exception { + // Create two commits and push. + add(git, "a.txt", "1"); + Commit c1 = createCommit(git, admin.getIdent(), "subject: 1"); + add(git, "b.txt", "2"); + Commit c2 = createCommit(git, admin.getIdent(), "subject: 2"); + pushHead(git, "refs/for/master", false); + PatchSet.Id c1ps1 = getPatchSetId(c1); + PatchSet.Id c2ps1 = getPatchSetId(c2); + + // Swap the order of commits and push again. + git.reset().setMode(ResetType.HARD).setRef("HEAD^^").call(); + git.cherryPick().include(c2.getCommit()).include(c1.getCommit()).call(); + pushHead(git, "refs/for/master", false); + PatchSet.Id c1ps2 = getPatchSetId(c1); + PatchSet.Id c2ps2 = getPatchSetId(c2); + + for (PatchSet.Id ps : ImmutableList.of(c2ps2, c1ps2)) { + List related = getRelated(ps); + assertEquals(2, related.size()); + assertEquals("related to " + ps, c1.getChangeId(), related.get(0).changeId); + assertEquals("related to " + ps, c2.getChangeId(), related.get(1).changeId); + } + + for (PatchSet.Id ps : ImmutableList.of(c2ps1, c1ps1)) { + List related = getRelated(ps); + assertEquals(2, related.size()); + assertEquals("related to " + ps, c2.getChangeId(), related.get(0).changeId); + assertEquals("related to " + ps, c1.getChangeId(), related.get(1).changeId); + } + } + + @Test + public void getRelatedReorderAndExtend() throws GitAPIException, + IOException, Exception { + // Create two commits and push. + add(git, "a.txt", "1"); + Commit c1 = createCommit(git, admin.getIdent(), "subject: 1"); + add(git, "b.txt", "2"); + Commit c2 = createCommit(git, admin.getIdent(), "subject: 2"); + pushHead(git, "refs/for/master", false); + PatchSet.Id c1ps1 = getPatchSetId(c1); + PatchSet.Id c2ps1 = getPatchSetId(c2); + + // Swap the order of commits, create a new commit on top, and push again. + git.reset().setMode(ResetType.HARD).setRef("HEAD^^").call(); + git.cherryPick().include(c2.getCommit()).include(c1.getCommit()).call(); + add(git, "c.txt", "3"); + Commit c3 = createCommit(git, admin.getIdent(), "subject: 3"); + pushHead(git, "refs/for/master", false); + PatchSet.Id c1ps2 = getPatchSetId(c1); + PatchSet.Id c2ps2 = getPatchSetId(c2); + PatchSet.Id c3ps1 = getPatchSetId(c3); + + + for (PatchSet.Id ps : ImmutableList.of(c3ps1, c2ps2, c1ps2)) { + List related = getRelated(ps); + assertEquals(3, related.size()); + assertEquals("related to " + ps, c3.getChangeId(), related.get(0).changeId); + assertEquals("related to " + ps, c1.getChangeId(), related.get(1).changeId); + assertEquals("related to " + ps, c2.getChangeId(), related.get(2).changeId); + } + + for (PatchSet.Id ps : ImmutableList.of(c2ps1, c1ps1)) { + List related = getRelated(ps); + assertEquals(3, related.size()); + assertEquals("related to " + ps, c3.getChangeId(), related.get(0).changeId); + assertEquals("related to " + ps, c2.getChangeId(), related.get(1).changeId); + assertEquals("related to " + ps, c1.getChangeId(), related.get(2).changeId); + } + } + + private List getRelated(PatchSet.Id ps) throws IOException { + String url = String.format("/changes/%d/revisions/%d/related", + ps.getParentKey().get(), ps.get()); + RelatedInfo related = OutputFormat.JSON_COMPACT.newGson().fromJson( + session.get(url).getReader(), + new TypeToken() {}.getType()); + return related.changes; + } + + private PatchSet.Id getPatchSetId(Commit c) throws OrmException { + return Iterables.getOnlyElement( + db.changes().byKey(new Change.Key(c.getChangeId()))).currentPatchSetId(); + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/RelatedInfo.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/RelatedInfo.java new file mode 100644 index 0000000000..4089910103 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/RelatedInfo.java @@ -0,0 +1,21 @@ +// Copyright (C) 2013 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.server.change; + +import java.util.List; + +public class RelatedInfo { + public List changes; +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 5a3ab3b79e..f57f2d2cd1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -974,7 +974,7 @@ public class ChangeJson { int tz; } - static class CommitInfo { + public static class CommitInfo { final String kind = "gerritcodereview#commit"; String commit; List parents; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java index 87c238ccf6..b8441f3eb6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java @@ -92,7 +92,6 @@ public class GetRelated implements RestReadView { throws OrmException, IOException { Map changes = allOpenChanges(rsrc); Map patchSets = allPatchSets(changes.keySet()); - List list = children(rsrc, rw, changes, patchSets); Map commits = Maps.newHashMap(); for (PatchSet p : patchSets.values()) { @@ -112,11 +111,19 @@ public class GetRelated implements RestReadView { } } + Set added = Sets.newHashSet(); + List parents = Lists.newArrayList(); for (RevCommit c; (c = rw.next()) != null;) { PatchSet p = commits.get(c.name()); - Change g = p != null ? changes.get(p.getId().getParentKey()) : null; - list.add(new ChangeAndCommit(g, p, c)); + Change g = null; + if (p != null) { + g = changes.get(p.getId().getParentKey()); + added.add(p.getId().getParentKey()); + } + parents.add(new ChangeAndCommit(g, p, c)); } + List list = children(rsrc, rw, changes, patchSets, added); + list.addAll(parents); if (list.size() == 1) { ChangeAndCommit r = list.get(0); @@ -155,7 +162,8 @@ public class GetRelated implements RestReadView { } private List children(RevisionResource rsrc, RevWalk rw, - Map changes, Map patchSets) + Map changes, Map patchSets, + Set added) throws OrmException, IOException { // children is a map of parent commit name to PatchSet built on it. Multimap children = allChildren(changes.keySet()); @@ -192,7 +200,9 @@ public class GetRelated implements RestReadView { if (!c.has(seenCommit)) { c.add(seenCommit); q.addFirst(ps.getRevision().get()); - graph.add(new ChangeAndCommit(change, ps, c)); + if (added.add(ps.getId().getParentKey())) { + graph.add(new ChangeAndCommit(change, ps, c)); + } } } }