From e8a1328d05aa55b7ace0d101e34b87422219c959 Mon Sep 17 00:00:00 2001 From: Mathias Kinzler Date: Fri, 28 Jan 2011 15:03:02 +0100 Subject: [PATCH] RebaseCommand: detect and handle fast-forward properly This bug was hidden by an incomplete test: the current Rebase implementation using the "git rebase -i" pattern does not work correctly if fast-forwarding is involved. The reason for this is that the log command does not return any commits in this case. In addition, a check for already merged commits was introduced to avoid spurious conflicts. Change-Id: Ib9898fe0f982fa08e41f1dca9452c43de715fdb6 Signed-off-by: Mathias Kinzler --- .../eclipse/jgit/api/RebaseCommandTest.java | 35 +++++- .../org/eclipse/jgit/api/RebaseCommand.java | 105 +++++++++++++++++- .../org/eclipse/jgit/api/RebaseResult.java | 6 +- 3 files changed, 139 insertions(+), 7 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 af3bb1cfa..f1a3f783f 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 @@ -132,7 +132,7 @@ public class RebaseCommandTest extends RepositoryTestCase { // create a topic branch createBranch(first, "refs/heads/topic"); // create file2 on master - writeTrashFile("file2", "file2"); + File file2 = writeTrashFile("file2", "file2"); git.add().addFilepattern("file2").call(); git.commit().setMessage("Add file2").call(); assertTrue(new File(db.getWorkTree(), "file2").exists()); @@ -141,7 +141,38 @@ public class RebaseCommandTest extends RepositoryTestCase { assertFalse(new File(db.getWorkTree(), "file2").exists()); RebaseResult res = git.rebase().setUpstream("refs/heads/master").call(); - assertEquals(Status.UP_TO_DATE, res.getStatus()); + assertTrue(new File(db.getWorkTree(), "file2").exists()); + checkFile(file2, "file2"); + assertEquals(Status.FAST_FORWARD, res.getStatus()); + } + + @Test + public void testFastForwardWithMultipleCommits() throws Exception { + // create file1 on master + writeTrashFile(FILE1, FILE1); + git.add().addFilepattern(FILE1).call(); + RevCommit first = git.commit().setMessage("Add file1").call(); + + assertTrue(new File(db.getWorkTree(), FILE1).exists()); + // create a topic branch + createBranch(first, "refs/heads/topic"); + // create file2 on master + File file2 = writeTrashFile("file2", "file2"); + git.add().addFilepattern("file2").call(); + git.commit().setMessage("Add file2").call(); + assertTrue(new File(db.getWorkTree(), "file2").exists()); + // write a second commit + writeTrashFile("file2", "file2 new content"); + git.add().addFilepattern("file2").call(); + git.commit().setMessage("Change content of file2").call(); + + checkoutBranch("refs/heads/topic"); + assertFalse(new File(db.getWorkTree(), "file2").exists()); + + RebaseResult res = git.rebase().setUpstream("refs/heads/master").call(); + assertTrue(new File(db.getWorkTree(), "file2").exists()); + checkFile(file2, "file2 new content"); + assertEquals(Status.FAST_FORWARD, res.getStatus()); } @Test 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 e5e604592..7241042c0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java @@ -63,8 +63,10 @@ import java.util.Map; import org.eclipse.jgit.JGitText; import org.eclipse.jgit.api.RebaseResult.Status; import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.api.errors.InvalidRefNameException; import org.eclipse.jgit.api.errors.JGitInternalException; import org.eclipse.jgit.api.errors.NoHeadException; +import org.eclipse.jgit.api.errors.RefAlreadyExistsException; import org.eclipse.jgit.api.errors.RefNotFoundException; import org.eclipse.jgit.api.errors.UnmergedPathsException; import org.eclipse.jgit.api.errors.WrongRepositoryStateException; @@ -189,6 +191,7 @@ public class RebaseCommand extends GitCommand { public RebaseResult call() throws NoHeadException, RefNotFoundException, JGitInternalException, GitAPIException { RevCommit newHead = null; + boolean lastStepWasForward = false; checkCallable(); checkParameters(); try { @@ -237,11 +240,17 @@ public class RebaseCommand extends GitCommand { monitor.beginTask(MessageFormat.format( JGitText.get().applyingCommit, commitToPick .getShortMessage()), ProgressMonitor.UNKNOWN); - // TODO if the first parent of commitToPick is the current HEAD, - // we should fast-forward instead of cherry-pick to avoid + // if the first parent of commitToPick is the current HEAD, + // we do a fast-forward instead of cherry-pick to avoid // unnecessary object rewriting - newHead = new Git(repo).cherryPick().include(commitToPick) - .call(); + newHead = tryFastForward(commitToPick); + lastStepWasForward = newHead != null; + if (!lastStepWasForward) + // TODO if the content of this commit is already merged here + // we should skip this step in order to avoid confusing + // pseudo-changed + newHead = new Git(repo).cherryPick().include(commitToPick) + .call(); monitor.endTask(); if (newHead == null) { return stop(commitToPick); @@ -274,6 +283,8 @@ public class RebaseCommand extends GitCommand { } } FileUtils.delete(rebaseDir, FileUtils.RECURSIVE); + if (lastStepWasForward) + return new RebaseResult(Status.FAST_FORWARD); return new RebaseResult(Status.OK); } return new RebaseResult(Status.UP_TO_DATE); @@ -496,6 +507,7 @@ public class RebaseCommand extends GitCommand { RevCommit headCommit = walk.lookupCommit(headId); monitor.beginTask(JGitText.get().obtainingCommitsForCherryPick, ProgressMonitor.UNKNOWN); + LogCommand cmd = new Git(repo).log().addRange(upstreamCommit, headCommit); Iterable commitsToUse = cmd.call(); @@ -503,6 +515,22 @@ public class RebaseCommand extends GitCommand { cherryPickList.add(commit); } + // if the upstream commit is in a direct line to the current head, + // the log command will not report any commits; in this case, + // we create the cherry-pick list ourselves + if (cherryPickList.isEmpty()) { + Iterable parents = new Git(repo).log().add( + upstreamCommit).call(); + for (RevCommit parent : parents) { + if (parent.equals(headCommit)) + break; + if (parent.getParentCount() != 1) + throw new JGitInternalException( + JGitText.get().canOnlyCherryPickCommitsWithOneParent); + cherryPickList.add(parent); + } + } + // nothing to do: return with UP_TO_DATE_RESULT if (cherryPickList.isEmpty()) return RebaseResult.UP_TO_DATE_RESULT; @@ -548,6 +576,75 @@ public class RebaseCommand extends GitCommand { return null; } + /** + * checks if we can fast-forward and returns the new head if it is possible + * + * @param newCommit + * @return the new head, or null + * @throws RefNotFoundException + * @throws IOException + */ + public RevCommit tryFastForward(RevCommit newCommit) + throws RefNotFoundException, IOException { + Ref head = repo.getRef(Constants.HEAD); + if (head == null || head.getObjectId() == null) + throw new RefNotFoundException(MessageFormat.format( + JGitText.get().refNotResolved, Constants.HEAD)); + + ObjectId headId = head.getObjectId(); + if (headId == null) + throw new RefNotFoundException(MessageFormat.format( + JGitText.get().refNotResolved, Constants.HEAD)); + RevCommit headCommit = walk.lookupCommit(headId); + if (walk.isMergedInto(newCommit, headCommit)) + return newCommit; + + String headName; + if (head.isSymbolic()) + headName = head.getTarget().getName(); + else + headName = "detached HEAD"; + return tryFastForward(headName, headCommit, newCommit); + } + + private RevCommit tryFastForward(String headName, RevCommit oldCommit, + RevCommit newCommit) throws IOException, JGitInternalException { + boolean tryRebase = false; + for (RevCommit parentCommit : newCommit.getParents()) + if (parentCommit.equals(oldCommit)) + tryRebase = true; + if (!tryRebase) + return null; + + CheckoutCommand co = new CheckoutCommand(repo); + try { + co.setName(newCommit.name()).call(); + if (headName.startsWith(Constants.R_HEADS)) { + RefUpdate rup = repo.updateRef(headName); + rup.setExpectedOldObjectId(oldCommit); + rup.setNewObjectId(newCommit); + rup.setRefLogMessage("Fast-foward from " + oldCommit.name() + + " to " + newCommit.name(), false); + Result res = rup.update(walk); + switch (res) { + case FAST_FORWARD: + case NO_CHANGE: + case FORCED: + break; + default: + throw new IOException("Could not fast-forward"); + } + } + return newCommit; + } catch (RefAlreadyExistsException e) { + throw new JGitInternalException(e.getMessage(), e); + } catch (RefNotFoundException e) { + throw new JGitInternalException(e.getMessage(), e); + } catch (InvalidRefNameException e) { + throw new JGitInternalException(e.getMessage(), e); + } + } + private void checkParameters() throws WrongRepositoryStateException { if (this.operation != Operation.BEGIN) { // these operations are only possible while in a rebasing state 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 bdbdddaec..b60336ce1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseResult.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseResult.java @@ -67,7 +67,11 @@ public class RebaseResult { /** * Already up-to-date */ - UP_TO_DATE; + UP_TO_DATE, + /** + * Fast-forward, HEAD points to the new commit + */ + FAST_FORWARD; } static final RebaseResult UP_TO_DATE_RESULT = new RebaseResult(