Re: heads-up: git-index-pack in "next" is broken

From: Linus Torvalds <torvalds@osdl.org>
Date: Oct 18, 2006, 03:12 UTC
Message-ID: <Pine.LNX.4.64.0610171959180.3962@g5.osdl.org>
In-Reply-To: <Pine.LNX.4.64.0610172140270.1971@xanadu.home>

On Tue, 17 Oct 2006, Nicolas Pitre wrote:

> 
> But there _is_ a flag for damn sake.  Did you at least try to understand 
> the code and not just skim over it from 10000 feet above?

I only looked at it from the patches, and the actual data structure, and they didn't have it, so..

> There is _no_ confusion possible.

Ok. Good.

> Does this mean that, with your own change to xdiff that has just been 
> committed, you actually created a "problem"?  Because this is a change 
> that creates different behaviors whether a 32-bit or 64-bit architecture 
> is used, Right?

If you go back to that discussion, I actually pointed out several times that the whole bug _was_ actually introduced exactly because the xdiff code used things that behave differently depending on word-size.

My suggestion for a _proper_ fix was to not use "unsigned long" for that, and the patch I suggested (and eventually got merged) was to use the _low_ bits of the hash, exactly because the low bits are the ones that act the same, regardless of wordsize.

> But of course not.  We want it to behave differently on 64-bit than 
> 32-bit.

No, we actually don't. Not for xdiff, at least. The last thing you want is for different architectures to get different results. It's horrible. It means that bugs are hard to reproduce, and it means that even code that is "tested" is actually tested only for a particular architecture.

So the bug in xdiff was _exactly_ that somebody - totally incorrectly - thought it should work "better" on 64-bits.

> Please just try to understand why I'm claming this is not important in 
> this very case.  Please do me this favor.

Maybe the code is fine. Maybe the particular detail wasn't important. But the original code didn't have _any_ dependencies on things like structure alignment that caused it to do strange things.

And dammit, the fact is, I think the new format is just worse. I think it was a good thing to have the full SHA1 in the pack-file. I think the code got less understandable, and had more special cases, just because now we have two totally different kinds of deltas. So maybe I'm reacting to the fact that I think the bug happened in the first place for a very simple reason: the data structure wasn't unambiguous any more.

		Linus