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

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

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

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