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

["Shawn O. Pearce" <spearce@spearce.org>]

"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.