commit 399cf4dd6a7700f1beef1643e02d47103f30b4de Author: Alice Kober-Sotzek Date: Fri Mar 13 18:44:43 2020 +0100 ChangeEditModifier: Unify the logic used for change edits and apply-fix The general steps for applying the modifications, creating a new commit, and getting a change edit as result were the same. However, both use cases also have some slight differences (e.g. fixes are always applied on the patchset commit; a change-edit action can refer to the patchset commit or an existing change edit depending on whether an edit exists). Because of this, we had two different methods in the past. This change adjusts the code such that we only have one method which captures the general logic for creating/updating a change edit. Due to the above-mentioned differences, we needed to create strategy objects, though, to select different behaviors depending on the context/situation. The chosen approach is not perfect and could introduce additional complexity for first-time readers. If somebody thinks of a better structure in the future, feel free to adjust it. The follow-up change (I4832e0b72) combines the logic of updating the commit message with what we prepared in this change. The overall goal is to have only one method which knows how to correctly update the full commit (source code + commit message) with various modifications in one go (e.g. update the commit message and adjust several source lines at the same time). The existing REST endpoints for change edits don't need this combined operation at the moment. We could of course extend them in the future. The main use case will be applying suggested fixes of robot comments. Up to now, such fixes may only modify the source code. With I9658c0768, we also want to support modifications of the commit message. Change-Id: Icbb3787384ac5a80dc8d0558fe5360d461ab327d diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java index a05c392..15460ac 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java @@ -83,12 +83,12 @@ import org.eclipse.jgit.transport.ReceiveCommand; public class ChangeEditModifier { private final TimeZone tz; - private final ChangeIndexer indexer; private final Provider currentUser; private final PermissionBackend permissionBackend; private final ChangeEditUtil changeEditUtil; private final PatchSetUtil patchSetUtil; private final ProjectCache projectCache; + private final NoteDbEdits noteDbEdits; @Inject ChangeEditModifier( @@ -99,13 +99,14 @@ public class ChangeEditModifier { ChangeEditUtil changeEditUtil, PatchSetUtil patchSetUtil, ProjectCache projectCache) { - this.indexer = indexer; this.currentUser = currentUser; this.permissionBackend = permissionBackend; this.tz = gerritIdent.getTimeZone(); this.changeEditUtil = changeEditUtil; this.patchSetUtil = patchSetUtil; this.projectCache = projectCache; + + noteDbEdits = new NoteDbEdits(tz, indexer, currentUser); } /** @@ -130,7 +131,7 @@ public class ChangeEditModifier { PatchSet currentPatchSet = lookupCurrentPatchSet(notes); ObjectId patchSetCommitId = currentPatchSet.commitId(); - createEdit(repository, notes, currentPatchSet, patchSetCommitId, TimeUtil.nowTs()); + noteDbEdits.createEdit(repository, notes, currentPatchSet, patchSetCommitId, TimeUtil.nowTs()); } /** @@ -176,8 +177,7 @@ public class ChangeEditModifier { "Rebase change edit against root commit not supported"); } - Change change = changeEdit.getChange(); - RevCommit basePatchSetCommit = lookupCommit(repository, currentPatchSet); + RevCommit basePatchSetCommit = NoteDbEdits.lookupCommit(repository, currentPatchSet.commitId()); RevTree basePatchSetTree = basePatchSetCommit.getTree(); ObjectId newTreeId = merge(repository, changeEdit, basePatchSetTree); @@ -186,15 +186,8 @@ public class ChangeEditModifier { ObjectId newEditCommitId = createCommit(repository, basePatchSetCommit, newTreeId, commitMessage, nowTimestamp); - String newEditRefName = getEditRefName(change, currentPatchSet); - updateReferenceWithNameChange( - repository, - changeEdit.getRefName(), - currentEditCommit, - newEditRefName, - newEditCommitId, - nowTimestamp); - reindex(change); + noteDbEdits.baseEditOnDifferentPatchset( + repository, changeEdit, currentPatchSet, currentEditCommit, newEditCommitId, nowTimestamp); } /** @@ -218,7 +211,7 @@ public class ChangeEditModifier { Optional optionalChangeEdit = lookupChangeEdit(notes); PatchSet basePatchSet = getBasePatchSet(optionalChangeEdit, notes); - RevCommit basePatchSetCommit = lookupCommit(repository, basePatchSet); + RevCommit basePatchSetCommit = NoteDbEdits.lookupCommit(repository, basePatchSet.commitId()); RevCommit baseCommit = optionalChangeEdit.map(ChangeEdit::getEditCommit).orElse(basePatchSetCommit); @@ -233,14 +226,14 @@ public class ChangeEditModifier { createCommit(repository, basePatchSetCommit, baseTree, newCommitMessage, nowTimestamp); if (optionalChangeEdit.isPresent()) { - updateEdit( + noteDbEdits.updateEdit( notes.getProjectName(), repository, optionalChangeEdit.get(), newEditCommit, nowTimestamp); } else { - createEdit(repository, notes, basePatchSet, newEditCommit, nowTimestamp); + noteDbEdits.createEdit(repository, notes, basePatchSet, newEditCommit, nowTimestamp); } } @@ -262,7 +255,11 @@ public class ChangeEditModifier { Repository repository, ChangeNotes notes, String filePath, RawInput newContent) throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { - modifyTree(repository, notes, new ChangeFileContentModification(filePath, newContent)); + modifyTree( + repository, + notes, + new ModificationIntention.LatestCommit(), + ImmutableList.of(new ChangeFileContentModification(filePath, newContent))); } /** @@ -281,7 +278,11 @@ public class ChangeEditModifier { public void deleteFile(Repository repository, ChangeNotes notes, String file) throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { - modifyTree(repository, notes, new DeleteFileModification(file)); + modifyTree( + repository, + notes, + new ModificationIntention.LatestCommit(), + ImmutableList.of(new DeleteFileModification(file))); } /** @@ -303,7 +304,11 @@ public class ChangeEditModifier { Repository repository, ChangeNotes notes, String currentFilePath, String newFilePath) throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { - modifyTree(repository, notes, new RenameFileModification(currentFilePath, newFilePath)); + modifyTree( + repository, + notes, + new ModificationIntention.LatestCommit(), + ImmutableList.of(new RenameFileModification(currentFilePath, newFilePath))); } /** @@ -321,38 +326,11 @@ public class ChangeEditModifier { public void restoreFile(Repository repository, ChangeNotes notes, String file) throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { - modifyTree(repository, notes, new RestoreFileModification(file)); - } - - private void modifyTree( - Repository repository, ChangeNotes notes, TreeModification treeModification) - throws AuthException, BadRequestException, IOException, InvalidChangeOperationException, - PermissionBackendException, ResourceConflictException { - assertCanEdit(notes); - - Optional optionalChangeEdit = lookupChangeEdit(notes); - PatchSet basePatchSet = getBasePatchSet(optionalChangeEdit, notes); - RevCommit basePatchSetCommit = lookupCommit(repository, basePatchSet); - RevCommit baseCommit = - optionalChangeEdit.map(ChangeEdit::getEditCommit).orElse(basePatchSetCommit); - - ObjectId newTreeId = createNewTree(repository, baseCommit, ImmutableList.of(treeModification)); - - String commitMessage = baseCommit.getFullMessage(); - Timestamp nowTimestamp = TimeUtil.nowTs(); - ObjectId newEditCommit = - createCommit(repository, basePatchSetCommit, newTreeId, commitMessage, nowTimestamp); - - if (optionalChangeEdit.isPresent()) { - updateEdit( - notes.getProjectName(), - repository, - optionalChangeEdit.get(), - newEditCommit, - nowTimestamp); - } else { - createEdit(repository, notes, basePatchSet, newEditCommit, nowTimestamp); - } + modifyTree( + repository, + notes, + new ModificationIntention.LatestCommit(), + ImmutableList.of(new RestoreFileModification(file))); } /** @@ -377,39 +355,47 @@ public class ChangeEditModifier { PatchSet patchSet, List treeModifications) throws AuthException, BadRequestException, IOException, InvalidChangeOperationException, - MergeConflictException, PermissionBackendException, ResourceConflictException { + PermissionBackendException, ResourceConflictException { + return modifyTree( + repository, notes, new ModificationIntention.PatchsetCommit(patchSet), treeModifications); + } + + private ChangeEdit modifyTree( + Repository repository, + ChangeNotes notes, + ModificationIntention modificationIntention, + List treeModifications) + throws AuthException, BadRequestException, IOException, InvalidChangeOperationException, + PermissionBackendException, ResourceConflictException { assertCanEdit(notes); Optional optionalChangeEdit = lookupChangeEdit(notes); - ensureAllowedPatchSet(notes, optionalChangeEdit, patchSet); - - RevCommit patchSetCommit = lookupCommit(repository, patchSet); - ObjectId newTreeId = createNewTree(repository, patchSetCommit, treeModifications); - - if (optionalChangeEdit.isPresent()) { - ChangeEdit changeEdit = optionalChangeEdit.get(); - newTreeId = merge(repository, changeEdit, newTreeId); - if (ObjectId.isEqual(newTreeId, changeEdit.getEditCommit().getTree())) { - // Modifications are already contained in the change edit. - return changeEdit; - } + EditBehavior editBehavior = + optionalChangeEdit + .map(changeEdit -> new ExistingEditBehavior(changeEdit, noteDbEdits)) + .orElseGet(() -> new NewEditBehavior(noteDbEdits)); + ModificationTarget modificationTarget = + editBehavior.getModificationTarget(notes, modificationIntention); + + RevCommit commitToModify = modificationTarget.getCommit(repository); + ObjectId newTreeId = createNewTree(repository, commitToModify, treeModifications); + newTreeId = editBehavior.mergeTreesIfNecessary(repository, newTreeId, commitToModify); + + Optional unmodifiedEdit = editBehavior.getEditIfNoModification(newTreeId); + if (unmodifiedEdit.isPresent()) { + return unmodifiedEdit.get(); } - String commitMessage = - optionalChangeEdit.map(ChangeEdit::getEditCommit).orElse(patchSetCommit).getFullMessage(); + PatchSet basePatchset = modificationTarget.getBasePatchset(); + RevCommit basePatchsetCommit = NoteDbEdits.lookupCommit(repository, basePatchset.commitId()); + + String commitMessage = editBehavior.getNewCommitMessage(commitToModify); Timestamp nowTimestamp = TimeUtil.nowTs(); ObjectId newEditCommit = - createCommit(repository, patchSetCommit, newTreeId, commitMessage, nowTimestamp); + createCommit(repository, basePatchsetCommit, newTreeId, commitMessage, nowTimestamp); - if (optionalChangeEdit.isPresent()) { - return updateEdit( - notes.getProjectName(), - repository, - optionalChangeEdit.get(), - newEditCommit, - nowTimestamp); - } - return createEdit(repository, notes, patchSet, newEditCommit, nowTimestamp); + return editBehavior.updateEditInStorage( + repository, notes, basePatchset, newEditCommit, nowTimestamp); } private void assertCanEdit(ChangeNotes notes) @@ -437,30 +423,6 @@ public class ChangeEditModifier { } } - private static void ensureAllowedPatchSet( - ChangeNotes notes, Optional optionalChangeEdit, PatchSet patchSet) - throws InvalidChangeOperationException { - if (optionalChangeEdit.isPresent()) { - ChangeEdit changeEdit = optionalChangeEdit.get(); - if (!isBasedOn(changeEdit, patchSet)) { - throw new InvalidChangeOperationException( - String.format( - "Only the patch set %s on which the existing change edit is based may be modified " - + "(specified patch set: %s)", - changeEdit.getBasePatchSet().id(), patchSet.id())); - } - } else { - PatchSet.Id patchSetId = patchSet.id(); - PatchSet.Id currentPatchSetId = notes.getChange().currentPatchSetId(); - if (!patchSetId.equals(currentPatchSetId)) { - throw new InvalidChangeOperationException( - String.format( - "A change edit may only be created for the current patch set %s (and not for %s)", - currentPatchSetId, patchSetId)); - } - } - } - private Optional lookupChangeEdit(ChangeNotes notes) throws AuthException, IOException { return changeEditUtil.byChange(notes); @@ -480,19 +442,6 @@ public class ChangeEditModifier { return editBasePatchSet.id().equals(patchSet.id()); } - private static RevCommit lookupCommit(Repository repository, PatchSet patchSet) - throws IOException { - ObjectId patchSetCommitId = patchSet.commitId(); - return lookupCommit(repository, patchSetCommitId); - } - - private static RevCommit lookupCommit(Repository repository, ObjectId commitId) - throws IOException { - try (RevWalk revWalk = new RevWalk(repository)) { - return revWalk.parseCommit(commitId); - } - } - private static ObjectId createNewTree( Repository repository, RevCommit baseCommit, List treeModifications) throws BadRequestException, IOException, InvalidChangeOperationException { @@ -530,7 +479,7 @@ public class ChangeEditModifier { private ObjectId createCommit( Repository repository, - RevCommit basePatchSetCommit, + RevCommit basePatchsetCommit, ObjectId tree, String commitMessage, Timestamp timestamp) @@ -538,8 +487,8 @@ public class ChangeEditModifier { try (ObjectInserter objectInserter = repository.newObjectInserter()) { CommitBuilder builder = new CommitBuilder(); builder.setTreeId(tree); - builder.setParentIds(basePatchSetCommit.getParents()); - builder.setAuthor(basePatchSetCommit.getAuthorIdent()); + builder.setParentIds(basePatchsetCommit.getParents()); + builder.setAuthor(basePatchsetCommit.getAuthorIdent()); builder.setCommitter(getCommitterIdent(timestamp)); builder.setMessage(commitMessage); ObjectId newCommitId = objectInserter.insert(builder); @@ -553,107 +502,276 @@ public class ChangeEditModifier { return user.newCommitterIdent(commitTimestamp, tz); } - private ChangeEdit createEdit( - Repository repository, - ChangeNotes notes, - PatchSet basePatchSet, - ObjectId newEditCommitId, - Timestamp timestamp) - throws IOException { - Change change = notes.getChange(); - String editRefName = getEditRefName(change, basePatchSet); - updateReference( - notes.getProjectName(), - repository, - editRefName, - ObjectId.zeroId(), - newEditCommitId, - timestamp); - reindex(change); - - RevCommit newEditCommit = lookupCommit(repository, newEditCommitId); - return new ChangeEdit(change, editRefName, newEditCommit, basePatchSet); + /** + * Strategy to apply depending on the current situation regarding change edits (e.g. creating a + * new edit requires different storage modifications than updating an existing edit). + */ + private interface EditBehavior { + + ModificationTarget getModificationTarget( + ChangeNotes notes, ModificationIntention targetIntention) + throws InvalidChangeOperationException; + + ObjectId mergeTreesIfNecessary( + Repository repository, ObjectId newTreeId, ObjectId commitToModify) + throws IOException, MergeConflictException; + + String getNewCommitMessage(RevCommit commitToModify); + + Optional getEditIfNoModification(ObjectId newTreeId); + + ChangeEdit updateEditInStorage( + Repository repository, + ChangeNotes notes, + PatchSet basePatchSet, + ObjectId newEditCommitId, + Timestamp timestamp) + throws IOException; } - private String getEditRefName(Change change, PatchSet basePatchSet) { - IdentifiedUser me = currentUser.get().asIdentifiedUser(); - return RefNames.refsEdit(me.getAccountId(), change.getId(), basePatchSet.id()); + private static class ExistingEditBehavior implements EditBehavior { + + private final ChangeEdit changeEdit; + private final NoteDbEdits noteDbEdits; + + ExistingEditBehavior(ChangeEdit changeEdit, NoteDbEdits noteDbEdits) { + this.changeEdit = changeEdit; + this.noteDbEdits = noteDbEdits; + } + + @Override + public ModificationTarget getModificationTarget( + ChangeNotes notes, ModificationIntention targetIntention) + throws InvalidChangeOperationException { + ModificationTarget modificationTarget = targetIntention.getTargetWhenEditExists(changeEdit); + // It would be better to do this validation in the implementation of the REST endpoints + // before calling any write actions on ChangeEditModifier. + modificationTarget.ensureTargetMayBeModifiedDespiteExistingEdit(changeEdit); + return modificationTarget; + } + + @Override + public ObjectId mergeTreesIfNecessary( + Repository repository, ObjectId newTreeId, ObjectId commitToModify) + throws IOException, MergeConflictException { + if (ObjectId.isEqual(changeEdit.getEditCommit(), commitToModify)) { + return newTreeId; + } + return merge(repository, changeEdit, newTreeId); + } + + @Override + public String getNewCommitMessage(RevCommit commitToModify) { + return changeEdit.getEditCommit().getFullMessage(); + } + + @Override + public Optional getEditIfNoModification(ObjectId newTreeId) { + if (ObjectId.isEqual(newTreeId, changeEdit.getEditCommit().getTree())) { + // Modifications are already contained in the change edit. + return Optional.of(changeEdit); + } + return Optional.empty(); + } + + @Override + public ChangeEdit updateEditInStorage( + Repository repository, + ChangeNotes notes, + PatchSet basePatchSet, + ObjectId newEditCommitId, + Timestamp timestamp) + throws IOException { + return noteDbEdits.updateEdit( + notes.getProjectName(), repository, changeEdit, newEditCommitId, timestamp); + } } - private ChangeEdit updateEdit( - Project.NameKey projectName, - Repository repository, - ChangeEdit changeEdit, - ObjectId newEditCommitId, - Timestamp timestamp) - throws IOException { - String editRefName = changeEdit.getRefName(); - RevCommit currentEditCommit = changeEdit.getEditCommit(); - updateReference( - projectName, repository, editRefName, currentEditCommit, newEditCommitId, timestamp); - reindex(changeEdit.getChange()); + private static class NewEditBehavior implements EditBehavior { + + private final NoteDbEdits noteDbEdits; + + NewEditBehavior(NoteDbEdits noteDbEdits) { + this.noteDbEdits = noteDbEdits; + } + + @Override + public ModificationTarget getModificationTarget( + ChangeNotes notes, ModificationIntention targetIntention) + throws InvalidChangeOperationException { + ModificationTarget modificationTarget = targetIntention.getTargetWhenNoEdit(notes); + // It would be better to do this validation in the implementation of the REST endpoints + // before calling any write actions on ChangeEditModifier. + modificationTarget.ensureNewEditMayBeBasedOnTarget(notes.getChange()); + return modificationTarget; + } + + @Override + public ObjectId mergeTreesIfNecessary( + Repository repository, ObjectId newTreeId, ObjectId commitToModify) { + return newTreeId; + } - RevCommit newEditCommit = lookupCommit(repository, newEditCommitId); - return new ChangeEdit( - changeEdit.getChange(), editRefName, newEditCommit, changeEdit.getBasePatchSet()); + @Override + public String getNewCommitMessage(RevCommit commitToModify) { + return commitToModify.getFullMessage(); + } + + @Override + public Optional getEditIfNoModification(ObjectId newTreeId) { + return Optional.empty(); + } + + @Override + public ChangeEdit updateEditInStorage( + Repository repository, + ChangeNotes notes, + PatchSet basePatchSet, + ObjectId newEditCommitId, + Timestamp timestamp) + throws IOException { + return noteDbEdits.createEdit(repository, notes, basePatchSet, newEditCommitId, timestamp); + } } - private void updateReference( - Project.NameKey projectName, - Repository repository, - String refName, - ObjectId currentObjectId, - ObjectId targetObjectId, - Timestamp timestamp) - throws IOException { - RefUpdate ru = repository.updateRef(refName); - ru.setExpectedOldObjectId(currentObjectId); - ru.setNewObjectId(targetObjectId); - ru.setRefLogIdent(getRefLogIdent(timestamp)); - ru.setRefLogMessage("inline edit (amend)", false); - ru.setForceUpdate(true); - try (RevWalk revWalk = new RevWalk(repository)) { - RefUpdate.Result res = ru.update(revWalk); - String message = "cannot update " + ru.getName() + " in " + projectName + ": " + res; - if (res == RefUpdate.Result.LOCK_FAILURE) { - throw new LockFailureException(message, ru); + private static class NoteDbEdits { + private final TimeZone tz; + private final ChangeIndexer indexer; + private final Provider currentUser; + + NoteDbEdits(TimeZone tz, ChangeIndexer indexer, Provider currentUser) { + this.tz = tz; + this.indexer = indexer; + this.currentUser = currentUser; + } + + ChangeEdit createEdit( + Repository repository, + ChangeNotes notes, + PatchSet basePatchset, + ObjectId newEditCommitId, + Timestamp timestamp) + throws IOException { + Change change = notes.getChange(); + String editRefName = getEditRefName(change, basePatchset); + updateReference( + notes.getProjectName(), + repository, + editRefName, + ObjectId.zeroId(), + newEditCommitId, + timestamp); + reindex(change); + + RevCommit newEditCommit = lookupCommit(repository, newEditCommitId); + return new ChangeEdit(change, editRefName, newEditCommit, basePatchset); + } + + private String getEditRefName(Change change, PatchSet basePatchset) { + IdentifiedUser me = currentUser.get().asIdentifiedUser(); + return RefNames.refsEdit(me.getAccountId(), change.getId(), basePatchset.id()); + } + + ChangeEdit updateEdit( + Project.NameKey projectName, + Repository repository, + ChangeEdit changeEdit, + ObjectId newEditCommitId, + Timestamp timestamp) + throws IOException { + String editRefName = changeEdit.getRefName(); + RevCommit currentEditCommit = changeEdit.getEditCommit(); + updateReference( + projectName, repository, editRefName, currentEditCommit, newEditCommitId, timestamp); + reindex(changeEdit.getChange()); + + RevCommit newEditCommit = lookupCommit(repository, newEditCommitId); + return new ChangeEdit( + changeEdit.getChange(), editRefName, newEditCommit, changeEdit.getBasePatchSet()); + } + + private void updateReference( + Project.NameKey projectName, + Repository repository, + String refName, + ObjectId currentObjectId, + ObjectId targetObjectId, + Timestamp timestamp) + throws IOException { + RefUpdate ru = repository.updateRef(refName); + ru.setExpectedOldObjectId(currentObjectId); + ru.setNewObjectId(targetObjectId); + ru.setRefLogIdent(getRefLogIdent(timestamp)); + ru.setRefLogMessage("inline edit (amend)", false); + ru.setForceUpdate(true); + try (RevWalk revWalk = new RevWalk(repository)) { + RefUpdate.Result res = ru.update(revWalk); + String message = "cannot update " + ru.getName() + " in " + projectName + ": " + res; + if (res == RefUpdate.Result.LOCK_FAILURE) { + throw new LockFailureException(message, ru); + } + if (res != RefUpdate.Result.NEW && res != RefUpdate.Result.FORCED) { + throw new IOException(message); + } } - if (res != RefUpdate.Result.NEW && res != RefUpdate.Result.FORCED) { - throw new IOException(message); + } + + void baseEditOnDifferentPatchset( + Repository repository, + ChangeEdit changeEdit, + PatchSet currentPatchSet, + ObjectId currentEditCommit, + ObjectId newEditCommitId, + Timestamp nowTimestamp) + throws IOException { + String newEditRefName = getEditRefName(changeEdit.getChange(), currentPatchSet); + updateReferenceWithNameChange( + repository, + changeEdit.getRefName(), + currentEditCommit, + newEditRefName, + newEditCommitId, + nowTimestamp); + reindex(changeEdit.getChange()); + } + + private void updateReferenceWithNameChange( + Repository repository, + String currentRefName, + ObjectId currentObjectId, + String newRefName, + ObjectId targetObjectId, + Timestamp timestamp) + throws IOException { + BatchRefUpdate batchRefUpdate = repository.getRefDatabase().newBatchUpdate(); + batchRefUpdate.addCommand(new ReceiveCommand(ObjectId.zeroId(), targetObjectId, newRefName)); + batchRefUpdate.addCommand( + new ReceiveCommand(currentObjectId, ObjectId.zeroId(), currentRefName)); + batchRefUpdate.setRefLogMessage("rebase edit", false); + batchRefUpdate.setRefLogIdent(getRefLogIdent(timestamp)); + try (RevWalk revWalk = new RevWalk(repository)) { + batchRefUpdate.execute(revWalk, NullProgressMonitor.INSTANCE); + } + for (ReceiveCommand cmd : batchRefUpdate.getCommands()) { + if (cmd.getResult() != ReceiveCommand.Result.OK) { + throw new IOException("failed: " + cmd); + } } } - } - private void updateReferenceWithNameChange( - Repository repository, - String currentRefName, - ObjectId currentObjectId, - String newRefName, - ObjectId targetObjectId, - Timestamp timestamp) - throws IOException { - BatchRefUpdate batchRefUpdate = repository.getRefDatabase().newBatchUpdate(); - batchRefUpdate.addCommand(new ReceiveCommand(ObjectId.zeroId(), targetObjectId, newRefName)); - batchRefUpdate.addCommand( - new ReceiveCommand(currentObjectId, ObjectId.zeroId(), currentRefName)); - batchRefUpdate.setRefLogMessage("rebase edit", false); - batchRefUpdate.setRefLogIdent(getRefLogIdent(timestamp)); - try (RevWalk revWalk = new RevWalk(repository)) { - batchRefUpdate.execute(revWalk, NullProgressMonitor.INSTANCE); - } - for (ReceiveCommand cmd : batchRefUpdate.getCommands()) { - if (cmd.getResult() != ReceiveCommand.Result.OK) { - throw new IOException("failed: " + cmd); + static RevCommit lookupCommit(Repository repository, ObjectId commitId) throws IOException { + try (RevWalk revWalk = new RevWalk(repository)) { + return revWalk.parseCommit(commitId); } } - } - private PersonIdent getRefLogIdent(Timestamp timestamp) { - IdentifiedUser user = currentUser.get().asIdentifiedUser(); - return user.newRefLogIdent(timestamp, tz); - } + private PersonIdent getRefLogIdent(Timestamp timestamp) { + IdentifiedUser user = currentUser.get().asIdentifiedUser(); + return user.newRefLogIdent(timestamp, tz); + } - private void reindex(Change change) { - indexer.index(change); + private void reindex(Change change) { + indexer.index(change); + } } } diff --git a/java/com/google/gerrit/server/edit/ModificationIntention.java b/java/com/google/gerrit/server/edit/ModificationIntention.java new file mode 100644 index 0000000..531f682 --- /dev/null +++ b/java/com/google/gerrit/server/edit/ModificationIntention.java @@ -0,0 +1,72 @@ +// Copyright (C) 2020 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.edit; + +import com.google.gerrit.entities.PatchSet; +import com.google.gerrit.server.notedb.ChangeNotes; + +/** + * Intended modification target. + * + *

