From e406d500de01b9ae7155e296baebf3ec8024869d Mon Sep 17 00:00:00 2001 From: Christian Halstrick Date: Mon, 12 Feb 2018 15:44:04 +0100 Subject: [PATCH] Fix "jgit checkout -f" to overwrite dirty worktree files CheckoutCommand had a setForce() method. But this didn't correspond to native git's 'git checkout -f' option. Deprecate the old setForce() method and move its implementation to a new method setForceRefUpdate() and use it to implement the -B option in the CLI class Checkout. Add a setForced() method and use it to fix the associated '-f' option of the CLI Checkout class to behave like native git's 'git checkout -f' which overwrites dirty worktree files during checkout. This is still not fully matching native git's behavior: updating additionally dirty index entries is not done yet. Bug: 530771 Change-Id: I776b78eb623b6ea0aca42f681788f2e4b1667f15 Signed-off-by: Matthias Sohn --- .../jgit/ant/tasks/GitCheckoutTask.java | 2 +- .../org/eclipse/jgit/pgm/CheckoutTest.java | 16 +++++ .../jgit/pgm/internal/CLIText.properties | 3 +- .../src/org/eclipse/jgit/pgm/Checkout.java | 8 ++- .../eclipse/jgit/api/CheckoutCommandTest.java | 17 +++++- .../org/eclipse/jgit/api/CheckoutCommand.java | 58 +++++++++++++++++-- .../jgit/dircache/DirCacheCheckout.java | 42 ++++++++++---- 7 files changed, 126 insertions(+), 20 deletions(-) diff --git a/org.eclipse.jgit.ant/src/org/eclipse/jgit/ant/tasks/GitCheckoutTask.java b/org.eclipse.jgit.ant/src/org/eclipse/jgit/ant/tasks/GitCheckoutTask.java index 0b27cc264..5f80d00eb 100644 --- a/org.eclipse.jgit.ant/src/org/eclipse/jgit/ant/tasks/GitCheckoutTask.java +++ b/org.eclipse.jgit.ant/src/org/eclipse/jgit/ant/tasks/GitCheckoutTask.java @@ -123,7 +123,7 @@ public void execute() throws BuildException { } try { - checkout.setCreateBranch(createBranch).setForce(force) + checkout.setCreateBranch(createBranch).setForceRefUpdate(force) .setName(branch); checkout.call(); } catch (Exception e) { diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/CheckoutTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/CheckoutTest.java index b2115a4b7..f0e2b38cb 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/CheckoutTest.java +++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/CheckoutTest.java @@ -58,6 +58,7 @@ import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.CheckoutConflictException; import org.eclipse.jgit.diff.DiffEntry; +import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase; import org.eclipse.jgit.lib.CLIRepositoryTestCase; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.Ref; @@ -684,4 +685,19 @@ public void testCheckoutLink() throws Exception { assertTrue(Files.isSymbolicLink(path)); } } + + @Test + public void testCheckoutForce_Bug530771() throws Exception { + try (Git git = new Git(db)) { + File f = writeTrashFile("a", "Hello world"); + git.add().addFilepattern("a").call(); + git.commit().setMessage("create a").call(); + writeTrashFile("a", "Goodbye world"); + assertEquals("[]", + Arrays.toString(execute("git checkout -f HEAD"))); + assertEquals("Hello world", read(f)); + assertEquals("[a, mode:100644, content:Hello world]", + indexState(db, LocalDiskRepositoryTestCase.CONTENT)); + } + } } diff --git a/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties b/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties index d7d895ab3..538c87661 100644 --- a/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties +++ b/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties @@ -341,7 +341,8 @@ usage_fetchThinPack=fetch thin pack usage_filesToAddContentFrom=Files to add content from usage_fixAThinPackToBeComplete=fix a thin pack to be complete usage_forEachRefOutput=for-each-ref output -usage_forceCheckout=when switching branches, proceed even if the index or the working tree differs from HEAD +usage_forcedSwitchBranch=when switching branches do it forcefully. Succeed even if resetting an existing branch would cause commits to become unreachable +usage_forceCheckout=when checking out a commit succeed even if the working tree or the index is dirty. Overwrite the working tree or index in such cases usage_forceClean=required to delete files or directories usage_forceCreateBranchEvenExists=force create branch even exists usage_forcedFetch=force ref update fetch option diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Checkout.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Checkout.java index 6ff39fab0..7e1737f87 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Checkout.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Checkout.java @@ -69,8 +69,11 @@ class Checkout extends TextBuiltin { @Option(name = "-b", usage = "usage_createBranchAndCheckout") private boolean createBranch = false; + @Option(name = "-B", usage = "usage_forcedSwitchBranch") + private boolean forceSwitchBranch = false; + @Option(name = "--force", aliases = { "-f" }, usage = "usage_forceCheckout") - private boolean force = false; + private boolean forced = false; @Option(name = "--orphan", usage = "usage_orphan") private boolean orphan = false; @@ -103,7 +106,8 @@ protected void run() throws Exception { } else { command.setCreateBranch(createBranch); command.setName(name); - command.setForce(force); + command.setForceRefUpdate(forceSwitchBranch); + command.setForced(forced); command.setOrphan(orphan); } try { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CheckoutCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CheckoutCommandTest.java index 498005ded..749c344f2 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CheckoutCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CheckoutCommandTest.java @@ -63,6 +63,7 @@ import org.eclipse.jgit.api.CheckoutResult.Status; import org.eclipse.jgit.api.CreateBranchCommand.SetupUpstreamMode; +import org.eclipse.jgit.api.errors.CheckoutConflictException; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.api.errors.InvalidRefNameException; import org.eclipse.jgit.api.errors.InvalidRemoteException; @@ -109,7 +110,7 @@ public void setUp() throws Exception { git.add().addFilepattern("Test.txt").call(); initialCommit = git.commit().setMessage("Initial commit").call(); - // create a master branch and switch to it + // create a test branch and switch to it git.branchCreate().setName("test").call(); RefUpdate rup = db.updateRef(Constants.HEAD); rup.link("refs/heads/test"); @@ -137,6 +138,18 @@ public void testCheckout() throws Exception { assertEquals("refs/heads/master", git.getRepository().getFullBranch()); } + @Test + public void testCheckoutForced() throws Exception { + writeTrashFile("Test.txt", "Garbage"); + try { + git.checkout().setName("master").call().getObjectId(); + fail("Expected CheckoutConflictException didn't occur"); + } catch (CheckoutConflictException e) { + } + assertEquals(initialCommit.getId(), git.checkout().setName("master") + .setForced(true).call().getObjectId()); + } + @Test public void testCreateBranchOnCheckout() throws Exception { git.checkout().setCreateBranch(true).setName("test2").call(); @@ -165,7 +178,7 @@ public void testCheckoutWithConflict() throws Exception { assertEquals(Status.CONFLICTS, co.getResult().getStatus()); assertTrue(co.getResult().getConflictList().contains("Test.txt")); } - git.checkout().setName("master").setForce(true).call(); + git.checkout().setName("master").setForced(true).call(); assertThat(read("Test.txt"), is("Hello world")); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CheckoutCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CheckoutCommand.java index 455a2e665..e05f6f1bd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CheckoutCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CheckoutCommand.java @@ -162,7 +162,9 @@ private Stage(int number) { private String name; - private boolean force = false; + private boolean forceRefUpdate = false; + + private boolean forced = false; private boolean createBranch = false; @@ -269,7 +271,11 @@ public Ref call() throws GitAPIException, RefAlreadyExistsException, try { dco = new DirCacheCheckout(repo, headTree, dc, newCommit.getTree()); - dco.setFailOnConflict(!force); + dco.setFailOnConflict(true); + dco.setForce(forced); + if (forced) { + dco.setFailOnConflict(false); + } dco.setProgressMonitor(monitor); try { dco.checkout(); @@ -286,7 +292,7 @@ public Ref call() throws GitAPIException, RefAlreadyExistsException, ref = null; String toName = Repository.shortenRefName(name); RefUpdate refUpdate = repo.updateRef(Constants.HEAD, ref == null); - refUpdate.setForceUpdate(force); + refUpdate.setForceUpdate(forceRefUpdate); refUpdate.setRefLogMessage(refLogMessage + " to " + toName, false); //$NON-NLS-1$ Result updateResult; if (ref != null) @@ -666,10 +672,54 @@ public CheckoutCommand setOrphan(boolean orphan) { * set to a new start-point; if false, the existing branch will * not be changed * @return this instance + * @deprecated this method was badly named comparing its semantics to native + * git's checkout --force option, use + * {@link #setForceRefUpdate(boolean)} instead */ + @Deprecated public CheckoutCommand setForce(boolean force) { + return setForceRefUpdate(force); + } + + /** + * Specify to force the ref update in case of a branch switch. + * + * In releases prior to 5.2 this method was called setForce() but this name + * was misunderstood to implement native git's --force option, which is not + * true. + * + * @param forceRefUpdate + * if true and the branch with the given name + * already exists, the start-point of an existing branch will be + * set to a new start-point; if false, the existing branch will + * not be changed + * @return this instance + * @since 5.3 + */ + public CheckoutCommand setForceRefUpdate(boolean forceRefUpdate) { checkCallable(); - this.force = force; + this.forceRefUpdate = forceRefUpdate; + return this; + } + + /** + * Allow a checkout even if the workingtree or index differs from HEAD. This + * matches native git's '--force' option. + * + * JGit releases before 5.2 had a method setForce() offering + * semantics different from this new setForced(). This old + * semantic can now be found in {@link #setForceRefUpdate(boolean)} + * + * @param forced + * if set to true then allow the checkout even if + * workingtree or index doesn't match HEAD. Overwrite workingtree + * files and index content with the new content in this case. + * @return this instance + * @since 5.3 + */ + public CheckoutCommand setForced(boolean forced) { + checkCallable(); + this.forced = forced; return this; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java index ce7485bf1..307fd3f31 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java @@ -155,6 +155,8 @@ public CheckoutMetadata(EolStreamType eolStreamType, private boolean failOnConflict = true; + private boolean force = false; + private ArrayList toBeDeleted = new ArrayList<>(); private boolean initialCheckout; @@ -427,11 +429,11 @@ void processEntry(CanonicalTreeParser m, DirCacheBuildIterator i, DirCacheEntry entry = i.getDirCacheEntry(); if (entry.getLastModified() == 0) entry.setLastModified(f.getEntryLastModified()); - keep(entry); + keep(entry, f); } } else // The index contains a folder - keep(i.getDirCacheEntry()); + keep(i.getDirCacheEntry(), f); } else { // There is no entry in the merge commit. Means: we want to delete // what's currently in the index and working tree @@ -821,14 +823,14 @@ void processEntry(CanonicalTreeParser h, CanonicalTreeParser m, break; case 0xDFD: // 3 4 - keep(dce); + keep(dce, f); break; case 0xF0D: // 18 remove(name); break; case 0xDFF: // 5 5b 6 6b if (equalIdAndMode(iId, iMode, mId, mMode)) - keep(dce); // 5 6 + keep(dce, f); // 5 6 else conflict(name, dce, h, m); // 5b 6b break; @@ -858,7 +860,7 @@ void processEntry(CanonicalTreeParser h, CanonicalTreeParser m, conflict(name, dce, h, m); // 9 break; case 0xFD0: // keep without a rule - keep(dce); + keep(dce, f); break; case 0xFFD: // 12 13 14 if (equalIdAndMode(hId, hMode, iId, iMode)) @@ -878,7 +880,7 @@ void processEntry(CanonicalTreeParser h, CanonicalTreeParser m, conflict(name, dce, h, m); break; default: - keep(dce); + keep(dce, f); } return; } @@ -964,7 +966,7 @@ else if (m == null) if (initialCheckout) update(name, mId, mMode); else - keep(dce); + keep(dce, f); } else conflict(name, dce, h, m); } @@ -1027,7 +1029,7 @@ else if (m == null) // Nothing in Head // Something in Index // -> Merge contains nothing new. Keep the index. - keep(dce); + keep(dce, f); } else // Merge contains something and it is not the same as Index // Nothing in Head @@ -1176,7 +1178,7 @@ && isModified_IndexTree(name, iId, iMode, mId, mMode, // to the other one. // -> In all three cases we don't touch index and file. - keep(dce); + keep(dce, f); } } } @@ -1225,9 +1227,15 @@ private void conflict(String path, DirCacheEntry e, AbstractTreeIterator h, Abst } } - private void keep(DirCacheEntry e) { + private void keep(DirCacheEntry e, WorkingTreeIterator f) + throws IOException { if (e != null && !FileMode.TREE.equals(e.getFileMode())) builder.add(e); + if (force) { + if (f.isModified(e, true, this.walk.getObjectReader())) { + checkoutEntry(repo, e, this.walk.getObjectReader()); + } + } } private void remove(String path) { @@ -1261,6 +1269,20 @@ public void setFailOnConflict(boolean failOnConflict) { this.failOnConflict = failOnConflict; } + /** + * If true, dirty worktree files may be overridden. If + * false dirty worktree files will not be overridden in order + * not to delete unsaved content. This corresponds to native git's 'git + * checkout -f' option. By default this option is set to false. + * + * @param force + * a boolean. + * @since 5.3 + */ + public void setForce(boolean force) { + this.force = force; + } + /** * This method implements how to handle conflicts when * {@link #failOnConflict} is false