Re: heads-up: git-index-pack in "next" is broken
- From
Junio C Hamano <junkio@cox.net>
- Date
- Oct 18, 2006, 05:07 UTC
- Message-ID
- <7vu022gqji.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <7v64eii7hh.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <junkio@cox.net> writes:
Show 8 quoted lines
> Ah, I misread the code that uses union actually checks the type > in struct delta_entry (which embeds the union). There won't be > any collision problem and you support both types at the same > time just fine. > > And your patch to compare only the first 20-bytes makes sense > (assuming ulong is always shorter than 20-bytes which I think is > safe to assume).
Does this sound fair (the code is yours, just asking about the log message)?
If we really wanted to be purist, we could run comparison with the union and obj->type as two keys, but I do not think it is worth it.
-- >8 -- From: Nicolas Pitre <nico@cam.org> Date: Tue, 17 Oct 2006 16:23:26 -0400 Subject: [PATCH] index-pack: compare the first 20-bytes of the key.
The "union delta_base" is a strange beast. It is a 20-byte binary blob key to search a binary searchable deltas[] array, each element of which uses it to represent its base object with either a full 20-byte SHA-1 or an offset in the pack. Which representation is used is determined by another field of the deltas[] array element, obj->type, so there is no room for confusion, as long as we make sure we compare the keys for the same type only with appropriate length. The code compared the full union with memcmp().
When storing the in-pack offset, the union was first cleared before storing an unsigned long, so comparison worked fine.
On 64-bit architectures, however, the union typically is 24-byte long; the code did not clear the remaining 4-byte alignment padding when storing a full 20-byte SHA-1 representation. Using memcmp() to compare the whole union was wrong.
This fixes the comparison to look at the first 20-bytes of the union, regardless of the architecture. As long as ulong is smaller than 20-bytes this works fine.
Signed-off-by: Junio C Hamano <junkio@cox.net> --- index-pack.c | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/index-pack.c b/index-pack.c index fffddd2..56c590e 100644 --- a/index-pack.c +++ b/index-pack.c @@ -23,6 +23,12 @@ union delta_base { unsigned long offset; }; +/* + * Even if sizeof(union delta_base) == 24 on 64-bit archs, we really want + * to memcmp() only the first 20 bytes. + */ +#define UNION_BASE_SZ 20 + struct delta_entry { struct object_entry *obj; @@ -211,7 +217,7 @@ static int find_delta(const union delta_ struct delta_entry *delta = &deltas[next]; int cmp; - cmp = memcmp(base, &delta->base, sizeof(*base)); + cmp = memcmp(base, &delta->base, UNION_BASE_SZ); if (!cmp) return next; if (cmp < 0) { @@ -232,9 +238,9 @@ static int find_delta_childs(const union if (first < 0) return -1; - while (first > 0 && !memcmp(&deltas[first - 1].base, base, sizeof(*base))) + while (first > 0 && !memcmp(&deltas[first - 1].base, base, UNION_BASE_SZ)) --first; - while (last < end && !memcmp(&deltas[last + 1].base, base, sizeof(*base))) + while (last < end && !memcmp(&deltas[last + 1].base, base, UNION_BASE_SZ)) ++last; *first_index = first; *last_index = last; @@ -312,7 +318,7 @@ static int compare_delta_entry(const voi { const struct delta_entry *delta_a = a; const struct delta_entry *delta_b = b; - return memcmp(&delta_a->base, &delta_b->base, sizeof(union delta_base)); + return memcmp(&delta_a->base, &delta_b->base, UNION_BASE_SZ); } static void parse_pack_objects(void)
-- 1.4.2.4.gf9fe