threads / patch / 55517

patch, 2 parts[GSOC] ref-filter: reuse output buffer

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

## tl;dr

9 messages between Apr 19, 2021 and Apr 20, 2021. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

ZheNing Hu via GitGitGadget· Apr 19, 2021, 11:28 UTC · lore
 * Firstly, inlining show_ref_array_item().
 * Secondly, git for-each-ref reuse final buf for all refs output, the
   performance is slightly improved, this optimization is also applied to
   git tag -l and git branch -l.
Thanks.
ZheNing Hu (2):
  [GSOC] ref-filter: get rid of show_ref_array_item
  [GSOC] ref-filter: reuse output buffer
 builtin/branch.c       |  9 +++++----
 builtin/for-each-ref.c | 14 ++++++++++++--
 builtin/tag.c          | 14 ++++++++++++--
 ref-filter.c           | 25 ++++++++++---------------
 ref-filter.h           |  2 --
 5 files changed, 39 insertions(+), 25 deletions(-)
base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-935%2Fadlternative%2Fref-filter-opt-reuse-buf-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-935/adlternative/ref-filter-opt-reuse-buf-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/935
-- 
gitgitgadget
ZheNing Hu via GitGitGadget· Apr 19, 2021, 11:28 UTC · re: ZheNing Hu via GitGitGadget · lore

[PATCH 1/2] [GSOC] ref-filter: get rid of show_ref_array_item

From: ZheNing Hu <adlternative@gmail.com>

Inlining the exported function `show_ref_array_item()` which is not providing the right level of abstraction, simplifies the API and can unlock improvements at the former call sites.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/for-each-ref.c | 14 ++++++++++++--
 builtin/tag.c          | 14 ++++++++++++--
 ref-filter.c           | 25 ++++++++++---------------
 ref-filter.h           |  2 --
 4 files changed, 34 insertions(+), 21 deletions(-)
Show changes to 4 files +34 −21

builtin/for-each-ref.c, builtin/tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index cb9c81a04606..8520008604e3 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -80,8 +80,18 @@ 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);
+	for (i = 0; i < maxcount; i++) {
+		struct strbuf output = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (format_ref_array_item(array.items[i], &format, &output, &err))
+			die("%s", err.buf);
+		fwrite(output.buf, 1, output.len, stdout);
+		putchar('\n');
+
+		strbuf_release(&err);
+		strbuf_release(&output);
+	}
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d403417b5625..d92d8e110b4d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -63,8 +63,18 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
 	ref_array_sort(sorting, &array);
 
-	for (i = 0; i < array.nr; i++)
-		show_ref_array_item(array.items[i], format);
+	for (i = 0; i < array.nr; i++) {
+		struct strbuf output = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (format_ref_array_item(array.items[i], format, &output, &err))
+			die("%s", err.buf);
+		fwrite(output.buf, 1, output.len, stdout);
+		putchar('\n');
+
+		strbuf_release(&err);
+		strbuf_release(&output);
+	}
 	ref_array_clear(&array);
 	free(to_free);
 
diff --git a/ref-filter.c b/ref-filter.c
index f0bd32f71416..df67047fd615 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2435,27 +2435,22 @@ int format_ref_array_item(struct ref_array_item *info,
 	return 0;
 }
 
-void show_ref_array_item(struct ref_array_item *info,
-			 const struct ref_format *format)
-{
-	struct strbuf final_buf = STRBUF_INIT;
-	struct strbuf error_buf = STRBUF_INIT;
-
-	if (format_ref_array_item(info, format, &final_buf, &error_buf))
-		die("%s", error_buf.buf);
-	fwrite(final_buf.buf, 1, final_buf.len, stdout);
-	strbuf_release(&error_buf);
-	strbuf_release(&final_buf);
-	putchar('\n');
-}
-
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
+
 	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
-	show_ref_array_item(ref_item, format);
+	if (format_ref_array_item(ref_item, format, &output, &err))
+		die("%s", err.buf);
+	fwrite(output.buf, 1, output.len, stdout);
+	putchar('\n');
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	free_array_item(ref_item);
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 19ea4c413409..baf72a718965 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -119,8 +119,6 @@ int format_ref_array_item(struct ref_array_item *info,
 			  const struct ref_format *format,
 			  struct strbuf *final_buf,
 			  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);
 /*  Parse a single sort specifier and add it to the list */
 void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
 /*  Callback function for parsing the sort option */
