commit 4bdaf9f505ade733cba4c30f882932881aa5664f Author: Alice Kober-Sotzek Date: Fri Mar 13 20:46:03 2020 +0100 ChangeEditModifier: Combine the logic of commit message and tree edits Both methods were doing fairly the same with some slight differences (commit message update vs. tree update). In the future, we want to be able to update the commit message and the Git tree at the same time (see I9658c0768). Also see the previous change (Icbb378738) for further details. Change-Id: I4832e0b72bf77bbc88a0ac0ca7990d52e0e31178 diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java index 15460ac..80846d9 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java @@ -53,6 +53,7 @@ import com.google.inject.Singleton; import java.io.IOException; import java.sql.Timestamp; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.TimeZone; import org.eclipse.jgit.dircache.InvalidPathException; @@ -199,42 +200,18 @@ public class ChangeEditModifier { * modified * @param newCommitMessage the new commit message * @throws AuthException if the user isn't authenticated or not allowed to use change edits - * @throws UnchangedCommitMessageException if the commit message is the same as before + * @throws InvalidChangeOperationException if the commit message is the same as before * @throws PermissionBackendException * @throws BadRequestException if the commit message is malformed */ public void modifyMessage(Repository repository, ChangeNotes notes, String newCommitMessage) - throws AuthException, IOException, UnchangedCommitMessageException, + throws AuthException, IOException, InvalidChangeOperationException, PermissionBackendException, BadRequestException, ResourceConflictException { - assertCanEdit(notes); - newCommitMessage = CommitMessageUtil.checkAndSanitizeCommitMessage(newCommitMessage); - - Optional optionalChangeEdit = lookupChangeEdit(notes); - PatchSet basePatchSet = getBasePatchSet(optionalChangeEdit, notes); - RevCommit basePatchSetCommit = NoteDbEdits.lookupCommit(repository, basePatchSet.commitId()); - RevCommit baseCommit = - optionalChangeEdit.map(ChangeEdit::getEditCommit).orElse(basePatchSetCommit); - - String currentCommitMessage = baseCommit.getFullMessage(); - if (newCommitMessage.equals(currentCommitMessage)) { - throw new UnchangedCommitMessageException(); - } - - RevTree baseTree = baseCommit.getTree(); - Timestamp nowTimestamp = TimeUtil.nowTs(); - ObjectId newEditCommit = - createCommit(repository, basePatchSetCommit, baseTree, newCommitMessage, nowTimestamp); - - if (optionalChangeEdit.isPresent()) { - noteDbEdits.updateEdit( - notes.getProjectName(), - repository, - optionalChangeEdit.get(), - newEditCommit, - nowTimestamp); - } else { - noteDbEdits.createEdit(repository, notes, basePatchSet, newEditCommit, nowTimestamp); - } + modifyCommit( + repository, + notes, + new ModificationIntention.LatestCommit(), + CommitModification.builder().newCommitMessage(newCommitMessage).build()); } /** @@ -255,11 +232,13 @@ public class ChangeEditModifier { Repository repository, ChangeNotes notes, String filePath, RawInput newContent) throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { - modifyTree( + modifyCommit( repository, notes, new ModificationIntention.LatestCommit(), - ImmutableList.of(new ChangeFileContentModification(filePath, newContent))); + CommitModification.builder() + .addTreeModification(new ChangeFileContentModification(filePath, newContent)) + .build()); } /** @@ -278,11 +257,11 @@ public class ChangeEditModifier { public void deleteFile(Repository repository, ChangeNotes notes, String file) throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { - modifyTree( + modifyCommit( repository, notes, new ModificationIntention.LatestCommit(), - ImmutableList.of(new DeleteFileModification(file))); + CommitModification.builder().addTreeModification(new DeleteFileModification(file)).build()); } /** @@ -304,11 +283,13 @@ public class ChangeEditModifier { Repository repository, ChangeNotes notes, String currentFilePath, String newFilePath) throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { - modifyTree( + modifyCommit( repository, notes, new ModificationIntention.LatestCommit(), - ImmutableList.of(new RenameFileModification(currentFilePath, newFilePath))); + CommitModification.builder() + .addTreeModification(new RenameFileModification(currentFilePath, newFilePath)) + .build()); } /** @@ -326,11 +307,13 @@ public class ChangeEditModifier { public void restoreFile(Repository repository, ChangeNotes notes, String file) throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { - modifyTree( + modifyCommit( repository, notes, new ModificationIntention.LatestCommit(), - ImmutableList.of(new RestoreFileModification(file))); + CommitModification.builder() + .addTreeModification(new RestoreFileModification(file)) + .build()); } /** @@ -356,15 +339,20 @@ public class ChangeEditModifier { List treeModifications) throws AuthException, BadRequestException, IOException, InvalidChangeOperationException, PermissionBackendException, ResourceConflictException { - return modifyTree( - repository, notes, new ModificationIntention.PatchsetCommit(patchSet), treeModifications); + return modifyCommit( + repository, + notes, + new ModificationIntention.PatchsetCommit(patchSet), + CommitModification.builder() + .treeModifications(ImmutableList.copyOf(treeModifications)) + .build()); } - private ChangeEdit modifyTree( + private ChangeEdit modifyCommit( Repository repository, ChangeNotes notes, ModificationIntention modificationIntention, - List treeModifications) + CommitModification commitModification) throws AuthException, BadRequestException, IOException, InvalidChangeOperationException, PermissionBackendException, ResourceConflictException { assertCanEdit(notes); @@ -378,21 +366,26 @@ public class ChangeEditModifier { editBehavior.getModificationTarget(notes, modificationIntention); RevCommit commitToModify = modificationTarget.getCommit(repository); - ObjectId newTreeId = createNewTree(repository, commitToModify, treeModifications); + ObjectId newTreeId = + createNewTree(repository, commitToModify, commitModification.treeModifications()); newTreeId = editBehavior.mergeTreesIfNecessary(repository, newTreeId, commitToModify); - Optional unmodifiedEdit = editBehavior.getEditIfNoModification(newTreeId); + PatchSet basePatchset = modificationTarget.getBasePatchset(); + RevCommit basePatchsetCommit = NoteDbEdits.lookupCommit(repository, basePatchset.commitId()); + + String newCommitMessage = + createNewCommitMessage(editBehavior, commitModification, commitToModify); + newCommitMessage = editBehavior.mergeCommitMessageIfNecessary(newCommitMessage, commitToModify); + + Optional unmodifiedEdit = + editBehavior.getEditIfNoModification(newTreeId, newCommitMessage); if (unmodifiedEdit.isPresent()) { return unmodifiedEdit.get(); } - PatchSet basePatchset = modificationTarget.getBasePatchset(); - RevCommit basePatchsetCommit = NoteDbEdits.lookupCommit(repository, basePatchset.commitId()); - - String commitMessage = editBehavior.getNewCommitMessage(commitToModify); Timestamp nowTimestamp = TimeUtil.nowTs(); ObjectId newEditCommit = - createCommit(repository, basePatchsetCommit, newTreeId, commitMessage, nowTimestamp); + createCommit(repository, basePatchsetCommit, newTreeId, newCommitMessage, nowTimestamp); return editBehavior.updateEditInStorage( repository, notes, basePatchset, newEditCommit, nowTimestamp); @@ -428,11 +421,6 @@ public class ChangeEditModifier { return changeEditUtil.byChange(notes); } - private PatchSet getBasePatchSet(Optional optionalChangeEdit, ChangeNotes notes) { - Optional editBasePatchSet = optionalChangeEdit.map(ChangeEdit::getBasePatchSet); - return editBasePatchSet.isPresent() ? editBasePatchSet.get() : lookupCurrentPatchSet(notes); - } - private PatchSet lookupCurrentPatchSet(ChangeNotes notes) { return patchSetUtil.current(notes); } @@ -445,6 +433,10 @@ public class ChangeEditModifier { private static ObjectId createNewTree( Repository repository, RevCommit baseCommit, List treeModifications) throws BadRequestException, IOException, InvalidChangeOperationException { + if (treeModifications.isEmpty()) { + return baseCommit.getTree(); + } + ObjectId newTreeId; try { TreeCreator treeCreator = TreeCreator.basedOn(baseCommit); @@ -477,6 +469,25 @@ public class ChangeEditModifier { return threeWayMerger.getResultTreeId(); } + private String createNewCommitMessage( + EditBehavior editBehavior, CommitModification commitModification, RevCommit commitToModify) + throws InvalidChangeOperationException, BadRequestException { + if (!commitModification.newCommitMessage().isPresent()) { + return editBehavior.getUnmodifiedCommitMessage(commitToModify); + } + + String newCommitMessage = + CommitMessageUtil.checkAndSanitizeCommitMessage( + commitModification.newCommitMessage().get()); + + if (newCommitMessage.equals(commitToModify.getFullMessage())) { + throw new InvalidChangeOperationException( + "New commit message cannot be same as existing commit message"); + } + + return newCommitMessage; + } + private ObjectId createCommit( Repository repository, RevCommit basePatchsetCommit, @@ -516,9 +527,11 @@ public class ChangeEditModifier { Repository repository, ObjectId newTreeId, ObjectId commitToModify) throws IOException, MergeConflictException; - String getNewCommitMessage(RevCommit commitToModify); + String getUnmodifiedCommitMessage(RevCommit commitToModify); - Optional getEditIfNoModification(ObjectId newTreeId); + String mergeCommitMessageIfNecessary(String newCommitMessage, ObjectId commitToModify); + + Optional getEditIfNoModification(ObjectId newTreeId, String newCommitMessage); ChangeEdit updateEditInStorage( Repository repository, @@ -561,17 +574,35 @@ public class ChangeEditModifier { } @Override - public String getNewCommitMessage(RevCommit commitToModify) { + public String getUnmodifiedCommitMessage(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); + public String mergeCommitMessageIfNecessary(String newCommitMessage, ObjectId commitToModify) { + if (ObjectId.isEqual(changeEdit.getEditCommit(), commitToModify)) { + return newCommitMessage; } - return Optional.empty(); + String editCommitMessage = changeEdit.getEditCommit().getFullMessage(); + if (editCommitMessage.equals(newCommitMessage)) { + return editCommitMessage; + } + // Adjusted in follow-up change. + throw new UnsupportedOperationException( + "Merging different commit messages is not supported."); + } + + @Override + public Optional getEditIfNoModification( + ObjectId newTreeId, String newCommitMessage) { + if (!ObjectId.isEqual(newTreeId, changeEdit.getEditCommit().getTree())) { + return Optional.empty(); + } + if (!Objects.equals(newCommitMessage, changeEdit.getEditCommit().getFullMessage())) { + return Optional.empty(); + } + // Modifications are already contained in the change edit. + return Optional.of(changeEdit); } @Override @@ -613,12 +644,18 @@ public class ChangeEditModifier { } @Override - public String getNewCommitMessage(RevCommit commitToModify) { + public String getUnmodifiedCommitMessage(RevCommit commitToModify) { return commitToModify.getFullMessage(); } @Override - public Optional getEditIfNoModification(ObjectId newTreeId) { + public String mergeCommitMessageIfNecessary(String newCommitMessage, ObjectId commitToModify) { + return newCommitMessage; + } + + @Override + public Optional getEditIfNoModification( + ObjectId newTreeId, String newCommitMessage) { return Optional.empty(); } diff --git a/java/com/google/gerrit/server/edit/CommitModification.java b/java/com/google/gerrit/server/edit/CommitModification.java new file mode 100644 index 0000000..f9ed58e --- /dev/null +++ b/java/com/google/gerrit/server/edit/CommitModification.java @@ -0,0 +1,48 @@ +// 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.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import com.google.gerrit.server.edit.tree.TreeModification; +import java.util.Optional; + +@AutoValue +public abstract class CommitModification { + + public abstract ImmutableList treeModifications(); + + public abstract Optional newCommitMessage(); + + public static Builder builder() { + return new AutoValue_CommitModification.Builder(); + } + + @AutoValue.Builder + public abstract static class Builder { + public Builder addTreeModification(TreeModification treeModification) { + treeModificationsBuilder().add(treeModification); + return this; + } + + abstract ImmutableList.Builder treeModificationsBuilder(); + + public abstract Builder treeModifications(ImmutableList treeModifications); + + public abstract Builder newCommitMessage(String newCommitMessage); + + public abstract CommitModification build(); + } +} diff --git a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java index 9a25f52..7a15a1d 100644 --- a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java +++ b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java @@ -56,7 +56,6 @@ import com.google.gerrit.server.edit.ChangeEdit; import com.google.gerrit.server.edit.ChangeEditJson; import com.google.gerrit.server.edit.ChangeEditModifier; import com.google.gerrit.server.edit.ChangeEditUtil; -import com.google.gerrit.server.edit.UnchangedCommitMessageException; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -482,7 +481,7 @@ public class ChangeEdits implements ChildCollection