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

["Shawn O. Pearce" <spearce@spearce.org>]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> > +	uint32_t type : TYPE_BITS,
> > +		pack_id : PACK_ID_BITS,
> > +		depth : DEPTH_BITS;
> 
> uint32_t with bit-width specifiers look somewhat funny here...

Sure.  But the prior item is also uint32_t and before that a pointer
so it aligns out nice.  And we're using exactly 32 bits in this field.
So it just sort of made sense to me to declare it was a uint32_t.
 
> > @@ -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?

Yes.  Good call.  :-)
 
> 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;

-- 
Shawn.