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

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

Liu Yubao <yubao.liu@gmail.com> wrote:
> 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.

> @@ -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.