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

From: Linus Torvalds <torvalds@osdl.org>
Date: Oct 17, 2006, 20:51 UTC
Message-ID: <Pine.LNX.4.64.0610171339030.3962@g5.osdl.org>
In-Reply-To: <Pine.LNX.4.64.0610171615340.1971@xanadu.home>

On Tue, 17 Oct 2006, Nicolas Pitre wrote:

Show 6 quoted lines

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

Quite frankly, this all makes me go "Eww..". The original pack-file (well, v2) format was well-defined and had none of these issues. In contrast, the new code in 'next' is just _ugly_.

And the thing about "ugly" is that it also tends to mean "fragile" and "buggy" and "hard to extend later".

And maybe it's just me, but I consider unions to be bug-prone on their own. The "master" branch has exactly two unions: the "grep_expr" structure contains one (where the union member is clearly defined by the node type in that structure), and object.c has a "union any_object" that _literally_ exists as purely an allocation size issue (ie it is used _only_ to allocate the maximum size of any of the possible structures).

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.

Is this something you want to bet a big project on?

			Linus