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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2007, 08:15 UTC
Message-ID: <7vwsyc8bt3.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0706100741310.4059@racer.site>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 10 quoted lines

> On Sat, 9 Jun 2007, Johan Herland wrote:
> ...
>> @@ -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.

... quite a bit. A less uglier alternative we seem to use in other places is not much better (return NULL on failure or an error message string on error).

> ...  Guess how surprised 
> _I_ was, when I hit the error message which made me go mad.

To be fair, that ugly "char%d" was taken from mktag and not Johan's invention.

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

While I tend ot think that keeping two separate versions is probably better for this particular case, the above statement has a leap in its logic. With your "error code" scheme, you could implement a single, verifier/parser that defines the concrete and complete rule of how the data should look like. That unified verifier/parser itself should be silent. Then, you can have each of the callers decide how lenient it wants to be, depending on the seriousness of the error. You can make producer very strict and chatty while leaving consumer liberal and more silent.

There are pros-and-cons, however.

 - Such a scheme to return error codes and have two callers that
   have different behaviours is cumbersome to set up and use.

   A good example of this is the switch/case mess in each of the
   callers of run_command_v_opt() in builtin-push.c,
   builtin-revert.c, receive-pack.c etc.  For run_command, the
   mess is justifiable because the function has enough number of
   different callers, but in the current thread, we are only
   talking about two callers (parsing vs verifying of tag
   objects).

 - It has a risk to introduce inconsitent definition of the data
   format to have completely separate producer and consumer
   implementations; this is especially true when the data in
   question is complex.

   However, a tag is sufficiently simple that my personal
   feeling is that, combined with the cumbersomeness argument
   against the unified verifier, separate producer and consumer
   implementations would be easier to manage for this particular
   case.