[JGIT PATCH] Fix AbstractTreeIterator path comparion betwen 'a' and 'a/b'

[JGIT PATCH] Fix AbstractTreeIterator path comparion betwen 'a' and 'a/b'

[Tor Arne Vestbø]

The occurance of a '/' as the next character in the longer path
does not neccecarily mean the two paths are equal, for example
when the longer path has more components following the '/'.

Signed-off-by: Tor Arne Vestbø <torarnv@gmail.com>
---
 .../jgit/treewalk/AbstractTreeIteratorTest.java    |   93 ++++++++++++++++++++
 .../jgit/treewalk/AbstractTreeIterator.java        |    4 +-
 2 files changed, 95 insertions(+), 2 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java
new file mode 100644
index 0000000..4c74094
--- /dev/null
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java
@@ -0,0 +1,93 @@
+/*
+ * Copyright (C) 2009, Tor Arne Vestbø <torarnv@gmail.com>
+ *
+ * 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 Git Development Community 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.spearce.jgit.treewalk;
+
+import java.io.IOException;
+
+import org.spearce.jgit.errors.IncorrectObjectTypeException;
+import org.spearce.jgit.lib.FileMode;
+import org.spearce.jgit.lib.Repository;
+import org.spearce.jgit.lib.RepositoryTestCase;
+
+
+public class AbstractTreeIteratorTest extends RepositoryTestCase {
+
+
+	public class FakeTreeIterator extends WorkingTreeIterator {
+		public FakeTreeIterator(String path, FileMode fileMode) {
+			super(path);
+			mode = fileMode.getBits();
+			pathLen -= 1; // Get rid of extra '/'
+		}
+
+		@Override
+		public AbstractTreeIterator createSubtreeIterator(Repository repo)
+				throws IncorrectObjectTypeException, IOException {
+			return null;
+		}
+
+	}
+
+	public void testPathCompare() throws Exception {
+
+		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a/b", FileMode.REGULAR_FILE)) < 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a//", FileMode.TREE)) == 0);
+
+		assertTrue(new FakeTreeIterator("a/b", FileMode.REGULAR_FILE).pathCompare(
+				new FakeTreeIterator("a", FileMode.TREE)) > 0);
+
+		assertTrue(new FakeTreeIterator("a//", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a", FileMode.TREE)) == 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.REGULAR_FILE).pathCompare(
+				new FakeTreeIterator("a", FileMode.TREE)) < 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a", FileMode.REGULAR_FILE)) > 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.REGULAR_FILE).pathCompare(
+				new FakeTreeIterator("a", FileMode.REGULAR_FILE)) == 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a", FileMode.TREE)) == 0);
+	}
+
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/treewalk/AbstractTreeIterator.java b/org.spearce.jgit/src/org/spearce/jgit/treewalk/AbstractTreeIterator.java
index 2ff3b99..7dd3f38 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/treewalk/AbstractTreeIterator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/treewalk/AbstractTreeIterator.java
@@ -289,9 +289,9 @@ int pathCompare(final AbstractTreeIterator p, final int pMode) {
 		}
 
 		if (cPos < aLen)
-			return (a[cPos] & 0xff) - lastPathChar(pMode);
+			return ((a[cPos] & 0xff) - lastPathChar(pMode)) + (aLen - cPos - 1);
 		if (cPos < bLen)
-			return lastPathChar(mode) - (b[cPos] & 0xff);
+			return (lastPathChar(mode) - (b[cPos] & 0xff)) - (bLen - cPos - 1);
 		return lastPathChar(mode) - lastPathChar(pMode);
 	}
 
-- 
1.6.1.2.309.g2ea3

Re: [JGIT PATCH] Fix AbstractTreeIterator path comparion betwen 'a' and 'a/b'

[Shawn O. Pearce]

Tor Arne Vestbø <torarnv@gmail.com> wrote:
> The occurance of a '/' as the next character in the longer path
> does not neccecarily mean the two paths are equal, for example
> when the longer path has more components following the '/'.
> 
> Signed-off-by: Tor Arne Vestbø <torarnv@gmail.com>
> ---
>  .../jgit/treewalk/AbstractTreeIteratorTest.java    |   93 ++++++++++++++++++++
>  .../jgit/treewalk/AbstractTreeIterator.java        |    4 +-
>  2 files changed, 95 insertions(+), 2 deletions(-)
>  create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java

*sigh*

I can't get Eclipse to run this test.  Every time I try it comes
up with a CNFE:

  Class not found org.spearce.jgit.treewalk.AbstractTreeIteratorTest
  java.lang.ClassNotFoundException: org.spearce.jgit.treewalk.AbstractTreeIteratorTest
    at java.net.URLClassLoader$1.run(URLClassLoader.java:200)

Aside from this test not running, Eclipse says the tests pass.  But
Maven tells another story:

  (cd jgit-maven/jgit && mvn clean package)
  ...
  Failed tests:
    testNoDF_NoGap(org.spearce.jgit.treewalk.NameConflictTreeWalkTest)
    testDF_GapByOne(org.spearce.jgit.treewalk.NameConflictTreeWalkTest)
    testDF_SkipsSeenSubtree(org.spearce.jgit.treewalk.NameConflictTreeWalkTest)

  Tests run: 773, Failures: 3, Errors: 0, Skipped: 0

Yet under Eclipse this test says its fine.  But Maven actually
managed to run your new test case, where Eclipse didn't.

*confused*
 
-- 
Shawn.

Re: [JGIT PATCH] Fix AbstractTreeIterator path comparion betwen 'a' and 'a/b'

[Tor Arne Vestbø]

Shawn O. Pearce wrote:
> Aside from this test not running, Eclipse says the tests pass.  But
> Maven tells another story: Tests run: 773, Failures: 3, Errors: 0

Strange :/

Nevertheless, there shouldn't be any failures, I see all three of them
when running in Eclipse, so I messed up, sorry for that. Will fix ASAP.

Tor Arne

Re: [JGIT PATCH] Fix AbstractTreeIterator path comparion betwen 'a' and 'a/b'

[Shawn O. Pearce]

Tor Arne Vestbø <torarnv@gmail.com> wrote:
> Shawn O. Pearce wrote:
> > Aside from this test not running, Eclipse says the tests pass.  But
> > Maven tells another story: Tests run: 773, Failures: 3, Errors: 0
> 
> Strange :/
> 
> Nevertheless, there shouldn't be any failures, I see all three of them
> when running in Eclipse, so I messed up, sorry for that. Will fix ASAP.

OK, so I think this is a valid test case, and as it turns out,
it passes with the library unmodified:

--8<--
+	public void testPathCompare() throws Exception {
+		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a", FileMode.TREE)) == 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.REGULAR_FILE).pathCompare(
+				new FakeTreeIterator("a", FileMode.REGULAR_FILE)) == 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.REGULAR_FILE).pathCompare(
+				new FakeTreeIterator("a", FileMode.TREE)) < 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a", FileMode.REGULAR_FILE)) > 0);
+	}
----

Which means whatever problem you have been seeing in the decorator
code is different than what we were originally thinking.  Perhaps you
are trying to use the tree iterator APIs in a way that they aren't
meant to be used (like passing in full paths where only a path name
component is expected?), or there is something else lurking that
we don't understand.

-- 
Shawn.

Re: [JGIT PATCH] Fix AbstractTreeIterator path comparion betwen 'a' and 'a/b'

[Tor Arne Vestbø]

Shawn O. Pearce wrote:
> OK, so I think this is a valid test case, and as it turns out,
> it passes with the library unmodified:
> 
> --8<--
> +	public void testPathCompare() throws Exception {
> +		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
> +				new FakeTreeIterator("a", FileMode.TREE)) == 0);
> +
> +		assertTrue(new FakeTreeIterator("a", FileMode.REGULAR_FILE).pathCompare(
> +				new FakeTreeIterator("a", FileMode.REGULAR_FILE)) == 0);
> +
> +		assertTrue(new FakeTreeIterator("a", FileMode.REGULAR_FILE).pathCompare(
> +				new FakeTreeIterator("a", FileMode.TREE)) < 0);
> +
> +		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
> +				new FakeTreeIterator("a", FileMode.REGULAR_FILE)) > 0);
> +	}
> ----

Okey, I'll post a new patch with just those test cases, no changes to
the pathCompare, so we at least have those tests.

> Which means whatever problem you have been seeing in the decorator
> code is different than what we were originally thinking.  Perhaps you
> are trying to use the tree iterator APIs in a way that they aren't
> meant to be used (like passing in full paths where only a path name
> component is expected?), or there is something else lurking that
> we don't understand.


Ahh, I see. My patch was based on the assumption that 'a/b' was a valid
comparison, since I was seeing similar comparisons in live code. But as
you say, that's probably due to how I use the iterators. I will go back
and investigate how I ended up with 'a/b' paths in the first place.

Thanks :)

Tor Arne

[JGIT PATCH v2] Add a few test cases for AbstractTreeIterator's pathCompare

[Tor Arne Vestbø]

Tests that files sort before trees if the path is the same, and
that if both path and mode is the same, they are considered equal.

Signed-off-by: Tor Arne Vestbø <torarnv@gmail.com>
---
 .../jgit/treewalk/AbstractTreeIteratorTest.java    |   80 ++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java
new file mode 100644
index 0000000..d45e22c
--- /dev/null
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java
@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 2009, Tor Arne Vestbø <torarnv@gmail.com>
+ *
+ * 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 Git Development Community 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.spearce.jgit.treewalk;
+
+import java.io.IOException;
+
+import org.spearce.jgit.errors.IncorrectObjectTypeException;
+import org.spearce.jgit.lib.FileMode;
+import org.spearce.jgit.lib.Repository;
+import org.spearce.jgit.lib.RepositoryTestCase;
+
+
+public class AbstractTreeIteratorTest extends RepositoryTestCase {
+
+
+	public class FakeTreeIterator extends WorkingTreeIterator {
+		public FakeTreeIterator(String path, FileMode fileMode) {
+			super(path);
+			mode = fileMode.getBits();
+			pathLen -= 1; // Get rid of extra '/'
+		}
+
+		@Override
+		public AbstractTreeIterator createSubtreeIterator(Repository repo)
+				throws IncorrectObjectTypeException, IOException {
+			return null;
+		}
+
+	}
+
+	public void testPathCompare() throws Exception {
+		assertTrue(new FakeTreeIterator("a", FileMode.REGULAR_FILE).pathCompare(
+				new FakeTreeIterator("a", FileMode.TREE)) < 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a", FileMode.REGULAR_FILE)) > 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.REGULAR_FILE).pathCompare(
+				new FakeTreeIterator("a", FileMode.REGULAR_FILE)) == 0);
+
+		assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
+				new FakeTreeIterator("a", FileMode.TREE)) == 0);
+	}
+
+}
-- 
1.6.1.2.309.g2ea3

Re: [JGIT PATCH] Fix AbstractTreeIterator path comparion betwen 'a' and 'a/b'

[Shawn O. Pearce]

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Tor Arne Vestbø <torarnv@gmail.com> wrote:
> > The occurance of a '/' as the next character in the longer path
> > does not neccecarily mean the two paths are equal, for example
> > when the longer path has more components following the '/'.
> > 
> > Signed-off-by: Tor Arne Vestbø <torarnv@gmail.com>
> > ---
> >  .../jgit/treewalk/AbstractTreeIteratorTest.java    |   93 ++++++++++++++++++++
> >  .../jgit/treewalk/AbstractTreeIterator.java        |    4 +-
> >  2 files changed, 95 insertions(+), 2 deletions(-)
> >  create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/treewalk/AbstractTreeIteratorTest.java
> 
> *sigh*
> 
> I can't get Eclipse to run this test.  Every time I try it comes
> up with a CNFE:

@#*@!#@!!@*@ MAVEN ECLIPSE PLUGIN.

The Maven Eclipse plugin, which shouldn't have even been invoked
because JGit doesn't use Maven within Eclipse, was crashing and
causing the JDT to stop compiling.  No records was reported in
the Problems view, but my Error Log was full of JDT crashes due
to ClassCastExceptions.  Uninstalling the Maven plugin fixed the
build and made Eclipse come up with the same failures:

>   Failed tests:
>     testNoDF_NoGap(org.spearce.jgit.treewalk.NameConflictTreeWalkTest)
>     testDF_GapByOne(org.spearce.jgit.treewalk.NameConflictTreeWalkTest)
>     testDF_SkipsSeenSubtree(org.spearce.jgit.treewalk.NameConflictTreeWalkTest)
> 
>   Tests run: 773, Failures: 3, Errors: 0, Skipped: 0

So at least my workbench is now reporting the same as Maven on
the command line.  But elsewhere I use Maven (different workbench,
same Eclipse installation) so now I've just shot myself in the foot
and need to install a duplicate copy of Eclipse.  Whoopie.

</unrelated-to-your-patch>

I'm quite certain the breakage in testNoDF_NoGap on line 86 of
NameConflictTreeWalkTest is caused by your change.  Your code is
making "a.b" (a file) equal to "a/" (a tree).  That can't be right.

Going back through the code, the old version of pathCompare
still seems right to me.  And your test cases are somewhat
broken.  We shouldn't ever be looking at cases like this:

  assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
             new FakeTreeIterator("a/b", FileMode.REGULAR_FILE)) < 0);

There's no such thing as a tree entry with '/' in the name.  Really
this should have been:

  assertTrue(new FakeTreeIterator("a", FileMode.TREE).pathCompare(
             new FakeTreeIterator("a", FileMode.TREE)) < 0);

at which point they must be equal, because they are both a tree
named "a".  So that throws off a good chunk of your new test cases.
I don't know how I missed this yesterday when you showed me a version
of the tests, but I did.  We should never be doing a compare of
"a/b" in the pathCompare method.

-- 
Shawn.