Use ObjectId instead of RevId in PatchSet, and remove RevId

RevId began its existence in I43a6d6e4:

  Introduce RevId as an abstraction around revision strings

  This may make it easier to replace Git with some other tool like
  Hg or SVN, but it also offers us a convenient way to abstract the
  data type and improve type safety within Gerrit's code tree.

There are no plans to support another VCS in Gerrit in the foreseeable
future; that would be a huge project. Even if we were to start, moving
away from ObjectId in the storage layer is just a drop in the ocean,
considering the heavy usage of ObjectId elsewhere in Gerrit (to say
nothing of other JGit APIs).

The arguments around encapsulating the type and improving compile-time
safety still apply, but ObjectId serves the same purposes--arguably
better, since ObjectId has useful and well-tested and -benchmarked
methods upstream in JGit.

There were other arguments for using RevId in the past: ObjectId
couldn't be used as a field type in ReviewDb, and isn't GWT-compatible.
These arguments no longer hold, since both ReviewDb and GWT are gone.

The specific implementation of RevId itself had some shortcomings:

 * It didn't validate that the underlying value was a valid hex SHA-1.
 * It allowed null as a value, although nearly all callers assumed it
   didn't.
 * The id field was non-final. (In practice it was not possible to
   reassign it without using either gwtorm libraries or direct
   reflection.)
 * There were no convenient methods for converting to/from ObjectId,
   a very frequent operation, largely because of the historical GWT
   incompatibility.

Of course, these problems are fixable, but why bother? ObjectId serves
the purpose, and this way contributors don't have the cognitive overhead
of deciding which of two similar types to use.

Change-Id: Iff5644e21c51a7a8c12a113fd5a6a6ffaf60ae20

java/com/google/gerrit/git/ObjectIds.java

@@ -108,6 +108,20 @@ public class ObjectIds {
     return id != null ? id.copy() : ObjectId.zeroId();
   }
 
