Re: [PATCH 14/21] Add proper parsing of "tagger" line, but only when thorough_verify is set

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 10, 2007, 08:58 UTC
Message-ID: <Pine.LNX.4.64.0706100952130.4059@racer.site>
In-Reply-To: <200706090218.41941.johan@herland.net>

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

Show 5 quoted lines

> -	const char *type_line, *tag_line, *tagger_line;
> -	unsigned long type_len, tag_len;
> +	const char   *type_line, *tag_line, *tagger_line;
> +	unsigned long type_len,   tag_len,   tagger_len;
> +	const char *header_end;

This is ugly. Really ugly. Besides, it breaks the minimal patch paradigm.

Show 9 quoted lines

> @@ -81,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,
>  	if (size < 64)
>  		return error("Tag object failed preliminary size check");
>  
> -	/* Verify object line */
> +	/* Verify mandatory object line */
>  	if (prefixcmp(data, "object "))
>  		return error("Tag object (@ char 0): "
>  			"Does not start with \"object \"");

Hmph. I think everybody reading C code understands that this is mandatory. I even think that the comment is useless. It is sort of a code-in-human-language duplicated code.

Show 8 quoted lines

> -	/* Verify the tagger line */
> +	/*
> +	 * Verify mandatory tagger line, but only when we're checking
> +	 * thoroughly, i.e. on inserting a new tag, and on fsck.
> +	 * There are existing tag objects without a tagger line (most
> +	 * notably the "v0.99" tag in the main git repo), and we don't
> +	 * want to fail parsing on these.
> +	 */

I maintain that even with thorough checking, it is _wrong_ to error on a missing tagger. Since we have to deal with tagger-less tags _anyway_, and since it is not like you could really do something about it (the tag is immutable), you should go with a warning.

> -	 * Calculate lengths of header fields.
> +	 * Calculate lengths of header fields (0 for fields that are not given).

Does that really make sense? You effectively treat a missing field as if it were empty. IMHO that is wrong. Besides, this

> +	tagger_len     = header_end > tagger_line ?
> +			(header_end - tagger_line) - 1 : 0;

is really ugly, what with in-line spaces, _and_ special casing.

> -		/* Verify the tagger line */
> +		/* Verify tagger line */

Hmpf.

Ciao, Dscho