Re: [PATCH 2/3] sha1_file: add the ability to parse objects in "pack file format"

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Tue, 11 Jul 2006, Linus Torvalds wrote:

> On Tue, 11 Jul 2006, Junio C Hamano wrote:
> > 
> > I do not like to rely too heavily on what zlib compression's
> > beginning of stream looks like.
> 
> Well, I normally would agree with you if it was a "oh, all our zlib 
> objects seem to start with 0x78" thing, but after having dug into both the 
> zlib standard (which is actually an RFC, not just some random thing), and 
> looked at the sources, it's definitely the case that the "0x78" byte isn't 
> just an implementation detail.
> 
> It's actually really part of the specs, and not just happenstance.

Good.

> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -684,26 +684,74 @@ static void *map_sha1_file_internal(cons
>  	return map;
>  }
>  
> -static int unpack_sha1_header(z_stream *stream, void *map, unsigned long mapsize, void *buffer, unsigned long size)
> +static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)
>  {
> +	unsigned char c;
> +	unsigned int word, bits;
> +	unsigned long size;
> +	static const char *typename[8] = {
> +		NULL,	/* OBJ_EXT */
> +		"commit", "tree", "blob", "tag",
> +		NULL, NULL, NULL
> +	};

I completely forgot to mention that type_names[] is already declared in 
object.h. Obviously, it is not really important, but maybe it would be 
better to obey the DRY principle (think addition of "bind" object type).

> +	while (!(c & 0x80)) {
> +		if (bits >= 8*sizeof(long))

Another nit: while it is safe to assume that sizeof(long) == 
sizeof(unsigned long), it was nevertheless a little confusing to yours 
truly (especially since you changed it since your last patch).

>  static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size)
>  {
>  	int bytes = strlen(buffer) + 1;
>  	unsigned char *buf = xmalloc(1+size);
> +	unsigned long n;
>  
> -	memcpy(buf, (char *) buffer + bytes, stream->total_out - bytes);
> -	bytes = stream->total_out - bytes;
> +	n = stream->total_out - bytes;
> +	if (n > size)
> +		n = size;

Just out of curiosity: when can this happen? I mean, there is no error or 
something which could tell the caller that not the whole object was 
copied.

>  	memset(&stream, 0, sizeof(stream));
>  	deflateInit(&stream, zlib_compression_level);
> -	size = deflateBound(&stream, len+hdrlen);
> +	size = 8 + deflateBound(&stream, len+hdrlen);

Again, I had to think why this is correct. I think it should be something 
like 2 + sizeof(unsigned long) * 8 / 7, but I did not think all that hard.

It looks good to me, but I did not really test...

Ciao,
Dscho