Fix symlink content comparison on MacOS in tree walk

Symlinks on MacOS are written as UTF-8 NFD, but readSymbolicLink().toString() converts to NFC with potentially fewer bytes. May occur in particular if the link target has non-ASCII characters for which the NFC and NFD encodings differ. This may lead to an EOFException: Short read of block. This causes all kinds of weird effects in EGit, ranging from failing rebases (which report the exception to the user) to EGit decorations in the navigator silently disappearing (and never coming back). * Rename readContentAsNormalizedString() to readSymlinkTarget() as it's called only for symlinks. Also make it protected. * Fix by allowing the read to succeed even if less than the expected number of bytes are returned by the entry's input stream. * Override in FileTreeIterator to use fs.readSymlink() directly. Includes a new MacOS-only test. Change-Id: I264c5972d67b1cbb1ed690580f5706e671b9affd Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch> Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
9 years ago · 26832a00e4
4 changed files with 295 additions and 8 deletions
--- a/org.eclipse.jgit.test/tst-rsrc/org/eclipse/jgit/indexdiff/filerepo.txt
+++ b/org.eclipse.jgit.test/tst-rsrc/org/eclipse/jgit/indexdiff/filerepo.txt
@ -0,0 +1,30 @@
+blob
+mark :1
+data 10
+äéü.txt
+blob
+mark :2
+data 8
+test.txt
+blob
+mark :3
+data 5
+Test
+
+blob
+mark :4
+data 7
+äéü
+
+reset refs/heads/master
+commit refs/heads/master
+mark :5
+author A U Thor <author@example.com> 1450727513 +0100
+committer A U Thor <author@example.com> 1450727513 +0100
+data 26
+Initial commit with links
+M 120000 :1 testfolder/aeu.txt
+M 120000 :2 testfolder/link.txt
+M 100644 :3 testfolder/test.txt
+M 100644 :4 testfolder/äéü.txt
+
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/indexdiff/IndexDiffWithSymlinkTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/indexdiff/IndexDiffWithSymlinkTest.java
@ -0,0 +1,234 @@
+/*
+ * Copyright (C) 2015 Thomas Wolf <thomas.wolf@paranor.ch>
+ *
+ * This program and the accompanying materials are made available
+ * under the terms of the Eclipse Distribution License v1.0 which
+ * accompanies this distribution, is reproduced below, and is
+ * available at http://www.eclipse.org/org/documents/edl-v10.php
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Eclipse Foundation, Inc. nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.eclipse.jgit.indexdiff;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
+
+import java.io.BufferedOutputStream;
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.io.Writer;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+
+import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.IndexDiff;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.storage.file.FileRepositoryBuilder;
+import org.eclipse.jgit.treewalk.FileTreeIterator;
+import org.eclipse.jgit.treewalk.WorkingTreeIterator;
+import org.eclipse.jgit.treewalk.filter.PathFilterGroup;
+import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.SystemReader;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * MacOS-only test for dealing with symlinks in IndexDiff. Recreates using cgit
+ * a test repository prepared with git 2.2.1 on MacOS 10.7.5 containing some
+ * symlinks. Examines a symlink pointing to a file named "äéü.txt" (should be
+ * encoded as UTF-8 NFC), changes it through Java, examines it again to verify
+ * it's been changed to UTF-8 NFD, and finally calculates an IndexDiff.
+ */
+public class IndexDiffWithSymlinkTest extends LocalDiskRepositoryTestCase {
+
+	private static final String FILEREPO = "filerepo";
+
+	private static final String TESTFOLDER = "testfolder";
+
+	private static final String TESTTARGET = "äéü.txt";
+
+	private static final String TESTLINK = "aeu.txt";
+
+	private static final byte[] NFC = // "äéü.txt" in NFC
+	{ -61, -92, -61, -87, -61, -68, 46, 116, 120, 116 };
+
+	private static final byte[] NFD = // "äéü.txt" in NFD
+	{ 97, -52, -120, 101, -52, -127, 117, -52, -120, 46, 116, 120, 116 };
+
+	private File testRepoDir;
+
+	@Override
+	@Before
+	public void setUp() throws Exception {
+		assumeTrue(SystemReader.getInstance().isMacOS()
+				&& FS.DETECTED.supportsSymlinks());
+		super.setUp();
+		File testDir = createTempDirectory(this.getClass().getSimpleName());
+		InputStream in = this.getClass().getClassLoader().getResourceAsStream(
+				this.getClass().getPackage().getName().replace('.', '/') + '/'
+						+ FILEREPO + ".txt");
+		assertNotNull("Test repo file not found", in);
+		try {
+			testRepoDir = restoreGitRepo(in, testDir, FILEREPO);
+		} finally {
+			in.close();
+		}
+	}
+
+	private File restoreGitRepo(InputStream in, File testDir, String name)
+			throws Exception {
+		File exportedTestRepo = new File(testDir, name + ".txt");
+		copy(in, exportedTestRepo);
+		// Use CGit to restore
+		File restoreScript = new File(testDir, name + ".sh");
+		try (OutputStream out = new BufferedOutputStream(
+				new FileOutputStream(restoreScript));
+				Writer writer = new OutputStreamWriter(out,
+						StandardCharsets.UTF_8)) {
+			writer.write("echo `which git` 1>&2\n");
+			writer.write("echo `git --version` 1>&2\n");
+			writer.write("git init " + name + " && \\\n");
+			writer.write("cd ./" + name + " && \\\n");
+			writer.write("git fast-import < ../" + name + ".txt && \\\n");
+			writer.write("git checkout -f\n");
+		}
+		String[] cmd = { "/bin/sh", "./" + name + ".sh" };
+		int exitCode;
+		String stdErr;
+		Process process = Runtime.getRuntime().exec(cmd, null, testDir);
+		try (InputStream stdOutStream = process.getInputStream();
+				InputStream stdErrStream = process.getErrorStream();
+				OutputStream stdInStream = process.getOutputStream()) {
+			readStream(stdOutStream);
+			stdErr = readStream(stdErrStream);
+			process.waitFor();
+			exitCode = process.exitValue();
+		}
+		if (exitCode != 0) {
+			fail("cgit repo restore returned " + exitCode + '\n' + stdErr);
+		}
+		return new File(new File(testDir, name), Constants.DOT_GIT);
+	}
+
+	private void copy(InputStream from, File to) throws IOException {
+		try (OutputStream out = new FileOutputStream(to)) {
+			byte[] buffer = new byte[4096];
+			int n;
+			while ((n = from.read(buffer)) > 0) {
+				out.write(buffer, 0, n);
+			}
+		}
+	}
+
+	private String readStream(InputStream stream) throws IOException {
+		try (BufferedReader in = new BufferedReader(
+				new InputStreamReader(stream))) {
+			StringBuilder out = new StringBuilder();
+			String line;
+			while ((line = in.readLine()) != null) {
+				out.append(line).append('\n');
+			}
+			return out.toString();
+		}
+	}
+
+	@Test
+	public void testSymlinkWithEncodingDifference() throws Exception {
+		try (Repository testRepo = FileRepositoryBuilder.create(testRepoDir)) {
+			File workingTree = testRepo.getWorkTree();
+			File symLink = new File(new File(workingTree, TESTFOLDER),
+					TESTLINK);
+			// Read the symlink as it was created by cgit
+			Path linkTarget = Files.readSymbolicLink(symLink.toPath());
+			assertEquals("Unexpected link target", TESTTARGET,
+					linkTarget.toString());
+			byte[] raw = rawPath(linkTarget);
+			if (raw != null) {
+				assertArrayEquals("Expected an NFC link target", NFC, raw);
+			}
+			// Now re-create that symlink through Java
+			assertTrue("Could not delete symlink", symLink.delete());
+			Files.createSymbolicLink(symLink.toPath(), Paths.get(TESTTARGET));
+			// Read it again
+			linkTarget = Files.readSymbolicLink(symLink.toPath());
+			assertEquals("Unexpected link target", TESTTARGET,
+					linkTarget.toString());
+			raw = rawPath(linkTarget);
+			if (raw != null) {
+				assertArrayEquals("Expected an NFD link target", NFD, raw);
+			}
+			// Do the indexdiff
+			WorkingTreeIterator iterator = new FileTreeIterator(testRepo);
+			IndexDiff diff = new IndexDiff(testRepo, Constants.HEAD, iterator);
+			diff.setFilter(PathFilterGroup.createFromStrings(
+					Collections.singleton(TESTFOLDER + '/' + TESTLINK)));
+			diff.diff();
+			// We're testing that this does NOT throw "EOFException: Short read
+			// of block." The diff will not report any modified files -- the
+			// link modification is not visible to JGit, which always works with
+			// the Java internal NFC encoding. CGit does report the link as an
+			// unstaged modification here, though.
+		}
+	}
+
+	private byte[] rawPath(Path p) {
+		try {
+			Method method = p.getClass().getDeclaredMethod("asByteArray");
+			if (method != null) {
+				method.setAccessible(true);
+				return (byte[]) method.invoke(p);
+			}
+		} catch (NoSuchMethodException | IllegalAccessException
+				| IllegalArgumentException | InvocationTargetException e) {
+			// Ignore and fall through.
+		}
+		return null;
+	}
+}
--- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/FileTreeIterator.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/FileTreeIterator.java
@ -426,4 +426,9 @@ public class FileTreeIterator extends WorkingTreeIterator {
 	protected byte[] idSubmodule(final Entry e) {
 		return idSubmodule(getDirectory(), e);
 	}
+
+	@Override
+	protected String readSymlinkTarget(Entry entry) throws IOException {
+		return fs.readSymLink(getEntryFile());
+	}
 }
--- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
@ -1008,10 +1008,10 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {

 			return false;
 		} else {
-			if (mode == FileMode.SYMLINK.getBits())
-				return !new File(readContentAsNormalizedString(current()))
-						.equals(new File((readContentAsNormalizedString(entry,
-								reader))));
+			if (mode == FileMode.SYMLINK.getBits()) {
+				return !new File(readSymlinkTarget(current())).equals(
+						new File(readContentAsNormalizedString(entry, reader)));
+			}
 			// Content differs: that's a real change, perhaps
 			if (reader == null) // deprecated use, do no further checks
 				return true;
@ -1057,12 +1057,30 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 		return FS.detect().normalize(RawParseUtils.decode(cachedBytes));
 	}

-	private static String readContentAsNormalizedString(Entry entry) throws IOException {
+	/**
+	 * Reads the target of a symlink as a string. This default implementation
+	 * fully reads the entry's input stream and converts it to a normalized
+	 * string. Subclasses may override to provide more specialized
+	 * implementations.
+	 *
+	 * @param entry
+	 *            to read
+	 * @return the entry's content as a normalized string
+	 * @throws IOException
+	 *             if the entry cannot be read or does not denote a symlink
+	 * @since 4.6
+	 */
+	protected String readSymlinkTarget(Entry entry) throws IOException {
+		if (!entry.getMode().equals(FileMode.SYMLINK)) {
+			throw new java.nio.file.NotLinkException(entry.getName());
+		}
 		long length = entry.getLength();
 		byte[] content = new byte[(int) length];
-		InputStream is = entry.openInputStream();
-		IO.readFully(is, content, 0, (int) length);
-		return FS.detect().normalize(RawParseUtils.decode(content));
+		try (InputStream is = entry.openInputStream()) {
+			int bytesRead = IO.readFully(is, content, 0);
+			return FS.detect()
+					.normalize(RawParseUtils.decode(content, 0, bytesRead));
+		}
 	}

 	private static long computeLength(InputStream in) throws IOException {