From 59b42fbf18b2238e23b196f9fe2aca6198fe8ed4 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 23 Apr 2019 12:46:53 -0700 Subject: [PATCH] PatchLineComment: Replace RevId with ObjectId This change does not affect Comment's field names or types, since that type is used in the NoteDb JSON storage format, so it needs to remain backwards compatible. It's possible we could change this and then use a custom type adapter in Gson, but that may be more effort than it's worth. See Iff5644e2 context on removing RevId usages. Change-Id: If054aca4867f5868e108616e7524b15f5c52de55 --- .../gerrit/reviewdb/client/Comment.java | 10 +++++--- .../reviewdb/client/PatchLineComment.java | 24 ++++++++++--------- .../server/notedb/LegacyChangeNoteRead.java | 11 +++++---- .../server/notedb/ChangeNotesStateTest.java | 4 ++-- 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/java/com/google/gerrit/reviewdb/client/Comment.java b/java/com/google/gerrit/reviewdb/client/Comment.java index 3ef56aecdd..816e635aa9 100644 --- a/java/com/google/gerrit/reviewdb/client/Comment.java +++ b/java/com/google/gerrit/reviewdb/client/Comment.java @@ -16,9 +16,11 @@ package com.google.gerrit.reviewdb.client; import com.google.common.base.MoreObjects; import com.google.common.base.MoreObjects.ToStringHelper; +import com.google.gerrit.common.Nullable; import java.sql.Timestamp; import java.util.Comparator; import java.util.Objects; +import org.eclipse.jgit.lib.AnyObjectId; /** * This class represents inline comments in NoteDb. This means it determines the JSON format for @@ -192,7 +194,9 @@ public class Comment { public Range range; public String tag; - // Hex commit SHA1 of the commit of the patchset to which this comment applies. + // Hex commit SHA1 of the commit of the patchset to which this comment applies. Other classes call + // this "commitId", but this class uses the old ReviewDb term "revId", and this field name is + // serialized into JSON in NoteDb, so it can't easily be changed. public String revId; public String serverId; public boolean unresolved; @@ -250,8 +254,8 @@ public class Comment { this.range = range != null ? range.asCommentRange() : null; } - public void setRevId(RevId revId) { - this.revId = revId != null ? revId.get() : null; + public void setRevId(@Nullable AnyObjectId revId) { + this.revId = revId != null ? revId.name() : null; } public void setRealAuthor(Account.Id id) { diff --git a/java/com/google/gerrit/reviewdb/client/PatchLineComment.java b/java/com/google/gerrit/reviewdb/client/PatchLineComment.java index 03359ba155..1e1c9bea11 100644 --- a/java/com/google/gerrit/reviewdb/client/PatchLineComment.java +++ b/java/com/google/gerrit/reviewdb/client/PatchLineComment.java @@ -18,6 +18,8 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.client.Comment.Range; import java.sql.Timestamp; import java.util.Objects; +import org.eclipse.jgit.lib.AnyObjectId; +import org.eclipse.jgit.lib.ObjectId; /** * A comment left by a user on a specific line of a {@link Patch}. @@ -68,7 +70,7 @@ public final class PatchLineComment { plc.setRange(new CommentRange(r.startLine, r.startChar, r.endLine, r.endChar)); } plc.setTag(c.tag); - plc.setRevId(new RevId(c.revId)); + plc.setCommitId(ObjectId.fromString(c.revId)); plc.setStatus(status); plc.setRealAuthor(c.getRealAuthor().getId()); plc.setUnresolved(c.unresolved); @@ -110,8 +112,8 @@ public final class PatchLineComment { /** True if this comment requires further action. */ protected boolean unresolved; - /** The RevId for the commit to which this comment is referring. */ - protected RevId revId; + /** The ID of the commit to which this comment is referring. */ + protected ObjectId commitId; protected PatchLineComment() {} @@ -137,7 +139,7 @@ public final class PatchLineComment { side = o.side; message = o.message; parentUuid = o.parentUuid; - revId = o.revId; + commitId = o.commitId; if (o.range != null) { range = new CommentRange( @@ -232,12 +234,12 @@ public final class PatchLineComment { return range; } - public void setRevId(RevId rev) { - revId = rev; + public void setCommitId(AnyObjectId commitId) { + this.commitId = commitId.copy(); } - public RevId getRevId() { - return revId; + public ObjectId getCommitId() { + return commitId; } public void setTag(String tag) { @@ -266,7 +268,7 @@ public final class PatchLineComment { message, serverId, unresolved); - c.setRevId(revId); + c.setRevId(commitId); c.setRange(range); c.lineNbr = lineNbr; c.parentUuid = parentUuid; @@ -289,7 +291,7 @@ public final class PatchLineComment { && Objects.equals(message, c.getMessage()) && Objects.equals(parentUuid, c.getParentUuid()) && Objects.equals(range, c.getRange()) - && Objects.equals(revId, c.getRevId()) + && Objects.equals(commitId, c.getCommitId()) && Objects.equals(tag, c.getTag()) && Objects.equals(unresolved, c.getUnresolved()); } @@ -316,7 +318,7 @@ public final class PatchLineComment { builder.append("message=").append(Objects.toString(message, "")).append(','); builder.append("parentUuid=").append(Objects.toString(parentUuid, "")).append(','); builder.append("range=").append(Objects.toString(range, "")).append(','); - builder.append("revId=").append(revId != null ? revId.get() : "").append(','); + builder.append("revId=").append(commitId != null ? commitId.name() : "").append(','); builder.append("tag=").append(Objects.toString(tag, "")).append(','); builder.append("unresolved=").append(unresolved); builder.append('}'); diff --git a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java index 7e53630ec0..b711b3b517 100644 --- a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java +++ b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java @@ -23,7 +23,6 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Comment; import com.google.gerrit.reviewdb.client.CommentRange; import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.config.GerritServerId; import com.google.inject.Inject; import java.sql.Timestamp; @@ -34,6 +33,7 @@ import java.util.List; import java.util.Locale; import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.util.GitDateParser; import org.eclipse.jgit.util.MutableInteger; @@ -77,7 +77,8 @@ public class LegacyChangeNoteRead { byte[] bpsb = ChangeNoteUtil.BASE_PATCH_SET.getBytes(UTF_8); byte[] bpn = ChangeNoteUtil.PARENT_NUMBER.getBytes(UTF_8); - RevId revId = new RevId(parseStringField(note, p, changeId, ChangeNoteUtil.REVISION)); + ObjectId commitId = + ObjectId.fromString(parseStringField(note, p, changeId, ChangeNoteUtil.REVISION)); String fileName = null; PatchSet.Id psId = null; boolean isForBase = false; @@ -105,7 +106,7 @@ public class LegacyChangeNoteRead { ChangeNoteUtil.BASE_PATCH_SET); } - Comment c = parseComment(note, p, fileName, psId, revId, isForBase, parentNumber); + Comment c = parseComment(note, p, fileName, psId, commitId, isForBase, parentNumber); fileName = c.key.filename; if (!seen.add(c.key)) { throw parseException(changeId, "multiple comments for %s in note", c.key); @@ -120,7 +121,7 @@ public class LegacyChangeNoteRead { MutableInteger curr, String currentFileName, PatchSet.Id psId, - RevId revId, + ObjectId commitId, boolean isForBase, Integer parentNumber) throws ConfigInvalidException { @@ -189,7 +190,7 @@ public class LegacyChangeNoteRead { c.lineNbr = range.getEndLine(); c.parentUuid = parentUUID; c.tag = tag; - c.setRevId(revId); + c.setRevId(commitId); if (raId != null) { c.setRealAuthor(raId); } diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java index ebb4691ba8..77be394451 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java @@ -663,7 +663,7 @@ public class ChangeNotesStateTest extends GerritBaseTests { "message 1", "serverId", false); - c1.setRevId(new RevId("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); + c1.setRevId(ObjectId.fromString("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); String c1Json = Serializer.GSON.toJson(c1); Comment c2 = @@ -675,7 +675,7 @@ public class ChangeNotesStateTest extends GerritBaseTests { "message 2", "serverId", true); - c2.setRevId(new RevId("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")); + c2.setRevId(ObjectId.fromString("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")); String c2Json = Serializer.GSON.toJson(c2); assertRoundTrip(