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
2013-12-12 09:30:31 -08:00
parent d367fe158e
commit e2b9221ad4
6 changed files with 281 additions and 6 deletions
--- 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
@@ -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',
+  ],
+)
--- 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
@@ -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;
+}
--- 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
@@ -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<ReviewDb> 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<ChangeAndCommit> 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<ChangeAndCommit> 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<ChangeAndCommit> 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<ChangeAndCommit> 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<ChangeAndCommit> 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<ChangeAndCommit> 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<ChangeAndCommit> 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<RelatedInfo>() {}.getType());
+    return related.changes;
+  }
+
+  private PatchSet.Id getPatchSetId(Commit c) throws OrmException {
+    return Iterables.getOnlyElement(
+        db.changes().byKey(new Change.Key(c.getChangeId()))).currentPatchSetId();
+  }
+}
--- 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
@@ -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<ChangeAndCommit> changes;
+}
--- 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<CommitInfo> parents;
--- 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<RevisionResource> {
      throws OrmException, IOException {
    Map<Change.Id, Change> changes = allOpenChanges(rsrc);
    Map<PatchSet.Id, PatchSet> patchSets = allPatchSets(changes.keySet());
-    List<ChangeAndCommit> list = children(rsrc, rw, changes, patchSets);

    Map<String, PatchSet> commits = Maps.newHashMap();
    for (PatchSet p : patchSets.values()) {
@@ -112,11 +111,19 @@ public class GetRelated implements RestReadView<RevisionResource> {
      }
    }

+    Set<Change.Id> added = Sets.newHashSet();
+    List<ChangeAndCommit> 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<ChangeAndCommit> 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<RevisionResource> {
  }

  private List<ChangeAndCommit> children(RevisionResource rsrc, RevWalk rw,
-      Map<Change.Id, Change> changes, Map<PatchSet.Id, PatchSet> patchSets)
+      Map<Change.Id, Change> changes, Map<PatchSet.Id, PatchSet> patchSets,
+      Set<Change.Id> added)
      throws OrmException, IOException {
    // children is a map of parent commit name to PatchSet built on it.
    Multimap<String, PatchSet.Id> children = allChildren(changes.keySet());
@@ -192,7 +200,9 @@ public class GetRelated implements RestReadView<RevisionResource> {
        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));
+          }
        }
      }
    }