From de20148f846bdcf90358bb78557997afd717fb2b Mon Sep 17 00:00:00 2001 From: Roberto Tyley Date: Wed, 21 May 2014 12:36:52 +0100 Subject: [PATCH] Fix reading past FileTreeIterator EOF in FileTreeIteratorJava7Test Stepping past the '.git' entry with `fti.next(1)` is unnecessary and in fact a bug, as the subsequent access to FileTreeIterator is past it's end-of-file - it has only 1 valid entry ('link'). This bug is only visibly exposed in certain environments depending on the (unguaranteed) return order of `java.io.File.listFiles()`. On my box FileTreeIteratorJava7Test would fail consistently for these 3 tests: * testSymlinkActuallyModified * testSymlinkNotModifiedThoughNormalized * testSymlinkModifiedNotNormalized They all failed in the same way: testSymlinkActuallyModified(org.eclipse.jgit.treewalk.FileTreeIteratorJava7Test) Time elapsed: 0.063 sec <<< ERROR! org.eclipse.jgit.api.errors.JGitInternalException: /home/roberto/development/jgit/org.eclipse.jgit.java7.test/target/jgit_test_9202429389985749040_tmp /tmp_807992722429349842/.git (Is a directory) at java.io.FileInputStream.open(Native Method) at java.io.FileInputStream.(FileInputStream.java:146) at org.eclipse.jgit.treewalk.FileTreeIterator$FileEntry.openInputStream(FileTreeIterator.java:210) at org.eclipse.jgit.treewalk.WorkingTreeIterator.readContentAsNormalizedString(WorkingTreeIterator.java:984) at org.eclipse.jgit.treewalk.WorkingTreeIterator.contentCheck(WorkingTreeIterator.java:924) at org.eclipse.jgit.treewalk.WorkingTreeIterator.isModified(WorkingTreeIterator.java:860) at org.eclipse.jgit.treewalk.WorkingTreeIterator.isModified(WorkingTreeIterator.java:815) at org.eclipse.jgit.treewalk.FileTreeIteratorJava7Test.testSymlinkActuallyModified(FileTreeIteratorJava7Test.java:198) Theses tests are all working with a small repo that has just two entries: '.git' and 'link' (a symbolic link that's being tested on). `listFiles()` is called by FileTreeIterator to get a preliminary list of FileEntry objects: https://github.com/eclipse/jgit/blob/6d724dcd/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/FileTreeIterator.java#L139 Whether your tests appeared to pass or fail was dependent on the returned order of files from `listFiles()`: * ['.git', 'link'] - PASS (Eclipse Hudson appears to get this ordering) * ['link', '.git'] - FAIL (My env, Ubuntu 14.04/Java 1.7.0_55) The tree-iterator passes the resulting `FileEntry`s to it's init() method: https://github.com/eclipse/jgit/blob/6d724dcd/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java#L639-L665 ... where a count of valid entries is made (`entryCnt`), the 'invalid' entries (like'.git') being left in the hinterland of the `entries` array. The rearrangement in the entries array for our tests looks like this: * ['.git', 'link'] -> ['link', 'link'] * ['link', '.git'] -> ['link', '.git'] In both cases, `entryCnt` is set to 1, meaning that the _valid_ portion of the iterator is the same (ie ['link']), but that the portion after EOF, which we reach by calling `fti.next(1)`, is _different_ depending on your environment. The entry used by the iterator at that point will be either 'link' (if you're lucky) or '.git', which will blow up the test. Note that somewhat ironically, the 'self-check' assertions don't catch this bug, as 'path' data is only parsed _before_ EOF - so `fti.getEntryPathString()` returns the string "link" (and the assertion passes) regardless of whether you're about to read the '.git' entry or not. Change-Id: Ie58a7bc76b740ee52881ebf555564a74379028d6 Signed-off-by: Roberto Tyley --- .../org/eclipse/jgit/treewalk/FileTreeIteratorJava7Test.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/org.eclipse.jgit.java7.test/src/org/eclipse/jgit/treewalk/FileTreeIteratorJava7Test.java b/org.eclipse.jgit.java7.test/src/org/eclipse/jgit/treewalk/FileTreeIteratorJava7Test.java index 2b2d65e9b..2ed56463b 100644 --- a/org.eclipse.jgit.java7.test/src/org/eclipse/jgit/treewalk/FileTreeIteratorJava7Test.java +++ b/org.eclipse.jgit.java7.test/src/org/eclipse/jgit/treewalk/FileTreeIteratorJava7Test.java @@ -107,7 +107,6 @@ public void apply(DirCacheEntry ent) { new Git(db).reset().setMode(ResetType.HARD).call(); DirCacheIterator dci = new DirCacheIterator(db.readDirCache()); FileTreeIterator fti = new FileTreeIterator(db); - fti.next(1); // Skips .git // self-check assertEquals("link", fti.getEntryPathString()); @@ -147,7 +146,6 @@ public void apply(DirCacheEntry ent) { new Git(db).reset().setMode(ResetType.HARD).call(); DirCacheIterator dci = new DirCacheIterator(db.readDirCache()); FileTreeIterator fti = new FileTreeIterator(db); - fti.next(1); // Skips .git // self-check assertEquals("link", fti.getEntryPathString()); @@ -189,7 +187,6 @@ public void apply(DirCacheEntry ent) { FS.DETECTED.createSymLink(new File(trash, "link"), "newtarget"); DirCacheIterator dci = new DirCacheIterator(db.readDirCache()); FileTreeIterator fti = new FileTreeIterator(db); - fti.next(1); // Skips .git // self-check assertEquals("link", fti.getEntryPathString());