+  /**
+   * Return whether the given ID matches the given abbreviation.
+   *
+   * @param id object ID.
+   * @param abbreviation abbreviated hex object ID. May not be null, but may be an invalid hex SHA-1
+   *     abbreviation string.
+   * @return true if {@code id} is not null and {@code abbreviation} is a valid hex SHA-1
+   *     abbreviation that matches {@code id}, false otherwise.
+   */
+  public static boolean matchesAbbreviation(@Nullable AnyObjectId id, String abbreviation) {
+    requireNonNull(abbreviation);
+    return id != null && id.name().startsWith(abbreviation);
+  }
+
   private static void checkValidLength(int n) {
     checkArgument(n > 0);
     checkArgument(n <= STR_LEN);

java/com/google/gerrit/httpd/raw/CatServlet.java

@@ -139,7 +139,7 @@ public class CatServlet extends HttpServlet {
           rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
           return;
         }
-        revision = patchSet.getRevision().get();
+        revision = patchSet.getCommitId().name();
       }
     } catch (ResourceConflictException | NoSuchChangeException | AuthException e) {
       rsp.sendError(HttpServletResponse.SC_NOT_FOUND);

java/com/google/gerrit/reviewdb/BUILD

@@ -8,6 +8,7 @@ java_library(
     deps = [
         "//java/com/google/gerrit/common:annotations",
         "//java/com/google/gerrit/extensions:api",
+        "//java/com/google/gerrit/git",
         "//lib:guava",
         "//lib:protobuf",
         "//lib/auto:auto-value",

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

@@ -20,11 +20,14 @@ 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}. */
 public final class PatchSet {
@@ -156,7 +159,7 @@ public final class PatchSet {
 
   protected Id id;
 
-  @Nullable protected RevId revision;
+  @Nullable protected ObjectId commitId;
 
   protected Account.Id uploader;
 
@@ -195,7 +198,7 @@ public final class PatchSet {
 
   public PatchSet(PatchSet src) {
     this.id = src.id;
-    this.revision = src.revision;
+    this.commitId = src.commitId;
     this.uploader = src.uploader;
     this.createdOn = src.createdOn;
     this.groups = src.groups;
@@ -211,12 +214,19 @@ public final class PatchSet {
     return id.get();
   }
 
-  public RevId getRevision() {
-    return revision;
+  /**
+   * Get the ID of the commit associated with this patch set.
+   *
+   * <p>The commit associated with a patch set is also known as the <strong>revision</strong>.
+   *
+   * @return the commit ID.
+   */
+  public ObjectId getCommitId() {
+    return commitId;
   }
 
-  public void setRevision(RevId i) {
-    revision = i;
+  public void setCommitId(@Nullable AnyObjectId commitId) {
+    this.commitId = ObjectIds.copyOrNull(commitId);
   }
 
   public Account.Id getUploader() {
@@ -276,7 +286,7 @@ public final class PatchSet {
     }
     PatchSet p = (PatchSet) o;
     return Objects.equals(id, p.id)
-        && Objects.equals(revision, p.revision)
+        && Objects.equals(commitId, p.commitId)
         && Objects.equals(uploader, p.uploader)
         && Objects.equals(createdOn, p.createdOn)
         && Objects.equals(groups, p.groups)
@@ -286,7 +296,7 @@ public final class PatchSet {
 
   @Override
   public int hashCode() {
-    return Objects.hash(id, revision, uploader, createdOn, groups, pushCertificate, description);
+    return Objects.hash(id, commitId, uploader, createdOn, groups, pushCertificate, description);
   }
 
   @Override

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

@@ -1,51 +0,0 @@
-// Copyright (C) 2008 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.reviewdb.client;
-
-/** A revision identifier for a file or a change. */
-public final class RevId {
-
-  protected String id;
-
-  protected RevId() {}
-
-  public RevId(String str) {
-    id = str;
-  }
-
-  /** @return the value of this revision id. */
-  public String get() {
-    return id;
-  }
-
-  @Override
-  public int hashCode() {
-    return id.hashCode();
-  }
-
-  @Override
-  public boolean equals(Object o) {
-    return (o instanceof RevId) && id.equals(((RevId) o).id);
-  }
-
-  @Override
-  public String toString() {
-    return getClass().getSimpleName() + "{" + id + "}";
-  }
-
-  public boolean matches(String str) {
-    return id.startsWith(str.toLowerCase());
-  }
-}

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

@@ -17,7 +17,6 @@ package com.google.gerrit.reviewdb.converter;
 import com.google.gerrit.proto.Entities;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.protobuf.Parser;
 import java.sql.Timestamp;
 import java.util.List;
@@ -37,9 +36,9 @@ 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()));
-    RevId revision = patchSet.getRevision();
-    if (revision != null) {
-      builder.setCommitId(objectIdConverter.toProto(ObjectId.fromString(revision.get())));
+    ObjectId commitId = patchSet.getCommitId();
+    if (commitId != null) {
+      builder.setCommitId(objectIdConverter.toProto(commitId));
     }
     Account.Id uploader = patchSet.getUploader();
     if (uploader != null) {
@@ -68,7 +67,7 @@ public enum PatchSetProtoConverter implements ProtoConverter<Entities.PatchSet,
   public PatchSet fromProto(Entities.PatchSet proto) {
     PatchSet patchSet = new PatchSet(patchSetIdConverter.fromProto(proto.getId()));
     if (proto.hasCommitId()) {
-      patchSet.setRevision(new RevId(objectIdConverter.fromProto(proto.getCommitId()).name()));
+      patchSet.setCommitId(objectIdConverter.fromProto(proto.getCommitId()));
     }
     if (proto.hasUploaderAccountId()) {
       patchSet.setUploader(accountIdConverter.fromProto(proto.getUploaderAccountId()));

java/com/google/gerrit/server/ApprovalCopier.java

@@ -41,7 +41,6 @@ import java.util.Collections;
 import java.util.List;
 import java.util.TreeMap;
 import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevWalk;
 
 /**
@@ -128,11 +127,7 @@ public class ApprovalCopier {
 
         ChangeKind kind =
             changeKindCache.getChangeKind(
-                project.getNameKey(),
-                rw,
-                repoConfig,
-                ObjectId.fromString(priorPs.getRevision().get()),
-                ObjectId.fromString(ps.getRevision().get()));
+                project.getNameKey(), rw, repoConfig, priorPs.getCommitId(), ps.getCommitId());
 
         for (PatchSetApproval psa : priorApprovals) {
           if (wontCopy.contains(psa.getLabel(), psa.getAccountId())) {

java/com/google/gerrit/server/CommentsUtil.java

@@ -50,7 +50,6 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 
@@ -321,7 +320,7 @@ public class CommentsUtil {
           c.setCommitId(cache.getOldId(change, ps, null));
         }
       } else {
-        c.setCommitId(ObjectId.fromString(ps.getRevision().get()));
+        c.setCommitId(ps.getCommitId());
       }
     }
   }

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

@@ -27,7 +27,6 @@ import com.google.gerrit.reviewdb.client.Change;
 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.RevId;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -100,7 +99,7 @@ public class PatchSetUtil {
     update.setGroups(groups);
 
     PatchSet ps = new PatchSet(psId);
-    ps.setRevision(new RevId(commit.name()));
+    ps.setCommitId(commit);
     ps.setUploader(update.getAccountId());
     ps.setCreatedOn(new Timestamp(update.getWhen().getTime()));
     ps.setGroups(groups);
@@ -169,7 +168,7 @@ public class PatchSetUtil {
   public RevCommit getRevCommit(Project.NameKey project, PatchSet patchSet) throws IOException {
     try (Repository repo = repoManager.openRepository(project);
         RevWalk rw = new RevWalk(repo)) {
-      RevCommit src = rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
+      RevCommit src = rw.parseCommit(patchSet.getCommitId());
       rw.parseBody(src);
       return src;
     }

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

@@ -386,11 +386,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
         if (priorPs != patch) {
           kind =
               cache.getChangeKind(
-                  change.project(),
-                  rw,
-                  repoConfig,
-                  ObjectId.fromString(priorPs.getRevision().get()),
-                  ObjectId.fromString(patch.getRevision().get()));
+                  change.project(), rw, repoConfig, priorPs.getCommitId(), patch.getCommitId());
         }
       } catch (StorageException e) {
         // Do nothing; assume we have a complex change

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

@@ -276,8 +276,9 @@ public class ConsistencyChecker {
       // Check revision format.
       int psNum = ps.getId().get();
       String refName = ps.getId().toRefName();
-      ObjectId objId = parseObjectId(ps.getRevision().get(), "patch set " + psNum);
+      ObjectId objId = ps.getCommitId();
       if (objId == null) {
+        problem("Null commitId on patch set " + psNum);
         continue;
       }
       patchSetsBySha.put(objId, ps);
@@ -395,7 +396,11 @@ public class ConsistencyChecker {
   }
 
   private void checkExpectMergedAs() {
-    ObjectId objId = parseObjectId(fix.expectMergedAs, "expected merged commit");
+    if (!ObjectId.isId(fix.expectMergedAs)) {
+      problem("Invalid revision on expected merged commit: " + fix.expectMergedAs);
+      return;
+    }
+    ObjectId objId = ObjectId.fromString(fix.expectMergedAs);
     RevCommit commit = parseCommit(objId, "expected merged commit");
     if (commit == null) {
       return;
@@ -602,7 +607,7 @@ public class ConsistencyChecker {
     try {
       RefUpdate ru = repo.updateRef(ps.getId().toRefName());
       ru.setForceUpdate(true);
-      ru.setNewObjectId(ObjectId.fromString(ps.getRevision().get()));
+      ru.setNewObjectId(ps.getCommitId());
       ru.setRefLogIdent(newRefLogIdent());
       ru.setRefLogMessage("Repair patch set ref", true);
       RefUpdate.Result result = ru.update();
@@ -733,15 +738,6 @@ public class ConsistencyChecker {
     return serverIdent.get();
   }
 
-  private ObjectId parseObjectId(String objIdStr, String desc) {
-    try {
-      return ObjectId.fromString(objIdStr);
-    } catch (IllegalArgumentException e) {
-      problem(String.format("Invalid revision on %s: %s", desc, objIdStr));
-      return null;
-    }
-  }
-
   private RevCommit parseCommit(ObjectId objId, String desc) {
     try {
       return rw.parseCommit(objId);

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

@@ -99,8 +99,8 @@ public class DeleteChangeOp implements BatchUpdateOp {
     }
 
     RevWalk revWalk = ctx.getRevWalk();
-    ObjectId objectId = ObjectId.fromString(patchSet.getRevision().get());
-    return revWalk.isMergedInto(revWalk.parseCommit(objectId), revWalk.parseCommit(destId.get()));
+    return revWalk.isMergedInto(
+        revWalk.parseCommit(patchSet.getCommitId()), revWalk.parseCommit(destId.get()));
   }
 
   private void cleanUpReferences(Change.Id id, Collection<PatchSet> patchSets) throws IOException {

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

@@ -21,7 +21,6 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Patch;
 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.server.patch.PatchList;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.patch.PatchListEntry;
@@ -44,27 +43,20 @@ public class FileInfoJson {
 
   public Map<String, FileInfo> toFileInfoMap(Change change, PatchSet patchSet)
       throws PatchListNotAvailableException {
-    return toFileInfoMap(change, patchSet.getRevision(), null);
-  }
-
-  public Map<String, FileInfo> toFileInfoMap(Change change, RevId revision, @Nullable PatchSet base)
-      throws PatchListNotAvailableException {
-    ObjectId objectId = ObjectId.fromString(revision.get());
-    return toFileInfoMap(change, objectId, base);
+    return toFileInfoMap(change, patchSet.getCommitId(), null);
   }
 
   public Map<String, FileInfo> toFileInfoMap(
       Change change, ObjectId objectId, @Nullable PatchSet base)
       throws PatchListNotAvailableException {
-    ObjectId a = (base == null) ? null : ObjectId.fromString(base.getRevision().get());
+    ObjectId a = base != null ? base.getCommitId() : null;
     return toFileInfoMap(change, PatchListKey.againstCommit(a, objectId, Whitespace.IGNORE_NONE));
   }
 
-  public Map<String, FileInfo> toFileInfoMap(Change change, RevId revision, int parent)
+  public Map<String, FileInfo> toFileInfoMap(Change change, ObjectId objectId, int parent)
       throws PatchListNotAvailableException {
-    ObjectId b = ObjectId.fromString(revision.get());
     return toFileInfoMap(
-        change, PatchListKey.againstParentNum(parent + 1, b, Whitespace.IGNORE_NONE));
+        change, PatchListKey.againstParentNum(parent + 1, objectId, Whitespace.IGNORE_NONE));
   }
 
   private Map<String, FileInfo> toFileInfoMap(Change change, PatchListKey key)

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

@@ -54,8 +54,6 @@ public class PureRevert {
     }
 
     return pureRevertCache.isPureRevert(
-        notes.getProjectName(),
-        ObjectId.fromString(notes.getCurrentPatchSet().getRevision().get()),
-        claimedOriginalObjectId);
+        notes.getProjectName(), notes.getCurrentPatchSet().getCommitId(), claimedOriginalObjectId);
   }
 }

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

@@ -20,7 +20,6 @@ import com.google.gerrit.extensions.restapi.MergeConflictException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
@@ -151,10 +150,8 @@ public class RebaseChangeOp implements BatchUpdateOp {
           NoSuchChangeException, PermissionBackendException {
     // Ok that originalPatchSet was not read in a transaction, since we just
     // need its revision.
-    RevId oldRev = originalPatchSet.getRevision();
-
     RevWalk rw = ctx.getRevWalk();
-    RevCommit original = rw.parseCommit(ObjectId.fromString(oldRev.get()));
+    RevCommit original = rw.parseCommit(originalPatchSet.getCommitId());
     rw.parseBody(original);
     RevCommit baseCommit = rw.parseCommit(baseCommitId);
     CurrentUser changeOwner = identifiedUserFactory.create(notes.getChange().getOwner());

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

@@ -21,10 +21,10 @@ import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.git.ObjectIds;
 import com.google.gerrit.reviewdb.client.BranchNameKey;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -108,7 +108,7 @@ public class RebaseUtil {
     Base ret = null;
     for (ChangeData cd : queryProvider.get().byProjectCommit(rsrc.getProject(), base)) {
       for (PatchSet ps : cd.patchSets()) {
-        if (!ps.getRevision().matches(base)) {
+        if (!ObjectIds.matchesAbbreviation(ps.getCommitId(), base)) {
           continue;
         }
         if (ret == null || ret.patchSet().getId().get() < ps.getId().get()) {
@@ -143,8 +143,8 @@ public class RebaseUtil {
   public ObjectId findBaseRevision(
       PatchSet patchSet, BranchNameKey destBranch, Repository git, RevWalk rw)
       throws RestApiException, IOException {
-    String baseRev = null;
-    RevCommit commit = rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
+    ObjectId baseId = null;
+    RevCommit commit = rw.parseCommit(patchSet.getCommitId());
 
     if (commit.getParentCount() > 1) {
       throw new UnprocessableEntityException("Cannot rebase a change with multiple parents.");
@@ -153,12 +153,12 @@ public class RebaseUtil {
           "Cannot rebase a change without any parents (is this the initial commit?).");
     }
 
-    RevId parentRev = new RevId(commit.getParent(0).name());
+    ObjectId parentId = commit.getParent(0);
 
     CHANGES:
-    for (ChangeData cd : queryProvider.get().byBranchCommit(destBranch, parentRev.get())) {
+    for (ChangeData cd : queryProvider.get().byBranchCommit(destBranch, parentId.name())) {
       for (PatchSet depPatchSet : cd.patchSets()) {
-        if (!depPatchSet.getRevision().equals(parentRev)) {
+        if (!depPatchSet.getCommitId().equals(parentId)) {
           continue;
         }
         Change depChange = cd.change();
@@ -172,13 +172,13 @@ public class RebaseUtil {
             throw new ResourceConflictException(
                 "Change is already based on the latest patch set of the dependent change.");
           }
-          baseRev = cd.currentPatchSet().getRevision().get();
+          baseId = cd.currentPatchSet().getCommitId();
         }
         break CHANGES;
       }
     }
 
-    if (baseRev == null) {
+    if (baseId == null) {
       // We are dependent on a merged PatchSet or have no PatchSet
       // dependencies at all.
       Ref destRef = git.getRefDatabase().exactRef(destBranch.branch());
@@ -186,11 +186,11 @@ public class RebaseUtil {
         throw new UnprocessableEntityException(
             "The destination branch does not exist: " + destBranch.branch());
       }
-      baseRev = destRef.getObjectId().getName();
-      if (baseRev.equals(parentRev.get())) {
+      baseId = destRef.getObjectId();
+      if (baseId.equals(parentId)) {
         throw new ResourceConflictException("Change is already up to date.");
       }
     }
-    return ObjectId.fromString(baseRev);
+    return baseId;
   }
 }

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

@@ -227,7 +227,7 @@ public class RevisionJson {
         }
         if (want) {
           res.put(
-              in.getRevision().get(),
+              in.getCommitId().name(),
               toRevisionInfo(accountLoader, cd, in, repo, rw, false, changeInfo));
         }
       }
@@ -290,7 +290,7 @@ public class RevisionJson {
       checkState(rw != null);
       checkState(repo != null);
       Project.NameKey project = c.getProject();
-      String rev = in.getRevision().get();
+      String rev = in.getCommitId().name();
       RevCommit commit = rw.parseCommit(ObjectId.fromString(rev));
       rw.parseBody(commit);
       if (setCommit) {

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

@@ -41,7 +41,6 @@ import java.util.Map;
 import java.util.Set;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
@@ -230,13 +229,12 @@ public class WalkSorter {
       if (maxPs == null) {
         continue; // No patch sets matched.
       }
-      ObjectId id = ObjectId.fromString(maxPs.getRevision().get());
       try {
-        RevCommit c = rw.parseCommit(id);
+        RevCommit c = rw.parseCommit(maxPs.getCommitId());
         byCommit.put(c, PatchSetData.create(cd, maxPs, c));
       } catch (MissingObjectException | IncorrectObjectTypeException e) {
         logger.atWarning().withCause(e).log(
-            "missing commit %s for patch set %s", id.name(), maxPs.getId());
+            "missing commit %s for patch set %s", maxPs.getCommitId().name(), maxPs.getId());
       }
     }
     return byCommit;

java/com/google/gerrit/server/edit/ChangeEditJson.java

@@ -51,7 +51,7 @@ public class ChangeEditJson {
   public EditInfo toEditInfo(ChangeEdit edit, boolean downloadCommands) {
     EditInfo out = new EditInfo();
     out.commit = fillCommit(edit.getEditCommit());
-    out.baseRevision = edit.getBasePatchSet().getRevision().get();
+    out.baseRevision = edit.getBasePatchSet().getCommitId().name();
     out.basePatchSetNumber = edit.getBasePatchSet().getPatchSetId();
     out.ref = edit.getRefName();
     if (downloadCommands) {

java/com/google/gerrit/server/edit/ChangeEditModifier.java

@@ -124,7 +124,7 @@ public class ChangeEditModifier {
     }
 
     PatchSet currentPatchSet = lookupCurrentPatchSet(notes);
-    ObjectId patchSetCommitId = getPatchSetCommitId(currentPatchSet);
+    ObjectId patchSetCommitId = currentPatchSet.getCommitId();
     createEdit(repository, notes, currentPatchSet, patchSetCommitId, TimeUtil.nowTs());
   }
 
@@ -456,7 +456,7 @@ public class ChangeEditModifier {
 
   private static RevCommit lookupCommit(Repository repository, PatchSet patchSet)
       throws IOException {
-    ObjectId patchSetCommitId = getPatchSetCommitId(patchSet);
+    ObjectId patchSetCommitId = patchSet.getCommitId();
     return lookupCommit(repository, patchSetCommitId);
   }
 
@@ -483,7 +483,7 @@ public class ChangeEditModifier {
   private static ObjectId merge(Repository repository, ChangeEdit changeEdit, ObjectId newTreeId)
       throws IOException, MergeConflictException {
     PatchSet basePatchSet = changeEdit.getBasePatchSet();
-    ObjectId basePatchSetCommitId = getPatchSetCommitId(basePatchSet);
+    ObjectId basePatchSetCommitId = basePatchSet.getCommitId();
     ObjectId editCommitId = changeEdit.getEditCommit();
 
     ThreeWayMerger threeWayMerger = MergeStrategy.RESOLVE.newMerger(repository, true);
@@ -522,10 +522,6 @@ public class ChangeEditModifier {
     return user.newCommitterIdent(commitTimestamp, tz);
   }
 
-  private static ObjectId getPatchSetCommitId(PatchSet patchSet) {
-    return ObjectId.fromString(patchSet.getRevision().get());
-  }
-
   private ChangeEdit createEdit(
       Repository repository,
       ChangeNotes notes,

java/com/google/gerrit/server/edit/ChangeEditUtil.java

@@ -174,7 +174,7 @@ public class ChangeEditUtil {
           new StringBuilder("Patch Set ").append(inserter.getPatchSetId().get()).append(": ");
 
       // Previously checked that the base patch set is the current patch set.
-      ObjectId prior = ObjectId.fromString(basePatchSet.getRevision().get());
+      ObjectId prior = basePatchSet.getCommitId();
       ChangeKind kind =
           changeKindCache.getChangeKind(change.getProject(), rw, repo.getConfig(), prior, squashed);
       if (kind == ChangeKind.NO_CODE_CHANGE) {
@@ -232,7 +232,7 @@ public class ChangeEditUtil {
   private RevCommit squashEdit(
       RevWalk rw, ObjectInserter inserter, RevCommit edit, PatchSet basePatchSet)
       throws IOException, ResourceConflictException {
-    RevCommit parent = rw.parseCommit(ObjectId.fromString(basePatchSet.getRevision().get()));
+    RevCommit parent = rw.parseCommit(basePatchSet.getCommitId());
     if (parent.getTree().equals(edit.getTree())
         && edit.getFullMessage().equals(parent.getFullMessage())) {
       throw new ResourceConflictException("identical tree and message");

java/com/google/gerrit/server/events/EventFactory.java

@@ -285,7 +285,7 @@ public class EventFactory {
 
   private void addDependsOn(RevWalk rw, ChangeAttribute ca, Change change, PatchSet currentPs)
       throws IOException {
-    RevCommit commit = rw.parseCommit(ObjectId.fromString(currentPs.getRevision().get()));
+    RevCommit commit = rw.parseCommit(currentPs.getCommitId());
     final List<String> parentNames = new ArrayList<>(commit.getParentCount());
     for (RevCommit p : commit.getParents()) {
       parentNames.add(p.name());
@@ -296,7 +296,7 @@ public class EventFactory {
     for (ChangeData cd : queryProvider.get().byProjectCommits(change.getProject(), parentNames)) {
       for (PatchSet ps : cd.patchSets()) {
         for (String p : parentNames) {
-          if (!ps.getRevision().get().equals(p)) {
+          if (!ps.getCommitId().name().equals(p)) {
             continue;
           }
           ca.dependsOn.add(newDependsOn(requireNonNull(cd.change()), ps));
@@ -321,7 +321,7 @@ public class EventFactory {
     if (currentPs.getGroups().isEmpty()) {
       return;
     }
-    String rev = currentPs.getRevision().get();
+    String rev = currentPs.getCommitId().name();
     // Find changes in the same related group as this patch set, having a patch
     // set whose parent matches this patch set's revision.
     for (ChangeData cd :
@@ -329,7 +329,7 @@ public class EventFactory {
             queryProvider, indexConfig, change.getProject(), currentPs.getGroups())) {
       PATCH_SETS:
       for (PatchSet ps : cd.patchSets()) {
-        RevCommit commit = rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+        RevCommit commit = rw.parseCommit(ps.getCommitId());
         for (RevCommit p : commit.getParents()) {
           if (!p.name().equals(rev)) {
             continue;
@@ -355,7 +355,7 @@ public class EventFactory {
     DependencyAttribute d = new DependencyAttribute();
     d.number = c.getId().get();
     d.id = c.getKey().toString();
-    d.revision = ps.getRevision().get();
+    d.revision = ps.getCommitId().name();
     d.ref = ps.getRefName();
     return d;
   }
@@ -463,7 +463,7 @@ public class EventFactory {
    */
   public PatchSetAttribute asPatchSetAttribute(RevWalk revWalk, Change change, PatchSet patchSet) {
     PatchSetAttribute p = new PatchSetAttribute();
-    p.revision = patchSet.getRevision().get();
+    p.revision = patchSet.getCommitId().name();
     p.number = patchSet.getPatchSetId();
     p.ref = patchSet.getRefName();
     p.uploader = asAccountAttribute(patchSet.getUploader());

java/com/google/gerrit/server/git/MergedByPushOp.java

@@ -42,7 +42,6 @@ import java.io.IOException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
 import org.eclipse.jgit.lib.Constants;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -189,13 +188,12 @@ public class MergedByPushOp implements BatchUpdateOp {
                 }));
 
     changeMerged.fire(
-        change, patchSet, ctx.getAccount(), patchSet.getRevision().get(), ctx.getWhen());
+        change, patchSet, ctx.getAccount(), patchSet.getCommitId().name(), ctx.getWhen());
   }
 
   private PatchSetInfo getPatchSetInfo(ChangeContext ctx) throws IOException {
     RevWalk rw = ctx.getRevWalk();
-    RevCommit commit =
-        rw.parseCommit(ObjectId.fromString(requireNonNull(patchSet).getRevision().get()));
+    RevCommit commit = rw.parseCommit(requireNonNull(patchSet).getCommitId());
     return patchSetInfoFactory.get(rw, commit, psId);
   }
 }

java/com/google/gerrit/server/git/PureRevertCache.java

@@ -97,8 +97,8 @@ public class PureRevertCache {
             claimedRevert.getProjectName(), claimedRevert.getChange().getRevertOf());
     return isPureRevert(
         claimedRevert.getProjectName(),
-        ObjectId.fromString(claimedRevert.getCurrentPatchSet().getRevision().get()),
-        ObjectId.fromString(claimedOriginal.getCurrentPatchSet().getRevision().get()));
+        claimedRevert.getCurrentPatchSet().getCommitId(),
+        claimedOriginal.getCurrentPatchSet().getCommitId());
   }
 
   /**

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -88,7 +88,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CreateGroupPermissionSyncer;
@@ -2188,9 +2187,9 @@ class ReceiveCommits {
           // Schedule as a replacement to this one matching change.
           //
 
-          RevId currentPs = changes.get(0).currentPatchSet().getRevision();
+          ObjectId currentPs = changes.get(0).currentPatchSet().getCommitId();
           // If Commit is already current PatchSet of target Change.
-          if (p.commit.name().equals(currentPs.get())) {
+          if (p.commit.equals(currentPs)) {
             if (pending.size() == 1) {
               // There are no commits left to check, all commits in pending were already
               // current PatchSet of the corresponding target changes.

java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java

@@ -108,12 +108,11 @@ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook {
               .byProjectOpen(projectName)) {
         PatchSet ps = cd.currentPatchSet();
         if (ps != null) {
-          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);
+          if (allPatchSets.contains(ps.getCommitId())) {
+            r.add(ps.getCommitId());
           }
         }
       }

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

@@ -453,8 +453,8 @@ public class ChangeField {
   private static Set<String> getRevisions(ChangeData cd) {
     Set<String> revisions = new HashSet<>();
     for (PatchSet ps : cd.patchSets()) {
-      if (ps.getRevision() != null) {
-        revisions.add(ps.getRevision().get());
+      if (ps.getCommitId() != null) {
+        revisions.add(ps.getCommitId().name());
       }
     }
     return revisions;

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

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

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

@@ -67,7 +67,6 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.ReviewerByEmailSet;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.ReviewerStatusUpdate;
@@ -505,11 +504,11 @@ class ChangeNotesParser {
       throw new ConfigInvalidException(
           String.format(
               "Multiple revisions parsed for patch set %s: %s and %s",
-              psId.get(), patchSets.get(psId).getRevision(), rev.name()));
+              psId.get(), patchSets.get(psId).getCommitId().name(), rev.name()));
     }
     PatchSet ps = new PatchSet(psId);
     patchSets.put(psId, ps);
-    ps.setRevision(new RevId(rev.name()));
+    ps.setCommitId(rev);
     ps.setUploader(accountId);
     ps.setCreatedOn(ts);
     PendingPatchSetFields pending = pendingPatchSets.remove(psId);
@@ -743,7 +742,7 @@ class ChangeNotesParser {
     }
 
     for (PatchSet ps : patchSets.values()) {
-      ChangeRevisionNote rn = rns.get(ObjectId.fromString(ps.getRevision().get()));
+      ChangeRevisionNote rn = rns.get(ps.getCommitId());
       if (rn != null && rn.getPushCert() != null) {
         ps.setPushCertificate(rn.getPushCert());
       }

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

@@ -78,7 +78,7 @@ public class LegacyChangeNoteWrite {
    *
    * @param comments Comments to be written to the output stream, keyed by patch set ID; multiple
    *     patch sets are allowed since base revisions may be shared across patch sets. All of the
-   *     comments must share the same RevId, and all the comments for a given patch set must have
+   *     comments must share the same commitId, and all the comments for a given patch set must have
    *     the same side.
    * @param out output stream to write to.
    */
@@ -114,8 +114,8 @@ public class LegacyChangeNoteWrite {
         for (Comment c : psComments) {
           checkArgument(
               commitId.equals(c.getCommitId()),
-              "All comments being added must have all the same RevId. The "
-                  + "comment below does not have the same RevId as the others "
+              "All comments being added must have all the same commitId. The "
+                  + "comment below does not have the same commitId as the others "
                   + "(%s).\n%s",
               commitId,
               c);

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

@@ -134,10 +134,10 @@ public class PatchListCacheImpl implements PatchListCache {
   private PatchList get(Change change, PatchSet patchSet, Integer parentNum)
       throws PatchListNotAvailableException {
     Project.NameKey project = change.getProject();
-    if (patchSet.getRevision() == null) {
-      throw new PatchListNotAvailableException("revision is null for " + patchSet.getId());
+    ObjectId b = patchSet.getCommitId();
+    if (b == null) {
+      throw new PatchListNotAvailableException("commit ID is null for " + patchSet.getId());
     }
-    ObjectId b = ObjectId.fromString(patchSet.getRevision().get());
     Whitespace ws = Whitespace.IGNORE_NONE;
     if (parentNum != null) {
       return get(PatchListKey.againstParentNum(parentNum, b, ws), project);

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

@@ -262,16 +262,10 @@ public class PatchScriptFactory implements Callable<PatchScript> {
     if (ps.getId().get() == 0) {
       return getEditRev();
     }
-    if (ps.getRevision() == null || ps.getRevision().get() == null) {
+    if (ps.getCommitId() == null) {
       throw new NoSuchChangeException(changeId);
     }
-
-    try {
-      return ObjectId.fromString(ps.getRevision().get());
-    } catch (IllegalArgumentException e) {
-      logger.atSevere().log("Patch set %s has invalid revision", ps.getId());
-      throw new NoSuchChangeException(changeId, e);
-    }
+    return ps.getCommitId();
   }
 
   private ObjectId getEditRev() throws AuthException, IOException {

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

@@ -32,7 +32,6 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
 import org.eclipse.jgit.errors.MissingObjectException;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -77,7 +76,7 @@ public class PatchSetInfoFactory {
       throws PatchSetInfoNotAvailableException {
     try (Repository repo = repoManager.openRepository(project);
         RevWalk rw = new RevWalk(repo)) {
-      final RevCommit src = rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
+      RevCommit src = rw.parseCommit(patchSet.getCommitId());
       PatchSetInfo info = get(rw, src, patchSet.getId());
       info.setParents(toParentInfos(src.getParents(), rw));
       return info;

java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java

@@ -39,6 +39,7 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.index.IndexConfig;
 import com.google.gerrit.index.query.Predicate;
 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.RefNames;
 import com.google.gerrit.server.change.ChangeJson;
@@ -296,7 +297,7 @@ public class ProjectsConsistencyChecker {
                 // Auto-close by commit
                 for (ObjectId patchSetSha1 :
                     autoCloseableChange.patchSets().stream()
-                        .map(ps -> ObjectId.fromString(ps.getRevision().get()))
+                        .map(PatchSet::getCommitId)
                         .collect(toSet())) {
                   if (mergedSha1s.contains(patchSetSha1)) {
                     autoCloseableChangesByBranch.add(

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

@@ -387,7 +387,7 @@ public class ChangeData {
         return Optional.empty();
       }
 
-      ObjectId id = ObjectId.fromString(ps.getRevision().get());
+      ObjectId id = ps.getCommitId();
       Whitespace ws = Whitespace.IGNORE_NONE;
       PatchListKey pk =
           parentCount > 1
@@ -580,10 +580,9 @@ public class ChangeData {
     if (ps == null) {
       return false;
     }
-    String sha1 = ps.getRevision().get();
     try (Repository repo = repoManager.openRepository(project());
         RevWalk walk = new RevWalk(repo)) {
-      RevCommit c = walk.parseCommit(ObjectId.fromString(sha1));
+      RevCommit c = walk.parseCommit(ps.getCommitId());
       commitMessage = c.getFullMessage();
       commitFooters = c.getFooterLines();
       author = c.getAuthorIdent();
@@ -898,12 +897,7 @@ public class ChangeData {
               mergeUtilFactory.create(projectCache.get(project())).mergeStrategyName();
           mergeable =
               mergeabilityCache.get(
-                  ObjectId.fromString(ps.getRevision().get()),
-                  ref,
-                  str.type,
-                  mergeStrategy,
-                  c.getDest(),
-                  repo);
+                  ps.getCommitId(), ref, str.type, mergeStrategy, c.getDest(), repo);
         } catch (IOException e) {
           throw new StorageException(e);
         }

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

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

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

@@ -131,7 +131,7 @@ public class ConflictsPredicate {
           return false;
         }
 
-        other = ObjectId.fromString(object.currentPatchSet().getRevision().get());
+        other = object.currentPatchSet().getCommitId();
         ConflictKey conflictsKey =
             ConflictKey.create(
                 changeDataCache.getTestAgainst(),
@@ -207,7 +207,7 @@ public class ConflictsPredicate {
 
     ObjectId getTestAgainst() {
       if (testAgainst == null) {
-        testAgainst = ObjectId.fromString(cd.currentPatchSet().getRevision().get());
+        testAgainst = cd.currentPatchSet().getCommitId();
       }
       return testAgainst;
     }

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

@@ -38,7 +38,6 @@ import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.List;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 
 @Singleton
@@ -72,12 +71,11 @@ public class ApplyFix implements RestModifyView<FixResource, Void> {
     Project.NameKey project = revisionResource.getProject();
     ProjectState projectState = projectCache.checkedGet(project);
     PatchSet patchSet = revisionResource.getPatchSet();
-    ObjectId patchSetCommitId = ObjectId.fromString(patchSet.getRevision().get());
 
     try (Repository repository = gitRepositoryManager.openRepository(project)) {
       List<TreeModification> treeModifications =
           fixReplacementInterpreter.toTreeModifications(
-              repository, projectState, patchSetCommitId, fixResource.getFixReplacements());
+              repository, projectState, patchSet.getCommitId(), fixResource.getFixReplacements());
       ChangeEdit changeEdit =
           changeEditModifier.combineWithModifiedPatchSetTree(
               repository, revisionResource.getNotes(), patchSet, treeModifications);

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

@@ -60,7 +60,6 @@ import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.List;
 import java.util.Optional;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -358,9 +357,7 @@ public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditRe
         return Response.ok(
             fileContentUtil.getContent(
                 projectCache.checkedGet(rsrc.getChangeResource().getProject()),
-                base
-                    ? ObjectId.fromString(edit.getBasePatchSet().getRevision().get())
-                    : edit.getEditCommit(),
+                base ? edit.getBasePatchSet().getCommitId() : edit.getEditCommit(),
                 rsrc.getPath(),
                 null));
       } catch (ResourceNotFoundException | BadRequestException e) {
@@ -461,9 +458,7 @@ public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditRe
         if (base) {
           try (Repository repo = repoManager.openRepository(rsrc.getProject());
               RevWalk rw = new RevWalk(repo)) {
-            RevCommit commit =
-                rw.parseCommit(
-                    ObjectId.fromString(edit.get().getBasePatchSet().getRevision().get()));
+            RevCommit commit = rw.parseCommit(edit.get().getBasePatchSet().getCommitId());
             msg = commit.getFullMessage();
           }
         } else {

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

@@ -39,6 +39,6 @@ public class ChangeIncludedIn implements RestReadView<ChangeResource> {
   @Override
   public IncludedInInfo apply(ChangeResource rsrc) throws RestApiException, IOException {
     PatchSet ps = psUtil.current(rsrc.getNotes());
-    return includedIn.apply(rsrc.getProject(), ps.getRevision().get());
+    return includedIn.apply(rsrc.getProject(), ps.getCommitId().name());
   }
 }

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

@@ -143,12 +143,7 @@ public class CherryPickChange {
       throws IOException, InvalidChangeOperationException, IntegrationException, UpdateException,
           RestApiException, ConfigInvalidException, NoSuchProjectException {
     return cherryPick(
-        batchUpdateFactory,
-        change,
-        change.getProject(),
-        ObjectId.fromString(patch.getRevision().get()),
-        input,
-        dest);
+        batchUpdateFactory, change, change.getProject(), patch.getCommitId(), input, dest);
   }
 
   public Result cherryPick(

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

@@ -345,7 +345,7 @@ public class CreateChange
       throws BadRequestException, IOException, UnprocessableEntityException,
           ResourceConflictException {
     if (basePatchSet != null) {
-      return ObjectId.fromString(basePatchSet.getRevision().get());
+      return basePatchSet.getCommitId();
     }
 
     Ref destRef = repo.getRefDatabase().exactRef(inputBranch);

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

@@ -154,10 +154,10 @@ public class CreateMergePatchSet
       List<String> groups = null;
       if (!in.inheritParent && !in.baseChange.isEmpty()) {
         PatchSet basePS = findBasePatchSet(in.baseChange);
-        currentPsCommit = rw.parseCommit(ObjectId.fromString(basePS.getRevision().get()));
+        currentPsCommit = rw.parseCommit(basePS.getCommitId());
         groups = basePS.getGroups();
       } else {
-        currentPsCommit = rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+        currentPsCommit = rw.parseCommit(ps.getCommitId());
       }
 
       Timestamp now = TimeUtil.nowTs();

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

@@ -24,7 +24,6 @@ import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.inject.Inject;
 import java.io.IOException;
-import org.eclipse.jgit.lib.ObjectId;
 import org.kohsuke.args4j.Option;
 
 public class DownloadContent implements RestReadView<FileResource> {
@@ -45,8 +44,7 @@ public class DownloadContent implements RestReadView<FileResource> {
       throws ResourceNotFoundException, IOException, NoSuchChangeException {
     String path = rsrc.getPatchKey().fileName();
     RevisionResource rev = rsrc.getRevision();
-    ObjectId revstr = ObjectId.fromString(rev.getPatchSet().getRevision().get());
     return fileContentUtil.downloadContent(
-        projectCache.checkedGet(rev.getProject()), revstr, path, parent);
+        projectCache.checkedGet(rev.getProject()), rev.getPatchSet().getCommitId(), path, parent);
   }
 }

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

@@ -62,7 +62,6 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -163,13 +162,13 @@ public class Files implements ChildCollection<RevisionResource, FileResource> {
             Response.ok(
                 fileInfoJson.toFileInfoMap(
                     resource.getChange(),
-                    resource.getPatchSet().getRevision(),
+                    resource.getPatchSet().getCommitId(),
                     baseResource.getPatchSet()));
       } else if (parentNum > 0) {
         r =
             Response.ok(
                 fileInfoJson.toFileInfoMap(
-                    resource.getChange(), resource.getPatchSet().getRevision(), parentNum - 1));
+                    resource.getChange(), resource.getPatchSet().getCommitId(), parentNum - 1));
       } else {
         r = Response.ok(fileInfoJson.toFileInfoMap(resource.getChange(), resource.getPatchSet()));
       }
@@ -206,8 +205,7 @@ public class Files implements ChildCollection<RevisionResource, FileResource> {
           ObjectReader or = git.newObjectReader();
           RevWalk rw = new RevWalk(or);
           TreeWalk tw = new TreeWalk(or)) {
-        RevCommit c =
-            rw.parseCommit(ObjectId.fromString(resource.getPatchSet().getRevision().get()));
+        RevCommit c = rw.parseCommit(resource.getPatchSet().getCommitId());
 
         tw.addTree(c.getTree());
         tw.setRecursive(true);

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

@@ -29,7 +29,6 @@ import java.io.IOException;
 import java.io.OutputStream;
 import org.eclipse.jgit.api.ArchiveCommand;
 import org.eclipse.jgit.api.errors.GitAPIException;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -67,7 +66,7 @@ public class GetArchive implements RestReadView<RevisionResource> {
       final RevCommit commit;
       String name;
       try (RevWalk rw = new RevWalk(repo)) {
-        commit = rw.parseCommit(ObjectId.fromString(rsrc.getPatchSet().getRevision().get()));
+        commit = rw.parseCommit(rsrc.getPatchSet().getCommitId());
         name = name(f, rw, commit);
       }
 

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

@@ -26,7 +26,6 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.inject.Inject;
 import java.io.IOException;
 import java.util.concurrent.TimeUnit;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -55,8 +54,7 @@ public class GetCommit implements RestReadView<RevisionResource> {
     Project.NameKey p = rsrc.getChange().getProject();
     try (Repository repo = repoManager.openRepository(p);
         RevWalk rw = new RevWalk(repo)) {
-      String rev = rsrc.getPatchSet().getRevision().get();
-      RevCommit commit = rw.parseCommit(ObjectId.fromString(rev));
+      RevCommit commit = rw.parseCommit(rsrc.getPatchSet().getCommitId());
       rw.parseBody(commit);
       CommitInfo info =
           json.create(ImmutableSet.of())

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

@@ -33,7 +33,6 @@ import com.google.gerrit.server.project.ProjectCache;
 import com.google.inject.Inject;
 import java.io.IOException;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -77,7 +76,7 @@ public class GetContent implements RestReadView<FileResource> {
     }
     return fileContentUtil.getContent(
         projectCache.checkedGet(rsrc.getRevision().getProject()),
-        ObjectId.fromString(rsrc.getRevision().getPatchSet().getRevision().get()),
+        rsrc.getRevision().getPatchSet().getCommitId(),
         path,
         parent);
   }
@@ -91,7 +90,7 @@ public class GetContent implements RestReadView<FileResource> {
 
     try (Repository git = gitManager.openRepository(notes.getProjectName());
         RevWalk revWalk = new RevWalk(git)) {
-      RevCommit commit = revWalk.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+      RevCommit commit = revWalk.parseCommit(ps.getCommitId());
       return commit.getFullMessage();
     } catch (RepositoryNotFoundException e) {
       throw new NoSuchChangeException(changeId, e);
@@ -108,9 +107,7 @@ public class GetContent implements RestReadView<FileResource> {
     try (Repository git = gitManager.openRepository(notes.getProjectName());
         RevWalk revWalk = new RevWalk(git)) {
       return Text.forMergeList(
-              ComparisonType.againstAutoMerge(),
-              revWalk.getObjectReader(),
-              ObjectId.fromString(ps.getRevision().get()))
+              ComparisonType.againstAutoMerge(), revWalk.getObjectReader(), ps.getCommitId())
           .getContent();
     } catch (RepositoryNotFoundException e) {
       throw new NoSuchChangeException(changeId, e);

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

@@ -31,7 +31,6 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -67,8 +66,7 @@ public class GetMergeList implements RestReadView<RevisionResource> {
     Project.NameKey p = rsrc.getChange().getProject();
     try (Repository repo = repoManager.openRepository(p);
         RevWalk rw = new RevWalk(repo)) {
-      String rev = rsrc.getPatchSet().getRevision().get();
-      RevCommit commit = rw.parseCommit(ObjectId.fromString(rev));
+      RevCommit commit = rw.parseCommit(rsrc.getPatchSet().getCommitId());
       rw.parseBody(commit);
 
       if (uninterestingParent < 1 || uninterestingParent > commit.getParentCount()) {

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

@@ -32,7 +32,6 @@ import java.util.Locale;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipOutputStream;
 import org.eclipse.jgit.diff.DiffFormatter;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -67,8 +66,7 @@ public class GetPatch implements RestReadView<RevisionResource> {
     try {
       final RevWalk rw = new RevWalk(repo);
       try {
-        final RevCommit commit =
-            rw.parseCommit(ObjectId.fromString(rsrc.getPatchSet().getRevision().get()));
+        final RevCommit commit = rw.parseCommit(rsrc.getPatchSet().getCommitId());
         RevCommit[] parents = commit.getParents();
         if (parents.length > 1) {
           throw new ResourceConflictException("Revision has more than 1 parent.");

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

@@ -114,7 +114,7 @@ public class GetRelated implements RestReadView<RevisionResource> {
 
     if (result.size() == 1) {
       RelatedChangeAndCommitInfo r = result.get(0);
-      if (r.commit != null && r.commit.commit.equals(rsrc.getPatchSet().getRevision().get())) {
+      if (r.commit != null && r.commit.commit.equals(rsrc.getPatchSet().getCommitId().name())) {
         return Collections.emptyList();
       }
     }

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

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.restapi.change;
 
-import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.data.SubmitTypeRecord;
 import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.client.SubmitType;
@@ -50,8 +49,6 @@ import org.eclipse.jgit.lib.Repository;
 import org.kohsuke.args4j.Option;
 
 public class Mergeable implements RestReadView<RevisionResource> {
-  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
   @Option(
       name = "--other-branches",
       aliases = {"-o"},
@@ -106,7 +103,7 @@ public class Mergeable implements RestReadView<RevisionResource> {
     result.submitType = getSubmitType(cd);
 
     try (Repository git = gitManager.openRepository(change.getProject())) {
-      ObjectId commit = toId(ps);
+      ObjectId commit = ps.getCommitId();
       Ref ref = git.getRefDatabase().exactRef(change.getDest().branch());
       ProjectState projectState = projectCache.get(change.getProject());
       String strategy = mergeUtilFactory.create(projectState).mergeStrategyName();
@@ -161,15 +158,6 @@ public class Mergeable implements RestReadView<RevisionResource> {
     return refresh(change, commit, ref, submitType, strategy, git, old);
   }
 
-  private static ObjectId toId(PatchSet ps) {
-    try {
-      return ObjectId.fromString(ps.getRevision().get());
-    } catch (IllegalArgumentException e) {
-      logger.atSevere().log("Invalid revision on patch set %s", ps);
-      return null;
-    }
-  }
-
   private boolean refresh(
       final Change change,
       ObjectId commit,

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

@@ -193,8 +193,7 @@ public class Move extends RetryingRestModifyView<ChangeResource, MoveInput, Chan
       try (Repository repo = repoManager.openRepository(projectKey);
           RevWalk revWalk = new RevWalk(repo)) {
         RevCommit currPatchsetRevCommit =
-            revWalk.parseCommit(
-                ObjectId.fromString(psUtil.current(ctx.getNotes()).getRevision().get()));
+            revWalk.parseCommit(psUtil.current(ctx.getNotes()).getCommitId());
         if (currPatchsetRevCommit.getParentCount() > 1) {
           throw new ResourceConflictException("Merge commit cannot be moved");
         }

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

@@ -584,7 +584,7 @@ public class PostReview
 
   private Set<String> getAffectedFilePaths(RevisionResource revision)
       throws PatchListNotAvailableException {
-    ObjectId newId = ObjectId.fromString(revision.getPatchSet().getRevision().get());
+    ObjectId newId = revision.getPatchSet().getCommitId();
     DiffSummaryKey key =
         DiffSummaryKey.fromPatchListKey(
             PatchListKey.againstDefaultBase(newId, Whitespace.IGNORE_NONE));

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

@@ -119,7 +119,7 @@ public class PutMessage
     try (Repository repository = repositoryManager.openRepository(resource.getProject());
         RevWalk revWalk = new RevWalk(repository);
         ObjectInserter objectInserter = repository.newObjectInserter()) {
-      RevCommit patchSetCommit = revWalk.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+      RevCommit patchSetCommit = revWalk.parseCommit(ps.getCommitId());
 
       String currentCommitMessage = patchSetCommit.getFullMessage();
       if (input.message.equals(currentCommitMessage)) {

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

@@ -181,18 +181,18 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
               + baseChange.getKey()
               + " is a descendant of the current change - recursion not allowed");
     }
-    return ObjectId.fromString(base.patchSet().getRevision().get());
+    return base.patchSet().getCommitId();
   }
 
   private boolean isMergedInto(RevWalk rw, PatchSet base, PatchSet tip) throws IOException {
-    ObjectId baseId = ObjectId.fromString(base.getRevision().get());
-    ObjectId tipId = ObjectId.fromString(tip.getRevision().get());
+    ObjectId baseId = base.getCommitId();
+    ObjectId tipId = tip.getCommitId();
     return rw.isMergedInto(rw.parseCommit(baseId), rw.parseCommit(tipId));
   }
 
   private boolean hasOneParent(RevWalk rw, PatchSet ps) throws IOException {
     // Prevent rebase of exotic changes (merge commit, no ancestor).
-    RevCommit c = rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+    RevCommit c = rw.parseCommit(ps.getCommitId());
     return c.getParentCount() == 1;
   }
 

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

@@ -74,8 +74,8 @@ class RelatedChangesSorter {
       throws IOException, PermissionBackendException {
     checkArgument(!in.isEmpty(), "Input may not be empty");
     // Map of all patch sets, keyed by commit SHA-1.
-    Map<String, PatchSetData> byId = collectById(in);
-    PatchSetData start = byId.get(startPs.getRevision().get());
+    Map<ObjectId, PatchSetData> byId = collectById(in);
+    PatchSetData start = byId.get(startPs.getCommitId());
     checkArgument(start != null, "%s not found in %s", startPs, in);
 
     // Map of patch set -> immediate parent.
@@ -89,12 +89,12 @@ class RelatedChangesSorter {
 
     for (ChangeData cd : in) {
       for (PatchSet ps : cd.patchSets()) {
-        PatchSetData thisPsd = requireNonNull(byId.get(ps.getRevision().get()));
+        PatchSetData thisPsd = requireNonNull(byId.get(ps.getCommitId()));
         if (cd.getId().equals(start.id()) && !ps.getId().equals(start.psId())) {
           otherPatchSetsOfStart.add(thisPsd);
         }
         for (RevCommit p : thisPsd.commit().getParents()) {
-          PatchSetData parentPsd = byId.get(p.name());
+          PatchSetData parentPsd = byId.get(p);
           if (parentPsd != null) {
             parents.put(thisPsd, parentPsd);
             children.put(parentPsd, thisPsd);
@@ -112,9 +112,9 @@ class RelatedChangesSorter {
     return result;
   }
 
-  private Map<String, PatchSetData> collectById(List<ChangeData> in) throws IOException {
+  private Map<ObjectId, PatchSetData> collectById(List<ChangeData> in) throws IOException {
     Project.NameKey project = in.get(0).change().getProject();
-    Map<String, PatchSetData> result = Maps.newHashMapWithExpectedSize(in.size() * 3);
+    Map<ObjectId, PatchSetData> result = Maps.newHashMapWithExpectedSize(in.size() * 3);
     try (Repository repo = repoManager.openRepository(project);
         RevWalk rw = new RevWalk(repo)) {
       rw.setRetainBody(true);
@@ -126,10 +126,9 @@ class RelatedChangesSorter {
             project,
             cd.change().getProject());
         for (PatchSet ps : cd.patchSets()) {
-          String id = ps.getRevision().get();
-          RevCommit c = rw.parseCommit(ObjectId.fromString(id));
+          RevCommit c = rw.parseCommit(ps.getCommitId());
           PatchSetData psd = PatchSetData.create(cd, ps, c);
-          result.put(id, psd);
+          result.put(ps.getCommitId(), psd);
         }
       }
     }

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

@@ -172,8 +172,7 @@ public class Revert extends RetryingRestModifyView<ChangeResource, RevertInput,
         ObjectInserter oi = git.newObjectInserter();
         ObjectReader reader = oi.newReader();
         RevWalk revWalk = new RevWalk(reader)) {
-      RevCommit commitToRevert =
-          revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get()));
+      RevCommit commitToRevert = revWalk.parseCommit(patch.getCommitId());
       if (commitToRevert.getParentCount() == 0) {
         throw new ResourceConflictException("Cannot revert initial commit");
       }
@@ -198,7 +197,7 @@ public class Revert extends RetryingRestModifyView<ChangeResource, RevertInput,
             MessageFormat.format(
                 ChangeMessages.get().revertChangeDefaultMessage,
                 changeToRevert.getSubject(),
-                patch.getRevision().get());
+                patch.getCommitId().name());
       }
 
       ObjectId computedChangeId =

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.change;
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.Lists;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ChildCollection;
@@ -24,7 +25,6 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.git.ObjectIds;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.RevisionResource;
@@ -129,13 +129,13 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
     } else {
       List<RevisionResource> out = new ArrayList<>();
       for (PatchSet ps : psUtil.byChange(change.getNotes())) {
-        if (ps.getRevision() != null && ps.getRevision().get().startsWith(id)) {
+        if (ObjectIds.matchesAbbreviation(ps.getCommitId(), id)) {
           out.add(new RevisionResource(change, ps));
         }
       }
       // Not an existing patch set on a change, but might be an edit.
-      if (out.isEmpty() && id.length() == ObjectIds.STR_LEN) {
-        return loadEdit(change, new RevId(id));
+      if (out.isEmpty() && ObjectId.isId(id)) {
+        return loadEdit(change, ObjectId.fromString(id));
       }
       return out;
     }
@@ -149,14 +149,14 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
     return Collections.emptyList();
   }
 
-  private List<RevisionResource> loadEdit(ChangeResource change, RevId revid)
+  private List<RevisionResource> loadEdit(ChangeResource change, @Nullable ObjectId commitId)
       throws AuthException, IOException {
     Optional<ChangeEdit> edit = editUtil.byChange(change.getNotes(), change.getUser());
     if (edit.isPresent()) {
       PatchSet ps = new PatchSet(PatchSet.id(change.getId(), 0));
-      RevId editRevId = new RevId(ObjectId.toString(edit.get().getEditCommit()));
-      ps.setRevision(editRevId);
-      if (revid == null || editRevId.equals(revid)) {
+      ObjectId editCommitId = edit.get().getEditCommit();
+      ps.setCommitId(edit.get().getEditCommit());
+      if (commitId == null || editCommitId.equals(commitId)) {
         return Collections.singletonList(new RevisionResource(change, ps, edit));
       }
     }

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

@@ -37,7 +37,6 @@ import com.google.gerrit.reviewdb.client.BranchNameKey;
 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.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
@@ -203,7 +202,7 @@ public class Submit
       // TODO Allow submitting non-current revision by changing the current.
       throw new ResourceConflictException(
           String.format(
-              "revision %s is not current revision", rsrc.getPatchSet().getRevision().get()));
+              "revision %s is not current revision", rsrc.getPatchSet().getCommitId().name()));
     }
 
     try (MergeOp op = mergeOpProvider.get()) {
@@ -365,12 +364,11 @@ public class Submit
           .setVisible(true)
           .setEnabled(Boolean.TRUE.equals(enabled));
     }
-    RevId revId = resource.getPatchSet().getRevision();
     Map<String, String> params =
         ImmutableMap.of(
             "patchSet", String.valueOf(resource.getPatchSet().getPatchSetId()),
             "branch", change.getDest().shortName(),
-            "commit", abbreviateName(ObjectId.fromString(revId.get())),
+            "commit", abbreviateName(resource.getPatchSet().getCommitId()),
             "submitSize", String.valueOf(cs.size()));
     ParameterizedString tp = cs.size() > 1 ? titlePatternWithAncestors : titlePattern;
     return new UiAction.Description()
@@ -434,9 +432,7 @@ public class Submit
     try (Repository repo = repoManager.openRepository(project);
         RevWalk walk = new RevWalk(repo)) {
       for (ChangeData change : changes) {
-        RevCommit commit =
-            walk.parseCommit(
-                ObjectId.fromString(psUtil.current(change.notes()).getRevision().get()));
+        RevCommit commit = walk.parseCommit(psUtil.current(change.notes()).getCommitId());
         commits.put(change.getId(), commit);
       }
     }

java/com/google/gerrit/server/rules/StoredValues.java

@@ -42,7 +42,6 @@ import com.googlecode.prolog_cafe.lang.Prolog;
 import java.io.IOException;
 import java.util.HashMap;
 import java.util.Map;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 
@@ -96,9 +95,8 @@ public final class StoredValues {
           PatchListCache plCache = env.getArgs().getPatchListCache();
           Change change = getChange(engine);
           Project.NameKey project = change.getProject();
-          ObjectId b = ObjectId.fromString(ps.getRevision().get());
           Whitespace ws = Whitespace.IGNORE_NONE;
-          PatchListKey plKey = PatchListKey.againstDefaultBase(b, ws);
+          PatchListKey plKey = PatchListKey.againstDefaultBase(ps.getCommitId(), ws);
           PatchList patchList;
           try {
             patchList = plCache.get(plKey, project);

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

@@ -49,7 +49,8 @@ public enum CommitMergeStatus {
           + "Please rebase the change locally and upload the rebased commit for review."),
 
   SKIPPED_IDENTICAL_TREE(
-      "Marking change merged without cherry-picking to branch, as the resulting commit would be empty."),
+      "Marking change merged without cherry-picking to branch, as the resulting commit would be"
+          + " empty."),
 
   MISSING_DEPENDENCY("Depends on change that was not submitted."),
 
@@ -102,14 +103,14 @@ public enum CommitMergeStatus {
           commit, otherCommit, caller != null ? caller.getLoggableName() : "<user-not-available>");
     } else if (changes.size() == 1) {
       ChangeData cd = changes.get(0);
-      if (cd.currentPatchSet().getRevision().get().equals(otherCommit)) {
+      if (cd.currentPatchSet().getCommitId().name().equals(otherCommit)) {
         return String.format(
             "Commit %s depends on commit %s of change %d which cannot be merged.",
             commit, otherCommit, cd.getId().get());
       }
       Optional<PatchSet> patchSet =
           cd.patchSets().stream()
-              .filter(ps -> ps.getRevision().get().equals(otherCommit))
+              .filter(ps -> ps.getCommitId().name().equals(otherCommit))
               .findAny();
       if (patchSet.isPresent()) {
         return String.format(

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

@@ -53,7 +53,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevSort;
@@ -135,8 +134,7 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
         }
 
         // Get the underlying git commit object
-        String objIdStr = cd.currentPatchSet().getRevision().get();
-        RevCommit commit = or.rw.parseCommit(ObjectId.fromString(objIdStr));
+        RevCommit commit = or.rw.parseCommit(cd.currentPatchSet().getCommitId());
 
         // Always include the input, even if merged. This allows
         // SubmitStrategyOp to correct the situation later, assuming it gets

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

@@ -776,20 +776,12 @@ public class MergeOp implements AutoCloseable {
         commitStatus.logProblem(changeId, e);
         continue;
       }
-      if (ps == null || ps.getRevision() == null || ps.getRevision().get() == null) {
+      if (ps == null || ps.getCommitId() == null) {
         commitStatus.logProblem(changeId, "Missing patch set or revision on change");
         continue;
       }
 
-      String idstr = ps.getRevision().get();
-      ObjectId id;
-      try {
-        id = ObjectId.fromString(idstr);
-      } catch (IllegalArgumentException e) {
-        commitStatus.logProblem(changeId, e);
-        continue;
-      }
-
+      ObjectId id = ps.getCommitId();
       if (!revisions.containsEntry(id, ps.getId())) {
         if (revisions.containsValue(ps.getId())) {
           // TODO This is actually an error, the patch set ref exists but points to a revision that
@@ -798,7 +790,7 @@ public class MergeOp implements AutoCloseable {
           commitStatus.logProblem(
               changeId,
               "Revision "
-                  + idstr
+                  + id.name()
                   + " of patch set "
                   + ps.getPatchSetId()
                   + " does not match the revision of the patch set ref "
@@ -817,7 +809,7 @@ public class MergeOp implements AutoCloseable {
                 + " not found. Expected patch set ref of "
                 + ps.getPatchSetId()
                 + " to point to revision "
-                + idstr);
+                + id.name());
         continue;
       }
 

java/com/google/gerrit/sshd/commands/PatchSetParser.java

@@ -76,7 +76,7 @@ public class PatchSetParser {
           continue;
         }
         for (PatchSet ps : cd.patchSets()) {
-          if (ps.getRevision().matches(token)) {
+          if (ObjectIds.matchesAbbreviation(ps.getCommitId(), token)) {
             matches.add(ps);
           }
         }

java/com/google/gerrit/sshd/commands/ReviewCommand.java

@@ -243,7 +243,7 @@ public class ReviewCommand extends SshCommand {
   private void applyReview(PatchSet patchSet, ReviewInput review) throws RestApiException {
     gApi.changes()
         .id(patchSet.getId().changeId().get())
-        .revision(patchSet.getRevision().get())
+        .revision(patchSet.getCommitId().name())
         .review(review);
   }
 
@@ -314,7 +314,7 @@ public class ReviewCommand extends SshCommand {
   }
 
   private RevisionApi revisionApi(PatchSet patchSet) throws RestApiException {
-    return changeApi(patchSet).revision(patchSet.getRevision().get());
+    return changeApi(patchSet).revision(patchSet.getCommitId().name());
   }
 
   @Override

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

@@ -24,7 +24,6 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.AbstractChangeNotes;
@@ -70,7 +69,7 @@ public class TestChanges {
 
   public static PatchSet newPatchSet(PatchSet.Id id, String revision, Account.Id userId) {
     PatchSet ps = new PatchSet(id);
-    ps.setRevision(new RevId(revision));
+    ps.setCommitId(ObjectId.fromString(revision));
     ps.setUploader(userId);
     ps.setCreatedOn(TimeUtil.nowTs());
     return ps;

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -1301,14 +1301,14 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(ps2.getId().get()).isEqualTo(2);
 
     // rebase r1 onto r2 (referenced by commit)
-    ri.base = ps2.getRevision().get();
+    ri.base = ps2.getCommitId().name();
     gApi.changes().id(r1.getChangeId()).revision(r1.getCommit().name()).rebase(ri);
     PatchSet ps1 = r1.getPatchSet();
     assertThat(ps1.getId().get()).isEqualTo(2);
 
     // rebase r1 onto r3 (referenced by change number)
     ri.base = String.valueOf(r3.getChange().getId().get());
-    gApi.changes().id(r1.getChangeId()).revision(ps1.getRevision().get()).rebase(ri);
+    gApi.changes().id(r1.getChangeId()).revision(ps1.getCommitId().name()).rebase(ri);
     assertThat(r1.getPatchSetId().get()).isEqualTo(3);
   }
 

javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java

@@ -240,13 +240,16 @@ public class ChangeEditIT extends AbstractDaemonTest {
     PatchSet currentPatchSet = getCurrentPatchSet(changeId2);
 
     Optional<EditInfo> originalEdit = getEdit(changeId2);
-    assertThat(originalEdit).value().baseRevision().isEqualTo(previousPatchSet.getRevision().get());
+    assertThat(originalEdit)
+        .value()
+        .baseRevision()
+        .isEqualTo(previousPatchSet.getCommitId().name());
     Timestamp beforeRebase = originalEdit.get().commit.committer.date;
     gApi.changes().id(changeId2).edit().rebase();
     ensureSameBytes(getFileContentOfEdit(changeId2, FILE_NAME), CONTENT_NEW);
     ensureSameBytes(getFileContentOfEdit(changeId2, FILE_NAME2), CONTENT_NEW2);
     Optional<EditInfo> rebasedEdit = getEdit(changeId2);
-    assertThat(rebasedEdit).value().baseRevision().isEqualTo(currentPatchSet.getRevision().get());
+    assertThat(rebasedEdit).value().baseRevision().isEqualTo(currentPatchSet.getCommitId().name());
     assertThat(rebasedEdit).value().commit().committer().date().isNotEqualTo(beforeRebase);
   }
 
@@ -259,13 +262,16 @@ public class ChangeEditIT extends AbstractDaemonTest {
     PatchSet currentPatchSet = getCurrentPatchSet(changeId2);
 
     Optional<EditInfo> originalEdit = getEdit(changeId2);
-    assertThat(originalEdit).value().baseRevision().isEqualTo(previousPatchSet.getRevision().get());
+    assertThat(originalEdit)
+        .value()
+        .baseRevision()
+        .isEqualTo(previousPatchSet.getCommitId().name());
     Timestamp beforeRebase = originalEdit.get().commit.committer.date;
     adminRestSession.post(urlRebase(changeId2)).assertNoContent();
     ensureSameBytes(getFileContentOfEdit(changeId2, FILE_NAME), CONTENT_NEW);
     ensureSameBytes(getFileContentOfEdit(changeId2, FILE_NAME2), CONTENT_NEW2);
     Optional<EditInfo> rebasedEdit = getEdit(changeId2);
-    assertThat(rebasedEdit).value().baseRevision().isEqualTo(currentPatchSet.getRevision().get());
+    assertThat(rebasedEdit).value().baseRevision().isEqualTo(currentPatchSet.getCommitId().name());
     assertThat(rebasedEdit).value().commit().committer().date().isNotEqualTo(beforeRebase);
   }
 
@@ -275,7 +281,7 @@ public class ChangeEditIT extends AbstractDaemonTest {
     createEmptyEditFor(changeId2);
     gApi.changes().id(changeId2).edit().modifyFile(FILE_NAME, RawInputUtil.create(CONTENT_NEW));
     Optional<EditInfo> edit = getEdit(changeId2);
-    assertThat(edit).value().baseRevision().isEqualTo(currentPatchSet.getRevision().get());
+    assertThat(edit).value().baseRevision().isEqualTo(currentPatchSet.getCommitId().name());
     PushOneCommit push =
         pushFactory.create(
             admin.newIdent(),
@@ -382,7 +388,7 @@ public class ChangeEditIT extends AbstractDaemonTest {
     r = adminRestSession.getJsonAccept(urlEditMessage(changeId, true));
     try (Repository repo = repoManager.openRepository(project);
         RevWalk rw = new RevWalk(repo)) {
-      RevCommit commit = rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+      RevCommit commit = rw.parseCommit(ObjectId.fromString(ps.getCommitId().name()));
       assertThat(readContentFromJson(r)).isEqualTo(commit.getFullMessage());
     }
 

javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -1802,7 +1802,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
       throws Exception {
     Change.Id id = accidentallyPushNewPatchSetDirectlyToBranch();
     ChangeData cd = byChangeId(id);
-    String ps1Rev = Iterables.getOnlyElement(cd.patchSets()).getRevision().get();
+    String ps1Rev = Iterables.getOnlyElement(cd.patchSets()).getCommitId().name();
 
     String r = "refs/changes/" + id;
     assertPushOk(pushHead(testRepo, r, false), r);
@@ -1820,7 +1820,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
       throws Exception {
     Change.Id id = accidentallyPushNewPatchSetDirectlyToBranch();
     ChangeData cd = byChangeId(id);
-    String ps1Rev = Iterables.getOnlyElement(cd.patchSets()).getRevision().get();
+    String ps1Rev = Iterables.getOnlyElement(cd.patchSets()).getCommitId().name();
 
     String r = "refs/for/master";
     assertPushRejected(pushHead(testRepo, r, false), r, "no new changes");
@@ -2627,7 +2627,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
   private static Map<Integer, String> getPatchSetRevisions(ChangeData cd) throws Exception {
     Map<Integer, String> revisions = new HashMap<>();
     for (PatchSet ps : cd.patchSets()) {
-      revisions.put(ps.getPatchSetId(), ps.getRevision().get());
+      revisions.put(ps.getPatchSetId(), ps.getCommitId().name());
     }
     return revisions;
   }

javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java

@@ -776,7 +776,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
     PatchSet.Id psId = PatchSet.id(cd.getId(), psNum);
     PatchSet ps = cd.patchSet(psId);
     assertWithMessage("%s not found in %s", psId, cd.patchSets()).that(ps).isNotNull();
-    return ObjectId.fromString(ps.getRevision().get());
+    return ps.getCommitId();
   }
 
   private AccountGroup.UUID createSelfOwnedGroup(String name, TestAccount... members)

javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java

@@ -164,7 +164,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
     assertSubmitApproval(psId);
 
     assertThat(cd.patchSets()).hasSize(1);
-    assertThat(cd.patchSet(psId).getRevision().get()).isEqualTo(c.name());
+    assertThat(cd.patchSet(psId).getCommitId()).isEqualTo(c);
   }
 
   @Test
@@ -237,8 +237,8 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
     assertSubmitApproval(psId2);
 
     assertThat(cd.patchSets()).hasSize(2);
-    assertThat(cd.patchSet(psId1).getRevision().get()).isEqualTo(c1.name());
-    assertThat(cd.patchSet(psId2).getRevision().get()).isEqualTo(c2.name());
+    assertThat(cd.patchSet(psId1).getCommitId()).isEqualTo(c1);
+    assertThat(cd.patchSet(psId2).getCommitId()).isEqualTo(c2);
   }
 
   @Test
@@ -304,15 +304,15 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
     assertThat(cd2.change().isMerged()).isTrue();
     PatchSet.Id psId2_2 = cd2.change().currentPatchSetId();
     assertThat(psId2_2.get()).isEqualTo(2);
-    assertThat(cd2.patchSet(psId2_1).getRevision().get()).isEqualTo(c2_1.name());
-    assertThat(cd2.patchSet(psId2_2).getRevision().get()).isEqualTo(c2_2.name());
+    assertThat(cd2.patchSet(psId2_1).getCommitId()).isEqualTo(c2_1);
+    assertThat(cd2.patchSet(psId2_2).getCommitId()).isEqualTo(c2_2);
 
     ChangeData cd1 = r1.getChange();
     assertThat(cd1.change().isMerged()).isTrue();
     PatchSet.Id psId1_2 = cd1.change().currentPatchSetId();
     assertThat(psId1_2.get()).isEqualTo(2);
-    assertThat(cd1.patchSet(psId1_1).getRevision().get()).isEqualTo(c1_1.name());
-    assertThat(cd1.patchSet(psId1_2).getRevision().get()).isEqualTo(c1_2.name());
+    assertThat(cd1.patchSet(psId1_1).getCommitId()).isEqualTo(c1_1);
+    assertThat(cd1.patchSet(psId1_2).getCommitId()).isEqualTo(c1_2);
   }
 
   private PatchSetApproval getSubmitter(PatchSet.Id patchSetId) throws Exception {

javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java

@@ -225,7 +225,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
           break;
         case REBASE_IF_NECESSARY:
         case REBASE_ALWAYS:
-          String change2hash = change2.getChange().currentPatchSet().getRevision().get();
+          String change2hash = change2.getChange().currentPatchSet().getCommitId().name();
           assertThat(e.getMessage())
               .isEqualTo(
                   "Cannot rebase "

javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java

@@ -102,8 +102,8 @@ public class SubmitByRebaseAlwaysIT extends AbstractSubmitByRebase {
       ChangeData cd1 = changes.get(0).getChange();
       ChangeData cd2 = changes.get(1).getChange();
       assertThat(cd2.patchSets()).hasSize(2);
-      String change1CurrentCommit = cd1.currentPatchSet().getRevision().get();
-      String change2Ps1Commit = cd2.patchSet(PatchSet.id(cd2.getId(), 1)).getRevision().get();
+      String change1CurrentCommit = cd1.currentPatchSet().getCommitId().name();
+      String change2Ps1Commit = cd2.patchSet(PatchSet.id(cd2.getId(), 1)).getCommitId().name();
 
       assertThat(gApi.changes().id(cd2.getId().get()).revision(2).commit(false).message)
           .isEqualTo(

javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java

@@ -153,8 +153,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   @Test
   public void patchSetRefMissing() throws Exception {
     ChangeNotes notes = insertChange();
-    serverSideTestRepo.update(
-        "refs/other/foo", ObjectId.fromString(psUtil.current(notes).getRevision().get()));
+    serverSideTestRepo.update("refs/other/foo", psUtil.current(notes).getCommitId());
     String refName = notes.getChange().currentPatchSetId().toRefName();
     deleteRef(refName);
 
@@ -164,15 +163,15 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   @Test
   public void patchSetRefMissingWithFix() throws Exception {
     ChangeNotes notes = insertChange();
-    String rev = psUtil.current(notes).getRevision().get();
-    serverSideTestRepo.update("refs/other/foo", ObjectId.fromString(rev));
+    ObjectId commitId = psUtil.current(notes).getCommitId();
+    serverSideTestRepo.update("refs/other/foo", commitId);
     String refName = notes.getChange().currentPatchSetId().toRefName();
     deleteRef(refName);
 
     assertProblems(
         notes, new FixInput(), problem("Ref missing: " + refName, FIXED, "Repaired patch set ref"));
-    assertThat(serverSideTestRepo.getRepository().exactRef(refName).getObjectId().name())
-        .isEqualTo(rev);
+    assertThat(serverSideTestRepo.getRepository().exactRef(refName).getObjectId())
+        .isEqualTo(commitId);
   }
 
   @Test
@@ -280,13 +279,14 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   public void duplicatePatchSetRevisions() throws Exception {
     ChangeNotes notes = insertChange();
     PatchSet ps1 = psUtil.current(notes);
-    String rev = ps1.getRevision().get();
 
     notes =
-        incrementPatchSet(
-            notes, serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
+        incrementPatchSet(notes, serverSideTestRepo.getRevWalk().parseCommit(ps1.getCommitId()));
 
-    assertProblems(notes, null, problem("Multiple patch sets pointing to " + rev + ": [1, 2]"));
+    assertProblems(
+        notes,
+        null,
+        problem("Multiple patch sets pointing to " + ps1.getCommitId().name() + ": [1, 2]"));
   }
 
   @Test
@@ -322,14 +322,13 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     }
     notes = reload(notes);
 
-    String rev = psUtil.current(notes).getRevision().get();
     ObjectId tip = getDestRef(notes);
     assertProblems(
         notes,
         null,
         problem(
             "Patch set 1 ("
-                + rev
+                + psUtil.current(notes).getCommitId().name()
                 + ") is not merged into destination ref"
                 + " refs/heads/master ("
                 + tip.name()
@@ -339,40 +338,40 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   @Test
   public void newChangeIsMerged() throws Exception {
     ChangeNotes notes = insertChange();
-    String rev = psUtil.current(notes).getRevision().get();
+    ObjectId commitId = psUtil.current(notes).getCommitId();
     serverSideTestRepo
         .branch(notes.getChange().getDest().branch())
-        .update(serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
+        .update(serverSideTestRepo.getRevWalk().parseCommit(commitId));
 
     assertProblems(
         notes,
         null,
         problem(
             "Patch set 1 ("
-                + rev
+                + commitId.name()
                 + ") is merged into destination ref"
                 + " refs/heads/master ("
-                + rev
+                + commitId.name()
                 + "), but change status is NEW"));
   }
 
   @Test
   public void newChangeIsMergedWithFix() throws Exception {
     ChangeNotes notes = insertChange();
-    String rev = psUtil.current(notes).getRevision().get();
+    ObjectId commitId = psUtil.current(notes).getCommitId();
     serverSideTestRepo
         .branch(notes.getChange().getDest().branch())
-        .update(serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
+        .update(serverSideTestRepo.getRevWalk().parseCommit(commitId));
 
     assertProblems(
         notes,
         new FixInput(),
         problem(
             "Patch set 1 ("
-                + rev
+                + commitId.name()
                 + ") is merged into destination ref"
                 + " refs/heads/master ("
-                + rev
+                + commitId.name()
                 + "), but change status is NEW",
             FIXED,
             "Marked change as merged"));
@@ -385,10 +384,10 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   @Test
   public void extensionApiReturnsUpdatedValueAfterFix() throws Exception {
     ChangeNotes notes = insertChange();
-    String rev = psUtil.current(notes).getRevision().get();
+    ObjectId commitId = psUtil.current(notes).getCommitId();
     serverSideTestRepo
         .branch(notes.getChange().getDest().branch())
-        .update(serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
+        .update(serverSideTestRepo.getRevWalk().parseCommit(commitId));
 
     ChangeInfo info = gApi.changes().id(notes.getChangeId().get()).info();
     assertThat(info.status).isEqualTo(ChangeStatus.NEW);
@@ -400,22 +399,22 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   @Test
   public void expectedMergedCommitIsLatestPatchSet() throws Exception {
     ChangeNotes notes = insertChange();
-    String rev = psUtil.current(notes).getRevision().get();
+    ObjectId commitId = psUtil.current(notes).getCommitId();
     serverSideTestRepo
         .branch(notes.getChange().getDest().branch())
-        .update(serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)));
+        .update(serverSideTestRepo.getRevWalk().parseCommit(commitId));
 
     FixInput fix = new FixInput();
-    fix.expectMergedAs = rev;
+    fix.expectMergedAs = commitId.name();
     assertProblems(
         notes,
         fix,
         problem(
             "Patch set 1 ("
-                + rev
+                + commitId.name()
                 + ") is merged into destination ref"
                 + " refs/heads/master ("
-                + rev
+                + commitId.name()
                 + "), but change status is NEW",
             FIXED,
             "Marked change as merged"));
@@ -428,8 +427,8 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   @Test
   public void expectedMergedCommitNotMergedIntoDestination() throws Exception {
     ChangeNotes notes = insertChange();
-    String rev = psUtil.current(notes).getRevision().get();
-    RevCommit commit = serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
+    RevCommit commit =
+        serverSideTestRepo.getRevWalk().parseCommit(psUtil.current(notes).getCommitId());
     serverSideTestRepo.branch(notes.getChange().getDest().branch()).update(commit);
 
     FixInput fix = new FixInput();
@@ -451,8 +450,8 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   public void createNewPatchSetForExpectedMergeCommitWithNoChangeId() throws Exception {
     ChangeNotes notes = insertChange();
     String dest = notes.getChange().getDest().branch();
-    String rev = psUtil.current(notes).getRevision().get();
-    RevCommit commit = serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
+    RevCommit commit =
+        serverSideTestRepo.getRevWalk().parseCommit(psUtil.current(notes).getCommitId());
 
     RevCommit mergedAs =
         serverSideTestRepo
@@ -483,7 +482,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     notes = reload(notes);
     PatchSet.Id psId2 = PatchSet.id(notes.getChangeId(), 2);
     assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId2);
-    assertThat(psUtil.get(notes, psId2).getRevision().get()).isEqualTo(mergedAs.name());
+    assertThat(psUtil.get(notes, psId2).getCommitId()).isEqualTo(mergedAs);
 
     assertNoProblems(notes, null);
   }
@@ -492,8 +491,8 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   public void createNewPatchSetForExpectedMergeCommitWithChangeId() throws Exception {
     ChangeNotes notes = insertChange();
     String dest = notes.getChange().getDest().branch();
-    String rev = psUtil.current(notes).getRevision().get();
-    RevCommit commit = serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
+    RevCommit commit =
+        serverSideTestRepo.getRevWalk().parseCommit(psUtil.current(notes).getCommitId());
 
     RevCommit mergedAs =
         serverSideTestRepo
@@ -531,7 +530,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     notes = reload(notes);
     PatchSet.Id psId2 = PatchSet.id(notes.getChangeId(), 2);
     assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId2);
-    assertThat(psUtil.get(notes, psId2).getRevision().get()).isEqualTo(mergedAs.name());
+    assertThat(psUtil.get(notes, psId2).getCommitId()).isEqualTo(mergedAs);
 
     assertNoProblems(notes, null);
   }
@@ -539,30 +538,32 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   @Test
   public void expectedMergedCommitIsOldPatchSetOfSameChange() throws Exception {
     ChangeNotes notes = insertChange();
-    PatchSet ps1 = psUtil.current(notes);
-    String rev1 = ps1.getRevision().get();
+    ObjectId commitId1 = psUtil.current(notes).getCommitId();
     notes = incrementPatchSet(notes);
     PatchSet ps2 = psUtil.current(notes);
     serverSideTestRepo
         .branch(notes.getChange().getDest().branch())
-        .update(serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev1)));
+        .update(serverSideTestRepo.getRevWalk().parseCommit(commitId1));
 
     FixInput fix = new FixInput();
-    fix.expectMergedAs = rev1;
+    fix.expectMergedAs = commitId1.name();
     assertProblems(
         notes,
         fix,
-        problem("No patch set found for merged commit " + rev1, FIXED, "Marked change as merged"),
+        problem(
+            "No patch set found for merged commit " + commitId1.name(),
+            FIXED,
+            "Marked change as merged"),
         problem(
             "Expected merge commit "
-                + rev1
+                + commitId1.name()
                 + " corresponds to patch set 1,"
                 + " not the current patch set 2",
             FIXED,
             "Deleted patch set"),
         problem(
             "Expected merge commit "
-                + rev1
+                + commitId1.name()
                 + " corresponds to patch set 1,"
                 + " not the current patch set 2",
             FIXED,
@@ -573,7 +574,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId3);
     assertThat(notes.getChange().isMerged()).isTrue();
     assertThat(psUtil.byChangeAsMap(notes).keySet()).containsExactly(ps2.getId(), psId3);
-    assertThat(psUtil.get(notes, psId3).getRevision().get()).isEqualTo(rev1);
+    assertThat(psUtil.get(notes, psId3).getCommitId()).isEqualTo(commitId1);
   }
 
   @Test
@@ -584,33 +585,33 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     // Create dangling ref so next ID in the database becomes 3.
     PatchSet.Id psId2 = PatchSet.id(notes.getChangeId(), 2);
     RevCommit commit2 = patchSetCommit(psId2);
-    String rev2 = commit2.name();
     serverSideTestRepo.branch(psId2.toRefName()).update(commit2);
 
     notes = incrementPatchSet(notes);
     PatchSet ps3 = psUtil.current(notes);
     assertThat(ps3.getId().get()).isEqualTo(3);
 
-    serverSideTestRepo
-        .branch(notes.getChange().getDest().branch())
-        .update(serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev2)));
+    serverSideTestRepo.branch(notes.getChange().getDest().branch()).update(commit2);
 
     FixInput fix = new FixInput();
-    fix.expectMergedAs = rev2;
+    fix.expectMergedAs = commit2.name();
     assertProblems(
         notes,
         fix,
-        problem("No patch set found for merged commit " + rev2, FIXED, "Marked change as merged"),
+        problem(
+            "No patch set found for merged commit " + commit2.name(),
+            FIXED,
+            "Marked change as merged"),
         problem(
             "Expected merge commit "
-                + rev2
+                + commit2.name()
                 + " corresponds to patch set 2,"
                 + " not the current patch set 3",
             FIXED,
             "Deleted patch set"),
         problem(
             "Expected merge commit "
-                + rev2
+                + commit2.name()
                 + " corresponds to patch set 2,"
                 + " not the current patch set 3",
             FIXED,
@@ -622,7 +623,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     assertThat(notes.getChange().isMerged()).isTrue();
     assertThat(psUtil.byChangeAsMap(notes).keySet())
         .containsExactly(ps1.getId(), ps3.getId(), psId4);
-    assertThat(psUtil.get(notes, psId4).getRevision().get()).isEqualTo(rev2);
+    assertThat(psUtil.get(notes, psId4).getCommitId()).isEqualTo(commit2);
   }
 
   @Test
@@ -633,22 +634,22 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     // Create dangling ref with no patch set.
     PatchSet.Id psId2 = PatchSet.id(notes.getChangeId(), 2);
     RevCommit commit2 = patchSetCommit(psId2);
-    String rev2 = commit2.name();
     serverSideTestRepo.branch(psId2.toRefName()).update(commit2);
 
-    serverSideTestRepo
-        .branch(notes.getChange().getDest().branch())
-        .update(serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev2)));
+    serverSideTestRepo.branch(notes.getChange().getDest().branch()).update(commit2);
 
     FixInput fix = new FixInput();
-    fix.expectMergedAs = rev2;
+    fix.expectMergedAs = commit2.name();
     assertProblems(
         notes,
         fix,
-        problem("No patch set found for merged commit " + rev2, FIXED, "Marked change as merged"),
+        problem(
+            "No patch set found for merged commit " + commit2.name(),
+            FIXED,
+            "Marked change as merged"),
         problem(
             "Expected merge commit "
-                + rev2
+                + commit2.name()
                 + " corresponds to patch set 2,"
                 + " not the current patch set 1",
             FIXED,
@@ -658,7 +659,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId2);
     assertThat(notes.getChange().isMerged()).isTrue();
     assertThat(psUtil.byChangeAsMap(notes).keySet()).containsExactly(ps1.getId(), psId2);
-    assertThat(psUtil.get(notes, psId2).getRevision().get()).isEqualTo(rev2);
+    assertThat(psUtil.get(notes, psId2).getCommitId()).isEqualTo(commit2);
   }
 
   @Test
@@ -666,8 +667,8 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     ChangeNotes notes = insertChange();
     String dest = notes.getChange().getDest().branch();
     RevCommit parent = serverSideTestRepo.branch(dest).commit().message("parent").create();
-    String rev = psUtil.current(notes).getRevision().get();
-    RevCommit commit = serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
+    RevCommit commit =
+        serverSideTestRepo.getRevWalk().parseCommit(psUtil.current(notes).getCommitId());
     serverSideTestRepo.branch(dest).update(commit);
 
     String badId = "I0000000000000000000000000000000000000000";
@@ -702,8 +703,8 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     ChangeNotes notes1 = insertChange();
     PatchSet.Id psId1 = psUtil.current(notes1).getId();
     String dest = notes1.getChange().getDest().branch();
-    String rev = psUtil.current(notes1).getRevision().get();
-    RevCommit commit = serverSideTestRepo.getRevWalk().parseCommit(ObjectId.fromString(rev));
+    RevCommit commit =
+        serverSideTestRepo.getRevWalk().parseCommit(psUtil.current(notes1).getCommitId());
     serverSideTestRepo.branch(dest).update(commit);
 
     ChangeNotes notes2 = insertChange();
@@ -847,9 +848,9 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   }
 
   private ChangeNotes mergeChange(ChangeNotes notes) throws Exception {
-    final ObjectId oldId = getDestRef(notes);
-    final ObjectId newId = ObjectId.fromString(psUtil.current(notes).getRevision().get());
-    final String dest = notes.getChange().getDest().branch();
+    ObjectId oldId = getDestRef(notes);
+    ObjectId newId = psUtil.current(notes).getCommitId();
+    String dest = notes.getChange().getDest().branch();
 
     try (BatchUpdate bu = newUpdate(adminId)) {
       bu.addOp(

javatests/com/google/gerrit/git/ObjectIdsTest.java

@@ -17,6 +17,7 @@ package com.google.gerrit.git;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assert_;
 import static com.google.gerrit.git.ObjectIds.abbreviateName;
+import static org.eclipse.jgit.lib.Constants.OBJECT_ID_STRING_LENGTH;
 
 import java.util.function.Function;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
@@ -99,6 +100,28 @@ public class ObjectIdsTest {
     assertThat(copy.getClass()).isEqualTo(ObjectId.class);
   }
 
+  @Test
+  public void matchesAbbreviation() throws Exception {
+    assertThat(ObjectIds.matchesAbbreviation(null, "")).isFalse();
+    assertThat(ObjectIds.matchesAbbreviation(null, "0")).isFalse();
+    assertThat(ObjectIds.matchesAbbreviation(null, "00000")).isFalse();
+    assertThat(ObjectIds.matchesAbbreviation(null, "not a SHA-1")).isFalse();
+    assertThat(ObjectIds.matchesAbbreviation(null, ID.name())).isFalse();
+
+    assertThat(ObjectIds.matchesAbbreviation(ID, "")).isTrue();
+    for (int i = 1; i <= OBJECT_ID_STRING_LENGTH; i++) {
+      String prefix = ID.name().substring(0, i);
+      assertThat(ObjectIds.matchesAbbreviation(ID, prefix))
+          .named("match %s against %s", ID.name(), prefix)
+          .isTrue();
+    }
+
+    assertThat(ObjectIds.matchesAbbreviation(ID, "1")).isFalse();
+    assertThat(ObjectIds.matchesAbbreviation(ID, "x")).isFalse();
+    assertThat(ObjectIds.matchesAbbreviation(ID, "not a SHA-1")).isFalse();
+    assertThat(ObjectIds.matchesAbbreviation(ID, AMBIGUOUS_BLOB_ID.name())).isFalse();
+  }
+
   @FunctionalInterface
   private interface Func {
     void call() throws Exception;

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

@@ -25,10 +25,10 @@ import com.google.gerrit.proto.testing.SerializedClassSubject;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.protobuf.Parser;
 import java.lang.reflect.Type;
 import java.sql.Timestamp;
+import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Test;
 
 public class PatchSetProtoConverterTest {
@@ -37,7 +37,7 @@ public class PatchSetProtoConverterTest {
   @Test
   public void allValuesConvertedToProto() {
     PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73));
-    patchSet.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+    patchSet.setCommitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
     patchSet.setUploader(Account.id(452));
     patchSet.setCreatedOn(new Timestamp(930349320L));
     patchSet.setGroups(ImmutableList.of("group1, group2"));
@@ -82,7 +82,7 @@ public class PatchSetProtoConverterTest {
   @Test
   public void allValuesConvertedToProtoAndBackAgain() {
     PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73));
-    patchSet.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+    patchSet.setCommitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
     patchSet.setUploader(Account.id(452));
     patchSet.setCreatedOn(new Timestamp(930349320L));
     patchSet.setGroups(ImmutableList.of("group1, group2"));
@@ -127,7 +127,7 @@ public class PatchSetProtoConverterTest {
         .hasFields(
             ImmutableMap.<String, Type>builder()
                 .put("id", PatchSet.Id.class)
-                .put("revision", RevId.class)
+                .put("commitId", ObjectId.class)
                 .put("uploader", Account.Id.class)
                 .put("createdOn", Timestamp.class)
                 .put("groups", String.class)

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

@@ -23,7 +23,6 @@ import com.google.gerrit.reviewdb.client.Account;
 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.server.change.WalkSorter.PatchSetData;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.testing.GerritBaseTests;
@@ -337,7 +336,7 @@ public class WalkSorterTest extends GerritBaseTests {
     Change c = TestChanges.newChange(project, userId);
     ChangeData cd = ChangeData.createForTest(project, c.getId(), 1);
     cd.setChange(c);
-    cd.currentPatchSet().setRevision(new RevId(id.name()));
+    cd.currentPatchSet().setCommitId(id);
     cd.setPatchSets(ImmutableList.of(cd.currentPatchSet()));
     return cd;
   }
@@ -345,7 +344,7 @@ public class WalkSorterTest extends GerritBaseTests {
   private PatchSet addPatchSet(ChangeData cd, ObjectId id) throws Exception {
     TestChanges.incrementPatchSet(cd.change());
     PatchSet ps = new PatchSet(cd.change().currentPatchSetId());
-    ps.setRevision(new RevId(id.name()));
+    ps.setCommitId(id);
     List<PatchSet> patchSets = new ArrayList<>(cd.patchSets());
     patchSets.add(ps);
     cd.setPatchSets(patchSets);

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

@@ -35,7 +35,6 @@ import com.google.gerrit.reviewdb.client.Comment;
 import com.google.gerrit.reviewdb.client.LabelId;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.converter.ChangeMessageProtoConverter;
 import com.google.gerrit.reviewdb.converter.PatchSetApprovalProtoConverter;
 import com.google.gerrit.reviewdb.converter.PatchSetProtoConverter;
@@ -336,14 +335,14 @@ public class ChangeNotesStateTest extends GerritBaseTests {
   public void serializePatchSets() throws Exception {
     PatchSet ps1 = new PatchSet(PatchSet.id(ID, 1));
     ps1.setUploader(Account.id(2000));
-    ps1.setRevision(new RevId("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"));
+    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));
     ps2.setUploader(Account.id(3000));
-    ps2.setRevision(new RevId("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"));
+    ps2.setCommitId(ObjectId.fromString("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"));
     ps2.setCreatedOn(cols.lastUpdatedOn());
     ByteString ps2Bytes = toByteString(ps2, PatchSetProtoConverter.INSTANCE);
     assertThat(ps2Bytes.size()).isEqualTo(66);
@@ -768,7 +767,7 @@ public class ChangeNotesStateTest extends GerritBaseTests {
         .hasFields(
             ImmutableMap.<String, Type>builder()
                 .put("id", PatchSet.Id.class)
-                .put("revision", RevId.class)
+                .put("commitId", ObjectId.class)
                 .put("uploader", Account.Id.class)
                 .put("createdOn", Timestamp.class)
                 .put("groups", String.class)

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

@@ -977,7 +977,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
 
   @Test
   public void commitChangeNotesUnique() throws Exception {
-    // PatchSetId -> RevId must be a one to one mapping
+    // PatchSetId -> ObjectId must be a one to one mapping
     Change c = newChange();
 
     ChangeNotes notes = newNotes(c);
@@ -998,10 +998,10 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
           e,
           ConfigInvalidException.class,
           "Multiple revisions parsed for patch set 1:"
-              + " RevId{"
+              + " "
               + commit.name()
-              + "} and "
-              + ps.getRevision().get());
+              + " and "
+              + ps.getCommitId().name());
     }
   }
 
@@ -1026,8 +1026,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(notes.getChange().getSubject()).isEqualTo("PS2");
     assertThat(notes.getChange().getOriginalSubject()).isEqualTo("Change subject");
     assertThat(notes.getChange().currentPatchSetId()).isEqualTo(ps2.getId());
-    assertThat(ps2.getRevision().get()).isNotEqualTo(ps1.getRevision());
-    assertThat(ps2.getRevision().get()).isEqualTo(commit.name());
+    assertThat(ps2.getCommitId()).isNotEqualTo(ps1.getCommitId());
+    assertThat(ps2.getCommitId()).isEqualTo(commit);
     assertThat(ps2.getUploader()).isEqualTo(otherUser.getAccountId());
     assertThat(ps2.getCreatedOn()).isEqualTo(notes.getChange().getLastUpdatedOn());
 
@@ -1560,7 +1560,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
   }
 
   @Test
-  public void patchLineCommentNotesFormatMultiplePatchSetsSameRevId() throws Exception {
+  public void patchLineCommentNotesFormatMultiplePatchSetsSameCommitId() throws Exception {
     Change c = newChange();
     PatchSet.Id psId1 = c.currentPatchSetId();
     incrementPatchSet(c);

javatests/com/google/gerrit/server/update/BatchUpdateTest.java

@@ -252,9 +252,7 @@ public class BatchUpdateTest extends GerritBaseTests {
 
     try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
       ObjectId commitId =
-          repo.amend(ObjectId.fromString(notes.getCurrentPatchSet().getRevision().get()))
-              .message("PS2")
-              .create();
+          repo.amend(notes.getCurrentPatchSet().getCommitId()).message("PS2").create();
       bu.addOp(
           id,
           patchSetInserterFactory