Support Revert Submission with rebasing on previous reverts

This is another step in the implementation of Revert Submission
initiated in change I7188c0d52.

Now instead of just reverting all changes normally, we revert the
changes and then rebase them on top of the most recent revert change,
when needed.
These are the steps we take:

1. Loop over all of the projects in that submission, and for each
project we loop over all of the branches in the submission.
For each project + branch, the changes are reverted in topological
ordering.
This means if A is a parent of B, we ensure that B is reverted before A.
For each change we do either Option A or Option B:

Option A:
If this is the first change that should be reverted in that project
+ branch, we revert the change normally, and remember the created revert
change so that it can be used as base when we continue with the next
change. Then, continue to the next change in the list.

Option B:
Otherwise, if this change is not the first one that should be reverted
in that project + branch, it is reverted on top of the last revert
change which was created for the project / branch That means we now:
1. Revert the change, but only by creating a commit, without creating
a change. This is useful because we don't want to create an unnecessary
change that should be deleted after the relevant cherry-pick.
2. Do the following as a BatchUpdate:
2.1. Create the cherry-pick of the revert-commit. The first
cherry-pick of that project + branch will be done on top of the most
recent change of that submission, and then it will stack on top of
the latest revert created. E.g, the second revert will be on top of the
first revert. Special case with merge commits described below.
2.2. Post the Revert message on the original change that points to the
newly created revert change with PostRevertedMessageOp.
2.3. Notify whoever subscribed to notifications on the reverted change
with NotifyOp.

And then finally:
As a response return the list of revert changes as ChangeInfos

Special case - merge commit:
A special case that involves merge commits can happen if the submission
contains multiple changes for the same project and branch which are not
part of the same change series, and if the submit strategy is
"MERGE_ALWAYS" or "MERGE_IF_NECESSARY".
In case of merge commit, the revert is done on top of the merge commit.
There is a getBase() method which purpose is to find the base that the
first revert of a project + branch should be based on.
The function goes over all commits (of the branches that were updated by
the submission) in topological order by BFS (Breadth first search) until
it finds a commit that is part of the submission or finds a merge commit
that only has parent commits that belong to the changes that are part of
the submission (or other merge commits that satisfy that condition).
If it finds a commit that is part of the submission, we just return it,
since it's the same as the one found by WalkSorter earlier to be the
first one topologically.
Otherwise, we return the first merge commit that satisfy that condition.

