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:

> @@ -762,20 +765,65 @@ static int parse_sha1_header(char *hdr, 
>  	/*
>  	 * The length must be followed by a zero byte
>  	 */
> -	return *hdr ? -1 : 0;
> +	bytes++;
> +	if (*hdr)
> +		bytes = -1;
> +	return bytes;

Why not just say

	return *hdr ? -1 : bytes;

> +}
> +
> +static int parse_binary_sha1_header(char *hdr, char *type, unsigned long *sizep)
> +{
> +	unsigned char c;
> +	int bytes = 1;
> +	unsigned long size;
> +	unsigned object_type, bits;
> +	static const char *typename[8] = {
> +		NULL,	/* OBJ_EXT */
> +		"commit", "tree", "blob", "tag",
> +		NULL, NULL, NULL 
> +	};	
> +
> +	c = *hdr++;
> +	object_type = (c >> 4) & 7;
> +	if (!typename[object_type])
> +		return -1;

You might want to add a comment saying "since the type is lowercase in 
ascii format, object_type must be 6 or 7, which is an invalid object 
type." It took me a little to figure that out...

> +	strcpy(type, typename[object_type]);
> +	size = c & 15;

Just a nit here: I think 0xf is easier to read with boolean operations.

> +	bits = 4;
> +	while (!(c & 0x80)) {
> +		if (bits >= 8*sizeof(unsigned long))
> +			return -1;
> +		c = *hdr++;
> +		size += (unsigned long) (c & 0x7f) << bits;
> +		bytes++;
> +		bits += 7;
> +	}

Are you not losing the last byte by putting the "while" _before_ instead 
of _after_ the loop?

> @@ -1192,7 +1240,7 @@ struct packed_git *find_sha1_pack(const 
>  
>  int sha1_object_info(const unsigned char *sha1, char *type, unsigned long *sizep)
>  {
> -	int status;
> +	int status, hdrlen;
>  	unsigned long mapsize, size;
>  	void *map;
>  	z_stream stream;
> -

This hunk is unnecessary, right?

Ciao,
Dscho