From 591998c2d628ec4f6309caea826fab16a6de2adc Mon Sep 17 00:00:00 2001 From: Stefan Lay Date: Wed, 27 Nov 2013 11:01:47 +0100 Subject: [PATCH] Do not allow non-ff-rebase if there are uncommitted changes With this change jgit checks for uncommitted changes before a rebase is started. This is also done by native git. One reason is that an abort would override such changes. The check is skipped for a non-interactive rebase when it will result in a fast-forward. In this case there can be only checkout conflicts but no merge conflicts, so there cannot be an abort which overrides uncommitted changes. Bug: 422352 Change-Id: I1e0b59b2a4d80a686b67a6729e441924362b1236 Signed-off-by: Stefan Lay Signed-off-by: Matthias Sohn --- .../eclipse/jgit/api/RebaseCommandTest.java | 180 +++++++++++++++--- .../org/eclipse/jgit/api/RebaseCommand.java | 12 ++ .../org/eclipse/jgit/api/RebaseResult.java | 39 ++++ .../src/org/eclipse/jgit/api/Status.java | 44 ++++- 4 files changed, 240 insertions(+), 35 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java index 2c9fce818..b33ad6ba5 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java @@ -80,7 +80,6 @@ import org.eclipse.jgit.lib.ReflogEntry; import org.eclipse.jgit.lib.RepositoryState; import org.eclipse.jgit.merge.MergeStrategy; -import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.util.FileUtils; @@ -1200,9 +1199,9 @@ public void testRebaseWithUnstagedTopicChange() throws Exception { // rebase RebaseResult result = git.rebase().setUpstream("refs/heads/master") .call(); - assertEquals(Status.CONFLICTS, result.getStatus()); - assertEquals(1, result.getConflicts().size()); - assertEquals("file2", result.getConflicts().get(0)); + assertEquals(Status.UNCOMMITTED_CHANGES, result.getStatus()); + assertEquals(1, result.getUncommittedChanges().size()); + assertEquals("file2", result.getUncommittedChanges().get(0)); } @Test @@ -1233,9 +1232,9 @@ public void testRebaseWithUncommittedTopicChange() throws Exception { RebaseResult result = git.rebase().setUpstream("refs/heads/master") .call(); - assertEquals(Status.CONFLICTS, result.getStatus()); - assertEquals(1, result.getConflicts().size()); - assertEquals("file2", result.getConflicts().get(0)); + assertEquals(Status.UNCOMMITTED_CHANGES, result.getStatus()); + assertEquals(1, result.getUncommittedChanges().size()); + assertEquals("file2", result.getUncommittedChanges().get(0)); checkFile(uncommittedFile, "uncommitted file2"); assertEquals(RepositoryState.SAFE, git.getRepository().getRepositoryState()); @@ -1268,9 +1267,9 @@ public void testRebaseWithUnstagedMasterChange() throws Exception { // rebase RebaseResult result = git.rebase().setUpstream("refs/heads/master") .call(); - assertEquals(Status.CONFLICTS, result.getStatus()); - assertEquals(1, result.getConflicts().size()); - assertEquals(FILE1, result.getConflicts().get(0)); + assertEquals(Status.UNCOMMITTED_CHANGES, result.getStatus()); + assertEquals(1, result.getUncommittedChanges().size()); + assertEquals(FILE1, result.getUncommittedChanges().get(0)); } @Test @@ -1302,9 +1301,9 @@ public void testRebaseWithUncommittedMasterChange() throws Exception { // rebase RebaseResult result = git.rebase().setUpstream("refs/heads/master") .call(); - assertEquals(Status.CONFLICTS, result.getStatus()); - assertEquals(1, result.getConflicts().size()); - assertEquals(FILE1, result.getConflicts().get(0)); + assertEquals(Status.UNCOMMITTED_CHANGES, result.getStatus()); + assertEquals(1, result.getUncommittedChanges().size()); + assertEquals(FILE1, result.getUncommittedChanges().get(0)); } @Test @@ -1333,7 +1332,8 @@ public void testRebaseWithUnstagedMasterChangeBaseCommit() throws Exception { writeTrashFile("file0", "unstaged modified file0"); // rebase - assertEquals(Status.OK, git.rebase().setUpstream("refs/heads/master") + assertEquals(Status.UNCOMMITTED_CHANGES, + git.rebase().setUpstream("refs/heads/master") .call().getStatus()); } @@ -1371,12 +1371,8 @@ public void testRebaseWithUncommittedMasterChangeBaseCommit() // rebase RebaseResult result = git.rebase().setUpstream("refs/heads/master") .call(); - assertEquals(Status.FAILED, result.getStatus()); - // staged file0 causes DIRTY_INDEX - assertEquals(1, result.getFailingPaths().size()); - assertEquals(MergeFailureReason.DIRTY_INDEX, result.getFailingPaths() - .get("file0")); - assertEquals("unstaged modified file0", read(file0)); + assertEquals(Status.UNCOMMITTED_CHANGES, result.getStatus()); + assertEquals(1, result.getUncommittedChanges().size()); // index shall be unchanged assertEquals(indexState, indexState(CONTENT)); assertEquals(RepositoryState.SAFE, db.getRepositoryState()); @@ -1412,7 +1408,8 @@ public void testRebaseWithUnstagedMasterChangeOtherCommit() writeTrashFile("file0", "unstaged modified file0"); // rebase - assertEquals(Status.OK, git.rebase().setUpstream("refs/heads/master") + assertEquals(Status.UNCOMMITTED_CHANGES, + git.rebase().setUpstream("refs/heads/master") .call().getStatus()); } @@ -1453,17 +1450,91 @@ public void testRebaseWithUncommittedMasterChangeOtherCommit() // rebase RebaseResult result = git.rebase().setUpstream("refs/heads/master") .call(); - assertEquals(Status.FAILED, result.getStatus()); + assertEquals(Status.UNCOMMITTED_CHANGES, result.getStatus()); // staged file0 causes DIRTY_INDEX - assertEquals(1, result.getFailingPaths().size()); - assertEquals(MergeFailureReason.DIRTY_INDEX, result.getFailingPaths() - .get("file0")); + assertEquals(1, result.getUncommittedChanges().size()); assertEquals("unstaged modified file0", read(file0)); // index shall be unchanged assertEquals(indexState, indexState(CONTENT)); assertEquals(RepositoryState.SAFE, db.getRepositoryState()); } + @Test + public void testFastForwardRebaseWithModification() throws Exception { + // create file0 + file1, add and commit + writeTrashFile("file0", "file0"); + writeTrashFile(FILE1, "file1"); + git.add().addFilepattern("file0").addFilepattern(FILE1).call(); + RevCommit commit = git.commit().setMessage("commit1").call(); + + // create topic branch + createBranch(commit, "refs/heads/topic"); + + // still on master / modify file1, add and commit + writeTrashFile(FILE1, "modified file1"); + git.add().addFilepattern(FILE1).call(); + git.commit().setMessage("commit2").call(); + + // checkout topic branch / modify file0 and add to index + checkoutBranch("refs/heads/topic"); + writeTrashFile("file0", "modified file0 in index"); + git.add().addFilepattern("file0").addFilepattern(FILE1).call(); + // modify once more + writeTrashFile("file0", "modified file0"); + + // rebase + RebaseResult result = git.rebase().setUpstream("refs/heads/master") + .call(); + assertEquals(Status.FAST_FORWARD, result.getStatus()); + checkFile(new File(db.getWorkTree(), "file0"), "modified file0"); + checkFile(new File(db.getWorkTree(), FILE1), "modified file1"); + assertEquals("[file0, mode:100644, content:modified file0 in index]" + + "[file1, mode:100644, content:modified file1]", + indexState(CONTENT)); + assertEquals(RepositoryState.SAFE, db.getRepositoryState()); + } + + @Test + public void testRebaseWithModificationShouldNotDeleteData() + throws Exception { + // create file0 + file1, add and commit + writeTrashFile("file0", "file0"); + writeTrashFile(FILE1, "file1"); + git.add().addFilepattern("file0").addFilepattern(FILE1).call(); + RevCommit commit = git.commit().setMessage("commit1").call(); + + // create topic branch + createBranch(commit, "refs/heads/topic"); + + // still on master / modify file1, add and commit + writeTrashFile(FILE1, "modified file1"); + git.add().addFilepattern(FILE1).call(); + git.commit().setMessage("commit2").call(); + + // checkout topic branch / modify file1, add and commit + checkoutBranch("refs/heads/topic"); + writeTrashFile(FILE1, "modified file1 on topic"); + git.add().addFilepattern(FILE1).call(); + git.commit().setMessage("commit3").call(); + + writeTrashFile("file0", "modified file0"); + + RebaseResult result = git.rebase().setUpstream("refs/heads/master") + .call(); + // the following condition was true before commit 83b6ab233: + // jgit started the rebase and deleted the change on abort + // This test should verify that content was deleted + if (result.getStatus() == Status.STOPPED) + git.rebase().setOperation(Operation.ABORT).call(); + + checkFile(new File(db.getWorkTree(), "file0"), "modified file0"); + checkFile(new File(db.getWorkTree(), FILE1), + "modified file1 on topic"); + assertEquals("[file0, mode:100644, content:file0]" + + "[file1, mode:100644, content:modified file1 on topic]", + indexState(CONTENT)); + } + @Test public void testRebaseWithUncommittedDelete() throws Exception { // create file0 + file1, add and commit @@ -1595,9 +1666,9 @@ public void testRebaseShouldLeaveWorkspaceUntouchedWithUnstagedChangesConflict() // and attempt to rebase RebaseResult rebaseResult = git.rebase() .setUpstream("refs/heads/master").call(); - assertEquals(Status.CONFLICTS, rebaseResult.getStatus()); - assertEquals(1, rebaseResult.getConflicts().size()); - assertEquals(FILE1, rebaseResult.getConflicts().get(0)); + assertEquals(Status.UNCOMMITTED_CHANGES, rebaseResult.getStatus()); + assertEquals(1, rebaseResult.getUncommittedChanges().size()); + assertEquals(FILE1, rebaseResult.getUncommittedChanges().get(0)); checkFile(theFile, "dirty the file"); @@ -2585,6 +2656,59 @@ public String modifyCommitMessage(String commit) { } + @Test + public void testInteractiveRebaseWithModificationShouldNotDeleteDataOnAbort() + throws Exception { + // create file0 + file1, add and commit + writeTrashFile("file0", "file0"); + writeTrashFile(FILE1, "file1"); + git.add().addFilepattern("file0").addFilepattern(FILE1).call(); + git.commit().setMessage("commit1").call(); + + // modify file1, add and commit + writeTrashFile(FILE1, "modified file1"); + git.add().addFilepattern(FILE1).call(); + git.commit().setMessage("commit2").call(); + + // modify file1, add and commit + writeTrashFile(FILE1, "modified file1 a second time"); + git.add().addFilepattern(FILE1).call(); + git.commit().setMessage("commit3").call(); + + // modify file0, but do not commit + writeTrashFile("file0", "modified file0 in index"); + git.add().addFilepattern("file0").addFilepattern(FILE1).call(); + // do not commit + writeTrashFile("file0", "modified file0"); + + // start rebase + RebaseResult result = git.rebase().setUpstream("HEAD~2") + .runInteractively(new InteractiveHandler() { + + public void prepareSteps(List steps) { + steps.get(0).setAction(Action.EDIT); + steps.get(1).setAction(Action.PICK); + } + + public String modifyCommitMessage(String commit) { + return commit; + } + }).call(); + // the following condition was true before commit 83b6ab233: + // jgit started the rebase and deleted the change on abort + // This test should verify that content was deleted + if (result.getStatus() == Status.EDIT) + git.rebase().setOperation(Operation.ABORT).call(); + + checkFile(new File(db.getWorkTree(), "file0"), "modified file0"); + checkFile(new File(db.getWorkTree(), "file1"), + "modified file1 a second time"); + assertEquals("[file0, mode:100644, content:modified file0 in index]" + + "[file1, mode:100644, content:modified file1 a second time]", + indexState(CONTENT)); + + } + private File getTodoFile() { File todoFile = new File(db.getDirectory(), GIT_REBASE_TODO); return todoFile; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java index 3bbac4a85..55cf001c6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java @@ -257,6 +257,18 @@ public RebaseResult call() throws GitAPIException, NoHeadException, .resolve(upstreamCommitId)); break; case BEGIN: + if (stopAfterInitialization + || !walk.isMergedInto( + walk.parseCommit(repo.resolve(Constants.HEAD)), + upstreamCommit)) { + org.eclipse.jgit.api.Status status = Git.wrap(repo) + .status().call(); + if (status.hasUncommittedChanges()) { + List list = new ArrayList(); + list.addAll(status.getUncommittedChanges()); + return RebaseResult.uncommittedChanges(list); + } + } RebaseResult res = initFilesAndRewind(); if (stopAfterInitialization) return RebaseResult.INTERACTIVE_PREPARED_RESULT; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseResult.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseResult.java index 0587b4301..26d040342 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseResult.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseResult.java @@ -104,6 +104,18 @@ public boolean isSuccessful() { return false; } }, + /** + * The repository contains uncommitted changes and the rebase is not a + * fast-forward + * + * @since 3.2 + */ + UNCOMMITTED_CHANGES { + @Override + public boolean isSuccessful() { + return false; + } + }, /** * Conflicts: checkout of target HEAD failed */ @@ -185,6 +197,8 @@ public boolean isSuccessful() { private List conflicts; + private List uncommittedChanges; + private RebaseResult(Status status) { this.status = status; currentCommit = null; @@ -235,6 +249,20 @@ static RebaseResult conflicts(List conflicts) { return result; } + /** + * Create RebaseResult with status + * {@link Status#UNCOMMITTED_CHANGES} + * + * @param uncommittedChanges + * the list of paths + * @return the RebaseResult + */ + static RebaseResult uncommittedChanges(List uncommittedChanges) { + RebaseResult result = new RebaseResult(Status.UNCOMMITTED_CHANGES); + result.uncommittedChanges = uncommittedChanges; + return result; + } + /** * @return the overall status */ @@ -265,4 +293,15 @@ public Map getFailingPaths() { public List getConflicts() { return conflicts; } + + /** + * @return the list of uncommitted changes if status is + * {@link Status#UNCOMMITTED_CHANGES} + * + * @since 3.2 + */ + public List getUncommittedChanges() { + return uncommittedChanges; + } + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/Status.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/Status.java index e840c2f60..c3fcd8bfe 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/Status.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/Status.java @@ -43,6 +43,7 @@ package org.eclipse.jgit.api; import java.util.Collections; +import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -66,19 +67,22 @@ public class Status { private final boolean clean; + private final boolean hasUncommittedChanges;; + /** * @param diff */ public Status(IndexDiff diff) { super(); this.diff = diff; - clean = diff.getAdded().isEmpty() // - && diff.getChanged().isEmpty() // - && diff.getRemoved().isEmpty() // - && diff.getMissing().isEmpty() // - && diff.getModified().isEmpty() // - && diff.getUntracked().isEmpty() // - && diff.getConflicting().isEmpty(); + hasUncommittedChanges = !diff.getAdded().isEmpty() // + || !diff.getChanged().isEmpty() // + || !diff.getRemoved().isEmpty() // + || !diff.getMissing().isEmpty() // + || !diff.getModified().isEmpty() // + || !diff.getConflicting().isEmpty(); + clean = !hasUncommittedChanges // + && diff.getUntracked().isEmpty(); } /** @@ -89,6 +93,15 @@ public boolean isClean() { return clean; } + /** + * @return true if any tracked file is changed + * + * @since 3.2 + */ + public boolean hasUncommittedChanges() { + return hasUncommittedChanges; + } + /** * @return list of files added to the index, not in HEAD (e.g. what you get * if you call 'git add ...' on a newly created file) @@ -168,4 +181,21 @@ public Map getConflictingStageState() { public Set getIgnoredNotInIndex() { return Collections.unmodifiableSet(diff.getIgnoredNotInIndex()); } + + /** + * @return set of files and folders that are known to the repo and changed + * either in the index or in the working tree. + * + * @since 3.2 + */ + public Set getUncommittedChanges() { + Set uncommittedChanges = new HashSet(); + uncommittedChanges.addAll(diff.getAdded()); + uncommittedChanges.addAll(diff.getChanged()); + uncommittedChanges.addAll(diff.getRemoved()); + uncommittedChanges.addAll(diff.getMissing()); + uncommittedChanges.addAll(diff.getModified()); + uncommittedChanges.addAll(diff.getConflicting()); + return uncommittedChanges; + } }