Examples:
1. Submission that contains only a single change will have the same
behavior as performing a regular revert. The topic will also be set to
'Revert-<submission-ID>-<RandomString>.
2. The submission contains parent changes:
The changes are reverted in topological order, starting from the most
recent change. The first revert is regular revert and its parent change
is reverted on to of the revert change.
Afterwards, all other reverts would have as parent the last created
revert change (E.g A->B->C would have C's revert on top of C, and then B's
revert would be on top of C's revert, and so on).
3. The submission contains changes from multiple branches of the same
repository: loop over all of the branches and revert the changes from
that branch independently as in example 2. The parent of the first
revert should be the most recent change of that branch.
4. The submission contains changes from multiple repositories: Same as
different branches, essentially.
5. The submission contains a merge commit:
The revert changes are created on top of the  merge commit, which will be
found as described in the special case above. In case of a merge commit,
we will always need to rebase on top of the merge commit.

Change-Id: I220ae8ef23022af8f16305d920f311c98ff5eaf9
This commit is contained in:
Gal Paikin
2019-11-25 11:40:10 +01:00
parent d022a16822
commit 653a8528f6
3 changed files with 777 additions and 127 deletions

View File

@@ -14,12 +14,23 @@
package com.google.gerrit.server.restapi.change;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE;
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.Multimap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.RevertInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.RevertSubmissionInfo;
@@ -28,67 +39,137 @@ import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.WalkSorter;
import com.google.gerrit.server.change.WalkSorter.PatchSetData;
import com.google.gerrit.server.extensions.events.ChangeReverted;
import com.google.gerrit.server.git.CommitUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.mail.send.RevertedSender;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.Sequences;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ContributorAgreementsChecker;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.restapi.change.CherryPickChange.Result;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.Queue;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.commons.lang.RandomStringUtils;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
public class RevertSubmission
implements RestModifyView<ChangeResource, RevertInput>, UiAction<ChangeResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final Revert revert;
private final Provider<InternalChangeQuery> queryProvider;
private final ChangeResource.Factory changeResourceFactory;
private final Provider<CurrentUser> user;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
private final PatchSetUtil psUtil;
private final ContributorAgreementsChecker contributorAgreements;
private final CherryPickChange cherryPickChange;
private final ChangeJson.Factory json;
private final GitRepositoryManager repoManager;
private final WalkSorter sorter;
private final ChangeMessagesUtil cmUtil;
private final CommitUtil commitUtil;
private final ChangeNotes.Factory changeNotesFactory;
private final ChangeResource.Factory changeResourceFactory;
private final ChangeReverted changeReverted;
private final RevertedSender.Factory revertedSenderFactory;
private final Sequences seq;
private final NotifyResolver notifyResolver;
private final Revert revert;
private final BatchUpdate.Factory updateFactory;
private CherryPickInput cherryPickInput;
private List<ChangeInfo> results;
@Inject
RevertSubmission(
Revert revert,
Provider<InternalChangeQuery> queryProvider,
ChangeResource.Factory changeResourceFactory,
Provider<CurrentUser> user,
PermissionBackend permissionBackend,
ProjectCache projectCache,
PatchSetUtil psUtil,
ContributorAgreementsChecker contributorAgreements) {
this.revert = revert;
ContributorAgreementsChecker contributorAgreements,
CherryPickChange cherryPickChange,
ChangeJson.Factory json,
GitRepositoryManager repoManager,
WalkSorter sorter,
ChangeMessagesUtil cmUtil,
CommitUtil commitUtil,
ChangeNotes.Factory changeNotesFactory,
ChangeResource.Factory changeResourceFactory,
ChangeReverted changeReverted,
RevertedSender.Factory revertedSenderFactory,
Sequences seq,
NotifyResolver notifyResolver,
Revert revert,
BatchUpdate.Factory updateFactory) {
this.queryProvider = queryProvider;
this.changeResourceFactory = changeResourceFactory;
this.user = user;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.psUtil = psUtil;
this.contributorAgreements = contributorAgreements;
this.cherryPickChange = cherryPickChange;
this.json = json;
this.repoManager = repoManager;
this.sorter = sorter;
this.cmUtil = cmUtil;
this.commitUtil = commitUtil;
this.changeNotesFactory = changeNotesFactory;
this.changeResourceFactory = changeResourceFactory;
this.changeReverted = changeReverted;
this.revertedSenderFactory = revertedSenderFactory;
this.seq = seq;
this.notifyResolver = notifyResolver;
this.revert = revert;
this.updateFactory = updateFactory;
results = new ArrayList<>();
cherryPickInput = null;
}
@Override
public Response<RevertSubmissionInfo> apply(ChangeResource changeResource, RevertInput input)
throws RestApiException, NoSuchChangeException, IOException, UpdateException,
PermissionBackendException, NoSuchProjectException, ConfigInvalidException {
throws RestApiException, IOException, UpdateException, PermissionBackendException,
NoSuchProjectException, ConfigInvalidException, StorageException {
if (!changeResource.getChange().isMerged()) {
throw new ResourceConflictException(
@@ -121,30 +202,241 @@ public class RevertSubmission
"current patch set %s of change %s not found",
change.currentPatchSetId(), change.currentPatchSetId()));
}
return Response.ok(revertSubmission(changeDatas, input, submissionId));
}
private RevertSubmissionInfo revertSubmission(
List<ChangeData> changeDatas, RevertInput input, String submissionId)
throws RestApiException, NoSuchChangeException, IOException, UpdateException,
PermissionBackendException, NoSuchProjectException, ConfigInvalidException {
List<ChangeInfo> results;
results = new ArrayList<>();
if (input.topic == null) {
input.topic =
String.format(
"revert-%s-%s", submissionId, RandomStringUtils.randomAlphabetic(10).toUpperCase());
}
for (ChangeData changeData : changeDatas) {
ChangeResource change = changeResourceFactory.create(changeData.notes(), user.get());
// Reverts are done with retrying by using RetryingRestModifyView.
results.add(revert.apply(change, input).value());
return Response.ok(revertSubmission(changeDatas, input));
}
private RevertSubmissionInfo revertSubmission(
List<ChangeData> changeData, RevertInput revertInput)
throws RestApiException, IOException, UpdateException, PermissionBackendException,
NoSuchProjectException, ConfigInvalidException, StorageException {
Multimap<BranchNameKey, ChangeData> changesPerProjectAndBranch = ArrayListMultimap.create();
changeData.stream().forEach(c -> changesPerProjectAndBranch.put(c.change().getDest(), c));
cherryPickInput = new CherryPickInput();
// To create a revert change, we create a revert commit that is then cherry-picked. The revert
// change is created for the cherry-picked commit. Notifications are sent only for this change,
// but not for the intermediately created revert commit.
cherryPickInput.notify = revertInput.notify;
cherryPickInput.notifyDetails = revertInput.notifyDetails;
cherryPickInput.parent = 1;
cherryPickInput.keepReviewers = true;
for (BranchNameKey projectAndBranch : changesPerProjectAndBranch.keySet()) {
Project.NameKey project = projectAndBranch.project();
cherryPickInput.destination = projectAndBranch.branch();
Collection<ChangeData> changesInProjectAndBranch =
changesPerProjectAndBranch.get(projectAndBranch);
// Sort the changes topologically.
Iterator<PatchSetData> sortedChangesInProject =
sorter.sort(changesInProjectAndBranch).iterator();
Set<ObjectId> commitIdsInProjectAndBranch =
changesInProjectAndBranch.stream()
.map(c -> c.currentPatchSet().commitId())
.collect(Collectors.toSet());
while (sortedChangesInProject.hasNext()) {
ChangeNotes changeNotes = sortedChangesInProject.next().data().notes();
if (cherryPickInput.base == null) {
cherryPickInput.base = getBase(changeNotes, commitIdsInProjectAndBranch).name();
}
// This is the code in case this is the first revert of this project + branch, and the
// revert would be on top of the change being reverted.
if (cherryPickInput.base.equals(changeNotes.getCurrentPatchSet().commitId().getName())) {
ChangeInfo revertChangeInfo =
revert
.apply(changeResourceFactory.create(changeNotes, user.get()), revertInput)
.value();
results.add(revertChangeInfo);
cherryPickInput.base =
changeNotesFactory
.createChecked(Change.id(revertChangeInfo._number))
.getCurrentPatchSet()
.commitId()
.getName();
} else {
// This is the code in case this is the second revert (or more) of this project + branch.
ObjectId revCommitId =
commitUtil.createRevertCommit(revertInput.message, changeNotes, user.get());
// TODO (paiking): As a future change, the revert should just be done directly on the
// target rather than just creating a commit and then cherry-picking it.
cherryPickInput.message =
revertInput.message != null
? revertInput.message
: MessageFormat.format(
ChangeMessages.get().revertChangeDefaultMessage,
changeNotes.getChange().getSubject(),
changeNotes.getCurrentPatchSet().commitId().name());
ObjectId generatedChangeId = Change.generateChangeId();
Change.Id cherryPickRevertChangeId = Change.id(seq.nextChangeId());
// TODO (paiking): In the the future, the timestamp should be the same for all the revert
// changes.
try (BatchUpdate bu = updateFactory.create(project, user.get(), TimeUtil.nowTs())) {
bu.setNotify(
notifyResolver.resolve(
firstNonNull(cherryPickInput.notify, NotifyHandling.ALL),
cherryPickInput.notifyDetails));
bu.addOp(
changeNotes.getChange().getId(),
new CreateCherryPickOp(
revCommitId, revertInput.topic, generatedChangeId, cherryPickRevertChangeId));
bu.addOp(changeNotes.getChange().getId(), new PostRevertedMessageOp(generatedChangeId));
bu.addOp(
cherryPickRevertChangeId,
new NotifyOp(changeNotes.getChange(), cherryPickRevertChangeId));
bu.execute();
}
}
}
cherryPickInput.base = null;
}
RevertSubmissionInfo revertSubmissionInfo = new RevertSubmissionInfo();
revertSubmissionInfo.revertChanges = results;
return revertSubmissionInfo;
}
/**
* This function finds the base that the first revert in a project + branch should be based on. It
* searches using BFS for the first commit that is either: 1. Has 2 or more parents, and has as
* parents at least one commit that is part of the submission. 2. A commit that is part of the
* submission. If neither of those are true, it just continues the search by going to the parents.
*
* <p>If 1 is true, if all the parents are part of the submission, it just returns that commit. If
* only some of them are in the submission, the function changes and starts checking only the
* commits that are not part of the submission. If all of them are part of the submission (or they
* are also merge commits that have as parents only other merge commits, or other changes that are
* part of the submission), we will return possibleMergeCommitToReturn which is the original
* commit we started with when 1 was determined to be true.
*
* <p>If 2 is true, it will return the commit that WalkSorter has decided that it should be the
* first commit reverted (e.g changeNotes, which is also the commit that is the first in the
* topological sorting). Unless possibleMergeCommitToReturn is not null, which means we already
* encountered a merge commit with a part of the submission earlier, which means we should return
* that merge commit.
*
* <p>It doesn't run through the entire graph since it will stop once it finds at least one commit
* that is part of the submission.
*
* @param changeNotes changeNotes for the change that is found by WalkSorter to be the first one
* that should be reverted, the first in the topological sorting.
* @param commitIds The commitIds of this project and branch.
* @return the base of the first revert.
*/
private ObjectId getBase(ChangeNotes changeNotes, Set<ObjectId> commitIds)
throws StorageException, IOException {
try (Repository git = repoManager.openRepository(changeNotes.getProjectName());
ObjectInserter oi = git.newObjectInserter();
ObjectReader reader = oi.newReader();
RevWalk revWalk = new RevWalk(reader)) {
ObjectId startCommit =
git.getRefDatabase().findRef(changeNotes.getChange().getDest().branch()).getObjectId();
Queue<ObjectId> commitsToSearch = new LinkedList<>();
commitsToSearch.add(startCommit);
while (!commitsToSearch.isEmpty()) {
RevCommit revCommit = revWalk.parseCommit(commitsToSearch.poll());
if (commitIds.contains(revCommit.getId())) {
return changeNotes.getCurrentPatchSet().commitId();
}
if (revCommit.getParentCount() > 1) {
List<RevCommit> parentsInSubmission =
Arrays.stream(revCommit.getParents())
.filter(parent -> commitIds.contains(parent.getId()))
.collect(Collectors.toList());
if (parentsInSubmission.size() > 1) {
// Found a merge commit that has multiple parent commits that are part of the
// submission.
return revCommit.getId();
}
if (!parentsInSubmission.isEmpty()) {
// Found a merge commit that has only one parent in this submission, but also other
// parents not in the submission. Now we need to check if the others are merge commits
// that have as parents only other merge commits, or other changes from the
// submission.
commitsToSearch.clear();
commitsToSearch.addAll(
Arrays.stream(revCommit.getParents())
.filter(parent -> !commitIds.contains(parent.getId()))
.collect(Collectors.toList()));
if (isMergeCommitDescendantOfAllChangesInTheProjectAndBranchOfTheSubmission(
commitsToSearch, commitIds, revWalk, revCommit, changeNotes)) {
// Found a second commit of that submission that share the same merge commit.
return revCommit.getId();
}
// Couldn't find a second commit of that submission that share the same merge commit.
return changeNotes.getCurrentPatchSet().commitId();
}
}
commitsToSearch.addAll(Arrays.asList(revCommit.getParents()));
}
// This should never happen since it can only happen if we go through the entire repository
// without finding a single commit that matches any commit from the submission.
throw new StorageException(
String.format(
"Couldn't find change %s in the repository %s",
changeNotes.getChangeId(), changeNotes.getProjectName().get()));
}
}
/**
* This helper function calculates whether or not there exists a second commit that is part of the
* submission and ancestor of the same merge commit.
*
* @param commitsToSearch Starts as all the parents of the potential merge commit, except the one
* that is part of the submission.
* @param commitIds The commitIds of this project and branch.
* @param revWalk Used for walking through the revisions.
* @param potentialCommitToReturn The merge commit that is potentially a descendant of all changes
* in the project and branch of the submission.
* @param changeNotes changeNotes for the change that is found by WalkSorter to be the first one
* that should be reverted, the first in the topological sorting.
* @return True if found another commit of this submission, false if not found.
*/
private boolean isMergeCommitDescendantOfAllChangesInTheProjectAndBranchOfTheSubmission(
Queue<ObjectId> commitsToSearch,
Set<ObjectId> commitIds,
RevWalk revWalk,
RevCommit potentialCommitToReturn,
ChangeNotes changeNotes)
throws StorageException, IOException {
while (!commitsToSearch.isEmpty()) {
RevCommit revCommit = revWalk.parseCommit(commitsToSearch.poll());
if (revCommit.getParentCount() > 1) {
List<RevCommit> parents = Arrays.asList(revCommit.getParents());
if (parents.stream().anyMatch(p -> commitIds.contains(p.getId()))) {
// found another commit with a common descendant.
return true;
}
Arrays.asList(revCommit.getParents()).forEach(parent -> commitsToSearch.add(parent));
} else {
// We found a merge commit, we found that one of the parents is not a merge commit nor a
// change of this submission. Therefore the merge commit is not useful, and we just
// rebase on the most recent revert as usual.
return false;
}
}
// This should never happen since it can only happen if we go through the entire repository
// encountering only merge commits after encountering the change whose submission we are
// reverting.
throw new StorageException(
String.format(
"Couldn't find a non-merge commit after encountering commit %s when trying to revert the submission of change %d",
potentialCommitToReturn.getName(), changeNotes.getChange().getChangeId()));
}
@Override
public Description getDescription(ChangeResource rsrc) {
Change change = rsrc.getChange();
@@ -178,4 +470,103 @@ public class RevertSubmission
private Boolean isChangePartOfSubmission(String submissionId) {
return (queryProvider.get().setLimit(2).bySubmissionId(submissionId).size() > 1);
}
private class CreateCherryPickOp implements BatchUpdateOp {
private final ObjectId revCommitId;
private final String topic;
private final ObjectId computedChangeId;
private final Change.Id cherryPickRevertChangeId;
CreateCherryPickOp(
ObjectId revCommitId,
String topic,
ObjectId computedChangeId,
Change.Id cherryPickRevertChangeId) {
this.revCommitId = revCommitId;
this.topic = topic;
this.computedChangeId = computedChangeId;
this.cherryPickRevertChangeId = cherryPickRevertChangeId;
}
@Override
public boolean updateChange(ChangeContext ctx) throws Exception {
Change change = ctx.getChange();
Result cherryPickResult =
cherryPickChange.cherryPick(
updateFactory,
change,
change.getProject(),
revCommitId,
cherryPickInput,
BranchNameKey.create(
change.getProject(), RefNames.fullName(cherryPickInput.destination)),
true,
topic,
change.getId(),
computedChangeId,
cherryPickRevertChangeId);
// save the commit as base for next cherryPick of that branch
cherryPickInput.base =
changeNotesFactory
.createChecked(cherryPickResult.changeId())
.getCurrentPatchSet()
.commitId()
.getName();
results.add(
json.noOptions()
.format(change.getProject(), cherryPickResult.changeId(), ChangeInfo::new));
return true;
}
}
private class NotifyOp implements BatchUpdateOp {
private final Change change;
private final Change.Id revertChangeId;
NotifyOp(Change change, Change.Id revertChangeId) {
this.change = change;
this.revertChangeId = revertChangeId;
}
@Override
public void postUpdate(Context ctx) throws Exception {
changeReverted.fire(
change, changeNotesFactory.createChecked(revertChangeId).getChange(), ctx.getWhen());
try {
RevertedSender cm = revertedSenderFactory.create(ctx.getProject(), change.getId());
cm.setFrom(ctx.getAccountId());
cm.setNotify(ctx.getNotify(change.getId()));
cm.send();
} catch (Exception err) {
logger.atSevere().withCause(err).log(
"Cannot send email for revert change %s", change.getId());
}
}
}
/**
* create a message that describes the revert if the cherry-pick is successful, and point the
* revert of the change towards the cherry-pick. The cherry-pick is the updated change that acts
* as "revert-of" the original change.
*/
private class PostRevertedMessageOp implements BatchUpdateOp {
private final ObjectId computedChangeId;
PostRevertedMessageOp(ObjectId computedChangeId) {
this.computedChangeId = computedChangeId;
}
@Override
public boolean updateChange(ChangeContext ctx) throws Exception {
Change change = ctx.getChange();
PatchSet.Id patchSetId = change.currentPatchSetId();
ChangeMessage changeMessage =
ChangeMessagesUtil.newMessage(
ctx,
"Created a revert of this change as I" + computedChangeId.getName(),
ChangeMessagesUtil.TAG_REVERT);
cmUtil.addChangeMessage(ctx.getUpdate(patchSetId), changeMessage);
return true;
}
}
}