Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output

[Jeff King <peff@peff.net>]

On Wed, Apr 07, 2021 at 03:26:48PM +0000, ZheNing Hu via GitGitGadget wrote:

> diff --git a/ref-filter.c b/ref-filter.c
> index f0bd32f71416..27bbf9b6c8ac 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,
>  	return 0;
>  }
>  
> +void show_ref_array_items(struct ref_array_item **info,
> +			 const struct ref_format *format,
> +			 size_t n)
> +{
> +	struct strbuf final_buf = STRBUF_INIT;
> +	struct strbuf error_buf = STRBUF_INIT;
> +	size_t i;
> +
> +	for (i = 0; i < n; i++) {
> +		if (format_ref_array_item(info[i], format, &final_buf, &error_buf))
> +			die("%s", error_buf.buf);
> +		fwrite(final_buf.buf, 1, final_buf.len, stdout);
> +		strbuf_reset(&error_buf);
> +		strbuf_reset(&final_buf);
> +		putchar('\n');
> +	}
> +	strbuf_release(&error_buf);
> +	strbuf_release(&final_buf);
> +}

I think this is a reasonable direction to take the solution: wrapping
the loop so that the reuse of the buffers can be included there.

But I do wonder if we should go the opposite direction, and get rid of
show_ref_array_item() entirely. It only has two callers, both of which
could just write the loop themselves. That is more code, but perhaps it
would make it more clear what is going on in those callers, and to give
them more flexibility.

I notice there's a third user of the ref-filter.c code in
builtin/branch.c that does not use show_ref_array_item(). Instead, it
loops itself and calls format_ref_array_item(). I think this is because
it is sometimes columnizing the results. Perhaps git-tag and
for-each-ref would want to learn the same trick, in which case they'd be
happy to have the open-coded loop.

Either way, it probably makes sense to introduce the same optimization
to the case in builtin/branch.c.

-Peff