Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> Update sha1_loose_object_info() to optionally allow it to read
> from a loose object file of unknown/bogus type; as the function
> usually returns the type of the object it read in the form of enum
> for known types, add an optional "typename" field to receive the
> name of the type in textual form and a flag to indicate the reading
> of a loose object file of unknown/bogus type.
>
> Add parse_sha1_header_extended() which acts as a wrapper around
> parse_sha1_header() allowing more information to be obtained.
>
> Add unpack_sha1_header_to_strbuf() to unpack sha1 headers of
> unknown/corrupt objects which have a unknown sha1 header size to
> a strbuf structure. This was written by Junio C Hamano but tested
> by me.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---

I see that you made the type parsing to happen earlier than the
previous round (which used to do the size first and then type).

Not a problem, though.  Just something I noticed...

> @@ -1614,27 +1642,40 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
>   * too permissive for what we want to check. So do an anal
>   * object header parse by hand.
>   */
> -int parse_sha1_header(const char *hdr, unsigned long *sizep)
> +int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
> +			       unsigned int flags)
>  {
> -	char type[10];
> -	int i;
> +	struct strbuf typename = STRBUF_INIT;
>  	unsigned long size;
> +	int type;
>  
>  	/*
>  	 * The type can be at most ten bytes (including the
>  	 * terminating '\0' that we add), and is followed by
>  	 * a space.
>  	 */
> -	i = 0;
>  	for (;;) {
>  		char c = *hdr++;
>  		if (c == ' ')
>  			break;
> -		type[i++] = c;
> -		if (i >= sizeof(type))
> -			return -1;
> +		strbuf_addch(&typename, c);
>  	}
> -	type[i] = 0;

This _might_ have some performance impact in that strbuf_addch()
involves strbuf_grow(*, 1), which does "does it overflow to
increment sb->len by one?"; I would say it should be unmeasurable
because the function is expected to be used only on loose objects
and you shouldn't have very many of them without packing in your
repository in the first place.

I guess Peff's c1822d4f (strbuf: add an optimized 1-character
strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his
new strbuf_grow_ch(), and once that happens the performance worry
would disappear without this code to be changed at all.

> @@ -2541,7 +2596,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
>  	 * return value implicitly indicates whether the
>  	 * object even exists.
>  	 */
> -	if (!oi->typep && !oi->sizep) {
> +	if (!oi->typep && !oi->typename && !oi->sizep) {

You didn't have this check in the earlier round, and this new one is
correct, I think.  Good eyes to spot this potential problem.

Thanks.