Re: [PATCH 2/2] [GSOC] ref-filter: reuse output buffer
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 19, 2021, 21:04 UTC
- Message-ID
- <xmqq7dky9ear.fsf@gitster.g>
- In-Reply-To
- <1c7a69ba072ac740273ef06972122f74cf3fa684.1618831726.git.gitgitgadget@gmail.com>
"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.