Fix submittabilty of merge commits that resolve conflicts

Since I41c3c4 [1], problemsForSubmittingChanges() is called for the case
of a merge commit. Beforehand, it was only called for topics. That
change caused the submit button to be greyed out as soon as one change
in the ChangeSet was not mergeable. The issue with that logic was that
merge commits who resolved conflicts at the end of a chain of changes
were considered not mergeable.

We now check if the patch set is mergeable as a whole, taking into
account whether or not there is a resolving merge.

Tests are written according to the setup proposed in [2] and another
test case proposed in this change's comments [3].

[1] https://gerrit-review.googlesource.com/#/c/69745/14
[2] https://groups.google.com/forum/#!msg/repo-discuss/g1cpsRgs2bo/fnrt9vkcDwAJ
[3] https://gerrit-review.googlesource.com/#/c/74461/

Bug: Issue 3811
Change-Id: I30f2f8a8eb573135d98ad5c65ed5b63cb1db5f4b

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();

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");
+  }
+}

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>,
@@ -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 CLICK_FAILURE_OTHER_TOOLTIP;
+      }
     } 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();