opendev/gerrit
Code Issues Proposed changes

Add the work_in_progress option to the revert and revertSubmission endpoints

Browse Source 
Sometimes bots want to validate whether a change break a build/tests by
creating a revert of a change and validating it. Unless the revert fixes
the breakage, it won't need to notify the author of the original change
and/or get reviewed. This notification can cause panic to the former
author.

This change adds a work in progress (WIP) field to the RevertInput that
can be used for testing purposes where reviewers get notified only if we
determine we want to move forward with the revert.

CherryPickChange was also modified with a WIP parameter. Cherry picking
is used by revert submission (the first revert is created normally on
top of the branch tip, subsequent reverts are cherry picked). The
modifications done to CherryPickChange in this change are only used by
RevertSubmission and do not affect the serving path of the cherry pick
endpoint.

Bug: Issue 13357
Change-Id: Ida96151e1bed8d62f19510681154f8940df8e58d
This commit is contained in:
Youssef Elghareeb
2020-09-03 21:56:30 +02:00
parent a59037bda9
commit fd02daeb35
6 changed files with 128 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
Documentation/rest-api-changes.txt
View File

@@ -7393,6 +7393,10 @@ Name of the topic for the revert change. If not set, the default for Revert
endpoint is the topic of the change being reverted, and the default for the
RevertSubmission endpoint is `revert-{submission_id}-{timestamp.now}`.
Topic can't contain quotation marks.
|`work_in_progress` |optional|
When present, change is marked as Work In Progress. This will also override
the notify value to `OWNER`. +
If not set, the default is false.
|=============================
[[revert-submission-info]]

10
java/com/google/gerrit/extensions/api/changes/RevertInput.java
View File

