Re: [PATCH v6 3/6] t0021: write "OUT" only on success

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

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 26 Jun 2017, at 00:17, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> Lars Schneider <larsxschneider@gmail.com> writes:
>> 
>>> "rot13-filter.pl" used to write "OUT <size>" to the debug log even in case of
>>> an abort or error. Fix this by writing "OUT <size>" to the debug log only in
>>> the successful case if output is actually written.
>> 
>> Again, use of "Fix this" without clarifying what the problem is.  Is
>> this change needed because the size may not be known when the new
>> filter protocol is in use, or something?
>
> How about this?
>
>     "rot13-filter.pl" always writes "OUT <size>" to the debug log at the end
>     of an interaction.
>
>     This works without issues for the existing cases "abort", "error", and 
>     "success". In a subsequent patch 'convert: add "status=delayed" to 
>     filter process protocol' we will add a new case "delayed". In that case 
>     we do not send the data right away and it would be wrong/misleading to
>     the reader if we would write "OUT <size>" to the debug log.

I still do not quite get "we do not send the data right away" as
opposed to "at the end of an interaction" makes it wrong/misleading
to write "OUT <size>"?

    A new response "delayed" that will be introduced in subsequent
    patches accepts the input, without giving the filtered result
    right away, and at that moment, we do not even have the output
    size yet.

might be a very reasonable rationale for omitting "OUT: <size>" in
the output when "delayed" request is seen, but that still does not
explain why it is sensible to drop "OUT: <size>" for error or abort
case.

Having said all that, I tend to agree with the actual change.  When
we abort or error, we aren't producing any useful output, so it may
be sensible _not_ to say "OUT: 0" in the log output from the test
helper in these cases.  And if the change is explained that way,
readers would understand why this step is a good thing to have, with
or without the future "delayed" step in the series.

Thanks.