PatchSet: Disallow null commitId

This field has been nullable since the very beginning (I74ef9f34), but
honestly I have no idea why. Certainly the vast majority of callers
today assume that it's not null.

Force callers to provide a non-null commitId at construction time, and
disallow mutating it later. There was one stubborn caller in
PatchScriptFactory which I couldn't get rid of without disentangling the
logic of the whole class, so I punted and left a deprecated ugly named
static factory method just for this one case.

Change-Id: If8253781c101cbe4ed18210ec9ebe78dee8f4add

java/com/google/gerrit/reviewdb/client/PatchSet.java

@@ -15,18 +15,17 @@
 package com.google.gerrit.reviewdb.client;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static java.util.Objects.requireNonNull;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Splitter;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.common.Nullable;
-import com.google.gerrit.git.ObjectIds;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
-import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.ObjectId;
 
 /** A single revision of a {@link Change}. */
@@ -157,9 +156,24 @@ public final class PatchSet {
     }
   }
 
+  /**
+   * Create a patch set with no commit ID.
+   *
+   * <p>It is illegal to call {@link #getCommitId()} on the returned instance.
+   *
+   * @deprecated This method only exists to preserve behavior of one specific codepath in {@code
+   *     PatchScriptFactory}.
+   * @param id patch set ID.
+   * @return new patch set.
+   */
+  @Deprecated
+  public static PatchSet createWithNoCommitId(PatchSet.Id id) {
+    return new PatchSet(id);
+  }
+
   protected Id id;
 
-  @Nullable protected ObjectId commitId;
+  protected ObjectId commitId;
 
   protected Account.Id uploader;
 
