From 0ff691cdb55ad8247d38f706588728c44492b826 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Wed, 18 Dec 2013 15:30:53 +0100 Subject: [PATCH] Revert "Fix for core.autocrlf=input resulting in modified file..." This reverts commit 1def0a1257d32bce08e751242733cda3b2036cb8. We found this fix uncovers problems with unsmudged DirCacheEntry's. This surfaced because egit's ui test CreatePatchActionTest failed since jgit computes a wrong status. JGit doesn't detect modified content in now unsmudged entries. Hence revert this change until these problems are fixed. Change-Id: Ia04277ce316d35fc5b0d82c93d2078b856af24bb Signed-off-by: Matthias Sohn Signed-off-by: Christian Halstrick --- .../org/eclipse/jgit/lib/IndexDiffTest.java | 31 ------ .../jgit/treewalk/FileTreeIteratorTest.java | 12 +- .../jgit/dircache/DirCacheCheckout.java | 20 +--- .../src/org/eclipse/jgit/lib/IndexDiff.java | 3 +- .../jgit/treewalk/WorkingTreeIterator.java | 105 ++---------------- .../jgit/treewalk/filter/IndexDiffFilter.java | 5 +- .../util/io/EolCanonicalizingInputStream.java | 44 -------- 7 files changed, 20 insertions(+), 200 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java index a1435aed4..51ba5f13e 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java @@ -68,11 +68,9 @@ import org.eclipse.jgit.dircache.DirCacheEditor.PathEdit; import org.eclipse.jgit.dircache.DirCacheEntry; import org.eclipse.jgit.junit.RepositoryTestCase; -import org.eclipse.jgit.lib.CoreConfig.AutoCRLF; import org.eclipse.jgit.lib.IndexDiff.StageState; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.revwalk.RevCommit; -import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.treewalk.FileTreeIterator; import org.eclipse.jgit.util.IO; import org.junit.Test; @@ -531,35 +529,6 @@ public void testStageState() throws IOException { assertTrue(StageState.BOTH_ADDED.hasTheirs()); } - @Test - public void testAutoCRLFInput() throws Exception { - Git git = new Git(db); - FileBasedConfig config = db.getConfig(); - - // Make sure core.autocrlf is false before adding - config.setEnum(ConfigConstants.CONFIG_CORE_SECTION, null, - ConfigConstants.CONFIG_KEY_AUTOCRLF, AutoCRLF.FALSE); - config.save(); - - // File is already in repository with CRLF - writeTrashFile("crlf.txt", "this\r\ncontains\r\ncrlf\r\n"); - git.add().addFilepattern("crlf.txt").call(); - git.commit().setMessage("Add crlf.txt").call(); - - // Now set core.autocrlf to input - config.setEnum(ConfigConstants.CONFIG_CORE_SECTION, null, - ConfigConstants.CONFIG_KEY_AUTOCRLF, AutoCRLF.INPUT); - config.save(); - - FileTreeIterator iterator = new FileTreeIterator(db); - IndexDiff diff = new IndexDiff(db, Constants.HEAD, iterator); - diff.diff(); - - assertTrue( - "Expected no modified files, but there were: " - + diff.getModified(), diff.getModified().isEmpty()); - } - private void verifyStageState(StageState expected, int... stages) throws IOException { DirCacheBuilder builder = db.lockDirCache().builder(); 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 4ce4c8d04..6014f3b60 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 @@ -223,9 +223,7 @@ public void testDirCacheMatchingId() throws Exception { ObjectId fromRaw = ObjectId.fromRaw(fti.idBuffer(), fti.idOffset()); assertEquals("6b584e8ece562ebffc15d38808cd6b98fc3d97ea", fromRaw.getName()); - ObjectReader objectReader = db.newObjectReader(); - assertFalse(fti.isModified(dce, false, objectReader)); - objectReader.release(); + assertFalse(fti.isModified(dce, false)); } @Test @@ -244,9 +242,7 @@ public void testIsModifiedSymlink() throws Exception { .getConfig().get(WorkingTreeOptions.KEY)); while (!fti.getEntryPathString().equals("symlink")) fti.next(1); - ObjectReader objectReader = db.newObjectReader(); - assertFalse(fti.isModified(dce, false, objectReader)); - objectReader.release(); + assertFalse(fti.isModified(dce, false)); } @Test @@ -269,9 +265,7 @@ public void testIsModifiedFileSmudged() throws Exception { // If the rounding trick does not work we could skip the compareMetaData // test and hope that we are usually testing the intended code path. assertEquals(MetadataDiff.SMUDGED, fti.compareMetadata(dce)); - ObjectReader objectReader = db.newObjectReader(); - assertTrue(fti.isModified(dce, false, objectReader)); - objectReader.release(); + assertTrue(fti.isModified(dce, false)); } @Test 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 40efc95f8..f8c8442ff 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java @@ -326,8 +326,7 @@ void processEntry(CanonicalTreeParser m, DirCacheBuildIterator i, m.getEntryFileMode()); } else if (i.getDirCacheEntry() != null) { // The index contains a file (and not a folder) - if (f.isModified(i.getDirCacheEntry(), true, - this.walk.getObjectReader()) + if (f.isModified(i.getDirCacheEntry(), true) || i.getDirCacheEntry().getStage() != 0) // The working tree file is dirty or the index contains a // conflict @@ -661,9 +660,7 @@ void processEntry(CanonicalTreeParser h, CanonicalTreeParser m, break; case 0xFFD: // 12 13 14 if (equalIdAndMode(hId, hMode, iId, iMode)) - if (f == null - || f.isModified(dce, true, - this.walk.getObjectReader())) + if (f == null || f.isModified(dce, true)) conflict(name, dce, h, m); else remove(name); @@ -777,8 +774,7 @@ else if (m == null) // Nothing in Head // Something in Index if (dce != null - && (f == null || f.isModified(dce, true, - this.walk.getObjectReader()))) + && (f == null || f.isModified(dce, true))) // No file or file is dirty // Nothing in Merge and current path is part of // File/Folder conflict @@ -845,9 +841,7 @@ else if (m == null) // Something different from a submodule in Index // Nothing in Merge // Something in Head - if (f == null - || f.isModified(dce, true, - this.walk.getObjectReader())) + if (f == null || f.isModified(dce, true)) // file is dirty // Index contains the same as Head // Something different from a submodule in Index @@ -910,8 +904,7 @@ else if (m == null) // file content update(name, mId, mMode); } else if (dce != null - && (f == null || f.isModified(dce, true, - this.walk.getObjectReader()))) { + && (f == null || f.isModified(dce, true))) { // File doesn't exist or is dirty // Head and Index don't contain a submodule // Head contains the same as Index. Merge differs @@ -1048,8 +1041,7 @@ private boolean isModified(String path) throws CorruptObjectException, IOExcepti wtIt = tw.getTree(1, WorkingTreeIterator.class); if (dcIt == null || wtIt == null) return true; - if (wtIt.isModified(dcIt.getDirCacheEntry(), true, - this.walk.getObjectReader())) { + if (wtIt.isModified(dcIt.getDirCacheEntry(), true)) { return true; } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java index 8eb033355..33654447a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java @@ -440,8 +440,7 @@ public boolean diff(final ProgressMonitor monitor, int estWorkTreeSize, missing.add(treeWalk.getPathString()); } else { if (workingTreeIterator.isModified( - dirCacheIterator.getDirCacheEntry(), true, - treeWalk.getObjectReader())) { + dirCacheIterator.getDirCacheEntry(), true)) { // in index, in workdir, content differs => modified modified.add(treeWalk.getPathString()); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java index f2274d5ea..07ba9d73a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java @@ -73,12 +73,10 @@ import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.CoreConfig; -import org.eclipse.jgit.lib.CoreConfig.CheckStat; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.ObjectLoader; -import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.CoreConfig.CheckStat; import org.eclipse.jgit.submodule.SubmoduleWalk; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.IO; @@ -797,27 +795,23 @@ else if (!entry.isSmudged()) * @param forceContentCheck * True if the actual file content should be checked if * modification time differs. - * @param reader - * access to repository objects if necessary. * @return true if content is most likely different. - * @since 3.2 */ - public boolean isModified(DirCacheEntry entry, boolean forceContentCheck, - ObjectReader reader) { + public boolean isModified(DirCacheEntry entry, boolean forceContentCheck) { MetadataDiff diff = compareMetadata(entry); switch (diff) { case DIFFER_BY_TIMESTAMP: if (forceContentCheck) // But we are told to look at content even though timestamps // tell us about modification - return contentCheck(entry, reader); + return contentCheck(entry); else // We are told to assume a modification if timestamps differs return true; case SMUDGED: // The file is clean by timestamps but the entry was smudged. // Lets do a content check - return contentCheck(entry, reader); + return contentCheck(entry); case EQUAL: return false; case DIFFER_BY_METADATA: @@ -828,26 +822,6 @@ public boolean isModified(DirCacheEntry entry, boolean forceContentCheck, } } - /** - * Checks whether this entry differs from a given entry from the - * {@link DirCache}. - * - * File status information is used and if status is same we consider the - * file identical to the state in the working directory. Native git uses - * more stat fields than we have accessible in Java. - * - * @param entry - * the entry from the dircache we want to compare against - * @param forceContentCheck - * True if the actual file content should be checked if - * modification time differs. - * @return true if content is most likely different. - * @deprecated Use {@link #isModified(DirCacheEntry, boolean, ObjectReader)} - */ - public boolean isModified(DirCacheEntry entry, boolean forceContentCheck) { - return isModified(entry, false, null); - } - /** * Get the file mode to use for the current entry when it is to be updated * in the index. @@ -880,12 +854,10 @@ public FileMode getIndexFileMode(final DirCacheIterator indexIter) { * * @param entry * the entry to be checked - * @param reader - * acccess to repository data if necessary - * @return true if the content doesn't match, - * false if it matches + * @return true if the content matches, false + * otherwise */ - private boolean contentCheck(DirCacheEntry entry, ObjectReader reader) { + private boolean contentCheck(DirCacheEntry entry) { if (getEntryObjectId().equals(entry.getObjectId())) { // Content has not changed @@ -901,68 +873,7 @@ private boolean contentCheck(DirCacheEntry entry, ObjectReader reader) { return false; } else { - // Content differs: that's a real change, perhaps - if (reader == null) // deprecated use, do no further checks - return true; - switch (getOptions().getAutoCRLF()) { - case INPUT: - case TRUE: - InputStream dcIn = null; - try { - ObjectLoader loader = reader.open(entry.getObjectId()); - if (loader == null) - return true; - - // We need to compute the length, but only if it is not - // a binary stream. - dcIn = new EolCanonicalizingInputStream( - loader.openStream(), true, true /* abort if binary */); - long dcInLen; - try { - dcInLen = computeLength(dcIn); - } catch (EolCanonicalizingInputStream.IsBinaryException e) { - // ok, we know it's different so unsmudge the entry - entry.setLength(entry.getLength()); - return true; - } finally { - dcIn.close(); - } - - dcIn = new EolCanonicalizingInputStream( - loader.openStream(), true); - byte[] autoCrLfHash = computeHash(dcIn, dcInLen); - boolean changed = getEntryObjectId().compareTo( - autoCrLfHash, 0) != 0; - if (!changed) { - // Update the index with the eol'ed hash, so we can - // detect the no-change faster next time - entry.setObjectIdFromRaw(autoCrLfHash, 0); - } - // Ok, we know whether it has changed, so unsmudge the - // dirache entry - entry.setLength(loader.getSize()); - return changed; - } catch (IOException e) { - return true; - } finally { - if (dcIn != null) - try { - dcIn.close(); - } catch (IOException e) { - // empty - } - } - case FALSE: - // Ok, we know it's different so unsmudge the dircache entry - try { - ObjectLoader loader = reader.open(entry.getObjectId()); - if (loader != null) - entry.setLength((int) loader.getSize()); - } catch (IOException e) { - // panic, no, but don't unsmudge - } - break; - } + // Content differs: that's a real change! return true; } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/IndexDiffFilter.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/IndexDiffFilter.java index 150d5c786..1b231cce9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/IndexDiffFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/IndexDiffFilter.java @@ -53,7 +53,6 @@ import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.lib.FileMode; -import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.treewalk.WorkingTreeIterator; @@ -73,7 +72,7 @@ *

