Fix computation of id in WorkingTreeIterator with autocrlf and smudging

JGit failed to do checkouts when the index contained smudged entries and
autocrlf was on. In such cases the WorkingTreeIterator calculated the
SHA1 sometimes on content which was not correctly filtered. The SHA1 was
computed on content which two times went through a lf->crlf conversion.

We used to tell the treewalk whether it is a checkin or checkout
operation and always use the related filters when reading any content.
If on windows and autocrlf is true and we do a checkout operation then
we always used a lf->crlf conversion on any text content. That's not
correct. Even during a checkout we sometimes need the crlf->lf
conversion. E.g. when calculating the content-id for working-tree
content we need to use crlf->lf filtering although the overall operation
type is checkout.

Often this bug does not have effects because we seldom compute the
content-id of filesystem content during a checkout. But we do need to
know whether a file is dirty or not before we overwrite it during a
checkout. And if the index entries are smudged we don't trust the index
and compute filesystem-content-sha1's explicitly.

This caused EGit not to be able to switch branches anymore on Windows
when autocrlf was true. EGit denied the checkout because it thought
workingtree files are dirty because content-sha1 are computed on wrongly
filtered content.

Bug: 493360
Change-Id: I1072a57b4c529ba3aaa50b7b02d2b816bb64a9b8
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/EolRepositoryTest.java

@@ -51,6 +51,7 @@ import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.api.errors.NoFilepatternException;
 import org.eclipse.jgit.attributes.Attribute;
 import org.eclipse.jgit.dircache.DirCache;
+import org.eclipse.jgit.dircache.DirCacheEditor;
 import org.eclipse.jgit.dircache.DirCacheEntry;
 import org.eclipse.jgit.dircache.DirCacheIterator;
 import org.eclipse.jgit.errors.RevisionSyntaxException;
@@ -61,9 +62,11 @@ import org.eclipse.jgit.lib.CoreConfig.AutoCRLF;
 import org.eclipse.jgit.lib.CoreConfig.EOL;
 import org.eclipse.jgit.lib.FileMode;
 import org.eclipse.jgit.lib.ObjectLoader;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.eclipse.jgit.treewalk.FileTreeIterator;
 import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.IO;
 import org.junit.Assert;
 import org.junit.Test;
