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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sat, 9 Jun 2007, Johan Herland wrote:

> +#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