Re: [PATCH v2 01/27] t/helper/test-fast-rebase.c: don't leak "struct strbuf"

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 24, 2022, 04:13 UTC
Message-ID: <xmqqr16sm1im.fsf@gitster.g>
In-Reply-To: <patch-v2-01.27-8fbafe61053-20220323T203149Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 13 quoted lines

> Fix a memory leak that's been with us since f9500261e0a (fast-rebase:
> write conflict state to working tree, index, and HEAD, 2021-05-20)
> changed this code to move these strbuf_release() into an if/else
> block.
>
> We'll also add to "reflog_msg" in the "else" arm of the "if" block
> being modified here, and we'll append to "branch_msg" in both
> cases. But after f9500261e0a only the "if" block would free these two
> "struct strbuf".
>
> This change is needed so that a subsequent addition of a "goto
> cleanup" pattern when adding release_revisions() doesn't solve this
> unrelated bug while it's at it.

With or without the goto-cleanup change, this fix alone is the right fix for these leaks. I would not have written the last paragraph if I were working on this topic ;-)

Show 27 quoted lines

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/helper/test-fast-rebase.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c
> index fc2d4609043..993b90eaedd 100644
> --- a/t/helper/test-fast-rebase.c
> +++ b/t/helper/test-fast-rebase.c
> @@ -201,8 +201,6 @@ int cmd__fast_rebase(int argc, const char **argv)
>  		}
>  		if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0)
>  			die(_("unable to update HEAD"));
> -		strbuf_release(&reflog_msg);
> -		strbuf_release(&branch_name);
>  
>  		prime_cache_tree(the_repository, the_repository->index,
>  				 result.tree);
> @@ -221,5 +219,8 @@ int cmd__fast_rebase(int argc, const char **argv)
>  	if (write_locked_index(&the_index, &lock,
>  			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
>  		die(_("unable to write %s"), get_index_file());
> +
> +	strbuf_release(&reflog_msg);
> +	strbuf_release(&branch_name);
>  	return (result.clean == 0);
>  }