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

From: Junio C Hamano <junkio@cox.net>
Date: Oct 17, 2006, 21:54 UTC
Message-ID: <7vr6x6ip5d.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0610171339030.3962@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

Show 13 quoted lines

> On Tue, 17 Oct 2006, Nicolas Pitre wrote:
>> On Tue, 17 Oct 2006, Sergey Vlasov wrote:
>> > 
>> > Yes, on x86_64 this is 24 because of 8-byte alignment for longs:
>> 
>> Ah bummer.  Then this is most likely the cause.  And here's a simple 
>> fix (Junio please confirm):
>
> Why do you use "unsigned long" in the first place?
>
> For some structure like this, it sounds positively wrong. Pack-files 
> should be architecture-neutral, which means that they shouldn't depend on 
> word-size, and they should be in some neutral byte-order.

This is an in-core structure if I am not mistaken, and is never written out in the resulting pack file. The program is reading from .pack and building .idx that corresponds to it. I agree that it is ugly, but I do not hink using neutral byte-order in this part of the processing buys us anything in this particular case.

> In contrast, the new union introduced in "next" is just horrid. There's 
> not even any way to know which member to use, except apparently that it 
> expects that a SHA1 is never zero in the last 12 bytes. Which is probably 
> true, but still - that's some ugly stuff.

Again I agree with the ugliness objection, and I raised the "expecting no collision" issue which Nico refuted later. The code is probably safe (not just safe in practice but safe in theory as well), although that would not change the fact that it is ugly X-<.

Nico, could we have an un-uglied version please?