Re: [JGIT PATCH 6/5 v2] Add tests for RevWalk and its supporting code

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

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> 
> A few /09 interesting/ places in StartGenerator are not covered by the tests.
> 
> 		if (q instanceof DateRevQueue)
> 			pending = (DateRevQueue)q;
> 		else
> -->			pending = new DateRevQueue(q);

This one is difficult to test.  Near as I can tell from the code
in the revwalk package, it never happens.  But if we did get here
with a non date rev queue, DateRevQueue()'s constructor can copy
the data from q over to one with no risk.

Perhaps just test this constructor in isolation?

> and
> 
> 		if (tf != TreeFilter.ALL) {
> -->			rf = AndRevFilter.create(rf, new RewriteTreeFilter(w, tf));
> -->			pendingOutputType |= HAS_REWRITE | NEEDS_REWRITE;
> 		}

That says we have no tests on the path filter code.  Adding one is
likely to identify at least one bug.  Clearly we need additional
tests on the path filter sections.  And it isn't just for this one
little conditional; the entire RewriteTreeFilter and RewriteGenerator
are untested right now.

I can work on adding more test coverage here, but its not in the
critical path for me at day-job right now.  The other problems I
tried to address in this series were.  So its lower in my queue
so-to-speak.  But I'll see what I can do in the near future to
write additional tests for this area of the revwalk package.
 
> PendingGenerator
> 						if (n != null && n.commitTime >= last.commitTime) {
> 							// This is too close to call. The next commit we
> 							// would pop is dated after the last one produced.
> 							// We have to keep going to ensure that we carry
> 							// flags as much as necessary.
> 							//
> -->							overScan = OVER_SCAN;

*sigh*  We never get here in the test suite? I missed that.  I'll try to
work out a test case today and send an additional patch to get coverage
here.  I thought RevWalkCullTest testProperlyCullAllAncestors1() would
be hitting here.  It doesn't.
 
> I'll merge it anyway since this is still a huge improvement compared to the previous
> state.

Thanks.

A co-worker and I are also currently trying to track down a deadlock
between C git-fetch and JGit upload-pack.  Both get stuck waiting
for the other to answer.  Clearly its JGit's fault.

-- 
Shawn.