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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 26, 2018, 21:05 UTC
Message-ID: <xmqq1sic72ku.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <0102016133ff3ad8-abf99952-4df0-42a4-9ae9-06f602e84f3b-000000@eu-west-1.amazonses.com>

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

Show 12 quoted lines

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

Show 14 quoted lines

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