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

From: Jeff King <peff@peff.net>
Date: Apr 7, 2021, 21:27 UTC
Message-ID: <YG4jxKQ3z1R+8Jfz@coredump.intra.peff.net>
In-Reply-To: <pull.927.v2.git.1617809209164.gitgitgadget@gmail.com>

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

Show 27 quoted lines

> 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