ConsistencyChecker: Add a field to check expected merged SHA-1

When closing a change by push, the ref update to advance the branch
(ReceiveCommits.java:574) happens before closing the changes
(ReceiveCommits.java:614). This means in the event of some kinds of
database failures, a change might be merged into the destination
branch but never marked merged, *and* the actual commit SHA-1 that was
pushed might differ from any existing patch set.

Without doing a full walk of history, we have no easy way of
recovering from this situation only by inspecting the repo on the
server. The only way to detect a commit matching the change that
should have been inserted as a new patch set would be to inspect the
Change-Id footers, but we wouldn't know how far back in history to
walk. (This is additionally complicated by the fact that a commit with
the same Change-Id may have been merged in from a different branch.)

Fortunately, in the case we're talking about here, the end user should
have some record of the SHA-1 they're looking for, since they
previously ran "git push" on that SHA-1. So, this change provides a
new field in FixInput, expectMergedAs, to provide that SHA-1 and check
that it was actually the SHA-1 the change was merged as. In the event
that there is no patch set for the change at that SHA-1, one is
created.

Since the check would be potentially too expensive to do without
providing this SHA-1, it is only activated when this fix field is
provided.

Change-Id: Ie96633aae39d22694b2e6add072ce9ee82f1e30b

Documentation/rest-api-changes.txt

@@ -3891,6 +3891,9 @@ link:#fix-change[fix change] endpoint.
 |Field Name    |Description
 |`deletePatchSetIfCommitMissing`|If true, delete patch sets from the
 database if they refer to missing commit options.
+|`expectMergedAs`               |If set, check that the change is
+merged into the destination branch as this exact SHA-1. If not, insert
+a new patch set referring to this commit.
 |==========================
 
 [[git-person-info]]

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java

@@ -24,6 +24,7 @@ import com.google.common.base.Function;
 import com.google.common.collect.Lists;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.common.ProblemInfo;
 import com.google.gerrit.reviewdb.client.Account;
@@ -374,6 +375,198 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     assertProblems(c);
   }
 
