Re: [PATCH 1/4] builtin/pack-objects.c: remove unnecessary strbuf_reset()

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 29, 2023, 17:34 UTC
Message-ID: <xmqq4jkheor0.fsf@gitster.g>
In-Reply-To: <921a79c8bd6ab688fb9f403a59eeaed3176b630e.1693262936.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 8 quoted lines

> When reading input with the `--cruft` option, `git pack-objects` reads
> each line into a strbuf, and then moves it to either the list of
> discarded or fresh packs, depending on whether or not the input line
> starts with a '-' character.
>
> At the beginning of each loop iteration, the next line of input is read
> with `strbuf_getline()`, which calls `strbuf_reset()` (as a part of
> `strbuf_getwholeline()`) before reading the next line of input.

Yup, the reset() at the end of each iteration is not needed to prepare for the next interation (which getline() is perfectly capable of doing it itself) but helps to release the resource that is no longer used after the loop finishes iterating. If I were cleaning up this code, I would probably move the _reset() after the loop instead *and* keep the _release() at the end, so that future changes can add new (re)uses of buf in between.

But it is probalby not worth worrying about keeping a single line from the cruft object list a little longer than absolutely needed.

Will queue.  Thanks.

Show 22 quoted lines

>
> Thus, the call to `strbuf_reset()` (added back in b757353676
> (builtin/pack-objects.c: --cruft without expiration, 2022-05-20)) at the
> end of the loop is unnecessary, so let's remove it accordingly.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/pack-objects.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index d2a162d528..868efe7e0f 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3603,7 +3603,6 @@ static void read_cruft_objects(void)
>  			string_list_append(&discard_packs, buf.buf + 1);
>  		else
>  			string_list_append(&fresh_packs, buf.buf);
> -		strbuf_reset(&buf);
>  	}
>  
>  	string_list_sort(&discard_packs);