Re: [PATCH 2/2] [GSOC] ref-filter: reuse output buffer

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

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/branch.c b/builtin/branch.c
> index bcc00bcf182d..00081de1aed8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -411,6 +411,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>  {
>  	int i;
>  	struct ref_array array;
> +	struct strbuf out = STRBUF_INIT;
> +	struct strbuf err = STRBUF_INIT;
>  	int maxwidth = 0;
>  	const char *remote_prefix = "";
>  	char *to_free = NULL;
> @@ -440,8 +442,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>  	ref_array_sort(sorting, &array);
>  
>  	for (i = 0; i < array.nr; i++) {
> -		struct strbuf out = STRBUF_INIT;
> -		struct strbuf err = STRBUF_INIT;
> +		strbuf_reset(&out);
>  		if (format_ref_array_item(array.items[i], format, &out, &err))
>  			die("%s", err.buf);

This change relies on the fact that format_ref_array_item() will
never touch error when it returns 0 (success); otherwise, we'd end
up accumulating err from multiple calls to it in the loop until it
returns non-zero (failure), at which point we emit a single "fatal:"
prefix to show multiple error messages.  Which leans me ...

> @@ -452,10 +453,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>  			fwrite(out.buf, 1, out.len, stdout);
>  			putchar('\n');
>  		}
> -		strbuf_release(&err);
> -		strbuf_release(&out);
>  	}
>  
> +	strbuf_release(&err);
> +	strbuf_release(&out);

... to suspect that the _release() of err will always be a no-op.

It may be easier to follow if err is _reset() always where out is
_reset(), from code cleanliness's perspective.  Then nobody has to
wonder why we do not reset err inside loop even though we release
at the end.

It also is OK to document more clearly that we assume that the loop
will not exit without calling die() when err is not empty.  If we
take that route, we may want to drop _release(&err) at the end.

I do not know which of the two is better, but the code presented
which is halfway between these two does not quite look easy to
reason about.

Thanks.