+  @Test
+  public void expectedMergedCommitIsLatestPatchSet() throws Exception {
+    Change c = insertChange();
+    c.setStatus(Change.Status.MERGED);
+    PatchSet ps = insertPatchSet(c);
+    RevCommit commit = parseCommit(ps);
+    testRepo.update(c.getDest().get(), commit);
+
+    FixInput fix = new FixInput();
+    fix.expectMergedAs = commit.name();
+    assertThat(checker.check(c, fix).problems()).isEmpty();
+  }
+
+  @Test
+  public void expectedMergedCommitNotMergedIntoDestination() throws Exception {
+    Change c = insertChange();
+    c.setStatus(Change.Status.MERGED);
+    PatchSet ps = insertPatchSet(c);
+    RevCommit commit = parseCommit(ps);
+    testRepo.update(c.getDest().get(), commit);
+
+    FixInput fix = new FixInput();
+    RevCommit other =
+        testRepo.commit().message(commit.getFullMessage()).create();
+    fix.expectMergedAs = other.name();
+    List<ProblemInfo> problems = checker.check(c, fix).problems();
+    assertThat(problems).hasSize(1);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo(
+        "Expected merged commit " + other.name()
+        + " is not merged into destination ref refs/heads/master"
+        + " (" + commit.name() + ")");
+  }
+
+  @Test
+  public void createNewPatchSetForExpectedMergeCommitWithNoChangeId()
+      throws Exception {
+    Change c = insertChange();
+    c.setStatus(Change.Status.MERGED);
+    RevCommit parent =
+        testRepo.branch(c.getDest().get()).commit().message("parent").create();
+    PatchSet ps = insertPatchSet(c);
+    RevCommit commit = parseCommit(ps);
+
+    RevCommit mergedAs = testRepo.commit().parent(parent)
+        .message(commit.getShortMessage()).create();
+    testRepo.getRevWalk().parseBody(mergedAs);
+    assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)).isEmpty();
+    testRepo.update(c.getDest().get(), mergedAs);
+
+    assertProblems(c, "Patch set 1 (" + commit.name() + ") is not merged into"
+        + " destination ref refs/heads/master (" + mergedAs.name()
+        + "), but change status is MERGED");
+
+    FixInput fix = new FixInput();
+    fix.expectMergedAs = mergedAs.name();
+    List<ProblemInfo> problems = checker.check(c, fix).problems();
+    assertThat(problems).hasSize(1);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo(
+        "No patch set found for merged commit " + mergedAs.name());
+    assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED);
+    assertThat(p.outcome).isEqualTo("Inserted as patch set 2");
+
+    c = db.changes().get(c.getId());
+    PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2);
+    assertThat(c.currentPatchSetId()).isEqualTo(psId2);
+    assertThat(db.patchSets().get(psId2).getRevision().get())
+        .isEqualTo(mergedAs.name());
+
+    assertProblems(c);
+  }
+
+  @Test
+  public void createNewPatchSetForExpectedMergeCommitWithChangeId()
+      throws Exception {
+    Change c = insertChange();
+    c.setStatus(Change.Status.MERGED);
+    RevCommit parent =
+        testRepo.branch(c.getDest().get()).commit().message("parent").create();
+    PatchSet ps = insertPatchSet(c);
+    RevCommit commit = parseCommit(ps);
+
+    RevCommit mergedAs = testRepo.commit().parent(parent)
+        .message(commit.getShortMessage() + "\n"
+            + "\n"
+            + "Change-Id: " + c.getKey().get() + "\n").create();
+    testRepo.getRevWalk().parseBody(mergedAs);
+    assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID))
+        .containsExactly(c.getKey().get());
+    testRepo.update(c.getDest().get(), mergedAs);
+
+    assertProblems(c, "Patch set 1 (" + commit.name() + ") is not merged into"
+        + " destination ref refs/heads/master (" + mergedAs.name()
+        + "), but change status is MERGED");
+
+    FixInput fix = new FixInput();
+    fix.expectMergedAs = mergedAs.name();
+    List<ProblemInfo> problems = checker.check(c, fix).problems();
+    assertThat(problems).hasSize(1);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo(
+        "No patch set found for merged commit " + mergedAs.name());
+    assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED);
+    assertThat(p.outcome).isEqualTo("Inserted as patch set 2");
+
+    c = db.changes().get(c.getId());
+    PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2);
+    assertThat(c.currentPatchSetId()).isEqualTo(psId2);
+    assertThat(db.patchSets().get(psId2).getRevision().get())
+        .isEqualTo(mergedAs.name());
+
+    assertProblems(c);
+  }
+
+  @Test
+  public void expectedMergedCommitIsOldPatchSetOfSameChange()
+      throws Exception {
+    Change c = insertChange();
+    c.setStatus(Change.Status.MERGED);
+    PatchSet ps1 = insertPatchSet(c);
+    String rev1 = ps1.getRevision().get();
+    incrementPatchSet(c);
+    PatchSet ps2 = insertPatchSet(c);
+    testRepo.branch(c.getDest().get()).update(parseCommit(ps1));
+
+    FixInput fix = new FixInput();
+    fix.expectMergedAs = rev1;
+    List<ProblemInfo> problems = checker.check(c, fix).problems();
+    assertThat(problems).hasSize(1);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo(
+        "Expected merged commit " + rev1 + " corresponds to patch set "
+        + ps1.getId() + ", which is not the current patch set " + ps2.getId());
+  }
+
+  @Test
+  public void expectedMergedCommitWithMismatchedChangeId() throws Exception {
+    Change c = insertChange();
+    c.setStatus(Change.Status.MERGED);
+    RevCommit parent =
+        testRepo.branch(c.getDest().get()).commit().message("parent").create();
+    PatchSet ps = insertPatchSet(c);
+    RevCommit commit = parseCommit(ps);
+
+    String badId = "I0000000000000000000000000000000000000000";
+    RevCommit mergedAs = testRepo.commit().parent(parent)
+        .message(commit.getShortMessage() + "\n"
+            + "\n"
+            + "Change-Id: " + badId + "\n")
+        .create();
+    testRepo.getRevWalk().parseBody(mergedAs);
+    testRepo.update(c.getDest().get(), mergedAs);
+
+    FixInput fix = new FixInput();
+    fix.expectMergedAs = mergedAs.name();
+    List<ProblemInfo> problems = checker.check(c, fix).problems();
+    assertThat(problems).hasSize(1);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo(
+        "Expected merged commit " + mergedAs.name() + " has Change-Id: "
+        + badId + ", but expected " + c.getKey().get());
+  }
+
+  @Test
+  public void expectedMergedCommitMatchesMultiplePatchSets()
+      throws Exception {
+    Change c1 = insertChange();
+    c1.setStatus(Change.Status.MERGED);
+    insertPatchSet(c1);
+
+    RevCommit commit = testRepo.branch(c1.getDest().get()).commit().create();
+    Change c2 = insertChange();
+    PatchSet ps2 = newPatchSet(c2.currentPatchSetId(), commit, adminId);
+    updatePatchSetRef(ps2);
+    db.patchSets().insert(singleton(ps2));
+
+    Change c3 = insertChange();
+    PatchSet ps3 = newPatchSet(c3.currentPatchSetId(), commit, adminId);
+    updatePatchSetRef(ps3);
+    db.patchSets().insert(singleton(ps3));
+
+    FixInput fix = new FixInput();
+    fix.expectMergedAs = commit.name();
+    List<ProblemInfo> problems = checker.check(c1, fix).problems();
+    assertThat(problems).hasSize(1);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo(
+        "Multiple patch sets for expected merged commit " + commit.name()
+        + ": [" + ps2 + ", " + ps3 + "]");
+  }
+
   private Change insertChange() throws Exception {
     Change c = newChange(project, adminId);
     db.changes().insert(singleton(c));
@@ -388,7 +581,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   private PatchSet insertPatchSet(Change c) throws Exception {
     db.changes().upsert(singleton(c));
     RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit()
-        .parent(tip).create();
+        .parent(tip).message("Change " + c.getId().get()).create();
     PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId);
     updatePatchSetRef(ps);
     db.patchSets().insert(singleton(ps));
@@ -413,6 +606,13 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
   }
 