-- 
gitgitgadget
Junio C Hamano· Apr 19, 2021, 20:54 UTC · re: ZheNing Hu via GitGitGadget · lore

Re: [PATCH 1/2] [GSOC] ref-filter: get rid of show_ref_array_item

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> From: ZheNing Hu <adlternative@gmail.com>
>
> Inlining the exported function `show_ref_array_item()`
> which is not providing the right level of abstraction,
> simplifies the API and can unlock improvements at the
> former call sites.

I'll insert a comma right at the end of the first line while queuing.

Thanks.
ZheNing Hu via GitGitGadget· Apr 19, 2021, 11:28 UTC · re: ZheNing Hu via GitGitGadget · lore

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

From: ZheNing Hu <adlternative@gmail.com>

When we use `git for-each-ref`, every ref will allocate its own output strbuf. But we can reuse the final 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 23.7 ms ± 0.9 ms to 22.2 ms ± 1.0 ms. Optimization is relatively minor.

At the same time, we apply this optimization to `git tag -l` and `git branch -l`.

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.

Helped-by: Jeff King <peff@peff.net>
Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/branch.c       |  9 +++++----
 builtin/for-each-ref.c | 12 ++++++------
 builtin/tag.c          | 12 ++++++------
 3 files changed, 17 insertions(+), 16 deletions(-)
Show changes to 3 files +17 −16

builtin/branch.c, builtin/for-each-ref.c, builtin/tag.c

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);
 		if (column_active(colopts)) {
@@ -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);
 	ref_array_clear(&array);
 	free(to_free);
 }
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 8520008604e3..bf24c595c526 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	struct ref_array array;
 	struct ref_filter filter;
 	struct ref_format format = REF_FORMAT_INIT;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &format.quote_style,