@@ -192,8 +206,14 @@ public final class PatchSet {
 
   protected PatchSet() {}
 
-  public PatchSet(PatchSet.Id k) {
+  private PatchSet(PatchSet.Id id) {
-    id = k;
+    this.id = requireNonNull(id);
+    this.commitId = null;
+  }
+
+  public PatchSet(PatchSet.Id id, ObjectId commitId) {
+    this.id = requireNonNull(id);
+    this.commitId = commitId.copy();
   }
 
   public PatchSet(PatchSet src) {
@@ -219,16 +239,13 @@ public final class PatchSet {
    *
    * <p>The commit associated with a patch set is also known as the <strong>revision</strong>.
    *
-   * @return the commit ID.
+   * @return the commit ID, never null.
    */
   public ObjectId getCommitId() {
+    requireNonNull(commitId);
     return commitId;
   }
 
-  public void setCommitId(@Nullable AnyObjectId commitId) {
-    this.commitId = ObjectIds.copyOrNull(commitId);
-  }
-
   public Account.Id getUploader() {
     return uploader;
   }

java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.reviewdb.converter;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import com.google.gerrit.proto.Entities;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -36,10 +38,7 @@ public enum PatchSetProtoConverter implements ProtoConverter<Entities.PatchSet,
   public Entities.PatchSet toProto(PatchSet patchSet) {
     Entities.PatchSet.Builder builder =
         Entities.PatchSet.newBuilder().setId(patchSetIdConverter.toProto(patchSet.getId()));
-    ObjectId commitId = patchSet.getCommitId();
+    builder.setCommitId(objectIdConverter.toProto(patchSet.getCommitId()));
-    if (commitId != null) {
-      builder.setCommitId(objectIdConverter.toProto(commitId));
-    }
     Account.Id uploader = patchSet.getUploader();
     if (uploader != null) {
       builder.setUploaderAccountId(accountIdConverter.toProto(uploader));
@@ -65,10 +64,11 @@ public enum PatchSetProtoConverter implements ProtoConverter<Entities.PatchSet,
 
   @Override
   public PatchSet fromProto(Entities.PatchSet proto) {
-    PatchSet patchSet = new PatchSet(patchSetIdConverter.fromProto(proto.getId()));
+    checkArgument(proto.hasCommitId(), "missing commit_id: %s", proto);
-    if (proto.hasCommitId()) {
+    PatchSet patchSet =
-      patchSet.setCommitId(objectIdConverter.fromProto(proto.getCommitId()));
+        new PatchSet(
-    }
+            patchSetIdConverter.fromProto(proto.getId()),
+            objectIdConverter.fromProto(proto.getCommitId()));
     if (proto.hasUploaderAccountId()) {
       patchSet.setUploader(accountIdConverter.fromProto(proto.getUploaderAccountId()));
     }

java/com/google/gerrit/server/PatchSetUtil.java

@@ -98,8 +98,7 @@ public class PatchSetUtil {
     update.setPsDescription(description);
     update.setGroups(groups);
 
-    PatchSet ps = new PatchSet(psId);
+    PatchSet ps = new PatchSet(psId, commit);
-    ps.setCommitId(commit);
     ps.setUploader(update.getAccountId());
     ps.setCreatedOn(new Timestamp(update.getWhen().getTime()));
     ps.setGroups(groups);

java/com/google/gerrit/server/change/ConsistencyChecker.java

@@ -277,10 +277,6 @@ public class ConsistencyChecker {
       int psNum = ps.getId().get();
       String refName = ps.getId().toRefName();
       ObjectId objId = ps.getCommitId();
-      if (objId == null) {
-        problem("Null commitId on patch set " + psNum);
-        continue;
-      }
       patchSetsBySha.put(objId, ps);
 
       // Check ref existence.

java/com/google/gerrit/server/index/change/ChangeField.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.index.change;
 
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.gerrit.index.FieldDef.exact;
 import static com.google.gerrit.index.FieldDef.fullText;
 import static com.google.gerrit.index.FieldDef.intRange;
@@ -51,7 +52,6 @@ import com.google.gerrit.proto.Protos;
 import com.google.gerrit.reviewdb.client.Account;
 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.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
@@ -450,14 +450,8 @@ public class ChangeField {
   public static final FieldDef<ChangeData, Iterable<String>> EXACT_COMMIT =
       exact(ChangeQueryBuilder.FIELD_EXACTCOMMIT).buildRepeatable(ChangeField::getRevisions);
 
-  private static Set<String> getRevisions(ChangeData cd) {
+  private static ImmutableSet<String> getRevisions(ChangeData cd) {
-    Set<String> revisions = new HashSet<>();
+    return cd.patchSets().stream().map(ps -> ps.getCommitId().name()).collect(toImmutableSet());
-    for (PatchSet ps : cd.patchSets()) {
-      if (ps.getCommitId() != null) {
-        revisions.add(ps.getCommitId().name());
-      }
-    }
-    return revisions;
   }
 
   /** Tracking id extracted from a footer. */

java/com/google/gerrit/server/mail/send/ChangeEmail.java

@@ -204,7 +204,7 @@ public abstract class ChangeEmail extends NotificationEmail {
   }
 
   private void setCommitIdHeader() {
-    if (patchSet != null && patchSet.getCommitId() != null) {
+    if (patchSet != null) {
       setHeader(MailHeader.COMMIT.fieldName(), patchSet.getCommitId().name());
     }
   }

java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -506,9 +506,8 @@ class ChangeNotesParser {
               "Multiple revisions parsed for patch set %s: %s and %s",
               psId.get(), patchSets.get(psId).getCommitId().name(), rev.name()));
     }
-    PatchSet ps = new PatchSet(psId);
+    PatchSet ps = new PatchSet(psId, rev);
     patchSets.put(psId, ps);
-    ps.setCommitId(rev);
     ps.setUploader(accountId);
     ps.setCreatedOn(ts);
     PendingPatchSetFields pending = pendingPatchSets.remove(psId);

java/com/google/gerrit/server/patch/PatchListCacheImpl.java

@@ -135,9 +135,6 @@ public class PatchListCacheImpl implements PatchListCache {
       throws PatchListNotAvailableException {
     Project.NameKey project = change.getProject();
     ObjectId b = patchSet.getCommitId();
-    if (b == null) {
-      throw new PatchListNotAvailableException("commit ID is null for " + patchSet.getId());
-    }
     Whitespace ws = Whitespace.IGNORE_NONE;
     if (parentNum != null) {
       return get(PatchListKey.againstParentNum(parentNum, b, ws), project);

java/com/google/gerrit/server/patch/PatchScriptFactory.java

@@ -194,7 +194,13 @@ public class PatchScriptFactory implements Callable<PatchScript> {
     validatePatchSetId(psb);
 
     PatchSet psEntityA = psa != null ? psUtil.get(notes, psa) : null;
-    PatchSet psEntityB = psb.get() == 0 ? new PatchSet(psb) : psUtil.get(notes, psb);
+
+    // TODO(dborowitz): Shouldn't be creating a PatchSet with no commitId, but the logic depends on
+    // it somehow in a way that I don't follow, so old behavior is preserved for now.
+    @SuppressWarnings("deprecation")
+    PatchSet psEntityB =
+        psb.get() == 0 ? PatchSet.createWithNoCommitId(psb) : psUtil.get(notes, psb);
+
     if (psEntityA != null || psEntityB != null) {
       try {
         permissionBackend.currentUser().change(notes).check(ChangePermission.READ);
@@ -262,9 +268,6 @@ public class PatchScriptFactory implements Callable<PatchScript> {
     if (ps.getId().get() == 0) {
       return getEditRev();
     }
-    if (ps.getCommitId() == null) {
-      throw new NoSuchChangeException(changeId);
-    }
     return ps.getCommitId();
   }
 

java/com/google/gerrit/server/query/change/ChangeData.java

@@ -222,12 +222,12 @@ public class ChangeData {
    * @return instance for testing.
    */
   public static ChangeData createForTest(
-      Project.NameKey project, Change.Id id, int currentPatchSetId) {
+      Project.NameKey project, Change.Id id, int currentPatchSetId, ObjectId commitId) {
     ChangeData cd =
         new ChangeData(
             null, null, null, null, null, null, null, null, null, null, null, null, null, null,
             null, project, id, null, null);
-    cd.currentPatchSet = new PatchSet(PatchSet.id(id, currentPatchSetId));
+    cd.currentPatchSet = new PatchSet(PatchSet.id(id, currentPatchSetId), commitId);
     return cd;
   }
 

java/com/google/gerrit/server/query/change/CommitPredicate.java

@@ -46,8 +46,8 @@ public class CommitPredicate extends ChangeIndexPredicate {
 
   protected boolean equals(PatchSet p, String id) {
     boolean exact = getField() == EXACT_COMMIT;
-    String rev = p.getCommitId() != null ? p.getCommitId().name() : null;
+    String rev = p.getCommitId().name();
-    return (exact && id.equals(rev)) || (!exact && rev != null && rev.startsWith(id));
+    return (exact && id.equals(rev)) || (!exact && rev.startsWith(id));
   }
 
   @Override

java/com/google/gerrit/server/restapi/change/Revisions.java

@@ -153,9 +153,8 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
       throws AuthException, IOException {
     Optional<ChangeEdit> edit = editUtil.byChange(change.getNotes(), change.getUser());
     if (edit.isPresent()) {
-      PatchSet ps = new PatchSet(PatchSet.id(change.getId(), 0));
       ObjectId editCommitId = edit.get().getEditCommit();
-      ps.setCommitId(edit.get().getEditCommit());
+      PatchSet ps = new PatchSet(PatchSet.id(change.getId(), 0), editCommitId);
       if (commitId == null || editCommitId.equals(commitId)) {
         return Collections.singletonList(new RevisionResource(change, ps, edit));
       }

java/com/google/gerrit/server/submit/MergeOp.java

@@ -776,8 +776,8 @@ public class MergeOp implements AutoCloseable {
         commitStatus.logProblem(changeId, e);
         continue;
       }
-      if (ps == null || ps.getCommitId() == null) {
+      if (ps == null) {
-        commitStatus.logProblem(changeId, "Missing patch set or revision on change");
+        commitStatus.logProblem(changeId, "Missing patch set on change");
         continue;
       }
 

java/com/google/gerrit/testing/TestChanges.java

@@ -68,8 +68,7 @@ public class TestChanges {
   }
 
   public static PatchSet newPatchSet(PatchSet.Id id, String revision, Account.Id userId) {
-    PatchSet ps = new PatchSet(id);
+    PatchSet ps = new PatchSet(id, ObjectId.fromString(revision));
-    ps.setCommitId(ObjectId.fromString(revision));
     ps.setUploader(userId);
     ps.setCreatedOn(TimeUtil.nowTs());
     return ps;

javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java

@@ -32,6 +32,7 @@ import com.googlecode.prolog_cafe.lang.Term;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
+import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Test;
 
 public class PrologRuleEvaluatorIT extends AbstractDaemonTest {
@@ -149,7 +150,7 @@ public class PrologRuleEvaluatorIT extends AbstractDaemonTest {
   }
 
   private ChangeData makeChangeData() {
-    ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1);
+    ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId());
     cd.setChange(TestChanges.newChange(project, admin.id()));
     return cd;
   }

javatests/com/google/gerrit/reviewdb/converter/PatchSetProtoConverterTest.java

@@ -36,8 +36,10 @@ public class PatchSetProtoConverterTest {
 
   @Test
   public void allValuesConvertedToProto() {
-    PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73));
+    PatchSet patchSet =
-    patchSet.setCommitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+        new PatchSet(
+            PatchSet.id(Change.id(103), 73),
+            ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
     patchSet.setUploader(Account.id(452));
     patchSet.setCreatedOn(new Timestamp(930349320L));
     patchSet.setGroups(ImmutableList.of("group1, group2"));
@@ -65,7 +67,10 @@ public class PatchSetProtoConverterTest {
 
   @Test
   public void mandatoryValuesConvertedToProto() {
-    PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73));
+    PatchSet patchSet =
+        new PatchSet(
+            PatchSet.id(Change.id(103), 73),
+            ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
 
     Entities.PatchSet proto = patchSetProtoConverter.toProto(patchSet);
 
@@ -75,14 +80,18 @@ public class PatchSetProtoConverterTest {
                 Entities.PatchSet_Id.newBuilder()
                     .setChangeId(Entities.Change_Id.newBuilder().setId(103))
                     .setId(73))
+            .setCommitId(
+                Entities.ObjectId.newBuilder().setName("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
             .build();
     assertThat(proto).isEqualTo(expectedProto);
   }
 
   @Test
   public void allValuesConvertedToProtoAndBackAgain() {
-    PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73));
+    PatchSet patchSet =
-    patchSet.setCommitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+        new PatchSet(
+            PatchSet.id(Change.id(103), 73),
+            ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
     patchSet.setUploader(Account.id(452));
     patchSet.setCreatedOn(new Timestamp(930349320L));
     patchSet.setGroups(ImmutableList.of("group1, group2"));
@@ -96,7 +105,10 @@ public class PatchSetProtoConverterTest {
 
   @Test
   public void mandatoryValuesConvertedToProtoAndBackAgain() {
-    PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73));
+    PatchSet patchSet =
+        new PatchSet(
+            PatchSet.id(Change.id(103), 73),
+            ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
 
     PatchSet convertedPatchSet =
         patchSetProtoConverter.fromProto(patchSetProtoConverter.toProto(patchSet));

javatests/com/google/gerrit/server/change/WalkSorterTest.java

@@ -334,17 +334,15 @@ public class WalkSorterTest extends GerritBaseTests {
   private ChangeData newChange(TestRepository<Repo> tr, ObjectId id) throws Exception {
     Project.NameKey project = tr.getRepository().getDescription().getProject();
     Change c = TestChanges.newChange(project, userId);
-    ChangeData cd = ChangeData.createForTest(project, c.getId(), 1);
+    ChangeData cd = ChangeData.createForTest(project, c.getId(), 1, id);
     cd.setChange(c);
-    cd.currentPatchSet().setCommitId(id);
     cd.setPatchSets(ImmutableList.of(cd.currentPatchSet()));
     return cd;
   }
 
   private PatchSet addPatchSet(ChangeData cd, ObjectId id) throws Exception {
     TestChanges.incrementPatchSet(cd.change());
-    PatchSet ps = new PatchSet(cd.change().currentPatchSetId());
+    PatchSet ps = new PatchSet(cd.change().currentPatchSetId(), id);
-    ps.setCommitId(id);
     List<PatchSet> patchSets = new ArrayList<>(cd.patchSets());
     patchSets.add(ps);
     cd.setPatchSets(patchSets);

javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java

@@ -333,16 +333,18 @@ public class ChangeNotesStateTest extends GerritBaseTests {
 
   @Test
   public void serializePatchSets() throws Exception {
-    PatchSet ps1 = new PatchSet(PatchSet.id(ID, 1));
+    PatchSet ps1 =
+        new PatchSet(
+            PatchSet.id(ID, 1), ObjectId.fromString("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"));
     ps1.setUploader(Account.id(2000));
-    ps1.setCommitId(ObjectId.fromString("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"));
     ps1.setCreatedOn(cols.createdOn());
     ByteString ps1Bytes = toByteString(ps1, PatchSetProtoConverter.INSTANCE);
     assertThat(ps1Bytes.size()).isEqualTo(66);
 
-    PatchSet ps2 = new PatchSet(PatchSet.id(ID, 2));
+    PatchSet ps2 =
+        new PatchSet(
+            PatchSet.id(ID, 2), ObjectId.fromString("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"));
     ps2.setUploader(Account.id(3000));
-    ps2.setCommitId(ObjectId.fromString("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"));
     ps2.setCreatedOn(cols.lastUpdatedOn());
     ByteString ps2Bytes = toByteString(ps2, PatchSetProtoConverter.INSTANCE);
     assertThat(ps2Bytes.size()).isEqualTo(66);

javatests/com/google/gerrit/server/query/change/ChangeDataTest.java

@@ -23,18 +23,19 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.testing.GerritBaseTests;
 import com.google.gerrit.testing.TestChanges;
+import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Test;
 
 public class ChangeDataTest extends GerritBaseTests {
   @Test
   public void setPatchSetsClearsCurrentPatchSet() throws Exception {
     Project.NameKey project = Project.nameKey("project");
-    ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1);
+    ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId());
     cd.setChange(TestChanges.newChange(project, Account.id(1000)));
     PatchSet curr1 = cd.currentPatchSet();
     int currId = curr1.getId().get();
-    PatchSet ps1 = new PatchSet(PatchSet.id(cd.getId(), currId + 1));
+    PatchSet ps1 = new PatchSet(PatchSet.id(cd.getId(), currId + 1), ObjectId.zeroId());
-    PatchSet ps2 = new PatchSet(PatchSet.id(cd.getId(), currId + 2));
+    PatchSet ps2 = new PatchSet(PatchSet.id(cd.getId(), currId + 2), ObjectId.zeroId());
     cd.setPatchSets(ImmutableList.of(ps1, ps2));
     PatchSet curr2 = cd.currentPatchSet();
     assertThat(curr2).isNotSameInstanceAs(curr1);

javatests/com/google/gerrit/server/query/change/RegexPathPredicateTest.java

@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.testing.GerritBaseTests;
 import java.util.Arrays;
+import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Test;
 
 public class RegexPathPredicateTest extends GerritBaseTests {
@@ -83,7 +84,8 @@ public class RegexPathPredicateTest extends GerritBaseTests {
 
   private static ChangeData change(String... files) {
     Arrays.sort(files);
-    ChangeData cd = ChangeData.createForTest(Project.nameKey("project"), Change.id(1), 1);
+    ChangeData cd =
+        ChangeData.createForTest(Project.nameKey("project"), Change.id(1), 1, ObjectId.zeroId());
     cd.setCurrentFilePaths(Arrays.asList(files));
     return cd;
   }