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 <stefan.lay@sap.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

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 2c9fce8186a8dbed38aef92ec72e93256dec0e6f..b33ad6ba5b85794c99f9c8593172db6426999007 100644 (file)
--- 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.RefUpdate;
 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 class RebaseCommandTest extends RepositoryTestCase {
                // 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 class RebaseCommandTest extends RepositoryTestCase {
 
                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 class RebaseCommandTest extends RepositoryTestCase {
                // 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 class RebaseCommandTest extends RepositoryTestCase {
                // 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 class RebaseCommandTest extends RepositoryTestCase {
                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 class RebaseCommandTest extends RepositoryTestCase {
                // 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 class RebaseCommandTest extends RepositoryTestCase {
                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 class RebaseCommandTest extends RepositoryTestCase {
                // 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 class RebaseCommandTest extends RepositoryTestCase {
                // 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 class RebaseCommandTest extends RepositoryTestCase {
 
        }
 
+       @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<RebaseTodoLine> 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 3bbac4a856aac76be59508ce459884bebbab9a48..55cf001c6c483f89f132f2c897eafb27edae80c2 100644 (file)
--- 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 class RebaseCommand extends GitCommand<RebaseResult> {
                                                .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<String> list = new ArrayList<String>();
+                                               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 0587b4301837730cac278aa1a2ed646319eb3b30..26d040342e82d2bb55df51663f893fdd9df07696 100644 (file)
--- 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 class RebaseResult {
                                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 class RebaseResult {
 
        private List<String> conflicts;
 
+       private List<String> uncommittedChanges;
+
        private RebaseResult(Status status) {
                this.status = status;
                currentCommit = null;
@@ -235,6 +249,20 @@ public class RebaseResult {
                return result;
        }
 
+       /**
+        * Create <code>RebaseResult</code> with status
+        * {@link Status#UNCOMMITTED_CHANGES}
+        *
+        * @param uncommittedChanges
+        *            the list of paths
+        * @return the RebaseResult
+        */
+       static RebaseResult uncommittedChanges(List<String> uncommittedChanges) {
+               RebaseResult result = new RebaseResult(Status.UNCOMMITTED_CHANGES);
+               result.uncommittedChanges = uncommittedChanges;
+               return result;
+       }
+
        /**
         * @return the overall status
         */
@@ -265,4 +293,15 @@ public class RebaseResult {
        public List<String> getConflicts() {
                return conflicts;
        }
+
+       /**
+        * @return the list of uncommitted changes if status is
+        *         {@link Status#UNCOMMITTED_CHANGES}
+        *
+        * @since 3.2
+        */
+       public List<String> 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 e840c2f6085d09a4286c511ac7cc24f5714c6ce8..c3fcd8bfe89e20dabed748ed5ee229f2c30524bf 100644 (file)
--- 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 class Status {
                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 class Status {
        public Set<String> 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<String> getUncommittedChanges() {
+               Set<String> uncommittedChanges = new HashSet<String>();
+               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;
+       }
 }