See also {@link ModificationTarget}. Some modifications may have a fixed target (e.g. + * suggested fixes of robot comments). For other modifications, the presence of a change edit + * influences their target. The latter comes from the REST endpoints of change edits which work no + * matter whether a change edit is present or not. If it's not present, a new change edit is created + * based on the current patchset. As we don't want to create an "empty" commit for the new change + * edit first, we need this class/interface for the flexible handling. + */ +interface ModificationIntention { + + ModificationTarget getTargetWhenEditExists(ChangeEdit changeEdit); + + ModificationTarget getTargetWhenNoEdit(ChangeNotes notes); + + /** A specific patchset is the modification target. */ + class PatchsetCommit implements ModificationIntention { + + private final PatchSet patchSet; + + PatchsetCommit(PatchSet patchSet) { + this.patchSet = patchSet; + } + + @Override + public ModificationTarget getTargetWhenEditExists(ChangeEdit changeEdit) { + return new ModificationTarget.PatchsetCommit(patchSet); + } + + @Override + public ModificationTarget getTargetWhenNoEdit(ChangeNotes notes) { + return new ModificationTarget.PatchsetCommit(patchSet); + } + } + + /** + * The latest commit should be the modification target. If a change edit exists, it's considered + * to be the latest commit. Otherwise, defer to the latest patchset commit. + */ + class LatestCommit implements ModificationIntention { + + @Override + public ModificationTarget getTargetWhenEditExists(ChangeEdit changeEdit) { + return new ModificationTarget.EditCommit(changeEdit); + } + + @Override + public ModificationTarget getTargetWhenNoEdit(ChangeNotes notes) { + return new ModificationTarget.PatchsetCommit(notes.getCurrentPatchSet()); + } + } +} diff --git a/java/com/google/gerrit/server/edit/ModificationTarget.java b/java/com/google/gerrit/server/edit/ModificationTarget.java new file mode 100644 index 0000000..0de0149 --- /dev/null +++ b/java/com/google/gerrit/server/edit/ModificationTarget.java @@ -0,0 +1,140 @@ +// Copyright (C) 2020 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.edit; + +import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.PatchSet; +import com.google.gerrit.server.project.InvalidChangeOperationException; +import java.io.IOException; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; + +/** + * Target of the modification of a commit. + * + *

