diff --git a/java/com/google/gerrit/reviewdb/client/PatchSet.java b/java/com/google/gerrit/reviewdb/client/PatchSet.java index 3b7dc2450e..693aa070ed 100644 --- a/java/com/google/gerrit/reviewdb/client/PatchSet.java +++ b/java/com/google/gerrit/reviewdb/client/PatchSet.java @@ -15,18 +15,17 @@ package com.google.gerrit.reviewdb.client; import static com.google.common.base.Preconditions.checkArgument; +import static java.util.Objects.requireNonNull; import com.google.auto.value.AutoValue; import com.google.common.base.Splitter; import com.google.common.primitives.Ints; import com.google.gerrit.common.Nullable; -import com.google.gerrit.git.ObjectIds; import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Objects; -import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ObjectId; /** A single revision of a {@link Change}. */ @@ -157,9 +156,24 @@ public final class PatchSet { } } + /** + * Create a patch set with no commit ID. + * + *

It is illegal to call {@link #getCommitId()} on the returned instance. + * + * @deprecated This method only exists to preserve behavior of one specific codepath in {@code + * PatchScriptFactory}. + * @param id patch set ID. + * @return new patch set. + */ + @Deprecated + public static PatchSet createWithNoCommitId(PatchSet.Id id) { + return new PatchSet(id); + } + protected Id id; - @Nullable protected ObjectId commitId; + protected ObjectId commitId; protected Account.Id uploader; @@ -192,8 +206,14 @@ public final class PatchSet { protected PatchSet() {} - public PatchSet(PatchSet.Id k) { - id = k; + private PatchSet(PatchSet.Id id) { + this.id = requireNonNull(id); + this.commitId = null; + } + + public PatchSet(PatchSet.Id id, ObjectId commitId) { + this.id = requireNonNull(id); + this.commitId = commitId.copy(); } public PatchSet(PatchSet src) { @@ -219,16 +239,13 @@ public final class PatchSet { * *

The commit associated with a patch set is also known as the revision. * - * @return the commit ID. + * @return the commit ID, never null. */ public ObjectId getCommitId() { + requireNonNull(commitId); return commitId; } - public void setCommitId(@Nullable AnyObjectId commitId) { - this.commitId = ObjectIds.copyOrNull(commitId); - } - public Account.Id getUploader() { return uploader; } diff --git a/java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java index 2231051ecb..da844c6888 100644 --- a/java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java @@ -14,6 +14,8 @@ package com.google.gerrit.reviewdb.converter; +import static com.google.common.base.Preconditions.checkArgument; + import com.google.gerrit.proto.Entities; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.PatchSet; @@ -36,10 +38,7 @@ public enum PatchSetProtoConverter implements ProtoConverter> EXACT_COMMIT = exact(ChangeQueryBuilder.FIELD_EXACTCOMMIT).buildRepeatable(ChangeField::getRevisions); - private static Set getRevisions(ChangeData cd) { - Set revisions = new HashSet<>(); - for (PatchSet ps : cd.patchSets()) { - if (ps.getCommitId() != null) { - revisions.add(ps.getCommitId().name()); - } - } - return revisions; + private static ImmutableSet getRevisions(ChangeData cd) { + return cd.patchSets().stream().map(ps -> ps.getCommitId().name()).collect(toImmutableSet()); } /** Tracking id extracted from a footer. */ diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 4d56adba20..170215e3eb 100644 --- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java @@ -204,7 +204,7 @@ public abstract class ChangeEmail extends NotificationEmail { } private void setCommitIdHeader() { - if (patchSet != null && patchSet.getCommitId() != null) { + if (patchSet != null) { setHeader(MailHeader.COMMIT.fieldName(), patchSet.getCommitId().name()); } } diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index d268d5e21a..4588be6593 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -506,9 +506,8 @@ class ChangeNotesParser { "Multiple revisions parsed for patch set %s: %s and %s", psId.get(), patchSets.get(psId).getCommitId().name(), rev.name())); } - PatchSet ps = new PatchSet(psId); + PatchSet ps = new PatchSet(psId, rev); patchSets.put(psId, ps); - ps.setCommitId(rev); ps.setUploader(accountId); ps.setCreatedOn(ts); PendingPatchSetFields pending = pendingPatchSets.remove(psId); diff --git a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index b44fe0f9fd..0919e416e9 100644 --- a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java @@ -135,9 +135,6 @@ public class PatchListCacheImpl implements PatchListCache { throws PatchListNotAvailableException { Project.NameKey project = change.getProject(); ObjectId b = patchSet.getCommitId(); - if (b == null) { - throw new PatchListNotAvailableException("commit ID is null for " + patchSet.getId()); - } Whitespace ws = Whitespace.IGNORE_NONE; if (parentNum != null) { return get(PatchListKey.againstParentNum(parentNum, b, ws), project); diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java index 5d690bcdaf..8fffd693af 100644 --- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java +++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java @@ -194,7 +194,13 @@ public class PatchScriptFactory implements Callable { validatePatchSetId(psb); PatchSet psEntityA = psa != null ? psUtil.get(notes, psa) : null; - PatchSet psEntityB = psb.get() == 0 ? new PatchSet(psb) : psUtil.get(notes, psb); + + // TODO(dborowitz): Shouldn't be creating a PatchSet with no commitId, but the logic depends on + // it somehow in a way that I don't follow, so old behavior is preserved for now. + @SuppressWarnings("deprecation") + PatchSet psEntityB = + psb.get() == 0 ? PatchSet.createWithNoCommitId(psb) : psUtil.get(notes, psb); + if (psEntityA != null || psEntityB != null) { try { permissionBackend.currentUser().change(notes).check(ChangePermission.READ); @@ -262,9 +268,6 @@ public class PatchScriptFactory implements Callable { if (ps.getId().get() == 0) { return getEditRev(); } - if (ps.getCommitId() == null) { - throw new NoSuchChangeException(changeId); - } return ps.getCommitId(); } diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java index 207be967bf..c251c53abc 100644 --- a/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/java/com/google/gerrit/server/query/change/ChangeData.java @@ -222,12 +222,12 @@ public class ChangeData { * @return instance for testing. */ public static ChangeData createForTest( - Project.NameKey project, Change.Id id, int currentPatchSetId) { + Project.NameKey project, Change.Id id, int currentPatchSetId, ObjectId commitId) { ChangeData cd = new ChangeData( null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, project, id, null, null); - cd.currentPatchSet = new PatchSet(PatchSet.id(id, currentPatchSetId)); + cd.currentPatchSet = new PatchSet(PatchSet.id(id, currentPatchSetId), commitId); return cd; } diff --git a/java/com/google/gerrit/server/query/change/CommitPredicate.java b/java/com/google/gerrit/server/query/change/CommitPredicate.java index adb77ae767..2574148615 100644 --- a/java/com/google/gerrit/server/query/change/CommitPredicate.java +++ b/java/com/google/gerrit/server/query/change/CommitPredicate.java @@ -46,8 +46,8 @@ public class CommitPredicate extends ChangeIndexPredicate { protected boolean equals(PatchSet p, String id) { boolean exact = getField() == EXACT_COMMIT; - String rev = p.getCommitId() != null ? p.getCommitId().name() : null; - return (exact && id.equals(rev)) || (!exact && rev != null && rev.startsWith(id)); + String rev = p.getCommitId().name(); + return (exact && id.equals(rev)) || (!exact && rev.startsWith(id)); } @Override diff --git a/java/com/google/gerrit/server/restapi/change/Revisions.java b/java/com/google/gerrit/server/restapi/change/Revisions.java index defdebcb29..dca8b625b1 100644 --- a/java/com/google/gerrit/server/restapi/change/Revisions.java +++ b/java/com/google/gerrit/server/restapi/change/Revisions.java @@ -153,9 +153,8 @@ public class Revisions implements ChildCollection edit = editUtil.byChange(change.getNotes(), change.getUser()); if (edit.isPresent()) { - PatchSet ps = new PatchSet(PatchSet.id(change.getId(), 0)); ObjectId editCommitId = edit.get().getEditCommit(); - ps.setCommitId(edit.get().getEditCommit()); + PatchSet ps = new PatchSet(PatchSet.id(change.getId(), 0), editCommitId); if (commitId == null || editCommitId.equals(commitId)) { return Collections.singletonList(new RevisionResource(change, ps, edit)); } diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index e37960ce21..b21dd5f0d9 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -776,8 +776,8 @@ public class MergeOp implements AutoCloseable { commitStatus.logProblem(changeId, e); continue; } - if (ps == null || ps.getCommitId() == null) { - commitStatus.logProblem(changeId, "Missing patch set or revision on change"); + if (ps == null) { + commitStatus.logProblem(changeId, "Missing patch set on change"); continue; } diff --git a/java/com/google/gerrit/testing/TestChanges.java b/java/com/google/gerrit/testing/TestChanges.java index a83f55b37f..d41877d303 100644 --- a/java/com/google/gerrit/testing/TestChanges.java +++ b/java/com/google/gerrit/testing/TestChanges.java @@ -68,8 +68,7 @@ public class TestChanges { } public static PatchSet newPatchSet(PatchSet.Id id, String revision, Account.Id userId) { - PatchSet ps = new PatchSet(id); - ps.setCommitId(ObjectId.fromString(revision)); + PatchSet ps = new PatchSet(id, ObjectId.fromString(revision)); ps.setUploader(userId); ps.setCreatedOn(TimeUtil.nowTs()); return ps; diff --git a/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java b/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java index a88060c211..c6f2024c76 100644 --- a/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java +++ b/javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java @@ -32,6 +32,7 @@ import com.googlecode.prolog_cafe.lang.Term; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import org.eclipse.jgit.lib.ObjectId; import org.junit.Test; public class PrologRuleEvaluatorIT extends AbstractDaemonTest { @@ -149,7 +150,7 @@ public class PrologRuleEvaluatorIT extends AbstractDaemonTest { } private ChangeData makeChangeData() { - ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1); + ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId()); cd.setChange(TestChanges.newChange(project, admin.id())); return cd; } diff --git a/javatests/com/google/gerrit/reviewdb/converter/PatchSetProtoConverterTest.java b/javatests/com/google/gerrit/reviewdb/converter/PatchSetProtoConverterTest.java index a9d5b852c2..2fd801bff4 100644 --- a/javatests/com/google/gerrit/reviewdb/converter/PatchSetProtoConverterTest.java +++ b/javatests/com/google/gerrit/reviewdb/converter/PatchSetProtoConverterTest.java @@ -36,8 +36,10 @@ public class PatchSetProtoConverterTest { @Test public void allValuesConvertedToProto() { - PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73)); - patchSet.setCommitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); + PatchSet patchSet = + new PatchSet( + PatchSet.id(Change.id(103), 73), + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); patchSet.setUploader(Account.id(452)); patchSet.setCreatedOn(new Timestamp(930349320L)); patchSet.setGroups(ImmutableList.of("group1, group2")); @@ -65,7 +67,10 @@ public class PatchSetProtoConverterTest { @Test public void mandatoryValuesConvertedToProto() { - PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73)); + PatchSet patchSet = + new PatchSet( + PatchSet.id(Change.id(103), 73), + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); Entities.PatchSet proto = patchSetProtoConverter.toProto(patchSet); @@ -75,14 +80,18 @@ public class PatchSetProtoConverterTest { Entities.PatchSet_Id.newBuilder() .setChangeId(Entities.Change_Id.newBuilder().setId(103)) .setId(73)) + .setCommitId( + Entities.ObjectId.newBuilder().setName("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")) .build(); assertThat(proto).isEqualTo(expectedProto); } @Test public void allValuesConvertedToProtoAndBackAgain() { - PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73)); - patchSet.setCommitId(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); + PatchSet patchSet = + new PatchSet( + PatchSet.id(Change.id(103), 73), + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); patchSet.setUploader(Account.id(452)); patchSet.setCreatedOn(new Timestamp(930349320L)); patchSet.setGroups(ImmutableList.of("group1, group2")); @@ -96,7 +105,10 @@ public class PatchSetProtoConverterTest { @Test public void mandatoryValuesConvertedToProtoAndBackAgain() { - PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73)); + PatchSet patchSet = + new PatchSet( + PatchSet.id(Change.id(103), 73), + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); PatchSet convertedPatchSet = patchSetProtoConverter.fromProto(patchSetProtoConverter.toProto(patchSet)); diff --git a/javatests/com/google/gerrit/server/change/WalkSorterTest.java b/javatests/com/google/gerrit/server/change/WalkSorterTest.java index 3de56d41cc..88576497e0 100644 --- a/javatests/com/google/gerrit/server/change/WalkSorterTest.java +++ b/javatests/com/google/gerrit/server/change/WalkSorterTest.java @@ -334,17 +334,15 @@ public class WalkSorterTest extends GerritBaseTests { private ChangeData newChange(TestRepository tr, ObjectId id) throws Exception { Project.NameKey project = tr.getRepository().getDescription().getProject(); Change c = TestChanges.newChange(project, userId); - ChangeData cd = ChangeData.createForTest(project, c.getId(), 1); + ChangeData cd = ChangeData.createForTest(project, c.getId(), 1, id); cd.setChange(c); - cd.currentPatchSet().setCommitId(id); cd.setPatchSets(ImmutableList.of(cd.currentPatchSet())); return cd; } private PatchSet addPatchSet(ChangeData cd, ObjectId id) throws Exception { TestChanges.incrementPatchSet(cd.change()); - PatchSet ps = new PatchSet(cd.change().currentPatchSetId()); - ps.setCommitId(id); + PatchSet ps = new PatchSet(cd.change().currentPatchSetId(), id); List patchSets = new ArrayList<>(cd.patchSets()); patchSets.add(ps); cd.setPatchSets(patchSets); diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java index a3d3a0b906..8b9f5bbecc 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java @@ -333,16 +333,18 @@ public class ChangeNotesStateTest extends GerritBaseTests { @Test public void serializePatchSets() throws Exception { - PatchSet ps1 = new PatchSet(PatchSet.id(ID, 1)); + PatchSet ps1 = + new PatchSet( + PatchSet.id(ID, 1), ObjectId.fromString("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); ps1.setUploader(Account.id(2000)); - ps1.setCommitId(ObjectId.fromString("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); ps1.setCreatedOn(cols.createdOn()); ByteString ps1Bytes = toByteString(ps1, PatchSetProtoConverter.INSTANCE); assertThat(ps1Bytes.size()).isEqualTo(66); - PatchSet ps2 = new PatchSet(PatchSet.id(ID, 2)); + PatchSet ps2 = + new PatchSet( + PatchSet.id(ID, 2), ObjectId.fromString("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")); ps2.setUploader(Account.id(3000)); - ps2.setCommitId(ObjectId.fromString("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")); ps2.setCreatedOn(cols.lastUpdatedOn()); ByteString ps2Bytes = toByteString(ps2, PatchSetProtoConverter.INSTANCE); assertThat(ps2Bytes.size()).isEqualTo(66); diff --git a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java index 8158cdfb89..fa44f21040 100644 --- a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java +++ b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java @@ -23,18 +23,19 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.testing.GerritBaseTests; import com.google.gerrit.testing.TestChanges; +import org.eclipse.jgit.lib.ObjectId; import org.junit.Test; public class ChangeDataTest extends GerritBaseTests { @Test public void setPatchSetsClearsCurrentPatchSet() throws Exception { Project.NameKey project = Project.nameKey("project"); - ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1); + ChangeData cd = ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId()); cd.setChange(TestChanges.newChange(project, Account.id(1000))); PatchSet curr1 = cd.currentPatchSet(); int currId = curr1.getId().get(); - PatchSet ps1 = new PatchSet(PatchSet.id(cd.getId(), currId + 1)); - PatchSet ps2 = new PatchSet(PatchSet.id(cd.getId(), currId + 2)); + PatchSet ps1 = new PatchSet(PatchSet.id(cd.getId(), currId + 1), ObjectId.zeroId()); + PatchSet ps2 = new PatchSet(PatchSet.id(cd.getId(), currId + 2), ObjectId.zeroId()); cd.setPatchSets(ImmutableList.of(ps1, ps2)); PatchSet curr2 = cd.currentPatchSet(); assertThat(curr2).isNotSameInstanceAs(curr1); diff --git a/javatests/com/google/gerrit/server/query/change/RegexPathPredicateTest.java b/javatests/com/google/gerrit/server/query/change/RegexPathPredicateTest.java index 1b2d3bfdf3..45b780e39a 100644 --- a/javatests/com/google/gerrit/server/query/change/RegexPathPredicateTest.java +++ b/javatests/com/google/gerrit/server/query/change/RegexPathPredicateTest.java @@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.testing.GerritBaseTests; import java.util.Arrays; +import org.eclipse.jgit.lib.ObjectId; import org.junit.Test; public class RegexPathPredicateTest extends GerritBaseTests { @@ -83,7 +84,8 @@ public class RegexPathPredicateTest extends GerritBaseTests { private static ChangeData change(String... files) { Arrays.sort(files); - ChangeData cd = ChangeData.createForTest(Project.nameKey("project"), Change.id(1), 1); + ChangeData cd = + ChangeData.createForTest(Project.nameKey("project"), Change.id(1), 1, ObjectId.zeroId()); cd.setCurrentFilePaths(Arrays.asList(files)); return cd; }