Re: [PATCH] Don't allow fast-import tree delta chains to exceed maximum depth

From

Junio C Hamano <gitster@pobox.com>

Date

Nov 14, 2007, 05:46 UTC

Message-ID

<7vhcjpnzvq.fsf@gitster.siamese.dyndns.org>

In-Reply-To

<20071114044842.GA6876@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

>  Junio, this patch is against maint.  It will apply cleanly to maint
>  but is also crafted to ensure it should apply to next with git-am -3.
>  Its a real bug that's lasted a long time in fast-import.  I think
>  it is maint material.

Thanks.

> diff --git a/fast-import.c b/fast-import.c
> index c07e3d8..7544949 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -154,13 +154,16 @@ Format of STDIN stream:
>  
>  #define PACK_ID_BITS 16
>  #define MAX_PACK_ID ((1<<PACK_ID_BITS)-1)
> +#define DEPTH_BITS 13
> +#define MAX_DEPTH ((1<<DEPTH_BITS)-1)
>  
>  struct object_entry
>  {
>  	struct object_entry *next;
>  	uint32_t offset;
> -	unsigned type : TYPE_BITS;
> -	unsigned pack_id : PACK_ID_BITS;
> +	uint32_t type : TYPE_BITS,
> +		pack_id : PACK_ID_BITS,
> +		depth : DEPTH_BITS;
>  	unsigned char sha1[20];
>  };

uint32_t with bit-width specifiers look somewhat funny here...

>  
> @@ -1105,7 +1108,7 @@ static int store_object(
>  		unsigned pos = sizeof(hdr) - 1;
>  
>  		delta_count_by_type[type]++;
> -		last->depth++;
> +		e->depth = ++last->depth++;

"lvalue required as increment operand"?

Wouldn't it be easier to read like this?

diff --git a/fast-import.c b/fast-import.c
index 7544949..d32c412 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -161,7 +161,7 @@ struct object_entry
 {
 	struct object_entry *next;
 	uint32_t offset;
-	uint32_t type : TYPE_BITS,
+	unsigned type : TYPE_BITS,
 		pack_id : PACK_ID_BITS,
 		depth : DEPTH_BITS;
 	unsigned char sha1[20];
@@ -1108,7 +1108,7 @@ static int store_object(
 		unsigned pos = sizeof(hdr) - 1;
 
 		delta_count_by_type[type]++;
-		e->depth = ++last->depth++;
+		e->depth = last->depth + 1;
 
 		hdrlen = encode_header(OBJ_OFS_DELTA, deltalen, hdr);
 		write_or_die(pack_data->pack_fd, hdr, hdrlen);
@@ -1121,8 +1121,6 @@ static int store_object(
 		pack_size += sizeof(hdr) - pos;
 	} else {
 		e->depth = 0;
-		if (last)
-			last->depth = 0;
 		hdrlen = encode_header(type, datlen, hdr);
 		write_or_die(pack_data->pack_fd, hdr, hdrlen);
 		pack_size += hdrlen;
@@ -1138,6 +1136,7 @@ static int store_object(
 			free(last->data);
 		last->data = dat;
 		last->offset = e->offset;
+		last->depth = e->depth;
 		last->len = datlen;
 	}
 	return 0;