This is currently used in the context of change edits which involves both direct actions on + * change edits (e.g. creating a change edit; modifying a file of a change edit) as well as indirect + * creation/modification of them (e.g. via applying a suggested fix of a robot comment.) + * + *

Depending on the situation and exact action, either an existing {@link ChangeEdit} (-> {@link + * EditCommit} or a specific patchset commit (-> {@link PatchsetCommit}) is the target of a + * modification. + */ +public interface ModificationTarget { + + void ensureNewEditMayBeBasedOnTarget(Change change) throws InvalidChangeOperationException; + + void ensureTargetMayBeModifiedDespiteExistingEdit(ChangeEdit changeEdit) + throws InvalidChangeOperationException; + + /** Commit to modify. */ + RevCommit getCommit(Repository repository) throws IOException; + + /** + * Patchset within whose context the modification happens. This also applies to change edits as + * each change edit is based on a specific patchset. + */ + PatchSet getBasePatchset(); + + /** A specific patchset commit is the target of the modification. */ + class PatchsetCommit implements ModificationTarget { + + private final PatchSet patchset; + + PatchsetCommit(PatchSet patchset) { + this.patchset = patchset; + } + + @Override + public void ensureTargetMayBeModifiedDespiteExistingEdit(ChangeEdit changeEdit) + throws InvalidChangeOperationException { + if (!isBasedOn(changeEdit, patchset)) { + throw new InvalidChangeOperationException( + String.format( + "Only the patch set %s on which the existing change edit is based may be modified " + + "(specified patch set: %s)", + changeEdit.getBasePatchSet().id(), patchset.id())); + } + } + + private static boolean isBasedOn(ChangeEdit changeEdit, PatchSet patchSet) { + PatchSet editBasePatchSet = changeEdit.getBasePatchSet(); + return editBasePatchSet.id().equals(patchSet.id()); + } + + @Override + public void ensureNewEditMayBeBasedOnTarget(Change change) + throws InvalidChangeOperationException { + PatchSet.Id patchSetId = patchset.id(); + PatchSet.Id currentPatchSetId = change.currentPatchSetId(); + if (!patchSetId.equals(currentPatchSetId)) { + throw new InvalidChangeOperationException( + String.format( + "A change edit may only be created for the current patch set %s (and not for %s)", + currentPatchSetId, patchSetId)); + } + } + + @Override + public RevCommit getCommit(Repository repository) throws IOException { + try (RevWalk revWalk = new RevWalk(repository)) { + return revWalk.parseCommit(patchset.commitId()); + } + } + + @Override + public PatchSet getBasePatchset() { + return patchset; + } + } + + /** An existing {@link ChangeEdit} commit is the target of the modification. */ + class EditCommit implements ModificationTarget { + + private final ChangeEdit changeEdit; + + EditCommit(ChangeEdit changeEdit) { + this.changeEdit = changeEdit; + } + + @Override + public void ensureNewEditMayBeBasedOnTarget(Change change) { + // The current code will never create a new edit if one already exists. It would be a + // programmer error if this changes in the future (without adjusting the storage of change + // edits). + throw new IllegalStateException( + String.format( + "Change %d already has a change edit for the calling user. A new change edit can't" + + " be created.", + changeEdit.getChange().getChangeId())); + } + + @Override + public void ensureTargetMayBeModifiedDespiteExistingEdit(ChangeEdit changeEdit) { + // The target is the change edit and hence can be modified. + } + + @Override + public RevCommit getCommit(Repository repository) throws IOException { + return changeEdit.getEditCommit(); + } + + @Override + public PatchSet getBasePatchset() { + return changeEdit.getBasePatchSet(); + } + } +}