opendev/gerrit
Code Issues Proposed changes

Add revertOf to ReviewDb and NoteDb

Browse Source 
This new field will keep track of changes that revert another change.
It will only be populated when a change gets reverted by the API.

Future changes in this series will add the new field to the index,
populate it on reverts and add it to ChangeJson.

Change-Id: I17f71c6db554f4e31b2451416379107abf055b5b
This commit is contained in:
Patrick Hiesel 2017-07-13 10:58:22 +02:00
parent fbfd17b8b3
commit 0a6eb6a52b
12 changed files with 143 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
View File

@ -524,6 +524,10 @@ public final class Change {
@Column(id = 22)
protected boolean reviewStarted;
/** References a change that this change reverts. */
@Column(id = 23, notNull = false)
protected Id revertOf;
/** @see com.google.gerrit.server.notedb.NoteDbChangeState */
@Column(id = 101, notNull = false, length = Integer.MAX_VALUE)
protected String noteDbState;
@ -564,6 +568,7 @@ public final class Change {
workInProgress = other.workInProgress;
reviewStarted = other.reviewStarted;
noteDbState = other.noteDbState;
revertOf = other.revertOf;
}
/** Legacy 32 bit integer identity for a change. */
@ -733,6 +738,14 @@ public final class Change {
this.reviewStarted = reviewStarted;
}
public void setRevertOf(Id revertOf) {
this.revertOf = revertOf;
}
public Id getRevertOf() {
return this.revertOf;
}
public String getNoteDbState() {
return noteDbState;
}

6
gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
View File

@ -57,6 +57,7 @@ public abstract class AbstractChangeUpdate {
protected PatchSet.Id psId;
private ObjectId result;
protected boolean rootOnly;
protected AbstractChangeUpdate(
Config cfg,
@ -190,6 +191,11 @@ public abstract class AbstractChangeUpdate {
/** Whether no updates have been done. */
public abstract boolean isEmpty();
/** Wether this update can only be a root commit. */
public boolean isRootOnly() {
return rootOnly;
}
/**
* @return the NameKey for the project where the update will be stored, which is not necessarily
* the same as the change's project.

3
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
View File

@ -233,7 +233,8 @@ public class ChangeBundle {
// last time this file was updated.
checkColumns(Change.Id.class, 1);
checkColumns(Change.class, 1, 2, 3, 4, 5, 7, 8, 10, 12, 13, 14, 17, 18, 19, 20, 21, 22, 101);
checkColumns(
Change.class, 1, 2, 3, 4, 5, 7, 8, 10, 12, 13, 14, 17, 18, 19, 20, 21, 22, 23, 101);
checkColumns(ChangeMessage.Key.class, 1, 2);
checkColumns(ChangeMessage.class, 1, 2, 3, 4, 5, 6, 7);
checkColumns(PatchSet.Id.class, 1, 2);

1
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
View File

@ -83,6 +83,7 @@ public class ChangeNoteUtil {
public static final FooterKey FOOTER_TOPIC = new FooterKey("Topic");
public static final FooterKey FOOTER_TAG = new FooterKey("Tag");
public static final FooterKey FOOTER_WORK_IN_PROGRESS = new FooterKey("Work-in-progress");
public static final FooterKey FOOTER_REVERT_OF = new FooterKey("Revert-of");
private static final String AUTHOR = "Author";
private static final String BASE_PATCH_SET = "Base-for-patch-set";

4
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@ -623,6 +623,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return state.isWorkInProgress();
}
public Change.Id getRevertOf() {
return state.revertOf();
}
public boolean hasReviewStarted() {
return state.hasReviewStarted();
}

21
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@ -27,6 +27,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET_DE
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PRIVATE;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_READ_ONLY_UNTIL;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REAL_USER;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REVERT_OF;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBJECT;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMISSION_ID;
@ -169,6 +170,7 @@ class ChangeNotesParser {
private Boolean hasReviewStarted;
private ReviewerSet pendingReviewers;
private ReviewerByEmailSet pendingReviewersByEmail;
private Change.Id revertOf;
ChangeNotesParser(
Change.Id changeId,
@ -267,7 +269,8 @@ class ChangeNotesParser {
readOnlyUntil,
isPrivate,
workInProgress,
hasReviewStarted);
hasReviewStarted,
revertOf);
}
private PatchSet.Id buildCurrentPatchSetId() {
@ -415,6 +418,10 @@ class ChangeNotesParser {
parseIsPrivate(commit);
}
if (revertOf == null) {
revertOf = parseRevertOf(commit);
}
previousWorkInProgressFooter = null;
parseWorkInProgress(commit);
@ -1022,6 +1029,18 @@ class ChangeNotesParser {
throw invalidFooter(FOOTER_WORK_IN_PROGRESS, raw);
}
private Change.Id parseRevertOf(ChangeNotesCommit commit) throws ConfigInvalidException {
String footer = parseOneFooter(commit, FOOTER_REVERT_OF);
if (footer == null) {
return null;
}
Integer revertOf = Ints.tryParse(footer);
if (revertOf == null) {
throw invalidFooter(FOOTER_REVERT_OF, footer);
}
return new Change.Id(revertOf);
}
private void pruneReviewers() {
Iterator<Table.Cell<Account.Id, ReviewerStateInternal, Timestamp>> rit =
reviewers.cellSet().iterator();

19
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java
View File

@ -78,7 +78,8 @@ public abstract class ChangeNotesState {
null,
null,
null,
true);
true,
null);
}
static ChangeNotesState create(
@ -113,7 +114,8 @@ public abstract class ChangeNotesState {
@Nullable Timestamp readOnlyUntil,
@Nullable Boolean isPrivate,
@Nullable Boolean workInProgress,
boolean hasReviewStarted) {
boolean hasReviewStarted,
@Nullable Change.Id revertOf) {
if (hashtags == null) {
hashtags = ImmutableSet.of();
}
@ -135,7 +137,8 @@ public abstract class ChangeNotesState {
status,
isPrivate,
workInProgress,
hasReviewStarted),
hasReviewStarted,
revertOf),
ImmutableSet.copyOf(pastAssignees),
ImmutableSet.copyOf(hashtags),
ImmutableList.copyOf(patchSets.entrySet()),
@ -153,7 +156,8 @@ public abstract class ChangeNotesState {
readOnlyUntil,
isPrivate,
workInProgress,
hasReviewStarted);
hasReviewStarted,
revertOf);
}
/**
@ -205,6 +209,9 @@ public abstract class ChangeNotesState {
@Nullable
abstract Boolean hasReviewStarted();
@Nullable
abstract Change.Id revertOf();
}
// Only null if NoteDb is disabled.
@ -258,6 +265,9 @@ public abstract class ChangeNotesState {
@Nullable
abstract Boolean hasReviewStarted();
@Nullable
abstract Change.Id revertOf();
Change newChange(Project.NameKey project) {
ChangeColumns c = checkNotNull(columns(), "columns are required");
Change change =
@ -318,6 +328,7 @@ public abstract class ChangeNotesState {
change.setPrivate(c.isPrivate() == null ? false : c.isPrivate());
change.setWorkInProgress(c.isWorkInProgress() == null ? false : c.isWorkInProgress());
change.setReviewStarted(c.hasReviewStarted() == null ? false : c.hasReviewStarted());
change.setRevertOf(c.revertOf());
if (!patchSets().isEmpty()) {
change.setCurrentPatchSet(c.currentPatchSetId(), c.subject(), c.originalSubject());

16
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@ -32,6 +32,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET_DE
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PRIVATE;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_READ_ONLY_UNTIL;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REAL_USER;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REVERT_OF;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBJECT;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMISSION_ID;
@ -156,6 +157,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private Timestamp readOnlyUntil;
private Boolean isPrivate;
private Boolean workInProgress;
private Integer revertOf;
private ChangeDraftUpdate draftUpdate;
private RobotCommentUpdate robotCommentUpdate;
@ -512,6 +514,13 @@ public class ChangeUpdate extends AbstractChangeUpdate {
this.groups = groups;
}
public void setRevertOf(int revertOf) {
int ownId = getChange().getId().get();
checkArgument(ownId != revertOf, "A change cannot revert itself");
this.revertOf = revertOf;
rootOnly = true;
}
/** @return the tree id for the updated tree */
private ObjectId storeRevisionNotes(RevWalk rw, ObjectInserter inserter, ObjectId curr)
throws ConfigInvalidException, OrmException, IOException {
@ -755,6 +764,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addFooter(msg, FOOTER_WORK_IN_PROGRESS, workInProgress);
}
if (revertOf != null) {
addFooter(msg, FOOTER_REVERT_OF, revertOf);
}
cb.setMessage(msg.toString());
try {
ObjectId treeId = storeRevisionNotes(rw, ins, curr);
@ -804,7 +817,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
&& !currentPatchSet
&& readOnlyUntil == null
&& isPrivate == null
&& workInProgress == null;
&& workInProgress == null
&& revertOf == null;
}
ChangeDraftUpdate getDraftUpdate() {

3
gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
View File

@ -688,6 +688,9 @@ public class NoteDbUpdateManager implements AutoCloseable {
ObjectId curr = old;
for (U u : updates) {
if (u.isRootOnly() && !old.equals(ObjectId.zeroId())) {
throw new OrmException("Given ChangeUpdate is only allowed on initial commit");
}
ObjectId next = u.apply(or.rw, or.tempIns, curr);
if (next == null) {
continue;

2
gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
View File

@ -35,7 +35,7 @@ import java.util.concurrent.TimeUnit;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_155> C = Schema_155.class;
public static final Class<Schema_156> C = Schema_156.class;
public static int getBinaryVersion() {
return guessVersion(C);

26
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_156.java Normal file
View File

@ -0,0 +1,26 @@
// Copyright (C) 2017 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.server.schema;
import com.google.inject.Inject;
import com.google.inject.Provider;
/** Add revertOf field to change. */
public class Schema_156 extends SchemaVersion {
@Inject
Schema_156(Provider<Schema_155> prior) {
super(prior);
}
}

37
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@ -3494,6 +3494,43 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getPendingReviewersByEmail().asTable()).isEmpty();
}
@Test
public void revertOfIsNullByDefault() throws Exception {
Change c = newChange();
ChangeNotes notes = newNotes(c);
assertThat(notes.getRevertOf()).isNull();
}
@Test
public void setRevertOfPersistsValue() throws Exception {
Change changeToRevert = newChange();
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
ChangeUpdate update = newUpdate(c, changeOwner);
update.setChangeId(c.getKey().get());
update.setRevertOf(changeToRevert.getId().get());
update.commit();
assertThat(newNotes(c).getRevertOf()).isEqualTo(changeToRevert.getId());
}
@Test
public void setRevertOfToCurrentChangeFails() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
exception.expect(IllegalArgumentException.class);
exception.expectMessage("A change cannot revert itself");
update.setRevertOf(c.getId().get());
}
@Test
public void setRevertOfOnChildCommitFails() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
exception.expect(OrmException.class);
exception.expectMessage("Given ChangeUpdate is only allowed on initial commit");
update.setRevertOf(newChange().getId().get());
update.commit();
}
private boolean testJson() {
return noteUtil.getWriteJson();
}