Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 7, 2021, 20:31 UTC
- Message-ID
- <xmqqczv5zvj3.fsf@gitster.g>
- In-Reply-To
- <pull.927.v2.git.1617809209164.gitgitgadget@gmail.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?
Show 26 quoted lines
> 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.
Show 24 quoted lines
> 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.
Show 11 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)
IOW,
void show_ref_array_items(const struct ref_format *format, struct ref_array_item *info[], size_t n)
Show 8 quoted lines
> +{
> + 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.
Show 8 quoted lines
> + }
> + 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);
}Show 12 quoted lines
> 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?