From 4027c5c9ffc9fb99b2c06b9a7ecdeb18838e8218 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Fri, 10 Aug 2018 11:11:50 +0200 Subject: [PATCH] Fix ResolveMerger: rebase with autocrlf=true, direct checkout ResolveMerger.checkout() and cleanUp() check out files directly and must honor CR/LF settings and also smudge filters. Deprecate the 3-argument version of DirCacheCheckout.checkoutEntry(). It isn't used anymore anywhere in JGit (nor in EGit). Bug: 537410 Change-Id: I062b35401c8bd5bc99deb2f68f91089a0643504c Signed-off-by: Thomas Wolf --- .../org/eclipse/jgit/merge/MergerTest.java | 39 ++++++ .../jgit/treewalk/FileTreeIteratorTest.java | 2 +- .../jgit/dircache/DirCacheCheckout.java | 4 + .../org/eclipse/jgit/merge/ResolveMerger.java | 122 +++++++++++++++--- 4 files changed, 149 insertions(+), 18 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java index 58093a372..da4513d1f 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java @@ -60,6 +60,7 @@ import java.util.Map; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.MergeResult; import org.eclipse.jgit.api.MergeResult.MergeStatus; +import org.eclipse.jgit.api.RebaseResult; import org.eclipse.jgit.api.errors.CheckoutConflictException; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.api.errors.JGitInternalException; @@ -428,6 +429,44 @@ public class MergerTest extends RepositoryTestCase { indexState(CONTENT)); } + @Theory + public void rebaseWithCrlfAutoCrlfTrue(MergeStrategy strategy) + throws IOException, GitAPIException { + Git git = Git.wrap(db); + db.getConfig().setString("core", null, "autocrlf", "true"); + db.getConfig().save(); + writeTrashFile("crlf.txt", "line 1\r\nline 2\r\nline 3\r\n"); + git.add().addFilepattern("crlf.txt").call(); + RevCommit first = git.commit().setMessage("base").call(); + + git.checkout().setCreateBranch(true).setStartPoint(first) + .setName("brancha").call(); + + File testFile = writeTrashFile("crlf.txt", + "line 1\r\nmodified line\r\nline 3\r\n"); + git.add().addFilepattern("crlf.txt").call(); + git.commit().setMessage("on brancha").call(); + + git.checkout().setName("master").call(); + File otherFile = writeTrashFile("otherfile.txt", "a line\r\n"); + git.add().addFilepattern("otherfile.txt").call(); + git.commit().setMessage("on master").call(); + + git.checkout().setName("brancha").call(); + checkFile(testFile, "line 1\r\nmodified line\r\nline 3\r\n"); + assertFalse(otherFile.exists()); + + RebaseResult rebaseResult = git.rebase().setStrategy(strategy) + .setUpstream(db.resolve("master")).call(); + assertEquals(RebaseResult.Status.OK, rebaseResult.getStatus()); + checkFile(testFile, "line 1\r\nmodified line\r\nline 3\r\n"); + checkFile(otherFile, "a line\r\n"); + assertEquals( + "[crlf.txt, mode:100644, content:line 1\nmodified line\nline 3\n]" + + "[otherfile.txt, mode:100644, content:a line\n]", + indexState(CONTENT)); + } + /** * Merging two equal subtrees when the index does not contain any file in * that subtree should lead to a merged state. diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorTest.java index 0e009b954..e03167833 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorTest.java @@ -331,7 +331,7 @@ public class FileTreeIteratorTest extends RepositoryTestCase { DirCacheEntry dce = db.readDirCache().getEntry("symlink"); dce.setFileMode(FileMode.SYMLINK); try (ObjectReader objectReader = db.newObjectReader()) { - DirCacheCheckout.checkoutEntry(db, dce, objectReader); + DirCacheCheckout.checkoutEntry(db, dce, objectReader, false, null); FileTreeIterator fti = new FileTreeIterator(trash, db.getFS(), db.getConfig().get(WorkingTreeOptions.KEY)); 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 0b03eb152..bad71de8a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java @@ -1366,7 +1366,11 @@ public class DirCacheCheckout { * object reader to use for checkout * @throws java.io.IOException * @since 3.6 + * @deprecated since 5.1, use + * {@link #checkoutEntry(Repository, DirCacheEntry, ObjectReader, boolean, CheckoutMetadata)} + * instead */ + @Deprecated public static void checkoutEntry(Repository repo, DirCacheEntry entry, ObjectReader or) throws IOException { checkoutEntry(repo, entry, or, false, null); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java index 3042fdd3f..f60c95f64 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -78,6 +78,7 @@ import org.eclipse.jgit.dircache.DirCache; import org.eclipse.jgit.dircache.DirCacheBuildIterator; import org.eclipse.jgit.dircache.DirCacheBuilder; import org.eclipse.jgit.dircache.DirCacheCheckout; +import org.eclipse.jgit.dircache.DirCacheCheckout.CheckoutMetadata; import org.eclipse.jgit.dircache.DirCacheEntry; import org.eclipse.jgit.errors.BinaryBlobException; import org.eclipse.jgit.errors.CorruptObjectException; @@ -297,6 +298,12 @@ public class ResolveMerger extends ThreeWayMerger { */ private int inCoreLimit; + /** + * Keeps {@link CheckoutMetadata} for {@link #checkout()} and + * {@link #cleanUp()}. + */ + private Map checkoutMetadata; + private static MergeAlgorithm getMergeAlgorithm(Config config) { SupportedAlgorithm diffAlg = config.getEnum( CONFIG_DIFF_SECTION, null, CONFIG_KEY_ALGORITHM, @@ -313,6 +320,8 @@ public class ResolveMerger extends ThreeWayMerger { return new String[] { "BASE", "OURS", "THEIRS" }; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ } + private static final Attributes NO_ATTRIBUTES = new Attributes(); + /** * Constructor for ResolveMerger. * @@ -369,15 +378,20 @@ public class ResolveMerger extends ThreeWayMerger { /** {@inheritDoc} */ @Override protected boolean mergeImpl() throws IOException { - if (implicitDirCache) + if (implicitDirCache) { dircache = nonNullRepo().lockDirCache(); - + } + if (!inCore) { + checkoutMetadata = new HashMap<>(); + } try { return mergeTrees(mergeBase(), sourceTrees[0], sourceTrees[1], false); } finally { - if (implicitDirCache) + checkoutMetadata = null; + if (implicitDirCache) { dircache.unlock(); + } } } @@ -400,7 +414,8 @@ public class ResolveMerger extends ThreeWayMerger { if (cacheEntry.getFileMode() == FileMode.GITLINK) { new File(nonNullRepo().getWorkTree(), entry.getKey()).mkdirs(); } else { - DirCacheCheckout.checkoutEntry(db, cacheEntry, reader); + DirCacheCheckout.checkoutEntry(db, cacheEntry, reader, false, + checkoutMetadata.get(entry.getKey())); modifiedFiles.add(entry.getKey()); } } @@ -428,10 +443,12 @@ public class ResolveMerger extends ThreeWayMerger { DirCache dc = nonNullRepo().readDirCache(); Iterator mpathsIt=modifiedFiles.iterator(); while(mpathsIt.hasNext()) { - String mpath=mpathsIt.next(); + String mpath = mpathsIt.next(); DirCacheEntry entry = dc.getEntry(mpath); - if (entry != null) - DirCacheCheckout.checkoutEntry(db, entry, reader); + if (entry != null) { + DirCacheCheckout.checkoutEntry(db, entry, reader, false, + checkoutMetadata.get(mpath)); + } mpathsIt.remove(); } } @@ -480,6 +497,71 @@ public class ResolveMerger extends ThreeWayMerger { return newEntry; } + /** + * Remembers the {@link CheckoutMetadata} for the given path; it may be + * needed in {@link #checkout()} or in {@link #cleanUp()}. + * + * @param path + * of the current node + * @param attributes + * for the current node + * @throws IOException + * if the smudge filter cannot be determined + * @since 5.1 + */ + protected void addCheckoutMetadata(String path, Attributes attributes) + throws IOException { + if (checkoutMetadata != null) { + EolStreamType eol = EolStreamTypeUtil.detectStreamType( + OperationType.CHECKOUT_OP, workingTreeOptions, attributes); + CheckoutMetadata data = new CheckoutMetadata(eol, + tw.getFilterCommand(Constants.ATTR_FILTER_TYPE_SMUDGE)); + checkoutMetadata.put(path, data); + } + } + + /** + * Adds a {@link DirCacheEntry} for direct checkout and remembers its + * {@link CheckoutMetadata}. + * + * @param path + * of the entry + * @param entry + * to add + * @param attributes + * for the current entry + * @throws IOException + * if the {@link CheckoutMetadata} cannot be determined + * @since 5.1 + */ + protected void addToCheckout(String path, DirCacheEntry entry, + Attributes attributes) throws IOException { + toBeCheckedOut.put(path, entry); + addCheckoutMetadata(path, attributes); + } + + /** + * Remember a path for deletion, and remember its {@link CheckoutMetadata} + * in case it has to be restored in {@link #cleanUp()}. + * + * @param path + * of the entry + * @param isFile + * whether it is a file + * @param attributes + * for the entry + * @throws IOException + * if the {@link CheckoutMetadata} cannot be determined + * @since 5.1 + */ + protected void addDeletion(String path, boolean isFile, + Attributes attributes) throws IOException { + toBeDeleted.add(path); + if (isFile) { + addCheckoutMetadata(path, attributes); + } + } + /** * Processes one path and tries to merge taking git attributes in account. * This method will do all trivial (not content) merges and will also detect @@ -586,7 +668,7 @@ public class ResolveMerger extends ThreeWayMerger { // This will happen later. Set these values to 0 for know. DirCacheEntry e = add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_0, 0, 0); - toBeCheckedOut.put(tw.getPathString(), e); + addToCheckout(tw.getPathString(), e, attributes); } return true; } else { @@ -627,8 +709,9 @@ public class ResolveMerger extends ThreeWayMerger { // This will happen later. Set these values to 0 for know. DirCacheEntry e = add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_0, 0, 0); - if (e != null) - toBeCheckedOut.put(tw.getPathString(), e); + if (e != null) { + addToCheckout(tw.getPathString(), e, attributes); + } return true; } else { // we want THEIRS ... but THEIRS contains a folder or the @@ -642,7 +725,7 @@ public class ResolveMerger extends ThreeWayMerger { // Base, ours, and theirs all contain a folder: don't delete return true; } - toBeDeleted.add(tw.getPathString()); + addDeletion(tw.getPathString(), nonTree(modeO), attributes); return true; } } @@ -726,9 +809,12 @@ public class ResolveMerger extends ThreeWayMerger { result.setContainsConflicts(false); } updateIndex(base, ours, theirs, result, attributes); - if (result.containsConflicts() && !ignoreConflicts) - unmergedPaths.add(tw.getPathString()); - modifiedFiles.add(tw.getPathString()); + String currentPath = tw.getPathString(); + if (result.containsConflicts() && !ignoreConflicts) { + unmergedPaths.add(currentPath); + } + modifiedFiles.add(currentPath); + addCheckoutMetadata(currentPath, attributes); } else if (modeO != modeT) { // OURS or THEIRS has been deleted if (((modeO != 0 && !tw.idEqual(T_BASE, T_OURS)) || (modeT != 0 && !tw @@ -747,8 +833,9 @@ public class ResolveMerger extends ThreeWayMerger { if (isWorktreeDirty(work, ourDce)) return false; if (nonTree(modeT)) { - if (e != null) - toBeCheckedOut.put(tw.getPathString(), e); + if (e != null) { + addToCheckout(tw.getPathString(), e, attributes); + } } } @@ -1249,7 +1336,8 @@ public class ResolveMerger extends ThreeWayMerger { hasWorkingTreeIterator ? treeWalk.getTree(T_FILE, WorkingTreeIterator.class) : null, ignoreConflicts, hasAttributeNodeProvider - ? treeWalk.getAttributes() : new Attributes())) { + ? treeWalk.getAttributes() + : NO_ATTRIBUTES)) { cleanUp(); return false; }