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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Apr 22, 2009, 16:33 UTC
Message-ID: <20090422163349.GN23604@spearce.org>
In-Reply-To: <200904220116.51076.robin.rosenberg.lists@dewire.com>

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.

Show 9 quoted lines

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