From 315f1cfa5c63f4d3355704c80d00ac1323a36171 Mon Sep 17 00:00:00 2001 From: Robin Rosenberg Date: Wed, 24 Oct 2012 00:48:31 +0200 Subject: [PATCH] Update the revert command and things relating to revert Cherry-pick has been fixed, but even though revert does basically the same thing, the fixes were not carried over here. - Recognize the revert-states, analogous to the cherry picking states - Make reset handle a revert-in-progress - Update REVERT_HEAD and MERGE_MSG when revert fails due to conflicts - Clear revert state on commit and reset - Format the message similarily to how cherry-pick does. This is not exactly how C Git does it. The interface is still not the same as for cherry-picking. Change-Id: I8ea956fcbc9526d62a2365360feea23a9280eba3 Signed-off-by: Chris Aniszczyk --- .../eclipse/jgit/api/RevertCommandTest.java | 208 ++++++++++++++++++ .../eclipse/jgit/internal/JGitText.properties | 2 +- .../org/eclipse/jgit/api/CommitCommand.java | 3 + .../org/eclipse/jgit/api/ResetCommand.java | 9 + .../org/eclipse/jgit/api/RevertCommand.java | 60 ++++- .../src/org/eclipse/jgit/lib/Constants.java | 3 + .../src/org/eclipse/jgit/lib/Repository.java | 50 +++++ .../org/eclipse/jgit/lib/RepositoryState.java | 40 ++++ 8 files changed, 367 insertions(+), 8 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RevertCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RevertCommandTest.java index 11cef635a..a36a28c6b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RevertCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RevertCommandTest.java @@ -48,10 +48,17 @@ import java.io.File; import java.io.IOException; import java.util.Iterator; +import org.eclipse.jgit.api.MergeResult.MergeStatus; +import org.eclipse.jgit.api.ResetCommand.ResetType; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.api.errors.JGitInternalException; +import org.eclipse.jgit.dircache.DirCache; +import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.FileMode; +import org.eclipse.jgit.lib.RepositoryState; import org.eclipse.jgit.lib.RepositoryTestCase; +import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.storage.file.ReflogReader; import org.junit.Test; @@ -111,4 +118,205 @@ public class RevertCommandTest extends RepositoryTestCase { .startsWith("revert: Revert \"")); } + + @Test + public void testRevertDirtyIndex() throws Exception { + Git git = new Git(db); + RevCommit sideCommit = prepareRevert(git); + + // modify and add file a + writeTrashFile("a", "a(modified)"); + git.add().addFilepattern("a").call(); + // do not commit + + doRevertAndCheckResult(git, sideCommit, + MergeFailureReason.DIRTY_INDEX); + } + + @Test + public void testRevertDirtyWorktree() throws Exception { + Git git = new Git(db); + RevCommit sideCommit = prepareRevert(git); + + // modify file a + writeTrashFile("a", "a(modified)"); + // do not add and commit + + doRevertAndCheckResult(git, sideCommit, + MergeFailureReason.DIRTY_WORKTREE); + } + + @Test + public void testRevertConflictResolution() throws Exception { + Git git = new Git(db); + RevCommit sideCommit = prepareRevert(git); + + RevertCommand revert = git.revert(); + RevCommit newHead = revert.include(sideCommit.getId()).call(); + assertNull(newHead); + MergeResult result = revert.getFailingResult(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + assertTrue(new File(db.getDirectory(), Constants.MERGE_MSG).exists()); + assertEquals("Revert \"" + sideCommit.getShortMessage() + + "\"\n\nThis reverts commit " + sideCommit.getId().getName() + + ".\n\nConflicts:\n\ta\n", + db.readMergeCommitMsg()); + assertTrue(new File(db.getDirectory(), Constants.REVERT_HEAD) + .exists()); + assertEquals(sideCommit.getId(), db.readRevertHead()); + assertEquals(RepositoryState.REVERTING, db.getRepositoryState()); + + // Resolve + writeTrashFile("a", "a"); + git.add().addFilepattern("a").call(); + + assertEquals(RepositoryState.REVERTING_RESOLVED, + db.getRepositoryState()); + + git.commit().setOnly("a").setMessage("resolve").call(); + + assertEquals(RepositoryState.SAFE, db.getRepositoryState()); + } + + @Test + public void testRevertkConflictReset() throws Exception { + Git git = new Git(db); + + RevCommit sideCommit = prepareRevert(git); + + RevertCommand revert = git.revert(); + RevCommit newHead = revert.include(sideCommit.getId()).call(); + assertNull(newHead); + MergeResult result = revert.getFailingResult(); + + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + assertEquals(RepositoryState.REVERTING, db.getRepositoryState()); + assertTrue(new File(db.getDirectory(), Constants.REVERT_HEAD) + .exists()); + + git.reset().setMode(ResetType.MIXED).setRef("HEAD").call(); + + assertEquals(RepositoryState.SAFE, db.getRepositoryState()); + assertFalse(new File(db.getDirectory(), Constants.REVERT_HEAD) + .exists()); + } + + @Test + public void testRevertOverExecutableChangeOnNonExectuableFileSystem() + throws Exception { + Git git = new Git(db); + File file = writeTrashFile("test.txt", "a"); + assertNotNull(git.add().addFilepattern("test.txt").call()); + assertNotNull(git.commit().setMessage("commit1").call()); + + assertNotNull(git.checkout().setCreateBranch(true).setName("a").call()); + + writeTrashFile("test.txt", "b"); + assertNotNull(git.add().addFilepattern("test.txt").call()); + RevCommit commit2 = git.commit().setMessage("commit2").call(); + assertNotNull(commit2); + + assertNotNull(git.checkout().setName(Constants.MASTER).call()); + + DirCache cache = db.lockDirCache(); + cache.getEntry("test.txt").setFileMode(FileMode.EXECUTABLE_FILE); + cache.write(); + assertTrue(cache.commit()); + cache.unlock(); + + assertNotNull(git.commit().setMessage("commit3").call()); + + db.getFS().setExecute(file, false); + git.getRepository() + .getConfig() + .setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_FILEMODE, false); + + RevertCommand revert = git.revert(); + RevCommit newHead = revert.include(commit2).call(); + assertNotNull(newHead); + } + + @Test + public void testRevertConflictMarkers() throws Exception { + Git git = new Git(db); + RevCommit sideCommit = prepareRevert(git); + + RevertCommand revert = git.revert(); + RevCommit newHead = revert.include(sideCommit.getId()) + .call(); + assertNull(newHead); + MergeResult result = revert.getFailingResult(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + + String expected = "<<<<<<< master\na(latest)\n=======\na\n>>>>>>> ca96c31 second master\n"; + checkFile(new File(db.getWorkTree(), "a"), expected); + } + + @Test + public void testRevertOurCommitName() throws Exception { + Git git = new Git(db); + RevCommit sideCommit = prepareRevert(git); + + RevertCommand revert = git.revert(); + RevCommit newHead = revert.include(sideCommit.getId()) + .setOurCommitName("custom name").call(); + assertNull(newHead); + MergeResult result = revert.getFailingResult(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + + String expected = "<<<<<<< custom name\na(latest)\n=======\na\n>>>>>>> ca96c31 second master\n"; + checkFile(new File(db.getWorkTree(), "a"), expected); + } + + private RevCommit prepareRevert(final Git git) throws Exception { + // create, add and commit file a + writeTrashFile("a", "a"); + git.add().addFilepattern("a").call(); + git.commit().setMessage("first master").call(); + + // First commit + checkoutBranch("refs/heads/master"); + // modify, add and commit file a + writeTrashFile("a", "a(previous)"); + git.add().addFilepattern("a").call(); + RevCommit oldCommit = git.commit().setMessage("second master").call(); + + // modify, add and commit file a + writeTrashFile("a", "a(latest)"); + git.add().addFilepattern("a").call(); + git.commit().setMessage("side").call(); + + return oldCommit; + } + + private void doRevertAndCheckResult(final Git git, + final RevCommit sideCommit, final MergeFailureReason reason) + throws Exception { + // get current index state + String indexState = indexState(CONTENT); + + // revert + RevertCommand revert = git.revert(); + RevCommit resultCommit = revert.include(sideCommit.getId()).call(); + assertNull(resultCommit); + MergeResult result = revert.getFailingResult(); + assertEquals(MergeStatus.FAILED, result.getMergeStatus()); + // staged file a causes DIRTY_INDEX + assertEquals(1, result.getFailingPaths().size()); + assertEquals(reason, result.getFailingPaths().get("a")); + assertEquals("a(modified)", read(new File(db.getWorkTree(), "a"))); + // index shall be unchanged + assertEquals(indexState, indexState(CONTENT)); + assertEquals(RepositoryState.SAFE, db.getRepositoryState()); + + if (reason == null) { + ReflogReader reader = db.getReflogReader(Constants.HEAD); + assertTrue(reader.getLastEntry().getComment() + .startsWith("revert: ")); + reader = db.getReflogReader(db.getBranch()); + assertTrue(reader.getLastEntry().getComment() + .startsWith("revert: ")); + } + } } diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index b4ba2e88a..bccc3736f 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -73,7 +73,7 @@ cannotStoreObjects=cannot store objects cannotUnloadAModifiedTree=Cannot unload a modified tree. cannotWorkWithOtherStagesThanZeroRightNow=Cannot work with other stages than zero right now. Won't write corrupt index. canOnlyCherryPickCommitsWithOneParent=Cannot cherry-pick commit ''{0}'' because it has {1} parents, only commits with exactly one parent are supported. -canOnlyRevertCommitsWithOneParent=Can only revert commits which have exactly one parent +canOnlyRevertCommitsWithOneParent=Cannot revert commit ''{0}'' because it has {1} parents, only commits with exactly one parent are supported cantFindObjectInReversePackIndexForTheSpecifiedOffset=Can't find object in (reverse) pack index for the specified offset {0} cantPassMeATree=Can't pass me a tree! channelMustBeInRange0_255=channel {0} must be in range [0, 255] diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java index 5f559bcbf..f51b301cd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java @@ -249,6 +249,9 @@ public class CommitCommand extends GitCommand { } else if (state == RepositoryState.CHERRY_PICKING_RESOLVED) { repo.writeMergeCommitMsg(null); repo.writeCherryPickHead(null); + } else if (state == RepositoryState.REVERTING_RESOLVED) { + repo.writeMergeCommitMsg(null); + repo.writeRevertHead(null); } return revCommit; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/ResetCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/ResetCommand.java index 80abbec2e..6827026ba 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/ResetCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/ResetCommand.java @@ -149,6 +149,8 @@ public class ResetCommand extends GitCommand { final boolean cherryPicking = state .equals(RepositoryState.CHERRY_PICKING) || state.equals(RepositoryState.CHERRY_PICKING_RESOLVED); + final boolean reverting = state.equals(RepositoryState.REVERTING) + || state.equals(RepositoryState.REVERTING_RESOLVED); // resolve the ref to a commit final ObjectId commitId; @@ -219,6 +221,8 @@ public class ResetCommand extends GitCommand { resetMerge(); else if (cherryPicking) resetCherryPick(); + else if (reverting) + resetRevert(); else if (repo.readSquashCommitMsg() != null) repo.writeSquashCommitMsg(null /* delete */); } @@ -376,4 +380,9 @@ public class ResetCommand extends GitCommand { repo.writeMergeCommitMsg(null); } + private void resetRevert() throws IOException { + repo.writeRevertHead(null); + repo.writeMergeCommitMsg(null); + } + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java index 910637d0e..16522b74d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java @@ -66,6 +66,7 @@ import org.eclipse.jgit.lib.ObjectIdRef; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Ref.Storage; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.merge.MergeMessageFormatter; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ResolveMerger; import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; @@ -86,6 +87,8 @@ import org.eclipse.jgit.treewalk.FileTreeIterator; public class RevertCommand extends GitCommand { private List commits = new LinkedList(); + private String ourCommitName = null; + private List revertedRefs = new LinkedList(); private MergeResult failingResult; @@ -143,18 +146,31 @@ public class RevertCommand extends GitCommand { RevCommit srcCommit = revWalk.parseCommit(srcObjectId); // get the parent of the commit to revert - if (srcCommit.getParentCount() != 1) { + if (srcCommit.getParentCount() != 1) throw new MultipleParentsNotAllowedException( - JGitText.get().canOnlyRevertCommitsWithOneParent); - } + MessageFormat.format( + JGitText.get().canOnlyRevertCommitsWithOneParent, + srcCommit.name(), + Integer.valueOf(srcCommit.getParentCount()))); + RevCommit srcParent = srcCommit.getParent(0); revWalk.parseHeaders(srcParent); + String ourName = calculateOurName(headRef); + String revertName = srcCommit.getId().abbreviate(7).name() + + " " + srcCommit.getShortMessage(); //$NON-NLS-1$ + ResolveMerger merger = (ResolveMerger) MergeStrategy.RESOLVE .newMerger(repo); merger.setWorkingTreeIterator(new FileTreeIterator(repo)); merger.setBase(srcCommit.getTree()); + merger.setCommitNames(new String[] { "BASE", ourName, revertName }); //$NON-NLS-1$ //$NON-NLS-2$ + String shortMessage = "Revert \"" + srcCommit.getShortMessage() //$NON-NLS-1$ + + "\""; //$NON-NLS-1$ + String newMessage = shortMessage + "\n\n" + + "This reverts commit " + srcCommit.getId().getName() //$NON-NLS-1$ + + ".\n"; //$NON-NLS-1$ if (merger.merge(headCommit, srcParent)) { if (AnyObjectId.equals(headCommit.getTree().getId(), merger .getResultTreeId())) @@ -164,10 +180,6 @@ public class RevertCommand extends GitCommand { merger.getResultTreeId()); dco.setFailOnConflict(true); dco.checkout(); - String shortMessage = "Revert \"" + srcCommit.getShortMessage() + "\""; //$NON-NLS-2$ - String newMessage = shortMessage + "\n\n" //$NON-NLS-1$ - + "This reverts commit " //$NON-NLS-1$ - + srcCommit.getId().getName() + ".\n"; //$NON-NLS-1$ newHead = new Git(getRepository()).commit() .setMessage(newMessage) .setReflogComment("revert: " + shortMessage).call(); //$NON-NLS-1$ @@ -183,6 +195,20 @@ public class RevertCommand extends GitCommand { srcParent.getId() }, MergeStatus.FAILED, MergeStrategy.RESOLVE, merger.getMergeResults(), failingPaths, null); + else + failingResult = new MergeResult(null, + merger.getBaseCommit(0, 1), + new ObjectId[] { headCommit.getId(), + srcParent.getId() }, + MergeStatus.CONFLICTING, MergeStrategy.RESOLVE, + merger.getMergeResults(), failingPaths, null); + if (!merger.failed() && !unmergedPaths.isEmpty()) { + String message = new MergeMessageFormatter() + .formatWithConflicts(newMessage, + merger.getUnmergedPaths()); + repo.writeRevertHead(srcCommit.getId()); + repo.writeMergeCommitMsg(message); + } return null; } } @@ -230,6 +256,26 @@ public class RevertCommand extends GitCommand { commit.copy())); } + /** + * @param ourCommitName + * the name that should be used in the "OURS" place for conflict + * markers + * @return {@code this} + */ + public RevertCommand setOurCommitName(String ourCommitName) { + this.ourCommitName = ourCommitName; + return this; + } + + private String calculateOurName(Ref headRef) { + if (ourCommitName != null) + return ourCommitName; + + String targetRefName = headRef.getTarget().getName(); + String headName = Repository.shortenRefName(targetRefName); + return headName; + } + /** * @return the list of successfully reverted {@link Ref}'s. Never * null but maybe an empty list if no commit was diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java index 262f7ce7f..907742ee8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java @@ -560,6 +560,9 @@ public final class Constants { /** name of the file containing the commit msg for a squash commit */ public static final String SQUASH_MSG = "SQUASH_MSG"; + /** name of the file containing the ID of a revert commit in case of conflicts */ + public static final String REVERT_HEAD = "REVERT_HEAD"; + /** * name of the ref ORIG_HEAD used by certain commands to store the original * value of HEAD diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java index 9b2b8a7f0..75053796b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java @@ -1119,6 +1119,19 @@ public abstract class Repository { return RepositoryState.CHERRY_PICKING; } + if (new File(getDirectory(), Constants.REVERT_HEAD).exists()) { + try { + if (!readDirCache().hasUnmergedPaths()) { + // no unmerged paths + return RepositoryState.REVERTING_RESOLVED; + } + } catch (IOException e) { + // fall through to REVERTING + } + + return RepositoryState.REVERTING; + } + return RepositoryState.SAFE; } @@ -1362,6 +1375,27 @@ public abstract class Repository { return ObjectId.fromString(raw, 0); } + /** + * Return the information stored in the file $GIT_DIR/REVERT_HEAD. + * + * @return object id from REVERT_HEAD file or {@code null} if this file + * doesn't exist. Also if the file exists but is empty {@code null} + * will be returned + * @throws IOException + * @throws NoWorkTreeException + * if this is bare, which implies it has no working directory. + * See {@link #isBare()}. + */ + public ObjectId readRevertHead() throws IOException, NoWorkTreeException { + if (isBare() || getDirectory() == null) + throw new NoWorkTreeException(); + + byte[] raw = readGitDirectoryFile(Constants.REVERT_HEAD); + if (raw == null) + return null; + return ObjectId.fromString(raw, 0); + } + /** * Write cherry pick commit into $GIT_DIR/CHERRY_PICK_HEAD. This is used in * case of conflicts to store the cherry which was tried to be picked. @@ -1377,6 +1411,21 @@ public abstract class Repository { writeHeadsFile(heads, Constants.CHERRY_PICK_HEAD); } + /** + * Write revert commit into $GIT_DIR/REVERT_HEAD. This is used in case of + * conflicts to store the revert which was tried to be picked. + * + * @param head + * an object id of the revert commit or null to + * delete the file + * @throws IOException + */ + public void writeRevertHead(ObjectId head) throws IOException { + List heads = (head != null) ? Collections.singletonList(head) + : null; + writeHeadsFile(heads, Constants.REVERT_HEAD); + } + /** * Write original HEAD commit into $GIT_DIR/ORIG_HEAD. * @@ -1514,4 +1563,5 @@ public abstract class Repository { FileUtils.delete(headsFile, FileUtils.SKIP_MISSING); } } + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryState.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryState.java index 7e3ba51da..937c76e20 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryState.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryState.java @@ -173,6 +173,46 @@ public enum RepositoryState { public String getDescription() { return JGitText.get().repositoryState_merged; } }, + /** An unfinished revert. Must resolve or reset before continuing normally + */ + REVERTING { + @Override + public boolean canCheckout() { return false; } + + @Override + public boolean canResetHead() { return true; } + + @Override + public boolean canCommit() { return false; } + + @Override + public boolean canAmend() { return false; } + + @Override + public String getDescription() { return JGitText.get().repositoryState_conflicts; } + }, + + /** + * A revert where all conflicts have been resolved. The index does not + * contain any unmerged paths. + */ + REVERTING_RESOLVED { + @Override + public boolean canCheckout() { return true; } + + @Override + public boolean canResetHead() { return true; } + + @Override + public boolean canCommit() { return true; } + + @Override + public boolean canAmend() { return false; } + + @Override + public String getDescription() { return JGitText.get().repositoryState_merged; } + }, + /** * An unfinished rebase or am. Must resolve, skip or abort before normal work can take place */