Re: [PATCH 3/5] optimize parse_sha1_header() a little by detecting object type

From: Shawn O. Pearce <spearce@spearce.org>
Date: Dec 2, 2008, 15:53 UTC
Message-ID: <20081202155300.GL23984@spearce.org>
In-Reply-To: <49349579.2030506@gmail.com>

Liu Yubao <yubao.liu@gmail.com> wrote:

Show 14 quoted lines

> diff --git a/sha1_file.c b/sha1_file.c
> index dccc455..79062f0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1099,7 +1099,8 @@ static void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
>  
>  		if (!fstat(fd, &st)) {
>  			*size = xsize_t(st.st_size);
> -			map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
> +			if (*size > 0)
> +				map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
>  		}
>  		close(fd);
>  	}

This has nothing to do with this change description. Why are we returning NULL from map_sha1_file when the file length is 0 bytes? No loose object should ever be an empty file, there must always be some sort of type header present. So it probably is an error to have a 0 length file here. But that bug is a different change.

Show 9 quoted lines

> @@ -1257,6 +1258,8 @@ static int parse_sha1_header(const char *hdr, unsigned long length, unsigned lon
>  	 * terminating '\0' that we add), and is followed by
>  	 * a space, at least one byte for size, and a '\0'.
>  	 */
> +	if ('b' != *hdr && 'c' != *hdr && 't' != *hdr)	/* blob/commit/tag/tree */
> +		return -1;
>  	i = 0;
>  	while (hdr < hdr_end - 2) {
>  		char c = *hdr++;

Oh. I wouldn't do that. Its a cute trick and it works to quickly determine if the header is an uncompressed header vs. a zlib header vs. a new-style loose object header (which git cannot write anymore, but it still can read). But its just asking for trouble when/if a new object type was ever added to the type table.

Given that we know that no type name can be more than 10 bytes and if you use my patch from earlier today you can be certain hdr has a '\0' terminator, so you could write a function to test for the type against the hdr, stopping on either ' ' or '\0'. Or find the first ' ' in the first 10 bytes (which is what this loop does anyway) and then test that against the type name table.

-- 
Shawn.