@@ -17,6 +17,10 @@ package com.google.gerrit.extensions.api.changes;
import com.google.gerrit.extensions.restapi.DefaultInput;
import java.util.Map;
/**
* Input passed to {@code POST /changes/[change-id]/revert} and {@code POST
* /changes/[change-id]/revert_submission}
*/
public class RevertInput {
@DefaultInput public String message;
@@ -26,4 +30,10 @@ public class RevertInput {
public Map<RecipientType, NotifyInfo> notifyDetails;
public String topic;
/**
* Mark the change as work-in-progress. This will also override the {@link #notify} value to
* {@link NotifyHandling#OWNER}
*/
public boolean workInProgress;
}

4
java/com/google/gerrit/server/git/CommitUtil.java
View File

@@ -265,6 +265,9 @@ public class CommitUtil {
RevCommit revertCommit = revWalk.parseCommit(revertCommitId);
Change changeToRevert = notes.getChange();
Change.Id changeId = Change.id(seq.nextChangeId());
if (input.workInProgress) {
input.notify = NotifyHandling.OWNER;
}
NotifyResolver.Result notify =
notifyResolver.resolve(firstNonNull(input.notify, NotifyHandling.ALL), input.notifyDetails);
@@ -284,6 +287,7 @@ public class CommitUtil {
ccs.remove(user.getAccountId());
ins.setReviewersAndCcs(reviewers, ccs);
ins.setRevertOf(notes.getChangeId());
ins.setWorkInProgress(input.workInProgress);
try (BatchUpdate bu = updateFactory.create(notes.getProjectName(), user, ts)) {
bu.setRepository(git, revWalk, oi);

33
java/com/google/gerrit/server/restapi/change/CherryPickChange.java
View File

@@ -173,6 +173,7 @@ public class CherryPickChange {
TimeUtil.nowTs(),
null,
null,
null,
null);
}
@@ -204,7 +205,7 @@ public class CherryPickChange {
throws IOException, InvalidChangeOperationException, UpdateException, RestApiException,
ConfigInvalidException, NoSuchProjectException {
return cherryPick(
sourceChange, project, sourceCommit, input, dest, TimeUtil.nowTs(), null, null, null);
sourceChange, project, sourceCommit, input, dest, TimeUtil.nowTs(), null, null, null, null);
}
/**
@@ -245,7 +246,8 @@ public class CherryPickChange {
Timestamp timestamp,
@Nullable Change.Id revertedChange,
@Nullable ObjectId changeIdForNewChange,
@Nullable Change.Id idForNewChange)
@Nullable Change.Id idForNewChange,
@Nullable Boolean workInProgress)
throws IOException, InvalidChangeOperationException, UpdateException, RestApiException,
ConfigInvalidException, NoSuchProjectException {
@@ -365,7 +367,8 @@ public class CherryPickChange {
destChanges.get(0).notes(),
cherryPickCommit,
sourceChange.currentPatchSetId(),
newTopic);
newTopic,
workInProgress);
} else {
// Change key not found on destination branch. We can create a new
// change.
@@ -380,7 +383,8 @@ public class CherryPickChange {
sourceCommit,
input,
revertedChange,
idForNewChange);
idForNewChange,
workInProgress);
}
bu.execute();
return Result.create(changeId, cherryPickCommit.getFilesWithGitConflicts());
@@ -448,13 +452,17 @@ public class CherryPickChange {
ChangeNotes destNotes,
CodeReviewCommit cherryPickCommit,
PatchSet.Id sourcePatchSetId,
String topic)
String topic,
@Nullable Boolean workInProgress)
throws IOException {
Change destChange = destNotes.getChange();
PatchSet.Id psId = ChangeUtil.nextPatchSetId(git, destChange.currentPatchSetId());
PatchSetInserter inserter = patchSetInserterFactory.create(destNotes, psId, cherryPickCommit);
inserter.setMessage("Uploaded patch set " + inserter.getPatchSetId().get() + ".");
inserter.setTopic(topic);
if (workInProgress != null) {
inserter.setWorkInProgress(workInProgress);
}
bu.addOp(destChange.getId(), inserter);
if (destChange.getCherryPickOf() == null
|| !destChange.getCherryPickOf().equals(sourcePatchSetId)) {
@@ -474,11 +482,19 @@ public class CherryPickChange {
@Nullable ObjectId sourceCommit,
CherryPickInput input,
@Nullable Change.Id revertOf,
@Nullable Change.Id idForNewChange)
@Nullable Change.Id idForNewChange,
@Nullable Boolean workInProgress)
throws IOException, InvalidChangeOperationException {
Change.Id changeId = idForNewChange != null ? idForNewChange : Change.id(seq.nextChangeId());
ChangeInserter ins = changeInserterFactory.create(changeId, cherryPickCommit, refName);
ins.setRevertOf(revertOf);
if (workInProgress != null) {
ins.setWorkInProgress(workInProgress);
} else {
ins.setWorkInProgress(
(sourceChange != null && sourceChange.isWorkInProgress())
|| !cherryPickCommit.getFilesWithGitConflicts().isEmpty());
}
BranchNameKey sourceBranch = sourceChange == null ? null : sourceChange.getDest();
PatchSet.Id sourcePatchSetId = sourceChange == null ? null : sourceChange.currentPatchSetId();
ins.setMessage(
@@ -488,10 +504,7 @@ public class CherryPickChange {
: "Uploaded patch set 1.") // For revert commits, the message should not include
// cherry-pick information.
.setTopic(topic)
.setCherryPickOf(sourcePatchSetId)
.setWorkInProgress(
(sourceChange != null && sourceChange.isWorkInProgress())
|| !cherryPickCommit.getFilesWithGitConflicts().isEmpty());
.setCherryPickOf(sourcePatchSetId);
if (input.keepReviewers && sourceChange != null) {
ReviewerSet reviewerSet =
approvalsUtil.getReviewers(changeNotesFactory.createChecked(sourceChange));

18
java/com/google/gerrit/server/restapi/change/RevertSubmission.java
View File

@@ -203,6 +203,7 @@ public class RevertSubmission
checkPermissionsForAllChanges(changeResource, changeDatas);
input.topic = createTopic(input.topic, submissionId);
return Response.ok(revertSubmission(changeDatas, input));
}
@@ -258,6 +259,9 @@ public class RevertSubmission
cherryPickInput.base = null;
Project.NameKey project = projectAndBranch.project();
cherryPickInput.destination = projectAndBranch.branch();
if (revertInput.workInProgress) {
cherryPickInput.notify = NotifyHandling.OWNER;
}
Collection<ChangeData> changesInProjectAndBranch =
changesPerProjectAndBranch.get(projectAndBranch);
@@ -332,7 +336,11 @@ public class RevertSubmission
bu.addOp(
changeNotes.getChange().getId(),
new CreateCherryPickOp(
revCommitId, generatedChangeId, cherryPickRevertChangeId, timestamp));
revCommitId,
generatedChangeId,
cherryPickRevertChangeId,
timestamp,
revertInput.workInProgress));
bu.addOp(changeNotes.getChange().getId(), new PostRevertedMessageOp(generatedChangeId));
bu.addOp(
cherryPickRevertChangeId,
@@ -549,16 +557,19 @@ public class RevertSubmission
private final ObjectId computedChangeId;
private final Change.Id cherryPickRevertChangeId;
private final Timestamp timestamp;
private final boolean workInProgress;
CreateCherryPickOp(
ObjectId revCommitId,
ObjectId computedChangeId,
Change.Id cherryPickRevertChangeId,
Timestamp timestamp) {
Timestamp timestamp,
Boolean workInProgress) {
this.revCommitId = revCommitId;
this.computedChangeId = computedChangeId;
this.cherryPickRevertChangeId = cherryPickRevertChangeId;
this.timestamp = timestamp;
this.workInProgress = workInProgress;
}
@Override
@@ -575,7 +586,8 @@ public class RevertSubmission
timestamp,
change.getId(),
computedChangeId,
cherryPickRevertChangeId);
cherryPickRevertChangeId,
workInProgress);
// save the commit as base for next cherryPick of that branch
cherryPickInput.base =
changeNotesFactory

72
javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
View File

@@ -249,6 +249,17 @@ public class RevertIT extends AbstractDaemonTest {
assertThat(revertChange.revertOf).isEqualTo(gApi.changes().id(r.getChangeId()).get()._number);
}
@Test
public void revertChangeWithWip() throws Exception {
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
RevertInput in = createWipRevertInput();
ChangeInfo revertChange = gApi.changes().id(r.getChangeId()).revert(in).get();
assertThat(revertChange.workInProgress).isTrue();
}
@Test
public void revertWithDefaultTopic() throws Exception {
PushOneCommit.Result result = createChange();
@@ -320,6 +331,18 @@ public class RevertIT extends AbstractDaemonTest {
assertThat(sender.getMessages(r.getChangeId(), "revert")).hasSize(1);
}
@Test
public void revertNotificationsSupressedOnWip() throws Exception {
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).addReviewer(user.email());
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
sender.clear();
gApi.changes().id(r.getChangeId()).revert(createWipRevertInput()).get();
assertThat(sender.getMessages()).isEmpty();
}
@Test
public void suppressRevertNotifications() throws Exception {
PushOneCommit.Result r = createChange();
@@ -658,6 +681,49 @@ public class RevertIT extends AbstractDaemonTest {
assertThat(sender.getMessages(secondResult, "revert")).hasSize(1);
}
@Test
public void revertSubmissionWipNotificationsAreSupressed() throws Exception {
String changeId1 = createChange("first change", "a.txt", "message").getChangeId();
approve(changeId1);
gApi.changes().id(changeId1).addReviewer(user.email());
String changeId2 = createChange("second change", "b.txt", "other").getChangeId();
approve(changeId2);
gApi.changes().id(changeId2).addReviewer(user.email());
gApi.changes().id(changeId2).current().submit();
sender.clear();
RevertInput revertInput = createWipRevertInput();
// Setting the Notifications to ALL will be overridden because the WIP flag overrides the
// notifications to OWNER
revertInput.notify = NotifyHandling.ALL;
gApi.changes().id(changeId2).revertSubmission(revertInput);
assertThat(sender.getMessages()).isEmpty();
}
@Test
public void revertSubmissionWipMarksAllChangesAsWip() throws Exception {
String changeId1 = createChange("first change", "a.txt", "message").getChangeId();
approve(changeId1);
gApi.changes().id(changeId1).addReviewer(user.email());
String changeId2 = createChange("second change", "b.txt", "other").getChangeId();
approve(changeId2);
gApi.changes().id(changeId2).addReviewer(user.email());
gApi.changes().id(changeId2).current().submit();
sender.clear();
RevertInput revertInput = createWipRevertInput();
RevertSubmissionInfo revertSubmissionInfo =
gApi.changes().id(changeId2).revertSubmission(revertInput);
assertThat(revertSubmissionInfo.revertChanges.stream().allMatch(r -> r.workInProgress))
.isTrue();
}
@Test
public void revertSubmissionIdenticalTreeIsAllowed() throws Exception {
String unrelatedChange = createChange("change1", "a.txt", "message").getChangeId();
@@ -1294,4 +1360,10 @@ public class RevertIT extends AbstractDaemonTest {
}
return results;
}
private RevertInput createWipRevertInput() {
RevertInput input = new RevertInput();
input.workInProgress = true;
return input;
}
}