Re: [PATCH] Silence error messages unless 'thorough_verify' is set

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 10, 2007, 06:48 UTC
Message-ID: <Pine.LNX.4.64.0706100741310.4059@racer.site>
In-Reply-To: <200706092142.05446.johan@herland.net>

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

Show 11 quoted lines

> +#define FAIL(...) ( thorough_verify ? error(__VA_ARGS__) : -1 )
> +
>  	unsigned char sha1[20];
>  	char type[20];
>  	const char   *type_line, *tag_line, *keywords_line, *tagger_line;
> @@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,
>  	}
>  
>  	if (size < 65)
> -		return error("Tag object failed preliminary size check");
> +		return FAIL("Tag object failed preliminary size check");

This is ugly.

If you _have_ to output the error message in one case, and not in the other, I'd rather do

	enum tag_error { TAG_SIZE_CHECK, TAG_BLA_BLUB, ... };
	const char **tag_error_strings = { "tag: size error", ... };

Of course, you'd lose the ability to output some numbers. But those numbers that you output are even uglier than the code. Guess how surprised _I_ was, when I hit the error message which made me go mad.

Having said that, I still do not agree in this unifying.

Your rationale seems to be: use the same checking for the tag creation as for the tag validation.

But this is _wrong_. We _do_ have tags that do not conform to the strict standards of git-tag, and even if we did _not_, it would _still_ be wrong to be that strict when _reading_ tags.

To drive that point home: strict checking when creating tags is good. Strict checking when reading tags is bad.

I strongly encourage keeping both validations separate.

You'd also avoid having that many lines which are well over the encouraged 80 character limit.

Ciao, Dscho