* If no difference is found then we have to compare index and working-tree as * the last step. By making use of - * {@link WorkingTreeIterator#isModified(org.eclipse.jgit.dircache.DirCacheEntry, boolean, ObjectReader)} + * {@link WorkingTreeIterator#isModified(org.eclipse.jgit.dircache.DirCacheEntry, boolean)} * we can avoid the computation of the content id if the file is not dirty. *

* Instances of this filter should not be used for multiple {@link TreeWalk}s. @@ -219,7 +218,7 @@ public boolean include(TreeWalk tw) throws MissingObjectException, // Only one chance left to detect a diff: between index and working // tree. Make use of the WorkingTreeIterator#isModified() method to // avoid computing SHA1 on filesystem content if not really needed. - return wi.isModified(di.getDirCacheEntry(), true, tw.getObjectReader()); + return wi.isModified(di.getDirCacheEntry(), true); } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/io/EolCanonicalizingInputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/EolCanonicalizingInputStream.java index d23e1c160..f87ab6896 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/io/EolCanonicalizingInputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/EolCanonicalizingInputStream.java @@ -70,23 +70,6 @@ public class EolCanonicalizingInputStream extends InputStream { private boolean detectBinary; - private boolean abortIfBinary; - - /** - * A special exception thrown when {@link EolCanonicalizingInputStream} is - * told to throw an exception when attempting to read a binary file. The - * exception may be thrown at any stage during reading. - * - * @since 3.2 - */ - public static class IsBinaryException extends IOException { - private static final long serialVersionUID = 1L; - - IsBinaryException() { - super(); - } - } - /** * Creates a new InputStream, wrapping the specified stream * @@ -97,25 +80,8 @@ public static class IsBinaryException extends IOException { * @since 2.0 */ public EolCanonicalizingInputStream(InputStream in, boolean detectBinary) { - this(in, detectBinary, false); - } - - /** - * Creates a new InputStream, wrapping the specified stream - * - * @param in - * raw input stream - * @param detectBinary - * whether binaries should be detected - * @param abortIfBinary - * throw an IOException if the file is binary - * @since 3.2 - */ - public EolCanonicalizingInputStream(InputStream in, boolean detectBinary, - boolean abortIfBinary) { this.in = in; this.detectBinary = detectBinary; - this.abortIfBinary = abortIfBinary; } @Override @@ -162,14 +128,6 @@ public int read(byte[] bs, final int off, final int len) throws IOException { return i == off ? -1 : i - off; } - /** - * @return true if the stream has detected as a binary so far - * @since 3.2 - */ - public boolean isBinary() { - return isBinary; - } - @Override public void close() throws IOException { in.close(); @@ -182,8 +140,6 @@ private boolean fillBuffer() throws IOException { if (detectBinary) { isBinary = RawText.isBinary(buf, cnt); detectBinary = false; - if (isBinary && abortIfBinary) - throw new IsBinaryException(); } ptr = 0; return true;