Merge changes I25afc720,I30f2f8a8 into stable-2.12

* changes:
  Submit: Eliminate confusing error message
  Fix submittabilty of merge commits that resolve conflicts

4 files changed, 458 insertions, 18 deletions

diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
index 77c38c8f43..6dd26e9bbc 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -43,6 +43,8 @@ import org.eclipse.jgit.transport.PushResult;
 import org.eclipse.jgit.transport.RemoteRefUpdate;
 import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
 
+import java.util.List;
+
 public class PushOneCommit {
   public static final String SUBJECT = "test commit";
   public static final String FILE_NAME = "a.txt";
@@ -179,6 +181,13 @@ public class PushOneCommit {
       .committer(new PersonIdent(i, testRepo.getClock()));
   }
 
+  public void setParents(List<RevCommit> parents) throws Exception {
+    commitBuilder.noParents();
+    for (RevCommit p : parents) {
+      commitBuilder.parent(p);
+    }
+  }
+
   public Result to(String ref) throws Exception {
     commitBuilder.add(fileName, content);
     return execute(ref);
@@ -189,7 +198,7 @@ public class PushOneCommit {
     return execute(ref);
   }
 
-  private Result execute(String ref) throws Exception {
+  public Result execute(String ref) throws Exception {
     RevCommit c = commitBuilder.create();
     if (changeId == null) {
       changeId = GitUtil.getChangeId(testRepo, c).get();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
index d6c8dacb00..e5281b1568 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -103,7 +103,7 @@ public class ActionsIT extends AbstractDaemonTest {
       assertThat(info.label).isEqualTo("Submit whole topic");
       assertThat(info.method).isEqualTo("POST");
       assertThat(info.title).isEqualTo(
-          "Clicking the button would fail for other changes");
+          "See the \"Submitted Together\" tab for problems");
     } else {
       noSubmitWholeTopicAssertions(actions, 1);
     }
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java
new file mode 100644
index 0000000000..5569bbf690
--- /dev/null
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java
@@ -0,0 +1,361 @@
+// 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.acceptance.rest.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.change.Submit;
+import com.google.gerrit.server.git.ChangeSet;
+import com.google.gerrit.server.git.MergeSuperSet;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.testutil.ConfigSuite;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+@NoHttpd
+public class SubmitResolvingMergeCommitIT extends AbstractDaemonTest {
+  @Inject
+  private MergeSuperSet mergeSuperSet;
+
+  @Inject
+  private Submit submit;
+
+  @ConfigSuite.Default
+  public static Config submitWholeTopicEnabled() {
+    return submitWholeTopicEnabledConfig();
+  }
+
+  @Test
+  public void resolvingMergeCommitAtEndOfChain() throws Exception {
+    /*
+      A <- B <- C <------- D
+      ^                    ^
+      |                    |
+      E <- F <- G <- H <-- M*
+
+      G has a conflict with C and is resolved in M which is a merge
+      commit of H and D.
+    */
+
+    PushOneCommit.Result a = createChange("A");
+    PushOneCommit.Result b = createChange("B", "new.txt", "No conflict line",
+        ImmutableList.of(a.getCommit()));
+    PushOneCommit.Result c = createChange("C", ImmutableList.of(b.getCommit()));
+    PushOneCommit.Result d = createChange("D", ImmutableList.of(c.getCommit()));
+
+    PushOneCommit.Result e = createChange("E", ImmutableList.of(a.getCommit()));
+    PushOneCommit.Result f = createChange("F", ImmutableList.of(e.getCommit()));
+    PushOneCommit.Result g = createChange("G", "new.txt", "Conflicting line",
+        ImmutableList.of(f.getCommit()));
+    PushOneCommit.Result h = createChange("H", ImmutableList.of(g.getCommit()));
+
+    approve(a.getChangeId());
+    approve(b.getChangeId());
+    approve(c.getChangeId());
+    approve(d.getChangeId());
+    submit(d.getChangeId());
+
+    approve(e.getChangeId());
+    approve(f.getChangeId());
+    approve(g.getChangeId());
+    approve(h.getChangeId());
+
+    assertMergeable(e.getChange(), true);
+    assertMergeable(f.getChange(), true);
+    assertMergeable(g.getChange(), false);
+    assertMergeable(h.getChange(), false);
+
+    PushOneCommit.Result m = createChange("M", "new.txt", "Resolved conflict",
+        ImmutableList.of(d.getCommit(), h.getCommit()));
+    approve(m.getChangeId());
+
+    assertChangeSetMergeable(m.getChange(), true);
+
+    assertMergeable(m.getChange(), true);
+    submit(m.getChangeId());
+
+    assertMerged(e.getChangeId());
+    assertMerged(f.getChangeId());
+    assertMerged(g.getChangeId());
+    assertMerged(h.getChangeId());
+    assertMerged(m.getChangeId());
+  }
+
+  @Test
+  public void resolvingMergeCommitComingBeforeConflict() throws Exception {
+    /*
+      A <- B <- C <- D
+      ^    ^
+      |    |
+      E <- F* <- G
+
+      F is a merge commit of E and B and resolves any conflict.
+      However G is conflicting with C.
+    */
+
+    PushOneCommit.Result a = createChange("A");
+    PushOneCommit.Result b = createChange("B", "new.txt", "No conflict line",
+        ImmutableList.of(a.getCommit()));
+    PushOneCommit.Result c = createChange("C", "new.txt", "No conflict line #2",
+        ImmutableList.of(b.getCommit()));
+    PushOneCommit.Result d = createChange("D", ImmutableList.of(c.getCommit()));
+    PushOneCommit.Result e = createChange("E", "new.txt", "Conflicting line",
+        ImmutableList.of(a.getCommit()));
+    PushOneCommit.Result f = createChange("F", "new.txt", "Resolved conflict",
+        ImmutableList.of(b.getCommit(), e.getCommit()));
+    PushOneCommit.Result g = createChange("G", "new.txt", "Conflicting line #2",
+        ImmutableList.of(f.getCommit()));
+
+    assertMergeable(e.getChange(), true);
+
+    approve(a.getChangeId());
+    approve(b.getChangeId());
+    submit(b.getChangeId());
+
+    assertMergeable(e.getChange(), false);
+    assertMergeable(f.getChange(), true);
+    assertMergeable(g.getChange(), true);
+
+    approve(c.getChangeId());
+    approve(d.getChangeId());
+    submit(d.getChangeId());
+
+    approve(e.getChangeId());
+    approve(f.getChangeId());
+    approve(g.getChangeId());
+
+    assertMergeable(g.getChange(), false);
+    assertChangeSetMergeable(g.getChange(), false);
+  }
+
+  @Test
+  public void resolvingMergeCommitWithTopics() throws Exception {
+    /*
+      Project1:
+        A <- B <-- C <---
+        ^    ^          |
+        |    |          |
+        E <- F* <- G <- L*
+
+      G clashes with C, and F resolves the clashes between E and B.
+      Later, L resolves the clashes between C and G.
+
+      Project2:
+        H <- I
+        ^    ^
+        |    |
+        J <- K*
+
+      J clashes with I, and K resolves all problems.
+      G, K and L are in the same topic.
+    */
+    assume().that(isSubmitWholeTopicEnabled()).isTrue();
+
+    String project1Name = name("Project1");
+    String project2Name = name("Project2");
+    gApi.projects().create(project1Name);
+    gApi.projects().create(project2Name);
+    TestRepository<InMemoryRepository> project1 =
+        cloneProject(new Project.NameKey(project1Name));
+    TestRepository<InMemoryRepository> project2 =
+        cloneProject(new Project.NameKey(project2Name));
+
+    PushOneCommit.Result a = createChange(project1, "A");
+    PushOneCommit.Result b = createChange(project1, "B", "new.txt",
+        "No conflict line", ImmutableList.of(a.getCommit()));
+    PushOneCommit.Result c = createChange(project1, "C", "new.txt",
+        "No conflict line #2", ImmutableList.of(b.getCommit()));
+
+    approve(a.getChangeId());
+    approve(b.getChangeId());
+    approve(c.getChangeId());
+    submit(c.getChangeId());
+
+    PushOneCommit.Result e = createChange(project1, "E", "new.txt",
+        "Conflicting line", ImmutableList.of(a.getCommit()));
+    PushOneCommit.Result f = createChange(project1, "F", "new.txt",
+        "Resolved conflict", ImmutableList.of(b.getCommit(), e.getCommit()));
+    PushOneCommit.Result g = createChange(project1, "G", "new.txt",
+        "Conflicting line #2", ImmutableList.of(f.getCommit()),
+        "refs/for/master/" + name("topic1"));
+
+    PushOneCommit.Result h = createChange(project2, "H");
+    PushOneCommit.Result i = createChange(project2, "I", "new.txt",
+        "No conflict line", ImmutableList.of(h.getCommit()));
+    PushOneCommit.Result j = createChange(project2, "J", "new.txt",
+        "Conflicting line", ImmutableList.of(h.getCommit()));
+    PushOneCommit.Result k =
+        createChange(project2, "K", "new.txt", "Sadly conflicting topic-wise",
+            ImmutableList.of(i.getCommit(), j.getCommit()),
+            "refs/for/master/" + name("topic1"));
+
+    approve(h.getChangeId());
+    approve(i.getChangeId());
+    submit(i.getChangeId());
+
+    approve(e.getChangeId());
+    approve(f.getChangeId());
+    approve(g.getChangeId());
+    approve(j.getChangeId());
+    approve(k.getChangeId());
+
+    assertChangeSetMergeable(g.getChange(), false);
+    assertChangeSetMergeable(k.getChange(), false);
+
+    PushOneCommit.Result l =
+        createChange(project1, "L", "new.txt", "Resolving conflicts again",
+            ImmutableList.of(c.getCommit(), g.getCommit()),
+            "refs/for/master/" + name("topic1"));
+
+    approve(l.getChangeId());
+    assertChangeSetMergeable(l.getChange(), true);
+
+    submit(l.getChangeId());
+    assertMerged(c.getChangeId());
+    assertMerged(g.getChangeId());
+    assertMerged(k.getChangeId());
+  }
+
+  @Test
+  public void resolvingMergeCommitAtEndOfChainAndNotUpToDate() throws Exception {
+    /*
+        A <-- B
+         \
+          C  <- D
+           \   /
+             E
+
+        B is the target branch, and D should be merged with B, but one
+        of C conflicts with B
+    */
+
+    PushOneCommit.Result a = createChange("A");
+    PushOneCommit.Result b = createChange("B", "new.txt", "No conflict line",
+        ImmutableList.of(a.getCommit()));
+
+    approve(a.getChangeId());
+    approve(b.getChangeId());
+    submit(b.getChangeId());
+
+    PushOneCommit.Result c = createChange("C", "new.txt", "Create conflicts",
+        ImmutableList.of(a.getCommit()));
+    PushOneCommit.Result e = createChange("E", ImmutableList.of(c.getCommit()));
+    PushOneCommit.Result d = createChange("D", "new.txt", "Resolves conflicts",
+        ImmutableList.of(c.getCommit(), e.getCommit()));
+
+    approve(c.getChangeId());
+    approve(e.getChangeId());
+    approve(d.getChangeId());
+    assertMergeable(d.getChange(), false);
+    assertChangeSetMergeable(d.getChange(), false);
+  }
+
+  private void submit(String changeId) throws Exception {
+    gApi.changes()
+        .id(changeId)
+        .current()
+        .submit();
+  }
+
+  private void assertChangeSetMergeable(ChangeData change,
+      boolean expected) throws MissingObjectException,
+          IncorrectObjectTypeException, IOException, OrmException {
+    ChangeSet cs = mergeSuperSet.completeChangeSet(db, change.change());
+    assertThat(submit.isPatchSetMergeable(cs)).isEqualTo(expected);
+  }
+
+  private void assertMergeable(ChangeData change, boolean expected)
+      throws Exception {
+    change.setMergeable(null);
+    assertThat(change.isMergeable()).isEqualTo(expected);
+  }
+
+  private void assertMerged(String changeId) throws Exception {
+    assertThat(gApi
+        .changes()
+        .id(changeId)
+        .get()
+        .status).isEqualTo(ChangeStatus.MERGED);
+  }
+
+  private PushOneCommit.Result createChange(TestRepository<?> repo,
+      String subject, String fileName, String content, List<RevCommit> parents,
+      String ref) throws Exception {
+    PushOneCommit push = pushFactory.create(db, admin.getIdent(), repo,
+        subject, fileName, content);
+
+    if (!parents.isEmpty()) {
+      push.setParents(parents);
+    }
+
+    PushOneCommit.Result result;
+    if (fileName.isEmpty()) {
+      result = push.execute(ref);
+    } else {
+      result = push.to(ref);
+    }
+    result.assertOkStatus();
+    return result;
+  }
+
+  private PushOneCommit.Result createChange(TestRepository<?> repo,
+      String subject) throws Exception {
+    return createChange(repo, subject, "x", "x", new ArrayList<RevCommit>(),
+        "refs/for/master");
+  }
+
+  private PushOneCommit.Result createChange(TestRepository<?> repo,
+      String subject, String fileName, String content, List<RevCommit> parents)
+          throws Exception {
+    return createChange(repo, subject, fileName, content, parents,
+        "refs/for/master");
+  }
+
+  private PushOneCommit.Result createChange(String subject) throws Exception {
+    return createChange(testRepo, subject, "", "",
+        Collections.<RevCommit> emptyList(), "refs/for/master");
+  }
+
+  private PushOneCommit.Result createChange(String subject,
+      List<RevCommit> parents) throws Exception {
+    return createChange(testRepo, subject, "", "", parents, "refs/for/master");
+  }
+
+  private PushOneCommit.Result createChange(String subject, String fileName,
+      String content, List<RevCommit> parents) throws Exception {
+    return createChange(testRepo, subject, fileName, content, parents,
+        "refs/for/master");
+  }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
index 1a92ec0584..f8cb88963d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
@@ -19,6 +19,8 @@ import com.google.common.base.Predicate;
 import com.google.common.base.Strings;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Multimap;
+import com.google.common.collect.Sets;
 import com.google.gerrit.common.data.ParameterizedString;
 import com.google.gerrit.extensions.api.changes.SubmitInput;
 import com.google.gerrit.extensions.common.ChangeInfo;
@@ -27,9 +29,11 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.extensions.webui.UiAction;
+import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 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.ChangeMessagesUtil;
@@ -54,12 +58,18 @@ import com.google.inject.Singleton;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 @Singleton
 public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
@@ -81,8 +91,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
       "This change depends on other hidden changes which are not ready";
   private static final String CLICK_FAILURE_TOOLTIP =
       "Clicking the button would fail";
-  private static final String CLICK_FAILURE_OTHER_TOOLTIP =
-      "Clicking the button would fail for other changes";
+  private static final String CHANGES_NOT_MERGEABLE =
+      "See the \"Submitted Together\" tab for problems";
 
   public static class Output {
     transient Change change;
@@ -228,24 +238,18 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
         if (!changeControl.canSubmit()) {
           return BLOCKED_SUBMIT_TOOLTIP;
         }
-        // Recheck mergeability rather than using value stored in the index,
-        // which may be stale.
-        // TODO(dborowitz): This is ugly; consider providing a way to not read
-        // stored fields from the index in the first place.
-        c.setMergeable(null);
-        Boolean mergeable = c.isMergeable();
-        if (mergeable == null) {
-          log.error("Ephemeral error checking if change is submittable");
-          return CLICK_FAILURE_TOOLTIP;
-        }
-        if (!mergeable) {
-          return CLICK_FAILURE_OTHER_TOOLTIP;
-        }
         MergeOp.checkSubmitRule(c);
       }
+
+      Boolean csIsMergeable = isPatchSetMergeable(cs);
+      if (csIsMergeable == null) {
+        return CLICK_FAILURE_TOOLTIP;
+      } else if (!csIsMergeable) {
+        return CHANGES_NOT_MERGEABLE;
+      }
     } catch (ResourceConflictException e) {
       return BLOCKED_SUBMIT_TOOLTIP;
-    } catch (NoSuchChangeException | OrmException e) {
+    } catch (NoSuchChangeException | OrmException | IOException e) {
       log.error("Error checking if change is submittable", e);
       throw new OrmRuntimeException("Could not determine problems for the change", e);
     }
@@ -380,6 +384,72 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
     return change != null ? change.getStatus().name().toLowerCase() : "deleted";
   }
 
+  public Boolean isPatchSetMergeable(ChangeSet cs) throws OrmException, IOException {
+    Map<ChangeData, Boolean> mergeabilityMap = new HashMap<>();
+    for (ChangeData change : cs.changes()) {
+      mergeabilityMap.put(change, false);
+    }
+
+    Multimap<Branch.NameKey, ChangeData> cbb = cs.changesByBranch();
+    for (Branch.NameKey branch : cbb.keySet()) {
+      Collection<ChangeData> targetBranch = cbb.get(branch);
+      HashMap<Change.Id, RevCommit> commits =
+          findCommits(targetBranch, branch.getParentKey());
+
+      Set<ObjectId> allParents = Sets.newHashSetWithExpectedSize(cs.size());
+      for (RevCommit commit : commits.values()) {
+        for (RevCommit parent : commit.getParents()) {
+          allParents.add(parent.getId());
+        }
+      }
+
+      for (ChangeData change : targetBranch) {
+        RevCommit commit = commits.get(change.getId());
+        boolean isMergeCommit = commit.getParentCount() > 1;
+        boolean isLastInChain = !allParents.contains(commit.getId());
+
+        // Recheck mergeability rather than using value stored in the index,
+        // which may be stale.
+        // TODO(dborowitz): This is ugly; consider providing a way to not read
+        // stored fields from the index in the first place.
+        change.setMergeable(null);
+        Boolean mergeable = change.isMergeable();
+        if (mergeable == null) {
+          // Skip whole check, cannot determine if mergeable
+          return null;
+        }
+        mergeabilityMap.put(change, mergeable);
+
+        if (isLastInChain && isMergeCommit && mergeable) {
+          for (ChangeData c : targetBranch) {
+            mergeabilityMap.put(c, true);
+          }
+          break;
+        }
+      }
+    }
+    return !mergeabilityMap.values().contains(Boolean.FALSE);
+  }
+
+  private HashMap<Change.Id, RevCommit> findCommits(
+      Collection<ChangeData> changes, Project.NameKey project)
+          throws IOException, OrmException {
+    HashMap<Change.Id, RevCommit> commits = new HashMap<>();
+    if (!changes.isEmpty()) {
+      try (Repository repo = repoManager.openRepository(project);
+          RevWalk walk = new RevWalk(repo)) {
+        for (ChangeData change : changes) {
+          PatchSet patchSet = dbProvider.get().patchSets()
+              .get(change.change().currentPatchSetId());
+          String commitId = patchSet.getRevision().get();
+          RevCommit commit = walk.parseCommit(ObjectId.fromString(commitId));
+          commits.put(change.getId(), commit);
+        }
+      }
+    }
+    return commits;
+  }
+
   private RevisionResource onBehalfOf(RevisionResource rsrc, SubmitInput in)
       throws AuthException, UnprocessableEntityException, OrmException {
     ChangeControl caller = rsrc.getControl();