+  private RevCommit parseCommit(PatchSet ps) throws Exception {
+    RevCommit commit = testRepo.getRevWalk()
+        .parseCommit(ObjectId.fromString(ps.getRevision().get()));
+    testRepo.getRevWalk().parseBody(commit);
+    return commit;
+  }
+
   private void assertProblems(Change c, String... expected) {
     assertThat(Lists.transform(checker.check(c).problems(),
           new Function<ProblemInfo, String>() {

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FixInput.java

@@ -16,4 +16,6 @@ package com.google.gerrit.extensions.api.changes;
 
 public class FixInput {
   public boolean deletePatchSetIfCommitMissing;
+
+  public String expectMergedAs;
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java

@@ -18,10 +18,14 @@ import static com.google.gerrit.server.ChangeUtil.PS_ID_ORDER;
 import static com.google.gerrit.server.ChangeUtil.TO_PS_ID;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.base.Predicate;
 import com.google.common.collect.Collections2;
+import com.google.common.collect.FluentIterable;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
+import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.common.ProblemInfo;
@@ -29,13 +33,19 @@ import com.google.gerrit.extensions.common.ProblemInfo.Status;
 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.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.AtomicUpdate;
 import com.google.gwtorm.server.OrmException;
@@ -61,6 +71,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 /**
  * Checks changes for various kinds of inconsistency and corruption.
@@ -94,7 +105,9 @@ public class ConsistencyChecker {
   private final GitRepositoryManager repoManager;
   private final Provider<CurrentUser> user;
   private final Provider<PersonIdent> serverIdent;
+  private final ChangeControl.GenericFactory changeControlFactory;
   private final PatchSetInfoFactory patchSetInfoFactory;
+  private final PatchSetInserter.Factory patchSetInserterFactory;
 
   private FixInput fix;
   private Change change;
@@ -113,12 +126,16 @@ public class ConsistencyChecker {
       GitRepositoryManager repoManager,
       Provider<CurrentUser> user,
       @GerritPersonIdent Provider<PersonIdent> serverIdent,
-      PatchSetInfoFactory patchSetInfoFactory) {
+      ChangeControl.GenericFactory changeControlFactory,
+      PatchSetInfoFactory patchSetInfoFactory,
+      PatchSetInserter.Factory patchSetInserterFactory) {
     this.db = db;
     this.repoManager = repoManager;
     this.user = user;
     this.serverIdent = serverIdent;
+    this.changeControlFactory = changeControlFactory;
     this.patchSetInfoFactory = patchSetInfoFactory;
+    this.patchSetInserterFactory = patchSetInserterFactory;
     reset();
   }
 
@@ -303,15 +320,20 @@ public class ConsistencyChecker {
     if (tip == null) {
       return;
     }
-    boolean merged;
-    try {
-      merged = rw.isMergedInto(currPsCommit, tip);
-    } catch (IOException e) {
-      problem("Error checking whether patch set " + currPs.getId().get()
-          + " is merged");
-      return;
+
+    if (fix != null && fix.expectMergedAs != null) {
+      checkExpectMergedAs();
+    } else {
+      boolean merged;
+      try {
+        merged = rw.isMergedInto(currPsCommit, tip);
+      } catch (IOException e) {
+        problem("Error checking whether patch set " + currPs.getId().get()
+            + " is merged");
+        return;
+      }
+      checkMergedBitMatchesStatus(currPs, currPsCommit, merged);
     }
-    checkMergedBitMatchesStatus(currPs, currPsCommit, merged);
   }
 
   private void checkMergedBitMatchesStatus(PatchSet ps, RevCommit commit,
@@ -333,6 +355,120 @@ public class ConsistencyChecker {
     }
   }
 
+  private void checkExpectMergedAs() {
+    ObjectId objId =
+        parseObjectId(fix.expectMergedAs, "expected merged commit");
+    RevCommit commit = parseCommit(objId, "expected merged commit");
+    if (commit == null) {
+      return;
+    }
+    if (Objects.equals(commit, currPsCommit)) {
+      // Caller gave us latest patch set SHA-1; verified in checkPatchSets.
+      return;
+    }
+
+    try {
+      if (!rw.isMergedInto(commit, tip)) {
+        problem(String.format("Expected merged commit %s is not merged into"
+              + " destination ref %s (%s)",
+              commit.name(), change.getDest().get(), tip.name()));
+        return;
+      }
+
+      RevId revId = new RevId(commit.name());
+      List<PatchSet> patchSets = FluentIterable
+          .from(db.get().patchSets().byRevision(revId))
+          .filter(new Predicate<PatchSet>() {
+            @Override
+            public boolean apply(PatchSet ps) {
+              try {
+                Change c = db.get().changes().get(ps.getId().getParentKey());
+                return c != null && c.getDest().equals(change.getDest());
+              } catch (OrmException e) {
+                warn(e);
+                return true; // Should cause an error below, that's good.
+              }
+            }
+          }).toSortedList(ChangeUtil.PS_ID_ORDER);
+      switch (patchSets.size()) {
+        case 0:
+          // No patch set for this commit; insert one.
+          rw.parseBody(commit);
+          String changeId = Iterables.getFirst(
+              commit.getFooterLines(FooterConstants.CHANGE_ID), null);
+          // Missing Change-Id footer is ok, but mismatched is not.
+          if (changeId != null && !changeId.equals(change.getKey().get())) {
+            problem(String.format("Expected merged commit %s has Change-Id: %s,"
+                  + " but expected %s",
+                  commit.name(), changeId, change.getKey().get()));
+            return;
+          }
+          PatchSet ps = insertPatchSet(commit);
+          if (ps != null) {
+            checkMergedBitMatchesStatus(ps, commit, true);
+          }
+          break;
+
+        case 1:
+          // Existing patch set of this commit; check that it is the current
+          // patch set.
+          // TODO(dborowitz): This could be fixed if it's an older patch set of
+          // the current change.
+          PatchSet.Id id = patchSets.get(0).getId();
+          if (!id.equals(change.currentPatchSetId())) {
+            problem(String.format("Expected merged commit %s corresponds to"
+                  + " patch set %s, which is not the current patch set %s",
+                  commit.name(), id, change.currentPatchSetId()));
+          }
+          break;
+
+        default:
+          problem(String.format(
+                "Multiple patch sets for expected merged commit %s: %s",
+                commit.name(), patchSets));
+          break;
+      }
+    } catch (OrmException | IOException e) {
+      error("Error looking up expected merged commit " + fix.expectMergedAs,
+          e);
+    }
+  }
+
+  private PatchSet insertPatchSet(RevCommit commit) {
+    ProblemInfo p =
+        problem("No patch set found for merged commit " + commit.name());
+    if (!user.get().isIdentifiedUser()) {
+      p.status = Status.FIX_FAILED;
+      p.outcome =
+          "Must be called by an identified user to insert new patch set";
+      return null;
+    }
+
+    try {
+      ChangeControl ctl = changeControlFactory.controlFor(change, user.get());
+      PatchSetInserter inserter =
+          patchSetInserterFactory.create(repo, rw, ctl, commit);
+      change = inserter.setValidatePolicy(ValidatePolicy.NONE)
+          .setRunHooks(false)
+          .setSendMail(false)
+          .setAllowClosed(true)
+          .setUploader(((IdentifiedUser) user.get()).getAccountId())
+          // TODO: fix setMessage to work without init()
+          .setMessage(
+              "Patch set for merged commit inserted by consistency checker")
+          .insert();
+      p.status = Status.FIXED;
+      p.outcome = "Inserted as patch set " + change.currentPatchSetId().get();
+      return inserter.getPatchSet();
+    } catch (InvalidChangeOperationException | OrmException | IOException
+        | NoSuchChangeException e) {
+      warn(e);
+      p.status = Status.FIX_FAILED;
+      p.outcome = "Error inserting new patch set";
+      return null;
+    }
+  }
+
   private void fixMerged(ProblemInfo p) {
     try {
       change = db.get().changes().atomicUpdate(change.getId(),
@@ -486,7 +622,11 @@ public class ConsistencyChecker {
   private boolean error(String msg, Throwable t) {
     problem(msg);
     // TODO(dborowitz): Expose stack trace to administrators.
-    log.warn("Error in consistency check of change " + change.getId(), t);
+    warn(t);
     return false;
   }
+
+  private void warn(Throwable t) {
+    log.warn("Error in consistency check of change " + change.getId(), t);
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.change;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.collect.SetMultimap;
 import com.google.gerrit.common.ChangeHooks;
@@ -113,6 +114,7 @@ public class PatchSetInserter {
   private boolean runHooks;
   private boolean sendMail;
   private Account.Id uploader;
+  private boolean allowClosed;
 
   @Inject
   public PatchSetInserter(ChangeHooks hooks,
@@ -173,7 +175,15 @@ public class PatchSetInserter {
     return patchSet.getId();
   }
 
-  public PatchSetInserter setMessage(String message) throws OrmException {
+  public PatchSet getPatchSet() {
+    checkState(patchSet != null,
+        "getPatchSet() only valid after patch set is created");
+    return patchSet;
+  }
+
+  public PatchSetInserter setMessage(String message)
+      throws OrmException, IOException {
+    init();
     changeMessage = new ChangeMessage(
         new ChangeMessage.Key(
             ctl.getChange().getId(), ChangeUtil.messageUUID(db)),
@@ -217,6 +227,11 @@ public class PatchSetInserter {
     return this;
   }
 
+  public PatchSetInserter setAllowClosed(boolean allowClosed) {
+    this.allowClosed = allowClosed;
+    return this;
+  }
+
   public PatchSetInserter setUploader(Account.Id uploader) {
     this.uploader = uploader;
     return this;
@@ -247,7 +262,7 @@ public class PatchSetInserter {
     db.changes().beginTransaction(c.getId());
     try {
       updatedChange = db.changes().get(c.getId());
-      if (!updatedChange.getStatus().isOpen()) {
+      if (!updatedChange.getStatus().isOpen() && !allowClosed) {
         throw new InvalidChangeOperationException(String.format(
             "Change %s is closed", c.getId()));
       }
@@ -268,13 +283,13 @@ public class PatchSetInserter {
           db.changes().atomicUpdate(c.getId(), new AtomicUpdate<Change>() {
             @Override
             public Change update(Change change) {
-              if (change.getStatus().isClosed()) {
+              if (change.getStatus().isClosed() && !allowClosed) {
                 return null;
               }
               if (!change.currentPatchSetId().equals(currentPatchSetId)) {
                 return null;
               }
-              if (change.getStatus() != Change.Status.DRAFT) {
+              if (change.getStatus() != Change.Status.DRAFT && !allowClosed) {
                 change.setStatus(Change.Status.NEW);
               }
               change.setCurrentPatchSet(patchSetInfoFactory.get(commit,