@@ -82,6 +85,14 @@ public class EolRepositoryTest extends RepositoryTestCase {
 
 	private static final FileMode F = FileMode.REGULAR_FILE;
 
+	@DataPoint
+	public static boolean doSmudgeEntries = true;
+
+	@DataPoint
+	public static boolean dontSmudgeEntries = false;
+
+	private boolean smudge;
+
 	@DataPoint
 	public static String smallContents[] = {
 			generateTestData(3, 1, true, false),
@@ -117,10 +128,11 @@ public class EolRepositoryTest extends RepositoryTestCase {
 		return sb.toString();
 	}
 
-	public EolRepositoryTest(String[] testContent) {
+	public EolRepositoryTest(String[] testContent, boolean smudgeEntries) {
 		CONTENT_CRLF = testContent[0];
 		CONTENT_LF = testContent[1];
 		CONTENT_MIXED = testContent[2];
+		this.smudge = smudgeEntries;
 	}
 
 	protected String CONTENT_CRLF;
@@ -160,7 +172,7 @@ public class EolRepositoryTest extends RepositoryTestCase {
 
 	private ActualEntry entryMixed = new ActualEntry();
 
-	private DirCache dc;
+	private DirCache dirCache;
 
 	@Test
 	public void testDefaultSetup() throws Exception {
@@ -177,8 +189,10 @@ public class EolRepositoryTest extends RepositoryTestCase {
 			String indexContent) {
 		assertEquals(fileContent, entry.file);
 		assertEquals(indexContent, entry.index);
+		if (entry.indexContentLength != 0) {
 			assertEquals(fileContent.length(), entry.indexContentLength);
 		}
+	}
 
 	@Test
 	public void test_ConfigAutoCRLF_false() throws Exception {
@@ -584,6 +598,14 @@ public class EolRepositoryTest extends RepositoryTestCase {
 			dotGitattributes = null;
 		}
 
+		fileCRLF = createAndAddFile(git, "file1.txt", "a");
+
+		fileLF = createAndAddFile(git, "file2.txt", "a");
+
+		fileMixed = createAndAddFile(git, "file3.txt", "a");
+
+		RevCommit c = gitCommit(git, "create files");
+
 		fileCRLF = createAndAddFile(git, "file1.txt", CONTENT_CRLF);
 
 		fileLF = createAndAddFile(git, "file2.txt", CONTENT_LF);
@@ -593,6 +615,26 @@ public class EolRepositoryTest extends RepositoryTestCase {
 		gitCommit(git, "addFiles");
 
 		recreateWorktree(git);
+
+		if (smudge) {
+			DirCache dc = DirCache.lock(git.getRepository().getIndexFile(),
+					FS.detect());
+			DirCacheEditor editor = dc.editor();
+			for (int i = 0; i < dc.getEntryCount(); i++) {
+				editor.add(new DirCacheEditor.PathEdit(
+						dc.getEntry(i).getPathString()) {
+					public void apply(DirCacheEntry ent) {
+						ent.smudgeRacilyClean();
+					}
+				});
+			}
+			editor.commit();
+		}
+
+		// @TODO: find out why the following assertion would break the tests
+		// assertTrue(git.status().call().isClean());
+		git.checkout().setName(c.getName()).call();
+		git.checkout().setName("master").call();
 	}
 
 	private void recreateWorktree(Git git)
@@ -610,8 +652,8 @@ public class EolRepositoryTest extends RepositoryTestCase {
 		gitAdd(git, ".");
 	}
 
-	protected void gitCommit(Git git, String msg) throws GitAPIException {
+	protected RevCommit gitCommit(Git git, String msg) throws GitAPIException {
-		git.commit().setMessage(msg).call();
+		return git.commit().setMessage(msg).call();
 	}
 
 	protected void gitAdd(Git git, String path) throws GitAPIException {
@@ -644,7 +686,7 @@ public class EolRepositoryTest extends RepositoryTestCase {
 	}
 
 	private void collectRepositoryState() throws Exception {
-		dc = db.readDirCache();
+		dirCache = db.readDirCache();
 		walk = beginWalk();
 		if (dotGitattributes != null)
 			collectEntryContentAndAttributes(F, ".gitattributes", null);
@@ -680,7 +722,7 @@ public class EolRepositoryTest extends RepositoryTestCase {
 			e.attrs = e.attrs.trim();
 			e.file = new String(
 					IO.readFully(new File(db.getWorkTree(), pathName)));
-			DirCacheEntry dce = dc.getEntry(pathName);
+			DirCacheEntry dce = dirCache.getEntry(pathName);
 			ObjectLoader open = walk.getObjectReader().open(dce.getObjectId());
 			e.index = new String(open.getBytes());
 			e.indexContentLength = dce.getLength();

org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java

@@ -580,18 +580,32 @@ public class TreeWalk implements AutoCloseable, AttributesProvider {
 	}
 
 	/**
+	 * @param opType
+	 *            the operationtype (checkin/checkout) which should be used
 	 * @return the EOL stream type of the current entry using the config and
 	 *         {@link #getAttributes()} Note that this method may return null if
 	 *         the {@link TreeWalk} is not based on a working tree
-	 * @since 4.3
 	 */
-	public @Nullable EolStreamType getEolStreamType() {
+	// TODO(msohn) make this method public in 4.4
+	@Nullable
+	EolStreamType getEolStreamType(OperationType opType) {
 			if (attributesNodeProvider == null || config == null)
 				return null;
-			return EolStreamTypeUtil.detectStreamType(operationType,
+		return EolStreamTypeUtil.detectStreamType(opType,
 					config.get(WorkingTreeOptions.KEY), getAttributes());
 	}
 
+	/**
+	 * @return the EOL stream type of the current entry using the config and
+	 *         {@link #getAttributes()} Note that this method may return null if
+	 *         the {@link TreeWalk} is not based on a working tree
+	 * @since 4.3
+	 */
+	// TODO(msohn) deprecate this method in 4.4
+	public @Nullable EolStreamType getEolStreamType() {
+		return (getEolStreamType(operationType));
+	}
+
 	/** Reset this walker so new tree iterators can be added to it. */
 	public void reset() {
 		attrs = null;

org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java

@@ -86,6 +86,7 @@ import org.eclipse.jgit.lib.ObjectLoader;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.submodule.SubmoduleWalk;
+import org.eclipse.jgit.treewalk.TreeWalk.OperationType;
 import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.FS.ExecutionResult;
 import org.eclipse.jgit.util.Holder;
@@ -361,7 +362,8 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 				state.initializeDigestAndReadBuffer();
 
 				final long len = e.getLength();
-				InputStream filteredIs = possiblyFilteredInputStream(e, is, len);
+				InputStream filteredIs = possiblyFilteredInputStream(e, is, len,
+						OperationType.CHECKIN_OP);
 				return computeHash(filteredIs, canonLen);
 			} finally {
 				safeClose(is);
@@ -374,8 +376,15 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 
 	private InputStream possiblyFilteredInputStream(final Entry e,
 			final InputStream is, final long len) throws IOException {
+		return possiblyFilteredInputStream(e, is, len, null);
+
+	}
+
+	private InputStream possiblyFilteredInputStream(final Entry e,
+			final InputStream is, final long len, OperationType opType)
+			throws IOException {
 		if (getCleanFilterCommand() == null
-				&& getEolStreamType() == EolStreamType.DIRECT) {
+				&& getEolStreamType(opType) == EolStreamType.DIRECT) {
 			canonLen = len;
 			return is;
 		}
@@ -385,7 +394,7 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 			byte[] raw = rawbuf.array();
 			int n = rawbuf.limit();
 			if (!isBinary(raw, n)) {
-				rawbuf = filterClean(raw, n);
+				rawbuf = filterClean(raw, n, opType);
 				raw = rawbuf.array();
 				n = rawbuf.limit();
 			}
@@ -398,13 +407,14 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 				return is;
 			}
 
-		final InputStream lenIs = filterClean(e.openInputStream());
+		final InputStream lenIs = filterClean(e.openInputStream(),
+				opType);
 		try {
 			canonLen = computeLength(lenIs);
 		} finally {
 			safeClose(lenIs);
 		}
-		return filterClean(is);
+		return filterClean(is, opType);
 	}
 
 	private static void safeClose(final InputStream in) {
@@ -430,17 +440,23 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 		}
 	}
 
-	private ByteBuffer filterClean(byte[] src, int n) throws IOException {
+	private ByteBuffer filterClean(byte[] src, int n, OperationType opType)
+			throws IOException {
 		InputStream in = new ByteArrayInputStream(src);
 		try {
-			return IO.readWholeStream(filterClean(in), n);
+			return IO.readWholeStream(filterClean(in, opType), n);
 		} finally {
 			safeClose(in);
 		}
 	}
 
 	private InputStream filterClean(InputStream in) throws IOException {
-		in = handleAutoCRLF(in);
+		return filterClean(in, null);
+	}
+
+	private InputStream filterClean(InputStream in, OperationType opType)
+			throws IOException {
+		in = handleAutoCRLF(in, opType);
 		String filterCommand = getCleanFilterCommand();
 		if (filterCommand != null) {
 			FS fs = repository.getFS();
@@ -469,8 +485,9 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 		return in;
 	}
 
-	private InputStream handleAutoCRLF(InputStream in) throws IOException {
+	private InputStream handleAutoCRLF(InputStream in, OperationType opType)
-		return EolStreamTypeUtil.wrapInputStream(in, getEolStreamType());
+			throws IOException {
+		return EolStreamTypeUtil.wrapInputStream(in, getEolStreamType(opType));
 	}
 
 	/**
@@ -1332,10 +1349,28 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 	 * @since 4.3
 	 */
 	public EolStreamType getEolStreamType() throws IOException {
+		return getEolStreamType(null);
+	}
+
+	/**
+	 * @param opType
+	 *            The operationtype (checkin/checkout) which should be used
+	 * @return the eol stream type for the current entry or <code>null</code> if
+	 *         it cannot be determined. When state or state.walk is null or the
+	 *         {@link TreeWalk} is not based on a {@link Repository} then null
+	 *         is returned.
+	 * @throws IOException
+	 */
+	private EolStreamType getEolStreamType(OperationType opType)
+			throws IOException {
 		if (eolStreamTypeHolder == null) {
 			EolStreamType type=null;
 			if (state.walk != null) {
+				if (opType != null) {
+					type = state.walk.getEolStreamType(opType);
+				} else {
 					type=state.walk.getEolStreamType();
+				}
 			} else {
 				switch (getOptions().getAutoCRLF()) {
 				case FALSE: