Re: [PATCH RFC 02/24] ref-filter: add return value to some functions

> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> Add return flag to format_ref_array_item, show_ref_array_item,
>> get_ref_array_info and populate_value for further using.
>> Need it to handle situations when item is broken but we can not invoke
>> die() because we are in batch mode and all items need to be processed.
>>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored by: Jeff King <peff@peff.net>
>> ---
>>  ref-filter.c | 21 +++++++++++++--------
>>  ref-filter.h |  4 ++--
>>  2 files changed, 15 insertions(+), 10 deletions(-)
>
> Makes sense as a preparatory step to pass the return status
> throughout the call chain.  The functions that actually will detect
> issues should probably gain
>
>         /*
>          * a comment before the function
>          * that documents what it does and what values it returns
>          * to signal the failure.
>          */
>
> before them; those that only pass the errorcode through to the
> caller do not necessarily have to, though.

>
>> -void show_ref_array_item(struct ref_array_item *info,
>> +int show_ref_array_item(struct ref_array_item *info,
>>                        const struct ref_format *format)
>>  {
>>       struct strbuf final_buf = STRBUF_INIT;
>> +     int retval = format_ref_array_item(info, format, &final_buf);
>>
>> -     format_ref_array_item(info, format, &final_buf);
>>       fwrite(final_buf.buf, 1, final_buf.len, stdout);
>>       strbuf_release(&final_buf);
>> -     putchar('\n');
>> +     if (!retval)
>> +             putchar('\n');
>> +     return retval;
>
> This is questionable.  Why does it write final_buf out regardless of
> the return value, even though it refrains from writing terminating LF
> upon non-zero return from the same function that prepared final_buf?
>
> "Because final_buf is left unmodified when formatter returns an
> error, and calling fwrite on an empty buffer ends up being a no-op"
> is a wrong answer---it relies on having too intimate knowledge on
> how the callee happens to work currently.