From 24f82b533a1774f03fbedb5f32854aa8858fce6d Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Wed, 25 Mar 2020 09:13:20 +0100 Subject: [PATCH 1/2] Handle non-normalized index also for executable files Commit 60cf85a4 corrected the handling of check-in for files where the index version is non-normalized, i.e., contains CR-LF line endings. However, it did so only for regular files, not executable files. Bug: 561438 Change-Id: I372cc990c5efeb00315460f36459c0652d5d1e77 Signed-off-by: Thomas Wolf --- .../eclipse/jgit/api/CommitCommandTest.java | 42 +++++++++++++++---- .../eclipse/jgit/api/StatusCommandTest.java | 25 +++++++++++ .../jgit/treewalk/WorkingTreeIterator.java | 7 +++- 3 files changed, 63 insertions(+), 11 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CommitCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CommitCommandTest.java index b5661e844..b77492117 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CommitCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CommitCommandTest.java @@ -49,6 +49,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.Assume.assumeTrue; import java.io.File; import java.util.Date; @@ -633,40 +634,63 @@ public class CommitCommandTest extends RepositoryTestCase { } } - @Test - public void commitWithAutoCrlfAndNonNormalizedIndex() throws Exception { + private void nonNormalizedIndexTest(boolean executable) throws Exception { + String mode = executable ? "100755" : "100644"; try (Git git = new Git(db)) { // Commit a file with CR/LF into the index FileBasedConfig config = db.getConfig(); config.setString("core", null, "autocrlf", "false"); config.save(); - writeTrashFile("file.txt", "line 1\r\nline 2\r\n"); + File testFile = writeTrashFile("file.txt", "line 1\r\nline 2\r\n"); + if (executable) { + FS.DETECTED.setExecute(testFile, true); + } git.add().addFilepattern("file.txt").call(); git.commit().setMessage("Initial").call(); assertEquals( - "[file.txt, mode:100644, content:line 1\r\nline 2\r\n]", + "[file.txt, mode:" + mode + + ", content:line 1\r\nline 2\r\n]", indexState(CONTENT)); config.setString("core", null, "autocrlf", "true"); config.save(); writeTrashFile("file.txt", "line 1\r\nline 1.5\r\nline 2\r\n"); - writeTrashFile("file2.txt", "new\r\nfile\r\n"); + testFile = writeTrashFile("file2.txt", "new\r\nfile\r\n"); + if (executable) { + FS.DETECTED.setExecute(testFile, true); + } git.add().addFilepattern("file.txt").addFilepattern("file2.txt") .call(); git.commit().setMessage("Second").call(); assertEquals( - "[file.txt, mode:100644, content:line 1\r\nline 1.5\r\nline 2\r\n]" - + "[file2.txt, mode:100644, content:new\nfile\n]", + "[file.txt, mode:" + mode + + ", content:line 1\r\nline 1.5\r\nline 2\r\n]" + + "[file2.txt, mode:" + mode + + ", content:new\nfile\n]", indexState(CONTENT)); writeTrashFile("file2.txt", "new\r\nfile\r\ncontent\r\n"); git.add().addFilepattern("file2.txt").call(); git.commit().setMessage("Third").call(); assertEquals( - "[file.txt, mode:100644, content:line 1\r\nline 1.5\r\nline 2\r\n]" - + "[file2.txt, mode:100644, content:new\nfile\ncontent\n]", + "[file.txt, mode:" + mode + + ", content:line 1\r\nline 1.5\r\nline 2\r\n]" + + "[file2.txt, mode:" + mode + + ", content:new\nfile\ncontent\n]", indexState(CONTENT)); } } + @Test + public void commitWithAutoCrlfAndNonNormalizedIndex() throws Exception { + nonNormalizedIndexTest(false); + } + + @Test + public void commitExecutableWithAutoCrlfAndNonNormalizedIndex() + throws Exception { + assumeTrue(FS.DETECTED.supportsExecute()); + nonNormalizedIndexTest(true); + } + @Test public void testDeletionConflictWithAutoCrlf() throws Exception { try (Git git = new Git(db)) { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StatusCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StatusCommandTest.java index f3ac65ca0..18580ae73 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StatusCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StatusCommandTest.java @@ -45,6 +45,7 @@ package org.eclipse.jgit.api; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; import java.io.File; import java.io.IOException; @@ -54,6 +55,8 @@ import org.eclipse.jgit.api.errors.NoFilepatternException; import org.eclipse.jgit.errors.NoWorkTreeException; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.lib.Sets; +import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.util.FS; import org.junit.Test; public class StatusCommandTest extends RepositoryTestCase { @@ -168,4 +171,26 @@ public class StatusCommandTest extends RepositoryTestCase { assertEquals(Sets.of("a", "D/b", "D/D/d"), stat.getModified()); } } + + @Test + public void testExecutableWithNonNormalizedIndex() throws Exception { + assumeTrue(FS.DETECTED.supportsExecute()); + try (Git git = new Git(db)) { + // Commit a file with CR/LF into the index + FileBasedConfig config = db.getConfig(); + config.setString("core", null, "autocrlf", "false"); + config.save(); + File testFile = writeTrashFile("file.txt", "line 1\r\nline 2\r\n"); + FS.DETECTED.setExecute(testFile, true); + git.add().addFilepattern("file.txt").call(); + git.commit().setMessage("Initial").call(); + assertEquals( + "[file.txt, mode:100755, content:line 1\r\nline 2\r\n]", + indexState(CONTENT)); + config.setString("core", null, "autocrlf", "true"); + config.save(); + Status status = git.status().call(); + assertTrue("Expected no differences", status.isClean()); + } + } } 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 7f2d5365a..35d6e41a9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java @@ -1500,7 +1500,7 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator { } // Read blob from index and check for CR/LF-delimited text. DirCacheEntry entry = dirCache.getDirCacheEntry(); - if (FileMode.REGULAR_FILE.equals(entry.getFileMode())) { + if ((entry.getRawMode() & FileMode.TYPE_MASK) == FileMode.TYPE_FILE) { ObjectId blobId = entry.getObjectId(); if (entry.getStage() > 0 && entry.getStage() != DirCacheEntry.STAGE_2) { @@ -1517,7 +1517,10 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator { break; } if (entry.getStage() == DirCacheEntry.STAGE_2) { - blobId = entry.getObjectId(); + if ((entry.getRawMode() + & FileMode.TYPE_MASK) == FileMode.TYPE_FILE) { + blobId = entry.getObjectId(); + } break; } } From 2640d38f140fd9239395b9756c8b1f828ed37dd4 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Thu, 2 Apr 2020 21:15:18 +0200 Subject: [PATCH 2/2] FS.runInShell(): handle quoted filters and hooksPath containing blanks Revert commit 2323d7a. Using $0 in the shell command call results in the command string being taken literally. That was introduced to fix a problem with backslashes, but is actually not correct. First, the problem with backslashes occurred only on Win32/Cygwin, and has been properly fixed in commit 6f268f8. Second, this is used only for hooks (which don't have backslashes in their names) and filter commands from the git config, where the user is responsible for properly quoting or escaping such that the commands work. Third, using $0 actually breaks correctly quoted filter commands like in the bug report. The shell really takes the command literally, and then doesn't find the command because of quotes. So revert this change. At the same time there's a related problem with hooks. If the path to the hook contains blanks, runInShell() would also fail to find the hook. In this case, the command doesn't come from user input but is just a Java File object with an absolute path containing blanks. (Can occur if core.hooksPath points to such a path with blanks, or if the repository has such a path.) The path to the hook as obtained from the file system must be quoted. Add a test for a hook path with a blank. This reverts commit 2323d7a1ef909f9deb3f21329cf30bd1173ee9cf. Bug: 561666 Change-Id: I4d7df13e6c9b245fe1706e191e4316685a8a9d59 Signed-off-by: Thomas Wolf --- .../tst/org/eclipse/jgit/util/HookTest.java | 32 +++++++++++++++++++ .../src/org/eclipse/jgit/util/FS.java | 17 +++++++++- .../src/org/eclipse/jgit/util/FS_POSIX.java | 7 +++- .../eclipse/jgit/util/FS_Win32_Cygwin.java | 9 ++++-- 4 files changed, 61 insertions(+), 4 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/HookTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/HookTest.java index 70a2dbbfe..26653dbcc 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/HookTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/HookTest.java @@ -291,6 +291,38 @@ public class HookTest extends RepositoryTestCase { } } + @Test + public void testHookPathWithBlank() throws Exception { + assumeSupportedPlatform(); + + File file = writeHookFile("../../a directory/" + PreCommitHook.NAME, + "#!/bin/sh\necho \"test $1 $2\"\nread INPUT\necho $INPUT\n" + + "echo $GIT_DIR\necho $GIT_WORK_TREE\necho 1>&2 \"stderr\""); + StoredConfig cfg = db.getConfig(); + cfg.load(); + cfg.setString(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_HOOKS_PATH, + file.getParentFile().getAbsolutePath()); + cfg.save(); + try (ByteArrayOutputStream out = new ByteArrayOutputStream(); + ByteArrayOutputStream err = new ByteArrayOutputStream()) { + ProcessResult res = FS.DETECTED.runHookIfPresent(db, + PreCommitHook.NAME, new String[] { "arg1", "arg2" }, + new PrintStream(out), new PrintStream(err), "stdin"); + + assertEquals("unexpected hook output", + "test arg1 arg2\nstdin\n" + + db.getDirectory().getAbsolutePath() + '\n' + + db.getWorkTree().getAbsolutePath() + '\n', + out.toString("UTF-8")); + assertEquals("unexpected output on stderr stream", "stderr\n", + err.toString("UTF-8")); + assertEquals("unexpected exit code", 0, res.getExitCode()); + assertEquals("unexpected process status", ProcessResult.Status.OK, + res.getStatus()); + } + } + @Test public void testFailedPreCommitHookBlockCommit() throws Exception { assumeSupportedPlatform(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java index 2446de4c1..41bfde991 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java @@ -1747,7 +1747,7 @@ public abstract class FS { return new ProcessResult(Status.NOT_PRESENT); } String cmd = hookFile.getAbsolutePath(); - ProcessBuilder hookProcess = runInShell(cmd, args); + ProcessBuilder hookProcess = runInShell(shellQuote(cmd), args); hookProcess.directory(runDirectory.getAbsoluteFile()); Map environment = hookProcess.environment(); environment.put(Constants.GIT_DIR_KEY, @@ -1770,6 +1770,21 @@ public abstract class FS { } } + /** + * Quote a string (such as a file system path obtained from a Java + * {@link File} or {@link Path} object) such that it can be passed as first + * argument to {@link #runInShell(String, String[])}. + *

+ * This default implementation returns the string unchanged. + *

+ * + * @param cmd + * the String to quote + * @return the quoted string + */ + String shellQuote(String cmd) { + return cmd; + } /** * Tries to find a hook matching the given one in the given repository. diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java index 79c095fc1..9c8dab68c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java @@ -261,7 +261,7 @@ public class FS_POSIX extends FS { List argv = new ArrayList<>(4 + args.length); argv.add("sh"); //$NON-NLS-1$ argv.add("-c"); //$NON-NLS-1$ - argv.add("$0 \"$@\""); //$NON-NLS-1$ + argv.add(cmd + " \"$@\""); //$NON-NLS-1$ argv.add(cmd); argv.addAll(Arrays.asList(args)); ProcessBuilder proc = new ProcessBuilder(); @@ -269,6 +269,11 @@ public class FS_POSIX extends FS { return proc; } + @Override + String shellQuote(String cmd) { + return QuotedString.BOURNE.quote(cmd); + } + /** {@inheritDoc} */ @Override public ProcessResult runHookIfPresent(Repository repository, String hookName, diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32_Cygwin.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32_Cygwin.java index 41c239f1a..ac788a668 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32_Cygwin.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32_Cygwin.java @@ -149,14 +149,19 @@ public class FS_Win32_Cygwin extends FS_Win32 { List argv = new ArrayList<>(4 + args.length); argv.add("sh.exe"); //$NON-NLS-1$ argv.add("-c"); //$NON-NLS-1$ - argv.add("$0 \"$@\""); //$NON-NLS-1$ - argv.add(cmd.replace(File.separatorChar, '/')); + argv.add(cmd + " \"$@\""); //$NON-NLS-1$ + argv.add(cmd); argv.addAll(Arrays.asList(args)); ProcessBuilder proc = new ProcessBuilder(); proc.command(argv); return proc; } + @Override + String shellQuote(String cmd) { + return QuotedString.BOURNE.quote(cmd.replace(File.separatorChar, '/')); + } + /** {@inheritDoc} */ @Override public String relativize(String base, String other) {