ReceiveCommits: Don't advertise nonexistent change SHA-1s

Change-Id: I10a62163dfbbd409f4db53533c292d50ffddd724

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java

@@ -35,20 +35,25 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.edit.ChangeEditModifier;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.git.ReceiveCommitsAdvertiseRefsHook;
 import com.google.gerrit.server.git.SearchingChangeCacheImpl;
 import com.google.gerrit.server.git.TagCache;
 import com.google.gerrit.server.git.VisibleRefFilter;
+import com.google.gerrit.server.notedb.ChangeNoteUtil;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.project.Util;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.testutil.DisabledReviewDb;
+import com.google.gerrit.testutil.TestChanges;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
@@ -56,6 +61,7 @@ import org.junit.Before;
 import org.junit.Test;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
@@ -77,6 +83,13 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
   @Inject
   private Provider<CurrentUser> userProvider;
 
+  @Inject
+  private ChangeNoteUtil noteUtil;
+
+  @Inject
+  @AnonymousCowardName
+  private String anonymousCowardName;
+
   private AccountGroup.UUID admins;
 
   private ChangeData c1;
@@ -432,6 +445,48 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
         .containsExactly(obj(c3, 2), obj(c4, 1));
   }
 
+  @Test
+  public void receivePackOmitsMissingObject() throws Exception {
+    // Use the tactic from ConsistencyCheckerIT to insert a new patch set with a
+    // missing object.
+    String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
+    try (Repository repo = repoManager.openRepository(project)) {
+      TestRepository<?> tr = new TestRepository<>(repo);
+      String subject = "Subject for missing commit";
+      Change c = new Change(c3.change());
+      PatchSet.Id psId = new PatchSet.Id(c3.getId(), 2);
+      c.setCurrentPatchSet(psId, subject, c.getOriginalSubject());
+
+      PatchSet ps = TestChanges.newPatchSet(psId, rev, admin.getId());
+      db.patchSets().insert(Collections.singleton(ps));
+      db.changes().update(Collections.singleton(c));
+
+      if (notesMigration.commitChangeWrites()) {
+        PersonIdent committer = serverIdent.get();
+        PersonIdent author = noteUtil.newIdent(
+            accountCache.get(admin.getId()).getAccount(),
+            committer.getWhen(),
+            committer,
+            anonymousCowardName);
+        tr.branch(RefNames.changeMetaRef(c3.getId()))
+            .commit()
+            .author(author)
+            .committer(committer)
+            .message(
+                "Update patch set " + psId.get() + "\n"
+                    + "\n"
+                    + "Patch-set: " + psId.get() + "\n"
+                    + "Commit: " + rev + "\n"
+                    + "Subject: " + subject + "\n")
+            .create();
+      }
+      indexer.index(db, c.getProject(), c.getId());
+    }
+
+    assertThat(getReceivePackRefs().additionalHaves())
+        .containsExactly(obj(c4, 1));
+  }
+
   /**
    * Assert that refs seen by a non-admin user match expected.
    *

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsAdvertiseRefsHook.java

@@ -95,17 +95,21 @@ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook {
   @VisibleForTesting
   public Result advertiseRefs(Map<String, Ref> oldRefs) {
     Map<String, Ref> r = Maps.newHashMapWithExpectedSize(oldRefs.size());
+    Set<ObjectId> allPatchSets = Sets.newHashSetWithExpectedSize(oldRefs.size());
     for (Map.Entry<String, Ref> e : oldRefs.entrySet()) {
       String name = e.getKey();
       if (!skip(name)) {
         r.put(name, e.getValue());
       }
+      if (name.startsWith(RefNames.REFS_CHANGES)) {
+        allPatchSets.add(e.getValue().getObjectId());
+      }
     }
     return new AutoValue_ReceiveCommitsAdvertiseRefsHook_Result(
-        r, advertiseOpenChanges());
+        r, advertiseOpenChanges(allPatchSets));
   }
 
-  private Set<ObjectId> advertiseOpenChanges() {
+  private Set<ObjectId> advertiseOpenChanges(Set<ObjectId> allPatchSets) {
     // Advertise some recent open changes, in case a commit is based on one.
     int limit = 32;
     try {
@@ -117,7 +121,13 @@ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook {
           .byProjectOpen(projectName)) {
         PatchSet ps = cd.currentPatchSet();
         if (ps != null) {
-          r.add(ObjectId.fromString(ps.getRevision().get()));
+          ObjectId id = ObjectId.fromString(ps.getRevision().get());
+          // Ensure we actually observed a patch set ref pointing to this
+          // object, in case the database is out of sync with the repo and the
+          // object doesn't actually exist.
+          if (allPatchSets.contains(id)) {
+            r.add(id);
+          }
         }
       }
       return r;