@@ -81,17 +83,15 @@ 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++) {
-		struct strbuf output = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
-
+		strbuf_reset(&output);
 		if (format_ref_array_item(array.items[i], &format, &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
-
-		strbuf_release(&err);
-		strbuf_release(&output);
 	}
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d92d8e110b4d..592af1d154ea 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 		     struct ref_format *format)
 {
 	struct ref_array array;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	char *to_free = NULL;
 	int i;
 
@@ -64,17 +66,15 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++) {
-		struct strbuf output = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
-
+		strbuf_reset(&output);
 		if (format_ref_array_item(array.items[i], format, &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
-
-		strbuf_release(&err);
-		strbuf_release(&output);
 	}
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	ref_array_clear(&array);
 	free(to_free);
 
-- 
gitgitgadget
Junio C Hamano· Apr 19, 2021, 21:04 UTC · re: ZheNing Hu via GitGitGadget · lore

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

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 22 quoted lines
> 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 ...

Show 10 quoted lines
> @@ -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.
ZheNing Hu· Apr 20, 2021, 06:05 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> 于2021年4月20日周二 上午5:04写道:
Show 45 quoted lines
>
> "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.
>
Yes, it's a no-op to _release(&err) In the present situation.
Show 13 quoted lines
> 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.
>

René Scharfe mention that it make leaks checking harder if we without releasing this err. So on balance, adding err's _reset() in the loop seems like a viable option. The change in performance will also be minimal too.

Even though we're using _release() in the loop in v1, and then Peff think that we don't need to _release() err, but code cleanness wasn't a concern at the time.

So I'll add _reset() to the loop in the next iteration.
> Thanks.
>

Thanks. -- ZheNing Hu

ZheNing Hu via GitGitGadget· Apr 20, 2021, 16:52 UTC · re: ZheNing Hu via GitGitGadget · lore

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

 * Firstly, inlining show_ref_array_item().
 * Secondly, git for-each-ref reuse final buf for all refs output, the
   performance is slightly improved, this optimization is also applied to
   git tag -l and git branch -l.
Changes made in v2: reset err buffer in loop for code cleanness.
Thanks.
ZheNing Hu (2):
  [GSOC] ref-filter: get rid of show_ref_array_item
  [GSOC] ref-filter: reuse output buffer
 builtin/branch.c       | 10 ++++++----
 builtin/for-each-ref.c | 15 +++++++++++++--
 builtin/tag.c          | 15 +++++++++++++--
 ref-filter.c           | 25 ++++++++++---------------
 ref-filter.h           |  2 --
 5 files changed, 42 insertions(+), 25 deletions(-)
base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-935%2Fadlternative%2Fref-filter-opt-reuse-buf-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-935/adlternative/ref-filter-opt-reuse-buf-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/935
Range-diff vs v1:
 1:  ec98f2177d90 ! 1:  7e378eef4b34 [GSOC] ref-filter: get rid of show_ref_array_item
     @@ Metadata
       ## Commit message ##
          [GSOC] ref-filter: get rid of show_ref_array_item
      
     -    Inlining the exported function `show_ref_array_item()`
     +    Inlining the exported function `show_ref_array_item()`,
          which is not providing the right level of abstraction,
          simplifies the API and can unlock improvements at the
          former call sites.
 2:  1c7a69ba072a ! 2:  a17949b9f809 [GSOC] ref-filter: reuse output buffer
     @@ Commit message
          [GSOC] ref-filter: reuse output buffer
      
          When we use `git for-each-ref`, every ref will allocate
     -    its own output strbuf. But we can reuse the final strbuf
     -    for each step ref's output.
     +    its own output strbuf and error strbuf. But we can reuse
     +    the final strbuf for each step ref's output. The error
     +    buffer will also be reused, despite the fact that the git
     +    will exit when `format_ref_array_item()` return a non-zero
     +    value and output the contents of the error buffer.
      
          The performance for `git for-each-ref` on the Git repository
          itself with performance testing tool `hyperfine` changes from
     @@ Commit message
          (cat-file: use a single strbuf for all output, 2018-08-14)
          to speed up the cat-file builtin.
      
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Helped-by: Jeff King <peff@peff.net>
          Helped-by: René Scharfe <l.s.r@web.de>
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
     @@ builtin/branch.c: static void print_ref_list(struct ref_filter *filter, struct r
       	for (i = 0; i < array.nr; i++) {
      -		struct strbuf out = STRBUF_INIT;
      -		struct strbuf err = STRBUF_INIT;
     ++		strbuf_reset(&err);
      +		strbuf_reset(&out);
       		if (format_ref_array_item(array.items[i], format, &out, &err))
       			die("%s", err.buf);
     @@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc, const char **argv, const
      -		struct strbuf output = STRBUF_INIT;
      -		struct strbuf err = STRBUF_INIT;
      -
     ++		strbuf_reset(&err);
      +		strbuf_reset(&output);
       		if (format_ref_array_item(array.items[i], &format, &output, &err))
       			die("%s", err.buf);
     @@ builtin/tag.c: static int list_tags(struct ref_filter *filter, struct ref_sortin
      -		struct strbuf err = STRBUF_INIT;
      -
      +		strbuf_reset(&output);
     ++		strbuf_reset(&err);
       		if (format_ref_array_item(array.items[i], format, &output, &err))
       			die("%s", err.buf);
       		fwrite(output.buf, 1, output.len, stdout);
-- 
gitgitgadget
ZheNing Hu via GitGitGadget· Apr 20, 2021, 16:52 UTC · re: ZheNing Hu via GitGitGadget · lore

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

From: ZheNing Hu <adlternative@gmail.com>

When we use `git for-each-ref`, every ref will allocate its own output strbuf and error strbuf. But we can reuse the final strbuf for each step ref's output. The error buffer will also be reused, despite the fact that the git will exit when `format_ref_array_item()` return a non-zero value and output the contents of the error buffer.

The performance for `git for-each-ref` on the Git repository itself with performance testing tool `hyperfine` changes from 23.7 ms ± 0.9 ms to 22.2 ms ± 1.0 ms. Optimization is relatively minor.

At the same time, we apply this optimization to `git tag -l` and `git branch -l`.

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.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/branch.c       | 10 ++++++----
 builtin/for-each-ref.c | 13 +++++++------
 builtin/tag.c          | 13 +++++++------
 3 files changed, 20 insertions(+), 16 deletions(-)
Show changes to 3 files +20 −16

builtin/branch.c, builtin/for-each-ref.c, builtin/tag.c

diff --git a/builtin/branch.c b/builtin/branch.c
index bcc00bcf182d..b23b1d1752af 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,8 @@ 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(&err);
+		strbuf_reset(&out);
 		if (format_ref_array_item(array.items[i], format, &out, &err))
 			die("%s", err.buf);
 		if (column_active(colopts)) {
@@ -452,10 +454,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);
 	ref_array_clear(&array);
 	free(to_free);
 }
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 8520008604e3..b529228c6239 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -22,6 +22,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	struct ref_array array;
 	struct ref_filter filter;
 	struct ref_format format = REF_FORMAT_INIT;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &format.quote_style,
@@ -81,17 +83,16 @@ 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++) {
-		struct strbuf output = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
-
+		strbuf_reset(&err);
+		strbuf_reset(&output);
 		if (format_ref_array_item(array.items[i], &format, &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
-
-		strbuf_release(&err);
-		strbuf_release(&output);
 	}
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d92d8e110b4d..82fcfc098242 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -39,6 +39,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 		     struct ref_format *format)
 {
 	struct ref_array array;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	char *to_free = NULL;
 	int i;
 
@@ -64,17 +66,16 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++) {
-		struct strbuf output = STRBUF_INIT;
-		struct strbuf err = STRBUF_INIT;
-
+		strbuf_reset(&output);
+		strbuf_reset(&err);
 		if (format_ref_array_item(array.items[i], format, &output, &err))
 			die("%s", err.buf);
 		fwrite(output.buf, 1, output.len, stdout);
 		putchar('\n');
-
-		strbuf_release(&err);
-		strbuf_release(&output);
 	}
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	ref_array_clear(&array);
 	free(to_free);
 
-- 
gitgitgadget
ZheNing Hu via GitGitGadget· Apr 20, 2021, 16:52 UTC · re: ZheNing Hu via GitGitGadget · lore

[PATCH v2 1/2] [GSOC] ref-filter: get rid of show_ref_array_item

From: ZheNing Hu <adlternative@gmail.com>

Inlining the exported function `show_ref_array_item()`, which is not providing the right level of abstraction, simplifies the API and can unlock improvements at the former call sites.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/for-each-ref.c | 14 ++++++++++++--
 builtin/tag.c          | 14 ++++++++++++--
 ref-filter.c           | 25 ++++++++++---------------
 ref-filter.h           |  2 --
 4 files changed, 34 insertions(+), 21 deletions(-)
Show changes to 4 files +34 −21

builtin/for-each-ref.c, builtin/tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index cb9c81a04606..8520008604e3 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -80,8 +80,18 @@ 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);
+	for (i = 0; i < maxcount; i++) {
+		struct strbuf output = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (format_ref_array_item(array.items[i], &format, &output, &err))
+			die("%s", err.buf);
+		fwrite(output.buf, 1, output.len, stdout);
+		putchar('\n');
+
+		strbuf_release(&err);
+		strbuf_release(&output);
+	}
 	ref_array_clear(&array);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d403417b5625..d92d8e110b4d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -63,8 +63,18 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
 	ref_array_sort(sorting, &array);
 
-	for (i = 0; i < array.nr; i++)
-		show_ref_array_item(array.items[i], format);
+	for (i = 0; i < array.nr; i++) {
+		struct strbuf output = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		if (format_ref_array_item(array.items[i], format, &output, &err))
+			die("%s", err.buf);
+		fwrite(output.buf, 1, output.len, stdout);
+		putchar('\n');
+
+		strbuf_release(&err);
+		strbuf_release(&output);
+	}
 	ref_array_clear(&array);
 	free(to_free);
 
diff --git a/ref-filter.c b/ref-filter.c
index f0bd32f71416..df67047fd615 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2435,27 +2435,22 @@ int format_ref_array_item(struct ref_array_item *info,
 	return 0;
 }
 
-void show_ref_array_item(struct ref_array_item *info,
-			 const struct ref_format *format)
-{
-	struct strbuf final_buf = STRBUF_INIT;
-	struct strbuf error_buf = STRBUF_INIT;
-
-	if (format_ref_array_item(info, format, &final_buf, &error_buf))
-		die("%s", error_buf.buf);
-	fwrite(final_buf.buf, 1, final_buf.len, stdout);
-	strbuf_release(&error_buf);
-	strbuf_release(&final_buf);
-	putchar('\n');
-}
-
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
+	struct strbuf output = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
+
 	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
-	show_ref_array_item(ref_item, format);
+	if (format_ref_array_item(ref_item, format, &output, &err))
+		die("%s", err.buf);
+	fwrite(output.buf, 1, output.len, stdout);
+	putchar('\n');
+
+	strbuf_release(&err);
+	strbuf_release(&output);
 	free_array_item(ref_item);
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 19ea4c413409..baf72a718965 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -119,8 +119,6 @@ int format_ref_array_item(struct ref_array_item *info,
 			  const struct ref_format *format,
 			  struct strbuf *final_buf,
 			  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);
 /*  Parse a single sort specifier and add it to the list */
 void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
 /*  Callback function for parsing the sort option */
-- 
gitgitgadget

← back to recent threads