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

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

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

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

The implementation changed so much from the initial attempt, for
which the above title may have been appropriate, that reusing single
strbuf over and over is not the most important part of the change
anymore, I am afraid.  Besides, it uses TWO strbufs ;-)

Subject: [PATCH] ref-filter: introduce show_ref_array_items() helper

or something like that?

> From: ZheNing Hu <adlternative@gmail.com>
>
> When we use `git for-each-ref`, every ref will call
> `show_ref_array_item()` and allocate its own final strbuf
> and error strbuf. Instead, we can reuse these two strbuf
> for each step ref's output.
>
> The performance for `git for-each-ref` on the Git repository
> itself with performance testing tool `hyperfine` changes from
> 18.7 ms ± 0.4 ms to 18.2 ms ± 0.3 ms.
>
> This approach is similar to the one used by 79ed0a5
> (cat-file: use a single strbuf for all output, 2018-08-14)
> to speed up the cat-file builtin.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>     [GSOC] ref-filter: use single strbuf for all output
>     
>     Now git for-each-ref can reuse two buffers for all refs output, the
>     performance is slightly improved.
>     
>     Now there may be a question : Should the original interface
>     show_ref_array_items be retained?
> ...
>        /*  Callback function for parsing the sort option */

Again, not a very useful range-diff as the implementation changed so much.


>  builtin/for-each-ref.c |  4 +---
>  ref-filter.c           | 20 ++++++++++++++++++++
>  ref-filter.h           |  5 +++++
>  3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index cb9c81a04606..d630402230f3 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -16,7 +16,6 @@ static char const * const for_each_ref_usage[] = {
>  
>  int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  {
> -	int i;
>  	struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
>  	int maxcount = 0, icase = 0;
>  	struct ref_array array;
> @@ -80,8 +79,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  
>  	if (!maxcount || array.nr < maxcount)
>  		maxcount = array.nr;
> -	for (i = 0; i < maxcount; i++)
> -		show_ref_array_item(array.items[i], &format);
> +	show_ref_array_items(array.items, &format, maxcount);

The intention of this call is to pass an array and the number of
elements in the array as a pair to the function, right?  When you
design the API for a new helper function, do not split them apart by
inserting an unrelated parameter in the middle.

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

IOW,

	void show_ref_array_items(const struct ref_format *format,
				  struct ref_array_item *info[], 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);

OK, the contents of error_buf is already localized, so it is correct
not to have _() around the "%s" here.

> +		fwrite(final_buf.buf, 1, final_buf.len, stdout);
> +		strbuf_reset(&error_buf);
> +		strbuf_reset(&final_buf);
> +		putchar('\n');

This is inherited code, but splitting fwrite() and putchar() apart
like this makes the code hard to follow.  Perhaps clean it up later
when nothing else is going on in the code as leftoverbits, outside
the topic.

> +	}
> +	strbuf_release(&error_buf);
> +	strbuf_release(&final_buf);
> +}
> +
>  void show_ref_array_item(struct ref_array_item *info,
>  			 const struct ref_format *format)
>  {

Isn't the point of the new helper function so that this can become a
thin wrapper around it, i.e.

	void show_ref_array_item(...)
        {
		show_ref_array_items(format, &info, 1);
	}

> diff --git a/ref-filter.h b/ref-filter.h
> index 19ea4c413409..eb7e79a6676d 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -121,6 +121,11 @@ int format_ref_array_item(struct ref_array_item *info,
>  			  struct strbuf *error_buf);
>  /*  Print the ref using the given format and quote_style */
>  void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
> +/*  Print the refs using the given format and quote_style and maxcount */
> +void show_ref_array_items(struct ref_array_item **info,
> +			 const struct ref_format *format,
> +			 size_t n);

The inconsistency between "maxcount" vs "n" is irritating.  Calling
the parameter with a name that has the word "info" (because the new
parameter is about that array) and a word like "nelem" to hint that
it is the number of elements in the array) would be sensible.

void show_ref_array_items(const struct ref_format *format,
			  struct ref_array_item *info[], size_t info_count);

or something along the line, perhaps?