Re: [JGIT PATCH 10/10] BROKEN TEST: ObjectLoader stays valid across repacks

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

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> So, I had an idea and started hacking and suddenly the supposedly ok cases
> started crashing like this.
> 
> org.spearce.jgit.errors.MissingObjectException: Missing unknown 4a75554761c96be80602c05145d1ef41c77e1b72

You broke it!  :-)

> The hash codes printed are the same everytime it crashes, removing
> the invalid flags will create these codes and the test succeeds.

I am not surprised.  The hash codes are derived from the system
identity hash code for the object, which are assigned using a rather
deterministic algorithm.  Given the same sequence of tests through
the JVM and the same heap state, you are likely going to have the
same hash code for the same objects every run.

Actually, I think you missed it, but the two sets of hash codes
you printed here in the email are identical.
 
> One cannot obviously assume they are the same, but the numbers might be a lead to why it crashes here. Looks
> like as hash collision and a failure of the "equals" part to distinguish different WindowedFile's.

Nope, that's not it.  We never use .equals() on WindowedFile,
and we never use that hash field you were printing as any part
of an equality computation.  That hash field is mixed with the
offset of a window to hash it into the WindowCache.  We *always*
use reference equality ("foo.wp == wp") to test a WindowedFile.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
> index 9293eb9..f9e1991 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
> @@ -80,6 +80,8 @@
>  	/** Total number of windows actively in the associated cache. */
>  	int openCount;
>  
> +	boolean invalid;

This shadowed the "invalid" field in PackFile, causing the anonymous
inner class on l.93-103 that subclasses WindowedFile to write to
this new field, while PackFile still read from its own field.

If you had called this "invalid2", or marked it private, you
wouldn't have seen the test failure.


FWIW, I've decided to merge PackFile and WindowedFile together.
There is no point in keeping them separate anymore.  We used to
use WindowedFile for both the .pack and the .idx, but long ago
you showed it was faster to load the entire .idx into memory in
the PackIndex structure.  And this split is causing confusion,
like the one you just stepped in.

-- 
Shawn.